From d3a5a493fa89077335968ed04a3dab5fa8885926 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Wed, 10 May 2023 15:21:48 +0200 Subject: [PATCH] DEV: Add configurable? helper to Plugin::Instance (#21472) This reapplies commit 3073e5cfb0721b3b8009963c5fc7f61d414db317, with a fix that makes sure that plugins can be looked up both by the name present in metadata and directory name. --- .../admin/site_settings_controller.rb | 9 +++- app/controllers/application_controller.rb | 6 ++- lib/discourse.rb | 23 ++++++--- lib/plugin/instance.rb | 15 ++++-- lib/site_setting_extension.rb | 22 +++++++++ .../admin/site_settings_controller_spec.rb | 47 +++++++++++++++++++ 6 files changed, 107 insertions(+), 15 deletions(-) diff --git a/app/controllers/admin/site_settings_controller.rb b/app/controllers/admin/site_settings_controller.rb index 3b2ab4e9db0..3c77daab96c 100644 --- a/app/controllers/admin/site_settings_controller.rb +++ b/app/controllers/admin/site_settings_controller.rb @@ -259,10 +259,15 @@ class Admin::SiteSettingsController < Admin::AdminController end def raise_access_hidden_setting(id) - # note, as of Ruby 2.3 symbols are GC'd so this is considered safe - if SiteSetting.hidden_settings.include?(id.to_sym) + id = id.to_sym + + if SiteSetting.hidden_settings.include?(id) raise Discourse::InvalidParameters, "You are not allowed to change hidden settings" end + + if SiteSetting.plugins[id] && !Discourse.plugins_by_name[SiteSetting.plugins[id]].configurable? + raise Discourse::InvalidParameters, "You are not allowed to change unconfigurable settings" + end end def tag_notification_level(id) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index cbed26f8560..2eb57914f42 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -346,7 +346,11 @@ class ApplicationController < ActionController::Base # disabled. This allows plugins to be disabled programmatically. def self.requires_plugin(plugin_name) before_action do - raise PluginDisabled.new if Discourse.disabled_plugin_names.include?(plugin_name) + if plugin = Discourse.plugins_by_name[plugin_name] + raise PluginDisabled.new if !plugin.enabled? + else + Rails.logger.warn("Required plugin '#{plugin_name}' not found") + end end end diff --git a/lib/discourse.rb b/lib/discourse.rb index 1a8cefae740..259da18b60d 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -344,6 +344,7 @@ module Discourse def self.activate_plugins! @plugins = [] + @plugins_by_name = {} Plugin::Instance .find_all("#{Rails.root}/plugins") .each do |p| @@ -351,6 +352,18 @@ module Discourse if Discourse.has_needed_version?(Discourse::VERSION::STRING, v) p.activate! @plugins << p + @plugins_by_name[p.name] = p + + # The plugin directory name and metadata name should match, but that + # is not always the case + dir_name = p.path.split("/")[-2] + if p.name != dir_name + STDERR.puts "Plugin name is '#{p.name}', but plugin directory is named '#{dir_name}'" + # Plugins are looked up by directory name in SiteSettingExtension + # because SiteSetting.load_settings uses directory name as plugin + # name. We alias the two names just to make sure the look up works + @plugins_by_name[dir_name] = p + end else STDERR.puts "Could not activate #{p.metadata.name}, discourse does not meet required version (#{v})" end @@ -358,20 +371,16 @@ module Discourse DiscourseEvent.trigger(:after_plugin_activation) end - def self.disabled_plugin_names - plugins.select { |p| !p.enabled? }.map(&:name) - end - def self.plugins @plugins ||= [] end - def self.hidden_plugins - @hidden_plugins ||= [] + def self.plugins_by_name + @plugins_by_name ||= {} end def self.visible_plugins - self.plugins - self.hidden_plugins + plugins.filter(&:visible?) end def self.plugin_themes diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index cad7061ad12..a49a008e1c1 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -103,7 +103,16 @@ class Plugin::Instance @admin_route = { label: label, location: location } end + def configurable? + true + end + + def visible? + configurable? && !@hidden + end + def enabled? + return false if !configurable? @enabled_site_setting ? SiteSetting.get(@enabled_site_setting) : true end @@ -825,11 +834,7 @@ class Plugin::Instance end def hide_plugin - Discourse.hidden_plugins << self - end - - def enabled_site_setting_filter(filter = nil) - STDERR.puts("`enabled_site_setting_filter` is deprecated") + @hidden = true end def enabled_site_setting(setting = nil) diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index ebbea857695..0dcabb850ae 100644 --- a/lib/site_setting_extension.rb +++ b/lib/site_setting_extension.rb @@ -166,6 +166,20 @@ module SiteSettingExtension end end + def remove_setting(name_arg) + raise if !Rails.env.test? + + name = name_arg.to_sym + + categories.delete(name) + hidden_settings.delete(name) + refresh_settings.delete(name) + client_settings.delete(name) + previews.delete(name) + secret_settings.delete(name) + plugins.delete(name) + end + def settings_hash result = {} deprecated_settings = Set.new @@ -224,6 +238,9 @@ module SiteSettingExtension defaults .all(default_locale) + .reject do |setting_name, _| + plugins[name] && !Discourse.plugins_by_name[plugins[name]].configurable? + end .reject { |setting_name, _| !include_hidden && hidden_settings.include?(setting_name) } .map do |s, v| type_hash = type_supervisor.type_hash(s) @@ -544,6 +561,11 @@ module SiteSettingExtension end else define_singleton_method clean_name do + if plugins[name] + plugin = Discourse.plugins_by_name[plugins[name]] + return false if !plugin.configurable? && plugin.enabled_site_setting == name + end + if (c = current[name]).nil? refresh! current[name] diff --git a/spec/requests/admin/site_settings_controller_spec.rb b/spec/requests/admin/site_settings_controller_spec.rb index 1aabf0deeb0..56f46d91b8c 100644 --- a/spec/requests/admin/site_settings_controller_spec.rb +++ b/spec/requests/admin/site_settings_controller_spec.rb @@ -608,6 +608,53 @@ RSpec.describe Admin::SiteSettingsController do expect(response.status).to eq(422) end + it "does not allow changing of hidden settings" do + SiteSetting.setting(:hidden_setting, "hidden", hidden: true) + SiteSetting.refresh! + + put "/admin/site_settings/hidden_setting.json", params: { hidden_setting: "not allowed" } + + expect(SiteSetting.hidden_setting).to eq("hidden") + expect(response.status).to eq(422) + end + + context "with an plugin" do + let(:plugin) do + metadata = Plugin::Metadata.new + metadata.name = "discourse-plugin" + Plugin::Instance.new(metadata) + end + + before do + Discourse.plugins_by_name[plugin.name] = plugin + SiteSetting.setting(:plugin_setting, "default value", plugin: "discourse-plugin") + SiteSetting.refresh! + end + + after do + Discourse.plugins_by_name.delete(plugin.name) + SiteSetting.remove_setting(:plugin_setting) + end + + it "allows changing settings of configurable plugins" do + plugin.stubs(:configurable?).returns(true) + + put "/admin/site_settings/plugin_setting.json", params: { plugin_setting: "new value" } + + expect(SiteSetting.plugin_setting).to eq("new value") + expect(response.status).to eq(200) + end + + it "does not allow changing of unconfigurable settings" do + plugin.stubs(:configurable?).returns(false) + + put "/admin/site_settings/plugin_setting.json", params: { plugin_setting: "not allowed" } + + expect(SiteSetting.plugin_setting).to eq("default value") + expect(response.status).to eq(422) + end + end + it "fails when a setting does not exist" do put "/admin/site_settings/provider.json", params: { provider: "gotcha" } expect(response.status).to eq(422)