Add support for client credentials grant

[nick-vanpraet]

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).

[codecov-commenter]

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]

Don't understand create duplicate PR of #257

[RCheesley]

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

[kuzmany]

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]

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]

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]

@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]

@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]

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]

@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!

[linuxd3v]

I just checked and, it's been 999 days (April 7, 2022 - December 31, 2024) since original PR creator nick-vanpraet last commented.
I mean, I strongly suspect they maybe (oh boy, how can I say this politely) - no longer interested in this feature as it has been (*checks watch) - a long long *** time.
so what is mautic policy here - someone has to create a fresh PR?

[escopecz]

@linuxd3v yes, someone needs to take over and see the change being up with the project standards.

[nick-vanpraet]

I think mails are turned off for this repo or something. Anyway: rebased, implemented strlen/empty changes and fixed phpcs.

[escopecz]

@nick-vanpraet thank you sir!

@linuxd3v can you please give it a review and a test before the merge?

[abhisekmazumdar]

I tested this. It works as expected. This will be a great addition in the next release.

[RCheesley]

@escopecz are we good to merge this with the successful test above? It'd be great to make the release, perhaps in time with 7.0?

[escopecz]

Suggested change

	declare(strict_types=1);

[escopecz]

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?

[nick-vanpraet]

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.

[kuzmany]

Agree with @nick-vanpraet

[escopecz]

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.

[nick-vanpraet]

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.