chore: remove Meteor user invocation from rest api #38017
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 |
|
WalkthroughRefactors APIClass to replace implicit Meteor context dependency with explicit parameters. Removes safeGetMeteorUser utility, introducing performedBy parameter passing throughout user creation flow. Adds applyMeteorContext option flag and new createMeteorInvocation static helper for conditional Meteor context handling in routes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 #38017 +/- ##
===========================================
- Coverage 70.78% 70.78% -0.01%
===========================================
Files 3159 3159
Lines 109388 109384 -4
Branches 19710 19674 -36
===========================================
- Hits 77429 77423 -6
- Misses 29928 29936 +8
+ Partials 2031 2025 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 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/27 19:25 (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: Tue, 27 Jan 2026 19:25:04 GMT |
d-gubert
changed the title
ggazzo
force-pushed
the
chore/remove-invocation-from-rest
branch
from
c2fea9d to
adde004
Compare
ggazzo
force-pushed
the
chore/remove-invocation-from-rest
branch
2 times, most recently
from
1432e63 to
617afd0
Compare
ggazzo
changed the base branch from
develop
to
chore/stop-using-ids-for-functions
ggazzo
force-pushed
the
chore/stop-using-ids-for-functions
branch
2 times, most recently
from
c0f1958 to
671e348
Compare
ggazzo
force-pushed
the
chore/remove-invocation-from-rest
branch
from
617afd0 to
34c0088
Compare
ggazzo
force-pushed
the
chore/stop-using-ids-for-functions
branch
from
671e348 to
d531c0e
Compare
ggazzo
force-pushed
the
chore/remove-invocation-from-rest
branch
3 times, most recently
from
32d30db to
c203b32
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/server/lib/rooms/roomTypes/direct.ts (1)
54-72: Markuidparameter as optional in the function signature to match the interface contract.The
roomNamemethod indirect.tsshould mark theuidparameter as optional (uid?) to match theIRoomTypeServerDirectivesinterface definition, which declaresuserId?: string. Currently, the signatureasync roomName(room, uid)doesn't reflect this, even though the implementation handles the undefined case with a fallback (picking any subscription). While the fallback behavior is intentional, the type signature should accurately reflect thatuidis optional.apps/meteor/app/lib/server/functions/saveUser/saveNewUser.ts (1)
15-30: Store only the minimal identifier for audit trail instead of the full user object.
Accounts.createUserAsyncwill persist theperformedByobject directly to the user document, which includes theservicesfield containing sensitive data (OAuth tokens, login tokens, password hashes, 2FA credentials). This risks PII leakage and unnecessarily bloats the user document. SinceperformedByis not queried from user records anywhere, it should store only essential audit information.Follow the codebase pattern (e.g.,
createdByin other domains) and persist a minimal identifier:Suggested fix
- performedBy, + performedBy: { _id: performedBy._id, username: performedBy.username, name: performedBy.name },
🤖 Fix all issues with AI agents
In `@apps/meteor/app/api/server/definition.ts`:
- Line 195: The readonly property twoFactorChecked is declared as a non-optional
boolean but not initialized; set a default (e.g. initialize twoFactorChecked =
false as a property initializer or assign this.twoFactorChecked = false inside
the class constructor) so the non-optional type cannot be undefined at runtime;
ensure you update the class that declares readonly twoFactorChecked accordingly
(initialize in the constructor if readonly must be set there).
In `@apps/meteor/app/authentication/server/startup/index.js`:
- Around line 380-385: The IPostUserCreated event is being called with
performedBy possibly undefined because many callers of Accounts.insertUserDoc
(e.g., SAML.ts, registerUser.ts, createNewUser.ts, UserConverter.ts) don't pass
performedBy; update the insertUserDoc call sites to accept and forward a
required performedBy (or derive a sensible default like the current user id or a
system user id) and ensure the startup code that triggers
Apps.self.triggerEvent(AppEvents.IPostUserCreated, { user, performedBy:
options.performedBy }) always supplies a non-undefined performedBy;
specifically, add performedBy to the insertUserDoc signature/parameters where
missing, propagate it through user creation flows, and/or set
options.performedBy to a default before calling Apps.self?.triggerEvent so
IPostUserCreated receives a valid actor identifier.
| readonly queryOperations: TOptions extends { queryOperations: infer T } ? T : never; | ||
| readonly queryFields: TOptions extends { queryFields: infer T } ? T : never; | ||
|
|
||
| readonly twoFactorChecked: boolean; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "twoFactorChecked" -C3 apps/meteor/app/api/serverRepository: RocketChat/Rocket.Chat
Length of output: 1145
Initialize twoFactorChecked with a default boolean
Assign a default value (e.g. this.twoFactorChecked = false; in the class constructor or as a property initializer) so the non-optional type never yields undefined at runtime.
🤖 Prompt for AI Agents
In `@apps/meteor/app/api/server/definition.ts` at line 195, The readonly property
twoFactorChecked is declared as a non-optional boolean but not initialized; set
a default (e.g. initialize twoFactorChecked = false as a property initializer or
assign this.twoFactorChecked = false inside the class constructor) so the
non-optional type cannot be undefined at runtime; ensure you update the class
that declares readonly twoFactorChecked accordingly (initialize in the
constructor if readonly must be set there).
| if (!options.skipAppsEngineEvent) { | ||
| // `post` triggered events don't need to wait for the promise to resolve | ||
| Apps.self?.triggerEvent(AppEvents.IPostUserCreated, { user, performedBy: await safeGetMeteorUser() }).catch((e) => { | ||
| Apps.self?.triggerEvent(AppEvents.IPostUserCreated, { user, performedBy: options.performedBy }).catch((e) => { | ||
| Apps.self?.getRocketChatLogger().error({ msg: 'Error while executing post user created event', err: e }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls to insertUserDoc and inspect arguments
rg -nP --type=ts --type=js -C3 'insertUserDoc\s*\(' | head -100Repository: RocketChat/Rocket.Chat
Length of output: 3583
🏁 Script executed:
sed -n '350,430p' apps/meteor/app/authentication/server/startup/index.jsRepository: RocketChat/Rocket.Chat
Length of output: 2581
Ensure performedBy is passed to IPostUserCreated event.
Several Accounts.insertUserDoc call sites (e.g. SAML.ts, registerUser.ts, createNewUser.ts, UserConverter.ts) invoke insertUserDoc({…}, user) without supplying performedBy, causing the IPostUserCreated event to receive undefined. Require callers to include a valid performedBy or provide a sensible default before triggering the event.
🤖 Prompt for AI Agents
In `@apps/meteor/app/authentication/server/startup/index.js` around lines 380 -
385, The IPostUserCreated event is being called with performedBy possibly
undefined because many callers of Accounts.insertUserDoc (e.g., SAML.ts,
registerUser.ts, createNewUser.ts, UserConverter.ts) don't pass performedBy;
update the insertUserDoc call sites to accept and forward a required performedBy
(or derive a sensible default like the current user id or a system user id) and
ensure the startup code that triggers
Apps.self.triggerEvent(AppEvents.IPostUserCreated, { user, performedBy:
options.performedBy }) always supplies a non-undefined performedBy;
specifically, add performedBy to the insertUserDoc signature/parameters where
missing, propagate it through user creation flows, and/or set
options.performedBy to a default before calling Apps.self?.triggerEvent so
IPostUserCreated receives a valid actor identifier.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| token?: string, | ||
| ) { | ||
| const invocation = new DDPCommon.MethodInvocation({ | ||
| connection, |
There was a problem hiding this comment.
not totally sure, so maybe worth it for another PR, but I think we can generate the connection only on this context, so we could remove it from:
Rocket.Chat/apps/meteor/app/api/server/ApiClass.ts
Lines 861 to 862 in cc112ef
sampaiodiego
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
…Invocation utility
… invocation handling for improved context management
… invocation handling for improved context management
…ext handling in saveUser methods for improved clarity and type safety
ggazzo
force-pushed
the
chore/remove-invocation-from-rest
branch
from
c203b32 to
468ab81
Compare
2 participants
rocketchat.atlassian.net/browse/ARCH-1654
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.