fix(federation): prevent inviting federated users to non-federated rooms via slash command#37169
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 |
|
WalkthroughIntroduces sanitizeUsername for consistent username normalization. Enforces federated constraints when inviting/adding users: blocks native-federated users from non-federated rooms and surfaces a specific error. Updates /invite flow to use sanitized usernames and Users.findByUsernames. Changes addUserToRoom to require user objects (id-based), removing string-username paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Client User
participant SC as /invite Command
participant SVC as addUsersToRoom (method)
participant Users as Users Collection
participant Subs as Subscriptions
participant Rooms as Rooms Service
User->>SC: /invite @alice @remote:bunny
SC->>SC: sanitizeUsername(list)
SC->>Users: findByUsernames(usernames)
Users-->>SC: user docs
SC->>SVC: addUsersToRoom(roomId, users[])
SVC->>SVC: sanitizeUsername(each)
SVC->>Rooms: get room + federation flags
alt user is native federated AND room not federated
SVC-->>SC: throw error-federated-users-in-non-federated-rooms
SC-->>User: localized error (federated not allowed)
else proceed
SVC->>Subs: findOneByRoomIdAndUserId
alt subscription exists
SVC-->>User: ephemeral "already in room"
else no subscription
SVC->>Rooms: addUserToRoom(roomId, user{_id, username})
Rooms-->>SVC: success
SVC-->>SC: success
SC-->>User: invite success
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
ricardogarim
force-pushed
the
fix/federation-slash-invite
branch
from
4766f0d to
3c9f75e
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/slashcommands-invite/server/server.ts (1)
11-25: Broaden federated username detection
isFederatedUsername()only returns true when a username contains both@and:, so it misses handles like@federated.user@remote.serverfrom the test plan. Those keep only the leading@stripped, soUsers.findByUsernamesends up searching forfederated.user@remote.server. If the database still stores those users including the leading@, the lookup will now fail and/invitewill always return “User doesn’t exist” for precisely the case we’re trying to handle. Please extend the check to also treat usernames containing a second@(after the first character) as federated—for example by testingusername.includes(':') || username.indexOf('@', 1) !== -1—so both federation formats keep their exact value.
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
100-104: Remove unreachable branchAfter the
if (!newUser) { throw … }guard, the subsequentif (!newUser && !isUserNativeFederated(newUser))can never execute. Keeping it around is confusing and suggests we might still be callingisUserNativeFederatedonundefined. Please drop the dead branch (or restore the original string-based check if you still need a fallback) to keep the flow clear.
📜 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/methods/addUsersToRoom.ts(2 hunks)apps/meteor/app/slashcommands-invite/server/server.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (2)
packages/core-typings/src/IUser.ts (1)
isUserNativeFederated(275-276)packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(124-125)
⏰ 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). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
96-100: Remove unreachable dead code.Lines 96-100 are unreachable because line 84 already throws
error-user-not-foundwhen!newUser. Additionally, the condition has a logic error:!newUser && !isUserNativeFederated(newUser)would call the type guard on null/undefined if the first part were true.Apply this diff to remove the dead code:
if (isUserNativeFederated(newUser) && !isRoomNativeFederated(room)) { throw new Meteor.Error('error-federated-users-in-non-federated-rooms', 'Cannot add federated users to non-federated rooms', { method: 'addUsersToRoom', }); } - if (!newUser && !isUserNativeFederated(newUser)) { - throw new Meteor.Error('error-invalid-username', 'Invalid username', { - method: 'addUsersToRoom', - }); - } const subscription = newUser && (await Subscriptions.findOneByRoomIdAndUserId(data.rid, newUser._id));
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
103-103: Simplify fallback logic.Since
newUseris guaranteed to exist after line 84, the fallbacknewUser || usernamecan be simplified to justnewUser.Apply this diff:
- await addUserToRoom(data.rid, newUser || username, user); + await addUserToRoom(data.rid, newUser, user);
📜 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/methods/addUsersToRoom.ts(2 hunks)apps/meteor/app/slashcommands-invite/server/server.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/slashcommands-invite/server/server.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (2)
packages/core-typings/src/IUser.ts (1)
isUserNativeFederated(275-276)packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(124-125)
⏰ 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 (1)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
84-94: LGTM! Clean validation flow for federation constraints.The explicit user-existence check followed by the federated-user/non-federated-room guard correctly implements the PR objective. The error messages are clear and the logic is sound.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37169 +/- ##
==================================================
- Coverage 67.46% 67.44% -0.03%
==================================================
Files 3329 3330 +1
Lines 113381 113387 +6
Branches 20579 20580 +1
==================================================
- Hits 76490 76470 -20
- Misses 34291 34318 +27
+ Partials 2600 2599 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| }); | ||
| } | ||
|
|
||
| if (!newUser && !isUserNativeFederated(newUser)) { |
There was a problem hiding this comment.
Why are you validating isUserNativeFederated if the user is undefined?
This whole if is not necessary!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (2)
95-95: Variable shadowing reduces clarity.The
subscriptionvariable declared here shadows the earlier declaration at line 42. While functionally correct (they serve different purposes: checking invoker's membership vs. target user's membership), this reduces code clarity.Consider renaming for clarity:
- const subscription = await Subscriptions.findOneByRoomIdAndUserId(data.rid, newUser._id); - if (!subscription) { + const targetUserSubscription = await Subscriptions.findOneByRoomIdAndUserId(data.rid, newUser._id); + if (!targetUserSubscription) {
97-97: Remove unreachable fallback.The
|| usernamefallback is now unreachable since line 84 throws an error whennewUseris falsy. This guarantees thatnewUseris defined at this point.Simplify by removing the dead code:
- await addUserToRoom(data.rid, newUser || username, user); + await addUserToRoom(data.rid, newUser, user);
📜 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)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (2)
packages/core-typings/src/IUser.ts (1)
isUserNativeFederated(276-277)packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(124-125)
⏰ 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 (2)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (2)
3-3: LGTM! Appropriate imports for federation validation.The imported type guards are correctly used to validate federation constraints in the user addition flow.
83-93: Core fix correctly implements federation constraint.The user existence check (lines 83-87) followed by the federation validation (lines 89-93) properly addresses the PR objective to prevent federated users from being added to non-federated rooms. The ordering of checks also resolves the past review concern about validating federation when the user might be undefined.
The logic correctly:
- Ensures the user exists locally before proceeding
- Validates that federated users cannot be added to non-federated rooms
- Uses appropriate type guards for runtime checking
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
21-28: Add input validation and documentation tosanitizeUsername.The function has several areas for improvement:
- Missing input validation: The function doesn't validate the input. If
usernameisundefined,null, or an empty string, it will throw a runtime error or return an invalid value.- No documentation: Add JSDoc comments to explain the function's purpose, parameters, return value, and the federated username format it expects.
- Potential edge case: After sanitization, the username could be an empty string (e.g., if the input is just
"@"). Consider adding a check and throwing an error for empty results.- Heuristic limitation: The federated username detection (
includes('@') && includes(':')) is a heuristic that might not cover all edge cases (e.g., a username like"user:name@test"would be treated as federated). While this is probably acceptable, consider documenting this behavior.Apply this diff to add input validation and documentation:
+/** + * Sanitizes a username by removing leading '@' or ' @' prefix. + * Federated usernames (containing both '@' and ':') are returned unchanged. + * + * @param username - The username to sanitize + * @returns The sanitized username + * @throws {Error} If username is empty or invalid + * + * @example + * sanitizeUsername('@john') // returns 'john' + * sanitizeUsername(' @john') // returns 'john' + * sanitizeUsername('@user:server@domain.com') // returns '@user:server@domain.com' (federated) + */ export const sanitizeUsername = (username: string) => { + if (!username || typeof username !== 'string') { + throw new Error('Invalid username: must be a non-empty string'); + } + const isFederatedUsername = username.includes('@') && username.includes(':'); if (isFederatedUsername) { return username; } - return username.replace(/(^@)|( @)/, ''); + const sanitized = username.replace(/(^@)|( @)/, ''); + if (!sanitized) { + throw new Error('Invalid username: resulted in empty string after sanitization'); + } + return sanitized; };
📜 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/methods/addUsersToRoom.ts(3 hunks)apps/meteor/app/slashcommands-invite/server/server.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/slashcommands-invite/server/server.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (3)
packages/core-typings/src/IUser.ts (1)
isUserNativeFederated(276-277)packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(124-125)apps/meteor/app/lib/server/functions/addUserToRoom.ts (1)
addUserToRoom(22-160)
⏰ 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)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (5)
3-3: LGTM!The federation type guard imports are correctly added and align with the PR objective to validate federated users and rooms.
91-96: LGTM! Addresses past review feedback.The code now properly validates that the user exists before proceeding with federation checks, directly addressing rodrigok's previous comment about validating
isUserNativeFederatedon an undefined user. The error handling is clear and appropriate.
98-102: LGTM! Core fix for FDR-214.The federation validation correctly prevents federated users from being invited to non-federated rooms using the appropriate type guards. The error handling is clear and descriptive.
104-104: LGTM! Proper async handling.The subscription lookup is now correctly awaited, ensuring the subscription is resolved before checking whether the user is already in the room. This prevents potential race conditions.
106-106: LGTM! Improved type safety.The change to pass the
newUserobject instead of a username string aligns with theaddUserToRoomfunction signature and improves type safety. Based on the relevant code snippets,addUserToRoomexpects aPick<IUser, '_id' | 'username'>, whichnewUsersatisfies.
ricardogarim
force-pushed
the
fix/federation-slash-invite
branch
from
12095db to
8e5169c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
21-28: Consider more robust federated username detection.The current check (
username.includes('@') && username.includes(':')) is simplistic and may have false positives. For example,"user@email.com:123"would be treated as federated even if it's not a valid federated username format.Consider verifying the expected format (e.g.,
@username@domain:port) if the federated username specification is well-defined, or document why the current conservative approach is preferred.
📜 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 (3)
apps/meteor/app/lib/server/functions/addUserToRoom.ts(2 hunks)apps/meteor/app/lib/server/methods/addUsersToRoom.ts(3 hunks)apps/meteor/app/slashcommands-invite/server/server.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/lib/server/functions/addUserToRoom.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/app/slashcommands-invite/server/server.ts (1)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
sanitizeUsername(21-28)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (3)
packages/core-typings/src/IUser.ts (1)
isUserNativeFederated(276-277)packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(124-125)apps/meteor/app/lib/server/functions/addUserToRoom.ts (1)
addUserToRoom(22-160)
⏰ 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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (9)
apps/meteor/app/slashcommands-invite/server/server.ts (4)
7-7: LGTM!The import of
sanitizeUsernameis appropriate for the username normalization logic used at line 20.
20-20: LGTM!Using
sanitizeUsernameensures consistent username normalization across the codebase, handling both federated and non-federated usernames appropriately.
81-85: LGTM!The new error handling for federated users in non-federated rooms is correctly implemented. The error code matches the one thrown in
addUsersToRoom.ts, and the appropriate localized message is displayed to the user.
25-25: Confirm exact-match behavior of findByUsernames and input sanitation
findByUsernamesissues{ username: { $in: usernames } }without normalization; ensure theusernamesarray here is already sanitized to the exact stored (federated) username format, or switch tofindByUsernamesIgnoringCasefor case-insensitive lookup.apps/meteor/app/lib/server/methods/addUsersToRoom.ts (5)
3-3: LGTM!The imports of
isRoomNativeFederatedandisUserNativeFederatedare necessary for the federation validation logic introduced in this PR.
91-96: LGTM!The explicit error handling for missing users is a good improvement over silently continuing. This also addresses the past review comment by ensuring the user is defined before the federation check.
98-102: LGTM!The federation validation correctly prevents native federated users from being added to non-federated rooms, which is the core objective of this PR (FDR-214). The error code matches the handler in the slash command file.
104-104: LGTM!The subscription check correctly verifies if the user is already in the room before attempting to add them.
106-106: LGTM!Passing the
newUserobject instead of a username string aligns with the updatedaddUserToRoomfunction signature that requires user objects with_idandusernamefields.
2 participants
Proposed changes (including videos or screenshots)
As per FDR-214, this PR adds a check using
isUserNativeFederatedand!isRoomNativeFederatedto prevent inviting federated users to non-federated rooms.Issue(s)
Steps to test or reproduce
/invite @federated.user@remote.serverVerify that for non-federated rooms, the system displays the message: "You cannot add external users to a non-federated room", which comes from the existing i18n key:
You_cannot_add_external_users_to_non_federated_room.Further comments
Note that this change only addresses the invitation of federated users to non-federated rooms. However, as mentioned in the additional task FDR-223, we should also address in the future the user discovery while typing
/invite, since it currently lists only users that already exist locally (i.e., users who are part of some room and present in the users collection).Summary by CodeRabbit
New Features
Bug Fixes