Add hasRole to Auth for RBAC #939

dthyresson · 2020-08-05T15:26:19Z

Implement Role-based Authorization

Resolves: #806

See the included updated docs for the full run down -- it is not for the faint of heart.

I have hasRole working with Auth0 and have documented how the setup and configure access.

In short:

roles are defined on app_metadata
to get this info onto the JWT that is decoded, in Auth0 you have to use rules (see docs)
you decoded token will look something like

{
     "https://example.com/app_metadata": {
          "authorization": {
            "roles": [
              "admin"
            ]
          }
        },
        "https://example.com/user_metadata": {},
        "nickname": "user",
        "name": "user@example.com",
        "picture": "https://s.gravatar.com/avatar/me.png",
        "updated_at": "2020-08-04T13:44:59.691Z",
        "email": "user@example.com",
        "email_verified": true,
        "iss": "https://example.com/",
        "sub": "email|XXXXXXXX",
        "aud": "XXXXXXXXX",
        "iat": 1596627063,
        "exp": 1596663063,
}

Note the namespaced app_metadata
when decoding the token in auth.js getCurrentUser, extract roles and assign to currentUser.roles=[]
add hasRole check in auth.js
add hasRole to graphql
adds hasRole to auth provider context and useAuth hook
adds hasRole to Private route to check if assigned role

A auth.js could look like

import jwt from 'jsonwebtoken'
import { AuthenticationError, ForbiddenError } from '@redwoodjs/api'
import { context } from '@redwoodjs/api/dist/globalContext'

const NAMESPACE = 'https://example.com'

const requireAccessToken = (decoded, { type, token }) => {
  if (token || type === 'auth0' || decoded?.sub) {
    return
  } else {
    throw new Error('Invalid token')
  }
}

const appMetadata = (decoded) => {
  return decoded[`${NAMESPACE}/app_metadata`] || {}
}

const roles = (decoded) => {
  return appMetadata(decoded).authorization?.roles || []
}

const currentUserWithRoles = async (decoded) => {
  const currentUser = await userByUserId(decoded.sub)
  return { ...currentUser, roles: roles(decoded) }
}

export const getCurrentUser = async (decoded, { type, token }) => {
  try {
    requireAccessToken(decoded, { type, token })
    return currentUserWithRoles(decoded)
  } catch (error) {
    return decoded
  }
}

export const hasRole = (role) => {
  if (!context.currentUser.roles?.includes(role)) {
    throw new ForbiddenError("You don't have access to do that.")
  }
}

export const requireAuth = () => {
  if (!context.currentUser) {
    throw new AuthenticationError("You don't have permission to do that.")
  }
}

You would protect a service like:

export const myThings = () => {
  requireAuth()
  hasRole('admin')

  return db.user.findOne({ where: { id: context.currentUser.id } }).things()
}

You can also protect routes:

<Router>

  <Private unauthenticated="forbidden" role="admin">
    <Route path="/settings" page={SettingsPage} name="settings" />
    <Route path="/admin" page={AdminPage} name="sites" />
  </Private>

  <Route notfound page={NotFoundPage} />
  <Route path="/forbidden" page={ForbiddenPage} name="forbidden" />
</Router>

And also protect content in pages or components via the userAuth() hook:

const { isAuthenticated, hasRole } = useAuth()

...

{hasRole('admin') && (
  <Link to={routes.admin()}>Admin</Link>
)}

Have updated generators, too.

Notes

Want to get Pr out for initial implementation review.

Only tested with Auth0
Not yet tested on a new create auth app or the generators or upgrade

Edit: Updated to use ForbiddenError.

Edit: Tested with a newly created 15.3 app and yarn rw g auth auth0 and graphql and auth.js look ok

dthyresson · 2020-08-05T15:34:28Z

Note: once I clean up my test app UI, I'll try to include a screen recording of role auth being enforced.

dthyresson · 2020-08-05T16:47:11Z

Some thoughts:

Is this redundant:

  requireAuth()
  hasRole('admin')

The way hasRole is implemented, auth (ie currentUser) is required, so could leave off requireAuth().

Or,

could requireAuth() take an optional role and callhasRole()?

packages/auth/README.md

dac09 · 2020-08-05T17:18:09Z

packages/auth/README.md

+//    return appMetadata(decoded).authorization?.roles || []
+//  }
+//
+//  export const getCurrentUser = async (decoded) => {


I know we can't add types here, but is there a nice way to add some comments to show the shape of this function? e.g. mine looks like this in TS

interface TokenHeader { type: 'cli' | 'jwt' token: string } export const getCurrentUser = async ( decodedToken: string, tokenHeader: TokenHeader ) => { let user

☝️ Definitely a question best answered by @peterp

I've been dealing with this exact problem when creating serverless functions, which currently look like this:

export const handler = (event, context, callback) => { return { statusCode: '404' } }

Type-safety be damned! Unless I define those types myself... I don't know what's going on!?

One pattern that I've stumbled upon is to introduce a wrapping function:

export const handler = createServerlessFunction(({event, context })=> { return { statusCode: 200, body: JSON.stringify('pew pew pew'), } })

This is nice, because I can provide types for event and context, and can specify exactly what the user can return! I can also introduce some lifecycle things into this function that give me as a framework creator a bit more control. It's not nice because it's a bit redundant for just adding types.

Maybe, maybe, maybe we can introduce a wrapper for getCurrentUser? (These are always optional if you "understand" the structure that needs to be exported.

No related @peterp but if you wanted typescript magic in JS, have you tried adding // @ts-check to the top of the file? VSCode picks this up and does type checking if it can infer things.

One idea for this function is to use JS doc style comments

** /** * Handle how to auth a user with the decoded bearer token * @param decoded a decoded token on the type of Auth Provider used. Custom auth just returns the token * @param tokenDetails ... * @example ...... **//**

I don’t think we need this everywhere, but since this function is particularly an important one, it might be worth consideration!

@dac09 or @peterp Anything actionable for me here w/ re: to Token header and TS?

I don't think we have to add that in here... it could be done in another PR.

Or we can just add an issue and we can discuss it further.

Yeah makes sense!

Ok, I already added it ... I am just reworking all the docs and then testing. Have to step away for a bit but will commit soon.

Strange now seeing an error in CI that not seeing local. I'll have to investigate.

3:41:43 PM - Project 'tsconfig.json' is out of date because output file '.buildcache/AuthProvider.d.ts' does not exist 3:41:43 PM - Building project '/home/runner/work/redwood/redwood/packages/auth/tsconfig.json'... ##[error]src/AuthProvider.tsx(137,36): error TS2339: Property 'roles' does not exist on type 'CurrentUser'. ##[error]src/AuthProvider.tsx(198,11): error TS2322: Type '(role: string) => any' is not assignable to type '() => boolean'.

I am also going to setup an app w/ Netlify Identity and try out role access

packages/auth/README.md

packages/cli/src/commands/generate/auth/templates/auth.js.template

dac09 · 2020-08-05T17:26:05Z

Hey @dthyresson, top job with this one 💯! I'd like to spend some time testing this. Left some comments - as always, please take what I'm saying as suggestions - it's hard to convey that without adding "fluff" around what I'm saying

dthyresson · 2020-08-06T04:27:02Z

Question maybe for @aldonline, does packages/structure/src/model/RWRoute.ts need to be modified to handle the hasRole on a private route?

peterp · 2020-08-06T05:53:46Z

@dthyresson

Is this redundant:

  requireAuth()
  hasRole('admin')

I think so, we could make it something like this:

requireAuth({ role: 'admin' })

I would love to keep the surface area of the functions that we introduce into src/lib/auth.js to a minimum. Since it's in the user-space the more we add the more difficult it becomes to upgrade and remove things that we've introduced over time. It's also less daunting, for users, to mentally parse if we introduce a minimum amount of functionality.

So, I don't think we we should introduce a src/lib/auth.js#hasRole function. I think we should just use requireAuth, and I would try to keep the maximum amount of functions to getCurrentUser and requireAuth.

That said I do think the other functions you've introduced add a bunch of value for parsing and extracting values from a JWT. Maybe there's a way we can wrap that into a module that a user could import?

As an example:

import parseJWT from '@redwoodjs/api'

// ... 

const { roles } = parseJWT(token).appMetadata

dthyresson · 2020-08-06T12:27:09Z

I would love to keep the surface area of the functions that we introduce into src/lib/auth.js to a minimum.

Could not agree more @peterp . My auth.js was started to get bloated. I think this refactoring alleviates this.

I've refactored to:

requireAuth({ role: 'admin' }) -- much cleaner when protecting a service
remove no longer needed hasRole in GraphQLHandler (not sure why I thought I needed that there)
adds in parseJWT from '@redwoodjs/api' to extract app_metadata and roles from the common cases
adds test for parseJWT

There is still a hasRole on the context used in useAuth() hook on web side.

Edit: BTW -

The reason I did not implement

const { roles } = parseJWT(token).appMetadata

is because I think there is a case where the decoded token will have just a roles claim and not w/in app_metadata.

peterp · 2020-08-06T13:47:48Z

packages/auth/README.md

 //      throw new AuthenticationError("You don't have permission to do that.")
 //    }
+//
+//    if (!context.currentUser.roles?.includes(role)) {


Suggested change

// if (!context.currentUser.roles?.includes(role)) {

// if (typeof role !== 'undefined' && !context.currentUser.roles?.includes(role)) {

The specification of a role could be optional.

Yup, I noticed that as well and had not yet committed:

if (role && !context.currentUser.roles?.includes(role)) {

but will use your suggestion,

peterp · 2020-08-06T13:47:59Z

packages/auth/README.md

+// whether or not they are assigned a role, and optionally raise an
+// error if they're not.
+//
+//  export const requireAuth = ({ role }) => {


Suggested change

// export const requireAuth = ({ role }) => {

// export const requireAuth = ({ role } = {}) => {

peterp · 2020-08-06T13:49:08Z

packages/api/src/parseJWT.ts

+}
+
+const roles = (token: DecodedToken): { roles: string[] } => {
+  console.log(token)


Suggested change

console.log(token)

Sneaky sneaky console.log 🦕

Snuck in as part of my unit test.

peterp · 2020-08-06T13:52:56Z

packages/api/src/functions/graphql.ts

@@ -12,6 +12,8 @@ export type GetCurrentUser = (
  raw: AuthContextPayload[1]
 ) => Promise<null | object | string>

+export type HasRole = (role: string) => Promise<boolean>


Is this HasRole stuff still relevant? I thought you're overloading requireAuth as requireAuth({ role })

:) I removed this from my app's implementation ... but forgot to here.

It is not needed.

That's what happens when I try to code at 3am.

I'll do a full walkthrough my next push.

Hahaha! Nice! 3am coding sessions are the best

peterp · 2020-08-06T13:55:07Z

packages/auth/src/AuthProvider.tsx

+   * Checks if the "currentUser" from the api side
+   * is assigned a role
+   **/
+  hasRole(): Promise<boolean>


Suggested change

hasRole(): Promise<boolean>

hasRole(): boolean

Since it's checking against the data that's already in currentUser this probably doesn't have to be a promise

Ah yes. An earlier implementation has hasRole calling getCurrentUser hence the async but then realized did not need to do that.

dthyresson · 2020-08-06T20:53:29Z

FYI - I went through the Blog Tutorial again today with the intent to set Netlify Identity and test auth and RBAC with new rw changes.

Currently seeing a 500 error somewhere with currentUser:

api | POST /graphql 200 3.983 ms - 158
api | {
api |   exp: 1596750008,
api |   sub: 'ae930ab8-8cfd-4b53-8922-ee1af2d86XXX',
api |   email: 'dthyresson+rwblog-admin@example.com',
api |   app_metadata: { provider: 'email' },
api |   user_metadata: { full_name: 'DT' }
api | }
api | POST /graphql 500 0.740 ms - 595

Even though the Netlify widget thinks I am logged it and the token is in localStorage.

But, isAuthenticated I think is throwing the error (?).

Trying to track down.

dthyresson · 2020-08-06T21:56:08Z

I found the error via graphql playground:

Somehow my getCurrentUser didn't find the new parseJWT

{
  "error": {
    "errors": [
      {
        "message": "Context creation failed: (0 , _api.parseJWT) is not a function",
        "extensions": {
          "code": "INTERNAL_SERVER_ERROR",
          "exception": {
            "stacktrace": [
              "TypeError: Context creation failed: (0 , _api.parseJWT) is not a function",
              "    at getCurrentUser 
            "stacktrace": [

dthyresson · 2020-08-06T22:08:00Z

Ok, fixed it.

My PR code for auth.js worked fine ... but it had not reloaded because "graphql-scalars": "^1.2.6" was missing.

All good now ;)

dthyresson · 2020-08-06T22:28:28Z

Nice!

New Post and Edit are protected by "author" role, while I only have "admin" role:

 {hasRole('author') && (
<Link to={routes.newPost()} className="rw-button rw-button-green">
<div className="rw-button-icon">+</div> New Post
</Link>
)}

...

{hasRole('author') && (
  <Link
    to={routes.editPost({ id: post.id })}
    title={'Edit post ' + post.id}
    className="rw-button rw-button-small rw-button-blue"
  >
    Edit
  </Link>
)}

But if I give myself "author" as well ... in Netlify Identity.

then...

.gitignore

Implement hasRole in AuthProvider Begin hasRole tests Cloning route needs role prop

zwl1619 · 2020-08-09T08:01:28Z

@dthyresson Could you please implement authentication that uses Email and password, which not rely on 3rd party auth providers?

#745

dthyresson mentioned this pull request Aug 5, 2020

Implement Role-based Authorization #806

Closed

dac09 reviewed Aug 5, 2020

View reviewed changes

packages/auth/README.md Outdated Show resolved Hide resolved

dac09 reviewed Aug 5, 2020

View reviewed changes

packages/auth/README.md Show resolved Hide resolved

dac09 reviewed Aug 5, 2020

View reviewed changes

packages/cli/src/commands/generate/auth/templates/auth.js.template Outdated Show resolved Hide resolved

dac09 reviewed Aug 5, 2020

View reviewed changes

packages/cli/src/commands/generate/auth/templates/auth.js.template Outdated Show resolved Hide resolved

peterp added the topic/auth label Aug 6, 2020

peterp reviewed Aug 6, 2020

View reviewed changes

dthyresson commented Aug 7, 2020

View reviewed changes

.gitignore Show resolved Hide resolved

dthyresson added 7 commits August 8, 2020 04:03

Implement hasRole on AuthProvider/Context

4b13368

Implement hasRole in AuthProvider Begin hasRole tests Cloning route needs role prop

Auth0 RBAC and hasRole docs

3484e09

Adds test for hasRole in Private route

6259fa6

hasRole now raises ForbiddenError

33b5102

Cleans up auth generator with hasRole

6395a6a

Refactor hasRole and add parseJWT utility

ed7e932

Added parseJWT import to auth.js template

d6c4571

dthyresson added 7 commits August 8, 2020 04:06

Rework docs and PR suggestions.

b222746

Destructure decoded in default getCurrentUser case

10dae46

Using nvmrc. AuthProvider types for ubuntu 14.7

18e31e2

hasRole type always false

1fd8060

Trying hasRole(role: string) change

7eae766

parseJWT ts rework

7f50753

Rework parseJWT to get to green on build

aa0fd5b

dthyresson force-pushed the dt-add-has-role-to-auth branch from 3576b97 to aa0fd5b Compare August 8, 2020 09:06

peterp self-requested a review August 10, 2020 10:16

peterp approved these changes Aug 10, 2020

View reviewed changes

peterp added the next release label Aug 10, 2020

peterp added this to the Next release milestone Aug 10, 2020

peterp merged commit 577ba6e into redwoodjs:main Aug 10, 2020

dthyresson deleted the dt-add-has-role-to-auth branch December 23, 2021 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hasRole to Auth for RBAC #939

Add hasRole to Auth for RBAC #939

dthyresson commented Aug 5, 2020 •

edited

Loading

dthyresson commented Aug 5, 2020

dthyresson commented Aug 5, 2020

dac09 Aug 5, 2020

dthyresson Aug 5, 2020

peterp Aug 6, 2020

dac09 Aug 6, 2020 •

edited

Loading

dthyresson Aug 6, 2020

peterp Aug 6, 2020

peterp Aug 6, 2020

dac09 Aug 6, 2020

dthyresson Aug 6, 2020

dthyresson Aug 6, 2020

dac09 commented Aug 5, 2020

dthyresson commented Aug 6, 2020

peterp commented Aug 6, 2020 •

edited

Loading

dthyresson commented Aug 6, 2020 •

edited

Loading

peterp Aug 6, 2020

peterp Aug 6, 2020

dthyresson Aug 6, 2020

peterp Aug 6, 2020

peterp Aug 6, 2020

peterp Aug 6, 2020

dthyresson Aug 6, 2020

peterp Aug 6, 2020 •

edited

Loading

dthyresson Aug 6, 2020

peterp Aug 6, 2020

peterp Aug 6, 2020

dthyresson Aug 6, 2020

dthyresson commented Aug 6, 2020

dthyresson commented Aug 6, 2020

dthyresson commented Aug 6, 2020

dthyresson commented Aug 6, 2020

zwl1619 commented Aug 9, 2020 •

edited

Loading

	// if (!context.currentUser.roles?.includes(role)) {
	// if (typeof role !== 'undefined' && !context.currentUser.roles?.includes(role)) {

	// export const requireAuth = ({ role }) => {
	// export const requireAuth = ({ role } = {}) => {

Add hasRole to Auth for RBAC #939

Add hasRole to Auth for RBAC #939

Conversation

dthyresson commented Aug 5, 2020 • edited Loading

Implement Role-based Authorization

Notes

dthyresson commented Aug 5, 2020

dthyresson commented Aug 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dac09 Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dac09 commented Aug 5, 2020

dthyresson commented Aug 6, 2020

peterp commented Aug 6, 2020 • edited Loading

dthyresson commented Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterp Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dthyresson commented Aug 6, 2020

dthyresson commented Aug 6, 2020

dthyresson commented Aug 6, 2020

dthyresson commented Aug 6, 2020

zwl1619 commented Aug 9, 2020 • edited Loading

dthyresson commented Aug 5, 2020 •

edited

Loading

dac09 Aug 6, 2020 •

edited

Loading

peterp commented Aug 6, 2020 •

edited

Loading

dthyresson commented Aug 6, 2020 •

edited

Loading

peterp Aug 6, 2020 •

edited

Loading

zwl1619 commented Aug 9, 2020 •

edited

Loading