Reject push if files related are locked by LFS

Some migration of code from EE was required given it was not possible to
validate the `Commit` object from CE.

app/models/repository.rb

@@ -171,6 +171,13 @@ class Repository
     commits
   end
 
+  # Returns a list of commits that are not present in any reference
+  def new_commits(newrev)
+    refs = ::Gitlab::Git::RevList.new(raw, newrev: newrev).new_refs
+
+    refs.map { |sha| commit(sha.strip) }
+  end
+
   # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/384
   def find_commits_by_message(query, ref = nil, path = nil, limit = 1000, offset = 0)
     unless exists? && has_visible_content? && query.present?

ee/app/models/ee/repository.rb

@@ -22,13 +22,6 @@ module EE
       expire_content_cache
     end
 
-    # Returns a list of commits that are not present in any reference
-    def new_commits(newrev)
-      refs = ::Gitlab::Git::RevList.new(raw, newrev: newrev).new_refs
-
-      refs.map { |sha| commit(sha.strip) }
-    end
-
     def squash(user, merge_request)
       raw.squash(user, merge_request.id, branch: merge_request.target_branch,
                                          start_sha: merge_request.diff_start_sha,

ee/lib/ee/gitlab/checks/change_access.rb

@@ -19,9 +19,12 @@ module EE
         def exec
           return true if skip_authorization
 
-          super
+          super(skip_commits_check: true)
 
           push_rule_check
+          # Check of commits should happen as the last step
+          # given they're expensive in terms of performance
+          commits_check
 
           true
         end
@@ -91,10 +94,6 @@ module EE
             error = check_commit(commit)
             raise ::Gitlab::GitAccess::UnauthorizedError, error if error
           end
-
-          if error = check_commit_diff(commit)
-            raise ::Gitlab::GitAccess::UnauthorizedError, error
-          end
         end
 
         # If commit does not pass push rule validation the whole push should be rejected.
@@ -148,38 +147,22 @@ module EE
           end
         end
 
-        def check_commit_diff(commit)
-          validations = validations_for_commit(commit)
-
-          return if validations.empty?
-
-          commit.raw_deltas.each do |diff|
-            validations.each do |validation|
-              if error = validation.call(diff)
-                return error
-              end
-            end
-          end
-
-          nil
-        end
-
+        override :validations_for_commit
         def validations_for_commit(commit)
-          validations = base_validations
+          validations = super
 
-          return validations unless push_rule
+          validations.push(path_locks_validation) if validate_path_locks?
+          validations.concat(push_rule_commit_validations(commit))
+        end
 
-          validations << file_name_validation
+        def push_rule_commit_validations(commit)
+          return [] unless push_rule
 
-          if push_rule.max_file_size > 0
-            validations << file_size_validation(commit, push_rule.max_file_size)
+          [file_name_validation].tap do |validations|
+            if push_rule.max_file_size > 0
+              validations << file_size_validation(commit, push_rule.max_file_size)
+            end
           end
-
-          validations
-        end
-
-        def base_validations
-          validate_path_locks? ? [path_locks_validation] : []
         end
 
         def validate_path_locks?
@@ -204,10 +187,14 @@ module EE
 
         def file_name_validation
           lambda do |diff|
-            if (diff.renamed_file || diff.new_file) && blacklisted_regex = push_rule.filename_blacklisted?(diff.new_path)
-              return nil unless blacklisted_regex.present?
+            begin
+              if (diff.renamed_file || diff.new_file) && blacklisted_regex = push_rule.filename_blacklisted?(diff.new_path)
+                return nil unless blacklisted_regex.present?
 
-              "File name #{diff.new_path} was blacklisted by the pattern #{blacklisted_regex}."
+                "File name #{diff.new_path} was blacklisted by the pattern #{blacklisted_regex}."
+              end
+            rescue ::PushRule::MatchError => e
+              raise ::Gitlab::GitAccess::UnauthorizedError, e.message
             end
           end
         end
@@ -222,10 +209,6 @@ module EE
             end
           end
         end
-
-        def commits
-          project.repository.new_commits(newrev)
-        end
       end
     end
   end

lib/gitlab/checks/change_access.rb

@@ -33,13 +33,14 @@ module Gitlab
         @protocol = protocol
       end
 
-      def exec
+      def exec(skip_commits_check: false)
         return true if skip_authorization
 
         push_checks
         branch_checks
         tag_checks
         lfs_objects_exist_check
+        commits_check unless skip_commits_check
 
         true
       end
@@ -119,6 +120,50 @@ module Gitlab
         end
       end
 
+      def commits_check
+        return if deletion? || newrev.nil?
+
+        commits.each do |commit|
+          check_commit_diff(commit)
+        end
+      end
+
+      def check_commit_diff(commit)
+        validations = validations_for_commit(commit)
+
+        return if validations.empty?
+
+        commit.raw_deltas.each do |diff|
+          validations.each do |validation|
+            if error = validation.call(diff)
+              raise ::Gitlab::GitAccess::UnauthorizedError, error
+            end
+          end
+        end
+
+        nil
+      end
+
+      def validations_for_commit(commit)
+        validate_lfs_file_locks? ? [lfs_file_locks_validation] : []
+      end
+
+      def validate_lfs_file_locks?
+        project.lfs_enabled? && project.lfs_file_locks.any? && newrev && oldrev
+      end
+
+      def lfs_file_locks_validation
+        lambda do |diff|
+          path = diff.new_path || diff.old_path
+
+          lfs_lock = project.lfs_file_locks.find_by(path: path)
+
+          if lfs_lock && lfs_lock.user != user_access.user
+            return "The path '#{lfs_lock.path}' is locked in Git LFS by #{lfs_lock.user.name}"
+          end
+        end
+      end
+
       private
 
       def updated_from_web?
@@ -152,6 +197,10 @@ module Gitlab
           raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:lfs_objects_missing]
         end
       end
+
+      def commits
+        project.repository.new_commits(newrev)
+      end
     end
   end
 end

spec/ee/spec/models/ee/repository_spec.rb

-require 'spec_helper'
-
-describe EE::Repository do
-  let(:project) { create(:project, :repository) }
-  let(:repository) { project.repository }
-
-  describe '#new_commits' do
-    let(:new_refs) do
-      double(:git_rev_list, new_refs: %w[
-        c1acaa58bbcbc3eafe538cb8274ba387047b69f8
-        5937ac0a7beb003549fc5fd26fc247adbce4a52e
-      ])
-    end
-
-    it 'delegates to Gitlab::Git::RevList' do
-      expect(Gitlab::Git::RevList).to receive(:new).with(
-        repository.raw,
-        newrev: 'aaaabbbbccccddddeeeeffffgggghhhhiiiijjjj').and_return(new_refs)
-
-      commits = repository.new_commits('aaaabbbbccccddddeeeeffffgggghhhhiiiijjjj')
-
-      expect(commits).to eq([
-        repository.commit('c1acaa58bbcbc3eafe538cb8274ba387047b69f8'),
-        repository.commit('5937ac0a7beb003549fc5fd26fc247adbce4a52e')
-      ])
-    end
-  end
-end

spec/lib/gitlab/checks/change_access_spec.rb

@@ -208,5 +208,45 @@ describe Gitlab::Checks::ChangeAccess do
         end
       end
     end
+
+    context 'LFS file lock check' do
+      let(:owner) { create(:user) }
+      let!(:lock) { create(:lfs_file_lock, user: owner, project: project, path: 'README') }
+
+      before do
+        allow(project.repository).to receive(:new_commits).and_return(
+          project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51')
+        )
+      end
+
+      context 'with LFS not enabled' do
+        it 'skips the validation' do
+          expect_any_instance_of(described_class).not_to receive(:lfs_file_locks_validation)
+
+          subject.exec
+        end
+
+      end
+
+      context 'with LFS enabled' do
+        before do
+          allow(project).to receive(:lfs_enabled?).and_return(true)
+        end
+
+        context 'when change is sent by a different user' do
+          it 'raises an error if the user is not allowed to update the file' do
+            expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "The path 'README' is locked in Git LFS by #{lock.user.name}")
+          end
+        end
+
+        context 'when change is sent by the author od the lock' do
+          let(:user) { owner }
+
+          it "doesn't raise any error" do
+            expect { subject.exec }.not_to raise_error
+          end
+        end
+      end
+    end
   end
 end

spec/models/repository_spec.rb

@@ -262,6 +262,28 @@ describe Repository do
     end
   end
 
+  describe '#new_commits' do
+    let(:new_refs) do
+      double(:git_rev_list, new_refs: %w[
+        c1acaa58bbcbc3eafe538cb8274ba387047b69f8
+        5937ac0a7beb003549fc5fd26fc247adbce4a52e
+      ])
+    end
+
+    it 'delegates to Gitlab::Git::RevList' do
+      expect(Gitlab::Git::RevList).to receive(:new).with(
+        repository.raw,
+        newrev: 'aaaabbbbccccddddeeeeffffgggghhhhiiiijjjj').and_return(new_refs)
+
+      commits = repository.new_commits('aaaabbbbccccddddeeeeffffgggghhhhiiiijjjj')
+
+      expect(commits).to eq([
+        repository.commit('c1acaa58bbcbc3eafe538cb8274ba387047b69f8'),
+        repository.commit('5937ac0a7beb003549fc5fd26fc247adbce4a52e')
+      ])
+    end
+  end
+
   describe '#commits_by' do
     set(:project) { create(:project, :repository) }