Problems with Token refreshing.

[btimby]

First of all, thank you for this wonderful library. It works great and saved me tons of time.

I am having issues with one aspect: auto token refresh. I have identified 3 distinct problems, which I will describe below and for which I will suggest solutions. I will also reference this issue from multiple other related issues. I am happy to provide a PR once we can agree upon a suitable solution.

I derived my solution from the following, as my issue is a variant of the same issue:

#260

All of the problems start here:

https://github.com/requests/requests-oauthlib/blob/master/requests_oauthlib/oauth2_session.py#L338

    auth = kwargs.pop('auth', None)
    if client_id and client_secret and (auth is None):
        log.debug('Encoding client_id "%s" with client_secret as Basic auth credentials.', client_id)
        auth = requests.auth.HTTPBasicAuth(client_id, client_secret)
    token = self.refresh_token(
        self.auto_refresh_url, auth=auth, **kwargs)

Issue 1

If I pass an auth kwarg intended for the request() method, it is eradicated here. I am not sure if this is an issue in practice.

Issue 2

Box, Google and Onedrive do not expect the client_id and client_secret to be provided via Basic authentication ala Authorization header. They instead expect these POSTed in the body. There is no way to customize this behavior. Also, I don't know what vendors expect what method here, I only know the 3 above as that is what I am working with.

Issue 3

Any kwargs I pass which are intended for the request() method end up being passed to refresh_token() and are used when accessing the token endpoint. For example, if my original request() uploads a file, that file will be transferred to the token endpoint when attempting to refresh the token.

Issue 4

client_id and client_secret are not normally passed to request() but are necessary for token refresh. Passing them is not a problem except for unintended consequences of them being passed along to super().request()

Proposed solution.

I think all of the above can be solved by making two changes.

1. Pass the client_id and client_secret to __init__() where they are stored as attributes (of OAuth2Session or underlying WebApplicationClient. In fact client_id is already handled this way, but fetch_token(), refresh_token() etc. do no utilize it and expect it passed again as a kwarg. This addresses issue 4.

2. Add a refresh_kwargs argument that accepts a dict, and provides kwargs passed along to token_refresh() thus keeping them separate from the regular kwargs intended for request(). This solves issues 1 & 3.

3. Place the client_id and client_secret into the POST body by default, all three of the providers I have tried expect this instead of basic auth. Callers can use refresh_kwargs to provide an auth kwarg if necessary (their provider expects this instead). Optionally, if auth is passed, the client_id and client_secret could be omitted from the POST body (it is unlikely that a provider expects both). This solves issue 3.

I present my current workaround, which works perfectly for my providers.

    def __init__(self):
        # Omit token refresh args, we do this manually
        self.oauthsession = OAuth2Session(token=token, **kwargs)

    def request(self, method, url, chunk, headers={}, **kwargs):
        """
        Perform HTTP request for OAuth.
        """
        while True:
            try:
                return self.oauthsession.request(method, url, headers=headers,
                                                 **kwargs)
            except TokenExpiredError:
                # Do our own, since requests_oauthlib is broken.
                token = self.oauthsession.refresh_token(
                    self.REFRESH_TOKEN_URL,
                    refresh_token=self.oauth_access.refresh_token,
                    client_id=self.provider.client_id,
                    client_secret=self.provider.client_secret)
                self._refresh_token_callback(token)
                continue

[Lukasa]

Thanks for this issue report!

If I pass an auth kwarg intended for the request() method, it is eradicated here.

It should not be: the (auth is None) clause should prevent entering that block if a custom auth handler was provided.

As for issue 2, this was raised in #218 and has not since been resolved. I think we should fix it.

In general, though, I would be happy to merge a PR that resolves your problems 2, 3, and 4. 😄

[btimby]

I missed #218 yes, the three providers I reference do auth via body not basic auth.

New propsed change is to add refresh_kwargs to __init__(). If refresh_kwargs is present, basic auth will be skipped, giving caller control over auth method for refresh tokens. We can also fold the refresh_token_url into this new dict. This fixes 2, 3 & 4. So my case is handled like so:

    self.oauthsession = OAuth2Session(..., refresh_kwargs={'url': refresh_url,
        'client_id': client_id, 'client_secret': client_secret'})
    ...
    return self.oauthsession.request(method, url, headers=headers, **kwargs)

[Lukasa]

Yeah, I think that is reasonable. That, or we'd want to use a separate function to register those: a function you'd call after init. Either would be appropriate.

[btimby]

There is actually already auto_refresh_kwargs accepted by __init__(). But auth is generated anyway.

[btimby]

Not tested (yet):

btimby@cf3ff0a

Will make PR after I test.

[btimby]

And actually, in reference to Issue 1 above, there is still a problem with the auth kwarg. If I pass it to customize my authentication for token refresh and the tokens is NOT expired, then the kwarg is not popped, and is passed to request(). On the other hand, if the token is expired, the kwarg is popped and NOT passed to request().

The API around this is really complicated, since refresh_token() args can come from kwargs to refresh() or auto_refresh_kwargs. OR auth is auto-generated in certain cases.

To simplify this would be a breaking change, it would require relying solely on auto_refresh_kwargs, so the caller has explicit control over the authentication. I think it is probably what is wanted since providers are free to implement their own methods...

Thoughts?

[btimby]

The spec leaves authentication method up to the implementer.

6.  Refreshing an Access Token

   If the authorization server issued a refresh token to the client, the
   client makes a refresh request to the token endpoint by adding the
   following parameters using the "application/x-www-form-urlencoded"
   format per Appendix B with a character encoding of UTF-8 in the HTTP
   request entity-body:

   grant_type
         REQUIRED.  Value MUST be set to "refresh_token".

   refresh_token
         REQUIRED.  The refresh token issued to the client.

   scope
         OPTIONAL.  The scope of the access request as described by
         Section 3.3.  The requested scope MUST NOT include any scope
         not originally granted by the resource owner, and if omitted is
         treated as equal to the scope originally granted by the
         resource owner.

   Because refresh tokens are typically long-lasting credentials used to
   request additional access tokens, the refresh token is bound to the
   client to which it was issued.  If the client type is confidential or
   the client was issued client credentials (or assigned other
   authentication requirements), the client MUST authenticate with the
   authorization server as described in Section 3.2.1.

   For example, the client makes the following HTTP request using
   transport-layer security (with extra line breaks for display purposes
   only):

     POST /token HTTP/1.1
     Host: server.example.com
     Authorization: Basic czZCaGRSa3F0MzpnWDFmQmF0M2JW
     Content-Type: application/x-www-form-urlencoded

     grant_type=refresh_token&refresh_token=tGzv3JOkF0XG5Qx2TlKWIA

   The authorization server MUST:

   o  require client authentication for confidential clients or for any
      client that was issued client credentials (or with other
      authentication requirements),

   o  authenticate the client if client authentication is included and
      ensure that the refresh token was issued to the authenticated
      client, and

   o  validate the refresh token.

   If valid and authorized, the authorization server issues an access
   token as described in Section 5.1.  If the request failed
   verification or is invalid, the authorization server returns an error
   response as described in Section 5.2.

   The authorization server MAY issue a new refresh token, in which case
   the client MUST discard the old refresh token and replace it with the
   new refresh token.  The authorization server MAY revoke the old
   refresh token after issuing a new refresh token to the client.  If a
   new refresh token is issued, the refresh token scope MUST be
   identical to that of the refresh token included by the client in the
   request.

While the example shows HTTP basic authentication, there is a reference to section 3.2.1, which covers authentication for the token endpoint as a whole. It states there MUST be authentication for confidential clients, but does not specifically lay out the scheme required.

3.2.1.  Client Authentication

   Confidential clients or other clients issued client credentials MUST
   authenticate with the authorization server as described in
   Section 2.3 when making requests to the token endpoint.  Client
   authentication is used for:

   o  Enforcing the binding of refresh tokens and authorization codes to
      the client they were issued to.  Client authentication is critical
      when an authorization code is transmitted to the redirection
      endpoint over an insecure channel or when the redirection URI has
      not been registered in full.

   o  Recovering from a compromised client by disabling the client or
      changing its credentials, thus preventing an attacker from abusing
      stolen refresh tokens.  Changing a single set of client
      credentials is significantly faster than revoking an entire set of
      refresh tokens.

   o  Implementing authentication management best practices, which
      require periodic credential rotation.  Rotation of an entire set
      of refresh tokens can be challenging, while rotation of a single
      set of client credentials is significantly easier.

   A client MAY use the "client_id" request parameter to identify itself
   when sending requests to the token endpoint.  In the
   "authorization_code" "grant_type" request to the token endpoint, an
   unauthenticated client MUST send its "client_id" to prevent itself
   from inadvertently accepting a code intended for a client with a
   different "client_id".  This protects the client from substitution of
   the authentication code.  (It provides no additional security for the
   protected resource.)

Some grant types for example "Resource Owner Password Grant" would obviously use HTTP basic auth, others would not. There are OAuth2 profiles for using JWT, SAML etc. for performing authentication.

So it is my understanding that there is nothing special about HTTP basic authentication, in fact, I don't use it at all. Letting the caller control this completely seems like the right API. The caller can include the auth key to auto_refresh_kwarg if that is the intended method, or include other options otherwise.

[btimby]

Here is a breaking change that makes the API explicit. Caller can call refresh_token() and pass their arguments explicitly (as always). Caller also explicitly controls same arguments when refresh_token() is called automatically.

btimby@f22426b

What do you think of this?

[Lukasa]

This ambiguity is part of why OAuth2 is a well-loathed specification: it essentially doesn't mandate anything, but is littered with MAYs that grant an astonishing amount of freedom to implementers to do whatever the hell they want. This makes it very difficult to write a single library that has meaningfully helpful automation across a number of OAuth2 providers.

That said, the general direction of this change seems reasonable, though I'll want to see a PR before I can really commit to saying that it's the right way to go. 😄

[btimby]

I understand. This is why I propose just letting the caller decide. Although my experience so far is only with a handful of providers, my code to deal with them is fairly complex.

It seems from the current issues that refreshing tokens is a pain point for a bunch of users, and giving them explicit control would solve those problems. What I don't know however is how many folks would be affected if this "auto authentication" were to go away...

I will finalize and test my changes and submit a PR.

[btimby]

And by the way, I agree, I wish the spec were more explicit.