ahmido
1acbf8cc54
## Summary - remove tenant-based Graph options access from runtime service paths and enforce provider-only resolution - add `MicrosoftGraphOptionsResolver` and `ProviderConfigurationRequiredException` for centralized, actionable provider-config errors - turn `Tenant::graphOptions()` into a fail-fast kill switch to prevent legacy runtime usage - add and update tests (including guardrail) to enforce no reintroduction in `app/` - update Spec 088 artifacts (`spec`, `plan`, `research`, `tasks`, checklist) ## Validation - `vendor/bin/sail bin pint --dirty` - `vendor/bin/sail artisan test --compact --filter=NoLegacyTenantGraphOptions` - `vendor/bin/sail artisan test --compact tests/Feature/Filament` - `CI=1 vendor/bin/sail artisan test --compact` ## Notes - Branch includes the guardrail test for legacy callsite detection in `app/`. - Full suite currently green: 1227 passed, 5 skipped. Co-authored-by: Ahmed Darrazi <ahmeddarrazi@MacBookPro.fritz.box> Reviewed-on: #105
72 lines
3.6 KiB
Markdown
72 lines
3.6 KiB
Markdown
# 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.
|