From 7926a1f7bb64ea764841d594e9d8f73fa1b69298 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 25 Jul 2018 16:44:09 +0100 Subject: [PATCH] FIX: Remove `plugin.enabled?` checks at initialization time (#6166) Checking `plugin.enabled?` while initializing plugins causes issues in two ways: - An application restart is required for changes to take effect. A load-balanced multi-server environment could behave very weirdly if containers restart at different times. - In a multisite environment, it takes the `enabled?` setting from the default site. Changes on that site affect all other sites in the cluster. Instead, `plugin.enabled?` should be checked at runtime, in the context of a request. This commit removes `plugin.enabled?` from many `instance.rb` methods. I have added a working `plugin.enabled?` implementation for methods that actually affect security/functionality: - `post_custom_fields_whitelist` - `whitelist_staff_user_custom_field` - `add_permitted_post_create_param` --- app/controllers/posts_controller.rb | 4 +-- app/models/post.rb | 4 +-- lib/plugin/instance.rb | 40 +++++++++++++++++++---------- 3 files changed, 30 insertions(+), 18 deletions(-) diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 410a00b80f..7052b98f53 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -608,8 +608,8 @@ class PostsController < ApplicationController :visible ] - if Post.permitted_create_params.present? - permitted.concat(Post.permitted_create_params.to_a) + Post.plugin_permitted_create_params.each do |key, plugin| + permitted << key if plugin.enabled? end # param munging for WordPress diff --git a/app/models/post.rb b/app/models/post.rb index 14d57b824e..86561ad74e 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -16,8 +16,8 @@ class Post < ActiveRecord::Base include HasCustomFields include LimitedEdit - cattr_accessor :permitted_create_params - self.permitted_create_params = Set.new + cattr_accessor :plugin_permitted_create_params + self.plugin_permitted_create_params = {} # increase this number to force a system wide post rebake # Version 1, was the initial version diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index 7c0738c04d..b2c29ae4fd 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -92,26 +92,26 @@ class Plugin::Instance end end + # Applies to all sites in a multisite environment. Ignores plugin.enabled? def add_report(name, &block) reloadable_patch do |plugin| - if plugin.enabled? - Report.add_report(name, &block) - end + Report.add_report(name, &block) end end + # Applies to all sites in a multisite environment. Ignores plugin.enabled? def replace_flags settings = ::FlagSettings.new yield settings reloadable_patch do |plugin| - ::PostActionType.replace_flag_settings(settings) if plugin.enabled? + ::PostActionType.replace_flag_settings(settings) end end def whitelist_staff_user_custom_field(field) reloadable_patch do |plugin| - ::User.register_plugin_staff_custom_field(field, plugin) if plugin.enabled? + ::User.register_plugin_staff_custom_field(field, plugin) # plugin.enabled? is checked at runtime end end @@ -122,9 +122,10 @@ class Plugin::Instance end end + # Applies to all sites in a multisite environment. Ignores plugin.enabled? def add_body_class(class_name) reloadable_patch do |plugin| - ::ApplicationHelper.extra_body_classes << class_name if plugin.enabled? + ::ApplicationHelper.extra_body_classes << class_name end end @@ -180,27 +181,34 @@ class Plugin::Instance end end + # Add a post_custom_fields_whitelister block to the TopicView, respecting if the plugin is enabled def topic_view_post_custom_fields_whitelister(&block) reloadable_patch do |plugin| - ::TopicView.add_post_custom_fields_whitelister(&block) if plugin.enabled? + ::TopicView.add_post_custom_fields_whitelister do |user| + return [] unless plugin.enabled? + block.call(user) + end end end + # Applies to all sites in a multisite environment. Ignores plugin.enabled? def add_preloaded_group_custom_field(field) reloadable_patch do |plugin| - ::Group.preloaded_custom_field_names << field if plugin.enabled? + ::Group.preloaded_custom_field_names << field end end + # Applies to all sites in a multisite environment. Ignores plugin.enabled? def add_preloaded_topic_list_custom_field(field) reloadable_patch do |plugin| - ::TopicList.preloaded_custom_fields << field if plugin.enabled? + ::TopicList.preloaded_custom_fields << field end end + # Add a permitted_create_param to Post, respecting if the plugin is enabled def add_permitted_post_create_param(name) reloadable_patch do |plugin| - ::Post.permitted_create_params << name if plugin.enabled? + ::Post.plugin_permitted_create_params[name] = plugin end end @@ -286,27 +294,31 @@ class Plugin::Instance end end + # Applies to all sites in a multisite environment. Ignores plugin.enabled? def register_category_custom_field_type(name, type) reloadable_patch do |plugin| - Category.register_custom_field_type(name, type) if plugin.enabled? + Category.register_custom_field_type(name, type) end end + # Applies to all sites in a multisite environment. Ignores plugin.enabled? def register_topic_custom_field_type(name, type) reloadable_patch do |plugin| - ::Topic.register_custom_field_type(name, type) if plugin.enabled? + ::Topic.register_custom_field_type(name, type) end end + # Applies to all sites in a multisite environment. Ignores plugin.enabled? def register_post_custom_field_type(name, type) reloadable_patch do |plugin| - ::Post.register_custom_field_type(name, type) if plugin.enabled? + ::Post.register_custom_field_type(name, type) end end + # Applies to all sites in a multisite environment. Ignores plugin.enabled? def register_group_custom_field_type(name, type) reloadable_patch do |plugin| - ::Group.register_custom_field_type(name, type) if plugin.enabled? + ::Group.register_custom_field_type(name, type) end end