Skip to content
This repository has been archived by the owner on May 26, 2020. It is now read-only.

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

Merged
merged 7 commits into from
Mar 21, 2017

Conversation

jacoor
Copy link
Contributor

@jacoor jacoor commented Mar 1, 2017

No description provided.

@jacoor
Copy link
Contributor Author

jacoor commented Mar 1, 2017

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
Copy link
Contributor

Alex3917 commented Mar 3, 2017

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
Copy link
Contributor Author

jacoor commented Mar 3, 2017

@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.

Copy link
Owner

@jpadilla jpadilla left a 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?

@jacoor
Copy link
Contributor Author

jacoor commented Mar 4, 2017 via email

@jpadilla
Copy link
Owner

jpadilla commented Mar 4, 2017

@angvp thoughts on this?

@Alex3917
Copy link
Contributor

Alex3917 commented Mar 4, 2017

@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
Copy link
Contributor

angvp commented Mar 4, 2017

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
Copy link
Contributor Author

jacoor commented Mar 4, 2017

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

@jacoor
Copy link
Contributor Author

jacoor commented Mar 8, 2017

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

@jacoor
Copy link
Contributor Author

jacoor commented Mar 20, 2017

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

@angvp
Copy link
Contributor

angvp commented Mar 20, 2017

@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`.
Copy link
Owner

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?

Copy link
Contributor Author

@jacoor jacoor Mar 20, 2017

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.

Copy link
Owner

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!


# 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'))
Copy link
Owner

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@angvp angvp merged commit 3b1b15a into jpadilla:master Mar 21, 2017
@angvp
Copy link
Contributor

angvp commented Mar 21, 2017

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

@jacoor
Copy link
Contributor Author

jacoor commented Mar 21, 2017

Great! thanks!

@jpadilla jpadilla added this to the 1.10 milestone Mar 22, 2017
@jpadilla
Copy link
Owner

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants