From 7992ca06eaeab7c1c7905ccd3068e047359c90c4 Mon Sep 17 00:00:00 2001 From: snipe Date: Fri, 7 Aug 2026 17:00:23 +0100 Subject: [PATCH] Guard against duplicate checkout requests --- .../CancelCheckoutRequestAction.php | 26 +++++++++++++++++-- .../CreateCheckoutRequestAction.php | 22 ++++++++++++++-- app/Exceptions/DuplicateCheckoutRequest.php | 14 ++++++++++ app/Exceptions/NoActiveCheckoutRequest.php | 14 ++++++++++ app/Http/Controllers/Api/CheckoutRequest.php | 12 +++++++++ app/Http/Controllers/ViewAssetsController.php | 4 +++ app/Models/Traits/Requestable.php | 22 +++++++++++++--- .../lang/en-US/admin/hardware/message.php | 2 ++ 8 files changed, 109 insertions(+), 7 deletions(-) create mode 100644 app/Exceptions/DuplicateCheckoutRequest.php create mode 100644 app/Exceptions/NoActiveCheckoutRequest.php diff --git a/app/Actions/CheckoutRequests/CancelCheckoutRequestAction.php b/app/Actions/CheckoutRequests/CancelCheckoutRequestAction.php index 65a4b63fdb..043817b178 100644 --- a/app/Actions/CheckoutRequests/CancelCheckoutRequestAction.php +++ b/app/Actions/CheckoutRequests/CancelCheckoutRequestAction.php @@ -2,6 +2,7 @@ namespace App\Actions\CheckoutRequests; +use App\Exceptions\NoActiveCheckoutRequest; use App\Models\Actionlog; use App\Models\Asset; use App\Models\Company; @@ -9,18 +10,39 @@ use App\Models\Setting; use App\Models\User; use App\Notifications\RequestAssetCancelation; use Illuminate\Auth\Access\AuthorizationException; +use Illuminate\Support\Facades\DB; class CancelCheckoutRequestAction { + /** + * @throws AuthorizationException + * @throws NoActiveCheckoutRequest + */ public static function run(Asset $asset, User $user) { if (! Company::isCurrentUserHasAccess($asset)) { throw new AuthorizationException; } - $asset->cancelRequest(); + // Cancel + counter decrement share a transaction, and the + // decrement is gated on the actual affected row count. + // Previously this method unconditionally decremented by 1 + // regardless of whether the caller had an active request, + // driving the shared requests_counter negative on no-op calls + // and letting a duplicate-request cancel decrement by less + // than the number of rows it actually canceled. + $affected = DB::transaction(function () use ($asset) { + $affected = $asset->cancelRequest(); + if ($affected > 0) { + $asset->decrement('requests_counter', $affected); + } - $asset->decrement('requests_counter', 1); + return $affected; + }); + + if ($affected === 0) { + throw new NoActiveCheckoutRequest; + } $data['item'] = $asset; $data['target'] = $user; diff --git a/app/Actions/CheckoutRequests/CreateCheckoutRequestAction.php b/app/Actions/CheckoutRequests/CreateCheckoutRequestAction.php index 55647d45dd..4cb3a8aeae 100644 --- a/app/Actions/CheckoutRequests/CreateCheckoutRequestAction.php +++ b/app/Actions/CheckoutRequests/CreateCheckoutRequestAction.php @@ -3,6 +3,7 @@ namespace App\Actions\CheckoutRequests; use App\Exceptions\AssetNotRequestable; +use App\Exceptions\DuplicateCheckoutRequest; use App\Models\Actionlog; use App\Models\Asset; use App\Models\Company; @@ -10,6 +11,7 @@ use App\Models\Setting; use App\Models\User; use App\Notifications\RequestAssetNotification; use Illuminate\Auth\Access\AuthorizationException; +use Illuminate\Support\Facades\DB; use Log; class CreateCheckoutRequestAction @@ -17,6 +19,7 @@ class CreateCheckoutRequestAction /** * @throws AssetNotRequestable * @throws AuthorizationException + * @throws DuplicateCheckoutRequest */ public static function run(Asset $asset, User $user): string { @@ -27,6 +30,15 @@ class CreateCheckoutRequestAction throw new AuthorizationException; } + // Enforce single-active-request-per-user-per-asset. Without this + // gate the same POST fires repeatedly, each firing adds an + // active CheckoutRequest row AND bumps requests_counter, but a + // single cancellation only decrements the counter by 1, so the + // counter and admin queue drift apart. + if ($asset->isRequestedBy($user)) { + throw new DuplicateCheckoutRequest; + } + $data['item'] = $asset; $data['target'] = $user; $data['item_quantity'] = 1; @@ -41,8 +53,14 @@ class CreateCheckoutRequestAction $logaction->location_id = $user->location_id ?? null; $logaction->logaction('requested'); - $asset->request(); - $asset->increment('requests_counter', 1); + // Row write + counter increment share one transaction so a + // partial failure can't leave the counter incremented without a + // matching row (or vice versa). + DB::transaction(function () use ($asset) { + $asset->request(); + $asset->increment('requests_counter', 1); + }); + try { $settings->notify((new RequestAssetNotification($data))->locale($settings->locale)); } catch (\Exception $e) { diff --git a/app/Exceptions/DuplicateCheckoutRequest.php b/app/Exceptions/DuplicateCheckoutRequest.php new file mode 100644 index 0000000000..6e42a67f65 --- /dev/null +++ b/app/Exceptions/DuplicateCheckoutRequest.php @@ -0,0 +1,14 @@ +json(Helper::formatStandardApiResponse('success', null, trans('admin/hardware/message.requests.success'))); } catch (AssetNotRequestable $e) { return response()->json(Helper::formatStandardApiResponse('error', 'Asset is not requestable')); + } catch (DuplicateCheckoutRequest $e) { + return response()->json( + Helper::formatStandardApiResponse('error', null, trans('admin/hardware/message.requests.duplicate')), + 409, + ); } catch (AuthorizationException $e) { return response()->json(Helper::formatStandardApiResponse('error', null, trans('general.insufficient_permissions'))); } catch (Exception $e) { @@ -37,6 +44,11 @@ class CheckoutRequest extends Controller CancelCheckoutRequestAction::run($asset, auth()->user()); return response()->json(Helper::formatStandardApiResponse('success', null, trans('admin/hardware/message.requests.canceled'))); + } catch (NoActiveCheckoutRequest $e) { + return response()->json( + Helper::formatStandardApiResponse('error', null, trans('admin/hardware/message.requests.no_active')), + 404, + ); } catch (AuthorizationException $e) { return response()->json(Helper::formatStandardApiResponse('error', null, trans('general.insufficient_permissions'))); } catch (Exception $e) { diff --git a/app/Http/Controllers/ViewAssetsController.php b/app/Http/Controllers/ViewAssetsController.php index 6aae8750fc..5aed1b10f6 100755 --- a/app/Http/Controllers/ViewAssetsController.php +++ b/app/Http/Controllers/ViewAssetsController.php @@ -268,6 +268,8 @@ class ViewAssetsController extends Controller return redirect()->route('requestable-assets')->with('success')->with('success', trans('admin/hardware/message.requests.success')); } catch (AssetNotRequestable $e) { return redirect()->back()->with('error', 'Asset is not requestable'); + } catch (\App\Exceptions\DuplicateCheckoutRequest $e) { + return redirect()->back()->with('error', trans('admin/hardware/message.requests.duplicate')); } catch (AuthorizationException $e) { return redirect()->back()->with('error', trans('admin/hardware/message.requests.error')); } catch (Exception $e) { @@ -283,6 +285,8 @@ class ViewAssetsController extends Controller CancelCheckoutRequestAction::run($asset, auth()->user()); return redirect()->route('requestable-assets')->with('success')->with('success', trans('admin/hardware/message.requests.canceled')); + } catch (\App\Exceptions\NoActiveCheckoutRequest $e) { + return redirect()->back()->with('error', trans('admin/hardware/message.requests.no_active')); } catch (Exception $e) { report($e); diff --git a/app/Models/Traits/Requestable.php b/app/Models/Traits/Requestable.php index edda4aace1..95a71a694f 100644 --- a/app/Models/Traits/Requestable.php +++ b/app/Models/Traits/Requestable.php @@ -18,7 +18,11 @@ trait Requestable public function isRequestedBy(User $user) { - return $this->requests->where('canceled_at', null)->where('user_id', $user->id)->first(); + // Fresh query rather than filtering the loaded ->requests + // collection so a same-request-cycle check-then-cancel sees + // current DB state instead of a possibly stale eager-loaded + // snapshot. + return $this->requests()->whereNull('canceled_at')->where('user_id', $user->id)->first(); } public function scopeRequestedBy($query, User $user) @@ -42,12 +46,24 @@ trait Requestable $this->requests()->where('user_id', auth()->id())->delete(); } - public function cancelRequest($user_id = null) + /** + * Mark every active CheckoutRequest for $user_id (or the current + * auth user) as canceled. Returns the number of rows actually + * flipped so callers can gate side effects (counter decrement, + * notifications, log entries) on real work happening. The + * whereNull filter prevents already-canceled rows from getting + * their canceled_at bumped, and prevents no-op cancellations from + * looking like real events downstream. + */ + public function cancelRequest($user_id = null): int { if (! $user_id) { $user_id = auth()->id(); } - $this->requests()->where('user_id', $user_id)->update(['canceled_at' => Carbon::now()]); + return $this->requests() + ->where('user_id', $user_id) + ->whereNull('canceled_at') + ->update(['canceled_at' => Carbon::now()]); } } diff --git a/resources/lang/en-US/admin/hardware/message.php b/resources/lang/en-US/admin/hardware/message.php index c32edca069..da8610e04c 100644 --- a/resources/lang/en-US/admin/hardware/message.php +++ b/resources/lang/en-US/admin/hardware/message.php @@ -173,6 +173,8 @@ return [ 'success' => 'Request successfully submitted.', 'canceled' => 'Request successfully canceled.', 'cancel' => 'Cancel this item request', + 'duplicate' => 'You already have an active request for this item.', + 'no_active' => 'You have no active request to cancel for this item.', ], ];