Fix stale activation record on failed jail activation
Record activation only after a successful jail activate request and add regression coverage to prevent stale last_activation state.
This commit is contained in:
@@ -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 `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`.
|
- 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.
|
||||||
|
|
||||||
|
|||||||
@@ -272,6 +272,8 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue.
|
|||||||
|
|
||||||
**Docs changes needed:** Update `Docs/Refactoring.md`.
|
**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.
|
**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.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ from app.exceptions import (
|
|||||||
ConfigOperationError,
|
ConfigOperationError,
|
||||||
ConfigValidationError,
|
ConfigValidationError,
|
||||||
ConfigWriteError,
|
ConfigWriteError,
|
||||||
|
Fail2BanConnectionError,
|
||||||
FilterNameError,
|
FilterNameError,
|
||||||
FilterNotFoundError,
|
FilterNotFoundError,
|
||||||
JailAlreadyActiveError,
|
JailAlreadyActiveError,
|
||||||
@@ -46,11 +47,9 @@ from app.services import (
|
|||||||
filter_config_service,
|
filter_config_service,
|
||||||
jail_config_service,
|
jail_config_service,
|
||||||
)
|
)
|
||||||
from app.exceptions import Fail2BanConnectionError
|
|
||||||
from app.utils.runtime_state import (
|
from app.utils.runtime_state import (
|
||||||
clear_activation_record,
|
clear_activation_record,
|
||||||
clear_pending_recovery,
|
clear_pending_recovery,
|
||||||
create_pending_recovery,
|
|
||||||
record_activation,
|
record_activation,
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -382,8 +381,6 @@ async def activate_jail(
|
|||||||
"""
|
"""
|
||||||
req = body if body is not None else ActivateJailRequest()
|
req = body if body is not None else ActivateJailRequest()
|
||||||
|
|
||||||
activation_time = record_activation(app, name)
|
|
||||||
|
|
||||||
try:
|
try:
|
||||||
result = await jail_config_service.activate_jail(config_dir, socket_path, name, req)
|
result = await jail_config_service.activate_jail(config_dir, socket_path, name, req)
|
||||||
except JailNameError as exc:
|
except JailNameError as exc:
|
||||||
@@ -403,6 +400,9 @@ async def activate_jail(
|
|||||||
except Fail2BanConnectionError as exc:
|
except Fail2BanConnectionError as exc:
|
||||||
raise _bad_gateway(exc) from exc
|
raise _bad_gateway(exc) from exc
|
||||||
|
|
||||||
|
if result.active:
|
||||||
|
record_activation(app, name)
|
||||||
|
|
||||||
return result
|
return result
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -839,6 +839,24 @@ class TestActivateJail:
|
|||||||
|
|
||||||
assert resp.status_code == 409
|
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:
|
async def test_400_for_invalid_jail_name(self, config_client: AsyncClient) -> None:
|
||||||
"""POST /api/config/jails/ with bad name returns 400."""
|
"""POST /api/config/jails/ with bad name returns 400."""
|
||||||
from app.exceptions import JailNameError
|
from app.exceptions import JailNameError
|
||||||
@@ -1903,7 +1921,7 @@ class TestGetFail2BanLog:
|
|||||||
async def test_200_returns_log_response(self, config_client: AsyncClient) -> None:
|
async def test_200_returns_log_response(self, config_client: AsyncClient) -> None:
|
||||||
"""GET /api/config/fail2ban-log returns 200 with Fail2BanLogResponse."""
|
"""GET /api/config/fail2ban-log returns 200 with Fail2BanLogResponse."""
|
||||||
with patch(
|
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()),
|
AsyncMock(return_value=self._mock_log_response()),
|
||||||
):
|
):
|
||||||
resp = await config_client.get("/api/config/fail2ban-log")
|
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:
|
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."""
|
"""GET /api/config/fail2ban-log passes the lines query param to the service."""
|
||||||
with patch(
|
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()),
|
AsyncMock(return_value=self._mock_log_response()),
|
||||||
) as mock_fn:
|
) as mock_fn:
|
||||||
resp = await config_client.get("/api/config/fail2ban-log?lines=500")
|
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:
|
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."""
|
"""GET /api/config/fail2ban-log passes the filter query param to the service."""
|
||||||
with patch(
|
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()),
|
AsyncMock(return_value=self._mock_log_response()),
|
||||||
) as mock_fn:
|
) as mock_fn:
|
||||||
resp = await config_client.get("/api/config/fail2ban-log?filter=ERROR")
|
resp = await config_client.get("/api/config/fail2ban-log?filter=ERROR")
|
||||||
@@ -1944,7 +1962,7 @@ class TestGetFail2BanLog:
|
|||||||
from app.services.config_service import ConfigOperationError
|
from app.services.config_service import ConfigOperationError
|
||||||
|
|
||||||
with patch(
|
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'")),
|
AsyncMock(side_effect=ConfigOperationError("fail2ban is logging to 'STDOUT'")),
|
||||||
):
|
):
|
||||||
resp = await config_client.get("/api/config/fail2ban-log")
|
resp = await config_client.get("/api/config/fail2ban-log")
|
||||||
@@ -1956,7 +1974,7 @@ class TestGetFail2BanLog:
|
|||||||
from app.services.config_service import ConfigOperationError
|
from app.services.config_service import ConfigOperationError
|
||||||
|
|
||||||
with patch(
|
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")),
|
AsyncMock(side_effect=ConfigOperationError("outside the allowed directory")),
|
||||||
):
|
):
|
||||||
resp = await config_client.get("/api/config/fail2ban-log")
|
resp = await config_client.get("/api/config/fail2ban-log")
|
||||||
@@ -1968,7 +1986,7 @@ class TestGetFail2BanLog:
|
|||||||
from app.exceptions import Fail2BanConnectionError
|
from app.exceptions import Fail2BanConnectionError
|
||||||
|
|
||||||
with patch(
|
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")),
|
AsyncMock(side_effect=Fail2BanConnectionError("socket error", "/tmp/f.sock")),
|
||||||
):
|
):
|
||||||
resp = await config_client.get("/api/config/fail2ban-log")
|
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:
|
async def test_200_when_online(self, config_client: AsyncClient) -> None:
|
||||||
"""GET /api/config/service-status returns 200 with full status when online."""
|
"""GET /api/config/service-status returns 200 with full status when online."""
|
||||||
with patch(
|
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)),
|
AsyncMock(return_value=self._mock_status(online=True)),
|
||||||
):
|
):
|
||||||
resp = await config_client.get("/api/config/service-status")
|
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:
|
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."""
|
"""GET /api/config/service-status returns 200 with offline=False when daemon is down."""
|
||||||
with patch(
|
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)),
|
AsyncMock(return_value=self._mock_status(online=False)),
|
||||||
):
|
):
|
||||||
resp = await config_client.get("/api/config/service-status")
|
resp = await config_client.get("/api/config/service-status")
|
||||||
|
|||||||
Reference in New Issue
Block a user