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

custom JWT_RESPONSE_PAYLOAD_HANDLER negated by second call to view serializer #10

Closed
fablet opened this issue Jun 5, 2019 · 6 comments · Fixed by #22
Closed

custom JWT_RESPONSE_PAYLOAD_HANDLER negated by second call to view serializer #10

fablet opened this issue Jun 5, 2019 · 6 comments · Fixed by #22

Comments

@fablet
Copy link

fablet commented Jun 5, 2019

response_serializer = self.get_serializer(

The call to self.get_serializer at line 40 in BaseJSONWebTokenAPIView causes the payload created by api_settings.JWT_RESPONSE_PAYLOAD_HANDLER to be run back through the JSONWebTokenSerializer, effectively eliminating anything added to the payload by a custom handler other than the fields in JSONWebTokenSerializer.
For example, the custom payload handler, that I wrote and have been successfully using with the original GetBlimp package, uses "authentication_token", rather than "token" to return the token, and includes some additional pieces of data, but with the new view, the only thing returned by the response is "email".

@fitodic
Copy link
Collaborator

fitodic commented Jun 13, 2019

Yes, we are aware that this for is not entirely backwards-compatible with the original library. This is one of those places where the two libraries differ.

Have you considered overriding the view's serializer to support the extra fields you've added?

@3dluis
Copy link

3dluis commented Sep 4, 2019

Please approve the merge this this pull request

Pull Request

paymog added a commit to paymog/django-rest-framework-jwt that referenced this issue Dec 6, 2019
Remove the second response serialization because it breaks custom
responses. Fixes Styria-Digital#10
paymog added a commit to paymog/django-rest-framework-jwt that referenced this issue Dec 6, 2019
Remove the second response serialization because it breaks custom
response payload handlers. Fixes Styria-Digital#10
paymog added a commit to paymog/django-rest-framework-jwt that referenced this issue Dec 6, 2019
Remove the second response serialization because it breaks custom
response payload handlers. Fixes Styria-Digital#10
@paymog
Copy link

paymog commented Dec 6, 2019

See #22

paymog added a commit to paymog/django-rest-framework-jwt that referenced this issue Dec 6, 2019
Remove the second response serialization because it breaks custom
response payload handlers. Fixes Styria-Digital#10
@tchaguitos
Copy link

thanks @paymog :)

@marcosox
Copy link

Yes, we are aware that this for is not entirely backwards-compatible with the original library. This is one of those places where the two libraries differ.

Is this thing documented somewhere? I switched libraries and it silently broke my application. At first impression I would consider this a bug, since it negates completely the use of custom payload handlers if I understand correctly.

Anyway, what is the rationale behind this change?

paymog added a commit to paymog/django-rest-framework-jwt that referenced this issue Dec 12, 2019
Remove the second response serialization because it breaks custom
response payload handlers. Fixes Styria-Digital#10
paymog added a commit to paymog/django-rest-framework-jwt that referenced this issue Dec 12, 2019
Remove the second response serialization because it breaks custom
response payload handlers. Fixes Styria-Digital#10
paymog added a commit to paymog/django-rest-framework-jwt that referenced this issue Dec 13, 2019
Remove the second response serialization because it breaks custom
response payload handlers. Fixes Styria-Digital#10
@tchaguitos
Copy link

@fitodic @paymog \o/

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 a pull request may close this issue.

6 participants