feat: Update add/join methods to use abac rules #37339
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughAdds a CE no-op hook Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AddFlow as addUserToRoom / addAllUserToRoom
participant HookCE as beforeAddUserToRoom (CE)
participant EEpatch as EE.beforeAddUserToRoom (patch)
participant AbacSvc as AbacService
participant RoomOps as Room Operations
Client->>AddFlow: request to add user(s)
AddFlow->>HookCE: invoke (usernames, room, inviter)
alt EE patch loaded
HookCE->>EEpatch: run prev() and await
EEpatch->>AbacSvc: checkUsernamesMatchAttributes(usernames, room.abacAttributes)
alt non-compliant found
AbacSvc-->>EEpatch: throw MeteorError (details: [usernames])
EEpatch-->>HookCE: propagate error
HookCE-->>AddFlow: error
AddFlow-->>Client: reject with message
else all comply
AbacSvc-->>EEpatch: resolve
EEpatch-->>HookCE: resolve
end
else CE/no-op
HookCE-->>AddFlow: resolve
end
AddFlow->>RoomOps: proceed to add members/subscriptions
RoomOps-->>Client: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (9)📓 Common learnings📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
📚 Learning: 2025-10-28T16:53:42.761ZApplied to files:
📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
📚 Learning: 2025-10-06T20:30:45.540ZApplied to files:
📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37339 +/- ##
=============================================
+ Coverage 70.39% 70.43% +0.03%
=============================================
Files 3050 3050
Lines 105153 105163 +10
Branches 18588 18596 +8
=============================================
+ Hits 74022 74068 +46
+ Misses 29174 29141 -33
+ Partials 1957 1954 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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 (1)
ee/packages/abac/src/index.spec.ts (1)
823-884: Consider adding a test case for non-existent usernames.The current tests cover compliant and non-compliant users, but don't test the scenario where a provided username doesn't exist in the database at all. This would help verify the expected behavior when invalid usernames are passed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
apps/meteor/app/lib/server/functions/addUserToRoom.ts(2 hunks)apps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts(1 hunks)apps/meteor/app/slashcommands-invite/server/server.ts(1 hunks)apps/meteor/ee/server/configuration/abac.ts(1 hunks)apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts(1 hunks)apps/meteor/ee/server/hooks/abac/index.ts(1 hunks)apps/meteor/server/methods/addAllUserToRoom.ts(2 hunks)apps/meteor/tests/end-to-end/api/abac.ts(1 hunks)ee/packages/abac/src/index.spec.ts(3 hunks)ee/packages/abac/src/index.ts(1 hunks)packages/core-services/src/types/IAbacService.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/app/slashcommands-invite/server/server.ts
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
packages/core-services/src/types/IAbacService.tsapps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.tsee/packages/abac/src/index.tsapps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
packages/core-services/src/types/IAbacService.tsapps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.tsee/packages/abac/src/index.tsapps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/methods/addAllUserToRoom.tsapps/meteor/ee/server/hooks/abac/index.tsee/packages/abac/src/index.tsapps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/methods/addAllUserToRoom.tsapps/meteor/ee/server/hooks/abac/index.tsee/packages/abac/src/index.tsapps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
PR: RocketChat/Rocket.Chat#37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/server/methods/addAllUserToRoom.tsapps/meteor/ee/server/hooks/abac/index.tsapps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
ee/packages/abac/src/index.spec.tsapps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
ee/packages/abac/src/index.spec.tsapps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
ee/packages/abac/src/index.spec.tsapps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
ee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
ee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use test.beforeAll() and test.afterAll() for setup and teardown
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts
🧬 Code graph analysis (7)
packages/core-services/src/types/IAbacService.ts (1)
packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
apps/meteor/server/methods/addAllUserToRoom.ts (1)
apps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts (1)
beforeAddUserToRoom(4-6)
apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts (2)
apps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts (1)
beforeAddUserToRoom(4-6)packages/core-services/src/index.ts (2)
License(164-164)Abac(202-202)
ee/packages/abac/src/index.ts (1)
packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
apps/meteor/tests/end-to-end/api/abac.ts (3)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)apps/meteor/tests/e2e/utils/create-target-channel.ts (1)
deleteRoom(48-50)
apps/meteor/app/lib/server/functions/addUserToRoom.ts (2)
apps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts (1)
beforeAddUserToRoom(4-6)apps/meteor/lib/callbacks/beforeAddUserToRoom.ts (1)
beforeAddUserToRoom(5-5)
apps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
⏰ 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). (21)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (5/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (4/5) - Alpine (Official)
- GitHub Check: 🔨 Test API (CE) / MongoDB 7.0 (1/1) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (3/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (2/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (1/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 7.0 [legacy watchers] (3/5) - Alpine (Official)
- GitHub Check: 🔨 Test API (CE) / MongoDB 5.0 (1/1) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 7.0 (1/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 5.0 (4/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 7.0 (2/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 7.0 (4/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 7.0 (3/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 5.0 (2/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (2/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 5.0 (1/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 5.0 (3/4) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (1/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (4/5) - Alpine (Official)
- GitHub Check: 🔨 Test API (EE) / MongoDB 5.0 (1/1) - Alpine (Official)
🔇 Additional comments (14)
apps/meteor/app/slashcommands-invite/server/server.ts (1)
76-84: Good approach to support both Error instances and legacy error objects.The dual-path error handling maintains backward compatibility while supporting new Error instances that may be thrown by ABAC validation hooks. However, the Error instance handling has critical implementation issues (see next comment).
apps/meteor/ee/server/hooks/abac/index.ts (1)
1-1: LGTM!The side-effect import pattern ensures the ABAC hook module is loaded and registered at initialization time, enabling ABAC validation for user additions.
apps/meteor/ee/server/configuration/abac.ts (1)
10-11: LGTM!The dynamic import of ABAC hooks is properly sequenced after settings and permissions setup, ensuring the hooks are only loaded when the ABAC license is active.
apps/meteor/server/methods/addAllUserToRoom.ts (2)
9-9: LGTM!The import enables pre-join ABAC validation for bulk user additions.
54-57: Pass the actor parameter to beforeAddUserToRoom and consider safer null handling.The function receives
userIdas a parameter but omits it when callingbeforeAddUserToRoom. The hook signature expects anactorparameter (line 4 ofapps/meteor/app/lib/server/lib/beforeAddUserToRoom.tsshows_actor?: IUser), and the ABAC hook patches it to accept and forward this parameter. This should be:await beforeAddUserToRoom( users.map((u) => u.username!), room, { _id: userId } as IUser, );Additionally, while no undefined usernames were found empirically, the non-null assertion is defensive. Consider optional chaining or filtering falsy values instead:
.filter((u) => u.username).map((u) => u.username).⛔ Skipped due to learnings
Learnt from: Dnouv PR: RocketChat/Rocket.Chat#37057 File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27 Timestamp: 2025-09-25T09:59:26.461Z Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).Learnt from: Dnouv PR: RocketChat/Rocket.Chat#37057 File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27 Timestamp: 2025-09-25T09:59:26.461Z Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.Learnt from: Dnouv PR: RocketChat/Rocket.Chat#37057 File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27 Timestamp: 2025-09-25T09:59:26.461Z Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.Learnt from: ricardogarim PR: RocketChat/Rocket.Chat#37205 File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301 Timestamp: 2025-10-28T16:53:42.761Z Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.Learnt from: ricardogarim PR: RocketChat/Rocket.Chat#37205 File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301 Timestamp: 2025-10-28T16:53:42.761Z Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.apps/meteor/app/lib/server/functions/addUserToRoom.ts (1)
15-15: LGTM!The alias import distinguishes the patch-based hook from the callback-based hook, supporting the migration strategy.
apps/meteor/tests/end-to-end/api/abac.ts (1)
1415-1485: LGTM! Comprehensive test coverage for ABAC-based room access.The test suite effectively validates:
- Successful invites when user attributes match room requirements
- Blocked invites when user lacks required attributes
- Restored access after room attributes are removed
The test structure follows established patterns with proper setup and cleanup.
packages/core-services/src/types/IAbacService.ts (1)
20-20: LGTM!The new method signature is clear and follows the interface pattern. The
Promise<void>return type appropriately indicates that validation failures will be communicated via thrown errors.apps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts (1)
1-6: LGTM!The patchable hook provides a clean extension point for Enterprise Edition ABAC validation while remaining a no-op in Community Edition. The signature properly accommodates batch user validation with an optional actor parameter.
ee/packages/abac/src/index.spec.ts (2)
17-17: Mock setup looks good.The Users.find mock is properly wired and consistent with existing mock patterns in this test file.
Also applies to: 41-43
885-937: Well-structured tests for the query builder.The tests thoroughly verify that
buildNonCompliantConditionsconstructs the correct MongoDB query structure for finding non-compliant users.ee/packages/abac/src/index.ts (3)
432-443: Query construction logic is correct.The MongoDB query structure properly identifies users who lack the required ABAC attributes using the
$not + $elemMatch + $allpattern.
469-474: Address the TODO comment about error structure.The comment questions whether returning non-compliant usernames via an error's
detailsfield is the right approach. This design decision should be resolved before merging.Current implementation attaches details to a generic Error object, which works but isn't type-safe. Consider:
- Creating a custom error class with a typed
detailsproperty- Removing the TODO if you're confident in the current approach
- Logging details separately and using a generic error message
Which approach aligns with the project's error handling patterns?
445-480: Verify handling of non-existent usernames.The current implementation queries for non-compliant users who exist in the database. If a username in the provided list doesn't exist at all, it won't be returned by the query and won't be flagged as non-compliant.
Is this the intended behavior? Should the method assume that callers have already validated that all usernames exist, or should it explicitly check for non-existent users?
If validation is the caller's responsibility, consider adding a JSDoc comment to document this assumption.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/app/slashcommands-invite/server/server.ts (2)
88-91: Avoid empty backticks when details is invalid.When
e.detailsexists but isn't an array, line 88 setsdetailsto an empty string, resulting in the i18n message receivingdetails: `\`\``. This renders as empty backticks in the UI.Apply this diff to omit the
detailsparameter entirely when invalid:if (isErrorWithDetails(e)) { - const details = Array.isArray(e.details) ? e.details.join(', ') : ''; + const details = e.details.join(', '); void api.broadcast('notify.ephemeralMessage', userId, message.rid, { - msg: i18n.t(e.message, { lng: settings.get('Language') || 'en', details: `\`${details}\`` }), + msg: i18n.t(e.message, { + lng: settings.get('Language') || 'en', + ...(details && { details: `\`${details}\`` }) + }), }); return; }Note: If you apply the fix from the previous comment to strengthen
isErrorWithDetails, theArray.isArraycheck at line 88 becomes redundant and can be removed as shown above.
86-112: Add fallback handling for unexpected error shapes.If an error doesn't match either type guard, it's silently swallowed with no feedback to the user. This can create confusion when some invites fail without explanation.
Apply this diff to add a generic fallback:
if (isStringError(e)) { const { error } = e; if (error === 'error-federated-users-in-non-federated-rooms') { void api.broadcast('notify.ephemeralMessage', userId, message.rid, { msg: i18n.t('You_cannot_add_external_users_to_non_federated_room', { lng: settings.get('Language') || 'en' }), }); } else if (error === 'cant-invite-for-direct-room') { void api.broadcast('notify.ephemeralMessage', userId, message.rid, { msg: i18n.t('Cannot_invite_users_to_direct_rooms', { lng: settings.get('Language') || 'en' }), }); } else { void api.broadcast('notify.ephemeralMessage', userId, message.rid, { msg: i18n.t(error, { lng: settings.get('Language') || 'en' }), }); } + } else { + // Fallback for unexpected error shapes + void api.broadcast('notify.ephemeralMessage', userId, message.rid, { + msg: i18n.t('Error_inviting_user', { + lng: settings.get('Language') || 'en', + username: user.username || 'unknown' + }), + }); } } }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/app/slashcommands-invite/server/server.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/app/slashcommands-invite/server/server.ts
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/slashcommands-invite/server/server.ts (1)
13-19: Tighten the type guards to match their contracts.
isErrorWithDetailsnarrows todetails: string[], but it currently only checks thatdetailsis an array. That lets arbitrary payloads through, breaks the guarantee these helpers promise, and forces the downstreamArray.isArrayrecheck. Please require every entry to be a string. Likewise, add an explicit object check inisStringErrorbefore reading.errorso the guard can’t succeed on primitives by accident. With stronger guards you can rely on the narrowing inside the catch block without additional defensive checks.-function isErrorWithDetails(error: unknown): error is Error & { details: string[] } { - return error instanceof Error && Array.isArray((error as any).details); -} - -function isStringError(error: unknown): error is { error: string } { - return typeof (error as any)?.error === 'string'; -} +function isErrorWithDetails(error: unknown): error is Error & { details: string[] } { + if (!(error instanceof Error)) { + return false; + } + const { details } = error as { details?: unknown }; + return Array.isArray(details) && details.every((detail) => typeof detail === 'string'); +} + +function isStringError(error: unknown): error is { error: string } { + return typeof error === 'object' && error !== null && typeof (error as any).error === 'string'; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/app/slashcommands-invite/server/server.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
KevLehman
force-pushed
the
feat/join-prevent-obj
branch
from
d453c8b to
b5d08f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/app/slashcommands-invite/server/server.ts (1)
13-15: Strengthen type guard to validate array element types.The type guard claims
details: string[]but only checksArray.isArray(details)without verifying that elements are strings. This mismatch forces the defensiveArray.isArraycheck at line 88, undermining the type guard's purpose.Apply this diff to properly validate string array elements:
function isErrorWithDetails(error: unknown): error is Error & { details: string[] } { - return error instanceof Error && Array.isArray((error as any).details); + return error instanceof Error && Array.isArray((error as any).details) && (error as any).details.every((d: unknown) => typeof d === 'string'); }This ensures the type predicate accurately reflects runtime validation, eliminating the need for redundant checks downstream.
🧹 Nitpick comments (1)
ee/packages/abac/src/index.ts (1)
445-475: Consider a custom Error class for structured error details.The validation logic is correct and efficiently queries for non-compliant users. However, attaching a
detailsproperty directly to a standard Error instance (line 467) is informal and loses type safety.For better maintainability, consider creating a custom Error class:
class AbacValidationError extends Error { constructor(message: string, public readonly details: string[]) { super(message); this.name = 'AbacValidationError'; } }Then replace lines 466-468 with:
-const err = new Error('error-usernames-not-matching-abac-attributes'); -(err as any).details = Array.from(nonCompliantSet); -throw err; +throw new AbacValidationError('error-usernames-not-matching-abac-attributes', Array.from(nonCompliantSet));This provides compile-time type safety for consumers who need to access the non-compliant usernames.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
apps/meteor/app/lib/server/functions/addUserToRoom.ts(2 hunks)apps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts(1 hunks)apps/meteor/app/slashcommands-invite/server/server.ts(2 hunks)apps/meteor/ee/server/configuration/abac.ts(1 hunks)apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts(1 hunks)apps/meteor/ee/server/hooks/abac/index.ts(1 hunks)apps/meteor/server/methods/addAllUserToRoom.ts(2 hunks)apps/meteor/tests/end-to-end/api/abac.ts(1 hunks)ee/packages/abac/src/index.spec.ts(3 hunks)ee/packages/abac/src/index.ts(1 hunks)packages/core-services/src/types/IAbacService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/meteor/ee/server/hooks/abac/index.ts
- apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts
- apps/meteor/server/methods/addAllUserToRoom.ts
- apps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts
- apps/meteor/app/lib/server/functions/addUserToRoom.ts
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
packages/core-services/src/types/IAbacService.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/index.ts
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
packages/core-services/src/types/IAbacService.tsapps/meteor/ee/server/configuration/abac.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/index.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/app/slashcommands-invite/server/server.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
apps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/index.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/index.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
ee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
ee/packages/abac/src/index.spec.ts
🧬 Code graph analysis (3)
packages/core-services/src/types/IAbacService.ts (1)
packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
apps/meteor/tests/end-to-end/api/abac.ts (3)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)apps/meteor/tests/e2e/utils/create-target-channel.ts (1)
deleteRoom(48-50)
ee/packages/abac/src/index.ts (1)
packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
apps/meteor/app/slashcommands-invite/server/server.ts (1)
96-111: Error handling logic is sound.The multi-branch error handling correctly distinguishes between specific known errors and generic cases. The logic allows
Promise.allto continue processing other users even when one invitation fails.Note: Line 108 shares the same i18n key validation concern as line 91 (using
errordirectly without checking if it's a valid translation key), though it's less critical here since these error values typically originate from controlled system code.apps/meteor/ee/server/configuration/abac.ts (1)
10-11: LGTM!The dynamic import of ABAC hooks is correctly placed after core setup (settings and permissions) completes, ensuring proper initialization order.
packages/core-services/src/types/IAbacService.ts (1)
20-20: LGTM!The new method signature is clear and appropriate for username-attribute validation. The
Promise<void>return type correctly signals that validation errors are communicated via exceptions.apps/meteor/tests/end-to-end/api/abac.ts (1)
1415-1485: LGTM!The test suite comprehensively validates ABAC enforcement during invitation workflows. The test cases correctly cover:
- Baseline: invitation to non-ABAC room succeeds
- ABAC enforcement: invitation fails when user lacks required attributes
- Attribute removal: invitation succeeds after room loses ABAC requirements
The use of direct DB manipulation (line 1430) is appropriately documented and necessary to avoid triggering ABAC removal during test setup.
ee/packages/abac/src/index.spec.ts (3)
17-17: LGTM!The Users.find mock is correctly integrated into the existing mock factory and returns a cursor-like object that matches the MongoDB Node driver API.
Also applies to: 41-43
823-884: LGTM!The test suite comprehensively covers the
checkUsernamesMatchAttributesmethod:
- Edge cases (empty inputs) correctly skip queries
- Compliant scenario validates the exact MongoDB query structure
- Non-compliant scenario verifies both the error message and the structured details array
The detailed query structure assertion at lines 850-867 is valuable for preventing regressions in the validation logic.
885-937: LGTM!The tests for
buildNonCompliantConditionsthoroughly validate the MongoDB query fragment construction, including:
- Empty input handling
- Correct nesting structure ($not > $elemMatch)
- Use of $all operator for "must have all values" semantics
- Order preservation for multiple attributes
These tests ensure the ABAC query logic remains correct as the codebase evolves.
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/ABAC-25
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores