feat: Force LDAP sync for specific users via endpoint #37542
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 |
WalkthroughThis PR implements a new API endpoint Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant Endpoint as POST abac/users/sync
participant Validator as Request Validator
participant Manager as LDAPEEManager
participant UserModel as Users Model
participant LDAP as LDAP Connection
participant Abac as ABAC Module
Admin->>Endpoint: POST with identifiers
Endpoint->>Validator: Validate license & permission
Validator-->>Endpoint: ✓ Valid
Endpoint->>Validator: Validate payload schema
Validator-->>Endpoint: ✓ Schema valid
Endpoint->>UserModel: findUsersByIdentifiers(params)
UserModel-->>Endpoint: FindCursor<IUser>
Endpoint->>Manager: syncUsersAbacAttributes(cursor)
loop For each user
Manager->>LDAP: Resolve LDAP user attributes
LDAP-->>Manager: User attributes
Manager->>Abac: addSubjectAttributes(user, attributes)
Abac-->>Manager: ✓ Synced
end
Manager-->>Endpoint: ✓ Sync complete
Endpoint-->>Admin: 200 Success response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 #37542 +/- ##
============================================
Coverage ? 54.50%
============================================
Files ? 2659
Lines ? 49968
Branches ? 11125
============================================
Hits ? 27236
Misses ? 20591
Partials ? 2141
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/ldap-sync-force
branch
from
c06191e to
72caa83
Compare
📦 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 03:03 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.36]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 4 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 19 Nov 2025 03:03:55 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
🧹 Nitpick comments (2)
apps/meteor/ee/server/lib/ldap/Manager.ts (1)
131-150: Consider adding sync result feedback and improving error handling.The current implementation catches and logs errors but provides no feedback to the caller about which users succeeded or failed. For a user-triggered sync operation via the API, this makes troubleshooting difficult.
Consider collecting and returning sync results:
public static async syncUsersAbacAttributes(users: FindCursor<IUser>): Promise<{ succeeded: string[]; failed: Array<{ userId: string; error: string }> }> { if (!settings.get('LDAP_Enable') || !License.hasModule('abac') || !settings.get('ABAC_Enabled')) { return { succeeded: [], failed: [] }; } const succeeded: string[] = []; const failed: Array<{ userId: string; error: string }> = []; try { const ldap = new LDAPConnection(); await ldap.connect(); try { for await (const user of users) { try { await this.syncUserAbacAttribute(ldap, user); succeeded.push(user._id); } catch (error) { failed.push({ userId: user._id, error: String(error) }); logger.error({ msg: 'Failed to sync ABAC attributes for user', userId: user._id, error }); } } } finally { ldap.disconnect(); } } catch (error) { logger.error(error); throw error; } return { succeeded, failed }; }This would require updating the interface signature and the API endpoint to return meaningful feedback.
apps/meteor/ee/server/api/abac/index.ts (1)
182-207: Consider providing sync operation feedback.The endpoint returns a simple success response regardless of whether any users were found or if the sync operation succeeded or failed. For an admin-triggered operation, providing feedback about the operation result would improve usability and troubleshooting.
Consider returning operation statistics:
.post( 'abac/users/sync', { authRequired: true, permissionsRequired: ['abac-management'], license: ['abac', 'ldap-enterprise'], body: POSTAbacUsersSyncBodySchema, response: { - 200: GenericSuccessSchema, + 200: ajv.compile<{ success: true; usersProcessed: number }>({ + type: 'object', + properties: { + success: { type: 'boolean', enum: [true] }, + usersProcessed: { type: 'number' } + }, + required: ['success', 'usersProcessed'] + }), }, }, async function action() { if (!settings.get('ABAC_Enabled')) { throw new Error('error-abac-not-enabled'); } const { usernames, ids, emails, ldapIds } = this.bodyParams; - await LDAPEE.syncUsersAbacAttributes(Users.findUsersByIdentifiers({ usernames, ids, emails, ldapIds })); + const usersCursor = Users.findUsersByIdentifiers({ usernames, ids, emails, ldapIds }); + const usersProcessed = await usersCursor.count(); + + if (usersProcessed === 0) { + throw new Error('error-no-users-found'); + } + + await LDAPEE.syncUsersAbacAttributes(Users.findUsersByIdentifiers({ usernames, ids, emails, ldapIds })); - return API.v1.success(); + return API.v1.success({ usersProcessed }); }, )Note: This assumes the LDAPEEService signature remains unchanged. For better feedback, consider also updating the service layer to return sync results as suggested in the Manager.ts review.
📜 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 (7)
apps/meteor/ee/server/api/abac/index.ts(4 hunks)apps/meteor/ee/server/api/abac/schemas.ts(1 hunks)apps/meteor/ee/server/lib/ldap/Manager.ts(3 hunks)apps/meteor/ee/server/local-services/ldap/service.ts(2 hunks)apps/meteor/ee/server/sdk/types/ILDAPEEService.ts(1 hunks)packages/model-typings/src/models/IUsersModel.ts(1 hunks)packages/models/src/models/Users.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/local-services/ldap/service.tsapps/meteor/ee/server/lib/ldap/Manager.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/api/abac/index.ts
🧬 Code graph analysis (6)
apps/meteor/ee/server/sdk/types/ILDAPEEService.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)
packages/model-typings/src/models/IUsersModel.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)
apps/meteor/ee/server/local-services/ldap/service.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)
apps/meteor/ee/server/lib/ldap/Manager.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)packages/core-services/src/index.ts (2)
License(164-164)Abac(202-202)
packages/models/src/models/Users.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(187-258)
apps/meteor/ee/server/api/abac/index.ts (2)
apps/meteor/ee/server/api/abac/schemas.ts (3)
POSTAbacUsersSyncBodySchema(252-257)GenericSuccessSchema(17-17)GenericErrorSchema(259-259)packages/rest-typings/src/v1/Ajv.ts (1)
validateUnauthorizedErrorResponse(70-70)
🔇 Additional comments (5)
packages/model-typings/src/models/IUsersModel.ts (1)
94-97: LGTM!The interface definition correctly matches the implementation and is well-positioned in the file.
apps/meteor/ee/server/api/abac/schemas.ts (1)
220-257: LGTM!The schema validation is well-structured and correctly enforces the requirement that at least one identifier array must be provided (via
anyOf), while ensuring arrays are non-empty (minItems: 1) and contain valid identifiers (minLength: 1,uniqueItems: true).apps/meteor/ee/server/sdk/types/ILDAPEEService.ts (1)
1-10: LGTM!The interface extension correctly adds the new method signature with proper imports. The method signature is consistent with the implementation in the service and manager layers.
apps/meteor/ee/server/lib/ldap/Manager.ts (1)
758-771: LGTM!The single-user sync implementation correctly reuses the existing
findLDAPUserhelper and delegates toAbac.addSubjectAttributes. The error handling and early returns are appropriate for this context.apps/meteor/ee/server/local-services/ldap/service.ts (1)
27-29: LGTM!The service method correctly delegates to the manager implementation and follows the established pattern used by other sync methods in this class.
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/ABAC-12
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Release Notes