search

Colin-b / httpx_auth

Authentication classes to be used with httpx
MIT License
114 stars 26 forks source link

Basic auth should not be enforced for resource owner password flow #54

Closed paul121 closed 1 year ago

paul121 commented 2 years ago

I do not believe that the token request for resource owner password flow requires that the server accept basic auth. See the spec: https://datatracker.ietf.org/doc/html/rfc6749#section-4.3

The spec does, however, require that the server support basic auth for the client credentials grant: https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1

I could not authenticate with the Drupal Simple OAuth server implementation with Resource Owner Password creds until I removed the basic auth in OAuth2ResourceOwnerPasswordCredentials::_configure_client: https://github.com/Colin-b/httpx_auth/blob/75e0d19491df124e0808fd1c09cc7a18a87ec0c4/httpx_auth/authentication.py#L220

I think we could remove this altogether. If a server does require/support basic auth for this then a httpx client can be provided to OAuth2ResourceOwnerPasswordCredentials with this configured. As it is currently implemented basic auth will always be used. If this approach makes sense I'm happy to provide a PR.

Colin-b commented 2 years ago

Hi @paul121 ,

This is indeed a bug, we should not make the client authentication mandatory.

I would go for a breaking change as you said, meaning that the authentication method is up to the client (as it is not fixed to basic auth according to RFC).

A pull request is welcome of course, please make sure to document the change and update the test suite accordingly (meaning you will have to add a test case checking the behavior when a client with basic auth is provided).

Note that I might not be able to review the PR right away (holiday season ;))

Colin-b commented 2 years ago

Also another PR adding an authentication class for Drupal could be a nice addition if it makes your life easier :)

perusio commented 1 year ago

@Colin-b wouldn't it be preferable to breaking changes instead to introduce a keyword argument, e.g.,

no_client_auth: bool=False

such that it would work as it works now by default, but by setting it to True would skip the Basic Auth part?

Colin-b commented 1 year ago

Hello @perusio and @paul121 , I plan on fixing this issue this week. Apologies for the delay, I will let you know.

Colin-b commented 1 year ago

Version 0.17.0 is now available on pypi. This version will not use any authentication by default.

Should you need authentication, it can be provided via the new client_auth parameter (supporting a 2-tuple containing user and password for basic auth, or any authentication class instance).

paul121 commented 1 year ago

Thanks @Colin-b ! It has been awhile but I hope to try this library again.

perusio commented 1 year ago

@Colin-b thank you I will try it out.