From 951bd4f0e5827e4fe25f9b1d90f2aef3ac8a625f Mon Sep 17 00:00:00 2001 From: snipe Date: Sun, 2 Aug 2026 11:39:08 +0100 Subject: [PATCH] Fixed FD-56819 - concurrency issues --- app/Http/Controllers/Api/AssetsController.php | 30 +++++++- .../Assets/AssetCheckoutController.php | 23 +++++- .../Assets/BulkAssetsController.php | 21 ++++++ .../Licenses/LicenseCheckoutController.php | 32 +++++++-- .../Users/UserItemTransferController.php | 12 +++- app/Importer/AssetImporter.php | 14 +++- app/Services/PredefinedKitCheckoutService.php | 14 ++++ .../Api/AssetCheckoutRaceGuardTest.php | 72 +++++++++++++++++++ .../Ui/AssetCheckoutRaceGuardTest.php | 66 +++++++++++++++++ 9 files changed, 273 insertions(+), 11 deletions(-) create mode 100644 tests/Feature/Checkouts/Api/AssetCheckoutRaceGuardTest.php create mode 100644 tests/Feature/Checkouts/Ui/AssetCheckoutRaceGuardTest.php diff --git a/app/Http/Controllers/Api/AssetsController.php b/app/Http/Controllers/Api/AssetsController.php index 8381706f2d..6f73ff35c1 100644 --- a/app/Http/Controllers/Api/AssetsController.php +++ b/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); }); diff --git a/app/Http/Controllers/Assets/AssetCheckoutController.php b/app/Http/Controllers/Assets/AssetCheckoutController.php index d78e7ba4b2..ba6b743900 100644 --- a/app/Http/Controllers/Assets/AssetCheckoutController.php +++ b/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 diff --git a/app/Http/Controllers/Assets/BulkAssetsController.php b/app/Http/Controllers/Assets/BulkAssetsController.php index 672b71d475..1d5f189af0 100644 --- a/app/Http/Controllers/Assets/BulkAssetsController.php +++ b/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? diff --git a/app/Http/Controllers/Licenses/LicenseCheckoutController.php b/app/Http/Controllers/Licenses/LicenseCheckoutController.php index 2e8b736e6e..29abb0c26c 100644 --- a/app/Http/Controllers/Licenses/LicenseCheckoutController.php +++ b/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) { diff --git a/app/Http/Controllers/Users/UserItemTransferController.php b/app/Http/Controllers/Users/UserItemTransferController.php index 0231b10b64..93bfd2bc02 100644 --- a/app/Http/Controllers/Users/UserItemTransferController.php +++ b/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; diff --git a/app/Importer/AssetImporter.php b/app/Importer/AssetImporter.php index 9a20d15f51..6e2beb04fb 100644 --- a/app/Importer/AssetImporter.php +++ b/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', [ diff --git a/app/Services/PredefinedKitCheckoutService.php b/app/Services/PredefinedKitCheckoutService.php index 9a04d0aa4c..2c2b6bdd96 100644 --- a/app/Services/PredefinedKitCheckoutService.php +++ b/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()); diff --git a/tests/Feature/Checkouts/Api/AssetCheckoutRaceGuardTest.php b/tests/Feature/Checkouts/Api/AssetCheckoutRaceGuardTest.php new file mode 100644 index 0000000000..f70461a00f --- /dev/null +++ b/tests/Feature/Checkouts/Api/AssetCheckoutRaceGuardTest.php @@ -0,0 +1,72 @@ +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); + } +} diff --git a/tests/Feature/Checkouts/Ui/AssetCheckoutRaceGuardTest.php b/tests/Feature/Checkouts/Ui/AssetCheckoutRaceGuardTest.php new file mode 100644 index 0000000000..8c55a0fdc2 --- /dev/null +++ b/tests/Feature/Checkouts/Ui/AssetCheckoutRaceGuardTest.php @@ -0,0 +1,66 @@ +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); + } +}