From daa0d97e75d19b61a2f68855180c3e44020d40bb Mon Sep 17 00:00:00 2001 From: Allison Browne Date: Tue, 7 Nov 2023 10:11:47 -0500 Subject: [PATCH 1/2] Change from update to cancel for pipelines and builds As part of the work to disallow pipeline cancellation for certain users here we switch over cancellation related logic to user cancel_*. --- app/controllers/projects/jobs_controller.rb | 5 +++++ app/graphql/types/permission_types/ci/pipeline.rb | 1 + app/serializers/ci/job_entity.rb | 2 +- app/views/projects/ci/builds/_build.html.haml | 2 +- doc/api/graphql/reference/index.md | 1 + .../models/ci/project_cancellation_restriction.rb | 2 +- lib/api/ci/pipelines.rb | 2 +- .../types/permission_types/ci/pipeline_spec.rb | 13 +++++++++++++ spec/policies/ci/build_policy_spec.rb | 2 +- spec/requests/api/ci/jobs_spec.rb | 4 ++-- spec/serializers/ci/job_entity_spec.rb | 2 +- .../policies/project_policy_shared_context.rb | 2 +- 12 files changed, 29 insertions(+), 9 deletions(-) create mode 100644 spec/graphql/types/permission_types/ci/pipeline_spec.rb diff --git a/app/controllers/projects/jobs_controller.rb b/app/controllers/projects/jobs_controller.rb index 802ffd99e41d3a..25cc6b64d7fa16 100644 --- a/app/controllers/projects/jobs_controller.rb +++ b/app/controllers/projects/jobs_controller.rb @@ -14,6 +14,7 @@ class Projects::JobsController < Projects::ApplicationController before_action :authorize_read_build! before_action :authorize_update_build!, except: [:index, :show, :raw, :trace, :erase, :cancel, :unschedule] + before_action :authorize_cancel_build!, only: [:cancel] before_action :authorize_erase_build!, only: [:erase] before_action :authorize_use_build_terminal!, only: [:terminal, :terminal_websocket_authorize] before_action :verify_api_request!, only: :terminal_websocket_authorize @@ -174,6 +175,10 @@ def authorize_update_build! return access_denied! unless can?(current_user, :update_build, @build) end + def authorize_cancel_build! + return access_denied! unless can?(current_user, :cancel_build, @build) + end + def authorize_erase_build! return access_denied! unless can?(current_user, :erase_build, @build) end diff --git a/app/graphql/types/permission_types/ci/pipeline.rb b/app/graphql/types/permission_types/ci/pipeline.rb index cfd68380005b64..94adbf7c59b67f 100644 --- a/app/graphql/types/permission_types/ci/pipeline.rb +++ b/app/graphql/types/permission_types/ci/pipeline.rb @@ -8,6 +8,7 @@ class Pipeline < BasePermissionType abilities :admin_pipeline, :destroy_pipeline ability_field :update_pipeline, calls_gitaly: true + ability_field :cancel_pipeline, calls_gitaly: true end end end diff --git a/app/serializers/ci/job_entity.rb b/app/serializers/ci/job_entity.rb index 813938c2a188c8..828a9eb33a526e 100644 --- a/app/serializers/ci/job_entity.rb +++ b/app/serializers/ci/job_entity.rb @@ -53,7 +53,7 @@ class JobEntity < Grape::Entity alias_method :job, :object def cancelable? - job.cancelable? && can?(request.current_user, :update_build, job) + job.cancelable? && can?(request.current_user, :cancel_build, job) end def retryable? diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index 6ec9b4a233de34..f3018315c5a8c4 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -104,7 +104,7 @@ .btn-group - if can?(current_user, :read_job_artifacts, job) && job.artifacts? = link_button_to nil, download_project_job_artifacts_path(job.project, job), rel: 'nofollow', download: '', title: _('Download artifacts'), icon: 'download' - - if can?(current_user, :update_build, job) + - if can?(current_user, :cancel_build, job) - if job.active? && can?(current_user, :cancel_build, job) = link_button_to nil, cancel_project_job_path(job.project, job, continue: { to: request.fullpath }), method: :post, title: _('Cancel'), icon: 'cancel' - elsif job.scheduled? diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 6c22ffa5f22972..7102e7547d2d9c 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -22712,6 +22712,7 @@ Represents pipeline counts for the project. | Name | Type | Description | | ---- | ---- | ----------- | | `adminPipeline` | [`Boolean!`](#boolean) | If `true`, the user can perform `admin_pipeline` on this resource. | +| `cancelPipeline` | [`Boolean!`](#boolean) | If `true`, the user can perform `cancel_pipeline` on this resource. | | `destroyPipeline` | [`Boolean!`](#boolean) | If `true`, the user can perform `destroy_pipeline` on this resource. | | `updatePipeline` | [`Boolean!`](#boolean) | If `true`, the user can perform `update_pipeline` on this resource. | diff --git a/ee/app/models/ci/project_cancellation_restriction.rb b/ee/app/models/ci/project_cancellation_restriction.rb index 7eaca4f6a619df..69a46703809948 100644 --- a/ee/app/models/ci/project_cancellation_restriction.rb +++ b/ee/app/models/ci/project_cancellation_restriction.rb @@ -2,7 +2,7 @@ module Ci class ProjectCancellationRestriction - # Checks if cancellation restrictions are applied for piplines and processables + # Checks if cancellation restrictions are applied for pipelines and processables # based on the given project include Gitlab::Utils::StrongMemoize diff --git a/lib/api/ci/pipelines.rb b/lib/api/ci/pipelines.rb index 3361f4564b2632..b5123ab49dc550 100644 --- a/lib/api/ci/pipelines.rb +++ b/lib/api/ci/pipelines.rb @@ -352,7 +352,7 @@ class Pipelines < ::API::Base requires :pipeline_id, type: Integer, desc: 'The pipeline ID', documentation: { example: 18 } end post ':id/pipelines/:pipeline_id/cancel', urgency: :low, feature_category: :continuous_integration do - authorize! :update_pipeline, pipeline + authorize! :cancel_pipeline, pipeline # TODO: inconsistent behavior: when pipeline is not cancelable we should return an error ::Ci::CancelPipelineService.new(pipeline: pipeline, current_user: current_user).execute diff --git a/spec/graphql/types/permission_types/ci/pipeline_spec.rb b/spec/graphql/types/permission_types/ci/pipeline_spec.rb new file mode 100644 index 00000000000000..6830b659b12866 --- /dev/null +++ b/spec/graphql/types/permission_types/ci/pipeline_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Types::PermissionTypes::Ci::Pipeline, feature_category: :continuous_integration do + it 'has expected permission fields' do + expected_permissions = [ + :admin_pipeline, :destroy_pipeline, :update_pipeline, :cancel_pipeline + ] + + expect(described_class).to have_graphql_fields(expected_permissions).only + end +end diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb index ab92936440c5d7..30df3ddabafe0e 100644 --- a/spec/policies/ci/build_policy_spec.rb +++ b/spec/policies/ci/build_policy_spec.rb @@ -109,7 +109,7 @@ allow(project).to receive(:branch_allows_collaboration?).and_return(true) end - it 'enables update_build if user is maintainer' do + it 'enables updates if user is maintainer' do expect(policy).to be_allowed :cancel_build expect(policy).to be_allowed :update_build expect(policy).to be_allowed :update_commit_status diff --git a/spec/requests/api/ci/jobs_spec.rb b/spec/requests/api/ci/jobs_spec.rb index 386718cba4972d..e9e9ec34a41237 100644 --- a/spec/requests/api/ci/jobs_spec.rb +++ b/spec/requests/api/ci/jobs_spec.rb @@ -789,14 +789,14 @@ def go end context 'authorized user' do - context 'user with :update_build persmission' do + context 'user with :cancel_build persmission' do it 'cancels running or pending job' do expect(response).to have_gitlab_http_status(:created) expect(project.builds.first.status).to eq('success') end end - context 'user without :update_build permission' do + context 'user without :cancel_build permission' do let(:api_user) { reporter } it 'does not cancel job' do diff --git a/spec/serializers/ci/job_entity_spec.rb b/spec/serializers/ci/job_entity_spec.rb index 6dce87a1fc50fb..c3d0de11405c53 100644 --- a/spec/serializers/ci/job_entity_spec.rb +++ b/spec/serializers/ci/job_entity_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::JobEntity do +RSpec.describe Ci::JobEntity, feature_category: :continuous_integration do let(:user) { create(:user) } let(:job) { create(:ci_build, :running) } let(:project) { job.project } diff --git a/spec/support/shared_contexts/policies/project_policy_shared_context.rb b/spec/support/shared_contexts/policies/project_policy_shared_context.rb index 5014a810f35d2f..68eb35398134f3 100644 --- a/spec/support/shared_contexts/policies/project_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/project_policy_shared_context.rb @@ -54,7 +54,7 @@ create_environment create_merge_request_from admin_metrics_dashboard_annotation create_pipeline create_release create_wiki destroy_container_image push_code read_pod_logs - read_terraform_state resolve_note update_build update_commit_status + read_terraform_state resolve_note update_build cancel_build update_commit_status update_container_image update_deployment update_environment update_merge_request update_pipeline update_release destroy_release read_resource_group update_resource_group update_escalation_status -- GitLab From 338ccbdd6489fbb8a478e722f5bf2339be2b6d1f Mon Sep 17 00:00:00 2001 From: Allison Browne Date: Tue, 7 Nov 2023 16:01:28 -0500 Subject: [PATCH 2/2] Show and hide the retry button based on the update permission --- app/views/projects/ci/builds/_build.html.haml | 10 +++++----- spec/policies/ci/build_policy_spec.rb | 2 +- spec/requests/api/ci/jobs_spec.rb | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index f3018315c5a8c4..76d6b0a042dddd 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -104,10 +104,10 @@ .btn-group - if can?(current_user, :read_job_artifacts, job) && job.artifacts? = link_button_to nil, download_project_job_artifacts_path(job.project, job), rel: 'nofollow', download: '', title: _('Download artifacts'), icon: 'download' - - if can?(current_user, :cancel_build, job) - - if job.active? && can?(current_user, :cancel_build, job) - = link_button_to nil, cancel_project_job_path(job.project, job, continue: { to: request.fullpath }), method: :post, title: _('Cancel'), icon: 'cancel' - - elsif job.scheduled? + - if can?(current_user, :cancel_build, job) && job.active? + = link_button_to nil, cancel_project_job_path(job.project, job, continue: { to: request.fullpath }), method: :post, title: _('Cancel'), icon: 'cancel' + - if can?(current_user, :update_build, job) + - if job.scheduled? = render Pajamas::ButtonComponent.new(disabled: true, icon: 'planning') do %time.js-remaining-time{ datetime: job.scheduled_at.utc.iso8601 } = duration_in_numbers(job.execute_in) @@ -124,7 +124,7 @@ class: 'has-tooltip', icon: 'time-out' - elsif allow_retry - - if job.playable? && !admin && can?(current_user, :update_build, job) + - if job.playable? && !admin = link_button_to nil, play_project_job_path(job.project, job, return_to: request.original_url), method: :post, title: _('Play'), icon: 'play' - elsif job.retryable? = link_button_to nil, retry_project_job_path(job.project, job, return_to: request.original_url), method: :post, title: _('Retry'), icon: 'retry' diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb index 30df3ddabafe0e..ad568e60d5ce0a 100644 --- a/spec/policies/ci/build_policy_spec.rb +++ b/spec/policies/ci/build_policy_spec.rb @@ -109,7 +109,7 @@ allow(project).to receive(:branch_allows_collaboration?).and_return(true) end - it 'enables updates if user is maintainer' do + it 'enables updates if user is maintainer', :aggregate_failures do expect(policy).to be_allowed :cancel_build expect(policy).to be_allowed :update_build expect(policy).to be_allowed :update_commit_status diff --git a/spec/requests/api/ci/jobs_spec.rb b/spec/requests/api/ci/jobs_spec.rb index e9e9ec34a41237..758ff25c47bdcd 100644 --- a/spec/requests/api/ci/jobs_spec.rb +++ b/spec/requests/api/ci/jobs_spec.rb @@ -789,7 +789,7 @@ def go end context 'authorized user' do - context 'user with :cancel_build persmission' do + context 'user with :cancel_build permission' do it 'cancels running or pending job' do expect(response).to have_gitlab_http_status(:created) expect(project.builds.first.status).to eq('success') -- GitLab