Merge branch 'pedropombeiro/349540/2-split-audit-service' into 'master'

Refactor runner registration audit classes

See merge request gitlab-org/gitlab!80623

ee/app/services/audit_events/register_runner_audit_event_service.rb

+# frozen_string_literal: true
+
+module AuditEvents
+  class RegisterRunnerAuditEventService < RunnerRegistrationAuditEventService
+    def token_field
+      :runner_registration_token
+    end
+
+    def message
+      return "Registered #{runner_type} CI runner" if @runner.valid?
+
+      "Failed to register #{runner_type} CI runner"
+    end
+
+    def runner_path
+      super if @runner.persisted?
+    end
+  end
+end

ee/app/services/audit_events/runner_registration_audit_event_service.rb

@@ -2,10 +2,14 @@
 
 module AuditEvents
   class RunnerRegistrationAuditEventService < ::AuditEventService
-    def initialize(runner, registration_token, token_scope, action)
+    # Logs an audit event related to a runner registration event
+    #
+    # @param [Ci::Runner] runner
+    # @param [String, User] author the entity initiating the operation (e.g. a runner registration or authentication token)
+    # @param [Group, Project, nil] token_scope the scopes that the operation applies to (nil represents the instance)
+    def initialize(runner, author, token_scope)
       @token_scope = token_scope
       @runner = runner
-      @action = action
 
       raise ArgumentError, 'Missing token_scope' if token_scope.nil? && !runner.instance_type?
 
@@ -13,12 +17,17 @@ module AuditEvents
         custom_message: message,
         target_id: runner.id,
         target_type: runner.class.name,
-        target_details: runner_path,
-        runner_registration_token: registration_token[0...8]
+        target_details: runner_path
       }
+
+      if author.is_a?(String)
+        author = author[0...8]
+        details[token_field] = author
+      end
+
       details[:errors] = @runner.errors.full_messages unless @runner.errors.empty?
 
-      super(details[:runner_registration_token], token_scope, details)
+      super(author, token_scope, details)
     end
 
     def track_event
@@ -28,22 +37,21 @@ module AuditEvents
       unauth_security_event
     end
 
+    private
+
     def message
-      runner_type = @runner.runner_type.chomp('_type')
-
-      case @action
-      when :register
-        if @runner.valid?
-          "Registered #{runner_type} CI runner"
-        else
-          "Failed to register #{runner_type} CI runner"
-        end
-      end
+      raise NotImplementedError, "Please implement #{self.class}##{__method__}"
     end
 
-    def runner_path
-      return unless @runner.persisted?
+    def author_class
+      raise NotImplementedError, "Please implement #{self.class}##{__method__}"
+    end
 
+    def runner_type
+      @runner.runner_type.chomp('_type')
+    end
+
+    def runner_path
       url_helpers = ::Gitlab::Routing.url_helpers
 
       if @runner.group_type?

ee/app/services/ee/ci/register_runner_service.rb

@@ -18,7 +18,7 @@ module EE
       private
 
       def audit_log_event(runner, registration_token)
-        ::AuditEvents::RunnerRegistrationAuditEventService.new(runner, registration_token, token_scope, :register)
+        ::AuditEvents::RegisterRunnerAuditEventService.new(runner, registration_token, token_scope)
           .track_event
       end
     end

ee/spec/presenters/audit_event_presenter_spec.rb

@@ -69,16 +69,9 @@ RSpec.describe AuditEventPresenter do
     end
 
     context 'event authored by a runner registration token user' do
-      let(:audit_event) { build(:audit_event, user: nil, details: details) }
       let(:author_double) { double(:author) }
-      let(:details) do
-        {
-          author_name: nil,
-          ip_address: '127.0.0.1',
-          target_details: 'target name',
-          entity_path: 'path',
-          runner_registration_token: 'abc123'
-        }
+      let(:audit_event) do
+        build(:audit_event, ip_address: '127.0.0.1', target_type: ::Ci::Runner.name, entity_path: 'path', details: { runner_registration_token: 'abc123' })
       end
 
       it "returns author's full_path" do

ee/spec/services/audit_events/runner_registration_audit_event_service_spec.rb → ee/spec/services/audit_events/register_runner_audit_event_service_spec.rb

@@ -2,9 +2,11 @@
 
 require 'spec_helper'
 
-RSpec.describe AuditEvents::RunnerRegistrationAuditEventService do
-  let(:registration_token) { 'b6bce79c3a' }
-  let(:service) { described_class.new(runner, registration_token, entity, action) }
+RSpec.describe AuditEvents::RegisterRunnerAuditEventService do
+  let_it_be(:user) { create(:user) }
+
+  let(:author) { 'b6bce79c3a' }
+  let(:service) { described_class.new(runner, author, entity) }
   let(:common_attrs) do
     {
       author_id: -1,
@@ -16,12 +18,19 @@ RSpec.describe AuditEvents::RunnerRegistrationAuditEventService do
       details: {
         target_type: runner.class.name,
         target_id: runner.id,
-        runner_registration_token: registration_token[0...8],
         ip_address: nil
       }
     }
   end
 
+  shared_examples 'expected audit log' do
+    it 'returns audit event attributes' do
+      travel_to(timestamp) do
+        expect(subject.attributes).to eq(attrs.stringify_keys)
+      end
+    end
+  end
+
   describe '#track_event' do
     before do
       stub_licensed_features(admin_audit_log: true)
@@ -37,167 +46,133 @@ RSpec.describe AuditEvents::RunnerRegistrationAuditEventService do
       end
 
       let(:entity) { }
-
-      context 'with action as :register' do
-        let(:action) { :register }
-        let(:extra_attrs) { {} }
-        let(:target_details) { }
-        let(:attrs) do
-          common_attrs.deep_merge(
-            author_name: nil,
-            entity_id: -1,
-            entity_type: 'User',
+      let(:extra_attrs) { {} }
+      let(:target_details) { }
+      let(:attrs) do
+        common_attrs.deep_merge(
+          author_name: nil,
+          entity_id: -1,
+          entity_type: 'User',
+          entity_path: nil,
+          target_details: target_details,
+          details: {
+            runner_registration_token: author[0...8],
             entity_path: nil,
-            target_details: target_details,
+            target_details: target_details
+          }
+        ).deep_merge(extra_attrs)
+      end
+
+      context 'on runner that failed to create' do
+        let(:runner) { build(:ci_runner) }
+        let(:extra_attrs) do
+          {
             details: {
-              custom_message: 'Registered instance CI runner',
-              entity_path: nil,
-              target_details: target_details
+              custom_message: 'Failed to register instance CI runner',
+              errors: ['Runner some error']
             }
-          ).deep_merge(extra_attrs)
+          }
         end
 
-        context 'on runner that failed to create' do
-          let(:runner) { build(:ci_runner) }
-          let(:extra_attrs) do
-            {
-              details: {
-                custom_message: 'Failed to register instance CI runner',
-                errors: ['Runner some error']
-              }
-            }
-          end
-
-          before do
-            allow(runner).to receive(:valid?) do
-              runner.errors.add :runner, 'some error'
-              false
-            end
-          end
-
-          it 'returns audit event attributes of a failed runner registration', :aggregate_failures do
-            travel_to(timestamp) do
-              expect(subject.attributes).to eq(attrs.stringify_keys)
-              expect(runner.persisted?).to be_falsey
-            end
+        before do
+          allow(runner).to receive(:valid?) do
+            runner.errors.add :runner, 'some error'
+            false
           end
         end
 
-        context 'on persisted runner' do
-          let(:runner) { create(:ci_runner) }
-          let(:target_details) { ::Gitlab::Routing.url_helpers.admin_runner_path(runner) }
-          let(:extra_attrs) do
-            { details: { custom_message: 'Registered instance CI runner' } }
-          end
-
-          it 'returns audit event attributes' do
-            travel_to(timestamp) do
-              expect(subject.attributes).to eq(attrs.stringify_keys)
-            end
+        it 'returns audit event attributes of a failed runner registration', :aggregate_failures do
+          travel_to(timestamp) do
+            expect(subject.attributes).to eq(attrs.stringify_keys)
+            expect(runner.persisted?).to be_falsey
           end
         end
       end
 
-      context 'with unknown action' do
+      context 'on persisted runner' do
         let(:runner) { create(:ci_runner) }
-        let(:action) { :unknown }
-
-        it 'is not logged' do
-          is_expected.to be_nil
+        let(:target_details) { ::Gitlab::Routing.url_helpers.admin_runner_path(runner) }
+        let(:extra_attrs) do
+          { details: { custom_message: 'Registered instance CI runner' } }
         end
+
+        it_behaves_like 'expected audit log'
       end
     end
 
     context 'for group runner' do
-      let(:entity) { create(:group) }
-
-      context 'with action as :register' do
-        let(:action) { :register }
-        let(:extra_attrs) { {} }
-        let(:target_details) { }
-        let(:attrs) do
-          common_attrs.deep_merge(
-            author_name: registration_token[0...8],
-            entity_id: entity.id,
-            entity_type: entity.class.name,
+      let_it_be(:entity) { create(:group) }
+
+      let(:extra_attrs) { {} }
+      let(:target_details) { }
+      let(:attrs) do
+        common_attrs.deep_merge(
+          author_name: author[0...8],
+          entity_id: entity.id,
+          entity_type: entity.class.name,
+          entity_path: entity.full_path,
+          target_details: target_details,
+          details: {
+            author_name: author[0...8],
+            runner_registration_token: author[0...8],
+            custom_message: 'Registered group CI runner',
             entity_path: entity.full_path,
-            target_details: target_details,
+            target_details: target_details
+          }
+        ).deep_merge(extra_attrs)
+      end
+
+      context 'on runner that failed to create' do
+        let(:runner) { build(:ci_runner, :group, groups: [entity]) }
+        let(:extra_attrs) do
+          {
             details: {
-              author_name: registration_token[0...8],
-              custom_message: 'Registered group CI runner',
-              entity_path: entity.full_path,
-              target_details: target_details
+              custom_message: 'Failed to register group CI runner',
+              errors: ['Runner some error']
             }
-          ).deep_merge(extra_attrs)
+          }
         end
 
-        context 'on runner that failed to create' do
-          let(:runner) { build(:ci_runner, :group, groups: [entity]) }
-          let(:extra_attrs) do
-            {
-              details: {
-                custom_message: 'Failed to register group CI runner',
-                errors: ['Runner some error']
-              }
-            }
-          end
-
-          before do
-            allow(runner).to receive(:valid?) do
-              runner.errors.add :runner, 'some error'
-              false
-            end
-          end
-
-          it 'returns audit event attributes of a failed runner registration', :aggregate_failures do
-            travel_to(timestamp) do
-              expect(subject.attributes).to eq(attrs.stringify_keys)
-              expect(runner.persisted?).to be_falsey
-            end
+        before do
+          allow(runner).to receive(:valid?) do
+            runner.errors.add :runner, 'some error'
+            false
           end
         end
 
-        context 'on persisted runner' do
-          let(:runner) { create(:ci_runner, :group, groups: [entity]) }
-          let(:target_details) { ::Gitlab::Routing.url_helpers.group_runner_path(entity, runner) }
-          let(:extra_attrs) do
-            { details: { custom_message: 'Registered group CI runner' } }
-          end
-
-          it 'returns audit event attributes' do
-            travel_to(timestamp) do
-              expect(subject.attributes).to eq(attrs.stringify_keys)
-            end
+        it 'returns audit event attributes of a failed runner registration', :aggregate_failures do
+          travel_to(timestamp) do
+            expect(subject.attributes).to eq(attrs.stringify_keys)
+            expect(runner.persisted?).to be_falsey
           end
         end
       end
 
-      context 'with unknown action' do
+      context 'on persisted runner' do
         let(:runner) { create(:ci_runner, :group, groups: [entity]) }
-        let(:action) { :unknown }
-
-        it 'is not logged' do
-          is_expected.to be_nil
+        let(:target_details) { ::Gitlab::Routing.url_helpers.group_runner_path(entity, runner) }
+        let(:extra_attrs) do
+          { details: { custom_message: 'Registered group CI runner' } }
         end
+
+        it_behaves_like 'expected audit log'
       end
-    end
 
-    context 'for project runner' do
-      let(:entity) { create(:project) }
+      context 'for project runner' do
+        let_it_be(:entity) { create(:project) }
 
-      context 'with action as :register' do
-        let(:action) { :register }
         let(:extra_attrs) { {} }
         let(:target_details) { }
         let(:attrs) do
           common_attrs.deep_merge(
-            author_name: registration_token[0...8],
+            author_name: author[0...8],
             entity_id: entity.id,
             entity_type: entity.class.name,
             entity_path: entity.full_path,
             target_details: target_details,
             details: {
-              author_name: registration_token[0...8],
+              author_name: author[0...8],
+              runner_registration_token: author[0...8],
               entity_path: entity.full_path,
               target_details: target_details
             }
@@ -237,11 +212,7 @@ RSpec.describe AuditEvents::RunnerRegistrationAuditEventService do
             { details: { custom_message: 'Registered project CI runner' } }
           end
 
-          it 'returns audit event attributes' do
-            travel_to(timestamp) do
-              expect(subject.attributes).to eq(attrs.stringify_keys)
-            end
-          end
+          it_behaves_like 'expected audit log'
         end
       end
     end

ee/spec/services/ci/register_runner_service_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe ::Ci::RegisterRunnerService, '#execute' do
   let(:registration_token) { 'abcdefg123456' }
   let(:token) { }
-  let(:audit_service) { instance_double(::AuditEvents::RunnerRegistrationAuditEventService) }
+  let(:audit_service) { instance_double(::AuditEvents::RegisterRunnerAuditEventService) }
 
   before do
     stub_feature_flags(runner_registration_control: false)
@@ -27,8 +27,8 @@ RSpec.describe ::Ci::RegisterRunnerService, '#execute' do
 
   shared_examples 'a service logging a runner registration audit event' do
     it 'returns newly-created Runner' do
-      expect(::AuditEvents::RunnerRegistrationAuditEventService).to receive(:new)
-        .with(last_ci_runner, token, token_scope, :register)
+      expect(::AuditEvents::RegisterRunnerAuditEventService).to receive(:new)
+        .with(last_ci_runner, token, token_scope)
         .once.and_return(audit_service)
 
       is_expected.to eq(::Ci::Runner.last)
@@ -37,8 +37,8 @@ RSpec.describe ::Ci::RegisterRunnerService, '#execute' do
 
   shared_examples 'a service logging a failed runner registration audit event' do
     before do
-      expect(::AuditEvents::RunnerRegistrationAuditEventService).to receive(:new)
-        .with(a_ci_runner_with_errors, token, token_scope, :register)
+      expect(::AuditEvents::RegisterRunnerAuditEventService).to receive(:new)
+        .with(a_ci_runner_with_errors, token, token_scope)
         .once.and_return(audit_service)
     end