Skip to content
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

Add support for client credentials grant #269

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nick-vanpraet
Copy link

PR to replace #257 as that one seems abandoned.

Adds support for the client_credentials grant type added in M4 mautic/mautic#9837

I kept as much of the logic that was used in the Oauth Auth class in as I could (the debugging, the weird query parameter access token thing that I'm pretty sure is a security concern, etc).

@cla-bot cla-bot bot added the cla-signed label Apr 7, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2022

Codecov Report

Attention: Patch coverage is 0% with 72 lines in your changes missing coverage. Please review.

Project coverage is 48.09%. Comparing base (7478f55) to head (c445d71).
Report is 111 commits behind head on main.

Files Patch % Lines
lib/Auth/TwoLeggedOAuth2.php 0.00% 72 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #269      +/-   ##
============================================
- Coverage     51.45%   48.09%   -3.37%     
- Complexity      406      434      +28     
============================================
  Files            30       31       +1     
  Lines          1028     1100      +72     
============================================
  Hits            529      529              
- Misses          499      571      +72     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kuzmany
Copy link
Member

kuzmany commented Apr 7, 2022

Don't understand create duplicate PR of #257

@RCheesley
Copy link
Sponsor Member

@kuzmany guessing because there has been no response to feedback since September 2021!

@kuzmany
Copy link
Member

kuzmany commented Apr 7, 2022

Ok. But still would be good mention what is different between my version and this version.
We use that PR on production, works as expected and still don't see reason create new PR

@nick-vanpraet
Copy link
Author

Sure thing.

  • Requires baseUrl to be given
  • Implements accessTokenUpdated (for those who want it, gets set to true in requestAccessToken if nothing failed)
  • Implements getAccessTokenData method to return token, expires and token type
  • Checks the access token's expiry and presence in isAuthorized/validateAccessToken so code can re-use access tokens more easily
  • Implements getQueryParameters, straight copy from Oauth class. I imagine it's there to fix a bug, based on the comment above it.
  • Supports $this->_debug variable the same way Oauth class does

@RCheesley
Copy link
Sponsor Member

It would be great to get a decision made on this @escopecz @kuzmany and merge it if we want to accept it in favour of the outdated PR based on the feedback provided above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🧑🏻‍💻 Needs a code review
Development

Successfully merging this pull request may close these issues.

None yet

4 participants