Skip to content
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

Closed
1 of 6 tasks
m-natarajan opened this issue Jun 12, 2024 · 71 comments
Closed
1 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@m-natarajan
Copy link

m-natarajan commented Jun 12, 2024

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.

  1. Once logged in -> click on Settings > Security > Two-factor authentication.
  2. Click Copy and Download buttons.
  3. Click on Next button.

Expected Result:

  1. The recovery code should be visible on 2FA page (step 1).
  2. Upon clicking Copy the recovery code should be copied to clipboard.
  3. Upon clicking Download the recovery codes should be downloaded in a text file.

Actual Result:

  1. The recovery code is not visible on 2FA page (step 1).
  2. Upon clicking Copy nothing is copied to clipboard.
  3. Upon clicking Download the downloaded text file is empty (0 B).

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

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
  • Upwork Job URL: https://www.upwork.com/jobs/~0145289401f3afe680
  • Upwork Job ID: 1803637508828282692
  • Last Price Increase: 2024-06-20
Issue OwnerCurrent Issue Owner: @trjExpensify
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 12, 2024
Copy link

melvin-bot bot commented Jun 12, 2024

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@trjExpensify
Copy link
Contributor

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?

@techievivek techievivek self-assigned this Jun 13, 2024
@shawnborton
Copy link
Contributor

That makes sense to me.

@dannymcclain
Copy link
Contributor

Makes sense to me too.

@etCoderDysto
Copy link
Contributor

etCoderDysto commented Jun 13, 2024

Proposal

Please 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 useOnyx(ONYXKEYS.USER). Then we will use user?.validated prop to disable the 2FA menu item, hide the right icon, and alternatively display a hint text that says 'Verify your account first'.

disabled: !user?.validated,
shouldShowRightIcon: user?.validated,
hintText: !user?.validated && 'Verify your account first',

const menuItems = useMemo(() => {
const baseMenuItems = [
{
translationKey: 'twoFactorAuth.headerTitle',
icon: Expensicons.Shield,
action: waitForNavigate(() => Navigation.navigate(ROUTES.SETTINGS_2FA.getRoute())),

Note: we will fetch hintText value using translate method. And the changes concerning disabled, shouldShowRightIcon, and hintText will be made in baseMenuItems and baseMenuItems.map code blocks too.

Result:
Hint text might need some alignment improvement.

Screen.Recording.2024-06-13.at.7.37.52.in.the.evening.mp4

What 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:

  1. Fetch user data from onyx here
  2. Use user.validated prop to disable 'Download', 'Copy' and 'Next' button
  3. Use user.validated to decide, if we should display the same informative message displayed in BankAccountStep

Note: We will gray out 'Download' and 'Copy' texts too.

Result:

Screen.Recording.2024-06-19.at.3.58.10.in.the.afternoon.mp4

We can also hide the whole 2FA code section and next button to make a clear UI and to have a clean implementation.

image

Copy link

melvin-bot bot commented Jun 18, 2024

@trjExpensify, @techievivek Eep! 4 days overdue now. Issues have feelings too...

@trjExpensify
Copy link
Contributor

@techievivek what do you think of this proposal, and I take it we can send this external?

@melvin-bot melvin-bot bot removed the Overdue label Jun 18, 2024
@etCoderDysto
Copy link
Contributor

Update my proposal

  • I have added alternative solution

@techievivek techievivek added the External Added to denote the issue can be worked on by a contributor label Jun 20, 2024
@melvin-bot melvin-bot bot changed the title 2FA - No recovery codes in 2FA page for unverified account [$250] 2FA - No recovery codes in 2FA page for unverified account Jun 20, 2024
Copy link

melvin-bot bot commented Jun 20, 2024

Job added to Upwork: https://www.upwork.com/jobs/~0145289401f3afe680

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 20, 2024
Copy link

melvin-bot bot commented Jun 20, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)

@techievivek
Copy link
Contributor

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.

@nkdengineer
Copy link
Contributor

nkdengineer commented Jun 20, 2024

Proposal

Please 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 CodesStep

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)

  1. Build a new page ValidateAccountStep that will use the ValidateCodeForm like the add secondary contact flow here. That page will send the code and wait for the user to enter the codes. The implementation details can be found in the ContactMethodDetailsPage where there is a similar feature. That ValidateCodeStep will navigate to the CodeSteps upon successful validation
  2. In
    case CONST.TWO_FACTOR_AUTH_STEPS.CODES:
    , add a new step and render the CONST.TWO_FACTOR_AUTH_STEPS.VALIDATE_ACCOUNT and return the ValidateAccountStep if the currentStep is that step
  3. Connect to ONYXKEYS.ACCOUNT and in , check account?.validated. If false set currentStep to CONST.TWO_FACTOR_AUTH_STEPS.VALIDATE_ACCOUNT and set a local state hasValidateAccountStep to true (initialized as false), if not still use the CONST.TWO_FACTOR_AUTH_STEPS.CODES.
  4. Pass the hasValidateAccountStep local state above to the relevant steps here
  5. We need to update the step counter in https://github.com/Expensify/App/blob/main/src/pages/settings/Security/TwoFactorAuth/Steps/CodesStep.tsx#L52-L56 (and other steps after VALIDATE_ACCOUNT) to correctly show the steps in case of an additional VALIDATE_ACCOUNT step at the beginning. Here we can make use of the hasValidateAccountStep prop above. If it's false the step counter here stays the same, if true both step and total will be incremented by 1.

Few other considerations:

  • Should we allow going back from CodesStep to ValidateAccountStep. I believe no, as the user was already validated. So going back from CodesStep should dismiss the modal. Or we still show ValidateAccountStep but shows a message that the user was already validated

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

@trjExpensify
Copy link
Contributor

@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:

image

The latter option is probably more consistent with the bank account flow that prompts for validation:

image

@dannymcclain
Copy link
Contributor

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.

@dubielzyk-expensify
Copy link
Contributor

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

@dubielzyk-expensify
Copy link
Contributor

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

@shawnborton
Copy link
Contributor

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.

@etCoderDysto
Copy link
Contributor

I created diff with new changes. Please check if it works for you. The screen under the overlay won't change if the user opens CONTACT_METHOD_DETAILS page. backToDiff.txt

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

@etCoderDysto
Copy link
Contributor

@mollfpr PR is ready for final review 🎉

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Aug 19, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

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!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Aug 19, 2024
Copy link

melvin-bot bot commented Aug 21, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Monthly KSv2 labels Aug 22, 2024
@melvin-bot melvin-bot bot changed the title [$250] 2FA - No recovery codes in 2FA page for unverified account [HOLD for payment 2024-08-29] [$250] 2FA - No recovery codes in 2FA page for unverified account Aug 22, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Aug 22, 2024
Copy link

melvin-bot bot commented Aug 22, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Aug 22, 2024

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:

  • @mollfpr requires payment through NewDot Manual Requests
  • @etCoderDysto requires payment (Needs manual offer from BZ)
  • @ikevin127 requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented Aug 22, 2024

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:

  • [@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
  • [@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@mollfpr] Determine if we should create a regression test for this bug.
  • [@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 28, 2024
@ikevin127
Copy link
Contributor

@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) 🤷‍♂️

@trjExpensify
Copy link
Contributor

Same, all good though. Thanks for flagging!

@mollfpr, checklist time!

Copy link

melvin-bot bot commented Aug 29, 2024

Payment Summary

Upwork Job

BugZero Checklist (@trjExpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1803637508828282692/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2024
@mollfpr
Copy link
Contributor

mollfpr commented Sep 2, 2024

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

No offending PR.

[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

The regression step should be enough.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Precondition: Login with a new email and don't verify the account.

  1. Go to Settings page > Security
  2. Click on Two-factor authentication
  3. Verify it open the Two-factor authentication page with text and link to validate the account
  4. Click on verify your account here and verify your account
  5. After submit the magic code, verify it navigate to the Two-factor authentication page and showing the recovery codes.

@techievivek
Copy link
Contributor

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.

@trjExpensify
Copy link
Contributor

Thanks! Payment summary as follows:

Luthfi, go ahead and request. @etCoderDysto, I need your Upwork profile link please.

@melvin-bot melvin-bot bot removed the Overdue label Sep 2, 2024
@trjExpensify
Copy link
Contributor

Offer sent!

@etCoderDysto
Copy link
Contributor

I have accepted the offer. Thanks!

@trjExpensify
Copy link
Contributor

Paid, closing!

@garrettmknight
Copy link
Contributor

$250 approved for @mollfpr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Status: Done
Development

No branches or pull requests