Recovery code support for webauthn #3859
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3859 +/- ##
==========================================
- Coverage 98.79% 98.79% -0.01%
==========================================
Files 216 216
Lines 5404 5394 -10
==========================================
- Hits 5339 5329 -10
Misses 65 65
|
| @@ -13,7 +13,7 @@ def create | |||
|
|
|||
| if @user && (@user.mfa_enabled? || @user.webauthn_credentials.any?) | |||
| setup_webauthn_authentication(form_url: webauthn_create_session_path, session_options: { "user" => @user.id }) | |||
There was a problem hiding this comment.
session_options: { "user" => @user.id }
could we use session[:mfa_user] instead now in webauthn_create?
(Can punt this to later)
There was a problem hiding this comment.
Yeah I think we can rely on session[:mfa_user] instead. But will save that for a separate issue.
There was a problem hiding this comment.
Is there a difference between mfa_prompt and this view? We could possibly consolidate the two views into one if there aren't any differences.
(Can punt this to later)
There was a problem hiding this comment.
No there isn't. The only difference is prompt has a hard-coded callback, whereas mfa_prompt requires you to pass one through. I chatted about this with Betty, and we think we should switch to using just mfa_prompt so we don't have two of essentially the same file. But, that change is outside the scope of this PR.
| <% if @user.totp_enabled? %> | ||
| <%= label_tag :otp, t('multifactor_auths.otp_or_recovery'), class: 'form__label' %> | ||
| <%= text_field_tag :otp, '', class: 'form__input', autofocus: true, autocomplete: :off %> | ||
| <% elsif @user.webauthn_only_with_recovery? %> |
There was a problem hiding this comment.
Hot take: could we render this even if there aren't any recovery codes? A user could have generated codes and used them all... I don't think we should be expose any details of if the user has any codes.
There was a problem hiding this comment.
Chatted about this with Jenny, and we decided to keep it the way it is - specifically to not show the recovery code prompt if the user doesn't have any. The justification for this, is that it takes up half of the page, and would do so while serving the user no functionality. So if a user doesn't have recovery codes, it would just be confusing to have half the page prompting them for it. The simplest design philosophy for users is not showing them stuff that is irrelevant to them, which is what we are doing.
| <%= label_tag :otp, t('multifactor_auths.otp_or_recovery'), class: 'form__label' %> | ||
| <%= text_field_tag :otp, '', class: 'form__input', autofocus: true, autocomplete: :off %> | ||
| <% elsif @user.webauthn_only_with_recovery? %> | ||
| <%= label_tag :otp, t('multifactor_auths.recovery_code'), class: 'form__label' %> |
There was a problem hiding this comment.
nit: do we need the label tag here? It looks a bit redundant that the heading is also Recovery code
There was a problem hiding this comment.
I believe for accessibility it's better to keep the label tag there.
There was a problem hiding this comment.
I think the label will still be associated with the text field even if hidden?
There was a problem hiding this comment.
Herm ... I agree with Jenny. It does feel redundant 😅 . I think Eric & I had noted this while prototyping too. Staring at this longer, I'm inclined to advise dropping the label as well.
From an a11y standpoint, we can remove the label and add the aria-label attribute on the input (e.g. aria-label="Recovery code").
ericherscovich
force-pushed
the
eric/authn-recovery-codes
branch
from
8df7e38
to
e439db9
Compare
ericherscovich
force-pushed
the
eric/authn-recovery-codes
branch
3 times, most recently
from
441d039
to
426a15b
Compare
There was a problem hiding this comment.
It seems there are few last bits needed to be tweaked on UI and wording side. Feel free to merge once you're happy @jenshenny @bettymakes.
add form hidden css
Address false positive in password controller test
Linting remove unused css Remove spaces in css file Remove useless setup_multifactor_methods Fix misaligned end in prompt Fix tests up Linting
Fix tests and before_action
jenshenny
force-pushed
the
eric/authn-recovery-codes
branch
from
426a15b
to
9537bd5
Compare
and add an aria-label to the input for accessibility
jenshenny
force-pushed
the
eric/authn-recovery-codes
branch
from
9537bd5
to
42fe8ef
Compare
What problem are you solving?
This PR allows recovery codes to be used with webauthn. If a user generates webauthn credentials after #3821 was merged, or has OTP enabled, they will have recovery codes which can be used. On both prompts, this functionality is now supported.
If a user has webauthn only enabled, and has recovery codes, this is what they will see:
Contributes to #3800
What approach did you choose and why?
There is a case where a webauthn user wouldn't have recovery codes. That is, if they had webauthn only enabled before #3821 was merged. So, there is protection in rendering, in order to not render the recovery code prompt if a user does not have these recovery codes. The recovery code field simply uses the existing OTP field.