-
Notifications
You must be signed in to change notification settings - Fork 973
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop deprecated auth helpers from Connection and refactor auth middleware #1308
Conversation
Merge Authentication middleware. Add possibility of passing a proc. Update documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good removal! 🥳
This brings clarity to a place where the story was not so straight-forward to tell before!
@@ -14,18 +14,18 @@ | |||
|
|||
shared_examples 'does not interfere with existing authentication' do | |||
context 'and request already has an authentication header' do | |||
let(:response) { conn.get('/auth-echo', nil, authorization: 'Token token="bar"') } | |||
let(:response) { conn.get('/auth-echo', nil, authorization: 'OAuth oauth_token') } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement!
This broke passing a URL with embedded basic auth in the user info part of the URI, because basic_auth is still used in faraday/lib/faraday/connection.rb Lines 364 to 367 in 0f9626c
While I'm not against deprecating I added a test here: 1.x...etiennebarrie:test-basic-auth-in-url: it 'uses User Information from the URI for Basic authentication' do
conn.url_prefix = 'http://user:password@sushi.com'
expect(conn.url_prefix.to_s).to eq('http://sushi.com/')
request = conn.build_request(:get)
expect(request.headers['Authorization']).to eq("Basic #{Base64.strict_encode64('user:password')}")
end |
Thanks for reporting this issue @etiennebarrie, I'm surprised we didn't have tests supporting this use-case. |
Merge Authentication middleware. Add possibility of passing a proc. Update documentation. (cherry picked from commit fe600c7)
Description
Fixes #1302 as this is the last deprecation in the 1.x code 🙌
Todos
List any remaining work that needs to be done, i.e: