diff --git a/apps/platform/app/Filament/Resources/ProviderConnectionResource.php b/apps/platform/app/Filament/Resources/ProviderConnectionResource.php index a47b1de7..e18fcdaf 100644 --- a/apps/platform/app/Filament/Resources/ProviderConnectionResource.php +++ b/apps/platform/app/Filament/Resources/ProviderConnectionResource.php @@ -21,8 +21,8 @@ use App\Support\Badges\BadgeRenderer; use App\Support\Filament\TablePaginationProfiles; use App\Support\ManagedEnvironmentLinks; -use App\Support\Navigation\WorkspaceHubNavigation; use App\Support\Navigation\WorkspaceHubEnvironmentFilter; +use App\Support\Navigation\WorkspaceHubNavigation; use App\Support\OperationRunLinks; use App\Support\OperationRunType; use App\Support\OpsUx\OpsUxBrowserEvents; @@ -270,6 +270,12 @@ private static function resolveTenantExternalIdFromLivewireRequest(): ?string // Ignore and fall back to referer. } + $externalId = static::extractTenantExternalIdFromLivewireSnapshot(); + + if (is_string($externalId) && $externalId !== '') { + return $externalId; + } + $referer = request()->headers->get('referer'); if (! is_string($referer) || $referer === '') { @@ -279,6 +285,56 @@ private static function resolveTenantExternalIdFromLivewireRequest(): ?string return static::extractTenantExternalIdFromUrl($referer); } + private static function extractTenantExternalIdFromLivewireSnapshot(): ?string + { + $snapshotJson = request()->input('components.0.snapshot'); + + if (! is_string($snapshotJson) || $snapshotJson === '') { + return null; + } + + $snapshot = json_decode($snapshotJson, true); + + if (! is_array($snapshot)) { + return null; + } + + $componentName = data_get($snapshot, 'memo.name'); + + if (! is_string($componentName) || $componentName !== Pages\CreateProviderConnection::class) { + return null; + } + + $environmentId = data_get($snapshot, 'data.environmentId'); + + if (is_array($environmentId) || filter_var($environmentId, FILTER_VALIDATE_INT) === false) { + return null; + } + + $workspaceId = app(WorkspaceContext::class)->currentWorkspaceId(request()); + + if (! is_int($workspaceId)) { + return null; + } + + $tenant = ManagedEnvironment::query() + ->whereKey((int) $environmentId) + ->where('workspace_id', $workspaceId) + ->first(); + + if (! $tenant instanceof ManagedEnvironment) { + return null; + } + + $user = request()->user(); + + if ($user instanceof User && ! $user->canAccessTenant($tenant)) { + return null; + } + + return (string) $tenant->slug; + } + private static function extractTenantExternalIdFromUrl(string $url): ?string { $query = parse_url($url, PHP_URL_QUERY); @@ -341,14 +397,6 @@ private static function applyMembershipScope(Builder $query): Builder $workspaceId = app(WorkspaceContext::class)->currentWorkspaceId(request()); $user = auth()->user(); - if (! is_int($workspaceId)) { - $tenant = static::resolveTenantContextForCurrentPanel(); - - if ($tenant instanceof ManagedEnvironment) { - $workspaceId = (int) $tenant->workspace_id; - } - } - if (! is_int($workspaceId) || ! $user instanceof User) { return $query->whereRaw('1 = 0'); } diff --git a/apps/platform/app/Filament/Resources/ProviderConnectionResource/Pages/CreateProviderConnection.php b/apps/platform/app/Filament/Resources/ProviderConnectionResource/Pages/CreateProviderConnection.php index 0805ef04..d82bf5c6 100644 --- a/apps/platform/app/Filament/Resources/ProviderConnectionResource/Pages/CreateProviderConnection.php +++ b/apps/platform/app/Filament/Resources/ProviderConnectionResource/Pages/CreateProviderConnection.php @@ -9,19 +9,34 @@ use App\Support\Providers\ProviderConnectionType; use App\Support\Providers\ProviderConsentStatus; use App\Support\Providers\ProviderReasonCodes; +use App\Support\Providers\ProviderVerificationStatus; use App\Support\Providers\TargetScope\ProviderConnectionTargetScopeDescriptor; use App\Support\Providers\TargetScope\ProviderConnectionTargetScopeNormalizer; -use App\Support\Providers\ProviderVerificationStatus; use Filament\Notifications\Notification; use Filament\Resources\Pages\CreateRecord; use Illuminate\Validation\ValidationException; +use Livewire\Attributes\Locked; class CreateProviderConnection extends CreateRecord { protected static string $resource = ProviderConnectionResource::class; + #[Locked] + public ?int $environmentId = null; + protected bool $shouldMakeDefault = false; + public function mount(): void + { + $environmentId = request()->query('environment_id'); + + if (! is_array($environmentId) && filter_var($environmentId, FILTER_VALIDATE_INT) !== false) { + $this->environmentId = (int) $environmentId; + } + + parent::mount(); + } + protected function mutateFormDataBeforeCreate(array $data): array { $tenant = $this->currentTenant(); diff --git a/apps/platform/app/Policies/ProviderConnectionPolicy.php b/apps/platform/app/Policies/ProviderConnectionPolicy.php index d06a4c8b..7bfbf9f7 100644 --- a/apps/platform/app/Policies/ProviderConnectionPolicy.php +++ b/apps/platform/app/Policies/ProviderConnectionPolicy.php @@ -9,10 +9,10 @@ use App\Services\Auth\ManagedEnvironmentAccessScopeResolver; use App\Support\Auth\Capabilities; use App\Support\Workspaces\WorkspaceContext; -use Filament\Facades\Filament; use Illuminate\Auth\Access\HandlesAuthorization; use Illuminate\Auth\Access\Response; use Illuminate\Support\Facades\Gate; +use Livewire\Livewire as LivewireFacade; class ProviderConnectionPolicy { @@ -210,14 +210,6 @@ private function currentWorkspace(User $user): ?Workspace { $workspaceId = app(WorkspaceContext::class)->currentWorkspaceId(request()); - if (! is_int($workspaceId)) { - $filamentTenant = Filament::getTenant(); - - if ($filamentTenant instanceof ManagedEnvironment) { - $workspaceId = (int) $filamentTenant->workspace_id; - } - } - if (! is_int($workspaceId)) { return null; } @@ -237,33 +229,117 @@ private function currentWorkspace(User $user): ?Workspace private function resolveCreateTenant(Workspace $workspace): ?ManagedEnvironment { - $requestedEnvironmentId = request()->query('environment_id'); - - if (! is_numeric($requestedEnvironmentId)) { - $lastEnvironmentId = app(WorkspaceContext::class)->lastEnvironmentId(request()); - - if (is_int($lastEnvironmentId)) { - return ManagedEnvironment::query() - ->whereKey($lastEnvironmentId) - ->where('workspace_id', (int) $workspace->getKey()) - ->first(); - } - - $filamentTenant = Filament::getTenant(); - - if ($filamentTenant instanceof ManagedEnvironment && (int) $filamentTenant->workspace_id === (int) $workspace->getKey()) { - return $filamentTenant; - } + $requestedEnvironmentId = $this->requestedEnvironmentId(); + if ($requestedEnvironmentId === null) { return null; } return ManagedEnvironment::query() - ->whereKey((int) $requestedEnvironmentId) + ->whereKey($requestedEnvironmentId) ->where('workspace_id', (int) $workspace->getKey()) ->first(); } + private function requestedEnvironmentId(): ?int + { + $environmentId = request()->query('environment_id'); + + if (is_numeric($environmentId)) { + return (int) $environmentId; + } + + if (is_array($environmentId)) { + return null; + } + + try { + $resolved = $this->extractEnvironmentIdFromLivewireSnapshot(); + + if ($resolved !== null) { + return $resolved; + } + } catch (\Throwable) { + // Ignore and fall back to originalUrl() parsing. + } + + try { + $url = LivewireFacade::originalUrl(); + + $resolved = $this->extractEnvironmentIdFromUrl($url); + + if ($resolved !== null) { + return $resolved; + } + } catch (\Throwable) { + // Ignore and fall back to referer header. + } + + $referer = request()->headers->get('referer'); + + if (! is_string($referer) || $referer === '') { + return null; + } + + return $this->extractEnvironmentIdFromUrl($referer); + } + + private function extractEnvironmentIdFromLivewireSnapshot(): ?int + { + if (! request()->headers->has('x-livewire') && ! request()->headers->has('x-livewire-navigate')) { + return null; + } + + $snapshotJson = request()->input('components.0.snapshot'); + + if (! is_string($snapshotJson) || $snapshotJson === '') { + return null; + } + + $snapshot = json_decode($snapshotJson, true); + + if (! is_array($snapshot)) { + return null; + } + + $componentName = data_get($snapshot, 'memo.name'); + + if (! is_string($componentName) || $componentName !== \App\Filament\Resources\ProviderConnectionResource\Pages\CreateProviderConnection::class) { + return null; + } + + $environmentId = data_get($snapshot, 'data.environmentId'); + + if (is_array($environmentId) || filter_var($environmentId, FILTER_VALIDATE_INT) === false) { + return null; + } + + return (int) $environmentId; + } + + private function extractEnvironmentIdFromUrl(?string $url): ?int + { + if (! is_string($url) || $url === '') { + return null; + } + + $query = parse_url($url, PHP_URL_QUERY); + + if (! is_string($query) || $query === '') { + return null; + } + + parse_str($query, $params); + + $environmentId = $params['environment_id'] ?? null; + + if (is_array($environmentId) || filter_var($environmentId, FILTER_VALIDATE_INT) === false) { + return null; + } + + return (int) $environmentId; + } + private function tenantForConnection(ProviderConnection $connection): ?ManagedEnvironment { if ($connection->relationLoaded('tenant') && $connection->tenant instanceof ManagedEnvironment) { diff --git a/apps/platform/tests/Feature/Audit/ProviderConnectionIdentityAuditTest.php b/apps/platform/tests/Feature/Audit/ProviderConnectionIdentityAuditTest.php index c528f83f..678625d8 100644 --- a/apps/platform/tests/Feature/Audit/ProviderConnectionIdentityAuditTest.php +++ b/apps/platform/tests/Feature/Audit/ProviderConnectionIdentityAuditTest.php @@ -4,8 +4,8 @@ use App\Filament\Resources\ProviderConnectionResource\Pages\CreateProviderConnection; use App\Models\AuditLog; -use App\Models\ProviderConnection; use App\Models\ManagedEnvironment; +use App\Models\ProviderConnection; use App\Models\User; use Filament\Facades\Filament; use Illuminate\Foundation\Testing\RefreshDatabase; @@ -76,6 +76,9 @@ Filament::setTenant($tenant, true); Livewire::actingAs($user) + ->withQueryParams([ + 'environment_id' => (int) $tenant->getKey(), + ]) ->test(CreateProviderConnection::class) ->fillForm([ 'display_name' => 'Audit target scope connection', diff --git a/apps/platform/tests/Feature/ProviderConnections/MvpProviderScopeTest.php b/apps/platform/tests/Feature/ProviderConnections/MvpProviderScopeTest.php index c72a585f..6ab3f9c5 100644 --- a/apps/platform/tests/Feature/ProviderConnections/MvpProviderScopeTest.php +++ b/apps/platform/tests/Feature/ProviderConnections/MvpProviderScopeTest.php @@ -15,7 +15,11 @@ $tenant->makeCurrent(); Filament::setTenant($tenant, true); - Livewire::test(CreateProviderConnection::class) + $component = Livewire::withQueryParams([ + 'environment_id' => (int) $tenant->getKey(), + ])->test(CreateProviderConnection::class); + + $component ->fillForm([ 'display_name' => 'MVP Scope Connection', 'entra_tenant_id' => (string) fake()->uuid(), diff --git a/apps/platform/tests/Feature/ProviderConnections/ProviderConnectionNeutralitySpec238Test.php b/apps/platform/tests/Feature/ProviderConnections/ProviderConnectionNeutralitySpec238Test.php index fe697eed..eb79743b 100644 --- a/apps/platform/tests/Feature/ProviderConnections/ProviderConnectionNeutralitySpec238Test.php +++ b/apps/platform/tests/Feature/ProviderConnections/ProviderConnectionNeutralitySpec238Test.php @@ -81,6 +81,9 @@ Filament::setTenant($tenant, true); Livewire::actingAs($user) + ->withQueryParams([ + 'environment_id' => (int) $tenant->getKey(), + ]) ->test(CreateProviderConnection::class) ->fillForm([ 'display_name' => 'Missing target scope', diff --git a/apps/platform/tests/Feature/ProviderConnections/ScopeHardeningAuthoritySourcesTest.php b/apps/platform/tests/Feature/ProviderConnections/ScopeHardeningAuthoritySourcesTest.php new file mode 100644 index 00000000..eb137bca --- /dev/null +++ b/apps/platform/tests/Feature/ProviderConnections/ScopeHardeningAuthoritySourcesTest.php @@ -0,0 +1,197 @@ +active()->create([ + 'external_id' => 'spec339-scopehardening-env-remembered', + ]); + + [$user, $environment] = createUserWithTenant(tenant: $environment, role: 'owner'); + + $this->actingAs($user); + session()->put(WorkspaceContext::SESSION_KEY, (int) $environment->workspace_id); + session()->put(WorkspaceContext::LAST_ENVIRONMENT_IDS_SESSION_KEY, [ + (string) $environment->workspace_id => (int) $environment->getKey(), + ]); + + /** @var ProviderConnectionPolicy $policy */ + $policy = app(ProviderConnectionPolicy::class); + + $result = $policy->create($user); + + expect($result)->toBeInstanceOf(Response::class); + expect($result->denied())->toBeTrue(); +}); + +it('ScopeHardening requires explicit environment_id for ProviderConnectionPolicy::create even if Filament tenant is set', function (): void { + $environment = ManagedEnvironment::factory()->active()->create([ + 'external_id' => 'spec339-scopehardening-env-filament', + ]); + + [$user, $environment] = createUserWithTenant(tenant: $environment, role: 'owner'); + + Filament::setTenant($environment, true); + + $this->actingAs($user); + session()->put(WorkspaceContext::SESSION_KEY, (int) $environment->workspace_id); + + /** @var ProviderConnectionPolicy $policy */ + $policy = app(ProviderConnectionPolicy::class); + + $result = $policy->create($user); + + expect($result)->toBeInstanceOf(Response::class); + expect($result->denied())->toBeTrue(); +}); + +it('ScopeHardening ignores legacy query aliases for ProviderConnectionPolicy::create authority', function (): void { + $environment = ManagedEnvironment::factory()->active()->create([ + 'external_id' => 'spec339-scopehardening-env-alias', + ]); + + [$user, $environment] = createUserWithTenant(tenant: $environment, role: 'owner'); + + $request = Request::create('/admin/provider-connections/create', 'GET', [ + 'managed_environment_id' => (int) $environment->getKey(), + ]); + + $this->app->instance('request', $request); + + $this->actingAs($user); + session()->put(WorkspaceContext::SESSION_KEY, (int) $environment->workspace_id); + session()->put(WorkspaceContext::LAST_ENVIRONMENT_IDS_SESSION_KEY, [ + (string) $environment->workspace_id => (int) $environment->getKey(), + ]); + + /** @var ProviderConnectionPolicy $policy */ + $policy = app(ProviderConnectionPolicy::class); + + $result = $policy->create($user); + + expect($result)->toBeInstanceOf(Response::class); + expect($result->denied())->toBeTrue(); +}); + +it('ScopeHardening denies ProviderConnectionPolicy::create for environment_id that belongs to another workspace', function (): void { + $environmentA = ManagedEnvironment::factory()->active()->create([ + 'external_id' => 'spec339-scopehardening-env-a', + ]); + [$user, $environmentA] = createUserWithTenant(tenant: $environmentA, role: 'owner'); + + $workspaceB = Workspace::factory()->create(); + $environmentB = ManagedEnvironment::factory()->active()->create([ + 'workspace_id' => (int) $workspaceB->getKey(), + 'external_id' => 'spec339-scopehardening-env-b', + ]); + + $request = Request::create('/admin/provider-connections/create', 'GET', [ + 'environment_id' => (int) $environmentB->getKey(), + ]); + + $this->app->instance('request', $request); + + $this->actingAs($user); + session()->put(WorkspaceContext::SESSION_KEY, (int) $environmentA->workspace_id); + + /** @var ProviderConnectionPolicy $policy */ + $policy = app(ProviderConnectionPolicy::class); + + $result = $policy->create($user); + + expect($result)->toBeInstanceOf(Response::class); + expect($result->denied())->toBeTrue(); +}); + +it('ScopeHardening returns 403 (not 404) for the create page when environment_id is missing (UI entry behavior)', function (): void { + $environment = ManagedEnvironment::factory()->active()->create([ + 'external_id' => 'spec339-scopehardening-create-page', + ]); + + [$user, $environment] = createUserWithTenant(tenant: $environment, role: 'owner'); + + $this->actingAs($user) + ->withSession([ + WorkspaceContext::SESSION_KEY => (int) $environment->workspace_id, + ]) + ->get('/admin/provider-connections/create') + ->assertForbidden(); +}); + +it('ScopeHardening returns 404 for the create page when environment_id belongs to another workspace (UI entry behavior)', function (): void { + $environmentA = ManagedEnvironment::factory()->active()->create([ + 'external_id' => 'spec339-scopehardening-create-page-a', + ]); + [$user, $environmentA] = createUserWithTenant(tenant: $environmentA, role: 'owner'); + + $workspaceB = Workspace::factory()->create(); + $environmentB = ManagedEnvironment::factory()->active()->create([ + 'workspace_id' => (int) $workspaceB->getKey(), + 'external_id' => 'spec339-scopehardening-create-page-b', + ]); + + $this->actingAs($user) + ->withSession([ + WorkspaceContext::SESSION_KEY => (int) $environmentA->workspace_id, + ]) + ->get('/admin/provider-connections/create?environment_id='.(int) $environmentB->getKey()) + ->assertNotFound(); +}); + +it('ScopeHardening returns 403 (not 404) for the create page when legacy alias is provided (UI entry behavior)', function (): void { + $environment = ManagedEnvironment::factory()->active()->create([ + 'external_id' => 'spec339-scopehardening-create-page-alias', + ]); + + [$user, $environment] = createUserWithTenant(tenant: $environment, role: 'owner'); + + $this->actingAs($user) + ->withSession([ + WorkspaceContext::SESSION_KEY => (int) $environment->workspace_id, + ]) + ->get('/admin/provider-connections/create?managed_environment_id='.(int) $environment->getKey()) + ->assertForbidden(); +}); + +it('ScopeHardening returns 403 (not 404) for the create page even if a remembered environment exists (UI entry behavior)', function (): void { + $environment = ManagedEnvironment::factory()->active()->create([ + 'external_id' => 'spec339-scopehardening-create-page-remembered', + ]); + + [$user, $environment] = createUserWithTenant(tenant: $environment, role: 'owner'); + + $this->actingAs($user) + ->withSession([ + WorkspaceContext::SESSION_KEY => (int) $environment->workspace_id, + WorkspaceContext::LAST_ENVIRONMENT_IDS_SESSION_KEY => [ + (string) $environment->workspace_id => (int) $environment->getKey(), + ], + ]) + ->get('/admin/provider-connections/create') + ->assertForbidden(); +}); + +it('ScopeHardening returns 403 (not 404) for the create page even if Filament tenant is set (UI entry behavior)', function (): void { + $environment = ManagedEnvironment::factory()->active()->create([ + 'external_id' => 'spec339-scopehardening-create-page-filament', + ]); + + [$user, $environment] = createUserWithTenant(tenant: $environment, role: 'owner'); + + Filament::setTenant($environment, true); + + $this->actingAs($user) + ->withSession([ + WorkspaceContext::SESSION_KEY => (int) $environment->workspace_id, + ]) + ->get('/admin/provider-connections/create') + ->assertForbidden(); +}); diff --git a/apps/platform/tests/Feature/ProviderConnections/ScopeHardeningPolicyWorkspaceAuthorityTest.php b/apps/platform/tests/Feature/ProviderConnections/ScopeHardeningPolicyWorkspaceAuthorityTest.php new file mode 100644 index 00000000..c91f5876 --- /dev/null +++ b/apps/platform/tests/Feature/ProviderConnections/ScopeHardeningPolicyWorkspaceAuthorityTest.php @@ -0,0 +1,32 @@ +active()->create([ + 'external_id' => 'spec339-scopehardening-policy-env', + ]); + + [$user, $environment] = createUserWithTenant(tenant: $environment, role: 'owner'); + + expect($user)->toBeInstanceOf(User::class); + + session()->forget(WorkspaceContext::SESSION_KEY); + + Filament::setTenant($environment, true); + + /** @var ProviderConnectionPolicy $policy */ + $policy = app(ProviderConnectionPolicy::class); + + $result = $policy->viewAny($user); + + expect($result)->toBeInstanceOf(Response::class); + expect($result->denied())->toBeTrue(); +}); diff --git a/apps/platform/tests/Feature/ProviderConnections/ScopeHardeningResourceWorkspaceAuthorityTest.php b/apps/platform/tests/Feature/ProviderConnections/ScopeHardeningResourceWorkspaceAuthorityTest.php new file mode 100644 index 00000000..e4b88163 --- /dev/null +++ b/apps/platform/tests/Feature/ProviderConnections/ScopeHardeningResourceWorkspaceAuthorityTest.php @@ -0,0 +1,35 @@ +active()->create([ + 'external_id' => 'spec339-scopehardening-resource-env', + ]); + + [$user, $environment] = createUserWithTenant(tenant: $environment, role: 'owner'); + + $connection = ProviderConnection::factory()->create([ + 'workspace_id' => (int) $environment->workspace_id, + 'managed_environment_id' => (int) $environment->getKey(), + 'display_name' => 'Spec339 ScopeHardening Connection', + 'provider' => 'microsoft', + ]); + + $this->actingAs($user); + + session()->forget(WorkspaceContext::SESSION_KEY); + + Filament::setTenant($environment, true); + + $component = Livewire::test(ListProviderConnections::class); + + expect($component->instance())->toBeNull(); +}); diff --git a/specs/339-provider-connection-scope-hardening/plan.md b/specs/339-provider-connection-scope-hardening/plan.md new file mode 100644 index 00000000..ae8e30a1 --- /dev/null +++ b/specs/339-provider-connection-scope-hardening/plan.md @@ -0,0 +1,314 @@ +# Implementation Plan: Spec 339 - Provider Connection Scope Hardening + +- Branch: `339-provider-connection-scope-hardening` +- Date: 2026-05-31 +- Spec: `specs/339-provider-connection-scope-hardening/spec.md` +- Input: Spec 339 + repo inspection of Provider Connection policy/resource authority seams. + +## Summary + +Harden Provider Connections (credential-adjacent) so that authorization and scope authority come only from: + +- explicit workspace context (`WorkspaceContext`, session), +- explicit `environment_id` (filter/create), or +- record ownership (workspace + managed environment) for view/edit/actions. + +Remove hidden fallbacks (remembered environment and Filament tenant) as authority sources and guard the contract with targeted tests. + +## Technical Context + +- Language/Version: PHP 8.4.15, Laravel 12.52.x. +- Primary Dependencies: Filament 5.x, Livewire 4.x, Pest 4.x. +- Storage: PostgreSQL; no schema change expected. +- Testing: Pest Feature tests (ProviderConnections family). +- Validation Lanes: fast-feedback (Feature). Browser smoke only if visible UX changes are introduced. +- Target Platform: Sail locally; Dokploy/container posture unchanged. +- Project Type: Laravel monolith under `apps/platform`. +- Constraints: no new persisted truth, migrations, packages, or broad navigation redesign. + +## UI / Surface Guardrail Plan + +- **Guardrail scope**: existing operator-facing resource + credential-adjacent action gating (authority hardening). +- **Affected surfaces**: + - Provider Connections list/view/create/edit routes (`/admin/provider-connections*`). + - Create CTA availability and create authorization (must require explicit `environment_id`). + - Policy semantics (404 vs 403) and record-derived authority for actions. +- **Native vs custom**: native Filament + existing capability/policy seams; no custom UI framework. +- **Shared-family relevance**: workspace hub filter contract, authorization semantics, action-surface discipline for credential-adjacent actions. +- **State layers in scope**: URL query (`environment_id` only), session workspace context, record ownership. +- **Handling modes**: review-mandatory (security semantics). +- **Required tests / smoke**: + - Feature tests that prove forbidden fallbacks cannot authorize create/view/actions. + - Optional reuse of existing browser smoke only if UI copy/CTA visibility changes are required. +- **UI/Productization coverage**: no new surface; do not update UI audit coverage files unless navigation routes change. + +## Shared Pattern & System Fit + +- **Cross-cutting feature marker**: yes. +- **Systems touched (expected)**: + - `apps/platform/app/Policies/ProviderConnectionPolicy.php` + - `apps/platform/app/Filament/Resources/ProviderConnectionResource.php` + - `apps/platform/app/Support/Workspaces/WorkspaceContext.php` + - `apps/platform/app/Support/Navigation/WorkspaceHubEnvironmentFilter.php` + - `apps/platform/tests/Feature/ProviderConnections/*` +- **Shared abstractions reused**: `WorkspaceContext`, deny-as-not-found semantics, capability resolver and gates, existing ProviderConnections test family. +- **New abstraction introduced?**: none. +- **Bounded deviation / spread control**: do not introduce a new “scope resolver framework”; keep changes local to the ProviderConnection policy/resource seams. + +## OperationRun UX Impact + +No new `OperationRun` types or start UX changes. + +Provider operations reachable from Provider Connections must remain scoped to the record’s owning environment/workspace, but this spec’s changes should be limited to authority and authorization guardrails. + +## Implementation Approach + +### Phase 1 — Repo truth + failing tests first + +- Inventory the current forbidden fallbacks: + - `ProviderConnectionPolicy::currentWorkspace()` fallback to `Filament::getTenant()`. + - `ProviderConnectionPolicy::resolveCreateTenant()` fallback to `WorkspaceContext::lastEnvironmentId()` and `Filament::getTenant()`. + - Provider Connection resource query scoping fallback paths when workspace context is missing. +- Add failing tests proving: + - create is impossible without explicit `environment_id` (even if remembered environment exists), + - Filament tenant cannot substitute for workspace context, + - legacy query aliases do not widen access or grant authority. + +### Phase 2 — Policy authority hardening + +- Remove forbidden fallbacks in `ProviderConnectionPolicy`: + - workspace authority must come from `WorkspaceContext` only, + - create tenant must come from explicit `environment_id` only. +- Keep 404 vs 403 semantics consistent with repo rules. + +### Phase 3 — Resource scoping hardening + +- Ensure `ProviderConnectionResource` list/query scoping: + - requires workspace context for membership scope (no tenant-context fallback), + - uses `environment_id` only as an explicit filter, validated against workspace/access scope. +- Ensure record pages/actions always use record-derived tenant/workspace (no hidden context authority). + +### Phase 4 — Validation and regression posture + +Run narrow tests first: + +- `cd apps/platform && ./vendor/bin/sail artisan test --compact tests/Feature/ProviderConnections --filter=ScopeHardening` +- `cd apps/platform && ./vendor/bin/sail pint --dirty --format agent` + +Run optional browser smoke only if implementation changes visible CTA/copy: + +- `cd apps/platform && ./vendor/bin/sail artisan test --compact tests/Browser/Spec281ProviderConnectionScopeSmokeTest.php` + +## Deployment / Ops Impact + +- Migrations: none. +- Env vars: none. +- Queues/scheduler: none (no new job orchestration beyond existing provider operations). +- Filament assets: unchanged. + +> **Fill when the feature touches shared provider/platform seams, identity scope, governed-subject taxonomy, compare strategy selection, provider connection descriptors, or operator vocabulary that may leak provider-specific semantics into platform-core truth. Docs-only or template-only work may use concise `N/A`.** + +- **Shared provider/platform boundary touched?**: [yes / no / N/A] +- **Provider-owned seams**: [List or `N/A`] +- **Platform-core seams**: [List or `N/A`] +- **Neutral platform terms / contracts preserved**: [List or `N/A`] +- **Retained provider-specific semantics and why**: [none / short explanation] +- **Bounded extraction or follow-up path**: [none / document-in-feature / follow-up-spec / N/A] + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +- Inventory-first: clarify what is “last observed” vs snapshots/backups +- Read/write separation: any writes require preview + confirmation + audit + tests +- Graph contract path: Graph calls only via `GraphClientInterface` + `config/graph_contracts.php` +- Deterministic capabilities: capability derivation is testable (snapshot/golden tests) +- RBAC-UX: two planes (/admin vs /system) remain separated; cross-plane is 404; tenant-context routes (/admin/t/{tenant}/...) are tenant-scoped; canonical workspace-context routes under /admin remain tenant-safe; non-member tenant/workspace access is 404; member-but-missing-capability is 403; authorization checks use Gates/Policies + capability registries (no raw strings, no role-string checks) +- Workspace isolation: non-member workspace access is 404; tenant-plane routes require an established workspace context; workspace context switching is separate from Filament Tenancy +- RBAC-UX: destructive-like actions require `->requiresConfirmation()` and clear warning text +- RBAC-UX: global search is tenant-scoped; non-members get no hints; inaccessible results are treated as not found (404 semantics) +- Tenant isolation: all reads/writes tenant-scoped; cross-tenant views are explicit and access-checked +- Run observability: long-running/remote/queued work creates/reuses `OperationRun`; start surfaces enqueue-only; Monitoring is DB-only; DB-only <2s actions may skip runs but security-relevant ones still audit-log; auth handshake exception OPS-EX-AUTH-001 allows synchronous outbound HTTP on `/auth/*` without `OperationRun` +- OperationRun start UX: any feature that creates, queues, deduplicates, resumes, blocks, completes, or links `OperationRun` reuses the central OperationRun Start UX Contract; no local composition of queued toast/link/event/start-state messaging; `OperationRun UX Impact` is present in the active spec or plan +- Ops-UX 3-surface feedback: if `OperationRun` is used, default feedback is toast intent-only + progress surfaces + exactly-once terminal `OperationRunCompleted` (initiator-only); queued DB notifications remain explicit opt-in through the shared start UX contract; running DB notifications stay disallowed +- Ops-UX lifecycle: `OperationRun.status` / `OperationRun.outcome` transitions are service-owned (only via `OperationRunService`); context-only updates allowed outside +- Ops-UX summary counts: `summary_counts` keys come from `OperationSummaryKeys::all()` and values are flat numeric-only +- Ops-UX guards: CI has regression guards that fail with actionable output (file + snippet) when these patterns regress +- Ops-UX system runs: initiator-null runs emit no terminal DB notification; audit remains via Monitoring; tenant-wide alerting goes through Alerts (not OperationRun notifications) +- Automation: queued/scheduled ops use locks + idempotency; handle 429/503 with backoff+jitter +- Data minimization: Inventory stores metadata + whitelisted meta; logs contain no secrets/tokens +- Test governance (TEST-GOV-001): actual test-purpose classification, affected lanes, fixture/helper/factory/seed/context cost risks, heavy-family visibility, review-stop points, reviewer handoff, and any budget/baseline/trend follow-up are explicit; the narrowest proving lane mix is planned and any structural cost change has an escalation path +- Proportionality (PROP-001): any new structure, layer, persisted truth, or semantic machinery is justified by current release truth, current operator workflow, and why a narrower solution is insufficient +- No premature abstraction (ABSTR-001): no new factories, registries, resolvers, strategy systems, interfaces, type registries, or orchestration pipelines before at least 2 real concrete cases exist, unless security, tenant isolation, auditability, compliance evidence, or queue correctness require it now +- Persisted truth (PERSIST-001): new tables/entities/artifacts represent independent product truth or lifecycle; convenience projections and UI helpers stay derived +- Behavioral state (STATE-001): new states/statuses/reason codes change behavior, routing, permissions, lifecycle, audit, retention, or retry handling; presentation-only distinctions stay derived +- UI semantics (UI-SEM-001): avoid turning badges, explanation text, trust/confidence labels, or detail summaries into mandatory interpretation frameworks; prefer direct domain-to-UI mapping +- Shared pattern first (XCUT-001): cross-cutting interaction classes reuse existing shared contracts/presenters/builders/renderers first; any deviation is explicit, bounded, and justified against current-release truth +- Provider boundary (PROV-001): shared provider/platform seams are classified as provider-owned vs platform-core; provider-specific semantics stay out of platform-core contracts, taxonomy, identifiers, compare semantics, and operator vocabulary unless explicitly justified; bounded extraction beats speculative multi-provider frameworks +- V1 explicitness / few layers (V1-EXP-001, LAYER-001): prefer direct implementation, local mappings, and small helpers; any new layer replaces an old one or proves the old one cannot serve +- Spec discipline / bloat check (SPEC-DISC-001, BLOAT-001): related semantic changes are grouped coherently, and any new enum, DTO/presenter, persisted entity, interface/registry/resolver, or taxonomy includes a proportionality review covering operator problem, insufficiency, narrowness, ownership cost, rejected alternative, and whether it is current-release truth +- Badge semantics (BADGE-001): status-like badges use `BadgeCatalog` / `BadgeRenderer`; no ad-hoc mappings; new values include tests +- Filament-native UI (UI-FIL-001): admin/operator surfaces use native Filament components or shared primitives first; no ad-hoc status UI, local semantic color/border decisions, or hand-built replacements when native/shared semantics exist; any exception is explicitly justified +- Filament-native UI (UI-FIL-001): custom Filament UI follows `docs/ui/tenantpilot-enterprise-ui-standards.md`; no ad-hoc custom styling for cards, buttons, hovers, badges, icons, progress bars, empty states, or interactive rows +- Filament-native UI (UI-FIL-001): if local Blade/Tailwind cards are + still necessary, they preserve dark mode correctness, spacing + consistency, badge semantics, action hierarchy, progressive + disclosure, accessibility, and Filament visual language +- Filament-native UI (UI-FIL-001): custom Blade/Widget/Page surfaces + keep Filament-native interaction semantics, preserve one dominant + primary action per focused area, express state through + BADGE-001-aligned badges/labels/supporting text instead of arbitrary + button colors, and do not create independent button/card/spacing + systems +- Filament-native UI (UI-FIL-001): hover, pointer, focus, shadow, or + similar interactive affordance appears only when a repo-real + route/action and permitted capability exist; non-interactive rows + stay visibly static +- UI/UX surface taxonomy (UI-CONST-001 / UI-SURF-001): every changed operator-facing surface is classified as exactly one allowed surface type; ad-hoc interaction models are forbidden +- Decision-first operating model (DECIDE-001): each changed + operator-facing surface is classified as Primary Decision, + Secondary Context, or Tertiary Evidence / Diagnostics; primary + surfaces justify the human-in-the-loop moment, default-visible info + is limited to first-decision needs, deep proof is progressive + disclosed, one governance case stays decidable in one context where + practical, navigation follows workflows not storage structures, and + automation / alerts reduce attention load instead of adding noise +- Audience-aware disclosure (DECIDE-AUD-001 / OPSURF-001): detail or + status surfaces separate customer-readable decision content, + operator diagnostics, and support/raw evidence; customer-readable + default paths hide raw JSON, copied context, fingerprints, internal + reason ownership, platform reason families, and debug semantics; + one dominant next action is explicit; duplicate visible truth is + removed +- UI/UX inspect model (UI-HARD-001): each list surface has exactly one primary inspect/open model; redundant View beside row click or identifier click is forbidden; edit-as-inspect is limited to Config-lite resources +- UI/UX action hierarchy (UI-HARD-001 / UI-EX-001): standard CRUD and Registry rows expose at most one inline safe shortcut; destructive actions are grouped or in the detail header; queue exceptions are catalogued, justified, and tested +- UI/UX scope, truth, and naming (UI-HARD-001 / UI-NAMING-001 / OPSURF-001): scope signals are truthful, canonical nouns stay stable across shells, critical operational truth is default-visible, and standard lists remain scanable +- UI/UX placeholder ban (UI-HARD-001): empty `ActionGroup` / `BulkActionGroup` placeholders and declaration-only UI conformance are forbidden +- UI naming (UI-NAMING-001): operator-facing labels use `Verb + Object`; scope (`Workspace`, `Tenant`) is never the primary action label; source/domain is secondary unless disambiguation is required; runs/toasts/audit prose use the same domain vocabulary; implementation-first terms do not appear in primary operator UI +- Operator surfaces (OPSURF-001): `/admin` defaults are operator-first; default-visible content avoids raw implementation detail; diagnostics are explicitly revealed secondarily +- Operator surfaces (OPSURF-001): execution outcome, data completeness, governance result, and lifecycle/readiness are modeled as distinct status dimensions when all apply; they are not collapsed into one ambiguous status +- Operator surfaces (OPSURF-001): every mutating action communicates whether it changes TenantPilot only, the Microsoft tenant, or simulation only before execution +- Operator surfaces (OPSURF-001): dangerous actions follow configuration → safety checks/simulation → preview → hard confirmation where required → execute, unless a spec documents an explicit exemption and replacement safeguards +- Operator surfaces (OPSURF-001): workspace and tenant context remain explicit in navigation, actions, and page semantics; tenant surfaces do not silently expose workspace-wide actions +- Operator surfaces (OPSURF-001): each new or materially refactored operator-facing page defines a page contract covering persona, surface type, operator question, default-visible info, diagnostics-only info, status dimensions, mutation scope, primary actions, and dangerous actions +- Filament UI Action Surface Contract: for any new/modified Filament Resource/RelationManager/Page, define Header/Row/Bulk/Empty-State actions, ensure every List/Table has a surface-appropriate inspect affordance, remove redundant View when row click or identifier click already opens the same destination, keep standard CRUD/Registry rows to inspect plus at most one inline safe shortcut, group or relocate the rest to “More” or detail header, forbid empty bulk/overflow groups, require confirmations for destructive actions, write audit logs for mutations, enforce RBAC via central helpers (non-member 404, member missing capability 403), and ensure CI blocks merges if the contract is violated or not explicitly exempted +- Filament UI UX-001 (Layout & IA): Create/Edit uses Main/Aside (3-col grid, Main=columnSpan(2), Aside=columnSpan(1)); all fields inside Sections/Cards (no naked inputs); View uses Infolists (not disabled edit forms); status badges use BADGE-001; empty states have specific title + explanation + 1 CTA; max 1 primary + 1 secondary header action (see HDR-001); tables provide search/sort/filters for core dimensions; shared layout builders preferred for consistency +- Action-surface discipline (ACTSURF-001 / HDR-001): every changed + surface declares one broad action-surface class; the spec names the + one likely next operator action; navigation is separated from + mutation; record/detail/edit pages keep at most one visible primary + header action; monitoring/workbench surfaces separate scope/context, + selection actions, navigation, and object actions; risky or rare + actions are grouped and ordered by meaning/frequency/risk; any special + type or workflow-hub exception is explicit and justified +- UI review workflow: native/custom classification, shared-family + relevance, state-layer ownership, repository-signal treatment, + exception path, and the active feature PR close-out entry stay + explicit without duplicating the same decision across spec, plan, + tasks, checklist, and close-out surfaces +- UI/Productization coverage (UI-COV-001): every spec completes the UI + Surface Impact decision; changed reachable UI surfaces update + `docs/ui-ux-enterprise-audit/` coverage artifacts with page archetype, + design depth, repo-truth level, target pattern/mockup need, + customer-safe review need, and dangerous-action review need; a checked + `No UI surface impact` decision is used only with a short rationale + +## Test Governance Check + +> **Fill for any runtime-changing or test-affecting feature. Docs-only or template-only work may state concise `N/A` or `none`.** + +- **Test purpose / classification by changed surface**: [Unit / Feature / Heavy-Governance / Browser / N/A] +- **Affected validation lanes**: [fast-feedback / confidence / heavy-governance / browser / profiling / junit / N/A] +- **Why this lane mix is the narrowest sufficient proof**: [Why the chosen classification and lanes fit the actual proving purpose] +- **Narrowest proving command(s)**: [Exact commands reviewers should run before merge] +- **Fixture / helper / factory / seed / context cost risks**: [none / describe] +- **Expensive defaults or shared helper growth introduced?**: [no / describe explicit opt-in path] +- **Heavy-family additions, promotions, or visibility changes**: [none / describe] +- **Surface-class relief / special coverage rule**: [standard-native relief / named special profile / N/A] +- **Closing validation and reviewer handoff**: [What must be re-run, what reviewers should verify, and what exact proof command they should rely on] +- **Budget / baseline / trend follow-up**: [none / describe] +- **Review-stop questions**: [lane fit / breadth / hidden cost / heavy-family risk / escalation] +- **Escalation path**: [none / document-in-feature / follow-up-spec / reject-or-split] +- **Active feature PR close-out entry**: [Guardrail / Exception / Smoke Coverage / N/A] +- **Why no dedicated follow-up spec is needed**: [Routine upkeep stays inside this feature unless recurring pain or structural lane changes justify a separate spec] + +## Project Structure + +### Documentation (this feature) + +```text +specs/[###-feature]/ +├── plan.md # This file (/speckit.plan command output) +├── research.md # Phase 0 output (/speckit.plan command) +├── data-model.md # Phase 1 output (/speckit.plan command) +├── quickstart.md # Phase 1 output (/speckit.plan command) +├── contracts/ # Phase 1 output (/speckit.plan command) +└── tasks.md # Phase 2 output (/speckit.tasks command - NOT created by /speckit.plan) +``` + +### Source Code (repository root) + + +```text +# [REMOVE IF UNUSED] Option 1: Single project (DEFAULT) +src/ +├── models/ +├── services/ +├── cli/ +└── lib/ + +tests/ +├── contract/ +├── integration/ +└── unit/ + +# [REMOVE IF UNUSED] Option 2: Web application (when "frontend" + "backend" detected) +backend/ +├── src/ +│ ├── models/ +│ ├── services/ +│ └── api/ +└── tests/ + +frontend/ +├── src/ +│ ├── components/ +│ ├── pages/ +│ └── services/ +└── tests/ + +# [REMOVE IF UNUSED] Option 3: Mobile + API (when "iOS/Android" detected) +api/ +└── [same as backend above] + +ios/ or android/ +└── [platform-specific structure: feature modules, UI flows, platform tests] +``` + +**Structure Decision**: [Document the selected structure and reference the real +directories captured above] + +## Complexity Tracking + +> **Fill when Constitution Check has violations that must be justified OR when BLOAT-001 is triggered by new persistence, abstractions, states, or semantic frameworks.** + +| Violation | Why Needed | Simpler Alternative Rejected Because | +|-----------|------------|-------------------------------------| +| [e.g., 4th project] | [current need] | [why 3 projects insufficient] | +| [e.g., Repository pattern] | [specific problem] | [why direct DB access insufficient] | + +## Proportionality Review + +> **Fill when the feature introduces a new enum/status family, DTO/presenter/envelope, persisted entity/table/artifact, interface/contract/registry/resolver, taxonomy/classification system, or cross-domain UI framework.** + +- **Current operator problem**: [What present-day workflow or risk requires this?] +- **Existing structure is insufficient because**: [Why the current code cannot serve safely or clearly] +- **Narrowest correct implementation**: [Why this shape is the smallest viable one] +- **Ownership cost created**: [Maintenance, testing, cognitive load, migration, or review burden] +- **Alternative intentionally rejected**: [Simpler option and why it failed] +- **Release truth**: [Current-release truth or future-release preparation] diff --git a/specs/339-provider-connection-scope-hardening/spec.md b/specs/339-provider-connection-scope-hardening/spec.md new file mode 100644 index 00000000..f9981153 --- /dev/null +++ b/specs/339-provider-connection-scope-hardening/spec.md @@ -0,0 +1,235 @@ +# Feature Specification: Spec 339 - Provider Connection Scope Hardening + +**Feature Branch**: `339-provider-connection-scope-hardening` +**Created**: 2026-05-31 +**Status**: Draft +**Input**: Candidate `provider-connection-scope-hardening` from `docs/product/spec-candidates.md` + user-provided draft + repo inspection of Provider Connection scope/authority seams. + +## Spec Candidate Check *(mandatory — SPEC-GATE-001)* + +- **Problem**: Provider Connections are credential-adjacent. Their scope and authorization authority must not come from hidden UI/session state (remembered environment) or Filament tenant fallbacks. +- **Today's failure**: + - Provider Connection create authorization can still fall back to remembered environment (`WorkspaceContext::lastEnvironmentId`) or Filament tenant (`Filament::getTenant()`), allowing “implicit target environment” when `environment_id` is missing. + - Workspace authority can be inferred via Filament tenant when workspace context is missing, which is unsafe for a credential-adjacent surface. + - Legacy query keys (`tenant`, `tenant_id`, `managed_environment_id`) still exist in the ecosystem and must not regain authority via accidental parsing. +- **User-visible improvement**: Operators can trust that Provider Connection list/view/create/edit/actions are scoped only by: + - explicit workspace context (required), plus + - explicit `environment_id` (filter/create), or + - record ownership (`ProviderConnection.workspace_id` + `ProviderConnection.managed_environment_id`) for view/edit/actions. +- **Smallest enterprise-capable version**: + - define and enforce an explicit authority-source contract (allowed vs forbidden), + - remove hidden fallbacks in `ProviderConnectionPolicy` and Provider Connection resource query scoping, + - add targeted tests proving 404 vs 403 semantics and that legacy query aliases cannot grant authority. +- **Explicit non-goals**: + - no provider-neutral target-scope refactor (Spec 281 family), + - no provider registry / OAuth redesign, + - no schema changes, + - no broad link/query cleanup beyond Provider Connections (defer to `canonical-link-query-cleanup` candidate). +- **Permanent complexity imported**: a small set of contract tests + tightened policy/resource guardrails; no new persisted truth or abstraction layer. +- **Why now**: Spec 338 tightened the workspace/environment scope contract. Provider Connections remain the main credential-adjacent hub; harden scope authority before adding more provider operations or onboarding variants. +- **Why not local**: UI-only fixes do not guarantee consistent authority across create/view/actions. This must be enforced at the policy/resource boundary and guarded by tests. +- **Approval class**: Core Enterprise (security / authorization hardening). +- **Red flags triggered**: credential-adjacent surface + authorization semantics + cross-surface context drift. **Defense**: narrow scope, reuse existing `WorkspaceContext` + capabilities, and forbid new abstraction/persistence. +- **Score**: Nutzen: 2 | Dringlichkeit: 2 | Scope: 1 | Komplexität: 1 | Produktnähe: 2 | Wiederverwendung: 2 | **Gesamt: 10/12** +- **Decision**: approve. + +## Summary + +Provider Connections sit at the provider boundary and are credential-adjacent. This spec hardens their authority model so that: + +- **Create** requires an explicit `environment_id` (no remembered-environment or Filament-tenant fallback). +- **View/Edit/Actions** derive scope from the `ProviderConnection` record (workspace + managed environment ownership). +- **List filtering** uses explicit `environment_id` only, validated against the current workspace and access scope. +- **Hidden context** (topbar remembered environment, Filament tenant context, legacy query aliases) must not grant authority. + +This is a narrow P1 security hardening slice following the Spec 338 contract work, with tests that make regressions obvious. + +## Spec Scope Fields *(mandatory)* + +- **Scope**: workspace (workspace-owned hub with optional environment filter) +- **Primary Routes (representative)**: + - `/admin/provider-connections` + - `/admin/provider-connections?environment_id=` + - `/admin/provider-connections/create?environment_id=` + - `/admin/provider-connections/{record}` (view) + - `/admin/provider-connections/{record}/edit` +- **Data Ownership**: `ProviderConnection` is workspace-owned and is linked to one `ManagedEnvironment` via `managed_environment_id`. No schema changes. +- **RBAC**: + - Non-member / out-of-workspace access: 404 (deny-as-not-found) + - Member but missing capability: 403 + - Capabilities used by this surface: `Capabilities::PROVIDER_VIEW`, `Capabilities::PROVIDER_MANAGE`, and existing dedicated-credential capability variants. + +## Authority / Scope Contract *(Spec 339)* + +### A. Allowed authority sources (canonical) + +Create: +- explicit `environment_id` query parameter (validated and workspace-owned) +- explicit environment selection in form state (only if a picker is introduced later) +- workspace context from `WorkspaceContext` (required) + +View/Edit/Actions: +- `ProviderConnection.workspace_id` + `ProviderConnection.managed_environment_id` +- record → tenant (`ManagedEnvironment`) → `workspace_id` (derived) +- workspace context from `WorkspaceContext` for deny-as-not-found semantics and list scoping + +### B. Forbidden authority sources (must never grant permission) + +- remembered environment (`WorkspaceContext::lastEnvironmentId` and related helpers) +- Filament tenant fallback (`Filament::getTenant()`) as a source of workspace/environment authority +- legacy query aliases (`tenant`, `tenant_id`, `managed_environment_id`) as authority inputs +- Filament internal state/query keys as authority (`tableFilters[...]`, etc.) + +### C. 404 vs 403 semantics (authorization UX contract) + +- Non-member / no workspace context / wrong workspace / out-of-scope record: **404** (deny-as-not-found) +- Member but missing capability: **403** + +## Required Runtime Decisions + +### D1 — Create requires explicit `environment_id` (no hidden fallback) + +Decision: +- Provider Connection create is only valid with explicit `environment_id` that belongs to the current workspace. +- Remembered environment and Filament tenant must never substitute for missing `environment_id`. + +Acceptance: +- `/admin/provider-connections/create` without `environment_id` cannot be used to create a connection. +- Wrong-workspace `environment_id` denies as 404. +- Member without `PROVIDER_MANAGE` denies as 403. + +### D2 — Workspace authority requires explicit workspace context (no Filament fallback) + +Decision: +- Workspace authority for Provider Connections must come from `WorkspaceContext` (session workspace), not from Filament tenant fallback. + +Acceptance: +- If workspace context is missing, Provider Connections deny as 404 even if Filament tenant is set. + +### D3 — Environment filtering is explicit and inert + +Decision: +- `environment_id` may filter list results but must be validated against current workspace and the user’s environment access scope. +- Legacy query keys do not grant access and are not treated as authority. + +Acceptance: +- `?managed_environment_id=...`, `?tenant=...`, `?tenant_id=...` do not grant access or widen results. + +## UI Surface Impact *(mandatory — UI-COV-001)* + +- [ ] No UI surface impact +- [x] Existing page changed +- [ ] New page/route added +- [ ] Navigation changed +- [ ] Filament panel/provider surface changed +- [ ] New modal/drawer/wizard/action added +- [ ] New table/form/state added +- [ ] Customer-facing surface changed +- [x] Dangerous action changed +- [ ] Status/evidence/review presentation changed +- [x] Workspace/environment context presentation changed + +## UI/Productization Coverage *(UI-COV-001)* + +- **Route/page/surface**: Provider Connections (list, view, create, edit) + credential-adjacent actions (dedicated credential management and provider operations) in `ProviderConnectionResource`. +- **Current page archetype**: workspace admin “Integration/Settings” hub resource (credential-adjacent). +- **Design depth**: Domain Pattern Surface (security/authority hardening; minimal UX change expected). +- **Repo-truth level**: repo-verified (`ProviderConnectionResource`, `ProviderConnectionPolicy`, `WorkspaceContext`, `WorkspaceHubEnvironmentFilter` + existing ProviderConnections feature tests). +- **Existing pattern reused**: Spec 338 scope contract + existing deny-as-not-found semantics, `WorkspaceContext`, capabilities, and existing ProviderConnections test families. +- **New pattern required**: none (tighten existing seams; avoid new abstraction/persistence). +- **Screenshot required**: no (unless implementation changes visible CTA/empty-state messaging; then add one targeted screenshot in the implementation PR). +- **Page audit required**: no (existing archetype). +- **Customer-safe review required**: no (operator-only surface). +- **Dangerous-action review required**: yes (credential-adjacent actions must remain confirmation/authorization/audit-safe and scoped to record ownership). +- **Coverage files to update (in implementation PR)**: + - [ ] `docs/ui-ux-enterprise-audit/route-inventory.md` (only if navigation entries/routes change; not expected) + - [ ] `docs/ui-ux-enterprise-audit/design-coverage-matrix.md` (no new surface; expected `no`) + - [x] `N/A - no new reachable UI surface added; authority hardening only` + +## Cross-Cutting / Shared Pattern Reuse *(mandatory)* + +- **Cross-cutting feature?**: yes +- **Interaction class(es)**: navigation + scope presentation, authorization semantics, credential-adjacent action gating. +- **Systems touched (expected)**: + - `apps/platform/app/Filament/Resources/ProviderConnectionResource.php` + - `apps/platform/app/Policies/ProviderConnectionPolicy.php` + - `apps/platform/app/Support/Workspaces/WorkspaceContext.php` + - `apps/platform/app/Support/Navigation/WorkspaceHubEnvironmentFilter.php` + - `apps/platform/tests/Feature/ProviderConnections/*` +- **Existing pattern(s) to extend**: Spec 338 scope/authority contract (explicit `environment_id` filter, remembered environment is navigation-only, deny-as-not-found for out-of-scope). +- **Allowed deviation and why**: none (tighten existing seams; no new surface frameworking). +- **Consistency impact**: authority sources must match across list/create/policy/actions; tests must enforce that legacy query aliases and hidden context do not widen authority. +- **Review focus**: “credential-adjacent authority comes from explicit input or record ownership only.” + +## OperationRun UX Impact + +- **Touches OperationRun start/completion/link UX?**: no +- **N/A**: This spec hardens authority sources and authorization semantics. Provider operations that already run via `OperationRun` must remain scoped to the record’s owning environment/workspace; no new `OperationRun` types or start UX changes are introduced. + +## Provider Boundary / Platform Core Check + +- **Shared provider/platform boundary touched?**: yes +- **Boundary classification**: platform-core authority (workspace/environment scope) + provider-owned identity (provider scope identifiers) must remain separated. +- **Seams affected**: Provider Connection authority sources (workspace/environment), policy decisions, and filter/query semantics. +- **Why this does not deepen provider coupling accidentally**: The contract uses neutral platform terms (`workspace`, `environment_id`, record ownership) and explicitly forbids conflating provider “tenant” identifiers with platform scope authority. +- **Follow-up path**: none (follow-up work belongs in `canonical-link-query-cleanup` and/or Spec 281 family, not in this slice). + +## Testing / Lane / Runtime Impact *(mandatory for runtime behavior changes)* + +- **Test purpose / classification**: Feature (authorization/scope contract). +- **Validation lane(s)**: fast-feedback (Feature). Browser smoke is optional and only justified if visible UX changes occur. +- **Why this classification and these lanes are sufficient**: The risk is scope/authorization drift, which is best proven by targeted policy/resource tests (positive + negative cases) without introducing heavy or browser-wide cost by default. +- **Planned validation commands**: + - `cd apps/platform && ./vendor/bin/sail artisan test --compact tests/Feature/ProviderConnections --filter=ScopeHardening` + - `cd apps/platform && ./vendor/bin/sail pint --dirty --format agent` + +## Acceptance Criteria + +- **AC1**: Provider Connection create requires explicit `environment_id`; remembered environment and Filament tenant cannot substitute. +- **AC2**: Provider Connection list/view/edit/actions derive scope only from current workspace context + explicit `environment_id` (filter/create) or from record ownership (view/edit/actions). +- **AC3**: Legacy query aliases (`tenant`, `tenant_id`, `managed_environment_id`) do not grant access or widen results. +- **AC4**: 404 vs 403 semantics match the repo rules: out-of-scope/non-member is 404; missing capability is 403. +- **AC5**: Targeted feature tests are green and make authority regressions obvious. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 — View Provider Connections safely (Priority: P1) + +As an operator in a workspace, I can list and view Provider Connections for environments I am entitled to, and I can optionally filter the list by an explicit environment. + +**Independent Test**: A user with workspace membership and `PROVIDER_VIEW` can list and view records in-scope; a non-member or wrong-workspace access is 404; a member without capability is 403; legacy query aliases do not widen access. + +**Acceptance Scenarios**: +1. **Given** a workspace member with `PROVIDER_VIEW` on Environment A, **When** they visit `/admin/provider-connections`, **Then** the list contains only connections in environments they can access in the current workspace. +2. **Given** the same user, **When** they visit `/admin/provider-connections?environment_id=`, **Then** the list is filtered to Environment A and remains scope-safe. +3. **Given** a user without workspace membership, **When** they hit Provider Connections routes, **Then** the response is deny-as-not-found (404). +4. **Given** a member without `PROVIDER_VIEW`, **When** they attempt to list/view, **Then** the response is 403. +5. **Given** any user, **When** they add `tenant`, `tenant_id`, or `managed_environment_id` query keys, **Then** those keys do not grant access or change authority behavior. + +### User Story 2 — Create Provider Connection only with explicit target environment (Priority: P1) + +As an operator, I can create a Provider Connection only when I explicitly choose the target environment (by passing `environment_id`) and I am authorized for that environment. + +**Independent Test**: Create is impossible without explicit `environment_id`, even if a remembered environment or Filament tenant exists. + +**Acceptance Scenarios**: +1. **Given** a member with `PROVIDER_MANAGE` on Environment A, **When** they open `/admin/provider-connections/create?environment_id=`, **Then** create is allowed and the record is owned by Environment A + the current workspace. +2. **Given** a request to `/admin/provider-connections/create` without `environment_id`, **When** the user has a remembered environment, **Then** create is still denied (no implicit fallback). +3. **Given** an `environment_id` from another workspace, **When** the user attempts create, **Then** the response is 404. +4. **Given** a member without `PROVIDER_MANAGE`, **When** they attempt create for an in-scope environment, **Then** the response is 403. + +### User Story 3 — Perform credential-adjacent actions with record-derived authority (Priority: P2) + +As an operator, credential-adjacent actions (dedicated credential management, provider operations) execute only when I am authorized for the record’s owning environment/workspace, independent of remembered environment or Filament tenant state. + +**Independent Test**: A record-bound action uses the record’s ownership and denies 404/403 correctly. + +**Acceptance Scenarios**: +1. **Given** a Provider Connection record owned by Environment A, **When** a user with access to Environment A triggers an action, **Then** the action is authorized using record ownership and capability checks. +2. **Given** the same record, **When** a user’s remembered environment is Environment B, **Then** authorization still uses record ownership (A), not hidden context. +3. **Given** a member without the dedicated credential capability, **When** they attempt dedicated credential actions, **Then** the response is 403. + +## Follow-up spec candidates + +- `canonical-link-query-cleanup` (broader link/query inventory + alias cleanup after scope contract) +- `environment-resource-context-follow-through` (reduce hidden Filament context reliance inside selected environment resources) diff --git a/specs/339-provider-connection-scope-hardening/tasks.md b/specs/339-provider-connection-scope-hardening/tasks.md new file mode 100644 index 00000000..f39b4265 --- /dev/null +++ b/specs/339-provider-connection-scope-hardening/tasks.md @@ -0,0 +1,114 @@ +# Tasks: Spec 339 - Provider Connection Scope Hardening + +- Input: `specs/339-provider-connection-scope-hardening/spec.md`, `specs/339-provider-connection-scope-hardening/plan.md` +- Preparation status: implementation-ready. + +**Tests**: Required. This spec hardens credential-adjacent scope/authorization semantics. + +## Test Governance Checklist + +- [x] Lane assignment is explicit and narrowest sufficient (Feature). +- [x] No new default-heavy helpers/factories/seeds are introduced. +- [x] Scope/authority changes are guarded by deterministic tests before refactors. +- [x] Any exception resolves as `document-in-feature`, `follow-up-spec`, or `reject-or-split`. + +## Phase 1: Preparation And Repo Truth (blocks runtime changes) + +**Purpose**: Confirm repo truth and identify the exact forbidden fallback seams before changing behavior. + +- [x] T001 Re-read `spec.md` + `plan.md` + this `tasks.md`. +- [x] T002 Confirm working tree intent and record baseline commit (`git status`, `git log -1`). +- [x] T003 Inspect the confirmed authority seams: + - `apps/platform/app/Policies/ProviderConnectionPolicy.php`: + - `currentWorkspace()` (forbidden fallback via `Filament::getTenant()`). + - `resolveCreateTenant()` (forbidden fallbacks via remembered environment + Filament tenant). + - record-derived checks for workspace/environment ownership (ensure these remain the canonical authority for view/edit/actions). + - `apps/platform/app/Filament/Resources/ProviderConnectionResource.php`: + - `applyMembershipScope()` (must not infer workspace via tenant context). + - `resolveRequestedEnvironment()` / `WorkspaceHubEnvironmentFilter` use (explicit `environment_id` only). + - `apps/platform/app/Support/Workspaces/WorkspaceContext.php`: + - `lastEnvironmentId()` and remembered-environment helpers (navigation-only; must not grant create authority). +- [x] T004 Inventory existing ProviderConnections tests and identify the best location for new regression tests: + - `apps/platform/tests/Feature/ProviderConnections/ProviderConnectionListAuthorizationTest.php` + - `apps/platform/tests/Feature/ProviderConnections/ProviderConnectionsWorkspaceHubContractTest.php` + - `apps/platform/tests/Feature/ProviderConnections/AuthorizationSemanticsTest.php` + - `apps/platform/tests/Feature/ProviderConnections/RequiredFiltersTest.php` + +## Phase 2: Add failing contract tests first + +**Purpose**: Make authority changes reviewable and regression-proof. + +- [x] T005 Add a new Spec 339 test proving create requires explicit `environment_id`: + - Create is denied when `/admin/provider-connections/create` is requested without `environment_id`, + - even if a remembered environment exists in session (`WorkspaceContext::lastEnvironmentId`). + - Implemented via: `apps/platform/tests/Feature/ProviderConnections/ScopeHardeningAuthoritySourcesTest.php` +- [x] T006 Add a test proving workspace authority does not fall back to Filament tenant: + - When workspace context is missing, Provider Connection access is deny-as-not-found (404), + - even if `Filament::getTenant()` is set. + - Implemented via: + - `apps/platform/tests/Feature/ProviderConnections/ScopeHardeningPolicyWorkspaceAuthorityTest.php` + - `apps/platform/tests/Feature/ProviderConnections/ScopeHardeningResourceWorkspaceAuthorityTest.php` +- [x] T007 Add a test proving legacy query aliases do not grant authority: + - `?managed_environment_id=...`, `?tenant=...`, `?tenant_id=...` must not widen list scope or unlock create. + - Implemented via: `apps/platform/tests/Feature/ProviderConnections/ScopeHardeningAuthoritySourcesTest.php` +- [x] T008 Add a test proving wrong-workspace environment IDs deny as 404: + - `environment_id` from another workspace must not authorize create or list filtering. + +## Phase 3: Policy authority hardening (Spec 339 contract) + +**Purpose**: Remove forbidden authority fallbacks and enforce the 404/403 semantics contract. + +- [x] T009 Update `apps/platform/app/Policies/ProviderConnectionPolicy.php::currentWorkspace()`: + - remove `Filament::getTenant()` as an authority source for workspace selection, + - keep workspace membership validation, + - keep deny-as-not-found (404) behavior when workspace context is missing. +- [x] T010 Update `apps/platform/app/Policies/ProviderConnectionPolicy.php::resolveCreateTenant()`: + - require explicit `environment_id`, + - remove remembered-environment fallback and Filament-tenant fallback, + - ensure wrong-workspace `environment_id` denies as 404. +- [x] T011 Confirm 404 vs 403 semantics for all policy methods: + - non-member / out-of-scope record → 404, + - member missing capability → 403. + +## Phase 4: Resource scoping hardening (no hidden context authority) + +**Purpose**: Ensure list/query and record surfaces do not infer authority from hidden context. + +- [x] T012 Update `apps/platform/app/Filament/Resources/ProviderConnectionResource.php::applyMembershipScope()`: + - remove fallback that infers workspace ID from tenant context when session workspace is missing, + - keep query empty (no results) when workspace context is missing. +- [x] T013 Confirm Provider Connections list filtering uses explicit `environment_id` only: + - validate `environment_id` against current workspace and access scope, + - ignore legacy query aliases as authority. +- [x] T014 Confirm record pages and action dispatchers use record-derived tenant/workspace authority, not remembered environment or Filament tenant. + +## Phase 5: Credential-adjacent actions audit + +**Purpose**: Ensure high-risk actions remain safe and record-scoped. + +- [x] T015 Audit all actions on `ProviderConnectionResource` (edit + dedicated credential actions + provider operations): + - confirm policy methods are used consistently, + - confirm destructive-like actions retain `->requiresConfirmation()`, + - confirm record ownership controls scope for all actions. +- [x] T016 Confirm audit posture remains intact for credential-adjacent mutations: + - no new audit event families are introduced in this slice, + - existing audit logging remains correct and record-scoped. + +## Phase 6: Optional UX clarification (only if needed) + +- [x] T017 If create is denied without `environment_id`, ensure the list empty state or guidance makes the required next step obvious (filter/select an environment first) without introducing a new picker or redesign. (No changes needed.) + +## Phase 7: Validation + +- [x] T018 Run narrow tests first: + - `cd apps/platform && ./vendor/bin/sail artisan test --compact tests/Feature/ProviderConnections --filter=ScopeHardening` +- [x] T019 Run formatting and patch checks: + - `cd apps/platform && ./vendor/bin/sail pint --dirty --format agent` + - `git diff --check` + +## Explicit Non-Goals + +- [x] NT001 Do not add migrations, new tables, or new persisted truth. +- [x] NT002 Do not redesign Provider Connections UX or navigation placement. +- [x] NT003 Do not introduce a new scope/authority abstraction framework. +- [x] NT004 Do not reopen provider-neutral target-scope/identity refactors (Spec 281 family).