Fix elasticsearch guest results permissions

Fix pendings guest searches for project/group/global
searches.

changelogs/unreleased/es-guest-results.yml

+---
+title: Fix advanced global search permissions for guest users
+merge_request: 23177
+author:
+type: fixed

ee/lib/gitlab/elastic/search_results.rb

@@ -211,7 +211,10 @@ module Gitlab
 
       def merge_requests
         strong_memoize(:merge_requests) do
-          options = base_options.merge(project_ids: non_guest_project_ids)
+          options = base_options.merge(
+            project_ids: filter_project_ids_by_feature(:merge_requests, limit_project_ids)
+          )
+
           MergeRequest.elastic_search(query, options: options)
         end
       end
@@ -226,17 +229,14 @@ module Gitlab
         return Kaminari.paginate_array([]) if query.blank?
 
         strong_memoize(:blobs) do
-          project_ids = visible_project_ids
-
-          opt = base_options.merge(
-            additional_filter: repository_filter(project_ids),
-            project_ids: project_ids
+          options = base_options.merge(
+            additional_filter: repository_filter(limit_project_ids)
           )
 
           Repository.elastic_search(
             query,
             type: :blob,
-            options: opt.merge({ highlight: true })
+            options: options.merge({ highlight: true })
           )[:blobs][:results].response
         end
       end
@@ -245,17 +245,14 @@ module Gitlab
         return Kaminari.paginate_array([]) if query.blank?
 
         strong_memoize(:wiki_blobs) do
-          project_ids = visible_project_ids(visible_for_guests: true)
-
-          opt = base_options.merge(
-            additional_filter: wiki_filter(project_ids),
-            project_ids: project_ids
+          options = base_options.merge(
+            additional_filter: wiki_filter(limit_project_ids)
           )
 
           ProjectWiki.elastic_search(
             query,
             type: :wiki_blob,
-            options: opt.merge({ highlight: true })
+            options: options.merge({ highlight: true })
           )[:wiki_blobs][:results].response
         end
       end
@@ -270,11 +267,8 @@ module Gitlab
         return Kaminari.paginate_array([]) if query.blank?
 
         strong_memoize(:commits) do
-          project_ids = visible_project_ids
-
           options = base_options.merge(
-            additional_filter: repository_filter(project_ids),
-            project_ids: project_ids
+            additional_filter: repository_filter(limit_project_ids)
           )
 
           Repository.find_commits_by_message_with_elastic(
@@ -294,22 +288,24 @@ module Gitlab
         blob_filter(:repository, project_ids)
       end
 
-      def visible_project_ids(visible_for_guests: false)
-        visible_for_guests ? limit_project_ids : non_guest_project_ids
+      def filter_project_ids_by_feature(feature, project_ids)
+        return project_ids if project_ids == :any
+
+        Project
+          .id_in(project_ids)
+          .filter_by_feature_visibility(feature, current_user)
+          .pluck_primary_key
       end
 
       def blob_filter(feature, project_ids)
         key_name = "#{feature}_access_level"
 
+        project_ids = filter_project_ids_by_feature(feature, project_ids)
+
         conditions =
           if project_ids == :any
             [{ exists: { field: "id" } }]
           else
-            project_ids = Project
-              .id_in(project_ids)
-              .filter_by_feature_visibility(feature, current_user)
-              .pluck_primary_key
-
             [{ terms: { id: project_ids } }]
           end
 

ee/spec/elastic_integration/global_search_spec.rb

@@ -45,7 +45,7 @@ describe 'GlobalSearch', :elastic do
         expect_items_to_be_found(auditor)
         expect_items_to_be_found(member)
         expect_items_to_be_found(external_member)
-        expect_non_code_items_to_be_found(guest)
+        expect_items_to_be_found(guest, except: [:merge_requests, :blobs, :commits])
         expect_no_items_to_be_found(non_member)
         expect_no_items_to_be_found(external_non_member)
         expect_no_items_to_be_found(nil)
@@ -89,7 +89,7 @@ describe 'GlobalSearch', :elastic do
         expect_items_to_be_found(auditor)
         expect_items_to_be_found(member)
         expect_items_to_be_found(external_member)
-        expect_non_code_items_to_be_found(guest)
+        expect_items_to_be_found(guest, except: :merge_requests)
         expect_no_items_to_be_found(non_member)
         expect_no_items_to_be_found(external_non_member)
         expect_no_items_to_be_found(nil)
@@ -133,7 +133,7 @@ describe 'GlobalSearch', :elastic do
         expect_items_to_be_found(auditor)
         expect_items_to_be_found(member)
         expect_items_to_be_found(external_member)
-        expect_non_code_items_to_be_found(guest)
+        expect_items_to_be_found(guest, except: :merge_requests)
         expect_no_items_to_be_found(non_member)
         expect_no_items_to_be_found(external_non_member)
         expect_no_items_to_be_found(nil)
@@ -163,30 +163,35 @@ describe 'GlobalSearch', :elastic do
   end
 
   def expect_no_items_to_be_found(user)
-    results = search(user, 'term')
-    expect(results.issues_count).to eq(0)
-    expect(results.merge_requests_count).to eq(0)
-    expect(results.wiki_blobs_count).to eq(0)
-    expect(search(user, 'def').blobs_count).to eq(0)
-    expect(search(user, 'add').commits_count).to eq(0)
+    expect_items_to_be_found(user, except: :all)
   end
 
-  def expect_items_to_be_found(user)
-    results = search(user, 'term')
-    expect(results.issues_count).not_to eq(0)
-    expect(results.merge_requests_count).not_to eq(0)
-    expect(results.wiki_blobs_count).not_to eq(0)
-    expect(search(user, 'def').blobs_count).not_to eq(0)
-    expect(search(user, 'add').commits_count).not_to eq(0)
+  POSSIBLE_FEATURES = %i(issues merge_requests wiki_blobs blobs commits).freeze
+
+  def expect_items_to_be_found(user, only: nil, except: nil)
+    arr = if only
+            [only].flatten.compact
+          elsif except == :all
+            []
+          else
+            POSSIBLE_FEATURES - [except].flatten.compact
+          end
+
+    check_count = lambda do |feature, c|
+      if arr.include?(feature)
+        expect(c).to be > 0
+      else
+        expect(c).to eq(0)
+      end
     end
 
-  def expect_non_code_items_to_be_found(user)
     results = search(user, 'term')
-    expect(results.issues_count).not_to eq(0)
-    expect(results.wiki_blobs_count).not_to eq(0)
-    expect(results.merge_requests_count).to eq(0)
-    expect(search(user, 'def').blobs_count).to eq(0)
-    expect(search(user, 'add').commits_count).to eq(0)
+
+    check_count[:issues, results.issues_count]
+    check_count[:merge_requests, results.merge_requests_count]
+    check_count[:wiki_blobs, results.wiki_blobs_count]
+    check_count[:blobs, search(user, 'def').blobs_count]
+    check_count[:commits, search(user, 'add').commits_count]
   end
 
   def search(user, search, snippets: false)

ee/spec/services/search/global_service_spec.rb

@@ -38,7 +38,7 @@ describe Search::GlobalService do
           update_feature_access_level(project, feature_access_level)
           Gitlab::Elastic::Helper.refresh_index
 
-          expect_search_results(user, 'merge_requests', expected_count: expected_count, pending: pending?) do |user|
+          expect_search_results(user, 'merge_requests', expected_count: expected_count) do |user|
             described_class.new(user, search: merge_request.title).execute
           end
 
@@ -52,12 +52,6 @@ describe Search::GlobalService do
     context 'code' do
       let!(:project) { create(:project, project_level, :repository, namespace: group ) }
       let!(:note) { create :note_on_commit, project: project }
-      let(:pendings) do
-        [
-          { project_level: :public, feature_access_level: :private, membership: :guest, expected_count: 1 },
-          { project_level: :internal, feature_access_level: :private, membership: :guest, expected_count: 1 }
-        ]
-      end
 
       where(:project_level, :feature_access_level, :membership, :expected_count) do
         permission_table_for_guest_feature_access_and_non_private_project_only
@@ -69,7 +63,7 @@ describe Search::GlobalService do
           ElasticCommitIndexerWorker.new.perform(project.id)
           Gitlab::Elastic::Helper.refresh_index
 
-          expect_search_results(user, 'commits', expected_count: expected_count, pending: pending?) do |user|
+          expect_search_results(user, 'commits', expected_count: expected_count) do |user|
             described_class.new(user, search: 'initial').execute
           end
 

ee/spec/services/search/group_service_spec.rb

@@ -80,12 +80,6 @@ describe Search::GroupService, :elastic do
       let!(:merge_request2) { create :merge_request, target_project: project2, source_project: project2, title: merge_request.title }
       let!(:note) { create :note, project: project, noteable: merge_request }
       let!(:note2) { create :note, project: project2, noteable: merge_request2, note: note.note }
-      let(:pendings) do
-        [
-          { project_level: :public, feature_access_level: :enabled, membership: :guest, expected_count: 1 },
-          { project_level: :internal, feature_access_level: :enabled, membership: :guest, expected_count: 1 }
-        ]
-      end
 
       where(:project_level, :feature_access_level, :membership, :expected_count) do
         permission_table_for_reporter_feature_access
@@ -98,7 +92,7 @@ describe Search::GroupService, :elastic do
           end
           Gitlab::Elastic::Helper.refresh_index
 
-          expect_search_results(user, 'merge_requests', expected_count: expected_count, pending: pending?) do |user|
+          expect_search_results(user, 'merge_requests', expected_count: expected_count) do |user|
             described_class.new(user, group, search: merge_request.title).execute
           end
 
@@ -112,14 +106,6 @@ describe Search::GroupService, :elastic do
     context 'code' do
       let!(:project) { create(:project, project_level, :repository, namespace: group ) }
       let!(:note) { create :note_on_commit, project: project }
-      let(:pendings) do
-        [
-          { project_level: :public, feature_access_level: :enabled, membership: :guest, expected_count: 1 },
-          { project_level: :public, feature_access_level: :private, membership: :guest, expected_count: 1 },
-          { project_level: :internal, feature_access_level: :enabled, membership: :guest, expected_count: 1 },
-          { project_level: :internal, feature_access_level: :private, membership: :guest, expected_count: 1 }
-        ]
-      end
 
       where(:project_level, :feature_access_level, :membership, :expected_count) do
         permission_table_for_guest_feature_access_and_non_private_project_only
@@ -134,15 +120,15 @@ describe Search::GroupService, :elastic do
           ElasticCommitIndexerWorker.new.perform(project.id)
           Gitlab::Elastic::Helper.refresh_index
 
-          expect_search_results(user, 'commits', expected_count: expected_count, pending: pending?) do |user|
+          expect_search_results(user, 'commits', expected_count: expected_count) do |user|
             described_class.new(user, group, search: 'initial').execute
           end
 
-          expect_search_results(user, 'blobs', expected_count: expected_count, pending: pending?) do |user|
+          expect_search_results(user, 'blobs', expected_count: expected_count) do |user|
             described_class.new(user, group, search: '.gitmodules').execute
           end
 
-          expect_search_results(user, 'notes', expected_count: expected_count, pending: pending?) do |user|
+          expect_search_results(user, 'notes', expected_count: expected_count) do |user|
             described_class.new(user, group, search: note.note).execute
           end
         end

ee/spec/services/search/project_service_spec.rb

@@ -30,12 +30,6 @@ describe Search::ProjectService do
       let!(:merge_request2) { create :merge_request, target_project: project2, source_project: project2, title: merge_request.title }
       let!(:note) { create :note, project: project, noteable: merge_request }
       let!(:note2) { create :note, project: project2, noteable: merge_request2, note: note.note }
-      let(:pendings) do
-        [
-          { project_level: :public, feature_access_level: :enabled, membership: :guest, expected_count: 1 },
-          { project_level: :internal, feature_access_level: :enabled, membership: :guest, expected_count: 1 }
-        ]
-      end
 
       where(:project_level, :feature_access_level, :membership, :expected_count) do
         permission_table_for_reporter_feature_access
@@ -48,7 +42,7 @@ describe Search::ProjectService do
           end
           Gitlab::Elastic::Helper.refresh_index
 
-          expect_search_results(user, 'merge_requests', expected_count: expected_count, pending: pending?) do |user|
+          expect_search_results(user, 'merge_requests', expected_count: expected_count) do |user|
             described_class.new(project, user, search: merge_request.title).execute
           end
 
@@ -77,7 +71,7 @@ describe Search::ProjectService do
           end
           Gitlab::Elastic::Helper.refresh_index
 
-          expect_search_results(user, 'commits', expected_count: expected_count, pending: pending?) do |user|
+          expect_search_results(user, 'commits', expected_count: expected_count) do |user|
             described_class.new(project, user, search: 'initial').execute
           end