From 2bd82a93d0654adcbd54c7bffcfe2ff169de9b7d Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Wed, 23 Feb 2022 19:12:01 +0100 Subject: [PATCH 1/2] Add audit logs when assigning CI runner to project Changelog: added EE: true --- .../ci/runners/assign_runner_service.rb | 4 ++ .../runner_audit_event_service.rb | 14 +++++++ .../runner_custom_audit_event_service.rb | 21 ++++++++++ .../ee/ci/runners/assign_runner_service.rb | 29 +++++++++++++ .../ci/runners/assign_runner_service_spec.rb | 41 +++++++++++++++++++ .../ci/runners/assign_runner_service_spec.rb | 4 +- 6 files changed, 111 insertions(+), 2 deletions(-) create mode 100644 ee/app/services/audit_events/runner_custom_audit_event_service.rb create mode 100644 ee/app/services/ee/ci/runners/assign_runner_service.rb create mode 100644 ee/spec/services/ci/runners/assign_runner_service_spec.rb diff --git a/app/services/ci/runners/assign_runner_service.rb b/app/services/ci/runners/assign_runner_service.rb index 0e16ac35231659..ba36c3ee457596 100644 --- a/app/services/ci/runners/assign_runner_service.rb +++ b/app/services/ci/runners/assign_runner_service.rb @@ -3,6 +3,8 @@ module Ci module Runners class AssignRunnerService + attr_reader :runner, :project, :user + # @param [Ci::Runner] runner the runner to assign to a project # @param [Project] project the new project to assign the runner to # @param [User] user the user performing the operation @@ -20,3 +22,5 @@ def execute end end end + +Ci::Runners::AssignRunnerService.prepend_mod diff --git a/ee/app/services/audit_events/runner_audit_event_service.rb b/ee/app/services/audit_events/runner_audit_event_service.rb index b913202e38dfe0..7d67092d32e4a0 100644 --- a/ee/app/services/audit_events/runner_audit_event_service.rb +++ b/ee/app/services/audit_events/runner_audit_event_service.rb @@ -2,6 +2,8 @@ module AuditEvents class RunnerAuditEventService < ::AuditEventService + extend ::Gitlab::Utils::Override + # Logs an audit event related to a runner event # # @param [Ci::Runner] runner @@ -39,6 +41,18 @@ def track_event private + override :base_payload + def base_payload + payload = super + + if @token_scope + payload.merge(details: { + entity_id: @token_scope.id, + entity_type: @token_scope.class.name + }) + end + end + def message raise NotImplementedError, "Please implement #{self.class}##{__method__}" end diff --git a/ee/app/services/audit_events/runner_custom_audit_event_service.rb b/ee/app/services/audit_events/runner_custom_audit_event_service.rb new file mode 100644 index 00000000000000..51e08b44fe46f4 --- /dev/null +++ b/ee/app/services/audit_events/runner_custom_audit_event_service.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module AuditEvents + class RunnerCustomAuditEventService < RunnerAuditEventService + # Logs an audit event with a custom message related to a runner 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) + # @param [String] custom_message the message describing the event + def initialize(runner, author, token_scope, custom_message) + @custom_message = custom_message + + super(runner, author, token_scope) + end + + def message + @custom_message + end + end +end diff --git a/ee/app/services/ee/ci/runners/assign_runner_service.rb b/ee/app/services/ee/ci/runners/assign_runner_service.rb new file mode 100644 index 00000000000000..085ffc13cc1a50 --- /dev/null +++ b/ee/app/services/ee/ci/runners/assign_runner_service.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module EE + module Ci + module Runners + module AssignRunnerService + extend ::Gitlab::Utils::Override + include ::Audit::Changes + + override :execute + def execute + result = super + + audit_log_event if result + + result + end + + private + + AUDIT_MESSAGE = 'Assigned CI runner to project' + + def audit_log_event + ::AuditEvents::RunnerCustomAuditEventService.new(runner, user, project, AUDIT_MESSAGE).track_event + end + end + end + end +end diff --git a/ee/spec/services/ci/runners/assign_runner_service_spec.rb b/ee/spec/services/ci/runners/assign_runner_service_spec.rb new file mode 100644 index 00000000000000..26991499586409 --- /dev/null +++ b/ee/spec/services/ci/runners/assign_runner_service_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::Runners::AssignRunnerService, '#execute' do + let_it_be(:owner_project) { create(:project) } + let_it_be(:new_project) { create(:project) } + let_it_be(:project_runner) { create(:ci_runner, :project, projects: [owner_project]) } + + let(:audit_service) { instance_double(::AuditEvents::RunnerCustomAuditEventService) } + + subject { described_class.new(project_runner, new_project, user).execute } + + context 'with unauthorized user' do + let(:user) { build(:user) } + + it 'does not call assign_to on runner and returns false', :aggregate_failures do + expect(project_runner).not_to receive(:assign_to) + expect(::AuditEvents::RunnerCustomAuditEventService).not_to receive(:new) + + is_expected.to be_falsey + end + end + + context 'with admin user', :enable_admin_mode do + let(:user) { create_default(:admin) } + + before do + expect(audit_service).to receive(:track_event).once.and_return('track_event_return_value') + end + + it 'calls track_event on RunnerCustomAuditEventService and returns assign_to return value', :aggregate_failures do + expect(project_runner).to receive(:assign_to).with(new_project, user).once.and_return('assign_to return value') + expect(::AuditEvents::RunnerCustomAuditEventService).to receive(:new) + .with(project_runner, user, new_project, 'Assigned CI runner to project') + .once.and_return(audit_service) + + is_expected.to eq('assign_to return value') + end + end +end diff --git a/spec/services/ci/runners/assign_runner_service_spec.rb b/spec/services/ci/runners/assign_runner_service_spec.rb index 2997dfd7027989..00b176bb759ec5 100644 --- a/spec/services/ci/runners/assign_runner_service_spec.rb +++ b/spec/services/ci/runners/assign_runner_service_spec.rb @@ -5,8 +5,8 @@ RSpec.describe ::Ci::Runners::AssignRunnerService, '#execute' do subject { described_class.new(runner, project, user).execute } - let_it_be(:runner) { build(:ci_runner) } - let_it_be(:project) { build(:project) } + let_it_be(:runner) { create(:ci_runner, :project, projects: [project]) } + let_it_be(:project) { create(:project) } context 'without user' do let(:user) { nil } -- GitLab From ef5677469df3f12e38cd699ebb003318e27f72a0 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Mon, 28 Feb 2022 15:58:46 +0100 Subject: [PATCH 2/2] Address MR review comments --- .../ci/runners/assign_runner_service.rb | 12 +++-- .../runner_audit_event_service.rb | 15 +----- .../runner_custom_audit_event_service.rb | 6 +-- ...egister_runner_audit_event_service_spec.rb | 4 ++ .../runner_custom_audit_event_service_spec.rb | 53 +++++++++++++++++++ ...egister_runner_audit_event_service_spec.rb | 2 + 6 files changed, 70 insertions(+), 22 deletions(-) create mode 100644 ee/spec/services/audit_events/runner_custom_audit_event_service_spec.rb diff --git a/app/services/ci/runners/assign_runner_service.rb b/app/services/ci/runners/assign_runner_service.rb index ba36c3ee457596..886cd3a4e44120 100644 --- a/app/services/ci/runners/assign_runner_service.rb +++ b/app/services/ci/runners/assign_runner_service.rb @@ -3,11 +3,9 @@ module Ci module Runners class AssignRunnerService - attr_reader :runner, :project, :user - - # @param [Ci::Runner] runner the runner to assign to a project - # @param [Project] project the new project to assign the runner to - # @param [User] user the user performing the operation + # @param [Ci::Runner] runner: the runner to assign to a project + # @param [Project] project: the new project to assign the runner to + # @param [User] user: the user performing the operation def initialize(runner, project, user) @runner = runner @project = project @@ -19,6 +17,10 @@ def execute @runner.assign_to(@project, @user) end + + private + + attr_reader :runner, :project, :user end end end diff --git a/ee/app/services/audit_events/runner_audit_event_service.rb b/ee/app/services/audit_events/runner_audit_event_service.rb index 7d67092d32e4a0..84d749b9560c3e 100644 --- a/ee/app/services/audit_events/runner_audit_event_service.rb +++ b/ee/app/services/audit_events/runner_audit_event_service.rb @@ -2,8 +2,6 @@ module AuditEvents class RunnerAuditEventService < ::AuditEventService - extend ::Gitlab::Utils::Override - # Logs an audit event related to a runner event # # @param [Ci::Runner] runner @@ -21,6 +19,7 @@ def initialize(runner, author, token_scope) target_type: runner.class.name, target_details: runner_path } + details.merge!(entity_id: @token_scope.id, entity_type: @token_scope.class.name) if @token_scope if author.is_a?(String) author = author[0...8] @@ -41,18 +40,6 @@ def track_event private - override :base_payload - def base_payload - payload = super - - if @token_scope - payload.merge(details: { - entity_id: @token_scope.id, - entity_type: @token_scope.class.name - }) - end - end - def message raise NotImplementedError, "Please implement #{self.class}##{__method__}" end diff --git a/ee/app/services/audit_events/runner_custom_audit_event_service.rb b/ee/app/services/audit_events/runner_custom_audit_event_service.rb index 51e08b44fe46f4..d3f86e9358e5e5 100644 --- a/ee/app/services/audit_events/runner_custom_audit_event_service.rb +++ b/ee/app/services/audit_events/runner_custom_audit_event_service.rb @@ -5,9 +5,9 @@ class RunnerCustomAuditEventService < RunnerAuditEventService # Logs an audit event with a custom message related to a runner 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) - # @param [String] custom_message the message describing the event + # @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) + # @param [String] custom_message: the message describing the event def initialize(runner, author, token_scope, custom_message) @custom_message = custom_message 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 index 7c49a9b1040cda..97fd0f2d85870a 100644 --- 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 @@ -116,6 +116,8 @@ author_name: author[0...8], runner_registration_token: author[0...8], custom_message: 'Registered group CI runner', + entity_id: entity.id, + entity_type: entity.class.name, entity_path: entity.full_path, target_details: target_details } @@ -173,6 +175,8 @@ details: { author_name: author[0...8], runner_registration_token: author[0...8], + entity_id: entity.id, + entity_type: entity.class.name, entity_path: entity.full_path, target_details: target_details } diff --git a/ee/spec/services/audit_events/runner_custom_audit_event_service_spec.rb b/ee/spec/services/audit_events/runner_custom_audit_event_service_spec.rb new file mode 100644 index 00000000000000..db3422021c6308 --- /dev/null +++ b/ee/spec/services/audit_events/runner_custom_audit_event_service_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AuditEvents::RunnerCustomAuditEventService do + describe '#security_event' do + let(:logger) { instance_double(Gitlab::AuditJsonLogger) } + + let(:user) { create(:user) } + let(:entity) { create(:project) } + let(:target_details) { ::Gitlab::Routing.url_helpers.project_runner_path(entity, runner) } + let(:target_id) { runner.id } + let(:target_type) { 'Ci::Runner' } + let(:entity_type) { 'Project' } + let(:runner) { create(:ci_runner, :project, projects: [entity]) } + let(:custom_message) { 'Custom Event' } + let(:service) { described_class.new(runner, user, entity, custom_message) } + + before do + stub_licensed_features(audit_events: true) + end + + it 'logs the event to file' do + expect(service).to receive(:file_logger).and_return(logger) + expect(logger).to receive(:info).with(author_id: user.id, + author_name: user.name, + entity_id: entity.id, + entity_type: entity_type, + custom_message: custom_message, + target_details: target_details, + target_id: target_id, + target_type: target_type) + + expect { service.security_event }.to change(AuditEvent, :count).by(1) + + security_event = AuditEvent.last + + expect(security_event.details).to eq( + author_name: user.name, + custom_message: custom_message, + entity_id: entity.id, + entity_type: entity_type, + target_details: target_details, + target_id: target_id, + target_type: target_type + ) + + expect(security_event.author_id).to eq(user.id) + expect(security_event.entity_id).to eq(entity.id) + expect(security_event.entity_type).to eq(entity_type) + end + end +end diff --git a/ee/spec/services/audit_events/unregister_runner_audit_event_service_spec.rb b/ee/spec/services/audit_events/unregister_runner_audit_event_service_spec.rb index 5ba80eb366c10e..d58667d5055f85 100644 --- a/ee/spec/services/audit_events/unregister_runner_audit_event_service_spec.rb +++ b/ee/spec/services/audit_events/unregister_runner_audit_event_service_spec.rb @@ -42,6 +42,8 @@ target_details: target_details, details: { custom_message: "Unregistered #{entity_class_name&.downcase || 'instance'} CI runner", + entity_id: entity&.id || -1, + entity_type: entity ? entity_class_name : 'User', entity_path: entity&.full_path, target_details: target_details } -- GitLab