diff options
Diffstat (limited to 'spec')
6 files changed, 240 insertions, 53 deletions
diff --git a/spec/lib/gitlab/github_import/importer/lfs_object_importer_spec.rb b/spec/lib/gitlab/github_import/importer/lfs_object_importer_spec.rb index 4857f2afbe2..8fd328d9c1e 100644 --- a/spec/lib/gitlab/github_import/importer/lfs_object_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/lfs_object_importer_spec.rb @@ -2,20 +2,26 @@ require 'spec_helper' describe Gitlab::GithubImport::Importer::LfsObjectImporter do let(:project) { create(:project) } - let(:download_link) { "http://www.gitlab.com/lfs_objects/oid" } - - let(:github_lfs_object) do - Gitlab::GithubImport::Representation::LfsObject.new( - oid: 'oid', download_link: download_link - ) + let(:lfs_attributes) do + { + oid: 'oid', + size: 1, + link: 'http://www.gitlab.com/lfs_objects/oid' + } end + let(:lfs_download_object) { LfsDownloadObject.new(lfs_attributes) } + let(:github_lfs_object) { Gitlab::GithubImport::Representation::LfsObject.new(lfs_attributes) } + let(:importer) { described_class.new(github_lfs_object, project, nil) } describe '#execute' do it 'calls the LfsDownloadService with the lfs object attributes' do - expect_any_instance_of(Projects::LfsPointers::LfsDownloadService) - .to receive(:execute).with('oid', download_link) + allow(importer).to receive(:lfs_download_object).and_return(lfs_download_object) + + service = double + expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).with(project, lfs_download_object).and_return(service) + expect(service).to receive(:execute) importer.execute end diff --git a/spec/lib/gitlab/github_import/importer/lfs_objects_importer_spec.rb b/spec/lib/gitlab/github_import/importer/lfs_objects_importer_spec.rb index 5f5c6b803c0..50442552eee 100644 --- a/spec/lib/gitlab/github_import/importer/lfs_objects_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/lfs_objects_importer_spec.rb @@ -5,7 +5,15 @@ describe Gitlab::GithubImport::Importer::LfsObjectsImporter do let(:client) { double(:client) } let(:download_link) { "http://www.gitlab.com/lfs_objects/oid" } - let(:github_lfs_object) { ['oid', download_link] } + let(:lfs_attributes) do + { + oid: 'oid', + size: 1, + link: 'http://www.gitlab.com/lfs_objects/oid' + } + end + + let(:lfs_download_object) { LfsDownloadObject.new(lfs_attributes) } describe '#parallel?' do it 'returns true when running in parallel mode' do @@ -48,7 +56,7 @@ describe Gitlab::GithubImport::Importer::LfsObjectsImporter do allow(importer) .to receive(:each_object_to_import) - .and_yield(['oid', download_link]) + .and_yield(lfs_download_object) expect(Gitlab::GithubImport::Importer::LfsObjectImporter) .to receive(:new) @@ -71,7 +79,7 @@ describe Gitlab::GithubImport::Importer::LfsObjectsImporter do allow(importer) .to receive(:each_object_to_import) - .and_yield(github_lfs_object) + .and_yield(lfs_download_object) expect(Gitlab::GithubImport::ImportLfsObjectWorker) .to receive(:perform_async) diff --git a/spec/models/lfs_download_object_spec.rb b/spec/models/lfs_download_object_spec.rb new file mode 100644 index 00000000000..88838b127d2 --- /dev/null +++ b/spec/models/lfs_download_object_spec.rb @@ -0,0 +1,68 @@ +require 'rails_helper' + +describe LfsDownloadObject do + let(:oid) { 'cd293be6cea034bd45a0352775a219ef5dc7825ce55d1f7dae9762d80ce64411' } + let(:link) { 'http://www.example.com' } + let(:size) { 1 } + + subject { described_class.new(oid: oid, size: size, link: link) } + + describe 'validations' do + it { is_expected.to validate_numericality_of(:size).is_greater_than_or_equal_to(0) } + + context 'oid attribute' do + it 'must be 64 characters long' do + aggregate_failures do + expect(described_class.new(oid: 'a' * 63, size: size, link: link)).to be_invalid + expect(described_class.new(oid: 'a' * 65, size: size, link: link)).to be_invalid + expect(described_class.new(oid: 'a' * 64, size: size, link: link)).to be_valid + end + end + + it 'must contain only hexadecimal characters' do + aggregate_failures do + expect(subject).to be_valid + expect(described_class.new(oid: 'g' * 64, size: size, link: link)).to be_invalid + end + end + end + + context 'link attribute' do + it 'only http and https protocols are valid' do + aggregate_failures do + expect(described_class.new(oid: oid, size: size, link: 'http://www.example.com')).to be_valid + expect(described_class.new(oid: oid, size: size, link: 'https://www.example.com')).to be_valid + expect(described_class.new(oid: oid, size: size, link: 'ftp://www.example.com')).to be_invalid + expect(described_class.new(oid: oid, size: size, link: 'ssh://www.example.com')).to be_invalid + expect(described_class.new(oid: oid, size: size, link: 'git://www.example.com')).to be_invalid + end + end + + it 'cannot be empty' do + expect(described_class.new(oid: oid, size: size, link: '')).not_to be_valid + end + + context 'when localhost or local network addresses' do + subject { described_class.new(oid: oid, size: size, link: 'http://192.168.1.1') } + + before do + allow(ApplicationSetting) + .to receive(:current) + .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: setting)) + end + + context 'are allowed' do + let(:setting) { true } + + it { expect(subject).to be_valid } + end + + context 'are not allowed' do + let(:setting) { false } + + it { expect(subject).to be_invalid } + end + end + end + end +end diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index 06f865dc848..c525de6cd25 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -152,8 +152,11 @@ describe Projects::ImportService do it 'downloads lfs objects if lfs_enabled is enabled for project' do allow(project).to receive(:lfs_enabled?).and_return(true) + + service = double expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(oid_download_links) - expect_any_instance_of(Projects::LfsPointers::LfsDownloadService).to receive(:execute).twice + expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).and_return(service).twice + expect(service).to receive(:execute).twice subject.execute end @@ -211,8 +214,10 @@ describe Projects::ImportService do it 'does not have a custom repository importer downloads lfs objects' do allow(Gitlab::GithubImport::ParallelImporter).to receive(:imports_repository?).and_return(false) + service = double expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(oid_download_links) - expect_any_instance_of(Projects::LfsPointers::LfsDownloadService).to receive(:execute) + expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).and_return(service).twice + expect(service).to receive(:execute).twice subject.execute end diff --git a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb index d7a2829d5f8..f222c52199f 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb @@ -37,8 +37,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do describe '#execute' do it 'retrieves each download link of every non existent lfs object' do - subject.execute(new_oids).each do |oid, link| - expect(link).to eq "#{import_url}/gitlab-lfs/objects/#{oid}" + subject.execute(new_oids).each do |lfs_download_object| + expect(lfs_download_object.link).to eq "#{import_url}/gitlab-lfs/objects/#{lfs_download_object.oid}" end end @@ -50,8 +50,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do it 'adds credentials to the download_link' do result = subject.execute(new_oids) - result.each do |oid, link| - expect(link.starts_with?('http://user:password@')).to be_truthy + result.each do |lfs_download_object| + expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_truthy end end end @@ -60,8 +60,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do it 'does not add any credentials' do result = subject.execute(new_oids) - result.each do |oid, link| - expect(link.starts_with?('http://user:password@')).to be_falsey + result.each do |lfs_download_object| + expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_falsey end end end @@ -74,8 +74,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do it 'downloads without any credentials' do result = subject.execute(new_oids) - result.each do |oid, link| - expect(link.starts_with?('http://user:password@')).to be_falsey + result.each do |lfs_download_object| + expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_falsey end end end @@ -92,7 +92,7 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do describe '#parse_response_links' do it 'does not add oid entry if href not found' do - expect(Rails.logger).to receive(:error).with("Link for Lfs Object with oid whatever not found or invalid.") + expect(subject).to receive(:log_error).with("Link for Lfs Object with oid whatever not found or invalid.") result = subject.send(:parse_response_links, invalid_object_response) diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index 95c9b6e63b8..d952d1d1ffd 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -2,68 +2,156 @@ require 'spec_helper' describe Projects::LfsPointers::LfsDownloadService do let(:project) { create(:project) } - let(:oid) { '9e548e25631dd9ce6b43afd6359ab76da2819d6a5b474e66118c7819e1d8b3e8' } - let(:download_link) { "http://gitlab.com/#{oid}" } let(:lfs_content) { SecureRandom.random_bytes(10) } + let(:oid) { Digest::SHA256.hexdigest(lfs_content) } + let(:download_link) { "http://gitlab.com/#{oid}" } + let(:size) { lfs_content.size } + let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link) } + let(:local_request_setting) { false } - subject { described_class.new(project) } + subject { described_class.new(project, lfs_object) } before do + ApplicationSetting.create_from_defaults + + stub_application_setting(allow_local_requests_from_hooks_and_services: local_request_setting) allow(project).to receive(:lfs_enabled?).and_return(true) - WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + end + + shared_examples 'lfs temporal file is removed' do + it do + subject.execute - allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false) + expect(File.exist?(subject.send(:tmp_filename))).to be false + end + end + + shared_examples 'no lfs object is created' do + it do + expect { subject.execute }.not_to change { LfsObject.count } + end + + it 'returns error result' do + expect(subject.execute[:status]).to eq :error + end + + it 'an error is logged' do + expect(subject).to receive(:log_error) + + subject.execute + end + + it_behaves_like 'lfs temporal file is removed' + end + + shared_examples 'lfs object is created' do + it do + expect(subject).to receive(:download_and_save_file!).and_call_original + + expect { subject.execute }.to change { LfsObject.count }.by(1) + end + + it 'returns success result' do + expect(subject.execute[:status]).to eq :success + end + + it_behaves_like 'lfs temporal file is removed' end describe '#execute' do context 'when file download succeeds' do - it 'a new lfs object is created' do - expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1) + before do + WebMock.stub_request(:get, download_link).to_return(body: lfs_content) end + it_behaves_like 'lfs object is created' + it 'has the same oid' do - subject.execute(oid, download_link) + subject.execute expect(LfsObject.first.oid).to eq oid end + it 'has the same size' do + subject.execute + + expect(LfsObject.first.size).to eq size + end + it 'stores the content' do - subject.execute(oid, download_link) + subject.execute expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content end end context 'when file download fails' do - it 'no lfs object is created' do - expect { subject.execute(oid, download_link) }.to change { LfsObject.count } + before do + allow(Gitlab::HTTP).to receive(:get).and_return(code: 500, 'success?' => false) + end + + it_behaves_like 'no lfs object is created' + + it 'raise StandardError exception' do + expect(subject).to receive(:download_and_save_file!).and_raise(StandardError) + + subject.execute + end + end + + context 'when downloaded lfs file has a different size' do + let(:size) { 1 } + + before do + WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + end + + it_behaves_like 'no lfs object is created' + + it 'raise SizeError exception' do + expect(subject).to receive(:download_and_save_file!).and_raise(described_class::SizeError) + + subject.execute + end + end + + context 'when downloaded lfs file has a different oid' do + before do + WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + allow_any_instance_of(Digest::SHA256).to receive(:hexdigest).and_return('foobar') + end + + it_behaves_like 'no lfs object is created' + + it 'raise OidError exception' do + expect(subject).to receive(:download_and_save_file!).and_raise(described_class::OidError) + + subject.execute 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) } before do WebMock.stub_request(:get, download_link).with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNzd29yZA==' }).to_return(body: lfs_content) end it 'the request adds authorization headers' do - subject.execute(oid, download_link_with_credentials) + subject end end context 'when localhost requests are allowed' do let(:download_link) { 'http://192.168.2.120' } + let(:local_request_setting) { true } before do - allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true) + WebMock.stub_request(:get, download_link).to_return(body: lfs_content) end - it 'downloads the file' do - expect(subject).to receive(:download_and_save_file).and_call_original - - expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.by(1) - end + it_behaves_like 'lfs object is created' end context 'when a bad URL is used' do @@ -71,7 +159,9 @@ describe Projects::LfsPointers::LfsDownloadService do with_them do it 'does not download the file' do - expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count } + expect(subject).not_to receive(:download_lfs_file!) + + expect { subject.execute }.not_to change { LfsObject.count } end end end @@ -85,15 +175,11 @@ describe Projects::LfsPointers::LfsDownloadService do WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link }) end - it 'does not follow the redirection' do - expect(Rails.logger).to receive(:error).with(/LFS file with oid #{oid} couldn't be downloaded/) - - expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count } - end + it_behaves_like 'no lfs object is created' end end - context 'that is valid' do + context 'that is not blocked' do let(:redirect_link) { "http://example.com/"} before do @@ -101,21 +187,35 @@ describe Projects::LfsPointers::LfsDownloadService do WebMock.stub_request(:get, redirect_link).to_return(body: lfs_content) end - it 'follows the redirection' do - expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1) - end + it_behaves_like 'lfs object is created' + end + end + + context 'when the lfs object attributes are invalid' do + let(:oid) { 'foobar' } + + before do + expect(lfs_object).to be_invalid + end + + it_behaves_like 'no lfs object is created' + + it 'does not download the file' do + expect(subject).not_to receive(:download_lfs_file!) + + subject.execute end end context 'when an lfs object with the same oid already exists' do before do - create(:lfs_object, oid: 'oid') + create(:lfs_object, oid: oid) end it 'does not download the file' do - expect(subject).not_to receive(:download_and_save_file) + expect(subject).not_to receive(:download_lfs_file!) - subject.execute('oid', download_link) + subject.execute end end end |