feat(admin): add UI warning for concurrent 2FA priority#37991
feat(admin): add UI warning for concurrent 2FA priority#37991Its-Onkar wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 2d0ebed The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 |
|
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 |
|
|
WalkthroughAdded i18nDescription fields to two Two-Factor Authentication account settings and updated English locale descriptions to clarify that TOTP takes precedence over Email 2FA when both are enabled. Added a changeset and two git-rewrite reference files. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/i18n/src/locales/en.i18n.json (1)
372-375: TOTP priority clarification looks correct; consider tiny wording polishThe new notes on both descriptions clearly and accurately state that TOTP takes priority and Email 2FA will not be used when TOTP is enabled, which matches the described backend behavior and should reduce admin confusion.
If you want to nit-pick wording while you’re here, you could change “Users can setup their Two Factor Authentication…” to “Users can set up their two-factor authentication…” for grammatical correctness and consistency, but that’s optional.
📜 Review details
Configuration used: Organization 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.
📒 Files selected for processing (2)
apps/meteor/server/settings/accounts.tspackages/i18n/src/locales/en.i18n.json
🧰 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/settings/accounts.ts
🔇 Additional comments (2)
apps/meteor/server/settings/accounts.ts (2)
29-34: LGTM! Email 2FA i18n key is properly translated across all locales.The
i18nDescriptionkey is verified across 25+ language files, creating consistent UI hints for both authentication methods. The translations include priority notes explaining TOTP's precedence when enabled.
22-27: LGTM! i18nDescription follows established pattern.The addition of
i18nDescriptionfollows the existing convention used throughout this file and properly references the translation key.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/sixty-cooks-call.md (1)
6-6: Make the changeset description more specific for clarity.The description "Add clarification note for 2FA priority logic in admin settings" is somewhat generic. Consider revising it to explicitly mention that the change adds i18nDescription fields clarifying that TOTP takes priority over Email 2FA. This provides better context for reviewers reading the changelog.
Example revision: "Add i18nDescription fields to 2FA settings clarifying that TOTP takes priority over Email authentication"
📜 Review details
Configuration used: Organization 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.
📒 Files selected for processing (1)
.changeset/sixty-cooks-call.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
.changeset/sixty-cooks-call.md
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
.changeset/sixty-cooks-call.md
Its-Onkar
force-pushed
the
feat/2fa-priority-warning
branch
from
1ff8bb4 to
c83586a
Compare
Its-Onkar
force-pushed
the
feat/2fa-priority-warning
branch
from
c83586a to
093ec9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/i18n/src/locales/en.i18n.json (1)
372-374: 2FA precedence copy is correct; optional microcopy alignmentThe new notes accurately communicate that TOTP takes precedence and Email 2FA is skipped when both are enabled, matching the intended behavior and the linked issue.
If you want to tighten and align the two hints further (optional only), you could make the TOTP note explicitly mirror the email one, e.g.:
Optional wording tweak
- "Accounts_TwoFactorAuthentication_By_TOTP_Enabled_Description": "Users can setup their Two Factor Authentication using any TOTP App, like Google Authenticator or Authy. Note: When TOTP is enabled for a user, it takes priority over Email 2FA.", + "Accounts_TwoFactorAuthentication_By_TOTP_Enabled_Description": "Users can setup their Two Factor Authentication using any TOTP App, like Google Authenticator or Authy. Note: If Email 2FA is also enabled, TOTP takes priority and Email 2FA will not be used."
📜 Review details
Configuration used: Organization 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.
📒 Files selected for processing (6)
.changeset/sixty-cooks-call.md.git-rewrite/backup-refs.git-rewrite/heads.git-rewrite/raw-refsapps/meteor/server/settings/accounts.tspackages/i18n/src/locales/en.i18n.json
✅ Files skipped from review due to trivial changes (2)
- .git-rewrite/raw-refs
- .git-rewrite/heads
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/server/settings/accounts.ts
- .changeset/sixty-cooks-call.md
2 participants
Description
This PR adds clarification text to the Two Factor Authentication settings in the Admin panel to inform administrators that TOTP takes priority when both authentication methods are enabled.
Motivation
When both TOTP and Email 2FA are enabled simultaneously, only TOTP verification is prompted during login. The Email 2FA step is automatically skipped due to the backend's method selection logic. This behavior is not currently communicated in the UI, which can lead to security misconfigurations where admins expect both layers to be active (Closes #35528).
This PR adds a non-intrusive hint to the settings to manage administrator expectations without altering the underlying authentication flow.
Changes
i18nDescriptionfields toAccounts_TwoFactorAuthentication_By_TOTP_EnabledandAccounts_TwoFactorAuthentication_By_Email_Enabled.How to Test
Screenshots
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.