Merge branch 'jej/fs-prevent-push-when-missing-objects' into 'master'

Prevent git push when LFS objects are missing

Closes #24564

See merge request gitlab-org/gitlab-ce!13837

4 files changed, 101 insertions, 17 deletions

diff --git a/changelogs/unreleased/jej-fs-prevent-push-when-missing-objects.yml b/changelogs/unreleased/jej-fs-prevent-push-when-missing-objects.yml
new file mode 100644
index 00000000000..4eeedec2c99
--- /dev/null
+++ b/changelogs/unreleased/jej-fs-prevent-push-when-missing-objects.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent git push when LFS objects are missing
+merge_request: 13837
+author:
+type: added
diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index b6805230348..ef92fc5a0a0 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/lib/gitlab/checks/change_access.rb
@@ -12,7 +12,8 @@ module Gitlab
         change_existing_tags: 'You are not allowed to change existing tags on this project.',
         update_protected_tag: 'Protected tags cannot be updated.',
         delete_protected_tag: 'Protected tags cannot be deleted.',
-        create_protected_tag: 'You are not allowed to create this tag as it is protected.'
+        create_protected_tag: 'You are not allowed to create this tag as it is protected.',
+        lfs_objects_missing: 'LFS objects are missing. Ensure LFS is properly set up or try a manual "git lfs push --all".'
       }.freeze
 
       attr_reader :user_access, :project, :skip_authorization, :protocol
@@ -36,6 +37,7 @@ module Gitlab
         push_checks
         branch_checks
         tag_checks
+        lfs_objects_exist_check
 
         true
       end
@@ -136,6 +138,14 @@ module Gitlab
       def matching_merge_request?
         Checks::MatchingMergeRequest.new(@newrev, @branch_name, @project).match?
       end
+
+      def lfs_objects_exist_check
+        lfs_check = Checks::LfsIntegrity.new(project, @newrev)
+
+        if lfs_check.objects_missing?
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:lfs_objects_missing]
+        end
+      end
     end
   end
 end
diff --git a/lib/gitlab/checks/lfs_integrity.rb b/lib/gitlab/checks/lfs_integrity.rb
new file mode 100644
index 00000000000..27a95764dc1
--- /dev/null
+++ b/lib/gitlab/checks/lfs_integrity.rb
@@ -0,0 +1,24 @@
+module Gitlab
+  module Checks
+    class LfsIntegrity
+      REV_LIST_OBJECT_LIMIT = 2_000
+
+      def initialize(project, newrev)
+        @project = project
+        @newrev = newrev
+      end
+
+      def objects_missing?
+        return false unless @newrev && @project.lfs_enabled?
+
+        new_lfs_pointers = Gitlab::Git::LfsChanges.new(@project.repository, @newrev).new_pointers(object_limit: REV_LIST_OBJECT_LIMIT)
+
+        return false unless new_lfs_pointers.present?
+
+        existing_count = @project.lfs_objects.where(oid: new_lfs_pointers.map(&:lfs_oid)).count
+
+        existing_count != new_lfs_pointers.count
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb
index 6c25b7349e1..74a24a4424b 100644
--- a/spec/lib/gitlab/checks/change_access_spec.rb
+++ b/spec/lib/gitlab/checks/change_access_spec.rb
@@ -11,13 +11,13 @@ describe Gitlab::Checks::ChangeAccess do
     let(:changes) { { oldrev: oldrev, newrev: newrev, ref: ref } }
     let(:protocol) { 'ssh' }
 
-    subject do
+    subject(:change_access) do
       described_class.new(
         changes,
         project: project,
         user_access: user_access,
         protocol: protocol
-      ).exec
+      )
     end
 
     before do
@@ -26,7 +26,7 @@ describe Gitlab::Checks::ChangeAccess do
 
     context 'without failed checks' do
       it "doesn't raise an error" do
-        expect { subject }.not_to raise_error
+        expect { subject.exec }.not_to raise_error
       end
     end
 
@@ -34,7 +34,7 @@ describe Gitlab::Checks::ChangeAccess do
       it 'raises an error' do
         expect(user_access).to receive(:can_do_action?).with(:push_code).and_return(false)
 
-        expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.')
+        expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.')
       end
     end
 
@@ -45,7 +45,7 @@ describe Gitlab::Checks::ChangeAccess do
         allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true)
         expect(user_access).to receive(:can_do_action?).with(:admin_project).and_return(false)
 
-        expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to change existing tags on this project.')
+        expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to change existing tags on this project.')
       end
 
       context 'with protected tag' do
@@ -61,7 +61,7 @@ describe Gitlab::Checks::ChangeAccess do
             let(:newrev) { '0000000000000000000000000000000000000000' }
 
             it 'is prevented' do
-              expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /cannot be deleted/)
+              expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /cannot be deleted/)
             end
           end
 
@@ -70,7 +70,7 @@ describe Gitlab::Checks::ChangeAccess do
             let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' }
 
             it 'is prevented' do
-              expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /cannot be updated/)
+              expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /cannot be updated/)
             end
           end
         end
@@ -81,14 +81,14 @@ describe Gitlab::Checks::ChangeAccess do
           let(:ref) { 'refs/tags/v9.1.0' }
 
           it 'prevents creation below access level' do
-            expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /allowed to create this tag as it is protected/)
+            expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /allowed to create this tag as it is protected/)
           end
 
           context 'when user has access' do
             let!(:protected_tag) { create(:protected_tag, :developers_can_create, project: project, name: 'v*') }
 
             it 'allows tag creation' do
-              expect { subject }.not_to raise_error
+              expect { subject.exec }.not_to raise_error
             end
           end
         end
@@ -101,7 +101,7 @@ describe Gitlab::Checks::ChangeAccess do
         let(:ref) { 'refs/heads/master' }
 
         it 'raises an error' do
-          expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'The default branch of a project cannot be deleted.')
+          expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'The default branch of a project cannot be deleted.')
         end
       end
 
@@ -114,7 +114,7 @@ describe Gitlab::Checks::ChangeAccess do
         it 'raises an error if the user is not allowed to do forced pushes to protected branches' do
           expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true)
 
-          expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to force push code to a protected branch on this project.')
+          expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to force push code to a protected branch on this project.')
         end
 
         it 'raises an error if the user is not allowed to merge to protected branches' do
@@ -122,13 +122,13 @@ describe Gitlab::Checks::ChangeAccess do
           expect(user_access).to receive(:can_merge_to_branch?).and_return(false)
           expect(user_access).to receive(:can_push_to_branch?).and_return(false)
 
-          expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to merge code into protected branches on this project.')
+          expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to merge code into protected branches on this project.')
         end
 
         it 'raises an error if the user is not allowed to push to protected branches' do
           expect(user_access).to receive(:can_push_to_branch?).and_return(false)
 
-          expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to protected branches on this project.')
+          expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to protected branches on this project.')
         end
 
         context 'branch deletion' do
@@ -137,7 +137,7 @@ describe Gitlab::Checks::ChangeAccess do
 
           context 'if the user is not allowed to delete protected branches' do
             it 'raises an error' do
-              expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.')
+              expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.')
             end
           end
 
@@ -150,18 +150,63 @@ describe Gitlab::Checks::ChangeAccess do
               let(:protocol) { 'web' }
 
               it 'allows branch deletion' do
-                expect { subject }.not_to raise_error
+                expect { subject.exec }.not_to raise_error
               end
             end
 
             context 'over SSH or HTTP' do
               it 'raises an error' do
-                expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only delete protected branches using the web interface.')
+                expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only delete protected branches using the web interface.')
               end
             end
           end
         end
       end
     end
+
+    context 'LFS integrity check' do
+      let(:blob_object) { project.repository.blob_at_branch('lfs', 'files/lfs/lfs_object.iso') }
+
+      before do
+        allow_any_instance_of(Gitlab::Git::RevList).to receive(:new_objects) do |&lazy_block|
+          lazy_block.call([blob_object.id])
+        end
+      end
+
+      context 'with LFS not enabled' do
+        it 'skips integrity check' do
+          expect_any_instance_of(Gitlab::Git::RevList).not_to receive(:new_objects)
+
+          subject.exec
+        end
+      end
+
+      context 'with LFS enabled' do
+        before do
+          allow(project).to receive(:lfs_enabled?).and_return(true)
+        end
+
+        context 'deletion' do
+          let(:changes) { { oldrev: oldrev, ref: ref } }
+
+          it 'skips integrity check' do
+            expect_any_instance_of(Gitlab::Git::RevList).not_to receive(:new_objects)
+
+            subject.exec
+          end
+        end
+
+        it 'fails if any LFS blobs are missing' do
+          expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /LFS objects are missing/)
+        end
+
+        it 'succeeds if LFS objects have already been uploaded' do
+          lfs_object = create(:lfs_object, oid: blob_object.lfs_oid)
+          create(:lfs_objects_project, project: project, lfs_object: lfs_object)
+
+          expect { subject.exec }.not_to raise_error
+        end
+      end
+    end
   end
 end