Commit aeaf5860 authored by Toon Claes's avatar Toon Claes

Make the GroupFinder specs more strict

Ensure the results match exactly and project authorizations do allow access to
sibling groups/projects deeper down.

Also apply WHERE scopes before running the UNION, to increase performance.
parent eecd2102
...@@ -5,8 +5,10 @@ class GroupsFinder < UnionFinder ...@@ -5,8 +5,10 @@ class GroupsFinder < UnionFinder
end end
def execute def execute
groups = find_union(all_groups, Group).with_route.order_id_desc items = all_groups.map do |item|
by_parent(groups) by_parent(item)
end
find_union(items, Group).with_route.order_id_desc
end end
private private
...@@ -17,8 +19,6 @@ class GroupsFinder < UnionFinder ...@@ -17,8 +19,6 @@ class GroupsFinder < UnionFinder
groups = [] groups = []
if current_user if current_user
groups_for_ancestors = find_union([current_user.authorized_groups, authorized_project_groups], Group)
groups_for_descendants = current_user.authorized_groups
groups << Gitlab::GroupHierarchy.new(groups_for_ancestors, groups_for_descendants).all_groups groups << Gitlab::GroupHierarchy.new(groups_for_ancestors, groups_for_descendants).all_groups
end end
groups << Group.unscoped.public_to_user(current_user) groups << Group.unscoped.public_to_user(current_user)
...@@ -26,6 +26,14 @@ class GroupsFinder < UnionFinder ...@@ -26,6 +26,14 @@ class GroupsFinder < UnionFinder
groups groups
end end
def groups_for_ancestors
current_user.authorized_groups
end
def groups_for_descendants
current_user.groups
end
def by_parent(groups) def by_parent(groups)
return groups unless params[:parent] return groups unless params[:parent]
......
...@@ -45,23 +45,23 @@ describe GroupsFinder do ...@@ -45,23 +45,23 @@ describe GroupsFinder do
let!(:private_subgroup) { create(:group, :private, parent: parent_group) } let!(:private_subgroup) { create(:group, :private, parent: parent_group) }
context 'without a user' do context 'without a user' do
it 'only returns public subgroups' do it 'only returns parent and public subgroups' do
expect(described_class.new(nil, parent: parent_group).execute).to contain_exactly(public_subgroup) expect(described_class.new(nil).execute).to contain_exactly(parent_group, public_subgroup)
end end
end end
context 'with a user' do context 'with a user' do
subject { described_class.new(user, parent: parent_group).execute } subject { described_class.new(user).execute }
it 'returns public and internal subgroups' do it 'returns parent, public, and internal subgroups' do
is_expected.to contain_exactly(public_subgroup, internal_subgroup) is_expected.to contain_exactly(parent_group, public_subgroup, internal_subgroup)
end end
context 'being member' do context 'being member' do
it 'returns public subgroups, internal subgroups, and private subgroups user is member of' do it 'returns parent, public subgroups, internal subgroups, and private subgroups user is member of' do
private_subgroup.add_guest(user) private_subgroup.add_guest(user)
is_expected.to contain_exactly(public_subgroup, internal_subgroup, private_subgroup) is_expected.to contain_exactly(parent_group, public_subgroup, internal_subgroup, private_subgroup)
end end
end end
...@@ -74,24 +74,42 @@ describe GroupsFinder do ...@@ -74,24 +74,42 @@ describe GroupsFinder do
it 'returns all subgroups' do it 'returns all subgroups' do
parent_group.add_guest(user) parent_group.add_guest(user)
is_expected.to contain_exactly(public_subgroup, internal_subgroup, private_subgroup) is_expected.to contain_exactly(parent_group, public_subgroup, internal_subgroup, private_subgroup)
end end
end end
context 'authorized to private project' do context 'authorized to private project' do
it 'returns the subgroup of the project' do context 'project one level deep' do
subproject = create(:empty_project, :private, namespace: private_subgroup) let!(:subproject) { create(:empty_project, :private, namespace: private_subgroup) }
before do
subproject.add_guest(user) subproject.add_guest(user)
end
it 'includes the subgroup of the project' do
is_expected.to include(private_subgroup) is_expected.to include(private_subgroup)
end end
it 'returns all the parent groups if project is several levels deep' do it 'does not include private subgroups deeper down' do
private_subsubgroup = create(:group, :private, parent: private_subgroup) subsubgroup = create(:group, :private, parent: private_subgroup)
subsubproject = create(:empty_project, :private, namespace: private_subsubgroup)
is_expected.not_to include(subsubgroup)
end
end
context 'project two levels deep' do
let!(:private_subsubgroup) { create(:group, :private, parent: private_subgroup) }
let!(:subsubproject) { create(:empty_project, :private, namespace: private_subsubgroup) }
before do
subsubproject.add_guest(user) subsubproject.add_guest(user)
end
expect(described_class.new(user).execute).to include(private_subsubgroup, private_subgroup, parent_group) it 'returns all the ancestor groups' do
is_expected.to include(private_subsubgroup, private_subgroup, parent_group)
end
it 'returns the groups for a given parent' do
expect(described_class.new(user, parent: parent_group).execute).to include(private_subgroup)
end
end end
end 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