Merge branch 'pks-bulk-push-rules' into 'master'

Refactor push rules to allow for bulk checks

See merge request gitlab-org/gitlab!68836

ee/lib/ee/gitlab/checks/base_single_checker.rb → ee/lib/ee/gitlab/checks/base_checker.rb

@@ -3,7 +3,7 @@
 module EE
   module Gitlab
     module Checks
-      module BaseSingleChecker
+      module BaseChecker
         extend ActiveSupport::Concern
         include ::Gitlab::Utils::StrongMemoize
 

ee/lib/ee/gitlab/checks/single_change_access.rb → ee/lib/ee/gitlab/checks/changes_access.rb

@@ -3,12 +3,12 @@
 module EE
   module Gitlab
     module Checks
-      module SingleChangeAccess
+      module ChangesAccess
         extend ActiveSupport::Concern
         extend ::Gitlab::Utils::Override
 
-        override :ref_level_checks
-        def ref_level_checks
+        override :bulk_access_checks!
+        def bulk_access_checks!
           super
 
           PushRuleCheck.new(self).validate!

ee/lib/ee/gitlab/checks/push_rule_check.rb

@@ -3,7 +3,7 @@
 module EE
   module Gitlab
     module Checks
-      class PushRuleCheck < ::Gitlab::Checks::BaseSingleChecker
+      class PushRuleCheck < ::Gitlab::Checks::BaseBulkChecker
         def validate!
           return unless push_rule
 
@@ -19,17 +19,23 @@ module EE
         # @return [Nil] returns nil unless an error is raised
         # @raise [Gitlab::GitAccess::ForbiddenError] if check fails
         def check_tag_or_branch!
-          if tag_name
-            PushRules::TagCheck.new(change_access).validate!
+          changes_access.single_change_accesses.each do |single_change_access|
+            if single_change_access.tag_name
+              PushRules::TagCheck.new(single_change_access).validate!
             else
-            PushRules::BranchCheck.new(change_access).validate!
+              PushRules::BranchCheck.new(single_change_access).validate!
             end
           end
 
+          nil
+        end
+
         # @return [Nil] returns nil unless an error is raised
         # @raise [Gitlab::GitAccess::ForbiddenError] if check fails
         def check_file_size!
-          PushRules::FileSizeCheck.new(change_access).validate!
+          PushRules::FileSizeCheck.new(changes_access).validate!
+
+          nil
         end
 
         # Run the checks one after the other.

ee/lib/ee/gitlab/checks/push_rules/file_size_check.rb

@@ -4,7 +4,7 @@ module EE
   module Gitlab
     module Checks
       module PushRules
-        class FileSizeCheck < ::Gitlab::Checks::BaseSingleChecker
+        class FileSizeCheck < ::Gitlab::Checks::BaseBulkChecker
           LOG_MESSAGE = "Checking if any files are larger than the allowed size..."
 
           def validate!
@@ -12,6 +12,12 @@ module EE
 
             logger.log_timed(LOG_MESSAGE) do
               max_file_size = push_rule.max_file_size
+
+              changes_access.changes.each do |change|
+                newrev = change[:newrev]
+
+                next if newrev.blank? || ::Gitlab::Git.blank_ref?(newrev)
+
                 blobs = project.repository.new_blobs(newrev, dynamic_timeout: logger.time_left)
 
                 large_blob = blobs.find do |blob|
@@ -27,4 +33,5 @@ module EE
         end
       end
     end
+  end
 end

ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb

@@ -3,9 +3,16 @@
 require 'spec_helper'
 
 RSpec.describe EE::Gitlab::Checks::PushRuleCheck do
-  include_context 'push rules checks context'
+  include_context 'changes access checks context'
 
   let(:push_rule) { create(:push_rule, :commit_message) }
+  let(:project) { create(:project, :public, :repository, push_rule: push_rule) }
+
+  before do
+    allow(project.repository).to receive(:new_commits).and_return(
+      project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51')
+    )
+  end
 
   shared_examples "push checks" do
     before do
@@ -28,8 +35,11 @@ RSpec.describe EE::Gitlab::Checks::PushRuleCheck do
     end
 
     context 'when tag name exists' do
-      before do
-        allow(change_access).to receive(:tag_name).and_return(true)
+      let(:changes) do
+        [
+          # Update of a tag.
+          { oldrev: oldrev, newrev: newrev, ref: 'refs/tags/name' }
+        ]
       end
 
       it 'validates tags push rules' do
@@ -43,8 +53,11 @@ RSpec.describe EE::Gitlab::Checks::PushRuleCheck do
     end
 
     context 'when tag name does not exists' do
-      before do
-        allow(change_access).to receive(:tag_name).and_return(false)
+      let(:changes) do
+        [
+          # Update of a branch.
+          { oldrev: oldrev, newrev: newrev, ref: 'refs/heads/name' }
+        ]
       end
 
       it 'validates branches push rules' do
@@ -56,6 +69,24 @@ RSpec.describe EE::Gitlab::Checks::PushRuleCheck do
         subject.validate!
       end
     end
+
+    context 'when tag and branch exist' do
+      let(:changes) do
+        [
+          { oldrev: oldrev, newrev: newrev, ref: 'refs/heads/name' },
+          { oldrev: oldrev, newrev: newrev, ref: 'refs/tags/name' }
+        ]
+      end
+
+      it 'validates tag and branch push rules' do
+        expect_any_instance_of(EE::Gitlab::Checks::PushRules::TagCheck)
+          .to receive(:validate!)
+        expect_any_instance_of(EE::Gitlab::Checks::PushRules::BranchCheck)
+          .to receive(:validate!)
+
+        subject.validate!
+      end
+    end
   end
 
   describe '#validate!' do

ee/spec/lib/ee/gitlab/checks/push_rules/file_size_check_spec.rb

@@ -5,6 +5,29 @@ require 'spec_helper'
 RSpec.describe EE::Gitlab::Checks::PushRules::FileSizeCheck do
   include_context 'push rules checks context'
 
+  let(:changes) do
+    [
+      # Update of existing branch
+      { oldrev: oldrev, newrev: newrev, ref: ref },
+      # Creation of new branch
+      { newrev: newrev, ref: 'refs/heads/something' },
+      # Deletion of branch
+      { oldrev: oldrev, ref: 'refs/heads/deleteme' }
+    ]
+  end
+
+  let(:changes_access) do
+    Gitlab::Checks::ChangesAccess.new(
+      changes,
+      project: project,
+      user_access: user_access,
+      protocol: protocol,
+      logger: logger
+    )
+  end
+
+  subject { described_class.new(changes_access) }
+
   describe '#validate!' do
     let(:push_rule) { create(:push_rule, max_file_size: 1) }
     # SHA of the 2-mb-file branch

ee/spec/lib/gitlab/checks/single_change_access_spec.rb → ee/spec/lib/gitlab/checks/changes_access_spec.rb

@@ -2,13 +2,28 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::Checks::SingleChangeAccess do
+RSpec.describe Gitlab::Checks::ChangesAccess do
   describe '#validate!' do
     include_context 'push rules checks context'
 
     let(:push_rule) { create(:push_rule, deny_delete_tag: true) }
+    let(:changes) do
+      [
+        { oldrev: oldrev, newrev: newrev, ref: ref }
+      ]
+    end
+
+    let(:changes_access) do
+      Gitlab::Checks::ChangesAccess.new(
+        changes,
+        project: project,
+        user_access: user_access,
+        protocol: protocol,
+        logger: logger
+      )
+    end
 
-    subject { change_access }
+    subject { changes_access }
 
     it_behaves_like 'check ignored when push rule unlicensed'
 

lib/gitlab/checks/base_single_checker.rb

@@ -30,5 +30,3 @@ module Gitlab
     end
   end
 end
-
-Gitlab::Checks::BaseSingleChecker.prepend_mod_with('Gitlab::Checks::BaseSingleChecker')

lib/gitlab/checks/changes_access.rb

@@ -76,15 +76,16 @@ module Gitlab
         result
       end
 
-      protected
-
-      def single_access_checks!
-        # Iterate over all changes to find if user allowed all of them to be applied
-        changes.each do |change|
-          commits = Gitlab::Lazy.new { commits_for(change[:newrev]) }
+      def single_change_accesses
+        @single_changes_accesses ||=
+          changes.map do |change|
+            commits =
+              if change[:newrev].blank? || Gitlab::Git.blank_ref?(change[:newrev])
+                []
+              else
+                Gitlab::Lazy.new { commits_for(change[:newrev]) }
+              end
 
-          # If user does not have access to make at least one change, cancel all
-          # push by allowing the exception to bubble up
             Checks::SingleChangeAccess.new(
               change,
               user_access: user_access,
@@ -92,7 +93,16 @@ module Gitlab
               protocol: protocol,
               logger: logger,
               commits: commits
-          ).validate!
+            )
+          end
+      end
+
+      protected
+
+      def single_access_checks!
+        # Iterate over all changes to find if user allowed all of them to be applied
+        single_change_accesses.each do |single_change_access|
+          single_change_access.validate!
         end
       end
 
@@ -102,3 +112,5 @@ module Gitlab
     end
   end
 end
+
+Gitlab::Checks::ChangesAccess.prepend_mod_with('Gitlab::Checks::ChangesAccess')

spec/lib/gitlab/checks/changes_access_spec.rb

@@ -174,6 +174,101 @@ RSpec.describe Gitlab::Checks::ChangesAccess do
     end
   end
 
+  describe '#single_change_accesses' do
+    let(:commits_for) { {} }
+    let(:expected_accesses) { [] }
+
+    shared_examples '#single_change_access' do
+      before do
+        commits_for.each do |id, commits|
+          expect(subject)
+            .to receive(:commits_for)
+            .with(id)
+            .and_return(commits)
+        end
+      end
+
+      it 'returns an array of SingleChangeAccess' do
+        # Commits are wrapped in a Gitlab::Lazy and thus need to be resolved
+        # first such that we can directly compare types.
+        actual_accesses = subject.single_change_accesses
+          .each { |access| access.instance_variable_set(:@commits, access.commits.to_a) }
+
+        expect(actual_accesses).to match_array(expected_accesses)
+      end
+    end
+
+    context 'with no changes' do
+      let(:changes) { [] }
+
+      it_behaves_like '#single_change_access'
+    end
+
+    context 'with a single change and no new commits' do
+      let(:commits_for) { { 'new' => [] } }
+      let(:changes) do
+        [
+          { oldrev: 'old', newrev: 'new', ref: 'refs/heads/branch' }
+        ]
+      end
+
+      let(:expected_accesses) do
+        [
+          have_attributes(oldrev: 'old', newrev: 'new', ref: 'refs/heads/branch', commits: [])
+        ]
+      end
+
+      it_behaves_like '#single_change_access'
+    end
+
+    context 'with a single change and new commits' do
+      let(:commits_for) { { 'new' => [create_commit('new', [])] } }
+      let(:changes) do
+        [
+          { oldrev: 'old', newrev: 'new', ref: 'refs/heads/branch' }
+        ]
+      end
+
+      let(:expected_accesses) do
+        [
+          have_attributes(oldrev: 'old', newrev: 'new', ref: 'refs/heads/branch', commits: [create_commit('new', [])])
+        ]
+      end
+
+      it_behaves_like '#single_change_access'
+    end
+
+    context 'with multiple changes' do
+      let(:commits_for) do
+        {
+          'a' => [create_commit('a', [])],
+          'c' => [create_commit('c', [])],
+          'd' => []
+        }
+      end
+
+      let(:changes) do
+        [
+          { newrev: 'a', ref: 'refs/heads/a' },
+          { oldrev: 'b', ref: 'refs/heads/b' },
+          { oldrev: 'a', newrev: 'c', ref: 'refs/heads/c' },
+          { newrev: 'd', ref: 'refs/heads/d' }
+        ]
+      end
+
+      let(:expected_accesses) do
+        [
+          have_attributes(newrev: 'a', ref: 'refs/heads/a', commits: [create_commit('a', [])]),
+          have_attributes(oldrev: 'b', ref: 'refs/heads/b', commits: []),
+          have_attributes(oldrev: 'a', newrev: 'c', ref: 'refs/heads/c', commits: [create_commit('c', [])]),
+          have_attributes(newrev: 'd', ref: 'refs/heads/d', commits: [])
+        ]
+      end
+
+      it_behaves_like '#single_change_access'
+    end
+  end
+
   def create_commit(id, parent_ids)
     Gitlab::Git::Commit.new(project.repository, {
       id: id,