-
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?
Changes from all commits
33ec3ed
1efd392
2aeb5a2
6fca339
2bce515
06d086b
9a755c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,115 @@ | ||||||||||||||||||||||||||||||||
| <?php | ||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||
| * Copyright (c) 2021 Sebastian Sterk <sebastian@wiuwiu.de> | ||||||||||||||||||||||||||||||||
| * This file is licensed under the Affero General Public License version 3 or | ||||||||||||||||||||||||||||||||
| * later. | ||||||||||||||||||||||||||||||||
| * See the COPYING-README file. | ||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| namespace OCA\UserExternal; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| use OCP\Http\Client\IClientService; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||
| * User authentication against a generic HTTP auth interface | ||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||
| * @category Apps | ||||||||||||||||||||||||||||||||
| * @package UserExternal | ||||||||||||||||||||||||||||||||
| * @author Sebastian Sterk https://wiuwiu.de/Imprint | ||||||||||||||||||||||||||||||||
| * @license http://www.gnu.org/licenses/agpl AGPL | ||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||
| class HTTP extends Base { | ||||||||||||||||||||||||||||||||
| private $hashAlgo; | ||||||||||||||||||||||||||||||||
| private $accessKey; | ||||||||||||||||||||||||||||||||
| private $authenticationEndpoint; | ||||||||||||||||||||||||||||||||
| private $httpClientService; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||
| * Create new HTTP authentication provider | ||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||
| * @param string $authenticationEndpoint The HTTP endpoint URL for authentication | ||||||||||||||||||||||||||||||||
| * @param string|false $hashAlgo Hash algorithm for password (false for plain text) | ||||||||||||||||||||||||||||||||
| * @param string $accessKey Access key for additional security | ||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||
| public function __construct($authenticationEndpoint, $hashAlgo = false, $accessKey = '') { | ||||||||||||||||||||||||||||||||
| parent::__construct($authenticationEndpoint); | ||||||||||||||||||||||||||||||||
| $this->authenticationEndpoint = $authenticationEndpoint; | ||||||||||||||||||||||||||||||||
| $this->hashAlgo = $hashAlgo; | ||||||||||||||||||||||||||||||||
| $this->accessKey = $accessKey; | ||||||||||||||||||||||||||||||||
| $this->httpClientService = \OC::$server->get(IClientService::class); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||
| * Send user credentials to HTTP endpoint | ||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||
| * @param string $user The username | ||||||||||||||||||||||||||||||||
| * @param string $password The password | ||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||
| * @return bool True if authentication successful, false otherwise | ||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||
| public function sendUserData($user, $password) { | ||||||||||||||||||||||||||||||||
| if ($this->hashAlgo !== false) { | ||||||||||||||||||||||||||||||||
| $password = $this->hashPassword($password); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||
| $client = $this->httpClientService->newClient(); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
sebastiansterk marked this conversation as resolved.
Show resolved
Hide resolved
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, |
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.
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; |
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( |
sebastiansterk marked this conversation as resolved.
Show resolved
Hide resolved
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.
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.
sebastiansterk marked this conversation as resolved.
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,40 @@ | ||||||
| <?php | ||||||
| /** | ||||||
| * Copyright (c) 2021 Sebastian Sterk <sebastian@wiuwiu.de> | ||||||
| * This file is licensed under the Affero General Public License version 3 or | ||||||
| * later. | ||||||
| * See the COPYING-README file. | ||||||
| */ | ||||||
|
|
||||||
| class Test_User_HTTP extends \Test\TestCase { | ||||||
| /** | ||||||
| * @var OC_User_HTTP $instance | ||||||
|
||||||
| * @var OC_User_HTTP $instance | |
| * @var \OCA\UserExternal\HTTP $instance |
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) inargumentsfor 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.