fix: create federated user on DMs from RC > remote flow #37043
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 |
|
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/federation-backup #37043 +/- ##
===========================================================
+ Coverage 67.30% 69.75% +2.45%
===========================================================
Files 3342 3035 -307
Lines 113648 103432 -10216
Branches 20757 18385 -2372
===========================================================
- Hits 76486 72148 -4338
+ Misses 34554 29404 -5150
+ Partials 2608 1880 -728
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ricardogarim
force-pushed
the
fix/federation-dms
branch
2 times, most recently
from
bb8ada2 to
ecce9ce
Compare
ricardogarim
changed the base branch from
chore/federation-backup
to
use-published-package
ricardogarim
force-pushed
the
fix/federation-dms
branch
3 times, most recently
from
d90d476 to
0bb97a8
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/meteor/app/lib/server/functions/createRoom.ts (1)
166-167: Fix creatorId fallback: use owner._id, not usernamePassing
owner?.usernameascreatorbreaks DM federation and subscription opening. Downstream expects a user ID (see FederationMatrix.createDirectMessageRoom → Users.findOneById and createDirectRoom subscription logic).Apply this diff:
- return createDirectRoom(members as IUser[], extraData, { ...options, creator: options?.creator || owner?.username }); + return createDirectRoom(members as IUser[], extraData, { ...options, creator: options?.creator || owner?._id });ee/packages/federation-matrix/src/FederationMatrix.ts (2)
261-291: Normalize/relax federated username handling, fix origin parsing, and use a supported insert API
- Filter currently requires both “@” and “:”, skipping
user:domain.originviasplit(':')[1]loses ports (e.g.,:8448).Users.createmay not exist on the model; elsewhere this file usesinsertOne.Apply this diff:
- const federatedUsers = usernames.filter((username) => username?.includes(':') && username?.includes('@')); - for await (const username of federatedUsers) { + const federatedUsers = usernames + .filter((u): u is string => typeof u === 'string' && u.includes(':')) + .map((u) => (u.startsWith('@') ? u : `@${u}`)); + for await (const username of federatedUsers) { if (!username) { continue; } const existingUser = await Users.findOneByUsername(username); if (existingUser) { continue; } - await Users.create({ + const origin = username.split(':').slice(1).join(':'); + await Users.insertOne({ username, name: username, type: 'user' as const, status: UserStatus.OFFLINE, active: true, roles: ['user'], requirePasswordChange: false, federated: true, federation: { version: 1, - mui: username, - origin: username.split(':')[1], + mui: username, + origin, }, createdAt: new Date(), _updatedAt: new Date(), });
365-388: Bug: wrong lookup and user creation for remote IUser members
Users.findOne({ 'federation.mui': memberId })compares a Matrix user ID field to a Mongo user ID. This will never match and can lead to inserting bogus users withusername = memberId.Prefer checking by the Matrix ID and using the member’s existing data:
- if (memberId) { - const existingMemberMatrixUserId = await Users.findOne({ 'federation.mui': memberId }); - if (!existingMemberMatrixUserId) { - const newUser = { - username: memberId, - name: memberId, + if (memberId) { + const existingByMui = await Users.findOne({ 'federation.mui': memberMatrixUserId }); + if (!existingByMui) { + const newUser = { + username: typeof member !== 'string' ? member.username ?? memberMatrixUserId : memberMatrixUserId, + name: typeof member !== 'string' ? member.name ?? member.username ?? memberMatrixUserId : memberMatrixUserId, type: 'user' as const, status: UserStatus.OFFLINE, active: true, roles: ['user'], requirePasswordChange: false, federated: true, federation: { version: 1, - mui: memberId, - origin: memberMatrixUserId.split(':').pop(), + mui: memberMatrixUserId, + origin: memberMatrixUserId.split(':').slice(1).join(':'), }, createdAt: new Date(), _updatedAt: new Date(), }; await Users.insertOne(newUser); } }
📜 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 (6)
apps/meteor/app/lib/server/functions/createDirectRoom.ts(3 hunks)apps/meteor/app/lib/server/functions/createRoom.ts(1 hunks)apps/meteor/lib/callbacks.ts(2 hunks)apps/meteor/server/services/room/hooks/BeforeFederationActions.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(2 hunks)packages/core-services/src/types/IFederationMatrixService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
packages/models/src/index.ts (1)
Users(211-211)
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
packages/core-typings/src/IRoom.ts (2)
IRoom(21-95)IRoomNativeFederated(114-120)
apps/meteor/lib/callbacks.ts (1)
packages/core-typings/src/IRoom.ts (2)
IRoomNativeFederated(114-120)IRoom(21-95)
apps/meteor/app/lib/server/functions/createDirectRoom.ts (2)
packages/core-typings/src/IRoom.ts (2)
IRoomNativeFederated(114-120)IRoom(21-95)apps/meteor/lib/callbacks.ts (1)
callbacks(252-260)
⏰ 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 Docker Images for Testing (alpine)
🔇 Additional comments (5)
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
7-17: LGTM: widened input type while preserving runtime checksAccepting
Partial<IRoom | IRoomNativeFederated>matches upstream callers that pass partial room shapes. Behavior unchanged.apps/meteor/lib/callbacks.ts (1)
82-83: LGTM: updated beforeCreateDirectRoom signatureUsing
string[]andPartial<IRoomNativeFederated | IRoom>aligns with the new DM pre-create flow.apps/meteor/app/lib/server/functions/createDirectRoom.ts (2)
45-51: LGTM: widen roomExtraData to Partial<IRoomNativeFederated | IRoom>Matches federation-native room shapes without over-constraining callers.
72-73: Pre-create hook ensures federated users are created/normalizedThe beforeCreateDirectRoom callback in apps/meteor/ee/server/hooks/federation/index.ts calls FederationMatrix.ensureFederatedUsersExistLocally(members) when FederationActions.shouldPerformFederationAction(room) is true.
packages/core-services/src/types/IFederationMatrixService.ts (1)
10-11: LGTM — callers verified: all pass string[]beforeCreateDirectRoom is typed (members: string[]) (apps/meteor/lib/callbacks.ts); createDirectRoom passes membersUsernames (string[]) and the hook (apps/meteor/ee/server/hooks/federation/index.ts) calls ensureFederatedUsersExistLocally(members). No callers pass IUser or mixed arrays.
| const hasFederatedMembers = members.some((member) => { | ||
| if (typeof member === 'string') { | ||
| return member.includes(':') && member.includes('@'); | ||
| } | ||
| return member.username?.includes(':') && member.username?.includes('@'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broaden federated member detection (don’t require '@')
Requiring both “@” and “:” misses valid inputs like user:domain. This can prevent extraData.federated from being set and skip federation handling.
Apply this diff:
- const hasFederatedMembers = members.some((member) => {
- if (typeof member === 'string') {
- return member.includes(':') && member.includes('@');
- }
- return member.username?.includes(':') && member.username?.includes('@');
- });
+ const hasFederatedMembers = members.some((member) => {
+ const username = typeof member === 'string' ? member : member.username;
+ return typeof username === 'string' && username.includes(':');
+ });📝 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.
| const hasFederatedMembers = members.some((member) => { | |
| if (typeof member === 'string') { | |
| return member.includes(':') && member.includes('@'); | |
| } | |
| return member.username?.includes(':') && member.username?.includes('@'); | |
| }); | |
| const hasFederatedMembers = members.some((member) => { | |
| const username = typeof member === 'string' ? member : member.username; | |
| return typeof username === 'string' && username.includes(':'); | |
| }); |
🤖 Prompt for AI Agents
In apps/meteor/app/lib/server/functions/createRoom.ts around lines 137 to 143,
the federated-member detection currently requires both ':' and '@', which misses
valid forms like "user:domain"; change the predicate to detect federation if the
string (or member.username) contains ':' OR contains '@' (use || instead of &&)
so hasFederatedMembers returns true for inputs like "user:domain" as well as
"@user:domain" or "user@domain".
…is created before room setup
ricardogarim
force-pushed
the
fix/federation-dms
branch
from
0bb97a8 to
aa1454a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
7-7: Good use of type guard but consider parameter namingThe type guard pattern is excellent for type safety. However, the parameter type
Partial<IRoom | IRoomNativeFederated>is unusual - typically we'd seePartial<IRoom> | Partial<IRoomNativeFederated>.If the current union is intentional, consider documenting why. Otherwise:
-public static shouldPerformFederationAction(room: Partial<IRoom | IRoomNativeFederated>): room is IRoomNativeFederated { +public static shouldPerformFederationAction(room: Partial<IRoom> | Partial<IRoomNativeFederated>): room is IRoomNativeFederated {
📜 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 (6)
apps/meteor/app/lib/server/functions/createDirectRoom.ts(3 hunks)apps/meteor/app/lib/server/functions/createRoom.ts(1 hunks)apps/meteor/lib/callbacks.ts(2 hunks)apps/meteor/server/services/room/hooks/BeforeFederationActions.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(2 hunks)packages/core-services/src/types/IFederationMatrixService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/app/lib/server/functions/createRoom.ts
- packages/core-services/src/types/IFederationMatrixService.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
packages/core-typings/src/IRoom.ts (2)
IRoom(21-95)IRoomNativeFederated(114-120)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
packages/models/src/index.ts (1)
Users(211-211)
apps/meteor/app/lib/server/functions/createDirectRoom.ts (2)
packages/core-typings/src/IRoom.ts (2)
IRoomNativeFederated(114-120)IRoom(21-95)apps/meteor/lib/callbacks.ts (1)
callbacks(252-260)
apps/meteor/lib/callbacks.ts (1)
packages/core-typings/src/IRoom.ts (2)
IRoomNativeFederated(114-120)IRoom(21-95)
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
apps/meteor/lib/callbacks.ts (2)
24-24: LGTM! Type import added correctlyThe import of
IRoomNativeFederatedis correctly added to support the updated callback signature.
82-82: Verify external callback handlers No internal registrations ofbeforeCreateDirectRoomwere found—only its invocation at apps/meteor/app/lib/server/functions/createDirectRoom.ts:72. Ensure any plugin or app-level handlers for this callback are updated to the new signature(members: string[], room: Partial<IRoomNativeFederated | IRoom>)instead of(IUser[], IRoom).apps/meteor/app/lib/server/functions/createDirectRoom.ts (3)
4-4: LGTM! Type imports updated correctlyThe addition of
IRoomNativeFederatedtype import aligns with the federation flow changes.
45-45: Type safety improvement for federated roomsGood change to explicitly support federated room metadata in the
roomExtraDataparameter type.
72-73: Ignore callback placement concern beforeCreateDirectRoom is defined to receive usernames (string[]), and the only registered implementation (federation hook) uses only usernames without accessing user properties. No break introduced.Likely an incorrect or invalid review comment.
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
274-290: Simplified user creation logic looks goodThe refactored code is cleaner and more straightforward. The federation metadata is now derived directly from the username which is appropriate.
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
8-16: Logic is correct and handles all cases appropriatelyThe method correctly:
- Returns false for non-federated rooms
- Throws an error for federated but non-native rooms
- Returns true (with type narrowing) for native federated rooms
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/lib/callbacks.ts (1)
81-81: PreferUsername[]over rawstring[].We already import
Username, and the callback now receives usernames. Keeping theUsername[]typing preserves intent and avoids eroding type-safety downstream.- 'beforeCreateDirectRoom': (members: string[], room: IRoom) => void; + 'beforeCreateDirectRoom': (members: Username[], room: IRoom) => void;
📜 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/createDirectRoom.ts(2 hunks)apps/meteor/lib/callbacks.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/lib/server/functions/createDirectRoom.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/lib/callbacks.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes