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 127 commits behind head on main.

Files with missing lines 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
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
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.

@RCheesley
Copy link
Member

@nick-vanpraet could you take a look at the failing tests for this PR please? Appreciate it was a while back, it'd be great to get it tested and merged!

@escopecz and @kuzmany we do still need to decide to merge this in favour of the older PR. It seems sensible to do so given the outlined additional functionality. Let's make a decision and get it merged?

@kuzmany
Copy link
Member

kuzmany commented Sep 3, 2024

@RCheesley we use #257 for our customers. It's from 2021 then I am not able to say what community need to merge. I see some Dennis comments, but cannot spend more time on it, only for maintenance to push it to branch. It's up to community. As usual I prefer less work for done.

@ctotech-pl
Copy link

ctotech-pl commented Nov 11, 2024

Tested PR: #269 in my build server and it is working.

Just 2x NOTICES need to be fixed:

[Notice] Message: strlen(): Passing null to parameter #1 ($string) of type string is deprecated File: /vendor/mautic/api-library/lib/Auth/TwoLeggedOAuth2.php Line:158
[Notice] Message: strlen(): Passing null to parameter #1 ($string) of type string is deprecated File: /vendor/mautic/api-library/lib/Auth/TwoLeggedOAuth2.php Line:165

File: mautic\api-library\lib\Auth\TwoLeggedOAuth2.php

  • at lines 158 and 165, change to use !empty( ... ) instead of strlen( ... )
public function validateAccessToken()
{
    $this->log('validateAccessToken()');

    //Check to see if token in session has expired
    if ( !empty($this->_access_token) && !empty($this->_expires) && $this->_expires < (time() + 10)) {
        $this->log('access token expired');

        return false;
    }

    //Check for existing access token
    if ( !empty($this->_access_token) ) {
        $this->log('has valid access token');

        return true;
    }

    //If there is no existing access token, it can't be valid
    return false;
}

This is a low risk merge:

  1. Introduces a new file to extend available functionality.
  2. Does not change or effect existing functionality or behavior.
  3. Adheres to abstract and follows code standards and exception handling.
  4. Improves README.MD with new documentation.

I vote to merge and released PR: #269 immediately, as it is a critical missing feature with low-impact.

@RCheesley RCheesley closed this Nov 12, 2024
@RCheesley RCheesley reopened this Nov 12, 2024
@RCheesley
Copy link
Member

RCheesley commented Nov 12, 2024

@nick-vanpraet looks like there's some code style fixes to be done here, would you take a look please?

We also need to have the test coverage increased as you can see from CodeCov. Let us know if you need some help with that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🥳 Done
Development

Successfully merging this pull request may close these issues.

5 participants