057-filament-v5-upgrade #66
@ -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;
|
||||
}
|
||||
}
|
||||
|
||||
@ -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() ?? [];
|
||||
|
||||
@ -531,7 +531,7 @@ protected function isListArray(array $array): bool
|
||||
|
||||
/**
|
||||
* @param array<int, array{code?: mixed, message?: mixed}> $failures
|
||||
* @return array<int, array{code: string, message: string}>
|
||||
* @return array<int, array{code: string, reason_code: string, message: string}>
|
||||
*/
|
||||
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),
|
||||
];
|
||||
}
|
||||
|
||||
@ -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;
|
||||
|
||||
@ -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
|
||||
|
||||
|
||||
32
tests/Feature/Guards/NoAdHocRetryInBulkWorkersTest.php
Normal file
32
tests/Feature/Guards/NoAdHocRetryInBulkWorkersTest.php
Normal file
@ -0,0 +1,32 @@
|
||||
<?php
|
||||
|
||||
use Illuminate\Support\Str;
|
||||
|
||||
it('does not use ad-hoc sleep/usleep retry loops in bulk worker jobs', function () {
|
||||
$root = base_path('app/Jobs/Operations');
|
||||
|
||||
$iterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($root));
|
||||
|
||||
$paths = collect(iterator_to_array($iterator))
|
||||
->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([]);
|
||||
});
|
||||
@ -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 () {
|
||||
|
||||
62
tests/Feature/OpsUx/FailureSanitizationTest.php
Normal file
62
tests/Feature/OpsUx/FailureSanitizationTest.php
Normal file
@ -0,0 +1,62 @@
|
||||
<?php
|
||||
|
||||
use App\Filament\Resources\OperationRunResource;
|
||||
use App\Models\Tenant;
|
||||
use App\Models\User;
|
||||
use App\Services\OperationRunService;
|
||||
use Illuminate\Notifications\DatabaseNotification;
|
||||
|
||||
it('sanitizes persisted run failures and terminal notifications', function () {
|
||||
$tenant = Tenant::factory()->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();
|
||||
});
|
||||
28
tests/Unit/MicrosoftGraphClientRetryPolicyTest.php
Normal file
28
tests/Unit/MicrosoftGraphClientRetryPolicyTest.php
Normal file
@ -0,0 +1,28 @@
|
||||
<?php
|
||||
|
||||
use App\Services\Graph\MicrosoftGraphClient;
|
||||
use Illuminate\Support\Facades\Http;
|
||||
|
||||
it('retries 429 responses using the shared retry policy', function () {
|
||||
config([
|
||||
'graph.base_url' => '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);
|
||||
});
|
||||
Loading…
Reference in New Issue
Block a user