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

Cleanup job removes not expired tokens #1688

Open
jensljungblad opened this issue Jan 25, 2024 · 1 comment · May be fixed by #1690
Open

Cleanup job removes not expired tokens #1688

jensljungblad opened this issue Jan 25, 2024 · 1 comment · May be fixed by #1690

Comments

@jensljungblad
Copy link

This is a follow-up to #1612 which was closed by #1625.

The PR fixed half of the problem. The issue is that the rake task doorkeeper:db:cleanup:expired_tokens will still delete tokens which haven't yet expired. The reason is that the expires_in column is not considered.

Compare the logic in

class StaleRecordsCleaner
  # This method compares against `created_at`
  def clean_expired(ttl)
    table = @base_scope.arel_table
  
    @base_scope
      .where.not(expires_in: nil)
      .where(table[:created_at].lt(Time.current - ttl))
      .in_batches(&:delete_all)
  end
end

With

module Expirable
  # This method compares against `expires_at`
  def expired?
    !!(expires_in && Time.now.utc > expires_at)
  end
end
@fredplante
Copy link
Contributor

Hello @jensljungblad

I don't think there is an issue with this, could you provide a failing test that proves your point?

In Expirable, expires_at is computed as created_at + expires_in.
In StaleRecordsCleaner#clean_expired, ttl is basically the same as expires_in.

so we have:

created_at < (Time.current - expires_in)
VS
Time.now.utc > (created_at + expires_in)

And these 2 statements are equivalent. (I only have a doubt about Time.current vs Time.now.utc, I guess it's fine as long as you have UTC configured as a time zone)

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 a pull request may close this issue.

2 participants