diff --git a/app/assets/stylesheets/components/_btn.scss b/app/assets/stylesheets/components/_btn.scss index a08edb375bf..e84a3215c71 100644 --- a/app/assets/stylesheets/components/_btn.scss +++ b/app/assets/stylesheets/components/_btn.scss @@ -75,3 +75,19 @@ border-color: $gray; color: $gray; } + +.btn-account-action { + border: 0; + color: $blue; + font-size: .8125rem; + font-weight: normal; + margin-bottom: -3px; + margin-top: -3px; + padding: .5rem; + padding-bottom: .125rem; + padding-top: .125rem; + + a { + text-decoration: none; + } +} diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 08db60a38a0..f2a771d4068 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -19,8 +19,36 @@ def confirm end end + def delete + if current_user.total_mfa_options_enabled > 1 + handle_successful_delete + else + handle_failed_delete + end + redirect_to account_url + end + private + def handle_successful_delete + WebauthnConfiguration.where(user_id: current_user.id, id: params[:id]).destroy_all + flash[:success] = t('notices.webauthn_deleted') + track_delete(true) + end + + def handle_failed_delete + flash[:error] = t('errors.webauthn_setup.delete_last') + track_delete(false) + end + + def track_delete(success) + analytics.track_event( + Analytics::WEBAUTHN_DELETED, + success: success, + mfa_options_enabled: current_user.total_mfa_options_enabled + ) + end + def save_challenge_in_session credential_creation_options = ::WebAuthn.credential_creation_options user_session[:webauthn_challenge] = credential_creation_options[:challenge].bytes.to_a diff --git a/app/models/user.rb b/app/models/user.rb index bc59f4b9dc3..3166f42bf4c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -69,7 +69,8 @@ def need_two_factor_authentication?(_request) end def two_factor_enabled? - phone_configurations.any?(&:mfa_enabled?) || totp_enabled? || piv_cac_enabled? + phone_configurations.any?(&:mfa_enabled?) || totp_enabled? || piv_cac_enabled? || + webauthn_configurations.any? end def send_two_factor_authentication_code(_code) @@ -159,5 +160,14 @@ def send_custom_confirmation_instructions(id = nil, instructions = nil) send_devise_notification(:confirmation_instructions, @raw_confirmation_token, opts) end + + def total_mfa_options_enabled + total = [phone_mfa_enabled?, piv_cac_enabled?, totp_enabled?].count { |tf| tf } + total + webauthn_configurations.size + end + + def phone_mfa_enabled? + phone_configurations.any?(&:mfa_enabled?) + end end # rubocop:enable Rails/HasManyOrHasOneDependent diff --git a/app/models/webauthn_configuration.rb b/app/models/webauthn_configuration.rb index 8b69a5de172..7f99ec0bc7d 100644 --- a/app/models/webauthn_configuration.rb +++ b/app/models/webauthn_configuration.rb @@ -1,5 +1,5 @@ class WebauthnConfiguration < ApplicationRecord - belongs_to :user, inverse_of: :webauthn_configuration + belongs_to :user validates :user_id, presence: true validates :name, presence: true validates :credential_id, presence: true diff --git a/app/services/analytics.rb b/app/services/analytics.rb index fa41fdd51b4..7bd725a87bb 100644 --- a/app/services/analytics.rb +++ b/app/services/analytics.rb @@ -134,6 +134,7 @@ def browser USER_REGISTRATION_PIV_CAC_DISABLED = 'User Registration: piv cac disabled'.freeze USER_REGISTRATION_PIV_CAC_ENABLED = 'User Registration: piv cac enabled'.freeze USER_REGISTRATION_PIV_CAC_SETUP_VISIT = 'User Registration: piv cac setup visited'.freeze + WEBAUTHN_DELETED = 'WebAuthn Deleted'.freeze WEBAUTHN_SETUP_VISIT = 'WebAuthn Setup Visited'.freeze WEBAUTHN_SETUP_SUBMITTED = 'WebAuthn Setup Submitted'.freeze # rubocop:enable Metrics/LineLength diff --git a/app/view_models/account_show.rb b/app/view_models/account_show.rb index d7c72547ed4..45fb06b4db7 100644 --- a/app/view_models/account_show.rb +++ b/app/view_models/account_show.rb @@ -68,10 +68,6 @@ def piv_cac_partial end end - def webauthn_partial - 'accounts/actions/add_webauthn' - end - def manage_personal_key_partial yield if decorated_user.password_reset_profile.blank? end diff --git a/app/views/accounts/_webauthn.html.slim b/app/views/accounts/_webauthn.html.slim new file mode 100644 index 00000000000..3e480dc8b39 --- /dev/null +++ b/app/views/accounts/_webauthn.html.slim @@ -0,0 +1,15 @@ +.clearfix.border-top.border-blue-light + .p2.col.col-12 + .col.col-6.bold + = t('account.index.webauthn') + .right-align.col.col-6 + .btn.btn-account-action.rounded-lg.bg-light-blue + = link_to t('account.index.webauthn_add'), webauthn_setup_url +- current_user.webauthn_configurations.each do |cfg| + .p2.col.col-12.border-top.border-blue-light + .col.col-8.sm-6.truncate + = cfg.name + .col.col-4.sm-6.right-align + = button_to(t('account.index.webauthn_delete'), webauthn_setup_path(id: cfg.id), + method: :delete, class: 'btn btn-link') + .clearfix diff --git a/app/views/accounts/actions/_add_webauthn.html.slim b/app/views/accounts/actions/_add_webauthn.html.slim deleted file mode 100644 index 120fb67e9ec..00000000000 --- a/app/views/accounts/actions/_add_webauthn.html.slim +++ /dev/null @@ -1,3 +0,0 @@ -= link_to webauthn_setup_url do - span.hide = t('account.index.webauthn') - = t('forms.buttons.enable') diff --git a/app/views/accounts/show.html.slim b/app/views/accounts/show.html.slim index 43d733a0b1e..ebceab75168 100644 --- a/app/views/accounts/show.html.slim +++ b/app/views/accounts/show.html.slim @@ -46,10 +46,7 @@ h1.hide = t('titles.account') action: @view_model.totp_partial - if FeatureManagement.webauthn_enabled? - = render 'account_item', - name: t('account.index.webauthn'), - content: content_tag(:em, @view_model.totp_content), - action: @view_model.webauthn_partial + = render 'webauthn' - if current_user.piv_cac_available? = render 'account_item', diff --git a/app/views/users/webauthn_setup/new.html.slim b/app/views/users/webauthn_setup/new.html.slim index a271def3b13..0d09bfe4baa 100644 --- a/app/views/users/webauthn_setup/new.html.slim +++ b/app/views/users/webauthn_setup/new.html.slim @@ -31,7 +31,7 @@ ul.list-reset = hidden_field_tag :webauthn_public_key, '', id: 'webauthn_public_key' = hidden_field_tag :attestation_object, '', id: 'attestation_object' = hidden_field_tag :client_data_json, '', id: 'client_data_json' - = text_field_tag :name, '', required: true, pattern: '[A-Za-z0-9]*', id: 'name', + = text_field_tag :name, '', required: true, id: 'name', class: 'block col-12 field monospace', size: 16, maxlength: 20, 'aria-labelledby': 'totp-label' .col.col-6.sm-col-5.px1 diff --git a/config/locales/account/en.yml b/config/locales/account/en.yml index 3ddd24856ca..17476875454 100644 --- a/config/locales/account/en.yml +++ b/config/locales/account/en.yml @@ -26,6 +26,8 @@ en: reactivate_button: Enter the code you received via US mail success: Your account has been verified. webauthn: Hardware security key + webauthn_add: "+ Add hardware security key" + webauthn_delete: Remove items: delete_your_account: Delete your account personal_key: Personal key diff --git a/config/locales/account/es.yml b/config/locales/account/es.yml index bea02a24af6..b32a03c4e7d 100644 --- a/config/locales/account/es.yml +++ b/config/locales/account/es.yml @@ -26,6 +26,8 @@ es: reactivate_button: Ingrese el código que recibió por correo postal. success: Su cuenta ha sido verificada. webauthn: Clave de seguridad de hardware + webauthn_add: "+ Agregar clave de seguridad de hardware" + webauthn_delete: Retirar items: delete_your_account: Eliminar su cuenta personal_key: Clave personal diff --git a/config/locales/account/fr.yml b/config/locales/account/fr.yml index cd76489de60..1212575cb64 100644 --- a/config/locales/account/fr.yml +++ b/config/locales/account/fr.yml @@ -28,6 +28,8 @@ fr: reactivate_button: Entrez le code que vous avez reçu par la poste success: Votre compte a été vérifié. webauthn: Clé de sécurité matérielle + webauthn_add: "+ Ajouter une clé de sécurité matérielle" + webauthn_delete: Retirer items: delete_your_account: Supprimer votre compte personal_key: Clé personnelle diff --git a/config/locales/errors/en.yml b/config/locales/errors/en.yml index 2e37d79d262..aab2f394d27 100644 --- a/config/locales/errors/en.yml +++ b/config/locales/errors/en.yml @@ -52,6 +52,7 @@ en: letter for a new code. weak_password: Your password is not strong enough. %{feedback} webauthn_setup: + delete_last: Sorry, you can not remove your last MFA option. general_error: There was an error adding your hardward security key. Please try again. unique_name: That name is already taken. Please choose a different name. diff --git a/config/locales/errors/es.yml b/config/locales/errors/es.yml index 5aa1c6e15e4..45e05e16ae1 100644 --- a/config/locales/errors/es.yml +++ b/config/locales/errors/es.yml @@ -47,6 +47,7 @@ es: usps_otp_expired: NOT TRANSLATED YET weak_password: Su contraseña no es suficientemente segura. %{feedback} webauthn_setup: + delete_last: Lo sentimos, no puedes eliminar tu última opción de MFA. general_error: Hubo un error al agregar su clave de seguridad de hardware. Inténtalo de nuevo. unique_name: El nombre ya fue escogido. Por favor, elija un nombre diferente. diff --git a/config/locales/errors/fr.yml b/config/locales/errors/fr.yml index bf6afc1e7fb..5de9f9da324 100644 --- a/config/locales/errors/fr.yml +++ b/config/locales/errors/fr.yml @@ -49,6 +49,7 @@ fr: usps_otp_expired: NOT TRANSLATED YET weak_password: Votre mot de passe n'est pas assez fort. %{feedback} webauthn_setup: + delete_last: Désolé, vous ne pouvez pas supprimer votre dernière option MFA general_error: Une erreur s'est produite lors de l'ajout de votre clé de sécurité matérielle. Veuillez réessayer. unique_name: Ce nom est déjà pris. Veuillez choisir un autre nom. diff --git a/config/locales/notices/en.yml b/config/locales/notices/en.yml index 60d0ac6051a..72abeff218c 100644 --- a/config/locales/notices/en.yml +++ b/config/locales/notices/en.yml @@ -46,5 +46,6 @@ en: link: use a different email address text_html: Or, %{link} webauthn_added: You added a hardware security key. + webauthn_deleted: You deleted a hardware security key. session_timedout: We signed you out. For your security, %{app} ends your session when you haven’t moved to a new page for %{minutes} minutes. diff --git a/config/locales/notices/es.yml b/config/locales/notices/es.yml index 0ce7829e1be..d19462fb9ba 100644 --- a/config/locales/notices/es.yml +++ b/config/locales/notices/es.yml @@ -46,5 +46,6 @@ es: link: use un email diferente text_html: O %{link} webauthn_added: Agregaste una clave de seguridad de hardware. + webauthn_deleted: Has eliminado una clave de seguridad de hardware. session_timedout: Hemos terminado su sesión. Para su seguridad, %{app} cierra su sesión cuando usted no pasa a una nueva página durante %{minutes} minutos. diff --git a/config/locales/notices/fr.yml b/config/locales/notices/fr.yml index 9d840ab589e..3a39856f34b 100644 --- a/config/locales/notices/fr.yml +++ b/config/locales/notices/fr.yml @@ -48,6 +48,7 @@ fr: link: utilisez une adresse courriel différente text_html: Or, %{link} webauthn_added: Vous avez ajouté une clé de sécurité matérielle. + webauthn_deleted: Vous avez supprimé une clé de sécurité matérielle. session_timedout: Nous vous avons déconnecté. Pour votre sécurité, %{app} désactive votre session lorsque vous demeurez sur une page sans vous déplacer pendant %{minutes} minutes. diff --git a/config/routes.rb b/config/routes.rb index e5218c15fd6..adc12757ff3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -126,6 +126,7 @@ if FeatureManagement.webauthn_enabled? get '/webauthn_setup' => 'users/webauthn_setup#new', as: :webauthn_setup patch '/webauthn_setup' => 'users/webauthn_setup#confirm' + delete '/webauthn_setup' => 'users/webauthn_setup#delete' end delete '/authenticator_setup' => 'users/totp_setup#disable', as: :disable_totp diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index b6fcd732e08..2ac17d1fe70 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -82,5 +82,36 @@ patch :confirm, params: params end end + + describe 'delete' do + before do + allow(controller.current_user).to receive(:total_mfa_options_enabled).and_return(2) + end + + it 'deletes a webauthn configuration' do + cfg = create_webauthn_configuration(controller.current_user, 'key1', 'id1', 'foo1') + delete :delete, params: { id: cfg.id } + + expect(response).to redirect_to(account_url) + expect(flash.now[:success]).to eq t('notices.webauthn_deleted') + expect(WebauthnConfiguration.count).to eq(0) + end + + it 'tracks the delete' do + cfg = create_webauthn_configuration(controller.current_user, 'key1', 'id1', 'foo1') + + result = { success: true, mfa_options_enabled: 2 } + expect(@analytics).to receive(:track_event).with(Analytics::WEBAUTHN_DELETED, result) + + delete :delete, params: { id: cfg.id } + end + end + end + + def create_webauthn_configuration(user, name, id, key) + WebauthnConfiguration.create(user_id: user.id, + credential_public_key: key, + credential_id: id, + name: name) end end diff --git a/spec/features/users/webauthn_management_spec.rb b/spec/features/users/webauthn_management_spec.rb index e4d23460bc0..1b7c89156d0 100644 --- a/spec/features/users/webauthn_management_spec.rb +++ b/spec/features/users/webauthn_management_spec.rb @@ -3,16 +3,16 @@ feature 'Webauthn Management' do include WebauthnHelper - context 'with no webauthn associated yet' do - let(:user) { create(:user, :signed_up, with: { phone: '+1 202-555-1212' }) } + let(:user) { create(:user, :signed_up, with: { phone: '+1 202-555-1212' }) } + context 'with no webauthn associated yet' do it 'allows user to add a webauthn configuration' do mock_challenge sign_in_and_2fa_user(user) visit account_path expect(current_path).to eq account_path - click_link t('forms.buttons.enable'), href: webauthn_setup_url + click_link t('account.index.webauthn_add'), href: webauthn_setup_url expect(current_path).to eq webauthn_setup_path mock_press_button_on_hardware_key @@ -27,7 +27,7 @@ visit account_path expect(current_path).to eq account_path - click_link t('forms.buttons.enable'), href: webauthn_setup_url + click_link t('account.index.webauthn_add'), href: webauthn_setup_url expect(current_path).to eq webauthn_setup_path mock_press_button_on_hardware_key @@ -43,7 +43,7 @@ visit account_path expect(current_path).to eq account_path - click_link t('forms.buttons.enable'), href: webauthn_setup_url + click_link t('account.index.webauthn_add'), href: webauthn_setup_url expect(current_path).to eq webauthn_setup_path click_submit_default @@ -59,7 +59,7 @@ visit account_path expect(current_path).to eq account_path - click_link t('forms.buttons.enable'), href: webauthn_setup_url + click_link t('account.index.webauthn_add'), href: webauthn_setup_url expect(current_path).to eq webauthn_setup_path mock_press_button_on_hardware_key @@ -68,7 +68,7 @@ expect(current_path).to eq account_path expect(page).to have_content t('notices.webauthn_added') - click_link t('forms.buttons.enable'), href: webauthn_setup_url + click_link t('account.index.webauthn_add'), href: webauthn_setup_url expect(current_path).to eq webauthn_setup_path mock_press_button_on_hardware_key @@ -77,6 +77,55 @@ expect(current_path).to eq webauthn_setup_path expect(page).to have_content t('errors.webauthn_setup.unique_name') end + + it 'displays a link to add a hardware security key' do + sign_in_and_2fa_user(user) + + visit account_path + expect(page).to have_link(t('account.index.webauthn_add'), href: webauthn_setup_url) + end + end + + context 'with webauthn associations' do + it 'displays the user supplied names of the webauthn keys' do + create_webauthn_configuration(user, 'key1', '1', 'foo1') + create_webauthn_configuration(user, 'key2', '2', 'bar2') + + sign_in_and_2fa_user(user) + visit account_path + + expect(page).to have_content 'key1' + expect(page).to have_content 'key2' + end + + it 'allows the user to delete the webauthn key' do + create_webauthn_configuration(user, 'key1', '1', 'foo1') + + sign_in_and_2fa_user(user) + visit account_path + + expect(page).to have_content 'key1' + + click_button t('account.index.webauthn_delete') + + expect(page).to_not have_content 'key1' + expect(page).to have_content t('notices.webauthn_deleted') + end + + it 'prevents a user from deleting the last key' do + create_webauthn_configuration(user, 'key1', '1', 'foo1') + + sign_in_and_2fa_user(user) + PhoneConfiguration.first.update(mfa_enabled: false) + visit account_path + + expect(page).to have_content 'key1' + + click_button t('account.index.webauthn_delete') + + expect(page).to have_content 'key1' + expect(page).to have_content t('errors.webauthn_setup.delete_last') + end end def mock_challenge @@ -97,4 +146,11 @@ def mock_press_button_on_hardware_key def set_hidden_field(id, value) first("input##{id}", visible: false).set(value) end + + def create_webauthn_configuration(user, name, id, key) + WebauthnConfiguration.create(user_id: user.id, + credential_public_key: key, + credential_id: id, + name: name) + end end