expires_in field ignored if string instead of int and access token is not renewed when expired

[gerarar]

• requests_oauth2client version: latest
• Python version: N/A
• Operating System: N/A

Description

When the expires_in field in the token response is a string, the accepts_expires_in decorator ignores that field since it's not an int. Then the expires_at never gets set and passed to the BearerToken class, and the token is never renewed once it expires.
eg.

{
    "token_type": "Bearer",
    "issued_at": "1111111111111",
    "client_id": "client-id",
    "access_token": "access-token",
    "scope": "",
    "expires_in": "3599"
}

What I Did

I was able to fix this locally by adding an additional check to the accepts_expires_in decorator in utils.py.

if expires_in is None and expires_at is None:
    return f(*args, **kwargs)
if expires_in and isinstance(expires_in, str) and expires_in.isdigit():
    expires_at = datetime.now() + timedelta(seconds=int(expires_in))
elif expires_in and isinstance(expires_in, int) and expires_in >= 1:
    expires_at = datetime.now() + timedelta(seconds=expires_in)
return f(*args, expires_at=expires_at, **kwargs)

The second if checks if the expires_in is a string and then a valid int, otherwise it continues to the other checks.

Source code to be changed: https://github.com/guillp/requests_oauth2client/blob/main/requests_oauth2client/utils.py#L68-L72

[guillp]

Thanks for the report!

I would say that this kind of bug should be fixed on the Authorization Server side, not on the client, since the server does not respect RFC6749 $5.1 which specifies that expires_in must be an integer:

The parameters are included in the payload of the HTTP response using the application/json media type as defined by [RFC7159]. The parameters are serialized into a JavaScript Object Notation (JSON) structure by adding each parameter at the highest structure level. Parameter names and string values are included as JSON strings. Numerical values are included as JSON numbers. The order of parameters does not matter and can vary.

What AS implementation are you using? Did you report a bug there?

However, I must admit that I had to read the specification twice because the part describing expires_in does not mention that it must be an integer, and the example value is quoted in the description:

expires_in
RECOMMENDED. The lifetime in seconds of the access token. For
example, the value "3600" denotes that the access token will
expire in one hour from the time the response was generated.
If omitted, the authorization server SHOULD provide the
expiration time via other means or document the default value.

... so it is easy to make a mistake if you don't read the specification thoroughly. The example however is an integer:

{
"access_token":"2YotnFZFEjr1zCsicMWpAA",
"token_type":"Bearer",
"expires_in":3600,
"refresh_token":"tGzv3JOkF0XG5Qx2TlKWIA",
"example_parameter":"example_value"
}

Since the client side fix is nice and clean, I would accept a PR if you submit one. Or I will include that same fix sooner or later.

[gerarar]

We utilize Google Cloud Apigee as our AS, and they set the expires_in field as a string for # of secs, which we have no control over.
GC Documentation here: https://cloud.google.com/apigee/docs/api-platform/reference/policies/oauthv2-policy#generateresponseelement

We only just came across this issue when we started seeing random 401 errors, which we then traced it back to expired bearer access tokens. I then did some quick debugging and saw the expires_at was being set to None in the bearer token class.

I can try to put in a PR and try writing a test for it :)

[guillp]

We utilize Google Cloud Apigee as our AS, and they set the expires_in field as a string for # of secs.
Documentation here: https://cloud.google.com/apigee/docs/api-platform/reference/policies/oauthv2-policy#generateresponseelement

Oh I love when big corporations don't have the ressources to implement a simple, decade-old, standard properly... ^^

Since their Token Response is heavily customised with plenty of additionnal/non-standard fields (including token_type=BearerToken instead of Bearer), I would suggest that you implement a BearerToken subclass that properly handles those apigee-specific fields. You can then subclass OAuth2Client to use that subclass, like this:

class ApigeeBearerToken(BearerToken):
    TOKEN_TYPE = "BearerToken"
    # implement an __init__ that takes expires_in as a string, and optionally handles extra apigee attributes

class ApigeeClient(OAuth2Client):
    token_class = ApigeeBearerToken

client = ApigeeClient("https://whatever.google.com")

[gerarar]

Made the changes here and opened PR #27.

[gerarar]

Also just saw your previous comment.

So we're currently using the OAuth2ClientCredentialsAuth along with the OAuth2Client client, which currently satisfies our needs.

_oauth2_client = OAuth2Client(
    token_endpoint=token_endpoint,
    auth=(os.environ["CLIENT_ID"], os.environ["CLIENT_SECRET"]),
)
AUTH_CLIENT = OAuth2ClientCredentialsAuth(_oauth2_client)

I guess making a separate Apigee client and bearer token class is doable, but the current implemented classes in the project are already enough, it was just expires_in str -> int that through us for a loop.

[guillp]

Closing this, thanks again for the PR!