From fc3cfc9741e14a9fc15ad8a4e2e600bfa48a9057 Mon Sep 17 00:00:00 2001 From: Dmitrii Iurco Date: Sat, 25 Apr 2026 15:42:03 -0400 Subject: [PATCH] Fix post-deploy auth issues: best-effort service provisioning, integration test auth, test mock corrections - api/app.py: email/calendar/files provisioning now best-effort (non-fatal); fixed email_manager.create_email_user call to include domain argument - tests/integration: added module-level auth sessions to all integration test files; added admin auth to api fixture and _resolve_admin_pass() helper; added TEST_PEER_PASSWORD constant; added password to peer creation calls - tests/test_peer_provisioning.py: renamed rollback test to reflect new best-effort semantics (email failure no longer causes rollback) Co-Authored-By: Claude Sonnet 4.6 --- api/app.py | 56 +++++++------------- tests/integration/conftest.py | 48 ++++++++++++----- tests/integration/test_apply_propagation.py | 22 ++++++-- tests/integration/test_config_api.py | 20 +++++-- tests/integration/test_containers.py | 20 +++++-- tests/integration/test_live_api.py | 18 +++++-- tests/integration/test_negative_scenarios.py | 22 ++++++-- tests/integration/test_network_services.py | 20 +++++-- tests/integration/test_peer_lifecycle.py | 29 +++++++--- tests/test_peer_provisioning.py | 17 +++--- 10 files changed, 184 insertions(+), 88 deletions(-) diff --git a/api/app.py b/api/app.py index 4fdaf5a..410cf0a 100644 --- a/api/app.py +++ b/api/app.py @@ -1845,45 +1845,25 @@ def add_peer(): peer_name = data['name'] - # --- Provision service accounts with rollback on failure --- - provisioned = [] - try: - auth_manager.create_user(peer_name, password, 'peer') - provisioned.append('auth') + # --- Provision auth account (hard-required) --- + if not auth_manager.create_user(peer_name, password, 'peer'): + return jsonify({"error": f"Could not create auth account (duplicate name?)"}), 400 - email_manager.create_email_user(peer_name, password) - provisioned.append('email') - - calendar_manager.create_calendar_user(peer_name, password) - provisioned.append('calendar') - - file_manager.create_user(peer_name, password) - provisioned.append('files') - - except Exception as prov_err: - logger.error(f"Peer provisioning failed at step {provisioned}: {prov_err}") - # Rollback everything provisioned so far - if 'files' in provisioned: - try: - file_manager.delete_user(peer_name) - except Exception: - pass - if 'calendar' in provisioned: - try: - calendar_manager.delete_calendar_user(peer_name) - except Exception: - pass - if 'email' in provisioned: - try: - email_manager.delete_email_user(peer_name) - except Exception: - pass - if 'auth' in provisioned: - try: - auth_manager.delete_user(peer_name) - except Exception: - pass - return jsonify({"error": f"Peer provisioning failed: {prov_err}"}), 500 + # --- Provision service accounts (best-effort; failures logged but non-fatal) --- + provisioned = ['auth'] + domain = _configured_domain() + for step_name, step_fn in [ + ('email', lambda: email_manager.create_email_user(peer_name, domain, password)), + ('calendar', lambda: calendar_manager.create_calendar_user(peer_name, password)), + ('files', lambda: file_manager.create_user(peer_name, password)), + ]: + try: + if step_fn(): + provisioned.append(step_name) + else: + logger.warning(f"Peer {peer_name}: {step_name} account creation returned False (service may not be ready)") + except Exception as e: + logger.warning(f"Peer {peer_name}: {step_name} account creation failed (non-fatal): {e}") # Add peer to registry with all provided fields peer_info = { diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 6ce3cb4..2c8ed45 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -2,10 +2,12 @@ Shared fixtures for live integration tests. Configure with environment variables: - PIC_HOST API host (default: localhost) - PIC_API_PORT API port (default: 3000) - PIC_WEBUI_PORT WebUI port (default: 80) - PIC_WG_CONTAINER WireGuard container name (default: cell-wireguard) + PIC_HOST API host (default: localhost) + PIC_API_PORT API port (default: 3000) + PIC_WEBUI_PORT WebUI port (default: 80) + PIC_WG_CONTAINER WireGuard container name (default: cell-wireguard) + PIC_ADMIN_USER Admin username (default: admin) + PIC_ADMIN_PASS Admin password (default: read from data/api/.admin_initial_password or env) """ import os import json @@ -17,6 +19,8 @@ PIC_HOST = os.environ.get('PIC_HOST', 'localhost') API_PORT = int(os.environ.get('PIC_API_PORT', '3000')) WEBUI_PORT = int(os.environ.get('PIC_WEBUI_PORT', '80')) WG_CONTAINER = os.environ.get('PIC_WG_CONTAINER', 'cell-wireguard') +ADMIN_USER = os.environ.get('PIC_ADMIN_USER', 'admin') +ADMIN_PASS = os.environ.get('PIC_ADMIN_PASS', '') API_BASE = f"http://{PIC_HOST}:{API_PORT}" WEBUI_BASE = f"http://{PIC_HOST}:{WEBUI_PORT}" @@ -28,11 +32,33 @@ TEST_PEERS = ( 'bad-svc-peer', # guard against validation-test leak ) +TEST_PEER_PASSWORD = 'IntegrationTest123!' + + +def _resolve_admin_pass() -> str: + if ADMIN_PASS: + return ADMIN_PASS + # Try reading from the initial password file (present on first run before bootstrap) + candidate = os.path.join( + os.path.dirname(__file__), '..', '..', 'data', 'api', '.admin_initial_password' + ) + candidate = os.path.normpath(candidate) + if os.path.exists(candidate): + return open(candidate).read().strip() + raise RuntimeError( + "Admin password unknown. Set PIC_ADMIN_PASS env var or run make setup first." + ) + @pytest.fixture(scope='session') def api(): + """Authenticated requests.Session logged in as admin.""" s = requests.Session() s.headers['Content-Type'] = 'application/json' + password = _resolve_admin_pass() + r = s.post(f"{API_BASE}/api/auth/login", json={'username': ADMIN_USER, 'password': password}) + if r.status_code != 200: + raise RuntimeError(f"Integration test login failed: {r.status_code} {r.text}") return s @@ -71,17 +97,13 @@ def peer_rules(peer_ip: str) -> list[str]: return [line for line in iptables_forward().splitlines() if comment in line] -def get_live_service_vips() -> dict: +def get_live_service_vips(session: requests.Session = None) -> dict: """ - Read virtual IPs from the config API. - - The config API computes service_ips from the current ip_range at request time, - so it always matches what the running firewall_manager will use when applying - peer rules. Using docker exec on the API container is NOT reliable because - it spawns a fresh Python process that imports firewall_manager with its initial - hardcoded SERVICE_IPS, ignoring any update_service_ips() calls made at runtime. + Read virtual IPs from the config API using an authenticated session. + Falls back to a new unauthenticated request only if no session provided (legacy). """ - cfg = requests.get(f"{API_BASE}/api/config").json() + s = session or requests.Session() + cfg = s.get(f"{API_BASE}/api/config").json() sips = cfg.get('service_ips', {}) return { 'calendar': sips.get('vip_calendar', ''), diff --git a/tests/integration/test_apply_propagation.py b/tests/integration/test_apply_propagation.py index 4de60a0..77b6833 100644 --- a/tests/integration/test_apply_propagation.py +++ b/tests/integration/test_apply_propagation.py @@ -37,7 +37,7 @@ import requests from requests.exceptions import ConnectionError, Timeout sys.path.insert(0, os.path.dirname(__file__)) -from conftest import API_BASE +from conftest import API_BASE, _resolve_admin_pass # --------------------------------------------------------------------------- # Constants @@ -56,20 +56,32 @@ _CAL_PORT_B = 5233 # an alternate safe value used as the "changed" state # Helpers # --------------------------------------------------------------------------- + +_S = None + +@pytest.fixture(scope='module', autouse=True) +def _auth_session(): + global _S + _S = requests.Session() + _S.headers['Content-Type'] = 'application/json' + r = _S.post(f"{API_BASE}/api/auth/login", + json={'username': 'admin', 'password': _resolve_admin_pass()}) + assert r.status_code == 200, f"Login failed: {{r.text}}" + def get(path, **kw): - return requests.get(f"{API_BASE}{path}", **kw) + return _S.get(f"{API_BASE}{path}", **kw) def put(path, **kw): - return requests.put(f"{API_BASE}{path}", **kw) + return _S.put(f"{API_BASE}{path}", **kw) def post(path, **kw): - return requests.post(f"{API_BASE}{path}", **kw) + return _S.post(f"{API_BASE}{path}", **kw) def delete(path, **kw): - return requests.delete(f"{API_BASE}{path}", **kw) + return _S.delete(f"{API_BASE}{path}", **kw) def wait_for_healthy(timeout: int = _HEALTH_TIMEOUT) -> bool: diff --git a/tests/integration/test_config_api.py b/tests/integration/test_config_api.py index ff7249e..7f237e4 100644 --- a/tests/integration/test_config_api.py +++ b/tests/integration/test_config_api.py @@ -16,19 +16,31 @@ import requests import sys import os sys.path.insert(0, os.path.dirname(__file__)) -from conftest import API_BASE +from conftest import API_BASE, _resolve_admin_pass + +_S = None + +@pytest.fixture(scope='module', autouse=True) +def _auth_session(): + global _S + _S = requests.Session() + _S.headers['Content-Type'] = 'application/json' + r = _S.post(f"{API_BASE}/api/auth/login", + json={'username': 'admin', 'password': _resolve_admin_pass()}) + assert r.status_code == 200, f"Login failed: {{r.text}}" + def get(path, **kw): - return requests.get(f"{API_BASE}{path}", **kw) + return _S.get(f"{API_BASE}{path}", **kw) def put(path, **kw): - return requests.put(f"{API_BASE}{path}", **kw) + return _S.put(f"{API_BASE}{path}", **kw) def post(path, **kw): - return requests.post(f"{API_BASE}{path}", **kw) + return _S.post(f"{API_BASE}{path}", **kw) # --------------------------------------------------------------------------- diff --git a/tests/integration/test_containers.py b/tests/integration/test_containers.py index 0dbc244..5463ef4 100644 --- a/tests/integration/test_containers.py +++ b/tests/integration/test_containers.py @@ -19,7 +19,7 @@ import requests import sys import os sys.path.insert(0, os.path.dirname(__file__)) -from conftest import API_BASE +from conftest import API_BASE, _resolve_admin_pass # A non-critical container safe to restart during testing. # cell-ntp has no write-side effects and recovers in seconds. @@ -29,12 +29,24 @@ _SAFE_TO_RESTART = 'cell-ntp' _NONEXISTENT = 'cell-does-not-exist-xyz' + +_S = None + +@pytest.fixture(scope='module', autouse=True) +def _auth_session(): + global _S + _S = requests.Session() + _S.headers['Content-Type'] = 'application/json' + r = _S.post(f"{API_BASE}/api/auth/login", + json={'username': 'admin', 'password': _resolve_admin_pass()}) + assert r.status_code == 200, f"Login failed: {{r.text}}" + def get(path, **kw): - return requests.get(f"{API_BASE}{path}", **kw) + return _S.get(f"{API_BASE}{path}", **kw) def post(path, **kw): - return requests.post(f"{API_BASE}{path}", **kw) + return _S.post(f"{API_BASE}{path}", **kw) # Skip the entire module if the container endpoint is access-denied. @@ -42,7 +54,7 @@ def post(path, **kw): # is_local_request(). Run `make update` to rebuild and re-enable these tests. def _containers_accessible(): try: - return requests.get(f"{API_BASE}/api/containers", timeout=3).status_code != 403 + return _S.get(f"{API_BASE}/api/containers", timeout=3).status_code != 403 except Exception: return False diff --git a/tests/integration/test_live_api.py b/tests/integration/test_live_api.py index 780f25b..b20d042 100644 --- a/tests/integration/test_live_api.py +++ b/tests/integration/test_live_api.py @@ -7,16 +7,28 @@ Or: PIC_HOST=192.168.31.51 pytest tests/integration/test_live_api.py -v import pytest import sys, os sys.path.insert(0, os.path.dirname(__file__)) -from conftest import API_BASE +from conftest import API_BASE, _resolve_admin_pass # Shorthand helpers — always hits the live API import requests as _req + +_S = None + +@pytest.fixture(scope='module', autouse=True) +def _auth_session(): + global _S + _S = requests.Session() + _S.headers['Content-Type'] = 'application/json' + r = _S.post(f"{API_BASE}/api/auth/login", + json={'username': 'admin', 'password': _resolve_admin_pass()}) + assert r.status_code == 200, f"Login failed: {{r.text}}" + def get(path, **kw): - return _req.get(f"{API_BASE}{path}", **kw) + return _S.get(f"{API_BASE}{path}", **kw) def post(path, **kw): - return _req.post(f"{API_BASE}{path}", **kw) + return _S.post(f"{API_BASE}{path}", **kw) # --------------------------------------------------------------------------- diff --git a/tests/integration/test_negative_scenarios.py b/tests/integration/test_negative_scenarios.py index 84f5ee9..40ec7c6 100644 --- a/tests/integration/test_negative_scenarios.py +++ b/tests/integration/test_negative_scenarios.py @@ -22,27 +22,39 @@ import requests import sys import os sys.path.insert(0, os.path.dirname(__file__)) -from conftest import API_BASE +from conftest import API_BASE, _resolve_admin_pass # Sentinel peer name that should never exist in the registry _GHOST_PEER = 'ghost-peer-that-does-not-exist-xyz' _GHOST_CONTAINER = 'cell-container-does-not-exist-xyz' + +_S = None + +@pytest.fixture(scope='module', autouse=True) +def _auth_session(): + global _S + _S = requests.Session() + _S.headers['Content-Type'] = 'application/json' + r = _S.post(f"{API_BASE}/api/auth/login", + json={'username': 'admin', 'password': _resolve_admin_pass()}) + assert r.status_code == 200, f"Login failed: {{r.text}}" + def get(path, **kw): - return requests.get(f"{API_BASE}{path}", **kw) + return _S.get(f"{API_BASE}{path}", **kw) def post(path, **kw): - return requests.post(f"{API_BASE}{path}", **kw) + return _S.post(f"{API_BASE}{path}", **kw) def put(path, **kw): - return requests.put(f"{API_BASE}{path}", **kw) + return _S.put(f"{API_BASE}{path}", **kw) def delete(path, **kw): - return requests.delete(f"{API_BASE}{path}", **kw) + return _S.delete(f"{API_BASE}{path}", **kw) # --------------------------------------------------------------------------- diff --git a/tests/integration/test_network_services.py b/tests/integration/test_network_services.py index 8b331c0..00506ef 100644 --- a/tests/integration/test_network_services.py +++ b/tests/integration/test_network_services.py @@ -13,22 +13,34 @@ import requests import sys import os sys.path.insert(0, os.path.dirname(__file__)) -from conftest import API_BASE +from conftest import API_BASE, _resolve_admin_pass # Test DNS hostname to use — must be cleaned up after tests _TEST_DNS_HOSTNAME = 'inttest-dns-record' +_S: requests.Session = None + + +@pytest.fixture(scope='module', autouse=True) +def _auth_session(): + global _S + _S = requests.Session() + _S.headers['Content-Type'] = 'application/json' + r = _S.post(f"{API_BASE}/api/auth/login", + json={'username': 'admin', 'password': _resolve_admin_pass()}) + assert r.status_code == 200, f"Login failed: {r.text}" + def get(path, **kw): - return requests.get(f"{API_BASE}{path}", **kw) + return _S.get(f"{API_BASE}{path}", **kw) def post(path, **kw): - return requests.post(f"{API_BASE}{path}", **kw) + return _S.post(f"{API_BASE}{path}", **kw) def delete(path, **kw): - return requests.delete(f"{API_BASE}{path}", **kw) + return _S.delete(f"{API_BASE}{path}", **kw) # --------------------------------------------------------------------------- diff --git a/tests/integration/test_peer_lifecycle.py b/tests/integration/test_peer_lifecycle.py index b6a9484..38feed2 100644 --- a/tests/integration/test_peer_lifecycle.py +++ b/tests/integration/test_peer_lifecycle.py @@ -16,24 +16,37 @@ import pytest import requests import sys, os sys.path.insert(0, os.path.dirname(__file__)) -from conftest import API_BASE, peer_rules, iptables_forward, get_live_service_vips +from conftest import API_BASE, peer_rules, iptables_forward, get_live_service_vips, TEST_PEER_PASSWORD, _resolve_admin_pass # Service → virtual IP mapping (mirrors firewall_manager.SERVICE_IPS) ALL_SERVICES = {'calendar', 'files', 'mail', 'webdav'} ALL_PEERS = ('integration-test-full', 'integration-test-restricted', 'integration-test-none') +# Module-level authenticated session — set once by the autouse fixture below +_S: requests.Session = None + + +@pytest.fixture(scope='module', autouse=True) +def _auth_session(): + global _S + _S = requests.Session() + _S.headers['Content-Type'] = 'application/json' + r = _S.post(f"{API_BASE}/api/auth/login", + json={'username': 'admin', 'password': _resolve_admin_pass()}) + assert r.status_code == 200, f"Login failed: {r.text}" + def api_post(path, **kw): - return requests.post(f"{API_BASE}{path}", **kw) + return _S.post(f"{API_BASE}{path}", **kw) def api_get(path, **kw): - return requests.get(f"{API_BASE}{path}", **kw) + return _S.get(f"{API_BASE}{path}", **kw) def api_put(path, **kw): - return requests.put(f"{API_BASE}{path}", **kw) + return _S.put(f"{API_BASE}{path}", **kw) def api_delete(path, **kw): - return requests.delete(f"{API_BASE}{path}", **kw) + return _S.delete(f"{API_BASE}{path}", **kw) # --------------------------------------------------------------------------- @@ -109,6 +122,7 @@ class TestPeerFullAccess: 'name': self.PEER_NAME, 'public_key': keys['public_key'], 'service_access': list(ALL_SERVICES), + 'password': TEST_PEER_PASSWORD, }) assert r.status_code == 201, f"Peer creation failed: {r.text}" data = r.json() @@ -143,8 +157,9 @@ class TestPeerFullAccess: r = api_post('/api/peers', json={ 'name': self.PEER_NAME, 'public_key': keys['public_key'], + 'password': TEST_PEER_PASSWORD, }) - assert r.status_code == 400, "Duplicate peer should be rejected" + assert r.status_code in (400, 409), "Duplicate peer should be rejected" def test_delete_peer_full_access(self): r = api_delete(f'/api/peers/{self.PEER_NAME}') @@ -180,6 +195,7 @@ class TestPeerRestrictedAccess: 'public_key': keys['public_key'], 'service_access': ['calendar'], 'internet_access': False, + 'password': TEST_PEER_PASSWORD, }) assert r.status_code == 201, f"Peer creation failed: {r.text}" @@ -254,6 +270,7 @@ class TestPeerNoAccess: 'service_access': [], 'internet_access': False, 'peer_access': False, + 'password': TEST_PEER_PASSWORD, }) assert r.status_code == 201, f"Peer creation failed: {r.text}" diff --git a/tests/test_peer_provisioning.py b/tests/test_peer_provisioning.py index b711265..ba323f5 100644 --- a/tests/test_peer_provisioning.py +++ b/tests/test_peer_provisioning.py @@ -221,10 +221,11 @@ def test_create_peer_requires_public_key(admin_client): # ── POST /api/peers — rollback on failure ───────────────────────────────────── -def test_create_peer_rollback_on_email_failure( +def test_create_peer_email_failure_is_nonfatal( auth_mgr, mock_email_mgr, mock_calendar_mgr, mock_file_mgr, mock_wg_mgr, mock_peer_registry): - """If email_manager.create_email_user raises, auth user must be deleted (rollback).""" + """Email provisioning is best-effort: if create_email_user raises, peer creation + still succeeds (201) and the auth user is NOT rolled back.""" mock_email_mgr.create_email_user.side_effect = RuntimeError('SMTP server down') app.config['TESTING'] = True @@ -252,11 +253,15 @@ def test_create_peer_rollback_on_email_failure( with app.test_client() as client: r = _login(client) assert r.status_code == 200 - _post_peer(client) - # alice must not remain in the auth store (rolled back) + resp = _post_peer(client) + # Peer creation must succeed despite email failure (best-effort) + assert resp.status_code == 201, ( + f'expected 201 but got {resp.status_code}: {resp.data}' + ) + # Auth user must remain — no rollback for non-fatal service failures alice = auth_mgr.get_user('alice') - assert alice is None, ( - 'auth user alice was not rolled back after email_manager failure' + assert alice is not None, ( + 'auth user alice was incorrectly rolled back after non-fatal email failure' ) finally: for p in patches: