-
-
Notifications
You must be signed in to change notification settings - Fork 935
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
PKCE #452
base: dev
Are you sure you want to change the base?
PKCE #452
Conversation
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.
Can this PR be refactored in a way that a) doesn't make it a breaking change, and b) ensures that the PKCE is optional. One possible way to do this would be to have a separate (optional) model method to handle the PKCE after the generateAuthorizationCode method is invoked and resolved.
Also please change the PR to point to the |
Hang on, is this full implementation OF PKCE? |
This PR supports code_challenge parameter and code_challenge_method parameter to be saved with the authorization code. What is missing, code_challenge parameter and code_challenge_method parameter are not passed to the model.generateAuthorizationCode method, because it will be a breaking change. If is something wrong or missing, please send a feedback. |
Yeah, but there's still an issue with missing The But that also means, |
Thank you for the feedback. I can add a change in |
excuse my ignorance, but Isn't the idea of PKCE flow to eliminate the need of passing client_secret? I thought it's the main point, is to be used as a public auth method. For mobile apps or web apps. I need that change ASAP. So for the time being, I will pull your PR and modify it for my own needs for my Oauth2 server. But I will keep track of changes you make, though. Maybe you'll come up with something better. |
No, idea of PKCE describes technique to mitigate against the authorization code interception attack. I agree, that public clients should not use client_secret (as per rfc8252), but the server should assume client confidentiality by You have provided solution, where server assume client confidentiality by passing Yes, you should make your own fork, because any PR can be changed or deleted and then you can be in troubles. |
@visvk Can you rebase this PR to get rid of the conflicts? |
Sorry, but there are many conflicts in the rebase process. |
@visvk That should be fine. |
- move getCodeChallenge, getCodeChallengeMethod to code-response-type.js
@mjsalinger done. |
When can we expect PKCE in official version? Thanks :) |
Ok i now used your version and changed it a bit and its fully working in my SPA Application. Thanks for your work @visvk |
@yhc44 Do you want to use this feature in your SPA app? Because SPAs would not benefit from PKCE. |
@visvk Actually i've read that it is better than implicit grant. I dont want to use authorization grant with secret. Is it the same as PKCE in SPA? What do you recommend? |
Hey guys. First of all, awesome work on this lib. Save me a loooot of time. There's any ETA on this feature? This is almost a must have to the authorization server that I'm building for my client. Thanks in advance. |
Any idea of when this will be merged? |
@visvk What's the status on this? |
any news on this PR ? |
I'm quoting oauth.net which states that PKCE is meant for SPAs.
|
could PKCE be merged into the |
6 month later... Please is there any plan to merge this ? We are waiting for this feature. Thx |
@Rmannn why don't you make a fork of this repo and merge it yourself, while waiting for the official release? |
@patrykcieszkowski cause apparently they are not going to merge this pr anytime soon. Even in the next release. We are really thankful for this library, but we simply do not want to loose more time waiting for things that are not going to happen |
This isn't pegged for the next release - it's also a bit of a whopper so would need a bit of time for review, but would be brilliant for 4.1 |
@mjsalinger @thomseddon, Hi there, thanks for the library. My question is... have you planned this to any date in particular? Very thanks and congrats! |
Implementation of rfc7636 Proof Key for Code Exchange by OAuth Public Clients
Notes:
This can be a breaking change, so I've removed this part of codemodel.generateAuthorizationCode
is called with 2 new parameters (in total 5)codeChallenge
andcodeChallengeMethod
. Typically, the "code_challenge" and "code_challenge_method" valuesare stored in encrypted form in the "code" itself but could
alternatively be stored on the server associated with the code.
model.saveAuthorizationCode
: addedcodeChallenge
andcodeChallengeMethod
to the code object. We need to store the code challenge and it's method server side and then return it later on.model.getAuthorizationCode
when code was issued with PKCE parameters, it should return these parameters in thecode
object. Then thecode_verifier
is validatedTODO:
client_secret should not be requiredpossible withisClientAuthenticationRequired
optionFeedback is welcome!