FIX: When filtering tags for visibility, respect tag group permissions (#19152)

app/models/tag.rb

@@ -108,7 +108,7 @@ class Tag < ActiveRecord::Base
 
     return [] if scope_category_ids.empty?
 
-    filter_sql = guardian&.is_staff? ? '' : " AND tags.id NOT IN (#{DiscourseTagging.hidden_tags_query.select(:id).to_sql})"
+    filter_sql = guardian&.is_staff? ? '' : " AND tags.id IN (#{DiscourseTagging.visible_tags(guardian).select(:id).to_sql})"
 
     tag_names_with_counts = DB.query <<~SQL
       SELECT tags.name as tag_name, SUM(stats.topic_count) AS sum_topic_count

lib/discourse_tagging.rb

@@ -458,35 +458,61 @@ module DiscourseTagging
     end
   end
 
+  def self.visible_tags(guardian)
+    if guardian&.is_staff?
+      Tag.all
+    else
+      # Visible tags either have no permissions or have allowable permissions
+      Tag
+        .where.not(
+          id:
+            TagGroupMembership
+              .joins(tag_group: :tag_group_permissions)
+              .select(:tag_id)
+        )
+        .or(
+          Tag
+            .where(
+              id:
+                TagGroupPermission
+                  .joins(tag_group: :tag_group_memberships)
+                  .where(group_id: permitted_group_ids_query(guardian))
+                  .select('tag_group_memberships.tag_id'),
+            )
+        )
+    end
+  end
+
   def self.filter_visible(query, guardian = nil)
-    guardian&.is_staff? ? query : query.where("tags.id NOT IN (#{hidden_tags_query.select(:id).to_sql})")
+    guardian&.is_staff? ? query : query.where(id: visible_tags(guardian).select(:id))
   end
 
   def self.hidden_tag_names(guardian = nil)
-    guardian&.is_staff? ? [] : hidden_tags_query.pluck(:name) - permitted_tag_names(guardian)
+    guardian&.is_staff? ? [] : Tag.where.not(id: visible_tags(guardian).select(:id)).pluck(:name)
   end
 
-  # most restrictive level of tag groups
-  def self.hidden_tags_query
-    query = Tag.joins(:tag_groups)
-      .where('tag_groups.id NOT IN (
-        SELECT tag_group_id
-        FROM tag_group_permissions
-        WHERE group_id = ?)',
-        Group::AUTO_GROUPS[:everyone]
-      )
-
-    query
+  def self.permitted_group_ids_query(guardian = nil)
+    if guardian&.authenticated?
+      Group
+        .from(
+          Group.sanitize_sql(
+            ["(SELECT ? AS id UNION #{guardian.user.groups.select(:id).to_sql}) as groups", Group::AUTO_GROUPS[:everyone]]
+          )
+        )
+        .select(:id)
+    else
+      Group
+        .from(
+          Group.sanitize_sql(
+            ["(SELECT ? AS id) AS groups", Group::AUTO_GROUPS[:everyone]]
+          )
+        )
+        .select(:id)
+    end
   end
 
   def self.permitted_group_ids(guardian = nil)
-    group_ids = [Group::AUTO_GROUPS[:everyone]]
-
-    if guardian&.authenticated?
-      group_ids.concat(guardian.user.groups.pluck(:id))
-    end
-
-    group_ids
+    permitted_group_ids_query(guardian).pluck(:id)
   end
 
   # read-only tags for this user
@@ -502,11 +528,15 @@ module DiscourseTagging
 
   # explicit permissions to use these tags
   def self.permitted_tag_names(guardian = nil)
-    query = Tag.joins(tag_groups: :tag_group_permissions)
-      .where('tag_group_permissions.group_id IN (?) AND tag_group_permissions.permission_type = ?',
-        permitted_group_ids(guardian),
-        TagGroupPermission.permission_types[:full]
-      )
+    query =
+      Tag
+        .joins(tag_groups: :tag_group_permissions)
+        .where(
+          tag_group_permissions: {
+            group_id: permitted_group_ids(guardian),
+            permission_type: TagGroupPermission.permission_types[:full],
+          },
+        )
 
     query.pluck(:name).uniq
   end
@@ -516,11 +546,16 @@ module DiscourseTagging
     tag_names = Discourse.cache.read(TAGS_STAFF_CACHE_KEY)
 
     if !tag_names
-      tag_names = Tag.joins(tag_groups: :tag_group_permissions)
-        .where('tag_group_permissions.group_id = ? AND tag_group_permissions.permission_type = ?',
-          Group::AUTO_GROUPS[:everyone],
-          TagGroupPermission.permission_types[:readonly]
-        ).pluck(:name)
+      tag_names =
+        Tag
+          .joins(tag_groups: :tag_group_permissions)
+          .where(
+            tag_group_permissions: {
+              group_id: Group::AUTO_GROUPS[:everyone],
+              permission_type: TagGroupPermission.permission_types[:readonly],
+            },
+          )
+          .pluck(:name)
       Discourse.cache.write(TAGS_STAFF_CACHE_KEY, tag_names, expires_in: 1.hour)
     end
 

spec/lib/discourse_tagging_spec.rb

@@ -8,6 +8,7 @@ require 'discourse_tagging'
 RSpec.describe DiscourseTagging do
   fab!(:admin) { Fabricate(:admin) }
   fab!(:user)  { Fabricate(:user) }
+  let(:admin_guardian) { Guardian.new(admin) }
   let(:guardian) { Guardian.new(user) }
 
   fab!(:tag1) { Fabricate(:tag, name: "fun") }
@@ -20,6 +21,82 @@ RSpec.describe DiscourseTagging do
     SiteSetting.min_trust_level_to_tag_topics = 0
   end
 
+  describe 'visible_tags' do
+    fab!(:tag4) { Fabricate(:tag, name: "fun4") }
+
+    fab!(:user2)  { Fabricate(:user) }
+    let(:guardian2) { Guardian.new(user2) }
+
+    fab!(:group) { Fabricate(:group, name: "my-group") }
+    fab!(:group_user) { Fabricate(:group_user, group: group, user: user) }
+
+    fab!(:tag_group2) do
+      Fabricate(:tag_group, permissions: { "everyone" => 1 }, tag_names: [tag2.name])
+    end
+
+    fab!(:tag_group3) do
+      Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [tag3.name])
+    end
+
+    fab!(:tag_group4) do
+      Fabricate(:tag_group, permissions: { "my-group" => 1 }, tag_names: [tag4.name])
+    end
+
+    context 'for admin' do
+      it "includes tags with no tag_groups" do
+        expect(DiscourseTagging.visible_tags(admin_guardian)).to include(tag1)
+      end
+
+      it "includes tags with tags visible to everyone" do
+        expect(DiscourseTagging.visible_tags(admin_guardian)).to include(tag2)
+      end
+
+      it "includes tags which are visible to staff" do
+        expect(DiscourseTagging.visible_tags(admin_guardian)).to include(tag3)
+      end
+
+      it "includes tags which are visible to members of certain groups" do
+        expect(DiscourseTagging.visible_tags(admin_guardian)).to include(tag4)
+      end
+    end
+
+    context 'for users in a group' do
+      it "includes tags with no tag_groups" do
+        expect(DiscourseTagging.visible_tags(guardian)).to include(tag1)
+      end
+
+      it "includes tags with tags visible to everyone" do
+        expect(DiscourseTagging.visible_tags(guardian)).to include(tag2)
+      end
+
+      it "does not include tags which are only visible to staff" do
+        expect(DiscourseTagging.visible_tags(guardian)).not_to include(tag3)
+      end
+
+      it "includes tags which are visible to members of the group" do
+        expect(DiscourseTagging.visible_tags(guardian)).to include(tag4)
+      end
+    end
+
+    context 'for other users' do
+      it "includes tags with no tag_groups" do
+        expect(DiscourseTagging.visible_tags(guardian2)).to include(tag1)
+      end
+
+      it "includes tags with tags visible to everyone" do
+        expect(DiscourseTagging.visible_tags(guardian2)).to include(tag2)
+      end
+
+      it "does not include tags which are only visible to staff" do
+        expect(DiscourseTagging.visible_tags(guardian2)).not_to include(tag3)
+      end
+
+      it "does not include tags which are visible to members of another group" do
+        expect(DiscourseTagging.visible_tags(guardian2)).not_to include(tag4)
+      end
+    end
+  end
+
   describe 'filter_allowed_tags' do
     context 'for input fields' do
       it "doesn't return selected tags if there's a search term" do