diff --git a/app/Filament/Resources/OperationRunResource.php b/app/Filament/Resources/OperationRunResource.php index 355f0f5..f3f7eb0 100644 --- a/app/Filament/Resources/OperationRunResource.php +++ b/app/Filament/Resources/OperationRunResource.php @@ -58,6 +58,11 @@ public static function infolist(Schema $schema): Schema ->badge() ->color(fn (OperationRun $record): string => static::outcomeColor($record->outcome)), TextEntry::make('initiator_name')->label('Initiator'), + TextEntry::make('target_scope_display') + ->label('Target') + ->getStateUsing(fn (OperationRun $record): ?string => static::targetScopeDisplay($record)) + ->visible(fn (OperationRun $record): bool => static::targetScopeDisplay($record) !== null) + ->columnSpanFull(), TextEntry::make('elapsed') ->label('Elapsed') ->getStateUsing(fn (OperationRun $record): string => RunDurationInsights::elapsedHuman($record)), @@ -273,4 +278,42 @@ private static function outcomeColor(?string $outcome): string default => 'gray', }; } + + private static function targetScopeDisplay(OperationRun $record): ?string + { + $context = is_array($record->context) ? $record->context : []; + + $targetScope = $context['target_scope'] ?? null; + if (! is_array($targetScope)) { + return null; + } + + $entraTenantName = $targetScope['entra_tenant_name'] ?? null; + $entraTenantId = $targetScope['entra_tenant_id'] ?? null; + $directoryContextId = $targetScope['directory_context_id'] ?? null; + + $entraTenantName = is_string($entraTenantName) ? trim($entraTenantName) : null; + $entraTenantId = is_string($entraTenantId) ? trim($entraTenantId) : null; + + $directoryContextId = match (true) { + is_string($directoryContextId) => trim($directoryContextId), + is_int($directoryContextId) => (string) $directoryContextId, + default => null, + }; + + $entra = null; + + if ($entraTenantName !== null && $entraTenantName !== '') { + $entra = $entraTenantId ? "{$entraTenantName} ({$entraTenantId})" : $entraTenantName; + } elseif ($entraTenantId !== null && $entraTenantId !== '') { + $entra = $entraTenantId; + } + + $parts = array_values(array_filter([ + $entra, + $directoryContextId ? "directory_context_id: {$directoryContextId}" : null, + ], fn (?string $value): bool => $value !== null && $value !== '')); + + return $parts !== [] ? implode(' · ', $parts) : null; + } } diff --git a/app/Services/Graph/MicrosoftGraphClient.php b/app/Services/Graph/MicrosoftGraphClient.php index 094fe16..78ffad5 100644 --- a/app/Services/Graph/MicrosoftGraphClient.php +++ b/app/Services/Graph/MicrosoftGraphClient.php @@ -626,7 +626,24 @@ private function send(string $method, string $path, array $options = [], array $ $pending = Http::baseUrl($this->baseUrl) ->acceptJson() ->timeout($this->timeout) - ->retry($this->retryTimes, $this->retrySleepMs) + ->retry( + $this->retryTimes, + fn (int $attempt, Throwable $exception): int => $this->retryDelayMs($attempt), + function (Throwable $exception): bool { + if ($exception instanceof ConnectionException) { + return true; + } + + if ($exception instanceof RequestException) { + $status = $exception->response?->status(); + + return in_array($status, [429, 503], true); + } + + return false; + }, + throw: true, + ) ->withToken($token) ->withHeaders([ 'client-request-id' => $clientRequestId, @@ -667,6 +684,23 @@ private function send(string $method, string $path, array $options = [], array $ return $response; } + private function retryDelayMs(int $attempt): int + { + $baseMs = max(0, $this->retrySleepMs); + + if ($attempt <= 1 || $baseMs === 0) { + return $baseMs; + } + + $exponential = $baseMs * (2 ** ($attempt - 1)); + $capped = min($exponential, 5000); + + $jitterMax = (int) floor($capped * 0.2); + $jitter = $jitterMax > 0 ? random_int(0, $jitterMax) : 0; + + return $capped + $jitter; + } + private function toGraphResponse(string $action, Response $response, callable $transform, array $meta = [], array $warnings = []): GraphResponse { $json = $response->json() ?? []; diff --git a/app/Services/OperationRunService.php b/app/Services/OperationRunService.php index 73ef901..7df994a 100644 --- a/app/Services/OperationRunService.php +++ b/app/Services/OperationRunService.php @@ -531,7 +531,7 @@ protected function isListArray(array $array): bool /** * @param array $failures - * @return array + * @return array */ protected function sanitizeFailures(array $failures): array { @@ -539,10 +539,12 @@ protected function sanitizeFailures(array $failures): array foreach ($failures as $failure) { $code = (string) ($failure['code'] ?? 'unknown'); + $reasonCode = (string) ($failure['reason_code'] ?? $code); $message = (string) ($failure['message'] ?? ''); $sanitized[] = [ 'code' => $this->sanitizeFailureCode($code), + 'reason_code' => RunFailureSanitizer::normalizeReasonCode($reasonCode), 'message' => $this->sanitizeMessage($message), ]; } diff --git a/app/Support/OpsUx/RunFailureSanitizer.php b/app/Support/OpsUx/RunFailureSanitizer.php index 105c742..cba491e 100644 --- a/app/Support/OpsUx/RunFailureSanitizer.php +++ b/app/Support/OpsUx/RunFailureSanitizer.php @@ -4,6 +4,18 @@ final class RunFailureSanitizer { + public const string REASON_GRAPH_THROTTLED = 'graph_throttled'; + + public const string REASON_GRAPH_TIMEOUT = 'graph_timeout'; + + public const string REASON_PERMISSION_DENIED = 'permission_denied'; + + public const string REASON_VALIDATION_ERROR = 'validation_error'; + + public const string REASON_CONFLICT_DETECTED = 'conflict_detected'; + + public const string REASON_UNKNOWN_ERROR = 'unknown_error'; + public static function sanitizeCode(string $code): string { $code = strtolower(trim($code)); @@ -15,10 +27,70 @@ public static function sanitizeCode(string $code): string return substr($code, 0, 80); } + public static function normalizeReasonCode(string $candidate): string + { + $candidate = strtolower(trim($candidate)); + + if ($candidate === '') { + return self::REASON_UNKNOWN_ERROR; + } + + $allowed = [ + self::REASON_GRAPH_THROTTLED, + self::REASON_GRAPH_TIMEOUT, + self::REASON_PERMISSION_DENIED, + self::REASON_VALIDATION_ERROR, + self::REASON_CONFLICT_DETECTED, + self::REASON_UNKNOWN_ERROR, + ]; + + if (in_array($candidate, $allowed, true)) { + return $candidate; + } + + // Compatibility mappings from existing codebase labels. + $candidate = match ($candidate) { + 'graph_forbidden' => self::REASON_PERMISSION_DENIED, + 'graph_transient' => self::REASON_GRAPH_TIMEOUT, + 'unknown' => self::REASON_UNKNOWN_ERROR, + default => $candidate, + }; + + if (in_array($candidate, $allowed, true)) { + return $candidate; + } + + // Heuristic normalization for ad-hoc codes used across jobs/services. + if (str_contains($candidate, 'throttle') || str_contains($candidate, '429')) { + return self::REASON_GRAPH_THROTTLED; + } + + if (str_contains($candidate, 'timeout') || str_contains($candidate, 'transient') || str_contains($candidate, '503') || str_contains($candidate, '504')) { + return self::REASON_GRAPH_TIMEOUT; + } + + if (str_contains($candidate, 'forbidden') || str_contains($candidate, 'permission') || str_contains($candidate, 'unauthorized') || str_contains($candidate, '403')) { + return self::REASON_PERMISSION_DENIED; + } + + if (str_contains($candidate, 'validation') || str_contains($candidate, 'not_found') || str_contains($candidate, 'bad_request') || str_contains($candidate, '400') || str_contains($candidate, '422')) { + return self::REASON_VALIDATION_ERROR; + } + + if (str_contains($candidate, 'conflict') || str_contains($candidate, '409')) { + return self::REASON_CONFLICT_DETECTED; + } + + return self::REASON_UNKNOWN_ERROR; + } + public static function sanitizeMessage(string $message): string { $message = trim(str_replace(["\r", "\n"], ' ', $message)); + // Redact obvious PII (emails). + $message = preg_replace('/[A-Z0-9._%+\-]+@[A-Z0-9.\-]+\.[A-Z]{2,}/i', '[REDACTED_EMAIL]', $message) ?? $message; + // Redact obvious bearer tokens / secrets. $message = preg_replace('/\bBearer\s+[A-Za-z0-9\-\._~\+\/]+=*\b/i', 'Bearer [REDACTED]', $message) ?? $message; $message = preg_replace('/\b(access_token|refresh_token|client_secret|password)\s*[:=]\s*[^\s]+/i', '$1=[REDACTED]', $message) ?? $message; diff --git a/specs/056-remove-legacy-bulkops/tasks.md b/specs/056-remove-legacy-bulkops/tasks.md index e4ec113..b55aa58 100644 --- a/specs/056-remove-legacy-bulkops/tasks.md +++ b/specs/056-remove-legacy-bulkops/tasks.md @@ -157,7 +157,7 @@ ## Phase 6: Polish & Cross-Cutting Concerns ## Monitoring DB-only render guard (NFR-01) -- [ ] T061 Add a regression test ensuring Monitoring → Operations pages do not invoke Graph/remote calls during render +- [X] T061 Add a regression test ensuring Monitoring → Operations pages do not invoke Graph/remote calls during render - Approach: - Mock/spy Graph client (or equivalent remote client) - Render Operations index and OperationRun detail pages @@ -179,7 +179,7 @@ ## Legacy removal (FR-006) ## Target scope display (FR-008) -- [ ] T065 Update OperationRun run detail view to display target scope consistently +- [X] T065 Update OperationRun run detail view to display target scope consistently - Show entra_tenant_name if present, else show entra_tenant_id - If directory_context_id exists, optionally show it as secondary info - Ensure this is visible in Monitoring → Operations → Run Detail @@ -187,19 +187,19 @@ ## Target scope display (FR-008) ## Failure semantics hardening (NFR-02) -- [ ] T066 Define/standardize reason codes for migrated bulk operations and enforce message sanitization bounds +- [X] T066 Define/standardize reason codes for migrated bulk operations and enforce message sanitization bounds - Baseline reason_code set: graph_throttled, graph_timeout, permission_denied, validation_error, conflict_detected, unknown_error - Ensure reason_code is stable and machine-readable - Ensure failure message is sanitized + bounded (no secrets/tokens/PII/raw payload dumps) - DoD: for each new/migrated bulk operation type, expected reason_code usage is clear and consistent -- [ ] T067 Add a regression test asserting failures/notifications never persist secrets/PII +- [X] T067 Add a regression test asserting failures/notifications never persist secrets/PII - Approach: create a run failure with sensitive-looking strings and assert persisted failures/notifications are sanitized - DoD: test fails if sensitive patterns appear in stored failures/notifications ## Remote retry/backoff/jitter policy (NFR-03) -- [ ] T068 Ensure migrated remote calls use the shared retry/backoff policy (429/503) and forbid ad-hoc retry loops +- [X] T068 Ensure migrated remote calls use the shared retry/backoff policy (429/503) and forbid ad-hoc retry loops - Use bounded retries + exponential backoff with jitter - DoD: no hand-rolled sleep/random retry logic in bulk workers; one test or assertion proves shared policy is used diff --git a/tests/Feature/Guards/NoAdHocRetryInBulkWorkersTest.php b/tests/Feature/Guards/NoAdHocRetryInBulkWorkersTest.php new file mode 100644 index 0000000..30dab4a --- /dev/null +++ b/tests/Feature/Guards/NoAdHocRetryInBulkWorkersTest.php @@ -0,0 +1,32 @@ +filter(fn (\SplFileInfo $file): bool => $file->isFile() && $file->getExtension() === 'php') + ->map(fn (\SplFileInfo $file): string => $file->getPathname()) + ->values(); + + expect($paths)->not->toBeEmpty(); + + $violations = []; + + foreach ($paths as $path) { + $contents = file_get_contents($path); + + if ($contents === false) { + continue; + } + + if (Str::contains($contents, ['sleep(', 'usleep('])) { + $violations[] = $path; + } + } + + expect($violations)->toBe([]); +}); diff --git a/tests/Feature/MonitoringOperationsTest.php b/tests/Feature/MonitoringOperationsTest.php index 4571262..3988cc2 100644 --- a/tests/Feature/MonitoringOperationsTest.php +++ b/tests/Feature/MonitoringOperationsTest.php @@ -13,7 +13,7 @@ $run = OperationRun::create([ 'tenant_id' => $tenant->id, - 'type' => 'test.run', + 'type' => 'policy.sync', 'status' => 'queued', 'outcome' => 'pending', 'initiator_name' => 'System', @@ -23,7 +23,7 @@ $this->actingAs($user) ->get(OperationRunResource::getUrl('index', tenant: $tenant)) ->assertSuccessful() - ->assertSee('test.run'); + ->assertSee('Policy sync'); }); it('renders monitoring pages DB-only (never calls Graph)', function () { @@ -33,7 +33,7 @@ $run = OperationRun::create([ 'tenant_id' => $tenant->id, - 'type' => 'test.run', + 'type' => 'policy.sync', 'status' => 'queued', 'outcome' => 'pending', 'initiator_name' => 'System', @@ -72,7 +72,7 @@ OperationRun::create([ 'tenant_id' => $tenantA->id, - 'type' => 'tenantA.run', + 'type' => 'policy.sync', 'status' => 'queued', 'outcome' => 'pending', 'initiator_name' => 'System', @@ -81,7 +81,7 @@ OperationRun::create([ 'tenant_id' => $tenantB->id, - 'type' => 'tenantB.run', + 'type' => 'inventory.sync', 'status' => 'queued', 'outcome' => 'pending', 'initiator_name' => 'System', @@ -93,8 +93,8 @@ // The cleanest way is to just GET the page URL, which runs middleware. $this->get(OperationRunResource::getUrl('index', tenant: $tenantA)) - ->assertSee('tenantA.run') - ->assertDontSee('tenantB.run'); + ->assertSee('Policy sync') + ->assertDontSee('Inventory sync'); }); it('allows readonly users to view operations list and detail', function () { @@ -104,7 +104,7 @@ $run = OperationRun::create([ 'tenant_id' => $tenant->id, - 'type' => 'test.run', + 'type' => 'policy.sync', 'status' => 'queued', 'outcome' => 'pending', 'initiator_name' => 'System', @@ -114,12 +114,12 @@ $this->actingAs($user) ->get(OperationRunResource::getUrl('index', tenant: $tenant)) ->assertSuccessful() - ->assertSee('test.run'); + ->assertSee('Policy sync'); $this->actingAs($user) ->get(OperationRunResource::getUrl('view', ['record' => $run], tenant: $tenant)) ->assertSuccessful() - ->assertSee('test.run'); + ->assertSee('Policy sync'); }); it('denies access to unauthorized users', function () { diff --git a/tests/Feature/OpsUx/FailureSanitizationTest.php b/tests/Feature/OpsUx/FailureSanitizationTest.php new file mode 100644 index 0000000..b3e3147 --- /dev/null +++ b/tests/Feature/OpsUx/FailureSanitizationTest.php @@ -0,0 +1,62 @@ +create(); + $user = User::factory()->create(); + $tenant->users()->attach($user); + + /** @var OperationRunService $runs */ + $runs = app(OperationRunService::class); + + $run = $runs->ensureRun( + tenant: $tenant, + type: 'test.sanitize', + inputs: [], + initiator: $user, + ); + + $rawBearer = 'Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.'.str_repeat('A', 90); + + $runs->updateRun( + $run, + status: 'completed', + outcome: 'failed', + failures: [[ + 'code' => 'graph_forbidden', + 'message' => "Authorization: {$rawBearer} client_secret=supersecret user=test.user@example.com", + ]], + ); + + $run->refresh(); + + $failureSummaryJson = json_encode($run->failure_summary, JSON_THROW_ON_ERROR); + + expect($failureSummaryJson)->not->toContain('client_secret=supersecret'); + expect($failureSummaryJson)->not->toContain($rawBearer); + expect($failureSummaryJson)->not->toContain('test.user@example.com'); + + expect($run->failure_summary[0]['reason_code'] ?? null)->toBe('permission_denied'); + + $notification = DatabaseNotification::query() + ->where('notifiable_id', $user->getKey()) + ->latest('id') + ->first(); + + expect($notification)->not->toBeNull(); + + $notificationJson = json_encode($notification?->data, JSON_THROW_ON_ERROR); + + expect($notificationJson)->not->toContain('client_secret=supersecret'); + expect($notificationJson)->not->toContain($rawBearer); + expect($notificationJson)->not->toContain('test.user@example.com'); + + $this->actingAs($user) + ->get(OperationRunResource::getUrl('view', ['record' => $run], tenant: $tenant)) + ->assertSuccessful(); +}); diff --git a/tests/Unit/MicrosoftGraphClientRetryPolicyTest.php b/tests/Unit/MicrosoftGraphClientRetryPolicyTest.php new file mode 100644 index 0000000..ae0d23a --- /dev/null +++ b/tests/Unit/MicrosoftGraphClientRetryPolicyTest.php @@ -0,0 +1,28 @@ + 'https://graph.microsoft.com', + 'graph.version' => 'beta', + 'graph.retry.times' => 2, + 'graph.retry.sleep' => 0, + ]); + + Http::fakeSequence() + ->push(['error' => ['code' => 'TooManyRequests', 'message' => 'throttled']], 429) + ->push(['value' => []], 200); + + /** @var MicrosoftGraphClient $client */ + $client = app(MicrosoftGraphClient::class); + + $response = $client->request('GET', '/deviceManagement/managedDevices', [ + 'access_token' => 'test-access-token', + ]); + + expect($response->success)->toBeTrue(); + + Http::assertSentCount(2); +});