From 7a3ba8e9845b89c9f3f37d43e8edfeaa9093cfdf Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 26 Sep 2017 20:06:08 +0200 Subject: [PATCH] Make sure the user only sees groups he's allowed to see --- app/finders/group_descendants_finder.rb | 14 ++++++++-- spec/finders/group_descendants_finder_spec.rb | 28 +++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index 4ed9c0ea39a..fca139062e5 100644 --- a/app/finders/group_descendants_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -73,13 +73,23 @@ class GroupDescendantsFinder all_available: true).execute end - def all_descendant_groups + def all_visible_descendant_groups + groups_table = Group.arel_table + visible_for_user = if current_user + groups_table[:id].in( + Arel::Nodes::SqlLiteral.new(GroupsFinder.new(current_user, all_available: true).execute.select(:id).to_sql) + ) + else + groups_table[:visibility_level].eq(Gitlab::VisibilityLevel::PUBLIC) + end + Gitlab::GroupHierarchy.new(Group.where(id: parent_group)) .base_and_descendants + .where(visible_for_user) end def subgroups_matching_filter - all_descendant_groups + all_visible_descendant_groups .where.not(id: parent_group) .search(params[:filter]) end diff --git a/spec/finders/group_descendants_finder_spec.rb b/spec/finders/group_descendants_finder_spec.rb index 09a773aaf68..7b9dfcbfad0 100644 --- a/spec/finders/group_descendants_finder_spec.rb +++ b/spec/finders/group_descendants_finder_spec.rb @@ -58,6 +58,19 @@ describe GroupDescendantsFinder do expect(found_group.preloaded_member_count).to eq(1) end + it 'does not include subgroups the user does not have access to' do + subgroup.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + + public_subgroup = create(:group, :public, parent: group, path: 'public-group') + other_subgroup = create(:group, :private, parent: group, path: 'visible-private-group') + other_user = create(:user) + other_subgroup.add_developer(other_user) + + finder = described_class.new(current_user: other_user, parent_group: group) + + expect(finder.execute).to contain_exactly(public_subgroup, other_subgroup) + end + context 'with a filter' do let(:params) { { filter: 'test' } } @@ -68,6 +81,21 @@ describe GroupDescendantsFinder do expect(finder.execute).to contain_exactly(matching_subgroup, matching_project) end + it 'does not include subgroups the user does not have access to' do + _invisible_subgroup = create(:group, :private, parent: group, name: 'test1') + other_subgroup = create(:group, :private, parent: group, name: 'test2') + public_subgroup = create(:group, :public, parent: group, name: 'test3') + other_subsubgroup = create(:group, :private, parent: other_subgroup, name: 'test4') + other_user = create(:user) + other_subgroup.add_developer(other_user) + + finder = described_class.new(current_user: other_user, + parent_group: group, + params: params) + + expect(finder.execute).to contain_exactly(other_subgroup, public_subgroup, other_subsubgroup) + end + context 'with matching children' do it 'includes a group that has a subgroup matching the query and its parent' do matching_subgroup = create(:group, name: 'testgroup', parent: subgroup)