[25.0] Check for expiration in refresh token dictionary #20954
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #20855
We check for refresh token's expiration in
user_authnz_token.extra_data
, which must have worked for other providers. For tapis, however, this does not work because tapis stores the following data structure in theextra_data
field:As a result, this warning is logged
No
expiresor
expires_inkey found in token extra data, cannot refresh
on each web transaction.@nuwang: w.r.t. your comment, I don't think we should fix this in the tapis implementation: we have only one
extra_data
field, and it's already storing other data including the mapping that contains these keys. Even if we overrideset_extra_data
, I don't think we should store refresh token expiration data alongside the access token data (by flattening the data). Let me know if you had a different approach in mind.This solution also fixes a bug: in the current code, if the
expires
orexpires_at
key exists in theextra_data
dictionary, but the value isNone
, this line will cause a TypeError.This has been tested on cfde-galaxy-dev.
NOTE: Due to this bug, token refreshing functionality was broken. Fixing it will result in, potentially, spamming tapis with token refresh attempts - I don't know whether that will cause a problem; and if it does, we should disable token refreshing until we have a better solution (see related discussion: #20821)
(ping @natefoo)
How to test the changes?
(Select all options that apply)
License