diff --git a/app/models/lfs_download_object.rb b/app/models/lfs_download_object.rb new file mode 100644 index 00000000000..6383f95d546 --- /dev/null +++ b/app/models/lfs_download_object.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class LfsDownloadObject + include ActiveModel::Validations + + attr_accessor :oid, :size, :link + delegate :sanitized_url, :credentials, to: :sanitized_uri + + validates :oid, format: { with: /\A\h{64}\z/ } + validates :size, numericality: { greater_than_or_equal_to: 0 } + validates :link, public_url: { protocols: %w(http https) } + + def initialize(oid:, size:, link:) + @oid = oid + @size = size + @link = link + end + + def sanitized_uri + @sanitized_uri ||= Gitlab::UrlSanitizer.new(link) + end +end diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index afd32c0d968..5861b803996 100644 --- a/app/services/projects/import_service.rb +++ b/app/services/projects/import_service.rb @@ -94,11 +94,11 @@ module Projects return unless project.lfs_enabled? - oids_to_download = Projects::LfsPointers::LfsImportService.new(project).execute - download_service = Projects::LfsPointers::LfsDownloadService.new(project) + lfs_objects_to_download = Projects::LfsPointers::LfsImportService.new(project).execute - oids_to_download.each do |oid, link| - download_service.execute(oid, link) + lfs_objects_to_download.each do |lfs_download_object| + Projects::LfsPointers::LfsDownloadService.new(project, lfs_download_object) + .execute end rescue => e # Right now, to avoid aborting the importing process, we silently fail diff --git a/app/services/projects/lfs_pointers/lfs_download_link_list_service.rb b/app/services/projects/lfs_pointers/lfs_download_link_list_service.rb index a837ea82e38..7998976b00a 100644 --- a/app/services/projects/lfs_pointers/lfs_download_link_list_service.rb +++ b/app/services/projects/lfs_pointers/lfs_download_link_list_service.rb @@ -41,16 +41,17 @@ module Projects end def parse_response_links(objects_response) - objects_response.each_with_object({}) do |entry, link_list| + objects_response.each_with_object([]) do |entry, link_list| begin - oid = entry['oid'] link = entry.dig('actions', DOWNLOAD_ACTION, 'href') raise DownloadLinkNotFound unless link - link_list[oid] = add_credentials(link) - rescue DownloadLinkNotFound, URI::InvalidURIError - Rails.logger.error("Link for Lfs Object with oid #{oid} not found or invalid.") + link_list << LfsDownloadObject.new(oid: entry['oid'], + size: entry['size'], + link: add_credentials(link)) + rescue DownloadLinkNotFound, Addressable::URI::InvalidURIError + log_error("Link for Lfs Object with oid #{entry['oid']} not found or invalid.") end end end @@ -70,7 +71,7 @@ module Projects end def add_credentials(link) - uri = URI.parse(link) + uri = Addressable::URI.parse(link) if should_add_credentials?(uri) uri.user = remote_uri.user diff --git a/app/services/projects/lfs_pointers/lfs_download_service.rb b/app/services/projects/lfs_pointers/lfs_download_service.rb index b5128443435..398f00a598d 100644 --- a/app/services/projects/lfs_pointers/lfs_download_service.rb +++ b/app/services/projects/lfs_pointers/lfs_download_service.rb @@ -4,68 +4,93 @@ module Projects module LfsPointers class LfsDownloadService < BaseService - VALID_PROTOCOLS = %w[http https].freeze + SizeError = Class.new(StandardError) + OidError = Class.new(StandardError) + + attr_reader :lfs_download_object + delegate :oid, :size, :credentials, :sanitized_url, to: :lfs_download_object, prefix: :lfs + + def initialize(project, lfs_download_object) + super(project) + + @lfs_download_object = lfs_download_object + end # rubocop: disable CodeReuse/ActiveRecord - def execute(oid, url) - return unless project&.lfs_enabled? && oid.present? && url.present? + def execute + return unless project&.lfs_enabled? && lfs_download_object + return error("LFS file with oid #{lfs_oid} has invalid attributes") unless lfs_download_object.valid? + return if LfsObject.exists?(oid: lfs_oid) - return if LfsObject.exists?(oid: oid) - - sanitized_uri = sanitize_url!(url) - - with_tmp_file(oid) do |file| - download_and_save_file(file, sanitized_uri) - lfs_object = LfsObject.new(oid: oid, size: file.size, file: file) - - project.all_lfs_objects << lfs_object + wrap_download_errors do + download_lfs_file! end - rescue Gitlab::UrlBlocker::BlockedUrlError => e - Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded: #{e.message}") - rescue StandardError => e - Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded from #{sanitized_uri.sanitized_url}: #{e.message}") end # rubocop: enable CodeReuse/ActiveRecord private - def sanitize_url!(url) - Gitlab::UrlSanitizer.new(url).tap do |sanitized_uri| - # Just validate that HTTP/HTTPS protocols are used. The - # subsequent Gitlab::HTTP.get call will do network checks - # based on the settings. - Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url, - protocols: VALID_PROTOCOLS) + def wrap_download_errors(&block) + yield + rescue SizeError, OidError, StandardError => e + error("LFS file with oid #{lfs_oid} could't be downloaded from #{lfs_sanitized_url}: #{e.message}") + end + + def download_lfs_file! + with_tmp_file do |tmp_file| + download_and_save_file!(tmp_file) + project.all_lfs_objects << LfsObject.new(oid: lfs_oid, + size: lfs_size, + file: tmp_file) + + success end end - def download_and_save_file(file, sanitized_uri) - response = Gitlab::HTTP.get(sanitized_uri.sanitized_url, headers(sanitized_uri)) do |fragment| + def download_and_save_file!(file) + digester = Digest::SHA256.new + response = Gitlab::HTTP.get(lfs_sanitized_url, download_headers) do |fragment| + digester << fragment file.write(fragment) + + raise_size_error! if file.size > lfs_size end raise StandardError, "Received error code #{response.code}" unless response.success? + + raise_size_error! if file.size != lfs_size + raise_oid_error! if digester.hexdigest != lfs_oid end - def headers(sanitized_uri) - query_options.tap do |headers| - credentials = sanitized_uri.credentials - - if credentials[:user].present? || credentials[:password].present? + def download_headers + { stream_body: true }.tap do |headers| + if lfs_credentials[:user].present? || lfs_credentials[:password].present? # Using authentication headers in the request - headers[:http_basic_authentication] = [credentials[:user], credentials[:password]] + headers[:basic_auth] = { username: lfs_credentials[:user], password: lfs_credentials[:password] } end end end - def query_options - { stream_body: true } - end - - def with_tmp_file(oid) + def with_tmp_file create_tmp_storage_dir - File.open(File.join(tmp_storage_dir, oid), 'wb') { |file| yield file } + File.open(tmp_filename, 'wb') do |file| + begin + yield file + rescue StandardError => e + # If the lfs file is successfully downloaded it will be removed + # when it is added to the project's lfs files. + # Nevertheless if any excetion raises the file would remain + # in the file system. Here we ensure to remove it + File.unlink(file) if File.exist?(file) + + raise e + end + end + end + + def tmp_filename + File.join(tmp_storage_dir, lfs_oid) end def create_tmp_storage_dir @@ -79,6 +104,20 @@ module Projects def storage_dir @storage_dir ||= Gitlab.config.lfs.storage_path end + + def raise_size_error! + raise SizeError, 'Size mistmatch' + end + + def raise_oid_error! + raise OidError, 'Oid mismatch' + end + + def error(message, http_status = nil) + log_error(message) + + super + end end end end diff --git a/changelogs/unreleased/security-fix-lfs-import-project-ssrf-forgery.yml b/changelogs/unreleased/security-fix-lfs-import-project-ssrf-forgery.yml new file mode 100644 index 00000000000..b6315ec29d8 --- /dev/null +++ b/changelogs/unreleased/security-fix-lfs-import-project-ssrf-forgery.yml @@ -0,0 +1,5 @@ +--- +title: Add more LFS validations to prevent forgery +merge_request: +author: +type: security diff --git a/lib/gitlab/github_import/importer/lfs_object_importer.rb b/lib/gitlab/github_import/importer/lfs_object_importer.rb index a88c17aaf82..195383fd3e9 100644 --- a/lib/gitlab/github_import/importer/lfs_object_importer.rb +++ b/lib/gitlab/github_import/importer/lfs_object_importer.rb @@ -13,10 +13,12 @@ module Gitlab @project = project end + def lfs_download_object + LfsDownloadObject.new(oid: lfs_object.oid, size: lfs_object.size, link: lfs_object.link) + end + def execute - Projects::LfsPointers::LfsDownloadService - .new(project) - .execute(lfs_object.oid, lfs_object.download_link) + Projects::LfsPointers::LfsDownloadService.new(project, lfs_download_object).execute end end end diff --git a/lib/gitlab/github_import/representation/lfs_object.rb b/lib/gitlab/github_import/representation/lfs_object.rb index debe0fa0baf..a4606173f49 100644 --- a/lib/gitlab/github_import/representation/lfs_object.rb +++ b/lib/gitlab/github_import/representation/lfs_object.rb @@ -9,11 +9,11 @@ module Gitlab attr_reader :attributes - expose_attribute :oid, :download_link + expose_attribute :oid, :link, :size # Builds a lfs_object def self.from_api_response(lfs_object) - new({ oid: lfs_object[0], download_link: lfs_object[1] }) + new({ oid: lfs_object.oid, link: lfs_object.link, size: lfs_object.size }) end # Builds a new lfs_object using a Hash that was built from a JSON payload. 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 a005defb3ee..7faf0fc2868 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 fcc87196d5a..876beb39801 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 - allow(project).to receive(:lfs_enabled?).and_return(true) - WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + ApplicationSetting.create_from_defaults - allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false) + stub_application_setting(allow_local_requests_from_hooks_and_services: local_request_setting) + allow(project).to receive(:lfs_enabled?).and_return(true) + end + + shared_examples 'lfs temporal file is removed' do + it do + subject.execute + + 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