Merge branch 'security-29491-12-1-ce' into '12-1-stable'

Fix private feature Elasticsearch leak See merge request gitlab/gitlabhq!3452

Merge branch 'security-29491-12-1-ce' into '12-1-stable'
Fix private feature Elasticsearch leak See merge request gitlab/gitlabhq!3452
cdfcbb00 · Marin Jankovski · f84dc8c1 · ccb6a5a5 · cdfcbb00 · cdfcbb00
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -483,7 +483,7 @@ def self.public_or_visible_to_user(user = nil, min_access_level = nil)
  # 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
@@ -511,6 +511,11 @@ def self.with_feature_available_for_user(feature, user)
    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) }


--- a/app/models/project_feature.rb
+++ b/app/models/project_feature.rb
@@ -62,7 +62,8 @@ def str_from_access_level(level)
    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

--- a/changelogs/unreleased/security-29491.yml
+++ b/changelogs/unreleased/security-29491.yml
+---
+title: Fix private feature Elasticsearch leak
+merge_request:
+author:
+type: security
--- a/spec/models/project_feature_spec.rb
+++ b/spec/models/project_feature_spec.rb
@@ -178,4 +178,28 @@
      it { is_expected.to eq(ProjectFeature::ENABLED) }
    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
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3308,6 +3308,36 @@ def enable_lfs
    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) }


--- a/spec/support/helpers/project_helpers.rb
+++ b/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
--- a/spec/support/shared_contexts/policies/project_policy_table_shared_context.rb
+++ b/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