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

Use a single JWT library #503

Closed
sevdog opened this issue Sep 9, 2020 · 20 comments · Fixed by #819
Closed

Use a single JWT library #503

sevdog opened this issue Sep 9, 2020 · 20 comments · Fixed by #819

Comments

@sevdog
Copy link
Contributor

sevdog commented Sep 9, 2020

Expected behaviour

Use a single python JWT library.

Actual behaviour

As of now this package relies upon two different JWT python lirbaries:

  • PyJWT, declared in requirements-base file, for the following backends:

    • AppleIdAuth
    • AzureADB2COAuth2
    • AzureADOAuth2
    • AzureADTenantOAuth2
    • ExactTargetOAuth2
    • KeycloakOAuth2
    • MediaWiki
    • MicrosoftOAuth2
  • python-jose, declared in requirements-openidconnect file, for the following backends:

    • Auth0OAuth2
    • ElixirOpenIdConnect (which derives from OpenIdConnectAuth)
    • OpenIdConnectAuth

Related search: https://github.com/python-social-auth/social-core/search?l=Python&q=jwt

Any other comments?

If there are not any particular need for python-jose to be used instead of PyJWT for above listed backends a single JWT implementation should be used as requirements. This will greatly simplify package/requirements management.

Also if there are no need to have two different version of PyJWT (pyjwt>=1.7.1 in requirements-openidconnect.txt and PyJWT>=1.4.0 in requirements-base.txt) a single requirement should be enough..

@stale
Copy link

stale bot commented Nov 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale issues (closing soon) label Nov 8, 2020
@andersk
Copy link
Contributor

andersk commented Nov 8, 2020

Still an issue.

@stale stale bot removed the stale Stale issues (closing soon) label Nov 8, 2020
@trumpet2012
Copy link

This may be worthy of its own issue, but pyjwt recently had a major version release to 2.0.0 (https://github.com/jpadilla/pyjwt/blob/master/CHANGELOG.md#v200) and since the requirements don't specify an upper bound on the version, 2.0.0 gets installed which is incompatible with the code here that use it (I know the apple ID backend breaks).

I'm manually pinning pyjwt in our projects to pyjwt = "==1.7.1" for now.

@andersk
Copy link
Contributor

andersk commented Dec 28, 2020

@trumpet2012 That’s #532.

This was referenced Dec 30, 2020
@shaib
Copy link
Contributor

shaib commented Dec 30, 2020

@trumpet2012 I suspect the change in 4937977 may fix the Apple ID issue. Can you check it?

@trumpet2012
Copy link

@shaib Yep the Apple ID backend works with pyjwt version 2.0.0 after your changes.

@shaib
Copy link
Contributor

shaib commented Dec 31, 2020

@trumpet2012 Thanks, and a happy new year to you too!

nijel added a commit to nijel/social-core that referenced this issue Jan 15, 2021
It is already present in requirements-base.txt.

See python-social-auth#503
nijel added a commit that referenced this issue Jan 15, 2021
It is already present in requirements-base.txt.

See #503
@nijel
Copy link
Member

nijel commented Jan 15, 2021

The problem is that PyJWT is JWT only library while jose includes JWK (and other modules) as well, which is used in OpenIdConnectAuth. I have no knowledge of these, but switching doesn't seem that straightforward in this case. It might be more reasonable to use jose for all things instead...

@stale
Copy link

stale bot commented Mar 16, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale issues (closing soon) label Mar 16, 2021
@andersk
Copy link
Contributor

andersk commented Mar 16, 2021

Still an issue.

@stale stale bot closed this as completed Mar 24, 2021
@andersk
Copy link
Contributor

andersk commented Mar 24, 2021

WTF stalebot?

@nijel
Copy link
Member

nijel commented Mar 24, 2021

@omab Maybe it's time to disable stale bot here? If something like this is still desired, we can use https://github.com/actions/stale/, which works more reliably.

@nijel nijel reopened this Mar 24, 2021
@stale stale bot removed the stale Stale issues (closing soon) label Mar 24, 2021
@stale
Copy link

stale bot commented May 23, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale issues (closing soon) label May 23, 2021
@andersk
Copy link
Contributor

andersk commented May 23, 2021

Still an issue.

@stale stale bot removed the stale Stale issues (closing soon) label May 23, 2021
@sevdog
Copy link
Contributor Author

sevdog commented Jul 22, 2021

PyJWT from 2.0 has added JWK and support was improved in 2.1.

It should be checked if their implementation could replace jose.

@stale
Copy link

stale bot commented Sep 21, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale issues (closing soon) label Sep 21, 2021
@andersk
Copy link
Contributor

andersk commented Sep 21, 2021

There have been no releases since the last three times stalebot marked this “stale”. This isn’t helping anyone. Please consider turning stale-bot off.

@digismack digismack removed the stale Stale issues (closing soon) label Sep 21, 2021
@sevdog
Copy link
Contributor Author

sevdog commented Jan 17, 2022

I was trying to write a PR to handle this by using only pyjwt.

So far the main compatibility issue is jpadilla/pyjwt#314, since jose has builtin-support for this JWT extension required by OIDC but pyjwt no.

If I get some more time I will try to complete this.

@sirosen
Copy link

sirosen commented Jun 29, 2022

A minor update regarding jpadilla/pyjwt#314 which may have bearing on this project:

Those changes will be sufficient to satisfy my use-case, which is validation of the at_hash claim. I may put in a PR with an example of at_hash validation to see if I can get that into the docs.

(I'm not a pyjwt maintainer. Just a friendly citizen of the Internet. 🙂 )

@sevdog
Copy link
Contributor Author

sevdog commented Apr 28, 2023

I tried to re-take this issue and at the moment I have found these issues:

  • jose.jwt.encode and jwt.encode have a couple of subtle differences:
    • the first calls the first argument claims while the latter calls it payload (easy to fix)
    • the first accepts a dict as key while the latter only accepts strings (there is an issue where someone asked to support at least their own JWK instances as key)
    • the first accepts an access_token parameter which under the hood calculates the at_hash claim, the latter require the user to pass the at_hash in the payload (there is an usage help on how to calculate it)
  • jose and jwt have different claim validation, thus the JWTClaimsError of jose can be mapped to a set of different exceptions in jwt (maybe the InvalidTokenError is the better to get all errors?)

At the moment I do not have enough time to address this, if anyone is willing to do so feel free to code.

@sevdog sevdog mentioned this issue Aug 3, 2023
10 tasks
@nijel nijel linked a pull request Sep 12, 2023 that will close this issue
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

7 participants