jpadilla / django-rest-framework-jwt

JSON Web Token Authentication support for Django REST Framework
http://jpadilla.github.io/django-rest-framework-jwt/
MIT License
3.19k stars 652 forks source link

increased security - allow secret to be kept on user model. #310

Closed jacoor closed 7 years ago

jacoor commented 7 years ago

Goal is to allow secret to be kept on user level, so in case of user token being compromised I don't have to logout everybody by changing secret, I can only change secret on this one person. It also allows to force logout on all devices (by user) or to log out when changing password.

Alex3917 commented 7 years ago

Is there any advantage to using a secret over a DateTime field, and just not accepting tokens issued before the stored date if there is one?

jacoor commented 7 years ago

@Alex3917 that would be also a pretty good way to solve the issue however in my use case the date or the token needs to be stored in the user model because I want to simply invalidate one user tokens. Also, using a date that human can edit would be a bit dangerous. A mistake in changing the date could result in setting in future so the user would not be able to use the app until that date passes - debugging that could be a real pain. Probably to implement this safely it would require some button on user profile in admin, like reset JWT tokens or sth, that would set this DateTime to now(), instead just simple field with string.

jacoor commented 7 years ago

I'll give it thought. Glad you like it.

jpadilla commented 7 years ago

@angvp thoughts on this?

Alex3917 commented 7 years ago

@jacoor

Yeah that's how I do it currently, in terms of storing a datetime field on the user model and just setting it to timezone.now() whenever a user changes their password. Then I just validate it like this:

    """ Expire token on password change and force user to re-authenticate. """

    def authenticate_credentials(self, payload):
        user = super().authenticate_credentials(payload)

        orig_iat = int(payload['orig_iat'])
        password_last_changed = int(format(user.password_last_changed, 'U'))

        if orig_iat < password_last_changed:
            msg = 'Users must re-authenticate after changing password.'
            raise exceptions.AuthenticationFailed(msg)

        return user

I haven't run into any bugs with this, although I guess if this is part of a library that's going to be deployed in environments where server times could be out of sync or whatever then a uuid is probably more robust. (I personally like having the data about when users last changed/reset their passwords though.)

angvp commented 7 years ago

I agree with @jpadilla here, if you can get rid of that duplicated settings for auth model and use the method get_user_model [0] instead would be better.

[0] https://docs.djangoproject.com/en/1.10/topics/auth/customizing/#django.contrib.auth.get_user_model

jacoor commented 7 years ago

@jpadilla @angvp thank you for pointing me to the fix. Updated, tests passed.

jacoor commented 7 years ago

@remik please keep an eye on this too. We can remove our fork from projects when this is merged.

jacoor commented 7 years ago

@jpadilla @angvp any chance of merging this to master soon? Thanks!

angvp commented 7 years ago

@jpadilla I've reviewed and I'm ok to merge it, wanna do it and create the release on PyPI as well? :-) Thanks!.

angvp commented 7 years ago

Merged, please @jpadilla let us know when you build a new release :D

jacoor commented 7 years ago

Great! thanks!

jpadilla commented 7 years ago

Just released this under v1.10. Thank you all!