ahmido
3490fb9e2c
Summary
Implements Spec 067 “RBAC Troubleshooting & Tenant UI Bugfix Pack v1” for the tenant admin plane (/admin) with strict RBAC UX semantics:
Non-member tenant scope ⇒ 404 (deny-as-not-found)
Member lacking capability ⇒ 403 server-side, while the UI stays visible-but-disabled with standardized tooltips
What changed
Tenant view header actions now use centralized UI enforcement (no “normal click → error page” for readonly members).
Archived tenants remain resolvable in tenant-scoped routes for entitled members; an “Archived” banner is shown.
Adds tenant-scoped diagnostics page (/admin/t/{tenant}/diagnostics) with safe repair actions (confirmation + authorization + audit log).
Adds/updates targeted Pest tests to lock the 404 vs 403 semantics and action UX.
Implementation notes
Livewire v4.0+ compliance: Uses Filament v5 + Livewire v4 conventions; widget Blade views render a single root element.
Provider registration: Laravel 11+ providers stay in providers.php (no changes required).
Global search: No global search behavior/resources changed in this PR.
Destructive actions:
Tenant archive/restore/force delete and diagnostics repairs execute via ->action(...) and include ->requiresConfirmation().
Server-side authorization is enforced (non-members 404, insufficient capability 403).
Assets: No new assets. No change to php artisan filament:assets expectations.
Tests
Ran:
vendor/bin/sail bin pint --dirty
vendor/bin/sail artisan test --compact (focused files for Spec 067)
Co-authored-by: Ahmed Darrazi <ahmeddarrazi@MacBookPro.fritz.box>
Reviewed-on: #84
120 lines
6.0 KiB
Markdown
120 lines
6.0 KiB
Markdown
# 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).
|