diff --git a/.github/agents/copilot-instructions.md b/.github/agents/copilot-instructions.md index f1ecfec..176f98f 100644 --- a/.github/agents/copilot-instructions.md +++ b/.github/agents/copilot-instructions.md @@ -12,6 +12,8 @@ ## Active Technologies - PostgreSQL (JSONB for `InventoryItem.meta_jsonb`) (feat/047-inventory-foundations-nodes) - PostgreSQL (JSONB in `operation_runs.context`, `operation_runs.summary_counts`) (056-remove-legacy-bulkops) - PHP 8.4.15 (Laravel 12.47.0) + Filament v5.0.0, Livewire v4.0.1 (058-tenant-ui-polish) +- PHP 8.4 (per repo guidelines) + Laravel 12, Filament v5, Livewire v4 (067-rbac-troubleshooting) +- PostgreSQL (via Laravel Sail) (067-rbac-troubleshooting) - PHP 8.4.15 (feat/005-bulk-operations) @@ -31,9 +33,9 @@ ## Code Style PHP 8.4.15: Follow standard conventions ## Recent Changes +- 067-rbac-troubleshooting: Added PHP 8.4 (per repo guidelines) + Laravel 12, Filament v5, Livewire v4 - 058-tenant-ui-polish: Added PHP 8.4.15 (Laravel 12.47.0) + Filament v5.0.0, Livewire v4.0.1 - 058-tenant-ui-polish: Added [if applicable, e.g., PostgreSQL, CoreData, files or N/A] -- 056-remove-legacy-bulkops: Added PHP 8.4.x + Laravel 12, Filament v4, Livewire v3 diff --git a/app/Filament/Pages/TenantDiagnostics.php b/app/Filament/Pages/TenantDiagnostics.php new file mode 100644 index 0000000..7162f82 --- /dev/null +++ b/app/Filament/Pages/TenantDiagnostics.php @@ -0,0 +1,108 @@ +getKey(); + + $this->missingOwner = ! TenantMembership::query() + ->where('tenant_id', $tenantId) + ->where('role', 'owner') + ->exists(); + + $user = auth()->user(); + if (! $user instanceof User) { + abort(403, 'Not allowed'); + } + + $this->hasDuplicateMembershipsForCurrentUser = app(TenantDiagnosticsService::class) + ->userHasDuplicateMemberships($tenant, $user); + } + + /** + * @return array + */ + protected function getHeaderActions(): array + { + return [ + UiEnforcement::forAction( + Action::make('bootstrapOwner') + ->label('Bootstrap owner') + ->requiresConfirmation() + ->action(fn () => $this->bootstrapOwner()), + ) + ->requireCapability(Capabilities::TENANT_MANAGE) + ->destructive() + ->tooltip(UiTooltips::INSUFFICIENT_PERMISSION) + ->apply() + ->visible(fn (): bool => $this->missingOwner), + + UiEnforcement::forAction( + Action::make('mergeDuplicateMemberships') + ->label('Merge duplicate memberships') + ->requiresConfirmation() + ->action(fn () => $this->mergeDuplicateMemberships()), + ) + ->requireCapability(Capabilities::TENANT_MANAGE) + ->destructive() + ->tooltip(UiTooltips::INSUFFICIENT_PERMISSION) + ->apply() + ->visible(fn (): bool => $this->hasDuplicateMembershipsForCurrentUser), + ]; + } + + public function bootstrapOwner(): void + { + $tenant = Tenant::current(); + + $user = auth()->user(); + if (! $user instanceof User) { + abort(403, 'Not allowed'); + } + + app(TenantMembershipManager::class)->bootstrapRecover($tenant, $user, $user); + + $this->mount(); + } + + public function mergeDuplicateMemberships(): void + { + $tenant = Tenant::current(); + + $user = auth()->user(); + if (! $user instanceof User) { + abort(403, 'Not allowed'); + } + + app(TenantDiagnosticsService::class)->mergeDuplicateMembershipsForUser($tenant, $user, $user); + + $this->mount(); + } +} diff --git a/app/Filament/Resources/ProviderConnectionResource/Pages/EditProviderConnection.php b/app/Filament/Resources/ProviderConnectionResource/Pages/EditProviderConnection.php index ef44984..3b97d2c 100644 --- a/app/Filament/Resources/ProviderConnectionResource/Pages/EditProviderConnection.php +++ b/app/Filament/Resources/ProviderConnectionResource/Pages/EditProviderConnection.php @@ -163,7 +163,7 @@ protected function getHeaderActions(): array $user = auth()->user(); return $tenant instanceof Tenant - && $user instanceof User + && $user instanceof User && $user->canAccessTenant($tenant) && $record->status !== 'disabled'; }) @@ -242,9 +242,8 @@ protected function getHeaderActions(): array ->send(); }) ) - ->requireCapability(Capabilities::PROVIDER_RUN) - ->tooltip('You do not have permission to run provider operations.') ->preserveVisibility() + ->requireCapability(Capabilities::PROVIDER_RUN) ->apply(), UiEnforcement::forAction( diff --git a/app/Filament/Resources/TenantResource.php b/app/Filament/Resources/TenantResource.php index 48a2e68..d52f5a6 100644 --- a/app/Filament/Resources/TenantResource.php +++ b/app/Filament/Resources/TenantResource.php @@ -21,6 +21,7 @@ use App\Services\OperationRunService; use App\Services\Operations\BulkSelectionIdentity; use App\Support\Auth\Capabilities; +use App\Support\Auth\UiTooltips; use App\Support\Badges\BadgeDomain; use App\Support\Badges\BadgeRenderer; use App\Support\Badges\TagBadgeDomain; @@ -28,6 +29,7 @@ use App\Support\OperationRunLinks; use App\Support\OpsUx\OperationUxPresenter; use App\Support\OpsUx\OpsUxBrowserEvents; +use App\Support\Rbac\UiEnforcement; use BackedEnum; use Filament\Actions; use Filament\Actions\ActionGroup; @@ -262,95 +264,57 @@ public static function table(Table $table): Table ->label('View') ->icon('heroicon-o-eye') ->url(fn (Tenant $record) => static::getUrl('view', ['record' => $record], tenant: $record)), - Actions\Action::make('syncTenant') - ->label('Sync') - ->icon('heroicon-o-arrow-path') - ->color('warning') - ->requiresConfirmation() - ->visible(function (Tenant $record): bool { - if (! $record->isActive()) { - return false; - } + UiEnforcement::forAction( + Actions\Action::make('syncTenant') + ->label('Sync') + ->icon('heroicon-o-arrow-path') + ->color('warning') + ->requiresConfirmation() + ->visible(function (Tenant $record): bool { + if (! $record->isActive()) { + return false; + } - $user = auth()->user(); + $user = auth()->user(); - if (! $user instanceof User) { - return false; - } + if (! $user instanceof User) { + return false; + } - return $user->canAccessTenant($record); - }) - ->disabled(function (Tenant $record): bool { - $user = auth()->user(); + return $user->canAccessTenant($record); + }) + ->action(function (Tenant $record, AuditLogger $auditLogger, \Filament\Tables\Contracts\HasTable $livewire): void { + $user = auth()->user(); - if (! $user instanceof User) { - return true; - } + if (! $user instanceof User) { + abort(403); + } - /** @var CapabilityResolver $resolver */ - $resolver = app(CapabilityResolver::class); + if (! $user->canAccessTenant($record)) { + abort(404); + } - return ! $resolver->can($user, $record, Capabilities::TENANT_SYNC); - }) - ->tooltip(function (Tenant $record): ?string { - $user = auth()->user(); + /** @var CapabilityResolver $resolver */ + $resolver = app(CapabilityResolver::class); - if (! $user instanceof User) { - return null; - } + if (! $resolver->can($user, $record, Capabilities::TENANT_SYNC)) { + abort(403); + } - /** @var CapabilityResolver $resolver */ - $resolver = app(CapabilityResolver::class); + /** @var OperationRunService $opService */ + $opService = app(OperationRunService::class); - return $resolver->can($user, $record, Capabilities::TENANT_SYNC) - ? null - : 'You do not have permission to sync this tenant.'; - }) - ->action(function (Tenant $record, AuditLogger $auditLogger, \Filament\Tables\Contracts\HasTable $livewire): void { - $user = auth()->user(); - - if (! $user instanceof User) { - abort(403); - } - - if (! $user->canAccessTenant($record)) { - abort(404); - } - - /** @var CapabilityResolver $resolver */ - $resolver = app(CapabilityResolver::class); - - if (! $resolver->can($user, $record, Capabilities::TENANT_SYNC)) { - abort(403); - } - - /** @var OperationRunService $opService */ - $opService = app(OperationRunService::class); - - $supportedTypes = config('tenantpilot.supported_policy_types', []); - $typeNames = array_map( - static fn (array $typeConfig): string => (string) $typeConfig['type'], - $supportedTypes, - ); - sort($typeNames); - - $inputs = [ - 'scope' => 'full', - 'types' => $typeNames, - ]; - - $opRun = $opService->ensureRun( - tenant: $record, - type: 'policy.sync', - inputs: $inputs, - initiator: auth()->user() - ); - - if (! $opRun->wasRecentlyCreated && $opService->isStaleQueuedRun($opRun)) { - $opService->failStaleQueuedRun( - $opRun, - message: 'Run was queued but never started (likely a previous dispatch error). Re-queuing.' + $supportedTypes = config('tenantpilot.supported_policy_types', []); + $typeNames = array_map( + static fn (array $typeConfig): string => (string) $typeConfig['type'], + $supportedTypes, ); + sort($typeNames); + + $inputs = [ + 'scope' => 'full', + 'types' => $typeNames, + ]; $opRun = $opService->ensureRun( tenant: $record, @@ -358,295 +322,262 @@ public static function table(Table $table): Table inputs: $inputs, initiator: auth()->user() ); - } - if (! $opRun->wasRecentlyCreated && in_array($opRun->status, ['queued', 'running'], true)) { - Notification::make() - ->title('Policy sync already active') - ->body('This operation is already queued or running.') - ->warning() + if (! $opRun->wasRecentlyCreated && $opService->isStaleQueuedRun($opRun)) { + $opService->failStaleQueuedRun( + $opRun, + message: 'Run was queued but never started (likely a previous dispatch error). Re-queuing.' + ); + + $opRun = $opService->ensureRun( + tenant: $record, + type: 'policy.sync', + inputs: $inputs, + initiator: auth()->user() + ); + } + + if (! $opRun->wasRecentlyCreated && in_array($opRun->status, ['queued', 'running'], true)) { + Notification::make() + ->title('Policy sync already active') + ->body('This operation is already queued or running.') + ->warning() + ->actions([ + Actions\Action::make('view_run') + ->label('View Run') + ->url(OperationRunLinks::view($opRun, $record)), + ]) + ->send(); + + return; + } + + $opService->dispatchOrFail($opRun, function () use ($record, $supportedTypes, $opRun): void { + SyncPoliciesJob::dispatch((int) $record->getKey(), $supportedTypes, null, $opRun); + }); + + $auditLogger->log( + tenant: $record, + action: 'tenant.sync_dispatched', + resourceType: 'tenant', + resourceId: (string) $record->id, + status: 'success', + context: ['metadata' => ['tenant_id' => $record->tenant_id]], + ); + + OpsUxBrowserEvents::dispatchRunEnqueued($livewire); + OperationUxPresenter::queuedToast((string) $opRun->type) ->actions([ Actions\Action::make('view_run') ->label('View Run') ->url(OperationRunLinks::view($opRun, $record)), ]) ->send(); - - return; - } - - $opService->dispatchOrFail($opRun, function () use ($record, $supportedTypes, $opRun): void { - SyncPoliciesJob::dispatch((int) $record->getKey(), $supportedTypes, null, $opRun); - }); - - $auditLogger->log( - tenant: $record, - action: 'tenant.sync_dispatched', - resourceType: 'tenant', - resourceId: (string) $record->id, - status: 'success', - context: ['metadata' => ['tenant_id' => $record->tenant_id]], - ); - - OpsUxBrowserEvents::dispatchRunEnqueued($livewire); - OperationUxPresenter::queuedToast((string) $opRun->type) - ->actions([ - Actions\Action::make('view_run') - ->label('View Run') - ->url(OperationRunLinks::view($opRun, $record)), - ]) - ->send(); - }), + }) + ) + ->preserveVisibility() + ->requireCapability(Capabilities::TENANT_SYNC) + ->apply(), Actions\Action::make('openTenant') ->label('Open') ->icon('heroicon-o-arrow-right') ->color('primary') ->url(fn (Tenant $record) => \App\Filament\Resources\PolicyResource::getUrl('index', tenant: $record)) ->visible(fn (Tenant $record) => $record->isActive()), - Actions\Action::make('edit') - ->label('Edit') - ->icon('heroicon-o-pencil-square') - ->url(fn (Tenant $record) => static::getUrl('edit', ['record' => $record], tenant: $record)) - ->disabled(fn (Tenant $record): bool => ! static::canEdit($record)) - ->tooltip(fn (Tenant $record): ?string => static::canEdit($record) ? null : 'You do not have permission to edit this tenant.'), - Actions\Action::make('restore') - ->label('Restore') - ->color('success') - ->successNotificationTitle('Tenant reactivated') - ->requiresConfirmation() - ->visible(fn (Tenant $record): bool => $record->trashed()) - ->disabled(function (Tenant $record): bool { - $user = auth()->user(); + UiEnforcement::forAction( + Actions\Action::make('edit') + ->label('Edit') + ->icon('heroicon-o-pencil-square') + ->url(fn (Tenant $record) => static::getUrl('edit', ['record' => $record], tenant: $record)) + ) + ->requireCapability(Capabilities::TENANT_MANAGE) + ->apply(), + UiEnforcement::forAction( + Actions\Action::make('restore') + ->label('Restore') + ->color('success') + ->icon('heroicon-o-arrow-uturn-left') + ->successNotificationTitle('Tenant reactivated') + ->requiresConfirmation() + ->visible(fn (Tenant $record): bool => $record->trashed()) + ->action(function (Tenant $record, AuditLogger $auditLogger): void { + $user = auth()->user(); - if (! $user instanceof User) { - return true; - } + if (! $user instanceof User) { + abort(403); + } - /** @var CapabilityResolver $resolver */ - $resolver = app(CapabilityResolver::class); + /** @var CapabilityResolver $resolver */ + $resolver = app(CapabilityResolver::class); - return ! $resolver->can($user, $record, Capabilities::TENANT_DELETE); - }) - ->action(function (Tenant $record, AuditLogger $auditLogger): void { - $user = auth()->user(); + if (! $resolver->can($user, $record, Capabilities::TENANT_DELETE)) { + abort(403); + } - if (! $user instanceof User) { - abort(403); - } + $record->restore(); - /** @var CapabilityResolver $resolver */ - $resolver = app(CapabilityResolver::class); - - if (! $resolver->can($user, $record, Capabilities::TENANT_DELETE)) { - abort(403); - } - - $record->restore(); - - $auditLogger->log( - tenant: $record, - action: 'tenant.restored', - resourceType: 'tenant', - resourceId: (string) $record->id, - status: 'success', - context: ['metadata' => ['tenant_id' => $record->tenant_id]] - ); - }), - Actions\Action::make('admin_consent') - ->label('Admin consent') - ->icon('heroicon-o-clipboard-document') - ->url(fn (Tenant $record) => static::adminConsentUrl($record)) - ->visible(fn (Tenant $record) => static::adminConsentUrl($record) !== null) - ->disabled(function (Tenant $record): bool { - $user = auth()->user(); - - if (! $user instanceof User) { - return true; - } - - /** @var CapabilityResolver $resolver */ - $resolver = app(CapabilityResolver::class); - - return ! $resolver->can($user, $record, Capabilities::TENANT_MANAGE); - }) - ->tooltip(function (Tenant $record): ?string { - $user = auth()->user(); - - if (! $user instanceof User) { - return null; - } - - /** @var CapabilityResolver $resolver */ - $resolver = app(CapabilityResolver::class); - - return $resolver->can($user, $record, Capabilities::TENANT_MANAGE) - ? null - : 'You do not have permission to manage tenant consent.'; - }) - ->openUrlInNewTab(), + $auditLogger->log( + tenant: $record, + action: 'tenant.restored', + resourceType: 'tenant', + resourceId: (string) $record->id, + status: 'success', + context: ['metadata' => ['tenant_id' => $record->tenant_id]] + ); + }) + ) + ->preserveVisibility() + ->requireCapability(Capabilities::TENANT_DELETE) + ->apply(), + UiEnforcement::forAction( + Actions\Action::make('admin_consent') + ->label('Admin consent') + ->icon('heroicon-o-clipboard-document') + ->url(fn (Tenant $record) => static::adminConsentUrl($record)) + ->visible(fn (Tenant $record) => static::adminConsentUrl($record) !== null) + ->openUrlInNewTab(), + ) + ->preserveVisibility() + ->requireCapability(Capabilities::TENANT_MANAGE) + ->apply(), Actions\Action::make('open_in_entra') ->label('Open in Entra') ->icon('heroicon-o-arrow-top-right-on-square') ->url(fn (Tenant $record) => static::entraUrl($record)) ->visible(fn (Tenant $record) => static::entraUrl($record) !== null) ->openUrlInNewTab(), - Actions\Action::make('verify') - ->label('Verify configuration') - ->icon('heroicon-o-check-badge') - ->color('primary') - ->requiresConfirmation() - ->visible(fn (Tenant $record): bool => $record->isActive()) - ->disabled(function (Tenant $record): bool { - $user = auth()->user(); + UiEnforcement::forAction( + Actions\Action::make('verify') + ->label('Verify configuration') + ->icon('heroicon-o-check-badge') + ->color('primary') + ->requiresConfirmation() + ->visible(fn (Tenant $record): bool => $record->isActive()) + ->action(function ( + Tenant $record, + TenantConfigService $configService, + TenantPermissionService $permissionService, + RbacHealthService $rbacHealthService, + AuditLogger $auditLogger + ): void { + $user = auth()->user(); - if (! $user instanceof User) { - return true; - } + if (! $user instanceof User) { + abort(403); + } - /** @var CapabilityResolver $resolver */ - $resolver = app(CapabilityResolver::class); + /** @var CapabilityResolver $resolver */ + $resolver = app(CapabilityResolver::class); - return ! $resolver->can($user, $record, Capabilities::TENANT_MANAGE); - }) - ->action(function ( - Tenant $record, - TenantConfigService $configService, - TenantPermissionService $permissionService, - RbacHealthService $rbacHealthService, - AuditLogger $auditLogger - ) { - $user = auth()->user(); + if (! $resolver->can($user, $record, Capabilities::TENANT_MANAGE)) { + abort(403); + } - if (! $user instanceof User) { - abort(403); - } - - /** @var CapabilityResolver $resolver */ - $resolver = app(CapabilityResolver::class); - - if (! $resolver->can($user, $record, Capabilities::TENANT_MANAGE)) { - abort(403); - } - - static::verifyTenant($record, $configService, $permissionService, $rbacHealthService, $auditLogger); - }), + static::verifyTenant($record, $configService, $permissionService, $rbacHealthService, $auditLogger); + }), + ) + ->preserveVisibility() + ->requireCapability(Capabilities::TENANT_MANAGE) + ->apply(), static::rbacAction(), - Actions\Action::make('archive') - ->label('Deactivate') - ->color('danger') - ->icon('heroicon-o-archive-box-x-mark') - ->requiresConfirmation() - ->visible(fn (Tenant $record): bool => ! $record->trashed()) - ->disabled(function (Tenant $record): bool { - $user = auth()->user(); + UiEnforcement::forAction( + Actions\Action::make('archive') + ->label('Deactivate') + ->color('danger') + ->icon('heroicon-o-archive-box-x-mark') + ->requiresConfirmation() + ->visible(fn (Tenant $record): bool => ! $record->trashed()) + ->action(function (Tenant $record, AuditLogger $auditLogger): void { + $user = auth()->user(); - if (! $user instanceof User) { - return true; - } + if (! $user instanceof User) { + abort(403); + } - /** @var CapabilityResolver $resolver */ - $resolver = app(CapabilityResolver::class); + /** @var CapabilityResolver $resolver */ + $resolver = app(CapabilityResolver::class); - return ! $resolver->can($user, $record, Capabilities::TENANT_DELETE); - }) - ->action(function (Tenant $record, AuditLogger $auditLogger) { - $user = auth()->user(); + if (! $resolver->can($user, $record, Capabilities::TENANT_DELETE)) { + abort(403); + } - if (! $user instanceof User) { - abort(403); - } + $record->delete(); - /** @var CapabilityResolver $resolver */ - $resolver = app(CapabilityResolver::class); + $auditLogger->log( + tenant: $record, + action: 'tenant.archived', + resourceType: 'tenant', + resourceId: (string) $record->id, + status: 'success', + context: ['metadata' => ['tenant_id' => $record->tenant_id]] + ); - if (! $resolver->can($user, $record, Capabilities::TENANT_DELETE)) { - abort(403); - } - - $record->delete(); - - $auditLogger->log( - tenant: $record, - action: 'tenant.archived', - resourceType: 'tenant', - resourceId: (string) $record->id, - status: 'success', - context: ['metadata' => ['tenant_id' => $record->tenant_id]] - ); - - Notification::make() - ->title('Tenant deactivated') - ->body('The tenant has been archived and hidden from lists.') - ->success() - ->send(); - }), - Actions\Action::make('forceDelete') - ->label('Force delete') - ->color('danger') - ->icon('heroicon-o-trash') - ->requiresConfirmation() - ->visible(fn (?Tenant $record): bool => (bool) $record?->trashed()) - ->disabled(function (?Tenant $record): bool { - if (! $record instanceof Tenant) { - return true; - } - - $user = auth()->user(); - - if (! $user instanceof User) { - return true; - } - - /** @var CapabilityResolver $resolver */ - $resolver = app(CapabilityResolver::class); - - return ! $resolver->can($user, $record, Capabilities::TENANT_DELETE); - }) - ->action(function (?Tenant $record, AuditLogger $auditLogger) { - if ($record === null) { - return; - } - - $user = auth()->user(); - - if (! $user instanceof User) { - abort(403); - } - - /** @var CapabilityResolver $resolver */ - $resolver = app(CapabilityResolver::class); - - if (! $resolver->can($user, $record, Capabilities::TENANT_DELETE)) { - abort(403); - } - - $tenant = Tenant::withTrashed()->find($record->id); - - if (! $tenant?->trashed()) { Notification::make() - ->title('Tenant must be archived first') - ->danger() + ->title('Tenant deactivated') + ->body('The tenant has been archived and hidden from lists.') + ->success() ->send(); + }), + ) + ->preserveVisibility() + ->requireCapability(Capabilities::TENANT_DELETE) + ->apply(), + UiEnforcement::forAction( + Actions\Action::make('forceDelete') + ->label('Force delete') + ->color('danger') + ->icon('heroicon-o-trash') + ->requiresConfirmation() + ->visible(fn (?Tenant $record): bool => (bool) $record?->trashed()) + ->action(function (?Tenant $record, AuditLogger $auditLogger): void { + if ($record === null) { + return; + } - return; - } + $user = auth()->user(); - $auditLogger->log( - tenant: $tenant, - action: 'tenant.force_deleted', - resourceType: 'tenant', - resourceId: (string) $tenant->id, - status: 'success', - context: ['metadata' => ['tenant_id' => $tenant->tenant_id]] - ); + if (! $user instanceof User) { + abort(403); + } - $tenant->forceDelete(); + /** @var CapabilityResolver $resolver */ + $resolver = app(CapabilityResolver::class); - Notification::make() - ->title('Tenant permanently deleted') - ->success() - ->send(); - }), + if (! $resolver->can($user, $record, Capabilities::TENANT_DELETE)) { + abort(403); + } + + $tenant = Tenant::withTrashed()->find($record->id); + + if (! $tenant?->trashed()) { + Notification::make() + ->title('Tenant must be archived first') + ->danger() + ->send(); + + return; + } + + $auditLogger->log( + tenant: $tenant, + action: 'tenant.force_deleted', + resourceType: 'tenant', + resourceId: (string) $tenant->id, + status: 'success', + context: ['metadata' => ['tenant_id' => $tenant->tenant_id]] + ); + + $tenant->forceDelete(); + + Notification::make() + ->title('Tenant permanently deleted') + ->success() + ->send(); + }), + ) + ->preserveVisibility() + ->requireCapability(Capabilities::TENANT_DELETE) + ->apply(), ]), ]) ->bulkActions([ @@ -655,27 +586,45 @@ public static function table(Table $table): Table ->icon('heroicon-o-arrow-path') ->color('warning') ->requiresConfirmation() - ->visible(function (): bool { + ->visible(fn (): bool => auth()->user() instanceof User) + ->authorize(fn (): bool => auth()->user() instanceof User) + ->disabled(function (Collection $records): bool { $user = auth()->user(); if (! $user instanceof User) { - return false; + return true; } - return $user->tenants() - ->whereIn('role', RoleCapabilityMap::rolesWithCapability(Capabilities::TENANT_SYNC)) - ->exists(); + if ($records->isEmpty()) { + return true; + } + + /** @var CapabilityResolver $resolver */ + $resolver = app(CapabilityResolver::class); + + return $records + ->filter(fn ($record) => $record instanceof Tenant) + ->contains(fn (Tenant $tenant): bool => ! $resolver->can($user, $tenant, Capabilities::TENANT_SYNC)); }) - ->authorize(function (): bool { + ->tooltip(function (Collection $records): ?string { $user = auth()->user(); if (! $user instanceof User) { - return false; + return UiTooltips::insufficientPermission(); } - return $user->tenants() - ->whereIn('role', RoleCapabilityMap::rolesWithCapability(Capabilities::TENANT_SYNC)) - ->exists(); + if ($records->isEmpty()) { + return null; + } + + /** @var CapabilityResolver $resolver */ + $resolver = app(CapabilityResolver::class); + + $isDenied = $records + ->filter(fn ($record) => $record instanceof Tenant) + ->contains(fn (Tenant $tenant): bool => ! $resolver->can($user, $tenant, Capabilities::TENANT_SYNC)); + + return $isDenied ? UiTooltips::insufficientPermission() : null; }) ->action(function (Collection $records, AuditLogger $auditLogger): void { $user = auth()->user(); @@ -982,9 +931,7 @@ public static function rbacAction(): Actions\Action return; } - $actor = auth()->user(); - - $result = $service->run($record, $data, $actor, $token); + $result = $service->run($record, $data, $user, $token); Cache::forget($cacheKey); diff --git a/app/Filament/Resources/TenantResource/Pages/ViewTenant.php b/app/Filament/Resources/TenantResource/Pages/ViewTenant.php index d65f9d9..f92b500 100644 --- a/app/Filament/Resources/TenantResource/Pages/ViewTenant.php +++ b/app/Filament/Resources/TenantResource/Pages/ViewTenant.php @@ -3,11 +3,14 @@ namespace App\Filament\Resources\TenantResource\Pages; use App\Filament\Resources\TenantResource; +use App\Filament\Widgets\Tenant\TenantArchivedBanner; use App\Models\Tenant; use App\Services\Intune\AuditLogger; use App\Services\Intune\RbacHealthService; use App\Services\Intune\TenantConfigService; use App\Services\Intune\TenantPermissionService; +use App\Support\Auth\Capabilities; +use App\Support\Rbac\UiEnforcement; use Filament\Actions; use Filament\Notifications\Notification; use Filament\Resources\Pages\ViewRecord; @@ -16,11 +19,25 @@ class ViewTenant extends ViewRecord { protected static string $resource = TenantResource::class; + protected function getHeaderWidgets(): array + { + return [ + TenantArchivedBanner::class, + ]; + } + protected function getHeaderActions(): array { return [ Actions\ActionGroup::make([ - Actions\EditAction::make(), + UiEnforcement::forAction( + Actions\Action::make('edit') + ->label('Edit') + ->icon('heroicon-o-pencil-square') + ->url(fn (Tenant $record): string => TenantResource::getUrl('edit', ['record' => $record])) + ) + ->requireCapability(Capabilities::TENANT_MANAGE) + ->apply(), Actions\Action::make('admin_consent') ->label('Admin consent') ->icon('heroicon-o-clipboard-document') @@ -48,30 +65,40 @@ protected function getHeaderActions(): array TenantResource::verifyTenant($record, $configService, $permissionService, $rbacHealthService, $auditLogger); }), TenantResource::rbacAction(), - Actions\Action::make('archive') - ->label('Deactivate') - ->color('danger') - ->icon('heroicon-o-archive-box-x-mark') - ->requiresConfirmation() - ->visible(fn (Tenant $record) => ! $record->trashed()) - ->action(function (Tenant $record, AuditLogger $auditLogger) { - $record->delete(); + UiEnforcement::forAction( + Actions\Action::make('archive') + ->label('Deactivate') + ->color('danger') + ->icon('heroicon-o-archive-box-x-mark') + ->visible(fn (Tenant $record): bool => ! $record->trashed()) + ->action(function (Tenant $record, AuditLogger $auditLogger): void { + $record->delete(); - $auditLogger->log( - tenant: $record, - action: 'tenant.archived', - resourceType: 'tenant', - resourceId: (string) $record->id, - status: 'success', - context: ['metadata' => ['tenant_id' => $record->tenant_id]] - ); + $auditLogger->log( + tenant: $record, + action: 'tenant.archived', + resourceType: 'tenant', + resourceId: (string) $record->getKey(), + status: 'success', + context: [ + 'metadata' => [ + 'internal_tenant_id' => (int) $record->getKey(), + 'tenant_guid' => (string) $record->tenant_id, + ], + ] + ); - Notification::make() - ->title('Tenant deactivated') - ->body('The tenant has been archived and hidden from lists.') - ->success() - ->send(); - }), + Notification::make() + ->title('Tenant deactivated') + ->body('The tenant has been archived and hidden from lists.') + ->success() + ->send(); + }) + ) + ->preserveVisibility() + ->requireCapability(Capabilities::TENANT_DELETE) + ->destructive() + ->apply(), ]) ->label('Actions') ->icon('heroicon-o-ellipsis-vertical') diff --git a/app/Filament/Widgets/Tenant/TenantArchivedBanner.php b/app/Filament/Widgets/Tenant/TenantArchivedBanner.php new file mode 100644 index 0000000..ea73194 --- /dev/null +++ b/app/Filament/Widgets/Tenant/TenantArchivedBanner.php @@ -0,0 +1,28 @@ + + */ + protected function getViewData(): array + { + $tenant = Filament::getTenant(); + + return [ + 'tenant' => $tenant instanceof Tenant ? $tenant : null, + ]; + } +} diff --git a/app/Models/Tenant.php b/app/Models/Tenant.php index 172eee9..c1d98b6 100644 --- a/app/Models/Tenant.php +++ b/app/Models/Tenant.php @@ -152,6 +152,19 @@ public static function current(): self return $tenant; } + public function resolveRouteBinding($value, $field = null): ?Model + { + $field ??= $this->getRouteKeyName(); + + $query = static::query(); + + if ($field === 'external_id') { + $query = $query->withTrashed(); + } + + return $query->where($field, $value)->first(); + } + public function memberships(): HasMany { return $this->hasMany(TenantMembership::class); diff --git a/app/Models/User.php b/app/Models/User.php index 214ca75..a60c6f3 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -130,8 +130,8 @@ public function canAccessTenant(Model $tenant): bool return false; } - return $this->tenants() - ->whereKey($tenant->getKey()) + return $this->tenantMemberships() + ->where('tenant_id', $tenant->getKey()) ->exists(); } diff --git a/app/Services/Auth/TenantDiagnosticsService.php b/app/Services/Auth/TenantDiagnosticsService.php new file mode 100644 index 0000000..db8b270 --- /dev/null +++ b/app/Services/Auth/TenantDiagnosticsService.php @@ -0,0 +1,114 @@ +where('tenant_id', (int) $tenant->getKey()) + ->where('role', 'owner') + ->exists(); + } + + public function userHasDuplicateMemberships(Tenant $tenant, User $user): bool + { + return TenantMembership::query() + ->where('tenant_id', (int) $tenant->getKey()) + ->where('user_id', (int) $user->getKey()) + ->count() > 1; + } + + public function mergeDuplicateMembershipsForUser(Tenant $tenant, User $actor, User $member): void + { + DB::transaction(function () use ($tenant, $actor, $member): void { + $memberships = TenantMembership::query() + ->where('tenant_id', (int) $tenant->getKey()) + ->where('user_id', (int) $member->getKey()) + ->orderBy('created_at') + ->get(); + + if ($memberships->count() <= 1) { + return; + } + + $roles = $memberships->pluck('role')->all(); + $roleToKeep = $this->highestRole($roles); + + $membershipToKeep = $memberships->firstWhere('role', $roleToKeep) ?? $memberships->first(); + if (! $membershipToKeep instanceof TenantMembership) { + return; + } + + $idsToDelete = $memberships + ->reject(fn (TenantMembership $m): bool => $m->getKey() === $membershipToKeep->getKey()) + ->pluck($membershipToKeep->getKeyName()) + ->all(); + + $membershipToKeep->forceFill([ + 'role' => $roleToKeep, + ])->save(); + + TenantMembership::query() + ->whereIn($membershipToKeep->getKeyName(), $idsToDelete) + ->delete(); + + $this->auditLogger->log( + tenant: $tenant, + action: AuditActionId::TenantMembershipDuplicatesMerged->value, + context: [ + 'metadata' => [ + 'member_user_id' => (int) $member->getKey(), + 'kept_membership_id' => (string) $membershipToKeep->getKey(), + 'deleted_membership_ids' => array_values(array_map('strval', $idsToDelete)), + 'result_role' => $roleToKeep, + 'source_roles' => $roles, + ], + ], + actorId: (int) $actor->getKey(), + actorEmail: $actor->email, + actorName: $actor->name, + status: 'success', + resourceType: 'tenant', + resourceId: (string) $tenant->getKey(), + ); + }); + } + + /** + * @param array $roles + */ + private function highestRole(array $roles): string + { + $priority = [ + 'owner' => 3, + 'manager' => 2, + 'readonly' => 1, + ]; + + $bestRole = 'readonly'; + $bestScore = 0; + + foreach ($roles as $role) { + $score = $priority[$role] ?? 0; + if ($score > $bestScore) { + $bestScore = $score; + $bestRole = (string) $role; + } + } + + return $bestRole; + } +} diff --git a/app/Support/Audit/AuditActionId.php b/app/Support/Audit/AuditActionId.php index 724a4b5..e093b64 100644 --- a/app/Support/Audit/AuditActionId.php +++ b/app/Support/Audit/AuditActionId.php @@ -13,4 +13,7 @@ enum AuditActionId: string // Not part of the v1 contract, but used in codebase. case TenantMembershipBootstrapRecover = 'tenant_membership.bootstrap_recover'; + + // Diagnostics / repair actions. + case TenantMembershipDuplicatesMerged = 'tenant_membership.duplicates_merged'; } diff --git a/app/Support/Rbac/UiEnforcement.php b/app/Support/Rbac/UiEnforcement.php index 232cd42..e8daa36 100644 --- a/app/Support/Rbac/UiEnforcement.php +++ b/app/Support/Rbac/UiEnforcement.php @@ -8,6 +8,7 @@ use App\Models\User; use App\Services\Auth\CapabilityResolver; use App\Support\Auth\Capabilities; +use App\Support\Auth\UiTooltips as AuthUiTooltips; use Closure; use Filament\Actions\Action; use Filament\Actions\BulkAction; @@ -282,7 +283,7 @@ private function applyDisabledState(): void return; } - $tooltip = $this->customTooltip ?? UiTooltips::INSUFFICIENT_PERMISSION; + $tooltip = $this->customTooltip ?? AuthUiTooltips::insufficientPermission(); $this->action->disabled(function (?Model $record = null) { $context = $this->resolveContextWithRecord($record); diff --git a/app/Support/Rbac/UiTooltips.php b/app/Support/Rbac/UiTooltips.php index bd8e874..a2c6c84 100644 --- a/app/Support/Rbac/UiTooltips.php +++ b/app/Support/Rbac/UiTooltips.php @@ -30,4 +30,14 @@ final class UiTooltips * Modal description for destructive action confirmation. */ public const DESTRUCTIVE_CONFIRM_DESCRIPTION = 'This action cannot be undone.'; + + /** + * Tooltip for actions that are unavailable because the tenant is archived. + */ + public const TENANT_ARCHIVED = 'This tenant is archived.'; + + /** + * Tooltip for actions that are unavailable because a tenant must always have an owner. + */ + public const TENANT_OWNER_REQUIRED = 'This tenant must have at least one Owner.'; } diff --git a/resources/views/filament/pages/tenant-diagnostics.blade.php b/resources/views/filament/pages/tenant-diagnostics.blade.php new file mode 100644 index 0000000..fbeebc5 --- /dev/null +++ b/resources/views/filament/pages/tenant-diagnostics.blade.php @@ -0,0 +1,31 @@ + +
+
+

Tenant diagnostics

+

+ Identify common tenant configuration issues and apply safe repairs. +

+
+ + @if ($missingOwner) +
+
Missing owner
+
This tenant currently has no Owner members.
+
+ @endif + + @if ($hasDuplicateMembershipsForCurrentUser) +
+
Duplicate memberships
+
This tenant has duplicate membership rows for your user.
+
+ @endif + + @if (! $missingOwner && ! $hasDuplicateMembershipsForCurrentUser) +
+
All good
+
No known issues detected.
+
+ @endif +
+ diff --git a/resources/views/filament/widgets/tenant/tenant-archived-banner.blade.php b/resources/views/filament/widgets/tenant/tenant-archived-banner.blade.php new file mode 100644 index 0000000..4d8226c --- /dev/null +++ b/resources/views/filament/widgets/tenant/tenant-archived-banner.blade.php @@ -0,0 +1,14 @@ +@php + /** @var ?\App\Models\Tenant $tenant */ +@endphp + +
+ @if ($tenant?->trashed()) +
+
+
Archived
+
{{ \App\Support\Rbac\UiTooltips::TENANT_ARCHIVED }}
+
+
+ @endif +
diff --git a/specs/067-rbac-troubleshooting/checklists/requirements.md b/specs/067-rbac-troubleshooting/checklists/requirements.md new file mode 100644 index 0000000..c87a2b6 --- /dev/null +++ b/specs/067-rbac-troubleshooting/checklists/requirements.md @@ -0,0 +1,46 @@ +# Specification Quality Checklist: RBAC Troubleshooting & Tenant UI Bugfix Pack v1 + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-01-31 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details beyond constitution-required constraints (RBAC semantics, canonical capability registry references) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders (with minimal, clearly-scoped technical constraints where required) +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification (outside the minimal, constitution-required constraints) + +## Notes + +- Items marked incomplete require spec updates before `/speckit.clarify` or `/speckit.plan` + +- Open issues identified by consistency analysis: + - Diagnostics + duplicates repair lacks explicit test coverage (spec FR-006). + - Repair actions require explicit audit trail tasks/tests (spec FR-008). + - SC-001 relies on an undefined “standard click paths” set. + - Spec/plan include some implementation-detail language that should be reduced. + +- Status: + - Duplicate membership coverage is addressed by tasks T019 and T023. + - Repair audit trail coverage is addressed by tasks T020 and T023. + - SC-001 was reworded to remove the undefined 95% threshold. + - Plan template placeholders were removed. diff --git a/specs/067-rbac-troubleshooting/contracts/tenant-diagnostics.openapi.yaml b/specs/067-rbac-troubleshooting/contracts/tenant-diagnostics.openapi.yaml new file mode 100644 index 0000000..6dfe74e --- /dev/null +++ b/specs/067-rbac-troubleshooting/contracts/tenant-diagnostics.openapi.yaml @@ -0,0 +1,68 @@ +openapi: 3.0.3 +info: + title: TenantPilot Admin — Tenant Diagnostics (Conceptual) + version: 0.1.0 + description: | + Conceptual HTTP contract for tenant-scoped diagnostics and repairs. + + NOTE: These routes are implemented as Filament (Livewire) pages/actions. + The exact Livewire request payload is not part of this contract; this captures + the user-visible HTTP surfaces and the logical operations. +servers: + - url: /admin +paths: + /t/{tenant}/diagnostics: + get: + summary: View tenant diagnostics page + description: | + Tenant-scoped diagnostics view. Must be tenant-member-safe: + - Non-member: 404 + - Member: 200 (even if tenant is archived) + parameters: + - name: tenant + in: path + required: true + schema: + type: string + description: Filament tenancy slug (Tenant.external_id) + responses: + '200': + description: Diagnostics page rendered + content: + text/html: + schema: + type: string + '404': + description: Not found (deny-as-not-found for non-members) + '403': + description: Forbidden (member without capability attempting an action) + /t/{tenant}/diagnostics/repairs/bootstrap-owner: + post: + summary: Repair missing owner by promoting a member + description: | + Promotes the selected tenant member to owner. + Must require confirmation in the UI and enforce authorization server-side. + parameters: + - name: tenant + in: path + required: true + schema: + type: string + requestBody: + required: true + content: + application/json: + schema: + type: object + required: [user_id] + properties: + user_id: + type: integer + description: Internal users.id + responses: + '204': + description: Repair succeeded + '403': + description: Forbidden (member lacks capability) + '404': + description: Not found (non-member) diff --git a/specs/067-rbac-troubleshooting/data-model.md b/specs/067-rbac-troubleshooting/data-model.md new file mode 100644 index 0000000..1393f7c --- /dev/null +++ b/specs/067-rbac-troubleshooting/data-model.md @@ -0,0 +1,64 @@ +# Data Model — RBAC Troubleshooting & Tenant UI Bugfix Pack v1 + +**Spec**: [spec.md](spec.md) + +No new persistent tables are required for v1. Diagnostics findings are computed at runtime from existing tables. + +## Entities + +### Tenant (`tenants`) + +**Purpose**: Tenant-plane scope boundary, lifecycle state, and tenant-scoped configuration root. + +**Key fields (existing)**: +- `id` (bigint, PK) +- `tenant_id` (string GUID, external Microsoft Entra tenant identifier) +- `external_id` (string, used as Filament tenancy slug; often mirrors `tenant_id`) +- `status` (string, e.g. `active`, `archived`) +- `deleted_at` (nullable timestamp, soft delete) +- `name`, `environment`, `is_current`, `metadata` (assorted) + +**Lifecycle rules (existing)**: +- Soft delete sets `status='archived'`. +- Restore sets `status='active'`. + +### TenantMembership (`tenant_memberships`) + +**Purpose**: Tenant membership boundary + role assignment. + +**Key fields (existing)**: +- `id` (uuid, PK) +- `tenant_id` (bigint, FK → `tenants.id`) +- `user_id` (bigint, FK → `users.id`) +- `role` (enum: `owner`, `manager`, `operator`, `readonly`) +- `source` / `source_ref` (provenance) +- `created_by_user_id` (nullable FK) + +**Constraints (existing)**: +- Unique `(tenant_id, user_id)` +- Index `(tenant_id, role)` + +### DiagnosticsFinding (computed, not persisted) + +**Purpose**: Represent a detected integrity/operational issue for the current tenant. + +**Proposed shape (runtime DTO / array)**: +- `id` (string, stable key like `missing_owner`) +- `severity` (string, e.g. `warning`/`critical`) +- `title` (string) +- `description` (string) +- `repair_actions` (array of available actions given actor capabilities) + +## Derived rules / invariants + +- **Missing owner**: `tenant_memberships` has zero rows with `role='owner'` for the tenant. +- **Duplicate membership**: more than one membership row exists for a given `(tenant_id, user_id)` (should be prevented by DB uniqueness; diagnostics treats this as “historical/edge-case”). +- **Identifier misuse risk**: any query for membership/tenant scoping must use internal tenant key (`tenants.id`) and not the GUID. + +## State transitions + +- Tenant status: + - `active` → `archived` on soft delete + - `archived` → `active` on restore +- Membership role transitions: + - Allowed transitions are existing (`owner`, `manager`, `operator`, `readonly`) but last-owner demotion/removal is prohibited. diff --git a/specs/067-rbac-troubleshooting/plan.md b/specs/067-rbac-troubleshooting/plan.md new file mode 100644 index 0000000..f4bec4f --- /dev/null +++ b/specs/067-rbac-troubleshooting/plan.md @@ -0,0 +1,158 @@ +# Implementation Plan: RBAC Troubleshooting & Tenant UI Bugfix Pack v1 + +**Branch**: `067-rbac-troubleshooting` | **Date**: 2026-01-31 | **Spec**: [spec.md](spec.md) +**Input**: Feature specification from [specs/067-rbac-troubleshooting/spec.md](spec.md) + +**Note**: This template is filled in by the `/speckit.plan` command. See `.specify/scripts/` for helper scripts. + +## Summary + +Tighten tenant-plane (`/admin`) RBAC UX and eliminate sharp edges by: + +- Applying the existing `UiEnforcement` pattern consistently on Tenant admin surfaces (especially Tenant view header actions). +- Ensuring archived (soft-deleted) tenants can still resolve in tenant-scoped routes for entitled members (404 only for non-members). +- Adding a tenant-scoped diagnostics surface to detect and repair membership invariants (missing owner) and prevent GUID-vs-bigint mistakes. +- Adding targeted Pest tests to lock in 404 vs 403 semantics and action disable/tooltip UX, and re-running existing last-owner invariant tests as regression coverage. + +## Technical Context + +**Language/Version**: PHP 8.4 (per repo guidelines) +**Primary Dependencies**: Laravel 12, Filament v5, Livewire v4 +**Storage**: PostgreSQL (via Laravel Sail) +**Testing**: Pest v4 (PHPUnit v12 runner) +**Target Platform**: Laravel Sail (Docker) on macOS/Linux +**Project Type**: Web application (Laravel monolith) +**Performance Goals**: N/A (admin UX + correctness) +**Constraints**: Tenant-plane only (`/admin`); no `/system` expansion; no new outbound integration calls for diagnostics; must preserve RBAC-UX semantics (404 vs 403) +**Scale/Scope**: Touches Tenant admin UI, tenancy binding for archived tenants, membership invariants + tests + +## 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; non-member tenant access is 404; member-but-missing-capability is 403; authorization checks use Gates/Policies + capability registries (no raw strings, no role-string checks) +- 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` +- 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 +- Badge semantics (BADGE-001): status-like badges use `BadgeCatalog` / `BadgeRenderer`; no ad-hoc mappings; new values include tests + +**Gate evaluation**: PASS (no constitution violations intended). + +- No new Microsoft Graph call paths are introduced. +- Mutations involved (archive/restore, membership repairs) keep explicit confirmation and server-side enforcement. +- Auditing: membership repairs use existing audit logger patterns; tenant lifecycle actions already log. + +**Post-design re-check**: PASS (design artifacts: [research.md](research.md), [data-model.md](data-model.md), [contracts/](contracts/)). + +## 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 +app/ +├── Filament/ +│ ├── Resources/ +│ ├── Pages/ +│ └── Widgets/ +├── Models/ +├── Policies/ +├── Services/ +└── Support/ + +database/ +├── migrations/ +└── factories/ + +resources/ +├── views/ +└── css/ + +routes/ +└── web.php + +tests/ +├── Feature/ +└── Unit/ +``` + +**Structure Decision**: Laravel monolith. Changes will land in `app/Filament/**` (tenant UI), `app/Models/**` (tenant binding/invariants), `app/Services/**` (membership repairs), and `tests/**` (Pest feature tests). + +## Complexity Tracking + +> **Fill ONLY if Constitution Check has violations that must be justified** + +N/A. + +## Phase 0 — Research (repo-backed) + +### Key findings (evidence) + +- Tenancy config: [app/Providers/Filament/AdminPanelProvider.php](../../app/Providers/Filament/AdminPanelProvider.php#L32-L45) uses `->tenant(Tenant::class, slugAttribute: 'external_id')` and `->tenantRoutePrefix('t')`. +- Tenant archiving is soft delete + status update: [app/Models/Tenant.php](../../app/Models/Tenant.php#L63-L92) sets `status='archived'` during soft delete and restores back to `active`. +- Tenant view header actions currently bypass UI enforcement: + - `EditAction::make()` is unconditional (can lead to 403-on-click UX): [app/Filament/Resources/TenantResource/Pages/ViewTenant.php](../../app/Filament/Resources/TenantResource/Pages/ViewTenant.php#L19-L24) + - `Deactivate` currently executes without capability checks (security issue): [app/Filament/Resources/TenantResource/Pages/ViewTenant.php](../../app/Filament/Resources/TenantResource/Pages/ViewTenant.php#L51-L74) +- Tenant list Restore action lacks icon: [app/Filament/Resources/TenantResource.php](../../app/Filament/Resources/TenantResource.php#L406-L454) +- Membership invariant “last owner” is already enforced server-side: [app/Services/Auth/TenantMembershipManager.php](../../app/Services/Auth/TenantMembershipManager.php#L253-L287) +- Membership uniqueness is already enforced at DB level: [database/migrations/2026_01_25_022729_create_tenant_memberships_table.php](../../database/migrations/2026_01_25_022729_create_tenant_memberships_table.php#L12-L26) + +### Decisions + +- Archived tenant 404s for members: implement tenant route binding that includes soft-deleted tenants, then rely on membership middleware to enforce deny-as-not-found for non-members. +- Replace ad-hoc tooltips on tenant actions with centralized `UiTooltips` constants (add a tenant-archived tooltip string). +- Standardize tenant page header actions using `UiEnforcement` so member-without-capability sees disabled actions with tooltip (no normal-click 403). + +**Output**: [research.md](research.md) + +## Phase 1 — Design (data model + contracts) + +### Design outline + +- Tenant status UX + - Archived tenants remain viewable for entitled members. + - Actions become status-aware (e.g., Deactivate disabled when already archived). +- Diagnostics + - Tenant-scoped, DB-only page that reports: + - Missing owner (0 owners) + - Identifier misuse risk (guardrails against using external GUID where an internal FK is expected) + - Repair actions are capability-gated and audit-logged. + +### Artifacts + +- [data-model.md](data-model.md) +- [contracts/](contracts/) +- [quickstart.md](quickstart.md) + +## Phase 2 — Planning (for tasks.md) + +Dependency-ordered implementation outline (will be expanded in `tasks.md`): + +1. Fix tenant view header actions (Edit/Deactivate) to use UI enforcement + tooltips. +2. Add Restore icon consistency on tenant list row menu. +3. Ensure archived tenant is resolvable in tenant-scoped routes for members (avoid false 404). +4. Add tenant status banner on view page. +5. Add diagnostics page + safe repair actions for “missing owner”. +6. Add regression tests for: + - Readonly UX (disabled + tooltip) + - Readonly cannot deactivate + - Archived tenant view access (member OK, non-member 404) + - GUID vs bigint guard test +7. Keep the no-ad-hoc authorization guard green. diff --git a/specs/067-rbac-troubleshooting/quickstart.md b/specs/067-rbac-troubleshooting/quickstart.md new file mode 100644 index 0000000..ea81049 --- /dev/null +++ b/specs/067-rbac-troubleshooting/quickstart.md @@ -0,0 +1,30 @@ +# Quickstart — RBAC Troubleshooting & Tenant UI Bugfix Pack v1 + +## Prereqs + +- Docker running +- Laravel Sail dependencies installed + +## Run locally + +- Start containers: `vendor/bin/sail up -d` +- Run targeted tests (after implementation): + - `vendor/bin/sail artisan test --compact tests/Feature/Rbac/` + - (guard) `vendor/bin/sail artisan test --compact tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php` +- Format: `vendor/bin/sail bin pint --dirty` + +## Manual smoke (after implementation) + +1. Sign in to `/admin`. +2. Pick a tenant scope via the tenant menu. +3. As a read-only member: + - Tenant view page shows Edit/Deactivate disabled with tooltip. + - Attempting to force-execute mutations still fails server-side. +4. Archive a tenant, then: + - Member can still open tenant view and sees an Archived banner. + - Non-member gets 404 for tenant scope URLs. + +## Notes + +- Filament v5 requires Livewire v4.0+ (this repo is already on Livewire v4). +- Panel providers in Laravel 11+ are registered in `bootstrap/providers.php` (this repo follows that convention). diff --git a/specs/067-rbac-troubleshooting/research.md b/specs/067-rbac-troubleshooting/research.md new file mode 100644 index 0000000..bafea0a --- /dev/null +++ b/specs/067-rbac-troubleshooting/research.md @@ -0,0 +1,119 @@ +# Research — RBAC Troubleshooting & Tenant UI Bugfix Pack v1 + +**Date**: 2026-01-31 +**Spec**: [spec.md](spec.md) + +This document captures design decisions and supporting rationale. Where applicable, decisions are grounded in the current codebase and the TenantPilot Constitution. + +## Decision 1 — Archived tenants must remain resolvable in tenant-scoped routes + +**Decision**: Make the tenant binding used by Filament tenancy include archived (soft-deleted) tenants so entitled members can view `/admin/t/{tenant}/...` even when archived. + +**Rationale**: +- The current tenant lifecycle uses soft deletes + `status='archived'` on delete and restores back to `active`. +- If tenancy resolution excludes trashed records, the app will 404 *before* membership middleware can apply “deny-as-not-found for non-members”, causing false 404s for legitimate members. +- Constitution RBAC-UX requires: + - non-members → 404 + - members but missing capability → 403 + - archived is a lifecycle state, not an isolation boundary. + +**Evidence**: +- Tenant tenancy config (slug is `external_id`, route prefix `t`): [Admin panel tenancy](../../app/Providers/Filament/AdminPanelProvider.php#L32-L45) +- Archived behavior is implemented via soft deletes + status mutations: [Tenant booted hooks](../../app/Models/Tenant.php#L63-L92) + +**Alternatives considered**: +- Stop using soft deletes for tenants and represent archived purely via a `status` field. + - Rejected: larger behavioral change, wider surface area (queries, “activeQuery”, etc.), higher regression risk. +- Keep current 404 and treat “archived” as inaccessible. + - Rejected: contradicts spec FR-004 and the operational need to restore. + +## Decision 2 — Standardize Tenant view header actions via `UiEnforcement` + +**Decision**: Wrap Tenant view page header actions (Edit/Deactivate) in the existing UI enforcement helper so tenant members without the capability see disabled actions with tooltips, avoiding “normal click leads to 403 page”. + +**Rationale**: +- Constitution RBAC-UX-004 requires visible-but-disabled for members lacking capability. +- Existing code already follows this pattern in multiple resources (e.g., tenant list actions, membership manager relation managers). +- The current `ViewTenant` header actions include an unconditional edit action and an archive action that executes without capability checks. + +**Evidence**: +- Unconditional edit header action: [ViewTenant header action group](../../app/Filament/Resources/TenantResource/Pages/ViewTenant.php#L19-L24) +- Deactivate executes without capability gating: [ViewTenant archive action](../../app/Filament/Resources/TenantResource/Pages/ViewTenant.php#L51-L74) + +**Alternatives considered**: +- Hide actions entirely for users without capability. + - Rejected by default: RBAC-UX prefers visible-but-disabled. +- Leave the action visible and rely only on server-side 403. + - Rejected: fails the UX requirement (normal click to 403). + +## Decision 3 — Restore icon consistency is a UI-only patch + +**Decision**: Add an icon to the tenant row `Restore` action. + +**Rationale**: +- Improves consistency and reduces “is this safe?” uncertainty. +- No behavioral/authorization changes; minimal regression risk. + +**Evidence**: +- Restore action currently has no icon: [TenantResource restore action](../../app/Filament/Resources/TenantResource.php#L406-L454) + +**Alternatives considered**: +- Leave it as-is. + - Rejected: explicit FR-001. + +## Decision 4 — Diagnostics is DB-only and uses existing audit logging patterns + +**Decision**: Implement tenant-scoped diagnostics as DB-only rendering and DB-only repairs with explicit confirmation + server-side authorization, recording audit logs. + +**Rationale**: +- Constitution requires read/write separation and that monitoring/diagnostics pages do not trigger external calls during render. +- Repairs are security-relevant mutations and must be audited. + +**Alternatives considered**: +- Attach diagnostics to Monitoring/Operations. + - Rejected: these repairs are DB-only and do not need `OperationRun` by default; keep scope small. + +## Decision 5 — Membership invariants + +**Decision**: +- Continue using server-side guards preventing last-owner removal/demotion. +- Add diagnostics checks for “missing owner” and provide a safe repair (promote chosen member to owner) gated by an existing management capability. + +**Rationale**: +- Last-owner guard exists and is correct; the missing-owner case needs a UI recovery path. + +**Evidence**: +- Last-owner guard behavior: [TenantMembershipManager last-owner guards](../../app/Services/Auth/TenantMembershipManager.php#L253-L287) + +**Alternatives considered**: +- DB constraint enforcing at least one owner. + - Rejected: non-trivial, would complicate bulk edits and migrations; better handled at app layer with explicit repair tools. + +## Decision 6 — Duplicate memberships + +**Decision**: Keep the DB-level uniqueness constraint as the primary protection, but still implement diagnostics that can detect historical duplicates and merge them safely (no-op when none exist). + +**Rationale**: +- The uniqueness constraint exists today, but the product requirement includes a recovery/repair flow. + +**Evidence**: +- Unique constraint in migration: [tenant_memberships unique index](../../database/migrations/2026_01_25_022729_create_tenant_memberships_table.php#L12-L26) + +**Alternatives considered**: +- Remove diagnostics for duplicates since the DB constraint exists. + - Rejected: conflicts with FR-006 and the “UI-only recovery” goal. + +## Decision 7 — GUID vs bigint guardrails + +**Decision**: Explicitly distinguish: +- internal tenant primary key (`tenants.id`, bigint) +- external tenant identifiers (`tenant_id` GUID / `external_id` string) + +and ensure code paths that need an internal FK use `$tenant->getKey()` (cast to int), not `$tenant->tenant_id`. + +**Rationale**: +- Prevent PostgreSQL `invalid input syntax for type bigint` errors caused by passing the GUID into bigint `tenant_id` columns. + +**Alternatives considered**: +- Rename columns. + - Rejected (out of scope / migration-heavy). diff --git a/specs/067-rbac-troubleshooting/spec.md b/specs/067-rbac-troubleshooting/spec.md new file mode 100644 index 0000000..b0280ad --- /dev/null +++ b/specs/067-rbac-troubleshooting/spec.md @@ -0,0 +1,130 @@ +# Feature Specification: RBAC Troubleshooting & Tenant UI Bugfix Pack v1 + +**Feature Branch**: `067-rbac-troubleshooting` +**Created**: 2026-01-31 +**Status**: Draft +**Input**: RBAC troubleshooting + tenant admin UI bugfix pack focused on the tenant-plane admin experience (`/admin`). + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Permission-safe tenant UI interactions (Priority: P1) + +A tenant member with read-only permissions can browse tenant pages and menus without being sent to error pages for normal UI clicks. + +**Why this priority**: Eliminates confusing UX and reduces support load; also prevents “permission leak” regressions in high-risk actions. + +**Independent Test**: As a read-only member, verify restricted actions are visible-but-disabled with an explanatory tooltip and cannot be executed. + +**Acceptance Scenarios**: + +1. **Given** a user is a member of a tenant but lacks tenant management capability, **When** they view tenant lists and pages, **Then** management actions (e.g. Edit, Deactivate) are disabled and show an explanatory tooltip. +2. **Given** a user is a member but lacks tenant management capability, **When** they attempt to execute a blocked mutation (direct request / forced execution), **Then** the server denies it. + +--- + +### User Story 2 - Archived tenant remains viewable for entitled members (Priority: P1) + +A tenant member who is entitled to view a tenant can still access the tenant view even if the tenant is archived, and the UI clearly indicates status. + +**Why this priority**: Prevents accidental “false 404” for legitimate members; enables safe lifecycle operations like restore. + +**Independent Test**: Mark a tenant archived, ensure a viewing member can load it and sees an “Archived” banner; ensure non-members still receive 404. + +**Acceptance Scenarios**: + +1. **Given** a tenant is archived and a user is a member with tenant view capability, **When** they navigate to the tenant view, **Then** the page loads and shows an “Archived” status banner. +2. **Given** a tenant is archived and a user is not a member, **When** they navigate to the tenant scope URL, **Then** they receive a 404 (deny-as-not-found). + +--- + +### User Story 3 - Diagnose & repair tenant membership invariants (Priority: P2) + +An authorized tenant owner/manager can identify and fix membership integrity issues (missing owner, duplicates) via a dedicated troubleshooting screen. + +**Why this priority**: Provides a recovery path for real-world “sharp edges” without requiring direct database intervention. + +**Independent Test**: Seed broken states (0 owners, duplicate memberships), confirm diagnostics flag them and repair actions resolve them. + +**Acceptance Scenarios**: + +1. **Given** a tenant has no owners, **When** an authorized user opens diagnostics, **Then** the issue is surfaced and a repair action can promote a member to owner. +2. **Given** a tenant has duplicate memberships for the same user, **When** an authorized user runs “merge duplicates”, **Then** duplicates are removed and exactly one membership remains. + +### Edge Cases + +- A tenant member without management capability attempts to execute a blocked action via a forged request. +- A non-member attempts to access a tenant-scoped page (must be deny-as-not-found). +- Tenant is archived: view allowed for entitled members; lifecycle actions are status-appropriate. +- Tenant has 0 owners; tenant has 1 owner and an action would demote/remove the last owner. +- Membership duplicates exist and are resolved; ensure resulting owner count remains valid. +- A tenant has an external GUID identifier; ensure no internal lookups treat that GUID as the internal tenant primary key. + +## Requirements *(mandatory)* + +**Constitution alignment (required):** If this feature introduces any Microsoft Graph calls, any write/change behavior, +or any long-running/queued/scheduled work, the spec MUST describe contract registry updates, safety gates +(preview/confirmation/audit), tenant isolation, run observability (operation run record: type/identity/visibility), and tests. +If security-relevant DB-only actions intentionally skip an operation run record, the spec MUST describe audit log entries. + +**Constitution alignment (RBAC-UX):** If this feature introduces or changes authorization behavior, the spec MUST: +- state which authorization plane(s) are involved (tenant `/admin/t/{tenant}` vs platform `/system`), +- ensure any cross-plane access is deny-as-not-found (404), +- explicitly define 404 vs 403 semantics: + - non-member / not entitled to tenant scope → 404 (deny-as-not-found) + - member but missing capability → 403 +- describe how authorization is enforced server-side (Gates/Policies) for every mutation/operation-start/credential change, +- reference the canonical capability registry (no raw capability strings; no role-string checks in feature code), +- ensure global search is tenant-scoped and non-member-safe (no hints; inaccessible results treated as 404 semantics), +- ensure destructive-like actions require an explicit confirmation step, +- include at least one positive and one negative authorization test, and note any RBAC regression tests added/updated. + +**Constitution alignment (OPS-EX-AUTH-001):** OIDC/SAML login handshakes may perform synchronous outbound HTTP (e.g., token exchange) +on `/auth/*` endpoints without an operation run record. This MUST NOT be used for Monitoring/Operations pages. + +**Constitution alignment (BADGE-001):** If this feature changes status-like badges (status/outcome/severity/risk/availability/boolean), +the spec MUST describe how badge semantics stay centralized (no ad-hoc mappings) and which tests cover any new/changed values. + +### Functional Requirements + +- **FR-001 (Restore action icon)**: The tenant list row menu MUST display a consistent icon for the Restore action. +- **FR-002 (Readonly edit UX)**: For a tenant member lacking tenant management capability, the UI MUST present Edit as disabled with an explanatory tooltip and MUST avoid normal-click navigation leading to an error page. +- **FR-003 (Readonly cannot deactivate)**: Deactivate MUST be treated as a protected mutation and MUST NOT be executable by a tenant member lacking tenant management capability. +- **FR-004 (Archived tenant access)**: An entitled tenant member (tenant view capability) MUST be able to open archived tenants. A non-member MUST receive a 404 for tenant scope URLs. +- **FR-005 (Owner invariant)**: The system MUST prevent removing/demoting the last remaining tenant owner and MUST surface “missing owner” situations in diagnostics. +- **FR-006 (Membership integrity)**: The system MUST prevent duplicate memberships per (tenant, user). Diagnostics MUST identify duplicates and provide a safe repair flow for authorized users. +- **FR-007 (GUID vs internal ID)**: The system MUST treat external tenant identifiers (GUIDs) as external-only identifiers and MUST NOT use them where an internal tenant primary key is required. +- **FR-008 (Diagnostics access + safety)**: Diagnostics MUST not expose cross-tenant data to non-members. Repair actions MUST be permission-gated and MUST record an auditable trail. + +#### Authorization & Error Semantics (explicit) + +- Tenant-scope URLs: non-member / not entitled MUST be deny-as-not-found (404). +- Member but missing capability: server-side MUST deny execution (403). +- UI behavior: member-without-capability SHOULD see actions visible-but-disabled with tooltip to prevent normal-click 403 pages. + +#### Out of Scope + +- No new “Workspace” model in this spec. +- No platform-plane (`/system`) expansion. +- No outbound integration calls are introduced as part of diagnostics/repairs. + +#### Assumptions + +- “Archived” is treated as an archived/lifecycle state that remains viewable to entitled members. +- Deactivate (archive) uses the existing tenant lifecycle capability `Capabilities::TENANT_DELETE`. + +### Key Entities *(include if feature involves data)* + +- **Tenant**: The internal customer/workspace entity users are members of; has an internal primary key and an external identifier. +- **Tenant Membership**: Links a user to a tenant with one or more capability grants or derived entitlements. +- **Capability / Entitlement**: A named permission used to determine visibility/execution of actions within a tenant scope. +- **Diagnostics Finding**: A detected issue such as “missing owner”, “duplicate membership”, or “identifier misuse risk”. +- **Repair Action**: A controlled, permission-gated remediation step that fixes a finding and records an audit entry. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: Read-only members can navigate all tenant UI flows covered by automated tests without encountering an error page during normal click paths. +- **SC-002**: Protected tenant mutations (e.g., deactivate) have 0 successful executions by users lacking the required capability across automated tests. +- **SC-003**: Archived tenant access works as defined: entitled members can view; non-members get 404, verified by automated tests. +- **SC-004**: Membership integrity can be restored via UI-only repair actions for seeded broken states, verified by automated tests. diff --git a/specs/067-rbac-troubleshooting/tasks.md b/specs/067-rbac-troubleshooting/tasks.md new file mode 100644 index 0000000..f05eca5 --- /dev/null +++ b/specs/067-rbac-troubleshooting/tasks.md @@ -0,0 +1,156 @@ +--- + +description: "Task list for feature implementation" +--- + +# Tasks: RBAC Troubleshooting & Tenant UI Bugfix Pack v1 + +**Input**: Design documents from `/specs/067-rbac-troubleshooting/` +**Prerequisites**: plan.md (required), spec.md (required for user stories), checklists/requirements.md (required), research.md, data-model.md, contracts/, quickstart.md + +**Tests**: Required (Pest) for all runtime behavior changes. + +--- + +## Phase 1: Setup (Shared Infrastructure) + +**Purpose**: Ensure local environment is ready for implementation + testing. + +- [x] T001 Start Sail services and ensure DB is ready using vendor/bin/sail (vendor/bin/sail) +- [x] T002 [P] Run baseline tests to capture current behavior (tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php) + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: Shared primitives that user stories depend on. + +- [x] T003 [P] Review and, if needed, extend standardized RBAC tooltip strings in app/Support/Rbac/UiTooltips.php (app/Support/Rbac/UiTooltips.php) +- [x] T004 [P] Confirm the tenant lifecycle capability used for archive/restore is Capabilities::TENANT_DELETE (app/Support/Auth/Capabilities.php) + +**Checkpoint**: Foundation ready — user story work can begin. + +--- + +## Phase 3: User Story 1 — Permission-safe tenant UI interactions (Priority: P1) 🎯 MVP + +**Goal**: Readonly tenant members can browse tenant pages without normal-click navigation leading to error pages; protected actions are disabled with tooltips and blocked server-side. + +**Independent Test**: As a `readonly` member, `ViewTenant` shows Edit/Deactivate as disabled+tooltip and Deactivate cannot be executed. + +### Tests (write first) + +- [x] T005 [P] [US1] Add ViewTenant header action UX tests for readonly vs owner in tests/Feature/Filament/TenantViewHeaderUiEnforcementTest.php (tests/Feature/Filament/TenantViewHeaderUiEnforcementTest.php) +- [x] T006 [P] [US1] Add regression test that readonly cannot execute the archive action (silently blocked by Filament; ensure no state change) in tests/Feature/Filament/TenantViewHeaderUiEnforcementTest.php (tests/Feature/Filament/TenantViewHeaderUiEnforcementTest.php) + +### Implementation + +- [x] T007 [US1] Replace unconditional EditAction in ViewTenant header with a visible-but-disabled URL action + tooltip in app/Filament/Resources/TenantResource/Pages/ViewTenant.php (app/Filament/Resources/TenantResource/Pages/ViewTenant.php) +- [x] T008 [US1] Wrap Deactivate (archive) header action with UiEnforcement + Capabilities::TENANT_DELETE + destructive confirmation, and enforce server-side authorization (Gate/Policy) in the mutation handler, in app/Filament/Resources/TenantResource/Pages/ViewTenant.php (app/Filament/Resources/TenantResource/Pages/ViewTenant.php) +- [x] T009 [US1] Ensure no ad-hoc auth patterns were added to app/Filament/** and update allowlist only if unavoidable (tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php) + +--- + +## Phase 4: User Story 2 — Archived tenant remains viewable for entitled members (Priority: P1) + +**Goal**: Archived (soft-deleted) tenants remain resolvable in tenant-scoped routes for entitled members; UI clearly indicates archived status; non-members still get 404. + +**Independent Test**: Archive a tenant; member can access `/admin/t/{external_id}` and can mount `ViewTenant`; non-member receives 404. + +### Tests (write first) + +- [x] T010 [P] [US2] Add archived-tenant access test for members vs non-members in tests/Feature/TenantRBAC/ArchivedTenantRouteAccessTest.php (tests/Feature/TenantRBAC/ArchivedTenantRouteAccessTest.php) +- [x] T011 [P] [US2] Add UI assertion that archived tenants show an explicit archived indicator on ViewTenant in tests/Feature/Filament/ArchivedTenantViewTest.php (tests/Feature/Filament/ArchivedTenantViewTest.php) + +### Implementation + +- [x] T012 [US2] Update Tenant model route binding to resolve archived tenants by external_id using withTrashed() override in app/Models/Tenant.php (app/Models/Tenant.php) +- [x] T013 [US2] Add an “Archived” indicator on the tenant view header (e.g., subheading or banner) in app/Filament/Resources/TenantResource/Pages/ViewTenant.php (app/Filament/Resources/TenantResource/Pages/ViewTenant.php) +- [x] T014 [US2] Add a consistent icon for the Restore action in tenant list row menu in app/Filament/Resources/TenantResource.php (app/Filament/Resources/TenantResource.php) + +--- + +## Phase 5: User Story 3 — Diagnose & repair tenant membership invariants (Priority: P2) + +**Goal**: Authorized tenant members can see diagnostics for membership integrity (missing owner, duplicates) and run safe repair actions. + +**Independent Test**: Seed a missing-owner scenario; diagnostics reports it; authorized user can repair by promoting a member to owner; unauthorized user is blocked. + +### Tests (write first) + +- [x] T015 [P] [US3] Add tenant diagnostics access tests (member OK, non-member 404) in tests/Feature/TenantRBAC/TenantDiagnosticsAccessTest.php (tests/Feature/TenantRBAC/TenantDiagnosticsAccessTest.php) +- [x] T016 [P] [US3] Add repair action test for missing owner (promote member to owner) in tests/Feature/Filament/TenantDiagnosticsRepairsTest.php (tests/Feature/Filament/TenantDiagnosticsRepairsTest.php) +- [x] T017 [P] [US3] Add negative authorization test: member without required capability sees repair action disabled + tooltip (and cannot execute; silently blocked by Filament) in tests/Feature/Filament/TenantDiagnosticsRepairsTest.php (tests/Feature/Filament/TenantDiagnosticsRepairsTest.php) +- [x] T018 [P] [US3] Add guardrail test that diagnostics/repairs never use external GUID where internal tenant PK is required (prevents bigint cast errors) in tests/Feature/TenantRBAC/TenantGuidVsBigintGuardTest.php (tests/Feature/TenantRBAC/TenantGuidVsBigintGuardTest.php) +- [x] T019 [P] [US3] Add duplicate membership diagnostics + merge repair test (seed duplicates, assert finding, execute merge, assert exactly 1 membership remains) in tests/Feature/Filament/TenantDiagnosticsRepairsTest.php (tests/Feature/Filament/TenantDiagnosticsRepairsTest.php) +- [x] T020 [P] [US3] Add audit trail test: each repair action writes an AuditLog entry with stable action id + tenant + actor in tests/Feature/Filament/TenantDiagnosticsRepairsTest.php (tests/Feature/Filament/TenantDiagnosticsRepairsTest.php) + +### Implementation + +- [x] T021 [US3] Implement tenant-scoped diagnostics Filament page route under `/admin/t/{tenant}/diagnostics` in app/Filament/Pages/TenantDiagnostics.php (app/Filament/Pages/TenantDiagnostics.php) +- [x] T022 [US3] Implement a diagnostics service to compute findings (missing owner, duplicates) in app/Services/Auth/TenantDiagnosticsService.php (app/Services/Auth/TenantDiagnosticsService.php) +- [x] T023 [US3] Implement diagnostics page actions with UiEnforcement + confirmation + server-side authorization enforcement + audit logging (bootstrap owner, merge duplicates) in app/Filament/Pages/TenantDiagnostics.php (app/Filament/Pages/TenantDiagnostics.php) +- [x] T024 [US3] Ensure all tenant membership queries in new/changed code use the internal tenant key `(int) $tenant->getKey()` (never `$tenant->tenant_id`) and refactor any offenders found during implementation (app/Services/Auth/TenantDiagnosticsService.php) + +--- + +## Phase 6: Polish & Cross-Cutting Concerns + +- [x] T025 [P] Run formatter on touched files (dirty only) via vendor/bin/sail (vendor/bin/sail) +- [x] T026 Run targeted test suite covering all new tests + auth guard (tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php) + +--- + +## Dependencies & Execution Order + +### Phase Dependencies + +- **Phase 1 (Setup)** → blocks everything else +- **Phase 2 (Foundational)** → blocks all user stories +- **US1 / US2 / US3** can start after Phase 2, but US1 and US2 both touch `ViewTenant`, so coordinate to avoid merge conflicts +- **Polish** after all desired stories are complete + +### User Story Dependencies + +- **US1 (P1)**: independent after Phase 2 +- **US2 (P1)**: independent after Phase 2 +- **US3 (P2)**: independent after Phase 2 (reuses shared auth + tenant scoping) + +--- + +## Parallel Execution Examples + +### US1 + +- In parallel: + - T005 (tests) and T006 (tests) +- Then: + - T007 + T008 (implementation) + +### US2 + +- In parallel: + - T010 (route access tests) and T011 (UI indicator test) +- Then: + - T012 (route binding) and T013/T014 (UI fixes) + +### US3 + +- In parallel: + - T015–T020 (tests) +- Then: + - T021–T024 (implementation) + +--- + +## Implementation Strategy + +### Suggested MVP Scope + +- MVP = **User Story 1** only (T005–T009), because it fixes the most user-visible and security-adjacent UX issues and is easiest to validate independently. + +### Incremental Delivery + +1. US1 → validate UX + server-side enforcement +2. US2 → validate archived access + banner + restore icon +3. US3 → add diagnostics + repairs with strict authorization diff --git a/tests/Feature/Filament/ArchivedTenantViewTest.php b/tests/Feature/Filament/ArchivedTenantViewTest.php new file mode 100644 index 0000000..318149a --- /dev/null +++ b/tests/Feature/Filament/ArchivedTenantViewTest.php @@ -0,0 +1,25 @@ +actingAs($user); + + $tenant->delete(); + + Filament::setTenant($tenant, true); + + Livewire::test(ViewTenant::class, ['record' => $tenant->getRouteKey()]) + ->assertSee('Archived') + ->assertSee(UiTooltips::TENANT_ARCHIVED); +}); diff --git a/tests/Feature/Filament/BackupItemsBulkRemoveTest.php b/tests/Feature/Filament/BackupItemsBulkRemoveTest.php index 981c072..a96591b 100644 --- a/tests/Feature/Filament/BackupItemsBulkRemoveTest.php +++ b/tests/Feature/Filament/BackupItemsBulkRemoveTest.php @@ -9,7 +9,6 @@ use App\Models\Tenant; use Filament\Facades\Filament; use Illuminate\Foundation\Testing\RefreshDatabase; -use Filament\Facades\Filament; use Illuminate\Support\Facades\Queue; use Livewire\Livewire; diff --git a/tests/Feature/Filament/TenantDiagnosticsRepairsTest.php b/tests/Feature/Filament/TenantDiagnosticsRepairsTest.php new file mode 100644 index 0000000..04d1c16 --- /dev/null +++ b/tests/Feature/Filament/TenantDiagnosticsRepairsTest.php @@ -0,0 +1,116 @@ +actingAs($manager); + + Filament::setTenant($tenant, true); + + expect(TenantMembership::query() + ->where('tenant_id', (int) $tenant->getKey()) + ->where('role', 'owner') + ->count())->toBe(0); + + Livewire::test(TenantDiagnostics::class) + ->assertSee('Missing owner') + ->assertActionVisible('bootstrapOwner') + ->assertActionEnabled('bootstrapOwner') + ->mountAction('bootstrapOwner') + ->callMountedAction() + ->assertSuccessful(); + + expect(TenantMembership::query() + ->where('tenant_id', (int) $tenant->getKey()) + ->where('role', 'owner') + ->count())->toBe(1); + + expect(AuditLog::query() + ->where('tenant_id', (int) $tenant->getKey()) + ->where('action', AuditActionId::TenantMembershipBootstrapRecover->value) + ->exists())->toBeTrue(); + }); + + it('shows repair actions as disabled for readonly members', function () { + [$readonly, $tenant] = createUserWithTenant(role: 'readonly'); + + $this->actingAs($readonly); + + Filament::setTenant($tenant, true); + + // Force missing-owner state. + TenantMembership::query() + ->where('tenant_id', (int) $tenant->getKey()) + ->update(['role' => 'readonly']); + + Livewire::test(TenantDiagnostics::class) + ->assertActionVisible('bootstrapOwner') + ->assertActionDisabled('bootstrapOwner') + ->assertActionExists('bootstrapOwner', function (Action $action): bool { + return $action->getTooltip() === UiTooltips::INSUFFICIENT_PERMISSION; + }); + }); + + it('merges duplicate memberships for the current user (diagnostics repair)', function () { + [$owner, $tenant] = createUserWithTenant(role: 'owner'); + + $this->actingAs($owner); + + Filament::setTenant($tenant, true); + + // Intentionally create a broken state by temporarily dropping the unique uniqueness enforcement. + // Tests typically run on SQLite, which uses a unique index rather than a named table constraint. + $driver = DB::getDriverName(); + if ($driver === 'sqlite') { + DB::statement('DROP INDEX tenant_memberships_tenant_id_user_id_unique'); + } else { + DB::statement('ALTER TABLE tenant_memberships DROP CONSTRAINT tenant_memberships_tenant_id_user_id_unique'); + } + + TenantMembership::query()->create([ + 'tenant_id' => (int) $tenant->getKey(), + 'user_id' => (int) $owner->getKey(), + 'role' => 'readonly', + 'source' => 'manual', + 'created_by_user_id' => (int) $owner->getKey(), + ]); + + expect(TenantMembership::query() + ->where('tenant_id', (int) $tenant->getKey()) + ->where('user_id', (int) $owner->getKey()) + ->count())->toBeGreaterThan(1); + + Livewire::test(TenantDiagnostics::class) + ->assertActionVisible('mergeDuplicateMemberships') + ->assertActionEnabled('mergeDuplicateMemberships') + ->mountAction('mergeDuplicateMemberships') + ->callMountedAction() + ->assertSuccessful(); + + expect(TenantMembership::query() + ->where('tenant_id', (int) $tenant->getKey()) + ->where('user_id', (int) $owner->getKey()) + ->count())->toBe(1); + + expect(AuditLog::query() + ->where('tenant_id', (int) $tenant->getKey()) + ->where('action', AuditActionId::TenantMembershipDuplicatesMerged->value) + ->exists())->toBeTrue(); + }); +}); diff --git a/tests/Feature/Filament/TenantSetupTest.php b/tests/Feature/Filament/TenantSetupTest.php index e53cdd9..1d0ff22 100644 --- a/tests/Feature/Filament/TenantSetupTest.php +++ b/tests/Feature/Filament/TenantSetupTest.php @@ -232,7 +232,9 @@ public function request(string $method, string $path, array $options = []): Grap Filament::setTenant($tenant, true); Livewire::test(ViewTenant::class, ['record' => $tenant->getRouteKey()]) - ->callAction('archive'); + ->mountAction('archive') + ->callMountedAction() + ->assertHasNoActionErrors(); $this->assertSoftDeleted('tenants', ['id' => $tenant->id]); }); diff --git a/tests/Feature/Filament/TenantViewHeaderUiEnforcementTest.php b/tests/Feature/Filament/TenantViewHeaderUiEnforcementTest.php new file mode 100644 index 0000000..1e2ba24 --- /dev/null +++ b/tests/Feature/Filament/TenantViewHeaderUiEnforcementTest.php @@ -0,0 +1,68 @@ +actingAs($user); + + $tenant->makeCurrent(); + Filament::setTenant($tenant, true); + + Livewire::test(ViewTenant::class, ['record' => $tenant->getRouteKey()]) + ->assertActionVisible('edit') + ->assertActionDisabled('edit') + ->assertActionExists('edit', function (Action $action): bool { + return $action->getTooltip() === UiTooltips::INSUFFICIENT_PERMISSION; + }) + ->assertActionVisible('archive') + ->assertActionDisabled('archive') + ->assertActionExists('archive', function (Action $action): bool { + return $action->getTooltip() === UiTooltips::INSUFFICIENT_PERMISSION; + }); + }); + + it('shows edit and deactivate actions as enabled for owner members', function () { + [$user, $tenant] = createUserWithTenant(role: 'owner'); + + $this->actingAs($user); + + $tenant->makeCurrent(); + Filament::setTenant($tenant, true); + + Livewire::test(ViewTenant::class, ['record' => $tenant->getRouteKey()]) + ->assertActionVisible('edit') + ->assertActionEnabled('edit') + ->assertActionVisible('archive') + ->assertActionEnabled('archive'); + }); + + it('does not execute the deactivate action for readonly members (silently blocked by Filament)', function () { + [$user, $tenant] = createUserWithTenant(role: 'readonly'); + + $this->actingAs($user); + + $tenant->makeCurrent(); + Filament::setTenant($tenant, true); + + Livewire::test(ViewTenant::class, ['record' => $tenant->getRouteKey()]) + ->assertActionDisabled('archive') + ->mountAction('archive') + ->callMountedAction() + ->assertSuccessful(); + + expect(Tenant::withTrashed()->find($tenant->getKey())?->trashed())->toBeFalse(); + }); +}); diff --git a/tests/Feature/TenantRBAC/ArchivedTenantRouteAccessTest.php b/tests/Feature/TenantRBAC/ArchivedTenantRouteAccessTest.php new file mode 100644 index 0000000..152c546 --- /dev/null +++ b/tests/Feature/TenantRBAC/ArchivedTenantRouteAccessTest.php @@ -0,0 +1,28 @@ +delete(); + + $this->actingAs($user) + ->get("/admin/t/{$tenant->external_id}") + ->assertSuccessful(); +}); + +it('returns 404 for non-members on the tenant dashboard route for archived tenants', function () { + $tenant = Tenant::factory()->create(['external_id' => 'archived-tenant-a']); + $tenant->delete(); + + $user = User::factory()->create(); + + $this->actingAs($user) + ->get("/admin/t/{$tenant->external_id}") + ->assertNotFound(); +}); diff --git a/tests/Feature/TenantRBAC/TenantDiagnosticsAccessTest.php b/tests/Feature/TenantRBAC/TenantDiagnosticsAccessTest.php new file mode 100644 index 0000000..60bbd70 --- /dev/null +++ b/tests/Feature/TenantRBAC/TenantDiagnosticsAccessTest.php @@ -0,0 +1,24 @@ +actingAs($user) + ->get("/admin/t/{$tenant->external_id}/diagnostics") + ->assertSuccessful(); +}); + +it('returns 404 for non-members on the tenant diagnostics page', function () { + $tenant = Tenant::factory()->create(['external_id' => 'tenant-diag-a']); + $user = User::factory()->create(); + + $this->actingAs($user) + ->get("/admin/t/{$tenant->external_id}/diagnostics") + ->assertNotFound(); +}); diff --git a/tests/Feature/TenantRBAC/TenantGuidVsBigintGuardTest.php b/tests/Feature/TenantRBAC/TenantGuidVsBigintGuardTest.php new file mode 100644 index 0000000..3d2109c --- /dev/null +++ b/tests/Feature/TenantRBAC/TenantGuidVsBigintGuardTest.php @@ -0,0 +1,28 @@ +forceFill([ + 'external_id' => '00000000-0000-0000-0000-000000000123', + 'tenant_id' => '00000000-0000-0000-0000-000000000123', + ])->save(); + + $this->actingAs($user); + + Filament::setTenant($tenant, true); + + Livewire::test(TenantDiagnostics::class) + ->assertSuccessful(); +});