Skip to content

Drop deprecated auth helpers from Connection and refactor auth middleware#1308

Merged
olleolleolle merged 1 commit intomainfrom
drop-deprecated-auth-helpers
Aug 15, 2021
Merged

Drop deprecated auth helpers from Connection and refactor auth middleware#1308
olleolleolle merged 1 commit intomainfrom
drop-deprecated-auth-helpers

Conversation

@iMacTia
Copy link
Copy Markdown
Member

@iMacTia iMacTia commented Aug 14, 2021 •
edited by olleolleolle
Loading

Description

  • Drop deprecated auth helpers from Connection
  • Merge Authentication middleware.
  • Add possibility of passing a proc.
  • Update documentation.

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:

  • Tests
  • Documentation

@iMacTia
Drop deprecated ath helpers from Connection.
9439fb8 
Merge Authentication middleware.
Add possibility of passing a proc.
Update documentation.
@iMacTia iMacTia requested a review from olleolleolle August 14, 2021 14:08
@iMacTia iMacTia self-assigned this Aug 14, 2021
olleolleolle
olleolleolle approved these changes Aug 15, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@olleolleolle olleolleolle left a 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!

spec/faraday/request/authorization_spec.rb
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') }
Copy link
Copy Markdown
Member

@olleolleolle olleolleolle Aug 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement!

@olleolleolle olleolleolle mentioned this pull request Aug 15, 2021
Closed
3 tasks
@olleolleolle olleolleolle merged commit fe600c7 into main Aug 15, 2021
@iMacTia iMacTia deleted the drop-deprecated-auth-helpers branch August 15, 2021 08:39
@olleolleolle olleolleolle mentioned this pull request Aug 20, 2021
Closed
This was referenced Aug 31, 2021
Merged
Closed
@etiennebarrie
Copy link
Copy Markdown

etiennebarrie commented Aug 31, 2021 •
edited
Loading

This broke passing a URL with embedded basic auth in the user info part of the URI, because basic_auth is still used in url_prefix=

faraday/lib/faraday/connection.rb

Lines 364 to 367 in 0f9626c

with_uri_credentials(uri) do |user, password|
basic_auth user, password
uri.user = uri.password = nil
end 

~/src/github.com/lostisland/faraday (1.x) $ ruby -Ilib -rfaraday -rfaraday/net_http -rjson -e "puts JSON.load(Faraday.new(url: 'https://user:password@httpbin.org/headers').get.body)['headers']['Authorization']"
WARNING: `Faraday::Connection#basic_auth` is deprecated; it will be removed in version 2.0.
While initializing your connection, use `#request(:basic_auth, ...)` instead.
See https://lostisland.github.io/faraday/middleware/authentication for more usage info.
Basic dXNlcjpwYXNzd29yZA==

~/src/github.com/lostisland/faraday (main) $ ruby -Ilib -rfaraday -rfaraday/net_http -rjson -e "puts JSON.load(Faraday.new(url: 'https://user:password@httpbin.org/headers').get.body)['headers']['Authorization']"
lib/faraday/connection.rb:365:in `block in url_prefix=': undefined method `basic_auth' for #<Faraday::Connection:0x00007fed250abaa8 @parallel_manager=nil, @headers={}, @params={}, @options=#<Faraday::RequestOptions (empty)>, @ssl=#<Faraday::SSLOptions (empty)>, @default_parallel_manager=nil, @manual_proxy=nil, @builder=#<Faraday::RackBuilder:0x00007fed25092210 @adapter=Faraday::Adapter::NetHttp, @handlers=[Faraday::Request::UrlEncoded]>, @url_prefix=#<URI::HTTPS https://user:password@httpbin.org/headers>> (NoMethodError)
	from lib/faraday/connection.rb:506:in `with_uri_credentials'
	from lib/faraday/connection.rb:364:in `url_prefix='
	from lib/faraday/connection.rb:84:in `initialize'
	from lib/faraday.rb:96:in `new'
	from lib/faraday.rb:96:in `new'
	from -e:1:in `<main>'

While I'm not against deprecating basic_auth and authorization on Connection, I think being able to have basic auth embedded in the URL is really useful (it can be configured in secrets for example).

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

@iMacTia iMacTia mentioned this pull request Sep 1, 2021
Closed
@iMacTia
Copy link
Copy Markdown
Member Author

iMacTia commented Sep 1, 2021

Thanks for reporting this issue @etiennebarrie, I'm surprised we didn't have tests supporting this use-case.
I've opened an issue (#1319) from your comment and will discuss with the team on the course of action, please feel free to join the discussion there

jarl-dk pushed a commit to softace/faraday that referenced this pull request Sep 9, 2021
@iMacTia @jarl-dk
Drop deprecated auth helpers from Connection. (lostisland#1308)
3fee015 
Merge Authentication middleware.
Add possibility of passing a proc.
Update documentation.

(cherry picked from commit fe600c7)
@sambostock sambostock mentioned this pull request Sep 15, 2021
Merged
@coderabbitai coderabbitai bot mentioned this pull request Feb 16, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@olleolleolle olleolleolle olleolleolle approved these changes

Assignees

@iMacTia iMacTia

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[2.0] Deprecations to drop

3 participants

@iMacTia @etiennebarrie @olleolleolle