diff options
-rw-r--r-- | app/finders/members_finder.rb | 43 | ||||
-rw-r--r-- | app/models/member.rb | 2 | ||||
-rw-r--r-- | changelogs/unreleased/51854-api-to-get-all-project-group-members-returns-duplicates.yml | 5 | ||||
-rw-r--r-- | doc/api/members.md | 4 | ||||
-rw-r--r-- | lib/api/helpers/members_helpers.rb | 19 | ||||
-rw-r--r-- | spec/finders/members_finder_spec.rb | 44 | ||||
-rw-r--r-- | spec/requests/api/members_spec.rb | 13 |
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) |