cannamanage/docs/sprint-3/cannamanage-sprint3-plan-review.md

# Plan Review: CannaManage Sprint 3

**Date:** 2026-06-11  
**Reviewer:** Roo (Plan Reviewer)  
**Documents:** Sprint 3 Plan v2 (APPROVED by Planner)  
**Verdict:** ✅ APPROVED

---

## Summary

The Sprint 3 plan is comprehensive, technically sound, and well-ordered. All deferred Sprint 2 features (staff, portal, reports, prevention officer, club settings) are covered. Architecture decisions are consistent with the existing codebase. The plan correctly builds on Sprint 2's shared-schema + TenantFilterAspect pattern rather than the architecture doc's theoretical schema-per-tenant model. 7 non-blocking findings identified — all are suggestions for improvement, none require plan revision.

---

## Review Checklist

### 1. Does the plan cover ALL deferred Sprint 2 items?

| Deferred Item | Plan Coverage | Status |
|---------------|--------------|--------|
| STAFF role | Phase 1 (StaffPermission enum, StaffAccount entity, SpEL checker) | ✅ |
| Member portal | Phase 5 (session-based SecurityFilterChain, PortalController) | ✅ |
| Reports | Phase 4 (ReportController, iText 7 PDF, Commons CSV) | ✅ |
| Prevention Officer | Phase 6 (configurable limit, assignment endpoint, under-21 gate) | ✅ |
| Club settings | Phase 2 (ClubController GET/PUT /clubs/me, stats) | ✅ |
| Integration tests | Phase 7 (Testcontainers PostgreSQL, 6 test classes) | ✅ |

**Result: ✅ PASS** — All deferred items fully addressed.

---

### 2. Architecture consistency with Sprint 2 patterns

| Pattern | Sprint 2 Implementation | Sprint 3 Plan | Consistent? |
|---------|------------------------|---------------|-------------|
| Tenant isolation | `TenantFilterAspect` (shared-schema, Hibernate @Filter) | Continues shared-schema; `staff_accounts` has `tenant_id` | ✅ |
| JWT auth | `JwtService` + `JwtAuthFilter` (stateless, JJWT) | Extends with `jti` + `permissions` claims | ✅ |
| SecurityConfig | `@Order(1)` API + `@Order(2)` Public | Inserts Portal `@Order(2)`, shifts Public to `@Order(3)` | ✅ |
| Role model | `UserRole` enum: `ROLE_ADMIN`, `ROLE_STAFF`, `ROLE_MEMBER` | Uses same enum; STAFF already present in code | ✅ |
| Entity base | `AbstractTenantEntity` with `tenant_id` | New entities (StaffAccount, InviteToken) follow same pattern | ✅ |
| Repositories | Spring Data JPA in `cannamanage-service/.../repository/` | New repos placed in same package | ✅ |

**Result: ✅ PASS** — Fully consistent with established patterns.

---

### 3. Staff permission model well-defined?

| Aspect | Assessment |
|--------|-----------|
| Enum values | 8 permissions covering all current features + 1 future (`MANAGE_GROW_CALENDAR`) | ✅ |
| Storage | JSONB on `staff_accounts` — correct for PostgreSQL, supports flexible permission sets | ✅ |
| Enforcement | SpEL `@PreAuthorize` + custom `StaffPermissionChecker` bean | ✅ |
| JWT embedding | Permissions in JWT for stateless checks; blacklist fallback for revoked tokens | ✅ |
| ADMIN bypass | `StaffPermissionChecker.has()` returns `true` for ADMIN role first | ✅ |
| Templates | 3 role templates (Ausgabe, Lager, Vorstand) — matches architecture doc | ✅ |

**Result: ✅ PASS** — Well-designed, DSGVO-compliant least-privilege model.

---

### 4. Member portal auth design clean?

| Aspect | Assessment |
|--------|-----------|
| Dual SecurityFilterChain | `@Order(2)` for `/portal/**` — correct isolation from API chain | ✅ |
| Session-based | `SessionCreationPolicy.IF_REQUIRED` + 30min timeout | ✅ |
| CSRF | Enabled via `CookieCsrfTokenRepository.withHttpOnlyFalse()` — React SPA can read cookie | ✅ |
| Read-only | All portal endpoints are GET — minimal attack surface | ✅ |
| Data isolation | Member can only see own data (enforced by `memberId` from session principal) | ✅ |
| `SameSite=Strict` | Correct for CSRF prevention | ✅ |

**Result: ✅ PASS** — Clean separation of concerns.

---

### 5. Dependencies pinned to specific versions?

| Library | Plan Version | Pinned? |
|---------|-------------|---------|
| iText 7 | `8.0.x` | ⚠️ Range |
| Apache Commons CSV | `1.11+` | ⚠️ Range |
| Caffeine | `3.1+` | ⚠️ Range |
| Spring Boot Starter Mail | Boot-managed | ✅ (BOM) |
| Testcontainers | `1.19+` | ⚠️ Range |

**Result: ⚠️ WARNING (non-blocking)** — Versions use ranges instead of exact pins. This is acceptable for a plan document (exact versions determined at implementation time by checking latest stable), but the implementor should pin exact versions in the POM.

---

### 6. Flyway V3 migration complete?

| Schema Change | Covered in V3? |
|---------------|---------------|
| `staff_accounts` table | ✅ Full DDL with JSONB, indexes, unique constraint |
| `revoked_tokens` table | ✅ With jti index + expires index |
| `invite_tokens` table | ✅ With token index, FK to users |
| `users.prevention_officer` column | ✅ `ALTER TABLE ADD COLUMN` |
| Club extension columns (9 columns) | ✅ All `ALTER TABLE ADD COLUMN IF NOT EXISTS` |

**Result: ✅ PASS** — All new tables and columns defined in a single idempotent migration.

---

### 7. Test plan comprehensive?

| Phase | Unit Tests | Integration Tests | Total |
|-------|-----------|-------------------|-------|
| P1 (Staff + Revocation) | T-01 to T-06 | — | 6 |
| P2 (Club) | T-07 to T-10 | — | 4 |
| P3 (Staff CRUD + Invite) | T-11 to T-14 | — | 4 |
| P4 (Reports) | T-15 to T-18 | — | 4 |
| P5 (Portal) | — | T-19, T-20 | 2 |
| P6 (Prevention Officer) | T-21, T-22 | — | 2 |
| P7 (Integration) | — | T-23 to T-26 | 4 |
| **Total** | **18** | **8** | **26** |

Coverage check:
- Every phase has at least one test: ✅
- Edge cases (expired token, invalid regex, permission denial): ✅
- E2E flows (invite → set-password → login → permission check): ✅
- Tenant isolation: ✅
- Token revocation lifecycle: ✅

**Result: ✅ PASS** — Comprehensive coverage matching all implementation phases.

---

### 8. Gaps between API spec and implementation plan?

| API Spec Endpoint | Sprint 3 Plan | Gap? |
|-------------------|---------------|------|
| `GET /clubs/me` (§6.1) | Phase 2 — `ClubController` | ✅ Covered |
| `PUT /clubs/me` (§6.2) | Phase 2 — `ClubController` | ✅ Covered |
| `GET /clubs/me/stats` (§6.3) | Phase 2 — `ClubStatsResponse` DTO | ✅ Covered |
| `POST /auth/logout` (§5.3) | Not explicitly addressed | ⚠️ See finding #3 |
| `GET /reports/monthly` (§10) | Phase 4 — `ReportController` | ✅ Covered |
| `GET /reports/members` (§10) | Phase 4 — member list export | ✅ Covered |
| `GET /reports/recall/{batchId}` (§10) | Phase 4 — recall report | ✅ Covered |
| Staff endpoints (not in spec) | Phase 3 — new endpoints | ℹ️ Plan extends spec |
| Portal endpoints (not in spec) | Phase 5 — new `/portal/**` | ℹ️ Plan extends spec |
| `POST /auth/set-password` (not in spec) | Phase 3 step 3.9 | ℹ️ Plan extends spec |

**Result: ✅ PASS** — One minor gap (logout integration), otherwise plan both implements spec and correctly extends it for Sprint 3 scope.

---

### 9. Token revocation/blacklist approach sound?

| Aspect | Assessment |
|--------|-----------|
| DB-backed (no Redis) | Correct for MVP scale — simple, durable, no infrastructure dependency | ✅ |
| Caffeine cache (60s TTL, 10K max) | Appropriate tradeoff — worst case 60s window after revocation | ✅ |
| `jti` claim in JWT | Required for per-token revocation — currently missing, plan adds it | ✅ |
| `revokeAllForUser()` | Called on permission change + staff deactivation — correct triggers | ✅ |
| Cleanup scheduler (daily 3 AM) | Removes expired tokens to prevent table bloat | ✅ |
| Index on `jti` | Fast lookup for blacklist check | ✅ |
| Index on `expires_at` | Fast cleanup queries | ✅ |

**Result: ✅ PASS** — Sound, pragmatic approach for a club-scale application.

---

### 10. Risks not addressed?

| Potential Risk | Addressed? | Notes |
|----------------|-----------|-------|
| iText AGPL license | ✅ | Risk table mentions it, mitigation: switch to OpenPDF before go-live |
| Boot 4 `@EntityScan` issue | ✅ | Phase 7 step 7.1 explicitly addresses it |
| JSONB + Hibernate 6 | ✅ | `@JdbcTypeCode(SqlTypes.JSON)` mentioned |
| SMTP delivery | ✅ | Mailpit for dev, transactional email service for prod |
| Cache staleness | ✅ | Accepted as non-critical for club app |
| Portal CSRF + SPA | ✅ | `CookieCsrfTokenRepository.withHttpOnlyFalse()` |
| `JwtAuthFilter` portal path exclusion | ⚠️ | See finding #5 |
| Email domain regex DoS | ⚠️ | See finding #6 |

**Result: ✅ PASS** — All major risks addressed. Two minor items noted below.

---

## Findings

### ⚠️ Warnings (non-blocking — implement-time improvements)

#### 1. Dependency versions not pinned

**Plan §9** uses version ranges (`8.0.x`, `1.11+`, `3.1+`).

**Recommendation:** At implementation time, pin exact versions in the POM:
- iText 7: `8.0.5`
- Commons CSV: `1.12.0`
- Caffeine: `3.1.8`
- Testcontainers: `1.20.1`

---

#### 2. `revoked_tokens` table has no `tenant_id` column

The token blacklist is global across all tenants. This works correctly (tokens are unique by `jti` regardless of tenant), but it means:
- `revokeAllForUser(userId)` queries ALL tenants' revoked tokens
- No ability to purge one tenant's revoked tokens independently

**Recommendation:** Consider adding `tenant_id` for operational convenience. Not blocking — the current design is functionally correct.

---

#### 3. `POST /auth/logout` not integrated with token revocation

API spec §5.3 defines a logout endpoint that invalidates the refresh token. Sprint 3's `TokenRevocationService` adds access token revocation. The plan doesn't explicitly describe how these interact.

**Recommendation:** On `POST /auth/logout`:
1. Revoke refresh token (existing behavior)
2. Also add current access token's `jti` to `revoked_tokens` (new behavior)

This ensures immediate invalidation rather than waiting for natural token expiry. Implementor should handle this in Phase 1.

---

#### 4. Portal `formLogin` with JSON API may need `AuthenticationSuccessHandler`

The plan describes portal as "JSON API" (Decision D2: no Thymeleaf) but configures `formLogin()` with `defaultSuccessUrl`. Standard `formLogin` returns HTTP redirects (302), not JSON responses.

**Recommendation:** Implement a custom `AuthenticationSuccessHandler` that returns `200 OK` with session info as JSON, and a custom `AuthenticationFailureHandler` returning `401` JSON error. This aligns with the SPA architecture.

---

#### 5. `JwtAuthFilter.shouldNotFilter()` needs `/portal/**` exclusion

Current `shouldNotFilter()` skips `/api/v1/auth/`, `/swagger-ui`, `/v3/api-docs`. The plan adds a portal with session-based auth, but the JWT filter will still process `/portal/**` requests (finding no Bearer header → passes through).

**Recommendation:** Add `/portal/` to `shouldNotFilter()` for clarity and to avoid unnecessary filter processing:
```java
return path.startsWith("/api/v1/auth/")
    || path.startsWith("/portal/")  // ← add this
    || path.startsWith("/swagger-ui")
    || path.startsWith("/v3/api-docs");
```

Not strictly blocking (filter already passes through when no Bearer header present), but cleaner.

---

#### 6. Regex pattern validation — potential ReDoS

`validateEmailDomain()` uses `email.matches(club.getAllowedEmailPattern())` with admin-supplied regex. Malicious or poorly written regex could cause catastrophic backtracking (ReDoS).

**Recommendation:** Add a timeout or use `Pattern.compile()` with a simple validation check:
```java
try {
    Pattern.compile(pattern);  // validates syntax
    // Additionally: reject patterns with known dangerous constructs
    // Or use a timeout: java.util.concurrent.CompletableFuture with timeout
} catch (PatternSyntaxException e) {
    throw new InvalidRegexException(pattern);
}
```

The plan does validate invalid regex (step 2.7, test T-10) but doesn't mention ReDoS protection specifically.

---

#### 7. Architecture doc deviation should be noted

The architecture doc (03-Architecture.md) describes:
- Schema-per-tenant (Sprint 2 implemented shared-schema with `tenant_id`)
- `ROLE_CLUB_ADMIN` / `ROLE_PREVENTION_OFFICER` (code uses `ROLE_ADMIN` / boolean flag)
- 8-hour access token (code uses 1-hour)

These are expected evolutionary deviations — the architecture doc reflects initial design, and Sprint 2 made pragmatic choices. The architecture doc should be updated to reflect reality, but this doesn't block Sprint 3.

**Recommendation:** Add a backlog item to sync the wiki architecture doc with actual implementation post-Sprint 3.

---

## Traceability Matrix

| Requirement Source | Plan Step | Test Case | Status |
|-------------------|-----------|-----------|--------|
| Deferred: STAFF role | Phase 1 (1.1–1.15) | T-01 to T-06, T-25 | ✅ |
| Deferred: Club settings | Phase 2 (2.1–2.8) | T-07 to T-10 | ✅ |
| Deferred: Staff CRUD + invite | Phase 3 (3.1–3.15) | T-11 to T-14, T-25 | ✅ |
| Deferred: Reports (US-007, US-008) | Phase 4 (4.1–4.9) | T-15 to T-18 | ✅ |
| Deferred: Member portal | Phase 5 (5.1–5.7) | T-19, T-20 | ✅ |
| Deferred: Prevention Officer (US-010) | Phase 6 (6.1–6.8) | T-21, T-22 | ✅ |
| Deferred: Integration tests | Phase 7 (7.1–7.8) | T-23 to T-26 | ✅ |
| Decision D1: Token revocation | Phase 1 (1.5, 1.6, 1.10, 1.14) | T-05, T-06, T-26 | ✅ |
| Decision D2: Portal as JSON API | Phase 5 | T-19, T-20 | ✅ |
| Decision D3: Multiple prevention officers | Phase 6 (6.2, 6.7) | T-21 | ✅ |
| Decision D4: Minimal PDF branding | Phase 4 (4.4, 4.5) | T-16 | ✅ |
| Decision D5: Testcontainers | Phase 7 (7.2) | T-23 to T-26 | ✅ |
| Decision D6: Invite flow | Phase 3 (3.2–3.9) | T-11, T-12, T-25 | ✅ |
| Decision D7: Email domain whitelist | Phase 2 (2.7), Phase 3 (3.5) | T-09, T-10 | ✅ |

---

## Verdict

### ✅ APPROVED

The Sprint 3 plan is complete, technically sound, and ready for implementation. All 10 review checklist items pass. 7 non-blocking warnings are noted as implementation-time improvements — none require plan revision.

**Recommendation:** Proceed to implementation. The implementor should address warnings #3 (logout integration), #4 (portal auth handlers), and #5 (JwtAuthFilter exclusion) during Phase 1 and Phase 5 respectively.

---

## Review Metadata

| Field | Value |
|-------|-------|
| Review iteration | 1 of 3 (max) |
| Plan version reviewed | v2 |
| Time spent | ~15 minutes |
| Confidence | 92% |
| Blocking findings | 0 |
| Non-blocking findings | 7 |