-
-
Notifications
You must be signed in to change notification settings - Fork 932
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 PKCE support #658
base: dev
Are you sure you want to change the base?
Add PKCE support #658
Conversation
This partially addresses issue #637 by implementing PKCE. |
@jcdogo thanks for your work. @thomseddon any idea on when this will land? |
Can confirm that this implementation works well for practical purposes. Would love to get this well-reviewed and merged. Let me know if there's any help needed as we plan to use this in an open-source auth wrapper library. |
In need of this as well :) |
yeah definitely in need of this. @thomseddon any chance this could make it out soon? even the v5 version would be fine :) |
We have a React frontend so the client secret is visible in the frontend code which is download by every user. We need the PKCE support. |
I think we must merge this ASAP because npm audit fails on lack of support with PKCE support ! |
+----------------------------+--------+----------------------------------------------------------------+ | ||
| [code.codeChallenge] | String | The code challenge string used with PKCE (RFC7636). | | ||
+----------------------------+--------+----------------------------------------------------------------+ | ||
| [code.codeChallengeMethod] | String | The string for the code challenge method (`S256` or `plain` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| [code.codeChallengeMethod] | String | The string for the code challenge method (`S256` or `plain` | | |
| [code.codeChallengeMethod] | String | The string for the code challenge method (`S256` or `plain`). | |
codeChallengeMethod: 'plain', | ||
codeChallenge: codeVerifier | ||
}; | ||
var client = { id: 'foobar', isPublic: true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isPublic
option is not used, as a result any client which allows authorization_code
grant also accepts PKCE requests.
* new: Switch from jshint to eslin | ||
* fix: authorization_code grant should not be required in implicit flowt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be typo,
* new: Switch from jshint to eslin | |
* fix: authorization_code grant should not be required in implicit flowt | |
* new: Switch from jshint to eslint | |
* fix: authorization_code grant should not be required in implicit flow |
Thanks for this PR. I'd like to use this merge ASAP. @mjsalinger when could this PR be merged? |
This pull implements PKCE support (RFC7636). It is originally based on pull #452, but has been cleaned up a bit.
Summary of changes:
code_challenge
,code_challenge_method
, andcode_verifier
) are not passed to the server, the server behaves exactly the same as before. PKCE mode is enabled only when:code_challenge
(and optionallycode_challenge_method
) parameters are included during authorization code grant.code_verifier
parameter is included during token grant. Whencode_verifier
parameter is included,client_secret
is ignored since we are using PKCE for authentication.codeChallenge
andcodeChallengeMethod
) to the authorization code model. Changes are required toModel#saveAuthorizationCode
andModel#getAuthorizationCode
to persist and retrieve these 2 new fields if they are present.Model#saveAuthorizationCode
andModel#getAuthorizationCode
methods, they will continue to work just as they did before the change.Example of my changes to
saveAuthorizationCode
for a MongoDB model (in Typescript):Example of my changes to
getAuthorizationCode
for a MongoDB model (in Typescript)