From e00bb7f6a1e623809258316d14e74cfde6f96f19 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Mon, 9 Oct 2023 15:57:37 -0700 Subject: [PATCH] Clarify Ownership uniqueness validations when owned/invited (#4119) --- app/models/ownership.rb | 15 +++++++- config/locales/de.yml | 5 +++ config/locales/en.yml | 5 +++ config/locales/es.yml | 5 +++ config/locales/fr.yml | 5 +++ config/locales/ja.yml | 5 +++ config/locales/nl.yml | 5 +++ config/locales/pt-BR.yml | 5 +++ config/locales/zh-CN.yml | 5 +++ config/locales/zh-TW.yml | 5 +++ .../api/v1/owners_controller_test.rb | 15 +++++++- test/models/ownership_test.rb | 36 +++++++++++++------ 12 files changed, 98 insertions(+), 13 deletions(-) diff --git a/app/models/ownership.rb b/app/models/ownership.rb index 327d34b9e9e..aefd33321dc 100644 --- a/app/models/ownership.rb +++ b/app/models/ownership.rb @@ -4,7 +4,7 @@ class Ownership < ApplicationRecord belongs_to :authorizer, class_name: "User" has_many :api_key_rubygem_scopes, dependent: :destroy - validates :user_id, uniqueness: { scope: :rubygem_id } + validate :validate_unique_user delegate :name, to: :user, prefix: :owner delegate :name, to: :authorizer, prefix: true, allow_nil: true @@ -72,4 +72,17 @@ def unconfirmed? def safe_destroy destroy if unconfirmed? || rubygem.owners.many? end + + def validate_unique_user + return unless rubygem && user + ownerships = persisted? ? Ownership.where.not(id: id) : Ownership + other = ownerships.find_by(rubygem:, user:) + return unless other + + if other.confirmed? + errors.add :user_id, I18n.t("activerecord.errors.models.ownership.attributes.user_id.already_confirmed") + else + errors.add :user_id, I18n.t("activerecord.errors.models.ownership.attributes.user_id.already_invited") + end + end end diff --git a/config/locales/de.yml b/config/locales/de.yml index 87130912d7e..8b2cfaf34da 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -55,6 +55,11 @@ de: unpwn: blocked: models: + ownership: + attributes: + user_id: + already_confirmed: + already_invited: version: attributes: gem_full_name: diff --git a/config/locales/en.yml b/config/locales/en.yml index f0588d251ea..b97885ebbaa 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -64,6 +64,11 @@ en: unpwn: has previously appeared in a data breach and should not be used blocked: "domain '%{domain}' has been blocked for spamming. Please use a valid personal email." models: + ownership: + attributes: + user_id: + already_confirmed: "is already an owner of this gem" + already_invited: "is already invited to this gem" version: attributes: gem_full_name: diff --git a/config/locales/es.yml b/config/locales/es.yml index 87c4ed5ea1e..12ed44f39d3 100644 --- a/config/locales/es.yml +++ b/config/locales/es.yml @@ -66,6 +66,11 @@ es: unpwn: blocked: models: + ownership: + attributes: + user_id: + already_confirmed: + already_invited: version: attributes: gem_full_name: diff --git a/config/locales/fr.yml b/config/locales/fr.yml index 2130bc18a49..696929d89ce 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -66,6 +66,11 @@ fr: unpwn: blocked: models: + ownership: + attributes: + user_id: + already_confirmed: + already_invited: version: attributes: gem_full_name: diff --git a/config/locales/ja.yml b/config/locales/ja.yml index b1f32779bfe..66976e36499 100644 --- a/config/locales/ja.yml +++ b/config/locales/ja.yml @@ -59,6 +59,11 @@ ja: unpwn: 過去にデータ侵害を受けたためお使いになれません blocked: ドメイン %{domain} はスパムのため差し止められました。正しい個人のEメールを使ってください。 models: + ownership: + attributes: + user_id: + already_confirmed: + already_invited: version: attributes: gem_full_name: diff --git a/config/locales/nl.yml b/config/locales/nl.yml index 8ecb0b4b9cb..17d7b71425d 100644 --- a/config/locales/nl.yml +++ b/config/locales/nl.yml @@ -58,6 +58,11 @@ nl: unpwn: blocked: models: + ownership: + attributes: + user_id: + already_confirmed: + already_invited: version: attributes: gem_full_name: diff --git a/config/locales/pt-BR.yml b/config/locales/pt-BR.yml index a36052631dd..5bb974ba689 100644 --- a/config/locales/pt-BR.yml +++ b/config/locales/pt-BR.yml @@ -65,6 +65,11 @@ pt-BR: unpwn: já apareceu anteriormente em um vazamento de dados e não deve ser utilizada blocked: models: + ownership: + attributes: + user_id: + already_confirmed: + already_invited: version: attributes: gem_full_name: diff --git a/config/locales/zh-CN.yml b/config/locales/zh-CN.yml index 69da9625ce1..263a0147095 100644 --- a/config/locales/zh-CN.yml +++ b/config/locales/zh-CN.yml @@ -60,6 +60,11 @@ zh-CN: unpwn: 曾出现过数据泄露，不应该再使用 blocked: 域名 '%{domain}' 因发送垃圾邮件已被禁用。请使用另外有效的个人邮箱。 models: + ownership: + attributes: + user_id: + already_confirmed: + already_invited: version: attributes: gem_full_name: diff --git a/config/locales/zh-TW.yml b/config/locales/zh-TW.yml index 287672f5eef..63cd0ab7d4b 100644 --- a/config/locales/zh-TW.yml +++ b/config/locales/zh-TW.yml @@ -55,6 +55,11 @@ zh-TW: unpwn: blocked: models: + ownership: + attributes: + user_id: + already_confirmed: + already_invited: version: attributes: gem_full_name: diff --git a/test/functional/api/v1/owners_controller_test.rb b/test/functional/api/v1/owners_controller_test.rb index d29d3c02f32..5f2d14f059c 100644 --- a/test/functional/api/v1/owners_controller_test.rb +++ b/test/functional/api/v1/owners_controller_test.rb @@ -241,7 +241,20 @@ def self.should_respond_to(format) should respond_with :unprocessable_entity should "respond with error message" do - assert_equal "User has already been taken", @response.body + assert_equal "User is already an owner of this gem", @response.body + end + end + + context "owner has already been invited" do + setup do + post :create, params: { rubygem_id: @rubygem.slug, email: @second_user.email } + post :create, params: { rubygem_id: @rubygem.slug, email: @second_user.email } + end + + should respond_with :unprocessable_entity + + should "respond with error message" do + assert_equal "User is already invited to this gem", @response.body end end diff --git a/test/models/ownership_test.rb b/test/models/ownership_test.rb index 9347a346321..5feed93f5f3 100644 --- a/test/models/ownership_test.rb +++ b/test/models/ownership_test.rb @@ -13,17 +13,6 @@ class OwnershipTest < ActiveSupport::TestCase should have_db_index %i[user_id rubygem_id] should have_many(:api_key_rubygem_scopes).dependent(:destroy) - context "with ownership" do - setup do - @ownership = create(:ownership) - create(:version, rubygem: @ownership.rubygem) - end - - subject { @ownership } - - should validate_uniqueness_of(:user_id).scoped_to(:rubygem_id) - end - context "by_indexed_gem_name" do setup do @ownership = create(:ownership) @@ -108,6 +97,31 @@ class OwnershipTest < ActiveSupport::TestCase refute_predicate ownership, :valid? assert_contains ownership.errors[:rubygem], "must exist" end + + should "not create with a duplicate unconfirmed user and rubygem" do + existing_ownership = create(:ownership, :unconfirmed) + ownership = build(:ownership, user: existing_ownership.user, rubygem: existing_ownership.rubygem) + + refute_predicate ownership, :valid? + assert_contains ownership.errors[:user_id], "is already invited to this gem" + end + + should "not create with a duplicate confirmed user and rubygem" do + existing_ownership = create(:ownership) + ownership = build(:ownership, user: existing_ownership.user, rubygem: existing_ownership.rubygem) + + refute_predicate ownership, :valid? + assert_contains ownership.errors[:user_id], "is already an owner of this gem" + end + + should "not update to a duplicate confirmed user and rubygem" do + existing_ownership = create(:ownership) + ownership = create(:ownership, :unconfirmed, rubygem: existing_ownership.rubygem) + ownership.user = existing_ownership.user + + refute_predicate ownership, :valid? + assert_contains ownership.errors[:user_id], "is already an owner of this gem" + end end context "#valid_confirmation_token?" do