Fixed FD-56819 - concurrency issues

app/Http/Controllers/Api/AssetsController.php

@@ -1081,6 +1081,19 @@ class AssetsController extends Controller
             }
 
             if ($requestedCheckout) {
+                // Concurrency guard, same shape as Api\AssetsController::checkout.
+                // availableForCheckout() at line 1067 ran outside the transaction;
+                // without a row lock, two racing PATCH requests that both include
+                // assigned_user / assigned_asset / assigned_location could each
+                // pass that check and both proceed through checkOut(), producing
+                // duplicate checkout-history rows and a doubled checkout_counter.
+                // Re-fetch the row under lockForUpdate and re-check availability
+                // against the locked snapshot before invoking checkOut.
+                $locked = Asset::whereKey($asset->id)->lockForUpdate()->first();
+                if (! $locked || ! $locked->availableForCheckout()) {
+                    return false;
+                }
+
                 // Preserve the asset name if the name wasn't in the payload.
                 $asset_name = $request->has('name') ? $request->input('name') : $asset->name;
 
@@ -1342,8 +1355,23 @@ class AssetsController extends Controller
         //            $asset->location_id = $target->rtd_location_id;
         //        }
 
-        // Keep checkout mutation + checkout logging/event side effects atomic.
+        // Concurrency guard. availableForCheckout() above ran on an
+        // unlocked read, so two simultaneous checkout requests can both
+        // observe the asset as available and both proceed through
+        // checkOut(), producing duplicate checkout-history rows and
+        // double-incrementing checkout_counter on a single-assignment
+        // asset. Re-fetch the row under lockForUpdate INSIDE the
+        // transaction and re-check availability against the locked
+        // snapshot. Any concurrent checkout blocks on the row lock until
+        // this transaction commits, then sees the asset as no longer
+        // available. Mirrors the pattern in ConsumablesController::store
+        // (GHSA-x4g2-87xc-m5jm).
         $wasCheckedOut = DB::transaction(function () use ($asset, $target, $checkout_at, $expected_checkin, $note, $asset_name): bool {
+            $locked = Asset::whereKey($asset->id)->lockForUpdate()->first();
+            if (! $locked || ! $locked->availableForCheckout()) {
+                return false;
+            }
+
             return $asset->checkOut($target, auth()->user(), $checkout_at, $expected_checkin, $note, $asset_name, $asset->location_id);
         });
 

app/Http/Controllers/Assets/AssetCheckoutController.php

@@ -14,6 +14,7 @@ use App\Models\User;
 use Illuminate\Contracts\View\View;
 use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Http\RedirectResponse;
+use Illuminate\Support\Facades\DB;
 
 class AssetCheckoutController extends Controller
 {
@@ -146,7 +147,27 @@ class AssetCheckoutController extends Controller
                 'sign_in_place' => $request->boolean('sign_in_place'),
             ]);
 
-            if ($asset->checkOut($target, $admin, $checkout_at, $expected_checkin, $request->input('note'), $request->input('name'), null, $request->boolean('sign_in_place'))) {
+            // Concurrency guard. availableForCheckout() above ran on an
+            // unlocked read, so two simultaneous form submits can both
+            // observe the asset as available and both proceed through
+            // checkOut(), producing duplicate checkout-history rows and
+            // double-incrementing checkout_counter on a single-assignment
+            // asset. Re-fetch the row under lockForUpdate INSIDE a
+            // transaction and re-check availability against the locked
+            // snapshot; the second request blocks until the first commits
+            // and then sees the asset as no longer available. Mirrors the
+            // pattern in Api\AssetsController::checkout and
+            // ConsumablesController::store (GHSA-x4g2-87xc-m5jm).
+            $checkedOut = DB::transaction(function () use ($asset, $target, $admin, $checkout_at, $expected_checkin, $request): bool {
+                $locked = Asset::whereKey($asset->id)->lockForUpdate()->first();
+                if (! $locked || ! $locked->availableForCheckout()) {
+                    return false;
+                }
+
+                return (bool) $asset->checkOut($target, $admin, $checkout_at, $expected_checkin, $request->input('note'), $request->input('name'), null, $request->boolean('sign_in_place'));
+            });
+
+            if ($checkedOut) {
 
                 // When sign_in_place is requested and the target is a user, redirect to the
                 // acceptance/signature page so the user can sign in person. The signature is

app/Http/Controllers/Assets/BulkAssetsController.php

@@ -768,6 +768,27 @@ class BulkAssetsController extends Controller
                     // request, so the operator's explicit choice sticks.
                     $asset->requestable = $request->boolean('requestable');
 
+                    // Concurrency guard, same shape as Api\AssetsController::checkout.
+                    // Bulk checkout iterates over a selection of asset IDs and
+                    // calls checkOut per asset without a per-row lock; two
+                    // operators submitting overlapping bulk selections at the
+                    // same instant could each pass the caller-side selection
+                    // and both proceed through checkOut on the same asset,
+                    // landing duplicate history rows and doubling
+                    // checkout_counter for that asset. Re-fetch the row under
+                    // lockForUpdate and re-check availability before invoking
+                    // checkOut. Assets that racing bulk actions have already
+                    // claimed are skipped and surfaced as errors, matching how
+                    // the per-asset checkout path behaves.
+                    $locked = Asset::whereKey($asset->id)->lockForUpdate()->first();
+                    if (! $locked || ! $locked->availableForCheckout()) {
+                        $errors = array_merge_recursive($errors, [
+                            'asset_'.$asset->id => [trans('admin/hardware/message.checkout.not_available')],
+                        ]);
+
+                        continue;
+                    }
+
                     $checkout_success = $asset->checkOut($target, $admin, $checkout_at, $expected_checkin, e($request->input('note')), $asset->name, null);
 
                     // TODO - I think this logic is duplicated in the checkOut method?

app/Http/Controllers/Licenses/LicenseCheckoutController.php

@@ -303,16 +303,36 @@ class LicenseCheckoutController extends Controller
                 continue;
             }
 
-            $licenseSeat = $license->freeSeat();
-
-            // Update the seat with checkout info
-            $licenseSeat->assigned_to = $user->id;
-
-            if ($licenseSeat->save()) {
+            // Concurrency guard, same shape as Api\LicensesController::checkout.
+            // freeSeat() without $lock=true returns the first-available
+            // LicenseSeat unlocked; two racing bulkCheckout runs on the same
+            // license could each grab the same seat, both call save(), and
+            // both assigned_to writes land (second wins). The visible
+            // assignment is fine but logCheckout below runs twice and the
+            // decrement of $avail_count double-counts. Wrap each iteration
+            // in a transaction with freeSeat(lock: true) so the seat is
+            // pinned to this iteration until the save + log commit.
+            $seatClaimed = DB::transaction(function () use ($license, $user, &$avail_count, &$assigned_count) {
+                $licenseSeat = $license->freeSeat(lock: true);
+                if (! $licenseSeat) {
+                    return false;
+                }
+                $licenseSeat->assigned_to = $user->id;
+                if (! $licenseSeat->save()) {
+                    return false;
+                }
                 $avail_count--;
                 $assigned_count++;
                 $licenseSeat->logCheckout(trans('admin/licenses/general.bulk.checkout_all.log_msg'), $user);
                 Log::debug('License '.$license->name.' seat '.$licenseSeat->id.' checked out to '.$user->username);
+
+                return true;
+            });
+
+            if (! $seatClaimed) {
+                Log::debug('No free seat available for '.$user->username.'. Skipping...');
+
+                continue;
             }
 
             if ($avail_count == 0) {

app/Http/Controllers/Users/UserItemTransferController.php

@@ -98,8 +98,16 @@ class UserItemTransferController extends Controller
         $skipped = [];
 
         foreach ($ids as $assetId) {
-            $asset = Asset::find($assetId);
-            if (! $this->assetBelongsToSource($asset, $source) || ! $asset->canCheckoutTo($target)) {
+            // Concurrency guard, same shape as Api\AssetsController::checkout.
+            // Transfer walks source-owned assets and re-checks them out to the
+            // target user. The checkinAsset + checkOut pair opens a window
+            // where another operator's checkout could claim the asset between
+            // the check-in and the target's re-checkout. Lock the row for the
+            // duration of the transfer and re-verify source ownership + target
+            // eligibility against the locked snapshot. Assets that have moved
+            // since the caller loaded the transfer form are skipped.
+            $asset = Asset::whereKey($assetId)->lockForUpdate()->first();
+            if (! $asset || ! $this->assetBelongsToSource($asset, $source) || ! $asset->canCheckoutTo($target)) {
                 $skipped[] = 'asset:'.$assetId;
 
                 continue;

app/Importer/AssetImporter.php

@@ -327,7 +327,19 @@ class AssetImporter extends ItemImporter
 
             // If we have a target to checkout to, lets do so.
             if (isset($target) && ($target !== false)) {
-                $asset = $asset->fresh();
+                // Concurrency guard, same shape as Api\AssetsController::checkout.
+                // Two admins importing overlapping CSVs (or one admin importing
+                // while another checkout runs through the UI) could race here:
+                // the fresh() read + canCheckoutTo() check is followed by a
+                // checkOut() call with no row lock. Re-fetch the row under
+                // lockForUpdate and evaluate the ownership / eligibility
+                // conditions against the locked snapshot. If a racing operator
+                // claimed the asset in the interim, skip this row rather than
+                // stacking a duplicate history entry.
+                $asset = Asset::whereKey($asset->id)->lockForUpdate()->first();
+                if (! $asset) {
+                    return;
+                }
 
                 if (! $asset->canCheckoutTo($target)) {
                     $this->log(trans('general.error_checkout_company_mismatch', [

app/Services/PredefinedKitCheckoutService.php

@@ -3,6 +3,7 @@
 namespace App\Services;
 
 use App\Events\CheckoutableCheckedOut;
+use App\Models\Asset;
 use App\Models\PredefinedKit;
 use App\Models\User;
 use Illuminate\Foundation\Auth\Access\AuthorizesRequests;
@@ -153,6 +154,19 @@ class PredefinedKitCheckoutService
                 // assets
                 foreach ($assets_to_add as $asset) {
                     $asset->location_id = $user->location_id;
+
+                    // Concurrency guard, same shape as Api\AssetsController::checkout.
+                    // Kit checkout can race with any other checkout of the same asset.
+                    // Re-fetch under lockForUpdate and re-check availability before
+                    // invoking checkOut; a claimed asset gets skipped rather than
+                    // producing a duplicate history row and counter bump.
+                    $locked = Asset::whereKey($asset->id)->lockForUpdate()->first();
+                    if (! $locked || ! $locked->availableForCheckout()) {
+                        $errors[] = trans('admin/hardware/message.checkout.not_available').' ('.$asset->asset_tag.')';
+
+                        continue;
+                    }
+
                     $error = $asset->checkOut($user, $admin, $checkout_at, $expected_checkin, $note, null);
                     if ($error) {
                         array_merge_recursive($errors, $asset->getErrors()->toArray());

tests/Feature/Checkouts/Api/AssetCheckoutRaceGuardTest.php

@@ -0,0 +1,72 @@
+<?php
+
+namespace Tests\Feature\Checkouts\Api;
+
+use App\Events\CheckoutableCheckedOut;
+use App\Models\Asset;
+use App\Models\User;
+use Illuminate\Support\Facades\Event;
+use Tests\TestCase;
+
+/**
+ * Regression coverage for the concurrent-checkout race reported on
+ * 2026-08-02. Api\AssetsController::checkout() checked availableForCheckout
+ * outside its transaction, then called Asset::checkOut() inside the
+ * transaction with no row lock and no re-check. Two racing requests could
+ * both see the asset as available, both invoke checkOut(), and land
+ * duplicate checkout-history rows plus a doubled checkout_counter.
+ *
+ * The fix moves the availability re-check inside the transaction with
+ * lockForUpdate on the asset row, mirroring the ConsumablesController
+ * pattern from GHSA-x4g2-87xc-m5jm. This test cannot simulate two truly
+ * concurrent HTTP requests in phpunit, but it pins the behavioral
+ * consequence: an asset that has already been assigned mid-flight cannot
+ * be checked out a second time. Any refactor that removes the lock or the
+ * availability re-check would need to preserve this observable behavior.
+ */
+class AssetCheckoutRaceGuardTest extends TestCase
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        Event::fake([CheckoutableCheckedOut::class]);
+    }
+
+    public function test_second_checkout_of_already_assigned_asset_does_not_increment_counter()
+    {
+        $firstTarget = User::factory()->create();
+        $asset = Asset::factory()->assignedToUser($firstTarget)->create(['checkout_counter' => 1]);
+
+        $this->actingAsForApi(User::factory()->superuser()->create())
+            ->postJson(route('api.asset.checkout', $asset), [
+                'checkout_to_type' => 'user',
+                'assigned_user' => User::factory()->create()->id,
+            ])
+            ->assertStatusMessageIs('error');
+
+        $asset->refresh();
+        $this->assertSame(1, (int) $asset->checkout_counter, 'checkout_counter must not increment when checkout is refused');
+        $this->assertSame($firstTarget->id, (int) $asset->assigned_to, 'existing assignment must remain intact');
+    }
+
+    public function test_second_checkout_of_already_assigned_asset_does_not_fire_checkout_event()
+    {
+        $firstTarget = User::factory()->create();
+        $asset = Asset::factory()->assignedToUser($firstTarget)->create();
+
+        $this->actingAsForApi(User::factory()->superuser()->create())
+            ->postJson(route('api.asset.checkout', $asset), [
+                'checkout_to_type' => 'user',
+                'assigned_user' => User::factory()->create()->id,
+            ])
+            ->assertStatusMessageIs('error');
+
+        // A racing second checkout that slipped past both the outer and the
+        // locked re-check would fire CheckoutableCheckedOut and generate a
+        // history row + counter bump via the CheckoutableListener chain.
+        // Asserting the event never fires is a proxy for asserting no
+        // downstream side-effects occurred.
+        Event::assertNotDispatched(CheckoutableCheckedOut::class);
+    }
+}

tests/Feature/Checkouts/Ui/AssetCheckoutRaceGuardTest.php

@@ -0,0 +1,66 @@
+<?php
+
+namespace Tests\Feature\Checkouts\Ui;
+
+use App\Events\CheckoutableCheckedOut;
+use App\Models\Asset;
+use App\Models\User;
+use Illuminate\Support\Facades\Event;
+use Tests\TestCase;
+
+/**
+ * Regression coverage for the concurrent-checkout race reported on
+ * 2026-08-02. Assets\AssetCheckoutController::store() checked
+ * availableForCheckout outside its checkOut() call, with no lock and no
+ * re-check. Two racing form submits could both see the asset as available,
+ * both invoke checkOut(), and land duplicate checkout-history rows plus a
+ * doubled checkout_counter.
+ *
+ * The fix wraps checkOut() in a DB::transaction that begins with a
+ * lockForUpdate re-fetch + re-check, mirroring the API fix and the
+ * ConsumablesController pattern from GHSA-x4g2-87xc-m5jm. This test cannot
+ * simulate two truly concurrent form submits in phpunit, but it pins the
+ * behavioral consequence: an asset that has already been assigned
+ * mid-flight cannot be checked out a second time.
+ */
+class AssetCheckoutRaceGuardTest extends TestCase
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        Event::fake([CheckoutableCheckedOut::class]);
+    }
+
+    public function test_second_checkout_of_already_assigned_asset_does_not_increment_counter()
+    {
+        $firstTarget = User::factory()->create();
+        $asset = Asset::factory()->assignedToUser($firstTarget)->create(['checkout_counter' => 1]);
+
+        $this->actingAs(User::factory()->superuser()->create())
+            ->post(route('hardware.checkout.store', $asset), [
+                'checkout_to_type' => 'user',
+                'assigned_user' => User::factory()->create()->id,
+            ])
+            ->assertRedirect();
+
+        $asset->refresh();
+        $this->assertSame(1, (int) $asset->checkout_counter, 'checkout_counter must not increment when checkout is refused');
+        $this->assertSame($firstTarget->id, (int) $asset->assigned_to, 'existing assignment must remain intact');
+    }
+
+    public function test_second_checkout_of_already_assigned_asset_does_not_fire_checkout_event()
+    {
+        $firstTarget = User::factory()->create();
+        $asset = Asset::factory()->assignedToUser($firstTarget)->create();
+
+        $this->actingAs(User::factory()->superuser()->create())
+            ->post(route('hardware.checkout.store', $asset), [
+                'checkout_to_type' => 'user',
+                'assigned_user' => User::factory()->create()->id,
+            ])
+            ->assertRedirect();
+
+        Event::assertNotDispatched(CheckoutableCheckedOut::class);
+    }
+}