Merge branch 'group_members_all_api_fix' into 'master'

Shared groups fix for group/:id/members/all REST/GraphQL APIs

See merge request gitlab-org/gitlab!66778

app/finders/group_members_finder.rb

 # frozen_string_literal: true
 
 class GroupMembersFinder < UnionFinder
-  RELATIONS = %i(direct inherited descendants).freeze
+  RELATIONS = %i(direct inherited descendants shared_from_groups).freeze
   DEFAULT_RELATIONS = %i(direct inherited).freeze
   INVALID_RELATION_TYPE_ERROR_MSG = "is not a valid relation type. Valid relation types are #{RELATIONS.join(', ')}."
 
   RELATIONS_DESCRIPTIONS = {
     direct: 'Members in the group itself',
     inherited: "Members in the group's ancestor groups",
-    descendants: "Members in the group's subgroups"
+    descendants: "Members in the group's subgroups",
+    shared_from_groups: "Invited group's members"
   }.freeze
 
   include CreatedAtFilter
@@ -28,11 +29,7 @@ class GroupMembersFinder < UnionFinder
   end
 
   def execute(include_relations: DEFAULT_RELATIONS)
-    return filter_members(group_members_list) if include_relations == [:direct]
-
     groups = groups_by_relations(include_relations)
-    return GroupMember.none unless groups
-
     members = all_group_members(groups).distinct_on_user_with_max_access_level
 
     filter_members(members)
@@ -45,22 +42,14 @@ class GroupMembersFinder < UnionFinder
   def groups_by_relations(include_relations)
     check_relation_arguments!(include_relations)
 
-    case include_relations.sort
-    when [:inherited]
-      group.ancestors
-    when [:descendants]
-      group.descendants
-    when [:direct, :inherited]
-      group.self_and_ancestors
-    when [:descendants, :direct]
-      group.self_and_descendants
-    when [:descendants, :inherited]
-      find_union([group.ancestors, group.descendants], Group)
-    when [:descendants, :direct, :inherited]
-      group.self_and_hierarchy
-    else
-      nil
-    end
+    related_groups = []
+
+    related_groups << Group.by_id(group.id) if include_relations&.include?(:direct)
+    related_groups << group.ancestors if include_relations&.include?(:inherited)
+    related_groups << group.descendants if include_relations&.include?(:descendants)
+    related_groups << group.shared_with_groups.public_or_visible_to_user(user) if include_relations&.include?(:shared_from_groups)
+
+    find_union(related_groups, Group)
   end
 
   def filter_members(members)

app/models/members/group_member.rb

@@ -18,7 +18,7 @@ class GroupMember < Member
 
   default_scope { where(source_type: SOURCE_TYPE) } # rubocop:disable Cop/DefaultScope
 
-  scope :of_groups, ->(groups) { where(source_id: groups.select(:id)) }
+  scope :of_groups, ->(groups) { where(source_id: groups&.select(:id)) }
   scope :of_ldap_type, -> { where(ldap: true) }
   scope :count_users_by_group_id, -> { group(:source_id).count }
   scope :with_user, -> (user) { where(user: user) }

doc/api/graphql/reference/index.md

@@ -16555,6 +16555,7 @@ Group member relation.
 | <a id="groupmemberrelationdescendants"></a>`DESCENDANTS` | Members in the group's subgroups. |
 | <a id="groupmemberrelationdirect"></a>`DIRECT` | Members in the group itself. |
 | <a id="groupmemberrelationinherited"></a>`INHERITED` | Members in the group's ancestor groups. |
+| <a id="groupmemberrelationshared_from_groups"></a>`SHARED_FROM_GROUPS` | Invited group's members. |
 
 ### `GroupPermission`
 

doc/api/members.md

@@ -88,15 +88,20 @@ Example response:
 ]
 ```
 
-## List all members of a group or project including inherited members
+## List all members of a group or project including inherited and invited members 
 
-Gets a list of group or project members viewable by the authenticated user, including inherited members and permissions through ancestor groups.
+Gets a list of group or project members viewable by the authenticated user, including inherited members, invited users, and permissions through ancestor groups.
 
 If a user is a member of this group or project and also of one or more ancestor groups,
 only its membership with the highest `access_level` is returned.
 ([Improved](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56677) in GitLab 13.11.)
 This represents the effective permission of the user.
 
+Members from an invited group are returned if either:
+
+- The invited group is public.
+- The requester is also a member of the invited group.
+
 This function takes pagination parameters `page` and `per_page` to restrict the list of users.
 
 ```plaintext
@@ -202,11 +207,11 @@ Example response:
 }
 ```
 
-## Get a member of a group or project, including inherited members
+## Get a member of a group or project, including inherited and invited members
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/17744) in GitLab 12.4.
 
-Gets a member of a group or project, including members inherited through ancestor groups. See the corresponding [endpoint to list all inherited members](#list-all-members-of-a-group-or-project-including-inherited-members) for details.
+Gets a member of a group or project, including members inherited or invited through ancestor groups. See the corresponding [endpoint to list all inherited members](#list-all-members-of-a-group-or-project-including-inherited-and-invited-members) for details.
 
 ```plaintext
 GET /groups/:id/members/all/:user_id

lib/api/helpers/members_helpers.rb

@@ -50,7 +50,7 @@ module API
       end
 
       def find_all_members_for_group(group)
-        GroupMembersFinder.new(group).execute
+        GroupMembersFinder.new(group, current_user).execute(include_relations: [:inherited, :direct, :shared_from_groups])
       end
 
       def present_members(members)

spec/finders/group_members_finder_spec.rb

@@ -3,20 +3,29 @@
 require 'spec_helper'
 
 RSpec.describe GroupMembersFinder, '#execute' do
-  let(:group)         { create(:group) }
-  let(:sub_group)     { create(:group, parent: group) }
-  let(:sub_sub_group) { create(:group, parent: sub_group) }
-  let(:user1)         { create(:user) }
-  let(:user2)         { create(:user) }
-  let(:user3)         { create(:user) }
-  let(:user4)         { create(:user) }
-  let(:user5)         { create(:user, :two_factor_via_otp) }
+  let_it_be(:group)                { create(:group) }
+  let_it_be(:sub_group)            { create(:group, parent: group) }
+  let_it_be(:sub_sub_group)        { create(:group, parent: sub_group) }
+  let_it_be(:public_shared_group)  { create(:group, :public) }
+  let_it_be(:private_shared_group) { create(:group, :private) }
+  let_it_be(:user1)                { create(:user) }
+  let_it_be(:user2)                { create(:user) }
+  let_it_be(:user3)                { create(:user) }
+  let_it_be(:user4)                { create(:user) }
+  let_it_be(:user5)                { create(:user, :two_factor_via_otp) }
+
+  let!(:link) do
+    create(:group_group_link, shared_group: group,     shared_with_group: public_shared_group)
+    create(:group_group_link, shared_group: sub_group, shared_with_group: private_shared_group)
+  end
 
   let(:groups) do
     {
       group:                group,
       sub_group:            sub_group,
-      sub_sub_group: sub_sub_group
+      sub_sub_group:        sub_sub_group,
+      public_shared_group:  public_shared_group,
+      private_shared_group: private_shared_group
     }
   end
 
@@ -26,60 +35,61 @@ RSpec.describe GroupMembersFinder, '#execute' do
         user1_sub_sub_group:        create(:group_member, :maintainer, group: sub_sub_group,        user: user1),
         user1_sub_group:            create(:group_member, :developer,  group: sub_group,            user: user1),
         user1_group:                create(:group_member, :reporter,   group: group,                user: user1),
+        user1_public_shared_group:  create(:group_member, :maintainer, group: public_shared_group,  user: user1),
+        user1_private_shared_group: create(:group_member, :maintainer, group: private_shared_group, user: user1),
         user2_sub_sub_group:        create(:group_member, :reporter,   group: sub_sub_group,        user: user2),
         user2_sub_group:            create(:group_member, :developer,  group: sub_group,            user: user2),
         user2_group:                create(:group_member, :maintainer, group: group,                user: user2),
+        user2_public_shared_group:  create(:group_member, :developer,  group: public_shared_group,  user: user2),
+        user2_private_shared_group: create(:group_member, :developer,  group: private_shared_group, user: user2),
         user3_sub_sub_group:        create(:group_member, :developer,  group: sub_sub_group,        user: user3, expires_at: 1.day.from_now),
         user3_sub_group:            create(:group_member, :developer,  group: sub_group,            user: user3, expires_at: 2.days.from_now),
         user3_group:                create(:group_member, :reporter,   group: group,                user: user3),
+        user3_public_shared_group:  create(:group_member, :reporter,   group: public_shared_group,  user: user3),
+        user3_private_shared_group: create(:group_member, :reporter,   group: private_shared_group, user: user3),
         user4_sub_sub_group:        create(:group_member, :reporter,   group: sub_sub_group,        user: user4),
         user4_sub_group:            create(:group_member, :developer,  group: sub_group,            user: user4, expires_at: 1.day.from_now),
-        user4_group:         create(:group_member, :developer,  group: group,         user: user4, expires_at: 2.days.from_now)
+        user4_group:                create(:group_member, :developer,  group: group,                user: user4, expires_at: 2.days.from_now),
+        user4_public_shared_group:  create(:group_member, :developer,  group: public_shared_group,  user: user4),
+        user4_private_shared_group: create(:group_member, :developer,  group: private_shared_group, user: user4)
       }
     end
 
     it 'raises an error if a non-supported relation type is used' do
       expect do
         described_class.new(group).execute(include_relations: [:direct, :invalid_relation_type])
-      end.to raise_error(ArgumentError, "invalid_relation_type is not a valid relation type. Valid relation types are direct, inherited, descendants.")
+      end.to raise_error(ArgumentError, "invalid_relation_type is not a valid relation type. Valid relation types are direct, inherited, descendants, shared_from_groups.")
     end
 
     using RSpec::Parameterized::TableSyntax
 
     where(:subject_relations, :subject_group, :expected_members) do
-      nil                                 | :group         | [:user1_group,         :user2_group,         :user3_group,         :user4_group]
+      []                                                       | :group         | []
+      GroupMembersFinder::DEFAULT_RELATIONS                    | :group         | [:user1_group, :user2_group, :user3_group, :user4_group]
       [:direct]                                                | :group         | [:user1_group, :user2_group, :user3_group, :user4_group]
       [:inherited]                                             | :group         | []
       [:descendants]                                           | :group         | [:user1_sub_sub_group, :user2_sub_group, :user3_sub_group, :user4_sub_group]
-      [:direct, :inherited]               | :group         | [:user1_group,         :user2_group,         :user3_group,         :user4_group]
-      [:direct, :descendants]             | :group         | [:user1_sub_sub_group, :user2_group,         :user3_sub_group,     :user4_group]
-      [:descendants, :inherited]          | :group         | [:user1_sub_sub_group, :user2_sub_group,     :user3_sub_group,     :user4_sub_group]
-      [:direct, :descendants, :inherited] | :group         | [:user1_sub_sub_group, :user2_group,         :user3_sub_group,     :user4_group]
-      nil                                 | :sub_group     | [:user1_sub_group,     :user2_group,         :user3_sub_group,     :user4_group]
+      [:shared_from_groups]                                    | :group         | [:user1_public_shared_group, :user2_public_shared_group, :user3_public_shared_group, :user4_public_shared_group]
+      [:direct, :inherited, :descendants, :shared_from_groups] | :group         | [:user1_sub_sub_group, :user2_group, :user3_sub_group, :user4_public_shared_group]
+      []                                                       | :sub_group     | []
+      GroupMembersFinder::DEFAULT_RELATIONS                    | :sub_group     | [:user1_sub_group, :user2_group, :user3_sub_group, :user4_group]
       [:direct]                                                | :sub_group     | [:user1_sub_group, :user2_sub_group, :user3_sub_group, :user4_sub_group]
       [:inherited]                                             | :sub_group     | [:user1_group, :user2_group, :user3_group, :user4_group]
       [:descendants]                                           | :sub_group     | [:user1_sub_sub_group, :user2_sub_sub_group, :user3_sub_sub_group, :user4_sub_sub_group]
-      [:direct, :inherited]               | :sub_group     | [:user1_sub_group,     :user2_group,         :user3_sub_group,     :user4_group]
-      [:direct, :descendants]             | :sub_group     | [:user1_sub_sub_group, :user2_sub_group,     :user3_sub_group,     :user4_sub_group]
-      [:descendants, :inherited]          | :sub_group     | [:user1_sub_sub_group, :user2_group,         :user3_sub_sub_group, :user4_group]
-      [:direct, :descendants, :inherited] | :sub_group     | [:user1_sub_sub_group, :user2_group,         :user3_sub_group,     :user4_group]
-      nil                                 | :sub_sub_group | [:user1_sub_sub_group, :user2_group,         :user3_sub_group,     :user4_group]
+      [:shared_from_groups]                                    | :sub_group     | []
+      [:direct, :inherited, :descendants, :shared_from_groups] | :sub_group     | [:user1_sub_sub_group, :user2_group, :user3_sub_group, :user4_group]
+      []                                                       | :sub_sub_group | []
+      GroupMembersFinder::DEFAULT_RELATIONS                    | :sub_sub_group | [:user1_sub_sub_group, :user2_group, :user3_sub_group, :user4_group]
       [:direct]                                                | :sub_sub_group | [:user1_sub_sub_group, :user2_sub_sub_group, :user3_sub_sub_group, :user4_sub_sub_group]
       [:inherited]                                             | :sub_sub_group | [:user1_sub_group, :user2_group, :user3_sub_group, :user4_group]
       [:descendants]                                           | :sub_sub_group | []
-      [:direct, :inherited]               | :sub_sub_group | [:user1_sub_sub_group, :user2_group,         :user3_sub_group,     :user4_group]
-      [:direct, :descendants]             | :sub_sub_group | [:user1_sub_sub_group, :user2_sub_sub_group, :user3_sub_sub_group, :user4_sub_sub_group]
-      [:descendants, :inherited]          | :sub_sub_group | [:user1_sub_group,     :user2_group,         :user3_sub_group,     :user4_group]
-      [:direct, :descendants, :inherited] | :sub_sub_group | [:user1_sub_sub_group, :user2_group,         :user3_sub_group,     :user4_group]
+      [:shared_from_groups]                                    | :sub_sub_group | []
+      [:direct, :inherited, :descendants, :shared_from_groups] | :sub_sub_group | [:user1_sub_sub_group, :user2_group, :user3_sub_group, :user4_group]
     end
 
     with_them do
       it 'returns correct members' do
-        result = if subject_relations
-                   described_class.new(groups[subject_group]).execute(include_relations: subject_relations)
-                 else
-                   described_class.new(groups[subject_group]).execute
-                 end
+        result = described_class.new(groups[subject_group]).execute(include_relations: subject_relations)
 
         expect(result.to_a).to match_array(expected_members.map { |name| members[name] })
       end

spec/graphql/types/group_member_relation_enum_spec.rb

@@ -6,6 +6,6 @@ RSpec.describe Types::GroupMemberRelationEnum do
   specify { expect(described_class.graphql_name).to eq('GroupMemberRelation') }
 
   it 'exposes all the existing group member relation type values' do
-    expect(described_class.values.keys).to contain_exactly('DIRECT', 'INHERITED', 'DESCENDANTS')
+    expect(described_class.values.keys).to contain_exactly('DIRECT', 'INHERITED', 'DESCENDANTS', 'SHARED_FROM_GROUPS')
   end
 end

spec/requests/api/graphql/group/group_members_spec.rb

@@ -56,12 +56,16 @@ RSpec.describe 'getting group members information' do
   context 'member relations' do
     let_it_be(:child_group) { create(:group, :public, parent: parent_group) }
     let_it_be(:grandchild_group) { create(:group, :public, parent: child_group) }
+    let_it_be(:invited_group) { create(:group, :public) }
     let_it_be(:child_user) { create(:user) }
     let_it_be(:grandchild_user) { create(:user) }
+    let_it_be(:invited_user) { create(:user) }
+    let_it_be(:group_link) { create(:group_group_link, shared_group: child_group, shared_with_group: invited_group) }
 
     before_all do
       child_group.add_guest(child_user)
       grandchild_group.add_guest(grandchild_user)
+      invited_group.add_guest(invited_user)
     end
 
     it 'returns direct members' do
@@ -71,6 +75,13 @@ RSpec.describe 'getting group members information' do
       expect_array_response(child_user)
     end
 
+    it 'returns invited members plus inherited members' do
+      fetch_members(group: child_group, args: { relations: [:DIRECT, :INHERITED, :SHARED_FROM_GROUPS] })
+
+      expect(graphql_errors).to be_nil
+      expect_array_response(invited_user, user_1, user_2, child_user)
+    end
+
     it 'returns direct and inherited members' do
       fetch_members(group: child_group, args: { relations: [:DIRECT, :INHERITED] })