Merge branch 'security-support-object-storage-at-file-mover-11-11' into '11-11-stable'

Support object storage at FileMover class

See merge request gitlab/gitlabhq!3196

app/uploaders/file_mover.rb

 # frozen_string_literal: true
 
 class FileMover
+  include Gitlab::Utils::StrongMemoize
+
   attr_reader :secret, :file_name, :from_model, :to_model, :update_field
 
   def initialize(file_path, update_field = :description, from_model:, to_model:)
@@ -12,8 +14,12 @@ class FileMover
   end
 
   def execute
+    temp_file_uploader.retrieve_from_store!(file_name)
+
     return unless valid?
 
+    uploader.retrieve_from_store!(file_name)
+
     move
 
     if update_markdown
@@ -25,14 +31,23 @@ class FileMover
   private
 
   def valid?
+    if temp_file_uploader.file_storage?
       Pathname.new(temp_file_path).realpath.to_path.start_with?(
         (Pathname(temp_file_uploader.root) + temp_file_uploader.base_dir).to_path
       )
+    else
+      temp_file_uploader.exists?
+    end
   end
 
   def move
+    if temp_file_uploader.file_storage?
       FileUtils.mkdir_p(File.dirname(file_path))
       FileUtils.move(temp_file_path, file_path)
+    else
+      uploader.copy_file(temp_file_uploader.file)
+      temp_file_uploader.upload.destroy!
+    end
   end
 
   def update_markdown
@@ -46,28 +61,36 @@ class FileMover
 
   def update_upload_model
     return unless upload = temp_file_uploader.upload
+    return if upload.destroyed?
 
-    upload.update!(model_id: to_model.id, model_type: to_model.type)
+    upload.update!(model: to_model)
   end
 
   def temp_file_path
-    return @temp_file_path if @temp_file_path
-
-    temp_file_uploader.retrieve_from_store!(file_name)
-
-    @temp_file_path = temp_file_uploader.file.path
+    strong_memoize(:temp_file_path) do
+      temp_file_uploader.file.path
+    end
   end
 
   def file_path
-    return @file_path if @file_path
-
-    uploader.retrieve_from_store!(file_name)
-
-    @file_path = uploader.file.path
+    strong_memoize(:file_path) do
+      uploader.file.path
+    end
   end
 
   def uploader
-    @uploader ||= PersonalFileUploader.new(to_model, secret: secret)
+    @uploader ||=
+      begin
+        uploader = PersonalFileUploader.new(to_model, secret: secret)
+
+        # Enforcing a REMOTE object storage given FileUploader#retrieve_from_store! won't do it
+        # (there's no upload at the target yet).
+        if uploader.class.object_store_enabled?
+          uploader.object_store = ::ObjectStorage::Store::REMOTE
+        end
+
+        uploader
+      end
   end
 
   def temp_file_uploader
@@ -77,6 +100,8 @@ class FileMover
   def revert
     Rails.logger.warn("Markdown not updated, file move reverted for #{to_model}")
 
+    if temp_file_uploader.file_storage?
       FileUtils.move(file_path, temp_file_path)
     end
+  end
 end

spec/uploaders/file_mover_spec.rb

@@ -23,34 +23,35 @@ describe FileMover do
   subject { described_class.new(temp_file_path, from_model: user, to_model: snippet).execute }
 
   describe '#execute' do
+    let(:tmp_upload) { tmp_uploader.upload }
+
     before do
       tmp_uploader.store!(file)
+    end
 
-      expect(FileUtils).to receive(:mkdir_p).with(a_string_including(File.dirname(file_path)))
-      expect(FileUtils).to receive(:move).with(a_string_including(temp_file_path), a_string_including(file_path))
+    context 'local storage' do
+      before do
+        allow(FileUtils).to receive(:mkdir_p).with(a_string_including(File.dirname(file_path)))
+        allow(FileUtils).to receive(:move).with(a_string_including(temp_file_path), a_string_including(file_path))
         allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?).and_return(true)
         allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:size).and_return(10)
 
         stub_file_mover(temp_file_path)
       end
 
-    let(:tmp_upload) { tmp_uploader.upload }
-
       context 'when move and field update successful' do
         it 'updates the description correctly' do
           subject
 
           expect(snippet.reload.description)
-          .to eq(
-            "test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
-            "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "
-          )
+            .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
+                   "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ")
         end
 
         it 'updates existing upload record' do
           expect { subject }
             .to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') }
-          .from([user.id, 'User']).to([snippet.id, 'PersonalSnippet'])
+            .from([user.id, 'User']).to([snippet.id, 'Snippet'])
         end
 
         it 'schedules a background migration' do
@@ -71,10 +72,8 @@ describe FileMover do
           subject
 
           expect(snippet.reload.description)
-          .to eq(
-            "test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
-            "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "
-          )
+            .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
+                   "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ")
         end
 
         it 'does not change the upload record' do
@@ -84,6 +83,54 @@ describe FileMover do
       end
     end
 
+    context 'when tmp uploader is not local storage' do
+      before do
+        allow(PersonalFileUploader).to receive(:object_store_enabled?) { true }
+        tmp_uploader.object_store = ObjectStorage::Store::REMOTE
+        allow_any_instance_of(PersonalFileUploader).to receive(:file_storage?) { false }
+      end
+
+      after do
+        FileUtils.rm_f(File.join('personal_snippet', snippet.id.to_s, secret, filename))
+      end
+
+      context 'when move and field update successful' do
+        it 'updates the description correctly' do
+          subject
+
+          expect(snippet.reload.description)
+            .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
+                   "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ")
+        end
+
+        it 'creates new target upload record an delete the old upload' do
+          expect { subject }
+            .to change { Upload.last.attributes.values_at('model_id', 'model_type') }
+            .from([user.id, 'User']).to([snippet.id, 'Snippet'])
+
+          expect(Upload.count).to eq(1)
+        end
+      end
+
+      context 'when update_markdown fails' do
+        subject { described_class.new(file_path, :non_existing_field, from_model: user, to_model: snippet).execute }
+
+        it 'does not update the description' do
+          subject
+
+          expect(snippet.reload.description)
+            .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
+                   "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ")
+        end
+
+        it 'does not change the upload record' do
+          expect { subject }
+            .to change { Upload.last.attributes.values_at('model_id', 'model_type') }.from([user.id, 'User'])
+        end
+      end
+    end
+  end
+
   context 'security' do
     context 'when relative path is involved' do
       let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", '..', 'another_subdir_of_temp') }