Commit 581f3461 authored by Gabriel Mazetto's avatar Gabriel Mazetto

Merge branch 'allow-project-members-to-run-fork-pipelines-in-parent' into 'master'

Allow users to create pipelines for merge requests in the target project if the user has permission

See merge request gitlab-org/gitlab!35114
parents 46c072d4 b4c808ff
......@@ -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