diff --git a/CLAUDE.md b/CLAUDE.md index 5c65cb19..8a4ebbb5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,7 +76,7 @@ cascade layer is inert, while unlayered CSS beats every Tailwind utility — hence `SM022`/`SM023`. See `docs/module-authoring.md` § Styling. **Lifecycle hooks** (in `framework/core/simple_module_core/module.py`) — all no-op by default; subclasses override as needed: -`register_settings` → `register_menu_items` / `register_permissions` / `register_feature_flags` / `register_event_handlers` / `register_invalidations` / `register_health_checks` / `register_public_routes` / `register_csp_sources` / `register_setup_steps` → `register_exception_handlers` → `register_middleware` → `register_routes(api_router, view_router)` / `register_admin_routes(admin_router)` → async `on_startup` / `on_shutdown` (reverse order). `register_admin_routes` is only for modules that serve **both** public and admin pages: a module gets exactly one router per `view_prefix`, which `users` cannot express (sign-in at `/users/login`, management at `/admin/users`). Setting `ModuleMeta.admin_view_prefix` mounts a second view router there. A module whose views are *all* administrative just points `view_prefix` at `/admin/` and keeps using `register_routes`. The prefix is a URL convention, not a permission — guard these routes exactly as you would any other. `register_csp_sources(registry)` lets a module whitelist external asset origins (`registry.add("style-src", "https://rsms.me")`) — fetch directives only, validated at boot. `register_public_routes(registry)` lets a module exempt anonymous/read-only routes (STAC/OGC, webhooks) from `AuthMiddleware`; rules are method-aware (`registry.add_regex(r"…/tilejson$", methods={"GET"})`), so a GET read route can be public while sibling POST/PATCH mutations under the same prefix stay gated. See [docs/framework/public-routes.md](docs/framework/public-routes.md). `register_setup_steps(registry)` lets a module declare what a usable install still needs; while any required step is incomplete `SetupMiddleware` serves the first-run wizard at `/setup` instead of the app. The wizard ships with `simple_module_hosting` (`setup_wizard/` — routes mounted by `create_app`, page `Setup/Wizard` registered by `gen-pages`), so hosts carry no setup code; a step completes from the browser through its `SetupAction`, which runs only while *that* step is pending. A module that registers nothing never gates — that is how `keycloak` opts out, since its local users table is legitimately empty forever and a host-level superuser count would lock those installs out permanently. `register_invalidations(bus, app)` subscribes a module's **per-process caches** to `InvalidationBus`, so another worker's write drops this worker's entry instead of leaving it stale for its whole TTL; handlers may only *forget*, since there is no delivery guarantee. Publishing takes no hook — `await request.app.state.sm.invalidation.publish(channel, key=...)` from a `db.on_commit` callback. Cross-process delivery needs a transport, which `background_tasks` installs on its Redis connection (`SM_BG_TASKS_BROADCAST_INVALIDATIONS`); with none the bus is in-process and every cache still needs its TTL as a floor. See [docs/framework/invalidation.md](docs/framework/invalidation.md). +`register_settings` → `register_menu_items` / `register_permissions` / `register_feature_flags` / `register_event_handlers` / `register_invalidations` / `register_health_checks` / `register_public_routes` / `register_csp_sources` / `register_setup_steps` → `register_exception_handlers` → `register_middleware` → `register_routes(api_router, view_router)` / `register_admin_routes(admin_router)` → async `on_startup` / `on_shutdown` (reverse order). `register_admin_routes` is only for modules that serve **both** public and admin pages: a module gets exactly one router per `view_prefix`, which `users` cannot express (sign-in at `/users/login`, management at `/admin/users`). Setting `ModuleMeta.admin_view_prefix` mounts a second view router there. A module whose views are *all* administrative just points `view_prefix` at `/admin/` and keeps using `register_routes`. The prefix is a URL convention, not a permission — guard these routes exactly as you would any other. `register_csp_sources(registry)` lets a module whitelist external asset origins (`registry.add("style-src", "https://rsms.me")`) — fetch directives only, validated at boot; an origin read from DB-backed settings uses `registry.add_provider(fn)`, evaluated per response. `register_public_routes(registry)` lets a module exempt anonymous/read-only routes (STAC/OGC, webhooks) from `AuthMiddleware`; rules are method-aware (`registry.add_regex(r"…/tilejson$", methods={"GET"})`), so a GET read route can be public while sibling POST/PATCH mutations under the same prefix stay gated. See [docs/framework/public-routes.md](docs/framework/public-routes.md). `register_setup_steps(registry)` lets a module declare what a usable install still needs; while any required step is incomplete `SetupMiddleware` serves the first-run wizard at `/setup` instead of the app. The wizard ships with `simple_module_hosting` (`setup_wizard/` — routes mounted by `create_app`, page `Setup/Wizard` registered by `gen-pages`), so hosts carry no setup code; a step completes from the browser through its `SetupAction`, which runs only while *that* step is pending. A module that registers nothing never gates — that is how `keycloak` opts out, since its local users table is legitimately empty forever and a host-level superuser count would lock those installs out permanently. `register_invalidations(bus, app)` subscribes a module's **per-process caches** to `InvalidationBus`, so another worker's write drops this worker's entry instead of leaving it stale for its whole TTL; handlers may only *forget*, since there is no delivery guarantee. Publishing takes no hook — `await request.app.state.sm.invalidation.publish(channel, key=...)` from a `db.on_commit` callback. Cross-process delivery needs a transport, which `background_tasks` installs on its Redis connection (`SM_BG_TASKS_BROADCAST_INVALIDATIONS`); with none the bus is in-process and every cache still needs its TTL as a floor. See [docs/framework/invalidation.md](docs/framework/invalidation.md). `MenuRegistry.add_provider(fn)` (from `register_menu_items`) contributes per-request menu items evaluated in `InertiaLayoutDataMiddleware` after auth/tenant resolution, and `PermissionRegistry.add_source(name, provider)` (from `register_permissions`) contributes runtime-defined permissions from a sync in-memory cache, refreshed with `invalidate_source(name)`; see [docs/framework/permissions.md](docs/framework/permissions.md). diff --git a/Makefile b/Makefile index 22296852..ee8f1dc1 100644 --- a/Makefile +++ b/Makefile @@ -28,7 +28,7 @@ gen-pages: uv run --project host smpy host gen-pages --host-dir=host/client_app # Regenerate packages/i18n/src/{generated-resources,keys.generated}.ts from every -# installed module's locales/, plus host/locales and packages/ui/locales — without +# installed module's locales/, plus host/locales and the hosting package's ui_locales — without # booting the app. Pass flags with ARGS, e.g. `make gen-i18n ARGS=--allow-removals`. gen-i18n: uv run --project host python scripts/gen_i18n.py $(ARGS) diff --git a/docs/framework-conventions.md b/docs/framework-conventions.md index cc8c7b4f..5a6e3a6f 100644 --- a/docs/framework-conventions.md +++ b/docs/framework-conventions.md @@ -374,7 +374,7 @@ way. Menus are translated **on the server**, in `MenuRegistry.get_for_user(translate=…)`, so the Inertia payload carries finished text and every render site (sidebar, topbar, command palette) keeps reading `item.label`. Two consequences worth knowing: an admin-audience module's labels don't need to be in the anonymous catalog snapshot to render, and a key that resolves to nothing falls back to `label` — a missing translation degrades to English, never to a raw dotted key on screen. Both fields are optional, so modules written before them keep working unchanged. - Group headers are shared across modules, so they live in the `ui` namespace (`ui.nav_groups.access|appearance|content|system`) rather than each module inventing its own key — otherwise one module's "System" could translate differently from another's and split a single header in two. Those four are the whole vocabulary the bundled modules use; adding a fifth means adding the key to `packages/ui/locales/*.json` so every module can share it. + Group headers are shared across modules, so they live in the `ui` namespace (`ui.nav_groups.access|appearance|content|system`) rather than each module inventing its own key — otherwise one module's "System" could translate differently from another's and split a single header in two. Those four are the whole vocabulary the bundled modules use; adding a fifth means adding the key to `simple_module_hosting/ui_locales/*.json` so every module can share it. - `i18n` — active locale and translation bundle. The framework does not know the shape of `auth.user`. The `auth` module registers a `principal_serializer: Callable[[UserContext], dict]` on `app.state.principal_serializer` during `register_settings(app)`; the middleware calls it with `request.state.user` to build the `auth.user` payload. Without a registered serializer, `auth.user` is `None` even when a user is authenticated. @@ -676,7 +676,7 @@ Do NOT declare `const schema = z.object({ ... t('...') })` at module scope — i ### Host and shared-package strings - Host strings (landing page, error page) live in `host/locales/` and are namespaced `host.*`. -- Shared UI strings (`packages/ui/`) live in `packages/ui/locales/`, namespaced `ui.*`. +- Shared UI strings (`packages/ui/`) live in `simple_module_hosting/ui_locales/`, namespaced `ui.*`. - Both are auto-discovered at boot alongside module contributions. ### Strings the server composes diff --git a/docs/framework/i18n.md b/docs/framework/i18n.md index 46e148b1..90636b65 100644 --- a/docs/framework/i18n.md +++ b/docs/framework/i18n.md @@ -165,7 +165,7 @@ Module-scope `t(...)` calls capture the first-render locale and never update. ## Host and shared-package strings - **Host strings** (landing page, error page): `host/locales/.json`, namespace `host.*`. -- **Shared UI strings** (packages/ui): `packages/ui/locales/.json`, namespace `ui.*`. +- **Shared UI strings** (packages/ui): `simple_module_hosting/ui_locales/.json`, namespace `ui.*`. Both are auto-discovered alongside module contributions — no manual wiring. diff --git a/docs/framework/lifecycle.md b/docs/framework/lifecycle.md index 73b411bc..7e7b7ca0 100644 --- a/docs/framework/lifecycle.md +++ b/docs/framework/lifecycle.md @@ -179,6 +179,22 @@ can be extended — never `default-src`, `base-uri`, `form-action`, or single origin/scheme token; invalid declarations raise at boot. The origins land in both the development (Vite-widened) and production policies. +The hook runs before DB-backed settings are hydrated, so an origin that comes +from a setting can't be a static `add()`. Register a provider instead — it is +called on every response with the app and returns `(directive, source)` pairs: + +```python +def register_csp_sources(self, registry) -> None: + registry.add_provider(lambda app: [("img-src", app.state.maps.settings.tile_origin)]) +``` + +Providers must be synchronous and read only in-memory state +(`app.state..settings`), never the database. Their pairs go through the +same validation as `add()`; a provider that raises, or an invalid pair, is +logged once and dropped — the static policy still applies. The finished policy +is cached by the pairs returned, so a settings edit takes effect on the next +response. With no providers the header is the precomputed boot-time string. + ## `register_exception_handlers(app)` Register FastAPI exception handlers scoped to your module's exceptions: diff --git a/docs/framework/permissions.md b/docs/framework/permissions.md index 8a078416..a75372f9 100644 --- a/docs/framework/permissions.md +++ b/docs/framework/permissions.md @@ -96,9 +96,10 @@ class UserContext: name: str roles: list[str] = field(default_factory=list) tenant_id: str | None = None + permissions: list[str] = field(default_factory=list) ``` -The active auth provider (the `users` or `keycloak` module) owns extraction from the session cookie / bearer token. `InertiaLayoutDataMiddleware` then expands roles into a permission set (cached on `request.state.resolved_permissions`) and serializes `auth.user`, `auth.isAuthenticated`, `auth.permissions` into the Inertia shared props. `UserContext` carries roles, not permissions — permissions are derived from the `PermissionRegistry.role_map`. +The active auth provider (the `users` or `keycloak` module) owns extraction from the session cookie / bearer token. `InertiaLayoutDataMiddleware` then expands roles into a permission set (cached on `request.state.resolved_permissions`) and serializes `auth.user`, `auth.isAuthenticated`, `auth.permissions` into the Inertia shared props. Permissions are derived mostly from roles via the `PermissionRegistry.role_map`. `UserContext.permissions` adds keys the principal carries itself — a token scope or IdP claim a provider or principal resolver maps in. `resolve_principal_permissions` (and so `RequiresPermission`, `require_permission` and `auth.permissions`) merges them into the role-derived set, **dropping `*`**: like a grant source, a claim can never confer the wildcard; admin comes from roles only. They are written into the session cookie with `roles` and frozen there until the next sign-in, so anything an admin must be able to revoke belongs in a grant source, not here. `UserContext.has_permission(key)` is an exact-match check against that list alone. ## Role shorthand for menus diff --git a/docs/module-authoring.md b/docs/module-authoring.md index 27932bc9..38b92303 100644 --- a/docs/module-authoring.md +++ b/docs/module-authoring.md @@ -568,7 +568,22 @@ def register_csp_sources(self, registry): ``` Only fetch directives can be extended; each source must be a single -origin/scheme token, validated at boot. See +origin/scheme token, validated at boot. + +An origin that comes from a DB-backed setting (a configurable tile server) +can't be added at boot — the hook runs before settings hydrate. Register a +provider, evaluated per response: + +```python +def register_csp_sources(self, registry): + registry.add_provider(lambda app: [("img-src", app.state.maps.settings.tile_origin)]) +``` + +Providers must be synchronous and read in-memory state only +(`app.state..settings`); a raising provider or an invalid pair is logged +once and dropped, leaving the static policy in force. Sources must be +printable ASCII, as CSP's own grammar requires, so an internationalised domain +is written in its punycode form (`https://xn--exmple-cua.com`). See [docs/framework/lifecycle.md](framework/lifecycle.md#register_csp_sourcesregistry). ## First-run setup steps diff --git a/docs/modules/auth.md b/docs/modules/auth.md index 0a91d22e..f2159d36 100644 --- a/docs/modules/auth.md +++ b/docs/modules/auth.md @@ -28,8 +28,9 @@ from auth.contracts import UserContext, AuthProvider | `name` | `str` | display name — `full_name or email` of the user | | `roles` | `list[str]` | role names, e.g. `["admin"]` | | `tenant_id` | `str \| None` | only set when multi-tenant | +| `permissions` | `list[str]` | keys the principal carries itself (token scope / IdP claim), merged into the role-derived set by the resolver with `*` dropped; empty from `from_user`. Frozen in the session cookie like `roles` — revocable grants belong in a [grant source](/framework/permissions) | -Helpers: `has_role(role)` / `has_any_role(roles)` for in-handler role checks; `from_user(user)` builds a context from a duck-typed user object (`.id`, `.email`, `.full_name`, `.roles[*].name`, `.tenant_id`); `to_session_dict()` / `from_session_dict()` (de)serialise it into the signed session cookie. +Helpers: `has_role(role)` / `has_any_role(roles)` for in-handler role checks; `has_permission(key)` (exact match against `permissions` only — gate routes with `require_permission`, which also folds in roles and grant sources); `from_user(user)` builds a context from a duck-typed user object (`.id`, `.email`, `.full_name`, `.roles[*].name`, `.tenant_id`); `to_session_dict()` / `from_session_dict()` (de)serialise it into the signed session cookie (a cookie without `permissions` decodes to `[]`). `AuthProvider` is the `Protocol` an auth-provider module implements (`resolve_user`, `get_login_url`, `get_logout_url`, `get_public_paths`, `is_bearer_request`) — see [Pluggable auth](#pluggable-auth) below. diff --git a/docs/modules/file_storage.md b/docs/modules/file_storage.md index b09f86b6..1907e89a 100644 --- a/docs/modules/file_storage.md +++ b/docs/modules/file_storage.md @@ -201,6 +201,7 @@ DB-backed via `register_module_settings`; pydantic defaults seed at boot. Live e | `s3_access_key_id` | `""` | optional — falls back to env / IAM if empty | | `s3_secret_access_key` | `""` | optional | | `s3_endpoint_url` | `""` | optional — set for MinIO / Cloudflare R2 | +| `s3_signature_version` | `"s3v4"` | presign signature version; pinned so custom endpoints get SigV4 | | `s3_presign_ttl_seconds` | `300` | TTL for presigned `GET` URLs | | `max_file_size_bytes` | `100 * 1024 * 1024` | upload limit (100 MB) | | `allowed_content_types` | `None` | optional MIME-type whitelist; `None` ⇒ any | diff --git a/docs/superpowers/plans/2026-10-11-issue-batch-426-437.md b/docs/superpowers/plans/2026-10-11-issue-batch-426-437.md new file mode 100644 index 00000000..c46f7db9 --- /dev/null +++ b/docs/superpowers/plans/2026-10-11-issue-batch-426-437.md @@ -0,0 +1,28 @@ +# Issue batch #426–#437 — implementation plan + +Design: `docs/superpowers/specs/2026-10-11-issue-batch-426-437-design.md` (approved). + +Every task: write the failing test first, implement, run the touched suites +(`uv run pytest ` / `npx vitest run `), keep files under 300 lines. +Tasks touch disjoint files and run in parallel; implementers do **not** commit — +the lead commits each task by path once its tests pass. + +| Task | Issues | Files (primary) | Hard? | +|---|---|---|---| +| T1 | #426 | `framework/hosting/simple_module_hosting/_i18n_build.py`, `ui_locales/` (moved from `packages/ui/locales/`), comments in `Makefile`, `simple_module_core/__main__.py`, `diagnostics/_i18n.py`, `docs/framework-conventions.md`, `framework/hosting/tests/` | no | +| T2 | #427, #428 | `packages/ui/src/{types.ts,lib/home-url.ts,lib/brand.ts,layouts/{AdminLayout,SidebarLayout,PublicLayout}.tsx,components/BrandingFooter*.tsx}`, `modules/branding/branding/components/BrandingPreview.tsx`, users shared-props provider + test | no | +| T3 | #429, #431 | `framework/cli/simple_module_cli/templates/host/client_app/app.tsx`, `templates/module/tests/test___PACKAGE__.py.tpl`, `framework/cli/tests/` | no | +| T4 | #430, #433 | `modules/settings/settings/pages/ModulesEdit.tsx`, `modules/settings/tests-js/ModulesEdit.test.tsx`, `modules/settings/settings/cli.py`, `modules/settings/tests/test_cli_import.py`, `.env.example` | no | +| T5 | #432 | `modules/file_storage/file_storage/{settings.py,backends/s3.py}`, `modules/file_storage/tests/test_s3_backend.py`, file_storage locales if settings labels are translated | no | +| T6 | #434 | `framework/core/simple_module_core/csp.py`, `framework/hosting/simple_module_hosting/{middleware.py,_phase_helpers.py}`, CSP tests, `docs/framework/lifecycle.md`, `docs/module-authoring.md` § External asset origins | **yes** | +| T7 | #435 | `framework/hosting/simple_module_hosting/_module_routes.py` (+ wherever `app.state.sm` is created), `modules/users/users/startup.py`, `modules/dashboard/dashboard/stats.py`, their tests | no | +| T8 | #436 | `modules/auth/auth/contracts/schemas.py`, `framework/hosting/simple_module_hosting/permissions.py`, `modules/auth/tests/test_user_context.py`, `framework/hosting/tests/test_grant_sources.py` (or new), `docs/framework/permissions.md` | **yes** | +| T9 | #437 | `modules/users/users/auth_local/components/LoginForm.tsx` + new vitest | no | + +Done-check per task: the issue's repro is covered by a test that failed before +and passes after; touched suites green; `uv run ruff check`/`ruff format --check` +on touched Python, `npx biome check` on touched TS. + +After all tasks: `make lint`, `make test`, `make doctor`, regenerate i18n keys +(boot the host in development or the repo's gen script) and confirm +`packages/i18n/src/keys.generated.ts` only changes where expected. diff --git a/docs/superpowers/specs/2026-10-11-issue-batch-426-437-design.md b/docs/superpowers/specs/2026-10-11-issue-batch-426-437-design.md new file mode 100644 index 00000000..a5cb5d8f --- /dev/null +++ b/docs/superpowers/specs/2026-10-11-issue-batch-426-437-design.md @@ -0,0 +1,192 @@ +# Issue batch #426–#437 — design + +**Date:** 2026-10-11 · **Status:** approved · **Branch:** `feat/issue-batch-426-437` · **One combined PR** + +Twelve issues filed on 2026-10-11 from building Kuri on 0.0.35, fixed together. +#317 stays out (needs the external design deck). + +## Scope + +| # | Area | One-line fix | +|---|---|---| +| 426 | i18n | `ui` catalog ships inside `simple_module_hosting`, registered unconditionally | +| 427 | ui | Layout "home" links come from a `homeUrl` shared prop, not `/dashboard/` | +| 428 | ui | Default footer caption drops the framework's licence | +| 429 | scaffold | Template `app.tsx` calls `initTheme()` and wires the `title` callback | +| 430 | settings | Picker hides zero-field modules (unless they have `manage_url`) and opens on the first shown | +| 431 | scaffold | Generated module test uses `effective_route_paths` | +| 432 | file_storage | `s3_signature_version` setting, default `s3v4`, passed as botocore `Config` | +| 433 | settings | `import-from-env` reads `.env` under `os.environ` | +| 434 | core/hosting | CSP providers evaluated per response, so origins can follow DB settings | +| 435 | users/dashboard | "Module has views" comes from the routes actually mounted, so a root-mounted module counts | +| 436 | auth | `UserContext.permissions`, round-tripped through the session and merged by the resolver | +| 437 | users | Sign-in error gets `role="alert"` | + +## Design + +### #426 — ship the `ui` catalog + +`_i18n_build.py` registers `ui` only when `/packages/ui/locales` +exists, which is true only inside this monorepo. `hosting` already ships its own +catalog as package data (`simple_module_hosting/locales/`). + +**Move** `packages/ui/locales/{en,es}.json` to +`framework/hosting/simple_module_hosting/ui_locales/` (a sibling directory; the +flat `locales/` is taken by the `hosting` namespace) and always register `ui` +from there. One source of truth, shipped in the wheel by the existing +`packages = ["simple_module_hosting"]`, no copy to keep in sync and no +force-include that breaks sdist builds. Apps that want different wording already +have the #415 host override layer. Update the three code comments that name the +old path (`Makefile`, `simple_module_core/__main__.py`, `diagnostics/_i18n.py`) +plus the i18n diagnostics if they scan that path, and the docs +(`framework-conventions.md`). + +Rejected: copy + byte-identical sync test (two sources), hatch `force-include` +from `../../packages/ui/locales` (breaks building from the sdist). + +### #427 — `homeUrl` shared prop + +The users module already owns the landing URL (`login_redirect_url`, rewritten +at startup by `apply_login_redirect_fallback` when Dashboard is absent) and +already registers an Inertia shared-props provider. It adds +`homeUrl: ` there. `packages/ui` gets a `useHomeUrl()` hook +returning `props.homeUrl ?? '/'`, used by `AdminLayout`, `SidebarLayout` (logo) +and `PublicLayout`. `SharedProps` gains `homeUrl?: string`. + +Fallback is `/`, not `/dashboard/`: `/` is a host route in every app, so the +link can never 404 when the users module is absent. + +### #428 — no licence in the default footer + +Default caption becomes `© {year} {appName}`. `BRAND_LICENSE` is removed (its +only consumers are `BrandingFooter` and `BrandingPreview`); a host wanting a +licence line sets `footer_text`. Tests that expect `MIT` are updated. + +Rejected: a `license` branding setting — a free-text `footer_text` already covers it. + +### #429 — scaffold `app.tsx` + +The template gains what the in-repo `host/client_app/app.tsx` does for theme and +title: `title: (t) => formatTitle(t)`, `setTitleAppName(initialPage.props.branding?.appName)` +and `initTheme()` before the first render. `ErrorBoundary`/`OfflineBanner` parity +is out of scope. `test_cli_new_regressions.py` asserts the three calls. + +### #430 — settings picker + +Client-side in `ModulesEdit.tsx`: `visible = modules.filter(m => m.fields.length > 0 || m.manage_url)`; +search filters `visible`; initial selection is `visible[0]`. The JSON API is +unchanged. A module with no fields but a `manage_url` stays, because its card is +the way to its own settings page. + +### #431 — scaffold module test + +Template imports `effective_route_paths` from `simple_module_test` and asserts +on its result. `test_scaffolding_module.py` asserts the rendered test uses it. + +### #432 — SigV4 + +`FileStorageSettings.s3_signature_version: str = "s3v4"` (group S3, admin-editable +like the other S3 fields). `_build` in `backends/s3.py` passes +`config=botocore.config.Config(signature_version=...)`, imported lazily because +botocore only exists with the `s3` extra. Test: presigned URL contains +`X-Amz-Algorithm=AWS4-HMAC-SHA256` with a custom endpoint. + +### #433 — `.env` in `import-from-env` + +`import_from_env_impl(app, store, environ=None)` reads from +`{**parse_dotenv(find_env_file()), **os.environ}` by default — the real +environment wins, as with pydantic-settings — using the framework's existing +`simple_module_core.dotenv` helpers (no new dependency). No mutation of +`os.environ`. When nothing was imported and a `.env` exists, the command prints a +hint naming the expected `SM__` form. The `.env.example` colour +lines get a "quote values containing `#`" note. + +### #434 — CSP sources that follow settings + +`CspSourceRegistry.add_provider(fn)` where `fn(app) -> Iterable[tuple[str, str]]` +returns `(directive, source)` pairs, registered from the existing +`register_csp_sources` hook (no signature change). `SecurityHeadersMiddleware` +keeps the static policy string and, when providers exist, per response: + +1. calls each provider (sync, must only read in-memory state such as + `app.state..settings`); a provider that raises, or returns a pair that + fails the same directive/source validation `add()` applies, is logged once + and contributes nothing (fail closed — the static policy still applies); +2. looks the resulting `frozenset` of pairs up in a small dict cache of finished + policy strings and builds it with `extend_policy` on a miss. + +No version key or TTL is needed: the cache is keyed on the *value* providers +return, so a runtime settings edit (already hydrated per process by the settings +module and the `settings.values` invalidation channel) changes the key on the +next response. With no providers the middleware is byte-for-byte today's path. +Cache bounded (e.g. 32 entries, clear on overflow). + +Rejected: recompute on `SettingsReloaded` + invalidation subscriptions (more +wiring, a missed event leaves a stale policy); passing `app` to the hook (still +runs before DB hydration, so it fixes nothing). + +### #435 — root-mounted modules + +`view_prefix=""` is both the default ("no views") and "mounted at `/`", so the +issue's `is not None` would match every API-only module. Instead +`wire_module_routes` records each module whose view router actually received +routes in `app.state.sm.view_prefixes: dict[str, str]` (module name → prefix, +discovery order). `apply_login_redirect_fallback` and `dashboard/stats.py` use +that map: the first non-users module in it wins, and `""` yields `/`. The admin +URL in stats stays as it is (an admin prefix is never empty). + +Rejected: a `register_routes`-override check (an API-only module that overrides it +would be taken as root-mounted); `view_prefix: str | None` (breaking change for +every module). + +### #436 — `UserContext.permissions` + +`permissions: list[str] = field(default_factory=list)` appended after `tenant_id` +(positional order preserved), written by `to_session_dict` and read with +`payload.get("permissions") or []` so old cookies stay valid. Add +`has_permission(p)`. `resolve_principal_permissions` merges +`getattr(user, "permissions", None) or ()` after the role step, **dropping `*`** +— the same guarantee #398 gives grant sources, so a token or IdP claim can never +carry the wildcard. Duck-typed, so SM009 holds. `from_user` leaves it empty +(direct user grants keep coming from the permissions module's grant source). +Documented: these permissions are frozen in the session like `roles`; anything +revocable belongs in a grant source. + +### #437 — announce the sign-in error + +`role="alert"` on the `LoginForm` error paragraph, plus a vitest asserting +`getByRole('alert')` after a failed submit. + +## Decisions made (override at approval) + +1. #426 moves the catalog instead of copying it; `packages/ui/locales` is deleted. +2. #427 prop name `homeUrl`, owned by users, client fallback `/`. +3. #428 drops the licence entirely rather than adding a setting. +4. #432 is a setting (as the issue suggests), not a hard-coded value. +5. #434 is per-response providers with a value-keyed cache, not event-driven recompute. +6. #435 uses the mounted-routes map instead of a `ModuleMeta` change. +7. #436 filters `*` out of `UserContext.permissions`. +8. #429 adds theme + title only, not ErrorBoundary/OfflineBanner. + +## Testing + +- Python: `_i18n_build` registers `ui` with no `packages/ui` under the root; + users shared props expose `homeUrl`; S3 presign is SigV4; `import_from_env_impl` + reads a tmp `.env` and env wins over it; CSP provider adds/changes/fails closed + and the static path is unchanged; root-mounted fake module wins the login + fallback and gets `/` in dashboard stats while an API-only module does not; + `UserContext` round-trip with and without `permissions`, resolver merges them + and drops `*`; scaffold template content checks (#429, #431). +- JS: `useHomeUrl` in the three layouts, footer caption, settings picker + hide/initial selection, LoginForm alert. +- QA in the browser: `/admin/users/` back link, footer caption, `/admin/settings/` + picker, failed sign-in. + +## Risks + +- #426: the generated i18n keys/resources read the registry, so moving the + catalog must keep `keys.generated.ts` identical — checked by regenerating. +- #434: the per-response provider call is on the hot path; providers must stay + attribute reads. The no-provider path is unchanged, so only apps using it pay. +- #435: the test harness in `simple_module_test` also calls `wire_module_routes`, + so `app.state.sm` must exist there too. diff --git a/framework/cli/simple_module_cli/templates/host/client_app/app.tsx b/framework/cli/simple_module_cli/templates/host/client_app/app.tsx index 2bd27d6e..5af66a8b 100644 --- a/framework/cli/simple_module_cli/templates/host/client_app/app.tsx +++ b/framework/cli/simple_module_cli/templates/host/client_app/app.tsx @@ -1,6 +1,8 @@ import { createInertiaApp, router } from '@inertiajs/react'; import { configureI18n, updateI18n } from '@simple-module-py/i18n'; +import { formatTitle, setTitleAppName } from '@simple-module-py/ui/lib/app-title'; import { startSpaLinkInterception } from '@simple-module-py/ui/lib/spa-links'; +import { initTheme } from '@simple-module-py/ui/lib/theme'; import { createRoot } from 'react-dom/client'; import { resolvePage } from './pages'; @@ -11,11 +13,19 @@ type I18nBlock = { }; createInertiaApp({ + title: (title) => formatTitle(title), resolve: async (name) => { return await resolvePage(name); }, setup({ el, App, props }) { - const initial = (props.initialPage.props as { i18n?: I18nBlock }).i18n; + const pageProps = props.initialPage.props as { + i18n?: I18nBlock; + branding?: { appName?: string | null }; + }; + setTitleAppName(pageProps.branding?.appName); + // Before the first render, so a dark-theme user never sees a light frame. + initTheme(); + const initial = pageProps.i18n; configureI18n({ locale: initial?.locale ?? 'en', messages: initial?.messages ?? {}, diff --git a/framework/cli/simple_module_cli/templates/module/tests/test___PACKAGE__.py.tpl b/framework/cli/simple_module_cli/templates/module/tests/test___PACKAGE__.py.tpl index ec3e5e6c..4d8b5d59 100644 --- a/framework/cli/simple_module_cli/templates/module/tests/test___PACKAGE__.py.tpl +++ b/framework/cli/simple_module_cli/templates/module/tests/test___PACKAGE__.py.tpl @@ -8,6 +8,7 @@ that package is installed). No conftest.py is required. from __future__ import annotations from {{PACKAGE_NAME}}.module import {{MODULE_NAME}}Module +from simple_module_test import effective_route_paths class TestMeta: @@ -22,6 +23,8 @@ class TestRoutes: async def test_app_boots_with_module(self, build_test_app): """Module registers cleanly into a minimal FastAPI host.""" app = build_test_app({{MODULE_NAME}}Module) - paths = {getattr(r, "path", None) for r in app.routes} + # effective_route_paths reads the OpenAPI schema: FastAPI >= 0.137 + # includes routers lazily, so ``app.routes`` no longer lists them. + paths = effective_route_paths(app) # The placeholder GET / endpoint lives under the route_prefix. - assert any(p and p.startswith("/api/{{MODULE_SLUG}}") for p in paths) + assert any(p.startswith("/api/{{MODULE_SLUG}}") for p in paths) diff --git a/framework/cli/tests/test_cli_new_regressions.py b/framework/cli/tests/test_cli_new_regressions.py index f4047791..b99a94f2 100644 --- a/framework/cli/tests/test_cli_new_regressions.py +++ b/framework/cli/tests/test_cli_new_regressions.py @@ -230,6 +230,19 @@ def test_scaffold_adopts_every_locale_catalog_the_server_sends(tmp_path: Path) - ) +def test_sm_new_host_app_tsx_wires_theme_and_title(tmp_path: Path) -> None: + """Issue #429: the scaffolded app.tsx must init the theme and brand titles.""" + from simple_module_cli.scaffolding import create_host + + dest = tmp_path / "demo" + create_host(dest, name="demo-host", modules=["Dashboard", "Auth"]) + + app_tsx = (dest / "client_app" / "app.tsx").read_text() + assert "initTheme(" in app_tsx + assert "formatTitle" in app_tsx + assert "setTitleAppName" in app_tsx + + def test_sm_new_no_install_next_steps_include_initial_migration(tmp_path: Path) -> None: """Issue #135: ``host/migrations/versions/`` ships empty, so a fresh ``make migrate`` is a no-op. The printed next-steps must therefore diff --git a/framework/cli/tests/test_scaffolding_module.py b/framework/cli/tests/test_scaffolding_module.py index 9a356bd6..546d991a 100644 --- a/framework/cli/tests/test_scaffolding_module.py +++ b/framework/cli/tests/test_scaffolding_module.py @@ -30,6 +30,16 @@ async def test_creates_expected_module_files(self, tmp_path): # pytest try to register `tests.conftest` as a plugin twice. assert not (dest / "tests" / "__init__.py").exists() + async def test_generated_test_uses_effective_route_paths(self, tmp_path): + """#431: app.routes hides lazily-included routers on FastAPI >= 0.137.""" + from simple_module_cli.scaffolding import create_module + + dest = tmp_path / "simple-module-my-feature" + create_module(dest, name="MyFeature") + text = (dest / "tests" / "test_my_feature.py").read_text() + assert "effective_route_paths(app)" in text + assert "app.routes" not in text.replace("``app.routes``", "") + async def test_test_file_basename_is_unique_per_module(self, tmp_path): """Two modules in one workspace must not ship same-named test files. diff --git a/framework/core/simple_module_core/__main__.py b/framework/core/simple_module_core/__main__.py index b69bfa72..98a1cec5 100644 --- a/framework/core/simple_module_core/__main__.py +++ b/framework/core/simple_module_core/__main__.py @@ -11,10 +11,9 @@ declares the locale set this install promises, which also enables SM013 for a namespace that never shipped one; with it unset, each namespace is checked against the locale files it actually has, so key drift between two shipped -translations is still caught. Host-level ``host/locales/`` and shared -``packages/ui/locales/`` are picked up relative to the project root -(``SM_PROJECT_ROOT``, else the directory of the discovered ``.env``, else the -current working dir). +translations is still caught. Host-level ``host/locales/`` is picked up +relative to the project root (``SM_PROJECT_ROOT``, else the directory of the +discovered ``.env``, else the current working dir). """ from __future__ import annotations @@ -102,6 +101,15 @@ def _hosting_locales() -> Path | None: return locales if locales.is_dir() else None +def _hosting_ui_locales() -> Path | None: + """The shared ``ui`` catalog, which ships inside ``simple_module_hosting`` (#426).""" + hosting = _hosting_locales() + if hosting is None: + return None + ui = hosting.parent / "ui_locales" + return ui if ui.is_dir() else None + + def _discover_extra_locale_sources() -> list[tuple[str, str, Path]]: """Return ``[(reporter, namespace, path), ...]`` for hosting + host + ui locale dirs.""" # Anchor on the same project root the `.env` was loaded from @@ -116,9 +124,9 @@ def _discover_extra_locale_sources() -> list[tuple[str, str, Path]]: host_locales = root / "host" / "locales" if host_locales.is_dir(): out.append(("host", "host", host_locales)) - ui_locales = root / "packages" / "ui" / "locales" - if ui_locales.is_dir(): - out.append(("packages/ui", "ui", ui_locales)) + ui_locales = _hosting_ui_locales() + if ui_locales is not None: + out.append(("simple_module_hosting/ui", "ui", ui_locales)) return out diff --git a/framework/core/simple_module_core/csp.py b/framework/core/simple_module_core/csp.py index f7b95eac..f28ec582 100644 --- a/framework/core/simple_module_core/csp.py +++ b/framework/core/simple_module_core/csp.py @@ -10,8 +10,13 @@ from __future__ import annotations import re +from collections.abc import Callable, Iterable +from typing import Any -__all__ = ["CspSourceError", "CspSourceRegistry"] +__all__ = ["CspProvider", "CspSourceError", "CspSourceRegistry", "is_valid_source"] + +CspProvider = Callable[[Any], Iterable[tuple[str, str]]] +"""``fn(app) -> [(directive, source), ...]`` evaluated per response.""" # Fetch directives a module may extend. Deliberately excludes the policy's # structural directives (default-src, base-uri, form-action, frame-ancestors, @@ -37,8 +42,10 @@ # a data-ish scheme keyword. One token — anything that could smuggle a second # token or terminate/extend the clause (whitespace, ";", ",", quotes) is # rejected, and a wildcard prefix must be followed by a real host (`*.` alone -# is not a source). -_SOURCE_RE = re.compile(r"^(?:[A-Za-z][A-Za-z0-9+.-]*:(?://)?)?(?:(?:\*\.)?[^\s;,'\"*]+)?$") +# is not a source). Only printable ASCII: the header is latin-1 encoded and +# h11 rejects control bytes, so anything else would fail at send time — on +# every response, for a provider reading an admin-edited setting. +_SOURCE_RE = re.compile(r"(?:[A-Za-z][A-Za-z0-9+.-]*:(?://)?)?(?:(?:\*\.)?(?:(?![;,'\"*])[!-~])+)?") # CSP3 fallback chains: when a directive is absent from a policy, the browser # consults these directives in order (ending at ``default-src``). A clause we @@ -59,33 +66,64 @@ class CspSourceError(ValueError): """Invalid CSP directive or source declared by a module.""" +def _checked(directive: str, source: str) -> str: + """Validate one pair and return the normalized source token, or raise.""" + if directive not in _EXTENDABLE_DIRECTIVES: + raise CspSourceError( + f"CSP directive {directive!r} is not extendable; " + f"choose one of {sorted(_EXTENDABLE_DIRECTIVES)}" + ) + token = source.strip() + if not token or not _SOURCE_RE.fullmatch(token): + raise CspSourceError( + f"invalid CSP source {source!r} for {directive}: must be a single " + "origin or scheme token (no spaces, quotes, wildcards-only, or ';')" + ) + return token + + +def is_valid_source(directive: str, source: str) -> bool: + """Whether ``add(directive, source)`` would accept the pair (never raises).""" + try: + _checked(directive, source) + except (CspSourceError, AttributeError, TypeError): + return False + return True + + class CspSourceRegistry: """Collects per-directive extra CSP sources from modules.""" def __init__(self) -> None: self._sources: dict[str, list[str]] = {} + self._providers: list[CspProvider] = [] def add(self, directive: str, source: str) -> None: """Allow ``source`` in ``directive``, e.g. ``add("style-src", "https://rsms.me")``.""" - if directive not in _EXTENDABLE_DIRECTIVES: - raise CspSourceError( - f"CSP directive {directive!r} is not extendable; " - f"choose one of {sorted(_EXTENDABLE_DIRECTIVES)}" - ) - token = source.strip() - if not token or not _SOURCE_RE.match(token): - raise CspSourceError( - f"invalid CSP source {source!r} for {directive}: must be a single " - "origin or scheme token (no spaces, quotes, wildcards-only, or ';')" - ) + token = _checked(directive, source) bucket = self._sources.setdefault(directive, []) if token not in bucket: bucket.append(token) + def add_provider(self, provider: CspProvider) -> None: + """Contribute sources that depend on runtime state (e.g. DB settings). + + ``provider(app)`` runs on every response and returns + ``(directive, source)`` pairs. It must be synchronous and read only + in-memory state (``app.state..settings``) — never the DB. Pairs + that fail ``add()``'s validation, and providers that raise, are + dropped and logged by the host: the static policy still applies. + """ + self._providers.append(provider) + + @property + def providers(self) -> tuple[CspProvider, ...]: + return tuple(self._providers) + def __bool__(self) -> bool: return bool(self._sources) - def extend_policy(self, policy: str) -> str: + def extend_policy(self, policy: str, extra: Iterable[tuple[str, str]] = ()) -> str: """Fold the registered sources into an existing policy string. Existing clauses keep their order and gain only sources they don't @@ -94,8 +132,16 @@ def extend_policy(self, policy: str) -> str: back to (per ``_FALLBACK_CHAINS``, ending at ``default-src``) — without that seed the new clause would *narrow* the policy, since the browser stops consulting the fallback once the directive exists. + + ``extra`` adds already-validated ``(directive, source)`` pairs (from + providers) for this call only — the registry itself is not mutated. """ - if not self._sources: + sources = {d: list(v) for d, v in self._sources.items()} + for directive, source in extra: + bucket = sources.setdefault(directive, []) + if source not in bucket: + bucket.append(source) + if not sources: return policy directives: dict[str, list[str]] = {} order: list[str] = [] @@ -106,7 +152,7 @@ def extend_policy(self, policy: str) -> str: directive, _, rest = clause.partition(" ") directives[directive] = rest.split() order.append(directive) - for directive, extras in self._sources.items(): + for directive, extras in sources.items(): if directive not in directives: directives[directive] = list(self._seed_sources(directive, directives)) order.append(directive) @@ -126,7 +172,7 @@ def extend_policy(self, policy: str) -> str: continue bucket = directives[shadowing] for base in chain: - extras = self._sources.get(base) + extras = sources.get(base) if extras: bucket.extend(e for e in extras if e not in bucket) return "; ".join(f"{d} {' '.join(directives[d])}".rstrip() for d in order) diff --git a/framework/core/simple_module_core/diagnostics/_i18n.py b/framework/core/simple_module_core/diagnostics/_i18n.py index 68ce45bf..f3803bbb 100644 --- a/framework/core/simple_module_core/diagnostics/_i18n.py +++ b/framework/core/simple_module_core/diagnostics/_i18n.py @@ -44,7 +44,7 @@ def __init__( ``extra_sources`` is an optional list of ``(reporter_name, namespace, locale_dir)`` triples for locale directories that aren't owned by any ``ModuleBase`` instance — notably the host's ``host/locales/`` and - the shared ``packages/ui/locales/``. ``reporter_name`` is used as the + the shared ``simple_module_hosting/ui_locales/``. ``reporter_name`` is used as the ``module_name`` field on findings for display purposes. """ self.supported_locales = None if supported_locales is None else list(supported_locales) diff --git a/framework/core/simple_module_core/module.py b/framework/core/simple_module_core/module.py index 4ae0b864..64450961 100644 --- a/framework/core/simple_module_core/module.py +++ b/framework/core/simple_module_core/module.py @@ -182,10 +182,10 @@ def register_csp_sources(self, registry): registry.add("style-src", "https://rsms.me") registry.add("font-src", "https://rsms.me") - Only fetch directives can be extended (never default-src, - base-uri, form-action, frame-ancestors), and each source must be a - single origin/scheme token — invalid declarations raise at boot. - Called once, in dependency order. + Fetch directives only; invalid declarations raise at boot. Runs once, + *before* DB settings hydrate — for an origin read from settings use + ``registry.add_provider(lambda app: [(directive, source)])`` (sync, + in-memory state only, evaluated per response). """ def register_design_packs(self, registry: DesignPackRegistry) -> None: diff --git a/framework/core/simple_module_core/services.py b/framework/core/simple_module_core/services.py index b939a1af..5a1cd0a7 100644 --- a/framework/core/simple_module_core/services.py +++ b/framework/core/simple_module_core/services.py @@ -104,3 +104,6 @@ class Services: #: How the host scopes requests (#418), recorded where ``create_app`` #: decides on ``TenantMiddleware``. Defaulted for apps built outside it. tenancy: TenancyMode = TenancyMode.SINGLE + #: Module name -> ``view_prefix`` for modules whose view router received + #: routes, in discovery order. ``""`` here means mounted at ``/`` (#435). + view_prefixes: dict[str, str] = field(default_factory=dict) diff --git a/framework/core/tests/test_csp_registry.py b/framework/core/tests/test_csp_registry.py index 2a6031ad..6347257f 100644 --- a/framework/core/tests/test_csp_registry.py +++ b/framework/core/tests/test_csp_registry.py @@ -11,7 +11,7 @@ import pytest from simple_module_core import ModuleBase, ModuleMeta -from simple_module_core.csp import CspSourceError, CspSourceRegistry +from simple_module_core.csp import CspSourceError, CspSourceRegistry, is_valid_source _POLICY = ( "default-src 'self'; " @@ -114,3 +114,69 @@ class Quiet(ModuleBase): reg = CspSourceRegistry() Quiet().register_csp_sources(reg) assert not reg + + +class TestProviders: + """Settings-driven origins (#434): providers run per response, not at boot.""" + + def test_add_provider_registers_without_static_sources(self) -> None: + reg = CspSourceRegistry() + + def provider(app): + return [("img-src", "https://tiles.example")] + + reg.add_provider(provider) + assert reg.providers == (provider,) + # A provider is not a static source: the boot-time policy is unchanged. + assert reg.extend_policy(_POLICY) == _POLICY + + def test_providers_is_read_only(self) -> None: + reg = CspSourceRegistry() + assert reg.providers == () + assert isinstance(reg.providers, tuple) + + def test_is_valid_source_matches_add_validation(self) -> None: + assert is_valid_source("img-src", "https://tiles.example") + assert not is_valid_source("default-src", "https://tiles.example") + assert not is_valid_source("img-src", "https://x; script-src *") + assert not is_valid_source("img-src", "") + + def test_extend_policy_with_extra_pairs(self) -> None: + reg = CspSourceRegistry() + reg.add("style-src", "https://rsms.me") + out = reg.extend_policy(_POLICY, extra=[("connect-src", "https://api.example")]) + assert "https://rsms.me" in out + assert "connect-src 'self' https://api.example" in out + + def test_extra_pairs_do_not_mutate_the_registry(self) -> None: + reg = CspSourceRegistry() + reg.extend_policy(_POLICY, extra=[("connect-src", "https://api.example")]) + assert not reg + assert reg.extend_policy(_POLICY) == _POLICY + + def test_extra_pairs_are_deduplicated_against_static_and_policy(self) -> None: + reg = CspSourceRegistry() + reg.add("style-src", "https://rsms.me") + out = reg.extend_policy( + _POLICY, + extra=[("style-src", "https://rsms.me"), ("style-src", "https://fonts.googleapis.com")], + ) + assert out.count("https://rsms.me") == 1 + assert out.count("https://fonts.googleapis.com") == 1 + + +@pytest.mark.parametrize( + "source", + [ + "https://ex\u00e4mple.com", + "https://x.example\x00", + "https://x\x7f.example", + "https://a\u2028b", + ], +) +def test_source_must_be_printable_ascii(source) -> None: + # The header is latin-1 encoded and h11 rejects control bytes, so anything + # outside printable ASCII would fail at send time on every response. + assert not is_valid_source("img-src", source) + with pytest.raises(CspSourceError): + CspSourceRegistry().add("img-src", source) diff --git a/framework/hosting/simple_module_hosting/_csp_dynamic.py b/framework/hosting/simple_module_hosting/_csp_dynamic.py new file mode 100644 index 00000000..aff36b17 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_csp_dynamic.py @@ -0,0 +1,99 @@ +"""Per-response evaluation of module CSP providers (#434). + +``register_csp_sources`` runs before DB settings hydrate, so origins that come +from a setting are declared as providers (``registry.add_provider(fn)``) and +evaluated here on each response. The finished policy is cached by the *value* +the providers return, so a settings edit changes the key on the next response +without any invalidation wiring. + +Fail closed: a provider that raises, or a pair that fails the same validation +``CspSourceRegistry.add`` applies, contributes nothing — the static policy +still ships. Each distinct failure is logged once per process. +""" + +from __future__ import annotations + +import logging +from typing import Any + +from simple_module_core.csp import CspSourceRegistry, is_valid_source + +logger = logging.getLogger(__name__) + +_CACHE_MAX = 32 +_LOGGED_MAX = 256 + + +class DynamicCsp: + """Builds the CSP for one response from a static base plus provider output.""" + + def __init__(self, base: str, registry: CspSourceRegistry) -> None: + self.base = base + self._registry = registry + self._providers = registry.providers + self._cache: dict[frozenset[tuple[str, str]], str] = {} + self._logged: set[str] = set() + + def policy_for(self, app: Any) -> str: + pairs = frozenset(self._collect(app)) + if not pairs: + return self.base + policy = self._cache.get(pairs) + if policy is None: + if len(self._cache) >= _CACHE_MAX: + self._cache.clear() + # Sorted so the clause order is stable for a given set of pairs. + try: + policy = self._registry.extend_policy(self.base, extra=sorted(pairs)) + except Exception as exc: + # Last line of the fail-closed guarantee: this runs inside the + # response for every page, so a bug here must cost the extra + # origins, never the response. + self._warn_once( + f"build:{type(exc).__name__}:{exc}", + "CSP provider policy build failed (%r); serving the static policy", + exc, + ) + return self.base + self._cache[pairs] = policy + return policy + + def _collect(self, app: Any) -> list[tuple[str, str]]: + out: list[tuple[str, str]] = [] + for provider in self._providers: + name = getattr(provider, "__qualname__", repr(provider)) + try: + pairs = list(provider(app)) + except Exception as exc: + self._warn_once( + f"{name}:{type(exc).__name__}:{exc}", + "CSP provider %s raised %r; its sources are skipped", + name, + exc, + ) + continue + for pair in pairs: + if ( + isinstance(pair, tuple) + and len(pair) == 2 + and isinstance(pair[0], str) + and isinstance(pair[1], str) + and is_valid_source(pair[0], pair[1]) + ): + out.append((pair[0], pair[1].strip())) + else: + self._warn_once( + f"{name}:{pair!r}", + "CSP provider %s returned invalid pair %r; dropped", + name, + pair, + ) + return out + + def _warn_once(self, key: str, msg: str, *args: object) -> None: + if key in self._logged: + return + if len(self._logged) >= _LOGGED_MAX: + self._logged.clear() + self._logged.add(key) + logger.warning(msg, *args) diff --git a/framework/hosting/simple_module_hosting/_i18n_build.py b/framework/hosting/simple_module_hosting/_i18n_build.py index 8190143d..c2cacec0 100644 --- a/framework/hosting/simple_module_hosting/_i18n_build.py +++ b/framework/hosting/simple_module_hosting/_i18n_build.py @@ -47,10 +47,12 @@ def build_i18n_registry( if overrides.is_dir(): registry.add_overrides(overrides) - ui_locales = project_root / "packages" / "ui" / "locales" - if ui_locales.is_dir(): - registry.add_source("ui", ui_locales) - extra_sources.append(("packages/ui", "ui", ui_locales)) + # Shared UI strings ship inside this package (not ``packages/ui``) so a + # scaffolded app, which has no ``packages/ui`` checkout, still resolves + # every ``ui.*`` key. + ui_locales = Path(__file__).resolve().parent / "ui_locales" + registry.add_source("ui", ui_locales) + extra_sources.append(("simple_module_hosting/ui", "ui", ui_locales)) registry.load() return registry, extra_sources diff --git a/framework/hosting/simple_module_hosting/_inertia_shared.py b/framework/hosting/simple_module_hosting/_inertia_shared.py index e698d797..30244609 100644 --- a/framework/hosting/simple_module_hosting/_inertia_shared.py +++ b/framework/hosting/simple_module_hosting/_inertia_shared.py @@ -144,3 +144,16 @@ async def merge_shared_prop_providers(app: Any, request: Request, shared: dict) ) continue shared[key] = value + + +def default_home_url(app: Any) -> str: + """Where the layouts' "home" links point when no module publishes ``homeUrl``. + + The users module publishes its configured landing URL and wins; this covers + installs without it (Keycloak). Dashboard is the conventional landing page, + named by string from ``sm.view_prefixes`` so nothing is imported (SM009); + ``/`` is a host route in every app, so the link can never 404. + """ + sm = getattr(app.state, "sm", None) + prefix = getattr(sm, "view_prefixes", {}).get("Dashboard") + return f"{prefix}/" if prefix else "/" diff --git a/framework/hosting/simple_module_hosting/_module_routes.py b/framework/hosting/simple_module_hosting/_module_routes.py index 6dcf02bb..037d221c 100644 --- a/framework/hosting/simple_module_hosting/_module_routes.py +++ b/framework/hosting/simple_module_hosting/_module_routes.py @@ -12,9 +12,13 @@ __all__ = ["wire_module_routes"] -def wire_module_routes(app: FastAPI, module) -> None: +def wire_module_routes(app: FastAPI, module) -> bool: """Attach a module's API + view (+ admin view) routers to ``app``. + Returns whether the module mounted any view routes. ``view_prefix=""`` means + both "no views" and "mounted at /", so only a router that actually received + routes says which one it is (#435). + 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. @@ -23,6 +27,7 @@ def wire_module_routes(app: FastAPI, module) -> None: 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) + has_views = bool(view_router.routes) app.include_router(api_router) app.include_router(view_router) @@ -38,6 +43,7 @@ def wire_module_routes(app: FastAPI, module) -> None: module.register_admin_routes(admin_router) _clone_bare_prefix_route(admin_router, admin_prefix) app.include_router(admin_router) + return has_views def _clone_bare_prefix_route(router: APIRouter, prefix: str) -> None: diff --git a/framework/hosting/simple_module_hosting/_phase_helpers.py b/framework/hosting/simple_module_hosting/_phase_helpers.py index e5e1602d..2c3c3595 100644 --- a/framework/hosting/simple_module_hosting/_phase_helpers.py +++ b/framework/hosting/simple_module_hosting/_phase_helpers.py @@ -174,7 +174,9 @@ def install_middleware( } if settings.is_development: security_kwargs["strict_transport_security"] = None - app.add_middleware(SecurityHeadersMiddleware, **security_kwargs) + # Providers registered via ``registry.add_provider`` follow DB-backed + # settings, so the middleware evaluates them per response (#434). + app.add_middleware(SecurityHeadersMiddleware, csp_registry=csp_registry, **security_kwargs) # Compress response bodies. Added here so it sits inside CorrelationId and # RequestLogging (which set headers and read request state) but outside # everything that produces a body — including the /static mount, where it diff --git a/framework/hosting/simple_module_hosting/_route_guard.py b/framework/hosting/simple_module_hosting/_route_guard.py index 4d1ffe58..07023a16 100644 --- a/framework/hosting/simple_module_hosting/_route_guard.py +++ b/framework/hosting/simple_module_hosting/_route_guard.py @@ -53,7 +53,9 @@ def _common_prefix(paths: list[str]) -> str: """The literal leading part every path shares — never past a ``{param}``.""" if not paths: return "" - return os.path.commonprefix(paths).split("{", 1)[0] + # Character-wise on purpose: the cut happens at the first "{param}", which + # path-component comparison (commonpath) cannot express. + return os.path.commonprefix(paths).split("{", 1)[0] # noqa: RUF071 class _PrefixGuard: diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index 3aae5cff..acf647b7 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -234,8 +234,10 @@ def create_app(settings: Settings | None = None) -> FastAPI: ) # ── Phase 9: Routes, health, static files ────────────── + view_prefixes: dict[str, str] = {} # discovery order; only modules with view routes for mod in modules: - wire_module_routes(app, mod) + if wire_module_routes(app, mod): + view_prefixes[mod.meta.name] = mod.meta.view_prefix app.include_router(health_router) mount_setup_wizard(app, setup_registry) # /setup — what SetupMiddleware redirects to @@ -265,6 +267,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: invalidation=invalidation_bus, diagnostics=diagnostics_state, tenancy=mode_for(settings), + view_prefixes=view_prefixes, ) return app diff --git a/framework/hosting/simple_module_hosting/middleware.py b/framework/hosting/simple_module_hosting/middleware.py index 27bf1529..7de536b6 100644 --- a/framework/hosting/simple_module_hosting/middleware.py +++ b/framework/hosting/simple_module_hosting/middleware.py @@ -17,9 +17,11 @@ from starlette.requests import Request from starlette.types import ASGIApp, Message, Receive, Scope, Send +from simple_module_hosting._csp_dynamic import DynamicCsp from simple_module_hosting._inertia_shared import ( build_i18n_block, build_menu_translator, + default_home_url, merge_shared_prop_providers, ) from simple_module_hosting._observability import ( @@ -35,6 +37,7 @@ from simple_module_hosting.permissions import expand_permissions, resolve_principal_permissions if TYPE_CHECKING: + from simple_module_core.csp import CspSourceRegistry from simple_module_core.menu import MenuRegistry from simple_module_core.permissions import PermissionRegistry @@ -136,10 +139,18 @@ def __init__( *, content_security_policy: str | None = DEFAULT_CSP, strict_transport_security: str | None = _DEFAULT_HSTS, + csp_registry: CspSourceRegistry | None = None, ) -> None: self.app = app self.csp = content_security_policy self.hsts = strict_transport_security + # Providers (settings-driven origins, #434) are evaluated per response; + # with none, the precomputed string above is the whole story. + self._dynamic = ( + DynamicCsp(content_security_policy, csp_registry) + if content_security_policy and csp_registry is not None and csp_registry.providers + else None + ) async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: if scope["type"] != _SCOPE_HTTP: @@ -160,7 +171,8 @@ async def send_with_headers(message: Message) -> None: own = headers.get(_HEADER_CSP) or "" keeps_own = "sandbox" in {d.strip().split(" ")[0].lower() for d in own.split(";")} if self.csp and not keeps_own: - headers[_HEADER_CSP] = self.csp + dyn = self._dynamic + headers[_HEADER_CSP] = dyn.policy_for(scope.get("app")) if dyn else self.csp if self.hsts: headers[_HEADER_HSTS] = self.hsts await send(message) @@ -249,6 +261,8 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: # Merge module-registered shared-prop providers (e.g. branding) — read off # app.state without importing the plugin (SM009), defensively. await merge_shared_prop_providers(scope["app"], request, shared) + # After the providers, so a module's own value (users) wins. + shared.setdefault("homeUrl", default_home_url(scope["app"])) request.state.inertia_shared = shared diff --git a/framework/hosting/simple_module_hosting/permissions.py b/framework/hosting/simple_module_hosting/permissions.py index e9f1baae..438e3f19 100644 --- a/framework/hosting/simple_module_hosting/permissions.py +++ b/framework/hosting/simple_module_hosting/permissions.py @@ -46,6 +46,18 @@ def resolve_permissions( return permissions +def _role_and_claimed(user: Any, role_map: dict[str, list[str]] | None) -> set[str]: + """The role-derived set plus the principal's own ``permissions`` (GH #436). + + Read duck-typed (``UserContext.permissions`` lives in the auth module, which + framework code must not import). The claimed keys never include the + wildcard: like a grant source, a token or IdP claim cannot make an admin. + """ + permissions = resolve_permissions(getattr(user, "roles", None) or [], role_map=role_map) + permissions.update(p for p in getattr(user, "permissions", None) or () if p != WILDCARD) + return permissions + + async def resolve_principal_permissions( request: Request, user: Any, @@ -63,7 +75,7 @@ async def resolve_principal_permissions( sources: nothing they return could widen it, and a source may cost a read. """ role_map = registry.role_map if registry is not None else None - permissions = resolve_permissions(getattr(user, "roles", []), role_map=role_map) + permissions = _role_and_claimed(user, role_map) if registry is None or WILDCARD in permissions: return permissions for source in registry.grant_sources: @@ -138,7 +150,7 @@ def resolved_permissions_for(request: Request) -> set[str]: perm_registry = _registry_for(request) role_map = perm_registry.role_map if perm_registry is not None else None - permissions = resolve_permissions(user.roles, role_map=role_map) + permissions = _role_and_claimed(user, role_map) request.state.resolved_permissions = permissions return permissions diff --git a/packages/ui/locales/en.json b/framework/hosting/simple_module_hosting/ui_locales/en.json similarity index 100% rename from packages/ui/locales/en.json rename to framework/hosting/simple_module_hosting/ui_locales/en.json diff --git a/packages/ui/locales/es.json b/framework/hosting/simple_module_hosting/ui_locales/es.json similarity index 100% rename from packages/ui/locales/es.json rename to framework/hosting/simple_module_hosting/ui_locales/es.json diff --git a/framework/hosting/tests/test_csp_providers.py b/framework/hosting/tests/test_csp_providers.py new file mode 100644 index 00000000..f2033243 --- /dev/null +++ b/framework/hosting/tests/test_csp_providers.py @@ -0,0 +1,163 @@ +"""CSP providers follow runtime state instead of freezing at boot (#434). + +``register_csp_sources`` runs before DB settings hydrate, so an origin that +comes from a setting (a configurable tile server, an analytics host) cannot be +a static ``add()``. A provider is evaluated per response against ``app``. +""" + +from __future__ import annotations + +import logging +from types import SimpleNamespace + +import httpx +import pytest +import simple_module_hosting.app_builder as app_builder +from simple_module_core import ModuleBase, ModuleMeta +from simple_module_core.csp import CspSourceRegistry +from simple_module_hosting.middleware import SecurityHeadersMiddleware +from simple_module_test.fixtures import _build_app +from starlette.applications import Starlette +from starlette.responses import PlainTextResponse +from starlette.routing import Route + +_CSP = "content-security-policy" + + +class TilesModule(ModuleBase): + """Reads its tile origin from (DB-hydrated) settings on ``app.state``.""" + + meta = ModuleMeta(name="CspTiles") + + def register_settings(self, app) -> None: + app.state.csptiles = SimpleNamespace(tile_origin="https://tiles-a.example") + + def register_csp_sources(self, registry: CspSourceRegistry) -> None: + registry.add_provider(lambda app: [("img-src", app.state.csptiles.tile_origin)]) + + +@pytest.fixture +async def tiles_app(monkeypatch, settings): + real_discover = app_builder.discover_modules + monkeypatch.setattr( + app_builder, "discover_modules", lambda *a, **k: [*real_discover(*a, **k), TilesModule()] + ) + async for application in _build_app(settings, seed_admin=True): + yield application + + +async def _csp(app, path: str = "/health/live") -> str: + transport = httpx.ASGITransport(app=app) + async with httpx.AsyncClient(transport=transport, base_url="http://testserver") as c: + resp = await c.get(path) + return resp.headers[_CSP] + + +class TestThroughCreateApp: + async def test_provider_origin_reaches_real_response(self, tiles_app) -> None: + assert "https://tiles-a.example" in await _csp(tiles_app) + + async def test_changed_setting_changes_next_response(self, tiles_app) -> None: + await _csp(tiles_app) + tiles_app.state.csptiles.tile_origin = "https://tiles-b.example" + csp = await _csp(tiles_app) + assert "https://tiles-b.example" in csp + assert "https://tiles-a.example" not in csp + + +def _app( + registry: CspSourceRegistry | None, + state: dict | None = None, + csp: str = SecurityHeadersMiddleware.DEFAULT_CSP, +) -> Starlette: + async def ok(_request): + return PlainTextResponse("ok") + + inner = Starlette(routes=[Route("/", ok)]) + for k, v in (state or {}).items(): + setattr(inner.state, k, v) + inner.add_middleware( + SecurityHeadersMiddleware, + content_security_policy=csp, + csp_registry=registry, + ) + return inner + + +class TestMiddleware: + async def test_no_providers_header_identical_to_static(self) -> None: + reg = CspSourceRegistry() + reg.add("style-src", "https://rsms.me") + assert await _csp(_app(None), "/") == SecurityHeadersMiddleware.DEFAULT_CSP + assert await _csp(_app(reg), "/") == SecurityHeadersMiddleware.DEFAULT_CSP + + async def test_raising_provider_leaves_static_policy(self, caplog) -> None: + reg = CspSourceRegistry() + + def boom(app): + raise RuntimeError("settings not hydrated") + + reg.add_provider(boom) + app = _app(reg) + with caplog.at_level(logging.WARNING): + assert await _csp(app, "/") == SecurityHeadersMiddleware.DEFAULT_CSP + assert await _csp(app, "/") == SecurityHeadersMiddleware.DEFAULT_CSP + # Logged once per distinct failure, not once per response. + assert sum("settings not hydrated" in r.getMessage() for r in caplog.records) == 1 + + async def test_invalid_pair_dropped_valid_pair_kept(self, caplog) -> None: + reg = CspSourceRegistry() + reg.add_provider( + lambda app: [ + ("default-src", "*"), + ("img-src", "https://x; script-src *"), + ("img-src", "https://ok.example"), + ] + ) + with caplog.at_level(logging.WARNING): + csp = await _csp(_app(reg), "/") + assert "https://ok.example" in csp + assert "script-src *" not in csp + assert "default-src 'self';" in csp + assert any("default-src" in r.getMessage() for r in caplog.records) + + async def test_only_invalid_pairs_leave_static_policy(self) -> None: + reg = CspSourceRegistry() + reg.add_provider(lambda app: [("img-src", "")]) + assert await _csp(_app(reg), "/") == SecurityHeadersMiddleware.DEFAULT_CSP + + async def test_static_sources_kept_alongside_provider_sources(self) -> None: + reg = CspSourceRegistry() + reg.add("style-src", "https://rsms.me") + reg.add_provider(lambda app: [("connect-src", app.state.api)]) + base = reg.extend_policy(SecurityHeadersMiddleware.DEFAULT_CSP) + csp = await _csp(_app(reg, {"api": "https://api.example"}, base), "/") + assert "https://rsms.me" in csp + assert "connect-src 'self' https://api.example" in csp + + @pytest.mark.parametrize( + "source", + [ + "https://exämple.com", + "https://x.example\x00", + "https://x.example\x7f", + "https://x\u2028y", + ], + ) + async def test_unencodable_source_never_breaks_the_response(self, source) -> None: + # An admin-edited setting is the usual input: a value the header + # encoder cannot carry must be dropped, not 500 every page — including + # the settings page needed to correct it. + reg = CspSourceRegistry() + reg.add_provider(lambda app: [("img-src", source)]) + assert await _csp(_app(reg), "/") == SecurityHeadersMiddleware.DEFAULT_CSP + + async def test_policy_build_failure_falls_back_to_static(self, monkeypatch) -> None: + reg = CspSourceRegistry() + reg.add_provider(lambda app: [("img-src", "https://ok.example")]) + + def broken(*_a, **_k): + raise RuntimeError("boom") + + monkeypatch.setattr(reg, "extend_policy", broken) + assert await _csp(_app(reg), "/") == SecurityHeadersMiddleware.DEFAULT_CSP diff --git a/framework/hosting/tests/test_home_url_default.py b/framework/hosting/tests/test_home_url_default.py new file mode 100644 index 00000000..eec14c9e --- /dev/null +++ b/framework/hosting/tests/test_home_url_default.py @@ -0,0 +1,35 @@ +"""The host supplies a ``homeUrl`` default when no module does (#427). + +The users module publishes its configured landing URL; an install without it +(Keycloak) must still point the layouts' home links at the Dashboard rather +than falling back to ``/``. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +from simple_module_hosting._inertia_shared import default_home_url + + +def _app(prefixes: dict[str, str]) -> SimpleNamespace: + return SimpleNamespace(state=SimpleNamespace(sm=SimpleNamespace(view_prefixes=prefixes))) + + +def test_dashboard_installed_is_home() -> None: + assert default_home_url(_app({"Users": "/users", "Dashboard": "/dashboard"})) == "/dashboard/" + + +def test_without_dashboard_home_is_root() -> None: + assert default_home_url(_app({"Admin": "/admin/things"})) == "/" + + +def test_no_services_yet_home_is_root() -> None: + assert default_home_url(SimpleNamespace(state=SimpleNamespace())) == "/" + + +async def test_page_payload_carries_home_url(authenticated_client) -> None: + # In the full app the users module's own value wins; either way the key is + # always present, so the client fallback is a last resort only. + resp = await authenticated_client.get("/admin/", headers={"X-Inertia": "true"}) + assert resp.json()["props"]["homeUrl"].endswith("/") diff --git a/framework/hosting/tests/test_i18n_ui_namespace.py b/framework/hosting/tests/test_i18n_ui_namespace.py new file mode 100644 index 00000000..c338114c --- /dev/null +++ b/framework/hosting/tests/test_i18n_ui_namespace.py @@ -0,0 +1,15 @@ +"""The ``ui`` namespace ships with the hosting package (#426).""" + +from __future__ import annotations + +from simple_module_hosting.i18n_manifest import build_i18n_registry +from simple_module_hosting.settings import Settings + + +def test_ui_namespace_registered_without_packages_ui(tmp_path): + assert not (tmp_path / "packages" / "ui").exists() + settings = Settings(database_url="sqlite+aiosqlite:///:memory:") + registry, extra = build_i18n_registry(settings, [], tmp_path) + msgs = registry.messages("en") + assert msgs["ui.nav_groups.navigation"] == "Navigation" + assert any(ns == "ui" for _, ns, _ in extra) diff --git a/framework/hosting/tests/test_principal_permissions_claim.py b/framework/hosting/tests/test_principal_permissions_claim.py new file mode 100644 index 00000000..c8d88444 --- /dev/null +++ b/framework/hosting/tests/test_principal_permissions_claim.py @@ -0,0 +1,91 @@ +"""A principal's own ``permissions`` (token/IdP claims) reach the door (GH #436).""" + +from __future__ import annotations + +from types import SimpleNamespace + +from fastapi import Depends, FastAPI +from httpx import ASGITransport, AsyncClient +from simple_module_core.permissions import PermissionRegistry +from simple_module_hosting.permissions import ( + RequiresPermission, + resolve_principal_permissions, + resolved_permissions_for, +) + + +def _registry() -> PermissionRegistry: + reg = PermissionRegistry() + reg.add_group("x", ["x.read", "x.write"]) + return reg + + +def _user(permissions: list[str] | None = None, roles: list[str] | None = None) -> SimpleNamespace: + return SimpleNamespace(id="u1", roles=roles or [], permissions=permissions or []) + + +class TestResolvePrincipalPermissions: + async def test_user_permissions_are_merged(self): + held = await resolve_principal_permissions(None, _user(["x.read"]), _registry()) + assert held == {"x.read"} + + async def test_merged_without_a_registry(self): + held = await resolve_principal_permissions(None, _user(["x.read"]), None) + assert "x.read" in held + + async def test_merged_with_role_permissions(self): + reg = _registry() + reg.map_role("clerk", ["x.write"]) + held = await resolve_principal_permissions(None, _user(["x.read"], ["clerk"]), reg) + assert held == {"x.read", "x.write"} + + async def test_claimed_wildcard_is_dropped(self): + held = await resolve_principal_permissions(None, _user(["*", "x.read"]), _registry()) + assert held == {"x.read"} + + async def test_principal_without_attribute(self): + held = await resolve_principal_permissions(None, SimpleNamespace(roles=[]), _registry()) + assert held == set() + + +class TestSyncFallbackAgrees: + def _request(self, user) -> SimpleNamespace: + app = SimpleNamespace(state=SimpleNamespace(sm=SimpleNamespace(permissions=_registry()))) + return SimpleNamespace(app=app, state=SimpleNamespace(user=user)) + + def test_user_permissions_merged_wildcard_dropped(self): + held = resolved_permissions_for(self._request(_user(["*", "x.read"]))) + assert held == {"x.read"} + + +class TestRequiresPermission: + def _app(self, user) -> FastAPI: + app = FastAPI() + app.state.sm = SimpleNamespace(permissions=_registry()) + + @app.middleware("http") + async def set_user(request, call_next): + request.state.user = user + return await call_next(request) + + @app.get("/read", dependencies=[Depends(RequiresPermission("x.read"))]) + async def read(): + return {"ok": True} + + @app.get("/write", dependencies=[Depends(RequiresPermission("x.write"))]) + async def write(): + return {"ok": True} + + return app + + async def _status(self, user, path: str) -> int: + transport = ASGITransport(app=self._app(user)) + async with AsyncClient(transport=transport, base_url="http://t") as client: + return (await client.get(path)).status_code + + async def test_claimed_permission_admits(self): + assert await self._status(_user(["x.read"]), "/read") == 200 + assert await self._status(_user(["x.read"]), "/write") == 403 + + async def test_claimed_wildcard_is_not_a_wildcard(self): + assert await self._status(_user(["*"]), "/write") == 403 diff --git a/framework/hosting/tests/test_view_prefixes.py b/framework/hosting/tests/test_view_prefixes.py new file mode 100644 index 00000000..0c55fd01 --- /dev/null +++ b/framework/hosting/tests/test_view_prefixes.py @@ -0,0 +1,39 @@ +"""wire_module_routes records which modules actually mount view routes (#435).""" + +from __future__ import annotations + +from fastapi import APIRouter, FastAPI +from simple_module_core import ModuleBase, ModuleMeta +from simple_module_hosting._module_routes import wire_module_routes + + +def _module(name: str, view_prefix: str, *, views: bool, api: bool = False) -> ModuleBase: + class _M(ModuleBase): + meta = ModuleMeta(name=name, view_prefix=view_prefix, route_prefix=f"/api/{name}") + + def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: + if views: + view_router.add_api_route("/", lambda: "ok", methods=["GET"]) + if api: + api_router.add_api_route("/", lambda: "ok", methods=["GET"]) + + return _M() + + +def test_returns_whether_module_mounted_view_routes(): + app = FastAPI() + assert wire_module_routes(app, _module("Root", "", views=True)) is True + assert wire_module_routes(app, _module("Blog", "/blog", views=True)) is True + assert wire_module_routes(app, _module("Api", "", views=False, api=True)) is False + assert wire_module_routes(app, _module("Empty", "/empty", views=False)) is False + + +def test_services_view_prefixes_follow_discovery_order(app): + names = [m.meta.name for m in app.state.sm.modules] + recorded = list(app.state.sm.view_prefixes) + assert recorded == [n for n in names if n in recorded] + # Real module set: Users and Dashboard serve views, and each records its + # own prefix, not a placeholder. + by_name = {m.meta.name: m.meta.view_prefix for m in app.state.sm.modules} + assert {"Users", "Dashboard"} <= set(recorded) + assert all(app.state.sm.view_prefixes[n] == by_name[n] for n in recorded) diff --git a/host/client_app/pages/Error.tsx b/host/client_app/pages/Error.tsx index d2f12ccd..19b631d0 100644 --- a/host/client_app/pages/Error.tsx +++ b/host/client_app/pages/Error.tsx @@ -4,6 +4,7 @@ import { CopyableId } from '@simple-module-py/ui/components/CopyableId'; import { ErrorScreen } from '@simple-module-py/ui/components/ErrorScreen'; import { InterpolatedText } from '@simple-module-py/ui/components/InterpolatedText'; import { Button } from '@simple-module-py/ui/components/ui/button'; +import { useHomeUrl } from '@simple-module-py/ui/lib/home-url'; import type { SharedProps } from '@simple-module-py/ui/types'; import type { ReactNode } from 'react'; import { ErrorShell, useInAdminShell } from '../components/ErrorShell'; @@ -121,6 +122,7 @@ function ErrorPage({ }: Props) { const { t } = useT(); const { auth } = usePage<{ props: SharedProps }>().props as unknown as SharedProps; + const appHome = useHomeUrl(); const copy = useStatusCopy(status, Boolean(maintenance)); const inline = useInAdminShell(); @@ -150,7 +152,7 @@ function ErrorPage({ const showSignIn = Boolean(login_url); // "Home" is wherever this visitor's app starts. Sending a signed-in user to // the marketing page makes them navigate back in through the nav. - const homeHref = auth?.isAuthenticated ? '/dashboard/' : '/'; + const homeHref = auth?.isAuthenticated ? appHome : '/'; return ( <> diff --git a/host/client_app/pages/Landing.tsx b/host/client_app/pages/Landing.tsx index 5288d22a..791f8f68 100644 --- a/host/client_app/pages/Landing.tsx +++ b/host/client_app/pages/Landing.tsx @@ -4,6 +4,7 @@ import { Button } from '@simple-module-py/ui/components/ui/button'; import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; import { PublicLayout } from '@simple-module-py/ui/layouts/PublicLayout'; import { LOGIN_PATH } from '@simple-module-py/ui/lib/auth-routes'; +import { useHomeUrl } from '@simple-module-py/ui/lib/home-url'; import type { SharedProps } from '@simple-module-py/ui/types'; import { Database, LayoutTemplate, Package, Route, ShieldCheck, Stethoscope } from 'lucide-react'; import { CopyCommand } from '../components/CopyCommand'; @@ -38,12 +39,13 @@ const QUICKSTART: { comment: string; command: string; ok?: string }[] = [ function Landing() { const { t } = useT(); const { auth } = usePage<{ props: SharedProps }>().props as unknown as SharedProps; + const appHome = useHomeUrl(); // The admin UI is the only thing behind a session here, so the one CTA that // leads anywhere private is auth-aware: a signed-in visitor is offered the // dashboard, everyone else the sign-in page. Never /auth/* — that is the // JSON API prefix and has no page (see host/tests/test_public_auth_links). - const adminHref = auth?.isAuthenticated ? '/dashboard/' : LOGIN_PATH; + const adminHref = auth?.isAuthenticated ? appHome : LOGIN_PATH; const features = [ { diff --git a/modules/auth/auth/contracts/schemas.py b/modules/auth/auth/contracts/schemas.py index b6d0187f..78cb3430 100644 --- a/modules/auth/auth/contracts/schemas.py +++ b/modules/auth/auth/contracts/schemas.py @@ -20,6 +20,10 @@ class UserContext: name: str roles: list[str] = field(default_factory=list) tenant_id: str | None = None + # Permissions the principal carries itself — a token scope or IdP claim a + # provider maps in. Frozen in the session cookie like ``roles``; anything + # revocable belongs in a grant source. The resolver drops ``*`` from here. + permissions: list[str] = field(default_factory=list) @classmethod def from_user(cls, user: User | Any) -> UserContext: @@ -51,6 +55,7 @@ def to_session_dict(self) -> dict[str, Any]: "name": self.name, "roles": list(self.roles), "tenant_id": self.tenant_id, + "permissions": list(self.permissions), } @classmethod @@ -66,6 +71,8 @@ def from_session_dict(cls, payload: Any) -> UserContext | None: name=str(payload["name"]), roles=list(payload.get("roles") or []), tenant_id=payload.get("tenant_id"), + # Absent on cookies written before the field existed. + permissions=list(payload.get("permissions") or []), ) except (KeyError, TypeError, ValueError): return None @@ -73,5 +80,12 @@ def from_session_dict(cls, payload: Any) -> UserContext | None: def has_role(self, role: str) -> bool: return role in self.roles + def has_permission(self, permission: str) -> bool: + """Exact match against :attr:`permissions` only — no roles, no wildcard. + + Gate routes with ``RequiresPermission``, which also folds in roles and + grant sources; this is for the principal's own claims alone.""" + return permission in self.permissions + def has_any_role(self, roles: list[str]) -> bool: return bool(set(self.roles) & set(roles)) diff --git a/modules/auth/tests/test_user_context.py b/modules/auth/tests/test_user_context.py index cb6df56b..40625fd9 100644 --- a/modules/auth/tests/test_user_context.py +++ b/modules/auth/tests/test_user_context.py @@ -93,3 +93,45 @@ async def test_tenant_id_default_is_none(self): async def test_roles_default_is_empty_list(self): ctx = UserContext(id="1", email="a@b.com", name="A") assert ctx.roles == [] + + async def test_permissions_default_is_empty_list(self): + ctx = UserContext(id="1", email="a@b.com", name="A") + assert ctx.permissions == [] + + async def test_positional_construction_unchanged(self): + ctx = UserContext("1", "a@b.com", "A", ["admin"], "t1") + assert ctx.tenant_id == "t1" + assert ctx.permissions == [] + + +class TestUserContextPermissions: + async def test_session_round_trip_keeps_permissions(self): + ctx = UserContext(id="1", email="a@b.com", name="A", permissions=["x.read", "x.write"]) + payload = ctx.to_session_dict() + assert payload["permissions"] == ["x.read", "x.write"] + assert UserContext.from_session_dict(payload) == ctx + + async def test_legacy_cookie_without_permissions_key(self): + """A cookie written before the field existed still decodes.""" + payload = {"id": "1", "email": "a@b.com", "name": "A", "roles": [], "tenant_id": None} + ctx = UserContext.from_session_dict(payload) + assert ctx is not None + assert ctx.permissions == [] + + async def test_null_permissions_decodes_empty(self): + payload = {"id": "1", "email": "a@b.com", "name": "A", "permissions": None} + ctx = UserContext.from_session_dict(payload) + assert ctx is not None + assert ctx.permissions == [] + + async def test_from_user_leaves_permissions_empty(self): + from types import SimpleNamespace + + user = SimpleNamespace(id=1, email="a@b.com", full_name="A", roles=[]) + assert UserContext.from_user(user).permissions == [] + + async def test_has_permission_exact_match(self): + ctx = UserContext(id="1", email="a@b.com", name="A", permissions=["x.read"]) + assert ctx.has_permission("x.read") is True + assert ctx.has_permission("x.write") is False + assert ctx.has_permission("x") is False diff --git a/modules/branding/branding/components/BrandingPreview.tsx b/modules/branding/branding/components/BrandingPreview.tsx index c7ccee01..6ab04616 100644 --- a/modules/branding/branding/components/BrandingPreview.tsx +++ b/modules/branding/branding/components/BrandingPreview.tsx @@ -1,4 +1,4 @@ -import { BRAND_FOOTER_LINKS, BRAND_LICENSE } from '@simple-module-py/ui/lib/brand'; +import { BRAND_FOOTER_LINKS } from '@simple-module-py/ui/lib/brand'; import { deriveBrandRamp } from '@simple-module-py/ui/lib/color'; import type { FooterLink } from './dirty'; @@ -76,7 +76,7 @@ function FooterStrip({ brand }: { brand: PreviewBrand }) { return (
- {brand.footerText.trim() || `© ${PREVIEW_YEAR} ${brand.appName} · ${BRAND_LICENSE}`} + {brand.footerText.trim() || `© ${PREVIEW_YEAR} ${brand.appName}`} {previewFooterLinks(brand) diff --git a/modules/branding/branding/components/FooterField.tsx b/modules/branding/branding/components/FooterField.tsx index cc8994d6..a787937a 100644 --- a/modules/branding/branding/components/FooterField.tsx +++ b/modules/branding/branding/components/FooterField.tsx @@ -17,7 +17,7 @@ export const MAX_FOOTER_HREF = 500; export const MAX_FOOTER_TEXT = 200; interface FooterFieldProps { - /** The caption line. Blank keeps the framework's own `© {year} · MIT`. */ + /** The caption line. Blank keeps the framework's own `© {year} {appName}`. */ text: string; onTextChange: (next: string) => void; links: FooterLink[]; diff --git a/modules/dashboard/dashboard/stats.py b/modules/dashboard/dashboard/stats.py index 8667098f..ccf9f493 100644 --- a/modules/dashboard/dashboard/stats.py +++ b/modules/dashboard/dashboard/stats.py @@ -127,6 +127,7 @@ def _get_module_info(app: FastAPI, health_checks: list[dict[str, str]]) -> list[ # caller's permissions here: this whole payload is process-wide cached for # 30s, so anything user-specific would leak across sessions. The page gates # each link against the per-user `menus` prop instead. + prefixes = app.state.sm.view_prefixes return [ { "name": m.meta.name, @@ -135,7 +136,7 @@ def _get_module_info(app: FastAPI, health_checks: list[dict[str, str]]) -> list[ # in a mono face and read as the thing you would `uv add`. "package": get_module_package_name(m), "status": "loaded", - "url": f"{m.meta.view_prefix}/" if m.meta.view_prefix else "", + "url": f"{prefixes[m.meta.name]}/" if m.meta.name in prefixes else "", # A partly-administrative module (users) mounts its management # screens outside its own view_prefix, so the tile cannot find # them by prefix alone — ship the second mount point too. diff --git a/modules/dashboard/tests/test_dashboard.py b/modules/dashboard/tests/test_dashboard.py index 5007a9e7..040efd9a 100644 --- a/modules/dashboard/tests/test_dashboard.py +++ b/modules/dashboard/tests/test_dashboard.py @@ -150,6 +150,22 @@ async def test_view_less_modules_get_an_empty_url( for mod in resp.json()["system_info"]["modules"]: assert mod["url"] == "" or mod["url"].startswith("/"), mod + async def test_root_mounted_module_gets_slash_url( + self, app, authenticated_client: httpx.AsyncClient, monkeypatch + ): + """#435: view_prefix "" with real view routes is `/`; without, still empty.""" + sm = app.state.sm + name = sm.modules[0].meta.name + monkeypatch.setitem(sm.view_prefixes, name, "") + resp = await authenticated_client.get(_STATS_URL) + urls = {m["name"]: m["url"] for m in resp.json()["system_info"]["modules"]} + assert urls[name] == "/" + monkeypatch.delitem(sm.view_prefixes, name) + invalidate_stats_cache() + resp = await authenticated_client.get(_STATS_URL) + urls = {m["name"]: m["url"] for m in resp.json()["system_info"]["modules"]} + assert urls[name] == "" + async def test_every_module_entry_reports_health(self, authenticated_client: httpx.AsyncClient): resp = await authenticated_client.get(_STATS_URL) for mod in resp.json()["system_info"]["modules"]: diff --git a/modules/file_storage/file_storage/backends/s3.py b/modules/file_storage/file_storage/backends/s3.py index e177cc2d..95273c7d 100644 --- a/modules/file_storage/file_storage/backends/s3.py +++ b/modules/file_storage/file_storage/backends/s3.py @@ -23,6 +23,10 @@ if TYPE_CHECKING: pass +_MISSING_S3_EXTRA = ( + "S3 backend requires the 'aioboto3' package. Install with: uv add --optional s3 aioboto3" +) + class S3Backend: """S3-compatible object storage. @@ -40,10 +44,7 @@ def __init__(self, *, bucket: str, region: str, client_kwargs: dict[str, Any]) - try: import aioboto3 except ImportError as exc: - raise ConfigurationError( - "S3 backend requires the 'aioboto3' package. " - "Install with: uv add --optional s3 aioboto3" - ) from exc + raise ConfigurationError(_MISSING_S3_EXTRA) from exc self._aioboto3 = aioboto3 self._session = aioboto3.Session() self.bucket = bucket @@ -149,6 +150,12 @@ def _build(settings: FileStorageSettings) -> S3Backend: if settings.s3_access_key_id and settings.s3_secret_access_key: client_kwargs["aws_access_key_id"] = settings.s3_access_key_id client_kwargs["aws_secret_access_key"] = settings.s3_secret_access_key + try: + from botocore.config import Config + except ImportError as exc: + raise ConfigurationError(_MISSING_S3_EXTRA) from exc + # Without an explicit version, botocore presigns SigV2 for custom endpoints. + client_kwargs["config"] = Config(signature_version=settings.s3_signature_version) return S3Backend( bucket=settings.s3_bucket, region=settings.s3_region, diff --git a/modules/file_storage/file_storage/settings.py b/modules/file_storage/file_storage/settings.py index c1d24ca2..ad3dedc9 100644 --- a/modules/file_storage/file_storage/settings.py +++ b/modules/file_storage/file_storage/settings.py @@ -13,12 +13,15 @@ from pathlib import Path -from pydantic import Field, model_validator +from pydantic import Field, field_validator, model_validator from pydantic_settings import SettingsConfigDict from simple_module_core.settings_base import DbBackedSettings from file_storage import constants +# "s3" is SigV2, kept only for legacy S3-compatible stores that lack SigV4. +_SIGNATURE_VERSIONS = frozenset({"s3v4", "s3"}) + class FileStorageSettings(DbBackedSettings): """Configuration for the file_storage module. @@ -53,6 +56,11 @@ class FileStorageSettings(DbBackedSettings): json_schema_extra={"group": "S3"}, description="Custom endpoint for MinIO / R2. Blank uses AWS default.", ) + s3_signature_version: str = Field( + default="s3v4", + json_schema_extra={"group": "S3"}, + description="Presign signature version. Keep s3v4; SigV2 is rejected by most providers.", + ) s3_presign_ttl_seconds: int = Field( default=constants.DEFAULT_PRESIGN_TTL_SECONDS, json_schema_extra={"group": "S3"}, @@ -73,6 +81,15 @@ class FileStorageSettings(DbBackedSettings): description="Total storage the browse screen measures usage against. None = unmeasured.", ) + @field_validator("s3_signature_version") + @classmethod + def _known_signature_version(cls, value: str) -> str: + # botocore only fails on an unknown version when the first client is + # built, so an admin-UI typo would otherwise surface as failed uploads. + if value not in _SIGNATURE_VERSIONS: + raise ValueError(f"s3_signature_version must be one of {sorted(_SIGNATURE_VERSIONS)}") + return value + @model_validator(mode="after") def _validate_backend_config(self) -> FileStorageSettings: """Enforce per-backend required fields. diff --git a/modules/file_storage/tests/test_s3_backend.py b/modules/file_storage/tests/test_s3_backend.py index ec3d9589..50cc3397 100644 --- a/modules/file_storage/tests/test_s3_backend.py +++ b/modules/file_storage/tests/test_s3_backend.py @@ -100,4 +100,22 @@ async def test_s3_presigned_url_format(s3_settings): url = await backend.presigned_get_url("any/key.txt", ttl_seconds=300) assert url.startswith("http") assert "any/key.txt" in url - assert "X-Amz-Signature" in url or "Signature" in url + assert "X-Amz-Algorithm=AWS4-HMAC-SHA256" in url + assert "AWSAccessKeyId=" not in url + + +async def test_s3_signature_version_is_configurable(s3_settings): + s3_settings.s3_signature_version = "s3" + backend = build_backend(s3_settings) + url = await backend.presigned_get_url("any/key.txt", ttl_seconds=300) + assert "AWSAccessKeyId=" in url + + +@pytest.mark.parametrize("value", ["s3v5", "", "v4"]) +def test_unknown_signature_version_is_rejected_at_config_time(value) -> None: + # A typo saved in the admin UI must fail validation, not every upload. + from file_storage.settings import FileStorageSettings + from pydantic import ValidationError + + with pytest.raises(ValidationError): + FileStorageSettings(s3_signature_version=value) diff --git a/modules/settings/settings/cli.py b/modules/settings/settings/cli.py index d9dd223d..08fe8ca4 100644 --- a/modules/settings/settings/cli.py +++ b/modules/settings/settings/cli.py @@ -9,9 +9,11 @@ import asyncio import os +from collections.abc import Mapping import typer from fastapi import FastAPI +from simple_module_core.dotenv import find_env_file, parse_dotenv from settings.constants import MODULE_PACKAGE from settings.env_vars import env_prefix_for @@ -21,14 +23,24 @@ app = typer.Typer(help="Settings module administration.", no_args_is_help=True) -async def import_from_env_impl(app_inst: FastAPI, store: SettingsStore) -> int: - """Write a SYSTEM override for every ``SM__`` env var set.""" +async def import_from_env_impl( + app_inst: FastAPI, + store: SettingsStore, + environ: Mapping[str, str] | None = None, +) -> int: + """Write a SYSTEM override for every ``SM__`` var set. + + Default source is the project ``.env`` overlaid by the process environment + (real env wins). ``os.environ`` is never mutated. + """ + if environ is None: + environ = {**parse_dotenv(find_env_file()), **os.environ} registry = getattr(app_inst.state, MODULE_PACKAGE).module_registry count = 0 for package, cls in registry.items(): prefix = env_prefix_for(package) for field_name in cls.model_fields: - raw = os.environ.get(f"{prefix}{field_name.upper()}") + raw = environ.get(f"{prefix}{field_name.upper()}") if raw is None: continue vtype = value_type_for_field(cls, field_name) @@ -56,6 +68,11 @@ async def run() -> int: n = await import_from_env_impl(fastapi_app, store) await session.commit() typer.echo(f"Imported {n} override(s) from environment.") + if n == 0 and find_env_file().is_file(): + typer.echo( + "Hint: variables must be named SM__ " + "(e.g. SM_USERS_SMTP_PORT); none in the environment or .env matched." + ) return 0 raise typer.Exit(code=asyncio.run(run())) diff --git a/modules/settings/settings/pages/ModulesEdit.tsx b/modules/settings/settings/pages/ModulesEdit.tsx index 23cdefc0..2a72c79f 100644 --- a/modules/settings/settings/pages/ModulesEdit.tsx +++ b/modules/settings/settings/pages/ModulesEdit.tsx @@ -18,21 +18,26 @@ type Props = { function ModulesEdit({ modules, testable = {} }: Props) { const { t } = useT(); - const [selected, setSelected] = useState(modules[0]?.package); + // A module with no fields and no page of its own has nothing to show. + const visible = useMemo( + () => modules.filter((m) => m.fields.length > 0 || m.manage_url), + [modules], + ); + const [selected, setSelected] = useState(visible[0]?.package); const [q, setQ] = useState(''); const filtered = useMemo(() => { - if (!q) return modules; + if (!q) return visible; const query = q.toLowerCase(); - return modules.filter( + return visible.filter( (m) => m.module_name.toLowerCase().includes(query) || m.package.toLowerCase().includes(query) || m.fields.some((f) => f.name.toLowerCase().includes(query)), ); - }, [modules, q]); + }, [visible, q]); - const current = modules.find((m) => m.package === selected); + const current = visible.find((m) => m.package === selected); return ( <> diff --git a/modules/settings/tests-js/ModulesEdit.test.tsx b/modules/settings/tests-js/ModulesEdit.test.tsx index 9efeec58..f0426c05 100644 --- a/modules/settings/tests-js/ModulesEdit.test.tsx +++ b/modules/settings/tests-js/ModulesEdit.test.tsx @@ -51,4 +51,36 @@ describe('ModulesEdit', () => { expect(screen.getAllByRole('heading', { level: 1 })).toHaveLength(1); }); + test('does not list a module with no fields and no manage_url, and skips it initially', () => { + render( + , + ); + + expect(screen.queryByText('alpha')).not.toBeInTheDocument(); + expect(screen.getAllByText('beta').length).toBeGreaterThan(1); // list + selected form + // Initial selection is the first *visible* module, not the hidden one. + expect(screen.queryByText('No modules with settings')).not.toBeInTheDocument(); + }); + + test('still lists a zero-field module that has a manage_url', () => { + render( + , + ); + + expect(screen.getByText('branding')).toBeInTheDocument(); + }); }); diff --git a/modules/settings/tests/test_cli_import.py b/modules/settings/tests/test_cli_import.py index 26b0628b..17c650f0 100644 --- a/modules/settings/tests/test_cli_import.py +++ b/modules/settings/tests/test_cli_import.py @@ -13,6 +13,13 @@ _FIXTURE_ENV = ("SM_AUTH_PROVIDER", "SM_BG_TASKS_BROADCAST_INVALIDATIONS") +@pytest.fixture(autouse=True) +def _isolate_project_root(monkeypatch, tmp_path) -> None: + # import_from_env reads ``.env`` via find_env_file(); pin it to an empty dir + # so no test picks up the developer's real repo ``.env``. + monkeypatch.setenv("SM_PROJECT_ROOT", str(tmp_path)) + + def _drop_fixture_env(monkeypatch) -> None: for name in _FIXTURE_ENV: monkeypatch.delenv(name, raising=False) @@ -60,3 +67,42 @@ async def test_import_ignores_unknown_env(db_session, monkeypatch, app) -> None: n = await import_from_env_impl(app, store) assert n == 0 assert await store.get_overrides("users") == {} + + +@pytest.mark.asyncio +async def test_import_reads_dotenv_by_default(db_session, monkeypatch, app, tmp_path) -> None: + _drop_fixture_env(monkeypatch) + monkeypatch.delenv("SM_USERS_SMTP_PORT", raising=False) + monkeypatch.delenv("SM_USERS_ALLOW_SIGNUP", raising=False) + (tmp_path / ".env").write_text("SM_USERS_SMTP_PORT=2626\nSM_USERS_ALLOW_SIGNUP=true\n") + monkeypatch.setenv("SM_PROJECT_ROOT", str(tmp_path)) + # Real env wins over the file. + monkeypatch.setenv("SM_USERS_ALLOW_SIGNUP", "false") + + from settings.cli import import_from_env_impl + from settings.service import SettingService + from settings.store import SettingsStore + + store = SettingsStore(SettingService(db_session)) + n = await import_from_env_impl(app, store) + + users = await store.get_overrides("users") + assert users["smtp_port"] == ("2626", "int") + assert users["allow_signup"] == ("false", "bool") + assert n == 2 + + +@pytest.mark.asyncio +async def test_import_accepts_explicit_environ(db_session, monkeypatch, app) -> None: + _drop_fixture_env(monkeypatch) + monkeypatch.delenv("SM_USERS_SMTP_PORT", raising=False) + + from settings.cli import import_from_env_impl + from settings.service import SettingService + from settings.store import SettingsStore + + store = SettingsStore(SettingService(db_session)) + n = await import_from_env_impl(app, store, environ={"SM_USERS_SMTP_PORT": "2727"}) + + assert (await store.get_overrides("users"))["smtp_port"] == ("2727", "int") + assert n == 1 diff --git a/modules/users/tests-js/LoginForm.test.tsx b/modules/users/tests-js/LoginForm.test.tsx new file mode 100644 index 00000000..3e4520a2 --- /dev/null +++ b/modules/users/tests-js/LoginForm.test.tsx @@ -0,0 +1,31 @@ +import '@testing-library/jest-dom/vitest'; +import { render, screen } from '@testing-library/react'; +import { describe, expect, test, vi } from 'vitest'; + +import { LoginForm } from '../users/auth_local/components/LoginForm'; + +const baseProps = { + email: '', + onEmailChange: vi.fn(), + password: '', + onPasswordChange: vi.fn(), + remember: false, + onRememberChange: vi.fn(), + rememberDays: 30, + loading: false, + onSubmit: vi.fn(), + allowSignup: false, + oauthProviders: [], +}; + +describe('LoginForm', () => { + test('announces a sign-in error as an alert', () => { + render(); + expect(screen.getByRole('alert')).toHaveTextContent('Invalid credentials'); + }); + + test('renders no alert without an error', () => { + render(); + expect(screen.queryByRole('alert')).toBeNull(); + }); +}); diff --git a/modules/users/tests/test_login_redirect_root_mount.py b/modules/users/tests/test_login_redirect_root_mount.py new file mode 100644 index 00000000..9cbefa8d --- /dev/null +++ b/modules/users/tests/test_login_redirect_root_mount.py @@ -0,0 +1,39 @@ +"""Login fallback with a module mounted at the root (#435).""" + +from __future__ import annotations + +import pytest + + +def _fallback_url(app, prefixes: dict[str, str]) -> str: + """Run the login fallback against a Dashboard-less module set (#435).""" + from users.startup import apply_login_redirect_fallback + + sm = app.state.sm + saved_mods, saved_prefixes = sm.modules, dict(sm.view_prefixes) + settings = app.state.users.settings + saved_url = settings.login_redirect_url + object.__setattr__(sm, "modules", tuple(m for m in saved_mods if m.meta.name != "Dashboard")) + sm.view_prefixes.clear() + sm.view_prefixes.update(prefixes) + settings.login_redirect_url = "/dashboard/" + try: + apply_login_redirect_fallback(app, settings, "Users", "/dashboard/") + return settings.login_redirect_url + finally: + object.__setattr__(sm, "modules", saved_mods) + sm.view_prefixes.clear() + sm.view_prefixes.update(saved_prefixes) + settings.login_redirect_url = saved_url + + +@pytest.mark.anyio +async def test_login_redirect_fallback_prefers_root_mounted_module(users_app): + """#435: a module that serves views at ``/`` (view_prefix == "") wins.""" + assert _fallback_url(users_app, {"Users": "/users", "Home": "", "Other": "/o"}) == "/" + + +@pytest.mark.anyio +async def test_login_redirect_fallback_skips_api_only_module(users_app): + """An API-only module also has view_prefix == "" but is not in the map.""" + assert _fallback_url(users_app, {"Users": "/users", "Other": "/o"}) == "/o/" diff --git a/modules/users/tests/test_users_shared_props.py b/modules/users/tests/test_users_shared_props.py index b0acc585..918e04a6 100644 --- a/modules/users/tests/test_users_shared_props.py +++ b/modules/users/tests/test_users_shared_props.py @@ -109,3 +109,18 @@ def test_the_built_app_carries_it(self, app) -> None: settings module, and a stub would pass while the wiring was broken.""" providers = getattr(app.state, "inertia_shared_providers", []) assert users_shared_props in providers + + +class TestHomeUrlSharedProp: + def test_reflects_login_redirect_url(self) -> None: + request = _request(SimpleNamespace(settings=SimpleNamespace(login_redirect_url="/home/"))) + assert users_shared_props(request)["homeUrl"] == "/home/" + + def test_tracks_runtime_edits(self) -> None: + settings = SimpleNamespace(login_redirect_url="/a/") + request = _request(SimpleNamespace(settings=settings)) + settings.login_redirect_url = "/b/" + assert users_shared_props(request)["homeUrl"] == "/b/" + + def test_omitted_when_settings_are_missing(self) -> None: + assert "homeUrl" not in users_shared_props(_request(SimpleNamespace())) diff --git a/modules/users/users/auth_local/components/LoginForm.tsx b/modules/users/users/auth_local/components/LoginForm.tsx index a9147107..8a47ef41 100644 --- a/modules/users/users/auth_local/components/LoginForm.tsx +++ b/modules/users/users/auth_local/components/LoginForm.tsx @@ -105,7 +105,11 @@ export function LoginForm({
- {error &&

{error}

} + {error && ( +

+ {error} +

+ )} - + = {}; +vi.mock('@inertiajs/react', () => ({ usePage: () => ({ props }) })); + +import { useHomeUrl } from './home-url'; + +describe('useHomeUrl', () => { + beforeEach(() => { + props = {}; + }); + + it('returns the homeUrl shared prop', () => { + props = { homeUrl: '/home/' }; + expect(renderHook(() => useHomeUrl()).result.current).toBe('/home/'); + }); + + it('falls back to / when absent', () => { + expect(renderHook(() => useHomeUrl()).result.current).toBe('/'); + }); + + it.each([ + 'https://evil.example/', + '//evil.example/', + '/\\evil.example/', + 'javascript:alert(1)', + 'dashboard/', + ])('falls back to / for a value that is not a same-origin path: %s', (homeUrl) => { + props = { homeUrl }; + expect(renderHook(() => useHomeUrl()).result.current).toBe('/'); + }); +}); diff --git a/packages/ui/src/lib/home-url.ts b/packages/ui/src/lib/home-url.ts new file mode 100644 index 00000000..8cd3895a --- /dev/null +++ b/packages/ui/src/lib/home-url.ts @@ -0,0 +1,12 @@ +import { usePage } from '@inertiajs/react'; +import type { SharedProps } from '../types'; + +/** + * The app's home URL from the `homeUrl` shared prop (the users module's + * `login_redirect_url`), or `/` when no module supplies one. + */ +export function useHomeUrl(): string { + const { homeUrl } = usePage<{ props: SharedProps }>().props as unknown as SharedProps; + // Same-origin paths only: the value is admin-editable and now feeds hrefs. + return homeUrl && /^\/(?![/\\])/.test(homeUrl) ? homeUrl : '/'; +} diff --git a/packages/ui/src/types.ts b/packages/ui/src/types.ts index 96901e97..5088bbe4 100644 --- a/packages/ui/src/types.ts +++ b/packages/ui/src/types.ts @@ -35,7 +35,7 @@ export interface BrandingShared { banner: { message: string; severity: string } | null; /** * Caption line set by an administrator, or `null` when unset — in which case - * the framework's own `© {year} · {BRAND_LICENSE}` is shown. + * the framework's own `© {year} {appName}` is shown. */ footerText: string | null; /** @@ -62,6 +62,10 @@ export interface SharedProps { // auth provider is installed (e.g. a Keycloak-only deployment), which reads // the same as "closed" — the host isn't the one taking signups. signup?: { allowed: boolean }; + // Where the app's "home" is (the users module's `login_redirect_url`). Layouts + // link their logo / back-to-app / dashboard entry here instead of hardcoding + // a module's URL. Absent without the users module; callers fall back to `/`. + homeUrl?: string; // Also from the users module. `active` is true only for the shared demo // account's own session — an operator signed in normally on a demo instance // sees nothing. `readOnly` mirrors the `demo_read_only` setting that makes diff --git a/tests/e2e/test_shell_home_and_signin.py b/tests/e2e/test_shell_home_and_signin.py new file mode 100644 index 00000000..d72b3484 --- /dev/null +++ b/tests/e2e/test_shell_home_and_signin.py @@ -0,0 +1,62 @@ +"""E2E coverage for the app shell's home links, footer and sign-in error. + +* the admin shell renders the shared ``ui`` catalog, never raw ``ui.*`` keys + (#426), and its way back to the app follows the ``homeUrl`` shared prop + rather than a hardcoded ``/dashboard/`` (#427); +* the default footer caption names the app, not the framework's licence (#428); +* a failed sign-in is announced to assistive technology (#437). +""" + +from __future__ import annotations + +import re + +import pytest +from playwright.sync_api import Page, expect + +pytestmark = pytest.mark.e2e + +_RAW_UI_KEY = re.compile(r"\bui\.[a-z_]+\.[a-z_]+") + + +def _login(page: Page, username: str, password: str) -> None: + page.goto("/users/login") + page.locator("#email").fill(username) + page.locator("#password").fill(password) + page.get_by_role("button", name="Sign in").click() + page.wait_for_url("**/dashboard/**", timeout=15_000) + + +def test_admin_shell_is_translated_and_links_home( + page: Page, e2e_username: str, e2e_password: str +) -> None: + _login(page, e2e_username, e2e_password) + page.goto("/admin/users/") + back = page.get_by_role("link", name="Back to app") + expect(back).to_be_visible() + assert not _RAW_UI_KEY.search(page.locator("body").inner_text()) + + home = page.evaluate( + "() => JSON.parse(document.getElementById('app').dataset.page).props.homeUrl" + ) + assert home.startswith("/") + expect(back).to_have_attribute("href", home) + back.click() + page.wait_for_url(f"**{home}**") + + +def test_default_footer_names_the_app_not_a_licence(page: Page) -> None: + # The public landing page renders BrandingFooter; the sign-in page has no footer. + page.goto("/") + footer = page.locator("footer").last + expect(footer).to_contain_text("©") + assert "MIT" not in footer.inner_text() + + +def test_failed_sign_in_is_announced(page: Page, e2e_username: str) -> None: + # One deliberate failure: the limiter allows five per window. + page.goto("/users/login") + page.locator("#email").fill(e2e_username) + page.locator("#password").fill("definitely-not-the-password") + page.get_by_role("button", name="Sign in").click() + expect(page.get_by_role("alert")).to_be_visible()