fix(federation): duplicated events of room power_levels, name and topic #37174
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 an issue with duplicated federation events for room power levels, name, and topic changes by preventing federated users from making changes that would cause event loops between Matrix homeservers.
- Adds early return checks to prevent federated users from updating room names, topics, and user roles
- Updates room name handling to use native federation type checking and append federation identifiers
- Reorders code logic to check user permissions before processing federation actions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ee/packages/federation-matrix/src/FederationMatrix.ts | Adds federated user checks to prevent duplicate events in room operations |
| apps/meteor/app/channel-settings/server/functions/saveRoomName.ts | Updates federation type checking and room name formatting for federated rooms |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdjusts room-renaming logic for native federated rooms to append MRID to display names and to route updates via isRoomNativeFederated checks. In FederationMatrix, adds early-return guards preventing federated (non-local) users from renaming rooms, changing topics, or modifying room-scoped roles; standardizes local-style sender identifiers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client
participant Server as RC Server
participant Save as saveRoomName.ts
participant Fed as FederationMatrix
User->>Client: Rename/Topic/Role change action
Client->>Server: Request (roomId, userId, payload)
Server->>Save: saveRoomName(...)
Note over Save: Determine isDiscussion / isRoomNativeFederated
Save->>Save: Build displayName (+MRID if native federated)
Save->>Save: Choose slug (discussion or native federated)
alt Native federated
Save->>Fed: updateFName / federated pathway
else Non-federated
Save->>Fed: updateRoomName / non-federated pathway
end
Note over Fed: New guards for non-local senders
Fed->>Fed: If sender is federated → log + return
Fed->>Fed: Else compute local-style MUI and proceed
Fed-->>Server: Result (ok or no-op)
Server-->>Client: Response
sequenceDiagram
autonumber
participant Client
participant Server
participant Fed as FederationMatrix
rect rgba(230,245,255,0.6)
note right of Fed: Changed control flow
Client->>Server: updateRoomTopic / addUserRoleRoomScoped
Server->>Fed: Call method
alt Sender is federated (non-local)
Fed->>Fed: Log debug
Fed-->>Server: Early return (no change)
else Sender is local
Fed->>Fed: Derive local-style MUI
Fed->>Fed: Apply topic/role update
Fed-->>Server: Success
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 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
📜 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 (2)
apps/meteor/app/channel-settings/server/functions/saveRoomName.ts(3 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
packages/core-typings/src/IUser.ts (1)
isUserNativeFederated(276-277)
apps/meteor/app/channel-settings/server/functions/saveRoomName.ts (2)
packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(124-125)apps/meteor/server/services/room/service.ts (1)
getValidRoomName(84-86)
⏰ 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). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
apps/meteor/app/channel-settings/server/functions/saveRoomName.ts (2)
3-3: LGTM! Import change aligns with native federation handling.The switch from
isRoomFederatedtoisRoomNativeFederatedcorrectly narrows the check to rooms with the federation metadata object, which aligns with the changes that append MRID information.
80-80: Ignore undefineddisplayNameconcern: For federated rooms,displayNameis reassigned to a string before the trim check; otherwise, the!displayName?.trim()guard returns early—so at theslugifiedRoomNameassignment it’s always a defined, non-empty string.Likely an incorrect or invalid review comment.
ee/packages/federation-matrix/src/FederationMatrix.ts (4)
841-846: LGTM! Guard prevents federated users from renaming rooms.This guard correctly prevents non-local (federated) users from triggering room rename events, which addresses the duplicate federation events issue described in the PR objectives. The local-only
userMuiconstruction on line 846 is now safe because the guard ensures only local users reach that line.
862-867: LGTM! Guard prevents federated users from changing room topics.Similar to the room rename guard, this correctly prevents federated users from triggering topic change events, reducing duplicate federation synchronization issues. The
userMuiis correctly constructed for local users only.
887-897: LGTM! Sender validation prevents federated users from changing roles.The guard at lines 892-895 correctly blocks federated users from modifying room-scoped roles, preventing duplicate power-level events. The
senderMuiconstruction at line 897 is safe because it only executes for local senders.
903-903: Clarification: The past review comment misidentifies the user in this line.The past review comment suggests simplifying line 903 to always use local format, but this line constructs the MUI for the target user (the user receiving the role change), not the sender. The target user can still be federated (e.g., granting owner rights to a federated user as described in the PR objectives), so the conditional assignment is correct and necessary.
The sender (line 897:
senderMui) is already guaranteed to be local by the guard at lines 892-895.
| if (isRoomNativeFederated(room)) { | ||
| displayName = `${displayName}:${room.federation.mrid.split(':').pop()}`; | ||
| } |
There was a problem hiding this comment.
Handle undefined displayName before string concatenation.
The displayName parameter is typed as string | undefined, but the MRID-appending logic assumes it's defined. Additionally, room.federation.mrid.split(':').pop() can return undefined if the split produces an empty array, resulting in a string with "undefined" concatenated.
Apply this diff to safely handle undefined values:
await Room.beforeNameChange(room);
if (isRoomNativeFederated(room)) {
- displayName = `${displayName}:${room.federation.mrid.split(':').pop()}`;
+ const serverPart = room.federation.mrid.split(':').pop();
+ if (displayName && serverPart) {
+ displayName = `${displayName}:${serverPart}`;
+ }
}📝 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.
| if (isRoomNativeFederated(room)) { | |
| displayName = `${displayName}:${room.federation.mrid.split(':').pop()}`; | |
| } | |
| await Room.beforeNameChange(room); | |
| if (isRoomNativeFederated(room)) { | |
| const serverPart = room.federation.mrid.split(':').pop(); | |
| if (displayName && serverPart) { | |
| displayName = `${displayName}:${serverPart}`; | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/meteor/app/channel-settings/server/functions/saveRoomName.ts around
lines 66 to 68, the code assumes displayName and the MRID split result are
defined; instead guard against undefined by (1) checking displayName is a
non-empty string before concatenation (return or set a safe fallback like an
empty string if undefined), (2) safely derive the federation part by splitting
room.federation.mrid and selecting the last element only if it exists and is
non-empty (otherwise skip appending), and (3) perform the concatenation only
when both displayName and the federation part are valid to avoid producing
"undefined" in the final string.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37174 +/- ##
===============================================
Coverage 67.41% 67.42%
===============================================
Files 3329 3329
Lines 113381 113381
Branches 20570 20575 +5
===============================================
+ Hits 76441 76452 +11
+ Misses 34341 34329 -12
- Partials 2599 2600 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
https://rocketchat.atlassian.net/browse/FDR-215
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit