feature(pkce): added pkce support

[jankapunkt]

Summary

This implements PKCE support (related: #76) similar to oauthjs/node-oauth2-server#658

However, this did not 1:1 copy the implementation and rather modified it.

Linked issue(s)

#76

Involved parts of the project

• AuthorizationCodeGrantType
• TokenHandler

In the original PR there was also CodeResponseType covered. However, they targetetd oauthjs:dev and the content of this file differs from ours (and from 3.1.1: )

• CodeResponseType in oauthjs:3.1.1: https://github.com/oauthjs/node-oauth2-server/blob/v3.1.1/lib/response-types/code-response-type.js (this is the same as ours on development, right?)
• CodeResponseType in oauthjs:dev: https://github.com/oauthjs/node-oauth2-server/blob/dev/lib/response-types/code-response-type.js
• CodeResponseType in their PR: https://github.com/oauthjs/node-oauth2-server/blob/1dd54a4cab61c9771bdeeba1c74afc5a4e5c2b65/lib/response-types/code-response-type.js

This is open to debate as I am not 100% sure if the implementation for CodeResponseType is even required anymore.

Added tests?

Yes

OAuth2 standard

RFC 7636

Reproduction

Checkout this branch and run the tests.

Important

This needs to be tested very well, including integration with our own implementations!

[jankapunkt]

@Uzlopak do you mind checking this out?

[jankapunkt]

Someone found the time to test this?

[jorenvandeweyer]

I'll test and review this in the coming days.

[jorenvandeweyer]

Everything looks fine, but maybe we should add some documentation?

[jankapunkt]

Definitely, otherwise it will be really hard to set it up

[jorenvandeweyer]

We can already merge it if you want since we are not hosting our documentation at this moment.

(We also need to update the typescript definitions.)

[jankapunkt]

We can already merge it if you want since we are not hosting our documentation at this moment.

We don't? Well, I will open an issue to use GitHub pages for that, I can also provide the PR for it.

(We also need to update the typescript definitions.)

For this PKCE? Can you add this to the PR, please? I have no TS experience.

[jirehnimes]

Any updates about this?

[jankapunkt]

@jirehnimes it's basically implemented but needs further testing by people who use it in their workflow and a proper review. If you'd like to test it please let me know so I can advice you, if required.

[jirehnimes]

@jirehnimes it's basically implemented but needs further testing by people who use it in their workflow and a proper review. If you'd like to test it please let me know so I can advice you, if required.

My bad. Thanks for the fast response!

[martinssonj]

Hi!
I have tested out the PKCE flow. To get it to work for my usage I want the code challenge to be saved with the authorization code so it later can be used when checking the correct code and code verifier. I created a PR for that into the feature-pkce branch.

#161

[jankapunkt]

@martinssonj would you mind adding a final review to this one? I will follow this week and then we can merge this and release 4.3.0

[martinssonj]

@martinssonj would you mind adding a final review to this one? I will follow this week and then we can merge this and release 4.3.0

I will test it out in my project a final time tomorrow. Otherwise, it looks good to me.

[martinssonj]

Hi @jankapunkt. What do you think about merging this one? Let me know if there is anything I can assist with for this PR.

[jankapunkt]

Hey @martinssonj thanks for the reminder. I will add a review this weekend, promise (.all) 😅

[jankapunkt]

@martinssonj while reading again through RFC7636 I found that 4.4.1 missed to throw an error when an unknown (but not undefined) code challenge method, such as 'foo' is in the request. I added this in the latest commit.

I am reading through a few edge cases and see if we have them covered, too.

[martinssonj]

Nice that you found this.

[jankapunkt]

@martinssonj from my end it's all good now, would you mind adding a final review on this (I added two one more test) as I can't review my own pull request by our ruleset.

After that I will merge and release 4.3.0

@jorenvandeweyer @Uzlopak @HappyZombies feel free to comment / review, too if you like

[martinssonj]

Looks good.

[jankapunkt]

@martinssonj @jirehnimes the release is already available https://www.npmjs.com/package/@node-oauth/oauth2-server/v/4.3.0

The master branch is not updated yet as I have to fiddle with some legacy stuff in our CI

[kehers]

The current implementation expects code_challenge and code_challenge_method as POST parameters and not GET (authorize-handler:379 and authorize-handler:390). Curious if this should be so.

[jankapunkt]

The current implementation expects code_challenge and code_challenge_method as POST parameters and not GET (authorize-handler:379 and authorize-handler:390). Curious if this should be so.

Hi @kehers, the RFC 7636 does not explicitly state which of both methods to use. Is there an explicit issue in your setup by using POST?

// cc @martinssonj

[martinssonj]

It looks like GET should be supported by authorize from looking at this: https://datatracker.ietf.org/doc/html/rfc6749#section-3-1 and getResponseType is supporting query parameters so I think code challenge methods should as well.