Commit a305d692 authored by Shinya Maeda's avatar Shinya Maeda Committed by Kamil Trzciński

Fix fork users cannot create pipelines in parent

This commit fixes the bug that fork users cannot
create pipelines when the parent project protects
all of the branches.

This also introduce a feature flag to disable the
feature on a speicfic project. It's already globally
enabled for a while.
parent 773adfdb
...@@ -60,6 +60,21 @@ module AutoMerge ...@@ -60,6 +60,21 @@ module AutoMerge
end end
end end
##
# NOTE: This method is to be removed when `disallow_to_create_merge_request_pipelines_in_target_project`
# feature flag is removed.
def self.can_add_to_merge_train?(merge_request)
if Gitlab::Ci::Features.disallow_to_create_merge_request_pipelines_in_target_project?(merge_request.target_project)
merge_request.for_same_project?
else
true
end
end
def can_add_to_merge_train?(merge_request)
self.class.can_add_to_merge_train?(merge_request)
end
private private
# Overridden in child classes # Overridden in child classes
......
...@@ -48,12 +48,18 @@ module MergeRequests ...@@ -48,12 +48,18 @@ module MergeRequests
end end
def can_create_pipeline_in_target_project?(merge_request) def can_create_pipeline_in_target_project?(merge_request)
if Gitlab::Ci::Features.allow_to_create_merge_request_pipelines_in_target_project?(merge_request.target_project) if Gitlab::Ci::Features.disallow_to_create_merge_request_pipelines_in_target_project?(merge_request.target_project)
can?(current_user, :create_pipeline, merge_request.target_project)
else
merge_request.for_same_project? merge_request.for_same_project?
else
can?(current_user, :create_pipeline, merge_request.target_project) &&
can_update_source_branch_in_target_project?(merge_request)
end end
end end
def can_update_source_branch_in_target_project?(merge_request)
::Gitlab::UserAccess.new(current_user, container: merge_request.target_project)
.can_update_branch?(merge_request.source_branch_ref)
end
end end
end end
......
---
title: Fix fork users cannot create pipelines in a fork project when parent project
protects all branches
merge_request: 40724
author:
type: fixed
---
name: ci_disallow_to_create_merge_request_pipelines_in_target_project
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40724
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/235119
group: group::progressive delivery
type: development
default_enabled: false
...@@ -33,7 +33,7 @@ module AutoMerge ...@@ -33,7 +33,7 @@ module AutoMerge
def available_for?(merge_request) def available_for?(merge_request)
super do super do
merge_request.project.merge_trains_enabled? && merge_request.project.merge_trains_enabled? &&
(Gitlab::Ci::Features.allow_to_create_merge_request_pipelines_in_target_project?(merge_request.target_project) || merge_request.for_same_project?) && can_add_to_merge_train?(merge_request) &&
merge_request.actual_head_pipeline&.active? merge_request.actual_head_pipeline&.active?
end end
end end
......
...@@ -47,7 +47,7 @@ module AutoMerge ...@@ -47,7 +47,7 @@ module AutoMerge
def available_for?(merge_request) def available_for?(merge_request)
super do super do
merge_request.project.merge_trains_enabled? && merge_request.project.merge_trains_enabled? &&
(Gitlab::Ci::Features.allow_to_create_merge_request_pipelines_in_target_project?(merge_request.target_project) || merge_request.for_same_project?) && can_add_to_merge_train?(merge_request) &&
merge_request.actual_head_pipeline&.complete? merge_request.actual_head_pipeline&.complete?
end end
end end
......
...@@ -16,7 +16,7 @@ module MergeTrains ...@@ -16,7 +16,7 @@ module MergeTrains
def validate(merge_request) def validate(merge_request)
return error('merge trains is disabled') unless merge_request.project.merge_trains_enabled? return error('merge trains is disabled') unless merge_request.project.merge_trains_enabled?
return error('merge request is not on a merge train') unless merge_request.on_train? return error('merge request is not on a merge train') unless merge_request.on_train?
return error('fork merge request is not available for this project') if !Gitlab::Ci::Features.allow_to_create_merge_request_pipelines_in_target_project?(merge_request.target_project) && merge_request.for_fork? return error('this merge request cannot be added to merge train') unless can_add_to_merge_train?(merge_request)
success success
end end
...@@ -50,5 +50,9 @@ module MergeTrains ...@@ -50,5 +50,9 @@ module MergeTrains
success(pipeline: pipeline) success(pipeline: pipeline)
end end
def can_add_to_merge_train?(merge_request)
AutoMerge::BaseService.can_add_to_merge_train?(merge_request)
end
end end
end end
...@@ -16,6 +16,7 @@ RSpec.describe AutoMerge::AddToMergeTrainWhenPipelineSucceedsService do ...@@ -16,6 +16,7 @@ RSpec.describe AutoMerge::AddToMergeTrainWhenPipelineSucceedsService do
let(:pipeline) { merge_request.reload.all_pipelines.first } let(:pipeline) { merge_request.reload.all_pipelines.first }
before do before do
stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false)
stub_feature_flags(disable_merge_trains: false) stub_feature_flags(disable_merge_trains: false)
stub_licensed_features(merge_trains: true, merge_pipelines: true) stub_licensed_features(merge_trains: true, merge_pipelines: true)
project.add_maintainer(user) project.add_maintainer(user)
...@@ -138,9 +139,9 @@ RSpec.describe AutoMerge::AddToMergeTrainWhenPipelineSucceedsService do ...@@ -138,9 +139,9 @@ RSpec.describe AutoMerge::AddToMergeTrainWhenPipelineSucceedsService do
end end
context 'when merge request is submitted from a forked project' do context 'when merge request is submitted from a forked project' do
context 'when ci_allow_to_create_merge_request_pipelines_in_target_project feature flag is disabled' do context 'when ci_disallow_to_create_merge_request_pipelines_in_target_project feature flag is enabled' do
before do before do
stub_feature_flags(ci_allow_to_create_merge_request_pipelines_in_target_project: false) stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: true)
allow(merge_request).to receive(:for_same_project?) { false } allow(merge_request).to receive(:for_same_project?) { false }
end end
......
...@@ -20,6 +20,7 @@ RSpec.describe AutoMerge::MergeTrainService do ...@@ -20,6 +20,7 @@ RSpec.describe AutoMerge::MergeTrainService do
allow(AutoMergeProcessWorker).to receive(:perform_async) { } allow(AutoMergeProcessWorker).to receive(:perform_async) { }
stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false)
stub_feature_flags(disable_merge_trains: false) stub_feature_flags(disable_merge_trains: false)
stub_licensed_features(merge_trains: true, merge_pipelines: true) stub_licensed_features(merge_trains: true, merge_pipelines: true)
project.update!(merge_pipelines_enabled: true) project.update!(merge_pipelines_enabled: true)
...@@ -373,9 +374,9 @@ RSpec.describe AutoMerge::MergeTrainService do ...@@ -373,9 +374,9 @@ RSpec.describe AutoMerge::MergeTrainService do
end end
context 'when merge request is submitted from a forked project' do context 'when merge request is submitted from a forked project' do
context 'when ci_allow_to_create_merge_request_pipelines_in_target_project feature flag is disabled' do context 'when ci_disallow_to_create_merge_request_pipelines_in_target_project feature flag is enabled' do
before do before do
stub_feature_flags(ci_allow_to_create_merge_request_pipelines_in_target_project: false) stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: true)
allow(merge_request).to receive(:for_same_project?) { false } allow(merge_request).to receive(:for_same_project?) { false }
end end
......
...@@ -9,6 +9,7 @@ RSpec.describe MergeTrains::CreatePipelineService do ...@@ -9,6 +9,7 @@ RSpec.describe MergeTrains::CreatePipelineService do
let(:previous_ref) { 'refs/heads/master' } let(:previous_ref) { 'refs/heads/master' }
before do before do
stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false)
stub_feature_flags(disable_merge_trains: false) stub_feature_flags(disable_merge_trains: false)
project.add_maintainer(maintainer) project.add_maintainer(maintainer)
stub_licensed_features(merge_pipelines: true, merge_trains: true) stub_licensed_features(merge_pipelines: true, merge_trains: true)
...@@ -57,14 +58,14 @@ RSpec.describe MergeTrains::CreatePipelineService do ...@@ -57,14 +58,14 @@ RSpec.describe MergeTrains::CreatePipelineService do
end end
context 'when merge request is submitted from a forked project' do context 'when merge request is submitted from a forked project' do
context 'when ci_allow_to_create_merge_request_pipelines_in_target_project feature flag is disabled' do context 'when ci_disallow_to_create_merge_request_pipelines_in_target_project feature flag is enabled' do
before do before do
stub_feature_flags(ci_allow_to_create_merge_request_pipelines_in_target_project: false) stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: true)
allow(merge_request).to receive(:for_fork?) { true } allow(merge_request).to receive(:for_same_project?) { false }
end end
it_behaves_like 'returns an error' do it_behaves_like 'returns an error' do
let(:expected_reason) { 'fork merge request is not available for this project' } let(:expected_reason) { 'this merge request cannot be added to merge train' }
end end
end end
end end
......
...@@ -56,8 +56,11 @@ module Gitlab ...@@ -56,8 +56,11 @@ module Gitlab
::Feature.enabled?(:ci_if_parenthesis_enabled, default_enabled: true) ::Feature.enabled?(:ci_if_parenthesis_enabled, default_enabled: true)
end end
def self.allow_to_create_merge_request_pipelines_in_target_project?(target_project) # NOTE: The feature flag `disallow_to_create_merge_request_pipelines_in_target_project`
::Feature.enabled?(:ci_allow_to_create_merge_request_pipelines_in_target_project, target_project, default_enabled: true) # is a safe switch to disable the feature for a parituclar project when something went wrong,
# therefore it's not supposed to be enabled by default.
def self.disallow_to_create_merge_request_pipelines_in_target_project?(target_project)
::Feature.enabled?(:ci_disallow_to_create_merge_request_pipelines_in_target_project, target_project)
end end
def self.ci_plan_needs_size_limit?(project) def self.ci_plan_needs_size_limit?(project)
......
...@@ -123,6 +123,10 @@ RSpec.describe 'Merge request > User sees pipelines', :js do ...@@ -123,6 +123,10 @@ RSpec.describe 'Merge request > User sees pipelines', :js do
context 'when actor is a developer in parent project' do context 'when actor is a developer in parent project' do
let(:actor) { developer_in_parent } let(:actor) { developer_in_parent }
before do
stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false)
end
it 'creates a pipeline in the parent project when user proceeds with the warning' do it 'creates a pipeline in the parent project when user proceeds with the warning' do
visit project_merge_request_path(parent_project, merge_request) visit project_merge_request_path(parent_project, merge_request)
......
...@@ -5,13 +5,14 @@ require 'spec_helper' ...@@ -5,13 +5,14 @@ require 'spec_helper'
RSpec.describe MergeRequests::CreatePipelineService do RSpec.describe MergeRequests::CreatePipelineService do
include ProjectForksHelper include ProjectForksHelper
let_it_be(:project) { create(:project, :repository) } let_it_be(:project, reload: true) { create(:project, :repository) }
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let(:service) { described_class.new(project, actor, params) } let(:service) { described_class.new(project, actor, params) }
let(:actor) { user } let(:actor) { user }
let(:params) { {} } let(:params) { {} }
before do before do
stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false)
project.add_developer(user) project.add_developer(user)
end end
...@@ -58,9 +59,27 @@ RSpec.describe MergeRequests::CreatePipelineService do ...@@ -58,9 +59,27 @@ RSpec.describe MergeRequests::CreatePipelineService do
expect(subject.project).to eq(project) expect(subject.project).to eq(project)
end end
context 'when ci_allow_to_create_merge_request_pipelines_in_target_project feature flag is disabled' do context 'when source branch is protected' do
context 'when actor does not have permission to update the protected branch in target project' do
let!(:protected_branch) { create(:protected_branch, name: '*', project: project) }
it 'creates a pipeline in the source project' do
expect(subject.project).to eq(source_project)
end
end
context 'when actor has permission to update the protected branch in target project' do
let!(:protected_branch) { create(:protected_branch, :developers_can_merge, name: '*', project: project) }
it 'creates a pipeline in the target project' do
expect(subject.project).to eq(project)
end
end
end
context 'when ci_disallow_to_create_merge_request_pipelines_in_target_project feature flag is enabled' do
before do before do
stub_feature_flags(ci_allow_to_create_merge_request_pipelines_in_target_project: false) stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: true)
end end
it 'creates a pipeline in the source project' do it 'creates a pipeline in the source project' do
......
...@@ -212,6 +212,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do ...@@ -212,6 +212,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
end end
before do before do
stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false)
target_project.add_developer(assignee) target_project.add_developer(assignee)
target_project.add_maintainer(user) target_project.add_maintainer(user)
end end
......
...@@ -225,6 +225,10 @@ RSpec.describe MergeRequests::RefreshService do ...@@ -225,6 +225,10 @@ RSpec.describe MergeRequests::RefreshService do
context 'when service runs on forked project' do context 'when service runs on forked project' do
let(:project) { @fork_project } let(:project) { @fork_project }
before do
stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false)
end
it 'creates detached merge request pipeline for fork merge request', :sidekiq_inline do it 'creates detached merge request pipeline for fork merge request', :sidekiq_inline do
expect { subject } expect { subject }
.to change { @fork_merge_request.pipelines_for_merge_request.count }.by(1) .to change { @fork_merge_request.pipelines_for_merge_request.count }.by(1)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment