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

Fix backup codes screen status not updated if go back without checkbox confirmed #205

Conversation

renintw
Copy link
Contributor

@renintw renintw commented May 31, 2023

Fixes #190

This PR ensures that users cannot leave the Backup Codes screen by pressing Back without checking I have printed or saved these codes, and at the same time, prompt a Notice to remind users that they should confirm the checkbox before proceeding with other actions.

ScreenNavigation was extracted was done to make the subsequent commits easier to modify and read.

hasPrinted was extracted to useUser so that navigateToScreen can access it.

Screencast

No other errors in BackupCodes component exist

1.mp4

Other errors in BackupCodes component exist

2.mp4

Testing Steps

  1. Test in a Sandbox.
  2. enable 2fa.
  3. Go to backup codes and you can't Back if you don't confirm the checkbox, and an error Notice should prompt.
  4. Go to wp-content/plugins/two-factor/providers/class-two-factor-backup-codes.php and make it always throw an error on L295.
  5. Go to backup codes and you can click Back.

@renintw renintw self-assigned this May 31, 2023
@renintw renintw force-pushed the add/handle-revalidation-error-in-a-single-place branch from a3582a0 to 516f972 Compare June 6, 2023 20:44
@renintw renintw force-pushed the fix/backup-codes-screen-status-not-updated branch 2 times, most recently from f55f1b0 to cb9296b Compare June 6, 2023 22:40
@renintw renintw force-pushed the add/handle-revalidation-error-in-a-single-place branch from 516f972 to 32139c3 Compare June 6, 2023 23:19
@renintw renintw force-pushed the add/handle-revalidation-error-in-a-single-place branch from 32139c3 to 494358f Compare June 6, 2023 23:22
@renintw renintw force-pushed the fix/backup-codes-screen-status-not-updated branch from cb9296b to 8b03b2e Compare June 6, 2023 23:23
@renintw renintw force-pushed the fix/backup-codes-screen-status-not-updated branch from e56c49a to 663d824 Compare June 6, 2023 23:59
@renintw renintw marked this pull request as ready for review June 7, 2023 00:11
@renintw renintw added bug Something isn't working ui Related to user interface labels Jun 7, 2023
Copy link
Member

@iandunn iandunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I have some reservations about this approach.

It seems like there's two things going on here:

  1. The superficial issue is that the UI isn't updated automatically to reflect the current state after calling generatedCodes()
  2. The deeper issues is that the upstream plugin implicitly saves/enables backup codes to the database before the user intends, but that's not how the design was created.

IMO, the design is correct, and the codes shouldn't be active until the user confirms they want to save them. That gives the user a choice whether or not they want to activate them, and makes sure they're aware that the codes are active.

This PR does make the user aware that they're active, but I worry that forcing them to stay on the screen will feel like a frustrating UX. It also doesn't achieve the goal of giving them a choice to cancel. Code-wide, I feels like this may add more complexity than we need.

I thought of a few potential alternatives:

  1. Fix the UI state mismatch by calling refreshRecord() after the codes are generated

    -generateCodes();
    +await generateCodes();
    +await refreshRecord();
  2. Remove the confirm checkbox from the UI for now, and replace it with a message indicating the codes are active. Then when Backup codes are saved before user intends two-factor#507 is fixed, we could bring it back.

  3. Write our own API endpoint to generate backup codes, instead of calling the upstream API from the client. It could call the upstream endpoint internally, and then disable the backup codes immediately after. Then, when the user clicks confirm, our client would send another XHR to enable them.

If #1 would work, then that feels to me like the simplest option. We could also consider #2 if we want to go further.

I don't feel strongly though. What do you think @renintw and @adamwoodnz ?

settings/src/script.js Outdated Show resolved Hide resolved
@renintw
Copy link
Contributor Author

renintw commented Jun 8, 2023

but I worry that forcing them to stay on the screen will feel like a frustrating UX

I guess I wasn't aware of the case that if a user had already activated and saved a set of backup-codes, and then accidentally clicked the generate new backup codes button, or clicked it intentionally but then decided they didn't want new codes, this would be a frustrating UX like you said. In this case, the issue shouldn't be addressed by approach in this PR.

I'll open new PRs for #1 and #2, and extract the refactoring part in this PR that I think might be worth retaining.

Thanks for the feedback 🙏

Update

The problem seems to be solved in a different way in #217 and once WordPress/two-factor#507 is fixed, this issue The user should be able to enter the screen to see the status and regenerate codes, though. mentioned here could be fixed by changing this Line to disabled={ ! backupCodesEnabled && ! totpEnabled}

For extract the refactoring part, please see #215.

@renintw renintw force-pushed the add/handle-revalidation-error-in-a-single-place branch from 494358f to 8cca504 Compare June 8, 2023 11:54
@renintw
Copy link
Contributor Author

renintw commented Jun 8, 2023

Closed. See #217 for potential solution.

@renintw renintw closed this Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ui Related to user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants