Commit b4c808ff authored by Shinya Maeda's avatar Shinya Maeda Committed by Gabriel Mazetto

Allow to create pipelines for merge requests in the target project

This commit adds an extention to the pipeline creation service
parent 59b0dcc4
......@@ -48,12 +48,9 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
end
def set_pipeline_variables
@pipelines =
if can?(current_user, :read_pipeline, @merge_request.source_project)
@merge_request.all_pipelines
else
Ci::Pipeline.none
end
@pipelines = Ci::PipelinesForMergeRequestFinder
.new(@merge_request, current_user)
.execute
end
def close_merge_request_if_no_source_project
......
......@@ -32,13 +32,13 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
end
def pipelines
@pipelines = @merge_request.all_pipelines
@pipelines = Ci::PipelinesForMergeRequestFinder.new(@merge_request, current_user).execute
Gitlab::PollingInterval.set_header(response, interval: 10_000)
render json: {
pipelines: PipelineSerializer
.new(project: @project, current_user: @current_user)
.new(project: @project, current_user: current_user)
.represent(@pipelines)
}
end
......
......@@ -7,14 +7,29 @@ module Ci
EVENT = 'merge_request_event'
def initialize(merge_request)
def initialize(merge_request, current_user)
@merge_request = merge_request
@current_user = current_user
end
attr_reader :merge_request
attr_reader :merge_request, :current_user
delegate :commit_shas, :source_project, :source_branch, to: :merge_request
delegate :commit_shas, :target_project, :source_project, :source_branch, to: :merge_request
# Fetch all pipelines that the user can read.
def execute
if can_read_pipeline_in_target_project? && can_read_pipeline_in_source_project?
all
elsif can_read_pipeline_in_source_project?
all.for_project(merge_request.source_project)
elsif can_read_pipeline_in_target_project?
all.for_project(merge_request.target_project)
else
Ci::Pipeline.none
end
end
# Fetch all pipelines without permission check.
def all
strong_memoize(:all_pipelines) do
next Ci::Pipeline.none unless source_project
......@@ -35,13 +50,13 @@ module Ci
def pipelines_using_cte
cte = Gitlab::SQL::CTE.new(:shas, merge_request.all_commits.select(:sha))
source_pipelines_join = cte.table[:sha].eq(Ci::Pipeline.arel_table[:source_sha])
source_pipelines = filter_by(triggered_by_merge_request, cte, source_pipelines_join)
detached_pipelines = filter_by_sha(triggered_by_merge_request, cte)
source_sha_join = cte.table[:sha].eq(Ci::Pipeline.arel_table[:source_sha])
merged_result_pipelines = filter_by(triggered_by_merge_request, cte, source_sha_join)
detached_merge_request_pipelines = filter_by_sha(triggered_by_merge_request, cte)
pipelines_for_branch = filter_by_sha(triggered_for_branch, cte)
Ci::Pipeline.with(cte.to_arel) # rubocop: disable CodeReuse/ActiveRecord
.from_union([source_pipelines, detached_pipelines, pipelines_for_branch])
.from_union([merged_result_pipelines, detached_merge_request_pipelines, pipelines_for_branch])
end
def filter_by_sha(pipelines, cte)
......@@ -65,8 +80,7 @@ module Ci
# NOTE: this method returns only parent merge request pipelines.
# Child merge request pipelines have a different source.
def triggered_by_merge_request
source_project.ci_pipelines
.where(source: :merge_request_event, merge_request: merge_request) # rubocop: disable CodeReuse/ActiveRecord
Ci::Pipeline.triggered_by_merge_request(merge_request)
end
def triggered_for_branch
......@@ -86,5 +100,17 @@ module Ci
pipelines.order(Arel.sql(query)) # rubocop: disable CodeReuse/ActiveRecord
end
def can_read_pipeline_in_target_project?
strong_memoize(:can_read_pipeline_in_target_project) do
Ability.allowed?(current_user, :read_pipeline, target_project)
end
end
def can_read_pipeline_in_source_project?
strong_memoize(:can_read_pipeline_in_source_project) do
Ability.allowed?(current_user, :read_pipeline, source_project)
end
end
end
end
......@@ -269,6 +269,7 @@ module Ci
scope :for_ref, -> (ref) { where(ref: ref) }
scope :for_id, -> (id) { where(id: id) }
scope :for_iid, -> (iid) { where(iid: iid) }
scope :for_project, -> (project) { where(project: project) }
scope :created_after, -> (time) { where('ci_pipelines.created_at > ?', time) }
scope :created_before_id, -> (id) { where('ci_pipelines.id < ?', id) }
scope :before_pipeline, -> (pipeline) { created_before_id(pipeline.id).outside_pipeline_family(pipeline) }
......@@ -289,6 +290,15 @@ module Ci
)
end
# Returns the pipelines that associated with the given merge request.
# In general, please use `Ci::PipelinesForMergeRequestFinder` instead,
# for checking permission of the actor.
scope :triggered_by_merge_request, -> (merge_request) do
ci_sources.where(source: :merge_request_event,
merge_request: merge_request,
project: [merge_request.source_project, merge_request.target_project])
end
# Returns the pipelines in descending order (= newest first), optionally
# limited to a number of references.
#
......
......@@ -1028,6 +1028,10 @@ class MergeRequest < ApplicationRecord
target_project != source_project
end
def for_same_project?
target_project == source_project
end
# If the merge request closes any issues, save this information in the
# `MergeRequestsClosingIssues` model. This is a performance optimization.
# Calculating this information for a number of merge requests requires
......@@ -1293,7 +1297,7 @@ class MergeRequest < ApplicationRecord
def all_pipelines
strong_memoize(:all_pipelines) do
Ci::PipelinesForMergeRequestFinder.new(self).all
Ci::PipelinesForMergeRequestFinder.new(self, nil).all
end
end
......
......@@ -102,10 +102,6 @@ module MergeRequests
MergeRequests::CreatePipelineService.new(project, user).execute(merge_request)
end
def can_use_merge_request_ref?(merge_request)
!merge_request.for_fork?
end
def abort_auto_merge(merge_request, reason)
AutoMergeService.new(project, current_user).abort(merge_request, reason)
end
......
......@@ -9,7 +9,7 @@ module MergeRequests
end
def create_detached_merge_request_pipeline(merge_request)
Ci::CreatePipelineService.new(merge_request.source_project,
Ci::CreatePipelineService.new(pipeline_project(merge_request),
current_user,
ref: pipeline_ref_for_detached_merge_request_pipeline(merge_request))
.execute(:merge_request_event, merge_request: merge_request)
......@@ -31,13 +31,29 @@ module MergeRequests
private
def pipeline_project(merge_request)
if can_create_pipeline_in_target_project?(merge_request)
merge_request.target_project
else
merge_request.source_project
end
end
def pipeline_ref_for_detached_merge_request_pipeline(merge_request)
if can_use_merge_request_ref?(merge_request)
if can_create_pipeline_in_target_project?(merge_request)
merge_request.ref_path
else
merge_request.source_branch
end
end
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)
can?(current_user, :create_pipeline, merge_request.target_project)
else
merge_request.for_same_project?
end
end
end
end
......
......@@ -33,7 +33,7 @@ module AutoMerge
def available_for?(merge_request)
super do
merge_request.project.merge_trains_enabled? &&
!merge_request.for_fork? &&
(Gitlab::Ci::Features.allow_to_create_merge_request_pipelines_in_target_project?(merge_request.target_project) || merge_request.for_same_project?) &&
merge_request.actual_head_pipeline&.active?
end
end
......
......@@ -47,7 +47,7 @@ module AutoMerge
def available_for?(merge_request)
super do
merge_request.project.merge_trains_enabled? &&
!merge_request.for_fork? &&
(Gitlab::Ci::Features.allow_to_create_merge_request_pipelines_in_target_project?(merge_request.target_project) || merge_request.for_same_project?) &&
merge_request.actual_head_pipeline&.complete?
end
end
......
......@@ -7,11 +7,11 @@ module EE
override :execute
def execute(merge_request)
create_merge_request_pipeline_for(merge_request) || super
create_merged_result_pipeline_for(merge_request) || super
end
def create_merge_request_pipeline_for(merge_request)
return unless can_create_merge_request_pipeline_for?(merge_request)
def create_merged_result_pipeline_for(merge_request)
return unless can_create_merged_result_pipeline_for?(merge_request)
result = ::MergeRequests::MergeabilityCheckService.new(merge_request).execute(recheck: true)
......@@ -20,7 +20,7 @@ module EE
ref_payload = result.payload.fetch(:merge_ref_head)
::Ci::CreatePipelineService.new(merge_request.source_project, current_user,
::Ci::CreatePipelineService.new(merge_request.target_project, current_user,
ref: merge_request.merge_ref_path,
checkout_sha: ref_payload[:commit_id],
target_sha: ref_payload[:target_id],
......@@ -29,9 +29,9 @@ module EE
end
end
def can_create_merge_request_pipeline_for?(merge_request)
def can_create_merged_result_pipeline_for?(merge_request)
return false unless merge_request.project.merge_pipelines_enabled?
return false unless can_use_merge_request_ref?(merge_request)
return false unless can_create_pipeline_in_target_project?(merge_request)
can_create_pipeline_for?(merge_request)
end
......
......@@ -16,7 +16,7 @@ module MergeTrains
def validate(merge_request)
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('fork merge request is not supported') if merge_request.for_fork?
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?
success
end
......@@ -26,7 +26,7 @@ module MergeTrains
commit_message = commit_message(merge_request, previous_ref)
::MergeRequests::MergeToRefService.new(merge_request.project, merge_request.merge_user,
::MergeRequests::MergeToRefService.new(merge_request.target_project, merge_request.merge_user,
target_ref: merge_request.train_ref_path,
first_parent_ref: previous_ref,
commit_message: commit_message)
......@@ -39,7 +39,7 @@ module MergeTrains
end
def create_pipeline(merge_request, merge_status)
pipeline = ::Ci::CreatePipelineService.new(merge_request.source_project, merge_request.merge_user,
pipeline = ::Ci::CreatePipelineService.new(merge_request.target_project, merge_request.merge_user,
ref: merge_request.train_ref_path,
checkout_sha: merge_status[:commit_id],
target_sha: merge_status[:target_id],
......
......@@ -138,12 +138,15 @@ RSpec.describe AutoMerge::AddToMergeTrainWhenPipelineSucceedsService do
end
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
before do
allow(merge_request).to receive(:for_fork?) { true }
stub_feature_flags(ci_allow_to_create_merge_request_pipelines_in_target_project: false)
allow(merge_request).to receive(:for_same_project?) { false }
end
it { is_expected.to eq(false) }
end
end
context 'when the latest pipeline in the merge request is not active' do
before do
......
......@@ -373,12 +373,15 @@ RSpec.describe AutoMerge::MergeTrainService do
end
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
before do
allow(merge_request).to receive(:for_fork?) { true }
stub_feature_flags(ci_allow_to_create_merge_request_pipelines_in_target_project: false)
allow(merge_request).to receive(:for_same_project?) { false }
end
it { is_expected.to be_falsy }
end
end
context 'when the head pipeline of the merge request has not finished' do
before do
......
......@@ -57,12 +57,15 @@ RSpec.describe MergeTrains::CreatePipelineService do
end
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
before do
stub_feature_flags(ci_allow_to_create_merge_request_pipelines_in_target_project: false)
allow(merge_request).to receive(:for_fork?) { true }
end
it_behaves_like 'returns an error' do
let(:expected_reason) { 'fork merge request is not supported' }
let(:expected_reason) { 'fork merge request is not available for this project' }
end
end
end
......
......@@ -62,10 +62,8 @@ module API
# rubocop: enable CodeReuse/ActiveRecord
def merge_request_pipelines_with_access
authorize! :read_pipeline, user_project
mr = find_merge_request_with_access(params[:merge_request_iid])
mr.all_pipelines
::Ci::PipelinesForMergeRequestFinder.new(mr, current_user).execute
end
def automatically_mergeable?(merge_when_pipeline_succeeds, merge_request)
......@@ -384,8 +382,6 @@ module API
success Entities::Pipeline
end
post ':id/merge_requests/:merge_request_iid/pipelines' do
authorize! :create_pipeline, user_project
pipeline = ::MergeRequests::CreatePipelineService
.new(user_project, current_user, allow_duplicate: true)
.execute(find_merge_request_with_access(params[:merge_request_iid]))
......
......@@ -69,6 +69,10 @@ module Gitlab
def self.bulk_insert_on_create?(project)
::Feature.enabled?(:ci_bulk_insert_on_create, project, default_enabled: true)
end
def self.allow_to_create_merge_request_pipelines_in_target_project?(target_project)
::Feature.enabled?(:ci_allow_to_create_merge_request_pipelines_in_target_project, target_project)
end
end
end
end
......
......@@ -84,6 +84,111 @@ RSpec.describe 'Merge request > User sees pipelines', :js do
end
end
describe 'fork MRs in parent project', :sidekiq_inline do
include ProjectForksHelper
let_it_be(:parent_project) { create(:project, :public, :repository) }
let_it_be(:forked_project) { fork_project(parent_project, developer_in_fork, repository: true, target_project: create(:project, :public, :repository)) }
let_it_be(:developer_in_parent) { create(:user) }
let_it_be(:developer_in_fork) { create(:user) }
let_it_be(:reporter_in_parent_and_developer_in_fork) { create(:user) }
let(:merge_request) do
create(:merge_request, :with_detached_merge_request_pipeline,
source_project: forked_project, source_branch: 'feature',
target_project: parent_project, target_branch: 'master')
end
let(:config) do
{ test: { script: 'test', rules: [{ if: '$CI_MERGE_REQUEST_ID' }] } }
end
before_all do
parent_project.add_developer(developer_in_parent)
parent_project.add_reporter(reporter_in_parent_and_developer_in_fork)
forked_project.add_developer(developer_in_fork)
forked_project.add_developer(reporter_in_parent_and_developer_in_fork)
end
before do
stub_ci_pipeline_yaml_file(YAML.dump(config))
sign_in(actor)
end
after do
parent_project.all_pipelines.delete_all
forked_project.all_pipelines.delete_all
end
context 'when actor is a developer in parent project' do
let(:actor) { developer_in_parent }
it 'creates a pipeline in the parent project' do
visit project_merge_request_path(parent_project, merge_request)
create_merge_request_pipeline
check_pipeline(expected_project: parent_project)
check_head_pipeline(expected_project: parent_project)
end
end
context 'when actor is a developer in fork project' do
let(:actor) { developer_in_fork }
it 'creates a pipeline in the fork project' do
visit project_merge_request_path(parent_project, merge_request)
create_merge_request_pipeline
check_pipeline(expected_project: forked_project)
check_head_pipeline(expected_project: forked_project)
end
end
context 'when actor is a reporter in parent project and a developer in fork project' do
let(:actor) { reporter_in_parent_and_developer_in_fork }
it 'creates a pipeline in the fork project' do
visit project_merge_request_path(parent_project, merge_request)
create_merge_request_pipeline
check_pipeline(expected_project: forked_project)
check_head_pipeline(expected_project: forked_project)
end
end
def create_merge_request_pipeline
page.within('.merge-request-tabs') { click_link('Pipelines') }
click_button('Run Pipeline')
end
def check_pipeline(expected_project:)
page.within('.ci-table') do
expect(page).to have_selector('.commit', count: 2)
page.within(first('.commit')) do
page.within('.pipeline-tags') do
expect(page.find('.js-pipeline-url-link')[:href]).to include(expected_project.full_path)
expect(page).to have_content('detached')
end
page.within('.pipeline-triggerer') do
expect(page).to have_link(href: user_path(actor))
end
end
end
end
def check_head_pipeline(expected_project:)
page.within('.merge-request-tabs') { click_link('Overview') }
page.within('.ci-widget-content') do
expect(page.find('.pipeline-id')[:href]).to include(expected_project.full_path)
end
end
end
describe 'race condition' do
let(:project) { create(:project, :repository) }
let(:user) { create(:user) }
......
......@@ -3,11 +3,97 @@
require 'spec_helper'
RSpec.describe Ci::PipelinesForMergeRequestFinder do
describe '#execute' do
include ProjectForksHelper
subject { finder.execute }
let_it_be(:developer_in_parent) { create(:user) }
let_it_be(:developer_in_fork) { create(:user) }
let_it_be(:developer_in_both) { create(:user) }
let_it_be(:reporter_in_parent_and_developer_in_fork) { create(:user) }
let_it_be(:external_user) { create(:user) }
let_it_be(:parent_project) { create(:project, :repository, :private) }
let_it_be(:forked_project) { fork_project(parent_project, nil, repository: true, target_project: create(:project, :private, :repository)) }
let(:merge_request) do
create(:merge_request, source_project: forked_project, source_branch: 'feature',
target_project: parent_project, target_branch: 'master')
end
let!(:pipeline_in_parent) do
create(:ci_pipeline, :merged_result_pipeline, merge_request: merge_request, project: parent_project)
end
let!(:pipeline_in_fork) do
create(:ci_pipeline, :merged_result_pipeline, merge_request: merge_request, project: forked_project)
end
let(:finder) { described_class.new(merge_request, actor) }
before_all do
parent_project.add_developer(developer_in_parent)
parent_project.add_developer(developer_in_both)
parent_project.add_reporter(reporter_in_parent_and_developer_in_fork)
forked_project.add_developer(developer_in_fork)
forked_project.add_developer(developer_in_both)
forked_project.add_developer(reporter_in_parent_and_developer_in_fork)
end
context 'when actor has permission to read pipelines in both parent and forked projects' do
let(:actor) { developer_in_both }
it 'returns all pipelines' do
is_expected.to eq([pipeline_in_fork, pipeline_in_parent])
end
end
context 'when actor has permission to read pipelines in both parent and forked projects' do
let(:actor) { reporter_in_parent_and_developer_in_fork }
it 'returns all pipelines' do
is_expected.to eq([pipeline_in_fork, pipeline_in_parent])
end
end
context 'when actor has permission to read pipelines in the parent project only' do
let(:actor) { developer_in_parent }
it 'returns pipelines in parent' do
is_expected.to eq([pipeline_in_parent])
end
end
context 'when actor has permission to read pipelines in the forked project only' do
let(:actor) { developer_in_fork }
it 'returns pipelines in fork' do
is_expected.to eq([pipeline_in_fork])
end
end
context 'when actor does not have permission to read pipelines' do
let(:actor) { external_user }
it 'returns nothing' do
is_expected.to be_empty
end
end
context 'when actor is nil' do
let(:actor) { nil }
it 'returns nothing' do
is_expected.to be_empty
end
end
end
describe '#all' do
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.source_project }
subject { described_class.new(merge_request) }
subject { described_class.new(merge_request, nil) }
shared_examples 'returning pipelines with proper ordering' do
let!(:all_pipelines) do
......@@ -134,7 +220,7 @@ RSpec.describe Ci::PipelinesForMergeRequestFinder do
branch_pipeline_2,
branch_pipeline])
expect(described_class.new(merge_request_2).all)
expect(described_class.new(merge_request_2, nil).all)
.to eq([detached_merge_request_pipeline_2,
branch_pipeline_2,
branch_pipeline])
......
......@@ -3,9 +3,12 @@
require 'spec_helper'
RSpec.describe MergeRequests::CreatePipelineService do
include ProjectForksHelper
let_it_be(:project) { create(:project, :repository) }
let_it_be(:user) { create(:user) }
let(:service) { described_class.new(project, user, params) }
let(:service) { described_class.new(project, actor, params) }
let(:actor) { user }
let(:params) { {} }
before do
......@@ -26,11 +29,13 @@ RSpec.describe MergeRequests::CreatePipelineService do
let(:merge_request) do
create(:merge_request,
source_branch: 'feature',
source_project: project,
source_project: source_project,
target_branch: 'master',
target_project: project)
end
let(:source_project) { project }
it 'creates a detached merge request pipeline' do
expect { subject }.to change { Ci::Pipeline.count }.by(1)
......@@ -42,6 +47,50 @@ RSpec.describe MergeRequests::CreatePipelineService do
expect(subject.source).to eq('merge_request_event')
end
context 'with fork merge request' do
let_it_be(:forked_project) { fork_project(project, nil, repository: true, target_project: create(:project, :private, :repository)) }
let(:source_project) { forked_project }
context 'when actor has permission to create pipelines in target project' do
let(:actor) { user }
it 'creates a pipeline in the target project' do
expect(subject.project).to eq(project)
end
context 'when ci_allow_to_create_merge_request_pipelines_in_target_project feature flag is disabled' do
before do
stub_feature_flags(ci_allow_to_create_merge_request_pipelines_in_target_project: false)
end
it 'creates a pipeline in the source project' do
expect(subject.project).to eq(source_project)
end
end
end
context 'when actor has permission to create pipelines in forked project' do
let(:actor) { fork_user }
let(:fork_user) { create(:user) }
before do
source_project.add_developer(fork_user)
end
it 'creates a pipeline in the source project' do
expect(subject.project).to eq(source_project)
end
end
context 'when actor does not have permission to create pipelines' do
let(:actor) { create(:user) }
it 'returns nothing' do
expect(subject.full_error_messages).to include('Insufficient permissions to create a new pipeline')
end
end
end
context 'when service is called multiple times' do
it 'creates a pipeline once' do
expect do
......
......@@ -216,11 +216,12 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
target_project.add_maintainer(user)
end
it 'create legacy detached merge request pipeline for fork merge request' do
it 'create detached merge request pipeline for fork merge request' do
merge_request.reload
expect(merge_request.actual_head_pipeline)
.to be_legacy_detached_merge_request_pipeline
head_pipeline = merge_request.actual_head_pipeline
expect(head_pipeline).to be_detached_merge_request_pipeline
expect(head_pipeline.project).to eq(target_project)
end
end
......
......@@ -225,12 +225,13 @@ RSpec.describe MergeRequests::RefreshService do
context 'when service runs on forked project' do
let(:project) { @fork_project }
it 'creates legacy detached merge request pipeline for fork merge request', :sidekiq_might_not_need_inline do
it 'creates detached merge request pipeline for fork merge request', :sidekiq_inline do
expect { subject }
.to change { @fork_merge_request.pipelines_for_merge_request.count }.by(1)
expect(@fork_merge_request.pipelines_for_merge_request.first)
.to be_legacy_detached_merge_request_pipeline
merge_request_pipeline = @fork_merge_request.pipelines_for_merge_request.first
expect(merge_request_pipeline).to be_detached_merge_request_pipeline
expect(merge_request_pipeline.project).to eq(@project)
end
end
......
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