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

Remove verify from jwt.decode() to follow PyJWT v2.2.0. #472

Merged
merged 5 commits into from
Oct 12, 2021

Conversation

dajiaji
Copy link
Contributor

@dajiaji dajiaji commented Oct 9, 2021

According to the setup.py, simplejwt supports PyJWT v2.0.0 or later.

In this case, you can simply remove meaningless verify argument from jwt.decode() to support PyJWT v2.2.0.

Closes #467

@Andrew-Chen-Wang
Copy link
Member

Seems to not work either way

@dajiaji
Copy link
Contributor Author

dajiaji commented Oct 11, 2021

@Andrew-Chen-Wang Sorry for bothering you.

Since this was very easy and small fix, I skipped checking the tests.

In the latest PyJWT (v2.2.0), PyJWT.decode() cannot be overwritten with PyJWS.decode() because PyJWT.decode() does not have **kwargs. In the first place, PyJWT.decode() and PyJWS.decode() exist in different layers and play different roles, so the assumption that they can be overwritten is wrong. For now, I modified a test (test_decode_when_algorithm_not_available) to pass with the latest PyJWT.

In my development environment, all of the tests succeed without any problem.
I'm glad if you can check and merge it.

Copy link
Member

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

Thanks for this! No bother. I'm in class rn, but do you mind checking PyJWK has the same args? I'm aware PyJWT got rid of the **kwargs, for several things, so a double check that aud, leeway, and iss, can be passed to PyJWK is ok. IIRC, if the token is PyJWK, then those args are just missing.

@dajiaji
Copy link
Contributor Author

dajiaji commented Oct 11, 2021

do you mind checking PyJWK has the same args?

PyJWS does not have the same arguments. aud, leeway, and iss are used only in PyJWT and PyJWS does nothing for the aud, leeway and iss ( because aud, iss and exp/nbf/iat(leeway) are "JWT" claims). So you don't need to care about that.

@auvipy
Copy link
Contributor

auvipy commented Oct 11, 2021

do you mind checking PyJWK has the same args?

PyJWS does not have the same arguments. aud, leeway, and iss are used only in PyJWT and PyJWS does nothing for the aud, leeway and iss ( because aud, iss and exp/nbf/iat(leeway) are "JWT" claims). So you don't need to care about that.

I shouldn;t have convinced by your breaking change. that should have done in v3.0.

@Andrew-Chen-Wang
Copy link
Member

@dajiaji since we're passing all those args in token.decode, I worry that Python will throw an error saying too many arguments.

@dajiaji
Copy link
Contributor Author

dajiaji commented Oct 11, 2021

@dajiaji since we're passing all those args in token.decode, I worry that Python will throw an error saying too many arguments.

There is no need to worry. This simplejwt does not call PyJW"S" decode() directly except for the test that I modified and PyJW"S".decode() is called only in PyJW"T".decode() with correct arguments. No unexpected arguments will be passed to the PyJW"S".decode().

@auvipy I think it is better to revert the breaking change once (and release v2.2.1) too. But anyway, this PR is useful for the future v3.0.0 release.

@auvipy
Copy link
Contributor

auvipy commented Oct 12, 2021

@dajiaji since we're passing all those args in token.decode, I worry that Python will throw an error saying too many arguments.

There is no need to worry. This simplejwt does not call PyJW"S" decode() directly except for the test that I modified and PyJW"S".decode() is called only in PyJW"T".decode() with correct arguments. No unexpected arguments will be passed to the PyJW"S".decode().

@auvipy I think it is better to revert the breaking change once (and release v2.2.1) too. But anyway, this PR is useful for the future v3.0.0 release.

as already reverted

Copy link
Member

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

Cool thanks so much @dajiaji !

I agree, the PR will at least prevent any more breaking features for 3.0.0 upgrade.

@Andrew-Chen-Wang Andrew-Chen-Wang merged commit 43d5b2f into jazzband:master Oct 12, 2021
@felippem
Copy link
Contributor

felippem commented Oct 13, 2021

@Andrew-Chen-Wang will you release a version of this PR?

@Andrew-Chen-Wang
Copy link
Member

@felippem yes. If possible, can you create a PR for the CHANGELOG (I can't approve my own PRs). Try to match the style; if you can't, it's alright since I can fix it by editing your branch; I just need a PR.

felippem added a commit to felippem/djangorestframework-simplejwt that referenced this pull request Oct 13, 2021
@felippem
Copy link
Contributor

@felippem yes. If possible, can you create a PR for the CHANGELOG (I can't approve my own PRs). Try to match the style; if you can't, it's alright since I can fix it by editing your branch; I just need a PR.

@Andrew-Chen-Wang Done: #476

felippem added a commit to felippem/djangorestframework-simplejwt that referenced this pull request Oct 13, 2021
BotondD added a commit to tvbuddy-net/djangorestframework-simplejwt that referenced this pull request Nov 12, 2021
* Remove verify from jwt.decode() to follow PyJWT v2.2.0. (jazzband#472)

* Fix test not to overwrite PyJWT.decode with PyJWS.decode.

Co-authored-by: Andrew Chen Wang <60190294+Andrew-Chen-Wang@users.noreply.github.com>

* Add support to python 3.10

Co-authored-by: Ajitomi Daisuke <dajiaji@gmail.com>
Co-authored-by: Andrew Chen Wang <60190294+Andrew-Chen-Wang@users.noreply.github.com>
Co-authored-by: Jair Henrique <jair.henrique@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PyJWT==2.2.0
4 participants