From 2a6b737df144b2065083e8fca2aee45146ca6948 Mon Sep 17 00:00:00 2001 From: Anton Smith Date: Wed, 8 Jun 2022 19:46:14 +1200 Subject: [PATCH 1/4] Add audit event for disabling 2FA When two factor authentication is disabled for a user, this will now generate an instance level audit event. Changelog: added EE: true --- app/services/two_factor/destroy_service.rb | 8 ++++- doc/administration/audit_events.md | 1 + .../services/ee/two_factor/destroy_service.rb | 27 +++++++++++++++++ .../ee/two_factor/destroy_service_spec.rb | 30 +++++++++++++++++++ 4 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 ee/app/services/ee/two_factor/destroy_service.rb create mode 100644 ee/spec/services/ee/two_factor/destroy_service_spec.rb diff --git a/app/services/two_factor/destroy_service.rb b/app/services/two_factor/destroy_service.rb index b8bbe215d6e7d7..859012c21531c9 100644 --- a/app/services/two_factor/destroy_service.rb +++ b/app/services/two_factor/destroy_service.rb @@ -8,7 +8,7 @@ def execute result = disable_two_factor - notification_service.disabled_two_factor(user) if result[:status] == :success + notify_on_success(user) if result[:status] == :success result end @@ -20,5 +20,11 @@ def disable_two_factor user.disable_two_factor! end end + + def notify_on_success(user) + notification_service.disabled_two_factor(user) + end end end + +TwoFactor::DestroyService.prepend_mod_with('TwoFactor::DestroyService') diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 442f743f2c7744..a329adbed22efd 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -234,6 +234,7 @@ The following user actions are recorded: - Administrator added or removed ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/323905) in GitLab 14.1) - Removed SSH key ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/220127) in GitLab 14.1) - Added or removed GPG key ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/220127) in GitLab 14.1) +- A user's two-factor authentication was disabled ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/238177) in GitLab 15.1) Instance events can also be accessed via the [Instance Audit Events API](../api/audit_events.md#instance-audit-events). diff --git a/ee/app/services/ee/two_factor/destroy_service.rb b/ee/app/services/ee/two_factor/destroy_service.rb new file mode 100644 index 00000000000000..150e6a19242dac --- /dev/null +++ b/ee/app/services/ee/two_factor/destroy_service.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module EE + module TwoFactor + module DestroyService + 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, + target: user, + message: 'Disabled two-factor authentication', + created_at: DateTime.current + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + + super(user) + end + end + end +end diff --git a/ee/spec/services/ee/two_factor/destroy_service_spec.rb b/ee/spec/services/ee/two_factor/destroy_service_spec.rb new file mode 100644 index 00000000000000..6fffdb17889cab --- /dev/null +++ b/ee/spec/services/ee/two_factor/destroy_service_spec.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe TwoFactor::DestroyService do + let_it_be(:current_user) { create(:user, :two_factor) } + + subject { described_class.new(current_user, user: current_user).execute } + + context 'when disabling two-factor authentication succeeds' do + it 'creates an audit event', :aggregate_failures do + expect { subject }.to change(AuditEvent, :count).by(1) + + expect(AuditEvent.last).to have_attributes( + author: current_user, + entity_id: current_user.id, + target_id: current_user.id, + target_type: current_user.class.name, + target_details: current_user.name, + details: include(custom_message: 'Disabled two-factor authentication') + ) + end + end + + context 'when disabling two-factor authentication fails' do + it 'does not create an audit event', :aggregate_failures do + expect { subject }.not_to change(AuditEvent, :count) + end + end +end -- GitLab From 377ec2b0241c29cf223bd285ace6b3f5647fc9a2 Mon Sep 17 00:00:00 2001 From: Dmytro Biryukov Date: Fri, 10 Jun 2022 02:58:03 +0000 Subject: [PATCH 2/4] Apply 1 suggestion(s) to 1 file(s) --- ee/spec/services/ee/two_factor/destroy_service_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ee/spec/services/ee/two_factor/destroy_service_spec.rb b/ee/spec/services/ee/two_factor/destroy_service_spec.rb index 6fffdb17889cab..901e10caa7bd9d 100644 --- a/ee/spec/services/ee/two_factor/destroy_service_spec.rb +++ b/ee/spec/services/ee/two_factor/destroy_service_spec.rb @@ -23,6 +23,13 @@ end context 'when disabling two-factor authentication fails' do + before do + allow_next_instance_of(Users::UpdateService) do |instance| + allow(instance).to receive(:execute) + .and_return({ status: :fail }) + end + end + it 'does not create an audit event', :aggregate_failures do expect { subject }.not_to change(AuditEvent, :count) end -- GitLab From 9dac36cd90076bb77f8ba1cef0d9255e266fe917 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Thu, 16 Jun 2022 21:17:19 +0000 Subject: [PATCH 3/4] Apply 4 suggestion(s) to 2 file(s) --- ee/app/services/ee/two_factor/destroy_service.rb | 2 +- ee/spec/services/ee/two_factor/destroy_service_spec.rb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ee/app/services/ee/two_factor/destroy_service.rb b/ee/app/services/ee/two_factor/destroy_service.rb index 150e6a19242dac..da478a7cea077a 100644 --- a/ee/app/services/ee/two_factor/destroy_service.rb +++ b/ee/app/services/ee/two_factor/destroy_service.rb @@ -20,7 +20,7 @@ def notify_on_success(user) ::Gitlab::Audit::Auditor.audit(audit_context) - super(user) + super end end end diff --git a/ee/spec/services/ee/two_factor/destroy_service_spec.rb b/ee/spec/services/ee/two_factor/destroy_service_spec.rb index 901e10caa7bd9d..29f665943e29f2 100644 --- a/ee/spec/services/ee/two_factor/destroy_service_spec.rb +++ b/ee/spec/services/ee/two_factor/destroy_service_spec.rb @@ -5,11 +5,11 @@ RSpec.describe TwoFactor::DestroyService do let_it_be(:current_user) { create(:user, :two_factor) } - subject { described_class.new(current_user, user: current_user).execute } + subject(:disable_2fa) { described_class.new(current_user, user: current_user).execute } context 'when disabling two-factor authentication succeeds' do it 'creates an audit event', :aggregate_failures do - expect { subject }.to change(AuditEvent, :count).by(1) + expect { disable_2fa }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last).to have_attributes( author: current_user, @@ -30,7 +30,7 @@ end end - it 'does not create an audit event', :aggregate_failures do + it 'does not create an audit event' do expect { subject }.not_to change(AuditEvent, :count) end end -- GitLab From 512cb715b38ffd0e37b1c60dbe7c555c592fff8d Mon Sep 17 00:00:00 2001 From: Anton Smith Date: Fri, 17 Jun 2022 09:32:55 +1200 Subject: [PATCH 4/4] Change status to error --- ee/spec/services/ee/two_factor/destroy_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/services/ee/two_factor/destroy_service_spec.rb b/ee/spec/services/ee/two_factor/destroy_service_spec.rb index 29f665943e29f2..034fe8ebd5b16b 100644 --- a/ee/spec/services/ee/two_factor/destroy_service_spec.rb +++ b/ee/spec/services/ee/two_factor/destroy_service_spec.rb @@ -26,7 +26,7 @@ before do allow_next_instance_of(Users::UpdateService) do |instance| allow(instance).to receive(:execute) - .and_return({ status: :fail }) + .and_return({ status: :error }) end end -- GitLab