Merge branch '51854-api-to-get-all-project-group-members-returns-duplicates' into 'master'

Resolve "API to get all project/group members returns duplicates"

Closes #51854

See merge request gitlab-org/gitlab-ce!24005

7 files changed, 94 insertions, 36 deletions

diff --git a/app/finders/members_finder.rb b/app/finders/members_finder.rb
index f90a7868102..917de249104 100644
--- a/app/finders/members_finder.rb
+++ b/app/finders/members_finder.rb
@@ -9,25 +9,18 @@ class MembersFinder
     @group = project.group
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
-  def execute(include_descendants: false)
+  def execute(include_descendants: false, include_invited_groups_members: false)
     project_members = project.project_members
     project_members = project_members.non_invite unless can?(current_user, :admin_project, project)
 
-    if group
-      group_members = GroupMembersFinder.new(group).execute(include_descendants: include_descendants) # rubocop: disable CodeReuse/Finder
-      group_members = group_members.non_invite
+    union_members = group_union_members(include_descendants, include_invited_groups_members)
 
-      union = Gitlab::SQL::Union.new([project_members, group_members], remove_duplicates: false) # rubocop: disable Gitlab/Union
-
-      sql = distinct_on(union)
-
-      Member.includes(:user).from("(#{sql}) AS #{Member.table_name}")
+    if union_members.any?
+      distinct_union_of_members(union_members << project_members)
     else
       project_members
     end
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def can?(*args)
     Ability.allowed?(*args)
@@ -35,6 +28,34 @@ class MembersFinder
 
   private
 
+  def group_union_members(include_descendants, include_invited_groups_members)
+    [].tap do |members|
+      members << direct_group_members(include_descendants) if group
+      members << project_invited_groups_members if include_invited_groups_members
+    end
+  end
+
+  def direct_group_members(include_descendants)
+    GroupMembersFinder.new(group).execute(include_descendants: include_descendants).non_invite # rubocop: disable CodeReuse/Finder
+  end
+
+  def project_invited_groups_members
+    invited_groups_ids_including_ancestors = Gitlab::ObjectHierarchy
+      .new(project.invited_groups)
+      .base_and_ancestors
+      .public_or_visible_to_user(current_user)
+      .select(:id)
+
+    GroupMember.with_source_id(invited_groups_ids_including_ancestors)
+  end
+
+  def distinct_union_of_members(union_members)
+    union = Gitlab::SQL::Union.new(union_members, remove_duplicates: false) # rubocop: disable Gitlab/Union
+    sql = distinct_on(union)
+
+    Member.includes(:user).from([Arel.sql("(#{sql}) AS #{Member.table_name}")]) # rubocop: disable CodeReuse/ActiveRecord
+  end
+
   def distinct_on(union)
     # We're interested in a list of members without duplicates by user_id.
     # We prefer project members over group members, project members should go first.
diff --git a/app/models/member.rb b/app/models/member.rb
index 83b4f5b29c4..c7583434148 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -80,6 +80,8 @@ class Member < ApplicationRecord
   scope :owners_and_masters,  -> { owners_and_maintainers } # @deprecated
   scope :with_user, -> (user) { where(user: user) }
 
+  scope :with_source_id, ->(source_id) { where(source_id: source_id) }
+
   scope :order_name_asc, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.name', 'ASC')) }
   scope :order_name_desc, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.name', 'DESC')) }
   scope :order_recent_sign_in, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.last_sign_in_at', 'DESC')) }
diff --git a/changelogs/unreleased/51854-api-to-get-all-project-group-members-returns-duplicates.yml b/changelogs/unreleased/51854-api-to-get-all-project-group-members-returns-duplicates.yml
new file mode 100644
index 00000000000..4e16b95ec11
--- /dev/null
+++ b/changelogs/unreleased/51854-api-to-get-all-project-group-members-returns-duplicates.yml
@@ -0,0 +1,5 @@
+---
+title: Removes duplicated members from api/projects/:id/members/all
+merge_request: 24005
+author: Jacopo Beschi @jacopo-beschi
+type: changed
diff --git a/doc/api/members.md b/doc/api/members.md
index 0593d2c20ea..8784d577f99 100644
--- a/doc/api/members.md
+++ b/doc/api/members.md
@@ -62,7 +62,9 @@ Example response:
 ## List all members of a group or project including inherited members
 
 Gets a list of group or project members viewable by the authenticated user, including inherited members through ancestor groups.
-Returns multiple times the same user (with different member attributes) when the user is a member of the project/group and of one or more ancestor group.
+When a user is a member of the project/group and of one or more ancestor groups the user is returned only once with the project access_level (if exists)
+or the access_level for the user in the first group which he belongs to in the project groups ancestors chain.
+**Note:** We plan to [change](https://gitlab.com/gitlab-org/gitlab-ce/issues/62284) this behavior to return highest access_level instead.
 
 ```
 GET /groups/:id/members/all
diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb
index 73d58ee7f37..1395ffadab9 100644
--- a/lib/api/helpers/members_helpers.rb
+++ b/lib/api/helpers/members_helpers.rb
@@ -19,28 +19,13 @@ module API
           .non_request
       end
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def find_all_members_for_project(project)
-        shared_group_ids = project.project_group_links.pluck(:group_id)
-        project_group_ids = project.group&.self_and_ancestors&.pluck(:id)
-        source_ids = [project.id, project_group_ids, shared_group_ids]
-          .flatten
-          .compact
-        Member.includes(:user)
-          .joins(user: :project_authorizations)
-          .where(project_authorizations: { project_id: project.id })
-          .where(source_id: source_ids)
+        MembersFinder.new(project, current_user).execute(include_invited_groups_members: true)
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def find_all_members_for_group(group)
-        source_ids = group.self_and_ancestors.pluck(:id)
-        Member.includes(:user)
-          .where(source_id: source_ids)
-          .where(source_type: 'Namespace')
+        GroupMembersFinder.new(group).execute
       end
-      # rubocop: enable CodeReuse/ActiveRecord
     end
   end
 end
diff --git a/spec/finders/members_finder_spec.rb b/spec/finders/members_finder_spec.rb
index db48f00cd74..83348457caa 100644
--- a/spec/finders/members_finder_spec.rb
+++ b/spec/finders/members_finder_spec.rb
@@ -1,13 +1,13 @@
 require 'spec_helper'
 
 describe MembersFinder, '#execute' do
-  let(:group)        { create(:group) }
-  let(:nested_group) { create(:group, :access_requestable, parent: group) }
-  let(:project)      { create(:project, namespace: nested_group) }
-  let(:user1)        { create(:user) }
-  let(:user2)        { create(:user) }
-  let(:user3)        { create(:user) }
-  let(:user4)        { create(:user) }
+  set(:group)        { create(:group) }
+  set(:nested_group) { create(:group, :access_requestable, parent: group) }
+  set(:project)      { create(:project, namespace: nested_group) }
+  set(:user1)        { create(:user) }
+  set(:user2)        { create(:user) }
+  set(:user3)        { create(:user) }
+  set(:user4)        { create(:user) }
 
   it 'returns members for project and parent groups', :nested_groups do
     nested_group.request_access(user1)
@@ -31,4 +31,34 @@ describe MembersFinder, '#execute' do
 
     expect(result.to_a).to match_array([member1, member2, member3])
   end
+
+  context 'when include_invited_groups_members == true', :nested_groups do
+    subject { described_class.new(project, user2).execute(include_invited_groups_members: true) }
+
+    set(:linked_group) { create(:group, :public, :access_requestable) }
+    set(:nested_linked_group) { create(:group, parent: linked_group) }
+    set(:linked_group_member) { linked_group.add_developer(user1) }
+    set(:nested_linked_group_member) { nested_linked_group.add_developer(user2) }
+
+    it 'includes all the invited_groups members including members inherited from ancestor groups', :nested_groups do
+      create(:project_group_link, project: project, group: nested_linked_group)
+
+      expect(subject).to contain_exactly(linked_group_member, nested_linked_group_member)
+    end
+
+    it 'includes all the invited_groups members' do
+      create(:project_group_link, project: project, group: linked_group)
+
+      expect(subject).to contain_exactly(linked_group_member)
+    end
+
+    it 'excludes group_members not visible to the user' do
+      create(:project_group_link, project: project, group: linked_group)
+      private_linked_group = create(:group, :private)
+      private_linked_group.add_developer(user3)
+      create(:project_group_link, project: project, group: private_linked_group)
+
+      expect(subject).to contain_exactly(linked_group_member)
+    end
+  end
 end
diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb
index 48869cab4da..55f38079b1f 100644
--- a/spec/requests/api/members_spec.rb
+++ b/spec/requests/api/members_spec.rb
@@ -132,6 +132,19 @@ describe API::Members do
       expect(json_response.map { |u| u['id'] }).to match_array [maintainer.id, developer.id, nested_user.id, project_user.id, linked_group_user.id]
     end
 
+    it 'returns only one member for each user without returning duplicated members' do
+      linked_group.add_developer(developer)
+
+      get api("/projects/#{project.id}/members/all", developer)
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(response).to include_pagination_headers
+      expect(json_response).to be_an Array
+      expect(json_response.map { |u| u['id'] }).to eq [developer.id, maintainer.id, nested_user.id, project_user.id, linked_group_user.id]
+      expect(json_response.map { |u| u['access_level'] }).to eq [Gitlab::Access::DEVELOPER, Gitlab::Access::OWNER, Gitlab::Access::DEVELOPER,
+                                                                 Gitlab::Access::DEVELOPER, Gitlab::Access::DEVELOPER]
+    end
+
     it 'finds all group members including inherited members' do
       get api("/groups/#{nested_group.id}/members/all", developer)