Port LFS direct_upload from EEac/lfs-direct-upload-ee-to-ce

5 files changed, 500 insertions, 69 deletions

diff --git a/spec/lib/backup/manager_spec.rb b/spec/lib/backup/manager_spec.rb
index 5100f5737c2..84688845fa5 100644
--- a/spec/lib/backup/manager_spec.rb
+++ b/spec/lib/backup/manager_spec.rb
@@ -278,6 +278,10 @@ describe Backup::Manager do
       connection.directories.create(key: Gitlab.config.backup.upload.remote_directory)
     end
 
+    after do
+      Fog.unmock!
+    end
+
     context 'target path' do
       it 'uses the tar filename by default' do
         expect_any_instance_of(Fog::Collection).to receive(:create)
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index f7c04c19903..1e6bd993c08 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -243,17 +243,34 @@ describe 'Git LFS API and storage' do
             it_behaves_like 'responds with a file'
 
             context 'when LFS uses object storage' do
-              let(:before_get) do
-                stub_lfs_object_storage
-                lfs_object.file.migrate!(LfsObjectUploader::Store::REMOTE)
+              context 'when proxy download is enabled' do
+                let(:before_get) do
+                  stub_lfs_object_storage(proxy_download: true)
+                  lfs_object.file.migrate!(LfsObjectUploader::Store::REMOTE)
+                end
+
+                it 'responds with redirect' do
+                  expect(response).to have_gitlab_http_status(200)
+                end
+
+                it 'responds with the workhorse send-url' do
+                  expect(response.headers[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with("send-url:")
+                end
               end
 
-              it 'responds with redirect' do
-                expect(response).to have_gitlab_http_status(302)
-              end
+              context 'when proxy download is disabled' do
+                let(:before_get) do
+                  stub_lfs_object_storage(proxy_download: false)
+                  lfs_object.file.migrate!(LfsObjectUploader::Store::REMOTE)
+                end
+
+                it 'responds with redirect' do
+                  expect(response).to have_gitlab_http_status(302)
+                end
 
-              it 'responds with the file location' do
-                expect(response.location).to include(lfs_object.reload.file.path)
+                it 'responds with the file location' do
+                  expect(response.location).to include(lfs_object.reload.file.path)
+                end
               end
             end
           end
@@ -962,22 +979,61 @@ describe 'Git LFS API and storage' do
           end
 
           context 'and request is sent by gitlab-workhorse to authorize the request' do
-            before do
-              put_authorize
+            shared_examples 'a valid response' do
+              before do
+                put_authorize
+              end
+
+              it 'responds with status 200' do
+                expect(response).to have_gitlab_http_status(200)
+              end
+
+              it 'uses the gitlab-workhorse content type' do
+                expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
+              end
             end
 
-            it 'responds with status 200' do
-              expect(response).to have_gitlab_http_status(200)
+            shared_examples 'a local file' do
+              it_behaves_like 'a valid response' do
+                it 'responds with status 200, location of lfs store and object details' do
+                  expect(json_response['TempPath']).to eq(LfsObjectUploader.workhorse_local_upload_path)
+                  expect(json_response['RemoteObject']).to be_nil
+                  expect(json_response['LfsOid']).to eq(sample_oid)
+                  expect(json_response['LfsSize']).to eq(sample_size)
+                end
+              end
             end
 
-            it 'uses the gitlab-workhorse content type' do
-              expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
+            context 'when using local storage' do
+              it_behaves_like 'a local file'
             end
 
-            it 'responds with status 200, location of lfs store and object details' do
-              expect(json_response['StoreLFSPath']).to eq(LfsObjectUploader.workhorse_upload_path)
-              expect(json_response['LfsOid']).to eq(sample_oid)
-              expect(json_response['LfsSize']).to eq(sample_size)
+            context 'when using remote storage' do
+              context 'when direct upload is enabled' do
+                before do
+                  stub_lfs_object_storage(enabled: true, direct_upload: true)
+                end
+
+                it_behaves_like 'a valid response' do
+                  it 'responds with status 200, location of lfs remote store and object details' do
+                    expect(json_response['TempPath']).to be_nil
+                    expect(json_response['RemoteObject']).to have_key('ID')
+                    expect(json_response['RemoteObject']).to have_key('GetURL')
+                    expect(json_response['RemoteObject']).to have_key('StoreURL')
+                    expect(json_response['RemoteObject']).to have_key('DeleteURL')
+                    expect(json_response['LfsOid']).to eq(sample_oid)
+                    expect(json_response['LfsSize']).to eq(sample_size)
+                  end
+                end
+              end
+
+              context 'when direct upload is disabled' do
+                before do
+                  stub_lfs_object_storage(enabled: true, direct_upload: false)
+                end
+
+                it_behaves_like 'a local file'
+              end
             end
           end
 
@@ -1009,26 +1065,81 @@ describe 'Git LFS API and storage' do
             end
 
             context 'with object storage enabled' do
-              before do
-                stub_lfs_object_storage(background_upload: true)
+              context 'and direct upload enabled' do
+                let!(:fog_connection) do
+                  stub_lfs_object_storage(direct_upload: true)
+                end
+
+                ['123123', '../../123123'].each do |remote_id|
+                  context "with invalid remote_id: #{remote_id}" do
+                    subject do
+                      put_finalize_with_args('file.remote_id' => remote_id)
+                    end
+
+                    it 'responds with status 403' do
+                      subject
+
+                      expect(response).to have_gitlab_http_status(403)
+                    end
+                  end
+                end
+
+                context 'with valid remote_id' do
+                  before do
+                    fog_connection.directories.get('lfs-objects').files.create(
+                      key: 'tmp/upload/12312300',
+                      body: 'content'
+                    )
+                  end
+
+                  subject do
+                    put_finalize_with_args(
+                      'file.remote_id' => '12312300',
+                      'file.name' => 'name')
+                  end
+
+                  it 'responds with status 200' do
+                    subject
+
+                    expect(response).to have_gitlab_http_status(200)
+                  end
+
+                  it 'schedules migration of file to object storage' do
+                    subject
+
+                    expect(LfsObject.last.projects).to include(project)
+                  end
+
+                  it 'have valid file' do
+                    subject
+
+                    expect(LfsObject.last.file_store).to eq(ObjectStorage::Store::REMOTE)
+                    expect(LfsObject.last.file).to be_exists
+                  end
+                end
               end
 
-              it 'schedules migration of file to object storage' do
-                expect(ObjectStorage::BackgroundMoveWorker).to receive(:perform_async).with('LfsObjectUploader', 'LfsObject', :file, kind_of(Numeric))
+              context 'and background upload enabled' do
+                before do
+                  stub_lfs_object_storage(background_upload: true)
+                end
 
-                put_finalize(with_tempfile: true)
+                it 'schedules migration of file to object storage' do
+                  expect(ObjectStorage::BackgroundMoveWorker).to receive(:perform_async).with('LfsObjectUploader', 'LfsObject', :file, kind_of(Numeric))
+
+                  put_finalize(with_tempfile: true)
+                end
               end
             end
           end
 
           context 'invalid tempfiles' do
-            it 'rejects slashes in the tempfile name (path traversal' do
-              put_finalize('foo/bar')
-              expect(response).to have_gitlab_http_status(403)
+            before do
+              lfs_object.destroy
             end
 
-            it 'rejects tempfile names that do not start with the oid' do
-              put_finalize("foo#{sample_oid}")
+            it 'rejects slashes in the tempfile name (path traversal)' do
+              put_finalize('../bar', with_tempfile: true)
               expect(response).to have_gitlab_http_status(403)
             end
           end
@@ -1118,7 +1229,7 @@ describe 'Git LFS API and storage' do
             end
 
             it 'with location of lfs store and object details' do
-              expect(json_response['StoreLFSPath']).to eq(LfsObjectUploader.workhorse_upload_path)
+              expect(json_response['TempPath']).to eq(LfsObjectUploader.workhorse_local_upload_path)
               expect(json_response['LfsOid']).to eq(sample_oid)
               expect(json_response['LfsSize']).to eq(sample_size)
             end
@@ -1221,21 +1332,28 @@ describe 'Git LFS API and storage' do
     end
 
     def put_finalize(lfs_tmp = lfs_tmp_file, with_tempfile: false)
-      setup_tempfile(lfs_tmp) if with_tempfile
+      upload_path = LfsObjectUploader.workhorse_local_upload_path
+      file_path = upload_path + '/' + lfs_tmp if lfs_tmp
 
-      put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}", nil,
-          headers.merge('X-Gitlab-Lfs-Tmp' => lfs_tmp).compact
-    end
+      if with_tempfile
+        FileUtils.mkdir_p(upload_path)
+        FileUtils.touch(file_path)
+      end
 
-    def lfs_tmp_file
-      "#{sample_oid}012345678"
+      args = {
+        'file.path' => file_path,
+        'file.name' => File.basename(file_path)
+      }.compact
+
+      put_finalize_with_args(args)
     end
 
-    def setup_tempfile(lfs_tmp)
-      upload_path = LfsObjectUploader.workhorse_upload_path
+    def put_finalize_with_args(args)
+      put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}", args, headers
+    end
 
-      FileUtils.mkdir_p(upload_path)
-      FileUtils.touch(File.join(upload_path, lfs_tmp))
+    def lfs_tmp_file
+      "#{sample_oid}012345678"
     end
   end
 
diff --git a/spec/support/stub_object_storage.rb b/spec/support/stub_object_storage.rb
index 1a0a2feb27d..6e88641da42 100644
--- a/spec/support/stub_object_storage.rb
+++ b/spec/support/stub_object_storage.rb
@@ -1,17 +1,22 @@
 module StubConfiguration
   def stub_object_storage_uploader(
-        config:, uploader:, remote_directory:,
+        config:,
+        uploader:,
+        remote_directory:,
         enabled: true,
         proxy_download: false,
-        background_upload: false)
-    Fog.mock!
-
+        background_upload: false,
+        direct_upload: false
+  )
     allow(config).to receive(:enabled) { enabled }
     allow(config).to receive(:proxy_download) { proxy_download }
     allow(config).to receive(:background_upload) { background_upload }
+    allow(config).to receive(:direct_upload) { direct_upload }
 
     return unless enabled
 
+    Fog.mock!
+
     ::Fog::Storage.new(uploader.object_store_credentials).tap do |connection|
       begin
         connection.directories.create(key: remote_directory)
diff --git a/spec/uploaders/gitlab_uploader_spec.rb b/spec/uploaders/gitlab_uploader_spec.rb
index 60e35dcf235..4fba122cce1 100644
--- a/spec/uploaders/gitlab_uploader_spec.rb
+++ b/spec/uploaders/gitlab_uploader_spec.rb
@@ -27,7 +27,7 @@ describe GitlabUploader do
   describe '#file_cache_storage?' do
     context 'when file storage is used' do
       before do
-        uploader_class.cache_storage(:file)
+        expect(uploader_class).to receive(:cache_storage) { CarrierWave::Storage::File }
       end
 
       it { is_expected.to be_file_cache_storage }
@@ -35,7 +35,7 @@ describe GitlabUploader do
 
     context 'when is remote storage' do
       before do
-        uploader_class.cache_storage(:fog)
+        expect(uploader_class).to receive(:cache_storage) { CarrierWave::Storage::Fog }
       end
 
       it { is_expected.not_to be_file_cache_storage }
diff --git a/spec/uploaders/object_storage_spec.rb b/spec/uploaders/object_storage_spec.rb
index 489b6707c6e..1d406c71955 100644
--- a/spec/uploaders/object_storage_spec.rb
+++ b/spec/uploaders/object_storage_spec.rb
@@ -21,11 +21,11 @@ describe ObjectStorage do
   let(:object) { build_stubbed(:user) }
   let(:uploader) { uploader_class.new(object, :file) }
 
-  before do
-    allow(uploader_class).to receive(:object_store_enabled?).and_return(true)
-  end
-
   describe '#object_store=' do
+    before do
+      allow(uploader_class).to receive(:object_store_enabled?).and_return(true)
+    end
+
     it "reload the local storage" do
       uploader.object_store = described_class::Store::LOCAL
       expect(uploader.file_storage?).to be_truthy
@@ -35,28 +35,28 @@ describe ObjectStorage do
       uploader.object_store = described_class::Store::REMOTE
       expect(uploader.file_storage?).to be_falsey
     end
-  end
 
-  context 'object_store is Store::LOCAL' do
-    before do
-      uploader.object_store = described_class::Store::LOCAL
-    end
+    context 'object_store is Store::LOCAL' do
+      before do
+        uploader.object_store = described_class::Store::LOCAL
+      end
 
-    describe '#store_dir' do
-      it 'is the composition of (base_dir, dynamic_segment)' do
-        expect(uploader.store_dir).to start_with("uploads/-/system/user/")
+      describe '#store_dir' do
+        it 'is the composition of (base_dir, dynamic_segment)' do
+          expect(uploader.store_dir).to start_with("uploads/-/system/user/")
+        end
       end
     end
-  end
 
-  context 'object_store is Store::REMOTE' do
-    before do
-      uploader.object_store = described_class::Store::REMOTE
-    end
+    context 'object_store is Store::REMOTE' do
+      before do
+        uploader.object_store = described_class::Store::REMOTE
+      end
 
-    describe '#store_dir' do
-      it 'is the composition of (dynamic_segment)' do
-        expect(uploader.store_dir).to start_with("user/")
+      describe '#store_dir' do
+        it 'is the composition of (dynamic_segment)' do
+          expect(uploader.store_dir).to start_with("user/")
+        end
       end
     end
   end
@@ -92,7 +92,7 @@ describe ObjectStorage do
   describe '#file_cache_storage?' do
     context 'when file storage is used' do
       before do
-        uploader_class.cache_storage(:file)
+        expect(uploader_class).to receive(:cache_storage) { CarrierWave::Storage::File }
       end
 
       it { expect(uploader).to be_file_cache_storage }
@@ -100,7 +100,7 @@ describe ObjectStorage do
 
     context 'when is remote storage' do
       before do
-        uploader_class.cache_storage(:fog)
+        expect(uploader_class).to receive(:cache_storage) { CarrierWave::Storage::Fog }
       end
 
       it { expect(uploader).not_to be_file_cache_storage }
@@ -298,7 +298,9 @@ describe ObjectStorage do
     let(:remote_directory) { 'directory' }
 
     before do
-      uploader_class.storage_options double(object_store: double(remote_directory: remote_directory))
+      allow(uploader_class).to receive(:options) do
+        double(object_store: double(remote_directory: remote_directory))
+      end
     end
 
     subject { uploader.fog_directory }
@@ -310,7 +312,9 @@ describe ObjectStorage do
     let(:connection) { Settingslogic.new("provider" => "AWS") }
 
     before do
-      uploader_class.storage_options double(object_store: double(connection: connection))
+      allow(uploader_class).to receive(:options) do
+        double(object_store: double(connection: connection))
+      end
     end
 
     subject { uploader.fog_credentials }
@@ -323,4 +327,304 @@ describe ObjectStorage do
 
     it { is_expected.to eq(false) }
   end
+
+  describe '.workhorse_authorize' do
+    subject { uploader_class.workhorse_authorize }
+
+    before do
+      # ensure that we use regular Fog libraries
+      # other tests might call `Fog.mock!` and
+      # it will make tests to fail
+      Fog.unmock!
+    end
+
+    shared_examples 'uses local storage' do
+      it "returns temporary path" do
+        is_expected.to have_key(:TempPath)
+
+        expect(subject[:TempPath]).to start_with(uploader_class.root)
+        expect(subject[:TempPath]).to include(described_class::TMP_UPLOAD_PATH)
+      end
+
+      it "does not return remote store" do
+        is_expected.not_to have_key('RemoteObject')
+      end
+    end
+
+    shared_examples 'uses remote storage' do
+      it "returns remote store" do
+        is_expected.to have_key(:RemoteObject)
+
+        expect(subject[:RemoteObject]).to have_key(:ID)
+        expect(subject[:RemoteObject]).to have_key(:GetURL)
+        expect(subject[:RemoteObject]).to have_key(:DeleteURL)
+        expect(subject[:RemoteObject]).to have_key(:StoreURL)
+        expect(subject[:RemoteObject][:GetURL]).to include(described_class::TMP_UPLOAD_PATH)
+        expect(subject[:RemoteObject][:DeleteURL]).to include(described_class::TMP_UPLOAD_PATH)
+        expect(subject[:RemoteObject][:StoreURL]).to include(described_class::TMP_UPLOAD_PATH)
+      end
+
+      it "does not return local store" do
+        is_expected.not_to have_key('TempPath')
+      end
+    end
+
+    context 'when object storage is disabled' do
+      before do
+        allow(Gitlab.config.uploads.object_store).to receive(:enabled) { false }
+      end
+
+      it_behaves_like 'uses local storage'
+    end
+
+    context 'when object storage is enabled' do
+      before do
+        allow(Gitlab.config.uploads.object_store).to receive(:enabled) { true }
+      end
+
+      context 'when direct upload is enabled' do
+        before do
+          allow(Gitlab.config.uploads.object_store).to receive(:direct_upload) { true }
+        end
+
+        context 'uses AWS' do
+          before do
+            expect(uploader_class).to receive(:object_store_credentials) do
+              { provider: "AWS",
+                aws_access_key_id: "AWS_ACCESS_KEY_ID",
+                aws_secret_access_key: "AWS_SECRET_ACCESS_KEY",
+                region: "eu-central-1" }
+            end
+          end
+
+          it_behaves_like 'uses remote storage' do
+            let(:storage_url) { "https://uploads.s3-eu-central-1.amazonaws.com/" }
+
+            it 'returns links for S3' do
+              expect(subject[:RemoteObject][:GetURL]).to start_with(storage_url)
+              expect(subject[:RemoteObject][:DeleteURL]).to start_with(storage_url)
+              expect(subject[:RemoteObject][:StoreURL]).to start_with(storage_url)
+            end
+          end
+        end
+
+        context 'uses Google' do
+          before do
+            expect(uploader_class).to receive(:object_store_credentials) do
+              { provider: "Google",
+                google_storage_access_key_id: 'ACCESS_KEY_ID',
+                google_storage_secret_access_key: 'SECRET_ACCESS_KEY' }
+            end
+          end
+
+          it_behaves_like 'uses remote storage' do
+            let(:storage_url) { "https://storage.googleapis.com/uploads/" }
+
+            it 'returns links for Google Cloud' do
+              expect(subject[:RemoteObject][:GetURL]).to start_with(storage_url)
+              expect(subject[:RemoteObject][:DeleteURL]).to start_with(storage_url)
+              expect(subject[:RemoteObject][:StoreURL]).to start_with(storage_url)
+            end
+          end
+        end
+
+        context 'uses GDK/minio' do
+          before do
+            expect(uploader_class).to receive(:object_store_credentials) do
+              { provider: "AWS",
+                aws_access_key_id: "AWS_ACCESS_KEY_ID",
+                aws_secret_access_key: "AWS_SECRET_ACCESS_KEY",
+                endpoint: 'http://127.0.0.1:9000',
+                path_style: true,
+                region: "gdk" }
+            end
+          end
+
+          it_behaves_like 'uses remote storage' do
+            let(:storage_url) { "http://127.0.0.1:9000/uploads/" }
+
+            it 'returns links for S3' do
+              expect(subject[:RemoteObject][:GetURL]).to start_with(storage_url)
+              expect(subject[:RemoteObject][:DeleteURL]).to start_with(storage_url)
+              expect(subject[:RemoteObject][:StoreURL]).to start_with(storage_url)
+            end
+          end
+        end
+      end
+
+      context 'when direct upload is disabled' do
+        before do
+          allow(Gitlab.config.uploads.object_store).to receive(:direct_upload) { false }
+        end
+
+        it_behaves_like 'uses local storage'
+      end
+    end
+  end
+
+  describe '#store_workhorse_file!' do
+    subject do
+      uploader.store_workhorse_file!(params, :file)
+    end
+
+    context 'when local file is used' do
+      context 'when valid file is used' do
+        let(:target_path) do
+          File.join(uploader_class.root, uploader_class::TMP_UPLOAD_PATH)
+        end
+
+        before do
+          FileUtils.mkdir_p(target_path)
+        end
+
+        context 'when no filename is specified' do
+          let(:params) do
+            { "file.path" => "test/file" }
+          end
+
+          it 'raises an error' do
+            expect { subject }.to raise_error(uploader_class::RemoteStoreError, /Missing filename/)
+          end
+        end
+
+        context 'when invalid file is specified' do
+          let(:file_path) do
+            File.join(target_path, "..", "test.file")
+          end
+
+          before do
+            FileUtils.touch(file_path)
+          end
+
+          let(:params) do
+            { "file.path" => file_path,
+              "file.name" => "my_file.txt" }
+          end
+
+          it 'raises an error' do
+            expect { subject }.to raise_error(uploader_class::RemoteStoreError, /Bad file path/)
+          end
+        end
+
+        context 'when filename is specified' do
+          let(:params) do
+            { "file.path" => tmp_file,
+              "file.name" => "my_file.txt" }
+          end
+
+          let(:tmp_file) { Tempfile.new('filename', target_path) }
+
+          before do
+            FileUtils.touch(tmp_file)
+          end
+
+          after do
+            FileUtils.rm_f(tmp_file)
+          end
+
+          it 'succeeds' do
+            expect { subject }.not_to raise_error
+
+            expect(uploader).to be_exists
+          end
+
+          it 'proper path is being used' do
+            subject
+
+            expect(uploader.path).to start_with(uploader_class.root)
+            expect(uploader.path).to end_with("my_file.txt")
+          end
+
+          it 'source file to not exist' do
+            subject
+
+            expect(File.exist?(tmp_file.path)).to be_falsey
+          end
+        end
+      end
+    end
+
+    context 'when remote file is used' do
+      let!(:fog_connection) do
+        stub_uploads_object_storage(uploader_class)
+      end
+
+      context 'when valid file is used' do
+        context 'when no filename is specified' do
+          let(:params) do
+            { "file.remote_id" => "test/123123" }
+          end
+
+          it 'raises an error' do
+            expect { subject }.to raise_error(uploader_class::RemoteStoreError, /Missing filename/)
+          end
+        end
+
+        context 'when invalid file is specified' do
+          let(:params) do
+            { "file.remote_id" => "../test/123123",
+              "file.name" => "my_file.txt" }
+          end
+
+          it 'raises an error' do
+            expect { subject }.to raise_error(uploader_class::RemoteStoreError, /Bad file path/)
+          end
+        end
+
+        context 'when non existing file is specified' do
+          let(:params) do
+            { "file.remote_id" => "test/12312300",
+              "file.name" => "my_file.txt" }
+          end
+
+          it 'raises an error' do
+            expect { subject }.to raise_error(uploader_class::RemoteStoreError, /Missing file/)
+          end
+        end
+
+        context 'when filename is specified' do
+          let(:params) do
+            { "file.remote_id" => "test/123123",
+              "file.name" => "my_file.txt" }
+          end
+
+          let!(:fog_file) do
+            fog_connection.directories.get('uploads').files.create(
+              key: 'tmp/upload/test/123123',
+              body: 'content'
+            )
+          end
+
+          it 'succeeds' do
+            expect { subject }.not_to raise_error
+
+            expect(uploader).to be_exists
+          end
+
+          it 'path to not be temporary' do
+            subject
+
+            expect(uploader.path).not_to be_nil
+            expect(uploader.path).not_to include('tmp/upload')
+            expect(uploader.url).to include('/my_file.txt')
+          end
+
+          it 'url is used' do
+            subject
+
+            expect(uploader.url).not_to be_nil
+            expect(uploader.url).to include('/my_file.txt')
+          end
+        end
+      end
+    end
+
+    context 'when no file is used' do
+      let(:params) { {} }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error(uploader_class::RemoteStoreError, /Bad file/)
+      end
+    end
+  end
 end