summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMark Chao <mchao@gitlab.com>2019-09-16 17:50:41 +0800
committerMark Chao <mchao@gitlab.com>2019-10-01 17:19:39 +1300
commitccb6a5a56e722337fe5ea242a91829ea52e15c0c (patch)
tree5baeb2b752f0ab712b9f110150ec4ccdd6c14d97
parentf84dc8c1a1c40943ef991ecd5c7493b4aa8410de (diff)
downloadgitlab-ce-ccb6a5a56e722337fe5ea242a91829ea52e15c0c.tar.gz
EE port: Fix private feature Elasticsearch leak
Add spec to test different combinations. Accept string for required_minimum_access_level Allow more flexible project membership query
-rw-r--r--app/models/project.rb7
-rw-r--r--app/models/project_feature.rb3
-rw-r--r--changelogs/unreleased/security-29491.yml5
-rw-r--r--spec/models/project_feature_spec.rb24
-rw-r--r--spec/models/project_spec.rb30
-rw-r--r--spec/support/helpers/project_helpers.rb25
-rw-r--r--spec/support/shared_contexts/policies/project_policy_table_shared_context.rb118
7 files changed, 210 insertions, 2 deletions
diff --git a/app/models/project.rb b/app/models/project.rb
index 609c9d0821d..3d732f06d34 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -483,7 +483,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
@@ -511,6 +511,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 31e55bf6be6..84f13c44826 100644
--- a/spec/models/project_feature_spec.rb
+++ b/spec/models/project_feature_spec.rb
@@ -178,4 +178,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 e5d08cfb1bb..2f6b2b16a4d 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3308,6 +3308,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