From e636abeb0d92855cd97742a5d48d7018284106ee Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 14 Feb 2023 16:45:06 +1100 Subject: [PATCH] FIX: do not notify admins on suppressed categories (#20238) * FIX: do not notify admins on suppressed categories Avoid notifying admins on categories where they are not explicitly members in cases where SiteSetting.suppress_secured_categories_from_admin is enabled. This helps keep notification stream clean and avoids admins mistakenly being invited to discussions that should be suppressed --- app/services/post_alerter.rb | 3 +- lib/guardian/post_guardian.rb | 12 ++++++ spec/lib/guardian_spec.rb | 36 +++++++++++++++++ spec/services/post_action_notifier_spec.rb | 21 +++++----- spec/services/post_alerter_spec.rb | 47 ++++++++++++---------- 5 files changed, 84 insertions(+), 35 deletions(-) diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index db6906157d2..b6447978d7d 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -466,8 +466,7 @@ class PostAlerter return end - # Make sure the user can see the post - return unless Guardian.new(user).can_see?(post) + return if !Guardian.new(user).can_receive_post_notifications?(post) return if user.staged? && topic.category&.mailinglist_mirror? diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index 1ecd956f1bb..8946b1bddfd 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -249,6 +249,18 @@ module PostGuardian !post_action.post&.topic&.archived? end + def can_receive_post_notifications?(post) + return false if !authenticated? + + if is_admin? && SiteSetting.suppress_secured_categories_from_admin + topic = post.topic + if !topic.private_message? && topic.category.read_restricted + return secure_category_ids.include?(topic.category_id) + end + end + can_see_post?(post) + end + def can_see_post?(post) return false if post.blank? return true if is_admin? diff --git a/spec/lib/guardian_spec.rb b/spec/lib/guardian_spec.rb index 8bdfe06e42b..52c2885d7e0 100644 --- a/spec/lib/guardian_spec.rb +++ b/spec/lib/guardian_spec.rb @@ -820,6 +820,42 @@ RSpec.describe Guardian do end end + describe "can_receive_post_notifications?" do + it "returns false with a nil object" do + expect(Guardian.new.can_receive_post_notifications?(nil)).to be_falsey + expect(Guardian.new(user).can_receive_post_notifications?(nil)).to be_falsey + end + + it "does not allow anonymous to be notified" do + expect(Guardian.new.can_receive_post_notifications?(post)).to be_falsey + end + + it "allows public categories" do + expect(Guardian.new(trust_level_0).can_receive_post_notifications?(post)).to be_truthy + expect(Guardian.new(admin).can_receive_post_notifications?(post)).to be_truthy + end + + it "diallows secure categories with no access" do + secure_category = Fabricate(:category, read_restricted: true) + post.topic.update!(category_id: secure_category.id) + + expect(Guardian.new(trust_level_0).can_receive_post_notifications?(post)).to be_falsey + expect(Guardian.new(admin).can_receive_post_notifications?(post)).to be_truthy + + SiteSetting.suppress_secured_categories_from_admin = true + + expect(Guardian.new(admin).can_receive_post_notifications?(post)).to be_falsey + + secure_category.set_permissions(group => :write) + secure_category.save! + + group.add(admin) + group.save! + + expect(Guardian.new(admin).can_receive_post_notifications?(post)).to be_truthy + end + end + describe "can_see?" do it "returns false with a nil object" do expect(Guardian.new.can_see?(nil)).to be_falsey diff --git a/spec/services/post_action_notifier_spec.rb b/spec/services/post_action_notifier_spec.rb index 8ac91ea5a68..087f62416da 100644 --- a/spec/services/post_action_notifier_spec.rb +++ b/spec/services/post_action_notifier_spec.rb @@ -202,25 +202,24 @@ RSpec.describe PostActionNotifier do end context "with private message" do + fab!(:private_message) do + Fabricate(:topic, archetype: Archetype.private_message, category_id: nil) + end fab!(:user) { Fabricate(:user) } - fab!(:mention_post) { Fabricate(:post, user: user, raw: "Hello @eviltrout") } - let(:topic) do - topic = mention_post.topic - topic.update_columns archetype: Archetype.private_message, category_id: nil - topic + fab!(:mention_post) do + Fabricate(:post, topic: private_message, user: user, raw: "Hello @eviltrout") end it "won't notify someone who can't see the post" do - expect { - Guardian.any_instance.expects(:can_see?).with(instance_of(Post)).returns(false) - mention_post - PostAlerter.post_created(mention_post) - }.not_to change(evil_trout.notifications, :count) + expect { PostAlerter.post_created(mention_post) }.not_to change( + evil_trout.notifications, + :count, + ) end it "creates like notifications" do other_user = Fabricate(:user) - topic.allowed_users << user << other_user + private_message.allowed_users << user << other_user expect { PostActionCreator.like(other_user, mention_post) }.to change( user.notifications, :count, diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index fc4bf047952..8131cc46e3c 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -45,6 +45,15 @@ RSpec.describe PostAlerter do fab!(:user) { Fabricate(:user) } fab!(:tl2_user) { Fabricate(:user, trust_level: TrustLevel[2]) } + fab!(:private_category) do + Fabricate( + :private_category, + group: group, + email_in: "test@test.com", + email_in_allow_strangers: true, + ) + end + def create_post_with_alerts(args = {}) post = Fabricate(:post, args) PostAlerter.post_created(post) @@ -609,22 +618,13 @@ RSpec.describe PostAlerter do group_member = Fabricate(:user) group.add(group_member) - private_category = - Fabricate( - :private_category, - group: group, - email_in: "test@test.com", - email_in_allow_strangers: true, - ) - staged_user_post = create_post(user: staged_user, category: private_category) - linking = - create_post( - user: group_member, - category: private_category, - raw: "my magic topic\n##{Discourse.base_url}#{staged_user_post.url}", - ) + create_post( + user: group_member, + category: private_category, + raw: "my magic topic\n##{Discourse.base_url}#{staged_user_post.url}", + ) staged_user.reload expect( @@ -1392,6 +1392,17 @@ RSpec.describe PostAlerter do ) end + it "does not notify admins when suppress_secured_categories_from_admin is enabled" do + SiteSetting.suppress_secured_categories_from_admin = true + + topic = Fabricate(:topic, category: private_category) + post = Fabricate(:post, raw: "hello @#{admin.username} how are you today?", topic: topic) + + PostAlerter.post_created(post) + + expect(admin.notifications.count).to eq(0) + end + it "doesn't notify regular user about whispered reply" do _post = Fabricate(:post, user: user, topic: topic) @@ -1639,14 +1650,6 @@ RSpec.describe PostAlerter do group.add(group_member) group.add(staged_member) - private_category = - Fabricate( - :private_category, - group: group, - email_in: "test@test.com", - email_in_allow_strangers: false, - ) - level = CategoryUser.notification_levels[:watching] CategoryUser.set_notification_level_for_category(group_member, level, private_category.id) CategoryUser.set_notification_level_for_category(staged_member, level, private_category.id)