FIX: Improve group name validation (#31315)

This commit improves a few aspects regarding group name validation:

- `min_username_length` and `max_username_length` are validated with the
  shortest and longest names of users and groups

- skip validation of the group name when the group is an automatic one

app/models/group.rb

@@ -503,7 +503,7 @@ class Group < ActiveRecord::Base
   end
 
   def self.can_use_name?(name, group)
-    UsernameValidator.new(name).valid_format? &&
+    UsernameValidator.new(name, skip_length_validation: group.automatic).valid_format? &&
       (group.name == name || !User.username_exists?(name))
   end
 
@@ -1123,7 +1123,7 @@ class Group < ActiveRecord::Base
       self.name = stripped
     end
 
-    UsernameValidator.perform_validation(self, "name") ||
+    UsernameValidator.perform_validation(self, "name", skip_length_validation: automatic) ||
       begin
         normalized_name = User.normalize_username(self.name)
 

app/models/username_validator.rb

@@ -8,20 +8,22 @@ class UsernameValidator
   # field_name - name of the field that we're validating
   #
   # Example: UsernameValidator.perform_validation(user, 'name')
-  def self.perform_validation(object, field_name)
+  def self.perform_validation(object, field_name, opts = {})
-    validator = UsernameValidator.new(object.public_send(field_name))
+    validator = UsernameValidator.new(object.public_send(field_name), **opts)
     unless validator.valid_format?
       validator.errors.each { |e| object.errors.add(field_name.to_sym, e) }
     end
   end
 
-  def initialize(username)
+  def initialize(username, skip_length_validation: false)
     @username = username&.unicode_normalize
+    @skip_length_validation = skip_length_validation
     @errors = []
   end
 
   attr_accessor :errors
   attr_reader :username
+  attr_reader :skip_length_validation
 
   def user
     @user ||= User.new(user)
@@ -29,8 +31,8 @@ class UsernameValidator
 
   def valid_format?
     username_present?
-    username_length_min?
+    username_length_min? if !skip_length_validation
-    username_length_max?
+    username_length_max? if !skip_length_validation
     username_char_valid?
     username_char_allowed?
     username_first_char_valid?

config/locales/server.en.yml

@@ -2802,8 +2802,10 @@ en:
       discourse_connect_url_is_empty: "You must set a 'discourse connect url' before enabling this setting."
       enable_local_logins_disabled: "You must first enable 'enable local logins' before enabling this setting."
       min_username_length_exists: "You cannot set the minimum username length above the shortest username (%{username})."
+      min_group_name_length_exists: "You cannot set the minimum username length above the shortest group name (%{group_name})."
       min_username_length_range: "You cannot set the minimum above the maximum."
       max_username_length_exists: "You cannot set the maximum username length below the longest username (%{username})."
+      max_group_name_length_exists: "You cannot set the maximum username length below the longest group name (%{group_name})."
       max_username_length_range: "You cannot set the maximum below the minimum."
       invalid_hex_value: "Color values have to be 6-digit hexadecimal codes."
       empty_selectable_avatars: "You must first upload at least two selectable avatars before enabling this setting."

lib/validators/max_username_length_validator.rb

@@ -12,9 +12,16 @@ class MaxUsernameLengthValidator
       @max_range_violation = true
       return false
     end
-    return false if value < SiteSetting.min_username_length
+
+    if value < SiteSetting.min_username_length
+      @min_value_violation = true
+      return false
+    end
+
     @username = User.where("length(username) > ?", value).pick(:username)
-    @username.blank?
+    @group_name = Group.where(automatic: false).where("length(name) > ?", value).pick(:name)
+
+    @username.blank? && @group_name.blank?
   end
 
   def error_message
@@ -24,10 +31,12 @@ class MaxUsernameLengthValidator
         min: MAX_USERNAME_LENGTH_RANGE.begin,
         max: MAX_USERNAME_LENGTH_RANGE.end,
       )
-    elsif @username.blank?
+    elsif @min_value_violation
       I18n.t("site_settings.errors.max_username_length_range")
-    else
+    elsif @username.present?
       I18n.t("site_settings.errors.max_username_length_exists", username: @username)
+    elsif @group_name.present?
+      I18n.t("site_settings.errors.max_group_name_length_exists", group_name: @group_name)
     end
   end
 end

lib/validators/min_username_length_validator.rb

@@ -12,9 +12,16 @@ class MinUsernameLengthValidator
       @min_range_violation = true
       return false
     end
-    return false if value > SiteSetting.max_username_length
+
+    if value > SiteSetting.max_username_length
+      @max_value_violation = true
+      return false
+    end
+
     @username = User.where("length(username) < ?", value).pick(:username)
-    @username.blank?
+    @group_name = Group.where(automatic: false).where("length(name) < ?", value).pick(:name)
+
+    @username.blank? && @group_name.blank?
   end
 
   def error_message
@@ -24,10 +31,12 @@ class MinUsernameLengthValidator
         min: MIN_USERNAME_LENGTH_RANGE.begin,
         max: MIN_USERNAME_LENGTH_RANGE.end,
       )
-    elsif @username.blank?
+    elsif @max_value_violation
       I18n.t("site_settings.errors.min_username_length_range")
-    else
+    elsif @username.present?
       I18n.t("site_settings.errors.min_username_length_exists", username: @username)
+    elsif @group_name.present?
+      I18n.t("site_settings.errors.min_group_name_length_exists", group_name: @group_name)
     end
   end
 end

spec/models/group_spec.rb

@@ -416,6 +416,19 @@ RSpec.describe Group do
       expect(group.name).to_not eq("staff")
       expect(group.name).to eq(I18n.t("groups.default_names.staff", locale: "de"))
     end
+
+    it "can save groups" do
+      # Update all short usernames to ensure that the future minimum username
+      # length is met for all existing usernames
+      User.find_each { |u| u.update!(username: u.username * 2) }
+
+      # This a corner case when a group has a short name that is technically no
+      # longer allowed by `min_username_length`
+      Group.find(Group::AUTO_GROUPS[:everyone]).update!(name: "all")
+      SiteSetting.min_username_length = 10
+
+      expect { Group.refresh_automatic_groups! }.not_to raise_error
+    end
   end
 
   it "Correctly handles removal of primary group" do