Fix unable to see the first set of backup codes after enabling the 2FA #217

renintw · 2023-06-08T22:52:55Z

Fixes #216, and the behavior of #190 is changed.

This PR adds isSetupFinished to record.
It could fix the issue - Unable to see the first set of backup codes after enabling the 2FA,
and also somewhat change the problem raised in #190, as the flow after the fix here is something like

If users don't click the All finish button and go back to account screen -> the backup code button status is updated as enabled and clickable (This wasn't fixed by this PR, and actually this is related to the issue of this PR - the root cause for "as soon as the codes are generated, the screen gets directed to Manage screen" is that the backupCodesEnabled here has already been set to true, and this makes the issue in Handle the case that If "I have printed or saved these codes" in backupCodes is not clicked #190 go away. i.e., the backup code button status is updated as enabled and clickable)
However, even though the issue from Handle the case that If "I have printed or saved these codes" in backupCodes is not clicked #190 is resolved, another similar problem has popped up. The status on the account-status screen should not be set to enabled. This is because, after the fix implemented in this PR, there might be some confusion. For instance, (1) if users don't click All finish and directly navigate back to the account screen, they would see that the backup codes are enabled. This could be misleading. Similarly, (2) if users have already activated their backup codes once, they might out of curiosity or by accident click the generate new codes button. At this point, since they don't really intend to generate new codes, they might just click Back to leave the screen, or even log out, thinking they could still use the backup codes that they activated earlier - which they couldn't. Therefore, the status shouldn't be set to enabled, but at the same time, it shouldn't prevent users from clicking it as well. We could handle this in another PR.
If users click the All finish button and go back to account screen -> the status is correctly updated (enabled and clickable)

For more details about flow please see the screencast below.

This workaround may not seem ideal, but other methods like adding state or using the apiFetch middleware don't really handle all the cases quite well. Once the upstream PR is fixed, this can be removed. At that point, we will have the ability to decide when backupCodesEnabled should be enabled and control the state.

Screencast

Screen.Capture.on.2023-06-09.at.05-50-48.mp4

Testing Step

Set up 2FA while sandboxed.
When 2FA is successfully activated, see if it redirects to backup codes screen and stays there, if so, this issue is fixed.

renintw · 2023-06-08T22:56:05Z

settings/src/components/backup-codes.js

-	} );
+		userRecord.record.isSetupFinished = true;
+		// The codes have already been saved to usermeta, see `generateCodes()` above.
+		await refreshRecord( userRecord ); // This has the intended side-effect of redirecting to the Manage screen.


Not so sure about the meaning of the comment in this line, does it make sense to remove it now?

StevenDufresne · 2023-06-21T01:18:06Z

accident click the generate new codes button

Accidentally clicking a button, provided it's a standard button, isn't something to worry about. I think anytime that button is clicked, it's fair and expected that new codes are created. I don't think we need to complicate things by having extra confirmations provided we have recourse for unblocking locked accounts (which we do).

I would argue that we can always show the backup codes on the code view (we have them saved right?). I'm not quite sure why we treat them as temporary and never show them again?

renintw · 2023-06-21T08:23:56Z

I would argue that we can always show the backup codes on the code view (we have them saved right?). I'm not quite sure why we treat them as temporary and never show them again?

I'm not so sure about how it was decided as is in the first place, and I couldn't easily find a best practice for this argument. So I asked chatGPT about it, and here's what it said, it seems to make sense:

Two-factor authentication (2FA) backup codes are typically shown to the user only once, at the time they are generated. The idea is that the user should make a copy of these codes and keep them in a safe place, separate from their primary device. This is a security measure to prevent unauthorized access to the backup codes.

If the backup codes were always visible in the user interface, then anyone who managed to gain access to your account could also see and use these backup codes. This would defeat the purpose of having a second factor of authentication.

Therefore, it's standard practice to treat backup codes as temporary and only show them once. If the user loses their backup codes, they should have an option to generate a new set, which will invalidate the old ones. This way, if the old codes were discovered by someone else, they could not be used to access the account.

That being said, I can see that Google always shows the backup codes like you mentioned and put the re-generate button on the same page. But I also can remember there are some services hiding the codes right away after users leave the page.

Accidentally clicking a button, provided it's a standard button, isn't something to worry about. I think anytime that button is clicked, it's fair and expected that new codes are created. I don't think we need to complicate things by having extra confirmations provided we have recourse for unblocking locked accounts (which we do).

This was brought up here, and it was confirmed as an edge case. If we're not changing to always show the backup codes on the code view, I assume it's still a good-to-have feature, but with low priority, though.

renintw · 2023-06-21T10:36:55Z

Any further concerns about not merging this PR? If not, I'll squash merge it first and we can come back to revert it anytime we'd like to. Changes here are rather simple, only isSetupFinished is added, and there's a clear TODO comment about it.

StevenDufresne · 2023-06-22T00:04:39Z

If the backup codes were always visible in the user interface, then anyone who managed to gain access to your account could also see and use these backup codes. This would defeat the purpose of having a second factor of authentication

I think this is mostly incorrect, if a bad actor can access the backup codes, they can also access the regenerate button which kills the backup codes and gives them new ones and since we don't have logging you wouldn't know. That is a bigger topic thought.

Also, Github also shows them provided you re-auth.

At any rate, I'm okay with this to merge in the meantime.

…g the 2FA (#217)" This reverts commit 0cbad52.

iandunn · 2023-06-22T23:16:58Z

In our case at least, it's not possible to show the backup codes after the initial generation, because they're hashed before being stored in the database, similar to passwords.

…221) * Revert "Fix unable to see the first set of backup codes after enabling the 2FA (#217)" This reverts commit 0cbad52. * add hasSetupCompleted * Add {browser: true} to fix eslint error

renintw requested review from iandunn and adamwoodnz June 8, 2023 22:53

renintw self-assigned this Jun 8, 2023

renintw added the bug Something isn't working label Jun 8, 2023

renintw commented Jun 8, 2023

View reviewed changes

renintw force-pushed the fix/Unable-to-see-the-first-set-of-backup-codes branch from c66c577 to 9abb760 Compare June 8, 2023 23:03

renintw mentioned this pull request Jun 8, 2023

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

Closed

renintw force-pushed the fix/Unable-to-see-the-first-set-of-backup-codes branch from 9abb760 to 2ed2b4e Compare June 9, 2023 15:04

renintw added 2 commits June 23, 2023 01:02

add isSetupFinished to record

c0dc977

Add comments

f016d99

renintw force-pushed the fix/Unable-to-see-the-first-set-of-backup-codes branch from 2ed2b4e to f016d99 Compare June 22, 2023 17:02

renintw merged commit 0cbad52 into trunk Jun 22, 2023

renintw deleted the fix/Unable-to-see-the-first-set-of-backup-codes branch June 22, 2023 17:05

renintw restored the fix/Unable-to-see-the-first-set-of-backup-codes branch June 22, 2023 17:57

renintw deleted the fix/Unable-to-see-the-first-set-of-backup-codes branch June 22, 2023 18:11

renintw added a commit that referenced this pull request Jun 22, 2023

Revert "Fix unable to see the first set of backup codes after enablin…

8bd33aa

…g the 2FA (#217)" This reverts commit 0cbad52.

renintw mentioned this pull request Jun 22, 2023

Fix unable to see the first set of backup code - using localStroage #221

Merged

iandunn mentioned this pull request Sep 14, 2023

Prevent unintentional backup code regeneration #245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unable to see the first set of backup codes after enabling the 2FA #217

Fix unable to see the first set of backup codes after enabling the 2FA #217

renintw commented Jun 8, 2023 •

edited

Loading

renintw Jun 8, 2023

StevenDufresne commented Jun 21, 2023

renintw commented Jun 21, 2023 •

edited

Loading

renintw commented Jun 21, 2023 •

edited

Loading

StevenDufresne commented Jun 22, 2023

iandunn commented Jun 22, 2023

Fix unable to see the first set of backup codes after enabling the 2FA #217

Fix unable to see the first set of backup codes after enabling the 2FA #217

Conversation

renintw commented Jun 8, 2023 • edited Loading

Screencast

Testing Step

renintw Jun 8, 2023

Choose a reason for hiding this comment

StevenDufresne commented Jun 21, 2023

renintw commented Jun 21, 2023 • edited Loading

renintw commented Jun 21, 2023 • edited Loading

StevenDufresne commented Jun 22, 2023

iandunn commented Jun 22, 2023

renintw commented Jun 8, 2023 •

edited

Loading

renintw commented Jun 21, 2023 •

edited

Loading

renintw commented Jun 21, 2023 •

edited

Loading