Merge branch '300865-default-confidentiality-of-replies' into 'master'

Default confidentiality of replies & observe permissions

See merge request gitlab-org/gitlab!54122

app/models/note.rb

@@ -319,6 +319,7 @@ class Note < ApplicationRecord
 
   def noteable_assignee_or_author?(user)
     return false unless user
+    return false unless noteable.respond_to?(:author_id)
     return noteable.assignee_or_author?(user) if [MergeRequest, Issue].include?(noteable.class)
 
     noteable.author_id == user.id

app/policies/note_policy.rb

@@ -57,6 +57,10 @@ class NotePolicy < BasePolicy
     enable :resolve_note
   end
 
+  rule { can_read_confidential }.policy do
+    enable :mark_note_as_confidential
+  end
+
   rule { confidential & ~can_read_confidential }.policy do
     prevent :read_note
     prevent :admin_note

app/services/notes/build_service.rb

@@ -3,32 +3,36 @@
 module Notes
   class BuildService < ::BaseService
     def execute
-      should_resolve = false
       in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id)
+      discussion = nil
 
       if in_reply_to_discussion_id.present?
         discussion = find_discussion(in_reply_to_discussion_id)
 
-        unless discussion && can?(current_user, :create_note, discussion.noteable)
-          note = Note.new
-          note.errors.add(:base, _('Discussion to reply to cannot be found'))
-          return note
-        end
+        return discussion_not_found unless discussion && can?(current_user, :create_note, discussion.noteable)
 
         discussion = discussion.convert_to_discussion! if discussion.can_convert_to_discussion?
 
         params.merge!(discussion.reply_attributes)
-        should_resolve = discussion.resolved?
       end
 
+      new_note(params, discussion)
+    end
+
+    private
+
+    def new_note(params, discussion)
       note = Note.new(params)
       note.project = project
       note.author = current_user
 
-      if should_resolve
-        note.resolve_without_save(current_user)
-      end
+      parent_confidential = discussion&.confidential?
+      can_set_confidential = can?(current_user, :mark_note_as_confidential, note)
 
+      return discussion_not_found if parent_confidential && !can_set_confidential
+
+      note.confidential = (parent_confidential.nil? && can_set_confidential ? params.delete(:confidential) : parent_confidential)
+      note.resolve_without_save(current_user) if discussion&.resolved?
       note
     end
 
@@ -39,5 +43,11 @@ module Notes
         Note.find_discussion(discussion_id)
       end
     end
+
+    def discussion_not_found
+      note = Note.new
+      note.errors.add(:base, _('Discussion to reply to cannot be found'))
+      note
+    end
   end
 end

changelogs/unreleased/300865-default-confidentiality-of-replies.yml

+---
+title: Default confidentiality of replies
+merge_request: 54122
+author: Lee Tickett @leetickett
+type: added

spec/services/notes/build_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Notes::BuildService do
+  include AdminModeHelper
+
   let(:note) { create(:discussion_note_on_issue) }
   let(:project) { note.project }
   let(:author) { note.author }
@@ -147,6 +149,123 @@ RSpec.describe Notes::BuildService do
       end
     end
 
+    context 'confidential comments' do
+      before do
+        project.add_reporter(author)
+      end
+
+      context 'when replying to a confidential comment' do
+        let(:note) { create(:note_on_issue, confidential: true) }
+
+        context 'when the user can read confidential comments' do
+          subject do
+            described_class.new(
+              project,
+              author,
+              note: 'Test',
+              in_reply_to_discussion_id: note.discussion_id,
+              confidential: false
+            ).execute
+          end
+
+          it '`confidential` param is ignored and set to `true`' do
+            expect(subject.confidential).to be_truthy
+          end
+        end
+
+        context 'when the user cannot read confidential comments' do
+          let(:another_user) { create(:user) }
+
+          subject do
+            described_class.new(
+              project,
+              another_user,
+              note: 'Test',
+              in_reply_to_discussion_id: note.discussion_id,
+              confidential: false
+            ).execute
+          end
+
+          it 'returns `Discussion to reply to cannot be found` error' do
+            expect(subject.errors.first).to include("Discussion to reply to cannot be found")
+          end
+        end
+      end
+
+      context 'when replying to a public comment' do
+        let(:note) { create(:note_on_issue, confidential: false) }
+
+        subject do
+          described_class.new(
+            project,
+            author,
+            note: 'Test',
+            in_reply_to_discussion_id: note.discussion_id,
+            confidential: true
+          ).execute
+        end
+
+        it '`confidential` param is ignored and set to `false`' do
+          expect(subject.confidential).to be_falsey
+        end
+      end
+
+      context 'when creating a new comment' do
+        context 'when the `confidential` note flag is set to `true`' do
+          context 'when the user is allowed (reporter)' do
+            subject { described_class.new(project, author, note: 'Test', noteable: merge_request, confidential: true).execute }
+
+            it 'note `confidential` flag is set to `true`' do
+              expect(subject.confidential).to be_truthy
+            end
+          end
+
+          context 'when the user is allowed (issuable author)' do
+            let(:another_user) { create(:user) }
+            let(:issue) { create(:issue, author: another_user) }
+
+            subject { described_class.new(project, another_user, note: 'Test', noteable: issue, confidential: true).execute }
+
+            it 'note `confidential` flag is set to `true`' do
+              expect(subject.confidential).to be_truthy
+            end
+          end
+
+          context 'when the user is allowed (admin)' do
+            before do
+              enable_admin_mode!(another_user)
+            end
+
+            let(:another_user) { create(:admin) }
+
+            subject { described_class.new(project, another_user, note: 'Test', noteable: merge_request, confidential: true).execute }
+
+            it 'note `confidential` flag is set to `true`' do
+              expect(subject.confidential).to be_truthy
+            end
+          end
+
+          context 'when the user is not allowed' do
+            let(:another_user) { create(:user) }
+
+            subject { described_class.new(project, another_user, note: 'Test', noteable: merge_request, confidential: true).execute }
+
+            it 'note `confidential` flag is set to `false`' do
+              expect(subject.confidential).to be_falsey
+            end
+          end
+        end
+
+        context 'when the `confidential` note flag is set to `false`' do
+          subject { described_class.new(project, author, note: 'Test', noteable: merge_request, confidential: false).execute }
+
+          it 'note `confidential` flag is set to `false`' do
+            expect(subject.confidential).to be_falsey
+          end
+        end
+      end
+    end
+
     it 'builds a note without saving it' do
       new_note = described_class.new(project,
                                     author,