Sweep self-registered OAuth clients left without a grant for 30 days

app/controllers/oauth/authorizations_controller.rb

@@ -37,7 +37,11 @@ def new
   def create
     if denial?
       redirect_to error_redirect_uri("access_denied", "User denied the request"), allow_other_host: true
-    else
+    # Consent is use, so it restarts a self-registered client's retention
+    # period: the sweep (Oauth::Client.cleanup) mustn't remove the client while
+    # the code it is about to receive is still redeemable. A touch that finds no
+    # row means the sweep got there first, and a code would be dead on arrival.
+    elsif @client.touch
       code = Oauth::AuthorizationCode.generate \
         client_id: @client.client_id,
         identity_id: Current.identity.id,
@@ -46,6 +50,8 @@ def create
         scope: Oauth.canonical_scope(params[:scope].presence || "read")
 
       redirect_to success_redirect_uri(code), allow_other_host: true
+    else
+      oauth_error "invalid_request", "Unknown client"
     end
   end
 

app/controllers/oauth/tokens_controller.rb

@@ -45,7 +45,7 @@ def create
       granted = Oauth.canonical_scope(@auth_code.scope)
       permission = granted.split.include?("write") ? "write" : "read"
 
-      if access_token = @identity.access_tokens.redeem(@auth_code, oauth_client: @client, permission: permission)
+      if access_token = @client.redeem(@auth_code, identity: @identity, permission: permission)
         render json: token_response(access_token, scope: granted)
       else
         oauth_error "invalid_grant", "Authorization code already used"
@@ -60,6 +60,8 @@ def create
     else
       oauth_error "invalid_grant", "Invalid refresh token"
     end
+  rescue ActiveRecord::RecordNotFound
+    oauth_error "invalid_grant", "Unknown client"
   end
 
   private
@@ -138,11 +140,12 @@ def set_refreshable_access_token
     end
 
     # The code or refresh token must have been issued to the client_id in the
-    # request (RFC 6749 §4.1.3, §6).
+    # request (RFC 6749 §4.1.3, §6). A grant whose client has been swept away
+    # (Oauth::Client.cleanup) was issued to no client that still exists.
     # One issued to another client gets the same answer as a dead one, so
     # naming a public client tells nobody whether someone else's grant is live.
     def validate_client_id
-      unless oauth_client_id == (@client || @access_token.oauth_client).client_id
+      unless oauth_client_id == (@client || @access_token.oauth_client)&.client_id
         oauth_error "invalid_grant", authorization_code_grant? ? "Invalid or expired authorization code" : "Invalid refresh token"
       end
     end

app/models/identity/access_token.rb

@@ -34,6 +34,13 @@ class Identity::AccessToken < ApplicationRecord
 
   before_create :set_expiry_and_refresh_token, if: :oauth_client_id?
 
+  # Issuing or destroying a grant restarts its client's retention period
+  # (Oauth::Client.cleanup). The touch waits until the grant's own transaction
+  # commits. A revocation locks the grant, and touching the client in the same
+  # transaction would then wait on the client's lock. A replayed code exchange
+  # holds that lock while it waits on the grant, so the two would deadlock.
+  after_commit :touch_oauth_client, on: %i[ create destroy ]
+
   class << self
     # The token a bearer request may authenticate with: active and honored.
     # Whether it permits the request's method is the caller's question.
@@ -113,6 +120,10 @@ def refresh(permission: self.permission)
   end
 
   private
+    def touch_oauth_client
+      Oauth::Client.where(id: oauth_client_id).touch_all if oauth_client_id?
+    end
+
     def lock_grant
       self.class.lock.where(id: id).pluck(:id)
     end

app/models/oauth/client.rb

@@ -4,10 +4,20 @@ class Oauth::Client < ApplicationRecord
   SECRET_AUTH_METHODS = %w[ client_secret_post client_secret_basic ]
   AUTH_METHODS = %w[ none ] + SECRET_AUTH_METHODS
 
-  has_many :access_tokens, class_name: "Identity::AccessToken", foreign_key: :oauth_client_id, inverse_of: :oauth_client
+  # Registration is open, and MCP clients often register afresh on every
+  # install or reconnect, so a self-registered client left without a grant
+  # this long is abandoned (RFC 7591 §5).
+  UNUSED_RETENTION = 30.days
+
+  has_many :access_tokens, class_name: "Identity::AccessToken", foreign_key: :oauth_client_id, inverse_of: :oauth_client, dependent: :destroy
 
   has_secure_token :client_id, length: 32
 
+  # A code exchange locks the client and then issues a grant (#redeem).
+  # Destroying a client takes the same lock before its grants cascade, in the
+  # same order, so no grant can land after the cascade and be stranded.
+  before_destroy :lock_client, prepend: true
+
   validates :name, presence: true, length: { maximum: 255 }
   validates :client_id, uniqueness: true, allow_nil: true
   # Registration is open and unauthenticated, so what it stores is bounded:
@@ -27,6 +37,36 @@ class Oauth::Client < ApplicationRecord
   scope :trusted, -> { where trusted: true }
   scope :dynamically_registered, -> { where dynamically_registered: true }
 
+  # 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, -> { dynamically_registered.where.missing(:access_tokens).where(updated_at: ...UNUSED_RETENTION.ago) }
+
+  def self.cleanup
+    stale.find_each(&:destroy_if_still_unused)
+  end
+
+  # The sweep and #redeem take the same row lock, so a grant either lands
+  # before the recheck, which then spares the client, or finds the client
+  # already gone and is never issued. There is no foreign key to refuse a
+  # token for a deleted client. The recheck is the whole stale test, not just
+  # the absence of tokens: a grant issued and revoked since the sweep picked
+  # this client has touched it, and restarted its retention period.
+  def destroy_if_still_unused
+    with_lock do
+      destroy if self.class.stale.exists?(id)
+    end
+  rescue ActiveRecord::RecordNotFound
+    # Already gone, deleted since the sweep picked it.
+  end
+
+  # Raises ActiveRecord::RecordNotFound if the client has been swept since it
+  # was read.
+  def redeem(authorization_code, identity:, permission:)
+    with_lock do
+      identity.access_tokens.redeem authorization_code, oauth_client: self, permission: permission
+    end
+  end
 
   def confidential?
     token_endpoint_auth_method.in?(SECRET_AUTH_METHODS)
@@ -54,6 +94,14 @@ def allows_scope?(requested_scope)
   end
 
   private
+    # Takes the row lock without reloading, since lock! refuses a record with
+    # unsaved changes and destroy otherwise ignores them. Resetting the grants
+    # makes the cascade see every grant committed before the lock.
+    def lock_client
+      self.class.lock.where(id: id).pluck(:id)
+      access_tokens.reset
+    end
+
     def generate_client_secret
       self.client_secret ||= self.class.generate_unique_secure_token
     end

config/recurring.yml

@@ -39,6 +39,9 @@ production: &production
   cleanup_imports:
     command: "Account::Import.cleanup"
     schedule: every hour at minute 25
+  cleanup_oauth_clients:
+    command: "Oauth::Client.cleanup"
+    schedule: every day at 04:12
   incineration:
     class: "Account::IncinerateDueJob"
     schedule: every 8 hours at minute 16