Skip to content

Commit 9daf960

Browse files
snipesnipe
committed
A few more gate tweaks for API responses
1 parent 31993f4 commit 9daf960

4 files changed

Lines changed: 78 additions & 41 deletions

File tree

‎app/Http/Transformers/AssetsTransformer.php‎

Lines changed: 36 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -187,16 +187,23 @@ public function transformAsset(Asset $asset)
187187
$array['components'] = [];
188188

189189
foreach ($asset->components as $component) {
190-
$array['components'][] = [
190+
// Info-disclosure guard: if the caller is denied view
191+
// on this specific component, omit it from the response
192+
// entirely - not even id / pivot_id are exposed, so a
193+
// caller with an explicit components.view deny can't
194+
// enumerate what's on the asset.
195+
if (Gate::denies('view', $component)) {
196+
continue;
197+
}
191198

199+
$array['components'][] = [
192200
'id' => $component->id,
193201
'pivot_id' => $component->pivot->id,
194202
'name' => e($component->name),
195203
'qty' => $component->pivot->assigned_qty,
196204
'purchase_cost' => $component->purchase_cost,
197205
'purchase_total' => $component->calculated_purchase_cost,
198206
'checkout_date' => Helper::getFormattedDateObject($component->pivot->created_at, 'datetime'),
199-
200207
];
201208
}
202209
}
@@ -230,7 +237,7 @@ public function transformAssignedTo($asset)
230237
// stripped is PII (username, email, employee_num, jobtitle,
231238
// first/last name split). Instance check so FMCS scoping
232239
// applies too.
233-
if (! Gate::allows('view', $asset->assigned)) {
240+
if (Gate::denies('view', $asset->assigned)) {
234241
return [
235242
'id' => (int) $asset->assigned->id,
236243
'type' => 'user',
@@ -430,44 +437,49 @@ public function transformLicenseCheckedToAsset(LicenseSeat $licenseseat)
430437
public function transformCheckedoutComponents(Collection $components_assets, $total)
431438
{
432439
$array = [];
440+
$suppressed = 0;
433441
foreach ($components_assets as $component_checkout) {
434442
$component = $component_checkout->component;
435443

436444
// Info-disclosure guard: GET /api/v1/hardware/{asset}/assigned/components
437445
// is gated only on assets.view, so a caller with assets.view but
438-
// not components.view used to read the component's name / qty /
439-
// note straight off this response. Fall back to `{id, type}`
440-
// when the caller can't view the specific component. Instance
441-
// check so FMCS scoping applies too.
442-
$canViewComponent = $component && Gate::allows('view', $component);
446+
// an explicit deny on components.view used to read the component's
447+
// name / qty / note straight off this response. When denied, skip
448+
// the row entirely so nothing about the component (not even id
449+
// or existence) is exposed. Instance check so FMCS scoping
450+
// applies too.
451+
//
452+
// The controller-supplied $total still reflects the true row
453+
// count and would leak "there are N components you can't see",
454+
// so decrement it by the number of rows suppressed here.
455+
if (! $component || Gate::denies('view', $component)) {
456+
$suppressed++;
457+
458+
continue;
459+
}
443460

444461
$array[] = [
445462
'assigned_pivot_id' => $component_checkout->id,
446-
'name' => $canViewComponent
447-
? [
448-
'id' => $component->id,
449-
'name' => e($component->display_name),
450-
'type' => 'component',
451-
'deleted_at' => $component->deleted_at,
452-
]
453-
: [
454-
'id' => $component?->id,
455-
'type' => 'component',
456-
],
457-
'assigned_qty' => $canViewComponent ? $component_checkout->assigned_qty : null,
458-
'note' => $canViewComponent && $component_checkout->note ? e($component_checkout->note) : null,
463+
'name' => [
464+
'id' => $component->id,
465+
'name' => e($component->display_name),
466+
'type' => 'component',
467+
'deleted_at' => $component->deleted_at,
468+
],
469+
'assigned_qty' => $component_checkout->assigned_qty,
470+
'note' => ($component_checkout->note) ? e($component_checkout->note) : null,
459471
'created_at' => Helper::getFormattedDateObject($component_checkout->created_at, 'datetime'),
460472
'created_by' => $component_checkout->adminuser ? [
461473
'id' => (int) $component_checkout->adminuser->id,
462474
'name' => e($component_checkout->adminuser->display_name),
463475
] : null,
464476
'available_actions' => [
465-
'checkin' => (($component?->deleted_at == '') && Gate::allows('checkin', Component::class)),
466-
'view' => (($component?->deleted_at == '') && Gate::allows('view', Component::class)),
477+
'checkin' => (($component->deleted_at == '') && Gate::allows('checkin', Component::class)),
478+
'view' => (($component->deleted_at == '') && Gate::allows('view', Component::class)),
467479
],
468480
];
469481
}
470482

471-
return (new DatatablesTransformer)->transformDatatables($array, $total);
483+
return (new DatatablesTransformer)->transformDatatables($array, max(0, $total - $suppressed));
472484
}
473485
}

‎app/Http/Transformers/LicenseSeatsTransformer.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ private function transformAssignedUser(LicenseSeat $seat): ?array
7979
return null;
8080
}
8181

82-
if (! Gate::allows('view', $seat->user)) {
82+
if (Gate::denies('view', $seat->user)) {
8383
return [
8484
'id' => (int) $seat->user->id,
8585
'type' => 'user',

‎app/Http/Transformers/UsersTransformer.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ public function transformUser(User $user)
4242
// seat, an asset) legitimately needs to see WHO has it. What
4343
// gets stripped is PII (email, phone, address, department,
4444
// companies, permissions, employee_num, jobtitle, etc.).
45-
if (auth()->id() !== $user->id && ! Gate::allows('view', $user)) {
45+
if (auth()->id() !== $user->id && Gate::denies('view', $user)) {
4646
return [
4747
'id' => (int) $user->id,
4848
'type' => 'user',
@@ -186,7 +186,7 @@ public function transformUserCompact(User $user): array
186186
// Gate check runs against the target instance, not the class, so
187187
// FMCS company-scoping applies too - a caller with users.view can
188188
// still be denied identity of a user in a company they can't see.
189-
if (! Gate::allows('view', $user)) {
189+
if (Gate::denies('view', $user)) {
190190
return [
191191
'id' => (int) $user->id,
192192
'type' => 'user',

‎tests/Feature/Assets/Api/AssignedComponentsTest.php‎

Lines changed: 39 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,10 @@ public function test_can_get_components_assigned_to_specific_asset()
4343

4444
$componentsAssignedToAsset = $asset->components;
4545

46-
$response = $this->actingAsForApi(User::factory()->viewAssets()->create())
46+
// Needs viewComponents too - the transformer now omits components
47+
// the caller can't view, so a viewAssets-only actor would see an
48+
// empty list under the fix.
49+
$response = $this->actingAsForApi(User::factory()->viewAssets()->viewComponents()->create())
4750
->getJson(route('api.assets.assigned_components', $asset))
4851
->assertOk()
4952
// ->assertResponseContainsInRows($componentsAssignedToAsset)
@@ -60,9 +63,10 @@ public function test_can_get_components_assigned_to_specific_asset()
6063
* AssetsTransformer::transformCheckedoutComponents, a caller with
6164
* assets.view but not components.view could read the component's
6265
* name / assigned_qty / note through this endpoint (direct
63-
* GET /api/v1/components/{id} correctly returned 403). Now the row
64-
* collapses to just `{id, type: 'component'}` in the name block,
65-
* and qty / note are nulled, when the caller lacks components.view.
66+
* GET /api/v1/components/{id} correctly returned 403). Now denied
67+
* rows are omitted entirely - the response neither reveals the
68+
* component's id nor that it exists at all. total is decremented so
69+
* it doesn't leak "there are N components you can't see" either.
6670
*/
6771
public function test_does_not_leak_component_details_when_caller_lacks_components_view(): void
6872
{
@@ -79,18 +83,11 @@ public function test_does_not_leak_component_details_when_caller_lacks_component
7983
// viewAssets only. No viewComponents.
8084
$actor = User::factory()->viewAssets()->create();
8185

82-
$response = $this->actingAsForApi($actor)
86+
$this->actingAsForApi($actor)
8387
->getJson(route('api.assets.assigned_components', $asset))
8488
->assertOk()
85-
->assertJsonPath('total', 1)
86-
->assertJsonPath('rows.0.name.id', $component->id)
87-
->assertJsonPath('rows.0.name.type', 'component');
88-
89-
$nameBlock = $response->json('rows.0.name');
90-
$this->assertArrayNotHasKey('name', $nameBlock, 'Component name must not leak to a caller without components.view');
91-
92-
$this->assertNull($response->json('rows.0.assigned_qty'), 'assigned_qty must not leak');
93-
$this->assertNull($response->json('rows.0.note'), 'note must not leak');
89+
->assertJsonPath('total', 0)
90+
->assertJsonCount(0, 'rows');
9491
}
9592

9693
/**
@@ -120,4 +117,32 @@ public function test_returns_full_component_details_when_caller_has_components_v
120117
->assertJsonPath('rows.0.assigned_qty', 2)
121118
->assertJsonPath('rows.0.note', 'component note');
122119
}
120+
121+
/**
122+
* Distinct code path from the dedicated /assigned/components endpoint:
123+
* the main asset detail response inlines full component blocks under
124+
* the `components` key when the caller passes `?components=true`.
125+
* Same class of leak, same fix pattern applied inline in
126+
* AssetsTransformer::transformAsset. Denied components are omitted
127+
* from the response entirely.
128+
*/
129+
public function test_show_asset_with_components_true_query_omits_components_when_denied(): void
130+
{
131+
$asset = Asset::factory()->create();
132+
$component = Component::factory()->create(['name' => 'Hidden CPU', 'purchase_cost' => 500]);
133+
$component->assets()->attach($component->id, [
134+
'component_id' => $component->id,
135+
'assigned_qty' => 3,
136+
'created_by' => User::factory()->superuser()->create()->id,
137+
'asset_id' => $asset->id,
138+
]);
139+
140+
$actor = User::factory()->viewAssets()->create();
141+
142+
$response = $this->actingAsForApi($actor)
143+
->getJson(route('api.assets.show', ['hardware' => $asset->id, 'components' => 'true']))
144+
->assertOk();
145+
146+
$this->assertSame([], $response->json('components'), 'Denied components must be absent entirely - not even id / pivot_id');
147+
}
123148
}

0 commit comments

Comments
 (0)