From 3536b17e97ca71576644db9ac59bcbb2eeb3de4b Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 23 Mar 2016 10:34:20 +0000 Subject: [PATCH] Merge branch 'notifications-for-subscribers-confidential-issue-labels' into 'master' Restrict notifications for confidential issues Closes #14468 /cc @rymai See merge request !3334 --- CHANGELOG | 1 + app/models/label.rb | 8 +- app/services/notification_service.rb | 12 ++- app/views/projects/labels/_label.html.haml | 2 +- spec/services/issues/update_service_spec.rb | 7 +- spec/services/notification_service_spec.rb | 88 +++++++++++++++++++++ 6 files changed, 111 insertions(+), 7 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index e1610434bf..5a7de74136 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.6.1 (unreleased) - Add option to reload the schema before restoring a database backup. !2807 + - Restrict notifications for confidential issues. !3334 v 8.6.0 - Add ability to move issue to another project diff --git a/app/models/label.rb b/app/models/label.rb index f7ffc0b7f3..500d5a3552 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 3bdf00a829..eff0d96f93 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 4927d239c1..0612863296 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 4ffe753fef..6b214a0d96 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 b5407397c1..0f2aa3ae73 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