Merge branch 'xanf-improve-members-filtering' into 'master'

Resolve "Allow filtering for project-specific members in group members list"

See merge request gitlab-org/gitlab!18842

app/controllers/concerns/membership_actions.rb

@@ -144,4 +144,15 @@ module MembershipActions
         end
       end
   end
+
+  def requested_relations
+    case params[:with_inherited_permissions].presence
+    when 'exclude'
+      [:direct]
+    when 'only'
+      [:inherited]
+    else
+      [:inherited, :direct]
+    end
+  end
 end

app/controllers/groups/group_members_controller.rb

@@ -24,8 +24,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
 
     @sort = params[:sort].presence || sort_value_name
     @project = @group.projects.find(params[:project_id]) if params[:project_id]
-
-    @members = GroupMembersFinder.new(@group).execute
+    @members = find_members
 
     if can_manage_members
       @invited_members = @members.invite
@@ -52,6 +51,12 @@ class Groups::GroupMembersController < Groups::ApplicationController
 
   # MembershipActions concern
   alias_method :membershipable, :group
+
+  private
+
+  def find_members
+    GroupMembersFinder.new(@group).execute(include_relations: requested_relations)
+  end
 end
 
 Groups::GroupMembersController.prepend_if_ee('EE::Groups::GroupMembersController')

app/controllers/projects/project_members_controller.rb

@@ -17,7 +17,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController
     @skip_groups << @project.namespace_id unless @project.personal?
     @skip_groups += @project.group.ancestors.pluck(:id) if @project.group
 
-    @project_members = MembersFinder.new(@project, current_user).execute
+    @project_members = MembersFinder.new(@project, current_user).execute(include_relations: requested_relations)
 
     if params[:search].present?
       @project_members = @project_members.joins(:user).merge(User.search(params[:search]))

app/finders/group_members_finder.rb

@@ -6,15 +6,15 @@ class GroupMembersFinder < UnionFinder
   end
 
   # rubocop: disable CodeReuse/ActiveRecord
-  def execute(include_descendants: false)
+  def execute(include_relations: [:inherited, :direct])
     group_members = @group.members
     relations = []
 
-    return group_members unless @group.parent || include_descendants
+    return group_members if include_relations == [:direct]
 
-    relations << group_members
+    relations << group_members if include_relations.include?(:direct)
 
-    if @group.parent
+    if include_relations.include?(:inherited) && @group.parent
       parents_members = GroupMember.non_request
         .where(source_id: @group.ancestors.select(:id))
         .where.not(user_id: @group.users.select(:id))
@@ -22,7 +22,7 @@ class GroupMembersFinder < UnionFinder
       relations << parents_members
     end
 
-    if include_descendants
+    if include_relations.include?(:descendants)
       descendant_members = GroupMember.non_request
         .where(source_id: @group.descendants.select(:id))
         .where.not(user_id: @group.users.select(:id))

app/finders/members_finder.rb

@@ -9,14 +9,18 @@ class MembersFinder
     @group = project.group
   end
 
-  def execute(include_descendants: false, include_invited_groups_members: false)
+  def execute(include_relations: [:inherited, :direct])
     project_members = project.project_members
     project_members = project_members.non_invite unless can?(current_user, :admin_project, project)
 
-    union_members = group_union_members(include_descendants, include_invited_groups_members)
+    return project_members if include_relations == [:direct]
+
+    union_members = group_union_members(include_relations)
+
+    union_members << project_members if include_relations.include?(:direct)
 
     if union_members.any?
-      distinct_union_of_members(union_members << project_members)
+      distinct_union_of_members(union_members)
     else
       project_members
     end
@@ -28,15 +32,17 @@ class MembersFinder
 
   private
 
-  def group_union_members(include_descendants, include_invited_groups_members)
+  def group_union_members(include_relations)
     [].tap do |members|
-      members << direct_group_members(include_descendants) if group
-      members << project_invited_groups_members if include_invited_groups_members
+      members << direct_group_members(include_relations.include?(:descendants)) if group
+      members << project_invited_groups_members if include_relations.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
+    requested_relations = [:inherited, :direct]
+    requested_relations << :descendants if include_descendants
+    GroupMembersFinder.new(group).execute(include_relations: requested_relations).non_invite # rubocop: disable CodeReuse/Finder
   end
 
   def project_invited_groups_members

app/views/shared/members/_sort_dropdown.html.haml

@@ -8,3 +8,13 @@
       %li
         = link_to filter_group_project_member_path(sort: value), class: ("is-active" if @sort == value) do
           = title
+    %li.divider
+    %li{ data: { 'qa-selector': 'filter-members-with-inherited-permissions' } }
+      = link_to filter_group_project_member_path(with_inherited_permissions: nil), class: ("is-active" unless params[:with_inherited_permissions].present?) do
+        = _("Show all members")
+    %li{ data: { 'qa-selector': 'filter-members-with-inherited-permissions' } }
+      = link_to filter_group_project_member_path(with_inherited_permissions: 'exclude'), class: ("is-active" if params[:with_inherited_permissions] == 'exclude') do
+        = _("Show only direct members")
+    %li{ data: { 'qa-selector': 'filter-members-with-inherited-permissions' } }
+      = link_to filter_group_project_member_path(with_inherited_permissions: 'only'), class: ("is-active" if params[:with_inherited_permissions] == 'only') do
+        = _("Show only inherited members")

changelogs/unreleased/xanf-improve-members-filtering.yml

+---
+title: Added filtering of inherited members for subgroups
+merge_request: 18842
+author:
+type: added

doc/user/group/subgroups/index.md

@@ -4,7 +4,7 @@ type: reference, howto, concepts
 
 # Subgroups
 
->[Introduced](https://gitlab.com/gitlab-org/gitlab-foss/issues/2772) in GitLab 9.0.
+> [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/issues/2772) in GitLab 9.0.
 
 Subgroups, also known as nested groups or hierarchical groups, allow you to have up to 20
 levels of groups.
@@ -142,6 +142,16 @@ From the image above, we can deduce the following things:
 - Administrator is the Owner and member of **all** subgroups and for that reason,
   as with User3, there is no indication of an ancestor group.
 
+[From](https://gitlab.com/gitlab-org/gitlab/issues/21727) GitLab 12.6, you can filter
+this list using dropdown on the right side:
+
+![Group members filter](img/group_members_filter_v12_6.png)
+
+- **Show only direct members** displays only Administrator and User3, since these are
+  the only users that belong to group `four`, which is the one we're inspecting.
+- **Show only inherited members** displays User0, User1 and User2, no matter which group
+  above the hierarchy is the source of inherited permissions.
+
 ### Overriding the ancestor group membership
 
 NOTE: **Note:**
@@ -186,7 +196,7 @@ Here's a list of what you can't do with subgroups:
 
 [ce-2772]: https://gitlab.com/gitlab-org/gitlab-foss/issues/2772
 [permissions]: ../../permissions.md#group-members-permissions
-[reserved]:  ../../reserved_names.md
+[reserved]: ../../reserved_names.md
 [issue]: https://gitlab.com/gitlab-org/gitlab-foss/issues/30472#note_27747600
 
 <!-- ## Troubleshooting

doc/user/project/members/index.md

@@ -10,6 +10,31 @@ or import a new user to your project.
 To view, edit, add, and remove project's members, go to your
 project's **Settings > Members**.
 
+## Inherited membership
+
+When your project belongs to the group, group members inherit the membership and permission
+level for the project from the group.
+
+![Project members page](img/project_members.png)
+
+From the image above, we can deduce the following things:
+
+- There are 3 members that have access to the project.
+- User0 is a Reporter and has inherited their permissions from group `demo`
+  which contains current project.
+- For User1 there is no indication of a group, therefore they belong directly
+  to the project we're inspecting.
+- Administrator is the Owner and member of **all** groups and for that reason,
+  there is an indication of an ancestor group and inherited Owner permissions.
+
+[From](https://gitlab.com/gitlab-org/gitlab/issues/21727), you can filter this list
+using dropdown on the right side:
+
+![Project members filter](img/project_members_filter_v12_6.png)
+
+- **Show only direct members** displays only User1.
+- **Show only inherited members** displays User0 and Administrator.
+
 ## Add a user
 
 Right next to **People**, start typing the name or username of the user you

ee/app/finders/boards/users_finder.rb

@@ -8,7 +8,7 @@ module Boards
     end
 
     def execute
-      finder_service.execute(include_descendants: true).non_invite
+      finder_service.execute(include_relations: [:direct, :descendants, :inherited]).non_invite
     end
 
     private

ee/spec/finders/boards/users_finder_spec.rb

@@ -10,6 +10,12 @@ describe Boards::UsersFinder do
       let(:project) { create(:project) }
       let(:board) { create(:board, project: project) }
 
+      it 'requests correct relations' do
+        expect_any_instance_of(MembersFinder).to receive(:execute).with(include_relations: [:direct, :descendants, :inherited]).and_call_original
+
+        subject.execute
+      end
+
       it 'finds ProjectMembers with MemberFinder' do
         results = subject.execute
 
@@ -27,6 +33,12 @@ describe Boards::UsersFinder do
         group.add_developer(user)
       end
 
+      it 'requests correct relations' do
+        expect_any_instance_of(GroupMembersFinder).to receive(:execute).with(include_relations: [:direct, :descendants, :inherited]).and_call_original
+
+        subject.execute
+      end
+
       it 'finds GroupMembers with GroupMemberFinder' do
         results = subject.execute
 

lib/api/helpers/members_helpers.rb

@@ -29,7 +29,7 @@ module API
       end
 
       def find_all_members_for_project(project)
-        MembersFinder.new(project, current_user).execute(include_invited_groups_members: true)
+        MembersFinder.new(project, current_user).execute(include_relations: [:inherited, :direct, :invited_groups_members])
       end
 
       def find_all_members_for_group(group)

locale/gitlab.pot

@@ -16189,6 +16189,9 @@ msgstr ""
 msgid "Show all activity"
 msgstr ""
 
+msgid "Show all members"
+msgstr ""
+
 msgid "Show archived projects"
 msgstr ""
 
@@ -16216,6 +16219,12 @@ msgstr ""
 msgid "Show latest version"
 msgstr ""
 
+msgid "Show only direct members"
+msgstr ""
+
+msgid "Show only inherited members"
+msgstr ""
+
 msgid "Show parent pages"
 msgstr ""
 

spec/controllers/groups/group_members_controller_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 describe Groups::GroupMembersController do
   include ExternalAuthorizationServiceHelpers
 
-  let(:user)  { create(:user) }
+  let(:user) { create(:user) }
   let(:group) { create(:group, :public) }
   let(:membership) { create(:group_member, group: group) }
 
@@ -49,6 +49,35 @@ describe Groups::GroupMembersController do
         expect(assigns(:invited_members).count).to eq(1)
       end
     end
+
+    context 'when user has owner access to subgroup' do
+      let(:nested_group) { create(:group, parent: group) }
+      let(:nested_group_user) { create(:user) }
+
+      before do
+        group.add_owner(user)
+        nested_group.add_owner(nested_group_user)
+        sign_in(user)
+      end
+
+      it 'lists inherited group members by default' do
+        get :index, params: { group_id: nested_group }
+
+        expect(assigns(:members).map(&:user_id)).to contain_exactly(user.id, nested_group_user.id)
+      end
+
+      it 'lists direct group members only' do
+        get :index, params: { group_id: nested_group, with_inherited_permissions: 'exclude' }
+
+        expect(assigns(:members).map(&:user_id)).to contain_exactly(nested_group_user.id)
+      end
+
+      it 'lists inherited group members only' do
+        get :index, params: { group_id: nested_group, with_inherited_permissions: 'only' }
+
+        expect(assigns(:members).map(&:user_id)).to contain_exactly(user.id)
+      end
+    end
   end
 
   describe 'POST create' do

spec/controllers/projects/project_members_controller_spec.rb

@@ -4,6 +4,7 @@ require('spec_helper')
 
 describe Projects::ProjectMembersController do
   let(:user) { create(:user) }
+  let(:group) { create(:group, :public) }
   let(:project) { create(:project, :public) }
 
   describe 'GET index' do
@@ -12,6 +13,35 @@ describe Projects::ProjectMembersController do
 
       expect(response).to have_gitlab_http_status(200)
     end
+
+    context 'when project belongs to group' do
+      let(:user_in_group) { create(:user) }
+      let(:project_in_group) { create(:project, :public, group: group) }
+
+      before do
+        group.add_owner(user_in_group)
+        project_in_group.add_maintainer(user)
+        sign_in(user)
+      end
+
+      it 'lists inherited project members by default' do
+        get :index, params: { namespace_id: project_in_group.namespace, project_id: project_in_group }
+
+        expect(assigns(:project_members).map(&:user_id)).to contain_exactly(user.id, user_in_group.id)
+      end
+
+      it 'lists direct project members only' do
+        get :index, params: { namespace_id: project_in_group.namespace, project_id: project_in_group, with_inherited_permissions: 'exclude' }
+
+        expect(assigns(:project_members).map(&:user_id)).to contain_exactly(user.id)
+      end
+
+      it 'lists inherited project members only' do
+        get :index, params: { namespace_id: project_in_group.namespace, project_id: project_in_group, with_inherited_permissions: 'only' }
+
+        expect(assigns(:project_members).map(&:user_id)).to contain_exactly(user_in_group.id)
+      end
+    end
   end
 
   describe 'POST create' do

spec/features/groups/members/filter_members_spec.rb

@@ -3,42 +3,71 @@
 require 'spec_helper'
 
 describe 'Groups > Members > Filter members' do
-  let(:user)          { create(:user) }
-  let(:user_with_2fa) { create(:user, :two_factor_via_otp) }
-  let(:group)         { create(:group) }
+  let(:user)              { create(:user) }
+  let(:nested_group_user) { create(:user) }
+  let(:user_with_2fa)     { create(:user, :two_factor_via_otp) }
+  let(:group)             { create(:group) }
+  let(:nested_group)      { create(:group, parent: group) }
 
   before do
     group.add_owner(user)
     group.add_maintainer(user_with_2fa)
+    nested_group.add_maintainer(nested_group_user)
 
     sign_in(user)
   end
 
   it 'shows all members' do
-    visit_members_list
+    visit_members_list(group)
 
-    expect(first_member).to include(user.name)
-    expect(second_member).to include(user_with_2fa.name)
+    expect(member(0)).to include(user.name)
+    expect(member(1)).to include(user_with_2fa.name)
     expect(page).to have_css('.member-filter-2fa-dropdown .dropdown-toggle-text', text: 'Everyone')
   end
 
   it 'shows only 2FA members' do
-    visit_members_list(two_factor: 'enabled')
+    visit_members_list(group, two_factor: 'enabled')
 
-    expect(first_member).to include(user_with_2fa.name)
+    expect(member(0)).to include(user_with_2fa.name)
     expect(members_list.size).to eq(1)
     expect(page).to have_css('.member-filter-2fa-dropdown .dropdown-toggle-text', text: 'Enabled')
   end
 
   it 'shows only non 2FA members' do
-    visit_members_list(two_factor: 'disabled')
+    visit_members_list(group, two_factor: 'disabled')
 
-    expect(first_member).to include(user.name)
+    expect(member(0)).to include(user.name)
     expect(members_list.size).to eq(1)
     expect(page).to have_css('.member-filter-2fa-dropdown .dropdown-toggle-text', text: 'Disabled')
   end
 
-  def visit_members_list(options = {})
+  it 'shows inherited members by default' do
+    visit_members_list(nested_group)
+
+    expect(member(0)).to include(user.name)
+    expect(member(1)).to include(user_with_2fa.name)
+    expect(member(2)).to include(nested_group_user.name)
+    expect(members_list.size).to eq(3)
+
+    expect(page).to have_css('[data-qa-selector="filter-members-with-inherited-permissions"] a.is-active', text: 'Show all members')
+  end
+
+  it 'shows only group members' do
+    visit_members_list(nested_group, with_inherited_permissions: 'exclude')
+    expect(member(0)).to include(nested_group_user.name)
+    expect(members_list.size).to eq(1)
+    expect(page).to have_css('[data-qa-selector="filter-members-with-inherited-permissions"] a.is-active', text: 'Show only direct members')
+  end
+
+  it 'shows only inherited members' do
+    visit_members_list(nested_group, with_inherited_permissions: 'only')
+    expect(member(0)).to include(user.name)
+    expect(member(1)).to include(user_with_2fa.name)
+    expect(members_list.size).to eq(2)
+    expect(page).to have_css('[data-qa-selector="filter-members-with-inherited-permissions"] a.is-active', text: 'Show only inherited members')
+  end
+
+  def visit_members_list(group, options = {})
     visit group_group_members_path(group.to_param, options)
   end
 
@@ -46,11 +75,7 @@ describe 'Groups > Members > Filter members' do
     page.all('ul.content-list > li')
   end
 
-  def first_member
-    members_list.first.text
-  end
-
-  def second_member
-    members_list.last.text
+  def member(number)
+    members_list[number].text
   end
 end

spec/features/projects/members/group_members_spec.rb

@@ -31,6 +31,41 @@ describe 'Projects members' do
     end
   end
 
+  context 'with a group' do
+    it 'shows group and project members by default' do
+      visit project_project_members_path(project)
+
+      page.within first('.content-list') do
+        expect(page).to have_content(developer.name)
+
+        expect(page).to have_content(user.name)
+        expect(page).to have_content(group.name)
+      end
+    end
+
+    it 'shows project members only if requested' do
+      visit project_project_members_path(project, with_inherited_permissions: 'exclude')
+
+      page.within first('.content-list') do
+        expect(page).to have_content(developer.name)
+
+        expect(page).not_to have_content(user.name)
+        expect(page).not_to have_content(group.name)
+      end
+    end
+
+    it 'shows group members only if requested' do
+      visit project_project_members_path(project, with_inherited_permissions: 'only')
+
+      page.within first('.content-list') do
+        expect(page).not_to have_content(developer.name)
+
+        expect(page).to have_content(user.name)
+        expect(page).to have_content(group.name)
+      end
+    end
+  end
+
   context 'with a group and a project invitee' do
     before do
       group_invitee

spec/finders/group_members_finder_spec.rb

@@ -3,12 +3,13 @@
 require 'spec_helper'
 
 describe GroupMembersFinder, '#execute' do
-  let(:group)        { create(:group) }
+  let(:group) { create(:group) }
   let(:nested_group) { create(:group, parent: group) }
-  let(:user1)        { create(:user) }
-  let(:user2)        { create(:user) }
-  let(:user3)        { create(:user) }
-  let(:user4)        { create(:user) }
+  let(:deeper_nested_group) { create(:group, parent: nested_group) }
+  let(:user1) { create(:user) }
+  let(:user2) { create(:user) }
+  let(:user3) { create(:user) }
+  let(:user4) { create(:user) }
 
   it 'returns members for top-level group' do
     member1 = group.add_maintainer(user1)
@@ -20,7 +21,7 @@ describe GroupMembersFinder, '#execute' do
     expect(result.to_a).to match_array([member3, member2, member1])
   end
 
-  it 'returns members for nested group' do
+  it 'returns members & inherited members for nested group by default' do
     group.add_developer(user2)
     nested_group.request_access(user4)
     member1 = group.add_maintainer(user1)
@@ -32,6 +33,29 @@ describe GroupMembersFinder, '#execute' do
     expect(result.to_a).to match_array([member1, member3, member4])
   end
 
+  it 'does not return inherited members for nested group if requested' do
+    group.add_maintainer(user1)
+    group.add_developer(user2)
+    member2 = nested_group.add_maintainer(user2)
+    member3 = nested_group.add_maintainer(user3)
+
+    result = described_class.new(nested_group).execute(include_relations: [:direct])
+
+    expect(result.to_a).to match_array([member2, member3])
+  end
+
+  it 'returns only inherited members for nested group if requested' do
+    group.add_developer(user2)
+    nested_group.request_access(user4)
+    member1 = group.add_maintainer(user1)
+    nested_group.add_maintainer(user2)
+    nested_group.add_maintainer(user3)
+
+    result = described_class.new(nested_group).execute(include_relations: [:inherited])
+
+    expect(result.to_a).to match_array([member1])
+  end
+
   it 'returns members for descendant groups if requested' do
     member1 = group.add_maintainer(user2)
     member2 = group.add_maintainer(user1)
@@ -39,7 +63,7 @@ describe GroupMembersFinder, '#execute' do
     member3 = nested_group.add_maintainer(user3)
     member4 = nested_group.add_maintainer(user4)
 
-    result = described_class.new(group).execute(include_descendants: true)
+    result = described_class.new(group).execute(include_relations: [:direct, :descendants])
 
     expect(result.to_a).to match_array([member1, member2, member3, member4])
   end

spec/finders/members_finder_spec.rb

@@ -22,22 +22,64 @@ describe MembersFinder, '#execute' do
     expect(result).to contain_exactly(member1, member2, member3)
   end
 
+  it 'includes only non-invite members if user do not have amdin permissions on project' do
+    create(:project_member, :invited, project: project, invite_email: create(:user).email)
+    member1 = project.add_maintainer(user1)
+    member2 = project.add_developer(user2)
+
+    result = described_class.new(project, user2).execute(include_relations: [:direct])
+
+    expect(result).to contain_exactly(member1, member2)
+  end
+
+  it 'includes invited members if user have admin permissions on project' do
+    member_invite = create(:project_member, :invited, project: project, invite_email: create(:user).email)
+    member1 = project.add_maintainer(user1)
+    member2 = project.add_maintainer(user2)
+
+    result = described_class.new(project, user2).execute(include_relations: [:direct])
+
+    expect(result).to contain_exactly(member1, member2, member_invite)
+  end
+
   it 'includes nested group members if asked', :nested_groups do
     nested_group.request_access(user1)
     member1 = group.add_maintainer(user2)
     member2 = nested_group.add_maintainer(user3)
     member3 = project.add_maintainer(user4)
 
-    result = described_class.new(project, user2).execute(include_descendants: true)
+    result = described_class.new(project, user2).execute(include_relations: [:direct, :descendants])
 
     expect(result).to contain_exactly(member1, member2, member3)
   end
 
+  it 'returns only members of project if asked' do
+    nested_group.request_access(user1)
+    group.add_maintainer(user2)
+    nested_group.add_maintainer(user3)
+    member4 = project.add_maintainer(user4)
+
+    result = described_class.new(project, user2).execute(include_relations: [:direct])
+
+    expect(result).to contain_exactly(member4)
+  end
+
+  it 'returns only inherited members of project if asked' do
+    nested_group.request_access(user1)
+    member2 = group.add_maintainer(user2)
+    member3 = nested_group.add_maintainer(user3)
+    project.add_maintainer(user4)
+
+    result = described_class.new(project, user2).execute(include_relations: [:inherited])
+
+    expect(result).to contain_exactly(member2, member3)
+  end
+
   it 'returns the members.access_level when the user is invited', :nested_groups do
     member_invite = create(:project_member, :invited, project: project, invite_email: create(:user).email)
     member1 = group.add_maintainer(user2)
 
-    result = described_class.new(project, user2).execute(include_descendants: true)
+    result = described_class.new(project, user2).execute(include_relations: [:direct, :descendants])
 
     expect(result).to contain_exactly(member1, member_invite)
     expect(result.last.access_level).to eq(member_invite.access_level)
@@ -48,14 +90,14 @@ describe MembersFinder, '#execute' do
     group.add_developer(user1)
     nested_group.add_reporter(user1)
 
-    result = described_class.new(project, user1).execute(include_descendants: true)
+    result = described_class.new(project, user1).execute(include_relations: [:direct, :descendants])
 
     expect(result).to contain_exactly(member1)
     expect(result.first.access_level).to eq(Gitlab::Access::DEVELOPER)
   end
 
   context 'when include_invited_groups_members == true' do
-    subject { described_class.new(project, user2).execute(include_invited_groups_members: true) }
+    subject { described_class.new(project, user2).execute(include_relations: [:inherited, :direct, :invited_groups_members]) }
 
     set(:linked_group) { create(:group, :public) }
     set(:nested_linked_group) { create(:group, parent: linked_group) }