chore: ABAC QoL #37676
chore: ABAC QoL #37676
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 |
|
WalkthroughIntroduces a typed ABAC error hierarchy, strict attribute validation/normalization with size limits, memoized attribute-definition lookups via mem, and replaces ad-hoc diff helpers with a unified diffAttributeSets; index.ts and tests updated to use the new helpers and error classes. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 19:10", "12/03 20:24 (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]
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]
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]
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]
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]
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]
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.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]
Statistics (last 12 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 03 Dec 2025 20:24:04 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37676 +/- ##
==========================================
Coverage 54.27% 54.27%
==========================================
Files 2637 2637
Lines 50050 50050
Branches 11202 11202
==========================================
Hits 27163 27163
+ Misses 20720 20717 -3
- Partials 2167 2170 +3
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: 3
🧹 Nitpick comments (4)
ee/packages/abac/src/helper.spec.ts (1)
45-60: Consider importingbuildNonCompliantConditionsfromhelper.tsinstead of redefining it.The
buildNonCompliantConditionsfunction is reimplemented locally in the test file. According to the relevant code snippets, this function exists inee/packages/abac/src/helper.ts(lines 177-188). Testing a local copy doesn't validate the actual implementation.-import { validateAndNormalizeAttributes, MAX_ABAC_ATTRIBUTE_KEYS, MAX_ABAC_ATTRIBUTE_VALUES, diffAttributeSets } from './helper'; +import { validateAndNormalizeAttributes, MAX_ABAC_ATTRIBUTE_KEYS, MAX_ABAC_ATTRIBUTE_VALUES, diffAttributeSets, buildNonCompliantConditions } from './helper'; describe('buildNonCompliantConditions', () => { - type AttributeDef = { key: string; values: string[] }; - - const buildNonCompliantConditions = (attrs: AttributeDef[]): any[] => { - if (!attrs.length) { - return []; - } - - return attrs.map((attr) => ({ - abacAttributes: { - $not: { - $elemMatch: { - key: attr.key, - values: { $all: attr.values }, - }, - }, - }, - })); - }; - it('returns empty array for empty attributes list', () => {ee/packages/abac/src/errors.ts (1)
1-13: Missing error class forUsernamesNotMatchingAbacAttributescode.The
AbacErrorCode.UsernamesNotMatchingAbacAttributesis defined in the enum (line 10), but there's no corresponding error class likeAbacUsernamesNotMatchingAbacAttributesError. This inconsistency could cause confusion when the error handling pattern expects a dedicated class for each code.Add the missing error class:
+export class AbacUsernamesNotMatchingAbacAttributesError extends AbacError { + constructor(details?: unknown) { + super(AbacErrorCode.UsernamesNotMatchingAbacAttributes, details); + } +}ee/packages/abac/src/helper.ts (2)
110-112: Semantic mismatch: key limit violations throw value error.When exceeding
MAX_ABAC_ATTRIBUTE_KEYS, the code throwsAbacInvalidAttributeValuesErrorinstead ofAbacInvalidAttributeKeyError. This could confuse API consumers trying to handle specific error cases.Consider throwing the semantically correct error:
if (!aggregated.has(key)) { if (aggregated.size >= MAX_ABAC_ATTRIBUTE_KEYS) { - throw new AbacInvalidAttributeValuesError(); + throw new AbacInvalidAttributeKeyError(); } aggregated.set(key, bucket); }if (aggregated.size > MAX_ABAC_ATTRIBUTE_KEYS) { - throw new AbacInvalidAttributeValuesError(); + throw new AbacInvalidAttributeKeyError(); }Also applies to: 131-133
278-320: Duplicated logic for checking additions.The addition-detection logic at lines 278-298 and 299-319 is identical. The conditional split provides no benefit since additions need to be checked regardless of the
removedflag.Simplify by always checking for additions:
} - // Check additions (new keys or new values on existing keys) - if (!removed) { - for (const [key, nextValues] of nextMap) { - const currentValues = currentMap.get(key); - - if (!currentValues) { - // New key added - added = true; - break; - } - - for (const v of nextValues) { - if (!currentValues.has(v)) { - added = true; - break; - } - } - - if (added) { - break; - } - } - } else { - // Even if we've already seen removals, we might still want to know if additions happened too - for (const [key, nextValues] of nextMap) { - const currentValues = currentMap.get(key); - - if (!currentValues) { + // Check additions (new keys or new values on existing keys) + for (const [key, nextValues] of nextMap) { + const currentValues = currentMap.get(key); + + if (!currentValues) { + added = true; + break; + } + + for (const v of nextValues) { + if (!currentValues.has(v)) { added = true; break; } - - for (const v of nextValues) { - if (!currentValues.has(v)) { - added = true; - break; - } - } - - if (added) { - break; - } + } + + if (added) { + break; } }
📜 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 (6)
ee/packages/abac/package.json(1 hunks)ee/packages/abac/src/errors.ts(1 hunks)ee/packages/abac/src/helper.spec.ts(1 hunks)ee/packages/abac/src/helper.ts(3 hunks)ee/packages/abac/src/index.ts(21 hunks)ee/packages/abac/src/service.spec.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/service.spec.tsee/packages/abac/src/errors.tsee/packages/abac/src/helper.spec.tsee/packages/abac/src/index.tsee/packages/abac/src/helper.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.tsee/packages/abac/src/helper.spec.ts
🧠 Learnings (10)
📓 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.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.tsee/packages/abac/src/errors.tsee/packages/abac/src/index.tsee/packages/abac/src/helper.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.tsee/packages/abac/src/errors.tsee/packages/abac/src/index.tsee/packages/abac/src/helper.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.tsee/packages/abac/src/index.tsee/packages/abac/src/helper.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: 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:
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-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-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:
ee/packages/abac/src/index.ts
🧬 Code graph analysis (3)
ee/packages/abac/src/helper.spec.ts (1)
ee/packages/abac/src/helper.ts (5)
validateAndNormalizeAttributes(93-143)MAX_ABAC_ATTRIBUTE_VALUES(14-14)MAX_ABAC_ATTRIBUTE_KEYS(13-13)buildNonCompliantConditions(178-189)diffAttributeSets(245-323)
ee/packages/abac/src/index.ts (5)
ee/packages/abac/src/helper.ts (3)
diffAttributeSets(245-323)getAbacRoom(227-243)MAX_ABAC_ATTRIBUTE_KEYS(13-13)ee/packages/abac/src/errors.ts (6)
AbacInvalidAttributeValuesError(29-33)AbacDuplicateAttributeKeyError(53-57)AbacAttributeNotFoundError(41-45)AbacAttributeInUseError(47-51)AbacUnsupportedObjectTypeError(77-81)AbacUnsupportedOperationError(83-87)ee/packages/abac/src/audit.ts (1)
Audit(29-142)packages/core-services/src/types/IAbacService.ts (1)
AbacActor(11-11)packages/core-typings/src/ISubscription.ts (1)
ISubscription(10-77)
ee/packages/abac/src/helper.ts (3)
ee/packages/abac/src/errors.ts (5)
AbacInvalidAttributeKeyError(35-39)AbacInvalidAttributeValuesError(29-33)AbacAttributeDefinitionNotFoundError(59-63)AbacRoomNotFoundError(65-69)AbacCannotConvertDefaultRoomToAbacError(71-75)packages/models/src/index.ts (2)
AbacAttributes(232-232)Rooms(207-207)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
🔇 Additional comments (11)
ee/packages/abac/src/service.spec.ts (2)
606-610: LGTM!The test correctly expects
error-invalid-attribute-valuesfor empty value arrays, aligning with the newAbacInvalidAttributeValuesErrorclass from the error hierarchy.
633-642: LGTM!The updated test correctly verifies that
onRoomAttributesChangedis not invoked when the input contains duplicate values that normalize to the same set as existing attributes. This aligns with the newdiffAttributeSetslogic that properly detects no actual changes occurred.ee/packages/abac/src/helper.spec.ts (1)
5-40: LGTM!The
validateAndNormalizeAttributestests provide good coverage for key normalization, value deduplication, empty/whitespace values, max values limit, and max keys limit.ee/packages/abac/src/errors.ts (1)
15-27: LGTM!The
AbacErrorbase class is well-designed with proper prototype chain restoration viaObject.setPrototypeOf(this, new.target.prototype), ensuringinstanceofchecks work correctly after TypeScript transpilation.ee/packages/abac/src/index.ts (4)
12-31: LGTM!Good refactoring to import typed error classes and helper functions. The centralized imports improve consistency and maintainability.
603-611: LGTM!Good addition of
.catch()error handling for user removal promises. Logging errors rather than propagating them is appropriate here since these are cleanup operations that shouldn't block the main flow.
321-323: LGTM!Correct use of
diffAttributeSetsto only triggeronRoomAttributesChangedwhen there are actual additions, preventing unnecessary processing for normalization-only changes.
333-335: LGTM!Proper enforcement of
MAX_ABAC_ATTRIBUTE_KEYSlimit when adding new attribute keys.ee/packages/abac/package.json (1)
28-28: Thememdependency is actively used in the codebase. It's imported inee/packages/abac/src/helper.tsand applied to cache attribute definitions database lookups viagetAttributeDefinitionsCached = mem(getAttributeDefinitionsFromDb, {...}).ee/packages/abac/src/helper.ts (2)
152-176: LGTM!The validation logic is sound: first verifying all attribute keys exist in the database, then validating each value against the allowed set. The use of cached lookups improves performance for repeated validations.
227-243: LGTM!The function correctly enforces that ABAC attributes can only be set on private rooms (type
'p') and prevents conversion of default rooms. This aligns with the ABAC design where attributes are restricted to private rooms and teams. Based on learnings, this is consistent with the established pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ee/packages/abac/src/helper.ts (1)
145-151: Consider order-insensitive cacheKey for getAttributeDefinitionsCachedUsing
memwithcacheKey: JSON.stringifyfixes the identity issue for array arguments, but it still treats['dept', 'role']and['role', 'dept']as different cache entries. Since the Mongo$inquery is order-insensitive andensureAttributeDefinitionsExistpasses a set of unique keys, normalizing the key set before generating the cache key would give better cache hit rates:const getAttributeDefinitionsCached = mem(getAttributeDefinitionsFromDb, { maxAge: 30_000, cacheKey: ([keys]: [string[]]) => keys.slice().sort().join('\0'), });This keeps semantics unchanged while avoiding duplicate cache entries for the same logical key set.
Also applies to: 158-164
🧹 Nitpick comments (1)
ee/packages/abac/src/helper.ts (1)
246-324: diffAttributeSets implementation matches tests; minor cleanup opportunitiesFunctionally, the implementation aligns with the spec tests:
- Treats both current/next empty as
{ added: false, removed: false }.- Detects key removals or value removals first, then independently checks for additions (keys or values), including mixed add/remove cases.
- Is order-insensitive for values and for the presence of keys.
Two minor cleanups you might consider:
- The “additions” loop is duplicated in the
if (!removed)andelseblocks; factoring it into a small helper would reduce repetition without changing behavior.- The inline comments are somewhat redundant given the clear structure; if you want to adhere strictly to the “avoid code comments in the implementation” guideline, you could drop them and rely on self-descriptive naming.
Neither change is required for correctness, but they would slightly tighten the implementation.
📜 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 (3)
ee/packages/abac/src/helper.spec.ts(2 hunks)ee/packages/abac/src/helper.ts(3 hunks)ee/packages/abac/src/service.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/packages/abac/src/service.spec.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:
ee/packages/abac/src/helper.tsee/packages/abac/src/helper.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/helper.spec.ts
🧠 Learnings (8)
📓 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.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/helper.tsee/packages/abac/src/helper.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/helper.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/helper.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/helper.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/helper.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 `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
ee/packages/abac/src/helper.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/helper.spec.ts
🧬 Code graph analysis (2)
ee/packages/abac/src/helper.ts (3)
ee/packages/abac/src/errors.ts (5)
AbacInvalidAttributeKeyError(35-39)AbacInvalidAttributeValuesError(29-33)AbacAttributeDefinitionNotFoundError(59-63)AbacRoomNotFoundError(65-69)AbacCannotConvertDefaultRoomToAbacError(71-75)packages/models/src/index.ts (2)
AbacAttributes(232-232)Rooms(207-207)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
ee/packages/abac/src/helper.spec.ts (1)
ee/packages/abac/src/helper.ts (6)
validateAndNormalizeAttributes(93-143)MAX_ABAC_ATTRIBUTE_VALUES(14-14)MAX_ABAC_ATTRIBUTE_KEYS(13-13)diffAttributeSets(246-324)buildRoomNonCompliantConditionsFromSubject(203-226)extractAttribute(16-47)
⏰ 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 (7)
ee/packages/abac/src/helper.spec.ts (4)
3-10: Helper imports correctly reflect the updated ABAC helper APIThe imported symbols match the new helper.ts surface and are all exercised by the tests below; no unused or missing imports here.
12-47: validateAndNormalizeAttributes tests give solid coverage of key/value normalization and limitsThese tests nicely cover trimming, deduplication, empty-value rejection, and both per-key and total-key limit enforcement via the MAX_* constants. They align with the helper.ts implementation and should protect against regressions in the normalization rules.
49-190: diffAttributeSets test matrix thoroughly exercises add/remove semanticsThe scenarios here (empty↔non-empty, identical sets, order-insensitive equality, added/removed values and keys, and mixed changes) closely mirror the implementation logic in diffAttributeSets and should give high confidence that future changes won’t silently alter its boolean contract.
364-425: extractAttribute tests comprehensively cover scalar/array inputs and filtering rulesThe cases here (missing keys, scalar vs array values, trimming, deduplication, non-string filtering, and all-values-filtered-out) mirror the helper implementation closely and give good confidence that LDAP-derived attributes are normalized as expected.
ee/packages/abac/src/helper.ts (3)
1-14: New ABAC helper imports and MAX_ constants look consistent*Typing and imports line up with usage: ILDAPEntry/IAbacAttributeDefinition/IRoom and AbacAttributes/Rooms are all used, and exporting MAX_ABAC_ATTRIBUTE_KEYS/MAX_ABAC_ATTRIBUTE_VALUES as 10 matches the test expectations in helper.spec.ts.
93-143: validateAndNormalizeAttributes correctly enforces key pattern, deduplication, and size limitsThe implementation:
- Trims and validates keys against the allowed pattern.
- Aggregates values per normalized key, trimming, deduplicating, and skipping non-strings/whitespace.
- Enforces both per-key (MAX_ABAC_ATTRIBUTE_VALUES) and total-key (MAX_ABAC_ATTRIBUTE_KEYS) limits.
- Rejects keys that end up with no sanitized values.
This matches the new tests and the intended invariants for downstream ABAC logic.
227-244: getAbacRoom correctly restricts to private, non-default roomsThis helper:
- Restricts lookups to private rooms (
findOneByIdAndType(..., 'p', ...)).- Projects only the ABAC-relevant fields.
- Throws dedicated errors when the room is missing or marked as default/teamDefault, preventing ABAC conversion on default rooms.
That matches the documented ABAC behavior around private rooms and default/team-default restrictions. Based on learnings, this is the intended contract.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.