diff --git a/app/models/tag.rb b/app/models/tag.rb index 3d5224d4b98..6311dd3dc2f 100644 --- a/app/models/tag.rb +++ b/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 diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index 8e64bab2a56..2dc6fd493c0 100644 --- a/lib/discourse_tagging.rb +++ b/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 diff --git a/spec/lib/discourse_tagging_spec.rb b/spec/lib/discourse_tagging_spec.rb index ddcd88916d7..c952a899d33 100644 --- a/spec/lib/discourse_tagging_spec.rb +++ b/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