feat(federation): add support for user renames #37205
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 propagation of user display-name changes to federation: captures prior user state when real name updates, invokes an afterSaveUser callback, introduces Matrix ID helpers and tests, extends FederationMatrix with updateUserProfile, handles member displayname changes, and registers a system message type and translation for display-name changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant setRealName
participant Callbacks
participant FederationHook
participant FederationMatrix
participant Room
Client->>setRealName: request real-name update
setRealName->>setRealName: capture oldUser
setRealName->>Room: broadcast user.nameChanged / user.realNameChanged
setRealName->>Callbacks: onceTransactionCommittedSuccessfully (async)
Callbacks->>Callbacks: afterSaveUser(user, oldUser)
Callbacks->>FederationHook: native-federation-after-user-profile-update
alt displayName changed
FederationHook->>FederationMatrix: updateUserProfile(userId, newDisplayName)
loop per federated room
FederationMatrix->>Room: update member profile
FederationHook->>Room: send system message (user-display-name-changed)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)apps/meteor/app/lib/server/functions/setRealName.ts (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). (3)
🔇 Additional comments (4)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ricardogarim
force-pushed
the
feat/federation-rename-user
branch
from
cfe2740 to
3180191
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
apps/meteor/app/lib/server/functions/setRealName.ts (1)
54-68: Emit hook after commit: prefer awaiting the callback to surface errors.Use an async run to catch failures from downstream handlers (federation propagation, system messages) while still running post-commit.
- await onceTransactionCommitedSuccessfully(async () => { + await onceTransactionCommitedSuccessfully(async () => { if (settings.get('UI_Use_Real_Name') === true) { void api.broadcast('user.nameChanged', { _id: user._id, name: user.name, username: user.username, }); } void api.broadcast('user.realNameChanged', { _id: user._id, name, username: user.username, }); - void callbacks.run('afterSaveUser', { user, oldUser }); + // Prefer awaiting async callbacks chain + await callbacks.run?.('afterSaveUser', { user, oldUser }); + // If the async variant exists in this codebase, use it instead: + // await callbacks.runAsync('afterSaveUser', { user, oldUser }); }, session);Please confirm whether callbacks.runAsync exists in this package so we can switch to it; otherwise, keeping run but awaiting its Promise-capable return is fine.
packages/message-types/src/registrations/common.ts (1)
118-127: Make payload parsing robust; avoid brittle '|' split.Guard against missing delimiter and names containing '|'. Prefer JSON payload with fallback to a bounded split.
instance.registerType({ id: 'user-display-name-changed', system: true, - text: (t, message) => - t('User_changed_display_name', { - username: message.u.username, - old_name: message.msg.split('|')[0], - new_name: message.msg.split('|')[1], - }), + text: (t, message) => { + const raw = String(message.msg ?? ''); + // Prefer JSON payload: {"old_name":"...","new_name":"..."} + let oldName = ''; + let newName = ''; + try { + const p = JSON.parse(raw); + oldName = p.old_name ?? p.oldName ?? ''; + newName = p.new_name ?? p.newName ?? ''; + } catch { + const [o = '', n = ''] = raw.split('|', 2); + oldName = o; + newName = n; + } + return t('User_changed_display_name', { + username: message.u.username, + old_name: oldName, + new_name: newName, + }); + }, });If we switch the producer to send JSON (see hook comment), this will render correctly and remain backward-compatible with existing 'old|new' payloads.
ee/packages/federation-matrix/src/helpers/matrixId.spec.ts (1)
18-21: Add IPv6 MXID validation tests.Current tests don’t cover IPv6 homeserver forms. Add cases like:
- Valid: @user:[2001:db8::1], @user:[2001:db8::1]:8448
- Invalid: missing closing bracket, empty host inside brackets
Also applies to: 30-34
ee/packages/federation-matrix/src/events/member.ts (2)
60-91: Loop-free local rename handling looks good; minor resiliency nits.Solid approach avoiding afterSave loop and broadcasting directly. Consider:
- Wrap Users.updateOne with projection of name conflicts (rare) and log the old value from DB for traceability.
- Guard against over-notifying if Matrix sends redundant events (debounce by comparing current DB name to new value before write).
103-147: Non-local rename detection is correct; add IPv6-safe parsing dependency.Works as intended when prev_content.membership === 'join'. Ensure getUsernameServername remains IPv6-safe (see validator fix), otherwise state_key parsing could mis-identify server names.
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
721-726: Prefer indexed helper for subscription lookup.Use existing helper to leverage indexes and avoid magic field paths.
- const subscription = await Subscriptions.findOne({ 'rid': room._id, 'u._id': user._id }); + const subscription = await Subscriptions.findOneByRoomIdAndUserId(room._id, user._id);
959-1013: updateUserProfile: good shape; add small hardening.
- Early-return on identical current name to reduce Matrix calls.
- Consider batching per-room updates (if SDK supports) to reduce round-trips on large accounts.
ee/packages/federation-matrix/src/helpers/matrixId.ts (1)
10-12: Address CodeQL regex warning by trimming without regex.Replace the underscore-trim regex with index-based slicing to avoid potential backtracking flags.
- sanitized = sanitized.replace(/^_+|_+$/g, ''); + // trim leading/trailing underscores without regex (avoids backtracking warnings) + let start = 0; + while (start < sanitized.length && sanitized.charCodeAt(start) === 95 /* '_' */) start++; + let end = sanitized.length; + while (end > start && sanitized.charCodeAt(end - 1) === 95 /* '_' */) end--; + sanitized = sanitized.slice(start, end);
📜 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 (10)
apps/meteor/app/lib/server/functions/setRealName.ts(4 hunks)apps/meteor/ee/server/hooks/federation/index.ts(2 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(20 hunks)ee/packages/federation-matrix/src/events/member.ts(3 hunks)ee/packages/federation-matrix/src/helpers/matrixId.spec.ts(1 hunks)ee/packages/federation-matrix/src/helpers/matrixId.ts(1 hunks)packages/core-services/src/types/IFederationMatrixService.ts(1 hunks)packages/core-typings/src/IMessage/IMessage.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)packages/message-types/src/registrations/common.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/ee/server/hooks/federation/index.ts
🧬 Code graph analysis (7)
ee/packages/federation-matrix/src/helpers/matrixId.ts (1)
packages/core-typings/src/IUser.ts (2)
IUser(186-255)isUserNativeFederated(276-277)
ee/packages/federation-matrix/src/events/member.ts (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
getUsernameServername(41-53)createOrUpdateFederatedUser(63-106)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
ee/packages/federation-matrix/src/helpers/matrixId.ts (3)
validateFederatedUsername(33-65)constructMatrixId(24-31)getUserMatrixId(67-83)packages/core-typings/src/IUser.ts (1)
isUserNativeFederated(276-277)
ee/packages/federation-matrix/src/helpers/matrixId.spec.ts (2)
ee/packages/federation-matrix/src/helpers/matrixId.ts (3)
validateFederatedUsername(33-65)constructMatrixId(24-31)getUserMatrixId(67-83)packages/core-typings/src/IUser.ts (1)
IUser(186-255)
packages/message-types/src/registrations/common.ts (2)
packages/message-types/src/registrations/e2ee.ts (1)
instance(3-21)packages/message-types/src/registrations/omnichannel.ts (1)
instance(5-63)
apps/meteor/app/lib/server/functions/setRealName.ts (1)
apps/meteor/lib/callbacks.ts (1)
callbacks(252-260)
apps/meteor/ee/server/hooks/federation/index.ts (3)
apps/meteor/lib/callbacks.ts (1)
callbacks(252-260)packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(124-125)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(110-1031)
🪛 GitHub Actions: CI
ee/packages/federation-matrix/src/FederationMatrix.ts
[error] 213-213: TS2554: Expected 3 arguments, but got 4. TypeScript compile error in FederationMatrix.ts: function call with 4 args when 3 expected.
🪛 GitHub Check: CodeQL
ee/packages/federation-matrix/src/helpers/matrixId.ts
[failure] 11-11: Polynomial regular expression used on uncontrolled data
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of '_'.
🔇 Additional comments (6)
packages/i18n/src/locales/en.i18n.json (1)
5550-5550: JSON breakage: unescaped quotes around placeholders (and add trailing comma).The string includes raw " characters around {{old_name}} and {{new_name}}, which invalidates JSON. Also ensure the entry ends with a comma to keep the object valid.
Apply this fix:
- "User_changed_display_name": "changed display name from "{{old_name}}" to "{{new_name}}"", + "User_changed_display_name": "changed display name from {{old_name}} to {{new_name}}",Optional: for consistency with existing wording like “Room name changed to …”, consider “changed name from {{old_name}} to {{new_name}}”.
Likely an incorrect or invalid review comment.
packages/core-typings/src/IMessage/IMessage.ts (1)
92-92: Type addition looks good.The new message type value integrates cleanly with MessageTypesValues.
apps/meteor/ee/server/hooks/federation/index.ts (1)
219-264: Remove JSON serialization suggestion; remote-origin guard pattern is incorrect.The review comment contains two critical incompatibilities:
JSON serialization breaks message rendering: The parser in
packages/message-types/src/registrations/common.ts(line 123–125) hardcodes pipe-splitting:message.msg.split('|')[0]andmessage.msg.split('|')[1]. Existing producers inee/packages/federation-matrix/src/events/member.ts(lines 85, 140) and current PR code (line 255) all use pipe format. Changing only the PR code to JSON would cause the parser to fail and display garbled messages. A coordinated format change across all three producers AND the parser would be required—this is not a localized improvement.Remote-origin guard pattern is incorrect: The suggestion to check
user.username?.includes(':')does not reliably identify federated users. Federated users store their identity in thefederationobject (federation.origin,federation.mui), not in the username field. The correct guard would beif (user.federation?.origin)to skip remote-origin updates, not a username string match.The review comment should be disregarded as written.
Likely an incorrect or invalid review comment.
ee/packages/federation-matrix/src/helpers/matrixId.spec.ts (1)
61-66: Align validator with constructor for '=' handling.constructMatrixId preserves '=', but validateFederatedUsername may reject MXIDs containing raw '='. Add a positive test (e.g., @user=123:example.com) and update the validator to accept it, or sanitize to encode '=' consistently. See proposed validator fix in matrixId.ts review.
Also applies to: 73-76
ee/packages/federation-matrix/src/helpers/matrixId.ts (1)
24-31: Ensure helpers accept what we construct.Given constructMatrixId preserves '=', validateFederatedUsername must not reject such MXIDs; and getUserMatrixId should keep returning mui for native federated users even if their RC username changes (tests already cover). No code change needed here after the validator fix; just re-run tests.
Also applies to: 67-83
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
312-320: Verify invite.inviteUserToRoom signature before passing extra flags.You’re passing five args (invitee, roomId, inviter, boolean, displayName). If the SDK expects three, this will also fail at runtime/compile-time. Align with SDK or stage a typed overload.
Also applies to: 590-597
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ee/packages/federation-matrix/src/helpers/matrixId.ts (1)
33-65: IPv6 and '=' handling issues remain unresolved.The issues previously identified in the past review comment are still present:
IPv6 parsing bug (line 46): Splitting domain and port on ':' breaks IPv6 addresses like
@user:[2001:db8::1]:8448. The host must be extracted first (detecting bracketed IPv6[...]before parsing the port).Localpart inconsistency (line 43): The localpart regex requires encoded form
=[0-9a-fA-F]{2}butsanitizeForMatrixLocalpart(line 10) preserves raw '=' characters. This meansvalidateFederatedUsernamewill reject MXIDs created byconstructMatrixId.Please refer to the previous review comment for the detailed fix. These are critical correctness issues that will cause valid Matrix IDs to be rejected.
📜 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 (1)
ee/packages/federation-matrix/src/helpers/matrixId.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/helpers/matrixId.ts (1)
packages/core-typings/src/IUser.ts (2)
IUser(186-255)isUserNativeFederated(276-277)
🪛 GitHub Check: CodeQL
ee/packages/federation-matrix/src/helpers/matrixId.ts
[failure] 11-11: Polynomial regular expression used on uncontrolled data
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of ''.
This regular expression that depends on library input may run slow on strings with many repetitions of '_'.
🔇 Additional comments (1)
ee/packages/federation-matrix/src/helpers/matrixId.ts (1)
67-80: LGTM! Clean user Matrix ID resolution.The logic correctly handles both native federated users (returning their existing
mui) and local users (constructing a new Matrix ID). Error handling is appropriate with clear messages for missing required fields.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ee/packages/federation-matrix/src/helpers/matrixId.ts (1)
10-11: ReDoS vulnerability remains unaddressed.The polynomial regex
/_+$/flagged by CodeQL and previous reviews has not been mitigated. This pattern can cause catastrophic backtracking on strings with many consecutive underscores.
📜 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)
ee/packages/federation-matrix/src/helpers/matrixId.spec.ts(1 hunks)ee/packages/federation-matrix/src/helpers/matrixId.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/packages/federation-matrix/src/helpers/matrixId.spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/helpers/matrixId.ts (1)
packages/core-typings/src/IUser.ts (2)
IUser(186-255)isUserNativeFederated(276-277)
⏰ 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). (3)
- GitHub Check: ✅ Tests Done
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
ee/packages/federation-matrix/src/helpers/matrixId.ts (3)
24-65: LGTM!The server name validation correctly handles IPv6 (with brackets), IPv4, hostnames, and optional ports. The logic properly distinguishes between IPv6's colons and port separators.
67-108: Thorough validation added as requested.The function now validates serverName format comprehensively, including whitespace, port ranges, and overall structure. The port validation duplication (lines 82-100 and within validateServerNameFormat) is acceptable for providing clearer error messages.
126-139: LGTM!The function correctly distinguishes native federated users (returning existing
mui) from local users (constructing Matrix ID), with appropriate error handling for missing fields.
ricardogarim
force-pushed
the
feat/federation-rename-user
branch
from
ec28f38 to
def9191
Compare
As per FDR-169, this PR adds the ability to send and receive user name changes in federated rooms.
Depends on RocketChat/homeserver#291 and RocketChat/homeserver#298.
Screen.Recording.2025-10-29.at.00.53.49.mov
Summary by CodeRabbit:
Summary by CodeRabbit
New Features
Refactoring