-
Notifications
You must be signed in to change notification settings - Fork 40
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
Compatibility Fixes for PyJWT/Cryptography Versions #222
Conversation
@@ -277,8 +277,15 @@ def verify_token(self, token, id_name, token_use): | |||
# Compute and verify at_hash (formerly done by python-jose) | |||
if "at_hash" in verified: | |||
alg_obj = jwt.get_algorithm_by_name(header["alg"]) | |||
digest = alg_obj.compute_hash_digest(self.access_token) | |||
at_hash = base64.urlsafe_b64encode(digest[: (len(digest) // 2)]).rstrip("=") | |||
try: |
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.
Can you provide more details about what this is supposed to fix?
The linked issue (#223) points more to how the access_token
provided for the Cognito class changed.
The current code follows what the JWT library suggests for this:
https://github.com/jpadilla/pyjwt/blob/master/docs/usage.rst#oidc-login-flow (at the bottom)
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.
This is supposed to deal with functions expecting strings vs bytes differently with updated versions of the cryptography dependency.
The try-catch block allows the digest to be computed in either case and then the at_hash step is split based on the type returned by the digest computation since this changes depending on so that the strip step does not fail.
The idea was not to change function but to make it robust to some typing mismatches that were occurring. That said, I am not experiencing the issue now with python 3.11.9 and the updated dependencies.
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.
Okey, that is why I asked, beacuse after looking at different versions, there is no differences between them in what they return or expect as inputs.
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.
Going to close this one for now, as it no longer seems to be needed.
Thanks anyway 👍
Note the issue still seems to be active, and this is using
See below the trace
Minimal amount of code to reproduce the issue
|
I just hit this issue too. |
Added compatibility fixes for different byte vs str inputs and outputs. Prevents TypeErrors seen with: