API+UI: Use observers ton prevent double logging on restores

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

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

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

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;

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;

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

tests/Feature/AssetModels/Ui/RestoreAssetModelTest.php

@@ -0,0 +1,94 @@
+<?php
+
+namespace Tests\Feature\AssetModels\Ui;
+
+use App\Models\Actionlog;
+use App\Models\AssetModel;
+use App\Models\User;
+use Tests\TestCase;
+
+/**
+ * Regression coverage for the double-logging bug on the web-side asset
+ * model restore endpoint. Two separate sources were writing action_log
+ * rows for a single restore:
+ *
+ *   - The controller wrote a manual `restore` log after $model->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()).');
+    }
+}

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

tests/Feature/Users/Ui/RestoreUserTest.php

@@ -0,0 +1,91 @@
+<?php
+
+namespace Tests\Feature\Users\Ui;
+
+use App\Models\Actionlog;
+use App\Models\User;
+use Tests\TestCase;
+
+/**
+ * Regression coverage for the double-logging bug on the web-side user
+ * restore endpoint. Pre-fix:
+ *
+ *   - The controller wrote a manual `restore` log after $user->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.');
+    }
+}