diff --git a/app/Http/Controllers/ViewAssetsController.php b/app/Http/Controllers/ViewAssetsController.php index 83fed1ef6c..6aae8750fc 100755 --- a/app/Http/Controllers/ViewAssetsController.php +++ b/app/Http/Controllers/ViewAssetsController.php @@ -230,8 +230,14 @@ class ViewAssetsController extends Controller return redirect()->back()->with('success')->with('success', trans('admin/hardware/message.requests.canceled')); } else { + // AssetModel was previously missing from this gate, so a + // POST to /account/request/asset_model/{id} would bypass + // the model's `requestable` flag entirely and still + // create a request record. Now uses RequestableModels() + // to match the Asset / Accessory checks above. if (($fullItemType === Asset::class && is_null(Asset::RequestableAssets()->find($item->id))) - || ($fullItemType === Accessory::class && is_null(Accessory::RequestableAccessories()->find($item->id)))) { + || ($fullItemType === Accessory::class && is_null(Accessory::RequestableAccessories()->find($item->id))) + || ($fullItemType === AssetModel::class && is_null(AssetModel::RequestableModels()->find($item->id)))) { return redirect()->back()->with('error', trans('admin/hardware/message.requests.error')); } diff --git a/routes/web.php b/routes/web.php index e270350b44..d27adbef0c 100644 --- a/routes/web.php +++ b/routes/web.php @@ -489,7 +489,8 @@ Route::group(['prefix' => 'account', 'middleware' => ['auth']], function () { ->name('account.request-asset.cancel'); Route::post('request/{itemType}/{itemId}/{cancel_by_admin?}/{requestingUser?}', [ViewAssetsController::class, 'getRequestItem']) - ->name('account/request-item'); + ->name('account/request-item') + ->where('itemType', 'asset|asset_model|accessory'); Route::get( 'display-sig/{filename}', diff --git a/tests/Feature/Requests/AssetModelRequestGateTest.php b/tests/Feature/Requests/AssetModelRequestGateTest.php new file mode 100644 index 0000000000..40e71d8f48 --- /dev/null +++ b/tests/Feature/Requests/AssetModelRequestGateTest.php @@ -0,0 +1,89 @@ +create(); + $model = AssetModel::factory()->create(['requestable' => 0]); + + $response = $this->actingAs($user) + ->post(route('account/request-item', ['itemType' => 'asset_model', 'itemId' => $model->id])); + + $response->assertStatus(302); + $response->assertSessionHas('error'); + + // No request record should have been created. + $this->assertDatabaseMissing('action_logs', [ + 'item_id' => $model->id, + 'item_type' => AssetModel::class, + 'action_type' => 'requested', + ]); + } + + public function test_requestable_asset_model_can_still_be_requested(): void + { + // Non-regression: flipping the requestable flag ON must still + // let the request go through. + $user = User::factory()->create(); + $model = AssetModel::factory()->create(['requestable' => 1]); + + $response = $this->actingAs($user) + ->post(route('account/request-item', ['itemType' => 'asset_model', 'itemId' => $model->id])); + + $response->assertStatus(302); + $response->assertSessionHas('success'); + } + + public function test_arbitrary_item_type_is_rejected_by_route_constraint(): void + { + // Route now constrains {itemType} to asset|asset_model|accessory, + // so arbitrary values like `user` no longer resolve to + // App\Models\User via the studly_case concatenation inside + // the controller. Anything else 404s at the router. + $user = User::factory()->create(); + $victim = User::factory()->create(); + + $response = $this->actingAs($user) + ->post('/account/request/user/'.$victim->id); + + $response->assertNotFound(); + } + + public function test_ignored_item_types_do_not_reach_the_controller(): void + { + // Belt-and-suspenders check for a couple more shapes. + $user = User::factory()->create(); + + foreach (['location', 'component', 'consumable', 'license'] as $itemType) { + $response = $this->actingAs($user) + ->post('/account/request/'.$itemType.'/1'); + $response->assertNotFound(); + } + + // Make sure no stray Actionlog rows landed from the loop. + $this->assertSame(0, Actionlog::where('action_type', 'requested')->count()); + } +}