Commit 1db09731 authored by Jan Provaznik's avatar Jan Provaznik

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
parents 31315850 a9827e0e
...@@ -9,25 +9,18 @@ class MembersFinder ...@@ -9,25 +9,18 @@ class MembersFinder
@group = project.group @group = project.group
end end
# rubocop: disable CodeReuse/ActiveRecord def execute(include_descendants: false, include_invited_groups_members: false)
def execute(include_descendants: false)
project_members = project.project_members project_members = project.project_members
project_members = project_members.non_invite unless can?(current_user, :admin_project, project) project_members = project_members.non_invite unless can?(current_user, :admin_project, project)
if group union_members = group_union_members(include_descendants, include_invited_groups_members)
group_members = GroupMembersFinder.new(group).execute(include_descendants: include_descendants) # rubocop: disable CodeReuse/Finder
group_members = group_members.non_invite
union = Gitlab::SQL::Union.new([project_members, group_members], remove_duplicates: false) # rubocop: disable Gitlab/Union if union_members.any?
distinct_union_of_members(union_members << project_members)
sql = distinct_on(union)
Member.includes(:user).from("(#{sql}) AS #{Member.table_name}")
else else
project_members project_members
end end
end end
# rubocop: enable CodeReuse/ActiveRecord
def can?(*args) def can?(*args)
Ability.allowed?(*args) Ability.allowed?(*args)
...@@ -35,6 +28,34 @@ class MembersFinder ...@@ -35,6 +28,34 @@ class MembersFinder
private 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) def distinct_on(union)
# We're interested in a list of members without duplicates by user_id. # 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. # We prefer project members over group members, project members should go first.
......
...@@ -80,6 +80,8 @@ class Member < ApplicationRecord ...@@ -80,6 +80,8 @@ class Member < ApplicationRecord
scope :owners_and_masters, -> { owners_and_maintainers } # @deprecated scope :owners_and_masters, -> { owners_and_maintainers } # @deprecated
scope :with_user, -> (user) { where(user: user) } 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_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_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')) } scope :order_recent_sign_in, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.last_sign_in_at', 'DESC')) }
......
---
title: Removes duplicated members from api/projects/:id/members/all
merge_request: 24005
author: Jacopo Beschi @jacopo-beschi
type: changed
...@@ -62,7 +62,9 @@ Example response: ...@@ -62,7 +62,9 @@ Example response:
## List all members of a group or project including inherited members ## 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. 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 GET /groups/:id/members/all
......
...@@ -19,28 +19,13 @@ module API ...@@ -19,28 +19,13 @@ module API
.non_request .non_request
end end
# rubocop: disable CodeReuse/ActiveRecord
def find_all_members_for_project(project) def find_all_members_for_project(project)
shared_group_ids = project.project_group_links.pluck(:group_id) MembersFinder.new(project, current_user).execute(include_invited_groups_members: true)
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)
end end
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def find_all_members_for_group(group) def find_all_members_for_group(group)
source_ids = group.self_and_ancestors.pluck(:id) GroupMembersFinder.new(group).execute
Member.includes(:user)
.where(source_id: source_ids)
.where(source_type: 'Namespace')
end end
# rubocop: enable CodeReuse/ActiveRecord
end end
end end
end end
require 'spec_helper' require 'spec_helper'
describe MembersFinder, '#execute' do describe MembersFinder, '#execute' do
let(:group) { create(:group) } set(:group) { create(:group) }
let(:nested_group) { create(:group, :access_requestable, parent: group) } set(:nested_group) { create(:group, :access_requestable, parent: group) }
let(:project) { create(:project, namespace: nested_group) } set(:project) { create(:project, namespace: nested_group) }
let(:user1) { create(:user) } set(:user1) { create(:user) }
let(:user2) { create(:user) } set(:user2) { create(:user) }
let(:user3) { create(:user) } set(:user3) { create(:user) }
let(:user4) { create(:user) } set(:user4) { create(:user) }
it 'returns members for project and parent groups', :nested_groups do it 'returns members for project and parent groups', :nested_groups do
nested_group.request_access(user1) nested_group.request_access(user1)
...@@ -31,4 +31,34 @@ describe MembersFinder, '#execute' do ...@@ -31,4 +31,34 @@ describe MembersFinder, '#execute' do
expect(result.to_a).to match_array([member1, member2, member3]) expect(result.to_a).to match_array([member1, member2, member3])
end 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 end
...@@ -132,6 +132,19 @@ describe API::Members do ...@@ -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] 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 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 it 'finds all group members including inherited members' do
get api("/groups/#{nested_group.id}/members/all", developer) get api("/groups/#{nested_group.id}/members/all", developer)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment