From e4078c05fda67f91db547ca5c40fb11fc04dbfd2 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Thu, 8 Oct 2026 19:38:44 +0200 Subject: [PATCH] fix(web-app-n8n): provision the MCP surfaces against the n8n 2 API n8n 2 pages GET /rest/api-keys as {data: {items, counts, totals, owners}}, ten keys at a time, and matches `label` as a substring, so walking the body raised "'str' object has no attribute 'get'". It also enforces API key scopes, so activating the MCP server workflow needs `workflow:activate`. Both scripts now read every list and create response through one normalisation that unwraps the page, look the managed key up by label, and name an unexpected shape by its keys only. The normalisation is taken over from a392e2dd49 on feature/store-i18n. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../files/python/provision/client.py | 71 ++++++++++++++--- .../web-app-n8n/files/python/provision/mcp.py | 76 ++++++++++++++++--- .../files/provision/test_client.py | 30 +++++++- .../web-app-n8n/files/provision/test_mcp.py | 54 ++++++++++++- 4 files changed, 207 insertions(+), 24 deletions(-) diff --git a/roles/web-app-n8n/files/python/provision/client.py b/roles/web-app-n8n/files/python/provision/client.py index e1c6241d3d..1eec995767 100644 --- a/roles/web-app-n8n/files/python/provision/client.py +++ b/roles/web-app-n8n/files/python/provision/client.py @@ -25,6 +25,7 @@ import os import re import sys import urllib.error +import urllib.parse import urllib.request BASE = os.environ.get("N8N_BASE", "").rstrip("/") @@ -90,6 +91,52 @@ def call(path, method="GET", payload=None, api_key=None): return error.code, error.read().decode(errors="replace") +def shape(value): + """Describe a value by its type and, for a mapping, its keys, never its content. + + Args: + value: decoded response fragment. + """ + if isinstance(value, dict): + return f"a mapping with keys {sorted(map(str, value))}" + if isinstance(value, list): + return f"a list of {sorted({type(item).__name__ for item in value})}" + return f"a {type(value).__name__}" + + +def records(body, what): + """Return the records of a list response. + + Args: + body: decoded body: ``{"data": [...]}``, a bare list, or the paged + ``{"data": {"items": [...], ...}}`` of n8n 2. + what: noun naming the collection, for the failure message. + """ + listed = body.get("data", body) if isinstance(body, dict) else body + if isinstance(listed, dict) and "items" in listed: + listed = listed["items"] + if listed is None: + return [] + if not isinstance(listed, list) or not all( + isinstance(item, dict) for item in listed + ): + sys.exit(f"FAILED listing {what}: expected records, got {shape(listed)}") + return listed + + +def record(body, what): + """Return the single record of a create response. + + Args: + body: decoded body, either ``{"data": {...}}`` or a bare mapping. + what: noun naming the record, for the failure message. + """ + created = body.get("data", body) if isinstance(body, dict) else body + if not isinstance(created, dict): + sys.exit(f"FAILED creating {what}: expected a record, got {shape(created)}") + return created + + def login(): status, body = call( f"{REST}/login", @@ -106,14 +153,18 @@ def api_key(): n8n returns the usable secret only once, as ``rawApiKey``, and a listed key is redacted, so the key cannot be carried between runs. It is minted here and revoked before the run ends; an entry left behind under the managed name - belongs to a run that died and is deleted rather than guessed at. + belongs to a run that died and is deleted rather than guessed at. n8n pages + the listing at ten keys and matches ``label`` as a substring, so the query + narrows to the managed name and the exact match happens here. """ - status, body = call(f"{REST}/api-keys") + query = urllib.parse.urlencode({"label": KEY_NAME}) + status, body = call(f"{REST}/api-keys?{query}") if status != 200: sys.exit(f"FAILED listing api keys: {status} {body}") - existing = (body or {}).get("data") if isinstance(body, dict) else body - matches = [key for key in existing or [] if key.get("label") == KEY_NAME] + matches = [ + key for key in records(body, "api keys") if key.get("label") == KEY_NAME + ] if len(matches) > 1: sys.exit(f"FAILED: {len(matches)} api keys named {KEY_NAME}") for key in matches: @@ -136,7 +187,7 @@ def api_key(): ) if status not in (200, 201): sys.exit(f"FAILED creating api key {KEY_NAME}: {status} {body}") - created = (body or {}).get("data") if isinstance(body, dict) else body + created = record(body, f"api key {KEY_NAME}") return str(created["rawApiKey"]), str(created["id"]) @@ -174,8 +225,7 @@ def listed_credentials(): status, body = call(f"{REST}/credentials") if status != 200: sys.exit(f"FAILED listing credentials: {status} {body}") - existing = (body or {}).get("data") if isinstance(body, dict) else body - return list(existing or []) + return records(body, "credentials") def credential(provider): @@ -198,7 +248,7 @@ def credential(provider): status, body = call(f"{REST}/credentials", method="POST", payload=payload) if status not in (200, 201): sys.exit(f"FAILED creating {name}: {status} {body}") - created = (body or {}).get("data") if isinstance(body, dict) else body + created = record(body, name) return {"id": str(created["id"]), "name": name}, True status, body = call( @@ -317,8 +367,9 @@ def managed_workflow(key): status, body = call(f"{PUBLIC}/workflows", api_key=key) if status != 200: sys.exit(f"FAILED listing workflows: {status} {body}") - existing = (body or {}).get("data") if isinstance(body, dict) else body - matches = [flow for flow in existing or [] if flow.get("name") == WORKFLOW_NAME] + matches = [ + flow for flow in records(body, "workflows") if flow.get("name") == WORKFLOW_NAME + ] if len(matches) > 1: sys.exit(f"FAILED: {len(matches)} workflows named {WORKFLOW_NAME}") return matches[0] if matches else None diff --git a/roles/web-app-n8n/files/python/provision/mcp.py b/roles/web-app-n8n/files/python/provision/mcp.py index e54f5f5d39..007927a301 100644 --- a/roles/web-app-n8n/files/python/provision/mcp.py +++ b/roles/web-app-n8n/files/python/provision/mcp.py @@ -19,6 +19,7 @@ import json import os import sys import urllib.error +import urllib.parse import urllib.request BASE = os.environ.get("N8N_BASE", "").rstrip("/") @@ -95,6 +96,52 @@ def call(path, method="GET", payload=None, api_key=None): return error.code, error.read().decode(errors="replace") +def shape(value): + """Describe a value by its type and, for a mapping, its keys, never its content. + + Args: + value: decoded response fragment. + """ + if isinstance(value, dict): + return f"a mapping with keys {sorted(map(str, value))}" + if isinstance(value, list): + return f"a list of {sorted({type(item).__name__ for item in value})}" + return f"a {type(value).__name__}" + + +def records(body, what): + """Return the records of a list response. + + Args: + body: decoded body: ``{"data": [...]}``, a bare list, or the paged + ``{"data": {"items": [...], ...}}`` of n8n 2. + what: noun naming the collection, for the failure message. + """ + listed = body.get("data", body) if isinstance(body, dict) else body + if isinstance(listed, dict) and "items" in listed: + listed = listed["items"] + if listed is None: + return [] + if not isinstance(listed, list) or not all( + isinstance(item, dict) for item in listed + ): + sys.exit(f"FAILED listing {what}: expected records, got {shape(listed)}") + return listed + + +def record(body, what): + """Return the single record of a create response. + + Args: + body: decoded body, either ``{"data": {...}}`` or a bare mapping. + what: noun naming the record, for the failure message. + """ + created = body.get("data", body) if isinstance(body, dict) else body + if not isinstance(created, dict): + sys.exit(f"FAILED creating {what}: expected a record, got {shape(created)}") + return created + + def login(): status, body = call( f"{REST}/login", @@ -111,14 +158,18 @@ def api_key(): n8n returns the usable secret only once, as ``rawApiKey``, and a listed key is redacted, so the key cannot be carried between runs. It is minted here and revoked before the run ends; an entry left behind under the managed name - belongs to a run that died and is deleted rather than guessed at. + belongs to a run that died and is deleted rather than guessed at. n8n pages + the listing at ten keys and matches ``label`` as a substring, so the query + narrows to the managed name and the exact match happens here. """ - status, body = call(f"{REST}/api-keys") + query = urllib.parse.urlencode({"label": KEY_NAME}) + status, body = call(f"{REST}/api-keys?{query}") if status != 200: sys.exit(f"FAILED listing api keys: {status} {body}") - existing = (body or {}).get("data") if isinstance(body, dict) else body - matches = [key for key in existing or [] if key.get("label") == KEY_NAME] + matches = [ + key for key in records(body, "api keys") if key.get("label") == KEY_NAME + ] if len(matches) > 1: sys.exit(f"FAILED: {len(matches)} api keys named {KEY_NAME}") for key in matches: @@ -134,13 +185,14 @@ def api_key(): "workflow:read", "workflow:update", "workflow:list", + "workflow:activate", ], "expiresAt": None, }, ) if status not in (200, 201): sys.exit(f"FAILED creating api key {KEY_NAME}: {status} {body}") - created = (body or {}).get("data") if isinstance(body, dict) else body + created = record(body, f"api key {KEY_NAME}") return str(created["rawApiKey"]), str(created["id"]) @@ -166,8 +218,11 @@ def bearer_credential(): if status != 200: sys.exit(f"FAILED listing credentials: {status} {body}") - existing = (body or {}).get("data") if isinstance(body, dict) else body - matches = [item for item in existing or [] if item.get("name") == CREDENTIAL_NAME] + matches = [ + item + for item in records(body, "credentials") + if item.get("name") == CREDENTIAL_NAME + ] if len(matches) > 1: sys.exit(f"FAILED: {len(matches)} credentials named {CREDENTIAL_NAME}") @@ -180,7 +235,7 @@ def bearer_credential(): status, body = call(f"{REST}/credentials", method="POST", payload=payload) if status not in (200, 201): sys.exit(f"FAILED creating {CREDENTIAL_NAME}: {status} {body}") - created = (body or {}).get("data") if isinstance(body, dict) else body + created = record(body, CREDENTIAL_NAME) return {"id": str(created["id"]), "name": CREDENTIAL_NAME}, True status, body = call( @@ -268,8 +323,9 @@ def upsert_workflow(key, credential): if status != 200: sys.exit(f"FAILED listing workflows: {status} {body}") - existing = (body or {}).get("data") if isinstance(body, dict) else body - matches = [flow for flow in existing or [] if flow.get("name") == WORKFLOW_NAME] + matches = [ + flow for flow in records(body, "workflows") if flow.get("name") == WORKFLOW_NAME + ] if len(matches) > 1: sys.exit(f"FAILED: {len(matches)} workflows named {WORKFLOW_NAME}") diff --git a/tests/unit/python/roles/web-app-n8n/files/provision/test_client.py b/tests/unit/python/roles/web-app-n8n/files/provision/test_client.py index d698b892dc..261a6b5080 100644 --- a/tests/unit/python/roles/web-app-n8n/files/provision/test_client.py +++ b/tests/unit/python/roles/web-app-n8n/files/provision/test_client.py @@ -8,10 +8,12 @@ import io import json import unittest from unittest.mock import patch +from urllib.parse import parse_qs, urlsplit from . import PROJECT_ROOT SCRIPT_PATH = PROJECT_ROOT / "roles/web-app-n8n/files/python/provision/client.py" +PAGE_SIZE = 10 PROVIDER = { "id": "web-app-gitea", @@ -78,8 +80,10 @@ class FakeApi: self.deleted_credentials.append(dead) self.credentials = [c for c in self.credentials if c["id"] != dead] return 200, {"success": True} - if path == "/rest/api-keys" and method == "GET": - return 200, {"data": [dict(key) for key in self.keys]} + if path.startswith("/rest/api-keys?") and method == "GET": + label = parse_qs(urlsplit(path).query)["label"][0].lower() + items = [dict(key) for key in self.keys if label in key["label"].lower()] + return 200, {"data": {"items": items[:PAGE_SIZE]}} if path == "/rest/api-keys" and method == "POST": self.keys = [{"id": "k1", "label": payload["label"]}] return 200, {"data": {"id": "k1", "rawApiKey": "n8n-raw-key"}} @@ -179,6 +183,28 @@ class TestProvisionClient(unittest.TestCase): module.main() self.assertEqual(["k1"], api.deleted_keys) + def test_a_stale_managed_key_behind_a_full_page_is_still_replaced(self) -> None: + module = load_script() + humans = [{"id": f"h{n}", "label": f"human {n}"} for n in range(PAGE_SIZE)] + api = FakeApi(keys=[*humans, {"id": "old", "label": "infinito:mcp"}]) + with patch.object(module, "call", api): + module.main() + self.assertEqual(["old", "k1"], api.deleted_keys) + + def test_an_unknown_listing_shape_names_its_keys_but_never_a_value(self) -> None: + module = load_script() + api = FakeApi() + + def odd(path, method="GET", payload=None, api_key=None): + if path == "/rest/credentials" and method == "GET": + return 200, {"data": {"token": "t0ps3cret"}} + return api(path, method, payload, api_key) + + with patch.object(module, "call", odd), self.assertRaises(SystemExit) as exit_: + module.main() + self.assertIn("['token']", str(exit_.exception)) + self.assertNotIn("t0ps3cret", str(exit_.exception)) + def test_a_rotated_provider_token_replaces_the_stored_one(self) -> None: module = load_script() api = FakeApi(credentials=[dict(REFERENCE)]) diff --git a/tests/unit/python/roles/web-app-n8n/files/provision/test_mcp.py b/tests/unit/python/roles/web-app-n8n/files/provision/test_mcp.py index 81dcecf03a..ae5365a107 100644 --- a/tests/unit/python/roles/web-app-n8n/files/provision/test_mcp.py +++ b/tests/unit/python/roles/web-app-n8n/files/provision/test_mcp.py @@ -8,10 +8,12 @@ import io import json import unittest from unittest.mock import patch +from urllib.parse import parse_qs, urlsplit from . import PROJECT_ROOT SCRIPT_PATH = PROJECT_ROOT / "roles/web-app-n8n/files/python/provision/mcp.py" +PAGE_SIZE = 10 ENV = { "N8N_BASE": "http://n8n:5678", @@ -51,6 +53,7 @@ class FakeApi: self.deleted_keys = [] self.logins = 0 self.minted = 0 + self.scopes = set() def __call__(self, path, method="GET", payload=None, api_key=None): if path == "/rest/login": @@ -72,10 +75,13 @@ class FakeApi: self.deleted_credentials.append(dead) self.credentials = [c for c in self.credentials if c["id"] != dead] return 200, {"success": True} - if path == "/rest/api-keys" and method == "GET": - return 200, {"data": [dict(key) for key in self.keys]} + if path.startswith("/rest/api-keys?") and method == "GET": + label = parse_qs(urlsplit(path).query)["label"][0].lower() + items = [dict(key) for key in self.keys if label in key["label"].lower()] + return 200, {"data": {"items": items[:PAGE_SIZE]}} if path == "/rest/api-keys" and method == "POST": self.keys = [{"id": "k1", "label": payload["label"]}] + self.scopes = set(payload["scopes"]) return 200, {"data": {"id": "k1", "rawApiKey": "n8n-raw-key"}} if path.startswith("/rest/api-keys/") and method == "DELETE": self.deleted_keys.append(path.rsplit("/", 1)[-1]) @@ -89,6 +95,8 @@ class FakeApi: self.updated_workflows.append(payload) return 200, payload if path.endswith("/activate") and method == "POST": + if "workflow:activate" not in self.scopes: + return 403, '{"message":"Forbidden"}' self.activated_workflows.append(path.split("/")[-2]) return 200, {"active": True} if path.startswith("/api/v1/workflows/") and method == "DELETE": @@ -184,6 +192,48 @@ class TestProvisionMcp(unittest.TestCase): module.main() self.assertEqual(["old", "k1"], api.deleted_keys) + def test_a_stale_managed_key_behind_a_full_page_is_still_replaced(self) -> None: + module = load_script() + humans = [{"id": f"h{n}", "label": f"human {n}"} for n in range(PAGE_SIZE)] + api = FakeApi(keys=[*humans, {"id": "old", "label": "infinito:mcp"}]) + with patch.object(module, "call", api): + module.main() + self.assertEqual(["old", "k1"], api.deleted_keys) + + def test_an_unknown_listing_shape_names_its_keys_but_never_a_value(self) -> None: + module = load_script() + api = FakeApi() + + def odd(path, method="GET", payload=None, api_key=None): + if path.startswith("/rest/api-keys?"): + return 200, {"data": {"secret": "t0ps3cret"}} + return api(path, method, payload, api_key) + + with patch.object(module, "call", odd), self.assertRaises(SystemExit) as exit_: + module.main() + self.assertIn("['secret']", str(exit_.exception)) + self.assertNotIn("t0ps3cret", str(exit_.exception)) + + def test_an_unpaged_listing_of_n8n_1_still_finds_the_stale_key(self) -> None: + module = load_script() + api = FakeApi() + + def unpaged(path, method="GET", payload=None, api_key=None): + if path.startswith("/rest/api-keys?"): + return 200, {"data": [{"id": "old", "label": "infinito:mcp"}]} + return api(path, method, payload, api_key) + + with patch.object(module, "call", unpaged): + module.main() + self.assertEqual(["old", "k1"], api.deleted_keys) + + def test_a_key_that_only_contains_the_managed_name_is_left_alone(self) -> None: + module = load_script() + api = FakeApi(keys=[{"id": "near", "label": "infinito:mcp-legacy"}]) + with patch.object(module, "call", api): + module.main() + self.assertEqual(["k1"], api.deleted_keys) + def test_the_run_leaves_no_api_key_behind(self) -> None: module = load_script() api = FakeApi()