fix: Email 2FA auto opt in #37326
fix: Email 2FA auto opt in #37326
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: 3e12859 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 |
WalkthroughThis change fixes a bug where new users were automatically enrolled in email-based two-factor authentication even when prerequisite settings were disabled. The fix implements multi-flag validation: email 2FA auto opt-in now requires three conditions—2FA globally enabled, email 2FA enabled, and auto opt-in enabled—before applying to new users. Changes
Sequence DiagramsequenceDiagram
participant User
participant API as API/Server
participant AuthSystem as Authentication System
participant Settings as Settings Store
User->>API: Create new user account
API->>AuthSystem: insertUserDoc()
AuthSystem->>Settings: Check Accounts_TwoFactorAuthentication_Enabled
alt Enabled
AuthSystem->>Settings: Check Accounts_TwoFactorAuthentication_By_Email_Enabled
alt Enabled
AuthSystem->>Settings: Check Accounts_TwoFactorAuthentication_By_Email_Auto_Opt_In
alt Enabled
rect rgba(76, 175, 80, 0.2)
Note over AuthSystem: All conditions met
AuthSystem->>AuthSystem: Set user.services.email2fa.enabled = true
AuthSystem->>AuthSystem: Set changedAt timestamp
end
else Disabled
Note over AuthSystem: Auto opt-in disabled → skip
end
else Disabled
Note over AuthSystem: Email 2FA disabled → skip
end
else Disabled
Note over AuthSystem: 2FA disabled → skip
end
AuthSystem->>API: Return user document
API->>User: User created
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37326 +/- ##
===========================================
- Coverage 67.92% 67.91% -0.02%
===========================================
Files 3356 3356
Lines 114887 114887
Branches 20758 20770 +12
===========================================
- Hits 78040 78022 -18
- Misses 34157 34179 +22
+ Partials 2690 2686 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
yash-rajpal
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
![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)
.changeset/lucky-bulldogs-divide.md (1)
5-5: Consider hyphenating compound modifiers for clarity.The static analysis tools suggest hyphenating compound terms for readability:
- "auto opt in" → "auto opt-in"
- "email two factor authentication" → "email two-factor authentication"
- "two factor authentication" → "two-factor authentication"
While the message is clear as written, these hyphenations would align with standard English style guides for compound modifiers.
Apply this diff to improve readability:
-Fixes an issue related to creating new users, it should not auto opt in new users for email two factor authentication if any one of `Accounts_TwoFactorAuthentication_Enabled`, `Accounts_TwoFactorAuthentication_By_Email_Enabled` and `Accounts_TwoFactorAuthentication_By_Email_Auto_Opt_In` setting is disabled. +Fixes an issue related to creating new users. New users should not be auto-opted-in for email two-factor authentication if any one of `Accounts_TwoFactorAuthentication_Enabled`, `Accounts_TwoFactorAuthentication_By_Email_Enabled`, or `Accounts_TwoFactorAuthentication_By_Email_Auto_Opt_In` settings is disabled.
📜 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.
📒 Files selected for processing (3)
.changeset/lucky-bulldogs-divide.md(1 hunks)apps/meteor/app/authentication/server/startup/index.js(1 hunks)apps/meteor/tests/end-to-end/api/users.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/users.ts (3)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)apps/meteor/tests/data/users.helper.ts (2)
deleteUser(55-62)login(39-53)apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)
🪛 LanguageTool
.changeset/lucky-bulldogs-divide.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ...o creating new users, it should not auto opt in new users for email two factor au...
(QB_NEW_EN_HYPHEN)
[grammar] ~5-~5: Use a hyphen to join words.
Context: ...eating new users, it should not auto opt in new users for email two factor authen...
(QB_NEW_EN_HYPHEN)
[grammar] ~5-~5: Use a hyphen to join words.
Context: ... not auto opt in new users for email two factor authentication if any one of `Acc...
(QB_NEW_EN_HYPHEN)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/app/authentication/server/startup/index.js (1)
310-320: LGTM! Multi-flag validation correctly implemented.The three-flag check properly gates email 2FA auto opt-in by requiring all conditions to be met:
- Global 2FA enabled
- Email-based 2FA enabled
- Email 2FA auto opt-in enabled
This fixes the issue where enabling
Auto_Opt_Inwhile parent settings were disabled incorrectly opted in new users. The addition of thechangedAttimestamp is also a good practice for tracking when the setting was applied.apps/meteor/tests/end-to-end/api/users.ts (1)
697-829: LGTM! Comprehensive test coverage for multi-flag validation.The test suite thoroughly validates the email 2FA auto opt-in behavior:
- ✅ Baseline: Auto opt-in when all flags enabled
- ✅ Guard: No opt-in when email 2FA disabled
- ✅ Guard: No opt-in when global 2FA disabled
- ✅ Guard: No opt-in when auto opt-in disabled
The test structure is well-organized with:
- Proper cleanup in
afterEachto reset settings and delete test users- Consistent verification pattern across all test cases
- Clear test descriptions matching the expected behavior
|
Yash, can you check issues for '2FA' and link them as I think there are a couple this will fix? Possibly 35528, and others? |
Proposed changes (including videos or screenshots)
Upon creating new users, it should respect all settings related to email 2FA
Accounts_TwoFactorAuthentication_Enabled,Accounts_TwoFactorAuthentication_By_Email_EnabledandAccounts_TwoFactorAuthentication_By_Email_Auto_Opt_In. If one of them is disabled, it should auto opt in new users for email2FA.Issue(s)
Steps to test or reproduce
Accounts_TwoFactorAuthentication_By_Email_Auto_Opt_Inbut disable it's parent settings which are -Accounts_TwoFactorAuthentication_By_Email_EnabledandAccounts_TwoFactorAuthentication_EnabledFurther comments
SUP-866
Summary by CodeRabbit
Bug Fixes