fix(api): add subscriber WebSocket room permission checks [1209<-1031] #1206
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRole-based access control was added to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant MessageService
participant UserService
participant PermissionService
Client->>Gateway: Socket subscribe request
Gateway->>MessageService: subscribe(req, res)
MessageService->>UserService: findById(userId)
UserService-->>MessageService: user (with roles)
MessageService->>PermissionService: canAccess(method, userRoles, "message")
PermissionService-->>MessageService: true/false
alt Permission granted
MessageService->>Gateway: joinNotificationSockets(sessionId, Room.MESSAGE)
MessageService-->>Client: success response
else Permission denied
MessageService-->>Client: ForbiddenException
end
sequenceDiagram
participant Client
participant Gateway
participant SubscriberService
participant UserService
participant PermissionService
Client->>Gateway: Socket subscribe request
Gateway->>SubscriberService: subscribe(req, res)
SubscriberService->>UserService: findById(userId)
UserService-->>SubscriberService: user (with roles)
SubscriberService->>PermissionService: canAccess(method, userRoles, "subscriber")
PermissionService-->>SubscriberService: true/false
alt Permission granted
SubscriberService->>Gateway: joinNotificationSockets(sessionId, Room.SUBSCRIBER)
SubscriberService-->>Client: success response
else Permission denied
SubscriberService-->>Client: ForbiddenException
end
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
api/src/chat/services/subscriber.service.spec.ts (1)
123-145: Tests cover happy path but missing permission verification.The tests properly verify the gateway calls and response structure, but they don't test the actual permission checking logic. Consider adding tests for scenarios where permission checks fail.
Consider adding tests like:
it('should return 403 when user lacks permission', async () => { // Mock permission service to return false jest.spyOn(permissionService, 'canAccess').mockResolvedValue(false); const [req, res] = buildReqRes('GET', allUsers[0].id); await mockSubscriberService.subscribe(req, res); expect(res.status).toHaveBeenCalledWith(403); expect(mockGateway.joinNotificationSockets).not.toHaveBeenCalled(); });
🧹 Nitpick comments (2)
api/src/user/services/permission.service.ts (1)
86-121: Well-implemented permission checking method with room for improvement.The
canAccessmethod correctly implements role-based permission checking. However, consider these improvements:
- Method parameter typing: The
methodparameter should be more specific- Error handling: The catch block is too generic
- Code readability: The permission filtering logic could be clearer
Apply this diff to improve the method:
async canAccess( - method: string, + method: keyof typeof MethodToAction, userRoles: string[], targetModel: TModel, ): Promise<boolean> { try { const permissions = await this.getPermissions(); if (permissions && userRoles.length) { - const permissionsFromRoles = Object.entries(permissions) - .filter(([key, _]) => userRoles.includes(key)) - .map(([_, value]) => value); + const permissionsFromRoles = userRoles + .map(roleId => permissions[roleId]) + .filter(Boolean); if ( permissionsFromRoles.some((permission) => permission[targetModel]?.includes(MethodToAction[method]), ) ) { return true; } } - } catch (e) { - this.logger.error('Request has no ability to get access', e); + } catch (error) { + this.logger.error( + `Permission check failed for method ${method} on model ${targetModel}`, + error, + ); return false; } return false; }api/src/chat/services/subscriber.service.spec.ts (1)
106-116: Helper function parameter name mismatch.The parameter is named
userIdbut the JSDoc comment sayssubscriberId.Apply this diff to fix the parameter name:
- buildReqRes = (method: 'GET' | 'POST', userId: string) => [ + buildReqRes = (method: 'GET' | 'POST', subscriberId: string) => [ { sessionID: SESSION_ID, method, - session: { passport: { user: { id: userId } } }, + session: { passport: { user: { id: subscriberId } } }, } as SocketRequest,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/src/chat/services/message.service.spec.ts(4 hunks)api/src/chat/services/message.service.ts(3 hunks)api/src/chat/services/subscriber.service.spec.ts(5 hunks)api/src/chat/services/subscriber.service.ts(4 hunks)api/src/user/services/permission.service.ts(3 hunks)api/src/utils/test/fixtures/model.ts(2 hunks)api/src/utils/test/fixtures/permission.ts(2 hunks)api/src/utils/test/fixtures/subscriber.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
api/src/utils/test/fixtures/permission.ts (1)
Learnt from: medchedli
PR: Hexastack/Hexabot#1096
File: api/src/chat/repositories/block.repository.ts:0-0
Timestamp: 2025-07-03T17:41:54.853Z
Learning: In the Hexabot block schema, a block is designed so that it can never simultaneously have both an `attachedBlock` and non-empty `nextBlocks`; the two are mutually exclusive.
api/src/user/services/permission.service.ts (1)
Learnt from: medchedli
PR: Hexastack/Hexabot#1096
File: api/src/chat/repositories/block.repository.ts:0-0
Timestamp: 2025-07-03T17:41:54.853Z
Learning: In the Hexabot block schema, a block is designed so that it can never simultaneously have both an `attachedBlock` and non-empty `nextBlocks`; the two are mutually exclusive.
api/src/utils/test/fixtures/model.ts (1)
Learnt from: medchedli
PR: Hexastack/Hexabot#1096
File: api/src/chat/repositories/block.repository.ts:0-0
Timestamp: 2025-07-03T17:41:54.853Z
Learning: In the Hexabot block schema, a block is designed so that it can never simultaneously have both an `attachedBlock` and non-empty `nextBlocks`; the two are mutually exclusive.
🧬 Code Graph Analysis (6)
api/src/utils/test/fixtures/subscriber.ts (1)
api/src/utils/test/fixtures/permission.ts (1)
installPermissionFixtures(69-87)
api/src/chat/services/subscriber.service.spec.ts (3)
api/src/websocket/utils/socket-request.ts (1)
SocketRequest(20-131)api/src/websocket/utils/socket-response.ts (1)
SocketResponse(11-81)api/src/utils/test/utils.ts (1)
buildTestingMocks(252-305)
api/src/user/services/permission.service.ts (1)
api/src/user/controllers/user.controller.ts (1)
permissions(135-161)
api/src/chat/services/subscriber.service.ts (1)
api/src/user/services/permission.service.ts (1)
canAccess(94-121)
api/src/chat/services/message.service.spec.ts (4)
api/src/websocket/utils/socket-request.ts (1)
SocketRequest(20-131)api/src/websocket/utils/socket-response.ts (1)
SocketResponse(11-81)api/src/utils/test/utils.ts (1)
buildTestingMocks(252-305)api/src/utils/test/fixtures/message.ts (1)
installMessageFixtures(58-71)
api/src/chat/services/message.service.ts (1)
api/src/user/services/permission.service.ts (1)
canAccess(94-121)
⏰ 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). (3)
- GitHub Check: Frontend Tests
- GitHub Check: API-Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (31)
api/src/user/services/permission.service.ts (2)
2-2: Copyright year updated correctly.The copyright year has been updated from 2024 to 2025 as part of the regular maintenance.
25-26: New imports added for permission checking functionality.The imports for
MethodToActionandTModelare correctly added to support the newcanAccessmethod.api/src/utils/test/fixtures/permission.ts (2)
2-2: Copyright year updated correctly.The copyright year has been updated from 2024 to 2025 as part of the regular maintenance.
43-66: Permission fixtures added for Message and Subscriber models.The new permission fixtures correctly provide CREATE and READ permissions for the Message and Subscriber models. These fixtures align with the new permission checking functionality and follow the established pattern.
api/src/utils/test/fixtures/subscriber.ts (2)
18-18: Import correctly updated to use permission fixtures.The import has been appropriately changed from user fixtures to permission fixtures to align with the new permission-based access control system.
109-109: Function call updated to install permission fixtures.The change from
installUserFixtures()toinstallPermissionFixtures()is correct. SinceinstallPermissionFixtures()internally callsinstallUserFixtures()(as shown in the relevant code snippets), theusersvariable is still available while providing the necessary permission data for the new access control system.api/src/utils/test/fixtures/model.ts (2)
2-2: Copyright year updated correctly.The copyright year has been updated from 2024 to 2025 as part of the regular maintenance.
27-38: Model fixtures added for Message and Subscriber.The new model fixtures for Message and Subscriber are correctly structured and follow the established pattern. The identities 'message' and 'subscriber' align with the permission checking logic implemented in the PermissionService.
api/src/chat/services/subscriber.service.spec.ts (2)
21-22: New service imports added for permission checking.The imports for
PermissionService,UserService,SocketRequest, andSocketResponseare correctly added to support the updated testing requirements.Also applies to: 33-34
62-67: Test variables and helper function properly declared.The new test variables for permission and user services are correctly declared, and the
buildReqReshelper function type is well-defined.api/src/chat/services/message.service.spec.ts (9)
11-12: Good addition of required service imports.The imports for
PermissionServiceandUserServiceare correctly added to support the new permission checking functionality in the tests.
26-27: Appropriate imports for WebSocket utilities.The imports for
SocketRequestandSocketResponseare necessary for the newbuildReqReshelper function and improved test structure.
57-62: Well-structured test variable declarations.The addition of
userService,permissionService, andbuildReqResvariables provides good organization for the enhanced test suite.
71-85: Comprehensive service mock setup.The expansion of the
getMocksarray to includeUserServiceandPermissionServiceis correctly implemented to support the new permission checking functionality.
102-107: Constructor injection properly updated.The
mockMessageServiceinstantiation correctly includes the newuserServiceandpermissionServicedependencies, maintaining consistency with the actual service constructor.
108-118: Well-designed helper function for test setup.The
buildReqReshelper function effectively generates mockSocketRequestandSocketResponseobjects with appropriate session and method details, promoting code reuse and consistency across tests.
125-135: Improved test specificity for GET method.The test is now explicitly focused on testing the GET method behavior, which provides better clarity and separation of concerns.
137-147: Good test coverage for POST method.The addition of a separate test case for the POST method ensures comprehensive coverage of both HTTP methods supported by the subscribe endpoint.
66-66: All model dependencies are correctly registeredRoleModel and ModelModel are both defined in api/src/user/schemas/{role,model}.schema.ts and are imported into api/src/user/user.module.ts’s MongooseModule.forFeature. The chat‐service tests include these models so that the permission service’s populate calls (on role and model fields) will work. No further changes are needed.
api/src/chat/services/subscriber.service.ts (6)
9-13: Appropriate exception import for access control.The import of
ForbiddenExceptionis correctly added to support the new permission checking functionality in the subscribe method.
26-27: Required service imports for permission system.The imports for
PermissionServiceandUserServiceare necessary for implementing role-based access control in the WebSocket subscription process.
62-63: Constructor properly updated with new dependencies.The constructor injection of
userServiceandpermissionServiceis correctly implemented to support the new permission checking functionality.
82-94: Robust user validation and role checking.The implementation properly retrieves the user from the session and validates that they have roles before proceeding with permission checks. The error message "User is required!" clearly indicates the access requirement.
96-106: Well-implemented permission checking flow.The use of
permissionService.canAccesswith the request method, user roles, and 'subscriber' target model follows a clear and consistent pattern. The success response includes the appropriate room and status information.
108-108: Appropriate access denial handling.The
ForbiddenExceptionwith the message "Not able to access" provides clear feedback for unauthorized access attempts while maintaining security by not revealing specific permission details.api/src/chat/services/message.service.ts (6)
9-13: Appropriate exception import for access control.The import of
ForbiddenExceptionis correctly added to support the new permission checking functionality in the subscribe method.
15-16: Required service imports for permission system.The imports for
PermissionServiceandUserServiceare necessary for implementing role-based access control in the WebSocket subscription process.
44-45: Constructor properly updated with new dependencies.The constructor injection of
userServiceandpermissionServiceis correctly implemented to support the new permission checking functionality.
63-75: Consistent user validation and permission checking.The implementation follows the same robust pattern as the SubscriberService, properly retrieving the user from the session, validating roles, and checking permissions using the appropriate target model ('message'). The consistency across services ensures a unified security approach.
77-84: Well-implemented success response flow.The success path properly joins the notification sockets for the message room and returns an appropriate response with status 200 and the expected JSON payload structure.
86-86: Appropriate access denial handling.The
ForbiddenExceptionwith the message "Not able to access" provides clear feedback for unauthorized access attempts while maintaining security consistency with the SubscriberService implementation.
yassinedorbozgithub
force-pushed
the
1031-implement-websocket-gateway-unit-tests
branch
from
6281a33 to
40b3fb6
Compare
yassinedorbozgithub
changed the base branch from
main
to
1209-issue---add-authorization-module
yassinedorbozgithub
changed the title
…ent-websocket-gateway-unit-tests
…ent-websocket-gateway-unit-tests
Motivation
This PR introduces permission checks for Web Socket global rooms(subscriber, message)., ensuring that only authorized users can access and interact within them.
Fixes #1031
Demo
Authorization.Module.WebSocket.rooms.integration.webm
Type of change:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests