feat(api): add Authorization Module [main<-1209] #1210
Conversation
|
Warning Rate limit exceeded@yassinedorbozgithub has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
""" WalkthroughA new global Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AbilityGuard
participant AuthorizationService
participant UserService
participant PermissionService
Client->>AbilityGuard: Request with user and HTTP method
AbilityGuard->>AuthorizationService: canAccess(method, userId, model)
AuthorizationService->>UserService: getUserById(userId)
AuthorizationService->>PermissionService: getPermissions()
AuthorizationService-->>AbilityGuard: true/false (access granted/denied)
AbilityGuard-->>Client: Allow or deny request
Assessment against linked issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
yassinedorbozgithub
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
api/src/authorization/authorization.module.ts (1)
21-33: Consider architectural implications of the @global() decorator.Making this module global means all its providers will be available application-wide without explicit imports. While this simplifies usage of
AuthorizationService, it also:
- Reduces module encapsulation
- Makes dependency tracking harder
- Could lead to unintended coupling
Consider whether the
@Global()decorator is truly necessary, or if explicit imports in modules that need authorization would be more maintainable.api/src/authorization/authorization.service.ts (1)
55-58: Improve error handling to distinguish between different failure scenarios.Currently, all errors result in the same log message and return
false. Consider distinguishing between:
- User not found
- Permission loading failures
- Other unexpected errors
This would help with debugging and monitoring.
api/src/user/guards/ability.guard.ts (2)
70-76: Consider handling edge cases for model extraction from pathname.The current logic returns
falseifmodelFromPathnameis undefined. This might block access to valid routes that don't follow the/<model>/...pattern. Consider:
- Root path
/- Routes with prefixes like
/api/v1/<model>/...- Routes with multiple path segments before the model
87-87: Consider preserving the original error for better debugging.The generic error message doesn't provide context about what actually failed. Consider logging the original error or including more specific information.
- } catch (e) { - throw new NotFoundException('Failed to load permissions'); + } catch (error) { + this.logger.error('Failed to check permissions', error); + throw new NotFoundException('Failed to load permissions');Note: You'll need to inject
LoggerServicein the constructor if you implement this suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/src/app.module.ts(2 hunks)api/src/authorization/authorization.module.ts(1 hunks)api/src/authorization/authorization.service.ts(1 hunks)api/src/user/guards/ability.guard.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/src/authorization/authorization.service.ts (1)
api/src/user/controllers/user.controller.ts (1)
permissions(135-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
- GitHub Check: API-Tests
- GitHub Check: Frontend Tests
🔇 Additional comments (2)
api/src/app.module.ts (1)
30-30: LGTM!The
AuthorizationModuleis correctly imported and added to the application's module imports.Also applies to: 64-64
api/src/user/guards/ability.guard.ts (1)
78-88: LGTM! Clean refactoring to use AuthorizationService.The delegation of permission checking to
AuthorizationServiceimproves separation of concerns and makes the guard more maintainable.
| targetModel: TModel, | ||
| ): Promise<boolean> { | ||
| try { | ||
| const user = await this.userService.findOne(userId); |
There was a problem hiding this comment.
Ensure user roles are properly populated.
The findOne method might not populate the user's roles. Based on the code snippet from user.controller.ts, you should use findOneAndPopulate to ensure roles are loaded.
- const user = await this.userService.findOne(userId);
+ const user = await this.userService.findOneAndPopulate(userId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const user = await this.userService.findOne(userId); | |
| const user = await this.userService.findOneAndPopulate(userId); |
🤖 Prompt for AI Agents
In api/src/authorization/authorization.service.ts at line 39, replace the call
to this.userService.findOne(userId) with
this.userService.findOneAndPopulate(userId) to ensure the user's roles are
properly loaded and available for authorization checks.
There was a problem hiding this comment.
We don't need to use findOneAndPopulate because we only use the role IDs for permission checks.
There was a problem hiding this comment.
@medchedli You're absolutely right! Looking at the code, the roles are stored as an array of role IDs in the user document, and the permission check uses user.roles.includes(key) which works directly with the IDs. No need for population since we're only doing ID-based comparisons. Thanks for the clarification!
(\ /)
( ._.)
o_(")(")
learns something new
✏️ Learnings added
Learnt from: medchedli
PR: Hexastack/Hexabot#1210
File: api/src/authorization/authorization.service.ts:42-42
Timestamp: 2025-07-08T15:16:44.888Z
Learning: In the Hexabot codebase, user roles are stored as an array of role IDs in the user document, not as references to role objects. Therefore, `userService.findOne()` is sufficient for authorization checks since `user.roles.includes(key)` works directly with the role IDs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const permissions = await this.permissionService.getPermissions(); | ||
|
|
||
| if (permissions && user?.roles?.length) { | ||
| const permissionsFromRoles = Object.entries(permissions) | ||
| .filter(([key, _]) => user.roles.includes(key)) | ||
| .map(([_, value]) => value); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the structure of permissions returned by getPermissions().
The code assumes that getPermissions() returns an object with role IDs as keys. Please verify this assumption and document the expected return type for clarity.
🏁 Script executed:
#!/bin/bash
# Description: Check the implementation of getPermissions() method to understand its return type
# Search for the getPermissions method implementation
ast-grep --pattern $'getPermissions() {
$$$
}'
# Also search with async
ast-grep --pattern $'async getPermissions() {
$$$
}'
# Look for the method signature in PermissionService
rg -A 10 "getPermissions" api/src/user/services/permission.service.tsLength of output: 549
🏁 Script executed:
#!/bin/bash
# Locate the PermissionsTree type definition
rg -n "type PermissionsTree" -n api
rg -n "interface PermissionsTree" -n api
# Inspect buildTree implementation to see its return shape
rg -A 25 "buildTree" -n api/src/user/services/permission.service.tsLength of output: 932
Document the PermissionsTree return type for clarity
We’ve confirmed that getPermissions() returns a PermissionsTree (i.e., Record<roleId, Record<modelIdentity, string[]>>), mapping role IDs to model-action arrays. To make this explicit for future maintainers, please add an inline type annotation or JSDoc at the call site:
• File: api/src/authorization/authorization.service.ts (around lines 40–45)
import type { PermissionsTree } from '../../user/types/permission.type';
// Explicitly type the returned permissions structure
const permissions: PermissionsTree = await this.permissionService.getPermissions();— or —
/**
* PermissionsTree mapping role IDs to model permissions:
* Record<roleId, Record<modelIdentity, string[]>>
*/
const permissions = await this.permissionService.getPermissions();This documents the expected shape and reinforces the assumption that roles are the top-level keys.
🤖 Prompt for AI Agents
In api/src/authorization/authorization.service.ts around lines 40 to 45, the
variable 'permissions' assigned from getPermissions() lacks an explicit type
annotation. To clarify the expected structure for future maintainers, import the
PermissionsTree type from '../../user/types/permission.type' and annotate the
'permissions' variable with this type, or alternatively add a JSDoc comment
above the assignment describing the PermissionsTree as Record<roleId,
Record<modelIdentity, string[]>>.
|
|
||
| if (permissions && user?.roles?.length) { | ||
| const permissionsFromRoles = Object.entries(permissions) | ||
| .filter(([key, _]) => user.roles.includes(key)) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the type of user.roles for consistent filtering.
The filtering logic on line 44 uses user.roles.includes(key), which suggests user.roles is an array of role IDs. However, if roles are populated objects, this comparison won't work correctly.
🏁 Script executed:
#!/bin/bash
# Description: Check the User schema to understand the structure of roles field
# Search for User schema definition
ast-grep --pattern $'class User {
$$$
roles$$$
$$$
}'
# Also check for @Prop decorator on roles field
rg -B 2 -A 2 "roles.*:" api/src/user/schemas/user.schema.tsLength of output: 510
🏁 Script executed:
#!/bin/bash
# Show lines around the filter usage in authorization.service.ts
rg -n "user.roles.includes" -C 3 api/src/authorization/authorization.service.ts
# Show the containing method signature
rg -n "filter" -C 5 api/src/authorization/authorization.service.ts
# Show imports of User/UserFull in this file
rg -n "import.*User" api/src/authorization/authorization.service.tsLength of output: 1038
#!/bin/bash
# Find UserService and its findOne method signature
rg -n "class UserService" -C 10 api/src/user/services/user.service.ts
rg -n "findOne" -C 5 api/src/user/services/user.service.ts🏁 Script executed:
#!/bin/bash
# Inspect UserService class and findOne method to determine return type and roles field type
rg -n "class UserService" -C 10 api/src/user/services/user.service.ts
rg -n "findOne" -C 5 api/src/user/services/user.service.tsLength of output: 670
#!/bin/bash
# Locate BaseService file
base_service_file=$(fd base-service.ts -t f)
# Display the BaseService class definition
rg -n "export abstract class BaseService" -C 15 "$base_service_file"
# Show the findOne method signature and return type
rg -n -C 15 "findOne" "$base_service_file"#!/bin/bash
# Inspect BaseService to see what type findOne returns
rg -n "export abstract class BaseService" -C 10 api/src/utils/generics/base-service.ts
rg -n "findOne" -C 10 api/src/utils/generics/base-service.ts
# Inspect UserRepository to see if findOne is overridden or has custom logic
rg -n "export class UserRepository" -C 10 api/src/user/repositories/user.repository.ts
rg -n "findOne" -C 10 api/src/user/repositories/user.repository.tsNormalize user.roles to string[] before filtering
UserService.findOne returns a full user (UserFull) where roles is an array of Role objects, so calling user.roles.includes(key) (where key is a string ID) will never match. Extract the IDs as strings first:
- File:
api/src/authorization/authorization.service.ts, around line 44
- const permissionsFromRoles = Object.entries(permissions)
- .filter(([key, _]) => user.roles.includes(key))
- .map(([_, value]) => value);
+ // extract string IDs from Role objects (or use directly if already string[])
+ const roleIds = user.roles.map(r => typeof r === 'string' ? r : r._id.toString());
+ const permissionsFromRoles = Object.entries(permissions)
+ .filter(([key]) => roleIds.includes(key))
+ .map(([_, value]) => value);Ensure that user.roles is always a string[] (or converted to one) before using .includes.
🤖 Prompt for AI Agents
In api/src/authorization/authorization.service.ts at line 44, the code calls
user.roles.includes(key) but user.roles is an array of Role objects, not
strings, so the includes check will fail. To fix this, map user.roles to an
array of role ID strings before the filter call, ensuring you compare key
against a string array of role IDs rather than Role objects.
| try { | ||
| return await this.authorizationService.canAccess( | ||
| method, | ||
| user.id, | ||
| targetModel, | ||
| ); | ||
| } catch (e) { | ||
| throw new NotFoundException('Failed to load permissions'); | ||
| } |
There was a problem hiding this comment.
I suggest we use throw ForbiddenException if canAccess returns false
| try { | |
| return await this.authorizationService.canAccess( | |
| method, | |
| user.id, | |
| targetModel, | |
| ); | |
| } catch (e) { | |
| throw new NotFoundException('Failed to load permissions'); | |
| } | |
| try { | |
| const accessGranted = await this.authorizationService.canAccess( | |
| method, | |
| user.id, | |
| targetModel, | |
| ); | |
| if (!accessGranted) { | |
| throw new ForbiddenException( | |
| 'You do not have permission to access this resource', | |
| ); | |
| } | |
| return true; | |
| } catch (e) { | |
| throw new InternalServerErrorException( | |
| 'An unexpected error occurred during authorization', | |
| ); | |
| } |
| targetModel: TModel, | ||
| ): Promise<boolean> { | ||
| try { | ||
| const user = await this.userService.findOne(userId); |
There was a problem hiding this comment.
We don't need to use findOneAndPopulate because we only use the role IDs for permission checks.
| const permissionsFromRoles = Object.entries(permissions) | ||
| .filter(([key, _]) => user.roles.includes(key)) | ||
| .map(([_, value]) => value); | ||
| const targetModel = _parsedUrl?.pathname?.split('/')[1].toLowerCase() as |
There was a problem hiding this comment.
I suggest we tackle this in a different PR, but it's worth mentioning that we can improve the curent implementation by using the built-in @SetMetadata() decorator from NestJS. This decorator will allow us to attach custom metadata to our controller routes to indicate the database model associated with the route and also the required user roles or permissions needed to access it. https://docs.nestjs.com/fundamentals/execution-context#reflection-and-metadata
2 participants
Motivation
The main motivation of this PR is to add an Authorization Module that can manage access
Fixes #1209
Demo
Authorization.Module.REST.integration.webm
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor