refactor: move 2FA disable logic to Authorization service#38781
Open
Saraabodeeb wants to merge 5 commits intoRocketChat:developfrom
Open
refactor: move 2FA disable logic to Authorization service#38781Saraabodeeb wants to merge 5 commits intoRocketChat:developfrom
Saraabodeeb wants to merge 5 commits intoRocketChat:developfrom
Conversation
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
WalkthroughRefactors the 2FA disable flow by moving verification, DB update, and notification logic into Authorization.disable2FA(uid, code); the Meteor method now verifies authentication and delegates to that service. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MeteorMethod as "Meteor '2fa:disable'"
participant AuthService as "Authorization.disable2FA"
participant TOTP
participant UsersDB as "Users DB"
participant Notifier as "notifyOnUserChange"
Client->>MeteorMethod: call '2fa:disable'(code)
MeteorMethod->>AuthService: disable2FA(userId, code)
AuthService->>UsersDB: find user by uid
UsersDB-->>AuthService: user record
AuthService->>TOTP: verify(code, user.services.totp.secret, backups)
TOTP-->>AuthService: valid / invalid
alt valid
AuthService->>UsersDB: Users.disable2FAByUserId(uid)
UsersDB-->>AuthService: { modifiedCount: 1 }
AuthService->>Notifier: notifyOnUserChange({...diff: {'services.totp.enabled': false} })
Notifier-->>AuthService: ack
AuthService-->>MeteorMethod: true
MeteorMethod-->>Client: true
else invalid
AuthService-->>MeteorMethod: throw error / false
MeteorMethod-->>Client: error / false
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
KevLehman
changed the title
Saraabodeeb
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
KevLehman
requested changes
Feb 18, 2026
![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: 4
🧹 Nitpick comments (1)
apps/meteor/server/services/authorization/service.ts (1)
206-221:disable2FAreturnsfalsefor two unrelated failure modes — callers cannot distinguish them.Returning
falsefor both "user has no TOTP enabled" (line 208) and "TOTP verification failed" (line 217) collapses two semantically distinct outcomes into a single falsy signal. The Meteor method (and any future callers) cannot provide meaningful feedback to the user — e.g., the difference between "2FA is not active on your account" and "the code you entered is wrong."Consider returning a discriminated type or throwing distinct
Meteor.Error/service errors for each path:♻️ Suggested approach — throw on verification failure
async disable2FA(uid: string, code: string): Promise<boolean> { const user = await Users.findOneById(uid, { projection: { 'services.totp.enabled': 1, 'services.totp.secret': 1, 'services.totp.hashedBackup': 1 } }); if (!user?.services?.totp?.enabled) return false; const verified = await TOTP.verify({ secret: user.services.totp.secret, token: code, userId: uid, backupTokens: user.services.totp.hashedBackup, }); - if (!verified) return false; + if (!verified) throw new Error('totp-invalid'); const { modifiedCount } = await Users.disable2FAByUserId(uid); return modifiedCount > 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/authorization/service.ts` around lines 206 - 221, The disable2FA function currently returns false for both "TOTP not enabled" and "verification failed", making callers unable to distinguish the cases; update disable2FA to throw distinct errors instead of returning false for the two failure modes (use Meteor.Error or your service error class): when user?.services?.totp?.enabled is falsy, throw a clear error (e.g., code "2fa-not-enabled", message "Two-factor authentication is not enabled for this account"), and when TOTP.verify returns false throw a different error (e.g., code "invalid-2fa-token", message "Invalid two-factor authentication code"); keep the call to TOTP.verify and Users.disable2FAByUserId and return true on successful modifiedCount > 0 (or throw if DB update fails) so callers can distinguish all outcomes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/meteor/app/2fa/server/methods/disable.tsapps/meteor/server/services/authorization/service.tspackages/core-services/src/types/IAuthorization.tspackages/core-services/tests/Authorization.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/core-services/src/types/IAuthorization.tspackages/core-services/tests/Authorization.spec.tsapps/meteor/server/services/authorization/service.tsapps/meteor/app/2fa/server/methods/disable.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
packages/core-services/tests/Authorization.spec.ts
🧠 Learnings (9)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
packages/core-services/src/types/IAuthorization.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
packages/core-services/src/types/IAuthorization.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
packages/core-services/tests/Authorization.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
packages/core-services/tests/Authorization.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/core-services/tests/Authorization.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use `.spec.ts` extension for test files (e.g., `login.spec.ts`)
Applied to files:
packages/core-services/tests/Authorization.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
packages/core-services/tests/Authorization.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
packages/core-services/tests/Authorization.spec.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/2fa/server/methods/disable.ts
🧬 Code graph analysis (4)
packages/core-services/src/types/IAuthorization.ts (1)
apps/meteor/app/2fa/server/methods/disable.ts (1)
code(6-20)
packages/core-services/tests/Authorization.spec.ts (1)
packages/core-services/src/types/IAuthorization.ts (1)
IAuthorization(9-19)
apps/meteor/server/services/authorization/service.ts (2)
apps/meteor/app/2fa/server/methods/disable.ts (1)
code(6-20)apps/meteor/app/authentication/server/startup/index.js (1)
verified(222-222)
apps/meteor/app/2fa/server/methods/disable.ts (1)
apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnUserChange(377-389)
⏰ 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: cubic · AI code reviewer
🔇 Additional comments (3)
packages/core-services/src/types/IAuthorization.ts (1)
18-18: LGTM — interface contract is clean and consistent.The signature aligns with the implementation and the
Promise<boolean>return type is appropriate for the contract.packages/core-services/tests/Authorization.spec.ts (1)
1-8: No action required. The Jest configuration forpackages/core-servicesuses the server preset without defining a restrictivetestMatchpattern, which means it relies on Jest's default discovery patterns including**/?(*.)+(spec|test).[jt]s?(x). The test file attests/Authorization.spec.tsmatches this pattern and will be discovered by Jest.Likely an incorrect or invalid review comment.
apps/meteor/server/services/authorization/service.ts (1)
208-215:⚠️ Potential issue | 🟡 MinorAdd defensive null checks for
secretandhashedBackupafter the enabled check.The optional chaining guard on line 208 doesn't provide TypeScript type narrowing. While the type definition declares
secretandhashedBackupas required fields within the totp object, after the guard!user?.services?.totp?.enabled, TypeScript cannot guarantee these fields exist. The?.operator evaluates but doesn't narrow types. If the TOTP object is partially initialized or corrupted in the database,TOTP.verifycould receiveundefinedvalues.For consistency and safety, add explicit null checks for
secretbefore accessing it (as done inapps/meteor/app/2fa/server/code/TOTPCheck.ts:23).⛔ Skipped due to learnings
Learnt from: d-gubert Repo: RocketChat/Rocket.Chat PR: 38227 File: apps/meteor/app/api/server/router.ts:44-49 Timestamp: 2026-01-26T18:26:01.279Z Learning: In apps/meteor/app/api/server/router.ts, when retrieving bodyParams and queryParams from the Hono context via c.get(), do not add defensive defaults (e.g., ?? {}). The code should fail fast if these parameters are missing, as endpoint handlers expect them to be present and breaking here helps surface parsing problems rather than hiding them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/2fa/server/methods/disable.ts`:
- Around line 5-21: Add a TypeScript declaration augmenting module
'@rocket.chat/ddp-client' with interface ServerMethods that includes
"'2fa:disable'(code: string): Promise<boolean>;" and then apply that generic to
the Meteor.methods call (i.e., Meteor.methods<ServerMethods>({...})), keeping
the existing async '2fa:disable' implementation intact; this will provide proper
typing for useMethod('2fa:disable') in client code and match the pattern used by
other 2fa methods.
- Around line 10-19: Change Authorization.disable2FA to follow the existing 2FA
error pattern by throwing a distinguishable Meteor.Error (e.g., 'invalid-totp')
when verification fails instead of returning false, and keep returning/throwing
other errors for DB/update failures so callers can distinguish reasons; update
the method usage in disable.ts (the call to Authorization.disable2FA and
surrounding logic) to expect/propagate that error rather than treating all
falses the same, and add a ServerMethods interface declaration in this file
matching the pattern used in validateTempToken.ts / regenerateCodes.ts /
enable.ts to provide type safety for the RPC method signature.
In `@apps/meteor/server/services/authorization/service.ts`:
- Line 207: The Users.findOneById(uid) call in the authorization service loads
the entire user document; instead query only the TOTP fields used (e.g.
services.totp.*) by adding a projection to the findOneById call so only
services.totp.enabled and services.totp.secret (or services.totp.*) are
returned; update the call where Users.findOneById(uid) is invoked in the
authorization/service.ts function handling 2FA checks to include the projection
and adjust any downstream assumptions about missing fields accordingly.
In `@packages/core-services/tests/Authorization.spec.ts`:
- Around line 5-8: The current test is a tautology — remove the trivial expect
that compares the string literal and instead implement a runtime behavioral test
for disable2FA: call the concrete authorization implementation's disable2FA
method (use the service/class under test), mock dependencies like Users and TOTP
to simulate enabled/disabled states and valid/invalid codes, and assert the
expected outcomes (e.g., user 2FA flag cleared, errors thrown on invalid code).
If you only want a compile-time check, replace the test description to state it
is a TypeScript-only interface check and remove the runtime expect; otherwise
add mocks for Users and TOTP and assert concrete behavior of disable2FA. Ensure
references to IAuthorization and disable2FA remain so reviewers can locate the
change.
---
Nitpick comments:
In `@apps/meteor/server/services/authorization/service.ts`:
- Around line 206-221: The disable2FA function currently returns false for both
"TOTP not enabled" and "verification failed", making callers unable to
distinguish the cases; update disable2FA to throw distinct errors instead of
returning false for the two failure modes (use Meteor.Error or your service
error class): when user?.services?.totp?.enabled is falsy, throw a clear error
(e.g., code "2fa-not-enabled", message "Two-factor authentication is not enabled
for this account"), and when TOTP.verify returns false throw a different error
(e.g., code "invalid-2fa-token", message "Invalid two-factor authentication
code"); keep the call to TOTP.verify and Users.disable2FAByUserId and return
true on successful modifiedCount > 0 (or throw if DB update fails) so callers
can distinguish all outcomes.
18ba7a5 to
f136461
Compare
Author
|
Hi @KevLehman, I completely agree with testing the actual behavior! Since the 2fa:disable logic didn't have tests prior to this refactor, I want to make sure we set up the right testing foundation here. To start, I've added a strict Behavioral Unit Test (apps/meteor/server/services/authorization/service.spec.ts) that uses calledOnceWithExactly to verify the exact parameters passed to DB operations and notifications, rather than just mocking return values. I also want to add a proper Integration Test (inserting a real user into a Test DB and asserting the state change). However, to avoid MongoNotConnectedError in the CI, could you point me to the standard directory for DB-connected core service tests? (e.g., apps/meteor/tests/integration/...?). Once you point me to the right suite, I’ll happily add the full integration test! Let me know what you think. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/server/services/authorization/service.spec.ts (1)
62-81: Add a test for themodifiedCount: 0path after successful TOTP verification.The success suite only covers
modifiedCount: 1. There is a reachable code path in the implementation (lines 242–243 ofservice.ts) where TOTP verification passes butdisable2FAByUserIdreturns{ modifiedCount: 0 }, causing the method to returnfalsewithout callingnotifyOnUserChange. This path is currently untested.➕ Suggested additional test case
+ it('should return false and not notify when DB update modifies nothing', async () => { + const testUid = 'valid_uid'; + (Users.findOneById as sinon.SinonStub).resolves({ + services: { totp: { enabled: true, secret: 'secret', hashedBackup: [] } }, + }); + (TOTP.verify as sinon.SinonStub).resolves(true); + (Users.disable2FAByUserId as sinon.SinonStub).resolves({ modifiedCount: 0 }); + + const result = await service.disable2FA(testUid, 'correct_code'); + + expect(result).to.be.false; + expect((notifyListener.notifyOnUserChange as sinon.SinonStub).called).to.be.false; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/authorization/service.spec.ts` around lines 62 - 81, Add a test that covers the path where TOTP verification succeeds but the DB update returns modifiedCount: 0: stub Users.findOneById to return a user with services.totp enabled, stub TOTP.verify to resolve true, stub Users.disable2FAByUserId to resolve { modifiedCount: 0 }, call service.disable2FA with the test UID and code, assert the method returns false and assert notifyListener.notifyOnUserChange was not called; use the same helpers/stubs as the existing test (Users.findOneById, TOTP.verify, Users.disable2FAByUserId, notifyListener.notifyOnUserChange) and mirror the structure of the current success test.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/app/2fa/server/methods/disable.tsapps/meteor/server/services/authorization/service.spec.tsapps/meteor/server/services/authorization/service.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/services/authorization/service.spec.tsapps/meteor/app/2fa/server/methods/disable.tsapps/meteor/server/services/authorization/service.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/server/services/authorization/service.spec.ts
🧠 Learnings (11)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/server/services/authorization/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/server/services/authorization/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/server/services/authorization/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/server/services/authorization/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/server/services/authorization/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/server/services/authorization/service.spec.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/server/services/authorization/service.spec.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 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/app/2fa/server/methods/disable.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/2fa/server/methods/disable.tsapps/meteor/server/services/authorization/service.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/2fa/server/methods/disable.tsapps/meteor/server/services/authorization/service.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/server/services/authorization/service.ts
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/server/services/authorization/service.ts`:
- Around line 235-239: The diff passed to notifyOnUserChange currently only
reports 'services.totp.enabled': false but disable2FAByUserId replaces the
entire services.totp object (removing secret and hashedBackup), so update the
diff in the notifyOnUserChange call to reflect the full removal (include
'services.totp.secret': null and 'services.totp.hashedBackup': null along with
'services.totp.enabled': false) or change the DB operation to use $unset and
send corresponding unset semantics; modify the notifyOnUserChange invocation in
the disable2FAByUserId flow to emit the complete field removals so clients won't
retain stale cached values.
---
Duplicate comments:
In `@apps/meteor/app/2fa/server/methods/disable.ts`:
- Line 10: The Meteor.methods call in disable.ts was not updated to use the
ServerMethods generic; update the invocation of Meteor.methods to
Meteor.methods<ServerMethods>({...}) so the runtime binding is type-checked
against the declared ServerMethods interface, matching the pattern used in
enable.ts, validateTempToken.ts, and regenerateCodes.ts and ensuring the
exported disable 2FA method signatures conform to the ServerMethods interface.
---
Nitpick comments:
In `@apps/meteor/server/services/authorization/service.spec.ts`:
- Around line 62-81: Add a test that covers the path where TOTP verification
succeeds but the DB update returns modifiedCount: 0: stub Users.findOneById to
return a user with services.totp enabled, stub TOTP.verify to resolve true, stub
Users.disable2FAByUserId to resolve { modifiedCount: 0 }, call
service.disable2FA with the test UID and code, assert the method returns false
and assert notifyListener.notifyOnUserChange was not called; use the same
helpers/stubs as the existing test (Users.findOneById, TOTP.verify,
Users.disable2FAByUserId, notifyListener.notifyOnUserChange) and mirror the
structure of the current success test.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/server/services/authorization/service.ts (1)
1-1: Consider removing theMeteorimport from this service layer.
import { Meteor }was added here solely to useMeteor.Error(lines 214, 229). This is the only method in the service that reaches directly into Meteor's error type, coupling theIAuthorizationservice implementation to the Meteor runtime. The interface signature (Promise<boolean>) doesn't advertise these throws, which also means consuming code in non-Meteor contexts gets an undocumented dependency.💡 Suggested alternative
-import { Meteor } from 'meteor/meteor';- if (!user) { - throw new Meteor.Error('error-invalid-user', 'Invalid user'); - } + if (!user) { + throw Object.assign(new Error('Invalid user'), { error: 'error-invalid-user' }); + }- if (!verified) { - throw new Meteor.Error('invalid-totp'); - } + if (!verified) { + throw Object.assign(new Error('Invalid TOTP token'), { error: 'invalid-totp' }); + }The Meteor method layer in
apps/meteor/app/2fa/server/methods/disable.tscan catch and re-wrap asMeteor.Errorif needed, keeping the service framework-agnostic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/authorization/service.ts` at line 1, Remove the Meteor runtime dependency from the IAuthorization service by deleting the `import { Meteor }` and stop throwing `Meteor.Error` inside this service; instead throw a plain Error or a domain-specific error (e.g., AuthorizationError) from the service methods that currently use `Meteor.Error` (the places in this file that construct `new Meteor.Error(...)`). Keep the service interface and Promise<boolean> return types unchanged, and update callers in the Meteor method layer (for example the 2FA disable method) to catch the service Error and re-wrap it as `Meteor.Error` for transport to the client.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/server/services/authorization/service.spec.tsapps/meteor/server/services/authorization/service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/server/services/authorization/service.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/services/authorization/service.ts
🧠 Learnings (4)
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/server/services/authorization/service.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 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/server/services/authorization/service.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/server/services/authorization/service.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/server/services/authorization/service.ts
🧬 Code graph analysis (1)
apps/meteor/server/services/authorization/service.ts (2)
apps/meteor/app/2fa/server/methods/disable.ts (1)
code(11-19)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnUserChange(377-389)
⏰ 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: cubic · AI code reviewer
🔇 Additional comments (1)
apps/meteor/server/services/authorization/service.ts (1)
221-226: No type-safety issue here. ThesecretandhashedBackupfields are defined as required (non-optional) properties within thetotpobject in theIUsertype definition (packages/core-typings/src/IUser.tslines 88–93). After the guardif (!user.services?.totp?.enabled)ensurestotpexists andenabledis truthy, bothsecret: stringandhashedBackup: string[]are guaranteed to be accessible. The call toTOTP.verifycorrectly passessecret(required) andbackupTokens(optional), which aligns with the function signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/server/services/authorization/service.ts`:
- Around line 239-242: The object passed into notifyOnUserChange currently sets
'services.totp.secret' and 'services.totp.hashedBackup' to undefined which
JSON.stringify will drop, so replace those undefined entries by adding those key
paths to the notifyOnUserChange unset parameter (while keeping
'services.totp.enabled': false in the fields/modified object); update the
notifyOnUserChange call in service.ts (look for notifyOnUserChange invocation
around the block containing 'services.totp.enabled') to pass unset:
['services.totp.secret','services.totp.hashedBackup'] instead of assigning
undefined values.
---
Nitpick comments:
In `@apps/meteor/server/services/authorization/service.ts`:
- Line 1: Remove the Meteor runtime dependency from the IAuthorization service
by deleting the `import { Meteor }` and stop throwing `Meteor.Error` inside this
service; instead throw a plain Error or a domain-specific error (e.g.,
AuthorizationError) from the service methods that currently use `Meteor.Error`
(the places in this file that construct `new Meteor.Error(...)`). Keep the
service interface and Promise<boolean> return types unchanged, and update
callers in the Meteor method layer (for example the 2FA disable method) to catch
the service Error and re-wrap it as `Meteor.Error` for transport to the client.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
Following the feedback to make PRs more atomic and focused, this PR refactors the 2fa:disable logic. It moves the core functionality from the Meteor methods to the Authorization service within core-services.
Changes
Interface Update: Added disable2FA(uid: string, code: string) to the IAuthorization interface in packages/core-services.
Service Implementation: Implemented the disable2FA logic in Authorization service.
Added security verification using TOTP.verify to ensure the 2FA code is valid before disabling.
Used Users.disable2FAByUserId(uid) for database updates.
Method Refactoring: Updated the 2fa:disable Meteor method in apps/meteor to act as a proxy, delegating the core logic to the Authorization service.
Testing: Added a basic unit test in packages/core-services/tests/Authorization.spec.ts to verify the method definition.
Security Note
[!IMPORTANT]
As discussed in previous reviews, security is a priority. The service now handles the verification step internally before performing any database operations to prevent unauthorized 2FA deactivation.
Summary by CodeRabbit