Skip to content

Commit

Permalink
Extract expired scope on AccessToken & AccessGrant
Browse files Browse the repository at this point in the history
Like I did for AccessToken.expires_after, I've reused the
`#expiration_time_sql` method built-in to the `doorkeeper` gem to make
the code robust against a change of database e.g. from MySQL to
PostgreSQL.

I've re-opened the `Doorkeeper:AccessGrant` class like I did for
`Doorkeeper::AccessToken`. Unfortunately I've had to explicitly include
the `Models::ExpirationTimeSqlMath` concern, because while
`Doorkeeper::AccessToken` included it, `Doorkeeper:AccessGrant` does
not.

Extracting this scope has allowed me to simplify
`ExpiredOauthAccessRecordsDeleter`.

Note that as before I've had to wrap the call to `#expiration_time_sql`
in a call to `ActiveRecord::Sanitization::ClassMethods#sanitize_sql` [1]
in order to avoid Brakeman failing with a SQL Injection warning.

[1]: https://api.rubyonrails.org/v7.0.8/classes/ActiveRecord/Sanitization/ClassMethods.html#method-i-sanitize_sql
  • Loading branch information
floehopper committed Oct 10, 2023
1 parent 1d985b2 commit b6c4279
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 3 deletions.
7 changes: 7 additions & 0 deletions app/models/doorkeeper/access_grant.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module Doorkeeper
class AccessGrant < ::ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord
include Models::ExpirationTimeSqlMath

scope :expired, -> { where.not(expires_in: nil).where("#{sanitize_sql(expiration_time_sql)} < ?", Time.current) }
end
end
1 change: 1 addition & 0 deletions app/models/doorkeeper/access_token.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ module Doorkeeper
class AccessToken < ::ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord
scope :not_revoked, -> { where(revoked_at: nil) }
scope :expires_after, ->(time) { where.not(expires_in: nil).where("#{sanitize_sql(expiration_time_sql)} > ?", time) }
scope :expired, -> { where.not(expires_in: nil).where("#{sanitize_sql(expiration_time_sql)} < ?", Time.current) }
scope :ordered_by_expires_at, -> { order(expiration_time_sql) }
scope :ordered_by_application_name, -> { includes(:application).merge(Doorkeeper::Application.ordered_by_name) }
end
Expand Down
4 changes: 1 addition & 3 deletions lib/expired_oauth_access_records_deleter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ class ExpiredOauthAccessRecordsDeleter
access_token: EventLog::ACCESS_TOKENS_DELETED,
}.freeze

HAS_EXPIRED = "expires_in is not null AND DATE_ADD(created_at, INTERVAL expires_in second) < ?".freeze

def initialize(record_type:)
@record_class = CLASSES.fetch(record_type)
@event = EVENTS.fetch(record_type)
Expand All @@ -19,7 +17,7 @@ def initialize(record_type:)
attr_reader :record_class, :total_deleted

def delete_expired
@record_class.where(HAS_EXPIRED, Time.zone.now).in_batches do |relation|
@record_class.expired.in_batches do |relation|
records_by_user_id = relation.includes(:application).group_by(&:resource_owner_id)
all_users = User.where(id: records_by_user_id.keys)

Expand Down
15 changes: 15 additions & 0 deletions test/models/doorkeeper/access_grant_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
require "test_helper"

class Doorkeeper::AccessGrantTest < ActiveSupport::TestCase
context ".expired" do
should "return grants that have expired" do
grant_expiring_1_day_ago = create(:access_grant, expires_in: -1.day)
grant_expiring_in_1_day = create(:access_grant, expires_in: 1.day)

grants = Doorkeeper::AccessGrant.expired

assert_includes grants, grant_expiring_1_day_ago
assert_not_includes grants, grant_expiring_in_1_day
end
end
end
12 changes: 12 additions & 0 deletions test/models/doorkeeper/access_token_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,18 @@ class Doorkeeper::AccessTokenTest < ActiveSupport::TestCase
end
end

context ".expired" do
should "return tokens that have expired" do
token_expiring_1_day_ago = create(:access_token, expires_in: -1.day)
token_expiring_in_1_day = create(:access_token, expires_in: 1.day)

tokens = Doorkeeper::AccessToken.expired

assert_includes tokens, token_expiring_1_day_ago
assert_not_includes tokens, token_expiring_in_1_day
end
end

context ".ordered_by_expires_at" do
should "return tokens ordered by expiry time" do
token_expiring_in_2_weeks = create(:access_token, expires_in: 2.weeks)
Expand Down

0 comments on commit b6c4279

Please sign in to comment.