From 4f63bc8ed2ca3ac4a61b698f9cbcd322648cdea9 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Fri, 18 Nov 2022 14:37:36 +1100
Subject: [PATCH] FEATURE: hidden site setting to suppress unsecured categories
 from admins (#19098)

The hidden site setting `suppress_secured_categories_from_admin` will
suppress visibility of categories without explicit access from admins
in a few key areas (category drop downs and topic lists)

It is not intended to be a security wall since admins can amend any site
setting. Instead it is feature that allows hiding the categories from the
UI.

Admins will still be able to see topics in categories without explicit
access using direct URLs or flags.

Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
---
 app/models/user.rb                       |  8 +++++-
 config/site_settings.yml                 |  3 +++
 lib/guardian/topic_guardian.rb           | 14 +++++------
 spec/lib/guardian/topic_guardian_spec.rb | 32 ++++++++++++++++++++++--
 spec/models/user_spec.rb                 | 18 +++++++++++++
 5 files changed, 65 insertions(+), 10 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 4045830007c..8400c30a4e0 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1283,7 +1283,13 @@ class User < ActiveRecord::Base
   end
 
   def secure_category_ids
-    cats = self.admin? ? Category.unscoped.where(read_restricted: true) : secure_categories.references(:categories)
+    cats =
+      if self.admin? && !SiteSetting.suppress_secured_categories_from_admin
+        Category.unscoped.where(read_restricted: true)
+      else
+        secure_categories.references(:categories)
+      end
+
     cats.pluck('categories.id').sort
   end
 
diff --git a/config/site_settings.yml b/config/site_settings.yml
index bd52c577225..33e32eb071d 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -1771,6 +1771,9 @@ security:
     default: false
     client: true
     hidden: true
+  suppress_secured_categories_from_admin:
+    default: false
+    hidden: true
 
 onebox:
   post_onebox_maxlength:
diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb
index 04feb20b242..66df380baf4 100644
--- a/lib/guardian/topic_guardian.rb
+++ b/lib/guardian/topic_guardian.rb
@@ -262,13 +262,13 @@ module TopicGuardian
   end
 
   def filter_allowed_categories(records)
-    unless is_admin?
-      records = allowed_category_ids.size == 0 ?
-        records.where('topics.category_id IS NULL') :
-        records.where('topics.category_id IS NULL or topics.category_id IN (?)', allowed_category_ids)
-      records = records.references(:categories)
-    end
-    records
+    return records if is_admin? && !SiteSetting.suppress_secured_categories_from_admin
+
+    records = allowed_category_ids.size == 0 ?
+      records.where('topics.category_id IS NULL') :
+      records.where('topics.category_id IS NULL or topics.category_id IN (?)', allowed_category_ids)
+
+    records.references(:categories)
   end
 
   def can_edit_featured_link?(category_id)
diff --git a/spec/lib/guardian/topic_guardian_spec.rb b/spec/lib/guardian/topic_guardian_spec.rb
index c6da52b9ee4..bfe003457c3 100644
--- a/spec/lib/guardian/topic_guardian_spec.rb
+++ b/spec/lib/guardian/topic_guardian_spec.rb
@@ -6,9 +6,11 @@ RSpec.describe TopicGuardian do
   fab!(:tl3_user) { Fabricate(:leader) }
   fab!(:moderator) { Fabricate(:moderator) }
   fab!(:category) { Fabricate(:category) }
-  fab!(:topic) { Fabricate(:topic, category: category) }
-  fab!(:private_message_topic) { Fabricate(:private_message_topic) }
   fab!(:group) { Fabricate(:group) }
+  fab!(:private_category) { Fabricate(:private_category, group: group) }
+  fab!(:topic) { Fabricate(:topic, category: category) }
+  fab!(:private_topic) { Fabricate(:topic, category: private_category) }
+  fab!(:private_message_topic) { Fabricate(:private_message_topic) }
 
   before do
     Guardian.enable_topic_can_see_consistency_check
@@ -174,4 +176,30 @@ RSpec.describe TopicGuardian do
       )
     end
   end
+
+  describe '#filter_allowed_categories' do
+
+    it 'allows admin access to categories without explicit access' do
+      guardian = Guardian.new(admin)
+      list = Topic.where(id: private_topic.id)
+      list = guardian.filter_allowed_categories(list)
+
+      expect(list.count).to eq(1)
+    end
+
+    context 'when SiteSetting.suppress_secured_categories_from_admin is true' do
+      before do
+        SiteSetting.suppress_secured_categories_from_admin = true
+      end
+
+      it 'does not allow admin access to categories without explicit access' do
+        guardian = Guardian.new(admin)
+        list = Topic.where(id: private_topic.id)
+        list = guardian.filter_allowed_categories(list)
+
+        expect(list.count).to eq(0)
+      end
+    end
+
+  end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index b77e39ff74a..26af6b62289 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -3100,4 +3100,22 @@ RSpec.describe User do
       expect(user.visible_sidebar_tags).to contain_exactly(tag, hidden_tag)
     end
   end
+
+  describe '#secure_category_ids' do
+    fab!(:admin) { Fabricate(:admin) }
+    fab!(:group) { Fabricate(:group) }
+    fab!(:private_category) { Fabricate(:private_category, group: group) }
+
+    it 'allows admin to see all secure categories' do
+      expect(admin.secure_category_ids).to include(private_category.id)
+    end
+
+    context 'when SiteSetting.suppress_secured_categories_from_admin is true' do
+      it 'hides secure categories from admins' do
+        SiteSetting.suppress_secured_categories_from_admin = true
+        expect(admin.secure_category_ids).not_to include(private_category.id)
+      end
+    end
+
+  end
 end