This removes all usage of soft removals except for the "pending delete"
system implemented for projects. This in turn simplifies all the query
plans of the models that used soft removals. Since we don't really use
soft removals for anything useful there's no point in keeping it around.
This _does_ mean that hard removals of issues (which only admins can do
if I'm not mistaken) can influence the "iid" values, but that code is
broken to begin with. More on this (and how to fix it) can be found in
https://gitlab.com/gitlab-org/gitlab-ce/issues/31114.
Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/37447
1. Have `MigrateToGhostUser` be a service rather than a mixed-in module, to keep
things explicit. Specs testing the behavior of this class are moved into a
separate service spec file.
2. Add a `user.reported_abuse_reports` association to make the
`migrate_abuse_reports` method more consistent with the other `migrate_`
methods.
1. When the user is deleted.
2. Refactor out code relating to "migrating records to the ghost user" into a
`MigrateToGhostUser` concern, which is tested using a shared example.
When deleting a user, the following sequence could happen:
1. Project `mygroup/myproject` is scheduled for deletion
2. The group `mygroup` is deleted
3. Sidekiq worker runs to delete `mygroup/myproject`, but the namespace and routes have
been destroyed.
Closes#30334
- 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.
- 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
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.
- "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.
We saw from a recent incident that the `Users::DestroyService` would
attempt to delete a user over and over. Revoking the permissions
from the current user did not help. We should ensure that the
current user does, in fact, have permissions to delete the user.
Signed-off-by: Rémy Coutable <remy@rymai.me>
* Changed name of delete_user_service and worker to destroy
* Move and change delete_group_service to Groups::DestroyService
* Rename Notes::DeleteService to Notes::DestroyService
Tiago Botelho