Read about our upcoming Code of Conduct on this issue

Commit 4f4adf77 authored by Mark Chao's avatar Mark Chao Committed by Bob Van Landuyt
Browse files

Fix private feature Elasticsearch leak

Add spec to test different combinations.
parent 908437f720af
......@@ -482,7 +482,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
......@@ -510,6 +510,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) }
......
---
title: Fix private feature Elasticsearch leak
merge_request:
author:
type: security
......@@ -3469,6 +3469,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) }
......
# 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
# 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
# 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
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment