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

[Discussion] Cleanup of expired tokens #57

Open
JanThiel opened this issue Oct 24, 2019 · 5 comments
Open

[Discussion] Cleanup of expired tokens #57

JanThiel opened this issue Oct 24, 2019 · 5 comments

Comments

@JanThiel
Copy link
Contributor

JanThiel commented Oct 24, 2019

Hi @jonathan-dejong while updating to the latest version I figured out, that the jwt_data object growths really huge in size over time. I found some accounts having arrays of several thousand entries. All JWT tokens are stored indefinitely no matter if they are already expired and no matter when they expired. Also using the new token/refresh route leads to new tokens being generated on each call.

Should there be some kind of a clean-up? I believe so. For me it feels that a cleanup is required to keep the user_meta in a reasonable size and keeping DB query performance reasonable. Over time the current behaviour will slow down API auth requests due to slower DB queries.

The only reason to keep the expired tokens around is the "Token expired" error to work. Is this even really required? I know that it makes life easier to be able to tell that a token was valid, but after all: expired = invalid. So arguably I would suggest getting rid of expired tokens.
If there should be some grace period for "Token expired" to work, a buffer time like expired + 1 Month could do the trick.

In addition, it might be an easy short term solution, to allow the refresh route to do something like a token "replacement" and thus deleting the old token that was refreshed with the new token.

What do you think?

Best,
Jan

@jonathan-dejong
Copy link
Owner

Hi @JanThiel

This is excellent feedback! I really hadn't thought of this properly..
I agree that there should be some automatic cleanup happening.

Do you think that, instead of having a grace period it would make more sense to have a number of "revisions" stored. So say you keep the last 5 objects and then keep deleting the older ones as new ones are entered. That way I think the risk of hitting an invalid token that's really just expired is minimal while maintaining a decent size of the meta.

Because I can still see value in giving proper user feedback when their session has ended rather than hitting them with something like a "session invalid" which sounds so much harsher.

@pentatonicfunk
Copy link

i would vote for "grace" period + expired time. Hitting expired token is minimal, but it could happen more than rarely, especially on multiple client apps setup. And give "expired" feedback could be useful for the client to decide what to do next.

@JanThiel
Copy link
Contributor Author

JanThiel commented Jun 9, 2020

@jonathan-dejong Personally and from a security perspective I would still tend to not store any old tokens. The workflow for an "invalid" token should always be the same: Get a new one.
@pentatonicfunk What would be the different workflows you would consider, when you hit an expired vs an invalid token?

And all information you pass to the outside can be used against you. The lifetime of a particular token is already known to the initial requestor and shouldn't be a required information when using a token.

If you really want to supply someone with information about the invalidity of a token, the revisions approach will not really work in my oppinion. Take a setup where tokens are generated frequently. The last 5 revisions (or 10, or 50, ...) might just cover the last 5 minutes. So there is no benefit in storing them. As long as a token is valid, it should exist. If it expires, either delete it, or delete it after a configurable grace period.

That's my mindset on this at the moment.

@pentatonicfunk
Copy link

yes, i stand corrected. i dont think we need "expired" tokens at all to be stored. These are thrown by JWT::decode anyway.
So i think we can safe-ly process remove expired tokens while validate_token being called, since this method is the one that called frequently ?

@pentatonicfunk
Copy link

although if main concern is "size" of user meta, generate_token might be the place, since its the method that technically "add" size to that user meta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants