refactor: More polish #37736
refactor: More polish #37736
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 |
|
WalkthroughThe changes refactor ABAC (Attribute-Based Access Control) management across multiple modules by introducing helper predicates to detect ABAC-managed rooms and teams, centralizing user removal logic with consistent audit logging, and adding a new function to validate attribute definitions. Existing control flow is preserved while reducing code duplication. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 16:35", "12/10 18:45 (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]
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]
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]
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]
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]
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]
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.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]
Statistics (last 17 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 10 Dec 2025 18:45:49 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37736 +/- ##
=============================================
- Coverage 54.50% 54.28% -0.22%
=============================================
Files 2633 2633
Lines 50105 50105
Branches 11224 11224
=============================================
- Hits 27308 27198 -110
- Misses 20607 20732 +125
+ Partials 2190 2175 -15
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
KevLehman
force-pushed
the
refactor/review
branch
from
0326915 to
b3b0adb
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: 0
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/helper.spec.ts (1)
264-279: Import and use the exportedbuildNonCompliantConditionsfunction instead of defining a local copy.The function
buildNonCompliantConditionsis exported fromhelper.ts(line 179), but the test defines a local implementation instead of importing it. This creates divergence risk and means the test doesn't verify the actual implementation. Remove the local function definition and import the real function from the helper module.
🧹 Nitpick comments (4)
ee/packages/abac/src/index.ts (1)
620-634: Excellent refactoring—centralizes user removal with consistent auditing and error handling.This helper consolidates the user removal logic that was previously duplicated across multiple code paths, ensuring:
- Consistent removal options (
skipAppPreEvents,customSystemMessage)- Uniform audit logging with contextual reasons
- Graceful error handling that logs failures without blocking other operations
ee/packages/abac/src/helper.spec.ts (3)
70-78: Remove duplicate test case.This test is identical to the one at lines 44-51, testing the same scenario with the same inputs and expected outcome.
Apply this diff to remove the duplicate:
- it('throws when a key exceeds the maximum number of unique values (bucket size limit)', () => { - const values = Array.from({ length: MAX_ABAC_ATTRIBUTE_VALUES + 1 }, (_, i) => `value-${i}`); - - expect(() => - validateAndNormalizeAttributes({ - role: values, - }), - ).toThrow('error-invalid-attribute-values'); - }); -
80-86: Remove duplicate test case.This test is identical to the one at lines 36-42, testing the same scenario with identical inputs and expected behavior.
Apply this diff to remove the duplicate:
- it('throws when a key has only invalid/empty values after sanitization', () => { - expect(() => - validateAndNormalizeAttributes({ - role: [' ', '\n', '\t'], - }), - ).toThrow('error-invalid-attribute-values'); - });
465-465: Consider removing inline comment.Per coding guidelines, avoid code comments. The implementation detail about Set insertion order could be incorporated into the test description if needed.
Apply this diff:
- // Order is preserved by insertion into the Set in implementation: ['Eng', 'Sales'] expect(result).toEqual({
📜 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 (5)
apps/meteor/app/channel-settings/server/methods/saveRoomSettings.ts(5 hunks)ee/packages/abac/src/can-access-object.spec.ts(1 hunks)ee/packages/abac/src/helper.spec.ts(26 hunks)ee/packages/abac/src/index.ts(6 hunks)ee/packages/abac/src/service.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ee/packages/abac/src/service.spec.ts
- ee/packages/abac/src/can-access-object.spec.ts
- apps/meteor/app/channel-settings/server/methods/saveRoomSettings.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.spec.tsee/packages/abac/src/index.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 (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-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-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.spec.tsee/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-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-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-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-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
🧬 Code graph analysis (2)
ee/packages/abac/src/helper.spec.ts (1)
ee/packages/abac/src/helper.ts (7)
MAX_ABAC_ATTRIBUTE_KEYS(13-13)validateAndNormalizeAttributes(93-143)MAX_ABAC_ATTRIBUTE_VALUES(14-14)ensureAttributeDefinitionsExist(153-177)diffAttributeSets(246-324)buildNonCompliantConditions(179-190)extractAttribute(16-47)
ee/packages/abac/src/index.ts (4)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)packages/core-typings/src/IUser.ts (1)
IUser(187-259)packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts (1)
AbacAuditReason(6-6)ee/packages/abac/src/audit.ts (1)
Audit(29-142)
🔇 Additional comments (8)
ee/packages/abac/src/index.ts (3)
4-13: LGTM! Import changes support the new helper methods.The addition of
AbacAuditReasonandFindCursortypes is minimal and necessary for the new helper methods introduced in this refactoring.Also applies to: 17-17
636-643: LGTM! Batch removal helper with proper rate limiting.The
removeUserFromRoomListmethod effectively handles bulk user removals with appropriate rate limiting to prevent server overload.
562-562: LGTM! Call sites correctly updated to use the new helpers.All invocations use appropriate audit reasons ('realtime-policy-eval', 'room-attributes-change', 'ldap-sync') and apply rate limiting where needed. The type inference at line 649 aligns with concise TypeScript practices.
Also applies to: 603-603, 649-649, 662-662, 672-672
ee/packages/abac/src/helper.spec.ts (5)
8-20: LGTM! Mock setup is well-structured.The Jest mock for
AbacAttributes.findcorrectly simulates the MongoDB driver pattern withtoArray()returning the mock function result, enabling proper async testing ofensureAttributeDefinitionsExist.
89-116: LGTM! Well-structured test coverage.The new test suite properly covers key scenarios for
ensureAttributeDefinitionsExist: no-op on empty input, missing definitions, and invalid values. The mock reset inafterEachprevents test pollution.
118-259: LGTM! Assertion migration is correct.The conversion from Chai to Jest matchers (
toEqual) is accurate, and the test suite comprehensively covers edge cases including empty sets, additions, removals, and order independence.
337-431: LGTM! Comprehensive test coverage.The assertion updates to Jest
toEqualare correct, and the tests thoroughly cover single keys, multiple keys, and edge cases like empty value arrays.
433-494: LGTM! Assertion migration is correct.The conversion to Jest matchers (
toEqual,toBeUndefined) is accurate, and the tests comprehensively cover edge cases including missing keys, undefined values, trimming, deduplication, and filtering.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Release Notes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.