Merge branch '220127-audit-gpg-keys-create' into 'master'

Audit successful GPG keys creation and removal

See merge request gitlab-org/gitlab!65973

app/controllers/profiles/gpg_keys_controller.rb

@@ -22,7 +22,7 @@ class Profiles::GpgKeysController < Profiles::ApplicationController
   end
 
   def destroy
-    @gpg_key.destroy
+    GpgKeys::DestroyService.new(current_user).execute(@gpg_key)
 
     respond_to do |format|
       format.html { redirect_to profile_gpg_keys_url, status: :found }

app/services/gpg_keys/create_service.rb

@@ -3,9 +3,17 @@
 module GpgKeys
   class CreateService < Keys::BaseService
     def execute
-      key = user.gpg_keys.create(params)
+      key = create(params)
       notification_service.new_gpg_key(key) if key.persisted?
       key
     end
+
+    private
+
+    def create(params)
+      user.gpg_keys.create(params)
+    end
   end
 end
+
+GpgKeys::CreateService.prepend_mod

app/services/gpg_keys/destroy_service.rb

@@ -7,3 +7,5 @@ module GpgKeys
     end
   end
 end
+
+GpgKeys::DestroyService.prepend_mod

doc/administration/audit_events.md

@@ -166,6 +166,7 @@ The following user actions are recorded:
 - 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)
+- Added or removed GPG 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/models/concerns/auditable.rb

@@ -6,4 +6,8 @@ module Auditable
 
     ::Gitlab::Audit::EventQueue.push(event)
   end
+
+  def audit_details
+    raise NotImplementedError, "#{self.class} does not implement #{__method__}"
+  end
 end

ee/app/models/ee/gpg_key.rb

@@ -4,9 +4,15 @@ module EE
   module GpgKey
     extend ActiveSupport::Concern
 
+    include Auditable
+
     prepended do
       scope :preload_users, -> { preload(:user) }
       scope :for_user, -> (user) { where(user: user) }
     end
+
+    def audit_details
+      user.name
+    end
   end
 end

ee/app/models/ee/key.rb

@@ -4,6 +4,8 @@ module EE
   module Key
     extend ActiveSupport::Concern
 
+    include Auditable
+
     prepended do
       include UsageStatistics
 
@@ -41,5 +43,9 @@ module EE
         License.feature_available?(:enforce_ssh_key_expiration)
       end
     end
+
+    def audit_details
+      title
+    end
   end
 end

ee/app/services/ee/gpg_keys/create_service.rb

+# frozen_string_literal: true
+
+module EE
+  module GpgKeys
+    module CreateService
+      extend ::Gitlab::Utils::Override
+
+      override :create
+      def create(params)
+        super.tap do |key|
+          next unless key.persisted?
+
+          audit_context = {
+            name: 'add_gpg_key',
+            author: user,
+            scope: key.user,
+            target: key,
+            message: 'Added GPG key'
+          }
+
+          ::Gitlab::Audit::Auditor.audit(audit_context)
+        end
+      end
+    end
+  end
+end

ee/app/services/ee/gpg_keys/destroy_service.rb

+# frozen_string_literal: true
+
+module EE
+  module GpgKeys
+    module DestroyService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute(key)
+        super.tap do |destroyed|
+          next unless destroyed
+
+          audit_context = {
+            name: 'remove_gpg_key',
+            author: user,
+            scope: key.user,
+            target: key,
+            message: 'Removed GPG key'
+          }
+
+          ::Gitlab::Audit::Auditor.audit(audit_context)
+        end
+      end
+    end
+  end
+end

ee/lib/gitlab/audit/target.rb

@@ -14,7 +14,7 @@ module Gitlab
       end
 
       def details
-        @object.try(:name) || @object.try(:title) || 'unknown'
+        @object.try(:name) || @object.try(:audit_details) || 'unknown'
       end
     end
   end

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

@@ -26,7 +26,7 @@ RSpec.describe Gitlab::Audit::Target do
   describe '#details' do
     using RSpec::Parameterized::TableSyntax
 
-    where(:name, :title, :details) do
+    where(:name, :audit_details, :details) do
       'jackie' | 'wanderer' | 'jackie'
       'jackie' | nil        | 'jackie'
       nil      | 'wanderer' | 'wanderer'
@@ -35,7 +35,7 @@ RSpec.describe Gitlab::Audit::Target do
 
     before do
       allow(object).to receive(:name).and_return(name) if name
-      allow(object).to receive(:title).and_return(title) if title
+      allow(object).to receive(:audit_details).and_return(audit_details) if audit_details
     end
 
     with_them do

ee/spec/models/concerns/auditable_spec.rb

@@ -34,4 +34,10 @@ RSpec.describe Auditable do
       end
     end
   end
+
+  describe '#audit_details' do
+    it 'raises error to prompt for implementation' do
+      expect { instance.audit_details }.to raise_error(/does not implement audit_details/)
+    end
+  end
 end

ee/spec/models/ee/gpg_key_spec.rb

@@ -12,4 +12,10 @@ RSpec.describe GpgKey do
 
     it { is_expected.to contain_exactly(gpg_key) }
   end
+
+  describe '#audit_details' do
+    it "equals to the user's name" do
+      expect(gpg_key.audit_details).to eq(user.name)
+    end
+  end
 end

ee/spec/models/ee/key_spec.rb

@@ -72,4 +72,11 @@ RSpec.describe Key do
       end
     end
   end
+
+  describe '#audit_details' do
+    it 'equals to the title' do
+      key = build(:personal_key)
+      expect(key.audit_details).to eq(key.title)
+    end
+  end
 end

ee/spec/services/ee/gpg_keys/create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe GpgKeys::CreateService do
+  let_it_be(:user) { create(:user) }
+
+  let(:params) { attributes_for(:gpg_key) }
+
+  subject { described_class.new(user, params) }
+
+  it 'creates an audit event', :aggregate_failures do
+    expect { subject.execute }.to change(GpgKey, :count).by(1)
+                              .and change(AuditEvent, :count).by(1)
+
+    key = user.gpg_keys.last
+
+    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.user.name,
+      details: include(custom_message: 'Added GPG key')
+    )
+  end
+
+  it 'returns the correct value' do
+    expect(subject.execute).to eq(GpgKey.last)
+  end
+
+  context 'when create operation fails' do
+    let(:params) { attributes_for(:gpg_key).except(:key) }
+
+    it 'does not create an audit event' do
+      expect { subject.execute }.not_to change(AuditEvent, :count)
+    end
+
+    it 'returns the correct value' do
+      expect(subject.execute).to be_a_new(GpgKey)
+    end
+  end
+end

ee/spec/services/ee/gpg_keys/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe GpgKeys::DestroyService do
+  let_it_be(:user) { create(:user) }
+
+  subject { described_class.new(user) }
+
+  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 GPG key')
+    )
+  end
+
+  it 'returns the correct value' do
+    key = build(:gpg_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(:gpg_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