Add support of PKCE

[thaarok]

This is built on #951, but missing parts was added:

• Updated Storage classes to support new code_challenge/code_challenge_method fields
• Added support of PKCI even into non-openid Oauth2
• Tested using modified oauth2-demo-php: https://github.com/hkalina/oauth2-demo-php/tree/pkce

Any feedback is welcome!

[bshaffer]

This is a great PR! Thank you for adding PKCE support! Just some minor formatting changes and we can merge this.

I'll also add tests in another PR unless you'd like to add those as well

[thaarok]

@bshaffer Updated, thank you! :)

[jasverix]

Should probably mark this upgrade somehow, because it breaks the system if the developers does not apply a migration. I don't have a good suggestion on how you could have marked it, but when I first looked at the upgrade log, I did not detect that it was a breaking upgrade.

I will of course just do the migration and continue to use this package, but I just wanted to give this feedback. It is a very well working and good package.

[bshaffer]

I did not realize this was a breaking change. I will revert it. Can you post the migration you used to fix the issue?

[bshaffer]

This also SHOULDNT be a breaking change, it should be opt-in. Can you post the error you received as well?

[jasverix]

I can replicate the exact error message if you want. But it was something like 'column code_challenge does not exist' in the INSERT INTO query. See my comments from the code as well.

[bshaffer]

Great. I figured it was something like that. That shouldn’t be too hard to fix then.

[Wirone]

This is a breaking change because of change in AuthorizationCodeInterface::setAuthorizationCode() signature. Any class implementing this causes fatal error now. In our case Renovate Bot tried to upgrade this package, but fortunately our tests failed, so it wasn't merged.

@bshaffer what's the plan for it? Consider using PHP SemVer Checker 🙂.

[Wirone]

@bshaffer what's the situation? We have internal MR with changes aligning our code with this change, but we don't know if we should merge it because if revert is done, we also would need to revert on our side 🙈. Please let us know.

[bshaffer]

I am sorry, I am not really maintaining this library more than merging PRs. If you have a fix you'd like to submit I'm happy to review it.

[Wirone]

@bshaffer I was just referring to what you said:

I will revert it

But it never happened 😉. Since it's a change in the interface, there's no way to refactor it in non-breaking way (moving those 2 added arguments to separate method would be breaking as well). I believe it's a change worth keeping, so what should be done IMHO:

• revert this change and tag 1.14.1: people who did not upgrade yet would have clean upgrade path, without breaking change
• bring back this change and tag 2.0.0: people who already upgraded to 1.14.0 would have another BC break, but they should only bump to 2.0 and continue with their current code (that's why there shouldn't be other BC-breaks in 2.0, to be compatible with what people already did).

We just need clear information what's the plan about this, if it's going to be kept as-is, we'll just merge changes on our side and continue with this BC-break. If you're going to revert it, we won't change implementation on our side 🙂.

[bshaffer]

IMHO adding two empty methods to whatever custom classes extend the interface in a dependent library is not too much to ask. I understand that this breaks semver, but at this point the damage is done, and someone can simply pin to the previous version if they don't want the breaking change.

[rdamas]

The documentation here: https://bshaffer.github.io/oauth2-server-php-docs/cookbook/ should probably be updated to reflect the database change, the columns code_challenge and code_challenge_method are not mentioned yet.

[aarangara]

Is this (PKCE) only supposed to work with OpenID - because currently it does not seem to work for me when use_openid_connect = false.

Or any reason PCKE authorization cannot be used without use_openid_connect?

For e.g. The Oauth2\Controller\AuthorizeController does not have any code that set-ups the code_challenge and code_challenge_variable. The only change this MR had to that file is 'enforce_pkce' => false

But if you check Oauth2\OpenID\Controller\AuthorizeController, it has got some code in validateRequest does some checks for PKCE.

[campbsb]

The documentation here: https://bshaffer.github.io/oauth2-server-php-docs/cookbook/ should probably be updated to reflect the database change, the columns code_challenge and code_challenge_method are not mentioned yet.

+1

I've had to add best-guess definitions for these undocumented code_challenge and code_challenge_method fields.

[bshaffer]

Thanks for the nudge, I will update it

[bshaffer]

Updated!

bshaffer/oauth2-server-php-docs@ae2916d

[ArnoHolo]

I suggest you indicate it as a Breaking Change in the Release of the 1.14.0 version => https://github.com/bshaffer/oauth2-server-php/releases/tag/v1.14.0

[bshaffer]

@ArnoHolo done