fix: can't add local users on remote rooms #281
Conversation
WalkthroughjoinUser in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Internal Caller
participant RoomSvc as RoomService
participant StateSvc as StateService
participant EventBuilder as Event Builder
participant Emitter as EventEmitter
participant FedSvc as FederationService
participant Store as Persistence
Caller->>RoomSvc: joinUser(invite {roomId, sender}, userId)
alt invite.sender is local
RoomSvc->>StateSvc: getLatestState(roomId)
StateSvc-->>RoomSvc: latest state (includes create)
RoomSvc->>EventBuilder: build local membership join event
EventBuilder-->>RoomSvc: joinEvent
RoomSvc->>Store: persist joinEvent
RoomSvc->>Emitter: emit membership event
RoomSvc->>FedSvc: propagate join to other servers
FedSvc-->>RoomSvc: propagate result
RoomSvc-->>Caller: success/failure
else invite.sender is remote
RoomSvc->>FedSvc: makeJoin(roomId, userId)
FedSvc-->>RoomSvc: join response + state
RoomSvc->>StateSvc: reconcile/process returned state
StateSvc-->>RoomSvc: final joinEvent
RoomSvc->>Store: persist final joinEvent
RoomSvc-->>Caller: success/failure
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewersPoem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #281 +/- ##
=======================================
Coverage 60.55% 60.55%
=======================================
Files 67 67
Lines 6691 6691
=======================================
Hits 4052 4052
Misses 2639 2639 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rodrigok
force-pushed
the
fix/add-users-from-same-server-on-remote-rooms
branch
2 times, most recently
from
a43cd9c to
61c35f4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/federation-sdk/src/services/room.service.ts (1)
809-822: Use the room's actual version instead of the default.Line 821 uses
PersistentEventFactory.defaultRoomVersion, but the room may use a different version. Other methods in this file (e.g., lines 608, 689, 771) correctly retrieve the room version viastateService.getRoomVersion(roomId). Using the wrong version can cause auth validation failures, event format mismatches, and federation issues.Apply this diff to use the correct room version:
const room = await stateService.getLatestRoomState(roomId); const createEvent = room.get('m.room.create:'); if (!createEvent) { throw new Error( 'Room create event not found when trying to join a room', ); } + const roomVersion = await stateService.getRoomVersion(roomId); + const membershipEvent = await stateService.buildEvent<'m.room.member'>( { type: 'm.room.member', content: { membership: 'join' }, room_id: roomId, state_key: userId, auth_events: [], depth: 0, prev_events: [], origin_server_ts: Date.now(), sender: userId, }, - PersistentEventFactory.defaultRoomVersion, + roomVersion, );
🧹 Nitpick comments (3)
packages/federation-sdk/src/services/room.service.ts (3)
794-794: Clarify the variable name for better code readability.The variable
residentServeris misleading—it extracts the domain from the invite sender, not the room's resident/hosting server. In a federated room hosted on FED1, if a FED2 user sends the invite, this variable will be FED2. Consider renaming toinviteSenderServerorsenderDomainto avoid confusion.Apply this diff:
- const residentServer = extractDomainFromId(invite.sender); + const senderDomain = extractDomainFromId(invite.sender); // our own room, we can validate the join event by ourselves // once done, emit the event to all participating servers - if (residentServer === configService.serverName) { + if (senderDomain === configService.serverName) {(And update the corresponding reference at line 849.)
826-834: Extract event properties from the built event for consistency.The event emission hardcodes several properties (
content,sender,origin_server_ts) instead of extracting them frommembershipEvent.event. IfbuildEventmodifies these values, the emitted event will contain inconsistent data.Apply this diff to extract all properties from the built event:
this.eventEmitterService.emit('homeserver.matrix.membership', { event_id: membershipEvent.eventId, event: membershipEvent.event, room_id: roomId, state_key: userId, - content: { membership: 'join' }, - sender: userId, - origin_server_ts: Date.now(), + content: membershipEvent.event.content, + sender: membershipEvent.event.sender, + origin_server_ts: membershipEvent.event.origin_server_ts, });
787-794: Consider validating the invite event.The method accepts a
PersistentEventBaseasinvitebut doesn't validate that:
- It's actually an invite event (type
m.room.memberwithcontent.membership === 'invite')- The
state_keymatches theuserIdbeing joined- The invite is still valid (not expired or revoked)
While these checks may exist upstream, adding defensive validation here would make the function more robust and catch misuse earlier.
Example validation to add after line 794:
// Validate invite event if (invite.event.type !== 'm.room.member' || invite.event.content?.membership !== 'invite') { throw new Error('Invalid invite: expected m.room.member event with membership=invite'); } if (invite.event.state_key !== userId) { throw new Error(`Invalid invite: state_key ${invite.event.state_key} does not match userId ${userId}`); }
📜 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)
packages/federation-sdk/src/services/room.service.ts(1 hunks)packages/homeserver/src/controllers/internal/room.controller.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/homeserver/src/controllers/internal/room.controller.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/room.service.ts (2)
packages/room/src/types/_common.ts (1)
UserID(20-20)packages/room/src/manager/event-wrapper.ts (2)
roomId(101-103)extractDomainFromId(23-29)
rodrigok
force-pushed
the
fix/add-users-from-same-server-on-remote-rooms
branch
from
61c35f4 to
cb4a180
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/federation-sdk/src/services/room.service.ts (1)
845-845: UsePersistentEventFactory.defaultRoomVersioninstead of hardcoded room version.Line 845 hardcodes
roomVersion = '10'while the rest of the codebase, including the local join path at line 821, usesPersistentEventFactory.defaultRoomVersion. While both currently equal'10', using the constant ensures consistency and prevents maintenance issues if the default version changes.
🧹 Nitpick comments (3)
packages/federation-sdk/src/services/room.service.ts (3)
799-799: Add error handling for room state retrieval.The call to
getLatestRoomState(roomId)could throw an error if the room doesn't exist locally. Consider adding error handling to provide a clearer error message.Apply this diff:
- const room = await stateService.getLatestRoomState(roomId); + let room; + try { + room = await stateService.getLatestRoomState(roomId); + } catch (error) { + this.logger.error( + { roomId, error }, + 'Failed to get room state for local join', + ); + throw new HttpException( + 'Room not found or inaccessible', + HttpStatus.NOT_FOUND, + ); + }
798-843: Add logging to the local join path.The local join path lacks detailed logging compared to the remote join path (lines 880-955). Adding logs would help with debugging and monitoring.
Consider adding logging at key points:
if (residentServer === configService.serverName) { + this.logger.info( + { roomId, userId, inviteSender: invite.sender }, + 'Processing local join for user on same server as invite sender', + ); + const room = await stateService.getLatestRoomState(roomId); const createEvent = room.get('m.room.create:'); if (!createEvent) { + this.logger.error({ roomId }, 'Create event not found for local join'); throw new Error( 'Room create event not found when trying to join a room', ); } const membershipEvent = await stateService.buildEvent<'m.room.member'>(
809-822: Consider extracting membership event building into a helper method.The logic for building membership events is duplicated in multiple places (lines 243-258, 596-609, 674-690, 756-772, 809-822). Extracting this into a helper method would improve maintainability and reduce code duplication.
Example helper method:
private async buildMembershipEvent( roomId: RoomID, userId: UserID, membership: 'join' | 'leave' | 'invite' | 'ban', roomVersion: RoomVersion, options?: { reason?: string; displayname?: string; is_direct?: boolean; } ): Promise<PersistentEventBase> { return this.stateService.buildEvent<'m.room.member'>( { type: 'm.room.member', content: { membership, ...options, }, room_id: roomId, state_key: userId, auth_events: [], depth: 0, prev_events: [], origin_server_ts: Date.now(), sender: userId, }, roomVersion, ); }
📜 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)
packages/federation-sdk/src/services/room.service.ts(1 hunks)packages/homeserver/src/controllers/internal/room.controller.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/controllers/internal/room.controller.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/room.service.ts (2)
packages/federation-sdk/src/index.ts (2)
PersistentEventBase(25-25)UserID(28-28)packages/room/src/manager/event-wrapper.ts (2)
roomId(101-103)extractDomainFromId(23-29)
| async joinUser(invite: PersistentEventBase, userId: UserID) { | ||
| const configService = this.configService; | ||
| const stateService = this.stateService; | ||
| const federationService = this.federationService; | ||
| const roomId = invite.roomId; | ||
|
|
||
| // where the room is hosted at | ||
| const residentServer = extractDomainFromId(roomId); | ||
| const residentServer = extractDomainFromId(invite.sender); |
There was a problem hiding this comment.
Validate invite parameter and userId matching.
The method accepts an invite parameter but doesn't validate that it's a proper invite event. Consider adding validation to ensure:
invite.type === 'm.room.member'invite.content.membership === 'invite'userId === invite.stateKey(to ensure we're joining the correct user)
Without these checks, the method could process invalid invites or join the wrong user.
Apply this diff to add validation at the beginning of the method:
async joinUser(invite: PersistentEventBase, userId: UserID) {
+ // Validate that the invite is a proper invite event
+ if (invite.type !== 'm.room.member') {
+ throw new HttpException(
+ 'Invalid invite: expected m.room.member event',
+ HttpStatus.BAD_REQUEST,
+ );
+ }
+
+ if (invite.content.membership !== 'invite') {
+ throw new HttpException(
+ 'Invalid invite: expected membership to be invite',
+ HttpStatus.BAD_REQUEST,
+ );
+ }
+
+ if (invite.stateKey !== userId) {
+ throw new HttpException(
+ 'Invalid invite: userId does not match invite state_key',
+ HttpStatus.BAD_REQUEST,
+ );
+ }
+
const configService = this.configService;📝 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.
| async joinUser(invite: PersistentEventBase, userId: UserID) { | |
| const configService = this.configService; | |
| const stateService = this.stateService; | |
| const federationService = this.federationService; | |
| const roomId = invite.roomId; | |
| // where the room is hosted at | |
| const residentServer = extractDomainFromId(roomId); | |
| const residentServer = extractDomainFromId(invite.sender); | |
| async joinUser(invite: PersistentEventBase, userId: UserID) { | |
| // Validate that the invite is a proper invite event | |
| if (invite.type !== 'm.room.member') { | |
| throw new HttpException( | |
| 'Invalid invite: expected m.room.member event', | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| } | |
| if (invite.content.membership !== 'invite') { | |
| throw new HttpException( | |
| 'Invalid invite: expected membership to be invite', | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| } | |
| if (invite.stateKey !== userId) { | |
| throw new HttpException( | |
| 'Invalid invite: userId does not match invite state_key', | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| } | |
| const configService = this.configService; | |
| const stateService = this.stateService; | |
| const federationService = this.federationService; | |
| const roomId = invite.roomId; | |
| // where the room is hosted at | |
| const residentServer = extractDomainFromId(invite.sender); |
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/room.service.ts around lines 787 to 794,
the joinUser method does not validate the invite parameter or ensure the target
user matches the invite; add checks to verify invite.type === 'm.room.member',
invite.content?.membership === 'invite', and that the provided userId equals
invite.stateKey. If any check fails, throw or return an early error (e.g., throw
new Error or return a rejected promise) with a clear message indicating which
validation failed; perform these validations before extracting residentServer or
proceeding with any join logic.
sampaiodiego
deleted the
fix/add-users-from-same-server-on-remote-rooms
branch
3 participants
https://rocketchat.atlassian.net/browse/FDR-232
Summary by CodeRabbit
Bug Fixes
Refactor
Chores