From aa9612f1bb8ea820a7490e80290ce2bc77a05200 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Wed, 2 Feb 2022 17:06:12 +0100 Subject: [PATCH] Log CI runner unregistration audit events Changelog: added EE: true --- app/controllers/admin/runners_controller.rb | 2 +- app/controllers/groups/runners_controller.rb | 2 +- .../projects/runners_controller.rb | 2 +- app/graphql/mutations/ci/runner/delete.rb | 2 +- app/services/ci/unregister_runner_service.rb | 8 +- .../unregister_runner_audit_event_service.rb | 13 ++ .../ee/ci/unregister_runner_service.rb | 43 +++++ ...egister_runner_audit_event_service_spec.rb | 156 ++++++++++++++++++ .../ci/unregister_runner_service_spec.rb | 57 +++++++ lib/api/ci/runner.rb | 2 +- lib/api/ci/runners.rb | 2 +- .../admin/runners_controller_spec.rb | 2 +- .../groups/runners_controller_spec.rb | 2 +- .../projects/runners_controller_spec.rb | 2 +- .../mutations/ci/runner/delete_spec.rb | 6 +- spec/requests/api/ci/runners_spec.rb | 4 +- .../ci/unregister_runner_service_spec.rb | 2 +- 17 files changed, 290 insertions(+), 17 deletions(-) create mode 100644 ee/app/services/audit_events/unregister_runner_audit_event_service.rb create mode 100644 ee/app/services/ee/ci/unregister_runner_service.rb create mode 100644 ee/spec/services/audit_events/unregister_runner_audit_event_service_spec.rb create mode 100644 ee/spec/services/ci/unregister_runner_service_spec.rb diff --git a/app/controllers/admin/runners_controller.rb b/app/controllers/admin/runners_controller.rb index f7f78ab3229fb6..bdfeb131cb0930 100644 --- a/app/controllers/admin/runners_controller.rb +++ b/app/controllers/admin/runners_controller.rb @@ -34,7 +34,7 @@ def update end def destroy - Ci::UnregisterRunnerService.new(@runner).execute + Ci::UnregisterRunnerService.new(@runner, current_user).execute redirect_to admin_runners_path, status: :found end diff --git a/app/controllers/groups/runners_controller.rb b/app/controllers/groups/runners_controller.rb index b194aeff80daf4..ed5b4733cb01b9 100644 --- a/app/controllers/groups/runners_controller.rb +++ b/app/controllers/groups/runners_controller.rb @@ -35,7 +35,7 @@ def destroy if @runner.belongs_to_more_than_one_project? redirect_to group_settings_ci_cd_path(@group, anchor: 'runners-settings'), status: :found, alert: _('Runner was not deleted because it is assigned to multiple projects.') else - Ci::UnregisterRunnerService.new(@runner).execute + Ci::UnregisterRunnerService.new(@runner, current_user).execute redirect_to group_settings_ci_cd_path(@group, anchor: 'runners-settings'), status: :found end diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index 192a29730d9261..1c441fc858cfae 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -23,7 +23,7 @@ def update def destroy if @runner.only_for?(project) - Ci::UnregisterRunnerService.new(@runner).execute + Ci::UnregisterRunnerService.new(@runner, current_user).execute end redirect_to project_runners_path(@project), status: :found diff --git a/app/graphql/mutations/ci/runner/delete.rb b/app/graphql/mutations/ci/runner/delete.rb index 21c3d55881c564..19d18845e4c3e0 100644 --- a/app/graphql/mutations/ci/runner/delete.rb +++ b/app/graphql/mutations/ci/runner/delete.rb @@ -20,7 +20,7 @@ def resolve(id:, **runner_attrs) error = authenticate_delete_runner!(runner) return { errors: [error] } if error - ::Ci::UnregisterRunnerService.new(runner).execute + ::Ci::UnregisterRunnerService.new(runner, current_user).execute { errors: runner.errors.full_messages } end diff --git a/app/services/ci/unregister_runner_service.rb b/app/services/ci/unregister_runner_service.rb index 97d9852b7eda0c..e7addb30b02f36 100644 --- a/app/services/ci/unregister_runner_service.rb +++ b/app/services/ci/unregister_runner_service.rb @@ -2,11 +2,13 @@ module Ci class UnregisterRunnerService - attr_reader :runner + attr_reader :runner, :author # @param [Ci::Runner] runner the runner to unregister/destroy - def initialize(runner) + # @param [User, authentication token String] author the user or the authentication token that authorizes the removal + def initialize(runner, author) @runner = runner + @author = author end def execute @@ -14,3 +16,5 @@ def execute end end end + +Ci::UnregisterRunnerService.prepend_mod diff --git a/ee/app/services/audit_events/unregister_runner_audit_event_service.rb b/ee/app/services/audit_events/unregister_runner_audit_event_service.rb new file mode 100644 index 00000000000000..e916cd0737540a --- /dev/null +++ b/ee/app/services/audit_events/unregister_runner_audit_event_service.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module AuditEvents + class UnregisterRunnerAuditEventService < RunnerRegistrationAuditEventService + def token_field + :runner_authentication_token + end + + def message + "Unregistered #{runner_type} CI runner" + end + end +end diff --git a/ee/app/services/ee/ci/unregister_runner_service.rb b/ee/app/services/ee/ci/unregister_runner_service.rb new file mode 100644 index 00000000000000..62bde343f9fbbf --- /dev/null +++ b/ee/app/services/ee/ci/unregister_runner_service.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module EE + module Ci + # Unregisters a CI Runner and logs an audit event + # + module UnregisterRunnerService + extend ::Gitlab::Utils::Override + include ::Audit::Changes + + override :execute + def execute + scopes = runner_scopes # Save the scopes before destroying the record + + result = super + + audit_log_event(scopes) + + result + end + + private + + def runner_scopes + case runner.runner_type + when 'instance_type' + [nil] + when 'group_type' + runner.groups.to_a + when 'project_type' + runner.projects.to_a + end + end + + def audit_log_event(scopes) + scopes.each do |scope| + ::AuditEvents::UnregisterRunnerAuditEventService.new(runner, author, scope) + .track_event + end + end + 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 new file mode 100644 index 00000000000000..5ba80eb366c10e --- /dev/null +++ b/ee/spec/services/audit_events/unregister_runner_audit_event_service_spec.rb @@ -0,0 +1,156 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AuditEvents::UnregisterRunnerAuditEventService do + let_it_be(:user) { create(:user) } + + 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 + + shared_context 'when unregistering runner' do + let(:extra_attrs) { {} } + let(:attrs) do + entity_class_name = entity.class.name if entity + + common_attrs.deep_merge( + entity_id: entity&.id || -1, + entity_type: entity ? entity_class_name : 'User', + entity_path: entity&.full_path, + target_details: target_details, + details: { + custom_message: "Unregistered #{entity_class_name&.downcase || 'instance'} CI runner", + entity_path: entity&.full_path, + target_details: target_details + } + ).deep_merge(extra_attrs) + end + + context 'with authentication token author' do + let(:author) { 'b6bce79c3a' } + let(:extra_attrs) do + { + author_name: author[0...8], + details: { + author_name: author[0...8], + runner_authentication_token: author[0...8] + } + } + end + + it_behaves_like 'expected audit log' + end + + context 'with User author' do + let(:author) { user } + let(:extra_attrs) do + { + author_id: author.id, + author_name: author.name, + details: { author_name: author.name } + } + end + + it_behaves_like 'expected audit log' + 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_it_be(:runner) { create(:ci_runner) } + + let(:entity) { } + let(:extra_attrs) { {} } + let(:target_details) { ::Gitlab::Routing.url_helpers.admin_runner_path(runner) } + 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: 'Unregistered instance CI runner', + entity_path: nil, + target_details: target_details + } + ).deep_merge(extra_attrs) + end + + context 'with authentication token author' do + let(:author) { 'b6bce79c3a' } + let(:extra_attrs) do + { + details: { runner_authentication_token: author[0...8] } + } + end + + it_behaves_like 'expected audit log' + end + + context 'with User author' do + let(:author) { user } + + let(:extra_attrs) do + { author_id: author.id } + end + + it_behaves_like 'expected audit log' + end + end + + context 'for group runner' do + let_it_be(:entity) { create(:group) } + let_it_be(:runner) { create(:ci_runner, :group, groups: [entity]) } + + include_context 'when unregistering runner' do + let(:extra_attrs) { {} } + let(:target_details) { ::Gitlab::Routing.url_helpers.group_runner_path(entity, runner) } + end + end + + context 'for project runner' do + let_it_be(:entity) { create(:project) } + let_it_be(:runner) { create(:ci_runner, :project, projects: [entity]) } + + include_context 'when unregistering runner' do + let(:extra_attrs) { {} } + let(:target_details) { ::Gitlab::Routing.url_helpers.project_runner_path(entity, runner) } + end + end + end +end diff --git a/ee/spec/services/ci/unregister_runner_service_spec.rb b/ee/spec/services/ci/unregister_runner_service_spec.rb new file mode 100644 index 00000000000000..fa361615032e4a --- /dev/null +++ b/ee/spec/services/ci/unregister_runner_service_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::UnregisterRunnerService, '#execute' do + let(:audit_service) { instance_double(::AuditEvents::UnregisterRunnerAuditEventService) } + let(:current_user) { nil } + let(:token) { 'abc123' } + + subject { described_class.new(runner, current_user || token).execute } + + context 'on an instance runner' do + let(:runner) { create(:ci_runner) } + + it 'logs an audit event with the instance scope' do + expect(audit_service).to receive(:track_event).once.and_return('track_event_return_value') + expect(::AuditEvents::UnregisterRunnerAuditEventService).to receive(:new) + .with(runner, token, nil) + .once.and_return(audit_service) + + subject + end + end + + context 'on a group runner' do + let(:group) { create(:group) } + let(:runner) { create(:ci_runner, :group, groups: [group]) } + let(:current_user) { build(:user) } + + it 'logs an audit event with the group scope' do + expect(audit_service).to receive(:track_event).once.and_return('track_event_return_value') + expect(::AuditEvents::UnregisterRunnerAuditEventService).to receive(:new) + .with(runner, current_user, group) + .once.and_return(audit_service) + + subject + end + end + + context 'on a project runner' do + let(:project1) { create(:project) } + let(:project2) { create(:project) } + let(:runner) { create(:ci_runner, :project, projects: [project1, project2]) } + + it 'logs an audit event per project' do + expect(audit_service).to receive(:track_event).twice.and_return('track_event_return_value') + expect(::AuditEvents::UnregisterRunnerAuditEventService).to receive(:new) + .with(runner, token, project1) + .once.and_return(audit_service) + expect(::AuditEvents::UnregisterRunnerAuditEventService).to receive(:new) + .with(runner, token, project2) + .once.and_return(audit_service) + + subject + end + end +end diff --git a/lib/api/ci/runner.rb b/lib/api/ci/runner.rb index 4df9600322c4af..95ff000d644db4 100644 --- a/lib/api/ci/runner.rb +++ b/lib/api/ci/runner.rb @@ -57,7 +57,7 @@ class Runner < ::API::Base delete '/', feature_category: :runner do authenticate_runner! - destroy_conditionally!(current_runner) { ::Ci::UnregisterRunnerService.new(current_runner).execute } + destroy_conditionally!(current_runner) { ::Ci::UnregisterRunnerService.new(current_runner, params[:token]).execute } end desc 'Validates authentication credentials' do diff --git a/lib/api/ci/runners.rb b/lib/api/ci/runners.rb index 8a7ffab97dda92..8c91c1ca91610c 100644 --- a/lib/api/ci/runners.rb +++ b/lib/api/ci/runners.rb @@ -110,7 +110,7 @@ class Runners < ::API::Base authenticate_delete_runner!(runner) - destroy_conditionally!(runner) { ::Ci::UnregisterRunnerService.new(runner).execute } + destroy_conditionally!(runner) { ::Ci::UnregisterRunnerService.new(runner, current_user).execute } end desc 'List jobs running on a runner' do diff --git a/spec/controllers/admin/runners_controller_spec.rb b/spec/controllers/admin/runners_controller_spec.rb index 74f352e8ec24d1..eb22e688bc2b6c 100644 --- a/spec/controllers/admin/runners_controller_spec.rb +++ b/spec/controllers/admin/runners_controller_spec.rb @@ -105,7 +105,7 @@ describe '#destroy' do it 'destroys the runner' do - expect_next_instance_of(Ci::UnregisterRunnerService, runner) do |service| + expect_next_instance_of(Ci::UnregisterRunnerService, runner, user) do |service| expect(service).to receive(:execute).once.and_call_original end diff --git a/spec/controllers/groups/runners_controller_spec.rb b/spec/controllers/groups/runners_controller_spec.rb index 9f0615a96ae0e1..bcba4df6b99f87 100644 --- a/spec/controllers/groups/runners_controller_spec.rb +++ b/spec/controllers/groups/runners_controller_spec.rb @@ -190,7 +190,7 @@ end it 'destroys the runner and redirects' do - expect_next_instance_of(Ci::UnregisterRunnerService, runner) do |service| + expect_next_instance_of(Ci::UnregisterRunnerService, runner, user) do |service| expect(service).to receive(:execute).once.and_call_original end diff --git a/spec/controllers/projects/runners_controller_spec.rb b/spec/controllers/projects/runners_controller_spec.rb index 246a37129d71f4..227aee8bbf7adc 100644 --- a/spec/controllers/projects/runners_controller_spec.rb +++ b/spec/controllers/projects/runners_controller_spec.rb @@ -37,7 +37,7 @@ describe '#destroy' do it 'destroys the runner' do - expect_next_instance_of(Ci::UnregisterRunnerService, runner) do |service| + expect_next_instance_of(Ci::UnregisterRunnerService, runner, user) do |service| expect(service).to receive(:execute).once.and_call_original end diff --git a/spec/graphql/mutations/ci/runner/delete_spec.rb b/spec/graphql/mutations/ci/runner/delete_spec.rb index b53ee30f826ea9..7e7308d3e271f7 100644 --- a/spec/graphql/mutations/ci/runner/delete_spec.rb +++ b/spec/graphql/mutations/ci/runner/delete_spec.rb @@ -55,7 +55,7 @@ it 'deletes runner' do mutation_params[:id] = project_runner.to_global_id - expect_next_instance_of(::Ci::UnregisterRunnerService, project_runner) do |service| + expect_next_instance_of(::Ci::UnregisterRunnerService, project_runner, current_ctx[:current_user]) do |service| expect(service).to receive(:execute).once.and_call_original end @@ -76,7 +76,7 @@ mutation_params[:id] = two_projects_runner.to_global_id allow_next_instance_of(::Ci::UnregisterRunnerService) do |service| - expect(service).not_to receive(:execute).once + expect(service).not_to receive(:execute) end expect { subject }.not_to change { Ci::Runner.count } expect(subject[:errors]).to contain_exactly("Runner #{two_projects_runner.to_global_id} associated with more than one project") @@ -89,7 +89,7 @@ let(:current_ctx) { { current_user: admin_user } } it 'deletes runner' do - expect_next_instance_of(::Ci::UnregisterRunnerService, runner) do |service| + expect_next_instance_of(::Ci::UnregisterRunnerService, runner, current_ctx[:current_user]) do |service| expect(service).to receive(:execute).once.and_call_original end diff --git a/spec/requests/api/ci/runners_spec.rb b/spec/requests/api/ci/runners_spec.rb index 336ce70d8d23ff..4f4f1cf9a8a37e 100644 --- a/spec/requests/api/ci/runners_spec.rb +++ b/spec/requests/api/ci/runners_spec.rb @@ -530,7 +530,7 @@ def update_runner(id, user, args) context 'admin user' do context 'when runner is shared' do it 'deletes runner' do - expect_next_instance_of(Ci::UnregisterRunnerService, shared_runner) do |service| + expect_next_instance_of(Ci::UnregisterRunnerService, shared_runner, admin) do |service| expect(service).to receive(:execute).once.and_call_original end @@ -548,7 +548,7 @@ def update_runner(id, user, args) context 'when runner is not shared' do it 'deletes used project runner' do - expect_next_instance_of(Ci::UnregisterRunnerService, project_runner) do |service| + expect_next_instance_of(Ci::UnregisterRunnerService, project_runner, admin) do |service| expect(service).to receive(:execute).once.and_call_original end diff --git a/spec/services/ci/unregister_runner_service_spec.rb b/spec/services/ci/unregister_runner_service_spec.rb index f427e04f2282e4..1e8016cb04ff66 100644 --- a/spec/services/ci/unregister_runner_service_spec.rb +++ b/spec/services/ci/unregister_runner_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe ::Ci::UnregisterRunnerService, '#execute' do - subject { described_class.new(runner).execute } + subject { described_class.new(runner, 'some_token').execute } let(:runner) { create(:ci_runner) } -- GitLab