Merge branch 'vij-fix-billable-member-removal' into 'master'

Add Billable Member deletion API endpoint

See merge request gitlab-org/gitlab!55645

doc/api/members.md

@@ -310,6 +310,27 @@ Example response:
 ]
 ```
 
+## Remove a billable member from a group
+
+Removes a billable member from a group and its subgroups and projects.
+
+The user does not need to be a group member to qualify for removal.
+For example, if the user was added directly to a project within the group, you can
+still use this API to remove them.
+
+```plaintext
+DELETE /groups/:id/billable_members/:user_id
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id`      | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user |
+| `user_id` | integer | yes   | The user ID of the member |
+
+```shell
+curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/groups/:id/billable_members/:user_id"
+```
+
 ## Add a member to a group or project
 
 Adds a member to a group or project.

ee/app/services/billable_members/destroy_service.rb

+# frozen_string_literal: true
+
+# BillableMembers::DestroyService class
+#
+# Used to find and remove all billable Member records (GroupMember or ProjectMember)
+# within a group's hierarchy for the given user_id, so that a billable member can be completely
+# removed from the group and it's subgroups and projects
+#
+# Ex.
+#   BillableMembers::Destroy.new(group, user_id: 1, current_user: current_user).execute
+#
+module BillableMembers
+  class DestroyService
+    include BaseServiceUtility
+
+    InvalidGroupError = Class.new(StandardError)
+
+    def initialize(group, user_id:, current_user:)
+      @group = group
+      @user_id = user_id
+      @current_user = current_user
+    end
+
+    def execute
+      check_group_level
+      check_user_access
+
+      remove_user_from_resources
+
+      success
+    rescue InvalidGroupError, Gitlab::Access::AccessDeniedError => e
+      error(e.message)
+    end
+
+    private
+
+    attr_reader :group, :user_id, :current_user
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def remove_user_from_resources
+      memberships = ::Member.in_hierarchy(group).where(user_id: user_id)
+
+      memberships.find_each do |member|
+        ::Members::DestroyService.new(current_user).execute(member, skip_subresources: true)
+      end
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
+    def check_group_level
+      unless group.root?
+        raise InvalidGroupError, 'Invalid group provided, must be top-level'
+      end
+    end
+
+    def check_user_access
+      unless can?(current_user, :admin_group_member, group)
+        raise Gitlab::Access::AccessDeniedError, 'User unauthorized to remove member'
+      end
+    end
+  end
+end

ee/changelogs/unreleased/vij-fix-billable-member-removal.yml

+---
+title: Add remove Billable Member API endpoint
+merge_request: 55645
+author:
+type: added

ee/lib/ee/api/members.rb

@@ -77,6 +77,22 @@ module EE
 
             present users, with: ::EE::API::Entities::BillableMember, current_user: current_user
           end
+
+          desc 'Removes a billable member from a group or project.'
+          params do
+            requires :user_id, type: Integer, desc: 'The user ID of the member'
+          end
+          delete ":id/billable_members/:user_id" do
+            group = find_group!(params[:id])
+
+            result = ::BillableMembers::DestroyService.new(group, user_id: params[:user_id], current_user: current_user).execute
+
+            if result[:status] == :success
+              no_content!
+            else
+              bad_request!(nil)
+            end
+          end
         end
       end
     end

ee/spec/requests/api/members_spec.rb

@@ -351,7 +351,7 @@ RSpec.describe API::Members do
     end
   end
 
-  describe "GET /groups/:id/billable_members" do
+  context 'billable member endpoints' do
     let_it_be(:owner) { create(:user) }
     let_it_be(:maintainer) { create(:user) }
     let_it_be(:group) do
@@ -368,136 +368,185 @@ RSpec.describe API::Members do
       end
     end
 
-    let(:url) { "/groups/#{group.id}/billable_members" }
-    let(:params) { {} }
+    describe 'GET /groups/:id/billable_members' do
+      let(:url) { "/groups/#{group.id}/billable_members" }
+      let(:params) { {} }
 
-    subject do
-      get api(url, owner), params: params
-      json_response
-    end
+      subject(:get_billable_members) do
+        get api(url, owner), params: params
+        json_response
+      end
 
-    context 'with sub group and projects' do
-      let!(:project_user) { create(:user) }
-      let!(:project) do
-        create(:project, :public, group: nested_group) do |project|
-          project.add_developer(project_user)
+      context 'with sub group and projects' do
+        let_it_be(:project_user) { create(:user) }
+        let_it_be(:project) do
+          create(:project, :public, group: nested_group) do |project|
+            project.add_developer(project_user)
+          end
         end
-      end
 
-      let!(:linked_group_user) { create(:user, name: 'Scott McNeil') }
-      let!(:linked_group) do
-        create(:group) do |linked_group|
-          linked_group.add_developer(linked_group_user)
+        let_it_be(:linked_group_user) { create(:user, name: 'Scott McNeil') }
+        let_it_be(:linked_group) do
+          create(:group) do |linked_group|
+            linked_group.add_developer(linked_group_user)
+          end
         end
-      end
 
-      let!(:project_group_link) { create(:project_group_link, project: project, group: linked_group) }
+        let_it_be(:project_group_link) { create(:project_group_link, project: project, group: linked_group) }
 
-      it 'returns paginated billable users' do
-        subject
+        it 'returns paginated billable users' do
+          get_billable_members
 
-        expect_paginated_array_response(*[owner, maintainer, nested_user, project_user, linked_group_user].map(&:id))
-      end
+          expect_paginated_array_response(*[owner, maintainer, nested_user, project_user, linked_group_user].map(&:id))
+        end
 
-      context 'when the current user does not have the :admin_group_member ability' do
-        it 'is a bad request' do
-          not_an_owner = create(:user)
+        context 'when the current user does not have the :admin_group_member ability' do
+          it 'is a bad request' do
+            not_an_owner = create(:user)
 
-          get api(url, not_an_owner), params: params
+            get api(url, not_an_owner), params: params
 
-          expect(response).to have_gitlab_http_status(:bad_request)
+            expect(response).to have_gitlab_http_status(:bad_request)
+          end
         end
-      end
 
-      context 'with seach params provided' do
-        let(:params) { { search: nested_user.name } }
+        context 'with search params provided' do
+          let(:params) { { search: nested_user.name } }
 
-        it 'returns the relevant billable users' do
-          subject
+          it 'returns the relevant billable users' do
+            get_billable_members
 
-          expect_paginated_array_response([nested_user.id])
+            expect_paginated_array_response([nested_user.id])
+          end
         end
-      end
 
-      context 'with search and sort params provided' do
-        it 'accepts only sorting options defined in a list' do
-          EE::API::Helpers::MembersHelpers.member_sort_options.each do |sorting|
+        context 'with search and sort params provided' do
+          it 'accepts only sorting options defined in a list' do
+            EE::API::Helpers::MembersHelpers.member_sort_options.each do |sorting|
+              get api(url, owner), params: { search: 'name', sort: sorting }
+              expect(response).to have_gitlab_http_status(:ok)
+            end
+          end
+
+          it 'does not accept query string not defined in a list' do
+            defined_query_strings = EE::API::Helpers::MembersHelpers.member_sort_options
+            sorting = 'fake_sorting'
+
             get api(url, owner), params: { search: 'name', sort: sorting }
-            expect(response).to have_gitlab_http_status(:ok)
+
+            expect(defined_query_strings).not_to include(sorting)
+            expect(response).to have_gitlab_http_status(:bad_request)
           end
-        end
 
-        it 'does not accept query string not defined in a list' do
-          defined_query_strings = EE::API::Helpers::MembersHelpers.member_sort_options
-          sorting = 'fake_sorting'
+          context 'when a specific sorting is provided' do
+            let(:params) { { search: 'Scott', sort: 'name_desc' } }
 
-          get api(url, owner), params: { search: 'name', sort: sorting }
+            it 'returns the relevant billable users' do
+              get_billable_members
+
+              expect_paginated_array_response(*[linked_group_user, nested_user].map(&:id))
+            end
+          end
+        end
+      end
+
+      context 'with non owner' do
+        it 'returns error' do
+          get api(url, maintainer)
 
-          expect(defined_query_strings).not_to include(sorting)
           expect(response).to have_gitlab_http_status(:bad_request)
         end
+      end
 
-        context 'when a specific sorting is provided' do
-          let(:params) { { search: 'Scott', sort: 'name_desc' } }
+      context 'when group can not be found' do
+        let(:url) { "/groups/foo/billable_members" }
 
-          it 'returns the relevant billable users' do
-            subject
+        it 'returns error' do
+          get api(url, owner)
 
-            expect_paginated_array_response(*[linked_group_user, nested_user].map(&:id))
-          end
+          expect(response).to have_gitlab_http_status(:not_found)
+          expect(json_response['message']).to eq('404 Group Not Found')
         end
       end
-    end
 
-    context 'with non owner' do
-      it 'returns error' do
-        get api(url, maintainer)
+      context 'with non-root group' do
+        let(:child_group) { create :group, parent: group }
+        let(:url) { "/groups/#{child_group.id}/billable_members" }
 
-        expect(response).to have_gitlab_http_status(:bad_request)
+        it 'returns error' do
+          get_billable_members
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
       end
-    end
 
-    context 'when group can not be found' do
-      let(:url) { "/groups/foo/billable_members" }
+      context 'email' do
+        before do
+          group.add_owner(owner)
+        end
 
-      it 'returns error' do
-        get api(url, owner)
+        include_context 'group managed account with group members'
+
+        context 'when members have a public_email' do
+          before do
+            allow_next_found_instance_of(User) do |instance|
+              allow(instance).to receive(:public_email).and_return('public@email.com')
+            end
+          end
 
-        expect(response).to have_gitlab_http_status(:not_found)
-        expect(json_response['message']).to eq('404 Group Not Found')
+          it { is_expected.to include(a_hash_including('email' => 'public@email.com')) }
+        end
+
+        context 'when members have no public_email' do
+          it { is_expected.to include(a_hash_including('email' => '')) }
+        end
       end
     end
 
-    context 'with non-root group' do
-      let(:child_group) { create :group, parent: group }
-      let(:url) { "/groups/#{child_group.id}/billable_members" }
+    describe 'DELETE /groups/:id/billable_members/:user_id' do
+      context 'when the current user has insufficient rights' do
+        it 'returns 400' do
+          not_an_owner = create(:user)
 
-      it 'returns error' do
-        subject
+          delete api("/groups/#{group.id}/billable_members/#{maintainer.id}", not_an_owner)
 
-        expect(response).to have_gitlab_http_status(:bad_request)
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
       end
-    end
 
-    context 'email' do
-      before do
-        group.add_owner(owner)
+      shared_examples 'successful deletion' do
+        it 'deletes the member' do
+          expect(group.member?(user)).to be is_group_member
+
+          expect do
+            delete api("/groups/#{group.id}/billable_members/#{user.id}", owner)
+
+            expect(response).to have_gitlab_http_status(:no_content)
+          end.to change { source.members.count }.by(-1)
+        end
       end
 
-      include_context 'group managed account with group members'
+      context 'when authenticated as an owner' do
+        context 'with a user that is a GroupMember' do
+          let(:user) { maintainer }
+          let(:is_group_member) { true }
+          let(:source) { group }
 
-      context 'when members have a public_email' do
-        before do
-          allow_next_found_instance_of(User) do |instance|
-            allow(instance).to receive(:public_email).and_return('public@email.com')
-          end
+          it_behaves_like 'successful deletion'
         end
 
-        it { is_expected.to include(a_hash_including('email' => 'public@email.com')) }
-      end
+        context 'with a user that is only a ProjectMember' do
+          let(:user) { create(:user) }
+          let(:is_group_member) { false }
+          let(:source) { project }
+          let(:project) do
+            create(:project, group: group) do |project|
+              project.add_developer(user)
+            end
+          end
 
-      context 'when members have no public_email' do
-        it { is_expected.to include(a_hash_including('email' => '')) }
+          it_behaves_like 'successful deletion'
+        end
       end
     end
   end

ee/spec/services/billable_members/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe BillableMembers::DestroyService do
+  describe '#execute' do
+    let_it_be(:current_user) { create(:user) }
+    let_it_be(:root_group) { create(:group) }
+    let(:group) { root_group }
+
+    let(:user_id) { nil }
+
+    subject(:execute) { described_class.new(group, user_id: user_id, current_user: current_user).execute }
+
+    context 'when unauthorized' do
+      it 'raises an access error' do
+        result = execute
+
+        expect(result[:status]).to eq :error
+        expect(result[:message]).to eq 'User unauthorized to remove member'
+      end
+    end
+
+    context 'when authorized' do
+      let(:subgroup) { create(:group, parent: root_group) }
+      let(:project_1) { create(:project, group: subgroup) }
+      let(:project_2) { create(:project, group: root_group) }
+
+      let(:group_member) { create(:group_member, group: root_group) }
+      let(:subgroup_member) { create(:group_member, group: subgroup) }
+      let(:project_member) { create(:project_member, project: project_1) }
+
+      before do
+        group.add_owner(current_user)
+      end
+
+      context 'when passing a sub group to the service' do
+        let(:group) { subgroup }
+
+        it 'raises an invalid group error' do
+          result = execute
+
+          expect(result[:status]).to eq :error
+          expect(result[:message]).to eq 'Invalid group provided, must be top-level'
+        end
+      end
+
+      context 'when removing a group member' do
+        let(:user_id) { group_member.user_id }
+
+        it 'removes the member' do
+          execute
+
+          expect(root_group.members).not_to include(group_member)
+        end
+      end
+
+      context 'when removing a subgroup member' do
+        let(:user_id) { subgroup_member.user_id }
+
+        it 'removes the member' do
+          execute
+
+          expect(subgroup.members).not_to include(subgroup_member)
+        end
+      end
+
+      context 'when removing a project member' do
+        let(:user_id) { project_member.user_id }
+
+        it 'removes the member' do
+          execute
+
+          expect(project_1.members).not_to include(project_member)
+        end
+      end
+
+      context 'when the user is a direct member of multiple projects' do
+        let(:multi_project_user) { create(:user) }
+        let(:user_id) { multi_project_user.id }
+
+        it 'removes the user from all the projects' do
+          project_1.add_developer(multi_project_user)
+          project_2.add_developer(multi_project_user)
+
+          execute
+
+          expect(multi_project_user.projects).not_to include(project_1)
+          expect(multi_project_user.projects).not_to include(project_2)
+        end
+      end
+    end
+  end
+end