Skip to content

Commit a67b320

Browse files
snipesnipe
authored
Merge pull request #15907 from uberbrady/protect_assigned_to_assigned_type_rebased
Rebased version of #15629 - prevent setting assigned_to without setting assigned_type
2 parents 9ef2099 + eccdcc3 commit a67b320

File tree

8 files changed

+167
-9
lines changed

8 files changed

+167
-9
lines changed

‎app/Http/Controllers/Api/AssetsController.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -701,7 +701,9 @@ public function store(StoreAssetRequest $request): JsonResponse
701701

702702
return response()->json(Helper::formatStandardApiResponse('success', $asset, trans('admin/hardware/message.create.success')));
703703

704-
return response()->json(Helper::formatStandardApiResponse('success', (new AssetsTransformer)->transformAsset($asset), trans('admin/hardware/message.create.success')));
704+
// below is what we want the _eventual_ return to look like - in a more standardized format.
705+
// return response()->json(Helper::formatStandardApiResponse('success', (new AssetsTransformer)->transformAsset($asset), trans('admin/hardware/message.create.success')));
706+
705707
}
706708

707709
return response()->json(Helper::formatStandardApiResponse('error', null, $asset->getErrors()), 200);

‎app/Http/Requests/StoreAssetRequest.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ public function prepareForValidation(): void
3939
$this->merge([
4040
'asset_tag' => $this->asset_tag ?? Asset::autoincrement_asset(),
4141
'company_id' => $idForCurrentUser,
42-
'assigned_to' => $assigned_to ?? null,
4342
]);
4443
}
4544

‎app/Models/Asset.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,8 @@ public function declinedCheckout(User $declinedBy, $signature)
119119
'byod' => ['nullable', 'boolean'],
120120
'order_number' => ['nullable', 'string', 'max:191'],
121121
'notes' => ['nullable', 'string', 'max:65535'],
122-
'assigned_to' => ['nullable', 'integer'],
122+
'assigned_to' => ['nullable', 'integer', 'required_with:assigned_type'],
123+
'assigned_type' => ['nullable', 'required_with:assigned_to', 'in:'.User::class.",".Location::class.",".Asset::class],
123124
'requestable' => ['nullable', 'boolean'],
124125
'assigned_user' => ['nullable', 'exists:users,id,deleted_at,NULL'],
125126
'assigned_location' => ['nullable', 'exists:locations,id,deleted_at,NULL', 'fmcs_location'],

‎app/Observers/AssetObserver.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,9 @@ public function updating(Asset $asset)
4040

4141
// If the asset isn't being checked out or audited, log the update.
4242
// (Those other actions already create log entries.)
43-
if (($attributes['assigned_to'] == $attributesOriginal['assigned_to'])
44-
&& ($same_checkout_counter) && ($same_checkin_counter)
43+
if (array_key_exists('assigned_to', $attributes) && array_key_exists('assigned_to', $attributesOriginal)
44+
&& ($attributes['assigned_to'] == $attributesOriginal['assigned_to'])
45+
&& ($same_checkout_counter) && ($same_checkin_counter)
4546
&& ((isset( $attributes['next_audit_date']) ? $attributes['next_audit_date'] : null) == (isset($attributesOriginal['next_audit_date']) ? $attributesOriginal['next_audit_date']: null))
4647
&& ($attributes['last_checkout'] == $attributesOriginal['last_checkout']) && (!$restoring_or_deleting))
4748
{

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ public function testAssetApiIndexReturnsDueOrOverdueForAudit()
8383

8484
public function testAssetApiIndexReturnsDueForExpectedCheckin()
8585
{
86-
Asset::factory()->count(3)->create(['assigned_to' => '1', 'expected_checkin' => Carbon::now()->format('Y-m-d')]);
86+
Asset::factory()->count(3)->create(['assigned_to' => '1', 'assigned_type' => User::class, 'expected_checkin' => Carbon::now()->format('Y-m-d')]);
8787

8888
$this->actingAsForApi(User::factory()->superuser()->create())
8989
->getJson(
@@ -99,7 +99,7 @@ public function testAssetApiIndexReturnsDueForExpectedCheckin()
9999

100100
public function testAssetApiIndexReturnsOverdueForExpectedCheckin()
101101
{
102-
Asset::factory()->count(3)->create(['assigned_to' => '1', 'expected_checkin' => Carbon::now()->subDays(1)->format('Y-m-d')]);
102+
Asset::factory()->count(3)->create(['assigned_to' => '1', 'assigned_type' => User::class, 'expected_checkin' => Carbon::now()->subDays(1)->format('Y-m-d')]);
103103

104104
$this->actingAsForApi(User::factory()->superuser()->create())
105105
->getJson(route('api.assets.list-upcoming', ['action' => 'checkins', 'upcoming_status' => 'overdue']))
@@ -113,8 +113,8 @@ public function testAssetApiIndexReturnsOverdueForExpectedCheckin()
113113

114114
public function testAssetApiIndexReturnsDueOrOverdueForExpectedCheckin()
115115
{
116-
Asset::factory()->count(3)->create(['assigned_to' => '1', 'expected_checkin' => Carbon::now()->subDays(1)->format('Y-m-d')]);
117-
Asset::factory()->count(2)->create(['assigned_to' => '1', 'expected_checkin' => Carbon::now()->format('Y-m-d')]);
116+
Asset::factory()->count(3)->create(['assigned_to' => '1', 'assigned_type' => User::class, 'expected_checkin' => Carbon::now()->subDays(1)->format('Y-m-d')]);
117+
Asset::factory()->count(2)->create(['assigned_to' => '1', 'assigned_type' => User::class, 'expected_checkin' => Carbon::now()->format('Y-m-d')]);
118118

119119
$this->actingAsForApi(User::factory()->superuser()->create())
120120
->getJson(route('api.assets.list-upcoming', ['action' => 'checkins', 'upcoming_status' => 'due-or-overdue']))

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

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,71 @@ public function testSaveWithPendingStatusAndUserReturnsValidationError()
162162
$this->assertNotNull($response->json('messages.status_id'));
163163
}
164164

165+
public function testSaveWithAssignedToChecksOut()
166+
{
167+
$user = User::factory()->create();
168+
$response = $this->actingAsForApi(User::factory()->superuser()->create())
169+
->postJson(route('api.assets.store'), [
170+
'asset_tag' => '1235',
171+
'assigned_to' => $user->id,
172+
'assigned_type' => User::class,
173+
'model_id' => AssetModel::factory()->create()->id,
174+
'status_id' => Statuslabel::factory()->readyToDeploy()->create()->id,
175+
])
176+
->assertOk()
177+
->assertStatusMessageIs('success');
178+
179+
$asset = Asset::find($response->json()['payload']['id']);
180+
$this->assertEquals($user->id, $asset->assigned_to);
181+
$this->assertEquals('Asset created successfully. :)', $response->json('messages'));
182+
}
183+
184+
185+
public function testSaveWithNoAssignedTypeReturnsValidationError()
186+
{
187+
$response = $this->actingAsForApi(User::factory()->superuser()->create())
188+
->postJson(route('api.assets.store'), [
189+
'asset_tag' => '1235',
190+
'assigned_to' => '1',
191+
// 'assigned_type' => User::class, //deliberately omit assigned_type
192+
'model_id' => AssetModel::factory()->create()->id,
193+
'status_id' => Statuslabel::factory()->readyToDeploy()->create()->id,
194+
])
195+
->assertOk()
196+
->assertStatusMessageIs('error');
197+
$this->assertNotNull($response->json('messages.assigned_type'));
198+
}
199+
200+
public function testSaveWithBadAssignedTypeReturnsValidationError()
201+
{
202+
$response = $this->actingAsForApi(User::factory()->superuser()->create())
203+
->postJson(route('api.assets.store'), [
204+
'asset_tag' => '1235',
205+
'assigned_to' => '1',
206+
'assigned_type' => 'nonsense_string', //deliberately bad assigned_type
207+
'model_id' => AssetModel::factory()->create()->id,
208+
'status_id' => Statuslabel::factory()->readyToDeploy()->create()->id,
209+
])
210+
->assertOk()
211+
->assertStatusMessageIs('error');
212+
$this->assertNotNull($response->json('messages.assigned_type'));
213+
}
214+
215+
public function testSaveWithAssignedTypeAndNoAssignedToReturnsValidationError()
216+
{
217+
$response = $this->actingAsForApi(User::factory()->superuser()->create())
218+
->postJson(route('api.assets.store'), [
219+
'asset_tag' => '1235',
220+
//'assigned_to' => '1', //deliberately omit assigned_to
221+
'assigned_type' => User::class,
222+
'model_id' => AssetModel::factory()->create()->id,
223+
'status_id' => Statuslabel::factory()->readyToDeploy()->create()->id,
224+
])
225+
->assertOk()
226+
->assertStatusMessageIs('error');
227+
$this->assertNotNull($response->json('messages.assigned_to'));
228+
}
229+
165230
public function testSaveWithPendingStatusWithoutUserIsSuccessful()
166231
{
167232
$response = $this->actingAsForApi(User::factory()->superuser()->create())

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

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,86 @@ public function testCheckoutToUserOnAssetUpdate()
423423
$this->assertEquals($asset->assigned_type, 'App\Models\User');
424424
}
425425

426+
public function testCheckoutToUserWithAssignedToAndAssignedType()
427+
{
428+
$asset = Asset::factory()->create();
429+
$user = User::factory()->editAssets()->create();
430+
$assigned_user = User::factory()->create();
431+
432+
$response = $this->actingAsForApi($user)
433+
->patchJson(route('api.assets.update', $asset->id), [
434+
'assigned_to' => $assigned_user->id,
435+
'assigned_type' => User::class
436+
])
437+
->assertOk()
438+
->assertStatusMessageIs('success')
439+
->json();
440+
441+
$asset->refresh();
442+
$this->assertEquals($assigned_user->id, $asset->assigned_to);
443+
$this->assertEquals($asset->assigned_type, 'App\Models\User');
444+
}
445+
446+
public function testCheckoutToUserWithAssignedToWithoutAssignedType()
447+
{
448+
$asset = Asset::factory()->create();
449+
$user = User::factory()->editAssets()->create();
450+
$assigned_user = User::factory()->create();
451+
452+
$response = $this->actingAsForApi($user)
453+
->patchJson(route('api.assets.update', $asset->id), [
454+
'assigned_to' => $assigned_user->id,
455+
// 'assigned_type' => User::class //deliberately omit assigned_type
456+
])
457+
->assertOk()
458+
->assertStatusMessageIs('error');
459+
460+
$asset->refresh();
461+
$this->assertNotEquals($assigned_user->id, $asset->assigned_to);
462+
$this->assertNotEquals($asset->assigned_type, 'App\Models\User');
463+
$this->assertNotNull($response->json('messages.assigned_type'));
464+
}
465+
466+
public function testCheckoutToUserWithAssignedToWithBadAssignedType()
467+
{
468+
$asset = Asset::factory()->create();
469+
$user = User::factory()->editAssets()->create();
470+
$assigned_user = User::factory()->create();
471+
472+
$response = $this->actingAsForApi($user)
473+
->patchJson(route('api.assets.update', $asset->id), [
474+
'assigned_to' => $assigned_user->id,
475+
'assigned_type' => 'more_deliberate_nonsense' //deliberately bad assigned_type
476+
])
477+
->assertOk()
478+
->assertStatusMessageIs('error');
479+
480+
$asset->refresh();
481+
$this->assertNotEquals($assigned_user->id, $asset->assigned_to);
482+
$this->assertNotEquals($asset->assigned_type, 'App\Models\User');
483+
$this->assertNotNull($response->json('messages.assigned_type'));
484+
}
485+
486+
public function testCheckoutToUserWithoutAssignedToWithAssignedType()
487+
{
488+
$asset = Asset::factory()->create();
489+
$user = User::factory()->editAssets()->create();
490+
$assigned_user = User::factory()->create();
491+
492+
$response = $this->actingAsForApi($user)
493+
->patchJson(route('api.assets.update', $asset->id), [
494+
//'assigned_to' => $assigned_user->id, // deliberately omit assigned_to
495+
'assigned_type' => User::class
496+
])
497+
->assertOk()
498+
->assertStatusMessageIs('error');
499+
500+
$asset->refresh();
501+
$this->assertNotEquals($assigned_user->id, $asset->assigned_to);
502+
$this->assertNotEquals($asset->assigned_type, 'App\Models\User');
503+
$this->assertNotNull($response->json('messages.assigned_to'));
504+
}
505+
426506
public function testCheckoutToDeletedUserFailsOnAssetUpdate()
427507
{
428508
$asset = Asset::factory()->create();

‎tests/Unit/AssetTest.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
use App\Models\Asset;
55
use App\Models\AssetModel;
66
use App\Models\Category;
7+
use App\Models\User;
78
use Carbon\Carbon;
89
use Tests\TestCase;
910
use App\Models\Setting;
@@ -189,4 +190,13 @@ public function testWarrantyExpiresAttribute()
189190
$this->assertEquals(Carbon::createFromDate(2019, 1, 1)->format('Y-m-d'), $asset->warranty_expires->format('Y-m-d'));
190191

191192
}
193+
194+
public function testAssignedTypeWithoutAssignTo()
195+
{
196+
$user = User::factory()->create();
197+
$asset = Asset::factory()->create([
198+
'assigned_to' => $user->id
199+
]);
200+
$this->assertModelMissing($asset);
201+
}
192202
}

0 commit comments

Comments
 (0)