Fix Faraday deprecation warning

[sambostock]

Goal

This addresses the following deprecation warning:

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.

Design

We more-or-less use the approach suggested by the deprecation message and documentation.

However, Faraday 2.x will support passing a Basic auth username and password to the middleware, and encoding it automatically, but versions prior to that do not. Therefore, we encode it ourselves for backwards compatibility.

This is the same approach I suggested in octokit/octokit.rb#1359 (comment).

Changeset

The method for specifying the Authorization header has been updated.

Testing

Presumably the existing tests should cover it?

[sambostock]

As far as I can tell, this is backwards compatible at least as far back as Faraday 0.9.0, and forward compatible with Faraday 2.0.0, where as

http.request :authorization, "Basic", configuration.email, configuration.password

would be compatible with Faraday 2, but not backwards compatible with Faraday < 2.

[luke-belton]

hey @sambostock - thanks for this, I can see that this silences the deprecation warning. Were you able to test this successfully though? I've tried running it locally with an auth token and get HTTP 401s back from the Bugsnag API unless I make the following change:

http.request :authorization, "Token", configuration.auth_token

to

http.request :authorization, "token", configuration.auth_token

Thanks!

[sambostock]

Ahh, interesting. I think I was going by Faraday::Request::TokenAuthentication (removed in lostisland/faraday#1308), which used :Token. Thanks for checking that; I've updated it.

[johnkiely1]

Thanks for the PR @sambostock. We will review this as soon as priorities allow.

[imjoehaines]

Hey @sambostock, thanks for the PR!

Is there a downside to setting the Authorization header ourselves? i.e.:

if basic_authenticated?
  credentials = Base64.strict_encode64("#{configuration.email}:#{configuration.password}")

  http.headers[:Authorization] = "Basic #{credentials}"
elsif token_authenticated?
  http.headers[:Authorization] = "token #{configuration.auth_token}"
end

I've tested that this sends the same header as our tests (see also #37) and it's fully backwards compatible, so it seems like the best option to me

[sambostock]

@imjoehaines I can't think of any, other than it meaning we're responsible for more of the header implementation. But at this point, we're half way there already given we have to encode the credentials ourselves.

The code above is also using a similar approach to set headers, so seems fine to me 👍

[imjoehaines]

@imjoehaines I can't think of any, other than it meaning we're responsible for more of the header implementation. But at this point, we're half way there already given we have to encode the credentials ourselves.

The code above is also using a similar approach to set headers, so seems fine to me 👍

Cool, I think if we're encoding the header ourselves anyway then it makes sense to manually set it to avoid the potential BC breaks

I'll look at making that change & pushing a new release next week

[sambostock]

@imjoehaines I've updated this branch to reflect the discussion above 👍

[imjoehaines]

Thanks @sambostock!