From 94db843939fd36ca8a672a475d438e8f8a15c13f Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 5 Oct 2026 14:01:20 +0200 Subject: [PATCH 1/8] feat(hosting): ship the /setup wizard with the framework (#351) SetupMiddleware redirected a fresh install to /setup, but the route and page lived only in this repo's unpublished host, so any other host got / -> /setup -> 404. - simple_module_hosting.setup_wizard: GET /setup, POST /setup/test-connections and POST /setup/steps/, mounted by create_app. Router-level gates: 404 outside setup mode, then the session CSRF token. - SetupStep.action (SetupAction/SetupField in core): a module hands the wizard a form spec and a handler; the wizard runs it only while that step is pending (409 otherwise). host.migrations uses it for alembic upgrade. - users ships the first-administrator action: password policy, then a DB lock (pg_advisory_xact_lock / SQLite write lock) plus an in-process lock, re-check and insert in one transaction, so concurrent POSTs mint one admin. - The wizard page ships from the wheel: gen-pages registers it as Setup/Wizard in modules.generated.ts/.assets.json/.css like a module's pages. Strings live in a new "hosting" catalog. - Required steps without an action are logged at boot; the gate logs once when it starts redirecting. - host/ no longer carries setup code; the UI-less /setup/site-basics is gone. Fixes #351 Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 15 + CLAUDE.md | 2 +- Makefile | 2 +- biome.json | 2 + docs/module-authoring.md | 48 +++- .../test_scaffolded_host_setup_wizard.py | 57 ++++ framework/core/simple_module_core/__init__.py | 4 +- .../core/simple_module_core/setup_steps.py | 64 +++++ .../simple_module_hosting/app_builder.py | 2 + .../hosting/simple_module_hosting/assets.py | 22 ++ .../simple_module_hosting/i18n_manifest.py | 6 + .../simple_module_hosting/locales/en.json | 24 ++ .../simple_module_hosting/locales/es.json | 24 ++ .../hosting/simple_module_hosting/manifest.py | 10 +- .../simple_module_hosting/setup_gate.py | 23 +- .../setup_wizard/__init__.py | 72 +++++ .../components}/ConnectionList.tsx | 14 +- .../setup_wizard/components/StepForm.tsx | 132 +++++++++ .../setup_wizard/migrate.py | 46 +++ .../setup_wizard/pages}/Wizard.tsx | 107 +++---- .../setup_wizard/payloads.py | 70 +++-- .../setup_wizard/routes.py | 131 +++++++++ .../tests/test_setup_password_policy.py | 69 ----- framework/hosting/tests/test_setup_routes.py | 162 ++++++----- framework/hosting/tsconfig.json | 12 + .../simple_module_test/setup_wizard.py | 37 +++ .../pages/Setup/AdministratorForm.tsx | 114 -------- host/locales/en.json | 37 --- host/locales/es.json | 37 --- host/main.py | 4 - host/routes_setup.py | 265 ------------------ modules/users/tests/test_users_setup_lock.py | 63 +++++ .../users/tests/test_users_setup_wizard.py | 187 ++++++++++++ modules/users/users/locales/en.json | 6 +- modules/users/users/setup.py | 51 +++- modules/users/users/setup_action.py | 138 +++++++++ packages/i18n/src/generated-resources.ts | 41 ++- packages/i18n/src/keys.generated.ts | 45 ++- scripts/check_untranslated_strings.mjs | 8 +- 39 files changed, 1381 insertions(+), 772 deletions(-) create mode 100644 framework/cli/tests/test_scaffolded_host_setup_wizard.py create mode 100644 framework/hosting/simple_module_hosting/locales/en.json create mode 100644 framework/hosting/simple_module_hosting/locales/es.json create mode 100644 framework/hosting/simple_module_hosting/setup_wizard/__init__.py rename {host/client_app/pages/Setup => framework/hosting/simple_module_hosting/setup_wizard/components}/ConnectionList.tsx (88%) create mode 100644 framework/hosting/simple_module_hosting/setup_wizard/components/StepForm.tsx create mode 100644 framework/hosting/simple_module_hosting/setup_wizard/migrate.py rename {host/client_app/pages/Setup => framework/hosting/simple_module_hosting/setup_wizard/pages}/Wizard.tsx (50%) rename host/setup_payloads.py => framework/hosting/simple_module_hosting/setup_wizard/payloads.py (57%) create mode 100644 framework/hosting/simple_module_hosting/setup_wizard/routes.py delete mode 100644 framework/hosting/tests/test_setup_password_policy.py create mode 100644 framework/hosting/tsconfig.json create mode 100644 framework/testing/simple_module_test/setup_wizard.py delete mode 100644 host/client_app/pages/Setup/AdministratorForm.tsx delete mode 100644 host/routes_setup.py create mode 100644 modules/users/tests/test_users_setup_lock.py create mode 100644 modules/users/tests/test_users_setup_wizard.py create mode 100644 modules/users/users/setup_action.py 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 e34086f9..447d25bd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -75,7 +75,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). **Middleware pipeline** (Starlette `add_middleware` is LIFO — last added runs first). Execution order on a request: `(ProxyHeaders, if SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → GZip → SecurityHeaders → Session → → Tenant (opt-in) → Locale → InertiaLayoutData → InertiaCache → Setup → Maintenance → CommitBeforeResponse → app`. `InertiaCache` answers for `InertiaLayoutData` merging per-user `auth`/`menus` into every payload: a response to an `X-Inertia` request is forced to `private, no-store` with its ETag dropped, and both representations of a URL gain `Vary: X-Inertia` — so no cache can store the JSON payload or hand it back for a page request. A module wanting its public page content cached should set `Cache-Control` and an ETag on the *document*; that path is left alone. `GZip` compresses any response over 500 bytes, including the `/static` mount — the built CSS is ~139 KB raw versus ~21 KB gzipped, and uncompressed assets dominated cold page load. `ProxyHeaders` (uvicorn's `ProxyHeadersMiddleware`) is installed only when `SM_TRUSTED_PROXY` is set, sitting outermost so the `X-Forwarded-*`-corrected scheme/client IP reach everything downstream (request logs). Inertia does not depend on it: the page url is rewritten to the root-relative form the protocol specifies (`_inertia_url.py`), so no scheme travels in the payload to disagree with the document's — the cross-scheme `pushState` `SecurityError` of GH #223 cannot recur on an install that never set the variable. When two modules add middleware at the same dependency tier, the module that sorts **later** wraps outermost. Use `depends_on` to express relative order — don't rely on names. `Maintenance` serves a 503 page to everyone but admins while `maintenance_mode` is set on `HostSettings`; it sits inside `InertiaCache` because its 503 is an Inertia payload produced by short-circuiting, and outside the cache guard that payload would ship storable. `Setup` runs just before it, for the same cache reason and because an install that was never set up has nothing meaningful to put into maintenance. diff --git a/Makefile b/Makefile index f10beff7..778adce8 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 b6776898..58a0f0d7 100644 --- a/docs/module-authoring.md +++ b/docs/module-authoring.md @@ -506,11 +506,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: @@ -518,20 +520,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` @@ -539,6 +564,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/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 fa1395b6..e1a8eae2 100644 --- a/framework/core/simple_module_core/__init__.py +++ b/framework/core/simple_module_core/__init__.py @@ -46,7 +46,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 @@ -89,6 +89,8 @@ "PublicRoute", "PublicRouteRegistry", "Services", + "SetupAction", + "SetupField", "SetupRegistry", "SetupStep", "TenantRole", 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/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index fc0fb7a5..67217ae9 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -45,6 +45,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__) @@ -265,6 +266,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 ebcdac82..738e90f7 100644 --- a/framework/hosting/simple_module_hosting/i18n_manifest.py +++ b/framework/hosting/simple_module_hosting/i18n_manifest.py @@ -48,6 +48,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..0f8a73be 100644 --- a/framework/hosting/simple_module_hosting/manifest.py +++ b/framework/hosting/simple_module_hosting/manifest.py @@ -26,10 +26,12 @@ from simple_module_hosting.assets import ( compute_module_assets, + framework_assets, render_assets_json, render_modules_css, ) from simple_module_hosting.page_globs import glob_patterns_for +from simple_module_hosting.setup_wizard import pages_dir as wizard_pages_dir logger = logging.getLogger(__name__) @@ -159,7 +161,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: wizard_pages_dir()} manifest_path = output_dir / "modules.manifest.json" manifest_payload = {name: path.as_posix() for name, path in pages_map.items()} @@ -186,7 +192,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..9644cd0d 100644 --- a/framework/hosting/simple_module_hosting/setup_gate.py +++ b/framework/hosting/simple_module_hosting/setup_gate.py @@ -87,18 +87,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 +118,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 +176,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..845050ca --- /dev/null +++ b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py @@ -0,0 +1,46 @@ +"""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 logging + +from fastapi import HTTPException, Request + +logger = logging.getLogger(__name__) + + +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. + 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 + + request.app.state.migration = await migration_status(request.app.state.sm.db.engine) + 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..5a15cc6a 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,46 @@ 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); - } - } + const actionable = steps.filter((step) => step.action !== null); return (
- +
{/* No site nav here on purpose. The public shell offers "Log in", @@ -97,55 +72,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)} - - - - - + {actionable.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 57% rename from host/setup_payloads.py rename to framework/hosting/simple_module_hosting/setup_wizard/payloads.py index e8b5a685..faa5cb7d 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. @@ -13,6 +13,8 @@ from fastapi import Request CHECK_DATABASE = "host.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 +55,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 +69,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..af40c185 --- /dev/null +++ b/framework/hosting/simple_module_hosting/setup_wizard/routes.py @@ -0,0 +1,131 @@ +"""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)} + + return await inertia.render( + _PAGE_WIZARD, + { + "checks": await connection_status(request), + "steps": steps_payload(registry, pending, translator.t), + "csrfToken": get_csrf_token(request), + }, + ) + + +@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_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 18c36e24..b86f37a5 100644 --- a/host/locales/en.json +++ b/host/locales/en.json @@ -79,42 +79,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 1769b498..71009ae2 100644 --- a/host/locales/es.json +++ b/host/locales/es.json @@ -79,42 +79,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/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..328aeec3 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,16 +21,61 @@ 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: the handler pulls in the user manager and fastapi-users, + # which registering a step at boot has no need for. + 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(), ) diff --git a/modules/users/users/setup_action.py b/modules/users/users/setup_action.py new file mode 100644 index 00000000..ebdd99a1 --- /dev/null +++ b/modules/users/users/setup_action.py @@ -0,0 +1,138 @@ +"""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 import func, select +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 + +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) + ) + + +async def _has_active_superuser(session: AsyncSession) -> bool: + count = await session.scalar( + select(func.count()) + .select_from(User) + .where(User.is_superuser.is_(True), User.is_active.is_(True)) + ) + return bool(count) + + +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 _has_active_superuser(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 1575203f..3ee58d05 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -489,31 +489,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': '', @@ -1195,6 +1182,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 b21567cf..8252127c 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -622,42 +622,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: { @@ -1502,6 +1489,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 From c24335070e52a721d7e1a8e9d7bf72599a92d460 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 5 Oct 2026 14:03:11 +0200 Subject: [PATCH 2/8] fix(setup): don't echo migration errors to the anonymous caller The migrations action is reachable without auth while the step is pending, and an alembic failure routinely carries the database URL, SQL or paths. Log the error with the correlation id; return a generic detail. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../setup_wizard/migrate.py | 11 +++++-- .../test_setup_migrate_error_redaction.py | 29 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 framework/hosting/tests/test_setup_migrate_error_redaction.py diff --git a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py index 845050ca..4c330268 100644 --- a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py +++ b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py @@ -38,8 +38,15 @@ def _upgrade() -> None: 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 + # 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) logger.info("Setup: migrations applied") 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..70214fb0 --- /dev/null +++ b/framework/hosting/tests/test_setup_migrate_error_redaction.py @@ -0,0 +1,29 @@ +"""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 From 44e1dae0d8e8bedd14676f760b83be5afba625e5 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 6 Oct 2026 19:29:53 +0200 Subject: [PATCH 3/8] refactor(optimize): drop duplicate constant, redundant filter and import in setup wizard Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/hosting/simple_module_hosting/manifest.py | 3 +-- .../simple_module_hosting/setup_wizard/pages/Wizard.tsx | 3 +-- .../hosting/simple_module_hosting/setup_wizard/payloads.py | 3 ++- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/framework/hosting/simple_module_hosting/manifest.py b/framework/hosting/simple_module_hosting/manifest.py index 0f8a73be..f77e92a5 100644 --- a/framework/hosting/simple_module_hosting/manifest.py +++ b/framework/hosting/simple_module_hosting/manifest.py @@ -31,7 +31,6 @@ render_modules_css, ) from simple_module_hosting.page_globs import glob_patterns_for -from simple_module_hosting.setup_wizard import pages_dir as wizard_pages_dir logger = logging.getLogger(__name__) @@ -165,7 +164,7 @@ def write_module_pages_manifest( # 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: wizard_pages_dir()} + 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()} diff --git a/framework/hosting/simple_module_hosting/setup_wizard/pages/Wizard.tsx b/framework/hosting/simple_module_hosting/setup_wizard/pages/Wizard.tsx index 5a15cc6a..cd29a26c 100644 --- a/framework/hosting/simple_module_hosting/setup_wizard/pages/Wizard.tsx +++ b/framework/hosting/simple_module_hosting/setup_wizard/pages/Wizard.tsx @@ -44,7 +44,6 @@ function Wizard() { const appName = branding?.appName ?? BRAND_DEFAULT_APP_NAME; const brandInitial = appName.trim().charAt(0).toUpperCase() || 'S'; - const actionable = steps.filter((step) => step.action !== null); return (
    @@ -86,7 +85,7 @@ function Wizard() { - {actionable.map((step) => + {steps.map((step) => step.action ? ( diff --git a/framework/hosting/simple_module_hosting/setup_wizard/payloads.py b/framework/hosting/simple_module_hosting/setup_wizard/payloads.py index faa5cb7d..2a9956c8 100644 --- a/framework/hosting/simple_module_hosting/setup_wizard/payloads.py +++ b/framework/hosting/simple_module_hosting/setup_wizard/payloads.py @@ -12,7 +12,8 @@ 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" From 0765406680f53f93805ae7bfa4a8a033abf63edb Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 6 Oct 2026 19:32:10 +0200 Subject: [PATCH 4/8] fix(setup): mark the wizard document no-store (embeds CSRF token) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../hosting/simple_module_hosting/setup_wizard/routes.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/framework/hosting/simple_module_hosting/setup_wizard/routes.py b/framework/hosting/simple_module_hosting/setup_wizard/routes.py index af40c185..82a7a187 100644 --- a/framework/hosting/simple_module_hosting/setup_wizard/routes.py +++ b/framework/hosting/simple_module_hosting/setup_wizard/routes.py @@ -76,7 +76,7 @@ async def setup_index( # predicate says. pending = {s.id for s in await registry.incomplete_all(request.app)} - return await inertia.render( + response = await inertia.render( _PAGE_WIZARD, { "checks": await connection_status(request), @@ -84,6 +84,10 @@ async def setup_index( "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") From 45e283bdc634454502745e7e20ee80b5fd50c1bb Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 6 Oct 2026 19:42:23 +0200 Subject: [PATCH 5/8] fix(setup): review fixes - keep app loggers alive after in-process migrations, share admin-exists query Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../templates/host/migrations/env.py | 4 +++- host/migrations/env.py | 4 +++- modules/users/users/setup.py | 21 ++++++++++++------- modules/users/users/setup_action.py | 13 ++---------- 4 files changed, 21 insertions(+), 21 deletions(-) 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 4c5a19f8..65f7d182 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/host/migrations/env.py b/host/migrations/env.py index 0617f470..97ca3bce 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/modules/users/users/setup.py b/modules/users/users/setup.py index 328aeec3..7d2a8325 100644 --- a/modules/users/users/setup.py +++ b/modules/users/users/setup.py @@ -30,8 +30,8 @@ def _admin_action() -> SetupAction: """The wizard form that completes the step — see ``users.setup_action``.""" - # Imported lazily: the handler pulls in the user manager and fastapi-users, - # which registering a step at boot has no need for. + # 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( @@ -79,6 +79,16 @@ def build_admin_step() -> SetupStep: ) +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: """True once at least one active superuser exists. @@ -88,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 index ebdd99a1..c2dedabd 100644 --- a/modules/users/users/setup_action.py +++ b/modules/users/users/setup_action.py @@ -28,13 +28,13 @@ from fastapi_users import exceptions as fu_exceptions from pydantic import EmailStr from pydantic import ValidationError as PydanticValidationError -from sqlalchemy import func, select 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__) @@ -77,15 +77,6 @@ async def _lock_admin_creation(session: AsyncSession) -> None: ) -async def _has_active_superuser(session: AsyncSession) -> bool: - count = await session.scalar( - select(func.count()) - .select_from(User) - .where(User.is_superuser.is_(True), User.is_active.is_(True)) - ) - return bool(count) - - def _process_lock(app) -> asyncio.Lock: """One in-process lock per app. @@ -113,7 +104,7 @@ async def create_first_administrator(request: Request, data: dict) -> dict: async with _process_lock(request.app), request.app.state.sm.db.session_factory() as session: await _lock_admin_creation(session) - if await _has_active_superuser(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 — From 52438ce3371b7cdfdc8ada79c5f723002a3ebfd8 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 6 Oct 2026 19:58:58 +0200 Subject: [PATCH 6/8] fix(setup): boot an unmigrated first-run install to the wizard QA found a fresh unmigrated database crashed the boot in Users/Permissions on_startup (no such table), so /setup was unreachable and the migrations step useless. A failing on_startup on a behind-head first run is now deferred and replayed after the wizard applies the migrations. Also add the missing sqlmodel import to the scaffolded alembic script template. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../templates/host/migrations/script.py.mako | 1 + .../simple_module_hosting/_lifespan.py | 33 +++++++- .../setup_wizard/migrate.py | 4 + .../tests/test_lifespan_deferred_startup.py | 75 +++++++++++++++++++ 4 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 framework/hosting/tests/test_lifespan_deferred_startup.py 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/hosting/simple_module_hosting/_lifespan.py b/framework/hosting/simple_module_hosting/_lifespan.py index cdbe7d07..2e3408b7 100644 --- a/framework/hosting/simple_module_hosting/_lifespan.py +++ b/framework/hosting/simple_module_hosting/_lifespan.py @@ -13,6 +13,7 @@ from __future__ import annotations +import logging from collections.abc import AsyncGenerator, Callable, Sequence from contextlib import asynccontextmanager @@ -21,6 +22,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 +101,17 @@ 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.""" + deferred = getattr(app.state, "deferred_startup", []) + if not deferred: + return + await hydrate_settings_from_db(app) + while deferred: + await deferred[0].on_startup(app) + deferred.pop(0) + + def build_lifespan(modules: Sequence) -> Callable: """Return the ``lifespan`` context manager for an app over *modules*.""" @@ -120,8 +134,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/setup_wizard/migrate.py b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py index 4c330268..0459e772 100644 --- a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py +++ b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py @@ -49,5 +49,9 @@ def _upgrade() -> None: 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/framework/hosting/tests/test_lifespan_deferred_startup.py b/framework/hosting/tests/test_lifespan_deferred_startup.py new file mode 100644 index 00000000..449db88f --- /dev/null +++ b/framework/hosting/tests/test_lifespan_deferred_startup.py @@ -0,0 +1,75 @@ +"""A first-run install boots behind head; failing on_startup hooks are deferred.""" + +from __future__ import annotations + +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) From c94b0057b8a59dabb459eb202a75bf18f04214b5 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 6 Oct 2026 20:04:36 +0200 Subject: [PATCH 7/8] fix(setup): replay deferred startup from the gate too, serialise migrations, keep app logging Review findings: any worker that observes the schema at head now finishes the deferred on_startup hooks (not only the one that ran the migrations); replay is locked and never raises after migrations committed; concurrent migration runs are serialised; in-process alembic no longer clobbers the app's logging. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../simple_module_hosting/_lifespan.py | 30 +++++++++++--- .../simple_module_hosting/setup_gate.py | 10 +++++ .../setup_wizard/migrate.py | 39 ++++++++++++++++++- .../tests/test_lifespan_deferred_startup.py | 19 +++++++++ .../test_setup_migrate_error_redaction.py | 26 +++++++++++++ 5 files changed, 116 insertions(+), 8 deletions(-) diff --git a/framework/hosting/simple_module_hosting/_lifespan.py b/framework/hosting/simple_module_hosting/_lifespan.py index 2e3408b7..8e623d11 100644 --- a/framework/hosting/simple_module_hosting/_lifespan.py +++ b/framework/hosting/simple_module_hosting/_lifespan.py @@ -13,6 +13,7 @@ from __future__ import annotations +import asyncio import logging from collections.abc import AsyncGenerator, Callable, Sequence from contextlib import asynccontextmanager @@ -102,14 +103,31 @@ async def _is_first_run(app: FastAPI) -> bool: async def run_deferred_startup(app: FastAPI) -> None: - """Replay the ``on_startup`` hooks that could not run on an unmigrated DB.""" - deferred = getattr(app.state, "deferred_startup", []) + """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 - await hydrate_settings_from_db(app) - while deferred: - await deferred[0].on_startup(app) - deferred.pop(0) + 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: diff --git a/framework/hosting/simple_module_hosting/setup_gate.py b/framework/hosting/simple_module_hosting/setup_gate.py index 9644cd0d..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"]) diff --git a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py index 0459e772..d3456eb4 100644 --- a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py +++ b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py @@ -9,12 +9,45 @@ from __future__ import annotations import asyncio +import contextlib import logging +from collections.abc import Iterator 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() -> Iterator[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.""" @@ -33,10 +66,12 @@ def _upgrade() -> None: # 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") + with _preserve_logging(): + command.upgrade(AlembicConfig(ini_path), "heads") try: - await asyncio.to_thread(_upgrade) + 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 diff --git a/framework/hosting/tests/test_lifespan_deferred_startup.py b/framework/hosting/tests/test_lifespan_deferred_startup.py index 449db88f..898a066f 100644 --- a/framework/hosting/tests/test_lifespan_deferred_startup.py +++ b/framework/hosting/tests/test_lifespan_deferred_startup.py @@ -2,6 +2,7 @@ from __future__ import annotations +import asyncio from types import SimpleNamespace from unittest.mock import AsyncMock, patch @@ -73,3 +74,21 @@ 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 index 70214fb0..bb0b3c46 100644 --- a/framework/hosting/tests/test_setup_migrate_error_redaction.py +++ b/framework/hosting/tests/test_setup_migrate_error_redaction.py @@ -27,3 +27,29 @@ def _boom(*_args, **_kwargs) -> None: 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) From fecaff4ee21979d6602bb6e7dcf81424e18fec4d Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 7 Oct 2026 10:02:30 +0200 Subject: [PATCH 8/8] fix: reconcile setup wizard with main (ty Generator annotation, app_builder line cap) Merging main brought #408's middleware wiring into app_builder.py (302 lines); move project-root resolution to _project_root.py. #409's new contextmanager used the Iterator annotation that ty 0.0.85 flags (see #411). Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/core/simple_module_core/dotenv.py | 2 +- .../simple_module_hosting/_project_root.py | 39 +++++++++++++++++++ .../simple_module_hosting/app_builder.py | 37 +----------------- .../setup_wizard/migrate.py | 4 +- 4 files changed, 43 insertions(+), 39 deletions(-) create mode 100644 framework/hosting/simple_module_hosting/_project_root.py 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/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 245d1630..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 ( @@ -58,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() diff --git a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py index d3456eb4..3b214b39 100644 --- a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py +++ b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py @@ -11,7 +11,7 @@ import asyncio import contextlib import logging -from collections.abc import Iterator +from collections.abc import Generator from fastapi import HTTPException, Request @@ -24,7 +24,7 @@ @contextlib.contextmanager -def _preserve_logging() -> Iterator[None]: +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