Consistently check permission for creating pipelines,

updating builds and updating pipelines. We check against
being able to merge or push if the ref is protected.

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