Merge branch 'sh-delete-user-permission-check' into 'master'

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

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

changelogs/unreleased/sh-delete-user-permission-check.yml

+---
+title: Add user deletion permission check in `Users::DestroyService`
+merge_request:
+author:

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