Use author to run subscribed pipeline

Merge branch 'security-aw-fix-pipeline-sub-vulns' into 'master'

See merge request gitlab-org/security/gitlab!2361

Changelog: security

config/gitlab_loose_foreign_keys.yml

@@ -144,6 +144,9 @@ ci_subscriptions_projects:
   - table: projects
     column: upstream_project_id
     on_delete: async_delete
+  - table: users
+    column: author_id
+    on_delete: async_delete
 ci_triggers:
   - table: users
     column: owner_id

db/migrate/20220329130330_add_author_to_ci_subscriptions_projects.rb

+# frozen_string_literal: true
+
+class AddAuthorToCiSubscriptionsProjects < Gitlab::Database::Migration[2.0]
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_ci_subscriptions_projects_author_id'
+
+  def up
+    unless column_exists?(:ci_subscriptions_projects, :author_id)
+      add_column :ci_subscriptions_projects, :author_id, :bigint
+    end
+
+    add_concurrent_index :ci_subscriptions_projects, :author_id, name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :ci_subscriptions_projects, INDEX_NAME
+    remove_column :ci_subscriptions_projects, :author_id
+  end
+end

db/schema_migrations/20220329130330

+8726707f9f4bb8d256886c592b6a11ba8487de24f5340c837800f5ce0c32df9d
\ No newline at end of file

db/structure.sql

@@ -13253,7 +13253,8 @@ ALTER SEQUENCE ci_stages_id_seq OWNED BY ci_stages.id;
 CREATE TABLE ci_subscriptions_projects (
     id bigint NOT NULL,
     downstream_project_id bigint NOT NULL,
-    upstream_project_id bigint NOT NULL
+    upstream_project_id bigint NOT NULL,
+    author_id bigint
 );
 CREATE SEQUENCE ci_subscriptions_projects_id_seq
@@ -27752,6 +27753,8 @@ CREATE INDEX index_ci_stages_on_pipeline_id_and_position ON ci_stages USING btre
 CREATE INDEX index_ci_stages_on_project_id ON ci_stages USING btree (project_id);
+CREATE INDEX index_ci_subscriptions_projects_author_id ON ci_subscriptions_projects USING btree (author_id);
+
 CREATE INDEX index_ci_subscriptions_projects_on_upstream_project_id ON ci_subscriptions_projects USING btree (upstream_project_id);
 CREATE UNIQUE INDEX index_ci_subscriptions_projects_unique_subscription ON ci_subscriptions_projects USING btree (downstream_project_id, upstream_project_id);

ee/app/controllers/projects/subscriptions_controller.rb

@@ -11,7 +11,10 @@ class Projects::SubscriptionsController < Projects::ApplicationController
   urgency :low
 
   def create
-    subscription = project.upstream_project_subscriptions.create(upstream_project: upstream_project)
+    subscription = project.upstream_project_subscriptions.create(
+      upstream_project: upstream_project,
+      author: current_user
+    )
 
     if subscription.persisted?
       flash[:notice] = _('Subscription successfully created.')

ee/app/models/ci/subscriptions/project.rb

@@ -13,6 +13,7 @@ class Project < Ci::ApplicationRecord
 
       belongs_to :downstream_project, class_name: '::Project', optional: false
       belongs_to :upstream_project, class_name: '::Project', optional: false
+      belongs_to :author, class_name: '::User'
 
       validates :upstream_project_id, uniqueness: { scope: :downstream_project_id }
 
@@ -20,6 +21,10 @@ class Project < Ci::ApplicationRecord
         errors.add(:upstream_project, 'needs to be public') unless upstream_public?
       end
 
+      def self.with_downstream_and_author
+        preload(:author, :downstream_project)
+      end
+
       private
 
       def upstream_public?

ee/app/models/ee/project.rb

@@ -97,7 +97,6 @@ def lock_for_confirmation!(id)
       has_many :upstream_project_subscriptions, class_name: 'Ci::Subscriptions::Project', foreign_key: :downstream_project_id, inverse_of: :downstream_project
       has_many :upstream_projects, class_name: 'Project', through: :upstream_project_subscriptions, source: :upstream_project, disable_joins: true
       has_many :downstream_project_subscriptions, class_name: 'Ci::Subscriptions::Project', foreign_key: :upstream_project_id, inverse_of: :upstream_project
-      has_many :downstream_projects, class_name: 'Project', through: :downstream_project_subscriptions, source: :downstream_project, disable_joins: true
 
       has_many :incident_management_oncall_schedules, class_name: 'IncidentManagement::OncallSchedule', inverse_of: :project
       has_many :incident_management_oncall_rotations, class_name: 'IncidentManagement::OncallRotation', through: :incident_management_oncall_schedules, source: :rotations

ee/app/services/ci/trigger_downstream_subscription_service.rb

@@ -3,12 +3,23 @@
 module Ci
   class TriggerDownstreamSubscriptionService < ::BaseService
     def execute(pipeline)
-      pipeline.project.downstream_projects.each do |downstream_project|
-        ::Ci::CreatePipelineService.new(downstream_project, pipeline.user, ref: downstream_project.default_branch)
-          .execute(:pipeline) do |downstream_pipeline|
+      subscriptions(pipeline).each do |subscription|
+        # Subscription's author was introduced afterwards. When not set we default
+        # to the downstream project's creator.
+        ::Ci::CreatePipelineService.new(
+          subscription.downstream_project,
+          (subscription.author || subscription.downstream_project.creator),
+          ref: subscription.downstream_project.default_branch
+        ).execute(:pipeline) do |downstream_pipeline|
           downstream_pipeline.build_source_project(source_project: pipeline.project)
         end
       end
     end
+
+    private
+
+    def subscriptions(pipeline)
+      pipeline.project.downstream_project_subscriptions.with_downstream_and_author
+    end
   end
 end

ee/spec/controllers/projects/subscriptions_controller_spec.rb

@@ -40,6 +40,9 @@
             context 'when subscription count is below the limit' do
               it 'creates a new subscription' do
                 expect { post_create }.to change { project.upstream_project_subscriptions.count }.from(0).to(1)
+
+                new_subscription = project.upstream_project_subscriptions.first
+                expect(new_subscription.author).to eq(user)
               end
 
               it 'sets the flash' do

ee/spec/factories/ci/subscriptions/projects.rb

@@ -4,5 +4,6 @@
   factory :ci_subscriptions_project, class: 'Ci::Subscriptions::Project' do
     downstream_project factory: :project
     upstream_project factory: [:project, :public]
+    author factory: :user
   end
 end

ee/spec/models/ci/pipeline_spec.rb

@@ -362,11 +362,12 @@
 
     context 'when pipeline project has downstream subscriptions' do
       let(:downstream_project) { create(:project) }
-      let(:project) { create(:project, :public, downstream_projects: [downstream_project]) }
+      let(:project) { create(:project, :public) }
       let(:pipeline) { create(:ci_empty_pipeline, project: project) }
 
       context 'when pipeline runs on a tag' do
         before do
+          create(:ci_subscriptions_project, downstream_project: downstream_project, upstream_project: project)
           pipeline.update!(tag: true)
         end
 

ee/spec/models/ci/subscriptions/project_spec.rb

@@ -9,10 +9,20 @@
   describe 'Relations' do
     it { is_expected.to belong_to(:downstream_project).required }
     it { is_expected.to belong_to(:upstream_project).required }
+    it { is_expected.to belong_to(:author) }
   end
 
   it_behaves_like 'includes Limitable concern' do
-    subject { build(:ci_subscriptions_project, upstream_project: upstream_project, downstream_project: downstream_project) }
+    let_it_be_with_reload(:user) { create(:user) }
+
+    subject do
+      build(
+        :ci_subscriptions_project,
+        upstream_project: upstream_project,
+        downstream_project: downstream_project,
+        author: user
+      )
+    end
   end
 
   describe 'Validations' do
@@ -40,4 +50,30 @@
       let!(:model) { create(:ci_subscriptions_project, upstream_project: parent) }
     end
   end
+
+  context 'loose foreign key on ci_subscriptions_projects.author_id' do
+    it_behaves_like 'cleanup by a loose foreign key' do
+      let!(:parent) { create(:user) }
+      let!(:model) { create(:ci_subscriptions_project, author: parent) }
+    end
+  end
+
+  describe '.with_downstream_and_author' do
+    it 'includes the author and downstream project' do
+      author = create(:user)
+      subscription = create(
+        :ci_subscriptions_project,
+        upstream_project: upstream_project,
+        downstream_project: downstream_project,
+        author: author
+      )
+
+      refound_subscription = described_class.where(id: subscription.id).with_downstream_and_author.first
+
+      expect do
+        expect(refound_subscription.author).to eq(author)
+        expect(refound_subscription.downstream_project).to eq(downstream_project)
+      end.not_to exceed_query_limit(0)
+    end
+  end
 end

ee/spec/models/project_spec.rb

@@ -47,7 +47,6 @@
     it { is_expected.to have_many(:upstream_project_subscriptions) }
     it { is_expected.to have_many(:upstream_projects) }
     it { is_expected.to have_many(:downstream_project_subscriptions) }
-    it { is_expected.to have_many(:downstream_projects) }
     it { is_expected.to have_many(:vulnerability_historical_statistics).class_name('Vulnerabilities::HistoricalStatistic') }
     it { is_expected.to have_many(:vulnerability_remediations).class_name('Vulnerabilities::Remediation') }
     it { is_expected.to have_many(:vulnerability_reads).class_name('Vulnerabilities::Read') }
@@ -3114,21 +3113,13 @@ def stub_default_url_options(host)
     end
   end
 
-  describe '#downstream_projects' do
-    it 'returns the downstream projects' do
-      downstream_project = create(:project, :public)
-      primary_project = create(:project, :public, downstream_projects: [downstream_project])
-
-      with_cross_joins_prevented do
-        expect(primary_project.downstream_projects).to eq([downstream_project])
-      end
-    end
-  end
-
   describe '#downstream_projects_count' do
     it 'returns the downstream projects count' do
+      primary_project = create(:project, :public)
       downstream_projects = create_list(:project, 2, :public)
-      primary_project = create(:project, :public, downstream_projects: downstream_projects)
+      downstream_projects.each do |project|
+        create(:ci_subscriptions_project, downstream_project: project, upstream_project: primary_project)
+      end
 
       with_cross_joins_prevented do
         expect(primary_project.downstream_projects_count).to eq(2)

ee/spec/services/ci/trigger_downstream_subscription_service_spec.rb

@@ -6,29 +6,75 @@
   describe '#execute' do
     subject(:execute) { described_class.new(pipeline.project, pipeline.user).execute(pipeline) }
 
+    let!(:pipeline) { create(:ci_pipeline, project: upstream_project) }
     let(:upstream_project) { create(:project, :public) }
-    let(:pipeline) { create(:ci_pipeline, project: upstream_project, user: create(:user)) }
 
     before do
       stub_ci_pipeline_yaml_file(YAML.dump(job_name: { script: 'echo 1' }))
     end
 
     context 'when pipeline project has downstream projects' do
-      let(:downstream_project) { create(:project, :repository, upstream_projects: [upstream_project]) }
+      let(:downstream_project) { create(:project, :repository) }
 
-      before do
-        downstream_project.add_developer(pipeline.user)
+      let!(:subscription) do
+        create(
+          :ci_subscriptions_project,
+          upstream_project: upstream_project,
+          downstream_project: downstream_project,
+          author: author
+        )
       end
 
-      it 'associates the downstream pipeline with the upstream project' do
-        expect { execute }
-          .to change { Ci::Sources::Project.count }.from(0).to(1)
-          .and change { Ci::Pipeline.count }.from(1).to(2)
+      shared_examples 'creates a downstream pipeline' do
+        it 'associates the downstream pipeline with the upstream project' do
+          expect { execute }
+            .to change { Ci::Sources::Project.count }.from(0).to(1)
+            .and change { Ci::Pipeline.count }.from(1).to(2)
 
-        project_source = Ci::Sources::Project.last
-        new_pipeline = Ci::Pipeline.last
-        expect(project_source.pipeline).to eq(new_pipeline)
-        expect(project_source.source_project_id).to eq(pipeline.project_id)
+          project_source = Ci::Sources::Project.last
+          new_pipeline = Ci::Pipeline.last
+          expect(project_source.pipeline).to eq(new_pipeline)
+          expect(project_source.source_project_id).to eq(pipeline.project_id)
+        end
+      end
+
+      context 'when subscription has an author' do
+        let(:author) { create(:user) }
+
+        before do
+          downstream_project.add_developer(author)
+        end
+
+        it_behaves_like 'creates a downstream pipeline'
+
+        it 'uses the subscription author as pipeline user' do
+          execute
+
+          expect(Ci::Pipeline.last.user).to eq(author)
+        end
+      end
+
+      context 'when the legacy subscription does not have an author' do
+        let(:author) { nil }
+
+        it_behaves_like 'creates a downstream pipeline'
+
+        it 'uses the downstream project creator as pipeline user' do
+          execute
+
+          expect(Ci::Pipeline.last.user).to eq(downstream_project.creator)
+        end
+
+        context 'when project creator no longer exists' do
+          before do
+            downstream_project.update!(creator: nil)
+          end
+
+          it 'does not create a downstream pipeline' do
+            expect { execute }
+              .not_to change { Ci::Pipeline.count }.from(1)
+          end
+        end
       end
     end
 

spec/lib/gitlab/import_export/all_models.yml

@@ -572,7 +572,6 @@ project:
 - remove_source_branch_after_merge
 - deleting_user
 - upstream_projects
-- downstream_projects
 - upstream_project_subscriptions
 - downstream_project_subscriptions
 - service_desk_setting