FIX: Validate category tag restrictions before sending new topics to review (#16292)

Tags (and tag groups) can be configured so that they can only be used in specific categories and (optionally) restrict topics in these categories to be able to add/use only these tags. These restrictions work as expected when a topic is created without going through the review queue; however, if the topic has to be reviewed by a moderator then these restrictions currently aren't checked before the topic is sent to the review queue, but they're checked later when a moderator tries to approve the topic. This is because if a user manages to submit a topic that doesn't meet the restrictions, moderators won't be able to approve and it'll be stuck in the review queue.

This PR prevents topics that don't meet the tags requirements from being sent to the review queue and shows the poster an error message that indicates which tags that cannot be used.

Internal ticket: t60562.

app/models/category.rb

@@ -933,6 +933,10 @@ class Category < ActiveRecord::Base
     end
   end
 
+  def has_restricted_tags?
+    tags.count > 0 || tag_groups.count > 0
+  end
+
   private
 
   def should_update_reviewables?

config/locales/server.en.yml

@@ -4810,6 +4810,12 @@ en:
         other: '"%{tag_name}" is restricted to the following categories: %{category_names}'
       synonym: 'Synonyms are not allowed. Use "%{tag_name}" instead.'
       has_synonyms: '"%{tag_name}" cannot be used because it has synonyms.'
+      restricted_tags_cannot_be_used_in_category:
+        one: 'The "%{tags}" tag cannot be used in the "%{category}" category. Please remove it.'
+        other: 'The following tags cannot be used in the "%{category}" category: %{tags}. Please remove them.'
+      category_does_not_allow_tags:
+        one: 'The "%{category}" category does not allow the "%{tags}" tag. Please remove it.'
+        other: 'The "%{category}" category does not allow the following tags: "%{tags}". Please remove them.'
     required_tags_from_group:
       one: "You must include at least %{count} %{tag_group_name} tag. The tags in this group are: %{tags}."
       other: "You must include at least %{count} %{tag_group_name} tags. The tags in this group are: %{tags}."

lib/discourse_tagging.rb

@@ -175,6 +175,51 @@ module DiscourseTagging
     end
   end
 
+  def self.validate_category_restricted_tags(guardian, model, category, tags = [])
+    return true if tags.blank? || category.blank?
+
+    tags = tags.map(&:name) if Tag === tags[0]
+    tags_restricted_to_categories = Hash.new { |h, k| h[k] = Set.new }
+
+    query = Tag.where(name: tags)
+    query.joins(tag_groups: :categories).pluck(:name, 'categories.id').each do |(tag, cat_id)|
+      tags_restricted_to_categories[tag] << cat_id
+    end
+    query.joins(:categories).pluck(:name, 'categories.id').each do |(tag, cat_id)|
+      tags_restricted_to_categories[tag] << cat_id
+    end
+
+    unallowed_tags = tags_restricted_to_categories.keys.select do |tag|
+      !tags_restricted_to_categories[tag].include?(category.id)
+    end
+
+    if unallowed_tags.present?
+      msg = I18n.t(
+        "tags.forbidden.restricted_tags_cannot_be_used_in_category",
+        count: unallowed_tags.size,
+        tags: unallowed_tags.sort.join(", "),
+        category: category.name
+      )
+      model.errors.add(:base, msg)
+      return false
+    end
+
+    if !category.allow_global_tags && category.has_restricted_tags?
+      unrestricted_tags = tags - tags_restricted_to_categories.keys
+      if unrestricted_tags.present?
+        msg = I18n.t(
+          "tags.forbidden.category_does_not_allow_tags",
+          count: unrestricted_tags.size,
+          tags: unrestricted_tags.sort.join(", "),
+          category: category.name
+        )
+        model.errors.add(:base, msg)
+        return false
+      end
+    end
+    true
+  end
+
   TAG_GROUP_RESTRICTIONS_SQL ||= <<~SQL
     tag_group_restrictions AS (
       SELECT t.id as tag_id, tgm.id as tgm_id, tg.id as tag_group_id, tg.parent_tag_id as parent_tag_id,

lib/topic_creator.rb

@@ -30,9 +30,10 @@ class TopicCreator
       existing_tags = tags.present? ? Tag.where(name: tags) : []
       valid_tags = guardian.can_create_tag? ? tags : existing_tags
 
-      # both add to topic.errors
+      # all add to topic.errors
       DiscourseTagging.validate_min_required_tags_for_category(guardian, topic, category, valid_tags)
       DiscourseTagging.validate_required_tags_from_group(guardian, topic, category, existing_tags)
+      DiscourseTagging.validate_category_restricted_tags(guardian, topic, category, valid_tags)
     end
 
     DiscourseEvent.trigger(:after_validate_topic, topic, self)

spec/integration/category_tag_spec.rb

@@ -31,11 +31,25 @@ describe "category tag restrictions" do
     end
 
     it "tags belonging to that category can only be used there" do
-      post = create_post(category: category_with_tags, tags: [tag1.name, tag2.name, tag3.name])
-      expect(post.topic.tags).to contain_exactly(tag1, tag2)
+      msg = I18n.t(
+        "tags.forbidden.category_does_not_allow_tags",
+        count: 1,
+        tags: tag3.name,
+        category: category_with_tags.name
+      )
+      expect {
+        create_post(category: category_with_tags, tags: [tag1.name, tag2.name, tag3.name])
+      }.to raise_error(StandardError, msg)
 
-      post = create_post(category: other_category, tags: [tag1.name, tag2.name, tag3.name])
-      expect(post.topic.tags).to contain_exactly(tag3)
+      msg = I18n.t(
+        "tags.forbidden.restricted_tags_cannot_be_used_in_category",
+        count: 2,
+        tags: [tag1, tag2].map(&:name).sort.join(", "),
+        category: other_category.name
+      )
+      expect {
+        create_post(category: other_category, tags: [tag1.name, tag2.name, tag3.name])
+      }.to raise_error(StandardError, msg)
     end
 
     it "search can show only permitted tags" do
@@ -55,10 +69,19 @@ describe "category tag restrictions" do
     end
 
     it "can't create new tags in a restricted category" do
-      post = create_post(category: category_with_tags, tags: [tag1.name, "newtag"])
-      expect_same_tag_names(post.topic.tags, [tag1])
-      post = create_post(category: category_with_tags, tags: [tag1.name, "newtag"], user: admin)
-      expect_same_tag_names(post.topic.tags, [tag1])
+      msg = I18n.t(
+        "tags.forbidden.category_does_not_allow_tags",
+        count: 1,
+        tags: "newtag",
+        category: category_with_tags.name
+      )
+      expect {
+        create_post(category: category_with_tags, tags: [tag1.name, "newtag"])
+      }.to raise_error(StandardError, msg)
+
+      expect {
+        create_post(category: category_with_tags, tags: [tag1.name, "newtag"], user: admin)
+      }.to raise_error(StandardError, msg)
     end
 
     it "can create new tags in a non-restricted category" do
@@ -149,8 +172,15 @@ describe "category tag restrictions" do
     end
 
     it "enforces restrictions when creating a topic" do
-      post = create_post(category: category, tags: [tag1.name, "newtag"])
-      expect(post.topic.tags.map(&:name)).to eq([tag1.name])
+      msg = I18n.t(
+        "tags.forbidden.category_does_not_allow_tags",
+        count: 1,
+        tags: "newtag",
+        category: category.name
+      )
+      expect {
+        create_post(category: category, tags: [tag1.name, "newtag"])
+      }.to raise_error(StandardError, msg)
     end
 
     it "handles colons" do

spec/lib/new_post_manager_spec.rb

@@ -52,6 +52,53 @@ describe NewPostManager do
       expect(result.post).to be_a(Post)
     end
 
+    it "doesn't enqueue topic if it doesn't meet the tag restrictions of the category" do
+      tag1 = Fabricate(:tag)
+      tag2 = Fabricate(:tag)
+      tag3 = Fabricate(:tag)
+      tag_group = Fabricate(:tag_group, tags: [tag2])
+      category = Fabricate(:category, tags: [tag1], tag_groups: [tag_group])
+      category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true
+      category.save!
+
+      manager = NewPostManager.new(
+        user,
+        raw: 'this is a new post',
+        title: 'this is a new post',
+        category: category.id,
+        tags: [tag1.name, tag3.name]
+      )
+      result = manager.perform
+      expect(result.success?).to eq(false)
+      expect(result.reviewable.persisted?).to eq(false)
+      expect(result.errors.full_messages).to contain_exactly(
+        I18n.t(
+          "tags.forbidden.category_does_not_allow_tags",
+          count: 1,
+          category: category.name,
+          tags: tag3.name
+        )
+      )
+
+      manager = NewPostManager.new(
+        user,
+        raw: 'this is a new post',
+        title: 'this is a new post',
+        category: category.id,
+        tags: [tag2.name, tag3.name]
+      )
+      result = manager.perform
+      expect(result.success?).to eq(false)
+      expect(result.reviewable.persisted?).to eq(false)
+      expect(result.errors.full_messages).to contain_exactly(
+        I18n.t(
+          "tags.forbidden.category_does_not_allow_tags",
+          count: 1,
+          category: category.name,
+          tags: tag3.name
+        )
+      )
+    end
   end
 
   context "default handler" do

spec/lib/topic_creator_spec.rb

@@ -84,6 +84,11 @@ describe TopicCreator do
     context 'tags' do
       fab!(:tag1) { Fabricate(:tag, name: "fun") }
       fab!(:tag2) { Fabricate(:tag, name: "fun2") }
+      fab!(:tag3) { Fabricate(:tag, name: "fun3") }
+      fab!(:tag4) { Fabricate(:tag, name: "fun4") }
+      fab!(:tag5) { Fabricate(:tag, name: "fun5") }
+      fab!(:tag_group1) { Fabricate(:tag_group, tags: [tag1]) }
+      fab!(:tag_group2) { Fabricate(:tag_group, tags: [tag2]) }
 
       before do
         SiteSetting.tagging_enabled = true
@@ -180,6 +185,203 @@ describe TopicCreator do
           ).to be_truthy
         end
       end
+
+      context "when category has restricted tags or tag groups" do
+        fab!(:category) { Fabricate(:category, tags: [tag3], tag_groups: [tag_group1]) }
+
+        it "allows topics without any tags" do
+          tc = TopicCreator.new(
+            user,
+            Guardian.new(user),
+            title: "hello this is a test topic without tags",
+            raw: "hello this is a test topic without tags",
+            category: category.id,
+          )
+          expect(tc.valid?).to eq(true)
+          expect(tc.errors).to be_empty
+          topic = tc.create
+          expect(topic.tags).to be_empty
+        end
+
+        it "allows topics if they use tags only from the tags set that the category restricts" do
+          tc = TopicCreator.new(
+            user,
+            Guardian.new(user),
+            title: "hello this is a test topic with tags",
+            raw: "hello this is a test topic with tags",
+            category: category.id,
+            tags: [tag1.name, tag3.name]
+          )
+          expect(tc.valid?).to eq(true)
+          expect(tc.errors).to be_empty
+          topic = tc.create
+          expect(topic.tags).to contain_exactly(tag1, tag3)
+        end
+
+        it "allows topics to use tags that are restricted in multiple categories" do
+          category2 = Fabricate(:category, tags: [tag5], tag_groups: [tag_group1])
+          tc = TopicCreator.new(
+            user,
+            Guardian.new(user),
+            title: "hello this is a test topic with tags",
+            raw: "hello this is a test topic with tags",
+            category: category2.id,
+            tags: [tag1.name, tag5.name]
+          )
+          expect(tc.valid?).to eq(true)
+          expect(tc.errors).to be_empty
+          topic = tc.create
+          expect(topic.tags).to contain_exactly(tag1, tag5)
+
+          tc = TopicCreator.new(
+            user,
+            Guardian.new(user),
+            title: "hello this is a test topic with tags 1",
+            raw: "hello this is a test topic with tags",
+            category: category.id,
+            tags: [tag1.name, tag3.name]
+          )
+          expect(tc.valid?).to eq(true)
+          expect(tc.errors).to be_empty
+          topic = tc.create
+          expect(topic.tags).to contain_exactly(tag1, tag3)
+
+          tc = TopicCreator.new(
+            user,
+            Guardian.new(user),
+            title: "hello this is a test topic with tags 2",
+            raw: "hello this is a test topic with tags",
+            category: category.id,
+            tags: [tag1.name, tag5.name]
+          )
+          expect(tc.valid?).to eq(false)
+          expect(tc.errors.full_messages).to contain_exactly(
+            I18n.t(
+              "tags.forbidden.restricted_tags_cannot_be_used_in_category",
+              count: 1,
+              tags: tag5.name,
+              category: category.name
+            )
+          )
+        end
+
+        it "rejects topics if they use a tag outside the set of tags that the category restricts" do
+          tc = TopicCreator.new(
+            user,
+            Guardian.new(user),
+            title: "hello this is a test topic with tags",
+            raw: "hello this is a test topic with tags",
+            category: category.id,
+            tags: [tag2.name, tag1.name]
+          )
+          expect(tc.valid?).to eq(false)
+          expect(tc.errors.full_messages).to contain_exactly(
+            I18n.t(
+              "tags.forbidden.category_does_not_allow_tags",
+              count: 1,
+              tags: tag2.name,
+              category: category.name
+            )
+          )
+
+          tc = TopicCreator.new(
+            user,
+            Guardian.new(user),
+            title: "hello this is a test topic with tags",
+            raw: "hello this is a test topic with tags",
+            category: category.id,
+            tags: [tag2.name, tag5.name, tag3.name]
+          )
+          expect(tc.valid?).to eq(false)
+          expect(tc.errors.full_messages).to contain_exactly(
+            I18n.t(
+              "tags.forbidden.category_does_not_allow_tags",
+              count: 2,
+              tags: [tag2, tag5].map(&:name).sort.join(", "),
+              category: category.name
+            )
+          )
+        end
+
+        it "rejects topics in other categories if a restricted tag of a category are used" do
+          category2 = Fabricate(:category)
+          tc = TopicCreator.new(
+            user,
+            Guardian.new(user),
+            title: "hello this is a test topic with tags",
+            raw: "hello this is a test topic with tags",
+            category: category2.id,
+            tags: [tag1.name, tag2.name]
+          )
+          expect(tc.valid?).to eq(false)
+          expect(tc.errors.full_messages).to contain_exactly(
+            I18n.t(
+              "tags.forbidden.restricted_tags_cannot_be_used_in_category",
+              count: 1,
+              tags: tag1.name,
+              category: category2.name
+            )
+          )
+        end
+
+        context "and allows other tags" do
+          before { category.update!(allow_global_tags: true) }
+
+          it "allows topics to use tags that aren't restricted by any category" do
+            tc = TopicCreator.new(
+              user,
+              Guardian.new(user),
+              title: "hello this is a test topic with tags",
+              raw: "hello this is a test topic with tags",
+              category: category.id,
+              tags: [tag1.name, tag2.name, tag3.name, tag5.name]
+            )
+            expect(tc.valid?).to eq(true)
+            expect(tc.errors).to be_empty
+            topic = tc.create
+            expect(topic.tags).to contain_exactly(tag1, tag2, tag3, tag5)
+          end
+
+          it "rejects topics if they use restricted tags of another category" do
+            Fabricate(:category, tags: [tag5], tag_groups: [tag_group2])
+            tc = TopicCreator.new(
+              user,
+              Guardian.new(user),
+              title: "hello this is a test topic with tags",
+              raw: "hello this is a test topic with tags",
+              category: category.id,
+              tags: [tag1.name, tag5.name]
+            )
+            expect(tc.valid?).to eq(false)
+            expect(tc.errors.full_messages).to contain_exactly(
+              I18n.t(
+                "tags.forbidden.restricted_tags_cannot_be_used_in_category",
+                count: 1,
+                tags: tag5.name,
+                category: category.name
+              )
+            )
+
+            tc = TopicCreator.new(
+              user,
+              Guardian.new(user),
+              title: "hello this is a test topic with tags",
+              raw: "hello this is a test topic with tags",
+              category: category.id,
+              tags: [tag1.name, tag2.name, tag5.name]
+            )
+            expect(tc.valid?).to eq(false)
+            expect(tc.errors.full_messages).to contain_exactly(
+              I18n.t(
+                "tags.forbidden.restricted_tags_cannot_be_used_in_category",
+                count: 2,
+                tags: [tag2, tag5].map(&:name).sort.join(", "),
+                category: category.name
+              )
+            )
+          end
+        end
+      end
     end
 
     context 'personal message' do