From 1999583aee54fc1ff79efd2a7b38b3dc17b81d32 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Mon, 13 Nov 2023 16:47:09 +0000
Subject: [PATCH] Handle retired apps when deleting expired grants/tokens
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Trello: https://trello.com/c/GGWWve9b

We saw the following exception in production over the weekend:

> NoMethodError oauth_access_grants:delete_expired
>
> undefined method `name' for nil:NilClass application_names =
> records_by_user_id[user.id].map { |record| record.application.name }

The problem is that some of the expired `Doorkeeper::AccessToken`s
belong to retired applications. Commit
9a9cd02d8574529b19f572a8352f031c3f07455e updated the default scope of
`Doorkeeper::Application` so that it excludes retired apps by default.

The fix is to execute the query that finds the batch of expired
grants/tokens within `Doorkeeper::Application.unscoped` so that we
include retired apps.

As well as the tests added in this commit, I also used the Rails console
in development, and its SQL query output, to give me confidence that
this really is doing what we want. Given that we have an application and
an API user that has an access token for that application:

    > Doorkeeper::AccessToken.includes(:application).group_by(&:resource_owner_id)
    # Note that the query for the applications ignores retired apps
      Doorkeeper::AccessToken Load (0.8ms)  SELECT `oauth_access_tokens`.*
        FROM `oauth_access_tokens`
      Doorkeeper::Application Load (0.7ms)  SELECT `oauth_applications`.*
        FROM `oauth_applications` WHERE `oauth_applications`.`retired` = FALSE
        AND `oauth_applications`.`id` = 1 ORDER BY oauth_applications.name

    * Doorkeeper::Application.unscoped {
    *   Doorkeeper::AccessToken.includes(:application).group_by(&:resource_owner_id)
    > }
    # Note that the query for the applications is not ignoring retired apps
      Doorkeeper::AccessToken Load (0.6ms)  SELECT `oauth_access_tokens`.*
        FROM `oauth_access_tokens`
      Doorkeeper::Application Load (0.6ms)  SELECT `oauth_applications`.*
        FROM `oauth_applications` WHERE `oauth_applications`.`id` = 1
---
 lib/expired_oauth_access_records_deleter.rb   |  2 +-
 ...pired_oauth_access_records_deleter_test.rb | 26 +++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/lib/expired_oauth_access_records_deleter.rb b/lib/expired_oauth_access_records_deleter.rb
index b26b60a12..66ea3ba99 100644
--- a/lib/expired_oauth_access_records_deleter.rb
+++ b/lib/expired_oauth_access_records_deleter.rb
@@ -18,7 +18,7 @@ def initialize(record_type:)
 
   def delete_expired
     @record_class.expired.in_batches do |relation|
-      records_by_user_id = relation.includes(:application).group_by(&:resource_owner_id)
+      records_by_user_id = Doorkeeper::Application.unscoped { relation.includes(:application).group_by(&:resource_owner_id) }
       all_users = User.where(id: records_by_user_id.keys)
 
       all_users.each do |user|
diff --git a/test/lib/expired_oauth_access_records_deleter_test.rb b/test/lib/expired_oauth_access_records_deleter_test.rb
index a78c527f1..63c8a757b 100644
--- a/test/lib/expired_oauth_access_records_deleter_test.rb
+++ b/test/lib/expired_oauth_access_records_deleter_test.rb
@@ -19,6 +19,19 @@ class ExpiredOauthAccessRecordsDeleterTest < ActiveSupport::TestCase
       assert_equal [one_hour_grant], Doorkeeper::AccessGrant.where(resource_owner_id: user.id)
     end
 
+    should "delete expired `Doorkeeper::AccessGrant`s for retired applications" do
+      user = create(:user)
+      grant = create(:access_grant, resource_owner_id: user.id, expires_in: 0)
+      grant.application.update!(retired: true)
+
+      Timecop.travel(5.minutes.from_now)
+
+      deleter = ExpiredOauthAccessRecordsDeleter.new(record_type: :access_grant)
+      deleter.delete_expired
+
+      assert_equal [], Doorkeeper::AccessGrant.where(resource_owner_id: user.id)
+    end
+
     should "provide a count of the total number of records deleted" do
       user = create(:user)
       create(:access_grant, resource_owner_id: user.id, expires_in: 0)
@@ -60,6 +73,19 @@ class ExpiredOauthAccessRecordsDeleterTest < ActiveSupport::TestCase
       assert_equal [one_hour_token], user.authorisations
     end
 
+    should "delete expired `Doorkeeper::AccessToken`s for retired applications" do
+      user = create(:user)
+      token = create(:access_token, resource_owner_id: user.id, expires_in: 0)
+      token.application.update!(retired: true)
+
+      Timecop.travel(5.minutes.from_now)
+
+      deleter = ExpiredOauthAccessRecordsDeleter.new(record_type: :access_token)
+      deleter.delete_expired
+
+      assert_equal [], user.authorisations
+    end
+
     should "provide a count of the total number of records deleted" do
       user = create(:user)
       create(:access_token, resource_owner_id: user.id, expires_in: 0)