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

Support object storage at FileMover class

See merge request gitlab/gitlabhq!3195

2 files changed, 126 insertions, 54 deletions

diff --git a/app/uploaders/file_mover.rb b/app/uploaders/file_mover.rb
index dcf1e8792ad..12be1e2bb22 100644
--- a/app/uploaders/file_mover.rb
+++ b/app/uploaders/file_mover.rb
@@ -1,6 +1,8 @@
 # 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?
-    Pathname.new(temp_file_path).realpath.to_path.start_with?(
-      (Pathname(temp_file_uploader.root) + temp_file_uploader.base_dir).to_path
-    )
+    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
-    FileUtils.mkdir_p(File.dirname(file_path))
-    FileUtils.move(temp_file_path, file_path)
+    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}")
 
-    FileUtils.move(file_path, temp_file_path)
+    if temp_file_uploader.file_storage?
+      FileUtils.move(file_path, temp_file_path)
+    end
   end
 end
diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb
index d5e8a90cecd..a9e03f3d4e5 100644
--- a/spec/uploaders/file_mover_spec.rb
+++ b/spec/uploaders/file_mover_spec.rb
@@ -23,63 +23,110 @@ 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))
-      allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?).and_return(true)
-      allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:size).and_return(10)
+    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
+        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
 
-    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
 
-        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 '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, 'Snippet'])
+        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'])
+        it 'schedules a background migration' do
+          expect_any_instance_of(PersonalFileUploader).to receive(:schedule_background_upload).once
+
+          subject
+        end
       end
 
-      it 'schedules a background migration' do
-        expect_any_instance_of(PersonalFileUploader).to receive(:schedule_background_upload).once
+      context 'when update_markdown fails' do
+        before do
+          expect(FileUtils).to receive(:move).with(a_string_including(file_path), a_string_including(temp_file_path))
+        end
 
-        subject
+        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 }
+            .not_to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') }
+        end
       end
     end
 
-    context 'when update_markdown fails' do
+    context 'when tmp uploader is not local storage' do
       before do
-        expect(FileUtils).to receive(:move).with(a_string_including(file_path), a_string_including(temp_file_path))
+        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
 
-      subject { described_class.new(file_path, :non_existing_field, from_model: user, to_model: snippet).execute }
+      after do
+        FileUtils.rm_f(File.join('personal_snippet', snippet.id.to_s, secret, filename))
+      end
 
-      it 'does not update the description' do
-        subject
+      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/user/#{user.id}/secret55/banana_sample.gif) "\
-            "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "
-          )
+          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
 
-      it 'does not change the upload record' do
-        expect { subject }
-          .not_to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') }
+      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