ES: fix note leak for private feature

Note has 4 parent types. Since they are different features, they
can't share project ids filtering.

Each "has_parent" query must be "AND"ed with its respective filtered
project ids. The 4 queries are then "OR"ed.

ee/changelogs/unreleased/security-33712.yml

+---
+title: Fix private comment Elasticsearch leak on project search scope
+merge_request:
+author:
+type: security

ee/lib/elastic/latest/note_class_proxy.rb

@@ -3,6 +3,8 @@
 module Elastic
   module Latest
     class NoteClassProxy < ApplicationClassProxy
+      extend ::Gitlab::Utils::Override
+
       def es_type
         'note'
       end
@@ -60,6 +62,98 @@ module Elastic
         query_hash[:query][:bool][:filter] << filter
         query_hash
       end
+
+      def noteable_type_to_feature
+        {
+          'Issue': :issues,
+          'MergeRequest': :merge_requests,
+          'Snippet': :snippets,
+          'Commit': :repository
+        }
+      end
+
+      override :project_ids_filter
+      def project_ids_filter(query_hash, options)
+        query_hash[:query][:bool][:filter] ||= []
+
+        project_query = project_ids_query(
+          options[:current_user],
+          options[:project_ids],
+          options[:public_and_internal_projects],
+          options[:features]
+        )
+
+        filters = {
+          bool: {
+            should: []
+          }
+        }
+
+        # For each project id filter,
+        # extract the noteable_type attribute,
+        # and use that to filter at Note level.
+        project_query[:should].flatten.each do |condition|
+          noteable_type = condition.delete(:noteable_type).to_s
+
+          filters[:bool][:should] << {
+            bool: {
+              must: [
+                {
+                  has_parent: {
+                    parent_type: "project",
+                    query: {
+                      bool: {
+                        should: condition
+                      }
+                    }
+                  }
+                },
+                { term: { noteable_type: noteable_type } }
+              ]
+            }
+          }
+        end
+
+        query_hash[:query][:bool][:filter] << filters
+        query_hash
+      end
+
+      # Query notes based on the various feature permission of the noteable_type.
+      # Appends `noteable_type` (which will be removed in project_ids_filter)
+      # for base model filtering.
+      override :pick_projects_by_membership
+      def pick_projects_by_membership(project_ids, user, _ = nil)
+        noteable_type_to_feature.map do |noteable_type, feature|
+          condition =
+            if project_ids == :any
+              { term: { visibility_level: Project::PRIVATE } }
+            else
+              { terms: { id: filter_ids_by_feature(project_ids, user, feature) } }
+            end
+
+          limit =
+            { terms: { "#{feature}_access_level" => [::ProjectFeature::ENABLED, ::ProjectFeature::PRIVATE] } }
+
+          { bool: { filter: [condition, limit] }, noteable_type: noteable_type }
+        end
+      end
+
+      # Query notes based on the various feature permission of the noteable_type.
+      # Appends `noteable_type` (which will be removed in project_ids_filter)
+      # for base model filtering.
+      override :limit_by_feature
+      def limit_by_feature(condition, _ = nil, include_members_only:)
+        noteable_type_to_feature.map do |noteable_type, feature|
+          limit =
+            if include_members_only
+              { terms: { "#{feature}_access_level" => [::ProjectFeature::ENABLED, ::ProjectFeature::PRIVATE] } }
+            else
+              { term: { "#{feature}_access_level" => ::ProjectFeature::ENABLED } }
+            end
+
+          { bool: { filter: [condition, limit] }, noteable_type: noteable_type }
+        end
+      end
     end
   end
 end

ee/spec/lib/gitlab/elastic/search_results_spec.rb

@@ -235,7 +235,7 @@ describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need_inlin
     it 'redacts commit comments when user is a guest on a private project' do
       project_1.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
       project_1.add_guest(user)
-      note_on_commit = create(
+      create(
         :note_on_commit,
         project: project_1,
         note: 'foo note on commit'
@@ -245,13 +245,9 @@ describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need_inlin
 
       results = described_class.new(user, 'foo', limit_project_ids)
 
-      expect(results.send(:logger))
-        .to receive(:error)
-        .with(hash_including(message: "redacted_search_results", filtered: array_including([
-          { class_name: "Note", id: note_on_commit.id, ability: :read_note }
-      ])))
+      expect(results.send(:logger)).not_to receive(:error)
 
-      expect(results.notes_count).to eq(3) # 3 because redacting only happens when we instantiate the results
+      expect(results.notes_count).to eq(2)
       expect(results.objects('notes')).to match_array([@note_1, @note_2])
     end
   end

ee/spec/models/concerns/elastic/note_spec.rb

@@ -36,24 +36,23 @@ describe Note, :elastic do
     end
   end
 
-  it "searches notes", :sidekiq_might_not_need_inline do
-    issue = create :issue
+  it "searches notes", :sidekiq_inline do
+    project = create :project, :public
+    issue = create :issue, project: project
 
-    Sidekiq::Testing.inline! do
-      create :note, note: 'bla-bla term1', project: issue.project
-      create :note, project: issue.project
+    note = create :note, note: 'bla-bla term1', project: issue.project
+    create :note, project: issue.project
 
-      # The note in the project you have no access to except as an administrator
-      create :note, note: 'bla-bla term2'
+    # The note in the project you have no access to except as an administrator
+    outside_note = create :note, note: 'bla-bla term2'
 
-      Gitlab::Elastic::Helper.refresh_index
-    end
+    Gitlab::Elastic::Helper.refresh_index
 
     options = { project_ids: [issue.project.id] }
 
-    expect(described_class.elastic_search('term1 | term2', options: options).total_count).to eq(1)
-    expect(described_class.elastic_search('bla-bla', options: options).total_count).to eq(1)
-    expect(described_class.elastic_search('bla-bla', options: { project_ids: :any }).total_count).to eq(2)
+    expect(described_class.elastic_search('term1 | term2', options: options).records).to contain_exactly(note)
+    expect(described_class.elastic_search('bla-bla', options: options).records).to contain_exactly(note)
+    expect(described_class.elastic_search('bla-bla', options: { project_ids: :any }).records).to contain_exactly(outside_note)
   end
 
   it "indexes && searches diff notes" do
@@ -65,6 +64,10 @@ describe Note, :elastic do
       notes << create(:legacy_diff_note_on_merge_request, note: "term")
       notes << create(:legacy_diff_note_on_commit, note: "term")
 
+      notes.each do |note|
+        note.project.update!(visibility: Gitlab::VisibilityLevel::PUBLIC)
+      end
+
       Gitlab::Elastic::Helper.refresh_index
     end
 
@@ -138,6 +141,7 @@ describe Note, :elastic do
     it "finds note when user is authorized to see it", :sidekiq_might_not_need_inline do
       user = create :user
       issue = create :issue, :confidential, author: user
+      issue.project.add_guest user
 
       Sidekiq::Testing.inline! do
         create_notes_for(issue, 'bla-bla term')

ee/spec/services/elastic/index_record_service_spec.rb

@@ -277,7 +277,7 @@ describe Elastic::IndexRecordService, :elastic do
     note = nil
 
     Sidekiq::Testing.inline! do
-      project = create :project, :repository
+      project = create :project, :repository, :public
       note = create :note, project: project, note: 'note_1'
       Gitlab::Elastic::Helper.refresh_index
     end
@@ -323,16 +323,17 @@ describe Elastic::IndexRecordService, :elastic do
   context 'when updating an Issue' do
     context 'when changing the confidential value' do
       it 'updates issue notes excluding system notes' do
+        project = create(:project, :public)
         issue = nil
         Sidekiq::Testing.disable! do
-          issue = create(:issue, confidential: false)
-          subject.execute(issue.project, true)
+          issue = create(:issue, project: project, confidential: false)
+          subject.execute(project, true)
           subject.execute(issue, false)
-          create(:note, note: 'the_normal_note', noteable: issue, project: issue.project)
-          create(:note, note: 'the_system_note', system: true, noteable: issue, project: issue.project)
+          create(:note, note: 'the_normal_note', noteable: issue, project: project)
+          create(:note, note: 'the_system_note', system: true, noteable: issue, project: project)
         end
 
-        options = { project_ids: [issue.project.id] }
+        options = { project_ids: [project.id] }
 
         Sidekiq::Testing.inline! do
           expect(subject.execute(issue, false, 'changed_fields' => ['confidential'])).to eq(true)