Merge branch...

Merge branch 'pedropombeiro/354004/prevent-group-owners-from-deleting-other-project-runners' into 'master'

Prevent group owners from deleting certain project runners

See merge request gitlab-org/gitlab!82023

app/controllers/groups/runners_controller.rb

@@ -32,12 +32,12 @@ class Groups::RunnersController < Groups::ApplicationController
   end
 
   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
+    if can?(current_user, :delete_runner, @runner)
       Ci::Runners::UnregisterRunnerService.new(@runner, current_user).execute
 
       redirect_to group_settings_ci_cd_path(@group, anchor: 'runners-settings'), status: :found
+    else
+      redirect_to group_settings_ci_cd_path(@group, anchor: 'runners-settings'), status: :found, alert: _('Runner cannot be deleted, please contact your administrator.')
     end
   end
 

app/graphql/mutations/ci/runner/delete.rb

@@ -17,20 +17,11 @@ module Mutations
         def resolve(id:, **runner_attrs)
           runner = authorized_find!(id)
 
-          error = authenticate_delete_runner!(runner)
-          return { errors: [error] } if error
-
           ::Ci::Runners::UnregisterRunnerService.new(runner, current_user).execute
 
           { errors: runner.errors.full_messages }
         end
 
-        def authenticate_delete_runner!(runner)
-          return if current_user.can_admin_all_resources?
-
-          "Runner #{runner.to_global_id} associated with more than one project" if runner.runner_projects.count > 1
-        end
-
         def find_object(id)
           # TODO: remove this line when the compatibility layer is removed
           # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883

app/policies/ci/runner_policy.rb

@@ -9,6 +9,10 @@ module Ci
       @user.owns_runner?(@subject)
     end
 
+    condition(:belongs_to_multiple_projects) do
+      @subject.belongs_to_more_than_one_project?
+    end
+
     rule { anonymous }.prevent_all
 
     rule { admin }.policy do
@@ -22,6 +26,8 @@ module Ci
       enable :delete_runner
     end
 
+    rule { ~admin & belongs_to_multiple_projects }.prevent :delete_runner
+
     rule { ~admin & locked }.prevent :assign_runner
   end
 end

locale/gitlab.pot

@@ -31801,10 +31801,10 @@ msgstr ""
 msgid "Runner API"
 msgstr ""
 
-msgid "Runner tokens"
+msgid "Runner cannot be deleted, please contact your administrator."
 msgstr ""
 
-msgid "Runner was not deleted because it is assigned to multiple projects."
+msgid "Runner tokens"
 msgstr ""
 
 msgid "Runner was not updated."

spec/controllers/groups/runners_controller_spec.rb

@@ -208,11 +208,28 @@ RSpec.describe Groups::RunnersController do
       end
     end
 
-    context 'when user is an owner and runner in multiple projects' do
-      let(:project_2) { create(:project, group: group) }
+    context 'with runner associated with multiple projects' do
+      let_it_be(:project_2) { create(:project, group: group) }
+
       let(:runner_project_2) { create(:ci_runner, :project, projects: [project, project_2]) }
       let(:params_runner_project_2) { { group_id: group, id: runner_project_2 } }
 
+      context 'when user is an admin', :enable_admin_mode do
+        let(:user) { create(:user, :admin) }
+
+        before do
+          sign_in(user)
+        end
+
+        it 'destroys the project runner and redirects' do
+          delete :destroy, params: params_runner_project_2
+
+          expect(response).to have_gitlab_http_status(:found)
+          expect(Ci::Runner.find_by(id: runner_project_2.id)).to be_nil
+        end
+      end
+
+      context 'when user is an owner' do
         before do
           group.add_owner(user)
         end
@@ -221,10 +238,11 @@ RSpec.describe Groups::RunnersController do
           delete :destroy, params: params_runner_project_2
 
           expect(response).to have_gitlab_http_status(:found)
-        expect(flash[:alert]).to eq('Runner was not deleted because it is assigned to multiple projects.')
+          expect(flash[:alert]).to eq('Runner cannot be deleted, please contact your administrator.')
           expect(Ci::Runner.find_by(id: runner_project_2.id)).to be_present
         end
       end
+    end
 
     context 'when user is not an owner' do
       before do

spec/graphql/mutations/ci/runner/delete_spec.rb

@@ -6,8 +6,9 @@ RSpec.describe Mutations::Ci::Runner::Delete do
   include GraphqlHelpers
 
   let_it_be(:runner) { create(:ci_runner) }
+  let_it_be(:admin_user) { create(:user, :admin) }
+  let_it_be(:user) { create(:user) }
 
-  let(:user) { create(:user) }
   let(:current_ctx) { { current_user: user } }
 
   let(:mutation_params) do
@@ -27,12 +28,24 @@ RSpec.describe Mutations::Ci::Runner::Delete do
           subject
         end
       end
+
+      context 'with more than one associated project' do
+        let!(:project) { create(:project, creator_id: user.id) }
+        let!(:project2) { create(:project, creator_id: user.id) }
+        let!(:two_projects_runner) { create(:ci_runner, :project, description: 'Two projects runner', projects: [project, project2]) }
+
+        it 'raises an error' do
+          mutation_params[:id] = two_projects_runner.to_global_id
+
+          expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+        end
+      end
     end
 
     context 'with invalid params' do
-      it 'raises an error' do
-        mutation_params[:id] = "invalid-id"
+      let(:mutation_params) { { id: "invalid-id" } }
 
+      it 'raises an error' do
         expect { subject }.to raise_error(::GraphQL::CoercionError)
       end
     end
@@ -46,6 +59,8 @@ RSpec.describe Mutations::Ci::Runner::Delete do
     end
 
     context 'when user can delete owned runner' do
+      let_it_be(:user) { create(:user) }
+
       let!(:project) { create(:project, creator_id: user.id) }
       let!(:project_runner) { create(:ci_runner, :project, description: 'Project runner', projects: [project]) }
 
@@ -54,9 +69,11 @@ RSpec.describe Mutations::Ci::Runner::Delete do
       end
 
       context 'with one associated project' do
-        it 'deletes runner' do
-          mutation_params[:id] = project_runner.to_global_id
+        let(:mutation_params) do
+          { id: project_runner.to_global_id }
+        end
 
+        it 'deletes runner' do
           expect_next_instance_of(::Ci::Runners::UnregisterRunnerService, project_runner, current_ctx[:current_user]) do |service|
             expect(service).to receive(:execute).once.and_call_original
           end
@@ -70,24 +87,41 @@ RSpec.describe Mutations::Ci::Runner::Delete do
         let!(:project2) { create(:project, creator_id: user.id) }
         let!(:two_projects_runner) { create(:ci_runner, :project, description: 'Two projects runner', projects: [project, project2]) }
 
+        let(:mutation_params) do
+          { id: two_projects_runner.to_global_id }
+        end
+
+        context 'with user as admin', :enable_admin_mode do
+          let(:current_ctx) { { current_user: admin_user } }
+
+          it 'deletes runner' do
+            expect_next_instance_of(::Ci::Runners::UnregisterRunnerService, two_projects_runner, current_ctx[:current_user]) do |service|
+              expect(service).to receive(:execute).once.and_call_original
+            end
+
+            expect { subject }.to change { Ci::Runner.count }.by(-1)
+            expect(subject[:errors]).to be_empty
+          end
+        end
+
+        context 'with user as project maintainer' do
+          let_it_be(:user) { create(:user) }
+
           before do
             project2.add_maintainer(user)
           end
 
-        it 'does not delete project runner' do
-          mutation_params[:id] = two_projects_runner.to_global_id
-
+          it 'raises error' do
             allow_next_instance_of(::Ci::Runners::UnregisterRunnerService) do |service|
               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")
+            expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+          end
         end
       end
     end
 
     context 'when admin can delete runner', :enable_admin_mode do
-      let(:admin_user) { create(:user, :admin) }
       let(:current_ctx) { { current_user: admin_user } }
 
       it 'deletes runner' do