Commit 915789d6 authored by Nick Thomas's avatar Nick Thomas

Prefer server-provided authentication for LFS push mirroring

LFS servers can set an `authenticated: true` parameter in their upload
and verify instructions to tell the client not to search for
credentials to add to the request. However, this is optional - they may
simply return instructions containing an `Authorization: ....` header
instead. When this happens, the canonical LFS client doesn't overwrite
it with a locally-sourced Authorization header, so we shouldn't either.

This fixes pushing to GitHub, which seems to send the authenticated
hint intermittently.
parent 365b7a11
---
title: Prefer server-provided authentication for LFS push mirroring
merge_request: 44284
author:
type: fixed
......@@ -60,6 +60,7 @@ module Gitlab
}.merge(upload_action['header'] || {})
}
authenticated = true if params[:headers].key?('Authorization')
params[:basic_auth] = basic_auth unless authenticated
rsp = Gitlab::HTTP.put(upload_action['href'], params)
......@@ -75,6 +76,7 @@ module Gitlab
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)
......
......@@ -32,6 +32,9 @@ RSpec.describe Gitlab::Lfs::Client do
}
end
let(:authorized_upload_action) { upload_action.tap { |action| action['header']['Authorization'] = 'foo' } }
let(:authorized_verify_action) { verify_action.tap { |action| action['header']['Authorization'] = 'foo' } }
subject(:lfs_client) { described_class.new(base_url, credentials: credentials) }
describe '#batch' do
......@@ -124,6 +127,19 @@ RSpec.describe Gitlab::Lfs::Client do
end
end
context 'request is not marked as authenticated but includes an authorization header' do
it 'prefers the provided authorization header' do
stub = stub_upload(
object: object,
headers: authorized_upload_action['header']
).to_return(status: 200)
lfs_client.upload!(object, authorized_upload_action, authenticated: false)
expect(stub).to have_been_requested
end
end
context 'LFS object has no file' do
let(:object) { LfsObject.new }
......@@ -193,6 +209,19 @@ RSpec.describe Gitlab::Lfs::Client do
end
end
context 'request is not marked as authenticated but includes an authorization header' do
it 'prefers the provided authorization header' do
stub = stub_verify(
object: object,
headers: authorized_verify_action['header']
).to_return(status: 200)
lfs_client.verify!(object, authorized_verify_action, authenticated: false)
expect(stub).to have_been_requested
end
end
context 'server returns 400 error' do
it 'raises an error' do
stub_verify(object: object, headers: verify_action['header']).to_return(status: 400)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment