diff --git a/app/services/keys/destroy_service.rb b/app/services/keys/destroy_service.rb index eaf5eb35f5832fa4638af37447a3ec461ae60598..c7ebb484e94ee76111532571b57cd6ebbbd857fc 100644 --- a/app/services/keys/destroy_service.rb +++ b/app/services/keys/destroy_service.rb @@ -3,14 +3,22 @@ module Keys class DestroyService < ::Keys::BaseService def execute(key) - key.destroy if destroy_possible?(key) + return unless destroy_possible?(key) + + destroy(key) end + private + # overridden in EE::Keys::DestroyService def destroy_possible?(key) true end + + def destroy(key) + key.destroy + end end end -Keys::DestroyService.prepend_mod_with('Keys::DestroyService') +Keys::DestroyService.prepend_mod diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 057ee7a7eb302081adcd0ae16708a5e381f152a3..ccf632b7d6ae75b3051c00b9dda70b6322486c16 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -164,6 +164,7 @@ The following user actions are recorded: - A user's personal access token was successfully created or revoked ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/276921) in GitLab 13.6) - A failed attempt to create or revoke a user's personal access token ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/276921) in GitLab 13.6) - 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) 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/audit_events/build_service.rb b/ee/app/services/audit_events/build_service.rb index adf947d1cbe363cbcf888d6eef23a821dbe498b8..2ed2fd5def905dd402a726c479ba1ca0b6f0330b 100644 --- a/ee/app/services/audit_events/build_service.rb +++ b/ee/app/services/audit_events/build_service.rb @@ -13,7 +13,7 @@ def initialize(author:, scope:, target:, message:) @author = build_author(author) @scope = scope - @target = target + @target = build_target(target) @ip_address = build_ip_address @message = build_message(message) end @@ -60,8 +60,8 @@ def base_details_payload { author_name: @author.name, target_id: @target.id, - target_type: @target.class.name, - target_details: @target.name, + target_type: @target.type, + target_details: @target.details, custom_message: @message } end @@ -70,6 +70,10 @@ def build_author(author) author.impersonated? ? ::Gitlab::Audit::ImpersonatedAuthor.new(author) : author end + def build_target(target) + ::Gitlab::Audit::Target.new(target) + end + def build_message(message) if License.feature_available?(:admin_audit_log) && @author.impersonated? "#{message} (by #{@author.impersonated_by})" diff --git a/ee/app/services/ee/keys/destroy_service.rb b/ee/app/services/ee/keys/destroy_service.rb index 0c566d426f107e1a6935a083075b1319f0114009..483c34d3e80fb1ff7206a99f8ffb64d8b4079da3 100644 --- a/ee/app/services/ee/keys/destroy_service.rb +++ b/ee/app/services/ee/keys/destroy_service.rb @@ -9,6 +9,23 @@ module DestroyService def destroy_possible?(key) super && !key.is_a?(LDAPKey) end + + override :destroy + def destroy(key) + super.tap do |destroyed| + next unless destroyed + + audit_context = { + name: 'remove_ssh_key', + author: user, + scope: key.user, + target: key, + message: 'Removed SSH key' + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end end end end diff --git a/ee/lib/gitlab/audit/target.rb b/ee/lib/gitlab/audit/target.rb new file mode 100644 index 0000000000000000000000000000000000000000..7bfd2994915a445702138db42b731afc6ce6baf9 --- /dev/null +++ b/ee/lib/gitlab/audit/target.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Gitlab + module Audit + class Target + delegate :id, to: :@object + + def initialize(object) + @object = object + end + + def type + @object.class.name + end + + def details + @object.try(:name) || @object.try(:title) || 'unknown' + end + end + end +end diff --git a/ee/spec/lib/gitlab/audit/target_spec.rb b/ee/spec/lib/gitlab/audit/target_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..d4441d11576d076e95f0856a9c63a7cda8733941 --- /dev/null +++ b/ee/spec/lib/gitlab/audit/target_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Audit::Target do + let(:object) { double('object') } + + subject { described_class.new(object) } + + describe '#id' do + it 'returns object id' do + allow(object).to receive(:id).and_return(object_id) + + expect(subject.id).to eq(object_id) + end + end + + describe '#type' do + it 'returns object class name' do + allow(object).to receive_message_chain(:class, :name).and_return('User') + + expect(subject.type).to eq('User') + end + end + + describe '#details' do + using RSpec::Parameterized::TableSyntax + + where(:name, :title, :details) do + 'jackie' | 'wanderer' | 'jackie' + 'jackie' | nil | 'jackie' + nil | 'wanderer' | 'wanderer' + nil | nil | 'unknown' + end + + before do + allow(object).to receive(:name).and_return(name) if name + allow(object).to receive(:title).and_return(title) if title + end + + with_them do + it 'returns details' do + expect(subject.details).to eq(details) + end + end + end +end diff --git a/ee/spec/services/ee/keys/destroy_service_spec.rb b/ee/spec/services/ee/keys/destroy_service_spec.rb index be5771c2fb844ecce3431bbfd1a0c3340bd24426..daba122609674a3f3fa8ab475448d6f3fdf8b82d 100644 --- a/ee/spec/services/ee/keys/destroy_service_spec.rb +++ b/ee/spec/services/ee/keys/destroy_service_spec.rb @@ -3,8 +3,7 @@ require 'spec_helper' RSpec.describe Keys::DestroyService do - let(:user) { create(:user) } - let(:key) { create(:ldap_key) } + let_it_be(:user) { create(:user) } subject { described_class.new(user) } @@ -14,4 +13,42 @@ expect { subject.execute(key) }.not_to change(Key, :count) expect(key).not_to be_destroyed end + + it 'creates an audit event', :aggregate_failures do + key = create(:personal_key) + + expect { subject.execute(key) }.to change(AuditEvent, :count).by(1) + + expect(AuditEvent.last).to have_attributes( + author: user, + entity_id: key.user.id, + target_id: key.id, + target_type: key.class.name, + target_details: key.title, + details: include(custom_message: 'Removed SSH key') + ) + end + + it 'returns the correct value' do + key = build(:personal_key) + allow(key).to receive(:destroy).and_return(true) + + expect(subject.execute(key)).to eq(true) + end + + context 'when destroy operation fails' do + let(:key) { build(:personal_key) } + + before do + allow(key).to receive(:destroy).and_return(false) + end + + it 'does not create an audit event' do + expect { subject.execute(key) }.not_to change(AuditEvent, :count) + end + + it 'returns the correct value' do + expect(subject.execute(key)).to eq(false) + end + end end diff --git a/spec/services/keys/destroy_service_spec.rb b/spec/services/keys/destroy_service_spec.rb index 59ce4a941c701d8b386001a49d2bb3a3872bf2c1..dd40f9d73fd103e548b957a0a9fa50c1595b83f4 100644 --- a/spec/services/keys/destroy_service_spec.rb +++ b/spec/services/keys/destroy_service_spec.rb @@ -8,7 +8,7 @@ subject { described_class.new(user) } it 'destroys a key' do - key = create(:key) + key = create(:personal_key) expect { subject.execute(key) }.to change(Key, :count).by(-1) end