fix(folders,locking): a system context is recognised, and PATCH answers the lock

[rubenvdlinde]

What was broken

Three defects found while validating a live demo instance. None of them was found by the suite.

1. A repair step or cron job could not save an object with a bound folder at all

FolderManagementHandler::assertFolderIsAccessible() default-denies when there is no IUser. A system operation has no user by definition, so a repair step and an anonymous request were indistinguishable to it. runAsSystem() does not help: SystemOperationContext::run() flips a depth counter, it never sets a session user.

Every one of these callers catches and logs, so the failure is silent: the step records "could not run" and occ upgrade still reports success. That is how dossiq's RealignStatutoryVocabulary did nothing while leaving 18 cases and 7 caseTypes holding corrupted confidentiality values.

Scope in this repo — four write paths, all behind a catch-and-log:

Path	Reached from
lib/Command/RematerialiseCalculationsCommand.php:253	occ, prints "save failed" and still exits SUCCESS
lib/BackgroundJob/DsarDpiaDetectionJob.php:263	cron, no actor forwarding
lib/Service/VocabularyImportService.php (4 sites)	SeedVocabularyRegister on upgrade, and ImportSkosCsvCommand
lib/Service/Configuration/ImportHandler.php:2887	config import, when its fallback admin cannot read the folder

SystemOperationContext::isActive() is now recognised here as it already is in MultiTenancyTrait::hasRbacPermission() and PermissionHandler::hasPermission(). It resolves the app's own principal and runs the same four checks against that principal's mount. The guard is not skipped and not widened: an anonymous request has no principal to resolve and still lands on the default-deny.

2. PATCH reached validation before the lock guard

A locked object refused PUT with 423 naming the holder, while PATCH and POST-patch went straight to the merge. On the rig they answered 500 for a valid payload (the service-layer guard threw an untyped \Exception into a hardcoded 500) and 400 for one that fails hard validation. Two doors to the same object, two different answers, and the wrong one sends the caller off to fix their payload.

All three doors now go through one lockRefusalResponse(), placed before the merge and before validation. It asks the single predicate ObjectEntity::isLockedBySomeoneElse() with run identity arriving ambiently through FlowRunContext::currentRunUuid() (#3454) — the condition is not restated. The refusal is built once by LockedException::forObject(), which the service-layer guard now throws too, so a lock taken between the pre-read and the save answers 423 rather than 500.

3. The folder-binding window is not a race, and not intended

It is an identity defect. getOpenRegisterUserFolder() resolves the session user, so an object folder is created under Open Registers/ in whichever home the creating identity happened to have, and the compensating share back to everyone else is still a TODO that shares nothing. Measured on the rig: alice's case folder is /alice/files/Open Registers/Rig Probe Register/<uuid>, and one created under occ is /openregister/files/....

So a stored binding sits in exactly one user's mount, and re-validating it on every save asked every later editor to prove they were the creator. That is the reported 403: a non-admin cannot write a case somebody else created.

Re-validation of a binding the app itself wrote now asks the question that fits it — is this a folder OpenRegister manages? — through the new assertManagedFolderIsAccessible(). The caller-supplied @self.folder gate is untouched and still default-denies. A binding pointing outside Open Registers/ is still refused, which is exactly the planted cross-tenant binding the re-validation (#1431) was added for.

The managed test runs first, because assertFolderIsAccessible() writes a folder_access_denied audit row before every throw: asking it first would stamp a denial into the forensic record for every save that is then allowed. Verified on the rig — two accepted saves left the audit count unchanged at 10, two genuine refusals took it to 12.

The mount lookup uses IUserMountCache::getMountsForFileId(), not IRootFolder::getById(). The latter needs the owning user's mounts to be set up and answered 0 on the rig for a folder that demonstrably exists.

Tests

Proven red first, by neutralising each guard in turn and confirming the failure names the right thing. Each keeps a control that stays green, so a blanket exemption cannot pass as a fix.

• FolderManagementHandlerSystemContextTest (new, 7 tests) — a sessionless system context resolves the app principal and saves; the same context is still refused a folder its own principal cannot read; a userless request outside a system scope is still denied; a managed binding admits a user who did not create it; a binding outside the managed tree is still refused; an accepted save writes no denial audit row; the creator still passes when the mount cache is silent.
• ObjectsControllerTest::testAllThreeWriteDoorsRefuseALockedObjectIdentically — compares the whole response (status and body together) across PUT, PATCH and POST-patch, so a divergence in either half fails rather than being averaged away, then asserts the shared refusal names the holder. Red without the fix: PATCH returned 200 with an empty body, having gone straight through to the unstubbed saveObject.

Verified locally

CI is bottlenecked, so this was verified by exit code on a throwaway rig — its own compose project on port 8749, never :8080. Torn down with down -v.

Check	Exit
phpunit (19,217 tests, 46,995 assertions)	0
composer phpcs	0
psalm	0
composer phpstan	0
phpmd on each changed file, both rulesets	0
hydra-gates v1.15.1, full tree	0 — all 78 applicable gates ran and passed

phpcs was canary-tested rather than trusted: a deliberate violation planted in a changed file made it exit 1 and name that file, so the clean run is a real clean run.

Rig evidence, before → after:

Case	Before	After
occ save of a folder-bound object in a system context	FolderAccessDeniedException	SAVE OK
PUT on a locked object	423 naming the holder	423 naming the holder
PATCH / POST-patch on a locked object	500, lock unmentioned	423 naming the holder
The lock holder's own PATCH	200	200
bob writes a case alice created	403 folder_access_denied	200
bob writes a binding planted outside Open Registers/	403	403
Audit rows written by an accepted save	n/a	none

composer.lock carries a within-caret conduction/hydra-gates 1.15.0 → 1.15.1 bump, taken before running the gates so a stale vendored package could not invent failures or hide one.

Not fixed here, and deliberately

getOpenRegisterUserFolder() returning the session user is the root cause of defect 3, and moving object folders into the app's own home is the real repair. It is not in this PR: it relocates files out of users' Files trees while the share-back is still a TODO stub, which is a visible change to make on its own evidence rather than alongside a lock fix. The guard change above makes the symptom correct today without moving a single file.

🤖 Generated with Claude Code

[github-actions]

Quality Report — ConductionNL/openregister @ a514117

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-specs		✅
test-l10n		✅
test-l10n-parity		✅
format		✅
check-schema-l10n		✅
check-l10n-js		✅
composer			✅	✅ 174/174
npm			✅	✅ 543/543
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					❌
Newman					✅
Playwright					⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates					✅

---

Quality workflow — 2026-09-05 17:22 UTC

Download the full PDF report from the workflow artifacts.