Skip to content

feat(settings): add canonical PUT /api/settings (settings#update) - #464

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/settings-put-update
Aug 8, 2026
Merged

feat(settings): add canonical PUT /api/settings (settings#update)#464
rubenvdlinde merged 1 commit into
developmentfrom
fix/settings-put-update

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

This is a 405 / missing-route case, not the fleet-wide 500

The fleet brief claimed PUT /api/settings "resolves to a nonexistent method" in 12 apps. That is not what softwarecatalog does. Measured on the running dev instance (2026-08-08, curl -s -o /tmp/out -w "%{http_code}", -u admin:admin):

Verb /apps/softwarecatalog/api/settings Before After (expected)
GET settings#index 200 200
POST settings#create 200 200 (unchanged)
PUT (no route) 405 Method Not Allowed 200

405, not 500. appinfo/routes.php does not call \OCA\OpenRegister\AppHost\Routes::standard() and declared no settings#update, so the PUT verb simply had no route — the router rejected it before any controller was reached. And because the app ships its own lib/Controller/SettingsController.php, AppHost's Bootstrap::aliasControllerUnlessLeafDefinesIt() skips the generic controller, so the leaf owes every settings# method itself.

Did the canonical shape actually fit?

Yes, cleanly — better than expected for a ~70-method controller. create()'s body was already decomposed into exactly three private helpers, and those three correspond one-for-one to what index() reads back. The move was verbatim.

What update() writes

Exactly the three sections index() surfaces through SettingsService::getAllSettings():

Request key Persisted via
configuration / selectedRegister SettingsService::updateSettings()
userGroups.{generic,organizationAdmin,superUser} validateGroups() → matching setter (400 on invalid)
emailSettings SettingsService::updateEmailSettings()

Surfaces deliberately NOT absorbed

update() is not a catch-all. Every one of these keeps its own URL and its own handler, untouched:

  • getGeneralConfig/updateGeneralConfig, getSyncConfig/updateSyncConfig
  • getArchiMateConfig/updateArchiMateConfig, getEmailConfig/updateEmailConfig
  • getAmefConfig/updateAmefConfig, getVoorzieningenConfig/updateVoorzieningenConfig
  • getUserGroupsConfig/updateUserGroupsConfig, getCronjobConfig/updateCronjobConfig
  • getEolSyncConfig/updateEolSyncConfig, email templates, ArchiMate import/export/download, progress streaming, stats, object counts

A dedicated test (testUpdateIsNotACatchAllForOtherSettingsSurfaces) posts catalogLocation/syncTimeWindow/cronjobs/eolSync and asserts nothing is persisted.

Auth decision

create() declares @NoCSRFRequired and deliberately not @NoAdminRequired — so NC's security middleware requires an administrator. update() mirrors that exactly. Net privilege change: zero.

  • No posture was copied from a sibling read method. Only create() was read.
  • create() keeps its own @NoCSRFRequired even though it now delegates — the middleware evaluates attributes on the dispatched method, so delegation does not inherit them.
  • create() had no in-body guard, so nothing needed to move.
  • testUpdateHasIdenticalAuthPostureToCreate pins parity in both directions and additionally asserts update() can never gain @NoAdminRequired/@PublicPage, in docblock or attribute form.

This test caught a real defect during development: my first update() docblock explained the posture in prose containing the literal string @NoAdminRequired. A grep-based auth gate would have read that as the tag itself. Reworded.

Can-fail proof

Both halves were removed with the Edit tool (not git stash/git checkout --) and shown RED.

1. update() removedEXIT=2

1) …SettingsControllerCanonicalWriteTest::testCanonicalMethodExistsAndIsDispatchable with data set "update (PUT)"
SettingsController::update() is missing — the route would 500 on dispatch.
3) …SettingsRouteTableTest::testEverySettingsRouteTargetsAnExistingPublicMethod
Route settings#update points at a nonexistent SettingsController::update().
Tests: 17, Assertions: 45, Errors: 6, Failures: 3.

2. Route line removedEXIT=1

1) …SettingsRouteTableTest::testCanonicalRouteIsRegistered with data set "PUT  canonical write"
Expected exactly one route PUT /api/settings → settings#update, found 0.
2) …testCanonicalWriteIsDeclaredBeforeTheSpaCatchAll
settings#update PUT is not registered.
Tests: 6, Assertions: 172, Failures: 2.

3. Both restoredEXIT=0

OK (17 tests, 219 assertions)

Full unit suite: Tests: 512, Assertions: 1814, Deprecations: 2, Skipped: 25EXIT=0. The 2 deprecations are pre-existing (the 17 new tests run clean).

Tests

tests/Unit/Controller/SettingsControllerCanonicalWriteTest.php and tests/Unit/SettingsRouteTableTest.php:

  • Each canonical method asserted individually (public, non-static, zero required params) — never "the class exists".
  • Positive controls in both files ($inspected > 0, route table > 50 entries, settings# routes matched) so a scan that matched nothing cannot report green.
  • Route test evaluates appinfo/routes.php — a commented-out entry would satisfy a grep but fails here.
  • Every settings# route asserted to target an existing public method (gate-14 in miniature).
  • PUT asserted to precede the SPA /{path} catch-all.
  • Behavioural: update() writes all three sections; returns the identical payload create() used to; 400 on invalid groups; 500 mapping; create() delegates structurally (return $this->update();, and does not contain updateConfigSettings).

Coverage: new statements 17/17 covered, none uncovered. Repo line coverage measured at 13.37% (pcov, 4239/31712) vs the committed CI baseline of 13.01 — the ratchet moves up, not down.

Gates

run-hydra-gates.sh --scope-to-diff --base origin/development (gates taken from ConductionNL/.github@origin/main; the local .github checkout was 77 commits behind and was not used). Verdicts read from stdout, not the exit byte:

Zero FAILs. 57 gates green. Including gate-1 spdx, gate-5 route-auth, gate-9 semantic-auth, gate-14 route-reachability, gate-16 spec-coverage, gate-46 spec-anchor-existence, gate-47 security-change-has-tests, gate-48 csrf-cochange, gate-64 apphost-autoload-prelude.

3 applicable gates did not run (structural skips, honestly reported — not passes): gate-19 e2e-coverage, gate-62 store-plane, gate-63 settings-surface. All three skipped because this diff touches no spec file and no manifest, which is their subject matter.

The @spec anchor …method-decomposition/spec.md#requirement-settingscontroller-settings-crud-endpoints-req-decomp-013 was verified against the real gate-46 checker, with a positive control proving the checker reports a bogus anchor.

A full-repo (unscoped) gate run shows 17 pre-existing failures (a11y debt, 291 spec scenarios missing @e2e, etc.). None are attributable to this change: gate-46's 4 findings are all in SbomImportService/SbomParserService/suiteWizard.js, and settings#update does not appear in gate-25's contract-coverage log.

Also clean: php -l, PHPCS (phpcs.xml scopes lib only) and Psalm on the changed controller.

Not merged

Left open for the orchestrator.

`PUT /api/settings` answered **405 Method Not Allowed** on the dev instance,
not 500: softwarecatalog does not call `\OCA\OpenRegister\AppHost\Routes::standard()`
and declared no `settings#update`, so the PUT verb simply had no route.
Because the app ships its own `SettingsController`, AppHost's
`aliasControllerUnlessLeafDefinesIt()` skips the generic controller and the
leaf owes every `settings#` method itself.

Strict addition — nothing removed:

- `SettingsController::update()` takes the former `create()` body verbatim.
  It writes exactly the three sections `index()` reads back via
  `SettingsService::getAllSettings()`: `configuration`/`selectedRegister`,
  `userGroups.{generic,organizationAdmin,superUser}` and `emailSettings`.
  It is deliberately NOT a catch-all: the ~35 other `getXConfig`/`updateXConfig`
  pairs, email templates, ArchiMate import/export, progress and stats keep
  their own endpoints untouched.
- `create()` becomes `return $this->update();` and keeps its own
  `@NoCSRFRequired` tag — NC middleware only evaluates attributes on the
  dispatched method, so delegation cannot inherit the posture.
- `appinfo/routes.php` gains `settings#update` PUT `/api/settings`, before the
  SPA `/{path}` catch-all. Route and method land together so the method is
  never unreachable (gate-14).

Auth: `create()` declares `@NoCSRFRequired` and deliberately not
`@NoAdminRequired`, so NC requires an administrator. `update()` mirrors that
exactly; net privilege change is zero. A test pins the parity in both
directions and asserts `update()` can never gain `@NoAdminRequired`/`@PublicPage`.

Tests: 17 new assertions-heavy cases across two files, covering per-method
existence (item, not container) with a positive control, the evaluated route
table, the write behaviour, the non-catch-all guarantee, the 400 group-validation
path, exception mapping, and that `create()` delegates rather than duplicates.
New statements are 17/17 covered.
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/softwarecatalog @ a662f0e

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 16:26 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit b3838c6 into development Aug 8, 2026
27 of 29 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/settings-put-update branch August 8, 2026 16:28
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