Merge branch '39461-notes-api-for-issues-no-longer-returns-label-additions-removals' into 'master'

Resolve "Notes API for issues no longer returns label additions/removals"

Closes #39461

See merge request gitlab-org/gitlab-ce!15080

6 files changed, 66 insertions, 3 deletions

diff --git a/app/models/note.rb b/app/models/note.rb
index f9676361072..50c9caf8529 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -110,6 +110,7 @@ class Note < ActiveRecord::Base
     includes(:author, :noteable, :updated_by,
              project: [:project_members, { group: [:group_members] }])
   end
+  scope :with_metadata, -> { includes(:system_note_metadata) }
 
   after_initialize :ensure_discussion_id
   before_validation :nullify_blank_type, :nullify_blank_line_code
@@ -169,7 +170,13 @@ class Note < ActiveRecord::Base
   end
 
   def cross_reference?
-    system? && matches_cross_reference_regex?
+    return unless system?
+
+    if force_cross_reference_regex_check?
+      matches_cross_reference_regex?
+    else
+      SystemNoteService.cross_reference?(note)
+    end
   end
 
   def diff_note?
@@ -382,4 +389,10 @@ class Note < ActiveRecord::Base
   def set_discussion_id
     self.discussion_id ||= discussion_class.discussion_id(self)
   end
+
+  def force_cross_reference_regex_check?
+    return unless system?
+
+    SystemNoteMetadata::TYPES_WITH_CROSS_REFERENCES.include?(system_note_metadata&.action)
+  end
 end
diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb
index 1f9f8d7286b..29035480371 100644
--- a/app/models/system_note_metadata.rb
+++ b/app/models/system_note_metadata.rb
@@ -1,4 +1,14 @@
 class SystemNoteMetadata < ActiveRecord::Base
+  # These notes's action text might contain a reference that is external.
+  # We should always force a deep validation upon references that are found
+  # in this note type.
+  # Other notes can always be safely shown as all its references are
+  # in the same project (i.e. with the same permissions)
+  TYPES_WITH_CROSS_REFERENCES = %w[
+    commit cross_reference
+    close duplicate
+  ].freeze
+
   ICON_TYPES = %w[
     commit description merge confidential visible label assignee cross_reference
     title time_tracking branch milestone discussion task moved
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index e946218824c..fe71a405565 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -583,6 +583,10 @@ module SystemNoteService
     create_note(NoteSummary.new(issuable, issuable.project, author, body, action: action))
   end
 
+  def cross_reference?(note_text)
+    note_text =~ /\A#{cross_reference_note_prefix}/i
+  end
+
   private
 
   def notes_for_mentioner(mentioner, noteable, notes)
diff --git a/changelogs/unreleased/39461-notes-api-for-issues-no-longer-returns-label-additions-removals.yml b/changelogs/unreleased/39461-notes-api-for-issues-no-longer-returns-label-additions-removals.yml
new file mode 100644
index 00000000000..36c2f789eeb
--- /dev/null
+++ b/changelogs/unreleased/39461-notes-api-for-issues-no-longer-returns-label-additions-removals.yml
@@ -0,0 +1,5 @@
+---
+title: Label addition/removal are not going to be redacted wrongfully in the API.
+merge_request: 15080
+author:
+type: fixed
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index 0b9ab4eeb05..ceaaeca4046 100644
--- a/lib/api/notes.rb
+++ b/lib/api/notes.rb
@@ -33,7 +33,7 @@ module API
               # paginate() only works with a relation. This could lead to a
               # mismatch between the pagination headers info and the actual notes
               # array returned, but this is really a edge-case.
-              paginate(noteable.notes)
+              paginate(noteable.notes.with_metadata)
               .reject { |n| n.cross_reference_not_visible_for?(current_user) }
             present notes, with: Entities::Note
           else
@@ -50,7 +50,7 @@ module API
         end
         get ":id/#{noteables_str}/:noteable_id/notes/:note_id" do
           noteable = find_project_noteable(noteables_str, params[:noteable_id])
-          note = noteable.notes.find(params[:note_id])
+          note = noteable.notes.with_metadata.find(params[:note_id])
           can_read_note = can?(current_user, noteable_read_ability_name(noteable), noteable) && !note.cross_reference_not_visible_for?(current_user)
 
           if can_read_note
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 1ecb50586c7..6e7e8c4c570 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -231,6 +231,37 @@ describe Note do
     end
   end
 
+  describe '#cross_reference?' do
+    it 'falsey for user-generated notes' do
+      note = create(:note, system: false)
+
+      expect(note.cross_reference?).to be_falsy
+    end
+
+    context 'when the note might contain cross references' do
+      SystemNoteMetadata::TYPES_WITH_CROSS_REFERENCES.each do |type|
+        let(:note) { create(:note, :system) }
+        let!(:metadata) { create(:system_note_metadata, note: note, action: type) }
+
+        it 'delegates to the cross-reference regex' do
+          expect(note).to receive(:matches_cross_reference_regex?).and_return(false)
+
+          note.cross_reference?
+        end
+      end
+    end
+
+    context 'when the note cannot contain cross references' do
+      let(:commit_note) { build(:note, note: 'mentioned in 1312312313 something else.', system: true) }
+      let(:label_note) { build(:note, note: 'added ~2323232323', system: true) }
+
+      it 'scan for a `mentioned in` prefix' do
+        expect(commit_note.cross_reference?).to be_truthy
+        expect(label_note.cross_reference?).to be_falsy
+      end
+    end
+  end
+
   describe 'clear_blank_line_code!' do
     it 'clears a blank line code before validation' do
       note = build(:note, line_code: ' ')