Merge branch 'sh-fix-lfs-download-bitbucket' into 'master'

Fix LFS objects not downloading with Bitbucket

See merge request gitlab-org/gitlab!65380

app/models/lfs_download_object.rb

@@ -3,20 +3,32 @@
 class LfsDownloadObject
   include ActiveModel::Validations
 
-  attr_accessor :oid, :size, :link
+  attr_accessor :oid, :size, :link, :headers
   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) }
+  validate :headers_must_be_hash
 
-  def initialize(oid:, size:, link:)
+  def initialize(oid:, size:, link:, headers: {})
     @oid = oid
     @size = size
     @link = link
+    @headers = headers || {}
   end
 
   def sanitized_uri
     @sanitized_uri ||= Gitlab::UrlSanitizer.new(link)
   end
+
+  def has_authorization_header?
+    headers.keys.map(&:downcase).include?('authorization')
+  end
+
+  private
+
+  def headers_must_be_hash
+    errors.add(:base, "headers must be a Hash") unless headers.is_a?(Hash)
+  end
 end

app/services/projects/lfs_pointers/lfs_download_link_list_service.rb

@@ -81,11 +81,13 @@ module Projects
       def parse_response_links(objects_response)
         objects_response.each_with_object([]) do |entry, link_list|
           link = entry.dig('actions', DOWNLOAD_ACTION, 'href')
+          headers = entry.dig('actions', DOWNLOAD_ACTION, 'header')
 
           raise DownloadLinkNotFound unless link
 
           link_list << LfsDownloadObject.new(oid: entry['oid'],
                                              size: entry['size'],
+                                             headers: headers,
                                              link: add_credentials(link))
         rescue DownloadLinkNotFound, Addressable::URI::InvalidURIError
           log_error("Link for Lfs Object with oid #{entry['oid']} not found or invalid.")

app/services/projects/lfs_pointers/lfs_download_service.rb

@@ -11,7 +11,7 @@ module Projects
       LARGE_FILE_SIZE = 1.megabytes
 
       attr_reader :lfs_download_object
-      delegate :oid, :size, :credentials, :sanitized_url, to: :lfs_download_object, prefix: :lfs
+      delegate :oid, :size, :credentials, :sanitized_url, :headers, to: :lfs_download_object, prefix: :lfs
 
       def initialize(project, lfs_download_object)
         super(project)
@@ -71,17 +71,21 @@ module Projects
         raise_oid_error! if digester.hexdigest != lfs_oid
       end
 
-      def download_headers
-        { stream_body: true }.tap do |headers|
+      def download_options
+        http_options = { headers: lfs_headers, stream_body: true }
+
+        return http_options if lfs_download_object.has_authorization_header?
+
+        http_options.tap do |options|
           if lfs_credentials[:user].present? || lfs_credentials[:password].present?
             # Using authentication headers in the request
-            headers[:basic_auth] = { username: lfs_credentials[:user], password: lfs_credentials[:password] }
+            options[:basic_auth] = { username: lfs_credentials[:user], password: lfs_credentials[:password] }
           end
         end
       end
 
       def fetch_file(&block)
-        response = Gitlab::HTTP.get(lfs_sanitized_url, download_headers, &block)
+        response = Gitlab::HTTP.get(lfs_sanitized_url, download_options, &block)
 
         raise ResponseError, "Received error code #{response.code}" unless response.success?
       end

spec/models/lfs_download_object_spec.rb

@@ -6,8 +6,45 @@ RSpec.describe LfsDownloadObject do
   let(:oid) { 'cd293be6cea034bd45a0352775a219ef5dc7825ce55d1f7dae9762d80ce64411' }
   let(:link) { 'http://www.example.com' }
   let(:size) { 1 }
+  let(:headers) { { test: "asdf" } }
 
-  subject { described_class.new(oid: oid, size: size, link: link) }
+  subject { described_class.new(oid: oid, size: size, link: link, headers: headers) }
+
+  describe '#headers' do
+    it 'returns specified Hash' do
+      expect(subject.headers).to eq(headers)
+    end
+
+    context 'with nil headers' do
+      let(:headers) { nil }
+
+      it 'returns a Hash' do
+        expect(subject.headers).to eq({})
+      end
+    end
+  end
+
+  describe '#has_authorization_header?' do
+    it 'returns false' do
+      expect(subject.has_authorization_header?).to be false
+    end
+
+    context 'with uppercase form' do
+      let(:headers) { { 'Authorization' => 'Basic 12345' } }
+
+      it 'returns true' do
+        expect(subject.has_authorization_header?).to be true
+      end
+    end
+
+    context 'with lowercase form' do
+      let(:headers) { { 'authorization' => 'Basic 12345' } }
+
+      it 'returns true' do
+        expect(subject.has_authorization_header?).to be true
+      end
+    end
+  end
 
   describe 'validations' do
     it { is_expected.to validate_numericality_of(:size).is_greater_than_or_equal_to(0) }
@@ -66,5 +103,16 @@ RSpec.describe LfsDownloadObject do
         end
       end
     end
+
+    context 'headers attribute' do
+      it 'only nil and Hash values are valid' do
+        aggregate_failures do
+          expect(described_class.new(oid: oid, size: size, link: 'http://www.example.com', headers: nil)).to be_valid
+          expect(described_class.new(oid: oid, size: size, link: 'http://www.example.com', headers: {})).to be_valid
+          expect(described_class.new(oid: oid, size: size, link: 'http://www.example.com', headers: { 'test' => 123 })).to be_valid
+          expect(described_class.new(oid: oid, size: size, link: 'http://www.example.com', headers: 'test')).to be_invalid
+        end
+      end
+    end
   end
 end

spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe Projects::LfsPointers::LfsDownloadLinkListService do
   let(:lfs_endpoint) { "#{import_url}/info/lfs/objects/batch" }
   let!(:project) { create(:project, import_url: import_url) }
   let(:new_oids) { { 'oid1' => 123, 'oid2' => 125 } }
+  let(:headers) { { 'X-Some-Header' => '456' }}
   let(:remote_uri) { URI.parse(lfs_endpoint) }
 
   let(:request_object) { HTTParty::Request.new(Net::HTTP::Post, '/') }
@@ -18,7 +19,7 @@ RSpec.describe Projects::LfsPointers::LfsDownloadLinkListService do
       {
         'oid' => oid, 'size' => size,
         'actions' => {
-          'download' => { 'href' => "#{import_url}/gitlab-lfs/objects/#{oid}" }
+          'download' => { 'href' => "#{import_url}/gitlab-lfs/objects/#{oid}", header: headers }
         }
       }
     end
@@ -48,12 +49,20 @@ RSpec.describe Projects::LfsPointers::LfsDownloadLinkListService do
   end
 
   describe '#execute' do
+    let(:download_objects) { subject.execute(new_oids) }
+
     it 'retrieves each download link of every non existent lfs object' do
-      subject.execute(new_oids).each do |lfs_download_object|
+      download_objects.each do |lfs_download_object|
         expect(lfs_download_object.link).to eq "#{import_url}/gitlab-lfs/objects/#{lfs_download_object.oid}"
       end
     end
 
+    it 'stores headers' do
+      download_objects.each do |lfs_download_object|
+        expect(lfs_download_object.headers).to eq(headers)
+      end
+    end
+
     context 'when lfs objects size is larger than the batch size' do
       def stub_successful_request(batch)
         response = custom_response(success_net_response, objects_response(batch))

spec/services/projects/lfs_pointers/lfs_download_service_spec.rb

@@ -155,13 +155,24 @@ RSpec.describe Projects::LfsPointers::LfsDownloadService do
     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) }
+      let!(:request_stub) { stub_full_request(download_link).with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNzd29yZA==' }).to_return(body: lfs_content) }
 
-      before do
-        stub_full_request(download_link).with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNzd29yZA==' }).to_return(body: lfs_content)
+      it 'the request adds authorization headers' do
+        subject.execute
+
+        expect(request_stub).to have_been_requested
       end
 
-      it 'the request adds authorization headers' do
-        subject
+      context 'when Authorization header is present' do
+        let(:auth_header) { { 'Authorization' => 'Basic 12345' } }
+        let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link_with_credentials, headers: auth_header) }
+        let!(:request_stub) { stub_full_request(download_link).with(headers: auth_header).to_return(body: lfs_content) }
+
+        it 'request uses the header auth' do
+          subject.execute
+
+          expect(request_stub).to have_been_requested
+        end
       end
     end