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

On refresh I think the user is not checked again (whether it exists) #87

Open
gabn88 opened this issue Feb 27, 2019 · 4 comments
Open

Comments

@gabn88
Copy link

gabn88 commented Feb 27, 2019

I've been going through most of the code now, trying to find differences between django-rest-framework-jwt and this package to migrate to this package if we can and if the security aspects allow it.

So far there are some minor differences:

  1. refresh tokens and access tokens are created with a different type instead of the same type (in -jwt you can refresh a token by sending an access token to the /refresh endpoint). I like the simple-jwt approach better, but it requires a change on the client.
  2. By default, the payload is only cryptographically within the token and these are not added to the response as seperate JSON fields. This is easy to change by changing the Serializers and views and actually the simple-jwt approach is better.
  3. The token handling is done by pyjwt, which is much cleaner.

But there are also some major differences/drawbacks:

  1. The access token and refresh token issued by the /refresh endpoint is only tested for validity, there are no extra checks whether the user exists and is still active. The payload is directly copied from the refreshtoken send to the /refresh endpoint and also not updated/checked. So only when a new access_token is obtained from /obtain (which require a user to enter his/her credentials) this data is refreshed/checked.
  2. One of the main issues of -jwt is also an issue in this package, namely that request.user is undefined in the Django middleware when authenticating using jwt, see request.user shows AnonymousUser in middleware jpadilla/django-rest-framework-jwt#45, also for a middleware that fixes it (but sadly requires another authentication round-trip).
@davesque
Copy link
Member

refresh tokens and access tokens are created with a different type instead of the same type (in -jwt you can refresh a token by sending an access token to the /refresh endpoint). I like the simple-jwt approach better, but it requires a change on the client.

Simple JWT has this functionality. Look at "Sliding Tokens" in the readme. Although I think sliding tokens are not a good idea. I recommend against using them instead of a refresh/access token pair.

The access token and refresh token issued by the /refresh endpoint is only tested for validity, there are no extra checks whether the user exists and is still active. The payload is directly copied from the refreshtoken send to the /refresh endpoint and also not updated/checked. So only when a new access_token is obtained from /obtain (which require a user to enter his/her credentials) this data is refreshed/checked.

That's intentional. One of the reasons people use JWTs instead of DB-backed sessions is that there's no interaction required with a database. If the signature checks out, then why not trust the token unless you know specifically that it has been revoked? A couple people have recommended that there be a way to blacklist tokens by providing a logout view. Perhaps we could also make a logout-from-all-devices view or something that blacklists all outstanding tokens for a user. Otherwise, I don't want to require a database query whenever someone submits a token to prove authorization.

One of the main issues of -jwt is also an issue in this package, namely that request.user is undefined in the Django middleware when authenticating using jwt, see GetBlimp/django-rest-framework-jwt#45, also for a middleware that fixes it (but sadly requires another authentication round-trip).

This seems more like an issue with the way DRF does authentication. Would one of the workarounds in that issue be sufficient? Maybe you should search for/open an issue for this with the DRF project.

@gabn88
Copy link
Author

gabn88 commented Mar 1, 2019

Thank you for your response. 👍

I'm not fully into the spec of JWT, but I feel there must be a check if the user still exists and is active somewhere on the refresh. Most of the scheme's I've seen also state that a refresh requires contact with an authentication server (and database). Whereas accessing a resource with an access token does not (per definition, it might) require a check on the user. This is the main benefit of JWT. Not the fact that a refresh does not require an DB hit.Just my 2 cents.

I have now added those check myself on our application, so it is no showstopper, but I was just wondering how you think about it.

@nithinmurali
Copy link

That's intentional. One of the reasons people use JWTs instead of DB-backed sessions is that there's no interaction required with a database. If the signature checks out, then why not trust the token unless you know specifically that it has been revoked? A couple people have recommended that there be a way to blacklist tokens by providing a logout view. Perhaps we could also make a logout-from-all-devices view or something that blacklists all outstanding tokens for a user. Otherwise, I don't want to require a database query whenever someone submits a token to prove authorization.

I think the /refresh should hit the database and check if the user actually exists. As the refresh token is used in a single request, i don't think hitting db would cause an overhead. In my case there is an endpoint for the user to delete himself. But there is no way for me to logout him after he deletes himself. As currently the refresh token dosen't check that user exists, the user can stay logged-in indefinitely even after deleting himself.

@ShamansCoding
Copy link

ShamansCoding commented Apr 26, 2019

I think that might be some kind of optional behavior. Some times it is very convenience. In my case, I'm sending link to user photo inside the token. And also some permissions inside token that might change, during a user session.

Can it be possible?

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

No branches or pull requests

4 participants