Create a flow to replace the user's two-factor device#79266
Conversation
…ling for device replacement flow
This comment has been minimized.
This comment has been minimized.
|
Hi @brunovjk! Feel free to give this a first pass now if you like, but it won't be testable til the backend PRs go out. I'll remove the HOLD and ping you when that happens |
|
CC @trjExpensify since you were in the slack thread |
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.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de2f21b28b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
I think the secret code with the copy button should be styled more like our input rows: 
And reduce the spacing between the code itself and the text above it as it's a bit too big. Otherwise this is pretty good. cc @dannymcclain for 👀 as well. I could see us simplifying it further, but I think it works relatively well as is |
|
Agree with Jon's feedback. One question about our standard copy-to-clipboard pattern though: on mobile that translates to a long press, @dubielzyk-expensify do you think that's too hidden for something like this on mobile? Most of, if not all of, the other fields people are copying are very optional copy actions—this feels a bit more critical so I can see the argument for an exposed, always visible button here. Thoughts? |
|
I lifted the secret-code-copy-button component for this flow from the 2FA enablement flow - here's what that looks like on main today 2fa.enable.main.mp4I suspect the reason that it's already different from our typical input rows is for exactly the reason @dannymcclain mentioned - copy here is a primary necessary action, especially on mobile (since it's not possible to scan the qr code unless you have a second phone handy...). Here's what the current 2fa enablement flow looks like today on mobile 
I'm happy to change how it looks but I do feel like we should have these two flows match |
|
Ok @chuckdries hear me out - can we add a container around the secret authenticator key and button like we have on the 2FA codes? And also use Expensify Mono for the secret key text? Something like one of these: 
... Also, if it's not too much to ask, can we please fix that copy button? (The icon is on the wrong side in real life but is correct in my mocks above.) |
|
👍 |
|
Love it. Great suggestion @dannymcclain and nice work @chuckdries ! #teamwork |
|
Let's gooooo! |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93430046cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useEffect(() => { | ||
| if (!account?.twoFactorAuthSecretKey) { | ||
| return; | ||
| } | ||
| Navigation.navigate(ROUTES.SETTINGS_2FA_REPLACE_VERIFY_NEW); |
There was a problem hiding this comment.
This effect treats any existing twoFactorAuthSecretKey as proof that the old-device verification just succeeded. If a user previously started the replace flow and then closed the app (or otherwise left mid‑flow), that secret key remains persisted in Onyx, so reopening “Replace device” will skip the old-device verification and force them into verify‑new with a stale secret. If the backend invalidates pending secrets or the user wants to restart, they’ll be stuck in the wrong step. Consider clearing the secret when entering the replace flow (e.g., via the new clearTwoFactorAuthSecretKey) or gating the redirect on an explicit “replacement in progress” flag.
Useful? React with 👍 / 👎.
|
@chuckdries It seems we have conflicts to resolve? Thank you. |
|
@chuckdries there are conflicts, could you handle them please? |
|
Those merge conflicts were a little gnarly - @brunovjk would you mind taking another pass on this one? TYSM! |
|
Sure, but we still have some warnings here. @chuckdries do you think they might be related to our PR? Thank you. |
|
@chuckdries The app seems to be crashing after setting up the first two-factor authentication: pr.movDo you think it's related to our PR? I can't reproduce it in production. Thank you. |
|
Yep, the failures were me being a fool while resolving the merge conflicts. I've fixed recording of the whole flow working once again - iOS HybridApptwo.factor.replace.flow.after.merge.fix.mp4 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @lakchote has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.3.36-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.36-10 🚀
|
Explanation of Change
This PR introduces a new flow that allows users to change their registered two-factor authentication device in-place, rather than disabling the feature then re-enabling it. I tried to keep the changeset relatively small and follow the path of least resistance.
At this stage I'm looking for feedback from design and product. PR on hold until necessary backend PRs are mergedAPIs are now available!Video of first implementation - updated implementation videos in screenshots/videos section at bottom
2fa.replace.device.flow.2.mp4
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/583049
PROPOSAL:
Tests
Happy path
Invalid codes are rejected
Offline tests
N/A - Two-factor authentication screens are unavailable offline
QA Steps
Same as 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./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: Native
2fa.device.change.android.mp4
iOS: Native
2fa.device.change.ios.mp4
iOS: mWeb Safari
2fa.device.change.mobile.safari.mp4
MacOS: Chrome / Safari
2fa.device.change.desktop.chrome.mp4