From a7b7a2253c41267fb67d8f56b4c2f92293601a9d Mon Sep 17 00:00:00 2001
From: James Edwards-Jones <jedwardsjones@gitlab.com>
Date: Fri, 25 Aug 2017 02:30:12 +0100
Subject: [PATCH] Prevent git push when LFS objects are missing

---
 ...j-fs-prevent-push-when-missing-objects.yml |  5 ++
 lib/gitlab/checks/change_access.rb            | 12 +++-
 lib/gitlab/checks/lfs_integrity.rb            | 24 ++++++++
 spec/lib/gitlab/checks/change_access_spec.rb  | 55 ++++++++++++++++++-
 4 files changed, 93 insertions(+), 3 deletions(-)
 create mode 100644 changelogs/unreleased/jej-fs-prevent-push-when-missing-objects.yml
 create mode 100644 lib/gitlab/checks/lfs_integrity.rb

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..3a7d3f2176b 100644
--- a/spec/lib/gitlab/checks/change_access_spec.rb
+++ b/spec/lib/gitlab/checks/change_access_spec.rb
@@ -11,13 +11,19 @@ describe Gitlab::Checks::ChangeAccess do
     let(:changes) { { oldrev: oldrev, newrev: newrev, ref: ref } }
     let(:protocol) { 'ssh' }
 
-    subject do
+    let(:change_access) do
       described_class.new(
         changes,
         project: project,
         user_access: user_access,
         protocol: protocol
-      ).exec
+      )
+    end
+
+    subject do
+      # TODO: Replace use of `subject` with `subject.exec`
+      # Then rename change_access back to subject
+      change_access.exec
     end
 
     before do
@@ -163,5 +169,50 @@ describe Gitlab::Checks::ChangeAccess do
         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)
+
+          change_access.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)
+
+            change_access.exec
+          end
+        end
+
+        it 'fails if any LFS blobs are missing' do
+          expect { change_access.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 { change_access.exec }.not_to raise_error
+        end
+      end
+    end
   end
 end
-- 
GitLab