-
-
Notifications
You must be signed in to change notification settings - Fork 425
Add automatic expires_at calculation from expires_in #565
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
base: master
Are you sure you want to change the base?
Conversation
This change adds automatic calculation of expires_at from expires_in in the OAuth2 token response. This improves the automatic token refresh functionality by ensuring that expires_at is always set correctly without requiring manual intervention. - Added automatic calculation of expires_at when expires_in is present - Uses float precision for accurate expiration tracking - Maintains backward compatibility with existing code - All tests pass on Python 3.9 Fixes requests#561
@@ -404,7 +422,7 @@ def fetch_token( | |||
r = hook(r) | |||
|
|||
self._client.parse_request_body_response(r.text, scope=self.scope) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't r
here have the actual time the server sent the response?
That should be more reliable than time.time()
considering delays and clock skew between hosts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also should completely avoid fiddling with floats because both is in whole seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hramrach would that just be the Date
header in response?
I am not sure if we're always guaranteed to have that so wdyt about a fallback to time.time()
if not present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hramrach - addressed your feedback! can you take another look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, HTTP does not guarantee a date header.
A sender that generates a Date header field SHOULD generate its field value as the best available approximation of the date and time of message generation. In theory, the date ought to represent the moment just before generating the message content. In practice, a sender can generate the date value at any time during message origination.
An origin server with a clock (as defined in [Section 5.6.7](https://httpwg.org/specs/rfc9110.html#http.date)) MUST generate a Date header field in all [2xx (Successful)](https://httpwg.org/specs/rfc9110.html#status.2xx), [3xx (Redirection)](https://httpwg.org/specs/rfc9110.html#status.3xx), and [4xx (Client Error)](https://httpwg.org/specs/rfc9110.html#status.4xx) responses, and MAY generate a Date header field in [1xx (Informational)](https://httpwg.org/specs/rfc9110.html#status.1xx) and [5xx (Server Error)](https://httpwg.org/specs/rfc9110.html#status.5xx) responses.
When storing the Date value using time.time() as fallback is appropriate. I don't think storing the fractional seconds is needed, or adds any value.
- Changed expires_at calculation to use int() for consistent truncation - Added Date header parsing with fallback to time.time() - Updated tests
try: | ||
# Parse HTTP date format (RFC 7231) | ||
dt = datetime.strptime(response_date, "%a, %d %b %Y %H:%M:%S GMT") | ||
# Convert UTC time tuple to Unix timestamp (returns integer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GMT here should be parsed as %Z. Otherwise the time is likely wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not seem to make any difference:
TZ=PDT python3.13
Python 3.13.5 (main, Jun 12 2025, 00:40:24) [GCC] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from datetime import datetime
>>> datetime.strptime('Tue, 02 Sep 2025 10:28:50 GMT', "%a, %d %b %Y %H:%M:%S GMT")
datetime.datetime(2025, 9, 2, 10, 28, 50)
>>> datetime.strptime('Tue, 02 Sep 2025 10:28:50 GMT', "%a, %d %b %Y %H:%M:%S %Z")
datetime.datetime(2025, 9, 2, 10, 28, 50)
>>> datetime.strptime('Tue, 02 Sep 2025 10:28:50 GMT', "%a, %d %b %Y %H:%M:%S %Z").tzinfo
>>> datetime.strptime('Tue, 02 Sep 2025 10:28:50 GMT', "%a, %d %b %Y %H:%M:%S GMT").tzinfo
See also https://docs.python.org/3/library/datetime.html#datetime.datetime.utcnow
Warning
Because naive datetime objects are treated by many datetime methods as local times, it is preferred to use aware datetimes to represent times in UTC. As such, the recommended way to create an object representing the current time in UTC is by calling datetime.now(timezone.utc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll still parse with %Z for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would hope that parsing with %Z would make strptime
generate the tzinfo
but no, there is no tzinfo
either way.
As the warning in the documentation suggests handling of time data in python is less than ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To spell it out more clearly: with timezones python leans towards implicit rather than explicit, and in the long run that is a mess because you never know in what timezone your timestamp is.
Then to get the best possible result you would parse GMT as fixed string. Then you know that the timastamp is GMT, and can manually add explicit tzinfo
hardcoded to timezone.utc
making the timestamp unambiguous.
Or you can forget all this, leave the code as is, hope it works with the implicit timezones.
Also in general this adds the For a full fix something needs to use the new field instead of |
@hramrach my initial thought was that the oauth library would raise a TokenExpiredError here which would result in refreshing of the token here in requests-oauthlib. But I see now that oauthlib only checks _expires_at - shouldn't it check expires_in at least? and to make this work, we'd need to make it support the new expires_at field too? Let me know if that sounds right! and i can create an issue / put up a fix! |
Indeed, although the expires_at is non-standard it is already supported. Then calculating it when not present should suffice. Thanks |
This change adds automatic calculation of expires_at from expires_in in the OAuth2 token response. This improves the automatic token refresh functionality by ensuring that expires_at is always set correctly without requiring manual intervention.
Fixes #561
Documentation
The "Refreshing tokens" section in
oauth2_workflow.rst
should be updated to mention thatexpires_at
is now automatically calculated fromexpires_in
.Testing
expires_at
calculation