Merge branch 'elastic-security' into 'security'

Rework ES permissions check

See merge request !507

app/models/concerns/elastic/application_search.rb

@@ -158,11 +158,11 @@ module Elastic
       end
 
       def project_ids_filter(query_hash, options)
-        if options[:project_ids]
-          condition = project_ids_condition(
+        project_query = project_ids_query(
           options[:current_user],
           options[:project_ids],
-            options[:public_and_internal_projects]
+          options[:public_and_internal_projects],
+          options[:feature]
         )
 
         query_hash[:query][:bool][:filter] ||= []
@@ -170,31 +170,64 @@ module Elastic
           has_parent: {
             parent_type: "project",
             query: {
-                bool: {
-                  should: condition
-                }
+              bool: project_query
             }
           }
         }
-        end
 
         query_hash
       end
 
-      def project_ids_condition(current_user, project_ids, public_and_internal_projects)
-        conditions = [{
+      def project_ids_query(current_user, project_ids, public_and_internal_projects, feature = nil)
+        conditions = []
+
+        private_project_condition = {
+          bool: {
+            filter: {
               terms: { id: project_ids }
-        }]
+            }
+          }
+        }
+
+        if feature
+          private_project_condition[:bool][:must_not] = {
+            term: { "#{feature}_access_level" => ProjectFeature::DISABLED }
+          }
+        end
+
+        conditions << private_project_condition
 
         if public_and_internal_projects
-          conditions << { term: { visibility_level: Project::PUBLIC } }
+          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
-            conditions << { term: { visibility_level: Project::INTERNAL } }
+            conditions << if feature
+                            {
+                              bool: {
+                                filter: [
+                                  { term: { visibility_level: Project::INTERNAL } },
+                                  { term: { "#{feature}_access_level" => ProjectFeature::ENABLED } }
+                                ]
+                              }
+                            }
+                          else
+                            { term: { visibility_level: Project::INTERNAL } }
+                          end
           end
         end
 
-        conditions
+        { should: conditions }
       end
     end
   end

app/models/concerns/elastic/issues_search.rb

@@ -45,6 +45,7 @@ module Elastic
             basic_query_hash(%w(title^2 description), query)
           end
 
+        options[:feature] = 'issues'
         query_hash = project_ids_filter(query_hash, options)
         query_hash = confidentiality_filter(query_hash, options[:current_user])
 

app/models/concerns/elastic/merge_requests_search.rb

@@ -67,6 +67,7 @@ module Elastic
             basic_query_hash(%w(title^2 description), query)
           end
 
+        options[:feature] = 'merge_requests'
         query_hash = project_ids_filter(query_hash, options)
 
         self.__elasticsearch__.search(query_hash)

app/models/concerns/elastic/notes_search.rb

@@ -18,6 +18,9 @@ module Elastic
           indexes :author_id, type: :integer
           indexes :confidential, type: :boolean
         end
+
+        indexes :noteable_type,  type: :string, index: :not_analyzed
+        indexes :noteable_id,    type: :integer, index: :not_analyzed
       end
 
       def as_indexed_json(options = {})
@@ -25,7 +28,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, :note, :project_id, :created_at, :updated_at].each do |attr|
+        [:id, :note, :project_id, :noteable_type, :noteable_id, :created_at, :updated_at].each do |attr|
           data[attr.to_s] = safely_read_attribute_for_elasticsearch(attr)
         end
 
@@ -55,11 +58,6 @@ module Elastic
           }
         }
 
-        if query.blank?
-          query_hash[:query][:bool][:must] = [{ match_all: {} }]
-          query_hash[:track_scores] = true
-        end
-
         query_hash = project_ids_filter(query_hash, options)
         query_hash = confidentiality_filter(query_hash, options[:current_user])
 

app/models/concerns/elastic/projects_search.rb

@@ -2,6 +2,14 @@ module Elastic
   module ProjectsSearch
     extend ActiveSupport::Concern
 
+    TRACKED_FEATURE_SETTINGS = %w(
+      issues_access_level
+      merge_requests_access_level
+      snippets_access_level
+      wiki_access_level
+      repository_access_level
+    )
+
     included do
       include ApplicationSearch
 
@@ -22,7 +30,14 @@ module Elastic
         indexes :created_at,          type: :date
         indexes :updated_at,          type: :date
         indexes :archived,            type: :boolean
+
         indexes :visibility_level,            type: :integer
+        indexes :issues_access_level,         type: :integer
+        indexes :merge_requests_access_level, type: :integer
+        indexes :snippets_access_level,       type: :integer
+        indexes :wiki_access_level,           type: :integer
+        indexes :repository_access_level,     type: :integer
+
         indexes :last_activity_at,    type: :date
         indexes :last_pushed_at,      type: :date
       end
@@ -49,6 +64,10 @@ module Elastic
           data[attr.to_s] = safely_read_attribute_for_elasticsearch(attr)
         end
 
+        TRACKED_FEATURE_SETTINGS.each do |feature|
+          data[feature] = project_feature.public_send(feature)
+        end
+
         data
       end
 
@@ -85,9 +104,7 @@ module Elastic
 
         if options[:project_ids]
           filters << {
-            bool: {
-              should: project_ids_condition(options[:current_user], options[:project_ids], options[:public_and_internal_projects])
-            }
+            bool: project_ids_query(options[:current_user], options[:project_ids], options[:public_and_internal_projects])
           }
         end
 

app/models/concerns/elastic/snippets_search.rb

@@ -79,14 +79,23 @@ module Elastic
                    {
                      bool: {
                        should: [
-                         { terms: { visibility_level: [Snippet::PUBLIC, Snippet::INTERNAL] } },
                          { term: { author_id: user.id } },
                          { terms: { project_id: user.authorized_projects.pluck(:id) } },
+                         { bool: {
+                             filter: { terms: { visibility_level: [Snippet::PUBLIC, Snippet::INTERNAL] } },
+                             must_not: { exists: { field: 'project_id' } }
+                           }
+                         }
                        ]
                      }
                    }
                  else
-                   { term: { visibility_level: Snippet::PUBLIC } }
+                   {
+                     bool: {
+                       filter: { term: { visibility_level: Snippet::PUBLIC } },
+                       must_not: { exists: { field: 'project_id' } }
+                     }
+                   }
                  end
 
         query_hash[:query][:bool][:filter] = filter

app/models/project_feature.rb

@@ -43,6 +43,12 @@ class ProjectFeature < ActiveRecord::Base
   default_value_for :wiki_access_level,           value: ENABLED, allows_nil: false
   default_value_for :repository_access_level,     value: ENABLED, allows_nil: false
 
+  after_commit on: :update do
+    if current_application_settings.elasticsearch_indexing?
+      ElasticIndexerWorker.perform_async(:update, 'Project', project_id)
+    end
+  end
+
   def feature_available?(feature, user)
     access_level = public_send(ProjectFeature.access_level_attribute(feature))
     get_permission(user, access_level)

changelogs/unreleased-ee/elastic-security.yml

+---
+title: 'Elastic security fix: Respect feature visibility level'
+merge_request:
+author:

lib/gitlab/elastic/search_results.rb

@@ -127,7 +127,8 @@ module Gitlab
       end
 
       def merge_requests
-        MergeRequest.elastic_search(query, options: base_options)
+        options = base_options.merge(project_ids: non_guest_project_ids)
+        MergeRequest.elastic_search(query, options: options)
       end
 
       def blobs
@@ -135,7 +136,7 @@ module Gitlab
           Kaminari.paginate_array([])
         else
           opt = {
-            additional_filter: build_filter_by_project
+            additional_filter: repository_filter
           }
 
           Repository.search(
@@ -151,7 +152,7 @@ module Gitlab
           Kaminari.paginate_array([])
         else
           options = {
-            additional_filter: build_filter_by_project
+            additional_filter: repository_filter
           }
 
           Repository.find_commits_by_message_with_elastic(
@@ -163,14 +164,28 @@ module Gitlab
         end
       end
 
-      def build_filter_by_project
-        conditions = [{ terms: { id: limit_project_ids } }]
+      def repository_filter
+        conditions = [{ terms: { id: non_guest_project_ids } }]
 
         if public_and_internal_projects
-          conditions << { term: { visibility_level: Project::PUBLIC } }
+          conditions << {
+                          bool: {
+                            filter: [
+                              { term: { visibility_level: Project::PUBLIC } },
+                              { term: { repository_access_level: ProjectFeature::ENABLED } }
+                            ]
+                          }
+                        }
 
           if current_user
-            conditions << { term: { visibility_level: Project::INTERNAL } }
+            conditions << {
+                            bool: {
+                              filter: [
+                                { term: { visibility_level: Project::INTERNAL } },
+                                { term: { repository_access_level: ProjectFeature::ENABLED } }
+                              ]
+                            }
+                          }
           end
         end
 
@@ -179,13 +194,28 @@ module Gitlab
             parent_type: 'project',
             query: {
               bool: {
-                should: conditions
+                should: conditions,
+                must_not: { term: { repository_access_level: ProjectFeature::DISABLED } }
               }
             }
           }
         }
       end
 
+      def guest_project_ids
+        if current_user
+          current_user.authorized_projects.
+            where('project_authorizations.access_level = ?', Gitlab::Access::GUEST).
+            pluck(:id)
+        else
+          []
+        end
+      end
+
+      def non_guest_project_ids
+        @non_guest_project_ids ||= limit_project_ids - guest_project_ids
+      end
+
       def default_scope
         'projects'
       end

lib/tasks/gitlab/elastic.rake

@@ -114,6 +114,61 @@ namespace :gitlab do
       puts "Index recreated".color(:green)
     end
 
+    desc "GitLab | Elasticsearch | Add feature access levels to project"
+    task add_feature_visibility_levels_to_project: :environment do
+      client = Project.__elasticsearch__.client
+
+      #### Check if this task has already been run ####
+      mapping = client.indices.get(index: Project.index_name)
+      project_fields = mapping['gitlab-development']['mappings']['project']['properties'].keys
+
+      if project_fields.include?('issues_access_level')
+        puts 'Index mapping is already up to date'.color(:yellow)
+        exit
+      end
+      ####
+
+      project_fields = {
+        properties: {
+          issues_access_level: {
+              type: :integer
+          },
+          merge_requests_access_level: {
+              type: :integer
+          },
+          snippets_access_level: {
+              type: :integer
+          },
+          wiki_access_level: {
+              type: :integer
+          },
+          repository_access_level: {
+              type: :integer
+          }
+        }
+      }
+
+      note_fields = {
+        properties: {
+          noteable_type: {
+            type: :string,
+            index: :not_analyzed
+          },
+          noteable_id: {
+            type: :integer
+          }
+        }
+      }
+
+      client.indices.put_mapping(index: Project.index_name, type: :project, body: project_fields)
+      client.indices.put_mapping(index: Project.index_name, type: :note, body: note_fields)
+
+      Project.__elasticsearch__.import
+      Note.searchable.import_with_parent
+
+      puts "Done".color(:green)
+    end
+
     def batch_size
       ENV.fetch('BATCH', 300).to_i
     end

spec/elastic_integration/global_search_spec.rb

+require 'spec_helper'
+
+describe 'GlobalSearch' do
+  let(:features) { %i(issues merge_requests repository builds) }
+  let(:non_member) { create :user }
+  let(:member) { create :user }
+  let(:guest) { create :user }
+
+  before do
+    stub_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
+    Gitlab::Elastic::Helper.create_empty_index
+
+    project.team << [member, :developer]
+    project.team << [guest, :guest]
+  end
+
+  after do
+    Gitlab::Elastic::Helper.delete_index
+    stub_application_setting(elasticsearch_search: false, elasticsearch_indexing: false)
+  end
+
+  context "Respect feature visibility levels" do
+    context "Private projects" do
+      let(:project) { create(:project, :private) }
+
+      # The feature can be disabled but the data may actually exist
+      it "does not find items if features are disabled" do
+        create_items(project, feature_settings(:disabled))
+
+        expect_no_items_to_be_found(member)
+        expect_no_items_to_be_found(guest)
+        expect_no_items_to_be_found(non_member)
+        expect_no_items_to_be_found(nil)
+      end
+
+      it "shows items to member only if features are enabled" do
+        create_items(project, feature_settings(:enabled))
+
+        expect_items_to_be_found(member)
+        expect_non_code_items_to_be_found(guest)
+        expect_no_items_to_be_found(non_member)
+        expect_no_items_to_be_found(nil)
+      end
+    end
+
+    context "Internal projects" do
+      let(:project) { create(:project, :internal) }
+
+      # The feature can be disabled but the data may actually exist
+      it "does not find items if features are disabled" do
+        create_items(project, feature_settings(:disabled))
+
+        expect_no_items_to_be_found(member)
+        expect_no_items_to_be_found(guest)
+        expect_no_items_to_be_found(non_member)
+        expect_no_items_to_be_found(nil)
+      end
+
+      it "shows items to member only if features are enabled" do
+        create_items(project, feature_settings(:enabled))
+
+        expect_items_to_be_found(member)
+        expect_items_to_be_found(guest)
+        expect_items_to_be_found(non_member)
+        expect_no_items_to_be_found(nil)
+      end
+
+      it "shows items to member only if features are private" do
+        create_items(project, feature_settings(:private))
+
+        expect_items_to_be_found(member)
+        expect_non_code_items_to_be_found(guest)
+        expect_no_items_to_be_found(non_member)
+        expect_no_items_to_be_found(nil)
+      end
+    end
+
+    context "Public projects" do
+      let(:project) { create(:project, :public) }
+
+      # The feature can be disabled but the data may actually exist
+      it "does not find items if features are disabled" do
+        create_items(project, feature_settings(:disabled))
+
+        expect_no_items_to_be_found(member)
+        expect_no_items_to_be_found(guest)
+        expect_no_items_to_be_found(non_member)
+        expect_no_items_to_be_found(nil)
+      end
+
+      it "finds items if features are enabled" do
+        create_items(project, feature_settings(:enabled))
+
+        expect_items_to_be_found(member)
+        expect_items_to_be_found(guest)
+        expect_items_to_be_found(non_member)
+        expect_items_to_be_found(nil)
+      end
+
+      it "shows items to member only if features are private" do
+        create_items(project, feature_settings(:private))
+
+        expect_items_to_be_found(member)
+        expect_non_code_items_to_be_found(guest)
+        expect_no_items_to_be_found(non_member)
+        expect_no_items_to_be_found(nil)
+      end
+    end
+  end
+
+  def create_items(project, feature_settings = nil)
+    Sidekiq::Testing.inline! do
+      project.project_feature.update!(feature_settings) if feature_settings
+
+      create :issue, title: 'term', project: project
+      create :merge_request, title: 'term', target_project: project, source_project: project
+      project.repository.index_blobs
+      project.repository.index_commits
+
+      Gitlab::Elastic::Helper.refresh_index
+    end
+  end
+
+  # access_level can be :disabled, :enabled or :private
+  def feature_settings(access_level)
+    Hash[features.collect { |k| ["#{k}_access_level", ProjectFeature.const_get(access_level.to_s.upcase)] }]
+  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(search(user, 'def').blobs_count).to eq(0)
+    expect(search(user, 'add').commits_count).to eq(0)
+  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(search(user, 'def').blobs_count).not_to eq(0)
+    expect(search(user, 'add').commits_count).not_to eq(0)
+  end
+
+  def expect_non_code_items_to_be_found(user)
+    results = search(guest, 'term')
+    expect(results.issues_count).not_to eq(0)
+    expect(results.merge_requests_count).to eq(0)
+    expect(search(guest, 'def').blobs_count).to eq(0)
+    expect(search(guest, 'add').commits_count).to eq(0)
+  end
+
+  def search(user, search)
+    Search::GlobalService.new(user, search: search).execute
+  end
+end

spec/models/concerns/elastic/issue_spec.rb

@@ -11,9 +11,9 @@ describe Issue, elastic: true do
     stub_application_setting(elasticsearch_search: false, elasticsearch_indexing: false)
   end
 
-  it "searches issues" do
-    project = create :empty_project
+  let(:project) { create :empty_project }
 
+  it "searches issues" do
     Sidekiq::Testing.inline! do
       create :issue, title: 'bla-bla term', project: project
       create :issue, description: 'bla-bla term', project: project
@@ -31,7 +31,6 @@ describe Issue, elastic: true do
   end
 
   it "returns json with all needed elements" do
-    project = create :empty_project
     issue = create :issue, project: project
 
     expected_hash = issue.attributes.extract!('id', 'iid', 'title', 'description', 'created_at',

spec/models/concerns/elastic/note_spec.rb

@@ -54,6 +54,8 @@ describe Note, elastic: true do
       id
       note
       project_id
+      noteable_type
+      noteable_id
       created_at
       updated_at
       issue

spec/models/concerns/elastic/project_spec.rb

@@ -59,6 +59,16 @@ describe Project, elastic: true do
       'last_activity_at'
     )
 
+    expected_hash.merge!(
+      project.project_feature.attributes.extract!(
+        'issues_access_level',
+        'merge_requests_access_level',
+        'snippets_access_level',
+        'wiki_access_level',
+        'repository_access_level'
+      )
+    )
+
     expected_hash['name_with_namespace'] = project.name_with_namespace
     expected_hash['path_with_namespace'] = project.path_with_namespace
 

spec/models/concerns/elastic/snippet_spec.rb

@@ -30,17 +30,17 @@ describe Snippet, elastic: true do
     it 'returns only public snippets when user is blank' do
       result = described_class.elastic_search_code('password', options: { user: nil })
 
-      expect(result.total_count).to eq(2)
-      expect(result.records).to match_array [public_snippet, project_public_snippet]
+      expect(result.total_count).to eq(1)
+      expect(result.records).to match_array [public_snippet]
     end
 
-    it 'returns only public and internal snippets for regular users' do
-      regular_user = create(:user)
+    it 'returns only public and internal personal snippets for non-members' do
+      non_member = create(:user)
 
-      result = described_class.elastic_search_code('password', options: { user: regular_user })
+      result = described_class.elastic_search_code('password', options: { user: non_member })
 
-      expect(result.total_count).to eq(4)
-      expect(result.records).to match_array [public_snippet, internal_snippet, project_public_snippet, project_internal_snippet]
+      expect(result.total_count).to eq(2)
+      expect(result.records).to match_array [public_snippet, internal_snippet]
     end
 
     it 'returns public, internal snippets, and project private snippets for project members' do
@@ -56,8 +56,8 @@ describe Snippet, elastic: true do
     it 'returns private snippets where the user is the author' do
       result = described_class.elastic_search_code('password', options: { user: author })
 
-      expect(result.total_count).to eq(5)
-      expect(result.records).to match_array [public_snippet, internal_snippet, private_snippet, project_public_snippet, project_internal_snippet]
+      expect(result.total_count).to eq(3)
+      expect(result.records).to match_array [public_snippet, internal_snippet, private_snippet]
     end
 
     it 'returns all snippets for admins' do