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

Add support for djangorestframework-simplejwt #76

Merged
merged 1 commit into from
Mar 11, 2019

Conversation

jgadelange
Copy link
Contributor

Since in the Rest Framework Documentation currently simplejwt is listed as package for implementing JWT with DRF I think this package should support that as wel.

This PR contains the necessary changes to support that. However, it does need an update to the README and should add some tests. Since I don't have the time right now to add those as well I added the PR without those.

Fixes #74

@st4lk
Copy link
Owner

st4lk commented Feb 26, 2019

refresh = serializers.SerializerMethodField()

@classmethod
def get_token_instance(cls, user):
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be get_token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this should be get_token_instance() since otherwise it would conflict with the get_token() used by the SerializerMethodField in the SimpleJWTObtainSlidingSerializer.
It should be changed in SimpleJWTBaseSerializer

token = serializers.SerializerMethodField()

@classmethod
def get_token_instance(cls, user):
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be get_token?

return str(self.token)


class SimpleJWTObtainSlidingSerializer(serializers.Serializer):
Copy link
Owner

Choose a reason for hiding this comment

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

Why it is not inherit SimpleJWTBaseSerializer ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Should be SimpleJWTBaseSerializer

@st4lk
Copy link
Owner

st4lk commented Mar 11, 2019

I'm merging this, but unfortunately, the code is not fully correct. Will continue the work on that.
Thanks for the starting initiative!

@st4lk st4lk merged commit e3770aa into st4lk:master Mar 11, 2019
@basilbegonia
Copy link

Hi, I'd like to help out with this. Can you point me in the right direction on how the code is not fully correct?

@st4lk
Copy link
Owner

st4lk commented Mar 22, 2019

@basilbegonia @jgadelange
Sorry for confusion. I meant that the integration of simple-jwt into this project wasn't complete.
I add some improvements here: #78

If you fill like it wasn't correct or has errors, then fixes are welcome :)

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.

3 participants