search

jazzband / django-rest-knox

Authentication Module for django rest auth
MIT License
1.1k stars 206 forks source link

Count non expiring tokens when determining if the limit is reached #318

Open morty opened 9 months ago

morty commented 9 months ago

Fixes #280

Thanks to @pablomm for the code in the ticket

giovannicimolin commented 9 months ago

@morty @pablomm Thanks for the contribution! That's a nice catch!

I have one concern with this change though. Once we start considering non-expiring tokens, it is possible that users end up getting locked out of their accounts after issuing a certain number of tokens without logging out properly.

We either need this behind settings flag or a way for new log-ins to remove the oldest unused token. Do you think my concern makes sense?

pablomm commented 8 months ago

HI @giovannicimolin and thanks to @morty for turning the fix into a pull request.

In my case, I use these non-expiring tokens for users that uses my platform through an API. They have access to a control panel (which they access with another authentication system) where they can create, view and delete their api tokens (knox tokens which may be non-expiring). So logging out does not make sense in my use case.

The problem you comment, wouldn't it also happen if someone creates too many tokens within the expiration time?

morty commented 8 months ago

To get to this situation two settings (TOKEN_TTL and TOKEN_LIMIT_PER_USER) would have had to be changed from their default values. That's a deliberate choice. For TOKEN_LIMIT_PER_USER to be ignored without warning if TOKEN_TTL is None seems dangerous. Maybe a warning in the documentation for TOKEN_LIMIT_PER_USER saying that when the limit is reached users won't be able to get another one until one of their tokens has timed out and if they are non-expiring then will be locked out until they can use another method to delete a token?

My client is using this with a very small number of end users where they could handle support requests to delete tokens if required. I could see this wouldn't be possible where there are a very large number of end users.

If LogoutAllView allowed other authentication methods like LoginView then the user could clean out their tokens with username and password. But it looks like this might have been explored before based on the warning in the docs for the LogoutAllView.

johnraz commented 2 months ago

@morty @giovannicimolin I believe the change made here makes full sense, this is clearly a bug that needs to be fixed.

I would add a note to the documentation (and probably a comment in the code) that clearly explains the consequence of having dangling non-expiring tokens around.

Using non-expiring tokens should be also discouraged as it's a bad practice anyway.