diff --git a/Docs/Architekture.md b/Docs/Architekture.md index 551bd47..c8ccc74 100644 --- a/Docs/Architekture.md +++ b/Docs/Architekture.md @@ -252,6 +252,7 @@ Pure helper modules with no framework dependencies. | Module | Purpose | |---|---| | `fail2ban_client.py` | Async client that communicates with fail2ban via its Unix domain socket — sends commands and parses responses using the fail2ban protocol. Modelled after [`./fail2ban-master/fail2ban/client/csocket.py`](../fail2ban-master/fail2ban/client/csocket.py) and [`./fail2ban-master/fail2ban/client/fail2banclient.py`](../fail2ban-master/fail2ban/client/fail2banclient.py). | +| `jail_socket.py` | Low-level jail reload operations (`reload_all`) extracted to break service dependencies. Used by `jail_service`, `jail_config_service`, `action_config_service`, and `filter_config_service` to avoid circular imports between sibling services. | | `ip_utils.py` | Validates IPv4/IPv6 addresses and CIDR ranges using the `ipaddress` stdlib module, normalises formats | | `jail_utils.py` | Jail helper functions for configuration and status inference | | `jail_config.py` | Jail config parser and serializer for fail2ban config manipulation | @@ -774,6 +775,7 @@ These principles govern all architectural decisions in BanGUI. | Principle | Application | |---|---| | **Separation of Concerns** | Frontend and backend are independent. Backend layers (router → service → repository) never mix responsibilities. | +| **Service Independence** | Services must not import other services at the same layer (e.g., `jail_config_service` must not import `jail_service`). Shared logic belongs in the utils layer (`app/utils/`). This prevents circular dependencies, improves testability, and keeps each service focused on its domain. | | **Single Responsibility** | Each module, service, and component has one well-defined job. | | **Dependency Inversion** | Services depend on abstractions (protocols), not concrete implementations. FastAPI `Depends()` wires everything. | | **Async Everything** | All I/O is non-blocking. No synchronous database, HTTP, or socket calls anywhere in the backend. | diff --git a/Docs/Tasks.md b/Docs/Tasks.md index b58f6df..1d1b83b 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,26 +1,3 @@ -### T-06 · Eliminate `AppState` Protocol / `ApplicationContext` dataclass redundancy - -**Where found:** `backend/app/dependencies.py` — `AppState` Protocol (lines ~40–60) and `ApplicationContext` dataclass (lines ~62–75) describe identical fields. - -**Why this is needed:** Every new field must be added to both. The Protocol is only used for a single `cast()` call inside `_build_app_context`. Maintenance burden with no benefit. - -**Goal:** One typed representation. Remove `AppState` Protocol; cast directly or use `ApplicationContext`. - -**What to do:** -1. Delete the `AppState` Protocol class. -2. Replace `state = cast("AppState", request.app.state)` with `state = request.app.state` (type: ignore or use `ApplicationState` directly since it's the concrete type set in `create_app`). -3. Access fields from `state` directly using the `ApplicationState` / `RuntimeState` types. - -**Possible traps and issues:** -- `mypy` / pyright may report type errors on `request.app.state` accesses — use `cast(ApplicationState, request.app.state)` once, then access typed. -- Ensure all fields accessed in `_build_app_context` are present on `ApplicationState`. - -**Docs changes needed:** None. - -**Doc references:** `backend/app/dependencies.py`, `backend/app/utils/runtime_state.py` - ---- - ### T-07 · Break cross-service import: `jail_config_service` imports `jail_service` **Where found:** `backend/app/services/jail_config_service.py` — `import app.services.jail_service as jail_service` diff --git a/backend/app/services/action_config_service.py b/backend/app/services/action_config_service.py index 01bb8a0..36b5bcd 100644 --- a/backend/app/services/action_config_service.py +++ b/backend/app/services/action_config_service.py @@ -25,13 +25,6 @@ from app.exceptions import ( ConfigWriteError, JailNotFoundInConfigError, ) -import app.services.jail_service as jail_service -from app.utils.config_file_utils import ( - _get_active_jail_names as _config_file_get_active_jail_names, - _parse_jails_sync as _config_file_parse_jails_sync, - _safe_jail_name, - build_parser, -) from app.models.config import ( ActionConfig, ActionConfigUpdate, @@ -42,6 +35,17 @@ from app.models.config import ( ) from app.utils import conffile_parser from app.utils.async_utils import run_blocking +from app.utils.config_file_utils import ( + _get_active_jail_names as _config_file_get_active_jail_names, +) +from app.utils.config_file_utils import ( + _parse_jails_sync as _config_file_parse_jails_sync, +) +from app.utils.config_file_utils import ( + _safe_jail_name, + build_parser, +) +from app.utils.jail_socket import reload_all log: structlog.stdlib.BoundLogger = structlog.get_logger() @@ -681,7 +685,7 @@ async def update_action( if do_reload: try: - await jail_service.reload_all(socket_path) + await reload_all(socket_path) except Exception as exc: # noqa: BLE001 log.warning( "reload_after_action_update_failed", @@ -749,7 +753,7 @@ async def create_action( if do_reload: try: - await jail_service.reload_all(socket_path) + await reload_all(socket_path) except Exception as exc: # noqa: BLE001 log.warning( "reload_after_action_create_failed", @@ -874,7 +878,7 @@ async def assign_action_to_jail( if do_reload: try: - await jail_service.reload_all(socket_path) + await reload_all(socket_path) except Exception as exc: # noqa: BLE001 log.warning( "reload_after_assign_action_failed", @@ -932,7 +936,7 @@ async def remove_action_from_jail( if do_reload: try: - await jail_service.reload_all(socket_path) + await reload_all(socket_path) except Exception as exc: # noqa: BLE001 log.warning( "reload_after_remove_action_failed", diff --git a/backend/app/services/filter_config_service.py b/backend/app/services/filter_config_service.py index 6e5ca68..ef305e7 100644 --- a/backend/app/services/filter_config_service.py +++ b/backend/app/services/filter_config_service.py @@ -23,14 +23,6 @@ from app.exceptions import ( FilterReadonlyError, JailNotFoundInConfigError, ) -import app.services.jail_service as jail_service -from app.utils.config_file_utils import ( - _get_active_jail_names as _config_file_get_active_jail_names, - _parse_jails_sync as _config_file_parse_jails_sync, - _safe_filter_name, - _safe_jail_name, - set_jail_local_key_sync, -) from app.models.config import ( AssignFilterRequest, FilterConfig, @@ -41,6 +33,18 @@ from app.models.config import ( ) from app.utils import conffile_parser from app.utils.async_utils import run_blocking +from app.utils.config_file_utils import ( + _get_active_jail_names as _config_file_get_active_jail_names, +) +from app.utils.config_file_utils import ( + _parse_jails_sync as _config_file_parse_jails_sync, +) +from app.utils.config_file_utils import ( + _safe_filter_name, + _safe_jail_name, + set_jail_local_key_sync, +) +from app.utils.jail_socket import reload_all log: structlog.stdlib.BoundLogger = structlog.get_logger() @@ -508,7 +512,7 @@ async def update_filter( if do_reload: try: - await jail_service.reload_all(socket_path) + await reload_all(socket_path) except Exception as exc: # noqa: BLE001 log.warning( "reload_after_filter_update_failed", @@ -582,7 +586,7 @@ async def create_filter( if do_reload: try: - await jail_service.reload_all(socket_path) + await reload_all(socket_path) except Exception as exc: # noqa: BLE001 log.warning( "reload_after_filter_create_failed", @@ -704,7 +708,7 @@ async def assign_filter_to_jail( if do_reload: try: - await jail_service.reload_all(socket_path) + await reload_all(socket_path) except Exception as exc: # noqa: BLE001 log.warning( "reload_after_assign_filter_failed", diff --git a/backend/app/services/jail_config_service.py b/backend/app/services/jail_config_service.py index 335359b..9e3adfe 100644 --- a/backend/app/services/jail_config_service.py +++ b/backend/app/services/jail_config_service.py @@ -25,17 +25,6 @@ from app.exceptions import ( JailNotFoundError, JailNotFoundInConfigError, ) -import app.services.jail_service as jail_service -from app.utils.config_file_utils import ( - _build_inactive_jail, - _parse_jails_sync as _config_file_parse_jails_sync, - _get_active_jail_names as _config_file_get_active_jail_names, - _probe_fail2ban_running, - _safe_jail_name, - _validate_jail_config_sync as _config_file_validate_jail_config_sync, - start_daemon, - wait_for_fail2ban, -) from app.models.config import ( ActivateJailRequest, InactiveJail, @@ -46,7 +35,23 @@ from app.models.config import ( ) from app.services import health_service from app.utils.async_utils import run_blocking -from app.utils.fail2ban_client import Fail2BanClient +from app.utils.config_file_utils import ( + _build_inactive_jail, + _probe_fail2ban_running, + _safe_jail_name, + start_daemon, + wait_for_fail2ban, +) +from app.utils.config_file_utils import ( + _get_active_jail_names as _config_file_get_active_jail_names, +) +from app.utils.config_file_utils import ( + _parse_jails_sync as _config_file_parse_jails_sync, +) +from app.utils.config_file_utils import ( + _validate_jail_config_sync as _config_file_validate_jail_config_sync, +) +from app.utils.jail_socket import reload_all log: structlog.stdlib.BoundLogger = structlog.get_logger() @@ -404,7 +409,7 @@ async def _activate_jail( # Activation reload — if it fails, roll back immediately # # ---------------------------------------------------------------------- # try: - await jail_service.reload_all(socket_path, include_jails=[name]) + await reload_all(socket_path, include_jails=[name]) except JailNotFoundError as exc: # Jail configuration is invalid (e.g. missing logpath that prevents # fail2ban from loading the jail). Roll back and provide a specific error. @@ -546,7 +551,7 @@ async def _rollback_activation_async( # Step 2 — reload fail2ban with the restored config. try: - await jail_service.reload_all(socket_path) + await reload_all(socket_path) log.info("jail_activation_rollback_reload_ok", jail=name) except Exception as exc: # noqa: BLE001 log.warning("jail_activation_rollback_reload_failed", jail=name, error=str(exc)) @@ -626,7 +631,7 @@ async def _deactivate_jail( ) try: - await jail_service.reload_all(socket_path, exclude_jails=[name]) + await reload_all(socket_path, exclude_jails=[name]) except Exception as exc: # noqa: BLE001 log.warning("reload_after_deactivate_failed", jail=name, error=str(exc)) diff --git a/backend/app/services/jail_service.py b/backend/app/services/jail_service.py index 259bf4a..e43de7d 100644 --- a/backend/app/services/jail_service.py +++ b/backend/app/services/jail_service.py @@ -36,7 +36,6 @@ from app.utils.fail2ban_client import ( Fail2BanCommand, Fail2BanConnectionError, Fail2BanResponse, - Fail2BanToken, ) from app.utils.fail2ban_response import ( ensure_list, @@ -44,6 +43,7 @@ from app.utils.fail2ban_response import ( ok, to_dict, ) +from app.utils.jail_socket import reload_all if TYPE_CHECKING: from collections.abc import Awaitable @@ -55,6 +55,8 @@ if TYPE_CHECKING: log: structlog.stdlib.BoundLogger = structlog.get_logger() +__all__ = ["reload_all"] + class IpLookupResult(TypedDict): """Result returned by :func:`lookup_ip`. @@ -73,12 +75,6 @@ class IpLookupResult(TypedDict): _SOCKET_TIMEOUT: float = 10.0 -# Guard against concurrent reload_all calls. Overlapping ``reload --all`` -# commands sent to fail2ban's socket produce undefined behaviour and may cause -# jails to be permanently removed from the daemon. Serialising them here -# ensures only one reload stream is in-flight at a time. -_reload_all_lock: asyncio.Lock | None = None - # Capability detection for optional fail2ban transmitter commands (backend, idle). # These commands are not supported in all fail2ban versions. Caching the result # avoids sending unsupported commands every polling cycle and spamming the @@ -87,19 +83,6 @@ _backend_cmd_supported: bool | None = None _backend_cmd_lock: asyncio.Lock | None = None -def _get_reload_all_lock() -> asyncio.Lock: - """Return the shared reload-all lock, initialising it lazily. - - Asyncio primitives must be created inside an active event loop in test - environments that create new loops per test. Lazily initialising the lock - avoids binding it to the import-time loop. - """ - global _reload_all_lock - if _reload_all_lock is None: - _reload_all_lock = asyncio.Lock() - return _reload_all_lock - - def _get_backend_cmd_lock() -> asyncio.Lock: """Return the shared backend capability probe lock, initialising it lazily. @@ -605,65 +588,6 @@ async def reload_jail(socket_path: str, name: str) -> None: raise JailOperationError(str(exc)) from exc -async def reload_all( - socket_path: str, - *, - include_jails: list[str] | None = None, - exclude_jails: list[str] | None = None, -) -> None: - """Reload all fail2ban jails at once. - - Fetches the current jail list first so that a ``['start', name]`` entry - can be included in the config stream for every active jail. Without a - non-empty stream the end-of-reload phase deletes every jail that received - no configuration commands. - - *include_jails* are added to the stream (e.g. a newly activated jail that - is not yet running). *exclude_jails* are removed from the stream (e.g. a - jail that was just deactivated and should not be restarted). - - Args: - socket_path: Path to the fail2ban Unix domain socket. - include_jails: Extra jail names to add to the start stream. - exclude_jails: Jail names to remove from the start stream. - - Raises: - JailNotFoundError: If a jail in *include_jails* does not exist or - its configuration is invalid (e.g. missing logpath). - JailOperationError: If fail2ban reports the operation failed for - a different reason. - ~app.utils.fail2ban_client.Fail2BanConnectionError: If the socket - cannot be reached. - """ - client = Fail2BanClient(socket_path=socket_path, timeout=_SOCKET_TIMEOUT) - async with _get_reload_all_lock(): - try: - # Resolve jail names so we can build the minimal config stream. - status_raw = ok(await client.send(["status"])) - status_dict = to_dict(status_raw) - jail_list_raw: str = str(status_dict.get("Jail list", "")) - jail_names = [n.strip() for n in jail_list_raw.split(",") if n.strip()] - - # Merge include/exclude sets so the stream matches the desired state. - names_set: set[str] = set(jail_names) - if include_jails: - names_set.update(include_jails) - if exclude_jails: - names_set -= set(exclude_jails) - - stream: list[list[object]] = [["start", n] for n in sorted(names_set)] - ok(await client.send(["reload", "--all", [], cast("Fail2BanToken", stream)])) - log.info("all_jails_reloaded") - except ValueError as exc: - # Detect UnknownJailException (missing or invalid jail configuration) - # and re-raise as JailNotFoundError for better error specificity. - if is_not_found_error(exc): - # Extract the jail name from include_jails if available. - jail_name = include_jails[0] if include_jails else "unknown" - raise JailNotFoundError(jail_name) from exc - raise JailOperationError(str(exc)) from exc - - async def restart(socket_path: str) -> None: """Stop the fail2ban daemon via the Unix socket. diff --git a/backend/app/utils/jail_socket.py b/backend/app/utils/jail_socket.py new file mode 100644 index 0000000..e3450ad --- /dev/null +++ b/backend/app/utils/jail_socket.py @@ -0,0 +1,108 @@ +"""Low-level socket operations for jail management. + +Provides shared socket utilities for reloading jails across all services. +These operations are extracted to a utility layer to avoid circular dependencies +between sibling services (jail_service, jail_config_service, action_config_service, +filter_config_service). +""" + +from __future__ import annotations + +import asyncio +from typing import cast + +import structlog + +from app.exceptions import JailNotFoundError, JailOperationError +from app.utils.fail2ban_client import ( + Fail2BanClient, + Fail2BanToken, +) +from app.utils.fail2ban_response import ( + is_not_found_error, + ok, + to_dict, +) + +log: structlog.stdlib.BoundLogger = structlog.get_logger() + +# Socket communication timeout in seconds. +SOCKET_TIMEOUT: float = 10.0 + +# Guard against concurrent reload_all calls. Overlapping ``reload --all`` +# commands sent to fail2ban's socket produce undefined behaviour and may cause +# jails to be permanently removed from the daemon. Serialising them here +# ensures only one reload stream is in-flight at a time. +_reload_all_lock: asyncio.Lock | None = None + + +def _get_reload_all_lock() -> asyncio.Lock: + """Return the shared reload-all lock, initialising it lazily. + + Asyncio primitives must be created inside an active event loop in test + environments that create new loops per test. Lazily initialising the lock + avoids binding it to the import-time loop. + """ + global _reload_all_lock + if _reload_all_lock is None: + _reload_all_lock = asyncio.Lock() + return _reload_all_lock + + +async def reload_all( + socket_path: str, + *, + include_jails: list[str] | None = None, + exclude_jails: list[str] | None = None, +) -> None: + """Reload all fail2ban jails at once. + + Fetches the current jail list first so that a ``['start', name]`` entry + can be included in the config stream for every active jail. Without a + non-empty stream the end-of-reload phase deletes every jail that received + no configuration commands. + + *include_jails* are added to the stream (e.g. a newly activated jail that + is not yet running). *exclude_jails* are removed from the stream (e.g. a + jail that was just deactivated and should not be restarted). + + Args: + socket_path: Path to the fail2ban Unix domain socket. + include_jails: Extra jail names to add to the start stream. + exclude_jails: Jail names to remove from the start stream. + + Raises: + JailNotFoundError: If a jail in *include_jails* does not exist or + its configuration is invalid (e.g. missing logpath). + JailOperationError: If fail2ban reports the operation failed for + a different reason. + ~app.utils.fail2ban_client.Fail2BanConnectionError: If the socket + cannot be reached. + """ + client = Fail2BanClient(socket_path=socket_path, timeout=SOCKET_TIMEOUT) + async with _get_reload_all_lock(): + try: + # Resolve jail names so we can build the minimal config stream. + status_raw = ok(await client.send(["status"])) + status_dict = to_dict(status_raw) + jail_list_raw: str = str(status_dict.get("Jail list", "")) + jail_names = [n.strip() for n in jail_list_raw.split(",") if n.strip()] + + # Merge include/exclude sets so the stream matches the desired state. + names_set: set[str] = set(jail_names) + if include_jails: + names_set.update(include_jails) + if exclude_jails: + names_set -= set(exclude_jails) + + stream: list[list[object]] = [["start", n] for n in sorted(names_set)] + ok(await client.send(["reload", "--all", [], cast("Fail2BanToken", stream)])) + log.info("all_jails_reloaded") + except ValueError as exc: + # Detect UnknownJailException (missing or invalid jail configuration) + # and re-raise as JailNotFoundError for better error specificity. + if is_not_found_error(exc): + # Extract the jail name from include_jails if available. + jail_name = include_jails[0] if include_jails else "unknown" + raise JailNotFoundError(jail_name) from exc + raise JailOperationError(str(exc)) from exc diff --git a/backend/tests/test_services/test_jail_service.py b/backend/tests/test_services/test_jail_service.py index 5ad91a6..be26745 100644 --- a/backend/tests/test_services/test_jail_service.py +++ b/backend/tests/test_services/test_jail_service.py @@ -15,6 +15,7 @@ from app.models.geo import GeoDetail, GeoInfo from app.models.jail import JailDetailResponse, JailListResponse from app.services import ban_service, jail_service from app.services.jail_service import JailNotFoundError, JailOperationError +from app.utils import jail_socket # --------------------------------------------------------------------------- # Helpers @@ -75,6 +76,7 @@ def _patch_client(responses: dict[str, Any]) -> Any: stack = contextlib.ExitStack() stack.enter_context(patch("app.services.jail_service.Fail2BanClient", _FakeClient)) stack.enter_context(patch("app.services.ban_service.Fail2BanClient", _FakeClient)) + stack.enter_context(patch("app.utils.jail_socket.Fail2BanClient", _FakeClient)) return stack @@ -281,12 +283,12 @@ class TestLockInitialization: async def test_reload_all_lock_is_lazy_initialised(self) -> None: """The reload-all lock should be created lazily on first use.""" - jail_service._reload_all_lock = None + jail_socket._reload_all_lock = None - lock = _ = jail_service._get_reload_all_lock() + lock = _ = jail_socket._get_reload_all_lock() assert isinstance(lock, asyncio.Lock) - assert jail_service._reload_all_lock is lock + assert jail_socket._reload_all_lock is lock async def test_backend_cmd_lock_is_lazy_initialised(self) -> None: """The backend capability probe lock should be created lazily on first use."""