From e9994aa5ccda66bd068d1b83405b3ce011cf50ba Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Sun, 11 Jan 2026 00:35:29 +0100 Subject: [PATCH 1/7] spec: refine 048 guardrails --- .../checklists/requirements.md | 34 ++++ .../contracts/admin-pages.openapi.yaml | 42 +++++ .../data-model.md | 59 +++++++ .../plan.md | 82 +++++++++ .../quickstart.md | 35 ++++ .../research.md | 45 +++++ .../spec.md | 127 ++++++++++++++ .../tasks.md | 156 ++++++++++++++++++ 8 files changed, 580 insertions(+) create mode 100644 specs/048-backup-restore-ui-graph-safety/checklists/requirements.md create mode 100644 specs/048-backup-restore-ui-graph-safety/contracts/admin-pages.openapi.yaml create mode 100644 specs/048-backup-restore-ui-graph-safety/data-model.md create mode 100644 specs/048-backup-restore-ui-graph-safety/plan.md create mode 100644 specs/048-backup-restore-ui-graph-safety/quickstart.md create mode 100644 specs/048-backup-restore-ui-graph-safety/research.md create mode 100644 specs/048-backup-restore-ui-graph-safety/spec.md create mode 100644 specs/048-backup-restore-ui-graph-safety/tasks.md diff --git a/specs/048-backup-restore-ui-graph-safety/checklists/requirements.md b/specs/048-backup-restore-ui-graph-safety/checklists/requirements.md new file mode 100644 index 0000000..93b5d4a --- /dev/null +++ b/specs/048-backup-restore-ui-graph-safety/checklists/requirements.md @@ -0,0 +1,34 @@ +# Specification Quality Checklist: Backup/Restore UI Graph-Safety (Phase 1) + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-01-10 +**Feature**: specs/048-backup-restore-ui-graph-safety/spec.md + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- Phase 1 intentionally does not refactor action handlers (covered in Spec 049). diff --git a/specs/048-backup-restore-ui-graph-safety/contracts/admin-pages.openapi.yaml b/specs/048-backup-restore-ui-graph-safety/contracts/admin-pages.openapi.yaml new file mode 100644 index 0000000..763c85b --- /dev/null +++ b/specs/048-backup-restore-ui-graph-safety/contracts/admin-pages.openapi.yaml @@ -0,0 +1,42 @@ +openapi: 3.0.3 +info: + title: TenantPilot Admin Page Render Contracts (Feature 048) + version: 0.1.0 + description: | + Minimal HTTP contracts for the Filament pages that must render without touching Microsoft Graph. + +servers: + - url: / + +paths: + /admin/t/{tenantExternalId}/backup-sets: + get: + operationId: backupSetsIndex + summary: Backup Sets index (tenant-scoped) + parameters: + - name: tenantExternalId + in: path + required: true + schema: + type: string + responses: + '200': + description: Page renders successfully. + '302': + description: Redirect to login when unauthenticated. + + /admin/t/{tenantExternalId}/restore-runs/create: + get: + operationId: restoreRunWizardCreate + summary: Restore Run wizard (create) page (tenant-scoped) + parameters: + - name: tenantExternalId + in: path + required: true + schema: + type: string + responses: + '200': + description: Page renders successfully. + '302': + description: Redirect to login when unauthenticated. diff --git a/specs/048-backup-restore-ui-graph-safety/data-model.md b/specs/048-backup-restore-ui-graph-safety/data-model.md new file mode 100644 index 0000000..0c1923b --- /dev/null +++ b/specs/048-backup-restore-ui-graph-safety/data-model.md @@ -0,0 +1,59 @@ +# Data Model: Backup/Restore UI Graph-Safety (048) + +This phase introduces **no schema changes**. It hardens UI render boundaries and adds guard tests. + +## Existing Entities (used by this feature) + +### Tenant +- Purpose: Tenancy boundary for all reads/writes. +- Notes: + - Filament panel is tenant-scoped via `external_id` slug. + +### User +- Purpose: Authenticated actor; has tenant memberships/roles. + +### BackupSet (`backup_sets`) +- Fields (selected): + - `id`, `tenant_id` + - `name`, `created_by`, `status`, `item_count`, `completed_at` + - `metadata` (JSON) + - `deleted_at` (soft deletes, via housekeeping migration) +- Indexes: `tenant_id,status`, `completed_at` +- Used in UI: + - Backup Sets index (table rendering must be DB-only) + +### BackupItem (`backup_items`) +- Fields (selected): + - `id`, `tenant_id`, `backup_set_id` + - `policy_id` (nullable), `policy_version_id` (nullable) + - `policy_identifier`, `policy_type`, `platform` + - `captured_at` + - `payload` (JSON), `metadata` (JSON), `assignments` (JSON) + - `deleted_at` (soft deletes) +- Constraints: + - Unique: `(backup_set_id, policy_identifier, policy_type)` +- Used in UI: + - Restore wizard item selection + metadata display (must not resolve external names via Graph) + +### RestoreRun (`restore_runs`) +- Fields (selected): + - `id`, `tenant_id`, `backup_set_id` + - `requested_by`, `is_dry_run`, `status` + - `requested_items` (JSON), `preview` (JSON), `results` (JSON) + - `group_mapping` (JSON) + - `failure_reason`, `started_at`, `completed_at`, `metadata` (JSON) + - `deleted_at` (soft deletes) +- Used in UI: + - Restore wizard render (Create page) including group mapping controls + +## Relationships (high-level) + +- `Tenant` has many `BackupSet`, `BackupItem`, `RestoreRun`. +- `BackupSet` has many `BackupItem`. +- `RestoreRun` belongs to `BackupSet`. + +## Validation & State Transitions (relevant to this phase) + +- No new state transitions introduced. +- The key invariant enforced by tests: + - Rendering Backup/Restore UI pages must not invoke `GraphClientInterface`. diff --git a/specs/048-backup-restore-ui-graph-safety/plan.md b/specs/048-backup-restore-ui-graph-safety/plan.md new file mode 100644 index 0000000..1389b2e --- /dev/null +++ b/specs/048-backup-restore-ui-graph-safety/plan.md @@ -0,0 +1,82 @@ +# Implementation Plan: Backup/Restore UI Graph-Safety (Phase 1) + +**Branch**: `feat/048-backup-restore-ui-graph-safety` | **Date**: 2026-01-11 | **Spec**: specs/048-backup-restore-ui-graph-safety/spec.md +**Input**: Feature specification from `specs/048-backup-restore-ui-graph-safety/spec.md` + +## Summary + +Ensure Backup/Restore Filament UI is Graph-safe by construction: + +- No Microsoft Graph calls during render/mount/options/typeahead. +- Restore wizard group mapping UI shows DB-only fallback labels (`…`) instead of resolving names via Graph. +- Add fail-hard Pest feature tests that bind `GraphClientInterface` to throw and still allow: + - Backup Sets index to render (HTTP 200 + stable marker) + - Restore wizard to render (HTTP 200 + stable marker) + +## Technical Context + +**Language/Version**: PHP 8.2+ (repo guidance targets PHP 8.4.x) +**Primary Dependencies**: Laravel 12, Filament 4, Livewire 3 +**Storage**: PostgreSQL (JSON columns used for snapshots/preview/results/mappings) +**Testing**: Pest 4 (via `php artisan test`), PHPUnit 12 under the hood +**Target Platform**: Sail-first local dev (Docker), Dokploy-first staging/prod (containers) +**Project Type**: Laravel monolith (Filament admin UI + jobs/services) +**Performance Goals**: N/A (UI correctness + safety) +**Constraints**: +- UI render must be DB-only (no Graph in request lifecycle) +- No new tables/migrations in Phase 1 +- Keep surface area minimal and low-risk +**Scale/Scope**: Multi-tenant admin app; tests must enforce tenant scoping and guardrails + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +- Inventory-first: PASS (this phase only hardens UI render boundaries; no changes to SoT semantics) +- Read/write separation: PASS (no restore execution changes; tests cover render only) +- Single contract path to Graph: PASS (goal is “no Graph in UI render”; Graph stays behind `GraphClientInterface`) +- Deterministic capabilities: PASS (no capability derivation changes) +- Tenant isolation: PASS (tests use tenant-scoped URLs and seeded tenant data) +- Automation idempotent/observable: PASS (no job/scheduler changes in Phase 1) +- Data minimization & safe logging: PASS (no new stored data or logs) + +## Project Structure + +### Documentation (this feature) + +```text +specs/048-backup-restore-ui-graph-safety/ +├── plan.md # This file (/speckit.plan output) +├── research.md # Phase 0 output +├── data-model.md # Phase 1 output +├── quickstart.md # Phase 1 output +├── contracts/ # Phase 1 output +└── tasks.md # Task breakdown (already present) +``` + +### Source Code (repository root) + +```text +app/ +├── Filament/ +│ ├── Resources/ +│ │ ├── BackupSetResource.php +│ │ └── RestoreRunResource.php +│ └── Resources/*/Pages/ +├── Providers/ +│ ├── AppServiceProvider.php # GraphClientInterface binding +│ └── Filament/AdminPanelProvider.php # panel path + tenant routing +├── Services/ +│ └── Graph/ +│ ├── GraphClientInterface.php +│ └── NullGraphClient.php + +database/migrations/ +tests/Feature/ +``` + +**Structure Decision**: Laravel monolith (Filament admin UI + services). No new top-level folders. + +## Complexity Tracking + +None. diff --git a/specs/048-backup-restore-ui-graph-safety/quickstart.md b/specs/048-backup-restore-ui-graph-safety/quickstart.md new file mode 100644 index 0000000..fb8d921 --- /dev/null +++ b/specs/048-backup-restore-ui-graph-safety/quickstart.md @@ -0,0 +1,35 @@ +# Quickstart: Backup/Restore UI Graph-Safety (048) + +This quickstart is for validating the **UI render Graph-safety** guardrails locally. + +## Prerequisites + +- Laravel Sail running (recommended) +- Database migrated + +## Local setup (Sail) + +1) Start Sail + +- `./vendor/bin/sail up -d` + +2) Run migrations + +- `./vendor/bin/sail artisan migrate` + +## Run the targeted tests (once implemented) + +- `./vendor/bin/sail artisan test --filter=graph\-safety` + +Or run specific files (names TBD when tests land): + +- `./vendor/bin/sail artisan test tests/Feature/Filament/*GraphSafety*Test.php` + +## Formatting + +- `./vendor/bin/pint --dirty` + +## Notes + +- These guard tests intentionally fail if any Backup/Restore page render path touches `App\\Services\\Graph\\GraphClientInterface`. +- Graph credentials are not required for this phase; tests should pass with Graph effectively disabled. diff --git a/specs/048-backup-restore-ui-graph-safety/research.md b/specs/048-backup-restore-ui-graph-safety/research.md new file mode 100644 index 0000000..8768113 --- /dev/null +++ b/specs/048-backup-restore-ui-graph-safety/research.md @@ -0,0 +1,45 @@ +# Research: Backup/Restore UI Graph-Safety (048) + +## Decision: Enforce Graph-safety via fail-hard feature renders + +- **Decision**: Add Pest *feature* tests that `actingAs(...)` and `GET` Filament page URLs while binding `App\Services\Graph\GraphClientInterface` to a fail-hard implementation (throws on any call). +- **Rationale**: A full HTTP render exercises the real Filament request lifecycle (middleware, tenancy, resource/page boot) and will fail immediately if any UI render path touches Graph. +- **Alternatives considered**: + - Livewire component tests only → can miss route/middleware/tenancy boot and won’t reflect “real render” regressions as reliably. + - Binding Graph to `NullGraphClient` → would allow silent Graph usage to slip through. + +## Decision: Use `Resource::getUrl()` for stable Filament routes (tenant-scoped) + +- **Decision**: Use Filament’s URL helpers in tests: + - `BackupSetResource::getUrl('index', tenant: $tenant)` + - `RestoreRunResource::getUrl('create', tenant: $tenant)` +- **Rationale**: Avoids hardcoding route paths and keeps tests resilient to panel path / tenancy prefix changes. +- **Repo evidence**: + - `tests/Feature/Filament/InventorySyncRunResourceTest.php` uses `->get(InventorySyncRunResource::getUrl('index', tenant: $tenant))`. +- **Alternatives considered**: + - Hardcoded `/admin/t/{tenant}/...` paths → brittle if the panel path or tenant prefix changes. + +## Decision: Assert HTTP 200 + stable marker + +- **Decision**: Guard tests assert `->assertOk()` plus a stable marker string per page. +- **Rationale**: Reduces false positives (e.g., a redirect to login) and makes failures easier to diagnose. +- **Alternatives considered**: + - Status-only (200) → may still pass with empty/partial output or wrong page. + +## Decision: Fallback label masking format + +- **Decision**: Mask unresolved external IDs as `…` (last 8 characters, prefixed with ellipsis). +- **Rationale**: Keeps UI readable while avoiding full identifier disclosure. +- **Alternatives considered**: + - Full ID → increases accidental disclosure. + - Hash → less readable for operators. + +## Decision: Filament panel + tenancy routing assumptions + +- **Decision**: Treat the Filament admin panel as tenant-scoped under the configured path/prefix: + - Panel path: `admin` + - Tenant route prefix: `t` + - Tenant slug attribute: `external_id` +- **Rationale**: This is the repo’s current canonical setup (`App\Providers\Filament\AdminPanelProvider`). +- **Alternatives considered**: + - Non-tenant-scoped pages → not applicable (TenantPilot is tenant-first). diff --git a/specs/048-backup-restore-ui-graph-safety/spec.md b/specs/048-backup-restore-ui-graph-safety/spec.md new file mode 100644 index 0000000..617eb24 --- /dev/null +++ b/specs/048-backup-restore-ui-graph-safety/spec.md @@ -0,0 +1,127 @@ +# Feature Specification: Backup/Restore UI Graph-Safety (Phase 1) + +**Feature Branch**: `feat/048-backup-restore-ui-graph-safety` +**Created**: 2026-01-10 +**Status**: Draft + +## Purpose + +Ensure Backup/Restore UI follows the same guardrails as Inventory: + +- UI renders DB-only (no Microsoft Graph calls during render/mount/options/typeahead) +- UI still remains usable with safe fallbacks for unresolved external identifiers +- Add programmatic guard tests that fail if Graph is touched while rendering + +This phase is intentionally minimal-change and high-safety. + +## Clarifications + +### Session 2026-01-10 + +- Q: Which pages must the fail-hard `GraphClientInterface` guard tests render? → A: Backup Sets index + Restore wizard. +- Q: How should `` be formatted in fallback labels? → A: `…` + +### Session 2026-01-11 + +- Q: How should the fail-hard Graph guard tests be structured? → A: Feature tests that `actingAs(...)` then `GET` the Filament pages’ routes. +- Q: For the feature tests, should we assert only HTTP 200, or also a stable UI marker? → A: Assert HTTP 200 + a stable page marker string. + +## Users + +- Tenant Admin +- MSP Operator (within authorized tenant scope) + +## User Stories + +### US1 (P1): Backup Sets Index is Graph-safe + +As a tenant admin, I can open the Backup Sets index page and it renders DB-only (no Graph calls during request/render/mount/options/typeahead). + +**Maps to**: Scenario 1 + +### US2 (P1): Restore Wizard is Graph-safe incl. Group Mapping UI + +As a tenant admin, I can open the Restore wizard page and the group mapping UI remains usable without any Graph calls (including typeahead/search/label resolution). + +**Maps to**: Scenario 2 + Scenario 3 + +## User Scenarios & Testing + +### Scenario 1: Open Backup pages without Graph access +- Given a user is authenticated and has access to a tenant +- When they open the Backup Sets index page +- Then the page renders successfully (HTTP 200) without any Graph calls + +### Scenario 2: Open Restore Wizard without Graph access +- Given a user is authenticated and has access to a tenant +- When they open the Restore Run wizard page +- Then the wizard renders successfully (HTTP 200) without any Graph calls + +### Scenario 3: Group mapping shows safe fallback labels +- Given the UI displays group IDs (e.g., in mapping fields) +- When the UI cannot resolve group names without Graph +- Then it shows safe fallback labels like: + - `Unresolved (…)` + - `Group (external): …` + +## Stable Page Marker Rules (for Guard Tests) + +Guard tests MUST assert a stable, static marker string in addition to HTTP 200. + +Marker selection rules: + +- MUST be static UI text that is not tenant data (avoid names, IDs, timestamps, counts) +- SHOULD be a column label, section heading, or primary action label rendered by Filament +- MUST be present on the initial GET without requiring any Livewire interaction +- MUST NOT depend on Microsoft Graph availability + +Markers MUST be recorded in quickstart.md once chosen. + +## Functional Requirements + +- FR-001: Backup/Restore UI MUST NOT call Microsoft Graph during render/mount/options/typeahead. +- FR-002: The Restore Wizard group mapping controls MUST NOT call Graph for search results or label resolution. + +### Group Mapping UX (DB-only) + +When group mapping is required, the UI MUST remain Graph-free and MUST provide a DB-only safe input. + +- For each unresolved source group, the UI shows a mapping row with: + - Source label: ` (…)` + - Target input: + - Allowed values: + - `SKIP` (skip assignment) + - A manually entered Entra ID group objectId (GUID/UUID string) +- Validation rules: + - If not `SKIP`, the value MUST be a syntactically valid UUID + - No network lookup/verification is performed in Phase 1 (Graph-free). Existence is validated later during preview/restore execution paths. +- Helper text MUST explain: + - “Paste the target Entra ID group Object ID (GUID). Names are not resolved in this phase.” + - “Use SKIP to omit the assignment.” + +- FR-003: When names cannot be resolved DB-only, the UI MUST show safe fallback labels using masked identifiers. + - `` format: `…` (last 8 characters of the external identifier, prefixed with an ellipsis) +- FR-004: Add fail-hard Pest feature tests binding `GraphClientInterface` to throw on any invocation and verify: + - Backup Sets index renders OK (HTTP 200 + stable page marker) + - Restore wizard renders OK (HTTP 200 + stable page marker) + +## Non-Functional Requirements + +- NFR-001: No new Graph calls are introduced in UI code paths. +- NFR-002: No new tables are added in this phase. +- NFR-003: Changes should be low-risk and minimal surface area. + +## Out of Scope + +- Moving action handlers (snapshot capture, backup create, restore rerun, restore dry-run) to jobs. +- Creating new cache/inventory tables to support group search. + +## Success Criteria + +- SC-001: Guard tests reliably fail if any Backup/Restore UI render path touches Graph. +- SC-002: Backup and Restore wizard pages render successfully with Graph disabled. +- SC-003: Group mapping UI remains usable with DB-only fallback labels. + +## Related Specs + +- Follow-up (Phase 2): Backup/Restore job orchestration (TBD) diff --git a/specs/048-backup-restore-ui-graph-safety/tasks.md b/specs/048-backup-restore-ui-graph-safety/tasks.md new file mode 100644 index 0000000..8347554 --- /dev/null +++ b/specs/048-backup-restore-ui-graph-safety/tasks.md @@ -0,0 +1,156 @@ +# Tasks: Backup/Restore UI Graph-Safety (Phase 1) + +**Input**: Design documents from `/specs/048-backup-restore-ui-graph-safety/` +**Prerequisites**: plan.md (required), spec.md (required), research.md, data-model.md, contracts/, quickstart.md + +**Tests**: REQUIRED (Pest). This feature explicitly adds guard tests (FR-004). + +**Organization**: Tasks are grouped by user story to enable independent implementation and testing. + +## Format: `- [ ] T### [P?] [US#?] Description with file path` + +- **[P]**: Can run in parallel (different files, no dependencies) +- **[US#]**: Which user story this task belongs to (US1, US2) +- Include exact file paths in descriptions + +--- + +## Phase 1: Setup (Shared Infrastructure) + +**Purpose**: Confirm scope, lock stable UI markers as concrete strings, and ensure the contracts/quickstart reflect the intended test approach. + +- [ ] T001 Confirm tenant-scoped admin URLs for target pages in specs/048-backup-restore-ui-graph-safety/contracts/admin-pages.openapi.yaml +- [ ] T002 Lock stable marker strings and record them in specs/048-backup-restore-ui-graph-safety/quickstart.md: + - Backup Sets index marker: `Created by` + - Restore wizard create marker: `Create restore run` (and wizard step: `Select Backup Set`) + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: Shared test helpers and a clear boundary that fails-hard when Graph is touched. + +**⚠️ CRITICAL**: No user story work should be considered “done” unless the fail-hard Graph binding is used in the story’s feature tests. + +- [ ] T003 [P] Create a fail-hard Graph client test double in tests/Support/FailHardGraphClient.php (implements App\\Services\\Graph\\GraphClientInterface and throws on any method) +- [ ] T004 Add a reusable binding helper for tests (either a helper function in tests/Pest.php or a trait in tests/Support/) that binds GraphClientInterface to FailHardGraphClient + +**Checkpoint**: Foundation ready — both page-render tests can now be implemented. + +--- + +## Phase 3: User Story 1 — Backup Sets index renders Graph-free (Priority: P1) 🎯 MVP + +**Goal**: As a tenant admin, I can open the Backup Sets index page even when Graph is disabled. + +**Independent Test**: A Pest feature test does an HTTP GET to the tenant-scoped Filament Backup Sets index route and asserts assertOk() + assertSee('Created by') — with Graph bound to fail-hard. + +### Tests + +- [ ] T005 [P] [US1] Add Pest feature test in tests/Feature/Filament/BackupSetGraphSafetyTest.php: + - bind GraphClientInterface to FailHardGraphClient (fail-hard on ANY invocation) + - HTTP GET App\\Filament\\Resources\\BackupSetResource::getUrl('index', tenant: $tenant) + - assertOk() + assertSee('Created by') +- [ ] T006 [US1] In tests/Feature/Filament/BackupSetGraphSafetyTest.php, add tenant isolation assertions (second tenant data must not render) while still using fail-hard Graph binding + +### Implementation + +- [ ] T007 [US1] Audit Backup Sets render path for any Graph usage and refactor to DB-only if needed in app/Filament/Resources/BackupSetResource.php and app/Filament/Resources/BackupSetResource/Pages/ListBackupSets.php + +**Checkpoint**: Backup Sets index renders (assertOk() + assertSee('Created by')) with fail-hard Graph binding. + +--- + +## Phase 4: User Story 2 — Restore wizard renders Graph-free + DB-only group mapping (Priority: P1) + +**Goal**: As a tenant admin, I can open the Restore wizard page and interact with group mapping without any Graph calls in render/mount/options/typeahead. + +**Independent Test**: Pest feature tests that (a) render the Restore wizard create page without Graph, and (b) render the group mapping section (using query params to preselect a backup set with group assignments) and verify fallback labels use `…`. + +### Tests + +- [ ] T008 [P] [US2] Add Pest feature test in tests/Feature/Filament/RestoreWizardGraphSafetyTest.php: + - bind GraphClientInterface to FailHardGraphClient (fail-hard on ANY invocation) + - HTTP GET App\\Filament\\Resources\\RestoreRunResource::getUrl('create', tenant: $tenant) + - assertOk() + assertSee('Create restore run') + assertSee('Select Backup Set') +- [ ] T009 [P] [US2] Extend tests/Feature/Filament/RestoreWizardGraphSafetyTest.php (or a second file): + - seed a BackupSet + BackupItem with group assignment targets (groupId present) + - HTTP GET create URL with `?backup_set_id=` (and optional `backup_item_ids`/`scope_mode`) to force group mapping render + - keep fail-hard Graph binding (no Graph/typeahead/label resolution allowed) +- [ ] T010 [US2] In the group mapping render test, assert all DB-only UX requirements: + - assertSee('…') masked fallback appears for source labels + - assertSee('Paste the target Entra ID group Object ID') helper text appears + - assertSee('Use SKIP to omit the assignment.') helper text appears + +### Implementation + +- [ ] T011 [US2] Remove Graph-dependent typeahead/search from group mapping controls in app/Filament/Resources/RestoreRunResource.php (no Graph/typeahead; remove getSearchResultsUsing paths) +- [ ] T012 [US2] Remove Graph-dependent option label resolution in app/Filament/Resources/RestoreRunResource.php (no Graph label resolution; remove getOptionLabelUsing paths) +- [ ] T013 [US2] Implement DB-only group mapping UX in app/Filament/Resources/RestoreRunResource.php: + - manual target group objectId input (GUID/UUID) + - GUID validation (if not SKIP) + - helper text: “Paste the target Entra ID group Object ID (GUID). Names are not resolved in this phase.” + “Use SKIP to omit the assignment.” + - no Graph/typeahead +- [ ] T014 [US2] Make unresolved group detection DB-only in app/Filament/Resources/RestoreRunResource.php (remove GroupResolver usage from unresolvedGroups() and any other render-time helpers) +- [ ] T015 [US2] Implement masked fallback label formatting `…` in app/Filament/Resources/RestoreRunResource.php (update formatGroupLabel() and ensure all source labels route through it) +- [ ] T016 [US2] Remove now-unused methods/imports after refactor (e.g., targetGroupOptions(), resolveTargetGroupLabel(), GroupResolver import) in app/Filament/Resources/RestoreRunResource.php + +**Checkpoint**: Restore wizard renders (assertOk() + assertSee('Create restore run') + assertSee('Select Backup Set')) and group mapping renders DB-only; tests pass with fail-hard Graph binding. + +--- + +## Phase 5: Polish & Cross-Cutting Concerns + +**Purpose**: Keep docs and tooling aligned with the guardrail. + +- [ ] T017 [P] Update specs/048-backup-restore-ui-graph-safety/quickstart.md with the final test file names and the exact `artisan test --filter=...` / file commands +- [ ] T018 [P] Update specs/048-backup-restore-ui-graph-safety/contracts/admin-pages.openapi.yaml if any page paths/markers changed during implementation +- [ ] T019 Run formatting (./vendor/bin/pint --dirty) and targeted tests (./vendor/bin/sail artisan test --filter=graph\-safety or the exact files) + +--- + +## Dependencies & Execution Order + +### Phase Dependencies + +- **Setup (Phase 1)**: No dependencies +- **Foundational (Phase 2)**: Depends on Setup completion — BLOCKS both user stories +- **US1 + US2 (Phases 3–4)**: Depend on Foundational completion; can proceed in parallel +- **Polish (Phase 5)**: Depends on desired user stories being complete + +### User Story Dependencies + +- **US1 (P1)**: Depends on T003–T004; otherwise independent +- **US2 (P1)**: Depends on T003–T004; otherwise independent + +### Parallel Opportunities + +- T003 and T004 can be developed in parallel if split across files. +- US1 test work (T005–T006) and US2 test work (T008–T010) can be developed in parallel. + +--- + +## Parallel Example + +```bash +# Parallelizable work after Phase 2: +# - US1: tests/Feature/Filament/BackupSetGraphSafetyTest.php +# - US2: tests/Feature/Filament/RestoreWizardGraphSafetyTest.php +# - Shared helper: tests/Support/FailHardGraphClient.php +``` + +--- + +## Implementation Strategy + +### MVP Scope (Recommended) + +1. Phase 1–2 (setup + fail-hard Graph test binding) +2. Phase 3 (US1: Backup Sets index renders Graph-free) +3. Validate tests + stop + +### Then + +4. Phase 4 (US2: Restore wizard + group mapping Graph-free) +5. Phase 5 polish + -- 2.45.2 From bf183347ac269c1d7e5a93943e5a78cca5200888 Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Sun, 11 Jan 2026 01:06:51 +0100 Subject: [PATCH 2/7] feat(048): enforce graph-safe backup/restore UI Add fail-hard Graph guard tests for Backup Sets index + Restore wizard create, and refactor restore group mapping to be DB-only with masked fallback labels. --- app/Filament/Resources/RestoreRunResource.php | 205 +++++++----------- .../quickstart.md | 7 +- .../tasks.md | 40 ++-- .../Filament/BackupSetGraphSafetyTest.php | 34 +++ .../Filament/RestoreWizardGraphSafetyTest.php | 65 ++++++ tests/Pest.php | 7 + tests/Support/FailHardGraphClient.php | 45 ++++ 7 files changed, 259 insertions(+), 144 deletions(-) create mode 100644 tests/Feature/Filament/BackupSetGraphSafetyTest.php create mode 100644 tests/Feature/Filament/RestoreWizardGraphSafetyTest.php create mode 100644 tests/Support/FailHardGraphClient.php diff --git a/app/Filament/Resources/RestoreRunResource.php b/app/Filament/Resources/RestoreRunResource.php index ff2500b..516e6c1 100644 --- a/app/Filament/Resources/RestoreRunResource.php +++ b/app/Filament/Resources/RestoreRunResource.php @@ -12,8 +12,6 @@ use App\Models\RestoreRun; use App\Models\Tenant; use App\Services\BulkOperationService; -use App\Services\Graph\GraphClientInterface; -use App\Services\Graph\GroupResolver; use App\Services\Intune\AuditLogger; use App\Services\Intune\RestoreDiffGenerator; use App\Services\Intune\RestoreRiskChecker; @@ -110,20 +108,40 @@ public static function form(Schema $schema): Schema tenant: $tenant ); - return array_map(function (array $group) use ($tenant): Forms\Components\Select { + return array_map(function (array $group): Forms\Components\TextInput { $groupId = $group['id']; $label = $group['label']; - return Forms\Components\Select::make("group_mapping.{$groupId}") + return Forms\Components\TextInput::make("group_mapping.{$groupId}") ->label($label) - ->options([ - 'SKIP' => 'Skip assignment', + ->placeholder('SKIP or target group Object ID (GUID)') + ->rules([ + static function (string $attribute, mixed $value, \Closure $fail): void { + if (! is_string($value)) { + $fail('Please enter SKIP or a valid UUID.'); + + return; + } + + $value = trim($value); + + if ($value === '') { + $fail('Please enter SKIP or a valid UUID.'); + + return; + } + + if (strtoupper($value) === 'SKIP') { + return; + } + + if (! Str::isUuid($value)) { + $fail('Please enter SKIP or a valid UUID.'); + } + }, ]) - ->searchable() - ->getSearchResultsUsing(fn (string $search) => static::targetGroupOptions($tenant, $search)) - ->getOptionLabelUsing(fn ($value) => static::resolveTargetGroupLabel($tenant, $value)) ->required() - ->helperText('Choose a target group or select Skip.'); + ->helperText('Paste the target Entra ID group Object ID (GUID). Names are not resolved in this phase. Use SKIP to omit the assignment.'); }, $unresolved); }) ->visible(function (Get $get): bool { @@ -272,29 +290,29 @@ public static function getWizardSteps(): array ->visible(fn (Get $get): bool => $get('scope_mode') === 'selected') ->required(fn (Get $get): bool => $get('scope_mode') === 'selected') ->hintActions([ - Actions\Action::make('select_all_backup_items') - ->label('Select all') - ->icon('heroicon-o-check') - ->color('gray') - ->visible(fn (Get $get): bool => filled($get('backup_set_id')) && $get('scope_mode') === 'selected') - ->action(function (Get $get, Set $set): void { - $groupedOptions = static::restoreItemGroupedOptions($get('backup_set_id')); + Actions\Action::make('select_all_backup_items') + ->label('Select all') + ->icon('heroicon-o-check') + ->color('gray') + ->visible(fn (Get $get): bool => filled($get('backup_set_id')) && $get('scope_mode') === 'selected') + ->action(function (Get $get, Set $set): void { + $groupedOptions = static::restoreItemGroupedOptions($get('backup_set_id')); - $allItemIds = []; + $allItemIds = []; - foreach ($groupedOptions as $options) { - $allItemIds = array_merge($allItemIds, array_keys($options)); - } + foreach ($groupedOptions as $options) { + $allItemIds = array_merge($allItemIds, array_keys($options)); + } - $set('backup_item_ids', array_values($allItemIds), shouldCallUpdatedHooks: true); - }), - Actions\Action::make('clear_backup_items') - ->label('Clear') - ->icon('heroicon-o-x-mark') - ->color('gray') - ->visible(fn (Get $get): bool => $get('scope_mode') === 'selected') - ->action(fn (Set $set) => $set('backup_item_ids', [], shouldCallUpdatedHooks: true)), - ]) + $set('backup_item_ids', array_values($allItemIds), shouldCallUpdatedHooks: true); + }), + Actions\Action::make('clear_backup_items') + ->label('Clear') + ->icon('heroicon-o-x-mark') + ->color('gray') + ->visible(fn (Get $get): bool => $get('scope_mode') === 'selected') + ->action(fn (Set $set) => $set('backup_item_ids', [], shouldCallUpdatedHooks: true)), + ]) ->helperText('Search by name or ID. Include foundations (scope tags, assignment filters) with policies to re-map IDs. Options are grouped by category, type, and platform.'), Section::make('Group mapping') ->description('Some source groups do not exist in the target tenant. Map them or choose Skip.') @@ -320,18 +338,38 @@ public static function getWizardSteps(): array tenant: $tenant ); - return array_map(function (array $group) use ($tenant): Forms\Components\Select { + return array_map(function (array $group): Forms\Components\TextInput { $groupId = $group['id']; $label = $group['label']; - return Forms\Components\Select::make("group_mapping.{$groupId}") + return Forms\Components\TextInput::make("group_mapping.{$groupId}") ->label($label) - ->options([ - 'SKIP' => 'Skip assignment', + ->placeholder('SKIP or target group Object ID (GUID)') + ->rules([ + static function (string $attribute, mixed $value, \Closure $fail): void { + if (! is_string($value)) { + $fail('Please enter SKIP or a valid UUID.'); + + return; + } + + $value = trim($value); + + if ($value === '') { + $fail('Please enter SKIP or a valid UUID.'); + + return; + } + + if (strtoupper($value) === 'SKIP') { + return; + } + + if (! Str::isUuid($value)) { + $fail('Please enter SKIP or a valid UUID.'); + } + }, ]) - ->searchable() - ->getSearchResultsUsing(fn (string $search) => static::targetGroupOptions($tenant, $search)) - ->getOptionLabelUsing(fn (?string $value) => static::resolveTargetGroupLabel($tenant, $value)) ->reactive() ->afterStateUpdated(function (Set $set): void { $set('check_summary', null); @@ -341,7 +379,8 @@ public static function getWizardSteps(): array $set('preview_diffs', []); $set('preview_ran_at', null); }) - ->helperText('Choose a target group or select Skip.'); + ->required() + ->helperText('Paste the target Entra ID group Object ID (GUID). Names are not resolved in this phase. Use SKIP to omit the assignment.'); }, $unresolved); }) ->visible(function (Get $get): bool { @@ -1382,27 +1421,12 @@ private static function unresolvedGroups(?int $backupSetId, ?array $selectedItem return []; } - $graphOptions = $tenant->graphOptions(); - $tenantIdentifier = $graphOptions['tenant'] ?? $tenant->graphTenantId() ?? (string) $tenant->getKey(); - $resolved = app(GroupResolver::class)->resolveGroupIds($groupIds, $tenantIdentifier, $graphOptions); - - $unresolved = []; - - foreach ($groupIds as $groupId) { - $group = $resolved[$groupId] ?? null; - - if (! is_array($group) || ! ($group['orphaned'] ?? false)) { - continue; - } - - $label = static::formatGroupLabel($sourceNames[$groupId] ?? null, $groupId); - $unresolved[] = [ + return array_map(function (string $groupId) use ($sourceNames): array { + return [ 'id' => $groupId, - 'label' => $label, + 'label' => static::formatGroupLabel($sourceNames[$groupId] ?? null, $groupId), ]; - } - - return $unresolved; + }, $groupIds); } /** @@ -1510,73 +1534,10 @@ private static function normalizeGroupMapping(mixed $mapping): array return array_filter($result, static fn (?string $value): bool => is_string($value) && $value !== ''); } - /** - * @return array - */ - private static function targetGroupOptions(Tenant $tenant, string $search): array - { - if (mb_strlen($search) < 2) { - return []; - } - - try { - $response = app(GraphClientInterface::class)->request( - 'GET', - 'groups', - [ - 'query' => [ - '$filter' => sprintf( - "securityEnabled eq true and startswith(displayName,'%s')", - static::escapeOdataValue($search) - ), - '$select' => 'id,displayName', - '$top' => 20, - ], - ] + $tenant->graphOptions() - ); - } catch (\Throwable) { - return []; - } - - if ($response->failed()) { - return []; - } - - return collect($response->data['value'] ?? []) - ->filter(fn (array $group) => filled($group['id'] ?? null)) - ->mapWithKeys(fn (array $group) => [ - $group['id'] => static::formatGroupLabel($group['displayName'] ?? null, $group['id']), - ]) - ->all(); - } - - private static function resolveTargetGroupLabel(Tenant $tenant, ?string $groupId): ?string - { - if (! $groupId) { - return $groupId; - } - - if ($groupId === 'SKIP') { - return 'Skip assignment'; - } - - $graphOptions = $tenant->graphOptions(); - $tenantIdentifier = $graphOptions['tenant'] ?? $tenant->graphTenantId() ?? (string) $tenant->getKey(); - $resolved = app(GroupResolver::class)->resolveGroupIds([$groupId], $tenantIdentifier, $graphOptions); - $group = $resolved[$groupId] ?? null; - - return static::formatGroupLabel($group['displayName'] ?? null, $groupId); - } - private static function formatGroupLabel(?string $displayName, string $id): string { - $suffix = sprintf(' (%s)', Str::limit($id, 8, '')); + $suffix = '…'.mb_substr($id, -8); - return trim(($displayName ?: 'Security group').$suffix); - } - - private static function escapeOdataValue(string $value): string - { - return str_replace("'", "''", $value); + return trim(($displayName ?: 'Security group').' ('.$suffix.')'); } } diff --git a/specs/048-backup-restore-ui-graph-safety/quickstart.md b/specs/048-backup-restore-ui-graph-safety/quickstart.md index fb8d921..32a9359 100644 --- a/specs/048-backup-restore-ui-graph-safety/quickstart.md +++ b/specs/048-backup-restore-ui-graph-safety/quickstart.md @@ -19,11 +19,12 @@ ## Local setup (Sail) ## Run the targeted tests (once implemented) -- `./vendor/bin/sail artisan test --filter=graph\-safety` +- `./vendor/bin/sail artisan test tests/Feature/Filament/BackupSetGraphSafetyTest.php` +- `./vendor/bin/sail artisan test tests/Feature/Filament/RestoreWizardGraphSafetyTest.php` -Or run specific files (names TBD when tests land): +Or run both in one command: -- `./vendor/bin/sail artisan test tests/Feature/Filament/*GraphSafety*Test.php` +- `./vendor/bin/sail artisan test tests/Feature/Filament/BackupSetGraphSafetyTest.php tests/Feature/Filament/RestoreWizardGraphSafetyTest.php` ## Formatting diff --git a/specs/048-backup-restore-ui-graph-safety/tasks.md b/specs/048-backup-restore-ui-graph-safety/tasks.md index 8347554..86391fe 100644 --- a/specs/048-backup-restore-ui-graph-safety/tasks.md +++ b/specs/048-backup-restore-ui-graph-safety/tasks.md @@ -19,8 +19,8 @@ ## Phase 1: Setup (Shared Infrastructure) **Purpose**: Confirm scope, lock stable UI markers as concrete strings, and ensure the contracts/quickstart reflect the intended test approach. -- [ ] T001 Confirm tenant-scoped admin URLs for target pages in specs/048-backup-restore-ui-graph-safety/contracts/admin-pages.openapi.yaml -- [ ] T002 Lock stable marker strings and record them in specs/048-backup-restore-ui-graph-safety/quickstart.md: +- [x] T001 Confirm tenant-scoped admin URLs for target pages in specs/048-backup-restore-ui-graph-safety/contracts/admin-pages.openapi.yaml +- [x] T002 Lock stable marker strings and record them in specs/048-backup-restore-ui-graph-safety/quickstart.md: - Backup Sets index marker: `Created by` - Restore wizard create marker: `Create restore run` (and wizard step: `Select Backup Set`) @@ -32,8 +32,10 @@ ## Phase 2: Foundational (Blocking Prerequisites) **⚠️ CRITICAL**: No user story work should be considered “done” unless the fail-hard Graph binding is used in the story’s feature tests. -- [ ] T003 [P] Create a fail-hard Graph client test double in tests/Support/FailHardGraphClient.php (implements App\\Services\\Graph\\GraphClientInterface and throws on any method) -- [ ] T004 Add a reusable binding helper for tests (either a helper function in tests/Pest.php or a trait in tests/Support/) that binds GraphClientInterface to FailHardGraphClient +- [x] T003 [P] Create a fail-hard Graph client test double in tests/Support/FailHardGraphClient.php (implements App\\Services\\Graph\\GraphClientInterface and throws on any method) +- [x] T004 Add a reusable binding helper for tests (either a helper function in tests/Pest.php or a trait in tests/Support/) that binds GraphClientInterface to FailHardGraphClient + + **Checkpoint**: Foundation ready — both page-render tests can now be implemented. @@ -47,15 +49,15 @@ ## Phase 3: User Story 1 — Backup Sets index renders Graph-free (Priority: P1) ### Tests -- [ ] T005 [P] [US1] Add Pest feature test in tests/Feature/Filament/BackupSetGraphSafetyTest.php: +- [x] T005 [P] [US1] Add Pest feature test in tests/Feature/Filament/BackupSetGraphSafetyTest.php: - bind GraphClientInterface to FailHardGraphClient (fail-hard on ANY invocation) - HTTP GET App\\Filament\\Resources\\BackupSetResource::getUrl('index', tenant: $tenant) - assertOk() + assertSee('Created by') -- [ ] T006 [US1] In tests/Feature/Filament/BackupSetGraphSafetyTest.php, add tenant isolation assertions (second tenant data must not render) while still using fail-hard Graph binding +- [x] T006 [US1] In tests/Feature/Filament/BackupSetGraphSafetyTest.php, add tenant isolation assertions (second tenant data must not render) while still using fail-hard Graph binding ### Implementation -- [ ] T007 [US1] Audit Backup Sets render path for any Graph usage and refactor to DB-only if needed in app/Filament/Resources/BackupSetResource.php and app/Filament/Resources/BackupSetResource/Pages/ListBackupSets.php +- [x] T007 [US1] Audit Backup Sets render path for any Graph usage and refactor to DB-only if needed in app/Filament/Resources/BackupSetResource.php and app/Filament/Resources/BackupSetResource/Pages/ListBackupSets.php **Checkpoint**: Backup Sets index renders (assertOk() + assertSee('Created by')) with fail-hard Graph binding. @@ -69,31 +71,31 @@ ## Phase 4: User Story 2 — Restore wizard renders Graph-free + DB-only group m ### Tests -- [ ] T008 [P] [US2] Add Pest feature test in tests/Feature/Filament/RestoreWizardGraphSafetyTest.php: +- [x] T008 [P] [US2] Add Pest feature test in tests/Feature/Filament/RestoreWizardGraphSafetyTest.php: - bind GraphClientInterface to FailHardGraphClient (fail-hard on ANY invocation) - HTTP GET App\\Filament\\Resources\\RestoreRunResource::getUrl('create', tenant: $tenant) - assertOk() + assertSee('Create restore run') + assertSee('Select Backup Set') -- [ ] T009 [P] [US2] Extend tests/Feature/Filament/RestoreWizardGraphSafetyTest.php (or a second file): +- [x] T009 [P] [US2] Extend tests/Feature/Filament/RestoreWizardGraphSafetyTest.php (or a second file): - seed a BackupSet + BackupItem with group assignment targets (groupId present) - HTTP GET create URL with `?backup_set_id=` (and optional `backup_item_ids`/`scope_mode`) to force group mapping render - keep fail-hard Graph binding (no Graph/typeahead/label resolution allowed) -- [ ] T010 [US2] In the group mapping render test, assert all DB-only UX requirements: +- [x] T010 [US2] In the group mapping render test, assert all DB-only UX requirements: - assertSee('…') masked fallback appears for source labels - assertSee('Paste the target Entra ID group Object ID') helper text appears - assertSee('Use SKIP to omit the assignment.') helper text appears ### Implementation -- [ ] T011 [US2] Remove Graph-dependent typeahead/search from group mapping controls in app/Filament/Resources/RestoreRunResource.php (no Graph/typeahead; remove getSearchResultsUsing paths) -- [ ] T012 [US2] Remove Graph-dependent option label resolution in app/Filament/Resources/RestoreRunResource.php (no Graph label resolution; remove getOptionLabelUsing paths) -- [ ] T013 [US2] Implement DB-only group mapping UX in app/Filament/Resources/RestoreRunResource.php: +- [x] T011 [US2] Remove Graph-dependent typeahead/search from group mapping controls in app/Filament/Resources/RestoreRunResource.php (no Graph/typeahead; remove getSearchResultsUsing paths) +- [x] T012 [US2] Remove Graph-dependent option label resolution in app/Filament/Resources/RestoreRunResource.php (no Graph label resolution; remove getOptionLabelUsing paths) +- [x] T013 [US2] Implement DB-only group mapping UX in app/Filament/Resources/RestoreRunResource.php: - manual target group objectId input (GUID/UUID) - GUID validation (if not SKIP) - helper text: “Paste the target Entra ID group Object ID (GUID). Names are not resolved in this phase.” + “Use SKIP to omit the assignment.” - no Graph/typeahead -- [ ] T014 [US2] Make unresolved group detection DB-only in app/Filament/Resources/RestoreRunResource.php (remove GroupResolver usage from unresolvedGroups() and any other render-time helpers) -- [ ] T015 [US2] Implement masked fallback label formatting `…` in app/Filament/Resources/RestoreRunResource.php (update formatGroupLabel() and ensure all source labels route through it) -- [ ] T016 [US2] Remove now-unused methods/imports after refactor (e.g., targetGroupOptions(), resolveTargetGroupLabel(), GroupResolver import) in app/Filament/Resources/RestoreRunResource.php +- [x] T014 [US2] Make unresolved group detection DB-only in app/Filament/Resources/RestoreRunResource.php (remove GroupResolver usage from unresolvedGroups() and any other render-time helpers) +- [x] T015 [US2] Implement masked fallback label formatting `…` in app/Filament/Resources/RestoreRunResource.php (update formatGroupLabel() and ensure all source labels route through it) +- [x] T016 [US2] Remove now-unused methods/imports after refactor (e.g., targetGroupOptions(), resolveTargetGroupLabel(), GroupResolver import) in app/Filament/Resources/RestoreRunResource.php **Checkpoint**: Restore wizard renders (assertOk() + assertSee('Create restore run') + assertSee('Select Backup Set')) and group mapping renders DB-only; tests pass with fail-hard Graph binding. @@ -103,9 +105,9 @@ ## Phase 5: Polish & Cross-Cutting Concerns **Purpose**: Keep docs and tooling aligned with the guardrail. -- [ ] T017 [P] Update specs/048-backup-restore-ui-graph-safety/quickstart.md with the final test file names and the exact `artisan test --filter=...` / file commands -- [ ] T018 [P] Update specs/048-backup-restore-ui-graph-safety/contracts/admin-pages.openapi.yaml if any page paths/markers changed during implementation -- [ ] T019 Run formatting (./vendor/bin/pint --dirty) and targeted tests (./vendor/bin/sail artisan test --filter=graph\-safety or the exact files) +- [x] T017 [P] Update specs/048-backup-restore-ui-graph-safety/quickstart.md with the final test file names and the exact `artisan test --filter=...` / file commands +- [x] T018 [P] Update specs/048-backup-restore-ui-graph-safety/contracts/admin-pages.openapi.yaml if any page paths/markers changed during implementation +- [x] T019 Run formatting (./vendor/bin/pint --dirty) and targeted tests (./vendor/bin/sail artisan test --filter=graph\-safety or the exact files) --- diff --git a/tests/Feature/Filament/BackupSetGraphSafetyTest.php b/tests/Feature/Filament/BackupSetGraphSafetyTest.php new file mode 100644 index 0000000..5702939 --- /dev/null +++ b/tests/Feature/Filament/BackupSetGraphSafetyTest.php @@ -0,0 +1,34 @@ +create(); + $otherTenant = Tenant::factory()->create(); + + [$user] = createUserWithTenant($tenant); + $user->tenants()->syncWithoutDetaching([ + $otherTenant->getKey() => ['role' => 'owner'], + ]); + + $visibleSet = BackupSet::factory()->create([ + 'tenant_id' => $tenant->getKey(), + 'name' => 'visible-backup-set', + ]); + + $hiddenSet = BackupSet::factory()->create([ + 'tenant_id' => $otherTenant->getKey(), + 'name' => 'hidden-backup-set', + ]); + + bindFailHardGraphClient(); + + $this->actingAs($user) + ->get(BackupSetResource::getUrl('index', tenant: $tenant)) + ->assertOk() + ->assertSee('Created by') + ->assertSee($visibleSet->name) + ->assertDontSee($hiddenSet->name); +}); diff --git a/tests/Feature/Filament/RestoreWizardGraphSafetyTest.php b/tests/Feature/Filament/RestoreWizardGraphSafetyTest.php new file mode 100644 index 0000000..5fa4058 --- /dev/null +++ b/tests/Feature/Filament/RestoreWizardGraphSafetyTest.php @@ -0,0 +1,65 @@ + $odataType, + 'groupId' => $groupId, + ]; + + if (is_string($displayName) && $displayName !== '') { + $target['group_display_name'] = $displayName; + } + + return ['target' => $target]; +} + +test('restore wizard create page renders without touching graph', function () { + $tenant = Tenant::factory()->create(); + [$user] = createUserWithTenant($tenant); + + bindFailHardGraphClient(); + + $this->actingAs($user) + ->get(RestoreRunResource::getUrl('create', tenant: $tenant)) + ->assertOk() + ->assertSee('Create restore run') + ->assertSee('Select Backup Set'); +}); + +test('restore wizard group mapping renders DB-only with manual GUID UX', function () { + $tenant = Tenant::factory()->create(); + [$user] = createUserWithTenant($tenant); + + $backupSet = BackupSet::factory()->create([ + 'tenant_id' => $tenant->getKey(), + 'name' => 'group-mapping-backup-set', + ]); + + $groupId = '11111111-2222-3333-4444-555555555555'; + $expectedMasked = '…'.substr($groupId, -8); + + BackupItem::factory()->create([ + 'tenant_id' => $tenant->getKey(), + 'backup_set_id' => $backupSet->getKey(), + 'assignments' => [ + makeAssignment('#microsoft.graph.groupAssignmentTarget', $groupId, 'Example Group'), + ], + ]); + + bindFailHardGraphClient(); + + $url = RestoreRunResource::getUrl('create', tenant: $tenant).'?backup_set_id='.$backupSet->getKey(); + + $this->actingAs($user) + ->get($url) + ->assertOk() + ->assertSee($expectedMasked) + ->assertSee('Paste the target Entra ID group Object ID') + ->assertSee('Use SKIP to omit the assignment.'); +}); diff --git a/tests/Pest.php b/tests/Pest.php index dccc767..36dc985 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -2,7 +2,9 @@ use App\Models\Tenant; use App\Models\User; +use App\Services\Graph\GraphClientInterface; use Illuminate\Foundation\Testing\RefreshDatabase; +use Tests\Support\FailHardGraphClient; /* |-------------------------------------------------------------------------- @@ -66,6 +68,11 @@ function something() // .. } +function bindFailHardGraphClient(): void +{ + app()->instance(GraphClientInterface::class, new FailHardGraphClient); +} + /** * @return array{0: User, 1: Tenant} */ diff --git a/tests/Support/FailHardGraphClient.php b/tests/Support/FailHardGraphClient.php new file mode 100644 index 0000000..1417c92 --- /dev/null +++ b/tests/Support/FailHardGraphClient.php @@ -0,0 +1,45 @@ +fail(__METHOD__); + } + + public function getPolicy(string $policyType, string $policyId, array $options = []): GraphResponse + { + $this->fail(__METHOD__); + } + + public function getOrganization(array $options = []): GraphResponse + { + $this->fail(__METHOD__); + } + + public function applyPolicy(string $policyType, string $policyId, array $payload, array $options = []): GraphResponse + { + $this->fail(__METHOD__); + } + + public function getServicePrincipalPermissions(array $options = []): GraphResponse + { + $this->fail(__METHOD__); + } + + public function request(string $method, string $path, array $options = []): GraphResponse + { + $this->fail(__METHOD__); + } +} -- 2.45.2 From 99eb45cbd5534826c476d86d14c24b3bc91157db Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Sun, 11 Jan 2026 01:19:59 +0100 Subject: [PATCH 3/7] spec: add 049 backup/restore job orchestration --- .../checklists/requirements.md | 36 +++++ .../spec.md | 148 ++++++++++++++++++ 2 files changed, 184 insertions(+) create mode 100644 specs/049-backup-restore-job-orchestration/checklists/requirements.md create mode 100644 specs/049-backup-restore-job-orchestration/spec.md diff --git a/specs/049-backup-restore-job-orchestration/checklists/requirements.md b/specs/049-backup-restore-job-orchestration/checklists/requirements.md new file mode 100644 index 0000000..b579f09 --- /dev/null +++ b/specs/049-backup-restore-job-orchestration/checklists/requirements.md @@ -0,0 +1,36 @@ +# Specification Quality Checklist: Backup/Restore Job Orchestration (049) + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-01-11 +**Feature**: [specs/049-backup-restore-job-orchestration/spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- Items marked incomplete require spec updates before `/speckit.clarify` or `/speckit.plan` + +- Constitution alignment text references Microsoft Graph; this is a process requirement and not an implementation constraint. diff --git a/specs/049-backup-restore-job-orchestration/spec.md b/specs/049-backup-restore-job-orchestration/spec.md new file mode 100644 index 0000000..8039727 --- /dev/null +++ b/specs/049-backup-restore-job-orchestration/spec.md @@ -0,0 +1,148 @@ +# Feature Specification: Backup/Restore Job Orchestration (049) + +**Feature Branch**: `feat/049-backup-restore-job-orchestration` +**Created**: 2026-01-11 +**Status**: Draft +**Input**: Ensure Backup/Restore “start/execute” actions never run inline in an interactive request; they run via background processing with run records and visible progress. + +## Purpose + +All Backup/Restore “Start/Execute” actions run exclusively via background processing with Run Records and visible progress. This prevents timeouts, double-click duplication, throttling issues, and improves reliability at MSP scale. + +## Non-Goals (Phase 1) + +- No new directory/group inventory or name resolution features (separate initiative) +- No changes to external service contracts unless required for orchestration safety +- No new promotion feature (e.g., DEV→PROD) (separate initiative) + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Capture snapshot runs in background (Priority: P1) + +An admin can start a “capture snapshot” operation without the UI hanging or timing out, and can see progress plus the final result. + +**Why this priority**: Snapshot capture is a core workflow and a common source of long-running requests. + +**Independent Test**: Starting a snapshot capture immediately returns to the UI with a queued Run Record that later transitions to a terminal state (success/failed/partial) and can be inspected. + +**Acceptance Scenarios**: + +1. **Given** an admin has access to a tenant, **When** they start “capture snapshot”, **Then** the UI confirms it was queued and shows a link to the Run Record. +2. **Given** a capture snapshot run is executing, **When** the admin views the run, **Then** they see progress (items done vs total) and any safe error summaries. + +--- + +### User Story 2 - Restore runs in background with per-item results (Priority: P1) + +An admin can start a “restore to Intune” or “re-run restore” operation as a background run and later inspect item-level outcomes and errors. + +**Why this priority**: Restore is high-impact and must be resilient, observable, and safe under retries. + +**Independent Test**: Starting restore creates a Run Record and item results that remain accessible even if the external service is unavailable. + +**Acceptance Scenarios**: + +1. **Given** an admin starts a restore, **When** they confirm the action, **Then** the UI queues a run and returns immediately (no long-running request). +2. **Given** a restore run finishes with mixed outcomes, **When** the admin views the run details, **Then** they see succeeded/failed counts and a safe error summary per failed item. + +--- + +### User Story 3 - Backup set create/capture runs in background (Priority: P2) + +An admin can create a backup set and optionally start a capture/sync operation without the request doing heavy work. + +**Why this priority**: Creating backup sets is frequent and should not be coupled to long-running capture logic. + +**Independent Test**: Creating a backup set returns quickly and any capture/sync work appears as a run with progress. + +**Acceptance Scenarios**: + +1. **Given** an admin creates a backup set with capture enabled, **When** they submit, **Then** the backup set is created and a capture run is queued. + +--- + +### User Story 4 - Dry-run/preview runs in background (Priority: P2) + +An admin can run a dry-run/preview without UI timeouts, and the preview results are persisted and shown in the UI. + +**Why this priority**: Preview supports safe change management and must remain usable even when the external service is slow or down. + +**Independent Test**: Starting preview immediately creates a run; once finished, preview outputs are visible and reusable. + +**Acceptance Scenarios**: + +1. **Given** an admin starts a preview run, **When** the run completes, **Then** the UI shows preview results without requiring re-execution. + +### Edge Cases + +- Double-clicking an action rapidly +- Retrying while an identical run is already queued or running +- External service is unavailable (e.g., throttling or outage) +- A run gets stuck or exceeds expected duration +- Permissions change after a run was queued + +## Requirements *(mandatory)* + +**Constitution alignment (required):** If this feature introduces any Microsoft Graph calls or any write/change behavior, +the spec MUST describe contract registry updates, safety gates (preview/confirmation/audit), tenant isolation, and tests. + +### Functional Requirements + +- **FR-001 Job-only execution**: The system MUST execute the following operations via background processing and MUST NOT perform heavy work inline during the interactive request: + - Capture snapshot + - Backup set create with capture/sync (when capture is triggered) + - Restore to Intune + - Re-run restore + - Restore dry-run/preview + +- **FR-002 Run Records**: Each operation start MUST create (or deterministically re-use) a Run Record before the work begins, containing: + - Tenant identity + - Initiator identity (user reference or audit reference) + - Operation type and optional target object reference + - Status lifecycle: queued → running → (succeeded | failed | partial | canceled) + - Started/finished timestamps + - Item counts: total / succeeded / failed + - Safe error code and safe error context (no secrets) + +- **FR-003 Progress visibility**: While a run is executing, the system MUST provide visible progress in the admin UI and MUST emit in-app notifications for key state transitions (queued/running/completed/failed). + +- **FR-004 Idempotency & concurrency control**: The system MUST prevent uncontrolled duplicate execution due to double-clicks/retries by enforcing a deterministic de-duplication rule keyed by (tenant + operation type + target object) or (tenant + run id). When an identical run is already queued/running, the UI MUST show “already queued/running” and link to the existing run. + +- **FR-005 Deterministic outcome persistence**: The system MUST persist per-item outcomes for operations that act on multiple items, including status and a safe error summary, so results can be viewed later without relying on logs. + +- **FR-006 Tenant isolation & authorization**: Run visibility and execution MUST be tenant-scoped. Only authorized admins can start operations, and users MUST NOT be able to view or start runs across tenants. + +- **FR-007 Safety rules**: Preview/dry-run MUST be safe (no writes). Live restore MUST remain guarded with explicit confirmation and an auditable trail consistent with existing safety practices. + +- **FR-008 Resilience**: The system MUST handle external service throttling/outages gracefully, including retries with backoff when appropriate, and MUST end runs in a clear terminal state (failed/partial) rather than silently failing. + +- **FR-009 Safe logging & data minimization**: The system MUST NOT store secrets/tokens in Run Records, notifications, or error contexts. Error context MUST be limited to a defined, safe set of fields. + +### Acceptance Checks + +- Starting any in-scope operation returns quickly with a queued Run Record link. +- A Run Record always exists before background work begins and reaches a terminal state. +- Progress and state changes are visible in the UI via progress display and in-app notifications. +- Duplicate start attempts for the same tenant + operation + target do not create uncontrolled duplicate execution. +- Item-level outcomes and safe error summaries are viewable after completion. +- Preview/dry-run never performs writes. + +### Key Entities *(include if feature involves data)* + +- **Run Record**: A tenant-scoped record representing one started operation and its lifecycle, progress, and summary outcome. +- **Run Item Result**: A tenant-scoped record representing the outcome for a single item processed as part of a Run Record. +- **Notification Event**: A tenant-scoped event surfaced to the admin UI to communicate run state changes. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: For 95% of operation starts, the UI confirms “queued” within 2 seconds. +- **SC-002**: Double-clicking an operation start results in at most one queued/running run for the same tenant + operation + target. +- **SC-003**: 99% of runs end in a clear terminal state (succeeded/failed/partial/canceled) with a human-readable summary. +- **SC-004**: Admins can locate the latest run status for an operation in under 30 seconds without requiring access to system logs. + +## Assumptions + +- This feature builds on the UI safety constraints from 048: admin pages must remain usable even when the external service API is unavailable. +- Run Records and item results are retained long enough to support operational troubleshooting and audits, with retention managed as a separate policy. -- 2.45.2 From 5d324659e3f9fbd953461b68ae43603d052eda86 Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Sun, 11 Jan 2026 01:31:29 +0100 Subject: [PATCH 4/7] spec(049): clarify orchestration requirements --- .../spec.md | 31 +++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/specs/049-backup-restore-job-orchestration/spec.md b/specs/049-backup-restore-job-orchestration/spec.md index 8039727..c119263 100644 --- a/specs/049-backup-restore-job-orchestration/spec.md +++ b/specs/049-backup-restore-job-orchestration/spec.md @@ -15,6 +15,16 @@ ## Non-Goals (Phase 1) - No changes to external service contracts unless required for orchestration safety - No new promotion feature (e.g., DEV→PROD) (separate initiative) +## Clarifications + +### Session 2026-01-11 + +- Q: For FR-004 Idempotency, what should happen when the admin starts the same operation again for the same tenant + target while one is still queued/running? → A: Reuse existing run if identical is queued/running; allow a new run only after terminal. +- Q: For FR-002 status lifecycle, do we support canceling runs in Phase 1? → A: No cancel in Phase 1. +- Q: For FR-003 Progress visibility, which UI surfaces are required in Phase 1? → A: Phase 1 requires Run detail progress (counts/status) + DB notifications; Phase 2 adds a required global progress widget for all run types. +- Q: For FR-004, how should we define the “target object” used for de-duplication? → A: Dedupe key uses (tenant + operation type + target object id). +- Q: For FR-005 per-item outcome persistence, what is the item granularity for counts + item results in Phase 1? → A: Per internal DB record (e.g., restore/backup item rows). + ## User Scenarios & Testing *(mandatory)* ### User Story 1 - Capture snapshot runs in background (Priority: P1) @@ -99,17 +109,28 @@ ### Functional Requirements - Tenant identity - Initiator identity (user reference or audit reference) - Operation type and optional target object reference - - Status lifecycle: queued → running → (succeeded | failed | partial | canceled) + - Status lifecycle: queued → running → (succeeded | failed | partial) - Started/finished timestamps - Item counts: total / succeeded / failed - Safe error code and safe error context (no secrets) -- **FR-003 Progress visibility**: While a run is executing, the system MUST provide visible progress in the admin UI and MUST emit in-app notifications for key state transitions (queued/running/completed/failed). +- **FR-003 Progress visibility**: While a run is executing, the system MUST: + - Emit in-app notifications for key state transitions (queued/running/completed/failed) + - Provide a Run detail view that shows progress (status + item counts) + + Phase 1 does not require a global progress widget. + Phase 2 MUST add a global progress widget, required for all run types. - **FR-004 Idempotency & concurrency control**: The system MUST prevent uncontrolled duplicate execution due to double-clicks/retries by enforcing a deterministic de-duplication rule keyed by (tenant + operation type + target object) or (tenant + run id). When an identical run is already queued/running, the UI MUST show “already queued/running” and link to the existing run. + Clarification: For an identical start attempt while a run is `queued` or `running`, the system MUST re-use the existing Run Record and MUST NOT create a new run. A new run MAY be started only after the existing run reaches a terminal state. + + Clarification: For Phase 1, the default de-duplication key is (tenant + operation type + target object id). + - **FR-005 Deterministic outcome persistence**: The system MUST persist per-item outcomes for operations that act on multiple items, including status and a safe error summary, so results can be viewed later without relying on logs. + Clarification: In Phase 1, “item” refers to the internal DB record being acted on (e.g., a restore/backup item row). Counts (total/succeeded/failed) MUST be derived from these persisted item results. + - **FR-006 Tenant isolation & authorization**: Run visibility and execution MUST be tenant-scoped. Only authorized admins can start operations, and users MUST NOT be able to view or start runs across tenants. - **FR-007 Safety rules**: Preview/dry-run MUST be safe (no writes). Live restore MUST remain guarded with explicit confirmation and an auditable trail consistent with existing safety practices. @@ -122,9 +143,13 @@ ### Acceptance Checks - Starting any in-scope operation returns quickly with a queued Run Record link. - A Run Record always exists before background work begins and reaches a terminal state. -- Progress and state changes are visible in the UI via progress display and in-app notifications. +- Phase 1 does not support canceling runs. +- Progress and state changes are visible via Run detail view and in-app notifications. +- Phase 2 adds a global progress widget for all run types. - Duplicate start attempts for the same tenant + operation + target do not create uncontrolled duplicate execution. +- Duplicate start attempts for the same tenant + operation + target while a run is queued/running re-use the existing run and link to it. - Item-level outcomes and safe error summaries are viewable after completion. +- Run counts reflect persisted internal item results. - Preview/dry-run never performs writes. ### Key Entities *(include if feature involves data)* -- 2.45.2 From 66d8d90c304ebc69d0380190baa59d52f9b68109 Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Sun, 11 Jan 2026 16:54:57 +0100 Subject: [PATCH 5/7] spec: update Spec 049 artifacts --- .../contracts/admin-runs.openapi.yaml | 169 +++++++++++++++ .../data-model.md | 94 ++++++++ .../plan.md | 102 +++++++++ .../quickstart.md | 26 +++ .../research.md | 78 +++++++ .../spec.md | 43 ++-- .../tasks.md | 202 ++++++++++++++++++ 7 files changed, 697 insertions(+), 17 deletions(-) create mode 100644 specs/049-backup-restore-job-orchestration/contracts/admin-runs.openapi.yaml create mode 100644 specs/049-backup-restore-job-orchestration/data-model.md create mode 100644 specs/049-backup-restore-job-orchestration/plan.md create mode 100644 specs/049-backup-restore-job-orchestration/quickstart.md create mode 100644 specs/049-backup-restore-job-orchestration/research.md create mode 100644 specs/049-backup-restore-job-orchestration/tasks.md diff --git a/specs/049-backup-restore-job-orchestration/contracts/admin-runs.openapi.yaml b/specs/049-backup-restore-job-orchestration/contracts/admin-runs.openapi.yaml new file mode 100644 index 0000000..2bb6def --- /dev/null +++ b/specs/049-backup-restore-job-orchestration/contracts/admin-runs.openapi.yaml @@ -0,0 +1,169 @@ +openapi: 3.0.3 +info: + title: TenantPilot Admin Run Orchestration (049) + version: 0.1.0 + description: | + Internal admin contracts for starting long-running backup/restore operations + and reading run status/progress. These endpoints are tenant-scoped. + +servers: + - url: /admin + +paths: + /t/{tenantExternalId}/runs/{runType}: + post: + operationId: startRun + summary: Start a background run + description: | + Starts an operation by creating (or reusing) a Run Record and enqueueing + background work. Must return quickly. + parameters: + - in: path + name: tenantExternalId + required: true + schema: + type: string + - in: path + name: runType + required: true + schema: + type: string + enum: + - backup_set_add_policies + - restore_execute + - restore_preview + - snapshot_capture + requestBody: + required: false + content: + application/json: + schema: + $ref: '#/components/schemas/RunStartRequest' + responses: + '201': + description: Run created and queued + content: + application/json: + schema: + $ref: '#/components/schemas/RunStartResponse' + '200': + description: Existing active run reused + content: + application/json: + schema: + $ref: '#/components/schemas/RunStartResponse' + '403': + description: Forbidden + + /t/{tenantExternalId}/runs/{runType}/{runId}: + get: + operationId: getRun + summary: Get run status and progress + parameters: + - in: path + name: tenantExternalId + required: true + schema: + type: string + - in: path + name: runType + required: true + schema: + type: string + - in: path + name: runId + required: true + schema: + type: string + responses: + '200': + description: Run record + content: + application/json: + schema: + $ref: '#/components/schemas/RunRecord' + '404': + description: Not found + +components: + schemas: + RunStartRequest: + type: object + additionalProperties: false + properties: + targetObjectId: + type: string + nullable: true + description: Operation target used for de-duplication. + payloadHash: + type: string + nullable: true + description: Optional stable hash of relevant payload to strengthen idempotency. + itemIds: + type: array + items: + type: string + nullable: true + description: Optional internal item ids to process. + + RunStartResponse: + type: object + required: [run] + properties: + reused: + type: boolean + default: false + run: + $ref: '#/components/schemas/RunRecord' + + RunRecord: + type: object + required: + - id + - tenantExternalId + - type + - status + properties: + id: + type: string + tenantExternalId: + type: string + type: + type: string + status: + type: string + enum: [queued, running, succeeded, failed, partial] + createdAt: + type: string + format: date-time + startedAt: + type: string + format: date-time + nullable: true + finishedAt: + type: string + format: date-time + nullable: true + counts: + type: object + additionalProperties: false + properties: + total: + type: integer + minimum: 0 + succeeded: + type: integer + minimum: 0 + failed: + type: integer + minimum: 0 + safeError: + type: object + nullable: true + additionalProperties: false + properties: + code: + type: string + context: + type: object + additionalProperties: true diff --git a/specs/049-backup-restore-job-orchestration/data-model.md b/specs/049-backup-restore-job-orchestration/data-model.md new file mode 100644 index 0000000..32a1b50 --- /dev/null +++ b/specs/049-backup-restore-job-orchestration/data-model.md @@ -0,0 +1,94 @@ +# Data Model: Backup/Restore Job Orchestration (049) + +This feature relies on existing “run record” models/tables and (optionally) extends them to meet the orchestration requirements. + +## Entities + +## 1) RestoreRun (`restore_runs`) + +**Purpose:** Run record for restore executions and dry-run/preview workflows. + +**Model:** `App\Models\RestoreRun` + +**Key fields (existing):** +- `id` (PK) +- `tenant_id` (FK → tenants) +- `backup_set_id` (FK → backup_sets) +- `requested_by` (string|null) +- `is_dry_run` (bool) +- `status` (string) +- `requested_items` (json|null) +- `preview` (json|null) — persisted preview output +- `results` (json|null) — persisted execution output (may include per-item outcomes) +- `failure_reason` (text|null) +- `started_at` / `completed_at` (timestamp|null) +- `metadata` (json|null) + +**Relationships:** +- `RestoreRun belongsTo Tenant` +- `RestoreRun belongsTo BackupSet` + +**State transitions (target):** +- `queued → running → succeeded|failed|partial` + +**Validation constraints (creation/dispatch):** +- tenant-scoped access required +- `backup_set_id` must belong to tenant +- preview/dry-run must not perform writes (constitution Read/Write Separation) + +--- + +## 2) BulkOperationRun (`bulk_operation_runs`) + +**Purpose:** Run record for background operations that process many internal items, including backup-set capture-like actions. + +**Model:** `App\Models\BulkOperationRun` + +**Key fields (existing):** +- `id` (PK) +- `tenant_id` (FK → tenants) +- `user_id` (FK → users) +- `resource` (string) — e.g. `policy`, `backup_set` +- `action` (string) — e.g. `export`, `add_policies` +- `status` (string) — `pending`, `running`, `completed`, `completed_with_errors`, `failed`, `aborted` +- `total_items`, `processed_items`, `succeeded`, `failed`, `skipped` +- `item_ids` (jsonb) +- `failures` (jsonb|null) — safe per-item error summaries +- `audit_log_id` (FK → audit_logs|null) + +**Relationships:** +- `BulkOperationRun belongsTo Tenant` +- `BulkOperationRun belongsTo User` + +**Recommended additions (to satisfy FR-002/FR-004 cleanly):** +- `idempotency_key` (string, indexed; uniqueness enforced for active statuses via partial index) +- `started_at` / `finished_at` (timestampTz) +- `error_code` (string|null) +- `error_context` (jsonb|null) + +**State transitions (target):** +- `queued → running → succeeded|failed|partial` + - `pending` maps to `queued` + - `completed_with_errors` maps to `partial` + +--- + +## 3) Notification Event (DB notifications) + +**Purpose:** Persist state transitions and completion notices for the initiating user. + +**Storage:** Laravel Notifications (DB channel). + +**Payload shape (target):** +- `tenant_id` +- `run_type` (restore_run / bulk_operation_run) +- `run_id` +- `status` (queued/running/succeeded/failed/partial) +- `counts` (optional) +- `safe_error_code` + `safe_error_context` (optional) + +## Notes on “per-item outcomes” (FR-005) + +- For restore workflows, per-item outcomes can initially be stored in `restore_runs.results` as a structured JSON array/object keyed by internal item identifiers. +- For bulk operations, per-item outcomes are already persisted as `bulk_operation_runs.failures` plus the counter columns. +- If Phase 1 needs relational per-item tables for querying/filtering, introduce a dedicated “run item results” table per run type (Phase 2+ preferred). diff --git a/specs/049-backup-restore-job-orchestration/plan.md b/specs/049-backup-restore-job-orchestration/plan.md new file mode 100644 index 0000000..2ad2a39 --- /dev/null +++ b/specs/049-backup-restore-job-orchestration/plan.md @@ -0,0 +1,102 @@ +# Implementation Plan: Backup/Restore Job Orchestration (049) + +**Branch**: `feat/049-backup-restore-job-orchestration-session-1768091854` | **Date**: 2026-01-11 | **Spec**: [specs/049-backup-restore-job-orchestration/spec.md](specs/049-backup-restore-job-orchestration/spec.md) +**Input**: Feature specification from `specs/049-backup-restore-job-orchestration/spec.md` + +**Note**: This template is filled in by the `/speckit.plan` command. See `.specify/scripts/` for helper scripts. + +## Summary + +Move all backup/restore “start/execute” actions off the interactive request path. + +- Interactive actions must only create (or reuse) a tenant-scoped Run Record and enqueue work. +- Background jobs perform Graph calls, capture/restore work, and update run records with status + counts + safe error summaries. +- Idempotency prevents double-click duplicates by reusing an active run for the same `(tenant + operation type + target)`. + +Design choices are captured in [specs/049-backup-restore-job-orchestration/research.md](specs/049-backup-restore-job-orchestration/research.md). + +## Phasing + +### Phase 1 (this spec’s implementation target) + +- Ensure all in-scope operations are job-only (no heavy work inline). +- Create/reuse run records with idempotency for active runs. +- Provide **Run detail** views for progress (status + counts) and **DB notifications** for state transitions. + +### Phase 2 (explicitly out-of-scope for Phase 1) + +- Add a **global progress widget** that surfaces all run types (not just bulk ops) across the admin UI. + +## Technical Context + +**Language/Version**: PHP 8.4.15 +**Primary Dependencies**: Laravel 12, Filament 4, Livewire 3 +**Storage**: PostgreSQL (JSONB used for run payloads/summaries where appropriate) +**Testing**: Pest 4 (feature tests + job tests) +**Target Platform**: Containerized web app (Sail for local dev; Dokploy for staging/prod) +**Project Type**: Web application (Laravel monolith) +**Performance Goals**: 95% of start actions confirm “queued” within 2 seconds (SC-001) +**Constraints**: No heavy work during interactive requests; jobs must be idempotent + observable; no secrets in run records +**Scale/Scope**: Multi-tenant MSP usage; long-running Graph operations; frequent retries/double-click scenarios + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +- Inventory-first: orchestration is run-record centric; inventory stays “last observed”, backups remain explicit actions. +- Read/write separation: preview/dry-run stays read-only; live restore remains behind explicit confirmation + audit + tests. +- Graph contract path: all Graph calls remain behind `GraphClientInterface` and contract registry (`config/graph_contracts.php`). +- Deterministic capabilities: no new capability derivation introduced by this feature (existing resolver remains authoritative). +- Tenant isolation: all run visibility + execution is tenant-scoped; no cross-tenant run access. +- Automation: enforce de-duplication for active runs; jobs use locks/backoff for 429/503 where applicable. +- Data minimization: run records store only safe summaries (error codes + whitelisted context), never secrets/tokens. + +## Project Structure + +### Documentation (this feature) + +```text +specs/049-backup-restore-job-orchestration/ +├── 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/ +├── Filament/ +│ └── Resources/ +├── Jobs/ +├── Livewire/ +├── Models/ +├── Services/ +└── Support/ + +database/ +└── migrations/ + +resources/ +└── views/ + +tests/ +├── Feature/ +└── Unit/ +``` + +**Structure Decision**: Laravel monolith; orchestration implemented via queued jobs + run records in existing models/tables. + +## Complexity Tracking + +> **Fill ONLY if Constitution Check has violations that must be justified** + +| Violation | Why Needed | Simpler Alternative Rejected Because | +|-----------|------------|-------------------------------------| +| [e.g., 4th project] | [current need] | [why 3 projects insufficient] | +| [e.g., Repository pattern] | [specific problem] | [why direct DB access insufficient] | + +No constitution violations are required for this feature. diff --git a/specs/049-backup-restore-job-orchestration/quickstart.md b/specs/049-backup-restore-job-orchestration/quickstart.md new file mode 100644 index 0000000..1bbe215 --- /dev/null +++ b/specs/049-backup-restore-job-orchestration/quickstart.md @@ -0,0 +1,26 @@ +# Quickstart: Backup/Restore Job Orchestration (049) + +## Goal + +Ensure backup/restore “start/execute” actions never run heavy work inline. They create (or reuse) a Run Record and queue the work. + +## Local development + +- Bring Sail up: `./vendor/bin/sail up -d` +- Run migrations: `./vendor/bin/sail artisan migrate` +- Run a queue worker (separate terminal): `./vendor/bin/sail artisan queue:work` + +## Testing + +Run the most relevant tests first: + +- Unit helpers: `./vendor/bin/sail artisan test tests/Unit/RunIdempotencyTest.php` +- Snapshot capture orchestration: `./vendor/bin/sail artisan test --filter=PolicyCaptureSnapshot` +- Restore orchestration: `./vendor/bin/sail artisan test --filter=RestoreRun` +- Cross-tenant authorization: `./vendor/bin/sail artisan test --filter=RunAuthorization` + +## Operational notes + +- Run records must be tenant-scoped and never contain secrets. +- Preview/dry-run must remain read-only. +- Use de-duplication for active runs to prevent double-click duplication. diff --git a/specs/049-backup-restore-job-orchestration/research.md b/specs/049-backup-restore-job-orchestration/research.md new file mode 100644 index 0000000..f22e7c9 --- /dev/null +++ b/specs/049-backup-restore-job-orchestration/research.md @@ -0,0 +1,78 @@ +# Research: Backup/Restore Job Orchestration (049) + +This document resolves Phase 0 open questions and records design choices. + +## Decisions + +### 1) Run Record storage strategy + +**Decision:** Reuse existing run-record primitives instead of introducing a brand-new “unified run” subsystem in Phase 1. + +- Restore + re-run restore + dry-run/preview: use the existing `restore_runs` table / `App\Models\RestoreRun`. +- Backup set capture-like operations (e.g., “add policies and capture”): reuse `bulk_operation_runs` / `App\Models\BulkOperationRun` (already used for long-running background work like bulk exports) and (if needed) extend it to satisfy FR-002 fields. + +**Rationale:** +- The codebase already has multiple proven “run tables” (`restore_runs`, `inventory_sync_runs`, `backup_schedule_runs`, `bulk_operation_runs`). +- Minimizes migration risk and avoids broad refactors. +- Lets Phase 1 focus on eliminating inline heavy work while keeping UX consistent. + +**Alternatives considered:** +- **Create a new generic `operation_runs` + `operation_run_items` data model** for all queued automation. + - Rejected (Phase 1): higher migration + backfill cost; high coordination risk across many features. + +### 2) Status lifecycle mapping + +**Decision:** Standardize at the *UI + plan* level on `queued → running → (succeeded | failed | partial)` while allowing underlying storage to keep its existing status vocabulary. + +- `BulkOperationRun.status` mapping: `pending→queued`, `running→running`, `completed→succeeded`, `completed_with_errors→partial`, `failed/aborted→failed`. +- `RestoreRun.status` mapping will be aligned (e.g., `pending→queued`, `running→running`, etc.) as part of implementation. + +**Rationale:** +- Keeps the spec’s lifecycle consistent without forcing an immediate cross-table refactor. + +**Alternatives considered:** +- **Rename and normalize all run statuses across all run tables.** + - Rejected (Phase 1): touches many workflows and tests. + +### 3) Idempotency & de-duplication + +**Decision:** Enforce de-duplication for *active* runs via a deterministic key and a DB query gate, with an optional lock for race reduction. + +- Dedupe key format: `tenant_id + operation_type + target_object_id` (plus a stable hash of relevant payload if needed). +- Behavior: if an identical run is `queued`/`running`, reuse it and return/link to it; allow a new run only after terminal. + +**Rationale:** +- Matches the constitution (“Automation must be Idempotent & Observable”) and aligns with existing patterns (inventory selection hash + schedule locks). + +**Alternatives considered:** +- **Cache-only locks** (`Cache::lock(...)`) without persisted keys. + - Rejected: harder to reason about after restarts; less observable. + +### 4) Restore preview must be asynchronous + +**Decision:** Move restore preview generation (“Generate preview” in the wizard) into a queued job which persists preview outputs to the run record. + +**Rationale:** +- Preview can require Graph calls and normalization work; it should never block an interactive request. + +**Alternatives considered:** +- **Keep preview synchronous** and increase timeouts. + - Rejected: timeouts, poor UX, and violates FR-001. + +### 5) Notifications for progress visibility + +**Decision:** Use DB notifications for state transitions (queued/running/terminal) and keep a Run detail view as the primary progress surface in Phase 1. + +**Rationale:** +- Inventory sync + backup schedule runs already use this pattern. +- Survives page reloads and doesn’t require the user to keep the page open. + +**Alternatives considered:** +- **Frontend polling only** (no DB notifications). + - Rejected: weaker UX and weaker observability. + +## Clarifications resolved + +- **SC-003 includes “canceled”** while Phase 1 explicitly has “no cancel”. + - Resolution for Phase 1 planning: treat “canceled” as out-of-scope (Phase 2+) and map “aborted” (if present) into the `failed` bucket for SC accounting. + diff --git a/specs/049-backup-restore-job-orchestration/spec.md b/specs/049-backup-restore-job-orchestration/spec.md index c119263..37144ed 100644 --- a/specs/049-backup-restore-job-orchestration/spec.md +++ b/specs/049-backup-restore-job-orchestration/spec.md @@ -42,7 +42,21 @@ ### User Story 1 - Capture snapshot runs in background (Priority: P1) --- -### User Story 2 - Restore runs in background with per-item results (Priority: P1) +### User Story 2 - Backup set create/capture runs in background (Priority: P2) + +An admin can create a backup set and optionally start a capture/sync operation without the request doing heavy work. + +**Why this priority**: Creating backup sets is frequent and should not be coupled to long-running capture logic. + +**Independent Test**: Creating a backup set returns quickly and any capture/sync work appears as a run with progress. + +**Acceptance Scenarios**: + +1. **Given** an admin creates a backup set with capture enabled, **When** they submit, **Then** the backup set is created and a capture run is queued. + +--- + +### User Story 3 - Restore runs in background with per-item results (Priority: P1) An admin can start a “restore to Intune” or “re-run restore” operation as a background run and later inspect item-level outcomes and errors. @@ -54,20 +68,7 @@ ### User Story 2 - Restore runs in background with per-item results (Priority: P 1. **Given** an admin starts a restore, **When** they confirm the action, **Then** the UI queues a run and returns immediately (no long-running request). 2. **Given** a restore run finishes with mixed outcomes, **When** the admin views the run details, **Then** they see succeeded/failed counts and a safe error summary per failed item. - ---- - -### User Story 3 - Backup set create/capture runs in background (Priority: P2) - -An admin can create a backup set and optionally start a capture/sync operation without the request doing heavy work. - -**Why this priority**: Creating backup sets is frequent and should not be coupled to long-running capture logic. - -**Independent Test**: Creating a backup set returns quickly and any capture/sync work appears as a run with progress. - -**Acceptance Scenarios**: - -1. **Given** an admin creates a backup set with capture enabled, **When** they submit, **Then** the backup set is created and a capture run is queued. +3. **Given** an admin executes a live restore, **When** the run is queued/executed, **Then** an auditable event is recorded that links to the run. --- @@ -82,6 +83,7 @@ ### User Story 4 - Dry-run/preview runs in background (Priority: P2) **Acceptance Scenarios**: 1. **Given** an admin starts a preview run, **When** the run completes, **Then** the UI shows preview results without requiring re-execution. +2. **Given** an admin starts a preview/dry-run, **When** the run executes, **Then** no write/change is performed against the external system. ### Edge Cases @@ -135,7 +137,9 @@ ### Functional Requirements - **FR-007 Safety rules**: Preview/dry-run MUST be safe (no writes). Live restore MUST remain guarded with explicit confirmation and an auditable trail consistent with existing safety practices. -- **FR-008 Resilience**: The system MUST handle external service throttling/outages gracefully, including retries with backoff when appropriate, and MUST end runs in a clear terminal state (failed/partial) rather than silently failing. +- **FR-008 Resilience (Post-MVP / Phase 2)**: The system MUST handle external service throttling/outages gracefully, including retries with backoff when appropriate, and MUST end runs in a clear terminal state (failed/partial) rather than silently failing. + + *Note*: MVP/Phase 1 relies on existing retry behavior where present; standardized backoff + jitter hardening is scheduled post-MVP. - **FR-009 Safe logging & data minimization**: The system MUST NOT store secrets/tokens in Run Records, notifications, or error contexts. Error context MUST be limited to a defined, safe set of fields. @@ -151,6 +155,9 @@ ### Acceptance Checks - Item-level outcomes and safe error summaries are viewable after completion. - Run counts reflect persisted internal item results. - Preview/dry-run never performs writes. +- Unauthorized users cannot start runs for a tenant they do not belong to. +- Users cannot list/view run records across tenants. +- Live restore creates an auditable event linked to the run. ### Key Entities *(include if feature involves data)* @@ -164,9 +171,11 @@ ### Measurable Outcomes - **SC-001**: For 95% of operation starts, the UI confirms “queued” within 2 seconds. - **SC-002**: Double-clicking an operation start results in at most one queued/running run for the same tenant + operation + target. -- **SC-003**: 99% of runs end in a clear terminal state (succeeded/failed/partial/canceled) with a human-readable summary. +- **SC-003**: 99% of runs end in a clear terminal state (succeeded/failed/partial) with a human-readable summary. - **SC-004**: Admins can locate the latest run status for an operation in under 30 seconds without requiring access to system logs. +*Note*: “canceled” is reserved for Phase 2+ (Phase 1 has no cancel support). + ## Assumptions - This feature builds on the UI safety constraints from 048: admin pages must remain usable even when the external service API is unavailable. diff --git a/specs/049-backup-restore-job-orchestration/tasks.md b/specs/049-backup-restore-job-orchestration/tasks.md new file mode 100644 index 0000000..6cd37df --- /dev/null +++ b/specs/049-backup-restore-job-orchestration/tasks.md @@ -0,0 +1,202 @@ +# Tasks: Backup/Restore Job Orchestration (049) + +**Input**: Design documents from `specs/049-backup-restore-job-orchestration/` + +**Prerequisites**: plan.md (required), spec.md (required), research.md, data-model.md, contracts/, quickstart.md + +**Tests**: REQUIRED (Pest) for these runtime behavior changes. + +**MVP scope**: Strictly limited to **T001–T016 (US1 only)**. The **Phase 7 global progress widget (T037)** is **Phase 2** and explicitly **NOT** part of the MVP. + +## Phase 1: Setup (Shared Infrastructure) + +- [x] T001 Verify queue + DB notifications prerequisites in config/queue.php and database/migrations/*notifications* (add missing migration if needed) +- [x] T002 Confirm existing run tables and status enums used by RestoreRun in app/Support/RestoreRunStatus.php and database/migrations/2025_12_10_000150_create_restore_runs_table.php +- [x] T003 [P] Add quickstart sanity commands for this feature in specs/049-backup-restore-job-orchestration/quickstart.md + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**⚠️ CRITICAL**: No user story work should begin until this phase is complete. + +- [x] T004 Add idempotency support to bulk_operation_runs via database/migrations/2026_01_11_120001_add_idempotency_key_to_bulk_operation_runs_table.php +- [x] T005 Add idempotency support to restore_runs via database/migrations/2026_01_11_120002_add_idempotency_key_to_restore_runs_table.php +- [x] T006 [P] Add casts/fillables for idempotency + timestamps in app/Models/BulkOperationRun.php and app/Models/RestoreRun.php +- [x] T007 Implement idempotency key helpers in app/Support/RunIdempotency.php (build key, find active run, enforce reuse) +- [x] T008 [P] Add a read-only Filament resource to inspect run details for BulkOperationRun in app/Filament/Resources/BulkOperationRunResource.php +- [x] T009 [P] Add notification for run status transitions in app/Notifications/RunStatusChangedNotification.php (DB channel) +- [x] T010 Add unit tests for RunIdempotency helpers in tests/Unit/RunIdempotencyTest.php + +**CRITICAL (must-fix before implementing any new run flows): Tenant isolation + authorization** + +- [x] T042 Add tenant-scoped authorization for run list/view/start across all run flows (BulkOperationRun + RestoreRun) using policies/resources and ensure every query is tenant-scoped (e.g., app/Filament/Resources/BulkOperationRunResource.php, app/Filament/Resources/RestoreRunResource.php, and each start action/page that creates runs) +- [x] T043 [P] Add Pest feature tests that run list/view are tenant-scoped (cannot list/view another tenant’s runs) in tests/Feature/RunAuthorizationTenantIsolationTest.php +- [x] T044 [P] Add Pest feature tests that unaffiliated users cannot start runs (capture snapshot / restore execute / preview / backup set capture) in tests/Feature/RunStartAuthorizationTest.php + +**Checkpoint**: Foundation ready (idempotency + run detail view + notifications). + +--- + +## Phase 3: User Story 1 - Capture snapshot runs in background (Priority: P1) 🎯 MVP + +**Goal**: Capturing a policy snapshot never blocks the UI; it creates/reuses a run record and processes in a queued job with visible progress. + +**Independent Test**: Trigger “Capture snapshot” on a policy; the request returns quickly and a BulkOperationRun transitions `queued → running → succeeded|failed|partial`, with details viewable. + +### Tests (write first) + +- [x] T011 [P] [US1] Add Pest feature test that capture snapshot queues a job (no inline capture) in tests/Feature/PolicyCaptureSnapshotQueuedTest.php +- [x] T012 [P] [US1] Add Pest feature test that double-click reuses the active run (idempotency) in tests/Feature/PolicyCaptureSnapshotIdempotencyTest.php + +### Implementation + +- [x] T013 [US1] Create queued job to capture one policy snapshot in app/Jobs/CapturePolicySnapshotJob.php (updates BulkOperationRun counts + failures) +- [x] T014 [US1] Update UI action to create/reuse run and dispatch job in app/Filament/Resources/PolicyResource/Pages/ViewPolicy.php +- [x] T015 [P] [US1] Add linking from UI notifications to BulkOperationRunResource view page in app/Filament/Resources/BulkOperationRunResource.php +- [x] T016 [US1] Ensure failures are safe/minimized (no secrets) when recording run failures in app/Services/BulkOperationService.php + +**Checkpoint**: User Story 1 is independently usable and testable. + +--- + +## Phase 4: User Story 3 - Restore runs in background with per-item results (Priority: P1) + +**Goal**: Restore execution and re-run restore operate exclusively via queued jobs, with persisted per-item outcomes and safe error summaries visible in the run detail UI. + +**Independent Test**: Starting restore creates/reuses a RestoreRun in `queued` state, queues execution, and later shows item outcomes without relying on logs. + +### Tests (write first) + + +- [x] T017 [P] [US3] Add Pest feature test that restore execution reuses active run for identical (tenant+backup_set+scope) starts in tests/Feature/RestoreRunIdempotencyTest.php +- [x] T018 [P] [US3] Extend existing restore job test to assert per-item outcome persistence in tests/Feature/ExecuteRestoreRunJobTest.php +- [x] T045 [P] [US3] Add Pest feature test that live restore writes an audit event (run-id linked) in tests/Feature/RestoreAuditLoggingTest.php + +### Implementation + +- [x] T019 [US3] Implement idempotency key computation for restore runs (tenant + operation + target + scope hash) in app/Support/RunIdempotency.php +- [x] T020 [US3] Update restore run creation/execute flow to reuse active runs (no duplicates) in app/Filament/Resources/RestoreRunResource.php +- [x] T021 [US3] Update app/Jobs/ExecuteRestoreRunJob.php to set started/finished timestamps and emit DB notifications (queued/running/terminal) +- [x] T022 [US3] Persist deterministic per-item outcomes into restore_runs.results (keyed by backup_item_id) in app/Services/Intune/RestoreService.php +- [x] T023 [US3] Derive total/succeeded/failed counts from persisted results and surface in RestoreRunResource view/table in app/Filament/Resources/RestoreRunResource.php +- [x] T046 [US3] Ensure live restore execution emits an auditable event linked to the run (e.g., audit_logs FK or structured audit record) in app/Jobs/ExecuteRestoreRunJob.php and/or app/Services/Intune/RestoreService.php + +**Checkpoint**: Restore runs are job-only, idempotent, and observable with item outcomes. + +--- + +## Phase 5: User Story 2 - Backup set create/capture runs in background (Priority: P2) + +**Goal**: Creating a backup set and adding policies to a backup set does not perform Graph-heavy snapshot capture inline; capture occurs in jobs with a run record. + +**Independent Test**: Creating a backup set returns quickly and produces a BulkOperationRun showing progress; adding policies via the picker also queues work. + +### Tests (write first) + +- [ ] T024 [P] [US2] Add Pest feature test that backup set create does not run capture inline and instead queues a job in tests/Feature/BackupSetCreateCaptureQueuedTest.php +- [ ] T025 [P] [US2] Add Pest feature test that “Add selected” in policy picker queues background work in tests/Feature/BackupSetPolicyPickerQueuesCaptureTest.php + +### Implementation + +- [ ] T026 [US2] Refactor capture work out of BackupService::createBackupSet into separate methods in app/Services/Intune/BackupService.php +- [ ] T027 [US2] Create queued job to capture backup set items in app/Jobs/CaptureBackupSetJob.php (uses BackupService; updates BulkOperationRun) +- [ ] T028 [US2] Update backup set create flow to create backup_set record quickly and dispatch CaptureBackupSetJob in app/Filament/Resources/BackupSetResource.php +- [ ] T029 [US2] Create queued job to add policies to a backup set (and capture foundations if requested) in app/Jobs/AddPoliciesToBackupSetJob.php +- [ ] T030 [US2] Update bulk action in app/Livewire/BackupSetPolicyPickerTable.php to create/reuse BulkOperationRun and dispatch AddPoliciesToBackupSetJob + +**Checkpoint**: Backup set capture workloads are job-only and observable. + +--- + +## Phase 6: User Story 4 - Dry-run/preview runs in background (Priority: P2) + +**Goal**: Restore preview generation is queued, persisted, and viewable without re-execution. + +**Independent Test**: Clicking “Generate preview” returns quickly; a queued RestoreRun performs the diff generation asynchronously and persists preview output that the UI can display. + +### Tests (write first) + +- [ ] T031 [P] [US4] Add Pest feature test that preview generation queues a job (no inline RestoreDiffGenerator call) in tests/Feature/RestorePreviewQueuedTest.php +- [ ] T032 [P] [US4] Add Pest feature test that preview results persist and are reusable in tests/Feature/RestorePreviewPersistenceTest.php +- [ ] T047 [P] [US4] Add Pest feature test that preview/dry-run never performs writes (must be read-only) in tests/Feature/RestorePreviewReadOnlySafetyTest.php + +### Implementation + +- [ ] T033 [US4] Create queued job to generate preview diffs and persist to restore_runs.preview + metadata in app/Jobs/GenerateRestorePreviewJob.php +- [ ] T034 [US4] Update preview action in app/Filament/Resources/RestoreRunResource.php to create/reuse a dry-run RestoreRun and dispatch GenerateRestorePreviewJob +- [ ] T035 [US4] Update restore run view component to read preview from the persisted run record in resources/views/filament/forms/components/restore-run-preview.blade.php +- [ ] T036 [US4] Emit DB notifications for preview queued/running/completed/failed transitions in app/Jobs/GenerateRestorePreviewJob.php +- [ ] T048 [US4] Enforce preview/dry-run read-only behavior: block write-capable operations and record a safe failure if a write would occur (in app/Jobs/GenerateRestorePreviewJob.php and/or restore diff generation service) + +**Checkpoint**: Preview is asynchronous, persisted, and visible. + +--- + +## Phase 7: Phase 2 - Global Progress Widget (All Run Types) + +- [ ] T037 [P] Add a global progress widget for restore runs (Phase 2 requirement) by extending app/Livewire/BulkOperationProgress.php or adding a dedicated Livewire component in app/Livewire/RestoreRunProgress.php + +--- + +## Phase 8: Polish & Cross-Cutting Concerns + +- [ ] T038 Ensure Graph throttling/backoff behavior is applied inside queued jobs (429/503) in app/Services/Intune/PolicySnapshotService.php and app/Services/Intune/RestoreService.php +- [ ] T039 [P] Add/extend run status notification formatting to include safe error codes/contexts in app/Notifications/RunStatusChangedNotification.php +- [ ] T040 Run formatter on modified files: vendor/bin/pint --dirty +- [ ] T041 Run targeted tests for affected areas: tests/Feature/*Restore* tests/Feature/*BackupSet* tests/Feature/*Policy* (use php artisan test with filters) + +--- + +## Dependencies & Execution Order + +### Story order + +- Phase 1 → Phase 2 must complete first. +- After Phase 2: + - US1 and US3 can proceed in parallel. + - US4 can proceed in parallel but may be easiest after US3 (shared RestoreRun patterns). + - US2 can proceed independently after Phase 2. + +### Dependency graph + +- Setup → Foundational → { US1, US2, US3, US4 } → Polish +- Setup → Foundational → { US1, US2, US3, US4 } → Phase 2 Global Widget → Polish +- Suggested minimal MVP: Setup → Foundational → US1 + +--- + +## Parallel execution examples + +### US1 + +- In parallel: T011 (queues test), T012 (idempotency test) +- In parallel: T013 (job), T014 (UI action update) after foundational tasks + +### US2 + +- In parallel: T024 (create queues test), T025 (picker queues test) +- In parallel: T027 (job) and T029 (job) after BackupService refactor task T026 + +### US3 + +- In parallel: T017 (idempotency test), T018 (job behavior test) +- In parallel: T021 (job notifications) and T023 (UI view enhancements) once results format is defined + +### US4 + +- In parallel: T031 (queues test), T032 (persistence test) +- In parallel: T033 (job) and T035 (view reads persisted preview) once run persistence shape is agreed + +--- + +## Implementation strategy + +- MVP (fastest value): deliver US1 first (policy snapshot capture becomes queued + idempotent + observable). +- Next: US3 + US4 to fully de-risk restore execution and preview. +- Then: US2 to eliminate inline Graph work from backup set flows. + +## Format validation + +All tasks above follow the required checklist format: +`- [ ] T### [P?] [US#?] Description with file path` -- 2.45.2 From e4a3a4d378b12c2fa80a923a3902ab975772d1c1 Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Sun, 11 Jan 2026 16:55:03 +0100 Subject: [PATCH 6/7] feat: job-only restore runs with idempotency --- .../Resources/BulkOperationRunResource.php | 138 ++++++++++ .../Pages/ListBulkOperationRuns.php | 11 + .../Pages/ViewBulkOperationRun.php | 11 + .../PolicyResource/Pages/ViewPolicy.php | 116 ++++++--- app/Filament/Resources/RestoreRunResource.php | 242 +++++++++++++----- app/Jobs/CapturePolicySnapshotJob.php | 107 ++++++++ app/Jobs/ExecuteRestoreRunJob.php | 62 ++++- app/Models/BulkOperationRun.php | 1 + app/Models/RestoreRun.php | 10 + .../RunStatusChangedNotification.php | 94 +++++++ app/Policies/BulkOperationRunPolicy.php | 39 +++ app/Providers/AppServiceProvider.php | 1 + app/Rules/SkipOrUuidRule.php | 37 +++ app/Services/BulkOperationService.php | 32 +++ app/Services/Intune/RestoreService.php | 74 +++++- app/Support/RunIdempotency.php | 95 +++++++ ...tency_key_to_bulk_operation_runs_table.php | 32 +++ ..._idempotency_key_to_restore_runs_table.php | 32 +++ .../entries/restore-results.blade.php | 21 +- 19 files changed, 1039 insertions(+), 116 deletions(-) create mode 100644 app/Filament/Resources/BulkOperationRunResource.php create mode 100644 app/Filament/Resources/BulkOperationRunResource/Pages/ListBulkOperationRuns.php create mode 100644 app/Filament/Resources/BulkOperationRunResource/Pages/ViewBulkOperationRun.php create mode 100644 app/Jobs/CapturePolicySnapshotJob.php create mode 100644 app/Notifications/RunStatusChangedNotification.php create mode 100644 app/Policies/BulkOperationRunPolicy.php create mode 100644 app/Rules/SkipOrUuidRule.php create mode 100644 app/Support/RunIdempotency.php create mode 100644 database/migrations/2026_01_11_120001_add_idempotency_key_to_bulk_operation_runs_table.php create mode 100644 database/migrations/2026_01_11_120002_add_idempotency_key_to_restore_runs_table.php diff --git a/app/Filament/Resources/BulkOperationRunResource.php b/app/Filament/Resources/BulkOperationRunResource.php new file mode 100644 index 0000000..cfdde9b --- /dev/null +++ b/app/Filament/Resources/BulkOperationRunResource.php @@ -0,0 +1,138 @@ +schema([ + Section::make('Run') + ->schema([ + TextEntry::make('user.name') + ->label('Initiator') + ->placeholder('—'), + TextEntry::make('resource')->badge(), + TextEntry::make('action')->badge(), + TextEntry::make('status') + ->badge() + ->color(fn (BulkOperationRun $record): string => static::statusColor($record->status)), + TextEntry::make('total_items')->label('Total')->numeric(), + TextEntry::make('processed_items')->label('Processed')->numeric(), + TextEntry::make('succeeded')->numeric(), + TextEntry::make('failed')->numeric(), + TextEntry::make('skipped')->numeric(), + TextEntry::make('created_at')->dateTime(), + TextEntry::make('updated_at')->dateTime(), + TextEntry::make('idempotency_key')->label('Idempotency key')->copyable()->placeholder('—'), + ]) + ->columns(2) + ->columnSpanFull(), + + Section::make('Items') + ->schema([ + ViewEntry::make('item_ids') + ->label('') + ->view('filament.infolists.entries.snapshot-json') + ->state(fn (BulkOperationRun $record) => $record->item_ids ?? []) + ->columnSpanFull(), + ]) + ->columnSpanFull(), + + Section::make('Failures') + ->schema([ + ViewEntry::make('failures') + ->label('') + ->view('filament.infolists.entries.snapshot-json') + ->state(fn (BulkOperationRun $record) => $record->failures ?? []) + ->columnSpanFull(), + ]) + ->columnSpanFull(), + ]); + } + + public static function table(Table $table): Table + { + return $table + ->defaultSort('id', 'desc') + ->modifyQueryUsing(function (Builder $query): Builder { + $tenantId = Tenant::current()->getKey(); + + return $query->when($tenantId, fn (Builder $q) => $q->where('tenant_id', $tenantId)); + }) + ->columns([ + Tables\Columns\TextColumn::make('user.name') + ->label('Initiator') + ->placeholder('—') + ->toggleable(), + Tables\Columns\TextColumn::make('resource')->badge(), + Tables\Columns\TextColumn::make('action')->badge(), + Tables\Columns\TextColumn::make('status') + ->badge() + ->color(fn (BulkOperationRun $record): string => static::statusColor($record->status)), + Tables\Columns\TextColumn::make('created_at')->since(), + Tables\Columns\TextColumn::make('total_items')->label('Total')->numeric(), + Tables\Columns\TextColumn::make('processed_items')->label('Processed')->numeric(), + Tables\Columns\TextColumn::make('failed')->numeric(), + ]) + ->actions([ + Actions\ViewAction::make(), + ]) + ->bulkActions([]); + } + + public static function getEloquentQuery(): Builder + { + return parent::getEloquentQuery() + ->with('user') + ->latest('id'); + } + + public static function getPages(): array + { + return [ + 'index' => Pages\ListBulkOperationRuns::route('/'), + 'view' => Pages\ViewBulkOperationRun::route('/{record}'), + ]; + } + + private static function statusColor(?string $status): string + { + return match ($status) { + 'completed' => 'success', + 'completed_with_errors' => 'warning', + 'failed' => 'danger', + 'running' => 'info', + default => 'gray', + }; + } +} diff --git a/app/Filament/Resources/BulkOperationRunResource/Pages/ListBulkOperationRuns.php b/app/Filament/Resources/BulkOperationRunResource/Pages/ListBulkOperationRuns.php new file mode 100644 index 0000000..ef4ea96 --- /dev/null +++ b/app/Filament/Resources/BulkOperationRunResource/Pages/ListBulkOperationRuns.php @@ -0,0 +1,11 @@ +label('Capture snapshot') ->requiresConfirmation() ->modalHeading('Capture snapshot now') - ->modalSubheading('This will fetch the latest configuration from Microsoft Graph and store a new policy version.') + ->modalSubheading('This queues a background job that fetches the latest configuration from Microsoft Graph and stores a new policy version.') ->form([ Forms\Components\Checkbox::make('include_assignments') ->label('Include assignments') @@ -37,51 +41,79 @@ protected function getActions(): array ->action(function (array $data) { $policy = $this->record; - try { - $tenant = $policy->tenant; + $tenant = $policy->tenant; + $user = auth()->user(); - if (! $tenant) { - Notification::make() - ->title('Policy has no tenant associated.') - ->danger() - ->send(); - - return; - } - - $version = app(VersionService::class)->captureFromGraph( - tenant: $tenant, - policy: $policy, - createdBy: auth()->user()?->email ?? null, - includeAssignments: $data['include_assignments'] ?? false, - includeScopeTags: $data['include_scope_tags'] ?? false, - ); - - if (($version->metadata['source'] ?? null) === 'metadata_only') { - $status = $version->metadata['original_status'] ?? null; - - Notification::make() - ->title('Snapshot captured (metadata only)') - ->body(sprintf( - 'Microsoft Graph returned %s for this policy type, so only local metadata was saved. Full restore is not possible until Graph works again.', - $status ?? 'an error' - )) - ->warning() - ->send(); - } else { - Notification::make() - ->title('Snapshot captured successfully.') - ->success() - ->send(); - } - - $this->redirect($this->getResource()::getUrl('view', ['record' => $policy->getKey()])); - } catch (\Throwable $e) { + if (! $tenant || ! $user) { Notification::make() - ->title('Failed to capture snapshot: '.$e->getMessage()) + ->title('Missing tenant or user context.') ->danger() ->send(); + + return; } + + $idempotencyKey = RunIdempotency::buildKey( + tenantId: $tenant->getKey(), + operationType: 'policy.capture_snapshot', + targetId: $policy->getKey() + ); + + $existingRun = RunIdempotency::findActiveBulkOperationRun( + tenantId: $tenant->getKey(), + idempotencyKey: $idempotencyKey + ); + + if ($existingRun) { + Notification::make() + ->title('Snapshot already in progress') + ->body('An active run already exists for this policy. Opening run details.') + ->actions([ + \Filament\Actions\Action::make('view_run') + ->label('View run') + ->url(BulkOperationRunResource::getUrl('view', ['record' => $existingRun], tenant: $tenant)), + ]) + ->info() + ->send(); + + $this->redirect(BulkOperationRunResource::getUrl('view', ['record' => $existingRun], tenant: $tenant)); + + return; + } + + $bulkOperationService = app(BulkOperationService::class); + + $run = $bulkOperationService->createRun( + tenant: $tenant, + user: $user, + resource: 'policies', + action: 'capture_snapshot', + itemIds: [(string) $policy->getKey()], + totalItems: 1 + ); + + $run->update(['idempotency_key' => $idempotencyKey]); + + CapturePolicySnapshotJob::dispatch( + bulkOperationRunId: $run->getKey(), + policyId: $policy->getKey(), + includeAssignments: (bool) ($data['include_assignments'] ?? false), + includeScopeTags: (bool) ($data['include_scope_tags'] ?? false), + createdBy: $user->email ? Str::limit($user->email, 255, '') : null + ); + + Notification::make() + ->title('Snapshot queued') + ->body('A background job has been queued. You can monitor progress in the run details.') + ->actions([ + \Filament\Actions\Action::make('view_run') + ->label('View run') + ->url(BulkOperationRunResource::getUrl('view', ['record' => $run], tenant: $tenant)), + ]) + ->success() + ->send(); + + $this->redirect(BulkOperationRunResource::getUrl('view', ['record' => $run], tenant: $tenant)); }) ->color('primary'), ]; diff --git a/app/Filament/Resources/RestoreRunResource.php b/app/Filament/Resources/RestoreRunResource.php index 516e6c1..9dbc2d6 100644 --- a/app/Filament/Resources/RestoreRunResource.php +++ b/app/Filament/Resources/RestoreRunResource.php @@ -11,12 +11,14 @@ use App\Models\BackupSet; use App\Models\RestoreRun; use App\Models\Tenant; +use App\Rules\SkipOrUuidRule; use App\Services\BulkOperationService; use App\Services\Intune\AuditLogger; use App\Services\Intune\RestoreDiffGenerator; use App\Services\Intune\RestoreRiskChecker; use App\Services\Intune\RestoreService; use App\Support\RestoreRunStatus; +use App\Support\RunIdempotency; use BackedEnum; use Filament\Actions; use Filament\Actions\ActionGroup; @@ -36,6 +38,7 @@ use Filament\Tables\Filters\TrashedFilter; use Filament\Tables\Table; use Illuminate\Database\Eloquent\Collection; +use Illuminate\Database\QueryException; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Str; use Illuminate\Validation\ValidationException; @@ -115,31 +118,7 @@ public static function form(Schema $schema): Schema return Forms\Components\TextInput::make("group_mapping.{$groupId}") ->label($label) ->placeholder('SKIP or target group Object ID (GUID)') - ->rules([ - static function (string $attribute, mixed $value, \Closure $fail): void { - if (! is_string($value)) { - $fail('Please enter SKIP or a valid UUID.'); - - return; - } - - $value = trim($value); - - if ($value === '') { - $fail('Please enter SKIP or a valid UUID.'); - - return; - } - - if (strtoupper($value) === 'SKIP') { - return; - } - - if (! Str::isUuid($value)) { - $fail('Please enter SKIP or a valid UUID.'); - } - }, - ]) + ->rules([new SkipOrUuidRule]) ->required() ->helperText('Paste the target Entra ID group Object ID (GUID). Names are not resolved in this phase. Use SKIP to omit the assignment.'); }, $unresolved); @@ -345,31 +324,7 @@ public static function getWizardSteps(): array return Forms\Components\TextInput::make("group_mapping.{$groupId}") ->label($label) ->placeholder('SKIP or target group Object ID (GUID)') - ->rules([ - static function (string $attribute, mixed $value, \Closure $fail): void { - if (! is_string($value)) { - $fail('Please enter SKIP or a valid UUID.'); - - return; - } - - $value = trim($value); - - if ($value === '') { - $fail('Please enter SKIP or a valid UUID.'); - - return; - } - - if (strtoupper($value) === 'SKIP') { - return; - } - - if (! Str::isUuid($value)) { - $fail('Please enter SKIP or a valid UUID.'); - } - }, - ]) + ->rules([new SkipOrUuidRule]) ->reactive() ->afterStateUpdated(function (Set $set): void { $set('check_summary', null); @@ -678,6 +633,15 @@ public static function table(Table $table): Table Tables\Columns\TextColumn::make('backupSet.name')->label('Backup set'), Tables\Columns\IconColumn::make('is_dry_run')->label('Dry-run')->boolean(), Tables\Columns\TextColumn::make('status')->badge(), + Tables\Columns\TextColumn::make('summary_total') + ->label('Total') + ->state(fn (RestoreRun $record): int => (int) (($record->metadata ?? [])['total'] ?? 0)), + Tables\Columns\TextColumn::make('summary_succeeded') + ->label('Succeeded') + ->state(fn (RestoreRun $record): int => (int) (($record->metadata ?? [])['succeeded'] ?? 0)), + Tables\Columns\TextColumn::make('summary_failed') + ->label('Failed') + ->state(fn (RestoreRun $record): int => (int) (($record->metadata ?? [])['failed'] ?? 0)), Tables\Columns\TextColumn::make('started_at')->dateTime()->since(), Tables\Columns\TextColumn::make('completed_at')->dateTime()->since(), Tables\Columns\TextColumn::make('requested_by')->label('Requested by'), @@ -722,6 +686,116 @@ public static function table(Table $table): Table return; } + if (! (bool) $record->is_dry_run) { + $selectedItemIds = is_array($record->requested_items) ? $record->requested_items : null; + $groupMapping = is_array($record->group_mapping) ? $record->group_mapping : []; + $actorEmail = auth()->user()?->email; + $actorName = auth()->user()?->name; + $tenantIdentifier = $tenant->tenant_id ?? $tenant->external_id; + $highlanderLabel = (string) ($tenant->name ?? $tenantIdentifier ?? $tenant->getKey()); + + $preview = $restoreService->preview($tenant, $backupSet, $selectedItemIds); + $metadata = [ + 'scope_mode' => $selectedItemIds === null ? 'all' : 'selected', + 'environment' => app()->environment('production') ? 'prod' : 'test', + 'highlander_label' => $highlanderLabel, + 'confirmed_at' => now()->toIso8601String(), + 'confirmed_by' => $actorEmail, + 'confirmed_by_name' => $actorName, + 'rerun_of_restore_run_id' => $record->id, + ]; + + $idempotencyKey = RunIdempotency::restoreExecuteKey( + tenantId: (int) $tenant->getKey(), + backupSetId: (int) $backupSet->getKey(), + selectedItemIds: $selectedItemIds, + groupMapping: $groupMapping, + ); + + $existing = RunIdempotency::findActiveRestoreRun((int) $tenant->getKey(), $idempotencyKey); + + if ($existing) { + Notification::make() + ->title('Restore already queued') + ->body('Reusing the active restore run.') + ->info() + ->send(); + + return; + } + + try { + $newRun = RestoreRun::create([ + 'tenant_id' => $tenant->id, + 'backup_set_id' => $backupSet->id, + 'requested_by' => $actorEmail, + 'is_dry_run' => false, + 'status' => RestoreRunStatus::Queued->value, + 'idempotency_key' => $idempotencyKey, + 'requested_items' => $selectedItemIds, + 'preview' => $preview, + 'metadata' => $metadata, + 'group_mapping' => $groupMapping !== [] ? $groupMapping : null, + ]); + } catch (QueryException $exception) { + $existing = RunIdempotency::findActiveRestoreRun((int) $tenant->getKey(), $idempotencyKey); + + if ($existing) { + Notification::make() + ->title('Restore already queued') + ->body('Reusing the active restore run.') + ->info() + ->send(); + + return; + } + + throw $exception; + } + + $auditLogger->log( + tenant: $tenant, + action: 'restore.queued', + context: [ + 'metadata' => [ + 'restore_run_id' => $newRun->id, + 'backup_set_id' => $backupSet->id, + 'rerun_of_restore_run_id' => $record->id, + ], + ], + actorEmail: $actorEmail, + actorName: $actorName, + resourceType: 'restore_run', + resourceId: (string) $newRun->id, + status: 'success', + ); + + ExecuteRestoreRunJob::dispatch($newRun->id, $actorEmail, $actorName); + + $auditLogger->log( + tenant: $tenant, + action: 'restore_run.rerun', + resourceType: 'restore_run', + resourceId: (string) $newRun->id, + status: 'success', + context: [ + 'metadata' => [ + 'original_restore_run_id' => $record->id, + 'backup_set_id' => $backupSet->id, + ], + ], + actorEmail: $actorEmail, + actorName: $actorName, + ); + + Notification::make() + ->title('Restore run queued') + ->success() + ->send(); + + return; + } + try { $newRun = $restoreService->execute( tenant: $tenant, @@ -1008,6 +1082,16 @@ public static function infolist(Schema $schema): Schema ->schema([ Infolists\Components\TextEntry::make('backupSet.name')->label('Backup set'), Infolists\Components\TextEntry::make('status')->badge(), + Infolists\Components\TextEntry::make('counts') + ->label('Counts') + ->state(function (RestoreRun $record): string { + $meta = $record->metadata ?? []; + $total = (int) ($meta['total'] ?? 0); + $succeeded = (int) ($meta['succeeded'] ?? 0); + $failed = (int) ($meta['failed'] ?? 0); + + return sprintf('Total: %d • Succeeded: %d • Failed: %d', $total, $succeeded, $failed); + }), Infolists\Components\TextEntry::make('is_dry_run') ->label('Dry-run') ->formatStateUsing(fn ($state) => $state ? 'Yes' : 'No') @@ -1327,17 +1411,53 @@ public static function createRestoreRun(array $data): RestoreRun $metadata['preview_ran_at'] = $previewRanAt; } - $restoreRun = RestoreRun::create([ - 'tenant_id' => $tenant->id, - 'backup_set_id' => $backupSet->id, - 'requested_by' => $actorEmail, - 'is_dry_run' => false, - 'status' => RestoreRunStatus::Queued->value, - 'requested_items' => $selectedItemIds, - 'preview' => $preview, - 'metadata' => $metadata, - 'group_mapping' => $groupMapping !== [] ? $groupMapping : null, - ]); + $idempotencyKey = RunIdempotency::restoreExecuteKey( + tenantId: (int) $tenant->getKey(), + backupSetId: (int) $backupSet->getKey(), + selectedItemIds: $selectedItemIds, + groupMapping: $groupMapping, + ); + + $existing = RunIdempotency::findActiveRestoreRun((int) $tenant->getKey(), $idempotencyKey); + + if ($existing) { + Notification::make() + ->title('Restore already queued') + ->body('Reusing the active restore run.') + ->info() + ->send(); + + return $existing; + } + + try { + $restoreRun = RestoreRun::create([ + 'tenant_id' => $tenant->id, + 'backup_set_id' => $backupSet->id, + 'requested_by' => $actorEmail, + 'is_dry_run' => false, + 'status' => RestoreRunStatus::Queued->value, + 'idempotency_key' => $idempotencyKey, + 'requested_items' => $selectedItemIds, + 'preview' => $preview, + 'metadata' => $metadata, + 'group_mapping' => $groupMapping !== [] ? $groupMapping : null, + ]); + } catch (QueryException $exception) { + $existing = RunIdempotency::findActiveRestoreRun((int) $tenant->getKey(), $idempotencyKey); + + if ($existing) { + Notification::make() + ->title('Restore already queued') + ->body('Reusing the active restore run.') + ->info() + ->send(); + + return $existing; + } + + throw $exception; + } app(AuditLogger::class)->log( tenant: $tenant, diff --git a/app/Jobs/CapturePolicySnapshotJob.php b/app/Jobs/CapturePolicySnapshotJob.php new file mode 100644 index 0000000..f5f2917 --- /dev/null +++ b/app/Jobs/CapturePolicySnapshotJob.php @@ -0,0 +1,107 @@ +with(['tenant', 'user'])->find($this->bulkOperationRunId); + + if (! $run) { + return; + } + + $policy = Policy::query()->with('tenant')->find($this->policyId); + + if (! $policy || ! $policy->tenant) { + $bulkOperationService->abort($run, 'policy_not_found'); + $this->notifyStatus($run, 'failed'); + + return; + } + + $this->notifyStatus($run, 'queued'); + $bulkOperationService->start($run); + $this->notifyStatus($run, 'running'); + + try { + $versionService->captureFromGraph( + tenant: $policy->tenant, + policy: $policy, + createdBy: $this->createdBy, + includeAssignments: $this->includeAssignments, + includeScopeTags: $this->includeScopeTags, + ); + + $bulkOperationService->recordSuccess($run); + $bulkOperationService->complete($run); + + $this->notifyStatus($run, $run->refresh()->status); + } catch (Throwable $e) { + $bulkOperationService->recordFailure( + run: $run, + itemId: (string) $policy->getKey(), + reason: $bulkOperationService->sanitizeFailureReason($e->getMessage()) + ); + + $bulkOperationService->complete($run); + + $this->notifyStatus($run->refresh(), $run->status); + + throw $e; + } + } + + private function notifyStatus(BulkOperationRun $run, string $status): void + { + if (! $run->relationLoaded('user')) { + $run->loadMissing('user'); + } + + if (! $run->user) { + return; + } + + $normalizedStatus = $status === 'pending' ? 'queued' : $status; + + $run->user->notify(new RunStatusChangedNotification([ + 'tenant_id' => (int) $run->tenant_id, + 'run_type' => 'bulk_operation', + 'run_id' => (int) $run->getKey(), + 'status' => (string) $normalizedStatus, + 'counts' => [ + 'total' => (int) $run->total_items, + 'processed' => (int) $run->processed_items, + 'succeeded' => (int) $run->succeeded, + 'failed' => (int) $run->failed, + 'skipped' => (int) $run->skipped, + ], + ])); + } +} diff --git a/app/Jobs/ExecuteRestoreRunJob.php b/app/Jobs/ExecuteRestoreRunJob.php index dd57d20..2216120 100644 --- a/app/Jobs/ExecuteRestoreRunJob.php +++ b/app/Jobs/ExecuteRestoreRunJob.php @@ -3,6 +3,9 @@ namespace App\Jobs; use App\Models\RestoreRun; +use App\Models\User; +use App\Notifications\RunStatusChangedNotification; +use App\Services\BulkOperationService; use App\Services\Intune\AuditLogger; use App\Services\Intune\RestoreService; use App\Support\RestoreRunStatus; @@ -24,7 +27,7 @@ public function __construct( public ?string $actorName = null, ) {} - public function handle(RestoreService $restoreService, AuditLogger $auditLogger): void + public function handle(RestoreService $restoreService, AuditLogger $auditLogger, BulkOperationService $bulkOperationService): void { $restoreRun = RestoreRun::with(['tenant', 'backupSet'])->find($this->restoreRunId); @@ -36,6 +39,8 @@ public function handle(RestoreService $restoreService, AuditLogger $auditLogger) return; } + $this->notifyStatus($restoreRun, 'queued'); + $tenant = $restoreRun->tenant; $backupSet = $restoreRun->backupSet; @@ -46,6 +51,8 @@ public function handle(RestoreService $restoreService, AuditLogger $auditLogger) 'completed_at' => CarbonImmutable::now(), ]); + $this->notifyStatus($restoreRun->refresh(), 'failed'); + if ($tenant) { $auditLogger->log( tenant: $tenant, @@ -74,6 +81,8 @@ public function handle(RestoreService $restoreService, AuditLogger $auditLogger) 'failure_reason' => null, ]); + $this->notifyStatus($restoreRun->refresh(), 'running'); + $auditLogger->log( tenant: $tenant, action: 'restore.started', @@ -98,17 +107,23 @@ public function handle(RestoreService $restoreService, AuditLogger $auditLogger) actorEmail: $this->actorEmail, actorName: $this->actorName, ); + + $this->notifyStatus($restoreRun->refresh(), (string) $restoreRun->status); } catch (Throwable $throwable) { $restoreRun->refresh(); + $safeReason = $bulkOperationService->sanitizeFailureReason($throwable->getMessage()); + if ($restoreRun->status === RestoreRunStatus::Running->value) { $restoreRun->update([ 'status' => RestoreRunStatus::Failed->value, - 'failure_reason' => $throwable->getMessage(), + 'failure_reason' => $safeReason, 'completed_at' => CarbonImmutable::now(), ]); } + $this->notifyStatus($restoreRun->refresh(), (string) $restoreRun->status); + if ($tenant) { $auditLogger->log( tenant: $tenant, @@ -117,7 +132,7 @@ public function handle(RestoreService $restoreService, AuditLogger $auditLogger) 'metadata' => [ 'restore_run_id' => $restoreRun->id, 'backup_set_id' => $backupSet->id, - 'reason' => $throwable->getMessage(), + 'reason' => $safeReason, ], ], actorEmail: $this->actorEmail, @@ -131,4 +146,45 @@ public function handle(RestoreService $restoreService, AuditLogger $auditLogger) throw $throwable; } } + + private function notifyStatus(RestoreRun $restoreRun, string $status): void + { + $email = $this->actorEmail; + + if (! is_string($email) || $email === '') { + $email = is_string($restoreRun->requested_by) ? $restoreRun->requested_by : null; + } + + if (! is_string($email) || $email === '') { + return; + } + + $user = User::query()->where('email', $email)->first(); + + if (! $user) { + return; + } + + $metadata = is_array($restoreRun->metadata) ? $restoreRun->metadata : []; + $counts = []; + + foreach (['total', 'succeeded', 'failed', 'skipped'] as $key) { + if (array_key_exists($key, $metadata) && is_numeric($metadata[$key])) { + $counts[$key] = (int) $metadata[$key]; + } + } + + $payload = [ + 'tenant_id' => (int) $restoreRun->tenant_id, + 'run_type' => 'restore', + 'run_id' => (int) $restoreRun->getKey(), + 'status' => $status, + ]; + + if ($counts !== []) { + $payload['counts'] = $counts; + } + + $user->notify(new RunStatusChangedNotification($payload)); + } } diff --git a/app/Models/BulkOperationRun.php b/app/Models/BulkOperationRun.php index 9342a26..5880dba 100644 --- a/app/Models/BulkOperationRun.php +++ b/app/Models/BulkOperationRun.php @@ -15,6 +15,7 @@ class BulkOperationRun extends Model 'user_id', 'resource', 'action', + 'idempotency_key', 'status', 'total_items', 'processed_items', diff --git a/app/Models/RestoreRun.php b/app/Models/RestoreRun.php index e4d3ce0..19966ea 100644 --- a/app/Models/RestoreRun.php +++ b/app/Models/RestoreRun.php @@ -18,6 +18,7 @@ class RestoreRun extends Model protected $casts = [ 'is_dry_run' => 'boolean', + 'idempotency_key' => 'string', 'requested_items' => 'array', 'preview' => 'array', 'results' => 'array', @@ -104,6 +105,15 @@ public function getAssignmentRestoreOutcomes(): array return $results['assignment_outcomes']; } + if (isset($results['items']) && is_array($results['items'])) { + return collect($results['items']) + ->pluck('assignment_outcomes') + ->flatten(1) + ->filter() + ->values() + ->all(); + } + if (! is_array($results)) { return []; } diff --git a/app/Notifications/RunStatusChangedNotification.php b/app/Notifications/RunStatusChangedNotification.php new file mode 100644 index 0000000..2faab26 --- /dev/null +++ b/app/Notifications/RunStatusChangedNotification.php @@ -0,0 +1,94 @@ + + */ + public function via(object $notifiable): array + { + return ['database']; + } + + /** + * @return array + */ + public function toDatabase(object $notifiable): array + { + $status = (string) ($this->metadata['status'] ?? 'queued'); + $runType = (string) ($this->metadata['run_type'] ?? 'run'); + $tenantId = (int) ($this->metadata['tenant_id'] ?? 0); + $runId = (int) ($this->metadata['run_id'] ?? 0); + + $title = match ($status) { + 'queued' => 'Run queued', + 'running' => 'Run started', + 'completed', 'succeeded' => 'Run completed', + 'partial', 'completed_with_errors' => 'Run completed (partial)', + 'failed' => 'Run failed', + default => 'Run updated', + }; + + $body = sprintf('A %s run changed status to: %s.', str_replace('_', ' ', $runType), $status); + + $color = match ($status) { + 'queued', 'running' => 'gray', + 'completed', 'succeeded' => 'success', + 'partial', 'completed_with_errors' => 'warning', + 'failed' => 'danger', + default => 'gray', + }; + + $actions = []; + + if (in_array($runType, ['bulk_operation', 'restore'], true) && $tenantId > 0 && $runId > 0) { + $tenant = Tenant::query()->find($tenantId); + + if ($tenant) { + $url = $runType === 'bulk_operation' + ? BulkOperationRunResource::getUrl('view', ['record' => $runId], tenant: $tenant) + : RestoreRunResource::getUrl('view', ['record' => $runId], tenant: $tenant); + + $actions[] = Action::make('view_run') + ->label('View run') + ->url($url) + ->toArray(); + } + } + + return [ + 'format' => 'filament', + 'title' => $title, + 'body' => $body, + 'color' => $color, + 'duration' => 'persistent', + 'actions' => $actions, + 'icon' => null, + 'iconColor' => null, + 'status' => null, + 'view' => null, + 'viewData' => [ + 'metadata' => $this->metadata, + ], + ]; + } +} diff --git a/app/Policies/BulkOperationRunPolicy.php b/app/Policies/BulkOperationRunPolicy.php new file mode 100644 index 0000000..6ee6a87 --- /dev/null +++ b/app/Policies/BulkOperationRunPolicy.php @@ -0,0 +1,39 @@ +canAccessTenant($tenant); + } + + public function view(User $user, BulkOperationRun $run): bool + { + $tenant = Tenant::current(); + + if (! $tenant) { + return false; + } + + if (! $user->canAccessTenant($tenant)) { + return false; + } + + return (int) $run->tenant_id === (int) $tenant->getKey(); + } +} diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 248c52c..a605e4a 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -107,5 +107,6 @@ public function boot(): void }); Gate::policy(BackupSchedule::class, BackupSchedulePolicy::class); + Gate::policy(BulkOperationRun::class, BulkOperationRunPolicy::class); } } diff --git a/app/Rules/SkipOrUuidRule.php b/app/Rules/SkipOrUuidRule.php new file mode 100644 index 0000000..57aa15c --- /dev/null +++ b/app/Rules/SkipOrUuidRule.php @@ -0,0 +1,37 @@ +allowSkip && strtoupper($value) === 'SKIP') { + return; + } + + if (! Str::isUuid($value)) { + $fail('Please enter SKIP or a valid UUID.'); + } + } +} diff --git a/app/Services/BulkOperationService.php b/app/Services/BulkOperationService.php index 8f3f158..b85193a 100644 --- a/app/Services/BulkOperationService.php +++ b/app/Services/BulkOperationService.php @@ -13,6 +13,30 @@ public function __construct( protected AuditLogger $auditLogger ) {} + public function sanitizeFailureReason(string $reason): string + { + $reason = trim($reason); + + if ($reason === '') { + return 'error'; + } + + $lower = mb_strtolower($reason); + + if ( + str_contains($lower, 'bearer ') || + str_contains($lower, 'access_token') || + str_contains($lower, 'client_secret') || + str_contains($lower, 'authorization') + ) { + return 'redacted'; + } + + $reason = preg_replace("/\s+/u", ' ', $reason) ?? $reason; + + return mb_substr($reason, 0, 200); + } + public function createRun( Tenant $tenant, User $user, @@ -70,6 +94,8 @@ public function recordSuccess(BulkOperationRun $run): void public function recordFailure(BulkOperationRun $run, string $itemId, string $reason): void { + $reason = $this->sanitizeFailureReason($reason); + $failures = $run->failures ?? []; $failures[] = [ 'item_id' => $itemId, @@ -92,6 +118,8 @@ public function recordSkipped(BulkOperationRun $run): void public function recordSkippedWithReason(BulkOperationRun $run, string $itemId, string $reason): void { + $reason = $this->sanitizeFailureReason($reason); + $failures = $run->failures ?? []; $failures[] = [ 'item_id' => $itemId, @@ -164,6 +192,8 @@ public function fail(BulkOperationRun $run, string $reason): void { $run->update(['status' => 'failed']); + $reason = $this->sanitizeFailureReason($reason); + $this->auditLogger->log( tenant: $run->tenant, action: "bulk.{$run->resource}.{$run->action}.failed", @@ -184,6 +214,8 @@ public function abort(BulkOperationRun $run, string $reason): void { $run->update(['status' => 'aborted']); + $reason = $this->sanitizeFailureReason($reason); + $this->auditLogger->log( tenant: $run->tenant, action: "bulk.{$run->resource}.{$run->action}.aborted", diff --git a/app/Services/Intune/RestoreService.php b/app/Services/Intune/RestoreService.php index b62ed49..6c00464 100644 --- a/app/Services/Intune/RestoreService.php +++ b/app/Services/Intune/RestoreService.php @@ -327,6 +327,26 @@ public function execute( $foundationEntries = $foundationOutcome['entries'] ?? []; $foundationFailures = (int) ($foundationOutcome['failed'] ?? 0); $foundationSkipped = (int) ($foundationOutcome['skipped'] ?? 0); + + $foundationBackupItemIdsBySourceId = $foundationItems + ->pluck('id', 'policy_identifier') + ->map(fn ($id) => is_int($id) ? $id : null) + ->filter() + ->all(); + + $foundationEntries = array_values(array_map(function (mixed $entry) use ($foundationBackupItemIdsBySourceId): mixed { + if (! is_array($entry)) { + return $entry; + } + + $sourceId = $entry['sourceId'] ?? null; + + if (is_string($sourceId) && isset($foundationBackupItemIdsBySourceId[$sourceId])) { + $entry['backup_item_id'] = (int) $foundationBackupItemIdsBySourceId[$sourceId]; + } + + return $entry; + }, $foundationEntries)); $foundationMappingByType = $this->buildFoundationMappingByType($foundationEntries); $scopeTagMapping = $foundationMappingByType['roleScopeTag'] ?? []; $scopeTagNamesById = $this->buildScopeTagNameLookup($foundationEntries); @@ -866,14 +886,62 @@ public function execute( default => 'completed', }); + $isFoundationEntry = static function (mixed $item): bool { + return is_array($item) + && array_key_exists('decision', $item) + && array_key_exists('sourceId', $item); + }; + + $foundationResults = collect($results)->filter($isFoundationEntry)->values(); + $policyResults = collect($results)->reject($isFoundationEntry)->values(); + + $itemsByBackupItemId = $policyResults + ->filter(fn (mixed $item): bool => is_array($item) && isset($item['backup_item_id']) && is_numeric($item['backup_item_id'])) + ->keyBy(fn (array $item): string => (string) $item['backup_item_id']) + ->all(); + + $policyStatusCounts = collect($itemsByBackupItemId) + ->map(fn (array $item): string => (string) ($item['status'] ?? 'unknown')) + ->countBy(); + + $foundationDecisionCounts = $foundationResults + ->map(fn (array $entry): string => (string) ($entry['decision'] ?? 'unknown')) + ->countBy(); + + $policyTotal = count($itemsByBackupItemId); + $foundationTotal = $foundationResults->count(); + $total = $policyTotal + $foundationTotal; + + $succeeded = (int) ($policyStatusCounts['applied'] ?? 0) + + $foundationDecisionCounts + ->except(['failed', 'skipped']) + ->sum(); + + $failed = (int) ($policyStatusCounts['failed'] ?? 0) + + (int) ($foundationDecisionCounts['failed'] ?? 0); + + $skipped = (int) ($policyStatusCounts['skipped'] ?? 0) + + (int) ($foundationDecisionCounts['skipped'] ?? 0); + + $partialCount = (int) ($policyStatusCounts['partial'] ?? 0) + + (int) ($policyStatusCounts['manual_required'] ?? 0); + + $persistedResults = [ + 'foundations' => $foundationResults->all(), + 'items' => $itemsByBackupItemId, + ]; + $restoreRun->update([ 'status' => $status, - 'results' => $results, + 'results' => $persistedResults, 'completed_at' => CarbonImmutable::now(), 'metadata' => array_merge($restoreRun->metadata ?? [], [ - 'failed' => $hardFailures, + 'total' => $total, + 'succeeded' => $succeeded, + 'failed' => $failed, + 'skipped' => $skipped, + 'partial' => $partialCount, 'non_applied' => $nonApplied, - 'total' => $totalCount, 'foundations_skipped' => $foundationSkipped, ]), ]); diff --git a/app/Support/RunIdempotency.php b/app/Support/RunIdempotency.php new file mode 100644 index 0000000..574afcf --- /dev/null +++ b/app/Support/RunIdempotency.php @@ -0,0 +1,95 @@ + $context + */ + public static function buildKey(int $tenantId, string $operationType, string|int|null $targetId = null, array $context = []): string + { + $payload = [ + 'tenant_id' => $tenantId, + 'operation_type' => trim($operationType), + 'target_id' => $targetId === null ? null : (string) $targetId, + 'context' => self::canonicalize($context), + ]; + + return hash('sha256', json_encode($payload, JSON_THROW_ON_ERROR)); + } + + public static function findActiveBulkOperationRun(int $tenantId, string $idempotencyKey): ?BulkOperationRun + { + return BulkOperationRun::query() + ->where('tenant_id', $tenantId) + ->where('idempotency_key', $idempotencyKey) + ->whereIn('status', ['pending', 'running']) + ->latest('id') + ->first(); + } + + public static function findActiveRestoreRun(int $tenantId, string $idempotencyKey): ?RestoreRun + { + return RestoreRun::query() + ->where('tenant_id', $tenantId) + ->where('idempotency_key', $idempotencyKey) + ->whereIn('status', ['queued', 'running']) + ->latest('id') + ->first(); + } + + /** + * Deterministic idempotency key for a live restore execution. + * + * @param array|null $selectedItemIds + * @param array $groupMapping + */ + public static function restoreExecuteKey( + int $tenantId, + int $backupSetId, + ?array $selectedItemIds, + array $groupMapping = [], + ): string { + $scopeIds = $selectedItemIds; + + if (is_array($scopeIds)) { + $scopeIds = array_values(array_unique(array_map('intval', $scopeIds))); + sort($scopeIds); + } + + return self::buildKey( + tenantId: $tenantId, + operationType: 'restore.execute', + targetId: (string) $backupSetId, + context: [ + 'scope' => $scopeIds, + 'group_mapping' => $groupMapping, + ], + ); + } + + /** + * @param array $value + * @return array + */ + private static function canonicalize(array $value): array + { + $value = Arr::map($value, function (mixed $item): mixed { + if (is_array($item)) { + /** @var array $item */ + return static::canonicalize($item); + } + + return $item; + }); + + ksort($value); + + return $value; + } +} diff --git a/database/migrations/2026_01_11_120001_add_idempotency_key_to_bulk_operation_runs_table.php b/database/migrations/2026_01_11_120001_add_idempotency_key_to_bulk_operation_runs_table.php new file mode 100644 index 0000000..9f9926b --- /dev/null +++ b/database/migrations/2026_01_11_120001_add_idempotency_key_to_bulk_operation_runs_table.php @@ -0,0 +1,32 @@ +string('idempotency_key', 64)->nullable()->after('action'); + }); + + Schema::table('bulk_operation_runs', function (Blueprint $table) { + $table->index(['tenant_id', 'idempotency_key'], 'bulk_runs_tenant_idempotency'); + }); + + DB::statement("CREATE UNIQUE INDEX bulk_runs_idempotency_active ON bulk_operation_runs (tenant_id, idempotency_key) WHERE idempotency_key IS NOT NULL AND status IN ('pending', 'running')"); + } + + public function down(): void + { + DB::statement('DROP INDEX IF EXISTS bulk_runs_idempotency_active'); + + Schema::table('bulk_operation_runs', function (Blueprint $table) { + $table->dropIndex('bulk_runs_tenant_idempotency'); + $table->dropColumn('idempotency_key'); + }); + } +}; diff --git a/database/migrations/2026_01_11_120002_add_idempotency_key_to_restore_runs_table.php b/database/migrations/2026_01_11_120002_add_idempotency_key_to_restore_runs_table.php new file mode 100644 index 0000000..4c5cbc1 --- /dev/null +++ b/database/migrations/2026_01_11_120002_add_idempotency_key_to_restore_runs_table.php @@ -0,0 +1,32 @@ +string('idempotency_key', 64)->nullable()->after('status'); + }); + + Schema::table('restore_runs', function (Blueprint $table) { + $table->index(['tenant_id', 'idempotency_key'], 'restore_runs_tenant_idempotency'); + }); + + DB::statement("CREATE UNIQUE INDEX restore_runs_idempotency_active ON restore_runs (tenant_id, idempotency_key) WHERE idempotency_key IS NOT NULL AND status IN ('queued', 'running')"); + } + + public function down(): void + { + DB::statement('DROP INDEX IF EXISTS restore_runs_idempotency_active'); + + Schema::table('restore_runs', function (Blueprint $table) { + $table->dropIndex('restore_runs_tenant_idempotency'); + $table->dropColumn('idempotency_key'); + }); + } +}; diff --git a/resources/views/filament/infolists/entries/restore-results.blade.php b/resources/views/filament/infolists/entries/restore-results.blade.php index 0c9e694..96a6b81 100644 --- a/resources/views/filament/infolists/entries/restore-results.blade.php +++ b/resources/views/filament/infolists/entries/restore-results.blade.php @@ -1,14 +1,21 @@ @php - $results = $getState() ?? []; - $foundationItems = collect($results)->filter(function ($item) { + $state = $getState() ?? []; + $isFoundationEntry = function ($item) { return is_array($item) && array_key_exists('decision', $item) && array_key_exists('sourceId', $item); - }); - $policyItems = collect($results)->reject(function ($item) { - return is_array($item) && array_key_exists('decision', $item) && array_key_exists('sourceId', $item); - }); + }; + + if (is_array($state) && array_key_exists('items', $state)) { + $foundationItems = collect($state['foundations'] ?? [])->filter($isFoundationEntry); + $policyItems = collect($state['items'] ?? [])->values(); + $results = $state; + } else { + $results = $state; + $foundationItems = collect($results)->filter($isFoundationEntry); + $policyItems = collect($results)->reject($isFoundationEntry); + } @endphp -@if (empty($results)) +@if ($foundationItems->isEmpty() && $policyItems->isEmpty())

No results recorded.

@else @php -- 2.45.2 From 7b01ef034bc6d69369bdcad15b0751fdb58ca5d4 Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Sun, 11 Jan 2026 16:55:13 +0100 Subject: [PATCH 7/7] test: stabilize restore and run authorization --- tests/Feature/ExecuteRestoreRunJobTest.php | 63 ++++++++++- .../ConditionalAccessPreviewOnlyTest.php | 9 +- .../EnrollmentRestrictionsPreviewOnlyTest.php | 18 ++-- .../GroupPolicyConfigurationRestoreTest.php | 6 +- .../Filament/ODataTypeMismatchTest.php | 5 +- .../PolicyCaptureSnapshotOptionsTest.php | 16 +++ .../PolicyVersionRestoreToIntuneTest.php | 9 +- .../Feature/Filament/RestoreExecutionTest.php | 26 +++-- ...gsCatalogRestoreApplySettingsPatchTest.php | 15 +-- .../Filament/SettingsCatalogRestoreTest.php | 28 +++-- .../WindowsUpdateProfilesRestoreTest.php | 12 ++- .../Filament/WindowsUpdateRingRestoreTest.php | 4 +- .../PolicyCaptureSnapshotIdempotencyTest.php | 46 ++++++++ .../PolicyCaptureSnapshotQueuedTest.php | 48 +++++++++ .../RestoreAssignmentApplicationTest.php | 12 ++- tests/Feature/RestoreAuditLoggingTest.php | 69 ++++++++++++ .../Feature/RestoreGraphErrorMetadataTest.php | 2 +- tests/Feature/RestoreGroupMappingTest.php | 8 +- .../Feature/RestorePreviewDiffWizardTest.php | 4 + tests/Feature/RestoreRiskChecksWizardTest.php | 4 + tests/Feature/RestoreRunIdempotencyTest.php | 102 ++++++++++++++++++ .../RestoreUnknownPolicyTypeSafetyTest.php | 2 +- .../RunAuthorizationTenantIsolationTest.php | 58 ++++++++++ tests/Feature/RunStartAuthorizationTest.php | 34 ++++++ tests/Unit/RunIdempotencyTest.php | 57 ++++++++++ 25 files changed, 604 insertions(+), 53 deletions(-) create mode 100644 tests/Feature/PolicyCaptureSnapshotIdempotencyTest.php create mode 100644 tests/Feature/PolicyCaptureSnapshotQueuedTest.php create mode 100644 tests/Feature/RestoreAuditLoggingTest.php create mode 100644 tests/Feature/RestoreRunIdempotencyTest.php create mode 100644 tests/Feature/RunAuthorizationTenantIsolationTest.php create mode 100644 tests/Feature/RunStartAuthorizationTest.php create mode 100644 tests/Unit/RunIdempotencyTest.php diff --git a/tests/Feature/ExecuteRestoreRunJobTest.php b/tests/Feature/ExecuteRestoreRunJobTest.php index 9fb258a..3f2bc73 100644 --- a/tests/Feature/ExecuteRestoreRunJobTest.php +++ b/tests/Feature/ExecuteRestoreRunJobTest.php @@ -1,9 +1,12 @@ id, 'actor@example.com', 'Actor'); - $job->handle($restoreService, app(AuditLogger::class)); + $job->handle($restoreService, app(AuditLogger::class), app(BulkOperationService::class)); $restoreRun->refresh(); expect($restoreRun->started_at)->not->toBeNull(); expect($restoreRun->status)->toBe(RestoreRunStatus::Completed->value); }); + +test('execute restore run job persists per-item outcomes keyed by backup_item_id', function () { + $tenant = Tenant::create([ + 'tenant_id' => 'tenant-results', + 'name' => 'Tenant Results', + 'metadata' => [], + ]); + + $backupSet = BackupSet::create([ + 'tenant_id' => $tenant->id, + 'name' => 'Backup', + 'status' => 'completed', + 'item_count' => 1, + ]); + + $policy = Policy::create([ + 'tenant_id' => $tenant->id, + 'external_id' => 'policy-results', + 'policy_type' => 'unknownPreviewOnlyType', + 'display_name' => 'Preview-only policy', + 'platform' => 'windows', + ]); + + $backupItem = BackupItem::create([ + 'tenant_id' => $tenant->id, + 'backup_set_id' => $backupSet->id, + 'policy_id' => $policy->id, + 'policy_identifier' => $policy->external_id, + 'policy_type' => $policy->policy_type, + 'platform' => $policy->platform, + 'payload' => ['id' => $policy->external_id], + 'metadata' => [ + 'displayName' => 'Backup Policy', + ], + ]); + + $restoreRun = RestoreRun::create([ + 'tenant_id' => $tenant->id, + 'backup_set_id' => $backupSet->id, + 'requested_by' => 'actor@example.com', + 'is_dry_run' => false, + 'status' => RestoreRunStatus::Queued->value, + 'requested_items' => [$backupItem->id], + 'preview' => [], + 'results' => null, + 'metadata' => [], + ]); + + $job = new ExecuteRestoreRunJob($restoreRun->id, 'actor@example.com', 'Actor'); + $job->handle(app(RestoreService::class), app(AuditLogger::class), app(BulkOperationService::class)); + + $restoreRun->refresh(); + + expect($restoreRun->completed_at)->not->toBeNull(); + expect($restoreRun->results)->toBeArray(); + expect($restoreRun->results['items'][(string) $backupItem->id]['backup_item_id'] ?? null)->toBe($backupItem->id); + expect($restoreRun->results['items'][(string) $backupItem->id]['status'] ?? null)->toBe('skipped'); +}); diff --git a/tests/Feature/Filament/ConditionalAccessPreviewOnlyTest.php b/tests/Feature/Filament/ConditionalAccessPreviewOnlyTest.php index e86f144..1294a72 100644 --- a/tests/Feature/Filament/ConditionalAccessPreviewOnlyTest.php +++ b/tests/Feature/Filament/ConditionalAccessPreviewOnlyTest.php @@ -103,9 +103,12 @@ public function request(string $method, string $path, array $options = []): Grap actorName: 'Tester', ); - expect($run->results)->toHaveCount(1); - expect($run->results[0]['status'])->toBe('skipped'); - expect($run->results[0]['reason'])->toBe('preview_only'); + expect($run->results['items'] ?? [])->toHaveCount(1); + + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($result['status'] ?? null)->toBe('skipped'); + expect($result['reason'] ?? null)->toBe('preview_only'); expect($client->applyCalls)->toBe(0); }); diff --git a/tests/Feature/Filament/EnrollmentRestrictionsPreviewOnlyTest.php b/tests/Feature/Filament/EnrollmentRestrictionsPreviewOnlyTest.php index 4bf6b6c..64703da 100644 --- a/tests/Feature/Filament/EnrollmentRestrictionsPreviewOnlyTest.php +++ b/tests/Feature/Filament/EnrollmentRestrictionsPreviewOnlyTest.php @@ -103,9 +103,12 @@ public function request(string $method, string $path, array $options = []): Grap actorName: 'Tester', ); - expect($run->results)->toHaveCount(1); - expect($run->results[0]['status'])->toBe('skipped'); - expect($run->results[0]['reason'])->toBe('preview_only'); + expect($run->results['items'] ?? [])->toHaveCount(1); + + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($result['status'] ?? null)->toBe('skipped'); + expect($result['reason'] ?? null)->toBe('preview_only'); expect($client->applyCalls)->toBe(0); }); @@ -203,9 +206,12 @@ public function request(string $method, string $path, array $options = []): Grap actorName: 'Tester', ); - expect($run->results)->toHaveCount(1); - expect($run->results[0]['status'])->toBe('skipped'); - expect($run->results[0]['reason'])->toBe('preview_only'); + expect($run->results['items'] ?? [])->toHaveCount(1); + + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($result['status'] ?? null)->toBe('skipped'); + expect($result['reason'] ?? null)->toBe('preview_only'); expect($client->applyCalls)->toBe(0); }); diff --git a/tests/Feature/Filament/GroupPolicyConfigurationRestoreTest.php b/tests/Feature/Filament/GroupPolicyConfigurationRestoreTest.php index 6d12db2..c46565d 100644 --- a/tests/Feature/Filament/GroupPolicyConfigurationRestoreTest.php +++ b/tests/Feature/Filament/GroupPolicyConfigurationRestoreTest.php @@ -164,8 +164,10 @@ public function request(string $method, string $path, array $options = []): Grap )->refresh(); expect($run->status)->toBe('completed'); - expect($run->results[0]['status'])->toBe('applied'); - expect($run->results[0]['definition_value_summary']['success'])->toBe(1); + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($result['status'] ?? null)->toBe('applied'); + expect($result['definition_value_summary']['success'] ?? null)->toBe(1); expect($client->applyPolicyCalls)->toHaveCount(1); expect($client->applyPolicyCalls[0]['policy_type'])->toBe('groupPolicyConfiguration'); diff --git a/tests/Feature/Filament/ODataTypeMismatchTest.php b/tests/Feature/Filament/ODataTypeMismatchTest.php index 1d1d8d5..78dd683 100644 --- a/tests/Feature/Filament/ODataTypeMismatchTest.php +++ b/tests/Feature/Filament/ODataTypeMismatchTest.php @@ -120,5 +120,8 @@ public function getServicePrincipalPermissions(array $options = []): GraphRespon ); expect($run->status)->toBe('failed'); - expect($run->results[0]['reason'])->toContain('mismatch'); + + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->toBeArray(); + expect($result['reason'] ?? null)->toContain('mismatch'); }); diff --git a/tests/Feature/Filament/PolicyCaptureSnapshotOptionsTest.php b/tests/Feature/Filament/PolicyCaptureSnapshotOptionsTest.php index 6b7227c..f7cd45f 100644 --- a/tests/Feature/Filament/PolicyCaptureSnapshotOptionsTest.php +++ b/tests/Feature/Filament/PolicyCaptureSnapshotOptionsTest.php @@ -1,6 +1,7 @@ create(); $tenant->makeCurrent(); $policy = Policy::factory()->for($tenant)->create([ @@ -58,6 +62,18 @@ 'include_scope_tags' => true, ]); + $job = null; + + Queue::assertPushed(CapturePolicySnapshotJob::class, function (CapturePolicySnapshotJob $queuedJob) use (&$job): bool { + $job = $queuedJob; + + return true; + }); + + expect($job)->not->toBeNull(); + + app()->call([$job, 'handle']); + $version = $policy->versions()->first(); expect($version)->not->toBeNull(); diff --git a/tests/Feature/Filament/PolicyVersionRestoreToIntuneTest.php b/tests/Feature/Filament/PolicyVersionRestoreToIntuneTest.php index d32d790..875f1dd 100644 --- a/tests/Feature/Filament/PolicyVersionRestoreToIntuneTest.php +++ b/tests/Feature/Filament/PolicyVersionRestoreToIntuneTest.php @@ -141,9 +141,14 @@ public function getServicePrincipalPermissions(array $options = []): GraphRespon actorName: $user->name, )->refresh(); + $backupItemId = is_array($run->requested_items) ? ($run->requested_items[0] ?? null) : null; + expect($backupItemId)->not->toBeNull(); + $result = $run->results['items'][(int) $backupItemId] ?? null; + expect($run->status)->toBe('completed'); - expect($run->results[0]['status'])->toBe('applied'); - expect($run->results[0]['definition_value_summary']['success'])->toBe(5); + expect($result)->not->toBeNull(); + expect($result['status'] ?? null)->toBe('applied'); + expect($result['definition_value_summary']['success'] ?? null)->toBe(5); $definitionValueCreateCalls = collect($client->requestCalls) ->filter(fn (array $call) => $call['method'] === 'POST' && str_contains($call['path'], '/definitionValues')) diff --git a/tests/Feature/Filament/RestoreExecutionTest.php b/tests/Feature/Filament/RestoreExecutionTest.php index 2fd856c..af6bf77 100644 --- a/tests/Feature/Filament/RestoreExecutionTest.php +++ b/tests/Feature/Filament/RestoreExecutionTest.php @@ -113,7 +113,9 @@ public function getServicePrincipalPermissions(array $options = []): GraphRespon ); expect($run->status)->toBe('completed'); - expect($run->results[0]['status'])->toBe('applied'); + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($result['status'] ?? null)->toBe('applied'); $this->assertDatabaseHas('audit_logs', [ 'action' => 'restore.executed', @@ -201,8 +203,8 @@ public function getServicePrincipalPermissions(array $options = []): GraphRespon ); expect($run->status)->toBe('completed'); - expect($run->results)->toHaveCount(1); - expect($run->results[0]['decision'])->toBe('created'); + expect($run->results['foundations'] ?? [])->toHaveCount(1); + expect(($run->results['foundations'][0]['decision'] ?? null))->toBe('created'); $this->assertDatabaseHas('audit_logs', [ 'action' => 'restore.foundation.created', @@ -301,8 +303,10 @@ public function getServicePrincipalPermissions(array $options = []): GraphRespon ); expect($run->status)->toBe('partial'); - expect($run->results[0]['status'])->toBe('partial'); - expect($run->results[0]['compliance_action_summary']['skipped'] ?? null)->toBe(1); + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($result['status'] ?? null)->toBe('partial'); + expect($result['compliance_action_summary']['skipped'] ?? null)->toBe(1); $this->assertDatabaseHas('audit_logs', [ 'action' => 'restore.compliance.actions.mapped', @@ -517,8 +521,10 @@ public function getServicePrincipalPermissions(array $options = []): GraphRespon expect($graphClient->createCalls)->toBe(1); expect($graphClient->createPayloads[0]['displayName'] ?? null)->toBe('Restored_Autopilot Profile'); expect($run->status)->toBe('completed'); - expect($run->results[0]['status'])->toBe('applied'); - expect($run->results[0]['created_policy_id'])->toBe('autopilot-created'); + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($result['status'] ?? null)->toBe('applied'); + expect($result['created_policy_id'] ?? null)->toBe('autopilot-created'); }); test('restore execution creates missing policy using contracts', function () { @@ -619,6 +625,8 @@ public function getServicePrincipalPermissions(array $options = []): GraphRespon expect($graphClient->createCalls)->toBe(1); expect($graphClient->createPayloads[0]['displayName'] ?? null)->toBe('Restored_Compliance Policy'); expect($run->status)->toBe('completed'); - expect($run->results[0]['status'])->toBe('applied'); - expect($run->results[0]['created_policy_id'])->toBe('compliance-created'); + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($result['status'] ?? null)->toBe('applied'); + expect($result['created_policy_id'] ?? null)->toBe('compliance-created'); }); diff --git a/tests/Feature/Filament/SettingsCatalogRestoreApplySettingsPatchTest.php b/tests/Feature/Filament/SettingsCatalogRestoreApplySettingsPatchTest.php index 68e7dcd..c94859f 100644 --- a/tests/Feature/Filament/SettingsCatalogRestoreApplySettingsPatchTest.php +++ b/tests/Feature/Filament/SettingsCatalogRestoreApplySettingsPatchTest.php @@ -162,23 +162,26 @@ public function request(string $method, string $path, array $options = []): Grap actorName: $user->name, )->refresh(); + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($run->status)->toBe('partial'); - expect($run->results[0]['status'])->toBe('manual_required'); - expect($run->results[0]['settings_apply']['manual_required'])->toBe(1); - expect($run->results[0]['settings_apply']['failed'])->toBe(0); - expect($run->results[0]['settings_apply']['issues'][0]['graph_request_id'])->toBe('req-setting-404'); + expect($result['status'] ?? null)->toBe('manual_required'); + expect($result['settings_apply']['manual_required'] ?? null)->toBe(1); + expect($result['settings_apply']['failed'] ?? null)->toBe(0); + expect($result['settings_apply']['issues'][0]['graph_request_id'] ?? null)->toBe('req-setting-404'); expect($client->applyPolicyCalls[0]['payload'])->not->toHaveKey('settings'); expect($client->requestCalls[0]['method'])->toBe('POST'); expect($client->requestCalls[0]['path'])->toBe('deviceManagement/configurationPolicies/scp-3/settings'); $results = $run->results; - $results[0]['assignment_summary'] = [ + $results['items'][$backupItem->id]['assignment_summary'] = [ 'success' => 0, 'failed' => 1, 'skipped' => 0, ]; - $results[0]['assignment_outcomes'] = [[ + $results['items'][$backupItem->id]['assignment_outcomes'] = [[ 'status' => 'failed', 'group_id' => 'group-1', 'mapped_group_id' => 'group-2', diff --git a/tests/Feature/Filament/SettingsCatalogRestoreTest.php b/tests/Feature/Filament/SettingsCatalogRestoreTest.php index 5b37350..d0e3bc3 100644 --- a/tests/Feature/Filament/SettingsCatalogRestoreTest.php +++ b/tests/Feature/Filament/SettingsCatalogRestoreTest.php @@ -177,13 +177,16 @@ public function request(string $method, string $path, array $options = []): Grap actorName: $user->name, )->refresh(); + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($run->status)->toBe('partial'); - expect($run->results[0]['status'])->toBe('manual_required'); - expect($run->results[0]['settings_apply']['manual_required'])->toBe(1); - expect($run->results[0]['settings_apply']['failed'])->toBe(0); - expect($run->results[0]['settings_apply']['issues'][0]['graph_error_message'])->toContain('settings are read-only'); - expect($run->results[0]['settings_apply']['issues'][0]['graph_request_id'])->toBe('req-123'); - expect($run->results[0]['settings_apply']['issues'][0]['graph_client_request_id'])->toBe('client-abc'); + expect($result['status'] ?? null)->toBe('manual_required'); + expect($result['settings_apply']['manual_required'] ?? null)->toBe(1); + expect($result['settings_apply']['failed'] ?? null)->toBe(0); + expect($result['settings_apply']['issues'][0]['graph_error_message'] ?? null)->toContain('settings are read-only'); + expect($result['settings_apply']['issues'][0]['graph_request_id'] ?? null)->toBe('req-123'); + expect($result['settings_apply']['issues'][0]['graph_client_request_id'] ?? null)->toBe('client-abc'); expect($client->applyPolicyCalls)->toHaveCount(1); expect($client->applyPolicyCalls[0]['payload'])->toHaveKey('name'); @@ -527,12 +530,15 @@ public function request(string $method, string $path, array $options = []): Grap actorName: $user->name, )->refresh(); + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($run->status)->toBe('partial'); - expect($run->results[0]['status'])->toBe('partial'); - expect($run->results[0]['created_policy_id'])->toBe('new-policy-123'); - expect($run->results[0]['created_policy_mode'])->toBe('metadata_only'); - expect($run->results[0]['settings_apply']['created_policy_id'])->toBe('new-policy-123'); - expect($run->results[0]['settings_apply']['created_policy_mode'])->toBe('metadata_only'); + expect($result['status'] ?? null)->toBe('partial'); + expect($result['created_policy_id'] ?? null)->toBe('new-policy-123'); + expect($result['created_policy_mode'] ?? null)->toBe('metadata_only'); + expect($result['settings_apply']['created_policy_id'] ?? null)->toBe('new-policy-123'); + expect($result['settings_apply']['created_policy_mode'] ?? null)->toBe('metadata_only'); expect($client->requestCalls)->toHaveCount(3); expect($client->requestCalls[0]['path'])->toBe('deviceManagement/configurationPolicies/scp-5/settings'); diff --git a/tests/Feature/Filament/WindowsUpdateProfilesRestoreTest.php b/tests/Feature/Filament/WindowsUpdateProfilesRestoreTest.php index 4ef31ec..058c034 100644 --- a/tests/Feature/Filament/WindowsUpdateProfilesRestoreTest.php +++ b/tests/Feature/Filament/WindowsUpdateProfilesRestoreTest.php @@ -117,7 +117,9 @@ public function getServicePrincipalPermissions(array $options = []): GraphRespon ); expect($run->status)->toBe('completed'); - expect($run->results[0]['status'])->toBe('applied'); + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($result['status'] ?? null)->toBe('applied'); expect(PolicyVersion::where('policy_id', $policy->id)->count())->toBe(1); @@ -194,7 +196,9 @@ public function getServicePrincipalPermissions(array $options = []): GraphRespon ); expect($run->status)->toBe('completed'); - expect($run->results[0]['status'])->toBe('applied'); + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($result['status'] ?? null)->toBe('applied'); expect(PolicyVersion::where('policy_id', $policy->id)->count())->toBe(1); @@ -277,7 +281,9 @@ public function getServicePrincipalPermissions(array $options = []): GraphRespon ); expect($run->status)->toBe('completed'); - expect($run->results[0]['status'])->toBe('applied'); + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($result['status'] ?? null)->toBe('applied'); expect(PolicyVersion::where('policy_id', $policy->id)->count())->toBe(1); diff --git a/tests/Feature/Filament/WindowsUpdateRingRestoreTest.php b/tests/Feature/Filament/WindowsUpdateRingRestoreTest.php index 441dc24..a69832f 100644 --- a/tests/Feature/Filament/WindowsUpdateRingRestoreTest.php +++ b/tests/Feature/Filament/WindowsUpdateRingRestoreTest.php @@ -124,7 +124,9 @@ public function getServicePrincipalPermissions(array $options = []): GraphRespon ); expect($run->status)->toBe('completed'); - expect($run->results[0]['status'])->toBe('applied'); + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + expect($result['status'] ?? null)->toBe('applied'); $this->assertDatabaseHas('audit_logs', [ 'action' => 'restore.executed', diff --git a/tests/Feature/PolicyCaptureSnapshotIdempotencyTest.php b/tests/Feature/PolicyCaptureSnapshotIdempotencyTest.php new file mode 100644 index 0000000..2cfb790 --- /dev/null +++ b/tests/Feature/PolicyCaptureSnapshotIdempotencyTest.php @@ -0,0 +1,46 @@ +actingAs($user); + + $tenant->makeCurrent(); + Filament::setTenant($tenant, true); + + $policy = Policy::factory()->for($tenant)->create(); + + Livewire::test(ViewPolicy::class, ['record' => $policy->getRouteKey()]) + ->callAction('capture_snapshot', data: [ + 'include_assignments' => true, + 'include_scope_tags' => true, + ]); + + Livewire::test(ViewPolicy::class, ['record' => $policy->getRouteKey()]) + ->callAction('capture_snapshot', data: [ + 'include_assignments' => true, + 'include_scope_tags' => true, + ]); + + $key = RunIdempotency::buildKey($tenant->getKey(), 'policy.capture_snapshot', $policy->getKey()); + + expect(BulkOperationRun::query() + ->where('tenant_id', $tenant->id) + ->where('idempotency_key', $key) + ->count())->toBe(1); + + Queue::assertPushed(CapturePolicySnapshotJob::class, 1); +}); diff --git a/tests/Feature/PolicyCaptureSnapshotQueuedTest.php b/tests/Feature/PolicyCaptureSnapshotQueuedTest.php new file mode 100644 index 0000000..6b48906 --- /dev/null +++ b/tests/Feature/PolicyCaptureSnapshotQueuedTest.php @@ -0,0 +1,48 @@ +actingAs($user); + + $tenant->makeCurrent(); + Filament::setTenant($tenant, true); + + $policy = Policy::factory()->for($tenant)->create(); + + $this->mock(VersionService::class, function (MockInterface $mock) { + $mock->shouldReceive('captureFromGraph')->never(); + }); + + Livewire::test(ViewPolicy::class, ['record' => $policy->getRouteKey()]) + ->callAction('capture_snapshot', data: [ + 'include_assignments' => true, + 'include_scope_tags' => true, + ]); + + Queue::assertPushed(CapturePolicySnapshotJob::class); + + $run = BulkOperationRun::query() + ->where('tenant_id', $tenant->id) + ->where('resource', 'policies') + ->where('action', 'capture_snapshot') + ->latest('id') + ->first(); + + expect($run)->not->toBeNull(); + expect($run->item_ids)->toBe([(string) $policy->getKey()]); +}); diff --git a/tests/Feature/RestoreAssignmentApplicationTest.php b/tests/Feature/RestoreAssignmentApplicationTest.php index 2d803b0..501db21 100644 --- a/tests/Feature/RestoreAssignmentApplicationTest.php +++ b/tests/Feature/RestoreAssignmentApplicationTest.php @@ -142,7 +142,10 @@ public function request(string $method, string $path, array $options = []): Grap ], ); - $summary = $run->results[0]['assignment_summary'] ?? null; + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + + $summary = $result['assignment_summary'] ?? null; expect($summary)->not->toBeNull(); expect($summary['success'])->toBe(2); @@ -242,12 +245,15 @@ public function request(string $method, string $path, array $options = []): Grap ], ); - $summary = $run->results[0]['assignment_summary'] ?? null; + $result = $run->results['items'][$backupItem->id] ?? null; + expect($result)->not->toBeNull(); + + $summary = $result['assignment_summary'] ?? null; expect($summary)->not->toBeNull(); expect($summary['success'])->toBe(0); expect($summary['failed'])->toBe(2); - expect($run->results[0]['status'])->toBe('partial'); + expect($result['status'] ?? null)->toBe('partial'); }); test('restore maps assignment filter identifiers', function () { diff --git a/tests/Feature/RestoreAuditLoggingTest.php b/tests/Feature/RestoreAuditLoggingTest.php new file mode 100644 index 0000000..5a7ac53 --- /dev/null +++ b/tests/Feature/RestoreAuditLoggingTest.php @@ -0,0 +1,69 @@ + 'tenant-audit', + 'name' => 'Tenant Audit', + 'metadata' => [], + ]); + + $backupSet = BackupSet::create([ + 'tenant_id' => $tenant->id, + 'name' => 'Backup', + 'status' => 'completed', + 'item_count' => 0, + ]); + + $restoreRun = RestoreRun::create([ + 'tenant_id' => $tenant->id, + 'backup_set_id' => $backupSet->id, + 'requested_by' => 'actor@example.com', + 'is_dry_run' => false, + 'status' => RestoreRunStatus::Queued->value, + 'requested_items' => null, + 'preview' => [], + 'results' => null, + 'metadata' => [], + ]); + + $restoreService = $this->mock(RestoreService::class, function (MockInterface $mock) use ($restoreRun) { + $mock->shouldReceive('executeForRun') + ->once() + ->andReturnUsing(function () use ($restoreRun): RestoreRun { + $restoreRun->update([ + 'status' => RestoreRunStatus::Completed->value, + 'completed_at' => now(), + ]); + + return $restoreRun->refresh(); + }); + }); + + $job = new ExecuteRestoreRunJob($restoreRun->id, 'actor@example.com', 'Actor'); + $job->handle($restoreService, app(AuditLogger::class), app(BulkOperationService::class)); + + $audit = AuditLog::query() + ->where('tenant_id', $tenant->id) + ->where('action', 'restore.started') + ->where('metadata->restore_run_id', $restoreRun->id) + ->latest('id') + ->first(); + + expect($audit)->not->toBeNull(); + expect($audit->metadata['backup_set_id'] ?? null)->toBe($backupSet->id); + expect($audit->actor_email)->toBe('actor@example.com'); +}); diff --git a/tests/Feature/RestoreGraphErrorMetadataTest.php b/tests/Feature/RestoreGraphErrorMetadataTest.php index 0222ca2..62724de 100644 --- a/tests/Feature/RestoreGraphErrorMetadataTest.php +++ b/tests/Feature/RestoreGraphErrorMetadataTest.php @@ -95,7 +95,7 @@ public function request(string $method, string $path, array $options = []): Grap expect($client->applyPolicyCalls)->toHaveCount(1); expect($run->status)->toBe('failed'); - $result = $run->results[0] ?? null; + $result = $run->results['items'][$backupItem->id] ?? null; expect($result)->toBeArray(); expect($result['graph_method'] ?? null)->toBe('PATCH'); expect($result['graph_path'] ?? null)->toBe('deviceManagement/endpointSecurityPolicy/esp-1'); diff --git a/tests/Feature/RestoreGroupMappingTest.php b/tests/Feature/RestoreGroupMappingTest.php index 1ddd818..a07e792 100644 --- a/tests/Feature/RestoreGroupMappingTest.php +++ b/tests/Feature/RestoreGroupMappingTest.php @@ -144,12 +144,14 @@ ]], ]); + $targetGroupId = fake()->uuid(); + $this->mock(GroupResolver::class, function (MockInterface $mock) { $mock->shouldReceive('resolveGroupIds') ->andReturnUsing(function (array $groupIds): array { return collect($groupIds) ->mapWithKeys(function (string $id) { - $resolved = $id === 'target-group-1'; + $resolved = $id === $targetGroupId; return [$id => [ 'id' => $id, @@ -178,7 +180,7 @@ 'scope_mode' => 'selected', 'backup_item_ids' => [$backupItem->id], 'group_mapping' => [ - 'source-group-1' => 'target-group-1', + 'source-group-1' => $targetGroupId, ], ]) ->goToNextWizardStep() @@ -192,7 +194,7 @@ expect($run)->not->toBeNull(); expect($run->group_mapping)->toBe([ - 'source-group-1' => 'target-group-1', + 'source-group-1' => $targetGroupId, ]); $this->assertDatabaseHas('audit_logs', [ diff --git a/tests/Feature/RestorePreviewDiffWizardTest.php b/tests/Feature/RestorePreviewDiffWizardTest.php index 90c0caa..ea1a937 100644 --- a/tests/Feature/RestorePreviewDiffWizardTest.php +++ b/tests/Feature/RestorePreviewDiffWizardTest.php @@ -100,6 +100,7 @@ ->goToNextWizardStep() ->goToNextWizardStep() ->goToNextWizardStep() + ->set('data.group_mapping.group-1', 'SKIP') ->callFormComponentAction('preview_diffs', 'run_restore_preview'); $summary = $component->get('data.preview_summary'); @@ -121,6 +122,9 @@ expect($first['scope_tags_changed'] ?? null)->toBeTrue(); expect($first['diff']['summary']['changed'] ?? null)->toBe(1); + $previewRanAt = $summary['generated_at'] ?? now()->toIso8601String(); + $component->set('data.preview_ran_at', $previewRanAt); + $component ->goToNextWizardStep() ->call('create') diff --git a/tests/Feature/RestoreRiskChecksWizardTest.php b/tests/Feature/RestoreRiskChecksWizardTest.php index 32a88f0..f545b5d 100644 --- a/tests/Feature/RestoreRiskChecksWizardTest.php +++ b/tests/Feature/RestoreRiskChecksWizardTest.php @@ -120,6 +120,10 @@ $component ->goToNextWizardStep() + ->set('data.group_mapping.source-group-1', 'SKIP') + ->set('data.check_summary', $summary) + ->set('data.check_results', $results) + ->set('data.checks_ran_at', $checksRanAt) ->callFormComponentAction('preview_diffs', 'run_restore_preview') ->goToNextWizardStep() ->call('create') diff --git a/tests/Feature/RestoreRunIdempotencyTest.php b/tests/Feature/RestoreRunIdempotencyTest.php new file mode 100644 index 0000000..b1d68eb --- /dev/null +++ b/tests/Feature/RestoreRunIdempotencyTest.php @@ -0,0 +1,102 @@ + 'tenant-idempotency', + 'name' => 'Tenant Idempotency', + 'metadata' => [], + ]); + + $tenant->makeCurrent(); + + $policy = Policy::create([ + 'tenant_id' => $tenant->id, + 'external_id' => 'policy-1', + 'policy_type' => 'unknownPreviewOnlyType', + 'display_name' => 'Preview-only policy', + 'platform' => 'windows', + ]); + + $backupSet = BackupSet::create([ + 'tenant_id' => $tenant->id, + 'name' => 'Backup', + 'status' => 'completed', + 'item_count' => 1, + ]); + + $backupItem = BackupItem::create([ + 'tenant_id' => $tenant->id, + 'backup_set_id' => $backupSet->id, + 'policy_id' => $policy->id, + 'policy_identifier' => $policy->external_id, + 'policy_type' => $policy->policy_type, + 'platform' => $policy->platform, + 'payload' => ['id' => $policy->external_id], + 'metadata' => [ + 'displayName' => 'Backup Policy', + ], + ]); + + $user = User::factory()->create([ + 'email' => 'executor@example.com', + 'name' => 'Executor', + ]); + + $this->actingAs($user); + $user->tenants()->syncWithoutDetaching([ + $tenant->getKey() => ['role' => 'owner'], + ]); + + $data = [ + 'backup_set_id' => $backupSet->id, + 'scope_mode' => 'selected', + 'backup_item_ids' => [$backupItem->id], + 'group_mapping' => [], + 'is_dry_run' => false, + 'check_summary' => [ + 'blocking' => 0, + 'warning' => 0, + 'safe' => 1, + 'has_blockers' => false, + ], + 'check_results' => [], + 'checks_ran_at' => now()->toIso8601String(), + 'preview_ran_at' => now()->toIso8601String(), + 'acknowledged_impact' => true, + 'tenant_confirm' => 'Tenant Idempotency', + ]; + + $first = RestoreRunResource::createRestoreRun($data); + $second = RestoreRunResource::createRestoreRun($data); + + expect($first->id)->toBe($second->id); + expect(RestoreRun::count())->toBe(1); + + $run = RestoreRun::query()->first(); + + expect($run)->not->toBeNull(); + expect($run->status)->toBe(RestoreRunStatus::Queued->value); + + Bus::assertDispatchedTimes(ExecuteRestoreRunJob::class, 1); +}); diff --git a/tests/Feature/RestoreUnknownPolicyTypeSafetyTest.php b/tests/Feature/RestoreUnknownPolicyTypeSafetyTest.php index f66f7c6..43f6742 100644 --- a/tests/Feature/RestoreUnknownPolicyTypeSafetyTest.php +++ b/tests/Feature/RestoreUnknownPolicyTypeSafetyTest.php @@ -110,7 +110,7 @@ public function request(string $method, string $path, array $options = []): Grap expect($client->applyPolicyCalls)->toHaveCount(0); - $result = $run->results[0] ?? null; + $result = $run->results['items'][$backupItem->id] ?? null; expect($result)->toBeArray(); expect($result['status'] ?? null)->toBe('skipped'); expect($result['restore_mode'] ?? null)->toBe('preview-only'); diff --git a/tests/Feature/RunAuthorizationTenantIsolationTest.php b/tests/Feature/RunAuthorizationTenantIsolationTest.php new file mode 100644 index 0000000..8553667 --- /dev/null +++ b/tests/Feature/RunAuthorizationTenantIsolationTest.php @@ -0,0 +1,58 @@ +create(); + $tenantB = Tenant::factory()->create(); + + BulkOperationRun::factory()->create([ + 'tenant_id' => $tenantA->getKey(), + 'resource' => 'tenant_a', + 'action' => 'alpha', + ]); + + BulkOperationRun::factory()->create([ + 'tenant_id' => $tenantB->getKey(), + 'resource' => 'tenant_b', + 'action' => 'beta', + ]); + + $user = User::factory()->create(); + $user->tenants()->syncWithoutDetaching([ + $tenantA->getKey() => ['role' => 'owner'], + $tenantB->getKey() => ['role' => 'owner'], + ]); + + $this->actingAs($user) + ->get(BulkOperationRunResource::getUrl('index', tenant: $tenantA)) + ->assertOk() + ->assertSee('tenant_a') + ->assertDontSee('tenant_b'); +}); + +test('bulk operation run view is forbidden cross-tenant (403)', function () { + $tenantA = Tenant::factory()->create(); + $tenantB = Tenant::factory()->create(); + + $runB = BulkOperationRun::factory()->create([ + 'tenant_id' => $tenantB->getKey(), + 'resource' => 'tenant_b', + 'action' => 'beta', + ]); + + $user = User::factory()->create(); + $user->tenants()->syncWithoutDetaching([ + $tenantA->getKey() => ['role' => 'owner'], + $tenantB->getKey() => ['role' => 'owner'], + ]); + + $this->actingAs($user) + ->get(BulkOperationRunResource::getUrl('view', ['record' => $runB], tenant: $tenantA)) + ->assertForbidden(); +}); diff --git a/tests/Feature/RunStartAuthorizationTest.php b/tests/Feature/RunStartAuthorizationTest.php new file mode 100644 index 0000000..ac67c6c --- /dev/null +++ b/tests/Feature/RunStartAuthorizationTest.php @@ -0,0 +1,34 @@ +create(); + + $this->actingAs($user); + Filament::setTenant($tenantA, true); + + $sync = app(InventorySyncService::class); + $allTypes = $sync->defaultSelectionPayload()['policy_types']; + + Livewire::test(InventoryLanding::class) + ->callAction('run_inventory_sync', data: ['tenant_id' => $tenantB->getKey(), 'policy_types' => $allTypes]) + ->assertStatus(403); + + Queue::assertNothingPushed(); + + expect(InventorySyncRun::query()->where('tenant_id', $tenantB->id)->exists())->toBeFalse(); + expect(BulkOperationRun::query()->where('tenant_id', $tenantB->id)->exists())->toBeFalse(); +}); diff --git a/tests/Unit/RunIdempotencyTest.php b/tests/Unit/RunIdempotencyTest.php new file mode 100644 index 0000000..bd189e0 --- /dev/null +++ b/tests/Unit/RunIdempotencyTest.php @@ -0,0 +1,57 @@ + 2, 'a' => 1]); + $keyA2 = RunIdempotency::buildKey(1, 'policy.capture_snapshot', 'abc', ['a' => 1, 'b' => 2]); + $keyB = RunIdempotency::buildKey(1, 'policy.capture_snapshot', 'def', ['a' => 1, 'b' => 2]); + + expect($keyA1)->toBe($keyA2) + ->and($keyA1)->not->toBe($keyB) + ->and($keyA1)->toMatch('/^[a-f0-9]{64}$/'); +}); + +it('finds only active bulk operation runs by idempotency key', function () { + $pending = BulkOperationRun::factory()->create([ + 'idempotency_key' => RunIdempotency::buildKey(1, 'bulk.policy.capture_snapshot', 'abc'), + 'status' => 'pending', + ]); + + $completed = BulkOperationRun::factory()->create([ + 'tenant_id' => $pending->tenant_id, + 'user_id' => $pending->user_id, + 'idempotency_key' => $pending->idempotency_key, + 'status' => 'completed', + ]); + + expect(RunIdempotency::findActiveBulkOperationRun($pending->tenant_id, $pending->idempotency_key)) + ->not->toBeNull() + ->id->toBe($pending->id); + + expect(RunIdempotency::findActiveBulkOperationRun($pending->tenant_id, $completed->idempotency_key)) + ->id->toBe($pending->id); +}); + +it('finds only active restore runs by idempotency key', function () { + $active = RestoreRun::factory()->create([ + 'idempotency_key' => RunIdempotency::buildKey(1, 'restore.execute', 123), + 'status' => 'queued', + ]); + + RestoreRun::factory()->create([ + 'tenant_id' => $active->tenant_id, + 'backup_set_id' => $active->backup_set_id, + 'idempotency_key' => $active->idempotency_key, + 'status' => 'completed', + ]); + + expect(RunIdempotency::findActiveRestoreRun($active->tenant_id, $active->idempotency_key)) + ->not->toBeNull() + ->id->toBe($active->id); +}); -- 2.45.2