Merge remote-tracking branch 'dev/master'

CHANGELOG-EE.md

 Please view this file on the master branch, on stable branches it's out of date.
 
+## 8.17.4 (2017-03-19)
+
+- Elastic security fix: Respect feature visibility level.
+
 ## 8.17.3 (2017-03-07)
 
 - No changes.
@@ -34,6 +38,13 @@ Please view this file on the master branch, on stable branches it's out of date.
 - Reduce queries needed to check if node is a primary or secondary Geo node.
 - Allow squashing merge requests into a single commit.
 
+## 8.16.8 (2017-03-19)
+
+- No changes.
+- No changes.
+- No changes.
+- Elastic security fix: Respect feature visibility level.
+
 ## 8.16.7 (2017-02-27)
 
 - Fixed merge request state not updating when approvals feature is active.
@@ -78,6 +89,12 @@ Please view this file on the master branch, on stable branches it's out of date.
 - Expose issue weight in the API. !1023 (Robert Schilling)
 - Copy <some text> to clipboard. !1048
 
+## 8.15.8 (2017-03-19)
+
+- No changes.
+- No changes.
+- Elastic security fix: Respect feature visibility level.
+
 ## 8.15.7 (2017-02-15)
 
 - No changes.

CHANGELOG.md

@@ -2,6 +2,11 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 8.17.4 (2017-03-19)
+
+- Only show public emails in atom feeds.
+- To protect against Server-side Request Forgery project import URLs are now prohibited against localhost or the server IP except for the assigned instance URL and port. Imports are also prohibited from ports below 1024 with the exception of ports 22, 80, and 443.
+
 ## 8.17.3 (2017-03-07)
 
 - Fix the redirect to custom home page URL. !9518
@@ -211,6 +216,14 @@ entry.
 - Remove deprecated GitlabCiService.
 - Requeue pending deletion projects.
 
+## 8.16.8 (2017-03-19)
+
+- No changes.
+- No changes.
+- No changes.
+- Only show public emails in atom feeds.
+- To protect against Server-side Request Forgery project import URLs are now prohibited against localhost or the server IP except for the assigned instance URL and port. Imports are also prohibited from ports below 1024 with the exception of ports 22, 80, and 443.
+
 ## 8.16.7 (2017-02-27)
 
 - Fix MR changes tab size count when there are over 100 files in the diff.
@@ -410,6 +423,14 @@ entry.
 - Add margin to markdown math blocks.
 - Add hover state to MR comment reply button.
 
+## 8.15.8 (2017-03-19)
+
+- No changes.
+- No changes.
+- Read true-up info from license and validate it. !1159
+- Only show public emails in atom feeds.
+- To protect against Server-side Request Forgery project import URLs are now prohibited against localhost or the server IP except for the assigned instance URL and port. Imports are also prohibited from ports below 1024 with the exception of ports 22, 80, and 443.
+
 ## 8.15.7 (2017-02-15)
 
 - No changes.

app/models/concerns/elastic/application_search.rb

@@ -158,43 +158,76 @@ module Elastic
       end
 
       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] ||= []
-          query_hash[:query][:bool][:filter] << {
-            has_parent: {
-              parent_type: "project",
-              query: {
-                bool: {
-                  should: condition
-                }
-              }
+        project_query = project_ids_query(
+          options[:current_user],
+          options[:project_ids],
+          options[:public_and_internal_projects],
+          options[:feature]
+        )
+
+        query_hash[:query][:bool][:filter] ||= []
+        query_hash[:query][:bool][:filter] << {
+          has_parent: {
+            parent_type: "project",
+            query: {
+              bool: project_query
             }
           }
-        end
+        }
 
         query_hash
       end
 
-      def project_ids_condition(current_user, project_ids, public_and_internal_projects)
-        conditions = [{
-          terms: { id: project_ids }
-        }]
+      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 :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