feat: Audit ABAC actions #37565
feat: Audit ABAC actions #37565
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 |
|
WalkthroughThreads an optional minimal actor through ABAC APIs and service methods, adds a centralized Audit module emitting typed ServerEvents, introduces audit schemas and a GET /abac/audit endpoint, extends core typings/exports and models registration, and updates tests to be actor-aware and assert audit emissions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as ABAC Endpoint
participant Service as AbacService
participant Audit as Audit Module
participant SE as ServerEvents Model
Client->>API: HTTP request (session user)
API->>API: getActorFromUser(this.user) → actor
API->>Service: Abac.method(..., actor)
Service->>Service: perform ABAC operation
Service->>Audit: Audit.<event>(payload, actor)
Audit->>SE: ServerEvents.createAuditServerEvent(event, payload, actor)
SE-->>Audit: persisted (async)
Audit-->>Service: resolved
Service-->>API: operation result
API-->>Client: HTTP response
alt Query audit events
Client->>API: GET /abac/audit?start=&end=&offset=&count=&actor=...
API->>SE: ServerEvents.findPaginated(filter by time/type/actor, pagination, sort)
SE-->>API: paginated events
API-->>Client: events + pagination
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 14:39 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 8 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Fri, 28 Nov 2025 14:39:35 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37565 +/- ##
=============================================
- Coverage 54.07% 54.03% -0.04%
=============================================
Files 2661 2661
Lines 50149 50149
Branches 11213 11213
=============================================
- Hits 27116 27098 -18
- Misses 20903 20918 +15
- Partials 2130 2133 +3
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
chore/audit-events
branch
from
adff5ae to
a8aabd7
Compare
KevLehman
force-pushed
the
chore/audit-events
branch
2 times, most recently
from
d416b1c to
7ecc215
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/tests/end-to-end/api/abac.ts (1)
260-310: Remove duplicate test.Lines 260-277 and 279-310 both test updating only the key of an attribute, with nearly identical test names ("PUT should update key only" vs "PUT should update key only (key-updated)"). The second test (279-310) creates a new attribute and performs the same verification as the first. This duplication reduces test suite maintainability.
Apply this diff to remove the duplicate:
- - it('PUT should update key only (key-updated)', async () => { - const testKey = `${initialKey}_update_test`; - await request - .post(`${v1}/abac/attributes`) - .set(credentials) - .send({ key: testKey, values: ['val1', 'val2'] }) - .expect(200); - - const listRes = await request.get(`${v1}/abac/attributes`).query({ key: testKey }).set(credentials).expect(200); - - const attr = listRes.body.attributes.find((a: any) => a.key === testKey); - expect(attr).to.exist; - const attrId = attr._id; - - const newKey = `${initialKey}_update_test_renamed`; - await request - .put(`${v1}/abac/attributes/${attrId}`) - .set(credentials) - .send({ key: newKey }) - .expect(200) - .expect((res) => { - expect(res.body).to.have.property('success', true); - }); - - await request - .get(`${v1}/abac/attributes/${attrId}`) - .set(credentials) - .expect(200) - .expect((res) => { - expect(res.body).to.have.property('key', newKey); - }); - });ee/packages/abac/src/index.ts (1)
310-316: Audit emits before confirming attribute change occurred.
Audit.objectAttributeChangedis called at Line 311 unconditionally, but the actual change detection happens at Lines 314-316. IfdidAttributesChangereturnsfalse, an audit entry is still emitted even though no effective change occurred. Consider moving the audit inside the conditional or adjusting the change type.const updated = await Rooms.setAbacAttributesById(rid, normalized); - void Audit.objectAttributeChanged({ _id: room._id }, room.abacAttributes || [], normalized, 'updated', actor); const previous: IAbacAttributeDefinition[] = room.abacAttributes || []; if (this.didAttributesChange(previous, normalized)) { + void Audit.objectAttributeChanged({ _id: room._id }, previous, normalized, 'updated', actor); await this.onRoomAttributesChanged(room, (updated?.abacAttributes as IAbacAttributeDefinition[] | undefined) ?? normalized); }
🧹 Nitpick comments (5)
apps/meteor/tests/end-to-end/api/abac.ts (1)
144-144: Consider removing the unused request call.This line makes a request but discards the response. If the intention is to verify the endpoint doesn't crash, consider adding an expectation or removing the call entirely.
apps/meteor/ee/server/api/abac/schemas.ts (1)
201-233: Consider addingsuccessto the required fields array.The response schema includes
successin properties but doesn't list it inrequired. While the API wrapper may handle this, aligning the schema with the actual response contract would be more precise.- required: ['events', 'count', 'offset', 'total'], + required: ['success', 'events', 'count', 'offset', 'total'],ee/packages/abac/src/audit.ts (1)
39-39: Hardcoded IP and empty useragent reduce audit trail value.The actor's IP is hardcoded as
'0.0.0.0'and useragent as''. If this information is available from the request context, threading it through would improve forensic value of audit logs.Consider whether the
AbacActortype could be extended to includeipanduseragentfields populated from the API request context ingetActorFromUser().packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts (2)
25-53: Consider addingprevioustoIServerEventAbacAttributeChangedfor consistency.
IServerEventAbacObjectAttributeChangedincludes bothpreviousandcurrentattribute definitions, butIServerEventAbacAttributeChanged(Lines 44-53) only hascurrentanddiff. For attribute CRUD operations (especially deletions and updates), havingpreviouswould align with the linked issue's requirement for "before/after diffs for value changes."interface IServerEventAbacAttributeChanged extends IAuditServerEventType< | { key: 'attributeKey'; value: string } | { key: 'reason'; value: AbacAuditReason } | { key: 'change'; value: AbacAttributeDefinitionChangeType } + | { key: 'previous'; value: IAbacAttributeDefinition | null | undefined } | { key: 'current'; value: IAbacAttributeDefinition | null | undefined } | { key: 'diff'; value: IAbacAttributeDefinition | undefined } > { t: 'abac.attribute.changed'; }
55-70:ValidAbacEventsincludes'abac.object.attributes.removed'but lacks a dedicated interface.The union type at Line 70 lists
'abac.object.attributes.removed', and Line 78 maps it toIServerEventAbacObjectAttributeChanged. This reuse is intentional (per AI summary), but the discrepancy could cause confusion. Consider adding a brief comment or type alias clarifying thatabac.object.attributes.removedreuses the same payload structure.
📜 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 (15)
apps/meteor/ee/server/api/abac/index.ts(13 hunks)apps/meteor/ee/server/api/abac/schemas.ts(2 hunks)apps/meteor/tests/end-to-end/api/abac.ts(4 hunks)ee/packages/abac/src/audit.ts(1 hunks)ee/packages/abac/src/can-access-object.spec.ts(1 hunks)ee/packages/abac/src/helper.ts(1 hunks)ee/packages/abac/src/index.spec.ts(34 hunks)ee/packages/abac/src/index.ts(22 hunks)ee/packages/abac/src/subject-attributes-validations.spec.ts(1 hunks)ee/packages/abac/src/user-auto-removal.spec.ts(14 hunks)packages/core-services/src/index.ts(1 hunks)packages/core-services/src/types/IAbacService.ts(1 hunks)packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts(1 hunks)packages/core-typings/src/index.ts(2 hunks)packages/models/src/index.ts(2 hunks)
🧰 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/subject-attributes-validations.spec.tsee/packages/abac/src/can-access-object.spec.tspackages/core-services/src/index.tspackages/core-typings/src/index.tsee/packages/abac/src/helper.tspackages/models/src/index.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/schemas.tsee/packages/abac/src/user-auto-removal.spec.tspackages/core-services/src/types/IAbacService.tsapps/meteor/ee/server/api/abac/index.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tsee/packages/abac/src/index.spec.tsee/packages/abac/src/index.tsee/packages/abac/src/audit.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/subject-attributes-validations.spec.tsee/packages/abac/src/can-access-object.spec.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/index.spec.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
📚 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 : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
ee/packages/abac/src/subject-attributes-validations.spec.tsee/packages/abac/src/user-auto-removal.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/subject-attributes-validations.spec.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/index.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/subject-attributes-validations.spec.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/index.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/subject-attributes-validations.spec.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/index.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 : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
ee/packages/abac/src/subject-attributes-validations.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/subject-attributes-validations.spec.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/index.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 : Group related tests in the same file
Applied to files:
ee/packages/abac/src/subject-attributes-validations.spec.ts
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/schemas.tsee/packages/abac/src/user-auto-removal.spec.tspackages/core-services/src/types/IAbacService.tsapps/meteor/ee/server/api/abac/index.tsee/packages/abac/src/index.tsee/packages/abac/src/audit.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:
apps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/schemas.tsee/packages/abac/src/user-auto-removal.spec.tspackages/core-services/src/types/IAbacService.tsapps/meteor/ee/server/api/abac/index.tsee/packages/abac/src/index.spec.tsee/packages/abac/src/index.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
ee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/index.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/index.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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
ee/packages/abac/src/index.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
ee/packages/abac/src/index.spec.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/abac/src/index.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
🧬 Code graph analysis (5)
ee/packages/abac/src/helper.ts (1)
packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
ee/packages/abac/src/user-auto-removal.spec.ts (2)
ee/packages/abac/src/index.ts (1)
AbacService(17-846)ee/packages/abac/src/audit.ts (1)
Audit(29-146)
packages/core-services/src/types/IAbacService.ts (4)
packages/core-services/src/index.ts (2)
AbacActor(56-56)IAbacService(56-56)packages/core-typings/src/IUser.ts (1)
IUser(187-258)packages/core-typings/src/IAbacAttribute.ts (2)
IAbacAttributeDefinition(3-14)IAbacAttribute(16-16)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts (3)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
ee/packages/abac/src/index.ts (3)
ee/packages/abac/src/helper.ts (1)
diffAttributes(36-78)ee/packages/abac/src/audit.ts (1)
Audit(29-146)packages/core-services/src/types/IAbacService.ts (1)
AbacActor(11-11)
🪛 GitHub Check: 🔎 Code Check / Code Lint
ee/packages/abac/src/audit.ts
[warning] 116-116:
Forbidden non-null assertion
[warning] 103-103:
Forbidden non-null assertion
[warning] 84-84:
Forbidden non-null assertion
[warning] 65-65:
Forbidden non-null assertion
[warning] 52-52:
Forbidden non-null assertion
[warning] 39-39:
Forbidden non-null assertion
🔇 Additional comments (39)
packages/core-services/src/types/IAbacService.ts (2)
11-11: LGTM! Clean actor type definition.The
AbacActortype appropriately captures the minimal user context needed for auditing ABAC operations.
14-49: LGTM! Consistent actor parameter addition.The optional
actor?: AbacActorparameter has been consistently added to all ABAC mutation methods, enabling actor-aware auditing across the service surface. Note thatcanAccessObjectcorrectly omits the actor parameter as it's a query operation rather than an audited mutation.ee/packages/abac/src/can-access-object.spec.ts (1)
24-26: LGTM! Necessary mock for audit functionality.The
ServerEvents.createAuditServerEventmock is correctly added to support the audit event creation introduced by this PR.packages/core-typings/src/index.ts (1)
3-3: LGTM! Correct public API exposure.The import and export of
IAuditServerAbacActioncorrectly exposes ABAC audit event types to the public API, following the established pattern for other audit event types in this file.Also applies to: 154-154
ee/packages/abac/src/subject-attributes-validations.spec.ts (1)
17-19: LGTM! Test collection initialization.The dummy collection initialization ensures the database has at least one collection before registering service models, which is a reasonable test setup pattern. The
@ts-expect-errordirective is appropriate here since this is test-only scaffolding outside the production schema.packages/models/src/index.ts (1)
123-123: LGTM! ServerEvents model registration.The import and registration of
ServerEventsRawcorrectly wires the ServerEvents model into the public model surface, following the established pattern for other models in this file.Also applies to: 268-268
packages/core-services/src/index.ts (1)
56-56: LGTM! AbacActor public export.Adding
AbacActorto the public exports enables consumers to properly type actor parameters when calling ABAC service methods, completing the actor-aware ABAC integration.apps/meteor/ee/server/api/abac/schemas.ts (2)
1-1: LGTM!Import addition for
IAuditServerActoris appropriate for the new audit event query schema.
152-199: LGTM!The query schema correctly validates audit event filters with date-time formatted strings, pagination fields, and actor filtering. The
additionalProperties: falseensures strict validation.ee/packages/abac/src/user-auto-removal.spec.ts (5)
7-7: LGTM!Import of
Auditmodule enables proper mocking and verification of audit events in tests.
32-32: LGTM!The
fakeActorconstant provides a consistent actor context for all test cases, properly typed with_id,username, andtypefields matching theAbacActorinterface.
81-96: LGTM!Test setup correctly initializes the audit spy with
mockResolvedValue()for the asyncAudit.actionPerformedmethod, and properly clears it inbeforeEach.
136-142: LGTM!Audit assertions correctly verify that
Audit.actionPerformedis called with expected parameters: audited users, room ID, and the'room-attributes-change'reason. The extraction of user IDs and use of Set for room/action verification is clean.
274-289: LGTM!Multi-attribute test properly verifies AND semantics enforcement with audit logging for all non-compliant users (u2, u3, u5).
ee/packages/abac/src/audit.ts (2)
119-134: LGTM!The
actionPerformedmethod correctly uses{ type: 'system' }as the actor for automated membership revocation actions, which is appropriate since these are triggered by system-level ABAC enforcement rather than direct user actions.
135-145: LGTM!The
subjectAttributeChangedmethod properly logs LDAP-sync-triggered attribute changes with a system actor context.ee/packages/abac/src/index.spec.ts (4)
5-6: LGTM!The
fakeActorconstant provides consistent actor context for unit tests, matching theAbacActorinterface structure.
25-25: LGTM!New mock for
findOneAndUpdatesupports the updated ABAC attribute update flow.
57-59: LGTM!The
ServerEvents.createAuditServerEventmock prevents audit calls from failing tests while allowing the service logic to be tested in isolation.
386-416: LGTM!The
addAbacAttributetests properly includefakeActorin all service calls while maintaining the existing behavior verification for valid insertions, error handling, and duplicate detection.apps/meteor/ee/server/api/abac/index.ts (5)
32-39: LGTM!The
getActorFromUserhelper cleanly extracts actor context with proper handling of undefined/null users. The optional chaining and ternary provide safe fallback.
66-66: LGTM!Actor context is now properly threaded to
setRoomAbacAttributesfor audit logging.
360-409: Audit endpoint implementation looks solid with a few observations.The endpoint correctly:
- Requires authentication and
abac-managementpermission- Requires both
abacandauditinglicenses- Filters by event types, time range, and optional actor
- Uses
allowDiskUse: truefor large result sets- Defaults sort to descending timestamp
Minor consideration: The event type strings are hardcoded. If these are defined as constants elsewhere (e.g., in core-typings), referencing them would reduce duplication.
384-387: Verify the date range filter behavior whenstartorendis undefined.When
startis undefined, it defaults tonew Date(0)(epoch). Whenendis undefined, it defaults tonew Date()(now). This seems intentional but verify this matches expected behavior - querying all events from epoch to now when no filters are provided could return a very large dataset.Consider whether this default behavior aligns with UX expectations, or if a reasonable default window (e.g., last 30 days) would be more appropriate for the common case.
375-377: Validate thatthis.queryParamstype casting is necessary.The cast
this.queryParams as Record<string, string | number | null | undefined>differs from similar casts elsewhere in the file that includestring[]. Verify this is intentional and won't cause type-related issues with array query parameters.packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts (2)
1-4: LGTM!The imports and minimal type definitions are appropriate for audit event payloads—picking only essential fields (
_id,usernamefor users;_idfor rooms) minimizes log size while retaining identifiability.
6-23: LGTM!The type definitions cover the expected audit reasons and change types comprehensively. The diff structure with optional
added,removed, andrenamedFromfields supports flexible change tracking.ee/packages/abac/src/index.ts (12)
11-12: LGTM!The new imports for
AuditanddiffAttributesare correctly added to support the audit functionality.
68-72: LGTM!Proper diff-based audit for subject attribute changes. The conditional check ensures audits are only emitted when there are meaningful diffs, avoiding noise.
74-88: LGTM!Actor threading and audit call for attribute creation are correctly implemented. Using
voidfor fire-and-forget async audit is appropriate per the requirements (non-blocking).
241-254: LGTM!Actor threading and audit call for attribute deletion are correctly implemented.
300-304: LGTM!Proper early-return audit when clearing all room ABAC attributes.
389-437: LGTM!Actor threading and differentiated audit calls for
key-addedvskey-updatedare correctly implemented. The early return at Lines 426-428 properly skips audit when values are unchanged.
463-477: LGTM!Correct differentiation between removing the last attribute (
objectAttributesRemoved) and removing one of many (objectAttributeRemoved).
536-544: LGTM!Audit correctly placed after the DB update, using the updated room attributes from the DB result.
646-654: LGTM!Audit for action performed correctly records each user removal with room context and reason.
729-731: LGTM!Audit for realtime policy evaluation correctly records access revocation when user is non-compliant.
776-786: LGTM!Audit calls for LDAP-sync triggered removals are correctly placed with appropriate reason. Both the no-attributes and non-compliant attribute paths are covered.
Also applies to: 800-810
507-512: Audit log may record incomplete state when concurrent ABAC attribute additions occur.The concern is partially valid. While a check at line 503-504 prevents the exact duplicate-key race condition, a different-key race condition remains:
- Thread A and B both call
addRoomAbacAttributeByKeywith different keys- Both read
room.abacAttributesintopreviousat roughly the same time- Thread A's
insertAbacAttributeIfNotExistsByIdsucceeds and persists key1- Thread B's
insertAbacAttributeIfNotExistsByIdsucceeds and persists key2- If Thread B's read happened before Thread A's write was visible,
previouswon't include key1- The fallback
[...previous, { key: key2, values }]used for the Audit log won't reflect the actual DB state (which includes both key1 and key2)The Audit record will show an incomplete state. Use
updated?.abacAttributesunconditionally or re-fetch the room state from the database to ensure log accuracy:const updated = await Rooms.insertAbacAttributeIfNotExistsById(rid, key, values); const next = updated?.abacAttributes ?? (await Rooms.findOneById(rid, { projection: { abacAttributes: 1 } }))?.abacAttributes ?? [...previous, { key, values }];
KevLehman
force-pushed
the
chore/audit-events
branch
from
fafa44d to
045f829
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/packages/abac/src/index.ts (1)
27-72: Subject-attribute audit misses full-removal cases
addSubjectAttributesonly callsAudit.subjectAttributeChangedwhenfinalAttributes.length > 0, anddiffAttributes(user?.abacAttributes, finalAttributes)currently returns[]whenfinalAttributesis empty. As a result, a user losing all ABAC attributes after LDAP sync is never logged viaabac.subject.attribute.changed, even though you log the resulting room removals.If the intent of
subjectAttributeChangedis to track LDAP diffs, consider:
- Emitting an event also when
finalAttributesis empty (e.g., passing the previous attributes as the diff for “all removed”), and/or- Adjusting
diffAttributesso(a ≠ [], b = [])yields a non-empty diff representing the removed keys.This keeps subject-level audit trails consistent with room/definition diffs.
♻️ Duplicate comments (4)
apps/meteor/tests/end-to-end/api/abac.ts (1)
644-673: Misplaced and potentially duplicate “DELETE attribute definition” testThis
it('DELETE attribute definition should remove the key (key-removed)'...)lives under “Default and Team Default Room Restrictions”, but it validates plain attribute‑definition deletion semantics (create attribute → delete by id → GET fails). That behavior is unrelated to default/team restriction logic and is already covered in the more focused “Usage & Deletion” section.Consider moving this test next to the other attribute CRUD/deletion tests (or removing it if it’s redundant with an existing “key-removed” test) to keep suites organized and avoid duplicated coverage.
ee/packages/abac/src/index.ts (1)
74-88: Attribute CRUD audit uses pre-update “current” and a partial modifier as diffIn
updateAbacAttributeByIdyou call:await AbacAttributes.updateOne({ _id }, { $set: modifier }); void Audit.attributeUpdated(existing, modifier as IAbacAttributeDefinition, actor);Issues:
existingis the pre-update document, butAudit.attributeUpdated’scurrentparameter is meant to represent the post-update state.modifiermay contain onlykeyor onlyvalues, so casting it toIAbacAttributeDefinitioncan produce incomplete or inconsistent shapes for thedifffield.This makes it hard for consumers of
abac.attribute.changedevents to reconstruct accurate before/after state or field-level diffs.A more precise pattern would be:
- Use
findOneAndUpdatewith{ returnDocument: 'after' }(or a follow-upfindOneById) to obtain the updated document.- Pass
currentas the updated attribute anddiffas a properly-computed diff (e.g., via a helper).Illustrative change:
- await AbacAttributes.updateOne({ _id }, { $set: modifier }); - void Audit.attributeUpdated(existing, modifier as IAbacAttributeDefinition, actor); + const updated = await AbacAttributes.findOneAndUpdate( + { _id }, + { $set: modifier }, + { returnDocument: 'after', projection: { key: 1, values: 1 } }, + ); + if (updated) { + void Audit.attributeUpdated(updated as IAbacAttributeDefinition, { + key: updated.key, + values: updated.values, + }, actor); + }Also applies to: 189-239, 241-254
ee/packages/abac/src/helper.ts (1)
36-77: diffAttributes drops “all-removed” cases and still returns an ambiguous flat diff shape
- When
ais non‑empty andbis empty, this returns[], soaddSubjectAttributeswill not emitAudit.subjectAttributeChangedwhen a user loses all ABAC attributes. That seems at odds with having a dedicated subject diff event and with ABAC‑24’s goal of auditing attribute changes, especially LDAP‑driven removals.- The returned
IAbacAttributeDefinition[]still conflates:
- keys only in
a(removals, usinga’s values),- keys in both with differing sets (changes, using
b’s values),- keys only in
b(additions, usingb’s values),
with no way for consumers to distinguish which is which.Consider:
- Treating
bempty as “all keys removed” and returningain that case, so subject‑attribute removal is auditable; and- Either:
- introducing a structured diff type (e.g.
{ added: IAbacAttributeDefinition[]; removed: ...; changed: ... }), or- clearly documenting the current conventions at the type/JSdoc level so audit consumers can interpret entries correctly.
Example change for the all‑removed case:
export function diffAttributes(a: IAbacAttributeDefinition[] = [], b: IAbacAttributeDefinition[] = []): IAbacAttributeDefinition[] { - if (!a?.length && b?.length) { - return b; - } - - if (!b?.length) { - return []; - } + if (!a?.length && b?.length) { + return b; + } + + if (!b?.length) { + // everything from `a` was removed + return a; + }ee/packages/abac/src/audit.ts (1)
30-41: Avoidactor.username!and factor a shared audit-actor helperAll user-actor payloads here use a non-null assertion on
actor.usernameand duplicate the same literal object:{ type: 'user', _id: actor._id, username: actor.username!, ip: '0.0.0.0', useragent: '' }This both violates the lint rule already reported on this file and is brittle if
AbacActor['username']is ever absent.Consider centralizing the mapping and handling the optional username once:
const toAuditUserActor = (actor: AbacActor): IAuditServerActor => ({ type: 'user', _id: actor._id, username: actor.username ?? 'unknown', // or make username required on AbacActor ip: '0.0.0.0', useragent: '', });Then each method can just call
toAuditUserActor(actor)instead of repeating the object and using!.Also applies to: 42-54, 55-67, 68-86, 87-105, 106-118
🧹 Nitpick comments (4)
apps/meteor/tests/end-to-end/api/abac.ts (1)
143-153: Avoid redundant key‑update coverage and unused pre‑flight GET
- The GET at Line 144 doesn’t feed assertions; it can be dropped unless you’re intentionally sanity‑checking the listing path here.
- You now have two very similar tests:
it('PUT should update key only', ...)usingattributeId, andit('PUT should update key only (key-updated)', ...)creating a fresh attribute.
Both assert that a single PUT renames the key and that a subsequent GET by id returns the new key. Unless you need two distinct flows for some reason (e.g., one for initial attribute and one for later‑created definitions), consider merging them to avoid extra test time and maintenance surface.Also applies to: 279-310
ee/packages/abac/src/index.ts (1)
613-669: Fire-and-forget Audit calls may surface as unhandled rejectionsAcross
onRoomAttributesChanged,canAccessObject, andonSubjectAttributesChangedyou fire audit calls asvoid Audit.actionPerformed(...)without awaiting or catching:
- If
Audit.*(orServerEvents.createAuditServerEvent) ever rejects (e.g., transient DB issues), these promises will be unhandled, which in modern Node.js can terminate the process depending on configuration.To keep audits non-blocking but safe, consider wrapping them in a catch:
void Audit.actionPerformed(...).catch((err) => { this.logger.error({ msg: 'ABAC audit emission failed', err }); });Same pattern could be applied to the other
void Audit.*sites.Also applies to: 671-737, 758-821
ee/packages/abac/src/audit.ts (1)
119-134:actionPerformedis hard‑coded to revocations despite its generic name
Audit.actionPerformedalways emitsaction: 'revoked-object-access', but the name suggests it could cover other ABAC actions (e.g., approved manual access, policy evaluation outcomes).If you expect additional ABAC membership actions, consider either:
- Adding an
actionparameter (with a string literal union inIServerEventAbacActionPerformed), or- Renaming this helper to something like
accessRevokedand adding separate helpers for other actions later.packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts (1)
65-79: Aligntliteral for'abac.object.attributes.removed'with its event nameIn the
IServerEventsaugmentation,'abac.object.attributes.removed'is mapped toIServerEventAbacObjectAttributeChanged, whosetis declared as:t: 'abac.object.attribute.changed';This means
IServerEvents['abac.object.attributes.removed']['t']will still be typed as'abac.object.attribute.changed', which is misleading and may break code that narrows ont.It would be clearer to either introduce a dedicated interface or widen the literal:
+interface IServerEventAbacObjectAttributesRemoved + extends IAuditServerEventType< + | { key: 'room'; value: MinimalRoom } + | { key: 'reason'; value: AbacAuditReason } + | { key: 'previous'; value: IAbacAttributeDefinition[] } + | { key: 'current'; value: IAbacAttributeDefinition[] | null } + | { key: 'change'; value: AbacAttributeDefinitionChangeType } + > { + t: 'abac.object.attributes.removed'; +} ... declare module '../IServerEvent' { interface IServerEvents { 'abac.subject.attribute.changed': IServerEventAbacSubjectAttributeChanged; 'abac.object.attribute.changed': IServerEventAbacObjectAttributeChanged; 'abac.attribute.changed': IServerEventAbacAttributeChanged; 'abac.action.performed': IServerEventAbacActionPerformed; - 'abac.object.attributes.removed': IServerEventAbacObjectAttributeChanged; + 'abac.object.attributes.removed': IServerEventAbacObjectAttributesRemoved; } }This keeps the event key and the
tliteral in sync for both variants.
📜 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 (15)
apps/meteor/ee/server/api/abac/index.ts(13 hunks)apps/meteor/ee/server/api/abac/schemas.ts(2 hunks)apps/meteor/tests/end-to-end/api/abac.ts(4 hunks)ee/packages/abac/src/audit.ts(1 hunks)ee/packages/abac/src/can-access-object.spec.ts(1 hunks)ee/packages/abac/src/helper.ts(1 hunks)ee/packages/abac/src/index.spec.ts(34 hunks)ee/packages/abac/src/index.ts(22 hunks)ee/packages/abac/src/subject-attributes-validations.spec.ts(1 hunks)ee/packages/abac/src/user-auto-removal.spec.ts(14 hunks)packages/core-services/src/index.ts(1 hunks)packages/core-services/src/types/IAbacService.ts(1 hunks)packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts(1 hunks)packages/core-typings/src/index.ts(2 hunks)packages/models/src/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ee/packages/abac/src/subject-attributes-validations.spec.ts
- packages/core-services/src/index.ts
- packages/core-typings/src/index.ts
- ee/packages/abac/src/can-access-object.spec.ts
- packages/models/src/index.ts
🧰 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:
apps/meteor/ee/server/api/abac/schemas.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/helper.tsee/packages/abac/src/index.spec.tsee/packages/abac/src/index.tsapps/meteor/tests/end-to-end/api/abac.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tsapps/meteor/ee/server/api/abac/index.tsee/packages/abac/src/audit.tspackages/core-services/src/types/IAbacService.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/user-auto-removal.spec.tsee/packages/abac/src/index.spec.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/ee/server/api/abac/schemas.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:
apps/meteor/ee/server/api/abac/schemas.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/index.spec.tsee/packages/abac/src/index.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/index.tsee/packages/abac/src/audit.tspackages/core-services/src/types/IAbacService.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:
apps/meteor/ee/server/api/abac/schemas.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/index.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/index.tsee/packages/abac/src/audit.tspackages/core-services/src/types/IAbacService.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 : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
ee/packages/abac/src/user-auto-removal.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/user-auto-removal.spec.tsee/packages/abac/src/index.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/user-auto-removal.spec.tsee/packages/abac/src/index.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/user-auto-removal.spec.tsee/packages/abac/src/index.spec.tsapps/meteor/tests/end-to-end/api/abac.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/user-auto-removal.spec.tsee/packages/abac/src/index.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/user-auto-removal.spec.tsee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/index.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/index.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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
ee/packages/abac/src/index.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
ee/packages/abac/src/index.spec.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/abac/src/index.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: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
🧬 Code graph analysis (5)
ee/packages/abac/src/helper.ts (1)
packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
ee/packages/abac/src/index.ts (3)
ee/packages/abac/src/helper.ts (1)
diffAttributes(36-78)ee/packages/abac/src/audit.ts (1)
Audit(29-146)packages/core-services/src/types/IAbacService.ts (1)
AbacActor(11-11)
apps/meteor/tests/end-to-end/api/abac.ts (2)
apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts (3)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
packages/core-services/src/types/IAbacService.ts (3)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)packages/core-typings/src/IAbacAttribute.ts (2)
IAbacAttributeDefinition(3-14)IAbacAttribute(16-16)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
apps/meteor/tests/end-to-end/api/abac.ts (1)
409-461: Dedicated-room delete test is well‑scoped and isolatedThe “DELETE room attribute key (dedicated room)” block correctly sets up its own room, attributes, and user, asserts only the targeted key is removed, and cleans up the room in
after. No issues from an isolation or correctness standpoint.apps/meteor/ee/server/api/abac/schemas.ts (1)
1-1: Audit events query/response schemas align with /abac/audit usageImporting
IAuditServerActorand definingGETAbacAuditEventsQuerySchema/GETAbacAuditEventsResponseSchemais consistent with the new/abac/auditendpoint: query supports optional actor/start/end/offset/count and the response enforcesevents,count,offset, andtotalwith asuccess: truewrapper. No structural issues spotted.Also applies to: 152-233
ee/packages/abac/src/user-auto-removal.spec.ts (1)
7-8: Actor-aware auto-removal tests exercise audit semantics thoroughlyThreading
fakeActorthrough the AbacService calls and spying onAudit.actionPerformedgives good coverage of when removals should and should not be audited (single/multi-attribute additions, idempotency, edge cases, and larger populations). The MongoMemoryServer +registerServiceModelssetup keeps tests realistic without hitting external services.Also applies to: 32-33, 80-82, 88-90, 128-143, 164-170, 190-196, 216-219, 274-289, 314-331, 350-383, 410-424
ee/packages/abac/src/index.spec.ts (1)
5-6: Unit tests correctly updated for actor-aware service API and new models surface
fakeActoris consistently passed into all mutating AbacService methods that now expect an actor, keeping the tests compiling and representative of real usage.- The additional
AbacAttributes.findOneAndUpdatemock andServerEvents.createAuditServerEventstub future‑proof the tests against audit internals without over-coupling expectations.- Existing business-logic assertions (validation, in-use checks, error mapping, limit enforcement, etc.) remain intact.
No issues spotted here.
Also applies to: 25-26, 45-59, 386-417, 519-668, 671-817, 868-933, 935-1179
ee/packages/abac/src/index.ts (1)
285-317: Room attribute audits look consistent with behavior; no functional issues spotted
setRoomAbacAttributes,updateRoomAbacAttributeValues,removeRoomAbacAttribute,addRoomAbacAttributeByKey, andreplaceRoomAbacAttributeByKeyall:
- Enforce default/teamDefault restrictions before mutating.
- Validate definitions and limits via
ensureAttributeDefinitionsExistand simple cardinality checks.- Emit appropriate audit events (
objectAttributeChanged,objectAttributeRemoved,objectAttributesRemoved) with previous/current snapshots and change types (updated,key-added,key-updated,key-removed).- Only call
onRoomAttributesChangedwhen constraints are tightened (new keys or added values), matching the tests’ expectations.This matches the ABAC-24 requirement to audit room attribute mutations and membership reevaluation triggers.
Also applies to: 389-437, 444-478, 480-555
apps/meteor/ee/server/api/abac/index.ts (1)
2-7: Actor threading and /abac/audit endpoint wiring look consistent
getActorFromUsercorrectly mapsIUserinto theAbacActorshape and is passed to all mutating and listingAbaccalls from this module, giving the service layer the context it needs for auditing.- The new
/abac/auditendpoint:
- Uses the Ajv query/response schemas you added.
- Applies pagination and optional
sort.tshandling.- Filters
ServerEventsby time window, actor (viaconvertSubObjectsIntoPaths), and ABAC event types.- Gates access by
authRequired,abac-managementpermission, and['abac', 'auditing']licenses.This matches the intended read side for the new ABAC audit events.
Also applies to: 32-40, 64-67, 88-89, 115-116, 142-143, 162-163, 185-195, 244-245, 269-270, 288-289, 307-308, 347-355, 360-409
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
ee/packages/abac/src/audit.ts (3)
68-105:objectAttributeChangedandobjectAttributeRemovedare identical.These two methods have identical implementations (same event name, same payload structure, same actor construction). If they're meant to serve different purposes, they should either:
- Use different event names, or
- Be consolidated into a single method
Otherwise, this duplication creates maintenance burden and potential confusion.
Note: These methods also have the hardcoded IP/useragent issue (lines 84, 103) and non-null assertions (lines 84, 103) flagged in other comments.
106-118: Note: This method shares the hardcoded IP/useragent issue (line 116) and non-null assertion issue (line 116) flagged in other comments.
29-67: Hardcoded IP and user agent reduce audit trail quality.Lines 39, 52, and 65 hardcode
ip: '0.0.0.0'anduseragent: ''for audit logs. For effective security auditing, these fields should capture actual request metadata (IP address and user agent from the HTTP request context). Consider threading the request context through the actor parameter or adding these as separate parameters.
Note: The non-null assertions on
actor.username!at lines 39, 52, 65 were already flagged in a previous review.
🧹 Nitpick comments (2)
ee/packages/abac/src/audit.ts (2)
135-146: Hardcodedreason: 'ldap-sync'limits reusability.The method name
subjectAttributeChangedis generic, but the reason is hardcoded to 'ldap-sync'. If subject attributes can change from other sources (e.g., API updates, other sync mechanisms), consider acceptingreasonas a parameter with 'ldap-sync' as the default.
38-38: Remove redundant type assertion.The explicit
as EventPayload<'abac.attribute.changed'>cast is unnecessary—TypeScript infers the type from theauditfunction's generic parameter. Remove it for consistency with other methods (lines 45-51, 58-63, etc.).
📜 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 (2)
ee/packages/abac/src/audit.ts(1 hunks)packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/audit.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
📚 Learning: 2025-11-27T17:56:26.027Z
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.027Z
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/audit.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/audit.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/audit.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
ee/packages/abac/src/audit.ts (2)
1-23: LGTM: Clean type definitions for audit events.The imports and type definitions establish a type-safe foundation for ABAC audit events, with minimal public API surface.
25-27: LGTM: Clean audit helper.The internal helper cleanly delegates to ServerEvents with proper typing.
Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/ABAC-24
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Tests
Types & Schemas
Utilities
Models/Exports
✏️ Tip: You can customize this high-level summary in your review settings.