From 4a96f5bf1c05892428cad107745b351c44d3bddc Mon Sep 17 00:00:00 2001 From: Allison Browne Date: Tue, 31 Oct 2023 12:12:17 -0400 Subject: [PATCH 1/2] Split out cancel build ability from update --- app/graphql/mutations/ci/job/cancel.rb | 2 +- app/graphql/types/permission_types/ci/job.rb | 1 + app/policies/ci/build_policy.rb | 2 ++ app/policies/ci/deployable_policy.rb | 5 ++++- app/policies/project_policy.rb | 2 ++ app/services/ci/build_cancel_service.rb | 2 +- app/views/projects/ci/builds/_build.html.haml | 2 +- doc/api/graphql/reference/index.md | 1 + lib/api/ci/jobs.rb | 5 +++-- lib/api/helpers.rb | 4 ++++ lib/gitlab/ci/status/build/cancelable.rb | 2 +- spec/policies/ci/build_policy_spec.rb | 6 ++++++ spec/policies/project_policy_spec.rb | 4 ++-- .../shared_examples/ci/deployable_policy_shared_examples.rb | 1 + 14 files changed, 30 insertions(+), 9 deletions(-) diff --git a/app/graphql/mutations/ci/job/cancel.rb b/app/graphql/mutations/ci/job/cancel.rb index dc9f4d1977930c..44a7772019d1b3 100644 --- a/app/graphql/mutations/ci/job/cancel.rb +++ b/app/graphql/mutations/ci/job/cancel.rb @@ -11,7 +11,7 @@ class Cancel < Base null: true, description: 'Job after the mutation.' - authorize :update_build + authorize :cancel_build def resolve(id:) job = authorized_find!(id: id) diff --git a/app/graphql/types/permission_types/ci/job.rb b/app/graphql/types/permission_types/ci/job.rb index c9a85317e67c7f..35904fb1fc332e 100644 --- a/app/graphql/types/permission_types/ci/job.rb +++ b/app/graphql/types/permission_types/ci/job.rb @@ -8,6 +8,7 @@ class Job < BasePermissionType abilities :read_job_artifacts, :read_build ability_field :update_build, calls_gitaly: true + ability_field :cancel_build, calls_gitaly: true end end end diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index bce7ceafe17499..71ea42e1f235cf 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -81,6 +81,7 @@ class BuildPolicy < CommitStatusPolicy end rule { ~can?(:jailbreak) & (archived | protected_ref) }.policy do + prevent :cancel_build prevent :update_build prevent :erase_build end @@ -88,6 +89,7 @@ class BuildPolicy < CommitStatusPolicy rule { can?(:admin_build) | (can?(:update_build) & owner_of_job & unprotected_ref) }.enable :erase_build rule { can?(:public_access) & branch_allows_collaboration }.policy do + enable :cancel_build enable :update_build enable :update_commit_status end diff --git a/app/policies/ci/deployable_policy.rb b/app/policies/ci/deployable_policy.rb index f0105b001f216d..e83bdd5361a6bb 100644 --- a/app/policies/ci/deployable_policy.rb +++ b/app/policies/ci/deployable_policy.rb @@ -11,7 +11,10 @@ module DeployablePolicy @subject.outdated_deployment? end - rule { outdated_deployment }.prevent :update_build + rule { outdated_deployment }.policy do + prevent :cancel_build + prevent :update_build + end end end end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 74d2272b670e58..e97e927dab32fc 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -468,6 +468,7 @@ class ProjectPolicy < BasePolicy enable :update_commit_status enable :create_build enable :update_build + enable :cancel_build enable :read_resource_group enable :update_resource_group enable :create_merge_request_from @@ -640,6 +641,7 @@ class ProjectPolicy < BasePolicy rule { builds_disabled | repository_disabled }.policy do prevent(*create_read_update_admin_destroy(:build)) + prevent :cancel_build prevent(*create_read_update_admin_destroy(:pipeline_schedule)) prevent(*create_read_update_admin_destroy(:environment)) prevent(*create_read_update_admin_destroy(:deployment)) diff --git a/app/services/ci/build_cancel_service.rb b/app/services/ci/build_cancel_service.rb index a23418ed7381bd..834d4febd10089 100644 --- a/app/services/ci/build_cancel_service.rb +++ b/app/services/ci/build_cancel_service.rb @@ -21,7 +21,7 @@ def execute attr_reader :build, :user def allowed? - user.can?(:update_build, build) + user.can?(:cancel_build, build) end def forbidden diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index a622895b4de158..6ec9b4a233de34 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -105,7 +105,7 @@ - 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 job.active? + - 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? = render Pajamas::ButtonComponent.new(disabled: true, icon: 'planning') do diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index f73865cc502347..a1901ce927964e 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -19952,6 +19952,7 @@ Represents the Geo replication and verification state of a job_artifact. | Name | Type | Description | | ---- | ---- | ----------- | +| `cancelBuild` | [`Boolean!`](#boolean) | If `true`, the user can perform `cancel_build` on this resource. | | `readBuild` | [`Boolean!`](#boolean) | If `true`, the user can perform `read_build` on this resource. | | `readJobArtifacts` | [`Boolean!`](#boolean) | If `true`, the user can perform `read_job_artifacts` on this resource. | | `updateBuild` | [`Boolean!`](#boolean) | If `true`, the user can perform `update_build` on this resource. | diff --git a/lib/api/ci/jobs.rb b/lib/api/ci/jobs.rb index 47e4135db270b6..b2c186b64bc61a 100644 --- a/lib/api/ci/jobs.rb +++ b/lib/api/ci/jobs.rb @@ -122,10 +122,11 @@ class Jobs < ::API::Base requires :job_id, type: Integer, desc: 'The ID of a job', documentation: { example: 88 } end post ':id/jobs/:job_id/cancel', urgency: :low, feature_category: :continuous_integration do - authorize_update_builds! + # TODO + authorize_cancel_builds! build = find_build!(params[:job_id]) - authorize!(:update_build, build) + authorize!(:cancel_build, build) build.cancel diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index a7e0a551283203..f17474394e2489 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -397,6 +397,10 @@ def authorize_update_builds! authorize! :update_build, user_project end + def authorize_cancel_builds! + authorize! :cancel_build, user_project + end + def require_repository_enabled!(subject = :global) not_found!("Repository") unless user_project.feature_available?(:repository, current_user) end diff --git a/lib/gitlab/ci/status/build/cancelable.rb b/lib/gitlab/ci/status/build/cancelable.rb index 43fb5cdbbe62cd..b8c8cfa802c8ce 100644 --- a/lib/gitlab/ci/status/build/cancelable.rb +++ b/lib/gitlab/ci/status/build/cancelable.rb @@ -6,7 +6,7 @@ module Status module Build class Cancelable < Status::Extended def has_action? - can?(user, :update_build, subject) + can?(user, :cancel_build, subject) end def action_icon diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb index 6ab89daff822e7..ab92936440c5d7 100644 --- a/spec/policies/ci/build_policy_spec.rb +++ b/spec/policies/ci/build_policy_spec.rb @@ -110,6 +110,7 @@ end it 'enables update_build 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 end @@ -130,6 +131,7 @@ end it 'does not include ability to update build' do + expect(policy).to be_disallowed :cancel_build expect(policy).to be_disallowed :update_build end @@ -139,6 +141,7 @@ end it 'does not include ability to update build' do + expect(policy).to be_disallowed :cancel_build expect(policy).to be_disallowed :update_build end end @@ -150,6 +153,7 @@ end it 'includes ability to update build' do + expect(policy).to be_allowed :cancel_build expect(policy).to be_allowed :update_build end end @@ -162,6 +166,7 @@ end it 'does not include ability to update build' do + expect(policy).to be_disallowed :cancel_build expect(policy).to be_disallowed :update_build end end @@ -172,6 +177,7 @@ end it 'includes ability to update build' do + expect(policy).to be_allowed :cancel_build expect(policy).to be_allowed :update_build end end diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 5e79569e0c4244..6e52ff8ed4fa88 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -287,7 +287,7 @@ def set_access_level(access_level) it 'disallows all permissions except pipeline when the feature is disabled' do builds_permissions = [ - :create_build, :read_build, :update_build, :admin_build, :destroy_build, + :create_build, :read_build, :update_build, :cancel_build, :admin_build, :destroy_build, :create_pipeline_schedule, :read_pipeline_schedule_variables, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule, :create_environment, :read_environment, :update_environment, :admin_environment, :destroy_environment, :create_deployment, :read_deployment, :update_deployment, :admin_deployment, :destroy_deployment @@ -319,7 +319,7 @@ def set_access_level(access_level) let(:repository_permissions) do [ :create_pipeline, :update_pipeline, :cancel_pipeline, :admin_pipeline, :destroy_pipeline, - :create_build, :read_build, :update_build, :admin_build, :destroy_build, + :create_build, :read_build, :cancel_build, :update_build, :admin_build, :destroy_build, :create_pipeline_schedule, :read_pipeline_schedule, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule, :create_environment, :read_environment, :update_environment, :admin_environment, :destroy_environment, :create_cluster, :read_cluster, :update_cluster, :admin_cluster, diff --git a/spec/support/shared_examples/ci/deployable_policy_shared_examples.rb b/spec/support/shared_examples/ci/deployable_policy_shared_examples.rb index 73bdc094237958..1f164a66026b28 100644 --- a/spec/support/shared_examples/ci/deployable_policy_shared_examples.rb +++ b/spec/support/shared_examples/ci/deployable_policy_shared_examples.rb @@ -20,6 +20,7 @@ end it { expect(policy).not_to be_allowed :update_build } + it { expect(policy).not_to be_allowed :cancel_build } end end end -- GitLab From 361ccd22a8c3c3b978110cff680d8113a1f45eea Mon Sep 17 00:00:00 2001 From: Allison Browne Date: Wed, 1 Nov 2023 10:21:55 -0400 Subject: [PATCH 2/2] Fix a spec remove a comment --- ee/app/policies/ee/ci/deployable_policy.rb | 2 ++ lib/api/ci/jobs.rb | 1 - spec/graphql/types/permission_types/ci/job_spec.rb | 2 +- .../ci/deployable_policy_shared_examples_ee.rb | 6 ++++++ 4 files changed, 9 insertions(+), 2 deletions(-) diff --git a/ee/app/policies/ee/ci/deployable_policy.rb b/ee/app/policies/ee/ci/deployable_policy.rb index 27fc147080d0b6..20a3e8b37c4f85 100644 --- a/ee/app/policies/ee/ci/deployable_policy.rb +++ b/ee/app/policies/ee/ci/deployable_policy.rb @@ -22,6 +22,7 @@ module DeployablePolicy enable :jailbreak enable :update_commit_status enable :update_build + enable :cancel_build end # Authorizing the user to access to protected entities. @@ -30,6 +31,7 @@ module DeployablePolicy rule { ~can?(:jailbreak) & protected_environment }.policy do prevent :update_commit_status prevent :update_build + prevent :cancel_build prevent :erase_build end end diff --git a/lib/api/ci/jobs.rb b/lib/api/ci/jobs.rb index b2c186b64bc61a..250fe2494894fa 100644 --- a/lib/api/ci/jobs.rb +++ b/lib/api/ci/jobs.rb @@ -122,7 +122,6 @@ class Jobs < ::API::Base requires :job_id, type: Integer, desc: 'The ID of a job', documentation: { example: 88 } end post ':id/jobs/:job_id/cancel', urgency: :low, feature_category: :continuous_integration do - # TODO authorize_cancel_builds! build = find_build!(params[:job_id]) diff --git a/spec/graphql/types/permission_types/ci/job_spec.rb b/spec/graphql/types/permission_types/ci/job_spec.rb index e4bc54190701fc..238f086c7ee83c 100644 --- a/spec/graphql/types/permission_types/ci/job_spec.rb +++ b/spec/graphql/types/permission_types/ci/job_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Types::PermissionTypes::Ci::Job do it 'has expected permission fields' do expected_permissions = [ - :read_job_artifacts, :read_build, :update_build + :read_job_artifacts, :read_build, :update_build, :cancel_build ] expect(described_class).to have_graphql_fields(expected_permissions).only diff --git a/spec/support/shared_examples/ci/deployable_policy_shared_examples_ee.rb b/spec/support/shared_examples/ci/deployable_policy_shared_examples_ee.rb index b1057b3f67a9bc..10f334a6e23195 100644 --- a/spec/support/shared_examples/ci/deployable_policy_shared_examples_ee.rb +++ b/spec/support/shared_examples/ci/deployable_policy_shared_examples_ee.rb @@ -20,6 +20,12 @@ it_behaves_like 'protected environments access', direct_access: true end + describe '#cancel_build?' do + subject { user.can?(:cancel_build, job) } + + it_behaves_like 'protected environments access', direct_access: true + end + describe '#update_commit_status?' do subject { user.can?(:update_commit_status, job) } -- GitLab