Fix country not shown in ban list due to geo rate limiting
list_bans() was calling geo_service.lookup() once per IP on the page (e.g. 100 sequential HTTP requests), hitting the ip-api.com free-tier single-IP limit of 45 req/min. IPs beyond the ~45th were added to the in-process negative cache (5 min TTL) and showed as no country until the TTL expired. The map endpoint never had this problem because it used lookup_batch (100 IPs per POST). Add http_session and app_db params to list_bans(). When http_session is provided (production path), the entire page is resolved in one lookup_batch() call instead of N individual ones. The legacy geo_enricher callback is kept for test compatibility. Update the dashboard router to use the batch path directly. Adds 3 tests covering the batch geo path, failure resilience, and http_session priority over geo_enricher.
This commit is contained in:
@@ -26,7 +26,7 @@ from app.models.ban import (
|
||||
TimeRange,
|
||||
)
|
||||
from app.models.server import ServerStatus, ServerStatusResponse
|
||||
from app.services import ban_service, geo_service
|
||||
from app.services import ban_service
|
||||
|
||||
router: APIRouter = APIRouter(prefix="/api/dashboard", tags=["Dashboard"])
|
||||
|
||||
@@ -109,15 +109,13 @@ async def get_dashboard_bans(
|
||||
socket_path: str = request.app.state.settings.fail2ban_socket
|
||||
http_session: aiohttp.ClientSession = request.app.state.http_session
|
||||
|
||||
async def _enricher(ip: str) -> geo_service.GeoInfo | None:
|
||||
return await geo_service.lookup(ip, http_session, db=db)
|
||||
|
||||
return await ban_service.list_bans(
|
||||
socket_path,
|
||||
range,
|
||||
page=page,
|
||||
page_size=page_size,
|
||||
geo_enricher=_enricher,
|
||||
http_session=http_session,
|
||||
app_db=db,
|
||||
origin=origin,
|
||||
)
|
||||
|
||||
|
||||
@@ -171,6 +171,8 @@ async def list_bans(
|
||||
*,
|
||||
page: int = 1,
|
||||
page_size: int = _DEFAULT_PAGE_SIZE,
|
||||
http_session: aiohttp.ClientSession | None = None,
|
||||
app_db: aiosqlite.Connection | None = None,
|
||||
geo_enricher: Any | None = None,
|
||||
origin: BanOrigin | None = None,
|
||||
) -> DashboardBanListResponse:
|
||||
@@ -180,6 +182,15 @@ async def list_bans(
|
||||
``timeofban`` falls within the specified *range_*. Results are ordered
|
||||
newest-first.
|
||||
|
||||
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
|
||||
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
|
||||
resolved individually via the supplied async callable.
|
||||
|
||||
Args:
|
||||
socket_path: Path to the fail2ban Unix domain socket.
|
||||
range_: Time-range preset (``"24h"``, ``"7d"``, ``"30d"``, or
|
||||
@@ -187,8 +198,13 @@ async def list_bans(
|
||||
page: 1-based page number (default: ``1``).
|
||||
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
|
||||
for efficient bulk geo resolution.
|
||||
app_db: Optional BanGUI application database used to persist newly
|
||||
resolved geo entries and to read back cached results.
|
||||
geo_enricher: Optional async callable ``(ip: str) -> GeoInfo | None``.
|
||||
When supplied every result is enriched with country and ASN data.
|
||||
Used as a fallback when *http_session* is ``None`` (e.g. tests).
|
||||
origin: Optional origin filter — ``"blocklist"`` restricts results to
|
||||
the ``blocklist-import`` jail, ``"selfblock"`` excludes it.
|
||||
|
||||
@@ -196,6 +212,8 @@ async def list_bans(
|
||||
:class:`~app.models.ban.DashboardBanListResponse` containing the
|
||||
paginated items and total count.
|
||||
"""
|
||||
from app.services import geo_service # noqa: PLC0415
|
||||
|
||||
since: int = _since_unix(range_)
|
||||
effective_page_size: int = min(page_size, _MAX_PAGE_SIZE)
|
||||
offset: int = (page - 1) * effective_page_size
|
||||
@@ -231,6 +249,17 @@ async def list_bans(
|
||||
) as cur:
|
||||
rows = await cur.fetchall()
|
||||
|
||||
# Batch-resolve geo data for all IPs on this page in a single API call.
|
||||
# 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, Any] = {}
|
||||
if http_session is not None and rows:
|
||||
page_ips: list[str] = [str(r["ip"]) for r in rows]
|
||||
try:
|
||||
geo_map = await geo_service.lookup_batch(page_ips, http_session, db=app_db)
|
||||
except Exception: # noqa: BLE001
|
||||
log.warning("ban_service_batch_geo_failed_list_bans")
|
||||
|
||||
items: list[DashboardBanItem] = []
|
||||
for row in rows:
|
||||
jail: str = str(row["jail"])
|
||||
@@ -245,7 +274,14 @@ async def list_bans(
|
||||
asn: str | None = None
|
||||
org: str | None = None
|
||||
|
||||
if geo_enricher is not None:
|
||||
if geo_map:
|
||||
geo = geo_map.get(ip)
|
||||
if geo is not None:
|
||||
country_code = geo.country_code
|
||||
country_name = geo.country_name
|
||||
asn = geo.asn
|
||||
org = geo.org
|
||||
elif geo_enricher is not None:
|
||||
try:
|
||||
geo = await geo_enricher(ip)
|
||||
if geo is not None:
|
||||
|
||||
@@ -290,6 +290,104 @@ class TestListBansGeoEnrichment:
|
||||
assert item.country_code is None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# list_bans — batch geo enrichment via http_session
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestListBansBatchGeoEnrichment:
|
||||
"""Verify that list_bans uses lookup_batch when http_session is provided."""
|
||||
|
||||
async def test_batch_geo_applied_via_http_session(
|
||||
self, f2b_db_path: str
|
||||
) -> None:
|
||||
"""Geo fields are populated via lookup_batch when http_session is given."""
|
||||
from app.services.geo_service import GeoInfo
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
fake_session = MagicMock()
|
||||
fake_geo_map = {
|
||||
"1.2.3.4": GeoInfo(country_code="DE", country_name="Germany", asn="AS3320", org="Deutsche Telekom"),
|
||||
"5.6.7.8": GeoInfo(country_code="US", country_name="United States", asn="AS15169", org="Google"),
|
||||
}
|
||||
|
||||
with patch(
|
||||
"app.services.ban_service._get_fail2ban_db_path",
|
||||
new=AsyncMock(return_value=f2b_db_path),
|
||||
), patch(
|
||||
"app.services.geo_service.lookup_batch",
|
||||
new=AsyncMock(return_value=fake_geo_map),
|
||||
):
|
||||
result = await ban_service.list_bans(
|
||||
"/fake/sock", "24h", http_session=fake_session
|
||||
)
|
||||
|
||||
assert result.total == 2
|
||||
de_item = next(i for i in result.items if i.ip == "1.2.3.4")
|
||||
us_item = next(i for i in result.items if i.ip == "5.6.7.8")
|
||||
assert de_item.country_code == "DE"
|
||||
assert de_item.country_name == "Germany"
|
||||
assert us_item.country_code == "US"
|
||||
assert us_item.country_name == "United States"
|
||||
|
||||
async def test_batch_failure_does_not_break_results(
|
||||
self, f2b_db_path: str
|
||||
) -> None:
|
||||
"""A lookup_batch failure still returns items with null geo fields."""
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
fake_session = MagicMock()
|
||||
|
||||
with patch(
|
||||
"app.services.ban_service._get_fail2ban_db_path",
|
||||
new=AsyncMock(return_value=f2b_db_path),
|
||||
), patch(
|
||||
"app.services.geo_service.lookup_batch",
|
||||
new=AsyncMock(side_effect=RuntimeError("batch geo down")),
|
||||
):
|
||||
result = await ban_service.list_bans(
|
||||
"/fake/sock", "24h", http_session=fake_session
|
||||
)
|
||||
|
||||
assert result.total == 2
|
||||
for item in result.items:
|
||||
assert item.country_code is None
|
||||
|
||||
async def test_http_session_takes_priority_over_geo_enricher(
|
||||
self, f2b_db_path: str
|
||||
) -> None:
|
||||
"""When both http_session and geo_enricher are provided, batch wins."""
|
||||
from app.services.geo_service import GeoInfo
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
fake_session = MagicMock()
|
||||
fake_geo_map = {
|
||||
"1.2.3.4": GeoInfo(country_code="DE", country_name="Germany", asn=None, org=None),
|
||||
"5.6.7.8": GeoInfo(country_code="DE", country_name="Germany", asn=None, org=None),
|
||||
}
|
||||
|
||||
async def enricher_should_not_be_called(ip: str) -> GeoInfo:
|
||||
raise AssertionError(f"geo_enricher was called for {ip!r} — should not happen")
|
||||
|
||||
with patch(
|
||||
"app.services.ban_service._get_fail2ban_db_path",
|
||||
new=AsyncMock(return_value=f2b_db_path),
|
||||
), patch(
|
||||
"app.services.geo_service.lookup_batch",
|
||||
new=AsyncMock(return_value=fake_geo_map),
|
||||
):
|
||||
result = await ban_service.list_bans(
|
||||
"/fake/sock",
|
||||
"24h",
|
||||
http_session=fake_session,
|
||||
geo_enricher=enricher_should_not_be_called,
|
||||
)
|
||||
|
||||
assert result.total == 2
|
||||
for item in result.items:
|
||||
assert item.country_code == "DE"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# list_bans — pagination
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user