From e418f7056f5057c9abebe46abf78eddae7635560 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Tue, 3 Sep 2024 12:08:14 +0200 Subject: [PATCH] FIX: prevents PM to large groups (#28681) This commit introduces a new hidden site setting: `group_pm_user_limit`, default to `1000` which will raise an error when attempting to create a PM target a large group. --- app/models/group.rb | 12 ++++++++++++ config/locales/server.en.yml | 4 ++++ config/site_settings.yml | 5 +++++ lib/has_errors.rb | 6 +++--- lib/topic_creator.rb | 13 ++++++++++++- spec/lib/topic_creator_spec.rb | 23 +++++++++++++++++++++++ spec/models/group_spec.rb | 25 +++++++++++++++++++++++++ 7 files changed, 84 insertions(+), 4 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 3c4b8ff78d0..25a31bcc181 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -464,7 +464,19 @@ class Group < ActiveRecord::Base Group.auto_groups_between(:trust_level_0, :trust_level_4).to_a end + class GroupPmUserLimitExceededError < StandardError + end + def set_message_default_notification_levels!(topic, ignore_existing: false) + if user_count > SiteSetting.group_pm_user_limit + raise GroupPmUserLimitExceededError, + I18n.t( + "groups.errors.default_notification_level_users_limit", + count: SiteSetting.group_pm_user_limit, + group_name: name, + ) + end + group_users .pluck(:user_id, :notification_level) .each do |user_id, notification_level| diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 0c349e9e8f4..192e92d0604 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -612,6 +612,9 @@ en: one: "%{count} user has been added to the group." other: "%{count} users have been added to the group." errors: + default_notification_level_users_limit: + one: "Default notification level can't be set on groups larger than %{count} user. Incorrect group: %{group_name}" + other: "Default notification level can't be set on groups larger than %{count} users. Incorrect group: %{group_name}" grant_trust_level_not_valid: "'%{trust_level}' is not a valid trust level." can_not_modify_automatic: "You cannot modify an automatic group" member_already_exist: @@ -754,6 +757,7 @@ en: reply_by_email_disabled: "Reply by email has been disabled." send_to_email_disabled: "Sorry, you cannot send personal messages to email." target_user_not_found: "One of the users you are sending this message to could not be found." + too_large_group: "The group: %{group_name}, has too many users to receive a message. Limit is %{limit}." unable_to_update: "There was an error updating that topic." unable_to_tag: "There was an error tagging the topic." unable_to_unlist: "Sorry, you cannot create an unlisted topic." diff --git a/config/site_settings.yml b/config/site_settings.yml index a76186e8273..253dd41bc5c 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1227,6 +1227,11 @@ posting: max_form_template_content_length: default: 5000 max: 150000 + group_pm_user_limit: + default: 1000 + type: integer + min: 1 + hidden: true email: email_time_window_mins: diff --git a/lib/has_errors.rb b/lib/has_errors.rb index 6b2f94d1341..70432e54d9b 100644 --- a/lib/has_errors.rb +++ b/lib/has_errors.rb @@ -15,14 +15,14 @@ module HasErrors false end - def rollback_with!(obj, error) - obj.errors.add(:base, error) + def rollback_with!(obj, error, **kwargs) + obj.errors.add(:base, error, **kwargs) rollback_from_errors!(obj) end def rollback_from_errors!(obj) add_errors_from(obj) - raise ActiveRecord::Rollback.new + raise ActiveRecord::Rollback.new, obj.errors.full_messages.join("\n") end def add_error(msg) diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index 1557dd46e3b..c3f5e3b3346 100644 --- a/lib/topic_creator.rb +++ b/lib/topic_creator.rb @@ -109,7 +109,18 @@ class TopicCreator end topic.reload.topic_allowed_groups.each do |topic_allowed_group| - topic_allowed_group.group.set_message_default_notification_levels!(topic) + group = topic_allowed_group.group + + begin + group.set_message_default_notification_levels!(topic) + rescue Group::GroupPmUserLimitExceededError => e + rollback_with!( + topic, + :too_large_group, + group_name: group.name, + limit: SiteSetting.group_pm_user_limit, + ) + end end end diff --git a/spec/lib/topic_creator_spec.rb b/spec/lib/topic_creator_spec.rb index 6eec7e9744d..2e918b81a10 100644 --- a/spec/lib/topic_creator_spec.rb +++ b/spec/lib/topic_creator_spec.rb @@ -563,6 +563,29 @@ RSpec.describe TopicCreator do end end + context "with too many users in a group" do + fab!(:group) { Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:everyone]) } + + before do + SiteSetting.group_pm_user_limit = 1 + Fabricate.times(2, :user).each { |user| group.add(user) } + pm_valid_attrs[:target_group_names] = group.name + end + + it "fails with an error" do + expect do + TopicCreator.create(user, Guardian.new(admin), pm_valid_attrs) + end.to raise_error( + ActiveRecord::Rollback, + I18n.t( + "activerecord.errors.models.topic.attributes.base.too_large_group", + limit: SiteSetting.group_pm_user_limit, + group_name: group.name, + ), + ) + end + end + context "with to emails" do it "works for staff" do SiteSetting.send_email_messages_allowed_groups = "1|3" diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index e467e7a894d..fafaf1b328c 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -94,6 +94,31 @@ RSpec.describe Group do end end + describe "#set_message_default_notification_levels!" do + context "with too many users in a group" do + fab!(:topic) + fab!(:large_group) { Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:everyone]) } + + before do + SiteSetting.group_pm_user_limit = 1 + Fabricate.times(2, :user).each { |user| large_group.add(user) } + end + + it "raises a GroupPmUserLimitExceededError error" do + expect do + large_group.reload.set_message_default_notification_levels!(topic) + end.to raise_error( + Group::GroupPmUserLimitExceededError, + I18n.t( + "groups.errors.default_notification_level_users_limit", + count: SiteSetting.group_pm_user_limit, + group_name: large_group.name, + ), + ) + end + end + end + describe "#builtin" do context "when verifying enum sequence" do before { @builtin = Group.builtin }