Add support for PKCE (Proof Key for Code Exchange [RFC 7636])

[rhertogh]

Support RFC 7636: Proof Key for Code Exchange. For more info please see https://oauth.net/2/pkce/

• [x] Code
• [x] Tests
• [x] Documentation

Fixes: #837

[D063520]

Hi, this is great! Could you provide some documentation too, then I would try it out ....

[rhertogh]

Hi, this is great! Could you provide some documentation too, then I would try it out ....

To enable PKCE set the pkceMethod to 'S256' or 'plain' (Note: plain is not recommended)

$provider = GenericProvider([
    // ...
    'pkceMethod' => 'S256',
    // ...
);

[jcomack]

@rhertogh Am I correct in my assumption that this will not work when using the GenericProvider and one will have to roll their own version that implements the AbstractProvider?

[rhertogh]

@jcomack

Am I correct in my assumption that this will not work when using the GenericProvider and one will have to roll their own version that implements the AbstractProvider?

No, the example I gave was unclear (the ClientTokenProvider mentioned in the older version of the example was a custom class I used to extend from the GenericProvider). The example is updated. So to be clear, you can use the GenericProvider.

[davidwindell]

This would be really helpful for Xero PKCE, thanks @rhertogh. Hopefully we'll see this merged soon :pray:

[davidwindell]

@rhertogh what is the reason getPkceMethod() returns null in the AbstractProvider?

[rhertogh]

@davidwindell

what is the reason getPkceMethod() returns null in the AbstractProvider?

Not all grant types support PKCE (actually only authorization-code supports it). Therefore it's disabled by default (PKCE is not send when the method is null).

[davidwindell]

Thanks, I suppose we need to get the provider we are using (https://github.com/calcinai/oauth2-xero/blob/master/src/Provider/Xero.php) to add getPkceMethod() then?

I've tested and this all works well for us. The only gotcha was realising the PKCE code needs to be stored so it can be returned afterwards, we did this like so:

$_SESSION['oauth2code']  = $provider->getPkceCode();

...

$token = $provider->getAccessToken('authorization_code', [
    'code' => $_GET['code'],
    'code_verifier' => $_SESSION['oauth2code']
]);

[rhertogh]

@davidwindell

... I've tested and this all works well for us. The only gotcha was realising the PKCE code needs to be stored so it can be returned afterwards, ...

Thanks for your feedback. This is indeed a necessary step, I've added the setPkceCode() method to aid in the process. The documentation is updated accordingly.

[rhertogh]

@shadowhand Could you approve running workflows on this PR to validate the tests.

[codecov[bot]]

Codecov Report

Merging #901 (f4d27c7) into master (8c7498c) will not change coverage. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##              master      #901   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       180       190   +10     
===========================================
  Files             20        20           
  Lines            441       442    +1     
===========================================
+ Hits             441       442    +1     

Impacted Files	Coverage Δ
src/Provider/AbstractProvider.php	100.00% <100.00%> (ø)
src/Provider/GenericProvider.php	100.00% <100.00%> (ø)
src/Token/AccessToken.php	100.00% <0.00%> (ø)
src/Grant/GrantFactory.php	100.00% <0.00%> (ø)
src/Tool/RequestFactory.php	100.00% <0.00%> (ø)
src/Tool/GuardedPropertyTrait.php	100.00% <0.00%> (ø)
src/Tool/ProviderRedirectTrait.php	100.00% <0.00%> (ø)
src/Tool/RequiredParameterTrait.php	100.00% <0.00%> (ø)
... and 2 more

[gdsmith]

Any movement on this? Looks like a good improvement, especially as Oauth are saying PKCE is recommended for any Authorisation Code flow now.

PKCE was originally designed to protect the authorization code flow in mobile apps, but its ability to prevent authorization code injection makes it useful for every type of OAuth client, even web apps that use a client secret.

[websmurf]

What needs to be done in order for this pull request to be merged? We're using the @rhertogh fork for some time now; but would like to be able to use the main package.

[gsirin]

@ramsey please merge this one!

[rhertogh]

@ramsey I've added tests for the missing code coverage parts (should be 100% now). Could you trigger a build to see the results?

[rhertogh]

@ramsey Suggestions have been committed.

[ramsey]

Thank you for contributing! 🎉

[websmurf]

Awesome work, thanks!

[gsirin]

Thank you!

[cdburgess]

When will this code show up in an official release? I see 2.6.1 is the latest that was released last December. Is there a better way to get this code to start working with PKCE requirements?

[ramsey]

I will try to tag a release in the next week.

[cdburgess]

Thanks!

[isaiahdahl]

@ramsey is this released yet? I see PKCE support in the docs but it doesn't seem to have trace of that support in the installed package.

[cdburgess]

I'm still waiting for it too. It hasn't been released yet. At least not in an actual version release.

[rhertogh]

@isaiahdahl, @cdburgess Until the new version is released you can use "league/oauth2-client": "dev-master#43c59dd" in your composer.json file. Just make sure to change it to the correct version when it is released.

[oddevan]

Great to see this! I'll update my Twitter provider with this once it's released.

[ChrisTitos]

Any update on releasing this feature?

[jakub-groncki]

Any chances of releasing this soon? Apparently it's a blocker for many developers.

[gsirin]

@ramsey please I need this one too

[skollro]

Waiting for the new release, too.