diff --git a/apps/meteor/server/services/room/service.ts b/apps/meteor/server/services/room/service.ts index 47d3dba8f49c9..d1b8c340312eb 100644 --- a/apps/meteor/server/services/room/service.ts +++ b/apps/meteor/server/services/room/service.ts @@ -375,4 +375,13 @@ export class RoomService extends ServiceClassInternal implements IRoomService { tmid, }); } + + async inviteUserAfterBan(subscription: ISubscription, inviterUser: Pick): Promise { + await Subscriptions.updateOne( + { _id: subscription._id }, + { $set: { status: 'INVITED', open: true, unread: 1, userMentions: 1, groupMentions: 0, alert: true, inviter: inviterUser } }, + ); + + void notifyOnSubscriptionChangedById(subscription._id, 'updated'); + } } diff --git a/ee/packages/federation-matrix/src/events/member.spec.ts b/ee/packages/federation-matrix/src/events/member.spec.ts new file mode 100644 index 0000000000000..9ffd5f61f6d54 --- /dev/null +++ b/ee/packages/federation-matrix/src/events/member.spec.ts @@ -0,0 +1,220 @@ +import { Room } from '@rocket.chat/core-services'; +import type { IRoom, ISubscription, IUser } from '@rocket.chat/core-typings'; +import { Emitter } from '@rocket.chat/emitter'; +import { Rooms, Subscriptions, Users } from '@rocket.chat/models'; + +import { member } from './member'; + +// Mock external dependencies +jest.mock('@rocket.chat/models', () => ({ + Rooms: { + findOneFederatedByMrid: jest.fn(), + findOne: jest.fn(), + }, + Subscriptions: { + findOneByRoomIdAndUserId: jest.fn(), + }, + Users: { + findOneByUsername: jest.fn(), + setName: jest.fn(), + }, +})); + +jest.mock('@rocket.chat/core-services', () => ({ + Room: { + performUserRemoval: jest.fn(), + performUserUnban: jest.fn(), + updateDirectMessageRoomName: jest.fn(), + performAcceptRoomInvite: jest.fn(), + createUserSubscription: jest.fn(), + create: jest.fn(), + }, + Upload: { + resetUserAvatar: jest.fn(), + setUserAvatar: jest.fn(), + }, +})); + +jest.mock('@rocket.chat/logger', () => ({ + Logger: jest.fn().mockImplementation(() => ({ + info: jest.fn(), + debug: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + })), +})); + +// Create a real emitter for our tests +const testEmitter = new Emitter(); + +jest.mock('@rocket.chat/federation-sdk', () => ({ + federationSDK: { + eventEmitterService: { + on: jest.fn((event: string, handler: (...args: any[]) => any) => { + testEmitter.on(event, handler); + }), + }, + getConfig: jest.fn().mockReturnValue('rc.local'), + }, +})); + +jest.mock('../helpers/createOrUpdateFederatedUser'); +jest.mock('../helpers/extractDomainFromMatrixUserId', () => ({ + extractDomainFromMatrixUserId: jest.fn().mockReturnValue('remote.server'), +})); +jest.mock('../helpers/getUsernameServername', () => ({ + getUsernameServername: jest.fn().mockImplementation((mxid: string, _serverName: string) => { + // For @user:rc.local -> local user + if (mxid.includes('rc.local')) { + const username = mxid.substring(1, mxid.indexOf(':')); + return [username, 'rc.local', true]; + } + // For remote users, return the full mxid + return [mxid, 'remote.server', false]; + }), +})); +jest.mock('../services/MatrixMediaService'); + +const mockRooms = Rooms as jest.Mocked; +const mockSubscriptions = Subscriptions as jest.Mocked; +const mockUsers = Users as jest.Mocked; +const mockRoom = Room as jest.Mocked; + +describe('federation member event: handleLeave', () => { + const fakeRoom: IRoom = { + _id: 'room1', + t: 'p', + name: 'test-room', + federation: { mrid: '!room:remote.server', origin: 'remote.server', version: 1 }, + msgs: 0, + usersCount: 2, + u: { _id: 'admin', username: 'admin' }, + ts: new Date(), + _updatedAt: new Date(), + } as any; + + const fakeUser: IUser = { + _id: 'user1', + username: 'testuser', + name: 'Test User', + roles: ['user'], + type: 'user', + } as any; + + const fakeSenderUser: IUser = { + _id: 'admin1', + username: 'admin', + name: 'Admin', + roles: ['admin'], + type: 'user', + } as any; + + beforeAll(() => { + // Register member event handlers + member(); + }); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + function emitLeaveEvent(userId: string, senderId: string, roomId: string): Promise { + return new Promise((resolve, _reject) => { + testEmitter.emit('homeserver.matrix.membership', { + event: { + type: 'm.room.member', + room_id: roomId, + sender: senderId, + state_key: userId, + content: { + membership: 'leave', + }, + unsigned: {}, + }, + }); + // Give the async handler time to run + setTimeout(resolve, 100); + }); + } + + it('should unban user when subscription is BANNED on leave event', async () => { + const bannedSubscription: Partial = { + _id: 'sub1', + rid: 'room1', + u: { _id: 'user1', username: 'testuser', name: 'Test User' }, + status: 'BANNED', + }; + + mockUsers.findOneByUsername.mockResolvedValueOnce(fakeUser).mockResolvedValueOnce(fakeSenderUser); + (mockRooms.findOneFederatedByMrid as jest.Mock).mockResolvedValueOnce(fakeRoom); + mockSubscriptions.findOneByRoomIdAndUserId.mockResolvedValueOnce(bannedSubscription as ISubscription); + mockRoom.performUserUnban.mockResolvedValueOnce(undefined as any); + + await emitLeaveEvent('@testuser:rc.local', '@admin:rc.local', '!room:remote.server'); + + expect(mockRoom.performUserUnban).toHaveBeenCalledWith(fakeRoom, fakeUser, fakeSenderUser); + expect(mockRoom.performUserRemoval).not.toHaveBeenCalled(); + }); + + it('should remove user when subscription exists and is NOT banned on leave event', async () => { + const normalSubscription: Partial = { + _id: 'sub1', + rid: 'room1', + u: { _id: 'user1', username: 'testuser', name: 'Test User' }, + // no status field = normal active subscription + }; + + mockUsers.findOneByUsername.mockResolvedValueOnce(fakeUser).mockResolvedValueOnce(fakeSenderUser); + (mockRooms.findOneFederatedByMrid as jest.Mock).mockResolvedValueOnce(fakeRoom); + mockSubscriptions.findOneByRoomIdAndUserId.mockResolvedValueOnce(normalSubscription as ISubscription); + mockRoom.performUserRemoval.mockResolvedValueOnce(undefined as any); + + await emitLeaveEvent('@testuser:rc.local', '@admin:rc.local', '!room:remote.server'); + + expect(mockRoom.performUserRemoval).toHaveBeenCalledWith(fakeRoom, fakeUser); + expect(mockRoom.performUserUnban).not.toHaveBeenCalled(); + }); + + /** + * PRD-313: "Removed from Room" Error on Second Re-invite + * + * This test reproduces the scenario where a leave event from a previous unban + * arrives AFTER a new INVITED subscription has been created for the re-invite. + * + * The sequence is: + * 1. User is banned (subscription status = BANNED) + * 2. Admin re-invites user, which triggers unban + new invite: + * - Banned subscription is removed + * - Unban event sent to Matrix (kick/leave event) + * - New invite sent to Matrix + * - New INVITED subscription created locally + * 3. Matrix processes the unban and delivers the leave event back + * 4. handleLeave runs: finds the NEW INVITED subscription (not banned) + * 5. BUG: Falls through to performUserRemoval, deleting the INVITED subscription + * 6. User sees "You've been removed from room" error + * + * Expected: handleLeave should NOT remove a subscription in INVITED status + * when processing a leave event, because the leave event is stale (from the unban) + * and the user has already been re-invited. + */ + it('should NOT remove a user with INVITED subscription status on a leave event (PRD-313)', async () => { + const invitedSubscription: Partial = { + _id: 'sub2', + rid: 'room1', + u: { _id: 'user1', username: 'testuser', name: 'Test User' }, + status: 'INVITED', + inviter: { _id: 'admin1', username: 'admin', name: 'Admin' }, + }; + + mockUsers.findOneByUsername.mockResolvedValueOnce(fakeUser).mockResolvedValueOnce(fakeSenderUser); + (mockRooms.findOneFederatedByMrid as jest.Mock).mockResolvedValueOnce(fakeRoom); + mockSubscriptions.findOneByRoomIdAndUserId.mockResolvedValueOnce(invitedSubscription as ISubscription); + + await emitLeaveEvent('@testuser:rc.local', '@admin:rc.local', '!room:remote.server'); + + // The INVITED subscription must NOT be removed - it represents a valid pending invite + expect(mockRoom.performUserRemoval).not.toHaveBeenCalled(); + // And it's not a banned subscription either + expect(mockRoom.performUserUnban).not.toHaveBeenCalled(); + }); +}); diff --git a/ee/packages/federation-matrix/src/events/member.ts b/ee/packages/federation-matrix/src/events/member.ts index ce834f7bf5df9..a279badd7075a 100644 --- a/ee/packages/federation-matrix/src/events/member.ts +++ b/ee/packages/federation-matrix/src/events/member.ts @@ -1,5 +1,5 @@ import { Room, Upload } from '@rocket.chat/core-services'; -import { isBannedSubscription } from '@rocket.chat/core-typings'; +import { isBannedSubscription, isInviteSubscription } from '@rocket.chat/core-typings'; import type { IRoomNativeFederated, IRoom, IUser, RoomType } from '@rocket.chat/core-typings'; import { federationSDK, type HomeserverEventSignatures, type PduForType } from '@rocket.chat/federation-sdk'; import { Logger } from '@rocket.chat/logger'; @@ -242,6 +242,10 @@ async function handleInvite({ const subscription = await Subscriptions.findOneByRoomIdAndUserId(room._id, inviteeUser._id); if (subscription) { + // if subscription state says the user is banned, it means the user was previously banned and is now being re-invited, so we need to unban the user instead of creating a new invite + if (isBannedSubscription(subscription)) { + await Room.inviteUserAfterBan(subscription, inviterUser); + } return; } @@ -358,6 +362,15 @@ async function handleLeave({ return; } + // PRD-313: When a user is unbanned and re-invited in a single operation, the unban sends + // a leave event to Matrix. If that leave event arrives after the new INVITED subscription + // has been created, we must NOT remove the subscription — the leave is stale, and the user + // has a valid pending invite. + if (subscription && isInviteSubscription(subscription)) { + logger.info({ msg: 'Ignoring stale leave event for user with pending invite', userId: leavingUser._id, roomId: room._id }); + return; + } + await Room.performUserRemoval(room, leavingUser); // update room name for DMs diff --git a/packages/core-services/src/types/IRoomService.ts b/packages/core-services/src/types/IRoomService.ts index 57c15e101a4c6..ffce6d17d0fa6 100644 --- a/packages/core-services/src/types/IRoomService.ts +++ b/packages/core-services/src/types/IRoomService.ts @@ -79,4 +79,5 @@ export interface IRoomService { ): Promise; markAsRead(room: IRoom, userId: string, readThreads?: boolean): Promise; readThread(params: { user: IUser; room: IRoom; tmid: string }): Promise; + inviteUserAfterBan(subscription: ISubscription, inviterUser: Pick): Promise; }