From 9b58b8e363fd388635385085c58be3d4637eaa45 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 6 Nov 2017 22:20:44 +0900 Subject: [PATCH] Do not allow jobs to be erased --- app/controllers/projects/jobs_controller.rb | 5 +++ app/models/ci/build.rb | 4 ++ app/policies/ci/build_policy.rb | 5 +++ app/serializers/build_details_entity.rb | 2 +- app/views/projects/jobs/show.html.haml | 2 +- lib/api/jobs.rb | 2 +- lib/api/v3/builds.rb | 2 +- spec/policies/ci/build_policy_spec.rb | 42 +++++++++++++++++++++ 8 files changed, 60 insertions(+), 4 deletions(-) diff --git a/app/controllers/projects/jobs_controller.rb b/app/controllers/projects/jobs_controller.rb index 1b985ea9763..fd6708666c3 100644 --- a/app/controllers/projects/jobs_controller.rb +++ b/app/controllers/projects/jobs_controller.rb @@ -5,6 +5,7 @@ class Projects::JobsController < Projects::ApplicationController only: [:index, :show, :status, :raw, :trace] before_action :authorize_update_build!, except: [:index, :show, :status, :raw, :trace, :cancel_all] + before_action :authorize_erase_build!, only: [:erase] layout 'project' @@ -131,6 +132,10 @@ class Projects::JobsController < Projects::ApplicationController return access_denied! unless can?(current_user, :update_build, build) end + def authorize_erase_build! + return access_denied! unless can?(current_user, :erase_build, build) + end + def build @build ||= project.builds.find(params[:id]) .present(current_user: current_user) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 6ca46ae89c1..0d992c4c01f 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -192,6 +192,10 @@ module Ci project.build_timeout end + def owned_by?(current_user) + user == current_user + end + # A slugified version of the build ref, suitable for inclusion in URLs and # domain names. Rules: # diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index 984e5482288..f158cda2f0e 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -10,6 +10,11 @@ module Ci end end + condition(:owner_of_build) do + can?(:developer_access) && @subject.owned_by?(@user) + end + rule { protected_ref }.prevent :update_build + rule { can?(:master_access) | owner_of_build }.enable :erase_build end end diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb index 8c89eea607f..69d46f5ec14 100644 --- a/app/serializers/build_details_entity.rb +++ b/app/serializers/build_details_entity.rb @@ -6,7 +6,7 @@ class BuildDetailsEntity < JobEntity expose :pipeline, using: PipelineEntity expose :erased_by, if: -> (*) { build.erased? }, using: UserEntity - expose :erase_path, if: -> (*) { build.erasable? && can?(current_user, :update_build, project) } do |build| + expose :erase_path, if: -> (*) { build.erasable? && can?(current_user, :erase_build, build) } do |build| erase_project_job_path(project, build) end diff --git a/app/views/projects/jobs/show.html.haml b/app/views/projects/jobs/show.html.haml index ce0e3872240..2abd2c9e652 100644 --- a/app/views/projects/jobs/show.html.haml +++ b/app/views/projects/jobs/show.html.haml @@ -71,7 +71,7 @@ class: 'js-raw-link-controller has-tooltip controllers-buttons' do = icon('file-text-o') - - if can?(current_user, :update_build, @project) && @build.erasable? + - if @build.erasable? && can?(current_user, :erase_build, @build) = link_to erase_project_job_path(@project, @build), method: :post, data: { confirm: 'Are you sure you want to erase this build?', placement: 'top', container: 'body' }, diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb index 3c1c412ba42..a116ab3c9bd 100644 --- a/lib/api/jobs.rb +++ b/lib/api/jobs.rb @@ -136,7 +136,7 @@ module API authorize_update_builds! build = find_build!(params[:job_id]) - authorize!(:update_build, build) + authorize!(:erase_build, build) return forbidden!('Job is not erasable!') unless build.erasable? build.erase(erased_by: current_user) diff --git a/lib/api/v3/builds.rb b/lib/api/v3/builds.rb index f493fd7c7ec..fa0bef39602 100644 --- a/lib/api/v3/builds.rb +++ b/lib/api/v3/builds.rb @@ -169,7 +169,7 @@ module API authorize_update_builds! build = get_build!(params[:build_id]) - authorize!(:update_build, build) + authorize!(:erase_build, build) return forbidden!('Build is not erasable!') unless build.erasable? build.erase(erased_by: current_user) diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb index 8e1bc3d1543..e5d5e1017cd 100644 --- a/spec/policies/ci/build_policy_spec.rb +++ b/spec/policies/ci/build_policy_spec.rb @@ -150,5 +150,47 @@ describe Ci::BuildPolicy do end end end + + # TODO: Finish spec + describe 'rules for erase build' do + let(:project) { create(:project, :repository) } + let(:another_user) { create(:user) } + + context 'when developer created a build' do + before do + project.add_developer(user) + end + + context 'when the build was created by the user' do + let(:build) { create(:ci_build, user: user) } + + it { expect(policy).to be_allowed :erase_build } + end + + context 'when the build was created by others' do + let(:build) { create(:ci_build, user: another_user) } + + it { expect(policy).to be_disallowed :erase_build } + end + end + + context 'when master erases a build' do + before do + project.add_master(user) + end + + context 'when the build was created by the user' do + let(:build) { create(:ci_build, user: user) } + + it { expect(policy).to be_allowed :erase_build } + end + + context 'when the build was created by others' do + let(:build) { create(:ci_build, user: another_user) } + + it { expect(policy).to be_allowed :erase_build } + end + end + end end end