Merge branch 'es_fix_global_search' into 'security'

[ES] Fix internal data exposure

We need also specs for this

Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/1046

See merge request !504

app/models/concerns/elastic/application_search.rb

@@ -147,9 +147,13 @@ module Elastic
         }
       end
 
-      def project_ids_filter(query_hash, project_ids, public_and_internal_projects = true)
-        if project_ids
-          condition = project_ids_condition(project_ids, public_and_internal_projects)
+      def project_ids_filter(query_hash, options)
+        if options[:project_ids]
+          condition = project_ids_condition(
+            options[:current_user],
+            options[:project_ids],
+            options[:public_and_internal_projects]
+          )
 
           query_hash[:query][:bool][:filter] = {
             has_parent: {
@@ -166,19 +170,17 @@ module Elastic
         query_hash
       end
 
-      def project_ids_condition(project_ids, public_and_internal_projects)
+      def project_ids_condition(current_user, project_ids, public_and_internal_projects)
         conditions = [{
           terms: { id: project_ids }
         }]
 
         if public_and_internal_projects
-          conditions << {
-            term: { visibility_level: Project::PUBLIC }
-          }
+          conditions << { term: { visibility_level: Project::PUBLIC } }
 
-          conditions << {
-            term: { visibility_level: Project::INTERNAL }
-          }
+          if current_user
+            conditions << { term: { visibility_level: Project::INTERNAL } }
+          end
         end
 
         conditions

app/models/concerns/elastic/issues_search.rb

@@ -44,7 +44,7 @@ module Elastic
           query_hash = basic_query_hash(%w(title^2 description), query)
         end
 
-        query_hash = project_ids_filter(query_hash, options[:project_ids], options[:public_and_internal_projects])
+        query_hash = project_ids_filter(query_hash, options)
         query_hash = confidentiality_filter(query_hash, options[:current_user])
 
         self.__elasticsearch__.search(query_hash)

app/models/concerns/elastic/merge_requests_search.rb

@@ -66,7 +66,7 @@ module Elastic
           query_hash = basic_query_hash(%w(title^2 description), query)
         end
 
-        query_hash = project_ids_filter(query_hash, options[:project_ids], options[:public_and_internal_projects])
+        query_hash = project_ids_filter(query_hash, options)
 
         self.__elasticsearch__.search(query_hash)
       end

app/models/concerns/elastic/milestones_search.rb

@@ -31,7 +31,7 @@ module Elastic
 
         query_hash = basic_query_hash(options[:in], query)
 
-        query_hash = project_ids_filter(query_hash, options[:project_ids], options[:public_and_internal_projects])
+        query_hash = project_ids_filter(query_hash, options)
 
         self.__elasticsearch__.search(query_hash)
       end

app/models/concerns/elastic/notes_search.rb

@@ -60,7 +60,7 @@ module Elastic
           query_hash[:track_scores] = true
         end
 
-        query_hash = project_ids_filter(query_hash, options[:project_ids], options[:public_and_internal_projects])
+        query_hash = project_ids_filter(query_hash, options)
         query_hash = confidentiality_filter(query_hash, options[:current_user])
 
         query_hash[:sort] = [

app/models/concerns/elastic/projects_search.rb

@@ -83,10 +83,10 @@ module Elastic
           }
         end
 
-        if options[:pids]
+        if options[:project_ids]
           filters << {
             bool: {
-              should: project_ids_condition(options[:pids], options[:public_and_internal_projects])
+              should: project_ids_condition(options[:current_user], options[:project_ids], options[:public_and_internal_projects])
             }
           }
         end

lib/gitlab/elastic/search_results.rb

 module Gitlab
   module Elastic
     class SearchResults
-      attr_reader :current_user, :query
+      attr_reader :current_user, :query, :public_and_internal_projects
 
       # Limit search results by passed project ids
       # It allows us to search only for projects user has access to
@@ -59,41 +59,28 @@ module Gitlab
 
       private
 
-      def projects
-        opt = {
-          pids: limit_project_ids,
-          public_and_internal_projects: @public_and_internal_projects
+      def base_options
+        {
+          current_user: current_user,
+          project_ids: limit_project_ids,
+          public_and_internal_projects: public_and_internal_projects
         }
+      end
 
-        @projects = Project.elastic_search(query, options: opt)
+      def projects
+        Project.elastic_search(query, options: base_options)
       end
 
       def issues
-        opt = {
-          project_ids: limit_project_ids,
-          current_user: current_user,
-          public_and_internal_projects: @public_and_internal_projects
-        }
-
-        Issue.elastic_search(query, options: opt)
+        Issue.elastic_search(query, options: base_options)
       end
 
       def milestones
-        opt = {
-          project_ids: limit_project_ids,
-          public_and_internal_projects: @public_and_internal_projects
-        }
-
-        Milestone.elastic_search(query, options: opt)
+        Milestone.elastic_search(query, options: base_options)
       end
 
       def merge_requests
-        opt = {
-          project_ids: limit_project_ids,
-          public_and_internal_projects: @public_and_internal_projects
-        }
-
-        MergeRequest.elastic_search(query, options: opt)
+        MergeRequest.elastic_search(query, options: base_options)
       end
 
       def blobs
@@ -101,7 +88,7 @@ module Gitlab
           Kaminari.paginate_array([])
         else
           opt = {
-            additional_filter: build_filter_by_project(limit_project_ids, @public_and_internal_projects)
+            additional_filter: build_filter_by_project
           }
 
           Repository.search(
@@ -117,7 +104,7 @@ module Gitlab
           Kaminari.paginate_array([])
         else
           options = {
-            additional_filter: build_filter_by_project(limit_project_ids, @public_and_internal_projects)
+            additional_filter: build_filter_by_project
           }
 
           Repository.find_commits_by_message_with_elastic(
@@ -129,17 +116,15 @@ module Gitlab
         end
       end
 
-      def build_filter_by_project(project_ids, public_and_internal_projects)
-        conditions = [{ terms: { id: project_ids } }]
+      def build_filter_by_project
+        conditions = [{ terms: { id: limit_project_ids } }]
 
         if public_and_internal_projects
-          conditions << {
-            term: { visibility_level: Project::PUBLIC }
-          }
+          conditions << { term: { visibility_level: Project::PUBLIC } }
 
-          conditions << {
-            term: { visibility_level: Project::INTERNAL }
-          }
+          if current_user
+            conditions << { term: { visibility_level: Project::INTERNAL } }
+          end
         end
 
         {

spec/lib/gitlab/elastic/search_results_spec.rb

@@ -374,7 +374,7 @@ describe Gitlab::Elastic::SearchResults, lib: true do
       Gitlab::Elastic::Helper.refresh_index
     end
 
-    it 'founds blobs' do
+    it 'finds blobs' do
       results = described_class.new(user, 'def', limit_project_ids)
       blobs = results.objects('blobs')
 
@@ -382,7 +382,7 @@ describe Gitlab::Elastic::SearchResults, lib: true do
       expect(results.blobs_count).to eq 4
     end
 
-    it 'founds blobs from public projects only' do
+    it 'finds blobs from public projects only' do
       project_2 = create :project, :private
       project_2.repository.index_blobs
       Gitlab::Elastic::Helper.refresh_index
@@ -408,7 +408,7 @@ describe Gitlab::Elastic::SearchResults, lib: true do
       Gitlab::Elastic::Helper.refresh_index
     end
 
-    it 'founds commits' do
+    it 'finds commits' do
       results = described_class.new(user, 'add', limit_project_ids)
       commits = results.objects('commits')
 
@@ -416,7 +416,7 @@ describe Gitlab::Elastic::SearchResults, lib: true do
       expect(results.commits_count).to eq 5
     end
 
-    it 'founds commits from public projects only' do
+    it 'finds commits from public projects only' do
       project_2 = create :project, :private
       project_2.repository.index_commits
       Gitlab::Elastic::Helper.refresh_index
@@ -434,4 +434,190 @@ describe Gitlab::Elastic::SearchResults, lib: true do
       expect(results.commits_count).to eq 0
     end
   end
+
+  describe 'Visibility levels' do
+    let(:internal_project) { create(:project, :internal, description: "Internal project") }
+    let(:private_project1) { create(:project, :private, description: "Private project") }
+    let(:private_project2) { create(:project, :private, description: "Private project where I'm a member") }
+    let(:public_project) { create(:project, :public, description: "Public project") }
+    let(:limit_project_ids) { [private_project2.id] }
+
+    before do
+      private_project2.project_members.create(user: user, access_level: ProjectMember::DEVELOPER)
+    end
+
+    context 'Issues' do
+      it 'finds right set of issues' do
+        issue_1 = create :issue, project: internal_project, title: "Internal project"
+        create :issue, project: private_project1, title: "Private project"
+        issue_3 = create :issue, project: private_project2, title: "Private project where I'm a member"
+        issue_4 = create :issue, project: public_project, title: "Public project"
+
+        Gitlab::Elastic::Helper.refresh_index
+
+        # Authenticated search
+        results = described_class.new(user, 'project', limit_project_ids)
+        issues = results.objects('issues')
+
+        expect(issues).to include issue_1
+        expect(issues).to include issue_3
+        expect(issues).to include issue_4
+        expect(results.issues_count).to eq 3
+
+        # Unauthenticated search
+        results = described_class.new(nil, 'project', [])
+        issues = results.objects('issues')
+
+        expect(issues).to include issue_4
+        expect(results.issues_count).to eq 1
+      end
+    end
+
+    context 'Milestones' do
+      it 'finds right set of milestine' do
+        milestone_1 = create :milestone, project: internal_project, title: "Internal project"
+        create :milestone, project: private_project1, title: "Private project"
+        milestone_3 = create :milestone, project: private_project2, title: "Private project where I'm a member"
+        milestone_4 = create :milestone, project: public_project, title: "Public project"
+
+        Gitlab::Elastic::Helper.refresh_index
+
+        # Authenticated search
+        results = described_class.new(user, 'project', limit_project_ids)
+        milestones = results.objects('milestones')
+
+        expect(milestones).to include milestone_1
+        expect(milestones).to include milestone_3
+        expect(milestones).to include milestone_4
+        expect(results.milestones_count).to eq 3
+
+        # Unauthenticated search
+        results = described_class.new(nil, 'project', [])
+        milestones = results.objects('milestones')
+
+        expect(milestones).to include milestone_4
+        expect(results.milestones_count).to eq 1
+      end
+    end
+
+    context 'Projects' do
+      it 'finds right set of projects' do
+        internal_project
+        private_project1
+        private_project2
+        public_project
+
+        Gitlab::Elastic::Helper.refresh_index
+
+        # Authenticated search
+        results = described_class.new(user, 'project', limit_project_ids)
+        milestones = results.objects('projects')
+
+        expect(milestones).to include internal_project
+        expect(milestones).to include private_project2
+        expect(milestones).to include public_project
+        expect(results.projects_count).to eq 3
+
+        # Unauthenticated search
+        results = described_class.new(nil, 'project', [])
+        projects = results.objects('projects')
+
+        expect(projects).to include public_project
+        expect(results.projects_count).to eq 1
+      end
+    end
+
+    context 'Merge Requests' do
+      it 'finds right set of merge requests' do
+        merge_request_1 = create :merge_request, target_project: internal_project, source_project: internal_project, title: "Internal project"
+        create :merge_request, target_project: private_project1, source_project: private_project1, title: "Private project"
+        merge_request_3 = create :merge_request, target_project: private_project2, source_project: private_project2, title: "Private project where I'm a member"
+        merge_request_4 = create :merge_request, target_project: public_project, source_project: public_project, title: "Public project"
+
+        Gitlab::Elastic::Helper.refresh_index
+
+        # Authenticated search
+        results = described_class.new(user, 'project', limit_project_ids)
+        merge_requests = results.objects('merge_requests')
+
+        expect(merge_requests).to include merge_request_1
+        expect(merge_requests).to include merge_request_3
+        expect(merge_requests).to include merge_request_4
+        expect(results.merge_requests_count).to eq 3
+
+        # Unauthenticated search
+        results = described_class.new(nil, 'project', [])
+        merge_requests = results.objects('merge_requests')
+
+        expect(merge_requests).to include merge_request_4
+        expect(results.merge_requests_count).to eq 1
+      end
+    end
+
+    context 'Commits' do
+      it 'finds right set of commits' do
+        [internal_project, private_project1, private_project2, public_project].each do |project|
+          project.repository.commit_file(
+            user,
+            'test-file',
+            'search test',
+            'search test',
+            'master',
+            false
+          )
+
+          project.repository.index_commits
+        end
+
+        Gitlab::Elastic::Helper.refresh_index
+
+        # Authenticated search
+        results = described_class.new(user, 'search', limit_project_ids)
+        commits = results.objects('commits')
+
+        expect(commits.map(&:project)).to match_array [internal_project, private_project2, public_project]
+        expect(results.commits_count).to eq 3
+
+        # Unauthenticated search
+        results = described_class.new(nil, 'search', [])
+        commits = results.objects('commits')
+
+        expect(commits.first.project).to eq public_project
+        expect(results.commits_count).to eq 1
+      end
+    end
+
+    context 'Blobs' do
+      it 'finds right set of blobs' do
+        [internal_project, private_project1, private_project2, public_project].each do |project|
+          project.repository.commit_file(
+            user,
+            'test-file',
+            'tesla',
+            'search test',
+            'master',
+            false
+          )
+
+          project.repository.index_blobs
+        end
+
+        Gitlab::Elastic::Helper.refresh_index
+
+        # Authenticated search
+        results = described_class.new(user, 'tesla', limit_project_ids)
+        blobs = results.objects('blobs')
+
+        expect(blobs.map{ |blob| blob._parent.to_i }).to match_array [internal_project.id, private_project2.id, public_project.id]
+        expect(results.blobs_count).to eq 3
+
+        # Unauthenticated search
+        results = described_class.new(nil, 'tesla', [])
+        blobs = results.objects('blobs')
+
+        expect(blobs.first._parent.to_i).to eq public_project.id.to_i
+        expect(results.blobs_count).to eq 1
+      end
+    end
+  end
 end

spec/models/concerns/elastic/project_spec.rb

@@ -24,9 +24,9 @@ describe Project, elastic: true do
       Gitlab::Elastic::Helper.refresh_index
     end
 
-    expect(described_class.elastic_search('test', options: { pids: project_ids }).total_count).to eq(1)
-    expect(described_class.elastic_search('test1', options: { pids: project_ids }).total_count).to eq(1)
-    expect(described_class.elastic_search('someone_elses_project', options: { pids: project_ids }).total_count).to eq(0)
+    expect(described_class.elastic_search('test', options: { project_ids: project_ids }).total_count).to eq(1)
+    expect(described_class.elastic_search('test1', options: { project_ids: project_ids }).total_count).to eq(1)
+    expect(described_class.elastic_search('someone_elses_project', options: { project_ids: project_ids }).total_count).to eq(0)
   end
 
   it "finds partial matches in project names" do
@@ -40,7 +40,7 @@ describe Project, elastic: true do
       Gitlab::Elastic::Helper.refresh_index
     end
 
-    expect(described_class.elastic_search('tesla', options: { pids: project_ids }).total_count).to eq(2)
+    expect(described_class.elastic_search('tesla', options: { project_ids: project_ids }).total_count).to eq(2)
   end
 
   it "returns json with all needed elements" do