From f579c7fcd8eb13002dd5bba358dce02c0e0d87a1 Mon Sep 17 00:00:00 2001
From: Abdul Wadood <awadood@gitlab.com>
Date: Thu, 11 Apr 2024 12:12:08 +0000
Subject: [PATCH] Return all visible groups for the Organization.groups GraphQL
query
As we want to display all visible groups on the organization groups
dashboard on /-/organizations/<organization path>/groups_and_projects.
Changelog: other
---
app/finders/organizations/groups_finder.rb | 28 +++++++++
.../organizations/groups_resolver.rb | 10 ++-
.../resolve_all_organization_groups.yml | 9 +++
.../organizations/groups_finder_spec.rb | 63 +++++++++++++++++++
.../organizations/organization_query_spec.rb | 37 ++++++++++-
5 files changed, 143 insertions(+), 4 deletions(-)
create mode 100644 app/finders/organizations/groups_finder.rb
create mode 100644 config/feature_flags/gitlab_com_derisk/resolve_all_organization_groups.yml
create mode 100644 spec/finders/organizations/groups_finder_spec.rb
diff --git a/app/finders/organizations/groups_finder.rb b/app/finders/organizations/groups_finder.rb
new file mode 100644
index 0000000000000..a4925664b743a
--- /dev/null
+++ b/app/finders/organizations/groups_finder.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module Organizations
+ class GroupsFinder < GroupsFinder
+ def execute
+ groups = find_union(filtered_groups, Group)
+
+ unless default_organization?
+ cte = Gitlab::SQL::CTE.new(:filtered_groups_cte, groups, materialized: false)
+ groups = Group.with(cte.to_arel).from(cte.alias_to(Group.arel_table)) # rubocop: disable CodeReuse/ActiveRecord -- CTE use
+ end
+
+ sort(groups).with_route
+ end
+
+ private
+
+ def default_organization?
+ params[:organization]&.default? == true
+ end
+
+ def all_groups
+ return [membership_groups, Group.none].compact if default_organization?
+
+ super
+ end
+ end
+end
diff --git a/app/graphql/resolvers/organizations/groups_resolver.rb b/app/graphql/resolvers/organizations/groups_resolver.rb
index 10c0c9545b087..d6ec89a924ae2 100644
--- a/app/graphql/resolvers/organizations/groups_resolver.rb
+++ b/app/graphql/resolvers/organizations/groups_resolver.rb
@@ -13,8 +13,16 @@ class GroupsResolver < Resolvers::GroupsResolver
alias_method :organization, :object
+ def resolve_groups(**args)
+ ::Organizations::GroupsFinder.new(context[:current_user], finder_params(args)).execute
+ end
+
def finder_params(args)
- extra_args = { organization: organization, include_ancestors: false, all_available: false }
+ extra_args = if Feature.enabled?(:resolve_all_organization_groups, current_user)
+ { organization: organization }
+ else
+ { organization: organization, include_ancestors: false, all_available: false }
+ end
super.merge(extra_args)
end
diff --git a/config/feature_flags/gitlab_com_derisk/resolve_all_organization_groups.yml b/config/feature_flags/gitlab_com_derisk/resolve_all_organization_groups.yml
new file mode 100644
index 0000000000000..9189c029bf4ee
--- /dev/null
+++ b/config/feature_flags/gitlab_com_derisk/resolve_all_organization_groups.yml
@@ -0,0 +1,9 @@
+---
+name: resolve_all_organization_groups
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/444218
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/146600
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/444981
+milestone: '16.11'
+group: group::tenant scale
+type: gitlab_com_derisk
+default_enabled: false
diff --git a/spec/finders/organizations/groups_finder_spec.rb b/spec/finders/organizations/groups_finder_spec.rb
new file mode 100644
index 0000000000000..2d2ee009f177b
--- /dev/null
+++ b/spec/finders/organizations/groups_finder_spec.rb
@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Organizations::GroupsFinder, feature_category: :groups_and_projects do
+ describe '#execute' do
+ let_it_be(:organization_user) { create(:organization_user) }
+ let_it_be(:organization) { organization_user.organization }
+ let_it_be(:user) { organization_user.user }
+ let_it_be(:public_group) { create(:group, name: 'public-group', organization: organization) }
+ let_it_be(:outside_organization_group) { create(:group) }
+ let_it_be(:private_group) { create(:group, :private, name: 'private-group', organization: organization) }
+ let_it_be(:no_access_group_in_org) { create(:group, :private, name: 'no-access', organization: organization) }
+
+ let(:current_user) { user }
+ let(:params) { { organization: organization } }
+ let(:finder) { described_class.new(current_user, params) }
+
+ subject(:result) { finder.execute.to_a }
+
+ before_all do
+ private_group.add_developer(user)
+ public_group.add_developer(user)
+ outside_organization_group.add_developer(user)
+ end
+
+ context 'when user is only authorized to read the public group' do
+ let(:current_user) { create(:user) }
+
+ it { is_expected.to contain_exactly(public_group) }
+ end
+
+ it 'return all groups inside the organization' do
+ expect(result).to contain_exactly(public_group, private_group)
+ end
+
+ it 'creates a CTE with filtered groups' do
+ allow(Gitlab::SQL::CTE).to receive(:new).and_call_original
+ expect(Gitlab::SQL::CTE).to receive(:new).with(
+ :filtered_groups_cte, kind_of(ActiveRecord::Relation), materialized: false
+ )
+
+ result
+ end
+
+ context 'with default organization' do
+ before do
+ allow(organization).to receive(:default?).and_return(true)
+ end
+
+ it { is_expected.to contain_exactly(public_group, private_group) }
+
+ it 'does not build a query using the CTE' do
+ allow(Gitlab::SQL::CTE).to receive(:new).and_call_original
+ expect(Gitlab::SQL::CTE).not_to receive(:new).with(
+ :filtered_groups_cte, kind_of(ActiveRecord::Relation), materialized: false
+ )
+
+ result
+ end
+ end
+ end
+end
diff --git a/spec/requests/api/graphql/organizations/organization_query_spec.rb b/spec/requests/api/graphql/organizations/organization_query_spec.rb
index 5bd0b27586661..83d09ebf97123 100644
--- a/spec/requests/api/graphql/organizations/organization_query_spec.rb
+++ b/spec/requests/api/graphql/organizations/organization_query_spec.rb
@@ -28,6 +28,8 @@
create(:group, name: 'other-group', organization: organization) { |g| g.add_developer(user) }
end
+ let_it_be(:organization_group) { create(:group, organization: organization) }
+
subject(:request_organization) { post_graphql(query, current_user: current_user) }
context 'when the user does not have access to the organization' do
@@ -110,6 +112,7 @@ def run_query
context 'when requesting groups' do
let(:groups) { graphql_data_at(:organization, :groups, :nodes) }
let_it_be(:parent_group) { create(:group, name: 'parent-group', organization: organization) }
+ let_it_be(:parent_group_global_id) { parent_group.to_global_id.to_s }
let_it_be(:public_group) do
create(:group, name: 'public-group', parent: parent_group, organization: organization)
end
@@ -125,10 +128,38 @@ def run_query
create(:group) { |g| g.add_developer(user) } # outside organization
end
- it 'does not return ancestors of authorized groups' do
+ it 'returns ancestors of authorized groups' do
+ request_organization
+
+ expect(groups.pluck('id')).to include(parent_group_global_id)
+ end
+
+ it 'returns all visible groups' do
request_organization
- expect(groups.pluck('id')).not_to include(parent_group.to_global_id.to_s)
+ expected_groups = [parent_group, public_group, private_group, other_group, organization_group]
+ .map { |group| group.to_global_id.to_s }
+ expect(groups.pluck('id')).to match_array(expected_groups)
+ end
+
+ context 'when resolve_all_organization_groups feature flag is disabled' do
+ before do
+ stub_feature_flags(resolve_all_organization_groups: false)
+ end
+
+ it 'does not return ancestors of authorized groups' do
+ request_organization
+
+ expect(groups.pluck('id')).not_to include(parent_group_global_id)
+ end
+
+ it 'does not return all visible groups' do
+ request_organization
+
+ visible_groups = [parent_group, organization_group]
+ .map { |group| group.to_global_id.to_s }
+ expect(groups.pluck('id')).not_to include(*visible_groups)
+ end
end
context 'with `search` argument' do
@@ -154,7 +185,7 @@ def run_query
end
describe 'group sorting' do
- let_it_be(:authorized_groups) { [public_group, private_group, other_group] }
+ let_it_be(:authorized_groups) { [parent_group, public_group, private_group, other_group, organization_group] }
let_it_be(:first_param) { 2 }
let_it_be(:data_path) { [:organization, :groups] }
--
GitLab