LG-12636: Show recommended PIV option for .gov/.mil email #10252
Conversation
| return [] | ||
| else | ||
| [TwoFactorAuthentication::SetUpPhoneSelectionPresenter.new(user: user)] | ||
| def show_option?(option) |
There was a problem hiding this comment.
Maybe this should be a method of the individual presenter classes? We could shift common helper methods to the base SetUpSelectionPresenter class if needed.
changelog: User-Facing Improvements, Multi-Factor Authentication, Sort setup options to recommend PIV for .gov/.mil email addresses
aduth
force-pushed
the
aduth-lg-12636-piv-mfa-order
branch
from
7d41bfd
to
06661bb
Compare
| informative: true, | ||
| class: 'margin-top-1', | ||
| ).with_content(t('two_factor_authentication.recommended')) %> | ||
| <% end %> |
There was a problem hiding this comment.
The 'Added' tag is similar to this but has the aria-hidden="true" attribute. I'm wondering if it should be on the Recommended tag also, or should it not be on the Added tag either.
There was a problem hiding this comment.
The ARIA is a little hard to follow here, but I think this is correct as-is.
- This is within a parent element that already has
aria-hidden="true" - For both elements that are
aria-hidden="true", they're technically hidden, but still referenced as part ofaria-describedbyon the input, which ensures they're announced as part of the description of the field.
Example how this announces in Safari + VoiceOver:
| def current_device_is_desktop? | ||
| !BrowserCache.parse(@user_agent).mobile? | ||
| end | ||
|
|
There was a problem hiding this comment.
I like how you refactored all of that.
Not tested or expected part of public API
| type == :piv_cac | ||
| elsif phishing_resistant_required? | ||
| phishing_resistant? | ||
| elsif desktop_only? |
There was a problem hiding this comment.
Would this line be considered as a bit of mental gymnastics? I was thinking !browser.mobile? could go into something like is_browser_mobile? for readability.
There was a problem hiding this comment.
Yeah, to the readability point, I initially tried browser.desktop? to avoid the negation, before realizing that the method doesn't actually exist and fixing in 7a1239d.
Personally I think the negation isn't too bad to warrant adding another method here.
|
Pushed a small change to include some analytics insights here in ec43d0b |
🎫 Ticket
LG-12636
🛠 Summary of changes
Recommends PIV as an MFA option in account creation for users with a ".gov" or ".mil" email:
📜 Testing Plan
Repeat the following steps with and without a ".gov" or ".mil" email:
👀 Screenshots