From a4d3bb7a579597da53fd1dac54dbd5f6644d4c93 Mon Sep 17 00:00:00 2001 From: Patrick Plate Date: Wed, 24 Jun 2026 15:08:47 +0200 Subject: [PATCH] =?UTF-8?q?review:=20Sprint=200=20plan=20review=20?= =?UTF-8?q?=E2=80=94=20APPROVED?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Home.md | 1 + Sprint-0-Plan-Review.md | 217 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 218 insertions(+) create mode 100644 Sprint-0-Plan-Review.md diff --git a/Home.md b/Home.md index e647b53..09f0cd7 100644 --- a/Home.md +++ b/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 diff --git a/Sprint-0-Plan-Review.md b/Sprint-0-Plan-Review.md new file mode 100644 index 0000000..a71a86a --- /dev/null +++ b/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 `Test` / `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 = `) 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).**