diff --git a/Docs/Tasks.md b/Docs/Tasks.md index fcd2a14..9423b31 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -157,6 +157,8 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue. **Why this is needed:** Multi-step orchestration workflows are business logic and do not belong in HTTP handler functions. Keeping them in the service layer makes the workflow testable in isolation and reusable if a second endpoint ever needs to restart the daemon. +**Status:** Completed ✅ + --- ### Task 9 — Move `sign_session_token` call out of `auth.py` router into `auth_service.login()` diff --git a/backend/app/routers/config_misc.py b/backend/app/routers/config_misc.py index a7a7d74..bdf12cb 100644 --- a/backend/app/routers/config_misc.py +++ b/backend/app/routers/config_misc.py @@ -33,7 +33,6 @@ from app.services import ( jail_service, log_service, ) -from app.utils.config_file_utils import start_daemon, wait_for_fail2ban log: structlog.stdlib.BoundLogger = structlog.get_logger() @@ -187,9 +186,11 @@ async def restart_fail2ban( """ start_cmd_parts: list[str] = start_cmd.split() - # Step 1: stop the daemon via socket. try: - await jail_service.restart(socket_path) + restarted = await jail_service.restart_daemon( + socket_path, + start_cmd_parts, + ) except JailOperationError as exc: raise HTTPException( status_code=status.HTTP_409_CONFLICT, @@ -198,16 +199,7 @@ async def restart_fail2ban( except Fail2BanConnectionError as exc: raise _bad_gateway(exc) from exc - # Step 2: start the daemon via subprocess. - await start_daemon(start_cmd_parts) - - # Step 3: probe the socket until fail2ban is responsive or the budget - # expires. - fail2ban_running: bool = await wait_for_fail2ban( - socket_path, - max_wait_seconds=10.0, - ) - if not fail2ban_running: + if not restarted: raise HTTPException( status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=( diff --git a/backend/app/services/jail_service.py b/backend/app/services/jail_service.py index a3eab27..21cb6f6 100644 --- a/backend/app/services/jail_service.py +++ b/backend/app/services/jail_service.py @@ -29,6 +29,7 @@ from app.models.jail import ( JailStatus, JailSummary, ) +from app.utils.config_file_utils import start_daemon, wait_for_fail2ban from app.utils.fail2ban_client import ( Fail2BanClient, Fail2BanCommand, @@ -759,6 +760,48 @@ async def restart(socket_path: str) -> None: raise JailOperationError(str(exc)) from exc +async def restart_daemon( + socket_path: str, + start_cmd_parts: list[str], + max_wait_seconds: float = _SOCKET_TIMEOUT, +) -> bool: + """Restart the fail2ban daemon and verify it comes back online. + + This function stops the daemon through the socket, starts it with the + configured command, and probes the socket until fail2ban accepts status + requests again. + + Args: + socket_path: Path to the fail2ban Unix domain socket. + start_cmd_parts: The configured fail2ban start command split into + executable and arguments. + max_wait_seconds: The maximum number of seconds to wait for the daemon + to become responsive after starting. + + Returns: + ``True`` when the daemon is started and responsive. + ``False`` when the command failed or fail2ban never became responsive. + + Raises: + JailOperationError: If the stop command failed. + ~app.utils.fail2ban_client.Fail2BanConnectionError: If the socket + cannot be reached while stopping fail2ban. + """ + await restart(socket_path) + + if not await start_daemon(start_cmd_parts): + log.warning( + "fail2ban_start_command_failed", + command=" ".join(start_cmd_parts), + ) + return False + + return await wait_for_fail2ban( + socket_path, + max_wait_seconds=max_wait_seconds, + ) + + # --------------------------------------------------------------------------- # Public API — Ban / Unban # --------------------------------------------------------------------------- diff --git a/backend/tests/test_routers/test_config.py b/backend/tests/test_routers/test_config.py index 6ec7bb1..ed65dfc 100644 --- a/backend/tests/test_routers/test_config.py +++ b/backend/tests/test_routers/test_config.py @@ -406,19 +406,9 @@ class TestRestartFail2ban: 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_misc.jail_service.restart", - AsyncMock(return_value=None), - ), - patch( - "app.routers.config_misc.start_daemon", - AsyncMock(return_value=True), - ), - patch( - "app.routers.config_misc.wait_for_fail2ban", - AsyncMock(return_value=True), - ), + with patch( + "app.routers.config_misc.jail_service.restart_daemon", + AsyncMock(return_value=True), ): resp = await config_client.post("/api/config/restart") @@ -426,19 +416,9 @@ class TestRestartFail2ban: 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_misc.jail_service.restart", - AsyncMock(return_value=None), - ), - patch( - "app.routers.config_misc.start_daemon", - AsyncMock(return_value=True), - ), - patch( - "app.routers.config_misc.wait_for_fail2ban", - AsyncMock(return_value=False), - ), + with patch( + "app.routers.config_misc.jail_service.restart_daemon", + AsyncMock(return_value=False), ): resp = await config_client.post("/api/config/restart") @@ -449,7 +429,7 @@ class TestRestartFail2ban: from app.services.jail_service import JailOperationError with patch( - "app.routers.config_misc.jail_service.restart", + "app.routers.config_misc.jail_service.restart_daemon", AsyncMock(side_effect=JailOperationError("stop failed")), ): resp = await config_client.post("/api/config/restart") @@ -461,33 +441,24 @@ class TestRestartFail2ban: from app.exceptions import Fail2BanConnectionError with patch( - "app.routers.config_misc.jail_service.restart", + "app.routers.config_misc.jail_service.restart_daemon", 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_misc.jail_service.restart", - AsyncMock(return_value=None), - ), - patch( - "app.routers.config_misc.start_daemon", - mock_start, - ), - patch( - "app.routers.config_misc.wait_for_fail2ban", - AsyncMock(return_value=True), - ), + async def test_service_restart_daemon_called(self, config_client: AsyncClient) -> None: + """The router delegates restart orchestration to jail_service.restart_daemon.""" + mock_restart = AsyncMock(return_value=True) + with patch( + "app.routers.config_misc.jail_service.restart_daemon", + mock_restart, ): - await config_client.post("/api/config/restart") + resp = await config_client.post("/api/config/restart") - mock_start.assert_awaited_once() + assert resp.status_code == 204 + mock_restart.assert_awaited_once() # --------------------------------------------------------------------------- diff --git a/backend/tests/test_services/test_jail_service.py b/backend/tests/test_services/test_jail_service.py index c2e3f08..643eb84 100644 --- a/backend/tests/test_services/test_jail_service.py +++ b/backend/tests/test_services/test_jail_service.py @@ -8,12 +8,12 @@ from unittest.mock import AsyncMock, patch import pytest +from app.exceptions import Fail2BanConnectionError from app.models.ban import ActiveBanListResponse, JailBannedIpsResponse from app.models.geo import GeoDetail, GeoInfo from app.models.jail import JailDetailResponse, JailListResponse from app.services import jail_service from app.services.jail_service import JailNotFoundError, JailOperationError -from app.exceptions import Fail2BanConnectionError # --------------------------------------------------------------------------- # Helpers @@ -492,6 +492,47 @@ class TestJailControls: ): await jail_service.restart(_SOCKET) + async def test_restart_daemon_returns_true_on_success(self) -> None: + """restart_daemon returns True when stop, start, and probe all succeed.""" + with ( + patch("app.services.jail_service.restart", AsyncMock(return_value=None)), + patch("app.services.jail_service.start_daemon", AsyncMock(return_value=True)), + patch("app.services.jail_service.wait_for_fail2ban", AsyncMock(return_value=True)), + ): + result = await jail_service.restart_daemon( + _SOCKET, + ["fail2ban-client", "start"], + ) + + assert result is True + + async def test_restart_daemon_returns_false_when_start_fails(self) -> None: + """restart_daemon returns False when the configured start command fails.""" + with ( + patch("app.services.jail_service.restart", AsyncMock(return_value=None)), + patch("app.services.jail_service.start_daemon", AsyncMock(return_value=False)), + ): + result = await jail_service.restart_daemon( + _SOCKET, + ["fail2ban-client", "start"], + ) + + assert result is False + + async def test_restart_daemon_returns_false_when_wait_fails(self) -> None: + """restart_daemon returns False when fail2ban does not become responsive.""" + with ( + patch("app.services.jail_service.restart", AsyncMock(return_value=None)), + patch("app.services.jail_service.start_daemon", AsyncMock(return_value=True)), + patch("app.services.jail_service.wait_for_fail2ban", AsyncMock(return_value=False)), + ): + result = await jail_service.restart_daemon( + _SOCKET, + ["fail2ban-client", "start"], + ) + + assert result is False + 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):