Styria-Digital / django-rest-framework-jwt

JSON Web Token Authentication support for Django REST Framework
https://styria-digital.github.io/django-rest-framework-jwt/
MIT License
191 stars 57 forks source link

JSONWebTokenSerializer.validate no longer fails if the user is not active #98

Open nigel-gott opened 3 years ago

nigel-gott commented 3 years ago

Hi, I am currently upgrading a project to use this fork from the original jpadilla version. So far the breaking changes have been clearly documented however I ran across this one which was not.

Previously ObtainJSONWebTokenView in jpadilla's version would raise ValidationError("User account is disabled.") if a non active user attempted to use the view and obtain a token. However now in the "Dropped support for drf<3.7, django<1.11. Refactored tests. " commit JSONWebTokenSerializer.validate was changed to no longer fail if the user was inactive.

The other views provided by this library use serializers like VerifyAuthTokenSerializer and RefreshAuthTokenSerializer which call check_user in their validate method which does raise for inactive users, however ObtainJSONWebTokenView uses JSONWebTokenSerializer which no longer does.

We can work around this change in our usage of drf-jwt for now, however:

  1. I'm not sure how intended this change in behaviour was, but perhaps there is a good reason for this change?
  2. It's a bit odd that the other views do check this but ObtainJSONWebTokenView does not.
  3. This is a breaking change from the old version and might trip up other users migrating.

If there is a good reason for this change then I am happy to open an MR updating the documentation to clearly state this change. However if not and we believe this should be fixed then I am also happy to fix it. To do so my initial thoughts are:

  1. Call check_user in JSONWebTokenSerializer.validate
  2. Create a new Serializer for ObtainJSONWebTokenView which calls check_user
  3. Copy the exact old check from jpadilla's JSONWebTokenSerializer.validate back into this libraries version

Let me know which if any of these you would prefer.

Thanks for the fork and all the new features!

jaap3 commented 2 years ago

Note that Django can be configured to use AUTHENTICATION_BACKENDS = ['django.contrib.auth.backends.AllowAllUsersModelBackend'], which will allows inactive users to authenticate.

Luckily we caught this in tests, otherwise it would've suddenly be possible to get tokens for inactive users.

As a workaround the is_active flag is now checked in a custom JWT_RESPONSE_PAYLOAD_HANDLER