SECURITY: ensure users have permission when moving categories

app/controllers/posts_controller.rb

@@ -200,6 +200,15 @@ class PostsController < ApplicationController
     if post.is_first_post?
       changes[:title] = params[:title] if params[:title]
       changes[:category_id] = params[:post][:category_id] if params[:post][:category_id]
+
+      if changes[:category_id] && changes[:category_id].to_i != post.topic.category_id.to_i
+        category = Category.find_by(id: changes[:category_id])
+        if category || (changes[:category_id].to_i == 0)
+          guardian.ensure_can_create_topic_on_category!(category)
+        else
+          return render_json_error(I18n.t('category.errors.not_found'))
+        end
+      end
     end
 
     # We don't need to validate edits to small action posts by staff

app/controllers/topics_controller.rb

@@ -226,6 +226,15 @@ class TopicsController < ApplicationController
     topic = Topic.find_by(id: params[:topic_id])
     guardian.ensure_can_edit!(topic)
 
+    if params[:category_id] && (params[:category_id].to_i != topic.category_id.to_i)
+      category = Category.find_by(id: params[:category_id])
+      if category || (params[:category_id].to_i == 0)
+        guardian.ensure_can_create_topic_on_category!(category)
+      else
+        return render_json_error(I18n.t('category.errors.not_found'))
+      end
+    end
+
     changes = {}
     PostRevisor.tracked_topic_fields.each_key do |f|
       changes[f] = params[f] if params.has_key?(f)

config/locales/server.en.yml

@@ -506,6 +506,7 @@ en:
     replace_paragraph: "(Replace this first paragraph with a brief description of your new category. This guidance will appear in the category selection area, so try to keep it below 200 characters. **Until you edit this description or create topics, this category won't appear on the categories page.**)"
     post_template: "%{replace_paragraph}\n\nUse the following paragraphs for a longer description, or to establish category guidelines or rules:\n\n- Why should people use this category? What is it for?\n\n- How exactly is this different than the other categories we already have?\n\n- What should topics in this category generally contain?\n\n- Do we need this category? Can we merge with another category, or subcategory?\n"
     errors:
+      not_found: "Category not found!"
       uncategorized_parent: "Uncategorized can't have a parent category"
       self_parent: "A subcategory's parent cannot be itself"
       depth: "You can't nest a subcategory under another"

lib/guardian/topic_guardian.rb

@@ -19,8 +19,12 @@ module TopicGuardian
   end
 
   def can_create_topic_on_category?(category)
+    # allow for category to be a number as well
+    category_id = category
+    category_id = category.id if Category === category
+
     can_create_topic?(nil) &&
-    (!category || Category.topic_create_allowed(self).where(id: category.id).count == 1)
+    (!category || Category.topic_create_allowed(self).where(id: category_id).count == 1)
   end
 
   def can_create_post_on_topic?(topic)

lib/post_revisor.rb

@@ -69,8 +69,10 @@ class PostRevisor
   end
 
   track_topic_field(:category_id) do |tc, category_id|
-    tc.record_change('category_id', tc.topic.category_id, category_id)
-    tc.check_result(tc.topic.change_category_to_id(category_id))
+    if tc.guardian.can_create_topic_on_category?(category_id)
+      tc.record_change('category_id', tc.topic.category_id, category_id)
+      tc.check_result(tc.topic.change_category_to_id(category_id))
+    end
   end
 
   track_topic_field(:tags) do |tc, tags|

spec/components/post_revisor_spec.rb

@@ -38,6 +38,32 @@ describe PostRevisor do
     end
   end
 
+  context 'editing category' do
+
+    it 'does not revise category when no permission to create a topic in category' do
+      category = Fabricate(:category)
+      category.set_permissions(staff: :full)
+      category.save!
+
+      post = create_post
+      old_id = post.topic.category_id
+
+      post.revise(post.user, category_id: category.id)
+
+      post.reload
+      expect(post.topic.category_id).to eq(old_id)
+
+      category.set_permissions(everyone: :full)
+      category.save!
+
+      post.revise(post.user, category_id: category.id)
+
+      post.reload
+      expect(post.topic.category_id).to eq(category.id)
+    end
+
+  end
+
   context 'revise wiki' do
 
     before do

spec/controllers/topics_controller_spec.rb

@@ -1136,22 +1136,6 @@ describe TopicsController do
           expect(@topic.title).to eq('This is a new title for the topic')
         end
 
-        it 'triggers a change of category' do
-          Topic.any_instance.expects(:change_category_to_id).with(123).returns(true)
-          put :update, params: {
-            topic_id: @topic.id, slug: @topic.title, category_id: 123
-          }, format: :json
-
-        end
-
-        it 'allows to change category to "uncategorized"' do
-          Topic.any_instance.expects(:change_category_to_id).with(0).returns(true)
-          put :update, params: {
-            topic_id: @topic.id, slug: @topic.title, category_id: ""
-          }, format: :json
-
-        end
-
         it "returns errors with invalid titles" do
           put :update, params: {
             topic_id: @topic.id, slug: @topic.title, title: 'asdf'
@@ -1170,7 +1154,6 @@ describe TopicsController do
         end
 
         it "returns errors with invalid categories" do
-          Topic.any_instance.expects(:change_category_to_id).returns(false)
           put :update, params: {
             topic_id: @topic.id, slug: @topic.title, category_id: -1
           }, format: :json
@@ -1197,8 +1180,9 @@ describe TopicsController do
           context 'when there are no changes' do
             it 'does not call the PostRevisor' do
               PostRevisor.any_instance.expects(:revise!).never
+
               put :update, params: {
-                topic_id: @topic.id, slug: @topic.title, title: @topic.title, category_id: nil
+                topic_id: @topic.id, slug: @topic.title, title: @topic.title, category_id: @topic.category_id
               }, format: :json
 
               expect(response).to be_success
@@ -1212,10 +1196,10 @@ describe TopicsController do
           end
 
           it "can add a category to an uncategorized topic" do
-            Topic.any_instance.expects(:change_category_to_id).with(456).returns(true)
+            c = Fabricate(:category)
 
             put :update, params: {
-              topic_id: @topic.id, slug: @topic.title, category_id: 456
+              topic_id: @topic.id, slug: @topic.title, category_id: c.id
             }, format: :json
 
             expect(response).to be_success

spec/requests/posts_controller_spec.rb

@@ -12,11 +12,47 @@ RSpec.describe PostsController do
 
   let(:private_post) { Fabricate(:post, user: user, topic: private_topic) }
 
+  describe '#update' do
+
+    it 'can not change category to a disallowed category' do
+      post = create_post
+      sign_in(post.user)
+
+      category = Fabricate(:category)
+      category.set_permissions(staff: :full)
+      category.save!
+
+      # strange API, why is topic id in here twice
+      put "/posts/#{post.id}.json", params: {
+        post: { category_id: category.id, raw: "this is a test edit to post" }
+      }
+
+      expect(response.status).not_to eq(200)
+      expect(post.topic.category_id).not_to eq(category.id)
+    end
+
+  end
+
   describe '#create' do
     before do
       sign_in(user)
     end
 
+    it 'can not create a post in a disallowed category' do
+
+      category.set_permissions(staff: :full)
+      category.save!
+
+      post "/posts.json", params: {
+        raw: 'this is the test content',
+        title: 'this is the test title for the topic',
+        category: category.id,
+        meta_data: { xyz: 'abc' }
+      }
+
+      expect(response.status).to eq(403)
+    end
+
     it 'creates the post' do
       post "/posts.json", params: {
         raw: 'this is the test content',

spec/requests/topics_controller_spec.rb

@@ -4,6 +4,25 @@ RSpec.describe TopicsController do
   let(:topic) { Fabricate(:topic) }
   let(:user) { Fabricate(:user) }
 
+  describe '#update' do
+
+    it 'can not change category to a disallowed category' do
+      post = create_post
+      sign_in(post.user)
+
+      category = Fabricate(:category)
+      category.set_permissions(staff: :full)
+      category.save!
+
+      # strange API, why is topic id in here twice
+      put "/t/#{post.topic_id}.json", params: { category_id: category.id, topic_id: post.topic_id }
+      expect(response.status).not_to eq(200)
+
+      expect(post.topic.category_id).not_to eq(category.id)
+    end
+
+  end
+
   describe '#show' do
     let(:private_topic) { Fabricate(:private_message_topic) }