feat: Remove users from room when new attributes are added to the room #37172
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 |
|
WalkthroughThis pull request implements automatic user removal from ABAC-protected rooms when new attributes are added. It extends the Changes
Sequence DiagramsequenceDiagram
actor Admin
participant RoomService
participant AbacService
participant Room DB
participant Users
Admin->>RoomService: setRoomAbacAttributes(roomId, newAttrs)
RoomService->>Room DB: Update ABAC attributes
Room DB-->>RoomService: Updated room
RoomService->>AbacService: onRoomAttributesChanged(room, newAttrs)
rect rgb(200, 220, 240)
Note over AbacService: Detect changes
AbacService->>AbacService: didAttributesChange(current, next)
end
alt Attributes added
AbacService->>AbacService: buildNonCompliantConditions(room, newAttrs)
AbacService->>Users: Query ABAC definitions & user attributes
Users-->>AbacService: User compliance results
rect rgb(240, 200, 200)
Note over AbacService: Remove non-compliant users
end
par Concurrent removals (p-limit: 20)
AbacService->>RoomService: removeUserFromRoom(roomId, user, {skipAppPreEvents: true, customSystemMessage: 'abac-removed-user-from-room'})
RoomService->>Room DB: Remove user subscription
Room DB-->>RoomService: Success
end
AbacService-->>RoomService: Removal complete
else No attribute changes
Note over AbacService: No-op, return early
end
RoomService-->>Admin: Success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Rationale: The changes introduce new ABAC lifecycle logic with compliance reevaluation, concurrency control, and user removal orchestration across multiple files. While changes are cohesive and focused on ABAC attribute management, reviewers must evaluate: (1) correctness of compliance detection and removal logic, (2) concurrency handling and race conditions, (3) integration with existing room/user removal flows, (4) test coverage adequacy, and (5) consistency across service layer contracts. The addition of comprehensive integration tests and clear separation of concerns moderates complexity. Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37172 +/- ##
=============================================
+ Coverage 70.09% 70.16% +0.07%
=============================================
Files 3035 3035
Lines 104228 104266 +38
Branches 18487 18499 +12
=============================================
+ Hits 73055 73155 +100
+ Misses 29228 29172 -56
+ Partials 1945 1939 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
KevLehman
force-pushed
the
feat/on-attribute-changed-hooks
branch
from
9d0dfe9 to
2e1e8e3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
packages/apps-engine/src/definition/messages/MessageType.ts (1)
143-145: New message type looks good; minor comment style nitUse a proper JSDoc block and “ABAC” capitalization for clarity.
-// ** Sent when a user was removed from an abac room */ +/** Sent when a user was removed due to ABAC policy */ee/packages/abac/src/index.ts (2)
435-446: Tighten Users.find projection for performanceOnly _id and username are required downstream. Avoid fetching entire user docs.
-const cursor = Users.find(query, { projection: { __rooms: 0 } }); +const cursor = Users.find(query, { projection: { _id: 1, username: 1 } });
405-410: Minor: fix comment typo“Should w remove all members?” → “Should we remove all members?”
ee/packages/abac/src/user-auto-removal.spec.ts (4)
9-18: Capture and assert Room.removeUserFromRoom options (use jest.fn).The PR adds skipAppPreEvents/customSystemMessage. Make the mock a jest.fn to capture the 3rd param and allow assertions that ABAC removals set those flags.
Apply this diff to the mock:
-jest.mock('@rocket.chat/core-services', () => ({ - ServiceClass: class {}, - Room: { - // Mimic the DB side-effects of removing a user from a room (no apps/system messages) - removeUserFromRoom: async (roomId: string, user: any) => { - const { Subscriptions } = await import('@rocket.chat/models'); - await Subscriptions.removeByRoomIdAndUserId(roomId, user._id); - }, - }, -})); +jest.mock('@rocket.chat/core-services', () => { + const removeUserFromRoom = jest.fn(async (roomId: string, user: any, options?: any) => { + const { Subscriptions } = await import('@rocket.chat/models'); + await Subscriptions.removeByRoomIdAndUserId(roomId, user._id); + // options captured implicitly by jest.fn for assertions + }); + return { + ServiceClass: class {}, + Room: { removeUserFromRoom }, + }; +});Then, in one of the tests after calling the service, assert the options (example):
const { Room } = await import('@rocket.chat/core-services'); expect(Room.removeUserFromRoom).toHaveBeenCalledWith( expect.any(String), expect.objectContaining({ _id: expect.any(String) }), expect.objectContaining({ skipAppPreEvents: true, customSystemMessage: 'abac-removed-user-from-room' }), );
31-43: Reuse the shared AbacService instance to add definitions.Avoid spinning extra AbacService instances; reuse the spied instance so logs/behavior are consistent and test setup is lighter.
-const insertDefinitions = async (defs: { key: string; values: string[] }[]) => { - const svc = new AbacService(); +const insertDefinitions = async (defs: { key: string; values: string[] }[]) => { await Promise.all( defs.map((def) => - svc.addAbacAttribute({ key: def.key, values: def.values }).catch((e: any) => { + service.addAbacAttribute({ key: def.key, values: def.values }).catch((e: any) => { if (e instanceof Error && e.message === 'error-duplicate-attribute-key') { return; } throw e; }), ), ); };
116-120: Remove shadowed debugSpy; use the outer spy instance.Local const debugSpy redefines the outer spy and can confuse readers. Rely on the outer jest.SpyInstance everywhere.
-const debugSpy = (service as any).logger.debug as jest.Mock;Repeat this removal in similar tests; the outer
debugSpyalready exposes.mock.callsand.mockClear().
120-128: DRY up log extraction for reevaluation events.The mapping/filtering repeats. Extract a tiny helper for readability.
const getReevalCalls = () => debugSpy.mock.calls.map((c) => c[0]).filter((a) => a && a.msg === 'Re-evaluating room subscriptions');Then use
const evaluationCalls = getReevalCalls();in tests.Also applies to: 150-159, 173-181, 199-206, 260-271, 284-315, 330-335, 351-359, 390-396
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts(3 hunks)apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts(1 hunks)apps/meteor/server/services/room/service.ts(2 hunks)ee/packages/abac/package.json(1 hunks)ee/packages/abac/src/index.spec.ts(6 hunks)ee/packages/abac/src/index.ts(10 hunks)ee/packages/abac/src/user-auto-removal.spec.ts(1 hunks)packages/apps-engine/src/definition/messages/MessageType.ts(1 hunks)packages/core-services/src/types/IRoomService.ts(2 hunks)packages/core-typings/src/IMessage/IMessage.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)packages/message-types/src/registrations/common.ts(1 hunks)
🔇 Additional comments (8)
packages/core-typings/src/IMessage/IMessage.ts (1)
107-107: LGTM! New ABAC message type follows naming conventions.The new message type literal
'abac-removed-user-from-room'is correctly added to theMessageTypesarray and follows the established kebab-case naming convention consistent with other system messages like'removed-user-from-team'.packages/message-types/src/registrations/common.ts (1)
204-208: Verify username parameter is correctly passed to the translation function.The registration doesn't pass the username to the translation function, but similar removal messages (like
'removed-user-from-team'at line 43) extract the username frommessage.msgand pass it as a parameter. The AI summary mentions the i18n key'abac_removed__username__from_the_room'which suggests a username placeholder is expected.If the localization string includes a
{username}or{user_removed}placeholder, apply this diff:instance.registerType({ id: 'abac-removed-user-from-room', system: true, - text: (t) => t('abac_removed_user_from_the_room'), + text: (t, message) => t('abac_removed_user_from_the_room', { username: message.msg }), });Run the following script to verify the i18n key format:
ee/packages/abac/src/index.spec.ts (1)
416-426: Tests capture the “re-evaluate on additions only” policy wellGood coverage for new/added values vs removals and replace/add flows.
Also applies to: 428-439, 441-452, 529-534, 536-542, 571-581
ee/packages/abac/src/index.ts (1)
197-219: didAttributesChange logic is clear and matches policyReturns true only on key/value additions; good guard to avoid unnecessary removals.
apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts (1)
158-160: ABAC export text: verify i18n key and consider italic formatting
- Confirm the i18n key used here matches the registration/locales (some files reference abac_removed__username__from_the_room vs abac_removed_user_from_the_room).
- Consider adding this message type to italicTypes for consistent HTML export styling like 'ru' and 'ul'.
Run to verify key consistency across registration and locales:
Refactor (outside the selected lines) to italicize the ABAC message in HTML export:
-const italicTypes: IMessage['t'][] = ['uj', 'ul', 'au', 'r', 'ru', 'wm', 'livechat-close']; +const italicTypes: IMessage['t'][] = ['uj', 'ul', 'au', 'r', 'ru', 'wm', 'livechat-close', 'abac-removed-user-from-room'];packages/core-services/src/types/IRoomService.ts (1)
44-48: Public typings update LGTM; ensure implementors alignThe options surface looks right. Please verify all implementors use byUser?: Pick<IUser, '_id' | 'username'>.
ee/packages/abac/src/user-auto-removal.spec.ts (2)
370-404: Nice large-population sanity test.Good balance between coverage and runtime; assertions are clear and deterministic.
20-26: Overall: solid integration harness and coverage.Setup/teardown are clean; scenarios match PR intent (new attributes tighten access). Assertions on memberships and idempotency look good.
Please add one assertion somewhere (after a removal) verifying Room.removeUserFromRoom was called with the ABAC-specific options once you switch the mock to jest.fn (see earlier comment).
Also applies to: 45-75, 83-92, 99-103
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ee/packages/abac/src/index.ts (4)
1-10: Good concurrency control setup.The addition of
p-limitto control concurrent user removals is a solid reliability pattern. A limit of 20 concurrent operations should prevent server overload while maintaining reasonable throughput.Consider making the concurrency limit configurable via environment variable or system setting if you need to tune it based on production load characteristics or server capacity.
197-218: Consider renamingprevMaptocurrentMapfor consistency.The logic correctly implements the "additions only" policy described in the PR objectives. However, the variable
prevMapis built from thecurrentparameter, creating a slight naming inconsistency.Apply this diff to improve clarity:
private didAttributesChange(current: IAbacAttributeDefinition[], next: IAbacAttributeDefinition[]) { let added = false; - const prevMap = new Map(current.map((a) => [a.key, new Set(a.values)])); + const currentMap = new Map(current.map((a) => [a.key, new Set(a.values)])); for (const { key, values } of next) { - const prevValues = prevMap.get(key); - if (!prevValues) { + const currentValues = currentMap.get(key); + if (!currentValues) { added = true; break; } for (const v of values) { - if (!prevValues.has(v)) { + if (!currentValues.has(v)) { added = true; break; }
468-468: UsePromise.allSettledinstead ofPromise.allfor resilience.
Promise.allfails fast on the first rejection, meaning if one user removal fails, the remaining removals won't complete. This could leave the room in an inconsistent state with some non-compliant users not removed.Apply this diff:
- await Promise.all(userRemovalPromises); + const results = await Promise.allSettled(userRemovalPromises); + const failures = results.filter((r) => r.status === 'rejected'); + if (failures.length > 0) { + this.logger.warn({ + msg: 'Some user removals failed', + rid, + failureCount: failures.length, + totalAttempted: results.length, + }); + }
428-430: Redundant check can be removed.The check
if (!nonCompliantConditions.length)is redundant because:
nonCompliantConditionsis created by mapping overnewAttributes- Line 406 already guards against empty/falsy
newAttributeswith an early return- If we reach line 428,
newAttributes.length > 0, sononCompliantConditions.length > 0You can safely remove lines 428-430.
📜 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/index.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/abac/src/index.ts (3)
packages/core-services/src/index.ts (2)
IAbacService(56-56)Room(171-171)packages/core-services/src/types/IAbacService.ts (1)
IAbacService(3-20)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 (4)
ee/packages/abac/src/index.ts (4)
15-20: LGTM!Proper logger initialization following standard patterns. The structured logging throughout the service improves observability.
113-116: Excellent logging additions.Replacing inline operations with structured logger calls significantly improves observability. The contextual information (room IDs, attributes, operations) will be valuable for debugging and auditing ABAC operations.
Also applies to: 137-140, 186-192, 288-292, 310-316, 334-338, 361-365, 382-386, 397-401
437-450: Performance testing for large rooms required. The code streams users with an async iterator and throttles removals via p-limit, but please load-test scenarios with thousands of users to confirm it completes within acceptable time.
417-436: Query correctly implements ABAC non-compliance. The$orof$not: { $elemMatch: { key, values: { $all: values } } }conditions will remove any user missing at least one required attribute, matching the intended “must satisfy all” semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
ee/packages/abac/src/index.ts (9)
10-10: Rename concurrency limiter to avoid shadowing and confusion with pagination limitlimit from p-limit shadows local limit used in listAbacAttributes, which hurts readability and risks mistakes.
-const limit = pLimit(20); +const withConcurrency = pLimit(20);- limit(() => + withConcurrency(() =>Also applies to: 442-442
460-465: Reduce PII/noise in logs; log counts instead of full user ID arraysLogging full usersToRemove leaks PII and can explode log volume for large rooms. Prefer counts.
- this.logger.debug({ - msg: 'Re-evaluating room subscriptions', - rid, - newAttributes, - usersThatWillBeRemoved: usersToRemove, - }); + this.logger.debug({ + msg: 'Re-evaluating room subscriptions', + rid, + newAttributes, + usersRemovalCount: usersToRemove.length, + });
467-467: Make removals resilient; avoid failing the whole batch on a single errorPromise.all fails fast. Use Promise.allSettled so one failed removal doesn’t abort the rest.
- await Promise.all(userRemovalPromises); + await Promise.allSettled(userRemovalPromises);
438-438: Type the promises array for clarityAvoid any[] and keep type-safety.
- const userRemovalPromises = []; + const userRemovalPromises: Array<Promise<unknown>> = [];
197-197: Add explicit return typeBe explicit for maintainability.
- private didAttributesChange(current: IAbacAttributeDefinition[], next: IAbacAttributeDefinition[]) { + private didAttributesChange(current: IAbacAttributeDefinition[], next: IAbacAttributeDefinition[]): boolean {
184-195: Optional: sanitize attribute values in logsDebug logs include full previous/new attribute values. If values may be sensitive, prefer logging keys/counts only.
9-10: Concurrency cap: consider configurabilityA fixed cap of 20 may be suboptimal across deployments. Consider making it configurable (env/setting) or deriving from CPU count.
427-430: Redundant length checknonCompliantConditions is derived directly from newAttributes (already checked for length). The extra length guard is redundant.
436-449: Narrow Users.find projection
The call currently fetches full user documents (excluding __rooms) butremoveUserFromRoomonly readsuser._idanduser.username. Change to:const cursor = Users.find(query, { projection: { _id: 1, username: 1 } });
📜 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/index.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/abac/src/index.ts (3)
packages/core-services/src/index.ts (2)
IAbacService(56-56)Room(171-171)packages/core-services/src/types/IAbacService.ts (1)
IAbacService(3-20)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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ee/packages/abac/src/index.ts (1)
381-393: Resolved: gated re-evaluation on replace to only when stricter (new values added)This addresses the earlier review concern. Looks good.
🧹 Nitpick comments (4)
ee/packages/abac/src/index.ts (4)
411-482: Stream removals and use allSettled to bound memory and improve resiliencePushing every removal promise into an array can OOM large rooms; a single failure also rejects the whole batch. Stream/batch with a cap and use Promise.allSettled.
- const cursor = Users.find(query, { projection: { __rooms: 0 } }); - const usersToRemove: string[] = []; - const userRemovalPromises = []; - for await (const doc of cursor) { - usersToRemove.push(doc._id); - userRemovalPromises.push( - limit(() => - Room.removeUserFromRoom(rid, doc, { - skipAppPreEvents: true, - customSystemMessage: 'abac-removed-user-from-room' as const, - }), - ), - ); - } + const cursor = Users.find(query, { projection: { __rooms: 0 } }); + const usersToRemove: string[] = []; + const pending: Promise<unknown>[] = []; + const BATCH = 200; + for await (const doc of cursor) { + usersToRemove.push(doc._id); + pending.push( + removeOpsLimit(() => + Room.removeUserFromRoom(rid, doc, { + skipAppPreEvents: true, + customSystemMessage: 'abac-removed-user-from-room' as const, + }), + ), + ); + if (pending.length >= BATCH) { + await Promise.allSettled(pending); + pending.length = 0; + } + } @@ - this.logger.debug({ - msg: 'Re-evaluating room subscriptions', - rid, - newAttributes, - usersThatWillBeRemoved: usersToRemove, - }); + this.logger.debug({ + msg: 'Re-evaluating room subscriptions', + rid, + newAttributes, + usersToRemoveCount: usersToRemove.length, + }); @@ - await Promise.all(userRemovalPromises); + if (pending.length) { + await Promise.allSettled(pending); + }Notes:
- Consider projecting only fields required by Room.removeUserFromRoom (e.g., _id, username) to reduce payloads if API allows.
9-10: Rename p-limit handle to avoid confusion with local “limit” variablesThere’s a method-scoped const limit used for pagination (Line 52), which shadows the module-level p-limit const. Rename the p-limit handle for clarity.
-const limit = pLimit(20); +const removeOpsLimit = pLimit(20); @@ - limit(() => + removeOpsLimit(() =>Also applies to: 449-455
197-218: didAttributesChange only detects stricter changes (new keys/values) — clarify intentLogic is correct for “only re-eval when stricter.” Consider renaming to isMoreRestrictive or add a short comment for future readers.
298-305: Avoid unnecessary writes on value re-order; normalize before compareOrder-only changes hit the DB (intended) but can be noisy. Normalize arrays (e.g., sort unique) before comparing and persisting.
- if (prevValues.length === values.length && prevValues.every((v, i) => v === values[i])) { + const normPrev = [...prevValues].sort(); + const normNext = [...values].sort(); + if (normPrev.length === normNext.length && normPrev.every((v, i) => v === normNext[i])) { return; } @@ - const prevSet = new Set(prevValues); - const added = values.some((v) => !prevSet.has(v)); + const prevSet = new Set(normPrev); + const added = normNext.some((v) => !prevSet.has(v)); @@ - await Rooms.updateAbacAttributeValuesArrayFilteredById(rid, key, values); + await Rooms.updateAbacAttributeValuesArrayFilteredById(rid, key, normNext); @@ - const next = previous.map((a, i) => (i === existingIndex ? { key, values } : a)); + const next = previous.map((a, i) => (i === existingIndex ? { key, values: normNext } : a));Also applies to: 307-317
📜 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/index.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/abac/src/index.ts (3)
packages/core-services/src/index.ts (2)
IAbacService(56-56)Room(171-171)packages/core-services/src/types/IAbacService.ts (1)
IAbacService(3-20)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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
ee/packages/abac/src/index.ts (1)
184-195: LGTM: setRoomAbacAttributes re-eval gated by didAttributesChange with clear loggingMatches the PR’s intent and avoids unnecessary scans.
KevLehman
force-pushed
the
feat/on-attribute-changed-hooks
branch
from
d8b0d8a to
a99bcbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (1)
13-17: Address the byUser type mismatch.As noted in previous reviews,
byUser?: IUsershould bebyUser?: Pick<IUser, '_id' | 'username'>to match theIRoomServiceinterface and simplify caller requirements.apps/meteor/server/services/room/service.ts (1)
87-93: Address the byUser type mismatch.As flagged in previous reviews,
byUser?: IUsershould bebyUser?: Pick<IUser, '_id' | 'username'>to match theIRoomServiceinterface declaration.
🧹 Nitpick comments (1)
apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts (1)
176-176: Consider adding 'abac-removed-user-from-room' to italicTypes.The new ABAC removal message is a system-automated action, similar to other message types in the
italicTypesarray (e.g., 'ru', 'au', 'ul', 'uj'). For consistent HTML export formatting, consider including it.Apply this diff:
-const italicTypes: IMessage['t'][] = ['uj', 'ul', 'au', 'r', 'ru', 'wm', 'livechat-close']; +const italicTypes: IMessage['t'][] = ['uj', 'ul', 'au', 'r', 'ru', 'wm', 'livechat-close', 'abac-removed-user-from-room'];
📜 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 (12)
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts(3 hunks)apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts(1 hunks)apps/meteor/server/services/room/service.ts(2 hunks)ee/packages/abac/package.json(1 hunks)ee/packages/abac/src/index.spec.ts(6 hunks)ee/packages/abac/src/index.ts(10 hunks)ee/packages/abac/src/user-auto-removal.spec.ts(1 hunks)packages/apps-engine/src/definition/messages/MessageType.ts(1 hunks)packages/core-services/src/types/IRoomService.ts(2 hunks)packages/core-typings/src/IMessage/IMessage.ts(1 hunks)packages/http-router/src/Router.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core-typings/src/IMessage/IMessage.ts
- packages/apps-engine/src/definition/messages/MessageType.ts
- ee/packages/abac/package.json
🧰 Additional context used
🧬 Code graph analysis (6)
packages/core-services/src/types/IRoomService.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
MessageTypesValues(114-114)
apps/meteor/server/services/room/service.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
MessageTypesValues(114-114)
ee/packages/abac/src/user-auto-removal.spec.ts (5)
ee/packages/abac/src/index.ts (1)
AbacService(13-487)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)packages/models/src/index.ts (1)
registerServiceModels(231-265)
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (2)
apps/meteor/server/services/room/service.ts (1)
removeUserFromRoom(87-93)packages/core-typings/src/IMessage/IMessage.ts (1)
MessageTypesValues(114-114)
ee/packages/abac/src/index.ts (3)
packages/core-services/src/index.ts (2)
IAbacService(56-56)Room(171-171)packages/core-services/src/types/IAbacService.ts (1)
IAbacService(3-20)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
packages/http-router/src/Router.ts (1)
apps/meteor/tests/e2e/page-objects/fragments/message.ts (1)
body(6-8)
🪛 Biome (2.1.2)
ee/packages/abac/src/user-auto-removal.spec.ts
[error] 10-10: Expected an identifier, an array pattern, or an object pattern but instead found 'class'.
Expected an identifier, an array pattern, or an object pattern here.
(parse)
[error] 10-10: expected : but instead found {
Remove {
(parse)
[error] 13-13: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 13-13: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 14-14: await is only allowed within async functions and at the top levels of modules.
(parse)
[error] 15-15: await is only allowed within async functions and at the top levels of modules.
(parse)
[error] 17-17: Expected an expression but instead found '}'.
Expected an expression here.
(parse)
[error] 10-10: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ 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). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (10)
packages/http-router/src/Router.ts (1)
267-267: LGTM! Improved test-mode debugging.Adding the actual response body to the validation failure log provides valuable debugging context when investigating schema mismatches during testing.
apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts (1)
158-160: LGTM! New ABAC message type correctly handled.The new case follows the established pattern for system message types and uses the appropriate i18n key.
packages/core-services/src/types/IRoomService.ts (1)
1-1: LGTM! Clean API signature update.The type narrowing for
byUsertoPick<IUser, '_id' | 'username'>is appropriate for a public API boundary, and the new optional fields (skipAppPreEvents,customSystemMessage) are well-typed and support the ABAC re-evaluation feature.Also applies to: 44-48
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (1)
24-35: LGTM! Pre-event gating and custom message handling are correct.The conditional logic correctly:
- Skips app pre-events when
skipAppPreEventsis true (needed for ABAC auto-removal)- Preserves post-event triggering at line 89
- Applies custom system messages when provided
Also applies to: 48-50
ee/packages/abac/src/index.spec.ts (1)
419-429: Excellent test coverage for hook triggering logic.The updated test expectations correctly verify that
onRoomAttributesChanged:
- Fires when new attributes or values are added (more restrictive)
- Does NOT fire when attributes or values are removed (less restrictive)
This aligns perfectly with the PR objectives to re-evaluate only when room constraints become stricter.
Also applies to: 431-455, 532-545, 574-584
ee/packages/abac/src/index.ts (3)
1-11: LGTM! Proper concurrency control and logging setup.The addition of
p-limitwith a 20-concurrent limit and Logger initialization provides good control over resource usage during bulk user removals.Also applies to: 16-21
201-222: Clean helper for detecting restrictive changes.The
didAttributesChangemethod correctly identifies when attributes become more restrictive by detecting:
- New attribute keys
- New values added to existing keys
This properly gates re-evaluation to only when needed.
415-486: Solid re-evaluation implementation with good error handling.The
onRoomAttributesChangedimplementation:
- ✅ Correctly builds non-compliance conditions
- ✅ Uses controlled concurrency with p-limit
- ✅ Skips app pre-events for ABAC removals (line 455)
- ✅ Uses custom system message 'abac-removed-user-from-room' (line 456)
- ✅ Proper error handling and logging
Note: Index optimization for the
$orquery at lines 442-445 is acknowledged in past reviews as deferred to when production data warrants it.ee/packages/abac/src/user-auto-removal.spec.ts (2)
9-18: Ignore static analysis false positives.The Biome parser errors are false positives related to the jest.mock factory function syntax, which is valid Jest code. The mock correctly simulates the subscription removal side effect.
104-404: Excellent integration test coverage.The test suite comprehensively validates:
- ✅ Correct user removal when attributes become more restrictive
- ✅ No removal when attributes become less restrictive
- ✅ AND semantics across multiple attributes
- ✅ Idempotency (no duplicate removals)
- ✅ Edge cases (supersets, missing keys, no ABAC attributes)
- ✅ Performance sanity check with 300 users
Each test verifies both the logging payload and actual membership changes, ensuring end-to-end correctness.
KevLehman
force-pushed
the
feat/on-attribute-changed-hooks
branch
from
e3409c8 to
1d0e210
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/models/src/models/Rooms.ts (1)
966-972: Good genericization; avoid theas anycast for stronger type-safetyThe API change is sound and backward compatible. However, the
as anyon the filter hides mismatches between the collection schema (IRoom) and arbitrary T.Consider either:
- Constrain T to extend IRoom and keep FindOptions, casting the return only, or
- Provide overloads (default returning IRoom, projected variant returning T) so the filter stays Filter.
Example adjustment to drop
as any:-findOneByIdAndType<T extends Document = IRoom>( +findOneByIdAndType<T extends IRoom = IRoom>( roomId: IRoom['_id'], type: IRoom['t'], - options: FindOptions<T> = {} as FindOptions<T>, + options: FindOptions<IRoom> = {}, ): Promise<T | null> { - return this.findOne<T>({ _id: roomId, t: type } as any, options); + return this.findOne({ _id: roomId, t: type }, options) as unknown as T; }If you need T for projections narrower than IRoom, prefer overloads to keep filter typing honest. Also, consider leaving
optionsas optional (no default) to avoid casting an empty object.packages/model-typings/src/models/IRoomsModel.ts (1)
208-208: Interface update looks good; consider tighter generic boundThe generic signature aligns with the implementation and preserves the default return type. To reduce accidental misuse, consider
T extends IRoom = IRoominstead ofDocument, or add documentation indicating T should represent a projection of IRoom.
📜 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(10 hunks)ee/packages/abac/src/index.ts(12 hunks)ee/packages/abac/src/user-auto-removal.spec.ts(1 hunks)packages/model-typings/src/models/IRoomsModel.ts(1 hunks)packages/models/src/models/Rooms.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
ee/packages/abac/src/index.ts (4)
packages/core-services/src/index.ts (2)
IAbacService(56-56)Room(171-171)packages/core-services/src/types/IAbacService.ts (1)
IAbacService(3-20)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
packages/model-typings/src/models/IRoomsModel.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
ee/packages/abac/src/user-auto-removal.spec.ts (5)
ee/packages/abac/src/index.ts (1)
AbacService(13-464)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)packages/models/src/index.ts (1)
registerServiceModels(231-265)
🪛 Biome (2.1.2)
ee/packages/abac/src/user-auto-removal.spec.ts
[error] 10-10: Expected an identifier, an array pattern, or an object pattern but instead found 'class'.
Expected an identifier, an array pattern, or an object pattern here.
(parse)
[error] 10-10: expected : but instead found {
Remove {
(parse)
[error] 13-13: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 13-13: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 14-14: await is only allowed within async functions and at the top levels of modules.
(parse)
[error] 15-15: await is only allowed within async functions and at the top levels of modules.
(parse)
[error] 17-17: Expected an expression but instead found '}'.
Expected an expression here.
(parse)
[error] 10-10: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ 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). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (16)
ee/packages/abac/src/index.spec.ts (6)
419-431: LGTM! Test correctly verifies hook invocation on new key addition.The test correctly expects
onRoomAttributesChangedto be called when a new key is added, including handling of duplicate values. The assertion that the hook receives the room object (not just the ID) aligns with the updated signature.
433-444: LGTM! Test correctly verifies no hook invocation on value removal.This test correctly implements the PR intent that removing values (making requirements less strict) does not trigger re-evaluation.
446-459: LGTM! Test correctly verifies hook invocation on value addition.This test correctly implements the PR intent that adding values (making requirements more strict) triggers re-evaluation.
536-551: LGTM! Tests correctly verify conditional hook invocation.Both tests correctly implement the PR intent:
- Line 536-543: Hook fires when values are added (more restrictive)
- Line 545-551: Hook does not fire when values are removed (less restrictive)
580-590: LGTM! Test correctly verifies no hook invocation on attribute removal.This test correctly implements the PR intent that removing an entire attribute does not trigger re-evaluation.
624-740: LGTM! Tests correctly verify hook invocations with updated attributes.All tests correctly verify that:
- The hook is called when attributes are added or replaced
- The hook receives the updated attributes from the DB (or constructed list as fallback)
- The tests cover both cases: DB returning updated doc and DB returning undefined
ee/packages/abac/src/index.ts (8)
1-11: LGTM! Concurrency control setup is appropriate.The use of
pLimit(20)to control concurrent user removals is a good safeguard against overloading the server. This limit balances throughput with resource consumption.
16-21: LGTM! Logger initialization is appropriate.The logger initialization in the constructor provides good observability for ABAC operations.
196-217: LGTM! Attribute change detection is correct and efficient.The
didAttributesChangehelper correctly detects when attributes become more restrictive by checking for:
- New keys (not in previous attributes)
- New values within existing keys (not in previous values set)
The use of Map and Set provides O(1) lookups, and short-circuiting on first addition keeps it efficient.
306-309: LGTM! Value addition detection is correct.The
wereAttributeValuesAddedhelper correctly detects when values are added to an attribute using Set-based lookup.
388-399: LGTM! Non-compliant user query logic is correct.The
buildNonCompliantConditionshelper correctly constructs MongoDB queries to find users who don't satisfy the room's ABAC requirements. The use of$notwith$elemMatchand$allcorrectly identifies users missing either:
- The attribute key entirely
- One or more required values for an existing key
MongoDB's handling of missing fields ensures users without
abacAttributesare also correctly identified as non-compliant.
401-463: LGTM! User re-evaluation and removal logic is well-implemented.The
onRoomAttributesChangedimplementation correctly:
- Handles the edge case of empty attributes (room becomes non-ABAC)
- Builds accurate non-compliant user queries using
__roomsfilter to target only current members- Controls concurrency with
pLimitto prevent server overload- Uses new
skipAppPreEventsandcustomSystemMessageoptions for appropriate removal notifications- Provides comprehensive logging for observability
- Handles errors gracefully without disrupting other operations
The logic correctly implements the PR objective of removing users who no longer satisfy stricter ABAC requirements.
177-179: LGTM! Room fetch projections are correctly expanded.All room fetch operations now correctly include
tandteamMainfields in the projection to satisfy the updatedonRoomAttributesChangedsignature.Also applies to: 268-270, 334-336, 359-361
117-120: LGTM! Logging additions improve observability.The debug logging for attribute updates, deletions, and room attribute removals provides good operational visibility into ABAC operations.
Also applies to: 141-144, 324-328
ee/packages/abac/src/user-auto-removal.spec.ts (2)
1-102: LGTM! Integration test infrastructure is well-designed.The test setup demonstrates several best practices:
- Uses
MongoMemoryServerfor isolated, reproducible integration tests- Mocks
Room.removeUserFromRoomappropriately to focus on subscription changes- Provides clear helper functions (
insertDefinitions,insertRoom,insertUsers) for readable test cases- Spies on logger to verify observability behavior
- Properly cleans up resources in
beforeEachandafterAll
104-410: LGTM! Comprehensive integration test coverage.The integration tests provide excellent coverage of the ABAC user removal functionality:
Scenarios covered:
- New key addition (104-167): Verifies compliant users stay, non-compliant users are removed
- Duplicate values (145-166): Ensures duplicate values are handled correctly
- Value addition/removal (169-219): Confirms re-evaluation only on additions
- Multi-attribute enforcement (221-288): Tests AND semantics across multiple attributes
- Idempotency (290-322): Confirms no duplicate removals on repeated operations
- Edge cases (324-374): Tests supersets, missing keys, and missing attributes
- Large populations (376-410): Lightweight performance sanity check with 300 users
Verification approach:
- Asserts both logging output and actual database state changes
- Verifies hook invocation with correct parameters
- Confirms membership updates for both retained and removed users
- Uses realistic ABAC attribute combinations
This comprehensive test suite gives high confidence in the correctness of the implementation.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/i18n/src/locales/en.i18n.json (1)
17-17: Restore the username placeholder for this system message.The value still renders only “was removed by ABAC,” so rooms never see which member was dropped. Please keep the
{{username}}placeholder (e.g.,@{{username}} was removed by ABAC) to match the key contract.- "abac_removed_user_from_the_room": "was removed by ABAC", + "abac_removed_user_from_the_room": "@{{username}} was removed by ABAC",apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (1)
24-35: Minor: catch type can be unknown (already raised before)You can change
catch (error: any)tocatch (error: unknown)and narrow, for stricter typing. This was mentioned in earlier reviews; consider it a nit.
🧹 Nitpick comments (3)
packages/models/src/models/Rooms.ts (1)
966-972: Consider the safety implications of theas anycast.The generic type parameter improves flexibility, but the
as anycast on line 971 bypasses type checking for the filter. While this is likely necessary for the generic implementation, it allows callers to specify a typeTthat may not accurately represent the actual document structure returned by the query.Consider whether the projection in
FindOptions<T>provides sufficient safety, or if additional runtime validation would be beneficial to ensure type consistency.ee/packages/abac/src/index.ts (1)
10-11: Consider making the concurrency limit configurable.The hardcoded limit of 20 may need adjustment based on server capacity and load patterns. Consider making this configurable through a setting or environment variable to allow tuning without code changes.
ee/packages/abac/src/index.spec.ts (1)
197-218: Prefer asserting the hook wasn’t called over log-string filteringUse the existing
(service as any).onRoomAttributesChangedspy to assert non-invocation for removal-only, instead of relying on a specific debug message string.Apply:
-const evaluationCalls = debugSpy.mock.calls - .map((c: any[]) => c[0]) - .filter((arg: any) => arg && arg.msg === 'Re-evaluating room subscriptions'); -expect(evaluationCalls.length).toBe(0); +expect((service as any).onRoomAttributesChanged).not.toHaveBeenCalled();
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts(3 hunks)apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts(1 hunks)apps/meteor/server/services/room/service.ts(2 hunks)ee/packages/abac/package.json(1 hunks)ee/packages/abac/src/index.spec.ts(10 hunks)ee/packages/abac/src/index.ts(11 hunks)ee/packages/abac/src/user-auto-removal.spec.ts(1 hunks)packages/apps-engine/src/definition/messages/MessageType.ts(1 hunks)packages/core-services/src/types/IRoomService.ts(2 hunks)packages/core-typings/src/IMessage/IMessage.ts(1 hunks)packages/http-router/src/Router.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)packages/message-types/src/registrations/common.ts(1 hunks)packages/model-typings/src/models/IRoomsModel.ts(1 hunks)packages/models/src/models/Rooms.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- ee/packages/abac/package.json
- packages/apps-engine/src/definition/messages/MessageType.ts
- packages/http-router/src/Router.ts
- packages/model-typings/src/models/IRoomsModel.ts
- packages/core-typings/src/IMessage/IMessage.ts
- apps/meteor/server/services/room/service.ts
- packages/message-types/src/registrations/common.ts
- apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts
🧰 Additional context used
🧬 Code graph analysis (5)
packages/core-services/src/types/IRoomService.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
MessageTypesValues(114-114)
packages/models/src/models/Rooms.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (2)
apps/meteor/server/services/room/service.ts (1)
removeUserFromRoom(87-93)packages/core-typings/src/IMessage/IMessage.ts (1)
MessageTypesValues(114-114)
ee/packages/abac/src/index.ts (4)
packages/core-services/src/index.ts (2)
IAbacService(56-56)Room(171-171)packages/core-services/src/types/IAbacService.ts (1)
IAbacService(3-20)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
ee/packages/abac/src/user-auto-removal.spec.ts (5)
ee/packages/abac/src/index.ts (1)
AbacService(13-459)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)packages/models/src/index.ts (1)
registerServiceModels(231-265)
🪛 Biome (2.1.2)
ee/packages/abac/src/user-auto-removal.spec.ts
[error] 10-10: Expected an identifier, an array pattern, or an object pattern but instead found 'class'.
Expected an identifier, an array pattern, or an object pattern here.
(parse)
[error] 10-10: expected : but instead found {
Remove {
(parse)
[error] 13-13: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 13-13: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 14-14: await is only allowed within async functions and at the top levels of modules.
(parse)
[error] 15-15: await is only allowed within async functions and at the top levels of modules.
(parse)
[error] 17-17: Expected an expression but instead found '}'.
Expected an expression here.
(parse)
[error] 10-10: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ 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 (11)
ee/packages/abac/src/index.ts (9)
18-21: LGTM!Logger initialization is clean and will provide valuable debugging and monitoring capabilities.
173-190: LGTM!The conditional check using
didAttributesChangecorrectly ensures thatonRoomAttributesChangedis only called when attributes become more restrictive, aligning with the PR's objectives.
192-213: LGTM!The
didAttributesChangehelper correctly identifies when new attribute values are added, triggering re-evaluation only when room requirements become more restrictive. The Set-based comparison is efficient.
262-284: LGTM!The method correctly triggers re-evaluation when a new attribute key is added to the room, passing the necessary room context to
onRoomAttributesChanged.
301-304: LGTM!The helper efficiently detects when new attribute values are added, ensuring re-evaluation occurs only when necessary.
351-381: LGTM!The method correctly handles both scenarios:
- When replacing an existing attribute, it only triggers re-evaluation if new values were added
- When adding a new attribute, it always triggers re-evaluation
This aligns with the PR's objective to re-evaluate only when requirements become stricter.
383-394: LGTM!The query construction correctly identifies users who don't satisfy all required attribute values using
$not,$elemMatch, and$alloperators.
396-458: Implementation successfully addresses PR objectives.The
onRoomAttributesChangedmethod effectively implements the core functionality:
- Identifies users who no longer meet room requirements
- Removes them with proper concurrency control
- Logs operations for debugging
- Handles edge cases (empty attributes, no users to remove)
This implementation aligns with the PR's goal to re-evaluate and remove users when room ABAC attributes become more restrictive.
451-457: Consider surfacing failures from onRoomAttributesChangedIn ee/packages/abac/src/index.ts (lines 451–457), the catch block logs errors but swallows them, so callers and monitoring won’t detect if re-evaluation or user removals fail. Evaluate whether to rethrow (after best-effort removals) or emit an event/metric for visibility.
packages/core-services/src/types/IRoomService.ts (1)
44-48: Public API looks good; ensure all implementers align with byUser Pick typeInterface is correct. Please align apps/meteor/server/services/room/service.ts and apps/meteor/app/lib/server/functions/removeUserFromRoom.ts to use byUser?: Pick<IUser, '_id' | 'username'> to avoid TS mismatches.
ee/packages/abac/src/index.spec.ts (1)
420-432: Tests correctly reflect “additions trigger, removals don’t” semanticsAssertions using post-operation attributes and guarding no-calls on removals look good and match the new contract.
Also applies to: 434-460, 537-544, 546-552, 581-591, 635-639, 651-655, 722-726, 737-741
KevLehman
force-pushed
the
feat/on-attribute-changed-hooks
branch
from
1d0e210 to
5c3f9b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (1)
13-17: Critical: byUser type mismatch with IRoomService interface.The implementation uses
byUser?: IUserbutIRoomServicedeclaresbyUser?: Pick<IUser, '_id' | 'username'>. This breaks type safety and can cause runtime errors when callers provide a minimal object.Apply this diff to align with the interface:
export const removeUserFromRoom = async function ( rid: string, user: IUser, - options?: { byUser?: IUser; skipAppPreEvents?: boolean; customSystemMessage?: MessageTypesValues }, + options?: { byUser?: Pick<IUser, '_id' | 'username'>; skipAppPreEvents?: boolean; customSystemMessage?: MessageTypesValues }, ): Promise<void> {Then cast
byUsertoIUserat the Apps event call sites (lines 27, 89):- await Apps.self?.triggerEvent(AppEvents.IPreRoomUserLeave, room, user, options?.byUser); + await Apps.self?.triggerEvent(AppEvents.IPreRoomUserLeave, room, user, options?.byUser as IUser | undefined);- await Apps.self?.triggerEvent(AppEvents.IPostRoomUserLeave, room, user, options?.byUser); + await Apps.self?.triggerEvent(AppEvents.IPostRoomUserLeave, room, user, options?.byUser as IUser | undefined);apps/meteor/server/services/room/service.ts (1)
87-92: Critical: byUser type mismatch with IRoomService interface.Same as the implementation file, this method signature uses
byUser?: IUserinstead ofbyUser?: Pick<IUser, '_id' | 'username'>as declared in the interface.Apply this diff:
async removeUserFromRoom( roomId: string, user: IUser, - options?: { byUser?: IUser; skipAppPreEvents?: boolean; customSystemMessage?: MessageTypesValues }, + options?: { byUser?: Pick<IUser, '_id' | 'username'>; skipAppPreEvents?: boolean; customSystemMessage?: MessageTypesValues }, ): Promise<void> { return removeUserFromRoom(roomId, user, options); }ee/packages/abac/src/user-auto-removal.spec.ts (1)
9-18: Fix jest.mock factory syntax to resolve parse errors.The inline
class {}syntax in the factory causes Biome parse errors. Additionally, theremoveUserFromRoommock should accept the optionaloptionsparameter to match the real signature.Apply this diff:
-jest.mock('@rocket.chat/core-services', () => ({ - ServiceClass: class {}, - Room: { - // Mimic the DB side-effects of removing a user from a room (no apps/system messages) - removeUserFromRoom: async (roomId: string, user: any) => { - const { Subscriptions } = await import('@rocket.chat/models'); - await Subscriptions.removeByRoomIdAndUserId(roomId, user._id); - }, - }, -})); +jest.mock('@rocket.chat/core-services', () => { + class MockServiceClass {} + const Room = { + // Mimic the DB side-effects of removing a user from a room (no apps/system messages) + removeUserFromRoom: async (roomId: string, user: any, _options?: any) => { + const { Subscriptions } = await import('@rocket.chat/models'); + await Subscriptions.removeByRoomIdAndUserId(roomId, user._id); + }, + }; + return { ServiceClass: MockServiceClass, Room }; +});
🧹 Nitpick comments (2)
ee/packages/abac/src/index.ts (2)
192-213: Consider extracting the inner loop for clarity.The logic is correct, but the nested breaks make the flow less obvious.
Apply this diff for a cleaner structure:
private didAttributesChange(current: IAbacAttributeDefinition[], next: IAbacAttributeDefinition[]) { - let added = false; const prevMap = new Map(current.map((a) => [a.key, new Set(a.values)])); for (const { key, values } of next) { const prevValues = prevMap.get(key); if (!prevValues) { - added = true; - break; + return true; } for (const v of values) { if (!prevValues.has(v)) { - added = true; - break; + return true; } } - if (added) { - break; - } } - - return added; + return false; }
354-381: LGTM! Handles both new and existing attribute replacements correctly.The method properly:
- Checks if values were added before re-evaluating existing keys (line 367)
- Always re-evaluates when inserting a new key (line 380)
Consider explicit handling if the DB update fails to return the updated document:
if (exists) { const updated = await Rooms.updateAbacAttributeValuesArrayFilteredById(rid, key, values); const prevValues = room.abacAttributes?.find((a) => a.key === key)?.values ?? []; if (this.wereAttributeValuesAdded(prevValues, values)) { - await this.onRoomAttributesChanged(room, updated?.abacAttributes || []); + if (!updated?.abacAttributes) { + this.logger.warn({ msg: 'Failed to retrieve updated attributes', rid, key }); + return; + } + await this.onRoomAttributesChanged(room, updated.abacAttributes); } return; }
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts(3 hunks)apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts(1 hunks)apps/meteor/server/services/room/service.ts(2 hunks)ee/packages/abac/package.json(1 hunks)ee/packages/abac/src/index.spec.ts(10 hunks)ee/packages/abac/src/index.ts(11 hunks)ee/packages/abac/src/user-auto-removal.spec.ts(1 hunks)packages/apps-engine/src/definition/messages/MessageType.ts(1 hunks)packages/core-services/src/types/IRoomService.ts(2 hunks)packages/core-typings/src/IMessage/IMessage.ts(1 hunks)packages/http-router/src/Router.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)packages/message-types/src/registrations/common.ts(1 hunks)packages/model-typings/src/models/IRoomsModel.ts(1 hunks)packages/models/src/models/Rooms.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/i18n/src/locales/en.i18n.json
- packages/message-types/src/registrations/common.ts
- packages/http-router/src/Router.ts
- packages/model-typings/src/models/IRoomsModel.ts
- apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts
🧰 Additional context used
🧬 Code graph analysis (6)
apps/meteor/server/services/room/service.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
MessageTypesValues(114-114)
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (3)
apps/meteor/server/services/room/service.ts (1)
removeUserFromRoom(87-93)packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IMessage/IMessage.ts (1)
MessageTypesValues(114-114)
packages/models/src/models/Rooms.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
packages/core-services/src/types/IRoomService.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IMessage/IMessage.ts (1)
MessageTypesValues(114-114)
ee/packages/abac/src/index.ts (4)
packages/core-services/src/index.ts (2)
IAbacService(56-56)Room(171-171)packages/core-services/src/types/IAbacService.ts (1)
IAbacService(3-20)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
ee/packages/abac/src/user-auto-removal.spec.ts (5)
ee/packages/abac/src/index.ts (1)
AbacService(13-459)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)packages/models/src/index.ts (1)
registerServiceModels(231-265)
🪛 Biome (2.1.2)
ee/packages/abac/src/user-auto-removal.spec.ts
[error] 10-10: Expected an identifier, an array pattern, or an object pattern but instead found 'class'.
Expected an identifier, an array pattern, or an object pattern here.
(parse)
[error] 10-10: expected : but instead found {
Remove {
(parse)
[error] 13-13: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 13-13: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 14-14: await is only allowed within async functions and at the top levels of modules.
(parse)
[error] 15-15: await is only allowed within async functions and at the top levels of modules.
(parse)
[error] 17-17: Expected an expression but instead found '}'.
Expected an expression here.
(parse)
[error] 10-10: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ 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 (23)
packages/core-typings/src/IMessage/IMessage.ts (1)
107-107: LGTM!The new message type 'abac-removed-user-from-room' is properly added to the MessageTypes array and will be included in the MessageTypesValues union type.
ee/packages/abac/package.json (1)
23-30: LGTM!The dependency updates align well with the ABAC feature requirements:
p-limit(3.1.0) for concurrency control during bulk user removals@rocket.chat/loggerfor enhanced loggingmongodb-memory-serverfor integration testingpackages/core-services/src/types/IRoomService.ts (1)
44-48: LGTM!The interface extension properly supports the new ABAC functionality with optional parameters that maintain backward compatibility.
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (2)
24-35: LGTM!The
skipAppPreEventsguard correctly prevents Apps from blocking ABAC-driven removals while preserving error handling for normal flows.
48-64: LGTM!The message handling logic properly prioritizes
customSystemMessageoverbyUser-based messages, maintaining backward compatibility for existing callers.ee/packages/abac/src/index.spec.ts (7)
421-433: LGTM!Test correctly verifies that adding a new attribute key triggers
onRoomAttributesChangedwith the updated attribute list, including duplicate values.
435-446: LGTM!Test correctly verifies that removing values (making requirements less strict) does not trigger re-evaluation.
448-461: LGTM!Test correctly verifies that adding values to an existing attribute triggers re-evaluation with the updated attribute list.
538-553: LGTM!Tests correctly verify that additions trigger the hook while removals do not, aligning with the PR's objective to re-evaluate only when requirements become more restrictive.
582-592: LGTM!Test correctly verifies that removing an entire attribute does not trigger re-evaluation (making requirements less strict).
626-656: LGTM!Tests correctly verify that both inserting new attributes and replacing existing ones trigger the hook with post-operation attributes.
713-742: LGTM!Tests correctly verify that adding new attributes triggers the hook with either DB-returned or constructed attribute lists.
ee/packages/abac/src/user-auto-removal.spec.ts (1)
104-411: Excellent integration test coverage!The test suite comprehensively validates the ABAC re-evaluation behavior across multiple scenarios:
- New attribute additions and removals
- Duplicate value handling
- Multi-attribute AND semantics
- Idempotency and no-op behavior
- Edge cases (supersets, missing attributes, missing users)
- Performance sanity check with 300 users
The tests properly verify both the debug logging output and actual database state changes.
ee/packages/abac/src/index.ts (10)
1-21: LGTM! Well-structured setup for concurrency and logging.The additions are appropriate:
- Concurrency limiter (20 operations) prevents server overload during bulk user removals
- Logger provides observability into ABAC operations
- Type imports (AtLeast, Users, Room) support the enhanced room lookups and removal operations
117-120: LGTM! Appropriate debug logging.
173-189: LGTM! Correctly gates re-evaluation on attribute additions.The flow properly:
- Captures previous attributes before update
- Checks if new attributes were added via
didAttributesChange- Triggers
onRoomAttributesChangedonly when constraints become stricterThe room projection now includes
tandteamMain, which are required for theremoveUserFromRoomcall downstream.
263-298: LGTM! Correctly handles both new and existing attribute updates.The method properly differentiates:
- New keys → always re-evaluate (line 283)
- Existing keys → re-evaluate only when values are added (line 295)
301-304: LGTM! Clean helper for detecting value additions.
307-323: LGTM! Correctly skips re-evaluation on attribute removal.When attributes are removed, the room becomes less restrictive, so no user removal is needed. The debug logging provides visibility.
329-348: LGTM! Correctly triggers re-evaluation when adding new attribute.Adding a new attribute key always increases restrictions, so re-evaluation is always required.
383-394: LGTM! Query correctly identifies non-compliant users.The
$not+$elemMatch+$allcombination ensures that users are flagged as non-compliant if they lack any of the required attribute values for a given key.Semantics: A user is compliant only if, for each room attribute key, they have all the required values (they may have additional values beyond the required ones).
461-461: LGTM! Enables default import syntax.
396-458: Confirm silent error handling in onRoomAttributesChanged. The method catches and logs all removal errors without propagation, whereas other callers let exceptions bubble. Verify that suppressing failures here is intentional.
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/ABAC-53
Steps to test or reproduce
Further comments
Why only when new attributes?
Suppose you have a room expecting [1,2,3] and a user with values [1,2,3,5]
If you then change the room expectations to be [1,2], the same user will still be able to access the room, cause the room attributes became "less strict".
However, if you instead change it to be [1,2,3,4], then we have to re-evaluate, as the room is now "more strict". This would give the result that the user cannot access the room, as attribute 4 is different.
Same happens if the attributes are changed to be [1,6]. The attribute list was reduced, but a new attribute was added, and thus we have to evaluate again.
(For users, not yet implemented, is the exact inverse of this logic)
Summary by CodeRabbit
New Features
Documentation