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

Add last_used possibility and filtering on tokens #347

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

roalter
Copy link

@roalter roalter commented May 13, 2024

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.

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.
@johnraz
Copy link
Collaborator

johnraz commented May 13, 2024

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.
@roalter
Copy link
Author

roalter commented May 13, 2024

@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)

  • The user creates a token via the LOGIN Api of the knox_auth. Everything fine.
  • The user logs out the actual token
  • The user logs out all tokens of a kind

Scenario b)

  • A user wants to authorize a service to use his credentials.
  • The user generates (over a tbd api) a token and shares the token.
  • The user normally does not want to logout this token.

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
Copy link
Collaborator

johnraz commented May 26, 2024

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 😉

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

Successfully merging this pull request may close these issues.

None yet

2 participants