From 6eca5a5c8204681ab7afd184e364c73d53effac4 Mon Sep 17 00:00:00 2001 From: Jio Castillo Date: Fri, 26 Sep 2025 21:10:14 -0700 Subject: [PATCH] Extend notifications and audit events for OTP and WebAuthn disablement Changelog: changed EE: true --- .../profiles/two_factor_auths_controller.rb | 8 +- app/mailers/emails/profile.rb | 23 +++++ app/mailers/previews/notify_preview.rb | 8 ++ app/models/user.rb | 4 + app/services/notification_service.rb | 10 ++- .../two_factor/destroy_otp_service.rb | 16 +++- app/services/webauthn/destroy_service.rb | 26 +++++- .../disabled_two_factor_otp_email.html.haml | 6 ++ .../disabled_two_factor_otp_email.text.erb | 5 ++ ...sabled_two_factor_webauthn_email.html.haml | 8 ++ ...isabled_two_factor_webauthn_email.text.erb | 7 ++ .../ee/two_factor/destroy_otp_service.rb | 26 ++++++ .../services/ee/webauthn/destroy_service.rb | 29 ++++++ .../two_factor_auths_controller_spec.rb | 10 ++- .../ee/two_factor/destroy_otp_service_spec.rb | 83 +++++++++++++++++ .../ee/webauthn/destroy_service_spec.rb | 88 +++++++++++++++++++ locale/gitlab.pot | 12 +++ .../two_factor_auths_controller_spec.rb | 65 +++++++++++++- spec/mailers/emails/profile_spec.rb | 72 ++++++++++++--- spec/services/notification_service_spec.rb | 24 ++++- .../two_factor/destroy_otp_service_spec.rb | 26 ++++++ .../services/webauthn/destroy_service_spec.rb | 70 ++++++++++++++- 22 files changed, 600 insertions(+), 26 deletions(-) create mode 100644 app/views/notify/disabled_two_factor_otp_email.html.haml create mode 100644 app/views/notify/disabled_two_factor_otp_email.text.erb create mode 100644 app/views/notify/disabled_two_factor_webauthn_email.html.haml create mode 100644 app/views/notify/disabled_two_factor_webauthn_email.text.erb create mode 100644 ee/app/services/ee/two_factor/destroy_otp_service.rb create mode 100644 ee/app/services/ee/webauthn/destroy_service.rb create mode 100644 ee/spec/services/ee/two_factor/destroy_otp_service_spec.rb create mode 100644 ee/spec/services/ee/webauthn/destroy_service_spec.rb diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index 046936ec5a8df4..43d35d9a7a4027 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -116,9 +116,13 @@ def destroy_otp end def destroy_webauthn - Webauthn::DestroyService.new(current_user, current_user, params[:id]).execute + result = Webauthn::DestroyService.new(current_user, current_user, params[:id]).execute - redirect_to profile_two_factor_auth_path, status: :found, notice: _("Successfully deleted WebAuthn device.") + if result[:status] == :success + redirect_to profile_two_factor_auth_path, status: :found, notice: _("Successfully deleted WebAuthn device.") + else + redirect_to profile_two_factor_auth_path, status: :found, alert: result[:message] + end end def skip diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index 92df173a4d98dc..5264e680d9a71d 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -264,6 +264,29 @@ def disabled_two_factor_email(user) ) end + def disabled_two_factor_otp_email(user) + return unless user + + @user = user + + email_with_layout( + to: @user.notification_email_or_default, + subject: subject(_("Two-factor authentication deleted - OTP")) + ) + end + + def disabled_two_factor_webauthn_email(user, device_name) + return unless user + + @user = user + @device_name = device_name + + email_with_layout( + to: @user.notification_email_or_default, + subject: subject(_("Two-factor authentication deleted - WebAuthn")) + ) + end + def new_email_address_added_email(user, email) return unless user diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb index 9961c4fd79aae1..51d4eb497f9afe 100644 --- a/app/mailers/previews/notify_preview.rb +++ b/app/mailers/previews/notify_preview.rb @@ -301,6 +301,14 @@ def disabled_two_factor_email Notify.disabled_two_factor_email(user).message end + def disabled_two_factor_otp_email + Notify.disabled_two_factor_otp_email(user).message + end + + def disabled_two_factor_webauthn_email + Notify.disabled_two_factor_webauthn_email(user, 'Macbook Touch ID').message + end + def new_email_address_added_email Notify.new_email_address_added_email(user, 'someone@gitlab.com').message end diff --git a/app/models/user.rb b/app/models/user.rb index 34a08354aa3892..d35d46b188cdc8 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1333,6 +1333,10 @@ def disable_webauthn! self.webauthn_registrations.destroy_all # rubocop:disable Cop/DestroyAll end + def destroy_webauthn_device(device_id) + self.webauthn_registrations.find(device_id).destroy + end + def reset_backup_codes! update(otp_backup_codes: nil) end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 5ef160b4253fc7..fddfa027c88525 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -56,10 +56,16 @@ def enabled_two_factor(user, type, options = {}) end end - def disabled_two_factor(user) + def disabled_two_factor(user, type = :two_factor, options = {}) return unless user.can?(:receive_notifications) - mailer.disabled_two_factor_email(user).deliver_later + if type == :webauthn + mailer.disabled_two_factor_webauthn_email(user, options[:device_name]).deliver_later + elsif type == :otp + mailer.disabled_two_factor_otp_email(user).deliver_later + else + mailer.disabled_two_factor_email(user).deliver_later + end end # Always notify user about ssh key added diff --git a/app/services/two_factor/destroy_otp_service.rb b/app/services/two_factor/destroy_otp_service.rb index 7947d5d05edf41..270b2875a25409 100644 --- a/app/services/two_factor/destroy_otp_service.rb +++ b/app/services/two_factor/destroy_otp_service.rb @@ -9,7 +9,11 @@ def execute return error(_('This user does not have a one-time password authenticator registered.')) end - ::Users::UpdateService.new(current_user, user: user).execute(&:disable_two_factor_otp!) + result = disable_two_factor_otp + + notify_on_success(user) if result[:status] == :success + + result end private @@ -17,5 +21,15 @@ def execute def authorized? can?(current_user, :disable_two_factor, user) end + + def disable_two_factor_otp + ::Users::UpdateService.new(current_user, user: user).execute(&:disable_two_factor_otp!) + end + + def notify_on_success(user) + notification_service.disabled_two_factor(user, :otp) + end end end + +TwoFactor::DestroyOtpService.prepend_mod diff --git a/app/services/webauthn/destroy_service.rb b/app/services/webauthn/destroy_service.rb index afad2680d42ea3..05512926ce8c63 100644 --- a/app/services/webauthn/destroy_service.rb +++ b/app/services/webauthn/destroy_service.rb @@ -13,8 +13,18 @@ def initialize(current_user, user, webauthn_registrations_id) def execute return error(_('You are not authorized to perform this action')) unless authorized? - webauthn_registration.destroy - user.reset_backup_codes! if last_two_factor_registration? + result = destroy_webauthn_device + + if result[:status] == :success + notify_on_success(user, webauthn_registration.name) + + if last_two_factor_registration? + user.reset_backup_codes! + notification_service.disabled_two_factor(user) + end + end + + result end private @@ -26,5 +36,17 @@ def last_two_factor_registration? def authorized? current_user.can?(:disable_two_factor, user) end + + def destroy_webauthn_device + ::Users::UpdateService.new(current_user, user: user).execute do |user| + user.destroy_webauthn_device(webauthn_registration.id) + end + end + + def notify_on_success(user, device_name) + notification_service.disabled_two_factor(user, :webauthn, { device_name: device_name }) + end end end + +Webauthn::DestroyService.prepend_mod diff --git a/app/views/notify/disabled_two_factor_otp_email.html.haml b/app/views/notify/disabled_two_factor_otp_email.html.haml new file mode 100644 index 00000000000000..0a54c8b7209f31 --- /dev/null +++ b/app/views/notify/disabled_two_factor_otp_email.html.haml @@ -0,0 +1,6 @@ +%p + = say_hi(@user) +%p + = _('A one-time password (OTP) authenticator has been deleted from your GitLab account.') +%p + = re_enable_two_factor_authentication_text(format: :html) diff --git a/app/views/notify/disabled_two_factor_otp_email.text.erb b/app/views/notify/disabled_two_factor_otp_email.text.erb new file mode 100644 index 00000000000000..bc009152db7b1a --- /dev/null +++ b/app/views/notify/disabled_two_factor_otp_email.text.erb @@ -0,0 +1,5 @@ +<%= say_hi(@user) %> + +<%= _('A one-time password (OTP) authenticator has been deleted from your GitLab account.') %> + +<%= re_enable_two_factor_authentication_text %> diff --git a/app/views/notify/disabled_two_factor_webauthn_email.html.haml b/app/views/notify/disabled_two_factor_webauthn_email.html.haml new file mode 100644 index 00000000000000..79ab87419cd2d1 --- /dev/null +++ b/app/views/notify/disabled_two_factor_webauthn_email.html.haml @@ -0,0 +1,8 @@ +%p + = say_hi(@user) +%p + = _('A WebAuthn device has been deleted from your GitLab account.') + %ul + %li= _('Device name: %{device_name}') % { device_name: sanitize_name(@device_name) } +%p + = re_enable_two_factor_authentication_text(format: :html) diff --git a/app/views/notify/disabled_two_factor_webauthn_email.text.erb b/app/views/notify/disabled_two_factor_webauthn_email.text.erb new file mode 100644 index 00000000000000..1039a2eaae220b --- /dev/null +++ b/app/views/notify/disabled_two_factor_webauthn_email.text.erb @@ -0,0 +1,7 @@ +<%= say_hi(@user) %> + +<%= _('A WebAuthn device has been deleted from your GitLab account.') %> + +<%= _('- Device name: %{device_name}') % { device_name: sanitize_name(@device_name) } %> + +<%= re_enable_two_factor_authentication_text %> diff --git a/ee/app/services/ee/two_factor/destroy_otp_service.rb b/ee/app/services/ee/two_factor/destroy_otp_service.rb new file mode 100644 index 00000000000000..f25bd675c3c788 --- /dev/null +++ b/ee/app/services/ee/two_factor/destroy_otp_service.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module EE + module TwoFactor # rubocop:disable Gitlab/BoundedContexts -- Overriding existing file + module DestroyOtpService + extend ::Gitlab::Utils::Override + + private + + override :notify_on_success + def notify_on_success(user) + audit_context = { + name: 'user_disable_two_factor', + author: current_user, + scope: user&.enterprise_group.presence || user, + target: user, + message: 'Disabled One-time password authenticator' + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + + super + end + end + end +end diff --git a/ee/app/services/ee/webauthn/destroy_service.rb b/ee/app/services/ee/webauthn/destroy_service.rb new file mode 100644 index 00000000000000..ddee4d0c7ed9f5 --- /dev/null +++ b/ee/app/services/ee/webauthn/destroy_service.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module EE + module Webauthn # rubocop:disable Gitlab/BoundedContexts -- Overriding existing file + module DestroyService + extend ::Gitlab::Utils::Override + + private + + override :notify_on_success + def notify_on_success(user, device_name) + audit_context = { + name: 'user_disable_two_factor', + author: current_user, + scope: user&.enterprise_group.presence || user, + target: user, + message: 'Deleted WebAuthn device', + additional_details: { + device_name: device_name + } + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + + super + end + end + end +end diff --git a/ee/spec/controllers/ee/profiles/two_factor_auths_controller_spec.rb b/ee/spec/controllers/ee/profiles/two_factor_auths_controller_spec.rb index c4c8e67b55bd4f..881b1296c1a844 100644 --- a/ee/spec/controllers/ee/profiles/two_factor_auths_controller_spec.rb +++ b/ee/spec/controllers/ee/profiles/two_factor_auths_controller_spec.rb @@ -154,7 +154,10 @@ def device_response target_id: user.id, target_type: user.class.name, target_details: user.name, - details: include(custom_message: 'Registered WebAuthn device') + details: hash_including( + custom_message: 'Registered WebAuthn device', + device_name: 'touch id' + ) ) end @@ -175,7 +178,10 @@ def device_response target_id: user.id, target_type: user.class.name, target_details: user.name, - details: include(custom_message: 'Registered WebAuthn device') + details: hash_including( + custom_message: 'Registered WebAuthn device', + device_name: 'touch id' + ) ) end end diff --git a/ee/spec/services/ee/two_factor/destroy_otp_service_spec.rb b/ee/spec/services/ee/two_factor/destroy_otp_service_spec.rb new file mode 100644 index 00000000000000..09658db7a6cca2 --- /dev/null +++ b/ee/spec/services/ee/two_factor/destroy_otp_service_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe TwoFactor::DestroyOtpService, feature_category: :system_access do + let_it_be(:current_user) { create(:user) } + + subject(:execute) { described_class.new(current_user, user: user).execute } + + describe 'audit events' do + context 'when licensed' do + before do + stub_licensed_features(admin_audit_log: true, audit_events: true, extended_audit_events: true) + end + + context 'when disabling the OTP authenticator' do + context 'when the user has an OTP authenticator enabled' do + let_it_be(:current_user) { create(:user, :two_factor_via_otp) } + let_it_be(:user) { current_user } + + it 'creates an audit event', :aggregate_failures do + expect { execute }.to change { AuditEvent.count }.by(1) + + expect(AuditEvent.last).to have_attributes( + author: current_user, + entity_type: "User", + entity_id: user.id, + target_id: user.id, + target_type: user.class.name, + target_details: user.name, + details: include(custom_message: 'Disabled One-time password authenticator') + ) + end + + context 'when on SaaS', :saas do + context 'when user is an Enterprise User', :aggregate_failures do + let_it_be(:enterprise_group) { create(:group) } + let_it_be(:current_user) do + create(:enterprise_user, :two_factor_via_otp, :with_namespace, enterprise_group: enterprise_group) + end + + let_it_be(:user) { current_user } + + it 'creates a group audit event' do + expect { execute }.to change { AuditEvent.count }.by(1) + + expect(AuditEvent.last).to have_attributes( + author: current_user, + entity_type: "Group", + entity_id: enterprise_group.id, + target_id: user.id, + target_type: user.class.name, + target_details: user.name, + details: include(custom_message: 'Disabled One-time password authenticator') + ) + end + end + end + end + + context 'when the user does not have an OTP authenticator enabled' do + let(:user) { current_user } + + it 'does not track audit event' do + expect { execute }.not_to change { AuditEvent.count } + end + end + end + end + + context 'when unlicensed' do + before do + stub_licensed_features(admin_audit_log: false, audit_events: false, extended_audit_events: false) + end + + let(:user) { create(:user, :two_factor_via_otp) } + + it 'does not track audit event' do + expect { execute }.not_to change { AuditEvent.count } + end + end + end +end diff --git a/ee/spec/services/ee/webauthn/destroy_service_spec.rb b/ee/spec/services/ee/webauthn/destroy_service_spec.rb new file mode 100644 index 00000000000000..4f9d779a841d8e --- /dev/null +++ b/ee/spec/services/ee/webauthn/destroy_service_spec.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Webauthn::DestroyService, feature_category: :system_access do + let_it_be(:user) { create(:user, :two_factor_via_webauthn, registrations_count: 1) } + let_it_be(:current_user) { user } + let(:webauthn_registration) { user.webauthn_registrations.first } + let(:webauthn_id) { webauthn_registration.id } + let(:webauthn_name) { webauthn_registration.name } + + subject(:execute) { described_class.new(current_user, user, webauthn_id).execute } + + describe 'audit events' do + context 'when licensed' do + before do + stub_licensed_features(admin_audit_log: true, audit_events: true, extended_audit_events: true) + end + + context 'when disabling a WebAuthn device' do + context 'when the user has two-factor authentication enabled' do + it 'creates an audit event', :aggregate_failures do + expect { execute }.to change { AuditEvent.count }.by(1) + + expect(AuditEvent.last).to have_attributes( + author: current_user, + entity_type: "User", + entity_id: user.id, + target_id: user.id, + target_type: user.class.name, + target_details: user.name, + details: hash_including( + custom_message: 'Deleted WebAuthn device', + device_name: webauthn_name + ) + ) + end + + context 'when on SaaS', :saas do + context 'when user is an Enterprise User', :aggregate_failures do + let_it_be(:enterprise_group) { create(:group) } + let_it_be(:current_user) do + create(:enterprise_user, :with_namespace, :two_factor_via_webauthn, registrations_count: 1, + enterprise_group: enterprise_group) + end + + let(:user) { current_user } + let(:webauthn_registration) { user.webauthn_registrations.first } + let(:webauthn_id) { webauthn_registration.id } + let(:webauthn_name) { webauthn_registration.name } + + it 'creates a group audit event' do + expect { execute }.to change { AuditEvent.count }.by(1) + + expect(AuditEvent.last).to have_attributes( + author: current_user, + entity_type: "Group", + entity_id: enterprise_group.id, + target_id: user.id, + target_type: user.class.name, + target_details: user.name, + details: hash_including( + custom_message: 'Deleted WebAuthn device', + device_name: webauthn_name + ) + ) + end + end + end + end + end + end + + context 'when unlicensed' do + before do + stub_licensed_features(admin_audit_log: false, audit_events: false, extended_audit_events: false) + end + + let_it_be(:user) { create(:user, :two_factor_via_webauthn, registrations_count: 1) } + let_it_be(:current_user) { user } + let(:webauthn_id) { webauthn_registration.id } + + it 'does not track audit event' do + expect { execute }.not_to change { AuditEvent.count } + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index b2417bed6761f6..4feca81ce294c6 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -2204,6 +2204,9 @@ msgstr "" msgid "A Let's Encrypt SSL certificate can not be obtained until your domain is verified." msgstr "" +msgid "A WebAuthn device has been deleted from your GitLab account." +msgstr "" + msgid "A WebAuthn device has been registered to your GitLab account." msgstr "" @@ -2291,6 +2294,9 @@ msgstr "" msgid "A non-confidential work item cannot have a confidential parent." msgstr "" +msgid "A one-time password (OTP) authenticator has been deleted from your GitLab account." +msgstr "" + msgid "A one-time password (OTP) authenticator has been registered to your GitLab account." msgstr "" @@ -68782,6 +68788,12 @@ msgstr "" msgid "Two-factor authentication" msgstr "" +msgid "Two-factor authentication deleted - OTP" +msgstr "" + +msgid "Two-factor authentication deleted - WebAuthn" +msgstr "" + msgid "Two-factor authentication disabled" msgstr "" diff --git a/spec/controllers/profiles/two_factor_auths_controller_spec.rb b/spec/controllers/profiles/two_factor_auths_controller_spec.rb index 5ad6f9d3874e36..7411388816e18e 100644 --- a/spec/controllers/profiles/two_factor_auths_controller_spec.rb +++ b/spec/controllers/profiles/two_factor_auths_controller_spec.rb @@ -458,6 +458,25 @@ def go end it_behaves_like 'user must enter a valid current password' + + it 'sends the user a notification email' do + expect_next_instance_of(NotificationService) do |notification| + expect(notification).to receive(:disabled_two_factor).with(user, :otp) + end + + go + end + + context 'when an invalid password is given' do + before do + allow(NotificationService).to receive(:new) + end + + it 'does not send a notification email' do + delete :destroy_otp, params: { current_password: 'invalid' } + expect(NotificationService).not_to have_received(:new) + end + end end context 'for a user that has only WebAuthn enabled' do @@ -498,7 +517,9 @@ def go create(:user, :two_factor_via_webauthn) end - let(:webauthn_id) { user.webauthn_registrations.first.id } + let(:webauthn_registration) { user.webauthn_registrations.first } + let(:webauthn_id) { webauthn_registration.id } + let(:webauthn_name) { webauthn_registration.name } let(:current_password) { user.password } let(:destroy_webauthn) do delete :destroy_webauthn, params: { id: webauthn_id, current_password: current_password } @@ -522,16 +543,56 @@ def go expect(response).to redirect_to profile_two_factor_auth_path end + it 'displays a notice on success' do + go + + expect(flash[:notice]).to eq _('Successfully deleted WebAuthn device.') + end + it 'calls the Webauthn::DestroyService' do service = double expect(Webauthn::DestroyService).to receive(:new).with(user, user, webauthn_id.to_s).and_return(service) - expect(service).to receive(:execute) + expect(service).to receive(:execute).and_return(ServiceResponse.success) go end it_behaves_like 'user must enter a valid current password' + + it 'sends the user a notification email' do + expect_next_instance_of(NotificationService) do |notification| + expect(notification).to receive(:disabled_two_factor).with(user, :webauthn, { device_name: webauthn_name }) + end + + go + end + + context 'when an invalid password is given' do + let(:current_password) { 'invalid' } + + before do + allow(NotificationService).to receive(:new) + end + + it 'does not send a notification email' do + go + expect(NotificationService).not_to have_received(:new) + end + end + + context 'when it fails to delete' do + it 'returns an error message' do + expect_next_instance_of(Webauthn::DestroyService) do |service| + expect(service).to receive(:execute).and_return({ status: :error, message: 'an error occurred' }) + end + + go + + expect(response).to redirect_to(profile_two_factor_auth_path) + expect(flash[:alert]).to eq('an error occurred') + end + end end describe 'PATCH skip' do diff --git a/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb index c84d680fece6c0..7b36bb785bb751 100644 --- a/spec/mailers/emails/profile_spec.rb +++ b/spec/mailers/emails/profile_spec.rb @@ -728,25 +728,75 @@ end end - describe 'disabled two-factor authentication email' do + describe 'disabled two-factor authentication emails' do let_it_be(:user) { create(:user) } - subject { Notify.disabled_two_factor_email(user) } + describe 'Two Factor' do + subject { Notify.disabled_two_factor_email(user) } - it_behaves_like 'an email sent from GitLab' - it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like 'a user cannot unsubscribe through footer link' + it_behaves_like 'an email sent from GitLab' + it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like 'a user cannot unsubscribe through footer link' - it 'is sent to the user' do - is_expected.to deliver_to user.email + it 'is sent to the user' do + is_expected.to deliver_to user.email + end + + it 'has the correct subject' do + is_expected.to have_subject(/^Two-factor authentication disabled$/i) + end + + it 'includes a link to two-factor authentication settings page' do + is_expected.to have_body_text(/#{profile_two_factor_auth_path}/) + end end - it 'has the correct subject' do - is_expected.to have_subject(/^Two-factor authentication disabled$/i) + describe 'OTP' do + let_it_be(:user) { create(:user) } + + subject { Notify.disabled_two_factor_otp_email(user) } + + it_behaves_like 'an email sent from GitLab' + it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like 'a user cannot unsubscribe through footer link' + + it 'is sent to the user' do + is_expected.to deliver_to user.email + end + + it 'has the correct subject' do + is_expected.to have_subject(/^Two-factor authentication deleted - OTP$/i) + end + + it 'includes a link to two-factor authentication settings page' do + is_expected.to have_body_text(/#{profile_two_factor_auth_path}/) + end end - it 'includes a link to two-factor authentication settings page' do - is_expected.to have_body_text(/#{profile_two_factor_auth_path}/) + describe 'WebAuthn' do + let_it_be(:user) { create(:user) } + + subject { Notify.disabled_two_factor_webauthn_email(user, 'Macbook Touch ID') } + + it_behaves_like 'an email sent from GitLab' + it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like 'a user cannot unsubscribe through footer link' + + it 'is sent to the user' do + is_expected.to deliver_to user.email + end + + it 'has the correct subject' do + is_expected.to have_subject(/^Two-factor authentication deleted - WebAuthn$/i) + end + + it 'includes the device name' do + is_expected.to have_body_text('Macbook Touch ID') + end + + it 'includes a link to two-factor authentication settings page' do + is_expected.to have_body_text(/#{profile_two_factor_auth_path}/) + end end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 63a59630554330..8e10f588ff1944 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -1010,10 +1010,28 @@ describe '#disabled_two_factor' do let_it_be(:user) { create(:user) } - subject { notification.disabled_two_factor(user) } + describe 'Two Factor' do + subject { notification.disabled_two_factor(user) } - it 'sends email to the user' do - expect { subject }.to have_enqueued_email(user, mail: 'disabled_two_factor_email') + it 'sends email to the user' do + expect { subject }.to have_enqueued_email(user, mail: 'disabled_two_factor_email') + end + end + + describe 'Time-based OTP' do + subject { notification.disabled_two_factor(user, :otp) } + + it 'sends email to the user' do + expect { subject }.to have_enqueued_email(user, mail: 'disabled_two_factor_otp_email') + end + end + + describe 'WebAuthn' do + subject { notification.disabled_two_factor(user, :webauthn, device_name: 'Macbook Touch ID') } + + it 'sends email to the user' do + expect { subject }.to have_enqueued_email(user, 'Macbook Touch ID', mail: 'disabled_two_factor_webauthn_email') + end end end diff --git a/spec/services/two_factor/destroy_otp_service_spec.rb b/spec/services/two_factor/destroy_otp_service_spec.rb index 289037a8c91819..827106d1c7ad3d 100644 --- a/spec/services/two_factor/destroy_otp_service_spec.rb +++ b/spec/services/two_factor/destroy_otp_service_spec.rb @@ -9,6 +9,10 @@ context 'when disabling the OTP authenticator' do context 'when the user does not have two-factor authentication enabled' do + before do + allow(NotificationService).to receive(:new) + end + let(:user) { current_user } it 'returns error' do @@ -19,11 +23,20 @@ } ) end + + it 'does not send a notification email' do + execute + expect(NotificationService).not_to have_received(:new) + end end context 'when the user has two-factor authentication enabled' do context 'when the executor is not authorized to disable the OTP authenticator' do context 'when disabling the OTP authenticator of another user' do + before do + allow(NotificationService).to receive(:new) + end + let(:user) { create(:user, :two_factor_via_otp) } it 'returns error' do @@ -38,6 +51,11 @@ it 'does not disable the OTP authenticator' do expect { execute }.not_to change { user.reload.two_factor_otp_enabled? }.from(true) end + + it 'does not send a notification email' do + execute + expect(NotificationService).not_to have_received(:new) + end end end @@ -50,6 +68,14 @@ it 'disables the OTP authenticator of the user' do expect { execute }.to change { user.reload.two_factor_otp_enabled? }.from(true).to(false) end + + it 'sends the user a notification email' do + expect_next_instance_of(NotificationService) do |notification| + expect(notification).to receive(:disabled_two_factor).with(user, :otp) + end + + execute + end end context 'when disabling their own OTP authenticator' do diff --git a/spec/services/webauthn/destroy_service_spec.rb b/spec/services/webauthn/destroy_service_spec.rb index dd04601ccf08dd..d98043f4a8b5c1 100644 --- a/spec/services/webauthn/destroy_service_spec.rb +++ b/spec/services/webauthn/destroy_service_spec.rb @@ -7,13 +7,19 @@ let(:current_user) { user } describe '#execute' do - let(:webauthn_id) { user.webauthn_registrations.first.id } + let(:webauthn_registration) { user.webauthn_registrations.first } + let(:webauthn_id) { webauthn_registration.id } + let(:webauthn_name) { webauthn_registration.name } subject { described_class.new(current_user, user, webauthn_id).execute } context 'with only one webauthn method enabled' do context 'when another user is calling the service' do context 'for a user without permissions' do + before do + allow(NotificationService).to receive(:new) + end + let(:current_user) { create(:user) } it 'does not destry the webauthn registration' do @@ -27,6 +33,11 @@ it 'returns error' do expect(subject[:status]).to eq(:error) end + + it 'does not send a notification email' do + subject + expect(NotificationService).not_to have_received(:new) + end end context 'for an admin' do @@ -39,10 +50,45 @@ expect(user.otp_backup_codes).to be_nil end + + it 'sends the user notification emails' do + expect_next_instance_of(NotificationService) do |notification| + expect(notification).to receive(:disabled_two_factor).with(user, :webauthn, + { device_name: webauthn_name }) + end + + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive(:disabled_two_factor).with(user) + end + + subject + end end end context 'when current user is calling the service' do + it 'removes the webauth registrations' do + expect { subject }.to change { user.webauthn_registrations.count }.by(-1) + end + + it 'removes the user backup codes' do + subject + expect(user.otp_backup_codes).to be_nil + end + + it 'sends the user notification emails' do + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive(:disabled_two_factor).with(user, :webauthn, + { device_name: webauthn_name }) + end + + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive(:disabled_two_factor).with(user) + end + + subject + end + context 'when there is also OTP enabled' do before do user.otp_required_for_login = true @@ -59,6 +105,17 @@ it 'does not remove the user backup codes' do expect { subject }.not_to change { user.otp_backup_codes } end + + it 'sends the user a notification email' do + expect_next_instance_of(NotificationService) do |notification| + expect(notification).to receive(:disabled_two_factor).with(user, :webauthn, + { device_name: webauthn_name }) + end + + expect(NotificationService).not_to receive(:new) + + subject + end end end end @@ -75,6 +132,17 @@ it 'does not remove the user backup codes' do expect { subject }.not_to change { user.otp_backup_codes } end + + it 'sends the user a notification email' do + expect_next_instance_of(NotificationService) do |notification| + expect(notification).to receive(:disabled_two_factor).with(user, :webauthn, + { device_name: webauthn_name }) + end + + expect(NotificationService).not_to receive(:new) + + subject + end end end end -- GitLab