From dcf8705e8c25c2ec7d213f38b0c6f6ef396b3749 Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Sun, 21 Dec 2025 16:19:44 +0100 Subject: [PATCH] fix: recreate settings catalog policy on unsupported settings endpoint --- .specify/plan.md | 4 +- .specify/spec.md | 3 +- .specify/tasks.md | 2 + app/Services/Graph/GraphContractRegistry.php | 11 + app/Services/Intune/RestoreService.php | 247 +++++++++++++++++- .../entries/restore-results.blade.php | 6 + .../Filament/SettingsCatalogRestoreTest.php | 112 ++++++++ 7 files changed, 381 insertions(+), 4 deletions(-) diff --git a/.specify/plan.md b/.specify/plan.md index 77109ae..2159414 100644 --- a/.specify/plan.md +++ b/.specify/plan.md @@ -24,7 +24,7 @@ ## Completed Workstreams (no new action needed) - **US1 Inventory (Phase 3)**: Filament policy listing with type/category/platform filters; tenant-scoped. - **US2 Backups (Phase 4)**: Backup sets/items in JSONB, immutable snapshots, audit logging, relation manager UX for attaching policies, soft-delete rules with restore-run guard. - **US3 Versions/Diffs (Phase 5)**: Version capture, timelines, human+JSON diffs, soft-deletes with audit. -- **US4 Restore (Phase 6)**: Preview, selective execution, conflict warnings, per-type restore level (enabled vs preview-only), PowerShell decode/encode respected, audit of outcomes. +- **US4 Restore (Phase 6)**: Preview, selective execution, conflict warnings, per-type restore level (enabled vs preview-only), PowerShell decode/encode respected, audit of outcomes; settings catalog fallback creates a new policy when the settings endpoint is unsupported, recording the new policy id and a manual cleanup warning. - **US6 Tenant Setup & Highlander (Phases 8 & 12)**: Tenant CRUD/verify, INTUNE_TENANT_ID override, `is_current` unique enforcement, “Make current” action, block deactivated tenants. - **US6 Permissions/Health (Phase 9)**: Required permissions list, compare/check service, Verify action updates status and audit, permissions panel in Tenant detail. - **US1b Settings Display (Phase 13)**: PolicyNormalizer + SnapshotValidator, warnings for malformed snapshots, normalized settings and pretty JSON on policy/version detail, list badges, README section. @@ -85,4 +85,4 @@ ### Restore Safety Gate ## Coordination - Update `.specify/tasks.md` to reflect progress on US7 wizard and future US8 contract tasks; no new entities or scope changes introduced here. - Stage validation required before production for any migration or restore-impacting change. -- Keep Graph integration behind abstraction; no secrets in logs; follow existing UX patterns (ActionGroup, warnings for risky ops). \ No newline at end of file +- Keep Graph integration behind abstraction; no secrets in logs; follow existing UX patterns (ActionGroup, warnings for risky ops). diff --git a/.specify/spec.md b/.specify/spec.md index 3b86243..bc89b99 100644 --- a/.specify/spec.md +++ b/.specify/spec.md @@ -362,6 +362,7 @@ ### Functional Requirements - **FR-020**: For PowerShell script objects (`deviceManagementScript` in `scope.supported_types`), the `scriptContent` MUST be base64-decoded when stored in backups/versions for readability/diffing and encoded again when sent back to Graph during restore. - **FR-021**: Restore behavior MUST follow the per-type configuration in `scope.restore_matrix`: `backup` determines full vs metadata-only snapshots; `restore` determines whether automated restore is enabled or preview-only; `risk` informs warning/confirmation UX. - **FR-022**: For high-risk types with `restore: preview-only` in `scope.restore_matrix` (e.g., `conditionalAccessPolicy`, `enrollmentRestriction`), TenantPilot MUST provide full backups, version history, and diffs plus detailed restore previews, but MUST NOT expose direct Graph apply actions; restore is manual, guided by the preview. +- **FR-036**: When `settingsCatalogPolicy` settings apply fails because the Graph settings endpoint is unsupported (route missing / method not allowed), the system MUST attempt a safe fallback by creating a new policy from the snapshot (including settings), record the new policy id, and mark the restore item as partial with a manual cleanup warning. ### Key Entities *(include if feature involves data)* @@ -690,4 +691,4 @@ ## Edge Cases - Token expiry mid-run → show “Please retry” + audit partial. -Previous draft archived under spechistory/spec.md \ No newline at end of file +Previous draft archived under spechistory/spec.md diff --git a/.specify/tasks.md b/.specify/tasks.md index 8e39428..f48cab7 100644 --- a/.specify/tasks.md +++ b/.specify/tasks.md @@ -710,10 +710,12 @@ ### Implementation 2. **Sanitizer:** In `GraphContractRegistry` allow and preserve `@odata.type` inside `settingInstance` and nested children (recursively); continue to strip read-only/meta fields and `id`. 3. **RestoreService:** Build `settingsPayload = sanitizeSettingsApplyPayload(snapshot['settings'])` and `POST` to the contract path; on failure mark item `manual_required` and persist Graph meta (`request_id`, `client_request_id`, error message). 4. **UI:** RestoreRun Results view shows clear admin message when `manual_required` due to settings_apply, including request ids. +5. **Fallback create:** If the settings apply call fails with route missing / method not allowed, create a new Settings Catalog policy via `POST deviceManagement/configurationPolicies` using a sanitized payload that includes the settings. Record the new policy id in restore results and mark the item as partial with a manual cleanup warning. ### Tests (Pest) - Unit: `tests/Unit/GraphContractRegistrySettingsApplySanitizerTest.php` (preserve `@odata.type`, strip ids) - Feature: `tests/Feature/Filament/SettingsCatalogRestoreApplySettingsTest.php` (mock Graph, assert POST body includes `@odata.type` and success/failure flows) +- Feature: add a restore test that simulates a settings apply route-missing error and verifies fallback policy creation + new policy id recorded. ### Verification - `./vendor/bin/pest tests/Unit/GraphContractRegistrySettingsApplySanitizerTest.php` diff --git a/app/Services/Graph/GraphContractRegistry.php b/app/Services/Graph/GraphContractRegistry.php index 4a4f79b..40b6a56 100644 --- a/app/Services/Graph/GraphContractRegistry.php +++ b/app/Services/Graph/GraphContractRegistry.php @@ -148,6 +148,17 @@ public function settingsWriteFallbackBodyShape(string $policyType): ?string return $shape; } + public function resourcePath(string $policyType): ?string + { + $resource = $this->get($policyType)['resource'] ?? null; + + if (! is_string($resource) || $resource === '') { + return null; + } + + return $resource; + } + /** * Sanitize a settings_apply payload for settingsCatalogPolicy. * Preserves `@odata.type` inside `settingInstance` and nested children while diff --git a/app/Services/Intune/RestoreService.php b/app/Services/Intune/RestoreService.php index 45d060c..993c4cb 100644 --- a/app/Services/Intune/RestoreService.php +++ b/app/Services/Intune/RestoreService.php @@ -147,6 +147,9 @@ public function execute( $settingsApply = null; $itemStatus = 'applied'; + $settings = []; + $resultReason = null; + $createdPolicyId = null; if ($item->policy_type === 'settingsCatalogPolicy') { $settings = $this->extractSettingsCatalogSettings($originalPayload); @@ -166,6 +169,37 @@ public function execute( graphOptions: $graphOptions, context: $context, ); + + if ($itemStatus === 'manual_required' && $settingsApply !== null + && $this->shouldAttemptSettingsCatalogCreate($settingsApply)) { + $createOutcome = $this->createSettingsCatalogPolicy( + originalPayload: $originalPayload, + settings: $settings, + graphOptions: $graphOptions, + context: $context, + fallbackName: $item->policy_identifier, + ); + + if ($createOutcome['success']) { + $createdPolicyId = $createOutcome['policy_id']; + $itemStatus = 'partial'; + $resultReason = 'Settings endpoint unsupported; created new policy. Manual cleanup required.'; + + if ($settingsApply !== null && $createdPolicyId) { + $settingsApply['created_policy_id'] = $createdPolicyId; + } + } elseif ($settingsApply !== null && $createOutcome['response']) { + $settingsApply['issues'][] = [ + 'setting_id' => null, + 'status' => 'manual_required', + 'reason' => 'Fallback policy create failed', + 'graph_error_message' => $createOutcome['response']->meta['error_message'] ?? null, + 'graph_error_code' => $createOutcome['response']->meta['error_code'] ?? null, + 'graph_request_id' => $createOutcome['response']->meta['request_id'] ?? null, + 'graph_client_request_id' => $createOutcome['response']->meta['client_request_id'] ?? null, + ]; + } + } } elseif ($settings !== []) { $settingsApply = [ 'total' => count($settings), @@ -221,7 +255,13 @@ public function execute( $result['settings_apply'] = $settingsApply; } - if ($itemStatus !== 'applied') { + if ($createdPolicyId) { + $result['created_policy_id'] = $createdPolicyId; + } + + if ($resultReason !== null) { + $result['reason'] = $resultReason; + } elseif ($itemStatus !== 'applied') { $result['reason'] = 'Some settings require attention'; } @@ -552,6 +592,211 @@ private function shouldRetrySettingsBulkApply(?string $errorMessage): bool || str_contains($message, 'request body'); } + private function shouldAttemptSettingsCatalogCreate(array $settingsApply): bool + { + $issues = $settingsApply['issues'] ?? []; + + foreach ($issues as $issue) { + $message = strtolower((string) ($issue['graph_error_message'] ?? $issue['reason'] ?? '')); + + if ($message === '') { + continue; + } + + if (str_contains($message, 'no odata route exists') || str_contains($message, 'no method match route template')) { + return true; + } + } + + return false; + } + + /** + * @return array{success:bool,policy_id:?string,response:?object} + */ + private function createSettingsCatalogPolicy( + array $originalPayload, + array $settings, + array $graphOptions, + array $context, + string $fallbackName, + ): array { + $resource = $this->contracts->resourcePath('settingsCatalogPolicy') ?? 'deviceManagement/configurationPolicies'; + $sanitizedSettings = $this->contracts->sanitizeSettingsApplyPayload('settingsCatalogPolicy', $settings); + + if ($sanitizedSettings === []) { + return [ + 'success' => false, + 'policy_id' => null, + 'response' => null, + ]; + } + + $payload = $this->buildSettingsCatalogCreatePayload($originalPayload, $sanitizedSettings, $fallbackName); + + $this->graphLogger->logRequest('create_settings_catalog_policy', $context + [ + 'endpoint' => $resource, + 'method' => 'POST', + 'settings_count' => count($sanitizedSettings), + ]); + + $response = $this->graphClient->request('POST', $resource, ['json' => $payload] + Arr::except($graphOptions, ['platform'])); + + $this->graphLogger->logResponse('create_settings_catalog_policy', $response, $context + [ + 'endpoint' => $resource, + 'method' => 'POST', + 'settings_count' => count($sanitizedSettings), + ]); + + $policyId = null; + if ($response->successful() && isset($response->data['id']) && is_string($response->data['id'])) { + $policyId = $response->data['id']; + } + + return [ + 'success' => $response->successful(), + 'policy_id' => $policyId, + 'response' => $response, + ]; + } + + /** + * @param array $settings + * @return array + */ + private function buildSettingsCatalogCreatePayload(array $originalPayload, array $settings, string $fallbackName): array + { + $payload = []; + $name = $this->resolvePayloadString($originalPayload, ['name', 'displayName']); + + $payload['name'] = $name ?? sprintf('Restored %s', $fallbackName); + + $description = $this->resolvePayloadString($originalPayload, ['description', 'Description']); + if ($description !== null) { + $payload['description'] = $description; + } + + $platforms = $this->resolvePayloadArray($originalPayload, ['platforms', 'Platforms']); + if ($platforms !== null) { + $payload['platforms'] = array_values($platforms); + } + + $technologies = $this->resolvePayloadArray($originalPayload, ['technologies', 'Technologies']); + if ($technologies !== null) { + $payload['technologies'] = array_values($technologies); + } + + $roleScopeTagIds = $this->resolvePayloadArray($originalPayload, ['roleScopeTagIds', 'RoleScopeTagIds']); + if ($roleScopeTagIds !== null) { + $payload['roleScopeTagIds'] = array_values($roleScopeTagIds); + } + + $templateReference = $this->resolvePayloadArray($originalPayload, ['templateReference', 'TemplateReference']); + if ($templateReference !== null) { + $payload['templateReference'] = $this->stripOdataAndReadOnly($templateReference); + } + + if ($settings !== []) { + $payload['settings'] = $settings; + } + + return $payload; + } + + /** + * @param array $payload + * @param array $keys + */ + private function resolvePayloadString(array $payload, array $keys): ?string + { + $value = $this->resolvePayloadValue($payload, $keys); + + if (! is_string($value) || trim($value) === '') { + return null; + } + + return $value; + } + + /** + * @param array $payload + * @param array $keys + * @return array|null + */ + private function resolvePayloadArray(array $payload, array $keys): ?array + { + $value = $this->resolvePayloadValue($payload, $keys); + + if (! is_array($value) || $value === []) { + return null; + } + + return $value; + } + + /** + * @param array $payload + * @param array $keys + */ + private function resolvePayloadValue(array $payload, array $keys): mixed + { + $normalized = array_map('strtolower', $keys); + + foreach ($payload as $key => $value) { + if (in_array(strtolower((string) $key), $normalized, true)) { + return $value; + } + } + + return null; + } + + /** + * @param array $payload + * @return array + */ + private function stripOdataAndReadOnly(array $payload): array + { + $clean = []; + $readOnlyKeys = ['id', 'createddatetime', 'lastmodifieddatetime', 'version']; + + foreach ($payload as $key => $value) { + $normalizedKey = strtolower((string) $key); + + if (str_starts_with($normalizedKey, '@odata')) { + continue; + } + + if (in_array($normalizedKey, $readOnlyKeys, true)) { + continue; + } + + if (is_array($value)) { + if (array_is_list($value)) { + $items = array_map(function ($item) { + if (is_array($item)) { + return $this->stripOdataAndReadOnly($item); + } + + return $item; + }, $value); + + $clean[$key] = array_values(array_filter($items, static fn ($item) => $item !== [])); + + continue; + } + + $clean[$key] = $this->stripOdataAndReadOnly($value); + + continue; + } + + $clean[$key] = $value; + } + + return $clean; + } + private function assertActiveContext(Tenant $tenant, BackupSet $backupSet): void { if (! $tenant->isActive()) { diff --git a/resources/views/filament/infolists/entries/restore-results.blade.php b/resources/views/filament/infolists/entries/restore-results.blade.php index fc19294..a9ff440 100644 --- a/resources/views/filament/infolists/entries/restore-results.blade.php +++ b/resources/views/filament/infolists/entries/restore-results.blade.php @@ -48,6 +48,12 @@ @endif + @if (! empty($item['created_policy_id'])) +
+ New policy created: {{ $item['created_policy_id'] }} (manual cleanup required). +
+ @endif + @if (! empty($item['graph_error_message']) || ! empty($item['graph_error_code']))
Graph error
diff --git a/tests/Feature/Filament/SettingsCatalogRestoreTest.php b/tests/Feature/Filament/SettingsCatalogRestoreTest.php index 63fedde..04a80c4 100644 --- a/tests/Feature/Filament/SettingsCatalogRestoreTest.php +++ b/tests/Feature/Filament/SettingsCatalogRestoreTest.php @@ -409,3 +409,115 @@ public function request(string $method, string $path, array $options = []): Grap expect($client->requestCalls[0]['payload'])->toHaveKey(0); expect($client->requestCalls[1]['payload'])->toHaveKey('settings'); }); + +test('restore creates a new policy when settings endpoint is unsupported', function () { + $policyResponse = new GraphResponse( + success: true, + data: [], + status: 200, + errors: [], + warnings: [], + meta: ['request_id' => 'req-policy', 'client_request_id' => 'client-policy'], + ); + + $settingsResponse = new GraphResponse( + success: false, + data: ['error' => ['code' => 'BadRequest', 'message' => 'No OData route exists that match template']], + status: 400, + errors: [['code' => 'BadRequest', 'message' => 'No OData route exists that match template']], + warnings: [], + meta: [ + 'error_code' => 'No method match route template', + 'error_message' => 'No OData route exists that match template', + 'request_id' => 'req-unsupported', + 'client_request_id' => 'client-unsupported', + ], + ); + + $createResponse = new GraphResponse( + success: true, + data: ['id' => 'new-policy-123'], + status: 201, + errors: [], + warnings: [], + meta: ['request_id' => 'req-create', 'client_request_id' => 'client-create'], + ); + + $client = new SettingsCatalogRestoreGraphClient($policyResponse, [$settingsResponse, $createResponse]); + + app()->instance(GraphClientInterface::class, $client); + + $tenant = Tenant::create([ + 'tenant_id' => 'tenant-5', + 'name' => 'Tenant Five', + 'metadata' => [], + ]); + + $policy = Policy::create([ + 'tenant_id' => $tenant->id, + 'external_id' => 'scp-5', + 'policy_type' => 'settingsCatalogPolicy', + 'display_name' => 'Settings Catalog Epsilon', + 'platform' => 'windows', + ]); + + $backupSet = BackupSet::create([ + 'tenant_id' => $tenant->id, + 'name' => 'Backup', + 'status' => 'completed', + 'item_count' => 1, + ]); + + $payload = [ + 'displayName' => 'Settings Catalog Epsilon', + 'platforms' => ['windows'], + 'technologies' => ['mdm'], + 'Settings' => [ + [ + 'id' => 'setting-1', + 'settingInstance' => [ + '@odata.type' => '#microsoft.graph.deviceManagementConfigurationSimpleSettingInstance', + 'settingDefinitionId' => 'test_setting', + 'simpleSettingValue' => [ + '@odata.type' => '#microsoft.graph.deviceManagementConfigurationIntegerSettingValue', + 'value' => 8, + ], + ], + ], + ], + ]; + + $backupItem = BackupItem::create([ + 'tenant_id' => $tenant->id, + 'backup_set_id' => $backupSet->id, + 'policy_id' => $policy->id, + 'policy_identifier' => $policy->external_id, + 'policy_type' => $policy->policy_type, + 'platform' => $policy->platform, + 'payload' => $payload, + ]); + + $user = User::factory()->create(); + $this->actingAs($user); + + $service = app(RestoreService::class); + $run = $service->execute( + tenant: $tenant, + backupSet: $backupSet, + selectedItemIds: [$backupItem->id], + dryRun: false, + actorEmail: $user->email, + actorName: $user->name, + )->refresh(); + + expect($run->status)->toBe('partial'); + expect($run->results[0]['status'])->toBe('partial'); + expect($run->results[0]['created_policy_id'])->toBe('new-policy-123'); + expect($run->results[0]['settings_apply']['created_policy_id'])->toBe('new-policy-123'); + + expect($client->requestCalls)->toHaveCount(2); + expect($client->requestCalls[0]['path'])->toBe('deviceManagement/configurationPolicies/scp-5/settings'); + expect($client->requestCalls[1]['path'])->toBe('deviceManagement/configurationPolicies'); + expect($client->requestCalls[1]['payload'])->toHaveKey('settings'); + expect($client->requestCalls[1]['payload'])->toHaveKey('name'); +});