jazzband / django-rest-knox

Authentication Module for django rest auth
MIT License
1.15k stars 211 forks source link

Add last_used possibility and filtering on tokens #347

Open roalter opened 4 months ago

roalter commented 4 months ago

This PR adds a accessed field to track the last usage of the token.

It is a nice-to-have feature, when the tokens are used for a longer duration to allow api access. The applicaiton grants usage of a token for a longer period of time (e.g. 90 days). He uses this token for interacting with the API. Those "api"-tokens should not be revoked by a LogoutAll call. This makes a field of "accessed" necessary to track token and when they where used (similar for GitHub or JFrog Artifactory API tokens)

The LogoutAll call is now limited (currently) to the KNOX_SETTINGS.PREFIX. It limits itself to the token created by the Login view. If you create it on your own with your own prefix, they are not affected.

Additionally a migration function (optional) has been included with automatically could upgrade the existing tokens from 8 to 15 characters.

johnraz commented 4 months ago

Thanks for submitting this PR.

I think the point of the logout all call is to actually invalidate all tokens.

If you don’t want to invalidate some of them then you should probably not use the logout all call to begin with.

Adding an « accessed » field to the DB sounds like something that should be tracked via logs to me - access logs are there for this specific reason.

Also it adds a write to the DB with pretty much all the calls made which doesn’t come cheap.

Maybe I’m missing the point of the feature but in its current state I’m not in favor of the change.

roalter commented 4 months ago

@johnraz : Maybe the usage of the framework could differ. In normal use-cases the token is obtained after a login to the API service. This might be the default and good choice for the most people.

My usecase varies a little as the token can be individually configured.

Scenario a)

Scenario b)

In scenario b the user might want to know when was the last time the token was used. This is a very common api-token use case. The user for this has the possibity to check if a token needs to be revoked or renewed. This scenario is not an auditing scenario (for which there are many other libraries and integrations for django possible).

From the DB costs, the updates a limited to a default of 60 seconds update (MIN_REFRESH). I'm not quite sure up to many users this might scales without performance issues.

johnraz commented 4 months ago

Thanks for detailing your case.

The user normally does not want to logout this token.

Logout all is not a "normal" operation, it's an operation where your account has been compromised and you want to make sure ALL your credentials are revoked.

This scenario is not an auditing scenario (for which there are many other libraries and integrations for django possible).

I don't agree with this statement.

In scenario b the user might want to know when was the last time the token was used.

This is an auditing scenario -> You want to audit when a token was last used.

Now I do see how having these data stored in a log aggregator is less convenient to query and implement behavior to take action upon it...

A more proper and backward-compatible implementation of this would be to have an "token_access_history" table with a FK pointing at the token. You could also more easily add a settings to decide if you want to track token access history or not.

Now this raises the question of what happens if you delete a token, cascade deleting the token would remove information about when the token was accessed in the access table...

Leading to question about soft deleting the token etc...

All of this again looks like an auditing scenario to me.

Feel free to adapt to the above and I can reconsider the feature.

@giovannicimolin @James1345 would love to hear your opinion about this 😉