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

Remove client_id and client_secret from body (generated via prepare_r… #326

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DmitryPaschenko
Copy link

Remove client_id and client_secret from body (generated via prepare_request_body) if HTTPBasicAuth used.

Usage:

token = oauth.fetch_token(
token_url='token_url_here',
code='code_here',
client_id='client_id_here',
client_secret='client_secret_here'
)

…equest_body) if HTTPBasicAuth used.

Usage:

token = oauth.fetch_token(
    token_url='token_url_here',
    code='code_here',
    client_id='client_id_here',
    client_secret='client_secret_here'
)
@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.025% when pulling 3dc13ad on DmitryPaschenko:fetch_token_without_auth into f8e741d on requests:master.

@JonathanHuot
Copy link
Contributor

Hi @DmitryPaschenko, the problem in this is that it takes out client_id from kwargs but it can still be present in self._client property. So, if you use OAuth2Session(client_id=xx) it will still be taken in account.

@jvanasco is working on a wider patch in oauthlib oauthlib/oauthlib#593 and requests-oauthlib (PR TBC).

@lepture
Copy link

lepture commented Sep 17, 2019

@JonathanHuot the concept in current implementation is not good to understand. Here are my thoughts:

There are two types of requests:

  1. using client information to get token
  2. using token to request other things

For "using token to request other things", currently, requests-oauthlib is using auth parameter which is great.

Why not use the auth feature in fetch_token too? Currently, it only happens in some cases like here https://github.com/requests/requests-oauthlib/blob/master/requests_oauthlib/oauth2_session.py#L291


From the later RFCs, we can understand fetching access token in this way:

we use client authentication information to exchange access token.

There are several methods for client authentication:

  1. none (just client_id)
  2. client_secret_basic (client_id and client_secret in header, using "basic" auth)
  3. client_secret_post (client_id and client_secret in payload "body")

For fetching token, this is usually called token_endpoint_auth_method.

You can get some inspiration from Authlib. Checkout

  1. https://github.com/lepture/authlib/blob/master/authlib/oauth2/client.py
  2. https://github.com/lepture/authlib/blob/master/authlib/oauth2/auth.py
  3. https://github.com/lepture/authlib/blob/master/authlib/client/oauth2_session.py

When fetching token, we just pass an auth parameter, this auth will decide where to add the client_id or client_secret. For instance:

auth = ClientAuth(client_id, client_secret, 'client_secret_basic')

This auth will add client_id, client_secret in header.

auth = ClientAuth(client_id, client_secret, 'client_secret_post')

This auth will add client_id, client_secret in body.

@jvanasco
Copy link
Contributor

Why not use the auth feature in fetch_token too?

unless I am missing something, fetch_token already accepts an auth

It is defined as

    :param auth: An auth tuple or method as accepted by `requests`.

the block you cited only creates a HTTP Basic Auth header if the auth parameter was not provided and the requires/is-intended-to-have the header.

@lepture
Copy link

lepture commented Sep 17, 2019

@jvanasco I mean, for client_secret_post and none we can also use auth.

@jvanasco
Copy link
Contributor

I am sorry for wildly misinterpreting your original message. I didn't have enough coffee this morning - but I understand it now.

you should probably create a new issue ticket with your above content. I think this PR ticket is a candidate to be closed, because it should have been made obsolete by PR #593 last year.

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.

5 participants