From cf721513e88171f7469544aabadda76c50659115 Mon Sep 17 00:00:00 2001 From: Lukas Date: Sun, 22 Mar 2026 20:32:40 +0100 Subject: [PATCH] Fix history origin filter path and add regression tests --- Docs/Tasks.md | 77 +++++++++++++++++++ backend/app/repositories/fail2ban_db_repo.py | 7 ++ backend/app/services/history_service.py | 4 +- .../test_fail2ban_db_repo.py | 29 +++++++ .../test_services/test_history_service.py | 13 ++++ 5 files changed, 129 insertions(+), 1 deletion(-) diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 9f86cca..975c759 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -7,3 +7,80 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue. --- ## Open Issues + +### Bug: `GET /api/history` returns 500 — missing `origin` parameter through the call chain + +**Severity:** Critical (page-level breakage — History page fails to load) + +The router (`backend/app/routers/history.py`) passes `origin=origin` (a `BanOrigin | None` query param) to `history_service.list_history()`, but **the service function signature does not accept an `origin` parameter**. This causes a `TypeError` at runtime → 500. + +The fix must be threaded through two layers: + +1. **`backend/app/services/history_service.py` → `list_history()`** — add `origin: BanOrigin | None = None` parameter, import `BanOrigin`, and forward it to `fail2ban_db_repo.get_history_page()`. +2. **`backend/app/repositories/fail2ban_db_repo.py` → `get_history_page()`** — add `origin: BanOrigin | None = None` parameter and apply the already-existing `_origin_sql_filter()` helper (defined at line 73 but never called in this function). + +**Status:** Done + +Key files: +- `backend/app/routers/history.py` (caller — correct, passes `origin`) +- `backend/app/services/history_service.py` (missing `origin` param) +- `backend/app/repositories/fail2ban_db_repo.py` (missing `origin` param in `get_history_page`; `_origin_sql_filter` already exists but isn't wired in) + +Tests: add/update tests for `list_history` and `get_history_page` with `origin` filtering. + +--- + +### Bug: `GET /api/config/jails/inactive` returns 500 — missing import of `_get_active_jail_names` + +**Severity:** Critical (Config → Jails tab cannot show inactive jails) + +`backend/app/services/jail_config_service.py` calls `_get_active_jail_names()` at 6 call sites (lines 504, 563, 699, 824, 882, 975) but **does not import it**. The current imports from `app.utils.config_file_utils` include `_parse_jails_sync` and `_build_inactive_jail` but are missing `_get_active_jail_names`. This causes `NameError` → 500. + +Fix: add `_get_active_jail_names` to the import block at line 33–38 in `jail_config_service.py`: + +```python +from app.utils.config_file_utils import ( + _build_inactive_jail, + _get_active_jail_names, # ← add + _ordered_config_files, + _parse_jails_sync, + _validate_jail_config_sync, +) +``` + +Key file: `backend/app/services/jail_config_service.py` + +--- + +### Bug: `GET /api/config/filters` returns 500 — missing imports of `_parse_jails_sync` and `_get_active_jail_names` + +**Severity:** Critical (Config → Filters tab fails to load) + +`backend/app/services/filter_config_service.py` calls `_parse_jails_sync()` (lines 509, 609, 880) and `_get_active_jail_names()` (lines 510, 610) but **neither is imported**. This causes `NameError` → 500. + +Fix: add the missing imports. Both are available from `app.utils.config_file_utils` (re-exported from `config_file_service`): + +```python +from app.utils.config_file_utils import ( + _get_active_jail_names, + _parse_jails_sync, +) +``` + +Key file: `backend/app/services/filter_config_service.py` + +--- + +### Bug: `GET /api/config/service-status` returns 500 — missing `bangui_version` in response construction + +**Severity:** Critical (Config → Server tab health check fails) + +The `ServiceStatusResponse` Pydantic model (`backend/app/models/config.py`, line 1005) declares `bangui_version: str = Field(...)` as a **required field with no default**. However, `config_service.get_service_status()` (`backend/app/services/config_service.py`, ~line 820) constructs the response **without supplying `bangui_version`**, causing a Pydantic `ValidationError` → 500. + +Fix: +1. Import `__version__` from `app` (or `app.__init__`) in `config_service.py`. +2. Add `bangui_version=__version__` to the `ServiceStatusResponse(...)` constructor call. + +Key files: +- `backend/app/services/config_service.py` (missing field in constructor) +- `backend/app/models/config.py` (model definition — correct, requires `bangui_version`) diff --git a/backend/app/repositories/fail2ban_db_repo.py b/backend/app/repositories/fail2ban_db_repo.py index acc17d3..06b8419 100644 --- a/backend/app/repositories/fail2ban_db_repo.py +++ b/backend/app/repositories/fail2ban_db_repo.py @@ -294,6 +294,7 @@ async def get_history_page( since: int | None = None, jail: str | None = None, ip_filter: str | None = None, + origin: BanOrigin | None = None, page: int = 1, page_size: int = 100, ) -> tuple[list[HistoryRecord], int]: @@ -314,6 +315,12 @@ async def get_history_page( wheres.append("ip LIKE ?") params.append(f"{ip_filter}%") + origin_clause, origin_params = _origin_sql_filter(origin) + if origin_clause: + origin_clause_clean = origin_clause.removeprefix(" AND ") + wheres.append(origin_clause_clean) + params.extend(origin_params) + where_sql: str = ("WHERE " + " AND ".join(wheres)) if wheres else "" effective_page_size: int = page_size diff --git a/backend/app/services/history_service.py b/backend/app/services/history_service.py index 94f04e0..9662edc 100644 --- a/backend/app/services/history_service.py +++ b/backend/app/services/history_service.py @@ -18,7 +18,7 @@ import structlog if TYPE_CHECKING: from app.models.geo import GeoEnricher -from app.models.ban import TIME_RANGE_SECONDS, TimeRange +from app.models.ban import TIME_RANGE_SECONDS, BanOrigin, TimeRange from app.models.history import ( HistoryBanItem, HistoryListResponse, @@ -62,6 +62,7 @@ async def list_history( range_: TimeRange | None = None, jail: str | None = None, ip_filter: str | None = None, + origin: BanOrigin | None = None, page: int = 1, page_size: int = _DEFAULT_PAGE_SIZE, geo_enricher: GeoEnricher | None = None, @@ -108,6 +109,7 @@ async def list_history( since=since, jail=jail, ip_filter=ip_filter, + origin=origin, page=page, page_size=effective_page_size, ) diff --git a/backend/tests/test_repositories/test_fail2ban_db_repo.py b/backend/tests/test_repositories/test_fail2ban_db_repo.py index 9f3c094..98146bc 100644 --- a/backend/tests/test_repositories/test_fail2ban_db_repo.py +++ b/backend/tests/test_repositories/test_fail2ban_db_repo.py @@ -136,3 +136,32 @@ async def test_get_history_page_and_for_ip(tmp_path: Path) -> None: history_for_ip = await fail2ban_db_repo.get_history_for_ip(db_path=db_path, ip="2.2.2.2") assert len(history_for_ip) == 1 assert history_for_ip[0].ip == "2.2.2.2" + + +@pytest.mark.asyncio +async def test_get_history_page_origin_filter(tmp_path: Path) -> None: + db_path = str(tmp_path / "fail2ban.db") + async with aiosqlite.connect(db_path) as db: + await _create_bans_table(db) + await db.executemany( + "INSERT INTO bans (jail, ip, timeofban, bancount, data) VALUES (?, ?, ?, ?, ?)", + [ + ("jail1", "1.1.1.1", 100, 1, "{}"), + ("blocklist-import", "2.2.2.2", 200, 1, "{}"), + ], + ) + await db.commit() + + page, total = await fail2ban_db_repo.get_history_page( + db_path=db_path, + since=None, + jail=None, + ip_filter=None, + origin="selfblock", + page=1, + page_size=10, + ) + + assert total == 1 + assert len(page) == 1 + assert page[0].ip == "1.1.1.1" diff --git a/backend/tests/test_services/test_history_service.py b/backend/tests/test_services/test_history_service.py index 6f6b45e..6e6694d 100644 --- a/backend/tests/test_services/test_history_service.py +++ b/backend/tests/test_services/test_history_service.py @@ -179,6 +179,19 @@ class TestListHistory: # 2 sshd bans for 1.2.3.4 assert result.total == 2 + 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", + new=AsyncMock(return_value=f2b_db_path), + ): + result = await history_service.list_history( + "fake_socket", origin="selfblock" + ) + + assert result.total == 4 + assert all(item.jail != "blocklist-import" for item in result.items) + 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(