diff --git a/Gemfile b/Gemfile index 56f7808ecde..a9ae2e76e9c 100644 --- a/Gemfile +++ b/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 diff --git a/Gemfile.lock b/Gemfile.lock index d26362f8280..d704846987c 100644 --- a/Gemfile.lock +++ b/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) diff --git a/app/models/api_key.rb b/app/models/api_key.rb index e34d9357ec9..064d37b2d10 100644 --- a/app/models/api_key.rb +++ b/app/models/api_key.rb @@ -2,7 +2,8 @@ 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 @@ -10,7 +11,9 @@ class ApiKey < ApplicationRecord 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 diff --git a/app/tasks/maintenance/backfill_api_key_owners_task.rb b/app/tasks/maintenance/backfill_api_key_owners_task.rb new file mode 100644 index 00000000000..ae6d13aee61 --- /dev/null +++ b/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 diff --git a/db/migrate/20231102190427_add_owner_to_api_keys.rb b/db/migrate/20231102190427_add_owner_to_api_keys.rb new file mode 100644 index 00000000000..3a47d2e7567 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 7f0cd97abff..aa18aa8c2e8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[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 diff --git a/test/models/api_key_test.rb b/test/models/api_key_test.rb index 1d7c0740f99..7f02a0f86cf 100644 --- a/test/models/api_key_test.rb +++ b/test/models/api_key_test.rb @@ -2,8 +2,8 @@ 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) @@ -11,6 +11,12 @@ class ApiKeyTest < ActiveSupport::TestCase 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: "") diff --git a/test/tasks/maintenance/backfill_api_key_owners_task_test.rb b/test/tasks/maintenance/backfill_api_key_owners_task_test.rb new file mode 100644 index 00000000000..1f833cf11b6 --- /dev/null +++ b/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 diff --git a/test/test_helper.rb b/test/test_helper.rb index 7ebe992745a..6198b1c8083 100644 --- a/test/test_helper.rb +++ b/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"