Skip to content

fix(federation): block username change of users in federated rooms #37210

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rodrigok merged 2 commits into from
Oct 14, 2025
Merged

fix(federation): block username change of users in federated rooms #37210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bcb908c
block username change of federated users or users in federated rooms
ricardogarim Oct 10, 2025
1778672
add tests
ricardogarim Oct 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion apps/meteor/app/lib/server/functions/setUsername.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { api } from '@rocket.chat/core-services';
import type { IUser } from '@rocket.chat/core-typings';
import { isUserNativeFederated } from '@rocket.chat/core-typings';
import type { Updater } from '@rocket.chat/models';
import { Invites, Users } from '@rocket.chat/models';
import { Invites, Users, Subscriptions } from '@rocket.chat/models';
import { Accounts } from 'meteor/accounts-base';
import { Meteor } from 'meteor/meteor';
import type { ClientSession } from 'mongodb';
Expand All @@ -20,6 +21,13 @@ import { SystemLogger } from '../../../../server/lib/logger/system';
import { settings } from '../../../settings/server';
import { notifyOnUserChange } from '../lib/notifyListener';

const isUserInFederatedRooms = async (userId: string): Promise<boolean> => {
const cursor = Subscriptions.findUserFederatedRoomIds(userId);
const hasAny = await cursor.hasNext();
await cursor.close();
return hasAny;
};

export const setUsernameWithValidation = async (userId: string, username: string, joinDefaultChannelsSilenced?: boolean): Promise<void> => {
if (!username) {
throw new Meteor.Error('error-invalid-username', 'Invalid username', { method: 'setUsername' });
Expand All @@ -31,6 +39,12 @@ export const setUsernameWithValidation = async (userId: string, username: string
throw new Meteor.Error('error-invalid-user', 'Invalid user', { method: 'setUsername' });
}

if (isUserNativeFederated(user) || (await isUserInFederatedRooms(userId))) {
throw new Meteor.Error('error-not-allowed', 'Cannot change username for federated users or users in federated rooms', {
method: 'setUsername',
});
}

if (user.username && !settings.get('Accounts_AllowUsernameChange')) {
throw new Meteor.Error('error-not-allowed', 'Not allowed');
}
Expand Down Expand Up @@ -84,6 +98,12 @@ export const _setUsername = async function (
return false;
}

if (isUserNativeFederated(fullUser) || (await isUserInFederatedRooms(userId))) {
throw new Meteor.Error('error-not-allowed', 'Cannot change username for federated users or users in federated rooms', {
method: 'setUsername',
});
}
Comment on lines +101 to +105
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Move federation check after user object is available.

The federation check at lines 101-105 happens before ensuring a valid user object is available. When fullUser is not provided (which line 107 suggests is possible with the fallback fullUser || await Users.findOneById(...)), calling isUserNativeFederated(fullUser) on an undefined/null value may not work correctly.

While the type guard might handle this gracefully by returning false, relying on this behavior is fragile. Additionally, a federated user might theoretically not be in any federated rooms, so the isUserInFederatedRooms fallback wouldn't catch this edge case.

Apply this diff to move the federation check after the user object is guaranteed to be available:

 	if (!validateUsername(username)) {
 		return false;
 	}
 
-	if (isUserNativeFederated(fullUser) || (await isUserInFederatedRooms(userId))) {
-		throw new Meteor.Error('error-not-allowed', 'Cannot change username for federated users or users in federated rooms', {
-			method: 'setUsername',
-		});
-	}
-
 	const user = fullUser || (await Users.findOneById(userId, { session }));
+	
+	if (isUserNativeFederated(user) || (await isUserInFederatedRooms(userId))) {
+		throw new Meteor.Error('error-not-allowed', 'Cannot change username for federated users or users in federated rooms', {
+			method: 'setUsername',
+		});
+	}
+
 	// User already has desired username, return
 	if (user.username === username) {
 		return user;
📝 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.

Suggested change
if (isUserNativeFederated(fullUser) || (await isUserInFederatedRooms(userId))) {
throw new Meteor.Error('error-not-allowed', 'Cannot change username for federated users or users in federated rooms', {
method: 'setUsername',
});
}
if (!validateUsername(username)) {
return false;
}
const user = fullUser || (await Users.findOneById(userId, { session }));
if (isUserNativeFederated(user) || (await isUserInFederatedRooms(userId))) {
throw new Meteor.Error('error-not-allowed', 'Cannot change username for federated users or users in federated rooms', {
method: 'setUsername',
});
}
// User already has desired username, return
if (user.username === username) {
return user;
}
🤖 Prompt for AI Agents 
In apps/meteor/app/lib/server/functions/setUsername.ts around lines 101 to 105,
the federation check runs before guaranteeing a valid user object; move the
federation check to after you resolve fullUser (i.e., keep the existing fallback
fullUser = fullUser || await Users.findOneById(userId) first), then perform the
checks using the now-guaranteed fullUser: call isUserNativeFederated(fullUser)
and, if needed, await isUserInFederatedRooms(userId) and throw the Meteor.Error
when either condition is true.


const user = fullUser || (await Users.findOneById(userId, { session }));
// User already has desired username, return
if (user.username === username) {
Expand Down
48 changes: 47 additions & 1 deletion apps/meteor/tests/unit/app/lib/server/functions/setUsername.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ describe('setUsername', () => {
findOneById: sinon.stub(),
setUsername: sinon.stub(),
},
Subscriptions: {
findUserFederatedRoomIds: sinon.stub(),
},
Accounts: {
sendEnrollmentEmail: sinon.stub(),
},
Expand Down Expand Up @@ -49,7 +52,7 @@ describe('setUsername', () => {
'../../../../server/database/utils': { onceTransactionCommitedSuccessfully: async (cb: any, _sess: any) => cb() },
'meteor/meteor': { Meteor: { Error } },
'@rocket.chat/core-services': { api: stubs.api },
'@rocket.chat/models': { Users: stubs.Users, Invites: stubs.Invites },
'@rocket.chat/models': { Users: stubs.Users, Invites: stubs.Invites, Subscriptions: stubs.Subscriptions },
'meteor/accounts-base': { Accounts: stubs.Accounts },
'underscore': stubs.underscore,
'../../../settings/server': { settings: stubs.settings },
Expand All @@ -65,9 +68,17 @@ describe('setUsername', () => {
'../../../../server/lib/logger/system': { SystemLogger: stubs.SystemLogger },
});

beforeEach(() => {
stubs.Subscriptions.findUserFederatedRoomIds.returns({
hasNext: sinon.stub().resolves(false),
close: sinon.stub().resolves(),
});
});

afterEach(() => {
stubs.Users.findOneById.reset();
stubs.Users.setUsername.reset();
stubs.Subscriptions.findUserFederatedRoomIds.reset();
stubs.Accounts.sendEnrollmentEmail.reset();
stubs.settings.get.reset();
stubs.api.broadcast.reset();
Expand Down Expand Up @@ -143,6 +154,41 @@ describe('setUsername', () => {
}
});

it('should throw an error if local user is in federated rooms', async () => {
stubs.Users.findOneById.resolves({ _id: userId, username: null });
stubs.validateUsername.returns(true);
stubs.checkUsernameAvailability.resolves(true);
stubs.Subscriptions.findUserFederatedRoomIds.returns({
hasNext: sinon.stub().resolves(true),
close: sinon.stub().resolves(),
});

try {
await setUsernameWithValidation(userId, 'newUsername');
} catch (error: any) {
expect(stubs.Subscriptions.findUserFederatedRoomIds.calledOnce).to.be.true;
expect(error.message).to.equal('error-not-allowed');
}
});

it('should throw an error if user is federated', async () => {
stubs.Users.findOneById.resolves({
_id: userId,
username: null,
federated: true,
federation: { version: 1, mui: '@user:origin', origin: 'origin' },
});
stubs.validateUsername.returns(true);
stubs.checkUsernameAvailability.resolves(true);

try {
await setUsernameWithValidation(userId, 'newUsername');
} catch (error: any) {
expect(stubs.Subscriptions.findUserFederatedRoomIds.notCalled).to.be.true;
expect(error.message).to.equal('error-not-allowed');
}
});
Comment on lines +174 to +190
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add error assertion to prevent false positives.

The test uses try-catch without asserting that an error was thrown. If the code doesn't throw an error, the test passes silently, creating a false positive.

Apply this diff to add proper error assertion:

 		it('should throw an error if user is federated', async () => {
 			stubs.Users.findOneById.resolves({
 				_id: userId,
 				username: null,
 				federated: true,
 				federation: { version: 1, mui: '@user:origin', origin: 'origin' },
 			});
 			stubs.validateUsername.returns(true);
 			stubs.checkUsernameAvailability.resolves(true);
 
+			let errorThrown = false;
 			try {
 				await setUsernameWithValidation(userId, 'newUsername');
 			} catch (error: any) {
+				errorThrown = true;
 				expect(stubs.Subscriptions.findUserFederatedRoomIds.notCalled).to.be.true;
 				expect(error.message).to.equal('error-not-allowed');
 			}
+			expect(errorThrown).to.be.true;
 		});
📝 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.

Suggested change
it('should throw an error if user is federated', async () => {
stubs.Users.findOneById.resolves({
_id: userId,
username: null,
federated: true,
federation: { version: 1, mui: '@user:origin', origin: 'origin' },
});
stubs.validateUsername.returns(true);
stubs.checkUsernameAvailability.resolves(true);
try {
await setUsernameWithValidation(userId, 'newUsername');
} catch (error: any) {
expect(stubs.Subscriptions.findUserFederatedRoomIds.notCalled).to.be.true;
expect(error.message).to.equal('error-not-allowed');
}
});
it('should throw an error if user is federated', async () => {
stubs.Users.findOneById.resolves({
_id: userId,
username: null,
federated: true,
federation: { version: 1, mui: '@user:origin', origin: 'origin' },
});
stubs.validateUsername.returns(true);
stubs.checkUsernameAvailability.resolves(true);
let errorThrown = false;
try {
await setUsernameWithValidation(userId, 'newUsername');
} catch (error: any) {
errorThrown = true;
expect(stubs.Subscriptions.findUserFederatedRoomIds.notCalled).to.be.true;
expect(error.message).to.equal('error-not-allowed');
}
expect(errorThrown).to.be.true;
});
🤖 Prompt for AI Agents 
In apps/meteor/tests/unit/app/lib/server/functions/setUsername.spec.ts around
lines 174 to 190, the test uses a try/catch but doesn’t assert that an error was
actually thrown, allowing false positives; update the test to explicitly assert
a rejection (e.g., replace the try/catch with an assertion like await
expect(setUsernameWithValidation(userId,
'newUsername')).to.be.rejectedWith('error-not-allowed') and then assert
stubs.Subscriptions.findUserFederatedRoomIds was not called, or if keeping
try/catch, add an explicit fail() after the await to ensure the test fails when
no error is thrown and keep the existing checks inside the catch.


it('should save the user identity when valid username is set', async () => {
stubs.Users.findOneById.resolves({ _id: userId, username: null });
stubs.settings.get.withArgs('Accounts_AllowUsernameChange').returns(true);
Expand Down
Loading