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()