diff --git a/app/Http/Controllers/Accessories/AccessoriesController.php b/app/Http/Controllers/Accessories/AccessoriesController.php index cc19a704d0..82b9f077e2 100755 --- a/app/Http/Controllers/Accessories/AccessoriesController.php +++ b/app/Http/Controllers/Accessories/AccessoriesController.php @@ -159,11 +159,15 @@ class AccessoriesController extends Controller // create-form input names so enrichInitialOrderFromRequest // in store() picks them up on save and writes them onto the // observer-created initial Order + OrderItem for the new row. - foreach ($accessory->lastOrderPrefill() as $field => $value) { - if ($value !== null) { - $cloned->{$field} = $value; - } - } + // Explicit assignments (not a foreach) so each typed value from + // lastOrderPrefill() lands on the matching typed model property + // without going through a mixed intermediate that would fail + // larastan's assign.propertyType check. + $prefill = $accessory->lastOrderPrefill(); + $cloned->supplier_id = $prefill['supplier_id']; + $cloned->purchase_date = $prefill['purchase_date']; + $cloned->purchase_cost = $prefill['purchase_cost']; + $cloned->order_number = $prefill['order_number']; return view('accessories/edit') ->with('cloned_model', $accessory_to_clone) diff --git a/app/Http/Controllers/Components/ComponentsController.php b/app/Http/Controllers/Components/ComponentsController.php index 38c1947f37..2bafa01851 100644 --- a/app/Http/Controllers/Components/ComponentsController.php +++ b/app/Http/Controllers/Components/ComponentsController.php @@ -252,14 +252,13 @@ class ComponentsController extends Controller $cloned_component->id = null; $cloned_component->deleted_at = null; - // See AccessoriesController::getClone — same rationale for - // carrying the source item's most recent acquisition context - // onto the cloned create form. - foreach ($component->lastOrderPrefill() as $field => $value) { - if ($value !== null) { - $cloned_component->{$field} = $value; - } - } + // See AccessoriesController::getClone for the rationale, including + // the note on why these are explicit assignments not a foreach. + $prefill = $component->lastOrderPrefill(); + $cloned_component->supplier_id = $prefill['supplier_id']; + $cloned_component->purchase_date = $prefill['purchase_date']; + $cloned_component->purchase_cost = $prefill['purchase_cost']; + $cloned_component->order_number = $prefill['order_number']; // Show the page return view('components/edit') diff --git a/app/Http/Controllers/Consumables/ConsumablesController.php b/app/Http/Controllers/Consumables/ConsumablesController.php index 4cb328c9c3..ed43e655d7 100644 --- a/app/Http/Controllers/Consumables/ConsumablesController.php +++ b/app/Http/Controllers/Consumables/ConsumablesController.php @@ -255,14 +255,13 @@ class ConsumablesController extends Controller $consumable->id = null; $consumable->created_by = null; - // See AccessoriesController::getClone — same rationale for - // carrying the source item's most recent acquisition context - // onto the cloned create form. - foreach ($consumable_to_close->lastOrderPrefill() as $field => $value) { - if ($value !== null) { - $consumable->{$field} = $value; - } - } + // See AccessoriesController::getClone for the rationale, including + // the note on why these are explicit assignments not a foreach. + $prefill = $consumable_to_close->lastOrderPrefill(); + $consumable->supplier_id = $prefill['supplier_id']; + $consumable->purchase_date = $prefill['purchase_date']; + $consumable->purchase_cost = $prefill['purchase_cost']; + $consumable->order_number = $prefill['order_number']; return view('consumables/edit') ->with('cloned_model', $consumable_to_close) diff --git a/app/Models/SnipeSCIMConfig.php b/app/Models/SnipeSCIMConfig.php index c1c8800c23..94e5a2c54f 100644 --- a/app/Models/SnipeSCIMConfig.php +++ b/app/Models/SnipeSCIMConfig.php @@ -255,8 +255,35 @@ class SnipeMutableCollection extends MutableCollection // stash the object into the request so the displayName uniqueness closure // (which re-runs after mapping) can recognize its own row instead of // treating it as an existing name collision. + // + // Missing-value guard: the per-member `required` rule that used to live on + // the SCIM config was dropped because it caused ValidationRuleParser to + // allocate O(N) rule stacks on the flattened payload, which OOMed on + // large group syncs (see the docblock above the members mapping in + // SnipeSCIMConfig::getGroupConfig). The check now happens here in a + // single walk so clients get a clean 400 pointing at the bad indices + // instead of the parent library's misleading 500 with an empty + // "One or more members are unknown: " message from findMany() eating + // the nulls. public function add($value, Model &$object) { + $missing = []; + foreach ((array) $value as $index => $entry) { + if (!is_array($entry) + || !array_key_exists('value', $entry) + || $entry['value'] === null + || $entry['value'] === '' + ) { + $missing[] = $index; + } + } + if ($missing !== []) { + throw new SCIMException( + 'Every members entry must include a "value" field. Missing at indices: ' . implode(',', $missing), + 400 + ); + } + if (! $object->exists) { $object->save(); request()->attributes->set('scim_in_flight_resource', $object); @@ -413,19 +440,19 @@ class SCIMMultiCompanyArray extends Attribute public function add($value, Model &$object) { - \Log::debug("MC ADD VALUE IS: " . print_r($value, true)); + \Log::debug('MC ADD VALUE IS: ' . print_r($value, true)); $this->applyCompanies($value, $object); } public function replace($value, Model &$object, $path = null, $removeIfNotSet = false) { - \Log::debug("MC REPLACE VALUE IS: " . print_r($value, true)); + \Log::debug('MC REPLACE VALUE IS: ' . print_r($value, true)); $this->applyCompanies($value, $object); } public function patch($operation, $value, Model &$object, ?Path $path = null, $removeIfNotSet = false) { - \Log::debug("MC PATCH VALUE IS: " . print_r($value, true)); + \Log::debug('MC PATCH VALUE IS: ' . print_r($value, true)); $this->applyCompanies($value, $object); } } @@ -710,7 +737,7 @@ class SnipeSCIMConfig } else { // Okta hits this one for creating a user - it does a full PUT for their ID \Log::debug("GetValuePAthFilter is null for path: $path"); - \Log::debug("GetValuePathFilter is now null and trying to set value of: " . print_r($value, true)); + \Log::debug('GetValuePathFilter is now null and trying to set value of: ' . print_r($value, true)); // the Addresses object is a 'list' (array with numeric indices) by definition... if (is_array($value) && array_is_list($value)) { foreach ($value as $address) { @@ -718,18 +745,18 @@ class SnipeSCIMConfig if (@$address['type'] == 'work') { foreach ($address as $key => $v) { if (array_key_exists($key, self::$addressmap)) { - \Log::debug("Addresses: Setting " . self::$addressmap[$key] . " to '$v'"); + \Log::debug('Addresses: Setting ' . self::$addressmap[$key] . " to '$v'"); $object->{self::$addressmap[$key]} = $v; } } } else { - //should we throw if you give us a 'home' address? I don't know. + // should we throw if you give us a 'home' address? I don't know. // what if you gave us _both_ ? } } } else { - \Log::debug("Unknown Address Object: " . print_r($value, true)); - throw new SCIMException("Unknown Address object of type: " . gettype($value), 422); + \Log::debug('Unknown Address Object: ' . print_r($value, true)); + throw new SCIMException('Unknown Address object of type: ' . gettype($value), 422); } } } @@ -892,8 +919,18 @@ class SnipeSCIMConfig } $fail('The name has already been taken.'); }), + // The per-member `required` rule on `value` used to live + // on the eloquent() below. Removed intentionally: Laravel's + // ValidationRuleParser::mergeRulesForAttribute allocates one + // rule stack per attribute path in the flattened payload, so + // an incoming members array of N entries produced O(N) rule + // stacks and blew the PHP memory_limit on large group syncs + // The per-member value check now lives inside + // SnipeMutableCollection::add() as one array walk, and the + // parent ensure() below adds a `max:` guardrail so a truly + // runaway payload still gets rejected with a clean 400. (new SnipeMutableCollection('members'))->withSubAttributes( - eloquent('value', 'id')->ensure('required'), + eloquent('value', 'id'), (new class('$ref') extends Eloquent { protected function doRead(&$object, $attributes = []) @@ -908,7 +945,7 @@ class SnipeSCIMConfig } }), eloquent('display', 'name') - )->ensure('nullable', 'array') + )->ensure('nullable', 'array', 'max:200000') ) ), ]; diff --git a/app/Models/Traits/HasOrders.php b/app/Models/Traits/HasOrders.php index 3d372f0f7e..4cbbc16998 100644 --- a/app/Models/Traits/HasOrders.php +++ b/app/Models/Traits/HasOrders.php @@ -143,9 +143,7 @@ trait HasOrders * Prefill values for the create / clone form's initial-acquisition * fields. Distinct from lastOrderDefaults() because this shape * includes `order_number` (per-shipment, not a "default" concept) - * and matches the request keys the create form posts back, so a - * controller can loop the return array to assign values directly - * onto a cloned model's attributes. + * and matches the request keys the create form posts back. * * Used by getClone() on Accessories / Consumables / Components to * carry the source item's most recent acquisition context onto the @@ -154,26 +152,35 @@ trait HasOrders * / price for a fast restock. Items with no order history return * an all-null array. * + * Returns native types (Carbon for purchase_date, float for + * purchase_cost) so getClone() can assign directly onto typed + * model properties without a coercion step and without tripping + * larastan's assign.propertyType on the cast-inferred property + * signatures. The @var below is needed because MorphMany::first() + * infers to Model|null; the annotation resolves $line to OrderItem + * so `->order` and `->price` accesses type through cleanly rather + * than falling into the Model::$order baseline ignore bucket. + * * @return array{ * supplier_id: ?int, - * purchase_date: ?string, - * purchase_cost: ?string, + * purchase_date: ?\Carbon\Carbon, + * purchase_cost: ?float, * order_number: ?string, * } */ public function lastOrderPrefill(): array { + /** @var \App\Models\OrderItem|null $line */ $line = $this->orderItems() ->with('order:id,order_number,supplier_id,purchase_date') ->latest('id') ->first(); - $order = $line?->order; return [ - 'supplier_id' => $order?->supplier_id, - 'purchase_date' => $order?->purchase_date?->toDateString(), - 'purchase_cost' => $line?->price !== null ? (string) $line->price : null, - 'order_number' => $order?->order_number, + 'supplier_id' => $line?->order?->supplier_id, + 'purchase_date' => $line?->order?->purchase_date, + 'purchase_cost' => $line?->price !== null ? (float) $line->price : null, + 'order_number' => $line?->order?->order_number, ]; } diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index b6051639a0..02dab4bf9a 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -5724,16 +5724,28 @@ parameters: count: 6 path: app/Models/Accessory.php + - + message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$order_number\.$#' + identifier: property.notFound + count: 1 + path: app/Models/Accessory.php + - message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$price\.$#' identifier: property.notFound count: 1 path: app/Models/Accessory.php + - + message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$purchase_date\.$#' + identifier: property.notFound + count: 1 + path: app/Models/Accessory.php + - message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$supplier_id\.$#' identifier: property.notFound - count: 1 + count: 2 path: app/Models/Accessory.php - @@ -6018,16 +6030,28 @@ parameters: count: 6 path: app/Models/Asset.php + - + message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$order_number\.$#' + identifier: property.notFound + count: 1 + path: app/Models/Asset.php + - message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$price\.$#' identifier: property.notFound count: 1 path: app/Models/Asset.php + - + message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$purchase_date\.$#' + identifier: property.notFound + count: 1 + path: app/Models/Asset.php + - message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$supplier_id\.$#' identifier: property.notFound - count: 1 + count: 2 path: app/Models/Asset.php - @@ -6930,16 +6954,28 @@ parameters: count: 6 path: app/Models/Component.php + - + message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$order_number\.$#' + identifier: property.notFound + count: 1 + path: app/Models/Component.php + - message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$price\.$#' identifier: property.notFound count: 1 path: app/Models/Component.php + - + message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$purchase_date\.$#' + identifier: property.notFound + count: 1 + path: app/Models/Component.php + - message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$supplier_id\.$#' identifier: property.notFound - count: 1 + count: 2 path: app/Models/Component.php - @@ -7140,16 +7176,28 @@ parameters: count: 6 path: app/Models/Consumable.php + - + message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$order_number\.$#' + identifier: property.notFound + count: 1 + path: app/Models/Consumable.php + - message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$price\.$#' identifier: property.notFound count: 1 path: app/Models/Consumable.php + - + message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$purchase_date\.$#' + identifier: property.notFound + count: 1 + path: app/Models/Consumable.php + - message: '#^Access to an undefined property Illuminate\\Database\\Eloquent\\Model\:\:\$supplier_id\.$#' identifier: property.notFound - count: 1 + count: 2 path: app/Models/Consumable.php - diff --git a/routes/web.php b/routes/web.php index 1f08bf25a6..47337ef12d 100644 --- a/routes/web.php +++ b/routes/web.php @@ -100,16 +100,6 @@ Route::middleware(['web', 'auth', 'authorize:superuser'])->prefix('oauth')->grou }); Route::group(['middleware' => 'auth'], function () { - /* - * Calendar (unified view across every HasCalendarEvents source). - * Companion API endpoint lives at /api/v1/calendar/events. - */ - Route::get('calendar', [App\Http\Controllers\CalendarEventsController::class, 'index']) - ->name('calendar.index') - ->breadcrumbs(fn (Tabuna\Breadcrumbs\Trail $trail) => $trail->parent('home') - ->push(trans('general.calendar'), route('calendar.index')) - ); - /* * Companies */ diff --git a/tests/Feature/Scim/CreateGroupWithMembersTest.php b/tests/Feature/Scim/CreateGroupWithMembersTest.php index 17f9d07a8b..9518ef9bb6 100644 --- a/tests/Feature/Scim/CreateGroupWithMembersTest.php +++ b/tests/Feature/Scim/CreateGroupWithMembersTest.php @@ -65,4 +65,67 @@ class CreateGroupWithMembersTest extends TestCase $response->assertStatus(201); $this->assertDatabaseHas('permission_groups', ['name' => 'SCIM Group No Members']); } + + public function test_post_group_with_many_members_does_not_explode_validator() + { + // Regression for the SCIM group-sync OOM at + // ValidationRuleParser::mergeRulesForAttribute:227. Previously the + // per-member `required` rule on `value` caused Laravel to allocate + // one rule stack per member entry in the flattened payload, so a + // 101k-member group sync blew a 256MB PHP process before any code + // ran. Rule was dropped from SnipeSCIMConfig::getGroupConfig and + // the check moved into SnipeMutableCollection::add so the validator + // does O(1) work on the members array regardless of size. + // + // 25 members here is a proxy for the customer's much larger sync. + // If the O(N) rule explosion regresses, larger integration + // environments would OOM again; a per-item explosion at N=25 is + // fine on 256MB but any code path that reintroduces it is caught + // by the mechanism-level assertion in + // GroupMembersValidationShapeTest. + Passport::actingAs(User::factory()->superuser()->create()); + + $members = User::factory()->count(25)->create(); + + $response = $this->postJson('/scim/v2/Groups', [ + 'schemas' => ['urn:ietf:params:scim:schemas:core:2.0:Group'], + 'displayName' => 'SCIM Group Many Members', + 'members' => $members->map(fn ($u) => ['value' => $u->id])->all(), + ]); + + $response->assertStatus(201); + $group = Group::where('name', 'SCIM Group Many Members')->firstOrFail(); + $this->assertSame(25, DB::table('users_groups')->where('group_id', $group->id)->count()); + } + + public function test_post_group_with_member_missing_value_returns_400_with_indices() + { + // Regression for the parent library's 500 with an empty + // "One or more members are unknown: " message when a members entry + // arrives without its `value` field. SnipeMutableCollection::add + // now catches this at attach time with a 400 that names the + // offending indices, matching how bad request bodies are surfaced + // elsewhere in the SCIM stack (SnipeRootComplex::add/replace also + // route malformed keys through 400s). + Passport::actingAs(User::factory()->superuser()->create()); + + $goodMember = User::factory()->create(); + + $response = $this->postJson('/scim/v2/Groups', [ + 'schemas' => ['urn:ietf:params:scim:schemas:core:2.0:Group'], + 'displayName' => 'SCIM Group Bad Member', + 'members' => [ + ['value' => $goodMember->id], + ['display' => 'no value key here'], + ['value' => null], + ], + ]); + + $response->assertStatus(400); + $body = $response->json(); + $this->assertStringContainsString('Every members entry must include', json_encode($body)); + // Both offending entries by their indices in the payload. + $this->assertStringContainsString('1', json_encode($body)); + $this->assertStringContainsString('2', json_encode($body)); + } }