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

fix!: update code and tests for pyjwt>=2.0.0 #560

Merged
merged 19 commits into from
Sep 22, 2021
Merged

Conversation

karls
Copy link
Contributor

@karls karls commented Feb 4, 2021

Fixes #556

This PR upgrades pyjwt to 2.0.1, which is a major version upgrade and a breaking change. Namely, pyjwt 2.x drops support for Python 2.x and Python 3.0-3.5. It also changes some user-facing APIs that require changes to twilio-python.

I've made a start on updating the code and fixing broken tests. It would be great to get feedback on the changes in twilio/jwt/__init__.py:143-144. Since the algorithms kwarg is now a required argument for jwt.decode(...) and this library uses both HS256 and RS256, the approach I've taken is to try and get the appropriate algorithm from the particular token's header and fall back to HS256 if it's missing. The alternative approach would be to explicitly pass in algorithms=["HS256", "RS256"] without trying to get the algorithm from the header.

That's as far as the "easy" changes go. The harder decision is dropping support for older Python versions and what kind of changes that requires. Happy to help, but I'd need guidance.

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

requirements.txt Outdated Show resolved Hide resolved
@wavenator
Copy link

If you want to merge this PR you will have to remove Python 3.5 < support. PyJWT does not support Python versions below 3.6.
You will have to edit the Readme.md and .travis.yml. Try finding more occurrences of lower Python versions.

@karls
Copy link
Contributor Author

karls commented Feb 17, 2021

Hey folks. I've gone through and removed references to Python 2.7, updated the build matrix, updated callsites using six for backwards compat and removed some compat code. It would be great to get some extra eyes on this and feedback — I'm not an expert in removing support for old Python versions. :-)

@wavenator
Copy link

Maybe removing the Python version badges from the Readme.md?

@karls
Copy link
Contributor Author

karls commented Feb 18, 2021

Maybe removing the Python version badges from the Readme.md?

I think that's generated by shields.io by looking at the supported Python versions as given in PyPI. Once this PR is merged and a new version is pushed to PyPI that supports Python 3.6+, I think that badge will auto-update itself.

Or did you mean just remove it completely?

@wavenator
Copy link

You got it right :)

@thinkingserious
Copy link
Contributor

Thank you @karls for the PR! And to @wavenator and @amloren1 for your feedback!

Currently, we are still supporting 3.5 and do not support 3.9. That said, both of those items are on our backlog. However, we do not have those items committed to a specific timeframe just yet. I've added a note internally to make sure we revisit this PR once we commit to dropping support for 3.5.

That said, additional comments and +1s on this PR will help move up the priority.

With best regards,

Elmer

@thinkingserious thinkingserious added status: code review request requesting a community code review or review from Twilio type: community enhancement feature request not on Twilio's roadmap labels Feb 18, 2021
@karls
Copy link
Contributor Author

karls commented Feb 20, 2021

Likewise, thank you for the update @thinkingserious and for feedback @wavenator and @amloren1.

Quite a few people seem to be blocked on this update — it would be great to get at least a rough idea, even a ballpark, of how long the wait for the official release would be.

FWIW, I've started using my fork in production (without issues so far), pinned to f155dbe. I only use a very small subset of the surface area of the library, though — just the AccessToken API and programmable video (rooms, media, etc). I think if there are other brave people willing to test this in production and provide feedback, it'll help with getting this PR and the new release in proper shape in time for the official release.

What do you think @thinkingserious?

@thinkingserious
Copy link
Contributor

thinkingserious commented Feb 23, 2021

Hello @karls, @amloren1,

Could I please have more context as to why this issue is a blocker with respect to the pyjwt upgrade? I'm trying to make sure I understand the consequence of dropping support of version 1.7.1, besides the necessity to continue supporting EOL versions of Python. Thank you!

Unfortunately, I don't have a ballpark to share just yet on the official deprecation timeline.

Internal tracking ID: DI-1105

@karls
Copy link
Contributor Author

karls commented Feb 24, 2021

People who need to or want to upgrade to PyJWT >= 2.0 are unable to do so because twilio-python versions 6.51.0 and newer pin PyJWT to 1.7.1, which the dependency resolver is unable to reconcile. And twilio-python versions earlier than 6.51.0 do not specify PyJWT version, which means that you can install PyJWT >= 2.0, but twilio-python will break if your codebase uses AccessTokens (or other classes from twilio.jwt.* that depend on encoding and decoding JWTs).

Does that help @thinkingserious?

@eshanholtz
Copy link
Contributor

@karls Thank you for the additional context and we appreciate the work you're doing to create a fork that unblocks folks! It looks like we can add support for 3.9 now without any issues, so I went ahead and approved and merged @tim-schilling's #563 PR.

With regards to the PyJWT >= 2.0 upgrade, since removing the hard version requirement breaks the existing AccessToken functionality, and upgrading the version would require us to drop currently supported Python versions, we're looking at a 1 month+ minimum timeline for rolling to a new major version of twilio-python, which would include these changes. This PR and the concerns raised in this thread have definitely accelerated our timeline for this, so please be patient and rest assured that we hear you and are working on this :)

For those of you in the community who are just finding this thread, please note that we use +1's on the original PR description to help us gauge community demand. Thank you all for your feedback and contributions!

@karls
Copy link
Contributor Author

karls commented Feb 26, 2021

Thank you for the update @eshanholtz. And I'm glad this issue is making its way higher up in the priority list. :-)

I'm going to deploy our app using karls@872f52e today in order to keep making sure things are working. We're on Python 3.8.8 FWIW, no issues so far at least. It would be awesome if other folks would be willing to temporarily test my fork in staging or even in production.

@christianbundy
Copy link

That said, additional comments and +1s on this PR will help move up the priority.

+1, we're also blocked from upgrading twilio-python because we use pyjwt 2.x.x. Thanks for moving this forward!

@hugorodgerbrown
Copy link

Thank you @karls for this PR - hopefully it gets through the approval process soon enough.

@hugorodgerbrown
Copy link

It would be awesome if other folks would be willing to temporarily test my fork in staging or even in production.

@karls we have forked your fork (we only permit pinning to our own repos), and have installed the latest commit on that (yunojuno@4889644). We've unpinned our other packages that depend on PyJWT (django-request-token and social-auth-core) so they can upgrade. I'll report back on impact.

NB we only use this lib for SMS (i.e. no JWT) so not a full stress test of the changes.

@karls
Copy link
Contributor Author

karls commented Mar 30, 2021

Hey @eshanholtz — just wanted to ping you to see how this has been progressing internally. Any exciting news?

@hugorodgerbrown
Copy link

Giving this a bump just in case anyone from Twilio is watching. We're running @karls's fork in production now, and it's working.

@bharling
Copy link

+1 Also running the fork in production and it's working well, would really like to see this in mainline soon if we can!

twilio/jwt/__init__.py Outdated Show resolved Hide resolved
@karls
Copy link
Contributor Author

karls commented Apr 17, 2021

A quick heads-up to people using my fork, I've updated the code as per @andersk's comments and I've added an explanation of the changes here: #560 (comment).

We're also running my fork with the latest code in production without issues — using only AccessToken and programmable video, though.

@rohanahata rohanahata mentioned this pull request Apr 19, 2021
@thinkingserious
Copy link
Contributor

Internal tracking ID: DI-1327

@karls
Copy link
Contributor Author

karls commented May 8, 2021

Any updates on timelines/progress internally?

@shwetha-manvinkurke
Copy link
Contributor

@karls it has moved up the backlog and we will update you on the timeline when we have that defined.

Copy link
Contributor

@JenniferMah JenniferMah left a comment

Choose a reason for hiding this comment

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

Hi @karls thanks for your patience! This feature continues to move up in our backlog as our team prepares to drop support for python 2.7, 3.4, and 3.5. We should have an update on the estimated timeline within the next week.

@JenniferMah JenniferMah changed the title fix!: update code and tests for pyjwt>=2.0.0 [DO NOT MERGE] fix!: update code and tests for pyjwt>=2.0.0 Jul 20, 2021
@JenniferMah
Copy link
Contributor

Hi Folks!
Thank you for your patience on this. On or after September 22 2021, Twilio will release version 7.0.0 of the Twilio Python helper library. This version will officially drop support for Python versions 2.7, 3.4, and 3.5. Twilio Python version 7.0.0 will also include the upgrade to PyJWT 2.0.

@eshanholtz eshanholtz changed the title [DO NOT MERGE] fix!: update code and tests for pyjwt>=2.0.0 fix!: update code and tests for pyjwt>=2.0.0 Sep 22, 2021
@eshanholtz eshanholtz merged commit e2df36a into twilio:main Sep 22, 2021
@Andrew-Chen-Wang
Copy link

Andrew-Chen-Wang commented Jan 19, 2024

Is it possible to bring back 1.7.1 compatibility such that we have both 1.7.1 and 2.0.0? Happy to open a PR if we can get it in a patch update soon. The python ecosystem, especially older packages, have been slow to update to 2+ PyJWT. My firm needs 1.7.1 compatibility, and it's relatively simple to add. Would be great if it'd be possible to patch and release a new version soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: code review request requesting a community code review or review from Twilio type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to PyJWT 2.0