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

Fix private feature Elasticsearch leak

See merge request gitlab/gitlabhq!3451

7 files changed, 210 insertions, 2 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 8efe4b06f87..a1bd5edaba9 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -459,7 +459,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
@@ -487,6 +487,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) }
 
diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb
index 78e82955342..dc7bfb92e09 100644
--- a/app/models/project_feature.rb
+++ b/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
diff --git a/changelogs/unreleased/security-29491.yml b/changelogs/unreleased/security-29491.yml
new file mode 100644
index 00000000000..ec4ada47c62
--- /dev/null
+++ b/changelogs/unreleased/security-29491.yml
@@ -0,0 +1,5 @@
+---
+title: Fix private feature Elasticsearch leak
+merge_request:
+author:
+type: security
diff --git a/spec/models/project_feature_spec.rb b/spec/models/project_feature_spec.rb
index dc7a8433064..45755c43827 100644
--- a/spec/models/project_feature_spec.rb
+++ b/spec/models/project_feature_spec.rb
@@ -174,4 +174,28 @@ describe ProjectFeature do
       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
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index ff9e94afc12..de5fe9ee8a8 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3424,6 +3424,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) }
 
diff --git a/spec/support/helpers/project_helpers.rb b/spec/support/helpers/project_helpers.rb
new file mode 100644
index 00000000000..61056b47aed
--- /dev/null
+++ b/spec/support/helpers/project_helpers.rb
@@ -0,0 +1,25 @@
+# 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
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
new file mode 100644
index 00000000000..e666b346b8b
--- /dev/null
+++ b/spec/support/shared_contexts/policies/project_policy_table_shared_context.rb
@@ -0,0 +1,118 @@
+# 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