diff options
| -rw-r--r-- | app/services/users/destroy_service.rb | 4 | ||||
| -rw-r--r-- | app/workers/delete_user_worker.rb | 2 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-delete-user-permission-check.yml | 4 | ||||
| -rw-r--r-- | spec/services/users/destroy_spec.rb | 31 |
4 files changed, 36 insertions, 5 deletions
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index 2d11305be13..bc0653cb634 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -7,6 +7,10 @@ module Users end def execute(user, options = {}) + unless current_user.admin? || current_user == user + raise Gitlab::Access::AccessDeniedError, "#{current_user} tried to destroy user #{user}!" + end + if !options[:delete_solo_owned_groups] && user.solo_owned_groups.present? user.errors[:base] << 'You must transfer ownership or delete groups before you can remove user' return user diff --git a/app/workers/delete_user_worker.rb b/app/workers/delete_user_worker.rb index 5483bbb210b..3340a7be4fe 100644 --- a/app/workers/delete_user_worker.rb +++ b/app/workers/delete_user_worker.rb @@ -7,5 +7,7 @@ class DeleteUserWorker current_user = User.find(current_user_id) Users::DestroyService.new(current_user).execute(delete_user, options.symbolize_keys) + rescue Gitlab::Access::AccessDeniedError => e + Rails.logger.warn("User could not be destroyed: #{e}") end end diff --git a/changelogs/unreleased/sh-delete-user-permission-check.yml b/changelogs/unreleased/sh-delete-user-permission-check.yml new file mode 100644 index 00000000000..c0e79aae2a8 --- /dev/null +++ b/changelogs/unreleased/sh-delete-user-permission-check.yml @@ -0,0 +1,4 @@ +--- +title: Add user deletion permission check in `Users::DestroyService` +merge_request: +author: diff --git a/spec/services/users/destroy_spec.rb b/spec/services/users/destroy_spec.rb index 46e58393218..c0bf27c698c 100644 --- a/spec/services/users/destroy_spec.rb +++ b/spec/services/users/destroy_spec.rb @@ -2,11 +2,11 @@ require 'spec_helper' describe Users::DestroyService, services: true do describe "Deletes a user and all their personal projects" do - let!(:user) { create(:user) } - let!(:current_user) { create(:user) } - let!(:namespace) { create(:namespace, owner: user) } - let!(:project) { create(:project, namespace: namespace) } - let(:service) { described_class.new(current_user) } + let!(:user) { create(:user) } + let!(:admin) { create(:admin) } + let!(:namespace) { create(:namespace, owner: user) } + let!(:project) { create(:project, namespace: namespace) } + let(:service) { described_class.new(admin) } context 'no options are given' do it 'deletes the user' do @@ -57,5 +57,26 @@ describe Users::DestroyService, services: true do expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound) end end + + context "deletion permission checks" do + it 'does not delete the user when user is not an admin' do + other_user = create(:user) + + expect { described_class.new(other_user).execute(user) }.to raise_error(Gitlab::Access::AccessDeniedError) + expect(User.exists?(user.id)).to be(true) + end + + it 'allows admins to delete anyone' do + described_class.new(admin).execute(user) + + expect(User.exists?(user.id)).to be(false) + end + + it 'allows users to delete their own account' do + described_class.new(user).execute(user) + + expect(User.exists?(user.id)).to be(false) + end + end end end |