Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
9acd946
refactor two factor options to use partial, create mfa setup path
jmdembe Apr 28, 2022
924c5f0
refactor mfa setup screen using partials and pass through path
jmdembe Apr 28, 2022
4a5a48f
Fix tests and add title to mfa setup screen
jmdembe Apr 29, 2022
e24fe7b
address lint errors
jmdembe Apr 29, 2022
9393561
fix tests again
jmdembe Apr 29, 2022
94a08c6
move mfa setup path behind a the mfa selection feature flag
jmdembe Apr 29, 2022
0303ffb
fix tests again
jmdembe Apr 29, 2022
cc11056
Merge branch 'main' into jd-finish-sad-path
jmdembe Apr 29, 2022
ff93ed1
change redirect to use confirmation path
jmdembe Apr 29, 2022
4c9d201
Merge branch 'main' into jd-finish-sad-path
jmdembe May 3, 2022
d7687c6
Merge branch 'main' into jd-finish-sad-path
jmdembe May 3, 2022
f799faa
WIP: add test for mfa selection controller
jmdembe May 3, 2022
bb72d51
fix indentation
jmdembe May 3, 2022
d710d84
WIP: change tests for mfa selection controller
jmdembe May 3, 2022
6b35b5e
write get test for mfa selection controller
jmdembe May 4, 2022
e4d8cd0
fix lint error
jmdembe May 4, 2022
0d103ac
update controller action
jmdembe May 4, 2022
87ae88c
change action, update test
jmdembe May 5, 2022
2ae8a1d
Merge branch 'main' into jd-finish-sad-path
jmdembe May 5, 2022
fca7307
Merge branch 'main' into jd-finish-sad-path
jmdembe May 6, 2022
cb2ff9a
address style comments for spec
jmdembe May 6, 2022
25fb7bb
Update spec/controllers/users/mfa_selection_controller_spec.rb
jmdembe May 6, 2022
50f0d77
undo removal of analytics test
jmdembe May 6, 2022
0e0c0ba
Merge branch 'main' into jd-finish-sad-path
jmdembe May 9, 2022
880782d
Merge branch 'main' into jd-finish-sad-path
jmdembe May 9, 2022
21a4581
reconcile changes with force selecting mfa option
jmdembe May 9, 2022
25f05a0
fix test for reconciled partial
jmdembe May 10, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions app/controllers/users/mfa_selection_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
module Users
class MfaSelectionController < ApplicationController
include UserAuthenticator
include MfaSetupConcern

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we ensure user is authenticated.
We also want to ensure that users arrive at this page properly.

before_action :authenticate_user before_action :confirm_user_authenticated_for_2fa_setup

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we ensure user is authenticated. We also want to ensure that users arrive at this page properly.

before_action :authenticate_user before_action :confirm_user_authenticated_for_2fa_setup

Was this suggestion implemented?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it had been originally, but I believe what happened is that with a merge that I had to make with another PR that it got lost in a merge somewhere. I created a patch in LG-6236

def index
@two_factor_options_form = TwoFactorOptionsForm.new(current_user)
@presenter = two_factor_options_presenter
analytics.track_event(Analytics::USER_REGISTRATION_2FA_SETUP_VISIT)
end

def update
result = submit_form
analytics.track_event(Analytics::USER_REGISTRATION_2FA_SETUP, result.to_h)

if result.success?
process_valid_form
else
@presenter = two_factor_options_presenter
render :index
end
end

private

def submit_form
@two_factor_options_form = TwoFactorOptionsForm.new(current_user)
@two_factor_options_form.submit(two_factor_options_form_params)
end

def two_factor_options_presenter
TwoFactorOptionsPresenter.new(
user_agent: request.user_agent,
user: current_user,
aal3_required: service_provider_mfa_policy.aal3_required?,
piv_cac_required: service_provider_mfa_policy.piv_cac_required?,
)
end

def process_valid_form
user_session[:selected_mfa_options] = @two_factor_options_form.selection
redirect_to confirmation_path(user_session[:selected_mfa_options].first)
end

def two_factor_options_form_params
params.require(:two_factor_options_form).permit(:selection, selection: [])
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ def process_valid_form

def confirm_user_needs_2fa_setup
return unless mfa_policy.two_factor_enabled?
return if params.has_key?(:multiple_mfa_setup)
return if service_provider_mfa_policy.user_needs_sp_auth_method_setup?
redirect_to after_mfa_setup_path
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,4 @@
</div>
<% end %>
</div>
<%= javascript_packs_tag_once('mfa_selection_component') %>
<%= javascript_packs_tag_once('mfa_selection_component') %>
2 changes: 1 addition & 1 deletion app/views/sign_up/completions/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
<%= render(AlertComponent.new(type: :warning, class: 'margin-bottom-4')) do %>
<%= link_to(
t('mfa.second_method_warning.link'),
two_factor_options_url(multiple_mfa_setup: ''),
mfa_setup_path,
) %>
<%= t('mfa.second_method_warning.text') %>
<% end %>
Expand Down
33 changes: 33 additions & 0 deletions app/views/users/mfa_selection/index.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<%= title t('two_factor_authentication.two_factor_choice') %>

<%= render PageHeadingComponent.new.with_content(t('two_factor_authentication.two_factor_choice')) %>

<% if IdentityConfig.store.select_multiple_mfa_options %>
<%= render AlertComponent.new(type: :info, class: 'margin-bottom-4') do %>
<%= t('mfa.info') %>
<% end %>
<% end %>

<%= validated_form_for @two_factor_options_form,
html: { autocomplete: 'off' },
method: :patch,
url: mfa_setup_path do |f| %>
<div class="margin-bottom-4">
<fieldset class="margin-0 padding-0 border-0">
<legend class="margin-bottom-2 usa-sr-only"><%= @presenter.intro %></legend>
<% @presenter.options.each do |option| %>
<div id="<%= "select_#{option.type}" %>" class="<%= option.html_class %>">
<%= render partial: 'partials/multi_factor_authentication/mfa_selection',
locals: { form: f, option: option } %>
</div>
<% end %>
</fieldset>
</div>

<%= f.button :submit, t('forms.buttons.continue'), class: 'usa-button--big usa-button--wide margin-bottom-1' %>
<% end %>

<%= render 'shared/cancel', link: destroy_user_session_path %>

<%= javascript_packs_tag_once('webauthn-unhide') %>

Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@

<p class="maxw-mobile-lg margin-bottom-0"><%= @presenter.intro %></p>

<%# If it is decided that there will be an info banner on this page for MFA setup, the text will need Gengo translations %>

<% if IdentityConfig.store.select_multiple_mfa_options %>
<%= render AlertComponent.new(type: :info, class: 'margin-bottom-4') do %>
<%= t('mfa.info') %>
Expand All @@ -32,7 +30,7 @@
<% @presenter.options.each do |option| %>
<div id="<%= "select_#{option.type}" %>" class="<%= option.html_class %>">
<% if IdentityConfig.store.select_multiple_mfa_options %>
<%= render partial: 'mfa_selection_component',
<%= render partial: 'partials/multi_factor_authentication/mfa_selection',
locals: { form: f, option: option } %>
<% else %>
<%= radio_button_tag(
Expand Down
2 changes: 2 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,8 @@
get '/otp/send' => 'users/two_factor_authentication#send_code'
get '/two_factor_options' => 'users/two_factor_authentication_setup#index'
patch '/two_factor_options' => 'users/two_factor_authentication_setup#create'
get '/mfa_setup' => 'users/mfa_selection#index'
patch '/mfa_setup' => 'users/mfa_selection#update'
get '/phone_setup' => 'users/phone_setup#index'
patch '/phone_setup' => 'users/phone_setup#create'
get '/aal3_required' => 'users/aal3#show'
Expand Down
157 changes: 157 additions & 0 deletions spec/controllers/users/mfa_selection_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
require 'rails_helper'

describe Users::MfaSelectionController do
let(:current_sp) { create(:service_provider) }

describe '#index' do
before do
allow(IdentityConfig.store).to receive(:select_multiple_mfa_options).and_return(true)
user = build(:user, :signed_up)
stub_sign_in(user)
end

context 'when the user is using one authenticator option' do
it 'shows the mfa setup screen' do
controller.user_session[:selected_mfa_options] = ['backup_code']

get :index

expect(response).to render_template(:index)
end
end
end

describe '#update' do
it 'submits the TwoFactorOptionsForm' do
user = build(:user)
stub_sign_in_before_2fa(user)

voice_params = {
two_factor_options_form: {
selection: 'voice',
},
}
params = ActionController::Parameters.new(voice_params)
response = FormResponse.new(success: true, errors: {}, extra: { selection: ['voice'] })

form = instance_double(TwoFactorOptionsForm)
allow(TwoFactorOptionsForm).to receive(:new).with(user).and_return(form)
expect(form).to receive(:submit).
with(params.require(:two_factor_options_form).permit(:selection)).
and_return(response)
expect(form).to receive(:selection).and_return(['voice'])

patch :update, params: voice_params
end

context 'when the selection is phone' do
it 'redirects to phone setup page' do
stub_sign_in_before_2fa

patch :update, params: {
two_factor_options_form: {
selection: 'phone',
},
}

expect(response).to redirect_to phone_setup_url
end
end

context 'when multi selection with phone first' do
it 'redirects properly' do
stub_sign_in_before_2fa
patch :update, params: {
two_factor_options_form: {
selection: ['phone', 'auth_app'],
},
}

expect(response).to redirect_to phone_setup_url
end
end

context 'when multi selection with auth app first' do
it 'redirects properly' do
stub_sign_in_before_2fa
patch :update, params: {
two_factor_options_form: {
selection: ['auth_app', 'phone', 'webauthn'],
},
}

expect(response).to redirect_to authenticator_setup_url
end
end

context 'when the selection is auth_app' do
it 'redirects to authentication app setup page' do
stub_sign_in_before_2fa

patch :update, params: {
two_factor_options_form: {
selection: 'auth_app',
},
}

expect(response).to redirect_to authenticator_setup_url
end
end

context 'when the selection is webauthn' do
it 'redirects to webauthn setup page' do
stub_sign_in_before_2fa

patch :update, params: {
two_factor_options_form: {
selection: 'webauthn',
},
}

expect(response).to redirect_to webauthn_setup_url
end
end

context 'when the selection is webauthn platform authenticator' do
it 'redirects to webauthn setup page with the platform param' do
stub_sign_in_before_2fa

patch :update, params: {
two_factor_options_form: {
selection: 'webauthn_platform',
},
}

expect(response).to redirect_to webauthn_setup_url(platform: true)
end
end

context 'when the selection is piv_cac' do
it 'redirects to piv/cac setup page' do
stub_sign_in_before_2fa

patch :update, params: {
two_factor_options_form: {
selection: 'piv_cac',
},
}

expect(response).to redirect_to setup_piv_cac_url
end
end

context 'when the selection is not valid' do
it 'renders index page' do
stub_sign_in_before_2fa

patch :update, params: {
two_factor_options_form: {
selection: 'foo',
},
}

expect(response).to render_template(:index)
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
it 'submits the TwoFactorOptionsForm' do
user = build(:user)
stub_sign_in_before_2fa(user)
stub_analytics

voice_params = {
two_factor_options_form: {
Expand All @@ -74,6 +75,8 @@
expect(form).to receive(:selection).and_return(['voice'])

patch :create, params: voice_params

expect(@analytics).to have_logged_event(Analytics::USER_REGISTRATION_2FA_SETUP, response.to_h)
end

it 'tracks analytics event' do
Expand Down
9 changes: 5 additions & 4 deletions spec/features/multi_factor_authentication/mfa_cta_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,12 @@
it 'redirects user to select additional authentication methods' do
visit_idp_from_sp_with_ial1(:oidc)
sign_up_and_set_password
select_2fa_option('backup_code')
check t('two_factor_authentication.two_factor_choice_options.backup_code')
click_continue
expect(page).to have_current_path(sign_up_completed_path)
click_on(t('mfa.second_method_warning.link'))
expect(page).to have_content(t('two_factor_authentication.two_factor_choice'))

set_up_mfa_with_backup_codes
click_link(t('mfa.second_method_warning.link'))
expect(page).to have_current_path(mfa_setup_path)
end
end
end
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

describe 'users/two_factor_authentication_setup/_mfa_selection_component.html.erb' do
describe 'partials/multi_factor_authentication/_mfa_selection.html.erb' do
include SimpleForm::ActionViewExtensions::FormHelper
include Devise::Test::ControllerHelpers

Expand All @@ -13,7 +13,7 @@
end

subject(:rendered) do
render partial: 'mfa_selection_component', locals: {
render partial: 'mfa_selection', locals: {
form: form_builder,
option: presenter.options[4],
}
Expand Down