FIX: min/max username length limits weren't validated (#17382)

* FIX: min/max username length limits weren't validated

The custom validators introduced in e0d7cda made so we ignored the mix
and max values set on site_settings.yml. That change allowed admins to
set values outside of the range defined on the yaml file.

Related to https://meta.discourse.org/t/group-names-with-more-than-60-characters-broken/232115?u=falco

Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>

config/site_settings.yml

@@ -542,14 +542,10 @@ users:
   min_username_length:
     client: true
     default: 3
-    min: 1
-    max: 60
     validator: "MinUsernameLengthValidator"
   max_username_length:
     client: true
     default: 20
-    min: 8
-    max: 60
     validator: "MaxUsernameLengthValidator"
   unicode_usernames:
     default: false

lib/site_settings/yaml_loader.rb

@@ -19,6 +19,10 @@ class SiteSettings::YamlLoader
             raise StandardError, "The site setting `#{setting_name}` in '#{@file}' is missing default value."
           end
 
+          if hash.values_at('min', 'max').any? && hash['validator'].present?
+            raise StandardError, "The site setting `#{setting_name}` in '#{@file}' will have it's min/max validation ignored because there is a validator also specified."
+          end
+
           yield category, setting_name, value, hash.deep_symbolize_keys!
         else
           # Simplest case. site_setting_name: 'default value'

lib/validators/max_username_length_validator.rb

@@ -1,18 +1,26 @@
 # frozen_string_literal: true
 
 class MaxUsernameLengthValidator
+  MAX_USERNAME_LENGTH_RANGE = 8..60
+
   def initialize(opts = {})
     @opts = opts
   end
 
   def valid_value?(value)
+    if !MAX_USERNAME_LENGTH_RANGE.cover?(value)
+      @max_range_violation = true
+      return false
+    end
     return false if value < SiteSetting.min_username_length
     @username = User.where('length(username) > ?', value).pluck_first(:username)
     @username.blank?
   end
 
   def error_message
-    if @username.blank?
+    if @max_range_violation
+      I18n.t('site_settings.errors.invalid_integer_min_max', min: MAX_USERNAME_LENGTH_RANGE.begin, max: MAX_USERNAME_LENGTH_RANGE.end)
+    elsif @username.blank?
       I18n.t("site_settings.errors.max_username_length_range")
     else
       I18n.t("site_settings.errors.max_username_length_exists", username: @username)

lib/validators/min_username_length_validator.rb

@@ -1,18 +1,26 @@
 # frozen_string_literal: true
 
 class MinUsernameLengthValidator
+  MIN_USERNAME_LENGTH_RANGE = 1..60
+
   def initialize(opts = {})
     @opts = opts
   end
 
   def valid_value?(value)
+    if !MIN_USERNAME_LENGTH_RANGE.cover?(value)
+      @min_range_violation = true
+      return false
+    end
     return false if value > SiteSetting.max_username_length
     @username = User.where('length(username) < ?', value).pluck_first(:username)
     @username.blank?
   end
 
   def error_message
-    if @username.blank?
+    if @min_length_violation
+      I18n.t('site_settings.errors.invalid_integer_min_max', min: MIN_USERNAME_LENGTH_RANGE.begin, max: MIN_USERNAME_LENGTH_RANGE.end)
+    elsif @username.blank?
       I18n.t("site_settings.errors.min_username_length_range")
     else
       I18n.t("site_settings.errors.min_username_length_exists", username: @username)

spec/lib/validators/max_username_length_validator_spec.rb

@@ -2,13 +2,32 @@
 
 describe MaxUsernameLengthValidator do
   it "checks for minimum range" do
-    SiteSetting.min_username_length = 6
+    User.update_all('username = username || username')
+    SiteSetting.min_username_length = 9
 
     validator = described_class.new
-    expect(validator.valid_value?(5)).to eq(false)
+    expect(validator.valid_value?(8)).to eq(false)
     expect(validator.error_message).to eq(I18n.t("site_settings.errors.max_username_length_range"))
   end
 
+  context "checks for valid ranges" do
+    it "fails for values below the valid range" do
+      expect do
+        SiteSetting.max_username_length = 5
+      end.to raise_error(Discourse::InvalidParameters)
+    end
+    it "fails for values above the valid range" do
+      expect do
+        SiteSetting.max_username_length = 61
+      end.to raise_error(Discourse::InvalidParameters)
+    end
+    it "works for values within the valid range" do
+      expect do
+        SiteSetting.max_username_length = 42
+      end.not_to raise_error
+    end
+  end
+
   it "checks for users with short usernames" do
     user = Fabricate(:user, username: 'jackjackjack')
 

spec/lib/validators/min_username_length_validator_spec.rb

@@ -9,6 +9,24 @@ describe MinUsernameLengthValidator do
     expect(validator.error_message).to eq(I18n.t("site_settings.errors.min_username_length_range"))
   end
 
+  context "checks for valid ranges" do
+    it "fails for values below the valid range" do
+      expect do
+        SiteSetting.min_username_length = 0
+      end.to raise_error(Discourse::InvalidParameters)
+    end
+    it "fails for values above the valid range" do
+      expect do
+        SiteSetting.min_username_length = 61
+      end.to raise_error(Discourse::InvalidParameters)
+    end
+    it "works for values within the valid range" do
+      expect do
+        SiteSetting.min_username_length = 4
+      end.not_to raise_error
+    end
+  end
+
   it "checks for users with short usernames" do
     user = Fabricate(:user, username: 'jack')