From d64cfa3b7815a84e21c6fe5a6204011b50b2785e Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Sun, 25 Oct 2026 23:33:44 +0000 Subject: [PATCH 1/7] Release the one-time token in transactional tests Since 0.6.0, TokenSession refuses a retrieval inside any open transaction, because a rollback could undo the token_shown_at claim after the token was shown. Rails' transactional tests run each test inside one transaction. So every test that created a key and read its token got nil, and the dashboard's token page redirected. When Rails.env is test and the only open transaction is an outer, non-joinable one, the claim now runs in it. Rails' transactional tests, rspec-rails and DatabaseCleaner's :transaction strategy open that transaction. The first retrieval returns the token. A second one, or one from a copy of the session, does not. A transaction opened on top of it is still refused, as in production. Outside the test environment nothing changes. The new tests cover the helper and the dashboard under use_transactional_tests = true, and the claim under a real commit, read from a second connection. Co-Authored-By: Claude Opus 5.5 --- lib/api_keys/helpers/token_session.rb | 30 +++- test/controllers/keys_controller_test.rb | 52 +++++- .../token_session_transactional_test.rb | 163 ++++++++++++++++++ 3 files changed, 240 insertions(+), 5 deletions(-) create mode 100644 test/helpers/token_session_transactional_test.rb diff --git a/lib/api_keys/helpers/token_session.rb b/lib/api_keys/helpers/token_session.rb index ab8a124..a0ca5b6 100644 --- a/lib/api_keys/helpers/token_session.rb +++ b/lib/api_keys/helpers/token_session.rb @@ -43,6 +43,12 @@ module Helpers # opened with raw SQL (`BEGIN`) is not detected. The dashboard opens no # transaction itself; a host `around_action` that wraps its requests in # one sees the refusal. + # + # The transaction that a transactional test opens around each test does + # not count (0.8.1): when Rails.env is test and the only open + # transaction is an outer, non-joinable one, the claim runs in it. The + # first retrieval in the test returns the token and a second one does + # not, as in production. A transaction opened on top of it is refused. # 2. Cache marker (0.5.1): each handoff has a random ID, and retrieval # records it in `Rails.cache` with `write(..., unless_exist: true)`; # a later retrieval of the same handoff finds the marker and gets nil. @@ -94,7 +100,8 @@ class TokenSession # transaction on the ApiKey connection, where its one-time claim could # be rolled back after the token was shown. The token is withheld, # nothing is recorded, and the handoff stays in the session for a - # retry outside the transaction. + # retry outside the transaction. The transaction of a transactional + # test does not count; one opened on top of it does. # - `:missing`: no usable handoff for this key (none stored, expired, # tampered, or bound to another key). # @@ -303,7 +310,26 @@ def claim_could_roll_back?(stored_id) return false if stored_id.nil? || !defined?(::ApiKeys::ApiKey) return false unless ::ApiKeys::ApiKey.column_names.include?(::ApiKeys::ApiKey::TOKEN_SHOWN_AT_COLUMN) - ::ApiKeys::ApiKey.connection_pool.with_connection(&:transaction_open?) + ::ApiKeys::ApiKey.connection_pool.with_connection do |connection| + connection.transaction_open? && !only_test_transaction_open?(connection) + end + end + + # Whether the only open transaction on +connection+ is the one a + # transactional test opens around each test. Rails' transactional + # tests (and rspec-rails, and DatabaseCleaner's :transaction strategy) + # open it as one outer, non-joinable transaction, and a system test's + # server thread shares it. A claim in it is as durable as every other + # write of the test: the test sees it, a second retrieval in the same + # test is refused, and the rollback at the end of the test removes the + # key with it. A transaction that the app or the test opens on top of + # it still refuses the retrieval, as in production. Only when + # Rails.env is test. + def only_test_transaction_open?(connection) + return false unless defined?(Rails) && Rails.respond_to?(:env) && Rails.env.test? + return false unless connection.open_transactions == 1 + + !connection.current_transaction.joinable? end # Inside a transaction the claim is not durable until the host commits, diff --git a/test/controllers/keys_controller_test.rb b/test/controllers/keys_controller_test.rb index 648527d..7af18d1 100644 --- a/test/controllers/keys_controller_test.rb +++ b/test/controllers/keys_controller_test.rb @@ -29,9 +29,9 @@ def default_url_options ApiKeys::ApplicationController.helper_method(:default_url_options) module ApiKeys - class KeysControllerTest < ActionController::TestCase - tests ApiKeys::KeysController - + # The dashboard's test setup, for KeysControllerTest and for the + # transactional test at the end of this file. + module KeysControllerTestSetup def setup super ApiKeys.reset_configuration! @@ -51,6 +51,11 @@ def setup @controller.prepend_view_path File.expand_path("../../app/views", __dir__) @controller.singleton_class.include(ApiKeys::Engine.routes.url_helpers) end + end + + class KeysControllerTest < ActionController::TestCase + include KeysControllerTestSetup + tests ApiKeys::KeysController test "dashboard responses prohibit caching, MIME sniffing, and referrers" do get :index @@ -1216,3 +1221,44 @@ def emitted_content_security_policy end end end + +module ApiKeys + # A host app's dashboard test, in a transactional test (0.8.1): the token + # page shows a new key's token once, and a reload or a copied session gets + # the "already shown" answer, as in production. + class KeysControllerInTransactionalTestsTest < ActionController::TestCase + include ActiveRecord::TestFixtures + include KeysControllerTestSetup + self.use_transactional_tests = true + tests ApiKeys::KeysController + + test "the token page shows a new key's token once" do + ApiKeys::ApiKey.connection_pool.with_connection do |connection| + assert_equal 1, connection.open_transactions, "precondition: the test's transaction is open" + end + post :create, params: { api_key: { name: "Transactional dashboard key", scopes: %w[read] } } + key = @user.api_keys.order(:created_at).last + assert_redirected_to key_path(key) + copied_cookie = @request.session[ApiKeys::TokenSession::DEFAULT_SESSION_KEY].deep_dup + + get :show, params: { id: key.id } + + assert_response :success + token = @controller.instance_variable_get(:@plaintext_token) + assert ApiKeys::Services::Digestor.match?(token: token, stored_digest: key.token_digest, + strategy: key.digest_algorithm.to_sym) + assert_includes response.body, token + assert key.reload.token_shown? + + get :show, params: { id: key.id } + assert_redirected_to keys_path + assert_equal KeysController::ALREADY_SHOWN_ALERT, flash[:alert], "a reload gets nothing" + + @request.session[ApiKeys::TokenSession::DEFAULT_SESSION_KEY] = copied_cookie + get :show, params: { id: key.id } + assert_redirected_to keys_path + assert_equal KeysController::ALREADY_SHOWN_ALERT, flash[:alert], "a copied session gets nothing" + assert_nil @controller.instance_variable_get(:@plaintext_token) + end + end +end diff --git a/test/helpers/token_session_transactional_test.rb b/test/helpers/token_session_transactional_test.rb new file mode 100644 index 0000000..d936535 --- /dev/null +++ b/test/helpers/token_session_transactional_test.rb @@ -0,0 +1,163 @@ +# frozen_string_literal: true + +require "test_helper" +require "active_record/fixtures" +require "tmpdir" + +module ApiKeys + module Helpers + # The one-time token handoff in a host app's tests. From 0.6.0 to 0.8.0 a + # retrieval inside any open transaction was refused, and a transactional + # test is always inside one, so a test that created a key and read its + # token got nil. Since 0.8.1 the test's own transaction does not count. + class TokenSessionInTransactionalTestsTest < ApiKeys::Test + include ActiveRecord::TestFixtures + self.use_transactional_tests = true + + def setup + super + # No cache marker: only the durable claim decides. + Rails.stubs(:cache).returns(ActiveSupport::Cache::NullStore.new) + TokenSession.stubs(:log_warn) + @key = User.create!(name: "Transactional owner").create_api_key!(name: "Transactional key") + @token = @key.token # reload forgets it + @session = {} + TokenSession.store(@session, @key) + @copied_cookie = @session.deep_dup + end + + def assert_only_the_test_transaction_is_open + ApiKeys::ApiKey.connection_pool.with_connection do |connection| + assert_equal 1, connection.open_transactions, "precondition: the test's transaction is open" + refute connection.current_transaction.joinable?, "precondition: it is the test's non-joinable transaction" + end + end + + test "the first retrieval returns the token and the second does not, as in production" do + assert_only_the_test_transaction_is_open + + first = TokenSession.retrieve_once_with_status(@session, api_key: @key) + replay = TokenSession.retrieve_once_with_status(@copied_cookie, api_key: @key) + refresh = TokenSession.retrieve_once_with_status(@session, api_key: @key) + + assert_equal :retrieved, first.status + assert_equal @token, first.token + assert @key.reload.token_shown?, "the claim is recorded in the test's transaction" + assert_equal :already_shown, replay.status, "a copied session gets nothing" + assert_nil replay.token + assert_equal :missing, refresh.status, "a refresh gets nothing" + assert_nil refresh.token + end + + test "retrieve_once returns the token once" do + assert_equal @token, TokenSession.retrieve_once(@session, api_key: @key) + assert_nil TokenSession.retrieve_once(@copied_cookie, api_key: @key) + end + + test "a transaction opened on top of the test's transaction still refuses the retrieval" do + { + "ApiKey.transaction" => ->(&work) { ApiKeys::ApiKey.transaction(&work) }, + "a host model's transaction" => ->(&work) { User.transaction(&work) }, + "a savepoint (requires_new)" => ->(&work) { ApiKeys::ApiKey.transaction(requires_new: true, &work) }, + "a non-joinable transaction" => ->(&work) { ApiKeys::ApiKey.transaction(joinable: false, &work) } + }.each do |label, wrap| + retrieval = nil + wrap.call { retrieval = TokenSession.retrieve_once_with_status(@session, api_key: @key) } + + assert_equal :inside_transaction, retrieval.status, label + assert_nil retrieval.token, label + refute @key.reload.token_shown?, "#{label}: nothing is recorded" + end + + assert_equal @token, TokenSession.retrieve_once(@session, api_key: @key), "the handoff was kept" + assert_nil TokenSession.retrieve_once(@copied_cookie, api_key: @key) + end + + test "outside the test environment the same transaction refuses the retrieval" do + Rails.stubs(:env).returns(ActiveSupport::EnvironmentInquirer.new("production")) + + retrieval = TokenSession.retrieve_once_with_status(@session, api_key: @key) + + assert_equal :inside_transaction, retrieval.status + assert_nil retrieval.token + refute @key.reload.token_shown? + end + end + + # The same handoff with real commits: no transaction is open, so the claim + # commits on its own before the token is returned. Another connection + # sees it, and a copy of the session gets nothing. + class TokenSessionWithRealCommitsTest < ApiKeys::Test + include ActiveRecord::TestFixtures + self.use_transactional_tests = false + + def setup + super + Rails.stubs(:cache).returns(ActiveSupport::Cache::NullStore.new) + end + + # Points ApiKeys::ApiKey at a file-backed SQLite database for the block, + # so a second connection reads what the first one committed. (The + # shared in-memory test database is private to one connection.) + def with_file_database + Dir.mktmpdir("api_keys_commit") do |dir| + ApiKeys::ApiKey.establish_connection(adapter: "sqlite3", database: File.join(dir, "commit.sqlite3"), pool: 4) + ApiKeys::ApiKey.connection_pool.with_connection do |connection| + connection.create_table :api_keys do |t| + t.string :prefix, null: false + t.string :token_digest, null: false + t.string :digest_algorithm, null: false + t.string :name + t.references :owner, polymorphic: true, null: true + t.references :created_by, polymorphic: true, null: true + t.text :scopes, default: "[]", null: false + t.text :metadata, default: "{}", null: false + t.text :restrictions, default: "{}", null: false + t.datetime :expires_at + t.datetime :last_used_at + t.bigint :requests_count, default: 0, null: false + t.datetime :token_shown_at + t.datetime :revoked_at + t.string :last4, null: false, default: "" + t.string :key_type + t.string :environment + t.timestamps + end + end + ApiKeys::ApiKey.reset_column_information + yield + ensure + ApiKeys::ApiKey.remove_connection + ApiKeys::ApiKey.reset_column_information + end + end + + test "the claim commits before the token is returned, and a copy of the session gets nothing" do + with_file_database do + key = User.create!(name: "Committing owner").create_api_key!(name: "Committed key") + session = {} + TokenSession.store(session, key) + copied_cookie = session.deep_dup + retrieval = shown_at = nil + + # This thread keeps its connection, so the reader gets another one. + ApiKeys::ApiKey.connection_pool.with_connection do |retriever| + refute retriever.transaction_open?, "precondition: no transaction is open" + retrieval = TokenSession.retrieve_once_with_status(session, api_key: key) + shown_at = Thread.new do + ApiKeys::ApiKey.connection_pool.with_connection do |reader| + refute_equal retriever, reader, "precondition: the claim is read on another connection" + reader.select_value("SELECT token_shown_at FROM api_keys WHERE id = #{Integer(key.id)}") + end + end.value + end + + assert_equal :retrieved, retrieval.status + assert_equal key.token, retrieval.token + assert shown_at, "the claim is committed: another connection sees it" + assert_nil TokenSession.retrieve_once(copied_cookie, api_key: key) + end + end + end + end +end From 8157c4c05be2feed9e9bf468796c74abd6b77d44 Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Sun, 25 Oct 2026 23:45:41 +0000 Subject: [PATCH 2/7] Build the add_created_by index concurrently on PostgreSQL The 0.7.0 add_created_by migration called add_reference with the polymorphic index. That builds the index inside the migration's DDL transaction, which blocks writes to api_keys, so strong_migrations refuses the migration on PostgreSQL. The migration now runs with disable_ddl_transaction!. It adds the columns without an index, then builds index_api_keys_on_created_by with algorithm: :concurrently on PostgreSQL and PostGIS. MySQL and SQLite refuse that option, so there the index is built as usual. Each step checks what exists, so a run that stopped half way can run again. The database check now loads strong_migrations. It starts from an install made before 0.6.0 and runs the four upgrade migrations through MigrationContext, as db:migrate does, so a missing disable_ddl_transaction! fails too. add_key_activity, add_device_authorizations and add_idempotency_keys already pass on PostgreSQL and MySQL and stay unchanged. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/test.yml | 7 +- .../add_created_by_to_api_keys.rb.erb | 28 ++++- test/databases/Gemfile | 1 + test/databases/check.rb | 109 ++++++++++++++---- test/trusted_key_check_test.rb | 36 +++++- 5 files changed, 146 insertions(+), 35 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b814044..dfe4714 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -50,9 +50,10 @@ jobs: path: test/reports/ retention-days: 7 - # The 0.7.0 migrations as the generators write them, and the row locks of - # Idempotency-Key and the device flow, on real databases with concurrent - # connections (test/databases/check.rb). + # The upgrade migrations as the generators write them and db:migrate runs + # them, with strong_migrations, and the row locks of Idempotency-Key and the + # device flow, on real databases with concurrent connections + # (test/databases/check.rb). databases: runs-on: ubuntu-latest timeout-minutes: 15 diff --git a/lib/generators/api_keys/templates/add_created_by_to_api_keys.rb.erb b/lib/generators/api_keys/templates/add_created_by_to_api_keys.rb.erb index 63f206c..4fade61 100644 --- a/lib/generators/api_keys/templates/add_created_by_to_api_keys.rb.erb +++ b/lib/generators/api_keys/templates/add_created_by_to_api_keys.rb.erb @@ -9,13 +9,27 @@ # # Run this migration if you're upgrading from a version of api_keys before # 0.7.0. New installs get the columns from the install generator. Existing -# keys keep NULL. The migration is idempotent: columns that already exist are -# left alone. +# keys keep NULL. +# +# On PostgreSQL the index is built concurrently, so api_keys takes writes +# while it builds, and strong_migrations accepts the migration. A concurrent +# build cannot run inside a transaction, so this migration runs without one. +# Each step checks what exists, so a run that stopped half way can run +# again. If a concurrent build fails, PostgreSQL keeps an invalid index: +# remove it before the next run. MySQL and SQLite build the index as usual. class AddCreatedByToApiKeys < ActiveRecord::Migration<%= migration_version %> + INDEX_NAME = "index_api_keys_on_created_by" + INDEX_COLUMNS = %i[created_by_type created_by_id].freeze + + disable_ddl_transaction! + def up - return if column_exists?(:api_keys, :created_by_id) + unless column_exists?(:api_keys, :created_by_id) + add_reference :api_keys, :created_by, polymorphic: true, null: true, type: foreign_key_type, index: false + end + return if index_exists?(:api_keys, INDEX_COLUMNS) - add_reference :api_keys, :created_by, polymorphic: true, null: true, type: foreign_key_type + add_index :api_keys, INDEX_COLUMNS, name: INDEX_NAME, **index_algorithm end def down @@ -30,4 +44,10 @@ class AddCreatedByToApiKeys < ActiveRecord::Migration<%= migration_version %> generators = Rails.application&.config&.generators generators&.options&.dig(generators.orm, :primary_key_type) || :bigint end + + # Concurrently on PostgreSQL (and PostGIS). MySQL and SQLite have no + # concurrent build and refuse the option. + def index_algorithm + connection.adapter_name.downcase.include?("postg") ? { algorithm: :concurrently } : {} + end end diff --git a/test/databases/Gemfile b/test/databases/Gemfile index 305482e..418d38a 100644 --- a/test/databases/Gemfile +++ b/test/databases/Gemfile @@ -11,3 +11,4 @@ gem "minitest", "~> 5.25" gem "pg" gem "mysql2" gem "sqlite3", ">= 2.1" +gem "strong_migrations" diff --git a/test/databases/check.rb b/test/databases/check.rb index 9720b8c..d8019d6 100644 --- a/test/databases/check.rb +++ b/test/databases/check.rb @@ -1,9 +1,11 @@ # frozen_string_literal: true -# Runs the 0.7.0 migrations as the generators write them, and the row locks -# of ApiKeys::IdempotencyKey and ApiKeys::DeviceAuthorization, against a real -# database with real concurrent connections. The main suite runs on -# in-memory SQLite, which has one connection. +# Runs the upgrade migrations of 0.6.0 and 0.7.0 as the generators write +# them and as db:migrate runs them, with strong_migrations checking each one, +# and the row locks of ApiKeys::IdempotencyKey and +# ApiKeys::DeviceAuthorization, against a real database with real concurrent +# connections. The main suite runs on in-memory SQLite, which has one +# connection. # # BUNDLE_GEMFILE=test/databases/Gemfile DB_ADAPTER=postgresql bundle exec ruby test/databases/check.rb # @@ -18,9 +20,11 @@ require "tmpdir" require "rails" require "active_record/railtie" +require "strong_migrations" require "api_keys" require "rails/generators" require "generators/api_keys/install_generator" +require "generators/api_keys/add_key_activity_generator" require "generators/api_keys/add_created_by_generator" require "generators/api_keys/add_device_authorizations_generator" require "generators/api_keys/add_idempotency_keys_generator" @@ -58,12 +62,16 @@ class DatabaseCheckApp < Rails::Application end ActiveRecord::Base.establish_connection(CONFIG) ActiveRecord::Migration.verbose = false +# strong_migrations does not support SQLite, and says to skip it there. +# There the check shows that the migrations run, as on any database. +StrongMigrations.skip_database(:primary) if ADAPTER == "sqlite3" # Writes each migration with its generator, as an app does. MIGRATIONS = Dir.mktmpdir("api_keys_migrations") [ - ApiKeys::Generators::InstallGenerator, ApiKeys::Generators::AddCreatedByGenerator, - ApiKeys::Generators::AddDeviceAuthorizationsGenerator, ApiKeys::Generators::AddIdempotencyKeysGenerator + ApiKeys::Generators::InstallGenerator, ApiKeys::Generators::AddKeyActivityGenerator, + ApiKeys::Generators::AddCreatedByGenerator, ApiKeys::Generators::AddDeviceAuthorizationsGenerator, + ApiKeys::Generators::AddIdempotencyKeysGenerator ].each do |generator| $stdout = StringIO.new generator.start([], destination_root: MIGRATIONS) @@ -71,8 +79,25 @@ class DatabaseCheckApp < Rails::Application $stdout = STDOUT end +# The upgrade migrations, in the order db:migrate runs them. +UPGRADES = %w[add_key_activity_to_api_keys add_created_by_to_api_keys create_api_keys_device_authorizations + create_api_keys_idempotency_keys].freeze + +def migration_file(name) + Dir["#{MIGRATIONS}/db/migrate/*_#{name}.rb"].sole +end + +# Runs one generated migration as db:migrate does: inside a DDL transaction +# unless the migration disables it, and checked by strong_migrations (which +# checks only the way up, by default). An unsafe migration raises. +def run_migration(name, direction) + file = migration_file(name) + ActiveRecord::MigrationContext.new(File.dirname(file)).run(direction, File.basename(file).to_i) + [ApiKeys::ApiKey, ApiKeys::DeviceAuthorization, ApiKeys::IdempotencyKey].each(&:reset_column_information) +end + def load_migration(name) - source = File.read(Dir["#{MIGRATIONS}/db/migrate/*_#{name}.rb"].sole) + source = File.read(migration_file(name)) # The install migration gives JSON columns literal defaults, which MySQL # refuses. That migration is older than 0.7.0 and has an issue of its own; # here it only makes the api_keys table that the new migrations need. @@ -82,12 +107,14 @@ def load_migration(name) id_type = ENV["UUID"].present? ? :uuid : :primary_key ActiveRecord::Schema.define { create_table(:users, id: id_type) { |t| t.string :name } } -%w[create_api_keys_table add_created_by_to_api_keys create_api_keys_device_authorizations - create_api_keys_idempotency_keys].each { |name| load_migration(name) } +load_migration("create_api_keys_table") CreateApiKeysTable.migrate(:up) -AddCreatedByToApiKeys.migrate(:up) # the install has the columns: nothing to do -CreateApiKeysDeviceAuthorizations.migrate(:up) -CreateApiKeysIdempotencyKeys.migrate(:up) +# An install from before 0.6.0: the api_keys table without the columns that +# the upgrades add. Then every upgrade, as an app runs them. +ActiveRecord::Base.connection.remove_reference(:api_keys, :created_by, polymorphic: true) +ApiKeys::ApiKey::REFUSAL_STATS_COLUMNS.each { |column| ActiveRecord::Base.connection.remove_column(:api_keys, column) } +ActiveRecord::Base.connection.remove_column(:api_keys, ApiKeys::ApiKey::TOKEN_SHOWN_AT_COLUMN) +UPGRADES.each { |name| run_migration(name, :up) } json = ADAPTER == "postgresql" ? :jsonb : :json ApiKeys::ApiKey.attribute :scopes, json, default: [] ApiKeys::ApiKey.attribute :metadata, json, default: {} @@ -128,26 +155,60 @@ def timed [result, Process.clock_gettime(Process::CLOCK_MONOTONIC) - started] end - def test_a_the_new_migrations_run_up_down_and_up_again + # Two migrations that strong_migrations refuses. + MIGRATION_VERSION = ActiveRecord::VERSION::STRING.to_f + + class FourColumnIndex < ActiveRecord::Migration[MIGRATION_VERSION] + def up = add_index(:api_keys, %i[name prefix last4 revoked_at]) + end + + # What the 0.7.0 add_created_by migration did: an index built inside the + # migration's transaction. + class IndexInsideTheTransaction < ActiveRecord::Migration[MIGRATION_VERSION] + def up = add_reference(:api_keys, :reviewer, polymorphic: true, null: true) + end + + def test_0_strong_migrations_checks_the_migrations_here + skip "strong_migrations does not support SQLite" if ADAPTER == "sqlite3" + + error = assert_raises(StrongMigrations::UnsafeMigration) { FourColumnIndex.migrate(:up) } + assert_match "more than three columns", error.message + return unless ADAPTER == "postgresql" + + error = assert_raises(StrongMigrations::UnsafeMigration) { IndexInsideTheTransaction.migrate(:up) } + assert_match "Adding an index non-concurrently", error.message + ApiKeys::ApiKey.reset_column_information + refute_includes ApiKeys::ApiKey.column_names, "reviewer_id", "strong_migrations stops before the statement" + end + + def test_a_the_upgrade_migrations_run_up_down_and_up_again tables = %i[api_keys_device_authorizations api_keys_idempotency_keys] + created_by_index = %i[created_by_type created_by_id] + connection = ActiveRecord::Base.connection Key.delete_all ApiKeys::DeviceAuthorization.delete_all ApiKeys::ApiKey.delete_all - AddCreatedByToApiKeys.migrate(:down) - CreateApiKeysIdempotencyKeys.migrate(:down) - CreateApiKeysDeviceAuthorizations.migrate(:down) - ApiKeys::ApiKey.reset_column_information + UPGRADES.reverse_each { |name| run_migration(name, :down) } refute ApiKeys::ApiKey.created_by_columns? - tables.each { |table| refute ActiveRecord::Base.connection.table_exists?(table) } + refute ApiKeys::ApiKey.token_shown_at_column? + refute ApiKeys::ApiKey.refusal_stats_columns? + tables.each { |table| refute connection.table_exists?(table) } - AddCreatedByToApiKeys.migrate(:up) - AddCreatedByToApiKeys.migrate(:up) # idempotent - CreateApiKeysDeviceAuthorizations.migrate(:up) - CreateApiKeysIdempotencyKeys.migrate(:up) - [ApiKeys::ApiKey, ApiKeys::DeviceAuthorization, Key].each(&:reset_column_information) + UPGRADES.each { |name| run_migration(name, :up) } assert ApiKeys::ApiKey.created_by_columns? - tables.each { |table| assert ActiveRecord::Base.connection.table_exists?(table) } + assert ApiKeys::ApiKey.token_shown_at_column? + assert ApiKeys::ApiKey.refusal_stats_columns? + assert connection.index_exists?(:api_keys, created_by_index, name: "index_api_keys_on_created_by") + tables.each { |table| assert connection.table_exists?(table) } + + # Idempotent: a second run, and a run that stopped after the columns + # (the index was not built), finish the work and add nothing twice. + AddKeyActivityToApiKeys.migrate(:up) + AddCreatedByToApiKeys.migrate(:up) + connection.remove_index(:api_keys, name: "index_api_keys_on_created_by") + AddCreatedByToApiKeys.migrate(:up) + assert_equal 1, connection.indexes(:api_keys).count { |index| index.columns == created_by_index.map(&:to_s) } end def test_b_the_foreign_keys_match_the_key_types diff --git a/test/trusted_key_check_test.rb b/test/trusted_key_check_test.rb index b82aec6..393deb9 100644 --- a/test/trusted_key_check_test.rb +++ b/test/trusted_key_check_test.rb @@ -275,18 +275,40 @@ class AddCreatedByGeneratorTest < Rails::Generators::TestCase destination File.expand_path("../tmp/add_created_by_generator", __dir__) setup :prepare_destination - test "generates one idempotent migration with the polymorphic reference" do + # test/databases/check.rb runs this migration as db:migrate does on + # PostgreSQL, MySQL and SQLite, with strong_migrations. + test "generates one idempotent migration that strong_migrations accepts on PostgreSQL" do run_generator assert_migration "db/migrate/add_created_by_to_api_keys.rb" do |migration| assert_includes migration, "class AddCreatedByToApiKeys < ActiveRecord::Migration[" - assert_includes migration, "return if column_exists?(:api_keys, :created_by_id)" - assert_includes migration, "add_reference :api_keys, :created_by, polymorphic: true, null: true" + assert_includes migration, "disable_ddl_transaction!" + assert_includes migration, "unless column_exists?(:api_keys, :created_by_id)" + assert_includes migration, "add_reference :api_keys, :created_by, polymorphic: true, null: true, " \ + "type: foreign_key_type, index: false" + assert_includes migration, "return if index_exists?(:api_keys, INDEX_COLUMNS)" + assert_includes migration, 'INDEX_NAME = "index_api_keys_on_created_by"' + assert_includes migration, "{ algorithm: :concurrently }" refute_includes migration, "def migration_version" end end - test "the generated migration really adds, re-runs and removes the columns" do + test "the index is built concurrently on PostgreSQL and PostGIS, and as usual elsewhere" do + run_generator + load migration_file_name("db/migrate/add_created_by_to_api_keys.rb") + migration = ::AddCreatedByToApiKeys.new + + { "PostgreSQL" => { algorithm: :concurrently }, "PostGIS" => { algorithm: :concurrently }, + "Mysql2" => {}, "Trilogy" => {}, "SQLite" => {} }.each do |adapter, options| + migration.stubs(:connection).returns(stub(adapter_name: adapter)) + + assert_equal options, migration.send(:index_algorithm), adapter + end + ensure + Object.send(:remove_const, :AddCreatedByToApiKeys) if Object.const_defined?(:AddCreatedByToApiKeys) + end + + test "the generated migration really adds, re-runs and removes the columns and the index" do run_generator load migration_file_name("db/migrate/add_created_by_to_api_keys.rb") migration = ::AddCreatedByToApiKeys.new @@ -299,6 +321,12 @@ class AddCreatedByGeneratorTest < Rails::Generators::TestCase ApiKeys::ApiKey.reset_column_information assert ApiKeys::ApiKey.created_by_columns? assert ApiKeys::ApiKey.columns_hash["created_by_id"].null, "existing keys keep NULL" + assert ActiveRecord::Base.connection.index_exists?(:api_keys, %i[created_by_type created_by_id], + name: "index_api_keys_on_created_by") + + # A run that stopped after the columns builds the index on the next run. + ActiveRecord::Base.connection.remove_index(:api_keys, name: "index_api_keys_on_created_by") + ActiveRecord::Migration.suppress_messages { migration.migrate(:up) } assert ActiveRecord::Base.connection.index_exists?(:api_keys, %i[created_by_type created_by_id]) ActiveRecord::Migration.suppress_messages { migration.migrate(:down) } From b38cfb96654a7118909092edcf1f8e7c4ff88c72 Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Sun, 25 Oct 2026 23:45:41 +0000 Subject: [PATCH 3/7] Bump version to 0.8.1 Record the two fixes in the CHANGELOG and the README: the one-time token in transactional tests, and the strong_migrations-safe add_created_by migration. The README also says how to run the database check. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 14 ++++++++++++++ README.md | 11 ++++++++++- lib/api_keys/version.rb | 2 +- 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c8af60..33fb2a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,17 @@ +## [0.8.1] - 2026-10-25 + +Two fixes found when an app on the fleet upgraded from 0.5 to 0.8.0. + +### Fixed + +- The one-time token is readable in transactional tests again, once, as in production. Since 0.6.0 a retrieval inside any open transaction was refused, and Rails' transactional tests run each test inside one, so a test that created a key and read its token got nil (`:inside_transaction`), and the dashboard's token page redirected. When `Rails.env` is test and the only open transaction is an outer, non-joinable one (what Rails' transactional tests, rspec-rails and DatabaseCleaner's `:transaction` strategy open, and what a system test's server thread shares), the claim of `token_shown_at` runs in it: the first retrieval returns the token, and a second one, or one from a copy of the session, does not. A transaction opened on top of it is still refused. Outside the test environment nothing changes. Apps can remove their test-only workarounds. +- `rails generate api_keys:add_created_by` writes a migration that strong_migrations accepts on PostgreSQL. The 0.7.0 migration built the polymorphic index inside the migration's transaction, which blocks writes to `api_keys`, and strong_migrations refused it. The migration now runs with `disable_ddl_transaction!`, adds the columns, then builds the index `index_api_keys_on_created_by` with `algorithm: :concurrently` on PostgreSQL (and PostGIS); MySQL and SQLite build it as usual. Each step checks what exists, so a run that stopped half way can run again. A migration generated by 0.7.0 or 0.8.0 that already ran needs nothing. + +### Tests + +- The database check (`test/databases/check.rb`, in CI on PostgreSQL, MySQL and SQLite) now runs the four upgrade migrations of 0.6.0 and 0.7.0 as `rails db:migrate` runs them, with strong_migrations, from an install made before 0.6.0. `add_key_activity`, `add_device_authorizations` and `add_idempotency_keys` already passed and are unchanged. +- The token handoff is tested in transactional tests (the helper and the dashboard) and with real commits. + ## [0.8.0] - 2026-10-25 ### Added diff --git a/README.md b/README.md index bfb25c5..ef5fa42 100644 --- a/README.md +++ b/README.md @@ -66,6 +66,8 @@ rails generate api_keys:add_created_by rails db:migrate ``` +On PostgreSQL this migration builds its index concurrently, outside a transaction, so it does not block writes to `api_keys` and strong_migrations accepts it (0.8.1). Each step checks what exists, so a run that stopped half way can run again. If a concurrent build fails, PostgreSQL keeps an invalid index: remove it before the next run. The 0.6.0 and 0.7.0 upgrade migrations (`add_key_activity`, `add_created_by`, `add_device_authorizations`, `add_idempotency_keys`) pass strong_migrations on PostgreSQL and MySQL as generated. + The device flow and Idempotency-Key each need a table of their own. New and existing installations run these only if they use the feature: ```bash @@ -91,7 +93,7 @@ On 0.6.x, nothing changes until you opt in to a 0.7.0 feature. A 0.6.0 job worke On 0.4.x, the calls of a custom key controller keep working unchanged: `create_api_key!(name:, key_type:, expires_at:)`, `ApiKeys::TokenSession.store(session, @api_key)` and `ApiKeys::TokenSession.retrieve_once(session)`. A handoff stored by 0.4.x stays readable after the deploy. Read the [CHANGELOG](CHANGELOG.md) for 0.5.0 to 0.6.0 before you upgrade. These changes are the ones a 0.4.x app sees: - `api_key_scopes_checkboxes` renders an empty `api_key[scopes][]` field before the boxes (0.5.1). A form with every box cleared now saves no scopes, where 0.4.x kept the old ones. If an edit form offers only some scopes, save `ApiKey#scopes_after_form_edit` (see [Form Builder Extensions](#form-builder-extensions-opt-in)). -- A token's one-time handoff is one-time across copies of the session (0.5.1), and once ever per key after `add_key_activity` (0.6.0). A retrieval inside an open database transaction is refused (0.6.0): exclude the token page from a host `around_action` that wraps requests in a transaction. +- A token's one-time handoff is one-time across copies of the session (0.5.1), and once ever per key after `add_key_activity` (0.6.0). A retrieval inside an open database transaction is refused (0.6.0): exclude the token page from a host `around_action` that wraps requests in a transaction. The transaction of a transactional test does not count (0.8.1). - Deploy job workers with the web processes. A refusal job from 0.6.0 or later errors on a 0.4.x worker. ## Quick Start @@ -554,6 +556,7 @@ The database applies it atomically, and the claim commits before the token is re - **With the column, the token is released at most once per key.** A cache flush, a restart, or an eviction changes nothing, and two simultaneous first retrievals are serialized by the database: one wins. `ApiKey#token_shown_at` and `#token_shown?` report it. A key whose row no longer exists gets nothing: zero rows claimed fails closed. - **A retrieval attempted inside a database transaction is refused, and can be retried outside it.** Inside a transaction the claim would commit only with it, after the token had been returned, and a rollback would undo it. No cache can stand in for that (a process-local store protects one process), so whatever the cache, the token is withheld with `:inside_transaction`, nothing is recorded, and the handoff stays in the session for a retrieval outside the transaction. The check is Active Record's transaction manager on the `ApiKey` connection: a transaction opened with raw SQL (`BEGIN`) is not detected, and one on another database's connection does not matter. The dashboard opens no transaction itself, but its controller inherits from your `ApplicationController` (or the configured parent): a host `around_action` that wraps requests in a transaction will see the refusal and must exclude the token page. +- **In a transactional test, the token is released once, as in production (0.8.1).** Rails' transactional tests (also rspec-rails and DatabaseCleaner's `:transaction` strategy) open one outer, non-joinable transaction around each test, and a system test's server thread shares it. When `Rails.env` is test and that is the only open transaction, the claim runs in it: the first retrieval in the test returns the token, and a second one, or one from a copy of the session, does not. The rollback at the end of the test removes the key with its claim. A transaction that the app or the test opens on top of it still refuses the retrieval, as in production, so a test sees a host `around_action` that would break the token page. From 0.6.0 to 0.8.0 every retrieval in a transactional test was refused. - **The column is written only by that claim.** The model marks it read-only, so no form, mass assignment, or `update` can clear it and reopen a handoff. - **The cache marker from 0.5.1 stays as a second layer.** Each handoff carries a random ID inside its encrypted payload, and retrieval records it in `Rails.cache` with `write(key, true, unless_exist: true, expires_in: 10.minutes)`. With the column it refuses a release the claim did not see (a 0.5.1 process that served the token during a rolling deploy), and it is the only cross-copy layer for a handoff that names no key (`TokenSession.store(session, "a token string")`). - **Without the column** (an existing install that has not run `rails generate api_keys:add_key_activity`), the gem behaves as 0.5.1 and the marker is the only cross-copy layer. The token is then released at most once only when every application process shares a store that implements `unless_exist:` atomically (Redis, Memcached), and that store keeps the marker for the whole ten-minute handoff lifetime. If the cache is flushed, restarted without persistence, or evicts the marker within those ten minutes, a copy of the session taken before retrieval can recover the token again for the rest of that window. **`MemoryStore`** only covers the process that holds it. **`FileStore`** checks and then writes, so it stops a later replay on the same host but not two requests that arrive at the same instant. **Solid Cache** (Rails 8's default production store) implements `unless_exist:` as a `SELECT … FOR UPDATE` followed by an upsert; on PostgreSQL that locks nothing while the key does not exist yet, so two simultaneous first retrievals can both succeed. **`ActiveSupport::Cache::NullStore`** (Rails' generated development and test default), or no `Rails.cache` at all, records nothing, so the session deletion is the only protection. @@ -1879,6 +1882,12 @@ bundle exec appraisal rails-8.0 rake test bundle exec appraisal rails-8.1 rake test ``` +`test/databases/check.rb` runs the generated upgrade migrations as `rails db:migrate` runs them, with strong_migrations, and the row locks of Idempotency-Key and the device flow, on a real database. CI runs it on PostgreSQL, MySQL and SQLite: + +```bash +BUNDLE_GEMFILE=test/databases/Gemfile DB_ADAPTER=postgresql bundle exec ruby test/databases/check.rb +``` + ## Development After checking out the repo, run `bin/setup` to install dependencies. Then run `bundle exec rake test`. You can also run `bin/console` for an interactive prompt. diff --git a/lib/api_keys/version.rb b/lib/api_keys/version.rb index d0c7b4c..2b5ddda 100644 --- a/lib/api_keys/version.rb +++ b/lib/api_keys/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module ApiKeys - VERSION = "0.8.0" + VERSION = "0.8.1" end From 79d6a8a493a0e162343661d2533bce71306ee3d7 Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Mon, 26 Oct 2026 00:02:47 +0000 Subject: [PATCH 4/7] Make every upgrade generator strong_migrations-safe on PostgreSQL The gem still ships add_key_types and add_restrictions, and strong_migrations refused both on PostgreSQL, as it did add_created_by. An app that upgrades from an old version must get migrations that a strong_migrations app accepts. add_key_types now runs without a DDL transaction, checks each step, and builds its indexes concurrently on PostgreSQL. strong_migrations calls the four-column index a best-practice problem on every database. The install has the same index, so the migration keeps it inside safety_assured when the app has strong_migrations. Its down removes the indexes before the columns, so no index is left cut down. add_restrictions now adds the check constraint with validate: false on PostgreSQL, then validates it, which does not block reads and writes. On MySQL the migration never ran, because MySQL refuses a literal default on a JSON column. It now uses the expression JSON_OBJECT(), which also gives existing keys an empty object. strong_migrations has no safe form of a check constraint on an existing MySQL table, so it still refuses this migration on MySQL, and the check shows that. add_authentication_index now also recognises PostGIS. The database check runs all seven upgrade migrations as db:migrate does, from an install made before 0.3.0 that has a key, on PostgreSQL, MySQL and SQLite. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 11 +- README.md | 4 +- ...dd_authentication_index_to_api_keys.rb.erb | 4 +- .../add_key_types_to_api_keys.rb.erb | 57 ++++++- .../add_restrictions_to_api_keys.rb.erb | 57 +++++-- test/databases/check.rb | 150 ++++++++++++++---- .../add_key_types_generator_test.rb | 111 +++++++++++++ .../add_restrictions_generator_test.rb | 76 ++++++++- 8 files changed, 414 insertions(+), 56 deletions(-) create mode 100644 test/generators/add_key_types_generator_test.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 33fb2a1..ef05fbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,15 +1,20 @@ ## [0.8.1] - 2026-10-25 -Two fixes found when an app on the fleet upgraded from 0.5 to 0.8.0. +Two fixes found when an app on the fleet upgraded from 0.5 to 0.8.0. The second one now covers every upgrade generator. ### Fixed - The one-time token is readable in transactional tests again, once, as in production. Since 0.6.0 a retrieval inside any open transaction was refused, and Rails' transactional tests run each test inside one, so a test that created a key and read its token got nil (`:inside_transaction`), and the dashboard's token page redirected. When `Rails.env` is test and the only open transaction is an outer, non-joinable one (what Rails' transactional tests, rspec-rails and DatabaseCleaner's `:transaction` strategy open, and what a system test's server thread shares), the claim of `token_shown_at` runs in it: the first retrieval returns the token, and a second one, or one from a copy of the session, does not. A transaction opened on top of it is still refused. Outside the test environment nothing changes. Apps can remove their test-only workarounds. -- `rails generate api_keys:add_created_by` writes a migration that strong_migrations accepts on PostgreSQL. The 0.7.0 migration built the polymorphic index inside the migration's transaction, which blocks writes to `api_keys`, and strong_migrations refused it. The migration now runs with `disable_ddl_transaction!`, adds the columns, then builds the index `index_api_keys_on_created_by` with `algorithm: :concurrently` on PostgreSQL (and PostGIS); MySQL and SQLite build it as usual. Each step checks what exists, so a run that stopped half way can run again. A migration generated by 0.7.0 or 0.8.0 that already ran needs nothing. +- Every upgrade generator now writes a migration that strong_migrations accepts on PostgreSQL. Before, it refused three of them. Those three now run with `disable_ddl_transaction!`, and each step checks what exists, so a run that stopped half way can run again. On MySQL and SQLite they run as before, except where this list says otherwise. A migration that an earlier version generated and that already ran needs nothing. + - `add_created_by` (0.7.0) built the polymorphic index inside the migration's transaction, which blocks writes to `api_keys`. It now adds the columns, then builds `index_api_keys_on_created_by` with `algorithm: :concurrently` on PostgreSQL (and PostGIS). + - `add_key_types` (0.3.0) built its three indexes inside the transaction. They are now built concurrently on PostgreSQL. strong_migrations calls the four-column index a best-practice problem on every database. The install migration has the same index, so the migration keeps it inside `safety_assured` when the app uses strong_migrations. + - `add_restrictions` (0.5.0) added its check constraint and checked every row under a lock that blocks reads and writes. On PostgreSQL it now adds the constraint without that check (`validate: false`), then validates it, which does not block reads and writes. On MySQL, which refuses a literal default on a JSON column, the column now defaults to the expression `JSON_OBJECT()` (MySQL 8.0.13 or later), so the migration runs and existing keys get an empty object. strong_migrations has no safe form of a check constraint on an existing MySQL table, so it still refuses this migration on MySQL. + - `add_authentication_index` (0.4.0) already built its index concurrently. It now also does so on PostGIS. + - `add_key_activity`, `add_device_authorizations` and `add_idempotency_keys` already passed and are unchanged. ### Tests -- The database check (`test/databases/check.rb`, in CI on PostgreSQL, MySQL and SQLite) now runs the four upgrade migrations of 0.6.0 and 0.7.0 as `rails db:migrate` runs them, with strong_migrations, from an install made before 0.6.0. `add_key_activity`, `add_device_authorizations` and `add_idempotency_keys` already passed and are unchanged. +- The database check (`test/databases/check.rb`, in CI on PostgreSQL, MySQL and SQLite) now runs all seven upgrade migrations as `rails db:migrate` runs them, with strong_migrations, from an install made before 0.3.0 that has a key. It runs them down and up again, and again after a run that stopped half way. - The token handoff is tested in transactional tests (the helper and the dashboard) and with real commits. ## [0.8.0] - 2026-10-25 diff --git a/README.md b/README.md index ef5fa42..573d065 100644 --- a/README.md +++ b/README.md @@ -32,6 +32,8 @@ And you're done! ### Upgrading an existing installation +Every upgrade generator below writes a migration that strong_migrations accepts on PostgreSQL (0.8.1). There the indexes on `api_keys` are built concurrently, and the restrictions check constraint is validated apart, outside a transaction, so writes to `api_keys` go on. On MySQL strong_migrations accepts all of them except `add_restrictions`: it has no safe form of a check constraint on an existing MySQL table. + Install the bounded bcrypt authentication lookup index before deploying the current hardening changes: ```bash @@ -66,7 +68,7 @@ rails generate api_keys:add_created_by rails db:migrate ``` -On PostgreSQL this migration builds its index concurrently, outside a transaction, so it does not block writes to `api_keys` and strong_migrations accepts it (0.8.1). Each step checks what exists, so a run that stopped half way can run again. If a concurrent build fails, PostgreSQL keeps an invalid index: remove it before the next run. The 0.6.0 and 0.7.0 upgrade migrations (`add_key_activity`, `add_created_by`, `add_device_authorizations`, `add_idempotency_keys`) pass strong_migrations on PostgreSQL and MySQL as generated. +On PostgreSQL this migration builds its index concurrently, outside a transaction, so it does not block writes to `api_keys` and strong_migrations accepts it (0.8.1). Each step checks what exists, so a run that stopped half way can run again. If a concurrent build fails, PostgreSQL keeps an invalid index: remove it before the next run. The device flow and Idempotency-Key each need a table of their own. New and existing installations run these only if they use the feature: diff --git a/lib/generators/api_keys/templates/add_authentication_index_to_api_keys.rb.erb b/lib/generators/api_keys/templates/add_authentication_index_to_api_keys.rb.erb index e4d3d4c..a575854 100644 --- a/lib/generators/api_keys/templates/add_authentication_index_to_api_keys.rb.erb +++ b/lib/generators/api_keys/templates/add_authentication_index_to_api_keys.rb.erb @@ -10,7 +10,7 @@ class AddAuthenticationIndexToApiKeys < ActiveRecord::Migration<%= migration_ver return if index_exists?(:api_keys, COLUMNS, name: INDEX_NAME) options = { name: INDEX_NAME } - options[:algorithm] = :concurrently if connection.adapter_name.downcase.include?("postgres") + options[:algorithm] = :concurrently if connection.adapter_name.downcase.include?("postg") add_index :api_keys, COLUMNS, **options end @@ -18,7 +18,7 @@ class AddAuthenticationIndexToApiKeys < ActiveRecord::Migration<%= migration_ver return unless index_exists?(:api_keys, COLUMNS, name: INDEX_NAME) options = { name: INDEX_NAME } - options[:algorithm] = :concurrently if connection.adapter_name.downcase.include?("postgres") + options[:algorithm] = :concurrently if connection.adapter_name.downcase.include?("postg") remove_index :api_keys, **options end end diff --git a/lib/generators/api_keys/templates/add_key_types_to_api_keys.rb.erb b/lib/generators/api_keys/templates/add_key_types_to_api_keys.rb.erb index 325f25d..3a4f2b8 100644 --- a/lib/generators/api_keys/templates/add_key_types_to_api_keys.rb.erb +++ b/lib/generators/api_keys/templates/add_key_types_to_api_keys.rb.erb @@ -5,14 +5,55 @@ # # Run this migration if you're upgrading from a version of api_keys that # didn't have key types support. +# +# On PostgreSQL the indexes are built concurrently, so api_keys takes writes +# while they build, and strong_migrations accepts the migration. A concurrent +# build cannot run inside a transaction, so this migration runs without one. +# Each step checks what exists, so a run that stopped half way can run +# again. If a concurrent build fails, PostgreSQL keeps an invalid index: +# remove it before the next run. MySQL and SQLite build the indexes as usual. class AddKeyTypesToApiKeys < ActiveRecord::Migration<%= migration_version %> - def change - add_column :api_keys, :key_type, :string - add_column :api_keys, :environment, :string - - add_index :api_keys, :key_type - add_index :api_keys, :environment - add_index :api_keys, [:owner_type, :owner_id, :key_type, :environment], - name: "index_api_keys_owner_type_env" + OWNER_TYPE_ENV_INDEX = "index_api_keys_owner_type_env" + OWNER_TYPE_ENV_COLUMNS = %i[owner_type owner_id key_type environment].freeze + + disable_ddl_transaction! + + def up + add_column :api_keys, :key_type, :string unless column_exists?(:api_keys, :key_type) + add_column :api_keys, :environment, :string unless column_exists?(:api_keys, :environment) + + add_index :api_keys, :key_type, **index_algorithm unless index_exists?(:api_keys, :key_type) + add_index :api_keys, :environment, **index_algorithm unless index_exists?(:api_keys, :environment) + return if index_exists?(:api_keys, OWNER_TYPE_ENV_COLUMNS) + + # strong_migrations calls a non-unique index of more than three columns + # a best-practice problem. The install migration has the same index, so + # this migration keeps it and tells strong_migrations so. + with_safety_assured do + add_index :api_keys, OWNER_TYPE_ENV_COLUMNS, name: OWNER_TYPE_ENV_INDEX, **index_algorithm + end + end + + def down + if index_exists?(:api_keys, OWNER_TYPE_ENV_COLUMNS) + remove_index :api_keys, name: OWNER_TYPE_ENV_INDEX, **index_algorithm + end + remove_index :api_keys, :environment, **index_algorithm if index_exists?(:api_keys, :environment) + remove_index :api_keys, :key_type, **index_algorithm if index_exists?(:api_keys, :key_type) + remove_column :api_keys, :environment if column_exists?(:api_keys, :environment) + remove_column :api_keys, :key_type if column_exists?(:api_keys, :key_type) + end + + private + + # Concurrently on PostgreSQL (and PostGIS). MySQL and SQLite have no + # concurrent build and refuse the option. + def index_algorithm + connection.adapter_name.downcase.include?("postg") ? { algorithm: :concurrently } : {} + end + + # strong_migrations' safety_assured when the app uses strong_migrations. + def with_safety_assured(&block) + respond_to?(:safety_assured) ? safety_assured(&block) : yield end end diff --git a/lib/generators/api_keys/templates/add_restrictions_to_api_keys.rb.erb b/lib/generators/api_keys/templates/add_restrictions_to_api_keys.rb.erb index 8df6254..1260804 100644 --- a/lib/generators/api_keys/templates/add_restrictions_to_api_keys.rb.erb +++ b/lib/generators/api_keys/templates/add_restrictions_to_api_keys.rb.erb @@ -8,47 +8,82 @@ # Run this migration if you're upgrading from a version of api_keys that # didn't have request restrictions support. The column defaults to an empty # object, which means "unrestricted", so existing keys are unaffected. +# +# On PostgreSQL the check constraint is added first without a check of the +# existing rows, then validated, which does not block reads and writes, so +# strong_migrations accepts the migration. It runs without a transaction, so +# the validation holds no lock from the earlier steps. Each step checks what +# exists, so a run that stopped half way can run again. MySQL refuses a +# literal default on a JSON column, so there the default is the expression +# JSON_OBJECT() (MySQL 8.0.13 or later). strong_migrations has no safe form of +# a check constraint on an existing MySQL table, so it refuses this migration +# on MySQL. class AddRestrictionsToApiKeys < ActiveRecord::Migration<%= migration_version %> CONSTRAINT_NAME = "api_keys_restrictions_is_object" + disable_ddl_transaction! + def up - add_column :api_keys, :restrictions, json_column_type, default: {}, null: false + unless column_exists?(:api_keys, :restrictions) + add_column :api_keys, :restrictions, json_column_type, default: restrictions_default, null: false + end add_restrictions_object_constraint end def down - remove_column :api_keys, :restrictions + remove_check_constraint :api_keys, name: CONSTRAINT_NAME if restrictions_constraint_exists? + remove_column :api_keys, :restrictions if column_exists?(:api_keys, :restrictions) end private # Helper method to determine the appropriate JSON column type based on the database adapter. - # Uses :jsonb for PostgreSQL for better performance and indexing, :json otherwise. + # Uses :jsonb for PostgreSQL (and PostGIS) for better performance and indexing, :json otherwise. def json_column_type - # Check connection availability for adapter name inspection - if ActiveRecord::Base.connection.adapter_name.downcase.include?('postgresql') - :jsonb - else - :json - end + postgresql? ? :jsonb : :json rescue ActiveRecord::ConnectionNotEstablished # Fallback during initial setup or if connection isn't available :text end + # MySQL refuses a literal default on a JSON column, but takes an + # expression, which also fills the existing rows with an empty object. + def restrictions_default + connection.adapter_name.downcase.match?(/mysql|trilogy/) ? -> { "(JSON_OBJECT())" } : {} + end + # JSON columns can also hold arrays and scalars. The application requires an # object so damaged or validation-bypassing writes cannot erase a policy. def add_restrictions_object_constraint return unless connection.supports_check_constraints? expression = case connection.adapter_name.downcase - when /postgres/ + when /postg/ "jsonb_typeof(restrictions) = 'object'" when /sqlite/ "json_valid(restrictions) AND json_type(restrictions) = 'object'" when /mysql|trilogy/ "JSON_TYPE(restrictions) = 'OBJECT'" end - add_check_constraint :api_keys, expression, name: CONSTRAINT_NAME if expression + return unless expression + + unless restrictions_constraint_exists? + # On PostgreSQL, add the constraint without a check of the existing + # rows: this takes a short lock only. + add_check_constraint :api_keys, expression, name: CONSTRAINT_NAME, **(postgresql? ? { validate: false } : {}) + end + # Checks the existing rows without blocking reads and writes. Nothing + # happens when the constraint is valid already. + validate_check_constraint :api_keys, name: CONSTRAINT_NAME if postgresql? + end + + def restrictions_constraint_exists? + return false unless connection.supports_check_constraints? + + connection.check_constraints(:api_keys).any? { |constraint| constraint.name == CONSTRAINT_NAME } + end + + def postgresql? + connection.adapter_name.downcase.include?("postg") end end diff --git a/test/databases/check.rb b/test/databases/check.rb index d8019d6..10e6abd 100644 --- a/test/databases/check.rb +++ b/test/databases/check.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true -# Runs the upgrade migrations of 0.6.0 and 0.7.0 as the generators write -# them and as db:migrate runs them, with strong_migrations checking each one, +# Runs every upgrade migration as the generators write them and as +# db:migrate runs them, with strong_migrations checking each one, # and the row locks of ApiKeys::IdempotencyKey and # ApiKeys::DeviceAuthorization, against a real database with real concurrent # connections. The main suite runs on in-memory SQLite, which has one @@ -20,10 +20,16 @@ require "tmpdir" require "rails" require "active_record/railtie" +# strong_migrations reads Rake.application when it refuses a migration, as +# under rails db:migrate. +require "rake" require "strong_migrations" require "api_keys" require "rails/generators" require "generators/api_keys/install_generator" +require "generators/api_keys/add_key_types_generator" +require "generators/api_keys/add_authentication_index_generator" +require "generators/api_keys/add_restrictions_generator" require "generators/api_keys/add_key_activity_generator" require "generators/api_keys/add_created_by_generator" require "generators/api_keys/add_device_authorizations_generator" @@ -69,9 +75,10 @@ class DatabaseCheckApp < Rails::Application # Writes each migration with its generator, as an app does. MIGRATIONS = Dir.mktmpdir("api_keys_migrations") [ - ApiKeys::Generators::InstallGenerator, ApiKeys::Generators::AddKeyActivityGenerator, - ApiKeys::Generators::AddCreatedByGenerator, ApiKeys::Generators::AddDeviceAuthorizationsGenerator, - ApiKeys::Generators::AddIdempotencyKeysGenerator + ApiKeys::Generators::InstallGenerator, ApiKeys::Generators::AddKeyTypesGenerator, + ApiKeys::Generators::AddAuthenticationIndexGenerator, ApiKeys::Generators::AddRestrictionsGenerator, + ApiKeys::Generators::AddKeyActivityGenerator, ApiKeys::Generators::AddCreatedByGenerator, + ApiKeys::Generators::AddDeviceAuthorizationsGenerator, ApiKeys::Generators::AddIdempotencyKeysGenerator ].each do |generator| $stdout = StringIO.new generator.start([], destination_root: MIGRATIONS) @@ -79,10 +86,16 @@ class DatabaseCheckApp < Rails::Application $stdout = STDOUT end -# The upgrade migrations, in the order db:migrate runs them. -UPGRADES = %w[add_key_activity_to_api_keys add_created_by_to_api_keys create_api_keys_device_authorizations +# Every upgrade migration, in the order db:migrate runs them. +UPGRADES = %w[add_key_types_to_api_keys add_authentication_index_to_api_keys add_restrictions_to_api_keys + add_key_activity_to_api_keys add_created_by_to_api_keys create_api_keys_device_authorizations create_api_keys_idempotency_keys].freeze +# strong_migrations has no safe form of a check constraint on an existing +# MySQL table, so it refuses add_restrictions on MySQL. There the check runs +# it without strong_migrations, and test_a shows the refusal. +UNCHECKED = ADAPTER == "mysql2" ? %w[add_restrictions_to_api_keys] : [] + def migration_file(name) Dir["#{MIGRATIONS}/db/migrate/*_#{name}.rb"].sole end @@ -90,12 +103,38 @@ def migration_file(name) # Runs one generated migration as db:migrate does: inside a DDL transaction # unless the migration disables it, and checked by strong_migrations (which # checks only the way up, by default). An unsafe migration raises. -def run_migration(name, direction) +def run_migration(name, direction, checked: !UNCHECKED.include?(name)) file = migration_file(name) - ActiveRecord::MigrationContext.new(File.dirname(file)).run(direction, File.basename(file).to_i) + run = -> { ActiveRecord::MigrationContext.new(File.dirname(file)).run(direction, File.basename(file).to_i) } + checked ? run.call : StrongMigrations::Checker.safety_assured(&run) +ensure [ApiKeys::ApiKey, ApiKeys::DeviceAuthorization, ApiKeys::IdempotencyKey].each(&:reset_column_information) end +# Takes away what one upgrade adds, as an install from before it looks. +def strip_upgrade(name) + connection = ActiveRecord::Base.connection + case name + when "add_key_types_to_api_keys" + connection.remove_index(:api_keys, name: "index_api_keys_owner_type_env") + %i[key_type environment].each do |column| + connection.remove_index(:api_keys, column) + connection.remove_column(:api_keys, column) + end + when "add_authentication_index_to_api_keys" + connection.remove_index(:api_keys, name: "index_api_keys_authentication_lookup") + when "add_restrictions_to_api_keys" + connection.remove_check_constraint(:api_keys, name: "api_keys_restrictions_is_object") + connection.remove_column(:api_keys, :restrictions) + when "add_key_activity_to_api_keys" + (ApiKeys::ApiKey::REFUSAL_STATS_COLUMNS + [ApiKeys::ApiKey::TOKEN_SHOWN_AT_COLUMN]).each do |column| + connection.remove_column(:api_keys, column) + end + when "add_created_by_to_api_keys" + connection.remove_reference(:api_keys, :created_by, polymorphic: true) + end +end + def load_migration(name) source = File.read(migration_file(name)) # The install migration gives JSON columns literal defaults, which MySQL @@ -109,12 +148,15 @@ def load_migration(name) ActiveRecord::Schema.define { create_table(:users, id: id_type) { |t| t.string :name } } load_migration("create_api_keys_table") CreateApiKeysTable.migrate(:up) -# An install from before 0.6.0: the api_keys table without the columns that -# the upgrades add. Then every upgrade, as an app runs them. -ActiveRecord::Base.connection.remove_reference(:api_keys, :created_by, polymorphic: true) -ApiKeys::ApiKey::REFUSAL_STATS_COLUMNS.each { |column| ActiveRecord::Base.connection.remove_column(:api_keys, column) } -ActiveRecord::Base.connection.remove_column(:api_keys, ApiKeys::ApiKey::TOKEN_SHOWN_AT_COLUMN) +# An install from before 0.3.0: the api_keys table without what the upgrades +# add. Then every upgrade, as an app runs them, on a table that has a row. +UPGRADES.each { |name| strip_upgrade(name) } +ActiveRecord::Base.connection.execute(<<~SQL) + INSERT INTO api_keys (#{%w[prefix token_digest digest_algorithm last4 scopes metadata created_at updated_at].join(", ")}) + VALUES ('ak_old_', 'old-digest', 'sha256', 'old4', '[]', '{}', CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) +SQL UPGRADES.each { |name| run_migration(name, :up) } +OLD_KEY_RESTRICTIONS = ActiveRecord::Base.connection.select_value("SELECT restrictions FROM api_keys WHERE prefix = 'ak_old_'") json = ADAPTER == "postgresql" ? :jsonb : :json ApiKeys::ApiKey.attribute :scopes, json, default: [] ApiKeys::ApiKey.attribute :metadata, json, default: {} @@ -181,34 +223,82 @@ def test_0_strong_migrations_checks_the_migrations_here refute_includes ApiKeys::ApiKey.column_names, "reviewer_id", "strong_migrations stops before the statement" end + def key_types_columns? + (%w[key_type environment] - ApiKeys::ApiKey.column_names).empty? + end + + def index_columns + ActiveRecord::Base.connection.indexes(:api_keys).map(&:columns) + end + + def restrictions_constraint + connection = ActiveRecord::Base.connection + return :unsupported unless connection.supports_check_constraints? + + connection.check_constraints(:api_keys).find { |constraint| constraint.name == "api_keys_restrictions_is_object" } + end + + UPGRADE_INDEXES = [%w[key_type], %w[environment], %w[owner_type owner_id key_type environment], + %w[prefix last4 digest_algorithm], %w[created_by_type created_by_id]].freeze + + def assert_every_upgrade_is_in_place + assert key_types_columns? + assert ApiKeys::ApiKey.restrictions_column? + assert ApiKeys::ApiKey.refusal_stats_columns? + assert ApiKeys::ApiKey.token_shown_at_column? + assert ApiKeys::ApiKey.created_by_columns? + UPGRADE_INDEXES.each { |columns| assert_equal 1, index_columns.count(columns), columns.join(", ") } + constraint = restrictions_constraint + refute_nil constraint, "the restrictions constraint exists" + assert constraint.validated?, "the restrictions constraint is validated" if ADAPTER == "postgresql" + %i[api_keys_device_authorizations api_keys_idempotency_keys].each do |table| + assert ActiveRecord::Base.connection.table_exists?(table), table + end + end + def test_a_the_upgrade_migrations_run_up_down_and_up_again - tables = %i[api_keys_device_authorizations api_keys_idempotency_keys] - created_by_index = %i[created_by_type created_by_id] connection = ActiveRecord::Base.connection + assert_every_upgrade_is_in_place + assert_equal({}, JSON.parse(OLD_KEY_RESTRICTIONS), "a key from before the upgrade is unrestricted") Key.delete_all ApiKeys::DeviceAuthorization.delete_all ApiKeys::ApiKey.delete_all UPGRADES.reverse_each { |name| run_migration(name, :down) } - refute ApiKeys::ApiKey.created_by_columns? - refute ApiKeys::ApiKey.token_shown_at_column? + refute key_types_columns? + refute ApiKeys::ApiKey.restrictions_column? refute ApiKeys::ApiKey.refusal_stats_columns? - tables.each { |table| refute connection.table_exists?(table) } + refute ApiKeys::ApiKey.token_shown_at_column? + refute ApiKeys::ApiKey.created_by_columns? + assert_empty UPGRADE_INDEXES & index_columns + %i[api_keys_device_authorizations api_keys_idempotency_keys].each { |table| refute connection.table_exists?(table) } + if ADAPTER == "mysql2" + error = assert_raises(StandardError) { run_migration("add_restrictions_to_api_keys", :up, checked: true) } + assert_match "#strong_migrations", error.message + refute ApiKeys::ApiKey.restrictions_column?, "strong_migrations stops before the statement" + end UPGRADES.each { |name| run_migration(name, :up) } - assert ApiKeys::ApiKey.created_by_columns? - assert ApiKeys::ApiKey.token_shown_at_column? - assert ApiKeys::ApiKey.refusal_stats_columns? - assert connection.index_exists?(:api_keys, created_by_index, name: "index_api_keys_on_created_by") - tables.each { |table| assert connection.table_exists?(table) } - - # Idempotent: a second run, and a run that stopped after the columns - # (the index was not built), finish the work and add nothing twice. - AddKeyActivityToApiKeys.migrate(:up) - AddCreatedByToApiKeys.migrate(:up) + assert_every_upgrade_is_in_place + + # Idempotent: a second run, and a run that stopped half way, finish the + # work and add nothing twice. + [AddKeyTypesToApiKeys, AddAuthenticationIndexToApiKeys, AddKeyActivityToApiKeys, AddCreatedByToApiKeys] + .each { |migration| migration.migrate(:up) } + StrongMigrations::Checker.safety_assured { AddRestrictionsToApiKeys.migrate(:up) } # MySQL refuses it checked + connection.remove_index(:api_keys, name: "index_api_keys_owner_type_env") + AddKeyTypesToApiKeys.migrate(:up) connection.remove_index(:api_keys, name: "index_api_keys_on_created_by") AddCreatedByToApiKeys.migrate(:up) - assert_equal 1, connection.indexes(:api_keys).count { |index| index.columns == created_by_index.map(&:to_s) } + if ADAPTER == "postgresql" # the constraint was added, but not validated yet + connection.remove_check_constraint(:api_keys, name: "api_keys_restrictions_is_object") + connection.add_check_constraint(:api_keys, "jsonb_typeof(restrictions) = 'object'", + name: "api_keys_restrictions_is_object", validate: false) + refute restrictions_constraint.validated?, "precondition: not validated" + AddRestrictionsToApiKeys.migrate(:up) + end + ApiKeys::ApiKey.reset_column_information + assert_every_upgrade_is_in_place end def test_b_the_foreign_keys_match_the_key_types diff --git a/test/generators/add_key_types_generator_test.rb b/test/generators/add_key_types_generator_test.rb new file mode 100644 index 0000000..27d4b27 --- /dev/null +++ b/test/generators/add_key_types_generator_test.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +require "test_helper" +require "rails/generators/test_case" +require "generators/api_keys/add_key_types_generator" + +module ApiKeys + module Generators + # test/databases/check.rb runs this migration as db:migrate does on + # PostgreSQL, MySQL and SQLite, with strong_migrations. Here: what the + # generator writes, and a run on the suite's SQLite database. + class AddKeyTypesGeneratorTest < Rails::Generators::TestCase + tests ApiKeys::Generators::AddKeyTypesGenerator + destination File.expand_path("../../tmp/add_key_types_generator", __dir__) + setup :prepare_destination + + MIGRATION = "db/migrate/add_key_types_to_api_keys.rb" + COLUMNS = { key_type: [:string, {}], environment: [:string, {}] }.freeze + INDEXES = [%w[key_type], %w[environment], %w[owner_type owner_id key_type environment]].freeze + + def teardown + Object.send(:remove_const, :AddKeyTypesToApiKeys) if Object.const_defined?(:AddKeyTypesToApiKeys) + super + end + + def generated_migration + run_generator + load migration_file_name(MIGRATION) + ::AddKeyTypesToApiKeys.new + end + + def index_columns + ApiKeys::ApiKey.connection.indexes(:api_keys).map(&:columns) + end + + test "generates an idempotent migration that strong_migrations accepts on PostgreSQL" do + run_generator + + assert_migration MIGRATION do |migration| + assert_includes migration, "class AddKeyTypesToApiKeys < ActiveRecord::Migration[" + assert_includes migration, "disable_ddl_transaction!" + assert_includes migration, "add_column :api_keys, :key_type, :string unless column_exists?(:api_keys, :key_type)" + assert_includes migration, "add_index :api_keys, :key_type, **index_algorithm unless index_exists?" + assert_includes migration, 'OWNER_TYPE_ENV_INDEX = "index_api_keys_owner_type_env"' + assert_includes migration, "with_safety_assured do" + assert_includes migration, "{ algorithm: :concurrently }" + refute_includes migration, "def change" + refute_includes migration, "def migration_version" + end + end + + test "the indexes are built concurrently on PostgreSQL and PostGIS, and as usual elsewhere" do + migration = generated_migration + { "PostgreSQL" => { algorithm: :concurrently }, "PostGIS" => { algorithm: :concurrently }, + "Mysql2" => {}, "Trilogy" => {}, "SQLite" => {} }.each do |adapter, options| + migration.stubs(:connection).returns(stub(adapter_name: adapter)) + + assert_equal options, migration.send(:index_algorithm), adapter + end + end + + test "the four-column index uses strong_migrations' safety_assured only when the app has it" do + migration = generated_migration + ran = [] + + migration.send(:with_safety_assured) { ran << :without } + migration.define_singleton_method(:safety_assured) do |&block| + ran << :assured + block.call + end + migration.send(:with_safety_assured) { ran << :with } + + assert_equal %i[without assured with], ran + end + + test "the generated migration adds, re-runs and removes the columns and the indexes" do + migration = generated_migration + connection = ApiKeys::ApiKey.connection + + OptionalColumnsSchema.without(COLUMNS) do + # SQLite keeps the four-column index, cut down to the owner columns, + # when the columns go. An install from before key types has none. + connection.remove_index(:api_keys, name: "index_api_keys_owner_type_env") + assert_empty INDEXES & index_columns, "precondition: not migrated" + ActiveRecord::Migration.suppress_messages do + migration.migrate(:up) + migration.migrate(:up) # idempotent: a second run is a no-op + end + ApiKeys::ApiKey.reset_column_information + assert_empty %w[key_type environment] - ApiKeys::ApiKey.column_names + INDEXES.each { |columns| assert_equal 1, index_columns.count(columns), columns.join(", ") } + + # A run that stopped before the last index builds it on the next run. + connection.remove_index(:api_keys, name: "index_api_keys_owner_type_env") + ActiveRecord::Migration.suppress_messages { migration.migrate(:up) } + assert_includes index_columns, INDEXES.last + + ActiveRecord::Migration.suppress_messages { migration.migrate(:down) } + ApiKeys::ApiKey.reset_column_information + assert_empty %w[key_type environment] & ApiKeys::ApiKey.column_names + assert_empty INDEXES & index_columns, "no index is left, not even a shortened one" + + # Leaves the shared test schema with its indexes. + ActiveRecord::Migration.suppress_messages { migration.migrate(:up) } + end + + INDEXES.each { |columns| assert_includes index_columns, columns } + end + end + end +end diff --git a/test/generators/add_restrictions_generator_test.rb b/test/generators/add_restrictions_generator_test.rb index 41619f7..51d11af 100644 --- a/test/generators/add_restrictions_generator_test.rb +++ b/test/generators/add_restrictions_generator_test.rb @@ -10,13 +10,22 @@ class AddRestrictionsGeneratorTest < Rails::Generators::TestCase tests ApiKeys::Generators::AddRestrictionsGenerator destination File.expand_path("../../tmp/add_restrictions_generator", __dir__) setup :prepare_destination + # Generator tests do not reset the configuration; another test may have + # left key types on. + setup { ApiKeys.reset_configuration! } test "generates a restrictions column migration that is unrestricted by default" do run_generator assert_migration "db/migrate/add_restrictions_to_api_keys.rb" do |migration| assert_includes migration, "class AddRestrictionsToApiKeys < ActiveRecord::Migration[" - assert_includes migration, "add_column :api_keys, :restrictions, json_column_type, default: {}, null: false" + assert_includes migration, "disable_ddl_transaction!" + assert_includes migration, "add_column :api_keys, :restrictions, json_column_type, default: restrictions_default, null: false" + assert_includes migration, "unless column_exists?(:api_keys, :restrictions)" + assert_includes migration, "unless restrictions_constraint_exists?" + assert_includes migration, "postgresql? ? { validate: false } : {}" + assert_includes migration, "validate_check_constraint :api_keys, name: CONSTRAINT_NAME if postgresql?" + assert_includes migration, '-> { "(JSON_OBJECT())" }' assert_includes migration, "def json_column_type" assert_includes migration, ":jsonb" assert_includes migration, "rescue ActiveRecord::ConnectionNotEstablished" @@ -27,6 +36,71 @@ class AddRestrictionsGeneratorTest < Rails::Generators::TestCase end end + def generated_migration + run_generator + load migration_file_name("db/migrate/add_restrictions_to_api_keys.rb") + ::AddRestrictionsToApiKeys.new + end + + def teardown + Object.send(:remove_const, :AddRestrictionsToApiKeys) if Object.const_defined?(:AddRestrictionsToApiKeys) + super + end + + # test/databases/check.rb runs this migration as db:migrate does on + # PostgreSQL, MySQL and SQLite, with strong_migrations. + test "each database gets its column type and default, and PostgreSQL validates apart" do + migration = generated_migration + { + "PostgreSQL" => [:jsonb, {}, true], "PostGIS" => [:jsonb, {}, true], + "Mysql2" => [:json, "(JSON_OBJECT())", false], "Trilogy" => [:json, "(JSON_OBJECT())", false], + "SQLite" => [:json, {}, false] + }.each do |adapter, (type, default, postgresql)| + migration.stubs(:connection).returns(stub(adapter_name: adapter)) + resolved = migration.send(:restrictions_default) + + assert_equal type, migration.send(:json_column_type), adapter + assert_equal default, resolved.respond_to?(:call) ? resolved.call : resolved, adapter + assert_equal postgresql, migration.send(:postgresql?), adapter + end + end + + test "the generated migration adds, re-runs and removes the column and its constraint" do + migration = generated_migration + connection = ApiKeys::ApiKey.connection + constraint = -> { connection.check_constraints(:api_keys).find { |c| c.name == "api_keys_restrictions_is_object" } } + + OptionalColumnsSchema.without(restrictions: [:text, { default: "{}", null: false }]) do + # A key from before restrictions existed: a plain row. + now = Time.current + ApiKeys::ApiKey.insert_all!([{ prefix: "ak_old_", token_digest: "old-digest", digest_algorithm: "sha256", + last4: "old4", created_at: now, updated_at: now }]) + old_key = ApiKeys::ApiKey.find_by!(token_digest: "old-digest") + ActiveRecord::Migration.suppress_messages do + migration.migrate(:up) + migration.migrate(:up) # idempotent: a second run is a no-op + end + ApiKeys::ApiKey.reset_column_information + + assert ApiKeys::ApiKey.restrictions_column? + assert constraint.call, "the object constraint exists" + raw = connection.select_value("SELECT restrictions FROM api_keys WHERE id = #{Integer(old_key.id)}") + assert_equal({}, JSON.parse(raw), "an existing key is unrestricted") + assert_raises(ActiveRecord::StatementInvalid) do + connection.execute("UPDATE api_keys SET restrictions = '[]' WHERE id = #{Integer(old_key.id)}") + end + + ActiveRecord::Migration.suppress_messages { migration.migrate(:down) } + ApiKeys::ApiKey.reset_column_information + refute ApiKeys::ApiKey.restrictions_column? + assert_nil constraint.call + end + + assert ApiKeys::ApiKey.restrictions_column?, "the shared test schema is restored" + ensure + ApiKeys::ApiKey.where(token_digest: "old-digest").delete_all + end + test "the SQLite constraint accepts objects and rejects scalar JSON" do connection = ActiveRecord::Base.connection table = :api_keys_restrictions_constraint_probe From 2389a2bebdf1ff87244127892fa4b4b42d5093c0 Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Mon, 26 Oct 2026 00:28:36 +0000 Subject: [PATCH 5/7] Address the review of 0.8.1 Four generated migrations failed rubocop-rails-omakase, the style a Rails app lints db/migrate with: arrays without inner spaces and a case whose end was not aligned. They now use %i[] arrays, two methods for the key types, and case on its own line under the assignment. A new test generates all eight migrations and runs RuboCop with the omakase config on them. rubocop-rails-omakase joins the test bundle. The Rails 8.0 appraisal keeps json below 3, because Active Support 8.0 passes an option that json 3 removed. After a failed concurrent build, PostgreSQL keeps an invalid index, and a second run reported success. add_created_by, add_key_types and add_authentication_index now remove an invalid index and build it again. The database check shows this on PostgreSQL. ApiKeys.test_environment? is now the one test-environment check, for the token handoff and for idempotency_in_test_transactions. The README and the code document the three cases of the token exemption, and a test pins the depth-1 joinable: false case. The README and the add_restrictions migration say what to do on MySQL with strong_migrations. The CHANGELOG says why the token fix needs no switch, and dates 0.8.1 2026-10-26. Co-Authored-By: Claude Opus 5.5 --- Appraisals | 4 ++ CHANGELOG.md | 18 +++++--- Gemfile | 3 ++ README.md | 6 +-- gemfiles/rails_7.2.gemfile | 1 + gemfiles/rails_8.0.gemfile | 2 + gemfiles/rails_8.1.gemfile | 1 + lib/api_keys.rb | 8 ++++ lib/api_keys/configuration.rb | 8 +--- lib/api_keys/helpers/token_session.rb | 16 +++++-- ...dd_authentication_index_to_api_keys.rb.erb | 33 +++++++++++--- .../add_created_by_to_api_keys.rb.erb | 18 +++++++- .../add_key_types_to_api_keys.rb.erb | 24 ++++++++-- .../add_restrictions_to_api_keys.rb.erb | 22 +++++---- ...eate_api_keys_device_authorizations.rb.erb | 17 ++++--- .../create_api_keys_idempotency_keys.rb.erb | 19 +++++--- .../templates/create_api_keys_table.rb.erb | 42 +++++++++-------- test/configuration_test.rb | 9 ++++ test/databases/check.rb | 35 +++++++++++++++ ...add_authentication_index_generator_test.rb | 3 +- .../add_key_types_generator_test.rb | 3 +- .../generated_migrations_style_test.rb | 45 +++++++++++++++++++ test/generators/install_generator_test.rb | 2 +- .../token_session_transactional_test.rb | 40 ++++++++++++++--- test/models/idempotency_key_test.rb | 2 +- test/trusted_key_check_test.rb | 1 + 26 files changed, 302 insertions(+), 80 deletions(-) create mode 100644 test/generators/generated_migrations_style_test.rb diff --git a/Appraisals b/Appraisals index 57efa8f..a751525 100644 --- a/Appraisals +++ b/Appraisals @@ -15,6 +15,10 @@ end appraise "rails-8.0" do gem "rails", ">= 8.0.5.1", "< 8.1.a" + # Active Support 8.0 passes quirks_mode to JSON.generate, which json 3 + # removed. RuboCop, for the style test of the generated migrations, allows + # json 3, so keep json 2 here. + gem "json", "< 3" end # Test against Rails 8.1 (latest) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef05fbe..1de67a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,20 +1,26 @@ -## [0.8.1] - 2026-10-25 +## [0.8.1] - 2026-10-26 Two fixes found when an app on the fleet upgraded from 0.5 to 0.8.0. The second one now covers every upgrade generator. ### Fixed -- The one-time token is readable in transactional tests again, once, as in production. Since 0.6.0 a retrieval inside any open transaction was refused, and Rails' transactional tests run each test inside one, so a test that created a key and read its token got nil (`:inside_transaction`), and the dashboard's token page redirected. When `Rails.env` is test and the only open transaction is an outer, non-joinable one (what Rails' transactional tests, rspec-rails and DatabaseCleaner's `:transaction` strategy open, and what a system test's server thread shares), the claim of `token_shown_at` runs in it: the first retrieval returns the token, and a second one, or one from a copy of the session, does not. A transaction opened on top of it is still refused. Outside the test environment nothing changes. Apps can remove their test-only workarounds. -- Every upgrade generator now writes a migration that strong_migrations accepts on PostgreSQL. Before, it refused three of them. Those three now run with `disable_ddl_transaction!`, and each step checks what exists, so a run that stopped half way can run again. On MySQL and SQLite they run as before, except where this list says otherwise. A migration that an earlier version generated and that already ran needs nothing. +- The one-time token is readable in transactional tests again, once, as in production. Since 0.6.0 a retrieval inside any open transaction was refused, and Rails' transactional tests run each test inside one, so a test that created a key and read its token got nil (`:inside_transaction`), and the dashboard's token page redirected. When `Rails.env` is test and the only open transaction is an outer, non-joinable one (what Rails' transactional tests, rspec-rails and DatabaseCleaner's `:transaction` strategy open, and what a system test's server thread shares), the claim of `token_shown_at` runs in it: the first retrieval returns the token, and a second one, or one from a copy of the session, does not. A transaction opened on top of it is still refused. Outside the test environment nothing changes. Unlike `config.idempotency_in_test_transactions`, this needs no switch: apps whose tests broke on 0.6.0 must get them back without new configuration, and the exemption covers only the test's own transaction, not every depth. The check reads the shape of the transaction, so in a test without a transaction of its own, an outer `transaction(joinable: false)` that the app opens is exempt too; the README lists the cases. Apps can remove their test-only workarounds. +- Every upgrade generator now writes a migration that strong_migrations accepts on PostgreSQL. Before, it refused three of them. Those three now run with `disable_ddl_transaction!`, and each step checks what exists, so a run that stopped half way can run again. An invalid index that a failed concurrent build left on PostgreSQL counts as absent: the next run removes it and builds it again (`add_created_by`, `add_key_types`, `add_authentication_index`). On MySQL and SQLite they run as before, except where this list says otherwise. A migration that an earlier version generated and that already ran needs nothing. - `add_created_by` (0.7.0) built the polymorphic index inside the migration's transaction, which blocks writes to `api_keys`. It now adds the columns, then builds `index_api_keys_on_created_by` with `algorithm: :concurrently` on PostgreSQL (and PostGIS). - `add_key_types` (0.3.0) built its three indexes inside the transaction. They are now built concurrently on PostgreSQL. strong_migrations calls the four-column index a best-practice problem on every database. The install migration has the same index, so the migration keeps it inside `safety_assured` when the app uses strong_migrations. - - `add_restrictions` (0.5.0) added its check constraint and checked every row under a lock that blocks reads and writes. On PostgreSQL it now adds the constraint without that check (`validate: false`), then validates it, which does not block reads and writes. On MySQL, which refuses a literal default on a JSON column, the column now defaults to the expression `JSON_OBJECT()` (MySQL 8.0.13 or later), so the migration runs and existing keys get an empty object. strong_migrations has no safe form of a check constraint on an existing MySQL table, so it still refuses this migration on MySQL. + - `add_restrictions` (0.5.0) added its check constraint and checked every row under a lock that blocks reads and writes. On PostgreSQL it now adds the constraint without that check (`validate: false`), then validates it, which does not block reads and writes. On MySQL, which refuses a literal default on a JSON column, the column now defaults to the expression `JSON_OBJECT()` (MySQL 8.0.13 or later), so the migration runs and existing keys get an empty object. strong_migrations has no safe form of a check constraint on an existing MySQL table, so it still refuses this migration on MySQL. There, check the size of `api_keys`, then wrap the call of `add_restrictions_object_constraint` in `safety_assured`, or run the migration in a maintenance window. - `add_authentication_index` (0.4.0) already built its index concurrently. It now also does so on PostGIS. - - `add_key_activity`, `add_device_authorizations` and `add_idempotency_keys` already passed and are unchanged. + - `add_key_activity`, `add_device_authorizations` and `add_idempotency_keys` already passed strong_migrations. +- Every generated migration, the install included, passes rubocop-rails-omakase, the Rails default style. Array literals of symbols are `%i[]`, the key types are two methods instead of an array, and each `case` has its own line under `expression =`. Before, four files had 16 offenses. + +### Changed + +- `ApiKeys.test_environment?` says whether `Rails.env` is test. The token handoff and `config.idempotency_in_test_transactions` both use it. ### Tests -- The database check (`test/databases/check.rb`, in CI on PostgreSQL, MySQL and SQLite) now runs all seven upgrade migrations as `rails db:migrate` runs them, with strong_migrations, from an install made before 0.3.0 that has a key. It runs them down and up again, and again after a run that stopped half way. +- The database check (`test/databases/check.rb`, in CI on PostgreSQL, MySQL and SQLite) now runs all seven upgrade migrations as `rails db:migrate` runs them, with strong_migrations, from an install made before 0.3.0 that has a key. It runs them down and up again, and again after a run that stopped half way. On PostgreSQL it also leaves an invalid index from a failed concurrent build and shows that the next run builds it again. +- A generator test generates all eight migrations and runs RuboCop with the omakase config on them. - The token handoff is tested in transactional tests (the helper and the dashboard) and with real commits. ## [0.8.0] - 2026-10-25 diff --git a/Gemfile b/Gemfile index b9e2395..45f8771 100644 --- a/Gemfile +++ b/Gemfile @@ -26,4 +26,7 @@ group :development, :test do # Ruby 4.0+ compatibility: ostruct removed from stdlib gem "ostruct" + + # Lints the generated migrations as a host app on omakase does + gem "rubocop-rails-omakase", require: false end diff --git a/README.md b/README.md index 573d065..252da33 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,7 @@ And you're done! ### Upgrading an existing installation -Every upgrade generator below writes a migration that strong_migrations accepts on PostgreSQL (0.8.1). There the indexes on `api_keys` are built concurrently, and the restrictions check constraint is validated apart, outside a transaction, so writes to `api_keys` go on. On MySQL strong_migrations accepts all of them except `add_restrictions`: it has no safe form of a check constraint on an existing MySQL table. +Every upgrade generator below writes a migration that strong_migrations accepts on PostgreSQL (0.8.1). There the indexes on `api_keys` are built concurrently, and the restrictions check constraint is validated apart, outside a transaction, so writes to `api_keys` go on. If a concurrent build fails (a lock or statement timeout, for example), PostgreSQL keeps an invalid index; the next run of the migration removes it and builds it again. On MySQL strong_migrations accepts all of them except `add_restrictions`: it has no safe form of a check constraint on an existing MySQL table, because MySQL copies the table to add one. On MySQL with strong_migrations, check the size of `api_keys` first. Then wrap the call of `add_restrictions_object_constraint` in the generated migration in `safety_assured do ... end`, or run the migration in a maintenance window. Install the bounded bcrypt authentication lookup index before deploying the current hardening changes: @@ -68,7 +68,7 @@ rails generate api_keys:add_created_by rails db:migrate ``` -On PostgreSQL this migration builds its index concurrently, outside a transaction, so it does not block writes to `api_keys` and strong_migrations accepts it (0.8.1). Each step checks what exists, so a run that stopped half way can run again. If a concurrent build fails, PostgreSQL keeps an invalid index: remove it before the next run. +On PostgreSQL this migration builds its index concurrently, outside a transaction, so it does not block writes to `api_keys` and strong_migrations accepts it (0.8.1). Each step checks what exists, so a run that stopped half way can run again, and an invalid index from a failed concurrent build is built again. The device flow and Idempotency-Key each need a table of their own. New and existing installations run these only if they use the feature: @@ -558,7 +558,7 @@ The database applies it atomically, and the claim commits before the token is re - **With the column, the token is released at most once per key.** A cache flush, a restart, or an eviction changes nothing, and two simultaneous first retrievals are serialized by the database: one wins. `ApiKey#token_shown_at` and `#token_shown?` report it. A key whose row no longer exists gets nothing: zero rows claimed fails closed. - **A retrieval attempted inside a database transaction is refused, and can be retried outside it.** Inside a transaction the claim would commit only with it, after the token had been returned, and a rollback would undo it. No cache can stand in for that (a process-local store protects one process), so whatever the cache, the token is withheld with `:inside_transaction`, nothing is recorded, and the handoff stays in the session for a retrieval outside the transaction. The check is Active Record's transaction manager on the `ApiKey` connection: a transaction opened with raw SQL (`BEGIN`) is not detected, and one on another database's connection does not matter. The dashboard opens no transaction itself, but its controller inherits from your `ApplicationController` (or the configured parent): a host `around_action` that wraps requests in a transaction will see the refusal and must exclude the token page. -- **In a transactional test, the token is released once, as in production (0.8.1).** Rails' transactional tests (also rspec-rails and DatabaseCleaner's `:transaction` strategy) open one outer, non-joinable transaction around each test, and a system test's server thread shares it. When `Rails.env` is test and that is the only open transaction, the claim runs in it: the first retrieval in the test returns the token, and a second one, or one from a copy of the session, does not. The rollback at the end of the test removes the key with its claim. A transaction that the app or the test opens on top of it still refuses the retrieval, as in production, so a test sees a host `around_action` that would break the token page. From 0.6.0 to 0.8.0 every retrieval in a transactional test was refused. +- **In a transactional test, the token is released once, as in production (0.8.1).** Rails' transactional tests (also rspec-rails and DatabaseCleaner's `:transaction` strategy) open one outer, non-joinable transaction around each test, and a system test's server thread shares it. When `Rails.env` is test and that is the only open transaction, the claim runs in it: the first retrieval in the test returns the token, and a second one, or one from a copy of the session, does not. The rollback at the end of the test removes the key with its claim. A transaction that the app or the test opens on top of it still refuses the retrieval, as in production, so a test sees a host `around_action` that would break the token page. From 0.6.0 to 0.8.0 every retrieval in a transactional test was refused. The check reads the shape of the transaction, not where it came from, so three cases follow. In a test without a transaction of its own (`use_transactional_tests = false`, or truncation), an outer `transaction(joinable: false)` that the app opens is exempt too, where production refuses it. An environment with another name (`ci`, `staging`) is never exempt: only `Rails.env` test is. A test transaction inside another one (fixtures and DatabaseCleaner's `:transaction` strategy together) is two deep, so it is refused, as in 0.8.0. No setting turns this on: apps whose tests broke on 0.6.0 need no new configuration. - **The column is written only by that claim.** The model marks it read-only, so no form, mass assignment, or `update` can clear it and reopen a handoff. - **The cache marker from 0.5.1 stays as a second layer.** Each handoff carries a random ID inside its encrypted payload, and retrieval records it in `Rails.cache` with `write(key, true, unless_exist: true, expires_in: 10.minutes)`. With the column it refuses a release the claim did not see (a 0.5.1 process that served the token during a rolling deploy), and it is the only cross-copy layer for a handoff that names no key (`TokenSession.store(session, "a token string")`). - **Without the column** (an existing install that has not run `rails generate api_keys:add_key_activity`), the gem behaves as 0.5.1 and the marker is the only cross-copy layer. The token is then released at most once only when every application process shares a store that implements `unless_exist:` atomically (Redis, Memcached), and that store keeps the marker for the whole ten-minute handoff lifetime. If the cache is flushed, restarted without persistence, or evicts the marker within those ten minutes, a copy of the session taken before retrieval can recover the token again for the rest of that window. **`MemoryStore`** only covers the process that holds it. **`FileStore`** checks and then writes, so it stops a later replay on the same host but not two requests that arrive at the same instant. **Solid Cache** (Rails 8's default production store) implements `unless_exist:` as a `SELECT … FOR UPDATE` followed by an upsert; on PostgreSQL that locks nothing while the key does not exist yet, so two simultaneous first retrievals can both succeed. **`ActiveSupport::Cache::NullStore`** (Rails' generated development and test default), or no `Rails.cache` at all, records nothing, so the session deletion is the only protection. diff --git a/gemfiles/rails_7.2.gemfile b/gemfiles/rails_7.2.gemfile index 0ac2e11..ace16c1 100644 --- a/gemfiles/rails_7.2.gemfile +++ b/gemfiles/rails_7.2.gemfile @@ -18,6 +18,7 @@ group :development, :test do gem "bootsnap", require: false gem "mocha", "~> 2.0" gem "ostruct" + gem "rubocop-rails-omakase", require: false end gemspec path: "../" diff --git a/gemfiles/rails_8.0.gemfile b/gemfiles/rails_8.0.gemfile index 3e68819..5e96940 100644 --- a/gemfiles/rails_8.0.gemfile +++ b/gemfiles/rails_8.0.gemfile @@ -4,6 +4,7 @@ source "https://rubygems.org" gem "rake", "~> 13.0" gem "rails", ">= 8.0.5.1", "< 8.1.a" +gem "json", "< 3" group :development, :test do gem "appraisal" @@ -18,6 +19,7 @@ group :development, :test do gem "bootsnap", require: false gem "mocha", "~> 2.0" gem "ostruct" + gem "rubocop-rails-omakase", require: false end gemspec path: "../" diff --git a/gemfiles/rails_8.1.gemfile b/gemfiles/rails_8.1.gemfile index 3d27c1d..96717ad 100644 --- a/gemfiles/rails_8.1.gemfile +++ b/gemfiles/rails_8.1.gemfile @@ -18,6 +18,7 @@ group :development, :test do gem "bootsnap", require: false gem "mocha", "~> 2.0" gem "ostruct" + gem "rubocop-rails-omakase", require: false end gemspec path: "../" diff --git a/lib/api_keys.rb b/lib/api_keys.rb index c49db16..96f9863 100644 --- a/lib/api_keys.rb +++ b/lib/api_keys.rb @@ -43,6 +43,14 @@ def reset_configuration! @configuration = ::ApiKeys::Configuration.new end + # Whether Rails.env is test, read at each call. The test-only exemptions + # use it: config.idempotency_in_test_transactions, and the one-time token + # handoff inside a transactional test's own transaction. + # @return [Boolean] + def test_environment? + defined?(Rails) && Rails.respond_to?(:env) && Rails.env.test? ? true : false + end + # Add-only backfill after widening a key type's `permissions:` ceiling, for # types that keep stored scopes. See ApiKeys::ApiKey.reconcile_scopes!. # @return [Integer] How many keys gained scopes. diff --git a/lib/api_keys/configuration.rb b/lib/api_keys/configuration.rb index 76b3e05..4e41193 100644 --- a/lib/api_keys/configuration.rb +++ b/lib/api_keys/configuration.rb @@ -307,7 +307,7 @@ def tenant_resolver=(value) def idempotency_in_test_transactions=(value) raise ArgumentError, "idempotency_in_test_transactions must be true or false" unless [true, false].include?(value) - if value && !test_environment? + if value && !ApiKeys.test_environment? raise ArgumentError, "idempotency_in_test_transactions is for transactional tests only: Rails.env must be test" end @@ -317,7 +317,7 @@ def idempotency_in_test_transactions=(value) # Whether hold may run inside an open transaction: the switch is on, and # Rails.env is still test when it is used. def idempotency_in_test_transactions? - @idempotency_in_test_transactions == true && test_environment? + @idempotency_in_test_transactions == true && ApiKeys.test_environment? end def trusted_key_check=(value) @@ -454,10 +454,6 @@ def environments=(value) private - def test_environment? - defined?(Rails) && Rails.respond_to?(:env) && Rails.env.test? ? true : false - end - def validate_resolved_prefix!(prefix) valid = prefix.is_a?(String) && prefix.present? && prefix.valid_encoding? && prefix.bytesize <= 64 && prefix.each_codepoint.none? { |codepoint| codepoint <= 0x20 || codepoint == 0x7f } diff --git a/lib/api_keys/helpers/token_session.rb b/lib/api_keys/helpers/token_session.rb index a0ca5b6..001b9ac 100644 --- a/lib/api_keys/helpers/token_session.rb +++ b/lib/api_keys/helpers/token_session.rb @@ -323,10 +323,20 @@ def claim_could_roll_back?(stored_id) # write of the test: the test sees it, a second retrieval in the same # test is refused, and the rollback at the end of the test removes the # key with it. A transaction that the app or the test opens on top of - # it still refuses the retrieval, as in production. Only when - # Rails.env is test. + # it still refuses the retrieval, as in production. + # + # The check reads the shape of the transaction, not where it came + # from, so three cases follow: + # - In a test without a transaction of its own, an outer + # `transaction(joinable: false)` that the app opens is exempt too. + # Production refuses it. + # - An environment with another name (`ci`, `staging`) is never + # exempt: only Rails.env test is. + # - A test transaction inside another one (fixtures and + # DatabaseCleaner's :transaction strategy together) is two deep, so + # it is not exempt, as in 0.8.0. def only_test_transaction_open?(connection) - return false unless defined?(Rails) && Rails.respond_to?(:env) && Rails.env.test? + return false unless ApiKeys.test_environment? return false unless connection.open_transactions == 1 !connection.current_transaction.joinable? diff --git a/lib/generators/api_keys/templates/add_authentication_index_to_api_keys.rb.erb b/lib/generators/api_keys/templates/add_authentication_index_to_api_keys.rb.erb index a575854..c7a7261 100644 --- a/lib/generators/api_keys/templates/add_authentication_index_to_api_keys.rb.erb +++ b/lib/generators/api_keys/templates/add_authentication_index_to_api_keys.rb.erb @@ -1,5 +1,9 @@ # frozen_string_literal: true +# On PostgreSQL the index is built concurrently, outside a transaction, so +# api_keys takes writes while it builds, and strong_migrations accepts the +# migration. If a concurrent build fails, PostgreSQL keeps an invalid index; +# the next run removes it and builds it again. class AddAuthenticationIndexToApiKeys < ActiveRecord::Migration<%= migration_version %> INDEX_NAME = "index_api_keys_authentication_lookup" COLUMNS = %i[prefix last4 digest_algorithm].freeze @@ -7,18 +11,35 @@ class AddAuthenticationIndexToApiKeys < ActiveRecord::Migration<%= migration_ver disable_ddl_transaction! def up + remove_invalid_index return if index_exists?(:api_keys, COLUMNS, name: INDEX_NAME) - options = { name: INDEX_NAME } - options[:algorithm] = :concurrently if connection.adapter_name.downcase.include?("postg") - add_index :api_keys, COLUMNS, **options + add_index :api_keys, COLUMNS, name: INDEX_NAME, **index_algorithm end def down return unless index_exists?(:api_keys, COLUMNS, name: INDEX_NAME) - options = { name: INDEX_NAME } - options[:algorithm] = :concurrently if connection.adapter_name.downcase.include?("postg") - remove_index :api_keys, **options + remove_index :api_keys, name: INDEX_NAME, **index_algorithm + end + + private + + # Concurrently on PostgreSQL (and PostGIS). MySQL and SQLite have no + # concurrent build and refuse the option. + def index_algorithm + connection.adapter_name.downcase.include?("postg") ? { algorithm: :concurrently } : {} + end + + # A concurrent build that fails (a lock or statement timeout, for example) + # leaves an invalid index on PostgreSQL, which index_exists? still finds. + # Treat it as absent: remove it, so that the next step builds it again. + # (IndexDefinition#valid? exists from Rails 7.1. The migrator's lock keeps + # a second run from removing an index that another run still builds.) + def remove_invalid_index + index = connection.indexes(:api_keys).find { |candidate| candidate.name == INDEX_NAME } + return unless index.respond_to?(:valid?) && !index.valid? + + remove_index :api_keys, name: INDEX_NAME, **index_algorithm end end diff --git a/lib/generators/api_keys/templates/add_created_by_to_api_keys.rb.erb b/lib/generators/api_keys/templates/add_created_by_to_api_keys.rb.erb index 4fade61..df23424 100644 --- a/lib/generators/api_keys/templates/add_created_by_to_api_keys.rb.erb +++ b/lib/generators/api_keys/templates/add_created_by_to_api_keys.rb.erb @@ -15,8 +15,9 @@ # while it builds, and strong_migrations accepts the migration. A concurrent # build cannot run inside a transaction, so this migration runs without one. # Each step checks what exists, so a run that stopped half way can run -# again. If a concurrent build fails, PostgreSQL keeps an invalid index: -# remove it before the next run. MySQL and SQLite build the index as usual. +# again. If a concurrent build fails, PostgreSQL keeps an invalid index; the +# next run removes it and builds it again. MySQL and SQLite build the +# index as usual. class AddCreatedByToApiKeys < ActiveRecord::Migration<%= migration_version %> INDEX_NAME = "index_api_keys_on_created_by" INDEX_COLUMNS = %i[created_by_type created_by_id].freeze @@ -27,6 +28,7 @@ class AddCreatedByToApiKeys < ActiveRecord::Migration<%= migration_version %> unless column_exists?(:api_keys, :created_by_id) add_reference :api_keys, :created_by, polymorphic: true, null: true, type: foreign_key_type, index: false end + remove_invalid_index(INDEX_COLUMNS) return if index_exists?(:api_keys, INDEX_COLUMNS) add_index :api_keys, INDEX_COLUMNS, name: INDEX_NAME, **index_algorithm @@ -50,4 +52,16 @@ class AddCreatedByToApiKeys < ActiveRecord::Migration<%= migration_version %> def index_algorithm connection.adapter_name.downcase.include?("postg") ? { algorithm: :concurrently } : {} end + + # A concurrent build that fails (a lock or statement timeout, for example) + # leaves an invalid index on PostgreSQL, which index_exists? still finds. + # Treat it as absent: remove it, so that the next step builds it again. + # (IndexDefinition#valid? exists from Rails 7.1. The migrator's lock keeps + # a second run from removing an index that another run still builds.) + def remove_invalid_index(columns) + index = connection.indexes(:api_keys).find { |candidate| candidate.columns == Array(columns).map(&:to_s) } + return unless index.respond_to?(:valid?) && !index.valid? + + remove_index :api_keys, name: index.name, **index_algorithm + end end diff --git a/lib/generators/api_keys/templates/add_key_types_to_api_keys.rb.erb b/lib/generators/api_keys/templates/add_key_types_to_api_keys.rb.erb index 3a4f2b8..0659ee3 100644 --- a/lib/generators/api_keys/templates/add_key_types_to_api_keys.rb.erb +++ b/lib/generators/api_keys/templates/add_key_types_to_api_keys.rb.erb @@ -10,8 +10,9 @@ # while they build, and strong_migrations accepts the migration. A concurrent # build cannot run inside a transaction, so this migration runs without one. # Each step checks what exists, so a run that stopped half way can run -# again. If a concurrent build fails, PostgreSQL keeps an invalid index: -# remove it before the next run. MySQL and SQLite build the indexes as usual. +# again. If a concurrent build fails, PostgreSQL keeps an invalid index; the +# next run removes it and builds it again. MySQL and SQLite build the +# indexes as usual. class AddKeyTypesToApiKeys < ActiveRecord::Migration<%= migration_version %> OWNER_TYPE_ENV_INDEX = "index_api_keys_owner_type_env" OWNER_TYPE_ENV_COLUMNS = %i[owner_type owner_id key_type environment].freeze @@ -22,8 +23,11 @@ class AddKeyTypesToApiKeys < ActiveRecord::Migration<%= migration_version %> add_column :api_keys, :key_type, :string unless column_exists?(:api_keys, :key_type) add_column :api_keys, :environment, :string unless column_exists?(:api_keys, :environment) - add_index :api_keys, :key_type, **index_algorithm unless index_exists?(:api_keys, :key_type) - add_index :api_keys, :environment, **index_algorithm unless index_exists?(:api_keys, :environment) + %i[key_type environment].each do |column| + remove_invalid_index(column) + add_index :api_keys, column, **index_algorithm unless index_exists?(:api_keys, column) + end + remove_invalid_index(OWNER_TYPE_ENV_COLUMNS) return if index_exists?(:api_keys, OWNER_TYPE_ENV_COLUMNS) # strong_migrations calls a non-unique index of more than three columns @@ -52,6 +56,18 @@ class AddKeyTypesToApiKeys < ActiveRecord::Migration<%= migration_version %> connection.adapter_name.downcase.include?("postg") ? { algorithm: :concurrently } : {} end + # A concurrent build that fails (a lock or statement timeout, for example) + # leaves an invalid index on PostgreSQL, which index_exists? still finds. + # Treat it as absent: remove it, so that the next step builds it again. + # (IndexDefinition#valid? exists from Rails 7.1. The migrator's lock keeps + # a second run from removing an index that another run still builds.) + def remove_invalid_index(columns) + index = connection.indexes(:api_keys).find { |candidate| candidate.columns == Array(columns).map(&:to_s) } + return unless index.respond_to?(:valid?) && !index.valid? + + remove_index :api_keys, name: index.name, **index_algorithm + end + # strong_migrations' safety_assured when the app uses strong_migrations. def with_safety_assured(&block) respond_to?(:safety_assured) ? safety_assured(&block) : yield diff --git a/lib/generators/api_keys/templates/add_restrictions_to_api_keys.rb.erb b/lib/generators/api_keys/templates/add_restrictions_to_api_keys.rb.erb index 1260804..ea230a7 100644 --- a/lib/generators/api_keys/templates/add_restrictions_to_api_keys.rb.erb +++ b/lib/generators/api_keys/templates/add_restrictions_to_api_keys.rb.erb @@ -17,7 +17,10 @@ # literal default on a JSON column, so there the default is the expression # JSON_OBJECT() (MySQL 8.0.13 or later). strong_migrations has no safe form of # a check constraint on an existing MySQL table, so it refuses this migration -# on MySQL. +# on MySQL: MySQL copies the table to add the constraint. Check the size of +# api_keys first. Then wrap the call of add_restrictions_object_constraint in +# up in `safety_assured do ... end`, or run the migration in a maintenance +# window. class AddRestrictionsToApiKeys < ActiveRecord::Migration<%= migration_version %> CONSTRAINT_NAME = "api_keys_restrictions_is_object" @@ -57,14 +60,15 @@ class AddRestrictionsToApiKeys < ActiveRecord::Migration<%= migration_version %> def add_restrictions_object_constraint return unless connection.supports_check_constraints? - expression = case connection.adapter_name.downcase - when /postg/ - "jsonb_typeof(restrictions) = 'object'" - when /sqlite/ - "json_valid(restrictions) AND json_type(restrictions) = 'object'" - when /mysql|trilogy/ - "JSON_TYPE(restrictions) = 'OBJECT'" - end + expression = + case connection.adapter_name.downcase + when /postg/ + "jsonb_typeof(restrictions) = 'object'" + when /sqlite/ + "json_valid(restrictions) AND json_type(restrictions) = 'object'" + when /mysql|trilogy/ + "JSON_TYPE(restrictions) = 'OBJECT'" + end return unless expression unless restrictions_constraint_exists? diff --git a/lib/generators/api_keys/templates/create_api_keys_device_authorizations.rb.erb b/lib/generators/api_keys/templates/create_api_keys_device_authorizations.rb.erb index 1bcf71c..cbcdd30 100644 --- a/lib/generators/api_keys/templates/create_api_keys_device_authorizations.rb.erb +++ b/lib/generators/api_keys/templates/create_api_keys_device_authorizations.rb.erb @@ -10,8 +10,6 @@ # Only apps that use the device flow need this table. class CreateApiKeysDeviceAuthorizations < ActiveRecord::Migration<%= migration_version %> def change - primary_key_type, foreign_key_type = primary_and_foreign_key_types - create_table :api_keys_device_authorizations, id: primary_key_type do |t| t.string :device_code_digest, null: false, index: { unique: true } t.string :user_code_digest, null: false, index: { unique: true } @@ -42,16 +40,23 @@ class CreateApiKeysDeviceAuthorizations < ActiveRecord::Migration<%= migration_v # The key types the install generator gives the api_keys table: the app's # `primary_key_type` generator setting, else Rails' defaults. - def primary_and_foreign_key_types + def primary_key_type + primary_key_type_setting || :primary_key + end + + def foreign_key_type + primary_key_type_setting || :bigint + end + + def primary_key_type_setting generators = Rails.application&.config&.generators - setting = generators&.options&.dig(generators.orm, :primary_key_type) - [setting || :primary_key, setting || :bigint] + generators&.options&.dig(generators.orm, :primary_key_type) end # The type of api_keys.id, so that the foreign key matches it exactly. def api_key_id_type column = connection.columns(:api_keys).find { |candidate| candidate.name == "id" } - return primary_and_foreign_key_types.last unless column + return foreign_key_type unless column sql_type = column.sql_type.to_s.downcase if sql_type.include?("uuid") diff --git a/lib/generators/api_keys/templates/create_api_keys_idempotency_keys.rb.erb b/lib/generators/api_keys/templates/create_api_keys_idempotency_keys.rb.erb index adc8ed3..69599d5 100644 --- a/lib/generators/api_keys/templates/create_api_keys_idempotency_keys.rb.erb +++ b/lib/generators/api_keys/templates/create_api_keys_idempotency_keys.rb.erb @@ -10,8 +10,6 @@ # Only apps that use ApiKeys::Idempotency need this table. class CreateApiKeysIdempotencyKeys < ActiveRecord::Migration<%= migration_version %> def change - primary_key_type, foreign_key_type = primary_and_foreign_key_types - create_table :api_keys_idempotency_keys, id: primary_key_type do |t| # Each API key has its own keys. A deleted API key takes them with it. t.references :api_key, null: false, type: api_key_id_type, foreign_key: { on_delete: :cascade }, index: false @@ -21,7 +19,7 @@ class CreateApiKeysIdempotencyKeys < ActiveRecord::Migration<%= migration_versio t.references :resource, polymorphic: true, null: true, type: foreign_key_type, index: false t.timestamps t.index :created_at - t.index [:api_key_id, :key], unique: true + t.index %i[api_key_id key], unique: true end end @@ -29,16 +27,23 @@ class CreateApiKeysIdempotencyKeys < ActiveRecord::Migration<%= migration_versio # The key types the install generator gives the api_keys table: the app's # `primary_key_type` generator setting, else Rails' defaults. - def primary_and_foreign_key_types + def primary_key_type + primary_key_type_setting || :primary_key + end + + def foreign_key_type + primary_key_type_setting || :bigint + end + + def primary_key_type_setting generators = Rails.application&.config&.generators - setting = generators&.options&.dig(generators.orm, :primary_key_type) - [setting || :primary_key, setting || :bigint] + generators&.options&.dig(generators.orm, :primary_key_type) end # The type of api_keys.id, so that the foreign key matches it exactly. def api_key_id_type column = connection.columns(:api_keys).find { |candidate| candidate.name == "id" } - return primary_and_foreign_key_types.last unless column + return foreign_key_type unless column sql_type = column.sql_type.to_s.downcase if sql_type.include?("uuid") diff --git a/lib/generators/api_keys/templates/create_api_keys_table.rb.erb b/lib/generators/api_keys/templates/create_api_keys_table.rb.erb index df33fbe..1260c70 100644 --- a/lib/generators/api_keys/templates/create_api_keys_table.rb.erb +++ b/lib/generators/api_keys/templates/create_api_keys_table.rb.erb @@ -5,8 +5,6 @@ class CreateApiKeysTable < ActiveRecord::Migration<%= migration_version %> RESTRICTIONS_CONSTRAINT_NAME = "api_keys_restrictions_is_object" def change - primary_key_type, foreign_key_type = primary_and_foreign_key_types - create_table :api_keys, id: primary_key_type do |t| # Identifiable prefix (e.g., "ak_live_", "ak_test_") for env/debugging t.string :prefix, null: false @@ -76,8 +74,8 @@ class CreateApiKeysTable < ActiveRecord::Migration<%= migration_version %> t.index :token_digest, unique: true # Index to optimize prefix-based lookups (especially for bcrypt) - t.index [:prefix, :digest_algorithm], name: "index_api_keys_on_prefix_and_digest_algorithm" - t.index [:prefix, :last4, :digest_algorithm], name: "index_api_keys_authentication_lookup" + t.index %i[prefix digest_algorithm], name: "index_api_keys_on_prefix_and_digest_algorithm" + t.index %i[prefix last4 digest_algorithm], name: "index_api_keys_authentication_lookup" # Optional indexes t.index :prefix @@ -87,7 +85,7 @@ class CreateApiKeysTable < ActiveRecord::Migration<%= migration_version %> t.index :last_used_at t.index :key_type t.index :environment - t.index [:owner_type, :owner_id, :key_type, :environment], name: "index_api_keys_owner_type_env" + t.index %i[owner_type owner_id key_type environment], name: "index_api_keys_owner_type_env" end add_restrictions_object_constraint @@ -95,14 +93,19 @@ class CreateApiKeysTable < ActiveRecord::Migration<%= migration_version %> private - # Helper method to determine the appropriate primary and foreign key types + # Helper methods to determine the appropriate primary and foreign key types # based on the Rails application's configuration. - def primary_and_foreign_key_types + def primary_key_type + primary_key_type_setting || :primary_key + end + + def foreign_key_type + primary_key_type_setting || :bigint # Assuming bigint is a safe default for foreign keys + end + + def primary_key_type_setting config = Rails.configuration.generators - setting = config.options[config.orm][:primary_key_type] - primary_key_type = setting || :primary_key - foreign_key_type = setting || :bigint # Assuming bigint is a safe default for foreign keys - [primary_key_type, foreign_key_type] + config.options[config.orm][:primary_key_type] end # Helper method to determine the appropriate JSON column type based on the database adapter. @@ -122,14 +125,15 @@ class CreateApiKeysTable < ActiveRecord::Migration<%= migration_version %> def add_restrictions_object_constraint return unless connection.supports_check_constraints? - expression = case connection.adapter_name.downcase - when /postgres/ - "jsonb_typeof(restrictions) = 'object'" - when /sqlite/ - "json_valid(restrictions) AND json_type(restrictions) = 'object'" - when /mysql|trilogy/ - "JSON_TYPE(restrictions) = 'OBJECT'" - end + expression = + case connection.adapter_name.downcase + when /postgres/ + "jsonb_typeof(restrictions) = 'object'" + when /sqlite/ + "json_valid(restrictions) AND json_type(restrictions) = 'object'" + when /mysql|trilogy/ + "JSON_TYPE(restrictions) = 'OBJECT'" + end add_check_constraint :api_keys, expression, name: RESTRICTIONS_CONSTRAINT_NAME if expression end end diff --git a/test/configuration_test.rb b/test/configuration_test.rb index 119d7cb..f97c934 100644 --- a/test/configuration_test.rb +++ b/test/configuration_test.rb @@ -4,6 +4,15 @@ module ApiKeys class ConfigurationTest < ApiKeys::Test + test "test_environment? is true only while Rails.env is test" do + assert ApiKeys.test_environment? + %w[production development ci].each do |environment| + Rails.stubs(:env).returns(ActiveSupport::EnvironmentInquirer.new(environment)) + + refute ApiKeys.test_environment?, environment + end + end + test "security-sensitive defaults fail closed" do assert ApiKeys.configuration.https_only_production assert ApiKeys.configuration.https_strict_mode diff --git a/test/databases/check.rb b/test/databases/check.rb index 10e6abd..7285b10 100644 --- a/test/databases/check.rb +++ b/test/databases/check.rb @@ -401,6 +401,41 @@ def test_h_the_lock_timeout_does_not_stay_on_the_connection else pass end end + + # A concurrent build that fails leaves an invalid index on PostgreSQL. The + # next run of the migration removes it and builds it again. A unique build + # over two equal rows fails in the same way as a lock timeout. + def test_i_a_failed_concurrent_build_is_built_again_on_the_next_run + skip "only PostgreSQL builds indexes concurrently" unless ADAPTER == "postgresql" + + connection = ActiveRecord::Base.connection + now = Time.current + twins = Array.new(2) do |index| + { prefix: "ak_twin_", token_digest: "twin-#{index}", digest_algorithm: "sha256", last4: "twin", + owner_type: "User", owner_id: @owner.id, created_by_type: "User", created_by_id: @owner.id, + key_type: "secret", environment: "test", created_at: now, updated_at: now } + end + ApiKeys::ApiKey.insert_all!(twins) + { + AddCreatedByToApiKeys => ["index_api_keys_on_created_by", %w[created_by_type created_by_id]], + AddKeyTypesToApiKeys => ["index_api_keys_owner_type_env", %w[owner_type owner_id key_type environment]], + AddAuthenticationIndexToApiKeys => ["index_api_keys_authentication_lookup", %w[prefix last4 digest_algorithm]] + }.each do |migration, (name, columns)| + connection.remove_index(:api_keys, name: name, algorithm: :concurrently) + assert_raises(ActiveRecord::RecordNotUnique) do + connection.execute("CREATE UNIQUE INDEX CONCURRENTLY #{name} ON api_keys (#{columns.join(", ")})") + end + broken = connection.indexes(:api_keys).find { |index| index.name == name } + refute broken.valid?, "precondition: #{name} is invalid" + + migration.migrate(:up) + + rebuilt = connection.indexes(:api_keys).find { |index| index.name == name } + assert rebuilt.valid?, "#{migration}: #{name} is valid" + refute rebuilt.unique, "#{migration}: #{name} is the migration's index" + assert_equal columns, rebuilt.columns + end + end end Minitest.after_run do diff --git a/test/generators/add_authentication_index_generator_test.rb b/test/generators/add_authentication_index_generator_test.rb index d345d67..89cef6c 100644 --- a/test/generators/add_authentication_index_generator_test.rb +++ b/test/generators/add_authentication_index_generator_test.rb @@ -18,7 +18,8 @@ class AddAuthenticationIndexGeneratorTest < Rails::Generators::TestCase assert_includes migration, 'INDEX_NAME = "index_api_keys_authentication_lookup"' assert_includes migration, "COLUMNS = %i[prefix last4 digest_algorithm].freeze" assert_includes migration, "index_exists?(:api_keys, COLUMNS, name: INDEX_NAME)" - assert_includes migration, "algorithm] = :concurrently" + assert_includes migration, "{ algorithm: :concurrently }" + assert_includes migration, "remove_invalid_index" assert_includes migration, "disable_ddl_transaction!" refute_includes migration, "def migration_version" end diff --git a/test/generators/add_key_types_generator_test.rb b/test/generators/add_key_types_generator_test.rb index 27d4b27..d2fd6ac 100644 --- a/test/generators/add_key_types_generator_test.rb +++ b/test/generators/add_key_types_generator_test.rb @@ -40,7 +40,8 @@ def index_columns assert_includes migration, "class AddKeyTypesToApiKeys < ActiveRecord::Migration[" assert_includes migration, "disable_ddl_transaction!" assert_includes migration, "add_column :api_keys, :key_type, :string unless column_exists?(:api_keys, :key_type)" - assert_includes migration, "add_index :api_keys, :key_type, **index_algorithm unless index_exists?" + assert_includes migration, "add_index :api_keys, column, **index_algorithm unless index_exists?(:api_keys, column)" + assert_includes migration, "remove_invalid_index(OWNER_TYPE_ENV_COLUMNS)" assert_includes migration, 'OWNER_TYPE_ENV_INDEX = "index_api_keys_owner_type_env"' assert_includes migration, "with_safety_assured do" assert_includes migration, "{ algorithm: :concurrently }" diff --git a/test/generators/generated_migrations_style_test.rb b/test/generators/generated_migrations_style_test.rb new file mode 100644 index 0000000..f10e2a9 --- /dev/null +++ b/test/generators/generated_migrations_style_test.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require "test_helper" +require "open3" +require "rbconfig" +require "stringio" +require "tmpdir" +require "rails/generators" +%w[install add_key_types add_authentication_index add_restrictions add_key_activity add_created_by + add_device_authorizations add_idempotency_keys].each { |name| require "generators/api_keys/#{name}_generator" } + +module ApiKeys + module Generators + # A host app on rubocop-rails-omakase (the Rails default) lints the + # migrations the generators write into db/migrate, so each one must pass + # as generated. + class GeneratedMigrationsStyleTest < ActiveSupport::TestCase + GENERATORS = [ + InstallGenerator, AddKeyTypesGenerator, AddAuthenticationIndexGenerator, AddRestrictionsGenerator, + AddKeyActivityGenerator, AddCreatedByGenerator, AddDeviceAuthorizationsGenerator, AddIdempotencyKeysGenerator + ].freeze + + test "every generated migration passes rubocop-rails-omakase" do + Dir.mktmpdir("api_keys_style") do |dir| + GENERATORS.each do |generator| + $stdout = StringIO.new + generator.start([], destination_root: dir) + ensure + $stdout = STDOUT + end + migrations = Dir["#{dir}/db/migrate/*.rb"].sort + assert_equal GENERATORS.size, migrations.size + + config = File.join(dir, ".rubocop.yml") + File.write(config, "inherit_gem: { rubocop-rails-omakase: rubocop.yml }\n") + output, status = Open3.capture2e(RbConfig.ruby, Gem.bin_path("rubocop", "rubocop"), "--config", config, + "--cache", "false", "--format", "simple", *migrations, chdir: dir) + + assert status.success?, output + assert_match "#{migrations.size} files inspected, no offenses detected", output + end + end + end + end +end diff --git a/test/generators/install_generator_test.rb b/test/generators/install_generator_test.rb index 780a56e..9764141 100644 --- a/test/generators/install_generator_test.rb +++ b/test/generators/install_generator_test.rb @@ -16,7 +16,7 @@ class InstallGeneratorTest < Rails::Generators::TestCase assert_migration "db/migrate/create_api_keys_table.rb" do |migration| assert_includes migration, "t.references :owner, polymorphic: true" - assert_includes migration, "[:owner_type, :owner_id, :key_type, :environment]" + assert_includes migration, "%i[owner_type owner_id key_type environment]" assert_includes migration, "jsonb_typeof(restrictions) = 'object'" assert_includes migration, "json_type(restrictions) = 'object'" refute_match(/^\s*t\.index :owner_id\b/, migration) diff --git a/test/helpers/token_session_transactional_test.rb b/test/helpers/token_session_transactional_test.rb index d936535..4f2c212 100644 --- a/test/helpers/token_session_transactional_test.rb +++ b/test/helpers/token_session_transactional_test.rb @@ -59,6 +59,7 @@ def assert_only_the_test_transaction_is_open "ApiKey.transaction" => ->(&work) { ApiKeys::ApiKey.transaction(&work) }, "a host model's transaction" => ->(&work) { User.transaction(&work) }, "a savepoint (requires_new)" => ->(&work) { ApiKeys::ApiKey.transaction(requires_new: true, &work) }, + # The shape of fixtures and DatabaseCleaner's :transaction together. "a non-joinable transaction" => ->(&work) { ApiKeys::ApiKey.transaction(joinable: false, &work) } }.each do |label, wrap| retrieval = nil @@ -74,13 +75,15 @@ def assert_only_the_test_transaction_is_open end test "outside the test environment the same transaction refuses the retrieval" do - Rails.stubs(:env).returns(ActiveSupport::EnvironmentInquirer.new("production")) + %w[production ci].each do |environment| + Rails.stubs(:env).returns(ActiveSupport::EnvironmentInquirer.new(environment)) - retrieval = TokenSession.retrieve_once_with_status(@session, api_key: @key) + retrieval = TokenSession.retrieve_once_with_status(@session, api_key: @key) - assert_equal :inside_transaction, retrieval.status - assert_nil retrieval.token - refute @key.reload.token_shown? + assert_equal :inside_transaction, retrieval.status, environment + assert_nil retrieval.token, environment + refute @key.reload.token_shown?, environment + end end end @@ -132,6 +135,33 @@ def with_file_database end end + # The documented boundary: the exemption reads the shape of the + # transaction, so in a test without a transaction of its own, an outer + # non-joinable transaction that the app opens is exempt too, and a + # rollback of it undoes the claim. Production refuses it. Pinned so the + # docs stay true. + test "in a test without its own transaction, an outer non-joinable transaction is exempt too" do + key = User.create!(name: "Non-joinable owner").create_api_key!(name: "Non-joinable key") + session = {} + TokenSession.store(session, key) + copied_cookie = session.deep_dup + retrieval = nil + + ApiKeys::ApiKey.transaction(joinable: false) do + retrieval = TokenSession.retrieve_once_with_status(session, api_key: key) + raise ActiveRecord::Rollback + end + + assert_equal :retrieved, retrieval.status, "exempt: the shape of a test transaction" + refute key.reload.token_shown?, "the rollback undid the claim" + Rails.stubs(:env).returns(ActiveSupport::EnvironmentInquirer.new("production")) + TokenSession.stubs(:log_warn) + ApiKeys::ApiKey.transaction(joinable: false) do + retrieval = TokenSession.retrieve_once_with_status(copied_cookie, api_key: key) + end + assert_equal :inside_transaction, retrieval.status, "production refuses the same transaction" + end + test "the claim commits before the token is returned, and a copy of the session gets nothing" do with_file_database do key = User.create!(name: "Committing owner").create_api_key!(name: "Committed key") diff --git a/test/models/idempotency_key_test.rb b/test/models/idempotency_key_test.rb index e81f8e7..4612fc4 100644 --- a/test/models/idempotency_key_test.rb +++ b/test/models/idempotency_key_test.rb @@ -285,7 +285,7 @@ class AddIdempotencyKeysGeneratorTest < Rails::Generators::TestCase assert_includes migration, "class CreateApiKeysIdempotencyKeys < ActiveRecord::Migration[" assert_includes migration, "create_table :api_keys_idempotency_keys" assert_includes migration, "foreign_key: { on_delete: :cascade }" - assert_includes migration, "t.index [:api_key_id, :key], unique: true" + assert_includes migration, "t.index %i[api_key_id key], unique: true" refute_match(/:body|:response_body/, migration) refute_includes migration, "def migration_version" end diff --git a/test/trusted_key_check_test.rb b/test/trusted_key_check_test.rb index 393deb9..718d32e 100644 --- a/test/trusted_key_check_test.rb +++ b/test/trusted_key_check_test.rb @@ -286,6 +286,7 @@ class AddCreatedByGeneratorTest < Rails::Generators::TestCase assert_includes migration, "unless column_exists?(:api_keys, :created_by_id)" assert_includes migration, "add_reference :api_keys, :created_by, polymorphic: true, null: true, " \ "type: foreign_key_type, index: false" + assert_includes migration, "remove_invalid_index(INDEX_COLUMNS)" assert_includes migration, "return if index_exists?(:api_keys, INDEX_COLUMNS)" assert_includes migration, 'INDEX_NAME = "index_api_keys_on_created_by"' assert_includes migration, "{ algorithm: :concurrently }" From 8033d5c17bccf912680d972a4a8de43b4798e4ed Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Mon, 26 Oct 2026 00:32:08 +0000 Subject: [PATCH 6/7] Build the probe's broken index without raw SQL Brakeman reads test/databases/check.rb with the engine, and it flagged the probe's CREATE UNIQUE INDEX, which interpolated the index name and columns into SQL. add_index with unique: true and algorithm: :concurrently builds the same failing index through Active Record. Co-Authored-By: Claude Opus 5.5 --- test/databases/check.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/databases/check.rb b/test/databases/check.rb index 7285b10..8e00b29 100644 --- a/test/databases/check.rb +++ b/test/databases/check.rb @@ -423,7 +423,7 @@ def test_i_a_failed_concurrent_build_is_built_again_on_the_next_run }.each do |migration, (name, columns)| connection.remove_index(:api_keys, name: name, algorithm: :concurrently) assert_raises(ActiveRecord::RecordNotUnique) do - connection.execute("CREATE UNIQUE INDEX CONCURRENTLY #{name} ON api_keys (#{columns.join(", ")})") + connection.add_index(:api_keys, columns, name: name, unique: true, algorithm: :concurrently) end broken = connection.indexes(:api_keys).find { |index| index.name == name } refute broken.valid?, "precondition: #{name} is invalid" From 0f501b5ada4b837ac5d1d306c017edf9e2c8d5a1 Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Mon, 26 Oct 2026 00:40:18 +0000 Subject: [PATCH 7/7] Correct the MySQL advice for add_restrictions On MySQL, strong_migrations refuses add_restrictions at add_column, for the callable JSON_OBJECT() default, before it reaches the check constraint. So the advice to wrap only the constraint call did not help, and a maintenance window alone does not get past strong_migrations. The README, the CHANGELOG and the migration comment now say: check the size of api_keys, wrap the whole body of up in safety_assured, and run it when the table can be locked for the copy. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- README.md | 2 +- .../templates/add_restrictions_to_api_keys.rb.erb | 12 ++++++------ 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1de67a8..bc1b99a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ Two fixes found when an app on the fleet upgraded from 0.5 to 0.8.0. The second - Every upgrade generator now writes a migration that strong_migrations accepts on PostgreSQL. Before, it refused three of them. Those three now run with `disable_ddl_transaction!`, and each step checks what exists, so a run that stopped half way can run again. An invalid index that a failed concurrent build left on PostgreSQL counts as absent: the next run removes it and builds it again (`add_created_by`, `add_key_types`, `add_authentication_index`). On MySQL and SQLite they run as before, except where this list says otherwise. A migration that an earlier version generated and that already ran needs nothing. - `add_created_by` (0.7.0) built the polymorphic index inside the migration's transaction, which blocks writes to `api_keys`. It now adds the columns, then builds `index_api_keys_on_created_by` with `algorithm: :concurrently` on PostgreSQL (and PostGIS). - `add_key_types` (0.3.0) built its three indexes inside the transaction. They are now built concurrently on PostgreSQL. strong_migrations calls the four-column index a best-practice problem on every database. The install migration has the same index, so the migration keeps it inside `safety_assured` when the app uses strong_migrations. - - `add_restrictions` (0.5.0) added its check constraint and checked every row under a lock that blocks reads and writes. On PostgreSQL it now adds the constraint without that check (`validate: false`), then validates it, which does not block reads and writes. On MySQL, which refuses a literal default on a JSON column, the column now defaults to the expression `JSON_OBJECT()` (MySQL 8.0.13 or later), so the migration runs and existing keys get an empty object. strong_migrations has no safe form of a check constraint on an existing MySQL table, so it still refuses this migration on MySQL. There, check the size of `api_keys`, then wrap the call of `add_restrictions_object_constraint` in `safety_assured`, or run the migration in a maintenance window. + - `add_restrictions` (0.5.0) added its check constraint and checked every row under a lock that blocks reads and writes. On PostgreSQL it now adds the constraint without that check (`validate: false`), then validates it, which does not block reads and writes. On MySQL, which refuses a literal default on a JSON column, the column now defaults to the expression `JSON_OBJECT()` (MySQL 8.0.13 or later), so the migration runs and existing keys get an empty object. On MySQL strong_migrations still refuses this migration twice: at `add_column`, for the callable default, and at the check constraint, which MySQL adds by copying the table. There, check the size of `api_keys`, then wrap the whole body of `up` in `safety_assured`, and run it when the table can be locked for the copy. - `add_authentication_index` (0.4.0) already built its index concurrently. It now also does so on PostGIS. - `add_key_activity`, `add_device_authorizations` and `add_idempotency_keys` already passed strong_migrations. - Every generated migration, the install included, passes rubocop-rails-omakase, the Rails default style. Array literals of symbols are `%i[]`, the key types are two methods instead of an array, and each `case` has its own line under `expression =`. Before, four files had 16 offenses. diff --git a/README.md b/README.md index 252da33..ed275b3 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,7 @@ And you're done! ### Upgrading an existing installation -Every upgrade generator below writes a migration that strong_migrations accepts on PostgreSQL (0.8.1). There the indexes on `api_keys` are built concurrently, and the restrictions check constraint is validated apart, outside a transaction, so writes to `api_keys` go on. If a concurrent build fails (a lock or statement timeout, for example), PostgreSQL keeps an invalid index; the next run of the migration removes it and builds it again. On MySQL strong_migrations accepts all of them except `add_restrictions`: it has no safe form of a check constraint on an existing MySQL table, because MySQL copies the table to add one. On MySQL with strong_migrations, check the size of `api_keys` first. Then wrap the call of `add_restrictions_object_constraint` in the generated migration in `safety_assured do ... end`, or run the migration in a maintenance window. +Every upgrade generator below writes a migration that strong_migrations accepts on PostgreSQL (0.8.1). There the indexes on `api_keys` are built concurrently, and the restrictions check constraint is validated apart, outside a transaction, so writes to `api_keys` go on. If a concurrent build fails (a lock or statement timeout, for example), PostgreSQL keeps an invalid index; the next run of the migration removes it and builds it again. On MySQL strong_migrations accepts all of them except `add_restrictions`, which it refuses twice: first at `add_column`, for the callable `JSON_OBJECT()` default (`add_column_default_callable`), then at the check constraint, which MySQL adds by copying the table. On MySQL with strong_migrations, check the size of `api_keys` first. Then wrap the whole body of `up` in the generated migration (the column and the constraint) in `safety_assured do ... end`, and run it when `api_keys` can be locked for the copy, for example in a maintenance window. Install the bounded bcrypt authentication lookup index before deploying the current hardening changes: diff --git a/lib/generators/api_keys/templates/add_restrictions_to_api_keys.rb.erb b/lib/generators/api_keys/templates/add_restrictions_to_api_keys.rb.erb index ea230a7..38da415 100644 --- a/lib/generators/api_keys/templates/add_restrictions_to_api_keys.rb.erb +++ b/lib/generators/api_keys/templates/add_restrictions_to_api_keys.rb.erb @@ -15,12 +15,12 @@ # the validation holds no lock from the earlier steps. Each step checks what # exists, so a run that stopped half way can run again. MySQL refuses a # literal default on a JSON column, so there the default is the expression -# JSON_OBJECT() (MySQL 8.0.13 or later). strong_migrations has no safe form of -# a check constraint on an existing MySQL table, so it refuses this migration -# on MySQL: MySQL copies the table to add the constraint. Check the size of -# api_keys first. Then wrap the call of add_restrictions_object_constraint in -# up in `safety_assured do ... end`, or run the migration in a maintenance -# window. +# JSON_OBJECT() (MySQL 8.0.13 or later). On MySQL strong_migrations refuses +# this migration twice: first the column, for its callable default, then the +# check constraint, which MySQL adds by copying the table. Check the size of +# api_keys first. Then wrap the whole body of up (the column and the +# constraint) in `safety_assured do ... end`, and run it when api_keys can be +# locked for the copy, for example in a maintenance window. class AddRestrictionsToApiKeys < ActiveRecord::Migration<%= migration_version %> CONSTRAINT_NAME = "api_keys_restrictions_is_object"