140 lines
7.1 KiB
Markdown
140 lines
7.1 KiB
Markdown
# Implementation Plan: 106 — Required Permissions Sidebar Context Fix
|
|
|
|
**Branch**: `106-required-permissions-sidebar-context` | **Date**: 2025-02-22 | **Spec**: [spec.md](spec.md)
|
|
**Input**: Feature specification from `/specs/106-required-permissions-sidebar-context/spec.md`
|
|
|
|
## Summary
|
|
|
|
Fix the `EnsureFilamentTenantSelected` middleware so that the Required Permissions page (`/admin/tenants/{tenant}/required-permissions`) renders the **workspace sidebar** instead of the tenant sidebar. The page uses `{tenant}` for data display and authorization only — it is a workspace-scoped page registered in the admin panel. The middleware currently calls `Filament::setTenant()` for all routes with a `{tenant}` parameter, which incorrectly triggers tenant-scoped navigation. The fix adds a workspace-scoped page allowlist check that skips `setTenant()` while preserving all 8 authorization checks.
|
|
|
|
## Technical Context
|
|
|
|
**Language/Version**: PHP 8.4 (Laravel 12)
|
|
**Primary Dependencies**: Filament v5, Livewire v4
|
|
**Storage**: N/A — no data changes
|
|
**Testing**: Pest v4 (`vendor/bin/sail artisan test`)
|
|
**Target Platform**: Web (Sail Docker)
|
|
**Project Type**: Web (Laravel + Filament admin panel)
|
|
**Performance Goals**: N/A — string comparison in middleware (negligible)
|
|
**Constraints**: Zero regression for tenant-scoped pages; zero data model changes
|
|
**Scale/Scope**: 1 middleware file, 1 new test file, ~20 lines of logic
|
|
|
|
## Constitution Check
|
|
|
|
*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.*
|
|
|
|
- [x] Inventory-first: N/A — no inventory changes
|
|
- [x] Read/write separation: N/A — no writes, pure navigation rendering fix
|
|
- [x] Graph contract path: N/A — no Graph calls introduced or modified
|
|
- [x] Deterministic capabilities: N/A — no capability changes
|
|
- [x] RBAC-UX: Two planes remain separated. The fix preserves all 8 authorization checks (workspace membership, tenant access, deny-as-not-found 404). Only sidebar rendering is changed. No cross-plane leakage.
|
|
- [x] Workspace isolation: Non-member access remains 404. Workspace context is preserved. The fix explicitly avoids changing workspace context switching behavior.
|
|
- [x] RBAC-UX destructive actions: N/A — no actions added or modified
|
|
- [x] RBAC-UX global search: N/A — no global search changes
|
|
- [x] Tenant isolation: All tenant reads remain scoped. The tenant param is still resolved and authorized — it is just not set as Filament active tenant for sidebar purposes.
|
|
- [x] Run observability: N/A — no operations, no long-running work
|
|
- [x] Automation: N/A — no queued/scheduled work
|
|
- [x] Data minimization: N/A — no logging changes
|
|
- [x] Badge semantics (BADGE-001): N/A — no badges
|
|
- [x] Filament UI Action Surface Contract: **Exemption** — no Filament Resource/Page/RelationManager created or modified. Only middleware navigation logic changes.
|
|
- [x] Filament UI UX-001 (Layout and IA): **Exemption** — no new screens, no layout changes. Only sidebar context selection is fixed.
|
|
|
|
**Post-Phase-1 re-check**: All gates still pass. No data model, no new UI surfaces, no Graph calls.
|
|
|
|
## Project Structure
|
|
|
|
### Documentation (this feature)
|
|
|
|
```text
|
|
specs/106-required-permissions-sidebar-context/
|
|
├── plan.md # This file
|
|
├── research.md # Phase 0 output — middleware analysis
|
|
├── data-model.md # Phase 1 output — state transition documentation
|
|
├── quickstart.md # Phase 1 output — implementation steps
|
|
└── tasks.md # Phase 2 output (created by /speckit.tasks)
|
|
```
|
|
|
|
### Source Code (repository root)
|
|
|
|
```text
|
|
app/Support/Middleware/
|
|
└── EnsureFilamentTenantSelected.php # Modified: add workspace-scoped allowlist
|
|
|
|
tests/Feature/RequiredPermissions/
|
|
└── RequiredPermissionsSidebarTest.php # New: sidebar context assertions
|
|
```
|
|
|
|
**Structure Decision**: Existing Laravel web application structure. No new directories needed. Changes are localized to the existing middleware file and a new test file in the existing test directory.
|
|
|
|
## Implementation Design
|
|
|
|
### Approach: Path-based Allowlist in Middleware
|
|
|
|
Follow the existing precedent set by `/admin/operations` and `/admin/operations/{run}` handling.
|
|
|
|
#### Change 1: Workspace-scoped page helper method
|
|
|
|
Add a private method to the middleware:
|
|
|
|
```php
|
|
private function isWorkspaceScopedPageWithTenant(string $path): bool
|
|
{
|
|
return preg_match('#^/admin/tenants/[^/]+/required-permissions$#', $path) === 1;
|
|
}
|
|
```
|
|
|
|
#### Change 2: Livewire `/livewire/update` referer check
|
|
|
|
In the existing `/livewire/update` block (after the operations check), add:
|
|
|
|
```php
|
|
if (preg_match('#^/admin/tenants/[^/]+/required-permissions$#', $refererPath) === 1) {
|
|
$this->configureNavigationForRequest($panel);
|
|
return $next($request);
|
|
}
|
|
```
|
|
|
|
#### Change 3: Skip `setTenant()` for workspace-scoped pages
|
|
|
|
In the `$tenantParameter !== null` block, **after** all 8 authorization checks pass but **before** `Filament::setTenant($tenant, true)`:
|
|
|
|
```php
|
|
if ($this->isWorkspaceScopedPageWithTenant($path)) {
|
|
$this->configureNavigationForRequest($panel);
|
|
return $next($request);
|
|
}
|
|
```
|
|
|
|
This preserves:
|
|
- All authorization (workspace membership, tenant access, deny-as-not-found)
|
|
- Workspace sidebar rendering (tenant is null -> workspace nav)
|
|
|
|
This skips:
|
|
- `Filament::setTenant()` (no tenant sidebar)
|
|
- `rememberLastTenantId()` (no session side-effect)
|
|
|
|
**FR-005 clarification (context bar / tenant name):** Because `Filament::setTenant()` is skipped, `Filament::getTenant()` will return `null`. The context bar heading is **not** sourced from Filament global tenant state — the `TenantRequiredPermissions` page resolves `$scopedTenant` directly from the `{tenant}` route parameter via `resolveScopedTenant()` and passes it to the view. The page heading (`$this->heading`) is set from `$scopedTenant->display_name`. Therefore, skipping `setTenant()` does NOT affect the tenant name display. T007 MUST verify this by asserting the tenant name appears in the response.
|
|
|
|
### Testing Plan
|
|
|
|
| Test | Type | What it asserts |
|
|
|---|---|---|
|
|
| Required Permissions shows workspace nav items | Feature | Response contains "Operations", "Manage workspaces" nav items |
|
|
| Required Permissions does not show tenant nav items | Feature | Response does not contain "Inventory", "Backups" nav items |
|
|
| Other `{tenant}` pages still get tenant sidebar | Feature | e.g., ProviderConnection page has tenant nav |
|
|
| Existing access tests still pass | Feature | No regression in `RequiredPermissionsAccessTest` |
|
|
| Existing filter tests still pass | Feature | No regression in `RequiredPermissionsFiltersTest` |
|
|
|
|
### Agent Output Contract (Filament v5 Blueprint section 15)
|
|
|
|
1. **Livewire v4.0+ compliance**: Yes — no Livewire components created/modified; middleware is plain PHP.
|
|
2. **Provider registration**: N/A — no providers changed.
|
|
3. **Global search**: N/A — no resources added; Required Permissions page is not globally searchable.
|
|
4. **Destructive actions**: N/A — no actions added.
|
|
5. **Asset strategy**: N/A — no assets added.
|
|
6. **Testing plan**: One new test file covering sidebar context; existing test files provide regression coverage.
|
|
|
|
## Complexity Tracking
|
|
|
|
No constitution violations. No complexity justifications needed.
|