Optimize commit and diff changes access check to reduce git operations

Git operations are costly. Before, if file locks and all file locks were
enabled we would iterate over each commit twice and over each diff as much
as 4 times. This updates uses lambdas to go through all neccessary validations
in only one iteration per commit and per diff. It also removes some unused
code in lib/gitlab/git_access.rb and adds examples to ensure the code keeps
working as intended.

CHANGELOG-EE

@@ -4,6 +4,8 @@ v 8.11.0 (unreleased)
   - Allow projects to be moved between repository storages
   - Add rake task to remove old repository copies from repositories moved to another storage
   - Performance improvement of push rules
+  - Temporary fix for #825 - LDAP sync converts access requests to members. !655
+  - Optimize commit and diff changes access check to reduce git operations
   - Change LdapGroupSyncWorker to use new LDAP group sync classes
   - Removed unused GitLab GEO database index
   - Enable monitoring for ES classes

lib/gitlab/checks/change_access.rb

@@ -12,7 +12,7 @@ module Gitlab
       end
 
       def exec
-        error = protected_branch_checks || tag_checks || push_checks || push_rules_checks
+        error = push_checks || tag_checks || protected_branch_checks || push_rule_check
 
         if error
           GitAccessStatus.new(false, error)
@@ -61,18 +61,6 @@ module Gitlab
         end
       end
 
-      def push_rules_checks
-        # Returns nil if all push rule checks passed successfully
-        # or the error if any hook fails
-        error = push_rule_check
-
-        if !error && license_allows_file_locks?
-          error = path_locks_check
-        end
-
-        error
-      end
-
       private
 
       def protected_tag?(tag_name)
@@ -88,47 +76,32 @@ module Gitlab
       end
 
       def push_rule_check
-        return unless project.push_rule && @newrev && @oldrev
+        return unless @newrev && @oldrev
 
         push_rule = project.push_rule
 
         # Prevent tag removal
         if Gitlab::Git.tag_name(@ref)
-          if push_rule.deny_delete_tag && protected_tag?(Gitlab::Git.tag_name(@ref)) && Gitlab::Git.blank_ref?(@newrev)
-            "You can not delete tag"
+          if push_rule.try(:deny_delete_tag) && protected_tag?(Gitlab::Git.tag_name(@ref)) && Gitlab::Git.blank_ref?(@newrev)
+            return  "You can not delete a tag"
           end
         else
+          commit_validation = push_rule.try(:commit_validation?)
+
           # if newrev is blank, the branch was deleted
-          return if Gitlab::Git.blank_ref?(@newrev) || !push_rule.commit_validation?
+          return if Gitlab::Git.blank_ref?(@newrev) ||
+            !(commit_validation || validate_path_locks?)
 
           commits.each do |commit|
             next if commit_from_annex_sync?(commit.safe_message)
 
-            if error = check_commit(commit, push_rule)
-              return error
+            if commit_validation
+              error = check_commit(commit, push_rule)
+              return error if error
             end
-          end
-        end
-
-        nil
-      end
-
-      def path_locks_check
-        return unless project.path_locks.any? && @newrev && @oldrev
 
-        # locks protect default branch only
-        return if project.default_branch != branch_name(@ref)
-
-        commits.each do |commit|
-          next if commit_from_annex_sync?(commit.safe_message)
-
-          commit.raw_diffs.each do |diff|
-            path = diff.new_path || diff.old_path
-
-            lock_info = project.find_path_lock(path)
-
-            if lock_info && lock_info.user != user_access.user
-              return "The path '#{lock_info.path}' is locked by #{lock_info.user.name}"
+            if error = check_commit_diff(commit, push_rule)
+              return error
             end
           end
         end
@@ -165,34 +138,82 @@ module Gitlab
           end
         end
 
-        if error = check_commit_diff(commit, push_rule)
-          return error
-        end
-
         nil
       end
 
       def check_commit_diff(commit, push_rule)
-        if push_rule.file_name_regex.present?
-          commit.raw_diffs.each do |diff|
-            if (diff.renamed_file || diff.new_file) && diff.new_path =~ Regexp.new(push_rule.file_name_regex)
-              return "File name #{diff.new_path.inspect} is prohibited by the pattern '#{push_rule.file_name_regex}'"
+        validations = validations_for_commit(commit, push_rule)
+
+        return if validations.empty?
+
+        commit.raw_diffs(deltas_only: true).each do |diff|
+          validations.each do |validation|
+            if error = validation.call(diff)
+              return error
             end
           end
         end
 
+        nil
+      end
+
+      def validations_for_commit(commit, push_rule)
+        validations = base_validations
+
+        return validations unless push_rule
+
+        if push_rule.file_name_regex.present?
+          validations << file_name_validation(push_rule.file_name_regex)
+        end
+
         if push_rule.max_file_size > 0
-          commit.raw_diffs.each do |diff|
-            next if diff.deleted_file
+          validations << file_size_validation(commit, push_rule.max_file_size)
+        end
 
-            blob = project.repository.blob_at(commit.id, diff.new_path)
-            if blob && blob.size && blob.size > push_rule.max_file_size.megabytes
-              return "File #{diff.new_path.inspect} is larger than the allowed size of #{push_rule.max_file_size} MB"
-            end
+        validations
+      end
+
+      def base_validations
+        validate_path_locks? ? [path_locks_validation] : []
+      end
+
+      def validate_path_locks?
+        @validate_path_locks ||= license_allows_file_locks? &&
+          project.path_locks.any? && @newrev && @oldrev &&
+          project.default_branch == @branch_name # locks protect default branch only
+      end
+
+      def path_locks_validation
+        lambda do |diff|
+          path = diff.new_path || diff.old_path
+
+          lock_info = project.find_path_lock(path)
+
+          if lock_info && lock_info.user != user_access.user
+            return "The path '#{lock_info.path}' is locked by #{lock_info.user.name}"
           end
         end
+      end
 
-        nil
+      def file_name_validation(file_name_regex)
+        regexp = Regexp.new(file_name_regex)
+
+        lambda do |diff|
+          if (diff.renamed_file || diff.new_file) && diff.new_path =~ regexp
+            return "File name #{diff.new_path.inspect} is prohibited by the pattern '#{file_name_regex}'"
+          end
+        end
+      end
+
+      def file_size_validation(commit, max_file_size)
+        lambda do |diff|
+          return if diff.deleted_file
+
+          blob = project.repository.blob_at(commit.id, diff.new_path)
+          if blob && blob.size && blob.size > max_file_size.megabytes
+            return "File #{diff.new_path.inspect} is larger than the allowed size of #{max_file_size} MB"
+          end
+        end
       end
 
       def commits

lib/gitlab/git_access.rb

@@ -128,73 +128,6 @@ module Gitlab
 
     private
 
-    def commits(newrev, oldrev, project)
-      if oldrev
-        project.repository.commits_between(oldrev, newrev)
-      else
-        project.repository.commits(newrev)
-      end
-    end
-
-    # If commit does not pass push rule validation the whole push should be rejected.
-    # This method should return nil if no error found or status object if there are some errors.
-    # In case of errors - all other checks will be canceled and push will be rejected.
-    def check_commit(commit, push_rule)
-      unless push_rule.commit_message_allowed?(commit.safe_message)
-        return build_status_object(false, "Commit message does not follow the pattern '#{push_rule.commit_message_regex}'")
-      end
-
-      unless push_rule.author_email_allowed?(commit.committer_email)
-        return build_status_object(false, "Committer's email '#{commit.committer_email}' does not follow the pattern '#{push_rule.author_email_regex}'")
-      end
-
-      unless push_rule.author_email_allowed?(commit.author_email)
-        return build_status_object(false, "Author's email '#{commit.author_email}' does not follow the pattern '#{push_rule.author_email_regex}'")
-      end
-
-      # Check whether author is a GitLab member
-      if push_rule.member_check
-        unless User.existing_member?(commit.author_email.downcase)
-          return build_status_object(false, "Author '#{commit.author_email}' is not a member of team")
-        end
-
-        if commit.author_email.casecmp(commit.committer_email) == -1
-          unless User.existing_member?(commit.committer_email.downcase)
-            return build_status_object(false, "Committer '#{commit.committer_email}' is not a member of team")
-          end
-        end
-      end
-
-      if status_object = check_commit_diff(commit, push_rule)
-        return status_object
-      end
-
-      nil
-    end
-
-    def check_commit_diff(commit, push_rule)
-      if push_rule.file_name_regex.present?
-        commit.raw_diffs.each do |diff|
-          if (diff.renamed_file || diff.new_file) && diff.new_path =~ Regexp.new(push_rule.file_name_regex)
-            return build_status_object(false, "File name #{diff.new_path.inspect} is prohibited by the pattern '#{push_rule.file_name_regex}'")
-          end
-        end
-      end
-
-      if push_rule.max_file_size > 0
-        commit.raw_diffs.each do |diff|
-          next if diff.deleted_file
-
-          blob = project.repository.blob_at(commit.id, diff.new_path)
-          if blob && blob.size && blob.size > push_rule.max_file_size.megabytes
-            return build_status_object(false, "File #{diff.new_path.inspect} is larger than the allowed size of #{push_rule.max_file_size} MB")
-          end
-        end
-      end
-
-      nil
-    end
-
     def protected_branch_action(oldrev, newrev, branch_name)
       # we dont allow force push to protected branch
       if forced_push?(oldrev, newrev)

spec/lib/gitlab/checks/change_access_spec.rb

@@ -95,5 +95,116 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
         end
       end
     end
+
+    context 'push rules checks' do
+      let(:project) { create(:project, :public, push_rule: push_rule) }
+
+      before do
+        allow(project.repository).to receive(:new_commits).and_return(
+          project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51')
+        )
+      end
+
+      context 'tag deletion' do
+        let(:changes) do
+          {
+            oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c',
+            newrev: '0000000000000000000000000000000000000000',
+            ref: 'refs/tags/v1.0.0'
+          }
+        end
+        let(:push_rule) { create(:push_rule, deny_delete_tag: true) }
+
+        before { allow(user_access).to receive(:can_do_action?).with(:admin_project).and_return(true) }
+
+        it 'returns an error if the rule denies tag deletion' do
+          expect(subject.status).to be(false)
+          expect(subject.message).to eq('You can not delete a tag')
+        end
+      end
+
+      context 'commit message rules' do
+        let(:push_rule) { create(:push_rule, :commit_message) }
+
+        it 'returns an error if the rule fails' do
+          expect(subject.status).to be(false)
+          expect(subject.message).to eq("Commit message does not follow the pattern '#{push_rule.commit_message_regex}'")
+        end
+      end
+
+      context 'author email rules' do
+        let(:push_rule) { create(:push_rule, author_email_regex: '.*@valid.com') }
+
+        before do
+          allow_any_instance_of(Commit).to receive(:committer_email).and_return('mike@valid.com')
+          allow_any_instance_of(Commit).to receive(:author_email).and_return('mike@valid.com')
+        end
+
+        it 'returns an error if the rule fails for the committer' do
+          allow_any_instance_of(Commit).to receive(:committer_email).and_return('ana@invalid.com')
+
+          expect(subject.status).to be(false)
+          expect(subject.message).to eq("Committer's email 'ana@invalid.com' does not follow the pattern '.*@valid.com'")
+        end
+
+        it 'returns an error if the rule fails for the author' do
+          allow_any_instance_of(Commit).to receive(:author_email).and_return('joan@invalid.com')
+
+          expect(subject.status).to be(false)
+          expect(subject.message).to eq("Author's email 'joan@invalid.com' does not follow the pattern '.*@valid.com'")
+        end
+      end
+
+      context 'existing member rules' do
+        let(:push_rule) { create(:push_rule, member_check: true) }
+
+        before do
+          allow(User).to receive(:existing_member?).and_return(false)
+          allow_any_instance_of(Commit).to receive(:author_email).and_return('some@mail.com')
+        end
+
+        it 'returns an error if the commit author is not a GitLab member' do
+          expect(subject.status).to be(false)
+          expect(subject.message).to eq("Author 'some@mail.com' is not a member of team")
+        end
+      end
+
+      context 'file name rules' do
+        # Notice that the commit used creates a file named 'README'
+        let(:push_rule) { create(:push_rule, file_name_regex: 'READ*') }
+
+        it "returns an error if a new or renamed filed doesn't match the file name regex" do
+          expect(subject.status).to be(false)
+          expect(subject.message).to eq("File name \"README\" is prohibited by the pattern 'READ*'")
+        end
+      end
+
+      context 'max file size rules' do
+        let(:push_rule) { create(:push_rule, max_file_size: 1) }
+
+        before { allow_any_instance_of(Blob).to receive(:size).and_return(2.megabytes) }
+
+        it 'returns an error if file exceeds the maximum file size' do
+          expect(subject.status).to be(false)
+          expect(subject.message).to eq("File \"README\" is larger than the allowed size of 1 MB")
+        end
+      end
+    end
+
+    context 'file lock rules' do
+      let!(:path_lock) { create(:path_lock, path: 'README', project: project) }
+
+      before do
+        allow_any_instance_of(PathLocksHelper).to receive(:license_allows_file_locks?).and_return(true)
+
+        allow(project.repository).to receive(:new_commits).and_return(
+          project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51')
+        )
+      end
+      it 'returns an error if the changes update a path locked by another user' do
+        expect(subject.status).to be(false)
+        expect(subject.message).to eq("The path 'README' is locked by #{path_lock.user.name}")
+      end
+    end
   end
 end