From 1678f59701671be5bdf2cce9f06c7c3983c1c06f Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Wed, 5 Aug 2026 16:24:24 +0200 Subject: [PATCH 1/2] fix(security): give the two immutability guards a call site that actually runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both of procest's immutability rules were enforced by nothing. REQ-SUB-007 — `BewijsstukService::assertMutable()` was implemented and unit tested with ZERO production callers (hydra gate-6, orphan-auth). An authorization check that is never invoked is identical to having no check at all (OWASP A01:2021). A bewijsstuk linked to a vaststelling could be edited or deleted freely. REQ-IC-8 — `ChecklistRunImmutabilityListener` was worse than orphaned. It was never referenced by any registrar, so it was never subscribed to any event and never ran; and it declared the POST-persist `ObjectUpdatedEvent`, which OpenRegister dispatches AFTER `updateObjectEntity()` has committed the row, with no surrounding transaction. Even had it been registered, throwing from there could not have undone the mutation it objected to. The fix, for both: subscribe to OpenRegister's PRE-persist, stoppable `ObjectUpdatingEvent` / `ObjectDeletingEvent`. `stopPropagation()` makes MagicMapper raise `HookStoppedException` before anything is written — the same mechanism `LocationBagValidationListener` already uses and documents. This is the reachable enforcement point because the frontend writes through OpenRegister's generic objects API (ADR-022), not through a procest route; there is no bewijsstuk route to guard. `BewijsstukImmutabilityListener` reads the STORED state (`getOldObject()` on update, the entity itself on delete), never the incoming payload — otherwise a caller could clear `immutable` in the same request that mutates the document and walk through the guard. There is a test for exactly that bypass. Proof, not assertion — each test was re-run with lib/ reverted: - revert A (assertMutable has no caller, i.e. the shipped state): the 3 rejection tests fail, the 4 positive controls still pass. - revert B (guard reads the caller payload instead of the stored row): the bypass test fails. - revert C (checklist listener restored to post-persist ObjectUpdatedEvent): the pre-persist rejection test fails. Clean tree: 52/52 green in tests/Unit/Listener. The subsidieverlening-keten spec note is updated to say which half of REQ-SUB-007 now runs and which half still does not; the spec stays `partial` (verifyHash, the archief-trigger and the Docudesk PDF/A handover are still unwired, per the 2026-07-16 decision in procest#229). --- .../Registrar/ObjectListenerRegistrar.php | 42 +++ .../BewijsstukImmutabilityListener.php | 174 +++++++++++++ .../ChecklistRunImmutabilityListener.php | 46 ++-- .../specs/subsidieverlening-keten/spec.md | 13 +- tests/Stubs/Event/ObjectDeletingEventStub.php | 149 +++++++++++ .../BewijsstukImmutabilityListenerTest.php | 239 ++++++++++++++++++ .../ChecklistRunImmutabilityListenerTest.php | 182 +++++++++++++ tests/bootstrap.php | 6 + 8 files changed, 830 insertions(+), 21 deletions(-) create mode 100644 lib/Listener/BewijsstukImmutabilityListener.php create mode 100644 tests/Stubs/Event/ObjectDeletingEventStub.php create mode 100644 tests/Unit/Listener/BewijsstukImmutabilityListenerTest.php create mode 100644 tests/Unit/Listener/ChecklistRunImmutabilityListenerTest.php diff --git a/lib/AppInfo/Registrar/ObjectListenerRegistrar.php b/lib/AppInfo/Registrar/ObjectListenerRegistrar.php index 624376def..759542471 100644 --- a/lib/AppInfo/Registrar/ObjectListenerRegistrar.php +++ b/lib/AppInfo/Registrar/ObjectListenerRegistrar.php @@ -32,8 +32,11 @@ use OCA\OpenRegister\Event\ObjectCreatedEvent; use OCA\OpenRegister\Event\ObjectCreatingEvent; use OCA\OpenRegister\Event\ObjectDeletedEvent; +use OCA\OpenRegister\Event\ObjectDeletingEvent; use OCA\OpenRegister\Event\ObjectUpdatedEvent; use OCA\OpenRegister\Event\ObjectUpdatingEvent; +use OCA\Procest\Listener\BewijsstukImmutabilityListener; +use OCA\Procest\Listener\ChecklistRunImmutabilityListener; use OCA\Procest\Listener\KpiCacheInvalidationListener; use OCA\Procest\Listener\LocationBagValidationListener; use OCA\Procest\Listener\RoleMutationListener; @@ -68,8 +71,47 @@ public function register(IRegistrationContext $context): void $this->registerCacheInvalidationListeners(context: $context); $this->registerIntakeListeners(context: $context); + $this->registerImmutabilityListeners(context: $context); }//end register() + /** + * Register the pre-persist immutability guards. + * + * Both listeners subscribe to OpenRegister's PRE-persist, stoppable + * events. The post-persist pair cannot be used: OpenRegister dispatches + * `ObjectUpdatedEvent`/`ObjectDeletedEvent` after the row has already + * been written, with no surrounding transaction, so a listener there + * cannot stop the mutation it objects to. + * + * @param IRegistrationContext $context The registration context. + * + * @return void + * + * @spec openspec/specs/subsidieverlening-keten/spec.md + */ + private function registerImmutabilityListeners(IRegistrationContext $context): void + { + // REQ-SUB-007: a bewijsstuk linked to a vaststelling is immutable. + // This is the production call site for + // BewijsstukService::assertMutable(), which previously had none. + $context->registerEventListener( + event: ObjectUpdatingEvent::class, + listener: BewijsstukImmutabilityListener::class + ); + $context->registerEventListener( + event: ObjectDeletingEvent::class, + listener: BewijsstukImmutabilityListener::class + ); + + // REQ-IC-8: a submitted inspectionChecklistRun is append-only. The + // listener existed but was never registered, so the rule was not + // enforced by anything. + $context->registerEventListener( + event: ObjectUpdatingEvent::class, + listener: ChecklistRunImmutabilityListener::class + ); + }//end registerImmutabilityListeners() + /** * Register the KPI and role-routing cache-invalidation listeners. * diff --git a/lib/Listener/BewijsstukImmutabilityListener.php b/lib/Listener/BewijsstukImmutabilityListener.php new file mode 100644 index 000000000..e9851bc34 --- /dev/null +++ b/lib/Listener/BewijsstukImmutabilityListener.php @@ -0,0 +1,174 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * @version GIT: + * + * @link https://procest.nl + * + * @spec openspec/specs/subsidieverlening-keten/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Procest\Listener; + +use OCA\OpenRegister\Db\ObjectEntity; +use OCA\OpenRegister\Event\ObjectDeletingEvent; +use OCA\OpenRegister\Event\ObjectUpdatingEvent; +use OCA\Procest\Service\SettingsService; +use OCA\Procest\Service\Subsidie\BewijsstukService; +use OCP\AppFramework\OCS\OCSBadRequestException; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventListener; +use Psr\Log\LoggerInterface; +use Throwable; + +/** + * Reject mutation/deletion of a bewijsstuk linked to a vaststelling. + * + * @implements IEventListener + * + * @spec openspec/specs/subsidieverlening-keten/spec.md + */ +class BewijsstukImmutabilityListener implements IEventListener +{ + /** + * Constructor. + * + * @param SettingsService $settingsService Schema slug bridge. + * @param BewijsstukService $bewijsstukService Owns the REQ-SUB-007 + * immutability rule. + * @param LoggerInterface $logger Structured logger. + */ + public function __construct( + private readonly SettingsService $settingsService, + private readonly BewijsstukService $bewijsstukService, + private readonly LoggerInterface $logger, + ) { + }//end __construct() + + /** + * Inspect a pre-persist bewijsstuk mutation and reject it when frozen. + * + * @param Event $event The dispatched event. + * + * @return void + * + * @spec openspec/specs/subsidieverlening-keten/spec.md + */ + public function handle(Event $event): void + { + if ($event instanceof ObjectUpdatingEvent === true) { + // The STORED state decides, not the incoming payload. + $this->inspect(event: $event, stored: $event->getOldObject()); + return; + } + + if ($event instanceof ObjectDeletingEvent === true) { + $this->inspect(event: $event, stored: $event->getObject()); + return; + } + }//end handle() + + /** + * Apply `BewijsstukService::assertMutable()` to the stored state and stop + * the save when it rejects. + * + * @param ObjectUpdatingEvent|ObjectDeletingEvent $event The pre-persist, + * stoppable event. + * @param ObjectEntity|null $stored The state + * currently in the + * database. + * + * @return void + */ + private function inspect(ObjectUpdatingEvent|ObjectDeletingEvent $event, ?ObjectEntity $stored): void + { + if ($stored === null) { + return; + } + + try { + $payload = $stored->jsonSerialize(); + } catch (Throwable $e) { + $this->logger->debug( + 'Procest: bewijsstuk immutability listener could not read the stored payload: '.$e->getMessage() + ); + return; + } + + if (is_array($payload) === false || $this->isBewijsstukSchema(object: $payload) === false) { + return; + } + + try { + $this->bewijsstukService->assertMutable(bewijsstuk: $payload); + } catch (OCSBadRequestException $rejection) { + $event->setErrors( + [ + 'message' => $rejection->getMessage(), + 'code' => 'bewijsstuk.immutable', + ] + ); + $event->stopPropagation(); + $this->logger->info( + 'Procest: rejected a mutation on an immutable bewijsstuk (REQ-SUB-007)', + ['uuid' => (string) $stored->getUuid()] + ); + } + }//end inspect() + + /** + * Whether the supplied payload belongs to the `bewijsstuk` schema. + * + * @param array $object Object payload (incl. `@self`). + * + * @return bool True when this is a bewijsstuk. + */ + private function isBewijsstukSchema(array $object): bool + { + $expected = $this->settingsService->getConfigValue('bewijsstuk_schema'); + if ($expected === '') { + return false; + } + + $candidate = (string) ($object['@self']['schema'] ?? ($object['schema'] ?? '')); + + return $candidate !== '' && ( + $candidate === $expected + || str_ends_with($candidate, '/'.$expected) + ); + }//end isBewijsstukSchema() +}//end class diff --git a/lib/Listener/ChecklistRunImmutabilityListener.php b/lib/Listener/ChecklistRunImmutabilityListener.php index d5b6be66d..b30d87a77 100644 --- a/lib/Listener/ChecklistRunImmutabilityListener.php +++ b/lib/Listener/ChecklistRunImmutabilityListener.php @@ -5,13 +5,23 @@ * * Enforces REQ-IC-8: once a `inspectionChecklistRun` reaches * status = ingediend (or gearchiveerd), the object becomes append-only. - * Any UPDATE that mutates protected fields after submit is rejected with - * a RuntimeException whose message ("Checklist run is append-only") is the - * canonical spec error string surfaced via REQ-IC-4 / REQ-IC-8 scenarios. + * Any UPDATE that mutates protected fields after submit is rejected, with + * the canonical spec error string ("Checklist run is append-only") + * surfaced via REQ-IC-4 / REQ-IC-8 scenarios. * - * The listener never blocks the initial create (ObjectCreatedEvent) and - * lets a status transition from `in_uitvoering → ingediend` through; only - * subsequent edits to a submitted run trigger the rejection. + * The listener never blocks the initial create and lets a status transition + * from `in_uitvoering → ingediend` through; only subsequent edits to a + * submitted run trigger the rejection. + * + * It hooks OpenRegister's PRE-persist `ObjectUpdatingEvent`, which + * implements `StoppableEventInterface`: `stopPropagation()` makes + * MagicMapper raise `HookStoppedException` BEFORE the row is written. The + * post-persist `ObjectUpdatedEvent` this listener previously declared is + * dispatched AFTER `updateObjectEntity()` has already committed the row and + * OpenRegister opens no transaction around it, so throwing from there could + * not undo anything — the mutation landed and the caller merely saw an + * error. That, combined with the class never having been registered in + * `ObjectListenerRegistrar`, meant REQ-IC-8 was not enforced at all. * * @category Listener * @package OCA\Procest\Listener @@ -32,12 +42,11 @@ namespace OCA\Procest\Listener; -use OCA\OpenRegister\Event\ObjectUpdatedEvent; +use OCA\OpenRegister\Event\ObjectUpdatingEvent; use OCA\Procest\Service\SettingsService; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use Psr\Log\LoggerInterface; -use RuntimeException; use Throwable; /** @@ -69,19 +78,18 @@ public function __construct( }//end __construct() /** - * Inspect ObjectUpdatedEvent and reject illegal mutations. + * Inspect ObjectUpdatingEvent and reject illegal mutations before the + * row is written. * * @param Event $event The dispatched event * * @return void * - * @throws RuntimeException When a submitted run is being mutated. - * @spec openspec/specs/inspection-checklists/spec.md */ public function handle(Event $event): void { - if ($event instanceof ObjectUpdatedEvent === false) { + if ($event instanceof ObjectUpdatingEvent === false) { return; } @@ -89,16 +97,20 @@ public function handle(Event $event): void if ($this->isFrozenRunMutation(event: $event) === false) { return; } - - throw new RuntimeException('Checklist run is append-only'); - } catch (RuntimeException $rejection) { - // Re-throw rejection so OpenRegister surfaces it to the caller. - throw $rejection; } catch (Throwable $e) { $this->logger->debug( 'Procest: checklist immutability listener swallowed exception: '.$e->getMessage(), ); + return; }//end try + + $event->setErrors( + [ + 'message' => 'Checklist run is append-only', + 'code' => 'inspectionChecklistRun.appendOnly', + ] + ); + $event->stopPropagation(); }//end handle() /** diff --git a/openspec/specs/subsidieverlening-keten/spec.md b/openspec/specs/subsidieverlening-keten/spec.md index a8c8d56cc..2f30076b5 100644 --- a/openspec/specs/subsidieverlening-keten/spec.md +++ b/openspec/specs/subsidieverlening-keten/spec.md @@ -7,10 +7,15 @@ status-note: | run. Their capability methods are implemented and unit-tested yet have ZERO callers, so no user or API path can reach them: - - REQ-SUB-007 (bewijsstukken): `BewijsstukService::verifyHash()` and - `::assertMutable()` are never invoked — no hash is verified on read, and no - document is locked once linked to a vaststelling. There is no bewijsstuk - route, no nightly archief-trigger, and no Docudesk PDF/A handover. + - REQ-SUB-007 (bewijsstukken): PARTIALLY CLOSED on 2026-08-05. The + immutability half now runs: `BewijsstukService::assertMutable()` is invoked + from `BewijsstukImmutabilityListener`, which subscribes to OpenRegister's + PRE-persist `ObjectUpdatingEvent`/`ObjectDeletingEvent` and stops the save, + so a bewijsstuk linked to a vaststelling can no longer be edited or deleted + through the generic object write path the frontend uses (ADR-022). The rest + of the requirement is STILL not running: `::verifyHash()` has no caller so + no hash is verified on read, and there is no bewijsstuk route, no nightly + archief-trigger and no Docudesk PDF/A handover. - REQ-SUB-008 (staatssteun): `StaatssteunClassifier::requiresStaatssteunGrondslag()` is never invoked — no de-minimis gate runs on assessment, and no AGVV/TAM melding is emitted. diff --git a/tests/Stubs/Event/ObjectDeletingEventStub.php b/tests/Stubs/Event/ObjectDeletingEventStub.php new file mode 100644 index 000000000..a3e3ff5e5 --- /dev/null +++ b/tests/Stubs/Event/ObjectDeletingEventStub.php @@ -0,0 +1,149 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * @link https://procest.nl + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Event; + +use OCA\OpenRegister\Db\ObjectEntity; +use OCP\EventDispatcher\Event; +use Psr\EventDispatcher\StoppableEventInterface; + +if (class_exists(ObjectDeletingEvent::class) === false) { + /** + * Stub class for ObjectDeletingEvent — used only in standalone unit + * tests. + */ + class ObjectDeletingEvent extends Event implements StoppableEventInterface + { + /** + * Whether event propagation has been stopped. + * + * @var bool + */ + private bool $propagationStopped = false; + + /** + * Errors set by a hook that stopped propagation. + * + * @var array + */ + private array $errors = []; + + /** + * Modified data set by a hook. + * + * @var array + */ + private array $modifiedData = []; + + /** + * Constructor. + * + * @param ObjectEntity $object The entity being deleted. + */ + public function __construct( + private readonly ObjectEntity $object, + ) { + parent::__construct(); + }//end __construct() + + /** + * Get the entity being deleted. + * + * @return ObjectEntity + */ + public function getObject(): ObjectEntity + { + return $this->object; + }//end getObject() + + /** + * Whether propagation has been stopped by a hook. + * + * @return bool + */ + public function isPropagationStopped(): bool + { + return $this->propagationStopped; + }//end isPropagationStopped() + + /** + * Stop event propagation (used by hooks to reject the deletion). + * + * @return void + */ + public function stopPropagation(): void + { + $this->propagationStopped = true; + }//end stopPropagation() + + /** + * Set errors from a hook. + * + * @param array $errors Error details + * + * @return void + */ + public function setErrors(array $errors): void + { + $this->errors = $errors; + }//end setErrors() + + /** + * Get errors set by a hook. + * + * @return array + */ + public function getErrors(): array + { + return $this->errors; + }//end getErrors() + + /** + * Set modified data from a hook. + * + * @param array $data Modified data + * + * @return void + */ + public function setModifiedData(array $data): void + { + $this->modifiedData = $data; + }//end setModifiedData() + + /** + * Get modified data set by a hook. + * + * @return array + */ + public function getModifiedData(): array + { + return $this->modifiedData; + }//end getModifiedData() + }//end class +}//end if diff --git a/tests/Unit/Listener/BewijsstukImmutabilityListenerTest.php b/tests/Unit/Listener/BewijsstukImmutabilityListenerTest.php new file mode 100644 index 000000000..f58f1ee7f --- /dev/null +++ b/tests/Unit/Listener/BewijsstukImmutabilityListenerTest.php @@ -0,0 +1,239 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @spec openspec/specs/subsidieverlening-keten/spec.md + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Procest\Tests\Unit\Listener; + +use OCA\OpenRegister\Db\ObjectEntity; +use OCA\OpenRegister\Event\ObjectDeletingEvent; +use OCA\OpenRegister\Event\ObjectUpdatingEvent; +use OCA\Procest\Listener\BewijsstukImmutabilityListener; +use OCA\Procest\Service\SettingsService; +use OCA\Procest\Service\Subsidie\BewijsstukService; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; + +/** + * @covers \OCA\Procest\Listener\BewijsstukImmutabilityListener + * + * @uses \OCA\Procest\Service\Subsidie\BewijsstukService + */ +class BewijsstukImmutabilityListenerTest extends TestCase +{ + /** + * Schema id the listener is configured to recognise. + */ + private const SCHEMA = 'bewijsstuk-schema-id'; + + /** + * The listener under test. + * + * @var BewijsstukImmutabilityListener + */ + private BewijsstukImmutabilityListener $listener; + + /** + * Set up the listener with the REAL BewijsstukService, so the test + * exercises the actual assertMutable() rule and not a mock of it. + * + * @return void + */ + protected function setUp(): void + { + $settingsService = $this->createMock(SettingsService::class); + $settingsService->method('getConfigValue')->willReturnCallback( + static function (string $key, string $default=''): string { + return $key === 'bewijsstuk_schema' ? self::SCHEMA : $default; + } + ); + + $logger = $this->createMock(LoggerInterface::class); + + $this->listener = new BewijsstukImmutabilityListener( + $settingsService, + new BewijsstukService($this->createMock(SettingsService::class), $logger), + $logger, + ); + }//end setUp() + + /** + * Build a bewijsstuk entity. + * + * @param array $payload Bewijsstuk fields. + * @param string $schemaId Schema id (`@self.schema`). + * + * @return ObjectEntity + */ + private function entity(array $payload, string $schemaId=self::SCHEMA): ObjectEntity + { + $entity = new ObjectEntity(); + $entity->setObject($payload); + $entity->setSchemaId($schemaId); + $entity->setUuid('22222222-2222-2222-2222-222222222222'); + + return $entity; + }//end entity() + + /** + * An update to a bewijsstuk that is NOT linked to a vaststelling is + * allowed through — the positive control for every reject case below. + * + * @return void + */ + public function testMutableBewijsstukUpdateIsAllowed(): void + { + $event = new ObjectUpdatingEvent( + $this->entity(['immutable' => false, 'bewijsstukType' => 'factuur']), + $this->entity(['immutable' => false, 'bewijsstukType' => 'urenstaat']) + ); + + $this->listener->handle($event); + + $this->assertFalse( + $event->isPropagationStopped(), + 'A mutable bewijsstuk must remain editable' + ); + $this->assertSame([], $event->getErrors()); + }//end testMutableBewijsstukUpdateIsAllowed() + + /** + * An update to a vaststelling-linked bewijsstuk is rejected BEFORE the + * row is written (stopPropagation on the pre-persist event). + * + * @return void + */ + public function testImmutableBewijsstukUpdateIsRejected(): void + { + $event = new ObjectUpdatingEvent( + $this->entity(['immutable' => true, 'bewijsstukType' => 'factuur']), + $this->entity(['immutable' => true, 'bewijsstukType' => 'urenstaat']) + ); + + $this->listener->handle($event); + + $this->assertTrue( + $event->isPropagationStopped(), + 'An immutable bewijsstuk update must be stopped pre-persist' + ); + $this->assertSame('bewijsstuk.immutable', $event->getErrors()['code'] ?? null); + $this->assertStringContainsString('onveranderlijk', (string) ($event->getErrors()['message'] ?? '')); + }//end testImmutableBewijsstukUpdateIsRejected() + + /** + * The STORED state decides, not the incoming payload: clearing + * `immutable` in the same request that mutates the document must NOT + * unlock it. Without this the guard is trivially bypassable. + * + * @return void + */ + public function testPayloadCannotClearTheImmutableFlagToBypassTheGuard(): void + { + $event = new ObjectUpdatingEvent( + // Attacker-supplied new state claims the document is mutable. + $this->entity(['immutable' => false, 'bewijsstukType' => 'factuur']), + // Stored state says otherwise. + $this->entity(['immutable' => true, 'bewijsstukType' => 'urenstaat']) + ); + + $this->listener->handle($event); + + $this->assertTrue( + $event->isPropagationStopped(), + 'The guard must read the stored state, not the incoming payload' + ); + }//end testPayloadCannotClearTheImmutableFlagToBypassTheGuard() + + /** + * Deleting a vaststelling-linked bewijsstuk is rejected too — an + * immutability rule that only covers UPDATE is bypassable by + * delete-and-recreate. + * + * @return void + */ + public function testImmutableBewijsstukDeleteIsRejected(): void + { + $event = new ObjectDeletingEvent($this->entity(['immutable' => true])); + + $this->listener->handle($event); + + $this->assertTrue($event->isPropagationStopped()); + $this->assertSame('bewijsstuk.immutable', $event->getErrors()['code'] ?? null); + }//end testImmutableBewijsstukDeleteIsRejected() + + /** + * Deleting a bewijsstuk that is not linked to a vaststelling is allowed. + * + * @return void + */ + public function testMutableBewijsstukDeleteIsAllowed(): void + { + $event = new ObjectDeletingEvent($this->entity(['immutable' => false])); + + $this->listener->handle($event); + + $this->assertFalse($event->isPropagationStopped()); + }//end testMutableBewijsstukDeleteIsAllowed() + + /** + * Objects of another schema are untouched — the listener must not + * freeze unrelated registers just because they carry an `immutable` + * field. + * + * @return void + */ + public function testForeignSchemaIsIgnored(): void + { + $event = new ObjectUpdatingEvent( + $this->entity(['immutable' => true], 'some-other-schema'), + $this->entity(['immutable' => true], 'some-other-schema') + ); + + $this->listener->handle($event); + + $this->assertFalse( + $event->isPropagationStopped(), + 'Only the bewijsstuk schema is subject to REQ-SUB-007' + ); + }//end testForeignSchemaIsIgnored() + + /** + * A create (no stored state) is never blocked: `getOldObject()` is null + * on first write. + * + * @return void + */ + public function testMissingStoredStateIsAllowed(): void + { + $event = new ObjectUpdatingEvent($this->entity(['immutable' => true]), null); + + $this->listener->handle($event); + + $this->assertFalse($event->isPropagationStopped()); + }//end testMissingStoredStateIsAllowed() +}//end class diff --git a/tests/Unit/Listener/ChecklistRunImmutabilityListenerTest.php b/tests/Unit/Listener/ChecklistRunImmutabilityListenerTest.php new file mode 100644 index 000000000..d476ff26f --- /dev/null +++ b/tests/Unit/Listener/ChecklistRunImmutabilityListenerTest.php @@ -0,0 +1,182 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @spec openspec/specs/inspection-checklists/spec.md + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Procest\Tests\Unit\Listener; + +use OCA\OpenRegister\Db\ObjectEntity; +use OCA\OpenRegister\Event\ObjectUpdatingEvent; +use OCA\Procest\Listener\ChecklistRunImmutabilityListener; +use OCA\Procest\Service\SettingsService; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; + +/** + * @covers \OCA\Procest\Listener\ChecklistRunImmutabilityListener + */ +class ChecklistRunImmutabilityListenerTest extends TestCase +{ + /** + * Schema id the listener is configured to recognise. + */ + private const SCHEMA = 'checklist-run-schema-id'; + + /** + * The listener under test. + * + * @var ChecklistRunImmutabilityListener + */ + private ChecklistRunImmutabilityListener $listener; + + /** + * Set up the listener. + * + * @return void + */ + protected function setUp(): void + { + $settingsService = $this->createMock(SettingsService::class); + $settingsService->method('getConfigValue')->willReturnCallback( + static function (string $key, string $default=''): string { + return $key === 'inspection_checklist_run_schema' ? self::SCHEMA : $default; + } + ); + + $this->listener = new ChecklistRunImmutabilityListener( + $settingsService, + $this->createMock(LoggerInterface::class), + ); + }//end setUp() + + /** + * Build a checklist-run entity. + * + * @param array $payload Run fields. + * @param string $schemaId Schema id. + * + * @return ObjectEntity + */ + private function entity(array $payload, string $schemaId=self::SCHEMA): ObjectEntity + { + $entity = new ObjectEntity(); + $entity->setObject($payload); + $entity->setSchemaId($schemaId); + $entity->setUuid('33333333-3333-3333-3333-333333333333'); + + return $entity; + }//end entity() + + /** + * A run still in progress may be edited freely — positive control. + * + * @return void + */ + public function testRunInProgressMayBeEdited(): void + { + $event = new ObjectUpdatingEvent( + $this->entity(['status' => 'in_uitvoering', 'responses' => ['b']]), + $this->entity(['status' => 'in_uitvoering', 'responses' => ['a']]) + ); + + $this->listener->handle($event); + + $this->assertFalse($event->isPropagationStopped()); + }//end testRunInProgressMayBeEdited() + + /** + * The first submit (`in_uitvoering → ingediend`) is allowed through. + * + * @return void + */ + public function testFirstSubmitIsAllowed(): void + { + $event = new ObjectUpdatingEvent( + $this->entity(['status' => 'ingediend', 'responses' => ['a']]), + $this->entity(['status' => 'in_uitvoering', 'responses' => ['a']]) + ); + + $this->listener->handle($event); + + $this->assertFalse($event->isPropagationStopped()); + }//end testFirstSubmitIsAllowed() + + /** + * Editing a protected field on a submitted run is rejected BEFORE the row + * is written. + * + * @return void + */ + public function testEditingASubmittedRunIsRejectedPrePersist(): void + { + $event = new ObjectUpdatingEvent( + $this->entity(['status' => 'ingediend', 'responses' => ['tampered']]), + $this->entity(['status' => 'ingediend', 'responses' => ['original']]) + ); + + $this->listener->handle($event); + + $this->assertTrue( + $event->isPropagationStopped(), + 'A submitted checklist run must be append-only' + ); + $this->assertSame('Checklist run is append-only', $event->getErrors()['message'] ?? null); + }//end testEditingASubmittedRunIsRejectedPrePersist() + + /** + * A metadata-only refresh of a submitted run is not a material change and + * is allowed. + * + * @return void + */ + public function testMetadataOnlyRefreshOfASubmittedRunIsAllowed(): void + { + $event = new ObjectUpdatingEvent( + $this->entity(['status' => 'ingediend', 'responses' => ['a'], 'updatedAt' => '2026-08-05']), + $this->entity(['status' => 'ingediend', 'responses' => ['a'], 'updatedAt' => '2026-08-04']) + ); + + $this->listener->handle($event); + + $this->assertFalse($event->isPropagationStopped()); + }//end testMetadataOnlyRefreshOfASubmittedRunIsAllowed() + + /** + * Another schema's objects are untouched. + * + * @return void + */ + public function testForeignSchemaIsIgnored(): void + { + $event = new ObjectUpdatingEvent( + $this->entity(['status' => 'ingediend', 'responses' => ['x']], 'other-schema'), + $this->entity(['status' => 'ingediend', 'responses' => ['y']], 'other-schema') + ); + + $this->listener->handle($event); + + $this->assertFalse($event->isPropagationStopped()); + }//end testForeignSchemaIsIgnored() +}//end class diff --git a/tests/bootstrap.php b/tests/bootstrap.php index d5f70cece..8b6a10aec 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -290,6 +290,12 @@ static function (string $class): bool { include_once __DIR__.'/Stubs/Event/ObjectUpdatingEventStub.php'; } +// REQ-SUB-007 bewijsstuk immutability: the pre-persist delete counterpart, so +// BewijsstukImmutabilityListenerTest can exercise the reject path on delete. +if (class_exists('\\OCA\\OpenRegister\\Event\\ObjectDeletingEvent') === false) { + include_once __DIR__.'/Stubs/Event/ObjectDeletingEventStub.php'; +} + // OpenRegister AppHost stubs (ADR-040) — loaded when the openregister runtime // is absent so Application::register() (Bootstrap::register) and procest's // DashboardController (extends GenericDashboardController) resolve in bare CI From c1653bdf2f26a75cbc553427e280cc5afaac7d2c Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Wed, 5 Aug 2026 16:49:48 +0200 Subject: [PATCH 2/2] fix(quality): satisfy phpmd coupling and phpstan on the immutability guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two failures my previous commit introduced — phpstan and phpmd were both green on development before it, so these are mine, not pre-existing. phpmd CouplingBetweenObjects: the three new imports pushed `ObjectListenerRegistrar` to 14 dependencies against a limit of 13. Rather than raise the threshold, the immutability registrations move into their own `ImmutabilityListenerRegistrar`, which is what that class's own docblock says should happen ("Subsystem-scoped listeners live in their own registrars") and is the same shape as the bezwaar and workflow registrars. phpstan: `is_array($payload) === false` is always false — `jsonSerialize()` is declared `array`, so the guard was dead code. Removed rather than annotated. No suppression, no threshold change, no baseline entry. Revert control A re-run after the rework: with `assertMutable()`'s call removed the 3 rejection tests still fail and the 4 positive controls still pass. Clean tree: 52/52 in tests/Unit/Listener, gate-6 clean, phpcs/phpmd/phpstan clean on every changed file. --- .../ImmutabilityListenerRegistrar.php | 86 +++++++++++++++++++ lib/AppInfo/Registrar/ListenerRegistrar.php | 1 + .../Registrar/ObjectListenerRegistrar.php | 42 --------- .../BewijsstukImmutabilityListener.php | 2 +- 4 files changed, 88 insertions(+), 43 deletions(-) create mode 100644 lib/AppInfo/Registrar/ImmutabilityListenerRegistrar.php diff --git a/lib/AppInfo/Registrar/ImmutabilityListenerRegistrar.php b/lib/AppInfo/Registrar/ImmutabilityListenerRegistrar.php new file mode 100644 index 000000000..094851b54 --- /dev/null +++ b/lib/AppInfo/Registrar/ImmutabilityListenerRegistrar.php @@ -0,0 +1,86 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://procest.nl + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * @spec openspec/specs/subsidieverlening-keten/spec.md + */ + +declare(strict_types=1); + +namespace OCA\Procest\AppInfo\Registrar; + +use OCA\OpenRegister\Event\ObjectDeletingEvent; +use OCA\OpenRegister\Event\ObjectUpdatingEvent; +use OCA\Procest\Listener\BewijsstukImmutabilityListener; +use OCA\Procest\Listener\ChecklistRunImmutabilityListener; +use OCP\AppFramework\Bootstrap\IRegistrationContext; + +/** + * Registers the pre-persist immutability guards. + * + * @psalm-suppress UnusedClass + * + * @spec openspec/specs/subsidieverlening-keten/spec.md + */ +class ImmutabilityListenerRegistrar +{ + /** + * Register the immutability listeners. + * + * @param IRegistrationContext $context The registration context. + * + * @return void + * + * @spec openspec/specs/subsidieverlening-keten/spec.md + */ + public function register(IRegistrationContext $context): void + { + // REQ-SUB-007: a bewijsstuk linked to a vaststelling is immutable. + // This is the production call site for + // BewijsstukService::assertMutable(), which previously had none. + $context->registerEventListener( + event: ObjectUpdatingEvent::class, + listener: BewijsstukImmutabilityListener::class + ); + $context->registerEventListener( + event: ObjectDeletingEvent::class, + listener: BewijsstukImmutabilityListener::class + ); + + // REQ-IC-8: a submitted inspectionChecklistRun is append-only. The + // listener existed but was never referenced by any registrar, so the + // rule was not enforced by anything. + $context->registerEventListener( + event: ObjectUpdatingEvent::class, + listener: ChecklistRunImmutabilityListener::class + ); + }//end register() +}//end class diff --git a/lib/AppInfo/Registrar/ListenerRegistrar.php b/lib/AppInfo/Registrar/ListenerRegistrar.php index 01f4e1055..88bebbf28 100644 --- a/lib/AppInfo/Registrar/ListenerRegistrar.php +++ b/lib/AppInfo/Registrar/ListenerRegistrar.php @@ -58,6 +58,7 @@ class ListenerRegistrar public function register(IRegistrationContext $context): void { (new ObjectListenerRegistrar())->register(context: $context); + (new ImmutabilityListenerRegistrar())->register(context: $context); (new BezwaarListenerRegistrar())->register(context: $context); (new WorkflowListenerRegistrar())->register(context: $context); }//end register() diff --git a/lib/AppInfo/Registrar/ObjectListenerRegistrar.php b/lib/AppInfo/Registrar/ObjectListenerRegistrar.php index 759542471..624376def 100644 --- a/lib/AppInfo/Registrar/ObjectListenerRegistrar.php +++ b/lib/AppInfo/Registrar/ObjectListenerRegistrar.php @@ -32,11 +32,8 @@ use OCA\OpenRegister\Event\ObjectCreatedEvent; use OCA\OpenRegister\Event\ObjectCreatingEvent; use OCA\OpenRegister\Event\ObjectDeletedEvent; -use OCA\OpenRegister\Event\ObjectDeletingEvent; use OCA\OpenRegister\Event\ObjectUpdatedEvent; use OCA\OpenRegister\Event\ObjectUpdatingEvent; -use OCA\Procest\Listener\BewijsstukImmutabilityListener; -use OCA\Procest\Listener\ChecklistRunImmutabilityListener; use OCA\Procest\Listener\KpiCacheInvalidationListener; use OCA\Procest\Listener\LocationBagValidationListener; use OCA\Procest\Listener\RoleMutationListener; @@ -71,47 +68,8 @@ public function register(IRegistrationContext $context): void $this->registerCacheInvalidationListeners(context: $context); $this->registerIntakeListeners(context: $context); - $this->registerImmutabilityListeners(context: $context); }//end register() - /** - * Register the pre-persist immutability guards. - * - * Both listeners subscribe to OpenRegister's PRE-persist, stoppable - * events. The post-persist pair cannot be used: OpenRegister dispatches - * `ObjectUpdatedEvent`/`ObjectDeletedEvent` after the row has already - * been written, with no surrounding transaction, so a listener there - * cannot stop the mutation it objects to. - * - * @param IRegistrationContext $context The registration context. - * - * @return void - * - * @spec openspec/specs/subsidieverlening-keten/spec.md - */ - private function registerImmutabilityListeners(IRegistrationContext $context): void - { - // REQ-SUB-007: a bewijsstuk linked to a vaststelling is immutable. - // This is the production call site for - // BewijsstukService::assertMutable(), which previously had none. - $context->registerEventListener( - event: ObjectUpdatingEvent::class, - listener: BewijsstukImmutabilityListener::class - ); - $context->registerEventListener( - event: ObjectDeletingEvent::class, - listener: BewijsstukImmutabilityListener::class - ); - - // REQ-IC-8: a submitted inspectionChecklistRun is append-only. The - // listener existed but was never registered, so the rule was not - // enforced by anything. - $context->registerEventListener( - event: ObjectUpdatingEvent::class, - listener: ChecklistRunImmutabilityListener::class - ); - }//end registerImmutabilityListeners() - /** * Register the KPI and role-routing cache-invalidation listeners. * diff --git a/lib/Listener/BewijsstukImmutabilityListener.php b/lib/Listener/BewijsstukImmutabilityListener.php index e9851bc34..ba2d8e364 100644 --- a/lib/Listener/BewijsstukImmutabilityListener.php +++ b/lib/Listener/BewijsstukImmutabilityListener.php @@ -129,7 +129,7 @@ private function inspect(ObjectUpdatingEvent|ObjectDeletingEvent $event, ?Object return; } - if (is_array($payload) === false || $this->isBewijsstukSchema(object: $payload) === false) { + if ($this->isBewijsstukSchema(object: $payload) === false) { return; }