-
-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,7 @@ | ||
import logging | ||
import time | ||
import calendar | ||
from datetime import datetime | ||
|
||
from oauthlib.common import generate_token, urldecode | ||
from oauthlib.oauth2 import WebApplicationClient, InsecureTransportError | ||
|
@@ -171,6 +174,39 @@ def authorized(self): | |
""" | ||
return bool(self.access_token) | ||
|
||
def _add_expires_at(self, token, response_date=None): | ||
"""Add expires_at to token if expires_in is present. | ||
|
||
OAuth2 responses often include expires_in (seconds until token expires) but | ||
oauthlib expects expires_at (timestamp when token expires) for expiration checks. | ||
|
||
Uses response Date header if provided. Falls back to current time if | ||
Date header is not provided or malformed. | ||
|
||
:param token: OAuth2 token dict | ||
:param response_date: Optional Date header value from response (e.g. "Thu, 14 Mar 2024 08:30:00 GMT") | ||
:return: Token dict with expires_at if expires_in was present | ||
""" | ||
if token and 'expires_in' in token: | ||
# RFC 6749 requires expires_in to be 1*DIGIT, but some providers send it as string | ||
expires_in = int(token['expires_in']) | ||
|
||
# Try to use response Date header if provided | ||
if response_date: | ||
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) | ||
token['expires_at'] = calendar.timegm(dt.utctimetuple()) + expires_in | ||
return token | ||
except (TypeError, ValueError): | ||
# Skip if Date header is malformed or uses non-standard format | ||
log.debug("Failed to parse Date header: %s", response_date) | ||
|
||
# Fall back to current time (truncate to second for conservative expiry) | ||
token['expires_at'] = int(time.time()) + expires_in | ||
return token | ||
|
||
def authorization_url(self, url, state=None, **kwargs): | ||
"""Form an authorization URL. | ||
|
||
|
@@ -404,7 +440,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 commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't That should be more reliable than There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. @hramrach would that just be the I am not sure if we're always guaranteed to have that so wdyt about a fallback to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, HTTP does not guarantee a date header.
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. |
||
self.token = self._client.token | ||
self.token = self._add_expires_at(self._client.token, response_date=r.headers.get('Date')) | ||
log.debug("Obtained token %s.", self.token) | ||
return self.token | ||
|
||
|
@@ -494,6 +530,7 @@ def refresh_token( | |
r = hook(r) | ||
|
||
self.token = self._client.parse_request_body_response(r.text, scope=self.scope) | ||
self.token = self._add_expires_at(self.token, response_date=r.headers.get('Date')) | ||
if "refresh_token" not in self.token: | ||
log.debug("No new refresh token given. Re-using old.") | ||
self.token["refresh_token"] = refresh_token | ||
|
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:
See also https://docs.python.org/3/library/datetime.html#datetime.datetime.utcnow
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 thetzinfo
but no, there is notzinfo
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 totimezone.utc
making the timestamp unambiguous.Or you can forget all this, leave the code as is, hope it works with the implicit timezones.