Fix history origin filter path and add regression tests
This commit is contained in:
@@ -7,3 +7,80 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue.
|
|||||||
---
|
---
|
||||||
|
|
||||||
## Open Issues
|
## 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`)
|
||||||
|
|||||||
@@ -294,6 +294,7 @@ async def get_history_page(
|
|||||||
since: int | None = None,
|
since: int | None = None,
|
||||||
jail: str | None = None,
|
jail: str | None = None,
|
||||||
ip_filter: str | None = None,
|
ip_filter: str | None = None,
|
||||||
|
origin: BanOrigin | None = None,
|
||||||
page: int = 1,
|
page: int = 1,
|
||||||
page_size: int = 100,
|
page_size: int = 100,
|
||||||
) -> tuple[list[HistoryRecord], int]:
|
) -> tuple[list[HistoryRecord], int]:
|
||||||
@@ -314,6 +315,12 @@ async def get_history_page(
|
|||||||
wheres.append("ip LIKE ?")
|
wheres.append("ip LIKE ?")
|
||||||
params.append(f"{ip_filter}%")
|
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 ""
|
where_sql: str = ("WHERE " + " AND ".join(wheres)) if wheres else ""
|
||||||
|
|
||||||
effective_page_size: int = page_size
|
effective_page_size: int = page_size
|
||||||
|
|||||||
@@ -18,7 +18,7 @@ import structlog
|
|||||||
if TYPE_CHECKING:
|
if TYPE_CHECKING:
|
||||||
from app.models.geo import GeoEnricher
|
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 (
|
from app.models.history import (
|
||||||
HistoryBanItem,
|
HistoryBanItem,
|
||||||
HistoryListResponse,
|
HistoryListResponse,
|
||||||
@@ -62,6 +62,7 @@ async def list_history(
|
|||||||
range_: TimeRange | None = None,
|
range_: TimeRange | None = None,
|
||||||
jail: str | None = None,
|
jail: str | None = None,
|
||||||
ip_filter: str | None = None,
|
ip_filter: str | None = None,
|
||||||
|
origin: BanOrigin | None = None,
|
||||||
page: int = 1,
|
page: int = 1,
|
||||||
page_size: int = _DEFAULT_PAGE_SIZE,
|
page_size: int = _DEFAULT_PAGE_SIZE,
|
||||||
geo_enricher: GeoEnricher | None = None,
|
geo_enricher: GeoEnricher | None = None,
|
||||||
@@ -108,6 +109,7 @@ async def list_history(
|
|||||||
since=since,
|
since=since,
|
||||||
jail=jail,
|
jail=jail,
|
||||||
ip_filter=ip_filter,
|
ip_filter=ip_filter,
|
||||||
|
origin=origin,
|
||||||
page=page,
|
page=page,
|
||||||
page_size=effective_page_size,
|
page_size=effective_page_size,
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -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")
|
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 len(history_for_ip) == 1
|
||||||
assert history_for_ip[0].ip == "2.2.2.2"
|
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"
|
||||||
|
|||||||
@@ -179,6 +179,19 @@ class TestListHistory:
|
|||||||
# 2 sshd bans for 1.2.3.4
|
# 2 sshd bans for 1.2.3.4
|
||||||
assert result.total == 2
|
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:
|
async def test_unknown_ip_returns_empty(self, f2b_db_path: str) -> None:
|
||||||
"""Filtering by a non-existent IP returns an empty result set."""
|
"""Filtering by a non-existent IP returns an empty result set."""
|
||||||
with patch(
|
with patch(
|
||||||
|
|||||||
Reference in New Issue
Block a user