Commit 2afc3748 authored by charlie ablett's avatar charlie ablett

Merge branch 'epics-sgroup-check' into 'master'

Reduce number of permission checks for subgroups in EpicsFinder [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!59360
parents 7944fc12 f96a1b42
---
name: limit_epic_groups_query
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/59360
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/327624
milestone: '13.12'
type: development
group: group::product planning
default_enabled: false
# frozen_string_literal: true
# EpicsFinder
#
# Used to find and filter epics in a single group or a single group hierarchy.
# It can not be used for finding epics in multiple top-level groups.
#
# Params:
# iids: integer[]
# state: 'open' or 'closed' or 'all'
......@@ -97,6 +102,15 @@ class EpicsFinder < IssuableFinder
# all epics in all subgroups
next groups if can_read_all_epics_in_related_groups?(groups, include_confidential: false)
if Feature.enabled?(:limit_epic_groups_query, group)
next groups.public_to_user unless current_user
next groups.public_to_user(current_user) unless groups.user_is_member(current_user).exists?
end
# when traversal ids are enabled, we could avoid N+1 issue
# by taking all public groups plus groups where user is member
# and its descendants, but for now we have to check groups
# one by one
groups_user_can_read_epics(groups)
end
end
......@@ -221,11 +235,10 @@ class EpicsFinder < IssuableFinder
# `read_confidential_epic` policy. If that's the case we don't need to
# check membership on subgroups.
#
# `groups` is a list of groups in the same group hierarchy, by default
# these should be ordered by nested level in the group hierarchy in
# descending order (so top-level first), except if we fetch ancestors
# - in that case top-level group is group's root parent
parent = params.fetch(:include_ancestor_groups, false) ? groups.first.root_ancestor : group
# `groups` is a list of groups in the same group hierarchy, group is
# highest in the group hierarchy except if we fetch ancestors - in that
# case top-level group is group's root parent
parent = params.fetch(:include_ancestor_groups, false) ? group.root_ancestor : group
# If they can view confidential epics in this parent group they can
# definitely view confidential epics in subgroups.
......
......@@ -106,6 +106,8 @@ module EE
joins("INNER JOIN (#{epics_query.to_sql}) as epics on epics.group_id = namespaces.id")
end
scope :user_is_member, -> (user) { id_in(user.authorized_groups(with_minimal_access: false)) }
state_machine :ldap_sync_status, namespace: :ldap_sync, initial: :ready do
state :ready
state :started
......
......@@ -370,10 +370,12 @@ module EE
# Returns the groups a user has access to, either through a membership or a project authorization
override :authorized_groups
def authorized_groups
def authorized_groups(with_minimal_access: true)
return super() unless with_minimal_access
::Group.unscoped do
::Group.from_union([
super,
super(),
available_minimal_access_groups
])
end
......
......@@ -534,20 +534,59 @@ RSpec.describe EpicsFinder do
let_it_be(:public_group1) { create(:group, :public, parent: base_group) }
let_it_be(:public_epic1) { create(:epic, group: public_group1) }
let_it_be(:public_epic2) { create(:epic, :confidential, group: public_group1) }
let_it_be(:internal_group) { create(:group, :internal, parent: base_group) }
let_it_be(:internal_epic) { create(:epic, group: internal_group) }
let(:execute_params) { {} }
subject { described_class.new(search_user, group_id: base_group.id).execute(**execute_params) }
def execute
described_class.new(search_user, group_id: base_group.id).execute(**execute_params)
end
shared_examples 'avoids N+1 queries' do
it 'avoids N+1 queries on searched groups' do
execute # warm up
control = ActiveRecord::QueryRecorder.new(skip_cached: false) { execute }
create_list(:group, 5, :private, parent: base_group)
it 'returns only public epics' do
expect(subject).to match_array([base_epic2, public_epic1])
expect { execute }.not_to exceed_all_query_limit(control)
end
end
context 'when user is not set' do
let(:search_user) { nil }
it 'returns only public epics in public groups' do
expect(execute).to match_array([base_epic2, public_epic1])
end
it_behaves_like 'avoids N+1 queries'
end
context 'when user is not member of any groups being searched' do
it 'returns only public epics in public and internal groups' do
expect(execute).to match_array([base_epic2, public_epic1, internal_epic])
end
it_behaves_like 'avoids N+1 queries'
context 'when limit_epic_groups_query is disabled' do
before do
stub_feature_flags(limit_epic_groups_query: false)
end
it 'returns only public epics' do
expect(execute).to match_array([base_epic2, public_epic1, internal_epic])
end
end
end
context 'when skip_visibility_check is true' do
let(:execute_params) { { skip_visibility_check: true } }
it 'returns all epics' do
expect(subject).to match_array([base_epic1, base_epic2, private_epic1, private_epic2, public_epic1, public_epic2])
expect(execute).to match_array([base_epic1, base_epic2, private_epic1, private_epic2, public_epic1, public_epic2, internal_epic])
end
end
......@@ -557,17 +596,10 @@ RSpec.describe EpicsFinder do
end
it 'returns all nested epics' do
expect(subject).to match_array([base_epic1, base_epic2, private_epic1, private_epic2, public_epic1, public_epic2])
expect(execute).to match_array([base_epic1, base_epic2, private_epic1, private_epic2, public_epic1, public_epic2, internal_epic])
end
it 'does not execute more than 6 SQL queries' do
normal_query_count = 5
# sync_traversal_ids feature flag has to query for root_ancestor.
ff_query_count = 1
total_count = normal_query_count + ff_query_count
expect { subject }.not_to exceed_all_query_limit(total_count)
end
it_behaves_like 'avoids N+1 queries'
it 'does not check permission for subgroups because user inherits permission' do
finder = described_class.new(search_user, group_id: base_group.id)
......@@ -584,14 +616,14 @@ RSpec.describe EpicsFinder do
end
it 'returns also confidential epics from this subgroup' do
expect(subject).to match_array([base_epic2, private_epic1, private_epic2, public_epic1])
expect(execute).to match_array([base_epic2, private_epic1, private_epic2, public_epic1, internal_epic])
end
# if user is not member of top-level group, we need to check
# if he can read epics in each subgroup
it 'does not execute more than 15 SQL queries' do
it 'does not execute more than 17 SQL queries' do
# The limit here is fragile!
expect { subject }.not_to exceed_all_query_limit(15)
expect { execute }.not_to exceed_all_query_limit(17)
end
it 'checks permission for each subgroup' do
......@@ -609,7 +641,7 @@ RSpec.describe EpicsFinder do
end
it 'does not return any confidential epics in the base or subgroups' do
expect(subject).to match_array([base_epic2, private_epic1, public_epic1])
expect(execute).to match_array([base_epic2, private_epic1, public_epic1, internal_epic])
end
end
......@@ -619,7 +651,7 @@ RSpec.describe EpicsFinder do
end
it 'returns also confidential epics from this subgroup' do
expect(subject).to match_array([base_epic2, public_epic1, public_epic2])
expect(execute).to match_array([base_epic2, public_epic1, public_epic2, internal_epic])
end
end
end
......
......@@ -117,6 +117,25 @@ RSpec.describe Group do
expect(subject).to contain_exactly(group_with_no_pat_expiry_policy)
end
end
describe '.user_is_member' do
let_it_be(:user) { create(:user) }
let_it_be(:not_member_group) { create(:group) }
let_it_be(:shared_group) { create(:group) }
let_it_be(:direct_group) { create(:group) }
let_it_be(:inherited_group) { create(:group, parent: direct_group) }
let_it_be(:group_link) { create(:group_group_link, shared_group: shared_group, shared_with_group: direct_group) }
let_it_be(:minimal_access_group) { create(:group) }
before do
direct_group.add_guest(user)
create(:group_member, :minimal_access, user: user, source: minimal_access_group)
end
it 'returns only groups where user is direct or indirect member ignoring inheritance and minimal access level' do
expect(described_class.user_is_member(user)).to match_array([shared_group, direct_group])
end
end
end
describe 'validations' do
......
......@@ -1233,6 +1233,10 @@ RSpec.describe User do
end
it { is_expected.to contain_exactly private_group, project_group, minimal_access_group }
it 'ignores groups with minimal access if with_minimal_access=false' do
expect(user.authorized_groups(with_minimal_access: false)).to contain_exactly(private_group, project_group)
end
end
context 'feature available for specific groups only' do
......
......@@ -179,12 +179,14 @@ RSpec.describe 'Epics through GroupQuery' do
before do
group.reload
post_graphql(query, current_user: user)
end
it 'avoids n+1 queries when loading parent field' do
# warm up
post_graphql(query({ iids: [epic.iid] }), current_user: user)
control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do
post_graphql(query, current_user: user)
post_graphql(query({ iids: [epic.iid] }), current_user: user)
end.count
epics_with_parent = create_list(:epic, 3, group: group) do |epic|
......@@ -192,10 +194,10 @@ RSpec.describe 'Epics through GroupQuery' do
end
group.reload
# Added +1 to control_count due to an existing N+1 with licenses
# Added +5 to control_count due to an existing N+1 with licenses
expect do
post_graphql(query({ iids: epics_with_parent.pluck(:iid) }), current_user: user)
end.not_to exceed_all_query_limit(control_count + 1)
end.not_to exceed_all_query_limit(control_count + 5)
end
end
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