188 lines
9.2 KiB
Markdown
188 lines
9.2 KiB
Markdown
# Research: 109 — Tenant Review Pack Export v1 (CSV + ZIP)
|
|
|
|
**Date**: 2026-02-23
|
|
**Branch**: `109-review-pack-export`
|
|
|
|
---
|
|
|
|
## 1. OperationRun Integration
|
|
|
|
### Decision
|
|
Add `ReviewPackGenerate` case with value `tenant.review_pack.generate` to `OperationRunType` enum.
|
|
|
|
### Rationale
|
|
- Follows established enum pattern: 14 existing cases in `app/Support/OperationRunType.php`.
|
|
- Active-run dedupe is enforced at dispatch time via `scopeActive()` (`whereIn('status', ['queued', 'running'])`), not a DB-level unique index. Same pattern reused.
|
|
- Status lifecycle uses existing `OperationRunStatus` enum: `Queued`, `Running`, `Completed`.
|
|
- Outcome is set on the run record; failure details go into `context` JSONB (no `reason_code` column — stored as `context['reason_code']` and `context['error_message']`).
|
|
|
|
### Alternatives Considered
|
|
- Adding a DB partial unique index for active-run dedupe → Rejected; existing code uses application-level guard consistently. Adding a DB constraint would be a cross-cutting architectural change.
|
|
|
|
---
|
|
|
|
## 2. RBAC Capabilities
|
|
|
|
### Decision
|
|
Add two constants to `app/Support/Auth/Capabilities.php`:
|
|
- `REVIEW_PACK_VIEW` = `'review_pack.view'`
|
|
- `REVIEW_PACK_MANAGE` = `'review_pack.manage'`
|
|
|
|
### Rationale
|
|
- 35 existing capability constants follow the pattern `domain.action` (e.g., `tenant.view`, `entra_roles.manage`).
|
|
- Capabilities are discovered via `Capabilities::all()` using reflection — new constants are auto-discovered.
|
|
- No separate enum needed; the constants-class pattern is canonical.
|
|
- `REVIEW_PACK_MANAGE` covers generate + expire/delete (per clarification Q2). Confirmation dialog is the safety gate for destructive actions.
|
|
|
|
### Alternatives Considered
|
|
- Separate `REVIEW_PACK_DELETE` capability → Deferred; can be split out later with no migration needed.
|
|
|
|
---
|
|
|
|
## 3. StoredReport & Finding Data Access
|
|
|
|
### Decision
|
|
Query existing models directly; no new scopes or accessors needed for v1.
|
|
|
|
### Rationale
|
|
- `StoredReport` has `report_type` constants (`permission_posture`, `entra.admin_roles`), `payload` (array cast), `fingerprint`, `previous_fingerprint`. Query: latest per report_type per tenant.
|
|
- `Finding` has `finding_type` constants (`drift`, `permission_posture`, `entra_admin_roles`), `status` (new/acknowledged/resolved), `severity`, `fingerprint`, `evidence_jsonb`. Export scope: `status IN (new, acknowledged)` — resolved findings excluded.
|
|
- Both use `DerivesWorkspaceIdFromTenant` trait for workspace_id auto-derivation.
|
|
- CSV column mapping is straightforward from model attributes; no transformation layer needed.
|
|
|
|
### Alternatives Considered
|
|
- Adding dedicated export scopes on the models → Over-engineering for v1; simple where clauses suffice. Can be extracted later.
|
|
|
|
---
|
|
|
|
## 4. Tenant Hardening Fields
|
|
|
|
### Decision
|
|
Export the following fields to `hardening.json`:
|
|
- `rbac_last_checked_at` (datetime)
|
|
- `rbac_last_setup_at` (datetime)
|
|
- `rbac_canary_results` (array)
|
|
- `rbac_last_warnings` (array — includes computed `scope_limited` warning)
|
|
- `rbac_scope_mode` (string)
|
|
|
|
### Rationale
|
|
- These are the tenant RBAC hardening/write-safety fields per the Tenant model.
|
|
- The `getRbacLastWarningsAttribute` accessor enriches the raw array with `scope_limited` when `rbac_scope_mode === 'scope_group'` — need to use the accessor, not raw DB value.
|
|
- `app_client_secret` is encrypted and MUST NOT be exported (FR-008 compliance).
|
|
|
|
### Alternatives Considered
|
|
- Including ProviderConnection health in hardening.json → Deferred to v2; adds complexity and a separate model dependency.
|
|
|
|
---
|
|
|
|
## 5. Badge Integration
|
|
|
|
### Decision
|
|
Add `ReviewPackStatus` case to `BadgeDomain` enum and create `ReviewPackStatusBadge` mapper.
|
|
|
|
### Rationale
|
|
- 35 existing badge domains in `app/Support/Badges/BadgeDomain.php`.
|
|
- Pattern: enum case → mapper class implementing `BadgeMapper::spec()` returning `BadgeSpec(label, color, ?icon)`.
|
|
- Status values: `queued` (warning), `generating` (info), `ready` (success), `failed` (danger), `expired` (gray).
|
|
- Colors align with existing palette: `gray`, `info`, `success`, `warning`, `danger`, `primary`.
|
|
|
|
### Alternatives Considered
|
|
- Reusing existing badge domains → Not applicable; review pack status is a new domain with distinct semantics.
|
|
|
|
---
|
|
|
|
## 6. Filesystem Disk
|
|
|
|
### Decision
|
|
Add `exports` disk to `config/filesystems.php` using the `local` driver at `storage_path('app/private/exports')`.
|
|
|
|
### Rationale
|
|
- Existing `local` disk pattern: `storage_path('app/private')`, driver `local`, `serve: true`.
|
|
- `exports` is private (non-public URL); downloads go through signed route controller.
|
|
- Path `storage/app/private/exports/` keeps exports co-located with other private storage.
|
|
- In Dokploy deployments, `storage/app/` is typically volume-mounted; no extra volume config needed.
|
|
|
|
### Alternatives Considered
|
|
- Using S3 from the start → Deferred per clarification Q5; local disk for v1, S3 swappable later.
|
|
- Using existing `local` disk with a subdirectory → Rejected; dedicated disk gives cleaner config and allows independent retention/backup settings.
|
|
|
|
---
|
|
|
|
## 7. Console Schedule
|
|
|
|
### Decision
|
|
Add three new schedule entries to `routes/console.php`:
|
|
1. `tenantpilot:review-pack:prune` → `daily()` + `withoutOverlapping()`
|
|
2. `tenantpilot:posture:dispatch` → **Does not exist yet.** Must be created as a new Artisan command OR schedule existing job dispatch directly.
|
|
3. `tenantpilot:entra-roles:dispatch` → **Does not exist yet.** Same as above.
|
|
|
|
### Rationale
|
|
- Currently: Entra admin roles scan is dispatched as a closure via `daily()` + `withoutOverlapping()` (iterates connected tenants). Permission posture generation is event-driven from `ProviderConnectionHealthCheckJob`, not scheduled.
|
|
- For FR-015: the spec requires both to be command-based and scheduled. However, creating new Artisan dispatch commands is out of scope for spec 109 if they require significant scanning infrastructure changes.
|
|
- **Pragmatic approach**: FR-015 is listed as P3 (lowest priority). If the dispatch commands don't exist yet, wire what exists (the Entra roles closure is already daily) and defer `tenantpilot:posture:dispatch` creation to a separate spec/task. Document this in the plan.
|
|
|
|
### Alternatives Considered
|
|
- Creating full dispatch commands in this spec → Scope creep; the scan orchestration is a separate concern.
|
|
|
|
---
|
|
|
|
## 8. AlertRule `sla_due` Cleanup
|
|
|
|
### Decision
|
|
Remove `sla_due` from the AlertRuleResource form dropdown options; keep the `EVENT_SLA_DUE` constant on the model for backward compatibility.
|
|
|
|
### Rationale
|
|
- `sla_due` is defined as a constant on `AlertRule` and appears in the form dropdown at `AlertRuleResource.php` line ~379.
|
|
- No producer dispatches `sla_due` events — it's a dead option.
|
|
- Removing from the form prevents new rules from selecting it. Existing rules with `sla_due` continue to exist in the DB but won't match any events (harmless).
|
|
|
|
### Alternatives Considered
|
|
- Hard-deleting existing `sla_due` rules via migration → Too aggressive for v1; rules are workspace-owned data.
|
|
|
|
---
|
|
|
|
## 9. Download via Signed URL
|
|
|
|
### Decision
|
|
Implement `URL::signedRoute()` with configurable TTL (default: 60 minutes). Download controller validates signature, streams file from `exports` disk.
|
|
|
|
### Rationale
|
|
- No existing signed-URL or download-streaming pattern in the codebase — greenfield.
|
|
- Laravel's built-in `URL::signedRoute()` + `hasValidSignature()` middleware is production-proven.
|
|
- Download controller is a simple registered route (not Filament); validates signature + checks pack status (must be `ready`, not `expired`).
|
|
- TTL configurable via `config('tenantpilot.review_pack.download_url_ttl_minutes')`.
|
|
|
|
### Alternatives Considered
|
|
- Session-authenticated stream → Rejected per clarification Q1; notification links must be self-contained.
|
|
|
|
---
|
|
|
|
## 10. Notification
|
|
|
|
### Decision
|
|
Use Laravel's built-in database notification channel. Create `ReviewPackReadyNotification` and `ReviewPackFailedNotification` (or a single `ReviewPackStatusNotification` with context).
|
|
|
|
### Rationale
|
|
- Existing pattern: the app uses Filament's notification system (DB-backed) for user-facing notifications.
|
|
- Notification includes: pack status, generated_at, download URL (signed, for `ready`), or failure reason (sanitized, for `failed`).
|
|
- Single notification class with conditional rendering based on status is simpler than two separate classes.
|
|
|
|
### Alternatives Considered
|
|
- Two separate notification classes → Slightly cleaner typing but more boilerplate for identical structure. Single class preferred.
|
|
|
|
---
|
|
|
|
## 11. ZIP Assembly
|
|
|
|
### Decision
|
|
Use PHP `ZipArchive` with deterministic alphabetical file insertion order. Temporary file written to `sys_get_temp_dir()`, then moved to exports disk.
|
|
|
|
### Rationale
|
|
- `ZipArchive` is available in all PHP 8.4 builds (ext-zip).
|
|
- Deterministic order: files added alphabetically by path ensures same content → same ZIP bytes → stable sha256 for fingerprint verification.
|
|
- Write to temp first, then `Storage::disk('exports')->put()` — atomic; no partial files on disk if job fails mid-write.
|
|
- SHA-256 computed on the final file before persistence.
|
|
|
|
### Alternatives Considered
|
|
- Streaming ZIP (no temp file) → PHP ZipArchive doesn't support true streaming; would need a library like `maennchen/zipstream-php`. Deferred; temp file is fine for expected pack sizes.
|