feat(integration-registry): 5 built-in IntegrationProvider implementations (umbrella PR 3/N)

[rubenvdlinde]

Stacked on #1468. Tasks 12-17 of #1307 — Backend — Built-in Providers.

Task	Provider	Storage	Required app	Notes
3.1	FilesProvider	magic-column	null	list() delegates to FileService::getFilesForEntity(); mutation throws (FileController owns the write path)
3.2	NotesProvider	link-table	null	Full CRUD delegation to NoteService
3.3	TasksProvider	link-table	null	Full CRUD delegation to TaskService; composite {calendarId}/{taskUri} entity ids
3.4	TagsProvider	link-table	null	list() via ISystemTagObjectMapper; mutation throws (TagsController owns the write path)
3.5	AuditTrailProvider	query-time	null	Read-only by design (AD-22); mutation inherits NotImplementedException from the abstract base
3.6	Bootstrap wiring	—	—	New registerBuiltinIntegrationProviders() (DI) + bootBuiltinIntegrationProviders() (addProvider) helpers in Application.php

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 — registerBuiltinIntegrationProviders (DI) + bootBuiltinIntegrationProviders (addProvider) helpers
• openspec/changes/pluggable-integration-registry/tasks.md + plan.json

Tests

• 45 tests, 80 assertions — all green inside the openregister-postgres dev container
• 11 new for the 5 providers; the 34 from PRs 1 & 2 still pass

Spec deviations (flagged inline in tasks.md / plan.json)

1. FilesProvider + TagsProvider keep write paths at FileController / TagsController. Mutation methods throw NotImplementedException with a pointer to the umbrella's controller refactor (tasks 18-22). Surface change for callers: zero.
2. AuditTrailProvider mutations inherit the abstract base's defaults — per AD-22 query-time providers MUST throw on create/update/delete.
3. 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.

Stacking note

Base is feature/1307/schema-validator-refactor (PR #1468), which is in turn stacked on feature/1307/pluggable-integration-registry (PR #1467). When PRs #1467 and #1468 merge into development, GitHub will retarget this PR to development automatically — no rebase needed.

🤖 Generated with Claude Code

[WilcoLouwerse]

[BLOCKER] FilesProvider::list() has no user-scope guard

The provider calls FileService::getFilesForEntity($object) without scoping to the current user. If FileService::getFilesForEntity uses a raw IRootFolder path (or no folder at all) rather than IRootFolder->getUserFolder($uid), it can enumerate files belonging to other users. There is no IUser or uid injected into the provider, no evidence FileService internally restricts to the authenticated user's folder. The provider must inject IUserSession and verify returned nodes are within getUserFolder($uid).

[WilcoLouwerse]

[BLOCKER] NotesProvider methods pass no user context — notes from any user may leak

NoteService::getNotesForObject($objectId, ...) and the create/update/delete methods receive no user identity. If NoteService uses oc_comments rows keyed only on object_id without filtering by actor_id/owner, any authenticated user who knows an objectId can read or mutate notes written by other users. Inject IUserSession and pass the uid, or have NoteService enforce the filter itself.

[WilcoLouwerse]

[BLOCKER] TasksProvider::list() fetches tasks with no user/calendar ownership check

TaskService::getTasksForObject($objectId) is called without any uid scoping. CalDAV task lists are per-user; without filter, the provider can return tasks belonging to other users. calendarId in create() (line 918) is caller-supplied with no ownership verification — a user can create tasks in another user's calendar. Validate calendar ownership before delegating.

[WilcoLouwerse]

[BLOCKER] Bare catch(\Throwable $e){} silently swallows provider construction failures (Gate 7)

The inner loop catches every \Throwable with an empty body and no logging. A misconfigured or maliciously broken provider will fail invisibly, making debugging impossible and masking security-relevant injection errors. At minimum log at warning level with $e->getMessage() and the provider class name. The outer catch at line 123 has the same issue.

[WilcoLouwerse]

[BLOCKER] AuditTrailProvider::list() has no user-scope filter — exposes all objects' audit trail

AuditTrailMapper::findAllByObject($objectId) returns every audit entry for that object regardless of caller. Audit trails contain sensitive mutation history. The provider must verify the caller has read access to the parent object before surfacing its trail. No IUserSession is injected, so no ownership check is possible in the current implementation.

[WilcoLouwerse]

[BLOCKER] resolveObject() catches \Throwable with silent continue (Gate 7)

Lines 458-460 catch every \Throwable and silently continue — a real infrastructure failure (DB down, OOM) is treated the same as 'mapper not found'. The outer list() catch at line 431 does the same. Log at debug or warning level so the silent-failure gate is satisfied.

[WilcoLouwerse]

[CONCERN] Pagination default of 50 hardcoded; _page is zero-based — inconsistent with OR standard

$limit = (int) ($filters['_limit'] ?? 50) uses an undocumented default; offset is _page * limit (zero-based). OR's standard pagination typically uses _offset. Document the convention in the method docblock, or align with the OR-wide filter contract.

[WilcoLouwerse]

[CONCERN] TagsProvider hardcodes object type 'openregister' — silent empty for other types

getTagIdsForObjects([$objectId], 'openregister') hardcodes the object type. If OR object IDs were registered under a different type (e.g. fully-qualified class name, schema-scoped), the call returns an empty array and no error. The object type should be a named constant or injected, with a test covering the non-empty path.

[WilcoLouwerse]

[CONCERN] Test suite only covers metadata + NotImplementedException — no get()/list() delegation tests

The test docblock acknowledges wrapped-service delegation is not covered, deferring to 'integration tests'. But FilesProvider::list(), TasksProvider::list/create/delete, TagsProvider::list each have their own logic (resolveObject loop, splitEntityId, getTagIdsForObjects mapping). These are unit-testable with mocks and would catch the user-scoping bugs above.

[WilcoLouwerse]

[CONCERN] TasksProvider::create() accepts caller-supplied calendarId without validation

$calendarId = (string) ($payload['calendarId'] ?? '') passes the value directly to TaskService::createTask(). If TaskService does not verify the calendar belongs to the current user, an authenticated user can append tasks to another user's calendar. Validate calendar ownership (or shared with write access) before the delegate call.

[WilcoLouwerse]

[CONCERN] AuditTrailProvider::list() uses method_exists() duck-typing instead of a stable mapper interface

The provider checks method_exists($this->mapper, 'findAllByObject') then falls back to findAll(filters: ...). The production call path depends on which method the injected mapper happens to expose at runtime, making behaviour non-deterministic and untestable. Define a mapper interface and declare it as the constructor parameter type.

[WilcoLouwerse]

Review

🔴 Blockers (6)

• FilesProvider::list() has no user-scope guard — lib/Service/Integration/BuiltinProviders/FilesProvider.php:421
• NotesProvider methods pass no user context — notes from any user may leak — lib/Service/Integration/BuiltinProviders/NotesProvider.php:625
• TasksProvider::list() fetches tasks with no user/calendar ownership check — lib/Service/Integration/BuiltinProviders/TasksProvider.php:911
• Bare catch(\Throwable $e){} silently swallows provider construction failures (Gate 7) — lib/AppInfo/Application.php:141
• AuditTrailProvider::list() has no user-scope filter — exposes all objects' audit trail — lib/Service/Integration/BuiltinProviders/AuditTrailProvider.php:264
• resolveObject() catches \Throwable with silent continue (Gate 7) — lib/Service/Integration/BuiltinProviders/FilesProvider.php:444

🟡 Concerns (5)

• Pagination default of 50 hardcoded; _page is zero-based — inconsistent with OR standard — lib/Service/Integration/BuiltinProviders/NotesProvider.php:627
• TagsProvider hardcodes object type 'openregister' — silent empty for other types — lib/Service/Integration/BuiltinProviders/TagsProvider.php:763
• Test suite only covers metadata + NotImplementedException — no get()/list() delegation tests — tests/Unit/Service/Integration/BuiltinProvidersMetadataTest.php:1032
• TasksProvider::create() accepts caller-supplied calendarId without validation — lib/Service/Integration/BuiltinProviders/TasksProvider.php:918
• AuditTrailProvider::list() uses method_exists() duck-typing instead of a stable mapper interface — lib/Service/Integration/BuiltinProviders/AuditTrailProvider.php:265

🟢 Minor (1)

• bootBuiltinIntegrationProviders() declares parameter type as untyped (lib/AppInfo/Application.php:117)
  private function bootBuiltinIntegrationProviders($server): void omits the type hint for $server. Type as \OCP\IServerContainer or Psr\Container\ContainerInterface for static analysis.

Reviewed by WilcoLouwerse via automated batch review.

[WilcoLouwerse]

🔍 Retrospective audit findings — surfaced from #1515

While reviewing integration rollup PR #1515, three findings trace back to the built-in provider implementations in this PR:

1. 🔴 NotesProvider::update/delete accept any caller-supplied comment id without ownership check — paired with the controller IDOR (see feat(integration-registry): routes + controllers + OCS capability (umbrella PR 4/N) #1473), any authenticated user can edit/delete arbitrary comments by guessing integer IDs. Inline comment on #1515
2. 🔴 FilesProvider::list calls mapper->find() with no RBAC and the top-level catch (\Throwable) { return []; } swallows permission denials, infrastructure errors, and missing objects into the same empty-list response. Inline comment on #1515
3. 🔴 MarkerLookupTrait::findByMarker ships error_log() in production (Hydra gate-2 forbidden-pattern). Replace with $this->logger->warning(...). Inline comment on #1515

The PR body of #1515 surfaced finding 1 + 2 under "#1469 per-provider RBAC concerns" as a design-decision question — confirming they merged unresolved. Worth pinning the trust model (provider enforces vs controller enforces vs underlying NC API) and opening a fixup PR.