Consider expires_in when clear expired tokens with StaleRecordsCleaner #1690
Conversation
| @@ -28,6 +28,7 @@ def clean_expired(ttl) | |||
| @base_scope | |||
| .where.not(expires_in: nil) | |||
| .where(table[:created_at].lt(Time.current - ttl)) | |||
| .where(table[:created_at] + table[:expires_in].lt(Time.current)) | |||
There was a problem hiding this comment.
Won't we have an issues with timezones? I believe we're storing UTC and should use the same value regardless of configured application timezone, WDYT?
There was a problem hiding this comment.
Also I believe .where(table[:created_at].lt(Time.current - ttl)) does the same 🤔 By calculating ttl from configuration
There was a problem hiding this comment.
@nbulaj it does the same only if you rely on the ttl from configuration. If in your app your are creating tokens with different expiration times you need to consider expires_in and perform calculations with it.
There was a problem hiding this comment.
Hey @fredplante , you mean tokens created manually with some custom TTL, right?
There was a problem hiding this comment.
@nbulaj Yes, that's what I meant. For info, we had to create a custom task. Here is what we're doing (we use PG):
DoorkeeperExtensions::AccessToken
.where(refresh_token: nil)
.where.not(expires_in: nil)
.where("? > (oauth_access_tokens.created_at + INTERVAL '1 second' * expires_in)", Time.current)
.in_batches(&:delete_all)There was a problem hiding this comment.
Interesting, we have some time SQL math as well https://github.com/doorkeeper-gem/doorkeeper/blob/a02cd99a304b01ca1bf529ca9d6bba5e7ff67f00/lib/doorkeeper/models/concerns/expiration_time_sql_math.rb
BTW MR sounds correct to me
|
Hey @nbulaj @fredplante thanks for checking! |
Presumably fixes #1688.
Consider expires_in when comparing with Time.current.