Skip to content

Conversation

@yash-rajpal
Copy link
Member

@yash-rajpal yash-rajpal commented Nov 3, 2025

Proposed changes (including videos or screenshots)

Allows users to enable TOTP-based two factor authentication without requiring a verified email address

Issue(s)

Steps to test or reproduce

  • Create a new user, with unverified email
  • Login with this user
  • Try to enable TOTP 2FA from Accounts -> Security

Further comments

SUP-867

Summary by CodeRabbit

  • New Features
    • Users can now enable TOTP-based two-factor authentication regardless of email verification status.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Nov 3, 2025

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Nov 3, 2025

🦋 Changeset detected

Latest commit: 816ef1d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 41 packages
Name Type
@rocket.chat/meteor Minor
@rocket.chat/core-typings Minor
@rocket.chat/rest-typings Minor
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/gazzodown Major
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-client Major
@rocket.chat/ui-contexts Major
@rocket.chat/web-ui-registration Major
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/license Patch
@rocket.chat/media-calls Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Major
@rocket.chat/ui-voip Major
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch

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

@yash-rajpal yash-rajpal added the stat: QA assured Means it has been tested and approved by a company insider label Nov 3, 2025
@yash-rajpal yash-rajpal added this to the 7.13.0 milestone Nov 3, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Nov 3, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 3, 2025

Walkthrough

The changes enable TOTP-based two-factor authentication without requiring verified email addresses. A pre-check blocking 2FA enablement for unverified users has been removed from the enable method, and corresponding test expectations have been updated to verify successful TOTP setup regardless of email verification status.

Changes

Cohort / File(s) Summary
Changeset Documentation
\.changeset/dull-deers-live\.md
Added changeset entry for minor version bump of @rocket.chat/meteor documenting the feature enablement.
2FA Enable Method
apps/meteor/app/2fa/server/methods/enable\.ts
Removed email verification pre-check that previously blocked 2FA enablement. Other validations (authorization, user validity, already-enabled status) remain intact.
2FA Enable Test
apps/meteor/tests/end-to-end/api/methods/2fa-enable\.ts
Updated test case to verify that users with unverified emails can successfully obtain TOTP secret and QR code URL instead of receiving an error.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Area of focus: Review the removed validation logic to ensure no security implications or unintended side effects from bypassing email verification for 2FA setup.
  • Test alignment: Verify that test expectations properly reflect the new behavior and cover the happy path for unverified users.

Suggested reviewers

  • lucas-a-pelegrino
  • tassoevan

Poem

🐰 A hop, skip, and no-verify bound,
Two-factor hops without email ground,
TOTP codes dance without delay,
No SMTP needed—hip hip hooray!
Security's still sound, cheers all around! 🎉

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'fix: Allow TOTP 2FA without email verification' accurately describes the main change in the pull request. It is concise, clear, and directly relates to the primary modification: removing the email verification requirement for enabling TOTP-based two-factor authentication. The title captures the essential intent of the changeset without being vague or overly verbose.
Linked Issues check ✅ Passed The pull request successfully addresses the requirements outlined in linked issue SUP-867. The code changes remove the email verification pre-check from the 2FA enable method, allowing users to set up TOTP-based 2FA without a verified email address. The test case has been updated to verify that a user with an unverified email can successfully obtain a secret and QR code URL, confirming the implementation meets the stated objective to support servers without working SMTP setups.
Out of Scope Changes check ✅ Passed All code changes in the pull request are directly scoped to the stated objective of enabling TOTP-based 2FA without email verification. The modifications include removing the email verification check in the enable method, updating the corresponding test case, and adding a changeset file to document the feature. There are no unrelated changes or scope creep beyond the defined requirements.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/totp-2FA-unverified-email

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Nov 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.22%. Comparing base (c390d5c) to head (816ef1d).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37368      +/-   ##
===========================================
- Coverage    68.98%   68.22%   -0.76%     
===========================================
  Files         3358     3443      +85     
  Lines       114240   114759     +519     
  Branches     20537    20775     +238     
===========================================
- Hits         78803    78290     -513     
- Misses       33345    34364    +1019     
- Partials      2092     2105      +13     
Flag Coverage Δ
e2e 57.46% <ø> (-0.01%) ⬇️
e2e-api 42.83% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yash-rajpal yash-rajpal marked this pull request as ready for review November 3, 2025 17:48
@yash-rajpal yash-rajpal requested a review from a team as a code owner November 3, 2025 17:48
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 9a18288 and 723b5d0.

📒 Files selected for processing (3)
  • .changeset/dull-deers-live.md (1 hunks)
  • apps/meteor/app/2fa/server/methods/enable.ts (0 hunks)
  • apps/meteor/tests/end-to-end/api/methods/2fa-enable.ts (2 hunks)
💤 Files with no reviewable changes (1)
  • apps/meteor/app/2fa/server/methods/enable.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior

Applied to files:

  • apps/meteor/tests/end-to-end/api/methods/2fa-enable.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements

Applied to files:

  • apps/meteor/tests/end-to-end/api/methods/2fa-enable.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (e.g., toBeVisible, toHaveText)

Applied to files:

  • apps/meteor/tests/end-to-end/api/methods/2fa-enable.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use test.step() to organize complex test scenarios

Applied to files:

  • apps/meteor/tests/end-to-end/api/methods/2fa-enable.ts
🪛 LanguageTool
.changeset/dull-deers-live.md

[grammar] ~5-~5: Use a hyphen to join words.
Context: ...- Allows users to enable TOTP-based two factor authentication without requiring ...

(QB_NEW_EN_HYPHEN)

🔇 Additional comments (1)
apps/meteor/tests/end-to-end/api/methods/2fa-enable.ts (1)

55-78: LGTM! Test properly validates the new behavior.

The test correctly verifies that users with unverified emails can now obtain TOTP secrets and QR code URLs. The assertions are consistent with other tests in the file and properly validate the response structure.

@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Nov 13, 2025
@scuciatto scuciatto modified the milestone: 7.13.0 Nov 13, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Nov 13, 2025
@kodiakhq kodiakhq bot merged commit c0dee8d into develop Nov 13, 2025
50 checks passed
@kodiakhq kodiakhq bot deleted the fix/totp-2FA-unverified-email branch November 13, 2025 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants