ahmido
33a2b1a242
## Summary
Fixes the sidebar context bug where navigating to the **Required Permissions** page (`/admin/tenants/{id}/required-permissions`) would switch the sidebar from workspace navigation to tenant-scoped navigation, confusing users.
## Problem
The `EnsureFilamentTenantSelected` middleware detected a tenant ID in the URL and called `setTenant()`, which switched the entire sidebar to tenant-scoped navigation. The Required Permissions page is logically a **workspace-level** page that happens to reference a tenant — it should keep showing workspace nav.
## Changes
### Middleware (`EnsureFilamentTenantSelected.php`)
- **`isWorkspaceScopedPageWithTenant()`** — new private helper that detects workspace-scoped pages containing a tenant parameter via regex
- **Livewire referer bypass** — checks if a Livewire request originates from a workspace-scoped page and preserves workspace nav
- **`setTenant()` bypass** — skips tenant activation and `rememberLastTenantId()` for workspace-scoped pages
### Tests
- **`RequiredPermissionsSidebarTest.php`** (NEW) — 7 tests covering:
- Workspace nav visible on required-permissions page
- Tenant nav absent on required-permissions page
- Direct URL access preserves workspace nav
- 404 for non-member tenants
- 404 for tenants without entitlement
- Tenant pages still show tenant sidebar (regression guard)
- Scoped tenant resolves correctly on tenant pages
### Pre-existing test fixes
- **`RequiredPermissionsEmptyStateTest`** — fixed URL assertion (dynamic `TenantResource::getUrl()` instead of hardcoded `/admin/onboarding`)
- **`RequiredPermissionsLinksTest`** — fixed URL assertion + multiline HTML `data-testid` assertion
- **`RequiredPermissionsFiltersTest`** — fixed `entra_permissions` config leak from branch 105
## Test Results
| Suite | Result |
|-------|--------|
| RequiredPermissions (26 tests) | **26 pass** (73 assertions) |
| Full regression (1571 tests) | **1562 pass**, 2 fail (pre-existing OpsUx), 7 skipped |
The 2 failures are pre-existing in `OpsUx/OperationCatalogCoverageTest` and `OpsUx/OperationSummaryKeysSpecTest` — unrelated to this feature.
## Spec Artifacts
- `specs/106-required-permissions-sidebar-context/plan.md`
- `specs/106-required-permissions-sidebar-context/tasks.md`
- `specs/106-required-permissions-sidebar-context/research.md`
- `specs/106-required-permissions-sidebar-context/data-model.md`
- `specs/106-required-permissions-sidebar-context/quickstart.md`
Co-authored-by: Ahmed Darrazi <ahmed.darrazi@live.de>
Reviewed-on: #129
72 lines
5.2 KiB
Markdown
72 lines
5.2 KiB
Markdown
# Research: 106 — Required Permissions Sidebar Context Fix
|
||
|
||
**Date**: 2026-02-22 | **Branch**: `106-required-permissions-sidebar-context`
|
||
|
||
## RQ-001: How does `EnsureFilamentTenantSelected` currently handle workspace-scoped pages with `{tenant}` params?
|
||
|
||
**Finding**: The middleware at `app/Support/Middleware/EnsureFilamentTenantSelected.php` follows this flow:
|
||
|
||
1. **Livewire `/livewire/update`** — checks referer for known workspace-scoped patterns (currently only `/admin/operations/{run}`). If matched, calls `configureNavigationForRequest()` without setting tenant and returns early.
|
||
2. **`/admin/operations/{run}`** — early return with workspace nav (no tenant set).
|
||
3. **`/admin/operations`** — early return with workspace nav (no tenant set).
|
||
4. **Route has `{tenant}` param** (line ~74–131) — resolves tenant, validates workspace membership + tenant access, then **always** calls `Filament::setTenant($tenant, true)` and `configureNavigationForRequest()`. This is the root cause: `required-permissions` passes through here and gets tenant sidebar.
|
||
5. **Allowlisted workspace-scoped paths** (line ~133–141) — string prefix checks for `/admin/w/`, `/admin/workspaces`, `/admin/operations`, plus an exact-match array. These get workspace nav without tenant.
|
||
|
||
**Root cause**: Step 4 has no exclusion mechanism. Any page with a `{tenant}` route parameter — including `TenantRequiredPermissions` — gets `Filament::setTenant()` called, triggering tenant sidebar.
|
||
|
||
**Decision**: Add a path-based allowlist check **inside** step 4, before `Filament::setTenant()` is called. When the path matches a workspace-scoped page that uses `{tenant}` for data scoping (not sidebar scoping), skip `setTenant()` but still perform all authorization checks.
|
||
|
||
**Alternatives considered**:
|
||
- *Page-level override (e.g., `$isWorkspaceScoped` property)*: Would require Filament page resolution in middleware (too expensive / wrong layer).
|
||
- *Separate middleware for Required Permissions route*: Duplicates authorization logic; fragile.
|
||
- *Clear tenant after middleware runs*: Race condition with `configureNavigationForRequest()` which already ran.
|
||
|
||
## RQ-002: What pages currently use the allowlist pattern?
|
||
|
||
**Finding**: The middleware already has two precedent patterns for workspace-scoped exemptions:
|
||
|
||
| Pattern | Type | Location in middleware |
|
||
|---|---|---|
|
||
| `/admin/operations` | Exact match | Line ~67 |
|
||
| `/admin/operations/{run}` | Regex | Line ~61 |
|
||
| `/livewire/update` + referer `/admin/operations/{run}` | Referer check | Line ~49–56 |
|
||
| `/admin/w/`, `/admin/workspaces`, etc. | Prefix/exact | Line ~133–141 |
|
||
|
||
**Decision**: Follow the same pattern (regex match + referer check for Livewire) for `/admin/tenants/{tenant}/required-permissions`.
|
||
|
||
## RQ-003: How does `configureNavigationForRequest` determine sidebar?
|
||
|
||
**Finding** (lines 152–247):
|
||
- If `Filament::getTenant()` is filled → `$panel->navigation(true)` (default tenant nav).
|
||
- If tenant is null → builds a custom `NavigationBuilder` with workspace-scoped items only (Manage workspaces, Operations, Alert targets/rules/deliveries, Alerts, Audit Log).
|
||
|
||
**Decision**: No changes needed to `configureNavigationForRequest`. The fix is upstream: don't call `setTenant()` for workspace-scoped pages, so the method naturally shows workspace nav.
|
||
|
||
## RQ-004: What authorization must still run for exempted pages?
|
||
|
||
**Finding**: The `{tenant}` param resolution block (lines 74–131) performs:
|
||
1. Null user check → early return
|
||
2. `HasTenants` interface check → abort 404
|
||
3. Tenant resolution by `external_id` (including soft-deleted) → abort 404
|
||
4. Workspace ID null check → abort 404
|
||
5. Tenant workspace ownership check → abort 404
|
||
6. Workspace existence check → abort 404
|
||
7. User workspace membership check → abort 404
|
||
8. `canAccessTenant()` → abort 404
|
||
|
||
**Decision**: All 8 checks must still run. Only `Filament::setTenant()` and `app(WorkspaceContext::class)->rememberLastTenantId()` are skipped for exempted workspace-scoped pages. The tenant is still resolved and authorized — it just isn't set as Filament's active tenant.
|
||
|
||
## RQ-005: Livewire update handling
|
||
|
||
**Finding**: For Livewire `/livewire/update` requests, the middleware inspects the `Referer` header to determine context. Currently only `/admin/operations/{run}` is checked.
|
||
|
||
**Decision**: Add `/admin/tenants/{tenant}/required-permissions` to the referer pattern check. The regex `#^/admin/tenants/[^/]+/required-permissions$#` will match referers from the Required Permissions page, ensuring Livewire re-renders (filter changes) also get workspace sidebar.
|
||
|
||
## RQ-006: Impact on `rememberLastTenantId()`
|
||
|
||
**Finding**: When `Filament::setTenant()` is called in step 4, `rememberLastTenantId()` stores the tenant in session. This enables "return to last tenant" convenience.
|
||
|
||
**Decision**: For workspace-scoped exempted pages, `rememberLastTenantId()` should NOT be called. Visiting Required Permissions should not change the user's "last active tenant" context. The tenant is used only for data display on that page.
|
||
|
||
**Rationale**: If we called `rememberLastTenantId()`, navigating to Required Permissions would change the user's default tenant context globally — unexpected side effect for a workspace-level page.
|