Closed sambostock closed 2 years ago
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!
Ahh, interesting. I think I was going by Faraday::Request::TokenAuthentication
(removed in https://github.com/lostisland/faraday/pull/1308), which used :Token
. Thanks for checking that; I've updated it.
Thanks for the PR @sambostock. We will review this as soon as priorities allow.
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
@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 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
@imjoehaines I've updated this branch to reflect the discussion above 👍
Goal
This addresses the following deprecation warning:
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 https://github.com/octokit/octokit.rb/pull/1359#issuecomment-916389545.
Changeset
The method for specifying the
Authorization
header has been updated.Testing
Presumably the existing tests should cover it?