regression(federation): fix mixed DM invite not working (RocketChat#37924)

Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>

Author: 2 people

apps/meteor/server/services/room/service.ts

@@ -36,7 +36,7 @@ import { removeRoomOwner } from '../../methods/removeRoomOwner';
 export class RoomService extends ServiceClassInternal implements IRoomService {
 	protected name = 'room';
 
-	async updateDirectMessageRoomName(room: IRoom): Promise<boolean> {
+	async updateDirectMessageRoomName(room: IRoom, ignoreStatusFromSubs?: string[]): Promise<boolean> {
 		const subs = await Subscriptions.findByRoomId(room._id, { projection: { u: 1, status: 1 } }).toArray();
 
 		const uids = subs.map((sub) => sub.u._id);
@@ -47,7 +47,7 @@ export class RoomService extends ServiceClassInternal implements IRoomService {
 
 		for await (const sub of subs) {
 			// don't update the name if the user is invited but hasn't accepted yet
-			if (sub.status === 'INVITED') {
+			if (!ignoreStatusFromSubs?.includes(sub._id) && sub.status === 'INVITED') {
 				continue;
 			}
 			await Subscriptions.updateOne({ _id: sub._id }, { $set: roomNames[sub.u._id] });

ee/packages/federation-matrix/src/FederationMatrix.ts

@@ -291,13 +291,9 @@ export class FederationMatrix extends ServiceClass implements IFederationMatrixS
 						continue;
 					}
 
-					if (!isUserNativeFederated(member)) {
-						continue;
-					}
-
 					try {
 						await federationSDK.inviteUserToRoom(
-							userIdSchema.parse(member.username),
+							isUserNativeFederated(member) ? userIdSchema.parse(member.username) : `@${member.username}:${this.serverName}`,
 							roomIdSchema.parse(matrixRoomResult.room_id),
 							userIdSchema.parse(actualMatrixUserId),
 						);
@@ -919,8 +915,6 @@ export class FederationMatrix extends ServiceClass implements IFederationMatrixS
 
 		if (action === 'accept') {
 			await federationSDK.acceptInvite(room.federation.mrid, matrixUserId);
-
-			await Room.performAcceptRoomInvite(room, subscription, user);
 		}
 
 		if (action === 'reject') {

ee/packages/federation-matrix/src/events/member.ts

@@ -215,7 +215,7 @@ async function handleJoin({
 
 	// update room name for DMs
 	if (room.t === 'd') {
-		await Room.updateDirectMessageRoomName(room);
+		await Room.updateDirectMessageRoomName(room, [subscription._id]);
 	}
 
 	if (!subscription.status) {

ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts

@@ -1097,7 +1097,7 @@ const waitForRoomEvent = async (
 					});
 				});
 
-				it.failing('should accept the invitation by the Rocket.Chat user', async () => {
+				it('should accept the invitation by the Rocket.Chat user', async () => {
 					const response = await acceptRoomInvite(rcRoom._id, rcUserConfig2);
 					expect(response.success).toBe(true);
 				});
@@ -1204,22 +1204,7 @@ const waitForRoomEvent = async (
 						});
 
 						beforeAll(async () => {
-							// Create non-federated DM between rcUser1 and rcUser2
-							const nonFedDmResponse = await rcUser1.config.request
-								.post(api('dm.create'))
-								.set(rcUser1.config.credentials)
-								.send({
-									username: rcUser2.username,
-								})
-								.expect(200);
-
-							expect(nonFedDmResponse.body).toHaveProperty('success', true);
-
-							const roomInfoNonFed = await getRoomInfo(nonFedDmResponse.body.room._id, rcUser1.config);
-
-							rcRoomNonFed = roomInfoNonFed.room as IRoomNativeFederated;
-
-							// Create federated DM between rcUser1 and rcUser2 and the Synapse user
+							// First create a federated DM between rcUser1 and rcUser2 and the Synapse user, so this is the oldest room
 							const fedDmResponse = await rcUser1.config.request
 								.post(api('dm.create'))
 								.set(rcUser1.config.credentials)
@@ -1260,6 +1245,21 @@ const waitForRoomEvent = async (
 								// After acceptance, should display the Synapse user's ID
 								expect(sub).toHaveProperty('fname', `${federationConfig.hs1.adminMatrixUserId}, ${rcUser1.fullName}`);
 							});
+
+							// Then create non-federated DM between rcUser1 and rcUser2 which should be returned on duplication
+							const nonFedDmResponse = await rcUser1.config.request
+								.post(api('dm.create'))
+								.set(rcUser1.config.credentials)
+								.send({
+									username: rcUser2.username,
+								})
+								.expect(200);
+
+							expect(nonFedDmResponse.body).toHaveProperty('success', true);
+
+							const roomInfoNonFed = await getRoomInfo(nonFedDmResponse.body.room._id, rcUser1.config);
+
+							rcRoomNonFed = roomInfoNonFed.room as IRoomNativeFederated;
 						});
 
 						it('should have two DMs with same users', async () => {
@@ -1322,9 +1322,6 @@ const waitForRoomEvent = async (
 
 							expect(response.body).toHaveProperty('success', true);
 
-							// expect(response.body).toHaveProperty('room._id', rcRoom._id);
-
-							// TODO this is currently wrong, the correct value should be rcRoom._id
 							expect(response.body).toHaveProperty('room._id', rcRoomNonFed._id);
 						});
 					});
@@ -1435,7 +1432,7 @@ const waitForRoomEvent = async (
 							});
 						});
 
-						it.failing('should create a federated DM between rcUser1 and rcUser2 and the Synapse user', async () => {
+						it('should create a federated DM between rcUser1 and rcUser2 and the Synapse user', async () => {
 							const fedDmResponse = await rcUser1.config.request
 								.post(api('dm.create'))
 								.set(rcUser1.config.credentials)
@@ -1486,13 +1483,21 @@ const waitForRoomEvent = async (
 								{ delayMs: 100 },
 							);
 
-							// TODO accept not working
 							const response = await acceptRoomInvite(rcRoom._id, rcUser2.config);
 							expect(response.success).toBe(true);
+
+							await retry(
+								'wait for the join to be processed',
+								async () => {
+									const sub = await getSubscriptionByRoomId(rcRoom._id, rcUser2.config.credentials, rcUser2.config.request);
+
+									expect(sub).not.toHaveProperty('status');
+								},
+								{ delayMs: 100 },
+							);
 						});
 
-						// TODO this is failing because we cannot accept the invite properly on the line above
-						it.failing('should have two DMs with same users', async () => {
+						it('should have a single DM with same users before one leaves', async () => {
 							const roomsResponse = await rcUser1.config.request.get(api('rooms.get')).set(rcUser1.config.credentials).expect(200);
 
 							const dmRooms = roomsResponse.body.update.filter(
@@ -1506,7 +1511,9 @@ const waitForRoomEvent = async (
 
 							// at this time there should be only one DM with only two users (the non-federated one)
 							expect(dmRooms.length).toBe(1);
+						});
 
+						it('should leave the federated DM with three users', async () => {
 							// now the rc user leaves the federated DM
 							const response = await rcUser2.config.request
 								.post(api('rooms.leave'))
@@ -1518,19 +1525,21 @@ const waitForRoomEvent = async (
 
 							expect(response.body).toHaveProperty('success', true);
 
-							await retry(
-								'wait for the leave to be processed',
-								async () => {
-									expect(hs1Room1.getMyMembership()).toBe('leave');
+							// Verify room is no longer accessible to RC users
+							await retry('waiting for room cleanup', async () => {
+								const roomsResponse = await rcUser2.config.request.get(api('rooms.get')).set(rcUser2.config.credentials).expect(200);
 
-									const sub = await getSubscriptionByRoomId(rcRoom._id, rcUser1.config.credentials, rcUser1.config.request);
+								expect(roomsResponse.body).toHaveProperty('update');
+								expect(roomsResponse.body.update).toBeInstanceOf(Array);
 
-									// After leave, should display only the RC user's full name
-									expect(sub).toHaveProperty('fname', rcUser2.fullName);
-								},
-								{ delayMs: 100 },
-							);
+								const room = roomsResponse.body.update?.find((r: IRoom) => r._id === rcRoom._id);
+
+								// Room should not be in active rooms list
+								expect(room).toBeUndefined();
+							});
+						});
 
+						it('should have two DMs with same users', async () => {
 							// now there should be two DMs with the same users
 							const roomsResponseAfterLeave = await rcUser1.config.request
 								.get(api('rooms.get'))
@@ -1560,9 +1569,6 @@ const waitForRoomEvent = async (
 
 							expect(response.body).toHaveProperty('success', true);
 
-							// expect(response.body).toHaveProperty('room._id', rcRoom._id);
-
-							// TODO this is currently wrong, the correct value should be rcRoom._id
 							expect(response.body).toHaveProperty('room._id', rcRoom1on1._id);
 						});
 					});

ee/packages/federation-matrix/tests/end-to-end/room.spec.ts

@@ -14,6 +14,7 @@ import {
 } from '../../../../../apps/meteor/tests/data/rooms.helper';
 import { type IRequestConfig, getRequestConfig, createUser, deleteUser } from '../../../../../apps/meteor/tests/data/users.helper';
 import { IS_EE } from '../../../../../apps/meteor/tests/e2e/config/constants';
+import { retry } from '../../../../../apps/meteor/tests/end-to-end/api/helpers/retry';
 import { federationConfig } from '../helper/config';
 import { createDDPListener } from '../helper/ddp-listener';
 import { SynapseClient } from '../helper/synapse-client';
@@ -1612,13 +1613,20 @@ import { SynapseClient } from '../helper/synapse-client';
 
 				describe('It should reflect all the members and messagens on the rocket.chat side', () => {
 					it('It should show all the three users in the members list', async () => {
-						const members = await getRoomMembers(rid, rc1AdminRequestConfig);
-						expect(members.members.length).toBe(3);
-						expect(members.members.find((member: IUser) => member.username === federationConfig.rc1.adminUser)).not.toBeNull();
-						expect(
-							members.members.find((member: IUser) => member.username === federationConfig.rc1.additionalUser1.username),
-						).not.toBeNull();
-						expect(members.members.find((member: IUser) => member.username === federationConfig.hs1.adminMatrixUserId)).not.toBeNull();
+						await retry(
+							'Getting room members until all are present',
+							async () => {
+								const members = await getRoomMembers(rid, rc1AdminRequestConfig);
+
+								expect(members.members.length).toBe(3);
+								expect(members.members.find((member: IUser) => member.username === federationConfig.rc1.adminUser)).not.toBeNull();
+								expect(
+									members.members.find((member: IUser) => member.username === federationConfig.rc1.additionalUser1.username),
+								).not.toBeNull();
+								expect(members.members.find((member: IUser) => member.username === federationConfig.hs1.adminMatrixUserId)).not.toBeNull();
+							},
+							{ delayMs: 200 },
+						);
 					});
 				});
 			});

packages/core-services/src/types/IRoomService.ts

@@ -69,5 +69,5 @@ export interface IRoomService {
 		status?: 'INVITED';
 		roles?: ISubscription['roles'];
 	}): Promise<string | undefined>;
-	updateDirectMessageRoomName(room: IRoom): Promise<boolean>;
+	updateDirectMessageRoomName(room: IRoom, ignoreStatusFromSubs?: string[]): Promise<boolean>;
 }

packages/models/src/models/Rooms.ts

@@ -897,7 +897,13 @@ export class RoomsRaw extends BaseRaw<IRoom> implements IRoomsModel {
 			uids: { $size: uid.length, $all: uid },
 		};
 
-		return this.findOne<IRoom>(query, options);
+		return this.findOne<IRoom>(query, {
+			...options,
+			sort: {
+				federated: 1,
+				ts: 1,
+			},
+		});
 	}
 
 	findFederatedRooms(options: FindOptions<IRoom> = {}): FindCursor<IRoomFederated> {