chore: Run ABAC LDAP sync on license/setting changes #37562
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 implement a lifecycle-managed ABAC feature activation system, replacing the previous Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant License
participant Config as ABAC Config
participant Settings as Settings Registry
participant Watcher
participant LDAPEE
participant Manager as LDAP Manager
License->>Config: onToggledFeature (up)
Config->>Settings: initialize & loadSettings
Config->>Config: loadABACHooks()
Config->>Watcher: watch ABAC_Enabled
Note over License,Manager: ABAC becomes enabled
Watcher->>LDAPEE: syncAbacAttributes(users)
LDAPEE->>Manager: perform sync
Manager->>Manager: log "Starting ABAC attributes sync..."
Manager->>Manager: iterate & update users
Note over License,Manager: ABAC becomes disabled
License->>Config: onToggledFeature (down)
Config->>Watcher: stopWatcher()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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/20 21:27 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [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]
line "ddp-streamer-service" [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.14]
line "presence-service" [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.14]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.36]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 5 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 20 Nov 2025 21:27:24 GMT |
![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
📜 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)
apps/meteor/ee/server/configuration/abac.ts(1 hunks)apps/meteor/ee/server/lib/ldap/Manager.ts(1 hunks)apps/meteor/ee/server/settings/abac.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/abac.tsapps/meteor/ee/server/settings/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/ee/server/configuration/abac.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/abac.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
apps/meteor/ee/server/configuration/abac.ts
🧬 Code graph analysis (1)
apps/meteor/ee/server/configuration/abac.ts (3)
packages/core-services/src/index.ts (1)
License(164-164)apps/meteor/ee/server/settings/abac.ts (1)
addSettings(3-35)apps/meteor/ee/server/lib/abac/index.ts (1)
createPermissions(3-9)
⏰ 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). (18)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (5/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (4/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (2/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (4/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (2/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (4/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (3/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test API (CE) / MongoDB 8.2 (1/1)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (2/4)
- GitHub Check: 🔨 Test API (EE) / MongoDB 5.0 (1/1)
- GitHub Check: 🔨 Test Federation Matrix
- GitHub Check: 🔨 Test API (EE) / MongoDB 8.2 coverage (1/1)
🔇 Additional comments (4)
apps/meteor/ee/server/lib/ldap/Manager.ts (1)
141-141: LGTM: Helpful observability addition.The debug log clearly indicates when ABAC attribute synchronization begins for LDAP users, improving troubleshooting and monitoring capabilities.
apps/meteor/ee/server/configuration/abac.ts (2)
7-29: Good architectural improvement with lifecycle management.The migration from
License.onLicensetoLicense.onToggledFeaturewith explicit up/down handlers provides better lifecycle control. The cleanup viastopWatcher?.()ensures the setting watcher is properly removed when the feature is disabled.
19-23: Async callback behavior is acceptable; no deduplication mechanism needed based on code inspection.Analysis of
settings.watch()reveals the callback is not awaited when invoked (line 220 of CachedSettings.ts), butLDAPEE.syncUsersAbacAttributes()has robust internal error handling with try-catch logging. The callback will fire immediately ifABAC_Enabledis already true when the watcher is registered, which is expected behavior from this watch implementation.No internal deduplication mechanism exists in
syncUsersAbacAttributes, but the sync operation is safe to invoke multiple times—it simply updates user attributes. Concurrent invocations are unlikely given the synchronous watch registration timing within the feature lifecycle.apps/meteor/ee/server/settings/abac.ts (1)
3-4: LGTM: Proper async signature for lifecycle management.Changing the signature to return
Promise<void>and explicitly returning the result ofsettingsRegistry.addGroup()allows callers to properly await initialization. This aligns well with the new lifecycle approach inapps/meteor/ee/server/configuration/abac.ts(line 14).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37562 +/- ##
============================================
Coverage ? 54.25%
============================================
Files ? 2659
Lines ? 49968
Branches ? 11125
============================================
Hits ? 27109
Misses ? 20727
Partials ? 2132
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/ABAC-36
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.