From 333b5a19b29cd8f190351385f96edbeb5cf1e947 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Wed, 26 Jun 2019 17:41:07 +1000 Subject: [PATCH] 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) --- app/models/category.rb | 13 ++++- spec/models/category_spec.rb | 106 ++++++++++++++++++++++++----------- 2 files changed, 86 insertions(+), 33 deletions(-) diff --git a/app/models/category.rb b/app/models/category.rb index 53f14e45d28..9ba9d914b5c 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 18cf7ce280a..8d1b49621c4 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 - can_read_category.set_permissions(group => :readonly) - can_read_category.save + fab!(:can_post_category) do + c = Fabricate(:category) + c.set_permissions(group => :create_post) + c.save + c + end - 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) + fab!(:can_read_category) do + c = Fabricate(:category) + c.set_permissions(group => :readonly) + c.save + 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 + let(:user_guardian) { Guardian.new(user) } + let(:admin_guardian) { Guardian.new(admin) } + let(:anon_guardian) { Guardian.new(nil) } - expect(Category.scoped_to_permissions(nil, [:readonly]).count).to be(2) + context 'when disabling uncategorized' do + before do + SiteSetting.allow_uncategorized_topics = false + end - # everyone has special semantics, test it as well + 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 + + 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 + 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 + + 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) + + # 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