Skip to content

Add PKCE support#658

Open
jcdogo wants to merge 2 commits into
oauthjs:devfrom
dogomedia:features/pkce
Open

Add PKCE support#658
jcdogo wants to merge 2 commits into
oauthjs:devfrom
dogomedia:features/pkce

Conversation

@jcdogo

@jcdogo jcdogo commented Oct 8, 2020

Copy link
Copy Markdown

This pull implements PKCE support (RFC7636). It is originally based on pull #452, but has been cleaned up a bit.

Summary of changes:

  1. PKCE is completely optional. If the PKCE-related parameters (code_challenge, code_challenge_method, and code_verifier) are not passed to the server, the server behaves exactly the same as before. PKCE mode is enabled only when:
  • code_challenge (and optionally code_challenge_method) parameters are included during authorization code grant.
  • code_verifier parameter is included during token grant. When code_verifier parameter is included, client_secret is ignored since we are using PKCE for authentication.
  1. This change introduces 2 new optional fields (codeChallenge and codeChallengeMethod) to the authorization code model. Changes are required to Model#saveAuthorizationCode and Model#getAuthorizationCode to persist and retrieve these 2 new fields if they are present.
  2. 100% backwards compatible with existing implementations. If existing servers do not update the Model#saveAuthorizationCode and Model#getAuthorizationCode methods, they will continue to work just as they did before the change.
  3. Added lots of tests and updated the documentation.

Example of my changes to saveAuthorizationCode for a MongoDB model (in Typescript):

  const mongoOAuthCodeGrant = {
    code: code.authorizationCode,
    expires_at: code.expiresAt,
    redirect_uri: code.redirectUri,
    scope: code.scope,
    client_id: client.id,
    user_id: userId,
    oauth_client_id: mongoOAuthClient._id,
  };

  if (code.codeChallenge) {
    mongoOAuthCodeGrant.code_challenge = code.codeChallenge;

    if (code.codeChallengeMethod) {
      mongoOAuthCodeGrant.code_challenge_method = code.codeChallengeMethod;
    }
  }

  const saveResult = await db
    .collection(oauthAuthCodeGrantsCollectionName)
    .insertOne(mongoOAuthCodeGrant);

Example of my changes to getAuthorizationCode for a MongoDB model (in Typescript)

  const mongoAuthCodeGrant = await db
    .collection(oauthAuthCodeGrantsCollectionName)
    .findOne({code: authorizationCode});

  const user = await getUserById(mongoAuthCodeGrant.user_id);
  const client = await getClientById(mongoAuthCodeGrant.client_id);
  const grant: OAuthCodeGrant = {
    authorizationCode: mongoAuthCodeGrant.code,
    expiresAt: mongoAuthCodeGrant.expires_at,
    redirectUri: mongoAuthCodeGrant.redirect_uri,
    scope: mongoAuthCodeGrant.scope,
    client: client,
    user: user,
  };

  if (mongoAuthCodeGrant.code_challenge) {
    grant.codeChallenge = mongoAuthCodeGrant.code_challenge;

    if (mongoAuthCodeGrant.code_challenge_method) {
      grant.codeChallengeMethod = mongoAuthCodeGrant.code_challenge_method;
    }
  }

@jcdogo

jcdogo commented Oct 8, 2020

Copy link
Copy Markdown
Author

This partially addresses issue #637 by implementing PKCE.

@jensdev

jensdev commented Dec 14, 2020

Copy link
Copy Markdown

@jcdogo thanks for your work. @thomseddon any idea on when this will land?

@Nedomas

Nedomas commented Jan 22, 2021

Copy link
Copy Markdown

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.

@fjeddy

fjeddy commented Feb 7, 2021

Copy link
Copy Markdown

In need of this as well :)

@yinzara

yinzara commented Feb 16, 2021

Copy link
Copy Markdown

yeah definitely in need of this. @thomseddon any chance this could make it out soon? even the v5 version would be fine :)

@Legion2

Legion2 commented Feb 20, 2021

Copy link
Copy Markdown

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.

@lior1503

lior1503 commented May 5, 2021

Copy link
Copy Markdown

I think we must merge this ASAP because npm audit fails on lack of support with PKCE support !

Comment thread docs/model/spec.rst
+----------------------------+--------+----------------------------------------------------------------+
| [code.codeChallenge] | String | The code challenge string used with PKCE (RFC7636). |
+----------------------------+--------+----------------------------------------------------------------+
| [code.codeChallengeMethod] | String | The string for the code challenge method (`S256` or `plain` |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
| [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 };

Copy link
Copy Markdown

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.

Comment thread CHANGELOG.md
Comment on lines +8 to +9
* new: Switch from jshint to eslin
* fix: authorization_code grant should not be required in implicit flowt

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

might be typo,

Suggested change
* 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

@SolveXYZPro

SolveXYZPro commented May 21, 2022

Copy link
Copy Markdown

Thanks for this PR. I'd like to use this merge ASAP. @mjsalinger when could this PR be merged?

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.

10 participants