Merge branch 'recipients-confidential-notes' into 'master'

Check notes permissions (confidential notes) when sending notifications

See merge request gitlab-org/gitlab!27782

app/models/notification_recipient.rb

@@ -74,10 +74,12 @@ class NotificationRecipient
   end
 
   def unsubscribed?
-    return false unless @target
-    return false unless @target.respond_to?(:subscriptions)
+    subscribable_target = @target.is_a?(Note) ? @target.noteable : @target
 
-    subscription = @target.subscriptions.find { |subscription| subscription.user_id == @user.id }
+    return false unless subscribable_target
+    return false unless subscribable_target.respond_to?(:subscriptions)
+
+    subscription = subscribable_target.subscriptions.find { |subscription| subscription.user_id == @user.id }
     subscription && !subscription.subscribed
   end
 

app/policies/note_policy.rb

@@ -19,7 +19,7 @@ class NotePolicy < BasePolicy
   condition(:confidential, scope: :subject) { @subject.confidential? }
 
   condition(:can_read_confidential) do
-    access_level >= Gitlab::Access::REPORTER || @subject.noteable_assignee_or_author?(@user)
+    access_level >= Gitlab::Access::REPORTER || @subject.noteable_assignee_or_author?(@user) || admin?
   end
 
   rule { ~editable }.prevent :admin_note

app/services/notification_recipients/builder/base.rb

@@ -23,6 +23,11 @@ module NotificationRecipients
         raise 'abstract'
       end
 
+      # override if needed
+      def recipients_target
+        target
+      end
+
       def project
         target.project
       end
@@ -59,7 +64,7 @@ module NotificationRecipients
           project: project,
           group: group,
           custom_action: custom_action,
-          target: target,
+          target: recipients_target,
           acting_user: acting_user
         )
       end

app/services/notification_recipients/builder/new_note.rb

@@ -12,6 +12,10 @@ module NotificationRecipients
         note.noteable
       end
 
+      def recipients_target
+        note
+      end
+
       # NOTE: may be nil, in the case of a PersonalSnippet
       #
       # (this is okay because NotificationRecipient is written

spec/policies/note_policy_spec.rb

@@ -263,6 +263,7 @@ describe NotePolicy do
         let(:non_member) { create(:user) }
         let(:author) { create(:user) }
         let(:assignee) { create(:user) }
+        let(:admin) { create(:admin) }
 
         before do
           project.add_reporter(reporter)
@@ -294,6 +295,10 @@ describe NotePolicy do
             expect(permissions(maintainer, confidential_note)).to be_allowed(:read_note, :admin_note, :resolve_note, :award_emoji)
           end
 
+          it 'allows admins to read all notes and admin them' do
+            expect(permissions(admin, confidential_note)).to be_allowed(:read_note, :admin_note, :resolve_note, :award_emoji)
+          end
+
           it 'allows noteable author to read and resolve all notes' do
             expect(permissions(author, confidential_note)).to be_allowed(:read_note, :resolve_note, :award_emoji)
             expect(permissions(author, confidential_note)).to be_disallowed(:admin_note)

spec/services/notification_recipients/builder/new_note_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe NotificationRecipients::Builder::NewNote do
+  describe '#notification_recipients' do
+    let_it_be(:group)   { create(:group, :public) }
+    let_it_be(:project) { create(:project, :public, group: group) }
+    let_it_be(:issue) { create(:issue, project: project) }
+
+    let_it_be(:other_user)              { create(:user) }
+    let_it_be(:participant)             { create(:user) }
+    let_it_be(:non_member_participant)  { create(:user) }
+    let_it_be(:group_watcher)           { create(:user) }
+    let_it_be(:project_watcher)         { create(:user) }
+    let_it_be(:guest_project_watcher)   { create(:user) }
+    let_it_be(:subscriber)              { create(:user) }
+    let_it_be(:unsubscribed_user)       { create(:user) }
+    let_it_be(:non_member_subscriber)   { create(:user) }
+
+    let_it_be(:notification_setting_project_w) { create(:notification_setting, source: project, user: project_watcher, level: 2) }
+    let_it_be(:notification_setting_guest_w) { create(:notification_setting, source: project, user: guest_project_watcher, level: 2) }
+    let_it_be(:notification_setting_group_w) { create(:notification_setting, source: group, user: group_watcher, level: 2) }
+    let_it_be(:subscriptions) do
+      [
+        create(:subscription, project: project, user: subscriber, subscribable: issue, subscribed: true),
+        create(:subscription, project: project, user: unsubscribed_user, subscribable: issue, subscribed: false),
+        create(:subscription, project: project, user: non_member_subscriber, subscribable: issue, subscribed: true)
+      ]
+    end
+
+    subject { described_class.new(note) }
+
+    before do
+      project.add_developer(participant)
+      project.add_developer(project_watcher)
+      project.add_guest(guest_project_watcher)
+      project.add_developer(subscriber)
+      group.add_developer(group_watcher)
+
+      expect(issue).to receive(:participants).and_return([participant, non_member_participant])
+    end
+
+    context 'for public notes' do
+      let_it_be(:note) { create(:note, noteable: issue, project: project) }
+
+      it 'adds all participants, watchers and subscribers' do
+        expect(subject.notification_recipients.map(&:user)).to contain_exactly(
+          participant, non_member_participant, project_watcher, group_watcher, guest_project_watcher, subscriber, non_member_subscriber
+        )
+      end
+    end
+
+    context 'for confidential notes' do
+      let_it_be(:note) { create(:note, :confidential, noteable: issue, project: project) }
+
+      it 'adds all participants, watchers and subscribers that are project memebrs' do
+        expect(subject.notification_recipients.map(&:user)).to contain_exactly(
+          participant, project_watcher, group_watcher, subscriber
+        )
+      end
+    end
+  end
+end