Sweep dynamic registrations nobody authorized after a day

app/models/oauth/client.rb

@@ -9,6 +9,13 @@ class Oauth::Client < ApplicationRecord
   # this long is abandoned (RFC 7591 §5).
   UNUSED_RETENTION = 30.days
 
+  # Registration is unauthenticated and limited only per address, so a
+  # registration nobody ever authorized is kept just long enough for its user
+  # to finish consent. Its rows are then bounded by the registration rate over
+  # this period rather than over UNUSED_RETENTION. A user still on the consent
+  # screen when it lapses gets "Unknown client" and their app registers again.
+  UNAUTHORIZED_REGISTRATION_RETENTION = 24.hours
+
   # The grants a client may obtain at the authorization and token endpoints.
   # An allowlist, so a client is refused any grant type it doesn't list.
   GRANT_TYPES = %w[ authorization_code refresh_token ]
@@ -76,10 +83,20 @@ class Oauth::Client < ApplicationRecord
   # metadata document. Either comes back on its own when it's used again.
   scope :self_registered, -> { dynamically_registered.or(metadata_documents) }
 
-  # A grant touches its client when it is created or destroyed, so updated_at
-  # is the last grant activity: an app the user disconnected yesterday is not
-  # swept for having registered long ago.
-  scope :stale, -> { self_registered.where.missing(:access_tokens).where(updated_at: ...UNUSED_RETENTION.ago) }
+  # A grant touches its client when it is created or destroyed, and so does
+  # consent, so updated_at is the last grant activity: an app the user
+  # disconnected yesterday is not swept for having registered long ago.
+  scope :stale, -> { self_registered.where.missing(:access_tokens).merge(unused_beyond_retention) }
+
+  scope :unused_beyond_retention, -> do
+    where(updated_at: ...UNUSED_RETENTION.ago).or(never_authorized.where(created_at: ...UNAUTHORIZED_REGISTRATION_RETENTION.ago))
+  end
+
+  # A registration whose row nothing has touched since it was written: no
+  # consent approved, no grant issued or ended. Only dynamic registration
+  # takes the short retention; a metadata document client keeps
+  # UNUSED_RETENTION. A grant it still holds keeps it regardless (stale).
+  scope :never_authorized, -> { dynamically_registered.where(arel_table[:updated_at].eq(arel_table[:created_at])) }
 
   scope :personal_access, -> { where client_id: PERSONAL_ACCESS_CLIENT_ID }
 

config/recurring.yml

@@ -50,7 +50,7 @@ production: &production
     schedule: every hour at minute 25
   cleanup_oauth_clients:
     command: "Oauth::Client.cleanup"
-    schedule: every day at 04:12
+    schedule: every hour at minute 14
   incineration:
     class: "Account::IncinerateDueJob"
     schedule: every 8 hours at minute 16

test/integration/oauth_flow_test.rb

@@ -187,6 +187,17 @@ class OauthFlowTest < ActionDispatch::IntegrationTest
     assert_not_includes Oauth::Client.stale, client
   end
 
+  test "consent takes a dynamic registration off the one-day retention for registrations nobody authorized" do
+    sign_in_as :david
+    client = travel_to(25.hours.ago) { Oauth::Client.create!(name: "Fresh", redirect_uris: %w[ http://127.0.0.1:8888/callback ], dynamically_registered: true) }
+    assert_includes Oauth::Client.stale, client
+
+    post_consent client: client
+
+    assert_response :redirect
+    assert_not_includes Oauth::Client.stale, client
+  end
+
   test "consent issues no code for a client swept while consent was submitted" do
     sign_in_as :david
     client = Oauth::Client.create!(name: "Swept", redirect_uris: %w[ http://127.0.0.1:8888/callback ], dynamically_registered: true)

test/models/oauth/client_test.rb

@@ -403,7 +403,8 @@ class Oauth::ClientTest < ActiveSupport::TestCase
   end
 
   test "cleanup keeps recent, granted and operator-provisioned clients" do
-    recent = travel_to(29.days.ago) { register_client }
+    recent = travel_to(30.days.ago) { register_client }
+    travel_to(29.days.ago) { recent.touch }
     granted = travel_to(31.days.ago) { register_client.tap { identities(:david).access_tokens.create!(oauth_client: it) } }
     provisioned = travel_to(31.days.ago) { register_client(dynamically_registered: false, redirect_uris: %w[ https://app.example.com/cb ]) }
 
@@ -414,6 +415,49 @@ class Oauth::ClientTest < ActiveSupport::TestCase
     assert Oauth::Client.exists?(provisioned.id)
   end
 
+  test "cleanup destroys a dynamic registration nobody authorized within a day" do
+    lapsed = travel_to(25.hours.ago) { register_client }
+    pending = travel_to(23.hours.ago) { register_client }
+
+    Oauth::Client.cleanup
+
+    assert_not Oauth::Client.exists?(lapsed.id)
+    assert Oauth::Client.exists?(pending.id)
+  end
+
+  test "cleanup keeps a dynamic registration past a day once it has been authorized" do
+    granted = travel_to(2.days.ago) { register_client }
+    travel_to(47.hours.ago) { identities(:david).access_tokens.create!(oauth_client: granted) }
+
+    disconnected = travel_to(2.days.ago) { register_client }
+    travel_to(47.hours.ago) { identities(:david).access_tokens.create!(oauth_client: disconnected) }
+    travel_to(46.hours.ago) { disconnected.access_tokens.each(&:destroy) }
+
+    consented = travel_to(2.days.ago) { register_client }
+    travel_to(47.hours.ago) { consented.touch }
+
+    Oauth::Client.cleanup
+
+    assert Oauth::Client.exists?(granted.id)
+    assert Oauth::Client.exists?(disconnected.id), "a client that ever held a grant keeps the full retention period"
+    assert Oauth::Client.exists?(consented.id), "a client whose code is out keeps the full retention period"
+  end
+
+  test "cleanup keeps unauthorized clients that weren't dynamically registered past a day" do
+    trusted = travel_to(2.days.ago) { register_client(dynamically_registered: false, trusted: true, redirect_uris: %w[ https://app.example.com/cb ]) }
+    provisioned = travel_to(2.days.ago) { register_client(dynamically_registered: false, redirect_uris: %w[ https://app.example.com/cb ]) }
+    metadata_document = travel_to(2.days.ago) { register_metadata_document_client }
+    oauth_clients(:trusted_client, :personal_access).each { it.update_columns created_at: 2.days.ago, updated_at: 2.days.ago }
+
+    Oauth::Client.cleanup
+
+    assert Oauth::Client.exists?(trusted.id)
+    assert Oauth::Client.exists?(provisioned.id)
+    assert Oauth::Client.exists?(metadata_document.id)
+    assert Oauth::Client.exists?(oauth_clients(:trusted_client).id)
+    assert Oauth::Client.exists?(oauth_clients(:personal_access).id)
+  end
+
   test "cleanup counts the retention period from the client's last grant, not its registration" do
     client = travel_to(60.days.ago) { register_client }
     token = travel_to(40.days.ago) { identities(:david).access_tokens.create!(oauth_client: client) }