64 lines
4.5 KiB
Markdown
64 lines
4.5 KiB
Markdown
# Research — Spec 083 (Required Permissions hardening)
|
||
|
||
## Context recap
|
||
- Canonical manage-plane surface already exists as a Filament Page: `App\Filament\Pages\TenantRequiredPermissions` with slug `tenants/{tenant}/required-permissions` (admin panel).
|
||
- A legacy tenant-plane path prefix exists (`/admin/t/...`) via the tenant panel; spec requires `/admin/t/{tenant}/required-permissions` to remain non-existent and return 404.
|
||
|
||
## Decisions
|
||
|
||
### Decision 1 — Canonical route stays `/admin/tenants/{tenant}/required-permissions`
|
||
- **Chosen**: Keep the canonical manage URL exactly as specified in Spec 083.
|
||
- **Rationale**: Already aligned with the existing page slug and with the established route contract in Spec 080.
|
||
- **Alternatives considered**:
|
||
- Redirect from `/admin/t/...` → rejected (spec requires 404, no redirect).
|
||
|
||
### Decision 2 — Deny-as-not-found is implemented explicitly (404), not via “canAccess() only”
|
||
- **Chosen**: Enforce 404 using explicit `abort(404)` checks on request entry (e.g., `mount()`), instead of relying solely on Filament’s `canAccess()` return value.
|
||
- **Rationale**: Filament’s `canAccess()` may produce behavior that is not guaranteed to be a 404. Spec 083 requires strict 404 semantics for non-members / non-entitled.
|
||
- **Alternatives considered**:
|
||
- Only `canAccess()` returning false → rejected (status code semantics uncertain).
|
||
- Route-level middleware on just this page → possible, but still needs explicit entitlement checks; can be added later if desired.
|
||
|
||
### Decision 3 — Tenant entitlement is checked via `User::canAccessTenant($tenant)`
|
||
- **Chosen**: Use `User::canAccessTenant()` for tenant entitlement (no workspace-wide “view all tenants” override).
|
||
- **Rationale**: This matches existing patterns across the codebase, uses `tenant_memberships`, and aligns with the clarification outcome.
|
||
- **Alternatives considered**:
|
||
- Workspace membership only → rejected (Spec 083 requires tenant entitlement).
|
||
- Capability checks for read-only view → rejected (read-only access is entitlement-only; mutations are capability-gated elsewhere).
|
||
|
||
### Decision 4 — Remove cross-plane tenant fallback (`Tenant::current()`) from this surface
|
||
- **Chosen**: `TenantRequiredPermissions::resolveScopedTenant()` must be strict: resolve only from route parameter `{tenant}` (external_id or bound model). If absent/invalid → 404.
|
||
- **Rationale**: `Tenant::current()` can leak cross-plane context and violates FR-083-007.
|
||
- **Alternatives considered**:
|
||
- Keep fallback for convenience → rejected (security hardening goal).
|
||
|
||
### Decision 5 — DB-only render is guaranteed by using stored `tenant_permissions`
|
||
- **Chosen**: Continue using `TenantPermissionService::compare(... liveCheck:false ...)` for this page (no Graph calls).
|
||
- **Rationale**: With `liveCheck=false`, compare reads stored `tenant_permissions` only.
|
||
- **Alternatives considered**:
|
||
- Allow live-check on render → rejected (violates FR-083-010).
|
||
|
||
### Decision 6 — Freshness (“Last refreshed”) comes from `tenant_permissions.last_checked_at`
|
||
- **Chosen**: Define page “Last refreshed” as the max timestamp of stored permission checks for the tenant. Stale if missing or older than 30 days.
|
||
- **Rationale**: This is already stored in the database and does not require provider calls.
|
||
- **Alternatives considered**:
|
||
- Use latest verification run timestamps → possible, but increases coupling; not necessary for Spec 083.
|
||
|
||
### Decision 7 — Summary status logic is centralized in the view-model builder
|
||
- **Chosen**: Update `TenantRequiredPermissionsViewModelBuilder::deriveOverallStatus()` so:
|
||
- Blocked if any missing application permission (blocker)
|
||
- Else Needs attention if any warning exists (missing delegated, error rows folded into warning, or stale freshness)
|
||
- Else Ready
|
||
- **Rationale**: Aligns with Spec 083 summary rules and keeps mapping centralized.
|
||
- **Alternatives considered**:
|
||
- Compute in Blade view → rejected (harder to test, risks drift).
|
||
|
||
### Decision 8 — “Re-run verification” CTA links to `/admin/onboarding` (“Start verification” surface)
|
||
- **Chosen**: Link-only CTA points to the existing onboarding wizard page (admin panel slug `onboarding`).
|
||
- **Rationale**: Clarification outcome; capability gating occurs on the start/execute surface, not on this read-only page.
|
||
- **Alternatives considered**:
|
||
- Link to provider connection edit → rejected (not the requested primary action).
|
||
|
||
## Open questions
|
||
None remaining for Spec 083 (clarifications already settled).
|