Replace Tesla usage with customizable HTTP Client

[martosaur]

Hi 👋

https://github.com/ueberauth/oauth2/pull/170 was a great addition to the library, and I think we can make it even better!

Right now OAuth2 is very specific in the way it creates Tesla client, requiring adapter and middleware to be specified in oauth2 configuration. This prevents users from utilizing Tesla's full potential which includes module-based clients and runtime middleware. The real life example would be an app that uses OAuth2 to make requests to a number of services and wanting to use different telemetry middleware for each of those.

The proposed solution is inspired by goth approach: we can define a OAuth2.HttpClient behaviour which allows users to implement their own http client. But to make it easier for everyone, we make this behaviour match Tesla interface (Tesla.request/2 to be specific), so that for all users who don't mind using Tesla they could just set config :oauth2, http_client: Tesla and everything will work as it used to be.

To accommodate runtime middleware I also added an ability to pass tesla client and module to OAuth2.Client.new:

      iex> tesla_client = Tesla.client(runtime_middleware, Tesla.Adapter.Hackney)
      iex> OAuth2.Client.new(token: "123", http_client: {tesla_client, Tesla})

For non-Tesla users this will just provide a way to pass an arbitrary variable to their OAuth2.HttpClient.request/2 implementation.

As a bonus, Tesla will now be an optional dependency for the package.

Please let me know what you think!

[yordis]

Hey there, thank you so much for taking the time to contribute.

I would like to hear more about what you think about adding this abstraction. Ideally, a real production problem and usage that requires you to reach for such capabilities.

[martosaur]

Thanks for taking a look! Sure, I can think of multiple examples. Suppose you have an app that allows users to log in through either Google or Microsoft and you want to use Tesla.Middleware.Telemetry to export metrics to your Grafana dashboard. Naturally, you'd want to tag those metrics as either google or microsoft, and the middleware supports this with metadata option:

Tesla.Middleware.Telemetry, metadata: %{client: :google}

However, this option isn't currently available as OAuth2 will always pick middleware from the global configuration option (which itself is something although very popular, but generally discouraged)

Another example I can immediately think of is combining bypass and mock-based tests. To test with bypass we need to use a real adapter, like hackney or mint. For mock-based tests there is Tesla.Mock adapter or Mox. For this one I suspect there is a workaround, since we can update application configuration in tests setup, but this is really a last resort as it often makes tests impossible to run asynchronously.

[yordis]

Hey, thank you for getting back to us,

I appreciate the effort you put in, and your arguments are heard. Unfortunately, I am going to make the decision to reject the changes.

Tesla is already the abstraction around swappable HTTP clients, and also has a middleware pipeline you can fully control.

---

I can think of multiple examples.

I understand that I would love to hear from you about existing production needs instead of thinking about potential features and limitations caused by the implementation. You should know that we need to be pragmatic and cautious about introducing complexity based on hypothetical.

Suppose you have an app that allows users to log in through either Google or Microsoft and you want to use Tesla.Middleware.Telemetry to export metrics to your Grafana dashboard. Naturally, you'd want to tag those metrics as either google or microsoft, and the middleware supports this with metadata option:

You can currently inject any middleware you want https://github.com/ueberauth/oauth2/blob/e8bb2105a6dedaaf423efc1d02b4666cbc8ca43d/lib/oauth2/request.ex#L91

Another example I can immediately consider is combining bypass and mock-based tests. To test with bypass we need to use a real adapter, like hackney or mint. For mock-based tests there is Tesla.Mock adapter or Mox. For this one I suspect there is a workaround, since we can update application configuration in tests setup, but this is really a last resort as it often makes tests impossible to run asynchronously.

They are multiple ways to deal with the problem, I would come up with real situations so we can make a stronger decision.

[martosaur]

I'm sorry that I made those cases sound hypothetical. Both of them are very real for the application I work on at my day job.

You can currently inject any middleware you want

Could you please expand a little on this? The quoted line reads from the global application configuration. If I put this in config.exs or even runtime.exs:

config :oauth2,
  middleware: [
    {Tesla.Middleware.Telemetry, metadata: %{provider: :google}}
  ]

all requests made through OAuth2 will be tagged with client: :google tag, regardless of the actual provider. Is there some workaround I'm missing?

[yordis]

You can always write your middleware for Telemetry that looks at the URL, or I am OK with a PR to pass enough info into the pipeline that allows you to simplify this if it is not possible today based on the env from Tesla.

The telemetry middleware already gives you the entire env https://github.com/elixir-tesla/tesla/blob/d3ea190250f2807077dcc32d0d6ff85ea27b671a/lib/tesla/middleware/telemetry.ex#L96C7-L96C17 So you can manipulate however you like the telemetry event.

config :oauth2, middleware: [ {Tesla.Middleware.Telemetry, metadata: %{provider: :google}} ]

That should work https://github.com/elixir-tesla/tesla/blob/d3ea190250f2807077dcc32d0d6ff85ea27b671a/lib/tesla/middleware/telemetry.ex#L93

Make sure there are no misconfigurations happening and that it is not an issue with the Tesla version or with Tesla itself.

[martosaur]

Makes sense, creating a custom wrapper over any built-in middleware would work. Thank you for looking into this!