From c5328a90b4a52188962495b29f3f905db917ff43 Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Sun, 21 Dec 2025 15:21:31 +0100 Subject: [PATCH] fix: settings catalog restore apply --- app/Services/Graph/GraphContractRegistry.php | 37 ++-- app/Services/Intune/RestoreService.php | 165 +++++++++--------- config/graph_contracts.php | 6 +- ...gsCatalogRestoreApplySettingsPatchTest.php | 9 +- .../Filament/SettingsCatalogRestoreTest.php | 35 ++-- ...tractRegistrySettingsWriteStrategyTest.php | 26 +++ 6 files changed, 163 insertions(+), 115 deletions(-) diff --git a/app/Services/Graph/GraphContractRegistry.php b/app/Services/Graph/GraphContractRegistry.php index 82796d9..7cf6816 100644 --- a/app/Services/Graph/GraphContractRegistry.php +++ b/app/Services/Graph/GraphContractRegistry.php @@ -103,7 +103,7 @@ public function settingsWriteMethod(string $policyType): ?string return strtoupper($method); } - public function settingsWritePath(string $policyType, string $policyId, string $settingId): ?string + public function settingsWritePath(string $policyType, string $policyId, ?string $settingId = null): ?string { $contract = $this->get($policyType); $write = $contract['settings_write'] ?? null; @@ -113,11 +113,26 @@ public function settingsWritePath(string $policyType, string $policyId, string $ return null; } - return str_replace( - ['{id}', '{settingId}'], - [urlencode($policyId), urlencode($settingId)], - $template - ); + if ($settingId === null && str_contains($template, '{settingId}')) { + return null; + } + + $path = str_replace('{id}', urlencode($policyId), $template); + + if ($settingId !== null) { + $path = str_replace('{settingId}', urlencode($settingId), $path); + } + + return $path; + } + + public function settingsWriteBodyShape(string $policyType): string + { + $contract = $this->get($policyType); + $write = $contract['settings_write'] ?? null; + $shape = is_array($write) ? ($write['body_shape'] ?? 'collection') : 'collection'; + + return is_string($shape) && $shape !== '' ? $shape : 'collection'; } /** @@ -151,19 +166,21 @@ private function sanitizeSettingsItem(array $item): array // First pass: collect information and process items foreach ($item as $key => $value) { - if (strtolower($key) === 'id') { + $normalizedKey = strtolower((string) $key); + + if ($normalizedKey === 'id') { continue; } - if ($key === '@odata.type') { + if ($normalizedKey === '@odata.type') { $existingOdataType = $value; continue; } - if ($key === 'settingInstance' && is_array($value)) { + if ($normalizedKey === 'settinginstance' && is_array($value)) { $hasSettingInstance = true; - $result[$key] = $this->preserveOdataTypesRecursively($value); + $result['settingInstance'] = $this->preserveOdataTypesRecursively($value); continue; } diff --git a/app/Services/Intune/RestoreService.php b/app/Services/Intune/RestoreService.php index 4a81102..a86f15d 100644 --- a/app/Services/Intune/RestoreService.php +++ b/app/Services/Intune/RestoreService.php @@ -406,109 +406,106 @@ private function applySettingsCatalogPolicySettings( array $context, ): array { $method = $this->contracts->settingsWriteMethod('settingsCatalogPolicy'); - $issues = []; - $applied = 0; - $failed = 0; - $manualRequired = 0; + $path = $this->contracts->settingsWritePath('settingsCatalogPolicy', $policyId); + $bodyShape = strtolower($this->contracts->settingsWriteBodyShape('settingsCatalogPolicy')); - foreach ($settings as $setting) { - if (! is_array($setting)) { - continue; - } + $buildIssues = function (string $reason) use ($settings): array { + $issues = []; - $settingId = $this->resolveSettingsCatalogSettingId($setting); - $path = ($method && $settingId) - ? $this->contracts->settingsWritePath('settingsCatalogPolicy', $policyId, $settingId) - : null; + foreach ($settings as $setting) { + if (! is_array($setting)) { + continue; + } - if (! $method || ! $path || ! $settingId) { - $manualRequired++; - $issues[] = array_filter([ - 'setting_id' => $settingId, - 'status' => 'manual_required', - 'reason' => ! $settingId - ? 'Setting id missing (cannot apply automatically).' - : 'Settings write contract is not configured (cannot apply automatically).', - ], static fn ($value) => $value !== null && $value !== ''); - - continue; - } - - $sanitized = $this->contracts->sanitizeSettingsApplyPayload('settingsCatalogPolicy', [$setting])[0] ?? null; - - if (! is_array($sanitized) || $sanitized === []) { - $manualRequired++; $issues[] = [ - 'setting_id' => $settingId, + 'setting_id' => $this->resolveSettingsCatalogSettingId($setting), 'status' => 'manual_required', - 'reason' => 'Setting payload could not be sanitized (empty payload).', + 'reason' => $reason, ]; - - continue; } - $this->graphLogger->logRequest('apply_setting', $context + [ - 'setting_id' => $settingId, - 'endpoint' => $path, - 'method' => $method, - ]); + return $issues; + }; - $response = $this->graphClient->request($method, $path, ['json' => $sanitized] + Arr::except($graphOptions, ['platform'])); + if (! $method || ! $path) { + return [ + [ + 'total' => count($settings), + 'applied' => 0, + 'failed' => 0, + 'manual_required' => count($settings), + 'issues' => $buildIssues('Settings write contract is not configured (cannot apply automatically).'), + ], + 'manual_required', + ]; + } - $this->graphLogger->logResponse('apply_setting', $response, $context + [ - 'setting_id' => $settingId, - 'endpoint' => $path, - 'method' => $method, - ]); + $sanitized = $this->contracts->sanitizeSettingsApplyPayload('settingsCatalogPolicy', $settings); - if ($response->successful()) { - $applied++; + if (! is_array($sanitized) || $sanitized === []) { + return [ + [ + 'total' => count($settings), + 'applied' => 0, + 'failed' => 0, + 'manual_required' => count($settings), + 'issues' => $buildIssues('Settings payload could not be sanitized (empty payload).'), + ], + 'manual_required', + ]; + } - continue; - } + $payload = match ($bodyShape) { + 'wrapped' => ['settings' => $sanitized], + default => $sanitized, + }; - if ($response->status === 404) { - $manualRequired++; - $issues[] = [ - 'setting_id' => $settingId, + $this->graphLogger->logRequest('apply_settings_bulk', $context + [ + 'endpoint' => $path, + 'method' => $method, + 'settings_count' => count($sanitized), + ]); + + $response = $this->graphClient->request($method, $path, ['json' => $payload] + Arr::except($graphOptions, ['platform'])); + + $this->graphLogger->logResponse('apply_settings_bulk', $response, $context + [ + 'endpoint' => $path, + 'method' => $method, + 'settings_count' => count($sanitized), + ]); + + if ($response->successful()) { + return [ + [ + 'total' => count($settings), + 'applied' => count($settings), + 'failed' => 0, + 'manual_required' => 0, + 'issues' => [], + ], + 'applied', + ]; + } + + return [ + [ + 'total' => count($settings), + 'applied' => 0, + 'failed' => 0, + 'manual_required' => count($settings), + 'issues' => [[ + 'setting_id' => null, 'status' => 'manual_required', - 'reason' => 'Setting not found on target policy (404).', + 'reason' => 'Graph bulk apply failed', + 'http_status' => $response->status, 'graph_error_message' => $response->meta['error_message'] ?? null, 'graph_error_code' => $response->meta['error_code'] ?? null, 'graph_request_id' => $response->meta['request_id'] ?? null, 'graph_client_request_id' => $response->meta['client_request_id'] ?? null, - ]; - - continue; - } - - $failed++; - $issues[] = [ - 'setting_id' => $settingId, - 'status' => 'failed', - 'reason' => 'Graph apply failed', - 'graph_error_message' => $response->meta['error_message'] ?? null, - 'graph_error_code' => $response->meta['error_code'] ?? null, - 'graph_request_id' => $response->meta['request_id'] ?? null, - 'graph_client_request_id' => $response->meta['client_request_id'] ?? null, - ]; - } - - $summary = [ - 'total' => count($settings), - 'applied' => $applied, - 'failed' => $failed, - 'manual_required' => $manualRequired, - 'issues' => $issues, + ]], + ], + 'manual_required', ]; - - $status = match (true) { - $manualRequired > 0 => 'manual_required', - $failed > 0 => 'partial', - default => 'applied', - }; - - return [$summary, $status]; } private function assertActiveContext(Tenant $tenant, BackupSet $backupSet): void diff --git a/config/graph_contracts.php b/config/graph_contracts.php index ff738c2..27fa505 100644 --- a/config/graph_contracts.php +++ b/config/graph_contracts.php @@ -63,8 +63,10 @@ ], ], 'settings_write' => [ - 'path_template' => 'deviceManagement/configurationPolicies/{id}/settings/{settingId}', - 'method' => 'PATCH', + 'path_template' => 'deviceManagement/configurationPolicies/{id}/settings', + 'method' => 'POST', + 'bulk' => true, + 'body_shape' => 'collection', ], 'update_strategy' => 'settings_catalog_policy_with_settings', ], diff --git a/tests/Feature/Filament/SettingsCatalogRestoreApplySettingsPatchTest.php b/tests/Feature/Filament/SettingsCatalogRestoreApplySettingsPatchTest.php index 8bdee78..a183d93 100644 --- a/tests/Feature/Filament/SettingsCatalogRestoreApplySettingsPatchTest.php +++ b/tests/Feature/Filament/SettingsCatalogRestoreApplySettingsPatchTest.php @@ -12,7 +12,7 @@ uses(RefreshDatabase::class); -test('settings catalog restore marks manual_required when a setting PATCH returns 404', function () { +test('settings catalog restore marks manual_required when bulk apply returns 404', function () { $policyResponse = new GraphResponse( success: true, data: [], @@ -162,13 +162,16 @@ public function request(string $method, string $path, array $options = []): Grap expect($run->status)->toBe('partial'); expect($run->results[0]['status'])->toBe('manual_required'); expect($run->results[0]['settings_apply']['manual_required'])->toBe(1); + expect($run->results[0]['settings_apply']['failed'])->toBe(0); expect($run->results[0]['settings_apply']['issues'][0]['graph_request_id'])->toBe('req-setting-404'); expect($client->applyPolicyCalls[0]['payload'])->not->toHaveKey('settings'); - expect($client->requestCalls[0]['path'])->toBe('deviceManagement/configurationPolicies/scp-3/settings/setting-404'); + expect($client->requestCalls[0]['method'])->toBe('POST'); + expect($client->requestCalls[0]['path'])->toBe('deviceManagement/configurationPolicies/scp-3/settings'); $response = $this->get(route('filament.admin.resources.restore-runs.view', ['record' => $run])); $response->assertOk(); - $response->assertSee('Setting not found on target policy (404).'); + $response->assertSee('Graph bulk apply failed'); + $response->assertSee('Setting missing'); $response->assertSee('req-setting-404'); }); diff --git a/tests/Feature/Filament/SettingsCatalogRestoreTest.php b/tests/Feature/Filament/SettingsCatalogRestoreTest.php index 29a0c35..166788e 100644 --- a/tests/Feature/Filament/SettingsCatalogRestoreTest.php +++ b/tests/Feature/Filament/SettingsCatalogRestoreTest.php @@ -77,7 +77,7 @@ public function request(string $method, string $path, array $options = []): Grap } } -test('restore marks settings catalog policy as partial when a setting PATCH fails', function () { +test('restore marks settings catalog policy as manual_required when bulk settings apply fails', function () { $policyResponse = new GraphResponse( success: true, data: [], @@ -175,8 +175,9 @@ public function request(string $method, string $path, array $options = []): Grap )->refresh(); expect($run->status)->toBe('partial'); - expect($run->results[0]['status'])->toBe('partial'); - expect($run->results[0]['settings_apply']['failed'])->toBe(1); + expect($run->results[0]['status'])->toBe('manual_required'); + expect($run->results[0]['settings_apply']['manual_required'])->toBe(1); + expect($run->results[0]['settings_apply']['failed'])->toBe(0); expect($run->results[0]['settings_apply']['issues'][0]['graph_error_message'])->toContain('settings are read-only'); expect($run->results[0]['settings_apply']['issues'][0]['graph_request_id'])->toBe('req-123'); expect($run->results[0]['settings_apply']['issues'][0]['graph_client_request_id'])->toBe('client-abc'); @@ -191,12 +192,13 @@ public function request(string $method, string $path, array $options = []): Grap expect($client->applyPolicyCalls[0]['payload'])->not->toHaveKey('settings'); expect($client->requestCalls)->toHaveCount(1); - expect($client->requestCalls[0]['method'])->toBe('PATCH'); - expect($client->requestCalls[0]['path'])->toBe('deviceManagement/configurationPolicies/scp-1/settings/setting-1'); + expect($client->requestCalls[0]['method'])->toBe('POST'); + expect($client->requestCalls[0]['path'])->toBe('deviceManagement/configurationPolicies/scp-1/settings'); expect($client->requestCalls[0]['payload'])->toBeArray(); - expect($client->requestCalls[0]['payload'])->toHaveKey('@odata.type'); - expect($client->requestCalls[0]['payload'])->not->toHaveKey('id'); - expect($client->requestCalls[0]['payload']['settingInstance']['@odata.type'])->toBe('#microsoft.graph.deviceManagementConfigurationSimpleSettingInstance'); + expect($client->requestCalls[0]['payload'][0])->toHaveKey('@odata.type'); + expect($client->requestCalls[0]['payload'][0])->not->toHaveKey('id'); + expect($client->requestCalls[0]['payload'][0]['settingInstance']['@odata.type']) + ->toBe('#microsoft.graph.deviceManagementConfigurationSimpleSettingInstance'); $response = $this ->get(route('filament.admin.resources.restore-runs.view', ['record' => $run])); @@ -292,13 +294,14 @@ public function request(string $method, string $path, array $options = []): Grap expect($client->applyPolicyCalls[0]['payload']['description'])->toBe('desc'); expect($client->requestCalls)->toHaveCount(1); - expect($client->requestCalls[0]['method'])->toBe('PATCH'); - expect($client->requestCalls[0]['path'])->toBe('deviceManagement/configurationPolicies/scp-2/settings/setting-1'); + expect($client->requestCalls[0]['method'])->toBe('POST'); + expect($client->requestCalls[0]['path'])->toBe('deviceManagement/configurationPolicies/scp-2/settings'); - // Ensure we preserved settingInstance @odata.type and stripped ids in the per-setting call - expect($client->requestCalls[0]['payload'])->toHaveKey('@odata.type'); - expect($client->requestCalls[0]['payload']['@odata.type'])->toBe('#microsoft.graph.deviceManagementConfigurationSetting'); - expect($client->requestCalls[0]['payload'])->not->toHaveKey('id'); - expect($client->requestCalls[0]['payload']['settingInstance'])->toHaveKey('@odata.type'); - expect($client->requestCalls[0]['payload']['settingInstance']['@odata.type'])->toBe('#microsoft.graph.deviceManagementConfigurationChoiceSettingInstance'); + // Ensure we preserved settingInstance @odata.type and stripped ids in the bulk call + expect($client->requestCalls[0]['payload'][0])->toHaveKey('@odata.type'); + expect($client->requestCalls[0]['payload'][0]['@odata.type'])->toBe('#microsoft.graph.deviceManagementConfigurationSetting'); + expect($client->requestCalls[0]['payload'][0])->not->toHaveKey('id'); + expect($client->requestCalls[0]['payload'][0]['settingInstance'])->toHaveKey('@odata.type'); + expect($client->requestCalls[0]['payload'][0]['settingInstance']['@odata.type']) + ->toBe('#microsoft.graph.deviceManagementConfigurationChoiceSettingInstance'); }); diff --git a/tests/Unit/GraphContractRegistrySettingsWriteStrategyTest.php b/tests/Unit/GraphContractRegistrySettingsWriteStrategyTest.php index ea41e0d..575a40a 100644 --- a/tests/Unit/GraphContractRegistrySettingsWriteStrategyTest.php +++ b/tests/Unit/GraphContractRegistrySettingsWriteStrategyTest.php @@ -20,6 +20,32 @@ ->toBe('deviceManagement/configurationPolicies/policy-1/settings/setting-9'); }); +it('returns null when settings write path requires a setting id', function () { + config()->set('graph_contracts.types.settingsCatalogPolicy', [ + 'settings_write' => [ + 'path_template' => 'deviceManagement/configurationPolicies/{id}/settings/{settingId}', + 'method' => 'PATCH', + ], + ]); + + $registry = app(GraphContractRegistry::class); + + expect($registry->settingsWritePath('settingsCatalogPolicy', 'policy-1'))->toBeNull(); +}); + +it('defaults settings write body shape to collection', function () { + config()->set('graph_contracts.types.settingsCatalogPolicy', [ + 'settings_write' => [ + 'path_template' => 'deviceManagement/configurationPolicies/{id}/settings', + 'method' => 'POST', + ], + ]); + + $registry = app(GraphContractRegistry::class); + + expect($registry->settingsWriteBodyShape('settingsCatalogPolicy'))->toBe('collection'); +}); + it('returns null when settings write contract is missing', function () { config()->set('graph_contracts.types.settingsCatalogPolicy', []);