Merge branch 'unauthorized-participants' into 'master'

Don't send notifications to mentioned users that don't have access to the project in question.

Fixes internal issue https://dev.gitlab.org/gitlab/gitlabhq/issues/2325.

See merge request !664

CHANGELOG

@@ -44,7 +44,7 @@ v 7.11.0 (unreleased)
   - Fix bug where avatar filenames were not actually deleted from the database during removal (Stan Hu)
   - Fix bug where Slack service channel was not saved in admin template settings. (Stan Hu)
   - Protect OmniAuth request phase against CSRF.
-  -
+  - Don't send notifications to mentioned users that don't have access to the project in question.
   -
   - Move snippets UI to fluid layout
   - Improve UI for sidebar. Increase separation between navigation and content

app/models/concerns/participable.rb

@@ -35,8 +35,8 @@ module Participable
     end
   end
 
-  def participants(current_user = self.author)
-    self.class.participant_attrs.flat_map do |attr|
+  def participants(current_user = self.author, project = self.project)
+    participants = self.class.participant_attrs.flat_map do |attr|
       meth = method(attr)
 
       value = 
@@ -46,20 +46,28 @@ module Participable
           meth.call
         end
 
-      participants_for(value, current_user)
+      participants_for(value, current_user, project)
     end.compact.uniq
+
+    if project
+      participants.select! do |user|
+        user.can?(:read_project, project)
+      end
+    end
+
+    participants
   end
 
   private
   
-  def participants_for(value, current_user = nil)
+  def participants_for(value, current_user = nil, project = nil)
     case value
     when User
       [value]
     when Enumerable, ActiveRecord::Relation
-      value.flat_map { |v| participants_for(v, current_user) }
+      value.flat_map { |v| participants_for(v, current_user, project) }
     when Participable
-      value.participants(current_user)
+      value.participants(current_user, project)
     end
   end
 end

app/services/notification_service.rb

@@ -91,10 +91,14 @@ class NotificationService
   #  * project team members with notification level higher then Participating
   #
   def merge_mr(merge_request, current_user)
-    recipients = reject_muted_users([merge_request.author, merge_request.assignee], merge_request.target_project)
+    recipients = [merge_request.author, merge_request.assignee]
+
+    recipients = add_project_watchers(recipients, merge_request.target_project)
+    recipients = reject_muted_users(recipients, merge_request.target_project)
+
     recipients = add_subscribed_users(recipients, merge_request)
     recipients = reject_unsubscribed_users(recipients, merge_request)
-    recipients = recipients.concat(project_watchers(merge_request.target_project)).uniq
+
     recipients.delete(current_user)
 
     recipients.each do |recipient|
@@ -137,20 +141,17 @@ class NotificationService
     recipients = recipients.concat(participants)
 
     # Merge project watchers
-    recipients = recipients.concat(project_watchers(note.project)).compact.uniq
+    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
 
-    # Reject mutes users
     recipients = reject_muted_users(recipients, note.project)
 
     recipients = add_subscribed_users(recipients, note.noteable)
-    
     recipients = reject_unsubscribed_users(recipients, note.noteable)
 
-    # Reject author
     recipients.delete(note.author)
 
     # build notify method like 'note_commit_email'
@@ -287,6 +288,10 @@ class NotificationService
     users
   end
 
+  def add_project_watchers(recipients, project)
+    recipients.concat(project_watchers(project)).compact.uniq
+  end
+
   # Remove users with disabled notifications from array
   # Also remove duplications and nil recipients
   def reject_muted_users(users, project = nil)
@@ -403,11 +408,13 @@ class NotificationService
         [target.author, target.assignee]
       end
 
-    recipients = reject_muted_users(recipients, project)
+    recipients = add_project_watchers(recipients, project)
     recipients = reject_mention_users(recipients, project)
+    recipients = reject_muted_users(recipients, project)
+
     recipients = add_subscribed_users(recipients, target)
-    recipients = recipients.concat(project_watchers(project)).uniq
     recipients = reject_unsubscribed_users(recipients, target)
+
     recipients
   end
 

spec/services/issues/close_service_spec.rb

 require 'spec_helper'
 
 describe Issues::CloseService do
-  let(:project) { create(:empty_project) }
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
   let(:issue) { create(:issue, assignee: user2) }
+  let(:project) { issue.project }
 
   before do
     project.team << [user, :master]

spec/services/issues/update_service_spec.rb

 require 'spec_helper'
 
 describe Issues::UpdateService do
-  let(:project) { create(:empty_project) }
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
   let(:issue) { create(:issue) }
   let(:label) { create(:label) }
+  let(:project) { issue.project }
 
   before do
     project.team << [user, :master]

spec/services/notification_service_spec.rb

@@ -31,7 +31,8 @@ describe NotificationService do
 
   describe 'Notes' do
     context 'issue note' do
-      let(:issue) { create(:issue, assignee: create(:user)) }
+      let(:project) { create(:empty_project, :public) }
+      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') }
 
@@ -101,7 +102,8 @@ describe NotificationService do
     end
 
     context 'issue note mention' do
-      let(:issue) { create(:issue, assignee: create(:user)) }
+      let(:project) { create(:empty_project, :public) }
+      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: '@all mentioned') }
 
@@ -145,7 +147,8 @@ describe NotificationService do
     end
 
     context 'commit note' do
-      let(:note) { create(:note_on_commit) }
+      let(:project) { create(:project, :public) }
+      let(:note) { create(:note_on_commit, project: project) }
 
       before do
         build_team(note.project)
@@ -192,7 +195,8 @@ describe NotificationService do
   end
 
   describe 'Issues' do
-    let(:issue) { create :issue, assignee: create(:user), description: 'cc @participant' }
+    let(:project) { create(:empty_project, :public) }
+    let(:issue) { create :issue, project: project, assignee: create(:user), description: 'cc @participant' }
 
     before do
       build_team(issue.project)
@@ -295,7 +299,8 @@ describe NotificationService do
   end
 
   describe 'Merge Requests' do
-    let(:merge_request) { create :merge_request, assignee: create(:user) }
+    let(:project) { create(:project, :public) }
+    let(:merge_request) { create :merge_request, source_project: project, assignee: create(:user) }
 
     before do
       build_team(merge_request.target_project)