fix(auth): guard the aggregate user-groups route — a non-admin read what four guarded routes refuse (+10 gate-9 mismatches)

[rubenvdlinde]

1. A privilege bypass by sibling route

Four routes each return one slice of the user-groups configuration, and each carries an explicit isAdmin() === false → 403 guard:

GET /api/settings/user-groups/generic             -> getGenericUserGroups()
GET /api/settings/user-groups/organization-admin  -> getOrganizationAdminGroups()
GET /api/settings/user-groups/super-user          -> getSuperUserGroups()
GET /api/settings/user-groups/all                 -> getAllGroups()

A fifth returns all four at once and checked only that the caller was logged in:

GET /api/user-groups/config                       -> getUserGroupsConfig()

SettingsService::getUserGroupsConfig() is literally the union of the four guarded getters. Any authenticated user could read through it exactly the data the four dedicated routes refuse them — including allGroups, the full group list of the instance.

Which side to fix was not obvious from the finding

The four guarded getters have zero callers in src/. Every consumer — UserGroupsConfiguration.vue via settings.js — hits /api/user-groups/config.

Read as "dead code", the tempting move is to delete the four guarded endpoints. That would have left the unguarded aggregate as the only surviving reader. Tracing the sibling seam gives the opposite answer: the four are the correct implementation and the live route is the one missing the guard.

So the guard was added to the live route, and to updateUserGroupsConfig() alongside it — that one was admin-only through the absence of @NoAdminRequired, i.e. middleware alone with no in-body backstop, unlike every one of its siblings.

Why no gate caught it

• gate-7 (no-admin-idor) counts getUser() === null as an auth guard, and passes.
• gate-9 (semantic-auth) compares the annotation against the body and finds no mismatch — a @NoAdminRequired method with no admin check is self-consistent.

The defect lives in the relationship between two endpoints, which neither gate models.

2. Ten annotation/body mismatches (gate-9, measured full-tree)

Eight SettingsController methods declared @NoAdminRequired while their bodies return 403 to non-admins. Their own sibling setters (setGenericUserGroups, setSuperUserGroups, …) already omit the annotation, so the file's intended pattern was unambiguous: drop it and let the middleware refuse before the controller runs. Same effective policy, enforced one layer earlier.

Two more — AanbodController::getAanbod and AangebodenGebruikController::getGebruiksWhereAfnemer — carried @PublicPage while their specs (REQ-009, REQ-004) require an authenticated caller and their bodies enforce it.

These are not the self-authenticating webhook/portal shape that makes gate-9's advice dangerous. Nothing authenticates the caller from the request; the body tests the session. @PublicPage therefore only ever admitted callers the body would then reject — every anonymous call was admitted, routed, and rejected in the controller. Replaced with @NoAdminRequired; the in-body guard stays as deny-before-grant.

Full-tree gate-9: 10 findings → PASS, re-measured with the runner from ConductionNL/.github@main over the whole tree, not a diff scope.

Evidence

SettingsControllerUserGroupsConfigAuthTest, five arms:

Arm	Asserts
non-admin read	403, payload absent from the body, and the service consulted zero times
admin read	200 and the configuration itself, not merely a 200
anonymous read	still 401, not promoted to 403 — a 403 tells an anonymous prober the resource exists and is admin-only
non-admin write	403, service never reached
admin write	200

Can-fail proof: reverting SettingsController.php to origin/development turns exactly the two non-admin arms red — Failed asserting that 200 is identical to 403. That is a non-admin receiving the configuration.

A measured detail worth keeping. The deny-before-grant check uses a call counter, not expects($this->never()). The controller wraps its body in catch (\Exception), which swallows a PHPUnit expectation failure into a 500 and reports the leak as an unrelated server error — the first version of this test failed with 500 is identical to 403, which names the wrong defect.

Checks

Check	Result
phpcs --standard=phpcs.xml lib/	0 errors, 87 warnings — unchanged from base
phpunit -c phpunit-unit.xml	486 passed, 25 skipped
hydra-gates full-tree, gate-5 / 7 / 9 / 14 / 30	all PASS

One thing deliberately not changed

getArchiMateConfig(), getArchiMateSettings() and testArchiMateRoundTrip() are @NoAdminRequired with no admin guard, while importArchiMate() and updateArchiMateConfig() — the write halves — are admin-only. The read side of an admin-panel concern being open to any authenticated user is an inconsistency, but "who may read the AMEF register/schema configuration" is a product decision, not an obvious defect, so it is reported rather than changed here.

[github-actions]

Quality Report — ConductionNL/softwarecatalog @ 31b2514

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
check-vue-demi		✅
test-l10n		✅
composer			✅	✅ 128/128
npm			✅	✅ 718/718
PHPUnit					✅
Newman					⏭️
Playwright					✅
Hydra gates					✅

---

Quality workflow — 2026-08-08 11:15 UTC

Download the full PDF report from the workflow artifacts.