From 0de7e4339c72c993835a30273752a12e4e956fb8 Mon Sep 17 00:00:00 2001
From: Daniel Waterworth <me@danielwaterworth.com>
Date: Mon, 14 Oct 2019 18:47:15 +0100
Subject: [PATCH] FIX: Subcategory permissions validation

When a category has a subcategory, we ensure that no one who can see the
subcategory cannot see the parent. However, we don't take into account
the fact that, when no CategoryGroups exist, the default is that
everyone has full permissions.
---
 app/models/category.rb       | 25 +++++++++++++++++++++----
 spec/models/category_spec.rb | 17 +++++++++++++++--
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/app/models/category.rb b/app/models/category.rb
index 0037cc96b44..c0f66dbea98 100644
--- a/app/models/category.rb
+++ b/app/models/category.rb
@@ -802,10 +802,27 @@ class Category < ActiveRecord::Base
   end
 
   def subcategories_permissions
-    CategoryGroup.joins(:category)
-      .where(['categories.parent_category_id = ?', self.id])
-      .pluck(:group_id, :permission_type)
-      .uniq
+    everyone = Group[:everyone].id
+    full = CategoryGroup.permission_types[:full]
+
+    result =
+      DB.query(<<-SQL, id: id, everyone: everyone, full: full)
+        SELECT category_groups.group_id, category_groups.permission_type
+        FROM categories, category_groups
+        WHERE categories.parent_category_id = :id
+        AND categories.id = category_groups.category_id
+        UNION
+        SELECT :everyone, :full
+        FROM categories
+        WHERE categories.parent_category_id = :id
+        AND (
+          SELECT DISTINCT 1
+          FROM category_groups
+          WHERE category_groups.category_id = categories.id
+        ) IS NULL
+      SQL
+
+    result.map { |row| [row.group_id, row.permission_type] }
   end
 end
 
diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb
index dcbfe2b7881..35a58303df5 100644
--- a/spec/models/category_spec.rb
+++ b/spec/models/category_spec.rb
@@ -870,10 +870,12 @@ describe Category do
     fab!(:group2) { Fabricate(:group) }
     fab!(:parent_category) { Fabricate(:category_with_definition, name: "parent") }
     fab!(:subcategory) { Fabricate(:category_with_definition, name: "child1", parent_category_id: parent_category.id) }
-    fab!(:subcategory2) { Fabricate(:category_with_definition, name: "child2", parent_category_id: parent_category.id) }
 
     context "when changing subcategory permissions" do
       it "it is not valid if permissions are less restrictive" do
+        subcategory.set_permissions(group => :readonly)
+        subcategory.save!
+
         parent_category.set_permissions(group => :readonly)
         parent_category.save!
 
@@ -884,6 +886,9 @@ describe Category do
       end
 
       it "is valid if permissions are same or more restrictive" do
+        subcategory.set_permissions(group => :full, group2 => :create_post)
+        subcategory.save!
+
         parent_category.set_permissions(group => :full, group2 => :create_post)
         parent_category.save!
 
@@ -903,7 +908,9 @@ describe Category do
     end
 
     context "when changing parent category permissions" do
-      it "it is not valid if subcategory permissions are less restrictive" do
+      fab!(:subcategory2) { Fabricate(:category_with_definition, name: "child2", parent_category_id: parent_category.id) }
+
+      it "is not valid if subcategory permissions are less restrictive" do
         subcategory.set_permissions(group => :create_post)
         subcategory.save!
         subcategory2.set_permissions(group => :create_post, group2 => :create_post)
@@ -915,6 +922,12 @@ describe Category do
         expect(parent_category.errors.full_messages).to contain_exactly(I18n.t("category.errors.permission_conflict", group_names: group2.name))
       end
 
+      it "is not valid if the subcategory has no category groups, but the parent does" do
+        parent_category.set_permissions(group => :readonly)
+
+        expect(parent_category).not_to be_valid
+      end
+
       it "is valid if subcategory permissions are same or more restrictive" do
         subcategory.set_permissions(group => :create_post)
         subcategory.save!