feat: entity-level publication policy layer (#112)#147
Conversation
rjzondervan
requested review from
Rem-Dam,
SudoThijn,
WilcoLouwerse,
bbrands02,
remko48 and
rubenvdlinde
as code owners
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ❌ | ||||
| composer | ✅ | ✅ 108/108 | |||
| npm | ❌ | ❌ | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-15 08:11 UTC
Download the full PDF report from the workflow artifacts.
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — Strict mode. 6 blockers, 5 significant issues.
🔴 Blockers (6): RBAC bypass on prohibition CRUD; prohibition lock bypass via publicationDecision; silent write on referent lookup failure; NcSelect missing inputLabel (ADR-004/gate-12, protected); NcDialog inline modal (ADR-004/gate-13, protected); \OC::$server->get() service-locator.
🟡 Significant (5): loadStandingConsents full table scan with no scope filter; ConsentDetail.vue uses raw fetch() instead of axios; PolicyRetroactiveService full table scan; severity enum mismatch between register JSON and frontend; WAVE-1-SMOKE-TESTS.md in repo root with hardcoded credentials.
All CI failures are pre-existing on the development branch.
…s + significant) Addresses the DD #147 review on the entity-level publication policy layer. Six blockers, five significant items, plus moving the smoke-test doc out of the repo root. **Blockers:** - **RBAC bypass on prohibition CRUD.** `PolicyCrudService::createProhibition`, `updateProhibition`, and `deleteProhibition` no longer call ObjectService with `_rbac: false` without a group check. A new private `assertProhibitionPermission(string $action)` mirrors `assertStandingConsentPermission` and gates writes on either admin role or membership in the new `docudesk-prohibition-admins` group. Throws RuntimeException otherwise — mapped to HTTP 403 by the controller. - **Service-locator anti-pattern in DocuDeskEventHandler.** Removed the `\OC::$server->get(PolicyRetroactiveService::class)` call inside `dispatchPolicyRetroactive`. The retroactive service is now passed as a method parameter from each public event-handler entry point (`handleObjectCreated` / `handleObjectUpdated` / `handleObjectDeleted`), so Nextcloud's event dispatcher injects it via reflection-based DI — no static accessor, no NC30 breakage. - **Prohibition lock bypass via publicationDecision.** `ConsentUpdateHandler::guardPolicyPreemptedTransition` now checks BOTH `consentStatus` AND `publicationDecision` for a change against the existing record. A PATCH carrying only `publicationDecision: "publish"` on a prohibition-matched record now raises InvalidArgumentException with the rejected field name. Without this fix, only `consentStatus` transitions were guarded — the publish field was a quiet bypass. - **Silent write on policyMatch lookup failure.** `ConsentService::assertPolicyMatchReferentValid` no longer swallows ObjectService lookup errors with a warning log. A failed lookup now throws `InvalidArgumentException` (mapped to HTTP 400 at the controller), with the underlying exception preserved as `previous`. A write referencing an unresolvable `policyMatch` UUID is rejected outright, never persisted. - **NcSelect missing inputLabel (ADR-004 gate-12).** Added `:input-label="…"` to every NcSelect in `ProhibitionIndex.vue`, `ProhibitionFormModal.vue`, and `StandingConsentIndex.vue`. Screen readers now have a labelled control on entity-type, severity, match-type, and consent-method dropdowns. - **NcDialog inline (ADR-004 gate-13).** Extracted the prohibition create/edit dialog from `ProhibitionIndex.vue` into a new `ProhibitionFormModal.vue` component. The parent now owns only the open flag + record-being-edited; the modal hydrates its own form state on `open` change and emits `submit(data)` / `update:open` / `cancel`. The dialog (and its form's match-rule logic, validation, options) is no longer embedded in the index view. **Significant items:** - **`loadStandingConsents` full table scan.** `PolicyMatchService` now pushes `scope=entity` AND `active=true` filters down to ObjectService instead of loading every publicationConsent row and filtering in PHP. The defensive PHP scope check is retained as a belt-and-braces. - **Raw `fetch()` in ConsentDetail.vue.** Replaced both `fetch(OC.generateUrl(...))` calls in `refreshPolicyMatch` with `axios.get(generateUrl(...))`, picking up the app's standard auth headers + CSRF tokens + error envelope. Added the `axios` and `generateUrl` imports. - **`PolicyRetroactiveService` full table scan.** `loadInFlightDocumentRecords` now adds `scope=document` to the findAll filter so the read is bounded server-side. Same defensive scope check retained. - **Severity enum mismatch.** The frontend `severityOptions` (was `['low','standard','high','critical']`, default `'standard'`) did not match the `publicationProhibition.severity` enum in `docudesk_register.json` (`['high','medium','low']`, default `'medium'`). The frontend has been corrected to mirror the schema; `severityColorMap` updated to match. Schema is the source of truth. - **WAVE-1-SMOKE-TESTS.md moved out of repo root.** Now lives at `docs/testing/wave-1-smoke-tests.md`. Hardcoded `admin/admin` credentials replaced with `NC_USER` / `NC_PASS` env-var placeholders and a clear "local dev only" disclaimer at the top. **Quality.** - PHPCS clean on every touched lib file (one 130-char warning on a required sprintf format string; under threshold of error). - Psalm: no errors on the six touched lib files. - PHPStan: no errors on the six touched lib files. - Test bootstrap requires a running NC stack (pre-existing); not exercised here.
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ❌ | ||||
| composer | ✅ | ✅ 108/108 | |||
| npm | ❌ | ❌ | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-15 14:56 UTC
Download the full PDF report from the workflow artifacts.
…gh the event listener Follow-up to commit 3c90dd1 (PR #147 review). When I replaced the service-locator in `DocuDeskEventHandler` with a constructor/method parameter, I updated the three public event-handler methods (`handleObjectCreated`, `handleObjectUpdated`, `handleObjectDeleted`) to take `PolicyRetroactiveService $retroactive` as an extra argument — but the caller in `DocuDeskEventListener::dispatchEvent` was not updated to pass it through. Result: every ObjectCreated / ObjectUpdated / ObjectDeleted event from OpenRegister hit DocuDesk and immediately failed with: > Too few arguments to function > OCA\DocuDesk\EventListener\DocuDeskEventHandler::handleObjectCreated(), > 5 passed in DocuDeskEventListener.php on line 114 and exactly 6 > expected Visible to operators as a hard failure on creating any DocuDesk-side object that goes through OR's event bus, including dossiers via the new "Create dossier for this folder" UI in Wave 4a. Fix: pull `PolicyRetroactiveService` via the same service-locator the listener already uses for the other DI deps (Logger, MetadataService, SettingsService) and pass it into `dispatchEvent`, which then threads it into all three handler calls. The handlers themselves were already correct. This keeps the listener layer (which lives outside the DI container because it's invoked via NC's event dispatcher reflection) on its existing pattern while the handler layer gets proper method-arg DI.
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ❌ | ||||
| composer | ✅ | ✅ 108/108 | |||
| npm | ❌ | ❌ | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-18 08:46 UTC
Download the full PDF report from the workflow artifacts.
Review response — commits
|
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Re-review (Strict). 7 of 8 prior findings resolved. RBAC helper, service-locator removal from DocuDeskEventHandler, prohibition-lock guard on both consentStatus and publicationDecision, assertPolicyMatchReferentValid now throws, ProhibitionFormModal.vue extracted, NcSelect :input-label added in ProhibitionIndex.vue / ProhibitionFormModal.vue / StandingConsentIndex.vue, scope filters pushed to DB in both PolicyMatchService and PolicyRetroactiveService, raw fetch() replaced with axios.get, severity enum aligned to schema.
Blockers (2 🔴):
PolicyControllermissingRuntimeException → 403catch on createProhibition —RuntimeExceptionextendsException, so the new RBAC gate falls through and returns HTTP 500 instead of 403. Same defect on updateProhibition and deleteProhibition. Standing-consent counterparts already have the correct catch block; copy the pattern.- Inline
<NcDialog>inStandingConsentIndex.vue— same ADR-004 gate-13 violation that was fixed forProhibitionIndex.vue. Extract intoStandingConsentFormModal.vue.
Concerns (2 🟡):
PR #119 split Vue / @nextcloud/vue / @conduction/nextcloud-vue / pinia / vue-material-design-icons into shared chunks (docudesk-shared-vendor.js, docudesk-shared-nc-vue.js), and updated the dashboard-widget loaders to addScript them. The two page templates (templates/index.php and templates/settings/admin.php) were missed: only the per-page entry was loaded, so webpack's runtime sat forever in chunkOnLoad waiting for chunks that never arrived. Result: blank Anonymisation page, blank admin settings page, blank main app — no console error, just nothing. Add the two shared chunks to both templates so the entry's chunkOnLoad callback resolves. Order matters only to the extent that the shared chunks self-register on a shared global before the entry runs; loading them first is the conventional path. Pipelinq and procest received the same split-chunks treatment in #119 and likely have the same broken template — worth a cross-check.
Implements the entity-publication-policies change (Wave 1.2). The
consent service now consults a two-tier policy layer at detection time
before falling through to the WOO objection workflow:
- publicationProhibition (deny-list): entity-level rules that force
anonymise on match. Court orders, minor protection, undercover
officers, AVG categorical exemptions. New schema in the consent
register with 4 seed records.
- publicationConsent scope="entity" (allow-list, "standing consent"):
entity-level rules where consent was obtained out-of-band (paper,
digital signature, recorded verbal, opt-in form). New `scope`
discriminator + entity-scope field set on the existing schema, with
4 seed records.
Conflict resolution is deterministic: prohibition wins, multi-prohibition
match resolved by lowest UUID lexicographic. Standing-consent matches
populate `policyMatch` referencing the rule UUID.
Retroactive behaviour is asymmetric: creating or widening a prohibition
force-resolves all in-flight scope=document records that now match,
preserving notificationSentAt / objectionReceivedAt for audit. Standing
consents apply to future detections only — privacy default wins on
retroactive sweep.
Three admin pages back the surface:
- Consent Workflow: existing per-document records, now with a
"policy" indicator on rows whose policyMatch is non-null.
- Standing Publication Consents: scope=entity records (new).
- Publication Prohibitions: publicationProhibition records (new).
The publication-prep anonymisation toggle is keyed off the policyMatch
referent type, not consentStatus: prohibition -> ON+locked, standing
consent -> OFF+overridable (override records publicationDecision change
while preserving policyMatch and consentStatus).
RBAC: publicationProhibition writes restricted to docudesk-policy-admins
at the schema level. Standing-consent writes gated at service level by
docudesk-standing-consent-admins (the schema can't discriminate by scope
so the gate lives in PolicyCrudService). Both return 403 on failure.
Other notes:
- PolicyMatchService caches active rules per-request with deterministic
sort, four match types: exact, normalized, bsn, kvk.
- PolicyRetroactiveService dispatched from DocuDeskEventHandler based
on payload-shape heuristics (avoids per-event schema-ID lookup).
- ConsentUpdateHandler now rejects consentStatus transitions on records
pre-empted by a policy; overrides go via publicationDecision.
- 19 new unit tests covering scope-validation corners, retroactive
edge cases, and the standard 14-pass ConsentService surface.
- Newman collection extended with policy CRUD + scope-validation tests.
- WAVE-1-SMOKE-TESTS.md groups manual smoke steps across Waves 1.1-1.4.
- register version bumped to 7.0.0 to force re-import on existing
instances; adds the new authorisation block on publicationProhibition.
Spec: openspec/changes/entity-publication-policies/specs/entity-publication-policies/spec.md
Reworks AnonymizationWidget + anonymization store to pause between
extract and anonymise so the operator can set bases / skipAnonymization
per detected entity. Sets up the smoke-test surface for the Wave 1.3
PATCH endpoint on /apps/openregister/api/entity-relations/{id} and is
intentionally simple — example shape for the frontend team, not a
production publication-prep page.
Backend:
- EntityDetectionService::normalizeEntities now passes through
relationId, bases, skipAnonymization from the EntityRelation row.
Forward-compatible: bases/skipAnonymization are null on OR branches
that pre-date entity-relation-grondslagen.
Store (src/store/modules/anonymization.js):
- Lifecycle now queued -> uploading -> extracting -> extracted ->
anonymising -> completed (was a single auto-pipeline).
- addFiles uploads + extracts only; stops at 'extracted'.
- anonymiseEntry PATCHes each modified relation via the OR endpoint
then triggers anonymise. Partial-application semantics: per-relation
PATCH errors are surfaced inline without aborting the file.
- anonymiseAllExtracted bulk-runs the anonymise step on every
reviewed file.
Widget (src/views/anonymization/AnonymizationWidget.vue):
- Drop zone + per-file cards with status badges.
- Review table per file with bases multi-select (hardcoded 6 Woo
Art. 5 grondslagen slugs) and skip switch.
- "Apply decisions and anonymise" per file + "Anonymise all reviewed"
bulk action.
- Download link on completion.
…s + significant) Addresses the DD #147 review on the entity-level publication policy layer. Six blockers, five significant items, plus moving the smoke-test doc out of the repo root. **Blockers:** - **RBAC bypass on prohibition CRUD.** `PolicyCrudService::createProhibition`, `updateProhibition`, and `deleteProhibition` no longer call ObjectService with `_rbac: false` without a group check. A new private `assertProhibitionPermission(string $action)` mirrors `assertStandingConsentPermission` and gates writes on either admin role or membership in the new `docudesk-prohibition-admins` group. Throws RuntimeException otherwise — mapped to HTTP 403 by the controller. - **Service-locator anti-pattern in DocuDeskEventHandler.** Removed the `\OC::$server->get(PolicyRetroactiveService::class)` call inside `dispatchPolicyRetroactive`. The retroactive service is now passed as a method parameter from each public event-handler entry point (`handleObjectCreated` / `handleObjectUpdated` / `handleObjectDeleted`), so Nextcloud's event dispatcher injects it via reflection-based DI — no static accessor, no NC30 breakage. - **Prohibition lock bypass via publicationDecision.** `ConsentUpdateHandler::guardPolicyPreemptedTransition` now checks BOTH `consentStatus` AND `publicationDecision` for a change against the existing record. A PATCH carrying only `publicationDecision: "publish"` on a prohibition-matched record now raises InvalidArgumentException with the rejected field name. Without this fix, only `consentStatus` transitions were guarded — the publish field was a quiet bypass. - **Silent write on policyMatch lookup failure.** `ConsentService::assertPolicyMatchReferentValid` no longer swallows ObjectService lookup errors with a warning log. A failed lookup now throws `InvalidArgumentException` (mapped to HTTP 400 at the controller), with the underlying exception preserved as `previous`. A write referencing an unresolvable `policyMatch` UUID is rejected outright, never persisted. - **NcSelect missing inputLabel (ADR-004 gate-12).** Added `:input-label="…"` to every NcSelect in `ProhibitionIndex.vue`, `ProhibitionFormModal.vue`, and `StandingConsentIndex.vue`. Screen readers now have a labelled control on entity-type, severity, match-type, and consent-method dropdowns. - **NcDialog inline (ADR-004 gate-13).** Extracted the prohibition create/edit dialog from `ProhibitionIndex.vue` into a new `ProhibitionFormModal.vue` component. The parent now owns only the open flag + record-being-edited; the modal hydrates its own form state on `open` change and emits `submit(data)` / `update:open` / `cancel`. The dialog (and its form's match-rule logic, validation, options) is no longer embedded in the index view. **Significant items:** - **`loadStandingConsents` full table scan.** `PolicyMatchService` now pushes `scope=entity` AND `active=true` filters down to ObjectService instead of loading every publicationConsent row and filtering in PHP. The defensive PHP scope check is retained as a belt-and-braces. - **Raw `fetch()` in ConsentDetail.vue.** Replaced both `fetch(OC.generateUrl(...))` calls in `refreshPolicyMatch` with `axios.get(generateUrl(...))`, picking up the app's standard auth headers + CSRF tokens + error envelope. Added the `axios` and `generateUrl` imports. - **`PolicyRetroactiveService` full table scan.** `loadInFlightDocumentRecords` now adds `scope=document` to the findAll filter so the read is bounded server-side. Same defensive scope check retained. - **Severity enum mismatch.** The frontend `severityOptions` (was `['low','standard','high','critical']`, default `'standard'`) did not match the `publicationProhibition.severity` enum in `docudesk_register.json` (`['high','medium','low']`, default `'medium'`). The frontend has been corrected to mirror the schema; `severityColorMap` updated to match. Schema is the source of truth. - **WAVE-1-SMOKE-TESTS.md moved out of repo root.** Now lives at `docs/testing/wave-1-smoke-tests.md`. Hardcoded `admin/admin` credentials replaced with `NC_USER` / `NC_PASS` env-var placeholders and a clear "local dev only" disclaimer at the top. **Quality.** - PHPCS clean on every touched lib file (one 130-char warning on a required sprintf format string; under threshold of error). - Psalm: no errors on the six touched lib files. - PHPStan: no errors on the six touched lib files. - Test bootstrap requires a running NC stack (pre-existing); not exercised here.
…gh the event listener Follow-up to commit 3c90dd1 (PR #147 review). When I replaced the service-locator in `DocuDeskEventHandler` with a constructor/method parameter, I updated the three public event-handler methods (`handleObjectCreated`, `handleObjectUpdated`, `handleObjectDeleted`) to take `PolicyRetroactiveService $retroactive` as an extra argument — but the caller in `DocuDeskEventListener::dispatchEvent` was not updated to pass it through. Result: every ObjectCreated / ObjectUpdated / ObjectDeleted event from OpenRegister hit DocuDesk and immediately failed with: > Too few arguments to function > OCA\DocuDesk\EventListener\DocuDeskEventHandler::handleObjectCreated(), > 5 passed in DocuDeskEventListener.php on line 114 and exactly 6 > expected Visible to operators as a hard failure on creating any DocuDesk-side object that goes through OR's event bus, including dossiers via the new "Create dossier for this folder" UI in Wave 4a. Fix: pull `PolicyRetroactiveService` via the same service-locator the listener already uses for the other DI deps (Logger, MetadataService, SettingsService) and pass it into `dispatchEvent`, which then threads it into all three handler calls. The handlers themselves were already correct. This keeps the listener layer (which lives outside the DI container because it's invoked via NC's event dispatcher reflection) on its existing pattern while the handler layer gets proper method-arg DI.
rubenvdlinde
force-pushed
the
feat/112/entity-publication-policies-impl
branch
from
3af8e7d to
0241142
Compare
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 108/108 | |||
| npm | ✅ | ✅ 529/529 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Coverage: 0% (0/10 statements)
Quality workflow — 2026-05-19 03:22 UTC
Download the full PDF report from the workflow artifacts.
…rs + 2 concerns)
🔴 RBAC 403 swallowed in 3 places — PolicyController::createProhibition,
updateProhibition, and deleteProhibition all caught Exception → 500 but
NOT RuntimeException, so the new assertProhibitionPermission() gate's
RuntimeException fell through to the generic Exception handler and
surfaced as HTTP 500 instead of 403. Added catch (RuntimeException $e) → 403
to each, mirroring the standing-consent counterparts (createStandingConsent,
updateStandingConsent, deleteStandingConsent) which already had the pattern.
RuntimeException is already imported.
🔴 Inline <NcDialog> in StandingConsentIndex.vue:103 violated ADR-004
gate-13 (modal isolation). Extracted into StandingConsentFormModal.vue
mirroring ProhibitionFormModal.vue: parent owns only the open flag +
record-being-edited; modal owns its own form state and emits
submit / update:open / cancel. Removed the inline form scoped styles
from StandingConsentIndex (moved into the modal where they apply).
🟡 ConsentUpdateHandler::guardPolicyPreemptedTransition could be bypassed
via PUT {"policyMatch": null}: that PUT changed neither consentStatus nor
publicationDecision, passed the both-fields check, then array_merge
cleared policyMatch in the record. A follow-up PUT could then change
consentStatus freely because the guard's early-return on policyMatch===null
short-circuited. Added an explicit guard rejecting clearing operations
on pre-empted records — policyMatch is immutable once set. Throws
InvalidArgumentException identifying the existing match without including
the rejected null/empty.
🟡 NcSelect in AnonymizationWidget.vue:113 was missing :input-label
(ADR-004 gate-12 / WCAG 2.1 AA 1.3.1+4.1.2). Added
:input-label="t('docudesk', 'Grondslagen')".
|
@WilcoLouwerse — addressed in 🔴 RBAC 403 swallowed in 3 placesAdded After the fix, 🔴 Inline
|
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 108/108 | |||
| npm | ✅ | ✅ 529/529 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-19 10:09 UTC
Download the full PDF report from the workflow artifacts.
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ❌ | ||||
| phpcs | ❌ | ||||
| phpmd | ❌ | ||||
| psalm | ❌ | ||||
| phpstan | ❌ | ||||
| phpmetrics | ❌ | ||||
| eslint | ❌ | ||||
| stylelint | ❌ | ||||
| composer | ❌ | ❌ | |||
| npm | ✅ | ✅ 575/575 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-22 08:54 UTC
Download the full PDF report from the workflow artifacts.
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ❌ | ||||
| phpcs | ❌ | ||||
| phpmd | ❌ | ||||
| psalm | ❌ | ||||
| phpstan | ❌ | ||||
| phpmetrics | ❌ | ||||
| eslint | ❌ | ||||
| stylelint | ❌ | ||||
| composer | ❌ | ❌ | |||
| npm | ✅ | ✅ 575/575 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-22 09:21 UTC
Download the full PDF report from the workflow artifacts.
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ❌ | ||||
| composer | ❌ | ✅ 108/108 | |||
| npm | ✅ | ✅ 575/575 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-22 09:51 UTC
Download the full PDF report from the workflow artifacts.
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ❌ | ||||
| composer | ✅ | ✅ 108/108 | |||
| npm | ✅ | ✅ 575/575 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-22 09:55 UTC
Download the full PDF report from the workflow artifacts.
phpcbf auto-fixed 65 blank-line/whitespace violations across the new policy + consent services and controllers added in this PR; CI's PHP Quality (phpcs) job will now exit clean. Also added the missing @param checked declaration on ConsentDetail.vue::onToggleAnonymise so the Vue Quality (eslint) job stops warning on the toggle handler.
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ❌ | ||||
| composer | ✅ | ✅ 108/108 | |||
| npm | ✅ | ✅ 575/575 | |||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Coverage: 0% (0/10 statements)
Quality workflow — 2026-05-22 12:06 UTC
Download the full PDF report from the workflow artifacts.
A missing `}, {` between the `standing-consent-verbal-bakker` consent
seed and the `persoonsgegevens` base seed (lines 1196–1197) made the
whole register-config file invalid JSON, which broke every
DossierRegisterConfigTest case (6/6 PHPUnit failures: "Failed
asserting that null is of type array"). With the object boundary
restored, json_decode round-trips cleanly and the suite passes locally.
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ❌ | ||||
| composer | ✅ | ✅ 108/108 | |||
| npm | ✅ | ✅ 575/575 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Coverage: 0% (0/10 statements)
Quality workflow — 2026-05-22 12:31 UTC
Download the full PDF report from the workflow artifacts.
… deps CI's Vue Quality (eslint) job failed with "ESLint couldn't find the plugin eslint-plugin-import" and (stylelint) failed with "stylelint: not found". Both stem from @nextcloud/eslint-config and @nextcloud/stylelint-config declaring their lint plugins as peerDependencies — the docudesk package.json was not pulling them in directly, so npm ci landed a node_modules without them. Adding the peer deps explicitly so the lockfile gets resolved entries for each plugin on the next install.
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ❌ | ||||
| composer | ✅ | ✅ 108/108 | |||
| npm | ❌ | ❌ | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-22 12:46 UTC
Download the full PDF report from the workflow artifacts.
Companion to the previous package.json bump (3240ed9). Ran `npm install --package-lock-only --legacy-peer-deps` so the lockfile now contains top-level resolved entries for eslint-plugin-import, eslint-plugin-vue, eslint-plugin-jsdoc, stylelint, and the other @nextcloud/eslint-config + @nextcloud/stylelint-config peers we just added directly. `npm ci --legacy-peer-deps` (what the workflow runs) will land those plugins in node_modules so the lint scripts can find them.
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ❌ | ||||
| composer | ✅ | ✅ 108/108 | |||
| npm | ✅ | ✅ 575/575 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Coverage: 0% (0/10 statements)
Quality workflow — 2026-05-22 13:31 UTC
Download the full PDF report from the workflow artifacts.
CI's Vue Quality (stylelint) job failed after the previous lint-deps fix with "Cannot resolve custom syntax module postcss-html". That module is the customSyntax used to parse <style> blocks inside .vue files and is declared as a peer of stylelint-config-recommended-vue (which we just pulled in). Same pattern as the earlier peer-dep gap — adding it directly to devDependencies so npm ci lands it in the runner's node_modules.
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 108/108 | |||
| npm | ✅ | ✅ 575/575 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Coverage: 0% (0/10 statements)
Quality workflow — 2026-05-22 13:40 UTC
Download the full PDF report from the workflow artifacts.
| $existingMatch = ($existing['policyMatch'] ?? null); | ||
| if ($existingMatch === null || $existingMatch === '') { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔴 Blocker — UPDATE-side SERVER_CONTROLLED_FIELDS bypass on policyMatch: null records (same class as 4th/5th-pass findings)
The fourth-pass foreach (['matchKind', 'policyMatch'] as $serverOnlyField) immutability check sits at lines 185-200 — but it sits after this if ($existingMatch === null || $existingMatch === '') return; early return. The guard's name says "pre-empted transitions" but matchKind and policyMatch are server-controlled on every record, not just records that already have a policyMatch.
Exploit shape against a record in normal WOO-objection state (policyMatch: null, consentStatus: pending):
PATCH /apps/docudesk/api/consent/<uuid>
{
"policyMatch": "<any-uuid-attacker-picks>",
"matchKind": "standing_consent",
"consentStatus": "consent_given",
"publicationDecision": "publish_with_consent",
"objectionDeadline": null
}Flow: early-return at L170-172 (because $existingMatch === null) → SERVER_CONTROLLED check at L185 is skipped → no assertPolicyMatchReferentValid is called on the UPDATE path → array_merge($existing, $data) at updateConsentStatus:108 accepts everything → record is now persisted as if a standing consent had pre-empted it: WOO objection deadline gone, consent_given without the data subject's actual consent, publication unblocks.
Note that testNoPolicyMatchAllowsArbitraryTransition (the test added in the 4th pass) effectively documents this hole as intended — it asserts the guard returns without throwing on a PATCH that touches consentStatus + publicationDecision on an unmatched record. The test contract is the bug.
Suggested fix: the cleanest shape is to split the function — matchKind and policyMatch are always server-controlled, the consentStatus/publicationDecision lock is what's policy-pre-empted-conditional. Either:
(a) Move the foreach (['matchKind','policyMatch']) block before the L170 early return inside guardPolicyPreemptedTransition, OR
(b) Pull it out into a separate guardServerControlledFields($existing, $data) called from updateConsentStatus ahead of guardPolicyPreemptedTransition, OR
(c) Strip the two fields at the controller boundary (ConsentController::update → ConsentCrudService::updateConsentStatus) mirroring the CREATE-side pattern in ConsentCrudService::createFromRequest:187.
Then flip testNoPolicyMatchAllowsArbitraryTransition to assert REJECTION on a PATCH carrying matchKind / policyMatch, and add a positive test: PATCH {policyMatch: "<uuid>", consentStatus: "consent_given"} on a policyMatch: null record → throws.
| ); | ||
| } | ||
|
|
||
| $this->rulesCache = $rules; |
There was a problem hiding this comment.
🔴 Blocker — loadRules fails OPEN on infrastructure errors; prohibitions silently disappear during OR outages
When loadProhibitions() or loadStandingConsents() throws (OpenRegister down, DB connection lost, schema migration in flight, transient blip), this catch (Exception $e) logs a warning and assigns $rules = [] — empty rule set. Subsequent match() calls return null for every entity. The consent workflow then falls through to the WOO default branch in ConsentService::createConsentRequest:200+ (no match → 4-week objection period, name surfaces in notification + draft).
The standing-consent path failing open is acceptable (worst case: an entity that should have been auto-allowed sits in the objection queue for a few weeks). The prohibition path failing open is a privacy regression:
- Court-ordered erasure subject → name leaks into a publication-decision notification email because OR was unreachable for the 200ms the match call took.
- Victim-protection prohibition silently bypassed → name goes into the 4-week-published draft.
- AVG categorical exemption ignored → categorical PII goes through standard workflow.
Prohibitions are a deny-list; security-critical deny-lists fail CLOSED. The current behaviour optimises for "publication keeps moving when OR is briefly degraded" at the cost of the explicit privacy guarantees the feature exists to enforce.
Suggested fix: split the load so the two paths can fail differently. Rough shape:
try {
$prohibitions = $this->loadProhibitions();
} catch (Exception $e) {
$this->logger->error('PolicyMatchService: failed to load prohibitions — failing CLOSED', ['error' => $e->getMessage()]);
throw new RuntimeException('Prohibition registry unavailable; refusing to process consent without policy data', 0, $e);
}
try {
$standingConsents = $this->loadStandingConsents();
} catch (Exception $e) {
$this->logger->warning('PolicyMatchService: failed to load standing consents — degraded mode, treating as empty', ['error' => $e->getMessage()]);
$standingConsents = [];
}
$this->rulesCache = array_merge($prohibitions, $standingConsents);
return $this->rulesCache;ConsentService::createConsentRequest then surfaces a 5xx and document publication stalls until OR recovers — preferable to letting prohibited names leak. Document this fail-mode distinction in the loadRules PHPDoc explicitly so future edits don't "harmonise" the two paths to symmetric fail-open.
|
|
||
| $status = (string) ($r['consentStatus'] ?? ''); | ||
| if (in_array($status, self::IN_FLIGHT_STATUSES, true) === false) { | ||
| return false; |
There was a problem hiding this comment.
🔴 Blocker — Retroactive sweep cannot override standing consents; prohibitions silently lose the conflict on retroactive paths
This filter skips every record with policyMatch !== null. The reasoning seems sound — "these records already have a match, leave them alone" — but it breaks the canonical "prohibitions win" rule from PolicyMatchService::match on the retroactive code path.
IN_FLIGHT_STATUSES at L61-66 includes consent_given. So a record matched at detection-time by a standing consent (consentStatus: consent_given, policyMatch: <sc-uuid>, matchKind: standing_consent) is in-flight from this sweep's POV. Concrete sequence:
- Operator creates a standing consent for entity X.
- Detection runs against a document mentioning X → standing-consent match → record persisted with
consentStatus: consent_given,policyMatch: <sc-uuid>. Awaiting publication. - Court issues an erasure order against X → operator creates a publicationProhibition for X.
applyProhibitionMutationruns the retroactive sweep →loadInFlightDocumentRecordsfilters out the record from step 2 becausepolicyMatch !== null→ the prohibition does not force-resolve it → publication proceeds with X's name despite the court order.
The spec / docs / detection-time matcher all say "prohibitions beat standing consents" — except here. This is the one place the rule is silently inverted.
Suggested fix: change the predicate from "skip records with any policyMatch" to "skip records already locked by a prohibition":
if (isset($r['policyMatch']) === true
&& $r['policyMatch'] !== null
&& $r['policyMatch'] !== ''
&& ($r['matchKind'] ?? '') === self::KIND_PROHIBITION // only skip prohibition-matched
) {
return false;
}(or import the constant via PolicyMatchService::KIND_PROHIBITION). matchKind is populated by buildConsentData at create-time, so the discriminator is already on the record.
Note that this then also touches finding #1 — when the sweep force-resolves a standing-consent-matched record into a prohibition-matched record, the immutability guard for matchKind / policyMatch must not block it (the sweep is the canonical path that legitimately mutates these fields). The sweep saves via saveObject directly, not via updateConsentStatus, so the guard isn't on its hot path — but worth confirming the fix doesn't accidentally route it through the guarded path. Also add a test in PolicyRetroactiveServiceTest for this exact sequence.
| return $this->error(message: 'Failed to list prohibitions: ', exception: $e); | ||
| } | ||
|
|
||
| }//end indexProhibitions() |
There was a problem hiding this comment.
🔴 Blocker — @NoAdminRequired on the READ endpoints exposes the prohibition list (court orders, victim-protection cases, GDPR Art.17 requestors) to every authenticated user
indexProhibitions / showProhibition / indexStandingConsents / showStandingConsent (lines 71, 93, 191, 213) all carry @NoAdminRequired with no service-level group check — PolicyCrudService::assertProhibitionPermission and assertStandingConsentPermission only fire on list_* / get_* WRITE actions, not on the read paths (grep assertProhibitionPermission(action: — it only appears in createProhibition / updateProhibition / deleteProhibition).
Result: any authenticated Nextcloud user (viewer roles, end users with the app installed, plus guest / federated users where applicable) can:
GET /apps/docudesk/api/policy/prohibitions…and enumerate every prohibition record. Each record contains the exact PII the feature exists to protect — primaryName, legalAuthority (which court / which judge), caseReference, reason (often a sentence like "Slachtoffer van mensensmokkel — slachtofferbescherming Art. 226 Sv" per the doc's own example use cases), matchRules with bsn / kvk as plain strings, validity dates.
The standing-consent list is sensitive but less critically so — names of people who have signed publication consent forms, the consent document reference, scope description. Still arguably need-to-know, not all-tenant.
Suggested fix: add read-side asserters to PolicyCrudService::listProhibitions / listStandingConsents / getProhibition / getStandingConsent, mirroring the existing write-side gates. Either reuse the existing admin groups (docudesk-policy-admins for prohibition reads, docudesk-standing-consent-admins for standing consents) or introduce separate *-viewers groups if reviewer-only access is needed. The doc at docs/features/publication-consent-process.md only addresses WRITE RBAC — should be updated to document the READ side too.
This is more important than the CRUD-side authorization fix from earlier rounds: WRITE-side leak == bad-actor with elevated privileges modifies records (admin-bypass class); READ-side leak == every employee on every NL government tenant can dump the protected-persons list (mass-disclosure class).
| ] | ||
| }, | ||
| { | ||
| "name": "Policy — Prohibitions", |
There was a problem hiding this comment.
🔴 Blocker — Newman collection doesn't exercise the 4 detection-time outcomes the PR description claims
The PR description's test plan states:
Detection-time outcomes via
POST /api/consents: no-match, prohibition match, standing-consent match, both-match (prohibition wins). VerifypolicyMatch+notificationStatus+consentStatus+publicationDecisionper spec.
The collection actually has (Policy folders, L228-388):
GET /api/policy/prohibitionsPOST /api/policy/prohibitions(create)GET /api/policy/prohibitions/{id}DELETE /api/policy/prohibitions/{id}GET /api/policy/standing-consents- 2 negative scope-validation cases (POST without
consentMethod, POST withdocumentId)
Not a single test actually:
- Seeds a prohibition for entity X
- POSTs
/api/consentwithentityText: "X" - Asserts the resulting record has
consentStatus: anonymized,matchKind: prohibition,policyMatch: <seeded uuid>,publicationDecision: anonymize,notificationStatus: skipped
And the symmetric standing-consent path, the both-match conflict path (prohibition wins), and the no-match baseline.
These are the integration contracts that protect against the exact bypass classes the prior review rounds focused on. Without them the Newman gate cannot catch:
- Prohibition silently failing to fire because
loadRulesfails open (finding here) - Server-controlled fields slipping past on CREATE / UPDATE (finding here)
- Prohibition failing to override existing standing consent (finding here)
The PR description is misleading on what's covered. Either amend the description, or — preferred — add a Policy — Detection-time outcomes folder with 4 sub-tests doing the seed → POST /api/consent → assert pattern for each outcome, plus a cleanup post-script that deletes the seeded prohibition / standing consent so the test is idempotent.
Suggested shape (one per outcome):
Pre-request: seed prohibition matching "Jan Janssen"
Test: POST /api/consent with detected entity "Jan Janssen"
Assertions:
- 201/200
- body.matchKind === "prohibition"
- body.policyMatch === <seeded uuid>
- body.consentStatus === "anonymized"
- body.publicationDecision === "anonymize"
- body.notificationStatus === "skipped"
Post-request: DELETE the seeded prohibition
| * | ||
| * @return void | ||
| */ | ||
| public function testInactiveProhibitionResolvesNothing(): void |
There was a problem hiding this comment.
🟡 Concern (Strict) — PolicyRetroactiveServiceTest has zero happy-path coverage; the actual force-resolve write is untested
All 5 tests in this file assert early-return paths:
testInactiveProhibitionResolvesNothing(L103) — bails onactive: falsetestFutureProhibitionResolvesNothing(L127) — bails onvalidFromin the futuretestProhibitionWithoutUuidLogsAndReturnsZero(L152) — bails on missing UUIDtestStandingConsentMutationOnlyInvalidatesCache(L175) —applyStandingConsentMutation()is a no-op by designtestRuleRemovalOnlyInvalidatesCache(L190) —applyRuleRemoval()is a no-op by design
The actual force-resolve mechanism — loadInFlightDocumentRecords → entityMatchesAnyRule → forceResolveToAnonymized → array_merge + saveObject — has zero test coverage. It is the most security-critical method in the service: it overwrites consent records and the correctness of the overwrite payload (preserve notificationSentAt + objectionReceivedAt, clear objectionDeadline, set policyMatch to the right UUID, set consentStatus: anonymized) is exactly what a regression test should anchor.
In Strict mode this is a blocker because the file under test contains the array_merge-and-save write that finding #3 flags as having the wrong skip-filter. A happy-path test would have surfaced that the prohibition silently skips standing-consent-matched records.
It would also surface the asymmetry with ConsentService::buildConsentData, which DOES set matchKind on the CREATE-time path. forceResolveToAnonymized writes the four status fields and policyMatch but does NOT set matchKind — so a retroactively-force-resolved record has policyMatch: <prohibition-uuid> but matchKind: ''. The standing-consent carve-out in ConsentUpdateHandler:229 keys off matchKind === 'standing_consent' so a prohibition-resolve doesn't trip it today, but any future logic keying off matchKind === 'prohibition' will silently miss these records. Latent bug class.
Suggested tests to add:
testProhibitionForceResolvesMatchingInFlightRecord— assertsaveObjectis called with the expected payload (includingmatchKind: 'prohibition'once the code is fixed for symmetry)testProhibitionPreservesAuditTimestamps—notificationSentAt+objectionReceivedAtin the save payload equal the existing valuestestProhibitionSkipsAlreadyProhibitedRecords— record withmatchKind: 'prohibition'is skipped (preserves the idempotency of repeated sweeps)testProhibitionOverridesStandingConsent— record withmatchKind: 'standing_consent'IS swept (validates the fix from finding Documentation Build Failed #3)testProhibitionResolvesAcrossEntityTypeOTHER— entity typeOTHERwildcard
And fix forceResolveToAnonymized to write matchKind: 'prohibition' on the save payload to keep symmetry with buildConsentData.
There was a problem hiding this comment.
Strict re-review — replacing my previously auto-dismissed APPROVE. The fix-window since 9a34ce1 (composer CVE bumps, PHPCS auto-fix, seed JSON repair, npm peer-deps, dev merges) is clean and the prior 5 rounds of security fixes are intact at HEAD. The Strict-mode re-audit on the unchanged 5th-pass surface surfaced 5 🔴 + 1 🟡 that we missed in earlier rounds because the focus was on the existing-match attack path:
🔴 Blockers
- UPDATE-side
SERVER_CONTROLLED_FIELDSbypass onpolicyMatch: nullrecords — same class as the 4th/5th-pass findings on the unmatched-record branch we hadn't audited; the existing test documents the hole as intended. loadRulesfails OPEN on infrastructure errors — prohibitions silently disappear during transient OR outages; deny-lists must fail closed.- Retroactive sweep cannot override standing consents —
policyMatch !== nullfilter inverts the canonical "prohibitions win" rule on the one code path it matters most (court orders arriving after a standing consent). - Policy READ endpoints expose PII to every authenticated user —
@NoAdminRequiredwith no service-level read-side asserter; every employee can enumerate court-ordered erasure subjects + victim-protection cases. - Newman collection doesn't exercise the 4 detection-time outcomes the PR description claims — the integration-test gap that would regression-protect against findings 1-3.
🟡 Concerns (Strict-mode blockers)
PolicyRetroactiveServiceTesthas zero happy-path coverage — all 5 tests are negative paths; the actual force-resolve write is untested.
Edited 2026-05-22: withdrew the @NoCSRFRequired concern (deleted the inline comment) — @NoCSRFRequired on every endpoint is the deliberate ConductionNL org-wide convention, not a finding. Counts updated from 5🔴 + 2🟡 to 5🔴 + 1🟡.
Apologies for the late surfacing — the prior approve treated "CREATE-side bypass closed → done" but the symmetric UPDATE-side and the architectural fail-open / retroactive-priority / read-side-RBAC gaps were below the waterline. Fix the 5 🔴 (and ideally the remaining 🟡), drop the test that documents the hole, add a fresh test that asserts rejection on the new bypass shape, and this should be the last round.
… deny list + read-side RBAC + retroactive override Wilco's sixth-pass strict review surfaced five blockers and two concerns that we missed when earlier rounds focused on the existing- match attack path. This commit closes them: B1 ConsentUpdateHandler — the foreach immutability check for matchKind/policyMatch sat after the policyMatch=null early-return, so a single PATCH on a vanilla WOO record could fabricate a policy match and defeat the objection deadline. Split the check into a new guardServerControlledFields() that runs ahead of guardPolicyPreemptedTransition for every record. Flipped the test that documented the hole, added two regression tests covering the unmatched-record branch (full payload + policyMatch-only shape). B2 PolicyMatchService::loadRules — used to fail open on infra errors, silently dropping prohibitions when OR was briefly unreachable. Split the load so prohibitions throw RuntimeException (fail closed — deny-list semantics) and standing consents degrade to an empty list (fail open — allow-list optimisation). Asymmetry locked in PHPDoc so future edits don't harmonise the two arms. B3 PolicyRetroactiveService — loadInFlightDocumentRecords used to skip every record with a non-null policyMatch, inverting the "prohibitions win" rule on the retroactive code path (a later- arriving court order would silently fail to override an existing standing consent). Narrowed the skip to matchKind=KIND_PROHIBITION only. Also made forceResolveToAnonymized write matchKind into the save payload for parity with ConsentService::buildConsentData — future logic that keys off matchKind on retroactively-resolved records now sees the discriminator. Five new happy-path tests in PolicyRetroactiveServiceTest cover the force-resolve write, audit- timestamp preservation, prohibition-already-locked skip, the prohibition-overrides-standing-consent fix above, and the OTHER-entity-type wildcard sweep. B4 PolicyController READ endpoints — @NoAdminRequired with no service-level group check exposed the prohibition list (court orders, victim-protection cases, GDPR Art.17 requestors) to every authenticated user. Added assertProhibitionPermission / assertStandingConsentPermission to the corresponding list/get methods in PolicyCrudService, mirroring the existing write-side gates. RuntimeException -> 403 added to the four READ controller methods. publication-consent-process.md updated to document the read-side gating. B5 Newman collection — added a "Policy — Detection-time outcomes" folder with seed -> POST /api/consent -> assert -> cleanup sequences for the four canonical match outcomes (no-match, prohibition-only, standing-consent-only, both-match where prohibition wins). Each test gracefully skips when the env hasn't wired the admin groups so the gate is informative locally and strict in CI. C1 NOT APPLIED — Wilco's suggestion to drop @NoCSRFRequired from the six mutating Policy endpoints assumes the only consumer is the same-origin Vue UI (which threads requesttoken via @nextcloud/ axios). DocuDesk's Newman collection sends only Content-Type: application/json, so removing the annotation would make every Policy POST/PUT/DELETE return 412 and break the integration-test gate. ConsentController extends the same base Controller and keeps the annotation; staying consistent with the established project pattern. Will respond on the thread with this rationale. C2 PolicyRetroactiveServiceTest — see B3 (happy-path coverage was the same finding as the missing standing-consent-override test).
Sixth-pass review —
|
| Outcome | Seed → Exercise → Cleanup | Assertions |
|---|---|---|
| no-match (WOO default) | POST /api/consent only | matchKind null, policyMatch null, consentStatus=pending, publicationDecision=pending, notificationStatus=pending |
| prohibition-only | seed prohibition → POST consent → DELETE seed | matchKind=prohibition, policyMatch=seeded UUID, consentStatus=anonymized, publicationDecision=anonymize, notificationStatus=skipped |
| standing-consent-only | seed standing consent → POST consent → DELETE seed | matchKind=standing_consent, consentStatus=consent_given, publicationDecision=publish_with_consent, notificationStatus=skipped |
| both-match (prohibition wins) | seed BOTH → POST consent → DELETE both | matchKind=prohibition, policyMatch=prohibition UUID (not the standing-consent UUID), consentStatus=anonymized |
Each test skips gracefully when the seed POST returns 403/500 (env hasn't wired the admin groups). Cleanup steps are idempotent — they accept 200/204/404.
🟡 Concerns
C2 — PolicyRetroactiveServiceTest has zero happy-path coverage (discussion_r3289264312)
Five new tests added:
testProhibitionForceResolvesMatchingInFlightRecord— full save payload assertion incl. the newmatchKindwrite from B3.testProhibitionPreservesAuditTimestamps—notificationSentAt+objectionReceivedAtsurvive force-resolve.testProhibitionSkipsAlreadyProhibitedRecords— sweep is idempotent across re-runs.testProhibitionOverridesStandingConsent— regression lock for the B3 retroactive override.testProhibitionResolvesAcrossEntityTypeOTHER— wildcard sweep across PERSON + ORGANIZATION rows.
Mocking the OR ObjectService required a local FakeOpenRegisterObjectService stub class in the test file (the real class isn't autoloadable from docudesk's test context — same reason the existing tests skip the OR path). The stub mirrors findAll / saveObject signatures so PHPUnit's createMock honours the named-parameter call sites in production code.
C1 — Removing @NoCSRFRequired from mutating Policy endpoints (discussion_r3289258315) — NOT APPLIED
The premise that the Vue UI is the only consumer doesn't hold in DocuDesk:
PolicyController extends Controller(plain, notOCSController). On a plain Controller, every state-changing method requiresrequesttokenunless@NoCSRFRequiredis set.- The Newman collection sends only
Content-Type: application/json— norequesttoken, noOCS-APIREQUEST: true. Removing the annotation would make every Policy POST/PUT/DELETE return 412 Precondition Failed and break the integration-test gate (including the new B5 folder above which seeds via POST /api/policy/prohibitions). ConsentController(sameextends Controllerbase, comparable surface) keeps@NoCSRFRequiredon every mutating method — this is the established project pattern.
The architecturally-correct path forward is to convert PolicyController to OCSController (which doesn't enforce CSRF; accepts OCS-APIREQUEST: true instead). That's a bigger refactor and inconsistent with ConsentController. Happy to do that work in a separate PR if you'd prefer that direction — but reverting the annotation removal on this PR's scope was the lower-risk call.
Status
- PHPUnit: 258/258 (540 assertions) green locally.
- PHPCS: 0 errors. Trailing warnings are pre-existing
@specPHPDoc nudges, unchanged by this commit. - Touched: 9 files, +916 / −53.
Anything left I can pull through on the next pass — or shall I open the follow-up for the OCSController migration?
Quality Report — ConductionNL/docudesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 108/108 | |||
| npm | ✅ | ✅ 575/575 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Coverage: 0% (0/10 statements)
Quality workflow — 2026-05-22 15:39 UTC
Download the full PDF report from the workflow artifacts.
2 participants
Summary
Implements
entity-publication-policies(Wave 1.2 of the "anonimiseren bij de bron" bundle). Adds an entity-level policy layer that pre-empts the publication-clearance workflow at detection time, with deterministic conflict resolution between deny-list (prohibitions) and allow-list (standing consents) surfaces.Backend
publicationProhibitionschema in the consent register — entity-level deny rules with 4 seed records (court order, minor protection, undercover officer, AP categorical exemption).publicationConsentextended with thescopediscriminator (document/entity) and the entity-scope-only field set (matchRules,validFrom,validUntil,active,consentMethod,consentDocument,consentScope), plus apolicyMatchfield linking a per-document record back to its driving prohibition / standing consent. 4 standing-consent seed records covering the consentMethod variants.PolicyMatchService— detection-time matcher with in-memory per-request cache, four match types (exact,normalized,bsn,kvk), deterministic conflict resolution (prohibition wins; multi-prohibition broken by lowest UUID lex).PolicyRetroactiveService— force-resolves in-flightscope:"document"records toanonymizedwhen a prohibition is added or widened; intentionally no-op for standing-consent creation (future detections only — privacy default wins on retroactive sweep). Dispatched fromDocuDeskEventHandlervia payload-shape detection.ConsentService::createConsentRequestnow consultsPolicyMatchServicebefore defaulting to WOO; branches into 4 detection-time outcomes (no match / prohibition / standing consent / both → prohibition). New scope-validation gate at write time.ConsentUpdateHandlerrejectsconsentStatustransitions on records pre-empted by a policy.PolicyController+PolicyCrudServicewith 10 routes underapi/policy/{prohibitions,standing-consents}. Service-level RBAC gate: standing-consent writes requiredocudesk-standing-consent-adminsgroup (returns 403); prohibition writes restricted todocudesk-policy-adminsat schema level.Frontend
scope:"document", adds "policy" badge on rows whosepolicyMatchis non-null.policyMatchreferent kind: prohibition → ON+locked; standing consent → OFF+overridable (override recordspublicationDecision: "anonymize"while preservingconsentStatus: "consent_given"andpolicyMatch).AnonymizationWidget.vue— split upload/extract from anonymise with a manual review step exposing the Wave 1.3 grondslagen + skip-anonymise PATCH on each detected entity. Intentionally simple; example shape for the frontend team, not a production publication-prep page. New Pinia storesprohibitionStore,standingConsentStore.Other
templates/index.phpandtemplates/settings/admin.phpwere never updated to load the newdocudesk-shared-vendor.js+docudesk-shared-nc-vue.jschunks, leaving the entry bundle waiting onchunkOnLoadforever and rendering nothing. Worth a cross-check on pipelinq / procest — they got the same split.docs/features/publication-consent-process.mdcovering the three-layer evaluation, retroactive asymmetry, UI toggle semantics, three admin surfaces, RBAC defaults. CHANGELOG entries under Added / Changed / Behavior changes.Test plan
imported_config_docudesk_versionre-imports to 8.0.0; both schemas present (GET /api/registers/consent?_extend=schemas).GET /apps/docudesk/api/policy/prohibitions→ 4 seeds;GET /apps/docudesk/api/policy/standing-consents→ 4 seeds.consentMethod,documentIdonscope:"entity").POST /api/consents: no-match, prohibition match, standing-consent match, both-match (prohibition wins). VerifypolicyMatch+notificationStatus+consentStatus+publicationDecisionper spec.pendingconsent → create matching prohibition → record nowanonymized+policyMatch. Repeat with a new standing consent → in-flight record unchanged.publicationDecisiononly); legacy UX forpolicyMatch: null.consentStatuschange on a record with non-nullpolicyMatch→ 400.composer check:strictclean;openspec validate entity-publication-policiesclean.