diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index 8d4199f213..a02f2fceac 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -48,17 +48,15 @@ class Identity::AccessToken < ApplicationRecord enum :permission, %w[ read write ].index_by(&:itself), default: :read - # Tokens are stored as digests (TokenDigester). The raw values exist only in - # memory, on the record that issued or rotated them, to be handed to their - # holder once. Personal access tokens also keep their plaintext in `token` - # for now, because production code that reads only `token` shares this - # database until this code is deployed there. OAuth access tokens never do. + # Tokens are stored only as digests (TokenDigester). The raw values exist + # only in memory, on the record that issued or rotated them, to be handed to + # their holder once. # - # The plaintext `refresh_token` column is ignored until it's dropped, beside - # `token`. Without partial_inserts, a model that knows a column names it on + # The plaintext `token` and `refresh_token` columns are ignored until they're + # dropped. Without partial_inserts, a model that knows a column names it on # every INSERT, so only code that ignores it can keep creating tokens once # it's gone. - self.ignored_columns += %w[ refresh_token ] + self.ignored_columns += %w[ token refresh_token ] attribute :raw_token, :string attribute :raw_refresh_token, :string @@ -94,14 +92,10 @@ def find_honored(token) end end - # Finds the token within the current scope by its digest. A personal - # access token written by code that stores only plaintext has no digest - # yet, so it's found by plaintext and given one: no lookup but the first - # ever matches on plaintext. Code that stores only plaintext issues - # prefixed tokens too, so a prefix doesn't exempt a token from this. + # Finds the token within the current scope by its digest. def find_by_raw_token(raw) if raw.is_a?(String) && raw.present? - find_by(token_digest: TokenDigester.digests(raw)) || find_and_digest_undigested(raw) + find_by(token_digest: TokenDigester.digests(raw)) end end @@ -135,24 +129,6 @@ def redeem(authorization_code, **attributes) where(authorization_code_jti: authorization_code.jti).destroy_all nil end - - private - # MySQL compares `token` case-insensitively, so a match is confirmed - # byte for byte: a differently cased token neither authenticates nor - # installs a digest that the real token would then never match. - # - # The digest goes to the primary even when the token was found on the - # replica, as GET requests read. update_columns leaves updated_at alone. - # A datacenter that can't reach the primary (Deployment::DatabaseResolver) - # still authenticates the token and leaves the digest to one that can, - # or to the backfill rerun. - def find_and_digest_undigested(raw) - personal.where(token_digest: nil, token: raw).detect { |candidate| candidate.token == raw }&.tap do |access_token| - if Deployment::DatabaseResolver.in_primary_datacenter? - ApplicationRecord.with_writing_role { access_token.update_columns token_digest: TokenDigester.digest(raw) } - end - end - end end # OAuth tokens are honored only while OAuth acceptance is on (see @@ -188,7 +164,7 @@ def lapsed? # answered with it. def refresh(permission: self.permission) raw_token, raw_refresh_token = self.class.generate_token(:access), self.class.generate_token(:refresh) - rotated = { token: nil, token_digest: TokenDigester.digest(raw_token), + rotated = { token_digest: TokenDigester.digest(raw_token), refresh_token_digest: TokenDigester.digest(raw_refresh_token), expires_at: EXPIRES_IN.from_now, permission: permission, updated_at: Time.current } @@ -219,13 +195,10 @@ def lock_grant self.class.lock.where(id: id).pluck(:id) end - # Personal access tokens are written in plaintext as well as digested, - # for production code that reads only plaintext. An explicitly given - # token is digested like a generated one. + # An explicitly given raw token is digested like a generated one. def set_token - self.raw_token ||= token.presence || self.class.generate_token(oauth_client_id? ? :access : :personal) + self.raw_token ||= self.class.generate_token(oauth_client_id? ? :access : :personal) self.token_digest = TokenDigester.digest(raw_token) - self.token = oauth_client_id? ? nil : raw_token end def set_expiry_and_refresh_token diff --git a/db/migrate/20261017120000_backfill_remaining_token_digests.rb b/db/migrate/20261017120000_backfill_remaining_token_digests.rb new file mode 100644 index 0000000000..8832543b52 --- /dev/null +++ b/db/migrate/20261017120000_backfill_remaining_token_digests.rb @@ -0,0 +1,73 @@ +# Digests every access token still without one, as BackfillTokenDigests did, +# before code that finds tokens only by digest is deployed. Until then, +# production code that stores only plaintext kept writing personal access +# tokens, and each was digested only when it was first used. One never used +# since is digested here, or it would stop authenticating. +# +# MySQL and SQLite have no HMAC, so this runs in Ruby, a batch at a time, +# outside one long transaction. update_all writes only the digest: a grant's +# updated_at is its idle-lapse clock (Identity::AccessToken::REFRESH_IDLE_LIMIT), +# and backfilling must not restart it. +# +# Personal access tokens keep their plaintext, as BackfillTokenDigests left +# it, until the column is dropped, so rolling back to code that reads only +# plaintext still finds them. A personal access token is one without a +# client, or one issued by the personal access client where that client +# exists. OAuth access tokens are digest-only, so any plaintext one is +# cleared once digested. +# +# Rerunnable: it only touches rows without a digest, and does nothing once the +# plaintext column is gone. Its down is a no-op, so +# `db:migrate:redo:primary VERSION=20261017120000` runs it again. +class BackfillRemainingTokenDigests < ActiveRecord::Migration[8.2] + disable_ddl_transaction! + + BATCH_SIZE = 500 + + # Identity::AccessToken ignores the plaintext column, so this reads it + # through a model of its own. + class AccessToken < ActiveRecord::Base + self.table_name = "identity_access_tokens" + end + + class Client < ActiveRecord::Base + self.table_name = "oauth_clients" + end + + PERSONAL_ACCESS_CLIENT_ID = "fizzy-personal-access" + + def up + if column_exists?(:identity_access_tokens, :token) + refuse_duplicate_tokens + personal_client_ids = [ nil, *Client.where(client_id: PERSONAL_ACCESS_CLIENT_ID).pluck(:id) ] + + undigested.in_batches(of: BATCH_SIZE) do |batch| + batch.select(:id, :token, :oauth_client_id).each do |row| + AccessToken.where(id: row.id, token_digest: nil).update_all \ + token_digest: TokenDigester.digest(row.token), token: (row.oauth_client_id.in?(personal_client_ids) ? row.token : nil) + end + end + end + end + + def down + # Digests are derived from what's still stored: nothing to undo. + end + + private + def undigested + AccessToken.where(token_digest: nil).where.not(token: nil) + end + + # Two rows sharing a token would share a digest, which the unique index + # refuses partway through. Stop before writing anything instead, naming + # the rows but never the tokens. + def refuse_duplicate_tokens + duplicated = AccessToken.where.not(token: nil).group(:token).having("COUNT(*) > 1").pluck(:token) + + if duplicated.any? + ids = AccessToken.where(token: duplicated).pluck(:id) + raise "identity_access_tokens.token has #{duplicated.size} duplicated value(s), in rows #{ids.join(", ")}. Resolve them before digesting." + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 84cc4c91b7..670c8e1644 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.2].define(version: 2026_10_15_120200) do +ActiveRecord::Schema[8.2].define(version: 2026_10_17_120000) do create_table "accesses", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.datetime "accessed_at" t.uuid "account_id", null: false diff --git a/db/schema_sqlite.rb b/db/schema_sqlite.rb index 1033aa3dc9..14117a38f3 100644 --- a/db/schema_sqlite.rb +++ b/db/schema_sqlite.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.2].define(version: 2026_10_15_120200) do +ActiveRecord::Schema[8.2].define(version: 2026_10_17_120000) do create_table "accesses", id: :uuid, force: :cascade do |t| t.datetime "accessed_at" t.uuid "account_id", null: false diff --git a/docs/oauth/spec_conformance.md b/docs/oauth/spec_conformance.md index 856dcffc5d..2bd1bfbb38 100644 --- a/docs/oauth/spec_conformance.md +++ b/docs/oauth/spec_conformance.md @@ -18,6 +18,7 @@ Snapshot: 2026-10-06, at the heads of the OAuth train, rebased onto main: | [#3161](https://github.com/basecamp/fizzy/pull/3161) | `oauth-grant-idle-expiry` | 90-day idle expiry of grants (on #3160) | | [#3159](https://github.com/basecamp/fizzy/pull/3159) | `oauth-client-sweep` | Sweep of abandoned DCR clients (on #3161) | | [#3192](https://github.com/basecamp/fizzy/pull/3192) | `oauth-token-digests` | Tokens and client secrets stored as digests, step 1 of 3 (on #3169) | +| [#3202](https://github.com/basecamp/fizzy/pull/3202) | `oauth-token-digests-d2` | Personal access tokens stored and found only by digest, step 2 of 3 (on #3192) | Commits are cited by subject, with the short SHA as of the snapshot. Rebases change the SHAs of #3080 and above; the subjects stay the same. @@ -89,7 +90,7 @@ Status key: | Requirement | Status | |---|---| -| Tokens are stored as HMAC-SHA256 digests under a server pepper, byte-compatible with bc3's `Oauth::TokenDigester`, and looked up by a unique index on the digest. Raw values are shown once: in the token response, the registration response, or the response that creates a personal access token | **Fixed** #3192 `oauth-token-digests`, step 1 of 3. OAuth access and refresh tokens and client secrets are digest-only, and a refresh retry within the grace window is answered from an encrypted copy of the successor pair that expires with the window. Personal access tokens are digested and, until step 3, still also kept in plaintext, because production code that reads only plaintext shares the database while beta runs this. One written without a digest is found by plaintext once and digested. Step 2 stops reading and writing plaintext; step 3 drops the column. | +| Tokens are stored as HMAC-SHA256 digests under a server pepper, byte-compatible with bc3's `Oauth::TokenDigester`, and looked up by a unique index on the digest. Raw values are shown once: in the token response, the registration response, or the response that creates a personal access token | **Fixed** #3192 `oauth-token-digests`, step 1 of 3. OAuth access and refresh tokens and client secrets are digest-only, and a refresh retry within the grace window is answered from an encrypted copy of the successor pair that expires with the window. Step 2 (#3202 `oauth-token-digests-d2`) makes personal access tokens digest-only too: they're written and found only by digest, after a rerun of the backfill digests any that production wrote in plaintext before step 1 reached it. The plaintext they held before then stays in a column the code ignores, until step 3 drops it. | | Newly issued tokens carry a distinctive prefix naming their kind: `fizzy_pat_` (personal access token), `fizzy_at_` (OAuth access token), `fizzy_rt_` (OAuth refresh token). Tokens issued earlier have no prefix and still work | **Fixed** #3163. bc3 prefixes similarly (`bc_at_`, `bc_rt_`, and PATs as `bc_at_`). Client secrets carry `fizzy_cs_`, as bc3's carry `bc_cs_`. New tokens and secrets carry 44 base58 characters after the prefix, 257 bits, at least bc3's 32 bytes (rubric TK-1); earlier ones are 24 characters and keep working. Nobody has registered with GitHub's secret-scanning partner program, either here or in bc3. | ## Refresh tokens (RFC 6749 §6, OAuth 2.1 §4.3, RFC 9700 §4.14) diff --git a/test/controllers/accounts/exports_controller_test.rb b/test/controllers/accounts/exports_controller_test.rb index 1888583559..9de3368cb0 100644 --- a/test/controllers/accounts/exports_controller_test.rb +++ b/test/controllers/accounts/exports_controller_test.rb @@ -107,7 +107,7 @@ class Account::ExportsControllerTest < ActionDispatch::IntegrationTest export = Account::Export.create!(account: Current.account, user: users(:jason)) export.build sign_out - bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).token}" } + bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).raw_token}" } get account_export_path(export), as: :json, env: bearer_token assert_response :success diff --git a/test/controllers/active_storage/direct_uploads_controller_test.rb b/test/controllers/active_storage/direct_uploads_controller_test.rb index f5662b9e5f..0c027f221e 100644 --- a/test/controllers/active_storage/direct_uploads_controller_test.rb +++ b/test/controllers/active_storage/direct_uploads_controller_test.rb @@ -17,7 +17,7 @@ class ActiveStorage::DirectUploadsControllerTest < ActionDispatch::IntegrationTe post rails_direct_uploads_path, params: @blob_params, - headers: bearer_token_header(identity_access_tokens(:davids_api_token).token), + headers: bearer_token_header(identity_access_tokens(:davids_api_token).raw_token), as: :json assert_response :success @@ -27,7 +27,7 @@ class ActiveStorage::DirectUploadsControllerTest < ActionDispatch::IntegrationTe test "create with valid access token" do post rails_direct_uploads_path, params: @blob_params, - headers: bearer_token_header(identity_access_tokens(:davids_api_token).token), + headers: bearer_token_header(identity_access_tokens(:davids_api_token).raw_token), as: :json assert_response :success @@ -74,7 +74,7 @@ class ActiveStorage::DirectUploadsControllerTest < ActionDispatch::IntegrationTe test "create with read-only access token" do post rails_direct_uploads_path, params: @blob_params, - headers: bearer_token_header(identity_access_tokens(:jasons_api_token).token), + headers: bearer_token_header(identity_access_tokens(:jasons_api_token).raw_token), as: :json assert_response :unauthorized @@ -121,7 +121,7 @@ class ActiveStorage::DirectUploadsControllerTest < ActionDispatch::IntegrationTe test "create with valid access token in another account is forbidden" do post rails_direct_uploads_path(script_name: "/#{ActiveRecord::FixtureSet.identify("initech")}"), params: @blob_params, - headers: bearer_token_header(identity_access_tokens(:davids_api_token).token), + headers: bearer_token_header(identity_access_tokens(:davids_api_token).raw_token), as: :json assert_response :forbidden diff --git a/test/controllers/api_test.rb b/test/controllers/api_test.rb index a10954b9b3..cad9ad75b6 100644 --- a/test/controllers/api_test.rb +++ b/test/controllers/api_test.rb @@ -4,8 +4,8 @@ class ApiTest < ActionDispatch::IntegrationTest include OauthAvailabilityTestHelper setup do - @davids_bearer_token = bearer_token_env(identity_access_tokens(:davids_api_token).token) - @jasons_bearer_token = bearer_token_env(identity_access_tokens(:jasons_api_token).token) + @davids_bearer_token = bearer_token_env(identity_access_tokens(:davids_api_token).raw_token) + @jasons_bearer_token = bearer_token_env(identity_access_tokens(:jasons_api_token).raw_token) end test "authenticate with user credentials" do @@ -155,7 +155,7 @@ class ApiTest < ActionDispatch::IntegrationTest end test "the bearer scheme is matched case-insensitively" do - get boards_path(format: :json), env: { "HTTP_AUTHORIZATION" => "bearer #{identity_access_tokens(:davids_api_token).token}" } + get boards_path(format: :json), env: { "HTTP_AUTHORIZATION" => "bearer #{identity_access_tokens(:davids_api_token).raw_token}" } assert_response :success get boards_path(format: :json), env: { "HTTP_AUTHORIZATION" => "BEARER nonsense" } @@ -164,7 +164,7 @@ class ApiTest < ActionDispatch::IntegrationTest end test "Bearer elsewhere in another scheme's credentials is not bearer authentication" do - get boards_path(format: :json), env: { "HTTP_AUTHORIZATION" => "Basic Bearer #{identity_access_tokens(:davids_api_token).token}" } + get boards_path(format: :json), env: { "HTTP_AUTHORIZATION" => "Basic Bearer #{identity_access_tokens(:davids_api_token).raw_token}" } assert_response :redirect end diff --git a/test/controllers/my/access_tokens_controller_test.rb b/test/controllers/my/access_tokens_controller_test.rb index 02764bf2b2..231f09890b 100644 --- a/test/controllers/my/access_tokens_controller_test.rb +++ b/test/controllers/my/access_tokens_controller_test.rb @@ -77,7 +77,7 @@ class My::AccessTokensControllerTest < ActionDispatch::IntegrationTest test "create new token via JSON with bearer token" do sign_out - bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" } + bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).raw_token}" } assert_difference -> { identities(:david).access_tokens.count }, +1 do post my_access_tokens_path, params: { access_token: { description: "Fizzy CLI", permission: "read" } }, env: bearer_token, as: :json @@ -113,7 +113,7 @@ class My::AccessTokensControllerTest < ActionDispatch::IntegrationTest test "cannot create new token via JSON with read-only bearer token" do sign_out - bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).token}" } + bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).raw_token}" } assert_no_difference -> { identities(:jason).access_tokens.count } do post my_access_tokens_path, params: { access_token: { description: "Fizzy CLI", permission: "read" } }, env: bearer_token, as: :json @@ -131,7 +131,7 @@ class My::AccessTokensControllerTest < ActionDispatch::IntegrationTest test "index as JSON with bearer token and no account scope" do sign_out - bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" } + bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).raw_token}" } untenanted do get my_access_tokens_path, as: :json, env: bearer_token diff --git a/test/controllers/search_rate_limiting_test.rb b/test/controllers/search_rate_limiting_test.rb index c7ca969668..7e64f14efa 100644 --- a/test/controllers/search_rate_limiting_test.rb +++ b/test/controllers/search_rate_limiting_test.rb @@ -82,6 +82,6 @@ class SearchRateLimitingTest < ActionDispatch::IntegrationTest private def bearer_token_header(fixture) - { "Authorization" => "Bearer #{identity_access_tokens(fixture).token}" } + { "Authorization" => "Bearer #{identity_access_tokens(fixture).raw_token}" } end end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 4d993ea887..e69d0d43f3 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -212,7 +212,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest sign_in_as :jason sign_out - bearer = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).token}" } + bearer = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).raw_token}" } get user_path(users(:jason)), env: bearer assert_response :unauthorized @@ -222,7 +222,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest sign_in_as :jason sign_out - bearer = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).token}" } + bearer = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:jasons_api_token).raw_token}" } get user_path(users(:jason)), env: bearer, as: :json assert_response :success diff --git a/test/fixtures/identity/access_tokens.yml b/test/fixtures/identity/access_tokens.yml index 2c7cf7aa0d..3ce049941b 100644 --- a/test/fixtures/identity/access_tokens.yml +++ b/test/fixtures/identity/access_tokens.yml @@ -1,13 +1,11 @@ jasons_api_token: identity: jason - token: 018cf1425682700098f24f0799e3fe20 - token_digest: <%= TokenDigester.digest("018cf1425682700098f24f0799e3fe20") %> + token_digest: <%= TokenDigester.digest(AccessTokenTestHelper::RAW_FIXTURE_TOKENS.fetch(:jasons_api_token)) %> description: My Superscript permission: read davids_api_token: identity: david - token: x18cf1425682700098f24f0799e3fe20 - token_digest: <%= TokenDigester.digest("x18cf1425682700098f24f0799e3fe20") %> + token_digest: <%= TokenDigester.digest(AccessTokenTestHelper::RAW_FIXTURE_TOKENS.fetch(:davids_api_token)) %> description: My Superscript permission: write diff --git a/test/integration/active_storage_authorization_test.rb b/test/integration/active_storage_authorization_test.rb index 5fa4a87912..1da4adac3b 100644 --- a/test/integration/active_storage_authorization_test.rb +++ b/test/integration/active_storage_authorization_test.rb @@ -18,7 +18,7 @@ class ActiveStorageAuthorizationTest < ActionDispatch::IntegrationTest end test "bearer token with board access can view blob" do - bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" } + bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).raw_token}" } get rails_blob_path(@blob, disposition: :inline), env: bearer_token @@ -48,7 +48,7 @@ class ActiveStorageAuthorizationTest < ActionDispatch::IntegrationTest end test "bearer token with board access can view representation" do - bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" } + bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).raw_token}" } get rails_representation_path(@blob.representation(resize_to_limit: [ 100, 100 ])), env: bearer_token @@ -247,7 +247,7 @@ class ActiveStorageAuthorizationTest < ActionDispatch::IntegrationTest test "export owner can download their export with bearer token" do blob = create_export_blob_for(users(:david)) - bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" } + bearer_token = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).raw_token}" } get rails_blob_path(blob, disposition: :attachment), env: bearer_token diff --git a/test/integration/oauth_availability_test.rb b/test/integration/oauth_availability_test.rb index e77b1ff009..4226f2e376 100644 --- a/test/integration/oauth_availability_test.rb +++ b/test/integration/oauth_availability_test.rb @@ -252,7 +252,7 @@ class OauthAvailabilityTest < ActionDispatch::IntegrationTest test "personal access tokens still work while OAuth is dark" do with_oauth_availability acceptance: false do - get user_path(users(:jason)), env: bearer(identity_access_tokens(:jasons_api_token).token), as: :json + get user_path(users(:jason)), env: bearer(identity_access_tokens(:jasons_api_token).raw_token), as: :json end assert_response :success diff --git a/test/integration/oauth_flow_test.rb b/test/integration/oauth_flow_test.rb index faff4eadbf..ad9fadafd7 100644 --- a/test/integration/oauth_flow_test.rb +++ b/test/integration/oauth_flow_test.rb @@ -1184,7 +1184,7 @@ class OauthFlowTest < ActionDispatch::IntegrationTest end test "a personal access token issued before prefixes still authenticates" do - get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" }, as: :json + get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).raw_token}" }, as: :json assert_response :success end diff --git a/test/integration/oauth_grant_reach_test.rb b/test/integration/oauth_grant_reach_test.rb index e301c68d99..d026d83fa8 100644 --- a/test/integration/oauth_grant_reach_test.rb +++ b/test/integration/oauth_grant_reach_test.rb @@ -118,7 +118,7 @@ class OauthGrantReachTest < ActionDispatch::IntegrationTest end test "a personal access token still manages personal access tokens" do - pat = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" } + pat = { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).raw_token}" } get my_access_tokens_path, env: pat, as: :json diff --git a/test/integration/oauth_revocation_test.rb b/test/integration/oauth_revocation_test.rb index a73aba60fa..ab25f91bf5 100644 --- a/test/integration/oauth_revocation_test.rb +++ b/test/integration/oauth_revocation_test.rb @@ -129,7 +129,7 @@ class OauthRevocationTest < ActionDispatch::IntegrationTest end test "tokens issued before prefixes revoke as before" do - personal = identities(:david).access_tokens.create!(permission: :read, token: "legacyPersonalToken0123456789") + personal = identities(:david).access_tokens.create!(permission: :read, raw_token: "legacyPersonalToken0123456789") grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client), raw_token: "legacyAccessToken0123456789", raw_refresh_token: "legacyRefreshToken0123456789") diff --git a/test/migrations/backfill_remaining_token_digests_test.rb b/test/migrations/backfill_remaining_token_digests_test.rb new file mode 100644 index 0000000000..31d516ed27 --- /dev/null +++ b/test/migrations/backfill_remaining_token_digests_test.rb @@ -0,0 +1,103 @@ +require "test_helper" +require Rails.root.join("db/migrate/20261017120000_backfill_remaining_token_digests") + +class BackfillRemainingTokenDigestsTest < ActiveSupport::TestCase + # Production code that stores only plaintext kept writing personal access + # tokens after the first backfill, and one not used since has no digest. + setup do + @personal = identities(:david).access_tokens.create!(description: "Before digests") + store_plaintext_only @personal, "legacyPersonalToken0123456789", updated_at: 3.days.ago.round + + @grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) + store_plaintext_only @grant, "legacyAccessToken0123456789", updated_at: 80.days.ago.round + end + + test "digests every undigested token, keeping a personal token's plaintext and clearing a grant's" do + backfill + + @personal.reload + assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), @personal.token_digest + assert_equal "legacyPersonalToken0123456789", stored_plaintext(@personal) + + @grant.reload + assert_equal TokenDigester.digest("legacyAccessToken0123456789"), @grant.token_digest + assert_nil stored_plaintext(@grant) + end + + test "keeps the plaintext of a token issued by the personal access client" do + store_plaintext_only @grant, "legacyAccessToken0123456789", oauth_client_id: personal_access_client.id + + backfill + + assert_equal TokenDigester.digest("legacyAccessToken0123456789"), @grant.reload.token_digest + assert_equal "legacyAccessToken0123456789", stored_plaintext(@grant) + end + + # A grant's updated_at is its idle-lapse clock. + test "leaves updated_at alone" do + assert_no_changes -> { [ @personal.reload.updated_at, @grant.reload.updated_at ] } do + backfill + end + end + + test "leaves digested tokens alone, so it can run again" do + digested = identity_access_tokens(:davids_api_token) + + assert_no_changes -> { digested.reload.attributes } do + backfill + backfill + end + end + + test "the backfilled token authenticates by digest" do + backfill + + assert_equal @personal, Identity::AccessToken.find_honored("legacyPersonalToken0123456789") + assert_equal @grant, Identity::AccessToken.find_by_raw_token("legacyAccessToken0123456789") + end + + # The first backfill kept personal access tokens' plaintext beside their + # digests, so a later token can duplicate one already digested. + test "refuses duplicate tokens before digesting anything, without naming them" do + copy = identities(:kevin).access_tokens.create!(description: "Digested", raw_token: "legacyPersonalToken0123456789") + BackfillRemainingTokenDigests::AccessToken.where(id: copy.id).update_all token: "legacyPersonalToken0123456789" + + error = assert_raises(RuntimeError) { backfill } + + assert_match @personal.id, error.message + assert_match copy.id, error.message + assert_no_match(/legacyPersonalToken/, error.message) + assert_nil @personal.reload.token_digest + assert_nil @grant.reload.token_digest + end + + test "does nothing once the plaintext column is gone" do + migration = BackfillRemainingTokenDigests.new.tap { |migration| migration.verbose = false } + migration.stubs(:column_exists?).with(:identity_access_tokens, :token).returns(false) + + migration.up + + assert_nil @personal.reload.token_digest + end + + private + # Identity::AccessToken ignores the plaintext column, so it's written and + # read through the migration's own model. + def store_plaintext_only(access_token, raw, **attributes) + BackfillRemainingTokenDigests::AccessToken.where(id: access_token.id).update_all token: raw, token_digest: nil, **attributes + end + + # The personal access client may not exist yet, so the grant's client stands in for it. + def personal_access_client + Oauth::Client.find_by(client_id: BackfillRemainingTokenDigests::PERSONAL_ACCESS_CLIENT_ID) || + oauth_clients(:mcp_client).tap { |client| client.update_columns client_id: BackfillRemainingTokenDigests::PERSONAL_ACCESS_CLIENT_ID } + end + + def stored_plaintext(access_token) + BackfillRemainingTokenDigests::AccessToken.find(access_token.id).token + end + + def backfill + BackfillRemainingTokenDigests.new.tap { |migration| migration.verbose = false }.up + end +end diff --git a/test/migrations/backfill_token_digests_test.rb b/test/migrations/backfill_token_digests_test.rb index f2d83874b1..41f3b2504b 100644 --- a/test/migrations/backfill_token_digests_test.rb +++ b/test/migrations/backfill_token_digests_test.rb @@ -4,10 +4,10 @@ class BackfillTokenDigestsTest < ActiveSupport::TestCase setup do @personal = identities(:david).access_tokens.create!(description: "Before digests") - @personal.update_columns token: "legacyPersonalToken0123456789", token_digest: nil, updated_at: 3.days.ago.round + store_plaintext_only @personal, "legacyPersonalToken0123456789", updated_at: 3.days.ago.round @grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) - @grant.update_columns token: "legacyAccessToken0123456789", token_digest: nil, updated_at: 80.days.ago.round + store_plaintext_only @grant, "legacyAccessToken0123456789", updated_at: 80.days.ago.round end test "digests every undigested token, keeping a personal token's plaintext and clearing a grant's" do @@ -15,11 +15,11 @@ class BackfillTokenDigestsTest < ActiveSupport::TestCase @personal.reload assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), @personal.token_digest - assert_equal "legacyPersonalToken0123456789", @personal.token + assert_equal "legacyPersonalToken0123456789", stored_plaintext(@personal) @grant.reload assert_equal TokenDigester.digest("legacyAccessToken0123456789"), @grant.token_digest - assert_nil @grant.token + assert_nil stored_plaintext(@grant) end # A grant's updated_at is its idle-lapse clock. @@ -57,7 +57,7 @@ class BackfillTokenDigestsTest < ActiveSupport::TestCase test "refuses duplicate tokens before digesting anything, without naming them" do copy = identities(:kevin).access_tokens.create!(description: "Copy") - copy.update_columns token: "legacyPersonalToken0123456789", token_digest: nil + store_plaintext_only copy, "legacyPersonalToken0123456789" error = assert_raises(RuntimeError) { backfill } @@ -69,6 +69,16 @@ class BackfillTokenDigestsTest < ActiveSupport::TestCase end private + # Identity::AccessToken ignores the plaintext column, so it's written and + # read through the migration's own model. + def store_plaintext_only(access_token, raw, **attributes) + BackfillTokenDigests::AccessToken.where(id: access_token.id).update_all token: raw, token_digest: nil, **attributes + end + + def stored_plaintext(access_token) + BackfillTokenDigests::AccessToken.find(access_token.id).token + end + def backfill BackfillTokenDigests.new.tap { |migration| migration.verbose = false }.up end diff --git a/test/models/identity/access_token_test.rb b/test/models/identity/access_token_test.rb index 1bf37bf66b..9582012a85 100644 --- a/test/models/identity/access_token_test.rb +++ b/test/models/identity/access_token_test.rb @@ -90,7 +90,7 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase test "find_honored returns a read-only token and leaves the method check to the caller" do token = identity_access_tokens(:jasons_api_token) - assert_equal token, Identity::AccessToken.find_honored(token.token) + assert_equal token, Identity::AccessToken.find_honored(token.raw_token) assert_not token.allows?("POST") end @@ -112,11 +112,14 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase end end - test "a personal access token is stored digested and, for now, in plaintext" do - token = identities(:david).access_tokens.create!(description: "Personal").reload + test "a personal access token stores only its digest" do + token = identities(:david).access_tokens.create!(description: "Personal") + raw_token = token.raw_token + token.reload - assert_equal TokenDigester.digest(token.token), token.token_digest + assert_equal TokenDigester.digest(raw_token), token.token_digest assert_nil token.refresh_token_digest + assert_nil stored_plaintext(token) end test "an OAuth grant stores only digests of its tokens" do @@ -124,7 +127,7 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase raw_token, raw_refresh_token = grant.raw_token, grant.raw_refresh_token grant.reload - assert_nil grant.token + assert_nil stored_plaintext(grant) assert_equal TokenDigester.digest(raw_token), grant.token_digest assert_equal TokenDigester.digest(raw_refresh_token), grant.refresh_token_digest assert_nil grant.raw_token @@ -136,7 +139,7 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase raw_token, raw_refresh_token = grant.raw_token, grant.raw_refresh_token grant.reload - assert_nil grant.token + assert_nil stored_plaintext(grant) assert_equal TokenDigester.digest(raw_token), grant.token_digest assert_equal TokenDigester.digest(raw_refresh_token), grant.refresh_token_digest end @@ -163,79 +166,22 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase assert statements.none? { |sql| sql.match?(/\Wtoken\W*=/) }, statements.join("\n") end - # Production code that predates digests writes personal access tokens in - # plaintext only. Such a token still authenticates, and gets its digest the - # first time it's used, without moving its updated_at. - test "a personal access token without a digest is found by plaintext and digested" do - token = undigested_personal_token("legacyPersonalToken0123456789") - updated_at = token.updated_at + # BackfillRemainingTokenDigests digests any token stored only in plaintext + # before this code reads by digest alone. + test "a token stored only in plaintext doesn't authenticate" do + token = identities(:david).access_tokens.create!(description: "Before digests") + Identity::AccessToken.where(id: token.id).update_all token: "legacyPersonalToken0123456789", token_digest: nil - travel 1.day do - assert_equal token, Identity::AccessToken.find_honored("legacyPersonalToken0123456789") + statements = capture_sql do + assert_nil Identity::AccessToken.find_honored("legacyPersonalToken0123456789") end - - token.reload - assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), token.token_digest - assert_equal updated_at, token.updated_at - - statements = capture_sql { Identity::AccessToken.find_honored("legacyPersonalToken0123456789") } assert statements.none? { |sql| sql.match?(/\Wtoken\W*=/) }, statements.join("\n") - end - - # SaaS reads GET requests from the replica, where writes are refused. - test "a personal access token without a digest is digested when found from the replica" do - token = undigested_personal_token("legacyPersonalToken0123456789") - - found = ApplicationRecord.with_reading_role do - Identity::AccessToken.find_honored("legacyPersonalToken0123456789") - end - - assert_equal token, found - assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), token.reload.token_digest - end - - # Non-primary datacenters read local replicas and can't reach the primary. - test "a personal access token without a digest authenticates outside the primary datacenter, undigested" do - token = undigested_personal_token("legacyPersonalToken0123456789") - Deployment::DatabaseResolver.stubs(:in_primary_datacenter?).returns(false) - - statements = capture_sql { assert_equal token, Identity::AccessToken.find_honored("legacyPersonalToken0123456789") } - - assert statements.none? { |sql| sql.match?(/\AUPDATE/i) }, statements.join("\n") assert_nil token.reload.token_digest end - # Code that stores only plaintext issues prefixed personal access tokens too. - test "a prefixed personal access token without a digest is found by plaintext and digested" do - token = undigested_personal_token("fizzy_pat_#{"x" * 24}") - - assert_equal token, Identity::AccessToken.find_honored("fizzy_pat_#{"x" * 24}") - assert_equal TokenDigester.digest("fizzy_pat_#{"x" * 24}"), token.reload.token_digest - end - - # MySQL's collation matches `token` case-insensitively. - test "a differently cased token neither authenticates nor digests the one it resembles" do - token = undigested_personal_token("legacyPersonalToken0123456789") - - assert_nil Identity::AccessToken.find_honored("LEGACYPERSONALTOKEN0123456789") - assert_nil token.reload.token_digest - - assert_equal token, Identity::AccessToken.find_honored("legacyPersonalToken0123456789") - assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), token.reload.token_digest - end - - test "only personal access tokens fall back to plaintext" do - grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) - grant.update_columns token: "legacyAccessToken0123456789", token_digest: nil - - assert_nil Identity::AccessToken.find_by_raw_token("legacyAccessToken0123456789") - end - - test "the fallback stays within the scope it's called on" do - undigested_personal_token("legacyPersonalToken0123456789") - - assert_nil Identity::AccessToken.oauth.find_by_raw_token("legacyPersonalToken0123456789") - assert_nil Identity::AccessToken.where(identity: identities(:kevin)).find_by_raw_token("legacyPersonalToken0123456789") + test "the plaintext column is ignored" do + assert_not_includes Identity::AccessToken.column_names, "token" + assert_not Identity::AccessToken.new.respond_to?(:token) end # Dropped beside `token` only once every process runs code that ignores it. @@ -258,10 +204,8 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase end private - def undigested_personal_token(raw) - identities(:david).access_tokens.create!(description: "Before digests").tap do |token| - token.update_columns token: raw, token_digest: nil - end + def stored_plaintext(access_token) + Identity::AccessToken.where(id: access_token.id).pick(Arel.sql("token")) end def capture_sql(&block) diff --git a/test/test_helper.rb b/test/test_helper.rb index 4161cc5ebb..47ce5c6bdb 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -50,7 +50,7 @@ class TestCase fixtures :all include ActiveJob::TestHelper - include ActionTextTestHelper, CachingTestHelper, CardTestHelper, ChangeTestHelper, DnsTestHelper, + include AccessTokenTestHelper, ActionTextTestHelper, CachingTestHelper, CardTestHelper, ChangeTestHelper, DnsTestHelper, SearchIndexTestHelper, SessionTestHelper include Turbo::Broadcastable::TestHelper diff --git a/test/test_helpers/access_token_test_helper.rb b/test/test_helpers/access_token_test_helper.rb new file mode 100644 index 0000000000..1ebcd44d9a --- /dev/null +++ b/test/test_helpers/access_token_test_helper.rb @@ -0,0 +1,21 @@ +module AccessTokenTestHelper + # Fixture tokens are stored only as digests, as every token is. These are + # the raw values test/fixtures/identity/access_tokens.yml digests. + RAW_FIXTURE_TOKENS = { + jasons_api_token: "018cf1425682700098f24f0799e3fe20", + davids_api_token: "x18cf1425682700098f24f0799e3fe20" + } + + # Each fixture token carries its raw value in memory, as a token does on the + # record that issued it, so a test can present it as raw_token. + def identity_access_tokens(*) + super.tap do |found| + Array(found).each { |access_token| access_token.raw_token ||= raw_fixture_tokens_by_digest[access_token.token_digest] } + end + end + + private + def raw_fixture_tokens_by_digest + RAW_FIXTURE_TOKENS.values.index_by { |raw| TokenDigester.digest(raw) } + end +end