Improve LFS client performance and fix compatibility with Azure DevOps

The LFS client that is used in repository mirroring had a few
shortcomings:

1. Chunked transfers were never used because the Content-Length was
always specified. We now only including Content-Length if chunked
encodings are not requested.

2. Azure DevOps passes in the username in the upload URL, but this cause
HTTParty to serialize this in the Authorization header instead of using
the mirror credentials. We now strip the URL of username/passwords if
HTTP Basic Auth is used.

Relates to #340482

Changelog: fixed

lib/gitlab/lfs/client.rb

@@ -53,17 +53,11 @@ def upload!(object, upload_action, authenticated:)
 
         params = {
           body_stream: file,
-          headers: {
-            'Content-Length' => object.size.to_s,
-            'Content-Type' => 'application/octet-stream',
-            'User-Agent' => GIT_LFS_USER_AGENT
-          }.merge(upload_action['header'] || {})
+          headers: upload_headers(object, upload_action)
         }
 
-        authenticated = true if params[:headers].key?('Authorization')
-        params[:basic_auth] = basic_auth unless authenticated
-
-        rsp = Gitlab::HTTP.put(upload_action['href'], params)
+        url = set_basic_auth_and_extract_lfs_url!(params, upload_action['href'])
+        rsp = Gitlab::HTTP.put(url, params)
 
         raise ObjectUploadError.new(http_response: rsp) unless rsp.success?
       ensure
@@ -76,20 +70,51 @@ def verify!(object, verify_action, authenticated:)
           headers: build_request_headers(verify_action['header'])
         }
 
-        authenticated = true if params[:headers].key?('Authorization')
-        params[:basic_auth] = basic_auth unless authenticated
-
-        rsp = Gitlab::HTTP.post(verify_action['href'], params)
+        url = set_basic_auth_and_extract_lfs_url!(params, verify_action['href'])
+        rsp = Gitlab::HTTP.post(url, params)
 
         raise ObjectVerifyError.new(http_response: rsp) unless rsp.success?
       end
 
       private
 
+      def set_basic_auth_and_extract_lfs_url!(params, raw_url)
+        authenticated = true if params[:headers].key?('Authorization')
+        params[:basic_auth] = basic_auth unless authenticated
+        strip_userinfo = authenticated || params[:basic_auth].present?
+        lfs_url(raw_url, strip_userinfo)
+      end
+
       def build_request_headers(extra_headers = nil)
         DEFAULT_HEADERS.merge(extra_headers || {})
       end
 
+      def upload_headers(object, upload_action)
+        # This uses the httprb library to handle case-insensitive HTTP headers
+        headers = ::HTTP::Headers.new
+        headers.merge!(upload_action['header'])
+        transfer_encodings = Array(headers['Transfer-Encoding']&.split(',')).map(&:strip)
+
+        headers['Content-Length'] = object.size.to_s unless transfer_encodings.include?('chunked')
+        headers['Content-Type'] = 'application/octet-stream'
+        headers['User-Agent'] = GIT_LFS_USER_AGENT
+
+        headers.to_h
+      end
+
+      def lfs_url(raw_url, strip_userinfo)
+        # HTTParty will give precedence to the username/password
+        # specified in the URL. This causes problems with Azure DevOps,
+        # which includes a username in the URL. Stripping the userinfo
+        # from the URL allows the provided HTTP Basic Authentication
+        # credentials to be used.
+        if strip_userinfo
+          Gitlab::UrlSanitizer.new(raw_url).sanitized_url
+        else
+          raw_url
+        end
+      end
+
       attr_reader :credentials
 
       def batch_url

spec/lib/gitlab/lfs/client_spec.rb

@@ -114,6 +114,52 @@ def stub_batch(objects:, headers:, operation: 'upload', transfer: 'basic')
       end
     end
 
+    context 'server returns 200 OK with a chunked transfer request' do
+      before do
+        upload_action['header']['Transfer-Encoding'] = 'gzip, chunked'
+      end
+
+      it "makes an HTTP PUT with expected parameters" do
+        stub_upload(object: object, headers: upload_action['header'], chunked_transfer: true).to_return(status: 200)
+
+        lfs_client.upload!(object, upload_action, authenticated: true)
+      end
+    end
+
+    context 'server returns 200 OK with a username and password in the URL' do
+      let(:base_url) { "https://someuser:testpass@example.com" }
+
+      it "makes an HTTP PUT with expected parameters" do
+        stub_upload(
+          object: object,
+          headers: basic_auth_headers.merge(upload_action['header']),
+          url: "https://example.com/some/file"
+        ).to_return(status: 200)
+
+        lfs_client.upload!(object, upload_action, authenticated: true)
+      end
+    end
+
+    context 'no credentials in client' do
+      subject(:lfs_client) { described_class.new(base_url, credentials: {}) }
+
+      context 'server returns 200 OK with credentials in URL' do
+        let(:creds) { 'someuser:testpass' }
+        let(:base_url) { "https://#{creds}@example.com" }
+        let(:auth_headers) { { 'Authorization' => "Basic #{Base64.strict_encode64(creds)}" } }
+
+        it "makes an HTTP PUT with expected parameters" do
+          stub_upload(
+            object: object,
+            headers: auth_headers.merge(upload_action['header']),
+            url: "https://example.com/some/file"
+          ).to_return(status: 200)
+
+          lfs_client.upload!(object, upload_action, authenticated: true)
+        end
+      end
+    end
+
     context 'server returns 200 OK to an unauthenticated request' do
       it "makes an HTTP PUT with expected parameters" do
         stub = stub_upload(
@@ -171,16 +217,21 @@ def stub_batch(objects:, headers:, operation: 'upload', transfer: 'basic')
       end
     end
 
-    def stub_upload(object:, headers:)
+    def stub_upload(object:, headers:, url: upload_action['href'], chunked_transfer: false)
       headers = {
         'Content-Type' => 'application/octet-stream',
-        'Content-Length' => object.size.to_s,
         'User-Agent' => git_lfs_user_agent
       }.merge(headers)
 
-      stub_request(:put, upload_action['href']).with(
+      if chunked_transfer
+        headers['Transfer-Encoding'] = 'gzip, chunked'
+      else
+        headers['Content-Length'] = object.size.to_s
+      end
+
+      stub_request(:put, url).with(
         body: object.file.read,
-        headers: headers.merge('Content-Length' => object.size.to_s)
+        headers: headers
       )
     end
   end
@@ -196,11 +247,25 @@ def stub_upload(object:, headers:)
       end
     end
 
+    context 'server returns 200 OK with a username and password in the URL' do
+      let(:base_url) { "https://someuser:testpass@example.com" }
+
+      it "makes an HTTP PUT with expected parameters" do
+        stub_verify(
+          object: object,
+          headers: basic_auth_headers.merge(verify_action['header']),
+          url: "https://example.com/some/file/verify"
+        ).to_return(status: 200)
+
+        lfs_client.verify!(object, verify_action, authenticated: true)
+      end
+    end
+
     context 'server returns 200 OK to an unauthenticated request' do
       it "makes an HTTP POST with expected parameters" do
         stub = stub_verify(
           object: object,
-          headers: basic_auth_headers.merge(upload_action['header'])
+          headers: basic_auth_headers.merge(verify_action['header'])
         ).to_return(status: 200)
 
         lfs_client.verify!(object, verify_action, authenticated: false)
@@ -238,14 +303,14 @@ def stub_upload(object:, headers:)
       end
     end
 
-    def stub_verify(object:, headers:)
+    def stub_verify(object:, headers:, url: verify_action['href'])
       headers = {
         'Accept' => git_lfs_content_type,
         'Content-Type' => git_lfs_content_type,
         'User-Agent' => git_lfs_user_agent
       }.merge(headers)
 
-      stub_request(:post, verify_action['href']).with(
+      stub_request(:post, url).with(
         body: object.to_json(only: [:oid, :size]),
         headers: headers
       )