Skip to content
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

Decode with PyJWK #886

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Decode with PyJWK #886

wants to merge 14 commits into from

Conversation

luhn
Copy link

@luhn luhn commented Apr 28, 2023

This PR contains three proposed changes. You can accept or reject any of them as you see fit. This is just a rough draft, once the functionality is approved I'll clean it up, add tests, and document.

  1. Add algorithm string to PyJWK. This is useful in determine the appropriate algorithms value to pass into decode().

  2. Allow a PyJWK to be passed directly into decode(), so it's not necessary to pull PyJWK.key. (This would fix Should jwt.decode accept PyJWK keys? #864)

  3. If a PyJWK is passed into decode() and algorithms is not set, use the algorithm from the JWK. This change makes the API more convenient and reduces room for error: There's no reason that you should use any algorithm but the JWK's algorithm and doing otherwise is problematic at best and a possible security threat at worst.

jwt/api_jwk.py Outdated
@@ -52,6 +52,7 @@ def __init__(self, jwk_data: JWKDict, algorithm: str | None = None) -> None:
if not has_crypto and algorithm in requires_cryptography:
raise PyJWKError(f"{algorithm} requires 'cryptography' to be installed.")

self.algorithm = algorithm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo having both self.algorithm and self.Algorithm may be too similar

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understandable. Any suggestions? self.algorithm_name? self.alg? (To match the JWK field)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.algorithm_name seems alright, actually I think self.Algorithm is the one being wrong, and self.algorithm_class or something similar might be better. Anyway better not breaking any public attribute

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm +1 on @Viicos's suggestion.

@luhn
Copy link
Author

luhn commented Jun 15, 2023

Can I get feedback from a maintainer on whether or not these proposed changes would be accepted once properly documented and tested? @Viicos? @jpadilla?

@Viicos
Copy link
Contributor

Viicos commented Jun 15, 2023

I like the idea, I'm not an official maintainer so I'll let @jpadilla or @auvipy decide here :)

@auvipy auvipy self-requested a review June 17, 2023 03:51
@github-actions github-actions bot added the stale Issues without activity for more than 60 days label Aug 17, 2023
@github-actions github-actions bot closed this Aug 24, 2023
@auvipy auvipy reopened this Aug 24, 2023
@github-actions github-actions bot removed the stale Issues without activity for more than 60 days label Aug 25, 2023
@luhn luhn marked this pull request as ready for review October 16, 2023 05:24
@luhn
Copy link
Author

luhn commented Oct 16, 2023

Ready for review 👍

@auvipy
Copy link
Collaborator

auvipy commented Oct 16, 2023

restarted the CI, lets hope all are green

Copy link
Collaborator

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

      if not has_crypto and algorithm in requires_cryptography:
      raise PyJWKError(f"{algorithm} requires 'cryptography' to be installed.")

E jwt.exceptions.PyJWKError: ES256 requires 'cryptography' to be installed.

jwt/api_jwk.py:53: PyJWKError
=========================== short test summary info ============================
SKIPPED [1] tests/test_advisory.py:27: Requires cryptography library installed

@luhn
Copy link
Author

luhn commented Oct 16, 2023

Okay, I fixed tests w/o cryptography installed.

@github-actions github-actions bot added the stale Issues without activity for more than 60 days label Dec 16, 2023
@luhn
Copy link
Author

luhn commented Dec 20, 2023

Ping. Any more feedback on this?

@auvipy, it looks like you need to approve my changes.

@github-actions github-actions bot removed the stale Issues without activity for more than 60 days label Dec 21, 2023
@github-actions github-actions bot added the stale Issues without activity for more than 60 days label Feb 20, 2024
@github-actions github-actions bot closed this Feb 28, 2024
@luhn
Copy link
Author

luhn commented Feb 28, 2024

Bummed to see this was closed as stale. @jpadilla @Viicos @auvipy would it be possible to reopen and get merged? I'm happy to make any changes necessary.

@jpadilla jpadilla reopened this Feb 28, 2024
@jpadilla jpadilla removed the stale Issues without activity for more than 60 days label Feb 28, 2024
@jpadilla jpadilla added the keep label Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should jwt.decode accept PyJWK keys?
4 participants