diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 05f6f07af0c..07aa1fbdcb5 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -16,7 +16,7 @@ def confirm result = form.submit(request.protocol, params) analytics.track_event(Analytics::WEBAUTHN_SETUP_SUBMITTED, result.to_h) if result.success? - process_valid_webauthn(form.attestation_response) + process_valid_webauthn else process_invalid_webauthn(form) end @@ -46,6 +46,7 @@ def exclude_credentials end def handle_successful_delete + create_user_event(:webauthn_key_removed) WebauthnConfiguration.where(user_id: current_user.id, id: params[:id]).destroy_all flash[:success] = t('notices.webauthn_deleted') track_delete(true) @@ -57,10 +58,12 @@ def handle_failed_delete end def track_delete(success) + counts_hash = MfaContext.new(current_user.reload).enabled_two_factor_configuration_counts_hash + analytics.track_event( Analytics::WEBAUTHN_DELETED, success: success, - mfa_options_enabled: MfaContext.new(current_user).enabled_two_factor_configurations_count + mfa_method_counts: counts_hash ) end @@ -73,9 +76,8 @@ def two_factor_enabled? MfaPolicy.new(current_user).two_factor_enabled? end - def process_valid_webauthn(attestation_response) + def process_valid_webauthn mark_user_as_fully_authenticated - create_webauthn_configuration(attestation_response) redirect_to webauthn_setup_success_url end @@ -103,16 +105,6 @@ def mark_user_as_fully_authenticated user_session[:authn_at] = Time.zone.now end - def create_webauthn_configuration(attestation_response) - credential = attestation_response.credential - public_key = Base64.strict_encode64(credential.public_key) - id = Base64.strict_encode64(credential.id) - WebauthnConfiguration.create(user_id: current_user.id, - credential_public_key: public_key, - credential_id: id, - name: params[:name]) - end - def user_already_has_a_personal_key? TwoFactorAuthentication::PersonalKeyPolicy.new(current_user).configured? end diff --git a/app/forms/webauthn_setup_form.rb b/app/forms/webauthn_setup_form.rb index 2794827ede9..776e2498a04 100644 --- a/app/forms/webauthn_setup_form.rb +++ b/app/forms/webauthn_setup_form.rb @@ -22,7 +22,12 @@ def initialize(user, user_session) def submit(protocol, params) consume_parameters(params) success = valid? && valid_attestation_response?(protocol) - FormResponse.new(success: success, errors: errors.messages) + if success + create_webauthn_configuration + create_user_event + end + + FormResponse.new(success: success, errors: errors.messages, extra: extra_analytics_attributes) end # this gives us a hook to override the domain embedded in the attestation test object @@ -61,4 +66,22 @@ def safe_response(original_origin) errors.add :name, I18n.t('errors.webauthn_setup.attestation_error') false end + + def create_webauthn_configuration + credential = attestation_response.credential + public_key = Base64.strict_encode64(credential.public_key) + id = Base64.strict_encode64(credential.id) + WebauthnConfiguration.create(user_id: user.id, + credential_public_key: public_key, + credential_id: id, + name: name) + end + + def create_user_event + Event.create(user_id: user.id, event_type: :webauthn_key_added) + end + + def extra_analytics_attributes + { mfa_method_counts: MfaContext.new(user).enabled_two_factor_configuration_counts_hash } + end end diff --git a/app/models/event.rb b/app/models/event.rb index 870faba7d79..13d468e6fc7 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -15,6 +15,8 @@ class Event < ApplicationRecord piv_cac_disabled: 11, new_personal_key: 12, personal_key_used: 13, + webauthn_key_added: 14, + webauthn_key_removed: 15, } validates :event_type, presence: true diff --git a/config/locales/event_types/en.yml b/config/locales/event_types/en.yml index 95fc412f431..6dc6d1abcca 100644 --- a/config/locales/event_types/en.yml +++ b/config/locales/event_types/en.yml @@ -17,3 +17,5 @@ en: piv_cac_disabled: PIV/CAC card unassociated piv_cac_enabled: PIV/CAC card associated usps_mail_sent: Letter sent + webauthn_key_added: Hardware security key added + webauthn_key_removed: Hardware security key removed diff --git a/config/locales/event_types/es.yml b/config/locales/event_types/es.yml index e08d03f90da..e6d3666452d 100644 --- a/config/locales/event_types/es.yml +++ b/config/locales/event_types/es.yml @@ -17,3 +17,5 @@ es: piv_cac_disabled: Tarjeta PIV/CAC no asociada piv_cac_enabled: Tarjeta PIV/CAC asociada usps_mail_sent: Carta enviada + webauthn_key_added: Clave de seguridad de hardware añadido + webauthn_key_removed: Clave de seguridad de hardware eliminada diff --git a/config/locales/event_types/fr.yml b/config/locales/event_types/fr.yml index af06082d4d2..4d9fc103f25 100644 --- a/config/locales/event_types/fr.yml +++ b/config/locales/event_types/fr.yml @@ -17,3 +17,5 @@ fr: piv_cac_disabled: Carte PIV/CAC non associée piv_cac_enabled: Carte PIV/CAC associée usps_mail_sent: Lettre envoyée + webauthn_key_added: Clé de sécurité ajoutée + webauthn_key_removed: Clé de sécurité retirée diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index 420fdc6e276..39b4cc72bdb 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -34,7 +34,7 @@ describe 'when signed in and not account creation' do before do stub_analytics - user = build(:user, personal_key: 'ABCD-DEFG-HIJK-LMNO') + user = build(:user, :signed_up, :with_authentication_app) stub_sign_in(user) end @@ -88,7 +88,7 @@ end it 'tracks the submission' do - result = { success: true, errors: {} } + result = { success: true, errors: {}, mfa_method_counts: { auth_app: 1, phone: 1 } } expect(@analytics).to receive(:track_event). with(Analytics::WEBAUTHN_SETUP_SUBMITTED, result) @@ -97,19 +97,11 @@ end describe 'delete' do - before do - mock_mfa = MfaContext.new(controller.current_user) - allow(mock_mfa).to receive(:enabled_two_factor_configurations_count).and_return(2) - allow(MfaContext).to receive(:new).with(controller.current_user).and_return(mock_mfa) - mock_mfa_policy = MfaPolicy.new(controller.current_user) - allow(mock_mfa_policy).to receive(:multiple_factors_enabled?).and_return(true) - allow(MfaPolicy).to receive(:new).with( - controller.current_user - ).and_return(mock_mfa_policy) - end - it 'deletes a webauthn configuration' do + expect(Event).to receive(:create). + with(user_id: controller.current_user.id, event_type: :webauthn_key_removed) cfg = create_webauthn_configuration(controller.current_user, 'key1', 'id1', 'foo1') + delete :delete, params: { id: cfg.id } expect(response).to redirect_to(account_url) @@ -120,7 +112,7 @@ it 'tracks the delete' do cfg = create_webauthn_configuration(controller.current_user, 'key1', 'id1', 'foo1') - result = { success: true, mfa_options_enabled: 2 } + result = { success: true, mfa_method_counts: { auth_app: 1, phone: 1 } } expect(@analytics).to receive(:track_event).with(Analytics::WEBAUTHN_DELETED, result) delete :delete, params: { id: cfg.id } @@ -129,25 +121,21 @@ end describe 'when signed in and account creation' do - before do - user = build(:user) - stub_sign_in(user) - end - describe 'patch confirm' do - let(:params) do - { + it 'processes a valid webauthn and redirects to success page' do + user = build(:user) + stub_sign_in(user) + + allow(Figaro.env).to receive(:domain_name).and_return('localhost:3000') + expect(Event).to receive(:create).with(user_id: user.id, event_type: :webauthn_key_added) + controller.user_session[:webauthn_challenge] = challenge + + params = { attestation_object: attestation_object, client_data_json: client_data_json, name: 'mykey', } - end - before do - allow(Figaro.env).to receive(:domain_name).and_return('localhost:3000') - controller.user_session[:webauthn_challenge] = challenge - end - it 'processes a valid webauthn and redirects to success page' do patch :confirm, params: params expect(response).to redirect_to(webauthn_setup_success_url) diff --git a/spec/features/users/webauthn_management_spec.rb b/spec/features/users/webauthn_management_spec.rb index 40e5a7ae53a..89ac37aed24 100644 --- a/spec/features/users/webauthn_management_spec.rb +++ b/spec/features/users/webauthn_management_spec.rb @@ -18,6 +18,11 @@ mock_press_button_on_hardware_key expect(current_path).to eq webauthn_setup_success_path + + click_button t('forms.buttons.continue') + + expect(page).to have_current_path(account_path) + expect(page).to have_content t('event_types.webauthn_key_added') end it 'gives an error if the challenge/secret is incorrect' do diff --git a/spec/forms/webauthn_setup_form_spec.rb b/spec/forms/webauthn_setup_form_spec.rb index 1011e4a21b7..f13a45d7256 100644 --- a/spec/forms/webauthn_setup_form_spec.rb +++ b/spec/forms/webauthn_setup_form_spec.rb @@ -17,9 +17,10 @@ client_data_json: client_data_json, name: 'mykey', } + extra_attributes = { mfa_method_counts: { webauthn: 1 } } expect(FormResponse).to receive(:new). - with(success: true, errors: {}).and_return(result) + with(success: true, errors: {}, extra: extra_attributes).and_return(result) expect(subject.submit(protocol, params)).to eq result end end @@ -32,9 +33,10 @@ client_data_json: client_data_json, name: 'mykey', } + extra_attributes = { mfa_method_counts: {} } expect(FormResponse).to receive(:new). - with(success: false, errors: {}).and_return(result) + with(success: false, errors: {}, extra: extra_attributes).and_return(result) expect(subject.submit(protocol, params)).to eq result end @@ -48,9 +50,10 @@ client_data_json: client_data_json, name: 'mykey', } + extra_attributes = { mfa_method_counts: {} } expect(FormResponse).to receive(:new). - with(success: false, errors: + with(success: false, extra: extra_attributes, errors: { name: [I18n.t('errors.webauthn_setup.attestation_error')] }).and_return(result) expect(subject.submit(protocol, params)).to eq result end