Skip to content

Commit cdaf7f4

Browse files
committed
SECURITY: Only show tags to users with permission (#15148)
1 parent 715d4de commit cdaf7f4

File tree

5 files changed

+184
-8
lines changed

5 files changed

+184
-8
lines changed

Diff for: app/models/tag_user.rb

+18-1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,20 @@ class TagUser < ActiveRecord::Base
44
belongs_to :tag
55
belongs_to :user
66

7+
scope :notification_level_visible, -> (notification_levels = TagUser.notification_levels.values) {
8+
select("tag_users.*")
9+
.distinct
10+
.joins("LEFT OUTER JOIN tag_group_memberships ON tag_users.tag_id = tag_group_memberships.tag_id")
11+
.joins("LEFT OUTER JOIN tag_group_permissions ON tag_group_memberships.tag_group_id = tag_group_permissions.tag_group_id")
12+
.joins("LEFT OUTER JOIN group_users on group_users.user_id = tag_users.user_id")
13+
.where("(tag_group_permissions.group_id IS NULL
14+
OR tag_group_permissions.group_id = group_users.group_id
15+
OR group_users.group_id = :staff_group_id)
16+
AND tag_users.notification_level IN (:notification_levels)",
17+
staff_group_id: Group::AUTO_GROUPS[:staff],
18+
notification_levels: notification_levels)
19+
}
20+
721
def self.notification_levels
822
NotificationLevels.all
923
end
@@ -208,7 +222,10 @@ def self.notification_levels_for(user)
208222
[name, self.notification_levels[:muted]]
209223
end
210224
else
211-
notification_levels = TagUser.where(user: user).joins(:tag).pluck("tags.name", :notification_level)
225+
notification_levels = TagUser
226+
.notification_level_visible
227+
.where(user: user)
228+
.joins(:tag).pluck("tags.name", :notification_level)
212229
end
213230

214231
Hash[*notification_levels.flatten]

Diff for: app/services/post_alerter.rb

+13-7
Original file line numberDiff line numberDiff line change
@@ -125,9 +125,10 @@ def group_watchers(topic)
125125
end
126126

127127
def tag_watchers(topic)
128-
topic.tag_users
129-
.where(notification_level: TagUser.notification_levels[:watching_first_post])
130-
.pluck(:user_id)
128+
topic
129+
.tag_users
130+
.notification_level_visible([TagUser.notification_levels[:watching_first_post]])
131+
.distinct(:user_id).pluck(:user_id)
131132
end
132133

133134
def category_watchers(topic)
@@ -698,17 +699,22 @@ def notify_post_users(post, notified, include_topic_watchers: true, include_cate
698699
FROM tag_users
699700
LEFT JOIN topic_users tu ON tu.user_id = tag_users.user_id
700701
AND tu.topic_id = :topic_id
701-
WHERE tag_users.notification_level = :watching
702-
AND tag_users.tag_id IN (:tag_ids)
703-
AND (tu.user_id IS NULL OR tu.notification_level = :watching)
702+
LEFT JOIN tag_group_memberships tgm ON tag_users.tag_id = tgm.tag_id
703+
LEFT JOIN tag_group_permissions tgp ON tgm.tag_group_id = tgp.tag_group_id
704+
LEFT JOIN group_users gu ON gu.user_id = tag_users.user_id
705+
WHERE (tgp.group_id IS NULL OR tgp.group_id = gu.group_id OR gu.group_id = :staff_group_id)
706+
AND (tag_users.notification_level = :watching
707+
AND tag_users.tag_id IN (:tag_ids)
708+
AND (tu.user_id IS NULL OR tu.notification_level = :watching))
704709
SQL
705710
end
706711

707712
notify = User.where(condition,
708713
watching: TopicUser.notification_levels[:watching],
709714
topic_id: post.topic_id,
710715
category_id: post.topic.category_id,
711-
tag_ids: tag_ids
716+
tag_ids: tag_ids,
717+
staff_group_id: Group::AUTO_GROUPS[:staff]
712718
)
713719

714720
if post.topic.private_message?

Diff for: spec/fabricators/tag_group_permission_fabricator.rb

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
# frozen_string_literal: true
2+
3+
Fabricator(:tag_group_permission) do
4+
tag_group
5+
group
6+
permission_type TagGroupPermission.permission_types[:readonly]
7+
end

Diff for: spec/models/tag_user_spec.rb

+61
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,58 @@ def watching
2222
TagUser.notification_levels[:watching]
2323
end
2424

25+
context "notification_level_visible" do
26+
let!(:tag1) { Fabricate(:tag) }
27+
let!(:tag2) { Fabricate(:tag) }
28+
let!(:tag3) { Fabricate(:tag) }
29+
let!(:tag4) { Fabricate(:tag) }
30+
fab!(:user1) { Fabricate(:user) }
31+
fab!(:user2) { Fabricate(:user) }
32+
let!(:tag_user1) { TagUser.create(user: user1, tag: tag1, notification_level: TagUser.notification_levels[:watching]) }
33+
let!(:tag_user2) { TagUser.create(user: user1, tag: tag2, notification_level: TagUser.notification_levels[:tracking]) }
34+
let!(:tag_user3) { TagUser.create(user: user2, tag: tag3, notification_level: TagUser.notification_levels[:watching_first_post]) }
35+
let!(:tag_user4) { TagUser.create(user: user2, tag: tag4, notification_level: TagUser.notification_levels[:muted]) }
36+
37+
it "scopes to notification levels visible due to absence of tag group" do
38+
expect(TagUser.notification_level_visible.length).to be(4)
39+
end
40+
41+
it "scopes to notification levels visible by tag group permission" do
42+
group1 = Fabricate(:group)
43+
tag_group1 = Fabricate(:tag_group, tags: [tag1])
44+
Fabricate(:tag_group_permission, tag_group: tag_group1, group: group1)
45+
46+
group2 = Fabricate(:group)
47+
tag_group2 = Fabricate(:tag_group, tags: [tag2])
48+
Fabricate(:tag_group_permission, tag_group: tag_group2, group: group2)
49+
50+
Fabricate(:group_user, group: group1, user: user1)
51+
52+
expect(TagUser.notification_level_visible.pluck(:id)).to match_array([
53+
tag_user1.id, tag_user3.id, tag_user4.id
54+
])
55+
end
56+
57+
it "scopes to notification levels visible because user is staff" do
58+
group2 = Fabricate(:group)
59+
tag_group2 = Fabricate(:tag_group, tags: [tag2])
60+
Fabricate(:tag_group_permission, tag_group: tag_group2, group: group2)
61+
62+
staff_group = Group.find(Group::AUTO_GROUPS[:staff])
63+
Fabricate(:group_user, group: staff_group, user: user1)
64+
65+
expect(TagUser.notification_level_visible.length).to be(4)
66+
end
67+
68+
it "scopes to notification levels visible by specified notification level" do
69+
expect(TagUser.notification_level_visible([TagUser.notification_levels[:watching]]).length).to be(1)
70+
expect(TagUser.notification_level_visible(
71+
[TagUser.notification_levels[:watching],
72+
TagUser.notification_levels[:tracking]]
73+
).length).to be(2)
74+
end
75+
end
76+
2577
context "change" do
2678
it "watches or tracks on change" do
2779
user = Fabricate(:user)
@@ -264,6 +316,7 @@ def watching
264316
TagUser.create(user: user, tag: tag3, notification_level: TagUser.notification_levels[:watching_first_post])
265317
TagUser.create(user: user, tag: tag4, notification_level: TagUser.notification_levels[:muted])
266318
end
319+
267320
it "gets the tag_user notification levels for all tags the user is tracking and does not
268321
include tags the user is not tracking at all" do
269322
tag5 = Fabricate(:tag)
@@ -274,6 +327,14 @@ def watching
274327
expect(levels[tag4.name]).to eq(TagUser.notification_levels[:muted])
275328
expect(levels.key?(tag5.name)).to eq(false)
276329
end
330+
331+
it "does not show a tag is tracked if the user does not belong to the tag group with permissions" do
332+
group = Fabricate(:group)
333+
tag_group = Fabricate(:tag_group, tags: [tag2])
334+
Fabricate(:tag_group_permission, tag_group: tag_group, group: group)
335+
336+
expect(TagUser.notification_levels_for(user).keys).to match_array([tag1.name, tag3.name, tag4.name])
337+
end
277338
end
278339
end
279340
end

Diff for: spec/services/post_alerter_spec.rb

+85
Original file line numberDiff line numberDiff line change
@@ -1158,6 +1158,33 @@ def set_topic_notification_level(user, topic, level_name)
11581158
notification_data = JSON.parse(notification.data)
11591159
expect(notification_data["display_username"]).to eq(I18n.t("embed.replies", count: 2))
11601160
end
1161+
1162+
it "does not add notification if user does not belong to tag group with permissions" do
1163+
tag = Fabricate(:tag)
1164+
topic = Fabricate(:topic, tags: [tag])
1165+
post = Fabricate(:post, topic: topic)
1166+
tag_group = Fabricate(:tag_group, tags: [tag])
1167+
group = Fabricate(:group)
1168+
Fabricate(:tag_group_permission, tag_group: tag_group, group: group)
1169+
1170+
TagUser.change(user.id, tag.id, TagUser.notification_levels[:watching])
1171+
1172+
expect { PostAlerter.post_created(post) }.not_to change { Notification.count }
1173+
end
1174+
1175+
it "adds notification if user belongs to tag group with permissions" do
1176+
tag = Fabricate(:tag)
1177+
topic = Fabricate(:topic, tags: [tag])
1178+
post = Fabricate(:post, topic: topic)
1179+
tag_group = Fabricate(:tag_group, tags: [tag])
1180+
group = Fabricate(:group)
1181+
Fabricate(:group_user, group: group, user: user)
1182+
Fabricate(:tag_group_permission, tag_group: tag_group, group: group)
1183+
1184+
TagUser.change(user.id, tag.id, TagUser.notification_levels[:watching])
1185+
1186+
expect { PostAlerter.post_created(post) }.to change { Notification.count }.by(1)
1187+
end
11611188
end
11621189

11631190
context "on change" do
@@ -1224,6 +1251,64 @@ def set_topic_notification_level(user, topic, level_name)
12241251
expect(Notification.where(user_id: admin.id).count).to eq(1)
12251252
end
12261253
end
1254+
1255+
context "with tag groups" do
1256+
fab!(:tag) { Fabricate(:tag) }
1257+
fab!(:group) { Fabricate(:group) }
1258+
fab!(:user) { Fabricate(:user) }
1259+
fab!(:topic) { Fabricate(:topic, tags: [tag]) }
1260+
fab!(:post) { Fabricate(:post, topic: topic) }
1261+
1262+
shared_examples "tag user with notification level" do |notification_level, notification_type|
1263+
it "notifies a user who is watching a tag that does not belong to a tag group" do
1264+
TagUser.change(user.id, tag.id, TagUser.notification_levels[notification_level])
1265+
PostAlerter.post_created(post)
1266+
expect(user.notifications.where(notification_type: Notification.types[notification_type]).count).to eq(1)
1267+
end
1268+
1269+
it "does not notify a user watching a tag with tag group permissions that he does not belong to" do
1270+
tag_group = Fabricate(:tag_group, tags: [tag])
1271+
Fabricate(:tag_group_permission, tag_group: tag_group, group: group)
1272+
1273+
TagUser.change(user.id, tag.id, TagUser.notification_levels[notification_level])
1274+
1275+
PostAlerter.post_created(post)
1276+
1277+
expect(user.notifications.where(notification_type: Notification.types[notification_type]).count).to eq(0)
1278+
end
1279+
1280+
it "notifies a user watching a tag with tag group permissions that he belongs to" do
1281+
Fabricate(:group_user, group: group, user: user)
1282+
1283+
TagUser.change(user.id, tag.id, TagUser.notification_levels[notification_level])
1284+
1285+
PostAlerter.post_created(post)
1286+
1287+
expect(user.notifications.where(notification_type: Notification.types[notification_type]).count).to eq(1)
1288+
end
1289+
1290+
it "notifies a staff watching a tag with tag group permissions that he does not belong to" do
1291+
tag_group = Fabricate(:tag_group, tags: [tag])
1292+
Fabricate(:tag_group_permission, tag_group: tag_group, group: group)
1293+
staff_group = Group.find(Group::AUTO_GROUPS[:staff])
1294+
Fabricate(:group_user, group: staff_group, user: user)
1295+
1296+
TagUser.change(user.id, tag.id, TagUser.notification_levels[notification_level])
1297+
1298+
PostAlerter.post_created(post)
1299+
1300+
expect(user.notifications.where(notification_type: Notification.types[notification_type]).count).to eq(1)
1301+
end
1302+
end
1303+
1304+
context "with :watching notification level" do
1305+
include_examples "tag user with notification level", :watching, :posted
1306+
end
1307+
1308+
context "with :watching_first_post notification level" do
1309+
include_examples "tag user with notification level", :watching_first_post, :watching_first_post
1310+
end
1311+
end
12271312
end
12281313

12291314
describe '#extract_linked_users' do

0 commit comments

Comments
 (0)