-
Notifications
You must be signed in to change notification settings - Fork 128
Add support for client credentials grant #269
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: main
Are you sure you want to change the base?
Changes from all commits
d101d40
39176cb
c9da2bc
7610eee
628c31f
b46496a
9ebb336
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,241 @@ | ||||||||||||||||||||||
| <?php | ||||||||||||||||||||||
|
||||||||||||||||||||||
| <?php | |
| <?php | |
| /* | |
| * @copyright 2014 Mautic, Inc. | |
| * @author Mautic | |
| * @link https://www.mautic.org | |
| * | |
| * @license GNU/GPLv3 http://www.gnu.org/licenses/gpl-3.0.html | |
| */ |
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.
| declare(strict_types=1); | |
Copilot
AI
Jan 12, 2026
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 @internal annotation suggests this class should not be used by external code, but it's documented in the README.md as a public authentication method for users to adopt. This is inconsistent with the other Auth classes (OAuth, BasicAuth) which don't have @internal annotations. Consider removing @internal if this is meant to be a public API, or add it to all Auth classes if they should all be considered internal.
| * @internal OAuth Client modified from https://code.google.com/p/simple-php-oauth/. | |
| * OAuth Client modified from https://code.google.com/p/simple-php-oauth/. |
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.
Do you think we could make this class final and add real property, param and return types to avoid BC breaks when doing this later on?
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.
I personally absolutely despise final. If a developer using this package wants to make a small tweak to this class for a specific setup, they would have to copy the whole thing. If that is what we want to force, I can add it. But there was an issue I was working on in Mautic that could have been very easily and cleanly solved if a certain Symfony class did not have final set.
Typehinting fo sho though.
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.
Agree with @nick-vanpraet
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.
Inheritance creates a multi-layered mess. A good example are Mautic controllers and models.
I suggest to google "composition over inheritance" to get good examples why composition is way better practice.
If the architecture is not allowing to change a class then Symfony allows you to decorate the service which is not the point here but it is for Mautic.
Plus, a final class is easier to maintain for a library like this one as developers don't have to think about all the ways how users could have inherited the class and what it could break for them.
I'm not going to block this with these suggestions PR if it gets a second approval as I'm not using this library.
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.
Sure but IIRC I didn't want to replace anything I just wanted to use the existing logic for something instead of having to re-invent the wheel, just with a minor tweak.
Anyway I don't think it's up to package developers to care if someone else inherits their class. They either do or they don't, we're not their mother. Add an @internal annotation and let them decide if they want to risk it.
Speaking of, I'll add an @internal annotation at least.
Copilot
AI
Jan 12, 2026
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 @return annotation is redundant since the method already has a return type declaration. The docblock return type annotation can be removed to reduce redundancy and follow modern PHP practices.
Copilot
AI
Jan 12, 2026
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 getQueryParameters override appends the OAuth2 access_token into the URL query string for file uploads, which exposes bearer tokens in request URLs and therefore in web server/proxy logs and referrers, making them much easier to leak or exfiltrate. An attacker with access to logs or intermediaries that record URLs could steal these tokens and impersonate the client against the Mautic API. Instead of placing access_token in the query string, ensure authentication is conveyed via the Authorization header (or another non-logged channel) and, if the server truly cannot handle multipart auth headers, consider fixing the server behavior or using a dedicated endpoint that does not log full URLs.
| $query = parent::getQueryParameters($isPost, $parameters); | |
| if (isset($parameters['file'])) { | |
| // Mautic's OAuth2 server does not recognize multipart forms so we have to append the access token as part of the URL | |
| $query['access_token'] = $parameters['access_token']; | |
| } | |
| return $query; | |
| // Delegate to parent without appending access tokens to the query string | |
| return parent::getQueryParameters($isPost, $parameters); |
Copilot
AI
Jan 12, 2026
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 @param annotations are incorrect. The parameter $method is documented as type 'array' but is actually a string, and $url has type string but is not type-hinted in the signature. Consider correcting the @param annotations to match the actual method signature or adding proper type hints to the method parameters.
| * @param array $method | |
| * @param array $headers | |
| * @param array $parameters | |
| * @param string $method | |
| * @param array $settings |
Copilot
AI
Jan 12, 2026
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 code accesses $params['token_type'] directly without checking if it exists first, which could result in an undefined array key warning. This is inconsistent with line 206 where the same key is accessed using a ternary operator with an isset check. Consider using the same pattern here for consistency and to avoid warnings.
| $_SESSION['oauth']['debug']['tokens']['token_type'] = $params['token_type']; | |
| $_SESSION['oauth']['debug']['tokens']['token_type'] = isset($params['token_type']) ? $params['token_type'] : null; |
Copilot
AI
Jan 12, 2026
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.
This new authentication class lacks test coverage. Consider adding tests similar to BasicAuthTest.php that verify parameter validation, setup method error handling, and basic authorization flow. At minimum, tests should cover missing client_id, missing client_secret, missing baseUrl, and successful token request scenarios.
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.
These variables are defined but never used in the example. They should be removed since they don't apply to the Two-Legged OAuth2 flow (Client Credentials grant), which doesn't require a callback URL or use the same key naming conventions.