-
Notifications
You must be signed in to change notification settings - Fork 166
LG-428 Build 2FA selection at sign in #2317
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| module TwoFactorAuthentication | ||
| class OptionsController < ApplicationController | ||
| include TwoFactorAuthenticatable | ||
|
|
||
| def index | ||
| @two_factor_options_form = TwoFactorLoginOptionsForm.new(current_user) | ||
| @presenter = two_factor_options_presenter | ||
| analytics.track_event(Analytics::MULTI_FACTOR_AUTH_OPTION_LIST_VISIT) | ||
| end | ||
|
|
||
| def create | ||
| @two_factor_options_form = TwoFactorLoginOptionsForm.new(current_user) | ||
| result = @two_factor_options_form.submit(two_factor_options_form_params) | ||
| analytics.track_event(Analytics::MULTI_FACTOR_AUTH_OPTION_LIST, result.to_h) | ||
|
|
||
| if result.success? | ||
| process_valid_form | ||
| else | ||
| @presenter = two_factor_options_presenter | ||
| render :index | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def two_factor_options_presenter | ||
| TwoFactorLoginOptionsPresenter.new(current_user, view_context, current_sp) | ||
| end | ||
|
|
||
| def process_valid_form | ||
| factor_to_url = { | ||
| 'voice' => otp_send_url(otp_delivery_selection_form: { otp_delivery_preference: 'voice' }), | ||
| 'personal_key' => login_two_factor_personal_key_url, | ||
| 'sms' => otp_send_url(otp_delivery_selection_form: { otp_delivery_preference: 'sms' }), | ||
| 'auth_app' => login_two_factor_authenticator_url, | ||
| 'piv_cac' => login_two_factor_piv_cac_url, | ||
| } | ||
| url = factor_to_url[@two_factor_options_form.selection] | ||
| redirect_to url if url | ||
| end | ||
|
|
||
| def two_factor_options_form_params | ||
| params.require(:two_factor_options_form).permit(:selection) | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| class TwoFactorLoginOptionsForm | ||
| include ActiveModel::Model | ||
|
|
||
| attr_reader :selection | ||
|
|
||
| validates :selection, inclusion: { in: %w[voice sms auth_app piv_cac personal_key] } | ||
|
|
||
| def initialize(user) | ||
| self.user = user | ||
| end | ||
|
|
||
| def submit(params) | ||
| self.selection = params[:selection] | ||
|
|
||
| success = valid? | ||
|
|
||
| FormResponse.new(success: success, errors: errors.messages, extra: extra_analytics_attributes) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| attr_accessor :user | ||
| attr_writer :selection | ||
|
|
||
| def extra_analytics_attributes | ||
| { | ||
| selection: selection, | ||
| } | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| class AuthAppLoginOptionPolicy | ||
| def initialize(user) | ||
| @user = user | ||
| end | ||
|
|
||
| def configured? | ||
| !user.otp_secret_key.nil? | ||
| end | ||
|
|
||
| private | ||
|
|
||
| attr_reader :user | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| class PersonalKeyLoginOptionPolicy | ||
| def initialize(user) | ||
| @user = user | ||
| end | ||
|
|
||
| def configured? | ||
| user.personal_key.present? | ||
| end | ||
|
|
||
| private | ||
|
|
||
| attr_reader :user | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| class PivCacLoginOptionPolicy | ||
| def initialize(user) | ||
| @user = user | ||
| end | ||
|
|
||
| def configured? | ||
| FeatureManagement.piv_cac_enabled? && user.x509_dn_uuid.present? | ||
| end | ||
|
|
||
| private | ||
|
|
||
| attr_reader :user | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| class SmsLoginOptionPolicy | ||
| def initialize(user) | ||
| @user = user | ||
| end | ||
|
|
||
| def configured? | ||
| user.phone.present? | ||
| end | ||
|
|
||
| private | ||
|
|
||
| attr_reader :user | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| class VoiceLoginOptionPolicy | ||
| def initialize(user) | ||
| @user = user | ||
| end | ||
|
|
||
| def configured? | ||
| user_has_a_phone_number_that_we_can_call? | ||
| end | ||
|
|
||
| private | ||
|
|
||
| attr_reader :user | ||
|
|
||
| def user_has_a_phone_number_that_we_can_call? | ||
| phone = user.phone | ||
| phone.present? && !PhoneNumberCapabilities.new(phone).sms_only? | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| module TwoFactorAuthCode | ||
| class PersonalKeyPresenter < TwoFactorAuthCode::GenericDeliveryPresenter | ||
| def initialize; end | ||
|
|
||
| def help_text | ||
| '' | ||
| end | ||
|
|
||
| def fallback_question | ||
| t('two_factor_authentication.personal_key_fallback.question') | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,9 @@ | ||
| module TwoFactorAuthCode | ||
| class PhoneDeliveryPresenter < TwoFactorAuthCode::GenericDeliveryPresenter | ||
| attr_reader( | ||
| :otp_delivery_preference | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor style comment: Any reason not to put this in a single line without the parentheses?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The private attrs had the same format. When in Rome...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, that's because there were too many of them to fit on one line. Following what Rubocop recommends for one-line conditionals, we should keep things on one line if they fit on one line IMO. |
||
|
|
||
| def header | ||
| t('devise.two_factor_authentication.header_text') | ||
| end | ||
|
|
@@ -10,19 +14,19 @@ def phone_number_message | |
| expiration: Figaro.env.otp_valid_for) | ||
| end | ||
|
|
||
| def fallback_question | ||
| t('two_factor_authentication.phone_fallback.question') | ||
| end | ||
|
|
||
| def help_text | ||
| t("instructions.mfa.#{otp_delivery_preference}.confirm_code_html", | ||
| resend_code_link: resend_code_link) | ||
| '' | ||
| end | ||
|
|
||
| def fallback_links | ||
| [ | ||
| otp_fallback_options, | ||
| update_phone_link, | ||
| piv_cac_option, | ||
| personal_key_link, | ||
| account_reset_link, | ||
| ].compact | ||
| def update_phone_link | ||
| return unless unconfirmed_phone | ||
|
|
||
| link = view.link_to(t('forms.two_factor.try_again'), reenter_phone_number_path) | ||
| t('instructions.mfa.wrong_number_html', link: link) | ||
| end | ||
|
|
||
| def cancel_link | ||
|
|
@@ -43,95 +47,15 @@ def cancel_link | |
| :reenter_phone_number_path, | ||
| :phone_number, | ||
| :unconfirmed_phone, | ||
| :otp_delivery_preference, | ||
| :account_reset_token, | ||
| :confirmation_for_phone_change, | ||
| :voice_otp_delivery_unsupported, | ||
| :confirmation_for_idv | ||
| ) | ||
|
|
||
| def otp_fallback_options | ||
| if totp_enabled | ||
| otp_fallback_options_with_totp | ||
| elsif !voice_otp_delivery_unsupported | ||
| safe_join([phone_fallback_link, '.']) | ||
| end | ||
| end | ||
|
|
||
| def otp_fallback_options_with_totp | ||
| if voice_otp_delivery_unsupported | ||
| safe_join([auth_app_fallback_tag, '.']) | ||
| else | ||
| safe_join([phone_fallback_link, auth_app_fallback_link]) | ||
| end | ||
| end | ||
|
|
||
| def update_phone_link | ||
| return unless unconfirmed_phone | ||
|
|
||
| link = view.link_to(t('forms.two_factor.try_again'), reenter_phone_number_path) | ||
| t('instructions.mfa.wrong_number_html', link: link) | ||
| end | ||
|
|
||
| def account_reset_link | ||
| return if unconfirmed_phone || !FeatureManagement.account_reset_enabled? | ||
| account_reset_or_cancel_link | ||
| end | ||
|
|
||
| def account_reset_or_cancel_link | ||
| if account_reset_token | ||
| t('devise.two_factor_authentication.account_reset.pending_html', cancel_link: | ||
| view.link_to(t('devise.two_factor_authentication.account_reset.cancel_link'), | ||
| account_reset_cancel_url(token: account_reset_token))) | ||
| else | ||
| t('devise.two_factor_authentication.account_reset.text_html', link: | ||
| view.link_to(t('devise.two_factor_authentication.account_reset.link'), | ||
| account_reset_request_path(locale: LinkLocaleResolver.locale))) | ||
| end | ||
| end | ||
|
|
||
| def phone_fallback_link | ||
| t(fallback_instructions, link: phone_link_tag) | ||
| end | ||
|
|
||
| def phone_link_tag | ||
| view.link_to( | ||
| t("links.two_factor_authentication.#{fallback_method}"), | ||
| otp_send_path(locale: LinkLocaleResolver.locale, otp_delivery_selection_form: | ||
| { otp_delivery_preference: fallback_method }) | ||
| ) | ||
| end | ||
|
|
||
| def auth_app_fallback_link | ||
| t('links.phone_confirmation.auth_app_fallback_html', link: auth_app_fallback_tag) | ||
| end | ||
|
|
||
| def auth_app_fallback_tag | ||
| view.link_to( | ||
| t('links.two_factor_authentication.app'), | ||
| login_two_factor_authenticator_path(locale: LinkLocaleResolver.locale) | ||
| ) | ||
| end | ||
|
|
||
| def fallback_instructions | ||
| "instructions.mfa.#{otp_delivery_preference}.fallback_html" | ||
| end | ||
|
|
||
| def fallback_method | ||
| if otp_delivery_preference == 'voice' | ||
| 'sms' | ||
| elsif otp_delivery_preference == 'sms' | ||
| 'voice' | ||
| end | ||
| end | ||
|
|
||
| def resend_code_link | ||
| view.link_to( | ||
| t("links.two_factor_authentication.resend_code.#{otp_delivery_preference}"), | ||
| otp_send_path(locale: LinkLocaleResolver.locale, | ||
| otp_delivery_selection_form: | ||
| { otp_delivery_preference: otp_delivery_preference, resend: true }) | ||
| ) | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@monfresh These changes are all out of scope for this PR. I'm handling this refactoring in parallel work, as we already discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. See my comment in one of your refactoring PRs.