Add option to remove direct memberships of subgroups/projects

Show option only on removal of group members

Fix and extend UI tests

Extend tests of GroupMembersController and Members::DestroyService

Add changelog entry

Extend group tests

Apply 2 suggestion(s) to 1 file(s)

Test Members API with remove_sub_memberships parameter

Readd skip_subresources guard to delete_project_invitations_by

app/assets/javascripts/members/components/action_buttons/remove_member_button.vue

@@ -13,6 +13,11 @@ export default {
       type: Number,
       required: true,
     },
+    memberType: {
+      type: String,
+      required: false,
+      default: null,
+    },
     message: {
       type: String,
       required: true,
@@ -50,6 +55,7 @@ export default {
     :aria-label="title"
     :icon="icon"
     :data-member-path="computedMemberPath"
+    :data-member-type="memberType"
     :data-is-access-request="isAccessRequest"
     :data-message="message"
     data-qa-selector="delete_member_button"

app/assets/javascripts/members/components/action_buttons/user_action_buttons.vue

@@ -59,6 +59,7 @@ export default {
       <remove-member-button
         v-else
         :member-id="member.id"
+        :member-type="member.type"
         :message="message"
         :title="s__('Member|Remove member')"
       />

app/assets/javascripts/vue_shared/components/remove_member_modal.vue

@@ -22,6 +22,9 @@ export default {
     isAccessRequest() {
       return parseBoolean(this.modalData.isAccessRequest);
     },
+    isGroupMember() {
+      return this.modalData.memberType === 'GroupMember';
+    },
     actionText() {
       return this.isAccessRequest ? __('Deny access request') : __('Remove member');
     },
@@ -70,6 +73,9 @@ export default {
 
       <input ref="method" type="hidden" name="_method" value="delete" />
       <input :value="$options.csrf.token" type="hidden" name="authenticity_token" />
+      <gl-form-checkbox v-if="isGroupMember" name="remove_sub_memberships">
+        {{ __('Also remove direct user membership from subgroups and projects') }}
+      </gl-form-checkbox>
       <gl-form-checkbox v-if="!isAccessRequest" name="unassign_issuables">
         {{ __('Also unassign this user from related issues and merge requests') }}
       </gl-form-checkbox>

app/controllers/concerns/membership_actions.rb

@@ -43,10 +43,11 @@ module MembershipActions
 
   def destroy
     member = membershipable.members_and_requesters.find(params[:id])
+    skip_subresources = !ActiveRecord::Type::Boolean.new.cast(params.delete(:remove_sub_memberships))
     # !! is used in case unassign_issuables contains empty string which would result in nil
     unassign_issuables = !!ActiveRecord::Type::Boolean.new.cast(params.delete(:unassign_issuables))
 
-    Members::DestroyService.new(current_user).execute(member, unassign_issuables: unassign_issuables)
+    Members::DestroyService.new(current_user).execute(member, skip_subresources: skip_subresources, unassign_issuables: unassign_issuables)
 
     respond_to do |format|
       format.html do
@@ -54,7 +55,11 @@ module MembershipActions
           begin
             case membershipable
             when Namespace
-              _("User was successfully removed from group and any subresources.")
+              if skip_subresources
+                _("User was successfully removed from group.")
+              else
+                _("User was successfully removed from group and any subgroups and projects.")
+              end
             else
               _("User was successfully removed from project.")
             end

app/serializers/member_entity.rb

@@ -36,6 +36,8 @@ class MemberEntity < Grape::Entity
     GroupEntity.represent(member.source, only: [:id, :full_name, :web_url])
   end
 
+  expose :type
+
   expose :valid_level_roles, as: :valid_roles
 
   expose :user, if: -> (member) { member.user.present? }, using: MemberUserEntity

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

@@ -120,7 +120,7 @@
               = sprite_icon('leave', css_class: 'gl-icon')
               = _('Leave')
           - else
-            %button{ data: { member_path: member_path(member.member), message: remove_member_message(member), is_access_request: member.request?.to_s, qa_selector: 'delete_member_button' },
+            %button{ data: { member_path: member_path(member.member), member_type: member.type, message: remove_member_message(member), is_access_request: member.request?.to_s, qa_selector: 'delete_member_button' },
               class: "js-remove-member-button btn gl-button btn-danger align-self-center m-0 #{'ml-sm-2 btn-icon' unless force_mobile_view}",
               title: remove_member_title(member) }
               %span{ class: ('d-block d-sm-none' unless force_mobile_view) }

changelogs/unreleased/fix-option-remove-memberships-from-subresources.yml

+---
+title: 'Remove group member: add option to also remove direct user membership from
+  subgroups and projects'
+merge_request: 55980
+author: Jonas Wälter @wwwjon
+type: changed

doc/api/members.md

@@ -494,7 +494,10 @@ DELETE /projects/:id/members/:user_id
 | --------- | ---- | -------- | ----------- |
 | `id`      | integer/string | yes | The ID or [URL-encoded path of the project or group](README.md#namespaced-path-encoding) owned by the authenticated user |
 | `user_id` | integer | yes   | The user ID of the member |
-| `unassign_issuables` | boolean | false   | Flag indicating if the removed member should be unassigned from any issues or merge requests inside a given group or project |
+| `skip_subresources` | boolean | false   | Whether the deletion of direct memberships of the removed member in subgroups and projects should be skipped. Default is `false`. | 
+| `unassign_issuables` | boolean | false   | Whether the removed member should be unassigned from any issues or merge requests inside a given group or project. Default is `false`. |
+
+Example request:
 
 ```shell
 curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/groups/:id/members/:user_id"

lib/api/members.rb

@@ -155,6 +155,8 @@ module API
         desc 'Removes a user from a group or project.'
         params do
           requires :user_id, type: Integer, desc: 'The user ID of the member'
+          optional :skip_subresources, type: Boolean, default: false,
+                   desc: 'Flag indicating if the deletion of direct memberships of the removed member in subgroups and projects should be skipped'
           optional :unassign_issuables, type: Boolean, default: false,
                    desc: 'Flag indicating if the removed member should be unassigned from any issues or merge requests within given group or project'
         end
@@ -164,7 +166,7 @@ module API
           member = source_members(source).find_by!(user_id: params[:user_id])
 
           destroy_conditionally!(member) do
-            ::Members::DestroyService.new(current_user).execute(member, unassign_issuables: params[:unassign_issuables])
+            ::Members::DestroyService.new(current_user).execute(member, skip_subresources: params[:skip_subresources], unassign_issuables: params[:unassign_issuables])
           end
         end
         # rubocop: enable CodeReuse/ActiveRecord

locale/gitlab.pot

@@ -3208,6 +3208,9 @@ msgstr ""
 msgid "Also called \"Relying party service URL\" or \"Reply URL\""
 msgstr ""
 
+msgid "Also remove direct user membership from subgroups and projects"
+msgstr ""
+
 msgid "Also unassign this user from related issues and merge requests"
 msgstr ""
 
@@ -33011,7 +33014,10 @@ msgstr ""
 msgid "User was successfully created."
 msgstr ""
 
-msgid "User was successfully removed from group and any subresources."
+msgid "User was successfully removed from group and any subgroups and projects."
+msgstr ""
+
+msgid "User was successfully removed from group."
 msgstr ""
 
 msgid "User was successfully removed from project."

spec/controllers/groups/group_members_controller_spec.rb

@@ -288,7 +288,9 @@ RSpec.describe Groups::GroupMembersController do
   end
 
   describe 'DELETE destroy' do
-    let(:member) { create(:group_member, :developer, group: group) }
+    let(:sub_group) { create(:group, parent: group) }
+    let!(:member) { create(:group_member, :developer, group: group) }
+    let!(:sub_member) { create(:group_member, :developer, group: sub_group, user: member.user) }
 
     before do
       sign_in(user)
@@ -324,9 +326,19 @@ RSpec.describe Groups::GroupMembersController do
         it '[HTML] removes user from members' do
           delete :destroy, params: { group_id: group, id: member }
 
-          expect(response).to set_flash.to 'User was successfully removed from group and any subresources.'
+          expect(response).to set_flash.to 'User was successfully removed from group.'
           expect(response).to redirect_to(group_group_members_path(group))
           expect(group.members).not_to include member
+          expect(sub_group.members).to include sub_member
+        end
+
+        it '[HTML] removes user from members including subgroups and projects' do
+          delete :destroy, params: { group_id: group, id: member, remove_sub_memberships: true }
+
+          expect(response).to set_flash.to 'User was successfully removed from group and any subgroups and projects.'
+          expect(response).to redirect_to(group_group_members_path(group))
+          expect(group.members).not_to include member
+          expect(sub_group.members).not_to include sub_member
         end
 
         it '[JS] removes user from members' do

spec/fixtures/api/schemas/entities/member.json

@@ -8,6 +8,7 @@
     "requested_at",
     "source",
     "valid_roles",
+    "type",
     "can_update",
     "can_remove",
     "is_direct_member"
@@ -40,6 +41,7 @@
       "additionalProperties": false
     },
     "valid_roles": { "type": "object" },
+    "type": { "type": "string" },
     "created_by": {
       "type": "object",
       "required": ["name", "web_url"],

spec/frontend/members/components/action_buttons/invite_action_buttons_spec.js

@@ -39,6 +39,7 @@ describe('InviteActionButtons', () => {
     it('sets props correctly', () => {
       expect(findRemoveMemberButton().props()).toEqual({
         memberId: member.id,
+        memberType: null,
         message: `Are you sure you want to revoke the invitation for ${member.invite.email} to join "${member.source.fullName}"`,
         title: 'Revoke invite',
         isAccessRequest: false,

spec/frontend/members/components/action_buttons/remove_member_button_spec.js

@@ -24,6 +24,7 @@ describe('RemoveMemberButton', () => {
       store: createStore(state),
       propsData: {
         memberId: 1,
+        memberType: 'GroupMember',
         message: 'Are you sure you want to remove John Smith?',
         title: 'Remove member',
         isAccessRequest: true,
@@ -44,6 +45,7 @@ describe('RemoveMemberButton', () => {
 
     expect(wrapper.attributes()).toMatchObject({
       'data-member-path': '/groups/foo-bar/-/group_members/1',
+      'data-member-type': 'GroupMember',
       'data-message': 'Are you sure you want to remove John Smith?',
       'data-is-access-request': 'true',
       'aria-label': 'Remove member',

spec/frontend/members/components/action_buttons/user_action_buttons_spec.js

@@ -39,6 +39,7 @@ describe('UserActionButtons', () => {
     it('sets props correctly', () => {
       expect(findRemoveMemberButton().props()).toEqual({
         memberId: member.id,
+        memberType: 'GroupMember',
         message: `Are you sure you want to remove ${member.user.name} from "${member.source.fullName}"`,
         title: 'Remove member',
         isAccessRequest: false,
@@ -86,4 +87,40 @@ describe('UserActionButtons', () => {
       expect(findRemoveMemberButton().exists()).toBe(false);
     });
   });
+
+  describe('when group member', () => {
+    beforeEach(() => {
+      createComponent({
+        member: {
+          ...member,
+          type: 'GroupMember',
+        },
+        permissions: {
+          canRemove: true,
+        },
+      });
+    });
+
+    it('sets member type correctly', () => {
+      expect(findRemoveMemberButton().props().memberType).toBe('GroupMember');
+    });
+  });
+
+  describe('when project member', () => {
+    beforeEach(() => {
+      createComponent({
+        member: {
+          ...member,
+          type: 'ProjectMember',
+        },
+        permissions: {
+          canRemove: true,
+        },
+      });
+    });
+
+    it('sets member type correctly', () => {
+      expect(findRemoveMemberButton().props().memberType).toBe('ProjectMember');
+    });
+  });
 });

spec/frontend/members/mock_data.js

@@ -11,6 +11,7 @@ export const member = {
     fullName: 'Foo Bar',
     webUrl: 'https://gitlab.com/groups/foo-bar',
   },
+  type: 'GroupMember',
   user: {
     id: 123,
     name: 'Administrator',

spec/frontend/vue_shared/components/remove_member_modal_spec.js

-import { GlFormCheckbox, GlModal } from '@gitlab/ui';
+import { GlModal } from '@gitlab/ui';
 import { shallowMount } from '@vue/test-utils';
 import RemoveMemberModal from '~/vue_shared/components/remove_member_modal.vue';
 
@@ -15,12 +15,20 @@ describe('RemoveMemberModal', () => {
   });
 
   describe.each`
-    state                          | isAccessRequest | actionText               | checkboxTestDescription                                            | checkboxExpected | message
-    ${'removing a member'}         | ${'false'}      | ${'Remove member'}       | ${'shows a checkbox to allow removal from related issues and MRs'} | ${true}          | ${'Are you sure you want to remove Jane Doe from the Gitlab Org / Gitlab Test project?'}
-    ${'denying an access request'} | ${'true'}       | ${'Deny access request'} | ${'does not show a checkbox'}                                      | ${false}         | ${"Are you sure you want to deny Jane Doe's request to join the Gitlab Org / Gitlab Test project?"}
+    state                          | memberType         | isAccessRequest | actionText               | removeSubMembershipsCheckboxExpected | unassignIssuablesCheckboxExpected | message
+    ${'removing a group member'}   | ${'GroupMember'}   | ${'false'}      | ${'Remove member'}       | ${true}                              | ${true}                           | ${'Are you sure you want to remove Jane Doe from the Gitlab Org / Gitlab Test project?'}
+    ${'removing a project member'} | ${'ProjectMember'} | ${'false'}      | ${'Remove member'}       | ${false}                             | ${true}                           | ${'Are you sure you want to remove Jane Doe from the Gitlab Org / Gitlab Test project?'}
+    ${'denying an access request'} | ${'ProjectMember'} | ${'true'}       | ${'Deny access request'} | ${false}                             | ${false}                          | ${"Are you sure you want to deny Jane Doe's request to join the Gitlab Org / Gitlab Test project?"}
   `(
     'when $state',
-    ({ actionText, isAccessRequest, message, checkboxTestDescription, checkboxExpected }) => {
+    ({
+      actionText,
+      memberType,
+      isAccessRequest,
+      message,
+      removeSubMembershipsCheckboxExpected,
+      unassignIssuablesCheckboxExpected,
+    }) => {
       beforeEach(() => {
         wrapper = shallowMount(RemoveMemberModal, {
           data() {
@@ -29,6 +37,7 @@ describe('RemoveMemberModal', () => {
                 isAccessRequest,
                 message,
                 memberPath,
+                memberType,
               },
             };
           },
@@ -47,8 +56,20 @@ describe('RemoveMemberModal', () => {
         expect(wrapper.find('[data-testid=modal-message]').text()).toBe(message);
       });
 
-      it(`${checkboxTestDescription}`, () => {
-        expect(wrapper.find(GlFormCheckbox).exists()).toBe(checkboxExpected);
+      it(`shows ${
+        removeSubMembershipsCheckboxExpected ? 'a' : 'no'
+      } checkbox to remove direct memberships of subgroups/projects`, () => {
+        expect(wrapper.find('[name=remove_sub_memberships]').exists()).toBe(
+          removeSubMembershipsCheckboxExpected,
+        );
+      });
+
+      it(`shows ${
+        unassignIssuablesCheckboxExpected ? 'a' : 'no'
+      } checkbox to allow removal from related issues and MRs`, () => {
+        expect(wrapper.find('[name=unassign_issuables]').exists()).toBe(
+          unassignIssuablesCheckboxExpected,
+        );
       });
 
       it('submits the form when the modal is submitted', () => {

spec/requests/api/members_spec.rb

@@ -555,6 +555,34 @@ RSpec.describe API::Members do
     end
   end
 
+  describe 'DELETE /groups/:id/members/:user_id' do
+    let(:other_user) { create(:user) }
+    let(:nested_group) { create(:group, parent: group) }
+
+    before do
+      nested_group.add_developer(developer)
+      nested_group.add_developer(other_user)
+    end
+
+    it 'deletes only the member with skip_subresources=true' do
+      expect do
+        delete api("/groups/#{group.id}/members/#{developer.id}", maintainer), params: { skip_subresources: true }
+
+        expect(response).to have_gitlab_http_status(:no_content)
+      end.to change { group.members.count }.by(-1)
+          .and change { nested_group.members.count }.by(0)
+    end
+
+    it 'deletes member and its sub memberships with skip_subresources=false' do
+      expect do
+        delete api("/groups/#{group.id}/members/#{developer.id}", maintainer), params: { skip_subresources: false }
+
+        expect(response).to have_gitlab_http_status(:no_content)
+      end.to change { group.members.count }.by(-1)
+          .and change { nested_group.members.count }.by(-1)
+    end
+  end
+
   [false, true].each do |all|
     it_behaves_like 'GET /:source_type/:id/members/(all)', 'project', all do
       let(:source) { project }

spec/services/members/destroy_service_spec.rb

@@ -289,20 +289,51 @@ RSpec.describe Members::DestroyService do
     let(:group_project) { create(:project, :public, group: group) }
     let(:control_project) { create(:project, group: subsubgroup) }
 
+    context 'with memberships' do
       before do
-      create(:group_member, :developer, group: subsubgroup, user: member_user)
-      create(:project_member, :invited, project: group_project, created_by: member_user)
-      create(:group_member, :invited, group: group, created_by: member_user)
-      create(:project_member, :invited, project: subsubproject, created_by: member_user)
-      create(:group_member, :invited, group: subgroup, created_by: member_user)
-
+        subgroup.add_developer(member_user)
+        subsubgroup.add_developer(member_user)
         subsubproject.add_developer(member_user)
+        group_project.add_developer(member_user)
         control_project.add_maintainer(user)
         group.add_owner(user)
 
-      group_member = create(:group_member, :developer, group: group, user: member_user)
+        @group_member = create(:group_member, :developer, group: group, user: member_user)
+      end
+
+      context 'with skipping of subresources' do
+        before do
+          described_class.new(user).execute(@group_member, skip_subresources: true)
+        end
+
+        it 'removes the group membership' do
+          expect(group.members.map(&:user)).not_to include(member_user)
+        end
 
-      described_class.new(user).execute(group_member)
+        it 'does not remove the project membership' do
+          expect(group_project.members.map(&:user)).to include(member_user)
+        end
+
+        it 'does not remove the subgroup membership' do
+          expect(subgroup.members.map(&:user)).to include(member_user)
+        end
+
+        it 'does not remove the subsubgroup membership' do
+          expect(subsubgroup.members.map(&:user)).to include(member_user)
+        end
+
+        it 'does not remove the subsubproject membership' do
+          expect(subsubproject.members.map(&:user)).to include(member_user)
+        end
+
+        it 'does not remove the user from the control project' do
+          expect(control_project.members.map(&:user)).to include(user)
+        end
+      end
+
+      context 'without skipping of subresources' do
+        before do
+          described_class.new(user).execute(@group_member, skip_subresources: false)
         end
 
         it 'removes the project membership' do
@@ -328,6 +359,50 @@ RSpec.describe Members::DestroyService do
         it 'does not remove the user from the control project' do
           expect(control_project.members.map(&:user)).to include(user)
         end
+      end
+    end
+
+    context 'with invites' do
+      before do
+        create(:group_member, :developer, group: subsubgroup, user: member_user)
+        create(:project_member, :invited, project: group_project, created_by: member_user)
+        create(:group_member, :invited, group: group, created_by: member_user)
+        create(:project_member, :invited, project: subsubproject, created_by: member_user)
+        create(:group_member, :invited, group: subgroup, created_by: member_user)
+
+        subsubproject.add_developer(member_user)
+        control_project.add_maintainer(user)
+        group.add_owner(user)
+
+        @group_member = create(:group_member, :developer, group: group, user: member_user)
+      end
+
+      context 'with skipping of subresources' do
+        before do
+          described_class.new(user).execute(@group_member, skip_subresources: true)
+        end
+
+        it 'does not remove group members invited by deleted user' do
+          expect(group.members.not_accepted_invitations_by_user(member_user)).not_to be_empty
+        end
+
+        it 'does not remove project members invited by deleted user' do
+          expect(group_project.members.not_accepted_invitations_by_user(member_user)).not_to be_empty
+        end
+
+        it 'does not remove subgroup members invited by deleted user' do
+          expect(subgroup.members.not_accepted_invitations_by_user(member_user)).not_to be_empty
+        end
+
+        it 'does not remove subproject members invited by deleted user' do
+          expect(subsubproject.members.not_accepted_invitations_by_user(member_user)).not_to be_empty
+        end
+      end
+
+      context 'without skipping of subresources' do
+        before do
+          described_class.new(user).execute(@group_member, skip_subresources: false)
+        end
 
         it 'removes group members invited by deleted user' do
           expect(group.members.not_accepted_invitations_by_user(member_user)).to be_empty
@@ -345,6 +420,8 @@ RSpec.describe Members::DestroyService do
           expect(subsubproject.members.not_accepted_invitations_by_user(member_user)).to be_empty
         end
       end
+    end
+  end
 
   context 'deletion of invitations created by deleted project member' do
     let(:user) { project.owner }