-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix(auth): hasRole handles when currentUser.roles is a string #4678
fix(auth): hasRole handles when currentUser.roles is a string #4678
Conversation
@Tobbe mind taking a look please? This is something Rob and I already fixed on the dbAuth template |
Related #4489 |
@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:
to
so one could use |
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 |
Ok just checking. Thanks, |
packages/auth/src/AuthProvider.tsx
Outdated
if (typeof role === 'string') { | ||
return this.state.currentUser.roles?.includes(role) || false | ||
if (typeof roleToCheck === 'string') { | ||
return this.state.currentUser.roles?.includes(roleToCheck) || false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need an isArray
check on currentUser.roles
here as well? As you added below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ===
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there's something I don't understand about the code here....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Ok gents, I've updated all the templates! |
.replace('select: { id: true }', 'select: { id: true, roles: true }') | ||
.replace( | ||
'const currentUserRoles = context.currentUser?.roles', | ||
'const currentUserRoles = context.currentUser?.roles as string | string[]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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)
I'll wait for @cannikin to do a once over, and then merge! |
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) ...
We had an edge case where we had missing logic in hasRoles.
The updated code now handles the case where: