diff --git a/app/Modules/Clients/Http/Controllers/Api/ClientsController.php b/app/Modules/Clients/Http/Controllers/Api/ClientsController.php index db319ca4..08389193 100644 --- a/app/Modules/Clients/Http/Controllers/Api/ClientsController.php +++ b/app/Modules/Clients/Http/Controllers/Api/ClientsController.php @@ -31,6 +31,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; @@ -263,13 +264,24 @@ class ClientsController extends Controller $validated = $this->accountDeletion->validate($request, $client); - $name = $client->name; - $this->erasure->apply($client); - $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. + // + // The erasure stamp goes inside for the same reason: a deletion + // that rolls back must not leave a live account carrying a date + // on which it would be erased. + DB::transaction(function () use ($validated, $client): void { + $name = $client->name; + $this->erasure->apply($client); + $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 312ac6c2..3245a8a0 100644 --- a/app/Modules/Clients/Http/Controllers/ClientsController.php +++ b/app/Modules/Clients/Http/Controllers/ClientsController.php @@ -29,6 +29,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; @@ -278,13 +279,24 @@ class ClientsController extends Controller $validated = $this->accountDeletion->validate($request, $client); - $name = $client->name; - $this->erasure->apply($client); - $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. + // + // The erasure stamp goes inside for the same reason: a deletion + // that rolls back must not leave a live account carrying a date + // on which it would be erased. + DB::transaction(function () use ($validated, $client): void { + $name = $client->name; + $this->erasure->apply($client); + $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 00293a78..ba05c379 100644 --- a/app/Modules/Identity/Http/Controllers/Api/UsersController.php +++ b/app/Modules/Identity/Http/Controllers/Api/UsersController.php @@ -18,6 +18,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; @@ -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 response()->json(status: 204); } diff --git a/app/Modules/Identity/Http/Controllers/UsersController.php b/app/Modules/Identity/Http/Controllers/UsersController.php index 76b51fec..0ab113c2 100644 --- a/app/Modules/Identity/Http/Controllers/UsersController.php +++ b/app/Modules/Identity/Http/Controllers/UsersController.php @@ -18,6 +18,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; @@ -222,9 +223,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'); + } + }); +}