regression(federation): LDAP using wrong federation settings and username #37209
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 |
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in LDAP federation integration where the system was using incorrect federation settings references and improper username formatting. The fix ensures proper Matrix federation configuration and correct username formatting with "@" prefixes.
- Updated federation setting references from Matrix-specific to generic Federation Service settings
- Fixed username formatting to include "@" prefix for federated users
- Added missing federation metadata fields (mui and origin)
- Removed unused helper method for federated username generation
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
WalkthroughRenames federation setting keys, updates federated user identifier composition to include @ and home server, augments federation-related user data with mui and origin, and removes the getFederatedUsername method. Logic now uses slugifyUsername and getFederationHomeServer with the new settings keys for federation checks and domain comparison. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client
participant Server as Meteor Server
participant LDAP as LDAPManager
participant Settings
participant Fed as Federation Service
User->>Client: Initiate login/sync
Client->>Server: Request authentication
Server->>LDAP: Sync/authenticate via LDAP
LDAP->>Settings: Read Federation_Service_Enabled / Federation_Service_Domain
alt Federation enabled
LDAP->>LDAP: slugifyUsername(requestUsername)
LDAP->>Fed: getFederationHomeServer()
note over LDAP,Fed: Compose federated ID with "@username:home.server"<br/>(replaces prior getFederatedUsername)
LDAP->>LDAP: Attach mui, origin to user data
else Federation disabled
LDAP->>LDAP: Proceed without federation fields
end
LDAP-->>Server: User data (with/without federated fields)
Server-->>Client: Auth result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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 |
![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 (1)
apps/meteor/server/lib/ldap/Manager.ts (1)
199-207: Federation user data structure looks correct.The implementation properly constructs Matrix-compliant identifiers (@username:homeserver) for federated users. The
mui(Matrix User Identifier) andoriginfields are correctly populated.However, consider adding validation for the
homeServervalue before using it to construct identifiers, as invalid domain formats could lead to malformed MUIDs.For example, you could add a simple domain validation:
...(homeServer && { + // Validate homeServer format before using it + ...(!/^[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/.test(homeServer) && + (() => { throw new Error(`Invalid homeServer format: ${homeServer}`); })()), username: `@${username}:${homeServer}`,
📜 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 (1)
apps/meteor/server/lib/ldap/Manager.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/server/lib/ldap/Manager.ts
⏰ 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
🔇 Additional comments (2)
apps/meteor/server/lib/ldap/Manager.ts (2)
506-506: Consistent setting key update for federation domain.The federation domain comparison now uses
Federation_Service_Domaininstead ofFederation_Matrix_homeserver_domain, consistent with the setting key changes at line 493.
1-561: No remaining references to getFederatedUsername Removal is safe.
|
|
||
| protected static getFederationHomeServer(ldapUser: ILDAPEntry): string | undefined { | ||
| if (!settings.get<boolean>('Federation_Matrix_enabled')) { | ||
| if (!settings.get<boolean>('Federation_Service_Enabled')) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Setting key renamed from old Matrix federation system.
The federation enabled check now uses Federation_Service_Enabled instead of the deprecated Federation_Matrix_enabled setting. This aligns with the migration away from the old federation system.
Based on learnings: The old Matrix federation settings are being deprecated/removed.
Ensure that:
- The new
Federation_Service_Enabledsetting is properly defined in the settings registry - There's documentation or migration guidance for users upgrading from the old federation system
🏁 Script executed:
#!/bin/bash
# Description: Verify the new federation setting is defined and check for any remaining references to the old setting
# Search for Federation_Service_Enabled setting definition
echo "=== Searching for Federation_Service_Enabled setting definition ==="
rg -n "Federation_Service_Enabled" --type ts -g '!**/node_modules/**' -C 2
echo -e "\n=== Checking for remaining references to old Federation_Matrix_enabled ==="
rg -n "Federation_Matrix_enabled" --type ts -g '!**/node_modules/**'Length of output: 5354
🏁 Script executed:
#!/bin/bash
echo "=== Searching for documentation referencing Federation_Service_Enabled ==="
rg -n "Federation_Service_Enabled" -g "*.md"Length of output: 74
Remove deprecated Federation_Matrix_enabled references and add migration guide
- Update apps/meteor/server/services/federation/Settings.ts, apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Statistics.ts, and apps/meteor/client/hooks/useIsFederationEnabled.ts to use only
Federation_Service_Enabled. - Add documentation or upgrade notes for users migrating from the old
Federation_Matrix_enabledsetting.
🤖 Prompt for AI Agents
In apps/meteor/server/lib/ldap/Manager.ts around line 493, the code still checks
the deprecated Federation_Matrix_enabled setting; update references to use only
Federation_Service_Enabled and remove any legacy checks or fallbacks for
Federation_Matrix_enabled. Also update the three other files mentioned
(apps/meteor/server/services/federation/Settings.ts,
apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Statistics.ts,
apps/meteor/client/hooks/useIsFederationEnabled.ts) to read exclusively from
Federation_Service_Enabled, remove any code paths that reference
Federation_Matrix_enabled, and add a short upgrade note or migration guide in
the repository docs/UPGRADE or CHANGELOG describing how to migrate existing
Federation_Matrix_enabled values to Federation_Service_Enabled (e.g., copy or
map old value to new key during upgrade).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37209 +/- ##
==================================================
- Coverage 66.37% 66.36% -0.01%
==================================================
Files 3386 3386
Lines 115619 115618 -1
Branches 21351 21355 +4
==================================================
- Hits 76739 76727 -12
- Misses 36275 36283 +8
- Partials 2605 2608 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
changed the title
https://rocketchat.atlassian.net/browse/FDR-239
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Refactor
Bug Fixes