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

chore(refactor): python-jose is removed from project.optional-dependency. #3549

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Squidtyper
Copy link

in security, the tokens' encode and decode now uses pyjwt.

Description

Closes

in security, the tokens' encode and decode now uses pyjwt.
@Squidtyper Squidtyper requested review from a team as code owners June 6, 2024 12:24
@github-actions github-actions bot added area/dependencies This PR involves changes to the dependencies size: small pr/external Triage Required 🏥 This requires triage labels Jun 6, 2024
@Squidtyper Squidtyper changed the title python-jose is removed from project.optional-dependency. refractor: python-jose is removed from project.optional-dependency. Jun 6, 2024
@provinzkraut
Copy link
Member

Thanks @Squidtyper, this is a welcome change!
Am I correct to assume this is motivated by the maintenance status of python-jose?

@Squidtyper
Copy link
Author

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 Alc-Alc changed the title refractor: python-jose is removed from project.optional-dependency. chore(refactor): python-jose is removed from project.optional-dependency. Jun 6, 2024
pyproject.toml Outdated Show resolved Hide resolved
@cofin
Copy link
Member

cofin commented Jun 6, 2024

before merging this, I would like to evaluate this library instead of pyjwt. Both python-jose and pyjwt have gone through periods of inactivity in their history.

@cofin cofin added the do not merge Don't merge this label Jun 6, 2024
@sherbang
Copy link
Contributor

before merging this, I would like to evaluate this library instead of pyjwt. Both python-jose and pyjwt have gone through periods of inactivity in their history.

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 python-jose, which has unresolved security issues. I suggest that the priority now should be removing python-jose, and not delaying over considering which replacement library is marginally better.

Ref: jpadilla/pyjwt#942

@provinzkraut
Copy link
Member

joserfc and the authlib project as a whole are certainly interesting, but it seems to me that pyjwt is the better option right now.

I agree. pyjwt is the de-facto standard and does everything we need here. It being the most popular JWT library also has the benefit that if users are doing something else with JWTs outside what Litestar provides, it's most likely this is the library they'll be using.

@cofin
Copy link
Member

cofin commented Jun 21, 2024

However, either of these packages would be an improvement over python-jose, which has unresolved security issues. I suggest that the priority now should be removing python-jose, and not delaying over considering which replacement library is marginally better.

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 cofin removed do not merge Don't merge this Triage Required 🏥 This requires triage labels Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies This PR involves changes to the dependencies pr/external pr/internal size: small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants