fix: move ABAC room membership cleanup to persistent queue#40189
Open
Kshitizjain11 wants to merge 2 commits intoRocketChat:developfrom
Open
fix: move ABAC room membership cleanup to persistent queue#40189Kshitizjain11 wants to merge 2 commits intoRocketChat:developfrom
Kshitizjain11 wants to merge 2 commits intoRocketChat:developfrom
Conversation
Contributor
|
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 |
|
|
|
Contributor
WalkthroughReplaces immediate ABAC-driven room removals with a persistent work queue: adds a job payload type and a public job processor that loads room/user, calls the removal helper, logs/audits, and signals retryable failures; evaluation now enqueues removal jobs per non-compliant user. Changes
Sequence DiagramsequenceDiagram
participant Policy as Policy Evaluator
participant Queue as QueueWorker
participant Handler as processRoomMembershipRemovalJob
participant RoomSvc as Room Service
participant UserSvc as User Service
participant Audit as Audit/Logging
Policy->>Queue: queueWork('abac.processRoomMembershipRemovalJob', {rid, uid, reason})
Queue->>Queue: Enqueue Job
Note over Queue: Job persisted to queue
Queue->>Handler: Deliver Job
par Load Concurrently
Handler->>RoomSvc: findOneById(rid)
Handler->>UserSvc: findOneById(uid)
RoomSvc-->>Handler: Room (or null)
UserSvc-->>Handler: User (or null)
end
alt Room or User Missing
Handler->>Audit: Log Warning (missing entity)
Handler-->>Queue: Complete (no action)
else Both Exist
Handler->>RoomSvc: removeUserFromRoom(rid, user, {skipAppPreEvents, customSystemMessage})
alt Removal Succeeds
RoomSvc->>Audit: Log removal + Audit.actionPerformed
Handler-->>Queue: Complete
else Removal Fails
Handler->>Audit: Log Error
Handler-->>Queue: Throw "retry:..." error
Queue->>Queue: Retry Job per backoff policy
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
coderabbitai
bot
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core-services/src/types/IAbacService.ts (1)
13-17: Duplicate, divergentAbacRoomMembershipRemovalJobdefinition — consolidate and export.The same type is declared again in
ee/packages/abac/src/index.ts(lines 48‑52) but withreason: AbacAuditReasoninstead of the literal union used here. SinceAbacAuditReasonis broader (e.g. includes'system'peree/packages/abac/src/audit.ts), the implementation’s parameter can accept values the interface forbids, and additions toAbacAuditReasonwon’t be reflected here. Additionally, this type is referenced by a public interface method but is notexported, so external implementors/consumers can’t name it.Recommend exporting a single source of truth (reusing
AbacAuditReason) and importing it in the implementation.♻️ Proposed change
import type { IAbacAttributeDefinition, IAbacAttribute, IRoom, IUser, AbacAccessOperation, AbacObjectType, ILDAPEntry, + AbacAuditReason, } from '@rocket.chat/core-typings'; export type AbacActor = Pick<IUser, '_id' | 'username' | 'name'>; -type AbacRoomMembershipRemovalJob = { +export type AbacRoomMembershipRemovalJob = { rid: string; uid: string; - reason: 'realtime-policy-eval' | 'room-attributes-change' | 'ldap-sync' | 'virtru-pdp-sync'; + reason: AbacAuditReason; };Then in
ee/packages/abac/src/index.ts, drop the local redeclaration and importAbacRoomMembershipRemovalJobfrom@rocket.chat/core-services.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-services/src/types/IAbacService.ts` around lines 13 - 17, The AbacRoomMembershipRemovalJob type is duplicated and divergent; update the canonical declaration by changing its reason property to use AbacAuditReason and export the type (export type AbacRoomMembershipRemovalJob = { rid: string; uid: string; reason: AbacAuditReason; }) so external implementors can reference it, then remove the local redeclaration in the EE implementation and import AbacRoomMembershipRemovalJob from the core package instead of redefining it. Ensure all usages (e.g., the public interface method that references AbacRoomMembershipRemovalJob) compile against the exported type.ee/packages/abac/src/index.ts (1)
731-743: Preserve original error info on retry throw.Re-throwing a brand-new
Error('retry: ...')discards the original cause (message, stack, and any structured error fields). SinceQueueWorker.isServiceRetryErroronly needs the message to contain"retry", you can preserve the root cause — this helps when debugging retry storms from worker logs where only the released reason is surfaced.♻️ Proposed change
try { await this.removeUserFromRoomOrThrow(room, user, reason); } catch (err) { logger.error({ msg: 'Failed to process queued ABAC room membership removal job', rid, uid, reason, err, }); - throw new Error(`retry: failed to remove user ${uid} from room ${rid}`); + const originalMessage = err instanceof Error ? err.message : String(err); + throw new Error(`retry: failed to remove user ${uid} from room ${rid}: ${originalMessage}`, { + cause: err, + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/abac/src/index.ts` around lines 731 - 743, The new Error thrown in the catch block discards the original error details; instead preserve the root cause when signaling a retry. In the catch after calling removeUserFromRoomOrThrow, either re-throw the original error (throw err) or wrap it using Error's cause (throw new Error(`retry: failed to remove user ${uid} from room ${rid}`, { cause: err })) so logger.error and QueueWorker.isServiceRetryError still see the "retry" marker while retaining the original message/stack/fields; update the throw in this catch to one of these approaches and keep the existing logger.error call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ee/packages/abac/src/index.ts`:
- Around line 731-743: The new Error thrown in the catch block discards the
original error details; instead preserve the root cause when signaling a retry.
In the catch after calling removeUserFromRoomOrThrow, either re-throw the
original error (throw err) or wrap it using Error's cause (throw new
Error(`retry: failed to remove user ${uid} from room ${rid}`, { cause: err }))
so logger.error and QueueWorker.isServiceRetryError still see the "retry" marker
while retaining the original message/stack/fields; update the throw in this
catch to one of these approaches and keep the existing logger.error call.
In `@packages/core-services/src/types/IAbacService.ts`:
- Around line 13-17: The AbacRoomMembershipRemovalJob type is duplicated and
divergent; update the canonical declaration by changing its reason property to
use AbacAuditReason and export the type (export type
AbacRoomMembershipRemovalJob = { rid: string; uid: string; reason:
AbacAuditReason; }) so external implementors can reference it, then remove the
local redeclaration in the EE implementation and import
AbacRoomMembershipRemovalJob from the core package instead of redefining it.
Ensure all usages (e.g., the public interface method that references
AbacRoomMembershipRemovalJob) compile against the exported type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6acd13b6-8dca-406a-8e1e-068960b177cd
📒 Files selected for processing (3)
ee/packages/abac/src/index.tsee/packages/abac/src/service.spec.tspackages/core-services/src/types/IAbacService.ts
📜 Review details
⏰ 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: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/core-services/src/types/IAbacService.tsee/packages/abac/src/index.tsee/packages/abac/src/service.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/abac/src/service.spec.ts
🧠 Learnings (18)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
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: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
packages/core-services/src/types/IAbacService.tsee/packages/abac/src/index.tsee/packages/abac/src/service.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:
packages/core-services/src/types/IAbacService.tsee/packages/abac/src/index.tsee/packages/abac/src/service.spec.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/core-services/src/types/IAbacService.tsee/packages/abac/src/index.tsee/packages/abac/src/service.spec.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/core-services/src/types/IAbacService.tsee/packages/abac/src/index.tsee/packages/abac/src/service.spec.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 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:
ee/packages/abac/src/index.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
ee/packages/abac/src/index.tsee/packages/abac/src/service.spec.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
ee/packages/abac/src/index.tsee/packages/abac/src/service.spec.ts
📚 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/index.tsee/packages/abac/src/service.spec.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.
Applied to files:
ee/packages/abac/src/service.spec.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/service.spec.ts
📚 Learning: 2026-03-06T18:09:17.867Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
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/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
ee/packages/abac/src/service.spec.ts
🔇 Additional comments (3)
ee/packages/abac/src/index.ts (2)
48-52: Duplicate type — see consolidated fix at the root cause.Already flagged in
packages/core-services/src/types/IAbacService.tslines 13‑17. Prefer importing the shared type from@rocket.chat/core-servicesand removing this local redeclaration to avoid drift betweenreason: AbacAuditReason(here) and the literal union in the interface file.
851-866: LGTM — enqueue per non-compliant pair with per-item error isolation.Sequential
awaitplus per-iteration try/catch ensures a single enqueue failure doesn’t abort the rest. Routing throughQueueWorker.queueWork('work', 'abac.processRoomMembershipRemovalJob', …)correctly maps to the auto-exposedAbacService.processRoomMembershipRemovalJobaction viaapi.call.ee/packages/abac/src/service.spec.ts (1)
1213-1287: LGTM — new suites cover enqueue and retry paths.
evaluateRoomMembership with persistent queueasserts the exactQueueWorker.queueWorkcontract (queue, action key, payload shape), andprocessRoomMembershipRemovalJobexercises both the happy path (Room.removeUserFromRoominvocation with expected options) and the retry surface (retry:error thrown on removal failure). Good coverage for the new behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ee/packages/abac/src/index.ts">
<violation number="1" location="ee/packages/abac/src/index.ts:853">
P2: Enqueue failures are swallowed, causing known non-compliant room memberships to be skipped with no fallback removal in this run.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Author
|
Hi maintainers, this PR is ready for review and passes local validation for the ABAC queue refactor in issue #40180. Could someone please: Run/confirm QA for this change. Validation completed on my side:
|
coderabbitai
bot
removed
the
type: feature
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ee/packages/abac/src/service.spec.ts (2)
1213-1288: Consider an additional assertion that enqueue failure for one user doesn't block the others.Both tests use a single non-compliant user, so the fallback path is only exercised with one entry. Given the PR commit message explicitly introduces "fallback to immediate removal when queue enqueue fails", a test with multiple non-compliant pairs where
mockQueueWorkrejects for the first call and resolves for the second (or vice versa) would guard against a regression where a single enqueue rejection short-circuits the loop (e.g., unhandledawaitin aforEach/missingPromise.allSettled). Minor, but cheap insurance for the core behavior this PR is adding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/abac/src/service.spec.ts` around lines 1213 - 1288, The test suite lacks coverage ensuring one enqueue failure doesn't stop processing other non-compliant user-room pairs; add a test for evaluateRoomMembership that supplies multiple non-compliant pairs (via (service as any).pdp.evaluateUserRooms and the mocked room/user iterators), stub mockQueueWork to reject for the first call and resolve for the second, and then assert that for the rejected call mockRoomRemoveUserFromRoom was invoked for that pair while mockQueueWork was still called/resolved for the other pair (use service.evaluateRoomMembership to drive the behavior); this verifies per-item enqueue failures fall back to immediate removal without short-circuiting the loop.
1290-1325: Consider adding coverage for missing room/user and forreasonpropagation.The new
processRoomMembershipRemovalJobsuite covers the happy path and a generic removal failure, but a couple of behaviors worth pinning down:
- There's no test for the case where
Rooms.findOneById/Users.findOneByIdreturnsnull(stale job after room/user deletion). Whichever behavior the implementation chose — silently skip vs. throwretry:— it's worth locking in so a future refactor doesn't flip it and either cause infinite retries or mask real bugs.- The "retry" test only asserts the
retry:prefix. Consider also asserting that the original error message/cause is preserved in the thrown error, so operators debugging the queue get useful context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/abac/src/service.spec.ts` around lines 1290 - 1325, Add tests to pin down stale-job and error-propagation behavior for processRoomMembershipRemovalJob: add one test where mockFindOneByIdAndType resolves to null (room missing) and one where mockUsersFindOneById resolves to null (user missing) and assert the chosen behavior (either resolves silently or rejects with a 'retry:' error) so it can't change silently later; update the failing-removal test that uses mockRoomRemoveUserFromRoom.mockRejectedValue to also assert the thrown error includes the original message (e.g., contains 'boom') and add a small test that verifies the reason argument is passed through to mockRoomRemoveUserFromRoom (check options include customSystemMessage or reason propagation) to lock in expected semantics for processRoomMembershipRemovalJob.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ee/packages/abac/src/service.spec.ts`:
- Around line 1213-1288: The test suite lacks coverage ensuring one enqueue
failure doesn't stop processing other non-compliant user-room pairs; add a test
for evaluateRoomMembership that supplies multiple non-compliant pairs (via
(service as any).pdp.evaluateUserRooms and the mocked room/user iterators), stub
mockQueueWork to reject for the first call and resolve for the second, and then
assert that for the rejected call mockRoomRemoveUserFromRoom was invoked for
that pair while mockQueueWork was still called/resolved for the other pair (use
service.evaluateRoomMembership to drive the behavior); this verifies per-item
enqueue failures fall back to immediate removal without short-circuiting the
loop.
- Around line 1290-1325: Add tests to pin down stale-job and error-propagation
behavior for processRoomMembershipRemovalJob: add one test where
mockFindOneByIdAndType resolves to null (room missing) and one where
mockUsersFindOneById resolves to null (user missing) and assert the chosen
behavior (either resolves silently or rejects with a 'retry:' error) so it can't
change silently later; update the failing-removal test that uses
mockRoomRemoveUserFromRoom.mockRejectedValue to also assert the thrown error
includes the original message (e.g., contains 'boom') and add a small test that
verifies the reason argument is passed through to mockRoomRemoveUserFromRoom
(check options include customSystemMessage or reason propagation) to lock in
expected semantics for processRoomMembershipRemovalJob.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 153f7a79-529a-4e23-a00d-4e5bdc96681b
📒 Files selected for processing (2)
ee/packages/abac/src/index.tsee/packages/abac/src/service.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/packages/abac/src/index.ts
📜 Review details
⏰ 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: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/abac/src/service.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/abac/src/service.spec.ts
🧠 Learnings (18)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
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: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
ee/packages/abac/src/service.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/service.spec.ts
📚 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/service.spec.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2026-03-06T18:09:17.867Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
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/service.spec.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/service.spec.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
ee/packages/abac/src/service.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[ABAC] Move room membership removals to persistent queue jobs
Proposed changes
This PR refactors the room membership evaluation flow in ABAC so that user removals are no longer executed inline with
Promise.all. Instead, each removal is enqueued as a persistent background job and processed by Rocket.Chat’s existing queue infrastructure.The main changes are:
evaluateRoomMembership()now creates queue jobs for each user-room membership removal instead of removing users immediately.This keeps the enforcement path responsive while moving the expensive and failure-prone work into the background.
Issue(s)
Steps to test or reproduce
evaluateRoomMembership().yarn workspace @rocket.chat/abac test --runInBand src/service.spec.tsFurther comments
This change intentionally reuses Rocket.Chat’s existing persistent job queue instead of introducing a new queueing mechanism. The goal is to make ABAC membership cleanup more reliable under load and more resilient to temporary failures without changing the policy semantics.
Validation performed during implementation:
Summary by CodeRabbit