From 1a3401f418a7fd4b0d1df1ee5ee61c06cd641866 Mon Sep 17 00:00:00 2001 From: Lukas Date: Sat, 25 Apr 2026 18:53:47 +0200 Subject: [PATCH] T-10: Fix get_geo_batch_lookup for proper injection with GeoCache instance Instead of returning a bound method (geo_cache.lookup_batch), now inject the GeoCache instance directly into routers and services. This provides proper runtime isolation since T-04 made GeoCache a proper object. Changes: - Remove get_geo_batch_lookup() dependency provider - Add GeoCacheDep type alias for injecting GeoCache instances - Update all routers (bans, blocklist, dashboard, jails) to use GeoCacheDep - Update ban_service, blocklist_service, jail_service to accept GeoCache - Update service protocols to match new signatures - Update docstrings to reference GeoCache methods instead of module functions All callers now call geo_cache.lookup_batch(...) directly instead of geo_batch_lookup(...), providing real dependency injection with proper testing isolation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Docs/Tasks.md | 47 ----------------------- backend/app/dependencies.py | 14 ++----- backend/app/routers/bans.py | 6 +-- backend/app/routers/blocklist.py | 8 ++-- backend/app/routers/dashboard.py | 12 +++--- backend/app/routers/jails.py | 6 +-- backend/app/services/ban_service.py | 29 +++++++------- backend/app/services/blocklist_service.py | 12 +++--- backend/app/services/jail_service.py | 11 +++--- backend/app/services/protocols.py | 7 ++-- 10 files changed, 51 insertions(+), 101 deletions(-) diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 7020181..b05cc9e 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,50 +1,3 @@ -### T-08 · `_SOCKET_TIMEOUT` defined 6× — `constants.py` constant unused - -**Where found:** `backend/app/utils/constants.py:16` (defines `FAIL2BAN_SOCKET_TIMEOUT_SECONDS = 5.0` but is never imported); `ban_service.py` (5.0), `jail_service.py` (10.0), `config_service.py` (10.0), `server_service.py` (10.0), `log_service.py` (10.0), `jail_config_service.py` (10.0), `config_file_utils.py` (10.0) - -**Why this is needed:** `constants.py` has a module docstring saying "import from this module rather than hard-coding values" — and then nothing imports the socket timeout. The values also disagree: `ban_service` uses `5.0` while all others use `10.0`, creating silent inconsistency in timeout behaviour across endpoints. - -**Goal:** One constant in `constants.py`, imported everywhere. Decide on a single default (or two named constants for fast/slow operations). - -**What to do:** -1. In `constants.py`, define `FAIL2BAN_SOCKET_TIMEOUT_FAST: Final[float] = 5.0` (for health/metadata probes) and `FAIL2BAN_SOCKET_TIMEOUT: Final[float] = 10.0` (for command operations) — or a single value if appropriate. -2. Replace all local `_SOCKET_TIMEOUT` float literals with imports from `constants`. -3. Confirm `ban_service` should actually be `5.0` or `10.0` (intentional vs accidental discrepancy). - -**Possible traps and issues:** -- Changing `ban_service` from 5.0 to 10.0 (or vice versa) changes observable timeout behaviour. Decide deliberately. -- `fail2ban_metadata_service.py` also has `socket_timeout: float = 5.0` hardcoded inline — include this in the sweep. - -**Docs changes needed:** None. - -**Doc references:** `backend/app/utils/constants.py` - ---- - -### T-09 · `_since_unix` has two divergent implementations — latent window-boundary bug - -**Where found:** `backend/app/services/ban_service.py:393` (uses `time.time()`, applies `_TIME_RANGE_SLACK_SECONDS = 60`); `backend/app/services/history_service.py:53` (uses `datetime.now(UTC).timestamp()`, no slack) - -**Why this is needed:** The `ban_service` docstring explicitly explains why `time.time()` is used and why the 60-second slack is needed for consistency with fail2ban's timestamp storage. `history_service` quietly omits both, so history queries return a slightly different window boundary than ban queries for the same `TimeRange` parameter. Inconsistent windows cause "same data, different count" bugs in the UI. - -**Goal:** Single `_since_unix` function in a shared utility module, with documented rationale, used by both services. - -**What to do:** -1. Move `_since_unix` (with the `time.time()` + slack approach) to `app/utils/time_utils.py` (which already exists). -2. Delete both local implementations. -3. Import from `time_utils` in both services. -4. Decide consciously whether history queries should also use the slack (probably yes for consistency). - -**Possible traps and issues:** -- Adding the 60-second slack to history queries may change row counts in tests that assert exact counts against seeded timestamps. -- Confirm `_TIME_RANGE_SLACK_SECONDS` belongs in `constants.py`. - -**Docs changes needed:** `Docs/Backend-Development.md` — document the timestamp handling rationale. - -**Doc references:** `backend/app/utils/time_utils.py`, `backend/app/utils/constants.py` - ---- - ### T-10 · `get_geo_batch_lookup` is false injectability — module function pointer injection **Where found:** `backend/app/dependencies.py` — `get_geo_batch_lookup()` returns `geo_service.lookup_batch` (a module-level function) diff --git a/backend/app/dependencies.py b/backend/app/dependencies.py index 6334c1f..d43a6db 100644 --- a/backend/app/dependencies.py +++ b/backend/app/dependencies.py @@ -20,7 +20,6 @@ from fastapi import Depends, FastAPI, HTTPException, Request, status from app.config import Settings from app.models.auth import Session from app.models.config import PendingRecovery -from app.models.geo import GeoBatchLookup from app.models.server import ServerStatus from app.repositories.protocols import ( BlocklistRepository, @@ -72,7 +71,7 @@ def _session_cache_enabled(settings: Settings) -> bool: def _build_app_context(request: Request) -> ApplicationContext: - state = cast(ApplicationState, request.app.state) + state = cast("ApplicationState", request.app.state) session_cache = getattr(state, "session_cache", None) if session_cache is None: session_cache = NoOpSessionCache() @@ -194,15 +193,10 @@ async def get_fail2ban_start_command(settings: Settings = Depends(get_settings)) return settings.fail2ban_start_command -async def get_geo_batch_lookup(request: Request) -> GeoBatchLookup: - """Provide the geo batch lookup method from the application's GeoCache instance.""" - geo_cache: GeoCache = request.app.state.geo_cache - return geo_cache.lookup_batch # type: ignore[return-value] - - async def get_geo_cache(request: Request) -> GeoCache: """Provide the application's GeoCache instance.""" - return request.app.state.geo_cache + geo_cache: GeoCache = cast("GeoCache", request.app.state.geo_cache) + return geo_cache async def get_session_cache(app_context: Annotated[ApplicationContext, Depends(get_app_context)]) -> SessionCache: @@ -365,7 +359,7 @@ SchedulerDep = Annotated[AsyncIOScheduler, Depends(get_scheduler)] Fail2BanSocketDep = Annotated[str, Depends(get_fail2ban_socket)] Fail2BanConfigDirDep = Annotated[str, Depends(get_fail2ban_config_dir)] Fail2BanStartCommandDep = Annotated[str, Depends(get_fail2ban_start_command)] -GeoBatchLookupDep = Annotated[GeoBatchLookup, Depends(get_geo_batch_lookup)] +GeoCacheDep = Annotated[GeoCache, Depends(get_geo_cache)] ServerStatusDep = Annotated[ServerStatus, Depends(get_server_status)] PendingRecoveryDep = Annotated[PendingRecovery | None, Depends(get_pending_recovery)] SessionCacheDep = Annotated[SessionCache, Depends(get_session_cache)] diff --git a/backend/app/routers/bans.py b/backend/app/routers/bans.py index 1aa9777..9eb0c3a 100644 --- a/backend/app/routers/bans.py +++ b/backend/app/routers/bans.py @@ -16,7 +16,7 @@ from app.dependencies import ( AuthDep, DbDep, Fail2BanSocketDep, - GeoBatchLookupDep, + GeoCacheDep, HttpSessionDep, ) from app.models.ban import ActiveBanListResponse, BanRequest, UnbanAllResponse, UnbanRequest @@ -37,7 +37,7 @@ async def get_active_bans( db: DbDep, socket_path: Fail2BanSocketDep, http_session: HttpSessionDep, - geo_batch_lookup: GeoBatchLookupDep, + geo_cache: GeoCacheDep, ) -> ActiveBanListResponse: """Return every IP that is currently banned across all fail2ban jails. @@ -56,7 +56,7 @@ async def get_active_bans( """ return await ban_service.get_active_bans( socket_path, - geo_batch_lookup=geo_batch_lookup, + geo_cache=geo_cache, http_session=http_session, app_db=db, ) diff --git a/backend/app/routers/blocklist.py b/backend/app/routers/blocklist.py index 32bf764..f0dc1d0 100644 --- a/backend/app/routers/blocklist.py +++ b/backend/app/routers/blocklist.py @@ -28,7 +28,7 @@ from app.dependencies import ( AuthDep, DbDep, Fail2BanSocketDep, - GeoBatchLookupDep, + GeoCacheDep, HttpSessionDep, SchedulerDep, SettingsDep, @@ -118,7 +118,7 @@ async def run_import_now( db: DbDep, _auth: AuthDep, socket_path: Fail2BanSocketDep, - geo_batch_lookup: GeoBatchLookupDep, + geo_cache: GeoCacheDep, ) -> ImportRunResult: """Download and apply all enabled blocklist sources immediately. @@ -136,8 +136,8 @@ async def run_import_now( db, http_session, socket_path, - geo_is_cached=geo_service.is_cached, - geo_batch_lookup=geo_batch_lookup, + geo_is_cached=geo_cache.is_cached, + geo_cache=geo_cache, ban_ip=ban_service.ban_ip, ) diff --git a/backend/app/routers/dashboard.py b/backend/app/routers/dashboard.py index 860d913..15e2367 100644 --- a/backend/app/routers/dashboard.py +++ b/backend/app/routers/dashboard.py @@ -21,7 +21,7 @@ from app.dependencies import ( AuthDep, DbDep, Fail2BanSocketDep, - GeoBatchLookupDep, + GeoCacheDep, HttpSessionDep, ServerStatusDep, ) @@ -84,7 +84,7 @@ async def get_dashboard_bans( db: DbDep, socket_path: Fail2BanSocketDep, http_session: HttpSessionDep, - geo_batch_lookup: GeoBatchLookupDep, + geo_cache: GeoCacheDep, range: TimeRange = Query(default=_DEFAULT_RANGE, description="Time-range preset."), source: Literal["fail2ban", "archive"] = Query( default="fail2ban", @@ -125,7 +125,7 @@ async def get_dashboard_bans( page_size=page_size, http_session=http_session, app_db=db, - geo_batch_lookup=geo_batch_lookup, + geo_cache=geo_cache, origin=origin, ) @@ -140,7 +140,7 @@ async def get_bans_by_country( db: DbDep, socket_path: Fail2BanSocketDep, http_session: HttpSessionDep, - geo_batch_lookup: GeoBatchLookupDep, + geo_cache: GeoCacheDep, range: TimeRange = Query(default=_DEFAULT_RANGE, description="Time-range preset."), source: Literal["fail2ban", "archive"] = Query( default="fail2ban", @@ -177,8 +177,8 @@ async def get_bans_by_country( range, source=source, http_session=http_session, - geo_cache_lookup=geo_service.lookup_cached_only, - geo_batch_lookup=geo_batch_lookup, + geo_cache_lookup=geo_cache.lookup_cached_only, + geo_cache=geo_cache, app_db=db, origin=origin, country_code=country_code, diff --git a/backend/app/routers/jails.py b/backend/app/routers/jails.py index f5ab79b..fde6103 100644 --- a/backend/app/routers/jails.py +++ b/backend/app/routers/jails.py @@ -27,7 +27,7 @@ from app.dependencies import ( AuthDep, DbDep, Fail2BanSocketDep, - GeoBatchLookupDep, + GeoCacheDep, HttpSessionDep, ) from app.exceptions import ( @@ -427,7 +427,7 @@ async def get_jail_banned_ips( name: _NamePath, socket_path: Fail2BanSocketDep, http_session: HttpSessionDep, - geo_batch_lookup: GeoBatchLookupDep, + geo_cache: GeoCacheDep, page: int = 1, page_size: int = 25, search: str | None = None, @@ -470,7 +470,7 @@ async def get_jail_banned_ips( page=page, page_size=page_size, search=search, - geo_batch_lookup=geo_batch_lookup, + geo_cache=geo_cache, http_session=http_session, app_db=db, ) diff --git a/backend/app/services/ban_service.py b/backend/app/services/ban_service.py index 96d1744..1e61a1c 100644 --- a/backend/app/services/ban_service.py +++ b/backend/app/services/ban_service.py @@ -61,7 +61,8 @@ if TYPE_CHECKING: import aiohttp import aiosqlite - from app.models.geo import GeoBatchLookup, GeoCacheLookup, GeoEnricher, GeoInfo + from app.models.geo import GeoCacheLookup, GeoEnricher, GeoInfo + from app.services.geo_cache import GeoCache from app.repositories.protocols import HistoryArchiveRepository log: structlog.stdlib.BoundLogger = structlog.get_logger() @@ -214,7 +215,7 @@ async def _enrich_bans( async def get_active_bans( socket_path: str, - geo_batch_lookup: GeoBatchLookup | None = None, + geo_cache: GeoCache | None = None, geo_enricher: GeoEnricher | None = None, http_session: aiohttp.ClientSession | None = None, app_db: aiosqlite.Connection | None = None, @@ -227,7 +228,7 @@ async def get_active_bans( Geo enrichment strategy (highest priority first): 1. When *http_session* is provided the entire set of banned IPs is resolved - in a single :func:`~app.services.geo_service.lookup_batch` call (up to + in a single :meth:`GeoCache.lookup_batch` call (up to 100 IPs per HTTP request). This is far more efficient than concurrent per-IP lookups and stays within ip-api.com rate limits. 2. When only *geo_enricher* is provided (legacy / test path) each IP is @@ -239,7 +240,7 @@ async def get_active_bans( Used to enrich each ban entry with country and ASN data. Ignored when *http_session* is provided. http_session: Optional shared :class:`aiohttp.ClientSession`. When - provided, :func:`~app.services.geo_service.lookup_batch` is used + provided, :meth:`GeoCache.lookup_batch` is used for efficient bulk geo resolution. app_db: Optional BanGUI application database connection used to persist newly resolved geo entries across restarts. Only @@ -296,10 +297,10 @@ async def get_active_bans( if ban is not None: bans.append(ban) - if http_session is not None and bans and geo_batch_lookup is not None: + if http_session is not None and bans and geo_cache is not None: all_ips: list[str] = [ban.ip for ban in bans] try: - geo_map = await geo_batch_lookup(all_ips, http_session, db=app_db) + geo_map = await geo_cache.lookup_batch(all_ips, http_session, db=app_db) except Exception: # noqa: BLE001 log.warning("active_bans_batch_geo_failed") geo_map = {} @@ -331,7 +332,7 @@ async def list_bans( page_size: int = DEFAULT_PAGE_SIZE, http_session: aiohttp.ClientSession | None = None, app_db: aiosqlite.Connection | None = None, - geo_batch_lookup: GeoBatchLookup | None = None, + geo_cache: GeoCache | None = None, geo_enricher: GeoEnricher | None = None, history_archive_repo: HistoryArchiveRepository = default_history_archive_repo, origin: BanOrigin | None = None, @@ -345,7 +346,7 @@ async def list_bans( Geo enrichment strategy (highest priority first): 1. When *http_session* is provided the entire page of IPs is resolved in - one :func:`~app.services.geo_service.lookup_batch` call (up to 100 IPs + one :meth:`GeoCache.lookup_batch` call (up to 100 IPs per HTTP request). This avoids the 45 req/min rate limit of the single-IP endpoint and is the preferred production path. 2. When only *geo_enricher* is provided (legacy / test path) each IP is @@ -359,7 +360,7 @@ async def list_bans( page_size: Maximum items per page, capped at ``MAX_PAGE_SIZE`` (default: ``100``). http_session: Optional shared :class:`aiohttp.ClientSession`. When - provided, :func:`~app.services.geo_service.lookup_batch` is used + provided, :meth:`GeoCache.lookup_batch` is used for efficient bulk geo resolution. app_db: Optional BanGUI application database used to persist newly resolved geo entries and to read back cached results. @@ -414,10 +415,10 @@ async def list_bans( # This avoids hitting the 45 req/min single-IP rate limit when the # page contains many bans (e.g. after a large blocklist import). geo_map: dict[str, GeoInfo] = {} - if http_session is not None and rows and geo_batch_lookup is not None: + if http_session is not None and rows and geo_cache is not None: page_ips: list[str] = [r.ip for r in rows] try: - geo_map = await geo_batch_lookup(page_ips, http_session, db=app_db) + geo_map = await geo_cache.lookup_batch(page_ips, http_session, db=app_db) except Exception: # noqa: BLE001 log.warning("ban_service_batch_geo_failed_list_bans") @@ -499,7 +500,7 @@ async def bans_by_country( source: str = "fail2ban", http_session: aiohttp.ClientSession | None = None, geo_cache_lookup: GeoCacheLookup | None = None, - geo_batch_lookup: GeoBatchLookup | None = None, + geo_cache: GeoCache | None = None, geo_enricher: GeoEnricher | None = None, history_archive_repo: HistoryArchiveRepository = default_history_archive_repo, app_db: aiosqlite.Connection | None = None, @@ -607,11 +608,11 @@ async def bans_by_country( uncached=len(uncached), cached=len(geo_map), ) - if geo_batch_lookup is not None: + if geo_cache is not None: # Fire-and-forget: lookup_batch handles rate-limiting / retries. # The dirty-set flush task persists results to the DB. asyncio.create_task( # noqa: RUF006 - geo_batch_lookup(uncached, http_session, db=app_db), + geo_cache.lookup_batch(uncached, http_session, db=app_db), name="geo_bans_by_country", ) elif geo_enricher is not None and unique_ips: diff --git a/backend/app/services/blocklist_service.py b/backend/app/services/blocklist_service.py index cbec11a..7615dba 100644 --- a/backend/app/services/blocklist_service.py +++ b/backend/app/services/blocklist_service.py @@ -44,7 +44,7 @@ if TYPE_CHECKING: from apscheduler.schedulers.asyncio import AsyncIOScheduler from app.config import Settings - from app.models.geo import GeoBatchLookup + from app.services.geo_cache import GeoCache log: structlog.stdlib.BoundLogger = structlog.get_logger() @@ -303,7 +303,7 @@ async def import_source( *, ban_ip: Callable[[str, str, str], Awaitable[None]], geo_is_cached: Callable[[str], bool] | None = None, - geo_batch_lookup: GeoBatchLookup | None = None, + geo_cache: GeoCache | None = None, ) -> ImportSourceResult: """Download and apply bans from a single blocklist source. @@ -417,9 +417,9 @@ async def import_source( to_lookup=len(uncached_ips), ) - if uncached_ips and geo_batch_lookup is not None: + if uncached_ips and geo_cache is not None: try: - await geo_batch_lookup(uncached_ips, http_session, db=db) + await geo_cache.lookup_batch(uncached_ips, http_session, db=db) log.info( "blocklist_geo_prewarm_complete", source_id=source.id, @@ -448,7 +448,7 @@ async def import_all( *, ban_ip: Callable[[str, str, str], Awaitable[None]], geo_is_cached: Callable[[str], bool] | None = None, - geo_batch_lookup: GeoBatchLookup | None = None, + geo_cache: GeoCache | None = None, ) -> ImportRunResult: """Import all enabled blocklist sources. @@ -478,7 +478,7 @@ async def import_all( socket_path, db, geo_is_cached=geo_is_cached, - geo_batch_lookup=geo_batch_lookup, + geo_cache=geo_cache, ban_ip=ban_ip, ) results.append(result) diff --git a/backend/app/services/jail_service.py b/backend/app/services/jail_service.py index 7bcc5b9..e3fc567 100644 --- a/backend/app/services/jail_service.py +++ b/backend/app/services/jail_service.py @@ -52,7 +52,8 @@ if TYPE_CHECKING: import aiohttp import aiosqlite - from app.models.geo import GeoBatchLookup, GeoEnricher, GeoInfo + from app.models.geo import GeoEnricher, GeoInfo + from app.services.geo_cache import GeoCache log: structlog.stdlib.BoundLogger = structlog.get_logger() @@ -749,7 +750,7 @@ async def get_jail_banned_ips( page: int = 1, page_size: int = 25, search: str | None = None, - geo_batch_lookup: GeoBatchLookup | None = None, + geo_cache: GeoCache | None = None, http_session: aiohttp.ClientSession | None = None, app_db: aiosqlite.Connection | None = None, ) -> JailBannedIpsResponse: @@ -766,7 +767,7 @@ async def get_jail_banned_ips( page_size: Items per page; clamped to :data:`_MAX_PAGE_SIZE` (default 25). search: Optional case-insensitive substring filter applied to IP addresses. http_session: Optional shared :class:`aiohttp.ClientSession` for geo - enrichment via :func:`~app.services.geo_service.lookup_batch`. + enrichment via :meth:`GeoCache.lookup_batch`. app_db: Optional BanGUI application database for persistent geo cache. Returns: @@ -817,10 +818,10 @@ async def get_jail_banned_ips( page_bans = all_bans[start : start + page_size] # Geo-enrich only the page slice. - if http_session is not None and page_bans and geo_batch_lookup is not None: + if http_session is not None and page_bans and geo_cache is not None: page_ips = [b.ip for b in page_bans] try: - geo_map = await geo_batch_lookup(page_ips, http_session, db=app_db) + geo_map = await geo_cache.lookup_batch(page_ips, http_session, db=app_db) except Exception: # noqa: BLE001 log.warning("jail_banned_ips_geo_failed", jail=jail_name) geo_map = {} diff --git a/backend/app/services/protocols.py b/backend/app/services/protocols.py index 6905893..c69601f 100644 --- a/backend/app/services/protocols.py +++ b/backend/app/services/protocols.py @@ -38,7 +38,8 @@ if TYPE_CHECKING: MapColorThresholdsUpdate, RegexTestResponse, ) - from app.models.geo import GeoBatchLookup, GeoEnricher, GeoInfo + from app.models.geo import GeoEnricher, GeoInfo + from app.services.geo_cache import GeoCache from app.models.history import HistoryListResponse, IpDetailResponse from app.models.jail import JailDetailResponse, JailListResponse from app.models.server import ServerSettingsResponse, ServerSettingsUpdate, ServerStatus @@ -188,7 +189,7 @@ class BlocklistService(Protocol): db: aiosqlite.Connection, *, geo_is_cached: Callable[[str], bool] | None = None, - geo_batch_lookup: GeoBatchLookup | None = None, + geo_cache: GeoCache | None = None, ban_ip: Callable[[str, str, str], Awaitable[None]], ) -> ImportSourceResult: ... @@ -201,7 +202,7 @@ class BlocklistService(Protocol): *, ban_ip: Callable[[str, str, str], Awaitable[None]], geo_is_cached: Callable[[str], bool] | None = None, - geo_batch_lookup: GeoBatchLookup | None = None, + geo_cache: GeoCache | None = None, ) -> ImportRunResult: ...