Removes duplicated members from api/projects/:id/members/all51854-api-to-get-all-project-group-members-returns-duplicates

When using the members/all api the same user was returned multiple times
when he was a member of the project/group and also of one of the
ancestor groups.
Now the member is returned only once giving priority to the membership
on the project and maintaining the same behaviour of the members UI.

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)