chore: Add name prop to object's audit #37655
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 |
|
|
Caution Review failedThe pull request is closed. WalkthroughThe PR adds the room Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
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 |
KevLehman
force-pushed
the
chore/name-audit
branch
from
9e3c724 to
508b8ee
Compare
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 15:59", "12/01 16:35 (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]
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]
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]
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]
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]
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]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 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]
Statistics (last 10 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Mon, 01 Dec 2025 16:35:56 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37655 +/- ##
============================================
Coverage ? 54.31%
============================================
Files ? 2657
Lines ? 50112
Branches ? 11207
============================================
Hits ? 27219
Misses ? 20718
Partials ? 2175
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ee/packages/abac/src/index.ts (3)
285-315: Addingnameto room projection and audits forsetRoomAbacAttributeslooks goodFetching
namein thefindOneByIdAndTypeprojection and passing{ _id: room._id, name: room.name }intoAudit.objectAttributesRemoved/Audit.objectAttributeChangedcleanly enriches object identity in ABAC audits without changing behavior.If you want to reduce repetition, consider a small alias for this recurring room shape (and maybe a shared projection constant) reused across these ABAC methods, but this is purely optional.
387-439: Includenamein thekey-updatedaudit for consistencyIn
updateRoomAbacAttributeValues, thekey-addedpath already sends{ _id: room._id, name: room.name }toAudit.objectAttributeChanged, but thekey-updatedpath still sends only{ _id: room._id }. This makes audit payloads inconsistent even thoughnameis available from the projection.Suggest aligning the
key-updatedbranch:- await Rooms.updateAbacAttributeValuesArrayFilteredById(rid, key, values); - void Audit.objectAttributeChanged({ _id: room._id }, room.abacAttributes || [], [{ key, values }], 'key-updated', actor); + await Rooms.updateAbacAttributeValuesArrayFilteredById(rid, key, values); + void Audit.objectAttributeChanged( + { _id: room._id, name: room.name }, + room.abacAttributes || [], + [{ key, values }], + 'key-updated', + actor, + );
446-480: Also attachnamewhen removing the last ABAC attribute from a roomIn
removeRoomAbacAttribute, the non-last-attribute path correctly callsAudit.objectAttributeRemoved({ _id: room._id, name: room.name }, ...), but the “last attribute” path still uses{ _id: room._id }withAudit.objectAttributesRemoved, so some object-removal audits will havenamewhile others will not.Since
nameis now projected, you can make this consistent:- // if is the last attribute, just remove all + // if is the last attribute, just remove all if (previous.length === 1) { await Rooms.unsetAbacAttributesById(rid); - void Audit.objectAttributesRemoved({ _id: room._id }, previous, actor); + void Audit.objectAttributesRemoved({ _id: room._id, name: room.name }, previous, actor); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ee/packages/abac/src/index.ts(11 hunks)packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts(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:
packages/core-typings/src/ServerAudit/IAuditServerAbacAction.tsee/packages/abac/src/index.ts
🧠 Learnings (5)
📓 Common learnings
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.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-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-typings/src/ServerAudit/IAuditServerAbacAction.tsee/packages/abac/src/index.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.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/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 (2)
packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
ee/packages/abac/src/index.ts (4)
packages/models/src/index.ts (1)
Rooms(207-207)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)ee/packages/abac/src/audit.ts (1)
Audit(29-142)packages/core-services/src/types/IAbacService.ts (1)
AbacActor(11-11)
🔇 Additional comments (3)
packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts (1)
3-4: MinimalRoom includingnamealigns with updated ABAC auditsExtending
MinimalRoomto includenamematches the new audit payloads and remains type-safe sinceIRoom.nameis optional; this should be a non-breaking, useful enhancement for consumers of ABAC audit events.ee/packages/abac/src/index.ts (2)
485-513:addRoomAbacAttributeByKeyroom projection and audits are consistentThe updated
findOneByIdAndTypeprojection (includingname) and theAudit.objectAttributeChanged({ _id: room._id, name: room.name }, ...)call are consistent with the new MinimalRoom shape and with other ABAC methods; behavior and typing both look correct.
515-565:replaceRoomAbacAttributeByKeycorrectly enriches all object audits with room nameBoth the
key-updatedandkey-addedbranches now pass{ _id: room._id, name: room.name }toAudit.objectAttributeChanged, matching the new room projection and ensuring these audits always include room name when available. No functional or typing issues here.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.