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

Allow authorizing without basic auth header #142

Closed
sheharyarn opened this issue Aug 23, 2019 · 7 comments
Closed

Allow authorizing without basic auth header #142

sheharyarn opened this issue Aug 23, 2019 · 7 comments

Comments

@sheharyarn
Copy link
Contributor

sheharyarn commented Aug 23, 2019

Updating to v2.0.0 broke a few OAuth2 based integrations in my application. Since then I've been going over the changelog, commit history and the related issues to figure out what caused this.

Looking at Issue #128, PR #131 and the RFC spec, it's clear that you're implementing it correctly. But the fact of the matter is that many services don't support Basic Auth headers at all and instead require the client to send client_id and client_secret in params. Examples include Wrike and ClickUp.

Of course, we can just set these params ourselves, but services like QuickBooks (#140) and Dropbox require either the params to be present or the basic auth header, but not both. Otherwise, the OAuth2 server will return an error.

This is even more annoying when working with refresh tokens with services that don't support basic auth headers, because we end up creating a custom refresh strategy and duplicating the Client.refresh_token/3 code:

defmodule CustomRefresh do
  use OAuth2.Strategy

  @impl true
  defdelegate authorize_url(client, params), to: OAuth2.Strategy.Refresh

  @impl true
  def get_token(client, params, headers) do
    client
    |> OAuth2.Strategy.Refresh.get_token(params, headers)
    |> update_in([Access.key(:headers)], &List.keydelete(&1, "authorization", 0))
    |> OAuth2.Client.put_param(:client_id, client.client_id)
    |> OAuth2.Client.put_param(:client_secret, client.client_secret)
  end


  def refresh_token(%OAuth2.Client{} = client) do
    case client.token.refresh_token do
      nil ->
        {:error, :refresh_token_unavailable}

      refresh_token ->
        client
        |> Map.put(:strategy, __MODULE__)
        |> OAuth2.Client.put_param(:refresh_token, refresh_token)
        |> OAuth2.Client.get_token()
        |> case do
          {:ok, client} ->
            if client.token.refresh_token do
              {:ok, client}
            else
              {:ok, put_in(client.token.refresh_token, refresh_token)}
            end

          {:error, error} ->
            {:error, error}
        end
    end
  end
end
@sheharyarn
Copy link
Contributor Author

sheharyarn commented Aug 23, 2019

For starters, this line should be removed since it's not part of the spec, so that services like Quickbooks and Dropbox continue working as expected:

https://github.com/scrogson/oauth2/blob/be5982b7728aa5084902891f98e3d52a9e423a5f/lib/oauth2/strategy/auth_code.ex#L54

Second: while I understand you're implementing the spec correctly, there should be a way to tell the client the desired authentication mechanism. Something like this would be very helpful when configuring the client:

def client do
  OAuth2.Client.new([
    strategy: __MODULE__,
    client_id: "...",
    client_secret: "...",
    authentication_mode: OAuth2.Mode.BasicAuthHeader # or OAuth2.Mode.ClientParams
    ...
  ])
end

We can default to BasicAuthHeader mode since that is the preferred method of sending client credentials.

@sheharyarn
Copy link
Contributor Author

Update: I reached out to some vendors and requested them to add support for basic auth header, but both of them refused to do so since sending client params are "more common and preferred".

@mkreyman
Copy link

I'm having a similar issue. Provider that I'm writing a custom strategy for doesn't use either client_secret or basic auth header. I had to clone your repo and then comment out |> basic_auth() on line 59 in lib/oauth2/strategy/auth_code.ex, to be able to get a token. I haven't worked on the refresh token part yet, to comment on that. I need a way to optionally turn basic auth header off.

@mkreyman
Copy link

mkreyman commented Jan 24, 2020

Had to do the following to get rid of the header that was causing 406 with my provider...

def get_token(client, params, headers) do
  client
  |> put_header("Accept", "application/json")
  |> OAuth2.Strategy.AuthCode.get_token(params, headers)
  |> delete_header(:authorization)
end
...

defp delete_header(%{headers: headers} = client, key) do
  %{client | headers: List.keydelete(headers, "#{key}", 0)}
end

@scrogson
Copy link
Member

@mkreyman if you're implementing your own strategy which doesn't comply with the base OAuth2 spec, you should probably just not pipe through OAuth2.Strategy.AuthCode and build up the request as your server requires.

masonforest added a commit to masonforest/ueberauth_dropbox that referenced this issue Nov 17, 2021
Other changes:

* Use the new [Helpers.html#with_state_param](https://hexdocs.pm/ueberauth/Ueberauth.Strategy.Helpers.html#with_state_param/2)
* Configure a JSON serializer
* Remove Basic Auth from the `get_token` request as suggested [here](ueberauth/oauth2#142 (comment)).
Copy link

github-actions bot commented Feb 8, 2024

This issue has been automatically marked as "stale:discard". If this issue still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment.

Copy link

Closing this issue after a prolonged period of inactivity. If this issue is still relevant, feel free to re-open the issue. Thank you!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants