Merge branch 'jp-notes-validation' into 'master'

Add validation for confidentiality notes

See merge request gitlab-org/gitlab!83847

app/models/note.rb

@@ -35,6 +35,8 @@ class Note < ApplicationRecord
     contact: :read_crm_contact
   }.freeze
 
+  NON_DIFF_NOTE_TYPES = ['Note', 'DiscussionNote', nil].freeze
+
   # Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with notes.
   # See https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/10392/diffs#note_28719102
   alias_attribute :last_edited_by, :updated_by
@@ -97,6 +99,9 @@ class Note < ApplicationRecord
   validates :author, presence: true
   validates :discussion_id, presence: true, format: { with: /\A\h{40}\z/ }
 
+  validate :ensure_confidentiality_discussion_compliance
+  validate :ensure_noteable_can_have_confidential_note
+  validate :ensure_note_type_can_be_confidential
   validate :ensure_confidentiality_not_changed, on: :update
 
   validate unless: [:for_commit?, :importing?, :skip_project_check?] do |note|
@@ -143,7 +148,7 @@ class Note < ApplicationRecord
 
   scope :diff_notes, -> { where(type: %w(LegacyDiffNote DiffNote)) }
   scope :new_diff_notes, -> { where(type: 'DiffNote') }
-  scope :non_diff_notes, -> { where(type: ['Note', 'DiscussionNote', nil]) }
+  scope :non_diff_notes, -> { where(type: NON_DIFF_NOTE_TYPES) }
 
   scope :with_associations, -> do
     # FYI noteable cannot be loaded for LegacyDiffNote for commits
@@ -460,7 +465,7 @@ class Note < ApplicationRecord
   # and all its notes and if we don't care about the discussion's resolvability status.
   def discussion
     strong_memoize(:discussion) do
-      full_discussion = self.noteable.notes.find_discussion(self.discussion_id) if part_of_discussion?
+      full_discussion = self.noteable.notes.find_discussion(self.discussion_id) if self.noteable && part_of_discussion?
       full_discussion || to_discussion
     end
   end
@@ -727,6 +732,35 @@ class Note < ApplicationRecord
 
     errors.add(:confidential, _('can not be changed for existing notes'))
   end
+
+  def ensure_confidentiality_discussion_compliance
+    return if start_of_discussion?
+
+    if discussion.first_note.confidential? != confidential?
+      errors.add(:confidential, _('reply should have same confidentiality as top-level note'))
+    end
+
+  ensure
+    clear_memoization(:discussion)
+  end
+
+  def ensure_noteable_can_have_confidential_note
+    return unless confidential?
+    return if noteable_can_have_confidential_note?
+
+    errors.add(:confidential, _('can not be set for this resource'))
+  end
+
+  def ensure_note_type_can_be_confidential
+    return unless confidential?
+    return if NON_DIFF_NOTE_TYPES.include?(type)
+
+    errors.add(:confidential, _('can not be set for this type of note'))
+  end
+
+  def noteable_can_have_confidential_note?
+    for_issue?
+  end
 end
 
 Note.prepend_mod_with('Note')

ee/app/models/ee/note.rb

@@ -103,5 +103,10 @@ module EE
     def banzai_context_params
       { group: noteable.group, label_url_method: :group_epics_url }
     end
+
+    override :noteable_can_have_confidential_note?
+    def noteable_can_have_confidential_note?
+      for_epic? || super
+    end
   end
 end

ee/spec/requests/api/graphql/mutations/notes/create/note_spec.rb

@@ -6,12 +6,14 @@ RSpec.describe 'Adding a Note to an Epic' do
   include GraphqlHelpers
 
   let_it_be(:current_user) { create(:user) }
+  let_it_be(:group) { create(:group, :private) }
+  let_it_be(:epic) { create(:epic, group: group) }
 
-  let(:epic) { create(:epic, group: group) }
   let(:mutation) do
     variables = {
       noteable_id: GitlabSchema.id_from_object(epic).to_s,
-      body: 'Body text'
+      body: 'Body text',
+      confidential: true
     }
 
     graphql_mutation(:create_note, variables)
@@ -26,14 +28,14 @@ RSpec.describe 'Adding a Note to an Epic' do
   end
 
   context 'when the user does not have permission' do
-    let(:group) { create(:group, :private) }
-
     it_behaves_like 'a Note mutation when the user does not have permission'
   end
 
   context 'when the user has permission' do
-    let(:group) { create(:group, :public) }
+    before do
+      group.add_developer(current_user)
+    end
 
-    it_behaves_like 'a Note mutation that creates a Note'
+    it_behaves_like 'a Note mutation with confidential notes'
   end
 end

ee/spec/requests/api/notes_spec.rb

@@ -21,7 +21,7 @@ RSpec.describe API::Notes do
       stub_licensed_features(epics: true)
     end
 
-    it_behaves_like "noteable API", 'groups', 'epics', 'id' do
+    it_behaves_like "noteable API with confidential notes", 'groups', 'epics', 'id' do
       let(:parent) { group }
       let(:noteable) { epic }
       let(:note) { epic_note }

ee/spec/services/search/global_service_spec.rb

@@ -396,21 +396,9 @@ RSpec.describe Search::GlobalService do
     let_it_be(:project) { create(:project, :public, :repository) }
 
     context 'with notes on issues' do
-      let(:noteable) { create :issue, project: project }
+      let(:noteable) { create(:issue, project: project) }
 
-      it_behaves_like 'search notes shared examples', :note_on_issue
-    end
-
-    context 'with notes on merge requests' do
-      let(:noteable) { create :merge_request, target_project: project, source_project: project }
-
-      it_behaves_like 'search notes shared examples', :note_on_merge_request
-    end
-
-    context 'with notes on commits' do
-      let(:noteable) { create(:commit, project: project) }
-
-      it_behaves_like 'search notes shared examples', :note_on_commit
+      it_behaves_like 'search confidential notes shared examples', :note_on_issue
     end
   end
 end

ee/spec/support/shared_examples/services/search_notes_shared_examples.rb

 # frozen_string_literal: true
 
-RSpec.shared_examples 'search notes shared examples' do
+RSpec.shared_examples 'search confidential notes shared examples' do
   context 'notes confidentiality', :elastic, :sidekiq_inline do
     let_it_be(:user) { create(:user) }
 
@@ -42,7 +42,7 @@ RSpec.shared_examples 'search notes shared examples' do
       end
     end
 
-    # For now only issues can be confidential and have confidential notes,
+    # For now only issues and epics can be confidential and have confidential notes,
     # these specs are here to make sure not confidential notes on confidential issues
     # does not get leaked when mixed with other issuable notes.
     context 'with additional notes on a confidential issue' do

locale/gitlab.pot

@@ -43950,6 +43950,12 @@ msgstr ""
 msgid "can not be changed for existing notes"
 msgstr ""
 
+msgid "can not be set for this resource"
+msgstr ""
+
+msgid "can not be set for this type of note"
+msgstr ""
+
 msgid "can only be changed by a group admin."
 msgstr ""
 
@@ -45432,6 +45438,9 @@ msgid_plural "replies"
 msgstr[0] ""
 msgstr[1] ""
 
+msgid "reply should have same confidentiality as top-level note"
+msgstr ""
+
 msgid "repositories"
 msgstr ""
 

spec/controllers/projects/notes_controller_spec.rb

@@ -423,7 +423,21 @@ RSpec.describe Projects::NotesController do
       end
 
       context 'when creating a confidential note' do
-        let(:extra_request_params) { { format: :json } }
+        let(:project) { create(:project) }
+        let(:note_params) do
+          { note: note_text, noteable_id: issue.id, noteable_type: 'Issue' }.merge(extra_note_params)
+        end
+
+        let(:request_params) do
+          {
+            note: note_params,
+            namespace_id: project.namespace,
+            project_id: project,
+            target_type: 'issue',
+            target_id: issue.id,
+            format: :json
+          }
+        end
 
         context 'when `confidential` parameter is not provided' do
           it 'sets `confidential` to `false` in JSON response' do

spec/models/note_spec.rb

@@ -134,6 +134,74 @@ RSpec.describe Note do
           expect(existing_note.errors[:confidential]).to include('can not be changed for existing notes')
         end
       end
+
+      context 'for a new note' do
+        let_it_be(:noteable) { create(:issue) }
+
+        let(:note_params) { { confidential: true, noteable: noteable, project: noteable.project } }
+
+        subject { build(:note, **note_params) }
+
+        it 'allows to create a confidential note for an issue' do
+          expect(subject).to be_valid
+        end
+
+        context 'when noteable is not allowed to have confidential notes' do
+          let_it_be(:noteable) { create(:merge_request) }
+
+          it 'can not be set confidential' do
+            expect(subject).not_to be_valid
+            expect(subject.errors[:confidential]).to include('can not be set for this resource')
+          end
+        end
+
+        context 'when note type is not allowed to be confidential' do
+          let(:note_params) { { type: 'DiffNote', confidential: true, noteable: noteable, project: noteable.project } }
+
+          it 'can not be set confidential' do
+            expect(subject).not_to be_valid
+            expect(subject.errors[:confidential]).to include('can not be set for this type of note')
+          end
+        end
+
+        context 'when the note is a discussion note' do
+          let(:note_params) { { type: 'DiscussionNote', confidential: true, noteable: noteable, project: noteable.project } }
+
+          it { is_expected.to be_valid }
+        end
+
+        context 'when replying to a note' do
+          let(:note_params) { { confidential: true, noteable: noteable, project: noteable.project } }
+
+          subject { build(:discussion_note, discussion_id: original_note.discussion_id, **note_params) }
+
+          context 'when the note is reply to a confidential note' do
+            let_it_be(:original_note) { create(:note, confidential: true, noteable: noteable, project: noteable.project) }
+
+            it { is_expected.to be_valid }
+          end
+
+          context 'when the note is reply to a public note' do
+            let_it_be(:original_note) { create(:note, noteable: noteable, project: noteable.project) }
+
+            it 'can not be set confidential' do
+              expect(subject).not_to be_valid
+              expect(subject.errors[:confidential]).to include('reply should have same confidentiality as top-level note')
+            end
+          end
+
+          context 'when reply note is public but discussion is confidential' do
+            let_it_be(:original_note) { create(:note, confidential: true, noteable: noteable, project: noteable.project) }
+
+            let(:note_params) { { noteable: noteable, project: noteable.project } }
+
+            it 'can not be set confidential' do
+              expect(subject).not_to be_valid
+              expect(subject.errors[:confidential]).to include('reply should have same confidentiality as top-level note')
+            end
+          end
+        end
+      end
     end
   end
 
@@ -1199,8 +1267,8 @@ RSpec.describe Note do
   end
 
   describe "#discussion" do
-    let!(:note1) { create(:discussion_note_on_merge_request) }
-    let!(:note2) { create(:diff_note_on_merge_request, project: note1.project, noteable: note1.noteable) }
+    let_it_be(:note1) { create(:discussion_note_on_merge_request) }
+    let_it_be(:note2) { create(:diff_note_on_merge_request, project: note1.project, noteable: note1.noteable) }
 
     context 'when the note is part of a discussion' do
       subject { create(:discussion_note_on_merge_request, project: note1.project, noteable: note1.noteable, in_reply_to: note1) }

spec/policies/note_policy_spec.rb

@@ -359,39 +359,6 @@ RSpec.describe NotePolicy do
             expect(permissions(assignee, confidential_note)).to be_disallowed(:admin_note, :reposition_note, :resolve_note)
           end
         end
-
-        context 'for merge requests' do
-          let(:merge_request) { create(:merge_request, source_project: project, author: author, assignees: [assignee]) }
-          let(:confidential_note) { create(:note, :confidential, project: project, noteable: merge_request) }
-
-          it_behaves_like 'confidential notes permissions'
-
-          it 'allows noteable assignees to read all notes' do
-            expect(permissions(assignee, confidential_note)).to be_allowed(:read_note, :award_emoji)
-            expect(permissions(assignee, confidential_note)).to be_disallowed(:admin_note, :reposition_note, :resolve_note)
-          end
-        end
-
-        context 'for project snippets' do
-          let(:project_snippet) { create(:project_snippet, project: project, author: author) }
-          let(:confidential_note) { create(:note, :confidential, project: project, noteable: project_snippet) }
-
-          it_behaves_like 'confidential notes permissions'
-        end
-
-        context 'for personal snippets' do
-          let(:personal_snippet) { create(:personal_snippet, author: author) }
-          let(:confidential_note) { create(:note, :confidential, project: nil, noteable: personal_snippet) }
-
-          it 'allows snippet 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, :reposition_note)
-          end
-
-          it 'does not allow maintainers to read confidential notes and replies' do
-            expect(permissions(maintainer, confidential_note)).to be_disallowed(:read_note, :admin_note, :reposition_note, :resolve_note, :award_emoji)
-          end
-        end
       end
     end
   end

spec/requests/api/graphql/mutations/notes/create/note_spec.rb

@@ -17,8 +17,7 @@ RSpec.describe 'Adding a Note' do
       noteable_id: GitlabSchema.id_from_object(noteable).to_s,
       discussion_id: (GitlabSchema.id_from_object(discussion).to_s if discussion),
       merge_request_diff_head_sha: head_sha.presence,
-      body: body,
-      confidential: true
+      body: body
     }
 
     graphql_mutation(:create_note, variables)
@@ -49,7 +48,6 @@ RSpec.describe 'Adding a Note' do
       post_graphql_mutation(mutation, current_user: current_user)
 
       expect(mutation_response['note']['body']).to eq('Body text')
-      expect(mutation_response['note']['confidential']).to eq(true)
     end
 
     describe 'creating Notes in reply to a discussion' do
@@ -79,6 +77,25 @@ RSpec.describe 'Adding a Note' do
       end
     end
 
+    context 'for an issue' do
+      let(:noteable) { create(:issue, project: project) }
+      let(:mutation) do
+        variables = {
+          noteable_id: GitlabSchema.id_from_object(noteable).to_s,
+          body: body,
+          confidential: true
+        }
+
+        graphql_mutation(:create_note, variables)
+      end
+
+      before do
+        project.add_developer(current_user)
+      end
+
+      it_behaves_like 'a Note mutation with confidential notes'
+    end
+
     context 'when body only contains quick actions' do
       let(:head_sha) { noteable.diff_head_sha }
       let(:body) { '/merge' }

spec/requests/api/issues/get_project_issues_spec.rb

@@ -877,5 +877,35 @@ RSpec.describe API::Issues do
 
       expect(response).to have_gitlab_http_status(:not_found)
     end
+
+    context 'with a confidential note' do
+      let!(:note) do
+        create(
+          :note,
+          :confidential,
+          project: project,
+          noteable: issue,
+          author: create(:user)
+        )
+      end
+
+      it 'returns a full list of participants' do
+        get api("/projects/#{project.id}/issues/#{issue.iid}/participants", user)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        participant_ids = json_response.map { |el| el['id'] }
+        expect(participant_ids).to match_array([issue.author_id, note.author_id])
+      end
+
+      context 'when user cannot see a confidential note' do
+        it 'returns a limited list of participants' do
+          get api("/projects/#{project.id}/issues/#{issue.iid}/participants", create(:user))
+
+          expect(response).to have_gitlab_http_status(:ok)
+          participant_ids = json_response.map { |el| el['id'] }
+          expect(participant_ids).to match_array([issue.author_id])
+        end
+      end
+    end
   end
 end

spec/requests/api/notes_spec.rb

@@ -22,7 +22,7 @@ RSpec.describe API::Notes do
     let!(:issue) { create(:issue, project: project, author: user) }
     let!(:issue_note) { create(:note, noteable: issue, project: project, author: user) }
 
-    it_behaves_like "noteable API", 'projects', 'issues', 'iid' do
+    it_behaves_like "noteable API with confidential notes", 'projects', 'issues', 'iid' do
       let(:parent) { project }
       let(:noteable) { issue }
       let(:note) { issue_note }

spec/services/notes/create_service_spec.rb

@@ -106,7 +106,8 @@ RSpec.describe Notes::CreateService do
                        type: 'DiffNote',
                        noteable_type: 'MergeRequest',
                        noteable_id: merge_request.id,
-                       position: position.to_h)
+                       position: position.to_h,
+                       confidential: false)
           end
 
           before do
@@ -141,7 +142,8 @@ RSpec.describe Notes::CreateService do
                          type: 'DiffNote',
                          noteable_type: 'MergeRequest',
                          noteable_id: merge_request.id,
-                         position: position.to_h)
+                         position: position.to_h,
+                         confidential: false)
 
             expect(merge_request).not_to receive(:diffs)
 
@@ -173,7 +175,8 @@ RSpec.describe Notes::CreateService do
                          type: 'DiffNote',
                          noteable_type: 'MergeRequest',
                          noteable_id: merge_request.id,
-                         position: position.to_h)
+                         position: position.to_h,
+                         confidential: false)
             end
 
             it 'note is associated with a note diff file' do
@@ -201,7 +204,8 @@ RSpec.describe Notes::CreateService do
                          type: 'DiffNote',
                          noteable_type: 'MergeRequest',
                          noteable_id: merge_request.id,
-                         position: position.to_h)
+                         position: position.to_h,
+                         confidential: false)
             end
 
             it 'note is not associated with a note diff file' do
@@ -230,7 +234,8 @@ RSpec.describe Notes::CreateService do
                            type: 'DiffNote',
                            noteable_type: 'MergeRequest',
                            noteable_id: merge_request.id,
-                           position: image_position.to_h)
+                           position: image_position.to_h,
+                           confidential: false)
               end
 
               it 'note is not associated with a note diff file' do
@@ -306,7 +311,7 @@ RSpec.describe Notes::CreateService do
           let_it_be(:merge_request) { create(:merge_request, source_project: project, labels: [bug_label]) }
 
           let(:issuable) { merge_request }
-          let(:note_params) { opts.merge(noteable_type: 'MergeRequest', noteable_id: merge_request.id) }
+          let(:note_params) { opts.merge(noteable_type: 'MergeRequest', noteable_id: merge_request.id, confidential: false) }
           let(:merge_request_quick_actions) do
             [
               QuickAction.new(

spec/support/shared_examples/graphql/notes_creation_shared_examples.rb

@@ -85,3 +85,14 @@ RSpec.shared_examples 'a Note mutation when there are rate limit validation erro
     end
   end
 end
+
+RSpec.shared_examples 'a Note mutation with confidential notes' do
+  it_behaves_like 'a Note mutation that creates a Note'
+
+  it 'returns a Note with confidentiality enabled' do
+    post_graphql_mutation(mutation, current_user: current_user)
+
+    expect(mutation_response).to have_key('note')
+    expect(mutation_response['note']['confidential']).to eq(true)
+  end
+end

spec/support/shared_examples/requests/api/issuable_participants_examples.rb

@@ -28,34 +28,4 @@ RSpec.shared_examples 'issuable participants endpoint' do
 
     expect(response).to have_gitlab_http_status(:not_found)
   end
-
-  context 'with a confidential note' do
-    let!(:note) do
-      create(
-        :note,
-        :confidential,
-        project: project,
-        noteable: entity,
-        author: create(:user)
-      )
-    end
-
-    it 'returns a full list of participants' do
-      get api("/projects/#{project.id}/#{area}/#{entity.iid}/participants", user)
-
-      expect(response).to have_gitlab_http_status(:ok)
-      participant_ids = json_response.map { |el| el['id'] }
-      expect(participant_ids).to match_array([entity.author_id, note.author_id])
-    end
-
-    context 'when user cannot see a confidential note' do
-      it 'returns a limited list of participants' do
-        get api("/projects/#{project.id}/#{area}/#{entity.iid}/participants", create(:user))
-
-        expect(response).to have_gitlab_http_status(:ok)
-        participant_ids = json_response.map { |el| el['id'] }
-        expect(participant_ids).to match_array([entity.author_id])
-      end
-    end
-  end
 end

spec/support/shared_examples/requests/api/notes_shared_examples.rb

@@ -142,15 +142,6 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
       expect(json_response['author']['username']).to eq(user.username)
     end
 
-    it "creates a confidential note if confidential is set to true" do
-      post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), params: { body: 'hi!', confidential: true }
-
-      expect(response).to have_gitlab_http_status(:created)
-      expect(json_response['body']).to eq('hi!')
-      expect(json_response['confidential']).to be_truthy
-      expect(json_response['author']['username']).to eq(user.username)
-    end
-
     it "returns a 400 bad request error if body not given" do
       post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user)
 
@@ -312,26 +303,22 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
       put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes/#{note.id}", user), params: params
     end
 
-    context 'when eveything is ok' do
-      before do
+    context 'when only body param is present' do
+      let(:params) { { body: 'Hello!' } }
+
+      it 'updates the note text' do
         subject
-      end
 
-      it 'returns modified note' do
+        expect(note.reload.note).to eq('Hello!')
         expect(response).to have_gitlab_http_status(:ok)
         expect(json_response['body']).to eq('Hello!')
       end
-
-      it 'updates the note' do
-        expect(note.reload.note).to eq('Hello!')
-        expect(note.confidential).to be_falsey
-      end
     end
 
-    context 'when also confidential param is set' do
-      let(:params) { { body: 'Hello!', confidential: true } }
+    context 'when confidential param is present' do
+      let(:params) { { confidential: true } }
 
-      it 'fails to update the note' do
+      it 'does not allow to change confidentiality' do
         expect { subject }.not_to change { note.reload.note }
 
         expect(response).to have_gitlab_http_status(:bad_request)
@@ -376,3 +363,24 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
     end
   end
 end
+
+RSpec.shared_examples 'noteable API with confidential notes' do |parent_type, noteable_type, id_name|
+  it_behaves_like 'noteable API', parent_type, noteable_type, id_name
+
+  describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/notes" do
+    let(:params) { { body: 'hi!' } }
+
+    subject do
+      post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), params: params
+    end
+
+    it "creates a confidential note if confidential is set to true" do
+      post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), params: params.merge(confidential: true)
+
+      expect(response).to have_gitlab_http_status(:created)
+      expect(json_response['body']).to eq('hi!')
+      expect(json_response['confidential']).to be_truthy
+      expect(json_response['author']['username']).to eq(user.username)
+    end
+  end
+end