Open qd-xiaowang opened 2 months ago
Thank you very much. I have learned a lot!
---Original--- From: "Prateek @.> Date: Sat, Apr 20, 2024 07:04 AM To: @.>; Cc: @.**@.>; Subject: Re: [nestjs/docs.nestjs.com] Update guards.md && custom-decorators.md(PR #3000)
@prateekkathal requested changes on this pull request.
In content/custom-decorators.md:
> @@ -118,7 +118,7 @@ import { createParamDecorator, ExecutionContext } from @.***/common'; export const User = createParamDecorator( (data: string, ctx: ExecutionContext) => { const request = ctx.switchToHttp().getRequest(); - const user = request.user; + const user = request.body;
Why would we change this decorator? This is not a decorator that NestJS comes with out of the box. This is a custom decorator that people can define themselves. You are free to create a different decorator to support your use-case.
In content/guards.md:
> } + + private matchRoles(requiredRoles: string[], userRoles: string[]): boolean { + for (const role of requiredRoles) { + if (userRoles.includes(role)) { + return true;
Looks incorrect. This would return true as soon as first role is found. Doesn't match all roles.
In content/guards.md:
> @@ -224,7 +233,16 @@ export class RolesGuard { } const request = context.switchToHttp().getRequest(); const user = request.user; - return matchRoles(roles, user.roles); + return this.matchRoles(roles, user.roles); + } + + private matchRoles(requiredRoles: string[], userRoles: string[]): boolean {
Doesn't look this code is formatted. Kindly fix the formatting and the logic as per my previous message.
In content/guards.md:
> @@ -202,8 +202,17 @@ export class RolesGuard implements CanActivate { } const request = context.switchToHttp().getRequest(); const user = request.user; - return matchRoles(roles, user.roles); + return this.matchRoles(roles, user.roles);
Thanks for adding this. I don't see value in adding a private function here. Developers have there own logic to matchRoles.
I would rather just re-write this to:
return matchRoles(roles, user.roles); // Custom function to match roles
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: @.***>
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
[ ] Other... Please describe:
What is the current behavior?
return matchRoles(roles, user.roles); in guards.md
const user = request.user; in custom-decorators.md
What is the new behavior?
By making this adjustment, Ability to call the matchRoles function.
change request.user to request.body
Does this PR introduce a breaking change?
[x] No
Other information
I think it adds to the understanding
Because if you import directly, an error message will be displayed
error TS2304: Cannot find name 'matchRoles'.
When I was testing the decorator, the undefine firstname appeared