From 81c010fa009611b46efad9490f0f01210081b321 Mon Sep 17 00:00:00 2001 From: ahmido Date: Mon, 26 Jan 2026 19:23:40 +0000 Subject: [PATCH] fix: Harden SyncPoliciesJob supported types handling (#75) Harden SyncPoliciesJob type input parsing + fail fast when supported types are empty/mismatched. Pass supported policy types from Tenant sync action and add regression tests. Co-authored-by: Ahmed Darrazi Reviewed-on: https://git.cloudarix.de/ahmido/TenantAtlas/pulls/75 --- app/Filament/Resources/PolicyResource.php | 12 +- .../PolicyResource/Pages/ListPolicies.php | 6 +- app/Filament/Resources/TenantResource.php | 32 ++++- app/Jobs/SyncPoliciesJob.php | 71 +++++++++- app/Services/OperationRunService.php | 32 +++++ database/seeders/DatabaseSeeder.php | 13 +- database/seeders/PoliciesSeeder.php | 5 +- specs/999-seeder-external-id/plan.md | 17 +++ specs/999-seeder-external-id/spec.md | 19 +++ specs/999-seeder-external-id/tasks.md | 15 +++ tests/Feature/Database/PoliciesSeederTest.php | 23 ++++ tests/Feature/DatabaseSeederTest.php | 16 +++ .../OperationRunServiceStaleQueuedRunTest.php | 53 ++++++++ tests/Feature/SyncPoliciesJobDispatchTest.php | 20 +++ .../SyncPoliciesJobGraphDisabledTest.php | 43 ++++++ .../SyncPoliciesJobSupportedTypesTest.php | 126 ++++++++++++++++++ 16 files changed, 479 insertions(+), 24 deletions(-) create mode 100644 specs/999-seeder-external-id/plan.md create mode 100644 specs/999-seeder-external-id/spec.md create mode 100644 specs/999-seeder-external-id/tasks.md create mode 100644 tests/Feature/Database/PoliciesSeederTest.php create mode 100644 tests/Feature/DatabaseSeederTest.php create mode 100644 tests/Feature/OperationRunServiceStaleQueuedRunTest.php create mode 100644 tests/Feature/SyncPoliciesJobDispatchTest.php create mode 100644 tests/Feature/SyncPoliciesJobGraphDisabledTest.php create mode 100644 tests/Feature/SyncPoliciesJobSupportedTypesTest.php diff --git a/app/Filament/Resources/PolicyResource.php b/app/Filament/Resources/PolicyResource.php index 8d34514..ce3d7fc 100644 --- a/app/Filament/Resources/PolicyResource.php +++ b/app/Filament/Resources/PolicyResource.php @@ -448,11 +448,7 @@ public static function table(Table $table): Table return; } - SyncPoliciesJob::dispatch( - tenantId: (int) $tenant->getKey(), - policyIds: [(int) $record->getKey()], - operationRun: $opRun - ); + SyncPoliciesJob::dispatch((int) $tenant->getKey(), null, [(int) $record->getKey()], $opRun); OpsUxBrowserEvents::dispatchRunEnqueued($livewire); OperationUxPresenter::queuedToast((string) $opRun->type) ->actions([ @@ -766,11 +762,7 @@ public static function table(Table $table): Table return; } - SyncPoliciesJob::dispatch( - tenantId: (int) $tenant->getKey(), - policyIds: $ids, - operationRun: $opRun - ); + SyncPoliciesJob::dispatch((int) $tenant->getKey(), null, $ids, $opRun); OpsUxBrowserEvents::dispatchRunEnqueued($livewire); OperationUxPresenter::queuedToast((string) $opRun->type) ->actions([ diff --git a/app/Filament/Resources/PolicyResource/Pages/ListPolicies.php b/app/Filament/Resources/PolicyResource/Pages/ListPolicies.php index c83bd8c..36db4e6 100644 --- a/app/Filament/Resources/PolicyResource/Pages/ListPolicies.php +++ b/app/Filament/Resources/PolicyResource/Pages/ListPolicies.php @@ -84,11 +84,7 @@ protected function getHeaderActions(): array } $opService->dispatchOrFail($opRun, function () use ($tenant, $requestedTypes, $opRun): void { - SyncPoliciesJob::dispatch( - tenantId: (int) $tenant->getKey(), - types: $requestedTypes, - operationRun: $opRun - ); + SyncPoliciesJob::dispatch((int) $tenant->getKey(), $requestedTypes, null, $opRun); }); OpsUxBrowserEvents::dispatchRunEnqueued($livewire); OperationUxPresenter::queuedToast((string) $opRun->type) diff --git a/app/Filament/Resources/TenantResource.php b/app/Filament/Resources/TenantResource.php index 605c1c9..a3aab59 100644 --- a/app/Filament/Resources/TenantResource.php +++ b/app/Filament/Resources/TenantResource.php @@ -212,13 +212,39 @@ public static function table(Table $table): Table // Phase 3: Canonical Operation Run Start /** @var OperationRunService $opService */ $opService = app(OperationRunService::class); + + $supportedTypes = config('tenantpilot.supported_policy_types', []); + $typeNames = array_map( + static fn (array $typeConfig): string => (string) $typeConfig['type'], + $supportedTypes, + ); + sort($typeNames); + + $inputs = [ + 'scope' => 'full', + 'types' => $typeNames, + ]; $opRun = $opService->ensureRun( tenant: $record, type: 'policy.sync', - inputs: ['scope' => 'full'], + inputs: $inputs, initiator: auth()->user() ); + if (! $opRun->wasRecentlyCreated && $opService->isStaleQueuedRun($opRun)) { + $opService->failStaleQueuedRun( + $opRun, + message: 'Run was queued but never started (likely a previous dispatch error). Re-queuing.' + ); + + $opRun = $opService->ensureRun( + tenant: $record, + type: 'policy.sync', + inputs: $inputs, + initiator: auth()->user() + ); + } + if (! $opRun->wasRecentlyCreated && in_array($opRun->status, ['queued', 'running'], true)) { Notification::make() ->title('Policy sync already active') @@ -234,7 +260,9 @@ public static function table(Table $table): Table return; } - SyncPoliciesJob::dispatch($record->getKey(), null, $opRun); + $opService->dispatchOrFail($opRun, function () use ($record, $supportedTypes, $opRun): void { + SyncPoliciesJob::dispatch((int) $record->getKey(), $supportedTypes, null, $opRun); + }); $auditLogger->log( tenant: $record, diff --git a/app/Jobs/SyncPoliciesJob.php b/app/Jobs/SyncPoliciesJob.php index 3344114..af5668d 100644 --- a/app/Jobs/SyncPoliciesJob.php +++ b/app/Jobs/SyncPoliciesJob.php @@ -6,6 +6,8 @@ use App\Models\OperationRun; use App\Models\Policy; use App\Models\Tenant; +use App\Services\Graph\GraphClientInterface; +use App\Services\Graph\NullGraphClient; use App\Services\Intune\PolicySyncService; use App\Services\OperationRunService; use App\Support\OperationRunOutcome; @@ -23,7 +25,7 @@ class SyncPoliciesJob implements ShouldQueue public ?OperationRun $operationRun = null; /** - * @param array|null $types + * @param array|array|null $types * @param array|null $policyIds */ public function __construct( @@ -42,6 +44,28 @@ public function middleware(): array public function handle(PolicySyncService $service, OperationRunService $operationRunService): void { + $graph = app(GraphClientInterface::class); + + if (! config('graph.enabled') || $graph instanceof NullGraphClient) { + if ($this->operationRun) { + $operationRunService->updateRun( + $this->operationRun, + status: OperationRunStatus::Completed->value, + outcome: OperationRunOutcome::Failed->value, + failures: [ + [ + 'code' => 'graph.disabled', + 'message' => 'Microsoft Graph is not enabled. Set GRAPH_ENABLED=true (and/or GRAPH_TENANT_ID) in .env to use the real Graph client.', + ], + ], + ); + + return; + } + + throw new \RuntimeException('Microsoft Graph is not enabled (GRAPH_ENABLED/GRAPH_TENANT_ID missing).'); + } + $tenant = Tenant::findOrFail($this->tenantId); if ($this->policyIds !== null) { @@ -117,7 +141,50 @@ public function handle(PolicySyncService $service, OperationRunService $operatio $supported = config('tenantpilot.supported_policy_types', []); if ($this->types !== null) { - $supported = array_values(array_filter($supported, fn ($type) => in_array($type['type'], $this->types, true))); + $first = $this->types[0] ?? null; + $typesLookLikeSupportedConfig = is_array($first) && array_key_exists('type', $first); + + if ($typesLookLikeSupportedConfig) { + $supported = array_values(array_filter( + $this->types, + static fn ($type): bool => is_array($type) && isset($type['type']) && is_string($type['type']) && $type['type'] !== '' + )); + } else { + $requestedTypes = array_values(array_unique(array_filter(array_map( + static fn ($type): ?string => is_string($type) ? $type : (is_array($type) ? (string) ($type['type'] ?? '') : null), + $this->types, + ), static fn ($type): bool => is_string($type) && $type !== ''))); + + $supported = array_values(array_filter( + $supported, + static fn ($type): bool => is_array($type) + && isset($type['type']) + && is_string($type['type']) + && in_array($type['type'], $requestedTypes, true) + )); + } + } + + if ($supported === []) { + if ($this->operationRun) { + $operationRunService->updateRun( + $this->operationRun, + status: OperationRunStatus::Completed->value, + outcome: OperationRunOutcome::Failed->value, + failures: [ + [ + 'code' => $this->types === null + ? 'tenantpilot.supported_policy_types.empty' + : 'tenantpilot.supported_policy_types.no_match', + 'message' => $this->types === null + ? 'No supported policy types configured (tenantpilot.supported_policy_types is empty).' + : 'No requested policy types matched the supported policy type configuration.', + ], + ], + ); + } + + return; } $result = $service->syncPoliciesWithReport($tenant, $supported); diff --git a/app/Services/OperationRunService.php b/app/Services/OperationRunService.php index 7df994a..e67c29b 100644 --- a/app/Services/OperationRunService.php +++ b/app/Services/OperationRunService.php @@ -22,6 +22,38 @@ class OperationRunService { + public function isStaleQueuedRun(OperationRun $run, int $thresholdMinutes = 5): bool + { + if ($run->status !== OperationRunStatus::Queued->value) { + return false; + } + + if ($run->started_at !== null) { + return false; + } + + if ($run->created_at === null) { + return false; + } + + return $run->created_at->lte(now()->subMinutes($thresholdMinutes)); + } + + public function failStaleQueuedRun(OperationRun $run, string $message = 'Run was queued but never started.'): OperationRun + { + return $this->updateRun( + $run, + status: OperationRunStatus::Completed->value, + outcome: OperationRunOutcome::Failed->value, + failures: [ + [ + 'code' => 'run.stale_queued', + 'message' => $message, + ], + ], + ); + } + public function ensureRun( Tenant $tenant, string $type, diff --git a/database/seeders/DatabaseSeeder.php b/database/seeders/DatabaseSeeder.php index d4a562d..4ccee86 100644 --- a/database/seeders/DatabaseSeeder.php +++ b/database/seeders/DatabaseSeeder.php @@ -5,6 +5,7 @@ use App\Models\User; use Illuminate\Database\Console\Seeds\WithoutModelEvents; use Illuminate\Database\Seeder; +use Illuminate\Support\Facades\Hash; class DatabaseSeeder extends Seeder { @@ -19,9 +20,13 @@ public function run(): void PoliciesSeeder::class, ]); - User::factory()->create([ - 'name' => 'Test User', - 'email' => 'test@example.com', - ]); + User::query()->updateOrCreate( + ['email' => 'test@example.com'], + [ + 'name' => 'Test User', + 'email_verified_at' => now(), + 'password' => Hash::make('password'), + ], + ); } } diff --git a/database/seeders/PoliciesSeeder.php b/database/seeders/PoliciesSeeder.php index 8bf4c03..74ee9d5 100644 --- a/database/seeders/PoliciesSeeder.php +++ b/database/seeders/PoliciesSeeder.php @@ -10,10 +10,13 @@ class PoliciesSeeder extends Seeder { public function run(): void { + $seedTenantId = env('INTUNE_TENANT_ID', 'local-tenant'); + $tenant = Tenant::firstOrCreate([ - 'tenant_id' => env('INTUNE_TENANT_ID', 'local-tenant'), + 'tenant_id' => $seedTenantId, ], [ 'name' => 'Default Tenant', + 'external_id' => $seedTenantId, 'domain' => null, 'metadata' => [], ]); diff --git a/specs/999-seeder-external-id/plan.md b/specs/999-seeder-external-id/plan.md new file mode 100644 index 0000000..02b34c9 --- /dev/null +++ b/specs/999-seeder-external-id/plan.md @@ -0,0 +1,17 @@ +# Plan — 999 Fix seeding: tenants.external_id not-null + +## Tech +- Laravel 12 +- PostgreSQL via Sail + +## Approach +1. Confirm `tenants.external_id` schema requirements in migrations. +2. Update the relevant seeder(s) so the default tenant includes `external_id`. + - Prefer generating a UUID (`Str::uuid()`) if `external_id` is meant to be globally unique. +3. Run `./vendor/bin/sail artisan migrate:fresh --seed`. +4. Add/adjust a small Pest test to prevent regression. + +## Files +- `database/seeders/PoliciesSeeder.php` (likely) +- `database/seeders/DatabaseSeeder.php` (if orchestration changes needed) +- `tests/Feature/...` (new/updated regression test) diff --git a/specs/999-seeder-external-id/spec.md b/specs/999-seeder-external-id/spec.md new file mode 100644 index 0000000..a5761c9 --- /dev/null +++ b/specs/999-seeder-external-id/spec.md @@ -0,0 +1,19 @@ +# 999 - Fix seeding: tenants.external_id not-null + +## Problem +Running `php artisan migrate:fresh --seed` fails with: + +- `SQLSTATE[23502]: Not null violation: null value in column "external_id" of relation "tenants"` + +The failing insert is from `Database\Seeders\PoliciesSeeder` creating a default tenant without populating `external_id`. + +## Goal +- `migrate:fresh --seed` succeeds on a clean database. + +## Non-goals +- No changes to production data or existing tenant records. + +## Acceptance Criteria +- Seeding succeeds with PostgreSQL. +- A default tenant is created with a valid `external_id`. +- Tests covering the seeding path (minimal) pass. diff --git a/specs/999-seeder-external-id/tasks.md b/specs/999-seeder-external-id/tasks.md new file mode 100644 index 0000000..64214b6 --- /dev/null +++ b/specs/999-seeder-external-id/tasks.md @@ -0,0 +1,15 @@ +# Tasks — 999 Fix seeding: tenants.external_id not-null + +## Setup +- [x] T001 Verify schema requirement for `tenants.external_id` in migrations. + +## Tests +- [x] T010 Add regression test ensuring `migrate:fresh --seed` (or the tenant seeder) creates a tenant with non-null `external_id`. + +## Core +- [x] T020 Fix seeder to set `external_id` for the default tenant. + +## Validation +- [x] T900 Run `./vendor/bin/sail artisan migrate:fresh --seed`. +- [x] T910 Run targeted Pest test(s) for the change. +- [x] T920 Run `./vendor/bin/sail bin pint --dirty`. diff --git a/tests/Feature/Database/PoliciesSeederTest.php b/tests/Feature/Database/PoliciesSeederTest.php new file mode 100644 index 0000000..befa69c --- /dev/null +++ b/tests/Feature/Database/PoliciesSeederTest.php @@ -0,0 +1,23 @@ +set('tenantpilot.supported_policy_types', []); + + $_ENV['INTUNE_TENANT_ID'] = 'test-tenant-id'; + $_SERVER['INTUNE_TENANT_ID'] = 'test-tenant-id'; + putenv('INTUNE_TENANT_ID=test-tenant-id'); + + $this->artisan('db:seed', ['--class' => Database\Seeders\PoliciesSeeder::class]) + ->assertExitCode(0); + + $tenant = Tenant::query()->first(); + + expect($tenant)->not->toBeNull(); + expect($tenant->tenant_id)->toBe('test-tenant-id'); + expect($tenant->external_id)->toBe('test-tenant-id'); +}); diff --git a/tests/Feature/DatabaseSeederTest.php b/tests/Feature/DatabaseSeederTest.php new file mode 100644 index 0000000..09d9767 --- /dev/null +++ b/tests/Feature/DatabaseSeederTest.php @@ -0,0 +1,16 @@ +seed(DatabaseSeeder::class); + $this->seed(DatabaseSeeder::class); + + $user = \App\Models\User::query()->where('email', 'test@example.com')->first(); + + expect($user)->not->toBeNull(); + expect($user->email)->toBe('test@example.com'); + + expect(Auth::attempt(['email' => 'test@example.com', 'password' => 'password']))->toBeTrue(); +}); diff --git a/tests/Feature/OperationRunServiceStaleQueuedRunTest.php b/tests/Feature/OperationRunServiceStaleQueuedRunTest.php new file mode 100644 index 0000000..86da46f --- /dev/null +++ b/tests/Feature/OperationRunServiceStaleQueuedRunTest.php @@ -0,0 +1,53 @@ +create([ + 'status' => OperationRunStatus::Queued->value, + 'started_at' => null, + 'created_at' => now()->subMinutes(6), + ]); + + $service = app(OperationRunService::class); + + expect($service->isStaleQueuedRun($run))->toBeTrue(); +}); + +it('does not treat recent queued runs as stale', function () { + $run = OperationRun::factory()->create([ + 'status' => OperationRunStatus::Queued->value, + 'started_at' => null, + 'created_at' => now()->subMinute(), + ]); + + $service = app(OperationRunService::class); + + expect($service->isStaleQueuedRun($run))->toBeFalse(); +}); + +it('fails a stale queued run and marks it completed', function () { + $run = OperationRun::factory()->create([ + 'status' => OperationRunStatus::Queued->value, + 'outcome' => OperationRunOutcome::Pending->value, + 'started_at' => null, + 'completed_at' => null, + 'failure_summary' => [], + ]); + + $service = app(OperationRunService::class); + + $updated = $service->failStaleQueuedRun($run, message: 'Stale test run'); + + expect($updated->status)->toBe(OperationRunStatus::Completed->value); + expect($updated->outcome)->toBe(OperationRunOutcome::Failed->value); + expect($updated->completed_at)->not->toBeNull(); + + $failures = is_array($updated->failure_summary ?? null) ? $updated->failure_summary : []; + + expect($failures)->not->toBeEmpty(); + expect($failures[0]['code'] ?? null)->toBe('run.stale_queued'); +}); diff --git a/tests/Feature/SyncPoliciesJobDispatchTest.php b/tests/Feature/SyncPoliciesJobDispatchTest.php new file mode 100644 index 0000000..bc57b3e --- /dev/null +++ b/tests/Feature/SyncPoliciesJobDispatchTest.php @@ -0,0 +1,20 @@ +create(); + + SyncPoliciesJob::dispatch((int) $operationRun->tenant_id, null, null, $operationRun); + + Bus::assertDispatched(SyncPoliciesJob::class, function (SyncPoliciesJob $job) use ($operationRun): bool { + return $job->tenantId === (int) $operationRun->tenant_id + && $job->types === null + && $job->policyIds === null + && (int) $job->operationRun?->getKey() === (int) $operationRun->getKey(); + }); +}); diff --git a/tests/Feature/SyncPoliciesJobGraphDisabledTest.php b/tests/Feature/SyncPoliciesJobGraphDisabledTest.php new file mode 100644 index 0000000..e658f5f --- /dev/null +++ b/tests/Feature/SyncPoliciesJobGraphDisabledTest.php @@ -0,0 +1,43 @@ + false]); + + $tenant = Tenant::factory()->create([ + 'status' => 'active', + ]); + + $run = OperationRun::factory()->create([ + 'tenant_id' => $tenant->getKey(), + 'type' => 'policy.sync', + 'status' => OperationRunStatus::Queued->value, + 'outcome' => OperationRunOutcome::Pending->value, + ]); + + $job = new SyncPoliciesJob( + tenantId: (int) $tenant->getKey(), + types: null, + policyIds: null, + operationRun: $run, + ); + + $job->handle(app(PolicySyncService::class), app(OperationRunService::class)); + + $run->refresh(); + + expect($run->status)->toBe(OperationRunStatus::Completed->value); + expect($run->outcome)->toBe(OperationRunOutcome::Failed->value); + + $failures = is_array($run->failure_summary ?? null) ? $run->failure_summary : []; + + expect($failures)->not->toBeEmpty(); + expect($failures[0]['code'] ?? null)->toBe('graph.disabled'); +}); diff --git a/tests/Feature/SyncPoliciesJobSupportedTypesTest.php b/tests/Feature/SyncPoliciesJobSupportedTypesTest.php new file mode 100644 index 0000000..c86b9b1 --- /dev/null +++ b/tests/Feature/SyncPoliciesJobSupportedTypesTest.php @@ -0,0 +1,126 @@ + true]); + config(['tenantpilot.supported_policy_types' => []]); + + app()->instance(GraphClientInterface::class, new FakeGraphClient); + + $tenant = Tenant::factory()->create(); + $run = OperationRun::factory()->for($tenant)->create([ + 'type' => 'policy.sync', + 'status' => OperationRunStatus::Queued->value, + 'outcome' => OperationRunOutcome::Pending->value, + ]); + + $job = new SyncPoliciesJob( + tenantId: (int) $tenant->getKey(), + types: null, + policyIds: null, + operationRun: $run, + ); + + $service = mock(PolicySyncService::class); + $service->shouldNotReceive('syncPoliciesWithReport'); + + $job->handle($service, app(OperationRunService::class)); + + $run->refresh(); + + expect($run->status)->toBe(OperationRunStatus::Completed->value) + ->and($run->outcome)->toBe(OperationRunOutcome::Failed->value) + ->and($run->failure_summary)->toBeArray() + ->and($run->failure_summary[0]['code'] ?? null)->toBe('tenantpilot.supported_policy_types.empty'); +}); + +it('accepts supported policy config passed via the types argument', function () { + config(['graph.enabled' => true]); + config(['tenantpilot.supported_policy_types' => []]); + + app()->instance(GraphClientInterface::class, new FakeGraphClient); + + $tenant = Tenant::factory()->create(); + $run = OperationRun::factory()->for($tenant)->create([ + 'type' => 'policy.sync', + 'status' => OperationRunStatus::Queued->value, + 'outcome' => OperationRunOutcome::Pending->value, + ]); + + $supported = [ + ['type' => 'deviceConfiguration', 'platform' => 'all'], + ]; + + $job = new SyncPoliciesJob( + tenantId: (int) $tenant->getKey(), + types: $supported, + policyIds: null, + operationRun: $run, + ); + + $service = mock(PolicySyncService::class); + $service->shouldReceive('syncPoliciesWithReport') + ->once() + ->withArgs(function (Tenant $tenantArg, array $supportedArg) use ($tenant, $supported) { + expect($tenantArg->getKey())->toBe($tenant->getKey()); + expect($supportedArg)->toBe($supported); + + return true; + }) + ->andReturn([ + 'synced' => [101, 102, 103], + 'failures' => [], + ]); + + $job->handle($service, app(OperationRunService::class)); + + $run->refresh(); + + expect($run->status)->toBe(OperationRunStatus::Completed->value) + ->and($run->outcome)->toBe(OperationRunOutcome::Succeeded->value) + ->and($run->summary_counts['succeeded'] ?? null)->toBe(3); +});