fix(federation): block username change of users in federated rooms #37210
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughAdds federation-aware guards to username change flow: imports federation helpers, checks if a user is federated or in any federated rooms, and blocks username changes accordingly. Unit tests were extended to mock Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Caller
participant Service as setUsernameWithValidation
participant Typing as isUserNativeFederated
participant SubModel as Subscriptions.findUserFederatedRoomIds
Caller->>Service: request username change (userId, newUsername)
Service->>Typing: isUserNativeFederated(user)
alt User is federated
Service-->>Caller: throw error (federated user cannot rename)
else Not federated
Service->>SubModel: findUserFederatedRoomIds(userId)
SubModel-->>Service: cursor (hasNext/close)
alt User in federated rooms
Service-->>Caller: throw error (user in federated rooms)
else No federated rooms
Service->>Service: run existing validations (format, availability, enroll/first-run)
Service-->>Caller: proceed / complete username change
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37210 +/- ##
==================================================
- Coverage 66.37% 66.36% -0.01%
==================================================
Files 3386 3386
Lines 115619 115629 +10
Branches 21351 21359 +8
==================================================
+ Hits 76739 76741 +2
- Misses 36275 36280 +5
- Partials 2605 2608 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/app/lib/server/functions/setUsername.ts(4 hunks)apps/meteor/tests/unit/app/lib/server/functions/setUsername.spec.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/app/lib/server/functions/setUsername.ts (1)
packages/core-typings/src/IUser.ts (1)
isUserNativeFederated(276-277)
apps/meteor/tests/unit/app/lib/server/functions/setUsername.spec.ts (1)
apps/meteor/app/lib/server/functions/setUsername.ts (1)
setUsernameWithValidation(31-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 📦 Build Packages
🔇 Additional comments (8)
apps/meteor/app/lib/server/functions/setUsername.ts (4)
3-3: LGTM!The import of
isUserNativeFederatedis appropriate for checking if a user is a federated user based on the user object structure.
5-5: LGTM!The addition of
Subscriptionsto the imports enables checking if a user belongs to federated rooms.
24-29: LGTM!The helper function properly checks for federated room membership and correctly handles cursor cleanup by calling
close()after checkinghasNext().
42-46: LGTM!The federation guard is correctly placed after fetching and validating the user exists (lines 36-40), ensuring we have a valid user object before checking federation status.
apps/meteor/tests/unit/app/lib/server/functions/setUsername.spec.ts (4)
14-16: LGTM!The
Subscriptionsstub is properly added to support federation-aware tests.
55-55: LGTM!The
Subscriptionsmodel is correctly wired through proxyquire alongside other models.
71-76: LGTM!The
beforeEachhook correctly configures the default cursor mock using.returns()(not.resolves()), which matches the implementation's expectation thatfindUserFederatedRoomIdsreturns a cursor object synchronously.
81-81: LGTM!The stub is properly reset in the
afterEachhook.
| 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', | ||
| }); | ||
| } |
There was a problem hiding this comment.
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.
| 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.
| 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.resolves({ | ||
| 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'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Fix cursor mock and test assertion pattern.
This test has two issues:
-
Line 161 uses
.resolves()instead of.returns(): The implementation expectsfindUserFederatedRoomIdsto return a cursor object synchronously, but.resolves()makes it return a Promise. This is inconsistent with the correct pattern used inbeforeEach(line 72) and would cause the test to fail. -
Missing error assertion: The test uses try-catch without asserting that an error was actually thrown. If the code doesn't throw an error, the test passes silently, creating a false positive.
Apply this diff to fix both issues:
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.resolves({
+ stubs.Subscriptions.findUserFederatedRoomIds.returns({
hasNext: sinon.stub().resolves(true),
close: sinon.stub().resolves(),
});
+ let errorThrown = false;
try {
await setUsernameWithValidation(userId, 'newUsername');
} catch (error: any) {
+ errorThrown = true;
expect(stubs.Subscriptions.findUserFederatedRoomIds.calledOnce).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.
| 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.resolves({ | |
| 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 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(), | |
| }); | |
| let errorThrown = false; | |
| try { | |
| await setUsernameWithValidation(userId, 'newUsername'); | |
| } catch (error: any) { | |
| errorThrown = true; | |
| expect(stubs.Subscriptions.findUserFederatedRoomIds.calledOnce).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 157-172, change the mock for Subscriptions.findUserFederatedRoomIds to
return the cursor synchronously (use .returns(...) with the object containing
hasNext and close stubs) instead of .resolves(...), and replace the current
try/catch pattern with a proper assertion that the call rejects (e.g., await
expect(setUsernameWithValidation(userId,
'newUsername')).to.be.rejectedWith('error-not-allowed') or keep the try/catch
but add an explicit expect.fail() immediately after the await to ensure the test
fails if no error is thrown); also keep the assertion that
findUserFederatedRoomIds was called once inside the rejection assertion block or
after awaiting the rejected promise.
| 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'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
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.
| 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.
ricardogarim
force-pushed
the
fix/federation-block-username-change
branch
from
19c19a6 to
1778672
Compare
As per FDR-229, this PR introduces a block on username changes for users who are:
According to the Matrix specification, username changes are not allowed — only display name updates are permitted.
This change prevents breaking the federation flow.
A follow-up PR will enable updating display names both from Rocket.Chat to remote nodes and vice versa.
Summary by CodeRabbit