feat(ABAC): add new column to ABAC logs page #37837
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 |
|
WalkthroughExtended ABAC audit logging to support object attribute removal events with room membership tracking. Updated LogsPage UI to display room information and handle new action types. Added server event type definitions and international text labels for access control actions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 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 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 19:43", "12/16 20:51 (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, 0.11, 0.11, 0.11, 0.11, 0.11, 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, 0.11, 0.11, 0.11, 0.11, 0.11, 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, 0.12, 0.12, 0.12, 0.12, 0.12, 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, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 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, 0.11, 0.11, 0.11, 0.11, 0.11, 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, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 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.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 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, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 22 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Tue, 16 Dec 2025 20:52:00 GMT |
MartinSchoeler
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
608-613: Fix type safety issue:room.nameis not guaranteed to exist.The function signature on line 608 types
roomasAtLeast<IRoom, '_id'>, which only guarantees the_idproperty exists. However, line 613 accessesroom.namewithout ensuring it's available. Tracing the callers confirms thatnameis not included:
- Line 554:
roomisPick<IRoom, '_id' | 't' | 'teamId' | 'prid' | 'abacAttributes'>(noname)- Line 591:
roomisAtLeast<IRoom, '_id' | 't' | 'teamMain' | 'abacAttributes'>(noname)- Line 627:
roomcomes from cursors with projection{ _id: 1 }(lines 642-648, 658)This will result in
name: undefinedbeing passed to the audit system, creating incomplete audit logs.Apply this diff to fix the type signature:
-private async removeUserFromRoom(room: AtLeast<IRoom, '_id'>, user: IUser, reason: AbacAuditReason): Promise<void> { +private async removeUserFromRoom(room: AtLeast<IRoom, '_id' | 'name'>, user: IUser, reason: AbacAuditReason): Promise<void> {Additionally, update the query projections on lines 647 and 658 to include the
namefield:-{ projection: { _id: 1 } }, +{ projection: { _id: 1, name: 1 } },Also update line 510 to include
namein the room type:-room: Pick<IRoom, '_id' | 't' | 'teamId' | 'prid' | 'abacAttributes'>, +room: Pick<IRoom, '_id' | 't' | 'teamId' | 'prid' | 'abacAttributes' | 'name'>,And line 565:
-room: AtLeast<IRoom, '_id' | 't' | 'teamMain' | 'abacAttributes'>, +room: AtLeast<IRoom, '_id' | 't' | 'teamMain' | 'abacAttributes' | 'name'>,
🧹 Nitpick comments (2)
packages/i18n/src/locales/en.i18n.json (1)
78-80: ABAC i18n keys look consistent; only optional wording tweak to considerThe new keys and values are valid and align with existing ABAC-related entries; no structural or naming blockers here. If you want slightly clearer labels for the logs UI, consider something like “Automatic access removal” / “Manual access granted” for the two access actions, but that’s purely cosmetic. Also, adding them only to
en.i18n.jsonmatches the repository’s i18n workflow.apps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsx (1)
80-90: Consider moving JSX creation out of the select transform.Creating
UserAvatarinside theselectcallback couples data transformation with presentation. A cleaner approach would be to store only theuserIdin the data object and render the avatar component in the JSX.const eventInfo = { id: event._id, user: event.actor?.type === 'user' ? event.actor.username : t('System'), - ...(event.actor?.type === 'user' && { userAvatar: <UserAvatar size='x28' userId={event.actor._id} /> }), + ...(event.actor?.type === 'user' && { userId: event.actor._id }), timestamp: new Date(event.ts), element: t('ABAC_Room'), action: getActionLabel(event.data?.find((item) => item.key === 'change')?.value), room: undefined, };Then in the render:
{eventInfo.userId && ( <Box is='span' mie={4}> <UserAvatar size='x28' userId={eventInfo.userId} /> </Box> )}
📜 Review details
Configuration used: Organization 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 (4)
apps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsx(4 hunks)ee/packages/abac/src/index.ts(2 hunks)packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🧰 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/index.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tsapps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsx
🧠 Learnings (9)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
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:
ee/packages/abac/src/index.tspackages/core-typings/src/ServerAudit/IAuditServerAbacAction.tsapps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsx
📚 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/index.tspackages/i18n/src/locales/en.i18n.json
📚 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-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: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
ee/packages/abac/src/index.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/index.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/index.ts
📚 Learning: 2025-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-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/i18n/src/locales/en.i18n.json
🧬 Code graph analysis (2)
ee/packages/abac/src/index.ts (1)
ee/packages/abac/src/audit.ts (1)
Audit(30-148)
apps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsx (2)
packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts (2)
AbacAttributeDefinitionChangeType(10-19)AbacActionPerformed(8-8)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(142-159)
⏰ 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). (5)
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 📦 Meteor Build (coverage)
🔇 Additional comments (4)
packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts (1)
67-76: LGTM! New event type follows established patterns.The
IServerEventAbacObjectAttributesRemovedinterface is consistent with the existingIServerEventAbacObjectAttributeChangedstructure, and the module augmentation correctly maps the new event type.apps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsx (3)
48-75: LGTM! Action label function properly extended.The function signature and switch cases correctly accommodate the new
AbacActionPerformedtype values.
91-119: LGTM! Event mapping logic is well-structured.The switch statement correctly handles all ABAC event types, and the shared case for
abac.object.attribute.changedandabac.object.attributes.removedis appropriate given their identical data structure. The defensive null return for unknown types is properly handled in the render logic.
172-195: LGTM! Render logic correctly handles the new data structure.The null check guards against unhandled event types, and the title attribute on the name cell provides good UX for truncated content.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37837 +/- ##
=============================================
+ Coverage 54.35% 54.57% +0.22%
=============================================
Files 2639 2639
Lines 50115 50102 -13
Branches 11217 11212 -5
=============================================
+ Hits 27241 27345 +104
+ Misses 20699 20571 -128
- Partials 2175 2186 +11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
Issue(s)
ABAC-102
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Localization
✏️ Tip: You can customize this high-level summary in your review settings.