-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD for payment 2024-08-29] [$250] 2FA - No recovery codes in 2FA page for unverified account #43603
Comments
Triggered auto assignment to @trjExpensify ( |
Yeah, you shouldn't be able to set up 2FA with an unvalidated account, which I suspect is what's happening here, it's just not very good in the UI. We should improve the UI to prompt for validation as the first screen in the 2FA flow for an unvalidated account, so triggering a magic code and displaying an input to enter the code. After which, display the recovery codes and let the (now) validated user proceed to enable 2FA. CC: @Expensify/design for input on that. @techievivek do you think that can be worked on externally, and I suspect might need a little help on some backend work to support putting that in place? |
That makes sense to me. |
Makes sense to me too. |
ProposalPlease re-state the problem that we are trying to solve in this issue.What is the root cause of that problem?The root cause is that we are not checking if the user is validated in SecuritySettingsPage before we allow the user to open 2FA page. What changes do you think we should make in order to solve the problem?We should fetch user details from onyx using
App/src/pages/settings/Security/SecuritySettingsPage.tsx Lines 25 to 30 in 06cde0a
Note: we will fetch Result: Screen.Recording.2024-06-13.at.7.37.52.in.the.evening.mp4What alternative solutions did you explore? (Optional)We can follow the same pattern used in BankAccountStep in CodesStep. This is where the user is navigated to when open 2FA page. To do that we should:
Note: We will gray out 'Download' and 'Copy' texts too. Result: Screen.Recording.2024-06-19.at.3.58.10.in.the.afternoon.mp4We can also hide the whole 2FA code section and next button to make a clear UI and to have a clean implementation. ![]() |
@trjExpensify, @techievivek Eep! 4 days overdue now. Issues have feelings too... |
@techievivek what do you think of this proposal, and I take it we can send this external? |
|
Job added to Upwork: https://www.upwork.com/jobs/~0145289401f3afe680 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr ( |
This can be handled externally. Since the user is in an unvalidated state, they shouldn't be shown the 2FA page. We can manage this on the frontend, as the backend already ensures that recovery codes are not sent. |
ProposalPlease re-state the problem that we are trying to solve in this issue.The recovery code is not visible on 2FA page (step 1). What is the root cause of that problem?We don't have a step to validate the account (if account is unvalidated) before showing the What changes do you think we should make in order to solve the problem?Add a step to validate the account (if account is unvalidated)
Few other considerations:
Those are the main steps required to enable this behavior, there could be small UX adjustments/fixes that can be dealt with in PR phase. What alternative solutions did you explore? (Optional)NA |
@shawnborton @dannymcclain @dubielzyk-expensify can you guys weigh in on the desired design for this flow. Do we want to add an additional step first before the recovery codes step is displayed for an unvalidated account? This might feel cleaner and we could style that page a bit better maybe. Alternatively, do we want to replace the box that contains the recovery codes with the error message to prompt for validation. So hiding the box seen here, and just showing the error message: ![]() The latter option is probably more consistent with the bank account flow that prompts for validation: ![]() |
I don't mind either approach, but I do kinda like how your second suggestion is similar to the wallet flow. It feels a little less clear, but a bit more consistent 🤷♂️ If we went that route, I'd probably leave the box how it is (but make the buttons disabled like in the wallet flow) and maybe add some placeholder skeletons for where the codes should be? Or something like that? Curious for others' thoughts too. |
Yeah I think the second way sounds alright too. That page is more sparse so I think an error message would stand out reasonably well |
I don't feel strongly, but I'd probably hide the box and show the error to make it clearer where the action is, but I'm happy with Danny's suggestion too |
I like the second idea, and I tend to agree with Jon here - I would just kill the box entirely and show the error message in its place. |
I have tried the diff. And it seems to be working well 🎉. Thanks @adamgrzybowski 🙇🏻♂️. I will test this thoroughly, if things will go as expected PR will ready for final review soon. Screen.Recording.2024-08-05.at.10.15.39.at.night.mp4 |
This issue has not been updated in over 15 days. @trjExpensify, @mollfpr, @techievivek, @etCoderDysto eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.23-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-08-29. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
@trjExpensify Not sure why it says I require payment here as my only involvement with this issue is that I reported it (see OP reported by) 🤷♂️ |
Same, all good though. Thanks for flagging! @mollfpr, checklist time! |
Payment Summary
BugZero Checklist (@trjExpensify)
|
No offending PR.
The regression step should be enough.
Precondition: Login with a new email and don't verify the account.
|
It would be better to say, "Login with a new public email account"(this is so because we require verification for the user if they are part of domain control). Otherwise, it looks good to me. |
Thanks! Payment summary as follows:
Luthfi, go ahead and request. @etCoderDysto, I need your Upwork profile link please. |
Offer sent! |
I have accepted the offer. Thanks! |
Paid, closing! |
$250 approved for @mollfpr |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.4.82-1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @ikevin127
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1718145760414179
Action Performed:
Precondition: Login with a new email and don't verify account.
Note: This means that you login with a new email and you're not asked to input the magic code, unless you do this manually by navigating to Settings > Profile > Contact method > click on your email to verify it.
Expected Result:
Actual Result:
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Screen.Recording.2024-06-11.at.15.36.33.mov
Recording.206.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @trjExpensifyThe text was updated successfully, but these errors were encountered: