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

Include recovery codes #1164

Merged
merged 13 commits into from
Aug 11, 2023
Merged

Include recovery codes #1164

merged 13 commits into from
Aug 11, 2023

Conversation

ArtOfCode-
Copy link
Member

Fixes #266

Adds recovery codes for 2FA accounts. This instructs the user to save their recovery code before enabling 2FA.

Signing in with a recovery code disables 2FA on the account so the user can reconfigure it with a new app.

Also includes an email and script to set recovery codes on existing 2FA accounts, which can be manually run.

@ArtOfCode- ArtOfCode- requested a review from a team August 3, 2023 20:26
@cellio
Copy link
Member

cellio commented Aug 3, 2023

If I understand correctly, the "we've added codes" email is effectively sending a password in the clear in email not expected by the recipient. (As opposed to when you make a request and you're monitoring what's coming in.) Could we, instead, give the user a way to look up the code while logged in?

@trichoplax
Copy link
Contributor

Signing in with a recovery code disables 2FA on the account so the user can reconfigure it with a new app.

If a user considers the security of their account sufficiently important to switch on 2FA, could we avoid disabling it until they explicitly request it? Is it possible to make the reconfiguration with a new app the step that disables the previous 2FA, rather than having a period during which there is no 2FA?

@trichoplax
Copy link
Contributor

Could we, instead, give the user a way to look up the code while logged in?

I like the idea of this, perhaps combined with the ability to send an email saying "Here is where you can find your recovery codes". The page displaying the recovery codes could perhaps also have a <details> section to avoid displaying them immediately, similar to the recent change to the mobile sign in QR code page.

@ArtOfCode-
Copy link
Member Author

Is it possible to make the reconfiguration with a new app the step that disables the previous 2FA, rather than having a period during which there is no 2FA?

Not as easily. It'd be doable, but it's a bit of a pain to carry that state through and ensure the user actually goes through with resetting 2FA - easier to just disable it with a reminder to reconfigure it.

@ArtOfCode-
Copy link
Member Author

Could we, instead, give the user a way to look up the code while logged in?

Done - I've changed the email to direct the user to where to find it, and added the code to the 2FA settings behind a <details> section.

Copy link
Contributor

@Taeir Taeir left a comment

Choose a reason for hiding this comment

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

I tested the current implementation and the backup code is shown in a nice way during the setup process and can be accessed again with a 2FA code.

However, when you set the two factor method to be email, then you cannot access the backup code because it asks to enter a code, which one cannot do via email. (Also not sure if the backup code is as applicable there, maybe we just don't want to have this feature for email because it is not applicable and hide the option entirely?)

@Taeir
Copy link
Contributor

Taeir commented Aug 4, 2023

It also seems that I cannot enter a backup code because it contains letters and the UI wants me to input a number only. We need to remove that restriction (the feature works when I change the 2FA code field to be of type text rather than number).

app/views/two_factor_mailer/backup_code.html.erb Outdated Show resolved Hide resolved
scripts/create_backup_2fa_codes.rb Outdated Show resolved Hide resolved
I misunderstood how deliver_later works - I thought it was just async
but actually need an ActiveJob config that we don't have.
@ArtOfCode-
Copy link
Member Author

Should all be fixed.

Copy link
Contributor

@Taeir Taeir left a comment

Choose a reason for hiding this comment

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

Tested locally, now correctly is disabled with email sign ins. LGTM

@Taeir Taeir merged commit c142a8f into develop Aug 11, 2023
2 checks passed
@Taeir Taeir deleted the art/266-recovery-codes branch August 11, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling 2FA should include some recovery codes
4 participants