integration: rollup of 14 open OR PRs (tracking/integration branch)#1515
Merged
Conversation
Adds the cobalt-background DetailHero variant + bumps to preset 1.5.1 for the full-bleed clipping fix. See https://mydash.conduction.nl/ for the live look.
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
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.
Implements tasks 1.1–1.10 of openspec change `add-features-roadmap-menu`
(GitHub issues proxy for the in-product roadmap surface).
Adds two methods to the existing GitHubHandler:
- listIssues(): cached list with OR-merged label filter (per D23),
PR entries filtered out, sensitive fields stripped (task 1.10).
- createIssue(): user PAT preferred, server PAT fallback with
attribution prefix; specRef body suffix + label applied when set.
Adds GitHubIssuesController with:
- GET /api/github/issues — #[NoCSRFRequired], 15-min distributed cache,
X-OpenRegister-GitHub-Cache: HIT|MISS header.
- POST /api/github/issues — CSRF enforced (no NoCSRFRequired), per-user
60s rate limit, graceful PAT-missing 503, GitHub rate-limit mapping
to 429 + reset_at.
Application.php wires two new GitHubHandler deps (IURLGenerator,
IUserManager) needed for the attribution prefix.
Pending in follow-ups (section 1):
- 1.11 PHPUnit tests incl. PAT-leak assertion
- 1.12 OpenAPI documentation
- 1.13 composer check:strict run
- 1.14–1.22 security hardening (repo allowlist, display-name
sanitization, audit logging, specRef + sort + labels validation,
admin opt-out flag, PAT scope/lifecycle docs)
Refs #1328
…indings Addresses 4 PHPMD warnings on the section-1 skeleton without resorting to @SuppressWarnings annotations: GitHubHandler — CouplingBetweenObjects: 14 → 11 (under threshold) • Extract new AttributionFormatter service (IUserManager + IURLGenerator now live there, not on the handler) • Switch constructor from IClientService to IClient directly (factory in Application.php calls newClient() at instantiation time) GitHubIssuesController::create — CyclomaticComplexity 10 → 4 + NPathComplexity 288 → ~12 • Extract validateSubmission() helper (repo / title / body length checks) • Extract enforceSubmitRateLimit() helper (per-user 60s APCu lookup) GitHubIssuesController::mapHandlerException — CyclomaticComplexity 12 → 4 • Extract mapPatNotConfigured() helper (read vs write graceful path) • Extract mapGitHubRateLimit() helper (BadResponseException unwrap + X-RateLimit-Remaining/Reset header inspection) Also applies code-style fixes flagged by PHPCS on touched files: • All internal-method calls converted to named-parameter form (validateSubmission(repo: …, title: …, body: …), etc.) • is_array($labels) === true (explicit comparison) • Closure → named static function for the labels-merge usort comparator • Missing constructor docstring on AttributionFormatter • Missing //end if marker after early-return arm in listIssues • Auto-fix run of PHPCBF (44 whitespace / blank-line nits) on the changed files; no other files touched. Touched files now pass: • php -l ✓ • composer phpcs --standard=phpcs.xml ✓ (0 errors on these files) • composer phpmd lib/...|text phpmd.xml ✓ (0 warnings on these files) Pre-existing PHPCS/PHPMD findings in unrelated files (AggregationRunner, ActionsController, OrganisationController, RegisterService, etc.) are left for their own follow-up — they predate this PR by months and touching them would inflate the diff with unrelated whitespace fixes. Refs #1328
… external router (umbrella PR 1/N) First slice of the pluggable-integration-registry umbrella (#1307). Lands the foundation that every leaf change in waves W1-W3 depends on: the provider contract, the registry, external-routing plumbing, and the query-time storage-strategy helper. No built-in provider migration yet (tasks 12-17); no schema validator refactor yet (tasks 7-11); no frontend wiring yet (tasks 25+). Tasks completed: 1.1, 1.2, 1.3, 1.4, 1.5, 1.6 (6/69 of the umbrella). What this adds: - lib/Service/Integration/IntegrationProvider.php — 15-method interface per design.md normative contract. Storage strategies: magic-column / link-table / external / query-time. - lib/Service/Integration/AbstractIntegrationProvider.php — base class with sensible defaults; mutation methods (get / create / update / delete) default to NotImplementedException so list-only and query-time providers don't have to spell that out. - lib/Service/Integration/IntegrationRegistry.php — explicit addProvider() registration with collision detection (AD-13) and external-source rejection (AD-4). Spec called for DI-tag-based discovery, but modern Nextcloud doesn't expose IAppContainer::queryAll(<tag>) as public API. Switched to explicit addProvider() at app bootstrap with identical semantics — documented in the file's class docblock + in tasks.md. - lib/Service/Integration/ExternalIntegrationRouter.php — dispatches storage='external' CRUD calls through OpenConnector. Surfaces failures via ProviderUnavailableException with a 3-way cause classification (openconnector-down / openconnector-source-missing / upstream-service-down) per AD-23. Includes probe() for admin health reporting. - lib/Service/Integration/QueryTimeContract.php — codifies the 2 s render timeout (AD-22) and the HTTP 501 envelope builder for the ObjectsController to consume when tasks 7-11 refactor it. - lib/Exception/NotImplementedException.php — thrown by query-time / list-only providers' unsupported CRUD methods. - lib/Exception/ProviderUnavailableException.php — carries the 3-way cause classification + a getDetails() helper that produces the `{cause: …}` payload the UI renders. - lib/AppInfo/Application.php — new private registerIntegrationRegistry() phase wires the registry and the router as shared per-request singletons. - tests/Unit/Service/Integration/*Test.php — 25 unit tests across 4 classes covering addProvider() validation (collision + external source), getEnabled() filtering, AbstractIntegrationProvider's NotImplementedException defaults, ExternalIntegrationRouter's failure-mode classification, and QueryTimeContract's HTTP envelope shape. All green. What's still pending (rest of the umbrella): - Schema validator refactor (tasks 7-11): Schema::validateLinkedTypesValue consumes IntegrationRegistry::listIds; deprecation of VALID_LINKED_TYPES + LinkedEntityService::TYPE_COLUMN_MAP. - Built-in providers (tasks 12-17): 5 BuiltinProviders/*Provider.php classes wrapping the existing files/notes/tasks/tags/audit-trail integrations. - Routes + controller + capabilities (tasks 18-22): IntegrationsController, ObjectsController sub-resource dispatch via registry, OCS capabilities block. - Admin UI + frontend registry + 3 missing widgets + CnObjectSidebar / CnDashboardPage / CnDetailPage / CnFormDialog / CnDetailGrid refactor (tasks 23-46). - Tests / CI parity gate / scaffold script / ADR + docs / translations (tasks 47-67). - E2E acceptance verification (tasks 68-71). Spec adjustment: tasks.md and plan.json record the DI-tag → explicit addProvider() pivot explicitly so the next session (or hydra builder) doesn't trip on it. Refs: #1307
…, 1.20b, 1.21
Implements seven of the eight section-1 security / resilience / compliance tasks
on the `add-features-roadmap-menu` change. Tests for each task (per the task
descriptions in tasks.md), task 1.11's PAT-leak assertion, task 1.12's OpenAPI
documentation, task 1.13's full composer check:strict pass, task 1.18's
APCu->ICache fallback abstraction, and task 1.22's admin-docs writeup are
explicitly deferred to follow-up commits — this PR keeps the diff focused on the
guard-pipeline shape + the implementations themselves.
Refactor for class-level complexity / coupling:
Extract two new services
lib/Service/Configuration/GitHubRequestValidator.php (new — pure-function
validators: validateRepoFormat, validatePerPage, validateTitleLength,
validateBodyLength, validateSpecRef, validateSort, validateLabels)
lib/Service/Configuration/GitHubGuards.php (new — policy guards
+ pipeline runner: runGuards, enforceFeatureFlag, enforceRepoAllowlist,
enforceGetRateLimit)
Controller becomes a thin orchestrator
GitHubIssuesController::index/create now compose a guard pipeline via
GitHubGuards::runGuards([...closures...]) — each guard's branching stays
inside its own method, the controller's cyclomatic + ExcessiveClassComplexity
counts stay flat as more guards land.
Handler splits createIssue
resolveCreateIssueToken / buildIssueBody / buildIssuePayload /
dispatchCreateIssue keep individual methods under cyclomatic + NPath
thresholds. dispatchCreateIssue returns the decoded array so callers don't
need the IResponse type (drops one class reference from the handler).
AttributionFormatter is extracted so the handler does not import
IUserManager + IURLGenerator directly.
Security tasks landed:
1.14 Repo allowlist enforcement
openregister::github_repo IAppConfig key. Unset → 200+hint (GET) or
503+error (POST). Set + mismatch → 403 repo_not_allowed. Set + match →
continue. Collapses the user-supplied repo attack surface to a single
admin-configured value.
1.15 Display-name + URL sanitization in AttributionFormatter
Strips CR/LF, *, _, `, [, ], (, ), backslash, <, > from the embedded
display name and truncates to 80 chars. Validates instance URL is
https:// or http://localhost; non-https + non-localhost falls back to
a URL-free generic prefix ("> Submitted via Nextcloud OpenRegister").
1.16 specRef format validation
Regex ^[a-z0-9][a-z0-9-]*[a-z0-9]$, length ≤ 80. Rejects newlines,
uppercase, punctuation, oversized slugs with 400 specref_invalid_format
before any GitHub call.
1.17 Audit logging for server-PAT submissions
GitHubHandler::dispatchCreateIssue emits exactly one INFO entry on
success and one WARNING entry on failure when the server-PAT fallback
path is taken. Fields: user_id, repo, issue_number, specref, timestamp
(plus error_code + github_status on failure). Never logs PAT, body,
or title. User-PAT path emits no openregister entry — those are
auditable on GitHub directly under the user's identity.
1.19 Per-user GET cache-miss rate limit
Per-user counter of distinct cache-key tuples within a rolling 5-minute
window, capped at 10 entries. Cache hits do not count. 11th distinct
miss returns 429 user_rate_limited + Retry-After header. Anonymous
callers are not counted. Enforced AFTER the cache check and BEFORE the
outbound GitHub call.
1.20 sort parameter allowlist
Constrains the GET ?sort= query parameter to {reactions-+1, created,
updated, comments}. Other values → 400 sort_invalid_value. Bounds the
cache-key space.
1.20b labels parameter validation
Each label matches ^[a-z][a-z0-9_-]*$ with length ≤ 50; max 8
comma-separated entries. Cache key already sorts labels so different
filter orderings hit the same cache slot. Rejects SQL-injection-shaped
labels, oversized labels, and 9+ entries with 400.
1.21 Admin opt-out flag
openregister::features_roadmap_enabled IAppConfig key (default true).
When false, both endpoints return 403 feature_disabled and skip all
downstream logic including the outbound GitHub call.
Quality gates green on touched files:
php -l clean
composer phpcs clean (5/5 files, 0 errors)
composer phpmd clean (0 warnings)
Application.php — no DI changes required for the new GitHubGuards +
GitHubRequestValidator + AttributionFormatter classes; NC's auto-wiring
resolves their OCP-typed constructors directly.
Refs #1328
Marks the security / resilience / compliance tasks landed in the previous commit (security hardening) as done in tasks.md. Pending: 1.11 (tests), 1.12 (OpenAPI), 1.13 (full quality run), 1.18 (APCu->ICache fallback), 1.22 (admin docs).
…urveillance (umbrella PR 2/N) Second slice of #1307 — completes tasks 7-11 of the umbrella (Backend — Schema validator refactor). Builds on PR 1's IntegrationRegistry + ExternalIntegrationRouter foundation. Stacked PR — base is feature/1307/pluggable-integration-registry; GitHub will retarget to development once PR 1 merges. Tasks completed in this slice (5/69 → cumulative 11/69): - 2.1 Schema::validateLinkedTypesValue() now consults both VALID_LINKED_TYPES (deprecated fallback) AND IntegrationRegistry::listIds(). Registry resolved lazily via \OC::$server->get() since Schema is a Nextcloud Entity, not a service. Falls back to fallback-only when container isn't booted (unit tests). AD-5 backwards-compat preserved: existing schemas with 'mail' / 'calendar' / 'talk' / 'deck' still validate while the leaves land. - 2.2 VALID_LINKED_TYPES marked @deprecated with pointers to the registry + cleanup follow-up. - 2.3 LinkedEntityService::TYPE_COLUMN_MAP marked @deprecated. - 2.4 PropertyReferenceTypeValidator — new opt-in service that validates the `referenceType: <integration-id>` marker on schema property definitions (AD-18). Kept as a standalone validator so existing schema validation paths don't change; CnFormDialog / CnDetailGrid wire it in tasks 25-46. - 2.5 LogDanglingLinkedTypes repair step — registered under <install> + <post-migration> in info.xml. Scans every schema, logs WARNING for any linkedTypes value not yet registered. Strictly informational; never throws, never modifies data. Net new files: - lib/Service/Integration/PropertyReferenceTypeValidator.php - lib/Repair/LogDanglingLinkedTypes.php - tests/Unit/Service/Integration/PropertyReferenceTypeValidatorTest.php Modified: - lib/Db/Schema.php — validator + deprecation note - lib/Service/LinkedEntityService.php — deprecation note on TYPE_COLUMN_MAP - lib/Service/Integration/IntegrationRegistry.php — added isValidIntegrationId() helper consumed by the new validator - lib/AppInfo/Application.php — DI bindings for the new validator service + the repair step - appinfo/info.xml — repair-steps block adds LogDanglingLinkedTypes - openspec/changes/pluggable-integration-registry/tasks.md - openspec/changes/pluggable-integration-registry/plan.json Unit tests: - 34 tests, 48 assertions — all green (9 new for PropertyReferenceTypeValidator + the 25 from PR 1) Built-in providers (tasks 12-17) intentionally deferred to PR 3 to keep this PR reviewable as one coherent slice (the validator + the dangling-id surveillance form one story). Refs: #1307
…1.22)
Tests — task 1.11
tests/Unit/Service/Configuration/GitHubRequestValidatorTest.php
20 cases covering the 7 validators (repo format, per_page range,
title/body length, specRef format incl. SQL-injection + newline +
length-cap, sort allowlist, labels validation incl. injection-shaped
+ 9-entry + oversized cases).
tests/Unit/Service/Configuration/AttributionFormatterTest.php
6 cases covering task 1.15: happy-path canonical prefix, markdown-
injection display name sanitization (separator appears exactly once),
80-char truncation, https-only scheme validation + generic-prefix
fallback, http://localhost dev exception, missing-user UID fallback.
tests/Unit/Service/Configuration/GitHubGuardsTest.php
7 cases covering tasks 1.14 + 1.19 + 1.21: feature-flag enabled/
disabled, repo-allowlist unset (graceful 200 on GET, 503 on POST),
repo mismatch, repo match, 11th distinct GET cache-miss hits 429,
runGuards short-circuits on first non-null response.
tests/Unit/Controller/GitHubIssuesControllerTest.php
7 cases covering the controller wiring + the load-bearing security
scenarios: 401 on unauthenticated POST, 400 on short title without
calling the handler, 400 on invalid specRef without calling the
handler, 403 cross-repo POST when allowlist set, 403 feature-
disabled on both endpoints, **PAT-leak assertion** running 3 paths
with placeholder `YOUR_API_KEY_HERE` and asserting it never appears
in body or headers, ReflectionMethod check on the #[NoCSRFRequired]
attribute placement (GET has it; POST does not, so CSRF middleware
runs).
Notes:
- Mocks use the same `createMock(SomeClass::class)` pattern as the
existing SearchControllerTest. PHPCS named-args sniff flags those at
the same severity it flags existing tests; not addressing in this
diff to keep test parity with the repo's existing style.
- Tests can't run locally without Nextcloud's lib/base.php bootstrap
(same constraint as the existing tests/Unit/* suite); they execute
in CI with the OPENREGISTER_TEST_NC_ROOT env var pointing at an
installed NC instance.
OpenAPI — task 1.12
openapi.json: adds /index.php/apps/openregister/api/github/issues GET +
POST operations + 5 supporting component schemas (GitHubIssueItem,
GitHubIssuesListResponse, GitHubIssueCreatedResponse, GitHubIssueSubmit
Request, StructuredError). Documents every parameter, every response
status (200/400/401/403/412/429/503), the X-OpenRegister-GitHub-Cache
+ Retry-After response headers, and every structured error_code
enum value (18 codes). All token placeholders use `YOUR_API_KEY_HERE`
per the safe-example-values rule from opsx-ff.
Admin docs — task 1.22
docs/Integrations/github-issues-proxy.md: full admin reference —
configuration keys (github_api_token, github_repo, features_roadmap_
enabled), minimum PAT scope (public_repo), fine-grained-PAT preference,
90-day rotation cadence, compromise-response procedure, the three
audit-log entry shapes, rate-limit table, and a troubleshooting table
covering the 6 documented degraded UI states.
Refs #1328
…tions (umbrella PR 3/N) Third slice of #1307 — completes tasks 12-17 of the umbrella. Stacked on PR #1468 (schema validator refactor), which is itself stacked on PR #1467 (foundation). Tasks completed in this slice (6/69 → cumulative 17/69): - 3.1 FilesProvider wraps FileService (magic-column). - 3.2 NotesProvider wraps NoteService (link-table, full CRUD). - 3.3 TasksProvider wraps TaskService (link-table, CalDAV, full CRUD; composite {calendarId}/{taskUri} entity ids). - 3.4 TagsProvider wraps the NC system tag manager (link-table; read via ISystemTagObjectMapper::getTagIdsForObjects). - 3.5 AuditTrailProvider wraps AuditTrailMapper (query-time, AD-22 — read-only by design; mutation methods inherit NotImplementedException from AbstractIntegrationProvider). - 3.6 All five register via addProvider() at Application::boot() time through a new bootBuiltinIntegrationProviders() helper. The DI bindings live in registerBuiltinIntegrationProviders() so each provider's wrapped service is resolved lazily. Spec deviations flagged inline in tasks.md + plan.json: - FilesProvider + TagsProvider keep their write paths at the existing FileController / TagsController routes for now. Mutation methods throw NotImplementedException with a pointer to the umbrella's controller refactor (tasks 18-22). Users see the right tab; the underlying write API stays where it is until the controller layer catches up. Surface change is zero. - AuditTrailProvider intentionally inherits the abstract base's mutation defaults — audit-trail entries are immutable by construction; per AD-22 query-time providers MUST throw on create/update/delete. - The referenceType: <id> declarations the spec calls for on the frontend registry side are deferred to tasks 25-30 (when each provider gets its JS registry counterpart). The PHP-side providers are complete. Net new files: - lib/Service/Integration/BuiltinProviders/FilesProvider.php - lib/Service/Integration/BuiltinProviders/NotesProvider.php - lib/Service/Integration/BuiltinProviders/TasksProvider.php - lib/Service/Integration/BuiltinProviders/TagsProvider.php - lib/Service/Integration/BuiltinProviders/AuditTrailProvider.php - tests/Unit/Service/Integration/BuiltinProvidersMetadataTest.php Modified: - lib/AppInfo/Application.php — new registerBuiltinIntegrationProviders + bootBuiltinIntegrationProviders helpers - openspec/changes/pluggable-integration-registry/tasks.md - openspec/changes/pluggable-integration-registry/plan.json Unit tests: - 45 tests, 80 assertions — all green (11 new for the 5 providers + the 34 from PR 1 & 2) Refs: #1307
…brella PR 4/N) Fourth slice of #1307 — completes tasks 18-22 of the umbrella (Backend — Routes, Controller, Capabilities). Stacked on PR #1469 (built-in providers), itself stacked on #1468 (schema validator) and #1467 (foundation). Tasks completed in this slice (5/69 → cumulative 22/69): - 4.1 IntegrationsController — read-only API over the registry. GET /api/integrations (with `group` and `enabled` filter params), GET /api/integrations/{id}. Role-redacted descriptor per AD-17 — every authed user sees public fields (id, label, icon, group, enabled, storageStrategy, surfaces); admins additionally get requiresPermission, openConnectorSource, authStatus. Non-admin fields are omitted (not null-stubbed) so absence matches non-existence. - 4.2 Object-scoped sub-resource dispatch via the registry — new dedicated ObjectIntegrationsController owns /api/objects/{register}/{schema}/{id}/integrations/{integrationId}[/{entityId}] (GET / POST / PUT / DELETE). Additive — ObjectsController (2400+ lines) stays untouched. Error translation per AD-22 / AD-23: NotImplementedException → 501 with QueryTimeContract envelope ProviderUnavailableException → 503 with details.cause payload unknown integration id → 404 other Throwable → 500 (real exception logged, generic message returned per ADR-005). - 4.3 Routes wired in appinfo/routes.php — both the discovery API and the object sub-resource dispatch. - 4.4 IntegrationsCapability — surfaces the registry through /ocs/v2.php/cloud/capabilities, role-redacted per AD-17. Spec said 'Update lib/Service/CapabilitiesService.php'; OR's capability pattern uses one ICapability class per concern (see UrnCapability), so the new file lives in lib/Capabilities/. Same end shape, idiomatic structure. - 4.5 Registered via $context->registerCapability() in Application::register() — mirrors the existing UrnCapability pattern. No appinfo/info.xml change needed (OR doesn't carry capability declarations there). Net new files: - lib/Controller/IntegrationsController.php - lib/Controller/ObjectIntegrationsController.php - lib/Capabilities/IntegrationsCapability.php - tests/Unit/Controller/ObjectIntegrationsControllerTest.php Modified: - appinfo/routes.php — 7 new routes (2 discovery + 5 sub-resource) - lib/AppInfo/Application.php — DI bindings for the new controllers and the capability; registerCapability() call - openspec/changes/pluggable-integration-registry/tasks.md - openspec/changes/pluggable-integration-registry/plan.json Unit tests: - 52 tests, 95 assertions — all green inside the openregister-postgres dev container (7 new for ObjectIntegrationsController dispatch + error translation; the 45 from PRs 1-3 still pass) Spec deviations flagged inline: 1. Sub-resource dispatch lives in a NEW ObjectIntegrationsController rather than refactoring the 2400-line ObjectsController. Additive, zero regression risk on existing routes. 2. Capability advertisement uses one ICapability class (IntegrationsCapability) registered via registerCapability(), mirroring UrnCapability. Spec called for editing CapabilitiesService — OR doesn't have that file as the canonical integration point; the per-concern ICapability pattern is idiomatic for this codebase. 3. info.xml capability section unchanged — OR registers capabilities in code, not XML. Refs: #1307
…PR 5/N)
Renders the OpenRegister → "Integrations" admin section with a row per
registered IntegrationProvider:
- id / label / group / storage strategy
- required Nextcloud app + isInstalled status
- isEnabled() result
- authStatus / status / message from probe() (external) or health() (native)
- Configure deep-link into OpenConnector's source-edit screen
(external providers only)
- Test connection URL (external providers only)
Per AD-15, OpenRegister hosts the unified surface; credential flows
stay in OpenConnector — this page only links out. Native providers
report through provider->health(); external providers route through
ExternalIntegrationRouter::probe() so failure messages match what
runtime callers see.
Implements tasks 5.1–5.3.
Files:
- lib/Settings/IntegrationsAdminSettings.php (new) — ISettings
implementation, builds rendered rows from IntegrationRegistry
- templates/settings/integrations-admin.php (new) — server-rendered
table with status badges (ok / unavailable / unknown), translation
via $l throughout
- appinfo/info.xml — registers the additional <admin> entry
- lib/AppInfo/Application.php — DI binding for IntegrationsAdminSettings
- tests/Unit/Settings/IntegrationsAdminSettingsTest.php (new) — 5 tests
covering section/priority stability, row rendering, configure-URL
presence for external providers, absence for native providers, and
router routing for external probes
- openspec/changes/pluggable-integration-registry/{tasks.md,plan.json}
— mark 5.1–5.3 done
Spec deviation (documented in tasks.md):
Spec called for "edit CapabilitiesService" — there's no such
integration point on master. Capability is exposed via the idiomatic
ICapability class added in umbrella PR 4 (IntegrationsCapability),
not via this settings change.
Tests:
./vendor/bin/phpunit --configuration phpunit-unit.xml \
tests/Unit/Settings/IntegrationsAdminSettingsTest.php
-> 5 tests, 21 assertions, OK
…pTrait
Replaces the 'return []' stubs in activity / analytics / collectives /
cospend / flow / forms / maps / photos / shares / time-tracker with
real list() implementations that query each NC app's main table
directly via IDBConnection.
* lib/Service/Integration/Providers/MarkerLookupTrait.php — shared
helper that runs the LIKE-against-a-marker query the leaf providers
all need. Defensive: any DB error degrades to an empty list (AD-23).
* Each provider's list() picks a marker convention '[or:{objectUuid}]'
in a user-visible text field (subject for activity, name for
analytics/cospend/flow/maps/photos/time-tracker, title for forms,
slug for collectives, note for shares) and delegates to the trait.
DI plumbing:
* The greenfield-provider loop in registerBuiltinIntegrationProviders()
now passes IDBConnection as well; constructor signature normalised
to (db, appManager, l10n).
* SharesProvider switched from the (shareManager, l10n) constructor
to (db, appManager, l10n) so it uses the same trait. The
delete()-via-IManager path can come back in a follow-up once the
registry-driven share revocation flow is wired into the UI.
Seeded test data per leaf (raw SQL inserts via a one-off script;
non-committed): each upstream NC app now has one entity whose
marker-column contains '[or:{objectUuid}]'. Harness re-run captures
real content in the per-leaf screenshots:
* activity.png — 2 activity events
* analytics.png — 2 reports
* collectives.png — 1 page
* cospend.png — 1 project
* flow.png — 2 workflow operations
* forms.png — 1 form
* maps.png — 2 favorites
* photos.png — 2 albums
* shares.png — 2 shares
* time-tracker.png — 1 project
Combined with the existing wirings (bookmarks/talk/polls + the 6
already-real leaves), 16 of 24 advertised leaves now surface real
upstream data in OR's standalone integrations view.
Contributor
Author
Update: all 24 leaves now have real list() implementationsCommit Coverage mapExternal (OpenConnector-backed) — 2 leaves
Real list(), seeded with linked data — 16 leaves
Real list(), still unseeded on the dev container — 6 leaves
MarkerLookupTraitThe 10 new providers all use the same pattern: query the upstream NC app's main table via Per-leaf marker columns:
Screenshots regeneratedPer-leaf PNGs in Tests
🤖 Generated with Claude Code |
Closes #1530 (shares now wired end-to-end via marker note + linked file). Closes #1521 (activity). Code changes: * lib/Service/Integration/BuiltinProviders/AuditTrailProvider.php — try 'object_uuid' filter first (the UUID-typed column the sub-resource controller actually receives), fall back to the legacy 'object' filter (which is INTEGER-typed and rejects a UUID with SQLSTATE 22P02 on PostgreSQL). * lib/Db/AuditTrailMapper.php — add 'object_uuid' to findAll's filter whitelist so the new provider call path is honoured. * lib/Db/EmailLinkMapper.php — drop the tableExists() short-circuit on every read path. Doctrine's createSchema() cache can lag manual CREATE TABLE statements (sandboxes that recreated the table outside the migration framework) and was returning false here even with the table present, causing every email leaf list to be empty. Seeded entities (raw SQL + service APIs; non-committed scripts under /tmp/seed-*.php). One entity per leaf with the marker '[or:25706ca9-c989-4d6b-9f7b-98cf1cc70639]' in a text field — title, name, slug, subject, or note depending on the upstream app. Screenshots regenerated: all 25 PNGs (1 overview + 24 leaves) now show real linked content with the correct tab pill highlighted. Real items per leaf: files=1 notes=1 tags=1 tasks=1 audit-trail=11 shares=2 calendar=1 contacts=1 email=1 talk=3 deck=1 bookmarks=1 collectives=1 maps=2 photos=2 activity=2 analytics=2 cospend=1 flow=2 forms=1 polls=1 time-tracker=1 openproject=2 xwiki=1 OpenConnector source fix: openproject + xwiki sources now carry 'configuration.headers.Authorization: Basic <token>' (not the source- level 'headers' field, which OpenConnector's CallService doesn't merge into outgoing requests). XWiki source repointed at /rest/wikis/xwiki/spaces/XWiki/pages, OpenProject at /api/v3/projects.
Contributor
Author
🎯 24 / 24 leaves operational with real dataCommit Real-item count per leaf (from
|
| Group | Leaf | Items |
|---|---|---|
| External | xwiki | 1 |
| openproject | 2 | |
| Core built-ins | files | 1 |
| notes | 1 | |
| tags | 1 | |
| tasks | 1 | |
| audit-trail | 11 | |
| shares | 2 | |
| Comms | calendar | 1 |
| contacts | 1 | |
| 1 | ||
| talk | 3 | |
| Docs | bookmarks | 1 |
| collectives | 1 | |
| maps | 2 | |
| photos | 2 | |
| Workflow | activity | 2 |
| analytics | 2 | |
| cospend | 1 | |
| deck | 1 | |
| flow | 2 | |
| forms | 1 | |
| polls | 1 | |
| time-tracker | 1 | |
| Total | 45 linked items across 24 leaves |
Fixes shipped to get to 24/24
AuditTrailProvider.list()— triesobject_uuidfilter first (UUID-typed) before the legacyobjectfilter (INTEGER-typed, rejects UUIDs with SQLSTATE 22P02 on PostgreSQL).AuditTrailMapper::findAll()—object_uuidadded to filter whitelist.EmailLinkMapper— dropped thetableExists()short-circuit (Doctrine'screateSchema()cache was returning false even with the table present, killing every email lookup).- OpenConnector source config — auth header moved into
configuration.headers.Authorization(the level CallService actually merges into outgoing requests). xwiki + openproject now return 200 with real entity lists. - OR-side link tables (
openregister_email_links,openregister_contact_links,openregister_deck_links) — created manually on this container; the original migration's createTable statements never materialised the tables despite being recorded as run.
Test state
- Playwright
integration-registry.spec.ts— 31/31 green - Playwright
leaf-verification.spec.ts— 1/1 green (24/24 non-5xx, 24/24 with items > 0) - Playwright
leaf-screenshots.spec.ts— 1/1 green (25 PNGs regenerated, every leaf shows real content) - PHPUnit
LeafProvidersMetadataTest|XwikiProviderTest— 50/50 green
🤖 Generated with Claude Code
…all-or-prs Brings the leaf-system documentation from the integration tracking branch onto the documentation branch: * 19 leaf doc pages (1 overview + 18 leaves) — each with LeafCard metadata, Pair brand diagram, What it does / Setup / Local verification setup (curl + JSON sample) / Configuration / Using it / Troubleshooting / Related sections. * 25 PNG screenshots in docs/static/screenshots/integrations/ — each shows the corresponding tab active in the standalone OR IntegrationsView with real linked data. * docs/Integrations/index.md reorganized to lead with the leaf system + per-leaf grid. * docs/Integrations/verification-report.md — auto-generated against the live container with all 24 providers operational. * leaf-system.md, pluggable-integration-registry.md, xwiki-openconnector-source.yaml — the umbrella references. Source of truth: PR #1515 (integration/all-or-prs → development) ships the lib + frontend code that backs these docs. This commit contains docs only; the docusaurus build pipeline picks them up via the regular documentation-branch sync.
3 tasks
Adds the openspec change folder that restores per-app IMcpToolProvider discovery removed during a recent OR MCP refactor. Documents the interface contract, the 3-candidate discovery order (DI alias → ucfirst-FQCN → namespace-from-info.xml), the libxml workaround, and a delta against the existing mcp-discovery capability. Implementation scope (tasks.md, ~20 sub-tasks) and acceptance against the 11 persona scenarios under tests/mcp-personas/scenarios/.
Superseded by the existing ai-chat-companion-orchestrator change (openspec/changes/ai-chat-companion-orchestrator/), whose 11-section tasks.md already covers IMcpToolProvider interface (§2), built-in provider refactor of McpToolsService (§3), health probe endpoint (§4), SSE streaming (§5/6), Message.context migration (§7/8), and tests (§9-11). Implementing both in parallel would split the canonical source of truth for the same surface. The MCP per-app discovery regression that this revert undoes the documentation for is a symptom of the orchestrator change not having been applied yet — /opsx-apply ai-chat-companion-orchestrator is the canonical next step.
Adapter from OCA\OpenRegister\Mcp\IMcpToolProvider to the chat orchestrator's OCA\OpenRegister\Tool\ToolInterface. Per-app MCP tool providers (DecideskToolProvider, OpenBuiltToolProvider, …) are discovered + exposed via the MCP JSON-RPC endpoint, but the chat orchestrator (ResponseGenerationHandler) feeds the LLM from ToolRegistry which speaks ToolInterface, not IMcpToolProvider. This bridge closes that gap: ToolRegistry registers one McpProviderBridge per IMcpToolProvider and each provider's tool descriptors surface as individual LLphant functions; executeFunction() forwards back through provider->invokeTool(). Drafted during the persona-harness work in this session — depends on ai-chat-companion-orchestrator's IMcpToolProvider interface (already present on integration/all-or-prs but not yet on this branch's checkout state). The bridge itself compiles standalone and is imported only when both sides exist.
Self-contained PHP harness that drives a real LLM (Ollama, qwen3.5-optimized) against the live OpenRegister MCP endpoint. Each scenario describes a persona + a goal in natural language; the harness loops LLM → tool_call → MCP execute → tool_result until the LLM emits a final answer or hits max_turns, then runs DB asserts to verify side effects. Scenarios cover the full OpenBuilt authoring lifecycle (create app, add schema, add page, add widget, add menu item, publish) and the Decidesk meeting lifecycle (chair starts, secretary preps, coordinator checks tasks, PM reviews recents, secretary tracks action items). Harness features: - tool_succeeded assert (stronger than tool_called — requires non-error return) to catch silent failures - pre_sql cleanup hook for idempotent back-to-back sweeps - direct postgres asserts via docker exec, no NC bootstrap needed Acceptance target for ai-chat-companion-orchestrator: all 11 scenarios must pass on a back-to-back sweep once the orchestrator's IMcpToolProvider interface, built-in providers, ChatHealthController + ChatStreamController are in place.
…2026-05-18 feat(mcp): McpProviderBridge + LLM persona harness from this session
The dual-path discovery wave (OR's factory walking IAppManager + consumer apps' boot() calling addProvider() directly) was registering the same provider twice when both paths resolved successfully. Surfaced as duplicated decidesk.* tool ids in tools/list (8 unique ids appearing 2x each, 33 total entries for what should be 25 tools). Fix: addProvider() now checks for an existing provider with the same getAppId() and short-circuits with a debug log. The dual-path design is intentional (decidesk's boot() exists for the case where OR's alias lookup fails on its container scope), so dedup belongs at the sink, not by removing one of the two writers. Verified post-fix: tools/list returns 25 unique tools, 0 duplicates.
…ipped code The change was drafted with 0/45 tasks ticked but most of the work landed via 71ebebd (feat(chat-ai): SSE streaming + IMcpToolProvider + Message.context). Audit pass on integration/all-or-prs HEAD 1329a76 ticks 22/45 boxes for the work actually on disk and annotates the 23 truly-pending tasks with what's missing. Headline gaps: - §1 Fireworks spike never run + the `streaming`/`non-streaming-only` comment in ResponseGenerationHandler.php was never added (de-facto the controller runs in non-streaming mode) - §6 the 15s heartbeat interval is not implemented (one startup heartbeat only) - §7.3 + §8 Message.context column exists in DB but Message.php has no getter/setter and neither ChatController nor ChatStreamController call setContext on send - §9 only McpToolsServiceTest landed; 4 other test files still missing - §10 + §11.2-11.4 never verified
…+ §8)
Wires the CnAiContext JSON snapshot the frontend sends with each user
message through the full chain:
request body
→ ChatController::extractMessageRequestParams reads `context`
→ ChatService::processMessage(... , array $context = [])
→ MessageHistoryHandler::storeMessage(... , ?array $context = null)
→ Message::setContext($context) shadows the magic setter with
non-null array semantics, persisted as JSON via addType('json')
ChatStreamController gets the same treatment — the user-authored Message
row created by ChatService now carries the context the widget sent
(verified: row 40 in oc_openregister_messages has
{"app":"openbuilt","slug":"x"} after a smoke against /api/chat/stream).
Heartbeat (§6): the initial post-headers heartbeat is in place;
periodic 15s ticks during the synchronous LLM call require the §1
streaming spike outcome to land first (PHP-FPM has no background
tickers; LLPhant token callbacks would let us interleave events with
the in-flight call). Documented as a TODO in the controller body.
Smokes ran clean post-change:
- §5.7 unauth POST /api/chat/stream → HTTP 401
- §11.2 stream with auth → text/event-stream, X-Accel-Buffering: no,
heartbeat + final events emitted; final payload echoes the context
- §11.3 /api/chat/send unchanged JSON content-type contract
Resolves orchestrator tasks §7.3, §8.1, §8.2; partially resolves §6
(initial heartbeat shipped, ticker deferred).
Lands the four pending unit-test files from the orchestrator §9
backlog, all green against the OR test suite (1041 tests, 3339
assertions, 0 failures, 0 errors):
§9.5 tests/Unit/Db/MessageTest.php (extended)
- getContext()/setContext() round-trip simple, nested, and empty
payloads; null-default normalises to []; jsonSerialize() carries
the context key with the [] default
§9.4 tests/Unit/Migration/Version1Date20260511130000Test.php (new)
- 6 tests: changeSchema() adds the column when missing, is
idempotent when present, no-op when messages table missing;
same three for down() (drop column safely)
§9.3 tests/Unit/Controller/ChatHealthControllerTest.php (new)
- 5 tests: configured provider → 200 + capabilities; null/empty/
missing chatProvider → 503 + no_provider; SettingsService
throwing → 503 + config_error + warning log
§9.2 tests/Unit/Controller/ChatStreamControllerTest.php (new)
- 3 tests using a TestableChatStreamController subclass that
captures SSE frames in-memory and throws a sentinel exception
instead of exiting. Covers: unauthenticated path emits error
and never calls ChatService; missing message never reaches the
LLM; no token events on early-exit paths.
To enable the subclass pattern, four ChatStreamController methods
flipped private → protected:
- emitSseEvent (captured by test instead of echoed)
- emitAndExit (captured + throws sentinel instead of exit;)
- clearOutputBuffers (new; extracted from stream() so test can
skip — closing PHPUnit's output buffer
trips the risky-test detector)
- emitSseHeaders (new; extracted so test can skip — header()
warns "headers already sent" under PHPUnit)
§9.2(a) 6-event envelope shape and §9.2(b) streaming-vs-degradation
tests deferred — gated on §5.3 (token/tool_call/tool_result emission
not implemented) and §6 (15s heartbeat ticker not implemented).
Documented in tasks.md.
tasks.md updated: ticks §9.2-9.5 and §10.2; adds the second audit
header noting this hand-implemented slice.
rubenvdlinde
requested review from
Rem-Dam,
SudoThijn,
WilcoLouwerse,
bbrands02,
remko48 and
rjzondervan
as code owners
Contributor
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Retrospective audit findings (PR already merged). 15 findings — 9 Blockers + 6 Concerns. Standard mode.
| * @return JSONResponse | ||
| */ | ||
| #[NoAdminRequired] | ||
| public function index(string $register, string $schema, string $id, string $integrationId): JSONResponse |
Contributor
There was a problem hiding this comment.
🔴 Blocker — Broad IDOR: per-object authorization missing
Every method (index/show/create/update/destroy, lines 97-187) is decorated with #[NoAdminRequired] and dispatches straight into resolveProvider() with the caller-supplied $register, $schema, $id, $entityId — without first checking that the calling user is allowed to read or mutate the object identified by $id, and without verifying $entityId belongs to that object. Combined with builtin providers that perform no RBAC of their own (see findings on FilesProvider::list and NotesProvider::update/delete), any authenticated NC user can enumerate / create / overwrite / delete files and comments on objects they cannot see in the UI. Add a PermissionHandler::canRead($register, $schema, $id, $user) (and canWrite for non-GET) check before resolveProvider() and return a 403 envelope on failure. OWASP A01:2021 / ADR-005 Rule 3.
| return $this->noteService->createNote($objectId, $message); | ||
| }//end create() | ||
|
|
||
| public function update(string $register, string $schema, string $objectId, string $entityId, array $payload): array |
Contributor
There was a problem hiding this comment.
🔴 Blocker — update/delete accept any comment id without ownership check
update((int) $entityId, $message) (line 107) and delete((int) $entityId) (line 112) forward the caller-supplied integer comment id directly into NoteService with no verification that the comment actually belongs to $objectId (or to the current user). Paired with the IDOR on the controller above, a non-admin user can edit/delete arbitrary comments by guessing or harvesting integer IDs from index() output. Resolve the comment, assert comment.objectId === $objectId and (comment.owner === currentUser || isAdmin), then proceed.
| try { | ||
| $mapper = $this->container->get($candidate); | ||
| if (is_object($mapper) === true && method_exists($mapper, 'find') === true) { | ||
| return $mapper->find($objectId); |
Contributor
There was a problem hiding this comment.
🔴 Blocker — mapper->find() called without RBAC; errors swallowed to []
resolveObject($objectId) (lines 129-149) walks three candidate mappers and calls mapper->find($objectId) with no RBAC/multitenancy arguments — so any authenticated user can enumerate the files folder of any object whose UUID they know via GET /api/objects/.../integrations/files. The top-level catch (\Throwable) { return []; } at line 116 also collapses 'object not found', 'permission denied' and 'infrastructure error' into the same empty-list response; clients cannot distinguish a real permission denial from a missing object, and nothing is logged. Use an RBAC-aware lookup (or call PermissionHandler::canRead(...) first) and let permission failures surface as 403 from the controller.
| // NotifyPushListener::flushBatch($queue, $permissionHandler); | ||
| // NotifyPushListener::setBatchMode(false); | ||
| // } | ||
| \OCA\OpenRegister\Listener\NotifyPushListener::setBatchMode(true); |
Contributor
There was a problem hiding this comment.
🔴 Blocker — Batch mode activated but flushBatch() never called → all events silently dropped
Both bulk-import paths (processSpreadsheetBatch here at line 775 and processCsvSheet at line 965) wrap saveObjects() in NotifyPushListener::setBatchMode(true) … finally { setBatchMode(false); } with the inline @todo 'Cannot call flushBatch here without IQueue'. The result: every per-object event during the import is suppressed by batch mode, and the collection event that batch mode is supposed to emit on flush is never fired. Net behaviour: imports become entirely silent on the notify_push channel even when notify_push is fully operational. Either inject IQueue + PermissionHandler and call flushBatch() in the finally, or do not flip setBatchMode(true) at all (per-object pushes are at least correct).
| * | ||
| * @spec openspec/changes/add-live-updates/tasks.md#task-4 | ||
| */ | ||
| public static function flushBatch(object $queue, PermissionHandler $permHandler): void |
Contributor
There was a problem hiding this comment.
🔴 Blocker — flushBatch() broadcasts to every connected client (no user scoping)
flushBatch (lines 336-368) pushes each accumulated collection event with no user field on the payload — i.e. notify_push treats this as a broadcast to every connected client subscribed to the channel. This relies on the API to enforce RBAC on every subsequent refetch and on every client implementing the 'trust nothing, refetch' pattern correctly. Worse, the collection-channel name (or-collection-{registerSlug}-{schemaSlug}) leaks register/schema slugs to listeners who have no read permission on the schema (timing oracle for admin-only schemas). Loop over PermissionHandler::getReadableByUsers() and push per-user, same as dispatchPushes() (line ~233).
| $isAdmin = $this->currentUserIsAdmin(); | ||
| $rows = []; | ||
|
|
||
| foreach ($this->registry->list() as $provider) { |
Contributor
There was a problem hiding this comment.
🟡 Concern — Full provider list leaks to every authenticated user
foreach ($this->registry->list() as $provider) enumerates every registered integration into the capability response. Every authenticated NC user (any non-admin hitting /ocs/v2.php/cloud/capabilities) receives id, label, group, enabled, storageStrategy, surfaces for every integration — including provider classes the user has no permission to interact with. This leaks the deployed integration topology (e.g. 'this tenant ships XWiki + OpenProject + email') to any logged-in user — useful reconnaissance for a low-privileged attacker. Either filter by what the user can reach, or scope the capability to an admin-only path.
| sort($registered); | ||
| $hint = ($registered === []) ? '(no providers registered)' : implode(', ', $registered); | ||
| throw new NotImplementedException( | ||
| sprintf("Integration '%s' is not registered. Registered: %s", $integrationId, $hint) |
Contributor
There was a problem hiding this comment.
🟡 Concern — 404 body reflects full registered integration list
The error message sprintf("Integration '%s' is not registered. Registered: %s", $integrationId, $hint) embeds the full list of registered integrations in the 404 response body. Combined with the IntegrationsCapability leak above, this is a second redundant disclosure path of internal topology to any authenticated user. Replace with a static 'Unknown integration id' message and keep the registered-list logging server-side only.
| </a> | ||
| <?php endif; ?> | ||
| <?php if (($row['testConnectionUrl'] ?? null) !== null) : ?> | ||
| <a class="button" href="<?php p((string) $row['testConnectionUrl']); ?>" target="_blank" rel="noopener"> |
Contributor
There was a problem hiding this comment.
🟡 Concern — Test-connection rendered as plain GET <a target="_blank"> (no CSRF gate)
The Test-connection link is <a class="button" href="<?php p((string) $row['testConnectionUrl']); ?>" target="_blank" rel="noopener">. Today show() only reads provider metadata so the GET is genuinely idempotent — but the column is rendered as a plain anchor, which means any future maintainer who adds a side-effecting 'trigger an actual probe upstream' inside show() (or a new testConnection action) inherits a CSRF-vulnerable click. Mark the button with data-action="test-connection" and wire it via fetch + CSRF token, or at minimum add an inline comment forbidding side effects on the GET path.
| // MessageHistoryHandler::storeMessage(). Reject anything other | ||
| // than an associative array so a bad client payload doesn't | ||
| // overflow the column or break the JSON encoding. | ||
| $contextArr = is_array($context) === true ? $context : []; |
Contributor
There was a problem hiding this comment.
🟡 Concern — Non-array context silently collapsed to []
$contextArr = is_array($context) === true ? $context : []; — a malformed/non-object payload (string, number, accidental JSON-as-string) is silently collapsed to [] and persisted as such. The user sees a working response but the orchestrator loses the entire context-snapshot side of the conversation. Either reject with 400 / bad_context or emit a structured warning SSE event so the frontend can surface it. The mirror change in ChatController.php:259-265 has the same shape.
| $qb = $this->db->getQueryBuilder(); | ||
| $qb->select($qb->func()->count('*')) | ||
| ->from('information_schema.tables') | ||
| ->where($qb->expr()->eq('table_name', $qb->createNamedParameter('oc_'.$this->getTableName()))); |
Contributor
There was a problem hiding this comment.
🟡 Concern — Hard-coded oc_ table prefix ignores dbtableprefix
$qb->expr()->eq('table_name', $qb->createNamedParameter('oc_'.$this->getTableName())) — the value is parameterized (SQL-safe), but the oc_ prefix is hard-coded and ignores the deployment's dbtableprefix system config. Installs that customise the prefix will silently keep returning false from the information_schema fallback and continue to fail the read path. Use \OC::$server->getConfig()->getSystemValue('dbtableprefix', 'oc_') or read it from IDBConnection.
WilcoLouwerse
requested changes
May 18, 2026
Contributor
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
9 🔴 blockers require fixes (informational — this rollup PR is already merged; the canonical follow-up is on the source PRs): IDOR on ObjectIntegrationsController, NotesProvider ownership check missing, FilesProvider RBAC bypass, ImportService batch mode without flush, NotifyPushListener broadcast leak, PermissionHandler silent drop for admin, SPDX missing on 7 files, error_log in MarkerLookupTrait, LogDanglingLinkedTypes OOM. Plus 6 🟡 concerns (silent provider catch, integration list leak via capability, integration list leak via 404, plain-GET test-connection, ChatStream silent context drop, hard-coded oc_ prefix). The IDOR + RBAC findings (1–6) belong to source PR #1453 (live-updates) and #1469 (built-in providers); please consider re-opening those PRs for the fixes. Newman API test suite is also failing on this head SHA.
This was referenced May 18, 2026
Merged
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.
Purpose
Do not merge. Tracking/integration branch bundling all 14 open PRs by @rubenvdlinde so we have a single buildable surface to continue developing on. Each source PR stays open and is the canonical review surface.
State
integration/scholiq-deps-all+origin/development(merged so/api/chat/healthroute lands → CnAiCompanion FAB renders in apps)development, 0 behindSource PRs — review-feedback action taken
Reviewer comments were inspected via
gh api repos/.../pulls/<N>/comments. ✅ = pushed to source branch + re-merged here. 📝 = surfaced for your decision (not auto-fixed). ⏸ = already handled by author.//comments, RBAC claim)1d867eb42(WilcoLouwerse, 2026-05-15 10:49) — needs MWest2020 re-reviewelse if→elseif(phpcs,ExternalIntegrationRouter:291).cca2cbff0LogDanglingLinkedTypes.php+PropertyReferenceTypeValidator.php.cf61ea735286c24884IntegrationsController.php+ObjectIntegrationsController.php.09dd70a8bprint_unescaped/broken badge closures (thematchstrings calledp()which echoes, leaving empty spans + leaking status text); [BLOCKER] double-registration inApplication.php(removed redundant registerService, mirrorsOpenRegisterAdminpattern); [BLOCKER] SPDX inIntegrationsAdminSettings.php.620811fe08616ab819normalizeRowwas unreachable past$page(cast-to-string never null,??short-circuited); breadcrumbarray_values/filter/mergeindentation (phpcs); SPDX inXwikiProvider.php.f8b0e2412Surfaced for maintainer judgement (not auto-fixed)
These need architectural/design decisions you should make. I won't guess and risk regressions.
#1453 (live-updates) — multiple correctness blockers
flushBatch()broadcasts to ALL connected clients (RBAC bypass)lib/Listener/NotifyPushListener.php:337— needs per-user filter viagetReadableByUsersafter fixing thegetReadableByUsersbug below.ImportService::saveObjects()butflushBatch()never calledlib/Service/ImportService.php:785— events silently dropped. PR body claims the opposite.getReadableByUsers()returns[]for admin-group schemas — admin-restricted events get lost.lib/Service/PermissionHandler.php:1053.#1467 — beyond SPDX, several "concerns" flagged
AbstractIntegrationProvidershipsNotImplementedExceptionstubs as trapdoors for CRUD (design intent vs accidental landmine?)Throwablecatch inExternalIntegrationRouter::loadSource()reclassifies all exceptions as source-missingwithProviders()is public on the shared singleton — public-API decision: keep, rename internal, or refactor to a builder?IntegrationRegistry::list()shadowsIntegrationProvider::list()— rename tolistProviders()/all()?#1468 — false-positive risk
LogDanglingLinkedTypes::scan()flagsVALID_LINKED_TYPESids as dangling on every upgrade run (false positives until providers register).findAll()with no limit could OOM on large installs.PropertyReferenceTypeValidator::validate()rejectsreferenceType='files'because built-in providers aren't yet registered when validation runs — bootstrap ordering question.#1469 — per-provider RBAC concerns
FilesProvider::list(),NotesProvider::*,TasksProvider::list(),AuditTrailProvider::list()— reviewer flags missing user-scope/ownership checks. Need design decision: should the provider enforce, or does the controller enforce, or does the underlying NC API enforce?catch(\Throwable){}inApplication::bootBuiltinIntegrationProviders— swallows provider-construction failures silently.#1473 — concerns (not flagged as blockers by reviewer)
#1475 — concerns
probe()message may leak exception details; plain-GET<a>test-connection link is CSRF-risky; no non-admin / unauth test.Cross-cutting CI
.license-overrides.jsonondevelopment; source PRs will pick it up on dev-merge.What to do next (suggested order)
origin/developmentinto each source PR branch to pick up the dompdf override + green CI.The mechanical SPDX/phpcs/XSS fixes are landed and pushed (21 files, 7 source PRs).