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

Adds a hasRole() and isAuthenticated() to determine the role membership or if the request is authenticated #3006

Merged
merged 22 commits into from Jul 20, 2021

Conversation

dthyresson
Copy link
Contributor

@dthyresson dthyresson commented Jul 7, 2021

Currently we have hasRole method on the web side of things, but it would be neat having it on the api side. We currently have requireAuth, but that will throw an error while hasRole would most likely return a boolean.

For now it's super easy to just add copy the requireAuth logic and change out the throwing of exceptions to boolean, but would be nice seeing a native way in future RedwoodJS versions.

Fixes #1730

Adds a hasRole() and isAuthenticated() in auth.js that return booleans rather than exceptions (as is done by requireAuth) so these checks can be used to determine the role membership or if the request is authenticated.

Also:

  • Uses roles instead of role to be clear than can check a set of values Fixes TS error in default auth.ts #3024
  • Moves the parseJWT util of api and into api/auth where it really belongs

NOTE: Draft tested in local RBAC blog identity app.

Also needs to update dbAuth.auth.js.template (Done!)

@redwoodjs-bot redwoodjs-bot bot added this to New issues in Current-Release-Sprint Jul 7, 2021
@dthyresson dthyresson self-assigned this Jul 7, 2021
@dthyresson dthyresson marked this pull request as draft July 7, 2021 16:29
@dthyresson dthyresson changed the title Dt auth has role Adds a hasRole() and isAuthenticated() to determine the role membership or if the request is authenticated Jul 7, 2021
@thedavidprice thedavidprice added this to the next-release-priority milestone Jul 7, 2021
@redwoodjs-bot redwoodjs-bot bot moved this from New issues to In progress (priority) in Current-Release-Sprint Jul 7, 2021
@jtoar
Copy link
Contributor

jtoar commented Jul 8, 2021

@dthyresson do you think the RFC here is part of the future for addressing the disparity? #2431

@dthyresson
Copy link
Contributor Author

@dthyresson do you think the RFC here is part of the future for addressing the disparity? #2431

I don't think this addresses #2431 since as I read the issue, that is more of a web-side concern to check membership/permissions -- and that can depend highly on the auth provider and the implementation.

This PR simply intends to make checking server-side if the user belongs to a role (or not) rather than throwing exceptions when checking.

It helps if you just "need to know if they do" without having to do some try/catch instead.

I have a feeling that dbAuth might be a good first case for having both roles and permissions implemented --and then can think of a way web-side to enforce.

@dthyresson
Copy link
Contributor Author

Will need doc change to use roles instead of role and also update RBAC cookbook.

@dthyresson dthyresson marked this pull request as ready for review July 12, 2021 17:21
@dthyresson dthyresson requested a review from dac09 July 12, 2021 17:23
dthyresson and others added 2 commits July 16, 2021 10:28
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
@dthyresson
Copy link
Contributor Author

From @Tobbe

I can't make up my mind about this one

The code clearly handles that you don't pass a role in. But on the other hand it feels too weird to call hasRole(). From just looking at that, without reading the implementation, it's not very easy to know what it would do. Would it check if the user has any role? Any role at all? Or would it check if the user has the role undefined?

I see. The questions are what if the developer either set roles to

  • undefined
  • empty array

In both cases, the function should return false.

But, then does the question, does the user have undefined roles mean they have no roles assigned, ie unassigned roles -- and then really need to check if their role assignment is undefined or roles length is 0.

Or, should undefined throw an Error?

Or, is undefined simply invalid and return false because preventing access is better than granted access.

Perhaps just a comment to clarify?

@Tobbe
Copy link
Member

Tobbe commented Jul 16, 2021

From a theoretical/academical point of view I think it should be a compilation error in a TS project, and a runtime error in a JS project.

But from a DX pov it's probably nicer to do what you've done and just return false. So a comment is probably the best solution.

Copy link
Contributor Author

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

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

Added comment in JSDoc if not roles

@dthyresson dthyresson requested a review from dac09 July 19, 2021 13:48
@dthyresson dthyresson linked an issue Jul 19, 2021 that may be closed by this pull request
@dthyresson dthyresson merged commit ecf8aa9 into redwoodjs:main Jul 20, 2021
Current-Release-Sprint automation moved this from In progress (priority) to Done Jul 20, 2021
@dthyresson dthyresson deleted the dt-auth-has-role branch December 23, 2021 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Default auth.[jt]s is not formatted correctly TS error in default auth.ts RBAC: Add hasRole to API
5 participants