From f099a2e4aaad0823f1ccdad71f6b47b0be00e8f0 Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 21 Jul 2026 16:44:49 +0100 Subject: [PATCH] Do not remove files on soft-delete --- .../Accessories/AccessoriesController.php | 14 +- .../Controllers/Api/AssetModelsController.php | 16 +- .../Controllers/Assets/AssetsController.php | 13 +- .../Controllers/BulkAccessoriesController.php | 15 +- app/Http/Controllers/CompaniesController.php | 15 +- .../Components/ComponentsController.php | 18 +- .../Controllers/DepartmentsController.php | 13 +- app/Http/Controllers/LocationsController.php | 12 +- tests/Feature/Console/Commands/PurgeTest.php | 231 +++++++++++++++++- 9 files changed, 268 insertions(+), 79 deletions(-) diff --git a/app/Http/Controllers/Accessories/AccessoriesController.php b/app/Http/Controllers/Accessories/AccessoriesController.php index ec7149d9fd..004ebbf053 100755 --- a/app/Http/Controllers/Accessories/AccessoriesController.php +++ b/app/Http/Controllers/Accessories/AccessoriesController.php @@ -9,7 +9,6 @@ use App\Models\Accessory; use App\Models\Company; use Illuminate\Contracts\View\View; use Illuminate\Http\RedirectResponse; -use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Storage; use Illuminate\Support\Facades\Validator; @@ -218,14 +217,11 @@ class AccessoriesController extends Controller $accessory->loadCount('checkouts as checkouts_count'); if ($accessory->isDeletable()) { - if ($accessory->image) { - try { - Storage::disk('public')->delete('accessories'.'/'.$accessory->image); - } catch (\Exception $e) { - Log::debug($e); - } - } - + // Note: the image file is deliberately preserved across this + // soft-delete. Snipe-IT's `snipeit:purge` command permanently + // removes it later when the row is force-deleted. Keeping + // the file here means a restored soft-deleted row still has + // its image. $accessory->delete(); return redirect()->route('accessories.index')->with('success', trans('admin/accessories/message.delete.success')); diff --git a/app/Http/Controllers/Api/AssetModelsController.php b/app/Http/Controllers/Api/AssetModelsController.php index d5a36b6548..4acc3b67cf 100644 --- a/app/Http/Controllers/Api/AssetModelsController.php +++ b/app/Http/Controllers/Api/AssetModelsController.php @@ -16,7 +16,6 @@ use App\Models\Setting; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Http\Response; -use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Storage; /** @@ -278,14 +277,13 @@ class AssetModelsController extends Controller return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/models/message.assoc_users'))); } - if ($assetmodel->image) { - try { - Storage::disk('public')->delete('assetmodels/'.$assetmodel->image); - } catch (\Exception $e) { - Log::info($e); - } - } - + // Note: the image file is deliberately preserved across this + // soft-delete. Snipe-IT's `snipeit:purge` command permanently + // removes it later when the row is force-deleted. Keeping the + // file here means a restored soft-deleted row still has its + // image. Also fixes a latent path bug: the old delete used + // `assetmodels/` but handleImages stores under `models/`, so + // the unlink here was silently missing the file anyway. $assetmodel->delete(); return response()->json(Helper::formatStandardApiResponse('success', null, trans('admin/models/message.delete.success'))); diff --git a/app/Http/Controllers/Assets/AssetsController.php b/app/Http/Controllers/Assets/AssetsController.php index de63a2cb16..26451c78cc 100755 --- a/app/Http/Controllers/Assets/AssetsController.php +++ b/app/Http/Controllers/Assets/AssetsController.php @@ -564,14 +564,11 @@ class AssetsController extends Controller ->update(['assigned_to' => null, 'assigned_type' => null]); } - if ($asset->image) { - try { - Storage::disk('public')->delete('assets/'.basename($asset->image)); - } catch (\Exception $e) { - Log::debug($e); - } - } - + // Note: the image file is deliberately preserved across this + // soft-delete. Snipe-IT's `snipeit:purge` command permanently + // removes it later when the row is force-deleted. Keeping the + // file here means a restored soft-deleted row still has its + // image. $asset->delete(); return redirect()->route('hardware.index')->with('success', trans('admin/hardware/message.delete.success')); diff --git a/app/Http/Controllers/BulkAccessoriesController.php b/app/Http/Controllers/BulkAccessoriesController.php index 8f153106fb..7f558c8e15 100644 --- a/app/Http/Controllers/BulkAccessoriesController.php +++ b/app/Http/Controllers/BulkAccessoriesController.php @@ -5,8 +5,6 @@ namespace App\Http\Controllers; use App\Models\Accessory; use Illuminate\Http\Request; use Illuminate\Support\Facades\Gate; -use Illuminate\Support\Facades\Log; -use Illuminate\Support\Facades\Storage; class BulkAccessoriesController extends Controller { @@ -49,14 +47,11 @@ class BulkAccessoriesController extends Controller continue; } - if ($accessory->image) { - try { - Storage::disk('public')->delete('accessories/'.$accessory->image); - } catch (\Exception $e) { - Log::debug($e); - } - } - + // Note: the image file is deliberately preserved across this + // soft-delete. Snipe-IT's `snipeit:purge` command permanently + // removes it later when the row is force-deleted. Keeping + // the file here means a restored soft-deleted row still has + // its image. $accessory->delete(); $success_count++; } diff --git a/app/Http/Controllers/CompaniesController.php b/app/Http/Controllers/CompaniesController.php index ac3a778091..8e3521e4b0 100644 --- a/app/Http/Controllers/CompaniesController.php +++ b/app/Http/Controllers/CompaniesController.php @@ -13,8 +13,6 @@ use App\Models\User; use Illuminate\Contracts\View\View; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; -use Illuminate\Support\Facades\Log; -use Illuminate\Support\Facades\Storage; /** * This controller handles all actions related to Companies for @@ -155,14 +153,11 @@ final class CompaniesController extends Controller ->with('error', trans('admin/companies/message.assoc_users')); } - if ($company->image) { - try { - Storage::disk('public')->delete('companies'.'/'.$company->image); - } catch (\Exception $e) { - Log::debug($e); - } - } - + // Note: the image file is deliberately preserved across this + // soft-delete. Snipe-IT's `snipeit:purge` command permanently + // removes it later when the row is force-deleted. Keeping the + // file here means a restored soft-deleted row still has its + // image. $company->delete(); return redirect()->route('companies.index') diff --git a/app/Http/Controllers/Components/ComponentsController.php b/app/Http/Controllers/Components/ComponentsController.php index 90041c1857..f88a9d58db 100644 --- a/app/Http/Controllers/Components/ComponentsController.php +++ b/app/Http/Controllers/Components/ComponentsController.php @@ -11,8 +11,6 @@ use App\Models\Component; use Illuminate\Auth\Access\AuthorizationException; use Illuminate\Contracts\View\View; use Illuminate\Http\RedirectResponse; -use Illuminate\Support\Facades\Log; -use Illuminate\Support\Facades\Storage; /** * This class controls all actions related to Components for @@ -151,7 +149,7 @@ class ComponentsController extends Controller public function update(UpdateComponentRequest $request, Component $component) { $this->authorize('update', $component); - + // Update the component data $component->name = $request->input('name'); $component->category_id = $request->input('category_id'); @@ -200,15 +198,11 @@ class ComponentsController extends Controller $this->authorize('delete', $component); - // Remove the image if one exists - if ($component->image && Storage::disk('public')->exists('components/'.$component->image)) { - try { - Storage::disk('public')->delete('components/'.$component->image); - } catch (\Exception $e) { - Log::debug($e); - } - } - + // Note: the image file is deliberately preserved across this + // soft-delete. Snipe-IT's `snipeit:purge` command permanently + // removes it later when the row is force-deleted. Keeping the + // file here means a restored soft-deleted row still has its + // image. if ($component->numCheckedOut() > 0) { return redirect()->route('components.index')->with('error', trans('admin/components/message.delete.error_qty')); } diff --git a/app/Http/Controllers/DepartmentsController.php b/app/Http/Controllers/DepartmentsController.php index ad0322df07..567237abcf 100644 --- a/app/Http/Controllers/DepartmentsController.php +++ b/app/Http/Controllers/DepartmentsController.php @@ -8,7 +8,6 @@ use App\Models\Department; use Illuminate\Contracts\View\View; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; -use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Storage; class DepartmentsController extends Controller @@ -115,13 +114,11 @@ class DepartmentsController extends Controller return redirect()->to(route('departments.index'))->with('error', trans('admin/departments/message.assoc_users')); } - if ($department->image) { - try { - Storage::disk('public')->delete('departments'.'/'.$department->image); - } catch (\Exception $e) { - Log::debug($e); - } - } + // Note: the image file is deliberately preserved across this + // soft-delete. Snipe-IT's `snipeit:purge` command permanently + // removes it later when the row is force-deleted. Keeping the + // file here means a restored soft-deleted row still has its + // image. $department->delete(); return redirect()->back()->with('success', trans('admin/departments/message.delete.success')); diff --git a/app/Http/Controllers/LocationsController.php b/app/Http/Controllers/LocationsController.php index 1df7a07e2f..b03816fe03 100755 --- a/app/Http/Controllers/LocationsController.php +++ b/app/Http/Controllers/LocationsController.php @@ -243,13 +243,11 @@ class LocationsController extends Controller if ($location->isDeletable()) { - if ($location->image) { - try { - Storage::disk('public')->delete('locations/'.$location->image); - } catch (\Exception $e) { - Log::error($e); - } - } + // Note: the image file is deliberately preserved across this + // soft-delete. Snipe-IT's `snipeit:purge` command permanently + // removes it later when the row is force-deleted. Keeping + // the file here means a restored soft-deleted row still has + // its image. $location->delete(); return redirect()->to(route('locations.index'))->with('success', trans('admin/locations/message.delete.success')); diff --git a/tests/Feature/Console/Commands/PurgeTest.php b/tests/Feature/Console/Commands/PurgeTest.php index e25e9eec8a..835808a743 100644 --- a/tests/Feature/Console/Commands/PurgeTest.php +++ b/tests/Feature/Console/Commands/PurgeTest.php @@ -5,11 +5,13 @@ namespace Tests\Feature\Console\Commands; use App\Models\Accessory; use App\Models\Actionlog; use App\Models\Asset; +use App\Models\CheckoutAcceptance; use App\Models\License; use App\Models\Location; use App\Models\Maintenance; use App\Models\User; use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Storage; use Tests\TestCase; /** @@ -122,16 +124,233 @@ class PurgeTest extends TestCase public function test_soft_deleted_user_with_show_in_list_zero_is_preserved(): void { - // System users (LDAP-sync placeholders, etc.) set show_in_list=0 - // and are excluded from purge even when soft-deleted. This filter - // was in the pre-refactor code and must be preserved. - $systemUser = User::factory()->create(['show_in_list' => 0]); - $systemUser->delete(); + // show_in_list=0 excludes a user from checkout-target dropdowns + // in the UI. Purge preserves these users so they stick around + // even when soft-deleted (matches the pre-refactor behavior). + $nonCheckoutUser = User::factory()->create(['show_in_list' => 0]); + $nonCheckoutUser->delete(); $this->artisan('snipeit:purge', ['--force' => 'true'])->assertExitCode(0); // Row is gone-from-index (soft-deleted) but still in the table. - $this->assertDatabaseHas('users', ['id' => $systemUser->id]); + $this->assertDatabaseHas('users', ['id' => $nonCheckoutUser->id]); + } + + public function test_purge_removes_uploaded_files_for_soft_deleted_users(): void + { + // Regression guard: an intermediate refactor of Purge dropped the + // Storage::delete() step that removes uploaded avatars/documents + // under private_uploads/users/ when a user is purged. Without + // this test, that call could silently disappear again and leave + // orphan files on disk. The old inline code lived in a per-user + // loop; the current implementation batches via a single + // action_logs query, and either shape needs to actually unlink + // the file for the corresponding trashed user. + Storage::fake(); + $user = User::factory()->create(); + $filename = "u{$user->id}-avatar.png"; + Storage::put("private_uploads/users/{$filename}", 'fake image bytes'); + + Actionlog::factory()->create([ + 'item_type' => User::class, + 'item_id' => $user->id, + 'action_type' => 'uploaded', + 'filename' => $filename, + ]); + + $user->delete(); + Storage::assertExists("private_uploads/users/{$filename}"); + + $this->artisan('snipeit:purge', ['--force' => 'true'])->assertExitCode(0); + + Storage::assertMissing("private_uploads/users/{$filename}"); + $this->assertDatabaseMissing('users', ['id' => $user->id]); + } + + public function test_dry_run_does_not_delete_user_files(): void + { + // Companion guard: --dry-run must be side-effect-free on disk. + Storage::fake(); + $user = User::factory()->create(); + $filename = "u{$user->id}-avatar.png"; + Storage::put("private_uploads/users/{$filename}", 'fake image bytes'); + + Actionlog::factory()->create([ + 'item_type' => User::class, + 'item_id' => $user->id, + 'action_type' => 'uploaded', + 'filename' => $filename, + ]); + + $user->delete(); + + $this->artisan('snipeit:purge', ['--force' => 'true', '--dry-run' => true])->assertExitCode(0); + + Storage::assertExists("private_uploads/users/{$filename}"); + } + + public function test_purge_removes_image_files_for_soft_deleted_assets(): void + { + // Image column on the parent row itself. Snipe-IT stores these + // on the public disk under `{plural-type}/{filename}`. Removing + // them at purge time (rather than at soft-delete) means a + // restored soft-deleted asset still has its image intact. + Storage::fake('public'); + $asset = Asset::factory()->create(['image' => 'asset-42.jpg']); + Storage::disk('public')->put('assets/asset-42.jpg', 'fake image bytes'); + + $asset->delete(); + + $this->artisan('snipeit:purge', ['--force' => 'true'])->assertExitCode(0); + + Storage::disk('public')->assertMissing('assets/asset-42.jpg'); + $this->assertDatabaseMissing('assets', ['id' => $asset->id]); + } + + public function test_purge_removes_avatar_files_for_soft_deleted_users(): void + { + // Users' avatar column has its own public-disk subpath (`avatars`) + // distinct from every other model's `image` column. Covered + // separately because `UsersController::destroy` used to NOT + // delete the avatar and now (correctly) still doesn't; purge + // is the sole avatar-unlink path. + Storage::fake('public'); + $user = User::factory()->create(['avatar' => 'user-7.jpg']); + Storage::disk('public')->put('avatars/user-7.jpg', 'fake avatar bytes'); + + $user->delete(); + + $this->artisan('snipeit:purge', ['--force' => 'true'])->assertExitCode(0); + + Storage::disk('public')->assertMissing('avatars/user-7.jpg'); + $this->assertDatabaseMissing('users', ['id' => $user->id]); + } + + public function test_purge_removes_eula_pdfs_when_action_log_parent_is_purged(): void + { + // Signed-EULA PDFs live under `private_uploads/eula-pdfs/`. + // They're identified by an action_log with action_type of + // `accepted` or `declined`, not by item_type, so the routing + // logic in Purge has to key off action_type first. + Storage::fake(); + $asset = Asset::factory()->create(); + $eula = "eula-{$asset->id}.pdf"; + Storage::put("private_uploads/eula-pdfs/{$eula}", 'fake pdf bytes'); + + Actionlog::factory()->create([ + 'item_type' => Asset::class, + 'item_id' => $asset->id, + 'action_type' => 'accepted', + 'filename' => $eula, + ]); + + $asset->delete(); + + $this->artisan('snipeit:purge', ['--force' => 'true'])->assertExitCode(0); + + Storage::assertMissing("private_uploads/eula-pdfs/{$eula}"); + } + + public function test_purge_removes_signature_files_from_action_logs(): void + { + // Signatures live under `private_uploads/signatures/` and are + // referenced by the `accept_signature` column on action_logs + // (not by the `filename` column and not by any specific + // action_type). Purge must read that column separately. + Storage::fake(); + $asset = Asset::factory()->create(); + $sig = "sig-{$asset->id}.png"; + Storage::put("private_uploads/signatures/{$sig}", 'fake signature bytes'); + + Actionlog::factory()->create([ + 'item_type' => Asset::class, + 'item_id' => $asset->id, + 'action_type' => 'checkout', + 'accept_signature' => $sig, + ]); + + $asset->delete(); + + $this->artisan('snipeit:purge', ['--force' => 'true'])->assertExitCode(0); + + Storage::assertMissing("private_uploads/signatures/{$sig}"); + } + + public function test_purge_removes_audit_files_from_action_logs(): void + { + // Audit files (photos, notes attached during an audit) live + // under `private_uploads/audits/` and are keyed on + // `action_type = 'audit'` in the action_log. + Storage::fake(); + $asset = Asset::factory()->create(); + $auditFile = "audit-{$asset->id}.jpg"; + Storage::put("private_uploads/audits/{$auditFile}", 'fake audit photo'); + + Actionlog::factory()->create([ + 'item_type' => Asset::class, + 'item_id' => $asset->id, + 'action_type' => 'audit', + 'filename' => $auditFile, + ]); + + $asset->delete(); + + $this->artisan('snipeit:purge', ['--force' => 'true'])->assertExitCode(0); + + Storage::assertMissing("private_uploads/audits/{$auditFile}"); + } + + public function test_purge_matches_action_log_files_via_target_columns_too(): void + { + // Signatures/EULAs for checkouts are recorded with the + // checkoutable item under `item_*` and the recipient user under + // `target_*`. Purging the recipient user must clean up their + // signature file, even though the action_log's `item_type` + // points at Asset (not User). + Storage::fake(); + $user = User::factory()->create(); + $sig = "user-{$user->id}-sig.png"; + Storage::put("private_uploads/signatures/{$sig}", 'fake signature bytes'); + + Actionlog::factory()->create([ + 'item_type' => Asset::class, + 'item_id' => Asset::factory()->create()->id, + 'target_type' => User::class, + 'target_id' => $user->id, + 'action_type' => 'checkout', + 'accept_signature' => $sig, + ]); + + $user->delete(); + + $this->artisan('snipeit:purge', ['--force' => 'true'])->assertExitCode(0); + + Storage::assertMissing("private_uploads/signatures/{$sig}"); + } + + public function test_purge_removes_checkout_acceptance_signature_and_eula_files(): void + { + // CheckoutAcceptance stores its signature filename and the + // rendered EULA PDF inline on the row (not via a related + // action_log). Both need to be unlinked when the acceptance is + // itself purged. + Storage::fake(); + $acceptance = CheckoutAcceptance::factory() + ->withoutActionLog() + ->accepted() + ->create([ + 'signature_filename' => 'acceptance-sig.png', + 'stored_eula_file' => 'acceptance-eula.pdf', + ]); + Storage::put('private_uploads/signatures/acceptance-sig.png', 'sig bytes'); + Storage::put('private_uploads/eula-pdfs/acceptance-eula.pdf', 'pdf bytes'); + + $acceptance->delete(); + + $this->artisan('snipeit:purge', ['--force' => 'true'])->assertExitCode(0); + + Storage::assertMissing('private_uploads/signatures/acceptance-sig.png'); + Storage::assertMissing('private_uploads/eula-pdfs/acceptance-eula.pdf'); } public function test_soft_deleted_location_is_purged(): void