PKCE support #421
PKCE support #421
Conversation
|
Currently the feature is disable by default, but looking at https://www.oauth.com/oauth2-servers/pkce/authorization-code-exchange/ it seem PKCE could be on by default if we wanted :
|
|
This should also close #399 |
|
The openid server that we depend on now requires PKCE by default, and we use this plugin on one of our clients. When is this likely to be merged? |
* Merged PR that adds [PKCE support](oidc-wp/openid-connect-generic#421) * Integrated Hellō Quickstart * Removed unnecessary configuration options * Renamed all relevant identifiers to be Hellō Login specific Co-authored-by: Clement Boirie <[email protected]> Co-authored-by: Dick Hardt <[email protected]>
|
Hi! what is the current status of merge? Also is it possible to add the option for the ES384 code challenge method? |
|
This came to mind in the last week but I've been swamped with other life/project things. I will look at seeing about getting this merged and in a release in the next couple of weeks. |
|
Bump |
|
@petitphp I know it's been some time but I finally have some capacity to work on this some more, additionally I've begun adding unit testing into the plugin in order to help with on-going maintenance. Is there any chance you'd be able to resolve the current conflicts and write some unit tests for your work and then I'll get this merged in. I'd like you to get the commit history credit for the work. If not I will work on recreating your work in a new branch. Thanks! |
petitphp
force-pushed
the
feature/pkce-support
branch
from
8c2fcc8
to
eff0391
Compare
|
@timnolte I refreshed the branch to resolve the conflict. |
|
@petitphp so, when I just added some logging tests there was a bit of refactoring of code to make things testable so if we need to look at doing some refactoring in order to rely on dependency injection to make the code testable I'm on board with that. I think however that you should be able to initialize an instance of the plugin class with custom settings injected which might be the route to go without more major refactoring. |
|
@petitphp just a heads up that it looks like there is a coding issue with a filter flagged by PHPStan. |
timnolte
added
status: blocked
Add new setting to enable/disable PKCE feature. A new constant OIDC_ENABLE_PKCE is available to force the setting's value.
Update new state creation method to take an additional parameter with the PKCE code verifier 's value and store it in the state value.
The method will try to generate a code verifier (a random ASCII string) and a code challenge (SHA256 hash of the verifier) and return an array with them and the method use to create the code challenge. If the code verifier generation fails the method will return false.
This is the first step when integrating PKCE into the authentication workflow. When building the authentication URL a new code verifier and challenge are created, the code verifier is store in the state to be accessible at a later stage and the challenge is added as a query param to the URL along side the method use to generate the challenge from the verifier.
This is the second step when integrating PKCE into the authentication workflow. Add the code verifier to the auth token request's body. Code verifier is retieved from the state object created when building the authentication URL.
petitphp
force-pushed
the
feature/pkce-support
branch
from
a189ef5
to
c3ea74e
Compare
|
@timnolte I started working on some tests for the features in 550e0aa. I went with the "initialize a new instance" road. The tests cover the methods related to the PKCE. Let me know what you think.
Thanks ! I fix it in c3ea74e |
|
@petitphp what are your thoughts about refactoring your code to leverage this package? https://packagist.org/packages/jumbojett/openid-connect-php I'm thinking about beginning to refactor the plugin's code to leverage this package, which could also give us Back Channel Logout, among other things. Thanks! |
All Submissions:
Changes proposed in this Pull Request:
Add support for PKCE.
Closes #208 .
How to test the changes in this Pull Request:
Other information:
Changelog entry