diff --git a/app/controllers/users/webauthn_controller.rb b/app/controllers/users/webauthn_controller.rb index bd1a121b7c2..58d4fea1322 100644 --- a/app/controllers/users/webauthn_controller.rb +++ b/app/controllers/users/webauthn_controller.rb @@ -6,6 +6,7 @@ class WebauthnController < ApplicationController before_action :confirm_recently_authenticated_2fa before_action :set_form before_action :validate_configuration_exists + before_action :set_presenter def edit; end @@ -15,7 +16,7 @@ def update analytics.webauthn_update_name_submitted(**result.to_h) if result.success? - flash[:success] = t('two_factor_authentication.webauthn_platform.renamed') + flash[:success] = presenter.rename_success_alert_text redirect_to account_path else flash.now[:error] = result.first_error_message @@ -29,7 +30,7 @@ def destroy analytics.webauthn_delete_submitted(**result.to_h) if result.success? - flash[:success] = t('two_factor_authentication.webauthn_platform.deleted') + flash[:success] = presenter.delete_success_alert_text create_user_event(:webauthn_key_removed) revoke_remember_device(current_user) event = PushNotification::RecoveryInformationChangedEvent.new(user: current_user) @@ -49,6 +50,14 @@ def form alias_method :set_form, :form + delegate :configuration, to: :form + + def presenter + @presenter ||= TwoFactorAuthentication::WebauthnEditPresenter.new(configuration:) + end + + alias_method :set_presenter, :presenter + def form_class case action_name when 'edit', 'update' @@ -59,7 +68,7 @@ def form_class end def validate_configuration_exists - render_not_found if form.configuration.blank? + render_not_found if configuration.blank? end end end diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index a9b64774af4..298fc37f259 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -156,15 +156,19 @@ def handle_successful_delete else flash[:success] = t('notices.webauthn_deleted') end - track_delete(true) + track_delete(success: true, platform_authenticator: webauthn.platform_authenticator?) end def handle_failed_delete - track_delete(false) + track_delete(success: false, platform_authenticator: nil) end - def track_delete(success) - analytics.webauthn_delete_submitted(success:, configuration_id: delete_params[:id]) + def track_delete(success:, platform_authenticator:) + analytics.webauthn_delete_submitted( + success:, + configuration_id: delete_params[:id], + platform_authenticator:, + ) end def save_challenge_in_session diff --git a/app/forms/two_factor_authentication/webauthn_delete_form.rb b/app/forms/two_factor_authentication/webauthn_delete_form.rb index a85049ba20a..73756f39434 100644 --- a/app/forms/two_factor_authentication/webauthn_delete_form.rb +++ b/app/forms/two_factor_authentication/webauthn_delete_form.rb @@ -51,7 +51,10 @@ def validate_has_multiple_mfa end def extra_analytics_attributes - { configuration_id: } + { + configuration_id:, + platform_authenticator: configuration&.platform_authenticator?, + } end end end diff --git a/app/forms/two_factor_authentication/webauthn_update_form.rb b/app/forms/two_factor_authentication/webauthn_update_form.rb index 67e92787228..abb537eb7fd 100644 --- a/app/forms/two_factor_authentication/webauthn_update_form.rb +++ b/app/forms/two_factor_authentication/webauthn_update_form.rb @@ -62,7 +62,10 @@ def validate_unique_name end def extra_analytics_attributes - { configuration_id: } + { + configuration_id:, + platform_authenticator: configuration&.platform_authenticator?, + } end end end diff --git a/app/presenters/two_factor_authentication/webauthn_edit_presenter.rb b/app/presenters/two_factor_authentication/webauthn_edit_presenter.rb new file mode 100644 index 00000000000..f9baa4cb0c5 --- /dev/null +++ b/app/presenters/two_factor_authentication/webauthn_edit_presenter.rb @@ -0,0 +1,61 @@ +module TwoFactorAuthentication + class WebauthnEditPresenter + include ActionView::Helpers::TranslationHelper + + attr_reader :configuration + + delegate :platform_authenticator?, to: :configuration + + def initialize(configuration:) + @configuration = configuration + end + + def heading + if platform_authenticator? + t('two_factor_authentication.webauthn_platform.edit_heading') + else + t('two_factor_authentication.webauthn_roaming.edit_heading') + end + end + + def nickname_field_label + if platform_authenticator? + t('two_factor_authentication.webauthn_platform.nickname') + else + t('two_factor_authentication.webauthn_roaming.nickname') + end + end + + def rename_button_label + if platform_authenticator? + t('two_factor_authentication.webauthn_platform.change_nickname') + else + t('two_factor_authentication.webauthn_roaming.change_nickname') + end + end + + def delete_button_label + if platform_authenticator? + t('two_factor_authentication.webauthn_platform.delete') + else + t('two_factor_authentication.webauthn_roaming.delete') + end + end + + def rename_success_alert_text + if platform_authenticator? + t('two_factor_authentication.webauthn_platform.renamed') + else + t('two_factor_authentication.webauthn_roaming.renamed') + end + end + + def delete_success_alert_text + if platform_authenticator? + t('two_factor_authentication.webauthn_platform.deleted') + else + t('two_factor_authentication.webauthn_roaming.deleted') + end + end + end +end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 223af635040..e775a18ab07 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -4802,22 +4802,25 @@ def vendor_outage( ) end - # @param [Boolean] success - # @param [Hash] error_details - # @param [Integer] configuration_id + # @param [Boolean] success Whether the submission was successful + # @param [Integer] configuration_id Database ID for the configuration + # @param [Boolean] platform_authenticator Whether the configuration was a platform authenticator + # @param [Hash] error_details Details for error that occurred in unsuccessful submission # Tracks when user attempts to delete a WebAuthn configuration # @identity.idp.previous_event_name WebAuthn Deleted def webauthn_delete_submitted( success:, configuration_id:, + platform_authenticator:, error_details: nil, **extra ) track_event( :webauthn_delete_submitted, success:, - error_details:, configuration_id:, + platform_authenticator:, + error_details:, **extra, ) end @@ -4848,21 +4851,24 @@ def webauthn_setup_visit(platform_authenticator:, enabled_mfa_methods_count:, ** ) end - # @param [Boolean] success - # @param [Hash] error_details - # @param [Integer] configuration_id + # @param [Boolean] success Whether the submission was successful + # @param [Integer] configuration_id Database ID for the configuration + # @param [Boolean] platform_authenticator Whether the configuration was a platform authenticator + # @param [Hash] error_details Details for error that occurred in unsuccessful submission # Tracks when user submits a name change for a WebAuthn configuration def webauthn_update_name_submitted( success:, configuration_id:, + platform_authenticator:, error_details: nil, **extra ) track_event( :webauthn_update_name_submitted, success:, - error_details:, + platform_authenticator:, configuration_id:, + error_details:, **extra, ) end diff --git a/app/views/accounts/_webauthn_roaming.html.erb b/app/views/accounts/_webauthn_roaming.html.erb index e50316bbce8..8672240e73a 100644 --- a/app/views/accounts/_webauthn_roaming.html.erb +++ b/app/views/accounts/_webauthn_roaming.html.erb @@ -2,21 +2,20 @@ <%= t('account.index.webauthn') %> -
- <% MfaContext.new(current_user).webauthn_roaming_configurations.each do |cfg| %> -
-
- <%= cfg.name %> -
- <% if MfaPolicy.new(current_user).multiple_factors_enabled? %> -
- <%= link_to( - t('account.index.webauthn_delete'), - webauthn_setup_delete_path(id: cfg.id), - ) %> -
- <% end %> -
+
+ <% MfaContext.new(current_user).webauthn_roaming_configurations.each do |configuration| %> + <%= render ManageableAuthenticatorComponent.new( + configuration:, + user_session:, + manage_url: edit_webauthn_path(id: configuration.id), + manage_api_url: api_internal_two_factor_authentication_webauthn_path(id: configuration.id), + custom_strings: { + deleted: t('two_factor_authentication.webauthn_roaming.deleted'), + renamed: t('two_factor_authentication.webauthn_roaming.renamed'), + manage_accessible_label: t('two_factor_authentication.webauthn_roaming.manage_accessible_label'), + }, + role: 'list-item', + ) %> <% end %>
@@ -25,5 +24,6 @@ link_to(webauthn_setup_path, **tag_options, &block) end, icon: :add, - class: 'usa-button usa-button--outline margin-top-2', + outline: true, + class: 'margin-top-2', ).with_content(t('account.index.webauthn_add')) %> diff --git a/app/views/users/webauthn/edit.html.erb b/app/views/users/webauthn/edit.html.erb index c8be25675b3..5dc61948680 100644 --- a/app/views/users/webauthn/edit.html.erb +++ b/app/views/users/webauthn/edit.html.erb @@ -1,6 +1,6 @@ -<% self.title = t('two_factor_authentication.webauthn_platform.edit_heading') %> +<% self.title = @presenter.heading %> -<%= render PageHeadingComponent.new.with_content(t('two_factor_authentication.webauthn_platform.edit_heading')) %> +<%= render PageHeadingComponent.new.with_content(@presenter.heading) %> <%= simple_form_for( @form, @@ -12,20 +12,17 @@ <%= render ValidatedFieldComponent.new( form: f, name: :name, - label: t('two_factor_authentication.webauthn_platform.nickname'), + label: @presenter.nickname_field_label, ) %> - <%= f.submit( - t('two_factor_authentication.webauthn_platform.change_nickname'), - class: 'display-block margin-top-5', - ) %> + <%= f.submit(@presenter.rename_button_label, class: 'display-block margin-top-5') %> <% end %> <%= render ButtonComponent.new( action: ->(**tag_options, &block) do button_to( webauthn_path(id: @form.configuration.id), - form: { aria: { label: t('two_factor_authentication.webauthn_platform.delete') } }, + form: { aria: { label: @presenter.delete_button_label } }, **tag_options, &block ) @@ -35,6 +32,6 @@ wide: true, danger: true, class: 'display-block margin-top-2', - ).with_content(t('two_factor_authentication.webauthn_platform.delete')) %> + ).with_content(@presenter.delete_button_label) %> <%= render 'shared/cancel', link: account_path %> diff --git a/config/locales/account/en.yml b/config/locales/account/en.yml index e34c1204fbf..8c51de05e9a 100644 --- a/config/locales/account/en.yml +++ b/config/locales/account/en.yml @@ -55,7 +55,6 @@ en: webauthn: Security key webauthn_add: Add security key webauthn_confirm_delete: Yes, remove key - webauthn_delete: Remove key webauthn_platform: Face or touch unlock webauthn_platform_add: Add face or touch unlock webauthn_platform_confirm_delete: Yes, remove face or touch unlock diff --git a/config/locales/account/es.yml b/config/locales/account/es.yml index 9e72845a8cf..daf1a7c82bf 100644 --- a/config/locales/account/es.yml +++ b/config/locales/account/es.yml @@ -56,7 +56,6 @@ es: webauthn: Clave de seguridad webauthn_add: Añadir clave de seguridad webauthn_confirm_delete: Si quitar la llave - webauthn_delete: Quitar llave webauthn_platform: El desbloqueo facial o táctil webauthn_platform_add: Añadir el desbloqueo facial o táctil webauthn_platform_confirm_delete: Si, quitar el desbloqueo facial o táctil diff --git a/config/locales/account/fr.yml b/config/locales/account/fr.yml index 445776e7d5a..6fb436a6fb8 100644 --- a/config/locales/account/fr.yml +++ b/config/locales/account/fr.yml @@ -59,7 +59,6 @@ fr: webauthn: Clé de sécurité webauthn_add: Ajouter une clé de sécurité webauthn_confirm_delete: Oui, supprimer la clé - webauthn_delete: Supprimer la clé webauthn_platform: Le déverouillage facial ou déverrouillage par empreinte digitale webauthn_platform_add: Ajouter le déverouillage facial ou déverrouillage par empreinte digitale webauthn_platform_confirm_delete: Oui, supprimer le déverouillage facial ou diff --git a/config/locales/two_factor_authentication/en.yml b/config/locales/two_factor_authentication/en.yml index e55ae0021a5..f09a8862afb 100644 --- a/config/locales/two_factor_authentication/en.yml +++ b/config/locales/two_factor_authentication/en.yml @@ -208,4 +208,12 @@ en: renamed: Successfully renamed your face or touch unlock method webauthn_platform_header_text: Use face or touch unlock webauthn_platform_use_key: Use screen unlock + webauthn_roaming: + change_nickname: Change nickname + delete: Delete this device + deleted: Successfully deleted a security key method + edit_heading: Manage your security key settings + manage_accessible_label: Manage security key + nickname: Nickname + renamed: Successfully renamed your security key method webauthn_use_key: Use security key diff --git a/config/locales/two_factor_authentication/es.yml b/config/locales/two_factor_authentication/es.yml index 5f094a6efae..ac4d95f3f8f 100644 --- a/config/locales/two_factor_authentication/es.yml +++ b/config/locales/two_factor_authentication/es.yml @@ -223,4 +223,12 @@ es: facial o táctil webauthn_platform_header_text: Usar desbloqueo facial o táctil webauthn_platform_use_key: Usar el desbloqueo de pantalla + webauthn_roaming: + change_nickname: Cambiar apodo + delete: Eliminar este dispositivo + deleted: Se ha eliminado correctamente un método de clave de seguridad + edit_heading: Gestionar la configuración de su clave de seguridad + manage_accessible_label: Gestionar la clave de seguridad + nickname: Apodo + renamed: Se ha cambiado correctamente el nombre de su método de clave de seguridad webauthn_use_key: Usar llave de seguridad diff --git a/config/locales/two_factor_authentication/fr.yml b/config/locales/two_factor_authentication/fr.yml index 01465df3f19..af4a50414f1 100644 --- a/config/locales/two_factor_authentication/fr.yml +++ b/config/locales/two_factor_authentication/fr.yml @@ -235,4 +235,12 @@ fr: empreinte digitale a été renommée avec succès webauthn_platform_header_text: Utilisez le déverrouillage facial ou tactile webauthn_platform_use_key: Utiliser le déverrouillage de l’écran + webauthn_roaming: + change_nickname: Changer de pseudo + delete: Supprimer cet appareil + deleted: Suppression réussie d’une méthode de clé de sécurité + edit_heading: Gérer les paramètres de votre clé de sécurité + manage_accessible_label: Gérer la clé de sécurité + nickname: Pseudo + renamed: Votre méthode de clé de sécurité a été renommée avec succès webauthn_use_key: Utiliser la clé de sécurité diff --git a/spec/components/manageable_authenticator_component_spec.rb b/spec/components/manageable_authenticator_component_spec.rb index 20d438abf24..56eed6d93ce 100644 --- a/spec/components/manageable_authenticator_component_spec.rb +++ b/spec/components/manageable_authenticator_component_spec.rb @@ -56,11 +56,10 @@ expect(edit_element.attr('tabindex')).to be_present expect(edit_element).to have_name( - format( - '%s: %s', + [ t('components.manageable_authenticator.manage_accessible_label'), configuration.name, - ), + ].join(': '), ) end @@ -80,11 +79,10 @@ it 'renders with buttons that have accessibly distinct manage label' do expect(rendered).to have_button( - format( - '%s: %s', + [ t('components.manageable_authenticator.manage_accessible_label'), configuration.name, - ), + ].join(': '), ) end @@ -147,7 +145,7 @@ let(:custom_strings) { { manage_accessible_label: custom_manage_accessible_label } } it 'overrides button label and affected linked content' do - manage_label = format('%s: %s', custom_manage_accessible_label, configuration.name) + manage_label = [custom_manage_accessible_label, configuration.name].join(': ') expect(rendered).to have_button(manage_label) edit_element = page.find_css('.manageable-authenticator__edit').first expect(edit_element).to have_name(manage_label) diff --git a/spec/controllers/api/internal/two_factor_authentication/webauthn_controller_spec.rb b/spec/controllers/api/internal/two_factor_authentication/webauthn_controller_spec.rb index 38067823cb0..513a0e4c424 100644 --- a/spec/controllers/api/internal/two_factor_authentication/webauthn_controller_spec.rb +++ b/spec/controllers/api/internal/two_factor_authentication/webauthn_controller_spec.rb @@ -27,6 +27,7 @@ :webauthn_update_name_submitted, success: true, configuration_id: configuration.id.to_s, + platform_authenticator: false, error_details: nil, ) end @@ -60,6 +61,7 @@ :webauthn_update_name_submitted, success: false, configuration_id: configuration.id.to_s, + platform_authenticator: false, error_details: { name: { blank: true } }, ) end @@ -118,6 +120,7 @@ :webauthn_delete_submitted, success: true, configuration_id: configuration.id.to_s, + platform_authenticator: false, error_details: nil, ) end @@ -174,6 +177,7 @@ :webauthn_delete_submitted, success: false, configuration_id: configuration.id.to_s, + platform_authenticator: false, error_details: { configuration_id: { only_method: true } }, ) end diff --git a/spec/controllers/users/webauthn_controller_spec.rb b/spec/controllers/users/webauthn_controller_spec.rb index 4a9d5792ea7..836ebf525b7 100644 --- a/spec/controllers/users/webauthn_controller_spec.rb +++ b/spec/controllers/users/webauthn_controller_spec.rb @@ -13,11 +13,12 @@ let(:params) { { id: configuration.id } } let(:response) { get :edit, params: params } - it 'assigns the form instance' do + it 'assigns the form and presenter instances' do response expect(assigns(:form)).to be_kind_of(TwoFactorAuthentication::WebauthnUpdateForm) expect(assigns(:form).configuration).to eq(configuration) + expect(assigns(:presenter)).to be_kind_of(TwoFactorAuthentication::WebauthnEditPresenter) end context 'signed out' do @@ -63,7 +64,7 @@ it 'redirects to account page with success message' do expect(response).to redirect_to(account_path) - expect(flash[:success]).to eq(t('two_factor_authentication.webauthn_platform.renamed')) + expect(flash[:success]).to eq(t('two_factor_authentication.webauthn_roaming.renamed')) end it 'assigns the form instance' do @@ -80,6 +81,7 @@ :webauthn_update_name_submitted, success: true, configuration_id: configuration.id.to_s, + platform_authenticator: false, error_details: nil, ) end @@ -96,6 +98,14 @@ context 'with invalid submission' do let(:name) { '' } + it 'assigns form and presenter instances' do + response + + expect(assigns(:form)).to be_kind_of(TwoFactorAuthentication::WebauthnUpdateForm) + expect(assigns(:form).configuration).to eq(configuration) + expect(assigns(:presenter)).to be_kind_of(TwoFactorAuthentication::WebauthnEditPresenter) + end + it 'renders edit template with error' do expect(response).to render_template(:edit) expect(flash.now[:error]).to eq(t('errors.messages.blank')) @@ -108,6 +118,7 @@ :webauthn_update_name_submitted, success: false, configuration_id: configuration.id.to_s, + platform_authenticator: false, error_details: { name: { blank: true } }, ) end @@ -146,7 +157,7 @@ it 'responds with successful result' do expect(response).to redirect_to(account_path) - expect(flash[:success]).to eq(t('two_factor_authentication.webauthn_platform.deleted')) + expect(flash[:success]).to eq(t('two_factor_authentication.webauthn_roaming.deleted')) end it 'logs the submission attempt' do @@ -156,6 +167,7 @@ :webauthn_delete_submitted, success: true, configuration_id: configuration.id.to_s, + platform_authenticator: false, error_details: nil, ) end @@ -211,6 +223,7 @@ :webauthn_delete_submitted, success: false, configuration_id: configuration.id.to_s, + platform_authenticator: false, error_details: { configuration_id: { only_method: true } }, ) end diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index efa3a03fbe9..483c02b6cdc 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -156,6 +156,7 @@ success: true, error_details: nil, configuration_id: webauthn_configuration.id.to_s, + platform_authenticator: false, ) end @@ -165,6 +166,22 @@ delete :delete, params: { id: webauthn_configuration.id } end + + context 'when authenticator is the sole authentication method' do + let(:user) { create(:user) } + + it 'tracks the delete in analytics' do + delete :delete, params: { id: webauthn_configuration.id } + + expect(@analytics).to have_logged_event( + :webauthn_delete_submitted, + success: false, + error_details: nil, + configuration_id: webauthn_configuration.id.to_s, + platform_authenticator: nil, + ) + end + end end describe 'show_delete' do diff --git a/spec/features/webauthn/management_spec.rb b/spec/features/webauthn/management_spec.rb index f7ea5e8bbf0..20e91e42226 100644 --- a/spec/features/webauthn/management_spec.rb +++ b/spec/features/webauthn/management_spec.rb @@ -48,9 +48,9 @@ def expect_webauthn_platform_setup_error end context 'with webauthn roaming associations' do - it 'displays the user supplied names of the security keys' do - webauthn_config1 = create(:webauthn_configuration, user: user) - webauthn_config2 = create(:webauthn_configuration, user: user) + it 'displays the user supplied names of the platform authenticators' do + webauthn_config1 = create(:webauthn_configuration, user:) + webauthn_config2 = create(:webauthn_configuration, user:) sign_in_and_2fa_user(user) visit account_two_factor_authentication_path @@ -59,15 +59,15 @@ def expect_webauthn_platform_setup_error expect(page).to have_content webauthn_config2.name end - it 'allows the user to setup another key' do + it 'allows the user to setup another roaming authenticator' do mock_webauthn_setup_challenge - create(:webauthn_configuration, user: user) + create(:webauthn_configuration, user:) sign_in_and_2fa_user(user) visit_webauthn_setup - expect(current_path).to eq webauthn_setup_path + expect(page).to have_current_path webauthn_setup_path fill_in_nickname_and_click_continue mock_press_button_on_hardware_key_on_setup @@ -75,41 +75,147 @@ def expect_webauthn_platform_setup_error expect_webauthn_setup_success end - it 'allows user to delete security key when another 2FA option is set up' do - webauthn_config = create(:webauthn_configuration, user: user) + it 'allows user to delete a roaming authenticator when another 2FA option is set up' do + webauthn_config = create(:webauthn_configuration, user:) + name = webauthn_config.name sign_in_and_2fa_user(user) visit account_two_factor_authentication_path - expect(page).to have_content webauthn_config.name + expect(page).to have_content(name) - click_link t('account.index.webauthn_delete') + click_link( + [ + t('two_factor_authentication.webauthn_roaming.manage_accessible_label'), + name, + ].join(': '), + ) - expect(current_path).to eq webauthn_setup_delete_path + expect(current_path).to eq(edit_webauthn_path(id: webauthn_config.id)) - click_button t('account.index.webauthn_confirm_delete') + click_button t('two_factor_authentication.webauthn_roaming.delete') - expect(page).to_not have_content webauthn_config.name - expect(page).to have_content t('notices.webauthn_deleted') + expect(page).to_not have_content(name) + expect(page).to have_content(t('two_factor_authentication.webauthn_roaming.deleted')) expect(user.reload.webauthn_configurations.empty?).to eq(true) end - it 'prevents a user from deleting the last key' do - webauthn_config = create(:webauthn_configuration, user: user) + it 'allows user to rename a roaming authenticator' do + webauthn_config = create(:webauthn_configuration, user:) + name = webauthn_config.name + + sign_in_and_2fa_user(user) + visit account_two_factor_authentication_path + + expect(page).to have_content(name) + + click_link( + [ + t('two_factor_authentication.webauthn_roaming.manage_accessible_label'), + name, + ].join(': '), + ) + + expect(current_path).to eq(edit_webauthn_path(id: webauthn_config.id)) + expect(page).to have_field( + t('two_factor_authentication.webauthn_roaming.nickname'), + with: name, + ) + + fill_in t('two_factor_authentication.webauthn_roaming.nickname'), with: 'new name' + + click_button t('two_factor_authentication.webauthn_roaming.change_nickname') + + expect(page).to have_content('new name') + expect(page).to have_content(t('two_factor_authentication.webauthn_roaming.renamed')) + end + + it 'allows the user to cancel deletion of the roaming authenticator' do + webauthn_config = create(:webauthn_configuration, user:) + name = webauthn_config.name + + sign_in_and_2fa_user(user) + visit account_two_factor_authentication_path + + expect(page).to have_content(name) + + click_link( + [ + t('two_factor_authentication.webauthn_roaming.manage_accessible_label'), + name, + ].join(': '), + ) + + expect(current_path).to eq(edit_webauthn_path(id: webauthn_config.id)) + + click_link t('links.cancel') + + expect(page).to have_content(name) + end + + it 'prevents a user from deleting the last roaming authenticator' do + webauthn_config = create(:webauthn_configuration, user:) + name = webauthn_config.name sign_in_and_2fa_user(user) PhoneConfiguration.first.update(mfa_enabled: false) user.backup_code_configurations.destroy_all - visit account_two_factor_authentication_path - expect(current_path).to eq account_two_factor_authentication_path + expect(page).to have_content(name) + + click_link( + [ + t('two_factor_authentication.webauthn_roaming.manage_accessible_label'), + name, + ].join(': '), + ) - expect(page).to have_content webauthn_config.name - expect(page).to_not have_link t('account.index.webauthn_delete') + expect(current_path).to eq(edit_webauthn_path(id: webauthn_config.id)) + + click_button t('two_factor_authentication.webauthn_roaming.delete') + + expect(page).to have_current_path(edit_webauthn_path(id: webauthn_config.id)) + expect(page).to have_content(t('errors.manage_authenticator.remove_only_method_error')) + expect(user.reload.webauthn_configurations.empty?).to eq(false) + end + + it 'requires a user to use a unique name when renaming' do + webauthn_config = create(:webauthn_configuration, user:) + create(:webauthn_configuration, user:, name: 'existing') + name = webauthn_config.name + + sign_in_and_2fa_user(user) + + expect(page).to have_content(name) + + click_link( + [ + t('two_factor_authentication.webauthn_roaming.manage_accessible_label'), + name, + ].join(': '), + ) + + expect(current_path).to eq(edit_webauthn_path(id: webauthn_config.id)) + expect(page).to have_field( + t('two_factor_authentication.webauthn_roaming.nickname'), + with: name, + ) + + fill_in t('two_factor_authentication.webauthn_roaming.nickname'), with: 'existing' + + click_button t('two_factor_authentication.webauthn_roaming.change_nickname') + + expect(current_path).to eq(edit_webauthn_path(id: webauthn_config.id)) + expect(page).to have_field( + t('two_factor_authentication.webauthn_roaming.nickname'), + with: 'existing', + ) + expect(page).to have_content(t('errors.manage_authenticator.unique_name_error')) + expect(page).to have_css('.usa-input--error') end it 'gives an error if name is taken and stays on the configuration screen' do - webauthn_config = create(:webauthn_configuration, user: user) + webauthn_config = create(:webauthn_configuration, user:) mock_webauthn_setup_challenge sign_in_and_2fa_user(user) @@ -126,6 +232,120 @@ def expect_webauthn_platform_setup_error expect(current_path).to eq webauthn_setup_path expect(page).to have_content t('errors.webauthn_setup.unique_name') end + + context 'with javascript enabled', :js do + it 'allows user to delete a roaming authenticator when another 2FA option is set up' do + webauthn_config = create(:webauthn_configuration, user:) + name = webauthn_config.name + + sign_in_and_2fa_user(user) + visit account_two_factor_authentication_path + + expect(page).to have_content(name) + + click_button( + [ + t('two_factor_authentication.webauthn_roaming.manage_accessible_label'), + name, + ].join(': '), + ) + + # Verify user can cancel deletion. There's an implied assertion here that the button becomes + # clickable again, since the following confirmation occurs upon successive button click. + dismiss_confirm(wait: 5) { click_button t('components.manageable_authenticator.delete') } + + # Verify user confirms deletion + accept_confirm(wait: 5) { click_button t('components.manageable_authenticator.delete') } + + expect(page).to have_content( + t('two_factor_authentication.webauthn_roaming.deleted'), + wait: 5, + ) + expect(page).to_not have_content(name) + expect(user.reload.webauthn_configurations.empty?).to eq(true) + end + + it 'allows user to rename a roaming authenticator' do + webauthn_config = create(:webauthn_configuration, user:) + name = webauthn_config.name + + sign_in_and_2fa_user(user) + visit account_two_factor_authentication_path + + expect(page).to have_content(name) + + click_button( + [ + t('two_factor_authentication.webauthn_roaming.manage_accessible_label'), + name, + ].join(': '), + ) + click_button t('components.manageable_authenticator.rename') + + expect(page).to have_field(t('components.manageable_authenticator.nickname'), with: name) + + fill_in t('components.manageable_authenticator.nickname'), with: 'new name' + + click_button t('components.manageable_authenticator.save') + + expect(page).to have_content( + t('two_factor_authentication.webauthn_roaming.renamed'), + wait: 5, + ) + expect(page).to have_content('new name') + end + + it 'prevents a user from deleting the last roaming authenticator', allow_browser_log: true do + webauthn_config = create(:webauthn_configuration, user:) + name = webauthn_config.name + + sign_in_and_2fa_user(user) + PhoneConfiguration.first.update(mfa_enabled: false) + user.backup_code_configurations.destroy_all + + expect(page).to have_content(name) + + click_button( + [ + t('two_factor_authentication.webauthn_roaming.manage_accessible_label'), + name, + ].join(': '), + ) + accept_confirm(wait: 5) { click_button t('components.manageable_authenticator.delete') } + + expect(page).to have_content( + t('errors.manage_authenticator.remove_only_method_error'), + wait: 5, + ) + expect(user.reload.webauthn_configurations.empty?).to eq(false) + end + + it 'requires a user to use a unique name when renaming', allow_browser_log: true do + webauthn_config = create(:webauthn_configuration, user:) + create(:webauthn_configuration, user:, name: 'existing') + name = webauthn_config.name + + sign_in_and_2fa_user(user) + + expect(page).to have_content(name) + + click_button( + [ + t('two_factor_authentication.webauthn_roaming.manage_accessible_label'), + name, + ].join(': '), + ) + click_button t('components.manageable_authenticator.rename') + + expect(page).to have_field(t('components.manageable_authenticator.nickname'), with: name) + + fill_in t('components.manageable_authenticator.nickname'), with: 'existing' + + click_button t('components.manageable_authenticator.save') + + expect(page).to have_content(t('errors.manage_authenticator.unique_name_error'), wait: 5) + end + end end context 'with webauthn platform associations' do @@ -140,7 +360,7 @@ def expect_webauthn_platform_setup_error expect(page).to have_content webauthn_config2.name end - it 'allows the user to setup another key' do + it 'allows the user to setup another platform authenticator' do mock_webauthn_setup_challenge create(:webauthn_configuration, :platform_authenticator, user:) @@ -175,11 +395,10 @@ def expect_webauthn_platform_setup_error expect(page).to have_content(name) click_link( - format( - '%s: %s', + [ t('two_factor_authentication.webauthn_platform.manage_accessible_label'), name, - ), + ].join(': '), ) expect(current_path).to eq(edit_webauthn_path(id: webauthn_config.id)) @@ -201,11 +420,10 @@ def expect_webauthn_platform_setup_error expect(page).to have_content(name) click_link( - format( - '%s: %s', + [ t('two_factor_authentication.webauthn_platform.manage_accessible_label'), name, - ), + ].join(': '), ) expect(current_path).to eq(edit_webauthn_path(id: webauthn_config.id)) @@ -232,11 +450,10 @@ def expect_webauthn_platform_setup_error expect(page).to have_content(name) click_link( - format( - '%s: %s', + [ t('two_factor_authentication.webauthn_platform.manage_accessible_label'), name, - ), + ].join(': '), ) expect(current_path).to eq(edit_webauthn_path(id: webauthn_config.id)) @@ -246,7 +463,7 @@ def expect_webauthn_platform_setup_error expect(page).to have_content(name) end - it 'prevents a user from deleting the last key' do + it 'prevents a user from deleting the last platform authenticator' do webauthn_config = create(:webauthn_configuration, :platform_authenticator, user:) name = webauthn_config.name @@ -257,11 +474,10 @@ def expect_webauthn_platform_setup_error expect(page).to have_content(name) click_link( - format( - '%s: %s', + [ t('two_factor_authentication.webauthn_platform.manage_accessible_label'), name, - ), + ].join(': '), ) expect(current_path).to eq(edit_webauthn_path(id: webauthn_config.id)) @@ -283,11 +499,10 @@ def expect_webauthn_platform_setup_error expect(page).to have_content(name) click_link( - format( - '%s: %s', + [ t('two_factor_authentication.webauthn_platform.manage_accessible_label'), name, - ), + ].join(': '), ) expect(current_path).to eq(edit_webauthn_path(id: webauthn_config.id)) @@ -306,6 +521,7 @@ def expect_webauthn_platform_setup_error with: 'existing', ) expect(page).to have_content(t('errors.manage_authenticator.unique_name_error')) + expect(page).to have_css('.usa-input--error') end it 'gives an error if name is taken and stays on the configuration screen' do @@ -338,11 +554,10 @@ def expect_webauthn_platform_setup_error expect(page).to have_content(name) click_button( - format( - '%s: %s', + [ t('two_factor_authentication.webauthn_platform.manage_accessible_label'), name, - ), + ].join(': '), ) # Verify user can cancel deletion. There's an implied assertion here that the button becomes @@ -370,11 +585,10 @@ def expect_webauthn_platform_setup_error expect(page).to have_content(name) click_button( - format( - '%s: %s', + [ t('two_factor_authentication.webauthn_platform.manage_accessible_label'), name, - ), + ].join(': '), ) click_button t('components.manageable_authenticator.rename') @@ -391,7 +605,7 @@ def expect_webauthn_platform_setup_error expect(page).to have_content('new name') end - it 'prevents a user from deleting the last key', allow_browser_log: true do + it 'prevents a user from deleting the last platform authenticator', allow_browser_log: true do webauthn_config = create(:webauthn_configuration, :platform_authenticator, user:) name = webauthn_config.name @@ -402,11 +616,10 @@ def expect_webauthn_platform_setup_error expect(page).to have_content(name) click_button( - format( - '%s: %s', + [ t('two_factor_authentication.webauthn_platform.manage_accessible_label'), name, - ), + ].join(': '), ) accept_confirm(wait: 5) { click_button t('components.manageable_authenticator.delete') } @@ -427,11 +640,10 @@ def expect_webauthn_platform_setup_error expect(page).to have_content(name) click_button( - format( - '%s: %s', + [ t('two_factor_authentication.webauthn_platform.manage_accessible_label'), name, - ), + ].join(': '), ) click_button t('components.manageable_authenticator.rename') diff --git a/spec/forms/two_factor_authentication/webauthn_delete_form_spec.rb b/spec/forms/two_factor_authentication/webauthn_delete_form_spec.rb index eb28c95004a..f0c210aceb9 100644 --- a/spec/forms/two_factor_authentication/webauthn_delete_form_spec.rb +++ b/spec/forms/two_factor_authentication/webauthn_delete_form_spec.rb @@ -14,7 +14,21 @@ it 'returns a successful result' do expect(result.success?).to eq(true) - expect(result.to_h).to eq(success: true, configuration_id:) + expect(result.to_h).to eq( + success: true, + configuration_id:, + platform_authenticator: false, + ) + end + + context 'with platform authenticator' do + let(:configuration) do + create(:webauthn_configuration, :platform_authenticator, user:) + + it 'includes platform authenticator detail in result' do + expect(result.to_h[:platform_authenticator]).to eq(true) + end + end end context 'with blank configuration' do @@ -28,6 +42,7 @@ configuration_id: { configuration_not_found: true }, }, configuration_id:, + platform_authenticator: nil, ) end end @@ -43,6 +58,7 @@ configuration_id: { configuration_not_found: true }, }, configuration_id:, + platform_authenticator: nil, ) end end @@ -58,6 +74,7 @@ configuration_id: { configuration_not_found: true }, }, configuration_id:, + platform_authenticator: nil, ) end end @@ -74,8 +91,19 @@ configuration_id: { only_method: true }, }, configuration_id:, + platform_authenticator: false, ) end + + context 'with platform authenticator' do + let(:configuration) do + create(:webauthn_configuration, :platform_authenticator, user:) + + it 'includes platform authenticator detail in result' do + expect(result.to_h[:platform_authenticator]).to eq(true) + end + end + end end end diff --git a/spec/forms/two_factor_authentication/webauthn_update_form_spec.rb b/spec/forms/two_factor_authentication/webauthn_update_form_spec.rb index 8f85687ecbd..4d371bd7822 100644 --- a/spec/forms/two_factor_authentication/webauthn_update_form_spec.rb +++ b/spec/forms/two_factor_authentication/webauthn_update_form_spec.rb @@ -13,7 +13,21 @@ it 'returns a successful result' do expect(result.success?).to eq(true) - expect(result.to_h).to eq(success: true, configuration_id:) + expect(result.to_h).to eq( + success: true, + configuration_id:, + platform_authenticator: false, + ) + end + + context 'with platform authenticator' do + let(:configuration) do + create(:webauthn_configuration, :platform_authenticator, user:, name: original_name) + + it 'includes platform authenticator detail in result' do + expect(result.to_h[:platform_authenticator]).to eq(true) + end + end end it 'saves the new name' do @@ -33,6 +47,7 @@ configuration_id: { configuration_not_found: true }, }, configuration_id:, + platform_authenticator: nil, ) end end @@ -48,6 +63,7 @@ configuration_id: { configuration_not_found: true }, }, configuration_id:, + platform_authenticator: nil, ) end end @@ -63,6 +79,7 @@ configuration_id: { configuration_not_found: true }, }, configuration_id:, + platform_authenticator: nil, ) end @@ -86,6 +103,7 @@ name: { blank: true }, }, configuration_id:, + platform_authenticator: false, ) end @@ -96,6 +114,16 @@ expect(configuration.reload.name).to eq(original_name) end + + context 'with platform authenticator' do + let(:configuration) do + create(:webauthn_configuration, :platform_authenticator, user:, name: original_name) + + it 'includes platform authenticator detail in result' do + expect(result.to_h[:platform_authenticator]).to eq(true) + end + end + end end context 'with duplicate name' do @@ -111,6 +139,7 @@ name: { duplicate: true }, }, configuration_id:, + platform_authenticator: false, ) end @@ -121,6 +150,16 @@ expect(configuration.reload.name).to eq(original_name) end + + context 'with platform authenticator' do + let(:configuration) do + create(:webauthn_configuration, :platform_authenticator, user:, name: original_name) + + it 'includes platform authenticator detail in result' do + expect(result.to_h[:platform_authenticator]).to eq(true) + end + end + end end end diff --git a/spec/presenters/two_factor_authentication/webauthn_edit_presenter_spec.rb b/spec/presenters/two_factor_authentication/webauthn_edit_presenter_spec.rb new file mode 100644 index 00000000000..c52957eac15 --- /dev/null +++ b/spec/presenters/two_factor_authentication/webauthn_edit_presenter_spec.rb @@ -0,0 +1,103 @@ +require 'rails_helper' + +RSpec.describe TwoFactorAuthentication::WebauthnEditPresenter do + let(:configuration) { build(:webauthn_configuration) } + + subject(:presenter) { described_class.new(configuration:) } + + describe '#heading' do + subject(:heading) { presenter.heading } + + context 'with roaming authenticator' do + let(:configuration) { build(:webauthn_configuration) } + + it { expect(heading).to eq(t('two_factor_authentication.webauthn_roaming.edit_heading')) } + end + + context 'with platform authenticator' do + let(:configuration) { build(:webauthn_configuration, :platform_authenticator) } + + it { expect(heading).to eq(t('two_factor_authentication.webauthn_platform.edit_heading')) } + end + end + + describe '#nickname_field_label' do + subject(:heading) { presenter.nickname_field_label } + + context 'with roaming authenticator' do + let(:configuration) { build(:webauthn_configuration) } + + it { expect(heading).to eq(t('two_factor_authentication.webauthn_roaming.nickname')) } + end + + context 'with platform authenticator' do + let(:configuration) { build(:webauthn_configuration, :platform_authenticator) } + + it { expect(heading).to eq(t('two_factor_authentication.webauthn_platform.nickname')) } + end + end + + describe '#rename_button_label' do + subject(:heading) { presenter.rename_button_label } + + context 'with roaming authenticator' do + let(:configuration) { build(:webauthn_configuration) } + + it { expect(heading).to eq(t('two_factor_authentication.webauthn_roaming.change_nickname')) } + end + + context 'with platform authenticator' do + let(:configuration) { build(:webauthn_configuration, :platform_authenticator) } + + it { expect(heading).to eq(t('two_factor_authentication.webauthn_platform.change_nickname')) } + end + end + + describe '#delete_button_label' do + subject(:heading) { presenter.delete_button_label } + + context 'with roaming authenticator' do + let(:configuration) { build(:webauthn_configuration) } + + it { expect(heading).to eq(t('two_factor_authentication.webauthn_roaming.delete')) } + end + + context 'with platform authenticator' do + let(:configuration) { build(:webauthn_configuration, :platform_authenticator) } + + it { expect(heading).to eq(t('two_factor_authentication.webauthn_platform.delete')) } + end + end + + describe '#rename_success_alert_text' do + subject(:heading) { presenter.rename_success_alert_text } + + context 'with roaming authenticator' do + let(:configuration) { build(:webauthn_configuration) } + + it { expect(heading).to eq(t('two_factor_authentication.webauthn_roaming.renamed')) } + end + + context 'with platform authenticator' do + let(:configuration) { build(:webauthn_configuration, :platform_authenticator) } + + it { expect(heading).to eq(t('two_factor_authentication.webauthn_platform.renamed')) } + end + end + + describe '#delete_success_alert_text' do + subject(:heading) { presenter.delete_success_alert_text } + + context 'with roaming authenticator' do + let(:configuration) { build(:webauthn_configuration) } + + it { expect(heading).to eq(t('two_factor_authentication.webauthn_roaming.deleted')) } + end + + context 'with platform authenticator' do + let(:configuration) { build(:webauthn_configuration, :platform_authenticator) } + + it { expect(heading).to eq(t('two_factor_authentication.webauthn_platform.deleted')) } + end + end +end diff --git a/spec/views/accounts/_webauthn_roaming.html.erb_spec.rb b/spec/views/accounts/_webauthn_roaming.html.erb_spec.rb new file mode 100644 index 00000000000..27f4375a3db --- /dev/null +++ b/spec/views/accounts/_webauthn_roaming.html.erb_spec.rb @@ -0,0 +1,22 @@ +require 'rails_helper' + +RSpec.describe 'accounts/_webauthn_roaming.html.erb' do + let(:user) do + create( + :user, + webauthn_configurations: create_list(:webauthn_configuration, 2), + ) + end + let(:user_session) { { auth_events: [] } } + + subject(:rendered) { render partial: 'accounts/webauthn_roaming' } + + before do + allow(view).to receive(:current_user).and_return(user) + allow(view).to receive(:user_session).and_return(user_session) + end + + it 'renders a list of roaming authenticators' do + expect(rendered).to have_selector('[role="list"] [role="list-item"]', count: 2) + end +end diff --git a/spec/views/users/webauthn/edit.html.erb_spec.rb b/spec/views/users/webauthn/edit.html.erb_spec.rb index d5bbc9fb16a..30785d7e71a 100644 --- a/spec/views/users/webauthn/edit.html.erb_spec.rb +++ b/spec/views/users/webauthn/edit.html.erb_spec.rb @@ -6,6 +6,7 @@ let(:nickname) { 'Example' } let(:configuration) { create(:webauthn_configuration, :platform_authenticator, name: nickname) } let(:user) { create(:user, webauthn_configurations: [configuration]) } + let(:presenter) { TwoFactorAuthentication::WebauthnEditPresenter.new(configuration:) } let(:form) do TwoFactorAuthentication::WebauthnUpdateForm.new( user:, @@ -17,6 +18,7 @@ before do @form = form + @presenter = presenter end it 'renders form to update configuration' do