From 8be39c5bf0ba6836fdc59ef70e657d457098f019 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Wed, 19 Feb 2025 19:33:46 +0200 Subject: [PATCH] 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 | 4 ++-- app/models/username_validator.rb | 12 +++++++----- config/locales/server.en.yml | 2 ++ lib/validators/max_username_length_validator.rb | 17 +++++++++++++---- lib/validators/min_username_length_validator.rb | 17 +++++++++++++---- spec/models/group_spec.rb | 13 +++++++++++++ 6 files changed, 50 insertions(+), 15 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 0106b02cc72..009f9bf394c 100644 --- a/app/models/group.rb +++ b/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) diff --git a/app/models/username_validator.rb b/app/models/username_validator.rb index c11c770711e..8ba990750a6 100644 --- a/app/models/username_validator.rb +++ b/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) - validator = UsernameValidator.new(object.public_send(field_name)) + def self.perform_validation(object, field_name, opts = {}) + 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_max? + username_length_min? if !skip_length_validation + username_length_max? if !skip_length_validation username_char_valid? username_char_allowed? username_first_char_valid? diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 39e36682d25..a82a51d7af7 100644 --- a/config/locales/server.en.yml +++ b/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." diff --git a/lib/validators/max_username_length_validator.rb b/lib/validators/max_username_length_validator.rb index 49bcc1b400c..c57e15b808f 100644 --- a/lib/validators/max_username_length_validator.rb +++ b/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 diff --git a/lib/validators/min_username_length_validator.rb b/lib/validators/min_username_length_validator.rb index 6faad1ad49a..57e7de93404 100644 --- a/lib/validators/min_username_length_validator.rb +++ b/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 diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 4e8cef69b5f..486cc0d1784 100644 --- a/spec/models/group_spec.rb +++ b/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