Doorkeeper: Rake task doorkeeper:db:cleanup:expired_tokens fetches expired tokens with an inconsistent expiration time calculation

[pjmartorell]

The rake task doorkeeper:db:cleanup:expired_tokens uses clean_expired method to clean stale tokens, but the query that fetches expired tokens is not consistent with what we consider "expired", since it uses Doorkeeper.config.access_token_expires_in as a reference instead of the expires_in attribute of the access token. Every token may have a different expires_in value depending on the moment the global configuration Doorkeeper.config.access_token_expires_in was changed and is the expires_in the one that indicates if a token is expired or not.
Compare:

https://github.com/doorkeeper-gem/doorkeeper/blob/c54e73115738edb5eb819147516592253198beeb/lib/doorkeeper/rake/db.rake#L20-L25

https://github.com/doorkeeper-gem/doorkeeper/blob/c54e73115738edb5eb819147516592253198beeb/lib/doorkeeper/orm/active_record/stale_records_cleaner.rb#L23-L29

With:
https://github.com/doorkeeper-gem/doorkeeper/blob/c54e73115738edb5eb819147516592253198beeb/lib/doorkeeper/models/concerns/expirable.rb#L26-L33

So in my opinion clean_expired should be:

 # Clears expired records 
 def clean_expired
   @base_scope.where("created_at + expires_in * interval '1 second' < ?", Time.current).in_batches(&:delete_all) 
 end 

Note that the syntax is in PostgreSQL, I couldn't come up with a more agnostic query. ttl argument would be no longer needed.

[nbulaj]

H @pjmartorell . This was discussed somewhere in other issue, you could search by interval '1 second'.

The problem with the last solution is that we must support all the databases at least for ActiveRecord: MySQL / MSSQL / Postgres / SQLIte / etc. And there were also more issues as I remember

[pjmartorell]

@nbulaj I don't have much more ideas apart from creating adapters for every database. An alternative would be to store an expires_at date in a new column, which would simplify the query and could be written in ActiveRecord easily, but I don't think it's worth to introduce this change just to fix this rake task.

[Yoshiji]

I came across this rake task and had the same questioning. So far, I can think of 3 different approaches:

1. allow an optional argument for the rake task, overwriting the ttl given to the cleaner:

   # lib/doorkeeper/rake/db.rake#L24
   cleaner.clean_expired(args.fetch(:ttl, Doorkeeper.config.access_token_expires_in))
   

   One can call the rake task with their own TTL. This helps for apps having constraints like "we must keep an audit log of 30 days min". It can be a solution but it does not solve the issue entirely: if the overwrite ttl arg is smaller than the highest(s) expires_in value in the DB, you might delete a valid token anyway.

2. use ActiveRecord's destroy instead of delete_all: Load the tokens as ApplicationRecord instances, and call destroy if expired?. Obviously, the downsides are increased execution time, memory consumption, and DB delete statements, but this rake task does not have to be fast: quickness to provide feedback to the user is not very important.
   Combine this solution with an optional argument, like the one mentioned above in step 1, and you can reduce the amount of delete statement on the first run(s), for the apps having _a lot of old_ tokens.

3. as @pjmartorell mentioned, store the expires_at rather than computing it, and use it for deletion. As simple as adding a callback before save to compute + store. I don't know all the internals of the gem, but some parts of the code could benefit from this: not having to compute the expires_at at runtime (small perf improvement at read time at the price of a slightly slower write - but the tokens are usually written once), and the devs reading the data would have a clear value rather than having to mentally calculate created_at + expires_in.seconds

---

If any solution is accepted, I can help with the implementation.

[pjmartorell]

• Option 1: I think adding TTL would change the meaning of this rake task, so I think it should be added as a separate rake task, but I don't know if it's worth to keep adding more rake tasks with such specific use cases.
• Option 2: I think it's the most simple and easy approach. As you said, it would probably take more time and memory on the first run.
• Option 3: I think it would be the best and more accurate solution but it would imply rewrite most parts of the code, creating a migration to add expires_at and remove expires_in, etc. so I think it's unfeasible.

[Yoshiji]

Shall I proceed and do a PR implementing option 2: loop through all tokens, and token.destroy if token.expired??

What do you think, @nbulaj @pjmartorell?

[Yoshiji]

up @nbulaj

[nbulaj]

Hi @Yoshiji . Yep, would be great to see a PR.

Can we also do it in a backward compatible way? Maybe we can introduce just a new task for doorkeeper with another name so developers could use what they prefer (faster, more accurate but slower, etc). WDYT?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.