openid-connect-generic
openid-connect-generic copied to clipboard
oidc-wp
PKCE support
All Submissions:
- [x] Have you followed the plugin Contributing guideline?
- [x] Does your code follow the WordPress' coding standards?
- [x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Changes proposed in this Pull Request:
Add support for PKCE.
Closes #208 .
How to test the changes in this Pull Request:
- Ensure the OAuth client has PKCE support enable/enforce
- Configure the plugin with the required client informations and check the "Enable PKCE support" setting
- Log in using OpenID Connect button on the login page
- User should be logged in successfuly
Other information:
- [x] Have you added an explanation of what your changes do and why you'd like us to include them?
- [ ] Have you written new tests for your changes, as applicable?
- [ ] Have you successfully run tests with your changes locally?
Changelog entry
Add support for PKCE
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 :
The PKCE extension does not add any new responses, so clients can always use the PKCE extension even if an authorization server does not support it.
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?
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.
@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!
@timnolte I refreshed the branch to resolve the conflict.
I was looking to add tests, but I'm not sure what's the best way to do that. I'm trying to dynamically activate the PKCE option for my tests, but I didn't find a way of changing the plugin's settings after it has initialized. It's possible to use the phpunit.xml.dist file to set constant and control that option, but it'll be active for all the tests. Would that be ok ?
@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 I started working on some tests for the features in https://github.com/oidc-wp/openid-connect-generic/pull/421/commits/550e0aa2190daf9b11efc706282b4abba864f60f. I went with the "initialize a new instance" road. The tests cover the methods related to the PKCE. Let me know what you think.
just a heads up that it looks like there is a coding issue with a filter flagged by PHPStan.
Thanks ! I fix it in https://github.com/oidc-wp/openid-connect-generic/pull/421/commits/c3ea74e8329febba423fe753045a8c254b6b2d1b
@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!