diff --git a/CHANGELOG.md b/CHANGELOG.md index c754a425..7f571c10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,21 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] ### Added +- **The `/setup` wizard ships with the framework** (#351). Since 0.0.33 + `SetupMiddleware` redirected a fresh install to `/setup`, but the route and + page lived only in this repository's unpublished host, so any other host got + `/` → `/setup` → 404. `create_app` now mounts the wizard + (`simple_module_hosting.setup_wizard`) and `gen-pages` registers its page + (`Setup/Wizard`) from the wheel; a host needs no setup code and should delete + any copy it carries. Steps complete from the browser through a new + `SetupStep.action` (`SetupAction` / `SetupField` in `simple_module_core`), + POSTed to `/setup/steps/` with the session CSRF token. An action runs only + while its own step is pending (409 otherwise); `users` ships the + first-administrator action, which re-checks under a database lock inside the + inserting transaction so concurrent requests create one superuser. Required + steps without an action are logged at boot. The old host-only + `/setup/administrator`, `/setup/migrations` and the UI-less + `/setup/site-basics` endpoints are gone. - **Postgres test runs** (#343) — `SM_TEST_DATABASE_URL` points the `simple_module_test` fixtures at Postgres, and `make test-py-pg` runs the whole Python suite there. The schema is reset once per test, so `app` and diff --git a/CLAUDE.md b/CLAUDE.md index 069a0f04..9aa714a8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,7 +76,7 @@ cascade layer is inert, while unlayered CSS beats every Tailwind utility — hence `SM022`/`SM023`. See `docs/module-authoring.md` § Styling. **Lifecycle hooks** (in `framework/core/simple_module_core/module.py`) — all no-op by default; subclasses override as needed: -`register_settings` → `register_menu_items` / `register_permissions` / `register_feature_flags` / `register_event_handlers` / `register_invalidations` / `register_health_checks` / `register_public_routes` / `register_csp_sources` / `register_setup_steps` → `register_exception_handlers` → `register_middleware` → `register_routes(api_router, view_router)` / `register_admin_routes(admin_router)` → async `on_startup` / `on_shutdown` (reverse order). `register_admin_routes` is only for modules that serve **both** public and admin pages: a module gets exactly one router per `view_prefix`, which `users` cannot express (sign-in at `/users/login`, management at `/admin/users`). Setting `ModuleMeta.admin_view_prefix` mounts a second view router there. A module whose views are *all* administrative just points `view_prefix` at `/admin/` and keeps using `register_routes`. The prefix is a URL convention, not a permission — guard these routes exactly as you would any other. `register_csp_sources(registry)` lets a module whitelist external asset origins (`registry.add("style-src", "https://rsms.me")`) — fetch directives only, validated at boot. `register_public_routes(registry)` lets a module exempt anonymous/read-only routes (STAC/OGC, webhooks) from `AuthMiddleware`; rules are method-aware (`registry.add_regex(r"…/tilejson$", methods={"GET"})`), so a GET read route can be public while sibling POST/PATCH mutations under the same prefix stay gated. See [docs/framework/public-routes.md](docs/framework/public-routes.md). `register_setup_steps(registry)` lets a module declare what a usable install still needs; while any required step is incomplete `SetupMiddleware` serves the first-run wizard at `/setup` instead of the app. A module that registers nothing never gates — that is how `keycloak` opts out, since its local users table is legitimately empty forever and a host-level superuser count would lock those installs out permanently. `register_invalidations(bus, app)` subscribes a module's **per-process caches** to `InvalidationBus`, so another worker's write drops this worker's entry instead of leaving it stale for its whole TTL; handlers may only *forget*, since there is no delivery guarantee. Publishing takes no hook — `await request.app.state.sm.invalidation.publish(channel, key=...)` from a `db.on_commit` callback. Cross-process delivery needs a transport, which `background_tasks` installs on its Redis connection (`SM_BG_TASKS_BROADCAST_INVALIDATIONS`); with none the bus is in-process and every cache still needs its TTL as a floor. See [docs/framework/invalidation.md](docs/framework/invalidation.md). +`register_settings` → `register_menu_items` / `register_permissions` / `register_feature_flags` / `register_event_handlers` / `register_invalidations` / `register_health_checks` / `register_public_routes` / `register_csp_sources` / `register_setup_steps` → `register_exception_handlers` → `register_middleware` → `register_routes(api_router, view_router)` / `register_admin_routes(admin_router)` → async `on_startup` / `on_shutdown` (reverse order). `register_admin_routes` is only for modules that serve **both** public and admin pages: a module gets exactly one router per `view_prefix`, which `users` cannot express (sign-in at `/users/login`, management at `/admin/users`). Setting `ModuleMeta.admin_view_prefix` mounts a second view router there. A module whose views are *all* administrative just points `view_prefix` at `/admin/` and keeps using `register_routes`. The prefix is a URL convention, not a permission — guard these routes exactly as you would any other. `register_csp_sources(registry)` lets a module whitelist external asset origins (`registry.add("style-src", "https://rsms.me")`) — fetch directives only, validated at boot. `register_public_routes(registry)` lets a module exempt anonymous/read-only routes (STAC/OGC, webhooks) from `AuthMiddleware`; rules are method-aware (`registry.add_regex(r"…/tilejson$", methods={"GET"})`), so a GET read route can be public while sibling POST/PATCH mutations under the same prefix stay gated. See [docs/framework/public-routes.md](docs/framework/public-routes.md). `register_setup_steps(registry)` lets a module declare what a usable install still needs; while any required step is incomplete `SetupMiddleware` serves the first-run wizard at `/setup` instead of the app. The wizard ships with `simple_module_hosting` (`setup_wizard/` — routes mounted by `create_app`, page `Setup/Wizard` registered by `gen-pages`), so hosts carry no setup code; a step completes from the browser through its `SetupAction`, which runs only while *that* step is pending. A module that registers nothing never gates — that is how `keycloak` opts out, since its local users table is legitimately empty forever and a host-level superuser count would lock those installs out permanently. `register_invalidations(bus, app)` subscribes a module's **per-process caches** to `InvalidationBus`, so another worker's write drops this worker's entry instead of leaving it stale for its whole TTL; handlers may only *forget*, since there is no delivery guarantee. Publishing takes no hook — `await request.app.state.sm.invalidation.publish(channel, key=...)` from a `db.on_commit` callback. Cross-process delivery needs a transport, which `background_tasks` installs on its Redis connection (`SM_BG_TASKS_BROADCAST_INVALIDATIONS`); with none the bus is in-process and every cache still needs its TTL as a floor. See [docs/framework/invalidation.md](docs/framework/invalidation.md). `MenuRegistry.add_provider(fn)` (from `register_menu_items`) contributes per-request menu items evaluated in `InertiaLayoutDataMiddleware` after auth/tenant resolution, and `PermissionRegistry.add_source(name, provider)` (from `register_permissions`) contributes runtime-defined permissions from a sync in-memory cache, refreshed with `invalidate_source(name)`; see [docs/framework/permissions.md](docs/framework/permissions.md). diff --git a/Makefile b/Makefile index 22501c43..22296852 100644 --- a/Makefile +++ b/Makefile @@ -135,7 +135,7 @@ ci-js-typecheck: exit 1; \ fi; \ done - @for cfg in modules/*/tsconfig.json packages/*/tsconfig.json; do \ + @for cfg in modules/*/tsconfig.json packages/*/tsconfig.json framework/*/tsconfig.json; do \ [ -f "$$cfg" ] || continue; \ echo "tsc -p $$cfg"; \ npx tsc --noEmit -p "$$cfg" || exit 1; \ diff --git a/biome.json b/biome.json index bdc3b459..5086cae3 100644 --- a/biome.json +++ b/biome.json @@ -7,6 +7,8 @@ "modules/*/*/pages/**", "modules/*/*/**/components/**", "modules/*/tests-js/**", + "framework/hosting/simple_module_hosting/setup_wizard/**", + "framework/hosting/tsconfig.json", "!.claude", "!host/client_app/modules.generated.ts", "!host/client_app/modules.manifest.json", diff --git a/docs/module-authoring.md b/docs/module-authoring.md index 3ac3dbcf..27932bc9 100644 --- a/docs/module-authoring.md +++ b/docs/module-authoring.md @@ -574,11 +574,13 @@ origin/scheme token, validated at boot. See ## First-run setup steps A module can declare what an install still needs before it is usable. While -any required step reports incomplete, `SetupMiddleware` serves the wizard at -`/setup` instead of the app: +any required step reports incomplete, `SetupMiddleware` redirects every +request to the wizard at `/setup`. The wizard ships with +`simple_module_hosting`: `create_app` mounts its routes and `smpy gen-pages` +registers its page (`Setup/Wizard`), so a host needs no code of its own for it. ```python -from simple_module_core import SetupRegistry, SetupStep +from simple_module_core import SetupAction, SetupField, SetupRegistry, SetupStep async def has_administrator(app) -> bool: @@ -586,20 +588,43 @@ async def has_administrator(app) -> bool: ... # return True once satisfied +async def create_administrator(request, data: dict) -> dict: + ... # validate `data`, lock, re-check, create; raise HTTPException to refuse + return {"created": True} + + class MyModule(ModuleBase): def register_setup_steps(self, registry: SetupRegistry) -> None: registry.add( SetupStep( id="mymodule.administrator", title="Create an administrator", + title_key="mymodule.setup.administrator.title", description="An account that can sign in and manage this install.", is_complete=has_administrator, order=30, + action=SetupAction( + handler=create_administrator, + fields=[ + SetupField(name="email", label="Email", type="email"), + SetupField(name="password", label="Password", type="password"), + ], + submit_label="Create administrator", + ), ) ) ``` -Three things are worth knowing before you add one. +The wizard lists every registered step and, for each pending step with an +`action`, renders `fields` as a form. Submitting it POSTs the values as JSON to +`/setup/steps/`, which calls `handler(request, data)` and returns its +dict. Titles, descriptions, field labels and the submit label reach the page as +backend data, so give each a `*_key` into your module's catalog; an unresolved +key falls back to the literal. A step with no `action` can only be completed +out of band (a CLI command, an environment variable); the host logs each such +required step at boot so an operator facing a form-less wizard can find out why. + +A few things are worth knowing before you add one. **Registering nothing is a valid answer, and it is how a module opts out.** The `users` module contributes the "an administrator exists" step; `keycloak` @@ -607,6 +632,21 @@ deliberately does not, because an install using an external identity provider has a legitimately empty local users table and a host-level superuser count would hold it behind the wizard forever. +**An action runs only while its own step is pending.** The wizard answers 404 +once setup is complete, and 409 for a step that is already done even while +other steps keep the wizard open. "Setup mode" alone is not a safe gate: the +host always registers `host.migrations`, so an install whose schema falls +behind head re-enters setup mode with its administrators intact. Every +`/setup` mutation also carries the session's CSRF token +(`simple_module_hosting.csrf`); the wizard page sends it for you. + +**An action that creates something unique must re-check under a lock.** The +step check runs before your handler and outside its transaction, so two +concurrent requests can both pass it. `users.setup_action` shows the pattern: +take a database lock (`pg_advisory_xact_lock` on Postgres, a no-op `UPDATE` +that claims SQLite's write lock), re-check the predicate, then insert, all in +one transaction. + **A step whose predicate raises counts as complete.** Failing closed on a transient database error would open an anonymous admin-creation form on a live install — that is failing *open* on security, so the framework fails the diff --git a/framework/cli/simple_module_cli/templates/host/migrations/env.py b/framework/cli/simple_module_cli/templates/host/migrations/env.py index 2b7b0f46..7df5aa20 100644 --- a/framework/cli/simple_module_cli/templates/host/migrations/env.py +++ b/framework/cli/simple_module_cli/templates/host/migrations/env.py @@ -27,7 +27,9 @@ config = context.config if config.config_file_name is not None: - fileConfig(config.config_file_name) + # Not the default disable_existing_loggers=True: the setup wizard runs this + # in-process, and that default would silence every app logger until restart. + fileConfig(config.config_file_name, disable_existing_loggers=False) target_metadata = build_module_metadata() include_object = make_include_object(target_metadata) diff --git a/framework/cli/simple_module_cli/templates/host/migrations/script.py.mako b/framework/cli/simple_module_cli/templates/host/migrations/script.py.mako index 6fcfd30e..3943349d 100644 --- a/framework/cli/simple_module_cli/templates/host/migrations/script.py.mako +++ b/framework/cli/simple_module_cli/templates/host/migrations/script.py.mako @@ -8,6 +8,7 @@ Create Date: ${create_date} from collections.abc import Sequence import sqlalchemy as sa +import sqlmodel # noqa: F401 (autogenerate may emit sqlmodel types) from alembic import op ${imports if imports else ""} diff --git a/framework/cli/tests/test_scaffolded_host_setup_wizard.py b/framework/cli/tests/test_scaffolded_host_setup_wizard.py new file mode 100644 index 00000000..44366703 --- /dev/null +++ b/framework/cli/tests/test_scaffolded_host_setup_wizard.py @@ -0,0 +1,57 @@ +"""A freshly scaffolded host gets a working /setup with no files of its own. + +GH #351: ``SetupMiddleware`` redirected every request to ``/setup`` while the +route and page lived only in the framework repo's unpublished host, so a host +made by ``smpy create-host`` answered ``/`` → ``/setup`` → 404. The wizard now +ships with ``simple_module_hosting``: ``create_app`` mounts the route and +``gen-pages`` registers the page, so the scaffold must stay free of both. +""" + +from __future__ import annotations + +import json +import re + +import pytest +from simple_module_hosting.manifest import write_module_pages_manifest +from simple_module_hosting.setup_wizard import PAGES_NAME, pages_dir + +pytestmark = pytest.mark.anyio + + +async def test_scaffold_carries_no_setup_code_and_resolves_the_wizard(tmp_path) -> None: + from simple_module_cli.scaffolding import create_host + + dest = tmp_path / "demo" + create_host(dest, name="demo-host", modules=[]) + client_app = dest / "client_app" + + # Nothing host-side: no route module, no page. + assert not (dest / "routes_setup.py").exists() + assert not list((client_app / "pages").rglob("Setup*")) + assert "setup" not in (dest / "main.py").read_text(encoding="utf-8").replace( + "setup_logging", "" + ) + + # What `smpy gen-pages` writes for this host, with no modules installed. + write_module_pages_manifest([], client_app, repo_root=dest) + + manifest = json.loads((client_app / "modules.manifest.json").read_text(encoding="utf-8")) + assert manifest[PAGES_NAME] == pages_dir().as_posix() + assert (pages_dir() / "Wizard.tsx").is_file() + + generated = (client_app / "modules.generated.ts").read_text(encoding="utf-8") + assert f'"{PAGES_NAME}": import.meta.glob' in generated + + # The scaffold's resolver keys a module glob entry as `/`, + # which is how `inertia.render("Setup/Wizard")` finds the page. + pages_ts = (client_app / "pages.ts").read_text(encoding="utf-8") + assert "pages[`${moduleName}/${match[1]}`]" in pages_ts + match = re.search(r"/pages/(.+)\.tsx$", (pages_dir() / "Wizard.tsx").as_posix()) + assert match and f"{PAGES_NAME}/{match.group(1)}" == "Setup/Wizard" + + # Tailwind must scan the wheel's wizard, and Vite must be allowed to serve it. + css = (client_app / "modules.generated.css").read_text(encoding="utf-8") + assert f'@source "{pages_dir().as_posix()}/**/*.{{ts,tsx}}";' in css + assets = json.loads((client_app / "modules.assets.json").read_text(encoding="utf-8")) + assert assets[PAGES_NAME]["pages"] == pages_dir().as_posix() diff --git a/framework/core/simple_module_core/__init__.py b/framework/core/simple_module_core/__init__.py index 44aafdb1..efb9576c 100644 --- a/framework/core/simple_module_core/__init__.py +++ b/framework/core/simple_module_core/__init__.py @@ -47,7 +47,7 @@ from simple_module_core.permissions import PermissionRegistry from simple_module_core.public_routes import PublicRoute, PublicRouteRegistry from simple_module_core.services import Services -from simple_module_core.setup_steps import SetupRegistry, SetupStep +from simple_module_core.setup_steps import SetupAction, SetupField, SetupRegistry, SetupStep from simple_module_core.tenancy import TENANT_ROLE_PREFIX, TenantRole, is_tenant_role, tenant_role from simple_module_core.versioning import FRAMEWORK_API_VERSION, check_framework_compatibility @@ -91,6 +91,8 @@ "PublicRoute", "PublicRouteRegistry", "Services", + "SetupAction", + "SetupField", "SetupRegistry", "SetupStep", "TenantRole", diff --git a/framework/core/simple_module_core/dotenv.py b/framework/core/simple_module_core/dotenv.py index fb529543..3f228c1f 100644 --- a/framework/core/simple_module_core/dotenv.py +++ b/framework/core/simple_module_core/dotenv.py @@ -38,7 +38,7 @@ def find_env_file() -> Path: settings layer (``BootstrapSettings``) and every out-of-process tool (diagnostics CLI, worker entrypoints, users bootstrap) resolve through here, so they can never disagree about which file is in effect. Compare - ``app_builder._resolve_project_root`` in the hosting package — a + ``_project_root.resolve_project_root`` in the hosting package — a separate walk that anchors the static/i18n root instead; the two are kept distinct on purpose (see that function's docstring). """ diff --git a/framework/core/simple_module_core/setup_steps.py b/framework/core/simple_module_core/setup_steps.py index 9aa31e3b..e0ce8534 100644 --- a/framework/core/simple_module_core/setup_steps.py +++ b/framework/core/simple_module_core/setup_steps.py @@ -28,6 +28,55 @@ # real check is a database query. SetupCheckFn = Callable[..., Awaitable[bool]] +# Takes ``(request, data)`` — the Starlette request and the submitted form as a +# dict — and returns a JSON-able dict (or ``None``). Typed loosely so core does +# not depend on Starlette; the wizard in ``simple_module_hosting`` calls it. +SetupActionFn = Callable[..., Awaitable[dict | None]] + + +@dataclass +class SetupField: + """One input the wizard renders for a :class:`SetupAction`. + + ``label`` reaches the page as backend data, so it is resolved server-side + through ``label_key`` with the literal as the fallback — the same rule as + ``SetupStep.title``. + """ + + name: str + label: str + label_key: str = "" + type: str = "text" + """The HTML input type: ``text``, ``email``, ``password``, ...""" + required: bool = True + autocomplete: str = "" + min_length: int | None = None + + +@dataclass +class SetupAction: + """How the wizard lets an operator complete a step from the browser. + + The wizard renders ``fields`` as a form and POSTs it as JSON to + ``/setup/steps/``, which calls ``handler(request, data)``. + + The wizard only calls the handler while **this step** reports incomplete, + never merely while "setup mode" is on: an install whose schema falls behind + head re-enters setup mode with its administrators intact, and an action + gated on the weaker condition would let an anonymous request perform it + there. The handler still owns its own race: two requests can both pass that + check, so a handler that creates something unique must re-check inside the + transaction that creates it. + + A step without an action can only be completed out of band (a CLI, an + environment variable); the host logs such steps at boot. + """ + + handler: SetupActionFn + fields: list[SetupField] = field(default_factory=list) + submit_label: str = "Continue" + submit_label_key: str = "" + @dataclass class SetupStep: @@ -63,6 +112,8 @@ class SetupStep: """Catalog key for ``description``, with the same fallback rule.""" required: bool = True order: int = 100 + action: SetupAction | None = None + """How the wizard completes this step; ``None`` for out-of-band steps.""" module: str = field(default="") @@ -96,6 +147,10 @@ def all_steps(self) -> list[SetupStep]: """Every registered step, required or not, in display order.""" return sorted(self._steps, key=lambda s: s.order) + def get(self, step_id: str) -> SetupStep | None: + """The step registered under *step_id*, or ``None``.""" + return next((s for s in self._steps if s.id == step_id), None) + @property def required_steps(self) -> list[SetupStep]: return [s for s in self.all_steps if s.required] @@ -137,5 +192,14 @@ async def incomplete_all(self, app) -> list[SetupStep]: """ return await self._evaluate(app, self.all_steps) + async def is_pending(self, app, step: SetupStep) -> bool: + """Whether *step* specifically is still unsatisfied. + + Same fail-safe as :meth:`incomplete`: a raising predicate counts as + complete, so a database hiccup closes a step's action rather than + opening it. + """ + return bool(await self._evaluate(app, [step])) + async def is_setup_complete(self, app) -> bool: return not await self.incomplete(app) diff --git a/framework/hosting/simple_module_hosting/_lifespan.py b/framework/hosting/simple_module_hosting/_lifespan.py index cdbe7d07..8e623d11 100644 --- a/framework/hosting/simple_module_hosting/_lifespan.py +++ b/framework/hosting/simple_module_hosting/_lifespan.py @@ -13,6 +13,8 @@ from __future__ import annotations +import asyncio +import logging from collections.abc import AsyncGenerator, Callable, Sequence from contextlib import asynccontextmanager @@ -21,6 +23,8 @@ from simple_module_hosting.migrations import migration_status from simple_module_hosting.setup_gate import STEP_MIGRATIONS +logger = logging.getLogger(__name__) + async def hydrate_settings_from_db(app: FastAPI) -> None: """Merge DB-stored overrides into every registered settings object. @@ -98,6 +102,34 @@ async def _is_first_run(app: FastAPI) -> bool: return False +async def run_deferred_startup(app: FastAPI) -> None: + """Replay the ``on_startup`` hooks that could not run on an unmigrated DB. + + Called from the wizard's migrations action and from the setup gate's + schema re-check, so a worker that did not itself run the migrations (or an + operator's out-of-band ``make migrate``) still finishes starting its + modules. Serialised per app; a hook that fails again is logged and dropped + rather than raised, because this runs after the migrations have already + committed and a 500 there would help nobody. + """ + deferred = getattr(app.state, "deferred_startup", None) + if not deferred: + return + lock = getattr(app.state, "deferred_startup_lock", None) + if lock is None: # no await between check and set, so this cannot race + lock = app.state.deferred_startup_lock = asyncio.Lock() + async with lock: + if not deferred: + return + await hydrate_settings_from_db(app) + while deferred: + mod = deferred.pop(0) + try: + await mod.on_startup(app) + except Exception: + logger.exception("Deferred on_startup of %s failed after migrations", mod.meta.name) + + def build_lifespan(modules: Sequence) -> Callable: """Return the ``lifespan`` context manager for an app over *modules*.""" @@ -120,8 +152,25 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: await hydrate_settings_from_db(app) + # Behind head on a first run the tables a module's on_startup reads do + # not exist yet. Boot must still reach the wizard, so a hook that fails + # here is deferred and replayed once the wizard has applied the + # migrations (run_deferred_startup) rather than aborting the process. + app.state.deferred_startup = [] + tolerate = not app.state.migration["is_current"] for mod in modules: - await mod.on_startup(app) + try: + await mod.on_startup(app) + except Exception: + if not tolerate: + raise + logger.warning( + "on_startup of %s failed on an unmigrated database; " + "deferring it until the setup wizard has run the migrations", + mod.meta.name, + exc_info=True, + ) + app.state.deferred_startup.append(mod) yield for mod in reversed(modules): await mod.on_shutdown(app) diff --git a/framework/hosting/simple_module_hosting/_project_root.py b/framework/hosting/simple_module_hosting/_project_root.py new file mode 100644 index 00000000..a863b4ab --- /dev/null +++ b/framework/hosting/simple_module_hosting/_project_root.py @@ -0,0 +1,39 @@ +"""Where the host project lives: anchors static files and i18n catalogs.""" + +from __future__ import annotations + +import os +from pathlib import Path + +_ENV_PROJECT_ROOT = "SM_PROJECT_ROOT" + + +_PROJECT_ROOT_SENTINELS = ("pyproject.toml", ".env", "alembic.ini") + + +def resolve_project_root() -> Path: + """Return the project root directory. + + Prefers the ``SM_PROJECT_ROOT`` environment variable when set. + + Otherwise walks up from the current working directory looking for a + project sentinel (``pyproject.toml``, ``.env`` or ``alembic.ini``). This + works whether the framework is installed as a wheel into ``site-packages`` + or run from a workspace clone. + + Falls back to ``parents[3]`` for the in-tree dev loop only when the walk + finds nothing — which still keeps ``framework/`` users working without + setting the env var explicitly. + + Compare ``simple_module_core.dotenv.find_env_file``: both honor + ``SM_PROJECT_ROOT`` first, but this anchors the static/i18n root while + that anchors which ``.env`` loads — different sentinels, kept separate. + """ + override = os.environ.get(_ENV_PROJECT_ROOT) + if override: + return Path(override) + cwd = Path.cwd().resolve() + for candidate in (cwd, *cwd.parents): + if any((candidate / s).exists() for s in _PROJECT_ROOT_SENTINELS): + return candidate + return Path(__file__).resolve().parents[3] diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index 9fc7e74d..15b83d10 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -3,8 +3,6 @@ from __future__ import annotations import logging -import os -from pathlib import Path from fastapi import FastAPI from simple_module_core import BodyLimitRegistry, CspSourceRegistry @@ -37,6 +35,7 @@ wire_module_routes, ) from simple_module_hosting._preapp_config import merge_host_settings +from simple_module_hosting._project_root import resolve_project_root as _resolve_project_root from simple_module_hosting._registrations import run_module_registrations from simple_module_hosting._secret_key import assert_not_placeholder from simple_module_hosting._settings_registration import ( @@ -47,6 +46,7 @@ from simple_module_hosting.i18n_manifest import build_i18n_registry from simple_module_hosting.settings import Settings from simple_module_hosting.setup_gate import register_migration_step +from simple_module_hosting.setup_wizard import mount_setup_wizard from simple_module_hosting.static_files import PrecompressedStaticFiles logger = logging.getLogger(__name__) @@ -57,40 +57,6 @@ _REDOC_URL = "/api/redoc" _STATIC_MOUNT_PATH = "/static" _STATIC_DIR_NAME = "static" -_ENV_PROJECT_ROOT = "SM_PROJECT_ROOT" - - -_PROJECT_ROOT_SENTINELS = ("pyproject.toml", ".env", "alembic.ini") - - -def _resolve_project_root() -> Path: - """Return the project root directory. - - Prefers the ``SM_PROJECT_ROOT`` environment variable when set. - - Otherwise walks up from the current working directory looking for a - project sentinel (``pyproject.toml``, ``.env`` or ``alembic.ini``). This - works whether the framework is installed as a wheel into ``site-packages`` - or run from a workspace clone. - - Falls back to ``parents[3]`` for the in-tree dev loop only when the walk - finds nothing — which still keeps ``framework/`` users working without - setting the env var explicitly. - - Compare ``simple_module_core.dotenv.find_env_file``: both honor - ``SM_PROJECT_ROOT`` first, but this anchors the static/i18n root while - that anchors which ``.env`` loads — different sentinels, kept separate. - """ - override = os.environ.get(_ENV_PROJECT_ROOT) - if override: - return Path(override) - cwd = Path.cwd().resolve() - for candidate in (cwd, *cwd.parents): - if any((candidate / s).exists() for s in _PROJECT_ROOT_SENTINELS): - return candidate - return Path(__file__).resolve().parents[3] - - _PROJECT_ROOT = _resolve_project_root() @@ -270,6 +236,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: wire_module_routes(app, mod) app.include_router(health_router) + mount_setup_wizard(app, setup_registry) # /setup — what SetupMiddleware redirects to static_dir = _PROJECT_ROOT / "host" / _STATIC_DIR_NAME if static_dir.is_dir(): diff --git a/framework/hosting/simple_module_hosting/assets.py b/framework/hosting/simple_module_hosting/assets.py index 11ce696b..7c3f1b28 100644 --- a/framework/hosting/simple_module_hosting/assets.py +++ b/framework/hosting/simple_module_hosting/assets.py @@ -136,6 +136,28 @@ def compute_module_assets(modules: Sequence[ModuleBase]) -> list[ModuleAssets]: return result +def framework_assets() -> ModuleAssets: + """The frontend the framework itself ships: the ``/setup`` wizard. + + Emitted through the same records as a module's assets so a host's + ``vite.config.ts`` treats it identically — allowed in ``server.fs``, its + bare imports resolved against the host's ``node_modules``, its classes + scanned by Tailwind — with nothing for the host to add by hand. + """ + from simple_module_hosting.setup_wizard import PAGES_NAME, package_dir, pages_dir + + root = package_dir() + return ModuleAssets( + name=PAGES_NAME, + package_name="simple_module_hosting.setup_wizard", + package_dir=root, + pages_dir=pages_dir(), + theme_css=None, + styles_css=None, + components_dir=root / COMPONENTS_DIR, + ) + + _CSS_HEADER = """\ /* AUTO-GENERATED by simple_module_hosting.assets — do not edit by hand. * Regenerate with: smpy gen-pages diff --git a/framework/hosting/simple_module_hosting/i18n_manifest.py b/framework/hosting/simple_module_hosting/i18n_manifest.py index a4439be4..79e258a3 100644 --- a/framework/hosting/simple_module_hosting/i18n_manifest.py +++ b/framework/hosting/simple_module_hosting/i18n_manifest.py @@ -50,6 +50,12 @@ def build_i18n_registry( for namespace, locale_dir in mod.locale_dirs().items(): registry.add_source(namespace, locale_dir, audience=audience) + # The framework's own catalog — the setup wizard ships with this package, + # so its strings must too, whatever host is serving it. + hosting_locales = Path(__file__).resolve().parent / "locales" + registry.add_source("hosting", hosting_locales) + extra_sources.append(("simple_module_hosting", "hosting", hosting_locales)) + host_locales = project_root / "host" / "locales" if host_locales.is_dir(): registry.add_source("host", host_locales) diff --git a/framework/hosting/simple_module_hosting/locales/en.json b/framework/hosting/simple_module_hosting/locales/en.json new file mode 100644 index 00000000..8ee076b9 --- /dev/null +++ b/framework/hosting/simple_module_hosting/locales/en.json @@ -0,0 +1,24 @@ +{ + "setup": { + "title": "Set up your install", + "subtitle": "A few things before this application is ready to use.", + "connections": { + "heading": "Connections", + "description": "Checking the services this install depends on.", + "retest": "Test again", + "testing": "Testing…" + }, + "migrations": { + "apply": "Apply migrations" + }, + "steps": { + "heading": "Setup steps", + "migrations": { + "title": "Apply database migrations", + "description": "Bring the database schema up to the version this code expects." + } + }, + "working": "Working…", + "done": "Done. Reloading…" + } +} diff --git a/framework/hosting/simple_module_hosting/locales/es.json b/framework/hosting/simple_module_hosting/locales/es.json new file mode 100644 index 00000000..59868a7f --- /dev/null +++ b/framework/hosting/simple_module_hosting/locales/es.json @@ -0,0 +1,24 @@ +{ + "setup": { + "title": "Configura tu instalación", + "subtitle": "Unas cuantas cosas antes de que esta aplicación esté lista para usarse.", + "connections": { + "heading": "Conexiones", + "description": "Comprobando los servicios de los que depende esta instalación.", + "retest": "Probar de nuevo", + "testing": "Probando…" + }, + "migrations": { + "apply": "Aplicar migraciones" + }, + "steps": { + "heading": "Pasos de configuración", + "migrations": { + "title": "Aplicar las migraciones de la base de datos", + "description": "Actualiza el esquema de la base de datos a la versión que espera este código." + } + }, + "working": "Procesando…", + "done": "Hecho. Recargando…" + } +} diff --git a/framework/hosting/simple_module_hosting/manifest.py b/framework/hosting/simple_module_hosting/manifest.py index 9bbb86d0..f77e92a5 100644 --- a/framework/hosting/simple_module_hosting/manifest.py +++ b/framework/hosting/simple_module_hosting/manifest.py @@ -26,6 +26,7 @@ from simple_module_hosting.assets import ( compute_module_assets, + framework_assets, render_assets_json, render_modules_css, ) @@ -159,7 +160,11 @@ def write_module_pages_manifest( if repo_root is None: repo_root = repo_root_from_client_app(output_dir) - pages_map = compute_module_pages(modules) + # The framework's own frontend (the setup wizard) is emitted alongside the + # modules', so every host's resolver finds ``Setup/Wizard`` and its Vite + # config serves it from the wheel — with no host-side file to add. + framework = framework_assets() + pages_map = {**compute_module_pages(modules), framework.name: framework.pages_dir} manifest_path = output_dir / "modules.manifest.json" manifest_payload = {name: path.as_posix() for name, path in pages_map.items()} @@ -186,7 +191,7 @@ def write_module_pages_manifest( # Rendered from the richer asset record rather than pages_map, so that a # module shipping CSS but no pages/ still contributes its stylesheets. - assets = compute_module_assets(modules) + assets = [*compute_module_assets(modules), framework] css_path = output_dir / "modules.generated.css" css_text = render_modules_css(assets, in_repo=lambda p: _is_in_repo_module(p, repo_root)) diff --git a/framework/hosting/simple_module_hosting/setup_gate.py b/framework/hosting/simple_module_hosting/setup_gate.py index 1ca6d7e5..d5e71258 100644 --- a/framework/hosting/simple_module_hosting/setup_gate.py +++ b/framework/hosting/simple_module_hosting/setup_gate.py @@ -78,6 +78,16 @@ async def _database_migrated(app) -> bool: logger.debug("Migration re-check failed, using the boot snapshot: %s", exc) return False app.state.migration = status + if status["is_current"]: + # Finish starting the modules whose on_startup needed these tables — + # this worker may not be the one that ran the migrations. Never lets + # a failure there change the verdict, which only reports the schema. + from simple_module_hosting._lifespan import run_deferred_startup + + try: + await run_deferred_startup(app) + except Exception: + logger.exception("Replaying deferred on_startup hooks failed") return bool(status["is_current"]) @@ -87,18 +97,25 @@ def register_migration_step(registry) -> None: Host-owned rather than module-owned because no module owns the schema as a whole — it is the union of whatever modules are installed. """ - from simple_module_core.setup_steps import SetupStep + from simple_module_core.setup_steps import SetupAction, SetupStep + + from simple_module_hosting.setup_wizard.migrate import apply_migrations registry.set_owner("Host") registry.add( SetupStep( id=STEP_MIGRATIONS, title="Apply database migrations", - title_key="host.setup.steps.migrations.title", + title_key="hosting.setup.steps.migrations.title", description="Bring the database schema up to the version this code expects.", - description_key="host.setup.steps.migrations.description", + description_key="hosting.setup.steps.migrations.description", is_complete=_database_migrated, order=20, + action=SetupAction( + handler=apply_migrations, + submit_label="Apply migrations", + submit_label_key="hosting.setup.migrations.apply", + ), ) ) registry.set_owner("") @@ -111,6 +128,7 @@ def __init__(self, app: ASGIApp) -> None: self.app = app self._verdict: bool | None = None self._verdict_expires: float = 0.0 + self._announced = False async def _is_complete(self, registry, starlette_app) -> bool: """``registry.is_setup_complete`` behind a short TTL cache. @@ -168,6 +186,15 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: await self.app(scope, receive, send) return + if not self._announced: + # Once per process: a fresh install answering 302 to everything is + # baffling without a line in the log that says why. + self._announced = True + logger.warning( + "Setup is incomplete; redirecting requests to %s until every " + "required setup step is complete.", + SETUP_PATH, + ) await self._redirect(scope, receive, send) async def _redirect(self, scope: Scope, receive: Receive, send: Send) -> None: diff --git a/framework/hosting/simple_module_hosting/setup_wizard/__init__.py b/framework/hosting/simple_module_hosting/setup_wizard/__init__.py new file mode 100644 index 00000000..994429f6 --- /dev/null +++ b/framework/hosting/simple_module_hosting/setup_wizard/__init__.py @@ -0,0 +1,72 @@ +"""The first-run setup wizard, shipped with the framework. + +``SetupMiddleware`` redirects every request to ``/setup`` while a required +:class:`~simple_module_core.setup_steps.SetupStep` is incomplete. The wizard +those redirects land on used to live in this repository's own host, so any +other host taking the package got ``/`` → ``/setup`` → 404 (GH #351). +``create_app`` now mounts it for every host. + +The wizard is generic: it lists the registered steps and renders a form for +each pending step that carries a :class:`~simple_module_core.setup_steps.SetupAction`. +What a step's form *does* belongs to the module that owns the step — the +framework never imports a plugin module (SM009); the module hands the wizard a +handler instead. + +The page ships from this package too: :func:`pages_dir` is registered in +``modules.generated.ts`` under :data:`PAGES_NAME`, exactly like a module's +``pages/``, so a host's Vite build picks it up from the wheel. +""" + +from __future__ import annotations + +import logging +from pathlib import Path + +logger = logging.getLogger(__name__) + +#: The ``modules.generated.ts`` key the wizard's pages are registered under — +#: ``pages/Wizard.tsx`` resolves as the Inertia page ``Setup/Wizard``. +PAGES_NAME = "Setup" + +_PACKAGE_DIR = Path(__file__).resolve().parent + + +def package_dir() -> Path: + """The wizard's frontend root (``pages/`` and ``components/``).""" + return _PACKAGE_DIR + + +def pages_dir() -> Path: + return _PACKAGE_DIR / "pages" + + +def mount_setup_wizard(app, setup_registry) -> None: + """Mount ``/setup`` and report steps the wizard cannot complete. + + Mounted unconditionally: every route refuses (404) once setup is complete, + so on a configured install this is inert. An install with no registered + step never reaches it, because the middleware never redirects there. + """ + from simple_module_hosting.setup_wizard.routes import router + + app.include_router(router) + report_unactionable_steps(setup_registry) + + +def report_unactionable_steps(setup_registry) -> None: + """Log required steps that carry no wizard action. + + Such a step can only be completed out of band — a CLI command, an + environment variable. Without this line an operator facing a wizard with + no form has nothing to tell them why, which is the silent dead end + GH #351 reported. + """ + for step in setup_registry.required_steps: + if step.action is None: + logger.warning( + "Setup step %r (module %r) is required but offers no wizard action; " + "while it is incomplete the app redirects to /setup and the step " + "must be completed out of band.", + step.id, + step.module or "host", + ) diff --git a/host/client_app/pages/Setup/ConnectionList.tsx b/framework/hosting/simple_module_hosting/setup_wizard/components/ConnectionList.tsx similarity index 88% rename from host/client_app/pages/Setup/ConnectionList.tsx rename to framework/hosting/simple_module_hosting/setup_wizard/components/ConnectionList.tsx index 8f0c4222..79e93016 100644 --- a/host/client_app/pages/Setup/ConnectionList.tsx +++ b/framework/hosting/simple_module_hosting/setup_wizard/components/ConnectionList.tsx @@ -17,7 +17,13 @@ export interface CheckResult { * different fixes, and an operator staring at a red dot has no way to tell * which one they have. */ -export function ConnectionList({ initial }: { initial: CheckResult[] }) { +export function ConnectionList({ + initial, + csrfToken, +}: { + initial: CheckResult[]; + csrfToken: string; +}) { const { t } = useT(); const [checks, setChecks] = useState(initial); const [busy, setBusy] = useState(false); @@ -29,7 +35,7 @@ export function ConnectionList({ initial }: { initial: CheckResult[] }) { try { const resp = await fetch('/setup/test-connections', { method: 'POST', - headers: { Accept: 'application/json' }, + headers: { Accept: 'application/json', 'X-CSRF-Token': csrfToken }, }); // A 404 here means setup completed in another tab, and the body is not // the JSON this expects. Without the check, `resp.json()` throws into an @@ -69,7 +75,9 @@ export function ConnectionList({ initial }: { initial: CheckResult[] }) { ); diff --git a/framework/hosting/simple_module_hosting/setup_wizard/components/StepForm.tsx b/framework/hosting/simple_module_hosting/setup_wizard/components/StepForm.tsx new file mode 100644 index 00000000..cbd5084f --- /dev/null +++ b/framework/hosting/simple_module_hosting/setup_wizard/components/StepForm.tsx @@ -0,0 +1,132 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { Button } from '@simple-module-py/ui/components/ui/button'; +import { Input } from '@simple-module-py/ui/components/ui/input'; +import { Label } from '@simple-module-py/ui/components/ui/label'; +import { useState } from 'react'; + +/** One input of a step's form — `SetupField` on the server. */ +export interface StepField { + name: string; + label: string; + type: string; + required: boolean; + autocomplete: string; + minLength: number | null; +} + +/** `SetupAction` on the server, minus the handler. Labels arrive translated. */ +export interface StepAction { + submitLabel: string; + fields: StepField[]; +} + +/** + * Turn a FastAPI error body into one line of text. + * + * `detail` is a string for `HTTPException`, but an array of + * `{loc, msg, ...}` objects for a 422 — which is exactly what a short password + * or a malformed address produces. Interpolating that array straight into an + * Error yields "[object Object]", the one form of the message that tells the + * operator nothing. + */ +export function errorMessage(body: unknown, fallback: string): string { + const detail = (body as { detail?: unknown })?.detail; + if (typeof detail === 'string' && detail) return detail; + if (Array.isArray(detail)) { + const parts = detail + .map((item) => (typeof item === 'string' ? item : (item as { msg?: string })?.msg)) + .filter(Boolean); + if (parts.length > 0) return parts.join('; '); + } + return fallback; +} + +/** + * Completes one setup step through the action its module registered. + * + * On success the browser goes to `/` rather than reloading the wizard: if this + * was the last required step every /setup route now 404s, and if it was not, + * the gate sends the browser straight back here. + */ +export function StepForm({ + stepId, + action, + csrfToken, +}: { + stepId: string; + action: StepAction; + csrfToken: string; +}) { + const { t } = useT(); + const [busy, setBusy] = useState(false); + const [error, setError] = useState(null); + const [done, setDone] = useState(false); + + async function submit(event: React.FormEvent) { + event.preventDefault(); + setBusy(true); + setError(null); + + const form = new FormData(event.currentTarget); + const data: Record = {}; + for (const field of action.fields) { + const value = form.get(field.name); + // An empty optional field is "not given", not an empty string. + data[field.name] = typeof value === 'string' && value !== '' ? value : null; + } + + try { + const resp = await fetch(`/setup/steps/${encodeURIComponent(stepId)}`, { + method: 'POST', + // Accept: application/json matters. The host renders an Inertia error + // *page* for a 4xx unless the caller prefers JSON, and a bare fetch() + // sends Accept: */* — so without it the body is HTML, json() throws, + // and the operator never sees why the request was refused. + headers: { + 'Content-Type': 'application/json', + Accept: 'application/json', + 'X-CSRF-Token': csrfToken, + }, + body: JSON.stringify(data), + }); + if (!resp.ok) { + const body = await resp.json().catch(() => ({})); + throw new Error(errorMessage(body, resp.statusText || String(resp.status))); + } + setDone(true); + window.location.href = '/'; + } catch (err) { + setError((err as Error).message); + } finally { + setBusy(false); + } + } + + return ( +
+ {action.fields.map((field) => { + const id = `setup-${stepId}-${field.name}`; + return ( +
+ + +
+ ); + })} + + {error &&

{error}

} + {done &&

{t(keys.hosting.setup.done)}

} + + +
+ ); +} diff --git a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py new file mode 100644 index 00000000..3b214b39 --- /dev/null +++ b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py @@ -0,0 +1,92 @@ +"""The ``host.migrations`` step's wizard action: ``alembic upgrade heads``. + +Reachable only while that step is pending — the wizard refuses a completed +step's action — which is what bounds an endpoint that runs migrations over +HTTP. An unmigrated database otherwise means dropping the operator to a shell, +the sharpest edge in the whole onboarding path. +""" + +from __future__ import annotations + +import asyncio +import contextlib +import logging +from collections.abc import Generator + +from fastapi import HTTPException, Request + +logger = logging.getLogger(__name__) + +# The endpoint is anonymous, so nothing stops two requests (a double click, a +# second tab) from starting two Alembic runs against one database at once. +# Serialized here; the second run then finds the schema at head and is a no-op. +_MIGRATION_LOCK = asyncio.Lock() + + +@contextlib.contextmanager +def _preserve_logging() -> Generator[None]: + """Undo what the migration env's ``fileConfig`` does to the process's logging. + + ``fileConfig`` replaces the root logger's handlers and level with + ``alembic.ini``'s (a bare WARN console handler) and, for an ``env.py`` that + predates ``disable_existing_loggers=False``, disables every existing + logger. Run in-process, that would strip the app's JSON formatter and + correlation filter and silence its INFO logs until restart. + """ + root = logging.getLogger() + handlers, level = list(root.handlers), root.level + disabled = { + name: lg.disabled + for name, lg in logging.root.manager.loggerDict.items() + if isinstance(lg, logging.Logger) + } + try: + yield + finally: + root.handlers[:] = handlers + root.setLevel(level) + for name, was_disabled in disabled.items(): + logging.getLogger(name).disabled = was_disabled + + +async def apply_migrations(request: Request, _data: dict) -> dict: + """Run every module's migrations to head and refresh the boot snapshot.""" + from alembic import command + from alembic.config import Config as AlembicConfig + + from simple_module_hosting.migrations import default_alembic_ini, migration_status + + # Resolved through the hosting helper rather than hardcoded: this runs + # inside a request, and a literal "host/alembic.ini" is only correct while + # the process cwd happens to be the project root. + ini_path = default_alembic_ini() + + def _upgrade() -> None: + # "heads", not "head": each module's first migration sets its own + # branch_labels, so the history legitimately has several heads and + # "head" raises CommandError("Multiple head revisions are present"). + # This is what `make migrate` runs. + with _preserve_logging(): + command.upgrade(AlembicConfig(ini_path), "heads") + + try: + async with _MIGRATION_LOCK: + await asyncio.to_thread(_upgrade) + except Exception as exc: + # The caller is anonymous, and a migration error routinely carries the + # database URL, SQL or filesystem paths — so the detail goes to the + # log only, and the response points the operator at it. + correlation_id = getattr(request.state, "correlation_id", "") or "" + logger.exception("Setup: migration run failed (correlation_id=%s)", correlation_id) + detail = "Migrations failed; see the server log" + if correlation_id: + detail += f" (correlation id {correlation_id})" + raise HTTPException(status_code=500, detail=detail + ".") from exc + + request.app.state.migration = await migration_status(request.app.state.sm.db.engine) + # Modules whose on_startup could not run before the tables existed. + from simple_module_hosting._lifespan import run_deferred_startup + + await run_deferred_startup(request.app) + logger.info("Setup: migrations applied") + return {"migration": request.app.state.migration} diff --git a/host/client_app/pages/Setup/Wizard.tsx b/framework/hosting/simple_module_hosting/setup_wizard/pages/Wizard.tsx similarity index 50% rename from host/client_app/pages/Setup/Wizard.tsx rename to framework/hosting/simple_module_hosting/setup_wizard/pages/Wizard.tsx index 12845e0e..cd29a26c 100644 --- a/host/client_app/pages/Setup/Wizard.tsx +++ b/framework/hosting/simple_module_hosting/setup_wizard/pages/Wizard.tsx @@ -1,6 +1,5 @@ import { Head, usePage } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; -import { Button } from '@simple-module-py/ui/components/ui/button'; import { Card, CardContent, @@ -10,70 +9,45 @@ import { } from '@simple-module-py/ui/components/ui/card'; import { BRAND_ACCENT, BRAND_DEFAULT_APP_NAME } from '@simple-module-py/ui/lib/brand'; import type { SharedProps } from '@simple-module-py/ui/types'; -import { CheckCircle2, Circle, Database } from 'lucide-react'; -import { useState } from 'react'; -import { AdministratorForm } from './AdministratorForm'; -import { type CheckResult, ConnectionList } from './ConnectionList'; +import { CheckCircle2, Circle } from 'lucide-react'; +import { type CheckResult, ConnectionList } from '../components/ConnectionList'; +import { type StepAction, StepForm } from '../components/StepForm'; interface SetupStep { id: string; title: string; description: string; complete: boolean; -} - -interface MigrationState { - current: string | null; - head: string | null; - isCurrent: boolean; + /** Present only while the step is pending and its module offers a form. */ + action: StepAction | null; } interface WizardProps { checks: CheckResult[]; steps: SetupStep[]; - migration: MigrationState; + csrfToken: string; } /** - * First-run setup. + * First-run setup, shipped by `simple_module_hosting`. * * Served in place of the app while any required step is incomplete, and - * unreachable (404) the moment they all pass — which is also what bounds the - * migration button below, an endpoint that can run Alembic over HTTP. + * unreachable (404) the moment they all pass. Each pending step whose module + * registered an action gets its own form; the rest are listed so the operator + * can see what is left and complete it out of band. */ function Wizard() { const { t } = useT(); const page = usePage<{ props: WizardProps & SharedProps }>().props as unknown as WizardProps & SharedProps; - const { checks, steps, migration, branding } = page; - const [migrating, setMigrating] = useState(false); - const [migrationError, setMigrationError] = useState(null); + const { checks, steps, csrfToken, branding } = page; const appName = branding?.appName ?? BRAND_DEFAULT_APP_NAME; const brandInitial = appName.trim().charAt(0).toUpperCase() || 'S'; - async function applyMigrations() { - setMigrating(true); - setMigrationError(null); - try { - const resp = await fetch('/setup/migrations', { - method: 'POST', - headers: { Accept: 'application/json' }, - }); - if (!resp.ok) { - const body = await resp.json().catch(() => ({})); - throw new Error(body.detail || resp.statusText); - } - window.location.reload(); - } catch (err) { - setMigrationError((err as Error).message); - setMigrating(false); - } - } - return (
- +
{/* No site nav here on purpose. The public shell offers "Log in", @@ -97,55 +71,37 @@ function Wizard() {
-

{t(keys.host.setup.title)}

-

{t(keys.host.setup.subtitle)}

+

{t(keys.hosting.setup.title)}

+

{t(keys.hosting.setup.subtitle)}

- {t(keys.host.setup.connections.heading)} - {t(keys.host.setup.connections.description)} + {t(keys.hosting.setup.connections.heading)} + {t(keys.hosting.setup.connections.description)} - + - - - {t(keys.host.setup.migrations.heading)} - - {migration.isCurrent - ? t(keys.host.setup.migrations.current) - : t(keys.host.setup.migrations.behind)} - - - {!migration.isCurrent && ( - - - {migrationError &&

{migrationError}

} - - )} - - - - - {t(keys.host.setup.administrator.heading)} - {t(keys.host.setup.administrator.description)} - - - - - + {steps.map((step) => + step.action ? ( + + + {step.title} + {step.description && {step.description}} + + + + + + ) : null, + )} - {t(keys.host.setup.steps.heading)} + {t(keys.hosting.setup.steps.heading)}
    diff --git a/host/setup_payloads.py b/framework/hosting/simple_module_hosting/setup_wizard/payloads.py similarity index 56% rename from host/setup_payloads.py rename to framework/hosting/simple_module_hosting/setup_wizard/payloads.py index e8b5a685..2a9956c8 100644 --- a/host/setup_payloads.py +++ b/framework/hosting/simple_module_hosting/setup_wizard/payloads.py @@ -1,6 +1,6 @@ """What the setup wizard displays. -Split from ``routes_setup`` so that module holds the routes and the gating that +Split from ``routes`` so that module holds the routes and the gating that guards them, while this one holds the read-only shaping of what the page renders. They change for different reasons: a new dependency to probe touches this file, a new security condition touches that one. @@ -12,7 +12,10 @@ from fastapi import Request -CHECK_DATABASE = "host.database" +from simple_module_hosting._db_health import CHECK_DATABASE + +# A health-check *name*, not an import: the check exists only when the +# background_tasks module registered it, and is skipped otherwise. CHECK_REDIS = "background_tasks.redis" @@ -53,14 +56,12 @@ async def connection_status(request: Request) -> list[dict]: return [r for r in results if r is not None] -def steps_payload(registry, pending_ids: set[str], translate=None) -> list[dict]: - """Shape the registered steps for the wizard, resolving their catalog keys. +def _resolver(translate): + """``(key, fallback) -> str`` with the ``MenuRegistry`` fallback rule. - Steps are contributed by arbitrary modules, so their titles arrive as - backend data and cannot go through ``useT()`` in the page. Resolved here - instead, with the same fallback rule ``MenuRegistry`` uses: an unresolved - key keeps the English literal, because rendering ``users.administrator`` in - the UI would be worse than the text it replaced. + An unresolved key keeps the English literal, because rendering + ``users.setup.administrator.title`` in the UI would be worse than the text + it replaced. """ def render(key: str, fallback: str) -> str: @@ -69,12 +70,48 @@ def render(key: str, fallback: str) -> str: translated = translate(key) return fallback if translated == key else translated - return [ - { - "id": step.id, - "title": render(step.title_key, step.title), - "description": render(step.description_key, step.description), - "complete": step.id not in pending_ids, - } - for step in registry.all_steps - ] + return render + + +def _action_payload(action, render) -> dict: + return { + "submitLabel": render(action.submit_label_key, action.submit_label), + "fields": [ + { + "name": f.name, + "label": render(f.label_key, f.label), + "type": f.type, + "required": f.required, + "autocomplete": f.autocomplete, + "minLength": f.min_length, + } + for f in action.fields + ], + } + + +def steps_payload(registry, pending_ids: set[str], translate=None) -> list[dict]: + """Shape the registered steps for the wizard, resolving their catalog keys. + + Steps are contributed by arbitrary modules, so their titles arrive as + backend data and cannot go through ``useT()`` in the page. Resolved here + instead. + + A step's form is sent only while that step is pending: the action route + refuses a completed step anyway, and offering the form would only invite + a request that is bound to fail. + """ + render = _resolver(translate) + out: list[dict] = [] + for step in registry.all_steps: + pending = step.id in pending_ids + out.append( + { + "id": step.id, + "title": render(step.title_key, step.title), + "description": render(step.description_key, step.description), + "complete": not pending, + "action": _action_payload(step.action, render) if pending and step.action else None, + } + ) + return out diff --git a/framework/hosting/simple_module_hosting/setup_wizard/routes.py b/framework/hosting/simple_module_hosting/setup_wizard/routes.py new file mode 100644 index 00000000..82a7a187 --- /dev/null +++ b/framework/hosting/simple_module_hosting/setup_wizard/routes.py @@ -0,0 +1,135 @@ +"""The first-run setup wizard's HTTP surface. + +Served while any required :class:`SetupStep` is incomplete — see +``simple_module_hosting.setup_gate``. Unauthenticated by necessity: it exists +precisely when no account exists yet. + +Two gates, deliberately of different widths: + +* The page and the connection probe answer while *any* required step is + incomplete ("setup mode"), and 404 the moment none is. The middleware only + *redirects* other paths here — it exempts ``/setup`` itself — so these + handlers are the only thing that makes the wizard disappear on a configured + install. +* ``POST /setup/steps/`` additionally requires **that step** to be + incomplete. "Setup mode" is not a safe gate for an action: the host always + registers ``host.migrations``, so a live install whose schema falls behind + head — code deployed before the migration job ran — re-enters setup mode + with its administrators intact. Gated on the weaker condition, the + administrator step's action would let an anonymous request mint a fresh + superuser there. The step-level check answers 409. + +Every mutation carries the session-bound CSRF token (``RequiresCsrf``): the +page hands it out as the ``csrfToken`` prop and echoes it as ``X-CSRF-Token``. +""" + +from __future__ import annotations + +import json +import logging + +from fastapi import APIRouter, Depends, HTTPException, Request +from simple_module_inertia import InertiaResponse + +from simple_module_hosting.csrf import RequiresCsrf, get_csrf_token +from simple_module_hosting.i18n_deps import TranslatorDep +from simple_module_hosting.inertia_deps import InertiaDep +from simple_module_hosting.setup_wizard.payloads import connection_status, steps_payload + +logger = logging.getLogger(__name__) + +#: The Inertia page — ``Setup`` is the name the wizard's pages directory is +#: registered under in ``modules.generated.ts``; see ``setup_wizard.PAGES_NAME``. +_PAGE_WIZARD = "Setup/Wizard" + + +def _registry(request: Request): + return getattr(request.app.state.sm, "setup_registry", None) + + +async def _require_setup_mode(request: Request) -> None: + """404 unless the install still has incomplete required setup steps.""" + registry = _registry(request) + if not registry or not await registry.incomplete(request.app): + raise HTTPException(status_code=404) + + +# Setup mode first, CSRF second: on a configured install every /setup route +# answers 404 — the wizard does not exist there — rather than a 403 that +# advertises a live endpoint behind a missing token. +router = APIRouter( + prefix="/setup", + tags=["setup"], + dependencies=[Depends(_require_setup_mode), Depends(RequiresCsrf())], +) + + +@router.get("", response_model=None) +@router.get("/", response_model=None) +async def setup_index( + request: Request, inertia: InertiaDep, translator: TranslatorDep +) -> InertiaResponse: + """The wizard itself: connection status, then every registered step.""" + registry = _registry(request) + # incomplete_all, not incomplete: the latter only ever walks the *required* + # steps, so an optional one would render with a checkmark whatever its + # predicate says. + pending = {s.id for s in await registry.incomplete_all(request.app)} + + response = await inertia.render( + _PAGE_WIZARD, + { + "checks": await connection_status(request), + "steps": steps_payload(registry, pending, translator.t), + "csrfToken": get_csrf_token(request), + }, + ) + # The document embeds the session's CSRF token and connection diagnostics; + # the Inertia payload is already no-store via InertiaCache, the HTML is not. + response.headers["Cache-Control"] = "no-store" + return response + + +@router.post("/test-connections") +async def test_connections(request: Request) -> dict: + """Re-run the connection checks without reloading the page.""" + return {"checks": await connection_status(request)} + + +async def _read_form(request: Request) -> dict: + """The submitted form as a dict; an empty body is an empty form.""" + raw = await request.body() + if not raw.strip(): + return {} + try: + data = json.loads(raw) + except ValueError as exc: + raise HTTPException(status_code=422, detail="Request body must be JSON.") from exc + if not isinstance(data, dict): + raise HTTPException(status_code=422, detail="Request body must be a JSON object.") + return data + + +@router.post("/steps/{step_id}") +async def run_step_action(step_id: str, request: Request) -> dict: + """Complete one step through the action its module registered. + + Order matters: the wizard must be open at all (the router's 404, the same + answer as every other /setup route on a configured install), the CSRF + token must match (the router's 403), the step must + exist and offer an action (404), and the step itself must still be pending + (409). Only then is the module's handler called — and it remains + responsible for re-checking inside its own transaction, since two requests + can pass this check together. + """ + registry = _registry(request) + step = registry.get(step_id) + if step is None or step.action is None: + raise HTTPException(status_code=404) + if not await registry.is_pending(request.app, step): + raise HTTPException(status_code=409, detail="This setup step is already complete.") + + data = await _read_form(request) + result = await step.action.handler(request, data) + logger.info("Setup: ran the action for step %s", step_id) + return {"step": step_id, "result": result or {}} diff --git a/framework/hosting/tests/test_lifespan_deferred_startup.py b/framework/hosting/tests/test_lifespan_deferred_startup.py new file mode 100644 index 00000000..898a066f --- /dev/null +++ b/framework/hosting/tests/test_lifespan_deferred_startup.py @@ -0,0 +1,94 @@ +"""A first-run install boots behind head; failing on_startup hooks are deferred.""" + +from __future__ import annotations + +import asyncio +from types import SimpleNamespace +from unittest.mock import AsyncMock, patch + +import pytest +from fastapi import FastAPI +from simple_module_hosting._lifespan import build_lifespan, run_deferred_startup + + +def _module(name: str, *, fail_first: bool) -> SimpleNamespace: + calls = {"n": 0} + + async def on_startup(app): + calls["n"] += 1 + if fail_first and calls["n"] == 1: + raise RuntimeError("no such table") + + return SimpleNamespace( + meta=SimpleNamespace(name=name), + on_startup=on_startup, + on_shutdown=AsyncMock(), + calls=calls, + ) + + +def _app() -> FastAPI: + app = FastAPI() + engine = SimpleNamespace(dispose=AsyncMock()) + app.state.sm = SimpleNamespace(db=SimpleNamespace(engine=engine)) + return app + + +async def _boot(app, modules, *, is_current: bool, first_run: bool): + status = {"is_current": is_current, "pending_count": 0 if is_current else 1} + with ( + patch("simple_module_hosting._lifespan.migration_status", AsyncMock(return_value=status)), + patch("simple_module_hosting._lifespan._is_first_run", AsyncMock(return_value=first_run)), + patch("simple_module_hosting._lifespan.hydrate_settings_from_db", AsyncMock()), + ): + async with build_lifespan(modules)(app): + pass + + +async def test_failing_hook_is_deferred_then_replayed_on_unmigrated_first_run(): + app, good, bad = _app(), _module("good", fail_first=False), _module("bad", fail_first=True) + seen = {} + + async def capture(app_): + seen["deferred"] = [m.meta.name for m in app_.state.deferred_startup] + + with patch("simple_module_hosting._lifespan.hydrate_settings_from_db", AsyncMock()): + status = {"is_current": False, "pending_count": 1} + with ( + patch( + "simple_module_hosting._lifespan.migration_status", AsyncMock(return_value=status) + ), + patch("simple_module_hosting._lifespan._is_first_run", AsyncMock(return_value=True)), + ): + async with build_lifespan([good, bad])(app): + await capture(app) + await run_deferred_startup(app) + + assert seen["deferred"] == ["bad"] + assert good.calls["n"] == 1 + assert bad.calls["n"] == 2 + assert app.state.deferred_startup == [] + + +async def test_failing_hook_still_aborts_boot_when_schema_is_current(): + app = _app() + with pytest.raises(RuntimeError, match="no such table"): + await _boot(app, [_module("bad", fail_first=True)], is_current=True, first_run=False) + + +async def test_replay_survives_a_failing_hook_and_runs_once(): + app, bad = _app(), _module("bad", fail_first=False) + + async def boom(app_): + bad.calls["n"] += 1 + raise RuntimeError("still broken") + + bad.on_startup = boom + ok = _module("ok", fail_first=False) + app.state.deferred_startup = [bad, ok] + with patch("simple_module_hosting._lifespan.hydrate_settings_from_db", AsyncMock()): + await asyncio.gather(run_deferred_startup(app), run_deferred_startup(app)) + + assert bad.calls["n"] == 1 + assert ok.calls["n"] == 1 + assert app.state.deferred_startup == [] diff --git a/framework/hosting/tests/test_setup_migrate_error_redaction.py b/framework/hosting/tests/test_setup_migrate_error_redaction.py new file mode 100644 index 00000000..bb0b3c46 --- /dev/null +++ b/framework/hosting/tests/test_setup_migrate_error_redaction.py @@ -0,0 +1,55 @@ +"""The anonymous migrations action must not echo the failure to the client.""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest +from alembic import command +from fastapi import HTTPException +from simple_module_hosting.setup_wizard.migrate import apply_migrations + +_SECRET = "postgresql://admin:hunter2@db.internal/prod" + + +async def test_migration_failure_detail_is_redacted(monkeypatch, caplog) -> None: + def _boom(*_args, **_kwargs) -> None: + raise RuntimeError(f"could not connect to {_SECRET}") + + monkeypatch.setattr(command, "upgrade", _boom) + request = SimpleNamespace(state=SimpleNamespace(correlation_id="cid-123")) + + with pytest.raises(HTTPException) as info: + await apply_migrations(request, {}) + + assert info.value.status_code == 500 + assert _SECRET not in str(info.value.detail) + assert "cid-123" in str(info.value.detail) + # The operator still gets the real error, in the log. + assert _SECRET in caplog.text + + +async def test_in_process_migration_keeps_app_logging(monkeypatch) -> None: + """alembic's fileConfig must not leave the app's root logging replaced.""" + import logging + + root = logging.getLogger() + sentinel = logging.NullHandler() + root.addHandler(sentinel) + level = root.level + + def _clobber(*_args, **_kwargs) -> None: + # What env.py's fileConfig does to the root logger. + root.handlers[:] = [logging.StreamHandler()] + root.setLevel(logging.ERROR) + raise RuntimeError("stop after clobbering") + + monkeypatch.setattr(command, "upgrade", _clobber) + request = SimpleNamespace(state=SimpleNamespace(correlation_id="")) + try: + with pytest.raises(HTTPException): + await apply_migrations(request, {}) + assert sentinel in root.handlers + assert root.level == level + finally: + root.removeHandler(sentinel) diff --git a/framework/hosting/tests/test_setup_password_policy.py b/framework/hosting/tests/test_setup_password_policy.py deleted file mode 100644 index b8c7d22f..00000000 --- a/framework/hosting/tests/test_setup_password_policy.py +++ /dev/null @@ -1,69 +0,0 @@ -"""The setup wizard's admin route must enforce the real password policy. - -Found in browser QA: ``" "`` is eight characters, so a raw ``min_length=8`` -accepted it — and ``/setup/administrator`` is unauthenticated, so an anonymous -caller could create the install's first superuser with a whitespace-only -password. The route reimplemented a subset of ``UserManager.validate_password`` -rather than calling it, which is how the two drifted apart; it delegates now. -""" - -from __future__ import annotations - -import httpx -import pytest - -pytestmark = pytest.mark.anyio - - -def _mount(app): - from host.routes_setup import router as setup_router - - app.include_router(setup_router) - return app - - -async def _post(app, password: str, email: str = "root@example.com") -> httpx.Response: - async with httpx.AsyncClient( - transport=httpx.ASGITransport(app=_mount(app)), base_url="http://testserver" - ) as client: - # Accept: application/json is what the wizard's fetch sends. Without - # it the host renders an Inertia error *page* for a 4xx and the - # reason never reaches the operator. - return await client.post( - "/setup/administrator", - json={"email": email, "password": password}, - headers={"Accept": "application/json"}, - ) - - -@pytest.mark.parametrize( - "password,why", - [ - (" ", "whitespace-only, exactly eight characters"), - (" a ", "one real character padded to eight"), - ("short", "under the minimum"), - ("", "empty"), - ("12345678", "all digits — the policy rejects these"), - ], -) -async def test_weak_passwords_are_refused(setup_pending_app, password: str, why: str) -> None: - resp = await _post(setup_pending_app, password) - - assert resp.status_code == 422, f"accepted a password that is {why}: {resp.text[:120]}" - - -async def test_the_refusal_says_why(setup_pending_app) -> None: - """The operator has to be able to act on it — this route's 422 is rendered - straight into the wizard's error line.""" - resp = await _post(setup_pending_app, " ") - - detail = resp.json().get("detail") - assert isinstance(detail, str) and detail, f"unusable error body: {resp.text[:200]}" - assert "8" in detail or "characters" in detail.lower() - - -async def test_a_strong_password_is_accepted(setup_pending_app) -> None: - resp = await _post(setup_pending_app, "QaSetupPass1!") - - assert resp.status_code == 200, resp.text - assert resp.json()["created"] is True diff --git a/framework/hosting/tests/test_setup_routes.py b/framework/hosting/tests/test_setup_routes.py index 7a3240b5..d3714a66 100644 --- a/framework/hosting/tests/test_setup_routes.py +++ b/framework/hosting/tests/test_setup_routes.py @@ -1,137 +1,135 @@ -"""The /setup wizard's HTTP surface. +"""The /setup wizard's HTTP surface, as ``create_app`` mounts it for every host. ``test_setup_closes_after_completion`` is the security-relevant one. Every route here is unauthenticated by necessity — the wizard exists precisely when -no account exists — and one of them can run Alembic. What bounds that is the -routes refusing once an administrator exists, so it is asserted rather than -assumed. +no account exists — and one step action can run Alembic. What bounds that is +the routes refusing once setup is complete, so it is asserted rather than +assumed. The administrator action's own guarantees are pinned by the users +module's ``test_users_setup_wizard``. """ from __future__ import annotations -import httpx +import logging + import pytest +from simple_module_core.setup_steps import SetupAction, SetupRegistry, SetupStep +from simple_module_hosting.setup_gate import STEP_MIGRATIONS +from simple_module_hosting.setup_wizard import report_unactionable_steps +from simple_module_test.setup_wizard import post_step, wizard_client, wizard_headers pytestmark = pytest.mark.anyio +_STEP_ADMINISTRATOR = "users.administrator" -def _mount(app): - from host.routes_setup import router as setup_router - app.include_router(setup_router) - return app +async def test_create_app_mounts_the_wizard(setup_pending_app) -> None: + """No host-side router: a host that only calls create_app gets /setup.""" + async with wizard_client(setup_pending_app) as client: + root = await client.get("/", follow_redirects=False) + page = await client.get("/setup", follow_redirects=False) + assert root.status_code == 302 + assert root.headers["location"] == "/setup" + assert page.status_code == 200 + assert 'data-page="app"' in page.text + assert "Setup/Wizard" in page.text -async def _client(app) -> httpx.AsyncClient: - return httpx.AsyncClient(transport=httpx.ASGITransport(app=app), base_url="http://testserver") +async def test_wizard_lists_steps_with_forms_for_pending_ones(setup_pending_app) -> None: + async with wizard_client(setup_pending_app) as client: + resp = await client.get("/setup", headers={"X-Inertia": "true"}) -async def test_setup_reachable_without_auth(setup_pending_app) -> None: - async with await _client(_mount(setup_pending_app)) as client: - resp = await client.get("/setup", follow_redirects=False) + props = resp.json()["props"] + assert props["csrfToken"] + steps = {s["id"]: s for s in props["steps"]} - assert resp.status_code == 200 + admin = steps[_STEP_ADMINISTRATOR] + assert admin["complete"] is False + assert [f["name"] for f in admin["action"]["fields"]] == ["email", "password", "full_name"] + assert admin["action"]["submitLabel"] == "Create administrator" + + # The schema is at head, so its step is done and offers no form. + assert steps[STEP_MIGRATIONS]["complete"] is True + assert steps[STEP_MIGRATIONS]["action"] is None async def test_connection_checks_are_reported(setup_pending_app) -> None: """The wizard reports each dependency by name with a reason attached.""" - async with await _client(_mount(setup_pending_app)) as client: - resp = await client.post("/setup/test-connections") + async with wizard_client(setup_pending_app) as client: + headers = await wizard_headers(client) + resp = await client.post("/setup/test-connections", headers=headers) assert resp.status_code == 200 names = {c["name"] for c in resp.json()["checks"]} assert names == {"host.database", "background_tasks.redis"} -async def test_creating_an_admin_completes_setup(setup_pending_app) -> None: - app = _mount(setup_pending_app) - async with await _client(app) as client: - resp = await client.post( - "/setup/administrator", +async def test_mutations_require_the_csrf_token(setup_pending_app) -> None: + async with wizard_client(setup_pending_app) as client: + await client.get("/setup") # a session exists, but no token is echoed + probe = await client.post("/setup/test-connections") + action = await client.post( + f"/setup/steps/{_STEP_ADMINISTRATOR}", json={"email": "root@example.com", "password": "SetupPass1!"}, ) - assert resp.status_code == 200, resp.text - assert resp.json()["created"] is True - # The gate must now release for ordinary routes. - after = await client.get("/", follow_redirects=False) - - assert after.status_code != 302 + assert probe.status_code == 403 + assert action.status_code == 403 async def test_setup_closes_after_completion(app) -> None: - """Once an administrator exists every /setup route refuses. + """Once setup is complete every /setup route answers 404. - This is what bounds /setup/migrations — an unauthenticated endpoint that - can execute Alembic. It must be unreachable on a configured install. + This is what bounds the migrations action — an unauthenticated endpoint + that can execute Alembic. It must be unreachable on a configured install, + and with 404 rather than a CSRF 403 that advertises it. """ - async with await _client(_mount(app)) as client: + async with wizard_client(app) as client: for method, path in ( ("GET", "/setup"), ("POST", "/setup/test-connections"), - ("POST", "/setup/migrations"), - ("POST", "/setup/site-basics"), + ("POST", f"/setup/steps/{STEP_MIGRATIONS}"), + ("POST", f"/setup/steps/{_STEP_ADMINISTRATOR}"), ): resp = await client.request(method, path, json={}) assert resp.status_code == 404, f"{method} {path} answered {resp.status_code}" -async def test_administrator_route_closes_after_completion(app) -> None: - """The sharpest one: an open admin-creation form on a live install.""" - async with await _client(_mount(app)) as client: - resp = await client.post( - "/setup/administrator", - json={"email": "intruder@example.com", "password": "Whatever1!"}, - ) +async def test_unknown_step_is_404(setup_pending_app) -> None: + async with wizard_client(setup_pending_app) as client: + resp = await post_step(client, "nobody.registered.this") assert resp.status_code == 404 -async def test_administrator_route_stays_closed_when_only_migrations_pend(app, monkeypatch) -> None: - """A behind-head schema must not reopen admin creation. +async def test_a_completed_step_refuses_its_action(setup_pending_app) -> None: + """Setup mode is on (no admin), but the schema step is done: 409, never a + second Alembic run triggered anonymously.""" + async with wizard_client(setup_pending_app) as client: + resp = await post_step(client, STEP_MIGRATIONS) - ``create_app`` registers ``host.migrations`` for every install, so a live - deployment that ships code ahead of its migration job re-enters setup mode - with its administrators intact. Gating this route on "setup mode" rather - than on its own step would hand an anonymous request a fresh superuser - there — the routes must be gated per step, not per mode. - """ - behind = { - "current_revision": "abc123", - "head_revision": "def456", - "is_current": False, - "pending_count": 1, - } - app.state.migration = behind - - # The gate re-reads a behind-head verdict from the database rather than - # trusting the boot snapshot, so the stub has to keep saying "behind". - async def _still_behind(*_args, **_kwargs): - return dict(behind) - - monkeypatch.setattr( - "simple_module_hosting.migrations.migration_status", _still_behind, raising=True - ) + assert resp.status_code == 409 - async with await _client(_mount(app)) as client: - # The wizard itself opens — the schema really is behind and that is - # what it is for. - assert (await client.post("/setup/test-connections")).status_code == 200 - resp = await client.post( - "/setup/administrator", - json={"email": "intruder@example.com", "password": "Whatever1!"}, - ) +async def test_boot_reports_required_steps_without_an_action(caplog) -> None: + async def never(_app) -> bool: + return False - assert resp.status_code == 404 + async def handler(_request, _data): + return None + registry = SetupRegistry() + registry.add( + SetupStep(id="a.actionable", title="a", is_complete=never, action=SetupAction(handler)) + ) + registry.add(SetupStep(id="b.out_of_band", title="b", is_complete=never)) + registry.add(SetupStep(id="c.optional", title="c", is_complete=never, required=False)) -async def test_administrator_route_enforces_a_password_policy(setup_pending_app) -> None: - """``create_admin`` writes the hash directly, bypassing ``UserManager``.""" - async with await _client(_mount(setup_pending_app)) as client: - resp = await client.post( - "/setup/administrator", - json={"email": "root@example.com", "password": "short"}, - ) + with caplog.at_level(logging.WARNING, logger="simple_module_hosting.setup_wizard"): + report_unactionable_steps(registry) - assert resp.status_code == 422 + reported = " ".join(r.getMessage() for r in caplog.records) + assert "b.out_of_band" in reported + assert "a.actionable" not in reported + assert "c.optional" not in reported diff --git a/framework/hosting/tsconfig.json b/framework/hosting/tsconfig.json new file mode 100644 index 00000000..a08b2975 --- /dev/null +++ b/framework/hosting/tsconfig.json @@ -0,0 +1,12 @@ +{ + // Type-checks the pages simple_module_hosting ships itself (the /setup + // wizard). They are bundled by each host's Vite build from the installed + // wheel, so nothing else in the repo would compile them. + "extends": "@simple-module-py/tsconfig/base.json", + "compilerOptions": { + "paths": { + "@simple-module-py/ui/*": ["../../packages/ui/src/*"] + } + }, + "include": ["simple_module_hosting/**/*.ts", "simple_module_hosting/**/*.tsx"] +} diff --git a/framework/testing/simple_module_test/setup_wizard.py b/framework/testing/simple_module_test/setup_wizard.py new file mode 100644 index 00000000..eaa2cef3 --- /dev/null +++ b/framework/testing/simple_module_test/setup_wizard.py @@ -0,0 +1,37 @@ +"""Helpers for driving the ``/setup`` wizard from tests. + +Every wizard mutation carries the session-bound CSRF token, so a test has to +do what the page does: load the wizard (which mints the token into the +session cookie and hands it out as the ``csrfToken`` prop) and echo it back +as ``X-CSRF-Token``. +""" + +from __future__ import annotations + +import httpx +from simple_module_hosting.csrf import CSRF_HEADER + + +def wizard_client(app) -> httpx.AsyncClient: + """An anonymous client for *app*; it keeps the session cookie between calls.""" + return httpx.AsyncClient(transport=httpx.ASGITransport(app=app), base_url="http://testserver") + + +async def wizard_headers(client: httpx.AsyncClient) -> dict[str, str]: + """Load the wizard as Inertia would and return the headers its forms send. + + Raises ``AssertionError`` when the wizard is closed (404) — a test that + expected to post into it would otherwise fail with a confusing 403. + """ + resp = await client.get("/setup", headers={"X-Inertia": "true"}) + assert resp.status_code == 200, f"wizard not open: {resp.status_code}" + token = resp.json()["props"]["csrfToken"] + return {CSRF_HEADER: token, "Accept": "application/json"} + + +async def post_step( + client: httpx.AsyncClient, step_id: str, data: dict | None = None +) -> httpx.Response: + """Submit *step_id*'s wizard form, CSRF token included.""" + headers = await wizard_headers(client) + return await client.post(f"/setup/steps/{step_id}", json=data or {}, headers=headers) diff --git a/host/client_app/pages/Setup/AdministratorForm.tsx b/host/client_app/pages/Setup/AdministratorForm.tsx deleted file mode 100644 index c3cd65db..00000000 --- a/host/client_app/pages/Setup/AdministratorForm.tsx +++ /dev/null @@ -1,114 +0,0 @@ -import { keys, useT } from '@simple-module-py/i18n'; -import { Button } from '@simple-module-py/ui/components/ui/button'; -import { Input } from '@simple-module-py/ui/components/ui/input'; -import { Label } from '@simple-module-py/ui/components/ui/label'; -import { useState } from 'react'; - -/** Mirrors the server's `AdministratorIn.password` rule. */ -const MIN_PASSWORD_LENGTH = 8; - -/** - * Turn a FastAPI error body into one line of text. - * - * `detail` is a string for `HTTPException`, but an array of - * `{loc, msg, ...}` objects for a 422 — which is exactly what a short password - * or a malformed address produces here. Interpolating that array straight into - * an Error yields "[object Object]", i.e. the one form of the message that - * tells the operator nothing. - */ -function errorMessage(body: unknown, fallback: string): string { - const detail = (body as { detail?: unknown })?.detail; - if (typeof detail === 'string' && detail) return detail; - if (Array.isArray(detail)) { - const parts = detail - .map((item) => (typeof item === 'string' ? item : (item as { msg?: string })?.msg)) - .filter(Boolean); - if (parts.length > 0) return parts.join('; '); - } - return fallback; -} - -/** - * Creates the first administrator, which is what releases the setup gate. - * - * On success the page reloads rather than navigating: every /setup route - * starts 404ing the moment an admin exists, so a client-side transition would - * land on a route that has just disappeared. - */ -export function AdministratorForm() { - const { t } = useT(); - const [busy, setBusy] = useState(false); - const [error, setError] = useState(null); - const [done, setDone] = useState(false); - - async function submit(event: React.FormEvent) { - event.preventDefault(); - setBusy(true); - setError(null); - - const form = new FormData(event.currentTarget); - try { - const resp = await fetch('/setup/administrator', { - method: 'POST', - // Accept: application/json matters. The host renders an Inertia error - // *page* for a 4xx unless the caller prefers JSON, and a bare fetch() - // sends Accept: */* — so without this the response body is HTML, the - // json() below throws, and the operator sees "Unprocessable Entity" - // instead of the reason the request was refused. - headers: { 'Content-Type': 'application/json', Accept: 'application/json' }, - body: JSON.stringify({ - email: form.get('email'), - password: form.get('password'), - full_name: form.get('full_name') || null, - }), - }); - if (!resp.ok) { - const body = await resp.json().catch(() => ({})); - throw new Error(errorMessage(body, resp.statusText || String(resp.status))); - } - setDone(true); - window.location.href = '/'; - } catch (err) { - setError((err as Error).message); - } finally { - setBusy(false); - } - } - - return ( -
    -
    - - -
    - -
    - - -
    - -
    - - -
    - - {error &&

    {error}

    } - {done && ( -

    {t(keys.host.setup.administrator.created)}

    - )} - - -
    - ); -} diff --git a/host/locales/en.json b/host/locales/en.json index b5d3fb41..4f5fa800 100644 --- a/host/locales/en.json +++ b/host/locales/en.json @@ -81,42 +81,5 @@ "title": "You're offline", "description": "Changes may not be saved until your connection returns.", "restored": "Back online" - }, - "setup": { - "title": "Set up your install", - "subtitle": "A few things before this application is ready to use.", - "connections": { - "heading": "Connections", - "description": "Checking the services this install depends on.", - "retest": "Test again", - "testing": "Testing…" - }, - "migrations": { - "heading": "Database schema", - "behind": "The database is behind the version this code expects.", - "current": "The database schema is up to date.", - "apply": "Apply migrations", - "applying": "Applying…" - }, - "administrator": { - "heading": "Create an administrator", - "description": "This account can sign in and manage the install.", - "email": "Email", - "password": "Password", - "full_name": "Full name", - "submit": "Create administrator", - "submitting": "Creating…", - "created": "Administrator created. Reloading…" - }, - "steps": { - "heading": "Setup steps", - "complete": "Done", - "pending": "Pending", - "migrations": { - "title": "Apply database migrations", - "description": "Bring the database schema up to the version this code expects." - } - }, - "error": "Something went wrong. See the detail above." } } diff --git a/host/locales/es.json b/host/locales/es.json index a1c4ec54..7516f77d 100644 --- a/host/locales/es.json +++ b/host/locales/es.json @@ -81,42 +81,5 @@ "title": "Sin conexión", "description": "Es posible que los cambios no se guarden hasta que se restablezca la conexión.", "restored": "Conexión restablecida" - }, - "setup": { - "title": "Configura tu instalación", - "subtitle": "Unas cuantas cosas antes de que esta aplicación esté lista para usarse.", - "connections": { - "heading": "Conexiones", - "description": "Comprobando los servicios de los que depende esta instalación.", - "retest": "Probar de nuevo", - "testing": "Probando…" - }, - "migrations": { - "heading": "Esquema de la base de datos", - "behind": "La base de datos está por detrás de la versión que espera este código.", - "current": "El esquema de la base de datos está actualizado.", - "apply": "Aplicar migraciones", - "applying": "Aplicando…" - }, - "administrator": { - "heading": "Crea un administrador", - "description": "Esta cuenta puede iniciar sesión y administrar la instalación.", - "email": "Correo electrónico", - "password": "Contraseña", - "full_name": "Nombre completo", - "submit": "Crear administrador", - "submitting": "Creando…", - "created": "Administrador creado. Recargando…" - }, - "steps": { - "heading": "Pasos de configuración", - "complete": "Hecho", - "pending": "Pendiente", - "migrations": { - "title": "Aplicar las migraciones de la base de datos", - "description": "Actualiza el esquema de la base de datos a la versión que espera este código." - } - }, - "error": "Algo ha salido mal. Consulta el detalle de arriba." } } diff --git a/host/main.py b/host/main.py index de70b82e..0c699c66 100644 --- a/host/main.py +++ b/host/main.py @@ -19,7 +19,6 @@ from host.routes import router as host_router # noqa: E402 from host.routes_i18n import router as i18n_router # noqa: E402 from host.routes_legacy import router as legacy_router # noqa: E402 -from host.routes_setup import router as setup_router # noqa: E402 # merge_host_settings, not Settings(): log_level and the rest of the host # knobs live in the DB now. create_app falls back to this when passed no @@ -35,9 +34,6 @@ app = create_app(settings) app.include_router(host_router) app.include_router(i18n_router) -# Every /setup route 404s once setup completes, so this is inert on a -# configured install. -app.include_router(setup_router) # Mounted last: its catch-all {path:path} routes must not shadow a real # route that happens to share a legacy prefix. app.include_router(legacy_router) diff --git a/host/migrations/env.py b/host/migrations/env.py index 0d15e402..3ca4b201 100644 --- a/host/migrations/env.py +++ b/host/migrations/env.py @@ -29,7 +29,9 @@ # Set up Python logging from alembic.ini if config.config_file_name is not None: - fileConfig(config.config_file_name) + # Not the default disable_existing_loggers=True: the setup wizard runs this + # in-process, and that default would silence every app logger until restart. + fileConfig(config.config_file_name, disable_existing_loggers=False) # Build target metadata by importing every installed module's models. target_metadata = build_module_metadata() diff --git a/host/routes_setup.py b/host/routes_setup.py deleted file mode 100644 index 7ad3e271..00000000 --- a/host/routes_setup.py +++ /dev/null @@ -1,265 +0,0 @@ -"""The first-run setup wizard. - -Served while any required :class:`SetupStep` is incomplete — see -``simple_module_hosting.setup_gate``. Unauthenticated by necessity: it exists -precisely when no account exists yet. - -Every route here refuses once setup completes. That is what bounds the -exposure of ``/setup/migrations``, which can run Alembic: it is reachable only -before an administrator exists, and closes permanently the moment one does. -``_require_setup_mode`` is applied to each route rather than assumed from the -middleware, because the middleware only *redirects* other paths to here — it -deliberately exempts ``/setup`` itself, so these handlers are the only thing -standing between a configured install and an open admin-creation form. - -``/setup/administrator`` goes further and requires *its own* step to be -incomplete. "Some required step is incomplete" is not a safe gate for it: the -host always registers ``host.migrations``, so a live install whose schema -falls behind head — deploying code before the migration job runs — re-enters -setup mode with its administrators intact, and a route gated on the weaker -condition would let an anonymous request mint a fresh superuser there. -""" - -from __future__ import annotations - -import asyncio -import logging -from types import SimpleNamespace - -from fastapi import APIRouter, HTTPException, Request -from pydantic import EmailStr -from simple_module_hosting.i18n_deps import TranslatorDep -from simple_module_hosting.inertia_deps import InertiaDep -from simple_module_inertia import InertiaResponse -from sqlmodel import SQLModel - -from host.setup_payloads import connection_status, steps_payload - -logger = logging.getLogger(__name__) - -router = APIRouter(prefix="/setup", tags=["setup"]) - -_STEP_ADMINISTRATOR = "users.administrator" - - -def _resolve_password_policy(): - """Return an async ``(password, email) -> None`` that raises on a weak one. - - Wraps the users module's own ``UserManager.validate_password`` so the rule - has exactly one definition. Raises ImportError when no local-accounts - provider is installed, which the caller turns into a 400. - """ - from fastapi_users import exceptions as fu_exceptions - from users.manager import UserManager - - async def validate(password: str, email: str) -> None: - try: - await UserManager.validate_password(UserManager, password, SimpleNamespace(email=email)) - except fu_exceptions.InvalidPasswordException as exc: - raise HTTPException(status_code=422, detail=exc.reason) from exc - - return validate - - -async def _pending_step_ids(request: Request) -> set[str]: - """Ids of the required setup steps that are still incomplete.""" - registry = getattr(request.app.state.sm, "setup_registry", None) - if not registry: - return set() - return {s.id for s in await registry.incomplete(request.app)} - - -async def _require_setup_mode(request: Request) -> None: - """404 unless the install still has incomplete required setup steps.""" - if not await _pending_step_ids(request): - raise HTTPException(status_code=404) - - -async def _require_pending_step(request: Request, step_id: str) -> None: - """404 unless *step_id* specifically is still incomplete. - - The narrow gate, for routes whose effect only makes sense while that one - step is outstanding — see the module docstring on why "setup mode" alone - is too broad for admin creation. - """ - if step_id not in await _pending_step_ids(request): - raise HTTPException(status_code=404) - - -@router.get("", response_model=None) -@router.get("/", response_model=None) -async def setup_index( - request: Request, inertia: InertiaDep, translator: TranslatorDep -) -> InertiaResponse: - """The wizard itself: connection status, migrations, remaining steps.""" - await _require_setup_mode(request) - - registry = request.app.state.sm.setup_registry - # incomplete_all, not incomplete: the latter only ever walks the *required* - # steps, so an optional one would render with a checkmark whatever its - # predicate says. - pending = {s.id for s in await registry.incomplete_all(request.app)} - migration = getattr(request.app.state, "migration", None) or {} - - return await inertia.render( - "Setup/Wizard", - { - "checks": await connection_status(request), - "steps": steps_payload(registry, pending, translator.t), - "migration": { - "current": migration.get("current_revision"), - "head": migration.get("head_revision"), - "isCurrent": bool(migration.get("is_current", True)), - }, - }, - ) - - -@router.post("/test-connections") -async def test_connections(request: Request) -> dict: - """Re-run the connection checks without reloading the page.""" - await _require_setup_mode(request) - return {"checks": await connection_status(request)} - - -class AdministratorIn(SQLModel): - email: EmailStr - password: str - full_name: str | None = None - - -@router.post("/administrator") -async def create_administrator(request: Request, payload: AdministratorIn) -> dict: - """Create the first administrator, which is what releases the gate.""" - await _require_pending_step(request, _STEP_ADMINISTRATOR) - - # Imported here, not at module scope: the host must not hard-depend on the - # users module being installed. An install with an external identity - # provider never reaches this route, because it registers no setup step. - try: - from users.bootstrap import create_admin - - validate_password = _resolve_password_policy() - except ImportError as exc: # pragma: no cover - configuration error - raise HTTPException( - status_code=400, - detail="No local accounts provider is installed.", - ) from exc - - # Delegated, never reimplemented. create_admin writes the hash directly and - # never goes through the manager, so this route is the only thing standing - # between an anonymous caller and a weak password on the first superuser — - # and a local copy of "at least 8 characters" is exactly how it drifted out - # of step with the real policy (which also rejects all-digit passwords and - # ones containing the address). - await validate_password(payload.password, payload.email) - - async with request.app.state.sm.db.session_factory() as session: - result = await create_admin( - session, - email=payload.email, - password=payload.password, - full_name=payload.full_name, - ) - await session.commit() - - logger.info("Setup: administrator created (%s)", payload.email) - return {"created": result.created, "email": payload.email} - - -@router.post("/migrations") -async def apply_migrations(request: Request) -> dict: - """Run ``alembic upgrade head``. - - Reachable only while setup is incomplete (``_require_setup_mode``), which - is what bounds an endpoint that can execute migrations over HTTP. An - unmigrated database otherwise means dropping the operator to a shell — the - sharpest edge in the whole onboarding path. - """ - await _require_setup_mode(request) - - from alembic import command - from alembic.config import Config as AlembicConfig - from simple_module_hosting.migrations import default_alembic_ini - - # Resolved through the hosting helper rather than hardcoded: this runs - # inside a request, and a literal "host/alembic.ini" is only correct while - # the process cwd happens to be the project root. - ini_path = default_alembic_ini() - - def _upgrade() -> None: - # "heads", not "head": each module's first migration sets its own - # branch_labels, so the history legitimately has several heads and - # "head" raises CommandError("Multiple head revisions are present"). - # This is what `make migrate` runs. - command.upgrade(AlembicConfig(ini_path), "heads") - - try: - await asyncio.to_thread(_upgrade) - except Exception as exc: - logger.exception("Setup: migration run failed") - raise HTTPException(status_code=500, detail=str(exc)) from exc - - from simple_module_hosting.migrations import migration_status - - request.app.state.migration = await migration_status(request.app.state.sm.db.engine) - logger.info("Setup: migrations applied") - return {"migration": request.app.state.migration} - - -class SiteBasicsIn(SQLModel): - """The host settings the wizard may set. - - Only fields ``HostSettings`` actually declares belong here. ``site_name`` - used to be accepted, filtered back out just before the write, and then - echoed in ``saved`` — so the wizard reported persisting a value that never - reached the database. Branding owns the site name; it is not a host - setting. - """ - - i18n_default_locale: str | None = None - - -@router.post("/site-basics") -async def save_site_basics(request: Request, payload: SiteBasicsIn) -> dict: - """Persist the optional host settings collected by the wizard.""" - await _require_setup_mode(request) - - changes = {k: v for k, v in payload.model_dump().items() if v is not None} - if not changes: - return {"saved": {}} - - # importlib, not a static import: the host must not hard-depend on the - # settings module, and SM009 forbids naming a plugin package from - # framework code. The same reasoning applies here at the host layer. - import importlib - - service_cls = importlib.import_module("settings.service").SettingService - store_cls = importlib.import_module("settings.store").SettingsStore - apply_changes = importlib.import_module("settings.reload").apply_changes_and_reload - - async with request.app.state.sm.db.session_factory() as session: - store = store_cls(service_cls(session)) - await apply_changes_and_reload_safe(request, apply_changes, store, changes) - await session.commit() - - return {"saved": changes} - - -async def apply_changes_and_reload_safe(request: Request, apply_changes, store, changes: dict): - """Apply host settings changes, ignoring fields this build doesn't declare. - - A wizard shipped ahead of a module that declares a field should not 500 — - it should save what it can. - """ - try: - return await apply_changes( - request.app, - request.app.state.sm.event_bus, - store, - package="host", - changes=changes, - ) - except KeyError as exc: - logger.warning("Setup: skipping unknown host setting(s): %s", exc) - return None diff --git a/modules/users/tests/test_users_setup_lock.py b/modules/users/tests/test_users_setup_lock.py new file mode 100644 index 00000000..b9020ed8 --- /dev/null +++ b/modules/users/tests/test_users_setup_lock.py @@ -0,0 +1,63 @@ +"""The database lock behind the first-admin wizard action, across connections. + +``test_users_setup_wizard`` drives concurrency through one app, where the +in-process lock already serializes requests (and an in-memory SQLite engine +shares one connection anyway). Two workers share neither, so the database lock +is the real guarantee — exercised here on two real connections. +""" + +from __future__ import annotations + +import os + +import pytest +import sqlalchemy as sa +from simple_module_db import init_db +from sqlalchemy.exc import OperationalError +from users.models import User +from users.setup_action import _lock_admin_creation + +pytestmark = pytest.mark.anyio + + +async def test_sqlite_lock_holds_off_a_second_writer(tmp_path) -> None: + state = init_db(f"sqlite+aiosqlite:///{tmp_path / 'lock.db'}", sqlite_busy_timeout_ms=100) + try: + async with state.engine.begin() as conn: + await conn.run_sync(lambda sync: User.__table__.create(sync)) + + async with state.session_factory() as first, state.session_factory() as second: + await _lock_admin_creation(first) + + with pytest.raises(OperationalError, match="locked"): + await _lock_admin_creation(second) + await second.rollback() + + # Released at commit: the next worker gets through and re-checks. + await first.commit() + await _lock_admin_creation(second) + await second.rollback() + finally: + await state.engine.dispose() + + +@pytest.mark.skipif( + not os.environ.get("SM_TEST_DATABASE_URL", "").startswith("postgresql"), + reason="needs SM_TEST_DATABASE_URL pointing at Postgres", +) +async def test_postgres_advisory_lock_is_held_for_the_transaction() -> None: + state = init_db(os.environ["SM_TEST_DATABASE_URL"]) + try: + async with state.session_factory() as first, state.session_factory() as second: + await _lock_admin_creation(first) + probe = sa.text("SELECT pg_try_advisory_xact_lock(:key)") + from users.setup_action import _PG_LOCK_KEY + + assert await second.scalar(probe, {"key": _PG_LOCK_KEY}) is False + await second.rollback() + + await first.commit() + assert await second.scalar(probe, {"key": _PG_LOCK_KEY}) is True + await second.rollback() + finally: + await state.engine.dispose() diff --git a/modules/users/tests/test_users_setup_wizard.py b/modules/users/tests/test_users_setup_wizard.py new file mode 100644 index 00000000..27dee334 --- /dev/null +++ b/modules/users/tests/test_users_setup_wizard.py @@ -0,0 +1,187 @@ +"""The ``users.administrator`` wizard action: anonymous creation of the first admin. + +What is pinned here, in the order the action's docstring argues it: + +* it completes the step and releases the gate; +* it is gated on *its own step*, so an install that re-enters setup mode with + administrators intact (schema behind head) refuses it; +* it re-checks under a lock inside the inserting transaction, so concurrent + requests mint exactly one superuser; +* the password goes through the module's real policy. +""" + +from __future__ import annotations + +import asyncio +from types import SimpleNamespace + +import pytest +from fastapi import HTTPException +from simple_module_test.setup_wizard import post_step, wizard_client, wizard_headers +from sqlalchemy import func, select +from users.models import User +from users.setup import STEP_ADMINISTRATOR +from users.setup_action import create_first_administrator + +pytestmark = pytest.mark.anyio + +_BEHIND = { + "current_revision": "abc123", + "head_revision": "def456", + "is_current": False, + "pending_count": 1, +} + + +async def _superusers(app) -> list[str]: + async with app.state.sm.db.session_factory() as session: + rows = await session.scalars( + select(User.email).where(User.is_superuser.is_(True), User.is_active.is_(True)) + ) + return list(rows) + + +def _schema_behind(app, monkeypatch) -> None: + """Put the install back into setup mode the way a deploy-before-migrate does.""" + app.state.migration = dict(_BEHIND) + + # The gate re-reads a behind-head verdict from the database rather than + # trusting the boot snapshot, so the stub has to keep saying "behind". + async def _still_behind(*_args, **_kwargs): + return dict(_BEHIND) + + monkeypatch.setattr( + "simple_module_hosting.migrations.migration_status", _still_behind, raising=True + ) + + +async def test_creating_an_admin_completes_setup(setup_pending_app) -> None: + async with wizard_client(setup_pending_app) as client: + resp = await post_step( + client, + STEP_ADMINISTRATOR, + {"email": "root@example.com", "password": "SetupPass1!", "full_name": None}, + ) + assert resp.status_code == 200, resp.text + assert resp.json()["result"] == {"created": True, "email": "root@example.com"} + + # The gate must now release for ordinary routes, and the wizard close. + after = await client.get("/", follow_redirects=False) + wizard = await client.get("/setup") + + assert after.status_code != 302 + assert wizard.status_code == 404 + assert await _superusers(setup_pending_app) == ["root@example.com"] + + +async def test_refused_when_setup_mode_reopens_with_admins_intact(app, monkeypatch) -> None: + """A behind-head schema reopens the wizard, not admin creation. + + ``create_app`` registers ``host.migrations`` for every install, so a live + deployment that ships code ahead of its migration job re-enters setup mode + with its administrators intact. Gated on "setup mode" this action would + hand an anonymous request a fresh superuser there. + """ + _schema_behind(app, monkeypatch) + + async with wizard_client(app) as client: + resp = await post_step( + client, STEP_ADMINISTRATOR, {"email": "intruder@example.com", "password": "Whatever1!"} + ) + + assert resp.status_code == 409 + assert "intruder@example.com" not in await _superusers(app) + + +async def test_handler_rechecks_inside_its_transaction(app) -> None: + """The race loser: it passed the wizard's step check, then someone else + committed an admin. The in-transaction re-check must refuse it.""" + request = SimpleNamespace(app=app) + + with pytest.raises(HTTPException) as exc: + await create_first_administrator( + request, {"email": "late@example.com", "password": "LatePass1!"} + ) + + assert exc.value.status_code == 409 + assert "late@example.com" not in await _superusers(app) + + +async def test_concurrent_submissions_create_one_admin(setup_pending_app) -> None: + clients = [wizard_client(setup_pending_app) for _ in range(4)] + try: + # Every client loads the wizard first, as concurrent operators would — + # once the winner commits, the wizard is gone for anyone arriving later. + headers = [await wizard_headers(c) for c in clients] + responses = await asyncio.gather( + *( + c.post( + f"/setup/steps/{STEP_ADMINISTRATOR}", + json={"email": f"root{n}@example.com", "password": "RacePass1!"}, + headers=h, + ) + for n, (c, h) in enumerate(zip(clients, headers, strict=True)) + ) + ) + finally: + for c in clients: + await c.aclose() + + codes = sorted(r.status_code for r in responses) + assert codes.count(200) == 1, codes + # Losers are refused by the step gate or the in-transaction re-check (409), + # or — once the winner has closed the wizard entirely — by its absence (404). + assert all(code in (200, 404, 409) for code in codes), codes + async with setup_pending_app.state.sm.db.session_factory() as session: + count = await session.scalar( + select(func.count()).select_from(User).where(User.is_superuser.is_(True)) + ) + assert count == 1 + + +async def test_existing_non_admin_address_is_refused(setup_pending_app) -> None: + """create_admin leaves an existing account untouched; reporting success for + a step that is still pending would leave the operator stuck.""" + from users.bootstrap import create_standard_user + + async with setup_pending_app.state.sm.db.session_factory() as session: + await create_standard_user(session, email="user@example.com", password="UserPass1!") + + async with wizard_client(setup_pending_app) as client: + resp = await post_step( + client, STEP_ADMINISTRATOR, {"email": "user@example.com", "password": "Another1!"} + ) + + assert resp.status_code == 409 + assert await _superusers(setup_pending_app) == [] + + +@pytest.mark.parametrize( + "password,why", + [ + (" ", "whitespace-only, exactly eight characters"), + (" a ", "one real character padded to eight"), + ("short", "under the minimum"), + ("", "empty"), + ("12345678", "all digits — the policy rejects these"), + ], +) +async def test_weak_passwords_are_refused(setup_pending_app, password: str, why: str) -> None: + """``create_admin`` writes the hash directly, bypassing ``UserManager``.""" + async with wizard_client(setup_pending_app) as client: + resp = await post_step( + client, STEP_ADMINISTRATOR, {"email": "root@example.com", "password": password} + ) + + assert resp.status_code == 422, f"accepted a password that is {why}: {resp.text[:120]}" + # The wizard renders this straight into its error line. + detail = resp.json().get("detail") + assert isinstance(detail, str) and detail, f"unusable error body: {resp.text[:200]}" + + +async def test_malformed_payload_is_422(setup_pending_app) -> None: + async with wizard_client(setup_pending_app) as client: + resp = await post_step(client, STEP_ADMINISTRATOR, {"email": "not-an-address"}) + + assert resp.status_code == 422 + assert isinstance(resp.json()["detail"], str) diff --git a/modules/users/users/locales/en.json b/modules/users/users/locales/en.json index 0f92aece..662c83da 100644 --- a/modules/users/users/locales/en.json +++ b/modules/users/users/locales/en.json @@ -356,7 +356,11 @@ "setup": { "administrator": { "title": "Create an administrator", - "description": "An account that can sign in and manage this install." + "description": "An account that can sign in and manage this install.", + "email": "Email", + "password": "Password", + "full_name": "Full name", + "submit": "Create administrator" } }, "user_row": { diff --git a/modules/users/users/setup.py b/modules/users/users/setup.py index 8e8a6f04..7d2a8325 100644 --- a/modules/users/users/setup.py +++ b/modules/users/users/setup.py @@ -11,7 +11,7 @@ import logging -from simple_module_core.setup_steps import SetupStep +from simple_module_core.setup_steps import SetupAction, SetupField, SetupStep from sqlalchemy import func, select from users.models import User @@ -21,17 +21,72 @@ STEP_ADMINISTRATOR = "users.administrator" +_KEY = "users.setup.administrator" + +# Mirrors UserManager.validate_password's minimum so the browser refuses the +# obvious case before a round trip; the server enforces the real policy. +_MIN_PASSWORD_LENGTH = 8 + + +def _admin_action() -> SetupAction: + """The wizard form that completes the step — see ``users.setup_action``.""" + # Imported lazily: ``users.setup_action`` imports this module for + # ``session_has_administrator``, so a top-level import would be circular. + from users.setup_action import create_first_administrator + + return SetupAction( + handler=create_first_administrator, + fields=[ + SetupField( + name="email", + label="Email", + label_key=f"{_KEY}.email", + type="email", + autocomplete="email", + ), + SetupField( + name="password", + label="Password", + label_key=f"{_KEY}.password", + type="password", + autocomplete="new-password", + min_length=_MIN_PASSWORD_LENGTH, + ), + SetupField( + name="full_name", + label="Full name", + label_key=f"{_KEY}.full_name", + required=False, + autocomplete="name", + ), + ], + submit_label="Create administrator", + submit_label_key=f"{_KEY}.submit", + ) + + def build_admin_step() -> SetupStep: """The step that holds the app behind the wizard until an admin exists.""" return SetupStep( id=STEP_ADMINISTRATOR, title="Create an administrator", - title_key="users.setup.administrator.title", + title_key=f"{_KEY}.title", description="An account that can sign in and manage this install.", - description_key="users.setup.administrator.description", + description_key=f"{_KEY}.description", is_complete=has_administrator, order=30, + action=_admin_action(), + ) + + +async def session_has_administrator(session) -> bool: + """True when *session* sees at least one active superuser.""" + count = await session.scalar( + select(func.count()) + .select_from(User) + .where(User.is_superuser.is_(True), User.is_active.is_(True)) ) + return bool(count) async def has_administrator(app) -> bool: @@ -43,9 +98,4 @@ async def has_administrator(app) -> bool: """ session_factory = app.state.sm.db.session_factory async with session_factory() as session: - count = await session.scalar( - select(func.count()) - .select_from(User) - .where(User.is_superuser.is_(True), User.is_active.is_(True)) - ) - return bool(count) + return await session_has_administrator(session) diff --git a/modules/users/users/setup_action.py b/modules/users/users/setup_action.py new file mode 100644 index 00000000..c2dedabd --- /dev/null +++ b/modules/users/users/setup_action.py @@ -0,0 +1,129 @@ +"""The wizard action that completes ``users.administrator``: create the first admin. + +Anonymous by necessity — it runs before any account exists — so the bounds on +it are the whole of its security: + +1. The wizard calls it only while *this* step is pending (no active + superuser), never merely while "setup mode" is on. A live install whose + schema falls behind head re-enters setup mode with its administrators + intact; gated on the weaker condition this would mint a superuser there. +2. That check runs before the handler, outside any transaction, so two + concurrent requests can both pass it. The handler therefore takes a + database-level lock and re-checks **inside the transaction that inserts**: + the loser of the race sees the winner's committed admin and is refused. +3. The password goes through the users module's real policy + (``UserManager.validate_password``). ``create_admin`` writes the hash + directly and never consults it, and a local copy of "at least 8 + characters" is exactly how it once drifted from the policy. +""" + +from __future__ import annotations + +import asyncio +import logging +from types import SimpleNamespace + +import sqlalchemy as sa +from fastapi import HTTPException, Request +from fastapi_users import exceptions as fu_exceptions +from pydantic import EmailStr +from pydantic import ValidationError as PydanticValidationError +from sqlalchemy.ext.asyncio import AsyncSession +from sqlmodel import SQLModel + +from users.bootstrap import create_admin +from users.manager import UserManager +from users.models import User +from users.setup import session_has_administrator + +logger = logging.getLogger(__name__) + +# Arbitrary, fixed 64-bit key for pg_advisory_xact_lock. Only this action +# takes it, so any constant that no other module uses will do. +_PG_LOCK_KEY = 0x534D_5345_5455_5041 # "SMSETUPA" + + +class AdministratorIn(SQLModel): + email: EmailStr + password: str + full_name: str | None = None + + +async def _validate_password(password: str, email: str) -> None: + try: + await UserManager.validate_password(UserManager, password, SimpleNamespace(email=email)) + except fu_exceptions.InvalidPasswordException as exc: + raise HTTPException(status_code=422, detail=exc.reason) from exc + + +async def _lock_admin_creation(session: AsyncSession) -> None: + """Serialize admin creation across workers for the rest of this transaction. + + * Postgres: a transaction-scoped advisory lock, released at commit or + rollback. Taken before the re-check, so under READ COMMITTED the re-check + reads every admin committed by whoever held the lock before us. + * SQLite: a write statement that matches no rows. It still opens a write + transaction and takes the database's RESERVED lock, which a second + writer blocks on (``busy_timeout``) until this one commits; its own + re-check then runs against the committed admin. + """ + connection = await session.connection() + if connection.dialect.name == "postgresql": + await session.execute(sa.text("SELECT pg_advisory_xact_lock(:key)"), {"key": _PG_LOCK_KEY}) + return + table = User.__table__ + await session.execute( + sa.update(table).where(sa.false()).values(is_superuser=table.c.is_superuser) + ) + + +def _process_lock(app) -> asyncio.Lock: + """One in-process lock per app. + + The database lock is the real guarantee; this one keeps concurrent + requests in one worker from contending on it at all — and is what + serializes them on an in-memory SQLite engine, whose sessions share a + single connection and so cannot lock each other out. + """ + lock = getattr(app.state, "users_setup_lock", None) + if lock is None: + lock = asyncio.Lock() + app.state.users_setup_lock = lock + return lock + + +async def create_first_administrator(request: Request, data: dict) -> dict: + """Create the install's first administrator — the ``users.administrator`` action.""" + try: + payload = AdministratorIn.model_validate(data) + except PydanticValidationError as exc: + raise HTTPException( + status_code=422, detail="; ".join(e["msg"] for e in exc.errors()) + ) from exc + await _validate_password(payload.password, payload.email) + + async with _process_lock(request.app), request.app.state.sm.db.session_factory() as session: + await _lock_admin_creation(session) + if await session_has_administrator(session): + await session.rollback() + raise HTTPException(status_code=409, detail="An administrator already exists.") + # create_admin commits, which is what releases the database lock — + # after the user row and its admin role are both written. + result = await create_admin( + session, + email=payload.email, + password=payload.password, + full_name=payload.full_name, + ) + if not result.created: + # The address belongs to an account that is not an active + # superuser. create_admin(force=False) leaves it untouched, so + # nothing was granted — say so rather than reporting success + # for a step that is still pending. + await session.rollback() + raise HTTPException( + status_code=409, detail="An account with this email already exists." + ) + + logger.info("Setup: administrator created (%s)", payload.email) + return {"created": True, "email": payload.email} diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index 48745ab0..71542a32 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -491,31 +491,18 @@ export default { 'host.offline.description': '', 'host.offline.restored': '', 'host.offline.title': '', - 'host.setup.administrator.created': '', - 'host.setup.administrator.description': '', - 'host.setup.administrator.email': '', - 'host.setup.administrator.full_name': '', - 'host.setup.administrator.heading': '', - 'host.setup.administrator.password': '', - 'host.setup.administrator.submit': '', - 'host.setup.administrator.submitting': '', - 'host.setup.connections.description': '', - 'host.setup.connections.heading': '', - 'host.setup.connections.retest': '', - 'host.setup.connections.testing': '', - 'host.setup.error': '', - 'host.setup.migrations.apply': '', - 'host.setup.migrations.applying': '', - 'host.setup.migrations.behind': '', - 'host.setup.migrations.current': '', - 'host.setup.migrations.heading': '', - 'host.setup.steps.complete': '', - 'host.setup.steps.heading': '', - 'host.setup.steps.migrations.description': '', - 'host.setup.steps.migrations.title': '', - 'host.setup.steps.pending': '', - 'host.setup.subtitle': '', - 'host.setup.title': '', + 'hosting.setup.connections.description': '', + 'hosting.setup.connections.heading': '', + 'hosting.setup.connections.retest': '', + 'hosting.setup.connections.testing': '', + 'hosting.setup.done': '', + 'hosting.setup.migrations.apply': '', + 'hosting.setup.steps.heading': '', + 'hosting.setup.steps.migrations.description': '', + 'hosting.setup.steps.migrations.title': '', + 'hosting.setup.subtitle': '', + 'hosting.setup.title': '', + 'hosting.setup.working': '', 'keycloak.errors.callback_failed': '', 'keycloak.errors.invalid_state': '', 'keycloak.errors.token_validation_failed': '', @@ -1198,6 +1185,10 @@ export default { 'users.roles_tab.no_description': '', 'users.roles_tab.system_badge': '', 'users.setup.administrator.description': '', + 'users.setup.administrator.email': '', + 'users.setup.administrator.full_name': '', + 'users.setup.administrator.password': '', + 'users.setup.administrator.submit': '', 'users.setup.administrator.title': '', 'users.user_row.action_copy_reset': '', 'users.user_row.action_disable': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index 0b3e3bad..1c61d347 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -624,42 +624,29 @@ export const keys = { restored: 'host.offline.restored', title: 'host.offline.title', }, + }, + hosting: { setup: { - administrator: { - created: 'host.setup.administrator.created', - description: 'host.setup.administrator.description', - email: 'host.setup.administrator.email', - full_name: 'host.setup.administrator.full_name', - heading: 'host.setup.administrator.heading', - password: 'host.setup.administrator.password', - submit: 'host.setup.administrator.submit', - submitting: 'host.setup.administrator.submitting', - }, connections: { - description: 'host.setup.connections.description', - heading: 'host.setup.connections.heading', - retest: 'host.setup.connections.retest', - testing: 'host.setup.connections.testing', + description: 'hosting.setup.connections.description', + heading: 'hosting.setup.connections.heading', + retest: 'hosting.setup.connections.retest', + testing: 'hosting.setup.connections.testing', }, - error: 'host.setup.error', + done: 'hosting.setup.done', migrations: { - apply: 'host.setup.migrations.apply', - applying: 'host.setup.migrations.applying', - behind: 'host.setup.migrations.behind', - current: 'host.setup.migrations.current', - heading: 'host.setup.migrations.heading', + apply: 'hosting.setup.migrations.apply', }, steps: { - complete: 'host.setup.steps.complete', - heading: 'host.setup.steps.heading', + heading: 'hosting.setup.steps.heading', migrations: { - description: 'host.setup.steps.migrations.description', - title: 'host.setup.steps.migrations.title', + description: 'hosting.setup.steps.migrations.description', + title: 'hosting.setup.steps.migrations.title', }, - pending: 'host.setup.steps.pending', }, - subtitle: 'host.setup.subtitle', - title: 'host.setup.title', + subtitle: 'hosting.setup.subtitle', + title: 'hosting.setup.title', + working: 'hosting.setup.working', }, }, keycloak: { @@ -1505,6 +1492,10 @@ export const keys = { setup: { administrator: { description: 'users.setup.administrator.description', + email: 'users.setup.administrator.email', + full_name: 'users.setup.administrator.full_name', + password: 'users.setup.administrator.password', + submit: 'users.setup.administrator.submit', title: 'users.setup.administrator.title', }, }, diff --git a/scripts/check_untranslated_strings.mjs b/scripts/check_untranslated_strings.mjs index 72bc6c1c..0bfa7f7c 100644 --- a/scripts/check_untranslated_strings.mjs +++ b/scripts/check_untranslated_strings.mjs @@ -18,7 +18,13 @@ import { findUntranslated } from './lib/untranslated-strings.mjs'; const ROOT = cwd(); /** Everything whose rendered text a user can read. */ -const INCLUDE = ['modules/*/*/**/*.tsx', 'packages/ui/src/**/*.tsx', 'host/client_app/**/*.tsx']; +const INCLUDE = [ + 'modules/*/*/**/*.tsx', + 'packages/ui/src/**/*.tsx', + 'host/client_app/**/*.tsx', + // Pages the framework ships itself (the /setup wizard). + 'framework/hosting/simple_module_hosting/**/*.tsx', +]; /** * Vendored shadcn primitives are upstream code we re-sync, so their few