Merge branch '12726-preserve-issues-after-deleting-users' into 'master'

Deleting a user shouldn't delete associated issues.

Closes #12726

See merge request !7393

app/models/concerns/uniquify.rb

@@ -0,0 +1,30 @@
+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 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)
+    @base = base
+    @counter = nil
+
+    increment_counter! while yield(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 ||= 0
+    @counter += 1
+  end
+end

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
 

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
   #
@@ -120,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
@@ -337,6 +342,12 @@ class User < ActiveRecord::Base
         (?<user>#{Gitlab::Regex::FULL_NAMESPACE_REGEX_STR})
       }x
     end
+
+    # Return (create if necessary) the ghost user. The ghost user
+    # owns records previously belonging to deleted users.
+    def ghost
+      User.find_by_ghost(true) || create_ghost_user
+    end
   end
 
   #
@@ -435,6 +446,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
@@ -999,4 +1016,40 @@ 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
+
+    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, bio: bio,
+      email: email, name: "Ghost User", state: :blocked, ghost: true
+    )
+  ensure
+    Gitlab::ExclusiveLease.cancel(lease_key, uuid)
+  end
 end

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?

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
 
@@ -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,22 @@ module Users
 
       user_data
     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
+
+      user.issues.update_all(author_id: ghost_user.id)
+
+      user.reload
+    end
   end
 end

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?" },

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.

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

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:

db/migrate/20170206115204_add_column_ghost_to_users.rb

@@ -0,0 +1,11 @@
+class AddColumnGhostToUsers < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def up
+    add_column :users, :ghost, :boolean
+  end
+
+  def down
+    remove_column :users, :ghost
+  end
+end

db/schema.rb

@@ -1278,10 +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"
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree

spec/factories/users.rb

@@ -26,6 +26,11 @@ FactoryGirl.define do
       two_factor_via_otp
     end
 
+    trait :ghost do
+      ghost true
+      after(:build) { |user, _| user.block! }
+    end
+
     trait :two_factor_via_otp do
       before(:create) do |user|
         user.otp_required_for_login = true

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

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 }
 

spec/models/concerns/uniquify_spec.rb

@@ -0,0 +1,33 @@
+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
+      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
+      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
+      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
+      result = uniquify.string(-> (counter) { "test_#{counter}_string" }) do |s|
+        s == 'test__string'
+      end
+
+      expect(result).to eq('test_1_string')
+    end
+  end
+end

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) }
@@ -208,6 +208,22 @@ 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)
+        user.state = 'active'
+
+        expect(user).to be_invalid
+      end
+
+      it 'allows a blocked ghost user' do
+        user = build(:user, :ghost)
+        user.state = 'blocked'
+
+        expect(user).to be_valid
+      end
+    end
   end
 
   describe "scopes" do
@@ -1490,4 +1506,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('ghost1')
+      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('ghost1@example.com')
+      end
+    end
+  end
 end

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

spec/services/users/destroy_spec.rb

@@ -24,6 +24,54 @@ 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
+
+        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
+        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) }