diff --git a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb index 32ea41c4d2c..c1c23813a8b 100644 --- a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb +++ b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb @@ -46,11 +46,18 @@ def next_step def handle_invalid_piv_cac clear_piv_cac_information - # create new nonce for retry - create_piv_cac_nonce handle_invalid_otp(type: 'piv_cac') end + # This overrides the method in TwoFactorAuthenticatable so that we + # redirect back to ourselves rather than rendering the :show template. + # This removes the token from the address bar and preserves the error + # in the flash. + def render_show_after_invalid + flash[:error] = flash.now[:error] + redirect_to login_two_factor_piv_cac_url + end + def piv_cac_view_data { two_factor_authentication_method: two_factor_authentication_method, diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index 1d016e3fcbd..e52c79cbc77 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -13,10 +13,10 @@ class PivCacAuthenticationSetupController < ApplicationController def new if params.key?(:token) process_piv_cac_setup + elsif flash[:error_type].present? + render_error else - analytics.track_event(Analytics::USER_REGISTRATION_PIV_CAC_SETUP_VISIT) - @presenter = PivCacAuthenticationSetupPresenter.new(user_piv_cac_form) - render :new + render_prompt end end @@ -36,6 +36,17 @@ def redirect_to_piv_cac_service private + def render_prompt + analytics.track_event(Analytics::USER_REGISTRATION_PIV_CAC_SETUP_VISIT) + @presenter = PivCacAuthenticationSetupPresenter.new(user_piv_cac_form) + render :new + end + + def render_error + @presenter = PivCacAuthenticationSetupErrorPresenter.new(error: flash[:error_type]) + render :error + end + def two_factor_enabled? current_user.two_factor_enabled? end @@ -73,9 +84,9 @@ def next_step end def process_invalid_submission - @presenter = PivCacAuthenticationSetupErrorPresenter.new(user_piv_cac_form) clear_piv_cac_information - render :error + flash[:error_type] = user_piv_cac_form.error_type + redirect_to setup_piv_cac_url end def authorize_piv_cac_disable diff --git a/app/presenters/piv_cac_authentication_setup_error_presenter.rb b/app/presenters/piv_cac_authentication_setup_error_presenter.rb index 0b00fbdbb84..54e9771a5ea 100644 --- a/app/presenters/piv_cac_authentication_setup_error_presenter.rb +++ b/app/presenters/piv_cac_authentication_setup_error_presenter.rb @@ -1,6 +1,8 @@ class PivCacAuthenticationSetupErrorPresenter < PivCacAuthenticationSetupBasePresenter - def error - form.error_type + attr_accessor :error + + def initialize(error:) + @error = error end def may_select_another_certificate? diff --git a/app/views/users/piv_cac_authentication_setup/error.html.slim b/app/views/users/piv_cac_authentication_setup/error.html.slim index a0de89f5daa..62180f583e5 100644 --- a/app/views/users/piv_cac_authentication_setup/error.html.slim +++ b/app/views/users/piv_cac_authentication_setup/error.html.slim @@ -1,5 +1,3 @@ -- title @presenter.title - div class='alert alert-error' role='alert' = @presenter.title @@ -11,10 +9,9 @@ p.mt-tiny.mb3 = @presenter.description - cancel = sign_up_or_idv_no_js_link || link .mt2.pt1.border-top - - if @presenter.may_select_another_certificate? - = link_to t('forms.piv_cac_setup.choose_different_certificate'), - setup_piv_cac_url, class: 'h5' - br + = link_to t('forms.piv_cac_setup.choose_different_certificate'), + setup_piv_cac_url, class: 'h5' + br - if user_signing_up? || user_verifying_identity? - method = user_signing_up? ? :delete : :get @@ -25,5 +22,3 @@ p.mt-tiny.mb3 = @presenter.description user_signing_up: user_signing_up? - else = link_to cancel_link_text, cancel, class: 'h5' - -== javascript_pack_tag 'clipboard' diff --git a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb index 6d86cd32d07..bc0f98abbfc 100644 --- a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb @@ -102,8 +102,8 @@ expect(subject.current_user.reload.second_factor_attempts_count).to eq 1 end - it 're-renders the piv/cac entry screen' do - expect(response).to render_template(:show) + it 'redirects to the piv/cac entry screen' do + expect(response).to redirect_to login_two_factor_piv_cac_path end it 'displays flash error message' do @@ -126,8 +126,8 @@ expect(subject.current_user.reload.second_factor_attempts_count).to eq 1 end - it 're-renders the piv/cac entry screen' do - expect(response).to render_template(:show) + it 'redirects to the piv/cac entry screen' do + expect(response).to redirect_to login_two_factor_piv_cac_path end it 'displays flash error message' do diff --git a/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb b/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb index 03d9a4afbb2..bd282961ad3 100644 --- a/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb +++ b/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb @@ -111,7 +111,7 @@ context 'when redirected with an error token' do it 'renders the error template' do get :new, params: { token: bad_token } - expect(response).to render_template(:error) + expect(response).to redirect_to setup_piv_cac_path end it 'resets the piv/cac session information' do diff --git a/spec/features/users/piv_cac_management_spec.rb b/spec/features/users/piv_cac_management_spec.rb index 9a033a90408..e9918aacc36 100644 --- a/spec/features/users/piv_cac_management_spec.rb +++ b/spec/features/users/piv_cac_management_spec.rb @@ -58,6 +58,40 @@ def find_form(page, attributes) expect(user.x509_dn_uuid).to eq uuid end + scenario 'displays error for a bad piv/cac' do + stub_piv_cac_service + + sign_in_and_2fa_user(user) + visit account_path + click_link t('forms.buttons.enable'), href: setup_piv_cac_url + + expect(page).to have_link(t('forms.piv_cac_setup.submit')) + + nonce = get_piv_cac_nonce_from_link(find_link(t('forms.piv_cac_setup.submit'))) + visit_piv_cac_service(setup_piv_cac_url, + nonce: nonce, + error: 'certificate.bad') + expect(current_path).to eq setup_piv_cac_path + expect(page).to have_content(t('headings.piv_cac_setup.certificate.bad')) + end + + scenario 'displays error for an expired piv/cac' do + stub_piv_cac_service + + sign_in_and_2fa_user(user) + visit account_path + click_link t('forms.buttons.enable'), href: setup_piv_cac_url + + expect(page).to have_link(t('forms.piv_cac_setup.submit')) + + nonce = get_piv_cac_nonce_from_link(find_link(t('forms.piv_cac_setup.submit'))) + visit_piv_cac_service(setup_piv_cac_url, + nonce: nonce, + error: 'certificate.expired') + expect(current_path).to eq setup_piv_cac_path + expect(page).to have_content(t('headings.piv_cac_setup.certificate.expired')) + end + scenario "doesn't allow unassociation of a piv/cac" do stub_piv_cac_service @@ -109,7 +143,7 @@ def find_form(page, attributes) PivCacService.send(:reset_piv_cac_avaialable_agencies) end - scenario 'does not allow association of a piv/cac with an account' do + scenario "doesn't advertise association of a piv/cac with an account" do stub_piv_cac_service sign_in_and_2fa_user(user) diff --git a/spec/presenters/piv_cac_authentication_setup_error_presenter_spec.rb b/spec/presenters/piv_cac_authentication_setup_error_presenter_spec.rb index b952156ed1b..30d5095ea2a 100644 --- a/spec/presenters/piv_cac_authentication_setup_error_presenter_spec.rb +++ b/spec/presenters/piv_cac_authentication_setup_error_presenter_spec.rb @@ -1,17 +1,12 @@ require 'rails_helper' describe PivCacAuthenticationSetupErrorPresenter do - let(:presenter) { described_class.new(form) } - let(:form) do - OpenStruct.new( - error_type: error - ) - end + let(:presenter) { described_class.new(error: error) } let(:error) { 'certificate.none' } describe '#error' do it 'reflects the form' do - expect(presenter.error).to eq form.error_type + expect(presenter.error).to eq error end end