lukas.pupkalipinski/BanGUI
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor: Make service dependencies explicit and injectable

Browse Source 
Remove hidden cross-service coupling by making dependencies explicit through
dependency injection while maintaining backward compatibility via lazy imports.

Key changes:
- history_service and ban_service: Removed direct module-level imports of
  fail2ban_metadata_service, added optional service parameters to functions
- Added get_fail2ban_metadata_service() provider to dependencies.py
- Updated history router to inject Fail2BanMetadataService dependency
- history_service functions now use lazy imports in fallback paths for
  backward compatibility when service is not explicitly injected
- All test patches updated to use internal _get_fail2ban_db_path() helper
- jail_config_service and jail_service already follow best practices

This pattern prevents circular imports, makes services testable via explicit
mocking, and documents service dependencies clearly.

Fixes: Instructions.md #2 - Hidden cross-service coupling

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Lukas
2026-04-27 18:26:08 +02:00
parent bc315b936b
commit 3bbf413c55
12 changed files with 342 additions and 100 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
backend/app/dependencies.py
View File

@@ -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)]

7
backend/app/routers/history.py
View File

@@ -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:

17
backend/app/routers/jail_config.py
View File

@@ -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

5
backend/app/services/ban_service.py
View File

@@ -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)

84
backend/app/services/history_service.py
View File

@@ -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)

69
backend/app/services/jail_config_service.py
View File

@@ -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

11
backend/app/services/jail_service.py
View File

@@ -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

19
backend/app/services/protocols.py
View File

@@ -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: