From 90f6ced7de5312b408bcf5d98a91277ea5118225 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Mon, 11 Dec 2023 16:17:51 -0500 Subject: [PATCH 1/3] LG-11454 (1): Add controllers for WebAuthn management changelog: Upcoming Features, Face or Touch Unlock, Add new renaming interface for face or touch unlock --- .../webauthn_controller.rb | 52 ++++++ .../reauthentication_required_concern.rb | 6 +- app/controllers/users/webauthn_controller.rb | 61 +++++++ .../users/webauthn_setup_controller.rb | 7 +- .../webauthn_delete_form.rb | 57 +++++++ .../webauthn_update_form.rb | 68 ++++++++ app/services/analytics_events.rb | 39 ++++- app/views/users/webauthn/edit.html.erb | 40 +++++ config/locales/errors/en.yml | 4 + config/locales/errors/es.yml | 5 + config/locales/errors/fr.yml | 5 + .../locales/two_factor_authentication/en.yml | 7 + .../locales/two_factor_authentication/es.yml | 8 + .../locales/two_factor_authentication/fr.yml | 10 ++ config/routes.rb | 11 ++ .../webauthn_controller_spec.rb | 149 +++++++++++++++++ .../reauthentication_required_concern_spec.rb | 59 +++++-- .../users/webauthn_controller_spec.rb | 158 ++++++++++++++++++ .../users/webauthn_setup_controller_spec.rb | 14 +- .../webauthn_delete_form_spec.rb | 89 ++++++++++ .../webauthn_update_form_spec.rb | 142 ++++++++++++++++ .../users/webauthn/edit.html.erb_spec.rb | 42 +++++ 22 files changed, 1002 insertions(+), 31 deletions(-) create mode 100644 app/controllers/api/internal/two_factor_authentication/webauthn_controller.rb create mode 100644 app/controllers/users/webauthn_controller.rb create mode 100644 app/forms/two_factor_authentication/webauthn_delete_form.rb create mode 100644 app/forms/two_factor_authentication/webauthn_update_form.rb create mode 100644 app/views/users/webauthn/edit.html.erb create mode 100644 spec/controllers/api/internal/two_factor_authentication/webauthn_controller_spec.rb create mode 100644 spec/controllers/users/webauthn_controller_spec.rb create mode 100644 spec/forms/two_factor_authentication/webauthn_delete_form_spec.rb create mode 100644 spec/forms/two_factor_authentication/webauthn_update_form_spec.rb create mode 100644 spec/views/users/webauthn/edit.html.erb_spec.rb diff --git a/app/controllers/api/internal/two_factor_authentication/webauthn_controller.rb b/app/controllers/api/internal/two_factor_authentication/webauthn_controller.rb new file mode 100644 index 00000000000..4b594213b1e --- /dev/null +++ b/app/controllers/api/internal/two_factor_authentication/webauthn_controller.rb @@ -0,0 +1,52 @@ +module Api + module Internal + module TwoFactorAuthentication + class WebauthnController < ApplicationController + include CsrfTokenConcern + include ReauthenticationRequiredConcern + + before_action :render_unauthorized, unless: :recently_authenticated_2fa? + + after_action :add_csrf_token_header_to_response + + respond_to :json + + def update + result = ::TwoFactorAuthentication::WebauthnUpdateForm.new( + user: current_user, + configuration_id: params[:id], + ).submit(name: params[:name]) + + analytics.webauthn_update_name_submitted(**result.to_h) + + if result.success? + render json: { success: true } + else + render json: { success: false, error: result.first_error_message }, status: :bad_request + end + end + + def destroy + result = ::TwoFactorAuthentication::WebauthnDeleteForm.new( + user: current_user, + configuration_id: params[:id], + ).submit + + analytics.webauthn_delete_submitted(**result.to_h) + + if result.success? + render json: { success: true } + else + render json: { success: false, error: result.first_error_message }, status: :bad_request + end + end + + private + + def render_unauthorized + render json: { error: 'Unauthorized' }, status: :unauthorized + end + end + end + end +end diff --git a/app/controllers/concerns/reauthentication_required_concern.rb b/app/controllers/concerns/reauthentication_required_concern.rb index bea31e129d0..b236ab41815 100644 --- a/app/controllers/concerns/reauthentication_required_concern.rb +++ b/app/controllers/concerns/reauthentication_required_concern.rb @@ -3,7 +3,7 @@ module ReauthenticationRequiredConcern include TwoFactorAuthenticatableMethods def confirm_recently_authenticated_2fa - return if !user_fully_authenticated? || auth_methods_session.recently_authenticated_2fa? + return if !user_fully_authenticated? || recently_authenticated_2fa? analytics.user_2fa_reauthentication_required( auth_method: auth_methods_session.last_auth_event&.[](:auth_method), @@ -13,6 +13,10 @@ def confirm_recently_authenticated_2fa prompt_for_second_factor end + def recently_authenticated_2fa? + user_fully_authenticated? && auth_methods_session.recently_authenticated_2fa? + end + private def prompt_for_second_factor diff --git a/app/controllers/users/webauthn_controller.rb b/app/controllers/users/webauthn_controller.rb new file mode 100644 index 00000000000..0fb29194875 --- /dev/null +++ b/app/controllers/users/webauthn_controller.rb @@ -0,0 +1,61 @@ +module Users + class WebauthnController < ApplicationController + include ReauthenticationRequiredConcern + + before_action :confirm_two_factor_authenticated + before_action :confirm_recently_authenticated_2fa + before_action :set_form + before_action :validate_configuration_exists + + def edit; end + + def update + result = form.submit(name: params.dig(:form, :name)) + + analytics.webauthn_update_name_submitted(**result.to_h) + + if result.success? + flash[:success] = t('two_factor_authentication.webauthn_platform.renamed') + redirect_to account_path + else + flash.now[:error] = result.first_error_message + render :edit + end + end + + def destroy + result = form.submit + + analytics.webauthn_delete_submitted(**result.to_h) + + if result.success? + flash[:success] = t('two_factor_authentication.webauthn_platform.deleted') + redirect_to account_path + else + flash[:error] = result.first_error_message + redirect_to edit_webauthn_path(id: params[:id]) + end + end + + private + + def form + @form ||= form_class.new(user: current_user, configuration_id: params[:id]) + end + + alias_method :set_form, :form + + def form_class + case action_name + when 'edit', 'update' + TwoFactorAuthentication::WebauthnUpdateForm + when 'destroy' + TwoFactorAuthentication::WebauthnDeleteForm + end + end + + def validate_configuration_exists + render_not_found if form.configuration.blank? + end + end +end diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index d21ea2839ef..359360c724c 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -159,12 +159,7 @@ def handle_failed_delete end def track_delete(success) - counts_hash = MfaContext.new(current_user.reload).enabled_two_factor_configuration_counts_hash - analytics.webauthn_deleted( - success: success, - mfa_method_counts: counts_hash, - pii_like_keypaths: [[:mfa_method_counts, :phone]], - ) + analytics.webauthn_delete_submitted(success:, configuration_id: delete_params[:id]) 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 new file mode 100644 index 00000000000..a85049ba20a --- /dev/null +++ b/app/forms/two_factor_authentication/webauthn_delete_form.rb @@ -0,0 +1,57 @@ +module TwoFactorAuthentication + class WebauthnDeleteForm + include ActiveModel::Model + include ActionView::Helpers::TranslationHelper + + attr_reader :user, :configuration_id + + validate :validate_configuration_exists + validate :validate_has_multiple_mfa + + def initialize(user:, configuration_id:) + @user = user + @configuration_id = configuration_id + end + + def submit + success = valid? + + configuration.destroy if success + + FormResponse.new( + success:, + errors:, + extra: extra_analytics_attributes, + serialize_error_details_only: true, + ) + end + + def configuration + @configuration ||= user.webauthn_configurations.find_by(id: configuration_id) + end + + private + + def validate_configuration_exists + return if configuration.present? + errors.add( + :configuration_id, + :configuration_not_found, + message: t('errors.manage_authenticator.internal_error'), + ) + end + + def validate_has_multiple_mfa + return if !configuration || MfaPolicy.new(user).multiple_factors_enabled? + errors.add( + :configuration_id, + :only_method, + message: t('errors.manage_authenticator.remove_only_method_error'), + ) + end + + def extra_analytics_attributes + { configuration_id: } + 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 new file mode 100644 index 00000000000..67e92787228 --- /dev/null +++ b/app/forms/two_factor_authentication/webauthn_update_form.rb @@ -0,0 +1,68 @@ +module TwoFactorAuthentication + class WebauthnUpdateForm + include ActiveModel::Model + include ActionView::Helpers::TranslationHelper + + attr_reader :user, :configuration_id + + validate :validate_configuration_exists + validate :validate_unique_name + + def initialize(user:, configuration_id:) + @user = user + @configuration_id = configuration_id + end + + def submit(name:) + @name = name + + success = valid? + if valid? + configuration.name = name + success = configuration.valid? + errors.merge!(configuration.errors) + configuration.save if success + end + + FormResponse.new( + success:, + errors:, + extra: extra_analytics_attributes, + serialize_error_details_only: true, + ) + end + + def name + return @name if defined?(@name) + @name = configuration&.name + end + + def configuration + @configuration ||= user.webauthn_configurations.find_by(id: configuration_id) + end + + private + + def validate_configuration_exists + return if configuration.present? + errors.add( + :configuration_id, + :configuration_not_found, + message: t('errors.manage_authenticator.internal_error'), + ) + end + + def validate_unique_name + return unless user.webauthn_configurations.where.not(id: configuration_id).find_by(name:) + errors.add( + :name, + :duplicate, + message: t('errors.manage_authenticator.unique_name_error'), + ) + end + + def extra_analytics_attributes + { configuration_id: } + end + end +end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 8a57c7a2762..ee8038e0976 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -4718,14 +4718,20 @@ def vendor_outage( end # @param [Boolean] success - # @param [Integer] mfa_method_counts - # Tracks when WebAuthn is deleted - def webauthn_deleted(success:, mfa_method_counts:, pii_like_keypaths:, **extra) + # @param [Hash] error_details + # @param [Integer] configuration_id + # Tracks when user attempts to delete a WebAuthn configuration + def webauthn_delete_submitted( + success:, + configuration_id:, + error_details: nil, + **extra + ) track_event( - 'WebAuthn Deleted', - success: success, - mfa_method_counts: mfa_method_counts, - pii_like_keypaths: pii_like_keypaths, + :webauthn_delete_submitted, + success:, + error_details:, + configuration_id:, **extra, ) end @@ -4755,4 +4761,23 @@ def webauthn_setup_visit(platform_authenticator:, enabled_mfa_methods_count:, ** **extra, ) end + + # @param [Boolean] success + # @param [Hash] error_details + # @param [Integer] configuration_id + # Tracks when user submits a name change for a WebAuthn configuration + def webauthn_update_name_submitted( + success:, + configuration_id:, + error_details: nil, + **extra + ) + track_event( + :webauthn_update_name_submitted, + success:, + error_details:, + configuration_id:, + **extra, + ) + end end diff --git a/app/views/users/webauthn/edit.html.erb b/app/views/users/webauthn/edit.html.erb new file mode 100644 index 00000000000..c8be25675b3 --- /dev/null +++ b/app/views/users/webauthn/edit.html.erb @@ -0,0 +1,40 @@ +<% self.title = t('two_factor_authentication.webauthn_platform.edit_heading') %> + +<%= render PageHeadingComponent.new.with_content(t('two_factor_authentication.webauthn_platform.edit_heading')) %> + +<%= simple_form_for( + @form, + as: :form, + method: :put, + html: { autocomplete: 'off' }, + url: webauthn_path(id: @form.configuration.id), + ) do |f| %> + <%= render ValidatedFieldComponent.new( + form: f, + name: :name, + label: t('two_factor_authentication.webauthn_platform.nickname'), + ) %> + + <%= f.submit( + t('two_factor_authentication.webauthn_platform.change_nickname'), + 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') } }, + **tag_options, + &block + ) + end, + method: :delete, + big: true, + wide: true, + danger: true, + class: 'display-block margin-top-2', + ).with_content(t('two_factor_authentication.webauthn_platform.delete')) %> + +<%= render 'shared/cancel', link: account_path %> diff --git a/config/locales/errors/en.yml b/config/locales/errors/en.yml index 9a8f7fe4107..677a871510f 100644 --- a/config/locales/errors/en.yml +++ b/config/locales/errors/en.yml @@ -41,6 +41,10 @@ en: Try again in %{timeout}. general: Oops, something went wrong. Please try again. invalid_totp: Invalid code. Please try again. + manage_authenticator: + internal_error: There was an internal error processing your request. Please try again. + remove_only_method_error: You cannot remove your only authentication method. + unique_name_error: Name already in use. Please use a different name. max_password_attempts_reached: You’ve entered too many incorrect passwords. You can reset your password using the “Forgot your password?” link. messages: diff --git a/config/locales/errors/es.yml b/config/locales/errors/es.yml index 28936e735dd..de0b30cc8f4 100644 --- a/config/locales/errors/es.yml +++ b/config/locales/errors/es.yml @@ -44,6 +44,11 @@ es: veces. Inténtelo de nuevo en %{timeout}. general: '¡Oops! Algo salió mal. Inténtelo de nuevo.' invalid_totp: El código es inválido. Vuelva a intentarlo. + manage_authenticator: + internal_error: Se produjo un error interno al procesar tu solicitud. Por favor, + inténtalo de nuevo. + remove_only_method_error: No puede eliminar su único método de autenticación. + unique_name_error: Nombre ya en uso. Utilice un nombre diferente. max_password_attempts_reached: Ha ingresado demasiadas contraseñas incorrectas. Puede restablecer su contraseña usando el enlace “¿Olvidó su contraseña?”. messages: diff --git a/config/locales/errors/fr.yml b/config/locales/errors/fr.yml index bce5862a336..e7274032aaa 100644 --- a/config/locales/errors/fr.yml +++ b/config/locales/errors/fr.yml @@ -48,6 +48,11 @@ fr: reprises. Réessayez dans %{timeout}. general: Oups, une erreur s’est produite. Veuillez essayer de nouveau. invalid_totp: Code non valide. Veuillez essayer de nouveau. + manage_authenticator: + internal_error: Une erreur interne s’est produite lors du traitement de votre + demande. Veuillez réessayer. + remove_only_method_error: Vous ne pouvez pas supprimer votre seule méthode d’authentification. + unique_name_error: Ce nom est déjà utilisé. Veuillez utiliser un nom différent. max_password_attempts_reached: Vous avez inscrit des mots de passe incorrects un trop grand nombre de fois. Vous pouvez réinitialiser votre mot de passe en utilisant le lien « Vous avez oublié votre mot de passe? ». diff --git a/config/locales/two_factor_authentication/en.yml b/config/locales/two_factor_authentication/en.yml index d7cafce7f82..6b370a122b6 100644 --- a/config/locales/two_factor_authentication/en.yml +++ b/config/locales/two_factor_authentication/en.yml @@ -190,6 +190,13 @@ en: try_again: Face or touch unlock was unsuccessful. Please try again or %{link}. use_a_different_method: Use a different authentication method webauthn_header_text: Connect your security key + webauthn_platform: + change_nickname: Change nickname + delete: Delete this device + deleted: Successfully deleted a face or touch unlock method + edit_heading: Manage your face or touch unlock settings + nickname: Nickname + renamed: Successfully renamed your face or touch unlock method webauthn_platform_header_text: Use face or touch unlock webauthn_platform_use_key: Use face or touch unlock 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 6180bd85584..a89e9df1675 100644 --- a/config/locales/two_factor_authentication/es.yml +++ b/config/locales/two_factor_authentication/es.yml @@ -203,6 +203,14 @@ es: nuevo o %{link}. use_a_different_method: Utilice otro método de autenticación webauthn_header_text: Conecte su llave de seguridad + webauthn_platform: + change_nickname: Cambiar apodo + delete: Eliminar este dispositivo + deleted: Se ha eliminado correctamente un método de desbloqueo facial o táctil + edit_heading: Gestione su configuración de desbloqueo facial o táctil + nickname: Apodo + renamed: Se ha cambiado correctamente el nombre de su método de desbloqueo + facial o táctil webauthn_platform_header_text: Usar desbloqueo facial o táctil webauthn_platform_use_key: Usar desbloqueo facial o táctil 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 d229f590c02..095770387ed 100644 --- a/config/locales/two_factor_authentication/fr.yml +++ b/config/locales/two_factor_authentication/fr.yml @@ -213,6 +213,16 @@ fr: réessayer ou %{link}. use_a_different_method: Utilisez un autre moyen d’authentification webauthn_header_text: Connectez votre clé de sécurité + webauthn_platform: + change_nickname: Changer de pseudo + delete: Supprimer cet appareil + deleted: Suppression réussie d’une méthode de déverrouillage par reconnaissance + faciale ou par empreinte digitale + edit_heading: Gérez vos paramètres de déverrouillage par reconnaissance faciale + ou par empreinte digitale + nickname: Pseudo + renamed: Votre méthode de déverrouillage par reconnaissance faciale ou par + empreinte digitale a été renommée avec succès webauthn_platform_header_text: Utilisez le déverrouillage facial ou tactile webauthn_platform_use_key: Utilisez le déverrouillage facial ou tactile webauthn_use_key: Utiliser la clé de sécurité diff --git a/config/routes.rb b/config/routes.rb index 1a66d28b1be..25023207720 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -19,6 +19,10 @@ namespace :internal do get '/sessions' => 'sessions#show' put '/sessions' => 'sessions#update' + + namespace :two_factor_authentication do + resource :webauthn, path: '/webauthn/:id', controller: :webauthn, only: [:update, :destroy] + end end end @@ -218,6 +222,8 @@ get '/webauthn_setup' => 'users/webauthn_setup#new', as: :webauthn_setup patch '/webauthn_setup' => 'users/webauthn_setup#confirm' + + # Deprecated routes: Remove once LG-11454 is fully deployed to production. delete '/webauthn_setup' => 'users/webauthn_setup#delete' get '/webauthn_setup_delete' => 'users/webauthn_setup#show_delete' @@ -245,6 +251,11 @@ delete '/manage/phone/:id' => 'users/edit_phone#destroy' get '/manage/personal_key' => 'users/personal_keys#show', as: :manage_personal_key post '/manage/personal_key' => 'users/personal_keys#update' + resource :webauthn, + path: '/manage/webauthn/:id', + controller: 'users/webauthn', + path_names: { edit: '' }, + only: [:edit, :update, :destroy] get '/account/personal_key' => 'accounts/personal_keys#new', as: :create_new_personal_key post '/account/personal_key' => 'accounts/personal_keys#create' 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 new file mode 100644 index 00000000000..3d6c4d4a7df --- /dev/null +++ b/spec/controllers/api/internal/two_factor_authentication/webauthn_controller_spec.rb @@ -0,0 +1,149 @@ +require 'rails_helper' + +RSpec.describe Api::Internal::TwoFactorAuthentication::WebauthnController do + let(:user) { create(:user, :with_phone) } + let(:configuration) { create(:webauthn_configuration, user:) } + + before do + stub_analytics + stub_sign_in(user) if user + end + + describe '#update' do + let(:name) { 'example' } + let(:params) { { id: configuration.id, name: } } + let(:response) { put :update, params: params } + subject(:response_body) { JSON.parse(response.body, symbolize_names: true) } + + it 'responds with successful result' do + expect(response_body).to eq(success: true) + expect(response.status).to eq(200) + end + + it 'logs the submission attempt' do + response + + expect(@analytics).to have_logged_event( + :webauthn_update_name_submitted, + success: true, + configuration_id: configuration.id.to_s, + error_details: nil, + ) + end + + it 'includes csrf token in the response headers' do + expect(response.headers['X-CSRF-Token']).to be_kind_of(String) + end + + context 'signed out' do + let(:user) { nil } + let(:configuration) { create(:webauthn_configuration) } + + it 'responds with unauthorized response' do + expect(response_body).to eq(error: 'Unauthorized') + expect(response.status).to eq(401) + end + end + + context 'with invalid submission' do + let(:name) { '' } + + it 'responds with unsuccessful result' do + expect(response_body).to eq(success: false, error: t('errors.messages.blank')) + expect(response.status).to eq(400) + end + + it 'logs the submission attempt' do + response + + expect(@analytics).to have_logged_event( + :webauthn_update_name_submitted, + success: false, + configuration_id: configuration.id.to_s, + error_details: { name: { blank: true } }, + ) + end + end + + context 'not recently authenticated' do + before do + allow(controller).to receive(:recently_authenticated_2fa?).and_return(false) + end + + it 'responds with unauthorized response' do + expect(response_body).to eq(error: 'Unauthorized') + expect(response.status).to eq(401) + end + end + end + + describe '#destroy' do + let(:params) { { id: configuration.id } } + let(:response) { delete :destroy, params: params } + subject(:response_body) { JSON.parse(response.body, symbolize_names: true) } + + it 'responds with successful result' do + expect(response_body).to eq(success: true) + expect(response.status).to eq(200) + end + + it 'logs the submission attempt' do + response + + expect(@analytics).to have_logged_event( + :webauthn_delete_submitted, + success: true, + configuration_id: configuration.id.to_s, + error_details: nil, + ) + end + + it 'includes csrf token in the response headers' do + expect(response.headers['X-CSRF-Token']).to be_kind_of(String) + end + + context 'signed out' do + let(:user) { nil } + let(:configuration) { create(:webauthn_configuration) } + + it 'responds with unauthorized response' do + expect(response_body).to eq(error: 'Unauthorized') + expect(response.status).to eq(401) + end + end + + context 'with invalid submission' do + let(:user) { create(:user) } + + it 'responds with unsuccessful result' do + expect(response_body).to eq( + success: false, + error: t('errors.manage_authenticator.remove_only_method_error'), + ) + expect(response.status).to eq(400) + end + + it 'logs the submission attempt' do + response + + expect(@analytics).to have_logged_event( + :webauthn_delete_submitted, + success: false, + configuration_id: configuration.id.to_s, + error_details: { configuration_id: { only_method: true } }, + ) + end + end + + context 'not recently authenticated' do + before do + allow(controller).to receive(:recently_authenticated_2fa?).and_return(false) + end + + it 'responds with unauthorized response' do + expect(response_body).to eq(error: 'Unauthorized') + expect(response.status).to eq(401) + end + end + end +end diff --git a/spec/controllers/concerns/reauthentication_required_concern_spec.rb b/spec/controllers/concerns/reauthentication_required_concern_spec.rb index c13a50096ca..fb19df845b1 100644 --- a/spec/controllers/concerns/reauthentication_required_concern_spec.rb +++ b/spec/controllers/concerns/reauthentication_required_concern_spec.rb @@ -3,21 +3,21 @@ RSpec.describe ReauthenticationRequiredConcern, type: :controller do let(:user) { create(:user, :fully_registered, email: 'old_email@example.com') } - describe '#confirm_recently_authenticated_2fa' do - controller ApplicationController do - include ReauthenticationRequiredConcern + controller ApplicationController do + include ReauthenticationRequiredConcern - before_action :confirm_recently_authenticated_2fa + before_action :confirm_recently_authenticated_2fa - def index - render plain: 'Hello' - end + def index + render plain: 'Hello' end + end - before(:each) do - stub_sign_in(user) - end + before(:each) do + stub_sign_in(user) if user + end + describe '#confirm_recently_authenticated_2fa' do context 'recently authenticated' do it 'allows action' do get :index @@ -26,6 +26,16 @@ def index end end + context 'signed out' do + let(:user) { nil } + + it 'redirects to 2FA options' do + get :index + + expect(response.body).to eq 'Hello' + end + end + context 'authenticated outside the authn window' do let(:travel_time) { (IdentityConfig.store.reauthn_window + 1).seconds } @@ -61,4 +71,33 @@ def index end end end + + describe '#recently_authenticated_2fa?' do + subject(:recently_authenticated_2fa) { controller.recently_authenticated_2fa? } + + context 'recently authenticated' do + it { expect(recently_authenticated_2fa).to eq(true) } + end + + context 'signed out' do + let(:user) { nil } + + it { expect(recently_authenticated_2fa).to eq(false) } + end + + context 'authenticated outside the authn window' do + let(:travel_time) { (IdentityConfig.store.reauthn_window + 1).seconds } + + before do + controller.auth_methods_session.authenticate!(TwoFactorAuthenticatable::AuthMethod::TOTP) + travel travel_time + end + + around do |example| + freeze_time { example.run } + end + + it { expect(recently_authenticated_2fa).to eq(false) } + end + end end diff --git a/spec/controllers/users/webauthn_controller_spec.rb b/spec/controllers/users/webauthn_controller_spec.rb new file mode 100644 index 00000000000..b87ba455b22 --- /dev/null +++ b/spec/controllers/users/webauthn_controller_spec.rb @@ -0,0 +1,158 @@ +require 'rails_helper' + +RSpec.describe Users::WebauthnController do + let(:user) { create(:user, :with_phone) } + let(:configuration) { create(:webauthn_configuration, user:) } + + before do + stub_analytics + stub_sign_in(user) if user + end + + describe '#edit' do + let(:params) { { id: configuration.id } } + let(:response) { get :edit, params: params } + + it 'assigns the form instance' do + response + + expect(assigns(:form)).to be_kind_of(TwoFactorAuthentication::WebauthnUpdateForm) + expect(assigns(:form).configuration).to eq(configuration) + end + end + + describe '#update' do + let(:name) { 'example' } + let(:params) { { id: configuration.id, form: { name: } } } + let(:response) { put :update, params: params } + + 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')) + end + + it 'assigns the form instance' do + response + + expect(assigns(:form)).to be_kind_of(TwoFactorAuthentication::WebauthnUpdateForm) + expect(assigns(:form).configuration).to eq(configuration) + end + + it 'logs the submission attempt' do + response + + expect(@analytics).to have_logged_event( + :webauthn_update_name_submitted, + success: true, + configuration_id: configuration.id.to_s, + error_details: nil, + ) + end + + context 'signed out' do + let(:user) { nil } + let(:configuration) { create(:webauthn_configuration) } + + it 'redirects to sign-in page' do + expect(response).to redirect_to(new_user_session_url) + end + end + + context 'with invalid submission' do + let(:name) { '' } + + it 'renders edit template with error' do + expect(response).to render_template(:edit) + expect(flash.now[:error]).to eq(t('errors.messages.blank')) + end + + it 'logs the submission attempt' do + response + + expect(@analytics).to have_logged_event( + :webauthn_update_name_submitted, + success: false, + configuration_id: configuration.id.to_s, + error_details: { name: { blank: true } }, + ) + end + end + + context 'not recently authenticated' do + before do + allow(controller).to receive(:recently_authenticated_2fa?).and_return(false) + end + + it 'redirects to reauthenticate' do + expect(response).to redirect_to(login_two_factor_options_path) + end + end + end + + describe '#destroy' do + let(:params) { { id: configuration.id } } + let(:response) { delete :destroy, params: params } + + 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')) + end + + it 'logs the submission attempt' do + response + + expect(@analytics).to have_logged_event( + :webauthn_delete_submitted, + success: true, + configuration_id: configuration.id.to_s, + error_details: nil, + ) + end + + it 'assigns the form instance' do + response + + expect(assigns(:form)).to be_kind_of(TwoFactorAuthentication::WebauthnDeleteForm) + expect(assigns(:form).configuration).to eq(configuration) + end + + context 'signed out' do + let(:user) { nil } + let(:configuration) { create(:webauthn_configuration) } + + it 'redirects to sign-in page' do + expect(response).to redirect_to(new_user_session_url) + end + end + + context 'with invalid submission' do + let(:user) { create(:user) } + + it 'redirects to edit with unsuccessful result' do + expect(response).to redirect_to(edit_webauthn_path(id: configuration.id)) + expect(flash[:error]).to eq(t('errors.manage_authenticator.remove_only_method_error')) + end + + it 'logs the submission attempt' do + response + + expect(@analytics).to have_logged_event( + :webauthn_delete_submitted, + success: false, + configuration_id: configuration.id.to_s, + error_details: { configuration_id: { only_method: true } }, + ) + end + end + + context 'not recently authenticated' do + before do + allow(controller).to receive(:recently_authenticated_2fa?).and_return(false) + end + + it 'redirects to reauthenticate' do + expect(response).to redirect_to(login_two_factor_options_path) + end + end + end +end diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index 96758345f05..afdd6dcdc48 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -149,14 +149,14 @@ end it 'tracks the delete in analytics' do - result = { - success: true, - mfa_method_counts: { auth_app: 1, phone: 1 }, - pii_like_keypaths: [[:mfa_method_counts, :phone]], - } - expect(@analytics).to receive(:track_event).with('WebAuthn Deleted', result) - delete :delete, params: { id: webauthn_configuration.id } + + expect(@analytics).to have_logged_event( + :webauthn_delete_submitted, + success: true, + error_details: nil, + configuration_id: webauthn_configuration.id.to_s, + ) end it 'sends a recovery information changed event' do diff --git a/spec/forms/two_factor_authentication/webauthn_delete_form_spec.rb b/spec/forms/two_factor_authentication/webauthn_delete_form_spec.rb new file mode 100644 index 00000000000..eb28c95004a --- /dev/null +++ b/spec/forms/two_factor_authentication/webauthn_delete_form_spec.rb @@ -0,0 +1,89 @@ +require 'rails_helper' + +RSpec.describe TwoFactorAuthentication::WebauthnDeleteForm do + let(:user) { create(:user) } + let(:configuration) { create(:webauthn_configuration, user:) } + let(:configuration_id) { configuration&.id } + let(:form) { described_class.new(user:, configuration_id:) } + + describe '#submit' do + let(:result) { form.submit } + + context 'with having another mfa enabled' do + let(:user) { create(:user, :with_phone) } + + it 'returns a successful result' do + expect(result.success?).to eq(true) + expect(result.to_h).to eq(success: true, configuration_id:) + end + + context 'with blank configuration' do + let(:configuration) { nil } + + it 'returns an unsuccessful result' do + expect(result.success?).to eq(false) + expect(result.to_h).to eq( + success: false, + error_details: { + configuration_id: { configuration_not_found: true }, + }, + configuration_id:, + ) + end + end + + context 'with configuration that does not exist' do + let(:configuration_id) { 'does-not-exist' } + + it 'returns an unsuccessful result' do + expect(result.success?).to eq(false) + expect(result.to_h).to eq( + success: false, + error_details: { + configuration_id: { configuration_not_found: true }, + }, + configuration_id:, + ) + end + end + + context 'with configuration not belonging to the user' do + let(:configuration) { create(:webauthn_configuration) } + + it 'returns an unsuccessful result' do + expect(result.success?).to eq(false) + expect(result.to_h).to eq( + success: false, + error_details: { + configuration_id: { configuration_not_found: true }, + }, + configuration_id:, + ) + end + end + end + + context 'with user not having another mfa enabled' do + let(:user) { create(:user) } + + it 'returns an unsuccessful result' do + expect(result.success?).to eq(false) + expect(result.to_h).to eq( + success: false, + error_details: { + configuration_id: { only_method: true }, + }, + configuration_id:, + ) + end + end + end + + describe '#configuration' do + subject(:form_configuration) { form.configuration } + + it 'returns configuration' do + expect(form_configuration).to eq(configuration) + 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 new file mode 100644 index 00000000000..8f85687ecbd --- /dev/null +++ b/spec/forms/two_factor_authentication/webauthn_update_form_spec.rb @@ -0,0 +1,142 @@ +require 'rails_helper' + +RSpec.describe TwoFactorAuthentication::WebauthnUpdateForm do + let(:user) { create(:user) } + let(:original_name) { 'original-name' } + let(:configuration) { create(:webauthn_configuration, user:, name: original_name) } + let(:configuration_id) { configuration&.id } + let(:form) { described_class.new(user:, configuration_id:) } + + describe '#submit' do + let(:name) { 'new-namae' } + let(:result) { form.submit(name:) } + + it 'returns a successful result' do + expect(result.success?).to eq(true) + expect(result.to_h).to eq(success: true, configuration_id:) + end + + it 'saves the new name' do + result + + expect(configuration.reload.name).to eq(name) + end + + context 'with blank configuration' do + let(:configuration) { nil } + + it 'returns an unsuccessful result' do + expect(result.success?).to eq(false) + expect(result.to_h).to eq( + success: false, + error_details: { + configuration_id: { configuration_not_found: true }, + }, + configuration_id:, + ) + end + end + + context 'with configuration that does not exist' do + let(:configuration_id) { 'does-not-exist' } + + it 'returns an unsuccessful result' do + expect(result.success?).to eq(false) + expect(result.to_h).to eq( + success: false, + error_details: { + configuration_id: { configuration_not_found: true }, + }, + configuration_id:, + ) + end + end + + context 'with configuration not belonging to the user' do + let(:configuration) { create(:webauthn_configuration, name: original_name) } + + it 'returns an unsuccessful result' do + expect(result.success?).to eq(false) + expect(result.to_h).to eq( + success: false, + error_details: { + configuration_id: { configuration_not_found: true }, + }, + configuration_id:, + ) + end + + it 'does not save the new name' do + expect(configuration).not_to receive(:save) + + result + + expect(configuration.reload.name).to eq(original_name) + end + end + + context 'with blank name' do + let(:name) { '' } + + it 'returns an unsuccessful result' do + expect(result.success?).to eq(false) + expect(result.to_h).to eq( + success: false, + error_details: { + name: { blank: true }, + }, + configuration_id:, + ) + end + + it 'does not save the new name' do + expect(configuration).not_to receive(:save) + + result + + expect(configuration.reload.name).to eq(original_name) + end + end + + context 'with duplicate name' do + before do + create(:webauthn_configuration, user:, name:) + end + + it 'returns an unsuccessful result' do + expect(result.success?).to eq(false) + expect(result.to_h).to eq( + success: false, + error_details: { + name: { duplicate: true }, + }, + configuration_id:, + ) + end + + it 'does not save the new name' do + expect(configuration).not_to receive(:save) + + result + + expect(configuration.reload.name).to eq(original_name) + end + end + end + + describe '#name' do + subject(:name) { form.name } + + it 'returns configuration name' do + expect(name).to eq(configuration.name) + end + end + + describe '#configuration' do + subject(:form_configuration) { form.configuration } + + it 'returns configuration' do + expect(form_configuration).to eq(configuration) + end + end +end diff --git a/spec/views/users/webauthn/edit.html.erb_spec.rb b/spec/views/users/webauthn/edit.html.erb_spec.rb new file mode 100644 index 00000000000..d5bbc9fb16a --- /dev/null +++ b/spec/views/users/webauthn/edit.html.erb_spec.rb @@ -0,0 +1,42 @@ +require 'rails_helper' + +RSpec.describe 'users/webauthn/edit.html.erb' do + include Devise::Test::ControllerHelpers + + let(:nickname) { 'Example' } + let(:configuration) { create(:webauthn_configuration, :platform_authenticator, name: nickname) } + let(:user) { create(:user, webauthn_configurations: [configuration]) } + let(:form) do + TwoFactorAuthentication::WebauthnUpdateForm.new( + user:, + configuration_id: configuration.id, + ) + end + + subject(:rendered) { render } + + before do + @form = form + end + + it 'renders form to update configuration' do + expect(rendered).to have_selector( + "form[action='#{webauthn_path(id: configuration.id)}'] input[name='_method'][value='put']", + visible: false, + ) + end + + it 'initializes form with configuration values' do + expect(rendered).to have_field( + t('two_factor_authentication.webauthn_platform.nickname'), + with: nickname, + ) + end + + it 'has labelled form with button to delete configuration' do + expect(rendered).to have_button_to_with_accessibility( + t('two_factor_authentication.webauthn_platform.delete'), + webauthn_path(id: configuration.id), + ) + end +end From d6bcd0cf251cd3759e87f6ba2112ce1cfd55bf62 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 12 Dec 2023 10:11:16 -0500 Subject: [PATCH 2/3] Add previous_event_name tag for delete event --- app/services/analytics_events.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index ee8038e0976..618e69f3c9c 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -4721,6 +4721,7 @@ def vendor_outage( # @param [Hash] error_details # @param [Integer] configuration_id # Tracks when user attempts to delete a WebAuthn configuration + # @identity.idp.previous_event_name WebAuthn Deleted def webauthn_delete_submitted( success:, configuration_id:, From c9bd16751a7363ac3eb7cc6cc5f561d9ff09670b Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 13 Dec 2023 09:28:43 -0500 Subject: [PATCH 3/3] Use explicit routes in place of resource See: https://github.com/18F/identity-idp/pull/9742#discussion_r1423255646 --- config/routes.rb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index 25023207720..78ab0a170d6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -21,7 +21,8 @@ put '/sessions' => 'sessions#update' namespace :two_factor_authentication do - resource :webauthn, path: '/webauthn/:id', controller: :webauthn, only: [:update, :destroy] + put '/webauthn/:id' => 'webauthn#update', as: :webauthn + delete '/webauthn/:id' => 'webauthn#destroy', as: nil end end end @@ -251,11 +252,9 @@ delete '/manage/phone/:id' => 'users/edit_phone#destroy' get '/manage/personal_key' => 'users/personal_keys#show', as: :manage_personal_key post '/manage/personal_key' => 'users/personal_keys#update' - resource :webauthn, - path: '/manage/webauthn/:id', - controller: 'users/webauthn', - path_names: { edit: '' }, - only: [:edit, :update, :destroy] + get '/manage/webauthn/:id' => 'users/webauthn#edit', as: :edit_webauthn + put '/manage/webauthn/:id' => 'users/webauthn#update', as: :webauthn + delete '/manage/webauthn/:id' => 'users/webauthn#destroy', as: nil get '/account/personal_key' => 'accounts/personal_keys#new', as: :create_new_personal_key post '/account/personal_key' => 'accounts/personal_keys#create'