Merge branch 'notifications-for-subscribers-confidential-issue-labels' into 'master'

Restrict notifications for confidential issues

Closes #14468 

/cc @rymai 

See merge request !3334

5 files changed, 110 insertions, 7 deletions

diff --git a/app/models/label.rb b/app/models/label.rb
index f7ffc0b7f36..500d5a35521 100644
--- a/app/models/label.rb
+++ b/app/models/label.rb
@@ -97,12 +97,12 @@ class Label < ActiveRecord::Base
     end
   end
 
-  def open_issues_count
-    issues.opened.count
+  def open_issues_count(user = nil)
+    issues.visible_to_user(user).opened.count
   end
 
-  def closed_issues_count
-    issues.closed.count
+  def closed_issues_count(user = nil)
+    issues.visible_to_user(user).closed.count
   end
 
   def open_merge_requests_count
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 3bdf00a8291..eff0d96f93d 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -162,6 +162,7 @@ class NotificationService
 
     recipients = add_subscribed_users(recipients, note.noteable)
     recipients = reject_unsubscribed_users(recipients, note.noteable)
+    recipients = reject_users_without_access(recipients, note.noteable)
 
     recipients.delete(note.author)
     recipients = recipients.uniq
@@ -376,6 +377,14 @@ class NotificationService
     end
   end
 
+  def reject_users_without_access(recipients, target)
+    return recipients unless target.is_a?(Issue)
+
+    recipients.select do |user|
+      user.can?(:read_issue, target)
+    end
+  end
+
   def add_subscribed_users(recipients, target)
     return recipients unless target.respond_to? :subscribers
 
@@ -464,15 +473,16 @@ class NotificationService
     end
 
     recipients = reject_unsubscribed_users(recipients, target)
+    recipients = reject_users_without_access(recipients, target)
 
     recipients.delete(current_user)
-
     recipients.uniq
   end
 
   def build_relabeled_recipients(target, current_user, labels:)
     recipients = add_labels_subscribers([], target, labels: labels)
     recipients = reject_unsubscribed_users(recipients, target)
+    recipients = reject_users_without_access(recipients, target)
     recipients.delete(current_user)
     recipients.uniq
   end
diff --git a/app/views/projects/labels/_label.html.haml b/app/views/projects/labels/_label.html.haml
index 4927d239c1e..0612863296a 100644
--- a/app/views/projects/labels/_label.html.haml
+++ b/app/views/projects/labels/_label.html.haml
@@ -8,7 +8,7 @@
 
     %strong.append-right-20
       = link_to_label(label) do
-        = pluralize label.open_issues_count, 'open issue'
+        = pluralize label.open_issues_count(current_user), 'open issue'
 
     - if current_user
       .label-subscription{data: {url: toggle_subscription_namespace_project_label_path(@project.namespace, @project, label)}}
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index 4ffe753fef5..6b214a0d96b 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -151,7 +151,12 @@ describe Issues::UpdateService, services: true do
 
     context 'when the issue is relabeled' do
       let!(:non_subscriber) { create(:user) }
-      let!(:subscriber) { create(:user).tap { |u| label.toggle_subscription(u) } }
+      let!(:subscriber) do
+        create(:user).tap do |u|
+          label.toggle_subscription(u)
+          project.team << [u, :developer]
+        end
+      end
 
       it 'sends notifications for subscribers of newly added labels' do
         opts = { label_ids: [label.id] }
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index b5407397c1d..0f2aa3ae73c 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -111,6 +111,33 @@ describe NotificationService, services: true do
       end
     end
 
+    context 'confidential issue note' do
+      let(:project) { create(:empty_project, :public) }
+      let(:author) { create(:user) }
+      let(:assignee) { create(:user) }
+      let(:non_member) { create(:user) }
+      let(:member) { create(:user) }
+      let(:admin) { create(:admin) }
+      let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) }
+      let(:note) { create(:note_on_issue, noteable: confidential_issue, project: project, note: "#{author.to_reference} #{assignee.to_reference} #{non_member.to_reference} #{member.to_reference} #{admin.to_reference}") }
+
+      it 'filters out users that can not read the issue' do
+        project.team << [member, :developer]
+
+        expect(SentNotification).to receive(:record).with(confidential_issue, any_args).exactly(4).times
+
+        ActionMailer::Base.deliveries.clear
+
+        notification.new_note(note)
+
+        should_not_email(non_member)
+        should_email(author)
+        should_email(assignee)
+        should_email(member)
+        should_email(admin)
+      end
+    end
+
     context 'issue note mention' do
       let(:project) { create(:empty_project, :public) }
       let(:issue) { create(:issue, project: project, assignee: create(:user)) }
@@ -233,6 +260,36 @@ describe NotificationService, services: true do
 
         should_email(subscriber)
       end
+
+      context 'confidential issues' do
+        let(:author) { create(:user) }
+        let(:assignee) { create(:user) }
+        let(:non_member) { create(:user) }
+        let(:member) { create(:user) }
+        let(:admin) { create(:admin) }
+        let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignee: assignee) }
+
+        it "emails subscribers of the issue's labels that can read the issue" do
+          project.team << [member, :developer]
+
+          label = create(:label, issues: [confidential_issue])
+          label.toggle_subscription(non_member)
+          label.toggle_subscription(author)
+          label.toggle_subscription(assignee)
+          label.toggle_subscription(member)
+          label.toggle_subscription(admin)
+
+          ActionMailer::Base.deliveries.clear
+
+          notification.new_issue(confidential_issue, @u_disabled)
+
+          should_not_email(non_member)
+          should_not_email(author)
+          should_email(assignee)
+          should_email(member)
+          should_email(admin)
+        end
+      end
     end
 
     describe :reassigned_issue do
@@ -332,6 +389,37 @@ describe NotificationService, services: true do
         should_not_email(subscriber_to_label)
         should_email(subscriber_to_label2)
       end
+
+      context 'confidential issues' do
+        let(:author) { create(:user) }
+        let(:assignee) { create(:user) }
+        let(:non_member) { create(:user) }
+        let(:member) { create(:user) }
+        let(:admin) { create(:admin) }
+        let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignee: assignee) }
+        let!(:label_1) { create(:label, issues: [confidential_issue]) }
+        let!(:label_2) { create(:label) }
+
+        it "emails subscribers of the issue's labels that can read the issue" do
+          project.team << [member, :developer]
+
+          label_2.toggle_subscription(non_member)
+          label_2.toggle_subscription(author)
+          label_2.toggle_subscription(assignee)
+          label_2.toggle_subscription(member)
+          label_2.toggle_subscription(admin)
+
+          ActionMailer::Base.deliveries.clear
+
+          notification.relabeled_issue(confidential_issue, [label_2], @u_disabled)
+
+          should_not_email(non_member)
+          should_email(author)
+          should_email(assignee)
+          should_email(member)
+          should_email(admin)
+        end
+      end
     end
 
     describe :close_issue do