fix: settings catalog restore apply

This commit is contained in:
Ahmed Darrazi 2025-12-21 15:21:31 +01:00
parent 41f678efe5
commit c5328a90b4
6 changed files with 163 additions and 115 deletions

View File

@ -103,7 +103,7 @@ public function settingsWriteMethod(string $policyType): ?string
return strtoupper($method); 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); $contract = $this->get($policyType);
$write = $contract['settings_write'] ?? null; $write = $contract['settings_write'] ?? null;
@ -113,11 +113,26 @@ public function settingsWritePath(string $policyType, string $policyId, string $
return null; return null;
} }
return str_replace( if ($settingId === null && str_contains($template, '{settingId}')) {
['{id}', '{settingId}'], return null;
[urlencode($policyId), urlencode($settingId)], }
$template
); $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 // First pass: collect information and process items
foreach ($item as $key => $value) { foreach ($item as $key => $value) {
if (strtolower($key) === 'id') { $normalizedKey = strtolower((string) $key);
if ($normalizedKey === 'id') {
continue; continue;
} }
if ($key === '@odata.type') { if ($normalizedKey === '@odata.type') {
$existingOdataType = $value; $existingOdataType = $value;
continue; continue;
} }
if ($key === 'settingInstance' && is_array($value)) { if ($normalizedKey === 'settinginstance' && is_array($value)) {
$hasSettingInstance = true; $hasSettingInstance = true;
$result[$key] = $this->preserveOdataTypesRecursively($value); $result['settingInstance'] = $this->preserveOdataTypesRecursively($value);
continue; continue;
} }

View File

@ -406,109 +406,106 @@ private function applySettingsCatalogPolicySettings(
array $context, array $context,
): array { ): array {
$method = $this->contracts->settingsWriteMethod('settingsCatalogPolicy'); $method = $this->contracts->settingsWriteMethod('settingsCatalogPolicy');
$path = $this->contracts->settingsWritePath('settingsCatalogPolicy', $policyId);
$bodyShape = strtolower($this->contracts->settingsWriteBodyShape('settingsCatalogPolicy'));
$buildIssues = function (string $reason) use ($settings): array {
$issues = []; $issues = [];
$applied = 0;
$failed = 0;
$manualRequired = 0;
foreach ($settings as $setting) { foreach ($settings as $setting) {
if (! is_array($setting)) { if (! is_array($setting)) {
continue; continue;
} }
$settingId = $this->resolveSettingsCatalogSettingId($setting); $issues[] = [
$path = ($method && $settingId) 'setting_id' => $this->resolveSettingsCatalogSettingId($setting),
? $this->contracts->settingsWritePath('settingsCatalogPolicy', $policyId, $settingId)
: null;
if (! $method || ! $path || ! $settingId) {
$manualRequired++;
$issues[] = array_filter([
'setting_id' => $settingId,
'status' => 'manual_required', 'status' => 'manual_required',
'reason' => ! $settingId 'reason' => $reason,
? '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; return $issues;
};
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',
];
}
$sanitized = $this->contracts->sanitizeSettingsApplyPayload('settingsCatalogPolicy', $settings);
if (! is_array($sanitized) || $sanitized === []) { if (! is_array($sanitized) || $sanitized === []) {
$manualRequired++; return [
$issues[] = [ [
'setting_id' => $settingId, 'total' => count($settings),
'status' => 'manual_required', 'applied' => 0,
'reason' => 'Setting payload could not be sanitized (empty payload).', 'failed' => 0,
'manual_required' => count($settings),
'issues' => $buildIssues('Settings payload could not be sanitized (empty payload).'),
],
'manual_required',
]; ];
continue;
} }
$this->graphLogger->logRequest('apply_setting', $context + [ $payload = match ($bodyShape) {
'setting_id' => $settingId, 'wrapped' => ['settings' => $sanitized],
default => $sanitized,
};
$this->graphLogger->logRequest('apply_settings_bulk', $context + [
'endpoint' => $path, 'endpoint' => $path,
'method' => $method, 'method' => $method,
'settings_count' => count($sanitized),
]); ]);
$response = $this->graphClient->request($method, $path, ['json' => $sanitized] + Arr::except($graphOptions, ['platform'])); $response = $this->graphClient->request($method, $path, ['json' => $payload] + Arr::except($graphOptions, ['platform']));
$this->graphLogger->logResponse('apply_setting', $response, $context + [ $this->graphLogger->logResponse('apply_settings_bulk', $response, $context + [
'setting_id' => $settingId,
'endpoint' => $path, 'endpoint' => $path,
'method' => $method, 'method' => $method,
'settings_count' => count($sanitized),
]); ]);
if ($response->successful()) { if ($response->successful()) {
$applied++; return [
[
continue;
}
if ($response->status === 404) {
$manualRequired++;
$issues[] = [
'setting_id' => $settingId,
'status' => 'manual_required',
'reason' => 'Setting not found on target policy (404).',
'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), 'total' => count($settings),
'applied' => $applied, 'applied' => count($settings),
'failed' => $failed, 'failed' => 0,
'manual_required' => $manualRequired, 'manual_required' => 0,
'issues' => $issues, 'issues' => [],
],
'applied',
]; ];
}
$status = match (true) { return [
$manualRequired > 0 => 'manual_required', [
$failed > 0 => 'partial', 'total' => count($settings),
default => 'applied', 'applied' => 0,
}; 'failed' => 0,
'manual_required' => count($settings),
return [$summary, $status]; 'issues' => [[
'setting_id' => null,
'status' => 'manual_required',
'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,
]],
],
'manual_required',
];
} }
private function assertActiveContext(Tenant $tenant, BackupSet $backupSet): void private function assertActiveContext(Tenant $tenant, BackupSet $backupSet): void

View File

@ -63,8 +63,10 @@
], ],
], ],
'settings_write' => [ 'settings_write' => [
'path_template' => 'deviceManagement/configurationPolicies/{id}/settings/{settingId}', 'path_template' => 'deviceManagement/configurationPolicies/{id}/settings',
'method' => 'PATCH', 'method' => 'POST',
'bulk' => true,
'body_shape' => 'collection',
], ],
'update_strategy' => 'settings_catalog_policy_with_settings', 'update_strategy' => 'settings_catalog_policy_with_settings',
], ],

View File

@ -12,7 +12,7 @@
uses(RefreshDatabase::class); 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( $policyResponse = new GraphResponse(
success: true, success: true,
data: [], data: [],
@ -162,13 +162,16 @@ public function request(string $method, string $path, array $options = []): Grap
expect($run->status)->toBe('partial'); expect($run->status)->toBe('partial');
expect($run->results[0]['status'])->toBe('manual_required'); expect($run->results[0]['status'])->toBe('manual_required');
expect($run->results[0]['settings_apply']['manual_required'])->toBe(1); 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($run->results[0]['settings_apply']['issues'][0]['graph_request_id'])->toBe('req-setting-404');
expect($client->applyPolicyCalls[0]['payload'])->not->toHaveKey('settings'); 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 = $this->get(route('filament.admin.resources.restore-runs.view', ['record' => $run]));
$response->assertOk(); $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'); $response->assertSee('req-setting-404');
}); });

View File

@ -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( $policyResponse = new GraphResponse(
success: true, success: true,
data: [], data: [],
@ -175,8 +175,9 @@ public function request(string $method, string $path, array $options = []): Grap
)->refresh(); )->refresh();
expect($run->status)->toBe('partial'); expect($run->status)->toBe('partial');
expect($run->results[0]['status'])->toBe('partial'); expect($run->results[0]['status'])->toBe('manual_required');
expect($run->results[0]['settings_apply']['failed'])->toBe(1); 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_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_request_id'])->toBe('req-123');
expect($run->results[0]['settings_apply']['issues'][0]['graph_client_request_id'])->toBe('client-abc'); 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->applyPolicyCalls[0]['payload'])->not->toHaveKey('settings');
expect($client->requestCalls)->toHaveCount(1); expect($client->requestCalls)->toHaveCount(1);
expect($client->requestCalls[0]['method'])->toBe('PATCH'); expect($client->requestCalls[0]['method'])->toBe('POST');
expect($client->requestCalls[0]['path'])->toBe('deviceManagement/configurationPolicies/scp-1/settings/setting-1'); expect($client->requestCalls[0]['path'])->toBe('deviceManagement/configurationPolicies/scp-1/settings');
expect($client->requestCalls[0]['payload'])->toBeArray(); expect($client->requestCalls[0]['payload'])->toBeArray();
expect($client->requestCalls[0]['payload'])->toHaveKey('@odata.type'); expect($client->requestCalls[0]['payload'][0])->toHaveKey('@odata.type');
expect($client->requestCalls[0]['payload'])->not->toHaveKey('id'); expect($client->requestCalls[0]['payload'][0])->not->toHaveKey('id');
expect($client->requestCalls[0]['payload']['settingInstance']['@odata.type'])->toBe('#microsoft.graph.deviceManagementConfigurationSimpleSettingInstance'); expect($client->requestCalls[0]['payload'][0]['settingInstance']['@odata.type'])
->toBe('#microsoft.graph.deviceManagementConfigurationSimpleSettingInstance');
$response = $this $response = $this
->get(route('filament.admin.resources.restore-runs.view', ['record' => $run])); ->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->applyPolicyCalls[0]['payload']['description'])->toBe('desc');
expect($client->requestCalls)->toHaveCount(1); expect($client->requestCalls)->toHaveCount(1);
expect($client->requestCalls[0]['method'])->toBe('PATCH'); expect($client->requestCalls[0]['method'])->toBe('POST');
expect($client->requestCalls[0]['path'])->toBe('deviceManagement/configurationPolicies/scp-2/settings/setting-1'); 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 // Ensure we preserved settingInstance @odata.type and stripped ids in the bulk call
expect($client->requestCalls[0]['payload'])->toHaveKey('@odata.type'); expect($client->requestCalls[0]['payload'][0])->toHaveKey('@odata.type');
expect($client->requestCalls[0]['payload']['@odata.type'])->toBe('#microsoft.graph.deviceManagementConfigurationSetting'); expect($client->requestCalls[0]['payload'][0]['@odata.type'])->toBe('#microsoft.graph.deviceManagementConfigurationSetting');
expect($client->requestCalls[0]['payload'])->not->toHaveKey('id'); expect($client->requestCalls[0]['payload'][0])->not->toHaveKey('id');
expect($client->requestCalls[0]['payload']['settingInstance'])->toHaveKey('@odata.type'); expect($client->requestCalls[0]['payload'][0]['settingInstance'])->toHaveKey('@odata.type');
expect($client->requestCalls[0]['payload']['settingInstance']['@odata.type'])->toBe('#microsoft.graph.deviceManagementConfigurationChoiceSettingInstance'); expect($client->requestCalls[0]['payload'][0]['settingInstance']['@odata.type'])
->toBe('#microsoft.graph.deviceManagementConfigurationChoiceSettingInstance');
}); });

View File

@ -20,6 +20,32 @@
->toBe('deviceManagement/configurationPolicies/policy-1/settings/setting-9'); ->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 () { it('returns null when settings write contract is missing', function () {
config()->set('graph_contracts.types.settingsCatalogPolicy', []); config()->set('graph_contracts.types.settingsCatalogPolicy', []);