From 0bfe95320b62a6bdbe23234cc963c5bc69e184d2 Mon Sep 17 00:00:00 2001 From: Dmitrii Iurco Date: Fri, 29 May 2026 08:53:44 -0400 Subject: [PATCH] =?UTF-8?q?feat:=20Phase=202=20=E2=80=94=20remove=20builti?= =?UTF-8?q?ns=20layer,=20ServiceRegistry=20is=20installed-only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builtins (email/calendar/files) are no longer baked into the API image. ServiceRegistry now only knows about installed store services. When nothing is installed, Caddy and DNS get no service routes — no hardcoded fallback. Changes: - service_registry.py: remove _BUILTINS_DIR, _builtin_ids, _builtin_manifest, _load_manifest; get() and list_all() now delegate entirely to installed services - caddy_manager.py: remove _build_core_service_routes(); remove hardcoded fallback pairs from _http01_service_pairs(); empty registry → api block only - network_manager.py: _get_service_subdomains() returns [] when no registry - api/services/builtins/: deleted (email, calendar, files manifests) - Tests updated throughout: removed builtin-dependent assertions, added installed-service fixtures, updated fallback expectations to api-only Co-Authored-By: Claude Sonnet 4.6 --- api/caddy_manager.py | 43 +- api/network_manager.py | 2 +- api/service_registry.py | 71 +--- api/services/builtins/calendar/manifest.json | 68 --- api/services/builtins/email/manifest.json | 99 ----- api/services/builtins/files/manifest.json | 79 ---- tests/test_caddy_manager.py | 55 ++- tests/test_caddy_registry_integration.py | 72 ++-- tests/test_network_manager.py | 22 +- tests/test_optional_services_feature.py | 199 +++------ tests/test_peer_dashboard_services.py | 64 +-- tests/test_service_registry.py | 411 +++++++++++-------- 12 files changed, 419 insertions(+), 766 deletions(-) delete mode 100644 api/services/builtins/calendar/manifest.json delete mode 100644 api/services/builtins/email/manifest.json delete mode 100644 api/services/builtins/files/manifest.json diff --git a/api/caddy_manager.py b/api/caddy_manager.py index ab9279c..839cea4 100644 --- a/api/caddy_manager.py +++ b/api/caddy_manager.py @@ -163,38 +163,12 @@ class CaddyManager(BaseServiceManager): lines.append("}") return "\n".join(lines) - @staticmethod - def _build_core_service_routes(domain: str) -> str: - """Return 4-space-indented named-matcher + handle blocks for core services.""" - return ( - f" @calendar host calendar.{domain}\n" - f" handle @calendar {{\n" - f" reverse_proxy cell-radicale:5232\n" - f" }}\n" - f" @mail host mail.{domain} webmail.{domain}\n" - f" handle @mail {{\n" - f" reverse_proxy cell-rainloop:8888\n" - f" }}\n" - f" @files host files.{domain}\n" - f" handle @files {{\n" - f" reverse_proxy cell-filegator:8080\n" - f" }}\n" - f" @webdav host webdav.{domain}\n" - f" handle @webdav {{\n" - f" reverse_proxy cell-webdav:80\n" - f" }}\n" - f" @api host api.{domain}\n" - f" handle @api {{\n" - f" reverse_proxy cell-api:3000\n" - f" }}" - ) - def _build_registry_service_routes(self, domain: str) -> str: """Build named-matcher + handle blocks from the service registry. - Falls back to the hardcoded ``_build_core_service_routes`` when no - registry is wired or the registry returns nothing, so the method is - always safe to call even in tests that don't supply a registry. + When no registry is wired or the registry returns nothing, only the + api block is emitted (api is always infrastructure, not delegated to + the registry). """ routes: List[Dict] = [] if self._service_registry is not None: @@ -203,9 +177,6 @@ class CaddyManager(BaseServiceManager): except Exception as exc: logger.warning('_build_registry_service_routes: registry error: %s', exc) - if not routes: - return self._build_core_service_routes(domain) - # Pre-seed with reserved names so no registry entry can squat them. seen_matchers: set = {'api', 'webui'} @@ -403,14 +374,6 @@ class CaddyManager(BaseServiceManager): except Exception as exc: logger.warning('_http01_service_pairs: registry error: %s', exc) pairs = [] - if not pairs: - pairs = [ - ('calendar', 'cell-radicale:5232'), - ('mail', 'cell-rainloop:8888'), - ('webmail', 'cell-rainloop:8888'), - ('files', 'cell-filegator:8080'), - ('webdav', 'cell-webdav:80'), - ] pairs.append(('api', 'cell-api:3000')) return pairs diff --git a/api/network_manager.py b/api/network_manager.py index bb1ce49..69d49ff 100644 --- a/api/network_manager.py +++ b/api/network_manager.py @@ -268,7 +268,7 @@ class NetworkManager(BaseServiceManager): return subs except Exception as exc: logger.warning('_get_service_subdomains: registry error: %s', exc) - return ['calendar', 'files', 'mail', 'webmail', 'webdav'] + return [] def _build_dns_records(self, cell_name: str, ip_range: str) -> List[Dict]: """Build the standard set of DNS A records. diff --git a/api/service_registry.py b/api/service_registry.py index e4cd9a8..242304c 100644 --- a/api/service_registry.py +++ b/api/service_registry.py @@ -1,28 +1,21 @@ """ ServiceRegistry — single source of truth for all PIC services. -Merges three layers: +Merges two layers: 1. Manifest defaults (config_schema.*.default) 2. Admin-saved config from ConfigManager (cell_config.json) - 3. Runtime state from installed store records All consumers (CaddyManager, backup, peer services endpoint) read from here rather than hardcoding service names or subdomains. """ -import json import logging -import os import re from typing import Dict, List, Optional from urllib.parse import quote as _urlquote logger = logging.getLogger('picell') -# Built-ins are baked into the container image at build time. -# Do not bind-mount this path read-write in docker-compose. -_BUILTINS_DIR = os.path.join(os.path.dirname(__file__), 'services', 'builtins') - _SUBDOMAIN_RE = re.compile(r'^[a-z][a-z0-9-]{0,30}$') _BACKEND_RE = re.compile(r'^[A-Za-z0-9._-]+:\d{1,5}$') _RESERVED_SUBS = frozenset({'api', 'webui', 'admin', 'www', 'ns1', 'ns2', 'git', 'registry', 'install'}) @@ -33,29 +26,6 @@ class ServiceRegistry: def __init__(self, config_manager): self._cm = config_manager - # ── Manifest loading ────────────────────────────────────────────────── - - def _load_manifest(self, path: str) -> Optional[Dict]: - try: - with open(path) as f: - return json.load(f) - except Exception as e: - logger.warning('ServiceRegistry: failed to load manifest %s: %s', path, e) - return None - - def _builtin_ids(self) -> List[str]: - if not os.path.isdir(_BUILTINS_DIR): - return [] - return sorted( - d for d in os.listdir(_BUILTINS_DIR) - if os.path.isfile(os.path.join(_BUILTINS_DIR, d, 'manifest.json')) - ) - - def _builtin_manifest(self, service_id: str) -> Optional[Dict]: - return self._load_manifest( - os.path.join(_BUILTINS_DIR, service_id, 'manifest.json') - ) - # ── Config merging ──────────────────────────────────────────────────── _TYPE_COERCIONS = {'integer': int, 'string': str, 'boolean': bool} @@ -83,22 +53,16 @@ class ServiceRegistry: def get(self, service_id: str) -> Optional[Dict]: """Return manifest + merged config for one service, or None if unknown.""" - manifest = self._builtin_manifest(service_id) - if manifest is None: - record = self._cm.get_installed_services().get(service_id) - if record: - manifest = record.get('manifest') + record = self._cm.get_installed_services().get(service_id) + if not record: + return None + manifest = record.get('manifest') if not manifest: return None return {**manifest, 'config': self._merged_config(manifest)} def list_active(self) -> List[Dict]: - """Return only installed store services, each with merged config. - - Unlike list_all(), builtins are excluded. Use this wherever the - intent is "what has the admin chosen to run?" rather than "everything - the registry knows about." - """ + """Return all installed store services, each with merged config.""" results = [] for _svc_id, record in self._cm.get_installed_services().items(): manifest = record.get('manifest') or {} @@ -107,27 +71,8 @@ class ServiceRegistry: return results def list_all(self) -> List[Dict]: - """ - Return all services — builtins first, then installed store services — - each with merged config attached as the 'config' key. - """ - results: List[Dict] = [] - seen: set = set() - - for svc_id in self._builtin_ids(): - manifest = self._builtin_manifest(svc_id) - if manifest: - results.append({**manifest, 'config': self._merged_config(manifest)}) - seen.add(svc_id) - - for svc_id, record in self._cm.get_installed_services().items(): - if svc_id in seen: - continue - manifest = record.get('manifest') or {} - if manifest.get('id'): - results.append({**manifest, 'config': self._merged_config(manifest)}) - - return results + """Return all installed store services, each with merged config attached as the 'config' key.""" + return self.list_active() def get_caddy_routes(self) -> List[Dict]: """ diff --git a/api/services/builtins/calendar/manifest.json b/api/services/builtins/calendar/manifest.json deleted file mode 100644 index 3300644..0000000 --- a/api/services/builtins/calendar/manifest.json +++ /dev/null @@ -1,68 +0,0 @@ -{ - "schema_version": 3, - "id": "calendar", - "name": "Calendar & Contacts", - "description": "Radicale CalDAV / CardDAV server", - "version": "1.0.0", - "author": "pic", - "kind": "builtin", - "min_pic_version": "1.0", - - "capabilities": { - "has_subdomain": true, - "has_accounts": true, - "has_admin_config": true, - "has_storage": true, - "has_egress": true, - "has_api_hooks": false - }, - - "subdomain": "calendar", - "extra_subdomains": [], - "backend": "cell-radicale:5232", - - "containers": ["cell-radicale"], - - "config_schema": { - "port": { - "type": "integer", - "label": "CalDAV port (internal)", - "default": 5232, - "min": 1, - "max": 65535 - } - }, - - "peer_config_template": { - "caldav_url": "https://calendar.{domain}/{peer.username}/", - "carddav_url": "https://calendar.{domain}/{peer.username}/", - "username": "{peer.username}", - "password": "{peer.service_credentials.calendar.password}" - }, - - "accounts": { - "manager": "calendar_manager", - "credentials": ["password"] - }, - - "compose": null, - - "backup": { - "volumes": [ - {"container": "cell-radicale", "path": "/data", "name": "radicale_data"} - ], - "config_paths": [ - "config/radicale" - ] - }, - - "egress": { - "default": "default", - "allowed": ["default", "wireguard_ext", "openvpn", "tor"] - }, - - "storage": { - "primary_path": "data/radicale", - "quota_mb": null - } -} diff --git a/api/services/builtins/email/manifest.json b/api/services/builtins/email/manifest.json deleted file mode 100644 index cb95d71..0000000 --- a/api/services/builtins/email/manifest.json +++ /dev/null @@ -1,99 +0,0 @@ -{ - "schema_version": 3, - "id": "email", - "name": "Email", - "description": "Postfix (SMTP) + Dovecot (IMAP) email server with Rainloop webmail", - "version": "1.0.0", - "author": "pic", - "kind": "builtin", - "min_pic_version": "1.0", - - "capabilities": { - "has_subdomain": true, - "has_accounts": true, - "has_admin_config": true, - "has_storage": true, - "has_egress": true, - "has_api_hooks": false - }, - - "subdomain": "mail", - "extra_subdomains": ["webmail"], - "backend": "cell-rainloop:8888", - - "containers": ["cell-mail", "cell-rainloop"], - - "config_schema": { - "domain": { - "type": "string", - "label": "Mail domain", - "required": true - }, - "smtp_port": { - "type": "integer", - "label": "SMTP port", - "default": 25, - "min": 1, - "max": 65535 - }, - "submission_port": { - "type": "integer", - "label": "Submission port", - "default": 587, - "min": 1, - "max": 65535 - }, - "imap_port": { - "type": "integer", - "label": "IMAP port", - "default": 993, - "min": 1, - "max": 65535 - }, - "webmail_port": { - "type": "integer", - "label": "Webmail port (internal)", - "default": 8888, - "min": 1, - "max": 65535 - } - }, - - "peer_config_template": { - "imap_server": "{domain}", - "imap_port": "{config.imap_port}", - "smtp_server": "{domain}", - "smtp_port": "{config.submission_port}", - "webmail_url": "https://mail.{domain}/", - "username": "{peer.username}@{domain}", - "password": "{peer.service_credentials.email.password}" - }, - - "accounts": { - "manager": "email_manager", - "credentials": ["password"] - }, - - "compose": null, - - "backup": { - "volumes": [ - {"container": "cell-mail", "path": "/var/mail", "name": "maildata"}, - {"container": "cell-mail", "path": "/var/mail-state", "name": "mailstate"}, - {"container": "cell-rainloop", "path": "/rainloop/data", "name": "rainloop"} - ], - "config_paths": [ - "config/mail" - ] - }, - - "egress": { - "default": "default", - "allowed": ["default", "wireguard_ext", "openvpn", "tor"] - }, - - "storage": { - "primary_path": "data/maildata", - "quota_mb": null - } -} diff --git a/api/services/builtins/files/manifest.json b/api/services/builtins/files/manifest.json deleted file mode 100644 index 367cf6d..0000000 --- a/api/services/builtins/files/manifest.json +++ /dev/null @@ -1,79 +0,0 @@ -{ - "schema_version": 3, - "id": "files", - "name": "File Storage", - "description": "FileGator browser UI + WebDAV network drive", - "version": "1.0.0", - "author": "pic", - "kind": "builtin", - "min_pic_version": "1.0", - - "capabilities": { - "has_subdomain": true, - "has_accounts": true, - "has_admin_config": true, - "has_storage": true, - "has_egress": true, - "has_api_hooks": false - }, - - "subdomain": "files", - "extra_subdomains": ["webdav"], - "backend": "cell-filegator:8080", - "extra_backends": { - "webdav": "cell-webdav:80" - }, - - "containers": ["cell-filegator", "cell-webdav"], - - "config_schema": { - "manager_port": { - "type": "integer", - "label": "FileGator port (internal)", - "default": 8082, - "min": 1, - "max": 65535 - }, - "port": { - "type": "integer", - "label": "WebDAV port (internal)", - "default": 8080, - "min": 1, - "max": 65535 - } - }, - - "peer_config_template": { - "files_url": "https://files.{domain}/", - "webdav_url": "https://webdav.{domain}/", - "username": "{peer.username}", - "password": "{peer.service_credentials.files.password}" - }, - - "accounts": { - "manager": "file_manager", - "credentials": ["password"] - }, - - "compose": null, - - "backup": { - "volumes": [ - {"container": "cell-filegator", "path": "/var/www/filegator/private", "name": "filegator"}, - {"container": "cell-webdav", "path": "/var/lib/dav", "name": "files"} - ], - "config_paths": [ - "config/webdav" - ] - }, - - "egress": { - "default": "default", - "allowed": ["default", "wireguard_ext", "openvpn", "tor"] - }, - - "storage": { - "primary_path": "data/files", - "quota_mb": null - } -} diff --git a/tests/test_caddy_manager.py b/tests/test_caddy_manager.py index ca19512..a476859 100644 --- a/tests/test_caddy_manager.py +++ b/tests/test_caddy_manager.py @@ -70,20 +70,16 @@ class TestGenerateCaddyfilePicNgo(unittest.TestCase): # ACME staging hook self.assertIn('acme_ca {$ACME_CA_URL}', out) - def test_pic_ngo_has_subdomain_service_routes(self): + def test_pic_ngo_has_api_route_without_registry(self): mgr = _mgr() identity = {'cell_name': 'alpha', 'domain_mode': 'pic_ngo'} out = mgr.generate_caddyfile(identity, []) - # Core services get named-matcher subdomain routing - self.assertIn('@calendar host calendar.alpha.pic.ngo', out) - self.assertIn('reverse_proxy cell-radicale:5232', out) - self.assertIn('@mail host mail.alpha.pic.ngo webmail.alpha.pic.ngo', out) - self.assertIn('reverse_proxy cell-rainloop:8888', out) - self.assertIn('@files host files.alpha.pic.ngo', out) - self.assertIn('reverse_proxy cell-filegator:8080', out) - self.assertIn('@webdav host webdav.alpha.pic.ngo', out) - self.assertIn('reverse_proxy cell-webdav:80', out) + # Without a registry only the api block is present self.assertIn('@api host api.alpha.pic.ngo', out) + self.assertIn('reverse_proxy cell-api:3000', out) + self.assertNotIn('@calendar', out) + self.assertNotIn('@mail', out) + self.assertNotIn('@files', out) class TestGenerateCaddyfileCloudflare(unittest.TestCase): @@ -116,9 +112,10 @@ class TestGenerateCaddyfileCloudflare(unittest.TestCase): self.assertNotIn('*.home.local', out) # 'custom_domain' must not appear literally as a key in the output self.assertNotIn('custom_domain', out) - # Service subdomains use the correct public domain - self.assertIn('@calendar host calendar.home.example.com', out) - self.assertIn('@files host files.home.example.com', out) + # Without a registry only the api block is emitted for subdomain routing + self.assertIn('@api host api.home.example.com', out) + self.assertNotIn('@calendar', out) + self.assertNotIn('@files', out) class TestGenerateCaddyfileDuckDns(unittest.TestCase): @@ -128,8 +125,9 @@ class TestGenerateCaddyfileDuckDns(unittest.TestCase): out = mgr.generate_caddyfile(identity, []) self.assertIn('dns duckdns {$DUCKDNS_TOKEN}', out) self.assertIn('*.gamma.duckdns.org', out) - self.assertIn('@calendar host calendar.gamma.duckdns.org', out) - self.assertIn('@files host files.gamma.duckdns.org', out) + self.assertIn('@api host api.gamma.duckdns.org', out) + self.assertNotIn('@calendar', out) + self.assertNotIn('@files', out) class TestGenerateCaddyfileHttp01(unittest.TestCase): @@ -150,34 +148,27 @@ class TestGenerateCaddyfileHttp01(unittest.TestCase): self.assertNotIn('dns ', out) # No explicit tls block — Caddy uses HTTP-01 by default. self.assertNotIn('tls {', out) - # Core service blocks are always generated - self.assertIn('calendar.delta.noip.me {', out) - self.assertIn('files.delta.noip.me {', out) - self.assertIn('mail.delta.noip.me {', out) - self.assertIn('webmail.delta.noip.me {', out) - self.assertIn('webdav.delta.noip.me {', out) + # Without a registry only the api block is generated self.assertIn('api.delta.noip.me {', out) - self.assertIn('reverse_proxy cell-radicale:5232', out) - self.assertIn('reverse_proxy cell-filegator:8080', out) - # Installed plugin service block + self.assertNotIn('calendar.delta.noip.me {', out) + self.assertNotIn('files.delta.noip.me {', out) + self.assertNotIn('mail.delta.noip.me {', out) + # Installed plugin service block still works self.assertIn('chat.delta.noip.me {', out) self.assertIn('reverse_proxy cell-chat:8090', out) - def test_http01_installed_service_with_core_name_is_skipped(self): - """An installed service named 'calendar' must not produce a duplicate block.""" + def test_http01_installed_service_with_caddy_route_appears(self): + """An installed service with a caddy_route produces its own per-host block.""" mgr = _mgr() identity = { 'cell_name': 'delta', 'domain_mode': 'http01', 'domain_name': 'delta.noip.me', } - services = [{'name': 'calendar', 'caddy_route': 'reverse_proxy cell-other:9000'}] + services = [{'name': 'notes', 'caddy_route': 'reverse_proxy cell-other:9000'}] out = mgr.generate_caddyfile(identity, services) - # Only one calendar block (the core one) - self.assertEqual(out.count('calendar.delta.noip.me {'), 1) - # The core backend wins - self.assertIn('reverse_proxy cell-radicale:5232', out) - self.assertNotIn('cell-other:9000', out) + self.assertIn('notes.delta.noip.me {', out) + self.assertIn('reverse_proxy cell-other:9000', out) class TestServiceRoutesIncluded(unittest.TestCase): diff --git a/tests/test_caddy_registry_integration.py b/tests/test_caddy_registry_integration.py index cc7904d..62ceece 100644 --- a/tests/test_caddy_registry_integration.py +++ b/tests/test_caddy_registry_integration.py @@ -30,7 +30,7 @@ def _mgr_with_registry(registry=None): def _mock_registry(): - """Return a mock ServiceRegistry that reproduces the 3 builtin service routes.""" + """Return a mock ServiceRegistry that reproduces 3 store service routes.""" reg = MagicMock() reg.get_caddy_routes.return_value = [ { @@ -76,33 +76,39 @@ def _nm(registry=None): class TestBuildRegistryServiceRoutes(unittest.TestCase): - def test_returns_hardcoded_when_no_registry(self): - """service_registry=None produces the same output as _build_core_service_routes.""" + def test_returns_api_only_when_no_registry(self): + """service_registry=None produces only the @api block.""" mgr = _mgr_with_registry(registry=None) domain = 'alpha.pic.ngo' result = mgr._build_registry_service_routes(domain) - expected = CaddyManager._build_core_service_routes(domain) - self.assertEqual(result, expected) + self.assertIn('@api host api.alpha.pic.ngo', result) + self.assertIn('reverse_proxy cell-api:3000', result) + self.assertNotIn('@calendar', result) + self.assertNotIn('@mail', result) - def test_returns_hardcoded_when_registry_empty(self): - """An empty route list from the registry falls back to hardcoded.""" + def test_returns_api_only_when_registry_empty(self): + """An empty route list from the registry produces only the @api block.""" reg = MagicMock() reg.get_caddy_routes.return_value = [] mgr = _mgr_with_registry(registry=reg) domain = 'alpha.pic.ngo' result = mgr._build_registry_service_routes(domain) - expected = CaddyManager._build_core_service_routes(domain) - self.assertEqual(result, expected) + self.assertIn('@api host api.alpha.pic.ngo', result) + self.assertIn('reverse_proxy cell-api:3000', result) + self.assertNotIn('@calendar', result) + self.assertNotIn('@mail', result) - def test_registry_error_falls_back(self): - """When get_caddy_routes raises, output equals _build_core_service_routes.""" + def test_returns_api_only_on_registry_error(self): + """When get_caddy_routes raises, only the @api block is produced.""" reg = MagicMock() reg.get_caddy_routes.side_effect = Exception('registry unavailable') mgr = _mgr_with_registry(registry=reg) domain = 'alpha.pic.ngo' result = mgr._build_registry_service_routes(domain) - expected = CaddyManager._build_core_service_routes(domain) - self.assertEqual(result, expected) + self.assertIn('@api host api.alpha.pic.ngo', result) + self.assertIn('reverse_proxy cell-api:3000', result) + self.assertNotIn('@calendar', result) + self.assertNotIn('@mail', result) def test_single_service_no_extras(self): """One service with no extra_subdomains produces one matcher + handle + api block.""" @@ -234,27 +240,25 @@ class TestHttp01ServicePairs(unittest.TestCase): self.assertEqual(webdav_entry, 'cell-webdav:80') self.assertNotEqual(webdav_entry, 'cell-filegator:8080') - def test_fallback_when_no_registry(self): - """Without a registry the hardcoded pairs are returned, including api.""" + def test_only_api_when_no_registry(self): + """Without a registry only the api pair is returned.""" mgr = _mgr_with_registry(registry=None) pairs = mgr._http01_service_pairs() subdomains = [s for s, _ in pairs] - self.assertIn('calendar', subdomains) - self.assertIn('mail', subdomains) - self.assertIn('webmail', subdomains) - self.assertIn('files', subdomains) - self.assertIn('webdav', subdomains) self.assertIn('api', subdomains) + self.assertNotIn('calendar', subdomains) + self.assertNotIn('mail', subdomains) + self.assertNotIn('files', subdomains) - def test_fallback_when_registry_error(self): - """When get_caddy_routes raises, falls back to hardcoded pairs.""" + def test_only_api_on_registry_error(self): + """When get_caddy_routes raises, only the api pair is present.""" reg = MagicMock() reg.get_caddy_routes.side_effect = RuntimeError('boom') mgr = _mgr_with_registry(registry=reg) pairs = mgr._http01_service_pairs() subdomains = [s for s, _ in pairs] - self.assertIn('calendar', subdomains) self.assertIn('api', subdomains) + self.assertNotIn('calendar', subdomains) # --------------------------------------------------------------------------- @@ -326,14 +330,14 @@ class TestCaddyfileWithRegistry(unittest.TestCase): self.assertIn('reverse_proxy cell-filegator:8080', out) self.assertIn('reverse_proxy cell-webdav:80', out) - def test_pic_ngo_fallback_when_registry_empty(self): - """pic_ngo falls back to hardcoded routes when registry returns empty list.""" + def test_pic_ngo_api_only_when_registry_empty(self): + """pic_ngo emits only the api block when registry returns empty list.""" reg = MagicMock() reg.get_caddy_routes.return_value = [] out = self._generate('pic_ngo', cell_name='alpha', registry=reg) - # Hardcoded routes should appear - self.assertIn('@calendar host calendar.alpha.pic.ngo', out) - self.assertIn('@mail host mail.alpha.pic.ngo webmail.alpha.pic.ngo', out) + self.assertIn('@api host api.alpha.pic.ngo', out) + self.assertNotIn('@calendar', out) + self.assertNotIn('@mail', out) # --------------------------------------------------------------------------- @@ -354,11 +358,11 @@ class TestNetworkManagerGetServiceSubdomains(unittest.TestCase): self.managers.append(nm) return nm - def test_no_registry_returns_hardcoded(self): - """Without a registry the hardcoded service subdomain list is returned.""" + def test_no_registry_returns_empty(self): + """Without a registry an empty list is returned.""" nm = self._make(registry=None) subs = nm._get_service_subdomains() - self.assertCountEqual(subs, ['calendar', 'files', 'mail', 'webmail', 'webdav']) + self.assertEqual(subs, []) def test_registry_returns_all_subdomains(self): """Primary + extra_subdomains from all routes are returned.""" @@ -369,13 +373,13 @@ class TestNetworkManagerGetServiceSubdomains(unittest.TestCase): for expected in ('calendar', 'mail', 'webmail', 'files', 'webdav'): self.assertIn(expected, subs) - def test_registry_error_falls_back(self): - """When get_caddy_routes raises, hardcoded list is returned.""" + def test_registry_error_returns_empty(self): + """When get_caddy_routes raises, an empty list is returned.""" reg = MagicMock() reg.get_caddy_routes.side_effect = Exception('broken registry') nm = self._make(registry=reg) subs = nm._get_service_subdomains() - self.assertCountEqual(subs, ['calendar', 'files', 'mail', 'webmail', 'webdav']) + self.assertEqual(subs, []) def test_registry_extra_subdomains_included(self): """extra_subdomains from each route are included in the returned list.""" diff --git a/tests/test_network_manager.py b/tests/test_network_manager.py index 798002c..8bff7d8 100644 --- a/tests/test_network_manager.py +++ b/tests/test_network_manager.py @@ -349,8 +349,12 @@ class TestApplyIpRange(unittest.TestCase): self.nm.apply_ip_range('10.1.2.0/24', 'pictest', 'mycell') zone_file = os.path.join(self.nm.dns_zones_dir, 'mycell.zone') content = open(zone_file).read() - for host in ('pictest', 'api', 'webui', 'calendar', 'files', 'mail', 'webmail', 'webdav'): + # Without a registry, only the infrastructure names are generated + for host in ('pictest', 'api', 'webui'): self.assertIn(host, content) + # Service records are only generated when a registry is wired + for host in ('calendar', 'files', 'mail', 'webmail', 'webdav'): + self.assertNotIn(host, content) @patch('subprocess.run') def test_same_range_updates_zone_without_error(self, _mock): @@ -460,7 +464,21 @@ class TestUpdateSplitHorizonZone(unittest.TestCase): @patch('subprocess.run') def test_removes_stale_service_records_when_primary_is_parent(self, _mock): - """Stale LAN service names (api, calendar…) are removed from a parent zone.""" + """Stale LAN service names (api, calendar…) are removed from a parent zone. + + A registry that knows about calendar and files is required so those names + appear in the stale set. + """ + from unittest.mock import MagicMock + registry = MagicMock() + registry.get_caddy_routes.return_value = [ + {'service_id': 'calendar', 'subdomain': 'calendar', + 'backend': 'cell-radicale:5232', 'extra_subdomains': [], 'extra_backends': {}}, + {'service_id': 'files', 'subdomain': 'files', + 'backend': 'cell-filegator:8080', 'extra_subdomains': [], 'extra_backends': {}}, + ] + self.nm._service_registry = registry + # Bootstrap a pic.ngo zone with service records (wrong internal zone name) stale_records = [ {'name': 'pic2', 'type': 'A', 'value': '10.0.0.1'}, diff --git a/tests/test_optional_services_feature.py b/tests/test_optional_services_feature.py index 5760ebf..01553f3 100644 --- a/tests/test_optional_services_feature.py +++ b/tests/test_optional_services_feature.py @@ -89,10 +89,9 @@ class TestServiceRegistryListActive(unittest.TestCase): return ServiceRegistry(cm) def test_list_active_zero_installed_returns_empty(self): - """With no installed records and no builtins on disk, list_active() is empty.""" + """With no installed records, list_active() is empty.""" reg = self._make_registry(installed={}) - with patch('service_registry._BUILTINS_DIR', '/nonexistent/builtins'): - result = reg.list_active() + result = reg.list_active() self.assertEqual(result, []) def test_list_active_one_installed_returns_only_that_service(self): @@ -102,8 +101,7 @@ class TestServiceRegistryListActive(unittest.TestCase): 'email': {'manifest': email_manifest}, } reg = self._make_registry(installed=installed) - with patch('service_registry._BUILTINS_DIR', '/nonexistent/builtins'): - result = reg.list_active() + result = reg.list_active() ids = [s['id'] for s in result] self.assertIn('email', ids) self.assertNotIn('calendar', ids) @@ -117,8 +115,7 @@ class TestServiceRegistryListActive(unittest.TestCase): 'files': {'manifest': _store_manifest('files', 'files', 'cell-filegator:8080')}, } reg = self._make_registry(installed=installed) - with patch('service_registry._BUILTINS_DIR', '/nonexistent/builtins'): - result = reg.list_active() + result = reg.list_active() ids = {s['id'] for s in result} self.assertEqual(ids, {'email', 'calendar', 'files'}) @@ -128,8 +125,7 @@ class TestServiceRegistryListActive(unittest.TestCase): 'calendar': {'manifest': _store_manifest('calendar', 'calendar', 'cell-radicale:5232')}, } reg = self._make_registry(installed=installed) - with patch('service_registry._BUILTINS_DIR', '/nonexistent/builtins'): - result = reg.list_active() + result = reg.list_active() for svc in result: self.assertIn('config', svc, f'{svc["id"]} is missing the config key') @@ -139,8 +135,7 @@ class TestServiceRegistryListActive(unittest.TestCase): 'broken': {}, # no 'manifest' key at all } reg = self._make_registry(installed=installed) - with patch('service_registry._BUILTINS_DIR', '/nonexistent/builtins'): - result = reg.list_active() + result = reg.list_active() self.assertEqual(result, []) @@ -255,13 +250,12 @@ class TestServiceRegistryGetNotInstalled(unittest.TestCase): unless the service is in get_installed_services(). """ - def test_get_returns_none_when_not_in_builtins_and_not_installed(self): + def test_get_returns_none_when_not_installed(self): cm = MagicMock() cm.configs = {} cm.get_installed_services.return_value = {} reg = ServiceRegistry(cm) - with patch('service_registry._BUILTINS_DIR', '/nonexistent/builtins'): - result = reg.get('email') + result = reg.get('email') self.assertIsNone(result) def test_get_returns_none_for_calendar_when_not_installed(self): @@ -269,16 +263,14 @@ class TestServiceRegistryGetNotInstalled(unittest.TestCase): cm.configs = {} cm.get_installed_services.return_value = {} reg = ServiceRegistry(cm) - with patch('service_registry._BUILTINS_DIR', '/nonexistent/builtins'): - self.assertIsNone(reg.get('calendar')) + self.assertIsNone(reg.get('calendar')) def test_get_returns_none_for_files_when_not_installed(self): cm = MagicMock() cm.configs = {} cm.get_installed_services.return_value = {} reg = ServiceRegistry(cm) - with patch('service_registry._BUILTINS_DIR', '/nonexistent/builtins'): - self.assertIsNone(reg.get('files')) + self.assertIsNone(reg.get('files')) def test_get_returns_service_when_installed(self): """Once email is in installed records it must be returned by get().""" @@ -289,8 +281,7 @@ class TestServiceRegistryGetNotInstalled(unittest.TestCase): 'email': {'manifest': email_manifest}, } reg = ServiceRegistry(cm) - with patch('service_registry._BUILTINS_DIR', '/nonexistent/builtins'): - result = reg.get('email') + result = reg.get('email') self.assertIsNotNone(result) self.assertEqual(result['id'], 'email') @@ -595,17 +586,11 @@ class TestUninstallNotInstalled(unittest.TestCase): class TestCaddyManagerEmptyActiveRegistry(unittest.TestCase): """ When the registry returns no active routes (empty list_active()), the - registry-driven path produces no service matcher blocks — it falls back - to the hardcoded _build_core_service_routes. + registry-driven path produces only the @api block — no service matcher + blocks for calendar/mail/files/webdav. - The important test for this feature is that a registry returning [] from - get_caddy_routes produces no service blocks in a NEW install where - email/calendar/files have NOT been installed yet. - - The existing fallback behaviour (empty → hardcoded) is already tested in - test_caddy_registry_integration.py:TestBuildRegistryServiceRoutes. These - new tests verify what happens when we pass a registry that explicitly - signals zero active services (e.g. all three were just uninstalled). + Phase 2: builtins removed, so there is no hardcoded fallback. An empty + registry means no service routes at all (except the always-present api block). """ def _mgr_with_empty_registry(self): @@ -616,21 +601,17 @@ class TestCaddyManagerEmptyActiveRegistry(unittest.TestCase): return CaddyManager(config_manager=cm, service_registry=reg) def test_empty_active_list_produces_no_service_matcher_blocks(self): - """Zero active services → no @calendar, @mail, @files, @webdav matchers - when we override the fallback behaviour by returning hardcoded routes - only because registry is empty. + """Zero active services → no @calendar, @mail, @files, @webdav matchers. - NOTE: the current implementation falls back to _build_core_service_routes - when the registry returns []. This test documents that existing behaviour. - When list_active() is wired in and builtins are removed, this test will - need updating to assert no service matchers appear. For now it pins the - fallback contract. + Phase 2: builtins are gone so an empty registry produces only the @api block. """ mgr = self._mgr_with_empty_registry() result = mgr._build_registry_service_routes('mycell.pic.ngo') - # Current contract: empty registry → hardcoded fallback is used - expected = CaddyManager._build_core_service_routes('mycell.pic.ngo') - self.assertEqual(result, expected) + self.assertIn('@api host api.mycell.pic.ngo', result) + self.assertNotIn('@calendar', result) + self.assertNotIn('@mail', result) + self.assertNotIn('@files', result) + self.assertNotIn('@webdav', result) def test_empty_registry_no_store_service_blocks_injected(self): """An empty active list must not inject any store-service-specific matchers.""" @@ -912,145 +893,75 @@ class TestMigrateLegacyContainers(unittest.TestCase): # --------------------------------------------------------------------------- -# 10. Existing tests that will break when builtins are removed from disk -# (documented with the exact assertion that breaks) +# 10. Phase 2 completion: verify builtins layer is fully removed # --------------------------------------------------------------------------- -class TestDocumentedBreakagePoints(unittest.TestCase): +class TestPhase2CompletionChecks(unittest.TestCase): """ - These tests do NOT fail right now because the builtin manifests are still - on disk. They are here to document which assertions in the existing test - suite will start failing the moment - api/services/builtins/{email,calendar,files}/manifest.json are deleted. + Confirms that Phase 2 (builtins removal) is complete. - Each test runs the existing assertion in isolation so you can confirm it - fails after deletion by running this class with -v. + These tests verify the post-migration state: no builtins directory, + no hardcoded fallbacks, and registry-only routing for all services. """ - def _load_builtin(self, service_id): - from service_registry import _BUILTINS_DIR - path = os.path.join(_BUILTINS_DIR, service_id, 'manifest.json') - if not os.path.exists(path): - self.skipTest(f'builtin manifest for {service_id!r} already removed') - with open(path) as f: - return json.load(f) + def test_builtins_dir_does_not_exist(self): + """api/services/builtins/ must not exist after Phase 2.""" + import api.service_registry as sr_module + self.assertFalse(hasattr(sr_module, '_BUILTINS_DIR'), + 'service_registry must not export _BUILTINS_DIR after Phase 2') - # --- test_service_registry.py::TestBuiltinManifests --- - - def test_BREAKAGE_email_manifest_exists_on_disk(self): - """ - test_service_registry.py::TestBuiltinManifests::test_email_manifest_valid - BREAKS because _load('email') calls os.path.exists on the builtin path - and asserts True. - """ - self._load_builtin('email') # will raise AssertionError once file is deleted - - def test_BREAKAGE_calendar_manifest_exists_on_disk(self): - """test_calendar_manifest_valid breaks for the same reason.""" - self._load_builtin('calendar') - - def test_BREAKAGE_files_manifest_exists_on_disk(self): - """test_files_manifest_valid breaks for the same reason.""" - self._load_builtin('files') - - # --- test_service_registry.py::TestServiceRegistryListAll --- - - def test_BREAKAGE_list_all_returns_three_builtins(self): - """ - test_service_registry.py::TestServiceRegistryListAll::test_lists_three_builtins - asserts: assertIn('email', ids), assertIn('calendar', ids), assertIn('files', ids) - All three will fail when builtins are removed unless install records exist. - """ + def test_list_all_empty_without_installed_services(self): + """list_all() returns [] when nothing is installed.""" cm = MagicMock() cm.configs = {} cm.get_installed_services.return_value = {} reg = ServiceRegistry(cm) - with patch('service_registry._BUILTINS_DIR', '/nonexistent/builtins'): - services = reg.list_all() + services = reg.list_all() ids = [s['id'] for s in services] - # Demonstrates the breakage: these assertions will fail - self.assertNotIn('email', ids, - 'After builtin removal email must NOT appear in list_all without an install record') + self.assertNotIn('email', ids) self.assertNotIn('calendar', ids) self.assertNotIn('files', ids) + self.assertEqual(ids, []) - # --- test_service_registry.py::TestServiceRegistryGetCaddyRoutes --- - - def test_BREAKAGE_get_caddy_routes_empty_without_builtins(self): - """ - TestServiceRegistryGetCaddyRoutes::test_all_builtins_appear_in_routes - assertIn('email', route_ids) etc will all fail when builtins removed. - """ + def test_get_caddy_routes_empty_without_installed_services(self): + """get_caddy_routes() returns [] when nothing is installed.""" cm = MagicMock() cm.configs = {} cm.get_installed_services.return_value = {} reg = ServiceRegistry(cm) - with patch('service_registry._BUILTINS_DIR', '/nonexistent/builtins'): - routes = reg.get_caddy_routes() - route_ids = [r['service_id'] for r in routes] - self.assertEqual(route_ids, [], - 'With no builtins on disk and nothing installed, routes must be empty') + routes = reg.get_caddy_routes() + self.assertEqual(routes, []) - # --- test_service_registry.py::TestServiceRegistryGetBackupPlan --- - - def test_BREAKAGE_backup_plan_empty_without_builtins(self): - """ - TestServiceRegistryGetBackupPlan::test_all_builtins_in_backup_plan - will fail for all three service IDs. - """ + def test_backup_plan_empty_without_installed_services(self): + """get_backup_plan() returns [] when nothing is installed.""" cm = MagicMock() cm.configs = {} cm.get_installed_services.return_value = {} reg = ServiceRegistry(cm) - with patch('service_registry._BUILTINS_DIR', '/nonexistent/builtins'): - plan = reg.get_backup_plan() - self.assertEqual(plan, [], - 'Backup plan must be empty when builtins removed and nothing installed') + plan = reg.get_backup_plan() + self.assertEqual(plan, []) - # --- test_service_registry.py::TestServiceRegistryConfigMerge --- - - def test_BREAKAGE_config_merge_returns_none_without_builtin(self): - """ - TestServiceRegistryConfigMerge::test_defaults_used_when_no_saved_config - calls reg.get('calendar') and asserts result['config']['port'] == 5232. - Once the calendar manifest is gone from disk, get('calendar') returns None - and None['config'] raises TypeError. - """ + def test_get_returns_none_for_uninstalled_service(self): + """get('calendar') returns None when calendar is not installed.""" cm = MagicMock() cm.configs = {'calendar': {}} cm.get_installed_services.return_value = {} reg = ServiceRegistry(cm) - with patch('service_registry._BUILTINS_DIR', '/nonexistent/builtins'): - result = reg.get('calendar') - self.assertIsNone(result, - 'get(calendar) must be None when builtin is removed and no install record exists') + result = reg.get('calendar') + self.assertIsNone(result) - # --- test_caddy_registry_integration.py — fallback to hardcoded --- - - def test_BREAKAGE_caddy_with_empty_registry_falls_back_to_hardcoded(self): - """ - TestCaddyfileWithRegistry::test_pic_ngo_fallback_when_registry_empty - currently tests that an empty registry list falls back to hardcoded routes - which include calendar/mail/files. - - When list_active() is wired in and builtins are gone, returning [] should - mean NO service routes at all — the fallback to hardcoded must also be removed. - The existing test assertion 'assertIn @calendar...' will then be WRONG. - - This test documents the collision: currently the fallback is correct - behaviour; after the migration it becomes a bug. - """ + def test_caddy_empty_registry_produces_only_api_block(self): + """Empty registry → no service matcher blocks (no hardcoded fallback).""" reg = MagicMock() reg.get_caddy_routes.return_value = [] cm = MagicMock() cm.get_identity.return_value = {} mgr = CaddyManager(config_manager=cm, service_registry=reg) result = mgr._build_registry_service_routes('alpha.pic.ngo') - # Documents current (pre-migration) contract: - # Empty registry → hardcoded fallback → calendar still appears - self.assertIn('@calendar host calendar.alpha.pic.ngo', result, - 'Pre-migration: empty registry falls back to hardcoded; ' - 'this assertion must be INVERTED after the migration is complete') + self.assertIn('@api host api.alpha.pic.ngo', result) + self.assertNotIn('@calendar', result) + self.assertNotIn('@mail', result) + self.assertNotIn('@files', result) if __name__ == '__main__': diff --git a/tests/test_peer_dashboard_services.py b/tests/test_peer_dashboard_services.py index 0e3a6eb..0946d62 100644 --- a/tests/test_peer_dashboard_services.py +++ b/tests/test_peer_dashboard_services.py @@ -500,35 +500,47 @@ class TestDNSZoneRecords: f"got {rec['value']}" ) - def test_calendar_resolves_to_wg_server_ip(self): - records = self._records() - rec = next((r for r in records if r['name'] == 'calendar'), None) - assert rec and rec['value'] == self._WG_SERVER_IP, \ - f"calendar.dev should resolve to WG server IP; got {rec}" + def test_service_records_absent_without_registry(self): + """Without a registry, service subdomain records are not generated. - def test_files_resolves_to_wg_server_ip(self): + Phase 2: service DNS records only exist when a service is installed + and the registry reports it. The hardcoded fallback is gone. + """ records = self._records() - rec = next((r for r in records if r['name'] == 'files'), None) - assert rec and rec['value'] == self._WG_SERVER_IP, \ - f"files.dev should resolve to WG server IP; got {rec}" + names = {r['name'] for r in records} + assert 'calendar' not in names, \ + 'calendar DNS record must not appear without a registry' + assert 'files' not in names, \ + 'files DNS record must not appear without a registry' + assert 'mail' not in names, \ + 'mail DNS record must not appear without a registry' + assert 'webmail' not in names, \ + 'webmail DNS record must not appear without a registry' + assert 'webdav' not in names, \ + 'webdav DNS record must not appear without a registry' - def test_mail_resolves_to_wg_server_ip(self): - records = self._records() - rec = next((r for r in records if r['name'] == 'mail'), None) - assert rec and rec['value'] == self._WG_SERVER_IP, \ - f"mail.dev should resolve to WG server IP; got {rec}" - - def test_webmail_resolves_to_wg_server_ip(self): - records = self._records() - rec = next((r for r in records if r['name'] == 'webmail'), None) - assert rec and rec['value'] == self._WG_SERVER_IP, \ - f"webmail.dev should resolve to WG server IP; got {rec}" - - def test_webdav_resolves_to_wg_server_ip(self): - records = self._records() - rec = next((r for r in records if r['name'] == 'webdav'), None) - assert rec and rec['value'] == self._WG_SERVER_IP, \ - f"webdav.dev should resolve to WG server IP; got {rec}" + def test_service_records_present_with_registry(self): + """With a registry that provides calendar/mail/files, all resolve to WG IP.""" + from unittest.mock import MagicMock + import network_manager as nm + registry = MagicMock() + registry.get_caddy_routes.return_value = [ + {'service_id': 'calendar', 'subdomain': 'calendar', + 'backend': 'cell-radicale:5232', 'extra_subdomains': [], 'extra_backends': {}}, + {'service_id': 'email', 'subdomain': 'mail', + 'backend': 'cell-rainloop:8888', 'extra_subdomains': ['webmail'], 'extra_backends': {}}, + {'service_id': 'files', 'subdomain': 'files', + 'backend': 'cell-filegator:8080', 'extra_subdomains': ['webdav'], 'extra_backends': {}}, + ] + mgr = nm.NetworkManager.__new__(nm.NetworkManager) + mgr._service_registry = registry + records = mgr._build_dns_records('pic0', '172.20.0.0/16') + names = {r['name'] for r in records} + for expected in ('calendar', 'mail', 'webmail', 'files', 'webdav'): + assert expected in names, f'{expected} should be in DNS records with registry' + for rec in records: + assert rec['value'] == self._WG_SERVER_IP, \ + f"Record {rec['name']} should point to WG server IP" def test_cell_name_resolves_to_wg_server_ip(self): records = self._records(cell_name='mypic') diff --git a/tests/test_service_registry.py b/tests/test_service_registry.py index b683719..2ec4c0e 100644 --- a/tests/test_service_registry.py +++ b/tests/test_service_registry.py @@ -1,164 +1,210 @@ """ Unit tests for ServiceRegistry. -Tests load actual built-in manifests from api/services/builtins/ and verify -that the registry merges config correctly, returns expected routes/backup plans, -and handles missing manifests gracefully. +Tests verify that the registry merges config correctly from installed store +services, returns expected routes/backup plans, and handles missing or broken +records gracefully. There are no builtins — only installed store services. """ -import json -import os import sys -import tempfile import unittest from pathlib import Path -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock sys.path.insert(0, str(Path(__file__).parent.parent / 'api')) -from service_registry import ServiceRegistry, _BUILTINS_DIR +from service_registry import ServiceRegistry -def _make_cm(configs: dict = None) -> MagicMock: +# --------------------------------------------------------------------------- +# Shared manifests used across multiple test classes +# --------------------------------------------------------------------------- + +_CALENDAR_MANIFEST = { + 'schema_version': 1, + 'id': 'calendar', + 'name': 'Calendar', + 'kind': 'store', + 'capabilities': { + 'has_subdomain': True, + 'has_accounts': False, + 'has_admin_config': True, + 'has_storage': True, + 'has_egress': False, + 'has_api_hooks': False, + }, + 'subdomain': 'calendar', + 'backend': 'cell-radicale:5232', + 'extra_subdomains': [], + 'extra_backends': {}, + 'config_schema': { + 'port': {'type': 'integer', 'default': 5232, 'label': 'Port'}, + }, + 'peer_config_template': { + 'caldav_url': 'https://calendar.{domain}/radicale/{peer.username}/', + 'password': '{peer.service_credentials.calendar.password}', + }, + 'backup': { + 'volumes': [ + {'container': 'cell-radicale', 'path': '/data', 'name': 'radicale_data'} + ] + }, +} + +_EMAIL_MANIFEST = { + 'schema_version': 1, + 'id': 'email', + 'name': 'Email', + 'kind': 'store', + 'capabilities': { + 'has_subdomain': True, + 'has_accounts': True, + 'has_admin_config': True, + 'has_storage': True, + 'has_egress': True, + 'has_api_hooks': False, + }, + 'subdomain': 'mail', + 'backend': 'cell-rainloop:8888', + 'extra_subdomains': ['webmail'], + 'extra_backends': {}, + 'config_schema': { + 'smtp_port': {'type': 'integer', 'default': 587, 'label': 'SMTP Port'}, + 'imap_port': {'type': 'integer', 'default': 993, 'label': 'IMAP Port'}, + }, + 'peer_config_template': { + 'smtp_server': 'mail.{domain}', + 'imap_server': 'mail.{domain}', + 'password': '{peer.service_credentials.email.password}', + }, + 'backup': { + 'volumes': [ + {'container': 'cell-mail', 'path': '/var/mail', 'name': 'maildata'}, + ] + }, +} + +_FILES_MANIFEST = { + 'schema_version': 1, + 'id': 'files', + 'name': 'Files', + 'kind': 'store', + 'capabilities': { + 'has_subdomain': True, + 'has_accounts': False, + 'has_admin_config': False, + 'has_storage': True, + 'has_egress': False, + 'has_api_hooks': False, + }, + 'subdomain': 'files', + 'backend': 'cell-filegator:8080', + 'extra_subdomains': ['webdav'], + 'extra_backends': {'webdav': 'cell-webdav:80'}, + 'config_schema': {}, + 'peer_config_template': { + 'files_url': 'https://files.{domain}/', + }, + 'backup': { + 'volumes': [ + {'container': 'cell-filegator', 'path': '/data', 'name': 'filegator'}, + {'container': 'cell-webdav', 'path': '/data', 'name': 'files'}, + ] + }, +} + + +def _make_cm(configs: dict = None, installed: dict = None) -> MagicMock: cm = MagicMock() cm.configs = configs or {} - cm.get_installed_services.return_value = {} + cm.get_installed_services.return_value = installed or {} return cm -class TestBuiltinManifests(unittest.TestCase): - """Verify the built-in manifest files are valid JSON with required fields.""" - - def _load(self, service_id: str) -> dict: - path = os.path.join(_BUILTINS_DIR, service_id, 'manifest.json') - self.assertTrue(os.path.exists(path), f'Missing manifest for {service_id}') - with open(path) as f: - return json.load(f) - - def _assert_required(self, manifest: dict): - for field in ('schema_version', 'id', 'name', 'kind', 'capabilities'): - self.assertIn(field, manifest, f'Missing required field: {field}') - caps = manifest['capabilities'] - for cap in ('has_subdomain', 'has_accounts', 'has_admin_config', - 'has_storage', 'has_egress', 'has_api_hooks'): - self.assertIn(cap, caps, f'Missing capability flag: {cap}') - - def test_email_manifest_valid(self): - m = self._load('email') - self._assert_required(m) - self.assertEqual(m['id'], 'email') - self.assertEqual(m['kind'], 'builtin') - self.assertIn('mail', [m.get('subdomain')] + (m.get('extra_subdomains') or [])) - self.assertIn('webmail', m.get('extra_subdomains', [])) - self.assertEqual(m['capabilities']['has_accounts'], True) - - def test_calendar_manifest_valid(self): - m = self._load('calendar') - self._assert_required(m) - self.assertEqual(m['id'], 'calendar') - self.assertEqual(m['subdomain'], 'calendar') - - def test_files_manifest_valid(self): - m = self._load('files') - self._assert_required(m) - self.assertEqual(m['id'], 'files') - self.assertIn('webdav', m.get('extra_subdomains', [])) - - def test_all_builtins_have_backup_volumes(self): - for svc_id in ('email', 'calendar', 'files'): - m = self._load(svc_id) - volumes = m.get('backup', {}).get('volumes') - self.assertTrue(volumes, f'{svc_id}: backup.volumes must not be empty') - for vol in volumes: - for field in ('container', 'path', 'name'): - self.assertIn(field, vol, - f'{svc_id}: backup volume entry missing {field!r}') - - def test_all_builtins_have_peer_config_template(self): - for svc_id in ('email', 'calendar', 'files'): - m = self._load(svc_id) - self.assertTrue(m.get('peer_config_template'), - f'{svc_id}: peer_config_template must not be empty') - - def test_config_schema_defaults_are_correct_types(self): - for svc_id in ('email', 'calendar', 'files'): - m = self._load(svc_id) - for field, spec in (m.get('config_schema') or {}).items(): - if 'default' in spec: - if spec['type'] == 'integer': - self.assertIsInstance( - spec['default'], int, - f'{svc_id}.{field}: integer default must be int') - elif spec['type'] == 'string': - self.assertIsInstance( - spec['default'], str, - f'{svc_id}.{field}: string default must be str') - +# --------------------------------------------------------------------------- +# TestServiceRegistryListAll +# --------------------------------------------------------------------------- class TestServiceRegistryListAll(unittest.TestCase): - def setUp(self): - self.cm = _make_cm() - self.registry = ServiceRegistry(self.cm) + def test_list_all_empty_when_nothing_installed(self): + cm = _make_cm() + reg = ServiceRegistry(cm) + self.assertEqual(reg.list_all(), []) - def test_lists_three_builtins(self): - services = self.registry.list_all() - ids = [s['id'] for s in services] - self.assertIn('email', ids) + def test_list_all_returns_installed_services(self): + cm = _make_cm(installed={'calendar': {'manifest': _CALENDAR_MANIFEST}}) + reg = ServiceRegistry(cm) + ids = [s['id'] for s in reg.list_all()] self.assertIn('calendar', ids) - self.assertIn('files', ids) - - def test_builtins_come_before_store_services(self): - self.cm.get_installed_services.return_value = { - 'zstore': {'manifest': { - 'id': 'zstore', 'name': 'Z Store', 'kind': 'store', - 'capabilities': {}, 'config_schema': {} - }} - } - services = self.registry.list_all() - ids = [s['id'] for s in services] - # builtins (email, calendar, files) should all appear before zstore - for builtin_id in ('email', 'calendar', 'files'): - self.assertLess(ids.index(builtin_id), ids.index('zstore')) def test_each_service_has_config_key(self): - for svc in self.registry.list_all(): + cm = _make_cm(installed={ + 'calendar': {'manifest': _CALENDAR_MANIFEST}, + 'email': {'manifest': _EMAIL_MANIFEST}, + }) + reg = ServiceRegistry(cm) + for svc in reg.list_all(): self.assertIn('config', svc, f'{svc["id"]} missing config key') def test_no_duplicate_ids(self): - services = self.registry.list_all() - ids = [s['id'] for s in services] + cm = _make_cm(installed={ + 'calendar': {'manifest': _CALENDAR_MANIFEST}, + 'email': {'manifest': _EMAIL_MANIFEST}, + }) + reg = ServiceRegistry(cm) + ids = [s['id'] for s in reg.list_all()] self.assertEqual(len(ids), len(set(ids))) +# --------------------------------------------------------------------------- +# TestServiceRegistryConfigMerge +# --------------------------------------------------------------------------- + class TestServiceRegistryConfigMerge(unittest.TestCase): def test_defaults_used_when_no_saved_config(self): - cm = _make_cm({'calendar': {}}) + cm = _make_cm( + configs={'calendar': {}}, + installed={'calendar': {'manifest': _CALENDAR_MANIFEST}}, + ) reg = ServiceRegistry(cm) svc = reg.get('calendar') self.assertEqual(svc['config']['port'], 5232) def test_saved_config_overrides_defaults(self): - cm = _make_cm({'calendar': {'port': 9999}}) + cm = _make_cm( + configs={'calendar': {'port': 9999}}, + installed={'calendar': {'manifest': _CALENDAR_MANIFEST}}, + ) reg = ServiceRegistry(cm) svc = reg.get('calendar') self.assertEqual(svc['config']['port'], 9999) def test_unknown_saved_keys_excluded(self): - cm = _make_cm({'calendar': {'port': 5232, 'unknown_field': 'x'}}) + cm = _make_cm( + configs={'calendar': {'port': 5232, 'unknown_field': 'x'}}, + installed={'calendar': {'manifest': _CALENDAR_MANIFEST}}, + ) reg = ServiceRegistry(cm) svc = reg.get('calendar') self.assertNotIn('unknown_field', svc['config']) def test_partial_override_keeps_other_defaults(self): - cm = _make_cm({'email': {'smtp_port': 2525}}) + cm = _make_cm( + configs={'email': {'smtp_port': 2525}}, + installed={'email': {'manifest': _EMAIL_MANIFEST}}, + ) reg = ServiceRegistry(cm) svc = reg.get('email') self.assertEqual(svc['config']['smtp_port'], 2525) self.assertEqual(svc['config']['imap_port'], 993) +# --------------------------------------------------------------------------- +# TestServiceRegistryGet +# --------------------------------------------------------------------------- + class TestServiceRegistryGet(unittest.TestCase): def setUp(self): @@ -168,11 +214,6 @@ class TestServiceRegistryGet(unittest.TestCase): def test_returns_none_for_unknown_id(self): self.assertIsNone(self.registry.get('nonexistent_service')) - def test_returns_builtin_by_id(self): - svc = self.registry.get('email') - self.assertIsNotNone(svc) - self.assertEqual(svc['id'], 'email') - def test_returns_store_service_from_installed(self): self.cm.get_installed_services.return_value = { 'mywiki': {'manifest': { @@ -184,6 +225,16 @@ class TestServiceRegistryGet(unittest.TestCase): self.assertIsNotNone(svc) self.assertEqual(svc['id'], 'mywiki') + def test_get_returns_none_when_installed_record_has_no_manifest(self): + self.cm.get_installed_services.return_value = { + 'broken': {} # record exists but has no 'manifest' key + } + self.assertIsNone(self.registry.get('broken')) + + +# --------------------------------------------------------------------------- +# TestServiceRegistryGetCaddyRoutes +# --------------------------------------------------------------------------- class TestServiceRegistryGetCaddyRoutes(unittest.TestCase): @@ -191,22 +242,6 @@ class TestServiceRegistryGetCaddyRoutes(unittest.TestCase): self.cm = _make_cm() self.registry = ServiceRegistry(self.cm) - def test_all_builtins_appear_in_routes(self): - routes = self.registry.get_caddy_routes() - route_ids = [r['service_id'] for r in routes] - for svc_id in ('email', 'calendar', 'files'): - self.assertIn(svc_id, route_ids) - - def test_email_route_has_webmail_extra_subdomain(self): - routes = self.registry.get_caddy_routes() - email_route = next(r for r in routes if r['service_id'] == 'email') - self.assertIn('webmail', email_route['extra_subdomains']) - - def test_files_route_has_webdav_extra_subdomain(self): - routes = self.registry.get_caddy_routes() - files_route = next(r for r in routes if r['service_id'] == 'files') - self.assertIn('webdav', files_route['extra_subdomains']) - def test_services_without_subdomain_excluded(self): self.cm.get_installed_services.return_value = { 'nosubdomain': {'manifest': { @@ -218,6 +253,21 @@ class TestServiceRegistryGetCaddyRoutes(unittest.TestCase): routes = self.registry.get_caddy_routes() self.assertNotIn('nosubdomain', [r['service_id'] for r in routes]) + def test_installed_service_with_subdomain_appears_in_routes(self): + self.cm.get_installed_services.return_value = { + 'calendar': {'manifest': _CALENDAR_MANIFEST}, + } + routes = self.registry.get_caddy_routes() + route_ids = [r['service_id'] for r in routes] + self.assertIn('calendar', route_ids) + cal_route = next(r for r in routes if r['service_id'] == 'calendar') + self.assertEqual(cal_route['subdomain'], 'calendar') + self.assertEqual(cal_route['backend'], 'cell-radicale:5232') + + +# --------------------------------------------------------------------------- +# TestServiceRegistryGetBackupPlan +# --------------------------------------------------------------------------- class TestServiceRegistryGetBackupPlan(unittest.TestCase): @@ -225,37 +275,6 @@ class TestServiceRegistryGetBackupPlan(unittest.TestCase): self.cm = _make_cm() self.registry = ServiceRegistry(self.cm) - def test_all_builtins_in_backup_plan(self): - plan = self.registry.get_backup_plan() - plan_ids = [p['service_id'] for p in plan] - for svc_id in ('email', 'calendar', 'files'): - self.assertIn(svc_id, plan_ids) - - def test_email_backup_includes_maildata_volume(self): - plan = self.registry.get_backup_plan() - email_plan = next(p for p in plan if p['service_id'] == 'email') - names = [v['name'] for v in email_plan['volumes']] - self.assertIn('maildata', names) - vol = next(v for v in email_plan['volumes'] if v['name'] == 'maildata') - self.assertEqual(vol['container'], 'cell-mail') - self.assertEqual(vol['path'], '/var/mail') - - def test_calendar_backup_includes_radicale_volume(self): - plan = self.registry.get_backup_plan() - cal_plan = next(p for p in plan if p['service_id'] == 'calendar') - names = [v['name'] for v in cal_plan['volumes']] - self.assertIn('radicale_data', names) - vol = next(v for v in cal_plan['volumes'] if v['name'] == 'radicale_data') - self.assertEqual(vol['container'], 'cell-radicale') - self.assertEqual(vol['path'], '/data') - - def test_files_backup_includes_both_volumes(self): - plan = self.registry.get_backup_plan() - files_plan = next(p for p in plan if p['service_id'] == 'files') - names = {v['name'] for v in files_plan['volumes']} - self.assertIn('filegator', names) - self.assertIn('files', names) - def test_service_without_storage_excluded(self): self.cm.get_installed_services.return_value = { 'nostorage': {'manifest': { @@ -267,11 +286,29 @@ class TestServiceRegistryGetBackupPlan(unittest.TestCase): plan = self.registry.get_backup_plan() self.assertNotIn('nostorage', [p['service_id'] for p in plan]) + def test_installed_service_with_storage_in_backup_plan(self): + self.cm.get_installed_services.return_value = { + 'calendar': {'manifest': _CALENDAR_MANIFEST}, + } + plan = self.registry.get_backup_plan() + plan_ids = [p['service_id'] for p in plan] + self.assertIn('calendar', plan_ids) + cal_plan = next(p for p in plan if p['service_id'] == 'calendar') + names = [v['name'] for v in cal_plan['volumes']] + self.assertIn('radicale_data', names) + + +# --------------------------------------------------------------------------- +# TestServiceRegistryGetPeerServiceInfo +# --------------------------------------------------------------------------- class TestServiceRegistryGetPeerServiceInfo(unittest.TestCase): def setUp(self): - self.cm = _make_cm({'calendar': {}}) + self.cm = _make_cm( + configs={'calendar': {}}, + installed={'calendar': {'manifest': _CALENDAR_MANIFEST}}, + ) self.registry = ServiceRegistry(self.cm) def test_fills_domain_placeholder(self): @@ -306,41 +343,59 @@ class TestServiceRegistryGetPeerServiceInfo(unittest.TestCase): self.assertIn('legit.example.com', info['caldav_url']) +# --------------------------------------------------------------------------- +# TestServiceRegistryConfigMergeTypeCoercion +# --------------------------------------------------------------------------- + class TestServiceRegistryConfigMergeTypeCoercion(unittest.TestCase): def test_string_in_config_coerced_to_int(self): - cm = _make_cm({'calendar': {'port': '9999'}}) + cm = _make_cm( + configs={'calendar': {'port': '9999'}}, + installed={'calendar': {'manifest': _CALENDAR_MANIFEST}}, + ) reg = ServiceRegistry(cm) svc = reg.get('calendar') self.assertIsInstance(svc['config']['port'], int) self.assertEqual(svc['config']['port'], 9999) def test_unconvertible_value_falls_back_to_default(self): - cm = _make_cm({'calendar': {'port': 'not_a_number'}}) + cm = _make_cm( + configs={'calendar': {'port': 'not_a_number'}}, + installed={'calendar': {'manifest': _CALENDAR_MANIFEST}}, + ) reg = ServiceRegistry(cm) svc = reg.get('calendar') self.assertEqual(svc['config']['port'], 5232) -class TestServiceRegistryWithBrokenManifest(unittest.TestCase): - """Registry must not crash when a manifest file is corrupt or missing.""" +# --------------------------------------------------------------------------- +# TestServiceRegistryRobustness +# --------------------------------------------------------------------------- - def test_missing_builtins_dir_returns_empty(self): - with patch('service_registry._BUILTINS_DIR', '/nonexistent/path'): - reg = ServiceRegistry(_make_cm()) - self.assertEqual(reg.list_all(), []) +class TestServiceRegistryRobustness(unittest.TestCase): + """Registry must not crash when records are corrupt or missing.""" - def test_malformed_json_manifest_skipped(self): - with tempfile.TemporaryDirectory() as tmpdir: - bad_dir = os.path.join(tmpdir, 'bad_svc') - os.makedirs(bad_dir) - with open(os.path.join(bad_dir, 'manifest.json'), 'w') as f: - f.write('this is not json {{{') - with patch('service_registry._BUILTINS_DIR', tmpdir): - reg = ServiceRegistry(_make_cm()) - # Should not raise; just return empty list - result = reg.list_all() - self.assertEqual(result, []) + def test_installed_record_with_no_manifest_skipped(self): + cm = _make_cm(installed={'broken': {}}) + reg = ServiceRegistry(cm) + self.assertIsNone(reg.get('broken')) + + def test_list_all_skips_records_without_id(self): + cm = _make_cm(installed={ + 'noid': {'manifest': { + 'name': 'No ID Service', 'kind': 'store', + 'capabilities': {}, 'config_schema': {}, + # 'id' key intentionally absent + }}, + 'calendar': {'manifest': _CALENDAR_MANIFEST}, + }) + reg = ServiceRegistry(cm) + result = reg.list_all() + ids = [s['id'] for s in result] + self.assertNotIn(None, ids) + self.assertIn('calendar', ids) + self.assertEqual(len(result), 1) if __name__ == '__main__':