From 61c385e423dcf6a4d33cc10e43a1b09cef741e71 Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Tue, 25 Aug 2026 23:03:13 +0200 Subject: [PATCH] Delete an account and dispose of its content in one transaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a staff or client account is two writes: soft-delete the account, then cascade or reassign the files and folders it owns. All four destroy() paths (Users + Clients, web + API) ran them one after the other with nothing tying them together. If the second write throws, the account is already gone but its content is not handled. The concrete way in is the reassign branch: validate() checks reassign_to_id with exists(active), but apply() re-resolves it with findOrFail() a moment later (AccountContentDeletion:108), so a target deactivated or deleted in between throws — leaving a soft-deleted account whose files still point at it, and a UserDeleted log for a deletion that did not finish. Wrap the delete()+apply() pair in a single DB::transaction() in each of the four destroy() methods. validate() and the authorization guards stay outside it: they are read-only and must be able to reject before anything is written. cascadeDelete()/reassignTo() already open their own transaction, which nests as a savepoint under this one, so the account soft-delete, its activity log, and the content work now commit or roll back together. Tests: a DeletedAccountContent double that reports content to handle and then throws while handling it (tests/Helpers.php) drives one test per destroy() endpoint asserting the account survives the failure and no UserDeleted entry is written; each goes red against the un-wrapped controller. --- .../Controllers/Api/ClientsController.php | 16 +++++++++---- .../Http/Controllers/ClientsController.php | 16 +++++++++---- .../Http/Controllers/Api/UsersController.php | 13 +++++++--- .../Http/Controllers/UsersController.php | 13 +++++++--- tests/Feature/Api/ClientsTest.php | 16 +++++++++++++ tests/Feature/Api/UsersTest.php | 16 +++++++++++++ .../Feature/Clients/ClientsManagementTest.php | 20 ++++++++++++++++ .../Feature/Identity/UsersManagementTest.php | 19 +++++++++++++++ tests/Helpers.php | 24 +++++++++++++++++++ 9 files changed, 139 insertions(+), 14 deletions(-) diff --git a/app/Modules/Clients/Http/Controllers/Api/ClientsController.php b/app/Modules/Clients/Http/Controllers/Api/ClientsController.php index 919ebf1d..ebabca55 100644 --- a/app/Modules/Clients/Http/Controllers/Api/ClientsController.php +++ b/app/Modules/Clients/Http/Controllers/Api/ClientsController.php @@ -28,6 +28,7 @@ use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Http\Resources\Json\AnonymousResourceCollection; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Validator; use Illuminate\Validation\Rule; use Illuminate\Validation\Rules\Password; @@ -234,12 +235,19 @@ class ClientsController extends Controller $validated = $this->accountDeletion->validate($request, $client); - $name = $client->name; - $client->delete(); + // Soft-deleting the account and disposing of its files are two + // separate writes; keep them in one transaction so a failure in the + // second (e.g. the reassignment target deleted between validation + // and apply()'s findOrFail) cannot leave the account deleted with + // its content still pointing at it. + DB::transaction(function () use ($validated, $client): void { + $name = $client->name; + $client->delete(); - $this->activity->log(Action::UserDeleted, context: ['name' => $name]); + $this->activity->log(Action::UserDeleted, context: ['name' => $name]); - $this->accountDeletion->apply($validated, $client, $name); + $this->accountDeletion->apply($validated, $client, $name); + }); return response()->json(status: 204); } diff --git a/app/Modules/Clients/Http/Controllers/ClientsController.php b/app/Modules/Clients/Http/Controllers/ClientsController.php index b3d58555..444690ef 100644 --- a/app/Modules/Clients/Http/Controllers/ClientsController.php +++ b/app/Modules/Clients/Http/Controllers/ClientsController.php @@ -26,6 +26,7 @@ use App\Support\Pagination; use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; +use Illuminate\Support\Facades\DB; use Illuminate\Validation\Rule; use Illuminate\Validation\Rules\Password; use Inertia\Inertia; @@ -234,12 +235,19 @@ class ClientsController extends Controller $validated = $this->accountDeletion->validate($request, $client); - $name = $client->name; - $client->delete(); + // Soft-deleting the account and disposing of its files are two + // separate writes; keep them in one transaction so a failure in the + // second (e.g. the reassignment target deleted between validation + // and apply()'s findOrFail) cannot leave the account deleted with + // its content still pointing at it. + DB::transaction(function () use ($validated, $client): void { + $name = $client->name; + $client->delete(); - $this->activity->log(Action::UserDeleted, context: ['name' => $name]); + $this->activity->log(Action::UserDeleted, context: ['name' => $name]); - $this->accountDeletion->apply($validated, $client, $name); + $this->accountDeletion->apply($validated, $client, $name); + }); return redirect()->route('clients.index')->with('success', __('Client deleted.')); } diff --git a/app/Modules/Identity/Http/Controllers/Api/UsersController.php b/app/Modules/Identity/Http/Controllers/Api/UsersController.php index 3874459f..dae9f21d 100644 --- a/app/Modules/Identity/Http/Controllers/Api/UsersController.php +++ b/app/Modules/Identity/Http/Controllers/Api/UsersController.php @@ -17,6 +17,7 @@ use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Http\Resources\Json\AnonymousResourceCollection; +use Illuminate\Support\Facades\DB; use Illuminate\Validation\Rule; use Illuminate\Validation\Rules\Password; use Illuminate\Validation\ValidationException; @@ -215,9 +216,15 @@ class UsersController extends Controller $validated = $this->accountDeletion->validate($request, $user); - $name = $this->accounts->delete($user); - - $this->accountDeletion->apply($validated, $user, $name); + // Soft-deleting the account and disposing of its files are two + // separate writes; keep them in one transaction so a failure in the + // second (e.g. the reassignment target deleted between validation + // and apply()'s findOrFail) cannot leave the account deleted with + // its content still pointing at it. + DB::transaction(function () use ($validated, $user): void { + $name = $this->accounts->delete($user); + $this->accountDeletion->apply($validated, $user, $name); + }); return response()->json(status: 204); } diff --git a/app/Modules/Identity/Http/Controllers/UsersController.php b/app/Modules/Identity/Http/Controllers/UsersController.php index b09c438a..678d7d21 100644 --- a/app/Modules/Identity/Http/Controllers/UsersController.php +++ b/app/Modules/Identity/Http/Controllers/UsersController.php @@ -17,6 +17,7 @@ use App\Support\Pagination; use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; +use Illuminate\Support\Facades\DB; use Illuminate\Validation\Rule; use Illuminate\Validation\Rules\Password; use Illuminate\Validation\ValidationException; @@ -216,9 +217,15 @@ class UsersController extends Controller $validated = $this->accountDeletion->validate($request, $user); - $name = $this->accounts->delete($user); - - $this->accountDeletion->apply($validated, $user, $name); + // Soft-deleting the account and disposing of its files are two + // separate writes; keep them in one transaction so a failure in the + // second (e.g. the reassignment target deleted between validation + // and apply()'s findOrFail) cannot leave the account deleted with + // its content still pointing at it. + DB::transaction(function () use ($validated, $user): void { + $name = $this->accounts->delete($user); + $this->accountDeletion->apply($validated, $user, $name); + }); return redirect()->route('users.index')->with('success', __('User deleted.')); } diff --git a/tests/Feature/Api/ClientsTest.php b/tests/Feature/Api/ClientsTest.php index df634eb1..562882d1 100644 --- a/tests/Feature/Api/ClientsTest.php +++ b/tests/Feature/Api/ClientsTest.php @@ -196,6 +196,22 @@ test('reassign moves the content to the named account', function () { expect(File::query()->find($file->id)?->uploaded_by)->toBe($this->admin->id); }); +test('a failure while disposing of a deleted client\'s content rolls the deletion back', function () { + $client = User::factory()->client()->create(); + + failAccountContentDisposal(); + + $this->withToken($this->token)->deleteJson("/api/v1/clients/{$client->id}", [ + 'content_action' => 'reassign', + 'reassign_to_id' => $this->admin->id, + ])->assertStatus(500); + + // The soft-delete shares a transaction with the content step, so its + // failure leaves the client intact rather than deleted-but-orphaning. + expect(User::query()->whereKey($client->id)->exists())->toBeTrue() + ->and(ActivityLog::query()->where('action', Action::UserDeleted)->exists())->toBeFalse(); +}); + test('show reports what a delete would have to decide about', function () { $client = User::factory()->client()->create(); File::factory()->count(2)->create(['uploaded_by' => $client->id]); diff --git a/tests/Feature/Api/UsersTest.php b/tests/Feature/Api/UsersTest.php index 83dd377a..bd2edb53 100644 --- a/tests/Feature/Api/UsersTest.php +++ b/tests/Feature/Api/UsersTest.php @@ -388,6 +388,22 @@ test('deleting an account with no content needs no body, and is audited', functi expect($entry->context['name'])->toBe('Departing'); }); +test('a failure while disposing of a deleted account\'s content rolls the deletion back', function () { + $user = User::factory()->role(SystemRole::Uploader)->create(); + + failAccountContentDisposal(); + + $this->withToken($this->token)->deleteJson("/api/v1/users/{$user->id}", [ + 'content_action' => 'reassign', + 'reassign_to_id' => $this->admin->id, + ])->assertStatus(500); + + // The soft-delete shares a transaction with the content step, so its + // failure leaves the account intact rather than deleted-but-orphaning. + expect(User::query()->whereKey($user->id)->exists())->toBeTrue() + ->and(ActivityLog::query()->where('action', Action::UserDeleted)->exists())->toBeFalse(); +}); + /* |-------------------------------------------------------------------------- | Token abilities diff --git a/tests/Feature/Clients/ClientsManagementTest.php b/tests/Feature/Clients/ClientsManagementTest.php index 8dca766d..03f1c111 100644 --- a/tests/Feature/Clients/ClientsManagementTest.php +++ b/tests/Feature/Clients/ClientsManagementTest.php @@ -3,6 +3,8 @@ declare(strict_types=1); use App\Models\User; +use App\Modules\Audit\Action; +use App\Modules\Audit\ActivityLog; use App\Modules\Identity\Permissions\SystemRole; use App\Modules\Identity\UserType; use App\Modules\Platform\Capabilities\Edition; @@ -108,3 +110,21 @@ test('staff can update client settings and they take effect', function () { $this->flushSession(); $this->get('/register')->assertOk(); }); + +test('a failure while disposing of a deleted client\'s content rolls the deletion back', function () { + $client = User::factory()->client()->create(); + $reassignTarget = User::factory()->create(); + + failAccountContentDisposal(); + + $this->actingAs($this->admin)->delete("/clients/{$client->id}", [ + 'content_action' => 'reassign', + 'reassign_to_id' => $reassignTarget->id, + ])->assertStatus(500); + + // The soft-delete and its log share a transaction with the content step, + // so a failure there leaves the client intact rather than + // deleted-but-still-owning-files. + expect(User::query()->find($client->id))->not->toBeNull() + ->and(ActivityLog::query()->where('action', Action::UserDeleted)->exists())->toBeFalse(); +}); diff --git a/tests/Feature/Identity/UsersManagementTest.php b/tests/Feature/Identity/UsersManagementTest.php index d980d933..5b80870f 100644 --- a/tests/Feature/Identity/UsersManagementTest.php +++ b/tests/Feature/Identity/UsersManagementTest.php @@ -336,6 +336,25 @@ test('reassigning a deleted staff user\'s content transfers ownership and logs a ->and(ActivityLog::query()->where('action', Action::AccountContentReassigned)->exists())->toBeTrue(); }); +test('a failure while disposing of a deleted staff account\'s content rolls the deletion back', function () { + $adminUser = admin(); + $user = User::factory()->role(SystemRole::Uploader)->create(); + $reassignTarget = User::factory()->role(SystemRole::Uploader)->create(); + + failAccountContentDisposal(); + + $this->actingAs($adminUser)->delete("/users/{$user->id}", [ + 'content_action' => 'reassign', + 'reassign_to_id' => $reassignTarget->id, + ])->assertStatus(500); + + // The soft-delete and its log belong to the same transaction as the + // content step, so a failure there leaves the account intact rather than + // deleted-but-still-owning-files. + expect(User::query()->find($user->id))->not->toBeNull() + ->and(ActivityLog::query()->where('action', Action::UserDeleted)->exists())->toBeFalse(); +}); + test('users management requires granular permissions', function () { // Account Manager lacks manage_users entirely. $this->actingAs(User::factory()->role(SystemRole::AccountManager)->create()); diff --git a/tests/Helpers.php b/tests/Helpers.php index 7e8246a8..563f48b4 100644 --- a/tests/Helpers.php +++ b/tests/Helpers.php @@ -3,6 +3,7 @@ declare(strict_types=1); use App\Models\User; +use App\Modules\Files\DeletedAccountContent; use App\Modules\Files\Folders\FolderService; use App\Modules\Files\Models\File; use App\Modules\Files\Models\FileAssignment; @@ -258,3 +259,26 @@ function fakeIdToken(string $email = 'portal@example.test'): string return $encode(['alg' => 'none']).'.'.$encode(['preferred_username' => $email]).'.sig'; } + +/** + * Bind a DeletedAccountContent double that reports content to dispose of (so + * a delete demands a choice) and then throws while carrying that choice out. + * Lets the account-deletion tests prove the destroy() controllers roll their + * soft-delete back when the content step fails, rather than stranding a + * deleted account whose files still point at it. + */ +function failAccountContentDisposal(): void +{ + app()->instance(DeletedAccountContent::class, new class extends DeletedAccountContent + { + public function summarize(User $user): array + { + return ['files' => 1, 'folders' => 0]; + } + + public function reassignTo(User $from, User $to): array + { + throw new RuntimeException('content reassignment failed'); + } + }); +}