From 89705be72229c1b080d7f98312f2d73d2a970bed Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 26 Dec 2023 16:39:18 +1000 Subject: [PATCH] DEV: Add auto map from TL -> group site settings in DeprecatedSettings (#24959) When setting an old TL based site setting in the console e.g.: SiteSetting.min_trust_level_to_allow_ignore = TrustLevel[3] We will silently convert this to the corresponding Group::AUTO_GROUP. And vice-versa, when we read the value on the old setting, we will automatically get the lowest trust level corresponding to the lowest auto group for the new setting in the database. --- app/models/group.rb | 9 + lib/site_settings/deprecated_settings.rb | 98 +++++++- lib/site_settings/type_supervisor.rb | 14 +- .../site_settings/deprecated_test.yml | 10 + .../site_settings/deprecated_settings_spec.rb | 233 ++++++++++++++++++ spec/models/group_spec.rb | 25 ++ spec/models/site_setting_spec.rb | 37 --- 7 files changed, 377 insertions(+), 49 deletions(-) create mode 100644 spec/fixtures/site_settings/deprecated_test.yml create mode 100644 spec/lib/site_settings/deprecated_settings_spec.rb diff --git a/app/models/group.rb b/app/models/group.rb index 70a5e3e99a5..9839d4d4e7d 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -145,6 +145,15 @@ class Group < ActiveRecord::Base @visibility_levels = Enum.new(public: 0, logged_on_users: 1, members: 2, staff: 3, owners: 4) end + def self.auto_groups_between(lower, upper) + lower_group = Group::AUTO_GROUPS[lower.to_sym] + upper_group = Group::AUTO_GROUPS[upper.to_sym] + + return [] if lower_group.blank? || upper_group.blank? + + (lower_group..upper_group).to_a & AUTO_GROUPS.values + end + validates :mentionable_level, inclusion: { in: ALIAS_LEVELS.values } validates :messageable_level, inclusion: { in: ALIAS_LEVELS.values } diff --git a/lib/site_settings/deprecated_settings.rb b/lib/site_settings/deprecated_settings.rb index 5b0d2d39977..ab6ee72ad20 100644 --- a/lib/site_settings/deprecated_settings.rb +++ b/lib/site_settings/deprecated_settings.rb @@ -35,9 +35,80 @@ module SiteSettings::DeprecatedSettings ["min_trust_level_to_allow_ignore", "ignore_allowed_groups", false, "3.3"], ] + OVERRIDE_TL_GROUP_SETTINGS = %w[ + anonymous_posting_min_trust_level + shared_drafts_min_trust_level + min_trust_level_for_here_mention + approve_unless_trust_level + approve_new_topics_unless_trust_level + email_in_min_trust + min_trust_to_edit_wiki_post + allow_uploaded_avatars + min_trust_to_create_topic + min_trust_to_edit_post + min_trust_to_flag_posts + tl4_delete_posts_and_topics + min_trust_level_to_allow_user_card_background + min_trust_level_to_allow_invite + min_trust_level_to_allow_ignore + ] + + def group_to_tl(old_setting, new_setting) + tl_and_staff = is_tl_and_staff_setting?(old_setting) + + valid_auto_groups = + self.public_send("#{new_setting}_map") & + # We only want auto groups, no actual groups because they cannot be + # mapped to TLs. + Group.auto_groups_between(tl_and_staff ? :admins : :trust_level_0, :trust_level_4) + + # We don't want to return nil because this could lead to permission holes; + # so we return the max available permission in this case. + return tl_and_staff ? "admin" : TrustLevel[4] if valid_auto_groups.empty? + + if tl_and_staff + valid_auto_groups_excluding_staff_and_admins = + valid_auto_groups - [Group::AUTO_GROUPS[:staff], Group::AUTO_GROUPS[:admins]] + + if valid_auto_groups_excluding_staff_and_admins.any? + return valid_auto_groups_excluding_staff_and_admins.min - Group::AUTO_GROUPS[:trust_level_0] + end + + if valid_auto_groups.include?(Group::AUTO_GROUPS[:staff]) + "staff" + elsif valid_auto_groups.include?(Group::AUTO_GROUPS[:admins]) + "admin" + end + else + valid_auto_groups.min - Group::AUTO_GROUPS[:trust_level_0] + end + end + + def tl_to_group(old_setting, val) + tl_and_staff = is_tl_and_staff_setting?(old_setting) + + if val == "admin" + Group::AUTO_GROUPS[:admins] + elsif val == "staff" + Group::AUTO_GROUPS[:staff] + else + if tl_and_staff + "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:staff]}|#{val.to_i + Group::AUTO_GROUPS[:trust_level_0]}" + else + "#{val.to_i + Group::AUTO_GROUPS[:trust_level_0]}" + end + end + end + + def is_tl_and_staff_setting?(old_setting) + SiteSetting.type_supervisor.get_type(old_setting.to_sym) == :enum && + SiteSetting.type_supervisor.get_enum_class(old_setting.to_sym).name == + TrustLevelAndStaffSetting.name + end + def setup_deprecated_methods SETTINGS.each do |old_setting, new_setting, override, version| - unless override + if !override SiteSetting.singleton_class.public_send( :alias_method, :"_#{old_setting}", @@ -45,6 +116,12 @@ module SiteSettings::DeprecatedSettings ) end + if OVERRIDE_TL_GROUP_SETTINGS.include?(old_setting) + define_singleton_method "_group_to_tl_#{old_setting}" do |warn: true| + group_to_tl(old_setting, new_setting) + end + end + define_singleton_method old_setting do |warn: true| if warn Discourse.deprecate( @@ -53,10 +130,14 @@ module SiteSettings::DeprecatedSettings ) end - self.public_send(override ? new_setting : "_#{old_setting}") + if OVERRIDE_TL_GROUP_SETTINGS.include?(old_setting) + self.public_send("_group_to_tl_#{old_setting}") + else + self.public_send(override ? new_setting : "_#{old_setting}") + end end - unless override + if !override SiteSetting.singleton_class.public_send( :alias_method, :"_#{old_setting}?", @@ -75,7 +156,7 @@ module SiteSettings::DeprecatedSettings self.public_send("#{override ? new_setting : "_" + old_setting}?") end - unless override + if !override SiteSetting.singleton_class.public_send( :alias_method, :"_#{old_setting}=", @@ -91,7 +172,14 @@ module SiteSettings::DeprecatedSettings ) end - self.public_send("#{override ? new_setting : "_" + old_setting}=", val) + if OVERRIDE_TL_GROUP_SETTINGS.include?(old_setting) + # We want to set both the new group setting here to the equivalent of the + # TL, as well as setting the TL value in the DB so they remain in sync. + self.public_send("_#{old_setting}=", val) + self.public_send("#{new_setting}=", tl_to_group(old_setting, val).to_s) + else + self.public_send("#{override ? new_setting : "_" + old_setting}=", val) + end end end end diff --git a/lib/site_settings/type_supervisor.rb b/lib/site_settings/type_supervisor.rb index 04484b8257f..e2550df9f34 100644 --- a/lib/site_settings/type_supervisor.rb +++ b/lib/site_settings/type_supervisor.rb @@ -173,7 +173,7 @@ class SiteSettings::TypeSupervisor result = { type: type.to_s } if type == :enum - if (klass = enum_class(name)) + if (klass = get_enum_class(name)) result.merge!(valid_values: klass.values, translate_names: klass.translate_names?) else result.merge!( @@ -206,6 +206,10 @@ class SiteSettings::TypeSupervisor result end + def get_enum_class(name) + @enums[name] + end + def get_type(name) self.class.types[@types[name.to_sym]] end @@ -239,8 +243,8 @@ class SiteSettings::TypeSupervisor def validate_value(name, type, val) if type == self.class.types[:enum] - if enum_class(name) - unless enum_class(name).valid_value?(val) + if get_enum_class(name) + unless get_enum_class(name).valid_value?(val) raise Discourse::InvalidParameters.new("Invalid value `#{val}` for `#{name}`") end else @@ -289,10 +293,6 @@ class SiteSettings::TypeSupervisor self.class.parse_value_type(val) end - def enum_class(name) - @enums[name] - end - def json_schema_class(name) @json_schemas[name] end diff --git a/spec/fixtures/site_settings/deprecated_test.yml b/spec/fixtures/site_settings/deprecated_test.yml new file mode 100644 index 00000000000..5bf6c07b673 --- /dev/null +++ b/spec/fixtures/site_settings/deprecated_test.yml @@ -0,0 +1,10 @@ +category1: + use_https: true + min_trust_level_to_allow_invite: + default: 2 + enum: "TrustLevelSetting" + hidden: true + min_trust_level_to_allow_invite_tl_and_staff: + default: 2 + enum: "TrustLevelAndStaffSetting" + hidden: true diff --git a/spec/lib/site_settings/deprecated_settings_spec.rb b/spec/lib/site_settings/deprecated_settings_spec.rb new file mode 100644 index 00000000000..8d7372bd1c0 --- /dev/null +++ b/spec/lib/site_settings/deprecated_settings_spec.rb @@ -0,0 +1,233 @@ +# frozen_string_literal: true + +RSpec.describe SiteSettings::DeprecatedSettings do + def deprecate_override!(settings, tl_group_overrides = []) + @original_settings = SiteSettings::DeprecatedSettings::SETTINGS.dup + SiteSettings::DeprecatedSettings::SETTINGS.clear + SiteSettings::DeprecatedSettings::SETTINGS.push(settings) + + if tl_group_overrides.any? + @original_override_tl_group = SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.dup + SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.clear + SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.push(*tl_group_overrides) + end + + SiteSetting.setup_deprecated_methods + end + + after do + if defined?(@original_settings) + SiteSettings::DeprecatedSettings::SETTINGS.clear + SiteSettings::DeprecatedSettings::SETTINGS.concat(@original_settings) + end + + if defined?(@original_override_tl_group) + SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.clear + SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.concat( + @original_override_tl_group, + ) + end + end + + describe "when not overriding deprecated settings" do + let(:override) { false } + + # Necessary because Discourse.deprecate uses redis to see if the warning + # was already logged. + use_redis_snapshotting + + # NOTE: This fixture has some completely made up settings (e.g. min_trust_level_to_allow_invite_tl_and_staff) + let(:deprecated_test) { "#{Rails.root}/spec/fixtures/site_settings/deprecated_test.yml" } + + before do + SiteSetting.force_https = true + SiteSetting.load_settings(deprecated_test) + end + + it "should not act as a proxy to the new methods" do + deprecate_override!(["use_https", "force_https", override, "0.0.1"]) + + SiteSetting.use_https = false + + expect(SiteSetting.force_https).to eq(true) + expect(SiteSetting.force_https?).to eq(true) + end + + it "should log warnings when deprecated settings are called" do + deprecate_override!(["use_https", "force_https", override, "0.0.1"]) + + logger = + track_log_messages do + expect(SiteSetting.use_https).to eq(true) + expect(SiteSetting.use_https?).to eq(true) + end + expect(logger.warnings.count).to eq(3) + + logger = track_log_messages { SiteSetting.use_https(warn: false) } + expect(logger.warnings.count).to eq(0) + end + end + + describe "when overriding deprecated settings" do + let(:override) { true } + let(:deprecated_test) { "#{Rails.root}/spec/fixtures/site_settings/deprecated_test.yml" } + + before do + SiteSetting.force_https = true + SiteSetting.load_settings(deprecated_test) + end + + it "should act as a proxy to the new methods" do + deprecate_override!(["use_https", "force_https", override, "0.0.1"]) + + SiteSetting.use_https = false + + expect(SiteSetting.force_https).to eq(false) + expect(SiteSetting.force_https?).to eq(false) + end + + it "should log warnings when deprecated settings are called" do + deprecate_override!(["use_https", "force_https", override, "0.0.1"]) + + logger = + track_log_messages do + expect(SiteSetting.use_https).to eq(true) + expect(SiteSetting.use_https?).to eq(true) + end + expect(logger.warnings.count).to eq(2) + + logger = track_log_messages { SiteSetting.use_https(warn: false) } + expect(logger.warnings.count).to eq(0) + end + end + + describe "when overriding a trust level setting with a group setting" do + let(:override) { false } + let(:deprecated_test) { "#{Rails.root}/spec/fixtures/site_settings/deprecated_test.yml" } + + before { SiteSetting.load_settings(deprecated_test) } + + context "when getting an old TrustLevelSetting" do + before do + deprecate_override!( + ["min_trust_level_to_allow_invite", "invite_allowed_groups", override, "0.0.1"], + ) + end + + it "uses the minimum trust level from the trust level auto groups in the new group setting" do + SiteSetting.invite_allowed_groups = + "#{Group::AUTO_GROUPS[:trust_level_3]}|#{Group::AUTO_GROUPS[:trust_level_4]}" + expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[3]) + end + + it "returns TL4 if there are no trust level auto groups in the new group setting" do + SiteSetting.invite_allowed_groups = Fabricate(:group).id.to_s + expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[4]) + end + + it "returns TL4 if there are only staff and admin auto groups in the new group setting" do + SiteSetting.invite_allowed_groups = + "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:staff]}" + expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[4]) + end + + it "returns TL4 if there are no automated invite_allowed_groups" do + SiteSetting.invite_allowed_groups = Fabricate(:group).id.to_s + expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[4]) + end + end + + context "when getting an old TrustLevelAndStaffSetting" do + before do + deprecate_override!( + [ + "min_trust_level_to_allow_invite_tl_and_staff", + "invite_allowed_groups", + override, + "0.0.1", + ], + ["min_trust_level_to_allow_invite_tl_and_staff"], + ) + end + + it "returns staff if there are staff and admin auto groups in the new group setting" do + SiteSetting.invite_allowed_groups = + "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:staff]}" + expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("staff") + end + + it "returns admin if there is only the admin auto group in the new group setting" do + SiteSetting.invite_allowed_groups = "#{Group::AUTO_GROUPS[:admins]}" + expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("admin") + end + + it "returns the min trust level if the admin auto group as well as lower TL auto groups in the new group setting" do + SiteSetting.invite_allowed_groups = + "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:trust_level_3]}" + expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq(TrustLevel[3]) + end + + it "returns admin if there are no automated invite_allowed_groups" do + SiteSetting.invite_allowed_groups = Fabricate(:group).id.to_s + expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("admin") + end + end + + context "when setting an old TrustLevelSetting" do + before do + deprecate_override!( + ["min_trust_level_to_allow_invite", "invite_allowed_groups", override, "0.0.1"], + ["min_trust_level_to_allow_invite"], + ) + end + + it "converts the provided trust level to the appropriate auto group" do + SiteSetting.min_trust_level_to_allow_invite = TrustLevel[4] + expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[4]) + expect(SiteSetting.invite_allowed_groups).to eq(Group::AUTO_GROUPS[:trust_level_4].to_s) + end + + it "raises error with an invalid trust level" do + expect { SiteSetting.min_trust_level_to_allow_invite = 66 }.to raise_error( + Discourse::InvalidParameters, + ) + end + end + + context "when setting an old TrustLevelAndStaffSetting" do + before do + deprecate_override!( + [ + "min_trust_level_to_allow_invite_tl_and_staff", + "invite_allowed_groups", + override, + "0.0.1", + ], + ["min_trust_level_to_allow_invite_tl_and_staff"], + ) + end + + it "converts the provided trust level to the appropriate auto group" do + SiteSetting.min_trust_level_to_allow_invite_tl_and_staff = "admin" + expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("admin") + expect(SiteSetting.invite_allowed_groups).to eq(Group::AUTO_GROUPS[:admins].to_s) + + SiteSetting.min_trust_level_to_allow_invite_tl_and_staff = "staff" + expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("staff") + expect(SiteSetting.invite_allowed_groups).to eq(Group::AUTO_GROUPS[:staff].to_s) + + SiteSetting.min_trust_level_to_allow_invite_tl_and_staff = TrustLevel[3] + expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq(TrustLevel[3]) + expect(SiteSetting.invite_allowed_groups).to eq( + "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:staff]}|#{Group::AUTO_GROUPS[:trust_level_3]}", + ) + end + + it "raises error with an invalid trust level" do + expect { SiteSetting.min_trust_level_to_allow_invite_tl_and_staff = 66 }.to raise_error( + Discourse::InvalidParameters, + ) + end + end + end +end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 91addb84de2..a193ff3f64b 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -242,6 +242,31 @@ RSpec.describe Group do end end + describe ".auto_groups_between" do + it "returns the auto groups between lower and upper bounds" do + expect( + described_class.auto_groups_between(:trust_level_0, :trust_level_3), + ).to contain_exactly(10, 11, 12, 13) + end + + it "excludes the undefined groups between staff and TL0" do + expect(described_class.auto_groups_between(:admins, :trust_level_0)).to contain_exactly( + 1, + 2, + 3, + 10, + ) + end + + it "returns an empty array when lower group is higher than upper group" do + expect(described_class.auto_groups_between(:trust_level_1, :trust_level_0)).to be_empty + end + + it "returns an empty array when passing an unknown group" do + expect(described_class.auto_groups_between(:trust_level_0, :trust_level_1337)).to be_empty + end + end + describe ".refresh_automatic_group!" do it "does not include staged users in any automatic groups" do staged = Fabricate(:staged, trust_level: 1) diff --git a/spec/models/site_setting_spec.rb b/spec/models/site_setting_spec.rb index b605ebb84db..a4bcdf7acfa 100644 --- a/spec/models/site_setting_spec.rb +++ b/spec/models/site_setting_spec.rb @@ -144,43 +144,6 @@ RSpec.describe SiteSetting do end end - describe "deprecated site settings" do - before do - SiteSetting.force_https = true - @orig_logger = Rails.logger - Rails.logger = @fake_logger = FakeLogger.new - end - - after { Rails.logger = @orig_logger } - - it "should act as a proxy to the new methods" do - begin - original_settings = SiteSettings::DeprecatedSettings::SETTINGS.dup - SiteSettings::DeprecatedSettings::SETTINGS.clear - - SiteSettings::DeprecatedSettings::SETTINGS.push(["use_https", "force_https", true, "0.0.1"]) - - SiteSetting.setup_deprecated_methods - - expect do - expect(SiteSetting.use_https).to eq(true) - expect(SiteSetting.use_https?).to eq(true) - end.to change { @fake_logger.warnings.count }.by(2) - - expect do SiteSetting.use_https(warn: false) end.to_not change { @fake_logger.warnings } - - SiteSetting.use_https = false - - expect(SiteSetting.force_https).to eq(false) - expect(SiteSetting.force_https?).to eq(false) - ensure - SiteSettings::DeprecatedSettings::SETTINGS.clear - - SiteSettings::DeprecatedSettings::SETTINGS.concat(original_settings) - end - end - end - describe "cached settings" do it "should recalculate cached setting when dependent settings are changed" do SiteSetting.blocked_attachment_filenames = "foo"