fix: Implement avatar validation for federated users and add correspo…

.changeset/good-fireants-rush.md

@@ -0,0 +1,5 @@
+---
+'@rocket.chat/federation-matrix': patch
+---
+
+Implement avatar validation for federated users

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

@@ -0,0 +1,193 @@
+import { Room, Upload } from '@rocket.chat/core-services';
+import { federationSDK } from '@rocket.chat/federation-sdk';
+import { Rooms, Subscriptions, Users } from '@rocket.chat/models';
+
+import { member } from './member';
+
+jest.mock('lodash.debounce', () => ({
+	__esModule: true,
+	default: (fn: (...args: unknown[]) => unknown) => fn,
+}));
+
+const onMembershipEvent = jest.fn();
+
+jest.mock('@rocket.chat/federation-sdk', () => ({
+	federationSDK: {
+		eventEmitterService: {
+			on: (...args: unknown[]) => onMembershipEvent(...args),
+		},
+		queryProfile: jest.fn(),
+		downloadFromRemoteServer: jest.fn(),
+		getConfig: jest.fn().mockReturnValue('local.server'),
+	},
+}));
+
+jest.mock('@rocket.chat/models', () => ({
+	Rooms: {
+		findOneFederatedByMrid: jest.fn(),
+	},
+	Subscriptions: {
+		findOneByRoomIdAndUserId: jest.fn(),
+	},
+	Users: {
+		findOneByUsername: jest.fn(),
+		setFederationAvatarUrlById: jest.fn(),
+		setName: jest.fn(),
+	},
+}));
+
+jest.mock('@rocket.chat/core-services', () => ({
+	Room: {
+		updateDirectMessageRoomName: jest.fn(),
+		performAcceptRoomInvite: jest.fn(),
+	},
+	Upload: {
+		setUserAvatar: jest.fn(),
+		resetUserAvatar: jest.fn(),
+	},
+}));
+
+describe('member avatar validation', () => {
+	const queryProfileMock = federationSDK.queryProfile as jest.MockedFunction<typeof federationSDK.queryProfile>;
+	const downloadFromRemoteServerMock = federationSDK.downloadFromRemoteServer as jest.MockedFunction<
+		typeof federationSDK.downloadFromRemoteServer
+	>;
+	const findOneByUsernameMock = Users.findOneByUsername as jest.MockedFunction<typeof Users.findOneByUsername>;
+	const setFederationAvatarUrlByIdMock = Users.setFederationAvatarUrlById as jest.MockedFunction<typeof Users.setFederationAvatarUrlById>;
+	const findOneFederatedByMridMock = Rooms.findOneFederatedByMrid as jest.MockedFunction<typeof Rooms.findOneFederatedByMrid>;
+	const findOneByRoomIdAndUserIdMock = Subscriptions.findOneByRoomIdAndUserId as jest.MockedFunction<
+		typeof Subscriptions.findOneByRoomIdAndUserId
+	>;
+	const setUserAvatarMock = Upload.setUserAvatar as jest.MockedFunction<typeof Upload.setUserAvatar>;
+	const resetUserAvatarMock = Upload.resetUserAvatar as jest.MockedFunction<typeof Upload.resetUserAvatar>;
+
+	beforeEach(() => {
+		jest.clearAllMocks();
+		onMembershipEvent.mockClear();
+
+		member();
+	});
+
+	function getHandler() {
+		const [, handler] = onMembershipEvent.mock.calls[0];
+		return handler as ({ event }: { event: any }) => Promise<void>;
+	}
+
+	async function emitJoinEvent({ avatarUrl }: { avatarUrl?: string | null }) {
+		const handler = getHandler();
+
+		const event = {
+			room_id: '!room:remote.server',
+			state_key: '@alice:remote.server',
+			content: {
+				membership: 'join',
+				...(avatarUrl !== undefined ? { avatar_url: avatarUrl } : {}),
+			},
+		};
+
+		await handler({ event });
+	}
+
+	it('skips avatar download when current remote avatar matches stored federation avatar URL', async () => {
+		findOneByUsernameMock.mockResolvedValue({
+			_id: 'u1',
+			username: '@alice:remote.server',
+			name: 'Alice',
+			federation: { avatarUrl: 'mxc://remote.server/same' },
+		} as any);
+		findOneFederatedByMridMock.mockResolvedValue({ _id: 'r1', t: 'c' } as any);
+		findOneByRoomIdAndUserIdMock.mockResolvedValue({ _id: 's1' } as any);
+		queryProfileMock.mockResolvedValue({ avatar_url: 'mxc://remote.server/same' } as any);
+
+		await emitJoinEvent({ avatarUrl: 'mxc://remote.server/old' });
+
+		expect(downloadFromRemoteServerMock).not.toHaveBeenCalled();
+		expect(setUserAvatarMock).not.toHaveBeenCalled();
+		expect(resetUserAvatarMock).not.toHaveBeenCalled();
+		expect(setFederationAvatarUrlByIdMock).not.toHaveBeenCalled();
+	});
+
+	it('downloads and stores avatar when current remote avatar changed', async () => {
+		findOneByUsernameMock.mockResolvedValue({
+			_id: 'u1',
+			username: '@alice:remote.server',
+			name: 'Alice',
+			federation: { avatarUrl: 'mxc://remote.server/old' },
+		} as any);
+		findOneFederatedByMridMock.mockResolvedValue({ _id: 'r1', t: 'c' } as any);
+		findOneByRoomIdAndUserIdMock.mockResolvedValue({ _id: 's1' } as any);
+		queryProfileMock.mockResolvedValue({ avatar_url: 'mxc://remote.server/new' } as any);
+		downloadFromRemoteServerMock.mockResolvedValue(Buffer.from([0x89, 0x50, 0x4e, 0x47, 0, 0, 0, 0]));
+
+		await emitJoinEvent({ avatarUrl: 'mxc://remote.server/old' });
+
+		expect(downloadFromRemoteServerMock).toHaveBeenCalledWith('remote.server', 'new');
+		expect(setUserAvatarMock).toHaveBeenCalledTimes(1);
+		expect(setFederationAvatarUrlByIdMock).toHaveBeenCalledWith('u1', 'mxc://remote.server/new');
+	});
+
+	it('falls back to event payload when profile query fails', async () => {
+		findOneByUsernameMock.mockResolvedValue({
+			_id: 'u1',
+			username: '@alice:remote.server',
+			name: 'Alice',
+			federation: { avatarUrl: 'mxc://remote.server/old' },
+		} as any);
+		findOneFederatedByMridMock.mockResolvedValue({ _id: 'r1', t: 'c' } as any);
+		findOneByRoomIdAndUserIdMock.mockResolvedValue({ _id: 's1' } as any);
+		queryProfileMock.mockRejectedValue(new Error('profile unavailable'));
+		downloadFromRemoteServerMock.mockResolvedValue(Buffer.from([0x89, 0x50, 0x4e, 0x47, 0, 0, 0, 0]));
+
+		await emitJoinEvent({ avatarUrl: 'mxc://remote.server/fallback' });
+
+		expect(downloadFromRemoteServerMock).toHaveBeenCalledWith('remote.server', 'fallback');
+		expect(setFederationAvatarUrlByIdMock).toHaveBeenCalledWith('u1', 'mxc://remote.server/fallback');
+	});
+
+	it('does not treat missing avatar_url as removal when profile query fails', async () => {
+		findOneByUsernameMock.mockResolvedValue({
+			_id: 'u1',
+			username: '@alice:remote.server',
+			name: 'Alice',
+			federation: { avatarUrl: 'mxc://remote.server/old' },
+		} as any);
+		findOneFederatedByMridMock.mockResolvedValue({ _id: 'r1', t: 'c' } as any);
+		findOneByRoomIdAndUserIdMock.mockResolvedValue({ _id: 's1' } as any);
+		queryProfileMock.mockRejectedValue(new Error('profile unavailable'));
+
+		await emitJoinEvent({});
+
+		expect(downloadFromRemoteServerMock).not.toHaveBeenCalled();
+		expect(setUserAvatarMock).not.toHaveBeenCalled();
+		expect(resetUserAvatarMock).not.toHaveBeenCalled();
+		expect(setFederationAvatarUrlByIdMock).not.toHaveBeenCalled();
+	});
+
+	it('coalesces concurrent profile lookups for the same user', async () => {
+		findOneByUsernameMock.mockResolvedValue({
+			_id: 'u1',
+			username: '@alice:remote.server',
+			name: 'Alice',
+			federation: { avatarUrl: 'mxc://remote.server/same' },
+		} as any);
+		findOneFederatedByMridMock.mockResolvedValue({ _id: 'r1', t: 'c' } as any);
+		findOneByRoomIdAndUserIdMock.mockResolvedValue({ _id: 's1' } as any);
+
+		let resolveProfile: ((value: { avatar_url: string }) => void) | undefined;
+		const pendingProfile = new Promise<{ avatar_url: string }>((resolve) => {
+			resolveProfile = resolve;
+		});
+
+		queryProfileMock.mockImplementation(() => pendingProfile as any);
+
+		const firstJoin = emitJoinEvent({ avatarUrl: 'mxc://remote.server/old' });
+		const secondJoin = emitJoinEvent({ avatarUrl: 'mxc://remote.server/old' });
+
+		resolveProfile?.({ avatar_url: 'mxc://remote.server/same' });
+
+		await Promise.all([firstJoin, secondJoin]);
+
+		expect(queryProfileMock).toHaveBeenCalledTimes(1);
+		expect(downloadFromRemoteServerMock).not.toHaveBeenCalled();
+	});
+});

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

@@ -14,14 +14,39 @@ import { MatrixMediaService } from '../services/MatrixMediaService';
 
 const logger = new Logger('federation-matrix:member');
 
+const inFlightFederatedAvatarLookupsByUserId = new Map<string, Promise<string | null | undefined>>();
+
+async function getCurrentFederatedAvatarUrl(
+	userId: string,
+	fallbackAvatarUrl: string | null | undefined,
+): Promise<string | null | undefined> {
+	let lookupPromise = inFlightFederatedAvatarLookupsByUserId.get(userId);
+
+	if (!lookupPromise) {
+		lookupPromise = federationSDK.queryProfile(userId).then((profile) => profile?.avatar_url);
+		inFlightFederatedAvatarLookupsByUserId.set(userId, lookupPromise);
+	}
+
+	try {
+		return await lookupPromise;
+	} catch (error) {
+		logger.warn({ err: error, userId, msg: 'Failed to query current federated profile avatar, falling back to event payload' });
+		return fallbackAvatarUrl;
+	} finally {
+		if (inFlightFederatedAvatarLookupsByUserId.get(userId) === lookupPromise) {
+			inFlightFederatedAvatarLookupsByUserId.delete(userId);
+		}
+	}
+}
+
 async function downloadAndSetAvatar(user: IUser, avatarUrl: string | null): Promise<void> {
 	try {
 		// if no avatarUrl is provided, it means the user removed his avatar, so we need to set an empty avatar to remove the avatar from their side as well
 		if (!avatarUrl) {
 			await Upload.resetUserAvatar(user);
+			await Users.setFederationAvatarUrlById(user._id, '');
 			return;
 		}
-
 		if (!avatarUrl?.startsWith('mxc://')) {
 			return;
 		}
@@ -66,8 +91,8 @@ async function downloadAndSetAvatar(user: IUser, avatarUrl: string | null): Prom
 			return;
 		}
 
-		// TODO need to perform a validation to check if the user actually changed avatar
 		await Upload.setUserAvatar(user, buffer, contentType, 'rest');
+		await Users.setFederationAvatarUrlById(user._id, avatarUrl);
 	} catch (error) {
 		logger.error({ err: error, user: user.username, msg: `Error downloading/setting avatar for user` });
 	}
@@ -297,8 +322,13 @@ async function handleJoin({
 
 	// handle avatar updates to membership events
 	if (senderServerName !== federationSDK.getConfig('serverName')) {
-		// TODO if there is no avatar_url we may want to validate first if we should remove the user avatar because if may be dealing with an old join event, and the user may have changed their avatar since then, so we need to check if the avatar_url is different from the current one before removing it
-		void downloadAndSetAvatarDebounced(joiningUser._id, joiningUser, content.avatar_url || null);
+		const fallbackAvatarUrl = 'avatar_url' in content ? content.avatar_url ?? null : undefined;
+		const currentAvatarUrl = await getCurrentFederatedAvatarUrl(userId, fallbackAvatarUrl);
+		const storedAvatarUrl = joiningUser.federation?.avatarUrl || null;
+
+		if (currentAvatarUrl !== undefined && currentAvatarUrl !== storedAvatarUrl) {
+			void downloadAndSetAvatarDebounced(joiningUser._id, joiningUser, currentAvatarUrl);
+		}
 	}
 
 	// updates user name whenever we receive a join event, because Matrix sends a new join event with the updated display name whenever a user changes their display name