-
Notifications
You must be signed in to change notification settings - Fork 71
feat: add http post generic auth #276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a new HTTP authentication backend that allows users to authenticate against a generic HTTP endpoint via POST requests. The implementation adds support for configurable password hashing and access key authentication.
Key changes:
- Adds a new HTTP authentication class that sends POST requests to validate credentials
- Includes comprehensive test coverage for the new authentication method
- Updates documentation to describe the HTTP authentication configuration and usage
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/HTTP.php | Implements the core HTTP authentication backend with POST request handling |
| tests/http.php | Adds unit tests for the HTTP authentication functionality |
| tests/config.php | Extends test configuration to include HTTP backend settings |
| appinfo/info.xml | Updates app summary to include HTTP Generic authentication method |
| README.md | Adds comprehensive documentation for HTTP authentication configuration |
[skip-ci] Signed-off-by: Joas Schilling <coding@schilljs.com> Signed-off-by: Sebastian Sterk <7263970+sebastiansterk@users.noreply.github.com>
Bumps [symfony/process](https://github.com/symfony/process) from 5.4.7 to 5.4.46. - [Release notes](https://github.com/symfony/process/releases) - [Changelog](https://github.com/symfony/process/blob/7.1/CHANGELOG.md) - [Commits](symfony/process@v5.4.7...v5.4.46) --- updated-dependencies: - dependency-name: symfony/process dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Sebastian Sterk <7263970+sebastiansterk@users.noreply.github.com>
Make the app compatible with Nextcloud 30 Signed-off-by: Ralf <ervee@moskovic.org> Signed-off-by: Sebastian Sterk <7263970+sebastiansterk@users.noreply.github.com>
Signed-off-by: Sebastian Sterk <7263970+sebastiansterk@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Sebastian Sterk <7263970+sebastiansterk@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| class Test_User_HTTP extends \Test\TestCase { | ||
| /** | ||
| * @var OC_User_HTTP $instance |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PHPDoc type should be \OCA\UserExternal\HTTP instead of OC_User_HTTP. The OC_User_* naming appears to be used only in tests for backward compatibility, but the actual class uses the namespaced format.
| * @var OC_User_HTTP $instance | |
| * @var \OCA\UserExternal\HTTP $instance |
| if ($statusCode === 202) { | ||
| return true; | ||
| } else { | ||
| return false; | ||
| } |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This if-else block can be simplified to return $statusCode === 202; for better readability and conciseness.
| if ($statusCode === 202) { | |
| return true; | |
| } else { | |
| return false; | |
| } | |
| return $statusCode === 202; |
| $response = $client->post($this->authenticationEndpoint, [ | ||
| 'form_params' => [ | ||
| 'accessKey' => $this->accessKey, | ||
| 'user' => $user, | ||
| 'password' => $password | ||
| ], |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The accessKey parameter is always included in the POST request even when it's empty (default value is ''). Consider only including it in the form_params when it's non-empty to avoid sending unnecessary data. This would make the integration cleaner for endpoints that don't require an access key.
| $response = $client->post($this->authenticationEndpoint, [ | |
| 'form_params' => [ | |
| 'accessKey' => $this->accessKey, | |
| 'user' => $user, | |
| 'password' => $password | |
| ], | |
| $formParams = [ | |
| 'user' => $user, | |
| 'password' => $password | |
| ]; | |
| if ($this->accessKey !== '') { | |
| $formParams['accessKey'] = $this->accessKey; | |
| } | |
| $response = $client->post($this->authenticationEndpoint, [ | |
| 'form_params' => $formParams, |
| private function hashPassword($password) { | ||
| return hash($this->hashAlgo, $password); |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hash() function should be validated to ensure $this->hashAlgo is a valid algorithm. Consider adding validation in the constructor (e.g., in_array($hashAlgo, hash_algos())) to fail early with a clear error message if an invalid algorithm is provided.
|
|
||
| try { | ||
| $client = $this->httpClientService->newClient(); | ||
|
|
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove trailing whitespace on this line.
| ]); | ||
|
|
||
| $statusCode = $response->getStatusCode(); | ||
|
|
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove trailing whitespace on this line.
| return false; | ||
| } | ||
| } catch (\Exception $e) { | ||
| \OC::$server->getLogger()->error( |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use $this->logger instead of \OC::$server->getLogger(). The Base class provides a logger property that should be used for consistency with other authentication backends in this codebase (e.g., FTP, BasicAuth, IMAP).
| \OC::$server->getLogger()->error( | |
| $this->logger->error( |
| return hash($this->hashAlgo, $password); | ||
| } |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passwords are hashed client-side using hash($this->hashAlgo, $password) with user-provided algorithms (e.g., md5, sha1). Weak algorithms like MD5/SHA1 are unsuitable for password protection and enable credential disclosure if intercepted or reused, and client-side hashing lacks proper salting/work factors. Fix by removing client-side password hashing and rely on TLS to protect credentials in transit, or strictly enforce a strong algorithm (e.g., sha256) with HMAC and server-side verification; do not allow md5/sha1 for passwords.
| 'arguments' => array('https://example.com/auth_endpoint', 'md5'), | ||
| ), |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation recommends using weak hash algorithms for passwords (e.g., md5, sha1) in arguments for the HTTP backend. This encourages insecure configurations and could lead to credential compromise. Fix by removing weak algorithms from examples and explicitly recommending no client-side hashing (rely on HTTPS), or only strong algorithms (e.g., sha256) with proper server-side handling.
Implements #104