Files
atomic-design-poc/docs/project/archive/backlog/WP-69-intake-scholing-threshold-enforcement.md
T
ehoandClaude Opus 5 12f17d9d73 docs: archive the finished backlogs (RD-30)
Two backlog trees are complete: `docs/project/backlog/` (75 files, every
WP done) and `docs/project/refactor-backlog-setup/` (the arc before it).
Move both under `docs/project/archive/` with `git mv`, so history stays
intact through `git log --follow`. `SHOWCASE-ROADMAP.md` moves with them,
because it points at the now-archived backlog README.

Add `docs/project/archive/README.md`. It states that these trees are
historical and names the two directories that are still live.

Repoint every inbound reference named in RD-30's Files table: CLAUDE.md,
the root README, both backend READMEs, `LetterHtml.cs`, `a11y.mdx`, the
`document-feature` and `new-ssp` skills, and the readable-codebase PLAN,
README, and RD-19 ticket. Fix two upward-relative links inside the moved
WP files (WP-68, WP-69) that gained a directory level and would otherwise
break. Repoint `.prettierignore`'s two agent-prompt exclusions to their
new path, so prettier keeps leaving those files' exact wording alone.

Mark RD-30 done and check off its acceptance criteria; flip its README
row to done.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-08 23:00:38 +02:00

227 lines
14 KiB
Markdown

# WP-69 — Enforce the scholing threshold server-side
Status: done (5d73ca2)
Phase: 12 — DDD hardening
## Verification result (2026-08-18)
Backend 230 → 245 tests (+8 `Domain/IntakeRuleTests`, +7 `Acceptance/IntakeSubmissionTests`);
frontend ssp 238 → 242. `npm run ci` green incl. `check:seam` (`OK … (1000)`), api-client and
behaviour-spec drift.
**The bypass was proven closed against a running backend, not just by green tests:**
`POST /api/v1/intakes {"uren": 500}` with no scholing fields → **400** with the Dutch detail;
an intake-typed Concept submitted via `POST /applications/{id}/submit {"uren": 500}` → **400**,
and `GET /applications/{id}` afterwards still reports `"tag":"Concept"` (no state change, still
retryable); the same request plus `"aanvullendeScholing": false` → **200** with a referentie and
`InBehandeling`.
**Deviation from the Files list:** `EndpointTests.Worked_hours_submission_succeeds` had to be
touched despite being on the do-not-modify list — it posts `{ uren: 40 }` with no scholing
answer, which _is_ the crafted-POST bypass this WP closes, so the existing test was itself
asserting the vulnerable behaviour. Fixed minimally by adding `aanvullendeScholing = false`
(unknown to and ignored by `HerregistratieRequest`, so the paired `/herregistraties` row is
unaffected). The zero-hours 422 rows — the ordering regression net — are unmodified as planned.
## Why
WP-68 (F5) found that `IntakePolicy`'s doc-comment claimed _"the backend re-validates on
submit as the authority"_ — it doesn't. `GET /intake/policy` only echoes `ScholingThreshold`;
neither `SubmitApplicationRequest` (`DiplomaHerkomst`, `Uren`, `Documents`) nor `IntakeRequest`
(`Uren`) carries a scholing answer at all, so there's nothing for the server to re-validate.
Both submit paths apply only `SubmissionRules.RejectZeroUren`. A crafted POST — bypassing the
wizard entirely — can skip the scholing requirement (`scholingGevolgd`/`punten` in
`intake.machine.ts`) even though it's presented as mandatory in the UI. ADR-0001's canonical
"config value" example (the FE applies the threshold for instant feedback, the backend
re-validates as authority) is unenforced for the one rule it was written to illustrate.
## Read first
- `backend/src/BigRegister.Api/Domain/Intake/IntakePolicy.cs` (the corrected doc-comment,
WP-68)
- [ADR-0001 — BFF-lite + decision DTOs](../../../reference/architecture/0001-bff-lite-decision-dtos.md)
§"config value"
- `apps/ssp/src/app/herregistratie/domain/intake.machine.ts` (`lageUren`, `scholingGevolgd`,
`punten` — the wizard's existing FE-side rule and its answers)
- `backend/src/BigRegister.Api/Contracts/Dtos.cs` (`SubmitApplicationRequest`,
`IntakeRequest`, `DocumentRefDto`)
- `backend/src/BigRegister.Api/Program.cs` — the `intakes` and `applications/{id}/submit`
endpoints
## Stale premises in the original placeholder (verified 2026-08-18)
- **"Both submit paths" is half-stale.** `POST /api/v1/intakes` is **dead from the UI** — the
wizard submits via `draft-sync` → `POST /applications/{id}/submit`; no code in `apps/` or
`libs/` calls the generated `intakes()`/`herregistraties()` methods. It is still a live
crafted-POST surface, so fix both; do **not** delete it here (see Risks).
- **The submit endpoint does not distinguish intake from herregistratie** —
`Program.cs` lumps them: `_ /* herregistratie | intake */ => (RejectZeroUren(...), true)`.
`herregistratie.machine.ts` has no scholing question, so the new check **must** be gated on
`existing.Type == "intake"` or the herregistratie wizard starts 400-ing for every low-uren user.
- **`RejectMissingScholing(uren, scholing)` is under-specified.** The rule is three-valued
(answer present / `true` needs punten / punten without `true` is illegal); two parameters
cannot express it.
- **A live FE bug shares this rule and must be fixed here.** `intake.machine.ts:116` requires
`punten` whenever `scholingGevolgd === 'ja'` **regardless of `lageUren`**, while the template
renders both fields only inside `@if (scholingZichtbaar())` (= `lageUren`). Answer scholing
`'ja'`, then raise `uren` above the threshold: either the user is blocked by an error on an
**invisible** field, or `validateAll` emits `aanvullendeScholing: undefined` **together with**
`punten: 150` — exactly the payload the new server rule rejects. Both branches reachable today.
## Decisions
Made by a `planner` pass on 2026-08-18 — do not relitigate.
### 1. What the rule is (and deliberately is not)
The FE rule is **completeness**, not merit: below the threshold the scholing question must be
**answered**; `'nee'` is a legal answer that still submits. So the server authority is:
- `uren < IntakePolicy.ScholingThreshold` ⇒ an answer must be present;
- answer `true` ⇒ punten present and `>= 0` (mirrors `parseUren`);
- answer not `true` ⇒ punten must be **absent**.
**Out of scope, deliberately:** turning "few uren + no scholing" into an `Afgewezen` decision.
The wizard accepts that today; inventing a substantive rejection would create a _new_ FE/BE
divergence in the WP that closes one. **Boundary is `<`, not `<=`** — mirrors `lageUren`.
### 2. Wire shape
Two nullable fields appended (positionally last, defaulted) to both request records in
`Contracts/Dtos.cs`: `bool? AanvullendeScholing = null, int? ScholingPunten = null`.
- **`ScholingPunten`, not `Punten`** — `SubmitApplicationRequest` is shared by all three wizard
types and the herregistratie wizard has its own unrelated `punten`.
- **Illegal states are representable on the wire, unrepresentable past the boundary.** A JSON
DTO consumed by NSwag can't carry a union without hand-written polymorphism, and both fields
must be optional for the other wizards anyway. Closure happens at the rule boundary — the same
posture WP-68 took for `AanvraagStatus`. _Rejected:_ a nested `ScholingDto` (removes one of
three illegal combinations, adds a DTO); a closed `ScholingAnswer` type (one call site, not
persisted — ceremony).
- **Not persisted.** Submit-time rule input, not aggregate state: no `Aanvraag` column, **no EF
migration**. The draft JSON stays opaque (WP-68) — the answer arrives as an explicit field.
### 3. Rule home — `IntakePolicy`, not `SubmissionRules`
`public static string? RejectIncompleteScholing(int uren, bool? aanvullendeScholing, int? scholingPunten)`
— same "reason or null" idiom as `SubmissionRules`, so endpoints compose both identically.
The rule _is_ the threshold's enforcement and the class already owns the constant. Putting it in
`SubmissionRules` would either re-declare `1000` there (silent drift — exactly what WP-71's
`check:seam` exists to catch, and which it would **not** catch outside `IntakePolicy.cs`) or make
the generic cross-wizard class depend on one wizard's policy. `SubmissionRules.cs` and
`SubmissionRuleTests.cs` are **not modified**.
**`check:seam` constraint (load-bearing):** `scripts/check-seam.sh` greps _all_
`ScholingThreshold\s*=\s*[0-9]+` matches in `IntakePolicy.cs`. The new code must **reference**
the const (`uren < ScholingThreshold`, `$"…{ScholingThreshold}…"`) and must never introduce a
second literal (e.g. a default parameter `int scholingThreshold = 1000`) — a second match makes
`backend_value` two lines and fails with a misleading "drift" message.
### 4. HTTP shape: 400 ProblemDetails, matching WP-68 F1
A missing/contradictory conditionally-required field is a **contract violation**, not a business
outcome → `Results.Problem(detail: …, statusCode: 400)`. Deliberately unlike `RejectZeroUren`,
which is a _merit_ rejection (422 legacy / `Afgewezen` + 200 on the aanvraag path).
**Ordering: the zero-uren rejection wins.** Guard with `reject is null &&` so `{ uren: 0 }` is
decided on merit and completeness is moot — this keeps `EndpointTests`' 422 rows passing
**unmodified**. Place the check **before** the document-ownership check and
`ApplicationStore.Submit`, so a rejected submit leaves the aanvraag a Concept (retryable).
Gated on `existing.Type == "intake"`. `/applications/{id}/submit` already declares
`.ProducesProblem(400)` (WP-68 F1) — no metadata change; `/intakes` needs one added, with the
check _outside_ the `Submit(...)` helper so the 400 is not cached in `IdempotencyStore`.
Detail copy (Dutch, like all backend ProblemDetails — backend copy is not `$localize`d):
missing answer → `$"Beantwoord de vraag over aanvullende scholing: bij minder dan {ScholingThreshold} gewerkte uren is dit verplicht."`;
`true` without punten → `"Vul het aantal behaalde nascholingspunten in."`;
punten without `true` → `"Nascholingspunten horen alleen bij een gevolgde aanvullende scholing."`
### 5. Backwards compatibility
Fields optional on the wire, conditionally required by the rule (the same DTO serves registratie
and herregistratie, which never send them). **In-flight Concept drafts (WP-22) are unaffected** —
the draft JSON already holds `scholingGevolgd`/`punten`, its format doesn't change, and the new FE
derives the request fields at submit time. The one real incompatibility is a **stale FE bundle**
submitting a below-threshold intake: it gets a 400 with an actionable Dutch detail via
`problemDetail()`. Accepted — the POC has no API versioning, and the alternatives (grace period,
inferring from the draft) are what WP-68 forbade. _Rejected:_ a feature flag whose only purpose
is to leave a security gap open.
### 6. Frontend changes
- Wizard payload gains `aanvullendeScholing` + `scholingPunten` (`undefined` members are dropped
by `JSON.stringify` and bind to `null` server-side).
- **`intake.machine.ts` needs two narrowing edits** (see Stale premises — this is a live bug):
`validateStep('werk')` requires punten only when `lageUren(…) && scholingGevolgd === 'ja'`
(matching the template's `@if`), and `validateAll` computes punten from
`aanvullendeScholing === true` rather than `scholingGevolgd === 'ja'`, so a stale answer left by
raising `uren` can't leak into `ValidIntake`. `Answers` (the raw record) is unchanged — stale
raw answers are fine; `ValidIntake`, the _parsed_ type, must be honest.
- **No change** to `SCHOLING_THRESHOLD_DEFAULT`, `lageUren`, `SetPolicy`, the policy
adapter/store, or the template. **No new `$localize` id ⇒ no `messages.en.xlf` change.**
### 7. Test plan (WP-71 conventions)
G/W/T bodies, `Domain/<Aggregate>RuleTests.cs`, `Acceptance/`, fixtures via the `Given` builder —
never hand-built initializers.
**New `Domain/IntakeRuleTests.cs`** (pure rule, no HTTP; the arguments _are_ the Given, so these
degenerate to When/Then per `bdd.mdx`): answer required below threshold; not required _at_ the
threshold (pins `<` vs `<=`); `niet gevolgd` is a complete answer (pins §1's scope); `gevolgd`
requires punten; zero punten valid; negative refused; `[Theory]` — punten without `gevolgd`
refused (two rows, incl. the stale-punten shape §6 removes).
**New `Acceptance/IntakeSubmissionTests.cs`** (HTTP, both paths, `Given.Concept(type: "intake")`
- a local `Persist` mirroring `BesluitLifecycleTests`; the builder's default owner **is**
`StubIdentityProvider`'s default caller, so no header juggling): below threshold without an
answer → 400 **and still a Concept**; answered → 200; above threshold → 200; punten without
gevolgd → 400; **herregistratie unaffected** (guards the `Type` gate); `{ uren: 0 }` still
`Afgewezen` + 200, not 400 (pins the ordering); legacy `/intakes` enforces it too.
**Not modified:** `EndpointTests.cs` (its 422 rows are the ordering regression net),
`SubmissionRuleTests.cs`, `ApplicationTests.cs`, `Builders/AanvraagBuilder.cs`.
**Frontend:** `intake.machine.spec.ts` — drops punten when raising uren hides the question; does
not require punten for a hidden question. `intake.acceptance.spec.ts` — one journey: low uren →
`'ja'` + punten → back → raise uren → submit → both fields `undefined`.
### 8. Sequencing
1. `IntakePolicy.RejectIncompleteScholing` + `Domain/IntakeRuleTests.cs` (red→green, no wire change).
2. `Contracts/Dtos.cs` + both endpoints + `/intakes`' `.ProducesProblem(400)`.
3. `Acceptance/IntakeSubmissionTests.cs`; `dotnet test`.
4. **`npm run gen:api`** — after step 2, before the FE payload change. Commit `backend/swagger.json`
- `libs/shared/src/infrastructure/api-client.ts`. CI's drift job fails if skipped/hand-edited.
5. FE: `intake.machine.ts` narrowing + specs, then the wizard payload.
6. `npm run gen:snippets` (expect no diff) and **`npm run gen:behaviour-spec`** (will diff — new
test names; commit it or CI's drift step fails).
7. Docs in the same diff: rewrite `IntakePolicy`'s doc-comment from "gap deferred to WP-69" to what
it now guarantees; one line in ADR-0001 §"config value"/worked example B; `backend/README.md`'s
`/api/intakes` row (add the 400) + its `IntakePolicy.cs` bullet.
8. `npm run ci`.
## Out of scope
- Turning "few uren + no scholing" into an `Afgewezen` **decision** (§1) — a decision flag, an FE
change, and a separate WP.
- Deleting the dead `/intakes` + `/herregistraties` endpoints (with `EndpointTests`,
`backend/README.md`, `gen:api`) — real cleanup, but not this WP's security fix.
- The herregistratie wizard's `jaren`/`punten`, equally un-re-validated server-side.
- `docs/reference/fp-tea-atomic-design.md:587` / `ARCHITECTURE.md:464` still teach a
`visibleSteps`-with-a-`'scholing'`-step intake the fixed-3-step wizard no longer matches.
## Risks
- **Ordering regression (highest).** Running completeness before `RejectZeroUren` silently turns
`{ uren: 0 }` from 422/`Afgewezen` into 400 and breaks two existing endpoint tests. The
`reject is null &&` guard is load-bearing — keep the comment saying why.
- **`check:seam` false failure** if a second `ScholingThreshold = <digits>` literal lands in
`IntakePolicy.cs` (§3). The message will say "FE/BE seam drift" and mislead.
- **Missing the `Type == "intake"` gate** breaks the herregistratie wizard for every low-uren
user; the `herregistratie is unaffected` test is the only net.
- **Stale-bundle 400 loop:** the wizard's `Retry` re-sends the identical payload, so a pre-deploy
tab loops until reloaded. Acceptable for a POC.