test: ABAC + LDAP #37615
test: ABAC + LDAP #37615
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 |
|
|
Warning Rate limit exceeded@KevLehman has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds OpenLDAP CI service and LDIF test data, includes Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI/OpenLDAP
participant Sync as LDAP Sync Job
participant Server as Rocket.Chat Server
participant DB as Database
participant ABAC as ABAC Engine
participant Room as Room Service
Note over CI,Sync: OpenLDAP seeded from LDIF in CI
Sync->>CI: query users & attributes (triggered or scheduled)
CI-->>Sync: return LDAP entries with ABAC-relevant attrs
Sync->>DB: create/update user records including abacAttributes
DB-->>Server: user data available (includes abacAttributes)
Server->>ABAC: evaluate user.abacAttributes vs room attribute rules
alt attributes match room policy
ABAC-->>Room: request add membership
Room-->>Server: add user to room
else attributes no longer match
ABAC-->>Room: request remove membership
Room-->>Server: remove user from room
end
Note over Sync,Server: periodic or forced sync re-evaluates memberships
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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:03 (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 19:03:14 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37615 +/- ##
=============================================
+ Coverage 54.00% 54.36% +0.35%
=============================================
Files 2657 2661 +4
Lines 50112 50149 +37
Branches 11207 11213 +6
=============================================
+ Hits 27061 27261 +200
+ Misses 20915 20719 -196
- Partials 2136 2169 +33
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/tests/end-to-end/api/abac.ts (1)
145-146: Remove debugconsole.logstatement.This appears to be a debug artifact that should be removed before merging.
it('POST should fail creating duplicate key', async () => { const response = await request.get(`${v1}/abac/attributes`).set(credentials).expect(200); - console.log(response.body); await request
🧹 Nitpick comments (4)
apps/meteor/tests/end-to-end/api/abac.ts (3)
1921-1958: Consolidate duplicatebeforehooks into a single hook.The static analysis correctly identifies two separate
beforehooks in the samedescribeblock (lines 1921-1943 and 1945-1958). While Mocha executes multiplebeforehooks sequentially, consolidating them improves readability and makes the setup flow clearer.- before(async () => { + before(async function () { + this.timeout(15000); await updateSetting('LDAP_Enable', true); // ... all settings ... await updateSetting( 'LDAP_ABAC_AttributeMap', JSON.stringify({ departmentNumber: 'department', telephoneNumber: 'phone', }), ); - }); - before(async function () { - this.timeout(10000); // Wait for background sync to run once before tests start await request.post(`${v1}/ldap.syncNow`).set(credentials); await sleep(5000); // Force abac attribute sync for user john.young await request .post(`${v1}/abac/users/sync`) .set(credentials) .send({ emails: ['john.young@space.air'] }); await sleep(2000); });
1948-1957: Consider replacing fixedsleepcalls with polling for more reliable tests.Hardcoded sleep durations (5000ms, 2000ms) can cause flaky tests if sync takes longer or waste time if it's faster. Consider polling for the expected state:
// Example: Poll until user is synced const pollForUser = async (username: string, maxAttempts = 10) => { for (let i = 0; i < maxAttempts; i++) { const res = await request.get(`${v1}/users.info`).set(credentials).query({ username }); if (res.status === 200 && res.body.user) return res.body.user; await sleep(500); } throw new Error(`User ${username} not synced after ${maxAttempts} attempts`); };
2097-2103: Add assertion to verify the invite succeeded.The
groups.invitecall is missing.expect(200), which could mask failures during test setup.await request .post(`${v1}/groups.invite`) .set(credentials) .send({ roomId: roomIdWithAbac, usernames: ['david.scott', 'sergei.krikalev'], - }); + }) + .expect(200);development/ldap/02-data.ldif (1)
136-142: Minor: Remove excessive blank lines.Multiple consecutive blank lines between entries are unnecessary.
📜 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 (4)
apps/meteor/app/lib/server/functions/getFullUserData.ts(1 hunks)apps/meteor/tests/end-to-end/api/abac.ts(2 hunks)development/ldap/02-data.ldif(1 hunks)docker-compose-ci.yml(2 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:
apps/meteor/app/lib/server/functions/getFullUserData.tsapps/meteor/tests/end-to-end/api/abac.ts
🧠 Learnings (5)
📓 Common learnings
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.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:
apps/meteor/app/lib/server/functions/getFullUserData.tsapps/meteor/tests/end-to-end/api/abac.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:
apps/meteor/app/lib/server/functions/getFullUserData.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.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:
apps/meteor/tests/end-to-end/api/abac.ts
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/abac.ts (3)
apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)packages/core-typings/src/IUser.ts (1)
IUser(187-258)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
🪛 Biome (2.1.2)
apps/meteor/tests/end-to-end/api/abac.ts
[error] 1945-1958: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ 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). (10)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
🔇 Additional comments (3)
apps/meteor/app/lib/server/functions/getFullUserData.ts (1)
25-25: Verify the security implications of exposingabacAttributesin default fields.Adding
abacAttributestodefaultFieldsmeans any authenticated user can view another user's ABAC attributes without requiringview-full-other-user-infopermission. This may be intentional for ABAC functionality, but could expose sensitive organizational data (e.g., department, clearance level) to all users.Consider whether
abacAttributesshould be infullFieldsinstead, or if this exposure is by design for ABAC access decisions.development/ldap/02-data.ldif (1)
1-51: LGTM - Well-structured LDAP test data.The LDIF file provides good coverage for LDAP integration testing:
- Base DN and organizational units properly defined
- Groups with member references for group sync testing
- User entries with ABAC-relevant attributes (
departmentNumber,employeeType)- Edge cases like multi-valued attributes (john.young) and missing email (userwithnoemail)
docker-compose-ci.yml (1)
237-238: Volume mount path is correctly configured with required LDIF files present.Verification confirms the review comment's concern is valid: the path
./development/ldap:/opt/bitnami/openldap/data/correctly mounts the local directory containing the LDIF file (development/ldap/02-data.ldif). The directory structure is consistent, and the LDAP_CUSTOM_LDIF_DIR environment variable (line 244) properly references the mounted volume where the LDIF files are loaded from. No issues found—the configuration is correct.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docker-compose-ci.yml (1)
235-249: Re-evaluate usingbitnamilegacy/openldap:latest(legacy, unpinned) for CI LDAP.This service still relies on the legacy Bitnami OpenLDAP image with the
:latesttag, which is no longer maintained and is also unpinned. That combination is fragile for CI (image removal/retagging) and weak from a security posture perspective.Strongly consider:
- Pinning to a specific legacy tag or digest, and/or
- Migrating to a maintained OpenLDAP image or a project-built image from Bitnami sources.
If you decide to keep the legacy image, it’s worth documenting the risk and who owns migrating it later.
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/abac.ts (1)
8-8: Consider movingsleepto a generic test utility module.Importing
sleepfrom../../data/livechat/utilsintroduces a cross-feature dependency into ABAC tests. Ifsleepis (or becomes) widely used, consider relocating it to a neutral test utilities module (e.g., undertests/data/utils) and importing from there instead.
📜 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 (4)
apps/meteor/app/lib/server/functions/getFullUserData.ts(1 hunks)apps/meteor/tests/end-to-end/api/abac.ts(2 hunks)development/ldap/02-data.ldif(1 hunks)docker-compose-ci.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/lib/server/functions/getFullUserData.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:
apps/meteor/tests/end-to-end/api/abac.ts
🧠 Learnings (1)
📚 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.ts
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/abac.ts (3)
apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)packages/core-typings/src/IUser.ts (1)
IUser(187-258)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
🪛 Biome (2.1.2)
apps/meteor/tests/end-to-end/api/abac.ts
[error] 2062-2075: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ 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 (1)
development/ldap/02-data.ldif (1)
1-204: LDIF dataset looks consistent with LDAP + ABAC test expectations.The directory layout and attributes (especially
departmentNumberandtelephoneNumber) line up with the LDAP/ABAC tests, and the LDIF syntax is valid for seeding OpenLDAP in CI. No issues from a test-data or structure perspective.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/tests/end-to-end/api/abac.ts (1)
2037-2075: Merge duplicatebeforehooks and assert setup requests to satisfy lint and improve reliability.Within
describe('LDAP integration', ...)you have twobeforehooks in the same scope plus setup calls that don't assert responses:
- Biome flags this as a duplicate-hook error (
lint/suspicious/noDuplicateTestHooks).await request.post(...ldap.syncNow)and the/abac/users/synccall don't use.expect(200), so they can silently fail and only surface later as confusing test failures.Based on static analysis hints, this duplicate hook pattern must be resolved and response assertions added to prevent silent setup failures.
📜 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 (1)
apps/meteor/tests/end-to-end/api/abac.ts(2 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:
apps/meteor/tests/end-to-end/api/abac.ts
🧠 Learnings (7)
📓 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:
apps/meteor/tests/end-to-end/api/abac.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.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.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:
apps/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 : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/abac.ts (3)
apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)packages/core-typings/src/IUser.ts (1)
IUser(187-258)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
🪛 Biome (2.1.2)
apps/meteor/tests/end-to-end/api/abac.ts
[error] 2062-2075: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ 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 (3)
apps/meteor/tests/end-to-end/api/abac.ts (3)
8-8: LGTM: sleep utility imported for LDAP sync coordination.The import supports the LDAP background sync tests that require waiting for async operations to complete.
2077-2168: Comprehensive LDAP sync test coverage.The test suite thoroughly validates:
- LDAP user sync with mapped ABAC attributes (john.young)
- Selective user sync by emails (alan.bean, buzz.aldrin)
- Selective user sync by usernames (david.scott, gene.cernan)
Test logic correctly verifies attributes before and after sync, ensuring LDAP attribute mapping works as expected.
2223-2263: LDAP test data confirms stable department values for both users—test assumptions are valid.The LDIF data (development/ldap/02-data.ldif) contains:
- david.scott: departmentNumber: lifeSupport
- sergei.krikalev: departmentNumber: lifeSupport2
Both values differ from 'navControl', confirming the test's assumption that LDAP sync will replace the manually-set 'navControl' attribute with actual LDAP values, causing the users to be removed from the room requiring 'navControl' department. The LDIF data is stable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/meteor/tests/end-to-end/api/abac.ts (2)
2062-2075: Duplicatebeforehooks should be merged.This has already been flagged in a previous review. The two
beforehooks in the samedescribeblock trigger thelint/suspicious/noDuplicateTestHookserror. Merge them into a single hook and add.expect(200)assertions to the setup requests.
2170-2264: Missing cleanup and assertion for test room.The room cleanup issue was already flagged in a previous review. Additionally, the
groups.invitecall at lines 2215-2220 lacks an.expect(200)assertion, which could cause silent setup failures.Add the missing assertion:
await request .post(`${v1}/groups.invite`) .set(credentials) .send({ roomId: roomIdWithAbac, usernames: ['david.scott', 'sergei.krikalev'], - }); + }) + .expect(200);
🧹 Nitpick comments (1)
development/ldap/02-data.ldif (1)
136-143: Remove excessive blank lines between entries.There are 7 consecutive blank lines between the
james.irwinandneil.armstrongentries. A single blank line is sufficient to separate LDIF entries and would maintain consistency with the rest of the file.
📜 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 (4)
apps/meteor/app/lib/server/functions/getFullUserData.ts(1 hunks)apps/meteor/tests/end-to-end/api/abac.ts(2 hunks)development/ldap/02-data.ldif(1 hunks)docker-compose-ci.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/app/lib/server/functions/getFullUserData.ts
- docker-compose-ci.yml
🧰 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:
apps/meteor/tests/end-to-end/api/abac.ts
🧠 Learnings (9)
📓 Common learnings
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.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:
apps/meteor/tests/end-to-end/api/abac.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.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:
apps/meteor/tests/end-to-end/api/abac.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.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:
apps/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 : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/abac.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:
apps/meteor/tests/end-to-end/api/abac.ts
🪛 Biome (2.1.2)
apps/meteor/tests/end-to-end/api/abac.ts
[error] 2062-2075: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ 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 (4)
development/ldap/02-data.ldif (1)
1-204: LDIF test data structure looks good.The file provides a comprehensive set of LDAP test entries with:
- Proper object class hierarchies
- Multi-valued attributes for testing (e.g.,
john.youngwith multipledepartmentNumberandtelephoneNumbervalues)- Edge case coverage (
userwithnoemailentry)- Correct group membership references
apps/meteor/tests/end-to-end/api/abac.ts (3)
2077-2168: LDAP ABAC sync tests are well-structured.Good coverage of:
- Mapped attribute verification for LDAP users
- Selective user sync via
/abac/users/syncwithemailsandusernamesparams- Proper assertions on ABAC attribute presence and values
2266-2281: After hook correctly resets LDAP settings.The settings cleanup is comprehensive. The missing room cleanup was flagged in a previous review.
8-8: Import addition is appropriate.The
sleeputility is correctly imported for waiting on background LDAP sync operations in the test setup.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.