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

Allow None for JTI_CLAIM and TOKEN_TYPE_CLAIM #489

Closed

Conversation

Jaza
Copy link

@Jaza Jaza commented Nov 23, 2021

This is the only way I could get it working with an Auth0 JWT, which has neither 'jti' nor 'token_type'.

This is the only way I could get it working with an Auth0
JWT, which has neither 'jti' nor 'token_type'.
@Jaza
Copy link
Author

Jaza commented Nov 23, 2021

Related to #279

@Jaza
Copy link
Author

Jaza commented Nov 23, 2021

Related to #227

@Jaza
Copy link
Author

Jaza commented Nov 23, 2021

Related to #106

@Jaza
Copy link
Author

Jaza commented Nov 23, 2021

Related to #169

@Jaza
Copy link
Author

Jaza commented Nov 23, 2021

@Andrew-Chen-Wang
Copy link
Member

Closing due to inactivity and I don't think it's supposed to be None.

@Jaza
Copy link
Author

Jaza commented Feb 24, 2022

@Andrew-Chen-Wang errr, excuse me? You don't look at this PR for 3 months, and then you just close it?!

I've been running djangorestframework-simplejwt in production, with this change applied, for 3 months. I'd like to stop using a patched version of the library, I've been waiting for this to get reviewed and merged.

Do you have any alternative solution in mind, for supporting an Auth0 JWT? If not, then could you please re-open this PR, and consider merging it?

@Jaza
Copy link
Author

Jaza commented Feb 24, 2022

@davesque any chance you could chime in here?

@Andrew-Chen-Wang
Copy link
Member

Yes, I'm an inconsistent maintainer with flaws -- obviously not reading the spec for one -- who probably failed his exams 5 hours ago? I don't use Auth0, so please be patient as I closed this for inactivity. I didn't say I didn't care.

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.

Besides a test case, LGTM. Apologies for not looking carefully.

Comment on lines +103 to +104
if api_settings.TOKEN_TYPE_CLAIM is not None:
self.verify_token_type()
Copy link
Member

Choose a reason for hiding this comment

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

At first this seemed strange, but I remembered it's Auth0. If possible, please leave a comment mentioning how this should only apply to outside services. Customization of tokens should still stride to use the TOKEN_TYPE_CLAIM setting. A test case would be great!

@Jaza
Copy link
Author

Jaza commented Feb 25, 2022

@Andrew-Chen-Wang thanks for re-opening an reviewing. I'll add a comment and a test case as you've suggested.

Sorry if I sounded rude. I know, this is open source, you're a volunteer, you have limited time, you have to cull issues that might be abandoned, for your sanity. I don't expect you to be familiar with Auth0, nor with any other third-party services that people might use. I appreciate you maintaining this library. Keep it up.

@Andrew-Chen-Wang
Copy link
Member

Np and no worries. Sometimes my own prose are fairly rude, so understandable. Thanks for understanding. Looking for one more check by @2ykwang

@Andrew-Chen-Wang
Copy link
Member

lgtm then thanks!

@mrdinwiddie
Copy link
Contributor

mrdinwiddie commented May 2, 2022

@Andrew-Chen-Wang, do you know when this might be merged? Thanks in advance

Edit : I have opened a pr here #567
it has test cases and some documentation updates. Please let me know any feedback

@Andrew-Chen-Wang
Copy link
Member

Andrew-Chen-Wang commented May 4, 2022

thanks @denniskeends !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants