feat: live updates via notify_push (REST transport for realtime-updates)#1453
Conversation
Adds the deferred notify_push transport from realtime-updates spec (currently a SHOULD requirement, marked v1-deferred in RealtimeService). Two-transport architecture: notify_push for REST clients (consumed by @conduction/nextcloud-vue's object store), SSE for GraphQL clients (already shipped). Both transports hang off the same three OR object lifecycle events; no duplicate dispatch. Capabilities: - realtime-updates: MODIFIED (delta promotes notify_push to MUST, fully specs event strings, fan-out, dedup, batch mode, soft-fail) - admin-settings: NEW (initialises capability spec; adds Push notifications section with three-state probe) Descoped to follow-up: - pushEvents() extension on IntegrationProvider (waits on pluggable-integration-registry to merge, keeps this PR independent)
Introduces OCA\OpenRegister\Push\PushEvents with two constants:
- OR_OBJECT = 'or-object' (suffix: -{uuid})
- OR_COLLECTION = 'or-collection' (suffix: -{register-slug}-{schema-slug})
These are the stable event-name tokens consumed by the browser client
to subscribe to per-object and per-collection update streams.
Spec: openspec/changes/add-live-updates/tasks.md#task-2
Adds a query-based method that returns the deduplicated list of user IDs authorised to read a given ObjectEntity. Used by NotifyPushListener to fan out per-user push events without iterating all Nextcloud users. Approach: - Resolves the schema's effective authorization via resolveAuthorization() - Extracts group IDs from the read rule entries - Returns [] for open schemas (public/admin) — caller treats as broadcast - Fetches members via IGroupManager (one query per group) - Always includes the object owner Also fixes pre-existing phpcs violation: missing @param in buildPermissionCacheKey(). Spec: openspec/changes/add-live-updates/tasks.md#task-3
Adds an IEventListener that handles ObjectCreatedEvent, ObjectUpdatedEvent, and ObjectDeletedEvent and pushes notify_custom events via the Nextcloud notify_push app. Key design points: - Soft-fail: logs one DEBUG message per request when IQueue cannot be resolved (notify_push not installed); never warns or errors - Per-request dedup: same (uuid, action) fires only once - Batch mode: setBatchMode(true) suppresses per-object pushes; flushBatch() emits one collection event per (register, schema) pair - IAppConfig.push_available set to '1' on first successful push (consumed by admin settings page) - Slugs resolved lazily via RegisterMapper and SchemaMapper Also adds resetStaticState() for test isolation. Spec: openspec/changes/add-live-updates/tasks.md#task-4
Wires NotifyPushListener into the Nextcloud event dispatcher for ObjectCreatedEvent, ObjectUpdatedEvent, and ObjectDeletedEvent. Placed alongside the existing GraphQLSubscriptionListener registrations. Spec: openspec/changes/add-live-updates/tasks.md#task-5
Enables batch mode around the two saveObjects() call sites in ImportService so individual per-object push events are suppressed during bulk imports. A @todo documents where flushBatch() should be called once ImportService has access to the IQueue and PermissionHandler instances. Spec: openspec/changes/add-live-updates/tasks.md#task-6
…tion Adds three test files covering tasks 7, 8, and 9: - NotifyPushListenerTest (9 scenarios): soft-fail, object event on update, collection event on create/delete, slugs vs IDs, dedup, batch mode suppression, flush batch, push_available flag on first success - PushEventsTest (5 scenarios): constants have expected string values, are non-empty strings, and are distinct - NotifyPushEndToEndTest: exercises full listener flow with recording queue mock; automatically skipped when notify_push is not installed Uses getMockBuilder()->addMethods(['getSlug']) for Register/Schema mocks because getSlug() is a magic @method, not a real method. Uses resetStaticState() to avoid cross-test pollution of static fields. Spec: openspec/changes/add-live-updates/tasks.md#task-7 task-8 task-9
Admin settings:
- OpenRegisterAdmin gains IAppManager, IAppConfig, IInitialState dependencies
- getPushStatus() returns 'not_installed' | 'unreachable' | 'active' without
instantiating IQueue (safe for settings render)
- provideInitialState('push_status', ...) passes status to Vue
Frontend:
- PushNotificationsConfiguration.vue: status badge using NL Design System
CSS vars (no hardcoded hex); links to App Store and config guide
- Settings.vue: renders the push status section with :push-status prop
- settings.js: reads loadState('openregister', 'push_status', 'not_installed')
i18n: 10 push notification strings in English and Dutch
Docs:
- docs/Integrations/Deck.md: how Deck cards link to OR objects,
push event table, frontend subscription example
- docs/Integrations/index.md: adds Nextcloud-native integrations section
Spec: openspec/changes/add-live-updates/tasks.md#task-10
PHP quality fixes (lib/): - Organisation.php: expand 7 one-liner @var doc comments to proper short-description format required by Squiz.Commenting.VariableComment - MailAppScriptListener.php: phpcbf auto-fixes (blank line, //end tags) - TaskService.php: phpcbf auto-fixes + explicit === true in ternary Test fixes: - OpenRegisterAdminTest: update to pass all 5 constructor args after IAppManager/IAppConfig/IInitialState were added to OpenRegisterAdmin - PermissionHandlerCacheTest::testConditionalRulesEvaluatePerObjectUuid: update expected count from 2 to 4 — cache is intentionally bypassed for schemas with match rules (security fix in c10828b); all 4 hasPermission calls re-evaluate ConditionMatcher - ObjectServiceTest + ObjectServiceDeepTest: update expectException from ValidationException to DoesNotExistException — setSchema() was changed to rethrow DoesNotExistException (404) rather than wrap in ValidationException (500) - ObjectServiceTest: phpcbf auto-formatting sweep
rubenvdlinde
requested review from
Rem-Dam,
SudoThijn,
WilcoLouwerse,
bbrands02,
remko48 and
rjzondervan
as code owners
rubenvdlinde
added
ready-for-code-review
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ❌ 1/153 denied | |||
| npm | ✅ | ✅ 598/598 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
❌ Denied composer licenses
| Package | Version | License |
|---|---|---|
| dompdf/dompdf | v3.1.5 | LGPL-2.1 |
Quality workflow — 2026-05-09 09:14 UTC
Download the full PDF report from the workflow artifacts.
Adds docs/Integrations/OpenRegister.md as the canonical reference for
OR's emitted notify_custom events (or-object-{uuid} / or-collection-
{register}-{schema}), fan-out semantics, dedup, batch-mode, soft-fail
behaviour, and subscription examples.
Reorders docs/Integrations/index.md so OR's own push events appear
first, followed by Nextcloud-native integrations, then LLM/entity/
automation categories. Drops the duplicate Real-time push section
(merged into the OR push events entry).
Adds the icewind1991/notify_push:latest sidecar to docker-compose.yml so
the add-live-updates capability has a complete dev environment. Mounts
the same nextcloud volume read-only (notify_push reads config.php and
the apps/notify_push CSS), connects to the same Postgres DB.
Setup after first up:
docker exec -u www-data nextcloud php occ notify_push:setup \
http://notify_push:7867
docker exec -u www-data nextcloud php occ notify_push:self-test
OR's NotifyPushListener soft-fails when the binary is unreachable, so
this service is optional for environments that don't need realtime
push (existing AppConfig key openregister.push_available stays false).
The previous commit added notify_push without its required redis backend — the binary crashloops with 'No redis server is configured' on first up because NC's memcache.distributed defaults to APCu, which is per-process and unreachable from the notify_push container. Adds redis:7-alpine alongside notify_push (notify_push depends_on: [nextcloud, db, redis]), and documents the four occ config:system:set commands needed after first start so NC uses Redis as memcache.distributed and trusts the Docker subnet as a reverse proxy. The notify_push:setup command is still the last step, unchanged. Verified locally: stack comes up clean from a cold start, occ notify_push:self-test passes all six checks.
The IQueue::push payload was using {userId, data} but notify_push's
canonical wire format is {user, message, body} — see Deck's
SessionService::push at custom_apps/deck/lib/Service/SessionService.php
for the established Nextcloud pattern.
Without `message` in the payload, WebSocket clients have no event
name to filter by — the @nextcloud/notify_push listen(name, callback)
API matches on the message field. Our previous payload would have
silently never reached subscribers.
Verified end-to-end against a live notify_push setup (icewind1991/
notify_push:latest + redis, occ notify_push:self-test passing all
six checks). Event firing for a non-public schema with 10 authorised
readers (vng-gemma/contactpersoon, admin in vng-raadpleger group):
ObjectUpdatedEvent → 10 PUBLISH messages on notify_custom channel
with message=or-object-{uuid} (no collection
event, per spec REQ-ST-LU-007)
ObjectCreatedEvent → 20 PUBLISH messages: 10 or-object-{uuid} + 10
or-collection-{register-slug}-{schema-slug}
(collection event fires on create/delete only)
Each payload's body contains {action, register, schema, uuid, version}
slug-format strings, matching the realtime-updates spec delta.
Adjusted testCollectionEventUsesSlugsNotIds to read from `body` instead
of `data`. Other 8 listener tests pass without changes (they assert on
$queue->push being called N times rather than payload structure).
Sync the OpenRegister.md push-events doc with the actual IQueue::push wire format the listener now uses (user/message/body), matching the canonical NC notify_custom convention. Earlier draft showed userId/data which was a misreading of the API; verified end-to-end via Redis MONITOR while dispatching real ObjectUpdated/CreatedEvents on the live local stack.
Two requests rolled into one commit: 1. scripts/test-merge-feature-branch.sh — pulls a remote feature branch (or PR number) into the currently checked-out local branch via `git merge --no-ff`, then restarts the nextcloud container and runs `occ upgrade` + `occ notify_push:self-test`. Lets developers test PRs locally on top of their active branch without first merging to development. Easy to revert with `git reset --hard HEAD~1`. 2. docs/Integrations/OpenRegister.md gains a new "Subscription cost & guidelines" section answering the most common adoption worry: do extra subscribe() calls slow the page down? Concrete cost table for idle vs active subscriptions, three known anti-patterns to avoid (high-frequency field edits, bulk imports without batch mode, per-row subscriptions instead of per-list), and a worked example for the decidesk LiveMeeting case showing live-updates reduces total fetches ~50x compared to the previous 30s polling baseline. Together the two address the question "how do I test this without merging, and is it safe to subscribe liberally?" — the answers are "use the script" and "yes, idle subscriptions are free."
The merged code is going to development upstream regardless, so framing the local merge as "easy to revert" mischaracterises the workflow. It's a "test now, see it land later" preview, not a sandbox. Plain `git merge` (without --no-ff) keeps the history compatible with whatever upstream strategy lands the same code.
…r lookups Issue #1454: REST API PUT to update an object correctly dispatched ObjectUpdatedEvent (verified via [MagicMapper] Dispatching log) but the NotifyPushListener silently no-op'd — no Redis publish, no WS frame. Root cause: PermissionHandler::getReadableByUsers and the listener's own slug resolvers called SchemaMapper::find / RegisterMapper::find with default _rbac=true, _multitenancy=true. Those mappers verify that the current request user's tenant owns the schema/register — but the listener runs in the request's late lifecycle as a system-internal observer, not a user-facing operation. Cross-tenant events (any object whose schema isn't owned by the actor's tenant — common in shared registers like decidesk's meeting/agenda-item/participant) caused both lookups to throw "not found", leaving: - getReadableByUsers returning [] (logged as "schema lookup failed") - resolveRegisterSlug / resolveSchemaSlug returning null (silent — no log, but the push body's register/schema fields ended up null in payloads that did fire for owner-tenant schemas) The listener is system-level: it's emitting push notifications, not enforcing user-facing access control on the schema. RBAC and tenant gating belong on the read path users actually take, not on a passive event observer downstream of an event the OR backend already chose to dispatch. Verified end-to-end against the live local stack (notify_push container running, configured at http://localhost:7867): curl -u admin:admin -X PUT \ .../api/objects/decidesk/meeting/231842e0-... -d '{"title":"FIXED-FULL-1454", ...}' → Redis MONITOR sees: PUBLISH notify_custom { "user":"admin", "message":"or-object-231842e0-74bb-4556-93b0-6965c2047002", "body":{ "action":"update", "register":"decidesk", "schema":"meeting", "uuid":"231842e0-...", "version":null } } → notify_push WS frame sent to browser-1's connection with the same event-name + body. All 9 NotifyPushListener unit tests still pass.
MWest2020
left a comment
MWest2020
left a comment
There was a problem hiding this comment.
Review — feat: notify_push REST transport
Indrukwekkende scope, goed gestructureerde code, soft-fail / dedup / payload-content-scoping zijn netjes geregeld. Maar één blocker: de batch-mode integratie in ImportService is gebroken, en daarnaast twee design-vragen die expliciete review verdienen voor merge.
🔴 BLOCKER — ImportService dropt alle push events tijdens bulk imports
lib/Service/ImportService.php (lines 1381–1396 en 1417–1430 in de diff) wikkelt saveObjects() correct in try/finally rond setBatchMode(true) / setBatchMode(false) — maar roept nooit flushBatch() aan. Het commentaar in de code erkent dit zelfs expliciet:
} finally {
// Cannot call flushBatch here without IQueue — batch mode is cleared
// so individual events are re-enabled for subsequent calls.
\OCA\OpenRegister\Listener\NotifyPushListener::setBatchMode(false);
}Wat dat in de praktijk doet (zie NotifyPushListener::setBatchMode en handle()):
setBatchMode(true)→handle()schrijft events naarself::$batchedCollections, geen push.setBatchMode(false)→ resetself::$batchedCollections = []zonder iets te emitten.
Netto effect: een 1000-row import emits ZERO push events (noch per-object, noch collection-summary). Dit:
- contradicteert de PR description ("OR's ImportService already does this" — Subscription cost & guidelines, regel 389 in de diff);
- maakt de test plan-stap "bulk-import 100+ objects → exactly one or-collection-* event emitted at end" onhaalbaar;
- schendt de
MUST-promote uit de spec delta (openspec/changes/add-live-updates/specs/realtime-updates/spec.md).
Fix-suggestie: injecteer IQueue en PermissionHandler in ImportService (al genoemd als @todo add-live-updates/task-6) en roep flushBatch($queue, $permissionHandler) aan vóór setBatchMode(false). Tot die tijd is de huidige half-werkende staat erger dan helemaal geen batch mode — overweeg om de setBatchMode-calls tijdelijk te verwijderen totdat de flush er ook is, zodat imports in elk geval per-object pushes blijven sturen (zoals de PR-description voor de niet-geoptimaliseerde fallback al aanneemt op regel 389).
🟡 Cross-tenant fan-out — graag expliciete design-bevestiging
PermissionHandler::getReadableByUsers() (lines 1488–1578) doet het schema-lookup met _rbac: false, _multitenancy: false. Het code-comment legt netjes uit waarom (system-internal, anders no-op bij cross-tenant events — issue #1454). Maar: na het schema-lookup wordt $this->groupManager->get($groupId)->getUsers() aangeroepen, en dat retourneert globale Nextcloud-groep-leden zonder tenancy-filter.
Concreet scenario: tenant T1 maakt object aan, schema's authorization.read bevat groupX. groupX heeft leden in zowel T1 als T2. Iedereen in beide tenants krijgt een push. Dat is mogelijk gewenst (NC-groepen zijn globaal, schema-auteur koos groupX), maar het is een securityrelevante keuze die niet expliciet besproken wordt in design.md of de spec delta. Graag bevestigen of dit gedrag intentioneel is, of dat de fan-out per tenant moet filteren wanneer het object aan een specifieke tenant toebehoort.
🟡 Object-level RBAC niet meegenomen in fan-out (UX risico)
getReadableByUsers() baseert zich uitsluitend op schema-level authorization (resolveAuthorization($schema)) + object owner. Per-object ACL's (bijv. een object met meer restrictieve permissies dan zijn schema) worden niet gecheckt.
Gevolg: een gebruiker in groupX krijgt een push voor elk object van dat schema, ook objects die getReadableByUsers voor REST 403 geeft. De push-payload zelf bevat geen content (alleen action/register/schema/uuid/version) dus geen data-leak, maar de client-side refetch via REST faalt en kan een spurious 403-toast of cache-error tonen. Niet kritiek, wel een UX-finding die documentatie verdient — of een aanvulling op getReadableByUsers die per-object permissions consulteert.
🟡 Supply chain — icewind1991/notify_push:latest
docker-compose.yml:36:
notify_push:
image: icewind1991/notify_push:latestlatest is een moving tag → reproducibility en supply-chain auditability lijden. Pin naar een concrete release tag (b.v. icewind1991/notify_push:0.6.4 of de actuele stable). redis:7-alpine is alleen op major gepind — voor strikte reproducibility ook 7.2.4-alpine o.i.d. (minor punt; Redis 7.x patch-versies zijn meestal stabiel).
De DATABASE_URL=postgres://nextcloud:!ChangeMe!@db/nextcloud op regel 53 is duidelijk een placeholder, maar zou ik documenteren in een NOTE-comment direct boven de env-block ("Replace !ChangeMe! before any non-dev usage").
Kleinere observaties
- Geen per-request cache op
getReadableByUsers()—handle()roept het per event aan, en bij snelle opeenvolgende events op hetzelfde schema wordt de groep→user fan-out elke keer opnieuw uitgevoerd. Per-request dedup op(uuid|action)voorkomt al duplicate work voor hetzelfde object, maar 1000 verschillende objects op één schema betekent 1000 group-lookups. Geen blocker (de notify_push queue is asynchroon), maar overweeg een per-request memoization opschemaId → userIds. resolveRegisterSlug/resolveSchemaSluggebruiken ook_rbac:false, _multitenancy:false— zelfde rechtvaardiging als bijgetReadableByUsers, prima.setBatchMode(false)enresetStaticState()schoonmaakgedrag is consistent (clears$batchedCollections;resetStaticStateook$seen+$queueUnavailable). Goed voor test-isolatie.- Listener registratie unconditional in
Application.php— correct: constructor heeft geen IQueue dependency, runtime-resolve viagetServerContainer()->get()met try/catch. Soft-fail klopt. - Admin status detection (3 staten) in
OpenRegisterAdmin— controleertIAppManager::isInstalled+IAppConfigflagpush_availabledie de listener zet op eerste succesvolle push. Eenvoudig en robuust.
Verdict
Blocker: ImportService batch-mode bug fixen (één van: (a) flushBatch wiring afmaken, of (b) tijdelijk de setBatchMode-wrap weghalen zodat bulk imports per-object events blijven sturen).
Voor merge graag bevestigen: cross-tenant fan-out design-keuze (geïntentioneerd of bug?).
Niet-blocker maar fix-before-merge nice-to-have: notify_push image tag pinnen.
De overige bevindingen (object-RBAC, perf cache, redis pin) zijn opvolgers en hoeven deze PR niet te blokkeren.
Review — feat: live updates via notify_push (REST transport for realtime-updates)Verdict: 🔴 REQUEST_CHANGES — 2 blokkerende bevindingen: (1) batch-import stuurt na merge géén enkele collection-push — de Wat goed gaatUitstekend ontwerp: event-driven zonder nieuwe HTTP-endpoints, soft-fail via lazy container-resolve, correcte per-user fan-out via Findings1. 🔴 Blocker — batch-import stuurt géén collection-event: setBatchMode(true) zonder flushBatch() —
|
MWest2020
left a comment
MWest2020
left a comment
There was a problem hiding this comment.
Zie consolidated review comment voor complimenten, findings en suggested fixes.
| * @spec openspec/changes/add-live-updates/tasks.md#task-4 | ||
| */ | ||
| public static function flushBatch(object $queue, PermissionHandler $permHandler): void | ||
| { |
There was a problem hiding this comment.
[BLOCKER] flushBatch() broadcasts to ALL connected clients — RBAC bypass
flushBatch() accepts a $permHandler parameter but never calls it. Instead it emits IQueue::push('notify_custom', ['message' => ..., 'body' => ...]) with no user field, broadcasting to every connected client regardless of whether they have read access to the affected collection. This is a direct RBAC bypass on the batch-import push path. Either use $permHandler to resolve the authorised reader list and fan out per-user like dispatchPushes() does, or drop the batch collection push entirely until a safe broadcast mechanism is available.
| _multitenancy: $_multitenancy, | ||
| validation: $validation, | ||
| events: $events, | ||
| enrich: $enrich |
There was a problem hiding this comment.
[BLOCKER] Batch mode activated but flushBatch() is never called — events silently swallowed
processSpreadsheetBatch() calls NotifyPushListener::setBatchMode(true) then clears it with setBatchMode(false) in the finally block, but never calls flushBatch(). All push events during bulk imports are silently eaten. The batch mode flag also bleeds across calls: if setBatchMode(true) is set and an exception aborts the import before setBatchMode(false), subsequent saves in the same request will be silently suppressed. Both the missing flush and the bleed risk need fixing before merge.
| * @return array<string> Deduplicated list of user IDs, or empty array when the | ||
| * object is publicly readable (no restriction) or the schema | ||
| * cannot be resolved. | ||
| * |
There was a problem hiding this comment.
[BLOCKER] getReadableByUsers() returns [] for schemas with 'admin' group — admins never receive push events
When the schema's read authorization contains the admin group identifier, getReadableByUsers() short-circuits and returns an empty array. This means administrators configured as readers of a schema receive no live-update events. The admin group has known membership resolvable via IGroupManager; treating it as 'broadcast' and silently dropping the push is wrong. Either resolve admin group members via IGroupManager::get('admin') or document that admin-gated schemas are explicitly excluded — but that must be surfaced as a UI warning, not a silent drop.
There was a problem hiding this comment.
[BLOCKER] Redis port 6379 bound to 0.0.0.0 with no authentication
The new redis service exposes port 6379:6379 on all host interfaces with no requirepass, no --bind 127.0.0.1. Any process reachable on the host network can read and write the Redis instance, including Nextcloud session data and the notify_push channel state. At minimum add command: redis-server --requirepass ChangeMe and align the REDIS_URL in the notify_push service.
| * @spec openspec/changes/add-live-updates/tasks.md#task-4 | ||
| */ | ||
| private function dispatchPushes(string $action, ObjectEntity $object, object $queue): void | ||
| { |
There was a problem hiding this comment.
[BLOCKER] dispatchPushes() sends zero pushes when getReadableByUsers() returns [] — open/public schemas get no events
When PermissionHandler::getReadableByUsers() returns [] (open schema or no authorization configured), dispatchPushes() emits nothing — including no DEBUG log. The push_available flag is never set for open-schema instances, leaving the admin UI permanently stuck on 'unreachable'. For a government register that is publicly readable this means the entire notify_push feature is dead on arrival. There is no fallback, no warning, and no test for this path.
There was a problem hiding this comment.
[BLOCKER] E2E test asserts wrong payload key — will always silently pass with empty data
The test reads $pushedPayloads[0]['payload']['data'] ?? '{}' and json-decodes it, then asserts action, uuid, register, schema. But dispatchPushes() pushes with ['user' => ..., 'message' => ..., 'body' => $payload] — the key is body, not data. Because ?? '{}' provides a fallback, all assertions receive null and silently pass. The test provides false confidence. Fix: change 'data' to 'body' and remove the json_encode/decode round-trip.
| * @spec openspec/changes/add-live-updates/tasks.md#task-4 | ||
| */ | ||
| private function dispatchPushes(string $action, ObjectEntity $object, object $queue): void | ||
| { |
There was a problem hiding this comment.
[CONCERN] No rate limiting on push events — synchronous N×IQueue::push on every object save
dispatchPushes() calls IQueue::push() N times (N = number of authorised readers) synchronously on every object lifecycle event. For a schema with 200 readers, every single save call blocks for 200 IPC/Redis calls with no queue offloading, no rate limit, and no circuit breaker.
| }//end __construct() | ||
|
|
||
| /** | ||
| * Handle an object lifecycle event. |
There was a problem hiding this comment.
[CONCERN] Static $seen dedup accumulator not reset at end of request — risk in persistent PHP runtimes
$seen is a static property used for per-request deduplication. In persistent PHP workers (Roadrunner, Swoole, FrankenPHP) this accumulates across requests and silently suppresses legitimate pushes after the first occurrence of any (uuid, action) pair. The resetStaticState() method is marked @internal For use in unit tests only. Either clear $seen via a request-lifecycle hook or explicitly document that persistent PHP runtimes are unsupported.
| // silently no-ops with an empty reader list — no push fires. Issue #1454. | ||
| $schema = $this->schemaMapper->find( | ||
| id: $schemaId, | ||
| _rbac: false, |
There was a problem hiding this comment.
[CONCERN] getReadableByUsers() resolves schema bypassing RBAC/_multitenancy — cross-tenant data exposure risk
SchemaMapper::find() is called with _rbac: false, _multitenancy: false. In a multitenant deployment this means the push logic can resolve schemas and slugs across tenant boundaries. If schema UUIDs are guessable or shared across tenants, a cross-tenant event could leak the register/schema slug to users in the wrong tenant.
| * | ||
| * @spec openspec/changes/add-live-updates/tasks.md#task-4 | ||
| */ | ||
| private function dispatchPushes(string $action, ObjectEntity $object, object $queue): void |
There was a problem hiding this comment.
[CONCERN] Missing test: unauthorized user MUST NOT receive push — spec requirement without coverage
The realtime-updates spec states: 'GIVEN user external-1 does NOT have read access WHEN the object is updated THEN IQueue::push() MUST NOT be called with userId = external-1'. No unit test covers this scenario. This is the most important security property of the fan-out logic and it has no test coverage.
| * @spec openspec/changes/add-live-updates/tasks.md#task-4 | ||
| */ | ||
| public static function flushBatch(object $queue, PermissionHandler $permHandler): void | ||
| { |
There was a problem hiding this comment.
[CONCERN] $permHandler parameter in flushBatch() is dead code — masks intent
flushBatch(object $queue, PermissionHandler $permHandler) declares $permHandler but the parameter is referenced nowhere in the body. Remove the parameter if broadcast is truly intended, or implement per-user fan-out using it.
| ### Task 14 — Run `composer check:strict` and fix all findings | ||
| After all PHP changes, run `composer check:strict` (PHPCS, PHPMD, Psalm, PHPStan). Fix every finding before submitting — including any pre-existing issues encountered in files this change touches (per project convention: don't leave them for later). | ||
|
|
||
| ### Task 15 — Run unit tests and confirm no regressions |
There was a problem hiding this comment.
[CONCERN] tasks.md specifies 'userId'/'data' payload keys but implementation uses 'user'/'message'/'body'
Task 4 in tasks.md specifies ['userId' => $userId, 'data' => json_encode($payload)]. The actual implementation uses ['user' => $userId, 'message' => $objectChannel, 'body' => $payload]. This inconsistency caused the E2E test assertion bug. Update tasks.md and the spec delta to match the canonical notify_push format.
| * @return void | ||
| */ | ||
| public function testConditionalRulesEvaluatePerObjectUuid(): void | ||
| { |
There was a problem hiding this comment.
[CONCERN] Cache-bypass behavior change for match-rule schemas not reflected in PermissionHandler source diff
The test was updated to expect 4 delegations (from 2) with comment 'Schemas with match clauses bypass the cache entirely — security fix'. However, PermissionHandler.php diff contains no source change that implements this cache-bypass logic. Either the source change is missing from this PR, or the test is asserting against behavior that doesn't exist yet.
There was a problem hiding this comment.
[CONCERN] notify_push port 7867 exposed on all interfaces — unauthenticated WebSocket endpoint
The notify_push service binds port 7867:7867 on all host interfaces. Consider binding to 127.0.0.1:7867:7867 or restricting access via the Docker network.
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Review
🔴 Blockers (6)
- flushBatch() broadcasts to ALL connected clients — RBAC bypass —
lib/Listener/NotifyPushListener.php:337 - Batch mode activated but flushBatch() is never called — events silently swallowed —
lib/Service/ImportService.php:785 - getReadableByUsers() returns [] for schemas with 'admin' group — admins never receive push events —
lib/Service/Object/PermissionHandler.php:1019 - Redis port 6379 bound to 0.0.0.0 with no authentication —
docker-compose.yml:19 - dispatchPushes() sends zero pushes when getReadableByUsers() returns [] — open/public schemas get no events —
lib/Listener/NotifyPushListener.php:219 - E2E test asserts wrong payload key — will always silently pass with empty data —
tests/Unit/Integration/NotifyPushEndToEndTest.php:266
🟡 Concerns (8)
- No rate limiting on push events — synchronous N×IQueue::push on every object save —
lib/Listener/NotifyPushListener.php:219 - Static $seen dedup accumulator not reset at end of request — risk in persistent PHP runtimes —
lib/Listener/NotifyPushListener.php:141 - getReadableByUsers() resolves schema bypassing RBAC/_multitenancy — cross-tenant data exposure risk —
lib/Service/Object/PermissionHandler.php:1039 - Missing test: unauthorized user MUST NOT receive push — spec requirement without coverage —
lib/Listener/NotifyPushListener.php:218 - $permHandler parameter in flushBatch() is dead code — masks intent —
lib/Listener/NotifyPushListener.php:337 - tasks.md specifies 'userId'/'data' payload keys but implementation uses 'user'/'message'/'body' —
openspec/changes/add-live-updates/tasks.md:146 - Cache-bypass behavior change for match-rule schemas not reflected in PermissionHandler source diff —
tests/Unit/Service/Object/PermissionHandlerCacheTest.php:309 - notify_push port 7867 exposed on all interfaces — unauthenticated WebSocket endpoint —
docker-compose.yml:44
🟢 Minor (5)
- Author email inconsistency: dev@conductio.nl (typo: conductio vs conduction) (
lib/Listener/NotifyPushListener.php:1)
The file-level docblock usesdev@conductio.nl— noteconductio.nlvs the correct domainconduction.nl. Standardise toinfo@conduction.nlordev@conduction.nlacross all new files. - resolveRegisterSlug / resolveSchemaSlug duplicate the same bypass pattern — extract helper (
lib/Listener/NotifyPushListener.php:219)
resolveRegisterSlug()andresolveSchemaSlug()are near-identical: both catch\Throwableand return null, both callfind(id: ..., _rbac: false, _multitenancy: false)with identical inline comments. Extract a shared helper to keep the bypass justification comment in one place. - PushEvents is a pure constants class — should be declared final (
lib/Push/PushEvents.php:1)
PushEventscontains only constants and is not designed for extension. Addfinal class PushEventsto prevent accidental subclassing. - en.json missing trailing newline (
l10n/en.json:549)
The diff shows\ No newline at end of filefor bothl10n/en.jsonandl10n/nl.json. Many editors and POSIX tools expect a trailing newline; CI linters may flag it. - getReadableByUsers() silently skips match-rule authorization entries without a group key (
lib/Service/Object/PermissionHandler.php:1019)
The group-extraction loop silently skips entries that are arrays without agroupkey. Add a$this->logger->debug(...)when an unrecognised entry shape is encountered so operators can diagnose missing push events during setup.
Reviewed by WilcoLouwerse via automated batch review.
🔍 Retrospective audit findings — surfaced from #1515While reviewing integration rollup PR #1515, three 🔴 blockers trace back to the live-updates changes in this PR. Documenting here for follow-up:
The PR body of #1515 already surfaced findings 1 and 3 under "#1453 surfaced for maintainer judgement" — confirming they merged unresolved. Worth opening follow-up issues / a fixup PR. |
3 participants
Summary
Implements the deferred
notify_pushtransport from the existing realtime-updates capability. REST clients (in particular@conduction/nextcloud-vue's object store) gain real-time push delivery without polling. Companion frontend has shipped in @conduction/nextcloud-vue@1.0.0-beta.6 (PR #144 + 4 follow-up dep fixes); consumer apps procest, decidesk, pipelinq are bumped in procest#319, decidesk#161, pipelinq#329.GraphQL clients keep using the existing SSE transport. Both transports hang off the same three internal events (
ObjectCreatedEvent,ObjectUpdatedEvent,ObjectDeletedEvent) — no event duplication at source.What ships
lib/Push/PushEvents.php— typed constants (OR_OBJECT,OR_COLLECTION) following theOCA\Deck\NotifyPushEventspattern.lib/Listener/NotifyPushListener.php—IEventListenerthat emitsor-object-{uuid}per authorised user on every lifecycle event, plusor-collection-{register-slug}-{schema-slug}on create/delete only (no collection events on field edits — avoids list refetch storms). Soft-fails whennotify_pushisn't installed. Per-request dedup. Static batch-mode flag for bulk imports. Wire format:{user, message, body}matching the canonical Nextcloudnotify_customconvention used by Deck and Calendar (validated end-to-end against a live notify_push stack via Redis MONITOR — see commitsb5f91100dandd34f16e25).PermissionHandler::getReadableByUsers(ObjectEntity)— query-based fan-out target resolution. Open/public schemas return[](broadcast handled separately).ImportService— wraps bulk-save insetBatchMode(true)/flushBatchso a 1000-row import emits one collection event per affected(register, schema)pair, not 1000 × N_readers pushes.OpenRegisterAdminwith a Push notifications section (three states: not installed / installed but unreachable / active). Vue component uses NL Design System CSS vars. nl + en translations.docker-compose.yml— addsredis:7-alpine+icewind1991/notify_push:latestservices sodocker-compose up -dproduces a push-ready dev stack out of the box. Inline comments document the post-upocc config:system:set+notify_push:setupsteps.docs/Integrations/OpenRegister.md— full reference for OR's own push events (event strings, fan-out, dedup, batch mode, soft-fail, two-transport architecture, subscription cost & guidelines with idle-cost-is-zero analysis + decidesk LiveMeeting worked example).docs/Integrations/Deck.md— worked example documenting Deck'snotify_custompush events; sets the template for future integration docs.scripts/test-merge-feature-branch.sh— small helper to merge a remote PR/branch into the local active branch and restart Nextcloud, so OR PRs can be previewed locally ahead of upstream landing on development.End-to-end validated
Against a live local stack (PR's docker-compose with
notify_pushconfigured), realObjectCreatedEvent+ObjectUpdatedEventdispatches produced exactly the spec'd Redis publishes:ObjectUpdatedEventor-object-{uuid}, 0 × collection (field-edit storm avoidance)ObjectCreatedEventor-object-{uuid}, 10 ×or-collection-{r}-{s}Each payload's
bodycarries{action, register, schema, uuid, version}slug-format strings, matching the realtime-updates spec delta.occ notify_push:self-testpasses all six checks.Spec coverage
realtime-updates— promotes notify_push fromSHOULDtoMUST; specifies event strings, fan-out, dedup, batch-mode behaviour. Delta inopenspec/changes/add-live-updates/specs/realtime-updates/spec.md.admin-settings— initialises capability spec; adds Push notifications section.pushEvents()extension onIntegrationProviderwaits onpluggable-integration-registryto land in code (interface doesn't exist yet). This keeps the PR independent.Test plan
notify_push→ object saves still succeed; no WARNING/ERROR logs about missing pushor-collection-*event emitted at end, not one per objectOut of scope (separate changes)
pushEvents()extension onIntegrationProvider(waits onpluggable-integration-registrylanding)