ahmido
7217559e5a
Ziel: Spec/Plan/Tasks für “RBAC UI Enforcement Helper v2” (suite-wide, mixed visibility, record-scoped tenant) bereitstellen, damit die anschließende Implementierung sauber reviewbar ist.
Enthält
Feature-Spec inkl. RBAC-UX Contract (Non-member 404/hidden, member-no-cap disabled + Tooltip, member-with-cap enabled).
Implementation Plan + Research/Decisions.
Contracts:
UiEnforcement v2 (mixed visibility composition, tenant resolvers, bulk preflight).
Guardrails (CI-failing allowlist guard gegen ad-hoc Filament auth patterns).
Data-model/Quickstart/Tasks inkl. “Definition of Done”.
Review-Fokus
Scope: Tenant plane only (/admin/t/{tenant}), Platform plane out of scope.
Bulk semantics: authorization-only all-or-nothing; eligibility separat mit Feedback.
preserveVisibility() nur tenant-scoped, verboten für record-scoped/cross-tenant.
Standard tooltip copy: “Insufficient permission — ask a tenant Owner.”
Keine Code-Änderungen
PR ist spec-only (keine Runtime-Änderungen).
Co-authored-by: Ahmed Darrazi <ahmeddarrazi@MacBookPro.fritz.box>
Reviewed-on: #82
90 lines
4.6 KiB
Markdown
90 lines
4.6 KiB
Markdown
# Research: RBAC UI Enforcement Helper v2 (Suite-wide, Mixed Visibility, Record-Scoped)
|
||
|
||
**Branch**: `066-rbac-ui-enforcement-helper-v2`
|
||
**Date**: 2026-01-30
|
||
**Spec**: `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/spec.md`
|
||
**Plan**: `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/plan.md`
|
||
|
||
## Decisions
|
||
|
||
### D-001 — Mixed visibility needs explicit composition primitives
|
||
|
||
**Decision**: `UiEnforcement` supports mixed visibility via explicit composition methods:
|
||
- `preserveVisibility()` is allowed only on tenant-scoped surfaces where routing already denies non-members (404).
|
||
- Record-scoped / cross-tenant lists must use `andVisibleWhen(...)` / `andHiddenWhen(...)` (or default RBAC visibility), so non-members cannot discover actions.
|
||
|
||
**Rationale**: Filament does not provide a safe way to introspect/merge existing `->visible()` / `->hidden()` closures. Explicit composition avoids brittle “read back and wrap” behavior and preserves the non-member “cannot discover” contract.
|
||
|
||
**Alternatives considered**:
|
||
- Allow `preserveVisibility()` everywhere: rejected because it can leak action discoverability on record-scoped lists.
|
||
- Implicitly merge existing closures (helper tries to wrap previous `visible/hidden`): rejected as fragile and hard to review/test.
|
||
|
||
---
|
||
|
||
### D-002 — Tenant resolution must be explicit and per-surface
|
||
|
||
**Decision**: `UiEnforcement` supports tenant resolution modes:
|
||
- `tenantFromFilament()` (default) for tenant-scoped pages/actions.
|
||
- `tenantFromRecord()` for record-scoped resources where record == tenant.
|
||
- `tenantFrom(callable)` for record → tenant mapping.
|
||
|
||
**Rationale**: v2 targets record-scoped surfaces where `Filament::getTenant()` is intentionally null; the helper must not hard-depend on Filament tenancy context.
|
||
|
||
**Alternatives considered**:
|
||
- Require `Filament::getTenant()` always: rejected because it blocks record-scoped resources by design.
|
||
|
||
---
|
||
|
||
### D-003 — Bulk preflight is authorization-only by default
|
||
|
||
**Decision**: Default bulk “all-or-nothing” preflight applies to authorization only:
|
||
- If any selected record is unauthorized, the bulk action is disabled for members and cannot execute.
|
||
- Business eligibility (e.g., inactive/archived) remains separate and may be skipped with deterministic feedback.
|
||
|
||
**Rationale**: Mixing “eligibility” into authorization preflight tends to create confusing UX for operators and makes migrations riskier (unexpected disablement). Keeping them separate preserves predictable security semantics and reduces incidental coupling.
|
||
|
||
**Alternatives considered**:
|
||
- Disable bulk actions when any record is ineligible: rejected as overly strict and likely to regress existing workflows.
|
||
- Partial execution for authorized subset: rejected (v2 contract requires all-or-nothing for authorization).
|
||
|
||
---
|
||
|
||
### D-004 — Record-scoped non-member denial is 404
|
||
|
||
**Decision**: For record-scoped tenant pages/actions, non-members are denied as not found (404) on direct access/execution attempts.
|
||
|
||
**Rationale**: Prevents tenant/record enumeration and aligns with the constitution’s deny-as-not-found rule for non-members.
|
||
|
||
**Alternatives considered**:
|
||
- Return 403 for non-members: rejected as it can leak record existence.
|
||
|
||
---
|
||
|
||
### D-005 — Standard disabled tooltip copy
|
||
|
||
**Decision**: Default `UiTooltips` message for member-without-capability is:
|
||
> “Insufficient permission — ask a tenant Owner.”
|
||
|
||
**Rationale**: Clear, non-leaky, and gives the user the correct next step without exposing capability names or internal policy details.
|
||
|
||
**Alternatives considered**:
|
||
- “Insufficient permission.”: rejected as too vague for operators.
|
||
- Include capability identifier in tooltip: rejected as noisy and may leak internal policy shape; can be added selectively where appropriate.
|
||
|
||
---
|
||
|
||
### D-006 — Tests are the contract (per surface)
|
||
|
||
**Decision**: Each migrated surface gets at least one focused integration test covering:
|
||
- non-member: hidden + cannot execute (deny-as-not-found/404 where reachable),
|
||
- member without capability: visible but disabled + standard tooltip + cannot execute,
|
||
- member with capability: enabled executes (and destructive actions require confirmation).
|
||
|
||
Additionally, DB-only render is enforced for migrated pages/actions using `Http::preventStrayRequests()`.
|
||
|
||
**Rationale**: The helper exists to standardize behavior; tests must assert the user-facing contract and prevent drift.
|
||
|
||
**Alternatives considered**:
|
||
- Rely on unit tests for helper only: rejected because Filament execution semantics and closures are integration-sensitive.
|
||
|