diff --git a/Docs/Refactoring.md b/Docs/Refactoring.md index b7dd8dd..8655c57 100644 --- a/Docs/Refactoring.md +++ b/Docs/Refactoring.md @@ -8,4 +8,5 @@ This document catalogues architecture violations, code smells, and structural is - Moved `Fail2BanConnectionError` and `Fail2BanProtocolError` from `backend/app/utils/fail2ban_client.py` into `backend/app/exceptions.py`. Updated all router, service, and test call sites to import these domain exceptions from `app.exceptions` and retained backward compatibility through re-exporting in `app.utils.fail2ban_client`. - Moved config file exceptions (`ConfigDirError`, `ConfigFileNotFoundError`, `ConfigFileExistsError`, `ConfigFileWriteError`, `ConfigFileNameError`) from `backend/app/services/raw_config_io_service.py` into `backend/app/exceptions.py`. Updated router and tests to import the shared domain exceptions from `app.exceptions`. +- Fixed stale activation tracking in `backend/app/routers/jail_config.py` by recording `last_activation` only after a successful jail activation and preventing a failed activation attempt from leaving a stale runtime state record. diff --git a/Docs/Tasks.md b/Docs/Tasks.md index f2cf8e3..2469719 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -272,6 +272,8 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue. **Docs changes needed:** Update `Docs/Refactoring.md`. +**Status:** Completed ✅ + **Why this is needed:** A false `PendingRecovery` presented to the user offers a rollback for a jail that was never successfully activated. This could confuse operators into performing a rollback that modifies configuration unnecessarily. --- diff --git a/backend/app/routers/jail_config.py b/backend/app/routers/jail_config.py index 71b9f50..de4a9fc 100644 --- a/backend/app/routers/jail_config.py +++ b/backend/app/routers/jail_config.py @@ -18,6 +18,7 @@ from app.exceptions import ( ConfigOperationError, ConfigValidationError, ConfigWriteError, + Fail2BanConnectionError, FilterNameError, FilterNotFoundError, JailAlreadyActiveError, @@ -46,11 +47,9 @@ from app.services import ( filter_config_service, jail_config_service, ) -from app.exceptions import Fail2BanConnectionError from app.utils.runtime_state import ( clear_activation_record, clear_pending_recovery, - create_pending_recovery, record_activation, ) @@ -382,8 +381,6 @@ async def activate_jail( """ req = body if body is not None else ActivateJailRequest() - activation_time = record_activation(app, name) - try: result = await jail_config_service.activate_jail(config_dir, socket_path, name, req) except JailNameError as exc: @@ -403,6 +400,9 @@ async def activate_jail( except Fail2BanConnectionError as exc: raise _bad_gateway(exc) from exc + if result.active: + record_activation(app, name) + return result diff --git a/backend/tests/test_routers/test_config.py b/backend/tests/test_routers/test_config.py index 40a8284..6ec7bb1 100644 --- a/backend/tests/test_routers/test_config.py +++ b/backend/tests/test_routers/test_config.py @@ -839,6 +839,24 @@ class TestActivateJail: assert resp.status_code == 409 + async def test_failed_activation_does_not_set_last_activation( + self, config_client: AsyncClient + ) -> None: + """A failed activation must not leave a stale last_activation record.""" + from app.exceptions import Fail2BanConnectionError + + config_client._transport.app.state.last_activation = None + with patch( + "app.routers.jail_config.jail_config_service.activate_jail", + AsyncMock(side_effect=Fail2BanConnectionError("No socket", "/tmp/fake.sock")), + ): + resp = await config_client.post( + "/api/config/jails/sshd/activate", json={} + ) + + assert resp.status_code == 502 + assert config_client._transport.app.state.last_activation is None + async def test_400_for_invalid_jail_name(self, config_client: AsyncClient) -> None: """POST /api/config/jails/ with bad name returns 400.""" from app.exceptions import JailNameError @@ -1903,7 +1921,7 @@ class TestGetFail2BanLog: async def test_200_returns_log_response(self, config_client: AsyncClient) -> None: """GET /api/config/fail2ban-log returns 200 with Fail2BanLogResponse.""" with patch( - "app.routers.config_misc.config_service.read_fail2ban_log", + "app.routers.config_misc.log_service.read_fail2ban_log", AsyncMock(return_value=self._mock_log_response()), ): resp = await config_client.get("/api/config/fail2ban-log") @@ -1918,7 +1936,7 @@ class TestGetFail2BanLog: async def test_200_passes_lines_query_param(self, config_client: AsyncClient) -> None: """GET /api/config/fail2ban-log passes the lines query param to the service.""" with patch( - "app.routers.config_misc.config_service.read_fail2ban_log", + "app.routers.config_misc.log_service.read_fail2ban_log", AsyncMock(return_value=self._mock_log_response()), ) as mock_fn: resp = await config_client.get("/api/config/fail2ban-log?lines=500") @@ -1930,7 +1948,7 @@ class TestGetFail2BanLog: async def test_200_passes_filter_query_param(self, config_client: AsyncClient) -> None: """GET /api/config/fail2ban-log passes the filter query param to the service.""" with patch( - "app.routers.config_misc.config_service.read_fail2ban_log", + "app.routers.config_misc.log_service.read_fail2ban_log", AsyncMock(return_value=self._mock_log_response()), ) as mock_fn: resp = await config_client.get("/api/config/fail2ban-log?filter=ERROR") @@ -1944,7 +1962,7 @@ class TestGetFail2BanLog: from app.services.config_service import ConfigOperationError with patch( - "app.routers.config_misc.config_service.read_fail2ban_log", + "app.routers.config_misc.log_service.read_fail2ban_log", AsyncMock(side_effect=ConfigOperationError("fail2ban is logging to 'STDOUT'")), ): resp = await config_client.get("/api/config/fail2ban-log") @@ -1956,7 +1974,7 @@ class TestGetFail2BanLog: from app.services.config_service import ConfigOperationError with patch( - "app.routers.config_misc.config_service.read_fail2ban_log", + "app.routers.config_misc.log_service.read_fail2ban_log", AsyncMock(side_effect=ConfigOperationError("outside the allowed directory")), ): resp = await config_client.get("/api/config/fail2ban-log") @@ -1968,7 +1986,7 @@ class TestGetFail2BanLog: from app.exceptions import Fail2BanConnectionError with patch( - "app.routers.config_misc.config_service.read_fail2ban_log", + "app.routers.config_misc.log_service.read_fail2ban_log", AsyncMock(side_effect=Fail2BanConnectionError("socket error", "/tmp/f.sock")), ): resp = await config_client.get("/api/config/fail2ban-log") @@ -2011,7 +2029,7 @@ class TestGetServiceStatus: async def test_200_when_online(self, config_client: AsyncClient) -> None: """GET /api/config/service-status returns 200 with full status when online.""" with patch( - "app.routers.config_misc.config_service.get_service_status", + "app.routers.config_misc.health_service.get_service_status", AsyncMock(return_value=self._mock_status(online=True)), ): resp = await config_client.get("/api/config/service-status") @@ -2026,7 +2044,7 @@ class TestGetServiceStatus: async def test_200_when_offline(self, config_client: AsyncClient) -> None: """GET /api/config/service-status returns 200 with offline=False when daemon is down.""" with patch( - "app.routers.config_misc.config_service.get_service_status", + "app.routers.config_misc.health_service.get_service_status", AsyncMock(return_value=self._mock_status(online=False)), ): resp = await config_client.get("/api/config/service-status")