fix(auth): hasRole handles when currentUser.roles is a string #4678

dac09 · 2022-03-07T09:33:25Z

We had an edge case where we had missing logic in hasRoles.

The updated code now handles the case where:

currentUser.roles is a string
but hasRoles is called with an array of strings

dac09 · 2022-03-07T09:33:53Z

@Tobbe mind taking a look please? This is something Rob and I already fixed on the dbAuth template

dac09 · 2022-03-07T09:35:11Z

Related #4489

dthyresson · 2022-03-07T12:27:12Z

@dac09 Does this fix #4488 or #4489

I am almost done with #4492 so the directive can take a string or an array of string roles.

One changge I was making to auth was from:

export const hasRole = ({ roles }: { roles: AllowedRoles }): boolean => {

to

export const hasRole = ( roles : AllowedRoles ): boolean => {

so one could use hasRole('bazinga') instead of hasRole(roles: 'bazinga') per @cannikin

dac09 · 2022-03-07T12:28:08Z

It doesn't fix those issues DT (which is why I haven't linked them) - this fixes a different edge case we don't handle, as explained in the description

dthyresson · 2022-03-07T12:37:50Z

Ok just checking. Thanks,

packages/auth/src/__tests__/AuthProvider.test.tsx

Tobbe · 2022-03-07T12:41:17Z

packages/auth/src/AuthProvider.tsx

-      if (typeof role === 'string') {
-        return this.state.currentUser.roles?.includes(role) || false
+      if (typeof roleToCheck === 'string') {
+        return this.state.currentUser.roles?.includes(roleToCheck) || false


Don't we need an isArray check on currentUser.roles here as well? As you added below

Ummm in theory, if you passed a function or something, but we assume that you will pass either a string or an array, and don't do exhaustive checks for that. Think thats ok!

What I'm saying is if roleToCheck is a string, and this.state.currentUser.roles is a string we should not do includes(roleToCheck) but rather use ===.

Unless there's something I don't understand about the code here....

That's true, if you had admin_user and user, it'd return true in both cases if you checked for the role user with includes() 😬

So with the current setup there's 4 cases it needs to handle:

currentUser.roles is a string, roleToCheck is a string

currentUser.roles is an array, roleToCheck is a string

currentUser.roles is a string, roleToCheck is an array

currentUser.roles is an array, roleToCheck is an array

I feel like we're only covering 3 of these in here aren't we?

That check on lines 195-196 would cause the partial string problem if currentUser.roles is itself a string, but if it changes to === then it'll fail if currentUser.roles is an array.

Exactly, we have 4 cases it needs to handle, but only three branches in the code. I'd add another one to (explicitly) handle the missing case too

Ok bit late for me to play with this logic right now, but will look at it again in the morning :)

Once I have it I'll update the hasRole functions in all the auth templates!

@cannikin, @Tobbe how about this one?

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>

packages/cli/src/commands/setup/auth/templates/dbAuth.auth.ts.template

dac09 · 2022-03-09T09:10:33Z

Ok gents, I've updated all the templates!

dac09 · 2022-03-09T09:12:47Z

tasks/test-project/tasks.js

+      .replace('select: { id: true }', 'select: { id: true, roles: true }')
+      .replace(
+        'const currentUserRoles = context.currentUser?.roles',
+        'const currentUserRoles = context.currentUser?.roles as string | string[]'


This is to prevent typescript complaining on the smoke tests. TS knows from Primsa types that currentUser.roles can never be a string[] in the test project, so part of the code gets marked as an error, causing smoke tests with tsc to fail.

A TS error in a user's project is the desired effect, but not in the test project.

__fixtures__/test-project/api/src/functions/auth.ts

__fixtures__/test-project/api/src/lib/auth.ts

packages/auth/src/AuthProvider.tsx

packages/cli/src/commands/setup/auth/templates/auth.ts.template

__fixtures__/test-project/api/src/lib/auth.ts

dac09 · 2022-03-10T07:09:37Z

If you look at the auth file, it's littered with long comments everywhere. I will change it to roles, if you really want, because I don't think its worth either our time debating this, and it adds 0-0.1% more value.

…:dac09/redwood into fix/has-roles-when-currentUser-is-string * 'fix/has-roles-when-currentUser-is-string' of github.com:dac09/redwood: Fix react/prop-types lint warnings (redwoodjs#4674) Allow the number 0 for numericality validation values (redwoodjs#4700)

dac09 · 2022-03-10T08:55:08Z

I'll wait for @cannikin to do a once over, and then merge!

thedavidprice · 2022-03-10T21:41:16Z

Merging this to continue work on #4673

Thanks, all!

…d into feat/auth-checks-smoke-test * 'feat/auth-checks-smoke-test' of github.com:dac09/redwood: (21 commits) Remove supertokens-node from packages/api dependencies (redwoodjs#4715) fix(auth): hasRole handles when currentUser.roles is a string (redwoodjs#4678) Update dependency systeminformation to v5.11.7 (redwoodjs#4716) Update dependency webpack-manifest-plugin to v5 (redwoodjs#4693) Update graphqlcodegenerator monorepo (redwoodjs#4714) Update dependency @clerk/types to v1.28.3 (redwoodjs#4708) Update dependency @testing-library/react to v12.1.4 (redwoodjs#4709) Update dependency pino to v7.8.1 (redwoodjs#4703) Update dependency fastify to v3.27.4 (redwoodjs#4702) Update dependency @clerk/clerk-sdk-node to v2.9.8 (redwoodjs#4707) Update dependency @types/react to v17.0.40 (redwoodjs#4711) Update dependency @clerk/clerk-js to v2.17.3 (redwoodjs#4706) Fix react/prop-types lint warnings (redwoodjs#4674) Allow the number 0 for numericality validation values (redwoodjs#4700) update yarn.lock v0.49.1 update yarn.lock remove storybook type check (redwoodjs#4699) add bin proxy for rw-log-formatter to core (redwoodjs#4695) remove storybook type check (redwoodjs#4699) ...

dac09 added 2 commits March 7, 2022 16:15

fix(auth): hasRole handles when currentUser.roles is a string

f82e56a

Update auth tempaltes with hasRole function

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

a3b0141

jtoar assigned dac09 Mar 7, 2022

dac09 requested a review from Tobbe March 7, 2022 09:33

dac09 added the action/add-to-release label Mar 7, 2022

jtoar removed the action/add-to-release label Mar 7, 2022

dac09 added action/add-to-release release:fix labels Mar 7, 2022

jtoar removed the action/add-to-release label Mar 7, 2022

Merge branch 'main' into fix/has-roles-when-currentUser-is-string

6e0c32b

Tobbe reviewed Mar 7, 2022

View reviewed changes

Update packages/auth/src/__tests__/AuthProvider.test.tsx

1602a36

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>

dac09 mentioned this pull request Mar 8, 2022

Auth smoke tests #4673

Merged

3 tasks

dac09 added 3 commits March 9, 2022 15:00

Merge branch 'main' into fix/has-roles-when-currentUser-is-string

1cd8bec

Implement the 4 magic role scenarios in useAuth.hasrole

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

a9b0194

Update all lib/auth tempaltes | Regenerate fixture

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

e5bca47

dac09 commented Mar 9, 2022

View reviewed changes

packages/cli/src/commands/setup/auth/templates/dbAuth.auth.ts.template Show resolved Hide resolved

dac09 commented Mar 9, 2022

View reviewed changes

Merge branch 'main' into fix/has-roles-when-currentUser-is-string

6d91d65

Tobbe requested changes Mar 10, 2022

View reviewed changes

dac09 added 3 commits March 10, 2022 15:22

Merge branch 'main' into fix/has-roles-when-currentUser-is-string

fcf9974

Remove check for roles is undefined

f8df5e2

Update comments on all lib/auth templates

f1751a4

Tobbe approved these changes Mar 10, 2022

View reviewed changes

dac09 mentioned this pull request Mar 10, 2022

Allow requireAuth directive to be used with a string or array of strings #4680

Merged

Merge branch 'main' into fix/has-roles-when-currentUser-is-string

3bea205

cannikin approved these changes Mar 10, 2022

View reviewed changes

Merge branch 'main' into fix/has-roles-when-currentUser-is-string

b8531fb

thedavidprice merged commit ead826d into redwoodjs:main Mar 10, 2022

jtoar added this to the next-release milestone Mar 10, 2022

thedavidprice modified the milestones: next-release, v0.50.0 Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(auth): hasRole handles when currentUser.roles is a string #4678

fix(auth): hasRole handles when currentUser.roles is a string #4678

dac09 commented Mar 7, 2022

dac09 commented Mar 7, 2022

dac09 commented Mar 7, 2022

dthyresson commented Mar 7, 2022

dac09 commented Mar 7, 2022

dthyresson commented Mar 7, 2022

Tobbe Mar 7, 2022

dac09 Mar 7, 2022

Tobbe Mar 7, 2022

Tobbe Mar 7, 2022

cannikin Mar 7, 2022

cannikin Mar 7, 2022

cannikin Mar 7, 2022 •

edited

Loading

Tobbe Mar 7, 2022

dac09 Mar 7, 2022

dac09 Mar 9, 2022

dac09 commented Mar 9, 2022

dac09 Mar 9, 2022 •

edited

Loading

dac09 commented Mar 10, 2022

dac09 commented Mar 10, 2022

thedavidprice commented Mar 10, 2022

fix(auth): hasRole handles when currentUser.roles is a string #4678

fix(auth): hasRole handles when currentUser.roles is a string #4678

Conversation

dac09 commented Mar 7, 2022

dac09 commented Mar 7, 2022

dac09 commented Mar 7, 2022

dthyresson commented Mar 7, 2022

dac09 commented Mar 7, 2022

dthyresson commented Mar 7, 2022

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

cannikin Mar 7, 2022 • 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

dac09 commented Mar 9, 2022

dac09 Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

dac09 commented Mar 10, 2022

dac09 commented Mar 10, 2022

thedavidprice commented Mar 10, 2022

cannikin Mar 7, 2022 •

edited

Loading

dac09 Mar 9, 2022 •

edited

Loading