Merge branch 'security-33712-ce-12-4' into '12-4-stable'

Fix private comment Elasticsearch leak

See merge request gitlab/gitlabhq!3524

7 files changed, 342 insertions, 18 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 74da042d5a5..b0d305e80d6 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -516,7 +516,11 @@ class Project < ApplicationRecord
 
   # This scope returns projects where user has access to both the project and the feature.
   def self.filter_by_feature_visibility(feature, user)
-    with_feature_available_for_user(feature, user).public_or_visible_to_user(user)
+    with_feature_available_for_user(feature, user)
+      .public_or_visible_to_user(
+        user,
+        ProjectFeature.required_minimum_access_level_for_private_project(feature)
+      )
   end
 
   scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') }
diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb
index 2013f620b5b..caa65d32c86 100644
--- a/app/models/project_feature.rb
+++ b/app/models/project_feature.rb
@@ -24,6 +24,7 @@ class ProjectFeature < ApplicationRecord
 
   FEATURES = %i(issues merge_requests wiki snippets builds repository pages).freeze
   PRIVATE_FEATURES_MIN_ACCESS_LEVEL = { merge_requests: Gitlab::Access::REPORTER }.freeze
+  PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT = { repository: Gitlab::Access::REPORTER }.freeze
   STRING_OPTIONS = HashWithIndifferentAccess.new({
     'disabled' => DISABLED,
     'private'  => PRIVATE,
@@ -51,6 +52,15 @@ class ProjectFeature < ApplicationRecord
       PRIVATE_FEATURES_MIN_ACCESS_LEVEL.fetch(feature, Gitlab::Access::GUEST)
     end
 
+    # Guest users can perform certain features on public and internal projects, but not private projects.
+    def required_minimum_access_level_for_private_project(feature)
+      feature = ensure_feature!(feature)
+
+      PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT.fetch(feature) do
+        required_minimum_access_level(feature)
+      end
+    end
+
     def access_level_from_str(level)
       STRING_OPTIONS.fetch(level)
     end
diff --git a/spec/models/project_feature_spec.rb b/spec/models/project_feature_spec.rb
index 5b448c343a0..9ce1b8fd895 100644
--- a/spec/models/project_feature_spec.rb
+++ b/spec/models/project_feature_spec.rb
@@ -6,6 +6,16 @@ describe ProjectFeature do
   let(:project) { create(:project) }
   let(:user) { create(:user) }
 
+  describe 'PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT' do
+    it 'has higher level than that of PRIVATE_FEATURES_MIN_ACCESS_LEVEL' do
+      described_class::PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT.each do |feature, level|
+        if generic_level = described_class::PRIVATE_FEATURES_MIN_ACCESS_LEVEL[feature]
+          expect(level).to be >= generic_level
+        end
+      end
+    end
+  end
+
   describe '.quoted_access_level_column' do
     it 'returns the table name and quoted column name for a feature' do
       expected = '"project_features"."issues_access_level"'
@@ -246,10 +256,24 @@ describe ProjectFeature do
       expect(described_class.required_minimum_access_level('merge_requests')).to eq(Gitlab::Access::REPORTER)
     end
 
+    it 'handles repository' do
+      expect(described_class.required_minimum_access_level(:repository)).to eq(Gitlab::Access::GUEST)
+    end
+
     it 'raises error if feature is invalid' do
       expect do
         described_class.required_minimum_access_level(:foos)
       end.to raise_error
     end
   end
+
+  describe '.required_minimum_access_level_for_private_project' do
+    it 'returns higher permission for repository' do
+      expect(described_class.required_minimum_access_level_for_private_project(:repository)).to eq(Gitlab::Access::REPORTER)
+    end
+
+    it 'returns normal permission for issues' do
+      expect(described_class.required_minimum_access_level_for_private_project(:issues)).to eq(Gitlab::Access::GUEST)
+    end
+  end
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 1bda3094e75..a52d424b99a 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2911,7 +2911,7 @@ describe Project do
   end
 
   describe '#any_lfs_file_locks?', :request_store do
-    set(:project) { create(:project) }
+    let_it_be(:project) { create(:project) }
 
     it 'returns false when there are no LFS file locks' do
       expect(project.any_lfs_file_locks?).to be_falsey
@@ -3414,6 +3414,20 @@ describe Project do
         expect(projects).to eq([public_project])
       end
     end
+
+    context 'min_access_level' do
+      let!(:private_project) { create(:project, :private) }
+
+      before do
+        private_project.add_guest(user)
+      end
+
+      it 'excludes projects when user does not have required minimum access level' do
+        projects = described_class.all.public_or_visible_to_user(user, Gitlab::Access::REPORTER)
+
+        expect(projects).to contain_exactly(public_project)
+      end
+    end
   end
 
   describe '.ids_with_issuables_available_for' do
@@ -3542,7 +3556,7 @@ describe Project do
     include ProjectHelpers
     using RSpec::Parameterized::TableSyntax
 
-    set(:group) { create(:group) }
+    let_it_be(:group) { create(:group) }
     let!(:project) { create(:project, project_level, namespace: group ) }
     let(:user) { create_user_from_membership(project, membership) }
 
@@ -3565,6 +3579,66 @@ describe Project do
         end
       end
     end
+
+    context 'issues' do
+      let(:feature) { Issue }
+
+      where(:project_level, :feature_access_level, :membership, :expected_count) do
+        permission_table_for_guest_feature_access
+      end
+
+      with_them do
+        it "respects visibility" do
+          update_feature_access_level(project, feature_access_level)
+
+          expected_objects = expected_count == 1 ? [project] : []
+
+          expect(
+            described_class.filter_by_feature_visibility(feature, user)
+          ).to eq(expected_objects)
+        end
+      end
+    end
+
+    context 'wiki' do
+      let(:feature) { :wiki }
+
+      where(:project_level, :feature_access_level, :membership, :expected_count) do
+        permission_table_for_guest_feature_access
+      end
+
+      with_them do
+        it "respects visibility" do
+          update_feature_access_level(project, feature_access_level)
+
+          expected_objects = expected_count == 1 ? [project] : []
+
+          expect(
+            described_class.filter_by_feature_visibility(feature, user)
+          ).to eq(expected_objects)
+        end
+      end
+    end
+
+    context 'code' do
+      let(:feature) { :repository }
+
+      where(:project_level, :feature_access_level, :membership, :expected_count) do
+        permission_table_for_guest_feature_access_and_non_private_project_only
+      end
+
+      with_them do
+        it "respects visibility" do
+          update_feature_access_level(project, feature_access_level)
+
+          expected_objects = expected_count == 1 ? [project] : []
+
+          expect(
+            described_class.filter_by_feature_visibility(feature, user)
+          ).to eq(expected_objects)
+        end
+      end
+    end
   end
 
   describe '#pages_available?' do
diff --git a/spec/support/helpers/project_helpers.rb b/spec/support/helpers/project_helpers.rb
index 61056b47aed..89f0163b4b6 100644
--- a/spec/support/helpers/project_helpers.rb
+++ b/spec/support/helpers/project_helpers.rb
@@ -10,16 +10,18 @@ module ProjectHelpers
       nil
     when :non_member
       create(:user, name: membership)
+    when :admin
+      create(:user, :admin, name: 'admin')
     else
       create(:user, name: membership).tap { |u| target.add_user(u, membership) }
     end
   end
 
   def update_feature_access_level(project, access_level)
-    project.update!(
-      repository_access_level: access_level,
-      merge_requests_access_level: access_level,
-      builds_access_level: access_level
-    )
+    features = ProjectFeature::FEATURES.dup
+    features.delete(:pages)
+    params = features.each_with_object({}) { |feature, h| h["#{feature}_access_level"] = access_level }
+
+    project.update!(params)
   end
 end
diff --git a/spec/support/helpers/search_helpers.rb b/spec/support/helpers/search_helpers.rb
index d1d25fbabcd..db6e47459e9 100644
--- a/spec/support/helpers/search_helpers.rb
+++ b/spec/support/helpers/search_helpers.rb
@@ -20,6 +20,12 @@ module SearchHelpers
     end
   end
 
+  def has_search_scope?(scope)
+    page.within '.search-filter' do
+      has_link?(scope)
+    end
+  end
+
   def max_limited_count
     Gitlab::SearchResults::COUNT_LIMIT_MESSAGE
   end
diff --git a/spec/support/shared_contexts/policies/project_policy_table_shared_context.rb b/spec/support/shared_contexts/policies/project_policy_table_shared_context.rb
index e666b346b8b..0a918ccde81 100644
--- a/spec/support/shared_contexts/policies/project_policy_table_shared_context.rb
+++ b/spec/support/shared_contexts/policies/project_policy_table_shared_context.rb
@@ -3,13 +3,28 @@
 RSpec.shared_context 'ProjectPolicyTable context' do
   using RSpec::Parameterized::TableSyntax
 
+  let(:pendings) { {} }
+  let(:pending?) do
+    pendings.include?(
+      {
+        project_level: project_level,
+        feature_access_level: feature_access_level,
+        membership: membership,
+        expected_count: expected_count
+      }
+    )
+  end
+
   # rubocop:disable Metrics/AbcSize
+  # project_level, :feature_access_level, :membership, :expected_count
   def permission_table_for_reporter_feature_access
+    :public   | :enabled  | :admin      | 1
     :public   | :enabled  | :reporter   | 1
     :public   | :enabled  | :guest      | 1
     :public   | :enabled  | :non_member | 1
     :public   | :enabled  | :anonymous  | 1
 
+    :public   | :private  | :admin      | 1
     :public   | :private  | :reporter   | 1
     :public   | :private  | :guest      | 0
     :public   | :private  | :non_member | 0
@@ -20,11 +35,13 @@ RSpec.shared_context 'ProjectPolicyTable context' do
     :public   | :disabled | :non_member | 0
     :public   | :disabled | :anonymous  | 0
 
+    :internal | :enabled  | :admin      | 1
     :internal | :enabled  | :reporter   | 1
     :internal | :enabled  | :guest      | 1
     :internal | :enabled  | :non_member | 1
     :internal | :enabled  | :anonymous  | 0
 
+    :internal | :private  | :admin      | 1
     :internal | :private  | :reporter   | 1
     :internal | :private  | :guest      | 0
     :internal | :private  | :non_member | 0
@@ -35,11 +52,7 @@ RSpec.shared_context 'ProjectPolicyTable context' do
     :internal | :disabled | :non_member | 0
     :internal | :disabled | :anonymous  | 0
 
-    :private  | :enabled  | :reporter   | 1
-    :private  | :enabled  | :guest      | 1
-    :private  | :enabled  | :non_member | 0
-    :private  | :enabled  | :anonymous  | 0
-
+    :private  | :private  | :admin      | 1
     :private  | :private  | :reporter   | 1
     :private  | :private  | :guest      | 0
     :private  | :private  | :non_member | 0
@@ -51,12 +64,15 @@ RSpec.shared_context 'ProjectPolicyTable context' do
     :private  | :disabled | :anonymous  | 0
   end
 
+  # project_level, :feature_access_level, :membership, :expected_count
   def permission_table_for_guest_feature_access
+    :public   | :enabled  | :admin      | 1
     :public   | :enabled  | :reporter   | 1
     :public   | :enabled  | :guest      | 1
     :public   | :enabled  | :non_member | 1
     :public   | :enabled  | :anonymous  | 1
 
+    :public   | :private  | :admin      | 1
     :public   | :private  | :reporter   | 1
     :public   | :private  | :guest      | 1
     :public   | :private  | :non_member | 0
@@ -67,11 +83,13 @@ RSpec.shared_context 'ProjectPolicyTable context' do
     :public   | :disabled | :non_member | 0
     :public   | :disabled | :anonymous  | 0
 
+    :internal | :enabled  | :admin      | 1
     :internal | :enabled  | :reporter   | 1
     :internal | :enabled  | :guest      | 1
     :internal | :enabled  | :non_member | 1
     :internal | :enabled  | :anonymous  | 0
 
+    :internal | :private  | :admin      | 1
     :internal | :private  | :reporter   | 1
     :internal | :private  | :guest      | 1
     :internal | :private  | :non_member | 0
@@ -82,11 +100,7 @@ RSpec.shared_context 'ProjectPolicyTable context' do
     :internal | :disabled | :non_member | 0
     :internal | :disabled | :anonymous  | 0
 
-    :private  | :enabled  | :reporter   | 1
-    :private  | :enabled  | :guest      | 1
-    :private  | :enabled  | :non_member | 0
-    :private  | :enabled  | :anonymous  | 0
-
+    :private  | :private  | :admin      | 1
     :private  | :private  | :reporter   | 1
     :private  | :private  | :guest      | 1
     :private  | :private  | :non_member | 0
@@ -98,6 +112,196 @@ RSpec.shared_context 'ProjectPolicyTable context' do
     :private  | :disabled | :anonymous  | 0
   end
 
+  # This table is based on permission_table_for_guest_feature_access,
+  # but with a slight twist.
+  # Some features can be hidden away to GUEST, when project is private.
+  # (see ProjectFeature::PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT)
+  # This is the table for such features.
+  #
+  # e.g. `repository` feature has minimum requirement of GUEST,
+  # but a GUEST are prohibited from reading code if project is private.
+  #
+  # project_level, :feature_access_level, :membership, :expected_count
+  def permission_table_for_guest_feature_access_and_non_private_project_only
+    :public   | :enabled  | :admin      | 1
+    :public   | :enabled  | :reporter   | 1
+    :public   | :enabled  | :guest      | 1
+    :public   | :enabled  | :non_member | 1
+    :public   | :enabled  | :anonymous  | 1
+
+    :public   | :private  | :admin      | 1
+    :public   | :private  | :reporter   | 1
+    :public   | :private  | :guest      | 1
+    :public   | :private  | :non_member | 0
+    :public   | :private  | :anonymous  | 0
+
+    :public   | :disabled | :reporter   | 0
+    :public   | :disabled | :guest      | 0
+    :public   | :disabled | :non_member | 0
+    :public   | :disabled | :anonymous  | 0
+
+    :internal | :enabled  | :admin      | 1
+    :internal | :enabled  | :reporter   | 1
+    :internal | :enabled  | :guest      | 1
+    :internal | :enabled  | :non_member | 1
+    :internal | :enabled  | :anonymous  | 0
+
+    :internal | :private  | :admin      | 1
+    :internal | :private  | :reporter   | 1
+    :internal | :private  | :guest      | 1
+    :internal | :private  | :non_member | 0
+    :internal | :private  | :anonymous  | 0
+
+    :internal | :disabled | :reporter   | 0
+    :internal | :disabled | :guest      | 0
+    :internal | :disabled | :non_member | 0
+    :internal | :disabled | :anonymous  | 0
+
+    :private  | :private  | :admin      | 1
+    :private  | :private  | :reporter   | 1
+    :private  | :private  | :guest      | 0
+    :private  | :private  | :non_member | 0
+    :private  | :private  | :anonymous  | 0
+
+    :private  | :disabled | :reporter   | 0
+    :private  | :disabled | :guest      | 0
+    :private  | :disabled | :non_member | 0
+    :private  | :disabled | :anonymous  | 0
+  end
+
+  # :project_level, :issues_access_level, :merge_requests_access_level, :membership, :expected_count
+  def permission_table_for_milestone_access
+    :public   | :enabled  | :enabled  | :admin      | 1
+    :public   | :enabled  | :enabled  | :reporter   | 1
+    :public   | :enabled  | :enabled  | :guest      | 1
+    :public   | :enabled  | :enabled  | :non_member | 1
+    :public   | :enabled  | :enabled  | :anonymous  | 1
+
+    :public   | :enabled  | :private  | :admin      | 1
+    :public   | :enabled  | :private  | :reporter   | 1
+    :public   | :enabled  | :private  | :guest      | 1
+    :public   | :enabled  | :private  | :non_member | 1
+    :public   | :enabled  | :private  | :anonymous  | 1
+
+    :public   | :enabled  | :disabled | :admin      | 1
+    :public   | :enabled  | :disabled | :reporter   | 1
+    :public   | :enabled  | :disabled | :guest      | 1
+    :public   | :enabled  | :disabled | :non_member | 1
+    :public   | :enabled  | :disabled | :anonymous  | 1
+
+    :public   | :private  | :enabled  | :admin      | 1
+    :public   | :private  | :enabled  | :reporter   | 1
+    :public   | :private  | :enabled  | :guest      | 1
+    :public   | :private  | :enabled  | :non_member | 1
+    :public   | :private  | :enabled  | :anonymous  | 1
+
+    :public   | :private  | :private  | :admin      | 1
+    :public   | :private  | :private  | :reporter   | 1
+    :public   | :private  | :private  | :guest      | 1
+    :public   | :private  | :private  | :non_member | 0
+    :public   | :private  | :private  | :anonymous  | 0
+
+    :public   | :private  | :disabled | :admin      | 1
+    :public   | :private  | :disabled | :reporter   | 1
+    :public   | :private  | :disabled | :guest      | 1
+    :public   | :private  | :disabled | :non_member | 0
+    :public   | :private  | :disabled | :anonymous  | 0
+
+    :public   | :disabled | :enabled  | :admin      | 1
+    :public   | :disabled | :enabled  | :reporter   | 1
+    :public   | :disabled | :enabled  | :guest      | 1
+    :public   | :disabled | :enabled  | :non_member | 1
+    :public   | :disabled | :enabled  | :anonymous  | 1
+
+    :public   | :disabled | :private  | :admin      | 1
+    :public   | :disabled | :private  | :reporter   | 1
+    :public   | :disabled | :private  | :guest      | 0
+    :public   | :disabled | :private  | :non_member | 0
+    :public   | :disabled | :private  | :anonymous  | 0
+
+    :public   | :disabled | :disabled | :reporter   | 0
+    :public   | :disabled | :disabled | :guest      | 0
+    :public   | :disabled | :disabled | :non_member | 0
+    :public   | :disabled | :disabled | :anonymous  | 0
+
+    :internal | :enabled  | :enabled  | :admin      | 1
+    :internal | :enabled  | :enabled  | :reporter   | 1
+    :internal | :enabled  | :enabled  | :guest      | 1
+    :internal | :enabled  | :enabled  | :non_member | 1
+    :internal | :enabled  | :enabled  | :anonymous  | 0
+
+    :internal | :enabled  | :private  | :admin      | 1
+    :internal | :enabled  | :private  | :reporter   | 1
+    :internal | :enabled  | :private  | :guest      | 1
+    :internal | :enabled  | :private  | :non_member | 1
+    :internal | :enabled  | :private  | :anonymous  | 0
+
+    :internal | :enabled  | :disabled | :admin      | 1
+    :internal | :enabled  | :disabled | :reporter   | 1
+    :internal | :enabled  | :disabled | :guest      | 1
+    :internal | :enabled  | :disabled | :non_member | 1
+    :internal | :enabled  | :disabled | :anonymous  | 0
+
+    :internal | :private  | :enabled  | :admin      | 1
+    :internal | :private  | :enabled  | :reporter   | 1
+    :internal | :private  | :enabled  | :guest      | 1
+    :internal | :private  | :enabled  | :non_member | 1
+    :internal | :private  | :enabled  | :anonymous  | 0
+
+    :internal | :private  | :private  | :admin      | 1
+    :internal | :private  | :private  | :reporter   | 1
+    :internal | :private  | :private  | :guest      | 1
+    :internal | :private  | :private  | :non_member | 0
+    :internal | :private  | :private  | :anonymous  | 0
+
+    :internal | :private  | :disabled | :admin      | 1
+    :internal | :private  | :disabled | :reporter   | 1
+    :internal | :private  | :disabled | :guest      | 1
+    :internal | :private  | :disabled | :non_member | 0
+    :internal | :private  | :disabled | :anonymous  | 0
+
+    :internal | :disabled | :enabled  | :admin      | 1
+    :internal | :disabled | :enabled  | :reporter   | 1
+    :internal | :disabled | :enabled  | :guest      | 1
+    :internal | :disabled | :enabled  | :non_member | 1
+    :internal | :disabled | :enabled  | :anonymous  | 0
+
+    :internal | :disabled | :private  | :admin      | 1
+    :internal | :disabled | :private  | :reporter   | 1
+    :internal | :disabled | :private  | :guest      | 0
+    :internal | :disabled | :private  | :non_member | 0
+    :internal | :disabled | :private  | :anonymous  | 0
+
+    :internal | :disabled | :disabled | :reporter   | 0
+    :internal | :disabled | :disabled | :guest      | 0
+    :internal | :disabled | :disabled | :non_member | 0
+    :internal | :disabled | :disabled | :anonymous  | 0
+
+    :private  | :private  | :private  | :admin      | 1
+    :private  | :private  | :private  | :reporter   | 1
+    :private  | :private  | :private  | :guest      | 1
+    :private  | :private  | :private  | :non_member | 0
+    :private  | :private  | :private  | :anonymous  | 0
+
+    :private  | :private  | :disabled | :admin      | 1
+    :private  | :private  | :disabled | :reporter   | 1
+    :private  | :private  | :disabled | :guest      | 1
+    :private  | :private  | :disabled | :non_member | 0
+    :private  | :private  | :disabled | :anonymous  | 0
+
+    :private  | :disabled | :private  | :admin      | 1
+    :private  | :disabled | :private  | :reporter   | 1
+    :private  | :disabled | :private  | :guest      | 0
+    :private  | :disabled | :private  | :non_member | 0
+    :private  | :disabled | :private  | :anonymous  | 0
+
+    :private  | :disabled | :disabled | :reporter   | 0
+    :private  | :disabled | :disabled | :guest      | 0
+    :private  | :disabled | :disabled | :non_member | 0
+    :private  | :disabled | :disabled | :anonymous  | 0
+  end
+
+  # :project_level, :membership, :expected_count
   def permission_table_for_project_access
     :public   | :reporter   | 1
     :public   | :guest      | 1