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.

Sign up for GitHub

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

Jump to bottom

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
@Paultdx Paultdx linked a pull request Feb 10, 2024 that will close this issue
Open
@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
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Consider expires_in when clear expired tokens with StaleRecordsCleaner Paultdx/doorkeeper
2 participants
@jensljungblad @fredplante