Fix Gitlab::Elastic::SearchResults

lib/elastic/application_search.rb

@@ -113,7 +113,9 @@ module Elastic
       def project_ids_filter(query_hash, project_ids)
         if project_ids
           query_hash[:query][:filtered][:filter] = {
-            and: [ { terms: { project_id: project_ids } } ]
+            bool: {
+              must: [ { terms: { project_id: project_ids } } ]
+            }
           }
         end
 

lib/elastic/issues_search.rb

@@ -34,7 +34,7 @@ module Elastic
 
         # We don't use as_json(only: ...) because it calls all virtual and serialized attributtes
         # https://gitlab.com/gitlab-org/gitlab-ee/issues/349
-        [:id, :iid, :title, :description, :created_at, :updated_at, :state, :project_id, :author_id].each do |attr|
+        [:id, :iid, :title, :description, :created_at, :updated_at, :state, :project_id, :author_id, :assignee_id, :confidential].each do |attr|
           data[attr.to_s] = self.send(attr)
         end
 
@@ -52,36 +52,41 @@ module Elastic
         end
 
         query_hash = project_ids_filter(query_hash, options[:project_ids])
-        query_hash = confidentiality_filter(query_hash, options[:user])
+        query_hash = confidentiality_filter(query_hash, options[:current_user])
 
         self.__elasticsearch__.search(query_hash)
       end
 
-      def self.confidentiality_filter(query_hash, user)
-        return query_hash if user.blank? || user.admin?
-
-        query_hash[:query][:filtered][:filter] = {
-          bool: {
-            should: [
-              { term: { confidential: false } },
-              { bool: {
-                  must: [
-                    { term: { confidential: true } },
-                    { bool: {
-                        should: [
-                          { term: { author_id: user.id } },
-                          { term: { assignee_id: user.id } },
-                          { terms: { project_id: user.authorized_projects.pluck(:id) } }
-                        ]
-                      }
-                    }
-                  ]
-                }
-              }
-            ]
-          }
-        }
-
+      def self.confidentiality_filter(query_hash, current_user)
+        return query_hash if current_user.present? && current_user.admin?
+
+        filter = if current_user.present?
+                   {
+                     bool: {
+                       should: [
+                         { term: { confidential: false } },
+                         { bool: {
+                             must: [
+                               { term: { confidential: true } },
+                               { bool: {
+                                   should: [
+                                     { term: { author_id: current_user.id } },
+                                     { term: { assignee_id: current_user.id } },
+                                     { terms: { project_id: current_user.authorized_projects.pluck(:id) } }
+                                   ]
+                                 }
+                               }
+                             ]
+                           }
+                         }
+                       ]
+                     }
+                   }
+                 else
+                   { term: { confidential: false } }
+                 end
+
+        query_hash[:query][:filtered][:filter][:bool][:must] << filter
         query_hash
       end
     end

lib/gitlab/elastic/project_search_results.rb

@@ -3,8 +3,8 @@ module Gitlab
     class ProjectSearchResults < SearchResults
       attr_reader :project, :repository_ref
 
-      def initialize(user, project_id, query, repository_ref = nil)
-        @user = user
+      def initialize(current_user, project_id, query, repository_ref = nil)
+        @current_user = current_user
         @project = Project.find(project_id)
 
         @repository_ref = if repository_ref.present?

lib/gitlab/elastic/search_results.rb

 module Gitlab
   module Elastic
     class SearchResults
-      attr_reader :user, :query
+      attr_reader :current_user, :query
 
       # Limit search results by passed project ids
       # It allows us to search only for projects user has access to
       attr_reader :limit_project_ids
 
-      def initialize(user, limit_project_ids, query)
-        @user = user
+      def initialize(current_user, limit_project_ids, query)
+        @current_user = current_user
         @limit_project_ids = limit_project_ids || Project.all
         @query = Shellwords.shellescape(query) if query.present?
       end
@@ -64,8 +64,8 @@ module Gitlab
 
       def issues
         opt = {
-          projects_ids: limit_project_ids,
-          user: user
+          project_ids: limit_project_ids,
+          current_user: current_user
         }
 
         Issue.elastic_search(query, options: opt)

spec/lib/elastic/issue_spec.rb

@@ -33,7 +33,8 @@ describe "Issue", elastic: true do
     issue = create :issue, project: project
 
     expected_hash =  issue.attributes.extract!('id', 'iid', 'title', 'description', 'created_at',
-                                                'updated_at', 'state', 'project_id', 'author_id')
+                                                'updated_at', 'state', 'project_id', 'author_id',
+                                                'assignee_id', 'confidential')
 
     expected_hash['project'] = { "id" => project.id }
     expected_hash['author'] = { "id" => issue.author_id }

spec/lib/gitlab/elastic/search_results_spec.rb

@@ -103,6 +103,19 @@ describe Gitlab::Elastic::SearchResults, lib: true do
     context 'search by term' do
       let(:query) { 'issue' }
 
+      it 'should not list confidential issues for guests' do
+        results = described_class.new(nil, limit_project_ids, query)
+        issues = results.objects('issues')
+
+        expect(issues).to include issue
+        expect(issues).not_to include security_issue_1
+        expect(issues).not_to include security_issue_2
+        expect(issues).not_to include security_issue_3
+        expect(issues).not_to include security_issue_4
+        expect(issues).not_to include security_issue_5
+        expect(results.issues_count).to eq 1
+      end
+
       it 'should not list confidential issues for non project members' do
         results = described_class.new(non_member, limit_project_ids, query)
         issues = results.objects('issues')
@@ -175,6 +188,19 @@ describe Gitlab::Elastic::SearchResults, lib: true do
     context 'search by iid' do
       let(:query) { '#1' }
 
+      it 'should not list confidential issues for guests' do
+        results = described_class.new(nil, limit_project_ids, query)
+        issues = results.objects('issues')
+
+        expect(issues).to include issue
+        expect(issues).not_to include security_issue_1
+        expect(issues).not_to include security_issue_2
+        expect(issues).not_to include security_issue_3
+        expect(issues).not_to include security_issue_4
+        expect(issues).not_to include security_issue_5
+        expect(results.issues_count).to eq 1
+      end
+
       it 'should not list confidential issues for non project members' do
         results = described_class.new(non_member, limit_project_ids, query)
         issues = results.objects('issues')
@@ -257,8 +283,8 @@ describe Gitlab::Elastic::SearchResults, lib: true do
         iid: 1
       )
       @merge_request_2 = create(
-        :merge_request, 
-        :conflict, 
+        :merge_request,
+        :conflict,
         source_project: project_1,
         target_project: project_1,
         title: 'Merge Request 2',
@@ -315,7 +341,6 @@ describe Gitlab::Elastic::SearchResults, lib: true do
     end
   end
 
-
   describe 'project scoping' do
     before do
       [Project, MergeRequest, Issue, Milestone].each do |model|
@@ -356,7 +381,7 @@ describe Gitlab::Elastic::SearchResults, lib: true do
         model.__elasticsearch__.refresh_index!
       end
 
-      result = Gitlab::Elastic::SearchResults.new([project.id], 'term')
+      result = Gitlab::Elastic::SearchResults.new(user, [project.id], 'term')
 
       expect(result.issues_count).to eq(2)
       expect(result.merge_requests_count).to eq(2)