fix: Make OAuth2PKCEClient work with latest league/oauth2-client release

[nclavaud]

Closes #406

TL;DR

Make the OAuth2PKCEClient class work out-of-the-box with league/oauth2-client AbstractProvider: let the provider generate the PKCE code, store it in session, and re-use it to exchange the authorization code with an access token.

Why?

While trying to implement a new provider, I could not make this bundle work with the GenericProvider provided by league/oauth2-client. The access token could not be retrieved, because two different code challenges were generated: in this bundle, and in the AbstractProvider.

How?

This pull request removes the code challenge generation from the client class, and instead retrieves and stores the code challenge generated by the underlying provider. I feel like this is a much cleaner approach, as it removes duplicated code between the client and the provider.

This also requires bumping the dependency of league/oauth2-client to 2.7 minimum (where PKCE support was added).

Notes

On a side note, if things should go this way, I am not sure this dedicated PKCE client is needed. This could be implemented directly in the OAuth2Client.

What do you think?

[bocharsky-bw]

Don't we want to add PRCE to the options?

[bocharsky-bw]

I suppose this is the new method upstream that was not available on the moment when this client was written.

UPD: Actually, I see it was released on the latest 2.7 of league/oauth2-client, right? So, we literally pushed this dependency to the latest here. But it might be ok, at least tests are happy about it

[bocharsky-bw]

Seems league/oauth2-client added that feature in the latest release, so this PR now makes sense to me now

[nclavaud]

@bocharsky-bw Yes, I did more testing and I get it now: the current implementation (before this PR) works well with league/oauth2-client < 2.7, but 2.7 made it break. This PR works well with 2.7 only (not below).

I have explicitely changed the requirements in composer.json in another commit.

I am not sure how this could be released. Maybe release a patch version with a stricter version constraint league/oauth2-client <2.7 first, then release this PR in a minor update?

[nclavaud]

the current implementation (before this PR) works well with league/oauth2-client < 2.7, but 2.7 made it break

Actually I realize this is not always true: everything works well (this bundle v2.15.0 + league/oauth2-client 2.7) if the pkceMethod parameter of the provider is not set. If set, then things go wrong (the code_challenge generated by AbstractProvider conflicts with the one generated by this bundle).

[bocharsky-bw]

So this PR sounds like a fix for those who are using PKCE. I personally don't use it, but changes look reasonable to me

[nclavaud]

Actually I'm worried that this PR might break things for people currently using PKCE at the client level. Obviously, they did not enable PKCE at the provider level, otherwise it would conflict and it wouldn't work.

If this PR passes as is, without users enabling the PKCE at the provider level, things will break with message Unable to fetch token from OAuth2 server because there is no PKCE code verifier stored in the session.

I think this PR is a good move forward, but this would be a breaking change: it would require changing the provider configuration as well.

[bocharsky-bw]

Hey Nicolas, thank you for heading it up! Yeah, I see, it might break things I think... Hm, we either should release a major version, or try to find a legacy way for now to avoid BC breaks, e.g. with some legacy code that we will be able to drop on the next major release (but it won't require us to do release major immediately for this PR).

Do you see any way to avoid such breaks with some legacy code? Like probably checks if users use that PKCE at the client level? not sure if it would help...

[nclavaud]

Hey Victor, I have been wrapping my head around and this is what I suggest:

• we should close this PR
• I can open another PR to update the documentation/readme to make it clear how to use PKCE with the current version (v2.15), i.e. not enabling PKCE at the provider level, but at the client level with OAuth2PKCEClient (this step alone would solve How is OAuth2PKCEClient supposed to work? #406)
• I am keen to rework the changes suggested here to add PKCE-support in OAuthClient and deprecate OAuth2PKCEClient, this could pave the way to another major version without breaking things for current users.

[bocharsky-bw]

Hey Nicolas! How about adding some legacy code in OAuth2PKCEClient, e.g. we could check if getPkceCode() method exists on that $this->getOAuth2Provider() (we could leverage method_exists() PHP function)... and if it exists - do not generate the code ourselves, instead get it from that getPkceCode() like you already did in this PR. But if that method does not exist - generate the code ourselves like we're already doing in the bundle. We probably could extract this all code into a separate getPkceCode() private method on OAuth2PKCEClient, and we can log the deprecation method if the PKCE code was generated by our legacy code.

From the technical point of view, I would lean toward keeping that PKCE implementation in a separate OAuth2PKCEClient class (as it's done already), and unless I miss something important this should be enough to make it work, and we will be able to drop that legacy code in the future major version later but still keeping the OAuth2PKCEClient.

Also, I don't see any mentions of the PKCE implementation in the bundle's docs, so adding some docs/examples is warmly welcome! Especially if it will close an issue like #406

[nclavaud]

How about adding some legacy code in OAuth2PKCEClient, e.g. we could check if getPkceCode() method exists [...] and if it exists - do not generate the code ourselves, instead get it from that getPkceCode()

This could work indeed. We would need to check if the method exists, and also that PKCE was enabled on the provider (i.e. getPkceCode() does not return null).

From the technical point of view, I would lean toward keeping that PKCE implementation in a separate OAuth2PKCEClient class (as it's done already), and unless I miss something important this should be enough to make it work

My point about dropping this OAuth2PKCEClient class is that it could simplify the configuration. Users could edit the provider_options with pkceMethod: S256 and they are done.

If you keep the dedicated client, users will have to 1. edit the provider_options to enable PKCE and 2. change the client_class to use OAuth2PKCEClient instead of OAuth2Client.

I get your point about separating concerns, though. Maybe a decorator would work?

Anyway, I'll start with adding some documentation to explain the current behavior :)

[bocharsky-bw]

I see, thanks for a more detailed explanation of it! I will try to attract someone's else opinion on this. Starting with some docs sounds perfect for me 👍

[weaverryan]

Hi to both of you! And thanks for taking this on - it looks important. Here are my 2 cents - please tell me if you disagree or if I'm missing some important points.

This could work indeed. We would need to check if the method exists, and also that PKCE was enabled on the provider (i.e. getPkceCode() does not return null).

This makes sense to me also. However, I'm also not against bumping to 2.7 (as this PR does) because even 2.6 is quite old now. I'm not sure if that helps. Regardless, we would need to make sure that updating THIS bundle a minor version + updating league/oauth2-client a minor version doesn't result in a BC break, which I think is what @nclavaud has been thinking a lot about. Or, if necessary, that we at least would have a "hard" error. Like, if someone upgrades to the next version of this bundle + oauth2-client 2.7 AND they are using OAuth2PKCEClient, then we throw a clear exception while the container is building (this is if it turns out that there is no good way to "make it work" without a break. In other words, if we can't make things work, then we can at least cause an error that can't be ignored with clear instructions).

My point about dropping this OAuth2PKCEClient class is that it could simplify the configuration. Users could edit the provider_options with pkceMethod: S256 and they are done.

This makes sense to me too. Tbh, having the separate class (which I merged) looks weird to me now. pkce is just a "feature" that OAuth2Client may or may not use during its workflow.

Overall, because the oauth2-client package now properly supports pkce, I would rather use THEIR implementation vs our own. The trick is how to do that without breaking BC. Or, if we need to break BC, that's not a huge deal either - we could release a v3 of this bundle.

Cheers!

[bocharsky-bw]

Hey @nclavaud , just a friend ping, what do you think about #407 (comment) ? Do you have some time to work on this?