feat: ABAC Attributes LDAP integration #37379
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
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 |
|
Contributor
WalkthroughAdds LDAP → ABAC attribute sync: new enterprise settings and validation, a background cron and watcher wiring, manager and service APIs to sync/validate attribute maps, AbacService methods/helpers to apply mapped attributes to users, and unit tests for attribute-loss detection. Changes
Sequence Diagram(s)sequenceDiagram
participant Cron as Cron (LDAP_AbacSync)
participant Manager as LDAPEEManager
participant LDAP as LDAP Connection
participant Abac as AbacService
participant DB as User Collection
Cron->>Manager: syncAbacAttributes()
activate Manager
Manager->>Manager: check license & settings
Manager->>LDAP: connect()
activate LDAP
Manager->>Manager: parse LDAP_ABAC_AttributeMap
Manager->>LDAP: fetch users
LDAP-->>Manager: user list
deactivate LDAP
loop per user
Manager->>LDAP: resolve user entry
activate LDAP
LDAP-->>Manager: entry data
deactivate LDAP
Manager->>Abac: addSubjectAttributes(user, ldapEntry, map)
activate Abac
Abac->>DB: persist abacAttributes[]
alt attributes removed
Abac->>Abac: onSubjectAttributesChanged(user, next)
end
Abac-->>Manager: done
deactivate Abac
end
Manager->>LDAP: disconnect()
Manager-->>Cron: complete
deactivate Manager
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37379 +/- ##
=============================================
- Coverage 54.30% 54.29% -0.01%
=============================================
Files 2658 2658
Lines 49963 49964 +1
Branches 11126 11126
=============================================
- Hits 27132 27130 -2
- Misses 20697 20703 +6
+ Partials 2134 2131 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
KevLehman
force-pushed
the
feat/abac-ldap
branch
from
e546214 to
afa5d31
Compare
KevLehman
force-pushed
the
feat/abac-ldap
branch
from
afa5d31 to
ec3b9b8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ee/packages/abac/src/index.ts (1)
615-617: Clarify the hook's purpose and when it's invoked.The comment states this hook is for "when a user loses an ABAC attribute or value," but examining line 68 shows it's called via
didSubjectLoseAttributes, which detects any attribute or value loss. Consider adding more documentation about:
- What implementations should do (e.g., trigger subscription re-evaluation)
- Whether it's called only on loss or also on other changes
- Expected error handling behavior
apps/meteor/ee/server/lib/ldap/Manager.ts (1)
719-735: Consider performance optimizations for large user bases.The method loads all LDAP users into memory (line 726) and processes them sequentially. For deployments with thousands of LDAP users, this could cause:
- High memory usage from loading all users at once
- Slow sync times from sequential processing
- No visibility into sync progress
Consider these improvements:
private static async updateUserAbacAttributes(ldap: LDAPConnection): Promise<void> { const mapping = this.parseJson(settings.get('LDAP_ABAC_AttributeMap')); if (!mapping) { logger.error('LDAP to ABAC attribute mapping is not valid JSON'); return; } - const users = await Users.findLDAPUsers().toArray(); - for await (const user of users) { + let processed = 0; + let updated = 0; + const cursor = Users.findLDAPUsers(); + for await (const user of cursor) { const ldapUser = await this.findLDAPUser(ldap, user); if (!ldapUser) { + processed++; continue; } await Abac.addSubjectAttributes(user, ldapUser, mapping); + processed++; + updated++; + if (processed % 100 === 0) { + logger.info({ msg: 'ABAC attribute sync progress', processed, updated }); + } } + logger.info({ msg: 'ABAC attribute sync completed', processed, updated }); }This uses cursor iteration to avoid loading all users into memory and adds progress logging.
📜 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 (10)
apps/meteor/ee/server/configuration/ldap.ts(2 hunks)apps/meteor/ee/server/lib/ldap/Manager.ts(4 hunks)apps/meteor/ee/server/local-services/ldap/service.ts(1 hunks)apps/meteor/ee/server/sdk/types/ILDAPEEService.ts(1 hunks)apps/meteor/ee/server/settings/ldap.ts(1 hunks)ee/packages/abac/src/index.spec.ts(2 hunks)ee/packages/abac/src/index.ts(3 hunks)packages/core-services/src/types/IAbacService.ts(2 hunks)packages/core-typings/src/IUser.ts(2 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 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`.
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-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.tspackages/core-services/src/types/IAbacService.tsee/packages/abac/src/index.spec.tsapps/meteor/ee/server/settings/ldap.tspackages/i18n/src/locales/en.i18n.jsonapps/meteor/ee/server/lib/ldap/Manager.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.tspackages/core-services/src/types/IAbacService.tsee/packages/abac/src/index.spec.tsapps/meteor/ee/server/settings/ldap.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.tsapps/meteor/ee/server/configuration/ldap.tsapps/meteor/ee/server/settings/ldap.tsapps/meteor/ee/server/lib/ldap/Manager.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
ee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
ee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
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/index.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
ee/packages/abac/src/index.spec.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/ee/server/configuration/ldap.ts
🧬 Code graph analysis (5)
ee/packages/abac/src/index.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
packages/core-services/src/types/IAbacService.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)
ee/packages/abac/src/index.spec.ts (1)
packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
packages/core-typings/src/IUser.ts (1)
packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
apps/meteor/ee/server/lib/ldap/Manager.ts (1)
packages/core-services/src/index.ts (2)
License(164-164)Abac(202-202)
🔇 Additional comments (11)
apps/meteor/ee/server/sdk/types/ILDAPEEService.ts (1)
5-5: LGTM!The new method signature is consistent with the existing sync methods in the interface.
packages/core-typings/src/IUser.ts (1)
1-1: LGTM!The optional
abacAttributesfield properly extends the IUser interface with backward compatibility. The import is correctly typed.Also applies to: 257-257
apps/meteor/ee/server/local-services/ldap/service.ts (1)
21-23: LGTM!The delegation pattern is consistent with other sync methods in the service class.
apps/meteor/ee/server/settings/ldap.ts (1)
286-308: LGTM!The ABAC sync settings follow the established pattern for other LDAP sync features. License gating via
modules: ['abac', 'ldap-enterprise']is properly applied, and theenableQuerydependencies are correctly configured.apps/meteor/ee/server/configuration/ldap.ts (2)
63-68: LGTM!The ABAC cron job configuration follows the established pattern for other LDAP background sync jobs. The wiring for settings watchers and the LDAP_Enable toggle is consistent with existing sync features.
Also applies to: 73-73, 79-79
90-96: LGTM!The validation watcher for
LDAP_ABAC_AttributeMapfollows the same pattern as the existingLDAP_Groups_To_Rocket_Chat_Teamswatcher, with proper error handling and logging.packages/core-services/src/types/IAbacService.ts (1)
1-1: LGTM!The new
addSubjectAttributesmethod signature properly extends the IAbacService interface. The use ofRecord<string, string>for the mapping is clear and type-safe.Also applies to: 21-21
apps/meteor/ee/server/lib/ldap/Manager.ts (1)
106-128: LGTM!The
syncAbacAttributesmethod follows the established pattern of other sync methods with proper guard clauses, license checks, connection lifecycle management, and error handling.packages/i18n/src/locales/en.i18n.json (1)
2779-2784: All suggestions are well-supported by implementation and existing patterns.I've verified the review comment against the codebase:
Replace semantics: Confirmed. The
addSubjectAttributesimplementation inee/packages/abac/src/index.tsuses{ $set: { abacAttributes: finalAttributes } }(line 67), which replaces the entire field. This is not a merge—it's a full replacement. The suggestion to clarify this is accurate and important.Interval description: Valid pattern.
LDAP_Background_Sync_Interval_Descriptionexists at i18n line 2774 for the standard sync interval. The new ABAC interval at line 2779 lacks this description key, creating inconsistency. The suggestion maintains established UX patterns.Terminology alignment: Correct. "Subject attributes" terminology is already established in the codebase—
ABAC_Attributesis labeled as"Subject attributes"at i18n line 713. Aligning the new LDAP ABAC keys follows this convention.The settings file (
apps/meteor/ee/server/settings/ldap.ts) confirms the interval setting exists but lacks a corresponding description key in i18n, validating the recommendation to add it.ee/packages/abac/src/index.spec.ts (2)
1-2: LGTM: Clean type import for new test suite.The import is correctly scoped as a type-only import and matches the interface definition from
@rocket.chat/core-typings.
68-205: Excellent test coverage for critical security logic.This comprehensive test suite thoroughly validates the
didSubjectLoseAttributesmethod, which is essential for triggering subscription re-evaluation when users lose ABAC attributes during LDAP sync. The tests cover:
- Empty and no-op cases: Correctly handle empty inputs and no changes
- Addition-only scenarios: Verify no loss detected when attributes/values are only added
- Removal detection: Properly detect loss of entire attribute keys or individual values
- Edge cases: Pure function behavior, order-insensitive comparison, duplicate handling
- Multi-attribute scenarios: Correct behavior with multiple attributes
The test organization is logical and test names clearly communicate expected behavior. This level of coverage is appropriate given the security implications of this method.
KevLehman
force-pushed
the
feat/abac-ldap
branch
from
f847b14 to
a71f576
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (10)
apps/meteor/ee/server/configuration/ldap.ts(2 hunks)apps/meteor/ee/server/lib/ldap/Manager.ts(4 hunks)apps/meteor/ee/server/local-services/ldap/service.ts(1 hunks)apps/meteor/ee/server/sdk/types/ILDAPEEService.ts(1 hunks)apps/meteor/ee/server/settings/ldap.ts(1 hunks)ee/packages/abac/src/index.spec.ts(2 hunks)ee/packages/abac/src/index.ts(3 hunks)packages/core-services/src/types/IAbacService.ts(2 hunks)packages/core-typings/src/IUser.ts(2 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/meteor/ee/server/local-services/ldap/service.ts
- apps/meteor/ee/server/settings/ldap.ts
- apps/meteor/ee/server/sdk/types/ILDAPEEService.ts
- packages/i18n/src/locales/en.i18n.json
- packages/core-typings/src/IUser.ts
🧰 Additional context used
🧠 Learnings (10)
📓 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`.
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-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.tsapps/meteor/ee/server/lib/ldap/Manager.tsee/packages/abac/src/index.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/index.tsapps/meteor/ee/server/lib/ldap/Manager.tsee/packages/abac/src/index.spec.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:
apps/meteor/ee/server/configuration/ldap.tsapps/meteor/ee/server/lib/ldap/Manager.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/ee/server/configuration/ldap.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
ee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
ee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
ee/packages/abac/src/index.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
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/index.spec.ts
🧬 Code graph analysis (4)
packages/core-services/src/types/IAbacService.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)
ee/packages/abac/src/index.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
apps/meteor/ee/server/lib/ldap/Manager.ts (1)
packages/core-services/src/index.ts (2)
License(164-164)Abac(202-202)
ee/packages/abac/src/index.spec.ts (1)
packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)
⏰ 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
KevLehman
force-pushed
the
feat/abac-ldap
branch
from
a71f576 to
a74d188
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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/ee/server/configuration/ldap.ts(2 hunks)apps/meteor/ee/server/lib/ldap/Manager.ts(4 hunks)apps/meteor/ee/server/local-services/ldap/service.ts(1 hunks)apps/meteor/ee/server/sdk/types/ILDAPEEService.ts(1 hunks)apps/meteor/ee/server/settings/ldap.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/ee/server/settings/ldap.ts
🧰 Additional context used
🧠 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`.
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-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/ee/server/configuration/ldap.tsapps/meteor/ee/server/lib/ldap/Manager.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/ee/server/configuration/ldap.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/ee/server/lib/ldap/Manager.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/ee/server/lib/ldap/Manager.ts
🧬 Code graph analysis (2)
apps/meteor/ee/server/local-services/ldap/service.ts (1)
apps/meteor/ee/server/lib/ldap/Manager.ts (1)
LDAPEEManager(25-768)
apps/meteor/ee/server/lib/ldap/Manager.ts (1)
packages/core-services/src/index.ts (2)
License(164-164)Abac(202-202)
⏰ 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). (1)
- GitHub Check: CodeQL-Build
tassoevan
reviewed
Nov 12, 2025
tassoevan
approved these changes
Nov 12, 2025
KevLehman
added a commit
that referenced
this pull request
Nov 18, 2025
This was referenced Nov 19, 2025
KevLehman
added a commit
that referenced
this pull request
Nov 24, 2025
KevLehman
added a commit
that referenced
this pull request
Nov 27, 2025
This was referenced Nov 28, 2025
Merged
KevLehman
added a commit
that referenced
this pull request
Dec 1, 2025
KevLehman
added a commit
that referenced
this pull request
Dec 2, 2025
Merged
KevLehman
added a commit
that referenced
this pull request
Dec 8, 2025
KevLehman
added a commit
that referenced
this pull request
Dec 10, 2025
Merged
KevLehman
added a commit
that referenced
this pull request
Dec 15, 2025
MartinSchoeler
pushed a commit
that referenced
this pull request
Dec 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Issue(s)
ABAC-37
ABAC-38
ABAC-39
ABAC-40
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Settings
Localization
Tests