Page role authentication does not work when using roles instead of role #4275
Conversation
🦋 Changeset detectedLatest commit: 1b64da9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Hey @cmoileo thanks for the PR and interest to contribute to fix this issue!
There are a few issues with the current state of this implementation, I see that you are updating all the usages of publicData.role into publicData.roles which would break all apps that depend on this.
For the completion of this particular issue, we need to implement new handling to authorise correctly either when the user creates the session using role or roles
The only file that would need an implementation change would be packages/blitz-auth/src/client/index.tsx, please clean the PR from these changes, so that there is no possibility of regressions.
I have provided my suggestions. Please feel free to tag me for any confusions and request a re-review when the code is updated.
| if (role.includes(currentRole)) { | ||
| return true | ||
| } | ||
| const authorizeRole = (roles?: string | Array<string>, currentRoles?: string[]) => { |
There was a problem hiding this comment.
🔨 I have not tested it myself, but the api would need to look like this, to handle a case were a single currentRole exists or an array
const authorizeRole = (role?: string | Array<string>, currentRole?: string | Array<string>) => {
if (role && currentRole) {
if (Array.isArray(role) && role.some((r) => currentRole.includes(r))) {
return true
} else {
if (currentRole === role) {
return true
}
}
}
return false
}| @@ -184,7 +185,7 @@ export const useSession = (options: UseSessionOptions = {}): ClientSession => { | |||
| return session | |||
| } | |||
|
|
|||
| export const useAuthorizeIf = (condition?: boolean, role?: string | Array<string>) => { | |||
| export const useAuthorizeIf = (condition?: boolean, roles?: string | Array<string>) => { | |||
There was a problem hiding this comment.
🔨 no changes needed for this function, please revert
| @@ -426,7 +427,7 @@ test("supports reset via resetKeys right after error is triggered on component m | |||
| test("should support not only function as FallbackComponent", () => { | |||
| const consoleError = console.error as MockedFunction<(args: unknown[]) => void> | |||
|
|
|||
| const FancyFallback = forwardRef(({error}: ErrorFallbackProps) => ( | |||
| const FancyFallback = React.forwardRef(({error}: ErrorFallbackProps) => ( | |||
There was a problem hiding this comment.
I had problem passing these tests, forwardRef was not defined. Adding the React indicator fixed that problème.
| @@ -98,7 +98,7 @@ describe("publicDataStore", () => { | |||
| ret = data | |||
| }) | |||
| getPublicDataStore().clear() | |||
| expect(ret).toEqual({userId: null, role: null}) | |||
| expect(ret).toEqual({userId: null, roles: ["user"]}) | |||
There was a problem hiding this comment.
🔨 do not change existing tests.
| "@blitzjs/recipe-vanilla-extract": major | ||
| --- | ||
|
|
||
| I have changed User model in all of Prisma's schemas. I've changed the role key, changing it from a single string to an array of string called "roles". Then, I've changed all of functions where user is created to grant him a default roles array ["user"] instead of a single string "user". I've also changed all of types that where related to User's role and I've changed line 351 of file "blitz/packages/blitz-auth/src/client/index.tsx", to make the condition accept an array of string for roles. |
There was a problem hiding this comment.
🔨 this is a user facing changeset that will be present in the github release, so the text here should be on the user facing changes implemented here
siddhsuresh
changed the title
Co-authored-by: Siddharth Suresh <siddh.suresh@gmail.com>
Co-authored-by: Siddharth Suresh <siddh.suresh@gmail.com>
Co-authored-by: Siddharth Suresh <siddh.suresh@gmail.com>
Co-authored-by: Siddharth Suresh <siddh.suresh@gmail.com>
where it didn't need to be changed
|
Hello @siddhsuresh, thank's for your review ! :) I've removed every changes that are outside the packages/blitz-auth/src/client/index.tsx file, and I've pushed the changes. Don't we need to changes the User model aswell ?I juste reset as it where before. |
@cmoileo no we do not need to change the User model, the These are configurable in the On a basic relook, the build will fail with the current changes, please address it when you can. Thanks! |
|
Hey, going to close this for now. Feel free to open a new PR once everything is fixed. |
What are the changes and their implications?
I have changed the User model to make it able to have several roles instead of a single role.