Skip to content
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

feat: add ability to check JWT claims #211

Merged
merged 14 commits into from
Mar 29, 2021
Merged

Conversation

ctjlewis
Copy link
Contributor

@ctjlewis ctjlewis commented Mar 13, 2021

Description

I wanted to easily protect a route not only for logged-in users, but also for those with a given ROLE JWT claim. I figured I'd send this PR in for review in case this team was interested in merging the changes, or some version of them.

Usage

Example of an unauthorized route request. User with ['USER', 'MODERATOR'] claim trying to access a protected component that is only for ROLE claims that include 'ADMIN':

const WrappedComponent = withAuthenticationRequired(MyComponent, {
requiredClaims: {
'https://my.app.io/jwt/claims': {
ROLE: ['ADMIN'],
},
},
});
/**
* A user with USER and MODERATOR roles.
*/
const mockUser = {
name: '__test_user__',
'https://my.app.io/jwt/claims': {
USER: '__test_user__',
ROLE: ['USER', 'MODERATOR'],
},
};

Example of an authorized route request, demonstrates claim string -> string[] coercion, such that ROLE: "ADMIN" matches ROLE: ["ADMIN"] etc.

const WrappedComponent1 = withAuthenticationRequired(MyComponent, {
requiredClaims: {
'https://my.app.io/jwt/claims': {
ROLE: 'ADMIN',
},
},
});
const mockUser1 = {
name: '__test_user__',
'https://my.app.io/jwt/claims': {
USER: '__test_user__',
ROLE: ['ADMIN'],
},
};

Testing

Unit tests were added to ensure that protected routes only showed to users with all specified JWT claims. If requiredClaims in WithAuthenticationRequiredOptions is falsy, the claims check automatically passes.

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@ctjlewis
Copy link
Contributor Author

ctjlewis commented Mar 13, 2021

In trying to link this package and use it, I get the following error but can't figure out why (I'm sure it's something simple):

image

Because this did not involve any changes to package.json, I'm assuming it has to do with a Yarn/NPM conflict on my local machine?

@ctjlewis ctjlewis changed the title Add ability to check JWT claims feat: Add ability to check JWT claims Mar 13, 2021
@ctjlewis
Copy link
Contributor Author

ctjlewis commented Mar 15, 2021

Requesting notes from: @adamjmcgrath (sorry to ping)

[@Vivalldi: So sorry I tagged you, thought you were a code owner 😅]

@adamjmcgrath
Copy link
Contributor

Hi @ctjlewis - thanks for raising this PR, checking claims to access routes would be a great addition to the SDK.

I have a request about the implementation - could you change it to accepting function that returns a boolean to indicate if the claim check passes, eg

Example of an unauthorized route request. User with ['USER', 'MODERATOR'] claim trying to access a protected component that is only for ROLE claims that include 'ADMIN':

const WrappedComponent = withAuthenticationRequired(MyComponent, {
  claimCheck(claims) {
    return claims['https://my.app.io/jwt/claims'].includes('ADMIN')
  }
}); 

(The name claimCheck is consistent with some of our other js APIs)

@adamjmcgrath
Copy link
Contributor

@ctjlewis

In trying to link this package and use it, I get the following error but can't figure out why (I'm sure it's something simple):

It's a pain to link packages that use react hooks - you could try pointing react and react-dom to ../auth0-react/node_modules/react and ../auth0-react/node_modules/react-dom.

Or just try using npm pack and installing the tar file locally

@ctjlewis
Copy link
Contributor Author

That made the code change much smaller and more straightforward, probably the best approach. Please lmk what you think about the changes.

Remove string -> string[] coercion, renamed to `claimCheck`, update tests
Copy link
Contributor

@adamjmcgrath adamjmcgrath left a comment

Choose a reason for hiding this comment

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

looks good - couple of suggestions

src/with-authentication-required.tsx Outdated Show resolved Hide resolved
src/with-authentication-required.tsx Outdated Show resolved Hide resolved
@ctjlewis ctjlewis marked this pull request as ready for review March 25, 2021 03:39
@ctjlewis ctjlewis requested a review from a team as a code owner March 25, 2021 03:39
@ctjlewis
Copy link
Contributor Author

Though this assumes JWT claims will be string | string[], I think it's a good place to start. I would appreciate any help regarding docs or adding support for types outside of strings or arrays of strings.

@adamjmcgrath
Copy link
Contributor

adamjmcgrath commented Mar 25, 2021

Hi @ctjlewis

If you change the signature of claimCheck to accept a User I'd be happy to proceed with this PR

@ctjlewis
Copy link
Contributor Author

Please let me know if these changes are sufficient.

@adamjmcgrath
Copy link
Contributor

@ctjlewis - couple of other things

The dependent key for the useEffect should be routeIsAuthenticated not isAuthenticated
And we only want to run the claimCheck function if the user is authenticated.

I've made the changes and put them here for your reference 3a8514d

@ctjlewis
Copy link
Contributor Author

Thank you very much for your help on this Adam. I merged your suggestions.

@adamjmcgrath
Copy link
Contributor

adamjmcgrath commented Mar 26, 2021

Hey @ctjlewis

It doesn't look like you implemented the majority of changes I suggested in 3a8514d - could you take another look?

Also - could you revert your changes to with-auth.tsx?

@adamjmcgrath adamjmcgrath mentioned this pull request Mar 26, 2021
@ctjlewis
Copy link
Contributor Author

The changes in with-auth0.tsx were just to silence the following tsc error:

'auth0' is specified more than once, so this usage will be overwritten.ts(2783)

@adamjmcgrath
Copy link
Contributor

The changes in with-auth0.tsx were just to silence the following tsc error

Am not seeing that error on the CI server - perhaps you're using a different TS compiler than the project.

Copy link
Contributor

@adamjmcgrath adamjmcgrath left a comment

Choose a reason for hiding this comment

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

lgtm - thanks @ctjlewis!

@adamjmcgrath adamjmcgrath merged commit e06d8d3 into auth0:master Mar 29, 2021
@ctjlewis ctjlewis changed the title feat: Add ability to check JWT claims feat: add ability to check JWT claims Jul 18, 2021
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.

2 participants