From c47788fefd821eaedc777a2f372740ea529a943c Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Thu, 8 Sep 2022 19:50:52 +0200 Subject: [PATCH 1/5] Implement SetRunnerAssociatedProjectsService --- app/models/ci/runner.rb | 6 ++ .../set_runner_associated_projects_service.rb | 57 +++++++++++++++ ...runner_associated_projects_service_spec.rb | 73 +++++++++++++++++++ 3 files changed, 136 insertions(+) create mode 100644 app/services/ci/runners/set_runner_associated_projects_service.rb create mode 100644 spec/services/ci/runners/set_runner_associated_projects_service_spec.rb diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 60a9eade5a5416..28d9edcc135746 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -348,6 +348,12 @@ def deprecated_rest_status end end + def owner_project + return unless project_type? + + runner_projects.order(:id).first.project + end + def belongs_to_one_project? runner_projects.count == 1 end diff --git a/app/services/ci/runners/set_runner_associated_projects_service.rb b/app/services/ci/runners/set_runner_associated_projects_service.rb new file mode 100644 index 00000000000000..e8b35b7f04b368 --- /dev/null +++ b/app/services/ci/runners/set_runner_associated_projects_service.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +module Ci + module Runners + class SetRunnerAssociatedProjectsService + # @param [Ci::Runner] runner: the project runner to assign/unassign projects from + # @param [Array] project_ids: the IDs of the associated projects to assign the runner to + # @param [User] user: the user performing the operation + def initialize(runner, project_ids, user) + @runner = runner + @project_ids = project_ids + @user = user + end + + def execute + unless user.present? && user.can?(:assign_runner, runner) + return ServiceResponse.error(message: 'user not allowed to assign runner', http_status: :forbidden) + end + + set_associated_projects + end + + private + + def set_associated_projects + new_project_ids = [runner.owner_project.id] + project_ids + + response = ServiceResponse.success + runner.transaction do + # rubocop:disable CodeReuse/ActiveRecord + current_project_ids = runner.projects.ids + + new_projects = Project.where(id: project_ids - current_project_ids) + new_projects.each do |project| + next if runner.assign_to(project, user) + + response = ServiceResponse.error(message: 'failed to assign projects to runner') + raise ActiveRecord::Rollback, response.errors + end + + discarded_runner_projects = Ci::RunnerProject.where(project_id: current_project_ids - new_project_ids) + # rubocop:enable CodeReuse/ActiveRecord + discarded_runner_projects.each do |runner_project| + next if runner_project.destroy + + response = ServiceResponse.error(message: 'failed to destroy runner project') + raise ActiveRecord::Rollback, response.errors + end + end + + response + end + + attr_reader :runner, :project_ids, :user + end + end +end diff --git a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb new file mode 100644 index 00000000000000..d7377ef4a7b4d0 --- /dev/null +++ b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute' do + subject(:execute) { described_class.new(runner, project_ids, user).execute } + + let_it_be(:owner_project) { create(:project) } + let_it_be(:project2) { create(:project) } + let_it_be(:original_projects) { [owner_project, project2] } + let_it_be(:runner) { create(:ci_runner, :project, projects: original_projects) } + + context 'without user' do + let(:user) { nil } + let(:project_ids) { [project2.id] } + + it 'does not call assign_to on runner and returns error response', :aggregate_failures do + expect(runner).not_to receive(:assign_to) + + expect(execute).to be_error + expect(execute.message).to eq('user not allowed to assign runner') + end + end + + context 'with unauthorized user' do + let(:user) { build(:user) } + let(:project_ids) { [project2.id] } + + it 'does not call assign_to on runner and returns error message' do + expect(runner).not_to receive(:assign_to) + + expect(execute).to be_error + expect(execute.message).to eq('user not allowed to assign runner') + end + end + + context 'with admin user', :enable_admin_mode do + let(:user) { create_default(:user, :admin) } + let(:project_ids) { [project3.id, project4.id] } + + let(:project3) { create(:project) } + let(:project4) { create(:project) } + + context 'with successful requests' do + it 'calls assign_to on runner and returns success response' do + expect(runner).to receive(:assign_to).with(project3, user).once.ordered.and_call_original + expect(runner).to receive(:assign_to).with(project4, user).once.ordered.and_call_original + + expect(execute).to be_success + expect(runner.reload.projects.ids).to match_array([owner_project.id] + project_ids) + end + end + + context 'with failing assign_to requests' do + it 'returns error response and rolls back transaction' do + expect(runner).to receive(:assign_to).with(project3, user).once.ordered.and_call_original + expect(runner).to receive(:assign_to).with(project4, user).once.ordered.and_return(false) + + expect(execute).to be_error + expect(runner.reload.projects.ids).to match_array(original_projects.map(&:id)) + end + end + + context 'with failing destroy calls' do + it 'returns error response and rolls back transaction' do + expect_any_instance_of(Ci::RunnerProject).to receive(:destroy).and_return(false) # rubocop:disable RSpec/AnyInstanceOf + + expect(execute).to be_error + expect(runner.reload.projects.ids).to match_array(original_projects.map(&:id)) + end + end + end +end -- GitLab From 05ea6abff97365fc0b8a4b1a10b499b4a8dbf9d3 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Mon, 12 Sep 2022 11:48:58 +0200 Subject: [PATCH 2/5] Add auditing --- .../set_runner_associated_projects_service.rb | 2 + .../set_runner_associated_projects_service.rb | 31 +++++++++++ .../types/set_runner_associated_projects.yaml | 8 +++ ...runner_associated_projects_service_spec.rb | 51 +++++++++++++++++++ 4 files changed, 92 insertions(+) create mode 100644 ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb create mode 100644 ee/config/audit_events/types/set_runner_associated_projects.yaml create mode 100644 ee/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb diff --git a/app/services/ci/runners/set_runner_associated_projects_service.rb b/app/services/ci/runners/set_runner_associated_projects_service.rb index e8b35b7f04b368..c3461653537b04 100644 --- a/app/services/ci/runners/set_runner_associated_projects_service.rb +++ b/app/services/ci/runners/set_runner_associated_projects_service.rb @@ -55,3 +55,5 @@ def set_associated_projects end end end + +Ci::Runners::SetRunnerAssociatedProjectsService.prepend_mod diff --git a/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb b/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb new file mode 100644 index 00000000000000..f5c2a434b2d952 --- /dev/null +++ b/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module EE + module Ci + module Runners + module SetRunnerAssociatedProjectsService + extend ::Gitlab::Utils::Override + + override :execute + def execute + response = super + return response if response.error? + + audit_context = { + name: 'set_runner_associated_projects', + author: user, + scope: user, + target: runner, + message: 'Changed CI runner project assignments', + additional_details: { + action: :custom + } + } + ::Gitlab::Audit::Auditor.audit(audit_context) + + response + end + end + end + end +end diff --git a/ee/config/audit_events/types/set_runner_associated_projects.yaml b/ee/config/audit_events/types/set_runner_associated_projects.yaml new file mode 100644 index 00000000000000..8d889d95e06b99 --- /dev/null +++ b/ee/config/audit_events/types/set_runner_associated_projects.yaml @@ -0,0 +1,8 @@ +name: set_runner_associated_projects +description: Event triggered on successful assignment of associated projects to a CI runner +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/359958 +introduced_by_mr: +group: "group::runner" +milestone: 15.4 +saved_to_database: false +streamed: true diff --git a/ee/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb b/ee/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb new file mode 100644 index 00000000000000..9f104b9084d4ec --- /dev/null +++ b/ee/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#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]) } + + subject(:execute) { described_class.new(project_runner, [new_project.id], user).execute } + + context 'with unauthorized user' do + let(:user) { build(:user) } + + it 'does not call assign_to on runner and returns error response', :aggregate_failures do + expect(project_runner).not_to receive(:assign_to) + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + + expect(execute).to be_error + expect(execute.http_status).to eq :forbidden + end + end + + context 'with admin user', :enable_admin_mode do + let(:user) { create_default(:user, :admin) } + + context 'with assign_to returning true' do + it 'calls audit on Auditor and returns success response', :aggregate_failures do + expect(project_runner).to receive(:assign_to).with(new_project, user).once.and_return(true) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + a_hash_including( + name: 'set_runner_associated_projects', + author: user, + scope: user, + target: project_runner + )) + + expect(execute).to be_success + end + end + + context 'with assign_to returning false' do + it 'does not call audit on Auditor and returns error response', :aggregate_failures do + expect(project_runner).to receive(:assign_to).with(new_project, user).once.and_return(false) + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + + expect(execute).to be_error + end + end + end +end -- GitLab From f3b6300cef80444b4f7f5b659885ab2648476e2e Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Mon, 12 Sep 2022 19:12:24 +0200 Subject: [PATCH 3/5] Address MR review comments --- .../set_runner_associated_projects_service.rb | 44 +++++++++++++------ .../set_runner_associated_projects_service.rb | 4 +- ...runner_associated_projects_service_spec.rb | 6 ++- ...runner_associated_projects_service_spec.rb | 17 +++---- 4 files changed, 43 insertions(+), 28 deletions(-) diff --git a/app/services/ci/runners/set_runner_associated_projects_service.rb b/app/services/ci/runners/set_runner_associated_projects_service.rb index c3461653537b04..571b939caf1814 100644 --- a/app/services/ci/runners/set_runner_associated_projects_service.rb +++ b/app/services/ci/runners/set_runner_associated_projects_service.rb @@ -4,16 +4,16 @@ module Ci module Runners class SetRunnerAssociatedProjectsService # @param [Ci::Runner] runner: the project runner to assign/unassign projects from + # @param [User] current_user: the user performing the operation # @param [Array] project_ids: the IDs of the associated projects to assign the runner to - # @param [User] user: the user performing the operation - def initialize(runner, project_ids, user) + def initialize(runner:, current_user:, project_ids:) @runner = runner + @current_user = current_user @project_ids = project_ids - @user = user end def execute - unless user.present? && user.can?(:assign_runner, runner) + unless current_user.present? && current_user.can?(:assign_runner, runner) return ServiceResponse.error(message: 'user not allowed to assign runner', http_status: :forbidden) end @@ -29,20 +29,14 @@ def set_associated_projects runner.transaction do # rubocop:disable CodeReuse/ActiveRecord current_project_ids = runner.projects.ids + # rubocop:enable CodeReuse/ActiveRecord - new_projects = Project.where(id: project_ids - current_project_ids) - new_projects.each do |project| - next if runner.assign_to(project, user) - + unless associate_new_projects(new_project_ids, current_project_ids) response = ServiceResponse.error(message: 'failed to assign projects to runner') raise ActiveRecord::Rollback, response.errors end - discarded_runner_projects = Ci::RunnerProject.where(project_id: current_project_ids - new_project_ids) - # rubocop:enable CodeReuse/ActiveRecord - discarded_runner_projects.each do |runner_project| - next if runner_project.destroy - + unless disassociate_old_projects(new_project_ids, current_project_ids) response = ServiceResponse.error(message: 'failed to destroy runner project') raise ActiveRecord::Rollback, response.errors end @@ -51,7 +45,29 @@ def set_associated_projects response end - attr_reader :runner, :project_ids, :user + def associate_new_projects(new_project_ids, current_project_ids) + # rubocop:disable CodeReuse/ActiveRecord + missing_projects = Project.where(id: new_project_ids - current_project_ids) + # rubocop:enable CodeReuse/ActiveRecord + missing_projects.each do |project| + return false unless runner.assign_to(project, current_user) + end + + true + end + + def disassociate_old_projects(new_project_ids, current_project_ids) + # rubocop:disable CodeReuse/ActiveRecord + discarded_runner_projects = Ci::RunnerProject.where(project_id: current_project_ids - new_project_ids) + # rubocop:enable CodeReuse/ActiveRecord + discarded_runner_projects.each do |runner_project| + return false unless runner_project.destroy + end + + true + end + + attr_reader :runner, :current_user, :project_ids end end end diff --git a/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb b/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb index f5c2a434b2d952..8fd4f894fc475e 100644 --- a/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb +++ b/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb @@ -13,8 +13,8 @@ def execute audit_context = { name: 'set_runner_associated_projects', - author: user, - scope: user, + author: current_user, + scope: current_user, target: runner, message: 'Changed CI runner project assignments', additional_details: { diff --git a/ee/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb b/ee/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb index 9f104b9084d4ec..d529a7406f5dbe 100644 --- a/ee/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb +++ b/ee/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb @@ -3,12 +3,14 @@ require 'spec_helper' RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute' do + subject(:execute) do + described_class.new(runner: project_runner, current_user: user, project_ids: [new_project.id]).execute + end + 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]) } - subject(:execute) { described_class.new(project_runner, [new_project.id], user).execute } - context 'with unauthorized user' do let(:user) { build(:user) } diff --git a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb index d7377ef4a7b4d0..b3fee24e09d778 100644 --- a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb +++ b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute' do - subject(:execute) { described_class.new(runner, project_ids, user).execute } + subject(:execute) { described_class.new(runner: runner, current_user: user, project_ids: project_ids).execute } let_it_be(:owner_project) { create(:project) } let_it_be(:project2) { create(:project) } @@ -37,15 +37,11 @@ context 'with admin user', :enable_admin_mode do let(:user) { create_default(:user, :admin) } let(:project_ids) { [project3.id, project4.id] } - let(:project3) { create(:project) } let(:project4) { create(:project) } context 'with successful requests' do it 'calls assign_to on runner and returns success response' do - expect(runner).to receive(:assign_to).with(project3, user).once.ordered.and_call_original - expect(runner).to receive(:assign_to).with(project4, user).once.ordered.and_call_original - expect(execute).to be_success expect(runner.reload.projects.ids).to match_array([owner_project.id] + project_ids) end @@ -53,20 +49,21 @@ context 'with failing assign_to requests' do it 'returns error response and rolls back transaction' do - expect(runner).to receive(:assign_to).with(project3, user).once.ordered.and_call_original - expect(runner).to receive(:assign_to).with(project4, user).once.ordered.and_return(false) + expect(runner).to receive(:assign_to).with(project4, user).once.and_return(false) expect(execute).to be_error - expect(runner.reload.projects.ids).to match_array(original_projects.map(&:id)) + expect(runner.reload.projects).to match_array(original_projects) end end context 'with failing destroy calls' do it 'returns error response and rolls back transaction' do - expect_any_instance_of(Ci::RunnerProject).to receive(:destroy).and_return(false) # rubocop:disable RSpec/AnyInstanceOf + allow_next_found_instance_of(Ci::RunnerProject) do |runner_project| + allow(runner_project).to receive(:destroy).and_return(false) + end expect(execute).to be_error - expect(runner.reload.projects.ids).to match_array(original_projects.map(&:id)) + expect(runner.reload.projects).to match_array(original_projects) end end end -- GitLab From f226d1082a54eaed6e41a411076c0e8701d0daf3 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 13 Sep 2022 17:55:06 +0200 Subject: [PATCH 4/5] Address MR review comments --- .../set_runner_associated_projects_service.rb | 17 +++++------------ .../set_runner_associated_projects_service.rb | 13 +++++++++---- spec/models/ci/runner_spec.rb | 19 +++++++++++++++++++ 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/app/services/ci/runners/set_runner_associated_projects_service.rb b/app/services/ci/runners/set_runner_associated_projects_service.rb index 571b939caf1814..759bd2ffb160e5 100644 --- a/app/services/ci/runners/set_runner_associated_projects_service.rb +++ b/app/services/ci/runners/set_runner_associated_projects_service.rb @@ -13,7 +13,7 @@ def initialize(runner:, current_user:, project_ids:) end def execute - unless current_user.present? && current_user.can?(:assign_runner, runner) + unless current_user&.can?(:assign_runner, runner) return ServiceResponse.error(message: 'user not allowed to assign runner', http_status: :forbidden) end @@ -46,9 +46,7 @@ def set_associated_projects end def associate_new_projects(new_project_ids, current_project_ids) - # rubocop:disable CodeReuse/ActiveRecord - missing_projects = Project.where(id: new_project_ids - current_project_ids) - # rubocop:enable CodeReuse/ActiveRecord + missing_projects = Project.id_in(new_project_ids - current_project_ids) missing_projects.each do |project| return false unless runner.assign_to(project, current_user) end @@ -57,14 +55,9 @@ def associate_new_projects(new_project_ids, current_project_ids) end def disassociate_old_projects(new_project_ids, current_project_ids) - # rubocop:disable CodeReuse/ActiveRecord - discarded_runner_projects = Ci::RunnerProject.where(project_id: current_project_ids - new_project_ids) - # rubocop:enable CodeReuse/ActiveRecord - discarded_runner_projects.each do |runner_project| - return false unless runner_project.destroy - end - - true + Ci::RunnerProject + .destroy_by(project_id: current_project_ids - new_project_ids) + .all?(&:destroyed?) end attr_reader :runner, :current_user, :project_ids diff --git a/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb b/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb index 8fd4f894fc475e..a05cd42afb9c95 100644 --- a/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb +++ b/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb @@ -8,8 +8,15 @@ module SetRunnerAssociatedProjectsService override :execute def execute - response = super - return response if response.error? + super.tap do |result| + audit_event_service(result) + end + end + + private + + def audit_event_service(result) + return if result.error? audit_context = { name: 'set_runner_associated_projects', @@ -22,8 +29,6 @@ def execute } } ::Gitlab::Audit::Auditor.audit(audit_context) - - response end end end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index cb5c43a52707e8..7226c9db15d3cb 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -1186,6 +1186,25 @@ def does_db_update end end + describe '#owner_project' do + let_it_be(:project1) { create(:project) } + let_it_be(:project2) { create(:project) } + + subject(:owner_project) { project_runner.owner_project } + + context 'with project1 as first project associated with runner' do + let_it_be(:project_runner) { create(:ci_runner, :project, projects: [project1, project2]) } + + it { is_expected.to eq project1 } + end + + context 'with project2 as first project associated with runner' do + let_it_be(:project_runner) { create(:ci_runner, :project, projects: [project2, project1]) } + + it { is_expected.to eq project2 } + end + end + describe "belongs_to_one_project?" do it "returns false if there are two projects runner assigned to" do project1 = create(:project) -- GitLab From 952c6ad0c744e5c120e99aa80a1acff1e9871abd Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 13 Sep 2022 18:41:21 +0200 Subject: [PATCH 5/5] Add runner_path to audit event --- .../ci/runners/set_runner_associated_projects_service.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb b/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb index a05cd42afb9c95..0d48c569b550f3 100644 --- a/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb +++ b/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb @@ -23,6 +23,7 @@ def audit_event_service(result) author: current_user, scope: current_user, target: runner, + target_details: runner_path, message: 'Changed CI runner project assignments', additional_details: { action: :custom @@ -30,6 +31,12 @@ def audit_event_service(result) } ::Gitlab::Audit::Auditor.audit(audit_context) end + + def runner_path + url_helpers = ::Gitlab::Routing.url_helpers + + url_helpers.project_runner_path(runner.owner_project, runner) + end end end end -- GitLab