Merge branch '349495-codeowners-can-be-bypassed-for-moved-files' into 'master'

Resolve "Codeowners can be bypassed for moved files"

See merge request gitlab-org/gitlab!87359

app/services/merge_requests/reload_merge_head_diff_service.rb

@@ -43,3 +43,5 @@ def recreate_merge_head_diff
     end
   end
 end
+
+MergeRequests::ReloadMergeHeadDiffService.prepend_mod

ee/app/services/ee/merge_requests/reload_merge_head_diff_service.rb

+# frozen_string_literal: true
+
+module EE
+  module MergeRequests
+    module ReloadMergeHeadDiffService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute
+        super.tap do |response|
+          next unless response[:status] == :success
+          next unless merge_request.project.licensed_feature_available?(:code_owners)
+          next if merge_request.on_train?
+
+          sync_code_owner_approval_rules
+        end
+      end
+
+      private
+
+      def sync_code_owner_approval_rules
+        ::MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute
+      end
+    end
+  end
+end

ee/lib/gitlab/code_owners.rb

@@ -55,39 +55,11 @@ def self.loader_for_merge_request(merge_request, merge_request_diff)
     private_class_method :loader_for_merge_request
     def self.paths_for_merge_request(merge_request, merge_request_diff)
-      # Because MergeRequest#modified_paths is limited to only returning 1_000
-      #   records, if the diff_size is more than 1_000, we need to fall back to
-      #   the MUCH slower method of using Repository#diff_stats, which isn't
-      #   subject to the same limit.
-      #
-      if oversized_merge_request?(merge_request, merge_request_diff)
-        slow_path_lookup(merge_request, merge_request_diff)
-      else
-        fast_path_lookup(merge_request, merge_request_diff)
-      end
-    end
-    private_class_method :paths_for_merge_request
-
-    def self.oversized_merge_request?(merge_request, merge_request_diff)
-      mrd_to_check_for_overflow = merge_request_diff || merge_request.merge_request_diff
-      mrd_to_check_for_overflow.overflow?
+      # NOTE: merge_head_diff is preferred as we want to include latest changes from the target branch
+      merge_request_diff ||= merge_request.merge_head_diff || merge_request.merge_request_diff
+      merge_request_diff.modified_paths(fallback_on_overflow: true)
     end
-    private_class_method :oversized_merge_request?
-
-    def self.slow_path_lookup(merge_request, merge_request_diff)
-      merge_request_diff ||= merge_request.merge_request_diff
-
-      merge_request.project.repository.diff_stats(
-        merge_request_diff.base_commit_sha,
-        merge_request_diff.head_commit_sha
-      ).paths
-    end
-    private_class_method :slow_path_lookup
-
-    def self.fast_path_lookup(merge_request, merge_request_diff)
-      merge_request.modified_paths(past_merge_request_diff: merge_request_diff)
-    end
-    private_class_method :fast_path_lookup
+    private_class_method :paths_for_merge_request
   end
 end

ee/spec/lib/gitlab/code_owners_spec.rb

@@ -102,36 +102,6 @@
     end
   end
-  describe '.fast_path_lookup and .slow_path_lookup' do
-    let(:codeowner_lookup_ref) { merge_request.target_branch }
-    let(:codeowner_content) { 'files/ruby/feature.rb @owner-1' }
-    let(:merge_request) do
-      create(
-        :merge_request,
-        source_project: project,
-        source_branch: 'feature',
-        target_project: project,
-        target_branch: 'with-codeowners'
-      )
-    end
-
-    before do
-      stub_licensed_features(code_owners: true)
-    end
-
-    it 'returns equivalent results' do
-      fast_results = described_class.entries_for_merge_request(merge_request).first
-
-      expect(merge_request.merge_request_diff).to receive(:overflow?).and_return(true)
-
-      slow_results = described_class.entries_for_merge_request(merge_request).first
-
-      expect(slow_results.users).to eq(fast_results.users)
-      expect(slow_results.groups).to eq(fast_results.groups)
-      expect(slow_results.pattern).to eq(fast_results.pattern)
-    end
-  end
-
   describe '.entries_for_merge_request' do
     subject(:entries) { described_class.entries_for_merge_request(merge_request, merge_request_diff: merge_request_diff) }
@@ -150,36 +120,58 @@
     context 'when the feature is available' do
       before do
         stub_licensed_features(code_owners: true)
-
-        allow(merge_request).to receive(:modified_paths).with(past_merge_request_diff: merge_request_diff) { ['docs/CODEOWNERS'] }
       end
-      it 'returns owners for merge request' do
-        expect(entries.first).to have_attributes(pattern: 'docs/CODEOWNERS', users: [code_owner])
-      end
-      context 'when merge_request_diff is specified' do
-        let(:merge_request_diff) { merge_request.merge_request_diff }
-        it 'returns owners at the specified ref' do
-          expect(described_class).to receive(:fast_path_lookup).and_call_original
-          expect(entries.first).to have_attributes(pattern: 'docs/CODEOWNERS', users: [code_owner])
+      context 'when merge_head_diff exists' do
+        before do
+          merge_head_diff = instance_double(MergeRequestDiff)
+          expect(merge_head_diff).to receive(:modified_paths).with(fallback_on_overflow: true).and_return(modified_paths)
+          expect(merge_request).to receive(:merge_head_diff).and_return(merge_head_diff)
+          expect(merge_request).not_to receive(:merge_request_diff).and_call_original
+        end
+        context 'when the changed file paths have matching code owners' do
+          let(:modified_paths) { ['docs/CODEOWNERS'] }
+          it 'returns owners for merge request' do
+            expect(entries.first).to have_attributes(pattern: 'docs/CODEOWNERS', users: [code_owner])
+          end
+        end
+        context 'when the changed file paths do not have matching code owners' do
+          let(:modified_paths) { ['files/ruby/feature.rb'] }
+
+          it 'returns an empty array' do
+            expect(entries).to be_empty
+          end
         end
       end
-      context 'when the merge request is large (>1_000 files)' do
+      context 'when merge_head_diff does not exist' do
         before do
-          expect(merge_request.merge_request_diff).to receive(:overflow?) { true }
+          expect(merge_request).to receive(:merge_head_diff).and_return(nil)
         end
-        it 'generates paths via .slow_path_lookup' do
-          expect(described_class).not_to receive(:fast_path_lookup)
-          expect(described_class).to receive(:slow_path_lookup).and_call_original
+        it 'falls back to merge_request_diff' do
+          expect(merge_request.merge_request_diff).to receive(:modified_paths).with(fallback_on_overflow: true).and_call_original
           entries
         end
       end
+
+      context 'when merge_request_diff is specified' do
+        let(:merge_request_diff) { merge_request.merge_request_diff }
+        let(:modified_paths) { ['docs/CODEOWNERS'] }
+
+        before do
+          expect(merge_request_diff).to receive(:modified_paths).with(fallback_on_overflow: true).and_return(modified_paths)
+        end
+
+        it 'returns owners at the specified ref' do
+          expect(merge_request).not_to receive(:merge_head_diff)
+          expect(entries.first).to have_attributes(pattern: 'docs/CODEOWNERS', users: [code_owner])
+        end
+      end
     end
     context 'when the feature is not available' do

ee/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::ReloadMergeHeadDiffService do
+  let(:merge_request) { create(:merge_request) }
+
+  subject { described_class.new(merge_request).execute }
+
+  describe '#execute' do
+    before do
+      MergeRequests::MergeToRefService
+        .new(project: merge_request.project, current_user: merge_request.author)
+        .execute(merge_request)
+    end
+
+    context 'code_owners feature is available' do
+      before do
+        stub_licensed_features(code_owners: true)
+      end
+
+      context 'when reloading was successful' do
+        context 'when merge request is not on a merge train' do
+          it 'syncs code owner approval rules' do
+            sync_service = instance_double(MergeRequests::SyncCodeOwnerApprovalRules)
+            expect(sync_service).to receive(:execute)
+            expect(MergeRequests::SyncCodeOwnerApprovalRules).to receive(:new)
+              .with(merge_request)
+              .and_return(sync_service)
+            expect(subject[:status]).to eq(:success)
+          end
+        end
+
+        context 'when merge request is on a merge train' do
+          let(:merge_request) { create(:merge_request, :on_train) }
+
+          it 'does not sync code owner approval rules' do
+            expect(MergeRequests::SyncCodeOwnerApprovalRules).not_to receive(:new)
+            expect(subject[:status]).to eq(:success)
+          end
+        end
+      end
+
+      context 'when reloading failed' do
+        before do
+          allow(merge_request).to receive(:create_merge_head_diff!).and_raise('fail')
+        end
+
+        it 'does not sync code owner approval rules' do
+          expect(MergeRequests::SyncCodeOwnerApprovalRules).not_to receive(:new)
+          expect(subject[:status]).to eq(:error)
+        end
+      end
+    end
+
+    context 'code_owners feature is not available' do
+      before do
+        stub_licensed_features(code_owners: false)
+      end
+
+      context 'when reloading was successful' do
+        it 'syncs code owner approval rules' do
+          expect(MergeRequests::SyncCodeOwnerApprovalRules).not_to receive(:new)
+          expect(subject[:status]).to eq(:success)
+        end
+      end
+    end
+  end
+end