diff --git a/app/controllers/my/connected_apps_controller.rb b/app/controllers/my/connected_apps_controller.rb index 94da5900e2..8517622edb 100644 --- a/app/controllers/my/connected_apps_controller.rb +++ b/app/controllers/my/connected_apps_controller.rb @@ -19,7 +19,7 @@ def destroy private def set_connected_apps - tokens = oauth_tokens.includes(:oauth_client).order(:created_at) + tokens = oauth_tokens.unlapsed.includes(:oauth_client).order(:created_at) @connected_apps = tokens.group_by(&:oauth_client).sort_by { |client, _| client.name.downcase } end diff --git a/app/controllers/oauth/tokens_controller.rb b/app/controllers/oauth/tokens_controller.rb index 11942ee4e4..ee696c2dbb 100644 --- a/app/controllers/oauth/tokens_controller.rb +++ b/app/controllers/oauth/tokens_controller.rb @@ -25,6 +25,7 @@ class Oauth::TokensController < Oauth::BaseController before_action :set_refreshable_access_token, unless: :authorization_code_grant? before_action :validate_client_id + before_action :reject_lapsed_grant, unless: -> { authorization_code_grant? || @retired_refresh_token } with_options if: :authorization_code_grant? do before_action :validate_pkce @@ -152,6 +153,12 @@ def unretryable_replay? @retired_refresh_token && !@retired_refresh_token.retryable?(@access_token) end + def reject_lapsed_grant + if @access_token.lapsed? + oauth_error "invalid_grant", "Refresh token expired" + end + end + # A refresh request may narrow scope but never widen it (RFC 6749 ยง6). An # omitted scope keeps the original grant; a requested subset narrows the # rotated token; anything beyond the grant is invalid_scope. Only an absent diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index 715c107fe1..dd7ac7b693 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -1,6 +1,19 @@ class Identity::AccessToken < ApplicationRecord EXPIRES_IN = 1.hour + # A grant whose refresh token goes unused this long lapses, and the client + # must ask the user again. Each refresh restarts the clock, as each rotation + # in bc3 mints a refresh token good for its 90-day refresh_token_ttl. + # + # The clock is updated_at. Only issuance and rotation write a grant, and + # every rotation (this code's or any earlier version's) sets updated_at, so + # no stored deadline can fall out of step with the token it governs. + # + # Fixed seconds, not calendar days: requests run in the browser's time zone, + # where 90.days would shift by an hour across a daylight-saving change, and + # the UTC sweep would then disagree with the token endpoint. + REFRESH_IDLE_LIMIT = 90.days.in_seconds.seconds + belongs_to :identity belongs_to :oauth_client, class_name: "Oauth::Client", optional: true has_many :retired_refresh_tokens, class_name: "Oauth::RetiredRefreshToken", dependent: :delete_all @@ -13,6 +26,8 @@ class Identity::AccessToken < ApplicationRecord scope :personal, -> { where oauth_client_id: nil } scope :oauth, -> { where.not oauth_client_id: nil } scope :active, -> { where(expires_at: nil).or(where(expires_at: Time.current..)) } + scope :unlapsed, -> { where(updated_at: REFRESH_IDLE_LIMIT.ago..) } + scope :lapsed, -> { oauth.where(updated_at: ...REFRESH_IDLE_LIMIT.ago) } has_secure_token enum :permission, %w[ read write ].index_by(&:itself), default: :read @@ -28,6 +43,17 @@ def find_honored(token) end end + # Each grant is rechecked under its lock, so one renewed after it was + # selected (by code from before idle expiry, mid-deploy, whose rotation + # doesn't check for lapse) is spared. + def cleanup + lapsed.find_each do |grant| + grant.with_lock { grant.destroy if grant.lapsed? } + rescue ActiveRecord::RecordNotFound + # Already gone. + end + end + def find_by_refresh_token(refresh_token) oauth.find_by(refresh_token: refresh_token) end @@ -62,8 +88,14 @@ def expires_in (expires_at - Time.current).to_i if expires_at? end + def lapsed? + oauth_client_id? && (updated_at + REFRESH_IDLE_LIMIT).past? + end + # Rotates atomically on the presented refresh token, so a concurrent - # rotation wins the row and the loser comes up empty-handed. The presented + # rotation wins the row and the loser comes up empty-handed. A grant that + # lapses mid-request doesn't rotate either, so a sweep that found it lapsed + # never deletes a grant that was just renewed. The presented # token is retired, not forgotten, so presenting it again is recognized as # a retry or a replay (see Oauth::RetiredRefreshToken). def refresh(permission: self.permission) @@ -72,7 +104,7 @@ def refresh(permission: self.permission) expires_at: EXPIRES_IN.from_now, permission: permission, updated_at: Time.current } transaction do - if self.class.where(id: id, refresh_token: refresh_token).update_all(rotated) == 1 + if self.class.unlapsed.where(id: id, refresh_token: refresh_token).update_all(rotated) == 1 retired_refresh_tokens.create! refresh_token: refresh_token, successor_refresh_token: rotated[:refresh_token] assign_attributes rotated true diff --git a/app/models/oauth/retired_refresh_token.rb b/app/models/oauth/retired_refresh_token.rb index d1b16b2643..76fde92525 100644 --- a/app/models/oauth/retired_refresh_token.rb +++ b/app/models/oauth/retired_refresh_token.rb @@ -8,9 +8,9 @@ class Oauth::RetiredRefreshToken < ApplicationRecord # bc3's refresh_replay_grace default (Oauth::RefreshToken::Rotation). GRACE = 60.seconds - # As long as a replay can still be recognized. It matches bc3's 90-day - # refresh_token_ttl, after which a rotated token there has expired too. - RETENTION = 90.days + # As long as a replay can still be recognized: past the idle limit, the + # grant a retired token came from has lapsed unless it rotated since. + RETENTION = Identity::AccessToken::REFRESH_IDLE_LIMIT belongs_to :access_token, class_name: "Identity::AccessToken" diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index a6e941d269..650bffde0a 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -59,7 +59,7 @@
<%= render "users/theme" %> <%= render "users/transfer", user: @user %> - <%= render "users/connected_apps" if Current.identity.access_tokens.oauth.exists? %> + <%= render "users/connected_apps" if Current.identity.access_tokens.oauth.unlapsed.exists? %> <%= render "users/access_tokens" %> <%= render "users/data_export" %>
diff --git a/config/recurring.yml b/config/recurring.yml index 7097319553..66c8d1a1bf 100644 --- a/config/recurring.yml +++ b/config/recurring.yml @@ -30,6 +30,9 @@ production: &production cleanup_oauth_retired_refresh_tokens: command: "Oauth::RetiredRefreshToken.cleanup" schedule: every day at 04:22 + cleanup_lapsed_oauth_grants: + command: "Identity::AccessToken.cleanup" + schedule: every day at 04:32 cleanup_exports: command: "Export.cleanup" schedule: every hour at minute 20 diff --git a/test/integration/oauth_grant_idle_expiry_test.rb b/test/integration/oauth_grant_idle_expiry_test.rb new file mode 100644 index 0000000000..3ab8e96de1 --- /dev/null +++ b/test/integration/oauth_grant_idle_expiry_test.rb @@ -0,0 +1,119 @@ +require "test_helper" + +# A grant lapses once its refresh token goes unused for the idle window, as +# bc3's refresh tokens do: each refresh restarts the clock, and a lapsed grant +# takes a fresh authorization, with consent. +class OauthGrantIdleExpiryTest < ActionDispatch::IntegrationTest + setup do + @client = oauth_clients(:mcp_client) + freeze_time + @grant = identities(:david).access_tokens.create!(oauth_client: @client, permission: :read) + end + + test "a grant refreshes up to the end of the idle window" do + later_by Identity::AccessToken::REFRESH_IDLE_LIMIT + refresh @grant.refresh_token + + assert_response :success + end + + test "a grant idle past the window lapses" do + later_by Identity::AccessToken::REFRESH_IDLE_LIMIT + 1.second + refresh @grant.refresh_token + + assert_response :bad_request + assert_equal "invalid_grant", response.parsed_body["error"] + end + + test "each refresh restarts the idle clock" do + later_by Identity::AccessToken::REFRESH_IDLE_LIMIT - 1.day + refresh @grant.refresh_token + assert_response :success + + later_by Identity::AccessToken::REFRESH_IDLE_LIMIT - 1.day + refresh response.parsed_body["refresh_token"] + + assert_response :success + end + + test "a lapsed grant leaves Connected Apps" do + sign_in_as :david + + later_by Identity::AccessToken::REFRESH_IDLE_LIMIT + 1.second + get my_connected_apps_path + + assert_response :success + assert_no_match @client.name, response.body + end + + test "the sweep removes lapsed grants and keeps live ones" do + live = identities(:david).access_tokens.create!(oauth_client: @client, permission: :read) + personal = identities(:david).access_tokens.create!(permission: :read, description: "PAT") + + later_by Identity::AccessToken::REFRESH_IDLE_LIMIT - 1.day + live.refresh + + later_by 1.day + 1.second + Identity::AccessToken.cleanup + + assert_not Identity::AccessToken.exists?(@grant.id) + assert Identity::AccessToken.exists?(live.id) + assert Identity::AccessToken.exists?(personal.id) + end + + test "the sweep spares a grant renewed after it was selected" do + later_by Identity::AccessToken::REFRESH_IDLE_LIMIT + 1.second + # The sweep holds the grant as it was read when selected, before the renewal. + selection = stub + selection.stubs(:find_each).yields(Identity::AccessToken.find(@grant.id)) + Identity::AccessToken.stubs(:lapsed).returns(selection) + + Identity::AccessToken.where(id: @grant.id).update_all(updated_at: Time.current) + Identity::AccessToken.cleanup + + assert Identity::AccessToken.exists?(@grant.id) + end + + test "rotation itself refuses a lapsed grant, so one racing the deadline can't renew it" do + later_by Identity::AccessToken::REFRESH_IDLE_LIMIT + 1.second + presented = @grant.refresh_token + + assert_nil @grant.refresh + assert_equal presented, @grant.reload.refresh_token + end + + test "the idle clock is the grant's last rotation, so a rotation by older code mid-deploy restarts it too" do + later_by Identity::AccessToken::REFRESH_IDLE_LIMIT - 1.day + rotated_by_older_code = Identity::AccessToken.generate_unique_secure_token + Identity::AccessToken.where(id: @grant.id).update_all(refresh_token: rotated_by_older_code, updated_at: Time.current) + + later_by Identity::AccessToken::REFRESH_IDLE_LIMIT - 1.day + refresh rotated_by_older_code + + assert_response :success + end + + test "a lapsed grant takes its entry point off the profile page too" do + sign_in_as :david + + get user_path(users(:david)) + assert_match my_connected_apps_path, response.body + + later_by Identity::AccessToken::REFRESH_IDLE_LIMIT + 1.second + get user_path(users(:david)) + assert_no_match my_connected_apps_path, response.body + end + + private + # Elapsed time in the app's zone, as the model counts it: travel would add + # calendar days in the local zone, an hour off across a DST change. + def later_by(duration) + travel_to Time.current + duration + end + + def refresh(refresh_token) + untenanted do + post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: refresh_token, client_id: @client.client_id } + end + end +end diff --git a/test/models/identity/access_token_test.rb b/test/models/identity/access_token_test.rb index c8daf759c5..a0f1a83654 100644 --- a/test/models/identity/access_token_test.rb +++ b/test/models/identity/access_token_test.rb @@ -78,4 +78,18 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase test "find_honored rejects unknown tokens" do assert_nil Identity::AccessToken.find_honored("nonsense") end + + test "the idle limit is a fixed span of seconds, whatever the zone's daylight saving" do + travel_to Time.utc(2026, 12, 1, 12) do + cutoff = Time.current - Identity::AccessToken::REFRESH_IDLE_LIMIT.to_i + grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) + grant.update_columns updated_at: cutoff - 30.minutes + + Time.use_zone("America/New_York") do + assert grant.lapsed? + assert_includes Identity::AccessToken.lapsed, grant + assert_not_includes Identity::AccessToken.unlapsed, grant + end + end + end end