Skip to content

fix: always stringify user_id claim #887

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vgrozdanic
Copy link
Contributor

USER_ID_CLAIM is often used as a sub claim, and according to the RFC, sub claim should always be string.

Django knows how to convert strings to int when doing database lookups so this should not make any problems there. So far any other value, but integers were stringified, this PR introduces a breaking change where int values will also be stringified.

This is a first part of #831

@vgrozdanic vgrozdanic requested a review from a team March 10, 2025 19:41
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.

The encoded tokens in the rest of the test suite have a claim that uses an integer user ID which will need to be changed. Currently the tests with user ID claim encoded with an integer still pass given the Authentication class, but we now need to change the tokens to see if a string user ID claim is also valid to pass to the ORM

Also, given this PR merges, we can loosen the PyJWT requirement constraint.

Thanks!

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 again for the PR and checking in on things in the project! Well appreciated! Can you run mypy against this? (TODO add mypy to pre-commit hook)

@vgrozdanic
Copy link
Contributor Author

vgrozdanic commented Mar 26, 2025

thanks again for the PR and checking in on things in the project! Well appreciated!

No problem, i have been using this library for years and finally decided it's time to contribute more :)

Can you run mypy against this?

I did when i was developing this, and when run in strict mode it resulted in:

Found 102 errors in 15 files (checked 30 source files)

From what i could tell in those findings, none is resulted by my change, but some clean up and typing is needed in the whole project to have mypy passing before we can add it to pre-commit and CI. I would like to do some more clean up so that we are ready for 6.0.0 release, and after that i can devote more time to cleaning up and preparing codebase for adding mypy

(TODO add mypy to pre-commit hook)

I added a ticket to add mypy to the project: #896

@Andrew-Chen-Wang
Copy link
Member

No problem, i have been using this library for years and finally decided it's time to contribute more :)

Thank you!

Thanks for opening ticket 896. I believe there was a separate PR to fix all the type issues in #889 and #890

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.

2 participants