FIX: do not include uncategorized_category_id in `topic_create_allowed` if posting in uncategorized is disabled

FIX: do not include uncategorized_category_id in topic_create_allowed if posting in uncategorized is disabled

Previously users were still allowed to create topic via API even if uncategorized was disabled.

Not 100% happy with all this special casing, but I guess we have to do something.

This also splits up a mega spec now that we have fab! into a more easy to understand structure (I hope)

diff --git a/app/models/category.rb b/app/models/category.rb
index 53f14e4..9ba9d91 100644
--- a/app/models/category.rb
+++ b/app/models/category.rb
@@ -109,7 +109,18 @@ class Category < ActiveRecord::Base
 
   TOPIC_CREATION_PERMISSIONS ||= [:full]
   POST_CREATION_PERMISSIONS  ||= [:create_post, :full]
-  scope :topic_create_allowed, -> (guardian) { scoped_to_permissions(guardian, TOPIC_CREATION_PERMISSIONS) }
+
+  scope :topic_create_allowed, -> (guardian) do
+
+    scoped = scoped_to_permissions(guardian, TOPIC_CREATION_PERMISSIONS)
+
+    if !SiteSetting.allow_uncategorized_topics && !guardian.is_staff?
+      scoped = scoped.where.not(id: SiteSetting.uncategorized_category_id)
+    end
+
+    scoped
+  end
+
   scope :post_create_allowed,  -> (guardian) { scoped_to_permissions(guardian, POST_CREATION_PERMISSIONS) }
 
   delegate :post_template, to: 'self.class'
diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb
index 18cf7ce..8d1b496 100644
--- a/spec/models/category_spec.rb
+++ b/spec/models/category_spec.rb
@@ -97,55 +97,97 @@ describe Category do
   end
 
   describe "topic_create_allowed and post_create_allowed" do
-    it "works" do
-
-      # NOTE we also have the uncategorized category ... hence the increased count
-
-      _default_category = Fabricate(:category)
-      full_category = Fabricate(:category)
-      can_post_category = Fabricate(:category)
-      can_read_category = Fabricate(:category)
+    fab!(:group) { Fabricate(:group) }
 
+    fab!(:user) do
       user = Fabricate(:user)
-      group = Fabricate(:group)
       group.add(user)
       group.save
+      user
+    end
 
-      admin = Fabricate(:admin)
+    fab!(:admin) { Fabricate(:admin) }
 
-      full_category.set_permissions(group => :full)
-      full_category.save
+    fab!(:default_category) { Fabricate(:category) }
 
-      can_post_category.set_permissions(group => :create_post)
-      can_post_category.save
+    fab!(:full_category) do
+      c = Fabricate(:category)
+      c.set_permissions(group => :full)
+      c.save
+      c
+    end
+
+    fab!(:can_post_category) do
+      c = Fabricate(:category)
+      c.set_permissions(group => :create_post)
+      c.save
+      c
+    end
+
+    fab!(:can_read_category) do
+      c = Fabricate(:category)
+      c.set_permissions(group => :readonly)
+      c.save
+    end
+
+    let(:user_guardian) { Guardian.new(user) }
+    let(:admin_guardian) { Guardian.new(admin) }
+    let(:anon_guardian) { Guardian.new(nil) }
+
+    context 'when disabling uncategorized' do
+      before do
+        SiteSetting.allow_uncategorized_topics = false
+      end
+
+      it "allows everything to admins unconditionally" do
+        count = Category.count
+
+        expect(Category.topic_create_allowed(admin_guardian).count).to eq(count)
+        expect(Category.post_create_allowed(admin_guardian).count).to eq(count)
+        expect(Category.secured(admin_guardian).count).to eq(count)
+      end
+
+      it "allows normal users correct access to all categories" do
+
+        # Sam: I am mixed here, once disabling uncategorized maybe users should no
+        # longer be allowed to know about it so all counts should go down?
+        expect(Category.secured(user_guardian).count).to eq(5)
+        expect(Category.post_create_allowed(user_guardian).count).to eq(4)
+        expect(Category.topic_create_allowed(user_guardian).count).to eq(2)
+      end
+    end
 
-      can_read_category.set_permissions(group => :readonly)
-      can_read_category.save
+    it "allows everything to admins unconditionally" do
+      count = Category.count
 
-      guardian = Guardian.new(admin)
-      expect(Category.topic_create_allowed(guardian).count).to be(5)
-      expect(Category.post_create_allowed(guardian).count).to be(5)
-      expect(Category.secured(guardian).count).to be(5)
+      expect(Category.topic_create_allowed(admin_guardian).count).to eq(count)
+      expect(Category.post_create_allowed(admin_guardian).count).to eq(count)
+      expect(Category.secured(admin_guardian).count).to eq(count)
+    end
 
-      guardian = Guardian.new(user)
-      expect(Category.secured(guardian).count).to be(5)
-      expect(Category.post_create_allowed(guardian).count).to be(4)
-      expect(Category.topic_create_allowed(guardian).count).to be(3) # explicitly allowed once, default allowed once
+    it "allows normal users correct access to all categories" do
+      expect(Category.secured(user_guardian).count).to eq(5)
+      expect(Category.post_create_allowed(user_guardian).count).to eq(4)
+      expect(Category.topic_create_allowed(user_guardian).count).to eq(3)
+    end
 
-      expect(Category.scoped_to_permissions(nil, [:readonly]).count).to be(2)
+    it "allows anon correct access" do
+      expect(Category.scoped_to_permissions(anon_guardian, [:readonly]).count).to eq(2)
+      expect(Category.post_create_allowed(anon_guardian).count).to eq(0)
+      expect(Category.topic_create_allowed(anon_guardian).count).to eq(0)
 
-      # everyone has special semantics, test it as well
+      # nil has special semantics
+      expect(Category.scoped_to_permissions(nil, [:readonly]).count).to eq(2)
+    end
+
+    it "handles :everyone scope" do
       can_post_category.set_permissions(everyone: :create_post)
       can_post_category.save
 
-      expect(Category.post_create_allowed(guardian).count).to be(4)
+      expect(Category.post_create_allowed(user_guardian).count).to eq(4)
 
       # anonymous has permission to create no topics
-      guardian = Guardian.new(nil)
-      expect(Category.post_create_allowed(guardian).count).to be(0)
-      expect(Category.topic_create_allowed(guardian).count).to be(0)
-      expect(Category.scoped_to_permissions(guardian, [:readonly]).count).to be(3)
-
+      expect(Category.scoped_to_permissions(user_guardian, [:readonly]).count).to eq(3)
     end
 
   end

GitHub sha: 333b5a19

2 Likes

DEV: correct edge case introduced in 333b5a19