-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix flakiness in mandatory 2FA enforcement #77588
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…e rather than route, remove imperative routing to and from it
…Also, centralize enforcement to rendering layer -procedurally blocking navigation never prevented initial navigation by url
|
@bernhardoj Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@hoangzinh Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Hi @bernhardoj, I'm hoping you could review this PR since you authored the original RequireTwoFactorAuthenticationPage component |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@chuckdries yeah, I can review it |
trjExpensify
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No qualms from me with fixing the flakiness here 👍
|
@chuckdries I think we need to prevent the debug modal page shortcut, too. App/src/hooks/useDebugShortcut.tsx Lines 6 to 15 in 6d7eaa6
w.mp4 |
@chuckdries I can't seem to test this case because it prevents me from disabling the 2FA if a workspace is connected to Xero.
Maybe we can also show the same modal if a domain is configured to require 2FA (I'm not sure what this means, actually, that's why I connect my workspace to Xero instead to show the 2FA require modal) |
Ooh good catch. I am pretty sure we need to allow Xero users to reset their 2FA devices, just like users on 2FA enforced domains, but I'm going to confirm that and report back |
|
@chuckdries have you got any confirmation? |
|
Not yet, we're still talking it over. I'll circle back here on Monday. |
|
I suppose when we add 2FA and press 2026-01-15.14.51.20.mov |
|
When I enable 2FA in OD domains |
|
Minor issue on mobile 2026-01-15.15.25.40.mov |
|
On native apps we have an issue with the floating button 2026-01-15.15.33.42.mov |
This is acceptable
This is expected behavior. The OD page specifies that "Domain members will be prompted to set up Two-Factor Authentication on their account when they sign in."
This is acceptable
Good catch! This is now fixed. |
|
@ZhenjaHorbach gentle bump 🙏 |
Sorry for the delay |
|
LGTM! |
|
@Julesssss Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🎯 @ZhenjaHorbach, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #80092. |
Julesssss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement, lets keep an eye out for regressions in next checklist
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.3.6-0 🚀
|
|
This PR failing because of an issue |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.6-4 🚀
|

Explanation of Change
I observed a number of situations where we should've shown the RequireTwoFactorAuthenticationPage, all the data was correct in onyx, but we didn't. The core of this PR is that I removed
/2fa-requiredas a routable url. There's no reason a user would ever need to navigate to it on purpose or share a link to it, and we explicitly prevented the user from navigating away from it. Treating it as a route effectively meant that the current URL was a second source of truth for "should the user be locked out of the app" that needed to be kept in sync with the account data in onyx via imperative routing in an effect. When we failed to do so, the app would become soft locked, where the user was stuck on whatever page they were on and we prevented them from navigating away, but never showed the lockout screen. video demonstrationThis PR makes it so that if the user's account data says they should be forced to enable 2FA, we simply render the lockout page on top of the rest of the app.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/577605
PROPOSAL:
Tests
Prerequisites:
Base case: User is prompted to enable 2FA immediately after signing in
2fa.prompt.immediately.on.login.mp4
Case: User is able to set up 2FA after being shown prompt
2fa.prompt.allows.2fa.setup.mp4
Case: User is re-prompted for 2FA setup immediately after disabling it
2fa.prompt.device.reset.mp4
Case: 2FA prompt always comes back if user bails out of 2FA setup flow
2fa.prompt.always.reappears.mp4
Offline tests
None
QA Steps
Same as manual tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari