fix: 109 remediate 12 consistency findings from project analysis
This commit is contained in:
Ahmed Darrazi
parent
fb87a0e177
commit
80f96df3d2
@ -12,7 +12,7 @@ ### Decision
|
||||
|
||||
### 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.
|
||||
- Active-run dedupe is enforced at dispatch time via `scopeActive()` (`whereIn('status', ['queued', 'running'])`). The existing DB partial unique index on `operation_runs` (`operation_runs_active_unique`) serves as a safety backstop. 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']`).
|
||||
|
||||
|
||||
@ -76,7 +76,7 @@ ### User Story 3 — RBAC Enforcement (Priority: P2)
|
||||
1. **Given** a user with no workspace membership, **When** they attempt to list or download a tenant's review packs, **Then** they receive a 404 (deny-as-not-found).
|
||||
2. **Given** a workspace member with `REVIEW_PACK_VIEW` but not `REVIEW_PACK_MANAGE`, **When** they view the list and download a ready pack, **Then** both actions succeed; **When** they attempt to trigger generation, **Then** they receive a 403.
|
||||
3. **Given** a workspace member with `REVIEW_PACK_MANAGE`, **When** they trigger generation and download the result, **Then** both actions succeed.
|
||||
4. **Given** an Owner/Manager invoking expire or delete on a pack, **When** they invoke the destructive action, **Then** a confirmation dialog is shown before execution.
|
||||
4. **Given** an Owner/Manager invoking expire on a pack, **When** they invoke the destructive action, **Then** a confirmation dialog is shown before execution.
|
||||
|
||||
---
|
||||
|
||||
@ -127,7 +127,8 @@ ## Requirements *(mandatory)*
|
||||
- Two new RBAC capabilities (`REVIEW_PACK_VIEW`, `REVIEW_PACK_MANAGE`) registered in the canonical capability registry; no raw strings in feature code.
|
||||
- Filament Resource + Tenant Dashboard card with UI Action Matrix and UX-001 compliance (described below).
|
||||
- Status badge on `review_packs.status` — centralized badge mapping per BADGE-001.
|
||||
- Destructive actions (expire/delete) enforce `->requiresConfirmation()`.
|
||||
- Destructive actions (expire) enforce `->requiresConfirmation()`.
|
||||
- Audit exemption (Expire action): Expire mutates the pack row (`status → expired`, `updated_at` updated) and deletes the storage file. The pack row itself is the audit trail (status change + timestamp + `initiated_by_user_id`). No separate `AuditLog` entry required — the mutation is observable via the persisted status field.
|
||||
- Authorization plane: tenant-context (`/admin/t/{tenant}/...`); 404 for non-members, 403 for members lacking capability.
|
||||
- All generation and expiry operations tracked via `operation_runs`; no observable mutation skips the OperationRun trail.
|
||||
|
||||
@ -148,21 +149,21 @@ ## Requirements *(mandatory)*
|
||||
|
||||
### Functional Requirements
|
||||
|
||||
- **FR-001**: System MUST provide a `review_packs` table with columns: `id`, `workspace_id` (FK NOT NULL), `tenant_id` (FK NOT NULL), `operation_run_id` (FK nullable), `generated_at`, `fingerprint`, `previous_fingerprint` (nullable), `status` (enum: queued/generating/ready/failed/expired), `summary` (jsonb), `options` (jsonb), `file_disk`, `file_path`, `file_size` (bigint), `sha256`, `expires_at`, timestamps. Unique index on `(workspace_id, tenant_id, fingerprint)`; additional indexes on `(workspace_id, tenant_id, generated_at)` and `(status, expires_at)`.
|
||||
- **FR-001**: System MUST provide a `review_packs` table with columns: `id`, `workspace_id` (FK NOT NULL), `tenant_id` (FK NOT NULL), `operation_run_id` (FK nullable), `initiated_by_user_id` (FK → users, nullable), `generated_at`, `fingerprint`, `previous_fingerprint` (nullable), `status` (enum: queued/generating/ready/failed/expired), `summary` (jsonb), `options` (jsonb), `file_disk`, `file_path`, `file_size` (bigint), `sha256`, `expires_at`, timestamps. Partial unique index on `(workspace_id, tenant_id, fingerprint)` WHERE `fingerprint IS NOT NULL AND status NOT IN ('expired', 'failed')`; additional indexes on `(workspace_id, tenant_id, generated_at)` and `(status, expires_at)`.
|
||||
- **FR-002**: System MUST register `REVIEW_PACK_VIEW` and `REVIEW_PACK_MANAGE` in the canonical RBAC capability registry.
|
||||
- **FR-003**: System MUST implement a `ReviewPackResource` Filament Resource under Monitoring → Exports nav group with list, view, and modal-generate.
|
||||
- **FR-004**: System MUST implement a Tenant Dashboard card showing the latest pack's status, `generated_at`, `expires_at`, and "Generate pack" (manage) / "Download latest" (view/manage) actions.
|
||||
- **FR-005**: System MUST implement `tenant.review_pack.generate` OperationRun with active-run dedupe (unique active run per workspace+tenant+run_type) and content dedupe (fingerprint uniqueness per workspace+tenant).
|
||||
- **FR-006**: System MUST implement a queued Job that collects stored reports (`permission_posture`, `entra.admin_roles`), findings (`drift`, `permission_posture`, `entra_admin_roles`), tenant hardening/write-safety status fields, and recent operation_runs (last 30 days), then assembles and stores a ZIP artifact. The job MUST NOT trigger Graph API calls; it operates exclusively on existing DB data. `summary.json` MUST include a `data_freshness` object with per-source timestamps indicating the age of each data input.
|
||||
- **FR-007**: The generated ZIP MUST contain: `summary.json`, `findings.csv`, `operations.csv` (default on, togglable), `hardening.json`, `reports/permission_posture.json`, `reports/entra_admin_roles.json`, `metadata.json`. File order in ZIP MUST be stable and deterministic.
|
||||
- **FR-007**: The generated ZIP MUST contain: `summary.json`, `findings.csv`, `operations.csv` (default on, togglable), `hardening.json`, `reports/permission_posture.json`, `reports/entra_admin_roles.json`, `metadata.json`. File order in ZIP MUST be stable and deterministic. `metadata.json` MUST include at minimum: `generator_version` (app version string), `generated_at` (ISO 8601), `tenant_id`, `tenant_external_id`, `pack_fingerprint`, `options` (include_pii, include_operations), and `data_model_version` (schema version for forward compatibility).
|
||||
- **FR-008**: The export MUST NOT include webhook URLs, email recipient lists, tokens, client secrets, or raw Graph API response dumps.
|
||||
- **FR-009**: When `include_pii = false`, system MUST redact `principal.display_name` (and equivalent PII fields) across all exported files, retaining object IDs and types.
|
||||
- **FR-009**: When `include_pii = false`, system MUST redact `principal.display_name` across all exported files, retaining object IDs and types. v1 scope: `principal.display_name` is the only targeted PII field; additional PII fields discovered during implementation follow the same redaction pattern.
|
||||
- **FR-010**: System MUST compute a deterministic fingerprint: `sha256(tenant_id + include_pii + include_operations + sorted_report_fingerprints + max_finding_last_seen_at + hardening_status_tuple)`; a `ready` unexpired pack with the same fingerprint MUST be reused without creating a new artifact.
|
||||
- **FR-011**: Pack files MUST be written to `Storage::disk('exports')` (private, non-public); downloads MUST use signed temporary URLs (`URL::signedRoute()`) with a configurable TTL; the URL generation endpoint MUST enforce `REVIEW_PACK_VIEW` (non-members receive 404); the signed download controller validates the signature but does not require an active session.
|
||||
- **FR-012**: System MUST compute and persist `sha256` and `file_size` for each generated pack.
|
||||
- **FR-013**: System MUST set `expires_at` on each pack (default: 90 days from `generated_at`; configurable via `config('tenantpilot.review_pack.retention_days')`).
|
||||
- **FR-014**: System MUST provide Artisan command `tenantpilot:review-pack:prune` that marks packs past `expires_at` as `expired` and deletes their storage files. Hard-delete of DB rows is off by default; when invoked with `--hard-delete`, rows that have been in `expired` status for longer than the grace period (default: 30 days, configurable via `config('tenantpilot.review_pack.hard_delete_grace_days')`) are permanently removed. Without `--hard-delete`, expired rows remain queryable for audit trails.
|
||||
- **FR-015**: System MUST wire `tenantpilot:posture:dispatch` and `tenantpilot:entra-roles:dispatch` in the Laravel console scheduler with at least daily frequency.
|
||||
- **FR-015**: System MUST wire `tenantpilot:entra-roles:dispatch` in the Laravel console scheduler with at least daily frequency. System SHOULD wire `tenantpilot:posture:dispatch` when the command infrastructure exists (deferred — command does not yet exist; see research.md §7).
|
||||
- **FR-016**: System MUST remove or hide the `sla_due` event_type option from the AlertRule form field without breaking existing AlertRule data.
|
||||
- **FR-017**: System MUST send a DB notification to the initiator when a pack transitions to `ready` (with download link) or `failed` (with reason).
|
||||
- **FR-018**: On generation failure, system MUST record a stable `reason_code` (`review_pack.generation_failed` or `review_pack.storage_failed`) on the OperationRun with a sanitized error message.
|
||||
@ -174,7 +175,7 @@ ## UI Action Matrix *(mandatory)*
|
||||
|
||||
| Surface | Location | Header Actions | Inspect Affordance | Row Actions (max 2 visible) | Bulk Actions | Empty-State CTA(s) | View Header Actions | Destructive Confirmation | Audit log? | Notes |
|
||||
|---|---|---|---|---|---|---|---|---|---|---|
|
||||
| ReviewPackResource (List) | app/Filament/Resources/ReviewPackResource.php | "Generate Pack" modal (REVIEW_PACK_MANAGE) | Clickable row to View page | "Download" (ready only, REVIEW_PACK_VIEW), "Expire" (REVIEW_PACK_MANAGE, destructive + requiresConfirmation) | — | "No review packs yet" + "Generate first pack" CTA | — | Expire + Delete require requiresConfirmation() | Yes (OperationRun) | Generate opens options modal with include_pii toggle |
|
||||
| ReviewPackResource (List) | app/Filament/Resources/ReviewPackResource.php | "Generate Pack" modal (REVIEW_PACK_MANAGE) | Clickable row to View page | "Download" (ready only, REVIEW_PACK_VIEW), "Expire" (REVIEW_PACK_MANAGE, destructive + requiresConfirmation) | — | "No review packs yet" + "Generate first pack" CTA | — | Expire requires requiresConfirmation() | Yes (OperationRun) | Generate opens options modal with include_pii toggle |
|
||||
| ReviewPackResource (View) | app/Filament/Resources/ReviewPackResource/Pages/ViewReviewPack.php | "Download" (REVIEW_PACK_VIEW), "Regenerate" (REVIEW_PACK_MANAGE, Owner/Manager) | — | — | — | — | Download, Regenerate | Regenerate requires requiresConfirmation() if ready pack exists | Yes (OperationRun) | Infolist layout; status badge per BADGE-001 |
|
||||
| Tenant Dashboard Card | app/Filament/Widgets/TenantReviewPackCard.php | — | — | "Generate pack" (REVIEW_PACK_MANAGE), "Download latest" (REVIEW_PACK_VIEW) | — | "No pack yet — Generate first" | — | — | Yes (via OperationRun) | Shows latest pack status, generated_at, expires_at |
|
||||
|
||||
@ -213,18 +214,18 @@ ### Session 2026-02-23
|
||||
- Q: Expire/Delete authorization — reuse `REVIEW_PACK_MANAGE` or add a separate `REVIEW_PACK_DELETE` capability? → A: Reuse `REVIEW_PACK_MANAGE`; the `->requiresConfirmation()` dialog provides the safety gate. A dedicated delete capability can be split out later if needed.
|
||||
- Q: Stale data threshold — should the generation job trigger on-demand Graph scans if stored reports are older than 24h? → A: No. Generate with existing data (DB-only, no Graph calls). Include `data_freshness` timestamps per source in `summary.json` so the reviewer can judge staleness and optionally trigger manual scans before regenerating.
|
||||
- Q: Prune grace period for hard-delete — how long do expired rows stay in the DB, and is hard-delete on by default? → A: 30-day grace period after expiry. Hard-delete is off by default; operators opt in via `--hard-delete` flag on the prune command. Expired rows remain queryable for audit purposes until explicitly purged.
|
||||
- Q: `exports` disk storage backend — local disk or S3-compatible object storage? → A: Local disk (`storage/app/exports/`) with a persistent Docker volume mapped in Dokploy. Downloads streamed via signed route controller. S3 can be swapped later by changing the disk config without code changes.
|
||||
- Q: `exports` disk storage backend — local disk or S3-compatible object storage? → A: Local disk (`storage/app/private/exports/`) with a persistent Docker volume mapped in Dokploy. Downloads streamed via signed route controller. S3 can be swapped later by changing the disk config without code changes.
|
||||
|
||||
---
|
||||
|
||||
## Assumptions
|
||||
|
||||
- `stored_reports`, `findings`, `operation_runs`, tenant, and workspace models exist per Specs 104/105/108; this spec adds no structural changes to those tables.
|
||||
- The `exports` filesystem disk is configured as a local private disk at `storage/app/exports/` in `config/filesystems.php`. In Dokploy deployments, this path MUST be mapped to a persistent Docker volume. S3-compatible storage can be substituted later by changing the disk driver without code changes.
|
||||
- The `exports` filesystem disk is configured as a local private disk at `storage/app/private/exports/` in `config/filesystems.php`. In Dokploy deployments, this path MUST be mapped to a persistent Docker volume. S3-compatible storage can be substituted later by changing the disk driver without code changes.
|
||||
- The `OperationRun` active-run dedupe guard follows the same pattern established by prior specs; `tenant.review_pack.generate` is added to the run_type registry.
|
||||
- The canonical RBAC capability registry location is established by Specs 001/066; `REVIEW_PACK_VIEW` and `REVIEW_PACK_MANAGE` are added following the same pattern.
|
||||
- `principal.display_name` is the primary PII field in Entra admin roles stored_report payloads; additional PII fields follow the same redaction pattern if discovered.
|
||||
- Default `include_pii = true` for internal MSP use; workspace-level override is out of scope for v1.
|
||||
- "Force regenerate" (superseding a ready unexpired pack) is an optional Owner/Manager action; if deferred, the UI simply omits the button in v1.
|
||||
- Findings exported are scoped to `status IN (open, acknowledged)` and `last_seen_at >= now() - 30 days`; exact scope confirmed against Finding model scopes during implementation.
|
||||
- Findings exported are scoped to `status IN (new, acknowledged)` and `last_seen_at >= now() - 30 days`; exact scope confirmed against Finding model scopes during implementation.
|
||||
- ZIP assembly uses PHP ZipArchive with deterministic, alphabetical file insertion order for stable fingerprinting.
|
||||
|
||||
@ -3,7 +3,7 @@ # Tasks: Tenant Review Pack Export v1 (CSV + ZIP)
|
||||
**Input**: Design documents from `/specs/109-review-pack-export/`
|
||||
**Prerequisites**: plan.md, spec.md, research.md, data-model.md, contracts/api-contracts.md, quickstart.md
|
||||
|
||||
**Tests**: Required (Pest Feature tests). Six test files covering generation, download, RBAC, prune, resource, widget, and schedule.
|
||||
**Tests**: Required (Pest Feature tests). Seven test files covering generation, download, RBAC, prune, resource, widget, and schedule.
|
||||
**Operations**: `OperationRun` of type `tenant.review_pack.generate` tracks all generation runs. Failure `reason_code` stored in `context` jsonb column. Active-run dedupe via application-level guard (`scopeActive()`).
|
||||
**RBAC**:
|
||||
- Gate/Policy enforcement: `REVIEW_PACK_VIEW` (list + download), `REVIEW_PACK_MANAGE` (generate + expire).
|
||||
@ -73,7 +73,7 @@ ### Implementation for User Story 1
|
||||
- [ ] T018 [US1] Create `ListReviewPacks` page extending ListRecords in `app/Filament/Resources/ReviewPackResource/Pages/ListReviewPacks.php`
|
||||
- [ ] T019 [US1] Create `ViewReviewPack` page with Infolist layout: status badge (BADGE-001), summary section (data_freshness per-source timestamps, generated_at, expires_at, file_size, sha256), options section (include_pii, include_operations), initiator + OperationRun link; header actions "Download" (REVIEW_PACK_VIEW, visible when ready) + "Regenerate" (REVIEW_PACK_MANAGE, requiresConfirmation when ready pack exists, pre-fills current options, sets previous_fingerprint) in `app/Filament/Resources/ReviewPackResource/Pages/ViewReviewPack.php`
|
||||
- [ ] T020 [P] [US1] Create `TenantReviewPackCard` widget with 5 display states: no pack ("No review pack yet" + Generate CTA), queued/generating (status badge + "Generation in progress"), ready (badge + generated_at + expires_at + file_size + Download action REVIEW_PACK_VIEW + "Generate new" REVIEW_PACK_MANAGE), failed (badge + sanitized reason + Retry=Generate REVIEW_PACK_MANAGE), expired (badge + expiry date + "Generate new" REVIEW_PACK_MANAGE); data = latest ReviewPack for current tenant in `app/Filament/Widgets/TenantReviewPackCard.php`
|
||||
- [ ] T021 [P] [US1] Create generation tests: happy path (job processes, status→ready, file on disk, sha256+file_size populated, notification sent to initiator, OperationRun completed+success), failure path (exception → status→failed, OperationRun failed, reason_code in context, failure notification), empty reports (job succeeds with empty report sections, status→ready), PII redaction (include_pii=false → display_name replaced with placeholder, UUIDs retained), ZIP contents (exactly 7 files in alphabetical order: findings.csv, hardening.json, metadata.json, operations.csv, reports/entra_admin_roles.json, reports/permission_posture.json, summary.json) in `tests/Feature/ReviewPack/ReviewPackGenerationTest.php`
|
||||
- [ ] T021 [P] [US1] Create generation tests: happy path (job processes, status→ready, file on disk, sha256+file_size populated, notification sent to initiator, OperationRun completed+success), failure path (exception → status→failed, OperationRun failed, reason_code in context, failure notification), empty reports (job succeeds with empty report sections, status→ready), PII redaction (include_pii=false → display_name replaced with placeholder, UUIDs retained), ZIP contents (exactly 7 files in alphabetical order: findings.csv, hardening.json, metadata.json, operations.csv, reports/entra_admin_roles.json, reports/permission_posture.json, summary.json), performance baseline (seed 1,000 findings + 10 stored reports and assert job completes within 60s per SC-001) in `tests/Feature/ReviewPack/ReviewPackGenerationTest.php`
|
||||
- [ ] T022 [P] [US1] Create download tests: signed URL → 200 with Content-Type application/zip + Content-Disposition + X-Review-Pack-SHA256 headers; expired signature → 403; expired pack (status=expired) → 404; non-ready pack (status=queued) → 404; non-existent pack → 404 in `tests/Feature/ReviewPack/ReviewPackDownloadTest.php`
|
||||
- [ ] T023 [P] [US1] Create resource Livewire tests: list page renders with columns, empty state shows CTA, generate action modal opens with toggle fields, view page displays infolist sections with correct data, expire action requires confirmation and updates status in `tests/Feature/ReviewPack/ReviewPackResourceTest.php`
|
||||
- [ ] T024 [P] [US1] Create widget Livewire tests: no-pack state shows generate CTA, ready state shows download + generate actions, generating state shows in-progress message, failed state shows retry action, expired state shows generate action in `tests/Feature/ReviewPack/ReviewPackWidgetTest.php`
|
||||
|
||||
Loading…
Reference in New Issue
Block a user