From f76f936a894c6f14386f40eff962bce1ecbc868e Mon Sep 17 00:00:00 2001 From: snipe Date: Mon, 20 Jul 2026 14:23:27 +0100 Subject: [PATCH] Kits: Fixed FD-56594 - tightened up kit API routes, fixed typo --- .../Api/PredefinedKitsController.php | 27 ++ routes/api.php | 4 +- .../PredefinedKits/Api/UpdateKitItemsTest.php | 235 ++++++++++++++++++ 3 files changed, 264 insertions(+), 2 deletions(-) create mode 100644 tests/Feature/PredefinedKits/Api/UpdateKitItemsTest.php diff --git a/app/Http/Controllers/Api/PredefinedKitsController.php b/app/Http/Controllers/Api/PredefinedKitsController.php index 53f2cef152..0225483ed0 100644 --- a/app/Http/Controllers/Api/PredefinedKitsController.php +++ b/app/Http/Controllers/Api/PredefinedKitsController.php @@ -7,6 +7,7 @@ use App\Http\Controllers\Controller; use App\Http\Transformers\PredefinedKitsTransformer; use App\Http\Transformers\SelectlistTransformer; use App\Models\Accessory; +use App\Models\AssetModel; use App\Models\Consumable; use App\Models\License; use App\Models\PredefinedKit; @@ -208,6 +209,13 @@ class PredefinedKitsController extends Controller { $this->authorize('update', PredefinedKit::class); $kit = PredefinedKit::findOrFail($kit_id); + + // Mirror storeLicense: verify the caller can view the license they + // are attaching. Without this, kits.edit alone would let them attach + // (and leak the name of) a license they cannot read directly. + $license = License::findOrFail($license_id); + $this->authorize('view', $license); + $quantity = $request->input('quantity', 1); if ($quantity < 1) { $quantity = 1; @@ -263,6 +271,13 @@ class PredefinedKitsController extends Controller $quantity = 1; } + // Verify the caller can view the model they are attaching. Without + // this, kits.edit alone would let them attach (and leak the name + // of) a model they cannot read directly. Mirrors storeLicense / + // storeConsumable / storeAccessory. + $model = AssetModel::findOrFail($model_id); + $this->authorize('view', $model); + $relation = $kit->models(); if ($relation->find($model_id)) { return response()->json(Helper::formatStandardApiResponse('error', null, ['model' => trans('admin/kits/general.model_already_attached')])); @@ -281,6 +296,10 @@ class PredefinedKitsController extends Controller { $this->authorize('update', PredefinedKit::class); $kit = PredefinedKit::findOrFail($kit_id); + + $model = AssetModel::findOrFail($model_id); + $this->authorize('view', $model); + $quantity = $request->input('quantity', 1); if ($quantity < 1) { $quantity = 1; @@ -357,6 +376,10 @@ class PredefinedKitsController extends Controller { $this->authorize('update', PredefinedKit::class); $kit = PredefinedKit::findOrFail($kit_id); + + $consumable = Consumable::findOrFail($consumable_id); + $this->authorize('view', $consumable); + $quantity = $request->input('quantity', 1); if ($quantity < 1) { $quantity = 1; @@ -433,6 +456,10 @@ class PredefinedKitsController extends Controller { $this->authorize('update', PredefinedKit::class); $kit = PredefinedKit::findOrFail($kit_id); + + $accessory = Accessory::findOrFail($accessory_id); + $this->authorize('view', $accessory); + $quantity = $request->input('quantity', 1); if ($quantity < 1) { $quantity = 1; diff --git a/routes/api.php b/routes/api.php index a7ff098394..7a87124116 100644 --- a/routes/api.php +++ b/routes/api.php @@ -1302,14 +1302,14 @@ Route::group(['prefix' => 'v1', 'middleware' => ['api', 'api-throttle:api']], fu Route::put('models/{model_id}', [ Api\PredefinedKitsController::class, - 'updateModels', + 'updateModel', ] )->name('api.kits.models.update'); Route::delete('models/{model_id}', [ Api\PredefinedKitsController::class, - 'detachModels', + 'detachModel', ] )->name('api.kits.models.destroy'); diff --git a/tests/Feature/PredefinedKits/Api/UpdateKitItemsTest.php b/tests/Feature/PredefinedKits/Api/UpdateKitItemsTest.php new file mode 100644 index 0000000000..184bbfb298 --- /dev/null +++ b/tests/Feature/PredefinedKits/Api/UpdateKitItemsTest.php @@ -0,0 +1,235 @@ +create(); + $license = License::factory()->create(); + $kit->licenses()->attach($license->id, ['quantity' => 1]); + + $this->actingAsForApi(User::factory()->viewLicenses()->create()) + ->putJson(route('api.kits.licenses.update', ['kit_id' => $kit->id, 'license_id' => $license->id]), ['quantity' => 5]) + ->assertForbidden(); + + $this->assertDatabaseHas('kits_licenses', ['kit_id' => $kit->id, 'license_id' => $license->id, 'quantity' => 1]); + } + + public function test_updating_kit_license_requires_view_permission_on_license() + { + $kit = PredefinedKit::factory()->create(); + $license = License::factory()->create(); + + // The bypass: kits.edit alone previously let a user attach (and leak) + // a license they could not read directly. + $this->actingAsForApi(User::factory()->editPredefinedKits()->create()) + ->putJson(route('api.kits.licenses.update', ['kit_id' => $kit->id, 'license_id' => $license->id]), ['quantity' => 5]) + ->assertForbidden(); + + $this->assertDatabaseMissing('kits_licenses', ['kit_id' => $kit->id, 'license_id' => $license->id]); + } + + public function test_can_update_kit_license_with_both_permissions() + { + $kit = PredefinedKit::factory()->create(); + $license = License::factory()->create(); + + $this->actingAsForApi(User::factory()->editPredefinedKits()->viewLicenses()->create()) + ->putJson(route('api.kits.licenses.update', ['kit_id' => $kit->id, 'license_id' => $license->id]), ['quantity' => 5]) + ->assertOk() + ->assertStatusMessageIs('success'); + + $this->assertDatabaseHas('kits_licenses', ['kit_id' => $kit->id, 'license_id' => $license->id, 'quantity' => 5]); + } + + // ------------------------------------------------------------------------- + // Consumables (update) + // ------------------------------------------------------------------------- + + public function test_updating_kit_consumable_requires_kit_edit_permission() + { + $kit = PredefinedKit::factory()->create(); + $consumable = Consumable::factory()->create(); + $kit->consumables()->attach($consumable->id, ['quantity' => 1]); + + $this->actingAsForApi(User::factory()->viewConsumables()->create()) + ->putJson(route('api.kits.consumables.update', ['kit_id' => $kit->id, 'consumable_id' => $consumable->id]), ['quantity' => 5]) + ->assertForbidden(); + + $this->assertDatabaseHas('kits_consumables', ['kit_id' => $kit->id, 'consumable_id' => $consumable->id, 'quantity' => 1]); + } + + public function test_updating_kit_consumable_requires_view_permission_on_consumable() + { + $kit = PredefinedKit::factory()->create(); + $consumable = Consumable::factory()->create(); + + $this->actingAsForApi(User::factory()->editPredefinedKits()->create()) + ->putJson(route('api.kits.consumables.update', ['kit_id' => $kit->id, 'consumable_id' => $consumable->id]), ['quantity' => 5]) + ->assertForbidden(); + + $this->assertDatabaseMissing('kits_consumables', ['kit_id' => $kit->id, 'consumable_id' => $consumable->id]); + } + + public function test_can_update_kit_consumable_with_both_permissions() + { + $kit = PredefinedKit::factory()->create(); + $consumable = Consumable::factory()->create(); + + $this->actingAsForApi(User::factory()->editPredefinedKits()->viewConsumables()->create()) + ->putJson(route('api.kits.consumables.update', ['kit_id' => $kit->id, 'consumable_id' => $consumable->id]), ['quantity' => 5]) + ->assertOk() + ->assertStatusMessageIs('success'); + + $this->assertDatabaseHas('kits_consumables', ['kit_id' => $kit->id, 'consumable_id' => $consumable->id, 'quantity' => 5]); + } + + // ------------------------------------------------------------------------- + // Accessories (update) + // ------------------------------------------------------------------------- + + public function test_updating_kit_accessory_requires_kit_edit_permission() + { + $kit = PredefinedKit::factory()->create(); + $accessory = Accessory::factory()->create(); + $kit->accessories()->attach($accessory->id, ['quantity' => 1]); + + $this->actingAsForApi(User::factory()->viewAccessories()->create()) + ->putJson(route('api.kits.accessories.update', ['kit_id' => $kit->id, 'accessory_id' => $accessory->id]), ['quantity' => 5]) + ->assertForbidden(); + + $this->assertDatabaseHas('kits_accessories', ['kit_id' => $kit->id, 'accessory_id' => $accessory->id, 'quantity' => 1]); + } + + public function test_updating_kit_accessory_requires_view_permission_on_accessory() + { + $kit = PredefinedKit::factory()->create(); + $accessory = Accessory::factory()->create(); + + $this->actingAsForApi(User::factory()->editPredefinedKits()->create()) + ->putJson(route('api.kits.accessories.update', ['kit_id' => $kit->id, 'accessory_id' => $accessory->id]), ['quantity' => 5]) + ->assertForbidden(); + + $this->assertDatabaseMissing('kits_accessories', ['kit_id' => $kit->id, 'accessory_id' => $accessory->id]); + } + + public function test_can_update_kit_accessory_with_both_permissions() + { + $kit = PredefinedKit::factory()->create(); + $accessory = Accessory::factory()->create(); + + $this->actingAsForApi(User::factory()->editPredefinedKits()->viewAccessories()->create()) + ->putJson(route('api.kits.accessories.update', ['kit_id' => $kit->id, 'accessory_id' => $accessory->id]), ['quantity' => 5]) + ->assertOk() + ->assertStatusMessageIs('success'); + + $this->assertDatabaseHas('kits_accessories', ['kit_id' => $kit->id, 'accessory_id' => $accessory->id, 'quantity' => 5]); + } + + // ------------------------------------------------------------------------- + // Models (attach + update) + // ------------------------------------------------------------------------- + + public function test_attaching_kit_model_requires_kit_edit_permission() + { + $kit = PredefinedKit::factory()->create(); + $model = AssetModel::factory()->create(); + + $this->actingAsForApi(User::factory()->viewAssetModels()->create()) + ->postJson(route('api.kits.models.store', $kit), ['model' => $model->id, 'quantity' => 1]) + ->assertForbidden(); + + $this->assertDatabaseMissing('kits_models', ['kit_id' => $kit->id, 'model_id' => $model->id]); + } + + public function test_attaching_kit_model_requires_view_permission_on_model() + { + $kit = PredefinedKit::factory()->create(); + $model = AssetModel::factory()->create(); + + $this->actingAsForApi(User::factory()->editPredefinedKits()->create()) + ->postJson(route('api.kits.models.store', $kit), ['model' => $model->id, 'quantity' => 1]) + ->assertForbidden(); + + $this->assertDatabaseMissing('kits_models', ['kit_id' => $kit->id, 'model_id' => $model->id]); + } + + public function test_can_attach_kit_model_with_both_permissions() + { + $kit = PredefinedKit::factory()->create(); + $model = AssetModel::factory()->create(); + + $this->actingAsForApi(User::factory()->editPredefinedKits()->viewAssetModels()->create()) + ->postJson(route('api.kits.models.store', $kit), ['model' => $model->id, 'quantity' => 1]) + ->assertOk() + ->assertStatusMessageIs('success'); + + $this->assertDatabaseHas('kits_models', ['kit_id' => $kit->id, 'model_id' => $model->id]); + } + + public function test_updating_kit_model_requires_kit_edit_permission() + { + $kit = PredefinedKit::factory()->create(); + $model = AssetModel::factory()->create(); + $kit->models()->attach($model->id, ['quantity' => 1]); + + $this->actingAsForApi(User::factory()->viewAssetModels()->create()) + ->putJson(route('api.kits.models.update', ['kit_id' => $kit->id, 'model_id' => $model->id]), ['quantity' => 5]) + ->assertForbidden(); + + $this->assertDatabaseHas('kits_models', ['kit_id' => $kit->id, 'model_id' => $model->id, 'quantity' => 1]); + } + + public function test_updating_kit_model_requires_view_permission_on_model() + { + $kit = PredefinedKit::factory()->create(); + $model = AssetModel::factory()->create(); + + $this->actingAsForApi(User::factory()->editPredefinedKits()->create()) + ->putJson(route('api.kits.models.update', ['kit_id' => $kit->id, 'model_id' => $model->id]), ['quantity' => 5]) + ->assertForbidden(); + + $this->assertDatabaseMissing('kits_models', ['kit_id' => $kit->id, 'model_id' => $model->id]); + } + + public function test_can_update_kit_model_with_both_permissions() + { + $kit = PredefinedKit::factory()->create(); + $model = AssetModel::factory()->create(); + + $this->actingAsForApi(User::factory()->editPredefinedKits()->viewAssetModels()->create()) + ->putJson(route('api.kits.models.update', ['kit_id' => $kit->id, 'model_id' => $model->id]), ['quantity' => 5]) + ->assertOk() + ->assertStatusMessageIs('success'); + + $this->assertDatabaseHas('kits_models', ['kit_id' => $kit->id, 'model_id' => $model->id, 'quantity' => 5]); + } +}