rhertogh commented 3 years ago

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 commented 3 years ago

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

rhertogh commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

davidwindell commented 3 years ago

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

rhertogh commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

codecov[bot] commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@ramsey please merge this one!

rhertogh commented 2 years ago

@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 commented 2 years ago

@ramsey Suggestions have been committed.

ramsey commented 2 years ago

Thank you for contributing! 🎉

websmurf commented 2 years ago

Awesome work, thanks!

gsirin commented 2 years ago

Thank you!

cdburgess commented 2 years ago

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 commented 2 years ago

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

cdburgess commented 2 years ago

Thanks!

isaiahdahl commented 2 years ago

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

CleanShot 2022-10-11 at 12 44 03

cdburgess commented 2 years ago

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

rhertogh commented 2 years ago

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