diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index 46c94444eb6..10bd31603b6 100644 --- a/lib/site_setting_extension.rb +++ b/lib/site_setting_extension.rb @@ -4,6 +4,8 @@ module SiteSettingExtension include SiteSettings::DeprecatedSettings include HasSanitizableFields + SiteSettingChangeResult = Struct.new(:previous_value, :new_value) + # support default_locale being set via global settings # this also adds support for testing the extension and global settings # for site locale @@ -457,9 +459,13 @@ module SiteSettingExtension clear_uploads_cache(name) notify_clients!(name) if client_settings.include? name clear_cache! - if old_val != current[name] - DiscourseEvent.trigger(:site_setting_changed, name, old_val, current[name]) + + if defined?(Rails::Console) + details = "Updated via Rails console" + log(name, val, old_val, Discourse.system_user, details) end + + DiscourseEvent.trigger(:site_setting_changed, name, old_val, current[name]) end def notify_changed! @@ -519,13 +525,9 @@ module SiteSettingExtension if has_setting?(name) prev_value = public_send(name) set(name, value) - value = prev_value = "[FILTERED]" if secret_settings.include?(name.to_sym) - StaffActionLogger.new(user).log_site_setting_change( - name, - prev_value, - value, - { details: detailed_message }.compact_blank, - ) + # Logging via the rails console is already handled in add_override! + log(name, value, prev_value, user, detailed_message) unless defined?(Rails::Console) + SiteSettingChangeResult.new(prev_value, public_send(name)) else raise Discourse::InvalidParameters.new( I18n.t("errors.site_settings.invalid_site_setting", name: name), @@ -777,6 +779,17 @@ module SiteSettingExtension end end + def log(name, value, prev_value, user = Discourse.system_user, detailed_message = nil) + value = prev_value = "[FILTERED]" if secret_settings.include?(name.to_sym) + return if hidden_settings.include?(name.to_sym) + StaffActionLogger.new(user).log_site_setting_change( + name, + prev_value, + value, + { details: detailed_message }.compact_blank, + ) + end + def default_uploads @default_uploads ||= {} diff --git a/spec/jobs/disable_bootstrap_mode_spec.rb b/spec/jobs/disable_bootstrap_mode_spec.rb index 601bc9c1ef0..2f2a0c0d19c 100644 --- a/spec/jobs/disable_bootstrap_mode_spec.rb +++ b/spec/jobs/disable_bootstrap_mode_spec.rb @@ -19,22 +19,25 @@ RSpec.describe Jobs::DisableBootstrapMode do it "turns off bootstrap mode if bootstrap_mode_min_users is set to 0" do SiteSetting.bootstrap_mode_min_users = 0 - StaffActionLogger.any_instance.expects(:log_site_setting_change).times(4) + StaffActionLogger.any_instance.expects(:log_site_setting_change).times(3) Jobs::DisableBootstrapMode.new.execute(user_id: admin.id) + expect(SiteSetting.bootstrap_mode_enabled).to eq(false) end it "does not amend setting that is not in bootstrap state" do SiteSetting.bootstrap_mode_min_users = 0 SiteSetting.default_trust_level = TrustLevel[3] - StaffActionLogger.any_instance.expects(:log_site_setting_change).times(3) + StaffActionLogger.any_instance.expects(:log_site_setting_change).times(2) Jobs::DisableBootstrapMode.new.execute(user_id: admin.id) + expect(SiteSetting.bootstrap_mode_enabled).to eq(false) end it "successfully turns off bootstrap mode" do SiteSetting.bootstrap_mode_min_users = 5 6.times { Fabricate(:user) } - StaffActionLogger.any_instance.expects(:log_site_setting_change).times(4) + StaffActionLogger.any_instance.expects(:log_site_setting_change).times(3) Jobs::DisableBootstrapMode.new.execute(user_id: admin.id) + expect(SiteSetting.bootstrap_mode_enabled).to eq(false) end end end diff --git a/spec/jobs/enable_bootstrap_mode_spec.rb b/spec/jobs/enable_bootstrap_mode_spec.rb index 32c9ca25281..a2f1400eee2 100644 --- a/spec/jobs/enable_bootstrap_mode_spec.rb +++ b/spec/jobs/enable_bootstrap_mode_spec.rb @@ -26,15 +26,16 @@ RSpec.describe Jobs::EnableBootstrapMode do it "does not amend setting that is not in default state" do SiteSetting.default_trust_level = TrustLevel[3] - StaffActionLogger.any_instance.expects(:log_site_setting_change).times(3) + StaffActionLogger.any_instance.expects(:log_site_setting_change).times(2) Jobs::EnableBootstrapMode.new.execute(user_id: admin.id) + expect(SiteSetting.bootstrap_mode_enabled).to eq(true) end it "successfully turns on bootstrap mode" do - StaffActionLogger.any_instance.expects(:log_site_setting_change).times(4) Jobs::EnableBootstrapMode.new.execute(user_id: admin.id) expect(admin.reload.moderator).to be_truthy expect(Jobs::SendSystemMessage.jobs.size).to eq(0) + expect(SiteSetting.bootstrap_mode_enabled).to eq(true) end end end diff --git a/spec/lib/site_setting_extension_spec.rb b/spec/lib/site_setting_extension_spec.rb index 15648f95f26..e039159612d 100644 --- a/spec/lib/site_setting_extension_spec.rb +++ b/spec/lib/site_setting_extension_spec.rb @@ -1056,4 +1056,91 @@ RSpec.describe SiteSettingExtension do end end end + + describe "logging Site Settings via the Rails Console" do + around do |example| + # Ensure Rails::Console is defined for the duration of each example. + if !Rails.const_defined?(:Console) + Rails.const_set("Console", Module.new) + example.run + Rails.send(:remove_const, "Console") + else + example.run + end + end + + before do + settings.setting(:log_test, "initial") + settings.refresh! + end + + context "when using the direct setter" do + it "logs the change exactly once" do + logger_spy = instance_spy(StaffActionLogger) + allow(StaffActionLogger).to receive(:new).with(Discourse.system_user).and_return(logger_spy) + + settings.log_test = "changed" + expect(settings.log_test).to eq("changed") + expect(logger_spy).to have_received(:log_site_setting_change).with( + :log_test, + "initial", + "changed", + { details: "Updated via Rails console" }, + ).once + end + end + + context "when using set_and_log" do + it "logs the change exactly once without double logging" do + logger_spy = instance_spy(StaffActionLogger) + allow(StaffActionLogger).to receive(:new).with(Discourse.system_user).and_return(logger_spy) + + settings.set_and_log("log_test", "changed", Discourse.system_user) + expect(settings.log_test).to eq("changed") + expect(logger_spy).to have_received(:log_site_setting_change).with( + :log_test, + "initial", + "changed", + { details: "Updated via Rails console" }, + ).once + end + end + + context "for secret settings" do + before do + settings.setting(:secret_test, "old_secret", secret: true) + settings.refresh! + end + + it "logs filtered values" do + logger_spy = instance_spy(StaffActionLogger) + allow(StaffActionLogger).to receive(:new).with(Discourse.system_user).and_return(logger_spy) + + settings.secret_test = "new_secret" + expect(settings.secret_test).to eq("new_secret") + expect(logger_spy).to have_received(:log_site_setting_change).with( + :secret_test, + "[FILTERED]", + "[FILTERED]", + { details: "Updated via Rails console" }, + ).once + end + end + + context "for hidden settings" do + before do + settings.setting(:hidden_test, "old_hidden", hidden: true) + settings.refresh! + end + + it "does not log the change" do + logger_spy = instance_spy(StaffActionLogger) + allow(StaffActionLogger).to receive(:new).with(Discourse.system_user).and_return(logger_spy) + + settings.hidden_test = "changed" + expect(settings.hidden_test).to eq("changed") + expect(logger_spy).not_to have_received(:log_site_setting_change) + end + end + end end diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index 6413f1d0cdf..370c0ed9609 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -555,7 +555,7 @@ RSpec.describe CategoriesController do expect(category.category_groups.map { |g| [g.group_id, g.permission_type] }.sort).to eq( [[Group[:everyone].id, readonly], [Group[:staff].id, create_post]], ) - expect(UserHistory.count).to eq(6) # 1 + 5 (bootstrap mode) + expect(UserHistory.count).to eq(5) end end end @@ -785,7 +785,7 @@ RSpec.describe CategoriesController do }, } expect(response.status).to eq(200) - expect(UserHistory.count).to eq(7) # 2 + 5 (bootstrap mode) + expect(UserHistory.count).to eq(6) end it "updates per-category settings correctly" do