feat(federation): validate user email domain #38356
Open
+647
−18
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
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 |
|
Contributor
WalkthroughReplaces direct Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (Room/Invite/Hook)
participant FederationMatrix as FederationMatrix
participant Authorization as Authorization Service
participant Settings as Settings Store
Caller->>FederationMatrix: canUserAccessFederation(user)
FederationMatrix->>Authorization: hasPermission(user._id, 'access-federation')
Authorization-->>FederationMatrix: permissionResult
alt permissionResult = false
FederationMatrix-->>Caller: false
else permissionResult = true
FederationMatrix->>Settings: read Federation_Service_Validate_User_Domain
Settings-->>FederationMatrix: flagValue
alt flagValue = true
FederationMatrix->>FederationMatrix: check user's verified email & domain == serverName
alt email verified && domain matches
FederationMatrix-->>Caller: true
else
FederationMatrix-->>Caller: false
end
else flagValue = false
FederationMatrix-->>Caller: true
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38356 +/- ##
===========================================
+ Coverage 70.84% 70.87% +0.02%
===========================================
Files 3160 3160
Lines 109768 109769 +1
Branches 19702 19685 -17
===========================================
+ Hits 77770 77794 +24
+ Misses 29968 29953 -15
+ Partials 2030 2022 -8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/28 18:16 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 28 Jan 2026 18:16:13 GMT |
sampaiodiego
force-pushed
the
federation-validate-user-domain
branch
2 times, most recently
from
fc57eb4 to
30988f5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/meteor/server/services/room/service.ts`:
- Line 152: RoomService.join expects a full IUser because
canUserAccessFederation reads user.emails; fix the two call sites so they pass a
complete user object: in the slashcommands-join caller (server.ts) include
emails in the Mongo projection when fetching the user (add "emails" to the
projection before calling RoomService.join), and in
getRoomByNameOrIdWithOptionToJoin adjust the parameter type (or the caller) so
it returns/passes a full IUser rather than Pick<IUser, '_id' | 'username' |
'federated' | 'federation'>; reference RoomService.join and
canUserAccessFederation when updating types/fetch logic to ensure user.emails is
present.
In `@ee/packages/federation-matrix/src/FederationMatrix.ts`:
- Around line 820-824: The domain comparison in the user email check is
case-sensitive; update the logic in the block using user.emails (the anonymous
callback that does email.address.split('@')[1]) to normalize both sides to the
same case before comparing: extract the domain safely from email.address, call
.toLowerCase() (or .toLocaleLowerCase()) on the extracted domain and on
this.serverName, then compare those lowercased values and keep the existing
email.verified requirement; ensure the code handles malformed addresses (no '@')
gracefully when extracting the domain.
In `@ee/packages/federation-matrix/tests/end-to-end/permissions.spec.ts`:
- Around line 711-713: The test is incorrectly checking subscription.name
against the invited user's username; change the lookup to match the room name
instead. In the getSubscriptions result (subscriptions.update) replace the
predicate used to find invitedSub (currently (sub) => sub.name ===
userWithNonMatchingEmail.username) with a check against the channel name, e.g.
(sub) => sub.fname?.includes(channelName) or (sub) => sub.name === channelName
so the test verifies the room subscription was rejected rather than relying on
username mismatch.
| * Method called by users to join a room. | ||
| */ | ||
| async join({ room, user, joinCode }: { room: IRoom; user: Pick<IUser, '_id' | 'federated' | 'federation'>; joinCode?: string }) { | ||
| async join({ room, user, joinCode }: { room: IRoom; user: IUser; joinCode?: string }) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the RoomService.join method to verify the current signature
find apps/meteor -name "*.ts" -type f | grep -i room | grep -i serviceRepository: RocketChat/Rocket.Chat
Length of output: 622
🏁 Script executed:
# Let's check the exact file and method signature
cat -n apps/meteor/server/services/room/service.ts | head -200 | tail -100Repository: RocketChat/Rocket.Chat
Length of output: 4100
🏁 Script executed:
# Search for the join method definition and surrounding code
rg -nP --type=ts "async join\s*\(" apps/meteor/server/services/room/service.ts -A 10 -B 2Repository: RocketChat/Rocket.Chat
Length of output: 743
🏁 Script executed:
# Find all calls to .join( method in the codebase
rg -nP --type=ts "\.join\s*\(\s*\{" apps/meteor ee/packages | grep -v "node_modules" | head -50Repository: RocketChat/Rocket.Chat
Length of output: 595
🏁 Script executed:
# Find canUserAccessFederation method definition
rg -nP --type=ts "canUserAccessFederation" apps/meteor ee/packages -B 2 -A 10 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 5921
🏁 Script executed:
# Check all the call sites in detail - start with joinRoom.ts
cat -n apps/meteor/app/lib/server/methods/joinRoom.ts | head -50Repository: RocketChat/Rocket.Chat
Length of output: 1253
🏁 Script executed:
# Check the channels.ts call site
cat -n apps/meteor/app/api/server/v1/channels.ts | sed -n '220,240p'Repository: RocketChat/Rocket.Chat
Length of output: 646
🏁 Script executed:
# Check the slashcommands-join call site
cat -n apps/meteor/app/slashcommands-join/server/server.ts | sed -n '40,60p'Repository: RocketChat/Rocket.Chat
Length of output: 729
🏁 Script executed:
# Check getRoomByNameOrIdWithOptionToJoin.ts
cat -n apps/meteor/app/lib/server/functions/getRoomByNameOrIdWithOptionToJoin.ts | sed -n '80,95p'Repository: RocketChat/Rocket.Chat
Length of output: 542
🏁 Script executed:
# Check the function signature
rg -nP --type=ts "export.*getRoomByNameOrIdWithOptionToJoin" apps/meteor -B 5 -A 15 | head -40Repository: RocketChat/Rocket.Chat
Length of output: 2276
🏁 Script executed:
# Get more context on where canUserAccessFederation accesses emails
cat -n ee/packages/federation-matrix/src/FederationMatrix.ts | sed -n '811,830p'Repository: RocketChat/Rocket.Chat
Length of output: 577
🏁 Script executed:
# Verify what properties the slashcommands-join actually fetches for user
cat -n apps/meteor/app/slashcommands-join/server/server.ts | sed -n '40,55p'Repository: RocketChat/Rocket.Chat
Length of output: 642
🏁 Script executed:
# Check if there are any other calls to canUserAccessFederation and what user objects they pass
rg -nP --type=ts "canUserAccessFederation" apps/meteor ee/packages -B 5 | grep -E "(const user|user:)" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 187
🏁 Script executed:
# Let's look for git history or type definition changes - check current IUser type
rg -nP --type=ts "interface IUser" apps/meteor ee/packages | head -5Repository: RocketChat/Rocket.Chat
Length of output: 311
Fix caller types to pass complete user objects to RoomService.join.
The user parameter expects full IUser type for canUserAccessFederation to access user.emails for domain validation. Two call sites have issues:
-
apps/meteor/app/slashcommands-join/server/server.ts:46— fetches user with only{ federated: 1, federation: 1 }projection, excludingemails. Includeemailsin the projection before passing toRoom.join. -
apps/meteor/app/lib/server/functions/getRoomByNameOrIdWithOptionToJoin.ts:17— function signature acceptsPick<IUser, '_id' | 'username' | 'federated' | 'federation'>but passes toRoom.joinexpecting fullIUser. Update the parameter type or ensure callers pass complete user objects.
🤖 Prompt for AI Agents
In `@apps/meteor/server/services/room/service.ts` at line 152, RoomService.join
expects a full IUser because canUserAccessFederation reads user.emails; fix the
two call sites so they pass a complete user object: in the slashcommands-join
caller (server.ts) include emails in the Mongo projection when fetching the user
(add "emails" to the projection before calling RoomService.join), and in
getRoomByNameOrIdWithOptionToJoin adjust the parameter type (or the caller) so
it returns/passes a full IUser rather than Pick<IUser, '_id' | 'username' |
'federated' | 'federation'>; reference RoomService.join and
canUserAccessFederation when updating types/fetch logic to ensure user.emails is
present.
Comment on lines
+820
to
+824
| return ( | ||
| user.emails?.some((email) => { | ||
| const domain = email.address.split('@')[1]; | ||
| return domain === this.serverName && email.verified; | ||
| }) ?? false |
Contributor
There was a problem hiding this comment.
Email domain comparison should be case-insensitive.
Per RFC 5321, domain names in email addresses are case-insensitive. If a user's email was stored as [email protected] but serverName is example.com, they would incorrectly fail validation.
🔧 Suggested fix
return (
user.emails?.some((email) => {
- const domain = email.address.split('@')[1];
- return domain === this.serverName && email.verified;
+ const domain = email.address.split('@')[1]?.toLowerCase();
+ return domain === this.serverName.toLowerCase() && email.verified;
}) ?? false
);📝 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
| return ( | |
| user.emails?.some((email) => { | |
| const domain = email.address.split('@')[1]; | |
| return domain === this.serverName && email.verified; | |
| }) ?? false | |
| return ( | |
| user.emails?.some((email) => { | |
| const domain = email.address.split('@')[1]?.toLowerCase(); | |
| return domain === this.serverName.toLowerCase() && email.verified; | |
| }) ?? false |
🤖 Prompt for AI Agents
In `@ee/packages/federation-matrix/src/FederationMatrix.ts` around lines 820 -
824, The domain comparison in the user email check is case-sensitive; update the
logic in the block using user.emails (the anonymous callback that does
email.address.split('@')[1]) to normalize both sides to the same case before
comparing: extract the domain safely from email.address, call .toLowerCase() (or
.toLocaleLowerCase()) on the extracted domain and on this.serverName, then
compare those lowercased values and keep the existing email.verified
requirement; ensure the code handles malformed addresses (no '@') gracefully
when extracting the domain.
Comment on lines
+711
to
+790
| const subscriptions = await getSubscriptions(userWithNonMatchingEmailRequestConfig); | ||
| const invitedSub = subscriptions.update.find((sub) => sub.name === userWithNonMatchingEmail.username); | ||
| expect(invitedSub).toBeUndefined(); |
Contributor
There was a problem hiding this comment.
Incorrect subscription lookup - should check by room name, not username.
The subscription's name property contains the room name, not the user's username. This assertion passes only because no subscription has name === username, not because the room subscription was properly rejected. Compare with line 107 which correctly uses sub.fname?.includes(channelName).
🔧 Suggested fix
const subscriptions = await getSubscriptions(userWithNonMatchingEmailRequestConfig);
-const invitedSub = subscriptions.update.find((sub) => sub.name === userWithNonMatchingEmail.username);
+const invitedSub = subscriptions.update.find((sub) => sub.fname?.includes(channelName));
expect(invitedSub).toBeUndefined();📝 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
| const subscriptions = await getSubscriptions(userWithNonMatchingEmailRequestConfig); | |
| const invitedSub = subscriptions.update.find((sub) => sub.name === userWithNonMatchingEmail.username); | |
| expect(invitedSub).toBeUndefined(); | |
| const subscriptions = await getSubscriptions(userWithNonMatchingEmailRequestConfig); | |
| const invitedSub = subscriptions.update.find((sub) => sub.fname?.includes(channelName)); | |
| expect(invitedSub).toBeUndefined(); |
🤖 Prompt for AI Agents
In `@ee/packages/federation-matrix/tests/end-to-end/permissions.spec.ts` around
lines 711 - 713, The test is incorrectly checking subscription.name against the
invited user's username; change the lookup to match the room name instead. In
the getSubscriptions result (subscriptions.update) replace the predicate used to
find invitedSub (currently (sub) => sub.name ===
userWithNonMatchingEmail.username) with a check against the channel name, e.g.
(sub) => sub.fname?.includes(channelName) or (sub) => sub.name === channelName
so the test verifies the room subscription was rejected rather than relying on
username mismatch.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
ggazzo
previously approved these changes
Jan 27, 2026
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
stat: ready to merge
dionisio-bot
bot
added
stat: ready to merge
dionisio-bot
bot
added
stat: ready to merge
sampaiodiego
removed
the
stat: QA assured
dionisio-bot
bot
removed
the
stat: ready to merge
sampaiodiego
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
sampaiodiego
force-pushed
the
federation-validate-user-domain
branch
2 times, most recently
from
05925f3 to
cc112ef
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ee/packages/federation-matrix/tests/end-to-end/permissions.spec.ts`:
- Around line 283-362: Combine the two beforeAll hooks into a single beforeAll
inside the "Federation_Service_Validate_User_Domain Setting" describe block:
move the permissions.grant POST (the
rc1AdminRequestConfig.request.post(api('permissions.update')) call) and the RC
user creation steps (creating rcValidUser1 and rcValidUser2 via createUser,
marking them verified with users.update, and obtaining their configs via
getRequestConfig) into one sequential beforeAll so all setup runs in one place;
ensure you preserve the existing order (grant permission first, then
create/verify users and call getRequestConfig) and keep references to
rcValidUser1, rcValidUser2, rc1AdminRequestConfig, createUser, and
getRequestConfig intact.
ee/packages/federation-matrix/tests/end-to-end/permissions.spec.ts
Outdated
Show resolved
Hide resolved
sampaiodiego
force-pushed
the
federation-validate-user-domain
branch
from
cc112ef to
54367e9
Compare
sampaiodiego
force-pushed
the
federation-validate-user-domain
branch
from
54367e9 to
121765e
Compare
ggazzo
approved these changes
Jan 28, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Create a new setting with an extra layer of validation to restrict the usage of federation to only users with a validated email address that matches the configured federation domain.
Issue(s)
FEDCORE-42
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.