Check more permissions when creating cross pipeline

ee/app/models/ee/commit_status_enums.rb

@@ -10,7 +10,8 @@ module EE
       override :failure_reasons
       def failure_reasons
         super.merge(protected_environment_failure: 1_000,
-                    insufficient_permissions: 1_001)
+                    insufficient_bridge_permissions: 1_001,
+                    invalid_bridge_trigger: 1_002)
       end
     end
   end

ee/app/presenters/ee/commit_status_presenter.rb

@@ -6,7 +6,8 @@ module EE
     prepended do
       EE_CALLOUT_FAILURE_MESSAGES = const_get(:CALLOUT_FAILURE_MESSAGES).merge(
         protected_environment_failure: 'The environment this job is deploying to is protected. Only users with permission may successfully run this job.',
-        insufficient_permissions: 'This job could not be executed because of insufficient permissions.'
+        insufficient_bridge_permissions: 'This job could not be executed because of insufficient permissions to create downstream pipeline.',
+        invalid_bridge_trigger: 'This job could not be executed because downstream pipeline trigger definition is not valid.'
       ).freeze
 
       EE::CommitStatusPresenter.private_constant :EE_CALLOUT_FAILURE_MESSAGES

ee/app/services/ci/create_cross_project_pipeline_service.rb

@@ -7,8 +7,12 @@ module Ci
     def execute(bridge)
       @bridge = bridge
 
+      unless target_project_exists?
+        return bridge.drop!(:invalid_bridge_trigger)
+      end
+
       unless can_create_cross_pipeline?
-        return bridge.drop!(:insufficient_permissions)
+        return bridge.drop!(:insufficient_bridge_permissions)
       end
 
       create_pipeline do |pipeline|
@@ -24,22 +28,34 @@ module Ci
 
     private
 
+    def target_project_exists?
+      target_project.present? &&
+        can?(current_user, :read_project, target_project)
+    end
+
     def can_create_cross_pipeline?
-      # TODO should we check update_pipeline in the first condition?
-      #
-      can?(current_user, :create_pipeline, project) &&
-        can?(current_user, :create_pipeline, target_project) &&
-        can?(@bridge.target_user, :create_pipeline, target_project)
+      can?(current_user, :update_pipeline, project) &&
+        can?(target_user, :create_pipeline, target_project)
     end
 
     def create_pipeline
       ::Ci::CreatePipelineService
-        .new(target_project, @bridge.target_user, ref: @bridge.target_ref)
+        .new(target_project, target_user, ref: target_ref)
         .execute(:pipeline, ignore_skip_ci: true) do |pipeline|
           yield pipeline
         end
     end
 
+    def target_user
+      strong_memoize(:target_user) { @bridge.target_user }
+    end
+
+    def target_ref
+      strong_memoize(:target_ref) do
+        @bridge.target_ref || target_project.default_branch
+      end
+    end
+
     def target_project
       strong_memoize(:target_project) do
         Project.find_by_full_path(@bridge.target_project_name)

ee/spec/services/ci/create_cross_project_pipeline_service_spec.rb

@@ -32,13 +32,53 @@ describe Ci::CreateCrossProjectPipelineService, '#execute' do
     upstream_project.add_developer(user)
   end
 
-  context 'when user does not have ability to create a pipeline' do
+  context 'when downstream project has not been found' do
+    let(:trigger) do
+      { trigger: { project: 'unknown/project' } }
+    end
+
+    it 'does not create a pipeline' do
+      expect { service.execute(bridge) }
+        .not_to change { Ci::Pipeline.count }
+    end
+
+    it 'changes pipeline bridge job status to failed' do
+      service.execute(bridge)
+
+      expect(bridge.reload).to be_failed
+      expect(bridge.failure_reason).to eq 'invalid_bridge_trigger'
+    end
+  end
+
+  context 'when user can not access downstream project' do
+    it 'does not create a new pipeline' do
+      expect { service.execute(bridge) }
+        .not_to change { Ci::Pipeline.count }
+    end
+
     it 'changes status of the bridge build' do
+      service.execute(bridge)
+
+      expect(bridge.reload).to be_failed
+      expect(bridge.failure_reason).to eq 'invalid_bridge_trigger'
+    end
+  end
+
+  context 'when user does not have access to create pipeline' do
+    before do
+      downstream_project.add_guest(user)
+    end
+
+    it 'does not create a new pipeline' do
       expect { service.execute(bridge) }
         .not_to change { Ci::Pipeline.count }
+    end
+
+    it 'changes status of the bridge build' do
+      service.execute(bridge)
 
-      expect(bridge).to be_failed
-      expect(bridge.failure_reason).to eq 'insufficient_permissions'
+      expect(bridge.reload).to be_failed
+      expect(bridge.failure_reason).to eq 'insufficient_bridge_permissions'
     end
   end
 
@@ -47,7 +87,7 @@ describe Ci::CreateCrossProjectPipelineService, '#execute' do
       downstream_project.add_developer(user)
     end
 
-    it 'creates a new pipeline' do
+    it 'creates only one new pipeline' do
       expect { service.execute(bridge) }
         .to change { Ci::Pipeline.count }.by(1)
     end
@@ -63,11 +103,23 @@ describe Ci::CreateCrossProjectPipelineService, '#execute' do
       expect(pipeline.source_bridge).to be_a ::Ci::Bridge
     end
 
-    it 'changes bridge status when downstream pipeline gets proceesed' do
+    it 'updates bridge status when downstream pipeline gets proceesed' do
       pipeline = service.execute(bridge)
 
       expect(pipeline.reload).to be_pending
       expect(bridge.reload).to be_success
     end
+
+    context 'when target ref is not specified' do
+      let(:trigger) do
+        { trigger: { project: downstream_project.full_path } }
+      end
+
+      it 'is using default branch name' do
+        pipeline = service.execute(bridge)
+
+        expect(pipeline.ref).to eq 'master'
+      end
+    end
   end
 end