diff --git a/CLAUDE.md b/CLAUDE.md index 7304b48a..2c5b8f81 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -74,10 +74,10 @@ 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_health_checks` / `register_public_routes` / `register_csp_sources` → `register_exception_handlers` → `register_middleware` → `register_routes(api_router, view_router)` → async `on_startup` / `on_shutdown` (reverse order). `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_settings` → `register_menu_items` / `register_permissions` / `register_feature_flags` / `register_event_handlers` / `register_health_checks` / `register_public_routes` / `register_csp_sources` → `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). **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 → 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 and Inertia's absolute page url). 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. +`(ProxyHeaders, if SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → GZip → SecurityHeaders → Session → → Tenant (opt-in) → Locale → InertiaLayoutData → InertiaCache → 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 and Inertia's absolute page url). 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. **Database**: per-module `Base` via `create_module_base("")`. Every module owns its own `MetaData` (so Alembic autogenerate can attribute tables to a module), but all tables live in the host's single schema. `__tablename__` must be prefixed with the module name to avoid collisions (`orders_order`). Postgres and SQLite share the same layout. @@ -85,6 +85,8 @@ Standard mixins in `simple_module_db.mixins`: `AuditMixin`, `SoftDeleteMixin` (b **Migrations** live in `host/migrations/versions/` — not in module packages. `host/alembic/env.py` calls `build_module_metadata()` + `make_include_object()` so autogenerate covers every installed module and ignores host-owned tables. First migration of each module should set `branch_labels = ("",)` to enable per-module `downgrade @base`. +**Admin section**. Administrative screens live under `/admin/*`, register into `MenuSection.ADMIN_SIDEBAR`, and render in `AdminLayout` — all three together, not one of the three. `SidebarLayout` renders whichever menu its `menuKey` names, so a page left on `AuthenticatedLayout` after its menu item moved shows a sidebar that no longer contains it. `group=` sub-clusters *within* the admin sidebar (`Access`, `Appearance`, `System`); it is no longer used to carve an admin area out of the main sidebar. `/admin` itself is a host route (`host/routes.py`) that renders from the `adminSidebar` shared prop, so an installed module contributes a card without touching it. Old URLs 301 from `host/routes_legacy.py`. Only view URLs moved — `/api/*` is a separate contract and stays put. + **Inertia**. `inertia.render("/", ...)` maps to `modules///pages/.tsx`, where `` is the PascalCase of the module directory (`blog_posts` → `BlogPosts`). Host-level pages under `host/client_app/pages/` use a bare ``. `InertiaLayoutDataMiddleware` populates shared props (`auth`, `menus`, `i18n`); use `InertiaDep` from `simple_module_hosting.inertia_deps`. Mismatched keys fire `SM003` (orphan page) / `SM004` (phantom render). **CSRF defence**. Baseline protection comes from `SameSite=Lax` on the session cookie (Starlette default): browsers don't attach the cookie to cross-site POST/PUT/DELETE, so a forged form-submit from another origin is unauthenticated. Raw `fetch()` calls in page code don't need a token header by default. Modules wanting defence in depth opt into `simple_module_hosting.csrf` — `RequiresCsrf` as a router dependency plus `get_csrf_token(request)` exposed as a view prop; callers echo it as `X-CSRF-Token` on unsafe methods. diff --git a/README.md b/README.md index b8acc8d1..649c880a 100644 --- a/README.md +++ b/README.md @@ -45,7 +45,7 @@ make migrate make dev ``` -Hit `http://localhost:8000` — you land on the public page. `/users/login` is the email+password login, `/dashboard/` is the authenticated home, and `/dashboard/doctor` is the admin-only "smpy doctor" panel (static checks, migrations, dev server, modules). +Hit `http://localhost:8000` — you land on the public page. `/users/login` is the email+password login, `/dashboard/` is the authenticated home, and `/admin/doctor/` is the admin-only "smpy doctor" panel (static checks, migrations, dev server, modules). ## Create a new module @@ -109,7 +109,7 @@ docs/ ## Configuration -Local deployments only need one env var — everything else has sensible defaults and is managed in the admin UI at `/settings/modules`. +Local deployments only need one env var — everything else has sensible defaults and is managed in the admin UI at `/admin/settings/`. | Variable | Default | Required | |---|---|---| @@ -186,7 +186,7 @@ SM_AUTH_PROVIDER=keycloak Then re-run `make gen-pages` so the frontend manifest picks up the active provider's pages (`make dev` does this for you), and configure the realm under -`/settings/modules`. Switching back is the same knob set to `users`. +`/admin/settings/`. Switching back is the same knob set to `users`. Two caveats when running Keycloak: @@ -221,7 +221,7 @@ The auto-bootstrap is idempotent — it only creates the user if the `users_user ### Inviting users -1. Log in as admin and navigate to `/users/admin/invite`. +1. Log in as admin and navigate to `/admin/users/add`. 2. Fill in the invitee's email and optionally a full name and role(s). Click **Send invite**. 3. With the default `console` mailer, the invite link is logged to stdout (`tail -f` the server log). Copy the link and send it to the user. With `smtp`, the email is delivered automatically. 4. The invitee opens the link (`/users/invite/accept?token=…`), sets a password, and is immediately logged in. diff --git a/docs/e2e-testing.md b/docs/e2e-testing.md index 50155963..1a60d32c 100644 --- a/docs/e2e-testing.md +++ b/docs/e2e-testing.md @@ -2,7 +2,7 @@ Playwright-driven smoke tests live in [tests/e2e/](../tests/e2e/) — currently [`test_settings_ui.py`](../tests/e2e/test_settings_ui.py) (logs in, navigates to -`/settings/modules`, toggles a module setting, and verifies the change +`/admin/settings/`, toggles a module setting, and verifies the change hot-reloads into `app.state` without a server restart) and [`test_audit_log_ui.py`](../tests/e2e/test_audit_log_ui.py). diff --git a/docs/framework/lifecycle.md b/docs/framework/lifecycle.md index 4589fad3..b2b264da 100644 --- a/docs/framework/lifecycle.md +++ b/docs/framework/lifecycle.md @@ -75,11 +75,11 @@ def register_permissions(self, registry: PermissionRegistry) -> None: ) ``` -Permissions become available in the role admin UI (`/settings/permissions`). See [Permissions](/framework/permissions). +Permissions become available in the role admin UI (`/admin/users/` (Roles tab)). See [Permissions](/framework/permissions). ## `register_feature_flags(registry)` -Declare feature flags with defaults. The admin can toggle them at `/settings/feature-flags`. +Declare feature flags with defaults. The admin can toggle them at `/admin/feature-flags/`. ```python def register_feature_flags(self, registry: FeatureFlagRegistry) -> None: diff --git a/docs/framework/permissions.md b/docs/framework/permissions.md index 73f2c29e..e0440ae8 100644 --- a/docs/framework/permissions.md +++ b/docs/framework/permissions.md @@ -68,7 +68,7 @@ DEFAULT_ROLE_PERMISSIONS = { Host apps customize this by: -1. Editing roles in the admin UI at `/settings/permissions`. +1. Editing roles in the admin UI at `/admin/users/` (Roles tab). 2. Or seeding via code during `on_startup` in a custom host-level module. The framework ships only `admin: ["*"]`. The wildcard grants every declared permission. diff --git a/docs/framework/settings.md b/docs/framework/settings.md index a1ddc533..3cbc81e2 100644 --- a/docs/framework/settings.md +++ b/docs/framework/settings.md @@ -6,7 +6,7 @@ There are **three** separate settings surfaces in a running app. They serve diff |---|---|---|---| | **Framework env** (`Settings`) | `app.state.sm.settings` | No (read once at boot) | DB URL, secret key, log level, anything needed before the DB is open. | | **Module env** (`Env`) | `app.state..settings` | No | Module bootstrap knobs that must be resolved before DB-backed settings load. | -| **DB-backed settings** | `settings_setting` table, edited via `/settings/modules` | Yes | Everything else: SMTP creds, storage backends, feature toggles that operators tune. | +| **DB-backed settings** | `settings_setting` table, edited via `/admin/settings/` | Yes | Everything else: SMTP creds, storage backends, feature toggles that operators tune. | ## Framework settings @@ -123,7 +123,7 @@ async def users_config(state: UsersStateDep): ## DB-backed settings -After bootstrap, most configuration lives in the `settings_setting` table and is edited via the admin UI at `/settings/modules`. The CRUD layer is `SettingService` (in `settings.service`); typed reads (with USER > TENANT > SYSTEM resolution and registered defaults) go through a `SettingsAccessor` that wraps it: +After bootstrap, most configuration lives in the `settings_setting` table and is edited via the admin UI at `/admin/settings/`. The CRUD layer is `SettingService` (in `settings.service`); typed reads (with USER > TENANT > SYSTEM resolution and registered defaults) go through a `SettingsAccessor` that wraps it: ```python from settings.service import SettingService diff --git a/docs/guide/configuration.md b/docs/guide/configuration.md index d88db68f..0ae04b60 100644 --- a/docs/guide/configuration.md +++ b/docs/guide/configuration.md @@ -3,7 +3,7 @@ Runtime configuration has **two** sources, in order of precedence: 1. **Environment variables** (`SM_*`) — read at boot, before the DB connection is open. Use for bootstrap plumbing (DB URL, secret key, feature-flag overrides for tests). -2. **DB-backed settings** — edited from `/settings/modules` at runtime. Use for everything else: SMTP creds, storage backends, module-specific toggles. +2. **DB-backed settings** — edited from `/admin/settings/` at runtime. Use for everything else: SMTP creds, storage backends, module-specific toggles. Most deployments only need `SM_DATABASE_URL` and `SM_SECRET_KEY` in the environment — everything else is configurable from the admin UI. @@ -23,7 +23,7 @@ Prefix is always `SM_`. These are the pre-DB knobs read by `simple_module_hostin | `SM_MODULES_ENABLED` | unset (all enabled) | Comma-separated allow-list to disable modules without uninstalling them. | | `SM_AUTH_PUBLIC_PATHS` | `[]` | JSON array of anonymous-access path prefixes — a host-level escape hatch. Modules should prefer the `register_public_routes` hook. | -Multi-tenancy (`multi_tenant`, `tenant_header`) and i18n (`i18n_default_locale`, `i18n_supported_locales`, `i18n_cookie_name`) are **DB-backed host settings** now, not env vars — edit them under `host` at `/settings/modules`. (`smpy new --tenancy` still writes `SM_MULTI_TENANT=true` into `.env.example` as a scaffold convenience, and tests can override these.) +Multi-tenancy (`multi_tenant`, `tenant_header`) and i18n (`i18n_default_locale`, `i18n_supported_locales`, `i18n_cookie_name`) are **DB-backed host settings** now, not env vars — edit them under `host` at `/admin/settings/`. (`smpy new --tenancy` still writes `SM_MULTI_TENANT=true` into `.env.example` as a scaffold convenience, and tests can override these.) ## Database bootstrap knobs @@ -38,7 +38,7 @@ Only change if you know what you're doing — these must be set *before* the DB ## Internationalization -These are **DB-backed host settings** (under `host` in `/settings/modules`), not env vars. +These are **DB-backed host settings** (under `host` in `/admin/settings/`), not env vars. | Setting | Default | Notes | |---|---|---| @@ -48,7 +48,7 @@ These are **DB-backed host settings** (under `host` in `/settings/modules`), not ## Users module -Only the first-boot **bootstrap seed** is read from the env (prefix `SM_USERS_*`). Signup policy, mailer, SMTP creds, and base URL all moved to the DB-backed settings store — edit them under `users` at `/settings/modules`. +Only the first-boot **bootstrap seed** is read from the env (prefix `SM_USERS_*`). Signup policy, mailer, SMTP creds, and base URL all moved to the DB-backed settings store — edit them under `users` at `/admin/settings/`. | Variable | Default | Notes | |---|---|---| @@ -61,7 +61,7 @@ DB-backed users settings (defaults): `allow_signup=false`, `mailer=console` (or ## Background tasks (Celery) -The broker/result settings are DB-backed (under `background_tasks` in `/settings/modules`) with the defaults below. The generated `docker-compose.yml` sets `SM_BG_TASKS_BROKER_URL` / `SM_BG_TASKS_RESULT_BACKEND` on the `worker` and `beat` containers so they reach the in-container `redis` service. +The broker/result settings are DB-backed (under `background_tasks` in `/admin/settings/`) with the defaults below. The generated `docker-compose.yml` sets `SM_BG_TASKS_BROKER_URL` / `SM_BG_TASKS_RESULT_BACKEND` on the `worker` and `beat` containers so they reach the in-container `redis` service. | Setting | Default | Notes | |---|---|---| @@ -78,7 +78,7 @@ uv run smpy settings import-from-env This is idempotent — it only seeds keys that don't have a DB override yet. -From then on, edit at `/settings/modules` (requires the `settings.manage` permission). Changes apply immediately; no restart needed. +From then on, edit at `/admin/settings/` (requires the `settings.manage` permission). Changes apply immediately; no restart needed. ## Per-module settings convention diff --git a/docs/module-authoring.md b/docs/module-authoring.md index 5211f55e..08757c59 100644 --- a/docs/module-authoring.md +++ b/docs/module-authoring.md @@ -119,6 +119,48 @@ class MyModule(ModuleBase): ) ``` +### Administrative screens + +Admin pages belong under `/admin/*`, in `MenuSection.ADMIN_SIDEBAR`, rendered +by `AdminLayout`. Do all three or none — `SidebarLayout` renders whichever +menu its `menuKey` names, so a page still on `AuthenticatedLayout` after its +menu entry moved shows a sidebar that no longer lists it. + +If your module is administrative end to end, that is just a prefix: + +```python +class MyModule(ModuleBase): + meta = ModuleMeta(name="MyModule", view_prefix="/admin/my-module") +``` + +If it serves both public and admin pages — as `users` does, with sign-in at +`/users/login` and management at `/admin/users` — one `view_prefix` cannot +express both. Declare a second mount point and fill it from +`register_admin_routes`: + +```python +class MyModule(ModuleBase): + meta = ModuleMeta( + name="MyModule", + view_prefix="/my-module", # public pages + admin_view_prefix="/admin/my-module", # admin pages + ) + + def register_admin_routes(self, admin_router: APIRouter) -> None: + from my_module.admin.views import router as admin_views + + admin_router.include_router(admin_views) +``` + +The prefix is a URL convention, not a permission: nothing under `/admin` is +gated automatically. Guard these routes with the same dependencies you would +use anywhere else, and give the menu item a matching `permissions=` so it is +not offered to accounts whose click would 403. + +Point the menu item at the canonical path — the trailing-slash form when the +index is registered at `"/"`. Linking to the bare prefix costs a 307 on every +navigation, which `test_menu_urls_are_canonical` will fail you for. + ## API stability contract `simple_module_core` exposes `FRAMEWORK_API_VERSION` (PEP 440 string). At diff --git a/docs/modules/audit_log.md b/docs/modules/audit_log.md index 0f70c279..c1963f99 100644 --- a/docs/modules/audit_log.md +++ b/docs/modules/audit_log.md @@ -8,7 +8,7 @@ Automatic field-level audit trail for every SQLModel entity in the app. Each cre |---|---| | `name` | `AuditLog` | | `route_prefix` | `/api/audit_log` | -| `view_prefix` | `/audit_log` | +| `view_prefix` | `/admin/audit-log` | | `depends_on` | `["Users"]` | ## How capture works @@ -120,7 +120,7 @@ There is no write permission — the trail is append-only and written by the fra | Label | URL | Icon | Section | Group | Order | |---|---|---|---|---|---| -| `Audit Log` | `/audit_log` | `scroll-text` | `SIDEBAR` | `System` | `210` | +| `Audit Log` | `/admin/audit-log` | `scroll-text` | `ADMIN_SIDEBAR` | `System` | `210` | ## Inertia pages diff --git a/docs/modules/branding.md b/docs/modules/branding.md index f5f5906b..496ce712 100644 --- a/docs/modules/branding.md +++ b/docs/modules/branding.md @@ -10,7 +10,7 @@ Values persist in the shared [settings](/modules/settings) store (there is no br |---|---| | `name` | `Branding` | | `route_prefix` | `/api/branding` | -| `view_prefix` | `/branding` | +| `view_prefix` | `/admin/branding` | | `depends_on` | `["Settings", "FileStorage"]` | | `i18n_audience` | `"admin"` | @@ -58,7 +58,7 @@ When `file_storage` is backed by S3-compatible storage, the route returns a `302 | Method + path | Inertia component | Permission | |---|---|---| -| `GET /branding/` | `Branding/Manage` | `branding.view` | +| `GET /admin/branding/` | `Branding/Manage` | `branding.view` | Current branding reaches the page through the shared `branding` prop. The endpoint passes only what the shared prop *can't* carry: `designPacks` (which packs the installed modules registered) and `presets` (the built-in list, with swatches). @@ -186,7 +186,7 @@ The provider is defensive — it returns `{}` if branding state isn't mounted ye | Label | URL | Icon | Section | Group | Order | Roles | |---|---|---|---|---|---|---| -| `Branding` | `/branding/` | `palette` | `SIDEBAR` | `Administration` | `115` | `["admin"]` | +| `Branding` | `/admin/branding/` | `palette` | `ADMIN_SIDEBAR` | `Appearance` | `105` | `["admin"]` | ## Inertia pages diff --git a/docs/modules/feature_flags.md b/docs/modules/feature_flags.md index 21ac570d..8fafab5f 100644 --- a/docs/modules/feature_flags.md +++ b/docs/modules/feature_flags.md @@ -8,7 +8,7 @@ Runtime feature toggles. Modules register named flags with a default value; an a |---|---| | `name` | `FeatureFlags` | | `route_prefix` | `/api/feature_flags` | -| `view_prefix` | `/feature_flags` | +| `view_prefix` | `/admin/feature-flags` | | `depends_on` | _(none)_ | ## Registering a flag @@ -30,7 +30,7 @@ class OrdersModule(ModuleBase): ) ``` -Once registered, the flag shows up at `/feature_flags` in the admin UI. Unregistered names return 404 from the override endpoints — flags are explicit, not free-form keys. +Once registered, the flag shows up at `/admin/feature-flags` in the admin UI. Unregistered names return 404 from the override endpoints — flags are explicit, not free-form keys. ## Reading a flag at request time @@ -113,7 +113,7 @@ Unique constraint on `(scope, scope_id, name)`. The `scope_id=""` (instead of `N | Label | URL | Icon | Section | Group | Order | |---|---|---|---|---|---| -| `Feature Flags` | `/feature_flags` | `flag` | `SIDEBAR` | `Administration` | `110` | +| `Feature Flags` | `/admin/feature-flags` | `flag` | `ADMIN_SIDEBAR` | `System` | `110` | ## Inertia pages diff --git a/docs/modules/settings.md b/docs/modules/settings.md index 66224dd7..b63794aa 100644 --- a/docs/modules/settings.md +++ b/docs/modules/settings.md @@ -4,8 +4,8 @@ A DB-backed key/value store with system / tenant / user precedence, plus a per-m Two distinct surfaces: -1. **Generic key/value settings** — anything addressable by a string `key`. Useful for arbitrary config you don't want to wedge into a pydantic class. Edited at `/settings`. -2. **Per-module pydantic settings** — each module registers a `BaseSettings` subclass via `register_module_settings`. Hydrated from the DB at boot, edited at `/settings/modules`, hot-swapped on save with a `SettingsReloaded` event so dependents (SMTP clients, Celery configs, …) can rebuild. +1. **Generic key/value settings** — anything addressable by a string `key`. Useful for arbitrary config you don't want to wedge into a pydantic class. Edited at `/admin/settings/store`. +2. **Per-module pydantic settings** — each module registers a `BaseSettings` subclass via `register_module_settings`. Hydrated from the DB at boot, edited at `/admin/settings/`, hot-swapped on save with a `SettingsReloaded` event so dependents (SMTP clients, Celery configs, …) can rebuild. ## ModuleMeta @@ -13,7 +13,7 @@ Two distinct surfaces: |---|---| | `name` | `Settings` | | `route_prefix` | `/api/settings` | -| `view_prefix` | `/settings` | +| `view_prefix` | `/admin/settings` | | `depends_on` | _(none)_ | ## Public API for module authors @@ -100,11 +100,11 @@ All write endpoints require `settings.edit` / `settings.create` / `settings.dele | Method + path | Inertia component | |---|---| -| `GET /settings/` | `Settings/Browse` | -| `GET /settings/create` | `Settings/Create` | -| `GET /settings/{setting_id}/edit` | `Settings/Edit` | -| `GET /settings/modules` | `Settings/ModulesEdit` | -| `POST` / `PUT` / `DELETE /settings/...` | form actions; redirect to `/settings` | +| `GET /admin/settings/` | `Settings/Browse` | +| `GET /admin/settings/create` | `Settings/Create` | +| `GET /admin/settings/{setting_id}/edit` | `Settings/Edit` | +| `GET /admin/settings/modules` | legacy redirect → `/admin/settings/` | +| `POST` / `PUT` / `DELETE /admin/settings/...` | form actions; redirect to `/admin/settings` | ## Public contracts @@ -170,7 +170,7 @@ Unique constraint on `(scope, scope_id, key)`. | Label | URL | Icon | Section | Group | Order | |---|---|---|---|---|---| -| `Settings` | `/settings` | `settings` | `SIDEBAR` | `System` | `200` | +| `Settings` | `/admin/settings/` | `settings` | `ADMIN_SIDEBAR` | `System` | `200` | ## Events diff --git a/docs/modules/users.md b/docs/modules/users.md index 5c7fba80..97a55534 100644 --- a/docs/modules/users.md +++ b/docs/modules/users.md @@ -8,7 +8,8 @@ The default auth provider + user-management module: email/password login, OAuth/ |---|---| | `name` | `Users` | | `route_prefix` | `/api/users` | -| `view_prefix` | `/users` | +| `view_prefix` | `/users` (sign-in, self-service) | +| `admin_view_prefix` | `/admin/users` (management CRUD) | | `depends_on` | `["Auth"]` | ## Auth flow @@ -60,7 +61,7 @@ The module is built on [`fastapi-users`](https://fastapi-users.github.io/) for p | `PATCH /api/users/admin/{user_id}/verify` | → `UserListItem` (mark verified; idempotent) | | `POST /api/users/admin/{user_id}/reset-password-link` | → `PasswordResetLink` (`409` for external/SSO users) | -`POST /api/users/admin` creates an **active + verified** user directly — no invite email, no verification flow; the admin sets the password. It returns `409` if the email is already taken and `400` for an invalid password. The matching admin UI (Create form, Edit details card, and a delete "danger zone") lives under `/users/admin` — see [View routes](#view-routes) and [Inertia pages](#inertia-pages). +`POST /api/users/admin` creates an **active + verified** user directly — no invite email, no verification flow; the admin sets the password. It returns `409` if the email is already taken and `400` for an invalid password. The matching admin UI (Create form, Edit details card, and a delete "danger zone") lives under `/admin/users/` — see [View routes](#view-routes) and [Inertia pages](#inertia-pages). ### View routes @@ -207,7 +208,7 @@ Everything else is DB-backed (initial values are pydantic defaults; edit at `/se | Label | URL | Icon | Section | Group | Order | Roles | |---|---|---|---|---|---|---| -| `Users` | `/users/admin` | `users` | `SIDEBAR` | `Administration` | `100` | `["admin"]` | +| `Users` | `/admin/users/` | `users` | `ADMIN_SIDEBAR` | `Access` | `100` | `["admin"]` | | `Profile` | `/users/me` | `user` | `USER_DROPDOWN` | — | `990` | _logged-in_ | | `Logout` | `/users/logout` (POST) | `log-out` | `USER_DROPDOWN` | — | `999` | _logged-in_ | diff --git a/framework/core/simple_module_core/module.py b/framework/core/simple_module_core/module.py index b3d22a23..226625d0 100644 --- a/framework/core/simple_module_core/module.py +++ b/framework/core/simple_module_core/module.py @@ -28,6 +28,18 @@ class ModuleMeta: name: str route_prefix: str = "" view_prefix: str = "" + admin_view_prefix: str = "" + """Mount point for admin-only view routes, e.g. ``"/admin/users"``. + + A module gets exactly one ``view_prefix``, which is a problem for modules + that are only *partly* administrative: ``users`` serves ``/users/login`` + and the user-management CRUD from the same package, and those belong in + different places in the URL space. Declaring this gives such a module a + second view router mounted here, populated by ``register_admin_routes``. + + Modules that are administrative end to end don't need it — they just point + ``view_prefix`` at ``/admin/...`` directly. + """ depends_on: list[str] = field(default_factory=list) version: str = "1.0.0" requires_framework: str | None = None @@ -111,6 +123,19 @@ def register_routes( ) -> None: """Register API endpoints and Inertia view routes.""" + def register_admin_routes(self, admin_router: APIRouter) -> None: + """Register admin-only view routes, mounted at ``meta.admin_view_prefix``. + + Only needed by modules that serve both public and administrative + pages — see ``ModuleMeta.admin_view_prefix``. A module whose views are + all administrative should point ``view_prefix`` at ``/admin/...`` and + keep using ``register_routes``. + + Nothing here is gated automatically: the prefix is a URL convention, + not a permission. Guard these routes with the same dependencies you + would use anywhere else. + """ + def register_menu_items(self, registry: MenuRegistry) -> None: """Contribute menu items visible in the UI.""" diff --git a/framework/core/simple_module_core/permissions.py b/framework/core/simple_module_core/permissions.py index 8cb7335c..1fe15ae7 100644 --- a/framework/core/simple_module_core/permissions.py +++ b/framework/core/simple_module_core/permissions.py @@ -6,13 +6,26 @@ WILDCARD = "*" +ADMIN_ROLE = "admin" +"""The one role name the framework itself knows. + +Everything else about roles is module-owned, but the framework needs this to +resolve the wildcard grant below and to decide who can still reach the app +while maintenance mode is on. +""" + # Default role→permission mapping. Admin gets all permissions via the wildcard. # Additional mappings are added at registration time via PermissionRegistry.map_role. DEFAULT_ROLE_PERMISSIONS: dict[str, list[str]] = { - "admin": [WILDCARD], + ADMIN_ROLE: [WILDCARD], } +def is_admin(roles: list[str] | None) -> bool: + """Whether ``roles`` carries the framework's admin role.""" + return bool(roles) and ADMIN_ROLE in roles + + @dataclass class PermissionGroup: """A named group of related permissions (typically one per module).""" @@ -111,7 +124,7 @@ def get_permissions_for_roles( other roles get none. Override for richer mapping. """ if role_permission_map is None: - if "admin" in roles: + if is_admin(roles): return set(self.all_permissions) return set() diff --git a/framework/core/simple_module_core/redirect_safety.py b/framework/core/simple_module_core/redirect_safety.py new file mode 100644 index 00000000..82e09546 --- /dev/null +++ b/framework/core/simple_module_core/redirect_safety.py @@ -0,0 +1,56 @@ +"""Safety net for user-influenced redirect targets. + +Several flows park "where the visitor was heading" somewhere a browser can +reach — ``AuthMiddleware`` stashes it in the session before bouncing an +anonymous visitor to login, ``site_lock`` puts it in the unlock page's query +string. Anything replayed into a ``Location`` header is an open-redirect +surface, so every producer and consumer funnels through :func:`safe_next`. + +This lives in the framework rather than in whichever module needed it first: +it encodes no plugin knowledge, and duplicating URL-safety rules per module is +how one copy ends up missing a case. +""" + +from __future__ import annotations + +DEFAULT_FALLBACK = "/" + +SESSION_NEXT_KEY = "next" +"""Session key holding the post-login destination. + +This is the contract between ``AuthMiddleware`` (which writes it when it +bounces an anonymous visitor) and whichever provider completes the login and +sends the visitor onward. It is shared rather than redeclared per module +because a provider that reads a *different* key silently loses every deep +link — which is exactly how the local provider drifted from the Keycloak one. +""" + + +def safe_next(raw: str | None, *, fallback: str = DEFAULT_FALLBACK) -> str: + """Return ``raw`` if it is a same-site absolute path, else ``fallback``. + + Rejects protocol-relative (``//host``) and backslash-prefixed (``/\\host``) + targets — browsers resolve both off-site — plus anything carrying CR/LF, + which could otherwise be smuggled into the redirect header. + """ + if not raw or not raw.startswith("/"): + return fallback + if raw.startswith(("//", "/\\")): + return fallback + if "\r" in raw or "\n" in raw: + return fallback + return raw + + +def safe_next_or_none(raw: str | None) -> str | None: + """Like :func:`safe_next`, but ``None`` when ``raw`` is unusable. + + Callers that fall back to a *configured* destination (rather than ``/``) + need to tell "no target" apart from "the target was ``/``" — returning the + fallback would silently outrank a configured ``login_redirect_url``. + """ + result = safe_next(raw, fallback="") + return result or None + + +__all__ = ["DEFAULT_FALLBACK", "SESSION_NEXT_KEY", "safe_next", "safe_next_or_none"] diff --git a/framework/core/tests/test_redirect_safety.py b/framework/core/tests/test_redirect_safety.py new file mode 100644 index 00000000..077b3e35 --- /dev/null +++ b/framework/core/tests/test_redirect_safety.py @@ -0,0 +1,59 @@ +"""Tests for the shared redirect-target sanitiser.""" + +from __future__ import annotations + +import pytest +from simple_module_core.redirect_safety import safe_next, safe_next_or_none + + +class TestSafeNext: + @pytest.mark.parametrize( + "raw", + [ + "/dashboard/", + "/admin/users?page=2", + "/admin/users#anchor", + "/", + ], + ) + def test_same_site_paths_pass_through(self, raw: str) -> None: + assert safe_next(raw) == raw + + @pytest.mark.parametrize( + "raw", + [ + None, + "", + "https://evil.example/phish", + "dashboard/", + "javascript:alert(1)", + ], + ) + def test_non_relative_targets_are_rejected(self, raw: str | None) -> None: + assert safe_next(raw) == "/" + + @pytest.mark.parametrize("raw", ["//evil.example", "/\\evil.example"]) + def test_off_site_lookalikes_are_rejected(self, raw: str) -> None: + """Browsers resolve both forms against the remote host, not ours.""" + assert safe_next(raw) == "/" + + @pytest.mark.parametrize("raw", ["/ok\r\nLocation: https://evil.example", "/ok\nX: y"]) + def test_header_smuggling_is_rejected(self, raw: str) -> None: + assert safe_next(raw) == "/" + + def test_fallback_is_configurable(self) -> None: + assert safe_next("https://evil.example", fallback="/login") == "/login" + + +class TestSafeNextOrNone: + def test_valid_target_returned(self) -> None: + assert safe_next_or_none("/admin/settings") == "/admin/settings" + + @pytest.mark.parametrize("raw", [None, "", "//evil.example", "https://evil.example"]) + def test_unusable_target_is_none(self, raw: str | None) -> None: + assert safe_next_or_none(raw) is None + + def test_root_is_a_real_target_not_a_miss(self) -> None: + """``/`` must stay distinguishable from "nothing stashed" — otherwise a + caller cannot tell whether to fall back to its configured destination.""" + assert safe_next_or_none("/") == "/" diff --git a/framework/hosting/simple_module_hosting/_error_handlers.py b/framework/hosting/simple_module_hosting/_error_handlers.py index a0846ac7..87e3a7e9 100644 --- a/framework/hosting/simple_module_hosting/_error_handlers.py +++ b/framework/hosting/simple_module_hosting/_error_handlers.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging +from collections.abc import Mapping from fastapi.encoders import jsonable_encoder from fastapi.exceptions import RequestValidationError @@ -22,7 +23,30 @@ logger = logging.getLogger(__name__) -_INERTIA_ERROR_STATUSES = frozenset({403, 404, 500}) +_INERTIA_ERROR_STATUSES = frozenset({401, 403, 404, 419, 422, 429, 500, 503}) + +# Statuses whose remedy is "sign in", so the page offers that as its primary +# action rather than sending the visitor to the landing page. +_SIGN_IN_STATUSES = frozenset({401, 419}) + + +def _login_url(request: Request) -> str | None: + """Best-effort login URL for the sign-in statuses. + + Read off ``app.state`` rather than imported: the auth provider is a plugin + concern and ``SM009`` forbids framework code importing ``modules/*``. An + app with no auth provider installed simply gets no sign-in button. + """ + auth_state = getattr(request.app.state, "auth", None) + provider = getattr(auth_state, "auth_provider", None) + if provider is None: + return None + try: + return provider.get_login_url(request) + except Exception: + # A broken provider must not turn an error page into a second error. + logger.exception("Auth provider failed to supply a login URL") + return None def _explicit_accept_q(accept: str, media_type: str) -> float | None: @@ -85,9 +109,25 @@ def _wants_json(request: Request) -> bool: return is_api or bool(json_q) -async def render_error_page(request: Request, status_code: int, message: str) -> Response: - config: InertiaConfig = request.app.state.sm.inertia_config +async def render_error_page( + request: Request, + status_code: int, + message: str, + headers: Mapping[str, str] | None = None, +) -> Response: + """Render the Inertia error page for *status_code*. + + ``headers`` carries an ``HTTPException``'s own headers through to the + response. A rendered page is still the same status as the JSON body it + replaces, so ``WWW-Authenticate`` on a 401 and ``Retry-After`` on a + 429/503 have to survive the switch — widening the set of statuses that + render a page must not quietly narrow what those responses carry. + """ try: + # Inside the try, not above it: this lookup is exactly the kind of + # thing that is missing when the app is half-built, and an error page + # that raises while reporting an error leaves the caller with nothing. + config: InertiaConfig = request.app.state.sm.inertia_config inertia = Inertia(request, config) # This builds its own Inertia instead of going through get_inertia, so # the share step has to be repeated here. Without it the error page @@ -106,9 +146,14 @@ async def render_error_page(request: Request, status_code: int, message: str) -> "status": status_code, "message": message, "correlation_id": getattr(request.state, "correlation_id", "") or "", + "login_url": (_login_url(request) if status_code in _SIGN_IN_STATUSES else None), + # Set by MaintenanceMiddleware. A planned outage reads very + # differently from the same status code arriving unbidden. + "maintenance": bool(getattr(request.state, "maintenance", False)), }, ) response.status_code = status_code + _apply_headers(response, headers) return response except InertiaVersionConflictException as exc: return await inertia_version_conflict_exception_handler(request, exc) @@ -116,20 +161,36 @@ async def render_error_page(request: Request, status_code: int, message: str) -> # Fallback if Inertia rendering itself fails (e.g. missing session) logger.exception("Error page rendering failed, falling back to JSON") return JSONResponse( - status_code=status_code, content={"detail": message or "Internal Server Error"} + status_code=status_code, + content={"detail": message or "Internal Server Error"}, + headers=dict(headers) if headers else None, ) +def _apply_headers(response: Response, headers: Mapping[str, str] | None) -> None: + """Copy exception headers onto an already-built response. + + Set rather than appended: these are single-valued response headers, and + a duplicate ``Retry-After`` is worse than none. + """ + if not headers: + return + for key, value in headers.items(): + response.headers[key] = value + + async def http_exception_handler(request: Request, exc: HTTPException) -> Response: + # Preserve exception headers (WWW-Authenticate, Retry-After, ...) the way + # FastAPI's stock handler does — on the rendered page as well as the JSON + # body, since both answer with the same status. + headers = getattr(exc, "headers", None) if exc.status_code in _INERTIA_ERROR_STATUSES and not _wants_json(request): detail = str(exc.detail) if exc.detail else "" - return await render_error_page(request, exc.status_code, detail) - # Preserve exception headers (WWW-Authenticate, Retry-After, ...) the way - # FastAPI's stock handler does. + return await render_error_page(request, exc.status_code, detail, headers) return JSONResponse( status_code=exc.status_code, content={"detail": exc.detail}, - headers=getattr(exc, "headers", None), + headers=headers, ) diff --git a/framework/hosting/simple_module_hosting/_module_routes.py b/framework/hosting/simple_module_hosting/_module_routes.py new file mode 100644 index 00000000..6dcf02bb --- /dev/null +++ b/framework/hosting/simple_module_hosting/_module_routes.py @@ -0,0 +1,70 @@ +"""Mount a module's routers onto the app using its ``ModuleMeta`` prefixes. + +Split out of ``_phase_helpers`` once the admin router made this its own +responsibility — and to keep both files under the 300-line cap. +""" + +from __future__ import annotations + +from fastapi import APIRouter, FastAPI +from fastapi.routing import APIRoute + +__all__ = ["wire_module_routes"] + + +def wire_module_routes(app: FastAPI, module) -> None: + """Attach a module's API + view (+ admin view) routers to ``app``. + + The single canonical implementation so ``create_app`` and the test harness + in ``simple_module_test`` stay in lockstep if ``ModuleBase`` ever gains + a new router type. + """ + api_router = APIRouter(prefix=module.meta.route_prefix, tags=[module.meta.name]) + view_router = APIRouter(prefix=module.meta.view_prefix, tags=[f"{module.meta.name} Views"]) + module.register_routes(api_router, view_router) + _clone_bare_prefix_route(view_router, module.meta.view_prefix) + app.include_router(api_router) + app.include_router(view_router) + + # Second view router for modules that serve both public and admin pages + # and therefore cannot express both under a single view_prefix — see + # ``ModuleMeta.admin_view_prefix``. + admin_prefix = getattr(module.meta, "admin_view_prefix", "") + if admin_prefix: + admin_router = APIRouter( + prefix=admin_prefix, + tags=[f"{module.meta.name} Admin Views"], + ) + module.register_admin_routes(admin_router) + _clone_bare_prefix_route(admin_router, admin_prefix) + app.include_router(admin_router) + + +def _clone_bare_prefix_route(router: APIRouter, prefix: str) -> None: + """Serve ``"/foo"`` as well as ``"/foo/"`` for a bare-prefix route. + + Without this, FastAPI's ``redirect_slashes=True`` fires a 307 to + ``"/foo/"``, which clients like httpx strip ``X-Inertia`` from on follow — + turning that Inertia navigation into a broken HTML response. + + Only covers routes declared *directly* on the router. A route contributed + via ``router.include_router(...)`` — which is how most modules register — + is not visible here: ``include_router`` stores a placeholder and only + flattens into ``APIRoute`` objects later, so there is nothing to match + yet. Modules in that (majority) case point their menu item at the + canonical trailing-slash URL instead, which costs no redirect either. + """ + if not prefix: + return + bare_target = f"{prefix}/" + for route in list(router.routes): + if isinstance(route, APIRoute) and route.path == bare_target: + router.add_api_route( + "", + route.endpoint, + methods=list(route.methods or {"GET"}), + response_model=route.response_model, + include_in_schema=False, + dependencies=route.dependencies, + name=f"{route.name}__bare", + ) diff --git a/framework/hosting/simple_module_hosting/_phase_helpers.py b/framework/hosting/simple_module_hosting/_phase_helpers.py index 4f53c15a..cf79b304 100644 --- a/framework/hosting/simple_module_hosting/_phase_helpers.py +++ b/framework/hosting/simple_module_hosting/_phase_helpers.py @@ -11,9 +11,8 @@ from pathlib import Path from typing import TYPE_CHECKING -from fastapi import APIRouter, FastAPI +from fastapi import FastAPI from fastapi.exceptions import RequestValidationError -from fastapi.routing import APIRoute from fastapi.staticfiles import StaticFiles from inertia import ( InertiaVersionConflictException, @@ -35,8 +34,10 @@ ) from simple_module_hosting._host_services import _HostServices from simple_module_hosting._inertia_cache import InertiaCacheMiddleware +from simple_module_hosting._module_routes import wire_module_routes from simple_module_hosting.host_settings import HostSettings from simple_module_hosting.i18n_middleware import LocaleMiddleware +from simple_module_hosting.maintenance import MaintenanceMiddleware from simple_module_hosting.middleware import ( CorrelationIdMiddleware, InertiaLayoutDataMiddleware, @@ -104,12 +105,21 @@ def install_middleware( Order matters: last added = first executed. Execution order: (ProxyHeaders, if trusted_proxy) → CorrelationId → RequestLogging → Security → Session → [module] → (Tenant, if multi_tenant) → Locale - → Inertia → InertiaCache → CommitBeforeResponse. + → Inertia → InertiaCache → Maintenance → CommitBeforeResponse. """ # Added first, so it is innermost and its send-wrapper is the first to see # the response: the request's DB work commits before any byte reaches the # client, instead of in get_db's post-response exit code (GH #257). app.add_middleware(CommitBeforeResponseMiddleware) + # Inside InertiaCache, so its short-circuit is still governed by it. The + # maintenance 503 renders through Inertia and carries this user's auth + # block and menus like any other payload; short-circuiting *outside* the + # cache guard would ship exactly the per-user payload GH #272 exists to + # keep out of caches. Added before Inertia so it *executes* after it: the + # page needs the shared props (auth, menus, i18n) to render with a layout, + # and auth + locale — both further out — to know who is asking and in which + # language to answer. + app.add_middleware(MaintenanceMiddleware) # Paired with InertiaLayoutDataMiddleware below, which is what puts this # user's auth, permissions and menus into every Inertia payload: this one # makes sure the payload that results is never stored where a page request @@ -256,35 +266,4 @@ def check_settings_registration(app: FastAPI, modules: list) -> list[Diagnostic] return diagnostics -def wire_module_routes(app: FastAPI, module) -> None: - """Attach a module's API + view routers to ``app`` using its Meta prefixes. - - The single canonical implementation so ``create_app`` and the test harness - in ``simple_module_test`` stay in lockstep if ``ModuleBase`` ever gains - a new router type. - - Bare-prefix view routes (``view_prefix="/foo"`` + ``@router.get("/")``) - are also mounted at the trailing-slash-less form ``"/foo"``. Without this, - FastAPI's ``redirect_slashes=True`` fires a 307 to ``"/foo/"``, which - clients like httpx strip ``X-Inertia`` from on follow — turning every - Inertia navigation into a broken HTML response. Cloning the route at the - bare-prefix path serves the same handler directly, no redirect. - """ - api_router = APIRouter(prefix=module.meta.route_prefix, tags=[module.meta.name]) - view_router = APIRouter(prefix=module.meta.view_prefix, tags=[f"{module.meta.name} Views"]) - module.register_routes(api_router, view_router) - if module.meta.view_prefix: - bare_target = f"{module.meta.view_prefix}/" - for route in list(view_router.routes): - if isinstance(route, APIRoute) and route.path == bare_target: - view_router.add_api_route( - "", - route.endpoint, - methods=list(route.methods or {"GET"}), - response_model=route.response_model, - include_in_schema=False, - dependencies=route.dependencies, - name=f"{route.name}__bare", - ) - app.include_router(api_router) - app.include_router(view_router) +__all__ = ["wire_module_routes"] diff --git a/framework/hosting/simple_module_hosting/host_settings.py b/framework/hosting/simple_module_hosting/host_settings.py index 779d9071..b6d9989c 100644 --- a/framework/hosting/simple_module_hosting/host_settings.py +++ b/framework/hosting/simple_module_hosting/host_settings.py @@ -19,6 +19,16 @@ class HostSettings(BaseSettings): multi_tenant: bool = False tenant_header: str = "" + maintenance_mode: bool = False + """Serve everyone but admins a 503 page. + + DB-backed rather than an env var on purpose: flipping it must not need a + redeploy, which is exactly when you want it. + """ + maintenance_message: str = "" + """Optional operator note shown on the maintenance page. Empty = use the + generic translated copy.""" + i18n_default_locale: str = "en" i18n_supported_locales: list[str] = ["en"] i18n_cookie_name: str = "locale" diff --git a/framework/hosting/simple_module_hosting/maintenance.py b/framework/hosting/simple_module_hosting/maintenance.py new file mode 100644 index 00000000..c4e8925c --- /dev/null +++ b/framework/hosting/simple_module_hosting/maintenance.py @@ -0,0 +1,125 @@ +"""Maintenance mode — serve everyone but admins a 503 page. + +Sits late in the pipeline, after auth (so it knows who is asking), after +locale (so the page is translated) and after the Inertia shared-props +middleware (so the page keeps its layout instead of rendering bare). + +Admins pass through. That is the whole point: someone has to be able to reach +the settings screen and turn it back off. For the same reason the auth +provider's own routes stay open — an admin who is signed *out* when the switch +is flipped must still be able to sign in. +""" + +from __future__ import annotations + +import logging + +from simple_module_core.permissions import is_admin +from starlette.requests import Request +from starlette.responses import JSONResponse +from starlette.types import ASGIApp, Receive, Scope, Send + +logger = logging.getLogger(__name__) + +# Kept reachable while the gate is closed: liveness probes (so orchestrators +# do not kill the pod mid-maintenance), static assets and i18n bundles (or the +# 503 page renders unstyled and untranslated). +_ALWAYS_OPEN_PREFIXES = ( + "/health", + "/static/", + "/i18n/", +) + +# Both representations of the 503 advertise the same retry window, so it is +# stated once. An hour is a guess by construction — the switch carries no +# end time — but a client that backs off for an hour beats one that hammers. +_RETRY_AFTER = {"Retry-After": "3600"} + +__all__ = ["MaintenanceMiddleware"] + + +class MaintenanceMiddleware: + """Short-circuit non-admin traffic with a 503 while maintenance is on.""" + + def __init__(self, app: ASGIApp) -> None: + self.app = app + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] != "http": + await self.app(scope, receive, send) + return + + settings = self._host_settings(scope) + if settings is None or not getattr(settings, "maintenance_mode", False): + await self.app(scope, receive, send) + return + + path: str = scope["path"] + if any(path.startswith(p) for p in _ALWAYS_OPEN_PREFIXES): + await self.app(scope, receive, send) + return + + request = Request(scope) + if self._may_bypass(request, scope): + await self.app(scope, receive, send) + return + + # Distinguishes "we took the site down on purpose" from the generic + # 503 the page would otherwise show. The page needs the flag because + # the operator's message is optional — without it there would be + # nothing to say beyond "service unavailable". + request.state.maintenance = True + message = getattr(settings, "maintenance_message", "") or "" + response = await self._render(request, message) + await response(scope, receive, send) + + @staticmethod + def _host_settings(scope: Scope): + host_state = getattr(scope["app"].state, "host", None) + return getattr(host_state, "settings", None) + + @staticmethod + def _may_bypass(request: Request, scope: Scope) -> bool: + """Admins, anyone heading for the auth provider's own routes, and any + route a module deliberately opened to anonymous visitors.""" + user = getattr(request.state, "user", None) + if user is not None and is_admin(getattr(user, "roles", None)): + return True + + path: str = scope["path"] + + # Module-contributed public routes (register_public_routes hook) — the + # same registry AuthMiddleware consults. Without this, a route a module + # deliberately exempted from auth (branding's logo/favicon, a webhook, + # a STAC/OGC read endpoint) still 503s during maintenance. + public_routes = getattr(scope["app"].state, "public_routes", None) + if public_routes is not None and public_routes.matches(request.method, path): + return True + + # An admin locked out by the switch still needs the login flow. Ask the + # provider which paths those are rather than hardcoding a module's URLs. + auth_state = getattr(scope["app"].state, "auth", None) + provider = getattr(auth_state, "auth_provider", None) + if provider is None: + return False + try: + prefix_paths, exact_paths = provider.get_public_paths() + except Exception: + logger.exception("Auth provider failed to report public paths") + return False + return any(path.startswith(p) for p in prefix_paths) or path in exact_paths + + @staticmethod + async def _render(request: Request, message: str): + # Imported here rather than at module scope: _error_handlers imports + # from inertia, and a circular import at boot is a worse failure than + # a per-request attribute lookup. + from simple_module_hosting._error_handlers import _wants_json, render_error_page + + if _wants_json(request): + return JSONResponse( + status_code=503, + content={"detail": message or "Service temporarily unavailable"}, + headers=_RETRY_AFTER, + ) + return await render_error_page(request, 503, message, _RETRY_AFTER) diff --git a/framework/hosting/simple_module_hosting/redirects.py b/framework/hosting/simple_module_hosting/redirects.py index f813a0a9..973d8676 100644 --- a/framework/hosting/simple_module_hosting/redirects.py +++ b/framework/hosting/simple_module_hosting/redirects.py @@ -11,28 +11,30 @@ from urllib.parse import urlsplit from fastapi import Request +from simple_module_core.redirect_safety import safe_next def safe_referer_or_root(request: Request) -> str: """Return the Referer iff it's same-origin; otherwise fall back to ``/``. Only honors references that (a) resolve to the same scheme+host as the - current request, or (b) are relative paths that don't try to escape to a - protocol-relative URL (``//evil.example``). + current request, or (b) are relative paths. Either way the result is run + through :func:`~simple_module_core.redirect_safety.safe_next`, which is the + single owner of what counts as a safe same-site target. """ referer = request.headers.get("referer") if not referer: return "/" - # Protocol-relative URLs like "//evil.example/foo" resolve against the - # origin in browsers but leave the site — reject them. - if referer.startswith("//"): - return "/" - parsed = urlsplit(referer) - # Relative path with no scheme+host → same-origin by construction. + # Relative reference (no scheme+host). ``safe_next`` owns the rules here — + # it rejects protocol-relative ("//host") and backslash-prefixed ("/\\host") + # targets, both of which browsers resolve off-site, plus anything carrying + # CR/LF that could be smuggled into the Location header. Delegated rather + # than restated so the two sanitisers cannot drift: a second copy that + # missed one of those cases is exactly how this becomes an open redirect. if not parsed.scheme and not parsed.netloc: - return referer if referer.startswith("/") else "/" + return safe_next(referer) # Absolute URL → must match the current request's origin. current = request.url @@ -40,6 +42,6 @@ def safe_referer_or_root(request: Request) -> str: path = parsed.path or "/" if parsed.query: path = f"{path}?{parsed.query}" - return path + return safe_next(path) return "/" diff --git a/framework/hosting/tests/test_error_status_coverage.py b/framework/hosting/tests/test_error_status_coverage.py new file mode 100644 index 00000000..849d0406 --- /dev/null +++ b/framework/hosting/tests/test_error_status_coverage.py @@ -0,0 +1,229 @@ +"""Every status the handler renders must have copy on the page. + +The Inertia error page keys its title/description/accent off the numeric +status. A status the handler renders but the page has no row for falls back +to a bare "Error / An unexpected error occurred" — which is worse than the +generic message suggests, because the user is told nothing actionable. These +two lists live in different languages, so nothing but a test keeps them +honest. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest +from simple_module_hosting._error_handlers import ( + _INERTIA_ERROR_STATUSES, + _SIGN_IN_STATUSES, + _login_url, +) + +_ERROR_PAGE = Path(__file__).resolve().parents[3] / "host" / "client_app" / "pages" / "Error.tsx" + + +def _statuses_with_copy() -> set[int]: + """Numeric keys of the status table in Error.tsx.""" + source = _ERROR_PAGE.read_text(encoding="utf-8") + table = re.search( + r"const table: Record = \{(.*?)\n \};", source, re.DOTALL + ) + assert table, "status table not found in Error.tsx — did its shape change?" + return {int(m) for m in re.findall(r"^ (\d{3}):", table.group(1), re.MULTILINE)} + + +class TestStatusCopyParity: + def test_error_page_exists(self) -> None: + assert _ERROR_PAGE.is_file(), _ERROR_PAGE + + def test_every_rendered_status_has_copy(self) -> None: + missing = _INERTIA_ERROR_STATUSES - _statuses_with_copy() + assert not missing, ( + f"statuses rendered by the handler with no copy in Error.tsx: {sorted(missing)}" + ) + + def test_sign_in_statuses_are_rendered_statuses(self) -> None: + """Offering a sign-in button on a status that never reaches the page + would be dead code.""" + assert _SIGN_IN_STATUSES <= _INERTIA_ERROR_STATUSES + + @pytest.mark.parametrize("status", [401, 403, 404, 419, 422, 429, 500, 503]) + def test_expected_statuses_are_covered(self, status: int) -> None: + assert status in _INERTIA_ERROR_STATUSES + + +class TestErrorCopyIsReachable: + """Every host.error.* string must actually be rendered by something. + + Adding a status's copy and forgetting to wire it leaves a key that no + code path can reach — the page silently shows the generic message + instead. That is how ``maintenance_title`` was dead on arrival: the 503 + branch existed, but nothing ever selected the maintenance wording. + """ + + def test_every_error_key_is_referenced(self) -> None: + import json + + locales = _ERROR_PAGE.parents[3] / "host" / "locales" / "en.json" + catalog = json.loads(locales.read_text(encoding="utf-8"))["error"] + source = _ERROR_PAGE.read_text(encoding="utf-8") + + # Two spellings in the page: the `keys.host.error.x` path, and the + # `e.x` alias the status table uses. + unreferenced = sorted( + k for k in catalog if f"e.{k}" not in source and f"keys.host.error.{k}" not in source + ) + assert not unreferenced, ( + "host.error keys with no reference in Error.tsx — either render " + f"them or delete them: {unreferenced}" + ) + + +class _StubRequest: + def __init__(self, provider: object | None) -> None: + class _AuthState: + auth_provider = provider + + class _State: + auth = _AuthState() if provider is not None else None + + class _App: + state = _State() + + self.app = _App() + + +class TestLoginUrlLookup: + def test_returns_provider_url(self) -> None: + class _Provider: + def get_login_url(self, request, next_url=None): + return "/users/login" + + assert _login_url(_StubRequest(_Provider())) == "/users/login" + + def test_no_auth_provider_yields_none(self) -> None: + """An app with no auth module installed simply gets no sign-in button.""" + assert _login_url(_StubRequest(None)) is None + + def test_broken_provider_does_not_raise(self) -> None: + """An error page that itself errors is the worst possible outcome.""" + + class _Exploding: + def get_login_url(self, request, next_url=None): + raise RuntimeError("provider is down") + + assert _login_url(_StubRequest(_Exploding())) is None + + +class TestRenderFallback: + """render_error_page must never raise — it is the last line of defence.""" + + async def test_half_built_app_falls_back_to_json(self) -> None: + """``app.state.sm`` is missing while the app is still assembling. That + lookup used to sit outside the try, so the documented JSON fallback + never ran and the error page raised while reporting an error.""" + from simple_module_hosting._error_handlers import render_error_page + from starlette.applications import Starlette + from starlette.requests import Request + + app = Starlette() + request = Request( + { + "type": "http", + "http_version": "1.1", + "method": "GET", + "path": "/boom", + "raw_path": b"/boom", + "query_string": b"", + "scheme": "http", + "server": ("testserver", 80), + "client": ("testclient", 1234), + "headers": [], + "app": app, + } + ) + + resp = await render_error_page(request, 500, "kaboom") + + assert resp.status_code == 500 + assert b"kaboom" in resp.body + + +class TestExceptionHeadersSurvive: + """An ``HTTPException``'s headers must reach the caller on the rendered page + too, not only on the JSON body. + + Widening ``_INERTIA_ERROR_STATUSES`` to cover 401/429/503 moved exactly the + statuses whose headers carry meaning — ``WWW-Authenticate``, ``Retry-After`` + — onto the page-rendering branch. If that branch drops them, the framework + quietly stops honouring a contract its own comment still claims. + """ + + @staticmethod + def _request(app, headers: list[tuple[bytes, bytes]] | None = None): + from starlette.requests import Request + + return Request( + { + "type": "http", + "http_version": "1.1", + "method": "GET", + "path": "/gated", + "raw_path": b"/gated", + "query_string": b"", + "scheme": "http", + "server": ("testserver", 80), + "client": ("testclient", 1234), + "headers": headers or [], + "app": app, + } + ) + + async def test_render_fallback_carries_the_headers(self) -> None: + """Even the half-built-app JSON fallback keeps them.""" + from simple_module_hosting._error_handlers import render_error_page + from starlette.applications import Starlette + + resp = await render_error_page( + self._request(Starlette()), + 429, + "slow down", + {"Retry-After": "60"}, + ) + + assert resp.status_code == 429 + assert resp.headers["Retry-After"] == "60" + + async def test_http_exception_handler_forwards_them_to_the_page(self) -> None: + from simple_module_hosting._error_handlers import http_exception_handler + from starlette.applications import Starlette + from starlette.exceptions import HTTPException + + # Accept: text/html makes this browser-shaped, so it takes the + # page-rendering branch rather than the JSON one. + request = self._request(Starlette(), [(b"accept", b"text/html")]) + exc = HTTPException( + status_code=401, + detail="Not authenticated", + headers={"WWW-Authenticate": "Bearer"}, + ) + + resp = await http_exception_handler(request, exc) + + assert resp.status_code == 401 + assert resp.headers["WWW-Authenticate"] == "Bearer" + + async def test_json_branch_still_carries_them(self) -> None: + """The pre-existing behaviour this must not regress.""" + from simple_module_hosting._error_handlers import http_exception_handler + from starlette.applications import Starlette + from starlette.exceptions import HTTPException + + request = self._request(Starlette(), [(b"accept", b"application/json")]) + exc = HTTPException(429, "Too many", headers={"Retry-After": "30"}) + + resp = await http_exception_handler(request, exc) + + assert resp.status_code == 429 + assert resp.headers["Retry-After"] == "30" diff --git a/framework/hosting/tests/test_maintenance_mode.py b/framework/hosting/tests/test_maintenance_mode.py new file mode 100644 index 00000000..2799cb32 --- /dev/null +++ b/framework/hosting/tests/test_maintenance_mode.py @@ -0,0 +1,270 @@ +"""Maintenance mode gates everyone except the people who can turn it off.""" + +from __future__ import annotations + +import httpx +import pytest +from simple_module_hosting._inertia_cache import InertiaCacheMiddleware +from simple_module_hosting.maintenance import MaintenanceMiddleware +from starlette.applications import Starlette +from starlette.responses import PlainTextResponse +from starlette.routing import Route + + +class _User: + def __init__(self, roles: list[str]) -> None: + self.roles = roles + + +class _Provider: + """Mirrors the AuthProvider surface MaintenanceMiddleware actually uses.""" + + def __init__(self, *, explode: bool = False) -> None: + self._explode = explode + + def get_public_paths(self): + if self._explode: + raise RuntimeError("provider is down") + return (("/users/login",), ("/exact-public",)) + + +def _build_app( + *, + enabled: bool, + message: str = "", + user: _User | None = None, + provider: _Provider | None = _Provider(), + public_routes=None, + with_inertia_cache: bool = False, +) -> Starlette: + async def ok(request): + return PlainTextResponse("app reached") + + app = Starlette( + routes=[ + Route("/protected", ok), + Route("/users/login", ok), + Route("/exact-public", ok), + Route("/health", ok), + Route("/api/branding/logo", ok), + Route("/api/thing", ok), + ] + ) + + class _HostSettings: + maintenance_mode = enabled + maintenance_message = message + + class _HostState: + settings = _HostSettings() + + app.state.host = _HostState() + + class _AuthState: + auth_provider = provider + + app.state.auth = _AuthState() + app.state.public_routes = public_routes + + app.add_middleware(MaintenanceMiddleware) + # Real pipeline order: InertiaCache sits directly outside Maintenance, so + # its send-wrapper is what actually receives the 503's response messages. + # Added second so it wraps Maintenance and is itself wrapped by _SeedUser, + # matching install_middleware's (... -> InertiaCache -> Maintenance -> ...). + if with_inertia_cache: + app.add_middleware(InertiaCacheMiddleware) + # Stands in for AuthMiddleware, which runs further out and is what puts the + # resolved user on request.state. Added last so it executes first, exactly + # as the real pipeline orders them. + app.add_middleware(_SeedUser, user=user) + return app + + +class _SeedUser: + """Minimal stand-in for AuthMiddleware's contribution to request.state.""" + + def __init__(self, app, user: _User | None) -> None: + self.app = app + self.user = user + + async def __call__(self, scope, receive, send) -> None: + if scope["type"] == "http" and self.user is not None: + scope.setdefault("state", {})["user"] = self.user + await self.app(scope, receive, send) + + +async def _get(app, path: str, **kwargs) -> httpx.Response: + transport = httpx.ASGITransport(app=app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as c: + return await c.get(path, **kwargs) + + +async def _post(app, path: str, **kwargs) -> httpx.Response: + transport = httpx.ASGITransport(app=app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as c: + return await c.post(path, **kwargs) + + +class TestGateClosed: + async def test_anonymous_visitor_gets_503(self) -> None: + resp = await _get(_build_app(enabled=True), "/protected") + assert resp.status_code == 503 + + async def test_non_admin_gets_503(self) -> None: + app = _build_app(enabled=True, user=_User(["editor"])) + resp = await _get(app, "/protected") + assert resp.status_code == 503 + + async def test_admin_passes_through(self) -> None: + """Someone has to be able to reach settings and switch it back off.""" + app = _build_app(enabled=True, user=_User(["admin"])) + resp = await _get(app, "/protected") + assert resp.status_code == 200 + assert resp.text == "app reached" + + async def test_retry_after_is_advertised(self) -> None: + resp = await _get(_build_app(enabled=True), "/api/thing") + assert resp.headers.get("Retry-After") + + +class TestAlwaysReachable: + async def test_health_probe_survives(self) -> None: + """Orchestrators must not kill the pod mid-maintenance.""" + resp = await _get(_build_app(enabled=True), "/health") + assert resp.status_code == 200 + + async def test_login_prefix_stays_open(self) -> None: + """An admin signed out when the switch flipped must still get in.""" + resp = await _get(_build_app(enabled=True), "/users/login") + assert resp.status_code == 200 + + async def test_exact_public_path_stays_open(self) -> None: + resp = await _get(_build_app(enabled=True), "/exact-public") + assert resp.status_code == 200 + + +class TestGateOpen: + async def test_disabled_is_a_no_op(self) -> None: + resp = await _get(_build_app(enabled=False), "/protected") + assert resp.status_code == 200 + + async def test_anonymous_reaches_app_when_disabled(self) -> None: + resp = await _get(_build_app(enabled=False, user=None), "/protected") + assert resp.text == "app reached" + + +class TestDegradedDependencies: + async def test_missing_host_state_does_not_block_traffic(self) -> None: + """Fail open on a missing setting — a config gap must not take the + site down, which is the exact failure this feature would cause.""" + + async def ok(request): + return PlainTextResponse("app reached") + + app = Starlette(routes=[Route("/protected", ok)]) + app.add_middleware(MaintenanceMiddleware) + resp = await _get(app, "/protected") + assert resp.status_code == 200 + + async def test_broken_provider_still_gates(self) -> None: + """A provider that raises must not accidentally open the gate.""" + app = _build_app(enabled=True, provider=_Provider(explode=True)) + resp = await _get(app, "/protected") + assert resp.status_code == 503 + + async def test_no_auth_provider_still_gates(self) -> None: + app = _build_app(enabled=True, provider=None) + resp = await _get(app, "/protected") + assert resp.status_code == 503 + + +class TestJsonCallers: + async def test_api_caller_gets_json_not_html(self) -> None: + app = _build_app(enabled=True, message="Back at 14:00 UTC") + resp = await _get(app, "/api/thing") + assert resp.status_code == 503 + assert resp.json()["detail"] == "Back at 14:00 UTC" + + async def test_generic_detail_when_no_message_set(self) -> None: + resp = await _get(_build_app(enabled=True), "/api/thing") + assert resp.json()["detail"] + + +@pytest.mark.parametrize("roles", [[], ["viewer"], ["admin-ish"], ["Admin"]]) +async def test_only_the_exact_admin_role_bypasses(roles: list[str]) -> None: + """Substring or case-insensitive matching here would be a privilege bug.""" + app = _build_app(enabled=True, user=_User(roles)) + resp = await _get(app, "/protected") + assert resp.status_code == 503 + + +class TestModulePublicRoutes: + """A module's ``register_public_routes`` rules must also bypass the gate. + + AuthMiddleware already exempts these paths from login, so a route like + branding's public logo/favicon GET must stay reachable during maintenance + too — otherwise the sign-in/maintenance page itself loses its branding. + """ + + async def test_module_public_route_stays_open(self) -> None: + from simple_module_core.public_routes import PublicRouteRegistry + + registry = PublicRouteRegistry() + registry.add_exact("/api/branding/logo", methods=["GET"]) + resp = await _get(_build_app(enabled=True, public_routes=registry), "/api/branding/logo") + assert resp.status_code == 200 + assert resp.text == "app reached" + + async def test_uncovered_path_still_gates(self) -> None: + """A path with no matching rule at all is not exempted by an unrelated one.""" + from simple_module_core.public_routes import PublicRouteRegistry + + registry = PublicRouteRegistry() + registry.add_exact("/api/branding/logo", methods=["GET"]) + app = _build_app(enabled=True, public_routes=registry) + resp = await _get(app, "/api/thing") + assert resp.status_code == 503 + + async def test_wrong_method_on_a_covered_path_still_gates(self) -> None: + """The rule is GET-only — a POST to the same path must not bypass.""" + from simple_module_core.public_routes import PublicRouteRegistry + + registry = PublicRouteRegistry() + registry.add_exact("/api/branding/logo", methods=["GET"]) + app = _build_app(enabled=True, public_routes=registry) + resp = await _post(app, "/api/branding/logo") + assert resp.status_code == 503 + + async def test_no_registry_still_gates(self) -> None: + """A degraded/missing registry must fail closed, same as no provider.""" + resp = await _get(_build_app(enabled=True, public_routes=None), "/api/branding/logo") + assert resp.status_code == 503 + + +class TestInertiaCacheOrdering: + """Pins the exact regression `test_middleware_order.py` guards structurally: + with `InertiaCacheMiddleware` sitting directly outside `MaintenanceMiddleware` + (real pipeline order), the 503's own short-circuited response — not just a + response from `self.app` — must still pass through InertiaCache's + send-wrapper. Built here rather than asserted from order alone because a + correct middleware *order* does not by itself prove a short-circuiting + middleware's response reaches the wrapper outside it; the ASGI `send` + plumbing has to actually carry it, which is what this exercises end to end. + """ + + async def test_maintenance_503_gets_private_no_store_for_an_inertia_request(self) -> None: + app = _build_app(enabled=True, with_inertia_cache=True) + resp = await _get(app, "/protected", headers={"X-Inertia": "true"}) + assert resp.status_code == 503 + assert resp.headers["cache-control"] == "private, no-store" + assert "etag" not in resp.headers + assert "x-inertia" in resp.headers.get("vary", "").lower() + + async def test_maintenance_503_is_not_forced_private_for_a_plain_json_request(self) -> None: + """Only the Inertia representation needs the cache guard — a bare API + caller's 503 keeps whatever caching (none, here) it already had.""" + app = _build_app(enabled=True, with_inertia_cache=True) + resp = await _get(app, "/api/thing") + assert resp.status_code == 503 + assert "cache-control" not in resp.headers + assert "vary" not in resp.headers diff --git a/framework/hosting/tests/test_middleware_order.py b/framework/hosting/tests/test_middleware_order.py index 0410ec7f..096c2675 100644 --- a/framework/hosting/tests/test_middleware_order.py +++ b/framework/hosting/tests/test_middleware_order.py @@ -4,7 +4,7 @@ CorrelationId → RequestLogging → GZip → Security → Session → → Tenant (opt-in) → Locale → InertiaLayoutData - → InertiaCache → CommitBeforeResponse → app + → InertiaCache → Maintenance → CommitBeforeResponse → app InertiaCache sits directly inside InertiaLayoutData, the middleware that puts this user's auth block, permissions and menus into every Inertia payload. Being @@ -12,6 +12,12 @@ read the headers, and pairing the two keeps "what makes the payload per-user" and "what stops the payload being cached" from drifting apart. +Maintenance sits inside InertiaCache rather than outside it. Its 503 is an +Inertia payload carrying the same per-user auth block and menus, and it is +produced by short-circuiting — so if it sat outside, that payload would never +pass through the cache guard and would ship storable, which is the exact bug +InertiaCache exists to prevent. + Tenant/Locale must see ``request.state.user`` set by AuthMiddleware so DB queries get filtered correctly; CorrelationId must wrap everything so every log line carries its id. SiteLock must precede AuthMiddleware: it gates @@ -20,6 +26,12 @@ to be fully hidden. That inversion breaks the feature without failing any site_lock unit test, which is why the order is pinned here. +Maintenance sits after InertiaLayoutData because its 503 page renders +through Inertia and needs the shared props (auth, menus, i18n) — placed any +further out it would render bare, with no layout and untranslated copy. It is +also after Auth, which is what tells it whether the caller is an admin allowed +to pass through and switch it back off. + CommitBeforeResponse is innermost so its send-wrapper is the first to see ``http.response.start`` — that is what makes the request's DB work commit before any byte reaches the client (GH #257). Anything added inside it would @@ -52,6 +64,7 @@ "LocaleMiddleware", "InertiaLayoutDataMiddleware", "InertiaCacheMiddleware", + "MaintenanceMiddleware", "CommitBeforeResponseMiddleware", ) @@ -66,6 +79,7 @@ "LocaleMiddleware", "InertiaLayoutDataMiddleware", "InertiaCacheMiddleware", + "MaintenanceMiddleware", "CommitBeforeResponseMiddleware", ) diff --git a/framework/hosting/tests/test_redirects.py b/framework/hosting/tests/test_redirects.py index e7090cb8..4e6d125c 100644 --- a/framework/hosting/tests/test_redirects.py +++ b/framework/hosting/tests/test_redirects.py @@ -61,6 +61,8 @@ class TestSafeRefererBlocksHostedirects: "//evil.example/x", "//evil.example", r"\\evil.example/x", # backslash-prefixed — some browsers normalize + "/\\evil.example", # relative-looking, but browsers resolve it off-site + "/\\\\evil.example/x", "http://testserver.evil.example/", # suffix-confusion "http://evil.example@testserver/", # userinfo trick: host is "evil.example" "javascript:alert(1)", @@ -127,3 +129,30 @@ def test_fragment_dropped(self) -> None: def test_empty_path_becomes_root(self) -> None: req = _make_request(referer="http://testserver") assert safe_referer_or_root(req) == "/" + + +class TestDelegatesToSafeNext: + """``safe_referer_or_root`` funnels its result through ``safe_next``. + + Two sanitisers for one job is how one of them ends up missing a case: the + inline rules here accepted ``/\\evil.example`` and CR/LF-carrying paths + that ``safe_next`` rejects, so the protection you got depended on which + helper the call site happened to import. + """ + + def test_backslash_prefixed_relative_target_is_rejected(self) -> None: + """``urlsplit`` reports no scheme and no netloc for this, so the + relative branch used to hand it straight back — and browsers resolve + it against ``evil.example``.""" + req = _make_request(referer="/\\evil.example") + assert safe_referer_or_root(req) == "/" + + def test_crlf_in_a_relative_target_is_rejected(self) -> None: + """Otherwise smuggled into the Location header.""" + req = _make_request(referer="/ok\r\nLocation: https://evil.example") + assert safe_referer_or_root(req) == "/" + + def test_ordinary_same_site_path_still_passes(self) -> None: + assert safe_referer_or_root(_make_request(referer="/admin/users/?page=2")) == ( + "/admin/users/?page=2" + ) diff --git a/host/client_app/app.tsx b/host/client_app/app.tsx index 04dbe279..a9ef8c91 100644 --- a/host/client_app/app.tsx +++ b/host/client_app/app.tsx @@ -1,5 +1,6 @@ import { createInertiaApp, router } from '@inertiajs/react'; import { ErrorBoundary } from '@simple-module-py/ui/components/ErrorBoundary'; +import { OfflineBanner } from '@simple-module-py/ui/components/OfflineBanner'; import { formatTitle, setTitleAppName } from '@simple-module-py/ui/lib/app-title'; import { startSpaLinkInterception } from '@simple-module-py/ui/lib/spa-links'; import { useEffect, useRef } from 'react'; @@ -43,6 +44,10 @@ createInertiaApp({ return ( + {/* Outside the page, so connectivity is reported on the error and + auth screens too — losing the network on the login page is when + an unexplained failure is most confusing. */} + ); diff --git a/host/client_app/pages/Admin.tsx b/host/client_app/pages/Admin.tsx new file mode 100644 index 00000000..b83feca5 --- /dev/null +++ b/host/client_app/pages/Admin.tsx @@ -0,0 +1,86 @@ +import { Head, Link, usePage } from '@inertiajs/react'; +import { keys, useT } from '@simple-module-py/i18n'; +import { NavIcon } from '@simple-module-py/ui/components/NavIcon'; +import { PageShell } from '@simple-module-py/ui/components/PageShell'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; +import type { MenuItem, SharedProps } from '@simple-module-py/ui/types'; +import type React from 'react'; + +/** + * Admin section landing page. + * + * Built from the `adminSidebar` shared prop rather than a list of its own: + * that prop is already filtered by the viewer's roles and permissions, so a + * card here can never advertise a screen its owner cannot open — and adding a + * module contributes a card with no change to this file. + */ + +/** Group entries in registration order, so cards match sidebar order. */ +function groupItems(items: MenuItem[]): [string, MenuItem[]][] { + const groups = new Map(); + for (const item of items) { + const key = item.group || ''; + const existing = groups.get(key); + if (existing) existing.push(item); + else groups.set(key, [item]); + } + return [...groups.entries()]; +} + +function ToolCard({ item }: { item: MenuItem }) { + return ( + + + + + + + {item.label} + + {item.url} + + + ); +} + +function AdminPage() { + const { t } = useT(); + const { menus } = usePage<{ props: SharedProps }>().props as unknown as SharedProps; + const items = menus?.adminSidebar ?? []; + const grouped = groupItems(items); + + return ( + <> + + + {items.length === 0 ? ( +

{t(keys.host.admin.empty)}

+ ) : ( +
+ {grouped.map(([group, groupItems_]) => ( +
+ {group && ( +

+ {group} +

+ )} +
+ {groupItems_.map((item) => ( + + ))} +
+
+ ))} +
+ )} + + + ); +} + +AdminPage.layout = (page: React.ReactNode) => {page}; + +export default AdminPage; diff --git a/host/client_app/pages/Error.tsx b/host/client_app/pages/Error.tsx index 927e5b4d..8149f8f8 100644 --- a/host/client_app/pages/Error.tsx +++ b/host/client_app/pages/Error.tsx @@ -3,46 +3,116 @@ import { keys, useT } from '@simple-module-py/i18n'; import { CopyableId } from '@simple-module-py/ui/components/CopyableId'; import { ErrorScreen } from '@simple-module-py/ui/components/ErrorScreen'; import { Button } from '@simple-module-py/ui/components/ui/button'; -import { Home, LifeBuoy } from 'lucide-react'; +import { Home, LifeBuoy, LogIn } from 'lucide-react'; interface Props { status: number; message: string; correlation_id?: string; + /** Provider-specific login URL, when an auth provider is installed. Only + * used by the statuses where signing in is the actual remedy. */ + login_url?: string | null; + /** A planned outage, not an incident — set by MaintenanceMiddleware. */ + maintenance?: boolean; } -function ErrorPage({ status, message, correlation_id }: Props) { +type Accent = 'primary' | 'warning' | 'destructive'; + +interface StatusCopy { + title: string; + description: string; + accent: Accent; +} + +/** One row per status, rather than three parallel Record maps — + * those drift the moment a status is added to one and missed in another. */ +function useStatusCopy(status: number, maintenance: boolean): StatusCopy { const { t } = useT(); + const e = keys.host.error; - const titles: Record = { - 403: t(keys.host.error.forbidden_title), - 404: t(keys.host.error.not_found_title), - 500: t(keys.host.error.server_error_title), - }; + if (maintenance) { + // A planned outage. Same 503, but "we're doing this on purpose and it + // will end" is a different message from "something is broken". + return { + title: t(e.maintenance_title), + description: t(e.maintenance_description), + accent: 'warning', + }; + } - const descriptions: Record = { - 403: t(keys.host.error.forbidden_description), - 404: t(keys.host.error.not_found_description), - 500: t(keys.host.error.server_error_description), + const table: Record = { + 401: { + title: t(e.unauthorized_title), + description: t(e.unauthorized_description), + accent: 'warning', + }, + 403: { + title: t(e.forbidden_title), + description: t(e.forbidden_description), + accent: 'warning', + }, + 404: { + title: t(e.not_found_title), + description: t(e.not_found_description), + accent: 'primary', + }, + 419: { + title: t(e.session_expired_title), + description: t(e.session_expired_description), + accent: 'warning', + }, + 422: { + title: t(e.invalid_request_title), + description: t(e.invalid_request_description), + accent: 'warning', + }, + 429: { + title: t(e.rate_limited_title), + description: t(e.rate_limited_description), + accent: 'warning', + }, + 500: { + title: t(e.server_error_title), + description: t(e.server_error_description), + accent: 'destructive', + }, + 503: { + title: t(e.unavailable_title), + description: t(e.unavailable_description), + accent: 'destructive', + }, }; - const accents: Record = { - 403: 'warning', - 404: 'primary', - 500: 'destructive', - }; + return ( + table[status] ?? { + title: t(e.generic_title), + description: t(e.generic_description), + accent: 'primary', + } + ); +} + +function ErrorPage({ status, message, correlation_id, login_url, maintenance }: Props) { + const { t } = useT(); + const copy = useStatusCopy(status, Boolean(maintenance)); - const title = titles[status] || t(keys.host.error.generic_title); - const description = message || descriptions[status] || t(keys.host.error.generic_description); + // A server-supplied message wins over the canned description — it is the + // specific reason, where the table only knows the status class. + const description = message || copy.description; + // The server already decided this: `login_url` is sent only for the + // statuses in `_SIGN_IN_STATUSES` and is null otherwise. Re-deriving the + // list here would mean editing it in two languages, where missing one + // silently hides the button rather than failing. + const showSignIn = Boolean(login_url); return ( <> - + @@ -58,7 +128,15 @@ function ErrorPage({ status, message, correlation_id }: Props) { ) : undefined } > - + )} + } @@ -196,5 +196,5 @@ function RoleEdit({ role, assigned, groups }: Props) { ); } -RoleEdit.layout = (page: React.ReactNode) => {page}; +RoleEdit.layout = (page: React.ReactNode) => {page}; export default RoleEdit; diff --git a/modules/permissions/permissions/pages/UserEdit.tsx b/modules/permissions/permissions/pages/UserEdit.tsx index 0efaaa58..9bff53a5 100644 --- a/modules/permissions/permissions/pages/UserEdit.tsx +++ b/modules/permissions/permissions/pages/UserEdit.tsx @@ -6,7 +6,7 @@ import { Badge } from '@simple-module-py/ui/components/ui/badge'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; import { Input } from '@simple-module-py/ui/components/ui/input'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import { USERS_ADMIN_PATH } from '@simple-module-py/ui/lib/auth-routes'; import { Check, KeyRound, Link2, Package, Search, ShieldCheck } from 'lucide-react'; import type React from 'react'; @@ -58,7 +58,7 @@ function UserEdit({ user, roles, direct, inherited, inherited_by: inheritedBy, g function handleSubmit(e: React.FormEvent) { e.preventDefault(); - put(`/permissions/users/${user.id}`, { + put(`/admin/permissions/users/${user.id}`, { preserveScroll: true, onSuccess: () => toast.success(t(keys.permissions.toasts.saved)), onError: () => toast.error(t(keys.permissions.toasts.save_failed)), @@ -200,5 +200,5 @@ function UserEdit({ user, roles, direct, inherited, inherited_by: inheritedBy, g ); } -UserEdit.layout = (page: React.ReactNode) => {page}; +UserEdit.layout = (page: React.ReactNode) => {page}; export default UserEdit; diff --git a/modules/settings/settings/constants.py b/modules/settings/settings/constants.py index 69dc9cf1..8a213462 100644 --- a/modules/settings/settings/constants.py +++ b/modules/settings/settings/constants.py @@ -44,7 +44,7 @@ # ── Routing ────────────────────────────────────────────────────────── API_PREFIX: Final = "/api/settings" -VIEW_PREFIX: Final = "/settings" +VIEW_PREFIX: Final = "/admin/settings" VIEW_CREATE_PATH: Final = "/create" VIEW_EDIT_PATH: Final = "/{setting_id}/edit" VIEW_MODULES_PATH: Final = "/modules" diff --git a/modules/settings/settings/endpoints/views.py b/modules/settings/settings/endpoints/views.py index 7f0ebcf8..3509b078 100644 --- a/modules/settings/settings/endpoints/views.py +++ b/modules/settings/settings/endpoints/views.py @@ -38,6 +38,7 @@ VIEW_CREATE_PATH, VIEW_EDIT_PATH, VIEW_MODULES_PATH, + VIEW_PREFIX, VIEW_STORE_PATH, ) from settings.contracts.schemas import SettingCreate, SettingUpdate @@ -50,9 +51,11 @@ _PAGE_MODULES_EDIT = "Settings/ModulesEdit" # Row-level actions return to the raw store they were performed in, not to -# the module forms that now own the section root. -_REDIRECT_SETTINGS = "/settings/store" -_REDIRECT_MODULES = "/settings/" +# the module forms that now own the section root. Built from VIEW_PREFIX so +# they follow the section if it moves again — spelled out, they silently sent +# users to the pre-/admin paths after the move. +_REDIRECT_SETTINGS = f"{VIEW_PREFIX}{VIEW_STORE_PATH}" +_REDIRECT_MODULES = f"{VIEW_PREFIX}/" # Every screen in this section reads configuration: module field values, # their env var names, and now which of the two is in force. The matching JSON diff --git a/modules/settings/settings/module.py b/modules/settings/settings/module.py index c9e64187..45f73dc4 100644 --- a/modules/settings/settings/module.py +++ b/modules/settings/settings/module.py @@ -67,7 +67,7 @@ def register_menu_items(self, registry: MenuRegistry) -> None: url=MENU_URL, icon=MENU_ICON, order=MENU_ORDER, - section=MenuSection.SIDEBAR, + section=MenuSection.ADMIN_SIDEBAR, group="System", # Mirrors the view router's guard, so the entry is not offered # to accounts whose click would 403. diff --git a/modules/settings/settings/pages/Browse.tsx b/modules/settings/settings/pages/Browse.tsx index d11e3944..694983d1 100644 --- a/modules/settings/settings/pages/Browse.tsx +++ b/modules/settings/settings/pages/Browse.tsx @@ -12,7 +12,7 @@ import { TableHeader, TableRow, } from '@simple-module-py/ui/components/ui/table'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import { Box, Plus, Settings as SettingsIcon } from 'lucide-react'; import type React from 'react'; import type { ValueType } from './components/ValueInput'; @@ -153,5 +153,5 @@ function Browse({ settings }: Props) { ); } -Browse.layout = (page: React.ReactNode) => {page}; +Browse.layout = (page: React.ReactNode) => {page}; export default Browse; diff --git a/modules/settings/settings/pages/Create.tsx b/modules/settings/settings/pages/Create.tsx index 3e553961..31d6a334 100644 --- a/modules/settings/settings/pages/Create.tsx +++ b/modules/settings/settings/pages/Create.tsx @@ -13,7 +13,7 @@ import { SelectValue, } from '@simple-module-py/ui/components/ui/select'; import { Textarea } from '@simple-module-py/ui/components/ui/textarea'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import type React from 'react'; import { KeyField, type KnownKey } from './components/KeyField'; import ValueInput, { VALUE_TYPES, type ValueType } from './components/ValueInput'; @@ -160,5 +160,5 @@ function Create({ known_keys }: Props) { ); } -Create.layout = (page: React.ReactNode) => {page}; +Create.layout = (page: React.ReactNode) => {page}; export default Create; diff --git a/modules/settings/settings/pages/Edit.tsx b/modules/settings/settings/pages/Edit.tsx index edbfadae..64a25418 100644 --- a/modules/settings/settings/pages/Edit.tsx +++ b/modules/settings/settings/pages/Edit.tsx @@ -6,7 +6,7 @@ import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; import { Input } from '@simple-module-py/ui/components/ui/input'; import { Label } from '@simple-module-py/ui/components/ui/label'; import { Textarea } from '@simple-module-py/ui/components/ui/textarea'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import type React from 'react'; import ValueInput, { type ValueType } from './components/ValueInput'; import { ROUTES } from './routes'; @@ -128,5 +128,5 @@ function Edit({ setting }: Props) { ); } -Edit.layout = (page: React.ReactNode) => {page}; +Edit.layout = (page: React.ReactNode) => {page}; export default Edit; diff --git a/modules/settings/settings/pages/ModulesEdit.tsx b/modules/settings/settings/pages/ModulesEdit.tsx index 144e1110..09fca6bd 100644 --- a/modules/settings/settings/pages/ModulesEdit.tsx +++ b/modules/settings/settings/pages/ModulesEdit.tsx @@ -2,7 +2,7 @@ import { Head } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; import { Card } from '@simple-module-py/ui/components/ui/card'; import { Input } from '@simple-module-py/ui/components/ui/input'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import { Box, Search } from 'lucide-react'; import type React from 'react'; import { useMemo, useState } from 'react'; @@ -109,5 +109,5 @@ function ModulesEdit({ modules, testable = [] }: Props) { ); } -ModulesEdit.layout = (page: React.ReactNode) => {page}; +ModulesEdit.layout = (page: React.ReactNode) => {page}; export default ModulesEdit; diff --git a/modules/settings/settings/pages/routes.ts b/modules/settings/settings/pages/routes.ts index 1c76cb1e..6b901088 100644 --- a/modules/settings/settings/pages/routes.ts +++ b/modules/settings/settings/pages/routes.ts @@ -1,10 +1,10 @@ export const ROUTES = { /** Per-module forms — the section root, and where "Settings" now lands. */ - modules: '/settings/', + modules: '/admin/settings/', /** Raw key/value store, demoted from the root. */ - browse: '/settings/store', - create: '/settings/create', - edit: (id: number) => `/settings/${id}/edit`, - byId: (id: number) => `/settings/${id}`, - testConnection: (pkg: string) => `/settings/test-connection/${pkg}`, + browse: '/admin/settings/store', + create: '/admin/settings/create', + edit: (id: number) => `/admin/settings/${id}/edit`, + byId: (id: number) => `/admin/settings/${id}`, + testConnection: (pkg: string) => `/admin/settings/test-connection/${pkg}`, } as const; diff --git a/modules/settings/tests/test_module_settings_render.py b/modules/settings/tests/test_module_settings_render.py index 260bfa48..0dcbf3ca 100644 --- a/modules/settings/tests/test_module_settings_render.py +++ b/modules/settings/tests/test_module_settings_render.py @@ -61,7 +61,7 @@ async def test_client_side_visit_does_not_500( authenticated_client: httpx.AsyncClient, ) -> None: """The reported bug: reaching the page by clicking the sidebar link.""" - resp = await authenticated_client.get("/settings/", headers=_INERTIA) + resp = await authenticated_client.get("/admin/settings/", headers=_INERTIA) assert resp.status_code != _SERVER_ERROR assert resp.status_code == _OK @@ -71,7 +71,7 @@ async def test_the_path_survives_as_a_string( app_with_path_setting: FastAPI, authenticated_client: httpx.AsyncClient, ) -> None: - resp = await authenticated_client.get("/settings/", headers=_INERTIA) + resp = await authenticated_client.get("/admin/settings/", headers=_INERTIA) modules = resp.json()["props"]["modules"] demo = next(m for m in modules if m["package"] == "pathdemo") @@ -84,7 +84,7 @@ async def test_full_page_load_still_works( authenticated_client: httpx.AsyncClient, ) -> None: """The path that always worked must keep working.""" - resp = await authenticated_client.get("/settings/") + resp = await authenticated_client.get("/admin/settings/") assert resp.status_code == _OK assert resp.headers["content-type"].startswith("text/html") diff --git a/modules/settings/tests/test_settings_field_sources.py b/modules/settings/tests/test_settings_field_sources.py index 0bcaaca5..e4888fcd 100644 --- a/modules/settings/tests/test_settings_field_sources.py +++ b/modules/settings/tests/test_settings_field_sources.py @@ -53,7 +53,7 @@ def test_db_override_beats_env(self): class TestModulesView: async def test_fields_carry_their_source(self, authenticated_client): - resp = await authenticated_client.get("/settings/", follow_redirects=False) + resp = await authenticated_client.get("/admin/settings/", follow_redirects=False) assert resp.status_code == 200 def test_env_var_presence_is_detected(self, monkeypatch: pytest.MonkeyPatch): @@ -102,18 +102,18 @@ def test_overrides_mark_their_fields(self): class TestTestConnectionEndpoint: async def test_unknown_package_is_a_404(self, authenticated_client): - resp = await authenticated_client.post("/settings/test-connection/nosuchmodule") + resp = await authenticated_client.post("/admin/settings/test-connection/nosuchmodule") assert resp.status_code == 404 async def test_module_without_checks_is_a_404(self, authenticated_client): """Only modules that registered a check can be tested.""" - resp = await authenticated_client.post("/settings/test-connection/settings") + resp = await authenticated_client.post("/admin/settings/test-connection/settings") assert resp.status_code == 404 async def test_failing_check_still_returns_200_with_the_reason(self, authenticated_client): """An admin testing a connection needs to read the failure, not get an error status with the reason buried.""" - resp = await authenticated_client.post("/settings/test-connection/file_storage") + resp = await authenticated_client.post("/admin/settings/test-connection/file_storage") assert resp.status_code == 200, resp.text body = resp.json() assert body["checks"], body diff --git a/modules/settings/tests/test_settings_view_authz.py b/modules/settings/tests/test_settings_view_authz.py index ccac1cbc..71e08332 100644 --- a/modules/settings/tests/test_settings_view_authz.py +++ b/modules/settings/tests/test_settings_view_authz.py @@ -12,7 +12,7 @@ import pytest from simple_module_test.fixtures import forge_session_cookie -_VIEW_ROUTES = ["/settings/", "/settings/store", "/settings/create"] +_VIEW_ROUTES = ["/admin/settings/", "/admin/settings/store", "/admin/settings/create"] @pytest.fixture @@ -50,7 +50,7 @@ async def test_view_routes_reject_a_user_without_settings_view( async def test_the_api_and_the_screen_agree(plain_user_client: httpx.AsyncClient): """Same data, same answer — the gap between them was the bug.""" api = await plain_user_client.get("/api/settings/modules", follow_redirects=False) - view = await plain_user_client.get("/settings/", follow_redirects=False) + view = await plain_user_client.get("/admin/settings/", follow_redirects=False) assert api.status_code in (302, 401, 403) assert view.status_code in (302, 401, 403) diff --git a/modules/site_lock/site_lock/page.py b/modules/site_lock/site_lock/page.py index 6dface1d..877dc104 100644 --- a/modules/site_lock/site_lock/page.py +++ b/modules/site_lock/site_lock/page.py @@ -11,6 +11,8 @@ import importlib.resources from string import Template +from simple_module_core.redirect_safety import safe_next + _TEMPLATE = Template( (importlib.resources.files(__package__) / "templates" / "unlock.html").read_text( encoding="utf-8" @@ -21,21 +23,11 @@ _ERROR_BLOCK = '' -def safe_next(raw: str | None) -> str: - """Return ``raw`` if it is a same-site absolute path, else ``/``. - - Rejects protocol-relative (``//host``) and backslash-prefixed (``/\\host``) - targets — browsers resolve both off-site — plus anything carrying CR/LF, - which could otherwise be smuggled into the redirect header. Without this - the gate would be an open redirect. - """ - if not raw or not raw.startswith("/"): - return "/" - if raw.startswith(("//", "/\\")): - return "/" - if "\r" in raw or "\n" in raw: - return "/" - return raw +# ``safe_next`` is imported, not defined here: the implementation moved to +# ``simple_module_core.redirect_safety`` once AuthMiddleware needed the same +# rules. Re-exported so existing ``site_lock.page.safe_next`` callers keep +# working. +__all__ = ["render_unlock_page", "safe_next"] def render_unlock_page( diff --git a/modules/users/tests/test_users_login_deep_link.py b/modules/users/tests/test_users_login_deep_link.py new file mode 100644 index 00000000..914efa22 --- /dev/null +++ b/modules/users/tests/test_users_login_deep_link.py @@ -0,0 +1,117 @@ +"""An anonymous visit to a protected page should come back after login. + +AuthMiddleware stashes the target in the session; the login view surfaces it +as ``login_redirect_url``. Before this existed the target was dropped and +every login landed on the configured default. + +Filename is prefixed with the module name on purpose: no tests/ directory +here has an __init__.py, so test module basenames share one global namespace. +""" + +from __future__ import annotations + +import pytest + + +class TestDeepLinkAfterLogin: + """An anonymous visit to a protected page should come back after login. + + AuthMiddleware stashes the target in the session; the login view surfaces + it as ``login_redirect_url``. Before this existed the target was dropped + and every login landed on the configured default. + """ + + @pytest.mark.anyio + async def test_bounced_target_becomes_the_redirect_prop(self, anon_client): + bounced = await anon_client.get("/admin/settings/", follow_redirects=False) + assert bounced.status_code == 302 + + resp = await anon_client.get( + "/users/login", + headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, + ) + assert resp.json()["props"]["login_redirect_url"] == "/admin/settings/" + + @pytest.mark.anyio + async def test_query_string_is_preserved(self, anon_client): + await anon_client.get("/admin/settings/?tab=modules", follow_redirects=False) + + resp = await anon_client.get( + "/users/login", + headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, + ) + assert resp.json()["props"]["login_redirect_url"] == "/admin/settings/?tab=modules" + + @pytest.mark.anyio + async def test_reload_of_login_page_keeps_the_target(self, anon_client): + """Read-not-pop: reloading the login page must not lose the deep link.""" + await anon_client.get("/admin/settings/", follow_redirects=False) + + for _ in range(2): + resp = await anon_client.get( + "/users/login", + headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, + ) + assert resp.json()["props"]["login_redirect_url"] == "/admin/settings/" + + @pytest.mark.anyio + async def test_without_a_bounce_the_default_is_used(self, anon_client): + resp = await anon_client.get( + "/users/login", + headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, + ) + assert resp.json()["props"]["login_redirect_url"] == "/dashboard/" + + +class TestEveryProviderConsumesTheStashedTarget: + """All three completion paths must honour ``SESSION_NEXT_KEY`` and clear it. + + ``AuthMiddleware`` writes the key for every bounced request, whichever + provider the visitor eventually picks. A path that never reads it silently + drops the deep link; a path that reads without popping leaves a stale + target to fire on some later, unrelated visit to the login page. The OAuth + callback did neither, so signing in with Google lost the deep link that the + password and Keycloak paths kept — the exact per-module drift the shared + key exists to prevent. + + Asserted against the source because these are three different flows in + three packages, two of which need a live identity provider to exercise + end to end; the thing worth pinning is that none of them forgets. + """ + + @staticmethod + def _source(relative: str) -> str: + from pathlib import Path + + root = Path(__file__).resolve().parents[3] + return (root / relative).read_text(encoding="utf-8") + + @pytest.mark.parametrize( + "relative", + [ + "modules/users/users/oauth/api.py", + "modules/users/users/auth_local/api.py", + "modules/keycloak/keycloak/endpoints/api.py", + ], + ) + def test_login_completion_paths_clear_the_stashed_target(self, relative: str) -> None: + source = self._source(relative) + assert "SESSION_NEXT_KEY" in source, ( + f"{relative} completes a login without touching the shared post-login destination key" + ) + assert "pop(SESSION_NEXT_KEY" in source, ( + f"{relative} must pop SESSION_NEXT_KEY once login succeeds, or a " + "stale deep link fires on a later visit to the login page" + ) + + @pytest.mark.parametrize( + "relative", + [ + "modules/users/users/oauth/api.py", + "modules/keycloak/keycloak/endpoints/api.py", + ], + ) + def test_redirecting_providers_sanitise_the_target(self, relative: str) -> None: + """The value lands in a Location header, so it is re-checked on the way + out even though AuthMiddleware already validated it going in.""" + assert "safe_next_or_none" in self._source(relative) diff --git a/modules/users/tests/test_views.py b/modules/users/tests/test_views.py index 0594e33d..8cb4bdec 100644 --- a/modules/users/tests/test_views.py +++ b/modules/users/tests/test_views.py @@ -177,19 +177,19 @@ class TestAdminIndexPage: @pytest.mark.anyio async def test_admin_without_auth_is_redirected(self, anon_client): """Unauthenticated access to the admin page redirects to /users/login.""" - resp = await anon_client.get("/users/admin", follow_redirects=False) + resp = await anon_client.get("/admin/users", follow_redirects=False) assert resp.status_code == 302 assert resp.headers["location"].endswith("/users/login") @pytest.mark.anyio async def test_admin_with_admin_session_returns_200(self, admin_client): - resp = await admin_client.get("/users/admin") + resp = await admin_client.get("/admin/users/") assert resp.status_code == 200 @pytest.mark.anyio async def test_admin_inertia_component_is_users_index(self, admin_client): resp = await admin_client.get( - "/users/admin", + "/admin/users/", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, ) assert resp.status_code == 200 @@ -200,26 +200,26 @@ async def test_admin_inertia_component_is_users_index(self, admin_client): class TestAdminEditPage: @pytest.mark.anyio async def test_invalid_uuid_returns_404(self, admin_client): - resp = await admin_client.get("/users/admin/not-a-uuid") + resp = await admin_client.get("/admin/users/not-a-uuid") assert resp.status_code == 404 @pytest.mark.anyio async def test_unknown_uuid_returns_404(self, admin_client): missing_id = str(uuid.uuid4()) - resp = await admin_client.get(f"/users/admin/{missing_id}") + resp = await admin_client.get(f"/admin/users/{missing_id}") assert resp.status_code == 404 @pytest.mark.anyio async def test_existing_user_returns_200(self, admin_client, users_db): user = await _make_verified_user(users_db, email="edit_target@example.com") - resp = await admin_client.get(f"/users/admin/{user.id}") + resp = await admin_client.get(f"/admin/users/{user.id}") assert resp.status_code == 200 @pytest.mark.anyio async def test_existing_user_inertia_component(self, admin_client, users_db): user = await _make_verified_user(users_db, email="edit_target2@example.com") resp = await admin_client.get( - f"/users/admin/{user.id}", + f"/admin/users/{user.id}", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, ) assert resp.status_code == 200 @@ -232,7 +232,7 @@ async def test_admin_edit_page_unknown_user_returns_404(admin_client): import uuid resp = await admin_client.get( - f"/users/admin/{uuid.uuid4()}", + f"/admin/users/{uuid.uuid4()}", follow_redirects=False, ) assert resp.status_code == 404 diff --git a/modules/users/tests/test_views_admin.py b/modules/users/tests/test_views_admin.py index 58d18fb5..bb5466b1 100644 --- a/modules/users/tests/test_views_admin.py +++ b/modules/users/tests/test_views_admin.py @@ -20,7 +20,7 @@ async def test_status_filter_in_view(self, admin_client, users_db): await users_db.commit() resp = await admin_client.get( - "/users/admin?status=disabled", + "/admin/users/?status=disabled", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 @@ -35,7 +35,7 @@ async def test_status_filter_in_view(self, admin_client, users_db): @pytest.mark.anyio async def test_filters_defaults_in_props(self, admin_client): resp = await admin_client.get( - "/users/admin", + "/admin/users/", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 @@ -50,7 +50,7 @@ async def test_filters_defaults_in_props(self, admin_client): @pytest.mark.anyio async def test_invalid_filter_values_are_ignored(self, admin_client): resp = await admin_client.get( - "/users/admin?status=bad&sort=invalid&order=sideways", + "/admin/users/?status=bad&sort=invalid&order=sideways", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 @@ -71,7 +71,7 @@ async def test_page_past_the_end_clamps_to_last_page(self, admin_client, users_d await _make_user(users_db, email="clamp-b@x.com") resp = await admin_client.get( - "/users/admin?page=999&per_page=1", + "/admin/users/?page=999&per_page=1", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 @@ -90,7 +90,7 @@ async def test_pagination_prop_echoes_clamped_values(self, admin_client, users_d await _make_user(users_db, email="clamp-c@x.com") resp = await admin_client.get( - "/users/admin?page=0&per_page=1000", + "/admin/users/?page=0&per_page=1000", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 @@ -131,7 +131,7 @@ async def test_flag_true_when_permissions_installed(self, admin_client, users_ap ) try: resp = await admin_client.get( - f"/users/admin/{user.id}", + f"/admin/users/{user.id}", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, ) finally: @@ -156,7 +156,7 @@ async def test_flag_false_when_not_installed(self, admin_client, users_app, user ) try: resp = await admin_client.get( - f"/users/admin/{user.id}", + f"/admin/users/{user.id}", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, ) finally: @@ -176,7 +176,7 @@ class TestAdminAddPeoplePage: @pytest.mark.anyio async def test_add_page_renders_with_roles(self, admin_client): resp = await admin_client.get( - "/users/admin/add", + "/admin/users/add", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 @@ -188,7 +188,7 @@ async def test_add_page_renders_with_roles(self, admin_client): async def test_add_page_reports_whether_mail_can_be_delivered(self, admin_client): """Drives the copy-link panel — the page has to know before submitting.""" resp = await admin_client.get( - "/users/admin/add", + "/admin/users/add", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert "mailer_delivers" in resp.json()["props"] @@ -201,7 +201,7 @@ async def test_no_mailer_does_not_promise_delivery(self, admin_client, app): app.state.users.mailer = None try: resp = await admin_client.get( - "/users/admin/add", + "/admin/users/add", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.json()["props"]["mailer_delivers"] is False @@ -210,22 +210,22 @@ async def test_no_mailer_does_not_promise_delivery(self, admin_client, app): @pytest.mark.anyio async def test_add_page_requires_auth(self, anon_client): - resp = await anon_client.get("/users/admin/add", follow_redirects=False) + resp = await anon_client.get("/admin/users/add", follow_redirects=False) assert resp.status_code == 302 @pytest.mark.anyio @pytest.mark.parametrize( ("old_path", "mode"), - [("/users/admin/create", "create"), ("/users/admin/invite", "invite")], + [("/admin/users/create", "create"), ("/admin/users/invite", "invite")], ) async def test_old_urls_redirect_into_the_right_mode(self, admin_client, old_path, mode): """Existing links must land on the merged form with their mode preselected.""" resp = await admin_client.get(old_path, follow_redirects=False) assert resp.status_code == 307 - assert resp.headers["location"] == f"/users/admin/add?mode={mode}" + assert resp.headers["location"] == f"/admin/users/add?mode={mode}" @pytest.mark.anyio - @pytest.mark.parametrize("old_path", ["/users/admin/create", "/users/admin/invite"]) + @pytest.mark.parametrize("old_path", ["/admin/users/create", "/admin/users/invite"]) async def test_old_urls_require_auth(self, anon_client, old_path): """The legacy aliases must stay gated behind the same permission as the page they redirect to — an anonymous caller must not reach the diff --git a/modules/users/users/admin/components/RolesTab.tsx b/modules/users/users/admin/components/RolesTab.tsx index 6c4f30dc..95970a3c 100644 --- a/modules/users/users/admin/components/RolesTab.tsx +++ b/modules/users/users/admin/components/RolesTab.tsx @@ -61,7 +61,7 @@ export function RolesTab({ roles }: { roles: RoleItem[] }) { diff --git a/modules/users/users/admin/components/UserRow.tsx b/modules/users/users/admin/components/UserRow.tsx index c74ae03d..ff7980f6 100644 --- a/modules/users/users/admin/components/UserRow.tsx +++ b/modules/users/users/admin/components/UserRow.tsx @@ -78,8 +78,11 @@ export function UserRow({ user }: { user: UserListItem }) { diff --git a/modules/users/users/admin/components/UsersEmpty.tsx b/modules/users/users/admin/components/UsersEmpty.tsx index 979538ea..cb604fef 100644 --- a/modules/users/users/admin/components/UsersEmpty.tsx +++ b/modules/users/users/admin/components/UsersEmpty.tsx @@ -5,7 +5,7 @@ import { TableEmptyRow } from '@simple-module-py/ui/components/TableEmptyRow'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { Plus, UserPlus, Users } from 'lucide-react'; -const ADD_PEOPLE_URL = '/users/admin/add'; +const ADD_PEOPLE_URL = '/admin/users/add'; function AddPeopleAction() { const { t } = useT(); diff --git a/modules/users/users/admin/views.py b/modules/users/users/admin/views.py index 8da0d24b..cdea9141 100644 --- a/modules/users/users/admin/views.py +++ b/modules/users/users/admin/views.py @@ -30,7 +30,7 @@ async def _roles_payload(app) -> list[dict[str, str]]: @router.get( - "/admin", + "/", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) @@ -96,7 +96,7 @@ async def admin_index( @router.get( - "/admin/add", + "/add", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) @@ -126,27 +126,27 @@ async def admin_add_people_page( @router.get( - "/admin/invite", + "/invite", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) async def admin_invite_redirect() -> RedirectResponse: - """Old invite URL — the flow merged into /users/admin/add.""" - return RedirectResponse("/users/admin/add?mode=invite", status_code=307) + """Old invite URL — the flow merged into /admin/users/add.""" + return RedirectResponse("/admin/users/add?mode=invite", status_code=307) @router.get( - "/admin/create", + "/create", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) async def admin_create_redirect() -> RedirectResponse: - """Old create URL — the flow merged into /users/admin/add.""" - return RedirectResponse("/users/admin/add?mode=create", status_code=307) + """Old create URL — the flow merged into /admin/users/add.""" + return RedirectResponse("/admin/users/add?mode=create", status_code=307) @router.get( - "/admin/{user_id}", + "/{user_id}", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) diff --git a/modules/users/users/auth_local/api.py b/modules/users/users/auth_local/api.py index 209b4e99..a2f26b96 100644 --- a/modules/users/users/auth_local/api.py +++ b/modules/users/users/auth_local/api.py @@ -15,6 +15,7 @@ from fastapi import APIRouter, Depends, HTTPException, Request, Response, status from fastapi.security import OAuth2PasswordRequestForm from fastapi_users import exceptions as fu_exceptions +from simple_module_core.redirect_safety import SESSION_NEXT_KEY from users.auth_local.rate_limit import LoginRateLimiter, ThroughputLimiter from users.constants import SESSION_USER_ID_KEY @@ -111,6 +112,9 @@ async def login( login_response = await auth_backend.login(strategy, user) # Bridge the session cookie — AuthMiddleware reads this to identify the user request.session[SESSION_USER_ID_KEY] = str(user.id) + # The deep link has served its purpose; leaving it would send the *next* + # plain visit to /users/login off to a stale destination. + request.session.pop(SESSION_NEXT_KEY, None) return login_response @@ -166,6 +170,10 @@ async def accept_invite( await user_manager.on_after_login(user, request, response) login_response = await auth_backend.login(strategy, user) request.session[SESSION_USER_ID_KEY] = str(user.id) + # Invite acceptance routes the user itself, so a deep link stashed by an + # earlier bounce is stale here — drop it rather than leave it to fire on + # some later visit to the login page. + request.session.pop(SESSION_NEXT_KEY, None) return login_response diff --git a/modules/users/users/auth_local/views.py b/modules/users/users/auth_local/views.py index cf694526..90ea19b4 100644 --- a/modules/users/users/auth_local/views.py +++ b/modules/users/users/auth_local/views.py @@ -4,6 +4,7 @@ from fastapi import APIRouter, Depends, HTTPException, Request from inertia import InertiaResponse +from simple_module_core.redirect_safety import SESSION_NEXT_KEY, safe_next_or_none from simple_module_hosting.inertia_deps import InertiaDep from starlette.responses import RedirectResponse @@ -55,7 +56,14 @@ async def login_page(request: Request, inertia: InertiaDep) -> InertiaResponse: { "allow_signup": users_settings.allow_signup, "dev_accounts": dev_accounts, - "login_redirect_url": users_settings.login_redirect_url, + # Where AuthMiddleware bounced them from, when it bounced them. + # Read, not popped: a reload of the login page must not silently + # downgrade the deep link to the default landing page. The POST + # handler clears it once login actually succeeds. + "login_redirect_url": ( + safe_next_or_none(request.session.get(SESSION_NEXT_KEY)) + or users_settings.login_redirect_url + ), "oauth_providers": users_state.oauth_providers, }, ) diff --git a/modules/users/users/module.py b/modules/users/users/module.py index 94fbd1c4..4d3ea77a 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -29,7 +29,7 @@ _MODULE_DEPENDENCY_SETTINGS = "Settings" # Menu URLs -_URL_USERS_ADMIN = "/users/admin" +_URL_USERS_ADMIN = "/admin/users/" _URL_USERS_ME = "/users/me" _URL_USERS_LOGOUT = "/users/logout" @@ -44,6 +44,10 @@ class UsersModule(ModuleBase): name="Users", route_prefix="/api/users", view_prefix="/users", + # Sign-in and self-service stay on /users; the management CRUD + # belongs with the other admin screens. One view_prefix cannot + # express both, hence the second router. + admin_view_prefix="/admin/users", depends_on=[_MODULE_DEPENDENCY_AUTH, _MODULE_DEPENDENCY_SETTINGS], ) _is_auth_provider = True @@ -116,7 +120,7 @@ def register_audit_links(self, registry: AuditLinkRegistry) -> None: # The model class name — what snapshot_changes records. Keying # this off __tablename__ ("users_user") silently never matches. entity_type=User.__name__, - url_template=f"{_URL_USERS_ADMIN}/{{id}}", + url_template=f"{_URL_USERS_ADMIN}{{id}}", label="User", ) ) @@ -129,9 +133,9 @@ def register_menu_items(self, registry: MenuRegistry) -> None: url=_URL_USERS_ADMIN, icon=_ICON_USERS, order=100, - section=MenuSection.SIDEBAR, + section=MenuSection.ADMIN_SIDEBAR, roles=[ADMIN_ROLE_NAME], - group="Administration", + group="Access", ) ) # Self-service: profile + logout live in the user dropdown. @@ -161,7 +165,6 @@ def locale_dirs(self) -> dict[str, Path]: def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: from users.admin.api import admin_router - from users.admin.views import router as admin_views from users.auth_local import api as auth_local_api from users.auth_local.token_api import router as token_router from users.auth_local.views import router as auth_views @@ -198,7 +201,11 @@ def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None register_oauth_routes(api_router) view_router.include_router(auth_views) - view_router.include_router(admin_views) + + def register_admin_routes(self, admin_router: APIRouter) -> None: + from users.admin.views import router as admin_views + + admin_router.include_router(admin_views) async def on_startup(self, app: FastAPI) -> None: """Build the mailer, rate limiter, and apply production cookie params.""" diff --git a/modules/users/users/oauth/api.py b/modules/users/users/oauth/api.py index a86cf3b5..cc475c98 100644 --- a/modules/users/users/oauth/api.py +++ b/modules/users/users/oauth/api.py @@ -9,7 +9,8 @@ Why a custom handler rather than ``fastapi_users.get_oauth_router``: the stock ``/callback`` returns 204; Inertia needs the browser to land on a real page, so -``/callback`` returns a 303 redirect to ``login_redirect_url`` with the auth +``/callback`` returns a 303 redirect to the stashed deep link (or +``login_redirect_url``) with the auth cookie attached. Find-or-create + email-association go through ``UserManager.oauth_callback``. State CSRF uses Starlette's signed session cookie. @@ -22,6 +23,7 @@ from fastapi import APIRouter, Depends, HTTPException, Request, status from fastapi_users import exceptions as fu_exceptions +from simple_module_core.redirect_safety import SESSION_NEXT_KEY, safe_next_or_none from starlette.responses import RedirectResponse from users.constants import OAUTH_REGISTRATION_REQUEST_FLAG @@ -117,7 +119,15 @@ async def callback( login_response = await auth_backend.login(strategy, user) await user_manager.on_after_login(user, request, login_response) - redirect_url = request.app.state.users.settings.login_redirect_url + # Honour the deep link AuthMiddleware stashed before bouncing this + # visitor to login, exactly as the password and Keycloak paths do. + # Popped, not read: leaving it would send some later, unrelated visit + # to /users/login off to a stale destination. Re-sanitised on the way + # out because the value lands in a Location header. + redirect_url = ( + safe_next_or_none(request.session.pop(SESSION_NEXT_KEY, None)) + or request.app.state.users.settings.login_redirect_url + ) redirect = RedirectResponse(redirect_url, status_code=303) for key, value in login_response.headers.items(): if key.lower() == "set-cookie": diff --git a/modules/users/users/pages/Login.tsx b/modules/users/users/pages/Login.tsx index 975fb44b..ad76cee1 100644 --- a/modules/users/users/pages/Login.tsx +++ b/modules/users/users/pages/Login.tsx @@ -36,10 +36,11 @@ function Login() { const [needsVerification, setNeedsVerification] = useState(false); const [loading, setLoading] = useState(false); - const nextUrl = - typeof window !== 'undefined' - ? new URLSearchParams(window.location.search).get('next') || login_redirect_url - : login_redirect_url; + // Server-decided, deliberately. The post-login destination used to be read + // from `?next=` here, which let any crafted login link bounce the user to an + // arbitrary URL after signing in. AuthMiddleware now stashes the target in + // the session and the view sanitises it, so this prop is already safe. + const nextUrl = login_redirect_url; const submitLogin = (username: string, pwd: string) => { setError(null); diff --git a/modules/users/users/pages/Users/AddPeople.tsx b/modules/users/users/pages/Users/AddPeople.tsx index 4816d85e..c178715c 100644 --- a/modules/users/users/pages/Users/AddPeople.tsx +++ b/modules/users/users/pages/Users/AddPeople.tsx @@ -2,7 +2,7 @@ import { Link, router, usePage } from '@inertiajs/react'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import type React from 'react'; import { useState } from 'react'; import { toast } from 'sonner'; @@ -19,7 +19,7 @@ interface Props { mailer_delivers: boolean; } -const USERS_INDEX = '/users/admin'; +const USERS_INDEX = '/admin/users/'; function initialMode(): Mode { if (typeof window === 'undefined') return 'invite'; @@ -208,5 +208,5 @@ function AddPeople() { ); } -AddPeople.layout = (page: React.ReactNode) => {page}; +AddPeople.layout = (page: React.ReactNode) => {page}; export default AddPeople; diff --git a/modules/users/users/pages/Users/Edit.tsx b/modules/users/users/pages/Users/Edit.tsx index cd8d1786..d6d168a1 100644 --- a/modules/users/users/pages/Users/Edit.tsx +++ b/modules/users/users/pages/Users/Edit.tsx @@ -1,7 +1,7 @@ import { Link, router, usePage } from '@inertiajs/react'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; import { Button } from '@simple-module-py/ui/components/ui/button'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import type React from 'react'; import { useEffect, useMemo, useRef, useState } from 'react'; import { toast } from 'sonner'; @@ -231,7 +231,7 @@ function Edit() {
{dirty && Unsaved changes} {/* Back to what is persisted, not to what the page loaded with — discarding must not visually undo a section that already saved. */} @@ -291,5 +291,5 @@ function Edit() { ); } -Edit.layout = (page: React.ReactNode) => {page}; +Edit.layout = (page: React.ReactNode) => {page}; export default Edit; diff --git a/modules/users/users/pages/Users/Index.tsx b/modules/users/users/pages/Users/Index.tsx index 89b71ca9..eb30391b 100644 --- a/modules/users/users/pages/Users/Index.tsx +++ b/modules/users/users/pages/Users/Index.tsx @@ -13,7 +13,7 @@ import { TableRow, } from '@simple-module-py/ui/components/ui/table'; import { Tabs, TabsContent, TabsList, TabsTrigger } from '@simple-module-py/ui/components/ui/tabs'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import { ArrowDown, ArrowUp, @@ -105,7 +105,7 @@ function Index() { if (sort !== 'email') params.sort = sort; if (order !== 'asc') params.order = order; if (page > 1) params.page = String(page); - router.get('/users/admin', params, { preserveState: true, preserveScroll: true }); + router.get('/admin/users/', params, { preserveState: true, preserveScroll: true }); }, [search, filters], ); @@ -153,7 +153,7 @@ function Index() { // One entry point: invite-vs-create is a choice inside the form, not // a choice between two buttons made before seeing either.
))} + {menuKey === 'sidebar' && ( + + )} {footerNavSlot} @@ -248,7 +275,7 @@ function SidebarShell({ children, menuKey, theme, headerSlot, footerNavSlot }: S {/* Main content */}
`` it +leaves alone and *appends* beside. The root template shipped a plain +````, so every page ended up with two title elements — the static brand +name first, Inertia's page-specific one second — and the browser takes the +first in document order. Every tab read as the bare app name, and no +``<Head title>`` anywhere in the app had any effect. + +Nothing else catches this. The title renders, the page renders, every +role-and-name assertion still passes; only reading ``document.title`` shows it. +Asserting the element *count* matters as much as the text: a second, unmanaged +title reintroduces the bug the moment one is added back to the template. +""" + +from __future__ import annotations + +import pytest +from playwright.sync_api import Page, expect + +pytestmark = pytest.mark.e2e + +# (path, the page-specific fragment its title must carry) +_TITLED_PAGES = [ + ("/dashboard/", "Dashboard"), + ("/admin", "Administration"), +] + + +def _login(page: Page, username: str, password: str) -> None: + page.goto("/") + page.get_by_role("link", name="Log in").first.click() + page.locator("#email").fill(username) + page.locator("#password").fill(password) + page.get_by_role("button", name="Log in").click() + page.wait_for_url("**/dashboard/**", timeout=15_000) + + +def test_login_page_title_names_the_page(page: Page) -> None: + """Checked signed-out too: the sign-in page is the first tab a visitor + sees, and it is served by the same template.""" + page.goto("/users/login") + expect(page).to_have_title("Login — SimpleModule", timeout=10_000) + + +def test_signed_out_page_has_exactly_one_title_element(page: Page) -> None: + """Two titles is the actual defect — the text assertions only fail + because of the ordering it produces.""" + page.goto("/users/login") + expect(page).to_have_title("Login — SimpleModule", timeout=10_000) + assert page.locator("title").count() == 1 + + +@pytest.mark.parametrize(("path", "fragment"), _TITLED_PAGES) +def test_page_title_names_the_page( + page: Page, e2e_username: str, e2e_password: str, path: str, fragment: str +) -> None: + _login(page, e2e_username, e2e_password) + page.goto(path) + # Waits for hydration: the server-rendered title is the bare app name until + # the head manager commits, so reading straight after goto races it. + expect(page).to_have_title(f"{fragment} — SimpleModule", timeout=10_000) + assert page.locator("title").count() == 1 + + +def test_error_page_title_names_the_status( + page: Page, e2e_username: str, e2e_password: str +) -> None: + """Signed in on purpose: to an anonymous visitor an unknown path is an + auth bounce, not a 404, so this would assert against the login page.""" + _login(page, e2e_username, e2e_password) + page.goto("/no-such-page-anywhere") + expect(page).to_have_title("Page Not Found — SimpleModule", timeout=10_000) diff --git a/tests/e2e/test_i18n_rendering.py b/tests/e2e/test_i18n_rendering.py index e12b78e9..867dcdc7 100644 --- a/tests/e2e/test_i18n_rendering.py +++ b/tests/e2e/test_i18n_rendering.py @@ -46,12 +46,12 @@ _PAGES = [ ("Dashboard", "/dashboard/"), ("Files", "/file-storage/"), - ("Users", "/users/admin"), - ("Feature Flags", "/feature_flags/"), - ("Branding", "/branding/"), + ("Users", "/admin/users/"), + ("Feature Flags", "/admin/feature-flags/"), + ("Branding", "/admin/branding/"), ("Background Tasks", "/admin/background-tasks/"), - ("Settings", "/settings/"), - ("Audit Log", "/audit_log/"), + ("Settings", "/admin/settings/"), + ("Audit Log", "/admin/audit-log/"), ] diff --git a/tests/e2e/test_settings_ui.py b/tests/e2e/test_settings_ui.py index a93e553b..ec506a1f 100644 --- a/tests/e2e/test_settings_ui.py +++ b/tests/e2e/test_settings_ui.py @@ -1,6 +1,6 @@ """E2E smoke test for the Settings modules admin UI. -Drives a real browser through the sidebar layout at ``/settings/modules``, +Drives a real browser through the sidebar layout at ``/admin/settings/``, toggles a module setting, and verifies the change hot-reloads into ``app.state`` without a server restart by exercising a downstream endpoint whose behaviour flips when the setting flips. @@ -22,7 +22,7 @@ def _login(page: Page, username: str, password: str) -> None: page.locator("#password").fill(password) page.get_by_role("button", name="Log in").click() # Wait for the session cookie to land before navigating away, or - # /settings/modules bounces us back to login and the sidebar never renders. + # /admin/settings/ bounces us back to login and the sidebar never renders. page.wait_for_url("**/dashboard/**", timeout=15_000) @@ -35,7 +35,7 @@ def test_toggle_host_multi_tenant_persists( page.goto("/") _login(page, e2e_username, e2e_password) - page.goto("/settings/modules") + page.goto("/admin/settings/") expect(page.get_by_text("Host", exact=False)).to_be_visible() # Click the Host entry in the sidebar. diff --git a/tests/e2e/test_shell_ui.py b/tests/e2e/test_shell_ui.py index 084b37f3..f3a81f92 100644 --- a/tests/e2e/test_shell_ui.py +++ b/tests/e2e/test_shell_ui.py @@ -29,7 +29,7 @@ def test_breadcrumb_names_the_section_on_sub_pages( page.goto("/") _login(page, e2e_username, e2e_password) - page.goto("/users/admin/add") + page.goto("/admin/users/add") crumb = page.get_by_role("navigation", name="breadcrumb") expect(crumb.get_by_role("link", name="Users")).to_be_visible() expect(crumb.get_by_text("Add people")).to_be_visible() @@ -46,7 +46,7 @@ def test_command_palette_opens_filters_and_navigates( expect(palette).to_be_visible() palette.fill("Audit") page.keyboard.press("Enter") - page.wait_for_url("**/audit_log**", timeout=10_000) + page.wait_for_url("**/admin/audit-log**", timeout=10_000) # Reopen and close with Escape — no navigation this time. page.keyboard.press("Control+k") @@ -78,5 +78,5 @@ def test_user_search_treats_like_metacharacters_literally( page.goto("/") _login(page, e2e_username, e2e_password) - page.goto("/users/admin?q=_") + page.goto("/admin/users/?q=_") expect(page.get_by_text("No users match these filters")).to_be_visible() diff --git a/tests/loadtest/locustfile.py b/tests/loadtest/locustfile.py index cb7a8168..231a1da7 100644 --- a/tests/loadtest/locustfile.py +++ b/tests/loadtest/locustfile.py @@ -56,7 +56,7 @@ def users_list_api(self) -> None: def users_list_view(self) -> None: page = random.randint(1, 50) self.client.get( - f"/users/admin?page={page}&per_page=20", headers=_INERTIA, name="/users/admin" + f"/admin/users/?page={page}&per_page=20", headers=_INERTIA, name="/admin/users/" ) @task(8) diff --git a/tests/perf/test_asset_integrity.py b/tests/perf/test_asset_integrity.py index 99b3335f..7a737bb9 100644 --- a/tests/perf/test_asset_integrity.py +++ b/tests/perf/test_asset_integrity.py @@ -23,7 +23,7 @@ pytestmark = [pytest.mark.perf, pytest.mark.e2e] -ROUTES = ("/users/login", "/dashboard/", "/audit_log/", "/users/admin") +ROUTES = ("/users/login", "/dashboard/", "/admin/audit-log/", "/admin/users/") _SETTLE_MS = 1500 _CLIENT_ERROR = 400 # Chrome reports a module served as text/html this way; it is the signature of @@ -100,7 +100,7 @@ def test_lazy_page_chunks_resolve_under_the_static_prefix( ) # A route whose page component is a lazily-imported chunk. - page.goto(f"{base_url}/audit_log/", wait_until="load") + page.goto(f"{base_url}/admin/audit-log/", wait_until="load") page.wait_for_timeout(_SETTLE_MS) stray = [u for u in js_urls if u.startswith("/assets/")] diff --git a/tests/perf/test_nav_perf.py b/tests/perf/test_nav_perf.py index 017a910b..71dee527 100644 --- a/tests/perf/test_nav_perf.py +++ b/tests/perf/test_nav_perf.py @@ -29,8 +29,8 @@ # enforces that no menu URL redirects. ROUTES = ( ("dashboard", "/dashboard/"), - ("users_admin", "/users/admin"), - ("audit_log", "/audit_log/"), + ("users_admin", "/admin/users/"), + ("audit_log", "/admin/audit-log/"), ) @@ -111,7 +111,7 @@ def _on_response(response) -> None: page.on("response", _on_response) try: - measure_navigation(page, lambda: _click_sidebar(page, "/audit_log/"), "audit_log") + measure_navigation(page, lambda: _click_sidebar(page, "/admin/audit-log/"), "audit_log") finally: page.remove_listener("response", _on_response) diff --git a/tests/perf/test_page_load.py b/tests/perf/test_page_load.py index 8da53922..0eadad86 100644 --- a/tests/perf/test_page_load.py +++ b/tests/perf/test_page_load.py @@ -20,7 +20,7 @@ pytestmark = [pytest.mark.perf, pytest.mark.e2e] -ROUTES = ("/audit_log/", "/dashboard/") +ROUTES = ("/admin/audit-log/", "/dashboard/") # Compression must cut total transfer by at least this much. The observed # reduction is ~70%; 40% leaves generous headroom for bundle changes while # still failing loudly if compression silently stops being applied. diff --git a/tests/perf/test_perceived.py b/tests/perf/test_perceived.py index 07b6dd04..911136a3 100644 --- a/tests/perf/test_perceived.py +++ b/tests/perf/test_perceived.py @@ -33,8 +33,8 @@ ROUTES = ( ("dashboard", "/dashboard/"), - ("users_admin", "/users/admin"), - ("audit_log", "/audit_log/"), + ("users_admin", "/admin/users/"), + ("audit_log", "/admin/audit-log/"), ) _SETTLE_MS = 1200 # A 500px block inserted at the top of the body shifts essentially the whole diff --git a/tests/test_audit_log.py b/tests/test_audit_log.py index 4adbcb08..6ab358b5 100644 --- a/tests/test_audit_log.py +++ b/tests/test_audit_log.py @@ -201,7 +201,7 @@ async def test_invalid_pagination_returns_html( ): """View endpoint should clamp bad pagination values, never 422.""" resp = await authenticated_client.get( - "/audit_log/", + "/admin/audit-log/", params=params, follow_redirects=False, ) diff --git a/tests/test_principal_resolver_integration.py b/tests/test_principal_resolver_integration.py index 38ab337d..2ffb2c1a 100644 --- a/tests/test_principal_resolver_integration.py +++ b/tests/test_principal_resolver_integration.py @@ -48,11 +48,11 @@ async def pat_client(app_with_pat_resolver) -> AsyncGenerator[httpx.AsyncClient, async def test_bearer_token_authenticates_against_protected_view(pat_client): """Valid bearer token -> 200 on a protected view path (users admin).""" resp = await pat_client.get( - "/users/admin", + "/admin/users/", headers={"Authorization": "Bearer good"}, follow_redirects=False, ) - # /users/admin is a view route; with a valid resolver the request gets + # /admin/users/ is a view route; with a valid resolver the request gets # through AuthMiddleware (200) instead of redirecting to /users/login. assert resp.status_code == 200