jazzband / djangorestframework-simplejwt

A JSON Web Token authentication plugin for the Django REST Framework.
https://django-rest-framework-simplejwt.readthedocs.io/
MIT License
4k stars 662 forks source link

Token Verification ignores Blacklist #231

Open TeresaSiegmantel opened 4 years ago

TeresaSiegmantel commented 4 years ago

You can argue whether or not this is the right behaviour, but I would say it is not. As a user of a token verification endpoint, I expect a token that passed to be usable for authentication. This is not true for blacklisted refresh tokens.

Andrew-Chen-Wang commented 4 years ago

Purpose of blacklisted token is to make sure the revoked token is never used again. If it is used, then, imo, you send back a 403 status code. Correct me if I'm wrong (because I could be misinterpreting your issue), but are you suggesting that a blacklisted token that is still usable (as in not expired and would pass if the blacklist app was off) should be able to pass and allow authentication as in user.is_authenticated == True? @philipp-siegmantel

TeresaSiegmantel commented 4 years ago

Not exactly. What I propose is that TokenVerifySerializeror TokenVerifyView should check the blacklist. If the token is blacklisted, it should report that the token is invalid. Currently it only checks whether the token is "syntactically" correct.

Andrew-Chen-Wang commented 4 years ago

Gotcha. That makes sense. I can draft a PR tonight.

TeresaSiegmantel commented 4 years ago

Gotcha. That makes sense. I can draft a PR tonight.

Thanks a lot.

If you are to busy, I can ask if can contribute to this projekt during work hours. Just don't count on it, from what I heard the answer is probably no.

TeresaSiegmantel commented 4 years ago

I got an ok and am currently working on a pull request.

TeresaSiegmantel commented 4 years ago

Any updates on this? I opened a Merge Request a while ago, it's GH-239.

Andrew-Chen-Wang commented 4 years ago

@philipp-siegmantel SimpleJWT is going to be slow in write access (since I don't have any), but I did leave a review in your PR. Hopefully it can be added into a minor update if we do some semantic versioning.

duducp commented 4 years ago

Hello, I was wondering why there is no such thing in token validation, but I saw that it is being implemented. My suggestion is that this has a control variable in the settings so that the user can choose whether or not to use this validation.

I don't know if this is the right place but it would be very interesting for this module to use cache in token verification. Usually the token verification route is very much in demand and a cache here would relieve the response time a little.

Andrew-Chen-Wang commented 4 years ago

My suggestion is that this has a control variable in the settings so that the user can choose whether or not to use this validation.

It is controlled via the BLACKLIST_ROTATION (something like that) setting in the SIMPLE_JWT dict settings. According to the linked PR, even with the setting on, it did not work, although I haven't had the time to actually use the blacklist app in recent times.

Is the variable working for you?

it would be very interesting for this module to use cache in token verification. Usually the token verification route is very much in demand and a cache here would relieve the response time a little.

TL;DR The actual encryption and decryption of the token is not costly whatsoever. The actual database access (when using the authenticate method is the costly portion, but I don't think you were referring to that).

Caching has the unfortunate consequence of cache invalidation. For example, serving as a generic app, we only look for the is_active flag to make sure the user is still active. In case the user is made inactive, how would the developer know which cache to invalidate? Simple JWT wouldn't know either since we'd have to implement a signal on every post_save. Not only that, some devs might delete the user. This is why we have the "experimental" feature which allows for you to avoid the database entirely and rely solely on the expiration of the token (in case of MITM) and the JWT signing.

The actual encryption and decryption of the token is not costly whatsoever, so imo, with too much flexibility on the developer's end, I don't think it's wise for a package like this to implement caching. But it is very interesting to think about nonetheless 👍

Also:

Usually the token verification route is very much in demand and a cache here would relieve the response time a little.

The purpose of authentication is security based, regardless of speed. Not to say optimization isn't important, but security comes out as top regardless.

duducp commented 4 years ago

I did the test and the variable BLACKLIST_AFTER_ROTATION is working yes, if disabled the blacklist validation is not done.

About the cache I had thought about using it in the validation and the way we would invalidate it could be:

Most of all, this cache issue could be implemented in another PR.

Andrew-Chen-Wang commented 4 years ago

Sure @duducp a PR is definitely welcome. Something like the way Django handles session store would be great, as you described. To me, it's really not too necessary with most sites being small but with browser support with JS framework becoming increasingly likely to happen, it's definitely on the TODO list and something I'm keen in seeing the implementation of :)