Merge branch 'security-deprecate-lfs-link-service' into 'master'

Remove OID filtering during LFS imports

See merge request gitlab-org/security/gitlab!188

app/services/projects/lfs_pointers/lfs_download_service.rb

@@ -16,17 +16,14 @@ module Projects
         @lfs_download_object = lfs_download_object
       end
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def execute
         return unless project&.lfs_enabled? && lfs_download_object
         return error("LFS file with oid #{lfs_oid} has invalid attributes") unless lfs_download_object.valid?
-        return if LfsObject.exists?(oid: lfs_oid)
 
         wrap_download_errors do
           download_lfs_file!
         end
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
       private
 
@@ -39,14 +36,24 @@ module Projects
       def download_lfs_file!
         with_tmp_file do |tmp_file|
           download_and_save_file!(tmp_file)
-          project.lfs_objects << LfsObject.new(oid: lfs_oid,
-                                               size: lfs_size,
-                                               file: tmp_file)
+
+          project.lfs_objects << find_or_create_lfs_object(tmp_file)
 
           success
         end
       end
 
+      def find_or_create_lfs_object(tmp_file)
+        lfs_obj = LfsObject.safe_find_or_create_by!(
+          oid:  lfs_oid,
+          size: lfs_size
+        )
+
+        lfs_obj.update!(file: tmp_file) unless lfs_obj.file.file
+
+        lfs_obj
+      end
+
       def download_and_save_file!(file)
         digester = Digest::SHA256.new
         response = Gitlab::HTTP.get(lfs_sanitized_url, download_headers) do |fragment|

app/services/projects/lfs_pointers/lfs_object_download_list_service.rb

@@ -26,12 +26,12 @@ module Projects
           return []
         end
 
-        # Getting all Lfs pointers already in the database and linking them to the project
-        linked_oids = LfsLinkService.new(project).execute(lfs_pointers_in_repository.keys)
-        # Retrieving those oids not present in the database which we need to download
-        missing_oids = lfs_pointers_in_repository.except(*linked_oids)
-        # Downloading the required information and gathering it inside a LfsDownloadObject for each oid
-        LfsDownloadLinkListService.new(project, remote_uri: current_endpoint_uri).execute(missing_oids)
+        # Downloading the required information and gathering it inside an
+        #   LfsDownloadObject for each oid
+        #
+        LfsDownloadLinkListService
+          .new(project, remote_uri: current_endpoint_uri)
+          .execute(lfs_pointers_in_repository)
       rescue LfsDownloadLinkListService::DownloadLinksError => e
         raise LfsObjectDownloadListError, "The LFS objects download list couldn't be imported. Error: #{e.message}"
       end

changelogs/unreleased/security-deprecate-lfs-link-service.yml

+---
+title: Remove OID filtering during LFS imports
+merge_request:
+author:
+type: security

spec/services/projects/lfs_pointers/lfs_download_service_spec.rb

@@ -134,6 +134,21 @@ describe Projects::LfsPointers::LfsDownloadService do
       end
     end
 
+    context 'when an lfs object with the same oid already exists' do
+      let!(:existing_lfs_object) { create(:lfs_object, oid: oid) }
+
+      before do
+        stub_full_request(download_link).to_return(body: lfs_content)
+      end
+
+      it_behaves_like 'no lfs object is created'
+
+      it 'does not update the file attached to the existing LfsObject' do
+        expect { subject.execute }
+          .not_to change { existing_lfs_object.reload.file.file.file }
+      end
+    end
+
     context 'when credentials present' do
       let(:download_link_with_credentials) { "http://user:password@gitlab.com/#{oid}" }
       let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link_with_credentials) }
@@ -211,17 +226,5 @@ describe Projects::LfsPointers::LfsDownloadService do
         subject.execute
       end
     end
-
-    context 'when an lfs object with the same oid already exists' do
-      before do
-        create(:lfs_object, oid: oid)
-      end
-
-      it 'does not download the file' do
-        expect(subject).not_to receive(:download_lfs_file!)
-
-        subject.execute
-      end
-    end
   end
 end

spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb

@@ -24,7 +24,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do
   describe '#execute' do
     context 'when no lfs pointer is linked' do
       before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return([])
         allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links)
         expect(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:new).with(project, remote_uri: URI.parse(default_endpoint)).and_call_original
       end
@@ -35,12 +34,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do
         subject.execute
       end
 
-      it 'links existent lfs objects to the project' do
-        expect_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute)
-
-        subject.execute
-      end
-
       it 'retrieves the download links of non existent objects' do
         expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(all_oids)
 
@@ -48,32 +41,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do
       end
     end
 
-    context 'when some lfs objects are linked' do
-      before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(existing_lfs_objects.keys)
-        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links)
-      end
-
-      it 'retrieves the download links of non existent objects' do
-        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(oids)
-
-        subject.execute
-      end
-    end
-
-    context 'when all lfs objects are linked' do
-      before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(all_oids.keys)
-        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute)
-      end
-
-      it 'retrieves no download links' do
-        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with({}).and_call_original
-
-        expect(subject.execute).to be_empty
-      end
-    end
-
     context 'when lfsconfig file exists' do
       before do
         allow(project.repository).to receive(:lfsconfig_for).and_return("[lfs]\n\turl = #{lfs_endpoint}\n")