Merge branch 'dont-notify-users-without-project-access' into 'master'

Don't notify users without access to the project when they are (accidentally) mentioned in a note.

Addresses #2366.

See merge request !1216

3 files changed, 18 insertions, 5 deletions

diff --git a/CHANGELOG b/CHANGELOG
index bbf44ca14c9..d7d1eb7ab41 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -25,6 +25,7 @@ v 8.0.0 (unreleased)
   - Bring more UI consistency in way how projects, snippets and groups lists are rendered
   - Make all profiles public
   - Fixed login failure when extern_uid changes (Joel Koglin)
+  - Don't notify users without access to the project when they are (accidentally) mentioned in a note.
 
 v 7.14.1
   - Improve abuse reports management from admin area
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 3735a136365..e294b23bc23 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -107,12 +107,17 @@ class NotificationService
 
     recipients = []
 
+    mentioned_users = note.mentioned_users
+    mentioned_users.select! do |user|
+      user.can?(:read_project, note.project)
+    end
+
     # Add all users participating in the thread (author, assignee, comment authors)
     participants = 
       if target.respond_to?(:participants)
         target.participants(note.author)
       else
-        note.mentioned_users
+        mentioned_users
       end
     recipients = recipients.concat(participants)
 
@@ -120,8 +125,8 @@ class NotificationService
     recipients = add_project_watchers(recipients, note.project)
 
     # Reject users with Mention notification level, except those mentioned in _this_ note.
-    recipients = reject_mention_users(recipients - note.mentioned_users, note.project)
-    recipients = recipients + note.mentioned_users
+    recipients = reject_mention_users(recipients - mentioned_users, note.project)
+    recipients = recipients + mentioned_users
 
     recipients = reject_muted_users(recipients, note.project)
 
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 9da6c9dc949..8865335d0d1 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -31,13 +31,16 @@ describe NotificationService do
 
   describe 'Notes' do
     context 'issue note' do
-      let(:project) { create(:empty_project, :public) }
+      let(:project) { create(:empty_project, :private) }
       let(:issue) { create(:issue, project: project, assignee: create(:user)) }
       let(:mentioned_issue) { create(:issue, assignee: issue.assignee) }
-      let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@mention referenced') }
+      let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@mention referenced, @outsider also') }
 
       before do
         build_team(note.project)
+        project.team << [issue.author, :master]
+        project.team << [issue.assignee, :master]
+        project.team << [note.author, :master]
       end
 
       describe :new_note do
@@ -53,6 +56,7 @@ describe NotificationService do
           should_not_email(@u_participating.id)
           should_not_email(@u_disabled.id)
           should_not_email(@unsubscriber.id)
+          should_not_email(@u_outsider_mentioned)
 
           notification.new_note(note)
         end
@@ -444,12 +448,15 @@ describe NotificationService do
     @u_mentioned = create(:user, username: 'mention', notification_level: Notification::N_MENTION)
     @u_committer = create(:user, username: 'committer')
     @u_not_mentioned = create(:user, username: 'regular', notification_level: Notification::N_PARTICIPATING)
+    @u_outsider_mentioned = create(:user, username: 'outsider')
 
     project.team << [@u_watcher, :master]
     project.team << [@u_participating, :master]
+    project.team << [@u_participant_mentioned, :master]
     project.team << [@u_disabled, :master]
     project.team << [@u_mentioned, :master]
     project.team << [@u_committer, :master]
+    project.team << [@u_not_mentioned, :master]
   end
 
   def add_users_with_subscription(project, issuable)