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

Fixes #1193: deprecated JWT decode. #1194

Closed
wants to merge 4 commits into from

Conversation

jensens
Copy link
Member

@jensens jensens commented Aug 12, 2021

  • verify as kwarg of decode was already deprecated in PyJWT and was removed in 2.0. Uses now options style to pass this on.
  • encode returns a str (Py3!) which can not be decoded again.
  • Update to use and require PyJWT 2.1.0.

Note:
After merge, buildout.coredev needs to be updated.
I prepared an separate PR for this: plone/buildout.coredev#735
This is difficult to test with our current Jenkins setup.

@jensens jensens closed this Aug 12, 2021
@jensens jensens force-pushed the fix-and-update-deprecated-jwt branch from 376db45 to 34dc327 Compare August 12, 2021 21:30
@jensens jensens reopened this Aug 12, 2021
@mister-roboto
Copy link

@jensens thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@jensens jensens force-pushed the fix-and-update-deprecated-jwt branch from 5e1a135 to 4d044a4 Compare August 12, 2021 21:34
@plone plone deleted a comment from mister-roboto Aug 12, 2021
@jensens jensens force-pushed the fix-and-update-deprecated-jwt branch from 4d044a4 to 9a2ccf0 Compare August 12, 2021 21:43
@jensens jensens marked this pull request as ready for review August 12, 2021 22:17
@jensens jensens requested review from ericof and tisto August 12, 2021 22:18

# recent pyjwt
pyjwt = 2.1.0
Copy link
Member

Choose a reason for hiding this comment

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

I would put a comment here to remove this pin after release Plone version with coredev PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is needed for Plone 5.2 - which is still supported with Py3 here at master.

Copy link
Member

Choose a reason for hiding this comment

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

You won't update the version in the 5.2 branch of coredev? At least for Python 3?

Copy link
Member Author

@jensens jensens Aug 12, 2021

Choose a reason for hiding this comment

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

Well, with conditional version sections this would be technically possible.

Since the 8.x series is not part of the release of Plone 5.2 (see https://github.com/plone/buildout.coredev/blob/5.2/versions.cfg#L197 and https://github.com/plone/buildout.coredev/blob/5.2/sources.cfg#L118, it uses the 7.x) I do not think this is possible.

We could add a note to the README which versions/ preconditions are needed for use of the 8 series in Plone 5.2.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, it's weird to say that version 8.x is compatible with Plone 5.2 and it will not be tested on coredev. If any unreleased changes in any package, break the version 8.x, we won't know about it until the Plone version is released.

Copy link
Member

Choose a reason for hiding this comment

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

@jensens @mauritsvanrees I am wondering if we should defer this to the next major release of plone.restapi (9.x). Once plone.volto has seen a first release, we will need to move some things around (e.g. the block behavior needs to move from restapi to plone.volto). The plan is to have stuff ready for the conference, so it won't take so long. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like the new code from this PR also works with the old PyJWT. At least the tests pass.
So I think we can include this code even in 7.x.x. Just don't touch setup.py.
On PY3, token.decode("utf-8") should still be done, but only if the token is bytes, which PyJWT 1.7.1 gives us.

So that is one option: be a bit smarter in the code so we can support both versions of PyJWT.

Other option is to only merge the current PR when plone.restapi master only targets PY 3, which seems to be what Timo is saying.

I don't really mind either way.

Copy link
Member

Choose a reason for hiding this comment

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

@jensens is there a way we can support both PyJWT versions as @mauritsvanrees suggests?

Copy link
Member

Choose a reason for hiding this comment

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

This would still be good to have, fixing a minor pain point and getting rid of an old dependency version.
But this seems like the kind of change we could do in a Plone 6.0 beta.

Copy link
Member Author

Choose a reason for hiding this comment

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

I rebased the branch.

@tisto
Copy link
Member

tisto commented Aug 18, 2021

@jenkins-plone-org please run jobs

2 similar comments
@jensens
Copy link
Member Author

jensens commented Aug 20, 2021

@jenkins-plone-org please run jobs

@jensens
Copy link
Member Author

jensens commented Aug 30, 2021

@jenkins-plone-org please run jobs

@jensens jensens force-pushed the fix-and-update-deprecated-jwt branch 2 times, most recently from 1855487 to 094db97 Compare August 30, 2021 19:01
@jensens
Copy link
Member Author

jensens commented Aug 30, 2021

@jenkins-plone-org please run jobs

@jensens
Copy link
Member Author

jensens commented Aug 30, 2021

Jenkins will always fail here. There no easy way to test a branch on buildout.coredev together with branch on a package. Except on an own branch of the branch of buildout.coredev - I did this here plone/buildout.coredev#737

@tisto tisto modified the milestones: 9.x.x, Plone 6 Sep 25, 2021
@jensens jensens force-pushed the fix-and-update-deprecated-jwt branch from 094db97 to 968d314 Compare November 14, 2021 22:50
@jensens
Copy link
Member Author

jensens commented Nov 14, 2021

Except on an own branch of the branch of buildout.coredev

Nope, won't work as well.

jensens added 2 commits March 23, 2022 09:44
- Document usage of version 8 with Plone 5.2.
- Bumb version on feature level.
@jensens jensens force-pushed the fix-and-update-deprecated-jwt branch from cc0c76e to 4b0d5ac Compare March 23, 2022 08:44
@jensens
Copy link
Member Author

jensens commented Mar 23, 2022

Summary:

  • all done
  • this works only with newer PyJWT
    • without Python 2 support
    • but then also in Plone 5.2
  • tests are failing as unless buildout.coredev PyJWT pin is upgraded, it pins the old version.

@mauritsvanrees
Copy link
Member

For me, it would be fine to let plone.restapi master use latest pyjwt, as the branch is already for Python 3 only. New minor version. Maybe new major version, but for me it would not be needed.

You changed the versions.cfg here to pin pyjwt = 2.1.0, but this file is currently not used. Maybe base.cfg should extend it. @tisto may know how this is intended.

"Framework :: Plone :: Core",
"Intended Audience :: Developers",
"Operating System :: OS Independent",
"Programming Language :: Python",
"Programming Language :: Python :: 3.6",
"Programming Language :: Python :: 3.7",
"Programming Language :: Python :: 3.8",
"Programming Language :: Python :: 3.9",
Copy link
Member

Choose a reason for hiding this comment

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

@jensens Python 3.9 is not officially supported or tested with plone.restapi yet: https://github.com/plone/plone.restapi/blob/master/.github/workflows/tests.yml#L9

Copy link
Member

Choose a reason for hiding this comment

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

It is tested in coredev.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, Python 3.9 must work for Plone 6, so running tests with it here is important. I added issue #1345 to not forget about it.

@mauritsvanrees
Copy link
Member

Green now.
(Except plone-coredev 6, which can only pass when we update the PyJWT pin there, which we can only do after we merge this PR. Chicken versus egg.)

@mauritsvanrees
Copy link
Member

I am tired of waiting on someone to either approve and merge this, or say what needs to change.
So I have made a new PR #1377. It takes the main change of the current PR, but makes it conditional: the code works on both PyJWT version 1 and 2. That change should be easier to make a decision on and merge soon, instead of being in limbo since August.
The cost is an extra dependency (importlib-metadata) on Python 3.7 and lower, but this is likely already pulled in by other packages, and it is already pinned by Plone 5.2 and 6.0. (I could have used pkg_resources, but that is apparently deprecated for this use case.)

mister-roboto pushed a commit to plone/buildout.coredev that referenced this pull request Apr 22, 2022
Branch: refs/heads/master
Date: 2022-04-19T17:46:53+02:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.restapi@8a3c019

Make the PAS plugin compatible with PyJWT 1 and 2.

Takes code from plone/plone.restapi#1194
Fixes plone/plone.restapi#1193

Files changed:
A news/1193.bugfix
M setup.py
M src/plone/restapi/pas/plugin.py
Repository: plone.restapi

Branch: refs/heads/master
Date: 2022-04-22T11:34:51+02:00
Author: Jens W. Klein (jensens) <jk@kleinundpartner.at>
Commit: plone/plone.restapi@30ec4d8

Merge pull request #1377 from plone/maurits-support-pyjwt-1-and-2

Make the PAS plugin compatible with PyJWT 1 and 2.

Files changed:
A news/1193.bugfix
M setup.py
M src/plone/restapi/pas/plugin.py
@jensens
Copy link
Member Author

jensens commented Apr 22, 2022

solved with bbb #1377

@jensens jensens closed this Apr 22, 2022
@jensens jensens deleted the fix-and-update-deprecated-jwt branch April 22, 2022 09:35
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.

5 participants