feat(frontend): add banned-IPs admin UI#284
Conversation
Surfaces the banned-IP data from the security router: a dashboard StatCard, a dedicated /banned-ips page (table + client-side pagination and search), and manual unban action, admin-gated.
| <DataTable | ||
| columns={columns} | ||
| rows={items} | ||
| getRowKey={(row) => row.ip} |
There was a problem hiding this comment.
[bug] getRowKey={(row) => row.ip} is not unique. Backend list rows are per (vhost_id, ip) — the same attacker IP can appear once per ban table. Duplicate React keys cause unstable rendering (wrong domain/actions can glue to the wrong row) and make multi-domain bans hard to reason about.
Suggestion: Use a composite key such as `${row.vhost_id}:${row.ip}` (or ${row.domain}:${row.ip}). Keep Unban keyed by ip only, since the DELETE API is IP-scoped globally.
|
|
||
| listBannedIps(accessToken, controller.signal) | ||
| .then((response) => | ||
| guard(setBannedIps)(ok(response.items.filter((item) => item.banned).length)), |
There was a problem hiding this comment.
[suggestion] The StatCard counts response.items.filter((item) => item.banned).length, i.e. ban entries (one per vhost stick-table row), not unique source IPs. Label is "Banned IPs". If one IP is banned on three vhosts, the card shows 3 while operators may read that as three distinct attackers. The list page's "N banned IPs" footer has the same semantics.
Suggestion: Either count unique ip values for the card/footer, or rename labels to "Ban entries" / "Active bans" so the metric matches the data model.
| setIsLoading(true); | ||
| setError(null); | ||
|
|
||
| listBannedIps(accessToken, controller.signal) |
There was a problem hiding this comment.
[bug] listBannedIps hits GET /security/banned-ips, which is require_admin on the backend (viewers get 403 — see src/backend/app/routers/security.py and test_list_banned_ips_forbidden_for_viewer). The new page, nav item, and dashboard StatCard are still exposed to all authenticated users. Viewers always land on the error state for /banned-ips, and the dashboard "Banned IPs" card always resolves to "—". The page test that mocks a successful list for a non-admin (hasRole false) further hides this contract mismatch.
Suggestion: Either (a) gate nav + route (and optionally the StatCard) to hasRole("admin") and redirect others to Forbidden, matching the backend today, or (b) open GET /security/banned-ips to any authenticated user in a follow-up backend change if product wants viewers to read the list. Until one of those lands, do not present an always-broken surface to viewers.
| const response = await unbanIp("token", "203.0.113.10"); | ||
|
|
||
| expect(response).toEqual({ ip: "203.0.113.10", cleared: 1 }); | ||
| expect(fetchMock.mock.calls[0]?.[0]).toBe("/api/v1/security/banned-ips/203.0.113.10"); |
There was a problem hiding this comment.
[nit] Encoding coverage only asserts a simple IPv4 path segment. unbanIp correctly uses encodeURIComponent, which matters for IPv6 (: → %3A), but there is no regression test for that case.
Suggestion: Add one case with an IPv6 literal (e.g. 2001:db8::1) expecting the encoded path. Low priority.
| <p className="text-sm text-foreground"> | ||
| Are you sure you want to unban{" "} | ||
| <span className="font-mono font-semibold text-foreground">{bannedIp.ip}</span>{" "} | ||
| on <span className="font-semibold text-foreground">{bannedIp.domain}</span>? |
There was a problem hiding this comment.
[bug] The confirmation copy says the IP will be unbanned "on {domain}", but DELETE /security/banned-ips/{ip} clears that key from every active ban stick-table (BanListService.unban loops all auto-ban-enabled vhosts). An admin who unbans from a row for app.example.com will also clear the same IP on other domains. That is a correctness/UX safety issue: the dialog understates the blast radius.
Suggestion: Reword to match the API, e.g. "Unban {ip} across all virtual hosts?" Optionally mention the source row's domain as context ("seen on {domain}") without implying scope. If product ever needs per-vhost unban, the backend contract would need to change first.
| await waitFor(() => expect(screen.getByText(/no banned ips/i)).toBeInTheDocument()); | ||
| }); | ||
|
|
||
| it("admin sees the Unban action, viewer does not", async () => { |
There was a problem hiding this comment.
[suggestion] The test "admin sees the Unban action, viewer does not" mocks a successful listBannedIps for a non-admin. That encodes the product assumption that viewers can read the ban list, which contradicts the current admin-only GET. The suite will keep passing even though production viewers never reach a populated table.
Suggestion: Align the test with the chosen RBAC fix from Issue 3: either assert a Forbidden/redirect/error path for viewers, or (if GET is opened to viewers) add an integration/backend change and keep this UI assertion.
| : String(stats.bannedIps.count) | ||
| } | ||
| tone="error" | ||
| icon={<ShieldIcon />} |
There was a problem hiding this comment.
[nit] "Banned IPs" reuses ShieldIcon, which is already used for "Active policies". Five error/success-adjacent cards with a repeated icon reduce scanability on the denser xl:grid-cols-5 row.
Suggestion: Prefer a distinct icon (e.g. ban/slash-circle style) if one exists in the local icon set, or accept as-is for MVP.
|
|
||
| const total = filtered.length; | ||
| const start = (page - 1) * PAGE_SIZE; | ||
| const items = filtered.slice(start, start + PAGE_SIZE); |
There was a problem hiding this comment.
[suggestion] After unban/refresh, page is not clamped when the filtered list shrinks. With PAGE_SIZE = 50 this is uncommon, but if the user is on page 2+ and the last items on that page are unbanned (or expire), items becomes [] while total > 0 on earlier pages — the table shows the empty state ("No banned IPs") even though bans still exist, and the footer can show "Page 2 of 1".
Suggestion: After computing total / totalPages, clamp page (e.g. Math.min(page, totalPages) via effect or when applying the slice). Mirror the search path that already resets to page 1.
- Gate the /banned-ips route, nav entry, and dashboard StatCard to admins (RequireAdmin guard), matching the admin-only backend endpoint. - Use a composite (vhost_id:ip) row key since an IP can be banned per vhost. - Reword the unban dialog to reflect that DELETE clears the IP across all virtual hosts, not just the row's domain. - Count unique source IPs in the StatCard; label the list footer as entries. - Clamp the client-side page so a shrinking list can't strand an empty page. - Add a distinct BanIcon and an IPv6 unban-encoding test.
Summary
error)./banned-ipsadmin page: table (IP, domain, violations, expires-in) with client-side search and pagination, plus an admin-gated Unban action.use-banned-ipshook andbanned-ipsfeature API layer wrappingGET/DELETE /security/banned-ips.Note: the
GET /security/banned-ipsendpoint returns all tracked stick-table entries (including ones not yet past the ban threshold) without server-side pagination. The list and StatCard filter tobanned === trueclient-side, and pagination/search are done client-side accordingly.Test plan
pnpm run type-checkpnpm run lintpnpm run test(124 passed, including new hook/API/page tests)pnpm run buildCloses #277
Related to #176