-
Notifications
You must be signed in to change notification settings - Fork 654
increased security - allow secret to be kept on user model. #310
Conversation
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. |
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? |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this a lot. I'm wondering if we could get rid of JWT_AUTH_USER_MODEL
and just use get_user_model()
where possible like we're already doing. Thoughts?
I'll give it thought. Glad you like it.
|
@angvp thoughts on this? |
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:
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.) |
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 |
@remik please keep an eye on this too. We can remove our fork from projects when this is merged. |
@jpadilla I've reviewed and I'm ok to merge it, wanna do it and create the release on PyPI as well? :-) Thanks!. |
@@ -282,8 +290,6 @@ You can set this to a string if you want to use http cookies in addition to the | |||
The string you set here will be used as the cookie name that will be set in the response headers when requesting a token. The token validation | |||
procedure will also look into this cookie, if set. The 'Authorization' header takes precedence if both the header and the cookie are present in the request. | |||
|
|||
Another common value used for tokens and Authorization headers is `Bearer`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason as to why remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ee should not it be only mentioned in line 276? it is above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jacoor you're right, thanks!
rest_framework_jwt/utils.py
Outdated
|
||
# get user from token, BEFORE verification, to get user secret key | ||
unverified_payload = jwt.decode(token, None, False) | ||
secret_key = jwt_get_secret_key(unverified_payload.get('user_id')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on sending unverified_payload
completely to jwt_get_secret_key
instead of just the user_id
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good one. I'll fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix implemented, @jpadilla.
Thanks for pointing this out.
Merged, please @jpadilla let us know when you build a new release :D |
Great! thanks! |
Just released this under v1.10. Thank you all! |
No description provided.