Merge branch 'security-29491-ce' into 'master'

Fix private feature Elasticsearch leak

Closes gitlab-ee#381

See merge request gitlab/gitlabhq!3457

app/models/project.rb

@@ -482,7 +482,7 @@ class Project < ApplicationRecord
   # the feature is either public, enabled, or internal with permission for the user.
   # Note: this scope doesn't enforce that the user has access to the projects, it just checks
   # that the user has access to the feature. It's important to use this scope with others
-  # that checks project authorizations first.
+  # that checks project authorizations first (e.g. `filter_by_feature_visibility`).
   #
   # This method uses an optimised version of `with_feature_access_level` for
   # logged in users to more efficiently get private projects with the given
@@ -510,6 +510,11 @@ class Project < ApplicationRecord
     end
   end
 
+  # 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)
+  end
+
   scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') }
   scope :abandoned, -> { where('projects.last_activity_at < ?', 6.months.ago) }
 

app/models/project_feature.rb

@@ -62,7 +62,8 @@ class ProjectFeature < ApplicationRecord
     private
 
     def ensure_feature!(feature)
-      feature = feature.model_name.plural.to_sym if feature.respond_to?(:model_name)
+      feature = feature.model_name.plural if feature.respond_to?(:model_name)
+      feature = feature.to_sym
       raise ArgumentError, "invalid project feature: #{feature}" unless FEATURES.include?(feature)
 
       feature

changelogs/unreleased/security-29491.yml

+---
+title: Fix private feature Elasticsearch leak
+merge_request:
+author:
+type: security

spec/models/project_feature_spec.rb

@@ -228,4 +228,28 @@ describe ProjectFeature do
       end
     end
   end
+
+  describe '.required_minimum_access_level' do
+    it 'handles reporter level' do
+      expect(described_class.required_minimum_access_level(:merge_requests)).to eq(Gitlab::Access::REPORTER)
+    end
+
+    it 'handles guest level' do
+      expect(described_class.required_minimum_access_level(:issues)).to eq(Gitlab::Access::GUEST)
+    end
+
+    it 'accepts ActiveModel' do
+      expect(described_class.required_minimum_access_level(MergeRequest)).to eq(Gitlab::Access::REPORTER)
+    end
+
+    it 'accepts string' do
+      expect(described_class.required_minimum_access_level('merge_requests')).to eq(Gitlab::Access::REPORTER)
+    end
+
+    it 'raises error if feature is invalid' do
+      expect do
+        described_class.required_minimum_access_level(:foos)
+      end.to raise_error
+    end
+  end
 end

spec/models/project_spec.rb

@@ -3469,6 +3469,36 @@ describe Project do
     end
   end
 
+  describe '.filter_by_feature_visibility' do
+    include_context 'ProjectPolicyTable context'
+    include ProjectHelpers
+    using RSpec::Parameterized::TableSyntax
+
+    set(:group) { create(:group) }
+    let!(:project) { create(:project, project_level, namespace: group ) }
+    let(:user) { create_user_from_membership(project, membership) }
+
+    context 'reporter level access' do
+      let(:feature) { MergeRequest }
+
+      where(:project_level, :feature_access_level, :membership, :expected_count) do
+        permission_table_for_reporter_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
+  end
+
   describe '#pages_available?' do
     let(:project) { create(:project, group: group) }
 

spec/support/helpers/project_helpers.rb

+# frozen_string_literal: true
+
+module ProjectHelpers
+  # @params target [Project] membership target
+  # @params membership [Symbol] accepts the membership levels :guest, :reporter...
+  #                             and phony levels :non_member and :anonymous
+  def create_user_from_membership(target, membership)
+    case membership
+    when :anonymous
+      nil
+    when :non_member
+      create(:user, name: membership)
+    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
+    )
+  end
+end

spec/support/helpers/search_results_helpers.rb

+# frozen_string_literal: true
+
+module SearchResultHelpers
+  # @param target [Symbol] search target, e.g. "merge_requests", "blobs"
+  def expect_search_results(users, target, expected_count: nil, expected_objects: nil)
+    # TODO: https://gitlab.com/gitlab-org/gitlab/issues/32645
+    return if expected_count && expected_count > 0
+
+    users = Array(users)
+    target = target.to_s
+
+    users.each do |user|
+      user_name = user&.name || 'anonymous user'
+      results = yield(user)
+      objects = results.objects(target)
+
+      if expected_count
+        actual_count = results.public_send("#{target}_count")
+
+        expect(actual_count).to eq(expected_count), "expected count to be #{expected_count} for #{user_name}, got #{actual_count}"
+      end
+
+      if expected_objects
+        if expected_objects.empty?
+          expect(objects.empty?).to eq(true)
+        else
+          expect(objects).to contain_exactly(*expected_objects)
+        end
+      end
+    end
+  end
+end

spec/support/shared_contexts/policies/project_policy_table_shared_context.rb

+# frozen_string_literal: true
+
+RSpec.shared_context 'ProjectPolicyTable context' do
+  using RSpec::Parameterized::TableSyntax
+
+  # rubocop:disable Metrics/AbcSize
+  def permission_table_for_reporter_feature_access
+    :public   | :enabled  | :reporter   | 1
+    :public   | :enabled  | :guest      | 1
+    :public   | :enabled  | :non_member | 1
+    :public   | :enabled  | :anonymous  | 1
+
+    :public   | :private  | :reporter   | 1
+    :public   | :private  | :guest      | 0
+    :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  | :reporter   | 1
+    :internal | :enabled  | :guest      | 1
+    :internal | :enabled  | :non_member | 1
+    :internal | :enabled  | :anonymous  | 0
+
+    :internal | :private  | :reporter   | 1
+    :internal | :private  | :guest      | 0
+    :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  | :enabled  | :reporter   | 1
+    :private  | :enabled  | :guest      | 1
+    :private  | :enabled  | :non_member | 0
+    :private  | :enabled  | :anonymous  | 0
+
+    :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
+
+  def permission_table_for_guest_feature_access
+    :public   | :enabled  | :reporter   | 1
+    :public   | :enabled  | :guest      | 1
+    :public   | :enabled  | :non_member | 1
+    :public   | :enabled  | :anonymous  | 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  | :reporter   | 1
+    :internal | :enabled  | :guest      | 1
+    :internal | :enabled  | :non_member | 1
+    :internal | :enabled  | :anonymous  | 0
+
+    :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  | :enabled  | :reporter   | 1
+    :private  | :enabled  | :guest      | 1
+    :private  | :enabled  | :non_member | 0
+    :private  | :enabled  | :anonymous  | 0
+
+    :private  | :private  | :reporter   | 1
+    :private  | :private  | :guest      | 1
+    :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
+
+  def permission_table_for_project_access
+    :public   | :reporter   | 1
+    :public   | :guest      | 1
+    :public   | :non_member | 1
+    :public   | :anonymous  | 1
+
+    :internal | :reporter   | 1
+    :internal | :guest      | 1
+    :internal | :non_member | 1
+    :internal | :anonymous  | 0
+
+    :private  | :reporter   | 1
+    :private  | :guest      | 1
+    :private  | :non_member | 0
+    :private  | :anonymous  | 0
+  end
+  # rubocop:enable Metrics/AbcSize
+end