Commit 6fdb17cb authored by Timothy Andrew's avatar Timothy Andrew

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.
parent f2ed82fa
......@@ -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
......
......@@ -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?
......
......@@ -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
......
......@@ -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?" },
......
......@@ -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.
......@@ -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
......@@ -28,6 +28,7 @@ FactoryGirl.define do
trait :ghost do
ghost true
after(:build) { |user, _| user.block! }
end
trait :two_factor_via_otp do
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment