Commit 4b488c30 authored by Pedro Pombeiro's avatar Pedro Pombeiro Committed by Douglas Barbosa Alexandre

Prevent group owners from deleting certain project runners

Runners assigned to multiple projects cannot be deleted
by the group owner, only an administrator

Changelog: changed
parent 81811623
......@@ -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
......
......@@ -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
......
......@@ -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
......@@ -31768,10 +31768,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."
......
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment