From 60a9dca2b1e98d4e6fcbc6ca3dab3415680f4047 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Tue, 1 Sep 2026 21:58:54 +0200 Subject: [PATCH] test(openwebui): make the MCP client open the connection, not just record it GET /api/v1/tools/ builds its answer for an MCP server out of stored config alone - routers/tools.py:125-126 reads info off the connection row and never touches the network. So the three specs that read it pass unchanged against a provider that is down, refuses the deployed bearer, or serves nothing. What they establish is registration and the OIDC group to access-grant chain, not that the bridge carries. POST /api/v1/configs/tool_servers/verify is the one route in 0.11.0 that does a real MCP initialize plus tools/list without an LLM, and it authenticates with the key in the body it is handed. The spec reads the stored connection and echoes it back, so the handshake runs over the deployed URL with the deployed credential through Open WebUI's own client, and asserts three things per server: it answers, it serves at least one tool, and the tool set contains the read probe its meta/mcp.yml declares. Only bearer connections are verified. A system_oauth one resolves the caller's own upstream token per request and the administrator holds none, so it would fail on the caller rather than on the bridge; no provider declares auth: oidc today, but the filter keeps the spec honest when one does. The per-test budget scales with the number of registered servers instead of a fixed guess. MCP_EXPECTED_TOOLS carries the declared read probe per server. Its rendering is unit-tested by extracting the block from the template itself rather than restating it, and decoded the way a spec decodes it - dotenv_quote emits an escaped JSON string that decodeDotenvQuotedValue unwraps before the spec parses the payload, so comparing after one decode would test a value no spec ever sees. Co-Authored-By: Claude Opus 5 (1M context) --- .../files/playwright/_shared.js | 1 + .../files/playwright/playwright.spec.js | 1 + .../files/playwright/test-mcp-bridge.js | 86 +++++++++++++++++ .../templates/playwright.env.j2 | 6 ++ .../web-app-openwebui/templates/__init__.py | 3 + .../templates/test_playwright_read_probes.py | 95 +++++++++++++++++++ 6 files changed, 192 insertions(+) create mode 100644 roles/web-app-openwebui/files/playwright/test-mcp-bridge.js create mode 100644 tests/unit/python/roles/web-app-openwebui/templates/__init__.py create mode 100644 tests/unit/python/roles/web-app-openwebui/templates/test_playwright_read_probes.py diff --git a/roles/web-app-openwebui/files/playwright/_shared.js b/roles/web-app-openwebui/files/playwright/_shared.js index 50caa0c137..905c80ef0b 100644 --- a/roles/web-app-openwebui/files/playwright/_shared.js +++ b/roles/web-app-openwebui/files/playwright/_shared.js @@ -19,6 +19,7 @@ const env = { biberPassword: decodeDotenvQuotedValue(process.env.BIBER_PASSWORD), canonicalDomain: decodeDotenvQuotedValue(process.env.CANONICAL_DOMAIN), mcpExpectedServers: decodeDotenvQuotedValue(process.env.MCP_EXPECTED_SERVERS), + mcpExpectedTools: decodeDotenvQuotedValue(process.env.MCP_EXPECTED_TOOLS), lamBaseUrl: normalizeBaseUrl(process.env.LAM_BASE_URL || ""), ldapBindDn: decodeDotenvQuotedValue(process.env.LDAP_BIND_DN), ldapBindPassword: decodeDotenvQuotedValue(process.env.LDAP_BIND_PASSWORD), diff --git a/roles/web-app-openwebui/files/playwright/playwright.spec.js b/roles/web-app-openwebui/files/playwright/playwright.spec.js index 2378b5d9f6..047e193078 100644 --- a/roles/web-app-openwebui/files/playwright/playwright.spec.js +++ b/roles/web-app-openwebui/files/playwright/playwright.spec.js @@ -15,6 +15,7 @@ require("./test-biber-ldap-login").register(shared); require("./test-guest-persona").register(shared); require("./test-litellm-chat").register(shared); require("./test-mcp-servers").register(shared); +require("./test-mcp-bridge").register(shared); require("./test-mcp-non-member").register(shared); require("./test-mcp-grant-via-lam").register(shared); require("./test-seaweedfs"); diff --git a/roles/web-app-openwebui/files/playwright/test-mcp-bridge.js b/roles/web-app-openwebui/files/playwright/test-mcp-bridge.js new file mode 100644 index 0000000000..86431e42eb --- /dev/null +++ b/roles/web-app-openwebui/files/playwright/test-mcp-bridge.js @@ -0,0 +1,86 @@ +const { test, expect } = require("@playwright/test"); +const { resolveTimeout } = require("./timeouts"); +const { skipUnlessServiceEnabled } = require("./service-gating"); + +exports.register = function (shared) { + test("administrator: every registered MCP server answers Open WebUI's client", async ({ + page, + }) => { + skipUnlessServiceEnabled("mcp"); + + const expected = (shared.env.mcpExpectedServers || "") + .split(",") + .map((s) => s.trim()) + .filter(Boolean); + expect( + expected.length, + "the deploy must have discovered at least one MCP server when the mcp service is enabled" + ).toBeGreaterThan(0); + + test.setTimeout(resolveTimeout(60_000 + expected.length * 20_000)); + + await shared.signInViaDashboardOidc( + page, + shared.env.adminUsername, + shared.env.adminPassword, + "administrator" + ); + + const token = await page.evaluate(() => window.localStorage.getItem("token")); + expect(token, "OpenWebUI must store a session token after login").toBeTruthy(); + const headers = { Authorization: `Bearer ${token}` }; + const base = shared.env.openwebuiBaseUrl.replace(/\/+$/, ""); + + const readProbes = JSON.parse(shared.env.mcpExpectedTools || "{}"); + + const config = await page.request.get(`${base}/api/v1/configs/tool_servers`, { + headers, + }); + expect( + config.ok(), + `the admin tool-server config must answer the authenticated session (HTTP ${config.status()})` + ).toBeTruthy(); + const connections = (await config.json())?.TOOL_SERVER_CONNECTIONS ?? []; + + const verifiable = expected.filter((id) => { + const connection = connections.find( + (c) => c?.type === "mcp" && String(c?.info?.id ?? "") === id + ); + expect(connection, `${id} must be a configured MCP tool server`).toBeTruthy(); + return connection.auth_type === "bearer"; + }); + expect( + verifiable.length, + `none of ${JSON.stringify(expected)} authenticates with a deployment bearer; only a bearer connection carries its credential in the row, a system_oauth one resolves the caller's own upstream token and the administrator holds none, so there is nothing here this spec can verify against the bridge` + ).toBeGreaterThan(0); + + for (const id of verifiable) { + const connection = connections.find( + (c) => c?.type === "mcp" && String(c?.info?.id ?? "") === id + ); + + const verify = await page.request.post( + `${base}/api/v1/configs/tool_servers/verify`, + { headers, data: connection } + ); + expect( + verify.ok(), + `${id} did not answer Open WebUI's MCP client (HTTP ${verify.status()}); the endpoint is down, the deployed bearer is refused, or the transport disagrees` + ).toBeTruthy(); + + const specs = (await verify.json())?.specs ?? []; + expect( + specs.length, + `${id} completed the handshake but served no tool; a client would be configured against an empty surface` + ).toBeGreaterThan(0); + + const probe = readProbes[id]; + if (probe) { + expect( + specs.map((s) => String(s?.name ?? "")), + `${id} does not serve ${probe}, the read probe its meta/mcp.yml declares` + ).toContain(probe); + } + } + }); +}; diff --git a/roles/web-app-openwebui/templates/playwright.env.j2 b/roles/web-app-openwebui/templates/playwright.env.j2 index 6a42f51913..beae7ac7e2 100644 --- a/roles/web-app-openwebui/templates/playwright.env.j2 +++ b/roles/web-app-openwebui/templates/playwright.env.j2 @@ -40,6 +40,12 @@ MATOMO_SERVICE_ENABLED={{ lookup('config', application_id, 'services.matomo.enab MCP_EXPECTED_SERVERS={{ (OPENWEBUI_MCP_SERVERS | mcp_tool_server_connections | map(attribute='info.id') | list | join(',')) | dotenv_quote }} +{% set _read_probes = {} %} +{% for _server in OPENWEBUI_MCP_SERVERS | mcp_tool_server_connections %} +{% set _ = _read_probes.update({_server.info.id: lookup('config', _server.info.id, 'mcp.tools.read_probe.tool', '')}) %} +{% endfor %} +MCP_EXPECTED_TOOLS={{ _read_probes | to_json | dotenv_quote }} + MCP_SERVICE_ENABLED={{ lookup('config', application_id, 'mcp.enabled') | string | lower }} OIDC_ISSUER_URL={{ OIDC.CLIENT.ISSUER_URL | dotenv_quote }} diff --git a/tests/unit/python/roles/web-app-openwebui/templates/__init__.py b/tests/unit/python/roles/web-app-openwebui/templates/__init__.py new file mode 100644 index 0000000000..9f69c70b47 --- /dev/null +++ b/tests/unit/python/roles/web-app-openwebui/templates/__init__.py @@ -0,0 +1,3 @@ +from pathlib import Path + +PROJECT_ROOT: Path = Path(__file__).resolve().parents[6] diff --git a/tests/unit/python/roles/web-app-openwebui/templates/test_playwright_read_probes.py b/tests/unit/python/roles/web-app-openwebui/templates/test_playwright_read_probes.py new file mode 100644 index 0000000000..d867b092f9 --- /dev/null +++ b/tests/unit/python/roles/web-app-openwebui/templates/test_playwright_read_probes.py @@ -0,0 +1,95 @@ +"""``MCP_EXPECTED_TOOLS`` names the read probe each registered server declares. + +The bridge spec asserts that a server serves the tool its ``meta/mcp.yml`` +declares, and it learns which tool that is from this one env value. Rendered +wrong it does not fail: an empty map makes the spec skip the tool assertion for +every server and pass on the handshake alone, which is the weaker check it was +written to replace. + +The block is extracted from the template rather than restated here, so a change +to the template is a change to what this test renders. +""" + +from __future__ import annotations + +import json +import re +import unittest + +from ansible.parsing.dataloader import DataLoader +from ansible.template import Templar, trust_as_template + +from utils.cache.applications import get_application_defaults +from utils.cache.files import PROJECT_ROOT, read_text + +_TEMPLATE = PROJECT_ROOT / "roles/web-app-openwebui/templates/playwright.env.j2" +_BLOCK = re.compile( + r"\{%\s*set _read_probes.*?^MCP_EXPECTED_TOOLS=.*?$", + re.DOTALL | re.MULTILINE, +) +_SERVERS = ("web-app-gitea", "svc-db-qdrant", "web-svc-libretranslate") + + +def _render(server_ids: tuple[str, ...]) -> str: + """Render the template's own read-probe block for the given servers.""" + block = _BLOCK.search(read_text(str(_TEMPLATE))) + assert block, f"{_TEMPLATE} no longer carries the read-probe block" + variables = { + "applications": get_application_defaults(roles_dir=PROJECT_ROOT / "roles"), + "OPENWEBUI_MCP_SERVERS": [ + { + "id": server_id, + "url": f"http://{server_id}:8080/mcp", + "auth": "bearer_token", + "token": "irrelevant-to-this-test", + } + for server_id in server_ids + ], + } + templar = Templar(loader=DataLoader(), variables=variables) + return templar.template(trust_as_template(block.group(0))) + + +def _rendered_map(server_ids: tuple[str, ...]) -> dict: + """Return the map a spec reads, decoded the way the spec decodes it. + + ``dotenv_quote`` emits a quoted string with escaped inner quotes, which + ``decodeDotenvQuotedValue`` (personas/utils/dotenv.js:17-30) unwraps with a + JSON parse before the spec parses the payload itself. Decoding it once here + would compare against a string that never reaches a spec. + """ + line = _render(server_ids) + _, _, value = line.partition("=") + value = value.strip() + if value.startswith('"') and value.endswith('"'): + value = json.loads(value) + return json.loads(value) + + +class TestPlaywrightReadProbes(unittest.TestCase): + def test_every_server_gets_the_tool_its_role_declares(self) -> None: + rendered = _rendered_map(_SERVERS) + expected = { + server: ( + get_application_defaults(roles_dir=PROJECT_ROOT / "roles")[server] + .get("mcp", {}) + .get("tools", {}) + .get("read_probe", {}) + .get("tool", "") + ) + for server in _SERVERS + } + self.assertEqual(expected, rendered) + + def test_the_probes_are_non_empty_for_these_servers(self) -> None: + self.assertEqual([], [k for k, v in _rendered_map(_SERVERS).items() if not v]) + + def test_a_server_without_a_read_probe_renders_an_empty_string(self) -> None: + self.assertEqual({"web-app-pretix": ""}, _rendered_map(("web-app-pretix",))) + + def test_no_discovered_server_renders_an_empty_map(self) -> None: + self.assertEqual({}, _rendered_map(())) + + +if __name__ == "__main__": + unittest.main()