Skip to content

feat(gdpr): an erasure is previewed with counts and runs only from an approved preview - #3759

Merged
rubenvdlinde merged 11 commits into
developmentfrom
feat/data-subject-rights-across-the-instance
Sep 15, 2026
Merged

feat(gdpr): an erasure is previewed with counts and runs only from an approved preview#3759
rubenvdlinde merged 11 commits into
developmentfrom
feat/data-subject-rights-across-the-instance

Conversation

@rubenvdlinde

@rubenvdlinde rubenvdlinde commented Sep 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What this adds

OpenRegister could already erase a data subject across every register. What it could not do is answer first.

gdpr-data-subject-rights ships the data-subject-request object, the article 12 deadline, an RBAC and tenant scoped service, and an erasure that honours a legal hold and is mode-parameterised. delete-window-and-recorded-destruction ships the one destruction path with its window and its recorded destruction. permission-provenance-and-deny ships the resolver. The dsar-* changes ship the engine, the subsystem, the surface, the escalation and the policy pack. None of them counts what an erasure will touch before it runs, and erase(dryRun: true) reports one list of things it would have erased, which is the half a gemeente does not need.

The lawful answer to a data subject under the Archiefwet is "deels niet, en dit is waarom". That sentence is made of the protected count and its ground.

The preview. ErasurePreviewService classifies every object the subject appears on into three buckets and counts four kinds per bucket: objects, files, timeline entries and party records, split erasable, pseudonymised and protected. Every protected record is listed again by name with its ground, the legal basis, and what the handler does next.

It reuses rather than rebuilds. Discovery is DataSubjectRequestService::findSubjectObjects(), the loaded-entity sibling of the existing findSubjectData(). The retention refusal and its wording are ArchivalRetentionGuard::erasureRefusal(), which already names the legal hold, the Archiefwet obligation and the unresolved schema. The immutability check is RetentionService::validateNotImmutable(). An erasable object's files and timeline rows are DestructionScopeService::preview(), which is already the definition of what goes with an object.

Two decisions worth reading. An object whose hold cannot be resolved at all, because the guard threw or the schema row would not read, counts as protected and is named, under HOLD_UNRESOLVABLE (D-2). The guard's own SCHEMA_UNRESOLVED covers a question it can ask and not answer; this covers a failure to ask. And a whole-object erasure on a record that also carries another person's identifier of the same kind is downgraded to a scrub, named SHARED_RECORD, because destroying it would erase somebody who did not ask. Same kind matters: one person routinely carries an email, a phone and a bsn, so a second email is the signal, not a second identifier.

The run. ErasureRunner refuses three ways and writes nothing in all three: the preview was never approved, it was already spent, or it was approved for a world that has since moved. The digest is recomputed at run time, and it deliberately excludes generatedAt so an unchanged world agrees with itself. What the run destroys goes through DestructionScopeService::destroy() and DestructionRecorder::record(), and the destruction record names the data subject request (D-3). The destroy right and the retention clock are asked again per object, because an approval is not a permission and a hold placed after the approval has to win.

The recorded answer. openregister_erasure_previews keeps the preview verbatim with its digest, its author and its approval. A preview is reachable only by its author or an administrator, and a stranger is told the preview does not exist rather than that it is forbidden. "Forbidden" would confirm that somebody asked about this data subject, which is itself the disclosure.

Scope: this is the first PR of a size-L change

Tasks 1, 2 and the part of 6 they carry. Tasks 3 (the subject's own export as a background job with an expiring file), 4 (the reach listing and the one revocation act) and 5 (external grants with a required end date) ship on a follow-up branch off this one. Task 7 is the hand-over.

The request and answer contract consumers can build against

Four routes, all @NoAdminRequired, all scoped by the service's _rbac and _multitenancy object loads:

Route Answers
POST /api/gdpr/erasure-previews {subject, type?, eraseMode?, request?} gives back the recorded preview
GET /api/gdpr/erasure-previews/{id} the recorded preview
POST /api/gdpr/erasure-previews/{id}/approve the approved preview
POST /api/gdpr/erasure-previews/{id}/run the outcome

A preview answers:

{ uuid, subject, subjectType, eraseMode, requestId, digest, status, createdBy,
  approvedBy, approvedAt, consumedAt,
  report: {
    subject, type, eraseMode, generatedAt, matchedCount, digest,
    counts: { erasable|pseudonymised|protected: { objects, files, timelineEntries, partyRecords } },
    items:     [ { uuid, register, schema, matchedOn[], bucket, ground?, message?, basis?, action?, sharedWith?[], counts{} } ],
    protected: [ the protected items again, so the answer to the subject is written from one list ]
  },
  outcome: { ... once it has run } }

A run answers {preview, request, subject, eraseMode, ranAt, destroyed[], pseudonymised[], withheld[], refused[], failed[], *Count, complete}. complete is false whenever anything was withheld, refused or failed. A run that kept data back is not a complete erasure, so nobody is told a subject's data is gone when it is not.

Refusals are {error: "ERASURE_REFUSED", rule, message, ...} with rules erasure-preview-unknown (404), erasure-not-approved (409), erasure-already-run (409) and erasure-preview-stale (409).

For the consuming lanes: dossiq drives the preview on a zaak's parties and reads report.protected for the sentence it sends. filinq hangs erase-a-person-while-the-records-stay off the pseudonymised bucket, where the object survives and the values do not. portaliq and humaniq wait for the follow-up branch.

Verification

composer check:strict once, npm run lint once (exit 0, 932 inherited warnings, no errors), the diff check on the lines this PR touched.

The first diff check was red on four things, all fixed in 6f185cb1:

  • Gate-110 is the one worth reading. The migration shipped with <version> left where it was. occ upgrade would have answered "No upgrade required.", exited 0 and created no table. Nothing logged, nothing failed. Version moved to 2.1.27 in the same change.
  • Gate-16 wanted a @spec on findSubjectData, whose body this change rewrote.
  • phpcs wanted named parameters on the call it now makes, docblocks on four test helpers, and three route lines under 150 characters. Those lines were nearly shortened by trimming [^/]+ to [^/], which would have made every preview id a single character and passed the gate while breaking all three routes. They are wrapped instead, and the four routes are read back from the parsed file with their requirements intact.
  • eslint: the e2e spec declared an admin account it never used.

phpmd then put three findings on this change's own files, fixed in df5c9dee. Two are the house pattern and carry a stated reason: a Nextcloud DI constructor with ten authorities, and static access to a closed vocabulary whose members are compile-time constants, the same shape as DestructionScope. The third was real and is not excused: ErasurePreviewService sat at complexity 51 against a threshold of 50, so the payload walk that counts party records moved out into SubjectPartyCounter. It has no collaborators, it is the half worth testing on its own, and it now owns the single definition of "this subject's values" that the co-subject probe also needs, so the two cannot drift. phpmd is clean on every file this PR touches; the 56 findings it still reports are inherited, in lib/Service/Vocabulary/ and other files this PR never opened.

One reading the gates gave that is not a pass and is not fixed here: gate-19 (e2e-coverage) reported EMPTY SCOPE. It says no spec file was touched and that ADR-020 traceability is therefore UNVERIFIED by that run, although this PR does modify openspec/changes/data-subject-rights-across-the-instance/specs/gdpr-data-subject-rights/spec.md. The gate's own words: "This is not a pass." Worth a look from whoever owns gate-19. The @e2e anchors and the two reasoned excludes are in place either way, and gate-25 (contract-coverage) passes on all four new endpoints.

What the tests prove, and what they do not

The e2e spec asserts what a green unit suite hides: the four routes exist, the refusals answer by name over HTTP, and one handler's preview is not readable by another. It does not assert the counts, and the file's header says why. The preview discovers a subject through the PII index, and that index is written by file text extraction, so there is no HTTP door that attaches a detected entity to an object. A spec that created an object with an email in it and asserted "eight erasable, four protected" would assert against an empty index, which is a test that cannot fail. The two count-bearing scenarios carry an @e2e exclude naming the unit test that does assert them.

Mutation-checked, both reverted: blanking the approval guard reddens ErasurePreviewStoreTest::testAnUnapprovedErasureDoesNotRun on its own assertion, and blanking the digest comparison reddens ErasureRunnerTest::testAPreviewApprovedForAnotherWorldDoesNotRun on its own.

Inherited findings

Reported, not fixed, per the inherited-debt rule.

  • phpcs carries 267 inherited findings on lines this PR did not touch, 208 of them in appinfo/routes.php (the file this PR adds four routes to; its own new lines are clean) and 59 in tests/Unit/Controller/PermissionsControllerTest.php.
  • Gate-66 integration-parity warns, advisory and non-blocking, that four pre-existing IntegrationProviders (brp-haalcentraal, kvk, message-dispatch, opencorporates) advertise a render surface with no matching JS registration, so they mount nothing. Unrelated to this change.
  • Gate-96 (2 App Store description strings against voice.md §8) and gate-112 (19 committed Postman collections CI never runs) both warn, both pre-existing, both report-only.

Next

Review this one, then the follow-up branch picks up task 3 (the subject's own export) on top of it.

🤖 Generated with Claude Code

@rubenvdlinde
feat(rbac): a grant may end, belong to one area, or be derived at sig…
0a00db6 
…n-in

Section 8 of permission-provenance-and-deny: the three shapes of grant the
register row asked for, and the recount when the rule behind one moves.

AN END ON A GRANT (8.2). An entry carrying `until` stops answering the moment it
passes, read at resolution rather than swept. Nothing has to run to take the
right away, which matters because the job that takes a right away is the one
nobody notices has stopped. A workflow step binds a grant to its deadline by
writing that deadline in: no second clock to drift, and a step that moves its
deadline rewrites the grant the same way it wrote it.

AN AREA ON A GRANT (8.4). An entry carrying `scopedTo` answers only in the
registers and schemas it names, so `manage` can be delegated without handing the
instance over. The key is `scopedTo` and not `scope`, because a block already
carries a `scope` naming an object's visibility, and a second meaning for one
word in one block is how a rule gets read by the wrong reader.

ACCESS DERIVED AT SIGN-IN (8.1). Rules map what an identity provider asserts to
groups, a role and an area. OpenRegister does not speak to a provider and should
not start, so it asks the app that holds that session through
IdentityClaimsCollectingEvent and reads whatever comes back. A sign-in that
asserts nothing leaves the account with no derived access rather than with
yesterday's, because carrying the old set forward keeps somebody in a department
they left. A derived group is never a reserved one: `admin`, `public`,
`authenticated` and `mcp` are filtered out, so a rule can never hand a provider
the power to make somebody an administrator here. The fold happens after the
administrator check for the same reason, and a test asserts it.

Both constraints are read in resolveAuthorization, beside the mcp strip, because
that is the one step every path takes: the object read, the relation check and
both list emitters. A grant that expired on one surface and not another would be
the worst possible version of this.
@rubenvdlinde
feat(rbac): a rule change re-runs the derivation and says how many gr…
32c7c38 
…ants moved

Task 8.3. An access change nobody is told about is the one that surprises an
auditor, and narrowing a rule that reached two hundred people is a decision
somebody should see the size of before they walk away from the screen.

POST /api/permissions/derived-grants/reapply re-derives every account that has
claims stored and answers in numbers, naming the accounts whose access moved. The
claims are kept at sign-in precisely so this can run without waiting a fortnight
for everybody to sign in again.

Administrator only, and by the framework rather than by a check in the method:
the route carries no NoAdminRequired, so the middleware refuses everybody else
before the method runs. A body check beside that attribute is the exact mismatch
the semantic-auth gate exists to catch.

An instance with no rules says so rather than reporting that nothing moved. The
second reads as a rule that had no effect, which is the answer somebody
debugging a rule would most like to be given by mistake.
@rubenvdlinde
refactor(rbac): the re-run gets its own controller, and three methods…
9404320 
… get shorter

phpmd was right on all seven counts, and every one of them was the same shape:
something had grown past the size where it can be checked by eye.

The re-run moved out of PermissionsController, which was at the complexity and
coupling thresholds with it. Derived grants are a different noun from the
permission catalogue anyway: one publishes what can be granted, the other
re-runs what was derived.

derive(), apply() and names() each gave up a branch to a named helper, so the
reason a rule is skipped, the way one block key is filtered, and the spellings an
area answers to are now three things with names instead of three nested
conditions. The callback the account walk passes returns null rather than void,
because the return value is what stops the walk and stopping it would silently
leave the rest of the accounts unre-derived.
@rubenvdlinde
docs(rbac): the two store reads carry their spec tag
e1e651c 
Gate 16 was right: claimsFor() and forget() are public and changed in this PR,
and neither said which requirement it serves.
@rubenvdlinde
feat(gdpr): an erasure is previewed with counts and runs only from an…
f92a43c 
… approved preview

A gemeente cannot answer a data subject with one number. The lawful answer
under the Archiefwet is "deels niet, en dit is waarom", so the preview
counts objects, files, timeline entries and party records split three ways
-- erasable, pseudonymised, protected -- and names every protected record
with the ground and the sentence to pass back.

It writes nothing. A record whose hold cannot be resolved counts as
protected, because erasing on an unknown is the one failure that cannot be
undone. A whole-object request on a record that also holds another person's
identifier of the same kind is downgraded to a scrub, and the downgrade is
named.

The run refuses three ways and writes nothing in all three: never approved,
already spent, or approved for a world that has since moved (the digest is
recomputed). What it destroys goes through the delete window's recorded
destruction, and the destruction record names the data subject request.
@rubenvdlinde
test(gdpr): the refusals are asserted over HTTP, the counts in the un…
3cb4ad8 
…it suite

Three unit suites and one e2e spec. The split is deliberate and stated in
the e2e file's header: the preview discovers a subject through the PII
index, and that index is written by file text extraction, so there is no
HTTP door that attaches a detected entity to an object. A spec that created
an object with an email in it and asserted "eight erasable, four protected"
would assert against an empty index, which is a test that cannot fail. The
two count-bearing scenarios carry an @e2e exclude naming the unit test that
does assert them.

What the e2e does prove is what a green unit suite hides: the four routes
exist, the refusals answer by name over HTTP, and one handler's preview is
not readable by another.

Mutation-checked: blanking the approval guard reddens
testAnUnapprovedErasureDoesNotRun on its own assertion, and blanking the
digest comparison reddens testAPreviewApprovedForAnotherWorldDoesNotRun on
its own. Both reverted.
@rubenvdlinde
merge development into the data-subject-rights lane
eb35830
@rubenvdlinde
fix(gdpr): the diff check's own findings, and the version bump the mi…
6f185cb 
…gration needs

Gate-110 is the one that matters: a migration ships with the app version
left where it was, and `occ upgrade` answers "No upgrade required.", exits
0, and runs none of it. Nothing logged, nothing failed, no table. Version
moved to 2.1.27 in the same change.

The rest: gate-16 wanted a @SPEC on findSubjectData, whose body this change
rewrote; phpcs wanted named parameters on the call it now makes, docblocks
on four test helpers, and the three route lines under 150 characters.

The route lines were nearly shortened by trimming '[^/]+' to '[^/]', which
would have made every preview id a single character and passed the gate
while breaking all three routes. They are wrapped instead, and the four
routes are read back from the parsed file with their requirements intact.

eslint: the e2e spec declared an admin account it never used. Deleted, with
the reason it should not have one written down — every route under test is
@NoAdminRequired, so driving it as the administrator would pass the reach
rule by privilege and prove nothing about the rule.
@rubenvdlinde
merge development into the data-subject-rights lane
ad20d7b 
# Conflicts:
#	appinfo/info.xml
@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 57e8517

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 ✅ ✅ 653/653
app:check-code ⏭️
info.xml ✅
REUSE ❌
lockfile sync ✅
PHPUnit ❌
Newman ✅
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test.
Hydra gates ❌

Quality workflow — 2026-09-15 08:53 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
refactor(gdpr): the party counting moves out, so the class is under t…
df5c9de 
…he threshold rather than excused

phpmd put three findings on this change's own files. Two are the house
pattern and carry a stated reason: a Nextcloud DI constructor with ten
authorities, and static access to a closed vocabulary whose members are
compile-time constants, the same shape as DestructionScope.

The third was real. ErasurePreviewService sat at complexity 51 against a
threshold of 50, and the honest fix is not a suppression: the payload walk
that counts party records has no collaborators, is the half worth testing on
its own, and was only ever inside the preview because that is where it was
written. It is SubjectPartyCounter now, and it owns the one definition of
'this subject's values' that the co-subject probe also needs, so the two
cannot drift.

27 unit tests still green.
@rubenvdlinde
merge development into the data-subject-rights lane
fdb32b3 
# Conflicts:
#	appinfo/info.xml
@rubenvdlinde
rubenvdlinde merged commit 6c820b3 into development Sep 15, 2026
32 of 38 checks passed
@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 806d64b

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 ✅ ✅ 653/653
app:check-code ⏭️
info.xml ✅
REUSE ❌
lockfile sync ✅
PHPUnit ❌
Newman ❌
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test.
Hydra gates ❌

Quality workflow — 2026-09-15 18:41 UTC

Download the full PDF report from the workflow artifacts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@rubenvdlinde