diff options
author | Bob Van Landuyt <bob@vanlanduyt.co> | 2017-10-10 14:33:02 +0300 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2017-10-10 15:09:56 +0300 |
commit | deb45634ae841d64d1756c4cc0dc3c442e171ba9 (patch) | |
tree | 13827a92872cbc43a2b41e90998562d416c2a147 | |
parent | 524f65152fde2591a52d4c58d14c643ce379ec5b (diff) |
Use `EXISTS` instead of `WHERE id IN (...)` for authorized groups
-rw-r--r-- | app/finders/group_descendants_finder.rb | 30 | ||||
-rw-r--r-- | spec/finders/group_descendants_finder_spec.rb | 13 |
2 files changed, 25 insertions, 18 deletions
diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index 1e6523b70a2..3f231bd8b10 100644 --- a/app/finders/group_descendants_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -1,6 +1,4 @@ class GroupDescendantsFinder - include Gitlab::Allowable - attr_reader :current_user, :parent_group, :params def initialize(current_user: nil, parent_group:, params: {}) @@ -40,7 +38,7 @@ class GroupDescendantsFinder end def paginator - Gitlab::MultiCollectionPaginator.new(*collections, per_page: params[:per_page]) + @paginator ||= Gitlab::MultiCollectionPaginator.new(*collections, per_page: params[:per_page]) end def direct_child_groups @@ -51,18 +49,21 @@ class GroupDescendantsFinder def all_visible_descendant_groups groups_table = Group.arel_table - visible_for_user = groups_table[:visibility_level] - .in(Gitlab::VisibilityLevel.levels_for_user(current_user)) - visible_for_user = if current_user - visible_projects = GroupsFinder.new(current_user).execute.as('visible') - authorized = groups_table.project(1).from(visible_projects) - .where(visible_projects[:id].eq(groups_table[:id])) - .exists - visible_for_user.or(authorized) - end + visible_to_user = groups_table[:visibility_level] + .in(Gitlab::VisibilityLevel.levels_for_user(current_user)) + if current_user + authorized_groups = GroupsFinder.new(current_user, + all_available: false) + .execute.as('authorized') + authorized_to_user = groups_table.project(1).from(authorized_groups) + .where(authorized_groups[:id].eq(groups_table[:id])) + .exists + visible_to_user = visible_to_user.or(authorized_to_user) + end + hierarchy_for_parent .descendants - .where(visible_for_user) + .where(visible_to_user) end def subgroups_matching_filter @@ -94,7 +95,6 @@ class GroupDescendantsFinder def subgroups 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 # descendants to show to the user @@ -122,8 +122,6 @@ class GroupDescendantsFinder end def projects - return Project.none unless can?(current_user, :read_group, parent_group) - projects = if params[:filter] projects_matching_filter else diff --git a/spec/finders/group_descendants_finder_spec.rb b/spec/finders/group_descendants_finder_spec.rb index 86a7a793457..1aef49613ee 100644 --- a/spec/finders/group_descendants_finder_spec.rb +++ b/spec/finders/group_descendants_finder_spec.rb @@ -39,7 +39,7 @@ describe GroupDescendantsFinder do context 'with nested groups', :nested_groups do let!(:project) { create(:project, namespace: group) } - let!(:subgroup) { create(:group, parent: group) } + let!(:subgroup) { create(:group, :private, parent: group) } describe '#execute' do it 'contains projects and subgroups' do @@ -59,6 +59,15 @@ describe GroupDescendantsFinder do expect(finder.execute).to contain_exactly(public_subgroup, other_subgroup) 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 let(:params) { { filter: 'test' } } @@ -86,7 +95,7 @@ describe GroupDescendantsFinder do 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) + matching_subgroup = create(:group, :private, name: 'testgroup', parent: subgroup) expect(finder.execute).to contain_exactly(subgroup, matching_subgroup) end |