feat(integration-registry): routes + controllers + OCS capability (umbrella PR 4/N)#1473
Conversation
…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
There was a problem hiding this comment.
[BLOCKER] SPDX license header missing in new lib/ files (Gate 1)
IntegrationsController.php and ObjectIntegrationsController.php both lack standalone // SPDX-License-Identifier: and // SPDX-FileCopyrightText: comment lines required by hydra-gate-spdx. PHPDoc @license tags are not a substitute. Add the SPDX comment lines at the top of each file.
There was a problem hiding this comment.
[CONCERN] Capability served to unauthenticated callers — provider list leaks to the public
ICapability::getCapabilities() is called for every /ocs/v2.php/cloud/capabilities request, including anonymous ones. currentUserIsAdmin() correctly returns false for anonymous, but the full provider list (id, label, group, enabled, storageStrategy, surfaces) is still returned to an anonymous caller. Either return an empty providers array when the user is not authenticated, or implement IPublicCapability as an explicit opt-in and document it. Opt-in is safer.
There was a problem hiding this comment.
[CONCERN] resolveProvider leaks full registered-provider list in 404 message
When an unknown integrationId is requested, resolveProvider() throws NotImplementedException with message "Integration 'X' is not registered. Registered: files, tags, xwiki, …". This is returned verbatim in the 404 body. Any authenticated user can enumerate registered integration IDs by probing. Strip the hint or replace with a static message.
There was a problem hiding this comment.
[CONCERN] No rate limiting on discovery endpoints
show() calls describe($provider, $isAdmin) which for admin callers invokes $provider->health() — a potentially expensive network call to an upstream service. There is no #[UserRateThrottle] or #[AnonRateThrottle]. Add rate-limit attributes to both index() and show().
There was a problem hiding this comment.
[CONCERN] No test for unauthenticated or cross-user IDOR scenario
The test suite covers happy path, 404, 501, 503, but contains zero tests for: (1) unauthenticated request, (2) authenticated user not owning the object. Adding these tests would immediately expose the missing ownership guard.
There was a problem hiding this comment.
[CONCERN] Throwable silently swallowed in describe() — no log on health failure
When $provider->health() throws, the catch block sets a static authStatus payload but does not log the exception. A repeatedly failing health check is invisible in server logs. Log at warning level: $this->logger->warning('[IntegrationsController] health() threw for provider …', ['exception' => $e]).
There was a problem hiding this comment.
[CONCERN] collectPayload() passes raw request params to providers
collectPayload() calls $this->request->getParams() which returns all parameters including framework-injected keys (e.g. _route). The method removes only the five explicitly named path params. Any other framework-injected key is passed to $provider->create/update. Use $this->request->getPostParams() (body-only) or apply an allowlist/denylist that strips underscore-prefixed internal keys.
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Review
🔴 Blockers (4)
- Missing #[NoCSRFRequired] on public API routes (
lib/Controller/IntegrationsController.php:327)
Bothindex()andshow()are registered inappinfo/routes.phpas standard (non-OCS) routes and carry only#[NoAdminRequired]. Without#[NoCSRFRequired], every non-browser REST client (curl, mobile app, OpenConnector) hittingGET /api/integrationsorGET /api/integrations/{id}will get a CSRF token rejection. Sibling controllers (RegistersController, SchemasController) all declare#[NoCSRFRequired]alongside#[NoAdminRequired]on theirindex/show. Add#[NoCSRFRequired]. - Missing #[NoCSRFRequired] on all five ObjectIntegrations routes — including POST/PUT/DELETE (
lib/Controller/ObjectIntegrationsController.php:550)
All five methods (index,show,create,update,destroy) have only#[NoAdminRequired]. For the read methods this is the same issue as IntegrationsController, but forcreate(POST),update(PUT),destroy(DELETE), machine-to-machine callers without a CSRF token will be rejected with 401. This breaks the stated use-case of the sub-resource dispatch API. - No per-object ownership / permission check — IDOR on all five endpoints (Gate 8) (
lib/Controller/ObjectIntegrationsController.php:550)
Every method is#[NoAdminRequired]with no guard in the method body. Any authenticated user can callGET /api/objects/{register}/{schema}/{id}/integrations/{integrationId}where{id}belongs to another user and receive linked data (files, tags, audit trail, …). The object identified by{register}/{schema}/{id}is never checked for read/write permission before being passed to$provider->list/get/create/update/delete. Mutating operations have no write-permission check at all. Add a per-object ACL check (or delegate to existing ObjectsController auth). - SPDX license header missing in new lib/ files (Gate 1) —
lib/Controller/IntegrationsController.php:1
🟡 Concerns (6)
- Capability served to unauthenticated callers — provider list leaks to the public —
lib/Capabilities/IntegrationsCapability.php:161 - resolveProvider leaks full registered-provider list in 404 message —
lib/Controller/ObjectIntegrationsController.php:699 - No rate limiting on discovery endpoints —
lib/Controller/IntegrationsController.php:327 - No test for unauthenticated or cross-user IDOR scenario —
tests/Unit/Controller/ObjectIntegrationsControllerTest.php:860 - Throwable silently swallowed in describe() — no log on health failure —
lib/Controller/IntegrationsController.php:424 - collectPayload() passes raw request params to providers —
lib/Controller/ObjectIntegrationsController.php:806
🟢 Minor (1)
- objectIntegrations index/create share URL pattern — verify verb-based dispatch (
appinfo/routes.php:16)
objectIntegrations#index(GET) andobjectIntegrations#create(POST) both resolve to/api/objects/{register}/{schema}/{id}/integrations/{integrationId}. NC's router differentiates on HTTP verb, but pre-NC 27 had a known dedup issue. A comment confirming verb-based dispatch is tested would help.
Reviewed by WilcoLouwerse via automated batch review.
🔍 Retrospective audit findings — surfaced from #1515While reviewing integration rollup PR #1515, two findings trace back to the routes/controllers in this PR:
The PR body of #1515 lists finding 2 under "#1473 concerns (not flagged as blockers by reviewer)" but did not surface the broader IDOR pattern. Worth a fixup PR adding |
Stacked on #1469. Tasks 18-22 of #1307 — Backend — Routes, Controller, Capabilities.
IntegrationsController—GET /api/integrations(withgroup+enabledfilters) +GET /api/integrations/{id}. Role-redacted descriptor per AD-17 (public surface for everyone; admin-only fields omitted for non-admins, not null-stubbed).ObjectIntegrationsControllerowns/api/objects/{register}/{schema}/{id}/integrations/{integrationId}[/{entityId}](GET / POST / PUT / DELETE). Additive —ObjectsController(2400+ lines) untouched. Error translation:NotImplementedException→ 501 withQueryTimeContractenvelope;ProviderUnavailableException→ 503 withdetails.causepayload (AD-23); unknown id → 404; other → 500 (real exception logged, generic message returned per ADR-005).appinfo/routes.php— 2 discovery + 5 sub-resource.IntegrationsCapability— surfaces the registry through/ocs/v2.php/cloud/capabilities, role-redacted per AD-17. Lives inlib/Capabilities/mirroringUrnCapability(the canonical pattern in this codebase).$context->registerCapability()inApplication::register()— noappinfo/info.xmlchange (OR registers capabilities in code, not XML).Net new files
lib/Controller/IntegrationsController.phplib/Controller/ObjectIntegrationsController.phplib/Capabilities/IntegrationsCapability.phptests/Unit/Controller/ObjectIntegrationsControllerTest.phpModified
appinfo/routes.php— 7 new routeslib/AppInfo/Application.php— DI bindings for both controllers + the capability;registerCapability()callopenspec/changes/pluggable-integration-registry/tasks.md+plan.jsonTests
ObjectIntegrationsController(dispatch + error translation); the 45 from PRs 1-3 still passSpec deviations (flagged inline in tasks.md / plan.json)
ObjectIntegrationsController) rather than a refactor ofObjectsController. Additive, zero regression risk on existing routes. The legacy/api/objects/{...}/files,/api/objects/{...}/notesetc. keep working unchanged.ICapabilityclass (IntegrationsCapability) registered viaregisterCapability(), mirroringUrnCapability. Spec called for editinglib/Service/CapabilitiesService.php— OR doesn't have that file as the canonical integration point.Cumulative progress on the umbrella
22/69 umbrella tasks done after this PR merges.
Stacking note
Base is
feature/1307/builtin-providers(PR #1469), which is stacked onfeature/1307/schema-validator-refactor(PR #1468), which is stacked onfeature/1307/pluggable-integration-registry(PR #1467). When the earlier PRs merge intodevelopment, GitHub retargets this PR todevelopmentautomatically.🤖 Generated with Claude Code