feat(integration-registry): admin UI surfacing auth status (umbrella PR 5/N) by rubenvdlinde · Pull Request #1475 · ConductionNL/openregister

rubenvdlinde · 2026-05-11T19:05:19Z

Part of the pluggable-integration-registry umbrella. Stacked on #1473 (routes + controllers + capability) — GitHub auto-retargets to development after that merges.

Summary

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 of the umbrella.

Files

lib/Settings/IntegrationsAdminSettings.php (new) — ISettings implementation
templates/settings/integrations-admin.php (new) — server-rendered table with status badges
appinfo/info.xml — registers the additional <admin> entry
lib/AppInfo/Application.php — DI binding for IntegrationsAdminSettings
tests/Unit/Settings/IntegrationsAdminSettingsTest.php (new) — 5 tests

Spec deviation

Spec called for "edit CapabilitiesService" — there's no such integration point on master. Capability is exposed via the idiomatic ICapability class added in PR 4 (IntegrationsCapability), not via this settings change. Documented in tasks.md.

Test plan

./vendor/bin/phpunit --configuration phpunit-unit.xml tests/Unit/Settings/IntegrationsAdminSettingsTest.php → 5 tests, 21 assertions, OK
Full unit suite stays green after this change
Smoke-test in a live Nextcloud after the umbrella branch merges — admin section should appear at Settings → Administration → OpenRegister with the table populated by the 5 built-in providers from PR 3

…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

WilcoLouwerse · 2026-05-12T13:21:30Z

[BLOCKER] Double-registration: settings class registered in both info.xml and Application.php Bootstrap

IntegrationsAdminSettings is declared in appinfo/info.xml as an <admin> settings entry AND again registered as a service via $context->registerService() in Application.php (lines 24-35). Nextcloud's Bootstrap registers ISettings classes automatically from info.xml; manually calling registerService() for the same class will cause it to be instantiated a second time, potentially showing the admin page twice in the sidebar and doubling any side-effects. Remove the registerService() block — info.xml registration is sufficient and idiomatic.

WilcoLouwerse · 2026-05-12T13:21:31Z

@@ -0,0 +1,224 @@
+<?php


[BLOCKER] SPDX license header missing (Gate 1)

The file has an EUPL-1.2 mention only in the DocBlock comment but is missing the machine-readable SPDX header lines required by the SPDX gate. Compare with templates/settings/integrations-admin.php which correctly has SPDX-License-Identifier: EUPL-1.2 and SPDX-FileCopyrightText: as standalone comment lines. Add these two SPDX lines at the top of the file.

WilcoLouwerse · 2026-05-12T13:21:33Z

[BLOCKER] print_unescaped() used for status badges

Lines 388 and 389 call print_unescaped($statusBadge(...)) and print_unescaped($authBadge(...)). The closures embed the status string via p($status) inside a hard-coded <span>, but the default branch of the match expressions passes the raw $status/$authStatus string to p(). Any future change that forgets p() will silently introduce XSS. Build the badge HTML in a way that doesn't require print_unescaped, or add a contract test that an unknown-status value with HTML special characters is rendered safe.

WilcoLouwerse · 2026-05-12T13:21:34Z

+            return $this->urlGenerator->getAbsoluteURL(
+                sprintf('/index.php/apps/openconnector/sources/%s', rawurlencode($sourceId))
+            );
+        }


[CONCERN] router->probe() result passed verbatim — message field may contain exception details

probeHealth() returns $this->router->probe($provider) directly, and the message key from that result is passed to the template as $row['message']. The template does not render it currently, but the field is present in the row. If a future template change renders the message, it could expose internal stack traces or credential-adjacent error strings. Sanitise/truncate the message before adding it to the row, or document that probe() must never include sensitive data.

WilcoLouwerse · 2026-05-12T13:21:36Z

[CONCERN] Test connection link opens OCS endpoint via plain GET — possible CSRF risk

<a href="..." target="_blank"> for the OCS test-connection URL is opened via plain GET. If the underlying controller action has any side-effect (calling the external system), exposing it as a no-token GET link is a CSRF risk. Additionally, opening the raw OCS JSON in a new tab provides a poor UX. Use an XHR/fetch call with requesttoken, or confirm the endpoint is strictly read-only.

WilcoLouwerse · 2026-05-12T13:21:38Z

[CONCERN] No test for non-admin or unauthenticated user access

The test suite covers section/priority/row shape/router delegation but has no test asserting that a non-admin or anonymous user cannot reach this settings page. Add an integration test or at least assert that getSection() returns a value listed under <admin-section>.

WilcoLouwerse · 2026-05-12T13:21:39Z

+        if ($provider->getStorageStrategy() === 'external') {
+            return $this->router->probe($provider);
+        }
+


[CONCERN] probeHealth() called synchronously for every provider on page load — no timeout

buildRows() iterates every registered provider and calls probeHealth() synchronously. For external providers this goes through router->probe() which presumably makes outbound HTTP. With N external integrations, page load blocks for N × (network timeout) seconds. Defer to client-side XHR, cache with a short TTL (ICache), or execute with a hard 2 s timeout per probe.

WilcoLouwerse · 2026-05-12T13:21:41Z

[CONCERN] Section ID alignment not verified

getSection() returns 'openregister'. The existing <admin-section>OCA\OpenRegister\Sections\OpenRegisterAdmin</admin-section> must return 'openregister' as its getId() for this to resolve. If the existing section getId() returns a different value, the settings page will not appear in the sidebar. The test suite does not cover this wiring.

WilcoLouwerse

Review

🔴 Blockers (3)

Double-registration: settings class registered in both info.xml and Application.php Bootstrap — lib/AppInfo/Application.php:24
SPDX license header missing (Gate 1) — lib/Settings/IntegrationsAdminSettings.php:1
print_unescaped() used for status badges — templates/settings/integrations-admin.php:388

🟡 Concerns (5)

router->probe() result passed verbatim — message field may contain exception details — lib/Settings/IntegrationsAdminSettings.php:221
Test connection link opens OCS endpoint via plain GET — possible CSRF risk — templates/settings/integrations-admin.php:402
No test for non-admin or unauthenticated user access — tests/Unit/Settings/IntegrationsAdminSettingsTest.php:585
probeHealth() called synchronously for every provider on page load — no timeout — lib/Settings/IntegrationsAdminSettings.php:178
Section ID alignment not verified — appinfo/info.xml:9

🟢 Minor (1)

Test stub naming convention (tests/Unit/Settings/IntegrationsAdminSettingsTest.php:474)
Classes _AdminNativeProvider and _AdminExternalProvider use a leading underscore that is not PSR-4 idiomatic. Rename to AdminNativeProviderStub / AdminExternalProviderStub to follow project conventions.

Reviewed by WilcoLouwerse via automated batch review.

WilcoLouwerse blockers: 1. **XSS/broken-badge fix in templates/settings/integrations-admin.php** The original $statusBadge / $authBadge closures embedded `p()` calls inside the match-expression strings. `p()` echoes (it doesn't return), so: - the spans rendered as '<span class="..."></span>' (empty) - 'ok' / 'degraded' / etc. leaked to output buffering Rewritten as $statusBadgeClass / $authBadgeClass returning just the class string; template now emits both class + value through p() — single escape, no print_unescaped, no broken concatenation. 2. **Remove double-registration in lib/AppInfo/Application.php** IntegrationsAdminSettings is declared in info.xml <admin>; the explicit registerService block was redundant (IntegrationRegistry + ExternalIntegrationRouter are already registered earlier in the bootstrap, and the remaining deps autowire). Mirrors how OpenRegisterAdmin is registered. 3. **SPDX headers** added to lib/Settings/IntegrationsAdminSettings.php inside the file docblock (per feedback_spdx-in-docblock.md).

rubenvdlinde requested review from Rem-Dam, WilcoLouwerse, bbrands02 and rjzondervan as code owners May 11, 2026 19:05

WilcoLouwerse reviewed May 12, 2026

View reviewed changes

WilcoLouwerse requested changes May 12, 2026

View reviewed changes

This was referenced May 15, 2026

feat(integration-registry): 16 leaf providers (mega-leaf — depends on umbrella stack) #1514

Merged

integration: rollup of 14 open OR PRs (tracking/integration branch) #1515

Merged

Base automatically changed from feature/1307/routes-controller-capabilities to development May 18, 2026 11:19

rubenvdlinde merged commit cdd084e into development May 18, 2026
16 of 19 checks passed

rubenvdlinde deleted the feature/1307/admin-ui-for-auth branch May 18, 2026 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(integration-registry): admin UI surfacing auth status (umbrella PR 5/N)#1475

feat(integration-registry): admin UI surfacing auth status (umbrella PR 5/N)#1475
rubenvdlinde merged 2 commits into
developmentfrom
feature/1307/admin-ui-for-auth

rubenvdlinde commented May 11, 2026

Uh oh!

WilcoLouwerse May 12, 2026

Uh oh!

WilcoLouwerse May 12, 2026

Uh oh!

WilcoLouwerse May 12, 2026

Uh oh!

WilcoLouwerse May 12, 2026

Uh oh!

WilcoLouwerse May 12, 2026

Uh oh!

WilcoLouwerse May 12, 2026

Uh oh!

WilcoLouwerse May 12, 2026

Uh oh!

WilcoLouwerse May 12, 2026

Uh oh!

WilcoLouwerse left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rubenvdlinde commented May 11, 2026

Summary

Files

Spec deviation

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilcoLouwerse left a comment

Choose a reason for hiding this comment

Review

🔴 Blockers (3)

🟡 Concerns (5)

🟢 Minor (1)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants