Don't allow deleting a ghost user.

- Add a `destroy_user` ability. This didn't exist before, and was implicit in
  other abilities (only admins could access the admin area, so only they could
  destroy all users; a user can only access their own account page, and so can
  destroy only themselves).

- Grant this ability to admins, and when the current user is trying to destroy
  themselves. Disallow destroying ghost users in all cases.

- Modify the `Users::DestroyService` to check this ability. Also check it in
  views to decide whether or not to show the "Delete User" button.

- Add a short summary of the Ghost User to the bio.

8 files changed, 59 insertions, 5 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index 4bb9dc0c59f..13f6f2c3d77 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1043,8 +1043,10 @@ class User < ActiveRecord::Base
       User.find_by_email(s)
     end
 
+    bio = 'This is a "Ghost User", created to hold all issues authored by users that have since been deleted. This user cannot be removed.'
+
     User.create(
-      username: username, password: Devise.friendly_token,
+      username: username, password: Devise.friendly_token, bio: bio,
       email: email, name: "Ghost User", state: :blocked, ghost: true
     )
   ensure
diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb
index 03a2499e263..229846e368c 100644
--- a/app/policies/user_policy.rb
+++ b/app/policies/user_policy.rb
@@ -3,6 +3,14 @@ class UserPolicy < BasePolicy
 
   def rules
     can! :read_user if @user || !restricted_public_level?
+
+    if @user
+      if @user.admin? || @subject == @user
+        can! :destroy_user
+      end
+
+      cannot! :destroy_user if @subject.ghost?
+    end
   end
 
   def restricted_public_level?
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb
index 523279944ae..833da5bc5d1 100644
--- a/app/services/users/destroy_service.rb
+++ b/app/services/users/destroy_service.rb
@@ -7,7 +7,7 @@ module Users
     end
 
     def execute(user, options = {})
-      unless current_user.admin? || current_user == user
+      unless Ability.allowed?(current_user, :destroy_user, user)
         raise Gitlab::Access::AccessDeniedError, "#{current_user} tried to destroy user #{user}!"
       end
 
diff --git a/app/views/admin/users/_user.html.haml b/app/views/admin/users/_user.html.haml
index 3b5c713ac2d..a756cb7243a 100644
--- a/app/views/admin/users/_user.html.haml
+++ b/app/views/admin/users/_user.html.haml
@@ -34,7 +34,7 @@
             - if user.access_locked?
               %li
                 = link_to 'Unlock', unlock_admin_user_path(user), method: :put, class: 'btn-grouped btn btn-xs btn-success', data: { confirm: 'Are you sure?' }
-          - if user.can_be_removed?
+          - if user.can_be_removed? && can?(current_user, :destroy_user, @user)
             %li.divider
             %li
               = link_to 'Delete User', [:admin, user], data: { confirm: "USER #{user.name} WILL BE REMOVED! All issues, merge requests and groups linked to this user will also be removed! Consider cancelling this deletion and blocking the user instead. Are you sure?" },
diff --git a/app/views/admin/users/show.html.haml b/app/views/admin/users/show.html.haml
index 76b1291fe10..840d843f069 100644
--- a/app/views/admin/users/show.html.haml
+++ b/app/views/admin/users/show.html.haml
@@ -173,7 +173,7 @@
         .panel-heading
           Remove user
         .panel-body
-          - if @user.can_be_removed?
+          - if @user.can_be_removed? && can?(current_user, :destroy_user, @user)
             %p Deleting a user has the following effects:
             %ul
               %li All user content like authored issues, snippets, comments will be removed
@@ -189,3 +189,6 @@
                 %strong= @user.solo_owned_groups.map(&:name).join(', ')
               %p
                 You must transfer ownership or delete these groups before you can delete this user.
+            - else
+              %p
+                You don't have access to delete this user.
diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml
index a4f4079d556..02fb47ec981 100644
--- a/app/views/profiles/accounts/show.html.haml
+++ b/app/views/profiles/accounts/show.html.haml
@@ -115,7 +115,7 @@
       %h4.prepend-top-0.danger-title
         Remove account
     .col-lg-9
-      - if @user.can_be_removed?
+      - if @user.can_be_removed? && can?(current_user, :destroy_user, @user)
         %p
           Deleting an account has the following effects:
         %ul
@@ -131,4 +131,7 @@
             %strong= @user.solo_owned_groups.map(&:name).join(', ')
           %p
             You must transfer ownership or delete these groups before you can delete your account.
+        - else
+          %p
+            You don't have access to delete this user.
 .append-bottom-default
diff --git a/spec/factories/users.rb b/spec/factories/users.rb
index 58602c54c7a..249dabbaae8 100644
--- a/spec/factories/users.rb
+++ b/spec/factories/users.rb
@@ -28,6 +28,7 @@ FactoryGirl.define do
 
     trait :ghost do
       ghost true
+      after(:build) { |user, _| user.block! }
     end
 
     trait :two_factor_via_otp do
diff --git a/spec/policies/user_policy_spec.rb b/spec/policies/user_policy_spec.rb
new file mode 100644
index 00000000000..d5761390d39
--- /dev/null
+++ b/spec/policies/user_policy_spec.rb
@@ -0,0 +1,37 @@
+require 'spec_helper'
+
+describe UserPolicy, models: true do
+  let(:current_user) { create(:user) }
+  let(:user) { create(:user) }
+
+  subject { described_class.abilities(current_user, user).to_set }
+
+  describe "reading a user's information" do
+    it { is_expected.to include(:read_user) }
+  end
+
+  describe "destroying a user" do
+    context "when a regular user tries to destroy another regular user" do
+      it { is_expected.not_to include(:destroy_user) }
+    end
+
+    context "when a regular user tries to destroy themselves" do
+      let(:current_user) { user }
+
+      it { is_expected.to include(:destroy_user) }
+    end
+
+    context "when an admin user tries to destroy a regular user" do
+      let(:current_user) { create(:user, :admin) }
+
+      it { is_expected.to include(:destroy_user) }
+    end
+
+    context "when an admin user tries to destroy a ghost user" do
+      let(:current_user) { create(:user, :admin) }
+      let(:user) { create(:user, :ghost) }
+
+      it { is_expected.not_to include(:destroy_user) }
+    end
+  end
+end