Skip to content

Commit 095f794

Browse files
snipesnipe
committed
Fixed FD-57269 - tigher checks on kit gates
1 parent 716834d commit 095f794

3 files changed

Lines changed: 204 additions & 28 deletions

File tree

‎app/Http/Controllers/Kits/PredefinedKitsController.php‎

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@
44

55
use App\Http\Controllers\Controller;
66
use App\Http\Requests\ImageUploadRequest;
7+
use App\Models\Accessory;
8+
use App\Models\AssetModel;
9+
use App\Models\Consumable;
10+
use App\Models\License;
711
use App\Models\PredefinedKit;
812
use Illuminate\Auth\Access\AuthorizationException;
913
use Illuminate\Contracts\View\View;
@@ -193,7 +197,7 @@ public function editModel($kit_id, $model_id)
193197
* @author [D. Minaev] [<dmitriy.minaev.v@gmail.com>]
194198
*
195199
* @param int $modelId
196-
* @return View
200+
* @return RedirectResponse
197201
*/
198202
public function updateModel(Request $request, $kit_id, $model_id)
199203
{
@@ -209,6 +213,16 @@ public function updateModel(Request $request, $kit_id, $model_id)
209213
return redirect()->back()->withInput()->withErrors($validator);
210214
}
211215

216+
// Verify the user can view the
217+
// model id they are about to write into the pivot. Without this,
218+
// kits.edit alone would let them re-point the pivot at a model
219+
// they cannot read directly, including one in another company.
220+
$targetModel = AssetModel::find($request->input('model_id'));
221+
if (! $targetModel) {
222+
return redirect()->back()->withInput()->with('error', trans('admin/models/message.does_not_exist'));
223+
}
224+
$this->authorize('view', $targetModel);
225+
212226
$pivot = $kit->models()->wherePivot('id', $request->input('pivot_id'))->first()->pivot;
213227

214228
$pivot->model_id = $request->input('model_id');
@@ -274,7 +288,7 @@ public function editLicense($kit_id, $license_id)
274288
*
275289
* @param int $kit_id
276290
* @param int $license_id
277-
* @return View
291+
* @return RedirectResponse
278292
*/
279293
public function updateLicense(Request $request, $kit_id, $license_id)
280294
{
@@ -290,6 +304,14 @@ public function updateLicense(Request $request, $kit_id, $license_id)
290304
return redirect()->back()->withInput()->withErrors($validator);
291305
}
292306

307+
// Verify the caller can view the license id they are about to
308+
// write into the pivot. Mirrors the API sibling from FD-56594.
309+
$targetLicense = License::find($request->input('license_id'));
310+
if (! $targetLicense) {
311+
return redirect()->back()->withInput()->with('error', trans('admin/licenses/message.does_not_exist'));
312+
}
313+
$this->authorize('view', $targetLicense);
314+
293315
$pivot = $kit->licenses()->wherePivot('id', $request->input('pivot_id'))->first()->pivot;
294316

295317
$pivot->license_id = $request->input('license_id');
@@ -356,7 +378,7 @@ public function editAccessory($kit_id, $accessory_id)
356378
*
357379
* @param int $kit_id
358380
* @param int $accessory_id
359-
* @return View
381+
* @return RedirectResponse
360382
*/
361383
public function updateAccessory(Request $request, $kit_id, $accessory_id)
362384
{
@@ -372,6 +394,14 @@ public function updateAccessory(Request $request, $kit_id, $accessory_id)
372394
return redirect()->back()->withInput()->withErrors($validator);
373395
}
374396

397+
// Verify the caller can view the accessory id they are about
398+
// to write into the pivot. Mirrors the API sibling from FD-56594.
399+
$targetAccessory = Accessory::find($request->input('accessory_id'));
400+
if (! $targetAccessory) {
401+
return redirect()->back()->withInput()->with('error', trans('admin/accessories/message.does_not_exist'));
402+
}
403+
$this->authorize('view', $targetAccessory);
404+
375405
$pivot = $kit->accessories()->wherePivot('id', $request->input('pivot_id'))->first()->pivot;
376406

377407
$pivot->accessory_id = $request->input('accessory_id');
@@ -437,7 +467,7 @@ public function editConsumable($kit_id, $consumable_id)
437467
*
438468
* @param int $kit_id
439469
* @param int $consumableId
440-
* @return View
470+
* @return RedirectResponse
441471
*/
442472
public function updateConsumable(Request $request, $kit_id, $consumable_id)
443473
{
@@ -453,6 +483,14 @@ public function updateConsumable(Request $request, $kit_id, $consumable_id)
453483
return redirect()->back()->withInput()->withErrors($validator);
454484
}
455485

486+
// Verify the caller can view the consumable id they are about
487+
// to write into the pivot. Mirrors the API sibling from FD-56594.
488+
$targetConsumable = Consumable::find($request->input('consumable_id'));
489+
if (! $targetConsumable) {
490+
return redirect()->back()->withInput()->with('error', trans('admin/consumables/message.does_not_exist'));
491+
}
492+
$this->authorize('view', $targetConsumable);
493+
456494
$pivot = $kit->consumables()->wherePivot('id', $request->input('pivot_id'))->first()->pivot;
457495

458496
$pivot->consumable_id = $request->input('consumable_id');

‎phpstan-baseline.neon‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2826,30 +2826,6 @@ parameters:
28262826
count: 1
28272827
path: app/Http/Controllers/Kits/PredefinedKitsController.php
28282828

2829-
-
2830-
message: '#^Method App\\Http\\Controllers\\Kits\\PredefinedKitsController\:\:updateAccessory\(\) should return Illuminate\\Contracts\\View\\View but returns Illuminate\\Http\\RedirectResponse\.$#'
2831-
identifier: return.type
2832-
count: 3
2833-
path: app/Http/Controllers/Kits/PredefinedKitsController.php
2834-
2835-
-
2836-
message: '#^Method App\\Http\\Controllers\\Kits\\PredefinedKitsController\:\:updateConsumable\(\) should return Illuminate\\Contracts\\View\\View but returns Illuminate\\Http\\RedirectResponse\.$#'
2837-
identifier: return.type
2838-
count: 3
2839-
path: app/Http/Controllers/Kits/PredefinedKitsController.php
2840-
2841-
-
2842-
message: '#^Method App\\Http\\Controllers\\Kits\\PredefinedKitsController\:\:updateLicense\(\) should return Illuminate\\Contracts\\View\\View but returns Illuminate\\Http\\RedirectResponse\.$#'
2843-
identifier: return.type
2844-
count: 3
2845-
path: app/Http/Controllers/Kits/PredefinedKitsController.php
2846-
2847-
-
2848-
message: '#^Method App\\Http\\Controllers\\Kits\\PredefinedKitsController\:\:updateModel\(\) should return Illuminate\\Contracts\\View\\View but returns Illuminate\\Http\\RedirectResponse\.$#'
2849-
identifier: return.type
2850-
count: 3
2851-
path: app/Http/Controllers/Kits/PredefinedKitsController.php
2852-
28532829
-
28542830
message: '#^Negated boolean expression is always false\.$#'
28552831
identifier: booleanNot.alwaysFalse

‎tests/Feature/PredefinedKits/Ui/UpdateKitItemsAuthorizationTest.php‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
<?php
2+
3+
namespace Tests\Feature\PredefinedKits\Ui;
4+
5+
use App\Models\Accessory;
6+
use App\Models\AssetModel;
7+
use App\Models\Consumable;
8+
use App\Models\License;
9+
use App\Models\PredefinedKit;
10+
use App\Models\User;
11+
use Tests\TestCase;
12+
13+
/**
14+
* Web sibling of tests/Feature/PredefinedKits/Api/UpdateKitItemsTest.
15+
* FD-56594 added `findOrFail` + `authorize('view', $object)` to the four
16+
* update handlers on `Api\PredefinedKitsController`, but the parallel
17+
* handlers on the web `Kits\PredefinedKitsController` (`updateLicense`,
18+
* `updateModel`, `updateAccessory`, `updateConsumable`) were not covered
19+
* by that fix. A `kits.edit` holder could re-point an existing pivot at
20+
* an object id they cannot read directly, including one in another
21+
* company. This locks in the follow-up guards.
22+
*/
23+
class UpdateKitItemsAuthorizationTest extends TestCase
24+
{
25+
// -------------------------------------------------------------------------
26+
// Licenses
27+
// -------------------------------------------------------------------------
28+
29+
public function test_web_update_kit_license_requires_view_permission_on_target_license()
30+
{
31+
$kit = PredefinedKit::factory()->create();
32+
$existingLicense = License::factory()->create();
33+
$kit->licenses()->attach($existingLicense->id, ['quantity' => 1]);
34+
$pivotId = $kit->licenses()->wherePivot('license_id', $existingLicense->id)->first()->pivot->id;
35+
36+
// Attacker points the pivot at a different license they cannot view.
37+
$targetLicense = License::factory()->create();
38+
39+
$this->actingAs(User::factory()->editPredefinedKits()->create())
40+
->put(route('kits.licenses.update', ['kit' => $kit->id, 'license_id' => $existingLicense->id]), [
41+
'pivot_id' => $pivotId,
42+
'license_id' => $targetLicense->id,
43+
'quantity' => 5,
44+
])
45+
->assertForbidden();
46+
47+
$this->assertDatabaseMissing('kits_licenses', [
48+
'kit_id' => $kit->id,
49+
'license_id' => $targetLicense->id,
50+
]);
51+
$this->assertDatabaseHas('kits_licenses', [
52+
'kit_id' => $kit->id,
53+
'license_id' => $existingLicense->id,
54+
'quantity' => 1,
55+
]);
56+
}
57+
58+
public function test_web_update_kit_license_succeeds_when_view_permission_granted()
59+
{
60+
$kit = PredefinedKit::factory()->create();
61+
$existingLicense = License::factory()->create();
62+
$kit->licenses()->attach($existingLicense->id, ['quantity' => 1]);
63+
$pivotId = $kit->licenses()->wherePivot('license_id', $existingLicense->id)->first()->pivot->id;
64+
65+
$targetLicense = License::factory()->create();
66+
67+
$this->actingAs(User::factory()->editPredefinedKits()->viewLicenses()->create())
68+
->put(route('kits.licenses.update', ['kit' => $kit->id, 'license_id' => $existingLicense->id]), [
69+
'pivot_id' => $pivotId,
70+
'license_id' => $targetLicense->id,
71+
'quantity' => 5,
72+
])
73+
->assertRedirect(route('kits.edit', $kit->id));
74+
75+
$this->assertDatabaseHas('kits_licenses', [
76+
'kit_id' => $kit->id,
77+
'license_id' => $targetLicense->id,
78+
'quantity' => 5,
79+
]);
80+
}
81+
82+
// -------------------------------------------------------------------------
83+
// Models
84+
// -------------------------------------------------------------------------
85+
86+
public function test_web_update_kit_model_requires_view_permission_on_target_model()
87+
{
88+
$kit = PredefinedKit::factory()->create();
89+
$existingModel = AssetModel::factory()->create();
90+
$kit->models()->attach($existingModel->id, ['quantity' => 1]);
91+
$pivotId = $kit->models()->wherePivot('model_id', $existingModel->id)->first()->pivot->id;
92+
93+
$targetModel = AssetModel::factory()->create();
94+
95+
$this->actingAs(User::factory()->editPredefinedKits()->create())
96+
->put(route('kits.models.update', ['kit' => $kit->id, 'model_id' => $existingModel->id]), [
97+
'pivot_id' => $pivotId,
98+
'model_id' => $targetModel->id,
99+
'quantity' => 5,
100+
])
101+
->assertForbidden();
102+
103+
$this->assertDatabaseMissing('kits_models', [
104+
'kit_id' => $kit->id,
105+
'model_id' => $targetModel->id,
106+
]);
107+
}
108+
109+
// -------------------------------------------------------------------------
110+
// Accessories
111+
// -------------------------------------------------------------------------
112+
113+
public function test_web_update_kit_accessory_requires_view_permission_on_target_accessory()
114+
{
115+
$kit = PredefinedKit::factory()->create();
116+
$existingAccessory = Accessory::factory()->create();
117+
$kit->accessories()->attach($existingAccessory->id, ['quantity' => 1]);
118+
$pivotId = $kit->accessories()->wherePivot('accessory_id', $existingAccessory->id)->first()->pivot->id;
119+
120+
$targetAccessory = Accessory::factory()->create();
121+
122+
$this->actingAs(User::factory()->editPredefinedKits()->create())
123+
->put(route('kits.accessories.update', ['kit' => $kit->id, 'accessory_id' => $existingAccessory->id]), [
124+
'pivot_id' => $pivotId,
125+
'accessory_id' => $targetAccessory->id,
126+
'quantity' => 5,
127+
])
128+
->assertForbidden();
129+
130+
$this->assertDatabaseMissing('kits_accessories', [
131+
'kit_id' => $kit->id,
132+
'accessory_id' => $targetAccessory->id,
133+
]);
134+
}
135+
136+
// -------------------------------------------------------------------------
137+
// Consumables
138+
// -------------------------------------------------------------------------
139+
140+
public function test_web_update_kit_consumable_requires_view_permission_on_target_consumable()
141+
{
142+
$kit = PredefinedKit::factory()->create();
143+
$existingConsumable = Consumable::factory()->create();
144+
$kit->consumables()->attach($existingConsumable->id, ['quantity' => 1]);
145+
$pivotId = $kit->consumables()->wherePivot('consumable_id', $existingConsumable->id)->first()->pivot->id;
146+
147+
$targetConsumable = Consumable::factory()->create();
148+
149+
$this->actingAs(User::factory()->editPredefinedKits()->create())
150+
->put(route('kits.consumables.update', ['kit' => $kit->id, 'consumable_id' => $existingConsumable->id]), [
151+
'pivot_id' => $pivotId,
152+
'consumable_id' => $targetConsumable->id,
153+
'quantity' => 5,
154+
])
155+
->assertForbidden();
156+
157+
$this->assertDatabaseMissing('kits_consumables', [
158+
'kit_id' => $kit->id,
159+
'consumable_id' => $targetConsumable->id,
160+
]);
161+
}
162+
}

0 commit comments

Comments
 (0)