From 0ff2aac58b01645b32684cc3c92d7164ea6e4066 Mon Sep 17 00:00:00 2001 From: snipe Date: Mon, 17 Aug 2026 12:50:19 +0100 Subject: [PATCH] API+UI: Use observers ton prevent double logging on restores --- app/Http/Controllers/Api/UsersController.php | 15 +-- .../Controllers/AssetModelsController.php | 9 +- .../Controllers/Users/UsersController.php | 8 +- app/Observers/AssetModelObserver.php | 11 +++ app/Observers/LocationObserver.php | 5 + .../AssetModels/Api/RestoreAssetModelTest.php | 30 ++++++ .../AssetModels/Ui/RestoreAssetModelTest.php | 94 +++++++++++++++++++ tests/Feature/Users/Api/RestoreUserTest.php | 53 +++++++++++ tests/Feature/Users/Ui/RestoreUserTest.php | 91 ++++++++++++++++++ 9 files changed, 292 insertions(+), 24 deletions(-) create mode 100644 tests/Feature/AssetModels/Ui/RestoreAssetModelTest.php create mode 100644 tests/Feature/Users/Ui/RestoreUserTest.php diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 456e77606a..b07685b03a 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -17,7 +17,6 @@ use App\Http\Transformers\LicensesTransformer; use App\Http\Transformers\SelectlistTransformer; use App\Http\Transformers\UsersTransformer; use App\Models\Accessory; -use App\Models\Actionlog; use App\Models\Asset; use App\Models\Company; use App\Models\Consumable; @@ -487,7 +486,7 @@ class UsersController extends Controller // saved the user first, then filtered company IDs after the fact // (see syncCompaniesWithLogging below). A non-superuser could // submit a company_id belonging to another company they were not - // a member of; the user row was persisted before the filter ran, + // a member of, and the user row was persisted before the filter ran, // producing an unauthorized cross-tenant record even when the // pivot ended up empty (leaving the account as a floater under // null_company_is_floater installs). Reject the whole request if @@ -666,7 +665,7 @@ class UsersController extends Controller } // Pull out sensitive fields that require extra permission. The - // GATED_AUTH_FIELDS constant covers user-editable secrets; the + // GATED_AUTH_FIELDS constant covers user-editable secrets. The // additional keys below are internal state (2FA secrets, remember // tokens, activation codes) that must never be settable from a // request payload regardless of caller privilege. @@ -1015,14 +1014,8 @@ class UsersController extends Controller } if ($user->restore()) { - - $logaction = new Actionlog; - $logaction->item_type = User::class; - $logaction->item_id = $user->id; - $logaction->created_at = date('Y-m-d H:i:s'); - $logaction->created_by = auth()->id(); - $logaction->logaction('restore'); - + // The `restore` action_log entry is written by + // UserObserver::restoring, no manual write here. return response()->json(Helper::formatStandardApiResponse('success', null, trans('admin/users/message.success.restored')), 200); } diff --git a/app/Http/Controllers/AssetModelsController.php b/app/Http/Controllers/AssetModelsController.php index 3b2763104f..cebea52afe 100755 --- a/app/Http/Controllers/AssetModelsController.php +++ b/app/Http/Controllers/AssetModelsController.php @@ -5,7 +5,6 @@ namespace App\Http\Controllers; use App\Helpers\Helper; use App\Http\Requests\ImageUploadRequest; use App\Http\Requests\StoreAssetModelRequest; -use App\Models\Actionlog; use App\Models\AssetModel; use App\Models\CustomField; use App\Models\SnipeModel; @@ -236,12 +235,8 @@ class AssetModelsController extends Controller } if ($model->restore()) { - $logaction = new Actionlog; - $logaction->item_type = AssetModel::class; - $logaction->item_id = $model->id; - $logaction->created_at = date('Y-m-d H:i:s'); - $logaction->created_by = auth()->id(); - $logaction->logaction('restore'); + // The `restore` action_log entry is written by + // AssetModelObserver::restoring, no manual write here. // Redirect them to the deleted page if there are more, otherwise the section index $deleted_models = AssetModel::onlyTrashed()->count(); diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 6056f08b62..3611cae3c0 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -415,12 +415,8 @@ class UsersController extends Controller } if ($user->restore()) { - $logaction = new Actionlog; - $logaction->item_type = User::class; - $logaction->item_id = $user->id; - $logaction->created_at = date('Y-m-d H:i:s'); - $logaction->created_by = auth()->id(); - $logaction->logaction('restore'); + // The `restore` action_log entry is written by + // UserObserver::restoring - no manual write here. // Redirect them to the deleted page if there are more, otherwise the section index $deleted_users = User::onlyTrashed()->count(); diff --git a/app/Observers/AssetModelObserver.php b/app/Observers/AssetModelObserver.php index 063f71480c..3962020037 100644 --- a/app/Observers/AssetModelObserver.php +++ b/app/Observers/AssetModelObserver.php @@ -25,6 +25,17 @@ class AssetModelObserver } } + // Restoring a soft-deleted row fires `updating` (Laravel's + // restore() ultimately calls save()), which would otherwise + // log an "update" entry showing deleted_at flipping to null. + // The `restoring` observer already logs a dedicated "restore" + // entry for the same event, so filter deleted_at + updated_at + // out of the diff and skip the update log if nothing else + // changed. Same rationale on the soft-delete side, but Laravel's + // SoftDeletes trait uses a raw UPDATE that skips model events + // entirely, so this branch never fires for delete. + unset($changed['deleted_at'], $changed['updated_at']); + if (count($changed) > 0) { $logAction = new Actionlog; $logAction->item_type = AssetModel::class; diff --git a/app/Observers/LocationObserver.php b/app/Observers/LocationObserver.php index 384d71a117..feacacfcf3 100644 --- a/app/Observers/LocationObserver.php +++ b/app/Observers/LocationObserver.php @@ -25,6 +25,11 @@ class LocationObserver } } + // See AssetModelObserver::updating for the deleted_at / updated_at + // filter rationale (restore fires updating via save() and would + // otherwise log a noisy "update" entry alongside the "restore" one). + unset($changed['deleted_at'], $changed['updated_at']); + if (count($changed) > 0) { $logAction = new Actionlog; $logAction->item_type = Location::class; diff --git a/tests/Feature/AssetModels/Api/RestoreAssetModelTest.php b/tests/Feature/AssetModels/Api/RestoreAssetModelTest.php index bcfa9c37d5..b2a2c143be 100644 --- a/tests/Feature/AssetModels/Api/RestoreAssetModelTest.php +++ b/tests/Feature/AssetModels/Api/RestoreAssetModelTest.php @@ -73,4 +73,34 @@ class RestoreAssetModelTest extends TestCase $this->assertSame($before + 1, $after, 'Expected one restore action_log entry to be written.'); } + + public function test_restore_does_not_write_an_update_action_log(): void + { + // AssetModelObserver::updating fires during restore because + // Laravel's restore() calls save(). Pre-fix that observer + // logged an "update" entry showing `deleted_at` flipping to + // null. AssetModelObserver now filters deleted_at + updated_at + // out of the diff so no bogus update entry gets written on + // restore. See the matching test in the web-side + // RestoreAssetModelTest. + $model = AssetModel::factory()->create(); + $model->delete(); + + $before = Actionlog::where('item_type', AssetModel::class) + ->where('item_id', $model->id) + ->where('action_type', 'update') + ->count(); + + $this->actingAsForApi(User::factory()->superuser()->create()) + ->postJson(route('api.models.restore', $model->id)) + ->assertOk() + ->assertStatusMessageIs('success'); + + $after = Actionlog::where('item_type', AssetModel::class) + ->where('item_id', $model->id) + ->where('action_type', 'update') + ->count(); + + $this->assertSame($before, $after, 'Expected no update action_log entry on restore.'); + } } diff --git a/tests/Feature/AssetModels/Ui/RestoreAssetModelTest.php b/tests/Feature/AssetModels/Ui/RestoreAssetModelTest.php new file mode 100644 index 0000000000..b35e7793ca --- /dev/null +++ b/tests/Feature/AssetModels/Ui/RestoreAssetModelTest.php @@ -0,0 +1,94 @@ +restore(). + * - AssetModelObserver's `restoring` hook also wrote a `restore` log. + * - AssetModelObserver's `updating` hook fired via save() and wrote an + * `update` log showing `deleted_at` flipping from a timestamp to null. + * + * Three log rows per restore instead of one. The manual controller write + * was removed, and the updating observer now filters `deleted_at` + + * `updated_at` out of its diff so it no longer fires on restore. Tests + * pin both behaviors down. + */ +class RestoreAssetModelTest extends TestCase +{ + public function test_permission_required_to_restore_asset_model(): void + { + $model = AssetModel::factory()->create(); + $model->delete(); + + $this->actingAs(User::factory()->create()) + ->post(route('models.restore.store', $model->id)) + ->assertForbidden(); + } + + public function test_soft_deleted_asset_model_can_be_restored_via_web(): void + { + $model = AssetModel::factory()->create(); + $model->delete(); + $this->assertNotNull($model->fresh()->deleted_at); + + $this->actingAs(User::factory()->superuser()->create()) + ->post(route('models.restore.store', $model->id)) + ->assertRedirect() + ->assertSessionHas('success'); + + $this->assertNull($model->fresh()->deleted_at); + } + + public function test_restore_writes_exactly_one_restore_action_log(): void + { + $model = AssetModel::factory()->create(); + $model->delete(); + + $before = Actionlog::where('item_type', AssetModel::class) + ->where('item_id', $model->id) + ->where('action_type', 'restore') + ->count(); + + $this->actingAs(User::factory()->superuser()->create()) + ->post(route('models.restore.store', $model->id)) + ->assertRedirect(); + + $after = Actionlog::where('item_type', AssetModel::class) + ->where('item_id', $model->id) + ->where('action_type', 'restore') + ->count(); + + $this->assertSame($before + 1, $after, 'Expected exactly one restore action_log entry (regression: was 2 pre-fix, one from the controller and one from the observer).'); + } + + public function test_restore_does_not_write_an_update_action_log(): void + { + $model = AssetModel::factory()->create(); + $model->delete(); + + $before = Actionlog::where('item_type', AssetModel::class) + ->where('item_id', $model->id) + ->where('action_type', 'update') + ->count(); + + $this->actingAs(User::factory()->superuser()->create()) + ->post(route('models.restore.store', $model->id)) + ->assertRedirect(); + + $after = Actionlog::where('item_type', AssetModel::class) + ->where('item_id', $model->id) + ->where('action_type', 'update') + ->count(); + + $this->assertSame($before, $after, 'Expected no update action_log entry on restore (regression: was 1 pre-fix, from the updating observer catching deleted_at flipping to null via save()).'); + } +} diff --git a/tests/Feature/Users/Api/RestoreUserTest.php b/tests/Feature/Users/Api/RestoreUserTest.php index 3471ded0cf..d7d074453e 100644 --- a/tests/Feature/Users/Api/RestoreUserTest.php +++ b/tests/Feature/Users/Api/RestoreUserTest.php @@ -2,6 +2,7 @@ namespace Tests\Feature\Users\Api; +use App\Models\Actionlog; use App\Models\Company; use App\Models\User; use Tests\TestCase; @@ -52,6 +53,58 @@ class RestoreUserTest extends TestCase $this->assertNull($deleted_user->deleted_at); } + public function test_restore_writes_exactly_one_restore_action_log(): void + { + // Regression for the double-logging bug: pre-fix the controller + // wrote a manual `restore` log AND UserObserver::restoring wrote + // one, so a single restore left two rows. Manual write removed + // to leave the observer as the sole author. + $deleted_user = User::factory()->deletedUser()->create(); + + $before = Actionlog::where('item_type', User::class) + ->where('item_id', $deleted_user->id) + ->where('action_type', 'restore') + ->count(); + + $this->actingAsForApi(User::factory()->admin()->create()) + ->postJson(route('api.users.restore', ['user' => $deleted_user])) + ->assertOk() + ->assertStatusMessageIs('success'); + + $after = Actionlog::where('item_type', User::class) + ->where('item_id', $deleted_user->id) + ->where('action_type', 'restore') + ->count(); + + $this->assertSame($before + 1, $after, 'Expected exactly one restore action_log entry (regression: was 2 pre-fix).'); + } + + public function test_restore_does_not_write_an_update_action_log(): void + { + // UserObserver::updating uses an allowlist that excludes + // deleted_at, so this test would have passed pre-fix as well. + // Included as belt-and-suspenders against a future observer + // change that switches to an unfiltered raw-original diff. + $deleted_user = User::factory()->deletedUser()->create(); + + $before = Actionlog::where('item_type', User::class) + ->where('item_id', $deleted_user->id) + ->where('action_type', 'update') + ->count(); + + $this->actingAsForApi(User::factory()->admin()->create()) + ->postJson(route('api.users.restore', ['user' => $deleted_user])) + ->assertOk() + ->assertStatusMessageIs('success'); + + $after = Actionlog::where('item_type', User::class) + ->where('item_id', $deleted_user->id) + ->where('action_type', 'update') + ->count(); + + $this->assertSame($before, $after, 'Expected no update action_log entry on restore.'); + } + public function test_permissions_for_restoring_if_not_in_same_company_and_not_superadmin() { $this->settings->enableMultipleFullCompanySupport(); diff --git a/tests/Feature/Users/Ui/RestoreUserTest.php b/tests/Feature/Users/Ui/RestoreUserTest.php new file mode 100644 index 0000000000..c7cad88579 --- /dev/null +++ b/tests/Feature/Users/Ui/RestoreUserTest.php @@ -0,0 +1,91 @@ +restore(). + * - UserObserver's `restoring` hook also wrote a `restore` log. + * + * Two log rows per restore instead of one. UserObserver's `updating` + * hook did NOT contribute an extra "update" log on restore because it + * uses an allowlist of trackable fields that never included + * `deleted_at`. The manual controller write was removed to leave the + * observer as the single log author. + */ +class RestoreUserTest extends TestCase +{ + public function test_permission_required_to_restore_user(): void + { + $user = User::factory()->deletedUser()->create(); + + $this->actingAs(User::factory()->create()) + ->post(route('users.restore.store', $user->id)) + ->assertForbidden(); + } + + public function test_soft_deleted_user_can_be_restored_via_web(): void + { + $user = User::factory()->deletedUser()->create(); + $this->assertNotNull($user->deleted_at); + + $this->actingAs(User::factory()->admin()->create()) + ->post(route('users.restore.store', $user->id)) + ->assertRedirect() + ->assertSessionHas('success'); + + $this->assertNull($user->fresh()->deleted_at); + } + + public function test_restore_writes_exactly_one_restore_action_log(): void + { + $user = User::factory()->deletedUser()->create(); + + $before = Actionlog::where('item_type', User::class) + ->where('item_id', $user->id) + ->where('action_type', 'restore') + ->count(); + + $this->actingAs(User::factory()->admin()->create()) + ->post(route('users.restore.store', $user->id)) + ->assertRedirect(); + + $after = Actionlog::where('item_type', User::class) + ->where('item_id', $user->id) + ->where('action_type', 'restore') + ->count(); + + $this->assertSame($before + 1, $after, 'Expected exactly one restore action_log entry (regression: was 2 pre-fix, one from the controller and one from the observer).'); + } + + public function test_restore_does_not_write_an_update_action_log(): void + { + // UserObserver::updating uses an allowlist that excludes + // deleted_at, so this test would have passed pre-fix as well. + // Included as belt-and-suspenders against a future observer + // change that switches to an unfiltered raw-original diff. + $user = User::factory()->deletedUser()->create(); + + $before = Actionlog::where('item_type', User::class) + ->where('item_id', $user->id) + ->where('action_type', 'update') + ->count(); + + $this->actingAs(User::factory()->admin()->create()) + ->post(route('users.restore.store', $user->id)) + ->assertRedirect(); + + $after = Actionlog::where('item_type', User::class) + ->where('item_id', $user->id) + ->where('action_type', 'update') + ->count(); + + $this->assertSame($before, $after, 'Expected no update action_log entry on restore.'); + } +}