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 pyjwt for API tokens instead of itsdangerous's deprecated code #6267

Closed
wants to merge 1 commit into from

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Feb 12, 2022

Status

Ready for review

Description of Changes

itsdangerous deprecated TimedJSONWebSignatureSerializer in 2.0 and has
entirely removed it in 2.1, so we need to switch to something else. They
recommended authlib, except that's a pretty large general-purpose
authentication library.

pyjwt is a smallish (1k LOC) library that just provides JWT/JWS
functionality. For the HS512 algorithm, it uses the same implementation
from hashlib in the Python standard library as itsdangerous did.

Tests are provided to verify successful operation as well as different
failure modes, such as invalid tokens, incorrect secret key, wrong
algorithm, and lack of an expiry.

Tokens generated by itsdangerous are not decodable by pyjwt because
itsdangerous passed the expiry as a header while pyjwt includes the
expiry in the body/payload itself. This shouldn't be a significant issue
since these tokens already expire after 8 hours.

Fixes #6224.

Testing

  • Get a valid token: curl -X POST -d '{"username":"journalist", "passphrase":"correct horse battery staple profanity oil chewy","one_time_code":"XXXXXX"}' -H "Content-Type: application/json" http://127.0.0.1:8081/api/v1/token. Then try it against an API endpoint: curl -H 'Authorization: Token {token}' http://127.0.0.1:8081/api/v1/users and get a proper response back.
  • Log in with SD Client, perform various interactions (uses API token for authentication)
  • Log out of SD client, select the previously used token out of the revoked_tokens database table and try to use it against the API, e.g. curl -H 'Authorization: Token {token}' http://127.0.0.1:8081/api/v1/users. You should get an error message saying the token is invalid or expired.
  • Edit line 122 of journalist_app/api.py so that the expiration is 10 (seconds). Get a valid token using the first curl command, then wait 10 seconds. Try making an API request using that token, and get a response back saying the token is invalid or expired.

Deployment

Any special considerations for deployment? Yes-ish.

Existing API tokens will no longer be valid and need to be reissued. This shouldn't be a big deal since they only live for 8 hours anyways.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container
  • I have written a test plan and validated it for this PR
  • These changes do not require documentation

If you added or updated a production code dependency:

  • I would like someone else to do the diff review
    • I already reviewed the code myself, it would be good for someone else to do a review as well.

itsdangerous deprecated TimedJSONWebSignatureSerializer in 2.0 and has
entirely removed it in 2.1, so we need to switch to something else. They
recommended authlib, except that's a pretty large general-purpose
authentication library.

pyjwt is a smallish (1k LOC) library that just provides JWT/JWS
functionality. For the HS512 algorithm, it uses the same implementation
from hashlib in the Python standard library as itsdangerous did.

Tests are provided to verify successful operation as well as different
failure modes, such as invalid tokens, incorrect secret key, wrong
algorithm, and lack of an expiry.

Tokens generated by itsdangerous are not decodable by pyjwt because
itsdangerous passed the expiry as a header while pyjwt includes the
expiry in the body/payload itself. This shouldn't be a significant issue
since these tokens already expire after 8 hours.

Fixes #6224.
@legoktm legoktm requested a review from a team as a code owner February 12, 2022 01:53
Copy link
Member

@lsd-cat lsd-cat left a comment

Choose a reason for hiding this comment

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

Well structured, clean and no specific behavior differences from before!

@eloquence eloquence marked this pull request as draft March 30, 2022 21:09
@legoktm
Copy link
Member Author

legoktm commented Aug 18, 2022

We want to move forward with #6403 instead.

@legoktm legoktm closed this Aug 18, 2022
@legoktm legoktm deleted the itsdangerous-to-pyjwt branch August 18, 2022 19:28
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.

Switch from itsdangerous to another JWT library
3 participants