summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/controllers/projects/pipelines_controller.rb4
-rw-r--r--app/policies/ci/build_policy.rb9
-rw-r--r--app/policies/ci/pipeline_policy.rb8
-rw-r--r--app/policies/project_policy.rb4
-rw-r--r--changelogs/unreleased/jprovazn-pipeline-policy.yml6
-rw-r--r--spec/policies/ci/build_policy_spec.rb13
-rw-r--r--spec/policies/ci/pipeline_policy_spec.rb12
-rw-r--r--spec/policies/project_policy_spec.rb2
-rw-r--r--spec/serializers/pipeline_serializer_spec.rb21
-rw-r--r--spec/services/ci/retry_pipeline_service_spec.rb29
10 files changed, 96 insertions, 12 deletions
diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb
index 0b1b46944aa..f7417a6a5aa 100644
--- a/app/controllers/projects/pipelines_controller.rb
+++ b/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
diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb
index 808a81cbbf9..8b65758f3e8 100644
--- a/app/policies/ci/build_policy.rb
+++ b/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
diff --git a/app/policies/ci/pipeline_policy.rb b/app/policies/ci/pipeline_policy.rb
index 6363c382ff8..540e4235299 100644
--- a/app/policies/ci/pipeline_policy.rb
+++ b/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)
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index 5759b1a376f..99a0d7118f2 100644
--- a/app/policies/project_policy.rb
+++ b/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
diff --git a/changelogs/unreleased/jprovazn-pipeline-policy.yml b/changelogs/unreleased/jprovazn-pipeline-policy.yml
new file mode 100644
index 00000000000..2997c6c8667
--- /dev/null
+++ b/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
diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb
index 41cf2ef7225..9ca156deaa0 100644
--- a/spec/policies/ci/build_policy_spec.rb
+++ b/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
diff --git a/spec/policies/ci/pipeline_policy_spec.rb b/spec/policies/ci/pipeline_policy_spec.rb
index 48a8064c5fc..a5e509cfa0f 100644
--- a/spec/policies/ci/pipeline_policy_spec.rb
+++ b/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
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index 8b9c4ac0b4b..6609f5f7afd 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/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) }
diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb
index e88e86c2998..b741308e2c5 100644
--- a/spec/serializers/pipeline_serializer_spec.rb
+++ b/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)
diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb
index f1acfc48468..a73bd7a0268 100644
--- a/spec/services/ci/retry_pipeline_service_spec.rb
+++ b/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