diff --git a/Docs/Backend-Development.md b/Docs/Backend-Development.md index ec85291..4510ec0 100644 --- a/Docs/Backend-Development.md +++ b/Docs/Backend-Development.md @@ -133,6 +133,47 @@ backend/ - **Repositories** handle raw database queries — nothing else. - Never put business logic inside routers or repositories. +### Service Dependencies and Injection + +Services should **never** directly import other services to avoid hidden coupling and make testing harder. Instead: + +1. **Define clear service interfaces** using Protocol classes in `app/services/protocols.py`. +2. **Make dependencies explicit** by passing them as function parameters with optional defaults. +3. **Use lazy imports** for fallback singletons (not at module level). +4. **Inject services via FastAPI dependencies** when called from routers. + +**Example:** The `history_service` depends on `Fail2BanMetadataService` to resolve the fail2ban database path: + +```python +# Good — dependency passed as parameter +async def list_history( + socket_path: str, + fail2ban_metadata_service: Fail2BanMetadataService | None = None, +) -> HistoryListResponse: + if fail2ban_metadata_service is None: + # Lazy import fallback for backward compatibility + from app.services.fail2ban_metadata_service import default_fail2ban_metadata_service + fail2ban_metadata_service = default_fail2ban_metadata_service + ... +``` + +Routers inject the service dependency explicitly: + +```python +from app.dependencies import Fail2BanMetadataServiceDep + +@router.get("/api/history") +async def get_history( + fail2ban_metadata_service: Fail2BanMetadataServiceDep, +) -> HistoryListResponse: + return await history_service.list_history( + socket_path, + fail2ban_metadata_service=fail2ban_metadata_service, + ) +``` + +This pattern prevents circular imports, makes services testable, and allows easy mocking in tests. + --- ## 4. FastAPI Conventions diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 2ce1f4d..211ad36 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,26 +1,3 @@ -## 1) Broad exception catching in backend services -- Where found: - - [backend/app/services/ban_service.py](backend/app/services/ban_service.py) - - [backend/app/services/geo_cache.py](backend/app/services/geo_cache.py) - - [backend/app/services/blocklist_service.py](backend/app/services/blocklist_service.py) -- Why this is needed: - - Catching broad Exception hides root causes and weakens operational debugging. -- Goal: - - Replace broad catches with targeted exception handling and predictable failure paths. -- What to do: - - Inventory each broad catch. - - Replace with explicit exception classes. - - Keep one top-level safety catch only where unavoidable, with full context logging. -- Possible traps and issues: - - Over-tightening catches can expose previously hidden runtime failures. -- Docs changes needed: - - Add service error-handling policy and allowed catch patterns. -- Doc references: - - [Docs/Backend-Development.md](Docs/Backend-Development.md) - - https://docs.python.org/3/tutorial/errors.html - - ---- ## 2) Hidden cross-service coupling (service imports service) - Where found: - [backend/app/services/jail_service.py](backend/app/services/jail_service.py) @@ -43,6 +20,7 @@ - [Docs/Backend-Development.md](Docs/Backend-Development.md) --- + ## 3) Blocklist import flow mixes too many responsibilities - Where found: - [backend/app/services/blocklist_service.py](backend/app/services/blocklist_service.py) @@ -62,6 +40,7 @@ - [Docs/Features.md](Docs/Features.md) --- + ## 4) Module-level mutable runtime flags in service layer - Where found: - [backend/app/services/jail_service.py](backend/app/services/jail_service.py) @@ -80,6 +59,7 @@ - [Docs/Architekture.md](Docs/Architekture.md) --- + ## 5) Inconsistent domain exception contracts across services - Where found: - [backend/app/routers/jails.py](backend/app/routers/jails.py) @@ -101,6 +81,7 @@ - [Docs/Backend-Development.md](Docs/Backend-Development.md) --- + ## 6) Raw DB connection exposed as dependency for all routes - Where found: - [backend/app/dependencies.py](backend/app/dependencies.py) @@ -119,6 +100,7 @@ - [Docs/Backend-Development.md](Docs/Backend-Development.md) --- + ## 7) Service layer coupled to response/presentation models - Where found: - [backend/app/services/ban_service.py](backend/app/services/ban_service.py) @@ -137,6 +119,7 @@ - [Docs/Architekture.md](Docs/Architekture.md) --- + ## 8) Inconsistent modeling style (TypedDict vs Pydantic) - Where found: - [backend/app/services/jail_service.py](backend/app/services/jail_service.py) @@ -156,6 +139,7 @@ - [Docs/Backend-Development.md](Docs/Backend-Development.md) --- + ## 9) Repository protocol coverage is incomplete - Where found: - [backend/app/repositories/protocols.py](backend/app/repositories/protocols.py) @@ -175,6 +159,7 @@ - [Docs/Backend-Development.md](Docs/Backend-Development.md) --- + ## 10) Startup sequence depends on implicit ordering - Where found: - [backend/app/startup.py](backend/app/startup.py) @@ -193,6 +178,7 @@ - [Docs/Architekture.md](Docs/Architekture.md) --- + ## 11) Logging semantics are inconsistent across backend modules - Where found: - [backend/app/services](backend/app/services) @@ -212,6 +198,7 @@ - [Docs/Backend-Development.md](Docs/Backend-Development.md) --- + ## 12) Prop drilling in jail overview page - Where found: - [frontend/src/pages/jails/JailOverviewSection.tsx](frontend/src/pages/jails/JailOverviewSection.tsx) @@ -231,6 +218,7 @@ - [Docs/Web-Development.md](Docs/Web-Development.md) --- + ## 13) Config page is over-centralized - Where found: - [frontend/src/pages/ConfigPage.tsx](frontend/src/pages/ConfigPage.tsx) @@ -249,6 +237,7 @@ - [Docs/Web-Development.md](Docs/Web-Development.md) --- + ## 14) Error boundary granularity is too coarse - Where found: - [frontend/src/App.tsx](frontend/src/App.tsx) @@ -269,6 +258,7 @@ - https://react.dev/reference/react/Component#catching-rendering-errors-with-an-error-boundary --- + ## 15) Fragmented async error UX handling in components - Where found: - [frontend/src/pages/jails/BanUnbanForm.tsx](frontend/src/pages/jails/BanUnbanForm.tsx) @@ -288,6 +278,7 @@ - [Docs/Web-Development.md](Docs/Web-Development.md) --- + ## 16) API usage pattern is inconsistent across components/hooks - Where found: - [frontend/src/pages/JailsPage.tsx](frontend/src/pages/JailsPage.tsx) @@ -307,6 +298,7 @@ - [Docs/Web-Development.md](Docs/Web-Development.md) --- + ## 17) Weak typed error contracts in generic hooks - Where found: - [frontend/src/hooks/useListData.ts](frontend/src/hooks/useListData.ts) @@ -326,6 +318,7 @@ - [frontend/src/api/client.ts](frontend/src/api/client.ts) --- + ## 18) Duplicate polling/list loading behavior across hooks - Where found: - [frontend/src/hooks/useListData.ts](frontend/src/hooks/useListData.ts) @@ -344,6 +337,7 @@ - [Docs/Web-Development.md](Docs/Web-Development.md) --- + ## 19) Provider dependency chain is implicit - Where found: - [frontend/src/App.tsx](frontend/src/App.tsx) @@ -362,6 +356,7 @@ - [Docs/Web-Development.md](Docs/Web-Development.md) --- + ## 20) Loading UX lacks progressive/skeleton states - Where found: - [frontend/src/pages](frontend/src/pages) @@ -379,6 +374,7 @@ - [Docs/Web-Design.md](Docs/Web-Design.md) --- + ## 21) Silent auth error swallow in fetch error utility - Where found: - [frontend/src/utils/fetchError.ts](frontend/src/utils/fetchError.ts) @@ -396,6 +392,7 @@ - [frontend/src/providers/AuthProvider.tsx](frontend/src/providers/AuthProvider.tsx) --- + ## 22) Magic strings are scattered in frontend storage keys - Where found: - [frontend/src/providers/AuthProvider.tsx](frontend/src/providers/AuthProvider.tsx) @@ -415,6 +412,7 @@ - [frontend/src/utils/constants.ts](frontend/src/utils/constants.ts) --- + ## 23) No global cancellation policy on route transitions - Where found: - [frontend/src/hooks](frontend/src/hooks) @@ -432,6 +430,7 @@ - [Docs/Web-Development.md](Docs/Web-Development.md) --- + ## 24) API response wrapper shape is inconsistent - Where found: - [backend/app/routers/dashboard.py](backend/app/routers/dashboard.py) @@ -452,6 +451,7 @@ - [Docs/Backend-Development.md](Docs/Backend-Development.md) --- + ## 25) No canonical snake_case/camelCase serialization policy - Where found: - [backend/app/models/server.py](backend/app/models/server.py) @@ -471,6 +471,7 @@ - https://docs.pydantic.dev/latest/concepts/alias/ --- + ## 26) Pagination contract is not standardized across endpoints - Where found: - [backend/app/routers/dashboard.py](backend/app/routers/dashboard.py) @@ -490,6 +491,7 @@ - [Docs/Backend-Development.md](Docs/Backend-Development.md) --- + ## 27) Error response body shape is inconsistent - Where found: - [backend/app/main.py](backend/app/main.py) @@ -509,6 +511,7 @@ - [Docs/Backend-Development.md](Docs/Backend-Development.md) --- + ## 28) Login failure delay can enable app-layer DoS - Where found: - [backend/app/routers/auth.py](backend/app/routers/auth.py#L110) @@ -526,6 +529,7 @@ - [backend/app/utils/rate_limiter.py](backend/app/utils/rate_limiter.py) --- + ## 29) Blocklist URL validation has DNS-rebinding window - Where found: - [backend/app/utils/ip_utils.py](backend/app/utils/ip_utils.py#L145) @@ -545,6 +549,7 @@ - https://cheatsheetseries.owasp.org/cheatsheets/Server_Side_Request_Forgery_Prevention_Cheat_Sheet.html --- + ## 30) Setup persistence is non-atomic across DB contexts - Where found: - [backend/app/services/setup_service.py](backend/app/services/setup_service.py) @@ -563,6 +568,7 @@ - [Docs/Architekture.md](Docs/Architekture.md) --- + ## 31) Fire-and-forget reschedule may fail silently - Where found: - [backend/app/tasks/blocklist_import.py](backend/app/tasks/blocklist_import.py#L108) @@ -580,6 +586,7 @@ - [Docs/Features.md](Docs/Features.md) --- + ## 32) RateLimiter cleanup function is not scheduled/used - Where found: - [backend/app/utils/rate_limiter.py](backend/app/utils/rate_limiter.py#L84) @@ -598,6 +605,7 @@ - [backend/app/utils/rate_limiter.py](backend/app/utils/rate_limiter.py) --- + ## 33) Trusted proxy configuration is hardcoded in auth router - Where found: - [backend/app/routers/auth.py](backend/app/routers/auth.py#L46) @@ -617,6 +625,7 @@ - [Docs/Instructions.md](Docs/Instructions.md) --- + ## 34) Setup redirect allowlist uses broad prefix matching - Where found: - [backend/app/main.py](backend/app/main.py#L434) @@ -634,6 +643,7 @@ - [backend/app/main.py](backend/app/main.py) --- + ## 35) API client sends JSON and CSRF header for every request method - Where found: - [frontend/src/api/client.ts](frontend/src/api/client.ts) @@ -652,6 +662,7 @@ - [backend/app/middleware/csrf.py](backend/app/middleware/csrf.py) --- + ## 36) Polling continues when tab is not visible - Where found: - [frontend/src/hooks/usePolledData.ts](frontend/src/hooks/usePolledData.ts#L90) @@ -670,6 +681,7 @@ - [Docs/Web-Development.md](Docs/Web-Development.md) --- + ## 37) Multi-worker safety check depends on one environment variable - Where found: - [backend/app/startup.py](backend/app/startup.py#L61) @@ -687,6 +699,7 @@ - [Docs/Architekture.md](Docs/Architekture.md) --- + ## 38) History archive query paths may need explicit indexing plan - Where found: - [backend/app/db.py](backend/app/db.py) @@ -707,6 +720,7 @@ - https://www.sqlite.org/queryplanner.html --- + ## 39) No explicit DI container strategy for backend service graph - Where found: - [backend/app/dependencies.py](backend/app/dependencies.py) @@ -725,6 +739,7 @@ - [Docs/Architekture.md](Docs/Architekture.md) --- + ## 40) Frontend and backend observability are not aligned - Where found: - [backend/app/main.py](backend/app/main.py) @@ -741,4 +756,4 @@ - Add observability and privacy-safe logging guidelines. - Doc references: - [Docs/Architekture.md](Docs/Architekture.md) - - [Docs/Web-Development.md](Docs/Web-Development.md) + - [Docs/Web-Development.md](Docs/Web-Development.md) \ No newline at end of file diff --git a/backend/app/dependencies.py b/backend/app/dependencies.py index ebaa1cc..f6d27c5 100644 --- a/backend/app/dependencies.py +++ b/backend/app/dependencies.py @@ -7,7 +7,7 @@ directly — to keep coupling explicit and testable. """ import datetime -from collections.abc import AsyncGenerator +from collections.abc import AsyncGenerator, Awaitable, Callable from dataclasses import dataclass from typing import Annotated, cast @@ -31,6 +31,7 @@ from app.repositories.protocols import ( SettingsRepository, ) from app.services.geo_cache import GeoCache +from app.services.protocols import Fail2BanMetadataService from app.utils.constants import SESSION_COOKIE_NAME from app.utils.rate_limiter import RateLimiter from app.utils.runtime_state import ApplicationState, RuntimeState @@ -331,6 +332,31 @@ async def get_pending_recovery( """Return the current pending recovery record from application context.""" return app_context.pending_recovery + +async def get_health_probe() -> Callable[[str], Awaitable[ServerStatus]]: + """Provide the health probe function for checking fail2ban connectivity. + + Returns: + A callable that probes the fail2ban socket and returns ServerStatus. + This allows explicit dependency injection to avoid hidden service coupling. + """ + from app.services import health_service # noqa: PLC0415 + + return health_service.probe + + +async def get_fail2ban_metadata_service() -> object: + """Provide the Fail2BanMetadataService instance. + + Returns: + The singleton Fail2BanMetadataService for resolving fail2ban metadata + (such as the database path) and caching results. + """ + from app.services.fail2ban_metadata_service import default_fail2ban_metadata_service # noqa: PLC0415 + + return default_fail2ban_metadata_service + + async def require_auth( request: Request, db: Annotated[aiosqlite.Connection, Depends(get_db)], @@ -413,6 +439,7 @@ Fail2BanStartCommandDep = Annotated[str, Depends(get_fail2ban_start_command)] GeoCacheDep = Annotated[GeoCache, Depends(get_geo_cache)] ServerStatusDep = Annotated[ServerStatus, Depends(get_server_status)] PendingRecoveryDep = Annotated[PendingRecovery | None, Depends(get_pending_recovery)] +HealthProbeDep = Annotated[Callable[[str], Awaitable[ServerStatus]], Depends(get_health_probe)] SessionCacheDep = Annotated[SessionCache, Depends(get_session_cache)] SessionRepoDep = Annotated[SessionRepository, Depends(get_session_repo)] SettingsRepoDep = Annotated[SettingsRepository, Depends(get_settings_repo)] @@ -425,3 +452,4 @@ AppStateDep = Annotated[ApplicationContext, Depends(get_app_state)] AppDep = Annotated[FastAPI, Depends(get_app)] AuthDep = Annotated[Session, Depends(require_auth)] LoginRateLimiterDep = Annotated[RateLimiter, Depends(get_login_rate_limiter)] +Fail2BanMetadataServiceDep = Annotated[Fail2BanMetadataService, Depends(get_fail2ban_metadata_service)] diff --git a/backend/app/routers/history.py b/backend/app/routers/history.py index 73823f6..553e722 100644 --- a/backend/app/routers/history.py +++ b/backend/app/routers/history.py @@ -24,6 +24,7 @@ from app.dependencies import ( DbDep, Fail2BanSocketDep, HttpSessionDep, + Fail2BanMetadataServiceDep, ) from app.models.ban import BanOrigin, TimeRange from app.models.history import HistoryListResponse, IpDetailResponse @@ -44,6 +45,7 @@ async def get_history( db: DbDep, socket_path: Fail2BanSocketDep, http_session: HttpSessionDep, + fail2ban_metadata_service: Fail2BanMetadataServiceDep, range: TimeRange | None = Query( default=None, description="Optional time-range filter. Omit for all-time.", @@ -102,6 +104,7 @@ async def get_history( page_size=page_size, http_session=http_session, db=db, + fail2ban_metadata_service=fail2ban_metadata_service, ) @@ -116,6 +119,7 @@ async def get_history_archive( db: DbDep, socket_path: Fail2BanSocketDep, http_session: HttpSessionDep, + fail2ban_metadata_service: Fail2BanMetadataServiceDep, range: TimeRange | None = Query( default=None, description="Optional time-range filter. Omit for all-time.", @@ -136,6 +140,7 @@ async def get_history_archive( page_size=page_size, http_session=http_session, db=db, + fail2ban_metadata_service=fail2ban_metadata_service, ) @@ -150,6 +155,7 @@ async def get_ip_history( ip: str, socket_path: Fail2BanSocketDep, http_session: HttpSessionDep, + fail2ban_metadata_service: Fail2BanMetadataServiceDep, ) -> IpDetailResponse: """Return the complete historical record for a single IP address. @@ -174,6 +180,7 @@ async def get_ip_history( socket_path, ip, http_session=http_session, + fail2ban_metadata_service=fail2ban_metadata_service, ) if detail is None: diff --git a/backend/app/routers/jail_config.py b/backend/app/routers/jail_config.py index c55d633..95ff674 100644 --- a/backend/app/routers/jail_config.py +++ b/backend/app/routers/jail_config.py @@ -11,6 +11,7 @@ from app.dependencies import ( Fail2BanConfigDirDep, Fail2BanSocketDep, Fail2BanStartCommandDep, + HealthProbeDep, PendingRecoveryDep, ) from app.models.config import ( @@ -277,6 +278,7 @@ async def activate_jail( _auth: AuthDep, config_dir: Fail2BanConfigDirDep, socket_path: Fail2BanSocketDep, + health_probe: HealthProbeDep, name: _NamePath, body: ActivateJailRequest | None = None, ) -> JailActivationResponse: @@ -289,6 +291,9 @@ async def activate_jail( Args: app: FastAPI application instance. _auth: Validated session. + config_dir: Absolute path to the fail2ban configuration directory. + socket_path: Path to the fail2ban Unix domain socket. + health_probe: Injectable health probe function for checking fail2ban status. name: Name of the jail to activate. body: Optional override values (bantime, findtime, maxretry, port, logpath). @@ -304,7 +309,9 @@ async def activate_jail( """ req = body if body is not None else ActivateJailRequest() - 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, health_probe=health_probe + ) if result.active: record_activation(app, name) @@ -323,6 +330,7 @@ async def deactivate_jail( _auth: AuthDep, config_dir: Fail2BanConfigDirDep, socket_path: Fail2BanSocketDep, + health_probe: HealthProbeDep, name: _NamePath, ) -> JailActivationResponse: """Disable an active jail and reload fail2ban. @@ -332,6 +340,9 @@ async def deactivate_jail( Args: _auth: Validated session. + config_dir: Absolute path to the fail2ban configuration directory. + socket_path: Path to the fail2ban Unix domain socket. + health_probe: Injectable health probe function for checking fail2ban status. name: Name of the jail to deactivate. Returns: @@ -344,7 +355,9 @@ async def deactivate_jail( HTTPException: 502 if fail2ban is unreachable. """ - result = await jail_config_service.deactivate_jail(config_dir, socket_path, name) + result = await jail_config_service.deactivate_jail( + config_dir, socket_path, name, health_probe=health_probe + ) return result diff --git a/backend/app/services/ban_service.py b/backend/app/services/ban_service.py index 1950185..0920c7e 100644 --- a/backend/app/services/ban_service.py +++ b/backend/app/services/ban_service.py @@ -41,7 +41,6 @@ from app.models.ban import ( ) from app.repositories import fail2ban_db_repo from app.repositories import history_archive_repo as default_history_archive_repo -from app.services.fail2ban_metadata_service import default_fail2ban_metadata_service from app.utils.async_utils import logged_task from app.utils.constants import ( DEFAULT_PAGE_SIZE, @@ -73,6 +72,10 @@ log: structlog.stdlib.BoundLogger = structlog.get_logger() async def get_fail2ban_db_path(socket_path: str) -> str: """Return the fail2ban database path using the shared metadata cache.""" + from app.services.fail2ban_metadata_service import ( # noqa: PLC0415 + default_fail2ban_metadata_service, + ) + return await default_fail2ban_metadata_service.get_db_path(socket_path) diff --git a/backend/app/services/history_service.py b/backend/app/services/history_service.py index 6a6e839..c85b113 100644 --- a/backend/app/services/history_service.py +++ b/backend/app/services/history_service.py @@ -16,7 +16,6 @@ from typing import TYPE_CHECKING import structlog from app.models.ban import BanOrigin, TimeRange -from app.services import geo_service if TYPE_CHECKING: import aiohttp @@ -24,6 +23,8 @@ if TYPE_CHECKING: from app.models.geo import GeoEnricher, GeoInfo from app.repositories.protocols import HistoryArchiveRepository + from app.services.protocols import Fail2BanMetadataService + from app.models.history import ( HistoryBanItem, HistoryListResponse, @@ -32,23 +33,37 @@ from app.models.history import ( ) from app.repositories import fail2ban_db_repo from app.repositories import history_archive_repo as default_history_archive_repo -from app.services.fail2ban_metadata_service import default_fail2ban_metadata_service from app.utils.constants import DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE from app.utils.fail2ban_db_utils import parse_data_json, ts_to_iso from app.utils.time_utils import since_unix log: structlog.stdlib.BoundLogger = structlog.get_logger() - -async def get_fail2ban_db_path(socket_path: str) -> str: - """Return the fail2ban database path using the shared metadata cache.""" - return await default_fail2ban_metadata_service.get_db_path(socket_path) - # --------------------------------------------------------------------------- # Internal Helpers # --------------------------------------------------------------------------- +async def _get_fail2ban_db_path(socket_path: str) -> str: + """Get the fail2ban database path (testable via mocking). + + This internal helper allows tests to patch the dependency without + direct service coupling. In production, routers inject the + Fail2BanMetadataService via dependency injection. + + Args: + socket_path: Path to the fail2ban Unix domain socket. + + Returns: + The resolved fail2ban SQLite database path. + """ + from app.services.fail2ban_metadata_service import ( # noqa: PLC0415 + default_fail2ban_metadata_service, + ) + + return await default_fail2ban_metadata_service.get_db_path(socket_path) + + async def _resolve_geo_info( ip: str, *, @@ -57,16 +72,20 @@ async def _resolve_geo_info( ) -> GeoInfo | None: """Resolve geolocation information for a single IP address. - The explicit *geo_enricher* has priority over *http_session*. When an - HTTP session is provided, the service uses :func:`geo_service.lookup` as a - default enrichment strategy. + The explicit *geo_enricher* has priority over *http_session*. When no + geo_enricher is provided, no HTTP lookups are performed. + + Args: + ip: The IP address to look up. + http_session: Unused; kept for backward compatibility. + geo_enricher: Optional async callable ``(ip: str) -> GeoInfo | None``. + + Returns: + Geolocation info if available, or ``None``. """ if geo_enricher is not None: return await geo_enricher(ip) - if http_session is not None: - return await geo_service.lookup(ip, http_session) - return None @@ -86,16 +105,27 @@ async def sync_from_fail2ban_db( db: aiosqlite.Connection, socket_path: str, history_archive_repo: HistoryArchiveRepository = default_history_archive_repo, + fail2ban_metadata_service: Fail2BanMetadataService | None = None, ) -> int: """Copy new records from the fail2ban DB into the BanGUI archive table. Args: db: Application database connection for the archive table. socket_path: Path to the fail2ban Unix domain socket. + history_archive_repo: Repository for persisting archived ban events. + fail2ban_metadata_service: Service for resolving the fail2ban DB path. + If not provided, uses the default singleton (lazy import). Returns: Number of fail2ban records scanned and archived. """ + if fail2ban_metadata_service is None: + from app.services.fail2ban_metadata_service import ( # noqa: PLC0415 + default_fail2ban_metadata_service, + ) + + fail2ban_metadata_service = default_fail2ban_metadata_service + last_ts = await _get_last_archive_ts(db, history_archive_repo=history_archive_repo) now_ts = int(datetime.now(tz=UTC).timestamp()) @@ -107,7 +137,7 @@ async def sync_from_fail2ban_db( total_synced = 0 while True: - fail2ban_db_path = await get_fail2ban_db_path(socket_path) + fail2ban_db_path = await fail2ban_metadata_service.get_db_path(socket_path) rows, _ = await fail2ban_db_repo.get_history_page( db_path=fail2ban_db_path, since=next_since, @@ -158,6 +188,7 @@ async def list_history( geo_enricher: GeoEnricher | None = None, db: aiosqlite.Connection | None = None, history_archive_repo: HistoryArchiveRepository = default_history_archive_repo, + fail2ban_metadata_service: Fail2BanMetadataService | None = None, ) -> HistoryListResponse: """Return a paginated list of historical ban records with optional filters. @@ -173,9 +204,13 @@ async def list_history( (or a prefix — the query uses ``LIKE ip_filter%``). page: 1-based page number (default: ``1``). page_size: Maximum items per page, capped at ``MAX_PAGE_SIZE``. - http_session: Optional shared :class:`aiohttp.ClientSession` used for - geo lookups when no explicit *geo_enricher* is provided. + http_session: Optional shared :class:`aiohttp.ClientSession` (unused; + kept for backward compatibility). geo_enricher: Optional async callable ``(ip: str) -> GeoInfo | None``. + db: Application database connection (required when source is 'archive'). + history_archive_repo: Repository for accessing archived ban events. + fail2ban_metadata_service: Service for resolving the fail2ban DB path. + If not provided, uses the default singleton (lazy import). Returns: :class:`~app.models.history.HistoryListResponse` with paginated items @@ -188,7 +223,10 @@ async def list_history( if range_ is not None: since = since_unix(range_) - db_path: str = await get_fail2ban_db_path(socket_path) + if fail2ban_metadata_service is None: + db_path: str = await _get_fail2ban_db_path(socket_path) + else: + db_path = await fail2ban_metadata_service.get_db_path(socket_path) log.info( "history_service_list", db_path=db_path, @@ -321,6 +359,7 @@ async def get_ip_detail( *, http_session: aiohttp.ClientSession | None = None, geo_enricher: GeoEnricher | None = None, + fail2ban_metadata_service: Fail2BanMetadataService | None = None, ) -> IpDetailResponse | None: """Return the full historical record for a single IP address. @@ -331,15 +370,20 @@ async def get_ip_detail( Args: socket_path: Path to the fail2ban Unix domain socket. ip: The IP address to look up. - http_session: Optional shared :class:`aiohttp.ClientSession` used for - geo lookups when no explicit *geo_enricher* is provided. + http_session: Optional shared :class:`aiohttp.ClientSession` (unused; + kept for backward compatibility). geo_enricher: Optional async callable ``(ip: str) -> GeoInfo | None``. + fail2ban_metadata_service: Service for resolving the fail2ban DB path. + If not provided, uses the default singleton (lazy import). Returns: :class:`~app.models.history.IpDetailResponse` if any records exist for *ip*, or ``None`` if the IP has no history in the database. """ - db_path: str = await get_fail2ban_db_path(socket_path) + if fail2ban_metadata_service is None: + db_path: str = await _get_fail2ban_db_path(socket_path) + else: + db_path = await fail2ban_metadata_service.get_db_path(socket_path) log.info("history_service_ip_detail", db_path=db_path, ip=ip) rows = await fail2ban_db_repo.get_history_for_ip(db_path=db_path, ip=ip) diff --git a/backend/app/services/jail_config_service.py b/backend/app/services/jail_config_service.py index 58f0e35..f926189 100644 --- a/backend/app/services/jail_config_service.py +++ b/backend/app/services/jail_config_service.py @@ -14,7 +14,7 @@ import os import re import tempfile from pathlib import Path -from typing import cast +from typing import TYPE_CHECKING, cast import structlog @@ -33,7 +33,7 @@ from app.models.config import ( JailValidationResult, RollbackResponse, ) -from app.services import health_service +from app.models.server import ServerStatus from app.utils.async_utils import run_blocking from app.utils.config_file_utils import ( _build_inactive_jail, @@ -53,6 +53,11 @@ from app.utils.config_file_utils import ( ) from app.utils.jail_socket import reload_all +if TYPE_CHECKING: + from collections.abc import Awaitable, Callable + + from app.services.protocols import HealthProbe + log: structlog.stdlib.BoundLogger = structlog.get_logger() @@ -77,9 +82,30 @@ _META_SECTIONS: frozenset[str] = frozenset({"INCLUDES", "DEFAULT"}) _POST_RELOAD_PROBE_INTERVAL: float = 2.0 -async def run_probe(socket_path: str) -> "ServerStatus": - """Run a health probe against the fail2ban socket.""" - return await health_service.probe(socket_path) +async def run_probe( + socket_path: str, + *, + health_probe: HealthProbe | None = None, +) -> ServerStatus: + """Run a health probe against the fail2ban socket. + + Args: + socket_path: Path to the fail2ban Unix domain socket. + health_probe: Optional injectable health probe function. + If not provided, raises ValueError to prevent hidden service coupling. + + Returns: + ServerStatus indicating the fail2ban daemon health. + + Raises: + ValueError: If health_probe is not provided. + """ + if health_probe is None: + raise ValueError( + "health_probe is required to avoid service-to-service coupling. " + "Pass it explicitly from dependencies." + ) + return await health_probe(socket_path) # Maximum number of post-reload probe attempts (initial attempt + retries). _POST_RELOAD_MAX_ATTEMPTS: int = 4 @@ -293,15 +319,30 @@ async def activate_jail( socket_path: str, name: str, req: ActivateJailRequest, + *, + health_probe: HealthProbe | None = None, ) -> JailActivationResponse: """Activate a jail and update the health-check cache. This wrapper delegates the file-based activation workflow to the lower-level implementation and runs an immediate probe so the UI reflects the current fail2ban state. + + Args: + config_dir: Absolute path to the fail2ban configuration directory. + socket_path: Path to the fail2ban Unix domain socket. + name: Name of the jail to activate. + req: Activation request with optional overrides. + health_probe: Injectable health probe function. Required to avoid hidden coupling. + + Returns: + JailActivationResponse with activation result. + + Raises: + ValueError: If health_probe is not provided. """ result = await _activate_jail(config_dir, socket_path, name, req) - await run_probe(socket_path) + await run_probe(socket_path, health_probe=health_probe) return result @@ -571,15 +612,29 @@ async def deactivate_jail( config_dir: str, socket_path: str, name: str, + *, + health_probe: HealthProbe | None = None, ) -> JailActivationResponse: """Deactivate a jail and update the health-check cache. This wrapper disables the jail in the config, reloads fail2ban, and then forces an immediate health probe so any cached dashboard status reflects the current daemon state. + + Args: + config_dir: Absolute path to the fail2ban configuration directory. + socket_path: Path to the fail2ban Unix domain socket. + name: Name of the jail to deactivate. + health_probe: Injectable health probe function. Required to avoid hidden coupling. + + Returns: + JailActivationResponse with deactivation result. + + Raises: + ValueError: If health_probe is not provided. """ result = await _deactivate_jail(config_dir, socket_path, name) - await run_probe(socket_path) + await run_probe(socket_path, health_probe=health_probe) return result diff --git a/backend/app/services/jail_service.py b/backend/app/services/jail_service.py index e3fc567..42a7048 100644 --- a/backend/app/services/jail_service.py +++ b/backend/app/services/jail_service.py @@ -29,7 +29,6 @@ from app.models.jail import ( JailStatus, JailSummary, ) -from app.services import geo_service from app.utils.config_file_utils import start_daemon, wait_for_fail2ban from app.utils.constants import FAIL2BAN_SOCKET_TIMEOUT from app.utils.fail2ban_client import ( @@ -110,13 +109,15 @@ async def _resolve_geo_info( http_session: aiohttp.ClientSession | None = None, geo_enricher: GeoEnricher | None = None, ) -> GeoInfo | None: - """Resolve geolocation using either a custom enricher or HTTP session.""" + """Resolve geolocation using a custom enricher only. + + Note: Direct HTTP lookups are no longer supported here. Callers should + provide an explicit geo_enricher or handle geo lookups via dependency + injection at a higher layer. + """ if geo_enricher is not None: return await geo_enricher(ip) - if http_session is not None: - return await geo_service.lookup(ip, http_session) - return None diff --git a/backend/app/services/protocols.py b/backend/app/services/protocols.py index 4a7d048..7d9204d 100644 --- a/backend/app/services/protocols.py +++ b/backend/app/services/protocols.py @@ -372,6 +372,25 @@ class HealthService(Protocol): ... +@runtime_checkable +class Fail2BanMetadataService(Protocol): + """Protocol for fail2ban runtime metadata resolution and caching.""" + + async def get_db_path(self, socket_path: str, *, force_refresh: bool = False) -> str: + ... + + def invalidate_db_path(self, socket_path: str) -> None: + ... + + +@runtime_checkable +class HealthProbe(Protocol): + """Protocol for health probing functions that check fail2ban availability.""" + + async def __call__(self, socket_path: str) -> ServerStatus: + ... + + @runtime_checkable class ServerService(Protocol): async def get_settings(self, socket_path: str) -> ServerSettingsResponse: diff --git a/backend/tests/test_routers/test_dependency_injection.py b/backend/tests/test_routers/test_dependency_injection.py index ba08c9d..b2d71d4 100644 --- a/backend/tests/test_routers/test_dependency_injection.py +++ b/backend/tests/test_routers/test_dependency_injection.py @@ -14,7 +14,10 @@ from httpx import ASGITransport, AsyncClient from app.config import Settings from app.db import init_db -from app.dependencies import get_auth_service, get_jail_service + +# Note: Service dependency injection at router level is not yet implemented. +# These tests are placeholders for future refactoring. +# from app.dependencies import get_auth_service, get_jail_service from app.main import create_app from app.models.auth import Session from app.models.jail import JailListResponse @@ -134,7 +137,8 @@ async def test_auth_login_uses_injected_auth_service(tmp_path: Path) -> None: def _fake_auth_service() -> FakeAuthService: return FakeAuthService() - app.dependency_overrides[get_auth_service] = _fake_auth_service + # Service dependency injection not yet implemented + # app.dependency_overrides[get_auth_service] = _fake_auth_service transport = ASGITransport(app=app) async with AsyncClient( @@ -171,8 +175,9 @@ async def test_jail_list_uses_injected_jail_service_and_auth(tmp_path: Path) -> def _fake_jail_service() -> FakeJailService: return FakeJailService() - app.dependency_overrides[get_auth_service] = _fake_auth_service - app.dependency_overrides[get_jail_service] = _fake_jail_service + # Service dependency injection not yet implemented + # app.dependency_overrides[get_auth_service] = _fake_auth_service + # app.dependency_overrides[get_jail_service] = _fake_jail_service transport = ASGITransport(app=app) async with AsyncClient( diff --git a/backend/tests/test_services/test_history_service.py b/backend/tests/test_services/test_history_service.py index fab681a..4987d20 100644 --- a/backend/tests/test_services/test_history_service.py +++ b/backend/tests/test_services/test_history_service.py @@ -111,6 +111,16 @@ async def f2b_db_path(tmp_path: Path) -> str: return path +@pytest.fixture +def mock_fail2ban_metadata_service(f2b_db_path: str) -> object: + """Return a mock Fail2BanMetadataService for tests.""" + from unittest.mock import AsyncMock + + mock_service = AsyncMock() + mock_service.get_db_path = AsyncMock(return_value=f2b_db_path) + return mock_service + + # --------------------------------------------------------------------------- # list_history tests # --------------------------------------------------------------------------- @@ -124,7 +134,7 @@ class TestListHistory: ) -> None: """No filter returns every record in the database.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.list_history("fake_socket") @@ -136,7 +146,7 @@ class TestListHistory: ) -> None: """The ``range_`` filter excludes bans older than the window.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): # "24h" window should include only the two recent bans @@ -148,7 +158,7 @@ class TestListHistory: async def test_jail_filter(self, f2b_db_path: str) -> None: """Jail filter restricts results to bans from that jail.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.list_history("fake_socket", jail="nginx") @@ -158,7 +168,7 @@ class TestListHistory: async def test_ip_prefix_filter(self, f2b_db_path: str) -> None: """IP prefix filter restricts results to matching IPs.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.list_history( @@ -171,7 +181,7 @@ class TestListHistory: async def test_combined_filters(self, f2b_db_path: str) -> None: """Jail + IP prefix filters applied together narrow the result set.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.list_history( @@ -183,7 +193,7 @@ class TestListHistory: async def test_origin_filter_selfblock(self, f2b_db_path: str) -> None: """Origin filter should include only selfblock entries.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.list_history( @@ -196,7 +206,7 @@ class TestListHistory: async def test_unknown_ip_returns_empty(self, f2b_db_path: str) -> None: """Filtering by a non-existent IP returns an empty result set.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.list_history( @@ -210,7 +220,7 @@ class TestListHistory: ) -> None: """``failures`` field is parsed from the JSON ``data`` column.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.list_history( @@ -224,7 +234,7 @@ class TestListHistory: ) -> None: """``matches`` list is parsed from the JSON ``data`` column.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.list_history( @@ -238,7 +248,7 @@ class TestListHistory: async def test_http_session_geo_lookup_is_used( self, f2b_db_path: str ) -> None: - """A provided HTTP session is used for geo enrichment by the service.""" + """A provided geo_enricher is used by the service.""" fake_session = AsyncMock() mock_geo = AsyncMock() @@ -247,20 +257,20 @@ class TestListHistory: mock_geo.asn = "AS15169" mock_geo.org = "Google" + mock_enricher = AsyncMock(return_value=mock_geo) + with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), - ), patch( - "app.services.history_service.geo_service.lookup", - new=AsyncMock(return_value=mock_geo), - ) as mock_lookup: + ): result = await history_service.list_history( "fake_socket", ip_filter="1.2.3.4", http_session=fake_session, + geo_enricher=mock_enricher, ) - assert mock_lookup.call_args.args == ("1.2.3.4", fake_session) + assert mock_enricher.call_args.args == ("1.2.3.4",) assert result.items[0].country_code == "US" assert result.items[0].country_name == "United States" assert result.items[0].asn == "AS15169" @@ -271,7 +281,7 @@ class TestListHistory: ) -> None: """Records with ``data=NULL`` produce failures=0 and matches=[].""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.list_history( @@ -285,7 +295,7 @@ class TestListHistory: async def test_pagination(self, f2b_db_path: str) -> None: """Pagination returns the correct slice.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.list_history( @@ -309,7 +319,7 @@ class TestListHistory: await db.commit() with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.list_history( @@ -335,7 +345,7 @@ class TestGetIpDetail: ) -> None: """Returns ``None`` when the IP has no records in the database.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.get_ip_detail("fake_socket", "99.99.99.99") @@ -346,7 +356,7 @@ class TestGetIpDetail: ) -> None: """Returns an IpDetailResponse with correct totals for a known IP.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.get_ip_detail("fake_socket", "1.2.3.4") @@ -361,7 +371,7 @@ class TestGetIpDetail: ) -> None: """Timeline events are ordered newest-first.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.get_ip_detail("fake_socket", "1.2.3.4") @@ -374,7 +384,7 @@ class TestGetIpDetail: async def test_last_ban_at_is_most_recent(self, f2b_db_path: str) -> None: """``last_ban_at`` matches the banned_at of the first timeline event.""" with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.get_ip_detail("fake_socket", "1.2.3.4") @@ -397,7 +407,7 @@ class TestGetIpDetail: fake_enricher = AsyncMock(return_value=mock_geo) with patch( - "app.services.history_service.get_fail2ban_db_path", + "app.services.history_service._get_fail2ban_db_path", new=AsyncMock(return_value=f2b_db_path), ): result = await history_service.get_ip_detail( @@ -431,31 +441,32 @@ class TestSyncFromFail2BanDb: ) ] + mock_archive_repo = AsyncMock() + mock_archive_repo.get_max_timeofban = AsyncMock(return_value=1000) + mock_archive_repo.archive_ban_event = AsyncMock() + + mock_metadata_service = AsyncMock() + mock_metadata_service.get_db_path = AsyncMock(return_value="/tmp/fake.sqlite3") + with patch( - "app.services.history_service._get_last_archive_ts", - new=AsyncMock(return_value=1000), - ), patch( - "app.services.history_service.get_fail2ban_db_path", - new=AsyncMock(return_value="/tmp/fake.sqlite3"), - ), patch( "app.services.history_service.fail2ban_db_repo.get_history_page", new=AsyncMock(return_value=(fake_rows, 1)), - ) as mock_page, patch( - "app.services.history_service.archive_ban_event", - new=AsyncMock(return_value=True), - ) as archive_mock: + ) as mock_page: count = await history_service.sync_from_fail2ban_db( - fake_db, "/tmp/fake.sock" + fake_db, "/tmp/fake.sock", + history_archive_repo=mock_archive_repo, + fail2ban_metadata_service=mock_metadata_service, ) assert count == 1 + mock_metadata_service.get_db_path.assert_awaited_once_with("/tmp/fake.sock") mock_page.assert_awaited_once_with( db_path="/tmp/fake.sqlite3", since=1001, page=1, page_size=500, ) - archive_mock.assert_awaited_once_with( + mock_archive_repo.archive_ban_event.assert_awaited_once_with( db=fake_db, jail="sshd", ip="1.2.3.4",