From 0f598ca51e7ada06f91a6a8717909627ee81a67c Mon Sep 17 00:00:00 2001 From: Natalie Tay Date: Wed, 1 Dec 2021 10:26:56 +0800 Subject: [PATCH] SECURITY: Only show tags to users with permission (#15148) --- app/models/tag_user.rb | 19 ++++- app/services/post_alerter.rb | 20 +++-- .../tag_group_permission_fabricator.rb | 7 ++ spec/models/tag_user_spec.rb | 61 +++++++++++++ spec/services/post_alerter_spec.rb | 85 +++++++++++++++++++ 5 files changed, 184 insertions(+), 8 deletions(-) create mode 100644 spec/fabricators/tag_group_permission_fabricator.rb diff --git a/app/models/tag_user.rb b/app/models/tag_user.rb index 6870ffbb5b5..ecb781673da 100644 --- a/app/models/tag_user.rb +++ b/app/models/tag_user.rb @@ -4,6 +4,20 @@ class TagUser < ActiveRecord::Base belongs_to :tag belongs_to :user + scope :notification_level_visible, -> (notification_levels = TagUser.notification_levels.values) { + select("tag_users.*") + .distinct + .joins("LEFT OUTER JOIN tag_group_memberships ON tag_users.tag_id = tag_group_memberships.tag_id") + .joins("LEFT OUTER JOIN tag_group_permissions ON tag_group_memberships.tag_group_id = tag_group_permissions.tag_group_id") + .joins("LEFT OUTER JOIN group_users on group_users.user_id = tag_users.user_id") + .where("(tag_group_permissions.group_id IS NULL + OR tag_group_permissions.group_id = group_users.group_id + OR group_users.group_id = :staff_group_id) + AND tag_users.notification_level IN (:notification_levels)", + staff_group_id: Group::AUTO_GROUPS[:staff], + notification_levels: notification_levels) + } + def self.notification_levels NotificationLevels.all end @@ -208,7 +222,10 @@ class TagUser < ActiveRecord::Base [name, self.notification_levels[:muted]] end else - notification_levels = TagUser.where(user: user).joins(:tag).pluck("tags.name", :notification_level) + notification_levels = TagUser + .notification_level_visible + .where(user: user) + .joins(:tag).pluck("tags.name", :notification_level) end Hash[*notification_levels.flatten] diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index d311391abaa..aa7db9f4464 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -185,9 +185,10 @@ class PostAlerter end def tag_watchers(topic) - topic.tag_users - .where(notification_level: TagUser.notification_levels[:watching_first_post]) - .pluck(:user_id) + topic + .tag_users + .notification_level_visible([TagUser.notification_levels[:watching_first_post]]) + .distinct(:user_id).pluck(:user_id) end def category_watchers(topic) @@ -783,9 +784,13 @@ class PostAlerter FROM tag_users LEFT JOIN topic_users tu ON tu.user_id = tag_users.user_id AND tu.topic_id = :topic_id - WHERE tag_users.notification_level = :watching - AND tag_users.tag_id IN (:tag_ids) - AND (tu.user_id IS NULL OR tu.notification_level = :watching) + LEFT JOIN tag_group_memberships tgm ON tag_users.tag_id = tgm.tag_id + LEFT JOIN tag_group_permissions tgp ON tgm.tag_group_id = tgp.tag_group_id + LEFT JOIN group_users gu ON gu.user_id = tag_users.user_id + WHERE (tgp.group_id IS NULL OR tgp.group_id = gu.group_id OR gu.group_id = :staff_group_id) + AND (tag_users.notification_level = :watching + AND tag_users.tag_id IN (:tag_ids) + AND (tu.user_id IS NULL OR tu.notification_level = :watching)) SQL end @@ -793,7 +798,8 @@ class PostAlerter watching: TopicUser.notification_levels[:watching], topic_id: post.topic_id, category_id: post.topic.category_id, - tag_ids: tag_ids + tag_ids: tag_ids, + staff_group_id: Group::AUTO_GROUPS[:staff] ) if group_ids.present? diff --git a/spec/fabricators/tag_group_permission_fabricator.rb b/spec/fabricators/tag_group_permission_fabricator.rb new file mode 100644 index 00000000000..593c39c96a5 --- /dev/null +++ b/spec/fabricators/tag_group_permission_fabricator.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +Fabricator(:tag_group_permission) do + tag_group + group + permission_type TagGroupPermission.permission_types[:readonly] +end diff --git a/spec/models/tag_user_spec.rb b/spec/models/tag_user_spec.rb index 3777d8c8f08..5fa07068278 100644 --- a/spec/models/tag_user_spec.rb +++ b/spec/models/tag_user_spec.rb @@ -22,6 +22,58 @@ describe TagUser do TagUser.notification_levels[:watching] end + context "notification_level_visible" do + let!(:tag1) { Fabricate(:tag) } + let!(:tag2) { Fabricate(:tag) } + let!(:tag3) { Fabricate(:tag) } + let!(:tag4) { Fabricate(:tag) } + fab!(:user1) { Fabricate(:user) } + fab!(:user2) { Fabricate(:user) } + let!(:tag_user1) { TagUser.create(user: user1, tag: tag1, notification_level: TagUser.notification_levels[:watching]) } + let!(:tag_user2) { TagUser.create(user: user1, tag: tag2, notification_level: TagUser.notification_levels[:tracking]) } + let!(:tag_user3) { TagUser.create(user: user2, tag: tag3, notification_level: TagUser.notification_levels[:watching_first_post]) } + let!(:tag_user4) { TagUser.create(user: user2, tag: tag4, notification_level: TagUser.notification_levels[:muted]) } + + it "scopes to notification levels visible due to absence of tag group" do + expect(TagUser.notification_level_visible.length).to be(4) + end + + it "scopes to notification levels visible by tag group permission" do + group1 = Fabricate(:group) + tag_group1 = Fabricate(:tag_group, tags: [tag1]) + Fabricate(:tag_group_permission, tag_group: tag_group1, group: group1) + + group2 = Fabricate(:group) + tag_group2 = Fabricate(:tag_group, tags: [tag2]) + Fabricate(:tag_group_permission, tag_group: tag_group2, group: group2) + + Fabricate(:group_user, group: group1, user: user1) + + expect(TagUser.notification_level_visible.pluck(:id)).to match_array([ + tag_user1.id, tag_user3.id, tag_user4.id + ]) + end + + it "scopes to notification levels visible because user is staff" do + group2 = Fabricate(:group) + tag_group2 = Fabricate(:tag_group, tags: [tag2]) + Fabricate(:tag_group_permission, tag_group: tag_group2, group: group2) + + staff_group = Group.find(Group::AUTO_GROUPS[:staff]) + Fabricate(:group_user, group: staff_group, user: user1) + + expect(TagUser.notification_level_visible.length).to be(4) + end + + it "scopes to notification levels visible by specified notification level" do + expect(TagUser.notification_level_visible([TagUser.notification_levels[:watching]]).length).to be(1) + expect(TagUser.notification_level_visible( + [TagUser.notification_levels[:watching], + TagUser.notification_levels[:tracking]] + ).length).to be(2) + end + end + context "change" do it "watches or tracks on change" do user = Fabricate(:user) @@ -264,6 +316,7 @@ describe TagUser do TagUser.create(user: user, tag: tag3, notification_level: TagUser.notification_levels[:watching_first_post]) TagUser.create(user: user, tag: tag4, notification_level: TagUser.notification_levels[:muted]) end + it "gets the tag_user notification levels for all tags the user is tracking and does not include tags the user is not tracking at all" do tag5 = Fabricate(:tag) @@ -274,6 +327,14 @@ describe TagUser do expect(levels[tag4.name]).to eq(TagUser.notification_levels[:muted]) expect(levels.key?(tag5.name)).to eq(false) end + + it "does not show a tag is tracked if the user does not belong to the tag group with permissions" do + group = Fabricate(:group) + tag_group = Fabricate(:tag_group, tags: [tag2]) + Fabricate(:tag_group_permission, tag_group: tag_group, group: group) + + expect(TagUser.notification_levels_for(user).keys).to match_array([tag1.name, tag3.name, tag4.name]) + end end end end diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index edc0a044e6b..d8a2c7722f2 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -1285,6 +1285,33 @@ describe PostAlerter do notification_data = JSON.parse(notification.data) expect(notification_data["display_username"]).to eq(I18n.t("embed.replies", count: 2)) end + + it "does not add notification if user does not belong to tag group with permissions" do + tag = Fabricate(:tag) + topic = Fabricate(:topic, tags: [tag]) + post = Fabricate(:post, topic: topic) + tag_group = Fabricate(:tag_group, tags: [tag]) + group = Fabricate(:group) + Fabricate(:tag_group_permission, tag_group: tag_group, group: group) + + TagUser.change(user.id, tag.id, TagUser.notification_levels[:watching]) + + expect { PostAlerter.post_created(post) }.not_to change { Notification.count } + end + + it "adds notification if user belongs to tag group with permissions" do + tag = Fabricate(:tag) + topic = Fabricate(:topic, tags: [tag]) + post = Fabricate(:post, topic: topic) + tag_group = Fabricate(:tag_group, tags: [tag]) + group = Fabricate(:group) + Fabricate(:group_user, group: group, user: user) + Fabricate(:tag_group_permission, tag_group: tag_group, group: group) + + TagUser.change(user.id, tag.id, TagUser.notification_levels[:watching]) + + expect { PostAlerter.post_created(post) }.to change { Notification.count }.by(1) + end end context "on change" do @@ -1351,6 +1378,64 @@ describe PostAlerter do expect(Notification.where(user_id: admin.id).count).to eq(1) end end + + context "with tag groups" do + fab!(:tag) { Fabricate(:tag) } + fab!(:group) { Fabricate(:group) } + fab!(:user) { Fabricate(:user) } + fab!(:topic) { Fabricate(:topic, tags: [tag]) } + fab!(:post) { Fabricate(:post, topic: topic) } + + shared_examples "tag user with notification level" do |notification_level, notification_type| + it "notifies a user who is watching a tag that does not belong to a tag group" do + TagUser.change(user.id, tag.id, TagUser.notification_levels[notification_level]) + PostAlerter.post_created(post) + expect(user.notifications.where(notification_type: Notification.types[notification_type]).count).to eq(1) + end + + it "does not notify a user watching a tag with tag group permissions that he does not belong to" do + tag_group = Fabricate(:tag_group, tags: [tag]) + Fabricate(:tag_group_permission, tag_group: tag_group, group: group) + + TagUser.change(user.id, tag.id, TagUser.notification_levels[notification_level]) + + PostAlerter.post_created(post) + + expect(user.notifications.where(notification_type: Notification.types[notification_type]).count).to eq(0) + end + + it "notifies a user watching a tag with tag group permissions that he belongs to" do + Fabricate(:group_user, group: group, user: user) + + TagUser.change(user.id, tag.id, TagUser.notification_levels[notification_level]) + + PostAlerter.post_created(post) + + expect(user.notifications.where(notification_type: Notification.types[notification_type]).count).to eq(1) + end + + it "notifies a staff watching a tag with tag group permissions that he does not belong to" do + tag_group = Fabricate(:tag_group, tags: [tag]) + Fabricate(:tag_group_permission, tag_group: tag_group, group: group) + staff_group = Group.find(Group::AUTO_GROUPS[:staff]) + Fabricate(:group_user, group: staff_group, user: user) + + TagUser.change(user.id, tag.id, TagUser.notification_levels[notification_level]) + + PostAlerter.post_created(post) + + expect(user.notifications.where(notification_type: Notification.types[notification_type]).count).to eq(1) + end + end + + context "with :watching notification level" do + include_examples "tag user with notification level", :watching, :posted + end + + context "with :watching_first_post notification level" do + include_examples "tag user with notification level", :watching_first_post, :watching_first_post + end + end end describe '#extract_linked_users' do