Merge branch 'optimize-change-access' into 'master'

Optimize commit and diff changes access check to reduce git operations

Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/880

See merge request !647

CHANGELOG-EE

@@ -5,6 +5,7 @@ v 8.11.0 (unreleased)
   - 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 = push_checks || tag_checks || protected_branch_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