Added support for PKCE #245
Conversation
|
Hi @nulltoken I have now implemented support for PKCE in this PR. Appreciate if you or somebody else of the maintainers could have a look, whenever you have the time of course 😊 |
|
@tanettrimas 👋 Thanks for this. I haven't dived into it yet, however, just by skimming through it, there are two things that stand out immediately:
Those two things add a lot of noise to the diff. |
|
Oh I did not notice that 🤦♂️ i’ll fix that 😄 |
|
The diff should be much more managble now 😅 |
|
@tanettrimas Great! It seems there still are some beautification related changes in Note: I'm definitely not against beautification changes and code style normalization. However, I'd prefer to see them isolated in a later dedicated Pull Request. |
|
Thanks for the feedback. Seems I misunderstood how the assertion functions in your codebase is supposed to be used, I will look into it and refactor it to not use them this way 😄 |
|
@tanettrimas Looks like this going along nicely! Could you also please update the README to mention the PKCE support? |
| ) => { | ||
| let challenge: string; | ||
|
|
||
| switch (algorithm) { |
There was a problem hiding this comment.
@tanettrimas How about adding a default: clause for this switch and make it throw?
There was a problem hiding this comment.
Why do I need a default? it is only used with constant values
There was a problem hiding this comment.
It's good coding practice to leave no execution branches unchecked, no matter how unlikely you think their execution might be.
Every public method (in this case, "public" means "an exported function in a module that someone else can consume") should validate its input, and throw if it receives an input it's not expected to handle.
I can see that you are already guarding against invalid values and returning an HTTP 400 error response if this happens, and today nothing in the rest of the code will pass an invalid value to this method. The thing is that tomorrow some other developer might decide to reuse this method somewhere else and not guard against invalid values, thus producing an unexpected behaviour and unintentionally introducing a bug.
| const res = await request(service.requestHandler) | ||
| .get('/jwks') | ||
| .expect(200); | ||
| const res = await request(service.requestHandler).get('/jwks').expect(200); |
There was a problem hiding this comment.
Please rollback this beautification and the remaining ones below.
|
@tanettrimas Hope everything's alright on your side. Just a few minor thingies to straighten and we should be to release your feature 😉 |
Hi :) Had some things to deal with on a personal level, but I will look to finish up next week 😄 |
|
Just wondering if this is likely to be merged. This would be a helpful addition! |
|
@tanettrimas Hi! Sorry for the LONG hiatus. Since the project has advanced a bit since this PR got started, could you please rebase it to the current HEAD so I can have a good look at it? Thanks! |
|
Hi, will do. I must admit that I lost a bit motivation after so much nit-picking on whitespace changes. But I will rebase and then you can give another go :) |
…e_method on authorize
Also made test less flaky so that it will detect changes to the openid configuration
I'm guessing that this is about @nulltoken's request for you to not unnecessarily beautify code in this PR. The issue is not about beautifying, but doing it so in the same PR where other things are being changed. The net effect is that there's more code for us to review, while making it hard for us to tell which changes are PR-related, and which are not. Consider, for example, the changes you've introduced in
As far as I can tell (and I could be wrong), all of those changes are only whitespace. The thing is: I can't easily tell if they are indeed only whitespace characters, or if you changed something relevant to the PR. If you believe that code might be more readable than it currently is, by all means, propose a new PR with only beautification changes (alongside potential adjustments in We really appreciate the effort you're putting for the project, and we welcome more of it (if you want, of course 😛). We only ask from you to make your contributions as focused as possible. Thanks! |
| ) => { | ||
| let challenge: string; | ||
|
|
||
| switch (algorithm) { |
There was a problem hiding this comment.
It's good coding practice to leave no execution branches unchecked, no matter how unlikely you think their execution might be.
Every public method (in this case, "public" means "an exported function in a module that someone else can consume") should validate its input, and throw if it receives an input it's not expected to handle.
I can see that you are already guarding against invalid values and returning an HTTP 400 error response if this happens, and today nothing in the rest of the code will pass an invalid value to this method. The thing is that tomorrow some other developer might decide to reuse this method somewhere else and not guard against invalid values, thus producing an unexpected behaviour and unintentionally introducing a bug.
A fair point, but I might remind you that it is possible to filter out whitespace changes in Github just to remove the "noise". I was previously not aware that my editor added all formatting by default - some of the files were wrongly formatted in the first place. But it's fine - just very tedious. |
Co-authored-by: Jordi Poveda <jorgepoveda@gmail.com>
PR Checklist
npm testlocally and all tests are passing.PR Description
This PR implements #218 which adds support for PKCE, both plain and SHA256-hashed code_verifier. It is based on the RFC and should be more-or-less compliant with it.
However, it is implemented as an opt-in approach (which means that it does not support OAuth2.1 since there PKCE is always mandatory. But that was not required for the task either I assume).
This means that if the requests does not have anything related to PKCE in them, then everything works as before.