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

Add BearerAuth middleware #462

Merged
merged 1 commit into from
Jun 24, 2021

Conversation

mdlkxzmcp
Copy link
Contributor

Adds a Tesla.Middleware.BearerAuth middleware.

Basically, I had to use the bearer auth method, but Tesla doesn't have a middleware for it. So here is my small contribution :)

Wanted to include a wiki article on it in the module docs, but it's only listed as a part of Oauth...

@teamon
Copy link
Member

teamon commented May 5, 2021

Hi @mdlkxzmcp

I appreciate the contribution!

If I understand correctly, what this middlewares does when used like this:

Tesla.client [
  {Tesla.Middleware.BearerAuth, Map.merge(%{token: token}, opts))}
]

is essentially the same as using Tesla.Middleware.Headers directly:

Tesla.client [
  {Tesla.Middleware.Headers, [{"authorization", "Bearer #{token}}]}
]

In general, adding such new middleware shouldn't be needed (as one can use the more generic Headers middleware).
However, we do include BaseAuth middleware for a nicer user experience, and I think it doesn't hurt to have a BearerAuth too.

Having said that, I'd like to add as little code as possible, as each line of code needs to be maintained for a long time. And this applies also to tests.

I'd suggest a minimal implementation, something along (not tested)

def call(env, next, opts) do
  token = Keyword.fetch!(opts, :token)

  env
  |> Tesla.put_headers({"authorization", "Bearer #{token}})
  |> Tesla.run(next)
end

and then just one or two test cases testing the middleware in isolation.

Please do let me know if you have any questions regarding all that.

@mdlkxzmcp
Copy link
Contributor Author

Hey @teamon!

It is indeed essentially the same as setting the header, however, I felt that, since it's related to authentication, it would be beneficial to have it as its own middleware.

I'm totally ok with rewriting it to be simpler like you showed. I simply wanted to follow the logic from other middleware modules :)

Should have a rewrite by the end of the day :>

@mdlkxzmcp mdlkxzmcp force-pushed the add_bearer_auth_middleware branch from 117768a to 9e69374 Compare May 6, 2021 07:22
@teamon teamon merged commit 8e76fdb into elixir-tesla:master Jun 24, 2021
@mdlkxzmcp mdlkxzmcp deleted the add_bearer_auth_middleware branch June 24, 2021 20:09
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.

2 participants