Implement entity-relation-grondslagen — decision-metadata PATCH endpoint (#1435)

[rjzondervan]

Implements the entity-relation-grondslagen change. Includes a spec rework committed before the implementation — see "Spec rework note" below.

Summary

Introduces an audited, decision-only PATCH endpoint for setting operator decisions on detected-entity occurrences:

• PATCH /api/entity-relations/{id} — body {bases?: ?array<string>, skipAnonymization?: bool}. Any other top-level key returns HTTP 400 with the offending field name. @NoAdminRequired; the acting user MUST have write-access to the relation's parent file.
• EntityRelationMapper::updateDecisionMetadata(EntityRelation, array, ?IUser) — single audited write path. Controller passes the loaded relation; mapper enforces the whitelist, validates field shapes, computes a diff, persists changes, and emits exactly one audit-trail entry (only when fields actually change — semantic no-ops produce no audit).
• Anonymise flow gains skip-awareness. markAsAnonymized adds AND skip_anonymization = 0 to its WHERE; FileTextController::anonymizeFile reads through the new findEntitiesForAnonymization; DocumentProcessingHandler::anonymizeDocument defensively filters skipped values server-side so the contract — "skipped relations are never redacted, full stop" — holds regardless of caller behaviour.

The post-hoc system fields anonymized / anonymizedValue are intentionally NOT in the whitelist; they're set only by markAsAnonymized to preserve the audit invariant "anonymized=true ⟹ the redaction code ran for this row".

Spec rework note

This branch's first commit (ef5464b94) is a substantive amendment to the spec that was approved as PR #1494. The original spec described "the anonymise endpoint accepts bases, persists, then strips before forwarding to OpenAnonymiser". Explore-mode investigation in apply uncovered two architectural mismatches:

1. OpenAnonymiser is called during detection (EntityRecognitionHandler::detectWithOpenAnonymiser), not at anonymise time. The "strip before forward" requirement had no counterpart in the code.
2. OR's anonymise endpoint takes a fileId and reads pre-detected relations from the DB; it never forwarded entity arrays anywhere external.

The rework decoupled bases-set from the anonymise call into the new PATCH endpoint, and added the skipAnonymization field on the same audited write path so operators can also opt-out per-occurrence. Both spec and design now describe the actual architecture.

The companion DocuDesk amendment lives on docudesk:feat/123/anonymisation-grondslagen-and-prohibition-gate-amend (local-only — to be pushed alongside this review).

Database

Column	Type	Default	Purpose
bases	JSON	NULL	UUIDs referencing legal grondslagen (consumer-owned vocabulary).
skip_anonymization	BOOLEAN	false	Operator opt-out from the next anonymise pass.

Migration is forward-only and idempotent (hasColumn guards). Existing rows pick up the defaults; no backfill.

Tests

33 unit tests, 133 assertions, all passing against master-nextcloud-1:

• tests/Unit/Db/EntityRelationTest.php — extended for the two new fields.
• tests/Unit/Db/EntityRelationMapperTest.php — construction wiring.
• tests/Unit/Db/EntityRelationMapperUpdateDecisionMetadataTest.php — whitelist, shape validation, semantic-no-op, diff-aware audit emission, audit-failure tolerance, actor resolution (explicit / session / system fallback).
• tests/Unit/Controller/EntityRelationsControllerTest.php — 401 / 403 / 404 / 400 / 200 / 500 HTTP contract.

$ docker exec -w /var/www/html/apps-extra/openregister master-nextcloud-1 \
    php vendor/bin/phpunit -c phpunit-unit.xml --filter "EntityRelation"
Tests: 33, Assertions: 133 — OK

Quality gates

• ✓ phpcs clean on all touched files (composer phpcs)
• ✓ php -l clean
• ✓ openspec validate entity-relation-grondslagen clean
• ✓ 33 / 33 unit tests passing
• ⏸ Live-stack manual smoke deferred to this PR's review phase

Test plan

• Review the spec-rework commit (ef5464b94) — confirm the decoupled-PATCH architecture matches expectations
• Review the implementation commit (139e1667d)
• Apply the migration on a dev install; PATCH a relation with {bases: ["uuid-a"]}, confirm the row is updated and one audit-trail entry exists
• PATCH a relation with {skipAnonymization: true}; run POST /api/files/:fileId/anonymize on the parent file; confirm the skipped row is NOT redacted and stays anonymized=false
• PATCH with {anonymized: true}; confirm HTTP 400 with field_not_editable error
• Confirm pre-change anonymise calls (no skip flags anywhere) behave identically to before
• Re-review docs/Features/entity-relation-decision-metadata.md for accuracy

Refs: #1435

[github-actions]

Quality Report — ConductionNL/openregister @ 554b045

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		❌
stylelint		✅
composer			✅	✅ 153/153
npm			✅	✅ 598/598
PHPUnit					❌
Newman					⏭️
Playwright					⏭️

---

Quality workflow — 2026-05-12 14:28 UTC

Download the full PDF report from the workflow artifacts.

[WilcoLouwerse]

REQUEST_CHANGES — Strict mode. 4 blockers (including 1 new from the most recent commit), 5 significant issues.

🔴 Blockers (4): Auth bypass for object/email-bound relations; @NoCSRFRequired on a state-mutating PATCH without justification; ADR-005 violation (getDisplayName() stored as audit actor instead of UID); Newman regression introduced by commit b8949d17 — Newman API Test Suite passed on 139e1667 and now fails with "socket hang up" on every request after the IEventDispatcher constructor parameter was added to EntityRelationMapper without a matching explicit registerService in Application.php.

🟡 Significant (5): Order-sensitive bases comparison causes spurious audit entries; getParams() content-type sensitivity; non-transactional audit write; fileId = 0 bypasses file auth; tasks.md item 3.2a left unchecked despite being implemented in this commit.

🟢 Minor (3): Missing @copyright on 3 new test files (gate-1); missing postSchemaChange() comment in migration; CHANGELOG.md documents the wrong method signature for updateDecisionMetadata.

CI note: PHPUnit failures on quality / PHPUnit (PHP 8.3/8.4, NC stable32) are pre-existing on the development branch. The Newman failure IS new — it regressed in this PR.

[rjzondervan]

Re: Newman regression attributed to b8949d17

Wanted to share reproduction results before applying the suggested registerService(EntityRelationMapper::class, …) fix in lib/AppInfo/Application.php, because I cannot reproduce the regression locally and the autowiring path appears healthy.

Tests I ran

1. Newman crud collection on this branch tip (80c2de5b9):

   • BASE_URL=http://localhost CONTAINER_NAME=master-nextcloud-1 COLLECTIONS="crud" bash tests/newman/run-all.sh
   • Result: 1 collection failed — assertion failures (HTTP 400 on a number of delete operations), no socket hang up, server kept responding.

2. Same Newman crud collection on development HEAD (9d50f4360 — before my branch diverged):

   • Result: identical set of HTTP 400 failures on the same delete operations.
   • Conclusion: the failures pre-exist b8949d17 and 80c2de5b9. They are not caused by this PR.

Direct evidence the IEventDispatcher autowiring works

Endpoint GET /apps/openregister/api/entities/stats is served by GdprEntitiesController, which constructor-injects EntityRelationMapper (the new signature with IEventDispatcher). Hit it from inside the NC container:

HTTP 200
{...,"byType":{"ORGANIZATION":57,"PERSON":53,"LOCATION":37,"DATE":17,"EMAIL":4,"IBAN":1,"PHONE":1},...}

That request constructs EntityRelationMapper through the DI container with all five deps. If the autowiring chain were broken we'd get a 500 here, not a 200 with a valid payload.

Additionally, sibling mappers (SchemaMapper, MagicMapper, ConfigurationMapper) all use IEventDispatcher via autowiring with no explicit registerService block — same pattern, no failures.

Ask

Could you share the exact failing collection / commit you ran and the failure mode (was it really "socket hang up"? or assertion errors like above?)? I want to make sure I'm not missing something CI-specific before signing off — but I don't want to add a speculative registerService block if the diagnosis is wrong, because that would obscure a different root cause.

Continuing with the other review items (ADR-005 audit actor → UID, PATCH authz, @NoCSRFRequired, the 5 significant items) in parallel.

[rjzondervan]

Review response — commit e34eac991

Addressed every flagged item except the Newman regression, which my earlier comment (#issuecomment-4460023069) showed doesn't reproduce.

Blockers

• ADR-005 audit actor. EntityRelationMapper::emitDecisionMetadataAuditEntry no longer persists $user->getDisplayName() on the audit-trail entry's userName column. The audit row records only the UID; the display name is dropped (set to null).
• Auth bypass on PATCH /api/entity-relations/{id}. Object- and email-bound relations no longer fall through with an accept-with-warning log. The controller now constructor-injects PermissionHandler, MagicMapper, SchemaMapper, RegisterMapper, and EmailLinkMapper. Object-bound relations route through PermissionHandler::hasPermission(schema, 'update', uid, owner, object). Email-bound relations resolve their parent ObjectEntity via EmailLink and run the same object-update verdict; email links without a parent object are denied. fileId = 0 / objectId = 0 / emailId = 0 no longer satisfy any branch. Added EmailLinkMapper::find(int $id) to support the email-bound lookup.
• @NoCSRFRequired. Removed from EntityRelationsController::update. Spec amended (design.md §D7) to record the decision — this is a browser-facing decision-time PATCH and Nextcloud's session-CSRF protection MUST stay on.
• Newman. Not addressed — see previous comment. The crud Newman collection fails identically on development HEAD (commit 9d50f4360) without any of my commits, with HTTP 400 assertion errors (not "socket hang up"). GET /api/entities/stats (which constructor-injects EntityRelationMapper) returns HTTP 200 locally — autowiring works. I'd push back on this blocker rather than apply a speculative registerService fix that might mask a different CI-environmental cause.

Significant items

• Order-sensitive bases diff → new private EntityRelationMapper::basesAreEqual(?array, ?array): bool does multiset equality (sort + unique). ['a', 'b'] vs ['b', 'a'] is a semantic no-op and produces no audit entry. null vs [] remains distinct (different operator intent). Storage preserves operator-supplied order; only the diff check normalises.
• getParams() content-type sensitivity → update() rejects requests with Content-Type other than application/json (or empty) with HTTP 415, so the PATCH never depends on body-parser heuristics for the decision-time mutation.
• Non-transactional audit write → row UPDATE + audit-trail INSERT now run inside a single $db->beginTransaction() / commit() / rollBack(). An audit failure rolls back the UPDATE and surfaces as HTTP 500 — clients never observe a persisted decision-metadata change without a matching audit entry. The downstream event dispatch stays OUTSIDE the transaction (post-commit, informational; listener failures still don't roll back). Test testAuditEmissionFailureRollsBackUpdateAndThrows updates the contract.
• fileId = 0 bypass → closed by > 0 guards on fileId / objectId / emailId in the authz dispatch helper.
• tasks.md 3.2a unchecked → items 1.1–1.3, 2.1–2.6, 3.1–3.8 all marked [x] to reflect what landed in this PR.

Minor items

• Test copyright headers → SPDX + Conduction headers added to EntityRelationsControllerTest, EntityRelationMapperTest, EntityRelationMapperUpdateDecisionMetadataTest, EntityRelationTest (also fixed the latter's namespace Unit\Db typo).
• CHANGELOG signature → corrected the updateDecisionMetadata signature from int $id to EntityRelation $relation and noted the transactional contract + multiset bases diff.
• Migration postSchemaChange() comment → documented why Version1Date20260512120000 intentionally does not override postSchemaChange (nullable column + DB-level default; no backfill needed).

Tests

44 / 44 green, 160 assertions across EntityRelationMapperUpdateDecisionMetadataTest, EntityRelationsControllerTest, EntityRelationTest, EntityRelationMapperTest. New cases cover multiset bases equality, audit-failure rollback, 403 on object-bound denial / 200 on grant, 403 on email-bound without parent object, 403 on relation without any subject.

[WilcoLouwerse]

Re-review (Strict). 3 of 4 prior findings resolved: 🔴 @NoCSRFRequired removed and spec amended (design.md §D7); 🔴 auth bypass closed — all three branches (file/object/email) now run an authorisation check, ALL-IDs-null/0 case correctly denies; 🔴 ADR-005 actor — getDisplayName() no longer persisted, audit row records only the UID. Transactional audit-write, HTTP 415 on non-JSON, multiset basesAreEqual, migration idempotency — all verified.

Newman/DI: accepting the pushback that the failure can't be conclusively diagnosed from the diff alone, but downgraded to a 🟡 follow-up because EntityRelationMapper is the only IEventDispatcher-dependent mapper without an explicit registerService() block, inconsistent with the codebase pattern (see inline).

Blocker (1 🔴):

• Double markAsAnonymized on the HTTP path — commit fb512791 added a markAsAnonymized($fileId, '[REDACTED]') at the end of DocumentProcessingHandler::anonymizeDocument, but FileTextController::anonymizeFile:547 also calls markAsAnonymized with 'anonymized_<date>' afterwards. The second UPDATE overwrites the first's anonymized_value, leaving rows with a value that doesn't match the placeholder actually written to the file.

Concerns (2 🟡):

• DI wiring inconsistency — see Newman note above.
• [<TYPE>: <entity_id>] placeholder format is unspec'd — landed today in 86f2d18803 post-author's response; needs a spec entry, and the sequential-DB-integer choice has mild cross-document correlation risk.

[rubenvdlinde]

Rebased on top of latest development (tip 95aed02c5) to pick up:

• fix(npm): regenerate package-lock.json after nc-vue major bump #1537 npm lockfile regeneration
• fix(phpcs): clean development to 0 errors across 41 files #1562 phpcs cleanup
• Implement text-extraction-eml — EML support in TextExtractionService (#1438) #1504 EML text extraction

Resolved 2 conflicts:

• openspec/changes/entity-relation-grondslagen/tasks.md: kept PR's full reworked task list (the rework commit on this branch deliberately re-introduces the skip_anonymization column + PATCH endpoint that dev's ADR-032 split removed).
• CHANGELOG.md (across two rebase steps): merged EML (dev) and entity-relation-grondslagen (PR) entries under ### Added / ### Behaviour changes; on the review-fix commit, kept the newer transaction-wrapped + multiset-equal-diff description.

appinfo/routes.php and lib/Db/EmailLinkMapper.php auto-merged. Force-pushed with lease.

[rjzondervan]

@WilcoLouwerse — addressed in eb02e6b41. Details below mapped to the three findings.

🔴 Double markAsAnonymized on the HTTP path

Picked option (b) per your recommendation: DocumentProcessingHandler::anonymizeDocument is now the sole canonical caller. The controller-side markAsAnonymized($fileId, 'anonymized_<date>') in FileTextController::anonymizeFile is removed, with an inline comment explaining why a controller-side call would race the DI-path call and clobber anonymized_value.

Value semantics: kept '[REDACTED]' (already in DocumentProcessingHandler since fb512791b) as the canonical column marker. The 'anonymized_<date>' timestamp form was incidental — no consumer queries depend on it; the column's purpose is "this file's relations have been redacted at least once" + a generic marker for audit log readability. Per-entity placeholders live in the file content ([<TYPE>: <entity_id>]) and the audit-trail row records the actor + timestamp, so the column doesn't need to carry redundant time info.

Tests updated: FileTextControllerTest::testAnonymizeFileWithSpecialEntities and ::testAnonymizeFileDeduplicatesEntities now assert expects($this->never())->method('markAsAnonymized') on the controller-side mock with explanatory comments. The DocumentProcessingHandler-side call is mocked through $this->fileService in these tests; coverage of the actual mark belongs in DocumentProcessingHandlerTest (which is a separate piece of work; flagged for follow-up if not already there).

🟡 DI wiring — explicit registerService() for EntityRelationMapper

Added the explicit block in Application::registerMappersWithCircularDependencies() immediately after MagicMapper, mirroring the pattern (constructor injects: IDBConnection, AuditTrailMapper, IUserSession, IEventDispatcher, LoggerInterface). Comment explains the rationale (every other event-dispatcher-dependent mapper is explicit; autowiring of framework-interface keys is version-sensitive). Removes the autowiring question entirely.

🟡 [<TYPE>: <entity_id>] placeholder is unspec'd + sequential-DB-integer correlation risk

Two new ADDED Requirements in specs/entity-relation-grondslagen/spec.md:

1. Placeholder format requirement — captures the exact shape ([<TYPE>: <entity_id>]), the entity_id source (openregister_entities PK, looked up via findEntityIdsByValueForFile), the whitespace contract (exactly one space after the colon), and the fallback to an 8-char UUID fragment when an entity row is missing for DI callers bypassing the standard detection path. Three scenarios: stable placeholder, byte-identical re-anonymise, fallback for missing rows.
2. Canonical-caller requirement — exactly ONE component (DocumentProcessingHandler::anonymizeDocument) calls markAsAnonymized per redaction. HTTP controllers, DI orchestrators, and event listeners MUST NOT call it independently. Two scenarios covering HTTP-call and DI-call paths.

Plus a Notes section "Placeholder format and cross-document correlation" addressing the correlation concern explicitly:

• The within-tenant correlation property is deliberate, not a leak — consumer reports (grondslagen-summary in DocuDesk) thread entities through dossier files using these stable ids; the feature wouldn't work without them.
• Cross-tenant correlation is impossible (per-tenant DB, independent PK sequences).
• HMAC with per-tenant salt was considered — it would carry the same stability and the same correlation, so doesn't change the privacy property; we kept raw entity_id for simplicity.
• Per-document salt would defeat correlation but break the consumer use case.

openspec validate entity-relation-grondslagen — clean.

Re-requesting review.

[WilcoLouwerse]

Re-review verdict — APPROVE

Commit eb02e6b41 addresses the three findings from my 2026-05-18 re-review, and the four findings from the 2026-05-15 round are confirmed resolved by e34eac991. CI Newman API Test Suite is passing on HEAD.

Resolved this round (eb02e6b)

• 🔴 Double markAsAnonymized on HTTP path — option (b) chosen; DocumentProcessingHandler::anonymizeDocument is sole canonical caller (writes '[REDACTED]'), FileTextController::anonymizeFile no longer marks. Tests now expects($this->never()). Call chain verified end-to-end.
• 🟡 DI wiring — explicit registerService for EntityRelationMapper — explicit factory block added in Application::registerMappersWithCircularDependencies(), mirroring the pattern used by SchemaMapper / MagicMapper / WebhookMapper.
• 🟡 Placeholder format unspec'd + correlation risk — two new Requirements in specs/entity-relation-grondslagen/spec.md (placeholder format with three scenarios incl. fallback; sole canonical caller for markAsAnonymized), plus a "Placeholder format and cross-document correlation" Notes section explicitly framing within-tenant correlation as deliberate (Woo dossier consumer requirement), with cross-tenant impossibility and HMAC alternatives discussed.

Resolved earlier (e34eac9)

• 🔴 @NoCSRFRequired on state-mutating PATCH — annotation removed; design §D7 records the rationale.
• 🔴 Auth bypass for object/email-bound relations — PermissionHandler wired, object-bound relations now route through hasPermission(schema, 'update', uid, owner, object); email-bound relations resolve parent ObjectEntity via EmailLink and run the same verdict; >0 guards close the fileId=0/objectId=0/emailId=0 bypass.
• 🔴 Newman regression / IEventDispatcher DI wiring — although the author initially pushed back on the diagnosis, the explicit registerService block from the follow-up effectively closes this; Newman now passes on HEAD.
• 🔴 ADR-005 violation — display name stored as audit actor — userName no longer persisted; only the stable UID is stored in the audit-trail row.

Minor non-blocking notes for follow-up

• DocumentProcessingHandler::anonymizeDocument does not emit the warning log that the new spec's fallback scenario says implementations SHOULD log when no matching entity-catalogue row is found. SHOULD-level, non-blocking.
• The fallback uses $entity['key'] ?? substr(uuid, 0, 8) rather than always a UUID fragment, so a DI caller supplying 'key' can influence the placeholder content. Spec says MAY, so within bounds.

LGTM. 🟢