fix(federation): error on first user interaction (#41689)

Author: sampaiodiego

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

@@ -326,7 +326,8 @@ export class RoomService extends ServiceClassInternal implements IRoomService {
 			...(inviter && { inviter: { _id: inviter._id, username: inviter.username!, ...(inviter.name && { name: inviter.name }) } }),
 			...autoTranslateConfig,
 			...getDefaultSubscriptionPref(userToBeAdded),
-			...(room.t === 'd' && inviter && { fname: inviter.name, name: inviter.username }),
+			// `name` is optional for users, so fall back to the username like `getNameForDMs` does
+			...(room.t === 'd' && inviter && { fname: inviter.name || inviter.username, name: inviter.username }),
 		});
 
 		if (insertedId) {

ee/packages/federation-matrix/src/api/_matrix/client/account.ts

@@ -5,6 +5,7 @@ import type { ClientRouter } from './_shared';
 import { isMatrixErrorProps, license, tags } from './_shared';
 import { createOrUpdateFederatedUser } from '../../../helpers/createOrUpdateFederatedUser';
 import { decodeXmppUserId, isFullXmppUserId, parseXmppUserId } from '../../../helpers/parseXmppUserId';
+import { validateFederatedUsername } from '../../../helpers/validateFederatedUsername';
 import { logger } from '../../logger';
 import { isAppServiceAuthenticatedMiddleware } from '../../middlewares/isAppServiceAuthenticated';
 
@@ -93,7 +94,18 @@ export const addAccountRoutes = (router: ClientRouter) => {
 					// The spec defines `username` as the desired localpart; normalize either form to the
 					// fully-qualified MXID, which is what gets stored and what `user_id` must carry.
 					const withSigil = body.username.startsWith('@') ? body.username : `@${body.username}`;
-					const userId = withSigil.includes(':') ? withSigil : `${withSigil}:${serverName}`;
+					const userId: string = withSigil.includes(':') ? withSigil : `${withSigil}:${serverName}`;
+
+					if (!validateFederatedUsername(userId)) {
+						logger.warn({ msg: 'Malformed user id during AS registration', username: body.username });
+						return {
+							statusCode: 400,
+							body: {
+								errcode: 'M_INVALID_USERNAME',
+								error: 'Could not derive a username from the provided user id',
+							},
+						};
+					}
 
 					if (isReservedByAnotherAppService(userId)) {
 						return {
@@ -146,6 +158,17 @@ export const addAccountRoutes = (router: ClientRouter) => {
 
 				const username = `@${decodedUsername.resource}:${serverName}`;
 
+				if (!validateFederatedUsername(username)) {
+					logger.warn({ msg: 'Malformed user id derived from XMPP user id during AS registration', username: body.username });
+					return {
+						statusCode: 400,
+						body: {
+							errcode: 'M_INVALID_USERNAME',
+							error: 'Could not derive a username from the provided XMPP user id',
+						},
+					};
+				}
+
 				if (isReservedByAnotherAppService(username)) {
 					return {
 						statusCode: 400,

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

@@ -1,15 +1,15 @@
 import { Room, Upload } from '@rocket.chat/core-services';
-import { isBannedSubscription, isRegisterUser } from '@rocket.chat/core-typings';
+import { isBannedSubscription } 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';
 import { Rooms, Subscriptions, Users } from '@rocket.chat/models';
 import debounce from 'lodash.debounce';
 import mem from 'mem';
 
-import { createOrUpdateFederatedUser } from '../helpers/createOrUpdateFederatedUser';
 import { extractDomainFromMatrixUserId } from '../helpers/extractDomainFromMatrixUserId';
 import { getFederatedRoomName } from '../helpers/getFederatedRoomName';
+import { getOrCreateFederatedUser } from '../helpers/getOrCreateFederatedUser';
 import { getUsernameServername } from '../helpers/getUsernameServername';
 import { MatrixMediaService } from '../services/MatrixMediaService';
 
@@ -74,40 +74,6 @@ async function downloadAndSetAvatar(user: IUser, avatarUrl: string | null): Prom
 	}
 }
 
-async function getOrCreateFederatedUser(userId: string): Promise<IUser> {
-	try {
-		const serverName = federationSDK.getConfig('serverName');
-		const [username, userServerName, isLocal] = getUsernameServername(userId, serverName);
-
-		const user = await Users.findOneByUsername(username);
-		if (user) {
-			return user;
-		}
-
-		const as = federationSDK.getAppServiceForUser(userId);
-		if (as) {
-			const user = await Users.findOneByUsername(userId);
-			if (!user) {
-				throw new Error('AppService user not found for creating user');
-			}
-			return user;
-		}
-
-		if (isLocal) {
-			throw new Error(`Local user ${username} not found for Matrix ID: ${userId}`);
-		}
-
-		return createOrUpdateFederatedUser({
-			username: userId,
-			name: userId,
-			origin: userServerName,
-		});
-	} catch (err) {
-		logger.error({ msg: 'Error getting or creating federated user', err, userId });
-		throw new Error(`Error getting or creating federated user ${userId}`);
-	}
-}
-
 async function getOrCreateFederatedRoom({
 	matrixRoomId,
 	roomName,
@@ -200,18 +166,7 @@ async function handleInvite({
 	unsigned,
 }: HomeserverEventSignatures['homeserver.matrix.membership']['event']): Promise<void> {
 	const inviterUser = await getOrCreateFederatedUser(senderId);
-	if (!inviterUser) {
-		throw new Error(`Failed to get or create inviter user: ${senderId}`);
-	}
-
-	if (!isRegisterUser(inviterUser)) {
-		throw new Error('Inviter user is not registered');
-	}
-
 	const inviteeUser = await getOrCreateFederatedUser(userId);
-	if (!inviteeUser) {
-		throw new Error(`Failed to get or create invitee user: ${userId}`);
-	}
 
 	const strippedState = unsigned.invite_room_state;
 
@@ -298,9 +253,6 @@ async function handleJoin({
 	content,
 }: HomeserverEventSignatures['homeserver.matrix.membership']['event']): Promise<void> {
 	const joiningUser = await getOrCreateFederatedUser(userId);
-	if (!joiningUser?.username) {
-		throw new Error(`Failed to get or create joining user: ${userId}`);
-	}
 
 	const room = await Rooms.findOneFederatedByMrid(roomId);
 	if (!room) {
@@ -343,7 +295,7 @@ async function handleJoin({
 		await Room.updateDirectMessageRoomName(
 			room,
 			[subscription._id],
-			[{ _id: joiningUser._id, name: content.displayname || joiningUser.name || joiningUser.username, username: joiningUser.username }],
+			[{ _id: joiningUser._id, name: content.displayname || joiningUser.name, username: joiningUser.username }],
 		);
 	}
 

ee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.spec.ts

@@ -11,13 +11,20 @@ jest.mock('@rocket.chat/models', () => ({
 
 const mockFindOneAndUpdate = Users.findOneAndUpdate as jest.MockedFunction<typeof Users.findOneAndUpdate>;
 
+const fakeUser = {
+	_id: 'user123',
+	username: '@alice:example.com',
+	name: '@alice:example.com',
+	federated: true,
+	federation: { version: 1, mui: '@alice:example.com', origin: 'example.com' },
+};
+
 describe('createOrUpdateFederatedUser', () => {
 	beforeEach(() => {
 		jest.clearAllMocks();
 	});
 
 	it('should assign the "federated" role to the new user', async () => {
-		const fakeUser = { _id: 'user123', username: '@alice:example.com' };
 		mockFindOneAndUpdate.mockResolvedValueOnce(fakeUser as any);
 
 		await createOrUpdateFederatedUser({ username: '@alice:example.com', origin: 'example.com' });
@@ -28,7 +35,6 @@ describe('createOrUpdateFederatedUser', () => {
 	});
 
 	it('should not assign the "user" role to federated users', async () => {
-		const fakeUser = { _id: 'user123', username: '@alice:example.com' };
 		mockFindOneAndUpdate.mockResolvedValueOnce(fakeUser as any);
 
 		await createOrUpdateFederatedUser({ username: '@alice:example.com', origin: 'example.com' });
@@ -38,7 +44,6 @@ describe('createOrUpdateFederatedUser', () => {
 	});
 
 	it('should set federated=true on the created/updated user', async () => {
-		const fakeUser = { _id: 'user123', username: '@alice:example.com' };
 		mockFindOneAndUpdate.mockResolvedValueOnce(fakeUser as any);
 
 		await createOrUpdateFederatedUser({ username: '@alice:example.com', origin: 'example.com' });
@@ -48,7 +53,6 @@ describe('createOrUpdateFederatedUser', () => {
 	});
 
 	it('should use the provided name when supplied', async () => {
-		const fakeUser = { _id: 'user123', username: '@alice:example.com' };
 		mockFindOneAndUpdate.mockResolvedValueOnce(fakeUser as any);
 
 		await createOrUpdateFederatedUser({ username: '@alice:example.com', name: 'Alice', origin: 'example.com' });
@@ -58,7 +62,6 @@ describe('createOrUpdateFederatedUser', () => {
 	});
 
 	it('should default name to username when name is not provided', async () => {
-		const fakeUser = { _id: 'user123', username: '@alice:example.com' };
 		mockFindOneAndUpdate.mockResolvedValueOnce(fakeUser as any);
 
 		await createOrUpdateFederatedUser({ username: '@alice:example.com', origin: 'example.com' });
@@ -68,7 +71,6 @@ describe('createOrUpdateFederatedUser', () => {
 	});
 
 	it('should set initial status to OFFLINE', async () => {
-		const fakeUser = { _id: 'user123', username: '@alice:example.com' };
 		mockFindOneAndUpdate.mockResolvedValueOnce(fakeUser as any);
 
 		await createOrUpdateFederatedUser({ username: '@alice:example.com', origin: 'example.com' });
@@ -78,7 +80,6 @@ describe('createOrUpdateFederatedUser', () => {
 	});
 
 	it('should store the origin server in the federation object', async () => {
-		const fakeUser = { _id: 'user123', username: '@alice:example.com' };
 		mockFindOneAndUpdate.mockResolvedValueOnce(fakeUser as any);
 
 		await createOrUpdateFederatedUser({ username: '@alice:example.com', origin: 'example.com' });
@@ -88,7 +89,6 @@ describe('createOrUpdateFederatedUser', () => {
 	});
 
 	it('should use upsert so the user is created if not found', async () => {
-		const fakeUser = { _id: 'user123', username: '@alice:example.com' };
 		mockFindOneAndUpdate.mockResolvedValueOnce(fakeUser as any);
 
 		await createOrUpdateFederatedUser({ username: '@alice:example.com', origin: 'example.com' });
@@ -105,8 +105,36 @@ describe('createOrUpdateFederatedUser', () => {
 		);
 	});
 
+	it('should throw when the returned document is not a native federated user', async () => {
+		mockFindOneAndUpdate.mockResolvedValueOnce({ _id: 'user123', username: '@alice:example.com', name: 'Alice' } as any);
+
+		await expect(createOrUpdateFederatedUser({ username: '@alice:example.com', origin: 'example.com' })).rejects.toThrow(
+			'Failed to create or update federated user: @alice:example.com',
+		);
+	});
+
+	// callers subscribe the returned document to a room, so a projection here silently hands them a
+	// user missing every field but the two projected ones
+	it('should not project fields away, so the full user document is returned', async () => {
+		mockFindOneAndUpdate.mockResolvedValueOnce(fakeUser as any);
+
+		await createOrUpdateFederatedUser({ username: '@alice:example.com', origin: 'example.com' });
+
+		const [, , options] = mockFindOneAndUpdate.mock.calls[0];
+		expect(options).not.toHaveProperty('projection');
+	});
+
+	// the pre-image of an upsert that inserted is null, which would fail every first contact
+	it('should ask for the document as it looks after the upsert', async () => {
+		mockFindOneAndUpdate.mockResolvedValueOnce(fakeUser as any);
+
+		await createOrUpdateFederatedUser({ username: '@alice:example.com', origin: 'example.com' });
+
+		const [, , options] = mockFindOneAndUpdate.mock.calls[0];
+		expect((options as any).returnDocument).toBe('after');
+	});
+
 	it('should return the user returned by findOneAndUpdate', async () => {
-		const fakeUser = { _id: 'user123', username: '@alice:example.com' };
 		mockFindOneAndUpdate.mockResolvedValueOnce(fakeUser as any);
 
 		const result = await createOrUpdateFederatedUser({ username: '@alice:example.com', origin: 'example.com' });

ee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.ts

@@ -1,4 +1,11 @@
-import { type IUser, UserStatus } from '@rocket.chat/core-typings';
+import {
+	type IRegisterUser,
+	type IUserNativeFederated,
+	isRegisterUser,
+	isUserNativeFederated,
+	UserStatus,
+} from '@rocket.chat/core-typings';
+import type { UserID } from '@rocket.chat/federation-sdk';
 import { Users } from '@rocket.chat/models';
 
 /**
@@ -9,11 +16,11 @@ import { Users } from '@rocket.chat/models';
  */
 
 export async function createOrUpdateFederatedUser(options: {
-	username: string;
+	username: UserID;
 	name?: string;
 	origin: string;
 	asId?: string;
-}): Promise<IUser> {
+}): Promise<IUserNativeFederated & IRegisterUser> {
 	const { username, name = username, origin, asId } = options;
 
 	// TODO: Have a specific method to handle this upsert
@@ -45,12 +52,12 @@ export async function createOrUpdateFederatedUser(options: {
 		},
 		{
 			upsert: true,
-			projection: { _id: 1, username: 1 },
 			returnDocument: 'after',
 		},
 	);
 
-	if (!user) {
+	// the upsert above writes every field these guards check, so they should never reject a returned document
+	if (!user || !isUserNativeFederated(user) || !isRegisterUser(user)) {
 		throw new Error(`Failed to create or update federated user: ${username}`);
 	}