FIX: validate parent category/subcategories permissions

See: https://meta.discourse.org/t/subcategories-do-not-inherit-permissions-from-parent-category/17174/23 for more details

This ensures users with access to child category can always at least see parent

app/assets/javascripts/discourse/models/category.js.es6

@@ -99,7 +99,7 @@ const Category = RestModel.extend({
         color: this.get("color"),
         text_color: this.get("text_color"),
         secure: this.get("secure"),
-        permissions: this.get("permissionsForUpdate"),
+        permissions: this._permissionsForUpdate(),
         auto_close_hours: this.get("auto_close_hours"),
         auto_close_based_on_last_post: this.get(
           "auto_close_based_on_last_post"
@@ -135,8 +135,8 @@ const Category = RestModel.extend({
     });
   },
 
-  @computed("permissions")
+  _permissionsForUpdate() {
-  permissionsForUpdate(permissions) {
+    const permissions = this.get("permissions");
     let rval = {};
     permissions.forEach(p => (rval[p.group_name] = p.permission.id));
     return rval;

app/controllers/categories_controller.rb

@@ -140,7 +140,7 @@ class CategoriesController < ApplicationController
 
       render_serialized(@category, CategorySerializer)
     else
-      return render_json_error(@category) unless @category.save
+      return render_json_error(@category)
     end
   end
 

app/models/category.rb

@@ -55,6 +55,8 @@ class Category < ActiveRecord::Base
 
   validate :ensure_slug
 
+  validate :permissions_compatibility_validator
+
   validates :auto_close_hours, numericality: { greater_than: 0, less_than_or_equal_to: 87600 }, allow_nil: true
 
   after_create :create_category_definition
@@ -631,6 +633,43 @@ class Category < ActiveRecord::Base
       true
     end
   end
+
+  def permissions_compatibility_validator
+    # when saving subcategories
+    if @permissions && parent_category_id.present?
+      return if parent_category.category_groups.empty?
+
+      parent_permissions = parent_category.category_groups.pluck(:group_id, :permission_type)
+      child_permissions = @permissions.empty? ? [[Group[:everyone].id, CategoryGroup.permission_types[:full]]] : @permissions
+      check_permissions_compatibility(parent_permissions, child_permissions)
+
+    # when saving parent category
+    elsif @permissions && subcategories.present?
+      return if @permissions.empty?
+
+      parent_permissions = @permissions
+      child_permissions = subcategories_permissions.uniq
+
+      check_permissions_compatibility(parent_permissions, child_permissions)
+    end
+  end
+
+  private
+
+  def check_permissions_compatibility(parent_permissions, child_permissions)
+    parent_groups = parent_permissions.map(&:first)
+    child_groups = child_permissions.map(&:first)
+    only_in_subcategory = child_groups - parent_groups
+
+    errors.add(:base, I18n.t("category.errors.permission_conflict")) if only_in_subcategory.present?
+  end
+
+  def subcategories_permissions
+    CategoryGroup.joins(:category)
+      .where(['categories.parent_category_id = ?', self.id])
+      .pluck(:group_id, :permission_type)
+      .uniq
+  end
 end
 
 # == Schema Information

config/locales/server.en.yml

@@ -586,6 +586,7 @@ en:
       email_already_used_in_group: "'%{email}' is already used by the group '%{group_name}'."
       email_already_used_in_category: "'%{email}' is already used by the category '%{category_name}'."
       description_incomplete: "The category description post must have at least one paragraph."
+      permission_conflict: "Subcategory permissions cannot be less restrictive than parent's."
     cannot_delete:
       uncategorized: "Can't delete Uncategorized"
       has_subcategories: "Can't delete this category because it has sub-categories."

spec/models/category_spec.rb

@@ -751,4 +751,77 @@ describe Category do
     end
   end
 
+  describe "validate permissions compatibility" do
+    let(:admin) { Fabricate(:admin) }
+    let(:group) { Fabricate(:group) }
+    let(:group2) { Fabricate(:group) }
+    let(:parent_category) { Fabricate(:category, name: "parent") }
+    let(:subcategory) { Fabricate(:category, name: "child1", parent_category_id: parent_category.id) }
+    let(:subcategory2) { Fabricate(:category, 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
+        parent_category.set_permissions(group => :readonly)
+        parent_category.save!
+
+        subcategory.set_permissions(group => :full, group2 => :readonly)
+
+        expect(subcategory.valid?).to eq(false)
+        expect(subcategory.errors.full_messages).to eq([I18n.t("category.errors.permission_conflict")])
+      end
+
+      it "is valid if permissions are same or more restrictive" do
+        parent_category.set_permissions(group => :full, group2 => :create_post)
+        parent_category.save!
+
+        subcategory.set_permissions(group => :create_post, group2 => :full)
+
+        expect(subcategory.valid?).to eq(true)
+      end
+
+      it "is valid if no permissions are set on parent" do
+        parent_category.set_permissions(everyone: :full)
+        parent_category.save!
+
+        subcategory.set_permissions(group => :create_post, group2 => :create_post)
+
+        expect(subcategory.valid?).to eq(true)
+      end
+    end
+
+    context "when changing parent category permissions" do
+      it "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)
+        subcategory2.save!
+
+        parent_category.set_permissions(group => :readonly)
+
+        expect(parent_category.valid?).to eq(false)
+        expect(parent_category.errors.full_messages).to eq([I18n.t("category.errors.permission_conflict")])
+      end
+
+      it "is valid if subcategory permissions are same or more restrictive" do
+        subcategory.set_permissions(group => :create_post)
+        subcategory.save!
+        subcategory2.set_permissions(group => :create_post, group2 => :create_post)
+        subcategory2.save!
+
+        parent_category.set_permissions(group => :full, group2 => :create_post)
+
+        expect(parent_category.valid?).to eq(true)
+
+      end
+
+      it "is valid if no permissions set on parent" do
+        subcategory.set_permissions(group => :create_post)
+        subcategory.save
+        parent_category.set_permissions(everyone: :full)
+
+        expect(parent_category.valid?).to eq(true)
+      end
+    end
+  end
+
 end