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/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 2c8af60..bc1b99a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,28 @@ +## [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. 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. 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. + +### 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. 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 ### Added 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 bfb25c5..ed275b3 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. 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: ```bash @@ -66,6 +68,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, 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: ```bash @@ -91,7 +95,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 +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. 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. @@ -1879,6 +1884,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/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 ab8a124..001b9ac 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,36 @@ 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. + # + # 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 ApiKeys.test_environment? + 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/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 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..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?("postgres") - 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?("postgres") - 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 63f206c..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 @@ -9,13 +9,29 @@ # # 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; 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 + + 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 + remove_invalid_index(INDEX_COLUMNS) + 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 +46,22 @@ 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 + + # 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 325f25d..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 @@ -5,14 +5,71 @@ # # 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; the +# next run removes it and builds it again. 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) + + %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 + # 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 + + # 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 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..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 @@ -8,47 +8,86 @@ # 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). 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" + 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/ - "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 + 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? + # 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/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/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/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..8e00b29 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 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 +# connection. # # BUNDLE_GEMFILE=test/databases/Gemfile DB_ADAPTER=postgresql bundle exec ruby test/databases/check.rb # @@ -18,9 +20,17 @@ 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" require "generators/api_keys/add_idempotency_keys_generator" @@ -58,11 +68,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::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 @@ -71,8 +86,57 @@ class DatabaseCheckApp < Rails::Application $stdout = STDOUT end +# 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 + +# 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, checked: !UNCHECKED.include?(name)) + file = migration_file(name) + 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(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 +146,17 @@ 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.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: {} @@ -128,26 +197,108 @@ def timed [result, Process.clock_gettime(Process::CLOCK_MONOTONIC) - started] end - def test_a_the_new_migrations_run_up_down_and_up_again - tables = %i[api_keys_device_authorizations api_keys_idempotency_keys] + # 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 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 + 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 - AddCreatedByToApiKeys.migrate(:down) - CreateApiKeysIdempotencyKeys.migrate(:down) - CreateApiKeysDeviceAuthorizations.migrate(:down) - ApiKeys::ApiKey.reset_column_information + UPGRADES.reverse_each { |name| run_migration(name, :down) } + refute key_types_columns? + refute ApiKeys::ApiKey.restrictions_column? + refute ApiKeys::ApiKey.refusal_stats_columns? + refute ApiKeys::ApiKey.token_shown_at_column? refute ApiKeys::ApiKey.created_by_columns? - tables.each { |table| refute ActiveRecord::Base.connection.table_exists?(table) } + 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_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) - AddCreatedByToApiKeys.migrate(:up) # idempotent - CreateApiKeysDeviceAuthorizations.migrate(:up) - CreateApiKeysIdempotencyKeys.migrate(:up) - [ApiKeys::ApiKey, ApiKeys::DeviceAuthorization, Key].each(&:reset_column_information) - assert ApiKeys::ApiKey.created_by_columns? - tables.each { |table| assert ActiveRecord::Base.connection.table_exists?(table) } + 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 @@ -250,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.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" + + 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 new file mode 100644 index 0000000..d2fd6ac --- /dev/null +++ b/test/generators/add_key_types_generator_test.rb @@ -0,0 +1,112 @@ +# 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, 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 }" + 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 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 new file mode 100644 index 0000000..4f2c212 --- /dev/null +++ b/test/helpers/token_session_transactional_test.rb @@ -0,0 +1,193 @@ +# 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) }, + # 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 + 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 + %w[production ci].each do |environment| + Rails.stubs(:env).returns(ActiveSupport::EnvironmentInquirer.new(environment)) + + retrieval = TokenSession.retrieve_once_with_status(@session, api_key: @key) + + assert_equal :inside_transaction, retrieval.status, environment + assert_nil retrieval.token, environment + refute @key.reload.token_shown?, environment + end + 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 + + # 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") + 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 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 b82aec6..718d32e 100644 --- a/test/trusted_key_check_test.rb +++ b/test/trusted_key_check_test.rb @@ -275,18 +275,41 @@ 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, "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 }" 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 +322,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) }