FIX: users shouldn't get notifications about mentions from system users

(unless the mentioned users are participants in a PM with the system user)

app/services/post_alerter.rb

@@ -38,6 +38,10 @@ class PostAlerter
     allowed_group_users(post)
   end
 
+  def only_allowed_users(users, post)
+    users.select { |u| allowed_users(post).include?(u) || allowed_group_users(post).include?(u) }
+  end
+
   def notify_about_reply?(post)
     post.post_type == Post.types[:regular] || post.post_type == Post.types[:whisper]
   end
@@ -50,17 +54,20 @@ class PostAlerter
 
     if mentioned_groups || mentioned_users
       mentioned_opts = {}
+      editor = post.last_editor
+
       if post.last_editor_id != post.user_id
         # Mention comes from an edit by someone else, so notification should say who added the mention.
-        editor = post.last_editor
         mentioned_opts = { user_id: editor.id, original_username: editor.username, display_username: editor.username }
       end
 
       expand_group_mentions(mentioned_groups, post) do |group, users|
+        users = only_allowed_users(users, post) if editor.id < 0
         notified += notify_users(users - notified, :group_mentioned, post, mentioned_opts.merge(group: group))
       end
 
       if mentioned_users
+        mentioned_users = only_allowed_users(mentioned_users, post) if editor.id < 0
         notified += notify_users(mentioned_users - notified, :mentioned, post, mentioned_opts)
       end
     end

spec/services/post_alerter_spec.rb

@@ -412,6 +412,14 @@ describe PostAlerter do
 
     let(:alice) { Fabricate(:user, username: 'alice') }
     let(:bob) { Fabricate(:user, username: 'bob') }
+    let(:carol) { Fabricate(:admin, username: 'carol') }
+    let(:dave) { Fabricate(:user, username: 'dave') }
+    let(:eve) { Fabricate(:user, username: 'eve') }
+    let(:group) { Fabricate(:group, name: 'group', mentionable_level: Group::ALIAS_LEVELS[:everyone]) }
+
+    before do
+      group.bulk_add([alice.id, carol.id])
+    end
 
     def create_post_with_alerts(args = {})
       post = Fabricate(:post, args)
@@ -463,6 +471,21 @@ describe PostAlerter do
               args = { user: bob, topic: pm_topic, raw: 'Hello @alice' }
               expect { create_post_with_alerts(args) }.to add_notification(alice, :mentioned)
             end
+
+            it "notifies about @username mentions by non-human users" do
+              args = { user: Discourse.system_user, topic: pm_topic, raw: 'Hello @alice' }
+              expect { create_post_with_alerts(args) }.to add_notification(alice, :mentioned)
+            end
+
+            it "notifies about @group mention" do
+              args = { user: bob, topic: pm_topic, raw: 'Hello @group' }
+              expect { create_post_with_alerts(args) }.to add_notification(alice, :group_mentioned)
+            end
+
+            it "notifies about @group mentions by non-human users" do
+              args = { user: Discourse.system_user, topic: pm_topic, raw: 'Hello @group' }
+              expect { create_post_with_alerts(args) }.to add_notification(alice, :group_mentioned)
+            end
           end
         end
 
@@ -475,21 +498,44 @@ describe PostAlerter do
             args = { user: bob, topic: pm_topic, raw: 'Hello @alice' }
             expect { create_post_with_alerts(args) }.to_not add_notification(alice, :mentioned)
           end
+
+          it "does not notify about @group mention" do
+            args = { user: bob, topic: pm_topic, raw: 'Hello @group' }
+            expect { create_post_with_alerts(args) }.to_not add_notification(alice, :group_mentioned)
+          end
         end
       end
 
       context "when mentioned user is not part of conversation" do
         it "notifies about @username mention when mentioned user is allowed to see message" do
-          carol = Fabricate(:admin, username: 'carol')
           args = { user: bob, topic: pm_topic, raw: 'Hello @carol' }
           expect { create_post_with_alerts(args) }.to add_notification(carol, :mentioned)
         end
 
+        it "does not notify about @username mention by non-human user even though mentioned user is allowed to see message" do
+          args = { user: Discourse.system_user, topic: pm_topic, raw: 'Hello @carol' }
+          expect { create_post_with_alerts(args) }.to_not add_notification(carol, :mentioned)
+        end
+
         it "does not notify about @username mention when mentioned user is not allowed to see message" do
-          dave = Fabricate(:user, username: 'dave')
           args = { user: bob, topic: pm_topic, raw: 'Hello @dave' }
           expect { create_post_with_alerts(args) }.to_not add_notification(dave, :mentioned)
         end
+
+        it "notifies about @group mention when mentioned user is allowed to see message" do
+          args = { user: bob, topic: pm_topic, raw: 'Hello @group' }
+          expect { create_post_with_alerts(args) }.to add_notification(carol, :group_mentioned)
+        end
+
+        it "does not notify about @group mention by non-human user even though mentioned user is allowed to see message" do
+          args = { user: Discourse.system_user, topic: pm_topic, raw: 'Hello @group' }
+          expect { create_post_with_alerts(args) }.to_not add_notification(carol, :group_mentioned)
+        end
+
+        it "does not notify about @group mention when mentioned user is not allowed to see message" do
+          args = { user: bob, topic: pm_topic, raw: 'Hello @group' }
+          expect { create_post_with_alerts(args) }.to_not add_notification(dave, :group_mentioned)
+        end
       end
     end
 
@@ -497,7 +543,8 @@ describe PostAlerter do
       let(:pm_topic) do
         Fabricate(:private_message_topic, user: alice, topic_allowed_users: [
           Fabricate.build(:topic_allowed_user, user: alice),
-          Fabricate.build(:topic_allowed_user, user: bob)
+          Fabricate.build(:topic_allowed_user, user: bob),
+          Fabricate.build(:topic_allowed_user, user: eve)
         ])
       end
       let(:first_post) { Fabricate(:post, topic: pm_topic, user: pm_topic.user) }
@@ -506,14 +553,20 @@ describe PostAlerter do
     end
 
     context "group message" do
-      let(:group) { Fabricate(:group) }
+      let(:some_group) { Fabricate(:group, name: 'some_group') }
       let(:pm_topic) do
         Fabricate(:private_message_topic, user: alice, topic_allowed_groups: [
-          Fabricate.build(:topic_allowed_group, group: group)
+          Fabricate.build(:topic_allowed_group, group: some_group)
+        ], topic_allowed_users: [
+          Fabricate.build(:topic_allowed_user, user: eve)
         ])
       end
       let(:first_post) { Fabricate(:post, topic: pm_topic, user: pm_topic.user) }
 
+      before do
+        some_group.add(alice)
+      end
+
       include_context "message"
     end
   end