Skip to content
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

treat basic auth and bearer auth the same #533

Merged
merged 1 commit into from Jan 30, 2022

Conversation

grosser
Copy link
Contributor

@grosser grosser commented Jan 21, 2022

... and avoid sending in the headers twice

found during #532 so preferable merge here first and then rebase for a smaller diff

@cben

Comment on lines 357 to +359
connection.basic_auth(@auth_options[:username], @auth_options[:password])
elsif @auth_options[:bearer_token]
connection.authorization(:Bearer, @auth_options[:bearer_token])
Copy link
Collaborator

@cben cben Jan 29, 2022

Choose a reason for hiding this comment

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

Nice! But this is printing warnings. Actually basic_auth also does now.

WARNING: Faraday::Connection#authorization is deprecated; it will be removed in version 2.0.
While initializing your connection, use #request(:authorization, ...) instead.
See https://lostisland.github.io/faraday/middleware/authentication for more usage info.
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.

(deprecation was here: lostisland/faraday#1306)

I think it's supposed to be (untested):

Suggested change
connection.basic_auth(@auth_options[:username], @auth_options[:password])
elsif @auth_options[:bearer_token]
connection.authorization(:Bearer, @auth_options[:bearer_token])
connection.request(:basic_auth, @auth_options[:username], @auth_options[:password])
elsif @auth_options[:bearer_token]
connection.request(:authorization, 'Bearer', @auth_options[:bearer_token])

@cben
Copy link
Collaborator

cben commented Jan 29, 2022

I like the overall simplification, but doesn't converting to file name -> file content go contrary to goals of #530?
Well, no @auth_options[:bearer_token_file] will still exist.

LGTM. close-cycling to re-run failed ruby 2.5 (looks like unrelated flake)

The faraday warnings thing is actually orthogonal, I could merge this as-is then send an MR.

@cben cben closed this Jan 29, 2022
@cben cben reopened this Jan 29, 2022
@grosser
Copy link
Contributor Author

grosser commented Jan 29, 2022

plz merge and let's fix warnings in it's own PR :)

@grosser
Copy link
Contributor Author

grosser commented Jan 29, 2022

I can also tack on the warning change if you want

@cben cben merged commit 7db0d48 into ManageIQ:master Jan 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants