Merge branch 'dblessing-project-bot-prevent-removal' into 'master'

Prevent a project bot from being removed as member

See merge request gitlab-org/gitlab!35899

app/policies/project_member_policy.rb

@@ -5,14 +5,17 @@ class ProjectMemberPolicy < BasePolicy
 
   condition(:target_is_owner, scope: :subject) { @subject.user == @subject.project.owner }
   condition(:target_is_self) { @user && @subject.user == @user }
+  condition(:project_bot) { @subject.user&.project_bot? }
 
   rule { anonymous }.prevent_all
   rule { target_is_owner }.prevent_all
 
-  rule { can?(:admin_project_member) }.policy do
+  rule { ~project_bot & can?(:admin_project_member) }.policy do
     enable :update_project_member
     enable :destroy_project_member
   end
 
+  rule { project_bot & can?(:admin_project_member) }.enable :destroy_project_bot_member
+
   rule { target_is_self }.enable :destroy_project_member
 end

app/services/members/destroy_service.rb

@@ -2,8 +2,8 @@
 
 module Members
   class DestroyService < Members::BaseService
-    def execute(member, skip_authorization: false, skip_subresources: false, unassign_issuables: false)
-      raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_destroy_member?(member)
+    def execute(member, skip_authorization: false, skip_subresources: false, unassign_issuables: false, destroy_bot: false)
+      raise Gitlab::Access::AccessDeniedError unless skip_authorization || authorized?(member, destroy_bot)
 
       @skip_auth = skip_authorization
 
@@ -28,6 +28,12 @@ module Members
 
     private
 
+    def authorized?(member, destroy_bot)
+      return can_destroy_bot_member?(member) if destroy_bot
+
+      can_destroy_member?(member)
+    end
+
     def delete_subresources(member)
       return unless member.is_a?(GroupMember) && member.user && member.group
 
@@ -55,6 +61,10 @@ module Members
       can?(current_user, destroy_member_permission(member), member)
     end
 
+    def can_destroy_bot_member?(member)
+      can?(current_user, destroy_bot_member_permission(member), member)
+    end
+
     def destroy_member_permission(member)
       case member
       when GroupMember
@@ -66,6 +76,12 @@ module Members
       end
     end
 
+    def destroy_bot_member_permission(member)
+      raise "Unsupported bot member type: #{member}" unless member.is_a?(ProjectMember)
+
+      :destroy_project_bot_member
+    end
+
     def enqueue_unassign_issuables(member)
       source_type = member.is_a?(GroupMember) ? 'Group' : 'Project'
 

app/services/resource_access_tokens/revoke_service.rb

@@ -35,7 +35,7 @@ module ResourceAccessTokens
     attr_reader :current_user, :access_token, :bot_user, :resource
 
     def remove_member
-      ::Members::DestroyService.new(current_user).execute(find_member)
+      ::Members::DestroyService.new(current_user).execute(find_member, destroy_bot: true)
     end
 
     def migrate_to_ghost_user

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

@@ -67,7 +67,7 @@
                     class: 'btn btn-default align-self-center mr-sm-2',
                     title: _('Resend invite')
 
-        - if user != current_user && member.can_update? && !user&.project_bot?
+        - if user != current_user && member.can_update?
           = form_for member, remote: true, html: { class: "js-edit-member-form form-group #{'d-sm-flex' unless force_mobile_view}" } do |f|
             = f.hidden_field :access_level
             .member-form-control.dropdown{ class: [("mr-sm-2 d-sm-inline-block" unless force_mobile_view)] }
@@ -117,7 +117,7 @@
                       method: :delete,
                       data: { confirm: leave_confirmation_message(member.source) },
                       class: "btn btn-remove align-self-center m-0 #{'ml-sm-2' unless force_mobile_view}"
-          - elsif !user&.project_bot?
+          - 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' },
               class: "js-remove-member-button btn btn-remove align-self-center m-0 #{'ml-sm-2' unless force_mobile_view}",
               title: remove_member_title(member) }

changelogs/unreleased/dblessing-project-bot-prevent-removal.yml

+---
+title: Prevent a project bot from being removed as member
+merge_request: 35899
+author:
+type: added

spec/policies/project_member_policy_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ProjectMemberPolicy do
+  let(:project) { create(:project) }
+  let(:maintainer_user) { create(:user) }
+  let(:member) { create(:project_member, project: project, user: member_user) }
+
+  subject { described_class.new(maintainer_user, member) }
+
+  before do
+    create(:project_member, :maintainer, project: project, user: maintainer_user)
+  end
+
+  context 'with regular member' do
+    let(:member_user) { create(:user) }
+
+    it { is_expected.to be_allowed(:update_project_member) }
+    it { is_expected.to be_allowed(:destroy_project_member) }
+
+    it { is_expected.not_to be_allowed(:destroy_project_bot_member) }
+  end
+
+  context 'with a bot member' do
+    let(:member_user) { create(:user, :project_bot) }
+
+    it { is_expected.to be_allowed(:destroy_project_bot_member) }
+
+    it { is_expected.not_to be_allowed(:update_project_member) }
+    it { is_expected.not_to be_allowed(:destroy_project_member) }
+  end
+end

spec/requests/api/members_spec.rb

@@ -495,4 +495,17 @@ RSpec.describe API::Members do
       end.to change { project.members.count }.by(0)
     end
   end
+
+  context 'remove bot from project' do
+    it 'returns a 403 forbidden' do
+      project_bot = create(:user, :project_bot)
+      create(:project_member, project: project, user: project_bot)
+
+      expect do
+        delete api("/projects/#{project.id}/members/#{project_bot.id}", maintainer)
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end.to change { project.members.count }.by(0)
+    end
+  end
 end

spec/services/members/destroy_service_spec.rb

@@ -153,6 +153,25 @@ RSpec.describe Members::DestroyService do
         end
       end
 
+      context 'with a project bot member' do
+        let(:member) { group_project.members.find_by(user_id: member_user.id) }
+        let(:member_user) { create(:user, :project_bot) }
+
+        before do
+          group_project.add_maintainer(member_user)
+        end
+
+        context 'when the destroy_bot flag is true' do
+          it_behaves_like 'a service destroying a member with access' do
+            let(:opts) { { destroy_bot: true } }
+          end
+        end
+
+        context 'when the destroy_bot flag is not specified' do
+          it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError'
+        end
+      end
+
       context 'with a group member' do
         let(:member) { group.members.find_by(user_id: member_user.id) }