Commit deb45634 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Use `EXISTS` instead of `WHERE id IN (...)` for authorized groups

parent 524f6515
class GroupDescendantsFinder class GroupDescendantsFinder
include Gitlab::Allowable
attr_reader :current_user, :parent_group, :params attr_reader :current_user, :parent_group, :params
def initialize(current_user: nil, parent_group:, params: {}) def initialize(current_user: nil, parent_group:, params: {})
...@@ -40,7 +38,7 @@ class GroupDescendantsFinder ...@@ -40,7 +38,7 @@ class GroupDescendantsFinder
end end
def paginator def paginator
Gitlab::MultiCollectionPaginator.new(*collections, per_page: params[:per_page]) @paginator ||= Gitlab::MultiCollectionPaginator.new(*collections, per_page: params[:per_page])
end end
def direct_child_groups def direct_child_groups
...@@ -51,18 +49,21 @@ class GroupDescendantsFinder ...@@ -51,18 +49,21 @@ class GroupDescendantsFinder
def all_visible_descendant_groups def all_visible_descendant_groups
groups_table = Group.arel_table groups_table = Group.arel_table
visible_for_user = groups_table[:visibility_level] visible_to_user = groups_table[:visibility_level]
.in(Gitlab::VisibilityLevel.levels_for_user(current_user)) .in(Gitlab::VisibilityLevel.levels_for_user(current_user))
visible_for_user = if current_user if current_user
visible_projects = GroupsFinder.new(current_user).execute.as('visible') authorized_groups = GroupsFinder.new(current_user,
authorized = groups_table.project(1).from(visible_projects) all_available: false)
.where(visible_projects[:id].eq(groups_table[:id])) .execute.as('authorized')
.exists authorized_to_user = groups_table.project(1).from(authorized_groups)
visible_for_user.or(authorized) .where(authorized_groups[:id].eq(groups_table[:id]))
end .exists
visible_to_user = visible_to_user.or(authorized_to_user)
end
hierarchy_for_parent hierarchy_for_parent
.descendants .descendants
.where(visible_for_user) .where(visible_to_user)
end end
def subgroups_matching_filter def subgroups_matching_filter
...@@ -94,7 +95,6 @@ class GroupDescendantsFinder ...@@ -94,7 +95,6 @@ class GroupDescendantsFinder
def subgroups def subgroups
return Group.none unless Group.supports_nested_groups? return Group.none unless Group.supports_nested_groups?
return Group.none unless can?(current_user, :read_group, parent_group)
# When filtering subgroups, we want to find all matches withing the tree of # When filtering subgroups, we want to find all matches withing the tree of
# descendants to show to the user # descendants to show to the user
...@@ -122,8 +122,6 @@ class GroupDescendantsFinder ...@@ -122,8 +122,6 @@ class GroupDescendantsFinder
end end
def projects def projects
return Project.none unless can?(current_user, :read_group, parent_group)
projects = if params[:filter] projects = if params[:filter]
projects_matching_filter projects_matching_filter
else else
......
...@@ -39,7 +39,7 @@ describe GroupDescendantsFinder do ...@@ -39,7 +39,7 @@ describe GroupDescendantsFinder do
context 'with nested groups', :nested_groups do context 'with nested groups', :nested_groups do
let!(:project) { create(:project, namespace: group) } let!(:project) { create(:project, namespace: group) }
let!(:subgroup) { create(:group, parent: group) } let!(:subgroup) { create(:group, :private, parent: group) }
describe '#execute' do describe '#execute' do
it 'contains projects and subgroups' do it 'contains projects and subgroups' do
...@@ -59,6 +59,15 @@ describe GroupDescendantsFinder do ...@@ -59,6 +59,15 @@ describe GroupDescendantsFinder do
expect(finder.execute).to contain_exactly(public_subgroup, other_subgroup) expect(finder.execute).to contain_exactly(public_subgroup, other_subgroup)
end end
it 'only includes public groups when no user is given' do
public_subgroup = create(:group, :public, parent: group)
_private_subgroup = create(:group, :private, parent: group)
finder = described_class.new(current_user: nil, parent_group: group)
expect(finder.execute).to contain_exactly(public_subgroup)
end
context 'with a filter' do context 'with a filter' do
let(:params) { { filter: 'test' } } let(:params) { { filter: 'test' } }
...@@ -86,7 +95,7 @@ describe GroupDescendantsFinder do ...@@ -86,7 +95,7 @@ describe GroupDescendantsFinder do
context 'with matching children' do context 'with matching children' do
it 'includes a group that has a subgroup matching the query and its parent' do it 'includes a group that has a subgroup matching the query and its parent' do
matching_subgroup = create(:group, name: 'testgroup', parent: subgroup) matching_subgroup = create(:group, :private, name: 'testgroup', parent: subgroup)
expect(finder.execute).to contain_exactly(subgroup, matching_subgroup) expect(finder.execute).to contain_exactly(subgroup, matching_subgroup)
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