DEV: Add configurable? helper to Plugin::Instance (#21472)

This reapplies commit 3073e5cfb0, with
a fix that makes sure that plugins can be looked up both by the name
present in metadata and directory name.

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)

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
 

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

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)

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]

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)