Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ def must_be_in_development
end

def token_from_params
error, subject = params.slice(:error, :subject)
error = params[:error]
subject = params[:subject]

if error.present?
with_nonce(error: error).to_json
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def create
private

def confirm_two_factor_enabled
return if confirmation_context? || current_user.two_factor_enabled?
return if confirmation_context? || current_user.phone_enabled?

redirect_to phone_setup_url
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ class PivCacAuthenticationSetupController < ApplicationController
include UserAuthenticator
include PivCacConcern

before_action :confirm_two_factor_authenticated
before_action :authenticate_user!
before_action :confirm_two_factor_authenticated, if: :two_factor_enabled?
before_action :authorize_piv_cac_setup, only: :new
before_action :authorize_piv_cac_disable, only: :delete

Expand All @@ -30,6 +31,10 @@ def delete

private

def two_factor_enabled?
current_user.two_factor_enabled?
end

def process_piv_cac_setup
result = user_piv_cac_form.submit
analytics.track_event(Analytics::USER_REGISTRATION_PIV_CAC_ENABLED, result.to_h)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ class TwoFactorAuthenticationController < ApplicationController
def show
if current_user.totp_enabled?
redirect_to login_two_factor_authenticator_url
elsif current_user.two_factor_enabled?
elsif current_user.phone_enabled?
validate_otp_delivery_preference_and_send_code
elsif current_user.piv_cac_enabled?
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.

Should the last 2 conditions be reversed, so that if someone has both PIV/CAC and phone, it defaults to PIV/CAC?

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.

I was going for minimal touch on this. If doing so doesn't result in a mass of tests breaking, I don't see why I wouldn't. Let me see what happens.

redirect_to login_two_factor_piv_cac_url
else
redirect_to two_factor_options_url
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ class TwoFactorAuthenticationSetupController < ApplicationController

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

Expand All @@ -18,12 +19,17 @@ def create
if result.success?
process_valid_form
else
@presenter = two_factor_options_presenter
render :index
end
end

private

def two_factor_options_presenter
TwoFactorOptionsPresenter.new(current_user, current_sp)
end

def authorize_2fa_setup
if user_fully_authenticated?
redirect_to account_url
Expand All @@ -38,6 +44,8 @@ def process_valid_form
redirect_to phone_setup_url
when 'auth_app'
redirect_to authenticator_setup_url
when 'piv_cac'
redirect_to setup_piv_cac_url
end
end

Expand Down
4 changes: 4 additions & 0 deletions app/forms/two_factor_options_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ def submit(params)
FormResponse.new(success: success, errors: errors.messages, extra: extra_analytics_attributes)
end

def selected?(type)
type == (selection || 'sms')
end

private

attr_accessor :user
Expand Down
4 changes: 4 additions & 0 deletions app/models/service_provider.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ def live?
active? && approved?
end

def piv_cac_available?
PivCacService.piv_cac_available_for_agency?(agency)
end

private

def redirect_uris_are_parsable
Expand Down
13 changes: 7 additions & 6 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,22 +57,23 @@ def confirm_piv_cac?(proposed_uuid)
end

def piv_cac_enabled?
x509_dn_uuid.present?
FeatureManagement.piv_cac_enabled? && x509_dn_uuid.present?
end

def piv_cac_available?
FeatureManagement.piv_cac_enabled? && (
piv_cac_enabled? ||
identities.any?(&:piv_cac_available?)
)
piv_cac_enabled? || identities.any?(&:piv_cac_available?)
end

def need_two_factor_authentication?(_request)
two_factor_enabled?
end

def phone_enabled?
phone.present?
end

def two_factor_enabled?
phone.present? || totp_enabled? || piv_cac_enabled?
phone_enabled? || totp_enabled? || piv_cac_enabled?
end

def send_two_factor_authentication_code(_code)
Expand Down
49 changes: 49 additions & 0 deletions app/presenters/two_factor_options_presenter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
class TwoFactorOptionsPresenter
include ActionView::Helpers::TranslationHelper

attr_reader :current_user, :service_provider

def initialize(current_user, sp)
@current_user = current_user
@service_provider = sp
end

def title
t('titles.two_factor_setup')
end

def heading
t('devise.two_factor_authentication.two_factor_choice')
end

def info
t('devise.two_factor_authentication.two_factor_choice_intro')
end

def label
t('forms.two_factor_choice.legend') + ':'
end

def options
available_2fa_types.map do |type|
OpenStruct.new(
type: type,
label: t("devise.two_factor_authentication.two_factor_choice_options.#{type}"),
info: t("devise.two_factor_authentication.two_factor_choice_options.#{type}_info"),
selected: type == :sms
)
end
end

private

def available_2fa_types
%w[sms voice auth_app] + piv_cac_if_available
end

def piv_cac_if_available
return [] if current_user.piv_cac_enabled?
return [] unless current_user.piv_cac_available? || service_provider&.piv_cac_available?
%w[piv_cac]
end
end
43 changes: 14 additions & 29 deletions app/views/users/two_factor_authentication_setup/index.html.slim
Original file line number Diff line number Diff line change
@@ -1,40 +1,25 @@
- title t('titles.two_factor_setup')
- title @presenter.title

h1.h3.my0 = t('devise.two_factor_authentication.two_factor_choice')
p.mt-tiny.mb3
= t('devise.two_factor_authentication.two_factor_choice_intro')
h1.h3.my0 = @presenter.heading
p.mt-tiny.mb3 = @presenter.info

= simple_form_for(@two_factor_options_form,
html: { autocomplete: 'off', role: 'form' },
method: :patch,
url: two_factor_options_path) do |f|
.mb3
fieldset.m0.p0.border-none.
legend.mb1.h4.serif.bold = t('forms.two_factor_choice.legend') + ':'
label.btn-border.col-12.mb1 for="two_factor_options_form_selection_sms"
.radio
= radio_button_tag 'two_factor_options_form[selection]', :sms, true
span.indicator.mt-tiny
span.blue.bold.fs-20p
= t('devise.two_factor_authentication.two_factor_choice_options.sms')
.regular.gray-dark.fs-10p.mb-tiny
= t('devise.two_factor_authentication.two_factor_choice_options.sms_info')
label.btn-border.col-12.mb1 for="two_factor_options_form_selection_voice"
.radio
= radio_button_tag 'two_factor_options_form[selection]', :voice, false
span.indicator.mt-tiny
span.blue.bold.fs-20p
= t('devise.two_factor_authentication.two_factor_choice_options.voice')
.regular.gray-dark.fs-10p.mb-tiny
= t('devise.two_factor_authentication.two_factor_choice_options.voice_info')
label.btn-border.col-12.mb1 for="two_factor_options_form_selection_auth_app"
.radio
= radio_button_tag 'two_factor_options_form[selection]', :auth_app, false
span.indicator.mt-tiny
span.blue.bold.fs-20p
= t('devise.two_factor_authentication.two_factor_choice_options.auth_app')
.regular.gray-dark.fs-10p.mb-tiny
= t('devise.two_factor_authentication.two_factor_choice_options.auth_app_info')
legend.mb1.h4.serif.bold = @presenter.label
- @presenter.options.each do |option|
label.btn-border.col-12.mb1 for="two_factor_options_form_selection_#{option.type}"
.radio
= radio_button_tag('two_factor_options_form[selection]',
option.type,
@two_factor_options_form.selected?(option.type))
span.indicator.mt-tiny
span.blue.bold.fs-20p = option.label
.regular.gray-dark.fs-10p.mb-tiny = option.info

= f.button :submit, t('forms.buttons.continue')

= render 'shared/cancel', link: destroy_user_session_path
2 changes: 2 additions & 0 deletions config/application.yml.example
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ development:
otp_valid_for: '10'
password_pepper: 'f22d4b2cafac9066fe2f4416f5b7a32c6942d82f7e00740c7594a095fa8de8db17c05314be7b18a5d6dd5683e73eadf6cc95aa633e5ad9a701edb95192a6a105'
password_strength_enabled: 'true'
piv_cac_agencies: '["Test Government Agency"]'
piv_cac_enabled: 'true'
piv_cac_service_url: 'https://localhost:8443/'
piv_cac_verify_token_url: 'https://localhost:8443/'
Expand Down Expand Up @@ -366,6 +367,7 @@ test:
otp_valid_for: '10'
password_pepper: 'f22d4b2cafac9066fe2f4416f5b7a32c6942d82f7e00740c7594a095fa8de8db17c05314be7b18a5d6dd5683e73eadf6cc95aa633e5ad9a701edb95192a6a105'
password_strength_enabled: 'false'
piv_cac_agencies: '["Test Government Agency"]'
piv_cac_enabled: 'true'
piv_cac_service_url: 'https://localhost:8443/'
piv_cac_verify_token_url: 'https://localhost:8443/'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,16 @@ def index
end

describe '#show' do
context 'when user is piv/cac enabled' do
it 'renders the piv/cac entry screen' do
stub_sign_in_before_2fa(build(:user))
allow(subject.current_user).to receive(:piv_cac_enabled?).and_return(true)
get :show

expect(response).to redirect_to login_two_factor_piv_cac_path
end
end

context 'when user is TOTP enabled' do
it 'renders the :confirm_totp view' do
stub_sign_in_before_2fa(build(:user))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,20 @@
end
end

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

patch :create, 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
Expand Down
36 changes: 36 additions & 0 deletions spec/features/users/sign_up_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,42 @@
expect(page).to have_current_path account_path
end

it 'does not allow a user to choose piv/cac as 2FA method during sign up' do
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.

Should we also add a controller test to make sure a user can't visit the piv_cac endpoint directly when piv_cac_available_for_agency? is false?

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.

I thought about it, but I'm not too worried. If someone does manage to get their piv/cac to work, it's okay. The main reason for showing only when they're associated with a supported agency is to avoid confusion for the majority of government employees we don't support yet.

allow(PivCacService).to receive(:piv_cac_available_for_agency?).and_return(false)
allow(FeatureManagement).to receive(:piv_cac_enabled?).and_return(true)
begin_sign_up_with_sp_and_loa(loa3: false)

expect(page).to have_current_path two_factor_options_path
expect(page).not_to have_content(
t('devise.two_factor_authentication.two_factor_choice_options.piv_cac')
)
end

context 'when piv/cac is allowed' do
it 'allows a user to choose piv/cac as 2FA method during sign up' do
allow(PivCacService).to receive(:piv_cac_available_for_agency?).and_return(true)
allow(FeatureManagement).to receive(:piv_cac_enabled?).and_return(true)

begin_sign_up_with_sp_and_loa(loa3: false)

expect(page).to have_current_path two_factor_options_path
expect(page).to have_content(
t('devise.two_factor_authentication.two_factor_choice_options.piv_cac')
)
end

it 'directs to the piv/cac setup page' do
allow(PivCacService).to receive(:piv_cac_available_for_agency?).and_return(true)
allow(FeatureManagement).to receive(:piv_cac_enabled?).and_return(true)

begin_sign_up_with_sp_and_loa(loa3: false)

expect(page).to have_current_path two_factor_options_path
select_2fa_option('piv_cac')
expect(page).to have_current_path setup_piv_cac_path
end
end

it 'does not bypass 2FA when accessing authenticator_setup_path if the user is 2FA enabled' do
user = create(:user, :signed_up)
sign_in_user(user)
Expand Down
Loading