-
-
Notifications
You must be signed in to change notification settings - Fork 215
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
Add last_used possibility and filtering on tokens #347
base: develop
Are you sure you want to change the base?
Conversation
In the AuthTokenManager class, a migrate method has been added. This method is designed to create a new instance of the token and delete the existing one. The main use case of the migrate method is to move user authentication tokens where necessary.
A new "accessed" field has been introduced to the AuthToken model wherein the timestamp of the last access (authenticated request) will be stored. This feature allows for tracking user activity and improves security by allowing further analysis of usage patterns. Tests have been added to ensure the last accessed time updates as expected.
Adjusted the logout function in the views.py file to only delete API-created sessions. Introduced a new function to return the token prefix, which is now used as a filter when deleting auth tokens.
Reformatted the token deletion line in the post method in views.py for better readability. In auth.py, the update_last_accessed method was refactored to improve clarity and return a boolean status indicating whether the access token was updated.
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. |
This commit includes a new test in tests.py to ensure that the logout_all function works properly and removes correct tokens when they are prefixed. This test checks that the function deletes only the prefixed AuthToken objects as expected.
@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. 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. |
Thanks for detailing your case.
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.
I don't agree with this statement.
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. 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 😉 |
@roalter Thanks for your contribution here!
I agree with @johnraz on this one, the logout all mechanism is a way to invalidate all tokens from Looking at your last few comments, it seems like this PR is trying to do two things:
Ah, that is a good point. Should we start with a last access only and then plan to have a mechanism to soft-delete tokens?
You can solve issue 2 by extending our library and building your own class MyCustomLogoutAll(LogoutAllView):
'''
Custom logout all that keeps api tokens alive.
'''
def post(self, request, format=None):
# Only logout API-created sessions!
request.user.auth_token_set.filter(
token_key__startswith=settings.MY_CUSTOM_TOKEN_API_PREFIX
).delete()
user_logged_out.send(sender=request.user.__class__, request=request, user=request.user)
return self.get_post_response(request) I maintain an application that also has API tokens and auth tokens, and we use completely different authentication methods for external and internal APIs. The frontend uses Knox tokens, while the public APIs use a separate solution that is managed separated (you can use DRF's own |
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.