Fix visibility reference check

When all references in a system note are checked, it's not reliable to
check if there was any not-visible reference returned by an individual
parser because that reference could be satisfied by another parser.

Instead we check if in the end all nodes were visible.

Changelog: fixed

app/models/note.rb

@@ -643,7 +643,7 @@ class Note < ApplicationRecord
       user_visible_reference_count > 0 && user_visible_reference_count == total_reference_count
     else
       refs = all_references(user)
-      refs.all.any? && refs.stateful_not_visible_counter == 0
+      refs.all.any? && refs.all_visible?
     end
   end
 

lib/banzai/reference_parser/base_parser.rb

@@ -212,9 +212,8 @@ module Banzai
       def gather_references(nodes, ids_only: false)
         nodes = nodes_user_can_reference(current_user, nodes)
         visible = nodes_visible_to_user(current_user, nodes)
-        not_visible = nodes - visible
 
-        { visible: referenced_by(visible, ids_only: ids_only), not_visible: not_visible }
+        { visible: referenced_by(visible, ids_only: ids_only), nodes: nodes, visible_nodes: visible }
       end
 
       # Returns a Hash containing the projects for a given list of HTML nodes.

lib/gitlab/reference_extractor.rb

@@ -6,16 +6,11 @@ module Gitlab
     REFERABLES = %i(user issue label milestone mentioned_user mentioned_group mentioned_project
                     merge_request snippet commit commit_range directly_addressed_user epic iteration vulnerability).freeze
     attr_accessor :project, :current_user, :author
-    # This counter is increased by a number of references filtered out by
-    # banzai reference exctractor. Note that this counter is stateful and
-    # not idempotent and is increased whenever you call `references`.
-    attr_reader :stateful_not_visible_counter
 
     def initialize(project, current_user = nil)
       @project = project
       @current_user = current_user
       @references = {}
-      @stateful_not_visible_counter = 0
 
       super()
     end
@@ -26,14 +21,19 @@ module Gitlab
 
     def references(type, ids_only: false)
       refs = super(type, project, current_user, ids_only: ids_only)
-      @stateful_not_visible_counter += refs[:not_visible].count
+      update_visible_nodes_set(refs[:nodes], refs[:visible_nodes])
 
       refs[:visible]
     end
 
+    # this method is stateful, it tracks if all nodes from `references`
+    # calls are visible or not
+    def all_visible?
+      not_visible_nodes.empty?
+    end
+
     def reset_memoized_values
       @references = {}
-      @stateful_not_visible_counter = 0
       super()
     end
 
@@ -76,5 +76,16 @@ module Gitlab
 
       @pattern = Regexp.union(patterns.compact)
     end
+
+    private
+
+    def update_visible_nodes_set(all, visible)
+      not_visible_nodes.merge(all)
+      not_visible_nodes.subtract(visible)
+    end
+
+    def not_visible_nodes
+      @not_visible_nodes ||= Set.new
+    end
   end
 end

spec/lib/banzai/reference_parser/base_parser_spec.rb

@@ -247,15 +247,15 @@ RSpec.describe Banzai::ReferenceParser::BaseParser do
       end
     end
 
-    it 'returns referenceable and visible objects, alongside nodes that are referenceable but not visible' do
-      expect(subject.gather_references(nodes)).to match(
-        visible: contain_exactly(6, 8, 10),
-        not_visible: match_array(nodes.select { |n| n.id.even? && n.id <= 5 })
-      )
+    it 'returns referenceable and visible objects, alongside all and visible nodes' do
+      referenceable = nodes.select { |n| n.id.even? }
+      visible = nodes.select { |n| [6, 8, 10].include?(n.id) }
+
+      expect_gathered_references(subject.gather_references(nodes), [6, 8, 10], referenceable, visible)
     end
 
     it 'is always empty if the input is empty' do
-      expect(subject.gather_references([])) .to match(visible: be_empty, not_visible: be_empty)
+      expect_gathered_references(subject.gather_references([]), [], [], [])
     end
   end
 

spec/lib/banzai/reference_parser/mentioned_group_parser_spec.rb

@@ -19,7 +19,7 @@ RSpec.describe Banzai::ReferenceParser::MentionedGroupParser do
         it 'returns empty array' do
           link['data-group'] = project.group.id.to_s
 
-          expect_gathered_references(subject.gather_references([link]), [], 1)
+          expect_gathered_references(subject.gather_references([link]), [], [link], [])
         end
       end
 
@@ -30,7 +30,7 @@ RSpec.describe Banzai::ReferenceParser::MentionedGroupParser do
         end
 
         it 'returns groups' do
-          expect_gathered_references(subject.gather_references([link]), [group], 0)
+          expect_gathered_references(subject.gather_references([link]), [group], [link], [link])
         end
       end
 
@@ -38,7 +38,7 @@ RSpec.describe Banzai::ReferenceParser::MentionedGroupParser do
         it 'returns an empty Array' do
           link['data-group'] = 'test-non-existing'
 
-          expect_gathered_references(subject.gather_references([link]), [], 1)
+          expect_gathered_references(subject.gather_references([link]), [], [link], [])
         end
       end
     end

spec/lib/banzai/reference_parser/mentioned_project_parser_spec.rb

@@ -19,7 +19,7 @@ RSpec.describe Banzai::ReferenceParser::MentionedProjectParser do
         it 'returns empty Array' do
           link['data-project'] = project.id.to_s
 
-          expect_gathered_references(subject.gather_references([link]), [], 1)
+          expect_gathered_references(subject.gather_references([link]), [], [link], [])
         end
       end
 
@@ -30,7 +30,7 @@ RSpec.describe Banzai::ReferenceParser::MentionedProjectParser do
         end
 
         it 'returns an Array of referenced projects' do
-          expect_gathered_references(subject.gather_references([link]), [project], 0)
+          expect_gathered_references(subject.gather_references([link]), [project], [link], [link])
         end
       end
 
@@ -38,7 +38,7 @@ RSpec.describe Banzai::ReferenceParser::MentionedProjectParser do
         it 'returns an empty Array' do
           link['data-project'] = 'inexisting-project-id'
 
-          expect_gathered_references(subject.gather_references([link]), [], 1)
+          expect_gathered_references(subject.gather_references([link]), [], [link], [])
         end
       end
     end

spec/lib/banzai/reference_parser/mentioned_user_parser_spec.rb

@@ -22,7 +22,7 @@ RSpec.describe Banzai::ReferenceParser::MentionedUserParser do
         end
 
         it 'returns empty list of users' do
-          expect_gathered_references(subject.gather_references([link]), [], 0)
+          expect_gathered_references(subject.gather_references([link]), [], [link], [link])
         end
       end
     end
@@ -35,7 +35,7 @@ RSpec.describe Banzai::ReferenceParser::MentionedUserParser do
         end
 
         it 'returns empty list of users' do
-          expect_gathered_references(subject.gather_references([link]), [], 0)
+          expect_gathered_references(subject.gather_references([link]), [], [link], [link])
         end
       end
     end
@@ -44,7 +44,7 @@ RSpec.describe Banzai::ReferenceParser::MentionedUserParser do
       it 'returns an Array of users' do
         link['data-user'] = user.id.to_s
 
-        expect_gathered_references(subject.gather_references([link]), [user], 0)
+        expect_gathered_references(subject.gather_references([link]), [user], [link], [link])
       end
     end
   end

spec/lib/banzai/reference_parser/project_parser_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe Banzai::ReferenceParser::ProjectParser do
         it 'returns an Array of projects' do
           link['data-project'] = project.id.to_s
 
-          expect_gathered_references(subject.gather_references([link]), [project], 0)
+          expect_gathered_references(subject.gather_references([link]), [project], [link], [link])
         end
       end
 
@@ -25,7 +25,7 @@ RSpec.describe Banzai::ReferenceParser::ProjectParser do
         it 'returns an empty Array' do
           link['data-project'] = ''
 
-          expect_gathered_references(subject.gather_references([link]), [], 1)
+          expect_gathered_references(subject.gather_references([link]), [], [link], [])
         end
       end
 
@@ -35,7 +35,7 @@ RSpec.describe Banzai::ReferenceParser::ProjectParser do
 
           link['data-project'] = private_project.id.to_s
 
-          expect_gathered_references(subject.gather_references([link]), [], 1)
+          expect_gathered_references(subject.gather_references([link]), [], [link], [])
         end
 
         it 'returns an Array when authorized' do
@@ -43,7 +43,7 @@ RSpec.describe Banzai::ReferenceParser::ProjectParser do
 
           link['data-project'] = private_project.id.to_s
 
-          expect_gathered_references(subject.gather_references([link]), [private_project], 0)
+          expect_gathered_references(subject.gather_references([link]), [private_project], [link], [link])
         end
       end
     end

spec/lib/gitlab/reference_extractor_spec.rb

@@ -332,14 +332,59 @@ RSpec.describe Gitlab::ReferenceExtractor do
       it 'returns visible references of given type' do
         expect(subject.references(:issue)).to eq([issue])
       end
+    end
 
-      it 'does not increase stateful_not_visible_counter' do
-        expect { subject.references(:issue) }.not_to change { subject.stateful_not_visible_counter }
-      end
+    it 'does not return any references' do
+      expect(subject.references(:issue)).to be_empty
+    end
+  end
+
+  describe '#all_visible?' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project2) { create(:project) }
+    let_it_be(:issue) { create(:issue, project: project) }
+    let_it_be(:issue2) { create(:issue, project: project2) }
+
+    let(:text) { "Ref. #{issue.to_reference} and #{issue2.to_reference(project)}" }
+
+    subject { described_class.new(project, user) }
+
+    before do
+      subject.analyze(text)
     end
 
-    it 'increases stateful_not_visible_counter' do
-      expect { subject.references(:issue) }.to change { subject.stateful_not_visible_counter }.by(1)
+    it 'returns true if no references were parsed yet' do
+      expect(subject.all_visible?).to be_truthy
+    end
+
+    context 'when references was already called' do
+      let(:membership) { [] }
+
+      before do
+        membership.each { |p| p.add_developer(user) }
+
+        subject.references(:issue)
+      end
+
+      it 'returns false' do
+        expect(subject.all_visible?).to be_falsey
+      end
+
+      context 'when user can access only some references' do
+        let(:membership) { [project] }
+
+        it 'returns false' do
+          expect(subject.all_visible?).to be_falsey
+        end
+      end
+
+      context 'when user can access all references' do
+        let(:membership) { [project, project2] }
+
+        it 'returns true' do
+          expect(subject.all_visible?).to be_truthy
+        end
+      end
     end
   end
 end

spec/models/note_spec.rb

@@ -500,15 +500,15 @@ RSpec.describe Note do
     let_it_be(:ext_issue) { create(:issue, project: ext_proj) }
 
     shared_examples "checks references" do
-      it "returns true" do
+      it "returns false" do
         expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
       end
 
-      it "returns false" do
+      it "returns true" do
         expect(note.system_note_with_references_visible_for?(private_user)).to be_truthy
       end
 
-      it "returns false if user visible reference count set" do
+      it "returns true if user visible reference count set" do
         note.user_visible_reference_count = 1
         note.total_reference_count = 1
 
@@ -516,7 +516,15 @@ RSpec.describe Note do
         expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_truthy
       end
 
-      it "returns true if ref count is 0" do
+      it "returns false if user visible reference count set but does not match total reference count" do
+        note.user_visible_reference_count = 1
+        note.total_reference_count = 2
+
+        expect(note).not_to receive(:reference_mentionables)
+        expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
+      end
+
+      it "returns false if ref count is 0" do
         note.user_visible_reference_count = 0
 
         expect(note).not_to receive(:reference_mentionables)
@@ -562,13 +570,35 @@ RSpec.describe Note do
       end
 
       it_behaves_like "checks references"
+    end
 
-      it "returns true if user visible reference count set and there is a private reference" do
-        note.user_visible_reference_count = 1
-        note.total_reference_count = 2
+    context "when there is a private issue and user reference" do
+      let_it_be(:ext_issue2) { create(:issue, project: ext_proj) }
 
-        expect(note).not_to receive(:reference_mentionables)
-        expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
+      let(:note) do
+        create :note,
+          noteable: ext_issue2, project: ext_proj,
+          note: "mentioned in #{private_issue.to_reference(ext_proj)} and pinged user #{private_user.to_reference}",
+          system: true
+      end
+
+      it_behaves_like "checks references"
+    end
+
+    context "when there is a publicly visible user reference" do
+      let(:note) do
+        create :note,
+          noteable: ext_issue, project: ext_proj,
+          note: "mentioned in #{ext_proj.owner.to_reference}",
+          system: true
+      end
+
+      it "returns true for other users" do
+        expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_truthy
+      end
+
+      it "returns true for anonymous users" do
+        expect(note.system_note_with_references_visible_for?(nil)).to be_truthy
       end
     end
   end

spec/support/helpers/reference_parser_helpers.rb

@@ -5,9 +5,10 @@ module ReferenceParserHelpers
     Nokogiri::HTML.fragment('<a></a>').children[0]
   end
 
-  def expect_gathered_references(result, visible, not_visible_count)
+  def expect_gathered_references(result, visible, nodes, visible_nodes)
     expect(result[:visible]).to eq(visible)
-    expect(result[:not_visible].count).to eq(not_visible_count)
+    expect(result[:nodes]).to eq(nodes)
+    expect(result[:visible_nodes]).to eq(visible_nodes)
   end
 
   RSpec.shared_examples 'no project N+1 queries' do