Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for OpenID Connect PKCE #951

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

darthsteven
Copy link

@darthsteven darthsteven commented Nov 20, 2018

Very much a WIP and not finished, just getting something out there and started.

Stay tuned!

Relates to #752

I will note that if some wants PCKE and can enforce PHP>7.0 then https://oauth2.thephpleague.com/ might be a better way to go.

@darthsteven
Copy link
Author

Note that I'm using this with a Drupal oauth2 server module:
https://www.drupal.org/project/oauth2_server/issues/3015065#comment-12863529
which basically swaps out the storage class.

I've not updated the built in storage classes, hence all the test failures.

@@ -84,6 +84,45 @@ public function validateRequest(RequestInterface $request, ResponseInterface $re
return false;
}

// @TODO: Should we enforce presence of a non-falsy code challenge?
if (isset($authCode['code_challenge']) && $authCode['code_challenge']) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not in the official spec but like other things in OAuth, providing an empty string should be considered the same as omitting the parameter. Maybe more pedantically:

Suggested change
if (isset($authCode['code_challenge']) && $authCode['code_challenge']) {
if (isset($authCode['code_challenge']) && $authCode['code_challenge'] !== '') {

$code_challenge_method = $request->query('code_challenge_method');

if ($this->config['enforce_pkce']) {
if (!$code_challenge) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!$code_challenge) {
if ($code_challenge === null || $code_challenge === '') {

$response->setError(400, 'invalid_code_challenge', 'The PKCE code challenge supplied is invalid');

return false;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent mistake

@keithgordon2
Copy link

I was very excited to see that you two had been working on PKCE. I need to implement PKCE and would prefer to keep this library. Is this still a work in progress of can I merge the six files and apdate the database to get it running?

@darthsteven
Copy link
Author

@keithgordon2 yeah, it should work fine at the moment. It's still technically a WIP in terms of getting the storage classes updated and tests passing, but if you are swapping out the storage class anyway, then this code works fine as such :)

@ezralazuardy
Copy link

@darthsteven any update to this? I need to implement PKCE for mobile authorization, and seems this library doesn't support this atm..

@keithgordon2
Copy link

Ezra,

I got the PKCE to work with darthsteven's changes and a couple quick database and database code changes.

@darthsteven
Copy link
Author

@ezralazuardy if you want some working PKCE code and are free to use any library then take a look at https://oauth2.thephpleague.com/ which has PKCE support already.

@ezralazuardy
Copy link

ezralazuardy commented Aug 26, 2020

@darthsteven yes, i've try it couple days ago and work perfectly for my project. thanks for the respond though..

@thaarok thaarok mentioned this pull request Feb 26, 2023
@thaarok
Copy link
Contributor

thaarok commented Feb 26, 2023

Thanks @darthsteven - I have tried to finish this in a new PR, as I would like to have this support in this lib: #1045
I will welcome if you check it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants