Merge remote-tracking branch 'dev/15-9-stable' into 15-9-stable

7 files changed, 127 insertions, 105 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5330484d047..bfe402bef6d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,12 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 15.9.7 (2023-05-03)
+
+### Security (1 change)
+
+- [Only maintainers of projects should be able to assign runners to them](gitlab-org/security/gitlab@695748314b758ca2d9992df7509025a6ac868000) ([merge request](gitlab-org/security/gitlab!3236))
+
 ## 15.9.6 (2023-05-01)
 
 ### Security (8 changes)
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index c7c1b69191f..916b312da47 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-15.9.6
\ No newline at end of file
+15.9.7
\ No newline at end of file
diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION
index c7c1b69191f..916b312da47 100644
--- a/GITLAB_PAGES_VERSION
+++ b/GITLAB_PAGES_VERSION
@@ -1 +1 @@
-15.9.6
\ No newline at end of file
+15.9.7
\ No newline at end of file
diff --git a/VERSION b/VERSION
index c7c1b69191f..916b312da47 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-15.9.6
\ No newline at end of file
+15.9.7
\ No newline at end of file
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 5e33fdae2f4..3608fdfac71 100644
--- a/app/services/ci/runners/set_runner_associated_projects_service.rb
+++ b/app/services/ci/runners/set_runner_associated_projects_service.rb
@@ -33,15 +33,9 @@ module Ci
           current_project_ids = runner.projects.ids
           # rubocop:enable CodeReuse/ActiveRecord
 
-          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
-
-          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
+          response = associate_new_projects(new_project_ids, current_project_ids)
+          response = disassociate_old_projects(new_project_ids, current_project_ids) if response.success?
+          raise ActiveRecord::Rollback, response.errors unless response.success?
         end
 
         response
@@ -49,16 +43,29 @@ module Ci
 
       def associate_new_projects(new_project_ids, current_project_ids)
         missing_projects = Project.id_in(new_project_ids - current_project_ids)
-        missing_projects.all? { |project| runner.assign_to(project, current_user) }
+
+        unless missing_projects.all? { |project| current_user.can?(:register_project_runners, project) }
+          return ServiceResponse.error(message: 'user is not authorized to add runners to project')
+        end
+
+        unless missing_projects.all? { |project| runner.assign_to(project, current_user) }
+          return ServiceResponse.error(message: 'failed to assign projects to runner')
+        end
+
+        ServiceResponse.success
       end
 
       def disassociate_old_projects(new_project_ids, current_project_ids)
         projects_to_be_deleted = current_project_ids - new_project_ids
-        return true if projects_to_be_deleted.empty?
+        return ServiceResponse.success if projects_to_be_deleted.empty?
+
+        all_destroyed =
+          Ci::RunnerProject
+            .destroy_by(project_id: projects_to_be_deleted)
+            .all?(&:destroyed?)
+        return ServiceResponse.success if all_destroyed
 
-        Ci::RunnerProject
-          .destroy_by(project_id: projects_to_be_deleted)
-          .all?(&:destroyed?)
+        ServiceResponse.error(message: 'failed to destroy runner project')
       end
 
       attr_reader :runner, :current_user, :project_ids
diff --git a/spec/graphql/mutations/ci/runner/update_spec.rb b/spec/graphql/mutations/ci/runner/update_spec.rb
index e0c8219e0f6..50351321be8 100644
--- a/spec/graphql/mutations/ci/runner/update_spec.rb
+++ b/spec/graphql/mutations/ci/runner/update_spec.rb
@@ -46,10 +46,17 @@ RSpec.describe Mutations::Ci::Runner::Update, feature_category: :runner_fleet do
       end
     end
 
-    context 'when user can update runner', :enable_admin_mode do
-      let_it_be(:admin_user) { create(:user, :admin) }
+    context 'when user can update runner' do
+      let_it_be(:user) { create(:user) }
 
-      let(:current_ctx) { { current_user: admin_user } }
+      let(:original_projects) { [project1, project2] }
+      let(:projects_with_maintainer_access) { original_projects }
+
+      let(:current_ctx) { { current_user: user } }
+
+      before do
+        projects_with_maintainer_access.each { |project| project.add_maintainer(user) }
+      end
 
       context 'with valid arguments' do
         let(:mutation_params) do
@@ -82,27 +89,22 @@ RSpec.describe Mutations::Ci::Runner::Update, feature_category: :runner_fleet do
 
       context 'with associatedProjects argument' do
         let_it_be(:project3) { create(:project) }
+        let_it_be(:project4) { create(:project) }
+
+        let(:new_projects) { [project3, project4] }
+        let(:mutation_params) do
+          {
+            id: runner.to_global_id,
+            description: 'updated description',
+            associated_projects: new_projects.map { |project| project.to_global_id.to_s }
+          }
+        end
 
         context 'with id set to project runner' do
-          let(:mutation_params) do
-            {
-              id: runner.to_global_id,
-              description: 'updated description',
-              associated_projects: [project3.to_global_id.to_s]
-            }
-          end
+          let(:projects_with_maintainer_access) { original_projects + new_projects }
 
           it 'updates runner attributes and project relationships', :aggregate_failures do
-            expect_next_instance_of(
-              ::Ci::Runners::SetRunnerAssociatedProjectsService,
-              {
-                runner: runner,
-                current_user: admin_user,
-                project_ids: [project3.id]
-              }
-            ) do |service|
-              expect(service).to receive(:execute).and_call_original
-            end
+            setup_service_expectations
 
             expected_attributes = mutation_params.except(:id, :associated_projects)
 
@@ -112,57 +114,32 @@ RSpec.describe Mutations::Ci::Runner::Update, feature_category: :runner_fleet do
             expect(response[:runner]).to be_an_instance_of(Ci::Runner)
             expect(response[:runner]).to have_attributes(expected_attributes)
             expect(runner.reload).to have_attributes(expected_attributes)
-            expect(runner.projects).to match_array([project1, project3])
+            expect(runner.projects).to match_array([project1] + new_projects)
           end
 
-          context 'with user not allowed to assign runner' do
-            before do
-              allow(admin_user).to receive(:can?).with(:assign_runner, runner).and_return(false)
-            end
+          context 'with missing permissions on one of the new projects' do
+            let(:projects_with_maintainer_access) { original_projects + [project3] }
 
             it 'does not update runner', :aggregate_failures do
-              expect_next_instance_of(
-                ::Ci::Runners::SetRunnerAssociatedProjectsService,
-                {
-                  runner: runner,
-                  current_user: admin_user,
-                  project_ids: [project3.id]
-                }
-              ) do |service|
-                expect(service).to receive(:execute).and_call_original
-              end
+              setup_service_expectations
 
               expected_attributes = mutation_params.except(:id, :associated_projects)
 
               response
 
-              expect(response[:errors]).to match_array(['user not allowed to assign runner'])
+              expect(response[:errors]).to match_array(['user is not authorized to add runners to project'])
               expect(response[:runner]).to be_nil
               expect(runner.reload).not_to have_attributes(expected_attributes)
-              expect(runner.projects).to match_array([project1, project2])
+              expect(runner.projects).to match_array(original_projects)
             end
           end
         end
 
         context 'with an empty list of projects' do
-          let(:mutation_params) do
-            {
-              id: runner.to_global_id,
-              associated_projects: []
-            }
-          end
+          let(:new_projects) { [] }
 
           it 'removes project relationships', :aggregate_failures do
-            expect_next_instance_of(
-              ::Ci::Runners::SetRunnerAssociatedProjectsService,
-              {
-                runner: runner,
-                current_user: admin_user,
-                project_ids: []
-              }
-            ) do |service|
-              expect(service).to receive(:execute).and_call_original
-            end
+            setup_service_expectations
 
             response
 
@@ -172,15 +149,9 @@ RSpec.describe Mutations::Ci::Runner::Update, feature_category: :runner_fleet do
           end
         end
 
-        context 'with id set to instance runner' do
-          let(:instance_runner) { create(:ci_runner, :instance) }
-          let(:mutation_params) do
-            {
-              id: instance_runner.to_global_id,
-              description: 'updated description',
-              associated_projects: [project2.to_global_id.to_s]
-            }
-          end
+        context 'with id set to instance runner', :enable_admin_mode do
+          let_it_be(:user) { create(:user, :admin) }
+          let_it_be(:runner) { create(:ci_runner, :instance) }
 
           it 'raises error', :aggregate_failures do
             expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ArgumentError) do
@@ -188,6 +159,19 @@ RSpec.describe Mutations::Ci::Runner::Update, feature_category: :runner_fleet do
             end
           end
         end
+
+        def setup_service_expectations
+          expect_next_instance_of(
+            ::Ci::Runners::SetRunnerAssociatedProjectsService,
+            {
+              runner: runner,
+              current_user: user,
+              project_ids: new_projects.map(&:id)
+            }
+          ) do |service|
+            expect(service).to receive(:execute).and_call_original
+          end
+        end
       end
 
       context 'with non-existing project ID in associatedProjects argument' do
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 9921f9322bd..d952fca25a5 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,17 +3,19 @@
 require 'spec_helper'
 
 RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', feature_category: :runner_fleet do
-  subject(:execute) { described_class.new(runner: runner, current_user: user, project_ids: project_ids).execute }
+  subject(:execute) do
+    described_class.new(runner: runner, current_user: user, project_ids: new_projects.map(&:id)).execute
+  end
 
   let_it_be(:owner_project) { create(:project) }
   let_it_be(:project2) { create(:project) }
-  let_it_be(:original_projects) { [owner_project, project2] }
 
+  let(:original_projects) { [owner_project, project2] }
   let(:runner) { create(:ci_runner, :project, projects: original_projects) }
 
   context 'without user' do
     let(:user) { nil }
-    let(:project_ids) { [project2.id] }
+    let(:new_projects) { [project2] }
 
     it 'does not call assign_to on runner and returns error response', :aggregate_failures do
       expect(runner).not_to receive(:assign_to)
@@ -24,8 +26,8 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', fe
   end
 
   context 'with unauthorized user' do
-    let(:user) { build(:user) }
-    let(:project_ids) { [project2.id] }
+    let(:user) { create(:user) }
+    let(:new_projects) { [project2] }
 
     it 'does not call assign_to on runner and returns error message' do
       expect(runner).not_to receive(:assign_to)
@@ -35,15 +37,19 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', fe
     end
   end
 
-  context 'with admin user', :enable_admin_mode do
-    let_it_be(:user) { create(:user, :admin) }
+  context 'with authorized user' do
+    let_it_be(:project3) { create(:project) }
+    let_it_be(:project4) { create(:project) }
+
+    let(:projects_with_maintainer_access) { original_projects }
 
-    let(:project3) { create(:project) }
-    let(:project4) { create(:project) }
+    before do
+      projects_with_maintainer_access.each { |project| project.add_maintainer(user) }
+    end
 
-    context 'with successful requests' do
+    shared_context 'with successful requests' do
       context 'when disassociating a project' do
-        let(:project_ids) { [project3.id, project4.id] }
+        let(:new_projects) { [project3, project4] }
 
         it 'reassigns associated projects and returns success response' do
           expect(execute).to be_success
@@ -51,12 +57,12 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', fe
           runner.reload
 
           expect(runner.owner_project).to eq(owner_project)
-          expect(runner.projects.ids).to match_array([owner_project.id] + project_ids)
+          expect(runner.projects.ids).to match_array([owner_project.id] + new_projects.map(&:id))
         end
       end
 
       context 'when disassociating no projects' do
-        let(:project_ids) { [project2.id, project3.id] }
+        let(:new_projects) { [project2, project3] }
 
         it 'reassigns associated projects and returns success response' do
           expect(execute).to be_success
@@ -64,12 +70,12 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', fe
           runner.reload
 
           expect(runner.owner_project).to eq(owner_project)
-          expect(runner.projects.ids).to match_array([owner_project.id] + project_ids)
+          expect(runner.projects.ids).to match_array([owner_project.id] + new_projects.map(&:id))
         end
       end
 
       context 'when disassociating all projects' do
-        let(:project_ids) { [] }
+        let(:new_projects) { [] }
 
         it 'reassigns associated projects and returns success response' do
           expect(execute).to be_success
@@ -82,28 +88,47 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', fe
       end
     end
 
-    context 'with failing assign_to requests' do
-      let(:project_ids) { [project3.id, project4.id] }
+    shared_context 'with failing destroy calls' do
+      let(:new_projects) { [project3, project4] }
 
       it 'returns error response and rolls back transaction' do
-        expect(runner).to receive(:assign_to).with(project4, user).once.and_return(false)
+        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).to eq(original_projects)
       end
     end
 
-    context 'with failing destroy calls' do
-      let(:project_ids) { [project3.id, project4.id] }
+    context 'with maintainer user' do
+      let(:user) { create(:user) }
+      let(:projects_with_maintainer_access) { original_projects + new_projects }
 
-      it 'returns error response and rolls back transaction' do
-        allow_next_found_instance_of(Ci::RunnerProject) do |runner_project|
-          allow(runner_project).to receive(:destroy).and_return(false)
-        end
+      it_behaves_like 'with successful requests'
+      it_behaves_like 'with failing destroy calls'
 
-        expect(execute).to be_error
-        expect(runner.reload.projects).to eq(original_projects)
+      context 'when associating new projects' do
+        let(:new_projects) { [project3, project4] }
+
+        context 'with missing permissions on one of the new projects' do
+          let(:projects_with_maintainer_access) { original_projects + [project3] }
+
+          it 'returns error response and rolls back transaction' do
+            expect(execute).to be_error
+            expect(execute.errors).to contain_exactly('user is not authorized to add runners to project')
+            expect(runner.reload.projects).to eq(original_projects)
+          end
+        end
       end
     end
+
+    context 'with admin user', :enable_admin_mode do
+      let(:user) { create(:user, :admin) }
+      let(:projects_with_maintainer_access) { original_projects + new_projects }
+
+      it_behaves_like 'with successful requests'
+      it_behaves_like 'with failing destroy calls'
+    end
   end
 end