From c76ae297e330f6f7d7a3224d4de97bfbb8b8c318 Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Sun, 15 Feb 2026 23:23:50 +0100 Subject: [PATCH] fix(onboarding): preserve workspace scope and consent flow --- .../ManagedTenantOnboardingWizard.php | 6 + app/Filament/Resources/TenantResource.php | 15 +-- .../AdminConsentCallbackController.php | 12 +- .../TenantOnboardingController.php | 7 + .../Directory/EntraGroupSyncService.php | 1 + .../Directory/RoleDefinitionsSyncService.php | 1 + app/Services/Intune/PolicySyncService.php | 1 + .../Inventory/DependencyExtractionService.php | 3 +- .../Inventory/InventorySyncService.php | 1 + .../VerificationReportSanitizer.php | 33 ++++- .../views/admin-consent-callback.blade.php | 12 +- tests/Feature/AdminConsentCallbackTest.php | 32 ++++- .../ManagedTenantOnboardingWizardTest.php | 19 +++ .../TenantOwnedWorkspaceInvariantTest.php | 123 ++++++++++++++++++ tests/Unit/TenantResourceConsentUrlTest.php | 16 +-- ...icationReportSanitizerNextStepsUrlTest.php | 36 +++++ 16 files changed, 286 insertions(+), 32 deletions(-) create mode 100644 tests/Feature/WorkspaceIsolation/TenantOwnedWorkspaceInvariantTest.php create mode 100644 tests/Unit/VerificationReportSanitizerNextStepsUrlTest.php diff --git a/app/Filament/Pages/Workspaces/ManagedTenantOnboardingWizard.php b/app/Filament/Pages/Workspaces/ManagedTenantOnboardingWizard.php index 6b50fc6..e916043 100644 --- a/app/Filament/Pages/Workspaces/ManagedTenantOnboardingWizard.php +++ b/app/Filament/Pages/Workspaces/ManagedTenantOnboardingWizard.php @@ -506,6 +506,12 @@ private function resumeLatestOnboardingSessionIfUnambiguous(): void private function initializeWizardData(): void { + // Ensure all entangled schema state paths exist at render time. + // Livewire v4 can throw when entangling to missing nested array keys. + $this->data['notes'] ??= ''; + $this->data['override_blocked'] ??= false; + $this->data['override_reason'] ??= ''; + if (! array_key_exists('connection_mode', $this->data)) { $this->data['connection_mode'] = 'existing'; } diff --git a/app/Filament/Resources/TenantResource.php b/app/Filament/Resources/TenantResource.php index 27efc67..f36e73c 100644 --- a/app/Filament/Resources/TenantResource.php +++ b/app/Filament/Resources/TenantResource.php @@ -1167,17 +1167,10 @@ public static function adminConsentUrl(Tenant $tenant): ?string return null; } - // Build explicit scope list from required permissions - $requiredPermissions = config('intune_permissions.permissions', []); - $scopes = collect($requiredPermissions) - ->pluck('key') - ->map(fn (string $permission) => "https://graph.microsoft.com/{$permission}") - ->join(' '); - - // Fallback to .default if no permissions configured - if (empty($scopes)) { - $scopes = 'https://graph.microsoft.com/.default'; - } + // Admin consent should use `.default` so the tenant consents to the app's configured + // application permissions. Keeping the URL short also avoids edge cases where a long + // scope string gets truncated and causes AADSTS900144 (missing `scope`). + $scopes = 'https://graph.microsoft.com/.default'; $query = http_build_query([ 'client_id' => $clientId, diff --git a/app/Http/Controllers/AdminConsentCallbackController.php b/app/Http/Controllers/AdminConsentCallbackController.php index 673c293..caaae87 100644 --- a/app/Http/Controllers/AdminConsentCallbackController.php +++ b/app/Http/Controllers/AdminConsentCallbackController.php @@ -20,6 +20,7 @@ public function __invoke( AuditLogger $auditLogger, ): View { $expectedState = $request->session()->pull('tenant_onboard_state'); + $workspaceId = $request->session()->pull('tenant_onboard_workspace_id'); $tenantKey = $request->string('tenant')->toString(); $state = $request->string('state')->toString(); $tenantIdentifier = $tenantKey ?: $this->parseState($state); @@ -30,7 +31,7 @@ public function __invoke( abort_if(empty($tenantIdentifier), 404); - $tenant = $this->resolveTenant($tenantIdentifier); + $tenant = $this->resolveTenant($tenantIdentifier, is_numeric($workspaceId) ? (int) $workspaceId : null); $error = $request->string('error')->toString() ?: null; $consentGranted = $request->has('admin_consent') @@ -75,7 +76,7 @@ public function __invoke( ]); } - private function resolveTenant(string $tenantIdentifier): Tenant + private function resolveTenant(string $tenantIdentifier, ?int $workspaceId): Tenant { /** @var Tenant|null $tenant */ $tenant = Tenant::withTrashed() @@ -87,12 +88,19 @@ private function resolveTenant(string $tenantIdentifier): Tenant } if ($tenant instanceof Tenant) { + if ($tenant->workspace_id === null && $workspaceId !== null) { + $tenant->forceFill(['workspace_id' => $workspaceId])->save(); + } + return $tenant; } + abort_if($workspaceId === null, ResponseAlias::HTTP_FORBIDDEN, 'Missing workspace context'); + return Tenant::create([ 'tenant_id' => $tenantIdentifier, 'name' => 'New Tenant', + 'workspace_id' => $workspaceId, ]); } diff --git a/app/Http/Controllers/TenantOnboardingController.php b/app/Http/Controllers/TenantOnboardingController.php index 62ee725..5b608a4 100644 --- a/app/Http/Controllers/TenantOnboardingController.php +++ b/app/Http/Controllers/TenantOnboardingController.php @@ -2,6 +2,7 @@ namespace App\Http\Controllers; +use App\Support\Workspaces\WorkspaceContext; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Illuminate\Support\Str; @@ -20,6 +21,12 @@ public function __invoke(Request $request): RedirectResponse $state = Str::uuid()->toString(); $request->session()->put('tenant_onboard_state', $state); + $workspaceId = app(WorkspaceContext::class)->currentWorkspaceId($request); + + if ($workspaceId !== null) { + $request->session()->put('tenant_onboard_workspace_id', (int) $workspaceId); + } + $url = "https://login.microsoftonline.com/{$tenantSegment}/v2.0/adminconsent?".http_build_query([ 'client_id' => $clientId, 'redirect_uri' => $redirectUri, diff --git a/app/Services/Directory/EntraGroupSyncService.php b/app/Services/Directory/EntraGroupSyncService.php index a0e111a..2bce61b 100644 --- a/app/Services/Directory/EntraGroupSyncService.php +++ b/app/Services/Directory/EntraGroupSyncService.php @@ -153,6 +153,7 @@ public function sync(Tenant $tenant, string $selectionKey): array $groupTypes = $item['groupTypes'] ?? null; $values = [ + 'workspace_id' => $tenant->workspace_id, 'display_name' => is_string($displayName) ? $displayName : $entraId, 'group_types' => is_array($groupTypes) ? $groupTypes : [], 'security_enabled' => (bool) ($item['securityEnabled'] ?? false), diff --git a/app/Services/Directory/RoleDefinitionsSyncService.php b/app/Services/Directory/RoleDefinitionsSyncService.php index 7f97a94..2234890 100644 --- a/app/Services/Directory/RoleDefinitionsSyncService.php +++ b/app/Services/Directory/RoleDefinitionsSyncService.php @@ -148,6 +148,7 @@ public function sync(Tenant $tenant): array $isBuiltIn = (bool) ($item['isBuiltIn'] ?? false); $values = [ + 'workspace_id' => $tenant->workspace_id, 'display_name' => is_string($displayName) ? $displayName : $entraId, 'is_built_in' => $isBuiltIn, 'last_seen_at' => $nowUtc, diff --git a/app/Services/Intune/PolicySyncService.php b/app/Services/Intune/PolicySyncService.php index b21d935..c2852d7 100644 --- a/app/Services/Intune/PolicySyncService.php +++ b/app/Services/Intune/PolicySyncService.php @@ -160,6 +160,7 @@ public function syncPoliciesWithReport(Tenant $tenant, ?array $supportedTypes = 'policy_type' => $policyType, ], [ + 'workspace_id' => $tenant->workspace_id, 'display_name' => $displayName, 'platform' => $policyPlatform, 'last_synced_at' => now(), diff --git a/app/Services/Inventory/DependencyExtractionService.php b/app/Services/Inventory/DependencyExtractionService.php index 90aed57..6f7d4b6 100644 --- a/app/Services/Inventory/DependencyExtractionService.php +++ b/app/Services/Inventory/DependencyExtractionService.php @@ -54,7 +54,7 @@ public function extractForPolicyData(InventoryItem $item, array $policyData): ar $limited = $sorted->take(50); $now = now(); - $payload = $limited->map(function (array $e) use ($now) { + $payload = $limited->map(function (array $e) use ($item, $now) { $metadata = $e['metadata'] ?? null; if (is_array($metadata)) { // Ensure portability across SQLite/Postgres when using upsert via query builder @@ -62,6 +62,7 @@ public function extractForPolicyData(InventoryItem $item, array $policyData): ar } return array_merge($e, [ + 'workspace_id' => $item->workspace_id, 'created_at' => $now, 'updated_at' => $now, ]); diff --git a/app/Services/Inventory/InventorySyncService.php b/app/Services/Inventory/InventorySyncService.php index 14acffc..49c60b6 100644 --- a/app/Services/Inventory/InventorySyncService.php +++ b/app/Services/Inventory/InventorySyncService.php @@ -302,6 +302,7 @@ private function executeSelectionUnderLock(OperationRun $operationRun, Tenant $t 'external_id' => $externalId, ], [ + 'workspace_id' => $tenant->workspace_id, 'display_name' => $displayName, 'category' => $typeConfig['category'] ?? null, 'platform' => $typeConfig['platform'] ?? null, diff --git a/app/Support/Verification/VerificationReportSanitizer.php b/app/Support/Verification/VerificationReportSanitizer.php index 566acd9..e6c4522 100644 --- a/app/Support/Verification/VerificationReportSanitizer.php +++ b/app/Support/Verification/VerificationReportSanitizer.php @@ -327,7 +327,7 @@ private static function sanitizeNextSteps(array $nextSteps): array } $label = self::sanitizeShortString($step['label'] ?? null, fallback: null); - $url = self::sanitizeShortString($step['url'] ?? null, fallback: null); + $url = self::sanitizeUrlString($step['url'] ?? null, fallback: null); if ($label === null || $url === null) { continue; @@ -342,6 +342,37 @@ private static function sanitizeNextSteps(array $nextSteps): array return $sanitized; } + private static function sanitizeUrlString(mixed $value, ?string $fallback): ?string + { + if (! is_string($value)) { + return $fallback; + } + + $value = trim($value); + + if ($value === '') { + return $fallback; + } + + if (self::containsForbiddenKeySubstring($value)) { + return $fallback; + } + + // URLs can be long (OAuth consent / remediation links). Truncating too aggressively can + // remove required query parameters (e.g. `scope`) and break the remediation flow. + $value = substr($value, 0, 2048); + + if (str_starts_with($value, '/')) { + return $value; + } + + if (str_starts_with($value, 'https://') || str_starts_with($value, 'http://')) { + return $value; + } + + return $fallback; + } + private static function sanitizeMessage(mixed $message): string { if (! is_string($message)) { diff --git a/resources/views/admin-consent-callback.blade.php b/resources/views/admin-consent-callback.blade.php index 3cfe8df..1a6a734 100644 --- a/resources/views/admin-consent-callback.blade.php +++ b/resources/views/admin-consent-callback.blade.php @@ -32,8 +32,16 @@ @endif

- - Zurück zur Tenant-Detailseite + @php + $isOnboarding = in_array($tenant->status, [\App\Models\Tenant::STATUS_DRAFT, \App\Models\Tenant::STATUS_ONBOARDING], true); + $backUrl = $isOnboarding + ? route('admin.onboarding') + : route('filament.admin.resources.tenants.view', ['tenant' => $tenant->external_id, 'record' => $tenant]); + $backLabel = $isOnboarding ? 'Zurück zum Onboarding' : 'Zurück zur Tenant-Detailseite'; + @endphp + + + {{ $backLabel }}

diff --git a/tests/Feature/AdminConsentCallbackTest.php b/tests/Feature/AdminConsentCallbackTest.php index 821d88b..d663bfe 100644 --- a/tests/Feature/AdminConsentCallbackTest.php +++ b/tests/Feature/AdminConsentCallbackTest.php @@ -2,13 +2,14 @@ use App\Models\ProviderConnection; use App\Models\Tenant; +use App\Models\Workspace; use App\Support\Providers\ProviderReasonCodes; use Illuminate\Foundation\Testing\RefreshDatabase; uses(RefreshDatabase::class); it('stores successful admin consent on provider connection status', function () { - $tenant = Tenant::create([ + $tenant = Tenant::factory()->create([ 'tenant_id' => 'tenant-1', 'name' => 'Contoso', ]); @@ -19,6 +20,10 @@ ])); $response->assertOk(); + $response->assertSee( + route('filament.admin.resources.tenants.view', ['tenant' => $tenant->external_id, 'record' => $tenant]), + false, + ); $connection = ProviderConnection::query() ->where('tenant_id', (int) $tenant->getKey()) @@ -37,8 +42,29 @@ ]); }); -it('creates tenant and provider connection when callback tenant does not exist', function () { +it('links back to onboarding when tenant is onboarding', function () { + $tenant = Tenant::factory()->create([ + 'tenant_id' => 'tenant-3', + 'name' => 'Onboarding Tenant', + 'status' => Tenant::STATUS_ONBOARDING, + ]); + $response = $this->get(route('admin.consent.callback', [ + 'tenant' => $tenant->tenant_id, + 'admin_consent' => 'true', + ])); + + $response->assertOk(); + $response->assertSee(route('admin.onboarding'), false); +}); + +it('creates tenant and provider connection when callback tenant does not exist', function () { + $workspace = Workspace::factory()->create(); + + $response = $this->withSession([ + 'tenant_onboard_workspace_id' => (int) $workspace->getKey(), + 'tenant_onboard_state' => 'state-456', + ])->get(route('admin.consent.callback', [ 'tenant' => 'new-tenant', 'state' => 'state-456', ])); @@ -60,7 +86,7 @@ }); it('records consent callback errors on provider connection state', function () { - $tenant = Tenant::create([ + $tenant = Tenant::factory()->create([ 'tenant_id' => 'tenant-2', 'name' => 'Fabrikam', ]); diff --git a/tests/Feature/ManagedTenantOnboardingWizardTest.php b/tests/Feature/ManagedTenantOnboardingWizardTest.php index aadb599..e27ebd5 100644 --- a/tests/Feature/ManagedTenantOnboardingWizardTest.php +++ b/tests/Feature/ManagedTenantOnboardingWizardTest.php @@ -97,6 +97,25 @@ ]); }); +it('initializes entangled wizard state keys to avoid Livewire entangle errors', function (): void { + $workspace = Workspace::factory()->create(); + $user = User::factory()->create(); + + WorkspaceMembership::factory()->create([ + 'workspace_id' => (int) $workspace->getKey(), + 'user_id' => (int) $user->getKey(), + 'role' => 'owner', + ]); + + session()->put(WorkspaceContext::SESSION_KEY, (int) $workspace->getKey()); + + Livewire::actingAs($user) + ->test(ManagedTenantOnboardingWizard::class) + ->assertSet('data.notes', '') + ->assertSet('data.override_blocked', false) + ->assertSet('data.override_reason', ''); +}); + it('is idempotent when identifying the same Entra tenant ID twice', function (): void { $workspace = Workspace::factory()->create(); $user = User::factory()->create(); diff --git a/tests/Feature/WorkspaceIsolation/TenantOwnedWorkspaceInvariantTest.php b/tests/Feature/WorkspaceIsolation/TenantOwnedWorkspaceInvariantTest.php new file mode 100644 index 0000000..db53cf9 --- /dev/null +++ b/tests/Feature/WorkspaceIsolation/TenantOwnedWorkspaceInvariantTest.php @@ -0,0 +1,123 @@ +create(); + $workspaceB = Workspace::factory()->create(); + + $tenant = Tenant::factory()->create([ + 'workspace_id' => $workspaceA->getKey(), + ]); + + $policy = Policy::factory()->create([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceA->getKey(), + ]); + + $backupSet = BackupSet::factory()->create([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceA->getKey(), + ]); + + $cases = [ + 'policies' => fn (int $workspaceId) => Policy::factory()->make([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceId, + ]), + 'policy_versions' => fn (int $workspaceId) => PolicyVersion::factory()->make([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceId, + 'policy_id' => $policy->getKey(), + ]), + 'backup_sets' => fn (int $workspaceId) => BackupSet::factory()->make([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceId, + ]), + 'backup_items' => fn (int $workspaceId) => BackupItem::factory()->make([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceId, + 'backup_set_id' => $backupSet->getKey(), + 'policy_id' => $policy->getKey(), + ]), + 'restore_runs' => fn (int $workspaceId) => RestoreRun::factory()->make([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceId, + 'backup_set_id' => $backupSet->getKey(), + ]), + 'backup_schedules' => fn (int $workspaceId) => BackupSchedule::make([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceId, + 'name' => 'Weekly backup', + 'is_enabled' => true, + 'timezone' => 'UTC', + 'frequency' => 'daily', + 'time_of_day' => '00:00:00', + 'days_of_week' => null, + 'policy_types' => ['settingsCatalogPolicy'], + 'include_foundations' => true, + 'retention_keep_last' => 30, + ]), + 'inventory_items' => fn (int $workspaceId) => InventoryItem::factory()->make([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceId, + ]), + 'inventory_links' => fn (int $workspaceId) => InventoryLink::factory()->make([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceId, + ]), + 'entra_groups' => fn (int $workspaceId) => EntraGroup::factory()->make([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceId, + ]), + 'findings' => fn (int $workspaceId) => Finding::factory()->make([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceId, + ]), + 'entra_role_definitions' => fn (int $workspaceId) => EntraRoleDefinition::factory()->make([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceId, + ]), + 'tenant_permissions' => fn (int $workspaceId) => TenantPermission::make([ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceId, + 'permission_key' => 'test.permission.'.uniqid(), + 'status' => 'missing', + ]), + ]; + + foreach ($cases as $table => $makeModel) { + $model = $makeModel((int) $workspaceA->getKey()); + + ($model::class)::withoutEvents(function () use ($model): void { + $model->save(); + }); + + $this->assertDatabaseHas($table, [ + 'tenant_id' => $tenant->getKey(), + 'workspace_id' => $workspaceA->getKey(), + ]); + + $mismatched = $makeModel((int) $workspaceB->getKey()); + + expect(fn () => $mismatched->save()) + ->toThrow(WorkspaceIsolationViolation::class); + } +}); diff --git a/tests/Unit/TenantResourceConsentUrlTest.php b/tests/Unit/TenantResourceConsentUrlTest.php index 5add285..57b67b5 100644 --- a/tests/Unit/TenantResourceConsentUrlTest.php +++ b/tests/Unit/TenantResourceConsentUrlTest.php @@ -8,7 +8,7 @@ uses(RefreshDatabase::class); it('includes scope parameter in admin consent url', function () { - // The adminConsentUrl builds scopes from intune_permissions config, not graph.scope + // The adminConsentUrl uses `.default` so the tenant consents to configured app permissions. $tenant = Tenant::create([ 'tenant_id' => 'b0091e5d-944f-4a34-bcd9-12cbfb7b75cf', 'name' => 'Test Tenant', @@ -17,17 +17,9 @@ $url = TenantResource::adminConsentUrl($tenant); - expect($url)->toContain('scope='); - - // Should contain permissions from intune_permissions config - $requiredPermissions = config('intune_permissions.permissions', []); - if (! empty($requiredPermissions)) { - $firstPermission = $requiredPermissions[0]['key']; - expect($url)->toContain(urlencode("https://graph.microsoft.com/{$firstPermission}")); - } else { - // Fallback to .default if no permissions configured - expect($url)->toContain(urlencode('https://graph.microsoft.com/.default')); - } + expect($url) + ->toContain('scope=') + ->toContain(urlencode('https://graph.microsoft.com/.default')); }); it('can derive admin consent url from provider connection credentials when tenant app_client_id is missing', function () { diff --git a/tests/Unit/VerificationReportSanitizerNextStepsUrlTest.php b/tests/Unit/VerificationReportSanitizerNextStepsUrlTest.php new file mode 100644 index 0000000..7c96239 --- /dev/null +++ b/tests/Unit/VerificationReportSanitizerNextStepsUrlTest.php @@ -0,0 +1,36 @@ + $clientId, + 'state' => 'tenantpilot|123', + 'redirect_uri' => $veryLongRedirect, + 'scope' => 'https://graph.microsoft.com/.default', + ]), + ); + + // This URL exceeds 200 chars; sanitizer must not drop the `scope`. + expect(strlen($url))->toBeGreaterThan(200); + + $sanitized = VerificationReportSanitizer::sanitizeNextStepsPayload([ + ['label' => 'Grant admin consent', 'url' => $url], + ]); + + expect($sanitized) + ->toBeArray() + ->not->toBeEmpty() + ->and($sanitized[0]['url'] ?? null)->toContain('scope=') + ->and($sanitized[0]['url'] ?? null)->toContain(urlencode('https://graph.microsoft.com/.default')); +});