FEATURE: Introduce site settings which require confirmation (#27315)

Many site settings can be distructive or have huge side-effects
for a site that the admin may not be aware of when changing it.

This commit introduces a `requires_confirmation` attribute that
can be added to any site setting. When it is true, a confirmation
dialog will open if that setting is changed in the admin UI,
optionally with a custom message that is defined in client.en.yml.

If the admin does not confirm, we reset the setting to its previous
clean value and do not save the new value.

app/assets/javascripts/admin/addon/mixins/setting-component.js

@@ -5,9 +5,11 @@ import Mixin from "@ember/object/mixin";
 import { service } from "@ember/service";
 import { htmlSafe } from "@ember/template";
 import { isNone } from "@ember/utils";
+import { Promise } from "rsvp";
 import JsonSchemaEditorModal from "discourse/components/modal/json-schema-editor";
 import { ajax } from "discourse/lib/ajax";
 import { fmt, propertyNotEqual } from "discourse/lib/computed";
+import { SITE_SETTING_REQUIRES_CONFIRMATION_TYPES } from "discourse/lib/constants";
 import { splitString } from "discourse/lib/utilities";
 import { deepEqual } from "discourse-common/lib/object";
 import discourseComputed, { bind } from "discourse-common/utils/decorators";
@@ -82,6 +84,7 @@ export default Mixin.create({
   modal: service(),
   router: service(),
   site: service(),
+  dialog: service(),
   attributeBindings: ["setting.setting:data-setting"],
   classNameBindings: [":row", ":setting", "overridden", "typeClass"],
   validationMessage: null,
@@ -204,10 +207,59 @@ export default Mixin.create({
     }
   },
 
+  confirmChanges(settingKey) {
+    return new Promise((resolve) => {
+      // Fallback is needed in case the setting does not have a custom confirmation
+      // prompt/confirm defined.
+      this.dialog.alert({
+        message: I18n.t(
+          `admin.site_settings.requires_confirmation_messages.${settingKey}.prompt`,
+          {
+            translatedFallback: I18n.t(
+              "admin.site_settings.requires_confirmation_messages.default.prompt"
+            ),
+          }
+        ),
+        buttons: [
+          {
+            label: I18n.t(
+              `admin.site_settings.requires_confirmation_messages.${settingKey}.confirm`,
+              {
+                translatedFallback: I18n.t(
+                  "admin.site_settings.requires_confirmation_messages.default.confirm"
+                ),
+              }
+            ),
+            class: "btn-primary",
+            action: () => resolve(true),
+          },
+          {
+            label: I18n.t("no_value"),
+            class: "btn-default",
+            action: () => resolve(false),
+          },
+        ],
+      });
+    });
+  },
+
   @action
   async update() {
     const key = this.buffered.get("setting");
 
+    let confirm = true;
+    if (
+      this.buffered.get("requires_confirmation") ===
+      SITE_SETTING_REQUIRES_CONFIRMATION_TYPES.simple
+    ) {
+      confirm = await this.confirmChanges(key);
+    }
+
+    if (!confirm) {
+      this.cancel();
+      return;
+    }
+
     if (!DEFAULT_USER_PREFERENCES.includes(key)) {
       await this.save();
       return;

app/assets/javascripts/discourse/app/lib/constants.js

@@ -84,9 +84,14 @@ export const TOPIC_VISIBILITY_REASONS = {
 export const SYSTEM_FLAG_IDS = {
   like: 2,
   notify_user: 6,
-  notify_moderators: 7,
   off_topic: 3,
   inappropriate: 4,
   spam: 8,
   illegal: 10,
+  notify_moderators: 7,
+};
+
+export const SITE_SETTING_REQUIRES_CONFIRMATION_TYPES = {
+  simple: "simple",
+  user_option: "user_option",
 };

config/locales/client.en.yml

@@ -6795,6 +6795,22 @@ en:
           add_types_title: "Allow extensions %{types}"
           add_types_toast: "%{types} file types added"
         mandatory_group: "Group is mandatory"
+        requires_confirmation_messages:
+          default:
+            prompt: "Changing this setting may have far-reaching or unintended consequences for your site. Are you sure you want to proceed?"
+            confirm: "Yes, I'm sure"
+          min_password_length:
+            prompt: "You’re about to change your password policy. This will affect all users changing their passwords from now on. Are you sure you want to proceed?"
+            confirm: "Yes, update password policy"
+          min_admin_password_length:
+            prompt: "You’re about to change your password policy. This will affect all admins changing their passwords from now on. Are you sure you want to proceed?"
+            confirm: "Yes, update password policy"
+          password_unique_charactes:
+            prompt: "You’re about to change your password policy. This will affect all users changing their passwords from now on. Are you sure you want to proceed?"
+            confirm: "Yes, update password policy"
+          block_common_passwords:
+            prompt: "You’re about to change your password policy. This will affect all users changing their passwords from now on. Are you sure you want to proceed?"
+            confirm: "Yes, update password policy"
 
       badges:
         title: Badges

config/site_settings.yml

@@ -630,16 +630,21 @@ users:
     default: 10
     min: 8
     max: 500
+    requires_confirmation: "simple"
   min_admin_password_length:
     client: true
     default: 15
     min: 8
     max: 500
+    requires_confirmation: "simple"
   password_unique_characters:
     default: 6
     min: 1
     max: 10
-  block_common_passwords: true
+    requires_confirmation: "simple"
+  block_common_passwords:
+    default: true
+    requires_confirmation: "simple"
   username_change_period: 3
   email_editable:
     client: true

lib/site_setting_extension.rb

@@ -95,6 +95,10 @@ module SiteSettingExtension
     @shadowed_settings ||= []
   end
 
+  def requires_confirmation_settings
+    @requires_confirmation_settings ||= {}
+  end
+
   def hidden_settings_provider
     @hidden_settings_provider ||= SiteSettings::HiddenProvider.new
   end
@@ -244,6 +248,7 @@ module SiteSettingExtension
           secret: secret_settings.include?(s),
           placeholder: placeholder(s),
           mandatory_values: mandatory_values[s],
+          requires_confirmation: requires_confirmation_settings[s],
         }.merge!(type_hash)
 
         opts[:plugin] = plugins[s] if plugins[s]
@@ -654,6 +659,14 @@ module SiteSettingExtension
 
       mandatory_values[name] = opts[:mandatory_values] if opts[:mandatory_values]
 
+      requires_confirmation_settings[name] = (
+        if SiteSettings::TypeSupervisor::REQUIRES_CONFIRMATION_TYPES.values.include?(
+             opts[:requires_confirmation],
+           )
+          opts[:requires_confirmation]
+        end
+      )
+
       categories[name] = opts[:category] || :uncategorized
 
       hidden_settings_provider.add_hidden(name) if opts[:hidden]

lib/site_settings/type_supervisor.rb

@@ -20,12 +20,15 @@ class SiteSettings::TypeSupervisor
     list_type
     textarea
     json_schema
+    requires_confirmation
   ].freeze
   VALIDATOR_OPTS = %i[min max regex hidden regex_error json_schema].freeze
 
   # For plugins, so they can tell if a feature is supported
   SUPPORTED_TYPES = %i[email username list enum].freeze
 
+  REQUIRES_CONFIRMATION_TYPES = { simple: "simple", user_option: "user_option" }.freeze
+
   def self.types
     @types ||=
       Enum.new(

lib/tasks/javascript.rake

@@ -166,6 +166,8 @@ task "javascript:update_constants" => :environment do
     export const TOPIC_VISIBILITY_REASONS = #{Topic.visibility_reasons.to_json};
 
     export const SYSTEM_FLAG_IDS = #{PostActionType.types.to_json}
+
+    export const SITE_SETTING_REQUIRES_CONFIRMATION_TYPES = #{SiteSettings::TypeSupervisor::REQUIRES_CONFIRMATION_TYPES.to_json}
   JS
 
   pretty_notifications = Notification.types.map { |n| "  #{n[0]}: #{n[1]}," }.join("\n")

spec/lib/site_setting_extension_spec.rb

@@ -892,6 +892,24 @@ RSpec.describe SiteSettingExtension do
     end
   end
 
+  describe "requires_confirmation settings" do
+    it "returns 'simple' for settings that require confirmation with 'simple' type" do
+      expect(
+        SiteSetting.all_settings.find { |s| s[:setting] == :min_password_length }[
+          :requires_confirmation
+        ],
+      ).to eq("simple")
+    end
+
+    it "returns nil for settings that do not require confirmation" do
+      expect(
+        SiteSetting.all_settings.find { |s| s[:setting] == :display_local_time_in_user_card }[
+          :requires_confirmation
+        ],
+      ).to eq(nil)
+    end
+  end
+
   describe "_map extension for list settings" do
     it "handles splitting group_list settings" do
       SiteSetting.personal_message_enabled_groups = "1|2"

spec/system/admin_site_setting_requires_confirmation_spec.rb

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+describe "Admin Site Setting Requires Confirmation", type: :system do
+  let(:settings_page) { PageObjects::Pages::AdminSiteSettings.new }
+  let(:dialog) { PageObjects::Components::Dialog.new }
+  fab!(:admin)
+
+  before do
+    SiteSetting.min_password_length = 10
+    sign_in(admin)
+  end
+
+  it "requires confirmation and shows the correct message" do
+    settings_page.visit("min_password_length")
+    settings_page.change_number_setting("min_password_length", 12)
+    expect(dialog).to be_open
+    expect(dialog).to have_content(
+      I18n.t(
+        "admin_js.admin.site_settings.requires_confirmation_messages.min_password_length.prompt",
+      ),
+    )
+    expect(dialog).to have_content(
+      I18n.t(
+        "admin_js.admin.site_settings.requires_confirmation_messages.min_password_length.confirm",
+      ),
+    )
+    dialog.click_yes
+    expect(dialog).to be_closed
+    expect(SiteSetting.min_password_length).to eq(12)
+  end
+
+  it "does not save the new setting value if the admin cancels confirmation" do
+    settings_page.visit("min_password_length")
+    settings_page.change_number_setting("min_password_length", 12)
+    expect(dialog).to be_open
+    dialog.click_no
+    expect(dialog).to be_closed
+    expect(SiteSetting.min_password_length).to eq(10)
+  end
+end

spec/system/admin_site_setting_search_spec.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 describe "Admin Site Setting Search", type: :system do
-  let(:settings_page) { PageObjects::Pages::AdminSettings.new }
+  let(:settings_page) { PageObjects::Pages::AdminSiteSettings.new }
   fab!(:admin)
 
   before do

spec/system/emojis/emoji_deny_list_spec.rb

@@ -10,7 +10,7 @@ describe "Emoji deny list", type: :system do
 
   describe "when editing admin settings" do
     before { SiteSetting.emoji_deny_list = "" }
-    let(:site_settings_page) { PageObjects::Pages::AdminSettings.new }
+    let(:site_settings_page) { PageObjects::Pages::AdminSiteSettings.new }
 
     skip "should allow admin to update emoji deny list" do
       site_settings_page.visit_category("posting")

spec/system/page_objects/pages/admin_site_settings.rb

@@ -2,14 +2,18 @@
 
 module PageObjects
   module Pages
-    class AdminSettings < PageObjects::Pages::Base
+    class AdminSiteSettings < PageObjects::Pages::Base
       def visit_filtered_plugin_setting(filter)
         page.visit("/admin/site_settings/category/plugins?filter=#{filter}")
         self
       end
 
-      def visit
+      def visit(filter = nil)
-        page.visit("/admin/site_settings")
+        if filter.present?
+          page.visit("/admin/site_settings?filter=#{filter}")
+        else
+          page.visit("/admin/site_settings")
+        end
         self
       end
 
@@ -18,17 +22,31 @@ module PageObjects
         self
       end
 
+      def find_setting(setting_name)
+        find(".admin-detail .row.setting[data-setting='#{setting_name}']")
+      end
+
       def toggle_setting(setting_name, text = "")
-        setting = find(".admin-detail .row.setting[data-setting='#{setting_name}']")
+        setting = find_setting(setting_name)
         setting.find(".setting-value span", text: text).click
-        setting.find(".setting-controls button.ok").click
+        save_setting(setting)
+      end
+
+      def change_number_setting(setting_name, value, save_changes = true)
+        setting = find_setting(setting_name)
+        setting.fill_in(with: value)
+        save_setting(setting) if save_changes
       end
 
       def select_from_emoji_list(setting_name, text = "", save_changes = true)
         setting = find(".admin-detail .row.setting[data-setting='#{setting_name}']")
         setting.find(".setting-value .value-list > .value button").click
         setting.find(".setting-value .emoji-picker .emoji[title='#{text}']").click
-        setting.find(".setting-controls button.ok").click if save_changes
+        save_setting(setting) if save_changes
+      end
+
+      def save_setting(setting_element)
+        setting_element.find(".setting-controls button.ok").click
       end
 
       def values_in_list(setting_name)
@@ -67,5 +85,10 @@ module PageObjects
         assert_selector(".admin-detail .row.setting", minimum: count)
       end
     end
+
+    # TODO (martin) Remove this after discourse-topic-voting no longer
+    # relies on this, it was renamed to AdminSiteSettings.
+    class AdminSettings < PageObjects::Pages::AdminSiteSettings
+    end
   end
 end