Merge branch '220127-audit-ssh-keys-destroy' into 'master'

Audit key destroy action

See merge request gitlab-org/gitlab!65615

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

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).
 

ee/app/services/audit_events/build_service.rb

@@ -13,7 +13,7 @@ module AuditEvents
 
       @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 @@ module AuditEvents
       {
         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 @@ module AuditEvents
       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})"

ee/app/services/ee/keys/destroy_service.rb

@@ -9,6 +9,23 @@ module EE
       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

ee/lib/gitlab/audit/target.rb

+# 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

ee/spec/lib/gitlab/audit/target_spec.rb

+# 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

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 @@ RSpec.describe Keys::DestroyService do
     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

spec/services/keys/destroy_service_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Keys::DestroyService do
   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