Skip to content

refactor(stuf)!: rename inkomend to inbound, and test the guard nobody watched - #844

Merged
rubenvdlinde merged 2 commits into
developmentfrom
refactor/stuf-inbound-english
Aug 14, 2026
Merged

refactor(stuf)!: rename inkomend to inbound, and test the guard nobody watched#844
rubenvdlinde merged 2 commits into
developmentfrom
refactor/stuf-inbound-english

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

What

  1. Renames the Dutch inkomend StUF receiver to inbound — method, route name, canonical URL, log messages and docblocks.
  2. Adds the test suite it never had: 5 tests, and deleting the WSSE guard makes them red.

The guard was real but unwatched

inbound() verifies a WSSE UsernameToken and answers 422 on mismatch. Nothing tested it. No test in the repo referenced the endpoint at all — delete the verifyWsse() call and every test stayed green.

Its two sibling public routes already had that cover in StufSoapRequestDispatcherAuthTest, whose docblock states the standard exactly right: these are #[PublicPage] routes, nothing in Nextcloud's middleware will refuse a caller, so the refusal has to come from the app and has to be tested. This applies that standard to the third route.

The negative control

Guard deleted → 2 failures, and the informative one is this: logInbound was called with a tampered envelope. The message was accepted and recorded. Guard restored → 5/5 green.

test what it pins
testATamperedWsseTokenIsRefused 422, and logInbound is never reached — an endpoint that records first and refuses second has already accepted the message
testAnUnresolvableEndpointIsRefused 400, and verifyWsse is never called
testAnEmptyBodyIsRefused 400 before anything is resolved
testAVerifiedSenderIsProcessedAndAcknowledged the positive control — without it, return 422 on line one would pass every refusal test
testTheTokenIsVerifiedAgainstTheResolvedEndpoint the token is checked against that sender's stored credentials, so one configured system cannot impersonate another

Why it was untestable

$rawXml = (string)file_get_contents(filename: 'php://input');

OCP\IRequest exposes no raw-body accessor, so there was no seam. Added a protected readRawBody() the test overrides. Production path unchanged — same call, one indirection.

The URL is a wire contract, so the old one still answers

/api/stuf/inbound is canonical. /api/stuf/inkomend remains as an explicitly deprecated alias routed to the same method, because that URL lives in the upstream zaaksysteem's configuration, not ours — renaming it alone turns a working webhook into a silent 404 on somebody else's schedule. The alias is commented as a migration step with an end.

If procest isn't deployed anywhere yet, say so and I'll drop the alias — it's the one line still carrying the Dutch spelling.

Dutch that is deliberately NOT in this PR

These are stored data values, so changing them is a data migration, not a rename:

  • 'direction' => 'inkomend'ContactMomentController, ContactMomentService, StufMessageHandler
  • "enum": ["uitgaand", "inkomend"]register.d/80-stuf-zkn-outbound.json, procest_register.json, 40-kcc-werkplek.json

Existing rows carry those values; renaming the code without migrating the data breaks every read. They belong in the Dutch→English programme's data tranche.

Also still Dutch and also wire contracts: the sibling routes /api/stuf/zaken and /api/stuf/personen.

Verification

  • php -l clean on all changed files.
  • Full unit suite: 1904 tests, 8007 assertions, 0 failures (run on PHP 8.4 in the container — this box has 8.2 and the app requires ^8.3).
  • No stale reference to stuf#inkomend, ->inkomend( or function inkomend anywhere in lib/, tests/ or appinfo/; the only surviving inkomend is the deliberate alias URL.

Closes #843.

🤖 Generated with Claude Code

…y watched

TWO THINGS, ONE CAUSE.

The endpoint was Dutch, and it was untested. Both are fixed here because the
second is what made the first worth doing carefully: renaming a webhook route
without a test suite means finding out from a municipality.

THE GUARD WAS REAL BUT UNWATCHED

inbound() verifies a WSSE UsernameToken and answers 422 on mismatch, and
NOTHING tested it - no test in the repository referenced the endpoint at all.
Delete the verifyWsse() call and every test stayed green. Its two sibling
public routes already had that cover in StufSoapRequestDispatcherAuthTest,
whose docblock states the standard: these are #[PublicPage] routes, nothing in
Nextcloud's middleware will refuse a caller, so the refusal has to come from
the app and has to be tested.

Negative control run before pushing: with the guard deleted the suite gives 2
failures, and the informative one is that logInbound was called with a
TAMPERED envelope - the message was accepted and recorded. Restored, 5/5 pass.

The five arms pin: 422 with logInbound never reached (an endpoint that records
first and refuses second has already accepted the message); 400 for an
unresolvable sender with verifyWsse never called; 400 for an empty body; a
POSITIVE control that a verified sender is acknowledged (without it, `return
422` on line one would pass every refusal test); and that the token is checked
against THAT sender's stored credentials, so one configured system cannot
impersonate another.

WHY IT WAS UNTESTABLE

`file_get_contents('php://input')` has no seam and OCP\IRequest exposes no
raw-body accessor. Added a protected readRawBody() the test overrides. The
production path is unchanged - same call, one indirection.

THE URL IS A WIRE CONTRACT

/api/stuf/inbound is canonical. /api/stuf/inkomend stays as an explicitly
deprecated alias to the same method, because that URL lives in the UPSTREAM
zaaksysteem's configuration, not ours: renaming it alone turns a working
webhook into a silent 404 on somebody else's schedule. It is commented as a
migration step with an end.

DUTCH DELIBERATELY LEFT OUT OF THIS PR

'direction' => 'inkomend' and the ["uitgaand","inkomend"] enums are STORED
DATA VALUES. Existing rows carry them, so renaming the code without migrating
the data breaks every read - that belongs in the Dutch-to-English programme's
data tranche. The sibling routes /api/stuf/zaken and /api/stuf/personen are
Dutch and are wire contracts too.

Verification: php -l clean; full unit suite 1904 tests, 8007 assertions, 0
failures on PHP 8.4 in the container (this box runs 8.2, the app needs ^8.3);
no stale reference to stuf#inkomend, ->inkomend( or function inkomend in lib/,
tests/ or appinfo/.

Closes #843.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/procest @ c00e8b0

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue3-compile
test-l10n
format
composer ✅ 104/104
npm ✅ 535/535
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-14 20:08 UTC

Download the full PDF report from the workflow artifacts.

CI caught this, and it was mine. The alias registered a SECOND routes.php
entry under the same name, `stuf#inbound`, distinguished only by
'postfix' => 'legacy-dutch-alias'.

Nextcloud's own RouteParser does support `postfix` - it appends to the
generated route name. openregister's AppHost Routes::standard() does NOT: its
assertNoDuplicateNames() keys on `name` alone and never reads `postfix`, so
the two-entries-one-name form throws

    InvalidArgumentException: Duplicate route name "stuf#inbound"
    in AppHost Routes::standard($extra)

at boot. That does not fail one endpoint, it takes the whole app's routing
down - which is why it surfaced as procest's E2E SEED failing rather than as a
routing test.

The alias now routes to its own method, inboundLegacyPath(), which delegates
to inbound(). The name is unique, the identifier is English, and the Dutch
spelling survives only in the URL string - which is the wire contract held in
the upstream zaaksysteem's configuration and the whole reason the alias
exists. The constraint is written at both the route and the method so the next
person does not rediscover it by breaking boot.

Verification, redone against the RIGHT tree. The earlier full-suite run was
made against the shared bind-mounted checkout, which is sitting on another
workstream's branch (refactor/adr-083-inject-openregister, 8 commits behind
development) where SamenwerkverzoekService gained a constructor argument its
test was never updated for - 8 errors that belong to that branch, not to this
one. Re-run against this worktree with vendor copied in: 1904 tests, 8007
assertions, 0 failures. StufControllerInboundTest still 5/5.

Also verified directly: parsing appinfo/routes.php gives 388 entries and ZERO
duplicate names, and the same check flags a planted duplicate - so the check
is not vacuous.
@rubenvdlinde
rubenvdlinde merged commit a845599 into development Aug 14, 2026
44 of 73 checks passed
@rubenvdlinde
rubenvdlinde deleted the refactor/stuf-inbound-english branch August 14, 2026 21:24
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/procest @ 18477c8

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue3-compile
test-l10n
format
composer ✅ 104/104
npm ✅ 535/535
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-14 21:27 UTC

Download the full PDF report from the workflow artifacts.

rubenvdlinde added a commit that referenced this pull request Aug 15, 2026
…alive (#846)

Finishes what #844 started on the StUF surface. Both old URLs keep answering:

    POST /api/stuf/cases     canonical    /api/stuf/zaken     deprecated alias
    POST /api/stuf/persons   canonical    /api/stuf/personen  deprecated alias

Same reasoning as /api/stuf/inkomend: the URL lives in the SENDING
zaaksysteem's configuration, not ours, so renaming it alone turns a working
SOAP receiver into a silent 404 on somebody else's schedule. Each alias needs
its OWN method because AppHost Routes::standard() rejects duplicates by `name`
and ignores `postfix` - that form throws at boot and takes the whole app's
routing down. Verified: 390 route entries, 0 duplicate names.

`zaken` MEANS TWO DIFFERENT THINGS AND ONLY ONE OF THEM MOVED

The StUF $service token is INTERNAL - the sending endpoint is resolved from
the envelope's zender, and the token's only other use is log context. It never
reaches the wire, so it is now SERVICE_CASES / SERVICE_PERSONS.

Deliberately untouched because they are statutory:
  ZrcController / ZgwService / ZgwRulesDispatcher   $resource === 'zaken' is
                                                    the ZGW REST resource
  AcController                                      scopesContain(...,'zaken')
                                                    is a ZGW autorisaties scope

That distinction is the whole risk in this change and a test asserts it.

Fault messages moved to English - 'Authenticatie mislukt', 'Ongeldig XML
bericht', 'Onbekend berichttype'. The StUF fault CODE (StUF001) is statutory
and unchanged; only the free-text faultstring moved.

Verification: a new 4-test suite asserts each alias dispatches with the
CORRECT service constant, not merely that it dispatches - passing the wrong
service would still return 200 and route StUF-ZKN traffic through the person
handler. Negative control: wiring casesLegacyPath() to persons(), the
plausible copy-paste slip, makes it fail; restored, green. php -l clean; full
unit suite 1921 tests, 8690 assertions, 0 failures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant