diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 48c8a37..4b56645 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -4,142 +4,43 @@ This document breaks the entire BanGUI project into development stages, ordered --- -## Bug Fix: "Raw Action Configuration" always empty — DONE +## Stage 0 — First-Run Bootstrap & Startup Fix -**Summary:** Renamed `GET /actions/{name}` and `PUT /actions/{name}` in `file_config.py` to `GET /actions/{name}/raw` and `PUT /actions/{name}/raw` to eliminate the route-shadowing conflict with `config.py`. Added `configActionRaw` endpoint helper in `endpoints.ts` and updated `fetchActionFile` / `updateActionFile` in `config.ts` to call it. Added `TestGetActionFileRaw` and `TestUpdateActionFileRaw` test classes. - -**Problem** -When a user opens the *Actions* tab in the Config screen, selects any action, and expands the "Raw Action Configuration" accordion, the text area is always blank. The `fetchContent` callback makes a `GET /api/config/actions/{name}` request expecting a `ConfFileContent` response (`{ content: string, name: string, filename: string }`), but the backend returns an `ActionConfig` (the fully-parsed structured model) instead. The `content` field is therefore `undefined` in the browser, which the `RawConfigSection` component renders as an empty string. - -**Root cause** -Both `backend/app/routers/config.py` and `backend/app/routers/file_config.py` are mounted with the prefix `/api/config` (see lines 107 and 63 respectively). Both define a `GET /actions/{name}` route: - -- `config.py` → returns `ActionConfig` (parsed detail) -- `file_config.py` → returns `ConfFileContent` (raw file text) - -In `backend/app/main.py`, `config.router` is registered on line 402 and `file_config.router` on line 403. FastAPI matches the first registered route, so the raw-content endpoint is permanently shadowed. - -The filters feature already solved the same conflict by using distinct paths (`/filters/{name}/raw` for raw and `/filters/{name}` for parsed). Actions must follow the same pattern. - -**Fix — backend (`backend/app/routers/file_config.py`)** -Rename the two action raw-file routes: - -| Old path | New path | -|---|---| -| `GET /actions/{name}` | `GET /actions/{name}/raw` | -| `PUT /actions/{name}` | `PUT /actions/{name}/raw` | - -Update the module-level docstring comment block at the top of `file_config.py` to reflect the new paths. - -**Fix — frontend (`frontend/src/api/endpoints.ts`)** -Add a new helper alongside the existing `configAction` entry: - -```ts -configActionRaw: (name: string): string => `/config/actions/${encodeURIComponent(name)}/raw`, -``` - -**Fix — frontend (`frontend/src/api/config.ts`)** -Change `fetchActionFile` and `updateActionFile` to call `ENDPOINTS.configActionRaw(name)` instead of `ENDPOINTS.configAction(name)`. - -**No changes needed elsewhere.** `ActionsTab.tsx` already passes `fetchActionFile` / `updateActionFile` into `RawConfigSection` via `fetchRaw` / `saveRaw`; the resolved URL is the only thing that needs to change. +These tasks fix a crash-on-first-boot regression and make the setup/login redirect flow reliable. They must be completed before any other feature work because the application cannot start without them. --- -## Rename dev jail `bangui-sim` → `manual-Jail` — DONE +### Task 0.1 — Fix: Create the database parent directory before connecting ✅ DONE -**Summary:** Renamed `jail.d/bangui-sim.conf` → `manual-Jail.conf` and `filter.d/bangui-sim.conf` → `manual-Jail.conf` (via `git mv`), updated all internal references. Updated `check_ban_status.sh`, `simulate_failed_logins.sh`, and `fail2ban-dev-config/README.md` to replace all `bangui-sim` references with `manual-Jail`. +**File:** `backend/app/main.py` -**Scope** -This is purely a Docker development-environment change. The frontend never hardcodes jail names; it reads them dynamically from the API. Only the files listed below need editing. - -**Files to update** - -1. **`Docker/fail2ban-dev-config/fail2ban/jail.d/bangui-sim.conf`** - - Rename the file to `manual-Jail.conf`. - - Change the section header from `[bangui-sim]` to `[manual-Jail]`. - - Change `filter = bangui-sim` to `filter = manual-Jail`. - - Update the file-header comment ("BanGUI — Simulated authentication failure jail" line and any other references to `bangui-sim`). - -2. **`Docker/fail2ban-dev-config/fail2ban/filter.d/bangui-sim.conf`** - - Rename the file to `manual-Jail.conf`. - - Update any internal comments that mention `bangui-sim`. - -3. **`Docker/check_ban_status.sh`** - - Change `readonly JAIL="bangui-sim"` to `readonly JAIL="manual-Jail"`. - - Update the file-header comment block that references `bangui-sim`. - -4. **`Docker/simulate_failed_logins.sh`** - - Update all comments that mention `bangui-sim` or `bangui-auth` to refer to `manual-Jail` instead. - - Do **not** change the log-line format string (`bangui-auth: authentication failure from `) unless the filter's `failregex` in the renamed `manual-Jail.conf` is also updated to match the new prefix; keep them in sync. - -5. **`Docker/fail2ban-dev-config/README.md`** - - Replace every occurrence of `bangui-sim` with `manual-Jail`. - -After renaming, run `docker compose -f Docker/compose.debug.yml restart fail2ban` and verify with `bash Docker/check_ban_status.sh` that the jail is active under its new name. +**Implemented:** In `_lifespan`, resolve `settings.database_path` to a `Path`, call `.parent.mkdir(parents=True, exist_ok=True)` before `aiosqlite.connect()`. Added `debug`-level structured log line after mkdir. Tests added in `TestLifespanDatabaseDirectoryCreation`. --- -## Bug Fix: Config screen content pane does not update when switching jails — DONE +### Task 0.2 — Fix: SetupRedirectMiddleware must treat a missing database as "setup not complete" ✅ DONE -**Summary:** Added `key={selectedActiveJail.name}` to `JailConfigDetail` and `key={selectedInactiveJail.name}` to `InactiveJailDetail` in `JailsTab.tsx`, forcing React to unmount and remount the detail component on jail selection changes. +**File:** `backend/app/main.py` -**Problem** -In the *Jails* tab of the Config screen, clicking a jail name in the left-hand list correctly highlights the new selection, but the right-hand content pane continues to show the previously selected jail (e.g. selecting `blocklist-import` after `manual-Jail` still displays `manual-Jail`'s configuration). - -**Root cause** -In `frontend/src/components/config/JailsTab.tsx`, the child components rendered by `ConfigListDetail` are not given a `key` prop: - -```tsx -{selectedActiveJail !== undefined ? ( - -) : selectedInactiveJail !== undefined ? ( - -) : null} -``` - -When the user switches between two jails of the same type (both active or both inactive), React reuses the existing component instance and only updates its props. Any internal state derived from the previous jail — including the `loadedRef` guard inside every nested `RawConfigSection` — is never reset. As a result, forms still show the old jail's values and the raw-config section refuses to re-fetch because `loadedRef.current` is already `true`. - -Compare with `ActionsTab.tsx`, where `ActionDetail` correctly uses `key={selectedAction.name}`: - -```tsx - -``` - -**Fix — `frontend/src/components/config/JailsTab.tsx`** -Add `key` props to both detail components so React unmounts and remounts them whenever the selected jail changes: - -```tsx -{selectedActiveJail !== undefined ? ( - { handleDeactivate(selectedActiveJail.name); }} - /> -) : selectedInactiveJail !== undefined ? ( - { setActivateTarget(selectedInactiveJail); }} - onDeactivate={ - selectedInactiveJail.has_local_override - ? (): void => { handleDeactivateInactive(selectedInactiveJail.name); } - : undefined - } - /> -) : null} -``` - -No other files need changing. The `key` change is the minimal, isolated fix. +**Implemented:** Changed the guard in `SetupRedirectMiddleware.dispatch` so that `db is None` also triggers the redirect to `/api/setup`. The condition is now `if db is None or not await setup_service.is_setup_complete(db)`. The `_setup_complete_cached` flag is only set after a successful `is_setup_complete(db)` call with a live `db`. Tests added in `TestSetupRedirectMiddlewareDbNone`. + +--- + +### Task 0.3 — Fix: SetupGuard must redirect to /setup on API errors, not allow through ✅ DONE + +**File:** `frontend/src/components/SetupGuard.tsx` + +**Implemented:** Changed the `.catch()` handler to set `status` to `"pending"` instead of `"done"`. Updated the comment to explain the conservative fallback. Tests added in `SetupGuard.test.tsx`. + +--- + +### Task 0.4 — Fix: SetupPage must redirect to /login when setup is already complete ✅ DONE + +**File:** `frontend/src/pages/SetupPage.tsx` + +**Implemented:** +1. Added `checking` boolean state (initialised to `true`). While `checking` is true, a full-screen `` is rendered instead of the form, preventing the form from flashing. +2. The `useEffect` sets `checking` to `false` in both the `.then()` (when setup is not complete) and the `.catch()` branch. Added a `console.warn` in the catch block. Added a `cancelled` flag and cleanup return to the effect. +Tests added in `SetupPage.test.tsx`. --- diff --git a/backend/app/config.py b/backend/app/config.py index 0f73ce5..4e89da2 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -85,4 +85,4 @@ def get_settings() -> Settings: A validated :class:`Settings` object. Raises :class:`pydantic.ValidationError` if required keys are absent or values fail validation. """ - return Settings() # type: ignore[call-arg] # pydantic-settings populates required fields from env vars + return Settings() # pydantic-settings populates required fields from env vars diff --git a/backend/app/main.py b/backend/app/main.py index a02c1c1..2b2828e 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -138,6 +138,9 @@ async def _lifespan(app: FastAPI) -> AsyncGenerator[None, None]: log.info("bangui_starting_up", database_path=settings.database_path) # --- Application database --- + db_path: Path = Path(settings.database_path) + db_path.parent.mkdir(parents=True, exist_ok=True) + log.debug("database_directory_ensured", directory=str(db_path.parent)) db: aiosqlite.Connection = await aiosqlite.connect(settings.database_path) db.row_factory = aiosqlite.Row await init_db(db) @@ -320,17 +323,15 @@ class SetupRedirectMiddleware(BaseHTTPMiddleware): if path.startswith("/api") and not getattr( request.app.state, "_setup_complete_cached", False ): - db: aiosqlite.Connection | None = getattr(request.app.state, "db", None) - if db is not None: - from app.services import setup_service # noqa: PLC0415 + from app.services import setup_service # noqa: PLC0415 - if await setup_service.is_setup_complete(db): - request.app.state._setup_complete_cached = True - else: - return RedirectResponse( - url="/api/setup", - status_code=status.HTTP_307_TEMPORARY_REDIRECT, - ) + db: aiosqlite.Connection | None = getattr(request.app.state, "db", None) + if db is None or not await setup_service.is_setup_complete(db): + return RedirectResponse( + url="/api/setup", + status_code=status.HTTP_307_TEMPORARY_REDIRECT, + ) + request.app.state._setup_complete_cached = True return await call_next(request) diff --git a/backend/tests/test_routers/test_setup.py b/backend/tests/test_routers/test_setup.py index e07cef4..f66987d 100644 --- a/backend/tests/test_routers/test_setup.py +++ b/backend/tests/test_routers/test_setup.py @@ -3,7 +3,7 @@ from __future__ import annotations from pathlib import Path -from unittest.mock import patch +from unittest.mock import AsyncMock, MagicMock, patch import aiosqlite import pytest @@ -11,7 +11,7 @@ from httpx import ASGITransport, AsyncClient from app.config import Settings from app.db import init_db -from app.main import create_app +from app.main import _lifespan, create_app # --------------------------------------------------------------------------- # Shared setup payload @@ -286,3 +286,149 @@ class TestSetupCompleteCaching: # Cache was warm — is_setup_complete must not have been called. assert call_count == 0 + +# --------------------------------------------------------------------------- +# Task 0.1 — Lifespan creates the database parent directory (Task 0.1) +# --------------------------------------------------------------------------- + + +class TestLifespanDatabaseDirectoryCreation: + """App lifespan creates the database parent directory when it does not exist.""" + + async def test_creates_nested_database_directory(self, tmp_path: Path) -> None: + """Lifespan creates intermediate directories for the database path. + + Verifies that a deeply-nested database path is handled correctly — + the parent directories are created before ``aiosqlite.connect`` is + called so the app does not crash on a fresh volume. + """ + nested_db = tmp_path / "deep" / "nested" / "bangui.db" + assert not nested_db.parent.exists() + + settings = Settings( + database_path=str(nested_db), + fail2ban_socket="/tmp/fake.sock", + session_secret="test-lifespan-mkdir-secret", + session_duration_minutes=60, + timezone="UTC", + log_level="debug", + ) + app = create_app(settings=settings) + + mock_scheduler = MagicMock() + mock_scheduler.start = MagicMock() + mock_scheduler.shutdown = MagicMock() + + with ( + patch("app.services.geo_service.init_geoip"), + patch( + "app.services.geo_service.load_cache_from_db", + new=AsyncMock(return_value=None), + ), + patch("app.tasks.health_check.register"), + patch("app.tasks.blocklist_import.register"), + patch("app.tasks.geo_cache_flush.register"), + patch("app.tasks.geo_re_resolve.register"), + patch("app.main.AsyncIOScheduler", return_value=mock_scheduler), + ): + async with _lifespan(app): + assert nested_db.parent.exists(), ( + "Expected lifespan to create database parent directory" + ) + + async def test_existing_database_directory_is_not_an_error( + self, tmp_path: Path + ) -> None: + """Lifespan does not raise when the database directory already exists. + + ``mkdir(exist_ok=True)`` must be used so that re-starts on an existing + volume do not fail. + """ + db_path = tmp_path / "bangui.db" + # tmp_path already exists — this simulates a pre-existing volume. + + settings = Settings( + database_path=str(db_path), + fail2ban_socket="/tmp/fake.sock", + session_secret="test-lifespan-exist-ok-secret", + session_duration_minutes=60, + timezone="UTC", + log_level="debug", + ) + app = create_app(settings=settings) + + mock_scheduler = MagicMock() + mock_scheduler.start = MagicMock() + mock_scheduler.shutdown = MagicMock() + + with ( + patch("app.services.geo_service.init_geoip"), + patch( + "app.services.geo_service.load_cache_from_db", + new=AsyncMock(return_value=None), + ), + patch("app.tasks.health_check.register"), + patch("app.tasks.blocklist_import.register"), + patch("app.tasks.geo_cache_flush.register"), + patch("app.tasks.geo_re_resolve.register"), + patch("app.main.AsyncIOScheduler", return_value=mock_scheduler), + ): + # Should not raise FileExistsError or similar. + async with _lifespan(app): + assert tmp_path.exists() + + +# --------------------------------------------------------------------------- +# Task 0.2 — Middleware redirects when app.state.db is None +# --------------------------------------------------------------------------- + + +class TestSetupRedirectMiddlewareDbNone: + """SetupRedirectMiddleware redirects when the database is not yet available.""" + + async def test_redirects_to_setup_when_db_not_set(self, tmp_path: Path) -> None: + """A ``None`` db on app.state causes a 307 redirect to ``/api/setup``. + + Simulates the race window where a request arrives before the lifespan + has finished initialising the database connection. + """ + settings = Settings( + database_path=str(tmp_path / "bangui.db"), + fail2ban_socket="/tmp/fake_fail2ban.sock", + session_secret="test-db-none-secret", + session_duration_minutes=60, + timezone="UTC", + log_level="debug", + ) + app = create_app(settings=settings) + # Deliberately do NOT set app.state.db to simulate startup not complete. + + transport = ASGITransport(app=app) + async with AsyncClient( + transport=transport, base_url="http://test" + ) as ac: + response = await ac.get("/api/auth/login", follow_redirects=False) + + assert response.status_code == 307 + assert response.headers["location"] == "/api/setup" + + async def test_health_reachable_when_db_not_set(self, tmp_path: Path) -> None: + """Health endpoint is always reachable even when db is not initialised.""" + settings = Settings( + database_path=str(tmp_path / "bangui.db"), + fail2ban_socket="/tmp/fake_fail2ban.sock", + session_secret="test-db-none-health-secret", + session_duration_minutes=60, + timezone="UTC", + log_level="debug", + ) + app = create_app(settings=settings) + + transport = ASGITransport(app=app) + async with AsyncClient( + transport=transport, base_url="http://test" + ) as ac: + response = await ac.get("/api/health") + + assert response.status_code == 200 + diff --git a/frontend/src/components/SetupGuard.tsx b/frontend/src/components/SetupGuard.tsx index f448f9d..cb7d49a 100644 --- a/frontend/src/components/SetupGuard.tsx +++ b/frontend/src/components/SetupGuard.tsx @@ -33,9 +33,9 @@ export function SetupGuard({ children }: SetupGuardProps): React.JSX.Element { if (!cancelled) setStatus(res.completed ? "done" : "pending"); }) .catch((): void => { - // If the check fails, optimistically allow through — the backend will - // redirect API calls to /api/setup anyway. - if (!cancelled) setStatus("done"); + // A failed check conservatively redirects to /setup — a crashed + // backend cannot serve protected routes anyway. + if (!cancelled) setStatus("pending"); }); return (): void => { cancelled = true; diff --git a/frontend/src/components/__tests__/SetupGuard.test.tsx b/frontend/src/components/__tests__/SetupGuard.test.tsx new file mode 100644 index 0000000..c7e4a76 --- /dev/null +++ b/frontend/src/components/__tests__/SetupGuard.test.tsx @@ -0,0 +1,77 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, screen, waitFor } from "@testing-library/react"; +import { MemoryRouter, Routes, Route } from "react-router-dom"; +import { FluentProvider, webLightTheme } from "@fluentui/react-components"; +import { SetupGuard } from "../SetupGuard"; + +// Mock the setup API module so tests never hit a real network. +vi.mock("../../api/setup", () => ({ + getSetupStatus: vi.fn(), +})); + +import { getSetupStatus } from "../../api/setup"; + +const mockedGetSetupStatus = vi.mocked(getSetupStatus); + +function renderGuard() { + return render( + + + + +
Protected
+ + } + /> + Setup Page} + /> + + + , + ); +} + +describe("SetupGuard", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("shows a spinner while the setup status is loading", () => { + // getSetupStatus resolves eventually — spinner should show immediately. + mockedGetSetupStatus.mockReturnValue(new Promise(() => {})); + renderGuard(); + expect(screen.getByRole("progressbar")).toBeInTheDocument(); + }); + + it("renders children when setup is complete", async () => { + mockedGetSetupStatus.mockResolvedValue({ completed: true }); + renderGuard(); + await waitFor(() => { + expect(screen.getByTestId("protected-content")).toBeInTheDocument(); + }); + }); + + it("redirects to /setup when setup is not complete", async () => { + mockedGetSetupStatus.mockResolvedValue({ completed: false }); + renderGuard(); + await waitFor(() => { + expect(screen.getByTestId("setup-page")).toBeInTheDocument(); + }); + expect(screen.queryByTestId("protected-content")).not.toBeInTheDocument(); + }); + + it("redirects to /setup when the API call fails", async () => { + // Task 0.3: a failed check must redirect to /setup, not allow through. + mockedGetSetupStatus.mockRejectedValue(new Error("Network error")); + renderGuard(); + await waitFor(() => { + expect(screen.getByTestId("setup-page")).toBeInTheDocument(); + }); + expect(screen.queryByTestId("protected-content")).not.toBeInTheDocument(); + }); +}); diff --git a/frontend/src/pages/SetupPage.tsx b/frontend/src/pages/SetupPage.tsx index e7c0f1c..1c86628 100644 --- a/frontend/src/pages/SetupPage.tsx +++ b/frontend/src/pages/SetupPage.tsx @@ -101,20 +101,36 @@ export function SetupPage(): React.JSX.Element { const styles = useStyles(); const navigate = useNavigate(); + const [checking, setChecking] = useState(true); const [values, setValues] = useState(DEFAULT_VALUES); const [errors, setErrors] = useState>>({}); const [apiError, setApiError] = useState(null); const [submitting, setSubmitting] = useState(false); // Redirect to /login if setup has already been completed. + // Show a full-screen spinner while the check is in flight to prevent + // the form from flashing before the redirect fires. useEffect(() => { + let cancelled = false; getSetupStatus() .then((res) => { - if (res.completed) navigate("/login", { replace: true }); + if (!cancelled) { + if (res.completed) { + navigate("/login", { replace: true }); + } else { + setChecking(false); + } + } }) .catch(() => { - /* ignore — stay on setup page */ + // Failed check: the backend may still be starting up. Stay on this + // page so the user can attempt setup once the backend is ready. + console.warn("SetupPage: setup status check failed — rendering setup form"); + if (!cancelled) setChecking(false); }); + return (): void => { + cancelled = true; + }; }, [navigate]); // --------------------------------------------------------------------------- @@ -187,6 +203,21 @@ export function SetupPage(): React.JSX.Element { // Render // --------------------------------------------------------------------------- + if (checking) { + return ( +
+ +
+ ); + } + return (
diff --git a/frontend/src/pages/__tests__/SetupPage.test.tsx b/frontend/src/pages/__tests__/SetupPage.test.tsx new file mode 100644 index 0000000..96c76aa --- /dev/null +++ b/frontend/src/pages/__tests__/SetupPage.test.tsx @@ -0,0 +1,88 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, screen, waitFor } from "@testing-library/react"; +import { MemoryRouter, Routes, Route } from "react-router-dom"; +import { FluentProvider, webLightTheme } from "@fluentui/react-components"; +import { SetupPage } from "../SetupPage"; + +// Mock the setup API so tests never hit a real network. +vi.mock("../../api/setup", () => ({ + getSetupStatus: vi.fn(), + submitSetup: vi.fn(), +})); + +// Mock the crypto utility — we only need it to resolve without testing SHA256. +vi.mock("../../utils/crypto", () => ({ + sha256Hex: vi.fn().mockResolvedValue("hashed-password"), +})); + +import { getSetupStatus } from "../../api/setup"; + +const mockedGetSetupStatus = vi.mocked(getSetupStatus); + +function renderPage() { + return render( + + + + } /> + Login
} + /> + + + , + ); +} + +describe("SetupPage", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("shows a full-screen spinner while the setup status check is in flight", () => { + // getSetupStatus never resolves — spinner should be visible immediately. + mockedGetSetupStatus.mockReturnValue(new Promise(() => {})); + renderPage(); + expect(screen.getByRole("progressbar")).toBeInTheDocument(); + // Form should NOT be visible yet. + expect( + screen.queryByRole("heading", { name: /bangui setup/i }), + ).not.toBeInTheDocument(); + }); + + it("renders the setup form once the status check resolves (not complete)", async () => { + // Task 0.4: form must not flash before the check resolves. + mockedGetSetupStatus.mockResolvedValue({ completed: false }); + renderPage(); + await waitFor(() => { + expect( + screen.getByRole("heading", { name: /bangui setup/i }), + ).toBeInTheDocument(); + }); + // Spinner should be gone. + expect(screen.queryByRole("progressbar")).not.toBeInTheDocument(); + }); + + it("redirects to /login when setup is already complete", async () => { + mockedGetSetupStatus.mockResolvedValue({ completed: true }); + renderPage(); + await waitFor(() => { + expect(screen.getByTestId("login-page")).toBeInTheDocument(); + }); + }); + + it("renders the form and logs a warning when the status check fails", async () => { + // Task 0.4: catch block must log a warning and keep the form visible. + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + mockedGetSetupStatus.mockRejectedValue(new Error("Connection refused")); + renderPage(); + await waitFor(() => { + expect( + screen.getByRole("heading", { name: /bangui setup/i }), + ).toBeInTheDocument(); + }); + expect(warnSpy).toHaveBeenCalledOnce(); + warnSpy.mockRestore(); + }); +});