fix: recreate settings catalog policy on unsupported settings endpoint
This commit is contained in:
Ahmed Darrazi
parent
cc1c5e6dd4
commit
dcf8705e8c
@ -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).
|
||||
- Keep Graph integration behind abstraction; no secrets in logs; follow existing UX patterns (ActionGroup, warnings for risky ops).
|
||||
|
||||
@ -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
|
||||
Previous draft archived under spechistory/spec.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`
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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<int, mixed> $settings
|
||||
* @return array<string, mixed>
|
||||
*/
|
||||
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<string, mixed> $payload
|
||||
* @param array<int, string> $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<string, mixed> $payload
|
||||
* @param array<int, string> $keys
|
||||
* @return array<int, mixed>|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<string, mixed> $payload
|
||||
* @param array<int, string> $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<string, mixed> $payload
|
||||
* @return array<string, mixed>
|
||||
*/
|
||||
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()) {
|
||||
|
||||
@ -48,6 +48,12 @@
|
||||
</div>
|
||||
@endif
|
||||
|
||||
@if (! empty($item['created_policy_id']))
|
||||
<div class="mt-2 text-xs text-amber-800">
|
||||
New policy created: {{ $item['created_policy_id'] }} (manual cleanup required).
|
||||
</div>
|
||||
@endif
|
||||
|
||||
@if (! empty($item['graph_error_message']) || ! empty($item['graph_error_code']))
|
||||
<div class="mt-2 rounded border border-amber-200 bg-amber-50 px-2 py-1 text-xs text-amber-900">
|
||||
<div class="font-semibold">Graph error</div>
|
||||
|
||||
@ -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');
|
||||
});
|
||||
|
||||
Loading…
Reference in New Issue
Block a user