Merge branch '2338-fix-elasticsearch-private-project-search' into 'master'

Fix a bug searching private projects with ES as an admin or auditor

Closes #2338

See merge request !2613

app/models/concerns/elastic/application_search.rb

@@ -157,6 +157,8 @@ module Elastic
         }
       end
 
+      # Builds an elasticsearch query that will select child documents from a
+      # set of projects, taking user access rules into account.
       def project_ids_filter(query_hash, options)
         project_query = project_ids_query(
           options[:current_user],
@@ -178,53 +180,82 @@ module Elastic
         query_hash
       end
 
-      def project_ids_query(current_user, project_ids, public_and_internal_projects, feature = nil)
-        conditions = []
-        limit_private_projects = {}
+      # Builds an elasticsearch query that will select projects the user is
+      # granted access to.
+      #
+      # If a project feature is specified, it indicates interest in child
+      # documents gated by that project feature - e.g., "issues". The feature's
+      # visibility level must be taken into account.
+      def project_ids_query(user, project_ids, public_and_internal_projects, feature = nil)
+        # At least one condition must be present, so pick no projects for
+        # anonymous users.
+        # Pick private, internal and public projects the user is a member of.
+        # Pick all private projects for admins & auditors.
+        conditions = [pick_projects_by_membership(project_ids, feature)]
 
-        if project_ids != :any
-          limit_private_projects[:filter] = { terms: { id: project_ids } }
-        end
+        if public_and_internal_projects
+          # Skip internal projects for anonymous and external users.
+          # Others are given access to all internal projects. Admins & auditors
+          # get access to internal projects where the feature is private.
+          conditions << pick_projects_by_visibility(Project::INTERNAL, user, feature) if user && !user.external?
 
-        if feature
-          limit_private_projects[:must_not] = {
-            term: { "#{feature}_access_level" => ProjectFeature::DISABLED }
-          }
+          # All users, including anonymous, can access public projects.
+          # Admins & auditors get access to public projects where the feature is
+          # private.
+          conditions << pick_projects_by_visibility(Project::PUBLIC, user, feature)
         end
 
-        conditions << { bool: limit_private_projects } unless limit_private_projects.empty?
+        { should: conditions }
+      end
 
-        if public_and_internal_projects
-          conditions << if feature
-                          {
-                            bool: {
-                              filter: [
-                                { term: { visibility_level: Project::PUBLIC } },
-                                { term: { "#{feature}_access_level" => ProjectFeature::ENABLED } }
-                              ]
-                            }
-                          }
-                        else
-                          { term: { visibility_level: Project::PUBLIC } }
-                        end
-
-          if current_user && !current_user.external?
-            conditions << if feature
-                            {
-                              bool: {
-                                filter: [
-                                  { term: { visibility_level: Project::INTERNAL } },
-                                  { term: { "#{feature}_access_level" => ProjectFeature::ENABLED } }
-                                ]
-                              }
-                            }
-                          else
-                            { term: { visibility_level: Project::INTERNAL } }
-                          end
+      private
+
+      # Most users come with a list of projects they are members of, which may
+      # be a mix of public, internal or private. Grant access to them all, as
+      # long as the project feature is not disabled.
+      #
+      # Admins & auditors are given access to all private projects. Access to
+      # internal or public projects where the project feature is private is not
+      # granted here.
+      def pick_projects_by_membership(project_ids, feature = nil)
+        condition =
+          if project_ids == :any
+            { term: { visibility_level: Project::PRIVATE } }
+          else
+            { terms: { id: project_ids } }
           end
-        end
 
-        { should: conditions }
+        limit_by_feature(condition, feature, include_members_only: true)
+      end
+
+      # Grant access to projects of the specified visibility level to the user.
+      #
+      # If a project feature is specified, access is only granted if the feature
+      # is enabled or, for admins & auditors, private.
+      def pick_projects_by_visibility(visibility, user, feature)
+        condition = { term: { visibility_level: visibility } }
+
+        limit_by_feature(condition, feature, include_members_only: user&.full_private_access?)
+      end
+
+      # If a project feature is specified, access is dependent on its visibility
+      # level being enabled (or private if `include_members_only: true`).
+      #
+      # This method is a no-op if no project feature is specified.
+      #
+      # Always denies access to projects when the feature is disabled - even to
+      # admins & auditors - as stale child documents may be present.
+      def limit_by_feature(condition, feature, include_members_only:)
+        return condition unless 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] } }
       end
     end
   end

changelogs/unreleased-ee/2338-fix-elasticsearch-private-project-search.yml

+---
+title: Fix a bug searching private projects with Elasticsearch as an admin or auditor
+merge_request: 2613
+author:

spec/elastic_integration/global_search_spec.rb

 require 'spec_helper'
 
 describe 'GlobalSearch' do
-  let(:features) { %i(issues merge_requests repository builds wiki) }
+  let(:features) { %i(issues merge_requests repository builds wiki snippets) }
   let(:admin) { create :user, admin: true }
   let(:auditor) {create :user, auditor: true }
   let(:non_member) { create :user }
@@ -151,7 +151,8 @@ describe 'GlobalSearch' do
       create :merge_request, title: 'term', target_project: project, source_project: project
       project.wiki.create_page('index_page', 'term')
 
-      project.project_feature.update!(feature_settings) if feature_settings
+      # Going through the project ensures its elasticsearch document is updated
+      project.update!(project_feature_attributes: feature_settings) if feature_settings
 
       project.repository.index_blobs
       project.repository.index_commits

spec/models/concerns/elastic/issue_spec.rb

@@ -19,8 +19,8 @@ describe Issue, elastic: true do
       create :issue, description: 'bla-bla term2', project: project
       create :issue, project: project
 
-      # The issue I have no access to
-      create :issue, title: 'bla-bla term3'
+      # The issue I have no access to except as an administrator
+      create :issue, title: 'bla-bla term3', project: create(:project, :private)
 
       Gitlab::Elastic::Helper.refresh_index
     end
@@ -29,6 +29,7 @@ describe Issue, elastic: true do
 
     expect(described_class.elastic_search('(term1 | term2 | term3) +bla-bla', options: options).total_count).to eq(2)
     expect(described_class.elastic_search(Issue.last.to_reference, options: options).total_count).to eq(1)
+    expect(described_class.elastic_search('bla-bla', options: { project_ids: :any }).total_count).to eq(3)
   end
 
   it "returns json with all needed elements" do

spec/models/concerns/elastic/merge_request_spec.rb

@@ -19,8 +19,8 @@ describe MergeRequest, elastic: true do
       create :merge_request, description: 'term2 in description', source_project: project, target_branch: "feature2"
       create :merge_request, source_project: project, target_branch: "feature3"
 
-      # The merge request you have no access to
-      create :merge_request, title: 'also with term3'
+      # The merge request you have no access to except as an administrator
+      create :merge_request, title: 'also with term3', source_project: create(:project, :private)
 
       Gitlab::Elastic::Helper.refresh_index
     end
@@ -29,6 +29,8 @@ describe MergeRequest, elastic: true do
 
     expect(described_class.elastic_search('term1 | term2 | term3', options: options).total_count).to eq(2)
     expect(described_class.elastic_search(MergeRequest.last.to_reference, options: options).total_count).to eq(1)
+    expect(described_class.elastic_search('term3', options: options).total_count).to eq(0)
+    expect(described_class.elastic_search('term3', options: { project_ids: :any }).total_count).to eq(1)
   end
 
   it "returns json with all needed elements" do

spec/models/concerns/elastic/milestone_spec.rb

@@ -19,7 +19,7 @@ describe Milestone, elastic: true do
       create :milestone, description: 'bla-bla term2', project: project
       create :milestone, project: project
 
-      # The milestone you have no access to
+      # The milestone you have no access to except as an administrator
       create :milestone, title: 'bla-bla term3'
 
       Gitlab::Elastic::Helper.refresh_index
@@ -28,6 +28,7 @@ describe Milestone, elastic: true do
     options = { project_ids: [project.id] }
 
     expect(described_class.elastic_search('(term1 | term2 | term3) +bla-bla', options: options).total_count).to eq(2)
+    expect(described_class.elastic_search('bla-bla', options: { project_ids: :any }).total_count).to eq(3)
   end
 
   it "returns json with all needed elements" do

spec/models/concerns/elastic/note_spec.rb

@@ -18,7 +18,7 @@ describe Note, elastic: true do
       create :note, note: 'bla-bla term1', project: issue.project
       create :note, project: issue.project
 
-      # The note in the project you have no access to
+      # The note in the project you have no access to except as an administrator
       create :note, note: 'bla-bla term2'
 
       Gitlab::Elastic::Helper.refresh_index
@@ -27,6 +27,8 @@ describe Note, elastic: true do
     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)
   end
 
   it "indexes && searches diff notes" do

spec/models/concerns/elastic/project_spec.rb

@@ -21,6 +21,9 @@ describe Project, elastic: true do
       create :project, path: 'someone_elses_project'
       project_ids += [project.id, project1.id, project2.id]
 
+      # The project you have no access to except as an administrator
+      create :project, :private, name: 'test3'
+
       Gitlab::Elastic::Helper.refresh_index
     end
 
@@ -28,6 +31,7 @@ describe Project, elastic: true do
     expect(described_class.elastic_search('test2', options: { project_ids: project_ids }).total_count).to eq(1)
     expect(described_class.elastic_search('awesome', options: { project_ids: project_ids }).total_count).to eq(1)
     expect(described_class.elastic_search('test*', options: { project_ids: project_ids }).total_count).to eq(2)
+    expect(described_class.elastic_search('test*', options: { project_ids: :any }).total_count).to eq(3)
     expect(described_class.elastic_search('someone_elses_project', options: { project_ids: project_ids }).total_count).to eq(0)
   end