Merge pull request #1688 from denkfabrik-li/fix/atomic-account-deletion

Delete an account and dispose of its content in one transaction

Resolved the conflict with #1678 the way that PR's merge note predicted:
the erasure stamp goes inside the new transaction, so a deletion that
rolls back cannot leave a live account carrying a date on which it would
be erased.

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);
     }

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.'));
     }

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);
     }

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.'));
     }

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]);

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

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();
+});

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());

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');
+        }
+    });
+}