From 51fc2fe2ec7393c1ef6bcfa648cf7476b4276a62 Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Thu, 12 Feb 2026 11:08:52 +0100 Subject: [PATCH] feat(spec-088): remove tenant graphOptions legacy path --- .github/agents/copilot-instructions.md | 4 +- app/Models/Tenant.php | 8 +- app/Providers/AppServiceProvider.php | 10 + app/Services/AssignmentBackupService.php | 6 +- app/Services/AssignmentRestoreService.php | 6 +- .../Directory/EntraGroupSyncService.php | 8 +- .../Directory/RoleDefinitionsSyncService.php | 4 +- .../Graph/AssignmentFilterResolver.php | 4 +- .../ConfigurationPolicyTemplateResolver.php | 11 +- .../Intune/FoundationMappingService.php | 4 +- .../Intune/FoundationSnapshotService.php | 4 +- .../Intune/PolicyCaptureOrchestrator.php | 10 +- app/Services/Intune/RestoreRiskChecker.php | 8 +- app/Services/Intune/TenantConfigService.php | 22 +- .../Intune/TenantPermissionService.php | 19 +- app/Services/Intune/VersionService.php | 6 +- .../MicrosoftGraphOptionsResolver.php | 31 + ...ProviderConfigurationRequiredException.php | 30 + .../analysis-report.md | 770 ++++++++++++++++++ .../checklists/requirements.md | 35 + .../contracts/README.md | 11 + .../data-model.md | 40 + .../plan.md | 105 +++ .../quickstart.md | 22 + .../research.md | 71 ++ .../spec.md | 88 ++ .../tasks.md | 128 +++ .../BackupWithAssignmentsConsistencyTest.php | 2 + .../EndpointSecurityPolicyRestore023Test.php | 2 + tests/Feature/Filament/BackupCreationTest.php | 4 + .../PolicyCaptureSnapshotOptionsTest.php | 1 + tests/Feature/Filament/RestorePreviewTest.php | 3 + tests/Feature/Filament/TenantSetupTest.php | 2 +- .../TenantVerificationReportWidgetTest.php | 6 +- tests/Feature/FoundationBackupTest.php | 2 + .../Guards/NoLegacyTenantGraphOptionsTest.php | 108 +++ .../TenantGraphOptionsKillSwitchTest.php | 17 + ...derOperationBlockedGuidanceSpec081Test.php | 4 +- .../MicrosoftGraphOptionsResolverTest.php | 85 ++ .../Rbac/UiEnforcementNonMemberHiddenTest.php | 57 +- .../RequiredPermissionsCopyActionsTest.php | 2 +- tests/Feature/RestoreRiskChecksWizardTest.php | 3 + tests/Feature/RestoreRunWizardExecuteTest.php | 2 + .../Feature/RestoreRunWizardMetadataTest.php | 1 + .../VerificationStartDedupeTest.php | 4 +- .../VersionCaptureMetadataOnlyTest.php | 1 + .../VersionCaptureWithAssignmentsTest.php | 1 + tests/Pest.php | 31 +- tests/Unit/AssignmentBackupServiceTest.php | 2 + tests/Unit/AssignmentFilterResolverTest.php | 3 +- tests/Unit/AssignmentRestoreServiceTest.php | 21 +- tests/Unit/FoundationMappingServiceTest.php | 7 + tests/Unit/FoundationSnapshotServiceTest.php | 4 + .../Intune/VersionServiceConcurrencyTest.php | 2 + tests/Unit/PolicyCaptureOrchestratorTest.php | 4 + tests/Unit/TenantPermissionServiceTest.php | 8 + 56 files changed, 1767 insertions(+), 87 deletions(-) create mode 100644 app/Services/Providers/MicrosoftGraphOptionsResolver.php create mode 100644 app/Services/Providers/ProviderConfigurationRequiredException.php create mode 100644 specs/087-legacy-runs-removal/analysis-report.md create mode 100644 specs/088-remove-tenant-graphoptions-legacy/checklists/requirements.md create mode 100644 specs/088-remove-tenant-graphoptions-legacy/contracts/README.md create mode 100644 specs/088-remove-tenant-graphoptions-legacy/data-model.md create mode 100644 specs/088-remove-tenant-graphoptions-legacy/plan.md create mode 100644 specs/088-remove-tenant-graphoptions-legacy/quickstart.md create mode 100644 specs/088-remove-tenant-graphoptions-legacy/research.md create mode 100644 specs/088-remove-tenant-graphoptions-legacy/spec.md create mode 100644 specs/088-remove-tenant-graphoptions-legacy/tasks.md create mode 100644 tests/Feature/Guards/NoLegacyTenantGraphOptionsTest.php create mode 100644 tests/Feature/Models/TenantGraphOptionsKillSwitchTest.php create mode 100644 tests/Feature/Providers/MicrosoftGraphOptionsResolverTest.php diff --git a/.github/agents/copilot-instructions.md b/.github/agents/copilot-instructions.md index 5bc8751..546f125 100644 --- a/.github/agents/copilot-instructions.md +++ b/.github/agents/copilot-instructions.md @@ -46,10 +46,8 @@ ## Code Style PHP 8.4.15: Follow standard conventions ## Recent Changes +- 088-remove-tenant-graphoptions-legacy: Added PHP 8.4.15 (Laravel 12) + Filament v5, Livewire v4, Pest v4 - 086-retire-legacy-runs-into-operation-runs: Spec docs updated (PHP 8.4.15 + Laravel 12, Filament v5, Livewire v4) - 085-tenant-operate-hub: Added PHP 8.4 (Laravel 12) + Filament v5, Livewire v4, Laravel Sail, Tailwind CSS v4 -- 085-tenant-operate-hub: Added PHP 8.4.15 (Laravel 12) + Filament v5, Livewire v4, Pest v4, Tailwind CSS v4, Laravel Sail -- 084-verification-surfaces-unification: Added PHP 8.4 (Laravel 12) + Filament v5 (Livewire v4), Queue/Jobs (Laravel), Microsoft Graph via `GraphClientInterface` -- 082-action-surface-contract: Added PHP 8.4.x + Laravel 12, Filament v5, Livewire v4 diff --git a/app/Models/Tenant.php b/app/Models/Tenant.php index 8b47a5a..5d4015e 100644 --- a/app/Models/Tenant.php +++ b/app/Models/Tenant.php @@ -297,11 +297,9 @@ public function graphTenantId(): ?string */ public function graphOptions(): array { - return [ - 'tenant' => $this->graphTenantId(), - 'client_id' => $this->app_client_id, - 'client_secret' => $this->app_client_secret, - ]; + throw new \BadMethodCallException( + 'Tenant::graphOptions() has been removed. Resolve a ProviderConnection via ProviderConnectionResolver and build options via ProviderGateway (or use MicrosoftGraphOptionsResolver).' + ); } public function scopeForTenant(Builder $query, self|int|string $tenant): Builder diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 0ca3725..78ee8b6 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -35,6 +35,9 @@ use App\Services\Intune\WindowsFeatureUpdateProfileNormalizer; use App\Services\Intune\WindowsQualityUpdateProfileNormalizer; use App\Services\Intune\WindowsUpdateRingNormalizer; +use App\Services\Providers\MicrosoftGraphOptionsResolver; +use App\Services\Providers\ProviderConnectionResolver; +use App\Services\Providers\ProviderGateway; use Filament\Events\TenantSet; use Illuminate\Cache\RateLimiting\Limit; use Illuminate\Http\Request; @@ -61,6 +64,13 @@ public function register(): void return $app->make(NullGraphClient::class); }); + $this->app->singleton(MicrosoftGraphOptionsResolver::class, function ($app): MicrosoftGraphOptionsResolver { + return new MicrosoftGraphOptionsResolver( + connections: $app->make(ProviderConnectionResolver::class), + gateway: $app->make(ProviderGateway::class), + ); + }); + $this->app->tag( [ AppProtectionPolicyNormalizer::class, diff --git a/app/Services/AssignmentBackupService.php b/app/Services/AssignmentBackupService.php index 8439de0..ba56088 100644 --- a/app/Services/AssignmentBackupService.php +++ b/app/Services/AssignmentBackupService.php @@ -8,6 +8,7 @@ use App\Services\Graph\AssignmentFilterResolver; use App\Services\Graph\GroupResolver; use App\Services\Graph\ScopeTagResolver; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use Illuminate\Support\Facades\Log; class AssignmentBackupService @@ -17,6 +18,7 @@ public function __construct( private readonly GroupResolver $groupResolver, private readonly AssignmentFilterResolver $assignmentFilterResolver, private readonly ScopeTagResolver $scopeTagResolver, + private readonly MicrosoftGraphOptionsResolver $graphOptionsResolver, ) {} /** @@ -58,8 +60,8 @@ public function enrichWithAssignments( } // Fetch assignments from Graph API - $graphOptions = $tenant->graphOptions(); - $tenantId = $graphOptions['tenant'] ?? $tenant->external_id ?? $tenant->tenant_id; + $graphOptions = $this->graphOptionsResolver->resolveForTenant($tenant); + $tenantId = (string) ($graphOptions['tenant'] ?? ''); $assignments = $this->assignmentFetcher->fetch( $policyType, $tenantId, diff --git a/app/Services/AssignmentRestoreService.php b/app/Services/AssignmentRestoreService.php index 2d8abee..c282fb9 100644 --- a/app/Services/AssignmentRestoreService.php +++ b/app/Services/AssignmentRestoreService.php @@ -10,6 +10,7 @@ use App\Services\Graph\GraphLogger; use App\Services\Graph\GraphResponse; use App\Services\Intune\AuditLogger; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use Illuminate\Support\Arr; use Illuminate\Support\Facades\Log; @@ -21,6 +22,7 @@ public function __construct( private readonly GraphLogger $graphLogger, private readonly AuditLogger $auditLogger, private readonly AssignmentFilterResolver $assignmentFilterResolver, + private readonly MicrosoftGraphOptionsResolver $graphOptionsResolver, ) {} /** @@ -86,8 +88,8 @@ public function restore( ]; } - $graphOptions = $tenant->graphOptions(); - $tenantIdentifier = $graphOptions['tenant'] ?? $tenant->graphTenantId() ?? (string) $tenant->getKey(); + $graphOptions = $this->graphOptionsResolver->resolveForTenant($tenant); + $tenantIdentifier = (string) ($graphOptions['tenant'] ?? ''); $context = [ 'tenant' => $tenantIdentifier, diff --git a/app/Services/Directory/EntraGroupSyncService.php b/app/Services/Directory/EntraGroupSyncService.php index 79f0353..e2bc547 100644 --- a/app/Services/Directory/EntraGroupSyncService.php +++ b/app/Services/Directory/EntraGroupSyncService.php @@ -2,15 +2,16 @@ namespace App\Services\Directory; -use App\Models\EntraGroup; use App\Jobs\EntraGroupSyncJob; +use App\Models\EntraGroup; use App\Models\OperationRun; use App\Models\Tenant; use App\Models\User; -use App\Services\OperationRunService; use App\Services\Graph\GraphClientInterface; use App\Services\Graph\GraphContractRegistry; use App\Services\Graph\GraphResponse; +use App\Services\OperationRunService; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use Carbon\CarbonImmutable; class EntraGroupSyncService @@ -18,6 +19,7 @@ class EntraGroupSyncService public function __construct( private readonly GraphClientInterface $graph, private readonly GraphContractRegistry $contracts, + private readonly MicrosoftGraphOptionsResolver $graphOptionsResolver, ) {} public function startManualSync(Tenant $tenant, User $user): OperationRun @@ -103,7 +105,7 @@ public function sync(Tenant $tenant, string $selectionKey): array $errorSummary = null; $errorCount = 0; - $options = $tenant->graphOptions(); + $options = $this->graphOptionsResolver->resolveForTenant($tenant); $useQuery = $query; $nextPath = $path; diff --git a/app/Services/Directory/RoleDefinitionsSyncService.php b/app/Services/Directory/RoleDefinitionsSyncService.php index ddb77b4..7f97a94 100644 --- a/app/Services/Directory/RoleDefinitionsSyncService.php +++ b/app/Services/Directory/RoleDefinitionsSyncService.php @@ -11,6 +11,7 @@ use App\Services\Graph\GraphContractRegistry; use App\Services\Graph\GraphResponse; use App\Services\OperationRunService; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use Carbon\CarbonImmutable; class RoleDefinitionsSyncService @@ -18,6 +19,7 @@ class RoleDefinitionsSyncService public function __construct( private readonly GraphClientInterface $graph, private readonly GraphContractRegistry $contracts, + private readonly MicrosoftGraphOptionsResolver $graphOptionsResolver, ) {} public function startManualSync(Tenant $tenant, User $user): OperationRun @@ -101,7 +103,7 @@ public function sync(Tenant $tenant): array $errorSummary = null; $errorCount = 0; - $options = $tenant->graphOptions(); + $options = $this->graphOptionsResolver->resolveForTenant($tenant); $useQuery = $query; $nextPath = $path; diff --git a/app/Services/Graph/AssignmentFilterResolver.php b/app/Services/Graph/AssignmentFilterResolver.php index ab7b0eb..764c134 100644 --- a/app/Services/Graph/AssignmentFilterResolver.php +++ b/app/Services/Graph/AssignmentFilterResolver.php @@ -3,12 +3,14 @@ namespace App\Services\Graph; use App\Models\Tenant; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use Illuminate\Support\Facades\Cache; class AssignmentFilterResolver { public function __construct( private readonly MicrosoftGraphClient $graphClient, + private readonly MicrosoftGraphOptionsResolver $graphOptionsResolver, ) {} /** @@ -39,7 +41,7 @@ private function fetchAllFilters(?Tenant $tenant = null): array $options = ['query' => ['$select' => 'id,displayName']]; if ($tenant) { - $options = array_merge($options, $tenant->graphOptions()); + $options = array_merge($options, $this->graphOptionsResolver->resolveForTenant($tenant)); } $response = $this->graphClient->request( diff --git a/app/Services/Intune/ConfigurationPolicyTemplateResolver.php b/app/Services/Intune/ConfigurationPolicyTemplateResolver.php index 3432c59..085d147 100644 --- a/app/Services/Intune/ConfigurationPolicyTemplateResolver.php +++ b/app/Services/Intune/ConfigurationPolicyTemplateResolver.php @@ -4,6 +4,7 @@ use App\Models\Tenant; use App\Services\Graph\GraphClientInterface; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use Illuminate\Support\Arr; class ConfigurationPolicyTemplateResolver @@ -25,6 +26,7 @@ class ConfigurationPolicyTemplateResolver public function __construct( private readonly GraphClientInterface $graphClient, + private readonly MicrosoftGraphOptionsResolver $graphOptionsResolver, ) {} /** @@ -155,7 +157,10 @@ public function getTemplate(Tenant $tenant, string $templateId, array $graphOpti return $this->templateCache[$tenantKey][$templateId]; } - $context = array_merge($tenant->graphOptions(), Arr::except($graphOptions, ['platform'])); + $context = array_merge( + $this->graphOptionsResolver->resolveForTenant($tenant), + Arr::except($graphOptions, ['platform']), + ); $path = sprintf('/deviceManagement/configurationPolicyTemplates/%s', urlencode($templateId)); $response = $this->graphClient->request('GET', $path, $context); @@ -189,7 +194,7 @@ public function listTemplatesByFamily(Tenant $tenant, string $templateFamily, ar $escapedFamily = str_replace("'", "''", $templateFamily); - $context = array_merge($tenant->graphOptions(), Arr::except($graphOptions, ['platform']), [ + $context = array_merge($this->graphOptionsResolver->resolveForTenant($tenant), Arr::except($graphOptions, ['platform']), [ 'query' => [ '$filter' => "templateFamily eq '{$escapedFamily}'", '$top' => 999, @@ -228,7 +233,7 @@ public function fetchTemplateSettingDefinitionIds(Tenant $tenant, string $templa return $this->templateDefinitionCache[$tenantKey][$templateId]; } - $context = array_merge($tenant->graphOptions(), Arr::except($graphOptions, ['platform']), [ + $context = array_merge($this->graphOptionsResolver->resolveForTenant($tenant), Arr::except($graphOptions, ['platform']), [ 'query' => [ '$expand' => 'settingDefinitions', '$top' => 999, diff --git a/app/Services/Intune/FoundationMappingService.php b/app/Services/Intune/FoundationMappingService.php index 3a93918..037c0fb 100644 --- a/app/Services/Intune/FoundationMappingService.php +++ b/app/Services/Intune/FoundationMappingService.php @@ -6,6 +6,7 @@ use App\Models\Tenant; use App\Services\Graph\GraphClientInterface; use App\Services\Graph\GraphContractRegistry; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use Illuminate\Support\Collection; class FoundationMappingService @@ -14,6 +15,7 @@ public function __construct( private readonly GraphClientInterface $graphClient, private readonly GraphContractRegistry $contracts, private readonly FoundationSnapshotService $snapshotService, + private readonly MicrosoftGraphOptionsResolver $graphOptionsResolver, ) {} /** @@ -271,7 +273,7 @@ private function createFoundation( $response = $this->graphClient->request( $method, $resource, - ['json' => $payload] + $tenant->graphOptions() + ['json' => $payload] + $this->graphOptionsResolver->resolveForTenant($tenant) ); if ($response->failed()) { diff --git a/app/Services/Intune/FoundationSnapshotService.php b/app/Services/Intune/FoundationSnapshotService.php index 33d4e03..f203d3d 100644 --- a/app/Services/Intune/FoundationSnapshotService.php +++ b/app/Services/Intune/FoundationSnapshotService.php @@ -5,12 +5,14 @@ use App\Models\Tenant; use App\Services\Graph\GraphClientInterface; use App\Services\Graph\GraphContractRegistry; +use App\Services\Providers\MicrosoftGraphOptionsResolver; class FoundationSnapshotService { public function __construct( private readonly GraphClientInterface $graphClient, private readonly GraphContractRegistry $contracts, + private readonly MicrosoftGraphOptionsResolver $graphOptionsResolver, ) {} /** @@ -39,7 +41,7 @@ public function fetchAll(Tenant $tenant, string $foundationType): array } $sanitized = $this->contracts->sanitizeQuery($foundationType, $query); - $options = $tenant->graphOptions(); + $options = $this->graphOptionsResolver->resolveForTenant($tenant); $items = []; $failures = []; $nextPath = $resource; diff --git a/app/Services/Intune/PolicyCaptureOrchestrator.php b/app/Services/Intune/PolicyCaptureOrchestrator.php index b7a5f78..ab266c5 100644 --- a/app/Services/Intune/PolicyCaptureOrchestrator.php +++ b/app/Services/Intune/PolicyCaptureOrchestrator.php @@ -9,6 +9,7 @@ use App\Services\Graph\AssignmentFilterResolver; use App\Services\Graph\GroupResolver; use App\Services\Graph\ScopeTagResolver; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use Illuminate\Support\Facades\Log; /** @@ -25,6 +26,7 @@ public function __construct( private readonly GroupResolver $groupResolver, private readonly AssignmentFilterResolver $assignmentFilterResolver, private readonly ScopeTagResolver $scopeTagResolver, + private readonly MicrosoftGraphOptionsResolver $graphOptionsResolver, ) {} /** @@ -40,8 +42,8 @@ public function capture( ?string $createdBy = null, array $metadata = [] ): array { - $graphOptions = $tenant->graphOptions(); - $tenantIdentifier = $graphOptions['tenant'] ?? $tenant->graphTenantId() ?? (string) $tenant->getKey(); + $graphOptions = $this->graphOptionsResolver->resolveForTenant($tenant); + $tenantIdentifier = (string) ($graphOptions['tenant'] ?? ''); // 1. Fetch policy snapshot $snapshot = $this->snapshotService->fetch($tenant, $policy, $createdBy); @@ -231,8 +233,8 @@ public function ensureVersionHasAssignments( bool $includeAssignments = false, bool $includeScopeTags = false ): PolicyVersion { - $graphOptions = $tenant->graphOptions(); - $tenantIdentifier = $graphOptions['tenant'] ?? $tenant->graphTenantId() ?? (string) $tenant->getKey(); + $graphOptions = $this->graphOptionsResolver->resolveForTenant($tenant); + $tenantIdentifier = (string) ($graphOptions['tenant'] ?? ''); if ($version->assignments !== null && $version->scope_tags !== null) { Log::debug('Version already has assignments, skipping', [ diff --git a/app/Services/Intune/RestoreRiskChecker.php b/app/Services/Intune/RestoreRiskChecker.php index 84be502..ff01760 100644 --- a/app/Services/Intune/RestoreRiskChecker.php +++ b/app/Services/Intune/RestoreRiskChecker.php @@ -8,6 +8,7 @@ use App\Models\PolicyVersion; use App\Models\Tenant; use App\Services\Graph\GroupResolver; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use Carbon\CarbonImmutable; use Illuminate\Support\Collection; use Illuminate\Support\Str; @@ -17,6 +18,7 @@ class RestoreRiskChecker public function __construct( private readonly GroupResolver $groupResolver, private readonly ConfigurationPolicyTemplateResolver $templateResolver, + private readonly MicrosoftGraphOptionsResolver $graphOptionsResolver, ) {} /** @@ -109,8 +111,8 @@ private function checkOrphanedGroups(Tenant $tenant, Collection $policyItems, ar ]; } - $graphOptions = $tenant->graphOptions(); - $tenantIdentifier = $graphOptions['tenant'] ?? $tenant->graphTenantId() ?? (string) $tenant->getKey(); + $graphOptions = $this->graphOptionsResolver->resolveForTenant($tenant); + $tenantIdentifier = (string) ($graphOptions['tenant'] ?? ''); $resolved = $this->groupResolver->resolveGroupIds($groupIds, $tenantIdentifier, $graphOptions); $orphaned = []; @@ -241,7 +243,7 @@ private function checkEndpointSecurityTemplates(Tenant $tenant, Collection $poli { $issues = []; $hasRestoreEnabled = false; - $graphOptions = $tenant->graphOptions(); + $graphOptions = $this->graphOptionsResolver->resolveForTenant($tenant); foreach ($policyItems as $item) { if ($item->policy_type !== 'endpointSecurityPolicy') { diff --git a/app/Services/Intune/TenantConfigService.php b/app/Services/Intune/TenantConfigService.php index aec333c..25bc78a 100644 --- a/app/Services/Intune/TenantConfigService.php +++ b/app/Services/Intune/TenantConfigService.php @@ -5,18 +5,32 @@ use App\Models\Tenant; use App\Services\Graph\GraphClientInterface; use App\Services\Graph\GraphErrorMapper; +use App\Services\Providers\MicrosoftGraphOptionsResolver; +use App\Services\Providers\ProviderConfigurationRequiredException; +use App\Support\Providers\ProviderReasonCodes; use Throwable; class TenantConfigService { - public function __construct(private readonly GraphClientInterface $graphClient) {} + public function __construct( + private readonly GraphClientInterface $graphClient, + private readonly MicrosoftGraphOptionsResolver $graphOptionsResolver, + ) {} /** * @return array{success:bool,error_message:?string,requires_consent:bool} */ public function testConnectivity(Tenant $tenant): array { - $options = $this->graphOptions($tenant); + try { + $options = $this->graphOptions($tenant); + } catch (ProviderConfigurationRequiredException $exception) { + return [ + 'success' => false, + 'error_message' => $exception->getMessage(), + 'requires_consent' => $exception->reasonCode === ProviderReasonCodes::ProviderConsentMissing, + ]; + } if ($options['tenant'] === null) { return [ @@ -52,11 +66,11 @@ public function testConnectivity(Tenant $tenant): array } /** - * @return array{tenant:?string,client_id:?string,client_secret:?string} + * @return array */ public function graphOptions(Tenant $tenant): array { - return $tenant->graphOptions(); + return $this->graphOptionsResolver->resolveForTenant($tenant); } private function requiresConsent(string $message): bool diff --git a/app/Services/Intune/TenantPermissionService.php b/app/Services/Intune/TenantPermissionService.php index ec72650..42e1ef4 100644 --- a/app/Services/Intune/TenantPermissionService.php +++ b/app/Services/Intune/TenantPermissionService.php @@ -5,12 +5,16 @@ use App\Models\Tenant; use App\Models\TenantPermission; use App\Services\Graph\GraphClientInterface; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use DateTimeInterface; use Illuminate\Support\Carbon; class TenantPermissionService { - public function __construct(private readonly GraphClientInterface $graphClient) {} + public function __construct( + private readonly GraphClientInterface $graphClient, + private readonly MicrosoftGraphOptionsResolver $graphOptionsResolver, + ) {} /** * @return array}> @@ -73,11 +77,14 @@ public function compare( if ($liveCheck && $grantedStatuses === null) { $liveCheckMeta['attempted'] = true; + if (! is_array($graphOptions)) { + $graphOptions = $this->graphOptionsResolver->resolveForTenant($tenant); + } + $appId = null; - if (is_array($graphOptions) && is_string($graphOptions['client_id'] ?? null) && $graphOptions['client_id'] !== '') { + + if (is_string($graphOptions['client_id'] ?? null) && $graphOptions['client_id'] !== '') { $appId = (string) $graphOptions['client_id']; - } elseif (is_string($tenant->graphOptions()['client_id'] ?? null) && $tenant->graphOptions()['client_id'] !== '') { - $appId = (string) $tenant->graphOptions()['client_id']; } if ($appId !== null) { @@ -328,8 +335,10 @@ private function configuredGrantedKeys(): array private function fetchLivePermissions(Tenant $tenant, ?array $graphOptions = null): array { try { + $graphOptions ??= $this->graphOptionsResolver->resolveForTenant($tenant); + $response = $this->graphClient->getServicePrincipalPermissions( - $graphOptions ?? $tenant->graphOptions() + $graphOptions ); if (! $response->success) { diff --git a/app/Services/Intune/VersionService.php b/app/Services/Intune/VersionService.php index 24e8b5c..ea8c4f5 100644 --- a/app/Services/Intune/VersionService.php +++ b/app/Services/Intune/VersionService.php @@ -9,6 +9,7 @@ use App\Services\Graph\AssignmentFilterResolver; use App\Services\Graph\GroupResolver; use App\Services\Graph\ScopeTagResolver; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use Carbon\CarbonImmutable; use Illuminate\Database\QueryException; use Illuminate\Database\UniqueConstraintViolationException; @@ -23,6 +24,7 @@ public function __construct( private readonly GroupResolver $groupResolver, private readonly AssignmentFilterResolver $assignmentFilterResolver, private readonly ScopeTagResolver $scopeTagResolver, + private readonly MicrosoftGraphOptionsResolver $graphOptionsResolver, ) {} public function captureVersion( @@ -119,8 +121,8 @@ public function captureFromGraph( bool $includeAssignments = true, bool $includeScopeTags = true, ): PolicyVersion { - $graphOptions = $tenant->graphOptions(); - $tenantIdentifier = $graphOptions['tenant'] ?? $tenant->graphTenantId() ?? (string) $tenant->getKey(); + $graphOptions = $this->graphOptionsResolver->resolveForTenant($tenant); + $tenantIdentifier = (string) ($graphOptions['tenant'] ?? ''); $snapshot = $this->snapshotService->fetch($tenant, $policy, $createdBy); diff --git a/app/Services/Providers/MicrosoftGraphOptionsResolver.php b/app/Services/Providers/MicrosoftGraphOptionsResolver.php new file mode 100644 index 0000000..6284a77 --- /dev/null +++ b/app/Services/Providers/MicrosoftGraphOptionsResolver.php @@ -0,0 +1,31 @@ + + */ + public function resolveForTenant(Tenant $tenant, array $overrides = []): array + { + $resolution = $this->connections->resolveDefault($tenant, 'microsoft'); + + if (! $resolution->resolved || $resolution->connection === null) { + throw ProviderConfigurationRequiredException::forTenant( + $tenant, + provider: 'microsoft', + resolution: $resolution, + ); + } + + return $this->gateway->graphOptions($resolution->connection, $overrides); + } +} diff --git a/app/Services/Providers/ProviderConfigurationRequiredException.php b/app/Services/Providers/ProviderConfigurationRequiredException.php new file mode 100644 index 0000000..24ef883 --- /dev/null +++ b/app/Services/Providers/ProviderConfigurationRequiredException.php @@ -0,0 +1,30 @@ +getKey(), + provider: $provider, + reasonCode: $resolution->effectiveReasonCode(), + extensionReasonCode: $resolution->extensionReasonCode, + message: $resolution->message ?? 'Provider configuration is required.', + ); + } +} diff --git a/specs/087-legacy-runs-removal/analysis-report.md b/specs/087-legacy-runs-removal/analysis-report.md new file mode 100644 index 0000000..961816e --- /dev/null +++ b/specs/087-legacy-runs-removal/analysis-report.md @@ -0,0 +1,770 @@ +# Spec 087 — Legacy Runs Removal: Analysis Report + +> **Report Date:** 2026-02-11 +> **Scope:** Analyse → Backfill → Cleanup → Drop for all non-canonical run tracking tables. +> **Canonical Run System:** `operation_runs` table + `OperationRun` model + `/admin/operations/{run}` + +--- + +## 1) Executive Summary + +### Legacy Sources Identified: **4** (plus 1 already dropped) + +| # | Source Table | Classification | Current State | Risk | +|---|---|---|---|---| +| 1 | `inventory_sync_runs` | **Legacy run tracking** | Active writes, has FK `operation_run_id` (nullable) | Medium — Drift depends on this as entity | +| 2 | `entra_group_sync_runs` | **Legacy run tracking** | Active writes, has FK `operation_run_id` (nullable) | Low — hidden nav, small footprint | +| 3 | `backup_schedule_runs` | **Legacy run tracking** | Active writes, has FK `operation_run_id` (nullable) | High — scheduler writes here every minute | +| 4 | `restore_runs` | **Domain entity** (NOT purely legacy tracking) | Active writes, has FK `operation_run_id` (nullable), SoftDeletes | High — 1,980-line resource, wizard, 20+ tests | +| 5 | `bulk_operation_runs` | **Already dropped** | Migration `2026_01_18` drops table; guard test prevents references | None — done | + +### Biggest Risks + +1. **`restore_runs` is a domain entity**, not just run tracking — it holds `requested_items`, `preview`, `results`, `group_mapping`, `metadata`. Its execution tracking (status, timestamps, counts) should come from `operation_runs`, but the entity itself must survive. **It cannot be dropped.** +2. **Drift detection** depends on `InventorySyncRun` as a domain entity for baseline/current run references (`Finding.baseline_run_id`, `Finding.current_run_id`, `InventoryItem.last_seen_run_id`). Dropping this table requires migrating those FKs to `operation_run_id`. +3. **BackupScheduleRun** is written by `RunBackupScheduleJob` on every scheduled execution — the scheduler produces ~N rows/minute where N = active schedules. Backfill must handle high volume. +4. **Graph API calls in UI** are limited to `TenantResource.php` (RBAC setup modal, not in run rendering) — **no run-related UI makes Graph calls**. + +### Quick Wins + +- `EntraGroupSyncRunResource` (`$shouldRegisterNavigation = false`) — lowest friction to remove. +- `BulkOperationRun` — already dropped, guard test exists. +- Both `InventorySyncRunResource.ViewInventorySyncRun` and `EntraGroupSyncRunResource.ViewEntraGroupSyncRun` already redirect to canonical `TenantlessOperationRunViewer` when `operation_run_id` is set. + +--- + +## 2) Inventory: Table-by-Table + +### 2a) `inventory_sync_runs` + +| Aspect | Detail | +|---|---| +| **Migration** | `2026_01_07_142719_create_inventory_sync_runs_table.php` | +| **Columns** | `id`, `tenant_id` (FK), `user_id` (FK, nullable, added `2026_01_09`), `selection_hash` (64), `selection_payload` (jsonb, nullable), `status` (string), `had_errors` (bool), `error_codes` (jsonb), `error_context` (jsonb), `started_at` (timestampTz), `finished_at` (timestampTz), `items_observed_count`, `items_upserted_count`, `errors_count`, `operation_run_id` (FK, nullable, added `2026_02_10`), `created_at`, `updated_at` | +| **FK Relationships** | `tenant_id → tenants`, `user_id → users`, `operation_run_id → operation_runs` | +| **Indexes** | `(tenant_id, selection_hash)`, `(tenant_id, status)`, `(tenant_id, finished_at)`, `(tenant_id, user_id)`, `operation_run_id` | +| **Model** | `App\Models\InventorySyncRun` (59 lines) | +| **Status constants** | `STATUS_PENDING`, `STATUS_RUNNING`, `STATUS_SUCCESS`, `STATUS_PARTIAL`, `STATUS_FAILED`, `STATUS_SKIPPED` | +| **Factory** | `InventorySyncRunFactory` ✓ | +| **Row count** | unknown (SQL: `SELECT COUNT(*) FROM inventory_sync_runs;`) | +| **Classification** | **Hybrid** — legacy run tracking BUT also used as domain entity by Drift (`Finding.baseline_run_id`, `Finding.current_run_id`, `InventoryItem.last_seen_run_id`) | +| **Inbound FKs** | `findings.baseline_run_id`, `findings.current_run_id`, `inventory_items.last_seen_run_id` (all → `inventory_sync_runs.id`), `operation_runs` ← via `inventory_sync_runs.operation_run_id` | + +**Start surfaces that write to this table:** +| Surface | File | Line(s) | +|---|---|---| +| `InventorySyncService::startSync()` | `app/Services/Inventory/InventorySyncService.php` | L43, L62, L68 | + +**Filament UI:** +| Component | File | Lines | +|---|---|---| +| `InventorySyncRunResource` | `app/Filament/Resources/InventorySyncRunResource.php` | 231 | +| `ListInventorySyncRuns` | `…/Pages/ListInventorySyncRuns.php` | 19 | +| `ViewInventorySyncRun` (redirects to OpRun) | `…/Pages/ViewInventorySyncRun.php` | 24 | +| `InventoryKpiHeader` widget (reads last sync) | `app/Filament/Widgets/Inventory/InventoryKpiHeader.php` | 163 | +| Nav: registered in Inventory cluster, sort=2, label="Sync History" | — | — | + +--- + +### 2b) `entra_group_sync_runs` + +| Aspect | Detail | +|---|---| +| **Migration** | `2026_01_11_120004_create_entra_group_sync_runs_table.php` | +| **Columns** | `id`, `tenant_id` (FK, cascadeOnDelete), `selection_key`, `slot_key` (nullable), `status`, `error_code` (nullable), `error_category` (nullable), `error_summary` (text, nullable), `safety_stop_triggered` (bool), `safety_stop_reason` (nullable), `pages_fetched`, `items_observed_count`, `items_upserted_count`, `error_count`, `initiator_user_id` (FK users, nullable), `started_at` (timestampTz), `finished_at` (timestampTz), `operation_run_id` (FK, nullable, added `2026_02_10`), `created_at`, `updated_at` | +| **FK Relationships** | `tenant_id → tenants`, `initiator_user_id → users`, `operation_run_id → operation_runs` | +| **Indexes** | `(tenant_id, selection_key)`, `(tenant_id, status)`, `(tenant_id, finished_at)`, `UNIQUE(tenant_id, selection_key, slot_key)`, `operation_run_id` | +| **Model** | `App\Models\EntraGroupSyncRun` (40 lines) | +| **Status constants** | `STATUS_PENDING`, `STATUS_RUNNING`, `STATUS_SUCCEEDED`, `STATUS_FAILED`, `STATUS_PARTIAL` | +| **Factory** | `EntraGroupSyncRunFactory` ✓ | +| **Row count** | unknown (SQL: `SELECT COUNT(*) FROM entra_group_sync_runs;`) | +| **Classification** | **Pure legacy run tracking** — no inbound FKs from other domain tables | +| **Inbound FKs** | None | + +**Start surfaces that write to this table:** +| Surface | File | Line(s) | +|---|---|---| +| `EntraGroupSyncJob::handle()` | `app/Jobs/EntraGroupSyncJob.php` | L56-L227 | + +**Filament UI:** +| Component | File | Lines | +|---|---|---| +| `EntraGroupSyncRunResource` | `app/Filament/Resources/EntraGroupSyncRunResource.php` | 168 | +| `ListEntraGroupSyncRuns` | `…/Pages/ListEntraGroupSyncRuns.php` | 11 | +| `ViewEntraGroupSyncRun` (redirects to OpRun) | `…/Pages/ViewEntraGroupSyncRun.php` | 24 | +| Nav: **hidden** (`$shouldRegisterNavigation = false`) | — | — | + +--- + +### 2c) `backup_schedule_runs` + +| Aspect | Detail | +|---|---| +| **Migration** | `2026_01_05_011034_create_backup_schedule_runs_table.php` | +| **Columns** | `id`, `backup_schedule_id` (FK, cascadeOnDelete), `tenant_id` (FK, cascadeOnDelete), `scheduled_for` (datetime), `started_at` (datetime, nullable), `finished_at` (datetime, nullable), `status` (enum: running/success/partial/failed/canceled/skipped), `summary` (json, nullable), `error_code` (nullable), `error_message` (text, nullable), `backup_set_id` (FK, nullable), `user_id` (FK, added `2026_01_06`), `operation_run_id` (FK, nullable, added `2026_02_10`), `created_at`, `updated_at` | +| **FK Relationships** | `backup_schedule_id → backup_schedules`, `tenant_id → tenants`, `backup_set_id → backup_sets`, `user_id → users`, `operation_run_id → operation_runs` | +| **Indexes** | `UNIQUE(backup_schedule_id, scheduled_for)`, `(backup_schedule_id, scheduled_for)`, `(tenant_id, created_at)`, `operation_run_id` | +| **Model** | `App\Models\BackupScheduleRun` (53 lines) | +| **Status constants** | `STATUS_RUNNING`, `STATUS_SUCCESS`, `STATUS_PARTIAL`, `STATUS_FAILED`, `STATUS_CANCELED`, `STATUS_SKIPPED` | +| **Factory** | **None** | +| **Row count** | unknown (SQL: `SELECT COUNT(*) FROM backup_schedule_runs;`) | +| **Classification** | **Legacy run tracking** — but references `backup_set_id` (produced artifact) | +| **Inbound FKs** | None | + +**Start surfaces that write to this table:** +| Surface | File | Line(s) | +|---|---|---| +| `RunBackupScheduleJob::handle()` | `app/Jobs/RunBackupScheduleJob.php` | L40, L80, L119-L1035 | + +**Filament UI:** +| Component | File | Lines | +|---|---|---| +| `BackupScheduleRunsRelationManager` | `app/Filament/Resources/BackupScheduleResource/RelationManagers/BackupScheduleRunsRelationManager.php` | 107 | +| `BackupScheduleOperationRunsRelationManager` (canonical) | `…/RelationManagers/BackupScheduleOperationRunsRelationManager.php` | 96 | +| Blade view: `backup-schedule-run-view.blade.php` (modal) | `resources/views/filament/modals/backup-schedule-run-view.blade.php` | 48 | +| Nav: no dedicated nav; shown as RelationManager tab on BackupSchedule | — | — | + +--- + +### 2d) `restore_runs` + +| Aspect | Detail | +|---|---| +| **Migration** | `2025_12_10_000150_create_restore_runs_table.php` + several amendments | +| **Columns** | `id`, `tenant_id` (FK, cascadeOnDelete), `backup_set_id` (FK, cascadeOnDelete), `requested_by` (nullable), `is_dry_run` (bool), `status` (string, uses `RestoreRunStatus` enum), `requested_items` (json), `preview` (json), `results` (json), `failure_reason` (text, nullable), `metadata` (json), `group_mapping` (json), `idempotency_key` (string, nullable), `started_at`, `completed_at`, `operation_run_id` (FK, nullable, added `2026_02_10`), `deleted_at` (soft deletes), `created_at`, `updated_at` | +| **FK Relationships** | `tenant_id → tenants`, `backup_set_id → backup_sets`, `operation_run_id → operation_runs` | +| **Indexes** | `(tenant_id, status)`, `started_at`, `completed_at`, `operation_run_id` | +| **Model** | `App\Models\RestoreRun` (157 lines, `SoftDeletes`) | +| **Status enum** | `RestoreRunStatus` (13 cases: Draft→Scoped→Checked→Previewed→Pending→Queued→Running→Completed→Partial→Failed→Cancelled→Aborted→CompletedWithErrors) | +| **Factory** | `RestoreRunFactory` ✓ | +| **Row count** | unknown (SQL: `SELECT COUNT(*) FROM restore_runs;`) | +| **Classification** | **Domain entity** — NOT droppable. Holds wizard state, preview diffs, group mappings, results. Execution tracking portion migrates to `operation_runs`. | +| **Inbound FKs** | None from other tables (but `BackupSet.restoreRuns()` hasMany) | + +**Start surfaces that write to this table:** +| Surface | File | Line(s) | +|---|---|---| +| `RestoreService::startRestore()` | `app/Services/Intune/RestoreService.php` | L120, L208, L213 | +| `RestoreRunResource` wizard | `app/Filament/Resources/RestoreRunResource.php` | ~1,980 lines total | +| `RestoreRunObserver` → `SyncRestoreRunToOperationRun` | `app/Observers/RestoreRunObserver.php` | L13-L30 | +| `ExecuteRestoreRunJob` | `app/Jobs/ExecuteRestoreRunJob.php` | L29-L155 | + +**Filament UI:** +| Component | File | Lines | +|---|---|---| +| `RestoreRunResource` (largest resource) | `app/Filament/Resources/RestoreRunResource.php` | 1,980 | +| `ListRestoreRuns` | `…/Pages/ListRestoreRuns.php` | 21 | +| `CreateRestoreRun` (wizard) | `…/Pages/CreateRestoreRun.php` | 167 | +| `ViewRestoreRun` | `…/Pages/ViewRestoreRun.php` | 11 | +| Blade: `restore-run-preview.blade.php` | `resources/views/filament/forms/components/restore-run-preview.blade.php` | 180 | +| Blade: `restore-run-checks.blade.php` | `resources/views/filament/forms/components/restore-run-checks.blade.php` | 121 | +| Nav: registered in tenant panel under "Backups & Restore" | — | — | + +--- + +### 2e) `bulk_operation_runs` — **ALREADY DROPPED** + +| Aspect | Detail | +|---|---| +| **Drop migration** | `2026_01_18_000001_drop_bulk_operation_runs_table.php` | +| **Model** | Deleted — `BulkOperationRun` class does not exist | +| **Guard test** | `tests/Feature/Guards/NoLegacyBulkOperationsTest.php` — scans codebase for `\bBulkOperationRun\b` | +| **Status** | ✅ Complete. No action needed. | + +--- + +## 3) Backfill Plan per Source + +### Existing Infrastructure + +All 4 legacy tables already have `operation_run_id` FK (nullable, added `2026_02_10_*`). This means: +- **New runs** may already link to `operation_runs` (depends on whether the write path sets the FK). +- **Historical runs** (before 2026-02-10) have `operation_run_id = NULL` and need backfill. + +### 3a) `inventory_sync_runs` → `operation_runs` Backfill + +``` +OperationRun mapping: +├── type = 'inventory.sync' (OperationRunType::InventorySync) +├── status = legacy.status mapped: +│ ├── pending → queued +│ ├── running → running +│ ├── success → completed +│ ├── partial → completed +│ ├── failed → completed +│ └── skipped → completed +├── outcome = legacy.status mapped: +│ ├── pending → pending +│ ├── running → pending +│ ├── success → succeeded +│ ├── partial → partially_succeeded +│ ├── failed → failed +│ └── skipped → cancelled +├── run_identity_hash = sha256("{tenant_id}|inventory.sync|{selection_hash}") +├── summary_counts = { +│ total: legacy.items_observed_count, +│ succeeded: legacy.items_upserted_count, +│ failed: legacy.errors_count +│ } +├── failure_summary = legacy.error_codes?.map(c => {code: c, reason_code: 'unknown', message: ''}) ?? [] +├── context = { +│ inputs: { selection_hash: legacy.selection_hash }, +│ selection_payload: legacy.selection_payload, +│ legacy: { source: 'inventory_sync_runs', id: legacy.id } +│ } +├── tenant_id = legacy.tenant_id +├── user_id = legacy.user_id +├── initiator_name = User.name ?? 'system' +├── started_at = legacy.started_at +├── completed_at = legacy.finished_at +├── created_at = legacy.created_at +└── updated_at = legacy.updated_at +``` + +**Missing data:** None significant. `user_id` may be NULL for system-initiated syncs. + +**Post-backfill:** Set `inventory_sync_runs.operation_run_id = created_operation_run.id`. + +**Drift FK migration:** After backfill, update: +- `findings.baseline_run_id` → new column `findings.baseline_operation_run_id` +- `findings.current_run_id` → new column `findings.current_operation_run_id` +- `inventory_items.last_seen_run_id` → new column `inventory_items.last_seen_operation_run_id` + +--- + +### 3b) `entra_group_sync_runs` → `operation_runs` Backfill + +``` +OperationRun mapping: +├── type = 'directory_groups.sync' (OperationRunType::DirectoryGroupsSync) +├── status = legacy.status mapped: +│ ├── pending → queued +│ ├── running → running +│ ├── succeeded → completed +│ ├── failed → completed +│ └── partial → completed +├── outcome = legacy.status mapped: +│ ├── pending → pending +│ ├── running → pending +│ ├── succeeded → succeeded +│ ├── failed → failed +│ └── partial → partially_succeeded +├── run_identity_hash = sha256("{tenant_id}|directory_groups.sync|{selection_key}:{slot_key}") +├── summary_counts = { +│ total: legacy.items_observed_count, +│ succeeded: legacy.items_upserted_count, +│ failed: legacy.error_count, +│ items: legacy.pages_fetched +│ } +├── failure_summary = legacy.error_code ? +│ [{code: legacy.error_code, reason_code: legacy.error_category ?? 'unknown', +│ message: substr(legacy.error_summary, 0, 120)}] : [] +├── context = { +│ inputs: { selection_key: legacy.selection_key, slot_key: legacy.slot_key }, +│ safety_stop: { triggered: legacy.safety_stop_triggered, reason: legacy.safety_stop_reason }, +│ legacy: { source: 'entra_group_sync_runs', id: legacy.id } +│ } +├── tenant_id = legacy.tenant_id +├── user_id = legacy.initiator_user_id +├── initiator_name = User.name ?? 'scheduler' +├── started_at = legacy.started_at +├── completed_at = legacy.finished_at +├── created_at = legacy.created_at +└── updated_at = legacy.updated_at +``` + +**Missing data:** None — all fields map cleanly. + +**Post-backfill:** Set `entra_group_sync_runs.operation_run_id = created_operation_run.id`. + +--- + +### 3c) `backup_schedule_runs` → `operation_runs` Backfill + +``` +OperationRun mapping: +├── type = legacy→type mapping: +│ └── All historical: 'backup_schedule.scheduled' (OperationRunType::BackupScheduleScheduled) +│ NOTE: run_now/retry distinction is lost in historical data +├── status = legacy.status mapped: +│ ├── running → running +│ ├── success → completed +│ ├── partial → completed +│ ├── failed → completed +│ ├── canceled → completed +│ └── skipped → completed +├── outcome = legacy.status mapped: +│ ├── running → pending +│ ├── success → succeeded +│ ├── partial → partially_succeeded +│ ├── failed → failed +│ ├── canceled → cancelled +│ └── skipped → cancelled +├── run_identity_hash = sha256("{tenant_id}|backup_schedule.scheduled|{schedule_id}:{scheduled_for_iso}") +├── summary_counts = legacy.summary (JSON) — extract from structure: +│ { total: summary.total_policies ?? 0, succeeded: summary.backed_up ?? 0, +│ failed: summary.errors ?? 0 } +├── failure_summary = legacy.error_code ? +│ [{code: legacy.error_code, reason_code: 'unknown', message: substr(legacy.error_message, 0, 120)}] : [] +├── context = { +│ inputs: { +│ backup_schedule_id: legacy.backup_schedule_id, +│ scheduled_for: legacy.scheduled_for +│ }, +│ backup_set_id: legacy.backup_set_id, +│ legacy: { source: 'backup_schedule_runs', id: legacy.id } +│ } +├── tenant_id = legacy.tenant_id +├── user_id = legacy.user_id +├── initiator_name = User.name ?? 'scheduler' +├── started_at = legacy.started_at +├── completed_at = legacy.finished_at +├── created_at = legacy.created_at +└── updated_at = legacy.updated_at +``` + +**Missing data:** +- `type` distinction (run_now vs retry vs scheduled) is lost for historical rows — all backfilled as `backup_schedule.scheduled`. +- `summary` JSON structure varies — needs defensive parsing. + +**Post-backfill:** Set `backup_schedule_runs.operation_run_id = created_operation_run.id`. + +--- + +### 3d) `restore_runs` → `operation_runs` Backfill + +**NOTE:** `restore_runs` is a domain entity. Only the execution-tracking portion (status, timestamps, counts) is backfilled to `operation_runs`. The entity itself persists. + +``` +OperationRun mapping: +├── type = 'restore.execute' (OperationRunType::RestoreExecute) +│ NOTE: only Queued→Running→Completed states get an OpRun. +│ Draft/Scoped/Checked/Previewed are wizard states, NOT execution runs. +├── SKIP condition = legacy.status IN (Draft, Scoped, Checked, Previewed, Pending) → do NOT backfill +├── status = legacy.status mapped: +│ ├── Queued → queued +│ ├── Running → running +│ ├── Completed → completed +│ ├── Partial → completed +│ ├── Failed → completed +│ ├── Cancelled → completed +│ ├── Aborted → completed +│ └── CompletedWithErrors → completed +├── outcome = legacy.status mapped: +│ ├── Queued → pending +│ ├── Running → pending +│ ├── Completed → succeeded +│ ├── Partial → partially_succeeded +│ ├── Failed → failed +│ ├── Cancelled → cancelled +│ ├── Aborted → failed +│ └── CompletedWithErrors → partially_succeeded +├── run_identity_hash = sha256("{tenant_id}|restore.execute|{restore_run_id}") +├── summary_counts = derived from legacy.results JSON: +│ { total: count(results.items), succeeded: count(results where ok), failed: count(results where !ok) } +├── failure_summary = legacy.failure_reason ? +│ [{code: 'restore.failed', reason_code: 'unknown', message: substr(legacy.failure_reason, 0, 120)}] : [] +├── context = { +│ inputs: { restore_run_id: legacy.id, backup_set_id: legacy.backup_set_id }, +│ is_dry_run: legacy.is_dry_run, +│ legacy: { source: 'restore_runs', id: legacy.id } +│ } +├── tenant_id = legacy.tenant_id +├── user_id = NULL (legacy has requested_by as string, not FK) +├── initiator_name = legacy.requested_by ?? 'system' +├── started_at = legacy.started_at +├── completed_at = legacy.completed_at +├── created_at = legacy.created_at +└── updated_at = legacy.updated_at +``` + +**Missing data:** +- `user_id` — `requested_by` is a string name, not an FK. Backfill can attempt `User::where('name', requested_by)->first()` but may not resolve. +- `summary_counts` — `results` JSON structure varies; needs defensive parsing. + +**Post-backfill:** Set `restore_runs.operation_run_id = created_operation_run.id`. + +**Existing sync infrastructure:** +- `RestoreRunObserver` already syncs RestoreRun → OperationRun on create/update via `SyncRestoreRunToOperationRun`. +- `AdapterRunReconciler` reconciles RestoreRun status → OperationRun via scheduled `ReconcileAdapterRunsJob` (every 30 min). + +--- + +### 3e) Existing `operation_run_id` Link Check + +All 4 legacy tables already have `operation_run_id` FK (nullable): + +| Table | Migration | FK constraint | On delete | +|---|---|---|---| +| `inventory_sync_runs` | `2026_02_10_090213` | `constrained('operation_runs')` | `nullOnDelete` | +| `entra_group_sync_runs` | `2026_02_10_090214` | `constrained('operation_runs')` | `nullOnDelete` | +| `backup_schedule_runs` | `2026_02_10_090215` | `constrained('operation_runs')` | `nullOnDelete` | +| `restore_runs` | `2026_02_10_115908` | `constrained('operation_runs')` | `nullOnDelete` | + +**Current behavior:** New write paths (post 2026-02-10) *should* be populating this FK, but needs verification per table: +- `InventorySyncService` — needs audit (may not set FK yet) +- `EntraGroupSyncJob` — needs audit +- `RunBackupScheduleJob` — needs audit (reconcile command exists for this) +- `RestoreRunObserver` → `SyncRestoreRunToOperationRun` — **actively syncs** (confirmed) + +SQL to verify: +```sql +SELECT 'inventory_sync_runs' AS tbl, COUNT(*) AS total, + COUNT(operation_run_id) AS linked, COUNT(*) - COUNT(operation_run_id) AS unlinked +FROM inventory_sync_runs +UNION ALL +SELECT 'entra_group_sync_runs', COUNT(*), COUNT(operation_run_id), COUNT(*) - COUNT(operation_run_id) +FROM entra_group_sync_runs +UNION ALL +SELECT 'backup_schedule_runs', COUNT(*), COUNT(operation_run_id), COUNT(*) - COUNT(operation_run_id) +FROM backup_schedule_runs +UNION ALL +SELECT 'restore_runs', COUNT(*), COUNT(operation_run_id), COUNT(*) - COUNT(operation_run_id) +FROM restore_runs; +``` + +--- + +## 4) Cleanup Plan + +### 4a) Code Removal + +#### Phase 1: `EntraGroupSyncRunResource` (Quick Win) + +| What | File | Action | +|---|---|---| +| Resource | `app/Filament/Resources/EntraGroupSyncRunResource.php` (168 lines) | Delete | +| List page | `…/Pages/ListEntraGroupSyncRuns.php` (11 lines) | Delete | +| View page | `…/Pages/ViewEntraGroupSyncRun.php` (24 lines) | Delete | +| Policy | `app/Policies/EntraGroupSyncRunPolicy.php` | Delete | +| Badge class | `app/Support/Badges/Domains/EntraGroupSyncRunStatusBadge.php` | Delete | +| Badge enum case | `BadgeDomain::EntraGroupSyncRun` → remove case | Edit | +| Badge catalog mapping | `BadgeCatalog` → remove mapping | Edit | +| Notification link | `RunStatusChangedNotification.php` → remove entra branch | Edit | + +#### Phase 2: `InventorySyncRunResource` (Medium — drift dependency) + +| What | File | Action | +|---|---|---| +| Resource | `app/Filament/Resources/InventorySyncRunResource.php` (231 lines) | Delete | +| List page | `…/Pages/ListInventorySyncRuns.php` (19 lines) | Delete | +| View page | `…/Pages/ViewInventorySyncRun.php` (24 lines) | Delete | +| Badge class | `app/Support/Badges/Domains/InventorySyncRunStatusBadge.php` | Delete | +| Badge enum case | `BadgeDomain::InventorySyncRun` → remove case | Edit | +| Badge catalog mapping | `BadgeCatalog` → remove mapping | Edit | +| KPI widget refs | `InventoryKpiHeader.php` → update to query `operation_runs` only | Edit | +| Nav item | Remove "Sync History" from Inventory cluster | — | + +**Prerequisite:** Drift tables must migrate FKs from `inventory_sync_runs.id` → `operation_runs.id`: +- `findings.baseline_run_id` + `findings.current_run_id` → `findings.baseline_operation_run_id` + `findings.current_operation_run_id` +- `inventory_items.last_seen_run_id` → `inventory_items.last_seen_operation_run_id` + +#### Phase 3: `BackupScheduleRunsRelationManager` (Medium) + +| What | File | Action | +|---|---|---| +| Legacy RM | `BackupScheduleRunsRelationManager.php` (107 lines) | Delete | +| Blade modal | `backup-schedule-run-view.blade.php` (48 lines) | Delete | +| Badge class | `app/Support/Badges/Domains/BackupScheduleRunStatusBadge.php` | Delete | +| Badge enum case | `BadgeDomain::BackupScheduleRun` → remove case | Edit | +| Badge catalog mapping | `BadgeCatalog` → remove mapping | Edit | +| BackupSchedule resource | Remove legacy RM, keep `BackupScheduleOperationRunsRelationManager` | Edit | +| Reconcile command | `TenantpilotReconcileBackupScheduleOperationRuns` → keep for transition, then archive | — | + +#### Phase 4: `RestoreRun` (Largest — domain entity) + +**`RestoreRun` is NOT removed** — it is a domain entity. The cleanup targets: + +| What | File | Action | +|---|---|---| +| Status tracking columns | Remove `status` duplication → read from `operationRun.status` | Deprecate, keep column | +| `started_at`, `completed_at` | Redundant with `operationRun.started_at/completed_at` | Deprecate, keep column | +| `failure_reason` | Move to `operationRun.failure_summary` | Deprecate, keep column | +| `RestoreRunObserver` sync | Keep (ensures OpRun is created on RestoreRun lifecycle) | Keep | +| `AdapterRunReconciler` | Eventually remove once all status reads go through OpRun | Keep for transition | +| Badge class | `app/Support/Badges/Domains/RestoreRunBadges` → derive from OpRun status | Edit | + +### 4b) Service/Job Cleanup + +| File | Action | +|---|---| +| `InventorySyncService.php` | Stop writing `InventorySyncRun::create()`, create only `OperationRun` | +| `EntraGroupSyncJob.php` | Stop writing `EntraGroupSyncRun::create()`, use only `OperationRun` | +| `RunBackupScheduleJob.php` | Stop writing `BackupScheduleRun::create()`, use only `OperationRun` | +| `ApplyBackupScheduleRetentionJob.php` | Update to read retention data from `OperationRun` context | +| `TenantpilotPurgeNonPersistentData.php` | Remove legacy table references | +| DriftRunSelector, DriftScopeKey, DriftFindingGenerator | Update to use `OperationRun` (after FK migration) | + +### 4c) Model Cleanup (after drop) + +| Model | Action | +|---|---| +| `InventorySyncRun` | Delete (after drift FK migration + table drop) | +| `EntraGroupSyncRun` | Delete (after table drop) | +| `BackupScheduleRun` | Delete (after table drop) | +| `RestoreRun` | **Keep** — domain entity | +| Factories for above | Delete respective factories | + +--- + +## 5) Redirect Strategy + +### Existing Routes for Legacy Run Views + +| Pattern | Name | Current Handler | +|---|---|---| +| `GET /admin/t/{tenant}/inventory-sync-runs/{record}` | `filament.tenant.resources.inventory-sync-runs.view` | `ViewInventorySyncRun` | +| `GET /admin/t/{tenant}/inventory-sync-runs` | `filament.tenant.resources.inventory-sync-runs.index` | `ListInventorySyncRuns` | +| `GET /admin/entra-group-sync-runs/{record}` | `filament.admin.resources.entra-group-sync-runs.view` | `ViewEntraGroupSyncRun` | +| `GET /admin/entra-group-sync-runs` | `filament.admin.resources.entra-group-sync-runs.index` | `ListEntraGroupSyncRuns` | +| `GET /admin/t/{tenant}/restore-runs` | `filament.tenant.resources.restore-runs.index` | `ListRestoreRuns` | +| `GET /admin/t/{tenant}/restore-runs/create` | `filament.tenant.resources.restore-runs.create` | `CreateRestoreRun` | +| `GET /admin/t/{tenant}/restore-runs/{record}` | `filament.tenant.resources.restore-runs.view` | `ViewRestoreRun` | + +### Redirect Logic + +For **dropped** resources (inventory sync, entra group sync): + +```php +// Register in routes/web.php after Resources are deleted +Route::get('/admin/t/{tenant}/inventory-sync-runs/{record}', function ($tenant, $record) { + $opRunId = DB::table('inventory_sync_runs') + ->where('id', $record)->value('operation_run_id'); + if ($opRunId) { + return redirect()->route('admin.operations.view', ['run' => $opRunId]); + } + abort(404); +}); + +// List pages → redirect to Operations index +Route::get('/admin/t/{tenant}/inventory-sync-runs', fn() => + redirect()->route('admin.operations.index')); + +// Same pattern for entra-group-sync-runs +``` + +**Constraint:** Redirect must NEVER create new `OperationRun` rows — only lookup + redirect. + +For `backup_schedule_runs`: No direct URL exists (only RelationManager modal), so no redirect needed. + +For `restore_runs`: **Not dropped** — resource stays. No redirect needed. + +--- + +## 6) Drop Plan + +### Tables to Drop (in order) + +| # | Table | Preconditions | Migration name | +|---|---|---|---| +| 1 | `entra_group_sync_runs` | All rows have `operation_run_id` ≠ NULL; no inbound FKs | `drop_entra_group_sync_runs_table` | +| 2 | `backup_schedule_runs` | All rows have `operation_run_id` ≠ NULL; no inbound FKs | `drop_backup_schedule_runs_table` | +| 3 | `inventory_sync_runs` | All rows have `operation_run_id` ≠ NULL; drift FKs migrated to `operation_run_id` | `drop_inventory_sync_runs_table` | + +**NOT dropped:** `restore_runs` (domain entity), `operation_runs` (canonical). + +### Preconditions per Table + +#### `entra_group_sync_runs` +- [ ] Backfill complete: `SELECT COUNT(*) FROM entra_group_sync_runs WHERE operation_run_id IS NULL` = 0 +- [ ] Verify count matches: `SELECT COUNT(*) FROM entra_group_sync_runs` ≈ `SELECT COUNT(*) FROM operation_runs WHERE type = 'directory_groups.sync' AND context->>'legacy'->>'source' = 'entra_group_sync_runs'` +- [ ] No code writes to table: arch guard test passes (`grep -r 'EntraGroupSyncRun' app/ | wc -l` = 0, excluding model file) +- [ ] Filament resource deleted +- [ ] Redirect route registered +- [ ] DB snapshot taken + +#### `backup_schedule_runs` +- [ ] Backfill complete: `SELECT COUNT(*) FROM backup_schedule_runs WHERE operation_run_id IS NULL` = 0 +- [ ] `RunBackupScheduleJob` writes only to `operation_runs` +- [ ] `TenantpilotReconcileBackupScheduleOperationRuns` command removed/archived +- [ ] `ApplyBackupScheduleRetentionJob` reads from `operation_runs` +- [ ] DB snapshot taken + +#### `inventory_sync_runs` +- [ ] Backfill complete: all rows linked +- [ ] Drift FK migration complete: `findings.*_run_id` + `inventory_items.last_seen_run_id` → `operation_run_id` columns +- [ ] `InventorySyncService` writes only to `operation_runs` +- [ ] All Drift services read from `OperationRun` +- [ ] DB snapshot taken + +### Release Gates + +Each drop migration should: +1. Check precondition (backfill count) in `up()` — abort if unlinked rows exist +2. Create backup table `_archive_{table}` before dropping (for rollback) +3. Drop FK constraints before dropping table +4. Be reversible: `down()` recreates table from archive + +--- + +## 7) Hotspot Files (Top 15) + +| # | File | Lines | Impact | Why it's hot | +|---|---|---|---|---| +| 1 | `app/Filament/Resources/RestoreRunResource.php` | 1,980 | High | Largest resource; full wizard, execution tracking, must decouple status reads | +| 2 | `app/Jobs/RunBackupScheduleJob.php` | ~1,035 | High | Heavy schedule→BackupScheduleRun writer; must rewrite to OpRun-only | +| 3 | `app/Services/OperationRunService.php` | 808 | Medium | Canonical service; backfill must use this; no breaking changes allowed | +| 4 | `app/Filament/Resources/OperationRunResource.php` | 483 | Medium | Canonical viewer; must absorb data previously shown in legacy resources | +| 5 | `app/Services/Inventory/InventorySyncService.php` | ~100 | Medium | Creates InventorySyncRun; must rewrite to OpRun-only | +| 6 | `app/Jobs/EntraGroupSyncJob.php` | ~227 | Medium | Creates EntraGroupSyncRun; must rewrite to OpRun-only | +| 7 | `app/Filament/Resources/InventorySyncRunResource.php` | 231 | Low | Will be deleted entirely | +| 8 | `app/Jobs/ExecuteRestoreRunJob.php` | ~155 | Medium | Bridges RestoreRun↔OpRun; sync logic must be verified | +| 9 | `app/Filament/Resources/EntraGroupSyncRunResource.php` | 168 | Low | Will be deleted entirely | +| 10 | `app/Filament/Pages/Monitoring/Operations.php` | 149 | Low | Must ensure all legacy run data is visible here after migration | +| 11 | `app/Filament/Pages/Operations/TenantlessOperationRunViewer.php` | 142 | Low | Canonical viewer page; needs "related links" for all legacy sources | +| 12 | `app/Filament/Widgets/Inventory/InventoryKpiHeader.php` | 163 | Medium | Reads InventorySyncRun; must switch to OpRun queries | +| 13 | `app/Listeners/SyncRestoreRunToOperationRun.php` | ~21 | Low | Existing bridge; must be verified/kept during transition | +| 14 | `app/Services/AdapterRunReconciler.php` | ~253 | Medium | Reconciles RestoreRun→OpRun; may be simplified or removed | +| 15 | `app/Console/Commands/TenantpilotReconcileBackupScheduleOperationRuns.php` | ~130 | Low | Reconcile command; kept during transition, then archived | + +--- + +## 8) Test Impact Map + +### Tests That WILL Break (must be updated/deleted) + +| # | Test File | Legacy Model | Action Required | +|---|---|---|---| +| 1 | `tests/Feature/Filament/InventorySyncRunResourceTest.php` | InventorySyncRun | Delete | +| 2 | `tests/Feature/Filament/EntraGroupSyncRunResourceTest.php` | EntraGroupSyncRun | Delete | +| 3 | `tests/Feature/Filament/InventoryPagesTest.php` | InventorySyncRunResource URL | Update URLs | +| 4 | `tests/Feature/Filament/InventoryHubDbOnlyTest.php` | InventorySyncRunResource URL | Update URLs | +| 5 | `tests/Feature/Guards/ActionSurfaceContractTest.php` | InventorySyncRunResource | Remove references | +| 6 | `tests/Feature/Operations/LegacyRunRedirectTest.php` | Both sync resources | Update to test redirect routes | +| 7 | `tests/Feature/Console/ReconcileBackupScheduleOperationRunsCommandTest.php` | BackupScheduleRun | Delete or convert | +| 8 | `tests/Feature/Console/PurgeNonPersistentDataCommandTest.php` | BackupScheduleRun + RestoreRun | Update to remove legacy refs | +| 9 | `tests/Feature/Rbac/EntraGroupSyncRunsUiEnforcementTest.php` | EntraGroupSyncRunResource | Delete | +| 10 | `tests/Feature/DirectoryGroups/StartSyncFromGroupsPageTest.php` | EntraGroupSyncRun counts | Update assertions | +| 11 | `tests/Feature/DirectoryGroups/ScheduledSyncDispatchTest.php` | EntraGroupSyncRun counts | Update assertions | +| 12 | `tests/Feature/RunStartAuthorizationTest.php` | All legacy models | Update to assert OpRun creation | + +### Tests That STAY (RestoreRun is domain entity): + +All 20 RestoreRun test files survive because RestoreRun persists. But tests that check `RestoreRun.status` directly may need to verify via `OperationRun` instead. + +### Tests That STAY (Drift uses InventorySyncRun): + +15 Drift test files — until the Drift FK migration is complete, these stay. After migration, they update to use `OperationRun` factories. + +### 3 New Tests Proposed + +#### Test 1: Backfill Idempotency +```php +it('backfill is idempotent — running twice produces the same operation_run links', function () { + // Create legacy rows, run backfill command, verify links + // Run backfill again, verify no duplicate OperationRuns created + // Assert operation_run_id unchanged on legacy rows +}); +``` + +#### Test 2: Verify Counts Match +```php +it('all legacy runs are linked to exactly one operation run after backfill', function () { + // For each legacy table: + // Assert COUNT(*) WHERE operation_run_id IS NULL = 0 + // Assert COUNT(DISTINCT operation_run_id) = COUNT(*) + // Assert OpRun count for type matches legacy count +}); +``` + +#### Test 3: Redirect Resolution +```php +it('legacy run view URLs redirect to canonical operation run viewer', function () { + $syncRun = InventorySyncRun::factory()->create(['operation_run_id' => $opRun->id]); + + $this->get("/admin/t/{$tenant->external_id}/inventory-sync-runs/{$syncRun->id}") + ->assertRedirect(route('admin.operations.view', ['run' => $opRun->id])); +}); + +it('returns 404 when legacy run has no operation_run_id', function () { + $syncRun = InventorySyncRun::factory()->create(['operation_run_id' => null]); + + $this->get("/admin/t/{$tenant->external_id}/inventory-sync-runs/{$syncRun->id}") + ->assertNotFound(); +}); +``` + +--- + +## 9) Graph/HTTP Call Sites in UI (Guardrail Audit) + +**Run-related rendering has ZERO Graph/HTTP calls.** ✅ + +The only Graph call sites in Filament UI are in `TenantResource.php` (RBAC setup modal): + +| Call Site | File | Lines | Risk | +|---|---|---|---| +| `searchRoleDefinitionsDelegated()` | `app/Filament/Resources/TenantResource.php` | L1267-L1271 | HIGH (keystroke-triggered) | +| `groupSearchOptions()` (scope_group_id) | `app/Filament/Resources/TenantResource.php` | L1387-L1395 | HIGH (keystroke-triggered) | +| `groupSearchOptions()` (existing_group_id) | `app/Filament/Resources/TenantResource.php` | L1387-L1395 | HIGH (keystroke-triggered) | + +These are **not run-related** and are not in scope for Spec 087, but documented for completeness. + +--- + +## 10) Search Queries Used + +Key ripgrep/search queries used to produce this report: + +```bash +rg 'create_.*runs.*_table|_runs_table' database/migrations/ +rg 'operation_run' database/migrations/ +rg 'InventorySyncRun' app/ tests/ --files-with-matches +rg 'EntraGroupSyncRun' app/ tests/ --files-with-matches +rg 'BackupScheduleRun' app/ tests/ --files-with-matches +rg 'RestoreRun' app/ tests/ --files-with-matches +rg 'BulkOperationRun' app/ tests/ --files-with-matches +rg 'getSearchResultsUsing|getOptionLabelUsing' app/Filament/ +rg 'GraphClient|Http::' app/Filament/ +rg 'OperationRunType' app/ --files-with-matches +rg '::create\(' app/Services/Inventory/ app/Jobs/EntraGroupSyncJob.php app/Jobs/RunBackupScheduleJob.php +find app/Models -name '*Run*.php' +find app/Filament -name '*Run*' -o -name '*Operation*' +find tests/ -name '*Run*' -o -name '*run*' +``` + +--- + +## 11) Implementation Sequence (Recommended) + +``` +Phase 0: Audit & verify current operation_run_id fill rates (SQL queries above) + ↓ +Phase 1: Backfill command (artisan tenantpilot:backfill-legacy-runs) + - Process each table independently, idempotent + - Write operation_run_id back to legacy rows + - Verify counts + ↓ +Phase 2: Cleanup Code — Quick Wins + 2a) Delete EntraGroupSyncRunResource + tests + badge + policy + 2b) Register redirect routes + 2c) Update guard tests + ↓ +Phase 3: Cleanup Code — Medium + 3a) Rewrite EntraGroupSyncJob to use OperationRun only + 3b) Delete InventorySyncRunResource + tests + badge + 3c) Migrate Drift FKs (new migration) + 3d) Rewrite InventorySyncService to use OperationRun only + 3e) Update InventoryKpiHeader widget + ↓ +Phase 4: Cleanup Code — Heavy + 4a) Rewrite RunBackupScheduleJob to use OperationRun only + 4b) Delete BackupScheduleRunsRelationManager + blade modal + badge + 4c) Delete reconcile command + ↓ +Phase 5: RestoreRun Decoupling + 5a) Deprecate status/timestamp reads from RestoreRun → read from OpRun + 5b) Keep RestoreRun entity (wizard state, results, group_mapping) + 5c) Simplify/remove AdapterRunReconciler + ↓ +Phase 6: Drop Migrations (with gates) + 6a) Drop entra_group_sync_runs (quick — no inbound FKs) + 6b) Drop backup_schedule_runs (medium — verify retention logic) + 6c) Drop inventory_sync_runs (last — after drift FK migration) + ↓ +Phase 7: Final Cleanup + 7a) Delete legacy models (InventorySyncRun, EntraGroupSyncRun, BackupScheduleRun) + 7b) Delete factories + 7c) Remove BadgeDomain enum cases + 7d) Update NoLegacyBulkOperationsTest guard scope to cover all dropped models +``` diff --git a/specs/088-remove-tenant-graphoptions-legacy/checklists/requirements.md b/specs/088-remove-tenant-graphoptions-legacy/checklists/requirements.md new file mode 100644 index 0000000..1383cc5 --- /dev/null +++ b/specs/088-remove-tenant-graphoptions-legacy/checklists/requirements.md @@ -0,0 +1,35 @@ +# Specification Quality Checklist: Remove Legacy Graph Options + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-02-11 +**Feature**: specs/088-remove-tenant-graphoptions-legacy/spec.md + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- Validation pass (2026-02-11): Spec is ready for planning. +- Optional DB cleanup is explicitly gated (only after zero usage confirmed). \ No newline at end of file diff --git a/specs/088-remove-tenant-graphoptions-legacy/contracts/README.md b/specs/088-remove-tenant-graphoptions-legacy/contracts/README.md new file mode 100644 index 0000000..1c5c8b8 --- /dev/null +++ b/specs/088-remove-tenant-graphoptions-legacy/contracts/README.md @@ -0,0 +1,11 @@ +# Contracts — Remove Legacy Tenant Graph Options + +This feature introduces **no new HTTP/API endpoints** and does not change the Microsoft Graph contract registry. + +## Why `contracts/` exists anyway +The SpecKit workflow expects a `contracts/` directory for features that introduce or modify external interfaces. + +## Contract impact for this feature +- **No changes** to `config/graph_contracts.php` are required. +- **No new** application routes/controllers are introduced. +- The change is limited to **internal configuration sourcing** (provider connection resolution + guardrail). diff --git a/specs/088-remove-tenant-graphoptions-legacy/data-model.md b/specs/088-remove-tenant-graphoptions-legacy/data-model.md new file mode 100644 index 0000000..7b5d43e --- /dev/null +++ b/specs/088-remove-tenant-graphoptions-legacy/data-model.md @@ -0,0 +1,40 @@ +# Data Model — Remove Legacy Tenant Graph Options + +## Summary +This feature is a behavioral refactor only. It changes **how Graph credentials/options are sourced** (provider connection only) and adds a CI guardrail. No schema changes are included. + +## Entities (existing) + +### Tenant (`app/Models/Tenant.php`) +- **Relevant fields (legacy)**: `app_client_id`, `app_client_secret`, `tenant_id`, `external_id` +- **Relevant method (deprecated)**: `graphOptions(): array` +- **Planned behavior**: `graphOptions()` remains but throws (kill-switch) to prevent legacy use. + +### ProviderConnection (`app/Models/ProviderConnection.php`) +- **Used by**: `ProviderConnectionResolver::resolveDefault($tenant, 'microsoft')` +- **Key fields**: `tenant_id`, `provider`, `is_default`, `status`, `entra_tenant_id` + +### ProviderCredential (`app/Models/ProviderCredential.php`) +- **Used by**: `CredentialManager::getClientCredentials($connection)` via `ProviderGateway::graphOptions()` +- **Expected payload**: `['client_id' => string, 'client_secret' => string]` + +## Relationships (existing) +- `Tenant::providerConnections()` → hasMany `ProviderConnection` +- `ProviderConnection::credential()` → hasOne/hasMany `ProviderCredential` (via relationship method in model) + +## Validation / Constraints +- Provider connection resolution must fail deterministically when: + - No default connection exists for tenant/provider + - Multiple defaults exist + - Connection is disabled / needs consent + - Missing `entra_tenant_id` + - Missing/invalid credential payload + +(These rules are currently enforced by `ProviderConnectionResolver`.) + +## State Transitions +- None added by this feature. + +## Out of Scope +- Dropping / migrating tenant credential columns. +- Changing provider resolution semantics. diff --git a/specs/088-remove-tenant-graphoptions-legacy/plan.md b/specs/088-remove-tenant-graphoptions-legacy/plan.md new file mode 100644 index 0000000..f198ce0 --- /dev/null +++ b/specs/088-remove-tenant-graphoptions-legacy/plan.md @@ -0,0 +1,105 @@ + +# Implementation Plan: Remove Legacy Tenant Graph Options + +**Branch**: `088-remove-tenant-graphoptions-legacy` | **Date**: 2026-02-11 | **Spec**: [spec.md](spec.md) +**Input**: Feature specification in [spec.md](spec.md) + +## Summary + +Hard cut removal of the deprecated tenant-based Graph options accessor. All Microsoft Graph configuration must be derived exclusively from a resolved `ProviderConnection` via `ProviderConnectionResolver::resolveDefault()` and `ProviderGateway::graphOptions()`. Add a CI guardrail scanning `app/` for `$tenant->graphOptions()` / `Tenant::graphOptions()` and implement a kill-switch by making `Tenant::graphOptions()` throw. + +## Technical Context + +**Language/Version**: PHP 8.4.15 (Laravel 12) +**Primary Dependencies**: Filament v5, Livewire v4, Pest v4 +**Storage**: PostgreSQL (Sail locally) +**Testing**: Pest via `vendor/bin/sail artisan test --compact` +**Target Platform**: Containerized Linux deployment (Sail local, Dokploy staging/prod) +**Project Type**: Laravel web application (monolith) +**Performance Goals**: N/A (internal refactor + CI guard) +**Constraints**: +- No fallback to tenant-stored credentials. +- Guardrail scans `app/` only (do not fail due to `tests/`). +- No new Graph endpoints; Graph calls remain routed through `GraphClientInterface`. +**Scale/Scope**: Repo-wide refactor of call sites in `app/Services/**`. + +## Constitution Check + +*GATE: Must pass before implementation. Re-check after design decisions.* + +- Inventory-first / snapshots: **N/A** (no inventory/snapshot semantics change) +- Read/write separation: **PASS** (no new write surfaces; existing flows only refactored) +- Single contract path to Graph: **PASS** (Graph calls remain through `GraphClientInterface`; only options sourcing changes) +- Deterministic capabilities: **N/A** (no capability mapping changes) +- RBAC-UX / workspace isolation / tenant isolation: **N/A** (no routing or authorization semantic changes) +- Run observability: **N/A** (no new operations or run tracking changes) +- Data minimization / safe logging: **PASS** (no secrets introduced; provider credentials remain in credential store) +- Filament UI Action Surface Contract: **N/A** (no UI resources/pages are added or modified) + +## Project Structure + +### Documentation (this feature) + +```text +specs/088-remove-tenant-graphoptions-legacy/ +├── plan.md +├── research.md +├── data-model.md +├── quickstart.md +├── contracts/ +└── tasks.md # created by /speckit.tasks (later) +``` + +### Source Code (repository root) + +```text +app/ +├── Models/ +│ └── Tenant.php +├── Services/ +│ ├── Providers/ +│ │ ├── ProviderConnectionResolver.php +│ │ └── ProviderGateway.php +│ └── ... (multiple Graph-enabled services) + +tests/ +└── Feature/ + └── Guards/ +``` + +**Structure Decision**: Laravel monolith; refactor stays within existing services/models. + +## Implementation Plan + +### Phase 0 — Outline & Research (completed) + +- Documented decisions and call sites in [research.md](research.md). + +### Phase 1 — Design & Implementation (planned) + +1. **Kill-switch the deprecated accessor** + - Update `Tenant::graphOptions()` in `app/Models/Tenant.php` to throw a clear exception explaining that provider connections are required. + +2. **Refactor all `app/` call sites off `$tenant->graphOptions()`** + - For each call site, resolve the default Microsoft provider connection via `ProviderConnectionResolver::resolveDefault($tenant, 'microsoft')`. + - If resolution fails, fail fast with a clear, actionable error (include the effective reason code/message). + - Replace option building with `ProviderGateway::graphOptions($connection, $overrides)`. + - Update `app/Services/Intune/TenantConfigService.php` (currently returns `$tenant->graphOptions()`), so it no longer provides a tenant-based path. + +3. **Add CI guardrail (Pest)** + - Add a new guard test under `tests/Feature/Guards/` that scans `app/` for forbidden patterns: + - `\$tenant->graphOptions(` + - `Tenant::graphOptions(` + - Ensure failure message explains how to fix (use provider connection resolution). + +4. **Verification** + - Run the guard test and the most relevant subsets described in [quickstart.md](quickstart.md). + - Run `vendor/bin/sail bin pint --dirty`. + +### Phase 2 — Tasks (next step) + +- Completed: `tasks.md` has been generated and Phase 1 is broken into smaller, reviewable steps. + +## Complexity Tracking + +No constitution violations are required for this feature. diff --git a/specs/088-remove-tenant-graphoptions-legacy/quickstart.md b/specs/088-remove-tenant-graphoptions-legacy/quickstart.md new file mode 100644 index 0000000..47d3444 --- /dev/null +++ b/specs/088-remove-tenant-graphoptions-legacy/quickstart.md @@ -0,0 +1,22 @@ +# Quickstart — Remove Legacy Tenant Graph Options + +## Preconditions +- Sail is running: `vendor/bin/sail up -d` + +## What to run (during implementation) + +### 1) Run the guardrail test (fast) +Once implemented, run the guard that enforces no deprecated accessor usage: +- `vendor/bin/sail artisan test --compact --filter=NoLegacyTenantGraphOptions` + +### 2) Run the most relevant test subset +After refactoring Graph option sourcing, run tests touching provider resolution / Graph-enabled flows: +- `vendor/bin/sail artisan test --compact tests/Feature/Guards` + +## Manual sanity checks (optional) +- If a tenant has **no default provider connection**, Graph-enabled flows should fail with a clear “provider connection required” style error. +- If a tenant has a configured default provider connection, flows should continue to work using `ProviderGateway::graphOptions($connection)`. + +## Formatting +Before finalizing the implementation PR: +- `vendor/bin/sail bin pint --dirty` diff --git a/specs/088-remove-tenant-graphoptions-legacy/research.md b/specs/088-remove-tenant-graphoptions-legacy/research.md new file mode 100644 index 0000000..d2fa3cd --- /dev/null +++ b/specs/088-remove-tenant-graphoptions-legacy/research.md @@ -0,0 +1,71 @@ +# Research — Remove Legacy Tenant Graph Options + +## Goal +Remove usage of the deprecated tenant-based Graph options accessor (`$tenant->graphOptions()` / `Tenant::graphOptions()`) and make provider-based resolution (`ProviderConnectionResolver` + `ProviderGateway`) the single source of truth for Microsoft Graph configuration. + +## Decisions + +### Decision: Provider connection is canonical for Graph options +- **Chosen**: Resolve `ProviderConnection` via `App\Services\Providers\ProviderConnectionResolver::resolveDefault($tenant, 'microsoft')`, then derive options via `App\Services\Providers\ProviderGateway::graphOptions($connection, $overrides)`. +- **Rationale**: Existing services already follow this path (e.g. onboarding/health); it centralizes credential storage and validation. +- **Alternatives considered**: + - Keep reading tenant columns (`Tenant.app_client_id` / `Tenant.app_client_secret`) as fallback — rejected (explicitly forbidden; introduces mixed credential sources). + +### Decision: Kill-switch behavior for `Tenant::graphOptions()` +- **Chosen**: Keep method for now, but make it throw a clear exception (fail-fast). +- **Rationale**: Ensures any missed call site fails deterministically in CI/runtime. +- **Alternatives considered**: + - Delete method outright — rejected (higher risk / more disruptive API break). + +### Decision: Guardrail scope and matching rules +- **Chosen**: Add a CI guard test that scans **`app/` only** for these forbidden patterns: + - `\$tenant->graphOptions(` + - `Tenant::graphOptions(` +- **Rationale**: Matches the agreed scope and avoids false positives on unrelated `graphOptions(...)` methods (e.g. `ProviderGateway::graphOptions`). +- **Alternatives considered**: + - Scan for all `->graphOptions(` — rejected (would flag many legitimate methods). + - Scan `tests/` too — rejected (would create noisy failures during refactors). + +## Current Code Findings (Call Sites) + +### Deprecated tenant accessor call sites (must be removed) +These are current `app/` references to `$tenant->graphOptions()`: +- `app/Services/AssignmentRestoreService.php` +- `app/Services/AssignmentBackupService.php` +- `app/Services/Intune/RestoreRiskChecker.php` +- `app/Services/Intune/TenantPermissionService.php` +- `app/Services/Intune/VersionService.php` +- `app/Services/Intune/FoundationMappingService.php` +- `app/Services/Intune/PolicyCaptureOrchestrator.php` +- `app/Services/Intune/FoundationSnapshotService.php` +- `app/Services/Intune/TenantConfigService.php` (wraps + returns `$tenant->graphOptions()`) +- `app/Services/Intune/ConfigurationPolicyTemplateResolver.php` +- `app/Services/Directory/EntraGroupSyncService.php` +- `app/Services/Directory/RoleDefinitionsSyncService.php` +- `app/Services/Graph/AssignmentFilterResolver.php` + +### Canonical provider-based examples to copy +- `app/Services/Intune/RbacOnboardingService.php` +- `app/Services/Intune/RbacHealthService.php` +- `app/Services/Intune/RestoreService.php` +- `app/Services/Intune/PolicySnapshotService.php` + +### Source of legacy accessor +- `app/Models/Tenant.php` defines the deprecated `graphOptions()` method. + +## Recommended Refactor Pattern + +```php +$resolution = $this->providerConnections()->resolveDefault($tenant, 'microsoft'); + +if (! $resolution->resolved || ! $resolution->connection instanceof ProviderConnection) { + // Fail fast with actionable reason. +} + +$connection = $resolution->connection; +$graphOptions = $this->providerGateway()->graphOptions($connection, $overrides); +``` + +## Notes / Non-goals +- No database schema changes (tenant credential columns stay for now). +- No changes to Graph contract registry (`config/graph_contracts.php`) are required for this feature; this work changes configuration sourcing only. diff --git a/specs/088-remove-tenant-graphoptions-legacy/spec.md b/specs/088-remove-tenant-graphoptions-legacy/spec.md new file mode 100644 index 0000000..bfdc4bc --- /dev/null +++ b/specs/088-remove-tenant-graphoptions-legacy/spec.md @@ -0,0 +1,88 @@ +# Feature Specification: Remove Legacy Graph Options + +**Feature Branch**: `088-remove-tenant-graphoptions-legacy` +**Created**: 2026-02-11 +**Status**: Draft +**Input**: Remove the legacy tenant-based Graph options path and make provider-based resolution the only source of Graph configuration. + +## Clarifications + +### Session 2026-02-11 + +- Q: How should `Tenant::graphOptions()` be made unusable (kill-switch)? → A: Keep the method, but make it throw a clear exception so any remaining call sites fail fast in CI/runtime. +- Q: Should we drop legacy tenant credential DB fields as part of this feature? → A: No — defer DB schema cleanup to a follow-up feature. +- Q: Which pattern should the CI guardrail block? → A: Block any call to the deprecated tenant accessor (`$tenant->graphOptions()` and `Tenant::graphOptions()`), but do not block other unrelated `graphOptions(...)` methods. +- Q: Should the CI guardrail scan only production code, or also tests? → A: Scan `app/` only. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Safe Refactors via Single Credential Source (Priority: P1) + +As a maintainer, I want all Microsoft Graph configuration to come from a single, canonical provider-based source, so refactors do not accidentally mix credential sources or introduce inconsistent authentication behavior. + +**Why this priority**: This removes a major source of drift and breakage, making changes safer and predictable. + +**Independent Test**: Can be fully tested by running the automated test suite and verifying that Graph configuration is resolved only via the canonical provider connection, with no tenant-stored credential fallback. + +**Acceptance Scenarios**: + +1. **Given** a tenant with a valid provider connection configured, **When** any Graph-enabled flow starts, **Then** it uses only the provider connection-derived configuration. +2. **Given** a tenant without a configured provider connection, **When** a Graph-enabled flow starts, **Then** the flow fails deterministically without using any tenant-stored credentials. + +--- + +### User Story 2 - Prevent Reintroduction of Legacy Paths (Priority: P2) + +As a maintainer, I want an automated guardrail that fails CI if the deprecated tenant-based Graph options accessor is reintroduced, so the codebase cannot regress silently. + +**Why this priority**: Prevents recurring tech debt and ensures the hard cut remains enforced. + +**Independent Test**: Can be fully tested by introducing a deliberate reference in a sandbox change and confirming CI fails. + +**Acceptance Scenarios**: + +1. **Given** a change that reintroduces a usage of the deprecated tenant-based Graph options accessor, **When** tests run in CI, **Then** the pipeline fails with a clear message describing the forbidden pattern. + +--- + +### Edge Cases + +- Tenants that previously relied on tenant-stored credentials must not silently keep working; they should fail clearly until a provider connection is configured. +- Flows that run non-interactively (jobs/scheduled work) must behave consistently with interactive flows: no alternate credential source. + +## Requirements *(mandatory)* + +### Assumptions + +- The canonical source of truth for Microsoft Graph configuration is the provider connection resolved for the tenant (default provider key: `microsoft`). +- No user-facing navigation or UI changes are required. +- Provider connection resolution semantics remain unchanged. +- Dropping/altering deprecated tenant credential fields in the database is out of scope for this feature. + +### Definitions + +- **Provider configuration is “missing or invalid”** when `ProviderConnectionResolver::resolveDefault($tenant, 'microsoft')` results in a blocked resolution (e.g. missing default connection, disabled/needs consent, missing/invalid credentials). +- Downstream Microsoft Graph authentication / HTTP failures are still treated as runtime Graph errors; this feature only removes the legacy tenant-credential configuration path and standardizes option sourcing. + +### Out of Scope + +- Any database schema changes to remove deprecated tenant credential fields (follow-up feature). + +### Functional Requirements + +- **FR-001**: The system MUST not use the deprecated tenant-based Graph options accessor anywhere under `app/`. +- **FR-002**: The system MUST derive Microsoft Graph configuration exclusively from the canonical provider-based resolution for the tenant. +- **FR-003**: The system MUST NOT fallback to tenant-stored credentials if provider-based configuration is missing or invalid. +- **FR-004**: If provider-based configuration is missing or invalid (see Definitions), the system MUST fail fast with a clear, actionable error indicating that provider configuration is required. +- **FR-005**: The system MUST include an automated guardrail that fails CI if the deprecated tenant-based Graph options accessor is referenced in application code (instance or static): `$tenant->graphOptions()` or `Tenant::graphOptions()`. +- **FR-005a**: The guardrail MUST scan `app/` (production code) and SHOULD NOT fail due to references in `tests/`. +- **FR-006**: The deprecated tenant-based Graph options accessor MUST throw a clear exception if called (kill-switch), and MUST NOT attempt to resolve credentials from tenant-stored fields. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: CI includes a guardrail that reports 0 allowed references to the deprecated tenant-based Graph options accessor. +- **SC-002**: 100% of Graph-enabled flows use provider-based resolution as their configuration source. +- **SC-003**: The existing automated test suite passes without disabling tests or reducing coverage. +- **SC-004**: When provider configuration is missing, affected flows fail with a consistent, actionable error (no silent fallback behavior). diff --git a/specs/088-remove-tenant-graphoptions-legacy/tasks.md b/specs/088-remove-tenant-graphoptions-legacy/tasks.md new file mode 100644 index 0000000..4a1b1db --- /dev/null +++ b/specs/088-remove-tenant-graphoptions-legacy/tasks.md @@ -0,0 +1,128 @@ +--- + +description: "Task list for implementing Spec 088" +--- + +# Tasks: Remove Legacy Tenant Graph Options + +**Input**: Design documents from `/specs/088-remove-tenant-graphoptions-legacy/` +**Prerequisites**: plan.md (required), spec.md (required), research.md, data-model.md, contracts/, quickstart.md + +**Tests**: REQUIRED (Pest) — this feature changes runtime behavior and introduces a CI guardrail. + +## Phase 1: Setup (Shared Infrastructure) + +- [x] T001 Verify test + format tooling paths (composer.json, phpunit.xml, specs/088-remove-tenant-graphoptions-legacy/quickstart.md) +- [x] T002 [P] Add a focused test runner note to specs/088-remove-tenant-graphoptions-legacy/quickstart.md (include `--filter=NoLegacyTenantGraphOptions`) + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**⚠️ CRITICAL**: No user story work can begin until this phase is complete. + +- [x] T003 Create provider-only Graph options resolver in app/Services/Providers/MicrosoftGraphOptionsResolver.php +- [x] T004 [P] Create exception for missing/invalid provider config in app/Services/Providers/ProviderConfigurationRequiredException.php +- [x] T005 Wire resolver dependencies (ProviderConnectionResolver + ProviderGateway) in app/Providers/AppServiceProvider.php +- [x] T006 Add tests for resolver failure/success paths in tests/Feature/Providers/MicrosoftGraphOptionsResolverTest.php +- [x] T007 Implement kill-switch: make Tenant::graphOptions() throw in app/Models/Tenant.php +- [x] T008 Add test asserting kill-switch behavior in tests/Feature/Models/TenantGraphOptionsKillSwitchTest.php + +**Checkpoint**: A single reusable provider-only resolver exists, with tests, and the legacy accessor fails fast. + +--- + +## Phase 3: User Story 1 — Safe Refactors via Single Credential Source (Priority: P1) 🎯 MVP + +**Goal**: All Graph-enabled flows use provider connection-derived configuration only. + +**Independent Test**: `vendor/bin/sail artisan test --compact --filter=MicrosoftGraphOptionsResolverTest` + run a few refactored service tests (as applicable) and ensure no `app/` references remain. + +### Implementation + +- [x] T009 [P] [US1] Refactor graph options usage in app/Services/AssignmentRestoreService.php (replace `$tenant->graphOptions()` with resolver) +- [x] T010 [P] [US1] Refactor graph options usage in app/Services/AssignmentBackupService.php (replace `$tenant->graphOptions()` with resolver) +- [x] T011 [P] [US1] Refactor graph options usage in app/Services/Intune/RestoreRiskChecker.php (replace `$tenant->graphOptions()` with resolver) +- [x] T012 [P] [US1] Refactor graph options usage in app/Services/Intune/TenantPermissionService.php (derive `client_id` from provider-based options; no tenant fallback) +- [x] T013 [P] [US1] Refactor graph options usage in app/Services/Intune/VersionService.php (replace `$tenant->graphOptions()` with resolver) +- [x] T014 [P] [US1] Refactor graph options usage in app/Services/Intune/FoundationMappingService.php (replace array merge with provider-based options) +- [x] T015 [P] [US1] Refactor graph options usage in app/Services/Intune/PolicyCaptureOrchestrator.php (replace `$tenant->graphOptions()` with resolver) +- [x] T016 [P] [US1] Refactor graph options usage in app/Services/Intune/FoundationSnapshotService.php (replace `$tenant->graphOptions()` with resolver) +- [x] T017 [P] [US1] Refactor/remove legacy wrapper in app/Services/Intune/TenantConfigService.php (must not return `$tenant->graphOptions()`; use resolver) +- [x] T018 [P] [US1] Refactor context building in app/Services/Intune/ConfigurationPolicyTemplateResolver.php (replace `array_merge($tenant->graphOptions(), ...)` with provider-based context) +- [x] T019 [P] [US1] Refactor graph options usage in app/Services/Directory/EntraGroupSyncService.php (replace `$tenant->graphOptions()` with resolver) +- [x] T020 [P] [US1] Refactor graph options usage in app/Services/Directory/RoleDefinitionsSyncService.php (replace `$tenant->graphOptions()` with resolver) +- [x] T021 [P] [US1] Refactor options merge in app/Services/Graph/AssignmentFilterResolver.php (replace `array_merge($options, $tenant->graphOptions())` with provider-based options) + +### Verification + +- [x] T022 [US1] Run focused tests for this story: `vendor/bin/sail artisan test --compact --filter=MicrosoftGraphOptionsResolver` +- [x] T023 [US1] Run a repo scan to ensure no `app/` usage remains (confirm via tests/Feature/Guards/NoLegacyTenantGraphOptionsTest.php once added) + +**Checkpoint**: `app/` contains zero tenant-based graph options usages, and runtime failures are provider-config actionable. + +--- + +## Phase 4: User Story 2 — Prevent Reintroduction of Legacy Paths (Priority: P2) + +**Goal**: CI fails if `$tenant->graphOptions()` or `Tenant::graphOptions()` appears anywhere under `app/`. + +**Independent Test**: `vendor/bin/sail artisan test --compact --filter=NoLegacyTenantGraphOptions`. + +### Tests (Guardrail) + +- [x] T024 [P] [US2] Create guard test scanning `app/` in tests/Feature/Guards/NoLegacyTenantGraphOptionsTest.php +- [x] T025 [US2] Ensure guard failure message is actionable (points to ProviderConnectionResolver + ProviderGateway) in tests/Feature/Guards/NoLegacyTenantGraphOptionsTest.php + +### Verification + +- [x] T026 [US2] Run guard test: `vendor/bin/sail artisan test --compact --filter=NoLegacyTenantGraphOptions` + +--- + +## Phase 5: Polish & Cross-Cutting Concerns + +- [x] T027 [P] Run formatter: `vendor/bin/sail bin pint --dirty` (app/**, tests/**) +- [x] T028 Run full affected test folders: `vendor/bin/sail artisan test --compact tests/Feature/Guards` +- [x] T029 Run a broader suite if needed: `vendor/bin/sail artisan test --compact` + +--- + +## Dependencies & Execution Order + +### User Story Dependencies + +- **US1 (P1)** depends on Phase 2 foundational tasks (resolver + kill-switch). +- **US2 (P2)** can be implemented after Phase 2, but should run after US1 refactor so the guard passes. + +### Dependency Graph (high level) + +- Phase 2 (T003–T008) + -→ US1 refactors (T009–T021) + -→ US1 verification (T022–T023) + -→ US2 guardrail (T024–T026) + -→ Polish (T027–T029) + +## Parallel Execution Examples + +### After Phase 2 is complete + +These refactors are parallelizable because they touch different files: +- T009–T016, T018–T021 can run in parallel ([P]). + +### US2 guardrail + +- T024 can be done in parallel with late US1 refactors, but it will only pass once all US1 call sites are removed. + +## Implementation Strategy + +### MVP (US1) + +1. Complete Phase 2 (resolver + kill-switch + tests). +2. Refactor all `app/` call sites (T009–T021). +3. Run focused tests (T022). + +### Then lock it in (US2) + +1. Add guardrail test (T024–T026). +2. Finish with formatting + regression checks (T027–T029). diff --git a/tests/Feature/BackupWithAssignmentsConsistencyTest.php b/tests/Feature/BackupWithAssignmentsConsistencyTest.php index 2a43ee6..e0b4278 100644 --- a/tests/Feature/BackupWithAssignmentsConsistencyTest.php +++ b/tests/Feature/BackupWithAssignmentsConsistencyTest.php @@ -16,6 +16,8 @@ beforeEach(function () { $this->tenant = Tenant::factory()->create(['status' => 'active']); + ensureDefaultProviderConnection($this->tenant, 'microsoft'); + $this->policy = Policy::factory()->create([ 'tenant_id' => $this->tenant->id, 'external_id' => 'test-policy-123', diff --git a/tests/Feature/EndpointSecurityPolicyRestore023Test.php b/tests/Feature/EndpointSecurityPolicyRestore023Test.php index 056976d..ea2382e 100644 --- a/tests/Feature/EndpointSecurityPolicyRestore023Test.php +++ b/tests/Feature/EndpointSecurityPolicyRestore023Test.php @@ -231,6 +231,8 @@ public function request(string $method, string $path, array $options = []): Grap app()->instance(GraphClientInterface::class, $client); $tenant = Tenant::factory()->create(); + + ensureDefaultProviderConnection($tenant); $backupSet = BackupSet::factory()->for($tenant)->create([ 'status' => 'completed', 'item_count' => 1, diff --git a/tests/Feature/Filament/BackupCreationTest.php b/tests/Feature/Filament/BackupCreationTest.php index a08c7d5..ce6585d 100644 --- a/tests/Feature/Filament/BackupCreationTest.php +++ b/tests/Feature/Filament/BackupCreationTest.php @@ -78,6 +78,8 @@ public function request(string $method, string $path, array $options = []): Grap 'metadata' => [], ]); + ensureDefaultProviderConnection($tenant); + $tenant->makeCurrent(); $policyA = Policy::create([ @@ -169,6 +171,8 @@ public function request(string $method, string $path, array $options = []): Grap 'metadata' => [], ]); + ensureDefaultProviderConnection($tenant); + $tenant->makeCurrent(); $policyA = Policy::create([ diff --git a/tests/Feature/Filament/PolicyCaptureSnapshotOptionsTest.php b/tests/Feature/Filament/PolicyCaptureSnapshotOptionsTest.php index 34cbe48..8af287e 100644 --- a/tests/Feature/Filament/PolicyCaptureSnapshotOptionsTest.php +++ b/tests/Feature/Filament/PolicyCaptureSnapshotOptionsTest.php @@ -24,6 +24,7 @@ $tenant = Tenant::factory()->create(); $tenant->makeCurrent(); + ensureDefaultProviderConnection($tenant, 'microsoft'); $policy = Policy::factory()->for($tenant)->create([ 'external_id' => 'policy-123', ]); diff --git a/tests/Feature/Filament/RestorePreviewTest.php b/tests/Feature/Filament/RestorePreviewTest.php index 929e83e..bd5d680 100644 --- a/tests/Feature/Filament/RestorePreviewTest.php +++ b/tests/Feature/Filament/RestorePreviewTest.php @@ -50,6 +50,7 @@ public function request(string $method, string $path, array $options = []): Grap 'name' => 'Tenant One', 'metadata' => [], ]); + ensureDefaultProviderConnection($tenant, 'microsoft'); $policy = Policy::create([ 'tenant_id' => $tenant->id, @@ -143,6 +144,7 @@ public function request(string $method, string $path, array $options = []): Grap 'name' => 'Tenant Preview', 'metadata' => [], ]); + ensureDefaultProviderConnection($tenant, 'microsoft'); $backupSet = BackupSet::create([ 'tenant_id' => $tenant->id, @@ -214,6 +216,7 @@ public function request(string $method, string $path, array $options = []): Grap 'name' => 'Tenant Two', 'metadata' => [], ]); + ensureDefaultProviderConnection($tenant, 'microsoft'); $policy = Policy::create([ 'tenant_id' => $tenant->id, diff --git a/tests/Feature/Filament/TenantSetupTest.php b/tests/Feature/Filament/TenantSetupTest.php index bcdcab9..fc38a21 100644 --- a/tests/Feature/Filament/TenantSetupTest.php +++ b/tests/Feature/Filament/TenantSetupTest.php @@ -108,7 +108,7 @@ 'status' => 'active', ]); - [$user, $tenant] = createUserWithTenant(tenant: $tenant, user: $user, role: 'owner'); + [$user, $tenant] = createUserWithTenant(tenant: $tenant, user: $user, role: 'owner', ensureDefaultMicrosoftProviderConnection: false); $this->actingAs($user); Filament::setTenant($tenant, true); diff --git a/tests/Feature/Filament/TenantVerificationReportWidgetTest.php b/tests/Feature/Filament/TenantVerificationReportWidgetTest.php index a6c3302..95cee60 100644 --- a/tests/Feature/Filament/TenantVerificationReportWidgetTest.php +++ b/tests/Feature/Filament/TenantVerificationReportWidgetTest.php @@ -21,7 +21,7 @@ it('starts tenant verification from header action and links to the canonical run viewer', function (): void { Queue::fake(); - [$user, $tenant] = createUserWithTenant(role: 'operator'); + [$user, $tenant] = createUserWithTenant(role: 'operator', ensureDefaultMicrosoftProviderConnection: false); $this->actingAs($user); $tenant->makeCurrent(); @@ -147,7 +147,7 @@ it('starts verification from the embedded widget CTA and uses canonical view-run links', function (): void { Queue::fake(); - [$user, $tenant] = createUserWithTenant(role: 'operator'); + [$user, $tenant] = createUserWithTenant(role: 'operator', ensureDefaultMicrosoftProviderConnection: false); $this->actingAs($user); $connection = ProviderConnection::factory()->create([ @@ -194,7 +194,7 @@ it('starts tenant verification from the tenant list row action via the unified run path', function (): void { Queue::fake(); - [$user, $tenant] = createUserWithTenant(role: 'operator'); + [$user, $tenant] = createUserWithTenant(role: 'operator', ensureDefaultMicrosoftProviderConnection: false); $this->actingAs($user); Filament::setTenant($tenant, true); diff --git a/tests/Feature/FoundationBackupTest.php b/tests/Feature/FoundationBackupTest.php index 2c29ada..16b04b2 100644 --- a/tests/Feature/FoundationBackupTest.php +++ b/tests/Feature/FoundationBackupTest.php @@ -11,6 +11,8 @@ beforeEach(function () { $this->tenant = Tenant::factory()->create(['status' => 'active']); + ensureDefaultProviderConnection($this->tenant); + $this->policy = Policy::factory()->create([ 'tenant_id' => $this->tenant->id, 'policy_type' => 'deviceConfiguration', diff --git a/tests/Feature/Guards/NoLegacyTenantGraphOptionsTest.php b/tests/Feature/Guards/NoLegacyTenantGraphOptionsTest.php new file mode 100644 index 0000000..57df4f6 --- /dev/null +++ b/tests/Feature/Guards/NoLegacyTenantGraphOptionsTest.php @@ -0,0 +1,108 @@ +graphOptions\s*\(/', + + // Avoid failing on the kill-switch error message string in app/Models/Tenant.php. + '/(? $files */ + $files = collect($directories) + ->filter(fn (string $dir): bool => is_dir($dir)) + ->flatMap(function (string $dir): array { + $iterator = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator($dir, FilesystemIterator::SKIP_DOTS) + ); + + $paths = []; + + foreach ($iterator as $file) { + if (! $file->isFile()) { + continue; + } + + $path = $file->getPathname(); + + if (! str_ends_with($path, '.php')) { + continue; + } + + $paths[] = $path; + } + + return $paths; + }) + ->filter(function (string $path) use ($excludedPaths, $self): bool { + if ($self && realpath($path) === $self) { + return false; + } + + foreach ($excludedPaths as $excluded) { + if (str_starts_with($path, $excluded)) { + return false; + } + } + + return true; + }) + ->values(); + + $hits = []; + + foreach ($files as $path) { + $contents = file_get_contents($path); + + if (! is_string($contents) || $contents === '') { + continue; + } + + foreach ($forbiddenPatterns as $pattern) { + if (! preg_match($pattern, $contents)) { + continue; + } + + $lines = preg_split('/\R/', $contents) ?: []; + + foreach ($lines as $index => $line) { + if (preg_match($pattern, $line)) { + $relative = str_replace($root.'/', '', $path); + $hits[] = $relative.':'.($index + 1).' -> '.trim($line); + } + } + } + } + + $howToFix = <<<'TXT' +Legacy tenant Graph options accessor usage detected. + +Do not call `$tenant->graphOptions()` or `Tenant::graphOptions()` anywhere under `app/`. + +Use provider connection resolution instead: +- `ProviderConnectionResolver::resolveDefault($tenant, 'microsoft')` +- `ProviderGateway::graphOptions($connection, $overrides)` + +Or prefer the canonical helper: `MicrosoftGraphOptionsResolver::resolveForTenant($tenant, $overrides)`. +TXT; + + expect($hits)->toBeEmpty($howToFix."\n\nMatches:\n".implode("\n", $hits)); +}); diff --git a/tests/Feature/Models/TenantGraphOptionsKillSwitchTest.php b/tests/Feature/Models/TenantGraphOptionsKillSwitchTest.php new file mode 100644 index 0000000..fc3ea27 --- /dev/null +++ b/tests/Feature/Models/TenantGraphOptionsKillSwitchTest.php @@ -0,0 +1,17 @@ +create([ + 'app_client_id' => 'legacy-client-id', + 'app_client_secret' => 'legacy-client-secret', + ]); + + $call = fn (): array => $tenant->graphOptions(); + + expect($call)->toThrow(BadMethodCallException::class); +}); diff --git a/tests/Feature/ProviderConnections/ProviderOperationBlockedGuidanceSpec081Test.php b/tests/Feature/ProviderConnections/ProviderOperationBlockedGuidanceSpec081Test.php index e72a8d6..75d3282 100644 --- a/tests/Feature/ProviderConnections/ProviderOperationBlockedGuidanceSpec081Test.php +++ b/tests/Feature/ProviderConnections/ProviderOperationBlockedGuidanceSpec081Test.php @@ -16,7 +16,7 @@ it('Spec081 shows blocked guidance with reason and manage-connections link on start surfaces', function (): void { Queue::fake(); - [$user, $tenant] = createUserWithTenant(role: 'operator'); + [$user, $tenant] = createUserWithTenant(role: 'operator', ensureDefaultMicrosoftProviderConnection: false); $this->actingAs($user); $tenant->makeCurrent(); @@ -60,7 +60,7 @@ }); it('Spec081 shows blocked guidance on tenant verify surface with manage-connections remediation link', function (): void { - [$user, $tenant] = createUserWithTenant(role: 'operator'); + [$user, $tenant] = createUserWithTenant(role: 'operator', ensureDefaultMicrosoftProviderConnection: false); $this->actingAs($user); $tenant->makeCurrent(); diff --git a/tests/Feature/Providers/MicrosoftGraphOptionsResolverTest.php b/tests/Feature/Providers/MicrosoftGraphOptionsResolverTest.php new file mode 100644 index 0000000..de096f4 --- /dev/null +++ b/tests/Feature/Providers/MicrosoftGraphOptionsResolverTest.php @@ -0,0 +1,85 @@ +create(); + + $resolver = app(MicrosoftGraphOptionsResolver::class); + + $call = fn (): array => $resolver->resolveForTenant($tenant); + + expect($call)->toThrow(ProviderConfigurationRequiredException::class); + + try { + $call(); + } catch (ProviderConfigurationRequiredException $exception) { + expect($exception->reasonCode)->toBe(ProviderReasonCodes::ProviderConnectionMissing); + expect($exception->provider)->toBe('microsoft'); + expect($exception->tenantId)->toBe((int) $tenant->getKey()); + } +}); + +it('throws when default provider connection needs consent', function (): void { + $tenant = Tenant::factory()->create(); + + ProviderConnection::factory()->create([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $tenant->workspace_id, + 'provider' => 'microsoft', + 'is_default' => true, + 'status' => 'needs_consent', + ]); + + $resolver = app(MicrosoftGraphOptionsResolver::class); + + $call = fn (): array => $resolver->resolveForTenant($tenant); + + expect($call)->toThrow(ProviderConfigurationRequiredException::class); + + try { + $call(); + } catch (ProviderConfigurationRequiredException $exception) { + expect($exception->reasonCode)->toBe(ProviderReasonCodes::ProviderConsentMissing); + } +}); + +it('builds graph options from default provider connection credentials', function (): void { + $tenant = Tenant::factory()->create(); + + $connection = ProviderConnection::factory()->create([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $tenant->workspace_id, + 'provider' => 'microsoft', + 'entra_tenant_id' => 'entra-tenant-id', + 'is_default' => true, + 'status' => 'connected', + ]); + + ProviderCredential::factory()->create([ + 'provider_connection_id' => $connection->getKey(), + 'type' => 'client_secret', + 'payload' => [ + 'client_id' => 'client-id', + 'client_secret' => 'client-secret', + ], + ]); + + $resolver = app(MicrosoftGraphOptionsResolver::class); + + $options = $resolver->resolveForTenant($tenant, ['query' => ['a' => 'b']]); + + expect($options['tenant'])->toBe('entra-tenant-id'); + expect($options['client_id'])->toBe('client-id'); + expect($options['client_secret'])->toBe('client-secret'); + expect($options['client_request_id'])->toBeString()->not->toBeEmpty(); + expect($options['query'])->toBe(['a' => 'b']); +}); diff --git a/tests/Feature/Rbac/UiEnforcementNonMemberHiddenTest.php b/tests/Feature/Rbac/UiEnforcementNonMemberHiddenTest.php index e8f3ec9..30423fb 100644 --- a/tests/Feature/Rbac/UiEnforcementNonMemberHiddenTest.php +++ b/tests/Feature/Rbac/UiEnforcementNonMemberHiddenTest.php @@ -1,5 +1,6 @@ create(); $tenant = Tenant::factory()->create(); - // No membership created + $otherTenant = Tenant::factory()->create(); - $this->actingAs($user); - $tenant->makeCurrent(); - Filament::setTenant($tenant, true); + // Create user with a valid workspace context, but without membership to $tenant + [$user] = createUserWithTenant(tenant: $otherTenant, role: 'owner'); - Livewire::test(ListPolicies::class) - ->assertActionHidden('sync'); + $this->actingAs($user) + ->get(PolicyResource::getUrl('index', tenant: $tenant)) + ->assertNotFound(); Queue::assertNothingPushed(); }); @@ -53,12 +52,9 @@ $tenantB = Tenant::factory()->create(); // User has no membership to tenantB - $this->actingAs($user); - $tenantB->makeCurrent(); - Filament::setTenant($tenantB, true); - - Livewire::test(ListPolicies::class) - ->assertActionHidden('sync'); + $this->actingAs($user) + ->get(PolicyResource::getUrl('index', tenant: $tenantB)) + ->assertNotFound(); Queue::assertNothingPushed(); }); @@ -70,20 +66,16 @@ }); it('blocks action execution for non-members (no side effects)', function () { - $user = User::factory()->create(); $tenant = Tenant::factory()->create(); + $otherTenant = Tenant::factory()->create(); + + // Create user with a valid workspace context, but without membership to $tenant + [$user] = createUserWithTenant(tenant: $otherTenant, role: 'owner'); // No membership - $this->actingAs($user); - $tenant->makeCurrent(); - Filament::setTenant($tenant, true); - - // Hidden actions are treated as disabled by Filament - // The action call returns 200 but no execution occurs - Livewire::test(ListPolicies::class) - ->mountAction('sync') - ->callMountedAction() - ->assertSuccessful(); + $this->actingAs($user) + ->get(PolicyResource::getUrl('index', tenant: $tenant)) + ->assertNotFound(); // Verify no side effects Queue::assertNothingPushed(); @@ -101,12 +93,12 @@ [$user, $tenant] = createUserWithTenant(role: 'owner'); - $this->actingAs($user); $tenant->makeCurrent(); Filament::setTenant($tenant, true); // Start the test - action should be visible for member - $component = Livewire::test(ListPolicies::class) + $component = Livewire::actingAs($user) + ->test(ListPolicies::class) ->assertActionVisible('sync') ->assertActionEnabled('sync'); @@ -131,21 +123,22 @@ it('hides action in UI after membership revocation on re-render', function () { [$user, $tenant] = createUserWithTenant(role: 'owner'); - $this->actingAs($user); $tenant->makeCurrent(); Filament::setTenant($tenant, true); // Initial state - action visible - Livewire::test(ListPolicies::class) + Livewire::actingAs($user) + ->test(ListPolicies::class) ->assertActionVisible('sync'); // Revoke membership $user->tenants()->detach($tenant->getKey()); app(\App\Services\Auth\CapabilityResolver::class)->clearCache(); - // New component instance (simulates page refresh) - Livewire::test(ListPolicies::class) - ->assertActionHidden('sync'); + // New request (simulates page refresh) should now be tenant-denied + $this->actingAs($user) + ->get(PolicyResource::getUrl('index', tenant: $tenant)) + ->assertNotFound(); Queue::assertNothingPushed(); }); diff --git a/tests/Feature/RequiredPermissions/RequiredPermissionsCopyActionsTest.php b/tests/Feature/RequiredPermissions/RequiredPermissionsCopyActionsTest.php index 842072c..851c2dc 100644 --- a/tests/Feature/RequiredPermissions/RequiredPermissionsCopyActionsTest.php +++ b/tests/Feature/RequiredPermissions/RequiredPermissionsCopyActionsTest.php @@ -10,7 +10,7 @@ 'app_client_id' => null, ]); - [$user, $tenant] = createUserWithTenant(tenant: $tenant, role: 'readonly'); + [$user, $tenant] = createUserWithTenant(tenant: $tenant, role: 'readonly', ensureDefaultMicrosoftProviderConnection: false); $this->actingAs($user) ->get("/admin/tenants/{$tenant->external_id}/required-permissions") diff --git a/tests/Feature/RestoreRiskChecksWizardTest.php b/tests/Feature/RestoreRiskChecksWizardTest.php index f545b5d..4ca1792 100644 --- a/tests/Feature/RestoreRiskChecksWizardTest.php +++ b/tests/Feature/RestoreRiskChecksWizardTest.php @@ -28,6 +28,7 @@ ]); $tenant->makeCurrent(); + ensureDefaultProviderConnection($tenant, 'microsoft'); $policy = Policy::create([ 'tenant_id' => $tenant->id, @@ -148,6 +149,7 @@ ]); $tenant->makeCurrent(); + ensureDefaultProviderConnection($tenant, 'microsoft'); $policy = Policy::create([ 'tenant_id' => $tenant->id, @@ -244,6 +246,7 @@ ]); $tenant->makeCurrent(); + ensureDefaultProviderConnection($tenant, 'microsoft'); $policy = Policy::create([ 'tenant_id' => $tenant->id, diff --git a/tests/Feature/RestoreRunWizardExecuteTest.php b/tests/Feature/RestoreRunWizardExecuteTest.php index 88f8ed4..ec4e2d5 100644 --- a/tests/Feature/RestoreRunWizardExecuteTest.php +++ b/tests/Feature/RestoreRunWizardExecuteTest.php @@ -30,6 +30,7 @@ ]); $tenant->makeCurrent(); + ensureDefaultProviderConnection($tenant, 'microsoft'); $policy = Policy::create([ 'tenant_id' => $tenant->id, @@ -103,6 +104,7 @@ ]); $tenant->makeCurrent(); + ensureDefaultProviderConnection($tenant, 'microsoft'); $policy = Policy::create([ 'tenant_id' => $tenant->id, diff --git a/tests/Feature/RestoreRunWizardMetadataTest.php b/tests/Feature/RestoreRunWizardMetadataTest.php index 9d29991..86d9c20 100644 --- a/tests/Feature/RestoreRunWizardMetadataTest.php +++ b/tests/Feature/RestoreRunWizardMetadataTest.php @@ -25,6 +25,7 @@ ]); $tenant->makeCurrent(); + ensureDefaultProviderConnection($tenant, 'microsoft'); $backupSet = BackupSet::create([ 'tenant_id' => $tenant->id, diff --git a/tests/Feature/Verification/VerificationStartDedupeTest.php b/tests/Feature/Verification/VerificationStartDedupeTest.php index c1837f9..b94d716 100644 --- a/tests/Feature/Verification/VerificationStartDedupeTest.php +++ b/tests/Feature/Verification/VerificationStartDedupeTest.php @@ -13,7 +13,7 @@ it('dedupes verification starts while a run is active', function (): void { Queue::fake(); - [$user, $tenant] = createUserWithTenant(role: 'operator'); + [$user, $tenant] = createUserWithTenant(role: 'operator', ensureDefaultMicrosoftProviderConnection: false); $this->actingAs($user); $tenant->makeCurrent(); @@ -60,7 +60,7 @@ it('dedupes tenant-default verification starts while a run is active', function (): void { Queue::fake(); - [$user, $tenant] = createUserWithTenant(role: 'operator'); + [$user, $tenant] = createUserWithTenant(role: 'operator', ensureDefaultMicrosoftProviderConnection: false); $this->actingAs($user); $tenant->makeCurrent(); diff --git a/tests/Feature/VersionCaptureMetadataOnlyTest.php b/tests/Feature/VersionCaptureMetadataOnlyTest.php index 6560ab7..f866fc2 100644 --- a/tests/Feature/VersionCaptureMetadataOnlyTest.php +++ b/tests/Feature/VersionCaptureMetadataOnlyTest.php @@ -12,6 +12,7 @@ it('persists metadata-only snapshot metadata on captured versions', function () { $tenant = Tenant::factory()->create(); + ensureDefaultProviderConnection($tenant, 'microsoft'); $policy = Policy::factory()->for($tenant)->create([ 'policy_type' => 'mamAppConfiguration', 'platform' => 'mobile', diff --git a/tests/Feature/VersionCaptureWithAssignmentsTest.php b/tests/Feature/VersionCaptureWithAssignmentsTest.php index 2cc8455..4cdbccf 100644 --- a/tests/Feature/VersionCaptureWithAssignmentsTest.php +++ b/tests/Feature/VersionCaptureWithAssignmentsTest.php @@ -11,6 +11,7 @@ beforeEach(function () { $this->tenant = Tenant::factory()->create(); + ensureDefaultProviderConnection($this->tenant, 'microsoft'); $this->policy = Policy::factory()->create([ 'tenant_id' => $this->tenant->id, 'external_id' => 'test-policy-id', diff --git a/tests/Pest.php b/tests/Pest.php index 4117f08..9d288f6 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -128,6 +128,7 @@ function createUserWithTenant( ?User $user = null, string $role = 'owner', ?string $workspaceRole = null, + bool $ensureDefaultMicrosoftProviderConnection = true, ): array { $user ??= User::factory()->create(); $tenant ??= Tenant::factory()->create(); @@ -171,6 +172,10 @@ function createUserWithTenant( $tenant->getKey() => ['role' => $role], ]); + if ($ensureDefaultMicrosoftProviderConnection) { + ensureDefaultProviderConnection($tenant, 'microsoft'); + } + return [$user, $tenant]; } @@ -187,7 +192,7 @@ function ensureDefaultProviderConnection(Tenant $tenant, string $provider = 'mic $connection = ProviderConnection::query() ->where('tenant_id', (int) $tenant->getKey()) ->where('provider', $provider) - ->where('is_default', true) + ->orderByDesc('is_default') ->orderBy('id') ->first(); @@ -200,6 +205,30 @@ function ensureDefaultProviderConnection(Tenant $tenant, string $provider = 'mic 'health_status' => 'ok', 'is_default' => true, ]); + } else { + $entraTenantId = trim((string) $connection->entra_tenant_id); + + $updates = []; + if (! $connection->is_default) { + $updates['is_default'] = true; + } + + if ($connection->status !== 'connected') { + $updates['status'] = 'connected'; + } + + if ($connection->health_status !== 'ok') { + $updates['health_status'] = 'ok'; + } + + if ($entraTenantId === '') { + $updates['entra_tenant_id'] = (string) ($tenant->tenant_id ?? fake()->uuid()); + } + + if ($updates !== []) { + $connection->forceFill($updates)->save(); + $connection->refresh(); + } } $credential = $connection->credential()->first(); diff --git a/tests/Unit/AssignmentBackupServiceTest.php b/tests/Unit/AssignmentBackupServiceTest.php index 82a71a0..a04b6cf 100644 --- a/tests/Unit/AssignmentBackupServiceTest.php +++ b/tests/Unit/AssignmentBackupServiceTest.php @@ -17,6 +17,8 @@ 'external_id' => 'tenant-123', ]); + ensureDefaultProviderConnection($tenant, 'microsoft'); + $backupItem = BackupItem::factory()->create([ 'tenant_id' => $tenant->id, 'metadata' => [], diff --git a/tests/Unit/AssignmentFilterResolverTest.php b/tests/Unit/AssignmentFilterResolverTest.php index e07c0da..8872cb4 100644 --- a/tests/Unit/AssignmentFilterResolverTest.php +++ b/tests/Unit/AssignmentFilterResolverTest.php @@ -3,6 +3,7 @@ use App\Services\Graph\AssignmentFilterResolver; use App\Services\Graph\GraphResponse; use App\Services\Graph\MicrosoftGraphClient; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Cache; @@ -10,7 +11,7 @@ beforeEach(function () { Cache::flush(); $this->graphClient = Mockery::mock(MicrosoftGraphClient::class); - $this->resolver = new AssignmentFilterResolver($this->graphClient); + $this->resolver = new AssignmentFilterResolver($this->graphClient, app(MicrosoftGraphOptionsResolver::class)); }); test('resolves assignment filters by id', function () { diff --git a/tests/Unit/AssignmentRestoreServiceTest.php b/tests/Unit/AssignmentRestoreServiceTest.php index ef12ecb..78308ff 100644 --- a/tests/Unit/AssignmentRestoreServiceTest.php +++ b/tests/Unit/AssignmentRestoreServiceTest.php @@ -9,6 +9,10 @@ use App\Services\Graph\GraphLogger; use App\Services\Graph\GraphResponse; use App\Services\Intune\AuditLogger; +use App\Services\Providers\MicrosoftGraphOptionsResolver; +use Illuminate\Foundation\Testing\RefreshDatabase; + +uses(RefreshDatabase::class); beforeEach(function () { config()->set('graph_contracts.types.deviceManagementScript', [ @@ -37,15 +41,18 @@ app(GraphLogger::class), $this->auditLogger, $this->filterResolver, + app(MicrosoftGraphOptionsResolver::class), ); }); it('uses the contract assignment payload key for assign actions', function () { - $tenant = Tenant::factory()->make([ + $tenant = Tenant::factory()->create([ 'tenant_id' => 'tenant-123', 'app_client_id' => null, 'app_client_secret' => null, ]); + + ensureDefaultProviderConnection($tenant, 'microsoft'); $policyId = 'policy-123'; $assignments = [ [ @@ -92,11 +99,13 @@ }); it('uses derived assign endpoints for app protection policies', function () { - $tenant = Tenant::factory()->make([ + $tenant = Tenant::factory()->create([ 'tenant_id' => 'tenant-123', 'app_client_id' => null, 'app_client_secret' => null, ]); + + ensureDefaultProviderConnection($tenant, 'microsoft'); $policyId = 'policy-123'; $assignments = [ [ @@ -140,11 +149,13 @@ }); it('maps assignment filter ids stored at the root of assignments', function () { - $tenant = Tenant::factory()->make([ + $tenant = Tenant::factory()->create([ 'tenant_id' => 'tenant-123', 'app_client_id' => null, 'app_client_secret' => null, ]); + + ensureDefaultProviderConnection($tenant, 'microsoft'); $policyId = 'policy-789'; $assignments = [ [ @@ -200,11 +211,13 @@ }); it('keeps assignment filters when mapping is missing but filter exists in target', function () { - $tenant = Tenant::factory()->make([ + $tenant = Tenant::factory()->create([ 'tenant_id' => 'tenant-123', 'app_client_id' => null, 'app_client_secret' => null, ]); + + ensureDefaultProviderConnection($tenant, 'microsoft'); $policyId = 'policy-999'; $assignments = [ [ diff --git a/tests/Unit/FoundationMappingServiceTest.php b/tests/Unit/FoundationMappingServiceTest.php index 21d91bd..f7e9c10 100644 --- a/tests/Unit/FoundationMappingServiceTest.php +++ b/tests/Unit/FoundationMappingServiceTest.php @@ -11,6 +11,7 @@ use Mockery\MockInterface; uses(RefreshDatabase::class); + class FoundationMappingGraphClient implements GraphClientInterface { public array $requests = []; @@ -59,6 +60,8 @@ public function request(string $method, string $path, array $options = []): Grap it('maps existing foundations by display name', function () { $tenant = Tenant::factory()->create(); + + ensureDefaultProviderConnection($tenant, 'microsoft'); $backupSet = BackupSet::factory()->for($tenant)->create(); $item = BackupItem::factory() ->for($tenant) @@ -121,6 +124,8 @@ public function request(string $method, string $path, array $options = []): Grap 'app_client_id' => 'client-1', 'app_client_secret' => 'secret-1', ]); + + ensureDefaultProviderConnection($tenant, 'microsoft'); $backupSet = BackupSet::factory()->for($tenant)->create(); $item = BackupItem::factory() ->for($tenant) @@ -191,6 +196,8 @@ public function request(string $method, string $path, array $options = []): Grap it('skips built-in scope tags', function () { $tenant = Tenant::factory()->create(); + + ensureDefaultProviderConnection($tenant, 'microsoft'); $backupSet = BackupSet::factory()->for($tenant)->create(); $item = BackupItem::factory() ->for($tenant) diff --git a/tests/Unit/FoundationSnapshotServiceTest.php b/tests/Unit/FoundationSnapshotServiceTest.php index 3c644a3..302ebdd 100644 --- a/tests/Unit/FoundationSnapshotServiceTest.php +++ b/tests/Unit/FoundationSnapshotServiceTest.php @@ -57,6 +57,8 @@ public function request(string $method, string $path, array $options = []): Grap it('returns a failure when the foundation contract is missing', function () { $tenant = Tenant::factory()->create(); + ensureDefaultProviderConnection($tenant, 'microsoft'); + $client = new FoundationSnapshotGraphClient([]); app()->instance(GraphClientInterface::class, $client); @@ -80,6 +82,8 @@ public function request(string $method, string $path, array $options = []): Grap 'app_client_secret' => 'secret-123', ]); + ensureDefaultProviderConnection($tenant, 'microsoft'); + $responses = [ new GraphResponse( success: true, diff --git a/tests/Unit/Intune/VersionServiceConcurrencyTest.php b/tests/Unit/Intune/VersionServiceConcurrencyTest.php index 0923138..26956b3 100644 --- a/tests/Unit/Intune/VersionServiceConcurrencyTest.php +++ b/tests/Unit/Intune/VersionServiceConcurrencyTest.php @@ -10,6 +10,7 @@ use App\Services\Intune\AuditLogger; use App\Services\Intune\PolicySnapshotService; use App\Services\Intune\VersionService; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use Illuminate\Foundation\Testing\RefreshDatabase; uses(RefreshDatabase::class); @@ -26,6 +27,7 @@ groupResolver: Mockery::mock(GroupResolver::class), assignmentFilterResolver: Mockery::mock(AssignmentFilterResolver::class), scopeTagResolver: Mockery::mock(ScopeTagResolver::class), + graphOptionsResolver: app(MicrosoftGraphOptionsResolver::class), ); $fired = false; diff --git a/tests/Unit/PolicyCaptureOrchestratorTest.php b/tests/Unit/PolicyCaptureOrchestratorTest.php index 3debf5d..be2a81a 100644 --- a/tests/Unit/PolicyCaptureOrchestratorTest.php +++ b/tests/Unit/PolicyCaptureOrchestratorTest.php @@ -9,6 +9,7 @@ use App\Services\Intune\PolicyCaptureOrchestrator; use App\Services\Intune\PolicySnapshotService; use App\Services\Intune\VersionService; +use App\Services\Providers\MicrosoftGraphOptionsResolver; use Illuminate\Foundation\Testing\RefreshDatabase; uses(RefreshDatabase::class); @@ -20,6 +21,8 @@ 'is_current' => true, ]); + ensureDefaultProviderConnection($tenant, 'microsoft'); + $policy = Policy::factory()->create([ 'tenant_id' => $tenant->id, 'policy_type' => 'mamAppConfiguration', @@ -46,6 +49,7 @@ groupResolver: Mockery::mock(GroupResolver::class), assignmentFilterResolver: Mockery::mock(AssignmentFilterResolver::class), scopeTagResolver: Mockery::mock(ScopeTagResolver::class), + graphOptionsResolver: app(MicrosoftGraphOptionsResolver::class), ); $result = $orchestrator->capture( diff --git a/tests/Unit/TenantPermissionServiceTest.php b/tests/Unit/TenantPermissionServiceTest.php index c8692d6..3259d12 100644 --- a/tests/Unit/TenantPermissionServiceTest.php +++ b/tests/Unit/TenantPermissionServiceTest.php @@ -36,6 +36,8 @@ function requiredPermissions(): array 'name' => 'Tenant OK', ]); + ensureDefaultProviderConnection($tenant, 'microsoft'); + foreach (requiredPermissions() as $permission) { TenantPermission::create([ 'tenant_id' => $tenant->id, @@ -67,6 +69,8 @@ function requiredPermissions(): array 'name' => 'Tenant Missing', ]); + ensureDefaultProviderConnection($tenant, 'microsoft'); + $first = $permissions[0]['key']; TenantPermission::create([ 'tenant_id' => $tenant->id, @@ -101,6 +105,8 @@ function requiredPermissions(): array 'name' => 'Tenant Error', ]); + ensureDefaultProviderConnection($tenant, 'microsoft'); + $permissions = requiredPermissions(); $first = $permissions[0]['key']; @@ -136,6 +142,8 @@ function requiredPermissions(): array $tenant = Tenant::factory()->create(); + ensureDefaultProviderConnection($tenant, 'microsoft'); + TenantPermission::create([ 'tenant_id' => $tenant->id, 'permission_key' => 'DeviceManagementRBAC.ReadWrite.All',