Fix deprecation warning since Faraday 1.7.1

[ybiquitous]

Close #1357

[sambostock]

It looks like set_authorization_header was also removed upstream in lostisland/faraday#1308

Based on the deprecation message and diff in that PR, and adjusting for consistency, I think we want:

-http.basic_auth                       "username", "password"
+http.request :authorization, "Basic", "username", "password"

-http.authorization           "token", "access-token"
+http.request :authorization, "token", "access-token"

-http.authorization           "Bearer", "bearer-token"
+http.request :authorization, "Bearer", "bearer-token"

[sambostock]

Actually, it looks like

http.request :authorization, "Basic", "username", "password"

isn't backwards compatible with Faraday versions prior to that PR. It blows up because of the extra password argument.

One forward and backward compatible way of handling it would be to encode the header ourselves

require "base64"
http.request :authorization, "Basic", Base64.strict_encode64("#{username}:#{password}")

[ybiquitous]

@sambostock Thanks for the feedback. Yes, this PR is not backward compatible with Faraday v2.

The PR aims to suppress the annoying warnings, and I think we may need additional code changes for Faraday v2.

[etiennebarrie]

Faraday::Connection#set_authorization_header has been removed from main, so it's not a long-term fix.

[olleolleolle]

Would it be fruitful to try with Faraday 1.8.0, which came in the meantime?

Perhaps not? lostisland/faraday@v1.7.1...v1.8.0

(Re-reading, I note that, yes http.request :authorization [...] is 2.0 way of expressing it.)

[ybiquitous]

@olleolleolle Thanks for sharing the new version. This PR seems still valid with Faraday 1.8.0.

[olleolleolle]

@ybiquitous Yeah, this PR 👍 excellent stuff. 1.0 + 2.0 compat may take another PR, and some trickery.

[iMacTia]

Hi all, just a quick nudge to let you know that we've added a "how to do this in Faraday 1.x" section in the documentation.
This should help solving the warning messages, which I understand is the objective of this PR.

Regarding cross-supporting both Faraday 1.x and 2.x, I'm afraid at the moment there's no suggested way of doing it.
The solution proposed here should work, or alternatively you could always test the Faraday version and use the correct middleware based on that.

I'm open to feedback/suggestions if you have any, but I agree this should be tackled on a separate PR

[ybiquitous]

@iMacTia Thank you. I've followed your advice and push the change c34a880, but the tests have failed:

Faraday::RackBuilder::StackLocked:
  can't modify middleware stack after making a request

https://github.com/octokit/octokit.rb/pull/1359/checks?check_run_id=3673807356#step:7:3522

Any advice?

---

EDIT: When using with Sawyer::Agent, it does not seem to work... 🤔

octokit.rb/lib/octokit/connection.rb

Lines 105 to 118 in c34a880

	@agent ||= Sawyer::Agent.new(endpoint, sawyer_options) do |http|
	http.headers[:accept] = default_media_type
	http.headers[:content_type] = "application/json"
	http.headers[:user_agent] = user_agent
	if basic_authenticated?
	http.request :basic_auth, @login, @password
	elsif token_authenticated?
	http.request :token_auth, @access_token
	elsif bearer_authenticated?
	http.request :authorization, 'Bearer', @bearer_token
	elsif application_authenticated?
	http.request :basic_auth, @client_id, @client_secret
	end
	end

[iMacTia]

@ybiquitous I was having a look myself and I'm finding it quite hard to understand what's going on.
The error is due to the fact that once the first request is fired on a connection, the underlying builder is locked, so that calling http.request is not allowed anymore.

However, looking at the code pointed by the stacktrace I can see that a new connection is being initialised, with no request being fired until the end of the process. So I don't really understand how that can be possible.

So far, the only thing I can think about is that this line is somehow injecting an already locked builder, but I'm having some really hard time following the code (and in particular usages of @middleware given the magic/hidden way these variables are injected 🤔

[ybiquitous]

@iMacTia Thanks for the investigation. Maybe, the MIDDLEWARE constant seems to be a failure cause.

octokit.rb/lib/octokit/default.rb

Lines 26 to 33 in d729c0d

	# Default Faraday middleware stack
	MIDDLEWARE = RACK_BUILDER_CLASS.new do |builder|
	builder.use Faraday::Request::Retry, exceptions: [Octokit::ServerError]
	builder.use Octokit::Middleware::FollowRedirects
	builder.use Octokit::Response::RaiseError
	builder.use Octokit::Response::FeedParser
	builder.adapter Faraday.default_adapter
	end

The following change can reduce 556 failures to 46 failures on my localhost:

--- a/lib/octokit/default.rb
+++ b/lib/octokit/default.rb
@@ -115,7 +115,7 @@ module Octokit
       # from {MIDDLEWARE}
       # @return [Faraday::RackBuilder or Faraday::Builder]
       def middleware
-        MIDDLEWARE
+        MIDDLEWARE.dup
       end
 
       # Default GitHub password for Basic Auth from ENV

(Note: this change is not an essential solution)

Faraday::RackBuilder::StackLocked still occurs...

[iMacTia]

Nice catch @ybiquitous 👏 ! It looks like a step in the right direction though. Somehow that same connection is being reused, causing new agents to fail after the first request gets processed 👍
Can you push the change that for works locally so we can check the remaining failing tests?

[ybiquitous]

Sure. I've pushed 1680bad.

[iMacTia]

Thanks @ybiquitous, that was indeed useful, I think I found the underlying issue:

octokit.rb/lib/octokit/client.rb

Lines 135 to 138 in d1628eb

	Octokit::Configurable.keys.each do |key|
	value = options.key?(key) ? options[key] : Octokit.instance_variable_get(:"@#{key}")
	instance_variable_set(:"@#{key}", value)
	end

The Client initialiser sets instance variables based on the main Octokit module variables. This is quite convoluted, but middleware is one of those. Your last commit only fixes the case where @middleware is accessed through the getter method, but in this case each instance variables is accessed directly

[ybiquitous]

@iMacTia Thank you so much! I've added @middleware.dup (9fadd8b) also to Octokit::Connection like Octokit::Client:

octokit.rb/lib/octokit/client.rb

Line 236 in d729c0d

conn_opts[:builder] = @middleware.dup if @middleware

---

EDIT: VCR::Errors::UnhandledHTTPRequestError errors now occur, instead of Faraday::RackBuilder::StackLocked. 🤔
https://github.com/octokit/octokit.rb/pull/1359/checks?check_run_id=3676528649#step:7:51

[iMacTia]

Only 4 failures left 🎉!
We need to get more information from VCR, you should be able to do so using the debug-logger

[ybiquitous]

@iMacTia Thank you. When I turned on the Faraday's logging as below,

--- a/lib/octokit/connection.rb
+++ b/lib/octokit/connection.rb
@@ -106,6 +106,7 @@ module Octokit
         http.headers[:accept] = default_media_type
         http.headers[:content_type] = "application/json"
         http.headers[:user_agent] = user_agent
+        http.response :logger
         if basic_authenticated?
           http.request :basic_auth, @login, @password
         elsif token_authenticated?

I got the following result. When using http.request :token_auth, the Authorization header is not set.
Is there something wrong with the usage of http.request?

---

Using http.authorization 'token', @access_token (before):

I, [2021-09-23T00:25:08.854295 #4998]  INFO -- request: GET https://api.github.com/
I, [2021-09-23T00:25:08.854330 #4998]  INFO -- request: Accept: "application/vnd.github.v3+json"
User-Agent: "Octokit Ruby Gem 4.21.0"
Content-Type: "application/json"
Authorization: "token xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
I, [2021-09-23T00:25:08.859670 #4998]  INFO -- response: Status 200

Using http.request :token_auth, @access_token (after):

I, [2021-09-23T00:25:27.288127 #5129]  INFO -- request: GET https://api.github.com/
I, [2021-09-23T00:25:27.288161 #5129]  INFO -- request: Accept: "application/vnd.github.v3+json"
User-Agent: "Octokit Ruby Gem 4.21.0"
Content-Type: "application/json"
I, [2021-09-23T00:25:27.295578 #5129]  INFO -- response: Status 401

EDIT: The result above was produced by the bundle exec rspec ./spec/octokit/client_spec.rb:266 command.

[iMacTia]

@ybiquitous it's an issue with middleware order. When you use the deprecated http.authorization, you're effectively setting a default header on the connection, so when the request hits the :logger middleware, the header is already there.
OTOH, when you use the recommended http.request :token_auth, you're injecting a middleware that will add the header on each request.
In order to see that header in your logs though, the :logger middleware needs to be inserted AFTER the :token_auth middleware.

I hope this makes sense, we're definitely getting close!

[ybiquitous]

@iMacTia Thanks! I understand it. 👍🏼

When I moved http.response :logger as below,

--- a/lib/octokit/connection.rb
+++ b/lib/octokit/connection.rb
@@ -115,6 +115,7 @@ module Octokit
         elsif application_authenticated?
           http.request :basic_auth, @client_id, @client_secret
         end
+        http.response :logger
       end
     end
 

I got the following output.

I, [2021-09-23T01:08:18.198989 #10907]  INFO -- request: GET https://api.github.com/
I, [2021-09-23T01:08:18.199023 #10907]  INFO -- request: Accept: "application/vnd.github.v3+json"
User-Agent: "Octokit Ruby Gem 4.21.0"
Content-Type: "application/json"
Authorization: "Token token=\"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\""
I, [2021-09-23T01:08:18.206699 #10907]  INFO -- response: Status 401

The reason of the test failures seems to be a diff in the value of Authorization:

-Authorization: "token xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+Authorization: "Token token=\"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\""

[iMacTia]

Aha! Yes that's exactly how the token_auth works. I was surprised as well while refactoring that part the other day and I suspect this is coming from some super-old pre-Oauth implementation of auth tokens 😂
If Authorization: "token xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" is what you want, then you can use the plain authorisation middleware instead, like you do for Bearer tokens:

http.request :authorization, 'token', @access_token

I know, this is super-confusing. And exactly the reason why I decided to remove it in Faraday 2.x 🤦‍♂️

[ybiquitous]

Thanks! I've pushed c7b5fa2 to fix the token failure. 😄

---

EDIT: Finally, we have only 1 failure:
https://github.com/octokit/octokit.rb/pull/1359/checks?check_run_id=3677676381#step:7:144

[ybiquitous]

#1359 (comment)

When following redirects, the Authorization header is not deleted... 🤔

octokit.rb/lib/octokit/middleware/follow_redirects.rb

Line 89 in d729c0d

env[:request_headers].delete("Authorization")

[iMacTia]

@ybiquitous so close! Checking the error it seems like this might also be a middleware order issue.

Octokit::Client redirect handling drops authentication info when redirecting to a different host

The expectation is that following a redirect, the Authorization header should not be present, but that's not the case and the test fails.
Now, I believe this might be due to the fact that the follow_redirect middleware is inserted before the authorisation one(s).
More precisely, it's added here.

So the stack looks like this:

...
follow_redirects
authorisation (or basic_auth)
...

When a 3xx response is received, the follow_redirects middleware strips the Authorisation header before following, but the authorisation middleware after that will put it back!

If the two middleware were swapped, then this wouldn't happen anymore.

The reason this worked with the old implementation was that the header was previously added during the connection initialisation, rather than by a middleware during the request lifecycle

[ybiquitous]

@iMacTia Thanks. As you thought, the authorization middleware inserts the Authorization header after the follow_redirects middleware deletes the header.

Is there a way to exclude the authorization middleware in the follow_redirects middleware?

[iMacTia]

@ybiquitous the recommended way would be for the middleware to be in the right order, BUT there's another workaround.
The authentication middleware(s) will not touch the request if the Authorisation header is already set (see this line).

That means that instead of removing the authorisation header completely, you could set it to some placeholder value.
Appreciate this is far from ideal, but there's no other way to leave the middleware where it is and just skip through it right now

[ybiquitous]

@iMacTia Thank you very much for your kind advice! 😄
I've added such a workaround via 23b4a5e.

[iMacTia]

My pleasure! I'm glad I could help ❤️
Feel free to mention me in future if you have any issue 🙌 !

[indigok]

Thanks to everyone for your work and patience on this 💖