Consider expires_in when clear expired tokens with StaleRecordsCleaner #1690
Conversation
| @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! |
Paultdx
force-pushed
the
fixes/check-for-expires-in-when-cleaning
branch
from
19d7e41 to
c5edbb8
Compare
Paultdx
force-pushed
the
fixes/check-for-expires-in-when-cleaning
branch
from
c5edbb8 to
99f8068
Compare
|
@nbulaj done. Thanks! 🙏 |
|
Hey @nbulaj do we still consider merging it? I will rebase it then or we can close it if its not needed. Thank you! |
Presumably fixes #1688.
Consider expires_in when comparing with Time.current.