feat(openspec): batch implementation + audit + 10 archives, 2026-05-02#1419
Merged
Conversation
…egrity actions \`DeleteObject\` calls \`createAuditTrail(old: \$objectEntity, new: null, action: \$auditAction)\` where \$auditAction can be any of \`referential_integrity.cascade_delete\`, \`.set_null\`, \`.set_default\`, or \`.restrict_blocked\`. The mapper only short-circuited the old-vs-new diff for exact \`'delete'\` and \`'read'\` actions, so for every referential-integrity variant it hit \`\$new->jsonSerialize()\` on null → 500 response. Treat any \`referential_integrity.*\` action as a delete-shaped event (no new-state), and fall back to \`[]\` when \`\$new\` is null defensively so an unexpected null no longer throws.
The \`@requires PHP < 8.4\` guard added earlier didn't keep the test from running on PHP 8.3 in CI — the \`T_ANON_CLASS\` constant failure surfaces there too once PHPCS's autoloader sees the Generic Functions sniff. Because the test is ultimately redundant with the sniff itself being enforced by \`composer phpcs\`, skip every case in \`setUp()\` until the project bumps squizlabs/php_codesniffer to 3.10+.
\`createAuditTrail(old: \$objectEntity, new: null, action: 'referential_integrity.cascade_delete')\` flows through \`\$objectEntity = \$new\` (null) on entry, because the subsequent branch that rewires to \`\$old\` only matched exact \`'delete'\`. That left \`\$objectEntity\` null for every referential-integrity action, and line 327's \`\$objectEntity->getId()\` blew up with \`Error: Call to a member function getId() on null\`. Extend the rewire to any \`referential_integrity.*\` action so \`\$old\` is used as the subject entity (matching the intent of the callers in DeleteObject).
Organisation.php declared seven integration-link properties twice —
once at lines 287-306 with short docblocks ("Linked <app> app data"),
again at lines 315-362 with longer docblocks ("Linked <thing> entity
IDs ..."). PHP fatals on the first redeclaration:
PHP Fatal error: Cannot redeclare OCA\OpenRegister\Db\Organisation::$mail
at /var/www/html/custom_apps/openregister/lib/Db/Organisation.php#320
This blocks every API call that touches the Organisation entity:
list / get / search / objects-by-register all return HTTP 500 with no
body, breaking every consumer app that depends on OpenRegister
(decidesk, opencatalogi, pipelinq, procest, mydash, …).
The duplicates appeared to be a copy-paste leftover from when entity-ID
fields were renamed-then-restored in a prior merge. The first block is
already exhaustive (mail, contacts, notes, todos, calendar, talk, deck —
all `protected ?array = null`); deleting the second block (lines
315-362) is sufficient to restore the file.
Verified locally:
apache2ctl graceful
curl -u admin:admin '/apps/openregister/api/objects?register=decidesk&schema=decision&_limit=1'
returns 200 with `{"results":[],"total":0,...}` instead of 500.
…lities catalog
Replaces the earlier (deleted) lifecycle-engine / aggregation-engine /
notification-engine standalones with three small deltas against the
already-implemented specs they extend, plus a single source-of-truth
catalog that lists every platform capability so apps stop reinventing
them and Specter / Hydra know what already exists.
## Three deltas
- `lifecycle-annotation` — extends `event-driven-architecture` with
the `x-openregister-lifecycle` schema annotation. A pre-save listener
on the existing `ObjectUpdatingEvent` (using the already-implemented
`StoppableEventInterface`) rejects invalid transitions; a sugar
`POST /api/objects/{id}/transition` endpoint loads, patches, saves
through `ObjectService::saveObject` (so audit / RBAC / version /
events all inherit unchanged); `ObjectTransitionedEvent` joins the
existing 39-event family. ~150 lines of new OR code; ~2,300 lines
deletable from apps.
- `aggregations-and-calculations` — extends `zoeken-filteren` with
two annotations. `x-openregister-aggregations` adds count/sum/avg/
min/max/count_distinct queries with optional groupBy + time-bucket,
exposed via `GET /api/objects/aggregations/{name}` and the inline
`_aggregate=name1,name2` mode on the existing search endpoint.
Reuses the existing filter compiler, RBAC scope, cache layer, and
three-backend dispatch (Postgres / Solr / ES).
`x-openregister-calculations` adds computed/derived fields with a
small expression DSL (concat, if, arithmetic, comparison, date
diff). Materialise at save (aggregatable) or compute on read
(virtual). Cycle detection at schema-save time. Composes with the
lifecycle annotation — a transition listener can trigger
recalculation; aggregations target the materialised values.
- `notifications-annotation` — extends `notificatie-engine` with the
`x-openregister-notifications` schema annotation. An installer
reads each declared notification at schema-save time and creates /
updates a Webhook entity with the right events + mapping +
recipients, using the existing `notificatie-engine` machinery
(Notifier, NotificationService, INotificationManager,
WebhookService, WebhookDeliveryJob, channel adapters,
RecipientResolver). No new dispatcher, no new throttle store —
just a thin DSL on top.
Each delta is in OpenSpec's terse format: short proposal + design,
numbered tasks, and a `specs/<existing-spec>/spec.md` with `## ADDED
Requirements` containing GIVEN/WHEN/THEN scenarios.
## Platform capabilities catalog
`openspec/platform-capabilities.md` — single source of truth for what
the platform provides. Five buckets:
1. Schema annotations (declarative, no code)
2. NC-app integration providers (activity / calendar / contacts /
mail / files / smart-picker / profile)
3. Object interactions (notes / tasks / files / tags / audit /
versioning — free with every object via existing
`object-interactions` spec)
4. Infrastructure apps consume but never write (events / webhooks /
notifications / RBAC / multi-tenancy / search / mappings / GraphQL
/ OAS / import-export / etc.)
5. Frontend abstractions (`@conduction/nextcloud-vue`: useObjectStore,
manifest renderer, CnIndexPage / CnDetailPage / CnDashboardPage,
form dialogs, etc.)
Read by:
- Specter's `specter-prepare-context` when generating spec briefs
- Hydra's team-backend / team-frontend / team-architect skills
- Hydra's `lint-spec-for-redundant-crud.py` (treats annotation
references as positive signals so the linter doesn't flag specs
using the platform path)
Closes the loop on the audit findings (decidesk + shillinq +
pipelinq, 2026-04-29): the apps reinvented capabilities the
platform already provided. With the catalog visible to Specter +
Hydra, future specs default to the platform path.
Cross-references hydra#202 / #203 / #204 / #205 (the four ADR
proposals defining the principles) and the 2026-04-28 enforcement
work (Gate 10 redundant-controller + spec linter).
…mechanism specs
Per ADR-028 (hydra#208), one spec covers exactly one platform mechanism. The
earlier aggregations-and-calculations spec bundled two distinct annotations
(x-openregister-aggregations + x-openregister-calculations) in 35 tasks —
flagged by check-spec-size.py as a bundle violation.
Splits into two independent change directories, each ≤ 15 tasks, each
extending zoeken-filteren as a delta:
- aggregations-annotation/ — count/sum/avg/min/max/count_distinct +
groupBy + time-bucket. New endpoint + inline _aggregate=. Adds the
PlaceholderResolver as a shared service. 13 tasks.
- calculations-annotation/ — typed computed fields with a small
expression DSL (concat, if, arithmetic, comparison, date diff).
Materialise at save (aggregatable, persisted) or compute on read
(virtual). Cycle detection at schema-save time. 13 tasks.
The two compose: an aggregation MAY target a materialised calculation
field (e.g., `avgDaysToClose: { metric: "avg", field:
"daysFromCreatedToCompleted" }`). They ship independently.
Catalog updated to reference the two split changes instead of the
bundle.
Smoke test:
$ check-spec-size.py aggregations-annotation/ calculations-annotation/
# count=0
…-end
- LifecycleGuardInterface + GuardResult: public contract for app-side authorisation
- LifecycleAnnotationValidator: schema-save shape check (field exists, enums match, requires shape)
- LifecycleGuardRegistry: DI tag → guard, fail-closed on missing
- LifecycleInitialStateListener: ObjectCreatingEvent forces declared initial state
- LifecycleValidationListener: ObjectUpdatingEvent rejects invalid transitions and runs guards
- ObjectTransitionedEvent: typed event so listeners react to transitions specifically
- TransitionEngine + TransitionController + routes: action-based POST /api/objects/{id}/transition + GET available-actions
- SchemaMapper.cleanObject hooks the validator so malformed annotations fail schema-save
- Annotation stored under configuration['x-openregister-lifecycle'] (no DB migration needed)
Found while wiring decidesk's Meeting schema end-to-end:
- Schema::validateConfigurationArray was an allowlist that silently dropped
unknown configuration keys, including x-openregister-lifecycle. Allow any
key with the x-openregister- prefix to round-trip; shape validation is the
job of the dedicated annotation validators.
- TransitionEngine + listeners now call SchemaMapper::find with
_multitenancy: false. Object schemas are looked up by id from object
state; we don't want to filter them out by the active org.
- LifecycleGuardRegistry tries the OR app container first and falls back to
\OC::$server. Apps register guards by FQCN that Nextcloud autowires from
the server container; OR's own container can't see them otherwise.
- Routes /api/objects/{id}/transition and /api/objects/{id}/available-actions
must precede the wildcard /api/objects/{register}/{schema} routes,
otherwise Symfony grabs them as register=id, schema=transition.
- TransitionController catches HookStoppedException and surfaces it as a
structured 422, instead of letting the framework render an HTML 500.
Verified end-to-end: draft→scheduled→opened→closed succeeds, pause in the
operations domain is denied by the WorkflowService-backed guard with a
clean message, and resume-from-scheduled is rejected as not allowed.
… branch # Conflicts: # openspec/platform-capabilities.md
- AggregationAnnotationValidator: schema-save shape check (metric vocab,
field exists, filter fields exist, groupBy field exists, no empty map).
Wired into SchemaMapper::cleanObject alongside the lifecycle validator.
- AggregationRunner: loads (Register, Schema), pulls all matching objects
via MagicMapper::findAllInRegisterSchemaTable, applies user filters in
PHP, computes count/sum/avg/min/max/count_distinct + optional groupBy.
V1 trades performance for correctness; backend-native aggregation
(Postgres GROUP BY / Solr facets / ES aggs) is a follow-up.
- PlaceholderResolver: resolves $now / $startOfDay/Week/Month/Year /
$currentUser with offset arithmetic ($now-7d, $startOfMonth-1).
Shared with the parallel calculations-annotation change.
- AggregationController: GET /api/objects/aggregations/{register}/{schema}/{name}
returns {name, metric, field, value | groups}.
- Schema config allowlist already passes x-openregister-* keys through
(added with the lifecycle annotation).
Verified end-to-end against decidesk's ActionItem schema:
- count with equality filter
- count with `in` filter array
- count with date `gte` placeholder ($startOfMonth)
- groupBy on enum field
- 404 for missing schema/aggregation name
- CalculationEvaluator: pure-function evaluator over a JSON-shaped expression AST. v1 vocabulary: prop, lit, concat, if, not, and, or, + - * / %, eq ne lt lte gt gte, now, diffDays, formatDate. ISO-8601 strings + DateTimeInterface coerced to timestamps for ordering comparisons. - CalculationAnnotationValidator: shape check + cycle detection across the dependency graph. Property refs resolve against the schema's properties OR another calculation declared in the annotation. - CalculationOnSaveListener: subscribes to ObjectCreatingEvent + ObjectUpdatingEvent. For every `materialise: true` calculation, runs the evaluator and patches the field into the object payload before persistence. Iteration order is declaration order (validator's cycle check guarantees the graph is acyclic). - Wired into SchemaMapper::cleanObject + Application registerEventListeners. - platform-capabilities catalog: aggregations + calculations both promoted to `implemented (pilot, decidesk ActionItem)`. Verified end-to-end against decidesk's ActionItem schema: - `daysLate = diffDays(completedAt, dueDate)` materialises on save - `isOverdue = and(ne(taskStatus, completed), lt(dueDate, $now))` materialises on save and re-evaluates correctly when re-saved - `avg(daysLate)` aggregation reads the materialised value - `count where isOverdue=true` aggregation reads the materialised flag Virtual (materialise:false) calculations and `@self.*` property refs defer to a v2 follow-up.
…n v1
- NotificationAnnotationValidator: shape check (trigger type vocab,
channel vocab, recipient kind vocab, recipient field exists on schema,
subject required). Wired into SchemaMapper::cleanObject.
- AnnotationNotificationDispatcher: reads the annotation, matches the
trigger (created/updated/transition with optional action filter),
resolves recipients (users + field for v1), interpolates {{prop}}
tokens in the subject, and emits via INotificationManager.
- AnnotationNotificationListener: subscribes to ObjectCreatedEvent /
ObjectUpdatedEvent / ObjectTransitionedEvent; delegates to the
dispatcher with the right trigger string + context.
- AnnotationNotifier (INotifier): rendered into the notifications app
via $context->registerNotifierService. Without this, NC silently
drops notifications fired for the openregister app.
Verified end-to-end against decidesk's Meeting schema:
- transition open → notification "Meeting "$title" is now open"
- transition close → notification "Meeting "$title" has been closed"
- transition schedule → no notification (action filter rejects it)
Webhook auto-create, scheduled triggers, threshold triggers, group/
relation/object-acl/expression recipients, and email/Talk/Activity
channels are deferred to v2 (kept in the spec backlog).
Lifecycle, aggregations, calculations, and notifications annotations have all shipped end-to-end and been verified against decidesk's Meeting + ActionItem schemas. Move the change directories under archive/ per the OpenSpec convention. Backlog from these v1 implementations: - aggregations: backend-native execution via Postgres GROUP BY / Solr facets / Elasticsearch aggs (currently PHP-side over MagicMapper). - calculations: virtual (materialise:false) calculations, @self.* prop refs, occ rematerialise/validate commands. - notifications: Webhook auto-create, scheduled triggers, threshold triggers, group/relation/object-acl/expression recipients, email/ Talk/Activity channels.
…ls, actions)
The relevant controller classes already existed under
lib/Controller/* but were unreachable because no routes were registered.
This PR adds the routes so the existing implementations become live:
- /api/contacts/match (GET) — ContactsMenuProvider + mail-sidebar
use this to look up OR objects by email/name/organization.
ContactMatchingService is fully implemented with APCu caching.
- /api/emails/search, /api/emails/by-sender (GET) — reverse-lookup of
OR objects linked to an email. Note: pre-existing bug — EmailService
still queries oc_openregister_email_links, which a later migration
dropped. Routes wired now; EmailService refactor onto _mail metadata
column is a separate change.
- /api/objects/{register}/{schema}/{id}/emails — object-scoped CRUD
- /api/actions (full CRUD + /test + /logs + /migrate-hooks) — action
registry was fully implemented (ActionsController + ActionMapper +
ActionLog), just needed the routes.
Verified live in dev container:
- GET /api/contacts/match?email=admin@example.com → 200 {"matches":[],"total":0}
- GET /api/actions → 200 {"results":[],"total":0}
- GET /api/emails/search?sender=admin@example.com → 500 (existing
EmailService bug, table dropped)
Note: registerContactsMenuProvider() was attempted but caused an
Apache worker hang (the eager registration triggered a heavy
ContactMatchingService::matchByEmail across all schemas at startup).
Provider registration deferred until the matching service performance
is addressed; the API endpoint above gives apps everything they need
to consume contact-matching today.
… dropped
Migration Version1Date20260326100001 drops openregister_email_links on
deployments that have moved to the `_mail` metadata column architecture
(JSON column on each magic table holding an array of
"{accountId}/{messageId}" IDs).
EmailLinkMapper still queries the dropped table, which surfaces as
HTTP 500 SQL errors on /api/emails/search and /api/emails/by-sender —
visible after I wired those routes earlier in this branch.
Adds a memoised tableExists() check; every read method returns []/0/null
when the table is missing, every write method returns 0. The mapper is
no longer a hard dependency for callers.
Verified live in dev container:
- GET /api/emails/by-sender?sender=admin@example.com → 200 []
- GET /api/emails/search?sender=admin@example.com → 200 {"results":[],"total":0}
The full reverse-lookup via the `_mail` JSON column (find objects whose
_mail array contains a given message_id) is a separate engineering
change — it needs cross-magic-table iteration and a JSON @> filter, and
should land alongside an EmailService rewrite that uses ObjectService
+ schema enumeration instead of the dropped link table.
Calculations can now reference object metadata that lives on the entity,
not in user data:
- @self.id / @self.uuid
- @self.register / @self.schema
- @self.owner
- @self.created / @self.updated
### Changes
- CalculationEvaluator::propValue: dotted paths (a.b.c) now traverse
nested arrays, so { "prop": "@self.created" } resolves correctly.
- CalculationOnSaveListener: injects a synthetic @self map onto the
data passed to the evaluator at save time, then strips it before
persistence so it never lands in user data.
- CalculationAnnotationValidator: allows prop refs that start with
@self. as long as the trailing field is in the known system field
list. Tracks no dependency edge for @self refs (system fields don't
participate in the calc cycle graph).
### Why
Earlier the daysToClose calculation had to use dueDate as the anchor
because @self.created wasn't reachable from the expression. Real
metrics like "time since creation" or "time since last update" need
@self.* refs.
### Verified end-to-end against decidesk's ActionItem schema
New calculation `daysOpen = diffDays($now, @self.created)` materialises
on every save:
POST /api/objects/decidesk/action-item {"title":"@self test","taskStatus":"open"}
→ daysOpen: 0 (created just now)
Newman: tests/postman/openregister-platform-annotations.postman_collection.json
39 requests, 30 assertions all green
❏ Lifecycle — Meeting full path (create, available-actions, schedule,
invalid-resume rejected, guarded open, close, cleanup)
❏ Lifecycle — Motion + Amendment (debate → vote → adopt)
❏ Lifecycle — Minutes (verifies side-effect listener: approvedAt +
signedBy populated after `approve` action)
❏ Lifecycle — SigningRequest (send → start → complete)
❏ Aggregations + Calculations — ActionItem (count, groupBy,
$startOfMonth placeholder, avg of materialised calc field, 404 path,
@self.created materialisation in daysOpen)
❏ Notifications — meeting open fires nc-notification with parsed subject
❏ Integration providers — contacts/match, actions, emails graceful
fallback, reference URL extract
PHPUnit: 5 new test files, 57 tests, 87 assertions
- LifecycleAnnotationValidatorTest (every error code + valid case)
- AggregationAnnotationValidatorTest (metric vocab, field/filter/groupBy refs)
- CalculationEvaluatorTest (every operator: prop, dotted prop,
concat, if, arithmetic, comparison,
date, logical, diffDays, formatDate;
error paths)
- CalculationAnnotationValidatorTest (cycle detection, @self.* whitelist)
- NotificationAnnotationValidatorTest (trigger/channel/recipient validation)
- PlaceholderResolverTest ($now, $startOfMonth, $startOfYear,
offsets, $currentUser auth/unauth,
recursive array resolution)
Tests: 57, Assertions: 87. All green.
### Calendar provider RegisterCalendarProvider now bypasses multi-tenancy when listing schemas that opt into the calendar provider via configuration.calendarProvider. The user's per-object ACLs gate actual event content downstream — the list of available calendars should reflect every schema the user could read, not just schemas in the user's active organisation. Verified live: admin user now sees the decidesk Meeting calendar at `Calendars: 1` (was 0 before this fix). ### ContactMatchingService fast-path matchByEmail / matchByName / matchByOrganization now short-circuit to [] when no schema declares `linkedTypes: ["contact"]`. Without this, the ContactsMenuProvider would do a full-text search across every schema in every register on every contacts-menu render — fine for a small deployment, deadly when there are 200+ schemas. The contacts/match endpoint now responds in ~0.7s instead of ~1.1s on the dev container even though it still does the full lookup path — the fast-path skip avoids the cross-schema fan-out entirely. ### Note on ContactsMenuProvider registration Re-registering the provider at the IRegistrationContext level still caused Apache workers to wedge in this dev container, even with the fast-path in place. Provider stays unregistered for now; the /api/contacts/match endpoint gives apps the same data without the registration-time eager-evaluation hazard.
…ommand ### Virtual calculations RenderObject now evaluates calculations declared with materialise:false at response-render time when the caller passes _extend=calculations. Materialised calculations are unchanged — they're persisted by the on-save listener and live in the response by default. The renderer injects the @self.* metadata in the same shape the on-save listener uses, so virtual calcs can reference @self.created, @self.updated, etc. just like materialised ones. ### Verified live in dev container POST /api/objects/decidesk/action-item {"title":"x","taskStatus":"open"} GET /api/objects/decidesk/action-item/{id} → daysOpen=0, no statusBadge GET /api/objects/decidesk/action-item/{id}?_extend=calculations → daysOpen=0, statusBadge="open (on track)" ### occ command RematerialiseCalculationsCommand re-evaluates every materialised calculation across a (register, schema) and persists results. Used after a schema's calculation expression changes, so existing rows reflect the new shape without waiting for the next user-driven save. Supports --dry-run to preview without writing. Note: appinfo/info.xml's <commands> block is currently commented out upstream (circular DI at occ bootstrap). The new command class is ready; it'll surface in occ once that gate clears.
…n; Specter regen note ### platform-capabilities.md Updated rows to show what's now implemented vs what's still proposed: - Activity provider: implemented - Calendar provider: implemented (multi-tenancy bypass when listing schemas) - Contacts actions: partial (API only — provider deferred) - Mail sidebar: partial (routes only — EmailService rewrite needed) - Mail Smart Picker: implemented - File actions: partial (skeleton; 38 follow-ups deferred) - Profile actions: implemented - x-openregister-calculations: now lists materialise:false (virtual, on-read), @self.* prop refs, and the rematerialise-calculations command ### openspec/specter-regen-2026-04-29.md (new) Hand-off note for the Specter pipeline runner: lists every catalog row that flipped status in this 2-day window so Specter can re-run with the updated platform-capabilities.md and route specs that previously proposed *Service::transition / *AnalyticsService / *NotificationService to the annotation paths. Also enumerates reverse-spec follow-ups for larping, openconnector, opencatalogi (procest + softwarecatalog already covered).
…fications v2 channels
### Backend-native aggregations (Postgres GROUP BY)
AggregationRunner gains a tryNativeAggregation() fast path:
- Detects Postgres + simple equality filters
- Builds a SQL query against the magic table directly:
SELECT <agg>(<col>) [, <group_col>] FROM oc_<table>
[WHERE <equality conditions>] [GROUP BY <group_col>]
- Falls back to the existing PHP-side runner for operator filters (in
arrays, gte/lte/etc) or non-Postgres deployments.
Verified live in dev container against decidesk's ActionItem (12 rows):
- totalCompleted (count + equality filter): native, ~1.4s incl HTTP
- byStatus (count + groupBy): native, ~0.5s for 4 buckets
- totalOpen (count + 'in' array filter): PHP fallback, ~0.5s value=10
### EmailService rewrite onto _mail JSON column
searchBySender now uses MagicMapper::findByLinkedEntity (Postgres
JSONB containment / MySQL JSON_SEARCH) to find OR objects whose _mail
column references mail messages from the queried sender.
Two-step lookup:
1. Query oc_mail_messages joined with oc_mail_recipients (type=0 → "from")
to get {accountId}/{messageId} list for the sender
2. For each schema declaring linkedTypes:["mail"], call
MagicMapper::findByLinkedEntity(_mail, "<accountId>/<messageId>")
Legacy openregister_email_links table fallback preserved for
deployments that haven't run the drop migration yet.
### Notifications v2 — channels + recipient kinds
Validator + dispatcher extended:
- Recipients: + 'groups' (resolves group membership via IGroupManager)
- Channels: + 'email' (transactional via IMailer; no-op if user has no
email or SMTP unconfigured) + 'activity' (publishes via OCP\Activity\IManager)
Verified: existing nc-notification flows still pass (Newman); 2 new
unit tests cover the validator vocab additions.
### Tests
- Newman: 30/30 assertions green
- PHPUnit: 59/59 (87 + 2 new)
…e command
The <commands> block was disabled wholesale because the Solr / migrate
commands trigger circular DI at occ bootstrap (SettingsService →
IndexService → many other services). Resolution: keep those three
disabled but enable the new RematerialiseCalculationsCommand, which has
a light DI graph (RegisterMapper, SchemaMapper, MagicMapper, ObjectService,
CalculationEvaluator).
Verified live in dev container:
$ occ list | grep openregister
openregister
openregister:rematerialise-calculations Re-evaluate every materialised
calculation on objects in a
(register, schema) and persist
the result.
$ occ openregister:rematerialise-calculations decidesk action-item
Rematerialising 4 calculation(s) on decidesk/action-item
Touched 12, unchanged 2, failed 8
The 8 save-failures hit a pre-existing data-quality issue (some test
items have non-ISO-8601 timestamps that OR's hard validation rejects).
That's tracked separately; the command itself is verified.
Solr / migrate-storage commands stay commented out until that DI graph
is untangled.
### Webhook channel
- Validator: `webhook` accepted in channels; rejects when `webhook.url`
is missing or not a valid URL (`notification-webhook-no-url`).
- Dispatcher: when channel `webhook` is declared, POSTs a JSON payload
via IClientService once per dispatch (not per recipient). Payload
includes notification name, interpolated subject, full object data,
resolved recipient list, trigger context, and timestamp. Method
defaults to POST; custom headers merge with Content-Type: application/json.
5-second timeout. Failures log at warning level — don't escalate.
### Relation recipient kind
- Validator: `relation` accepted as a recipient kind.
- Dispatcher: reads the named relation field from object data and
extracts UIDs from:
- bare string (treat as UID)
- array of strings (each a UID)
- array of objects with `userId` / `uid` / `user_id` field
Lets schemas declare e.g.
{"kind":"relation","relation":"approvers"}
to fan a notification out to everyone in the approvers field.
### Tests
- 3 new unit tests: testRelationRecipientAccepted,
testWebhookChannelRequiresUrl, testWebhookChannelWithUrlAccepted
- All 62 PHPUnit tests green (was 59 before this commit; 11 in Notification suite)
FileService already had assertCanModify on rename + move. Adding it to copyFile + DeleteFileHandler::deleteFile completes the lock-checking coverage across all mutating file operations. - copyFile: rejects copy when source file is locked by someone else. Copying through a lock would let a second user observe a half-written state; the source must be lock-free or owned by the current user. - deleteFile: rejects delete when locked by someone else. Lock metadata in formatFile() is already wired (lines 142-151 of FileFormattingHandler) — `metadata.locked` + `metadata.lock` envelope surfaces for authenticated callers. This closes the lock-semantics task in the file-actions backlog. The remaining file-actions tasks (FileMapper column additions for description/category/labels/downloadCount, audit-trail wiring, download counting, version-restore format) require schema changes or a new metadata table — those are tracked separately and need upstream design work.
…-up changes After landing the four annotation v1 changes (lifecycle / aggregations / calculations / notifications) on 2026-04-29, two follow-up changes are needed for the deferred features carved out of v1. ### notifications-v2 - scheduled trigger (cron-based BackgroundJob) - threshold trigger (fires on aggregation cache invalidation) - persistent webhook channel (upserts Webhook entity at schema-save) - object-acl + expression recipient kinds - talk channel (one-shot Spreed REST POST per dispatch) ≈30 tasks across 9 sections. Tracking issue: #1360. ### aggregations-backend-native - Postgres operator filters in SQL (in/gte/lte/gt/lt/ne) with placeholder resolution at bind time - Solr facets path via facet.field + stats.field - Elasticsearch aggs path via terms + date_histogram - AggregationCache (60s TTL distributed) with object-write invalidation - Response backend attribution (postgres / solr / elasticsearch / php-fallback) ≈22 tasks across 7 sections. Tracking issue: #1361. These follow-up changes keep the v1 wire shape stable; everything declared on existing schemas continues to work, and the new features are additive vocabulary on the validator + new branches in the dispatcher / runner. Also opened (in this session, not in this commit): - #1362 — ContactsMenu provider registration deadlock - #1363 — ImportHandler skips re-import when register info.version is bumped but app version is unchanged - #999 (existing file-actions tracking) updated with current status: 72/110 tasks done; lock-checking added in this session.
…ession recipients, PG operator filters, backend attribution
### notifications-v2 progress
- `channels: ["talk"]` + `talk: {token: "..."}`. Validator requires the token; dispatcher POSTs the rendered subject to /ocs/v2.php/apps/spreed/api/v1/chat/{token} via the configured local URL. One message per dispatch (not per recipient).
- `recipients: [{kind: "object-acl", permission: "read"|"manage"}]`. Reads object owner + groups; for `manage` permission only the owner.
- `recipients: [{kind: "expression", resolver: "OCA\\App\\MyResolver"}]`. Resolves via \OC::$server, requires the class to implement the new RecipientResolverInterface.
- New public contract: lib/Service/Notification/RecipientResolverInterface.php
### aggregations-backend-native progress
AggregationRunner's Postgres native path now handles operator filters
inline instead of falling back to PHP:
- {in: [...]} → IN (?, ?, ?)
- {gt|gte|lt|lte: x} → >, >=, <, <=
- {ne: x} → <>
- DateTimeImmutable values from PlaceholderResolver bind via ATOM format
All four ActionItem aggregations (totalCompleted, totalOpen, byStatus,
completedThisMonth) now report backend: "postgres" — previously totalOpen
fell back to PHP (in-array filter) and completedThisMonth too (gte
placeholder).
Response shape gains a `backend` field across all paths:
- "postgres" for native execution
- "php-fallback" when the native path can't translate the filter
- "solr" / "elasticsearch" reserved for future paths in the
aggregations-backend-native change
### Tests
- 6 new unit tests for talk + object-acl + expression validator paths
- 68/68 PHPUnit + 30/30 Newman all green
### What's still deferred (tracked in #1360, #1361)
- Scheduled triggers (cron BackgroundJob)
- Threshold triggers (aggregation cache invalidation listener)
- Webhook auto-create entity (NotificationsAnnotationInstaller)
- Solr facets path
- Elasticsearch aggs path
- AggregationCache (60s distributed)
…webhook installer, 60s aggregation cache - ScheduledNotificationJob: 60s TimedJob fires schedule-typed notifications by intervalSec, dedupes via distributed cache - AggregationThresholdListener: re-runs aggregations on writes, dispatches once per below->above transition (5 unit tests) - NotificationsAnnotationInstaller: SchemaCreated/UpdatedEvent listener auto-creates Webhook entities for webhook.persistent: true - AggregationCache: 60s distributed cache keyed on resolved filter + RBAC scope, evicted on object-write events - Validator extended for trigger.type scheduled (intervalSec) and threshold (aggregation/op/value) - Drop final from AggregationRunner + AnnotationNotificationDispatcher to enable mocking in unit tests - Fix RBAC CLI bypass for null userId in MultiTenancyTrait::hasRbacPermission (consistent with existing CLI bypasses for null user/org) - Repair UserServiceProfileActionsTest constructor (pre-existing 11/13 arg mismatch)
…cs refresh - Dispatcher: when webhook.persistent === true, skip the inline POST so the standard webhook delivery pipeline (managed via NotificationsAnnotationInstaller-created Webhook entity) handles delivery without double-firing. - AnnotationNotificationDispatcherTest (2 tests) covers persistent-skip + non-persistent-fires paths. - platform-capabilities.md: x-openregister-notifications row updated to v2 (scheduled / threshold triggers, object-acl + expression recipients, talk + persistent-webhook channels). Aggregations row updated to reflect Postgres-native + 60s cache. - docs/features/webhooks-and-notifications.md: notification rules section rewritten for v2 — channel table, trigger table, recipient kinds table, full example covering scheduled / threshold / persistent-webhook.
40 new unit tests across the v2 surface; 183/183 passing in Listener + Notification + Aggregation + BackgroundJob/ScheduledNotificationJob. - AggregationCacheTest (12): get/set hit/miss, RBAC scope isolates users, anonymous shares scope, key stable across filter reorder, distinct (register, schema, name) collision-free, backend-down fail-closed, exception swallowing on get/set. - AggregationCacheInvalidationListenerTest (5): Created/Updated/Deleted/Transitioned all evict; unrelated event ignored. - NotificationsAnnotationInstallerTest (8): create-from-empty, update-in-place by id, idempotent on repeat install, skip non-persistent, skip missing/invalid url, skip when webhook channel absent, exact-name match guards against fuzzy findAll, swallows mapper failures with warning log. - AnnotationNotificationDispatcherTest (+7): Talk one-shot post + URL/actor format, Talk silent without token, object-acl manage = owner only, object-acl read = owner + group members, expression resolver receives object+context, expression fails closed on interface mismatch, expression fails closed on missing service. - ScheduledNotificationJobTest (8): fires when due, skips when interval not elapsed, fires after gap, filter restricts to matching objects, empty filter matches all, non-scheduled triggers ignored, intervalSec < 60 skipped, per-object failure isolated, 30-day TTL on state cache. Two real bugs surfaced and fixed by the tests: - AnnotationNotificationDispatcher::resolveObjectAclRecipients used method_exists($object, 'getGroups') which returns false for Entity::__call magic accessors. Read-permission ACL recipients were silently empty in production. Fixed: call getGroups() directly under the existing try/catch. - AggregationCache was final, blocking PHPUnit from doubling it. Dropped final (consistent with AggregationRunner / dispatcher already being non-final for testability).
…er for non-admins `index()` is `#[NoAdminRequired]` and forwarded the caller-supplied `recipient` filter straight to the mapper with no scoping. Any authed Bob could call `/api/notification-history?recipient=alice` and read Alice's full history — channels, status, ruleId, dispatch timestamps, across tenants. This is the privacy/recon persistence vector flagged in the strict review. Fix: when the caller is not admin, force `recipient = currentUid` regardless of any value they passed; admins keep the free filter so operators can still audit. Existing 500-row page cap stays. Refs: #1419 review (blocker 3) — discussion_r3187494421
`POST /api/registers/import/rollback` was `@NoAdminRequired` and the only safety net was that the downstream `deleteObject` runs RBAC. With broad delete rights any user could rollback a different user's import (or a different tenant's, since the audit lookup never filters by org) just by guessing/learning the job UUID. Look up the first audit row for the supplied `importJobId`, verify the caller is either the original importer (`auditRow->getUser()` matches the current UID) or in the admin group; reject otherwise. 404 when no audit rows match the job UUID at all. Refs: #1419 review (blocker 4) — discussion_r3187494430
`matchEntities()` interpolated raw user-supplied `$subject` into the `%`-anchored ILIKE comparator. Bind-parameterised so no classic SQLi, but `%` and `_` inside `$subject` were not escaped — `subject=%@%` matched every email, `subject=_` matched every value. Combined with the downstream `eraseObjectsForSubject()` chain (admin-gated `vergetelheid`), this is a one-call wildcard PII erase well beyond legitimate DSAR semantics. The `mode === 'exact'` branch had the same bug. Pass `$subject` through `IDBConnection::escapeLikeParameter()` once before either branch builds its named parameter. Refs: #1419 review (blocker 5) — discussion_r3187494433
Every write surface (`create`/`update`/`patch`/`destroy`/`test`/ `migrateFromHooks`) was `@NoAdminRequired` with no per-tenant or owner guard. Actions persist as workflow hooks that fire on every matching object lifecycle event — a non-admin who briefly auths could register attacker-chosen workflow logic that survives password reset, session revocation, and even the source account being disabled (the action row carries no owner check on execution → self-rearming hook). Add `requireAdmin()` helper (returns 401/403 JSONResponse or null) and call it at the top of every mutation. `index`/`show` stay `@NoAdminRequired` so the management UI is still legible to non-admins. Drive-by: `patch()` was forwarding to `update(objectId: $id)`, but `update`'s signature is `update(int $id)` — fixed to `update(id: $id)` so PATCH actually works. Refs: #1419 review (blocker 6) — discussion_r3187494440
…versal in filesFolder Two converging persistence-shaped issues: 1. `writeToFiles()` defaulted the dashboard owner to the literal string `'admin'` when `$dashboard->getOwner()` was null, then wrote attacker-controlled bytes into `admin`'s home folder. Combined with NC's link-share endpoints, that's a re-anchor for re-compromise. 2. `$delivery['filesFolder']` is taken straight from the dashboard payload — user-controlled JSON — with no validation. The job runs as the dashboard owner (not the user who configured the schedule), so the configurer effectively borrows owner-fs permissions. Fixes: - Skip delivery (with a warning log) when owner is null/empty rather than falling back to admin. - Reject folder paths containing `..` (traversal) or that resolve to empty after stripping leading slashes. Persisting a separate `configurer` UID and validating the folder against an allowlist of paths the *configurer* explicitly owns is the right longer-term fix — left as a follow-up so this commit stays small. Refs: #1419 review (blocker 7) — discussion_r3187494444
The workflow triggers on `pull_request` and runs PR-supplied scripts (`tests/newman/run-all.sh`) inside a CI container, copying the entire working tree in via `tar | docker exec`. With no `permissions:` block the job inherits the repo-default GITHUB_TOKEN scope, which on some org configs grants `pull-requests: write` (comment/approve/push back). Add a workflow-level + job-level `permissions: contents: read`. The suite only needs to clone the PR ref. If a future step writes artifacts/comments, scope it explicitly on that job rather than relaxing the default. Pinning `pgvector/pgvector:pg16` and `nextcloud:32-apache` to digests (reviewer's second recommendation) is left as a follow-up — getting the wrong digest breaks CI immediately and digest lookup needs Docker Hub access at commit time. Refs: #1419 review (blocker 8) — discussion_r3187494448
`clearAll()` and `export()` carried `@NoAdminRequired`. Two paths: - `clearAll`: any authenticated caller could wipe every row in `oc_openregister_audit_trails`, breaking the AVG/GDPR Art 30 audit chain that operators rely on for supervisor review. - `export`: any authenticated caller could dump every audit row in bulk (CSV/etc) without an organisation filter — cross-tenant recon with one request. Add a `requireAdmin()` body-level gate (returns 401/403 JSONResponse or null). Keep `@NoAdminRequired` at the framework level so existing non-admin UI flows that touch other endpoints (`index`, `show`, `objects`, hash-chain verification) still load — those don't trip this gate. `index` cross-tenant scoping is a deeper RBAC question (audit rows have an `organisation` column but the existing LogService doesn't filter by it); left for follow-up. Refs: #1419 review (off-diff blocker — top-level comment 4378205122)
`parseExpiration()` returns `null` for any string that doesn't match the `^(\d+)([dhm])$` regex (e.g. "5x", "abc", "90 days"). The caller wrote that null straight into the persisted token row, where a null `expires` means a *non-expiring* token. Net effect: a typo in the expiration mints a permanent API key instead of producing an error. Throw `\InvalidArgumentException` from the controller surface when the input is supplied but unparseable. Existing "no expiration provided" path (`$expiresIn === null` or empty) still produces a non-expiring token — that's an explicit operator choice, not a malformed-input side effect. Refs: #1419 review (off-diff blocker — top-level comment 4378205122)
…enancy:false `/api/scopes` is documented as a discovery endpoint any authed user should be able to call to drive frontend feature gates. But the docblock only carries `@NoCSRFRequired`, so the framework was demanding admin membership before the body ran — defeating the point of self-discovery. Two compounding issues: 1. Add `@NoAdminRequired` so non-admin callers reach the body. The body's anonymous + per-caller permission computation already handles authorization correctly from there. 2. Drop `_multitenancy: false` from the four register/schema lookups. With it set, discovery returned every register/schema slug across every tenant — a complete cross-tenant data-shape map for any tenant user. Only RBAC stays bypassed because the body reduces the actions per-caller downstream. Refs: #1419 review (concern 1) — discussion_r3187494453
…rsor Two issues, one commit: 1. Both `events()` and `cursor()` only carried `@NoCSRFRequired`. The docblock contract assumes non-admins can poll, but without `@NoAdminRequired` the framework gate would have blocked them before the body ran. 2. `cursor()` returned `getMaxId()` — the global head pointer across every tenant. Any authed caller could observe the global write rate (and infer other tenants' activity) by polling. Add `@NoAdminRequired` to both methods. Replace `getMaxId()` with a new `getMaxIdForOrganisation()` mapper method scoped to the active organisation. Active org `null` ⇒ cursor `0` (fail-closed). Refs: #1419 review (concern 2) — discussion_r3187494457
Each URN triggers parse → register find → schema find → object find (~4 DB round-trips per item). Without a cap a caller can post 100k URNs and force ~400k DB round-trips per request. Today the route is effectively admin-only via the absence of @NoAdminRequired, so impact is limited; a future relaxation would convert the missing cap into a direct DoS lever. Reject with 422 above 1000. Refs: #1419 review (concern 3) — discussion_r3187494464
… rules
The per-request permission cache keys verdicts on
`(schemaId, action, userId, objectOwner, objectUuid)` and reuses them
within the request lifecycle. That is safe for schemas whose
authorization is purely group/role based, but unsafe for schemas
whose auth block contains a `match: { … }` clause: the rule reads
from `$object->getObject()`, which can mutate within a single
request via `saveObject()` / TransitionEngine. A write-then-write
pattern (e.g. batch update) would otherwise return a stale verdict
based on the pre-write field values.
Add `schemaHasMatchRule()` helper that walks the schema's
authorization block once and reports whether any entry carries a
non-empty `match`. `buildPermissionCacheKey()` returns null when it
does, forcing each call to re-evaluate the rule chain against fresh
object data. Cost: a hot-path per-request cache miss for the
specific schemas that opt into conditional rules — acceptable trade
versus the correctness regression.
Refs: #1419 review (concern 5) — discussion_r3187494474
…ch error `dispatchCustomScopeEvaluation()` caught listener exceptions, logged them, and returned `null` — letting the caller fall through to the standard rule chain. For canonical verbs that's safe (chain decides), but custom verbs (ZGW `besluit_nemen` etc.) explicitly delegate the verdict to the listener; the chain has no opinion and "deny by default" was the *intent*. The risk: when a listener has previously voted ALLOW for a verb but the dispatcher itself throws, the cleared verdict + chain-fallback can silently open access. Treat the dispatcher exception as a deny vote (`return false`). A genuinely down listener is a security event — let it surface as an explicit denial rather than a no-op. Listener `deny()` votes that land before the throw still propagate via the existing `hasVerdict()` path. Refs: #1419 review (concern 6) — discussion_r3187494479
…schema access `create()` forwarded `registerId` / `schemaId` straight to the mapper. The mapper validated only that at least one was non-null; never that the caller has read access to the referenced register/schema, or that they exist at all. Two issues: 1. A user could subscribe to any (register, schema) tuple in any tenant. Whether the downstream notification dispatcher re-checks permissions before sending is irrelevant — the subscription rows leak existence and let an attacker probe the schema namespace. 2. The mapper's success/404 distinction was a confirmed-existence channel for arbitrary integer IDs. Inject `RegisterMapper` + `SchemaMapper` and call `find()` (with default RBAC + multitenancy filters on) before insert. 404 on miss. Refs: #1419 review (concern 7) — discussion_r3187494490
The migration (Version1Date20260423100000) relaxed NOT NULL on `user_name` so referential-integrity rows can land without a displayable actor. The risk: the relax applies unconditionally, so any future code path that forgets to set `user_name` for a normal create/update silently inserts a row with no human-readable trace of the actor. `createAuditTrailEntry()` (used by ImportService and downstream consumers) was such a path — it set `user`, but never `user_name`. Backfill from `$user->getDisplayName()` when a session user is present, fall back to `'System'` (matches the convention in `createAuditTrail()`). A migration-level CHECK constraint (`(user_name IS NOT NULL) OR (action LIKE 'referential_integrity.%')`) is the more durable answer; this fix is the in-mapper backstop. Refs: #1419 review (concern 9) — discussion_r3187494503
…c messages `render()` previously surfaced `$e->getMessage()` directly in the response — useful for diagnostics, hostile when echoed to a hostile caller. The mapper layer + ReportRenderService chain (PhpSpreadsheet / Dompdf) routinely raise exceptions that include file paths, SQL fragments, or library-internal state. Inject `LoggerInterface`, log the original exception detail at error level (with identifier + format context for correlation), and return a generic message in the response body. Keep `InvalidArgumentException` verbatim — those are validation messages the caller controls and needs to act on. Refs: #1419 review (concern 11) — discussion_r3187494514 Same pattern applies in `AggregationController`, `MetricsController`, `RealtimeController`; left for follow-up since they're separate hunks reviewed off-line.
… sandbox flags Dompdf 3.1 has a history of SSRF / file-disclosure CVEs (CVE-2022-41343, CVE-2023-23924). The current writer correctly sets `isRemoteEnabled=false` and `isPhpEnabled=false`, but two gaps remained: 1. `<link rel="stylesheet">` and `@font-face` source declarations are honoured even with `isRemoteEnabled=false` because Dompdf resolves `file://` URLs locally. A stylesheet/font reference smuggled into the dashboard payload could read arbitrary FS paths. Strip both before handing HTML to Dompdf. 2. The sandbox flags are easy to lose in a future config refactor. Assert both are false at runtime; throw if they drift. A `composer audit` step in CI (reviewer's third recommendation) is already covered by the existing quality workflow's `Security (composer)` job — verified separately. Refs: #1419 review (concern 12) — discussion_r3187494519
…veness
The previous diff replaced the original `if (PROVISIONING) { … } else
{ … }` with two independent `if` statements branching on the same
$status read. Behaviour is equivalent today (the two conditions are
mutually exclusive), but a future refactor that mutates `$status`
between the branches would silently double-run, and a reader can't
tell at a glance that exactly one branch fires.
Fold the second `if` back into the `else`.
Refs: #1419 review (minor) — discussion_r3187494527
Contributor
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Re-review follow-ups — see consolidated verdict next.
Contributor
🔁 Re-review summary — substantial progress, still REQUEST_CHANGESTL;DR: 19 of 21 inline blockers/concerns fully resolved + 2 of 2 off-diff blockers fixed in 22 dedicated The verdict stays at ✅ Resolved (21 of 23 prior findings)All 8 inline 🔴 blockers + all 11 inline 🟡 concerns I marked as actionable have a dedicated fix commit. The 2 off-diff 🔴 blockers (audit-trail destructive endpoints, token-expiration regex) are both fixed too.
The corresponding 19 inline threads have been resolved here in the PR. The 2 partial / deferred items are noted in their reply threads and not blocking.
|
| Gate | Before | After | Delta |
|---|---|---|---|
| 1 spdx-headers | 48 | 48 | 0 — not yet addressed, easy mechanical fix |
2 forbidden-patterns (print_r in SchemaMapper.php:1045) |
1 | 1 | 0 — not yet addressed |
| 5 route-auth (Approval / Registers methods) | 5 | 6 | +1 (development merge brought in another) |
| 6 orphan-auth (TenantLifecycleService validators) | 5 | 5 | 0 — off-diff #4 above |
| 7 no-admin-IDOR | 185 | 178 | −7 (admin guards added) |
8 unsafe-auth-resolver (OasService.php:2233) |
1 | 1 | 0 — off-diff #5 above |
| 9 semantic-auth | 1 | 10 | +9 (admin-fix pattern; see follow-up) |
12 nc-input-labels (ViewObject.vue NcSelect) |
2 | 2 | 0 — not yet addressed |
14 route-reachability (ObjectsController::logs orphan) |
1 | 1 | 0 — not yet addressed |
Per the skill's mechanical-gate override, any failure on the protected list (5, 7, 8, 9, 12) forces REQUEST_CHANGES regardless of the inline-finding count. That's the formal reason the verdict isn't an APPROVE. Practically: the security work the author did is real and substantial; the remaining gates are split between (a) hygiene-after-fix (gate 9), (b) deferred follow-ups already documented (gates 6, 8), (c) cosmetic/process gaps (gates 1, 2, 12, 14), and (d) pre-existing methods needing auth attributes (gate 5).
Suggested path forward
- Quickest path to APPROVE — knock down the four mechanical hygiene gates: gate-1 (add
@copyrightto 48 files), gate-2 (replaceprint_rinSchemaMapper.php:1045), gate-12 (addinputLabelto twoNcSelectwidgets), gate-14 (register or deleteObjectsController::logs). All four are <30 minutes of work. Then drop@NoAdminRequiredfrom the admin-only methods that triggered gate-9 (~10 method-level edits) — that drops gate-9 to its baseline. - Defer to follow-up issues — the off-diff persistence items (SchemasController, TenantLifecycleService, OasService gate-8) and the C4
try/finallywrap. None are blocking on their own; track in tickets. - Still recommend splitting — the 870-file scale issue from the original scale comment hasn't gone away. The fix surface here is healthy, but reviewability of any further large batch will keep being constrained.
🤖 Re-review by /review-pr (Strict). Mechanical-gate command: bash scripts/run-gates-on-pr.sh ConductionNL openregister 1419 development.
WilcoLouwerse
requested changes
May 7, 2026
Contributor
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
❌ REQUEST_CHANGES — re-review (Strict)
Strong response to the prior review — 19 of 21 inline blockers/concerns + 2 of 2 off-diff blockers fixed in 22 dedicated fix(security): commits this morning. Each commit explicitly references its discussion ID. 19 prior threads resolved here.
Verdict stays at REQUEST_CHANGES because 5 protected mechanical gates still fail (5 / 7 / 8 / 9 / 12). After the security work, most of those are now hygiene rather than functional gaps — see the re-review summary for the breakdown.
Still open: C4 mapper request-state, C10 Verwerkingsactiviteiten admin-only via body, and one new gate-9 admin-fix pattern follow-up. The three off-diff persistence items (SchemasController config persistence, TenantLifecycleService orphan validators, OasService gate-8) from the first review's T2 comment are also still open — track as follow-ups.
Quickest path to APPROVE: knock down the 4 hygiene gates (SPDX 48 files, print_r in SchemaMapper.php:1045, two NcSelect inputLabel props, ObjectsController::logs route-or-delete) and drop @NoAdminRequired from the ~10 admin-only methods that now trigger gate-9. <30 min of work each.
🤖 Re-review by /review-pr (Strict mode).
Resolve conflicts in 17 files: openspec retrofit changes were renamed in development (e.g. retrofit-annotate-openregister-2026-04-30 → retrofit-2026-04-30-annotate-openregister). Take development's renamed @SPEC paths and keep this branch's @SuppressWarnings PHPDoc tags.
- ActionsController: drop @NoAdminRequired / #[NoAdminRequired] from create/update/patch/destroy/test/migrateFromHooks. The methods were carrying the attribute *and* calling requireAdmin() in the body, which is the exact contradiction gate-9 (semantic-auth) detects. Without the attribute, the methods are admin-only at the framework level by default; the body-level requireAdmin() stays as defence-in-depth. Refs: PR #1419 review (discussion_r3200014786 / re-review follow-up). - AuditTrailController: same pattern on export() and clearAll(). Updated requireAdmin() docblock to reflect the new layering. - ImportService: move setRequestImportJobId($importJobId) inside the try block in importFromExcel and importFromCsv, so the set/clear pair is fully wrapped — guarantees the request-scoped field is cleared even if the set itself were ever to throw under a long-lived worker reusing the singleton mapper. Refs: PR #1419 review (discussion_r3187494469). Refs #1419
20 tasks
WilcoLouwerse
approved these changes
May 7, 2026
Contributor
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
✅ APPROVE — every actionable security finding addressed
Third pass. All 23 actionable findings from the prior reviews are now resolved:
- 21 fix commits from @rubenvdlinde between 07:55Z and 08:14Z addressing every blocker + concern + the 2 off-diff blockers (
AuditTrailController::clearAll/export+UserService::parseExpiration). 6b24be2closes the last 3 open threads:- C4 (thread) —
setRequestImportJobId(...)is now wrapped in atry { … } finally { setRequestImportJobId(null); }insideImportService::importFromExcel/importFromCsv. Long-lived workers can no longer leak the field. - gate-9 follow-up (thread) —
@NoAdminRequired/#[NoAdminRequired]dropped fromActionsController::create/update/patch/destroy/test/migrateFromHooksandAuditTrailController::export/clearAll. BodyrequireAdmin()retained as defence-in-depth. Framework default (admin-required) now does the actual gating. - C10 (thread) — discussion closed. Author's argument is correct: in NC the framework default for controller methods is admin-required, so the absence of
@NoAdminRequiredonVerwerkingsactiviteiten::create/update/destroyalready enforces admin at the framework level. The bodyisAdmin()is defence-in-depth, not the only gate. My original recommendation to add@AuthorizedAdminSettingwas overzealous — would be a stronger statement (and supports delegated admins) but requires registering anAvgAdminSettings class that doesn't exist anywhere inlib/yet. Reasonable to defer.
- C4 (thread) —
⚠️ Verdict-override note
The skill's mechanical-gate-override rule normally forces REQUEST_CHANGES whenever any of gates 5/7/8/9/12 fail. Overriding to APPROVE here because:
- Gate-9 false positives: 8 of 10 hits are false positives. The scanner regex matches the literal
@NoAdminRequiredtext inside docblock prose like "Admin-only at the framework level (no @NoAdminRequired). BodyrequireAdmin()stays as defence-in-depth." — there is no annotation on the method itself. The remaining 2 hits areRegistersController::rollbackImport(intentional owner-OR-admin design, not pure admin-only) and pre-existingFilesController::preview. Filed as a hydra-gates follow-up: gate-9 needs to anchor the@NoAdminRequiredregex at the PHPDoc tag position (* @NoAdminRequiredat start-of-trimmed-line), not match it as substring. - Gate-5/7 hits are pre-existing patterns where openregister uses service-layer auth via
PermissionHandlerinstead of body checks. ADR-005 documents this for the app. - Gate-8 (1 hit,
OasService::resolveEffectiveAuthorization) is off-diff and tracked as a follow-up. - Gate-12 (2
NcSelectmissinginputLabelinViewObject.vue) — genuine WCAG 1.3.1/4.1.2 issue but is the only remaining in-scope hygiene item. Recommend addressing in a follow-up PR rather than blocking this batch.
Hygiene items to clean up in a follow-up PR
Not blocking, but easy wins:
- gate-1 SPDX: 48 new files missing
@copyrightPHPDoc tag. <30 min sweep. - gate-2:
print_r($property['$ref'], true)inSchemaMapper.php:1045. Replace withvar_export(..., true)or remove. - gate-12: add
inputLabelto twoNcSelectwidgets insrc/modals/object/ViewObject.vue. - gate-14: register or delete
ObjectsController::logsorphan method.
Off-diff follow-ups (track as separate issues)
SchemasController::updateallows any authed user to injectx-openregister-notificationsconfig (server-driven persistence vector) — see original off-diff comment.TenantLifecycleService::isValid{Environment,PromotionOrder,Status}defined-but-uncalled (gate-6).OasService::resolveEffectiveAuthorizationcatch (\Throwable) { return null; }(gate-8 / CWE-863).
Scale concern still stands
870 files in one PR — see original scale comment. Approving on the basis that the security work is solid and the merge will land soon, but consider splitting future batches into per-feature PRs so reviewability scales.
🤖 Re-review by /review-pr (Strict mode, verdict overridden to APPROVE per user direction). Mechanical-gate command: bash scripts/run-gates-on-pr.sh ConductionNL openregister 1419 development.
WilcoLouwerse
added a commit
that referenced
this pull request
May 7, 2026
Resolutions per #1353 review thread: Blockers - aggregations-and-calculations/tasks.md: explicit ADR-028 gating note pointing at split PR #1354 — change MUST NOT merge in current bundled form. - lifecycle-annotation spec: add auth contract for POST /transition (NoAdminRequired, CSRF enforced, 401/403 codes + scenarios). - lifecycle-annotation spec: forbid duplicate (from,to) transition pairs; this makes ObjectTransitionedEvent.action deterministically resolvable on direct PATCH; new 422 code lifecycle-duplicate-from-to + scenario. - aggregations spec: new MUST clause for organisation/register multi-tenancy scoping on aggregate queries (closes the gap that PR #1357/#1419 hit) + cross-org and cross-register scenarios. - notifications-annotation/tasks.md: explicit prerequisite note (depends_on aggregations-and-calculations) — PlaceholderResolver + cache-invalidation event live there. - platform-capabilities.md: mail-sidebar status corrected to implemented; RBAC and multi-tenancy rows now reference the active specs (rbac-scopes, tenant-isolation-audit/lifecycle/quotas) with explicit archive notes for the legacy slugs; geo-metadata-kaart reverted to proposed (still in changes/, not yet graduated). Concerns - lifecycle spec: define StoppableEvent rejection-metadata contract (setRejectionReason / getRejectionReason on ObjectUpdatingEvent; ObjectService::saveObject translates stopped+reason into HTTP 422). Add prerequisite task 1.2a to extend the event class if missing. - notifications spec: normative channel block format table (per kind) + explicit prohibition on inline webhook URLs (SSRF); end-to-end annotation example. - notifications spec: scenarios for created and updated triggers, including only_if_changed field-diff semantics. - aggregations spec: computeOn vocabulary ('save' default; 'transition:<name>' explicitly cross-spec dependent on lifecycle-annotation, with fail-fast 422 if used without lifecycle). - platform-capabilities.md: mark Hydra spec linter and CI catalog enforcement as planned, not shipped. - three deltas/tasks.md: explicit CHANGELOG-entry obligation tasks (deferred to implementing PR; spec PRs document the obligation). - lifecycle spec: auth contract requirement now captures NoAdminRequired in the spec body, not only in tasks.md. Minors - aggregations spec: time-bucket UTC semantics + v2 deferral note. - notifications spec: throttle window grammar pinned to ^([1-9][0-9]*) per (second|minute|hour|day|week)$. Composer.lock plugin-api-version downgrade (concern #14): no spec edit; addressed in the inline review reply (CI Composer version is the canonical regenerator; the field is an environment artefact and the symfony bumps in the same hunk are the substantive change). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Single-session sweep of the openspec backlog: ship real implementation for the 80%+ changes, surface design decisions on the in-flight ones, audit the closures, archive what genuinely shipped, leave honest open status on what didn't.
Code shipped
ScheduledWorkflowJob(varchar(36) overflow on prefixed UUID). Verified end-to-end.SaveObject(fixed anoldDatasnapshot bug along the way),saveObjectsStreamingprimitive withBatchOperationStatusvalue object. 39 reference-validation tests, all green.importJobIdtagging on audit rows;softDeleteByImportJobIdrollback contract; new endpointPOST /api/registers/import/rollback. Migration + entity + service + 16 tests.CustomScopeEvaluatingEvent+ pairedCustomScopeEvaluatedEventfor non-canonical action verbs. 5 tests.mailprofile + 6 integration tests (Greenmail SMTP / CalDAV / CardDAV).openspec validate --strict.geoadded toPropertyValidatorHandlervalidTypes.Archives
10 changes archived (6 with delta-merge into canonical specs, 3 with
--skip-specsfor stale deltas):Honest open status
6 changes remain in
openspec/changes/with genuine undone work documented:These were briefly bulk-ticked under a closure-by-decision pattern earlier in the session; that hid undone work and was reverted. The user explicitly chose BUILD on most of these (B1a/c/d → A, B2 → C, C1 → A) and the build is multi-day scope, not session-deliverable.
Cross-repo hand-off issues filed
Test plan
openspec validate --all --strict: 85/85 passing (probe-tested to confirm validator catches structural breaks)openspec list: 10 archived, 6 honest-openEmailServiceTestconstructor arg mismatch from prior unrelated extension); not session-introduced — separate cleanup ticket recommended🤖 Generated with Claude Code