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 unable to see the first set of backup code - using localStroage #221

Merged
merged 3 commits into from
Jun 26, 2023

Conversation

renintw
Copy link
Contributor

@renintw renintw commented Jun 22, 2023

Fixes #216
Clear comparison without disturbance from reverting: 2f6c51e...fix/Unable-to-see-the-first-set-of-backup-code

Before deploying #217, more cases were tested and a missing scenario was found: when leaving the account page and returning, the isSetupFinished state gets reset (as the 2FA component is unmounted), which leads to the direct generation of new backup codes upon entering the backup-codes screen again. Initially, I didn't want to use localStorage due to the concerns mentieond below. However, it appears an efficient way to reoslve this issue.

But if we feel that the troubles caused by these concerns outweigh the inconvenience of not being able to see the first set of codes and considering that the upstream issue #507 can reach a consensus and be fixed soon, then I believe no modifications are necessary for addressing this issue.

Concerns about localStorage

  • localStorage has no expiry date: We will need to remove the key from localStorage in our next release by using localStorage.removeItem('key') once we don't the key anymore.
  • If a user chooses to clear browser storage data, such as cookies and site data, information in localStorage will also be cleared.
  • LocalStorage is bound to a specific browser. The state will disappear if switching browsers.

@renintw renintw changed the base branch from trunk to refactor/structure-components-refactoring-and-test-update June 22, 2023 18:49
Base automatically changed from refactor/structure-components-refactoring-and-test-update to trunk June 22, 2023 18:49
@renintw renintw self-assigned this Jun 22, 2023
@renintw renintw added bug Something isn't working priority: high labels Jun 22, 2023
@renintw renintw requested a review from StevenDufresne June 22, 2023 19:56
@renintw renintw marked this pull request as ready for review June 22, 2023 19:58
@StevenDufresne
Copy link
Contributor

I'm uncomfortable with automatic code generation based on this property alone although I do support this as a hotfix. I wouldn't want users to have their backup codes regenerated without their knowledge, or in the confusing state as it appears to be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to see the first set of backup codes after completing the 2FA application
2 participants