review: Sprint 0 plan review — APPROVED

Home.md

@@ -41,6 +41,7 @@ plate-auth is the carve-out of [InspectFlow](https://git.plate-software.de/pplat
 | [Sprint-0-Assessment](Sprint-0-Assessment) | State of the InspectFlow auth code, what is reusable, risks, recommendation |
 | [Sprint-0-Plan](Sprint-0-Plan) | File-by-file extraction plan, package renames, Flyway consolidation, publishing pipeline |
 | [Sprint-0-Testplan](Sprint-0-Testplan) | Unit / integration / contract test coverage matrix |
+| [Sprint-0-Plan-Review](Sprint-0-Plan-Review) | ✅ Plan Review — APPROVED (2 warnings, panel confidence 82%) |
 
 ### Consumer guides
 

Sprint-0-Plan-Review.md

@@ -0,0 +1,217 @@
+# Plan Review: plate-auth Sprint 0 — Shared Auth Library Extraction
+
+**Date:** 2026-06-24
+**Module:** plate-auth
+**Reviewer:** Roo (Plan Reviewer)
+**Documents:** Sprint-0-Assessment.md v1, Sprint-0-Plan.md v1, Sprint-0-Testplan.md v1
+**Verdict:** ✅ APPROVED (with 2 warnings to resolve before code starts)
+
+---
+
+## Summary
+
+Sprint 0 plans the extraction of InspectFlow's Sprint 14.1–14.6 auth system into a standalone, versioned library (`de.platesoft:plate-auth-starter:0.1.0` + `@platesoft/auth@0.1.0`). The planning is thorough, well-structured, and architecturally sound. All referenced InspectFlow source paths were verified to exist. The 5 SPI seams are correctly factored, the Flyway strategy is viable, and the testplan provides 43 test cases covering all 8 acceptance criteria. Two internal document inconsistencies must be clarified before code begins, but neither requires re-architecture.
+
+---
+
+## Reviewed Documents
+
+| Document | Version | Rating |
+|----------|---------|--------|
+| Sprint-0-Assessment.md | v1 | ✅ |
+| Sprint-0-Plan.md | v1 | ✅ (2 warnings) |
+| Sprint-0-Testplan.md | v1 | ✅ (1 warning) |
+| Architecture.md | v1 | ⚠️ Minor inconsistency |
+| Vision.md | v1 | ✅ |
+| Roadmap.md | v1 | ⚠️ Minor inconsistency |
+| Open-Questions.md | v1 | ✅ |
+| Integration-Guide.md | v1 | ⚠️ See finding W-1 |
+| Migration-InspectFlow.md | v1 | ✅ |
+
+---
+
+## Checklist
+
+### Assessment
+
+| # | Check | Result | Notes |
+|---|-------|--------|-------|
+| 1 | Problem statement complete | ✅ | Clear forcing function (Sparkboard). Matches Vision.md. Well-motivated "option 2" over copy-paste. |
+| 2 | Affected components identified | ✅ | Class-by-class inventory with lines, coupling analysis, status classification (65% lift / 25% reshape / 10% leave). |
+| 3 | Current state accurate | ✅ | All 23 referenced InspectFlow Java classes verified to exist at stated paths. All 6 Flyway migrations (V26–V31) verified. Frontend files (`auth-config.ts`, `exchange.ts`, `auth.ts`) verified. |
+| 4 | Risk assessment realistic | ✅ | R1–R12 with proper likelihood/impact/mitigation. R5 (in-memory nonce store) correctly rated "Certain" with v0.3 mitigation. R1 (Flyway corruption) mitigated by separate history table. |
+| 5 | Solution options evaluated | ✅ | Section 6 gives clear GO recommendation with 6 explicit constraints. Open-Questions Q01–Q12 properly deferred to Ask Phase. |
+
+### Implementation Plan
+
+| # | Check | Result | Notes |
+|---|-------|--------|-------|
+| 6 | All requirements covered | ✅ | Vision success criteria (§ "What success looks like") → workstreams W1–W7 → acceptance A1–A8. Every Vision goal traceable. |
+| 7 | Correct patterns referenced | ✅ | Spring Boot 4 auto-config (`@AutoConfiguration`, `@ConditionalOnMissingBean`, `@ConfigurationProperties`), SPI with default no-ops, Flyway multi-location, NextAuth v5 `auth()` + Edge proxy. All correct for the stated tech stack. |
+| 8 | File paths correct | ✅ | All InspectFlow source paths verified via filesystem. Package rename `de.platesoft.inspectflow.*` → `de.platesoft.auth.*` is valid (no naming collision with existing packages). |
+| 9 | Implementation order logical | ✅ | W1 (scaffolding) → W2/W3 (parallel extraction) → W4 (SPI, depends on W2) → W5 (Flyway, depends on W2) → W6 (publish, depends on all) → W7 (integration, depends on all). Correct dependency graph. |
+| 10 | No gaps in steps | ⚠️ | See finding **W-1** (OrgValidator default inconsistency) and **W-2** (migration count V5 vs V6). Both are document alignment issues, not missing steps. |
+| 11 | Flyway migrations planned | ✅ | Separate `flyway_schema_history_auth` table. V26→V1, V27→V2, V28→V3, V29→V4, V31→V5. V30 stays in InspectFlow (correctly identified as T3). Second Flyway bean runs before JPA init. |
+| 12 | Error handling planned | ✅ | `@NotBlank @Size(min=32)` on secrets → fail-fast at boot. SPI fail-fast message for missing `OrgValidator`. Generic "invalid credentials" on login failure. No stack traces in 401/403. |
+| 13 | No scope creep | ✅ | Ground rule §1.3: "No new features. This sprint is an extraction." Deferred items explicitly listed in §11. HMAC wire-version constant (from Q06) is the only net-new protocol addition — justified for lockstep safety. |
+
+### Test Plan
+
+| # | Check | Result | Notes |
+|---|-------|--------|-------|
+| 14 | Coverage complete | ✅ | Every workstream has mapped tests. A1–A8 matrix in §9 shows full coverage. 43 test cases across 6 types. |
+| 15 | Test types appropriate | ✅ | Unit (15) for service logic, Integration (9) for DB+auto-config, Frontend-Unit (5) for npm package, E2E (6) as InspectFlow regression, Security (10) for auth-specific threats, Performance (3) for baseline. |
+| 16 | Edge cases covered | ✅ | Nonce replay (T-UT06), HMAC tamper (T-UT07), clock skew (T-UT08), short secret (T-SEC06), refresh rotation (T-SEC10), SQL injection probe (T-SEC08), constant-time compare (T-SEC09). |
+| 17 | Test class naming correct | ✅ | Follows `<Feature>Test` / `<Feature>IT` convention. Method naming: `method_scenario_expected()`. |
+| 18 | Test method naming correct | ✅ | Consistent `verbNoun_scenario_expectation()` pattern throughout. |
+| 19 | Test data defined | ✅ | §10 defines users, orgs, secrets, time control (injected `Clock`), recording mailers. Fixtures in stated paths. |
+| 20 | SSH tests identified | N/A | Not a PAISY project. No SSH testing applicable. E2E via InspectFlow Playwright suite serves as the live integration environment. |
+
+---
+
+## Expert Panel Review
+
+### 🏛️ Domain Expert — "Does extracting T1+T2 from InspectFlow leave InspectFlow's auth contract intact?"
+
+**Confidence: 85%**
+
+**Assessment:** Yes. The extraction is non-lossy:
+
+- All InspectFlow auth entities (`User`, `UserIdentity`, `Membership`, `Invitation`, `AccessRequest`, `LoginEvent`) move to plate-auth with identical column names and JPA `@Table` annotations → Hibernate maps to the same physical tables.
+- T3 entities (`Company`, `OnboardingService`, `TenantAutoMapService`) stay in InspectFlow, connected via the 5 SPIs.
+- The `OrgValidator` SPI replaces the `companyRepository.findById(...)` call — InspectFlow implements it wrapping `CompanyRepository.existsById(...)`. No behavioral change.
+- `OnboardingHook` SPI replaces the direct `OnboardingService` call in `OAuthService` → InspectFlow implements it by delegating to its existing `OnboardingService`.
+- The polymorphic FK design (`memberships.org_type = 'COMPANY'`, `memberships.org_id = <company_uuid>`) was already designed into Sprint 14.2 — this is not a new pattern being introduced.
+
+**Risk noted:** The existing `fn_membership_org_fk()` trigger (from V27) is dropped from plate-auth's migration. InspectFlow must retain this trigger in their own migration set or accept SPI-only validation. Migration-InspectFlow.md documents this correctly (§5.3 Step W5-3).
+
+### 🔧 Architecture Expert — "Are the 5 SPI seams properly factored? Is the wire-contract HMAC envelope sound? Is the polymorphic FK stable?"
+
+**Confidence: 80%**
+
+**SPI Assessment:**
+
+| SPI | Factoring | Verdict |
+|-----|-----------|---------|
+| `OrgValidator` | Single method `exists(OrgType, UUID)` — minimal surface, clear contract | ✅ Correct |
+| `OrgDisplayNameResolver` | Single method `displayName(OrgType, UUID)` — pure presentation | ✅ Correct |
+| `InvitationMailer` | Single method `sendInvitation(...)` — side-effect isolated | ✅ Correct |
+| `AccessRequestMailer` | Two methods: `notifyAdmins` + `notifyRequester` — clear lifecycle events | ✅ Correct |
+| `OnboardingHook` | `onFirstSignIn` + optional `onSubsequentSignIn` — T3 escape hatch | ✅ Correct |
+
+No SPI is over-broad (too many methods forcing implementation burden) or under-specified (requiring consumers to guess behavior). The `@ConditionalOnMissingBean` pattern is idiomatic Spring Boot 4.
+
+**HMAC Envelope Assessment:** Sound. SHA-256 with ≥32-char key, constant-time compare (`MessageDigest.isEqual`), 60s freshness window, UUID nonce with 5-min dedup TTL. The wire-version constant (Q06 leaning) adds forward compatibility for envelope format changes. In-memory nonce store is the known single-replica limitation — documented and deferred to v0.3.
+
+**Polymorphic FK Assessment:** Stable. The `(org_type, org_id)` pattern avoids a generic `organizations` table that would force cross-domain sync. Runtime validation via `OrgValidator` SPI is the right trade-off for a multi-consumer library. Optional DB trigger (consumer-defined) provides defense-in-depth for teams that want it.
+
+**Concern:** See finding W-1 regarding the `OrgValidator` default behavior inconsistency.
+
+### 🛡️ Risk/Compliance Expert — "What's the impact if InspectFlow can't migrate cleanly? Versioning + lockstep risk? Security gaps?"
+
+**Confidence: 82%**
+
+**Migration Risk:** Low. The rollback strategy (§10.4) is sound — revert `pom.xml` + redeploy old commit. Database schema is unchanged (same table names, same columns). The `flyway_schema_history_auth` baseline insertion for existing InspectFlow DBs is clearly documented in Migration-InspectFlow.md.
+
+**Lockstep Risk:** Mitigated by wire-version constant + npm peer deps + single-repo single-tag release. The risk of version drift is real but the chosen mitigation (fail-fast on wire-version mismatch) is adequate for a 2-consumer internal library.
+
+**Security Gaps:**
+- The security review checklist (§9, 10 sections, 30+ sub-items) is comprehensive.
+- No PII in JWT beyond email/userId/role — correct.
+- Refresh-token rotation is partially implemented (v0.1 keeps InspectFlow behavior) — documented for v0.3 hardening.
+- The `PermissiveOrgValidator` default (if it exists — see W-1) could be a security gap if a consumer forgets to implement `OrgValidator` and accidentally grants memberships to non-existent orgs. This is mitigated by the "WARN once on startup" behavior described in T-UT15, but should be elevated to a startup log at ERROR level.
+
+**Panel Confidence: 82%** (above 70% threshold → does not trigger automatic REVISE)
+
+---
+
+## Findings
+
+### ⚠️ W-1 — OrgValidator default behavior inconsistency (non-blocking, must resolve before W4)
+
+**Documents affected:** Sprint-0-Plan.md §4.5 W4-2, Integration-Guide.md §3.3, Sprint-0-Testplan.md T-UT15
+
+**Issue:** Three documents disagree on whether a default `OrgValidator` exists:
+
+| Source | States |
+|--------|--------|
+| Plan §4.5 W4-2 | "DefaultOrgValidator — **does NOT exist as a default**; auto-config requires the consumer to provide one if T2 endpoints are used." |
+| Integration-Guide §3.3 | "OrgValidator (default = `PermissiveOrgValidator`) — Accepts any `(org_type, org_id)` — replace via SPI" |
+| Testplan T-UT15 | "No user-provided `OrgValidator` bean; default `PermissiveOrgValidator` in effect" |
+
+**Impact:** If a consumer deploys without providing an `OrgValidator`, either (a) T2 endpoints fail-fast with a clear error (Plan's intent), or (b) any `(org_type, org_id)` is silently accepted (Guide/Testplan's intent). Option (b) is a potential security gap — memberships could be granted to non-existent orgs.
+
+**Recommendation:** Resolve in Ask Phase. Suggested approach: ship `PermissiveOrgValidator` as default (for rapid prototyping) but log a **WARN on every call** stating "No OrgValidator configured — accepting all org references without validation. Provide a bean to enforce org existence checks." This balances DX for greenfield (Sparkboard can boot immediately) with visibility for production (logs scream until you implement it). Document the choice in Architecture.md.
+
+---
+
+### ⚠️ W-2 — Migration count inconsistency: V5 vs V6 (non-blocking, must align before W5)
+
+**Documents affected:** Sprint-0-Plan.md §7.2, Architecture.md §8.1, Roadmap.md §v0.1
+
+**Issue:**
+
+| Source | Migration count |
+|--------|-----------------|
+| Plan §7.2 | 5 migrations (V1–V5): V26→V1, V27→V2, V28→V3, V29→V4, V31→V5 |
+| Architecture §8.1 | 6 files listed: V1, V2, V3, V4, **V5__add_microsoft_tenant_id_index.sql**, V6__create_login_events |
+| Roadmap §v0.1 | "Flyway migrations under `classpath:db/migration/auth/V1..V6`" |
+| Testplan T-IT01 | "5 rows (V1..V5)" |
+
+Architecture's V5 (`add_microsoft_tenant_id_index`) appears to be an index on `user_identities.tenant_id` (T1 data), which is distinct from V30 (`companies.microsoft_tenant_id`, T3 data). If this is correct, the migration count should be **6** (V1–V6), and the Plan/Testplan need updating from "V5" to "V6".
+
+**Recommendation:** Clarify: is there a separate index migration for `user_identities.tenant_id`? If yes → update Plan §7.2 and Testplan T-IT01 to say "V1..V6" and add V5 to the W5 step list. If no → update Architecture §8.1 to remove V5 and renumber V6→V5.
+
+---
+
+## Traceability Matrix
+
+| Vision Success Criterion | Plan Workstream | Acceptance | Test IDs |
+|--------------------------|----------------|------------|----------|
+| New Spring Boot app + plate-auth = Google SSO in <30min | W1, W2, W4, W6 | A4 | T-IT02, T-IT03, T-E2E02 |
+| New Next.js app + @platesoft/auth = sign-in in <30min | W3, W6 | A4 | T-FE01, T-FE05 |
+| InspectFlow bit-for-bit reproducible from library | W2, W4, W5, W7 | A3, A7 | T-E2E01–06, T-IT01 |
+| Sparkboard adopts on day 1 without seeing InspectFlow code | W1, W3, W6 | A4 | Integration-Guide validation |
+| SPI seams are clean | W4 | A5 | T-UT11, T-UT15, T-IT09 |
+| Exchange flow works artifact-to-artifact | W2, W3 | A2 | T-UT04–09, T-IT03, T-FE02, T-SEC01–03 |
+| Config namespace `plate.auth.*` | W2-B | A4 | T-UT14, T-SEC05, T-SEC06 |
+| Frontend artifact bundles + ships ESM | W3-A | A6 | T-FE01–05 |
+| No performance regression | All | A8 | T-PERF01–03 |
+
+**Coverage gaps:** None. Every Vision goal traces through at least one workstream, one acceptance criterion, and at least two test cases.
+
+---
+
+## Verdict
+
+### ✅ APPROVED
+
+The Sprint 0 planning suite (Assessment + Plan + Testplan + supporting docs) is **complete, correct, and feasible**. The extraction approach (rename + repackage + 5 SPIs) is architecturally sound. The 43-test coverage matrix maps cleanly to all 8 acceptance criteria. The Flyway strategy (separate history table) avoids collision with existing consumer migrations. The security checklist (§9, 30+ items) exceeds the standard for a library wrapping authentication.
+
+**Conditions for GO (must resolve before code starts):**
+
+1. **W-1:** Decide the `OrgValidator` default behavior (PermissiveOrgValidator vs fail-fast) and align all 3 documents. Add to Open-Questions as Q13 or resolve in Ask Phase.
+2. **W-2:** Confirm migration count (5 or 6) and align Plan §7.2, Architecture §8.1, Roadmap, and Testplan T-IT01.
+
+Neither condition requires re-architecture or re-planning. Both are document alignment fixes resolvable in <1 hour during the Ask Phase.
+
+**Panel Confidence:** 82% (Domain: 85%, Architecture: 80%, Risk: 82%)
+
+---
+
+## Cross-references
+
+- [Home](Home)
+- [Vision](Vision)
+- [Architecture](Architecture)
+- [Roadmap](Roadmap)
+- [Sprint-0-Assessment](Sprint-0-Assessment)
+- [Sprint-0-Plan](Sprint-0-Plan)
+- [Sprint-0-Testplan](Sprint-0-Testplan)
+- [Open-Questions](Open-Questions)
+- [Integration-Guide](Integration-Guide)
+- [Migration-InspectFlow](Migration-InspectFlow)
+
+---
+
+**End of Sprint-0-Plan-Review.md (v1).**