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.

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!

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

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)

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

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)

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)