Commit a8618c77 authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch 'fix-run-pipeline-in-target-project' into 'master'

Fix fork users cannot create pipelines in a fork project when parent project protects a feature branch

See merge request gitlab-org/gitlab!40724
parents 0e4147b4 a305d692
...@@ -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