fix(ldap): prevent excessive logging when Group BaseDN/Filter not configured #38246
Conversation
…figured When LDAP role/channel sync is enabled with "Validate membership for each group" strategy but Group BaseDN or Group Filter are not configured, the system was logging an error for every user and every LDAP group on every sync interval. This could result in thousands of log entries per second (~1GB of logs per day). This fix: 1. Adds early validation in syncUserRoles and syncUserChannels to check if BaseDN and Filter are configured before entering the iteration loop 2. Adds log throttling to isUserInGroup as a defense in depth measure Closes RocketChat#36705 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
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 |
🦋 Changeset detectedLatest commit: 2018cdf The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughFixes excessive LDAP logging occurring when Group BaseDN or Group Filter are not configured. Implements log throttling in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/ee/server/lib/ldap/Manager.ts">
<violation number="1" location="apps/meteor/ee/server/lib/ldap/Manager.ts:311">
P2: Missing group-config warning logs once per user sync, still causing log spam when BaseDN/Filter are unset</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| if (searchStrategy === 'each_group' && (!syncUserRolesBaseDN || !syncUserRolesFilter)) { | ||
| logger.warn( |
There was a problem hiding this comment.
P2: Missing group-config warning logs once per user sync, still causing log spam when BaseDN/Filter are unset
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/ee/server/lib/ldap/Manager.ts, line 311:
<comment>Missing group-config warning logs once per user sync, still causing log spam when BaseDN/Filter are unset</comment>
<file context>
@@ -300,6 +307,13 @@ export class LDAPEEManager extends LDAPManager {
}
+ if (searchStrategy === 'each_group' && (!syncUserRolesBaseDN || !syncUserRolesFilter)) {
+ logger.warn(
+ 'LDAP Sync User Roles: "Group BaseDN" and "Group Filter" are required when using "Validate membership for each group" strategy. Skipping role sync.',
+ );
</file context>
![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
🤖 Fix all issues with AI agents
In @.changeset/quiet-llamas-dance.md:
- Around line 12-15: The phrase "defense in depth" in the changelog should be
hyphenated as "defense-in-depth"; update the sentence that describes log
throttling for the isUserInGroup function to read "Adds log throttling to the
`isUserInGroup` function as a defense-in-depth measure" and ensure the functions
`syncUserRoles` and `syncUserChannels` and `isUserInGroup` are referenced
exactly as shown.
🧹 Nitpick comments (2)
apps/meteor/ee/server/lib/ldap/Manager.ts (2)
26-29: Drop the inline comment; the field name is self-explanatory.As per coding guidelines, avoid code comments in implementation.
♻️ Proposed change
- // Track if we've already logged the missing config error to prevent log spam private static hasLoggedMissingGroupConfig = false;
310-315: Consider throttling this warning as well.If background sync runs frequently, this (and the similar channel warning) will still log every sync; consider reusing the missing-config throttle or a time-based throttle to keep warnings to a reasonable rate.
| This fix: | ||
| 1. Adds early validation in `syncUserRoles` and `syncUserChannels` to check if BaseDN and Filter are | ||
| configured before entering the iteration loop | ||
| 2. Adds log throttling to the `isUserInGroup` function as a defense in depth measure |
There was a problem hiding this comment.
Hyphenate “defense-in-depth”.
Minor grammar: compound modifier should be hyphenated.
✏️ Proposed edit
-2. Adds log throttling to the `isUserInGroup` function as a defense in depth measure
+2. Adds log throttling to the `isUserInGroup` function as a defense-in-depth measure📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| This fix: | |
| 1. Adds early validation in `syncUserRoles` and `syncUserChannels` to check if BaseDN and Filter are | |
| configured before entering the iteration loop | |
| 2. Adds log throttling to the `isUserInGroup` function as a defense in depth measure | |
| This fix: | |
| 1. Adds early validation in `syncUserRoles` and `syncUserChannels` to check if BaseDN and Filter are | |
| configured before entering the iteration loop | |
| 2. Adds log throttling to the `isUserInGroup` function as a defense-in-depth measure |
🧰 Tools
🪛 LanguageTool
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...he isUserInGroup function as a defense in depth measure
(QB_NEW_EN_HYPHEN)
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...isUserInGroup function as a defense in depth measure
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In @.changeset/quiet-llamas-dance.md around lines 12 - 15, The phrase "defense
in depth" in the changelog should be hyphenated as "defense-in-depth"; update
the sentence that describes log throttling for the isUserInGroup function to
read "Adds log throttling to the `isUserInGroup` function as a defense-in-depth
measure" and ensure the functions `syncUserRoles` and `syncUserChannels` and
`isUserInGroup` are referenced exactly as shown.
1 participant
Summary
Fixes excessive LDAP logging issue (#36705) where the system could generate ~1GB of logs per day when:
The fix:
syncUserRolesandsyncUserChannelsto check if BaseDN and Filter are configured before entering the iteration loopisUserInGroupas a defense in depth measureChanges
isUserInGroupfunction now tracks whether it has already logged the missing config error to prevent repeated logging.Test plan
Issue(s)
Closes #36705
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.