FIX: check admin theme cookie against user selectable

previously admin got a free pass and could set theme via cookie to anything
including themes that are not selectable

this refactor ensures that only "preview" gets a free pass, all the rest
goes through the same pipeline

app/controllers/application_controller.rb

@@ -381,7 +381,8 @@ class ApplicationController < ActionController::Base
     theme_ids = []
 
     if preview_theme_id = request[:preview_theme_id]&.to_i
-      theme_ids << preview_theme_id
+      ids = [preview_theme_id]
+      theme_ids = ids if guardian.allow_themes?(ids, include_preview: true)
     end
 
     user_option = current_user&.user_option
@@ -394,10 +395,9 @@ class ApplicationController < ActionController::Base
       end
     end
 
-    theme_ids = user_option&.theme_ids || [] if theme_ids.blank?
-
-    unless guardian.allow_themes?(theme_ids)
-      theme_ids = []
+    if theme_ids.blank?
+      ids = user_option&.theme_ids || []
+      theme_ids = ids if guardian.allow_themes?(ids)
     end
 
     if theme_ids.blank? && SiteSetting.default_theme_id != -1

lib/guardian.rb

@@ -364,10 +364,10 @@ class Guardian
     UserExport.where(user_id: @user.id, created_at: (Time.zone.now.beginning_of_day..Time.zone.now.end_of_day)).count == 0
   end
 
-  def allow_themes?(theme_ids)
+  def allow_themes?(theme_ids, include_preview: false)
     return true if theme_ids.blank?
 
-    if is_staff? && (theme_ids - Theme.theme_ids).blank?
+    if include_preview && is_staff? && (theme_ids - Theme.theme_ids).blank?
       return true
     end
 

spec/components/guardian_spec.rb

@@ -2581,8 +2581,11 @@ describe Guardian do
     let(:theme2) { Fabricate(:theme) }
 
     it "allows staff to use any themes" do
-      expect(Guardian.new(moderator).allow_themes?([theme.id, theme2.id])).to eq(true)
-      expect(Guardian.new(admin).allow_themes?([theme.id, theme2.id])).to eq(true)
+      expect(Guardian.new(moderator).allow_themes?([theme.id, theme2.id])).to eq(false)
+      expect(Guardian.new(admin).allow_themes?([theme.id, theme2.id])).to eq(false)
+
+      expect(Guardian.new(moderator).allow_themes?([theme.id, theme2.id], include_preview: true)).to eq(true)
+      expect(Guardian.new(admin).allow_themes?([theme.id, theme2.id], include_preview: true)).to eq(true)
     end
 
     it "only allows normal users to use user-selectable themes or default theme" do

spec/requests/application_controller_spec.rb

@@ -92,6 +92,7 @@ RSpec.describe ApplicationController do
   describe "#handle_theme" do
     let(:theme) { Fabricate(:theme, user_selectable: true) }
     let(:theme2) { Fabricate(:theme, user_selectable: true) }
+    let(:non_selectable_theme) { Fabricate(:theme, user_selectable: false) }
     let(:user) { Fabricate(:user) }
     let(:admin) { Fabricate(:admin) }
 
@@ -148,6 +149,15 @@ RSpec.describe ApplicationController do
       get "/", params: { preview_theme_id: theme2.id }
       expect(response.status).to eq(200)
       expect(controller.theme_ids).to eq([theme2.id])
+
+      get "/", params: { preview_theme_id: non_selectable_theme.id }
+      expect(controller.theme_ids).to eq([non_selectable_theme.id])
+    end
+
+    it "does not allow non privileged user to preview themes" do
+      sign_in(user)
+      get "/", params: { preview_theme_id: non_selectable_theme.id }
+      expect(controller.theme_ids).to eq([SiteSetting.default_theme_id])
     end
 
     it "cookie can fail back to user if out of sync" do