From ac2028e1c22d223b8a3e89b9f14ad9d3792874d3 Mon Sep 17 00:00:00 2001 From: Lukas Date: Sat, 25 Apr 2026 18:44:59 +0200 Subject: [PATCH] Fix: Consolidate divergent _since_unix implementations (T-09) Consolidate the two divergent implementations of _since_unix from ban_service.py and history_service.py into a single shared utility function in time_utils.py. Changes: - Move _since_unix to app/utils/time_utils.py with consistent time.time() approach - Move TIME_RANGE_SLACK_SECONDS constant to app/utils/constants.py - Update ban_service.py to import since_unix from time_utils - Update history_service.py to import since_unix from time_utils - Both services now use the same window boundary calculation with 60-second slack - Add comprehensive tests for the shared since_unix function - Document timestamp handling rationale in Backend-Development.md This ensures dashboard and history queries return consistent row counts for the same time range by using the same timestamp calculation and slack window across all services. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Docs/Backend-Development.md | 33 +++++++++++ backend/app/services/ban_service.py | 38 ++---------- backend/app/services/history_service.py | 21 ++----- backend/app/utils/constants.py | 3 + backend/app/utils/time_utils.py | 29 +++++++++ .../tests/test_services/test_time_utils.py | 59 ++++++++++++++++++- 6 files changed, 132 insertions(+), 51 deletions(-) diff --git a/Docs/Backend-Development.md b/Docs/Backend-Development.md index 42dd0a3..cf714c2 100644 --- a/Docs/Backend-Development.md +++ b/Docs/Backend-Development.md @@ -69,6 +69,39 @@ from fail2ban.client.csocket import CSSocket # noqa: E402 - `print()` for logging — use `structlog`. - `json.loads` / `json.dumps` on Pydantic models — use `.model_dump()` / `.model_validate()`. +### Timestamp Handling + +Timestamp consistency is critical for accurate ban history queries across the dashboard and history endpoints. Follow these rules: + +**Rule 1: Use consistent UTC timestamps** +- All timestamps in the database are stored as Unix epochs (seconds since 1970-01-01 UTC). +- fail2ban stores timestamps using `time.time()`, which is always UTC epoch seconds. +- When querying fail2ban's SQLite database by timestamp, use `app.utils.time_utils.since_unix()` (not manual datetime calculations). + +**Rule 2: Time-range windows include a 60-second slack** +- The `since_unix()` function includes a 60-second slack window (`TIME_RANGE_SLACK_SECONDS` in `app.utils.constants`). +- This slack accommodates: + - Clock drift between the local system and fail2ban. + - Test seeding delays when timestamps are manually set to exact boundaries. +- The slack ensures that dashboard and history queries return consistent row counts for the same time range. + +**Rule 3: Never duplicate timestamp calculation logic** +- All services that query by time range must import and use `since_unix()`. +- Do not recalculate timestamps locally using `datetime` or `time` modules in service code. +- If you need a timestamp for a time range, use `since_unix()`. + +**Example:** +```python +from app.utils.time_utils import since_unix + +# Get all bans from the last 24 hours (with 60-second slack) +since_ts: int = since_unix("24h") +rows = await db.execute( + "SELECT * FROM bans WHERE timeofban >= ?", + (since_ts,) +) +``` + --- ## 3. Project Structure diff --git a/backend/app/services/ban_service.py b/backend/app/services/ban_service.py index 499e1a7..96d1744 100644 --- a/backend/app/services/ban_service.py +++ b/backend/app/services/ban_service.py @@ -13,7 +13,6 @@ from __future__ import annotations import asyncio import contextlib import ipaddress -import time from typing import TYPE_CHECKING, cast import structlog @@ -23,7 +22,6 @@ from app.models.ban import ( BLOCKLIST_JAIL, BUCKET_SECONDS, BUCKET_SIZE_LABEL, - TIME_RANGE_SECONDS, ActiveBan, ActiveBanListResponse, BanOrigin, @@ -57,6 +55,7 @@ from app.utils.fail2ban_response import ( ok, to_dict, ) +from app.utils.time_utils import since_unix if TYPE_CHECKING: import aiohttp @@ -318,33 +317,6 @@ async def get_active_bans( log.info("active_bans_fetched", total=len(bans)) return ActiveBanListResponse(bans=bans, total=len(bans)) - -_TIME_RANGE_SLACK_SECONDS: int = 60 - - -def _since_unix(range_: TimeRange) -> int: - """Return the Unix timestamp representing the start of the time window. - - Uses :func:`time.time` (always UTC epoch seconds on all platforms) to be - consistent with how fail2ban stores ``timeofban`` values in its SQLite - database. fail2ban records ``time.time()`` values directly, so - comparing against a timezone-aware ``datetime.now(UTC).timestamp()`` would - theoretically produce the same number but using :func:`time.time` avoids - any tz-aware datetime pitfalls on misconfigured systems. - - Args: - range_: One of the supported time-range presets. - - Returns: - Unix timestamp (seconds since epoch) equal to *now − range_* with a - small slack window for clock drift and test seeding delays. - """ - seconds: int = TIME_RANGE_SECONDS[range_] - return int(time.time()) - seconds - _TIME_RANGE_SLACK_SECONDS - - - - # --------------------------------------------------------------------------- # Public API # --------------------------------------------------------------------------- @@ -401,7 +373,7 @@ async def list_bans( paginated items and total count. """ - since: int = _since_unix(range_) + since: int = since_unix(range_) effective_page_size: int = min(page_size, MAX_PAGE_SIZE) offset: int = (page - 1) * effective_page_size @@ -570,7 +542,7 @@ async def bans_by_country( aggregation and the companion ban list. """ - since: int = _since_unix(range_) + since: int = since_unix(range_) if source not in ("fail2ban", "archive"): raise ValueError(f"Unsupported source: {source!r}") @@ -820,7 +792,7 @@ async def ban_trend( :class:`~app.models.ban.BanTrendResponse` with a full bucket list and the human-readable bucket-size label. """ - since: int = _since_unix(range_) + since: int = since_unix(range_) bucket_secs: int = BUCKET_SECONDS[range_] num_buckets: int = bucket_count(range_) @@ -920,7 +892,7 @@ async def bans_by_jail( :class:`~app.models.ban.BansByJailResponse` with per-jail counts sorted descending and the total ban count. """ - since: int = _since_unix(range_) + since: int = since_unix(range_) if source not in ("fail2ban", "archive"): raise ValueError(f"Unsupported source: {source!r}") diff --git a/backend/app/services/history_service.py b/backend/app/services/history_service.py index 5e45038..6a6e839 100644 --- a/backend/app/services/history_service.py +++ b/backend/app/services/history_service.py @@ -15,7 +15,7 @@ from typing import TYPE_CHECKING import structlog -from app.models.ban import TIME_RANGE_SECONDS, BanOrigin, TimeRange +from app.models.ban import BanOrigin, TimeRange from app.services import geo_service if TYPE_CHECKING: @@ -35,6 +35,7 @@ from app.repositories import history_archive_repo as default_history_archive_rep 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() @@ -43,25 +44,11 @@ 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) - # --------------------------------------------------------------------------- -# Constants +# Internal Helpers # --------------------------------------------------------------------------- -def _since_unix(range_: TimeRange) -> int: - """Return the Unix timestamp for the start of the given time window. - - Args: - range_: One of the supported time-range presets. - - Returns: - Unix timestamp (seconds since epoch) equal to *now − range_*. - """ - seconds: int = TIME_RANGE_SECONDS[range_] - return int(datetime.now(tz=UTC).timestamp()) - seconds - - async def _resolve_geo_info( ip: str, *, @@ -199,7 +186,7 @@ async def list_history( # Build WHERE clauses dynamically. since: int | None = None if range_ is not None: - since = _since_unix(range_) + since = since_unix(range_) db_path: str = await get_fail2ban_db_path(socket_path) log.info( diff --git a/backend/app/utils/constants.py b/backend/app/utils/constants.py index abd57fa..e0e99a1 100644 --- a/backend/app/utils/constants.py +++ b/backend/app/utils/constants.py @@ -65,6 +65,9 @@ TIME_RANGE_HOURS: Final[dict[str, int]] = { TIME_RANGE_365D: 365 * 24, } +TIME_RANGE_SLACK_SECONDS: Final[int] = 60 +"""Clock drift and test seeding tolerance for timestamp comparisons.""" + # --------------------------------------------------------------------------- # Pagination # --------------------------------------------------------------------------- diff --git a/backend/app/utils/time_utils.py b/backend/app/utils/time_utils.py index fc648d1..7e899c4 100644 --- a/backend/app/utils/time_utils.py +++ b/backend/app/utils/time_utils.py @@ -7,6 +7,7 @@ for working with time throughout the backend. """ import datetime +import time def utc_now() -> datetime.datetime: @@ -65,3 +66,31 @@ def hours_ago(hours: int) -> datetime.datetime: Timezone-aware UTC :class:`datetime.datetime`. """ return utc_now() - datetime.timedelta(hours=hours) + + +def since_unix(range_: str) -> int: + """Return the Unix timestamp for the start of a time-range window. + + Uses :func:`time.time` (always UTC epoch seconds on all platforms) to be + consistent with how fail2ban stores ``timeofban`` values in its SQLite + database. fail2ban records :func:`time.time()` values directly, so using + a timezone-aware :func:`datetime.datetime.now`\\ ``(UTC).timestamp()`` + would theoretically produce the same result but using :func:`time.time` + avoids any timezone-aware datetime pitfalls on misconfigured systems. + + A 60-second slack window is applied to accommodate clock drift and + test seeding delays. This ensures consistent query windows across services + (e.g., dashboard vs. history). + + Args: + range_: One of the supported time-range presets (e.g., ``"24h"``). + + Returns: + Unix timestamp (seconds since epoch) representing the start of the + time window: *now − range_ − slack*. + """ + from app.models.ban import TIME_RANGE_SECONDS # noqa: F401 + from app.utils.constants import TIME_RANGE_SLACK_SECONDS + + seconds: int = TIME_RANGE_SECONDS[range_] + return int(time.time()) - seconds - TIME_RANGE_SLACK_SECONDS diff --git a/backend/tests/test_services/test_time_utils.py b/backend/tests/test_services/test_time_utils.py index bb4ffa6..aea50a8 100644 --- a/backend/tests/test_services/test_time_utils.py +++ b/backend/tests/test_services/test_time_utils.py @@ -1,8 +1,17 @@ """Tests for app.utils.time_utils.""" import datetime +import time -from app.utils.time_utils import add_minutes, hours_ago, is_expired, utc_from_timestamp, utc_now +from app.utils.time_utils import ( + add_minutes, + hours_ago, + is_expired, + since_unix, + utc_from_timestamp, + utc_now, +) +from app.utils.constants import TIME_RANGE_SLACK_SECONDS class TestUtcNow: @@ -77,3 +86,51 @@ class TestHoursAgo: expected_min = before - datetime.timedelta(hours=1, seconds=1) expected_max = after - datetime.timedelta(hours=1) + datetime.timedelta(seconds=1) assert expected_min <= result <= expected_max + + +class TestSinceUnix: + """Tests for :func:`since_unix`.""" + + def test_since_unix_24h_returns_unix_timestamp(self) -> None: + """Verify since_unix returns an integer timestamp.""" + result = since_unix("24h") + assert isinstance(result, int) + + def test_since_unix_24h_is_roughly_24_hours_ago(self) -> None: + """Verify 24h preset returns a timestamp ~24 hours in the past.""" + before = int(time.time()) + result = since_unix("24h") + after = int(time.time()) + + # Allow 1 second tolerance for execution time + expected_min = before - (24 * 3600) - TIME_RANGE_SLACK_SECONDS - 1 + expected_max = after - (24 * 3600) - TIME_RANGE_SLACK_SECONDS + 1 + + assert expected_min <= result <= expected_max + + def test_since_unix_7d_is_roughly_7_days_ago(self) -> None: + """Verify 7d preset returns a timestamp ~7 days in the past.""" + before = int(time.time()) + result = since_unix("7d") + after = int(time.time()) + + # Allow 1 second tolerance for execution time + expected_min = before - (7 * 24 * 3600) - TIME_RANGE_SLACK_SECONDS - 1 + expected_max = after - (7 * 24 * 3600) - TIME_RANGE_SLACK_SECONDS + 1 + + assert expected_min <= result <= expected_max + + def test_since_unix_includes_slack_window(self) -> None: + """Verify 60-second slack is included in all presets.""" + now = int(time.time()) + result = since_unix("24h") + + # Verify slack is included: result should be (now - 24h - 60s) + # within tolerance + diff_without_slack = now - result + expected_without_slack = 24 * 3600 + actual_slack = diff_without_slack - expected_without_slack + + # The slack should be ~60 seconds + assert actual_slack >= TIME_RANGE_SLACK_SECONDS - 1 + assert actual_slack <= TIME_RANGE_SLACK_SECONDS + 1