chore(refactor): python-jose is removed from project.optional-dependency. #3549
Conversation
github-actions
bot
added
area/dependencies
Squidtyper
changed the title
|
Thanks @Squidtyper, this is a welcome change! |
|
Yes. We are eliminating the use of python-jose in our own code and we think litestar is better off with pyjwt as well. It's my first time contributing. I see there are some errors. |
Alc-Alc
changed the title
|
before merging this, I would like to evaluate this library instead of |
joserfc and the authlib project as a whole are certainly interesting, but it seems to me that pyjwt is the better option right now. We can start with Snyk Advisor scores of 79 for joserfc vs 91 for pyjwt which also shows more consistent commit activity for the last 2 years for pyjwt. Pyjwt is simply a much more popular package right now, which suggests it's more likely to have some longevity, it has far more contributors, and thus far more eyes on it for future security issues. The authlib project is interesting, in that they've built a small company around the auth code they've built, but looking at the activity graph of their github suggests that they haven't been very active in this for a while, so it may be a dying effort. However, either of these packages would be an improvement over Ref: jpadilla/pyjwt#942 |
I agree. |
Agree with this. So that we don't get bogged down without a fix, I'm inclined to go with this change. It can be revisited should there be a need. |
cofin
removed
do not merge
cofin
force-pushed
the
replace-python-jose-by-pyjwt
branch
from
4e1a021 to
e76d3fe
Compare
github-actions
bot
added
area/private-api
github-actions
bot
added
the
area/docs
|
im having trouble getting docs build to work |
provinzkraut
force-pushed
the
replace-python-jose-by-pyjwt
branch
2 times, most recently
from
1be42d5 to
2084b75
Compare
github-actions
bot
added
size: small
and removed
area/docs
There was a problem hiding this comment.
@Squidtyper Sorry for the long turnaround time on this. After the discussion we had about this, I agree with @cofin that this is okay to go in.
I've left two comments to address, once that's done, this can be merged.
| def base64url_decode(code: str) -> bytes: | ||
| padding = "=" * (4 - (len(code) % 4)) | ||
| return base64.urlsafe_b64decode(code + padding) |
There was a problem hiding this comment.
Why are we doing custom b64 decoding here?
There was a problem hiding this comment.
This also looks like functionality from: https://github.com/mpdavis/python-jose/blob/master/jose/utils.py#L65
| if isinstance(secret, str): | ||
| converted_secret: AllowedPublicKeys | str | bytes = secret | ||
| else: | ||
| if secret["kty"] == "RSA": | ||
| n = int.from_bytes(base64url_decode(secret["n"]), byteorder="big") | ||
| e = int.from_bytes(base64url_decode(secret["e"]), byteorder="big") | ||
| converted_secret = rsa.RSAPublicNumbers(e, n).public_key(default_backend()) | ||
| elif secret["kty"] == "EC": | ||
| x = int.from_bytes(base64url_decode(secret["x"]), byteorder="big") | ||
| y = int.from_bytes(base64url_decode(secret["y"]), byteorder="big") | ||
| converted_secret = ec.EllipticCurvePublicNumbers(x, y, ec.SECP256R1()).public_key(default_backend()) | ||
| elif secret["kty"] == "OKP" and secret["crv"] == "Ed25519": | ||
| x = base64url_decode(secret["x"]) | ||
| converted_secret = ed25519.Ed25519PublicKey.from_public_bytes(x) | ||
| elif secret["kty"] == "OKP" and secret["crv"] == "Ed448": | ||
| x = base64url_decode(secret["x"]) | ||
| converted_secret = ed448.Ed448PublicKey.from_public_bytes(x) | ||
| else: | ||
| raise TypeError("The secret is not a form of allowed public key.") |
There was a problem hiding this comment.
These changes seem to be unrelated to the switch to pyjwt. If this is a feature you want to add, this should be done in a separate PR.
There was a problem hiding this comment.
@provinzkraut Some details from the author would be good, but I do believe this is park of the JWK/backend implementations here: https://github.com/mpdavis/python-jose/tree/master/jose
@JacobCoffee There were some accidental commits in here. I've cleaned the branch and now everything's good. There seems to be however an issue with the |
in security, the tokens' encode and decode now uses pyjwt.
provinzkraut
force-pushed
the
replace-python-jose-by-pyjwt
branch
from
2084b75 to
fa8f184
Compare
|
Closing this in favour of #3684, so we can get a release out. @Squidtyper If you want to complete the work on the features you've started here, feel free to open another PR for this! :) |
|
Thank you a lot! |
in security, the tokens' encode and decode now uses pyjwt.
Description
Closes