Removes duplicated members from api/projects/:id/members/all

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.

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.

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')) }

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

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

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

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

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)