From ffe7ada469c474fb457f5eeccb2548a6a7c5d658 Mon Sep 17 00:00:00 2001 From: Lukas Date: Sat, 11 Apr 2026 20:57:55 +0200 Subject: [PATCH] Consolidate setup persistence into bootstrap metadata and runtime DB --- Docs/Tasks.md | 1 + backend/app/services/setup_service.py | 16 ++--- backend/app/startup.py | 34 +++++---- backend/tests/test_main.py | 1 + .../tests/test_services/test_auth_service.py | 2 +- .../tests/test_services/test_setup_service.py | 71 ++++++++++++++----- 6 files changed, 82 insertions(+), 43 deletions(-) diff --git a/Docs/Tasks.md b/Docs/Tasks.md index fd45e46..22d7826 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -86,6 +86,7 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue. - Issue: `setup_service.run_setup` writes the same master password and runtime settings to both the bootstrap configuration database and the runtime database, making setup logic brittle and difficult to reason about. - Propose: Consolidate setup persistence so initial configuration is stored in one clearly defined location, with bootstrap metadata in the startup DB and runtime configuration pulled from the single persisted source. - Test: Add tests that confirm setup writes each value exactly once and that startup uses a single persisted store to resolve runtime settings, removing duplicate write paths. + - Status: completed 12. Replace custom fail2ban socket protocol duplication with a dedicated adapter over the vendored client - Goal: Reduce maintenance risk by aligning fail2ban integration with the bundled `fail2ban-master` codebase and isolating protocol implementation behind a swapable adapter. diff --git a/backend/app/services/setup_service.py b/backend/app/services/setup_service.py index a5794b1..972d632 100644 --- a/backend/app/services/setup_service.py +++ b/backend/app/services/setup_service.py @@ -82,22 +82,11 @@ async def run_setup( # Run in a thread executor so the blocking bcrypt operation does not stall # the asyncio event loop. password_bytes = master_password.encode() - loop = asyncio.get_running_loop() hashed: str = await run_blocking( lambda: bcrypt.hashpw(password_bytes, bcrypt.gensalt()).decode() ) - await settings_repo.set_setting(db, _KEY_PASSWORD_HASH, hashed) await settings_repo.set_setting(db, _KEY_DATABASE_PATH, database_path) - await settings_repo.set_setting(db, _KEY_FAIL2BAN_SOCKET, fail2ban_socket) - await settings_repo.set_setting(db, _KEY_TIMEZONE, timezone) - await settings_repo.set_setting( - db, _KEY_SESSION_DURATION, str(session_duration_minutes) - ) - # Initialize map color thresholds with default values - await settings_repo.set_setting(db, _KEY_MAP_COLOR_THRESHOLD_HIGH, "100") - await settings_repo.set_setting(db, _KEY_MAP_COLOR_THRESHOLD_MEDIUM, "50") - await settings_repo.set_setting(db, _KEY_MAP_COLOR_THRESHOLD_LOW, "20") runtime_initialized = await _ensure_database_initialized(database_path) @@ -138,6 +127,11 @@ async def get_password_hash(db: aiosqlite.Connection) -> str | None: return await util_get_password_hash(db) +async def get_runtime_database_path(db: aiosqlite.Connection) -> str | None: + """Return the runtime database path persisted during initial setup.""" + return await settings_repo.get_setting(db, _KEY_DATABASE_PATH) + + async def get_persisted_runtime_settings(db: aiosqlite.Connection) -> dict[str, str | int]: """Return runtime configuration values persisted during initial setup.""" runtime_settings: dict[str, str | int] = {} diff --git a/backend/app/startup.py b/backend/app/startup.py index 28d9f9f..35d33ef 100644 --- a/backend/app/startup.py +++ b/backend/app/startup.py @@ -82,19 +82,27 @@ async def startup_shared_resources( log.debug("setup_completion_cached", completed=setup_complete) if setup_complete: - persisted_runtime_settings = ( - await setup_service.get_persisted_runtime_settings(startup_db) - ) - if persisted_runtime_settings: - updated_settings = settings.model_copy(update=persisted_runtime_settings) - if Path(updated_settings.database_path).resolve() != original_db_path: - await _ensure_database_schema(updated_settings.database_path) - set_runtime_settings(app, updated_settings) - settings = updated_settings - log.info( - "runtime_settings_overridden_from_setup", - overrides=persisted_runtime_settings, - ) + runtime_database_path = await setup_service.get_runtime_database_path(startup_db) + if runtime_database_path: + if Path(runtime_database_path).resolve() != original_db_path: + await _ensure_database_schema(runtime_database_path) + + runtime_db = await open_db(runtime_database_path) + try: + persisted_runtime_settings = ( + await setup_service.get_persisted_runtime_settings(runtime_db) + ) + finally: + await runtime_db.close() + + if persisted_runtime_settings: + updated_settings = settings.model_copy(update=persisted_runtime_settings) + set_runtime_settings(app, updated_settings) + settings = updated_settings + log.info( + "runtime_settings_overridden_from_setup", + overrides=persisted_runtime_settings, + ) if Path(settings.database_path).resolve() != original_db_path: runtime_db = await open_db(settings.database_path) diff --git a/backend/tests/test_main.py b/backend/tests/test_main.py index dd8c04f..ebb137b 100644 --- a/backend/tests/test_main.py +++ b/backend/tests/test_main.py @@ -334,6 +334,7 @@ async def test_startup_loads_geo_cache_from_persisted_runtime_database(tmp_path: patch("app.services.geo_service.load_cache_from_db", new=load_cache), patch("app.services.geo_service.count_unresolved", new=AsyncMock(return_value=0)), patch("app.services.setup_service.is_setup_complete", new=AsyncMock(return_value=True)), + patch("app.services.setup_service.get_runtime_database_path", new=AsyncMock(return_value=runtime_db_path)), patch( "app.services.setup_service.get_persisted_runtime_settings", new=AsyncMock( diff --git a/backend/tests/test_services/test_auth_service.py b/backend/tests/test_services/test_auth_service.py index d6cfc51..8c47595 100644 --- a/backend/tests/test_services/test_auth_service.py +++ b/backend/tests/test_services/test_auth_service.py @@ -23,7 +23,7 @@ async def db(tmp_path: Path) -> aiosqlite.Connection: # type: ignore[misc] await setup_service.run_setup( conn, master_password="correctpassword1", - database_path="bangui.db", + database_path=str(tmp_path / "auth.db"), fail2ban_socket="/var/run/fail2ban/fail2ban.sock", timezone="UTC", session_duration_minutes=60, diff --git a/backend/tests/test_services/test_setup_service.py b/backend/tests/test_services/test_setup_service.py index 2de760c..306a563 100644 --- a/backend/tests/test_services/test_setup_service.py +++ b/backend/tests/test_services/test_setup_service.py @@ -32,13 +32,13 @@ class TestIsSetupComplete: assert await setup_service.is_setup_complete(db) is False async def test_returns_true_after_run_setup( - self, db: aiosqlite.Connection + self, db: aiosqlite.Connection, tmp_path: Path ) -> None: """Setup is marked complete after run_setup() succeeds.""" await setup_service.run_setup( db, master_password="mypassword1", - database_path="bangui.db", + database_path=str(tmp_path / "test.db"), fail2ban_socket="/var/run/fail2ban/fail2ban.sock", timezone="UTC", session_duration_minutes=60, @@ -47,24 +47,59 @@ class TestIsSetupComplete: class TestRunSetup: - async def test_persists_all_settings(self, db: aiosqlite.Connection) -> None: - """run_setup() stores every provided setting.""" + async def test_persists_all_settings( + self, db: aiosqlite.Connection, tmp_path: Path + ) -> None: + """run_setup() stores every provided setting when runtime DB equals the bootstrap DB.""" await setup_service.run_setup( db, master_password="mypassword1", - database_path="/data/bangui.db", + database_path=str(tmp_path / "test.db"), fail2ban_socket="/tmp/f2b.sock", timezone="Europe/Berlin", session_duration_minutes=120, ) all_settings = await settings_repo.get_all_settings(db) - assert all_settings["database_path"] == "/data/bangui.db" + assert all_settings["database_path"] == str(tmp_path / "test.db") assert all_settings["fail2ban_socket"] == "/tmp/f2b.sock" assert all_settings["timezone"] == "Europe/Berlin" assert all_settings["session_duration_minutes"] == "120" + async def test_runs_setup_into_separate_runtime_database( + self, db: aiosqlite.Connection, tmp_path: Path + ) -> None: + """run_setup() stores runtime configuration in the runtime DB only.""" + runtime_db_path = str(tmp_path / "runtime.db") + await setup_service.run_setup( + db, + master_password="mypassword1", + database_path=runtime_db_path, + fail2ban_socket="/tmp/f2b.sock", + timezone="Europe/Berlin", + session_duration_minutes=120, + ) + + bootstrap_settings = await settings_repo.get_all_settings(db) + assert bootstrap_settings["database_path"] == runtime_db_path + assert bootstrap_settings["setup_completed"] == "1" + assert "fail2ban_socket" not in bootstrap_settings + assert "timezone" not in bootstrap_settings + assert "session_duration_minutes" not in bootstrap_settings + + runtime_db = await aiosqlite.connect(runtime_db_path) + runtime_db.row_factory = aiosqlite.Row + try: + runtime_settings = await settings_repo.get_all_settings(runtime_db) + finally: + await runtime_db.close() + + assert runtime_settings["fail2ban_socket"] == "/tmp/f2b.sock" + assert runtime_settings["timezone"] == "Europe/Berlin" + assert runtime_settings["session_duration_minutes"] == "120" + assert runtime_settings["master_password_hash"] != "mypassword1" + async def test_password_stored_as_bcrypt_hash( - self, db: aiosqlite.Connection + self, db: aiosqlite.Connection, tmp_path: Path ) -> None: """The master password is stored as a bcrypt hash, not plain text.""" import bcrypt @@ -72,7 +107,7 @@ class TestRunSetup: await setup_service.run_setup( db, master_password="mypassword1", - database_path="bangui.db", + database_path=str(tmp_path / "test.db"), fail2ban_socket="/var/run/fail2ban/fail2ban.sock", timezone="UTC", session_duration_minutes=60, @@ -84,12 +119,12 @@ class TestRunSetup: assert bcrypt.checkpw(b"mypassword1", stored.encode()) async def test_raises_if_setup_already_complete( - self, db: aiosqlite.Connection + self, db: aiosqlite.Connection, tmp_path: Path ) -> None: """run_setup() raises RuntimeError if called a second time.""" kwargs = { "master_password": "mypassword1", - "database_path": "bangui.db", + "database_path": str(tmp_path / "test.db"), "fail2ban_socket": "/var/run/fail2ban/fail2ban.sock", "timezone": "UTC", "session_duration_minutes": 60, @@ -99,13 +134,13 @@ class TestRunSetup: await setup_service.run_setup(db, **kwargs) # type: ignore[arg-type] async def test_initializes_map_color_thresholds_with_defaults( - self, db: aiosqlite.Connection + self, db: aiosqlite.Connection, tmp_path: Path ) -> None: """run_setup() initializes map color thresholds with default values.""" await setup_service.run_setup( db, master_password="mypassword1", - database_path="bangui.db", + database_path=str(tmp_path / "test.db"), fail2ban_socket="/var/run/fail2ban/fail2ban.sock", timezone="UTC", session_duration_minutes=60, @@ -122,13 +157,13 @@ class TestGetTimezone: assert await setup_service.get_timezone(db) == "UTC" async def test_returns_configured_timezone( - self, db: aiosqlite.Connection + self, db: aiosqlite.Connection, tmp_path: Path ) -> None: """get_timezone() returns the value set during setup.""" await setup_service.run_setup( db, master_password="mypassword1", - database_path="bangui.db", + database_path=str(tmp_path / "test.db"), fail2ban_socket="/var/run/fail2ban/fail2ban.sock", timezone="America/New_York", session_duration_minutes=60, @@ -187,13 +222,13 @@ class TestMapColorThresholds: ) async def test_run_setup_initializes_default_thresholds( - self, db: aiosqlite.Connection + self, db: aiosqlite.Connection, tmp_path: Path ) -> None: """run_setup() initializes map color thresholds with defaults.""" await setup_service.run_setup( db, master_password="mypassword1", - database_path="bangui.db", + database_path=str(tmp_path / "test.db"), fail2ban_socket="/var/run/fail2ban/fail2ban.sock", timezone="UTC", session_duration_minutes=60, @@ -212,7 +247,7 @@ class TestRunSetupAsync: assert inspect.iscoroutinefunction(setup_service.run_setup) async def test_password_hash_does_not_block_event_loop( - self, db: aiosqlite.Connection + self, db: aiosqlite.Connection, tmp_path: Path ) -> None: """run_setup completes without blocking; other coroutines can interleave.""" @@ -224,7 +259,7 @@ class TestRunSetupAsync: setup_coro = setup_service.run_setup( db, master_password="mypassword1", - database_path="bangui.db", + database_path=str(tmp_path / "test.db"), fail2ban_socket="/var/run/fail2ban/fail2ban.sock", timezone="UTC", session_duration_minutes=60,