diff --git a/specs/066-rbac-ui-enforcement-helper/checklists/requirements.md b/specs/066-rbac-ui-enforcement-helper/checklists/requirements.md index 91ef52d..871186f 100644 --- a/specs/066-rbac-ui-enforcement-helper/checklists/requirements.md +++ b/specs/066-rbac-ui-enforcement-helper/checklists/requirements.md @@ -1,8 +1,8 @@ -# Specification Quality Checklist: RBAC UI Enforcement Helper v1 +# Specification Quality Checklist: RBAC UI Enforcement Helper v2 (Suite-wide, Mixed Visibility, Record-Scoped) **Purpose**: Validate specification completeness and quality before proceeding to planning -**Created**: 2026-01-28 -**Feature**: [specs/066-rbac-ui-enforcement-helper/spec.md](../spec.md) +**Created**: 2026-01-30 +**Feature**: [spec.md](../spec.md) ## Content Quality @@ -31,4 +31,6 @@ ## Feature Readiness ## Notes -- No blockers found in this iteration. +- Validation run: 2026-01-30 +- Clarifications captured under `### Session 2026-01-30` and integrated into requirements/design sections. +- All checks passed; spec is ready for `/speckit.plan`. diff --git a/specs/066-rbac-ui-enforcement-helper/contracts/guardrails.md b/specs/066-rbac-ui-enforcement-helper/contracts/guardrails.md new file mode 100644 index 0000000..c61df9f --- /dev/null +++ b/specs/066-rbac-ui-enforcement-helper/contracts/guardrails.md @@ -0,0 +1,31 @@ +# Contract: RBAC Guardrails for Filament (v2) + +**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` + +## Goal + +Prevent re-introducing ad-hoc RBAC authorization patterns inside `app/Filament/**` by keeping a CI-failing, allowlist-driven guard. + +## Guard contract + +- The guard is CI-failing for new violations (no “warnings only” mode). +- Legacy violations are allowed only via an explicit allowlist. +- Every v2 migration batch removes allowlist entries for the migrated files. + +## Forbidden patterns (examples) + +The exact pattern list is defined in the guard test, but v2 migrations must remove ad-hoc patterns such as: + +- `Gate::...` checks inside Filament resources/pages when the enforcement helper is intended. +- `abort_*` calls used as the primary UI enforcement mechanism inside Filament resources/pages. +- Raw policy ability string literals (instead of `Capabilities::*` constants). +- Custom RBAC helper patterns that bypass `UiEnforcement`. + +## Allowed patterns + +- Capability checks via `UiEnforcement` and `Capabilities::*` registry constants. +- Defense-in-depth server-side authorization inside action handlers (403 for members without capability; 404 for non-members where reachable). +- Filament action UX patterns: `->requiresConfirmation()` for destructive/high-impact actions. + diff --git a/specs/066-rbac-ui-enforcement-helper/contracts/ui-enforcement.md b/specs/066-rbac-ui-enforcement-helper/contracts/ui-enforcement.md new file mode 100644 index 0000000..bba31b8 --- /dev/null +++ b/specs/066-rbac-ui-enforcement-helper/contracts/ui-enforcement.md @@ -0,0 +1,83 @@ +# Contract: UiEnforcement v2 (RBAC UI Enforcement Helper) + +**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` + +## Scope + +- Tenant plane only (`/admin/t/{tenant}`). +- Platform plane (`/system`) is out of scope for v2. + +## RBAC-UX Contract (summary) + +1) **Non-member** +- Actions are hidden and cannot execute. +- Direct access/execution attempts must deny-as-not-found (404) where reachable. + +2) **Member without capability** +- Action is visible but disabled. +- Disabled tooltip is standardized via `UiTooltips` (default copy is defined in the feature spec). +- Disabled action cannot execute (Filament no-op is acceptable). + +3) **Member with capability** +- Action is enabled and executes. +- Destructive/high-impact actions require confirmation (`->requiresConfirmation()`). + +## Mixed visibility composition (business + RBAC) + +### `preserveVisibility()` + +**Contract** +- The helper MUST NOT write `->visible(...)` or `->hidden(...)`. +- The helper MAY still set disabled state + tooltip + confirmation + server-side guard. + +**Restriction** +- Allowed only on tenant-scoped surfaces where routing already denies non-members (404). +- Forbidden for record-scoped / cross-tenant lists (must not leak discoverability). + +### `andVisibleWhen(callable $businessVisible)` + +**Contract** +- The helper combines business visibility AND RBAC visibility. +- If business visibility is false, the action is not visible (even for authorized members). + +### `andHiddenWhen(callable $businessHidden)` + +**Contract** +- The helper combines business-hidden OR RBAC-hidden semantics. + +## Tenant context providers + +### `tenantFromFilament()` (default) + +- Uses Filament tenant context (`Filament::getTenant()` or equivalent). +- Intended for tenant-scoped pages/actions. + +### `tenantFromRecord()` + +- Treats `$record` as the tenant. +- Intended for record-scoped surfaces like `TenantResource` row actions. + +### `tenantFrom(callable $resolver)` + +- Maps a record (or selection) to a tenant instance. +- Intended for “record → tenant” mapping surfaces. + +## Bulk preflight authorization + +### Default behavior (v2) + +- All-or-nothing **authorization**: if any selected record is unauthorized, the bulk action is disabled and cannot execute. +- Business eligibility is handled separately by the action implementation (e.g., skip inactive/archived records with clear feedback). + +### Preflight hooks + +- Custom: `preflightSelection(callable $preflight)` +- Built-ins: + - `preflightByTenantMembership()` + - `preflightByCapability()` + +**Performance** +- Preflight should use set-based DB queries where feasible; avoid N+1 membership checks. + diff --git a/specs/066-rbac-ui-enforcement-helper/data-model.md b/specs/066-rbac-ui-enforcement-helper/data-model.md new file mode 100644 index 0000000..4b27238 --- /dev/null +++ b/specs/066-rbac-ui-enforcement-helper/data-model.md @@ -0,0 +1,75 @@ +# Data Model: RBAC UI Enforcement Helper v2 + +**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` + +## Overview + +This feature does not introduce new database tables. It defines contracts and behaviors that compose existing tenant-plane RBAC (membership + capability gates) into consistent Filament UI states across tenant-scoped and record-scoped surfaces. + +## Entities (existing) + +### Tenant + +The isolation boundary for all tenant-plane actions. + +**Identity** +- `tenants.id` (PK) +- A user is a “member” if `user->canAccessTenant($tenant)` is true. + +### User + +Authenticated tenant-plane actor. + +**Key behaviors** +- Must support membership checks (`canAccessTenant($tenant)`). +- Capability checks are performed via Gates/Policies using `Capabilities::*` identifiers. + +### TenantMembership + +Joins a User to a Tenant with a role mapped to capabilities (Feature 065). + +**Key behaviors** +- Role → capabilities mapping is centralized and deterministic. +- Requests should avoid repeated membership DB queries (request-scope cache). + +### Capability (registry) + +Canonical capability keys live in `app/Support/Auth/Capabilities.php` and must be referenced via constants (`Capabilities::*`). + +## Entities (conceptual, not persisted) + +### UiEnforcementContext + +Context used to evaluate membership/capability for an action. + +**Fields** +- `user`: current authenticated user (or null) +- `tenantResolver`: one of: + - `tenantFromFilament` (tenant-scoped) + - `tenantFromRecord` (record == tenant) + - `tenantFrom(callable)` (record → tenant mapping) +- `record`: optional record for table/record actions +- `capability`: required capability identifier (from `Capabilities::*`) + +### UiEnforcementDecision + +The derived UI behavior applied to a Filament action. + +**Fields** +- `isVisible`: boolean (non-members must not be able to discover actions) +- `isEnabled`: boolean (members without capability see disabled action) +- `disabledTooltip`: string (standardized via `UiTooltips`) +- `requiresConfirmation`: boolean (destructive/high-impact actions) + +### BulkSelectionAuthorization (preflight) + +Derived authorization status for a bulk action selection. + +**Fields** +- `selectedIds`: list of record IDs +- `resolvedTenants`: list of tenant IDs derived via tenant resolver (record == tenant or mapping) +- `unauthorizedCount`: count of records failing authorization (determines all-or-nothing disable) +- `ineligibleCount`: optional count of business-ineligible records (may be skipped with deterministic feedback; does not affect authorization disable by default) + diff --git a/specs/066-rbac-ui-enforcement-helper/plan.md b/specs/066-rbac-ui-enforcement-helper/plan.md index 5715060..22beadd 100644 --- a/specs/066-rbac-ui-enforcement-helper/plan.md +++ b/specs/066-rbac-ui-enforcement-helper/plan.md @@ -1,58 +1,51 @@ -# Implementation Plan: RBAC UI Enforcement Helper v1 +# Implementation Plan: RBAC UI Enforcement Helper v2 (Suite-wide, Mixed Visibility, Record-Scoped) -**Branch**: `066-rbac-ui-enforcement-helper` | **Date**: 2026-01-28 | **Spec**: [spec.md](./spec.md) -**Input**: Feature specification from `/specs/066-rbac-ui-enforcement-helper/spec.md` +**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` +**Input**: Feature specification from `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/spec.md` + +**Note**: This template is filled in by the `/speckit.plan` command. See `.specify/scripts/` for helper scripts. ## Summary -Provide a single, centrally maintained enforcement helper (`UiEnforcement`) that codifies the RBAC-UX constitution rules for tenant-scoped Filament actions: - -- Non-member → 404 (deny-as-not-found), hidden in UI -- Member without capability → 403 on execution, visible-but-disabled in UI with standard tooltip -- Member with capability → enabled -- Destructive actions → `requiresConfirmation()` + clear warning - -The helper wraps/augments Filament Actions (header, table row, bulk) to provide default UI + server-side enforcement, and ships with regression tests + a CI-failing guard against ad-hoc authorization patterns. +- Extend the RBAC UI enforcement suite (Feature 066 v1) with explicit primitives for: + - mixed visibility composition (business visibility + RBAC visibility), + - record-scoped tenant resolution (record == tenant, or record → tenant mapping), + - deterministic bulk preflight authorization with all-or-nothing semantics (authorization-only by default). +- Migrate the highest-risk remaining Filament surfaces (BackupSet/RestoreRun mixed visibility, TenantResource record-scoped actions), plus Tier 2 allowlist “easy wins”, shrinking the CI-failing guard allowlist. +- Add focused integration tests per migrated surface to assert the RBAC-UX contract (hidden/disabled + tooltip + non-execution), and keep render DB-only (no outbound HTTP). ## Technical Context + + **Language/Version**: PHP 8.4 (Laravel 12) -**Primary Dependencies**: Filament v5, Livewire v4 -**Storage**: PostgreSQL (existing tables — no new tables) -**Testing**: Pest v4 (Feature + Unit tests) -**Target Platform**: Docker / Sail local, Dokploy VPS (Linux) -**Project Type**: Web / Monolith (backend + Filament admin) -**Performance Goals**: No additional DB queries beyond request-scope cached membership (FR-012) -**Constraints**: DB-only at render time (FR-013); no outbound HTTP -**Scale/Scope**: ~40+ tenant-scoped action surfaces; v1 migrates 3–6 exemplar surfaces +**Primary Dependencies**: Laravel 12 + Filament v5 + Livewire v4 +**Storage**: PostgreSQL (Sail) +**Testing**: Pest (PHPUnit) +**Target Platform**: Web application (Sail/Docker locally; container deploy via Dokploy) +**Project Type**: web +**Performance Goals**: Membership/capability evaluation is O(1) per request after initial load; bulk preflight avoids N+1 (set-based where feasible) +**Constraints**: DB-only render/hydration; no outbound HTTP during render; tenant isolation; hidden/disabled actions must not execute; destructive actions require confirmation +**Scale/Scope**: Suite-wide helper primitives + targeted migrations (Tier 1 + Tier 2 hitlist) rather than a full Filament sweep ## Constitution Check *GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* -| Principle | Status | Notes | -|-----------|--------|-------| -| Inventory-first | N/A | No Inventory changes | -| Read/write separation | ✔ | Helper enforces existing gates; no new writes | -| Graph contract path | N/A | No Graph calls | -| Deterministic capabilities | ✔ | Uses existing `Capabilities` registry | -| RBAC-UX planes | ✔ | Tenant-plane only; cross-plane logic untouched | -| Tenant isolation | ✔ | 404 for non-members; capability check requires membership first | -| Run observability | N/A | No long-running work; helper is request-scope only | -| Data minimization | ✔ | No additional logging beyond existing deny logs | -| Badge semantics | N/A | No badge changes | - -## Existing RBAC Primitives (Research) - -| Component | Location | Purpose | -|-----------|----------|---------| -| `Capabilities` | `app/Support/Auth/Capabilities.php` | Canonical tenant capability registry (constants) | -| `PlatformCapabilities` | `app/Support/Auth/PlatformCapabilities.php` | Platform-plane capabilities | -| `RoleCapabilityMap` | `app/Services/Auth/RoleCapabilityMap.php` | Role → capabilities mapping | -| `CapabilityResolver` | `app/Services/Auth/CapabilityResolver.php` | Request-scope cached role/capability resolution | -| `User::canAccessTenant()` | `app/Models/User.php:123` | Membership check | -| `AuthServiceProvider` | `app/Providers/AuthServiceProvider.php` | Registers Gates for all capabilities | -| Existing ad-hoc patterns | `app/Filament/**` | 50+ `->visible(fn ...)` / `->disabled(fn ...)` calls — target for migration | +- Inventory-first, snapshots-second: N/A (helper only; no inventory semantics changed). +- Read/write separation: PASS (no new write flows introduced; existing destructive flows remain confirmation-gated and server-authorized). +- Single contract path to Graph: PASS (helper + UI rendering remain DB-only; no Graph calls introduced). +- Deterministic capabilities: PASS (migrated surfaces reference `Capabilities::*` only; no ad-hoc ability strings). +- RBAC Standard: PASS (tenant plane only; platform plane out of scope; non-member is deny-as-not-found/404). +- Tenant isolation: PASS (record-scoped tenant resolution evaluates membership per tenant record; non-members receive 404 on direct access). +- Operations / run observability standard: N/A (no changes to OperationRun orchestration; only UI authorization enforcement). +- Automation (locks + idempotency): N/A (no new queued ops introduced). +- Data minimization & safe logging: PASS (no new persisted payloads; enforcement must not log secrets/PII). +- Badge semantics (BADGE-001): N/A. ## Project Structure @@ -60,129 +53,91 @@ ### Documentation (this feature) ```text specs/066-rbac-ui-enforcement-helper/ -├── spec.md # Feature specification -├── plan.md # This file -├── research.md # (no separate file needed — inline above) -├── data-model.md # (no schema changes) -├── quickstart.md # Adoption guide -├── checklists/ -│ └── requirements.md # Spec quality checklist -└── tasks.md # Phase 2 output (/speckit.tasks) +├── 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/ -├── Support/ -│ └── Rbac/ -│ ├── UiEnforcement.php # Central facade/builder -│ ├── TenantAccessContext.php # DTO: tenant, user, isMember, capabilityCheck -│ └── UiTooltips.php # Standardized tooltip strings -├── Services/Auth/ -│ ├── CapabilityResolver.php # (existing, reused) -│ └── RoleCapabilityMap.php # (existing, reused) ├── Filament/ -│ └── Resources/... # 3–6 exemplar migrations +│ └── Resources/ +│ ├── BackupSetResource.php +│ ├── RestoreRunResource.php +│ ├── TenantResource.php +│ ├── InventoryItemResource.php +│ ├── InventorySyncRunResource.php +│ ├── EntraGroupSyncRunResource.php +│ └── ProviderConnectionResource.php +├── Support/ +│ └── Auth/ +│ ├── UiEnforcement.php # expected from Feature 066 v1 (verify in implementation worktree) +│ └── UiTooltips.php # expected from Feature 066 v1 (verify in implementation worktree) +└── Support/Auth/Capabilities.php tests/ ├── Feature/ -│ └── Rbac/ -│ └── UiEnforcementTest.php # Integration tests +│ ├── Filament/ │ └── Guards/ -│ └── NoAdHocFilamentAuthPatternsTest.php # CI-failing guard (file-scan) -├── Unit/ -│ └── Support/Rbac/ -│ └── UiEnforcementTest.php # Unit tests - +└── Unit/ + └── Auth/ ``` -**Structure Decision**: All new code lives in `app/Support/Rbac/` (helper) + tests; no new models/tables required. - -## Key Design Decisions - -### UiEnforcement API (FR-001) - -```php -use App\Support\Rbac\UiEnforcement; - -// Basic usage -UiEnforcement::forAction($action) - ->requireMembership() // default: true - ->requireCapability(Capabilities::PROVIDER_MANAGE) - ->destructive() // optional: adds confirmation - ->apply(); - -// Table/row action (receives record or record-accessor closure) -UiEnforcement::forTableAction(Action $action, Model|Closure $record) - ->requireCapability(Capabilities::TENANT_DELETE) - ->destructive() - ->apply(); - -// Mixed visibility support (keep business visibility, add RBAC visibility) -UiEnforcement::forAction($action) - ->preserveVisibility() - ->requireCapability(Capabilities::TENANT_MANAGE) - ->apply(); - -// Bulk action (all-or-nothing) -UiEnforcement::forBulkAction($action) - ->requireCapability(Capabilities::TENANT_MANAGE) - ->apply(); -``` - -Internally: -1. Resolves current tenant + user via `Filament::getTenant()` + `auth()->user()` -2. Checks membership via `CapabilityResolver` (request-scope cached) -3. Sets `->hidden()` for non-members (FR-002a) -4. Sets `->disabled()` + `->tooltip()` for members without capability (FR-004) -5. Wraps handler with server-side guard (FR-005): `abort(404)` / `abort(403)` - -### Tooltip Copy (FR-008) - -```php -class UiTooltips -{ - public const INSUFFICIENT_PERMISSION = 'You don\'t have permission to do this. Ask a tenant admin.'; - public const DESTRUCTIVE_CONFIRM_TITLE = 'Are you sure?'; - public const DESTRUCTIVE_CONFIRM_DESCRIPTION = 'This action cannot be undone.'; -} -``` - -### Destructive Confirmation (FR-007) - -`->destructive()` calls: -- `$action->requiresConfirmation()` -- `$action->modalHeading(UiTooltips::DESTRUCTIVE_CONFIRM_TITLE)` -- `$action->modalDescription(UiTooltips::DESTRUCTIVE_CONFIRM_DESCRIPTION)` - -### All-or-nothing Bulk (FR-010a) - -Before rendering, bulk action checks all selected records. If any record fails capability check for the member, action is disabled. - -### Guardrail (FR-011) - -`tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php` scans `app/Filament/**` for forbidden patterns like: -- `Gate::allows(...)` / `Gate::denies(...)` -- `abort_if(...)` / `abort_unless(...)` - -It uses a legacy allowlist so CI fails only for **new** violations, and the allowlist should shrink as resources are migrated. - -## v1 Migration Targets (FR-009) - -| Surface | File | Current Pattern | Notes | -|---------|------|-----------------|-------| -| TenantResource table actions | `TenantResource.php` | Multiple `->visible(fn ...)` + `->disabled(fn ...)` | High-traffic, high-value | -| ProviderConnectionResource actions | `EditProviderConnection.php` | Multiple `canAccessTenant` + capability checks inline | Complex, good test case | -| BackupSetResource table actions | `BackupSetResource.php` | Many `->disabled(fn ...)` closures | Destructive actions | -| PolicyResource ListPolicies sync | `ListPolicies.php` | Inline checks | Good example | -| EntraGroupResource sync | `ListEntraGroups.php` | Inline checks | Good example | -| FindingResource actions | `FindingResource.php` | `->authorize(fn ...)` inline | Good example | +**Structure Decision**: Single Laravel web application with Filament resources. v2 work focuses on `app/Support/Auth` helper primitives, a small set of Filament resources/pages, and regression tests. ## Complexity Tracking -> No constitution violations. Complexity is low (helper + tests + 3–6 migrations). +No constitution violations required for this feature. -| Violation | Why Needed | Simpler Alternative Rejected Because | -|-----------|------------|-------------------------------------| -| — | — | — | +## Phase 0 — Research (output: `research.md`) + +See: `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/research.md` + +Goals: +- Confirm Filament v5 action/table semantics needed for enforcement (visibility/disabled/tooltips; bulk action lifecycle). +- Confirm the helper can compose business visibility deterministically without requiring “read back” of existing closures. +- Confirm bulk selection preflight can be implemented without N+1 membership checks (set-based queries where feasible). +- Verify the exact locations of the Feature 066 v1 helper + guard + allowlist in the implementation worktree (spec branch may not contain v1 code). + +## Phase 1 — Design & Contracts (outputs: `data-model.md`, `contracts/`, `quickstart.md`) + +See: +- `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/data-model.md` +- `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/contracts/` +- `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/quickstart.md` + +Design focus: +- Helper API: `preserveVisibility()` (tenant-scoped only), `andVisibleWhen(...)`, `andHiddenWhen(...)`. +- Tenant resolution modes: `tenantFromFilament()` (default), `tenantFromRecord()` (record == tenant), and `tenantFrom(callable)` for custom mapping. +- Bulk preflight: deterministic authorization-only all-or-nothing default; business eligibility handled separately with clear user feedback. +- Test strategy: per-surface Filament integration tests for hidden/disabled/tooltips and no-execution; DB-only rendering enforced via `Http::preventStrayRequests()`. + +## Phase 1 — Agent Context Update + +After Phase 1 artifacts are generated, update Codex context from the plan: + +- `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/.specify/scripts/bash/update-agent-context.sh codex` + +## Phase 2 — Implementation Outline (tasks created in `/speckit.tasks`) + +- Extend `UiEnforcement` helper primitives (mixed visibility composition + tenant resolvers + bulk preflight API). +- Migrate Tier 1 surfaces: + - BackupSet (resource + relation managers) for mixed visibility. + - RestoreRun (resource + actions) for mixed visibility. + - TenantResource row actions + bulk actions for record-scoped tenant enforcement and authorization-only bulk preflight. +- Migrate Tier 2 allowlist “easy wins” (Inventory + Entra group runs + ProviderConnection mixed visibility where present). +- Shrink the CI-failing allowlist: remove entries for every migrated file in the same batch. +- Add integration tests per migrated surface: + - 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 and executes; destructive actions require confirmation. +- Run quality gates per batch: `./vendor/bin/sail bin pint --dirty` + targeted Pest suites + guard tests. + +## Constitution Check (Post-Design) + +Re-check result: PASS. Design artifacts keep enforcement DB-only, tenant-plane only, capabilities-first, and preserve deny-as-not-found behavior for non-members (including record-scoped tenant surfaces). diff --git a/specs/066-rbac-ui-enforcement-helper/quickstart.md b/specs/066-rbac-ui-enforcement-helper/quickstart.md index d4128c9..8f5839b 100644 --- a/specs/066-rbac-ui-enforcement-helper/quickstart.md +++ b/specs/066-rbac-ui-enforcement-helper/quickstart.md @@ -1,262 +1,63 @@ -# Quickstart: UiEnforcement Helper +# Quickstart: RBAC UI Enforcement Helper v2 -> Adoption guide for developers adding RBAC enforcement to Filament actions. +**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` -## TL;DR +## Local setup -Replace ad-hoc `->visible(fn ...)` / `->disabled(fn ...)` closures with `UiEnforcement`. +1. Start the app stack: + - `./vendor/bin/sail up -d` +2. Install dependencies (if needed): + - `./vendor/bin/sail composer install` -```php -// ❌ Before (ad-hoc) -Action::make('sync') - ->visible(fn () => auth()->user()->can('provider:manage', Filament::getTenant())) - ->disabled(fn () => ! auth()->user()->can('provider:manage', Filament::getTenant())) - ->action(function () { - // no server-side guard - }); +## Recipes (once implemented) -// ✅ After (UiEnforcement) -UiEnforcement::forAction( - Action::make('sync') - ->action(fn () => $this->sync()) -) - ->requireCapability(Capabilities::PROVIDER_MANAGE) - ->apply(); -``` +> Scope: Tenant plane only (`/admin/t/{tenant}`). Platform plane (`/system`) is out of scope. -## When to Use +### Tenant-scoped action (default tenant from Filament) -| Scenario | Use UiEnforcement? | -|----------|-------------------| -| Tenant-scoped action (header, table, bulk) | ✅ Yes | -| Platform-scoped action (/system panel) | ❌ No (use Gate directly) | -| Read-only navigation link | ❌ No (use `->visible()` for nav items) | -| Destructive action (delete, detach, restore) | ✅ Yes, with `->destructive()` | +- Use `UiEnforcement` with the required `Capabilities::*` constant. +- Non-members should never see the action (routing already denies them). +- Members without capability see a disabled action + standard tooltip. -## API Reference +### Mixed visibility (business visibility + RBAC) -### Header / Page Actions +- If the surface already has business visibility rules, use composition: + - **Tenant-scoped**: you may use `preserveVisibility()` to keep existing visibility closures unchanged. + - **Record-scoped / cross-tenant lists**: do not use `preserveVisibility()`; instead use `andVisibleWhen(...)` / `andHiddenWhen(...)` so non-members cannot discover actions. -```php -use App\Support\Rbac\UiEnforcement; -use App\Support\Auth\Capabilities; +### Record-scoped tenant (record == tenant) -protected function getHeaderActions(): array -{ - return [ - UiEnforcement::forAction( - Action::make('createBackup') - ->action(fn () => $this->createBackup()) - ) - ->requireCapability(Capabilities::BACKUP_CREATE) - ->apply(), +- Configure enforcement with `tenantFromRecord()` so membership/capability checks are evaluated per row/record tenant. +- Non-members must be deny-as-not-found (404) on direct access/execution attempts. - UiEnforcement::forAction( - Action::make('deleteAllBackups') - ->action(fn () => $this->deleteAll()) - ) - ->requireCapability(Capabilities::BACKUP_MANAGE) - ->destructive() - ->apply(), - ]; -} -``` +### Bulk actions (authorization-only all-or-nothing) -### Table Row Actions +- Use bulk preflight to disable the action if any selected record is unauthorized. +- Keep business eligibility separate (e.g., skip inactive/archived records with deterministic feedback). -```php -public static function table(Table $table): Table -{ - return $table - ->columns([...]) - ->actions([ - UiEnforcement::forTableAction( - Action::make('restore') - ->action(fn (Policy $record) => $record->restore()), - fn () => $this->getRecord() // record accessor - ) - ->requireCapability(Capabilities::RESTORE_EXECUTE) - ->destructive() - ->apply(), - ]); -} -``` +## Notes on Filament execution semantics -### Bulk Actions +- Hidden actions do not execute (silent no-op). This is acceptable for the non-member contract. +- Disabled actions do not execute (silent no-op). This is acceptable for member-without-capability. +- Server-side authorization remains required (UI is not security). Where reachable: + - Non-member: deny-as-not-found (404) + - Member without capability: 403 -```php -->bulkActions([ - UiEnforcement::forBulkAction( - BulkAction::make('deleteSelected') - ->action(fn (Collection $records) => $records->each->delete()) - ) - ->requireCapability(Capabilities::TENANT_DELETE) - ->destructive() - ->apply(), -]) -``` +## Manual QA checklist (once implemented) -## Behavior Matrix +- Log in as a tenant member without the capability: + - Action visible but disabled, with tooltip “Insufficient permission — ask a tenant Owner.” + - Clicking does not execute. +- Remove membership: + - Action hidden; direct access returns 404. +- Bulk selection with mixed authorization: + - Bulk action is disabled; no partial execution. -| User Status | UI State | Server Response | -|-------------|----------|-----------------| -| Non-member | Hidden | Blocked (no execution, 200) | -| Member, no capability | Visible, disabled + tooltip | Blocked (no execution, 200) | -| Member, has capability | Enabled | Executes | -| Member, destructive action | Confirmation modal | Executes after confirm | +## Test run (once implemented) -> **Note on 404/403 Responses:** In Filament v5, hidden actions are automatically -> treated as disabled, so execution is blocked silently (returns 200 with no side -> effects). True 404 enforcement happens at the page/routing level via tenant -> middleware. The UiEnforcement helper includes defense-in-depth server-side -> guards that abort(404/403) if somehow reached, but the primary protection is -> Filament's isHidden/isDisabled chain. - -## Tooltip Customization - -Default tooltip: *"You don't have permission to do this. Ask a tenant admin."* - -Override per-action: - -```php -UiEnforcement::forAction($action) - ->requireCapability(Capabilities::PROVIDER_MANAGE) - ->tooltip('Contact your organization owner to enable this feature.') - ->apply(); -``` - -## Testing - -Test both UI state and execution blocking: - -```php -it('hides sync action for non-members', function () { - $user = User::factory()->create(); - $tenant = Tenant::factory()->create(); - // user is NOT a member - - actingAs($user); - Filament::setTenant($tenant, true); - - Livewire::test(ListPolicies::class) - ->assertActionHidden('sync'); -}); - -it('blocks action execution for non-members (no side effects)', function () { - $user = User::factory()->create(); - $tenant = Tenant::factory()->create(); - Queue::fake(); - - actingAs($user); - Filament::setTenant($tenant, true); - - // Hidden actions are blocked silently (200 but no execution) - Livewire::test(ListPolicies::class) - ->mountAction('sync') - ->callMountedAction() - ->assertSuccessful(); - - // Verify no side effects occurred - Queue::assertNothingPushed(); -}); - -it('disables sync action for readonly members', function () { - [$user, $tenant] = createUserWithTenant(role: 'readonly'); - - actingAs($user); - Filament::setTenant($tenant, true); - - Livewire::test(ListPolicies::class) - ->assertActionVisible('sync') - ->assertActionDisabled('sync'); -}); - -it('shows disabled tooltip for readonly members', function () { - [$user, $tenant] = createUserWithTenant(role: 'readonly'); - - actingAs($user); - Filament::setTenant($tenant, true); - - Livewire::test(ListPolicies::class) - ->assertActionHasTooltip('sync', UiTooltips::INSUFFICIENT_PERMISSION); -}); -``` - -## Migration Checklist - -When migrating an existing action: - -- [ ] Remove `->visible(fn ...)` closure (UiEnforcement handles this) -- [ ] Remove `->disabled(fn ...)` closure (UiEnforcement handles this) -- [ ] Remove inline `Gate::check()` / `abort_unless()` from action handler -- [ ] Wrap action with `UiEnforcement::forAction(...)->requireCapability(...)->apply()` -- [ ] Add `->destructive()` if action modifies/deletes data -- [ ] Add test for non-member (hidden + no execution) -- [ ] Add test for member without capability (disabled + tooltip) -- [ ] Add test for member with capability (enabled + executes) - -### Real Example: ListPolicies Sync Action - -```php -// Before (ad-hoc) -Action::make('sync') - ->icon('heroicon-o-arrow-path') - ->action(function (): void { - $tenant = Tenant::current(); - if (! $tenant) { - return; - } - // ... sync logic - }) - ->disabled(fn (): bool => ! Gate::allows(Capabilities::TENANT_SYNC, Tenant::current())) - -// After (UiEnforcement) -UiEnforcement::forAction( - Action::make('sync') - ->icon('heroicon-o-arrow-path') - ->action(function (): void { - $tenant = Tenant::current(); - if (! $tenant) { - return; - } - // ... sync logic - }) -) - ->requireCapability(Capabilities::TENANT_SYNC) - ->destructive() - ->apply() -``` - -## Common Mistakes - -### ❌ Forgetting `->apply()` - -```php -// This does nothing! -UiEnforcement::forAction($action) - ->requireCapability(Capabilities::PROVIDER_MANAGE); - // missing ->apply() -``` - -### ❌ Using with non-tenant panels - -```php -// UiEnforcement is tenant-scoped only! -// For /system panel, use Gate::check() directly -``` - -### ❌ Mixing old and new patterns - -```php -// Don't mix - pick one -UiEnforcement::forAction( - Action::make('sync') - ->visible(fn () => someOtherCheck()) // ❌ conflict -) - ->requireCapability(Capabilities::PROVIDER_MANAGE) - ->apply(); -``` - -## Questions? - -See [spec.md](./spec.md) for full requirements or [plan.md](./plan.md) for implementation details. +- `./vendor/bin/sail bin pint --dirty` +- `./vendor/bin/sail artisan test tests/Feature/Guards --compact` +- `./vendor/bin/sail artisan test tests/Feature/Filament --compact` +- `./vendor/bin/sail artisan test tests/Feature/Rbac --compact` diff --git a/specs/066-rbac-ui-enforcement-helper/research.md b/specs/066-rbac-ui-enforcement-helper/research.md new file mode 100644 index 0000000..352f6a4 --- /dev/null +++ b/specs/066-rbac-ui-enforcement-helper/research.md @@ -0,0 +1,89 @@ +# 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. + diff --git a/specs/066-rbac-ui-enforcement-helper/spec.md b/specs/066-rbac-ui-enforcement-helper/spec.md index 8226d05..2008257 100644 --- a/specs/066-rbac-ui-enforcement-helper/spec.md +++ b/specs/066-rbac-ui-enforcement-helper/spec.md @@ -1,163 +1,288 @@ -# Feature Specification: RBAC UI Enforcement Helper v1 +# Feature Specification: RBAC UI Enforcement Helper v2 (Suite-wide, Mixed Visibility, Record-Scoped) -**Feature Branch**: `066-rbac-ui-enforcement-helper` -**Created**: 2026-01-28 +**Feature Branch**: `066-rbac-ui-enforcement-helper-v2` +**Created**: 2026-01-30 **Status**: Draft -**Input**: Provide a suite-wide, consistent way to enforce tenant RBAC for admin UI actions (buttons/actions in lists, records, and bulk actions) without copy/paste authorization logic. +**Input**: Make the RBAC UI enforcement helper suite-ready by supporting mixed visibility, record-scoped tenancy, and deterministic bulk preflight authorization (follow-up to Feature 066 v1). ## Clarifications -### Session 2026-01-28 +### Session 2026-01-28 (carried forward from Feature 066 v1) -- Q: For Bulk Actions with mixed-permission records (some authorized, some not), what should the default behavior be? → A: All-or-nothing (if any selected record would be unauthorized, the bulk action is disabled for members and execution fails with 403 for members / 404 for non-members). +- Q: For Bulk Actions with mixed-permission records (some authorized, some not), what should the default behavior be? → A: All-or-nothing (if any selected record would be unauthorized, the bulk action is disabled for members; if execution is forced, it must fail with 403 for members / 404 for non-members). - Q: Should the helper render actions at all for non-members (in case a tenant page is reachable via misrouting), or always hide them? → A: Hide for non-members in UI, but still enforce 404 server-side for any execution attempt. - Q: How strict should the “no ad-hoc authorization patterns in app/Filament/**” guard be in v1? → A: CI-failing (new ad-hoc patterns fail tests/CI). -## User Scenarios & Testing *(mandatory)* +### Session 2026-01-30 - +- Q: For mixed visibility, should `UiEnforcement::preserveVisibility()` be restricted to tenant-scoped surfaces (tenant routing already denies non-members), and forbidden for record-scoped / cross-tenant lists? → A: Yes — tenant-scoped only; record-scoped must use `andVisibleWhen(...)` / `andHiddenWhen(...)` (or default RBAC visibility). +- Q: For bulk actions, should v2’s default “all-or-nothing” preflight apply to authorization only, or to authorization + business eligibility? → A: Authorization only — disable if any selected record is unauthorized; business-ineligible records may be skipped with a clear notification. +- Q: For record-scoped tenant pages/actions (e.g., TenantResource row actions / edit/view), should a non-member be denied as 404 or 403 on direct access/execution? → A: 404 (deny-as-not-found). +- Q: What should the default `UiTooltips` message be for member without capability (disabled action tooltip)? → A: “Insufficient permission — ask a tenant Owner.” +- Q: Which authorization plane(s) are in scope for `UiEnforcement` v2? → A: Tenant plane only (tenant users/memberships with `Capabilities::*`); platform plane (`/system`) is out of scope. -### User Story 1 - Tenant member sees consistent disabled UX (Priority: P1) +## Problem Statement -As a tenant member, I can clearly see which actions exist, and when I lack permission the action is visible but disabled with an explanatory tooltip. +Feature 066 v1 introduced a canonical RBAC UI enforcement helper (`UiEnforcement`) and a CI-failing file-scan guard (`NoAdHocFilamentAuthPatternsTest`) to prevent drift. This removed many ad-hoc patterns, but several high-value Filament surfaces remain difficult to migrate due to: -**Why this priority**: Prevents confusion and reduces support load while keeping the UI predictable for members. +1. Mixed visibility (business visibility + RBAC visibility) +2. Record-scoped tenancy (resources that are not tenant-scoped; the record *is* the tenant) +3. Bulk actions needing predictable all-or-nothing semantics without N+1 or partial execution surprises +4. Filament execution semantics (hidden/disabled blocks execution as a no-op; 404/403 cannot always be asserted at action execution time) -**Independent Test**: Can be tested by visiting a tenant-scoped admin page as a member with insufficient permissions and verifying the action is disabled, shows the standard tooltip, and cannot be executed. +v2 makes the helper suite-ready by adding explicit primitives for these cases and migrating remaining high-risk surfaces. -**Acceptance Scenarios**: +## Goals -1. **Given** a tenant member without the required capability, **When** they view an action on a tenant-scoped page, **Then** the action is visible but disabled and shows the standard “insufficient permission” tooltip. -2. **Given** a tenant member without the required capability, **When** they attempt to execute the action (including direct invocation, bypassing the UI), **Then** the server rejects with 403. +### G1 — Suite-wide RBAC UX consistency ---- +A tenant member sees a consistent UI pattern everywhere: -### User Story 2 - Non-members cannot infer tenant resources (Priority: P2) +- **Non-member**: cannot discover or act (deny-as-not-found for pages; actions hidden + cannot execute) +- **Member without capability**: action visible but disabled with a standard tooltip; cannot execute +- **Member with capability**: action enabled; executes normally -As a non-member of a tenant, I cannot discover tenant-scoped resources or actions; the system responds as “not found”. +### G2 — Eliminate remaining ad-hoc RBAC in Filament -**Why this priority**: Prevents tenant enumeration and cross-tenant information leakage. +Remove `Gate::...`, `abort_*`, policy ability string literals, and custom RBAC patterns from remaining targeted Filament surfaces, replacing them with `UiEnforcement` and canonical `Capabilities::*`. -**Independent Test**: Can be tested by attempting to access tenant-scoped pages/actions as a user without membership and verifying 404 behavior. +### G3 — Support mixed visibility and record-scoped tenancy -**Acceptance Scenarios**: +Provide first-class APIs so teams can migrate without special casing per resource. -1. **Given** a user who is not entitled to the tenant scope, **When** they attempt any tenant-scoped page or action, **Then** the system responds as 404 (deny-as-not-found). +### G4 — Keep enforcement testable and stable ---- +Guard remains stable (file scan), allowlist shrinks, and we add targeted integration tests for each migrated surface. -### User Story 3 - Maintainers add actions safely by default (Priority: P3) +## Non-Goals -As a maintainer, I can add new tenant-scoped actions using one standard pattern, and regression guards prevent introducing ad-hoc authorization logic. +- Redesign pages or change business flows beyond RBAC visibility/disable/confirm behavior. +- Rebuild authentication (063/064) or expand RBAC model semantics (065) beyond capabilities usage. +- Convert every remaining Filament file in one shot; v2 targets a defined set and continues shrinking allowlist incrementally. -**Why this priority**: Reduces RBAC regressions as the app grows and makes reviews easier. +## Terms -**Independent Test**: Can be tested by introducing a sample ad-hoc authorization pattern and confirming automated checks/tests flag it. +- **Member**: user has `canAccessTenant($tenant)` / membership. +- **Capability**: canonical string key defined in `app/Support/Auth/Capabilities.php` (single source of truth). +- **Tenant-scoped page**: Filament tenant context is set (`Filament::getTenant()` non-null). +- **Record-scoped tenant**: record itself defines tenant context (e.g., tenant list rows where each record is a Tenant). +- **Business visibility**: visibility logic unrelated to RBAC (trashed state, filter state, record status, etc.). +- **Authorization plane**: v2 applies to the tenant plane (`/admin/t/{tenant}`) only; the platform plane (`/system`) is out of scope. -**Acceptance Scenarios**: +## RBAC-UX Contract (v2) -1. **Given** a maintainer adds a new tenant-scoped action, **When** they use the central enforcement helper, **Then** member/non-member semantics and tooltip behavior match the standard without additional per-page customization. -2. **Given** a maintainer introduces a new ad-hoc authorization mapping in tenant-scoped admin UI code, **When** automated checks run, **Then** the change is flagged to prevent drift. +This feature follows the RBAC constitution rules and clarifies Filament execution reality. ---- +### 5.1 Non-member -[Add more user stories as needed, each with an assigned priority] +- **Pages/routes**: MUST be deny-as-not-found (404) using route/middleware scoping (already handled by tenant routing + membership guard). +- **Actions**: MUST be hidden and cannot execute. -### Edge Cases +**Note**: Filament treats hidden as non-executable (silent no-op). This is acceptable and is the enforced behavior. - +### 5.2 Member without capability -- Membership is revoked while the user has the page open (execution must still enforce 404 semantics). -- Capability changes mid-session (UI may be stale; server enforcement remains correct). -- Bulk actions with mixed-permission records: all-or-nothing (disable + tooltip for members; 403 on execution for members; 404 semantics for non-members). -- Target record is deleted/archived between render and execution (no information leakage in errors). +- Action is visible but disabled. +- Tooltip MUST be from `UiTooltips` (or a standard override). +- Execution is blocked for normal UI interaction (Filament disabled no-op). +- Server-side authorization remains required for any mutation action (Policy/Gate). Where an execution attempt is reachable, it MUST be forbidden (403) for members without the capability. + - Default tooltip text: “Insufficient permission — ask a tenant Owner.” -## Requirements *(mandatory)* +### 5.3 Member with capability -**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 (`OperationRun` type/identity/visibility), and tests. -If security-relevant DB-only actions intentionally skip `OperationRun`, the spec MUST describe `AuditLog` entries. +- Enabled and executes. +- Any destructive action MUST require confirmation (`->requiresConfirmation()`). -**Constitution alignment (RBAC-UX):** This feature defines a default pattern for tenant-plane admin actions. The implementation MUST: -- enforce membership as an isolation boundary (non-member / not entitled → 404 deny-as-not-found), -- enforce capability denials as 403 (after membership is established), -- keep actions visible-but-disabled with a standard tooltip for members lacking capability (except allowed sensitive exceptions), -- enforce authorization server-side for every mutation/operation-start/credential change, -- use the canonical capability registry (no raw capability string literals), -- ensure destructive-like actions require confirmation, -- ship regression tests and a guard against new ad-hoc authorization patterns. +### 5.4 Bulk actions (default) -**Constitution alignment (OPS-EX-AUTH-001):** OIDC/SAML login handshakes may perform synchronous outbound HTTP (e.g., token exchange) -on `/auth/*` endpoints without an `OperationRun`. This MUST NOT be used for Monitoring/Operations pages. +All-or-nothing (authorization): if any selected record fails authorization, the bulk action is disabled (members) and cannot execute; no partial execution. +Business eligibility (e.g., inactive/archived) is handled separately: ineligible records may be skipped with deterministic user feedback. -**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. - - +## Requirements ### Functional Requirements -- **FR-001**: The system MUST provide a single, centrally maintained enforcement mechanism that can be applied to tenant-scoped admin actions (including header actions, record actions, and bulk actions). -- **FR-002**: For tenant-scoped actions, the system MUST enforce membership as deny-as-not-found: users not entitled to the tenant scope MUST receive 404 semantics for action execution. -- **FR-002a**: For users not entitled to the tenant scope, the UI SHOULD NOT render tenant-scoped actions (default: hidden), while server-side execution MUST still enforce 404 semantics. -- **FR-003**: For tenant members, the system MUST enforce capability denial as 403 when executing an action without permission. -- **FR-004**: For tenant members lacking capability, the UI MUST render actions as visible-but-disabled and MUST show a standard tooltip explaining the missing permission. -- **FR-005**: The enforcement mechanism MUST also enforce the same rules server-side (UI state is never sufficient). -- **FR-006**: The enforcement mechanism MUST be capability-first and MUST reference capabilities only via the canonical capability registry (no ad-hoc string literals). -- **FR-007**: The enforcement mechanism MUST provide a standard confirmation behavior for destructive-like actions, including a clear warning message. -- **FR-008**: The system MUST provide standardized, non-leaky error and tooltip messages: - - 404 semantics for non-members without hints. - - 403 responses for insufficient capability without object details. -- **FR-009**: v1 MUST include limited adoption by migrating 3–6 exemplar action surfaces to the new pattern to prove the approach. -- **FR-010**: v1 MUST include regression tests that cover: non-member → 404, member without capability → disabled UI + 403 on execution, member with capability → allowed. -- **FR-010a**: For bulk actions with mixed-permission records, the default behavior MUST be all-or-nothing (members see disabled + tooltip; execution denies with 403; non-members receive 404 semantics). -- **FR-011**: v1 MUST include an automated, CI-failing guard that flags new ad-hoc authorization patterns in tenant-scoped admin UI code. -- **FR-012**: The enforcement mechanism MUST avoid introducing avoidable performance regressions (no per-record membership lookups during render). -- **FR-013**: The enforcement mechanism MUST NOT trigger outbound HTTP calls during render; it is DB-only. +- **FR-001 Mixed Visibility Support**: `UiEnforcement` MUST support combining RBAC visibility with existing business visibility patterns without overriding them. + - A resource can keep business `->visible()` conditions (trashed/state/filter). + - RBAC adds membership/capability constraints without losing business visibility behavior. -### Key Entities *(include if feature involves data)* +- **FR-002 Record-Scoped Tenant Context**: `UiEnforcement` MUST support record-scoped tenant resolution, where `$record` determines the tenant context. + - Table action closures receive `$record`, and enforcement evaluates membership/capability against that record-tenant. + - No hard dependency on `Filament::getTenant()` for these surfaces. -- **Tenant**: The isolation boundary for all tenant-scoped UI and actions. -- **User**: The authenticated actor attempting to view or execute actions. -- **Membership**: Whether a user is entitled to a tenant scope. -- **Capability**: A named permission from the canonical capability registry. -- **Action**: A discrete operation exposed in the tenant-scoped admin interface. +- **FR-003 Bulk All-or-Nothing Preflight**: `UiEnforcement` MUST support bulk actions with a preflight authorization check that is deterministic, avoids N+1 DB queries (set-based where possible), and disables actions for mixed authorization selections. + - The default preflight decision is based on authorization only; business eligibility remains separate (records may be skipped with clear feedback). -### Assumptions +- **FR-004 Canonical Capability Enforcement**: All v2-migrated surfaces MUST reference capabilities only via `Capabilities::*` constants. -- Default tooltip language is English (i18n may be added later). -- Non-destructive bulk actions are in scope for v1; destructive bulk actions may be supported but are not required for v1 completion. -- Global search tenant scoping is out of scope for this spec (covered by separate work), but this feature must not introduce new leaks. +- **FR-005 Stable Guardrails**: The file-scan guard MUST remain CI-failing and allowlist-driven. + - Allowlist entries MUST be removed as surfaces are migrated. + - New ad-hoc patterns in `app/Filament/**` MUST fail CI. -## Success Criteria *(mandatory)* +- **FR-006 Suite-wide Migration Targets (v2)**: v2 MUST migrate the following surface categories: + - **Tier 1 (high impact / high risk)**: + - Backup/Restore surfaces that currently contain ad-hoc RBAC patterns: + - BackupSet resources / relation managers (mixed visibility) + - RestoreRun resources / actions (mixed visibility) + - TenantResource table actions (record-scoped tenant) + - **Tier 2 (remaining allowlist “easy wins”)**: + - Inventory item / inventory sync run resources + - Any remaining Entra group run list pages + - ProviderConnection pages where mixed visibility exists (if not completed in v1) + - v2 MUST produce a measurable allowlist reduction (at minimum: remove allowlist entries for all migrated files). - +- **FR-007 Tests for Each Migrated Surface**: Each migrated surface MUST have at least one focused integration test asserting: + - Non-member: action hidden + cannot execute + - Member no cap: visible disabled + tooltip + - Member with cap: enabled executes + +### Non-Functional Requirements + +- **NFR-001 DB-only Rendering**: Helper and migrations MUST not introduce outbound HTTP during rendering (no Graph calls; DB only). +- **NFR-002 Performance / Query Discipline**: Membership/capability evaluation should be O(1) per request once membership is loaded (no repeated membership queries per action render). Bulk preflight MUST be set-based where feasible. + +## Design + +### 7.1 `UiEnforcement` API extensions (v2) + +#### 7.1.1 Preserve / Compose Visibility (mixed visibility) + +Add explicit composition methods: + +- `preserveVisibility()` + - Means: do not write `->visible(...)` / `->hidden(...)` closures. + - Only apply disabled/tooltip/confirmation, and always enforce server-side authorization for any mutation action (Policy/Gate), independent of UI visibility/disabled state. + - Optional: add additional defensive guards (e.g., explicit deny-as-not-found behavior) where the action/page lifecycle makes it reachable. + - Restriction: tenant-scoped surfaces only (record-scoped / cross-tenant lists MUST NOT use this). +- `andVisibleWhen(callable $businessVisible)` + - Wrap business-visible closure and AND it with RBAC membership/capability visibility checks. +- `andHiddenWhen(callable $businessHidden)` + - Combine business-hidden logic with RBAC hidden logic. + +**Rule**: Default `apply()` sets RBAC visibility itself. Mixed-visibility surfaces MUST explicitly opt in to preservation or composition. + +#### 7.1.2 Tenant context providers + +Introduce explicit tenant resolver configuration: + +- `tenantFromFilament()` (default) +- `tenantFromRecord()` (for record == tenant) +- `tenantFrom(callable $resolver)` (custom mapping for pages where record maps to tenant) + +This MUST work for: + +- header actions (no record parameter) +- table actions (record available in closure) +- bulk actions (records collection or selection IDs) + +#### 7.1.3 Bulk preflight + +For bulk actions add: + +- `preflightSelection(callable $preflight)` OR built-in modes: + - `preflightByTenantMembership()` using IDs and tenant context + - `preflightByCapability()` for all selected records + +**Default for v2**: disable bulk action if any unauthorized (all-or-nothing). + +### 7.2 Execution semantics (documented) + +Spec and quickstart MUST explicitly state: + +- hidden/disabled actions are blocked by Filament as no-ops, which is acceptable security behavior +- 404/403 semantics are asserted at routing/page access and by server-side checks where reachable + +### 7.3 Migration policy + +- Migrate 1–2 files per batch. +- Remove allowlist entry in the same batch. +- Run Pint + targeted tests + guard test each batch. + +## User Stories & Testing + +### US1 — Mixed Visibility: Backup/Restore surfaces become UiEnforcement-compliant (Priority: P1) + +As a tenant admin, I want backup/restore actions to follow the same RBAC UX patterns while preserving business visibility logic (trashed, filter state). + +**Acceptance Criteria** + +- Existing visibility behavior remains unchanged. +- RBAC behavior is consistent (hidden/disabled + tooltip). +- No ad-hoc `Gate/abort` patterns remain on migrated surfaces. + +### US2 — Record-Scoped Tenant: TenantResource actions enforced correctly (Priority: P1) + +As a tenant-plane user with multiple tenant memberships, I want tenant list row actions to be enforced per tenant record without leaking other tenants. + +**Acceptance Criteria** + +- Row actions use record-tenant context. +- Non-member never sees row actions for that tenant (hidden). +- Member lacking capability sees disabled + tooltip. + +### US3 — Bulk action preflight prevents partial execution (Priority: P2) + +As an operator, bulk actions should be disabled if selection contains unauthorized records. + +**Acceptance Criteria** + +- All-or-nothing semantics. +- No partial execution. +- Deterministic feedback (standard tooltip message). + +### US4 — Guard allowlist shrinks with each migration (Priority: P2) + +As a maintainer, I want CI to fail if new ad-hoc RBAC patterns are added in Filament. + +**Acceptance Criteria** + +- Allowlist reduction is measurable (count down). +- New violations fail tests. + +## Verification Checklist (manual + automated) + +### Automated + +- `vendor/bin/sail bin pint --dirty` +- `vendor/bin/sail artisan test tests/Feature/Guards --compact` +- Focused tests per migrated surface +- RBAC suites: `tests/Feature/Rbac` + `tests/Unit/Support/Rbac` + +### Manual spot checks (UI) + +- Login as Readonly → verify disabled tooltips and no execution. +- Remove membership → verify actions disappear and cannot execute. +- Bulk action selection with mixed permissions → action disabled. + +## Rollout / Migration Strategy + +- Keep the guard allowlist for legacy files. +- Every migration PR reduces allowlist entries. +- Any new Filament file MUST NOT introduce forbidden patterns. + +## v2 Deliverables + +### Code + +- `UiEnforcement` enhancements: mixed visibility + record-tenant context + bulk preflight support. +- Migrated Tier 1 and Tier 2 target surfaces. +- Expanded tests for v2 targets. + +### Docs + +- Update the feature quickstart with mixed-visibility recipes and record-tenant examples. +- Update spec/constitution references if needed (no new constitution rules unless discovered gaps). + +### Quality + +- Guard remains CI-failing and stable. +- Allowlist shrunk measurably. + +## Success Criteria ### Measurable Outcomes -- **SC-001**: For all migrated tenant-scoped action surfaces, 100% of non-member execution attempts are denied with 404 semantics (verified by automated tests). -- **SC-002**: For all migrated tenant-scoped action surfaces, 100% of member-but-unauthorized execution attempts are denied with 403 (verified by automated tests). -- **SC-003**: For all migrated tenant-scoped action surfaces, members lacking capability see the action visible-but-disabled with the standard tooltip (verified by automated tests and/or UI assertions). -- **SC-004**: At least one automated guard exists that flags newly introduced ad-hoc authorization patterns in tenant-scoped admin UI code. -- **SC-005**: v1 demonstrates adoption by migrating 3–6 exemplar action surfaces, reducing duplicate authorization wiring in those areas. +- **SC-001**: For every migrated surface, tests assert the RBAC-UX contract: non-member hidden/no-exec, member-no-cap disabled + tooltip, member-with-cap enabled + executes. +- **SC-002**: Guard allowlist removes entries for every migrated file, and CI continues to fail for newly introduced ad-hoc auth patterns in `app/Filament/**`. +- **SC-003**: Bulk preflight authorization is deterministic and does not degrade into per-record membership queries (verified via query count assertions on representative bulk selection tests). +- **SC-004**: Rendering/hydration tests for migrated pages/actions enforce DB-only rendering (stray HTTP requests are prevented during render). diff --git a/specs/066-rbac-ui-enforcement-helper/tasks.md b/specs/066-rbac-ui-enforcement-helper/tasks.md index 8ee4e18..f29b9a2 100644 --- a/specs/066-rbac-ui-enforcement-helper/tasks.md +++ b/specs/066-rbac-ui-enforcement-helper/tasks.md @@ -1,254 +1,215 @@ -# Tasks: RBAC UI Enforcement Helper v1 +--- -**Input**: Design documents from `/specs/066-rbac-ui-enforcement-helper/` -**Prerequisites**: plan.md ✓, spec.md ✓, quickstart.md ✓ - -**Tests**: REQUIRED (Pest) — this feature changes runtime authorization behavior. -**RBAC**: This feature IS the RBAC enforcement helper — all tasks enforce constitution RBAC-UX rules. - -**Organization**: Tasks grouped by user story for independent implementation. - -## Format: `[ID] [P?] [Story?] Description` - -- **[P]**: Can run in parallel (different files, no dependencies) -- **[Story]**: US1/US2/US3 for user story phases; omitted for Setup/Foundational/Polish +description: "Task list for RBAC UI Enforcement Helper v2" --- -## Phase 1: Setup +# Tasks: RBAC UI Enforcement Helper v2 (Suite-wide, Mixed Visibility, Record-Scoped) -**Purpose**: Create helper infrastructure with no external dependencies +**Input**: Design documents from `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/` +**Prerequisites**: `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/plan.md` (required), `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/spec.md` (required), `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/research.md`, `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/data-model.md`, `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/contracts/`, `/Users/ahmeddarrazi/Documents/projects/TenantAtlas-066-rbac-ui-enforcement-helper-v2/specs/066-rbac-ui-enforcement-helper/quickstart.md` -- [X] T001 Create directory structure `app/Support/Rbac/` -- [X] T002 [P] Create `UiTooltips.php` with tooltip constants in `app/Support/Rbac/UiTooltips.php` -- [X] T003 [P] Create `TenantAccessContext.php` DTO in `app/Support/Rbac/TenantAccessContext.php` +**Tests**: REQUIRED (Pest) for runtime behavior changes +**RBAC**: Tenant plane only (`/admin/t/{tenant}`); non-member is deny-as-not-found (404); member without capability is disabled + tooltip and cannot execute; destructive actions require confirmation +**Organization**: Tasks are grouped by user story (US1–US4) to enable independent delivery. + +## Phase 1: Setup (Shared Infrastructure) + +**Purpose**: Prepare local environment + validate baseline + +- [X] T001 Start containers with `./vendor/bin/sail up -d` (script: `./vendor/bin/sail`) +- [X] T002 Run baseline guard suite with `./vendor/bin/sail artisan test tests/Feature/Guards --compact` (folder: `tests/Feature/Guards`) +- [X] T003 Run baseline tenant RBAC suite with `./vendor/bin/sail artisan test tests/Feature/Rbac --compact` (folder: `tests/Feature/Rbac`) --- ## Phase 2: Foundational (Blocking Prerequisites) -**Purpose**: Core `UiEnforcement` helper — MUST complete before any user story tests +**Purpose**: Shared helper + guardrails required by all story migrations -**⚠️ CRITICAL**: No user story work can begin until this phase is complete +**Checkpoint**: `UiEnforcement` supports mixed visibility, record-scoped tenant resolution, and bulk preflight; guard exists and is allowlist-driven -- [X] T004 Implement `UiEnforcement::forAction()` static method in `app/Support/Rbac/UiEnforcement.php` -- [X] T005 Implement `->requireMembership()` method (default: true) in `app/Support/Rbac/UiEnforcement.php` -- [X] T006 Implement `->requireCapability(string $capability)` method in `app/Support/Rbac/UiEnforcement.php` -- [X] T007 Implement `->destructive()` method (confirmation modal) in `app/Support/Rbac/UiEnforcement.php` -- [X] T008 Implement `->tooltip(string $message)` override method in `app/Support/Rbac/UiEnforcement.php` -- [X] T009 Implement `->apply()` method (sets hidden/disabled/guards) in `app/Support/Rbac/UiEnforcement.php` -- [X] T010 Implement `UiEnforcement::forTableAction()` static method in `app/Support/Rbac/UiEnforcement.php` -- [X] T011 Implement `UiEnforcement::forBulkAction()` static method with all-or-nothing logic in `app/Support/Rbac/UiEnforcement.php` - -**Checkpoint**: `UiEnforcement` class ready — user story tests can now be written +- [X] T004 Create `UiTooltips` with the v2 default disabled tooltip copy in `app/Support/Auth/UiTooltips.php` +- [X] T005 Create/verify v1 baseline `UiEnforcement` helper in `app/Support/Auth/UiEnforcement.php` (apply hidden/disabled/tooltip + server-side guard using `Capabilities::*`) +- [X] T006 [P] Add mixed visibility APIs (`preserveVisibility()` tenant-scoped only, `andVisibleWhen()`, `andHiddenWhen()`) in `app/Support/Auth/UiEnforcement.php` +- [X] T007 [P] Add tenant resolver APIs (`tenantFromFilament()`, `tenantFromRecord()`, `tenantFrom(callable)`) in `app/Support/Auth/UiEnforcement.php` +- [X] T008 [P] Add bulk preflight APIs (`preflightSelection()`, `preflightByTenantMembership()`, `preflightByCapability()`) with authorization-only all-or-nothing default in `app/Support/Auth/UiEnforcement.php` +- [X] T009 [P] Add helper unit tests (mixed visibility restriction + tenant resolvers + preflight decisions) in `tests/Unit/Auth/UiEnforcementTest.php` +- [X] T010 [P] Add query-count regression test for bulk preflight (no N+1 membership lookups) in `tests/Unit/Auth/UiEnforcementBulkPreflightQueryCountTest.php` +- [X] T011 Add CI-failing allowlist-driven guard for ad-hoc Filament auth patterns in `tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php` +- [X] T012 Run the new guard test with `./vendor/bin/sail artisan test tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php --compact` (test: `tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php`) --- -## Phase 3: User Story 1 — Tenant member sees consistent disabled UX (Priority: P1) 🎯 MVP +## Phase 3: User Story 1 — Mixed Visibility (Backup/Restore) (Priority: P1) 🎯 MVP -**Goal**: Members lacking capability see actions visible-but-disabled with standard tooltip; 403 on execution +**Goal**: Backup/Restore actions preserve business visibility while enforcing RBAC UX (hidden/disabled + standard tooltip) via `UiEnforcement` -**Independent Test**: Visit tenant page as member with insufficient permission → action disabled with tooltip, cannot execute +**Independent Test**: A non-member cannot access tenant routes (404); a member without capability sees actions disabled with the standard tooltip and cannot execute; a member with capability can execute (with confirmation for destructive actions) ### Tests for User Story 1 -- [X] T012 [P] [US1] Test: member without capability sees disabled action + tooltip in `tests/Feature/Rbac/UiEnforcementMemberDisabledTest.php` -- [X] T013 [P] [US1] Test: member without capability is blocked from execution in `tests/Feature/Rbac/UiEnforcementMemberDisabledTest.php` -- [X] T014 [P] [US1] Test: member with capability sees enabled action + can execute in `tests/Feature/Rbac/UiEnforcementMemberDisabledTest.php` -- [X] T014a [P] [US1] Test: destructive action shows confirmation modal before execution in `tests/Feature/Rbac/UiEnforcementDestructiveTest.php` +- [X] T013 [P] [US1] Add BackupSet RBAC UX integration tests (include `Http::preventStrayRequests()`) in `tests/Feature/Filament/BackupSetUiEnforcementTest.php` +- [X] T014 [P] [US1] Add RestoreRun RBAC UX integration tests (include `Http::preventStrayRequests()`) in `tests/Feature/Filament/RestoreRunUiEnforcementTest.php` ### Implementation for User Story 1 -- [X] T015 [US1] Validate `->apply()` correctly sets `->disabled()` + `->tooltip()` for members lacking capability (logic in T009; this task verifies + adjusts if needed) in `app/Support/Rbac/UiEnforcement.php` -- [X] T016 [US1] Validate `->apply()` correctly blocks unauthorized execution (via Filament's isDisabled check + defense-in-depth abort) in `app/Support/Rbac/UiEnforcement.php` -- ~~T017 [US1] Migrate TenantResource table actions to UiEnforcement~~ **OUT OF SCOPE v1**: TenantResource is record==tenant, not tenant-scoped -- [X] T018 [US1] Migrate ProviderConnectionResource actions to UiEnforcement (mixed visibility via `preserveVisibility()`) in `app/Filament/Resources/ProviderConnectionResource.php` - -**Checkpoint**: US1 complete — members see consistent disabled UX with tooltip (exemplar: ListPolicies) +- [X] T015 [US1] Migrate mixed-visibility actions + bulk actions to `UiEnforcement` in `app/Filament/Resources/BackupSetResource.php` +- [X] T016 [US1] Migrate relation manager actions (incl. standardized tooltip copy via `UiTooltips`) in `app/Filament/Resources/BackupSetResource/RelationManagers/BackupItemsRelationManager.php` +- [X] T017 [US1] Migrate mixed-visibility actions + bulk actions to `UiEnforcement` in `app/Filament/Resources/RestoreRunResource.php` +- [X] T018 [US1] Replace `Gate`/`abort_*` access checks with `UiEnforcement` (tenant-scoped: non-member 404, member without cap 403) in `app/Filament/Resources/RestoreRunResource/Pages/CreateRestoreRun.php` +- [X] T019 [US1] Remove allowlist entries for migrated Backup/Restore files in `tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php` +- [X] T020 [US1] Run story tests with `./vendor/bin/sail artisan test tests/Feature/Filament/BackupSetUiEnforcementTest.php tests/Feature/Filament/RestoreRunUiEnforcementTest.php --compact` (tests: `tests/Feature/Filament/BackupSetUiEnforcementTest.php`) --- -## Phase 4: User Story 2 — Non-members cannot infer tenant resources (Priority: P2) +## Phase 4: User Story 2 — Record-Scoped Tenant (TenantResource actions) (Priority: P1) -**Goal**: Non-members receive 404 (deny-as-not-found) for all tenant-scoped actions; actions hidden in UI +**Goal**: Tenant list row actions are enforced per-tenant record (record-scoped tenancy) with non-member deny-as-not-found (404) and standardized member disabled UX -**Independent Test**: Access tenant page as non-member → actions hidden, execution returns 404 +**Independent Test**: A user sees only tenants they are a member of; direct access to another tenant record denies 404; row actions for permitted tenants are disabled/enabled per capability with standard tooltip ### Tests for User Story 2 -- [X] T019 [P] [US2] Test: non-member sees action hidden in UI in `tests/Feature/Rbac/UiEnforcementNonMemberHiddenTest.php` -- [X] T020 [P] [US2] Test: non-member action is blocked (via Filament hidden-action semantics) in `tests/Feature/Rbac/UiEnforcementNonMemberHiddenTest.php` -- [X] T021 [P] [US2] Test: membership revoked mid-session still enforces protection in `tests/Feature/Rbac/UiEnforcementNonMemberHiddenTest.php` +- [X] T021 [P] [US2] Extend tenant row action tests to assert disabled + tooltip + no-exec (include `Http::preventStrayRequests()`) in `tests/Feature/Filament/TenantActionsAuthorizationTest.php` +- [X] T022 [P] [US2] Add record-scoped non-member 404 tests for tenant view/edit routes in `tests/Feature/Filament/TenantPortfolioContextSwitchTest.php` ### Implementation for User Story 2 -- [X] T022 [US2] Validate `->apply()` correctly sets `->hidden()` for non-members (logic in T009; this task verifies + adjusts if needed) in `app/Support/Rbac/UiEnforcement.php` -- [X] T023 [US2] Validate `->apply()` blocks non-member execution (via Filament's isHidden → isDisabled chain; 404 server-side guard is defense-in-depth) in `app/Support/Rbac/UiEnforcement.php` -- [X] T024 [US2] Migrate BackupSetResource actions (row + bulk) to UiEnforcement (mixed visibility via `preserveVisibility()`) in `app/Filament/Resources/BackupSetResource.php` -- [X] T025 [US2] Migrate PolicyResource sync actions to UiEnforcement in `app/Filament/Resources/PolicyResource/Pages/ListPolicies.php` - -**Checkpoint**: US2 complete — non-members receive 404 semantics, no information leakage +- [X] T023 [US2] Migrate TenantResource row actions to `UiEnforcement` using `tenantFromRecord()` (remove ad-hoc `Gate`/`abort_*`) in `app/Filament/Resources/TenantResource.php` +- [X] T024 [US2] Migrate record-scoped edit-page destructive actions to `UiEnforcement` (remove ad-hoc `Gate`/`abort_*`) in `app/Filament/Resources/TenantResource/Pages/EditTenant.php` +- [X] T025 [US2] Remove allowlist entries for migrated TenantResource files in `tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php` +- [X] T026 [US2] Run story tests with `./vendor/bin/sail artisan test tests/Feature/Filament/TenantActionsAuthorizationTest.php tests/Feature/Filament/TenantPortfolioContextSwitchTest.php --compact` (tests: `tests/Feature/Filament/TenantActionsAuthorizationTest.php`) --- -## Phase 5: User Story 3 — Maintainers add actions safely by default (Priority: P3) +## Phase 5: User Story 3 — Bulk Preflight (All-or-Nothing Authorization) (Priority: P2) -**Goal**: CI-failing guard flags new ad-hoc authorization patterns; standard pattern documented +**Goal**: Bulk actions disable deterministically when selection contains any unauthorized record; no partial execution -**Independent Test**: Introduce ad-hoc `Gate::allows` or `abort_unless()` in Filament → guard test fails +**Independent Test**: Selecting tenants with mixed authorization disables the bulk action; selecting only authorized tenants executes; business-ineligible records may be skipped with clear feedback ### Tests for User Story 3 -- [X] T026 [P] [US3] Guard test: scan `app/Filament/**` for forbidden ad-hoc patterns (Gate + abort helpers) in `tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php` -- [X] T027 [P] [US3] Unit test: UiEnforcement uses only canonical Capabilities constants in `tests/Unit/Support/Rbac/UiEnforcementTest.php` +- [X] T027 [P] [US3] Update bulk sync tests to assert all-or-nothing authorization disable (include `Http::preventStrayRequests()`) in `tests/Feature/Filament/TenantPortfolioContextSwitchTest.php` ### Implementation for User Story 3 -- [X] T028 [US3] Replace Pest-Arch guard with stable file-scan guard (CI-failing, allowlist for legacy only) in `tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php` -- [X] T029 [US3] Migrate EntraGroupResource sync actions to UiEnforcement in `app/Filament/Resources/EntraGroupResource/Pages/ListEntraGroups.php` -- [X] T030 [US3] Remove Gate facade usage from FindingResource (migrate auth to canonical checks) in `app/Filament/Resources/FindingResource.php` - -**Checkpoint**: US3 complete — guardrail prevents regression (file-scan), exemplar surfaces migrated +- [X] T028 [US3] Add authorization-only bulk preflight for `syncSelected` (disable when any selected tenant lacks `Capabilities::TENANT_SYNC`) in `app/Filament/Resources/TenantResource.php` +- [X] T029 [US3] Ensure bulk action feedback cleanly separates authorization vs eligibility (skip inactive with notification; do not partially execute unauthorized) in `app/Filament/Resources/TenantResource.php` +- [X] T030 [US3] Run story tests with `./vendor/bin/sail artisan test tests/Feature/Filament/TenantPortfolioContextSwitchTest.php --compact` (test: `tests/Feature/Filament/TenantPortfolioContextSwitchTest.php`) --- -## Phase 6: Polish & Cross-Cutting Concerns +## Phase 6: User Story 4 — Guard Allowlist Shrink + Tier 2 Migrations (Priority: P2) -**Purpose**: Cleanup, additional tests, documentation +**Goal**: CI fails for new ad-hoc auth patterns in `app/Filament/**`; allowlist count shrinks measurably by migrating remaining targets -- [X] T031 [P] PHPDoc blocks present on all public methods in `app/Support/Rbac/UiEnforcement.php` -- [X] T032 [P] Update quickstart.md with migration examples in `specs/066-rbac-ui-enforcement-helper/quickstart.md` -- [X] T033 Run Pint formatter on new files with `vendor/bin/sail bin pint app/Support/Rbac` -- [X] T034 Run full test suite with `vendor/bin/sail artisan test --compact` — 837 passed, 5 skipped -- [X] T035 Validate quickstart.md examples work in codebase (ListPolicies migration verified) +**Independent Test**: Guard fails if a new forbidden pattern is introduced; allowlist removes entries as files are migrated; each migrated Tier 2 surface has a focused RBAC UX test + +### Tests for User Story 4 + +- [X] T031 [P] [US4] Extend inventory resource RBAC UX tests (include `Http::preventStrayRequests()`) in `tests/Feature/Filament/InventoryItemResourceTest.php` and `tests/Feature/Filament/InventorySyncRunResourceTest.php` +- [X] T032 [P] [US4] Extend Entra group runs RBAC UX tests (include `Http::preventStrayRequests()`) in `tests/Feature/Filament/EntraGroupSyncRunResourceTest.php` +- [X] T033 [P] [US4] Add provider connections RBAC UX tests for mixed visibility (include `Http::preventStrayRequests()`) in `tests/Feature/Filament/ProviderConnectionsUiEnforcementTest.php` + +### Implementation for User Story 4 + +- [X] T034 [US4] Migrate inventory resources away from ad-hoc `Gate`/`abort_*` to `UiEnforcement` in `app/Filament/Resources/InventoryItemResource.php`, `app/Filament/Resources/InventoryItemResource/Pages/ListInventoryItems.php`, and `app/Filament/Resources/InventorySyncRunResource.php` +- [X] T035 [US4] Migrate Entra group list/run start surfaces away from ad-hoc `Gate`/`abort_*` to `UiEnforcement` in `app/Filament/Resources/EntraGroupResource/Pages/ListEntraGroups.php` and `app/Filament/Resources/EntraGroupSyncRunResource/Pages/ListEntraGroupSyncRuns.php` +- [X] T036 [US4] Migrate provider connection mixed-visibility actions away from ad-hoc `Gate`/`abort_*` to `UiEnforcement` in `app/Filament/Resources/ProviderConnectionResource.php`, `app/Filament/Resources/ProviderConnectionResource/Pages/ListProviderConnections.php`, and `app/Filament/Resources/ProviderConnectionResource/Pages/EditProviderConnection.php` +- [X] T037 [US4] Remove allowlist entries for migrated Tier 2 files and note the allowlist count reduction in `tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php` +- [X] T038 [US4] Run story tests with `./vendor/bin/sail artisan test tests/Feature/Filament/InventoryItemResourceTest.php tests/Feature/Filament/InventorySyncRunResourceTest.php tests/Feature/Filament/EntraGroupSyncRunResourceTest.php tests/Feature/Filament/ProviderConnectionsUiEnforcementTest.php --compact` (tests: `tests/Feature/Filament/ProviderConnectionsUiEnforcementTest.php`) +- [X] T039 [US4] Run guard suite with `./vendor/bin/sail artisan test tests/Feature/Guards --compact` (folder: `tests/Feature/Guards`) --- -## Phase 7: Follow-up — Findings capability cleanup (Mini-feature) +## Phase 7: Polish & Cross-Cutting Concerns -**Purpose**: Avoid overloading broad capabilities (e.g. `TENANT_SYNC`) for findings acknowledgement. +**Purpose**: Reduce regressions and ensure suite-wide consistency -- [X] T036 Add `Capabilities::TENANT_FINDINGS_ACKNOWLEDGE` in `app/Support/Auth/Capabilities.php` -- [X] T037 Grant `TENANT_FINDINGS_ACKNOWLEDGE` to Owner/Manager/Operator (not Readonly) + update role-matrix tests -- [X] T038 Update Finding list acknowledge action to require `TENANT_FINDINGS_ACKNOWLEDGE` in `app/Filament/Resources/FindingResource/Pages/ListFindings.php` -- [X] T039 Refactor `FindingPolicy::update()` to use `CapabilityResolver` with `TENANT_FINDINGS_ACKNOWLEDGE` (remove ad-hoc `Gate::forUser(...)->allows(...)`) - ---- - -## Phase 8: Follow-up — Legacy allowlist shrink (Stepwise) - -**Purpose**: Keep shrinking the Filament guard allowlist with one-file migrations. - -- [X] T040 Remove `BackupScheduleResource.php` from the legacy allowlist after migration -- [X] T041 Migrate `ListEntraGroupSyncRuns.php` to UiEnforcement + add a focused Livewire test -- [X] T042 Remove `ListEntraGroupSyncRuns.php` from the legacy allowlist after migration -- [X] T043 Migrate `ListProviderConnections.php` create action to UiEnforcement + add a focused Livewire test -- [X] T044 Remove `ListProviderConnections.php` from the legacy allowlist after migration -- [X] T045 Migrate `DriftLanding.php` generation permission check to `CapabilityResolver` (remove Gate facade) + add a focused Livewire test -- [X] T046 Remove `DriftLanding.php` from the legacy allowlist after migration -- [X] T047 Migrate `RegisterTenant.php` page-level checks to `CapabilityResolver` + replace `abort_unless()` with `abort()` -- [X] T048 Remove `RegisterTenant.php` from the legacy allowlist after migration -- [X] T049 Migrate `EditProviderConnection.php` actions + save guards to `UiEnforcement`/`CapabilityResolver` (remove Gate facade + abort_unless) + add a focused Livewire test -- [X] T050 Remove `EditProviderConnection.php` from the legacy allowlist after migration -- [X] T051 Migrate `CreateRestoreRun.php` page authorization to `CapabilityResolver` (remove Gate facade + abort_unless) + add a focused Livewire test -- [X] T052 Remove `CreateRestoreRun.php` from the legacy allowlist after migration -- [X] T053 Migrate `InventoryItemResource.php` resource authorization to `CapabilityResolver` (remove Gate facade) + add a focused Pest test -- [X] T054 Remove `InventoryItemResource.php` from the legacy allowlist after migration -- [X] T055 Migrate `VersionsRelationManager.php` restore action to `UiEnforcement`/`CapabilityResolver` (remove Gate facade + abort_unless) + add a focused Livewire test -- [X] T056 Remove `VersionsRelationManager.php` from the legacy allowlist after migration -- [X] T057 Migrate `BackupItemsRelationManager.php` actions to `UiEnforcement` (remove Gate facade) + add a focused Livewire test -- [X] T058 Remove `BackupItemsRelationManager.php` from the legacy allowlist after migration -- [X] T059 Migrate `PolicyVersionResource.php` actions/bulk actions off ad-hoc patterns to `UiEnforcement`/`CapabilityResolver` (remove Gate facade + abort_unless) while preserving metadata-only restore behavior -- [X] T060 Remove `PolicyVersionResource.php` from the legacy allowlist after migration -- [X] T061 Migrate `RestoreRunResource.php` actions/bulk actions off ad-hoc patterns to `UiEnforcement`/`CapabilityResolver` (remove Gate facade + abort_unless) -- [X] T062 Remove `RestoreRunResource.php` from the legacy allowlist after migration -- [X] T063 Fix `UiEnforcement` server-side guard to use Filament lifecycle hooks (`->before()`) to preserve Filament action parameter injection -- [X] T064 Migrate `PolicyResource.php` actions/bulk actions off ad-hoc patterns to `UiEnforcement` (remove Gate facade + abort_unless) -- [X] T065 Remove `PolicyResource.php` from the legacy allowlist after migration -- [X] T066 Migrate `EditTenant.php` archive action off ad-hoc patterns to `UiEnforcement` (remove Gate facade + abort_unless) -- [X] T067 Remove `EditTenant.php` from the legacy allowlist after migration -- [X] T068 Migrate `TenantMembershipsRelationManager.php` actions off ad-hoc patterns to `UiEnforcement` (remove Gate facade) -- [X] T069 Remove `TenantMembershipsRelationManager.php` from the legacy allowlist after migration -- [X] T070 Migrate `TenantResource.php` off ad-hoc patterns to `CapabilityResolver` (remove Gate facade + abort_unless) -- [X] T071 Remove `TenantResource.php` from the legacy allowlist after migration +- [X] T040 [P] Ensure all destructive actions touched in v2 include `->requiresConfirmation()` in `app/Filament/Resources/BackupSetResource.php`, `app/Filament/Resources/RestoreRunResource.php`, and `app/Filament/Resources/TenantResource/Pages/EditTenant.php` +- [X] T041 Run formatting with `./vendor/bin/sail bin pint --dirty` (script: `./vendor/bin/pint`) +- [X] T042 Run focused test suites with `./vendor/bin/sail artisan test tests/Unit/Auth tests/Feature/Filament tests/Feature/Guards tests/Feature/Rbac --compact` (folders: `tests/Feature/Filament`) --- ## Dependencies & Execution Order -### Phase Dependencies +### User Story Dependency Graph -- **Setup (Phase 1)**: No dependencies — can start immediately -- **Foundational (Phase 2)**: Depends on Setup — BLOCKS all user stories -- **User Stories (Phase 3–5)**: All depend on Foundational; can proceed in parallel or by priority -- **Polish (Phase 6)**: Depends on all user stories - -### User Story Dependencies - -- **US1 (P1)**: Foundational only — no cross-story dependencies -- **US2 (P2)**: Foundational only — no cross-story dependencies -- **US3 (P3)**: Foundational only — no cross-story dependencies - -### Within Each User Story - -- Tests MUST be written FIRST and FAIL before implementation -- Wire logic in `UiEnforcement.php` before migrating Filament surfaces -- Migrate surfaces one at a time, verify tests pass +```text +Phase 1 (Setup) + ↓ +Phase 2 (Foundation: UiEnforcement v2 + guard baseline) + ↓ +US1 (Backup/Restore mixed visibility) ─┬─→ US4 (Tier 2 migrations + allowlist shrink) + ├─→ US2 (TenantResource record-scoped actions) + └─→ US3 (Bulk preflight all-or-nothing) +``` ### Parallel Opportunities -- T002 + T003 (Setup) can run in parallel -- All test tasks (T012–T014, T019–T021, T026–T027) can run in parallel -- US1, US2, US3 can run in parallel after Foundational -- T031 + T032 (Polish) can run in parallel +- Phase 2 tasks marked `[P]` can run in parallel (different files). +- Within US1, BackupSet and RestoreRun migrations can be done in parallel (different resources) if coordinated to avoid conflicting changes in `app/Support/Auth/UiEnforcement.php`. +- Within US4, Inventory vs EntraGroup vs ProviderConnection migrations can be parallelized (different files), but coordinate allowlist edits in `tests/Feature/Guards/NoAdHocFilamentAuthPatternsTest.php`. --- ## Parallel Example: User Story 1 ```bash -# Launch all tests for US1 together: -T012: "Test: member without capability sees disabled action + tooltip" -T013: "Test: member without capability receives 403 on execution" -T014: "Test: member with capability sees enabled action + can execute" +Task: "Add BackupSet RBAC UX integration tests in tests/Feature/Filament/BackupSetUiEnforcementTest.php" +Task: "Add RestoreRun RBAC UX integration tests in tests/Feature/Filament/RestoreRunUiEnforcementTest.php" +Task: "Migrate BackupSet mixed-visibility actions to UiEnforcement in app/Filament/Resources/BackupSetResource.php" +Task: "Migrate RestoreRun mixed-visibility actions to UiEnforcement in app/Filament/Resources/RestoreRunResource.php" +``` -# Then implement sequentially: -T015 → T016 → T017 → T018 +--- + +## Parallel Example: User Story 2 + +```bash +Task: "Extend tenant row action tests to assert disabled + tooltip + no-exec in tests/Feature/Filament/TenantActionsAuthorizationTest.php" +Task: "Add record-scoped non-member 404 tests for tenant view/edit routes in tests/Feature/Filament/TenantPortfolioContextSwitchTest.php" +Task: "Migrate TenantResource row actions to UiEnforcement using tenantFromRecord() in app/Filament/Resources/TenantResource.php" +Task: "Migrate record-scoped edit-page destructive actions to UiEnforcement in app/Filament/Resources/TenantResource/Pages/EditTenant.php" +``` + +--- + +## Parallel Example: User Story 3 + +```bash +Task: "Update bulk sync tests to assert all-or-nothing authorization disable in tests/Feature/Filament/TenantPortfolioContextSwitchTest.php" +Task: "Add authorization-only bulk preflight for syncSelected in app/Filament/Resources/TenantResource.php" +``` + +--- + +## Parallel Example: User Story 4 + +```bash +Task: "Extend inventory resource RBAC UX tests in tests/Feature/Filament/InventoryItemResourceTest.php" +Task: "Extend Entra group runs RBAC UX tests in tests/Feature/Filament/EntraGroupSyncRunResourceTest.php" +Task: "Add provider connections RBAC UX tests for mixed visibility in tests/Feature/Filament/ProviderConnectionsUiEnforcementTest.php" +Task: "Migrate Entra group run list surfaces to UiEnforcement in app/Filament/Resources/EntraGroupSyncRunResource.php" +Task: "Migrate provider connection pages/actions to UiEnforcement in app/Filament/Resources/ProviderConnectionResource/Pages/ListProviderConnections.php" ``` --- ## Implementation Strategy -### MVP First (User Story 1 Only) +### MVP First (User Story 1) -1. Complete Phase 1: Setup (T001–T003) -2. Complete Phase 2: Foundational (T004–T011) -3. Complete Phase 3: User Story 1 (T012–T018) -4. **STOP and VALIDATE**: Members see disabled + tooltip + 403 -5. Deploy/demo if ready +1. Complete Phase 1 + Phase 2 +2. Complete US1 (BackupSet + RestoreRun mixed visibility enforcement) +3. Validate with `tests/Feature/Filament/BackupSetUiEnforcementTest.php` and `tests/Feature/Filament/RestoreRunUiEnforcementTest.php` ### Incremental Delivery -1. Setup + Foundational → `UiEnforcement` ready -2. US1 → Consistent disabled UX for members (MVP!) -3. US2 → Non-member 404 enforcement -4. US3 → CI-failing guardrail + all 6 surfaces migrated -5. Polish → Docs, cleanup, full test suite - ---- - -## Summary - -| Metric | Count | -|--------|-------| -| Total tasks | 40 | -| Setup tasks | 3 | -| Foundational tasks | 8 | -| US1 tasks | 8 | -| US2 tasks | 7 | -| US3 tasks | 5 | -| Polish tasks | 5 | -| Follow-up tasks | 4 | -| Parallel opportunities | 13 | -| MVP scope | Phases 1–3 (T001–T018) | +1. US1 → migrate highest-risk backup/restore surfaces + tests + allowlist reduction +2. US2 → record-scoped tenant actions enforced per row + 404 non-member on direct access +3. US3 → bulk preflight all-or-nothing authorization +4. US4 → Tier 2 migrations + measurable allowlist shrink + guard stability