chore: Show ldap attribute sync settings on abac page#40088
Merged
dionisio-bot[bot] merged 7 commits intodevelopfrom Apr 13, 2026
Merged
chore: Show ldap attribute sync settings on abac page#40088dionisio-bot[bot] merged 7 commits intodevelopfrom
dionisio-bot[bot] merged 7 commits intodevelopfrom
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details⏰ 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). (8)
WalkthroughAdds a "Sync ABAC Attributes" accordion to ABAC settings, extracts LDAP sync logic into a new Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant UI as Admin UI
participant TestAPI as LDAP Test API
participant Modal as Confirmation Modal
participant SyncAPI as LDAP Sync API
participant Toast as Toast Service
Admin->>UI: Click "LDAP Sync Now"
UI->>TestAPI: POST /v1/ldap.testConnection
alt Connection fails
TestAPI-->>UI: Error
UI->>Toast: Dispatch error toast ("Connection_failed")
Toast-->>Admin: Show error
else Connection succeeds
TestAPI-->>UI: Success
UI->>Modal: Open confirmation modal
Admin->>Modal: Confirm
Modal-->>UI: Confirmation
UI->>SyncAPI: POST /v1/ldap.syncNow
alt Sync succeeds
SyncAPI-->>UI: { message }
UI->>Toast: Dispatch success toast (t(message))
Toast-->>Admin: Show success
else Sync fails
SyncAPI-->>UI: Error
UI->>Toast: Dispatch error toast (error.message)
Toast-->>Admin: Show error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40088 +/- ##
===========================================
- Coverage 70.59% 70.15% -0.44%
===========================================
Files 3273 3274 +1
Lines 116880 116249 -631
Branches 21110 20759 -351
===========================================
- Hits 82512 81557 -955
+ Misses 32309 31393 -916
- Partials 2059 3299 +1240
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
coderabbitai
bot
added
type: feature
KevLehman
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx">
<violation number="1" location="apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx:38">
P2: This introduces a second copy of the LDAP “Sync Now” flow that already exists in `LDAPGroupPage`; extract/reuse a shared action to avoid behavior drift between the two admin screens.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx`:
- Around line 35-43: The handler currently calls testConnection() (POST
/v1/ldap.testConnection) before performing a sync, which blocks users who have
sync permission but not test permission and conflates auth failures with
connection errors; update handleSyncNowButtonClick (and the button rendering
that only checks LDAP_Enable) to either gate the UI by the sync permission
(e.g., the permission that maps to sync-auth-services-users) instead of just
LDAP_Enable, or call syncNow() (POST /v1/ldap.syncNow) directly and distinguish
HTTP 403/authorization errors from connection failures in the catch: show a
permission-specific toast for auth errors and keep Connection_failed for real
connection errors; reference testConnection, syncNow, handleSyncNowButtonClick,
and dispatchToastMessage when making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 685a4866-451a-4075-8557-af1f8528fea3
📒 Files selected for processing (2)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
📜 Review details
🧰 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/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
🧠 Learnings (3)
📚 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/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsx
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
🔇 Additional comments (2)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsx (1)
20-28: Nice reuse of the existing settings plumbing.Rendering the LDAP ABAC fields through
SettingFieldkeeps the server-defined module andenableQuerybehavior intact instead of duplicating that logic in the page.apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1)
49-52: Nice reuse of the endpoint message key.Translating the
messagereturned byldap.syncNowkeeps the success toast aligned with the server's existing i18n keys instead of hard-coding another client-only string.
aleksandernsilva
requested changes
Apr 8, 2026
coderabbitai
bot
removed
type: feature
coderabbitai
bot
added
type: feature
coderabbitai
bot
removed
type: feature
coderabbitai
bot
added
the
type: feature
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/hooks/useLdapSync.tsx">
<violation number="1" location="apps/meteor/client/hooks/useLdapSync.tsx:20">
P2: Avoid force-casting API response values to `TranslationKey` without narrowing first; guard `message` before translating so unexpected responses do not result in invalid toast text.
(Based on your team's feedback about avoiding unsafe type casts when values may be missing or unexpected.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
aleksandernsilva
approved these changes
Apr 13, 2026
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
dionisio-bot
bot
added
stat: ready to merge
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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)
https://rocketchat.atlassian.net/browse/PRD-244
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Improvements