Commit ca4d2750 authored by Mark Chao's avatar Mark Chao

Fix private feature Elasticsearch leak

Add spec to test different combinations.
parent fce20855
......@@ -477,7 +477,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
......@@ -505,6 +505,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) }
......
---
title: Fix private feature Elasticsearch leak
merge_request:
author:
type: security
......@@ -128,7 +128,7 @@ module Elastic
# anonymous users.
# Pick private, internal and public projects the user is a member of.
# Pick all private projects for admins & auditors.
conditions = [pick_projects_by_membership(project_ids, features)]
conditions = [pick_projects_by_membership(project_ids, user, features)]
if public_and_internal_projects
# Skip internal projects for anonymous and external users.
......@@ -152,15 +152,28 @@ module Elastic
# Admins & auditors are given access to all private projects. Access to
# internal or public projects where the project feature is private is not
# granted here.
def pick_projects_by_membership(project_ids, features = nil)
condition =
def pick_projects_by_membership(project_ids, user, features = nil)
if features.nil?
if project_ids == :any
{ term: { visibility_level: Project::PRIVATE } }
return { term: { visibility_level: Project::PRIVATE } }
else
{ terms: { id: project_ids } }
return { terms: { id: project_ids } }
end
end
Array(features).map do |feature|
condition =
if project_ids == :any
{ term: { visibility_level: Project::PRIVATE } }
else
{ terms: { id: filter_ids_by_feature(project_ids, user, feature) } }
end
limit =
{ terms: { "#{feature}_access_level" => [::ProjectFeature::ENABLED, ::ProjectFeature::PRIVATE] } }
limit_by_feature(condition, features, include_members_only: true)
{ bool: { filter: [condition, limit] } }
end
end
# Grant access to projects of the specified visibility level to the user.
......@@ -198,6 +211,13 @@ module Elastic
{ bool: { filter: [condition, limit] } }
end
end
def filter_ids_by_feature(project_ids, user, feature)
Project
.id_in(project_ids)
.filter_by_feature_visibility(feature, user)
.pluck_primary_key
end
end
end
end
......@@ -278,20 +278,26 @@ module Gitlab
end
def wiki_filter
blob_filter(:wiki_access_level, visible_for_guests: true)
blob_filter(:wiki, visible_for_guests: true)
end
def repository_filter
blob_filter(:repository_access_level)
blob_filter(:repository)
end
def blob_filter(project_feature_name, visible_for_guests: false)
def blob_filter(feature, visible_for_guests: false)
project_ids = visible_for_guests ? limit_project_ids : non_guest_project_ids
key_name = "#{feature}_access_level"
conditions =
if project_ids == :any
[{ exists: { field: "id" } }]
else
project_ids = Project
.id_in(project_ids)
.filter_by_feature_visibility(feature, current_user)
.pluck_primary_key
[{ terms: { id: project_ids } }]
end
......@@ -300,7 +306,7 @@ module Gitlab
bool: {
filter: [
{ term: { visibility_level: Project::PUBLIC } },
{ term: { project_feature_name => ProjectFeature::ENABLED } }
{ term: { key_name => ProjectFeature::ENABLED } }
]
}
}
......@@ -310,7 +316,7 @@ module Gitlab
bool: {
filter: [
{ term: { visibility_level: Project::INTERNAL } },
{ term: { project_feature_name => ProjectFeature::ENABLED } }
{ term: { key_name => ProjectFeature::ENABLED } }
]
}
}
......@@ -323,7 +329,7 @@ module Gitlab
query: {
bool: {
should: conditions,
must_not: { term: { project_feature_name => ProjectFeature::DISABLED } }
must_not: { term: { key_name => ProjectFeature::DISABLED } }
}
}
}
......
......@@ -4,7 +4,7 @@ require 'spec_helper'
describe Gitlab::Elastic::ProjectSearchResults, :elastic do
let(:user) { create(:user) }
let(:project) { create(:project, :repository) }
let(:project) { create(:project, :public, :repository) }
let(:query) { 'hello world' }
before do
......
......@@ -8,8 +8,8 @@ describe Gitlab::Elastic::SearchResults, :elastic do
end
let(:user) { create(:user) }
let(:project_1) { create(:project, :repository, :wiki_repo) }
let(:project_2) { create(:project, :repository, :wiki_repo) }
let(:project_1) { create(:project, :public, :repository, :wiki_repo) }
let(:project_2) { create(:project, :public, :repository, :wiki_repo) }
let(:limit_project_ids) { [project_1.id] }
describe 'counts' do
......@@ -154,7 +154,7 @@ describe Gitlab::Elastic::SearchResults, :elastic do
end
it 'lists issue when search by a valid iid' do
results = described_class.new(user, '#2', limit_project_ids)
results = described_class.new(user, '#2', limit_project_ids, nil, false)
issues = results.objects('issues')
expect(issues).not_to include @issue_1
......@@ -218,8 +218,8 @@ describe Gitlab::Elastic::SearchResults, :elastic do
end
describe 'confidential issues' do
let(:project_3) { create(:project) }
let(:project_4) { create(:project) }
let(:project_3) { create(:project, :public) }
let(:project_4) { create(:project, :public) }
let(:limit_project_ids) { [project_1.id, project_2.id, project_3.id] }
let(:author) { create(:user) }
let(:assignee) { create(:user) }
......@@ -318,8 +318,8 @@ describe Gitlab::Elastic::SearchResults, :elastic do
expect(issues).to include @security_issue_2
expect(issues).to include @security_issue_3
expect(issues).to include @security_issue_4
expect(issues).not_to include @security_issue_5
expect(results.issues_count).to eq 5
expect(issues).to include @security_issue_5
expect(results.issues_count).to eq 6
end
end
......@@ -403,8 +403,8 @@ describe Gitlab::Elastic::SearchResults, :elastic do
expect(issues).not_to include @security_issue_2
expect(issues).to include @security_issue_3
expect(issues).to include @security_issue_4
expect(issues).not_to include @security_issue_5
expect(results.issues_count).to eq 3
expect(issues).to include @security_issue_5
expect(results.issues_count).to eq 4
end
end
end
......@@ -539,6 +539,7 @@ describe Gitlab::Elastic::SearchResults, :elastic do
it 'finds blobs from public projects only' do
project_2 = create :project, :repository, :private
project_2.repository.index_commits_and_blobs
project_2.add_reporter(user)
Gitlab::Elastic::Helper.refresh_index
results = described_class.new(user, 'def', [project_1.id])
......@@ -670,6 +671,7 @@ describe Gitlab::Elastic::SearchResults, :elastic do
project_2 = create :project, :repository, :private, :wiki_repo
project_2.wiki.create_page('index_page', 'term')
project_2.wiki.index_wiki_blobs
project_2.add_guest(user)
Gitlab::Elastic::Helper.refresh_index
expect(results.wiki_blobs_count).to eq 1
......@@ -706,6 +708,10 @@ describe Gitlab::Elastic::SearchResults, :elastic do
context 'search by member' do
let(:limit_project_ids) { [project_1.id] }
before do
project_1.add_guest(user)
end
it { is_expected.not_to be_empty }
end
......@@ -736,6 +742,7 @@ describe Gitlab::Elastic::SearchResults, :elastic do
it 'finds commits from public projects only' do
project_2 = create :project, :private, :repository
project_2.repository.index_commits_and_blobs
project_2.add_reporter(user)
Gitlab::Elastic::Helper.refresh_index
results = described_class.new(user, 'add', [project_1.id])
......
......@@ -7,7 +7,8 @@ describe Issue, :elastic do
stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
end
let(:project) { create :project }
let(:project) { create :project, :public }
let(:admin) { create :user, :admin }
context 'when limited indexing is on' do
set(:project) { create :project, name: 'test1' }
......@@ -71,7 +72,7 @@ describe Issue, :elastic do
expect(described_class.elastic_search('(term1 | term2 | term3) +bla-bla', options: options).total_count).to eq(2)
expect(described_class.elastic_search(Issue.last.to_reference, options: options).total_count).to eq(1)
expect(described_class.elastic_search('bla-bla', options: { project_ids: :any }).total_count).to eq(3)
expect(described_class.elastic_search('bla-bla', options: { project_ids: :any, public_and_internal_projects: true }).total_count).to eq(3)
end
it "returns json with all needed elements" do
......
......@@ -7,6 +7,8 @@ describe MergeRequest, :elastic do
stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
end
let(:admin) { create(:user, :admin) }
it_behaves_like 'limited indexing is enabled' do
set(:object) { create :merge_request, source_project: project }
set(:group) { create(:group) }
......@@ -17,7 +19,7 @@ describe MergeRequest, :elastic do
end
it "searches merge requests" do
project = create :project, :repository
project = create :project, :public, :repository
Sidekiq::Testing.inline! do
create :merge_request, title: 'bla-bla term1', source_project: project
......@@ -35,7 +37,7 @@ describe MergeRequest, :elastic do
expect(described_class.elastic_search('term1 | term2 | term3', options: options).total_count).to eq(2)
expect(described_class.elastic_search(MergeRequest.last.to_reference, options: options).total_count).to eq(1)
expect(described_class.elastic_search('term3', options: options).total_count).to eq(0)
expect(described_class.elastic_search('term3', options: { project_ids: :any }).total_count).to eq(1)
expect(described_class.elastic_search('term3', options: { project_ids: :any, public_and_internal_projects: true }).total_count).to eq(1)
end
it "returns json with all needed elements" do
......
......@@ -3,10 +3,99 @@
require 'spec_helper'
describe Search::GlobalService do
include SearchResultHelpers
include ProjectHelpers
using RSpec::Parameterized::TableSyntax
let(:user) { create(:user) }
before do
stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
end
it_behaves_like 'EE search service shared examples', ::Gitlab::SearchResults, ::Gitlab::Elastic::SearchResults do
let(:scope) { nil }
let(:service) { described_class.new(user, search: '*') }
end
context 'visibility', :elastic do
include_context 'ProjectPolicyTable context'
set(:group) { create(:group) }
let(:project) { create(:project, project_level, namespace: group) }
let(:project2) { create(:project, project_level) }
let(:user) { create_user_from_membership(project, membership) }
context 'merge request' do
let!(:merge_request) { create :merge_request, target_project: project, source_project: project }
where(:project_level, :feature_access_level, :membership, :expected_count) do
permission_table_for_reporter_feature_access
end
with_them do
it "respects visibility" do
[project, project2].each do |project|
update_feature_access_level(project, feature_access_level)
end
Gitlab::Elastic::Helper.refresh_index
expect_search_results(user, 'merge_requests', expected_count: expected_count) do |user|
described_class.new(user, search: merge_request.title).execute
end
end
end
end
context 'code' do
let!(:project) { create(:project, project_level, :repository, namespace: group ) }
let!(:project2) { create(:project, project_level, :repository) }
where(:project_level, :feature_access_level, :membership, :expected_count) do
permission_table_for_guest_feature_access
end
with_them do
it "respects visibility" do
[project, project2].each do |project|
update_feature_access_level(project, feature_access_level)
ElasticCommitIndexerWorker.new.perform(project.id)
end
Gitlab::Elastic::Helper.refresh_index
expect_search_results(user, 'commits', expected_count: expected_count) do |user|
described_class.new(user, search: 'initial').execute
end
expect_search_results(user, 'blobs', expected_count: expected_count) do |user|
described_class.new(user, search: '.gitmodules').execute
end
end
end
end
context 'project' do
where(:project_level, :membership, :expected_count) do
permission_table_for_project_access
end
with_them do
it "respects visibility" do
ElasticCommitIndexerWorker.new.perform(project.id)
Gitlab::Elastic::Helper.refresh_index
expected_objects = expected_count == 1 ? [project] : []
expect_search_results(
user,
'projects',
expected_count: expected_count,
expected_objects: expected_objects
) do |user|
described_class.new(user, search: project.name).execute
end
end
end
end
end
end
......@@ -3,7 +3,16 @@
require 'spec_helper'
describe Search::GroupService, :elastic do
include SearchResultHelpers
include ProjectHelpers
using RSpec::Parameterized::TableSyntax
let(:user) { create(:user) }
let(:group) { create(:group) }
before do
stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
end
it_behaves_like 'EE search service shared examples', ::Gitlab::GroupSearchResults, ::Gitlab::Elastic::GroupSearchResults do
let(:scope) { create(:group) }
......@@ -57,4 +66,88 @@ describe Search::GroupService, :elastic do
end
end
end
context 'visibility' do
include_context 'ProjectPolicyTable context'
set(:group) { create(:group) }
let!(:project) { create(:project, project_level, namespace: group) }
let!(:project2) { create(:project, project_level) }
let(:user) { create_user_from_membership(project, membership) }
context 'merge request' do
let!(:merge_request) { create :merge_request, target_project: project, source_project: project }
let!(:merge_request2) { create :merge_request, target_project: project2, source_project: project2, title: merge_request.title }
where(:project_level, :feature_access_level, :membership, :expected_count) do
permission_table_for_reporter_feature_access
end
with_them do
it "respects visibility" do
[project, project2].each do |project|
update_feature_access_level(project, feature_access_level)
end
Gitlab::Elastic::Helper.refresh_index
expect_search_results(user, 'merge_requests', expected_count: expected_count) do |user|
described_class.new(user, group, search: merge_request.title).execute
end
end
end
end
context 'code' do
let!(:project) { create(:project, project_level, :repository, namespace: group ) }
where(:project_level, :feature_access_level, :membership, :expected_count) do
permission_table_for_guest_feature_access
end
with_them do
it "respects visibility" do
[project, project2].each do |project|
update_feature_access_level(project, feature_access_level)
ElasticCommitIndexerWorker.new.perform(project.id)
end
ElasticCommitIndexerWorker.new.perform(project.id)
Gitlab::Elastic::Helper.refresh_index
expect_search_results(user, 'commits', expected_count: expected_count) do |user|
described_class.new(user, group, search: 'initial').execute
end
expect_search_results(user, 'blobs', expected_count: expected_count) do |user|
described_class.new(user, group, search: '.gitmodules').execute
end
end
end
end
context 'project search' do
let(:project) { create(:project, project_level, namespace: group) }
where(:project_level, :membership, :expected_count) do
permission_table_for_project_access
end
with_them do
it "respects visibility" do
ElasticCommitIndexerWorker.new.perform(project.id)
Gitlab::Elastic::Helper.refresh_index
expected_objects = expected_count == 1 ? [project] : []
expect_search_results(
user,
'projects',
expected_count: expected_count,
expected_objects: expected_objects
) do |user|
described_class.new(user, group, search: project.name).execute
end
end
end
end
end
end
......@@ -3,9 +3,75 @@
require 'spec_helper'
describe Search::ProjectService do
include SearchResultHelpers
include ProjectHelpers
using RSpec::Parameterized::TableSyntax
before do
stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
end
it_behaves_like 'EE search service shared examples', ::Gitlab::ProjectSearchResults, ::Gitlab::Elastic::ProjectSearchResults do
let(:user) { scope.owner }
let(:scope) { create(:project) }
let(:service) { described_class.new(scope, user, search: '*') }
end
context 'visibility', :elastic do
include_context 'ProjectPolicyTable context'
set(:group) { create(:group) }
let!(:project) { create(:project, project_level, namespace: group) }
let!(:project2) { create(:project, project_level) }
let(:user) { create_user_from_membership(project, membership) }
context 'merge request' do
let!(:merge_request) { create :merge_request, target_project: project, source_project: project }
let!(:merge_request2) { create :merge_request, target_project: project2, source_project: project2, title: merge_request.title }
where(:project_level, :feature_access_level, :membership, :expected_count) do
permission_table_for_reporter_feature_access
end
with_them do
it "respects visibility" do
[project, project2].each do |project|
update_feature_access_level(project, feature_access_level)
end
Gitlab::Elastic::Helper.refresh_index
expect_search_results(user, 'merge_requests', expected_count: expected_count) do |user|
described_class.new(project, user, search: merge_request.title).execute
end
end
end
end
context 'code' do
let!(:project) { create(:project, project_level, :repository, namespace: group ) }
let!(:project2) { create(:project, project_level, :repository) }
where(:project_level, :feature_access_level, :membership, :expected_count) do
permission_table_for_guest_feature_access
end
with_them do
it "respects visibility" do
[project, project2].each do |project|
update_feature_access_level(project, feature_access_level)
ElasticCommitIndexerWorker.new.perform(project.id)
end
Gitlab::Elastic::Helper.refresh_index
expect_search_results(user, 'commits', expected_count: expected_count) do |user|
described_class.new(project, user, search: 'initial').execute
end
expect_search_results(user, 'blobs', expected_count: expected_count) do |user|
described_class.new(project, user, search: '.gitmodules').execute
end
end
end
end
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
......@@ -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) }
......
# 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
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