feat: Remove users from rooms when subject attributes no longer match #37506
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 |
|
Pre-merge checks and finishing touches✅ Passed checks (5 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 (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (7)📓 Common learnings📚 Learning: 2025-10-27T14:38:46.994ZApplied to files:
📚 Learning: 2025-10-24T17:32:05.348ZApplied to files:
📚 Learning: 2025-10-06T20:30:45.540ZApplied to files:
📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
🧬 Code graph analysis (2)ee/packages/abac/src/index.spec.ts (1)
ee/packages/abac/src/index.ts (3)
⏰ 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 (6)
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 #37506 +/- ##
============================================
Coverage ? 54.26%
============================================
Files ? 2659
Lines ? 49968
Branches ? 11125
============================================
Hits ? 27113
Misses ? 20724
Partials ? 2131
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: 2
🧹 Nitpick comments (1)
packages/models/src/models/Users.ts (1)
132-138: ABAC setters look correct; consider explicit return typing for clarityThe new
setAbacAttributesById/unsetAbacAttributesByIdwrappers correctly usefindOneAndUpdatewithreturnDocument: 'after'and the expected$set/$unsetshapes. To keep this aligned withIUsersModeland other helpers likesetLastRoutingTime, it would be slightly clearer to give these methods an explicit return type (Promise<IUser | null>orPromise<WithId<IUser> | null>) instead of relying on inference, but behavior-wise this is fine.
📜 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 (5)
ee/packages/abac/src/index.spec.ts(8 hunks)ee/packages/abac/src/index.ts(2 hunks)ee/packages/abac/src/subject-attributes-validations.spec.ts(1 hunks)packages/model-typings/src/models/IUsersModel.ts(1 hunks)packages/models/src/models/Users.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 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
Repo: RocketChat/Rocket.Chat PR: 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
Repo: RocketChat/Rocket.Chat PR: 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:
ee/packages/abac/src/subject-attributes-validations.spec.tsee/packages/abac/src/index.spec.tsee/packages/abac/src/index.ts
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 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:
ee/packages/abac/src/subject-attributes-validations.spec.tsee/packages/abac/src/index.spec.tsee/packages/abac/src/index.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
ee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 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
Repo: RocketChat/Rocket.Chat PR: 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
🧬 Code graph analysis (5)
packages/models/src/models/Users.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)
ee/packages/abac/src/subject-attributes-validations.spec.ts (4)
packages/models/src/index.ts (1)
registerServiceModels(231-265)packages/core-typings/src/IUser.ts (1)
IUser(187-258)ee/packages/abac/src/index.ts (1)
AbacService(16-774)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
packages/model-typings/src/models/IUsersModel.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)
ee/packages/abac/src/index.spec.ts (1)
packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
ee/packages/abac/src/index.ts (3)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)packages/core-services/src/index.ts (1)
Room(171-171)
🔇 Additional comments (5)
packages/model-typings/src/models/IUsersModel.ts (1)
160-162: ABAC user DAO typings align with implementationThe new
setAbacAttributesById/unsetAbacAttributesByIdsignatures fit the intended usage: they take a userId plus attributes and return the updatedIUser | null, matching theUsersRawhelpers and howAbacServiceconsumes them.ee/packages/abac/src/subject-attributes-validations.spec.ts (1)
246-280: Room-removal integration tests nicely validate subject-driven clean-upThe
"removes user from rooms whose attributes become non-compliant after losing a value"scenario accurately mirrors the production logic: you seed rooms with differing ABAC constraints, attach them via__rooms, runaddSubjectAttributes, and assert bothabacAttributesand__roomson the stored user. This is a good high-signal test forbuildRoomNonCompliantConditionsFromSubjectandonSubjectAttributesChanged.ee/packages/abac/src/index.spec.ts (2)
21-23: Mocks and expectations correctly track new ABAC user DAO usageThe updated mocks for
Users.setAbacAttributesById/Users.unsetAbacAttributesByIdand the refactoredgetUpdatedAttributesFromCallensure tests are validating the new API surface instead of the oldupdateOnepath. The merge/dedup tests aroundaddSubjectAttributesremain accurate and now assert calls to the correct DAO methods.Also applies to: 75-101
1259-1397: Comprehensive tests forbuildRoomNonCompliantConditionsFromSubjectThe new
"buildRoomNonCompliantConditionsFromSubject (private)"suite thoroughly exercises the helper: empty attributes, multiple keys, value deduplication, last-wins semantics for duplicate keys, order resilience, and input immutability are all covered. This gives strong confidence that subject-based non-compliance queries used inonSubjectAttributesChangedbehave as intended.ee/packages/abac/src/index.ts (1)
688-773: Subject-driven room cleanup logic and helper look correct and well-scopedThe new
onSubjectAttributesChangedplusbuildRoomNonCompliantConditionsFromSubjectimplement the required behavior:
- Early exit when there is no user id or no rooms.
- When
_nextis empty, remove the user from all rooms inuser.__roomsthat have non-emptyabacAttributes.- When
_nextis non-empty, build$orconditions that:
- Match rooms with ABAC keys the user no longer has (
key: { $nin: userKeys }).- Match rooms where, for keys the user does have, the room requires any value outside the user’s value set (
values: { $elemMatch: { $nin: [...] } }).- Restrict removal to the current
user.__roomsset and usep-limit(20)for bounded concurrency.- Log and swallow errors so a failure in one removal doesn’t crash the service.
This matches the intended semantics of “remove users from rooms they no longer satisfy after attribute changes” and the accompanying tests verify the key scenarios (value loss, key loss, unrelated losses, complete loss).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ee/packages/abac/src/subject-attributes-validations.spec.ts (2)
214-218: Consider extracting the mock to beforeEach for clarity.The mock definition references
usersColwhich is initialized inbeforeEach(line 237). While this works because the mock is only invoked during test execution, moving the mock assignment intobeforeEachwould make the dependency more explicit and improve readability.Apply this diff:
- const originalCoreServices = jest.requireMock('@rocket.chat/core-services'); - originalCoreServices.Room.removeUserFromRoom = async (rid: string, user: IUser) => { - // @ts-expect-error - test - await usersCol.updateOne({ _id: user._id }, { $pull: { __rooms: rid } }); - }; + const originalCoreServices = jest.requireMock('@rocket.chat/core-services'); const insertRoom = async (room: { _id: string; abacAttributes?: IAbacAttributeDefinition[] }) => roomsCol.insertOne({ _id: room._id, name: room._id, t: 'p', abacAttributes: room.abacAttributes, }); const insertUser = async (user: IUser & { __rooms?: string[] }) => usersCol.insertOne({ ...user, __rooms: user.__rooms || [], }); beforeEach(async () => { service = new AbacService(); roomsCol = db.collection('rocketchat_room'); usersCol = db.collection('users'); + originalCoreServices.Room.removeUserFromRoom = async (rid: string, user: IUser) => { + // @ts-expect-error - test + await usersCol.updateOne({ _id: user._id }, { $pull: { __rooms: rid } }); + }; await Promise.all([roomsCol.deleteMany({}), usersCol.deleteMany({})]); });
67-75: Simplify the assertion.The expression
updated?.abacAttributes ?? undefinedis redundant. IfabacAttributesis undefined or null, the?? undefinedhas no effect.Apply this diff:
- expect(updated?.abacAttributes ?? undefined).toBeUndefined(); + expect(updated?.abacAttributes).toBeUndefined();
📜 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)
ee/packages/abac/src/subject-attributes-validations.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 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
Repo: RocketChat/Rocket.Chat PR: 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
Repo: RocketChat/Rocket.Chat PR: 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:
ee/packages/abac/src/subject-attributes-validations.spec.ts
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 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:
ee/packages/abac/src/subject-attributes-validations.spec.ts
🧬 Code graph analysis (1)
ee/packages/abac/src/subject-attributes-validations.spec.ts (3)
packages/models/src/index.ts (1)
registerServiceModels(231-265)packages/core-typings/src/IUser.ts (1)
IUser(187-258)ee/packages/abac/src/index.ts (1)
AbacService(16-774)
🔇 Additional comments (4)
ee/packages/abac/src/subject-attributes-validations.spec.ts (4)
13-23: Past issue resolved: Single teardown confirmed.The double-closing concern from the previous review has been properly addressed. The top-level
afterAllnow handles all resource cleanup, and the redundant innerafterAllhas been removed.
143-194: Excellent coverage of loss detection scenarios.The test suite thoroughly covers attribute loss detection logic including value reduction, complete key removal, value growth (no loss), reordering (order-independent comparison), and duplicate merging. The use of
Setcomparison at line 183 is particularly appropriate for handling order variations.
241-475: Comprehensive coverage of room removal scenarios.The test suite thoroughly validates the
onSubjectAttributesChangedhook behavior across all critical scenarios:
- Attribute value loss causing non-compliance
- Complete attribute key loss
- Attribute growth without triggering removal
- Room attributes requiring keys the user doesn't have
- Complete attribute removal
- Loss of attributes unrelated to room requirements
Each test clearly demonstrates the expected behavior and verifies both database state and room membership changes.
1-476: Well-structured and comprehensive test suite.This test file provides excellent coverage of the
AbacService.addSubjectAttributesfunctionality:Strengths:
- Clear separation between unit tests (attribute manipulation) and integration tests (room removal behavior)
- Comprehensive edge case coverage including merging, deduplication, loss detection, and room removal scenarios
- Good use of helper functions for test data creation
- Proper MongoDB in-memory setup for isolated testing
- Input immutability verification
- Clear test naming that describes expected behavior
The test suite aligns well with the service implementation in
index.tsand validates the complete workflow from LDAP attribute extraction through user removal from non-compliant rooms.
KevLehman
force-pushed
the
feat/on-subject-attr-changed
branch
from
a1da8de to
547f9a5
Compare
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 16:53", "11/19 17:53 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.14]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.14]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.36]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 5 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 19 Nov 2025 17:53:59 GMT |
KevLehman
force-pushed
the
feat/on-subject-attr-changed
branch
from
547f9a5 to
f5f152d
Compare
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/ABAC-54
https://rocketchat.atlassian.net/browse/ABAC-28
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Tests