From a9827e0e18b532fb5cc3f227ce6c6bddaf7a960b Mon Sep 17 00:00:00 2001 From: Jacopo Date: Tue, 29 Jan 2019 19:10:37 +0100 Subject: 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 | 43 +++++++++++++++------ app/models/member.rb | 2 + ...ll-project-group-members-returns-duplicates.yml | 5 +++ doc/api/members.md | 4 +- lib/api/helpers/members_helpers.rb | 19 +--------- spec/finders/members_finder_spec.rb | 44 ++++++++++++++++++---- spec/requests/api/members_spec.rb | 13 +++++++ 7 files changed, 94 insertions(+), 36 deletions(-) create mode 100644 changelogs/unreleased/51854-api-to-get-all-project-group-members-returns-duplicates.yml 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) -- cgit v1.2.3