From 1854c3f768d944b385be9a865deb5d869a84b7a6 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Thu, 10 Nov 2016 12:33:48 +0530
Subject: [PATCH 01/11] Add CE CHANGELOG for #12726.

---
 .../unreleased/12726-preserve-issues-after-deleting-users.yml | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 changelogs/unreleased/12726-preserve-issues-after-deleting-users.yml

diff --git a/changelogs/unreleased/12726-preserve-issues-after-deleting-users.yml b/changelogs/unreleased/12726-preserve-issues-after-deleting-users.yml
new file mode 100644
index 00000000000..4a1a199673c
--- /dev/null
+++ b/changelogs/unreleased/12726-preserve-issues-after-deleting-users.yml
@@ -0,0 +1,4 @@
+---
+title: Deleting a user doesn't delete issues they've created/are assigned to
+merge_request: 7393
+author:

From 29540d6f35da643b4f1894dc5ffd4c0a22f7cd3e Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Wed, 21 Dec 2016 15:20:09 +0530
Subject: [PATCH 02/11] Don't send notifications to ghost users.

We already skip sending notifications to blocked users. Simply add ghost users
to this list.
---
 app/services/notification_service.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 3734e3c4253..97db117aa99 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -466,6 +466,7 @@ class NotificationService
 
     users = users.to_a.compact.uniq
     users = users.reject(&:blocked?)
+    users = users.reject(&:ghost?)
 
     users.reject do |user|
       global_notification_setting = user.global_notification_setting

From ff19bbd3b40621ae94632b9aa68fd12645b6ed41 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 11 Nov 2016 11:57:43 +0530
Subject: [PATCH 03/11] Deleting a user shouldn't delete associated issues.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- "Associated" issues are issues the user has created + issues that the
  user is assigned to.

- Issues that a user owns are transferred to a "Ghost User" (just a
  regular user with `state = 'ghost'` that is created when
  `User.ghost` is called).

- Issues that a user is assigned to are moved to the "Unassigned" state.

- Fix a spec failure in `profile_spec` — a spec was asserting that when a user
  is deleted, `User.count` decreases by 1. After this change, deleting a user
  creates (potentially) a ghost user, causing `User.count` not to change. The
  spec has been updated to look for the relevant user in the assertion.
---
 app/models/user.rb                    | 32 ++++++++++++++++++-
 app/services/users/destroy_service.rb | 14 +++++++++
 spec/features/profile_spec.rb         |  4 +--
 spec/models/user_spec.rb              | 39 +++++++++++++++++++++++-
 spec/services/users/destroy_spec.rb   | 44 +++++++++++++++++++++++++++
 5 files changed, 129 insertions(+), 4 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index fada0e567f0..23beaa6c7c8 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -81,7 +81,6 @@ class User < ActiveRecord::Base
   has_many :authorized_projects, through: :project_authorizations, source: :project
 
   has_many :snippets,                 dependent: :destroy, foreign_key: :author_id
-  has_many :issues,                   dependent: :destroy, foreign_key: :author_id
   has_many :notes,                    dependent: :destroy, foreign_key: :author_id
   has_many :merge_requests,           dependent: :destroy, foreign_key: :author_id
   has_many :events,                   dependent: :destroy, foreign_key: :author_id
@@ -99,6 +98,11 @@ class User < ActiveRecord::Base
   has_many :assigned_issues,          dependent: :nullify, foreign_key: :assignee_id, class_name: "Issue"
   has_many :assigned_merge_requests,  dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest"
 
+  # Issues that a user owns are expected to be moved to the "ghost" user before
+  # the user is destroyed. If the user owns any issues during deletion, this
+  # should be treated as an exceptional condition.
+  has_many :issues,                   dependent: :restrict_with_exception, foreign_key: :author_id
+
   #
   # Validations
   #
@@ -181,6 +185,8 @@ class User < ActiveRecord::Base
           "administrator if you think this is an error."
       end
     end
+
+    state :ghost
   end
 
   mount_uploader :avatar, AvatarUploader
@@ -337,6 +343,30 @@ class User < ActiveRecord::Base
         (?<user>#{Gitlab::Regex::NAMESPACE_REF_REGEX_STR})
       }x
     end
+
+    # Return (create if necessary) the ghost user. The ghost user
+    # owns records previously belonging to deleted users.
+    def ghost
+      ghost_user = User.with_state(:ghost).first
+
+      ghost_user ||
+        begin
+          users = Enumerator.new do |y|
+            n = nil
+            loop do
+              user = User.new(
+                username: "ghost#{n}", password: Devise.friendly_token,
+                email: "ghost#{n}@example.com", name: "Ghost User", state: :ghost
+              )
+
+              y.yield(user)
+              n = n ? n.next : 0
+            end
+          end
+
+          users.lazy.select { |user| user.valid? }.first.tap(&:save!)
+        end
+    end
   end
 
   #
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb
index bc0653cb634..d88b81c04e2 100644
--- a/app/services/users/destroy_service.rb
+++ b/app/services/users/destroy_service.rb
@@ -26,6 +26,8 @@ module Users
         ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute
       end
 
+      move_issues_to_ghost_user(user)
+
       # Destroy the namespace after destroying the user since certain methods may depend on the namespace existing
       namespace = user.namespace
       user_data = user.destroy
@@ -33,5 +35,17 @@ module Users
 
       user_data
     end
+
+    private
+
+    def move_issues_to_ghost_user(user)
+      ghost_user = User.ghost
+
+      Issue.transaction do
+        user.issues.each { |issue| issue.update!(author: ghost_user) }
+      end
+
+      user.reload
+    end
   end
 end
diff --git a/spec/features/profile_spec.rb b/spec/features/profile_spec.rb
index 7a562b5e03d..406d7cf791c 100644
--- a/spec/features/profile_spec.rb
+++ b/spec/features/profile_spec.rb
@@ -4,7 +4,7 @@ describe 'Profile account page', feature: true do
   let(:user) { create(:user) }
 
   before do
-    login_as :user
+    login_as(user)
   end
 
   describe 'when signup is enabled' do
@@ -16,7 +16,7 @@ describe 'Profile account page', feature: true do
     it { expect(page).to have_content('Remove account') }
 
     it 'deletes the account' do
-      expect { click_link 'Delete account' }.to change { User.count }.by(-1)
+      expect { click_link 'Delete account' }.to change { User.where(id: user.id).count }.by(-1)
       expect(current_path).to eq(new_user_session_path)
     end
   end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index b69fd24c586..00e7927bf90 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -22,7 +22,7 @@ describe User, models: true do
     it { is_expected.to have_many(:deploy_keys).dependent(:destroy) }
     it { is_expected.to have_many(:events).dependent(:destroy) }
     it { is_expected.to have_many(:recent_events).class_name('Event') }
-    it { is_expected.to have_many(:issues).dependent(:destroy) }
+    it { is_expected.to have_many(:issues).dependent(:restrict_with_exception) }
     it { is_expected.to have_many(:notes).dependent(:destroy) }
     it { is_expected.to have_many(:assigned_issues).dependent(:nullify) }
     it { is_expected.to have_many(:merge_requests).dependent(:destroy) }
@@ -1490,4 +1490,41 @@ describe User, models: true do
       expect(user.admin).to be true
     end
   end
+
+  describe '.ghost' do
+    it "creates a ghost user if one isn't already present" do
+      ghost = User.ghost
+
+      expect(ghost).to be_ghost
+      expect(ghost).to be_persisted
+    end
+
+    it "does not create a second ghost user if one is already present" do
+      expect do
+        User.ghost
+        User.ghost
+      end.to change { User.count }.by(1)
+      expect(User.ghost).to eq(User.ghost)
+    end
+
+    context "when a regular user exists with the username 'ghost'" do
+      it "creates a ghost user with a non-conflicting username" do
+        create(:user, username: 'ghost')
+        ghost = User.ghost
+
+        expect(ghost).to be_persisted
+        expect(ghost.username).to eq('ghost0')
+      end
+    end
+
+    context "when a regular user exists with the email 'ghost@example.com'" do
+      it "creates a ghost user with a non-conflicting email" do
+        create(:user, email: 'ghost@example.com')
+        ghost = User.ghost
+
+        expect(ghost).to be_persisted
+        expect(ghost.email).to eq('ghost0@example.com')
+      end
+    end
+  end
 end
diff --git a/spec/services/users/destroy_spec.rb b/spec/services/users/destroy_spec.rb
index c0bf27c698c..4f0834064e5 100644
--- a/spec/services/users/destroy_spec.rb
+++ b/spec/services/users/destroy_spec.rb
@@ -24,6 +24,50 @@ describe Users::DestroyService, services: true do
       end
     end
 
+    context "a deleted user's issues" do
+      let(:project) { create :project }
+
+      before do
+        project.add_developer(user)
+      end
+
+      context "for an issue the user has created" do
+        let!(:issue) { create(:issue, project: project, author: user) }
+
+        before do
+          service.execute(user)
+        end
+
+        it 'does not delete the issue' do
+          expect(Issue.find_by_id(issue.id)).to be_present
+        end
+
+        it 'migrates the issue so that the "Ghost User" is the issue owner' do
+          migrated_issue = Issue.find_by_id(issue.id)
+
+          expect(migrated_issue.author).to eq(User.ghost)
+        end
+      end
+
+      context "for an issue the user was assigned to" do
+        let!(:issue) { create(:issue, project: project, assignee: user) }
+
+        before do
+          service.execute(user)
+        end
+
+        it 'does not delete issues the user is assigned to' do
+          expect(Issue.find_by_id(issue.id)).to be_present
+        end
+
+        it 'migrates the issue so that it is "Unassigned"' do
+          migrated_issue = Issue.find_by_id(issue.id)
+
+          expect(migrated_issue.assignee).to be_nil
+        end
+      end
+    end
+
     context "solo owned groups present" do
       let(:solo_owned)  { create(:group) }
       let(:member)      { create(:group_member) }

From ca16c3734b7b89f71bdc9e1c18152aa1599c4f89 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 3 Feb 2017 18:25:50 +0530
Subject: [PATCH 04/11] Extract code from `Namespace#clean_path` for ghost user
 generation.

1. Create a `Uniquify` class, which generalizes the process of generating unique
   strings, by accepting a function that defines what "uniqueness" means in a
   given context.

2. WIP: Make sure tests for `Namespace` pass, add more if necessary.

3. WIP: Add tests for `Uniquify`
---
 app/models/concerns/uniquify.rb | 25 +++++++++++++++++++++++++
 app/models/namespace.rb         | 10 ++--------
 app/models/user.rb              | 23 +++++++++++------------
 spec/models/user_spec.rb        |  4 ++--
 4 files changed, 40 insertions(+), 22 deletions(-)
 create mode 100644 app/models/concerns/uniquify.rb

diff --git a/app/models/concerns/uniquify.rb b/app/models/concerns/uniquify.rb
new file mode 100644
index 00000000000..1485ab6ae99
--- /dev/null
+++ b/app/models/concerns/uniquify.rb
@@ -0,0 +1,25 @@
+class Uniquify
+  # Return a version of the given 'base' string that is unique
+  # by appending a counter to it. Uniqueness is determined by
+  # repeated calls to `exists_fn`.
+  #
+  # If `base` is a function/proc, we expect that calling it with a
+  # candidate counter returns a string to test/return.
+  def string(base, exists_fn)
+    @counter = nil
+
+    if base.respond_to?(:call)
+      increment_counter! while exists_fn[base.call(@counter)]
+      base.call(@counter)
+    else
+      increment_counter! while exists_fn["#{base}#{@counter}"]
+      "#{base}#{@counter}"
+    end
+  end
+
+  private
+
+  def increment_counter!
+    @counter = @counter ? @counter.next : 1
+  end
+end
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index bd0336c984a..8cc3c1473e2 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -98,14 +98,8 @@ class Namespace < ActiveRecord::Base
       # Work around that by setting their username to "blank", followed by a counter.
       path = "blank" if path.blank?
 
-      counter = 0
-      base = path
-      while Namespace.find_by_path_or_name(path)
-        counter += 1
-        path = "#{base}#{counter}"
-      end
-
-      path
+      uniquify = Uniquify.new
+      uniquify.string(path, -> (s) { Namespace.find_by_path_or_name(s) })
     end
   end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 23beaa6c7c8..13529c9997a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -351,20 +351,19 @@ class User < ActiveRecord::Base
 
       ghost_user ||
         begin
-          users = Enumerator.new do |y|
-            n = nil
-            loop do
-              user = User.new(
-                username: "ghost#{n}", password: Devise.friendly_token,
-                email: "ghost#{n}@example.com", name: "Ghost User", state: :ghost
-              )
+          uniquify = Uniquify.new
 
-              y.yield(user)
-              n = n ? n.next : 0
-            end
-          end
+          username = uniquify.string("ghost", -> (s) { User.find_by_username(s) })
 
-          users.lazy.select { |user| user.valid? }.first.tap(&:save!)
+          email = uniquify.string(
+            -> (n) { "ghost#{n}@example.com" },
+            -> (s) { User.find_by_email(s) }
+          )
+
+          User.create(
+            username: username, password: Devise.friendly_token,
+            email: email, name: "Ghost User", state: :ghost
+          )
         end
     end
   end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 00e7927bf90..861d338ef95 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1513,7 +1513,7 @@ describe User, models: true do
         ghost = User.ghost
 
         expect(ghost).to be_persisted
-        expect(ghost.username).to eq('ghost0')
+        expect(ghost.username).to eq('ghost1')
       end
     end
 
@@ -1523,7 +1523,7 @@ describe User, models: true do
         ghost = User.ghost
 
         expect(ghost).to be_persisted
-        expect(ghost.email).to eq('ghost0@example.com')
+        expect(ghost.email).to eq('ghost1@example.com')
       end
     end
   end

From 53c34c7436112d7cac9c3887ada1d5ae630a206c Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Mon, 6 Feb 2017 17:18:27 +0530
Subject: [PATCH 05/11] Implement review comments from @DouweM and
 @nick.thomas.

1. Use an advisory lock to guarantee the absence of concurrency in `User.ghost`,
to prevent data races from creating more than one ghost, or preventing the
creation of ghost users by causing validation errors.

2. Use `update_all` instead of updating issues one-by-one.
---
 app/models/user.rb                      | 13 +++++
 app/services/users/destroy_service.rb   |  4 +-
 lib/gitlab/database/advisory_locking.rb | 70 +++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 2 deletions(-)
 create mode 100644 lib/gitlab/database/advisory_locking.rb

diff --git a/app/models/user.rb b/app/models/user.rb
index 13529c9997a..e4adcd6cde9 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -351,6 +351,17 @@ class User < ActiveRecord::Base
 
       ghost_user ||
         begin
+          # Since we only want a single ghost user in an instance, we use an
+          # advisory lock to ensure than this block is never run concurrently.
+          advisory_lock = Gitlab::Database::AdvisoryLocking.new(:ghost_user)
+          advisory_lock.lock
+
+          # Recheck if a ghost user is already present (one might have been)
+          # added between the time we last checked (first line of this method)
+          # and the time we acquired the lock.
+          ghost_user = User.with_state(:ghost).first
+          return ghost_user if ghost_user.present?
+
           uniquify = Uniquify.new
 
           username = uniquify.string("ghost", -> (s) { User.find_by_username(s) })
@@ -364,6 +375,8 @@ class User < ActiveRecord::Base
             username: username, password: Devise.friendly_token,
             email: email, name: "Ghost User", state: :ghost
           )
+        ensure
+          advisory_lock.unlock
         end
     end
   end
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb
index d88b81c04e2..bf1e49734cc 100644
--- a/app/services/users/destroy_service.rb
+++ b/app/services/users/destroy_service.rb
@@ -37,12 +37,12 @@ module Users
     end
 
     private
-
+    
     def move_issues_to_ghost_user(user)
       ghost_user = User.ghost
 
       Issue.transaction do
-        user.issues.each { |issue| issue.update!(author: ghost_user) }
+        user.issues.update_all(author_id: ghost_user.id)
       end
 
       user.reload
diff --git a/lib/gitlab/database/advisory_locking.rb b/lib/gitlab/database/advisory_locking.rb
new file mode 100644
index 00000000000..950898288be
--- /dev/null
+++ b/lib/gitlab/database/advisory_locking.rb
@@ -0,0 +1,70 @@
+# An advisory lock is an application-level database lock which isn't tied
+# to a specific table or row.
+#
+# Postgres names its advisory locks with integers, while MySQL uses strings.
+# We support both here by using a `LOCK_TYPES` map of symbols to integers.
+# The symbol (stringified) is used for MySQL, and the corresponding integer
+# is used for Postgres.
+module Gitlab
+  module Database
+    class AdvisoryLocking
+      LOCK_TYPES = {
+        ghost_user: 1
+      }
+
+      def initialize(lock_type)
+        @lock_type = lock_type
+      end
+
+      def lock
+        ensure_valid_lock_type!
+
+        query =
+          if Gitlab::Database.postgresql?
+            Arel::SelectManager.new(ActiveRecord::Base).project(
+              Arel::Nodes::NamedFunction.new("pg_advisory_lock", [LOCK_TYPES[@lock_type]])
+            )
+          elsif Gitlab::Database.mysql?
+            Arel::SelectManager.new(ActiveRecord::Base).project(
+              Arel::Nodes::NamedFunction.new("get_lock", [Arel.sql("'#{@lock_type}'"), -1])
+            )
+          end
+
+        run_query(query)
+      end
+
+      def unlock
+        ensure_valid_lock_type!
+
+        query =
+          if Gitlab::Database.postgresql?
+            Arel::SelectManager.new(ActiveRecord::Base).project(
+              Arel::Nodes::NamedFunction.new("pg_advisory_unlock", [LOCK_TYPES[@lock_type]])
+            )
+          elsif Gitlab::Database.mysql?
+            Arel::SelectManager.new(ActiveRecord::Base).project(
+              Arel::Nodes::NamedFunction.new("release_lock", [Arel.sql("'#{@lock_type}'")])
+            )
+          end
+
+        run_query(query)
+      end
+
+      private
+
+      def ensure_valid_lock_type!
+        unless valid_lock_type?
+          raise RuntimeError, "Trying to use an advisory lock with an invalid lock type, #{@lock_type}."
+        end
+      end
+
+      def valid_lock_type?
+        LOCK_TYPES.keys.include?(@lock_type)
+      end
+
+      def run_query(arel_query)
+        ActiveRecord::Base.connection.execute(arel_query.to_sql)
+      end
+    end
+  end
+end

From 8e684809765fa866a125c176327825ebc565f5b3 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Mon, 6 Feb 2017 17:37:05 +0530
Subject: [PATCH 06/11] Use a `ghost` boolean to track ghost users.

Rather than using a separate `ghost` state. This lets us have the benefits of
both ghost and blocked users (ghost: true, state: blocked) without having to
rewrite a number of queries to include cases for `state: ghost`.
---
 app/models/user.rb                                | 15 ++++++++++-----
 app/services/notification_service.rb              |  1 -
 .../20170206115204_add_column_ghost_to_users.rb   | 15 +++++++++++++++
 db/schema.rb                                      |  1 +
 spec/factories/users.rb                           |  4 ++++
 spec/models/user_spec.rb                          | 14 ++++++++++++++
 6 files changed, 44 insertions(+), 6 deletions(-)
 create mode 100644 db/migrate/20170206115204_add_column_ghost_to_users.rb

diff --git a/app/models/user.rb b/app/models/user.rb
index e4adcd6cde9..f71d7e90d33 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -124,6 +124,7 @@ class User < ActiveRecord::Base
   validate :unique_email, if: ->(user) { user.email_changed? }
   validate :owns_notification_email, if: ->(user) { user.notification_email_changed? }
   validate :owns_public_email, if: ->(user) { user.public_email_changed? }
+  validate :ghost_users_must_be_blocked
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
   before_validation :generate_password, on: :create
@@ -185,8 +186,6 @@ class User < ActiveRecord::Base
           "administrator if you think this is an error."
       end
     end
-
-    state :ghost
   end
 
   mount_uploader :avatar, AvatarUploader
@@ -347,7 +346,7 @@ class User < ActiveRecord::Base
     # Return (create if necessary) the ghost user. The ghost user
     # owns records previously belonging to deleted users.
     def ghost
-      ghost_user = User.with_state(:ghost).first
+      ghost_user = User.find_by_ghost(true)
 
       ghost_user ||
         begin
@@ -359,7 +358,7 @@ class User < ActiveRecord::Base
           # Recheck if a ghost user is already present (one might have been)
           # added between the time we last checked (first line of this method)
           # and the time we acquired the lock.
-          ghost_user = User.with_state(:ghost).first
+          ghost_user = User.find_by_ghost(true)
           return ghost_user if ghost_user.present?
 
           uniquify = Uniquify.new
@@ -373,7 +372,7 @@ class User < ActiveRecord::Base
 
           User.create(
             username: username, password: Devise.friendly_token,
-            email: email, name: "Ghost User", state: :ghost
+            email: email, name: "Ghost User", state: :blocked, ghost: true
           )
         ensure
           advisory_lock.unlock
@@ -477,6 +476,12 @@ class User < ActiveRecord::Base
     errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email)
   end
 
+  def ghost_users_must_be_blocked
+    if ghost? && !blocked?
+      errors.add(:ghost, 'cannot be enabled for a user who is not blocked.')
+    end
+  end
+
   def update_emails_with_primary_email
     primary_email_record = emails.find_by(email: email)
     if primary_email_record
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 97db117aa99..3734e3c4253 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -466,7 +466,6 @@ class NotificationService
 
     users = users.to_a.compact.uniq
     users = users.reject(&:blocked?)
-    users = users.reject(&:ghost?)
 
     users.reject do |user|
       global_notification_setting = user.global_notification_setting
diff --git a/db/migrate/20170206115204_add_column_ghost_to_users.rb b/db/migrate/20170206115204_add_column_ghost_to_users.rb
new file mode 100644
index 00000000000..57b66ca91a8
--- /dev/null
+++ b/db/migrate/20170206115204_add_column_ghost_to_users.rb
@@ -0,0 +1,15 @@
+class AddColumnGhostToUsers < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default :users, :ghost, :boolean, default: false, allow_null: false
+  end
+
+  def down
+    remove_column :users, :ghost
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 82bccda580a..c50d6fc4ad6 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1282,6 +1282,7 @@ ActiveRecord::Schema.define(version: 20170216141440) do
     t.string "incoming_email_token"
     t.boolean "authorized_projects_populated"
     t.boolean "notified_of_own_activity", default: false, null: false
+    t.boolean "ghost", default: false, null: false
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree
diff --git a/spec/factories/users.rb b/spec/factories/users.rb
index 1732b1a0081..58602c54c7a 100644
--- a/spec/factories/users.rb
+++ b/spec/factories/users.rb
@@ -26,6 +26,10 @@ FactoryGirl.define do
       two_factor_via_otp
     end
 
+    trait :ghost do
+      ghost true
+    end
+
     trait :two_factor_via_otp do
       before(:create) do |user|
         user.otp_required_for_login = true
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 861d338ef95..9e2b1c9290f 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -208,6 +208,20 @@ describe User, models: true do
         end
       end
     end
+
+    describe 'ghost users' do
+      it 'does not allow a non-blocked ghost user' do
+        user = build(:user, :ghost, state: :active)
+
+        expect(user).to be_invalid
+      end
+
+      it 'allows a blocked ghost user' do
+        user = build(:user, :ghost, state: :blocked)
+
+        expect(user).to be_valid
+      end
+    end
   end
 
   describe "scopes" do

From 8f01644ff4d25285475cbf053b140292ac50f225 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 7 Feb 2017 00:32:35 +0530
Subject: [PATCH 07/11] Implement review comments from @rymai and
 @yorickpeterse

1. Refactoring and specs in the `Uniquify` class.

2. Don't use the `AdvisoryLocking` class. Similar functionality is
provided (backed by Redis) in the `ExclusiveLease` class.
---
 app/models/concerns/uniquify.rb         | 18 ++++---
 app/models/user.rb                      | 14 +++--
 lib/gitlab/database/advisory_locking.rb | 70 -------------------------
 spec/models/concerns/uniquify_spec.rb   | 40 ++++++++++++++
 4 files changed, 61 insertions(+), 81 deletions(-)
 delete mode 100644 lib/gitlab/database/advisory_locking.rb
 create mode 100644 spec/models/concerns/uniquify_spec.rb

diff --git a/app/models/concerns/uniquify.rb b/app/models/concerns/uniquify.rb
index 1485ab6ae99..6734472bc6d 100644
--- a/app/models/concerns/uniquify.rb
+++ b/app/models/concerns/uniquify.rb
@@ -6,19 +6,23 @@ class Uniquify
   # If `base` is a function/proc, we expect that calling it with a
   # candidate counter returns a string to test/return.
   def string(base, exists_fn)
+    @base = base
     @counter = nil
 
-    if base.respond_to?(:call)
-      increment_counter! while exists_fn[base.call(@counter)]
-      base.call(@counter)
-    else
-      increment_counter! while exists_fn["#{base}#{@counter}"]
-      "#{base}#{@counter}"
-    end
+    increment_counter! while exists_fn[base_string]
+    base_string
   end
 
   private
 
+  def base_string
+    if @base.respond_to?(:call)
+      @base.call(@counter)
+    else
+      "#{@base}#{@counter}"
+    end
+  end
+
   def increment_counter!
     @counter = @counter ? @counter.next : 1
   end
diff --git a/app/models/user.rb b/app/models/user.rb
index f71d7e90d33..7207cdc4581 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -351,9 +351,15 @@ class User < ActiveRecord::Base
       ghost_user ||
         begin
           # Since we only want a single ghost user in an instance, we use an
-          # advisory lock to ensure than this block is never run concurrently.
-          advisory_lock = Gitlab::Database::AdvisoryLocking.new(:ghost_user)
-          advisory_lock.lock
+          # exclusive lease to ensure than this block is never run concurrently.
+          lease_key = "ghost_user_creation"
+          lease = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.minute.to_i)
+
+          until uuid = lease.try_obtain
+            # Keep trying until we obtain the lease. To prevent hammering Redis too
+            # much we'll wait for a bit between retries.
+            sleep(1)
+          end
 
           # Recheck if a ghost user is already present (one might have been)
           # added between the time we last checked (first line of this method)
@@ -375,7 +381,7 @@ class User < ActiveRecord::Base
             email: email, name: "Ghost User", state: :blocked, ghost: true
           )
         ensure
-          advisory_lock.unlock
+          Gitlab::ExclusiveLease.cancel(lease_key, uuid)
         end
     end
   end
diff --git a/lib/gitlab/database/advisory_locking.rb b/lib/gitlab/database/advisory_locking.rb
deleted file mode 100644
index 950898288be..00000000000
--- a/lib/gitlab/database/advisory_locking.rb
+++ /dev/null
@@ -1,70 +0,0 @@
-# An advisory lock is an application-level database lock which isn't tied
-# to a specific table or row.
-#
-# Postgres names its advisory locks with integers, while MySQL uses strings.
-# We support both here by using a `LOCK_TYPES` map of symbols to integers.
-# The symbol (stringified) is used for MySQL, and the corresponding integer
-# is used for Postgres.
-module Gitlab
-  module Database
-    class AdvisoryLocking
-      LOCK_TYPES = {
-        ghost_user: 1
-      }
-
-      def initialize(lock_type)
-        @lock_type = lock_type
-      end
-
-      def lock
-        ensure_valid_lock_type!
-
-        query =
-          if Gitlab::Database.postgresql?
-            Arel::SelectManager.new(ActiveRecord::Base).project(
-              Arel::Nodes::NamedFunction.new("pg_advisory_lock", [LOCK_TYPES[@lock_type]])
-            )
-          elsif Gitlab::Database.mysql?
-            Arel::SelectManager.new(ActiveRecord::Base).project(
-              Arel::Nodes::NamedFunction.new("get_lock", [Arel.sql("'#{@lock_type}'"), -1])
-            )
-          end
-
-        run_query(query)
-      end
-
-      def unlock
-        ensure_valid_lock_type!
-
-        query =
-          if Gitlab::Database.postgresql?
-            Arel::SelectManager.new(ActiveRecord::Base).project(
-              Arel::Nodes::NamedFunction.new("pg_advisory_unlock", [LOCK_TYPES[@lock_type]])
-            )
-          elsif Gitlab::Database.mysql?
-            Arel::SelectManager.new(ActiveRecord::Base).project(
-              Arel::Nodes::NamedFunction.new("release_lock", [Arel.sql("'#{@lock_type}'")])
-            )
-          end
-
-        run_query(query)
-      end
-
-      private
-
-      def ensure_valid_lock_type!
-        unless valid_lock_type?
-          raise RuntimeError, "Trying to use an advisory lock with an invalid lock type, #{@lock_type}."
-        end
-      end
-
-      def valid_lock_type?
-        LOCK_TYPES.keys.include?(@lock_type)
-      end
-
-      def run_query(arel_query)
-        ActiveRecord::Base.connection.execute(arel_query.to_sql)
-      end
-    end
-  end
-end
diff --git a/spec/models/concerns/uniquify_spec.rb b/spec/models/concerns/uniquify_spec.rb
new file mode 100644
index 00000000000..4e06a1f4c31
--- /dev/null
+++ b/spec/models/concerns/uniquify_spec.rb
@@ -0,0 +1,40 @@
+require 'spec_helper'
+
+describe Uniquify, models: true do
+  describe "#string" do
+    it 'returns the given string if it does not exist' do
+      uniquify = Uniquify.new
+
+      result = uniquify.string('test_string', -> (s) { false })
+
+      expect(result).to eq('test_string')
+    end
+
+    it 'returns the given string with a counter attached if the string exists' do
+      uniquify = Uniquify.new
+
+      result = uniquify.string('test_string', -> (s) { true if s == 'test_string' })
+
+      expect(result).to eq('test_string1')
+    end
+
+    it 'increments the counter for each candidate string that also exists' do
+      uniquify = Uniquify.new
+
+      result = uniquify.string('test_string', -> (s) { true if s == 'test_string' || s == 'test_string1' })
+
+      expect(result).to eq('test_string2')
+    end
+
+    it 'allows passing in a base function that defines the location of the counter' do
+      uniquify = Uniquify.new
+
+      result = uniquify.string(
+        -> (counter) { "test_#{counter}_string" },
+        -> (s) { true if s == 'test__string' }
+      )
+
+      expect(result).to eq('test_1_string')
+    end
+  end
+end

From 3bd2a98f64a12a2e23a6b9ce77427a9c2033a6bf Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 7 Feb 2017 12:13:24 +0530
Subject: [PATCH 08/11] Remove the default value for the `users.ghost` database
 column.

The default (false) is not strictly required, and this lets us avoid a
potentially expensive migration
---
 db/migrate/20170206115204_add_column_ghost_to_users.rb | 6 +-----
 db/schema.rb                                           | 4 ++--
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/db/migrate/20170206115204_add_column_ghost_to_users.rb b/db/migrate/20170206115204_add_column_ghost_to_users.rb
index 57b66ca91a8..cc1eeda1160 100644
--- a/db/migrate/20170206115204_add_column_ghost_to_users.rb
+++ b/db/migrate/20170206115204_add_column_ghost_to_users.rb
@@ -1,12 +1,8 @@
 class AddColumnGhostToUsers < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
   DOWNTIME = false
 
-  disable_ddl_transaction!
-
   def up
-    add_column_with_default :users, :ghost, :boolean, default: false, allow_null: false
+    add_column :users, :ghost, :boolean
   end
 
   def down
diff --git a/db/schema.rb b/db/schema.rb
index c50d6fc4ad6..1d94368f66e 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1278,11 +1278,11 @@ ActiveRecord::Schema.define(version: 20170216141440) do
     t.datetime "otp_grace_period_started_at"
     t.boolean "ldap_email", default: false, null: false
     t.boolean "external", default: false
-    t.string "organization"
     t.string "incoming_email_token"
+    t.string "organization"
     t.boolean "authorized_projects_populated"
     t.boolean "notified_of_own_activity", default: false, null: false
-    t.boolean "ghost", default: false, null: false
+    t.boolean "ghost"
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree

From f2ed82fa8486875660b80dd061827ac8b86d00b6 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Thu, 16 Feb 2017 12:35:10 +0530
Subject: [PATCH 09/11] Implement final review comments from @DouweM and @rymai

- Have `Uniquify` take a block instead of a Proc/function. This is more
  idiomatic than passing around a function in Ruby.

- Block a user before moving their issues to the ghost user. This avoids a data
  race where an issue is created after the issues are migrated to the ghost user,
  and before the destroy takes place.

- No need to migrate issues (to the ghost user) in a transaction, because
  we're using `update_all`

- Other minor changes
---
 app/models/concerns/uniquify.rb       |  9 ++--
 app/models/namespace.rb               |  2 +-
 app/models/user.rb                    | 72 +++++++++++++--------------
 app/services/users/destroy_service.rb | 13 +++--
 spec/models/concerns/uniquify_spec.rb | 23 +++------
 spec/services/users/destroy_spec.rb   |  4 ++
 6 files changed, 62 insertions(+), 61 deletions(-)

diff --git a/app/models/concerns/uniquify.rb b/app/models/concerns/uniquify.rb
index 6734472bc6d..a7fe5951b6e 100644
--- a/app/models/concerns/uniquify.rb
+++ b/app/models/concerns/uniquify.rb
@@ -1,15 +1,15 @@
 class Uniquify
   # Return a version of the given 'base' string that is unique
   # by appending a counter to it. Uniqueness is determined by
-  # repeated calls to `exists_fn`.
+  # repeated calls to the passed block.
   #
   # If `base` is a function/proc, we expect that calling it with a
   # candidate counter returns a string to test/return.
-  def string(base, exists_fn)
+  def string(base)
     @base = base
     @counter = nil
 
-    increment_counter! while exists_fn[base_string]
+    increment_counter! while yield(base_string)
     base_string
   end
 
@@ -24,6 +24,7 @@ class Uniquify
   end
 
   def increment_counter!
-    @counter = @counter ? @counter.next : 1
+    @counter ||= 0
+    @counter += 1
   end
 end
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 8cc3c1473e2..3137dd32f93 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -99,7 +99,7 @@ class Namespace < ActiveRecord::Base
       path = "blank" if path.blank?
 
       uniquify = Uniquify.new
-      uniquify.string(path, -> (s) { Namespace.find_by_path_or_name(s) })
+      uniquify.string(path) { |s| Namespace.find_by_path_or_name(s) }
     end
   end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 7207cdc4581..4bb9dc0c59f 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -346,43 +346,7 @@ class User < ActiveRecord::Base
     # Return (create if necessary) the ghost user. The ghost user
     # owns records previously belonging to deleted users.
     def ghost
-      ghost_user = User.find_by_ghost(true)
-
-      ghost_user ||
-        begin
-          # Since we only want a single ghost user in an instance, we use an
-          # exclusive lease to ensure than this block is never run concurrently.
-          lease_key = "ghost_user_creation"
-          lease = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.minute.to_i)
-
-          until uuid = lease.try_obtain
-            # Keep trying until we obtain the lease. To prevent hammering Redis too
-            # much we'll wait for a bit between retries.
-            sleep(1)
-          end
-
-          # Recheck if a ghost user is already present (one might have been)
-          # added between the time we last checked (first line of this method)
-          # and the time we acquired the lock.
-          ghost_user = User.find_by_ghost(true)
-          return ghost_user if ghost_user.present?
-
-          uniquify = Uniquify.new
-
-          username = uniquify.string("ghost", -> (s) { User.find_by_username(s) })
-
-          email = uniquify.string(
-            -> (n) { "ghost#{n}@example.com" },
-            -> (s) { User.find_by_email(s) }
-          )
-
-          User.create(
-            username: username, password: Devise.friendly_token,
-            email: email, name: "Ghost User", state: :blocked, ghost: true
-          )
-        ensure
-          Gitlab::ExclusiveLease.cancel(lease_key, uuid)
-        end
+      User.find_by_ghost(true) || create_ghost_user
     end
   end
 
@@ -1052,4 +1016,38 @@ class User < ActiveRecord::Base
       super
     end
   end
+
+  def self.create_ghost_user
+    # Since we only want a single ghost user in an instance, we use an
+    # exclusive lease to ensure than this block is never run concurrently.
+    lease_key = "ghost_user_creation"
+    lease = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.minute.to_i)
+
+    until uuid = lease.try_obtain
+      # Keep trying until we obtain the lease. To prevent hammering Redis too
+      # much we'll wait for a bit between retries.
+      sleep(1)
+    end
+
+    # Recheck if a ghost user is already present. One might have been
+    # added between the time we last checked (first line of this method)
+    # and the time we acquired the lock.
+    ghost_user = User.find_by_ghost(true)
+    return ghost_user if ghost_user.present?
+
+    uniquify = Uniquify.new
+
+    username = uniquify.string("ghost") { |s| User.find_by_username(s) }
+
+    email = uniquify.string(-> (n) { "ghost#{n}@example.com" }) do |s|
+      User.find_by_email(s)
+    end
+
+    User.create(
+      username: username, password: Devise.friendly_token,
+      email: email, name: "Ghost User", state: :blocked, ghost: true
+    )
+  ensure
+    Gitlab::ExclusiveLease.cancel(lease_key, uuid)
+  end
 end
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb
index bf1e49734cc..523279944ae 100644
--- a/app/services/users/destroy_service.rb
+++ b/app/services/users/destroy_service.rb
@@ -37,13 +37,18 @@ module Users
     end
 
     private
-    
+
     def move_issues_to_ghost_user(user)
+      # Block the user before moving issues to prevent a data race.
+      # If the user creates an issue after `move_issues_to_ghost_user`
+      # runs and before the user is destroyed, the destroy will fail with
+      # an exception. We block the user so that issues can't be created
+      # after `move_issues_to_ghost_user` runs and before the destroy happens.
+      user.block
+
       ghost_user = User.ghost
 
-      Issue.transaction do
-        user.issues.update_all(author_id: ghost_user.id)
-      end
+      user.issues.update_all(author_id: ghost_user.id)
 
       user.reload
     end
diff --git a/spec/models/concerns/uniquify_spec.rb b/spec/models/concerns/uniquify_spec.rb
index 4e06a1f4c31..83187d732e4 100644
--- a/spec/models/concerns/uniquify_spec.rb
+++ b/spec/models/concerns/uniquify_spec.rb
@@ -1,38 +1,31 @@
 require 'spec_helper'
 
 describe Uniquify, models: true do
+  let(:uniquify) { described_class.new }
+
   describe "#string" do
     it 'returns the given string if it does not exist' do
-      uniquify = Uniquify.new
-
-      result = uniquify.string('test_string', -> (s) { false })
+      result = uniquify.string('test_string') { |s| false }
 
       expect(result).to eq('test_string')
     end
 
     it 'returns the given string with a counter attached if the string exists' do
-      uniquify = Uniquify.new
-
-      result = uniquify.string('test_string', -> (s) { true if s == 'test_string' })
+      result = uniquify.string('test_string') { |s| s == 'test_string' }
 
       expect(result).to eq('test_string1')
     end
 
     it 'increments the counter for each candidate string that also exists' do
-      uniquify = Uniquify.new
-
-      result = uniquify.string('test_string', -> (s) { true if s == 'test_string' || s == 'test_string1' })
+      result = uniquify.string('test_string') { |s| s == 'test_string' || s == 'test_string1' }
 
       expect(result).to eq('test_string2')
     end
 
     it 'allows passing in a base function that defines the location of the counter' do
-      uniquify = Uniquify.new
-
-      result = uniquify.string(
-        -> (counter) { "test_#{counter}_string" },
-        -> (s) { true if s == 'test__string' }
-      )
+      result = uniquify.string(-> (counter) { "test_#{counter}_string" }) do |s|
+        s == 'test__string'
+      end
 
       expect(result).to eq('test_1_string')
     end
diff --git a/spec/services/users/destroy_spec.rb b/spec/services/users/destroy_spec.rb
index 4f0834064e5..922e82445d0 100644
--- a/spec/services/users/destroy_spec.rb
+++ b/spec/services/users/destroy_spec.rb
@@ -47,6 +47,10 @@ describe Users::DestroyService, services: true do
 
           expect(migrated_issue.author).to eq(User.ghost)
         end
+
+        it 'blocks the user before migrating issues to the "Ghost User' do
+          expect(user).to be_blocked
+        end
       end
 
       context "for an issue the user was assigned to" do

From 6fdb17cbbe5dc70d18f50e9d131ab70407976a71 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 17 Feb 2017 20:28:12 +0530
Subject: [PATCH 10/11] 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.
---
 app/models/user.rb                         |  4 ++-
 app/policies/user_policy.rb                |  8 +++++
 app/services/users/destroy_service.rb      |  2 +-
 app/views/admin/users/_user.html.haml      |  2 +-
 app/views/admin/users/show.html.haml       |  5 ++-
 app/views/profiles/accounts/show.html.haml |  5 ++-
 spec/factories/users.rb                    |  1 +
 spec/policies/user_policy_spec.rb          | 37 ++++++++++++++++++++++
 8 files changed, 59 insertions(+), 5 deletions(-)
 create mode 100644 spec/policies/user_policy_spec.rb

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

From 53aa043724a0395a2f95f41b8f3fb4c308691874 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 21 Feb 2017 12:50:05 +0530
Subject: [PATCH 11/11] Fix specs for the ghost user feature.

---
 spec/models/abuse_report_spec.rb | 2 +-
 spec/models/user_spec.rb         | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/spec/models/abuse_report_spec.rb b/spec/models/abuse_report_spec.rb
index c4486a32082..4e71597521d 100644
--- a/spec/models/abuse_report_spec.rb
+++ b/spec/models/abuse_report_spec.rb
@@ -2,7 +2,7 @@ require 'rails_helper'
 
 RSpec.describe AbuseReport, type: :model do
   subject     { create(:abuse_report) }
-  let(:user)  { create(:user) }
+  let(:user)  { create(:admin) }
 
   it { expect(subject).to be_valid }
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 9e2b1c9290f..6356f8b6c92 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -211,13 +211,15 @@ describe User, models: true do
 
     describe 'ghost users' do
       it 'does not allow a non-blocked ghost user' do
-        user = build(:user, :ghost, state: :active)
+        user = build(:user, :ghost)
+        user.state = 'active'
 
         expect(user).to be_invalid
       end
 
       it 'allows a blocked ghost user' do
-        user = build(:user, :ghost, state: :blocked)
+        user = build(:user, :ghost)
+        user.state = 'blocked'
 
         expect(user).to be_valid
       end