FIX: Return 422 when creating topics with tags w/out permission (#10400)

FIX: Return 422 when creating topics with tags w/out permission (#10400)

The UI prevents users from trying to create tags on topics when they don’t have permission, but if you are trying to add tags to a topic via the API and you don’t have permission before this change it would silently succeed in creating the topic, but it wouldn’t have any tags.

Now a 422 error will be returned with an error message when trying to create a topic with tags when tagging is disabled or you don’t have enough trust level to add tags to a topic.

Bug report: https://meta.discourse.org/t/-/70525/14

diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index bef10ef..cfa0a44 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -530,6 +530,7 @@ en:
               reply_by_email_disabled: "Reply by email has been disabled."
               target_user_not_found: "One of the users you are sending this message to could not be found."
               unable_to_update: "There was an error updating that topic."
+              unable_to_tag: "There was an error tagging the topic."
             featured_link:
               invalid: "is invalid. URL should include http:// or https://."
               invalid_category: "can't be edited in this category."
diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb
index 5cd3ad7..7b47a52 100644
--- a/lib/discourse_tagging.rb
+++ b/lib/discourse_tagging.rb
@@ -78,7 +78,7 @@ module DiscourseTagging
         parent_tags_map = DB.query("
           SELECT tgm.tag_id, tg.parent_tag_id
             FROM tag_groups tg
-      INNER JOIN tag_group_memberships tgm
+          INNER JOIN tag_group_memberships tgm
               ON tgm.tag_group_id = tg.id
            WHERE tg.parent_tag_id IS NOT NULL
              AND tgm.tag_id IN (?)
@@ -112,8 +112,9 @@ module DiscourseTagging
         topic.tags = []
       end
       topic.tags_changed = true
+      return true
     end
-    true
+    false
   end
 
   def self.validate_min_required_tags_for_category(guardian, topic, category, tags = [])
diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb
index c52396b..e9a5ad1 100644
--- a/lib/topic_creator.rb
+++ b/lib/topic_creator.rb
@@ -165,7 +165,10 @@ class TopicCreator
       end
     else
       valid_tags = DiscourseTagging.tag_topic_by_names(topic, @guardian, @opts[:tags])
-      rollback_from_errors!(topic) unless valid_tags
+      unless valid_tags
+        topic.errors.add(:base, :unable_to_tag)
+        rollback_from_errors!(topic)
+      end
     end
   end
 
diff --git a/spec/components/new_post_manager_spec.rb b/spec/components/new_post_manager_spec.rb
index 983a626..e8f6fa1 100644
--- a/spec/components/new_post_manager_spec.rb
+++ b/spec/components/new_post_manager_spec.rb
@@ -289,6 +289,9 @@ describe NewPostManager do
     it "calls custom enqueuing handlers" do
       Reviewable.set_priorities(high: 20.5)
       SiteSetting.reviewable_default_visibility = 'high'
+      SiteSetting.tagging_enabled = true
+      SiteSetting.min_trust_to_create_tag = 0
+      SiteSetting.min_trust_level_to_tag_topics = 0
 
       manager = NewPostManager.new(
         topic.user,
diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb
index e013559..2838e86 100644
--- a/spec/components/post_creator_spec.rb
+++ b/spec/components/post_creator_spec.rb
@@ -406,7 +406,7 @@ describe PostCreator do
 
           it "doesn't create tags" do
             expect { @post = creator_with_tags.create }.to change { Tag.count }.by(0)
-            expect(@post.topic.tags.size).to eq(0)
+            expect(@post.topic&.tags&.size).to eq(nil)
           end
         end
 
diff --git a/spec/models/reviewable_queued_post_spec.rb b/spec/models/reviewable_queued_post_spec.rb
index 70842e3..7312544 100644
--- a/spec/models/reviewable_queued_post_spec.rb
+++ b/spec/models/reviewable_queued_post_spec.rb
@@ -143,6 +143,12 @@ RSpec.describe ReviewableQueuedPost, type: :model do
   context "creating a topic" do
     let(:reviewable) { Fabricate(:reviewable_queued_post_topic, category: category) }
 
+    before do
+      SiteSetting.tagging_enabled = true
+      SiteSetting.min_trust_to_create_tag = 0
+      SiteSetting.min_trust_level_to_tag_topics = 0
+    end
+
     context "editing" do
 
       it "is editable and returns the fields" do
diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb
index 30ec063..500ba1e 100644
--- a/spec/requests/posts_controller_spec.rb
+++ b/spec/requests/posts_controller_spec.rb
@@ -939,7 +939,7 @@ describe PostsController do
         expect(response.status).to eq(422)
       end
 
-      it 'can not create a post in a disallowed category' do
+      it 'cannot create a post in a disallowed category' do
         category.set_permissions(staff: :full)
         category.save!
 
@@ -953,7 +953,7 @@ describe PostsController do
         expect(response.status).to eq(403)
       end
 
-      it 'can not create a post with a tag that is restricted' do
+      it 'cannot create a post with a tag that is restricted' do
         SiteSetting.tagging_enabled = true
         tag = Fabricate(:tag)
         category.allowed_tags = [tag.name]
@@ -970,6 +970,51 @@ describe PostsController do
         expect(json['errors']).to be_present
       end
 
+      it 'cannot create a post with a tag when tagging is disabled' do
+        SiteSetting.tagging_enabled = false
+        tag = Fabricate(:tag)
+
+        post "/posts.json", params: {
+          raw: 'this is the test content',
+          title: 'this is the test title for the topic',
+          tags: [tag.name],
+        }
+
+        expect(response.status).to eq(422)
+        json = response.parsed_body
+        expect(json['errors']).to be_present
+      end
+
+      it 'cannot create a post with a tag without tagging permission' do
+        SiteSetting.tagging_enabled = true
+        SiteSetting.min_trust_level_to_tag_topics = 4
+        tag = Fabricate(:tag)
+
+        post "/posts.json", params: {
+          raw: 'this is the test content',
+          title: 'this is the test title for the topic',
+          tags: [tag.name],
+        }
+
+        expect(response.status).to eq(422)
+        json = response.parsed_body
+        expect(json['errors']).to be_present
+      end
+
+      it 'can create a post with a tag when tagging is enabled' do
+        SiteSetting.tagging_enabled = true
+        tag = Fabricate(:tag)
+
+        post "/posts.json", params: {
+          raw: 'this is the test content',
+          title: 'this is the test title for the topic',
+          tags: [tag.name],
+        }
+
+        expect(response.status).to eq(200)
+        expect(Post.last.topic.tags.count).to eq(1)
+      end
+
       it 'creates the post' do
         post "/posts.json", params: {
           raw: 'this is the test content',

GitHub sha: 2032c11f

1 Like

This commit appears in #10400 which was approved by nlalonde. It was merged by blake.