Add polymorphic owner to ApiKey (#4212)

In preparation for ApiKeys belonging to trusted publishers in addition to users

Additionally, add a backfill so all ApiKeys will have an owner, and we can move to only read from that column, eventually deleting the user_id column

Gemfile

@@ -106,7 +106,8 @@ group :test do
   gem "rack-test", "~> 2.1", require: "rack/test"
   gem "rails-controller-testing", "~> 1.0"
   gem "mocha", "~> 2.0", require: false
-  gem "shoulda", "~> 4.0"
+  gem "shoulda-context", "~> 2.0"
+  gem "shoulda-matchers", "~> 5.3"
   gem "selenium-webdriver", "~> 4.8"
   gem "webmock", "~> 3.18"
   gem "simplecov", "~> 0.22", require: false

Gemfile.lock

@@ -594,12 +594,9 @@ GEM
       aws-sdk-core (>= 2)
       concurrent-ruby
       thor
-    shoulda (4.0.0)
-      shoulda-context (~> 2.0)
-      shoulda-matchers (~> 4.0)
     shoulda-context (2.0.0)
-    shoulda-matchers (4.4.1)
-      activesupport (>= 4.2.0)
+    shoulda-matchers (5.3.0)
+      activesupport (>= 5.2.0)
     simplecov (0.22.0)
       docile (~> 1.1)
       simplecov-html (~> 0.11)
@@ -779,7 +776,8 @@ DEPENDENCIES
   searchkick (~> 5.2)
   selenium-webdriver (~> 4.8)
   shoryuken (~> 6.1)
-  shoulda (~> 4.0)
+  shoulda-context (~> 2.0)
+  shoulda-matchers (~> 5.3)
   simplecov (~> 0.22)
   simplecov-cobertura (~> 2.1)
   sprockets-rails (~> 3.4)

app/models/api_key.rb

@@ -2,15 +2,18 @@ class ApiKey < ApplicationRecord
   API_SCOPES = %i[index_rubygems push_rubygem yank_rubygem add_owner remove_owner access_webhooks show_dashboard].freeze
   APPLICABLE_GEM_API_SCOPES = %i[push_rubygem yank_rubygem add_owner remove_owner].freeze
 
-  belongs_to :user
+  belongs_to :user, inverse_of: :api_keys
+  belongs_to :owner, polymorphic: true
 
   has_one :api_key_rubygem_scope, dependent: :destroy
   has_one :ownership, through: :api_key_rubygem_scope
   has_one :oidc_id_token, class_name: "OIDC::IdToken", dependent: :restrict_with_error
   has_one :oidc_api_key_role, through: :oidc_id_token, inverse_of: :api_key
   has_many :pushed_versions, class_name: "Version", inverse_of: :pusher_api_key, foreign_key: :pusher_api_key_id, dependent: :nullify
 
-  validates :user, :name, :hashed_key, presence: true
+  before_validation :set_owner_from_user
+
+  validates :name, :hashed_key, presence: true
   validate :exclusive_show_dashboard_scope, if: :can_show_dashboard?
   validate :scope_presence
   validates :name, length: { maximum: Gemcutter::MAX_FIELD_LENGTH }
@@ -115,4 +118,8 @@ def not_expired?
     return if changed == %w[expires_at]
     errors.add :base, "An expired API key cannot be used. Please create a new one." if expired?
   end
+
+  def set_owner_from_user
+    self.owner ||= user
+  end
 end

app/tasks/maintenance/backfill_api_key_owners_task.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class Maintenance::BackfillApiKeyOwnersTask < MaintenanceTasks::Task
+  def collection
+    ApiKey.where(owner_id: nil).or(ApiKey.where(owner_type: nil))
+  end
+
+  def process(api_key)
+    api_key.owner ||= api_key.user
+    api_key.save!(validate: false)
+  end
+end

db/migrate/20231102190427_add_owner_to_api_keys.rb

@@ -0,0 +1,7 @@
+class AddOwnerToApiKeys < ActiveRecord::Migration[7.0]
+  disable_ddl_transaction!
+
+  def change
+    add_reference :api_keys, :owner, polymorphic: true, null: true, index: {algorithm: :concurrently}
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2023_10_18_235829) do
+ActiveRecord::Schema[7.0].define(version: 2023_11_02_190427) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "hstore"
   enable_extension "pgcrypto"
@@ -54,7 +54,10 @@
     t.datetime "soft_deleted_at"
     t.string "soft_deleted_rubygem_name"
     t.datetime "expires_at", precision: nil
+    t.string "owner_type"
+    t.bigint "owner_id"
     t.index ["hashed_key"], name: "index_api_keys_on_hashed_key", unique: true
+    t.index ["owner_type", "owner_id"], name: "index_api_keys_on_owner"
     t.index ["user_id"], name: "index_api_keys_on_user_id"
   end
 

test/models/api_key_test.rb

@@ -2,15 +2,21 @@
 
 class ApiKeyTest < ActiveSupport::TestCase
   should belong_to :user
+  should belong_to :owner
   should validate_presence_of(:name)
-  should validate_presence_of(:user)
   should validate_presence_of(:hashed_key)
   should have_one(:api_key_rubygem_scope).dependent(:destroy)
 
   should "be valid with factory" do
     assert_predicate build(:api_key), :valid?
   end
 
+  should "set owner to user by default" do
+    api_key = create(:api_key)
+
+    assert_equal api_key.user, api_key.owner
+  end
+
   should "be invalid when name is empty string" do
     api_key = build(:api_key, name: "")
 

test/tasks/maintenance/backfill_api_key_owners_task_test.rb

@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+class Maintenance::BackfillApiKeyOwnersTaskTest < ActiveSupport::TestCase
+  test "#process performs a task iteration" do
+    element = create(:api_key)
+    element.update_columns(owner_id: nil, owner_type: nil)
+
+    assert_nil element.reload.owner
+
+    Maintenance::BackfillApiKeyOwnersTask.process(element)
+
+    assert_equal element.reload.user, element.owner
+  end
+
+  test "#collection returns the elements to process" do
+    create(:api_key)
+    nil_owner = create(:api_key, key: "other")
+    nil_owner.update_columns(owner_id: nil, owner_type: nil)
+
+    assert_nil nil_owner.reload.owner
+    assert_same_elements [nil_owner], Maintenance::BackfillApiKeyOwnersTask.collection
+  end
+end

test/test_helper.rb

@@ -28,7 +28,8 @@
 require "capybara/minitest"
 require "clearance/test_unit"
 require "webauthn/fake_client"
-require "shoulda"
+require "shoulda/context"
+require "shoulda/matchers"
 require "helpers/admin_helpers"
 require "helpers/gem_helpers"
 require "helpers/email_helpers"