feat(federation): accept remote join at 3rd party #37072
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 |
|
|
Caution Review failedThe pull request is closed. WalkthroughRemoves the Matrix invite event registration and deletes the invite orchestration file. Refactors membership handling by splitting join and leave logic into dedicated helpers that resolve rooms/users, create federated users on join when needed, and distinguish voluntary leaves from kicks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant HS as Homeserver
participant MemberEvt as member.ts handler
participant JoinAct as membershipJoinAction
participant LeaveAct as membershipLeaveAction
participant Rooms as Rooms Repo
participant Users as Users Repo
HS->>MemberEvt: matrix: state.member (event)
alt membership == 'join'
MemberEvt->>JoinAct: handle join
JoinAct->>Rooms: find by federation.mrid
alt room found
JoinAct->>Users: find by federation.mui (sender)
alt user exists
JoinAct->>Rooms: add user to room
else user missing
JoinAct->>Users: create federated user
JoinAct->>Users: fetch inserted user
JoinAct->>Rooms: add user to room
end
else room missing
Note over JoinAct: warn and return
end
else membership == 'leave'
MemberEvt->>LeaveAct: handle leave
LeaveAct->>Rooms: find by federation.mrid
alt room found
LeaveAct->>Users: find by federation.mui (state_key)
alt sender == state_key
LeaveAct->>Rooms: remove user (voluntary)
else kicked
LeaveAct->>Users: find kicker by federation.mui (sender)
LeaveAct->>Rooms: remove user with byUser context
end
else room missing
Note over LeaveAct: warn and return
end
else other membership
Note over MemberEvt: debug: ignored
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 (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
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: 2
📜 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 (3)
ee/packages/federation-matrix/src/events/index.ts(0 hunks)ee/packages/federation-matrix/src/events/invite.ts(0 hunks)ee/packages/federation-matrix/src/events/member.ts(1 hunks)
💤 Files with no reviewable changes (2)
- ee/packages/federation-matrix/src/events/invite.ts
- ee/packages/federation-matrix/src/events/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/events/member.ts (3)
ee/packages/federation-matrix/src/events/room.ts (1)
room(6-59)packages/models/src/index.ts (2)
Rooms(202-202)Users(211-211)packages/core-services/src/index.ts (1)
Room(169-169)
⏰ 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
| const [, serverName] = data.sender.split(':'); | ||
| if (!serverName) { | ||
| throw new Error('Invalid sender format, missing server name'); | ||
| } |
There was a problem hiding this comment.
Handle Matrix server name with ports correctly
Splitting on : drops everything after the first separator, so IDs like @alice:server.example:8448 lose the port and we persist the wrong origin. That breaks reconciliations and future joins from homeservers that expose non-default ports or IPv6 literals. Please slice everything after the first colon instead of relying on array destructuring.
- const [, serverName] = data.sender.split(':');
- if (!serverName) {
- throw new Error('Invalid sender format, missing server name');
- }
+ const separatorIndex = data.sender.indexOf(':');
+ if (separatorIndex === -1 || separatorIndex === data.sender.length - 1) {
+ throw new Error('Invalid sender format, missing server name');
+ }
+ const serverName = data.sender.slice(separatorIndex + 1);📝 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.
| const [, serverName] = data.sender.split(':'); | |
| if (!serverName) { | |
| throw new Error('Invalid sender format, missing server name'); | |
| } | |
| const separatorIndex = data.sender.indexOf(':'); | |
| if (separatorIndex === -1 || separatorIndex === data.sender.length - 1) { | |
| throw new Error('Invalid sender format, missing server name'); | |
| } | |
| const serverName = data.sender.slice(separatorIndex + 1); |
| if (data.content.membership === 'leave') { | ||
| return membershipLeaveAction(data); | ||
| } | ||
|
|
||
| const room = await Rooms.findOne({ 'federation.mrid': data.room_id }, { projection: { _id: 1 } }); | ||
| if (!room) { | ||
| logger.warn(`No bridged room found for Matrix room_id: ${data.room_id}`); | ||
| return; | ||
| if (data.content.membership === 'join') { | ||
| return membershipJoinAction(data); | ||
| } |
There was a problem hiding this comment.
Await membership handlers so errors surface
Returning the promise without awaiting bypasses this try/catch, so any rejection from the join/leave handlers bubbles out as an unhandled rejection with no log. Await the calls and return afterward so failures are captured and logged.
- if (data.content.membership === 'leave') {
- return membershipLeaveAction(data);
- }
+ if (data.content.membership === 'leave') {
+ await membershipLeaveAction(data);
+ return;
+ }
…
- if (data.content.membership === 'join') {
- return membershipJoinAction(data);
- }
+ if (data.content.membership === 'join') {
+ await membershipJoinAction(data);
+ return;
+ }📝 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 (data.content.membership === 'leave') { | |
| return membershipLeaveAction(data); | |
| } | |
| const room = await Rooms.findOne({ 'federation.mrid': data.room_id }, { projection: { _id: 1 } }); | |
| if (!room) { | |
| logger.warn(`No bridged room found for Matrix room_id: ${data.room_id}`); | |
| return; | |
| if (data.content.membership === 'join') { | |
| return membershipJoinAction(data); | |
| } | |
| if (data.content.membership === 'leave') { | |
| await membershipLeaveAction(data); | |
| return; | |
| } | |
| if (data.content.membership === 'join') { | |
| await membershipJoinAction(data); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/events/member.ts around lines 90 to 96, the
code returns the promise from membershipLeaveAction/membershipJoinAction without
awaiting, which bypasses the surrounding try/catch; change those lines to await
the call (await membershipLeaveAction(data); return; and await
membershipJoinAction(data); return;) so any thrown errors are caught and logged
by the existing try/catch (ensure the enclosing function is async if not
already).
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Refactor