From b6854c2f88c655afe95c4b79c5d14b9bca5d5cc4 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 11 Oct 2022 11:14:13 +1000 Subject: [PATCH] FIX: Deprecated settings should not override from UI (#18536) Unless we have specified `override = true` in the DeprecatedSettings class for an old -> new settings map, we should not allow people to change the old setting in the UI and have it affect the new setting. --- app/controllers/admin/site_settings_controller.rb | 9 +++++++-- spec/requests/admin/site_settings_controller_spec.rb | 10 ++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/site_settings_controller.rb b/app/controllers/admin/site_settings_controller.rb index 524b47827a1..846f4953a53 100644 --- a/app/controllers/admin/site_settings_controller.rb +++ b/app/controllers/admin/site_settings_controller.rb @@ -18,8 +18,13 @@ class Admin::SiteSettingsController < Admin::AdminController value = params[id] value.strip! if value.is_a?(String) - new_setting_name = SiteSettings::DeprecatedSettings::SETTINGS.find do |old_name, new_name, _, _| - break new_name if old_name == id + new_setting_name = SiteSettings::DeprecatedSettings::SETTINGS.find do |old_name, new_name, override, _| + if old_name == id + if !override + raise Discourse::InvalidParameters, "You cannot change this site setting because it is deprecated, use #{new_name} instead." + end + break new_name + end end id = new_setting_name if new_setting_name diff --git a/spec/requests/admin/site_settings_controller_spec.rb b/spec/requests/admin/site_settings_controller_spec.rb index 9759ac3045e..c573a89701c 100644 --- a/spec/requests/admin/site_settings_controller_spec.rb +++ b/spec/requests/admin/site_settings_controller_spec.rb @@ -52,6 +52,16 @@ RSpec.describe Admin::SiteSettingsController do expect(SiteSetting.search_tokenize_chinese).to eq(true) end + it 'throws an error when trying to change a deprecated setting with override = false' do + SiteSetting.personal_message_enabled_groups = Group::AUTO_GROUPS[:trust_level_4] + put "/admin/site_settings/enable_personal_messages.json", params: { + enable_personal_messages: false + } + + expect(response.status).to eq(422) + expect(SiteSetting.personal_message_enabled_groups).to eq(Group::AUTO_GROUPS[:trust_level_4]) + end + it 'allows value to be a blank string' do put "/admin/site_settings/test_setting.json", params: { test_setting: ''