Reject revokes the node's MQTT login and API key (server-26#195)

reject_node only set approval_status, so an approved-then-rejected node kept its dynsec login, its /upload key and /systems read access. - reject_node: status -> rejected first (fail-closed), then dynsec.delete_node_client, then delete node_keys. dynsec failure -> 502, key kept, retry is idempotent. Re-approve mints a fresh key. - upload.py: valid key alone is not enough; node must be approved (403), uncached read so revocation bites immediately. - dynsec.reconcile_all: deletes rejected nodes' logins on startup, healing nodes rejected before this change or whose revoke failed. - Reject confirm text says it revokes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-04 17:58:51 -04:00
parent d57738a8f6
commit 73147803ea
5 changed files with 262 additions and 7 deletions
@@ -0,0 +1,203 @@
+"""
+server-26#195 — reject must revoke: delete the node's dynsec (MQTT) login and
+its node_keys entry, /upload must refuse a node that is not approved, and
+dynsec.reconcile_all() must delete rejected nodes' logins on startup.
+"""
+import asyncio
+from unittest.mock import AsyncMock, MagicMock, patch
+
+from fastapi.testclient import TestClient
+
+from app.main import app
+from app.internal import dynsec
+from app.internal.auth import require_admin_token, require_service_or_firebase_token
+from app.routers import nodes, upload
+
+client = TestClient(app)
+
+
+def setup_function():
+    app.dependency_overrides[require_admin_token] = lambda: {"admin": True}
+    app.dependency_overrides[require_service_or_firebase_token] = lambda: {"admin": True}
+
+
+def teardown_function():
+    app.dependency_overrides.pop(require_admin_token, None)
+    app.dependency_overrides.pop(require_service_or_firebase_token, None)
+
+
+def _fake_store(node, key_doc):
+    store = {"nodes": node, "node_keys": key_doc}
+    calls = []
+
+    async def doc_get(c, i):
+        return store.get(c)
+
+    async def doc_update(c, i, data):
+        calls.append(("doc_update", c, data))
+        store[c] = {**(store[c] or {}), **data}
+
+    async def doc_delete(c, i):
+        calls.append(("doc_delete", c))
+        store[c] = None
+
+    async def doc_set(c, i, data, merge=False):
+        store[c] = data
+
+    return store, calls, doc_get, doc_update, doc_delete, doc_set
+
+
+# ---------------------------------------------------------------------------
+# reject_node
+# ---------------------------------------------------------------------------
+
+def test_reject_revokes_mqtt_login_and_key_in_fail_closed_order():
+    store, calls, doc_get, doc_update, doc_delete, _ = _fake_store(
+        {"node_id": "n1", "approval_status": "approved"}, {"node_id": "n1", "api_key": "live-key"})
+
+    async def delete_client(node_id):
+        calls.append(("dynsec_delete", node_id))
+
+    with patch.object(nodes.fstore, "doc_get", doc_get), \
+         patch.object(nodes.fstore, "doc_update", doc_update), \
+         patch.object(nodes.fstore, "doc_delete", doc_delete), \
+         patch.object(nodes.dynsec, "delete_node_client", delete_client):
+        resp = client.post("/nodes/n1/reject")
+
+    assert resp.status_code == 200
+    assert resp.json() == {"ok": True, "revoked": True}
+    assert calls == [
+        ("doc_update", "nodes", {"approval_status": "rejected"}),
+        ("dynsec_delete", "n1"),
+        ("doc_delete", "node_keys"),
+    ]
+    assert store["node_keys"] is None
+    assert store["nodes"]["approval_status"] == "rejected"
+
+
+def test_reject_dynsec_failure_is_502_and_keeps_key_but_status_is_already_rejected():
+    store, calls, doc_get, doc_update, doc_delete, _ = _fake_store(
+        {"node_id": "n1", "approval_status": "approved"}, {"node_id": "n1", "api_key": "live-key"})
+
+    with patch.object(nodes.fstore, "doc_get", doc_get), \
+         patch.object(nodes.fstore, "doc_update", doc_update), \
+         patch.object(nodes.fstore, "doc_delete", doc_delete), \
+         patch.object(nodes.dynsec, "delete_node_client",
+                      AsyncMock(side_effect=dynsec.DynsecError("timeout"))):
+        resp = client.post("/nodes/n1/reject")
+
+    assert resp.status_code == 502
+    assert "Retry reject" in resp.json()["detail"]
+    assert store["nodes"]["approval_status"] == "rejected"
+    assert store["node_keys"] == {"node_id": "n1", "api_key": "live-key"}
+    assert ("doc_delete", "node_keys") not in calls
+
+
+def test_reject_unknown_node_is_404_and_touches_nothing():
+    with patch.object(nodes.fstore, "doc_get", AsyncMock(return_value=None)), \
+         patch.object(nodes.fstore, "doc_update", AsyncMock()) as doc_update, \
+         patch.object(nodes.fstore, "doc_delete", AsyncMock()) as doc_delete, \
+         patch.object(nodes.dynsec, "delete_node_client", AsyncMock()) as delete_client:
+        resp = client.post("/nodes/ghost/reject")
+    assert resp.status_code == 404
+    doc_update.assert_not_awaited()
+    doc_delete.assert_not_awaited()
+    delete_client.assert_not_awaited()
+
+
+def test_reject_requires_admin():
+    app.dependency_overrides.pop(require_admin_token, None)
+    resp = client.post("/nodes/n1/reject")
+    assert resp.status_code in (401, 403)
+
+
+def test_reapprove_after_reject_mints_a_new_key_never_the_revoked_one():
+    store, _, doc_get, doc_update, doc_delete, doc_set = _fake_store(
+        {"node_id": "n1", "approval_status": "approved"}, {"node_id": "n1", "api_key": "old-key"})
+
+    with patch.object(nodes.fstore, "doc_get", doc_get), \
+         patch.object(nodes.fstore, "doc_update", doc_update), \
+         patch.object(nodes.fstore, "doc_delete", doc_delete), \
+         patch.object(nodes.fstore, "doc_set", doc_set), \
+         patch.object(nodes.dynsec, "delete_node_client", AsyncMock()), \
+         patch.object(nodes.dynsec, "upsert_node_client", AsyncMock()) as upsert, \
+         patch.object(nodes.mqtt_handler, "publish_node_key", MagicMock()):
+        assert client.post("/nodes/n1/reject").status_code == 200
+        assert client.post("/nodes/n1/approve").status_code == 200
+
+    new_key = store["node_keys"]["api_key"]
+    assert new_key != "old-key" and len(new_key) == 64
+    upsert.assert_awaited_once_with("n1", new_key)
+
+
+# ---------------------------------------------------------------------------
+# /upload approval gate
+# ---------------------------------------------------------------------------
+
+def _upload(node_doc, key="live-key"):
+    store = {"node_keys": {"node_id": "n1", "api_key": "live-key"}, "nodes": node_doc}
+
+    async def doc_get(c, i):
+        return store.get(c)
+
+    with patch.object(upload.fstore, "doc_get", doc_get), \
+         patch.object(upload, "upload_audio", AsyncMock(return_value=None)) as up:
+        resp = client.post(
+            "/upload",
+            data={"call_id": "c1", "node_id": "n1"},
+            files={"file": ("a.mp3", b"\x00\x01", "audio/mpeg")},
+            headers={"Authorization": f"Bearer {key}"},
+        )
+    return resp, up
+
+
+def test_upload_refuses_valid_key_from_rejected_node():
+    resp, up = _upload({"node_id": "n1", "approval_status": "rejected"})
+    assert resp.status_code == 403
+    up.assert_not_awaited()
+
+
+def test_upload_refuses_valid_key_from_pending_or_missing_node():
+    for node_doc in ({"node_id": "n1", "approval_status": "pending"}, {"node_id": "n1"}, None):
+        resp, up = _upload(node_doc)
+        assert resp.status_code == 403, node_doc
+        up.assert_not_awaited()
+
+
+def test_upload_accepts_approved_node():
+    resp, up = _upload({"node_id": "n1", "approval_status": "approved"})
+    assert resp.status_code == 200
+    up.assert_awaited_once()
+
+
+def test_upload_wrong_key_still_401_before_approval_check():
+    resp, up = _upload({"node_id": "n1", "approval_status": "approved"}, key="wrong")
+    assert resp.status_code == 401
+    up.assert_not_awaited()
+
+
+# ---------------------------------------------------------------------------
+# dynsec.reconcile_all
+# ---------------------------------------------------------------------------
+
+def test_reconcile_deletes_rejected_logins_and_still_syncs_approved():
+    async def collection_list(c, **filters):
+        status = filters.get("approval_status")
+        return {
+            "rejected": [{"node_id": "bad1"}, {"node_id": "bad2"}],
+            "approved": [{"node_id": "good"}],
+        }[status]
+
+    async def doc_get(c, i):
+        return {"node_id": i, "api_key": "k-" + i}
+
+    delete_client = AsyncMock(side_effect=[dynsec.DynsecError("boom"), None])
+    with patch.object(dynsec.fstore, "collection_list", collection_list), \
+         patch.object(dynsec.fstore, "doc_get", doc_get), \
+         patch.object(dynsec, "delete_node_client", delete_client), \
+         patch.object(dynsec, "upsert_node_client", AsyncMock()) as upsert:
+        asyncio.run(dynsec.reconcile_all())
+
+    # one failed revoke does not stop the rest, nor the approved sync
+    assert [c.args[0] for c in delete_client.await_args_list] == ["bad1", "bad2"]
+    upsert.assert_awaited_once_with("good", "k-good")