From f3c4427e8990507c48ed7ff61f6a8c851521c8a0 Mon Sep 17 00:00:00 2001
From: Pedro Pombeiro <noreply@pedro.pombei.ro>
Date: Mon, 14 Feb 2022 14:11:05 +0100
Subject: [PATCH] Refactor runner registration audit classes

as extensible classes
---
 .../register_runner_audit_event_service.rb    |  19 ++
 ...runner_registration_audit_event_service.rb |  42 +--
 .../services/ee/ci/register_runner_service.rb |   2 +-
 .../presenters/audit_event_presenter_spec.rb  |  11 +-
 ...egister_runner_audit_event_service_spec.rb | 220 ++++++++++++++++
 ...r_registration_audit_event_service_spec.rb | 249 ------------------
 .../ci/register_runner_service_spec.rb        |  10 +-
 7 files changed, 272 insertions(+), 281 deletions(-)
 create mode 100644 ee/app/services/audit_events/register_runner_audit_event_service.rb
 create mode 100644 ee/spec/services/audit_events/register_runner_audit_event_service_spec.rb
 delete mode 100644 ee/spec/services/audit_events/runner_registration_audit_event_service_spec.rb

diff --git a/ee/app/services/audit_events/register_runner_audit_event_service.rb b/ee/app/services/audit_events/register_runner_audit_event_service.rb
new file mode 100644
index 00000000000000..373f85659f8788
--- /dev/null
+++ b/ee/app/services/audit_events/register_runner_audit_event_service.rb
@@ -0,0 +1,19 @@
+# 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
diff --git a/ee/app/services/audit_events/runner_registration_audit_event_service.rb b/ee/app/services/audit_events/runner_registration_audit_event_service.rb
index 513d60f3db3af1..24e2e573b705d0 100644
--- a/ee/app/services/audit_events/runner_registration_audit_event_service.rb
+++ b/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 @@ def initialize(runner, registration_token, token_scope, action)
         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 @@ def track_event
       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?
diff --git a/ee/app/services/ee/ci/register_runner_service.rb b/ee/app/services/ee/ci/register_runner_service.rb
index a970bfd7948f60..0dfe339b137e7e 100644
--- a/ee/app/services/ee/ci/register_runner_service.rb
+++ b/ee/app/services/ee/ci/register_runner_service.rb
@@ -18,7 +18,7 @@ def execute(registration_token, attributes)
       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
diff --git a/ee/spec/presenters/audit_event_presenter_spec.rb b/ee/spec/presenters/audit_event_presenter_spec.rb
index b393b0ca94ff0f..658f2422584a54 100644
--- a/ee/spec/presenters/audit_event_presenter_spec.rb
+++ b/ee/spec/presenters/audit_event_presenter_spec.rb
@@ -69,16 +69,9 @@
     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
diff --git a/ee/spec/services/audit_events/register_runner_audit_event_service_spec.rb b/ee/spec/services/audit_events/register_runner_audit_event_service_spec.rb
new file mode 100644
index 00000000000000..7c49a9b1040cda
--- /dev/null
+++ b/ee/spec/services/audit_events/register_runner_audit_event_service_spec.rb
@@ -0,0 +1,220 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+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,
+      created_at: timestamp,
+      id: subject.id,
+      target_type: runner.class.name,
+      target_id: runner.id,
+      ip_address: nil,
+      details: {
+        target_type: runner.class.name,
+        target_id: runner.id,
+        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)
+    end
+
+    subject { service.track_event }
+
+    let(:timestamp) { Time.zone.local(2021, 12, 28) }
+
+    context 'for instance runner' do
+      before do
+        stub_licensed_features(extended_audit_events: true, admin_audit_log: true)
+      end
+
+      let(:entity) { }
+      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
+          }
+        ).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
+        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_behaves_like 'expected audit log'
+      end
+    end
+
+    context 'for group runner' do
+      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
+          }
+        ).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
+        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_behaves_like 'expected audit log'
+      end
+
+      context 'for project runner' do
+        let_it_be(:entity) { create(:project) }
+
+        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],
+              entity_path: entity.full_path,
+              target_details: target_details
+            }
+          ).deep_merge(extra_attrs)
+        end
+
+        context 'on runner that failed to create' do
+          let(:runner) { build(:ci_runner, :project, projects: [entity]) }
+          let(:extra_attrs) do
+            {
+              details: {
+                custom_message: 'Failed to register project 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
+          end
+        end
+
+        context 'on persisted runner' do
+          let(:runner) { create(:ci_runner, :project, projects: [entity]) }
+          let(:target_details) { ::Gitlab::Routing.url_helpers.project_runner_path(entity, runner) }
+          let(:extra_attrs) do
+            { details: { custom_message: 'Registered project CI runner' } }
+          end
+
+          it_behaves_like 'expected audit log'
+        end
+      end
+    end
+  end
+end
diff --git a/ee/spec/services/audit_events/runner_registration_audit_event_service_spec.rb b/ee/spec/services/audit_events/runner_registration_audit_event_service_spec.rb
deleted file mode 100644
index 7c9390c79a8310..00000000000000
--- a/ee/spec/services/audit_events/runner_registration_audit_event_service_spec.rb
+++ /dev/null
@@ -1,249 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe AuditEvents::RunnerRegistrationAuditEventService do
-  let(:registration_token) { 'b6bce79c3a' }
-  let(:service) { described_class.new(runner, registration_token, entity, action) }
-  let(:common_attrs) do
-    {
-      author_id: -1,
-      created_at: timestamp,
-      id: subject.id,
-      target_type: runner.class.name,
-      target_id: runner.id,
-      ip_address: nil,
-      details: {
-        target_type: runner.class.name,
-        target_id: runner.id,
-        runner_registration_token: registration_token[0...8],
-        ip_address: nil
-      }
-    }
-  end
-
-  describe '#track_event' do
-    before do
-      stub_licensed_features(admin_audit_log: true)
-    end
-
-    subject { service.track_event }
-
-    let(:timestamp) { Time.zone.local(2021, 12, 28) }
-
-    context 'for instance runner' do
-      before do
-        stub_licensed_features(extended_audit_events: true, admin_audit_log: true)
-      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',
-            entity_path: nil,
-            target_details: target_details,
-            details: {
-              custom_message: 'Registered instance CI runner',
-              entity_path: nil,
-              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: '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
-          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
-          end
-        end
-      end
-
-      context 'with unknown action' do
-        let(:runner) { create(:ci_runner) }
-        let(:action) { :unknown }
-
-        it 'is not logged' do
-          is_expected.to be_nil
-        end
-      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,
-            entity_path: entity.full_path,
-            target_details: target_details,
-            details: {
-              author_name: registration_token[0...8],
-              custom_message: 'Registered group CI runner',
-              entity_path: entity.full_path,
-              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: {
-                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
-          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
-          end
-        end
-      end
-
-      context 'with unknown action' do
-        let(:runner) { create(:ci_runner, :group, groups: [entity]) }
-        let(:action) { :unknown }
-
-        it 'is not logged' do
-          is_expected.to be_nil
-        end
-      end
-    end
-
-    context 'for project runner' do
-      let(: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],
-            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],
-              entity_path: entity.full_path,
-              target_details: target_details
-            }
-          ).deep_merge(extra_attrs)
-        end
-
-        context 'on runner that failed to create' do
-          let(:runner) { build(:ci_runner, :project, projects: [entity]) }
-          let(:extra_attrs) do
-            {
-              details: {
-                custom_message: 'Failed to register project 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
-          end
-        end
-
-        context 'on persisted runner' do
-          let(:runner) { create(:ci_runner, :project, projects: [entity]) }
-          let(:target_details) { ::Gitlab::Routing.url_helpers.project_runner_path(entity, runner) }
-          let(:extra_attrs) 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
-        end
-      end
-    end
-  end
-end
diff --git a/ee/spec/services/ci/register_runner_service_spec.rb b/ee/spec/services/ci/register_runner_service_spec.rb
index 3b25f0b08eeb91..971f6cf8f709d8 100644
--- a/ee/spec/services/ci/register_runner_service_spec.rb
+++ b/ee/spec/services/ci/register_runner_service_spec.rb
@@ -5,7 +5,7 @@
 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 @@
 
   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 @@
 
   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
 
-- 
GitLab