diff options
Diffstat (limited to 'spec')
| -rw-r--r-- | spec/lib/backup/manager_spec.rb | 4 | ||||
| -rw-r--r-- | spec/requests/lfs_http_spec.rb | 196 | ||||
| -rw-r--r-- | spec/support/stub_object_storage.rb | 13 | ||||
| -rw-r--r-- | spec/uploaders/gitlab_uploader_spec.rb | 4 | ||||
| -rw-r--r-- | spec/uploaders/object_storage_spec.rb | 352 |
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 |