diff --git a/app/models/oauth/client.rb b/app/models/oauth/client.rb index 5c766d0deb..d0b1f2e99e 100644 --- a/app/models/oauth/client.rb +++ b/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 } diff --git a/config/recurring.yml b/config/recurring.yml index a4a97edd40..962d6764f4 100644 --- a/config/recurring.yml +++ b/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 diff --git a/test/integration/oauth_flow_test.rb b/test/integration/oauth_flow_test.rb index ab4fbeab50..e6e54801e6 100644 --- a/test/integration/oauth_flow_test.rb +++ b/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) diff --git a/test/models/oauth/client_test.rb b/test/models/oauth/client_test.rb index f10d6b7c20..5b882c0896 100644 --- a/test/models/oauth/client_test.rb +++ b/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) }