From a20cf3e02b00a0917ffcc8b7b69a392d77083fd5 Mon Sep 17 00:00:00 2001
From: Joshua Wehner <joshua@forem.com>
Date: Fri, 7 Apr 2023 17:04:45 +0200
Subject: [PATCH] Unique cross-model-slug check is likely misconfigured
 (#19263)

* Page unique_cross_model_slug seems misconfigured

* Update validator test

* More verbose invalid attribute message

* Try dynamic model/attribute registration

* Rubocop

* Use the cross-model-slug existence check

* Register with the dynamic cross-model-slug checker

* Cleanup

* Add missing keys to fr

* Update format regex

* Adjust validation: always presence, unique if changed

* Tests can create reserved-word pages when they need to

* Slugs can be mixed case?

* Case-sensitive is better, actually

* Refactor to use CrossModel check

* Refactor, rename for clarity

* Refactor, avoid mocking oneself

* Refactor, injectable everything

* Add reservedword check to extracted exists? checker

* Move to concerns

* Without dynamic registration

* extend when needed

* Cleanup comments, remove registration references

---------

Co-authored-by: Goran <gorang.pub@gmail.com>
---
 app/models/concerns/unique_across_models.rb   |  8 +++
 app/models/cross_model_slug.rb                | 37 +++++++++++
 app/models/organization.rb                    |  7 +--
 app/models/page.rb                            |  6 +-
 app/models/podcast.rb                         |  8 +--
 app/models/user.rb                            | 14 ++---
 app/services/users/username_generator.rb      | 53 +++++++++-------
 app/validators/cross_model_slug_validator.rb  | 39 ++++++++++++
 .../unique_cross_model_slug_validator.rb      | 43 -------------
 config/locales/validators/en.yml              |  6 +-
 config/locales/validators/fr.yml              |  4 +-
 spec/factories/pages.rb                       |  7 +++
 spec/models/user_spec.rb                      |  4 +-
 .../services/users/username_generator_spec.rb | 63 ++++++++++++++-----
 spec/system/admin/admin_manages_pages_spec.rb |  3 +
 ....rb => cross_model_slug_validator_spec.rb} | 14 ++++-
 16 files changed, 205 insertions(+), 111 deletions(-)
 create mode 100644 app/models/concerns/unique_across_models.rb
 create mode 100644 app/models/cross_model_slug.rb
 create mode 100644 app/validators/cross_model_slug_validator.rb
 delete mode 100644 app/validators/unique_cross_model_slug_validator.rb
 rename spec/validators/{unique_cross_model_slug_validator_spec.rb => cross_model_slug_validator_spec.rb} (84%)

diff --git a/app/models/concerns/unique_across_models.rb b/app/models/concerns/unique_across_models.rb
new file mode 100644
index 000000000..fe24e8fb7
--- /dev/null
+++ b/app/models/concerns/unique_across_models.rb
@@ -0,0 +1,8 @@
+# extend to add :unique_across_models, which validates a slug or name across
+# all "slug-like" models via CrossModelSlug
+module UniqueAcrossModels
+  def unique_across_models(attribute, **options)
+    validates attribute, presence: true
+    validates attribute, cross_model_slug: true, **options, if: :"#{attribute}_changed?"
+  end
+end
diff --git a/app/models/cross_model_slug.rb b/app/models/cross_model_slug.rb
new file mode 100644
index 000000000..135a35f3b
--- /dev/null
+++ b/app/models/cross_model_slug.rb
@@ -0,0 +1,37 @@
+#
+# We have a simple top-level route equivalent to `/slug`. Because of this,
+# we want to verify that newly created records don't overlap with a previously-
+# defined `slug` -- in other words, slug values should be unique across all
+# relevant models. Except also, models might use "username" instead of "slug".
+#
+# "Slug-like" models are all included in a cross-model-uniqueness check. An
+# impacted models are checked for the existence of a record matching a given
+# value. Additionally, we have some special cases (eg, sitemap) that we want to
+# apply across all registered models.
+#
+class CrossModelSlug
+  MODELS = {
+    "User" => :username,
+    "Page" => :slug,
+    "Podcast" => :slug,
+    "Organization" => :slug
+  }.freeze
+
+  class << self
+    def exists?(value)
+      # Presence check is likely redundant, but is **much** cheaper than the
+      # cross-model check
+      return false if value.blank?
+
+      value = value.downcase
+
+      # Reserved check may be redundant, but allows this to be used outside of Validator
+      return true if ReservedWords.all.include?(value)
+      return true if value.include?("sitemap-") # https://github.com/forem/forem/pull/6704
+
+      MODELS.detect do |class_name, attribute|
+        class_name.constantize.exists?({ attribute => value })
+      end
+    end
+  end
+end
diff --git a/app/models/organization.rb b/app/models/organization.rb
index 606bd7b28..26b923715 100644
--- a/app/models/organization.rb
+++ b/app/models/organization.rb
@@ -3,9 +3,9 @@ class Organization < ApplicationRecord
 
   include Images::Profile.for(:profile_image_url)
 
+  extend UniqueAcrossModels
   COLOR_HEX_REGEXP = /\A#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})\z/
   INTEGER_REGEXP = /\A\d+\z/
-  SLUG_REGEXP = /\A[a-zA-Z0-9\-_]+\z/
 
   acts_as_followable
 
@@ -47,9 +47,6 @@ class Organization < ApplicationRecord
   validates :proof, length: { maximum: 1500 }
   validates :secret, length: { is: 100 }, allow_nil: true
   validates :secret, uniqueness: true
-  validates :slug, exclusion: { in: ReservedWords.all, message: :reserved_word }
-  validates :slug, format: { with: SLUG_REGEXP }, length: { in: 2..30 }
-  validates :slug, presence: true, uniqueness: { case_sensitive: false }
   validates :spent_credits_count, presence: true
   validates :summary, length: { maximum: 250 }
   validates :tag_line, length: { maximum: 60 }
@@ -59,7 +56,7 @@ class Organization < ApplicationRecord
   validates :unspent_credits_count, presence: true
   validates :url, length: { maximum: 200 }, url: { allow_blank: true, no_local: true }
 
-  validates :slug, unique_cross_model_slug: true, if: :slug_changed?
+  unique_across_models :slug, length: { in: 2..30 }
 
   mount_uploader :profile_image, ProfileImageUploader
   mount_uploader :nav_image, ProfileImageUploader
diff --git a/app/models/page.rb b/app/models/page.rb
index d657b4afc..5b47311ea 100644
--- a/app/models/page.rb
+++ b/app/models/page.rb
@@ -1,4 +1,5 @@
 class Page < ApplicationRecord
+  extend UniqueAcrossModels
   TEMPLATE_OPTIONS = %w[contained full_within_layout nav_bar_included json].freeze
 
   TERMS_SLUG = "terms".freeze
@@ -7,11 +8,10 @@ class Page < ApplicationRecord
 
   validates :title, presence: true
   validates :description, presence: true
-  validates :slug, presence: true, format: /\A[0-9a-z\-_]*\z/
   validates :template, inclusion: { in: TEMPLATE_OPTIONS }
   validate :body_present
-  validates :slug, unique_cross_model_slug: true, if: :slug_changed?
-  validates :slug, uniqueness: true
+
+  unique_across_models :slug
 
   before_validation :set_default_template
   before_save :evaluate_markdown
diff --git a/app/models/podcast.rb b/app/models/podcast.rb
index 65d04389d..edad43d80 100644
--- a/app/models/podcast.rb
+++ b/app/models/podcast.rb
@@ -17,13 +17,9 @@ class Podcast < ApplicationRecord
   validates :main_color_hex, :title, :feed_url, :image, presence: true
   validates :main_color_hex, format: /\A([a-fA-F]|[0-9]){6}\Z/
   validates :feed_url, uniqueness: true, url: { schemes: %w[https http] }
-  validates :slug,
-            presence: true,
-            uniqueness: true,
-            format: { with: /\A[a-zA-Z0-9\-_]+\Z/ },
-            exclusion: { in: ReservedWords.all, message: I18n.t("models.podcast.slug_is_reserved") }
 
-  validates :slug, unique_cross_model_slug: true, if: :slug_changed?
+  extend UniqueAcrossModels
+  unique_across_models :slug
 
   after_save :bust_cache
 
diff --git a/app/models/user.rb b/app/models/user.rb
index bc22e25e2..e496f9091 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -24,8 +24,8 @@ class User < ApplicationRecord
 
   include StringAttributeCleaner.nullify_blanks_for(:email)
 
+  extend UniqueAcrossModels
   USERNAME_MAX_LENGTH = 30
-  USERNAME_REGEXP = /\A[a-zA-Z0-9_]+\z/
   # follow the syntax in https://interledger.org/rfcs/0026-payment-pointers/#payment-pointer-syntax
   PAYMENT_POINTER_REGEXP = %r{
     \A                # start
@@ -136,14 +136,6 @@ class User < ApplicationRecord
   validates :spent_credits_count, presence: true
   validates :subscribed_to_user_subscriptions_count, presence: true
   validates :unspent_credits_count, presence: true
-  validates :username, length: { in: 2..USERNAME_MAX_LENGTH }, format: USERNAME_REGEXP
-  validates :username, presence: true, exclusion: {
-    in: ReservedWords.all,
-    message: proc { I18n.t("models.user.username_is_reserved") }
-  }
-  validates :username, uniqueness: { case_sensitive: false, message: lambda do |_obj, data|
-    I18n.t("models.user.is_taken", username: (data[:value]))
-  end }, if: :username_changed?
 
   # add validators for provider related usernames
   Authentication::Providers.username_fields.each do |username_field|
@@ -158,7 +150,9 @@ class User < ApplicationRecord
   end
 
   validate :non_banished_username, :username_changed?
-  validates :username, unique_cross_model_slug: true, if: :username_changed?
+
+  unique_across_models :username, length: { in: 2..USERNAME_MAX_LENGTH }
+
   validate :can_send_confirmation_email
   validate :update_rate_limit
   # NOTE: when updating the password on a Devise enabled model, the :encrypted_password
diff --git a/app/services/users/username_generator.rb b/app/services/users/username_generator.rb
index f6c3eacd7..b242de61f 100644
--- a/app/services/users/username_generator.rb
+++ b/app/services/users/username_generator.rb
@@ -7,46 +7,55 @@
 # @todo Extract username validation in separate class
 module Users
   class UsernameGenerator
+    attr_reader :usernames
+
     def self.call(...)
       new(...).call
     end
 
-    # @param list [Array<String>] a list of usernames
-    def initialize(list = [])
-      @list = list
+    # @param usernames [Array<String>] a list of usernames
+    def initialize(usernames = [], detector: CrossModelSlug, generator: nil)
+      @detector = detector
+      @generator = generator || method(:random_username)
+      @usernames = usernames
     end
 
     def call
-      from_list(modified_list) || from_list(list_with_suffix) || from_list(Array.new(3) { random_letters })
+      first_available_from(normalized_usernames) ||
+        first_available_from(suffixed_usernames) ||
+        first_available_from(random_usernames)
+    end
+
+    def normalized_usernames
+      @normalized_usernames ||= filtered_usernames.map { |s| s.downcase.gsub(/[^0-9a-z_]/i, "").delete(" ") }
+    end
+
+    def filtered_usernames
+      @filtered_usernames ||= usernames.select { |s| s.is_a?(String) && s.present? }
+    end
+
+    def random_username
+      ("a".."z").to_a.sample(12).join
+    end
+
+    def random_usernames
+      Array.new(3) { @generator.call }
     end
 
     private
 
-    def from_list(list)
+    def first_available_from(list)
       list.detect { |username| !username_exists?(username) }
     end
 
     def username_exists?(username)
-      User.exists?(username: username) ||
-        Organization.exists?(slug: username) ||
-        Page.exists?(slug: username) ||
-        Podcast.exists?(slug: username)
+      @detector.exists?(username)
     end
 
-    def filtered_list
-      @list.select { |s| s.is_a?(String) && s.present? }
-    end
+    def suffixed_usernames
+      return [] unless filtered_usernames.any?
 
-    def modified_list
-      filtered_list.map { |s| s.downcase.gsub(/[^0-9a-z_]/i, "").delete(" ") }
-    end
-
-    def list_with_suffix
-      modified_list.map { |s| [s, rand(100)].join("_") }
-    end
-
-    def random_letters
-      ("a".."z").to_a.sample(12).join
+      normalized_usernames.map { |stem| [stem, rand(100)].join("_") }
     end
   end
 end
diff --git a/app/validators/cross_model_slug_validator.rb b/app/validators/cross_model_slug_validator.rb
new file mode 100644
index 000000000..fbe7bb5c2
--- /dev/null
+++ b/app/validators/cross_model_slug_validator.rb
@@ -0,0 +1,39 @@
+##
+# Validates if the give attribute is used across the reserved spaces.
+class CrossModelSlugValidator < ActiveModel::EachValidator
+  FORMAT_REGEX = /\A[0-9a-z\-_]+\z/
+
+  def validate_each(record, attribute, value)
+    return if value.blank?
+
+    correct_format?(record, attribute, value)
+    not_on_reserved_list?(record, attribute, value)
+    unique_across_models?(record, attribute, value)
+  end
+
+  private
+
+  def not_on_reserved_list?(record, attribute, value)
+    return unless ReservedWords.all.include?(value)
+
+    record.errors.add(attribute, I18n.t("validators.cross_model_slug_validator.is_reserved"))
+  end
+
+  def correct_format?(record, attribute, value)
+    return if value.match?(FORMAT_REGEX)
+
+    record.errors.add(attribute, I18n.t("validators.cross_model_slug_validator.is_invalid"))
+  end
+
+  def unique_across_models?(record, attribute, value)
+    # attribute_changed? is likely redundant, but is much cheaper than the cross-model exists check
+    return unless record.public_send("#{attribute}_changed?")
+    return unless already_exists?(value)
+
+    record.errors.add(attribute, I18n.t("validators.cross_model_slug_validator.is_taken"))
+  end
+
+  def already_exists?(value)
+    CrossModelSlug.exists?(value.downcase)
+  end
+end
diff --git a/app/validators/unique_cross_model_slug_validator.rb b/app/validators/unique_cross_model_slug_validator.rb
deleted file mode 100644
index 10f5eeb7c..000000000
--- a/app/validators/unique_cross_model_slug_validator.rb
+++ /dev/null
@@ -1,43 +0,0 @@
-##
-# Validates if the give attribute is used across the reserved spaces.
-class UniqueCrossModelSlugValidator < ActiveModel::EachValidator
-  class_attribute :model_and_attribute_name_for_uniqueness_test
-
-  # Why a class attribute?  Allow for other implementations to extend
-  # this behavior.
-  self.model_and_attribute_name_for_uniqueness_test = {
-    Organization => :slug,
-    Page => :slug,
-    Podcast => :slug,
-    User => :username
-  }
-
-  def validate_each(record, attribute, value)
-    return unless already_exists?(value: value, record: record)
-
-    record.errors.add(attribute, options[:message] || I18n.t("validators.unique_cross_model_slug_validator.is_taken"))
-  end
-
-  private
-
-  ##
-  # Answers the question if it's okay for the record to use the given value.
-  #
-  # @param value [String] the value we're to check in the various classes
-  # @param record [ActiveRecord::Base] the record that we're attempting to validate
-  #
-  # @return [TrueClass] if the value already exists across the various classes.
-  # @return [FalseClass] if the given value is not already used.
-  #
-  # @see CLASS_AND_ATTRIBUTE_NAME_FOR_UNIQUENESS_TEST
-  def already_exists?(value:, record:)
-    return false unless value
-    return true if value.include?("sitemap-")
-
-    model_and_attribute_name_for_uniqueness_test.detect do |model, attribute|
-      next if record.is_a?(model)
-
-      model.exists?(attribute => value)
-    end
-  end
-end
diff --git a/config/locales/validators/en.yml b/config/locales/validators/en.yml
index 82e5430b0..a015352b3 100644
--- a/config/locales/validators/en.yml
+++ b/config/locales/validators/en.yml
@@ -11,7 +11,9 @@ en:
       default: must be a valid published Article identifier
     profile_validator:
       bio_too_long: Bio is too long
-    unique_cross_model_slug_validator:
-      is_taken: is taken
+    cross_model_slug_validator:
+      is_taken: has already been taken
+      is_invalid: is invalid
+      is_reserved: is reserved
     valid_domain_csv_validator:
       invalid_list_format: must be a comma-separated list of valid domains
diff --git a/config/locales/validators/fr.yml b/config/locales/validators/fr.yml
index 88c22376c..66822c9cf 100644
--- a/config/locales/validators/fr.yml
+++ b/config/locales/validators/fr.yml
@@ -11,7 +11,9 @@ fr:
       default: doit être un identifiant d'article publié valide
     profile_validator:
       bio_too_long: La biographie est trop longue
-    unique_cross_model_slug_validator:
+    cross_model_slug_validator:
       is_taken: est déjà pris
+      is_invalid: is invalid
+      is_reserved: is reserved
     valid_domain_csv_validator:
       invalid_list_format: doit être une liste de domaines valides, séparés par des virgules.
diff --git a/spec/factories/pages.rb b/spec/factories/pages.rb
index d0c2471a7..7cab9578c 100644
--- a/spec/factories/pages.rb
+++ b/spec/factories/pages.rb
@@ -5,5 +5,12 @@ FactoryBot.define do
     slug          { Faker::Internet.slug }
     description   { Faker::Lorem.sentence }
     template      { "contained" }
+
+    # Validations prevent creating pages with reserved words, but empty test
+    # database populated without these objects, this allows a bypass to create
+    # those pages for specific test scenarios.
+    trait :without_validations do
+      to_create { |instance| instance.save(validate: false) }
+    end
   end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index b424e57ea..acf308ae0 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -209,7 +209,7 @@ RSpec.describe User do
       context "when evaluating the custom error message for username uniqueness" do
         subject { create(:user, username: "test_user_123") }
 
-        it { is_expected.to validate_uniqueness_of(:username).with_message("test_user_123 is taken.").case_insensitive }
+        it { is_expected.to validate_uniqueness_of(:username).with_message("has already been taken").case_insensitive }
       end
       # rubocop:enable RSpec/NestedGroups
 
@@ -222,7 +222,7 @@ RSpec.describe User do
       create(:user, username: "test_user_123")
       same_username = build(:user, username: "test_user_123")
       expect(same_username).not_to be_valid
-      expect(same_username.errors[:username].to_s).to include("test_user_123 is taken.")
+      expect(same_username.errors[:username].to_s).to include("has already been taken")
     end
 
     it "validates username against reserved words" do
diff --git a/spec/services/users/username_generator_spec.rb b/spec/services/users/username_generator_spec.rb
index 9cb233a2b..ff0a50535 100644
--- a/spec/services/users/username_generator_spec.rb
+++ b/spec/services/users/username_generator_spec.rb
@@ -1,31 +1,64 @@
 require "rails_helper"
 
 RSpec.describe Users::UsernameGenerator, type: :service do
-  let(:user) { create(:user) }
+  let(:does_not_exist) do
+    detector = Object.new
+    def detector.exists?(_username)
+      false
+    end
+    detector
+  end
+
+  def result_from(usernames)
+    described_class.call(usernames, detector: does_not_exist)
+  end
 
   it "returns randomly generated username if empty list is passed" do
-    expect(described_class.call([""])).to be_present
+    expect(result_from([""])).to match(%([a-z]+{12}))
+    expect(result_from([])).to match(%([a-z]+{12}))
+  end
+
+  it "returns randomly generated username if bad list is passed" do
+    expect(result_from([nil, nil, 123, User])).to match(%([a-z]+{12}))
   end
 
   it "returns supplied username if does not exist" do
-    expect(described_class.call(["foo"])).to eq("foo")
+    expect(result_from(["username"])).to eq("username")
   end
 
-  it "returns modified username" do
-    expect(described_class.call(["foo.bar"])).to eq("foobar")
+  it "returns normalized username" do
+    expect(result_from(["user.name"])).to eq("username")
   end
 
-  it "returns supplied username with suffix if exists" do
-    expect(described_class.call([user.username])).to start_with("#{user.username}_")
-  end
+  context "when username already exists" do
+    subject(:result) { described_class.call ["username"], detector: username_exists }
 
-  it "returns randomly generated username" do
-    expect(described_class.call).to be_present
-  end
+    let(:username_exists) do
+      detector = Object.new
+      def detector.exists?(username)
+        username == "username"
+      end
+      detector
+    end
 
-  it "returns nil if all generation methods are exhausted" do
-    username_generator = described_class.new
-    allow(username_generator).to receive(:random_letters).and_return(user.username)
-    expect(username_generator.call).to be_nil
+    it "returns supplied username with suffix" do
+      expect(result).to match(/username_\d+/)
+    end
+
+    context "when all generation methods are exhausted" do
+      subject(:result) do
+        described_class.call [],
+                             detector: username_exists,
+                             generator: pseudo_random
+      end
+
+      let(:pseudo_random) do
+        -> { "username" }
+      end
+
+      it "returns nil" do
+        expect(result).to be_nil
+      end
+    end
   end
 end
diff --git a/spec/system/admin/admin_manages_pages_spec.rb b/spec/system/admin/admin_manages_pages_spec.rb
index 582639490..1cd4fd95d 100644
--- a/spec/system/admin/admin_manages_pages_spec.rb
+++ b/spec/system/admin/admin_manages_pages_spec.rb
@@ -80,17 +80,20 @@ RSpec.describe "Admin manages pages" do
   describe "when the defaults are overridden" do
     before do
       create(:page,
+             :without_validations, # validations prevent reserved_word "code-of-conduct"
              slug: "code-of-conduct",
              body_html: "<div>Code of Conduct</div>",
              title: "Code of Conduct",
              description: "A page that describes how to behave on this platform",
              is_top_level_path: true)
       create(:page,
+             :without_validations, # validations prevent reserved_word "privacy"
              slug: "privacy",
              body_html: "<div>Privacy Policy</div>",
              title: "Privacy Policy",
              description: "A page that describes the privacy policy", is_top_level_path: true)
       create(:page,
+             :without_validations, # validations prevent reserved_word "terms"
              slug: "terms",
              body_html: "<div>Terms of Use</div>",
              title: "Terms of Use",
diff --git a/spec/validators/unique_cross_model_slug_validator_spec.rb b/spec/validators/cross_model_slug_validator_spec.rb
similarity index 84%
rename from spec/validators/unique_cross_model_slug_validator_spec.rb
rename to spec/validators/cross_model_slug_validator_spec.rb
index 54d92db0d..1a75c8c4b 100644
--- a/spec/validators/unique_cross_model_slug_validator_spec.rb
+++ b/spec/validators/cross_model_slug_validator_spec.rb
@@ -1,6 +1,6 @@
 require "rails_helper"
 
-RSpec.describe UniqueCrossModelSlugValidator do
+RSpec.describe CrossModelSlugValidator do
   subject(:record) { validatable.new.tap { |m| m.name = name } }
 
   let(:validatable) do
@@ -15,10 +15,14 @@ RSpec.describe UniqueCrossModelSlugValidator do
         @enforce_validation = true
       end
 
+      def name_changed?
+        true
+      end
+
       attr_accessor :enforce_validation
       alias_method :enforce_validation?, :enforce_validation
 
-      validates :name, unique_cross_model_slug: true, if: :enforce_validation?
+      validates :name, cross_model_slug: true, if: :enforce_validation?
     end
   end
 
@@ -36,6 +40,12 @@ RSpec.describe UniqueCrossModelSlugValidator do
     it { is_expected.not_to be_valid }
   end
 
+  context "when name is a ReservedWord" do
+    let(:name) { "members" }
+
+    it { is_expected.not_to be_valid }
+  end
+
   context "when name exists in User model" do
     let(:user) { create(:user) }
     let(:name) { user.username }