From 320195d1fc67a5576550618df41adb4965efbd51 Mon Sep 17 00:00:00 2001
From: Drew Blessing <drew@blessing.io>
Date: Thu, 2 Jul 2020 13:32:23 -0500
Subject: [PATCH] Prevent a project bot from being removed as member via the
API
A project bot user is tied to a project access token. The user
should not be removed from the project directly. Instead, the
project access token should be removed, which will also remove
the member and user.
---
app/policies/project_member_policy.rb | 5 ++-
app/services/members/destroy_service.rb | 20 +++++++++--
.../resource_access_tokens/revoke_service.rb | 2 +-
app/views/shared/members/_member.html.haml | 4 +--
.../dblessing-project-bot-prevent-removal.yml | 5 +++
spec/policies/project_member_policy_spec.rb | 33 +++++++++++++++++++
spec/requests/api/members_spec.rb | 13 ++++++++
spec/services/members/destroy_service_spec.rb | 19 +++++++++++
8 files changed, 95 insertions(+), 6 deletions(-)
create mode 100644 changelogs/unreleased/dblessing-project-bot-prevent-removal.yml
create mode 100644 spec/policies/project_member_policy_spec.rb
diff --git a/app/policies/project_member_policy.rb b/app/policies/project_member_policy.rb
index f2f18406bd3..ca33b95e523 100644
--- a/app/policies/project_member_policy.rb
+++ b/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
diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb
index 7e620047c78..fdd43260521 100644
--- a/app/services/members/destroy_service.rb
+++ b/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'
diff --git a/app/services/resource_access_tokens/revoke_service.rb b/app/services/resource_access_tokens/revoke_service.rb
index eea6bff572b..efeb0bfb8d5 100644
--- a/app/services/resource_access_tokens/revoke_service.rb
+++ b/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
diff --git a/app/views/shared/members/_member.html.haml b/app/views/shared/members/_member.html.haml
index cc318ecded7..79dc3043e8d 100644
--- a/app/views/shared/members/_member.html.haml
+++ b/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) }
diff --git a/changelogs/unreleased/dblessing-project-bot-prevent-removal.yml b/changelogs/unreleased/dblessing-project-bot-prevent-removal.yml
new file mode 100644
index 00000000000..af29cee48ba
--- /dev/null
+++ b/changelogs/unreleased/dblessing-project-bot-prevent-removal.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent a project bot from being removed as member
+merge_request: 35899
+author:
+type: added
diff --git a/spec/policies/project_member_policy_spec.rb b/spec/policies/project_member_policy_spec.rb
new file mode 100644
index 00000000000..ab8f8b83e7f
--- /dev/null
+++ b/spec/policies/project_member_policy_spec.rb
@@ -0,0 +1,33 @@
+# 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
diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb
index db910c6f097..bbce5b4cfb6 100644
--- a/spec/requests/api/members_spec.rb
+++ b/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
diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb
index ce1077b1167..13e7b4c1006 100644
--- a/spec/services/members/destroy_service_spec.rb
+++ b/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) }
--
2.30.9