FIX: allowed_theme_ids should not be persisted in GlobalSettings (#14756)

FIX: allowed_theme_ids should not be persisted in GlobalSettings (#14756)

  • FIX: allowed_theme_ids should not be persisted in GlobalSettings

It was observed that the memoized value of GlobalSetting.allowed_theme_ids would be persisted across requests, which could lead to unpredictable/undesired behaviours in a multisite environment.

This change moves that logic out of GlobalSettings so that the returned theme IDs are correct for the current site.

Uses get_set_cache, which ultimately uses DistributedCache, which will take care of multisite issues for us.

diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb
index 95f9bbe..33cfe6d 100644
--- a/app/controllers/admin/themes_controller.rb
+++ b/app/controllers/admin/themes_controller.rb
@@ -282,7 +282,7 @@ class Admin::ThemesController < Admin::AdminController
   private
 
   def ban_in_allowlist_mode!
-    raise Discourse::InvalidAccess if !GlobalSetting.allowed_theme_ids.nil?
+    raise Discourse::InvalidAccess if !Theme.allowed_remote_theme_ids.nil?
   end
 
   def ban_for_remote_theme!
diff --git a/app/models/global_setting.rb b/app/models/global_setting.rb
index fba25ec..c83db37 100644
--- a/app/models/global_setting.rb
+++ b/app/models/global_setting.rb
@@ -238,23 +238,6 @@ class GlobalSetting
       end
   end
 
-  # test only
-  def self.reset_allowed_theme_ids!
-    @allowed_theme_ids = nil
-  end
-
-  def self.allowed_theme_ids
-    return nil if allowed_theme_repos.blank?
-
-    @allowed_theme_ids ||= begin
-      urls = allowed_theme_repos.split(",").map(&:strip)
-      Theme
-        .joins(:remote_theme)
-        .where('remote_themes.remote_url in (?)', urls)
-        .pluck(:id)
-    end
-  end
-
   def self.add_default(name, default)
     unless self.respond_to? name
       define_singleton_method(name) do
diff --git a/app/models/theme.rb b/app/models/theme.rb
index e6c81bb..41e4ba8 100644
--- a/app/models/theme.rb
+++ b/app/models/theme.rb
@@ -183,6 +183,18 @@ class Theme < ActiveRecord::Base
     end
   end
 
+  def self.allowed_remote_theme_ids
+    return nil if GlobalSetting.allowed_theme_repos.blank?
+
+    get_set_cache "allowed_remote_theme_ids" do
+      urls = GlobalSetting.allowed_theme_repos.split(",").map(&:strip)
+      Theme
+        .joins(:remote_theme)
+        .where('remote_themes.remote_url in (?)', urls)
+        .pluck(:id)
+    end
+  end
+
   def self.components_for(theme_id)
     get_set_cache "theme_components_for_#{theme_id}" do
       ChildTheme.where(parent_theme_id: theme_id).pluck(:child_theme_id)
diff --git a/lib/guardian.rb b/lib/guardian.rb
index 6932228..f623f45 100644
--- a/lib/guardian.rb
+++ b/lib/guardian.rb
@@ -494,7 +494,7 @@ class Guardian
   def allow_themes?(theme_ids, include_preview: false)
     return true if theme_ids.blank?
 
-    if allowed_theme_ids = GlobalSetting.allowed_theme_ids
+    if allowed_theme_ids = Theme.allowed_remote_theme_ids
       if (theme_ids - allowed_theme_ids).present?
         return false
       end
diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb
index 0185b32..3ff0420 100644
--- a/spec/components/guardian_spec.rb
+++ b/spec/components/guardian_spec.rb
@@ -3196,14 +3196,9 @@ describe Guardian do
 
     context "allowlist mode" do
       before do
-        GlobalSetting.reset_allowed_theme_ids!
         global_setting :allowed_theme_repos, "  https://magic.com/repo.git, https://x.com/git"
       end
 
-      after do
-        GlobalSetting.reset_allowed_theme_ids!
-      end
-
       it "should respect theme allowlisting" do
         r = RemoteTheme.create!(remote_url: "https://magic.com/repo.git")
         theme.update!(remote_theme_id: r.id)
diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb
index 472bc03..92b7020 100644
--- a/spec/requests/admin/themes_controller_spec.rb
+++ b/spec/requests/admin/themes_controller_spec.rb
@@ -102,26 +102,23 @@ describe Admin::ThemesController do
 
     context 'when theme allowlist mode is enabled' do
       before do
-        GlobalSetting.reset_allowed_theme_ids!
-        global_setting :allowed_theme_repos, "https://github.com/discourse/discourse-brand-header"
-      end
-
-      after do
-        GlobalSetting.reset_allowed_theme_ids!
+        global_setting :allowed_theme_repos, "https://github.com/discourse/discourse-brand-header.git"
       end
 
       it "allows allowlisted imports" do
-        RemoteTheme.stubs(:import_theme)
+        expect(Theme.allowed_remote_theme_ids.length).to eq(0)
+
         post "/admin/themes/import.json", params: {
-          remote: '    https://github.com/discourse/discourse-brand-header       '
+          remote: '    https://github.com/discourse/discourse-brand-header.git       '
         }
 
+        expect(Theme.allowed_remote_theme_ids.length).to eq(1)
         expect(response.status).to eq(201)
       end
 
       it "prevents adding disallowed themes" do
         RemoteTheme.stubs(:import_theme)
-        remote = '    https://bad.com/discourse/discourse-brand-header       '
+        remote = '    https://bad.com/discourse/discourse-brand-header.git       '
 
         post "/admin/themes/import.json", params: { remote: remote }
 
@@ -138,7 +135,7 @@ describe Admin::ThemesController do
     it 'can import a theme from Git' do
       RemoteTheme.stubs(:import_theme)
       post "/admin/themes/import.json", params: {
-        remote: '    https://github.com/discourse/discourse-brand-header       '
+        remote: '    https://github.com/discourse/discourse-brand-header.git       '
       }
 
       expect(response.status).to eq(201)
@@ -311,14 +308,9 @@ describe Admin::ThemesController do
 
     context 'when theme allowlist mode is enabled' do
       before do
-        GlobalSetting.reset_allowed_theme_ids!
         global_setting :allowed_theme_repos, "  https://magic.com/repo.git, https://x.com/git"
       end
 
-      after do
-        GlobalSetting.reset_allowed_theme_ids!
-      end
-
       it 'unconditionally bans theme_fields from updating' do
         r = RemoteTheme.create!(remote_url: "https://magic.com/repo.git")
         theme.update!(remote_theme_id: r.id)

GitHub sha: cfc62dbace3485f828d2691b8831b73d85a1dc94

This commit appears in #14756 which was approved by davidtaylorhq. It was merged by jbrw.