Merge branch 'jprovazn-pipeline-policy' into 'master'

Enable update_(build|pipeline) for maintainers

Closes #45905

See merge request gitlab-org/gitlab-ce!18735

app/controllers/projects/pipelines_controller.rb

@@ -181,4 +181,8 @@ class Projects::PipelinesController < Projects::ApplicationController
     # Also see https://gitlab.com/gitlab-org/gitlab-ce/issues/42343
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42339')
   end
+
+  def authorize_update_pipeline!
+    return access_denied! unless can?(current_user, :update_pipeline, @pipeline)
+  end
 end

app/policies/ci/build_policy.rb

@@ -14,11 +14,20 @@ module Ci
       @subject.triggered_by?(@user)
     end
 
+    condition(:branch_allows_maintainer_push) do
+      @subject.project.branch_allows_maintainer_push?(@user, @subject.ref)
+    end
+
     rule { protected_ref }.policy do
       prevent :update_build
       prevent :erase_build
     end
 
     rule { can?(:admin_build) | (can?(:update_build) & owner_of_job) }.enable :erase_build
+
+    rule { can?(:public_access) & branch_allows_maintainer_push }.policy do
+      enable :update_build
+      enable :update_commit_status
+    end
   end
 end

app/policies/ci/pipeline_policy.rb

@@ -4,8 +4,16 @@ module Ci
 
     condition(:protected_ref) { ref_protected?(@user, @subject.project, @subject.tag?, @subject.ref) }
 
+    condition(:branch_allows_maintainer_push) do
+      @subject.project.branch_allows_maintainer_push?(@user, @subject.ref)
+    end
+
     rule { protected_ref }.prevent :update_pipeline
 
+    rule { can?(:public_access) & branch_allows_maintainer_push }.policy do
+      enable :update_pipeline
+    end
+
     def ref_protected?(user, project, tag, ref)
       access = ::Gitlab::UserAccess.new(user, project: project)
 

app/policies/project_policy.rb

@@ -76,7 +76,7 @@ class ProjectPolicy < BasePolicy
   condition(:request_access_enabled, scope: :subject, score: 0) { project.request_access_enabled }
 
   desc "Has merge requests allowing pushes to user"
-  condition(:has_merge_requests_allowing_pushes, scope: :subject) do
+  condition(:has_merge_requests_allowing_pushes) do
     project.merge_requests_allowing_push_to_user(user).any?
   end
 
@@ -354,9 +354,7 @@ class ProjectPolicy < BasePolicy
   # to run pipelines for the branches they have access to.
   rule { can?(:public_access) & has_merge_requests_allowing_pushes }.policy do
     enable :create_build
-    enable :update_build
     enable :create_pipeline
-    enable :update_pipeline
   end
 
   rule do

changelogs/unreleased/jprovazn-pipeline-policy.yml

@@ -0,0 +1,6 @@
+---
+title: Allow maintainers to retry pipelines on forked projects (if allowed in merge
+  request)
+merge_request:
+author:
+type: fixed

spec/policies/ci/build_policy_spec.rb

@@ -94,6 +94,19 @@ describe Ci::BuildPolicy do
           end
         end
       end
+
+      context 'when maintainer is allowed to push to pipeline branch' do
+        let(:project) { create(:project, :public) }
+        let(:owner) { user }
+
+        it 'enables update_build if user is maintainer' do
+          allow_any_instance_of(Project).to receive(:empty_repo?).and_return(false)
+          allow_any_instance_of(Project).to receive(:branch_allows_maintainer_push?).and_return(true)
+
+          expect(policy).to be_allowed :update_build
+          expect(policy).to be_allowed :update_commit_status
+        end
+      end
     end
 
     describe 'rules for protected ref' do

spec/policies/ci/pipeline_policy_spec.rb

@@ -62,5 +62,17 @@ describe Ci::PipelinePolicy, :models do
         end
       end
     end
+
+    context 'when maintainer is allowed to push to pipeline branch' do
+      let(:project) { create(:project, :public) }
+      let(:owner) { user }
+
+      it 'enables update_pipeline if user is maintainer' do
+        allow_any_instance_of(Project).to receive(:empty_repo?).and_return(false)
+        allow_any_instance_of(Project).to receive(:branch_allows_maintainer_push?).and_return(true)
+
+        expect(policy).to be_allowed :update_pipeline
+      end
+    end
   end
 end

spec/policies/project_policy_spec.rb

@@ -404,7 +404,7 @@ describe ProjectPolicy do
       )
     end
     let(:maintainer_abilities) do
-      %w(create_build update_build create_pipeline update_pipeline)
+      %w(create_build create_pipeline)
     end
 
     subject { described_class.new(user, project) }

spec/serializers/pipeline_serializer_spec.rb

@@ -114,7 +114,9 @@ describe PipelineSerializer do
         Gitlab::GitalyClient.reset_counts
       end
 
-      shared_examples 'no N+1 queries' do
+      context 'with the same ref' do
+        let(:ref) { 'feature' }
+
         it 'verifies number of queries', :request_store do
           recorded = ActiveRecord::QueryRecorder.new { subject }
 
@@ -123,12 +125,6 @@ describe PipelineSerializer do
         end
       end
 
-      context 'with the same ref' do
-        let(:ref) { 'feature' }
-
-        it_behaves_like 'no N+1 queries'
-      end
-
       context 'with different refs' do
         def ref
           @sequence ||= 0
@@ -136,7 +132,16 @@ describe PipelineSerializer do
           "feature-#{@sequence}"
         end
 
-        it_behaves_like 'no N+1 queries'
+        it 'verifies number of queries', :request_store do
+          recorded = ActiveRecord::QueryRecorder.new { subject }
+
+          # For each ref there is a permission check if maintainer can update
+          # pipeline. With the same ref this check is cached but if refs are
+          # different then there is an extra query per ref
+          # https://gitlab.com/gitlab-org/gitlab-ce/issues/46368
+          expect(recorded.count).to be_within(1).of(51)
+          expect(recorded.cached_count).to eq(0)
+        end
       end
 
       def create_pipeline(status)

spec/services/ci/retry_pipeline_service_spec.rb

@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe Ci::RetryPipelineService, '#execute' do
+  include ProjectForksHelper
+
   let(:user) { create(:user) }
   let(:project) { create(:project) }
   let(:pipeline) { create(:ci_pipeline, project: project) }
@@ -266,6 +268,33 @@ describe Ci::RetryPipelineService, '#execute' do
     end
   end
 
+  context 'when maintainer is allowed to push to forked project' do
+    let(:user) { create(:user) }
+    let(:project) { create(:project, :public) }
+    let(:forked_project) { fork_project(project) }
+    let(:pipeline) { create(:ci_pipeline, project: forked_project, ref: 'fixes') }
+
+    before do
+      project.add_master(user)
+      create(:merge_request,
+        source_project: forked_project,
+        target_project: project,
+        source_branch: 'fixes',
+        allow_maintainer_to_push: true)
+      create_build('rspec 1', :failed, 1)
+    end
+
+    it 'allows to retry failed pipeline' do
+      allow_any_instance_of(Project).to receive(:fetch_branch_allows_maintainer_push?).and_return(true)
+      allow_any_instance_of(Project).to receive(:empty_repo?).and_return(false)
+
+      service.execute(pipeline)
+
+      expect(build('rspec 1')).to be_pending
+      expect(pipeline.reload).to be_running
+    end
+  end
+
   def statuses
     pipeline.reload.statuses
   end