Prevent unintentional backup code regeneration

[iandunn]

Fixes #244

This removes the local storage item, and instead uses the number of remaining backup codes to determine if the provider has already been setup. That comes from the database, so it spans browsers/devices/sessions.

I wasn't able to reproduce the bug mentioned in #221, so I don't think this will cause a regression, but I'd appreciate some more eyes to double check.

I initially ran into the bug from #217, but was able to fix that by refreshing the user when navigating to Account Status.

[iandunn]

This looks like it regresses the solution in #217, so I'm looking into that.

[iandunn]

I think 93df040 fixes the #217 regression.

[renintw]

Screen.Capture.on.2023-09-19.at.03-22-49.mov

The first half of the video is testing #217 (Unable to see the first set of backup codes after enabling the 2FA). It seems that after completing the activation of 2FA, the initial set of backup codes still can't be seen.
The second half of the video (starting from switching to the Profile and then switching back) is testing #221 (When leaving the account page and returning to the backup codes page, a new set of backup codes is regenerated without asking). There's no regression in this part, it doesn't automatically generate new backup codes after leaving the account page and returning.

Regarding the issue with #217, it seems that the regression is not from the changes made here. It's already like this in the current production.

Update

I think 93df040 fixes the #217 regression.

I found that this commit didn't seem to handle this issue: (Unable to see the first set of backup codes after enabling the 2FA). What's the issue it's trying to address? Is it making the account screen show the correct status?

[iandunn]

🤔 I can't reproduce the problem in the video. This is what I'm doing:

1. wp user meta list iandunn-test |grep factor
2. wp user meta delete iandunn-test _two_factor_backup_codes. Sometimes I've seen multiple _two_factor_backup_codes entries in the database, I think because React's Strict Mode causes the hook to fire twice. So you need to make sure you delete both.
3. wp user meta delete iandunn-test _two_factor_totp_key
4. Open https://wordpress.org/support/users/iandunn-test/edit/account/
5. Enable TOTP

When it navigates to the Backup Codes screen, it generates new codes like normal.

Can you check that you don't have any backup codes saved previously? The entry on AccountStatus may just be disabled b/c a primary provider isn't enabled, rather than because there aren't any codes saved.

If that isn't it, can you see anything that might be going wrong?

What's the issue it's trying to address? Is it making the account screen show the correct status?

#244 has a description of the problem and reproduction steps

[iandunn]

I rebased to fix the merge conflict, but still can't reproduce the problem you saw.

[iandunn]

What's the issue it's trying to address? Is it making the account screen show the correct status?

#244 has a description of the problem and reproduction steps

Er, nevermind, I realize now that you were asking about 93df040 rather than the overall PR. IIRC, the regression that I saw before 93df040 was that visiting the Backup Codes screen would regenerate new codes every time, rather than recognizing that they had been saved previously and displaying the Manage screen.

[renintw]

🤔 I can't reproduce the problem in the video. This is what I'm doing:

1. wp user meta list iandunn-test |grep factor
2. wp user meta delete iandunn-test _two_factor_backup_codes. Sometimes I've seen multiple _two_factor_backup_codes entries in the database, I think because React's Strict Mode causes the hook to fire twice. So you need to make sure you delete both.
3. wp user meta delete iandunn-test _two_factor_totp_key
4. Open https://wordpress.org/support/users/iandunn-test/edit/account/
5. Enable TOTP

When it navigates to the Backup Codes screen, it generates new codes like normal.

Following your steps worked for me. Before, I always went through Dashboard > Users > Profile to uncheck the Enabled option under Two-Factor Options. It seems that unchecking Recovery Codes there doesn't actually delete the _two_factor_backup_codes from the database. 🤔

visiting the Backup Codes screen would regenerate new codes every time, rather than recognizing that they had been saved previously and displaying the Manage screen.

I've figured out why it was initially set up that way:

The original intention was for the backup codes badge in the account status to show as "enabled" only when a user checks "I have printed or saved these codes". If it's not checked, the badge would consistently display as "not enabled", reminding the user that they haven't noted down the backup codes somewhere. But I believe in many actual use cases, users might see the generated backup codes and save them, then directly hit "Back". This would leave them confused as to why the status is "not enabled". If they go into the backup codes screen again, new codes are generated. They might hit "Back" again and again... until they realize they need to check "I have printed or saved these codes" to actually enable it.

So, I think the current process is better: If users note it down and then press "Back", the status shows as "enabled". Or if they don't note it down and press "Back", the status still displays as "enabled", but they can return and generate new codes.

Given this, I feel the "I have printed or saved these codes" option may no longer be that necessary?

[renintw]

Apart from a small follow-up question and an issue that might be related to an upstream repo, as mentioned in the comment, everything looks good to me. 👍

[iandunn]

Given this, I feel the "I have printed or saved these codes" option may no longer be that necessary?

🤔 I think it's still helpful, to reinforce to the user that it's really important to write them down.

Once WordPress/two-factor#507 if fixed, then we can update our client to not save the codes until the user clicks "All Finished". I created #256 to track that.

[StevenDufresne]

I've reverted this PR as it's causing a couple issues:

• It creates some problems with buttons appearing to be pressed before they are listening to the refresh state
• It forces components to re-render too often introducing fatal errors (password view)

[renintw]

It creates some problems with buttons appearing to be pressed before they are listening to the refresh state
It forces components to re-render too often introducing fatal errors (password view)

I think they are both reflecting the same situation - when pressing 'Back' to return to the Account Screen, it would first do refreshRecord on the current screen and then go back. Since it's using refreshRecord(userRecord); here, it tries to save with an empty password on the password screen, triggering zxcvbn and causing an error.

I guess this can be solved by not refreshing all records, but only the backup codes part.

[iandunn]

Thanks for catching that! I think #257 will fix it, do you mind checking it out?