Add support for pyjwt leeway
#790
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sirosen
requested review from
aaschaer,
ada-globus,
derek-globus and
kurtmckee
as code owners
On the surface, this may look like a potentially incompatible change because we remove `leeway` from the passed `jwt_params`. However, those are passed to `options` and `leeway` isn't a supported value there for pyjwt, so the change is in effect strictly additive. pyjwt source has a comment indicating that `leeway` might be added to `options` in the future (it would make sense), along with values we control like `audience`. For the time being, however, this makes sense as a mechanism for passing `leeway` for JWT handling in the SDK. Because the same `leeway` is used for the `iat`, `nbf`, and `exp` claims, we can check that `leeway` is passed correctly by using it to make a very old `exp` claim pass validation in our tests. A new default is set for `leeway` of 0.5s internally. This is not part of the `decode_id_token` docs -- kept as an implementation detail -- but it makes the default behavior slightly more tolerant of clock drift. As such, this part of the change is documented as a fix in the changelog, whereas the rest is an addition.
sirosen
force-pushed
the
support-jwt-leeway
branch
from
bed4cd4
to
951020f
Compare
ada-globus
approved these changes
Jul 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
See also: globus/globus-cli#820
On the surface, this may look like a potentially incompatible change because we remove
leewayfrom the passedjwt_params. However, those are passed tooptionsandleewayisn't a supported value there for pyjwt, so the change is in effect strictly additive.pyjwt source has a comment indicating that
leewaymight be added tooptionsin the future (it would make sense), along with values we control likeaudience. For the time being, however, this makes sense as a mechanism for passingleewayfor JWT handling in the SDK.Because the same
leewayis used for theiat,nbf, andexpclaims, we can check thatleewayis passed correctly by using it to make a very oldexpclaim pass validation in our tests.A new default is set for
leewayof 0.5s internally. This is not part of thedecode_id_tokendocs -- kept as an implementation detail -- but it makes the default behavior slightly more tolerant of clock drift. As such, this part of the change is documented as a fix in the changelog, whereas the rest is an addition.📚 Documentation preview 📚: https://globus-sdk-python--790.org.readthedocs.build/en/790/