diff options
| -rw-r--r-- | app/models/ci/pipeline.rb | 2 | ||||
| -rw-r--r-- | app/policies/ci/build_policy.rb | 11 | ||||
| -rw-r--r-- | app/policies/ci/pipeline_policy.rb | 19 | ||||
| -rw-r--r-- | lib/gitlab/user_access.rb | 4 | ||||
| -rw-r--r-- | spec/policies/ci/build_policy_spec.rb | 52 | ||||
| -rw-r--r-- | spec/policies/ci/pipeline_policy_spec.rb | 47 |
6 files changed, 93 insertions, 42 deletions
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index a46c1304667..06ce01095ea 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -169,7 +169,7 @@ module Ci Ability.allowed?(user, :create_pipeline, project) && if repo.ref_exists?("#{Gitlab::Git::BRANCH_REF_PREFIX}#{ref}") - access.can_merge_to_branch?(ref) + access.can_push_or_merge_to_branch?(ref) elsif repo.ref_exists?("#{Gitlab::Git::TAG_REF_PREFIX}#{ref}") access.can_create_tag?(ref) else diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index 2d7405dc240..85245528602 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -11,19 +11,20 @@ module Ci cannot! :"#{rule}_commit_status" unless can? :"#{rule}_build" end - if can?(:update_build) && protected_action? + if can?(:update_build) && !can_user_update? cannot! :update_build end end private - def protected_action? - return false unless build.action? + def can_user_update? + user_access.can_push_or_merge_to_branch?(build.ref) + end - !::Gitlab::UserAccess + def user_access + @user_access ||= ::Gitlab::UserAccess .new(user, project: build.project) - .can_merge_to_branch?(build.ref) end end end diff --git a/app/policies/ci/pipeline_policy.rb b/app/policies/ci/pipeline_policy.rb index 10aa2d3e72a..e71cc358353 100644 --- a/app/policies/ci/pipeline_policy.rb +++ b/app/policies/ci/pipeline_policy.rb @@ -1,7 +1,24 @@ module Ci class PipelinePolicy < BasePolicy + alias_method :pipeline, :subject + def rules - delegate! @subject.project + delegate! pipeline.project + + if can?(:update_pipeline) && !can_user_update? + cannot! :update_pipeline + end + end + + private + + def can_user_update? + user_access.can_push_or_merge_to_branch?(pipeline.ref) + end + + def user_access + @user_access ||= ::Gitlab::UserAccess + .new(user, project: pipeline.project) end end end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 3b922da7ced..bb05c474fa2 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -48,6 +48,10 @@ module Gitlab end end + def can_push_or_merge_to_branch?(ref) + can_push_to_branch?(ref) || can_merge_to_branch?(ref) + end + def can_push_to_branch?(ref) return false unless can_access_git? diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb index 48a139d4b83..b4c6f3141fb 100644 --- a/spec/policies/ci/build_policy_spec.rb +++ b/spec/policies/ci/build_policy_spec.rb @@ -96,55 +96,37 @@ describe Ci::BuildPolicy, :models do end end - describe 'rules for manual actions' do + describe 'rules for protected branch' do let(:project) { create(:project) } before do project.add_developer(user) - end - - context 'when branch build is assigned to is protected' do - before do - create(:protected_branch, :no_one_can_push, - name: 'some-ref', project: project) - end - context 'when build is a manual action' do - let(:build) do - create(:ci_build, :manual, ref: 'some-ref', pipeline: pipeline) - end - - it 'does not include ability to update build' do - expect(policies).not_to include :update_build - end - end + create(:protected_branch, branch_policy, + name: build.ref, project: project) + end - context 'when build is not a manual action' do - let(:build) do - create(:ci_build, ref: 'some-ref', pipeline: pipeline) - end + context 'when no one can push or merge to the branch' do + let(:branch_policy) { :no_one_can_push } - it 'includes ability to update build' do - expect(policies).to include :update_build - end + it 'does not include ability to update build' do + expect(policies).not_to include :update_build end end - context 'when branch build is assigned to is not protected' do - context 'when build is a manual action' do - let(:build) { create(:ci_build, :manual, pipeline: pipeline) } + context 'when developers can push to the branch' do + let(:branch_policy) { :developers_can_push } - it 'includes ability to update build' do - expect(policies).to include :update_build - end + it 'includes ability to update build' do + expect(policies).to include :update_build end + end - context 'when build is not a manual action' do - let(:build) { create(:ci_build, pipeline: pipeline) } + context 'when developers can push to the branch' do + let(:branch_policy) { :developers_can_merge } - it 'includes ability to update build' do - expect(policies).to include :update_build - end + it 'includes ability to update build' do + expect(policies).to include :update_build end end end diff --git a/spec/policies/ci/pipeline_policy_spec.rb b/spec/policies/ci/pipeline_policy_spec.rb new file mode 100644 index 00000000000..4ecf07a1bf2 --- /dev/null +++ b/spec/policies/ci/pipeline_policy_spec.rb @@ -0,0 +1,47 @@ +require 'spec_helper' + +describe Ci::PipelinePolicy, :models do + let(:user) { create(:user) } + let(:pipeline) { create(:ci_empty_pipeline, project: project) } + + let(:policies) do + described_class.abilities(user, pipeline).to_set + end + + describe 'rules' do + describe 'rules for protected branch' do + let(:project) { create(:project) } + + before do + project.add_developer(user) + + create(:protected_branch, branch_policy, + name: pipeline.ref, project: project) + end + + context 'when no one can push or merge to the branch' do + let(:branch_policy) { :no_one_can_push } + + it 'does not include ability to update pipeline' do + expect(policies).not_to include :update_pipeline + end + end + + context 'when developers can push to the branch' do + let(:branch_policy) { :developers_can_push } + + it 'includes ability to update pipeline' do + expect(policies).to include :update_pipeline + end + end + + context 'when developers can push to the branch' do + let(:branch_policy) { :developers_can_merge } + + it 'includes ability to update pipeline' do + expect(policies).to include :update_pipeline + end + end + end + end +end |