Commit d71973ad 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 040eff3e
...@@ -1079,8 +1079,10 @@ class User < ActiveRecord::Base ...@@ -1079,8 +1079,10 @@ class User < ActiveRecord::Base
User.find_by_email(s) User.find_by_email(s)
end 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( 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 email: email, name: "Ghost User", state: :blocked, ghost: true
) )
ensure ensure
......
...@@ -3,6 +3,14 @@ class UserPolicy < BasePolicy ...@@ -3,6 +3,14 @@ class UserPolicy < BasePolicy
def rules def rules
can! :read_user if @user || !restricted_public_level? 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 end
def restricted_public_level? def restricted_public_level?
......
...@@ -7,7 +7,7 @@ module Users ...@@ -7,7 +7,7 @@ module Users
end end
def execute(user, options = {}) 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}!" raise Gitlab::Access::AccessDeniedError, "#{current_user} tried to destroy user #{user}!"
end end
......
...@@ -37,7 +37,7 @@ ...@@ -37,7 +37,7 @@
- if user.access_locked? - if user.access_locked?
%li %li
= link_to 'Unlock', unlock_admin_user_path(user), method: :put, class: 'btn-grouped btn btn-xs btn-success', data: { confirm: 'Are you sure?' } = 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.divider
%li %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?" }, = 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 @@ ...@@ -173,7 +173,7 @@
.panel-heading .panel-heading
Remove user Remove user
.panel-body .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: %p Deleting a user has the following effects:
%ul %ul
%li All user content like authored issues, snippets, comments will be removed %li All user content like authored issues, snippets, comments will be removed
...@@ -189,3 +189,6 @@ ...@@ -189,3 +189,6 @@
%strong= @user.solo_owned_groups.map(&:name).join(', ') %strong= @user.solo_owned_groups.map(&:name).join(', ')
%p %p
You must transfer ownership or delete these groups before you can delete this user. 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 @@ ...@@ -115,7 +115,7 @@
%h4.prepend-top-0.danger-title %h4.prepend-top-0.danger-title
Remove account Remove account
.col-lg-9 .col-lg-9
- if @user.can_be_removed? - if @user.can_be_removed? && can?(current_user, :destroy_user, @user)
%p %p
Deleting an account has the following effects: Deleting an account has the following effects:
%ul %ul
...@@ -131,4 +131,7 @@ ...@@ -131,4 +131,7 @@
%strong= @user.solo_owned_groups.map(&:name).join(', ') %strong= @user.solo_owned_groups.map(&:name).join(', ')
%p %p
You must transfer ownership or delete these groups before you can delete your account. 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 .append-bottom-default
...@@ -32,6 +32,7 @@ FactoryGirl.define do ...@@ -32,6 +32,7 @@ FactoryGirl.define do
trait :ghost do trait :ghost do
ghost true ghost true
after(:build) { |user, _| user.block! }
end end
trait :two_factor_via_otp do 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