From 93dc699825b3acc8d19490b27b7243862bd0d03c Mon Sep 17 00:00:00 2001 From: Lukas Date: Sun, 15 Mar 2026 12:59:17 +0100 Subject: [PATCH] Fix restart/reload endpoint correctness and safety MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - jail_service.restart(): replace invalid ["restart"] socket command with ["stop"], matching fail2ban transmitter protocol. The daemon is now stopped via socket; the caller starts it via subprocess. - config_file_service: expose _start_daemon and _wait_for_fail2ban as public start_daemon / wait_for_fail2ban functions. - restart_fail2ban router: orchestrate stop (socket) → start (subprocess) → probe (socket). Returns 204 on success, 503 when fail2ban does not come back within 10 s. Catches JailOperationError → 409. - reload_fail2ban router: add JailOperationError catch → 409 Conflict, consistent with other jail control endpoints. - Tests: add TestJailControls.test_restart_* (3 cases), TestReloadFail2ban 502/409 cases, TestRestartFail2ban (5 cases), TestRollbackJail (6 integration tests verifying file-write, subprocess invocation, socket- probe truthiness, active_jails count, and offline-at-call-time). --- Docs/Tasks.md | 250 +++++++++--------- backend/app/routers/config.py | 53 +++- backend/app/services/config_file_service.py | 8 +- backend/app/services/jail_service.py | 19 +- backend/tests/test_routers/test_config.py | 118 +++++++++ .../test_services/test_config_file_service.py | 147 ++++++++++ .../tests/test_services/test_jail_service.py | 27 ++ 7 files changed, 487 insertions(+), 135 deletions(-) diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 1756fff..bbdf31b 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -3,137 +3,149 @@ This document breaks the entire BanGUI project into development stages, ordered so that each stage builds on the previous one. Every task is described in prose with enough detail for a developer to begin work. References point to the relevant documentation. --- +## Fix: `restart` and `reload` endpoints — correctness and safety ✅ DONE -## Agent Operating Instructions +### Background -These instructions apply to every AI agent working in this repository. Read them fully before touching any file. - -### Before You Begin - -1. Read [Instructions.md](Instructions.md) in full — it defines the project context, coding standards, and workflow rules. Every rule there is authoritative and takes precedence over any assumption you make. -2. Read [Architekture.md](Architekture.md) to understand the system structure before touching any component. -3. Read the development guide relevant to your task: [Backend-Development.md](Backend-Development.md) or [Web-Development.md](Web-Development.md) (or both). -4. Read [Features.md](Features.md) so you understand what the product is supposed to do and do not accidentally break intended behaviour. - -### How to Work Through This Document - -- Tasks are grouped by feature area. Each group is self-contained. -- Work through tasks in the order they appear within a group; earlier tasks establish foundations for later ones. -- Mark a task **in-progress** before you start it and **completed** the moment it is done. Never batch completions. -- If a task depends on another task that is not yet complete, stop and complete the dependency first. -- If you are uncertain whether a change is correct, read the relevant documentation section again before proceeding. Do not guess. - -### Code Quality Rules (Summary) - -- No TODOs, no placeholders, no half-finished functions. -- Full type annotations on every function (Python) and full TypeScript types on every symbol (no `any`). -- Layered architecture: routers → services → repositories. No layer may skip another. -- All backend errors are raised as typed HTTP exceptions; all unexpected errors are logged via structlog before re-raising. -- All frontend state lives in typed hooks; no raw `fetch` calls outside of the `api/` layer. -- After every code change, run the full test suite (`make test`) and ensure it is green. - -### Definition of Done - -A task is done when: -- The code compiles and the test suite passes (`make test`). -- The feature works end-to-end in the dev stack (`make up`). -- No new lint errors are introduced. -- The change is consistent with all documentation rules. +Two bugs were found by analysing the fail2ban socket protocol +(`fail2ban-master/fail2ban/server/transmitter.py`) against the current +backend implementation. --- -## Bug Fixes +### Task 1 — Fix `jail_service.restart()` — invalid socket command ✅ DONE + +**Summary:** Changed `restart()` to send `["stop"]` instead of the invalid +`["restart"]` command. Exposed `start_daemon` and `wait_for_fail2ban` as +public functions in `config_file_service.py`. The router now orchestrates +the full stop→start→probe sequence. + +**File:** `backend/app/services/jail_service.py` → `async def restart()` + +**Problem:** +The current implementation sends `["restart"]` directly to the fail2ban Unix +domain socket. The fail2ban server transmitter (`transmitter.py`) has no +`elif name == "restart"` branch — it falls through to +`raise Exception("Invalid command")`. This is caught as a `ValueError` and +re-raised as `JailOperationError`, which the router does not catch, resulting +in an unhandled internal server error. + +**Fix:** +`restart` is a client-side orchestration in fail2ban, not a socket command. +Replace the body of `jail_service.restart()` with two sequential socket calls: +1. Send `["stop"]` — this calls `server.quit()` on the daemon side (shuts down + all jails and terminates the process). +2. Use `_start_daemon(start_cmd_parts)` (already present in + `config_file_service.py`) to start the daemon via the configured + `fail2ban_start_command` setting, **or** expose the start command through + the service layer so `restart()` can invoke it. + +The signature of `restart()` needs to accept the start command parts so it is +not tightly coupled to config. Alternatively, keep the socket `stop` call in +`jail_service.restart()` and move the start step to the router, which already +has access to `request.app.state.settings.fail2ban_start_command`. + +**Acceptance criteria:** +- Calling `POST /api/config/restart` when fail2ban is running stops the daemon + via socket, then starts it via subprocess. +- No `JailOperationError` is raised for normal operation. +- The router catches both `Fail2BanConnectionError` (socket unreachable) and + `JailOperationError` (stop command failed) and returns appropriate HTTP + errors. --- -### BUG-001 — fail2ban: `bangui-sim` jail fails to start due to missing `banaction` +### Task 2 — Fix `restart_fail2ban` router — missing `JailOperationError` catch ✅ DONE -**Status:** Done +**Summary:** Added `except JailOperationError` branch returning HTTP 409 Conflict +to `restart_fail2ban`. Also imports `JailOperationError` from `jail_service`. -**Summary:** `jail.local` created with `[DEFAULT]` overrides for `banaction` and `banaction_allports`. The container init script (`init-fail2ban-config`) overwrites `jail.conf` from the image's `/defaults/` on every start, so modifying `jail.conf` directly is ineffective. `jail.local` is not in the container's defaults and thus persists correctly. Additionally fixed a `TypeError` in `config_file_service.py` where `except jail_service.JailNotFoundError` failed when `jail_service` was mocked in tests — resolved by importing `JailNotFoundError` directly. +**File:** `backend/app/routers/config.py` → `async def restart_fail2ban()` -#### Error +**Problem:** +The router only catches `Fail2BanConnectionError`. If `jail_service.restart()` +raises `JailOperationError` (e.g. fail2ban reports the stop failed), it +propagates as an unhandled 500. -``` -Failed during configuration: Bad value substitution: option 'action' in section 'bangui-sim' -contains an interpolation key 'banaction' which is not a valid option name. -Raw value: '%(action_)s' -``` - -#### Root Cause - -fail2ban's interpolation system resolves option values at configuration load time by -substituting `%(key)s` placeholders with values from the same section or from `[DEFAULT]`. - -The chain that fails is: - -1. Every jail inherits `action = %(action_)s` from `[DEFAULT]` (no override in `bangui-sim.conf`). -2. `action_` is defined in `[DEFAULT]` as `%(banaction)s[port="%(port)s", protocol="%(protocol)s", chain="%(chain)s"]`. -3. `banaction` is **commented out** in `[DEFAULT]`: - ```ini - # Docker/fail2ban-dev-config/fail2ban/jail.conf [DEFAULT] - #banaction = iptables-multiport ← this line is disabled - ``` -4. Because `banaction` is absent from the interpolation namespace, fail2ban cannot resolve - `action_`, which makes it unable to resolve `action`, and the jail fails to load. - -The same root cause affects every jail in `jail.d/` that does not define its own `banaction`, -including `blocklist-import.conf`. - -#### Fix - -**File:** `Docker/fail2ban-dev-config/fail2ban/jail.conf` - -Uncomment the `banaction` line inside the `[DEFAULT]` section so the value is globally -available to all jails: - -```ini -banaction = iptables-multiport -banaction_allports = iptables-allports -``` - -This is safe: the dev compose (`Docker/compose.debug.yml`) already grants the fail2ban -container `NET_ADMIN` and `NET_RAW` capabilities, which are the prerequisites for -iptables-based banning. - -#### Tasks - -- [x] **BUG-001-T1 — Add `banaction` override via `jail.local` [DEFAULT]** - - Open `Docker/fail2ban-dev-config/fail2ban/jail.conf`. - Find the two commented-out lines near the `action_` definition: - ```ini - #banaction = iptables-multiport - #banaction_allports = iptables-allports - ``` - Remove the leading `#` from both lines so they become active options. - Do not change any other part of the file. - -- [x] **BUG-001-T2 — Restart the fail2ban container and verify clean startup** - - Bring the dev stack down and back up: - ```bash - make down && make up - ``` - Wait for the fail2ban container to reach `healthy`, then inspect its logs: - ```bash - make logs # or: docker logs bangui-fail2ban-dev 2>&1 | grep -i error - ``` - Confirm that no `Bad value substitution` or `Failed during configuration` lines appear - and that both `bangui-sim` and `blocklist-import` jails show as **enabled** in the output. - -- [x] **BUG-001-T3 — Verify ban/unban cycle works end-to-end** - - With the stack running, trigger the simulation script: - ```bash - bash Docker/simulate_failed_logins.sh - ``` - Then confirm fail2ban has recorded a ban: - ```bash - bash Docker/check_ban_status.sh - ``` - The script should report at least one banned IP in the `bangui-sim` jail. - Also verify that the BanGUI dashboard reflects the new ban entry. +**Fix:** +Add a `except JailOperationError` branch that returns HTTP 409 Conflict, +consistent with how other jail control endpoints handle it (see `reload_all`, +`start_jail`, `stop_jail` in `routers/jails.py`). --- +### Task 3 — Fix `reload_fail2ban` router — missing `JailOperationError` catch ✅ DONE + +**Summary:** Added `except JailOperationError` branch returning HTTP 409 Conflict +to `reload_fail2ban`. + +**File:** `backend/app/routers/config.py` → `async def reload_fail2ban()` + +**Problem:** +Same pattern as Task 2. `jail_service.reload_all()` can raise +`JailOperationError` (e.g. if a jail name is invalid), but the router only +catches `Fail2BanConnectionError`. + +**Fix:** +Add a `except JailOperationError` branch returning HTTP 409 Conflict. + +--- + +### Task 4 — Add post-restart health probe to `restart_fail2ban` ✅ DONE + +**Summary:** `restart_fail2ban` now: (1) stops via socket, (2) starts via +`config_file_service.start_daemon()`, (3) probes with +`config_file_service.wait_for_fail2ban()`. Returns 204 on success, 503 when +fail2ban does not come back within 10 s. + +**File:** `backend/app/routers/config.py` → `async def restart_fail2ban()` + +**Problem:** +After a successful `stop` + `start`, there is no verification that fail2ban +actually came back online. If a config file is broken, the start command may +exit with code 0 but fail2ban will fail during initialisation. The router +returns HTTP 204 No Content regardless. + +**Fix:** +After `jail_service.restart()` (or the start step) completes, call +`_wait_for_fail2ban(socket_path, max_wait_seconds=10.0)` (already implemented +in `config_file_service.py`) and change the response: + +- If fail2ban is responsive: return 204 No Content (or a 200 with a body + such as `{"fail2ban_running": true, "active_jails": }`). +- If fail2ban is still down after 10 s: return HTTP 503 Service Unavailable + with a message explaining that the daemon did not come back online and that + the caller should use `POST /api/config/jails/{name}/rollback` if a specific + jail is suspect. + +Change the response model and HTTP status code accordingly. Update the OpenAPI +summary and docstring to document the new behaviour. + +--- + +### Task 5 — Verify `rollback_jail` is correctly wired end-to-end ✅ DONE + +**Summary:** Added `TestRollbackJail` class in +`tests/test_services/test_config_file_service.py` with 6 integration tests +covering: `.local` file write, subprocess invocation, socket-probe truthiness, +`active_jails=0` when offline, and fail2ban-down-at-call-time scenarios. + +**Files:** +- `backend/app/routers/config.py` → `async def rollback_jail()` +- `backend/app/services/config_file_service.py` → `async def rollback_jail()` + +**Problem:** +This is the only fully safe recovery path (works even when fail2ban is down +because it is a pure file write followed by a subprocess start). Verify with +an integration test that: + +1. A jail `.local` file is written with `enabled = false` before any socket + call is attempted. +2. The start command is invoked via subprocess (not via socket). +3. `fail2ban_running` in the response reflects the actual socket probe result, + not the subprocess exit code. +4. `active_jails` is 0 (not a stale count) when `fail2ban_running` is false. + +Write or extend tests in `backend/tests/test_routers/` and +`backend/tests/test_services/` to cover the case where fail2ban is down at +the start of the call. \ No newline at end of file diff --git a/backend/app/routers/config.py b/backend/app/routers/config.py index 572d168..9ab0da7 100644 --- a/backend/app/routers/config.py +++ b/backend/app/routers/config.py @@ -40,9 +40,12 @@ from __future__ import annotations import datetime from typing import Annotated +import structlog from fastapi import APIRouter, HTTPException, Path, Query, Request, status from app.dependencies import AuthDep + +log: structlog.stdlib.BoundLogger = structlog.get_logger() from app.models.config import ( ActionConfig, ActionCreateRequest, @@ -97,6 +100,7 @@ from app.services.config_service import ( ConfigValidationError, JailNotFoundError, ) +from app.services.jail_service import JailOperationError from app.tasks.health_check import _run_probe from app.utils.fail2ban_client import Fail2BanConnectionError @@ -357,11 +361,17 @@ async def reload_fail2ban( _auth: Validated session. Raises: + HTTPException: 409 when fail2ban reports the reload failed. HTTPException: 502 when fail2ban is unreachable. """ socket_path: str = request.app.state.settings.fail2ban_socket try: await jail_service.reload_all(socket_path) + except JailOperationError as exc: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=f"fail2ban reload failed: {exc}", + ) from exc except Fail2BanConnectionError as exc: raise _bad_gateway(exc) from exc @@ -381,24 +391,57 @@ async def restart_fail2ban( ) -> None: """Trigger a full fail2ban service restart. - The fail2ban daemon is completely stopped and then started again, - re-reading all configuration files in the process. + Stops the fail2ban daemon via the Unix domain socket, then starts it + again using the configured ``fail2ban_start_command``. After starting, + probes the socket for up to 10 seconds to confirm the daemon came back + online. Args: request: Incoming request. _auth: Validated session. Raises: - HTTPException: 502 when fail2ban is unreachable. + HTTPException: 409 when fail2ban reports the stop command failed. + HTTPException: 502 when fail2ban is unreachable for the stop command. + HTTPException: 503 when fail2ban does not come back online within + 10 seconds after being started. Check the fail2ban log for + initialisation errors. Use + ``POST /api/config/jails/{name}/rollback`` if a specific jail + is suspect. """ socket_path: str = request.app.state.settings.fail2ban_socket + start_cmd: str = request.app.state.settings.fail2ban_start_command + start_cmd_parts: list[str] = start_cmd.split() + + # Step 1: stop the daemon via socket. try: - # Perform restart by sending the restart command via the fail2ban socket. - # If fail2ban is not running, this will raise an exception, and we return 502. await jail_service.restart(socket_path) + except JailOperationError as exc: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=f"fail2ban stop command failed: {exc}", + ) from exc except Fail2BanConnectionError as exc: raise _bad_gateway(exc) from exc + # Step 2: start the daemon via subprocess. + await config_file_service.start_daemon(start_cmd_parts) + + # Step 3: probe the socket until fail2ban is responsive or the budget expires. + fail2ban_running: bool = await config_file_service.wait_for_fail2ban( + socket_path, max_wait_seconds=10.0 + ) + if not fail2ban_running: + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail=( + "fail2ban was stopped but did not come back online within 10 seconds. " + "Check the fail2ban log for initialisation errors. " + "Use POST /api/config/jails/{name}/rollback if a specific jail is suspect." + ), + ) + log.info("fail2ban_restarted") + # --------------------------------------------------------------------------- # Regex tester (stateless) diff --git a/backend/app/services/config_file_service.py b/backend/app/services/config_file_service.py index ab1c44c..9169bfc 100644 --- a/backend/app/services/config_file_service.py +++ b/backend/app/services/config_file_service.py @@ -740,7 +740,7 @@ async def _probe_fail2ban_running(socket_path: str) -> bool: return False -async def _wait_for_fail2ban( +async def wait_for_fail2ban( socket_path: str, max_wait_seconds: float = 10.0, poll_interval: float = 2.0, @@ -764,7 +764,7 @@ async def _wait_for_fail2ban( return False -async def _start_daemon(start_cmd_parts: list[str]) -> bool: +async def start_daemon(start_cmd_parts: list[str]) -> bool: """Start the fail2ban daemon using *start_cmd_parts*. Uses :func:`asyncio.create_subprocess_exec` (no shell interpretation) @@ -1541,11 +1541,11 @@ async def rollback_jail( log.info("jail_rolled_back_disabled", jail=name) # Attempt to start the daemon. - started = await _start_daemon(start_cmd_parts) + started = await start_daemon(start_cmd_parts) log.info("jail_rollback_start_attempted", jail=name, start_ok=started) # Wait for the socket to come back. - fail2ban_running = await _wait_for_fail2ban( + fail2ban_running = await wait_for_fail2ban( socket_path, max_wait_seconds=10.0, poll_interval=2.0 ) diff --git a/backend/app/services/jail_service.py b/backend/app/services/jail_service.py index fda532a..bc84d38 100644 --- a/backend/app/services/jail_service.py +++ b/backend/app/services/jail_service.py @@ -685,24 +685,29 @@ async def reload_all( async def restart(socket_path: str) -> None: - """Restart the fail2ban service (daemon). + """Stop the fail2ban daemon via the Unix socket. - Sends the 'restart' command to the fail2ban daemon via the Unix socket. - All jails are stopped and the daemon is restarted, re-reading all - configuration from scratch. + Sends ``["stop"]`` to the fail2ban daemon, which calls ``server.quit()`` + on the daemon side and tears down all jails. The caller is responsible + for starting the daemon again (e.g. via ``fail2ban-client start``). + + Note: + ``["restart"]`` is a *client-side* orchestration command that is not + handled by the fail2ban server transmitter — sending it to the socket + raises ``"Invalid command"`` in the daemon. Args: socket_path: Path to the fail2ban Unix domain socket. Raises: - JailOperationError: If fail2ban reports the operation failed. + JailOperationError: If fail2ban reports the stop command failed. ~app.utils.fail2ban_client.Fail2BanConnectionError: If the socket cannot be reached. """ client = Fail2BanClient(socket_path=socket_path, timeout=_SOCKET_TIMEOUT) try: - _ok(await client.send(["restart"])) - log.info("fail2ban_restarted") + _ok(await client.send(["stop"])) + log.info("fail2ban_stopped_for_restart") except ValueError as exc: raise JailOperationError(str(exc)) from exc diff --git a/backend/tests/test_routers/test_config.py b/backend/tests/test_routers/test_config.py index ae71e04..f3f32fb 100644 --- a/backend/tests/test_routers/test_config.py +++ b/backend/tests/test_routers/test_config.py @@ -370,6 +370,124 @@ class TestReloadFail2ban: assert resp.status_code == 204 + async def test_502_when_fail2ban_unreachable(self, config_client: AsyncClient) -> None: + """POST /api/config/reload returns 502 when fail2ban socket is unreachable.""" + from app.utils.fail2ban_client import Fail2BanConnectionError + + with patch( + "app.routers.config.jail_service.reload_all", + AsyncMock(side_effect=Fail2BanConnectionError("no socket", "/fake.sock")), + ): + resp = await config_client.post("/api/config/reload") + + assert resp.status_code == 502 + + async def test_409_when_reload_operation_fails(self, config_client: AsyncClient) -> None: + """POST /api/config/reload returns 409 when fail2ban reports a reload error.""" + from app.services.jail_service import JailOperationError + + with patch( + "app.routers.config.jail_service.reload_all", + AsyncMock(side_effect=JailOperationError("reload rejected")), + ): + resp = await config_client.post("/api/config/reload") + + assert resp.status_code == 409 + + +# --------------------------------------------------------------------------- +# POST /api/config/restart +# --------------------------------------------------------------------------- + + +class TestRestartFail2ban: + """Tests for ``POST /api/config/restart``.""" + + async def test_204_on_success(self, config_client: AsyncClient) -> None: + """POST /api/config/restart returns 204 when fail2ban restarts cleanly.""" + with ( + patch( + "app.routers.config.jail_service.restart", + AsyncMock(return_value=None), + ), + patch( + "app.routers.config.config_file_service.start_daemon", + AsyncMock(return_value=True), + ), + patch( + "app.routers.config.config_file_service.wait_for_fail2ban", + AsyncMock(return_value=True), + ), + ): + resp = await config_client.post("/api/config/restart") + + assert resp.status_code == 204 + + async def test_503_when_fail2ban_does_not_come_back(self, config_client: AsyncClient) -> None: + """POST /api/config/restart returns 503 when fail2ban does not come back online.""" + with ( + patch( + "app.routers.config.jail_service.restart", + AsyncMock(return_value=None), + ), + patch( + "app.routers.config.config_file_service.start_daemon", + AsyncMock(return_value=True), + ), + patch( + "app.routers.config.config_file_service.wait_for_fail2ban", + AsyncMock(return_value=False), + ), + ): + resp = await config_client.post("/api/config/restart") + + assert resp.status_code == 503 + + async def test_409_when_stop_command_fails(self, config_client: AsyncClient) -> None: + """POST /api/config/restart returns 409 when fail2ban rejects the stop command.""" + from app.services.jail_service import JailOperationError + + with patch( + "app.routers.config.jail_service.restart", + AsyncMock(side_effect=JailOperationError("stop failed")), + ): + resp = await config_client.post("/api/config/restart") + + assert resp.status_code == 409 + + async def test_502_when_fail2ban_unreachable(self, config_client: AsyncClient) -> None: + """POST /api/config/restart returns 502 when fail2ban socket is unreachable.""" + from app.utils.fail2ban_client import Fail2BanConnectionError + + with patch( + "app.routers.config.jail_service.restart", + AsyncMock(side_effect=Fail2BanConnectionError("no socket", "/fake.sock")), + ): + resp = await config_client.post("/api/config/restart") + + assert resp.status_code == 502 + + async def test_start_daemon_called_after_stop(self, config_client: AsyncClient) -> None: + """start_daemon is called after a successful stop.""" + mock_start = AsyncMock(return_value=True) + with ( + patch( + "app.routers.config.jail_service.restart", + AsyncMock(return_value=None), + ), + patch( + "app.routers.config.config_file_service.start_daemon", + mock_start, + ), + patch( + "app.routers.config.config_file_service.wait_for_fail2ban", + AsyncMock(return_value=True), + ), + ): + await config_client.post("/api/config/restart") + + mock_start.assert_awaited_once() + # --------------------------------------------------------------------------- # POST /api/config/regex-test diff --git a/backend/tests/test_services/test_config_file_service.py b/backend/tests/test_services/test_config_file_service.py index ddbd548..5a9f80f 100644 --- a/backend/tests/test_services/test_config_file_service.py +++ b/backend/tests/test_services/test_config_file_service.py @@ -21,6 +21,7 @@ from app.services.config_file_service import ( activate_jail, deactivate_jail, list_inactive_jails, + rollback_jail, ) # --------------------------------------------------------------------------- @@ -3174,4 +3175,150 @@ class TestActivateJailRollback: assert "logpath" in result.message.lower() or "check that all logpath" in result.message.lower() +# --------------------------------------------------------------------------- +# rollback_jail +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +class TestRollbackJail: + """Integration tests for :func:`~app.services.config_file_service.rollback_jail`.""" + + async def test_local_file_written_enabled_false(self, tmp_path: Path) -> None: + """rollback_jail writes enabled=false to jail.d/{name}.local before any socket call.""" + (tmp_path / "jail.d").mkdir() + + with ( + patch( + "app.services.config_file_service.start_daemon", + AsyncMock(return_value=True), + ), + patch( + "app.services.config_file_service.wait_for_fail2ban", + AsyncMock(return_value=True), + ), + patch( + "app.services.config_file_service._get_active_jail_names", + AsyncMock(return_value={"sshd"}), + ), + ): + await rollback_jail(str(tmp_path), "/fake.sock", "sshd", ["fail2ban-client", "start"]) + + local = tmp_path / "jail.d" / "sshd.local" + assert local.is_file(), "jail.d/sshd.local must be written" + content = local.read_text() + assert "enabled = false" in content + + async def test_start_command_invoked_via_subprocess(self, tmp_path: Path) -> None: + """rollback_jail invokes the daemon start command via start_daemon, not via socket.""" + mock_start = AsyncMock(return_value=True) + + with ( + patch("app.services.config_file_service.start_daemon", mock_start), + patch( + "app.services.config_file_service.wait_for_fail2ban", + AsyncMock(return_value=True), + ), + patch( + "app.services.config_file_service._get_active_jail_names", + AsyncMock(return_value={"other"}), + ), + ): + await rollback_jail( + str(tmp_path), "/fake.sock", "sshd", ["fail2ban-client", "start"] + ) + + mock_start.assert_awaited_once_with(["fail2ban-client", "start"]) + + async def test_fail2ban_running_reflects_socket_probe_not_subprocess_exit( + self, tmp_path: Path + ) -> None: + """fail2ban_running in the response reflects the socket probe result. + + Even when start_daemon returns True (subprocess exit 0), if the socket + probe returns False the response must report fail2ban_running=False. + """ + with ( + patch( + "app.services.config_file_service.start_daemon", + AsyncMock(return_value=True), + ), + patch( + "app.services.config_file_service.wait_for_fail2ban", + AsyncMock(return_value=False), # socket still unresponsive + ), + ): + result = await rollback_jail( + str(tmp_path), "/fake.sock", "sshd", ["fail2ban-client", "start"] + ) + + assert result.fail2ban_running is False + + async def test_active_jails_zero_when_fail2ban_not_running( + self, tmp_path: Path + ) -> None: + """active_jails is 0 in the response when fail2ban_running is False.""" + with ( + patch( + "app.services.config_file_service.start_daemon", + AsyncMock(return_value=False), + ), + patch( + "app.services.config_file_service.wait_for_fail2ban", + AsyncMock(return_value=False), + ), + ): + result = await rollback_jail( + str(tmp_path), "/fake.sock", "sshd", ["fail2ban-client", "start"] + ) + + assert result.active_jails == 0 + + async def test_active_jails_count_from_socket_when_running( + self, tmp_path: Path + ) -> None: + """active_jails reflects the actual jail count from the socket when fail2ban is up.""" + with ( + patch( + "app.services.config_file_service.start_daemon", + AsyncMock(return_value=True), + ), + patch( + "app.services.config_file_service.wait_for_fail2ban", + AsyncMock(return_value=True), + ), + patch( + "app.services.config_file_service._get_active_jail_names", + AsyncMock(return_value={"sshd", "nginx", "apache-auth"}), + ), + ): + result = await rollback_jail( + str(tmp_path), "/fake.sock", "sshd", ["fail2ban-client", "start"] + ) + + assert result.active_jails == 3 + + async def test_fail2ban_down_at_start_still_succeeds_file_write( + self, tmp_path: Path + ) -> None: + """rollback_jail writes the local file even when fail2ban is down at call time.""" + # fail2ban is down: start_daemon fails and wait_for_fail2ban returns False. + with ( + patch( + "app.services.config_file_service.start_daemon", + AsyncMock(return_value=False), + ), + patch( + "app.services.config_file_service.wait_for_fail2ban", + AsyncMock(return_value=False), + ), + ): + result = await rollback_jail( + str(tmp_path), "/fake.sock", "sshd", ["fail2ban-client", "start"] + ) + + local = tmp_path / "jail.d" / "sshd.local" + assert local.is_file(), "local file must be written even when fail2ban is down" + assert result.disabled is True + assert result.fail2ban_running is False diff --git a/backend/tests/test_services/test_jail_service.py b/backend/tests/test_services/test_jail_service.py index 9ac80c0..4afb718 100644 --- a/backend/tests/test_services/test_jail_service.py +++ b/backend/tests/test_services/test_jail_service.py @@ -441,6 +441,33 @@ class TestJailControls: ) assert exc_info.value.name == "airsonic-auth" + async def test_restart_sends_stop_command(self) -> None: + """restart() sends the ['stop'] command to the fail2ban socket.""" + with _patch_client({"stop": (0, None)}): + await jail_service.restart(_SOCKET) # should not raise + + async def test_restart_operation_error_raises(self) -> None: + """restart() raises JailOperationError when fail2ban rejects the stop.""" + with _patch_client({"stop": (1, Exception("cannot stop"))}), pytest.raises( + JailOperationError + ): + await jail_service.restart(_SOCKET) + + async def test_restart_connection_error_propagates(self) -> None: + """restart() propagates Fail2BanConnectionError when socket is unreachable.""" + + class _FailClient: + def __init__(self, **_kw: Any) -> None: + self.send = AsyncMock( + side_effect=Fail2BanConnectionError("no socket", _SOCKET) + ) + + with ( + patch("app.services.jail_service.Fail2BanClient", _FailClient), + pytest.raises(Fail2BanConnectionError), + ): + await jail_service.restart(_SOCKET) + async def test_start_not_found_raises(self) -> None: """start_jail raises JailNotFoundError for unknown jail.""" with _patch_client({"start|ghost": (1, Exception("Unknown jail: 'ghost'"))}), pytest.raises(JailNotFoundError):