Skip to content

Commit 3a359bf

Browse files
snipesnipe
committed
SCIM: Fixed RB-21807 (SCIM shape parsing error)
1 parent 99ea2d3 commit 3a359bf

3 files changed

Lines changed: 202 additions & 0 deletions

File tree

‎app/Models/SnipeSCIMConfig.php‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,18 +285,53 @@ protected function doRead(&$object, $attributes = [])
285285

286286
public function add($value, Model &$object)
287287
{
288+
$value = $this->coerceScalar($value);
288289
$object->{$this->relationship_id_field} = $value ? $this->relationship_class::firstOrCreate([$this->relationship_field => $value])->id : null;
289290
}
290291

291292
public function replace($value, Model &$object, $path = null, $removeIfNotSet = false)
292293
{
294+
$value = $this->coerceScalar($value);
293295
$object->{$this->relationship_id_field} = $value ? $this->relationship_class::firstOrCreate([$this->relationship_field => $value])->id : null;
294296
}
295297

296298
public function patch($operation, $value, Model &$object, ?Path $path = null, $removeIfNotSet = false)
297299
{
300+
$value = $this->coerceScalar($value);
298301
$object->{$this->relationship_id_field} = $value ? $this->relationship_class::firstOrCreate([$this->relationship_field => $value])->id : null;
299302
}
303+
304+
// SCIM clients may send scalar-mapped attributes like `department`
305+
// and `location` as complex objects — {"value": "Engineering"} or
306+
// {"displayName": "Engineering"} — and SnipeRootComplex::replace()
307+
// additionally wraps sub-attribute values ({"remaining.path" => v})
308+
// when descending. MappedTable is a leaf that maps ONE relationship
309+
// field, so anything arriving as an array must be unwrapped before
310+
// firstOrCreate() gets it — otherwise Grammar::parameterize()
311+
// throws when the WHERE binding receives an array. Prefer the SCIM
312+
// conventional keys "value" and "displayName", then fall back to
313+
// any scalar leaf; return null if no usable value exists so the
314+
// caller nulls the relationship (matching the empty-string branch).
315+
private function coerceScalar($value)
316+
{
317+
if (! is_array($value)) {
318+
return $value;
319+
}
320+
321+
foreach (['value', 'displayName'] as $key) {
322+
if (isset($value[$key]) && is_scalar($value[$key])) {
323+
return $value[$key];
324+
}
325+
}
326+
327+
foreach ($value as $v) {
328+
if (is_scalar($v) && $v !== '') {
329+
return $v;
330+
}
331+
}
332+
333+
return null;
334+
}
300335
}
301336

302337
// Company is stored only in the company_user pivot, not company_id. Read from the pivot

‎database/migrations/2026_07_29_220000_make_departments_created_by_nullable.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?php
2+
3+
use Illuminate\Database\Migrations\Migration;
4+
use Illuminate\Database\Schema\Blueprint;
5+
use Illuminate\Support\Facades\Schema;
6+
7+
8+
return new class extends Migration
9+
{
10+
public function up(): void
11+
{
12+
Schema::table('departments', function (Blueprint $table) {
13+
$table->integer('created_by')->nullable()->change();
14+
});
15+
}
16+
17+
public function down(): void
18+
{
19+
Schema::table('departments', function (Blueprint $table) {
20+
$table->integer('created_by')->nullable(false)->change();
21+
});
22+
}
23+
};

‎tests/Feature/Scim/MappedTableComplexValueTest.php‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
<?php
2+
3+
namespace Tests\Feature\Scim;
4+
5+
use App\Models\Department;
6+
use App\Models\User;
7+
use Laravel\Passport\Passport;
8+
use Tests\TestCase;
9+
10+
/**
11+
* Regression for a Rollbar 500 on /scim/v2/Users/{id}:
12+
*
13+
* TypeError Illuminate\Database\Grammar::parameterize():
14+
* Argument #1 ($values) must be of type array, string given
15+
* at MappedTable::replace (SnipeSCIMConfig.php:293)
16+
*
17+
* MappedTable is a leaf attribute for the User schema's `department`
18+
* and `location` fields — both scalar-mapped foreign relations.
19+
* SCIM clients (Entra, Okta in some configurations) send those as
20+
* complex objects: {"department": {"value": "Engineering"}} or
21+
* {"department": {"displayName": "Engineering"}}. The parent
22+
* SnipeRootComplex router also wraps sub-attribute values into an
23+
* array on the descent. Either way, MappedTable was receiving an
24+
* array in a code path that unconditionally passed it to
25+
* firstOrCreate() as a WHERE value — triggering the Grammar TypeError.
26+
*
27+
* The fix coerces array inputs to their SCIM-standard scalar leaf
28+
* (`value` / `displayName`, then any first scalar) before hitting
29+
* firstOrCreate.
30+
*/
31+
class MappedTableComplexValueTest extends TestCase
32+
{
33+
public function test_put_with_department_as_complex_value_object_does_not_crash(): void
34+
{
35+
Passport::actingAs(User::factory()->superuser()->create());
36+
$target = User::factory()->create();
37+
38+
$response = $this->putJson('/scim/v2/Users/'.$target->id, [
39+
'schemas' => ['urn:ietf:params:scim:schemas:core:2.0:User'],
40+
'userName' => $target->username,
41+
'department' => ['value' => 'Engineering'],
42+
]);
43+
44+
$response->assertStatus(200);
45+
$this->assertDatabaseHas('departments', ['name' => 'Engineering']);
46+
$target->refresh();
47+
$this->assertSame('Engineering', $target->department->name);
48+
}
49+
50+
public function test_put_with_department_as_display_name_object_does_not_crash(): void
51+
{
52+
Passport::actingAs(User::factory()->superuser()->create());
53+
$target = User::factory()->create();
54+
55+
$response = $this->putJson('/scim/v2/Users/'.$target->id, [
56+
'schemas' => ['urn:ietf:params:scim:schemas:core:2.0:User'],
57+
'userName' => $target->username,
58+
'department' => ['displayName' => 'Product'],
59+
]);
60+
61+
$response->assertStatus(200);
62+
$target->refresh();
63+
$this->assertSame('Product', $target->department->name);
64+
}
65+
66+
public function test_put_with_location_as_complex_value_object_does_not_crash(): void
67+
{
68+
Passport::actingAs(User::factory()->superuser()->create());
69+
$target = User::factory()->create();
70+
71+
$response = $this->putJson('/scim/v2/Users/'.$target->id, [
72+
'schemas' => ['urn:ietf:params:scim:schemas:core:2.0:User'],
73+
'userName' => $target->username,
74+
'location' => ['value' => 'HQ'],
75+
]);
76+
77+
$response->assertStatus(200);
78+
$this->assertDatabaseHas('locations', ['name' => 'HQ']);
79+
$target->refresh();
80+
$this->assertSame('HQ', $target->location->name);
81+
}
82+
83+
public function test_put_with_scalar_department_still_works(): void
84+
{
85+
// Non-regression guard: the historical scalar shape must
86+
// continue to work unchanged.
87+
Passport::actingAs(User::factory()->superuser()->create());
88+
$target = User::factory()->create();
89+
90+
$response = $this->putJson('/scim/v2/Users/'.$target->id, [
91+
'schemas' => ['urn:ietf:params:scim:schemas:core:2.0:User'],
92+
'userName' => $target->username,
93+
'department' => 'Support',
94+
]);
95+
96+
$response->assertStatus(200);
97+
$target->refresh();
98+
$this->assertSame('Support', $target->department->name);
99+
}
100+
101+
public function test_patch_replace_with_complex_department_value_does_not_crash(): void
102+
{
103+
// PATCH shape from Entra / Okta: op=replace, no top-level
104+
// path, complex value under the attribute key.
105+
Passport::actingAs(User::factory()->superuser()->create());
106+
$target = User::factory()->create();
107+
108+
$response = $this->patchJson('/scim/v2/Users/'.$target->id, [
109+
'schemas' => ['urn:ietf:params:scim:api:messages:2.0:PatchOp'],
110+
'Operations' => [
111+
[
112+
'op' => 'replace',
113+
'value' => [
114+
'department' => ['value' => 'Finance'],
115+
],
116+
],
117+
],
118+
]);
119+
120+
$response->assertStatus(200);
121+
$target->refresh();
122+
$this->assertSame('Finance', $target->department->name);
123+
}
124+
125+
public function test_put_with_empty_complex_department_object_nulls_relationship(): void
126+
{
127+
// Clients sometimes send {} to clear a relationship. The
128+
// coercion falls through to null, which the caller uses to
129+
// null the foreign-key column. No crash, no orphan row.
130+
Passport::actingAs(User::factory()->superuser()->create());
131+
$existingDepartment = Department::factory()->create();
132+
$target = User::factory()->create(['department_id' => $existingDepartment->id]);
133+
134+
$response = $this->putJson('/scim/v2/Users/'.$target->id, [
135+
'schemas' => ['urn:ietf:params:scim:schemas:core:2.0:User'],
136+
'userName' => $target->username,
137+
'department' => new \stdClass, // empty object → [] on decode
138+
]);
139+
140+
$response->assertStatus(200);
141+
$target->refresh();
142+
$this->assertNull($target->department_id);
143+
}
144+
}

0 commit comments

Comments
 (0)