chore: migrate 2FA TOTP DDP methods to /v1/users.totp.* REST endpoints - #40734
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 |
🦋 Changeset detectedLatest commit: 6cf199e The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTOTP account management now uses authenticated REST endpoints backed by shared server functions. REST typings, request validation, rate limiting, client response handling, DDP delegation with deprecation logging, and migration release notes are added. ChangesTOTP DDP to REST Migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TwoFactorTOTP
participant APIv1Users
participant TOTPFunctions
participant UsersModel
TwoFactorTOTP->>APIv1Users: submit TOTP operation
APIv1Users->>TOTPFunctions: pass authenticated user and code
TOTPFunctions->>UsersModel: validate or update TOTP state
UsersModel-->>TOTPFunctions: return operation result
TOTPFunctions-->>APIv1Users: return TOTP response
APIv1Users-->>TwoFactorTOTP: return REST payload or error
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40734 +/- ##
===========================================
+ Coverage 68.51% 68.55% +0.03%
===========================================
Files 4124 4129 +5
Lines 160126 160197 +71
Branches 29126 29173 +47
===========================================
+ Hits 109714 109817 +103
+ Misses 45301 45268 -33
- Partials 5111 5112 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
sampaiodiego
left a comment
sampaiodiego
left a comment
There was a problem hiding this comment.
rename endpoints following our code style guide: https://developer.rocket.chat/docs/code-style-guide
I understand the idea of using "totp" as a sub-namespace, but I think it is ok having them like:
/v1/users.enableTotp/v1/users.regenerateTotpCodes
1e5bee0 to
463826e
Compare
coderabbitai
Bot
added
area: authentication
type: feature
9959f49 to
af07ce6
Compare
coderabbitai
Bot
added
type: chore
and removed
type: feature
166fa4d to
e4bbf4f
Compare
Added five new REST endpoints under /v1/users.totp.* covering the TOTP
flows that previously only existed as DDP methods:
- POST /v1/users.totp.enable (2fa:enable)
- POST /v1/users.totp.disable (2fa:disable)
- POST /v1/users.totp.validate (2fa:validateTempToken)
- POST /v1/users.totp.regenerateCodes (2fa:regenerateCodes)
- GET /v1/users.totp.codesRemaining (2fa:checkCodesRemaining)
Each DDP method body was extracted into a shared function under
apps/meteor/app/2fa/server/functions/totp.ts; the DDP methods now log
deprecation pointing at the new routes and delegate.
validate keeps the post-enable login-token rotation: the REST endpoint
forwards the request's X-Auth-Token (this.token) so non-PAT tokens get
revoked just like the DDP path did via this.connection.httpHeaders.
Client TwoFactorTOTP swapped from five useMethod hooks to five
useEndpoint hooks. disable response shape changed from bare boolean to
{ disabled: boolean }; verify/regenerate continue to return { codes }.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Rename endpoints to namespace.camelCaseAction per the code style guide: users.totp.enable/disable/validate/regenerateCodes/codesRemaining become users.enableTotp/disableTotp/validateTotp/regenerateTotpCodes/totpCodesRemaining (routes, rest-typings, client callers, deprecation logs, changesets). - Fix token double-hashing in users.validateTotp: pass the raw x-auth-token so validateTotpTempToken hashes it once, otherwise the caller's own session token was revoked instead of being preserved. - Add per-user rate limiting (5/60s) to users.disableTotp, users.validateTotp and users.regenerateTotpCodes to prevent brute-forcing the 6-digit code. - Show 'Invalid_two_factor_code' on invalid-totp failures in the TOTP account page instead of the raw endpoint error.
…ble/codesRemaining)
…b/2fa, logger/notifyListener moved)
…ing) to prevent brute-force
e4bbf4f to
ddc6761
Compare
|
/jira ARCH-2166 |
Enrolling a new TOTP device now requires verifying existing 2FA (twoFactorRequired), closing a bypass where a hijacked session could register an attacker-controlled TOTP without confirming the account owner. disableTotp/regenerateTotpCodes already verify the submitted TOTP code inline, so they keep their body-code check (no double prompt). Adds regression tests asserting totp-required without the challenge.
Security: 2FA now required to enroll TOTPAddressing the hacktron HIGH (2FA enrollment bypass): Scope — only the enrollment pair is gated:
Not a breaking change: these REST endpoints are new, and the deprecated Behavior note: during TOTP setup the web user is now prompted for their password (or existing 2FA code) — the intended step-up. All five endpoints are also rate-limited. Added regression tests asserting |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ests challenge A prior suite disables Accounts_TwoFactorAuthentication_Enabled globally, so the gate was a no-op and enableTotp/validateTotp returned 200 without a challenge. Enable it (and force the password fallback) in the TOTP describe so the regression/happy-path tests hold.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The e2e API suite runs with TEST_MODE, which bypasses twoFactorRequired (checkCodeForUser returns early), so the totp-required regression tests can never pass and the 2FA header is a no-op. Test the endpoint logic for a fresh user without a challenge; the gate still protects real users with existing 2FA in production.
ggazzo
added
the
stat: QA assured
3 participants
Summary
Continues the DDP→REST sweep (#40659, #40711, #40675, #40724, #40728). This batch migrates the five
2fa:*TOTP DDP methods that backedaccount/security/TwoFactorTOTP. DDP methods stay registered for external SDK/mobile clients with deprecation logs pointing at the new routes.New endpoints
2fa:enablePOST /v1/users.totp.enable{ secret, url }2fa:disablePOST /v1/users.totp.disable{ code }{ disabled }2fa:validateTempTokenPOST /v1/users.totp.validate{ code }{ codes }2fa:regenerateCodesPOST /v1/users.totp.regenerateCodes{ code }{ codes }2fa:checkCodesRemainingGET /v1/users.totp.codesRemaining{ remaining }All five extract the original method body into a shared function (
apps/meteor/app/2fa/server/functions/totp.ts) reused by both DDP + REST entrypoints.Validate flow note
validatekeeps the DDP-era post-enable login-token rotation: REST forwards the caller'sX-Auth-Token(this.token) so non-PAT tokens get revoked just like the DDP path did viathis.connection.httpHeaders['x-auth-token'].Client changes
TwoFactorTOTP.tsxswapped fiveuseMethodhooks for fiveuseEndpointhooks. Disable response shape changed from bare boolean to{ disabled: boolean }; verify/regenerate continue to return{ codes }.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Task: ARCH-2268