diff --git a/CHANGELOG.md b/CHANGELOG.md index 612ae7cb9..c436a0ea2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,18 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] ### Changed +- **Behaviour change (#486):** for settings classes that read the environment + (today `background_tasks`, `SM_BG_TASKS_*`), an env var now beats a value + stored from the admin UI, and `SM_REDIS_URL` beats a stored broker/result + backend. Saving a field the environment sets returns a 422 on that field. +- **Behaviour change (#488):** self-registered users now get the standard + `user` role; OAuth-provisioned users get it only while `allow_signup` is on. + A new data migration (`b5018b617142`) seeds the role on **fresh installs + only** (no rows in `users_user`), so a role an admin deleted is never brought + back. On an existing install without a `user` role, signups still get no + role and a warning is logged — create the role in Admin → Roles to opt in. +- **Contract change (#489):** `StorageBackend.presigned_get_url` takes + keyword-only `content_disposition` / `content_type`. - **Behaviour change (#456):** the OpenAPI schema moved from `/openapi.json` to `/api/openapi.json`, and is no longer served outside development unless `openapi_public` (`SM_OPENAPI_PUBLIC`) is on. The bare `/openapi.json` was @@ -300,6 +312,19 @@ All notable changes to this project are documented in this file. The format is b `tenants` resolver it selects among the user's own memberships. ### Fixed +- Settings editor: list/dict fields are edited and stored as JSON, and + `X | None` fields as their real type with empty meaning unset (#487). + Secrets inside such fields are stored intact, and any field declared with + `SecretStr`/`SecretBytes` is masked on every read path whatever its name. +- Module settings classes without an `env_prefix` are no longer pinned by + unrelated bare env vars (`HOME`, `USER`, ...) (#486). +- S3 presigned downloads keep the original filename and content type, and the + streamed download no longer 500s on a non-Latin-1 filename (#489). +- The scaffolded `run_worker.py` hydrates DB-backed `background_tasks` + settings like the repo's own worker, via + `background_tasks.runner.build_runner_celery()` (#490). +- A failing verification email no longer leaves a new signup without its role + (#488). - The Celery worker/beat process now hydrates DB-backed `background_tasks` settings at boot (it used to read the environment only, so admin edits reached the web process but never the worker). diff --git a/docs/framework/invalidation.md b/docs/framework/invalidation.md index 140a60807..50a4a42d6 100644 --- a/docs/framework/invalidation.md +++ b/docs/framework/invalidation.md @@ -71,7 +71,7 @@ A transport failure never propagates. The caller has already committed; turning ## Operating it -`background_tasks` installs the transport during `on_startup`, on its own `broker_url` — which resolves as stored override → `SM_BG_TASKS_BROKER_URL` (deprecated) → `SM_REDIS_URL` → the localhost default. So it always follows Celery's broker, by construction rather than by convention. +`background_tasks` installs the transport during `on_startup`, on its own `broker_url` — which resolves as `SM_BG_TASKS_BROKER_URL` (deprecated) → `SM_REDIS_URL` → stored override → the localhost default. So it always follows Celery's broker, by construction rather than by convention. | Setting | Default | What it does | |---|---|---| diff --git a/docs/modules/background_tasks.md b/docs/modules/background_tasks.md index 846ef8c65..3ca65c051 100644 --- a/docs/modules/background_tasks.md +++ b/docs/modules/background_tasks.md @@ -126,7 +126,7 @@ Entry values are plain Celery [beat entries](https://docs.celeryq.dev/en/stable/ Beat persists its schedule (`celery.beat:PersistentScheduler`) so it survives restarts. -The worker and beat read the same DB-backed settings the web process does: `scripts/run_worker.py` hydrates `BackgroundTasksSettings` from the database at boot exactly as the web lifespan does (`cls(**db_overrides)`: a stored value wins over the same field's environment variable, and the environment seeds only what the database leaves alone), so an admin-edited queue or time limit reaches the worker after a restart. Because the web process behaves identically, this adds no new trust boundary; still, treat write access to these settings as control of the workers. If the database cannot be read the worker starts from env and defaults and logs a warning. +The worker and beat read the same DB-backed settings the web process does: `scripts/run_worker.py` (and the copy `smpy new` scaffolds) calls `background_tasks.runner.build_runner_celery()`, which hydrates `BackgroundTasksSettings` from the database at boot exactly as the web lifespan does. Precedence is env → DB → default: an `SM_BG_TASKS_*` variable that is set wins over a stored value for the same field, and the database supplies everything the environment leaves unset. So an admin-edited queue or time limit reaches the worker after a restart, unless the deployment pins that field in its environment. Because the web process behaves identically, this adds no new trust boundary; still, treat write access to these settings as control of the workers. If the database cannot be read the worker starts from env and defaults and logs a warning. ## Routing and queues diff --git a/docs/reference/env-vars.md b/docs/reference/env-vars.md index 51543838b..eaa038ce6 100644 --- a/docs/reference/env-vars.md +++ b/docs/reference/env-vars.md @@ -75,7 +75,7 @@ Most users settings (signup, mailer, SMTP, OAuth/OIDC credentials, rate limits) Celery config (`broker_url`, `result_backend`, queue, retention, etc.) is now **DB-backed** — edit it in the admin UI at `/admin/settings/` under BackgroundTasks. Defaults are `redis://localhost:6379/0` (broker) and `/1` (result backend). Because workers read the broker/backend once at process start, changing those values requires a worker restart. -Three fields are still read from the environment, because they have to work *before* any DB row exists. A stored DB value still wins once hydration runs. +Three fields are still read from the environment, because they have to work *before* any DB row exists. When one of these variables is set it wins over a stored DB value for that field (env → DB → default), and so does `SM_REDIS_URL` for both the broker and the result backend. The admin UI refuses to save a field the environment sets. | Variable | Default | Notes | |---|---|---| diff --git a/framework/cli/simple_module_cli/templates/host/_optional/background_tasks/run_worker.py b/framework/cli/simple_module_cli/templates/host/_optional/background_tasks/run_worker.py index 4ef964121..7b57d8f1a 100644 --- a/framework/cli/simple_module_cli/templates/host/_optional/background_tasks/run_worker.py +++ b/framework/cli/simple_module_cli/templates/host/_optional/background_tasks/run_worker.py @@ -1,26 +1,24 @@ """Entry point for the Celery worker and beat services. -Both the web process and the worker go through the same -``background_tasks.celery_app.build_celery`` factory so the broker -config, autodiscovered tasks, and signal handlers stay in lockstep. +Usage:: -Run locally: uv run celery -A scripts.run_worker:celery worker -l info uv run celery -A scripts.run_worker:celery beat -l info + +Both the web process and the worker go through the same +:func:`background_tasks.celery_app.build_celery` factory, so the broker +config, autodiscovered tasks, and signal handlers stay in lockstep. The +bootstrap around it (``.env``, host settings, DB-hydrated module settings) +lives in :mod:`background_tasks.runner`, shared with scaffolded apps. """ from __future__ import annotations from pathlib import Path -from simple_module_core.dotenv import load_dotenv_into_environ - -# Match the precedence uvicorn gives the web process: load ``.env`` into the -# environment before importing settings, so the worker doesn't fall back to -# SQLite defaults when celery is launched outside the host's cwd. -load_dotenv_into_environ(Path(__file__).resolve().parent.parent / ".env") - -from background_tasks.celery_app import build_celery # noqa: E402 -from background_tasks.settings import BackgroundTasksSettings # noqa: E402 +from background_tasks.runner import build_runner_celery -celery = build_celery(BackgroundTasksSettings()) +# Module-level name ``celery`` is what ``celery -A scripts.run_worker:celery`` +# looks for. Keep it stable. The fallback is the repo-root ``.env`` (next to +# ``.env.example``, not under ``host/``). +celery = build_runner_celery(Path(__file__).resolve().parent.parent / ".env") diff --git a/framework/cli/tests/test_cli_recipes.py b/framework/cli/tests/test_cli_recipes.py index 02d67ffdf..e592139f3 100644 --- a/framework/cli/tests/test_cli_recipes.py +++ b/framework/cli/tests/test_cli_recipes.py @@ -32,8 +32,22 @@ def test_recipe_writes_run_worker_script(tmp_path: Path) -> None: script = tmp_path / "scripts" / "run_worker.py" assert script.is_file() text = script.read_text() - assert "from background_tasks.celery_app import build_celery" in text - assert "celery = build_celery(BackgroundTasksSettings())" in text + # GH #490: the shared runner, which hydrates DB-backed settings — not a + # local copy built from the environment alone. + assert "from background_tasks.runner import build_runner_celery" in text + assert "celery = build_runner_celery(" in text + assert "BackgroundTasksSettings()" not in text + + +def test_scaffolded_worker_matches_the_repo_worker() -> None: + """Two copies of the worker bootstrap drifted once (GH #490); keep them one.""" + from simple_module_cli import recipes + + root = Path(__file__).resolve().parents[3] + template = Path(recipes.__file__).parent / "templates/host/_optional/background_tasks" + assert (template / "run_worker.py").read_text() == ( + root / "scripts" / "run_worker.py" + ).read_text() def test_recipe_leaves_docker_assets_to_the_scaffold(tmp_path: Path) -> None: diff --git a/host/migrations/versions/b5018b617142_users_seed_user_role.py b/host/migrations/versions/b5018b617142_users_seed_user_role.py new file mode 100644 index 000000000..25aca0269 --- /dev/null +++ b/host/migrations/versions/b5018b617142_users_seed_user_role.py @@ -0,0 +1,72 @@ +"""users_role: seed the standard ``user`` role (GH #488) + +Self-registered accounts are given the ``user`` role, which the manager looks +up rather than creates (a role an admin deleted must stay deleted). Nothing +inserted it on a fresh install, so every signup came out with no role and no +permissions. This inserts it, but only on a fresh install: when +``users_user`` has no rows and no role with that name or id exists. + +An install that already has users is left alone. There, a missing ``user`` +role most likely means an admin deleted it on purpose, and re-inserting it on +upgrade would silently hand signups (and everything mapped onto ``user``) the +access that admin removed. Such an install keeps the manager's "no role" +warning path until an admin creates the role. + +The values are inlined, not imported from ``users.constants``: a migration +must keep meaning what it meant when it was written. + +Downgrade is a no-op: the role may be assigned to users by then, and an +existing install may have had it before this revision ran. + +Revision ID: b5018b617142 +Revises: e878882924e7 +Create Date: 2026-10-18 12:30:00.000000 +""" + +import uuid +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op +from fastapi_users_db_sqlalchemy.generics import GUID + +# revision identifiers, used by Alembic. +revision: str = "b5018b617142" +down_revision: str | None = "e878882924e7" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +_USER_ROLE_ID = uuid.UUID("00000000-0000-0000-0000-000000000002") +_USER_ROLE_NAME = "user" +_USER_ROLE_DESCRIPTION = "Standard user" + +_role = sa.table( + "users_role", + sa.column("id", GUID()), + sa.column("name", sa.String(64)), + sa.column("description", sa.String(255)), +) +_user = sa.table("users_user", sa.column("id", GUID())) + + +def upgrade() -> None: + bind = op.get_bind() + if bind.execute(sa.select(sa.func.count()).select_from(_user)).scalar_one(): + return # an existing install: never undo an admin's deletion + existing = bind.execute( + sa.select(sa.func.count()) + .select_from(_role) + .where(sa.or_(_role.c.name == _USER_ROLE_NAME, _role.c.id == _USER_ROLE_ID)) + ).scalar_one() + if existing: + return + bind.execute( + _role.insert().values( + id=_USER_ROLE_ID, name=_USER_ROLE_NAME, description=_USER_ROLE_DESCRIPTION + ) + ) + + +def downgrade() -> None: + # Never delete a role that may be assigned to users. + pass diff --git a/modules/api_tokens/tests/test_api_tokens_signup.py b/modules/api_tokens/tests/test_api_tokens_signup.py new file mode 100644 index 000000000..7e3fabd42 --- /dev/null +++ b/modules/api_tokens/tests/test_api_tokens_signup.py @@ -0,0 +1,85 @@ +"""A self-registered user can reach their tokens page on a fresh install (GH #488). + +``api_tokens.use`` is mapped onto the standard ``user`` role and onto tenant +roles. A fresh install is single-tenant, so there is no membership to fall +back on: signups only get the permission if registration attaches the role. +Hence ``multi_tenant`` off here — with it on, the tenant role hides the bug. +""" + +from __future__ import annotations + +import pytest +from _api_tokens_support import INERTIA_HEADERS +from simple_module_hosting.settings import Settings +from simple_module_test.tenant_client import session_client +from sqlalchemy import select + + +@pytest.fixture +def settings(settings: Settings) -> Settings: + return settings.model_copy(update={"multi_tenant": False, "tenant_header": ""}) + + +EMAIL = "signup@example.com" + + +async def _register(app, client) -> str: + from users.bootstrap import ensure_user_role + from users.models import User + + # Test schemas come from ``create_all``: seed the role the migration would. + async with app.state.sm.db.session_factory() as session: + await ensure_user_role(session) + await session.commit() + users = app.state.users + users.settings = users.settings.model_copy(update={"allow_signup": True}) + resp = await client.post( + "/api/users/auth/register", json={"email": EMAIL, "password": "SecurePass1!"} + ) + assert resp.status_code == 201, resp.text + async with app.state.sm.db.session_factory() as session: + user = (await session.execute(select(User).where(User.email == EMAIL))).scalar_one() + user.is_verified = True + await session.commit() + return str(user.id) + + +async def test_registration_attaches_the_user_role(app, client): + from users.constants import USER_ROLE_NAME + from users.models import Role, UserRole + + user_id = await _register(app, client) + async with app.state.sm.db.session_factory() as session: + names = ( + await session.execute( + select(Role.name).join(UserRole).where(UserRole.user_id == user_id) + ) + ).scalars() + assert list(names) == [USER_ROLE_NAME] + + +async def test_registered_user_can_open_the_tokens_page(app, client): + from api_tokens.constants import USER_URL + + user_id = await _register(app, client) + async with session_client(app, {"user_id": user_id}) as c: + resp = await c.get(USER_URL, headers=INERTIA_HEADERS) + assert resp.status_code == 200, resp.text + + +async def test_admin_created_user_gets_only_the_roles_picked(app, authenticated_client): + """The admin flow calls ``create()`` with no request: no role picked, no role.""" + from users.models import User, UserRole + + resp = await authenticated_client.post( + "/api/users/admin", + json={"email": "picked@example.com", "password": "SecurePass1!", "role_names": []}, + ) + assert resp.status_code == 201, resp.text + async with app.state.sm.db.session_factory() as session: + roles = ( + await session.execute( + select(UserRole).join(User).where(User.email == "picked@example.com") + ) + ).all() + assert roles == [] diff --git a/modules/background_tasks/background_tasks/runner.py b/modules/background_tasks/background_tasks/runner.py new file mode 100644 index 000000000..41c145418 --- /dev/null +++ b/modules/background_tasks/background_tasks/runner.py @@ -0,0 +1,71 @@ +"""Build the Celery app for a worker or beat process. + +The repo's ``scripts/run_worker.py`` and the one ``smpy new`` scaffolds both +call :func:`build_runner_celery`. They used to carry their own copies of this +bootstrap, and the scaffolded copy fell behind: it built Celery from the +environment alone, so a generated app's worker ignored settings saved in the +admin UI and could read a different ``.env`` from the web process (GH #490). +""" + +from __future__ import annotations + +import os +from pathlib import Path +from typing import TYPE_CHECKING + +from simple_module_core.dotenv import find_env_file, load_dotenv_into_environ + +if TYPE_CHECKING: + from celery import Celery + +_PROJECT_ROOT = "SM_PROJECT_ROOT" + + +def _load_env(fallback_env_file: Path | None) -> None: + """Load ``.env`` the way the web process resolves it. + + ``find_env_file`` (SM_PROJECT_ROOT, then a bounded walk-up) keeps the worker + and the web process on the same file. ``fallback_env_file`` applies only + when that resolves to no file at all — celery launched from an unrelated + cwd, possibly inside some other checkout, with SM_PROJECT_ROOT unset. It is + adopted by setting SM_PROJECT_ROOT rather than by loading the file directly, + so every later reader (``BootstrapSettings``' sqlite anchor, the host + settings' ``.env`` scan) resolves the same project instead of the cwd. + """ + env_file = find_env_file() + if ( + fallback_env_file is not None + and _PROJECT_ROOT not in os.environ + and not env_file.is_file() + and fallback_env_file.is_file() + ): + os.environ[_PROJECT_ROOT] = str(fallback_env_file.parent) + env_file = find_env_file() + load_dotenv_into_environ(env_file) + + +def build_runner_celery(fallback_env_file: Path | None = None) -> Celery: + """``.env`` → host settings → DB-hydrated ``BackgroundTasksSettings`` → Celery.""" + _load_env(fallback_env_file) + + # Imported after ``.env`` is in the environment, so nothing below can + # resolve a setting before it is. + from simple_module_hosting import merge_host_settings + + from background_tasks.celery_app import build_celery + from background_tasks.sync_db import set_database_url + from background_tasks.tenant_context import set_default_tenant + from background_tasks.worker_settings import load_runner_settings + + # The worker never builds the app, so resolve the host settings the same + # way host/main.py does (env → DB → default): task bodies must hit the same + # database with the same fail-closed tenant rules as request code. + host = merge_host_settings() + default_tenant = None if host.multi_tenant else host.default_tenant or None + set_database_url( + host.database_url, tenant_strict=host.multi_tenant, default_tenant=default_tenant + ) + set_default_tenant(default_tenant) + # Hydrated from the DB like the web process, so an admin-edited setting + # reaches the worker at boot, not only the web app. + return build_celery(load_runner_settings()) diff --git a/modules/background_tasks/background_tasks/settings.py b/modules/background_tasks/background_tasks/settings.py index 966bad4bd..220c36689 100644 --- a/modules/background_tasks/background_tasks/settings.py +++ b/modules/background_tasks/background_tasks/settings.py @@ -9,13 +9,17 @@ seeds both. They have to work *before* any DB row exists: the production validator below rejects the localhost defaults, so without them a containerised app can't boot far enough to hydrate settings, and a worker process (which -never sees ``app.state``) has no other source at all. A DB value still wins -once hydration runs, and the two fields keep separate overrides for anyone who -wants the broker and the backend on different Redis databases. +never sees ``app.state``) has no other source at all. Like every env var it +beats a stored value (env → DB → default): ``env_seeded_fields`` reports both +fields to the hydrator, since ``SM_REDIS_URL`` is read by their default +factories where pydantic's env source cannot see it. With it unset, the two +fields keep separate DB overrides for anyone who wants the broker and the +backend on different Redis databases. ``SM_BG_TASKS_BROKER_URL`` and ``SM_BG_TASKS_RESULT_BACKEND`` still work and take precedence over ``SM_REDIS_URL`` — a deployment that set them meant them — -but log a deprecation warning. They are answered by pydantic's own env source +but log a deprecation warning. Being real env reads, they also beat a stored +value (env → DB → default, GH #486). They are answered by pydantic's own env source now that ``env_prefix`` is set (GH #283), which is also what stops every other field being readable from its bare, unprefixed name. @@ -87,6 +91,13 @@ class BackgroundTasksSettings(BaseSettings): # and the documented ``SM_BG_TASKS_*`` names did nothing (GH #283). model_config = SettingsConfigDict(extra="ignore", env_prefix=ENV_PREFIX) + @classmethod + def env_seeded_fields(cls) -> frozenset[str]: + """Fields the environment sets through a default factory, not a source.""" + if os.environ.get(ENV_REDIS_URL): + return frozenset({"broker_url", "result_backend"}) + return frozenset() + broker_url: str = Field( default_factory=lambda: _redis_from_env(DEFAULT_BROKER_URL), json_schema_extra=_CELERY_RESTART, diff --git a/modules/background_tasks/background_tasks/worker_settings.py b/modules/background_tasks/background_tasks/worker_settings.py index 1328615ce..9c36f45f1 100644 --- a/modules/background_tasks/background_tasks/worker_settings.py +++ b/modules/background_tasks/background_tasks/worker_settings.py @@ -69,8 +69,7 @@ def load_runner_settings() -> BackgroundTasksSettings: an admin's DB-backed edit (a queue, a time limit) reached the web process but never the worker, and the two disagreed on config. Hydration is the same call the web lifespan makes (``cls(**db_overrides)``), - so precedence matches the web process: a stored value beats the same field's - environment variable, and env only seeds what the DB leaves alone. If the DB is + so precedence matches the web process: env → DB → default (GH #486). If the DB is unreachable or not yet migrated the worker still boots on env/defaults, loudly, rather than refusing to start. """ diff --git a/modules/background_tasks/tests/test_bg_redis_url.py b/modules/background_tasks/tests/test_bg_redis_url.py index e23456634..289584ec6 100644 --- a/modules/background_tasks/tests/test_bg_redis_url.py +++ b/modules/background_tasks/tests/test_bg_redis_url.py @@ -3,8 +3,9 @@ Celery namespaces result keys as ``celery-task-meta-*``, so a broker and a result backend can share one Redis database safely — that is the configuration upstream's own quickstart uses. Splitting them across two databases stays -possible through the DB-backed per-field overrides; it just is not something -an operator should have to decide before the app will boot. +possible through the DB-backed per-field overrides (with ``SM_REDIS_URL`` +unset, since env beats the DB) or the two legacy vars; it just is not +something an operator should have to decide before the app will boot. The legacy vars keep working because ``smpy_gis``, ``smpy_saas``, ``laco_wiki_python`` and the ``nodes-k8s`` manifests all set them. Breaking @@ -104,3 +105,43 @@ def test_redis_url_satisfies_the_production_validator( settings = BackgroundTasksSettings() assert settings.broker_url == "redis://redis:6379/0" + + +_STORED = { + "broker_url": ("redis://stored:6379/0", "string"), + "result_backend": ("redis://stored:6379/1", "string"), +} + + +def test_redis_url_beats_a_stored_value(monkeypatch: pytest.MonkeyPatch) -> None: + """env → DB → default holds for the shared variable too, though only a + default factory reads it.""" + from settings.hydrate import build_settings + + monkeypatch.setenv("SM_REDIS_URL", "redis://cache:6379/2") + + settings = build_settings(BackgroundTasksSettings, _STORED) + + assert settings.broker_url == "redis://cache:6379/2" + assert settings.result_backend == "redis://cache:6379/2" + + +def test_stored_values_apply_without_redis_url() -> None: + from settings.hydrate import build_settings + + settings = build_settings(BackgroundTasksSettings, _STORED) + + assert settings.broker_url == "redis://stored:6379/0" + assert settings.result_backend == "redis://stored:6379/1" + + +def test_legacy_var_still_beats_redis_url_and_the_db(monkeypatch: pytest.MonkeyPatch) -> None: + from settings.hydrate import build_settings + + monkeypatch.setenv("SM_REDIS_URL", "redis://cache:6379/2") + monkeypatch.setenv("SM_BG_TASKS_BROKER_URL", "redis://legacy:6379/0") + + settings = build_settings(BackgroundTasksSettings, _STORED) + + assert settings.broker_url == "redis://legacy:6379/0" + assert settings.result_backend == "redis://cache:6379/2" diff --git a/modules/background_tasks/tests/test_bg_runner_settings.py b/modules/background_tasks/tests/test_bg_runner_settings.py index 0c20601e5..0605e5aa6 100644 --- a/modules/background_tasks/tests/test_bg_runner_settings.py +++ b/modules/background_tasks/tests/test_bg_runner_settings.py @@ -2,6 +2,7 @@ from __future__ import annotations +import os from collections.abc import Iterator import pytest @@ -91,3 +92,68 @@ def test_stored_invalid_task_routes_do_not_stop_the_worker(db_url: str, caplog) assert cfg.task_routes_override == {} assert cfg.task_time_limit == 90 # the module's other overrides survive assert "task_routes_override" in caplog.text + + +def test_shared_runner_builds_celery_from_db_settings(db_url, tmp_path, monkeypatch) -> None: + """GH #490: the entry point scaffolded apps use must hydrate from the DB.""" + from background_tasks import tenant_context + from background_tasks.runner import build_runner_celery + + # The runner sets process-wide state; don't leak it into later tests. + monkeypatch.setattr(tenant_context, "_default_tenant", tenant_context._default_tenant) + for name in ("_tenant_strict", "_default_tenant"): + monkeypatch.setattr(sync_db, name, getattr(sync_db, name)) + _put(db_url, "background_tasks.task_default_queue", "from-db", "string") + monkeypatch.setenv("SM_PROJECT_ROOT", str(tmp_path)) + monkeypatch.setenv("SM_DATABASE_URL", db_url.replace("sqlite://", "sqlite+aiosqlite://")) + monkeypatch.delenv("SM_BG_TASKS_TASK_DEFAULT_QUEUE", raising=False) + + celery = build_runner_celery() + + assert celery.conf.task_default_queue == "from-db" + + +def _forget_after_test(monkeypatch, *names: str) -> None: + """Unset ``names`` now and again at teardown, even if the code under test sets them.""" + for name in names: + monkeypatch.setenv(name, "") + monkeypatch.delenv(name) + + +def test_fallback_env_file_is_adopted_as_the_project_root(tmp_path, monkeypatch) -> None: + """Launched from inside some other checkout, the walk-up stops at *its* + missing ``.env``; the script-relative file must still win, and the sqlite + anchor must follow it rather than the cwd (GH #490 review).""" + from background_tasks.runner import _load_env + from simple_module_hosting.bootstrap_settings import BootstrapSettings + + app = tmp_path / "app" + app.mkdir() + (app / ".env").write_text( + "SM_DATABASE_URL=sqlite+aiosqlite:///./app.db\nSM_TEST_RUNNER_MARKER=loaded\n" + ) + other = tmp_path / "other" + (other / ".git").mkdir(parents=True) + monkeypatch.chdir(other) + _forget_after_test(monkeypatch, "SM_PROJECT_ROOT", "SM_DATABASE_URL", "SM_TEST_RUNNER_MARKER") + + _load_env(app / ".env") + + assert os.environ["SM_PROJECT_ROOT"] == str(app) + assert os.environ["SM_TEST_RUNNER_MARKER"] == "loaded" + assert BootstrapSettings().database_url == f"sqlite+aiosqlite:///{(app / 'app.db').resolve()}" + + +def test_explicit_project_root_is_never_overridden(tmp_path, monkeypatch) -> None: + from background_tasks.runner import _load_env + + app = tmp_path / "app" + app.mkdir() + (app / ".env").write_text("SM_TEST_RUNNER_MARKER=fallback\n") + _forget_after_test(monkeypatch, "SM_TEST_RUNNER_MARKER") + monkeypatch.setenv("SM_PROJECT_ROOT", str(tmp_path / "chosen")) + + _load_env(app / ".env") + + assert os.environ["SM_PROJECT_ROOT"] == str(tmp_path / "chosen") + assert "SM_TEST_RUNNER_MARKER" not in os.environ diff --git a/modules/file_storage/file_storage/backends/filesystem.py b/modules/file_storage/file_storage/backends/filesystem.py index f2c7505ba..52ff6aea2 100644 --- a/modules/file_storage/file_storage/backends/filesystem.py +++ b/modules/file_storage/file_storage/backends/filesystem.py @@ -85,7 +85,14 @@ async def delete(self, key: str) -> None: async def exists(self, key: str) -> bool: return self._resolve(key).exists() - async def presigned_get_url(self, key: str, ttl_seconds: int) -> str: + async def presigned_get_url( + self, + key: str, + ttl_seconds: int, + *, + content_disposition: str | None = None, + content_type: str | None = None, + ) -> str: raise NotSupportedError("Filesystem backend cannot mint presigned URLs.") diff --git a/modules/file_storage/file_storage/backends/s3.py b/modules/file_storage/file_storage/backends/s3.py index 040c8c343..cbc1ae832 100644 --- a/modules/file_storage/file_storage/backends/s3.py +++ b/modules/file_storage/file_storage/backends/s3.py @@ -139,13 +139,26 @@ async def exists(self, key: str) -> bool: return False raise StorageBackendError(f"S3 head_object failed: {exc}") from exc - async def presigned_get_url(self, key: str, ttl_seconds: int) -> str: + async def presigned_get_url( + self, + key: str, + ttl_seconds: int, + *, + content_disposition: str | None = None, + content_type: str | None = None, + ) -> str: + params: dict[str, str] = {"Bucket": self.bucket, "Key": key} + # Signed into the URL, so S3 answers with these headers instead of the + # object's stored metadata — without them the browser names the file + # after its storage key (GH #489). + if content_disposition: + params["ResponseContentDisposition"] = content_disposition + if content_type: + params["ResponseContentType"] = content_type try: async with self._presign_client() as client: return await client.generate_presigned_url( - "get_object", - Params={"Bucket": self.bucket, "Key": key}, - ExpiresIn=ttl_seconds, + "get_object", Params=params, ExpiresIn=ttl_seconds ) except Exception as exc: raise StorageBackendError(f"presign failed: {exc}") from exc diff --git a/modules/file_storage/file_storage/contracts/service.py b/modules/file_storage/file_storage/contracts/service.py index 5fda50960..7f2958fbc 100644 --- a/modules/file_storage/file_storage/contracts/service.py +++ b/modules/file_storage/file_storage/contracts/service.py @@ -89,9 +89,21 @@ async def delete(self, key: str) -> None: async def exists(self, key: str) -> bool: """Return whether an object exists at ``key``.""" - async def presigned_get_url(self, key: str, ttl_seconds: int) -> str: + async def presigned_get_url( + self, + key: str, + ttl_seconds: int, + *, + content_disposition: str | None = None, + content_type: str | None = None, + ) -> str: """Return a short-lived URL the client can fetch directly. + ``content_disposition`` / ``content_type``, when given, are the headers + the fetched response must carry — the original filename and type, which + the backend's own object metadata may not hold. Backends that can sign + response-header overrides (S3's ``ResponseContentDisposition``) must. + Implementations without presigned-URL support must raise :class:`NotSupportedError`. The endpoint will not call this on backends with ``supports_presigned_url=False``. diff --git a/modules/file_storage/file_storage/downloads.py b/modules/file_storage/file_storage/downloads.py new file mode 100644 index 000000000..c12218d19 --- /dev/null +++ b/modules/file_storage/file_storage/downloads.py @@ -0,0 +1,60 @@ +"""How a stored file leaves the app: result shapes and download headers. + +``Content-Disposition`` (RFC 6266 / 5987) lives here so the service (presigned +redirects) and the serving helpers (streamed bodies) build it the same way +without importing each other. +""" + +from __future__ import annotations + +from collections.abc import AsyncIterator +from dataclasses import dataclass +from typing import TYPE_CHECKING +from urllib.parse import quote + +if TYPE_CHECKING: + from file_storage.contracts.service import StorageBackend + from file_storage.models import StoredFile + + +@dataclass +class StreamDownload: + """Response shape for backends that proxy bytes through the app.""" + + file: StoredFile + body: AsyncIterator[bytes] | None # None when the caller opted out via open_body=False + + +@dataclass +class RedirectDownload: + """Response shape for backends that issue presigned URLs.""" + + file: StoredFile + url: str + + +Download = StreamDownload | RedirectDownload + + +def content_disposition(filename: str, *, attachment: bool) -> str: + ascii_name = "".join( + c for c in filename.encode("ascii", "ignore").decode() if c.isprintable() and c not in '"\\' + ) + kind = "attachment" if attachment else "inline" + return f"{kind}; filename=\"{ascii_name or 'file'}\"; filename*=UTF-8''{quote(filename)}" + + +async def presigned_download_url( + backend: StorageBackend, row: StoredFile, ttl_seconds: int, *, attachment: bool +) -> str: + """A presigned URL whose response carries ``row``'s own name and type. + + Without the overrides the backend answers with the object's stored + metadata, and the browser saves the file under its opaque key (GH #489). + """ + return await backend.presigned_get_url( + row.key, + ttl_seconds, + content_disposition=content_disposition(row.filename, attachment=attachment), + content_type=row.content_type, + ) diff --git a/modules/file_storage/file_storage/endpoints/api.py b/modules/file_storage/file_storage/endpoints/api.py index f599537fa..761a58986 100644 --- a/modules/file_storage/file_storage/endpoints/api.py +++ b/modules/file_storage/file_storage/endpoints/api.py @@ -33,6 +33,7 @@ ) from file_storage.contracts.service import StorageNotFoundError from file_storage.deps import get_event_bus, get_file_storage_service +from file_storage.downloads import content_disposition from file_storage.format import format_bytes from file_storage.ranges import file_body_response from file_storage.service import ( @@ -208,7 +209,7 @@ async def download_file( row, request, headers={ - "Content-Disposition": f'attachment; filename="{row.filename}"', + "Content-Disposition": content_disposition(row.filename, attachment=True), "ETag": etag, }, etag=etag, diff --git a/modules/file_storage/file_storage/service.py b/modules/file_storage/file_storage/service.py index e17a8ff51..b8de759b0 100644 --- a/modules/file_storage/file_storage/service.py +++ b/modules/file_storage/file_storage/service.py @@ -12,7 +12,6 @@ import logging import uuid from collections.abc import AsyncIterator, Sequence -from dataclasses import dataclass from datetime import UTC, datetime from pathlib import Path from typing import TYPE_CHECKING @@ -24,6 +23,12 @@ from file_storage import constants, queries, thumbnails from file_storage.contracts.schemas import StoredFileOut from file_storage.contracts.service import StorageNotFoundError +from file_storage.downloads import ( + Download, + RedirectDownload, + StreamDownload, + presigned_download_url, +) from file_storage.models import StoredFile from file_storage.reads import FileStorageReads from file_storage.scope import PLATFORM_TENANT_ID, owning_tenant, platform_scope @@ -50,25 +55,6 @@ class StoredFileNotFoundError(Exception): """Raised when a StoredFile lookup misses; endpoints map this to HTTP 404.""" -@dataclass -class StreamDownload: - """Response shape for backends that proxy bytes through the app.""" - - file: StoredFile - body: AsyncIterator[bytes] | None # None when the caller opted out via open_body=False - - -@dataclass -class RedirectDownload: - """Response shape for backends that issue presigned URLs.""" - - file: StoredFile - url: str - - -Download = StreamDownload | RedirectDownload - - class FileStorageService(FileStoragePublic, FileStorageReads): """Orchestrates validation, hashing, backend IO, and DB lifecycle.""" @@ -195,8 +181,8 @@ async def download( """ row = await self.get(file_id, platform=platform) if self.backend.supports_presigned_url: - url = await self.backend.presigned_get_url( - row.key, self.settings.s3_presign_ttl_seconds + url = await presigned_download_url( + self.backend, row, self.settings.s3_presign_ttl_seconds, attachment=True ) return RedirectDownload(file=row, url=url) body = await self.backend.get(row.key) if open_body else None diff --git a/modules/file_storage/file_storage/serving.py b/modules/file_storage/file_storage/serving.py index 99e852ea5..f348282e5 100644 --- a/modules/file_storage/file_storage/serving.py +++ b/modules/file_storage/file_storage/serving.py @@ -3,13 +3,12 @@ from __future__ import annotations -from urllib.parse import quote - from fastapi import HTTPException, Request, Response, status from fastapi.responses import RedirectResponse from file_storage import constants, thumbnails from file_storage.contracts.service import StorageNotFoundError +from file_storage.downloads import content_disposition, presigned_download_url from file_storage.models import StoredFile from file_storage.ranges import file_body_response from file_storage.service import FileStorageService @@ -51,14 +50,6 @@ def is_active_content(content_type: str) -> bool: return thumbnails.base_type(content_type) in constants.ACTIVE_CONTENT_TYPES -def content_disposition(filename: str, *, attachment: bool) -> str: - ascii_name = "".join( - c for c in filename.encode("ascii", "ignore").decode() if c.isprintable() and c not in '"\\' - ) - kind = "attachment" if attachment else "inline" - return f"{kind}; filename=\"{ascii_name or 'file'}\"; filename*=UTF-8''{quote(filename)}" - - async def thumbnail_response( service: FileStorageService, row: StoredFile, @@ -111,8 +102,8 @@ async def public_file_response( return Response(status_code=status.HTTP_304_NOT_MODIFIED, headers=headers) if service.backend.supports_presigned_url and not active: - url = await service.backend.presigned_get_url( - row.key, service.settings.s3_presign_ttl_seconds + url = await presigned_download_url( + service.backend, row, service.settings.s3_presign_ttl_seconds, attachment=False ) # A cached redirect must not outlive the signature it points at. redirect_age = min(max_age, service.settings.s3_presign_ttl_seconds // 2) diff --git a/modules/file_storage/tests/test_api.py b/modules/file_storage/tests/test_api.py index 15608aedd..dcfff4f99 100644 --- a/modules/file_storage/tests/test_api.py +++ b/modules/file_storage/tests/test_api.py @@ -90,3 +90,22 @@ async def test_413_names_the_limit_it_enforced(app, authenticated_client: httpx. assert detail["code"] == constants.ErrorCode.TOO_LARGE assert detail["max_bytes"] == 4 assert "4 B" in detail["message"] + + +async def test_download_names_a_non_ascii_file_safely(authenticated_client: httpx.AsyncClient): + """The header was built by hand: a non-Latin-1 name could not be encoded as + a header at all, and a ``"`` in the name ended the quoted value early.""" + name = 'Ünïcödé "quoted" 報告.txt' + upload = await authenticated_client.post( + f"{constants.ROUTE_PREFIX_API}{constants.PATH_UPLOAD}", + files={"file": (name, b"hi", "text/plain")}, + ) + assert upload.status_code == 201, upload.text + file_id = upload.json()["id"] + + resp = await authenticated_client.get(f"{constants.ROUTE_PREFIX_API}/files/{file_id}/download") + assert resp.status_code == 200 + disposition = resp.headers["content-disposition"] + assert disposition.startswith("attachment; ") + assert "filename*=UTF-8''" in disposition + assert disposition.count('"') == 2 diff --git a/modules/file_storage/tests/test_backend_registry.py b/modules/file_storage/tests/test_backend_registry.py index 2a44fdcbb..ece25bca4 100644 --- a/modules/file_storage/tests/test_backend_registry.py +++ b/modules/file_storage/tests/test_backend_registry.py @@ -44,7 +44,9 @@ async def delete(self, key: str) -> None: ... async def exists(self, key: str) -> bool: return False - async def presigned_get_url(self, key: str, ttl_seconds: int) -> str: # pragma: no cover + async def presigned_get_url( + self, key: str, ttl_seconds: int, **_: str | None + ) -> str: # pragma: no cover raise NotImplementedError diff --git a/modules/file_storage/tests/test_file_storage_range_download.py b/modules/file_storage/tests/test_file_storage_range_download.py index 9f2da528f..77696c918 100644 --- a/modules/file_storage/tests/test_file_storage_range_download.py +++ b/modules/file_storage/tests/test_file_storage_range_download.py @@ -8,7 +8,7 @@ API = constants.ROUTE_PREFIX_API PAYLOAD = bytes(range(256)) * 800 # 204,800 bytes: several 64 KiB chunks SIZE = len(PAYLOAD) -DISPOSITION = 'attachment; filename="tiles.pmtiles"' +DISPOSITION = "attachment; filename=\"tiles.pmtiles\"; filename*=UTF-8''tiles.pmtiles" async def _upload(client: httpx.AsyncClient, **form) -> dict: diff --git a/modules/file_storage/tests/test_s3_backend.py b/modules/file_storage/tests/test_s3_backend.py index c7c7c303d..ec71a57b1 100644 --- a/modules/file_storage/tests/test_s3_backend.py +++ b/modules/file_storage/tests/test_s3_backend.py @@ -174,6 +174,40 @@ async def test_s3_presigned_url_format(s3_settings): assert "AWSAccessKeyId=" not in url +async def test_s3_presigned_url_signs_the_download_headers(s3_settings): + """GH #489: S3 answers with these instead of the object's own metadata.""" + from urllib.parse import parse_qs, urlsplit + + from file_storage.downloads import content_disposition + + backend = build_backend(s3_settings) + disposition = content_disposition("Résumé 2026.pdf", attachment=True) + url = await backend.presigned_get_url( + "t/abc123", 300, content_disposition=disposition, content_type="application/pdf" + ) + query = parse_qs(urlsplit(url).query) + assert query["response-content-disposition"] == [disposition] + assert query["response-content-type"] == ["application/pdf"] + + +async def test_s3_presigned_download_is_served_with_the_original_name(s3_settings): + """End to end against moto: the signed overrides become response headers.""" + import httpx + from file_storage.downloads import content_disposition + + backend = build_backend(s3_settings) + await _put(backend, "t/abc123", b"%PDF-1.4") + disposition = content_disposition("report.pdf", attachment=True) + url = await backend.presigned_get_url( + "t/abc123", 300, content_disposition=disposition, content_type="application/pdf" + ) + async with httpx.AsyncClient() as client: + resp = await client.get(url) + assert resp.status_code == 200, resp.text + assert resp.headers["content-disposition"] == disposition + assert resp.headers["content-type"] == "application/pdf" + + async def test_s3_signature_version_is_configurable(s3_settings): s3_settings.s3_signature_version = "s3" backend = build_backend(s3_settings) diff --git a/modules/file_storage/tests/test_service.py b/modules/file_storage/tests/test_service.py index 16b6491e8..da506e093 100644 --- a/modules/file_storage/tests/test_service.py +++ b/modules/file_storage/tests/test_service.py @@ -184,7 +184,8 @@ async def delete(self, key): ... async def exists(self, key): return True - async def presigned_get_url(self, key, ttl_seconds): + async def presigned_get_url(self, key, ttl_seconds, **headers): + self.headers = headers return f"https://signed/{key}" pb: StorageBackend = _PresigningBackend() # type: ignore[assignment] @@ -192,6 +193,10 @@ async def presigned_get_url(self, key, ttl_seconds): download2 = await svc.download(out.id) assert isinstance(download2, RedirectDownload) assert download2.url.startswith("https://signed/") + # GH #489: the redirect must carry the original name, not the storage key. + assert pb.headers["content_disposition"].startswith("attachment; ") + assert out.filename in pb.headers["content_disposition"] + assert pb.headers["content_type"] == out.content_type async def _drain(stream: AsyncIterator[bytes]) -> bytes: diff --git a/modules/settings/settings/_module_settings.py b/modules/settings/settings/_module_settings.py index 672920e01..dc7e0e3c0 100644 --- a/modules/settings/settings/_module_settings.py +++ b/modules/settings/settings/_module_settings.py @@ -19,6 +19,7 @@ from settings._secrets import ( # noqa: F401 _NEVER_SECRET_TYPES, SECRET_MASK, + annotation_holds_secret, conceals_secret, embeds_credential, is_named_secret, @@ -26,7 +27,7 @@ mask, ) from settings.env_vars import env_prefix_for -from settings.hydrate import value_type_for_field +from settings.hydrate import env_supplied_fields, input_type_for_field, value_type_for_field from settings.service import SettingService @@ -71,22 +72,28 @@ class ModuleSettingField: """ choices: list[str] | None = None """Closed set of accepted values, or ``None`` when the field is free text.""" + input_type: str = "" + """Editor control: ``type`` with an ``X | None`` unwrapped to ``X``.""" + nullable: bool = False + """``None`` is a valid value; the editor sends it for an empty input.""" @property def source(self) -> str: """Where the live value came from: ``db``, ``env`` or ``default``. - Mirrors the precedence in ``hydrate_settings``: DB overrides are passed - to the constructor explicitly, so they beat anything pydantic reads for - fields left unset, which in turn beats the field default. ``env`` only - appears for settings classes that actually declare an ``env_prefix``; - see :attr:`env_set`. Showing this is the difference between "why is - this not taking effect" being a five-minute question and an afternoon. + Mirrors the precedence in ``hydrate_settings``: env → DB → default + (GH #486). A stored override for a field the environment sets is + ignored, so ``env`` wins even when ``db_override`` is true. ``env`` only + appears for settings classes that declare an ``env_prefix`` (or name + the field in ``env_seeded_fields``) — the same rule + ``hydrate.env_supplied_fields`` applies; see :attr:`env_set`. Showing + this is the difference between "why is this not taking effect" being a + five-minute question and an afternoon. """ - if self.db_override: - return "db" if self.env_set: return "env" + if self.db_override: + return "db" return "default" @@ -126,28 +133,41 @@ def _field_view( settings: BaseSettings, prefix: str, overridden: frozenset[str] = frozenset(), + env_supplied: frozenset[str] | None = None, ) -> ModuleSettingField: cls = type(settings) + if env_supplied is None: + env_supplied = env_supplied_fields(cls) info = cls.model_fields[name] raw_value = getattr(settings, name) value_type = value_type_for_field(cls, name) + input_type, nullable = input_type_for_field(cls, name) named_secret = is_named_secret(name, value_type) extra = info.json_schema_extra if isinstance(info.json_schema_extra, dict) else {} default = resolve_default(info) env_var = f"{prefix}{name.upper()}" live_env_var = env_readable_var(settings, name) - env_set = live_env_var is not None and live_env_var in os.environ - # ``live_env_var`` is not None whenever env_set is True, but the narrowing - # is spelled out so a future edit cannot turn this into ``os.environ[None]``. - raw_env = os.environ[live_env_var] if env_set and live_env_var is not None else None + # The same answer ``build_settings`` acts on (GH #486), so a value supplied + # by the class's ``.env``, a secrets file or an alias is labelled "env" + # here exactly when hydration lets it beat a stored override. + env_set = name in env_supplied + raw_env = os.environ.get(live_env_var) if env_set and live_env_var is not None else None + + # Declared as ``SecretStr`` (also inside ``X | None`` or a container): + # masked whatever the field is called, since the store holds it in clear. + typed_secret = annotation_holds_secret(info.annotation) def hidden(value: Any) -> bool: - return conceals_secret(name, value, value_type) + return typed_secret or conceals_secret(name, value, value_type) # ``is_secret`` drives the editor's input type and the "reveal" affordance, # so it has to be true when *any* of the three readings is being hidden — # otherwise the form offers to edit a value it is showing as dots. - secret = named_secret or any(embeds_credential(v) for v in (raw_value, default, raw_env)) + secret = ( + typed_secret + or named_secret + or any(embeds_credential(v) for v in (raw_value, default, raw_env)) + ) return ModuleSettingField( name=name, env_var=env_var, @@ -166,6 +186,8 @@ def hidden(value: Any) -> bool: # Resolved value panel's env row into a row of dots for every key. env_value=(mask(raw_env) if hidden(raw_env) else raw_env) if raw_env is not None else None, choices=choices_for(info), + input_type=input_type, + nullable=nullable, ) @@ -228,8 +250,10 @@ def _build_view( ) -> ModuleSettingsView: prefix = env_prefix_for(package) overridden = (overrides or {}).get(package, frozenset()) + from_env = env_supplied_fields(type(settings)) fields = [ - _field_view(name, settings, prefix, overridden) for name in type(settings).model_fields + _field_view(name, settings, prefix, overridden, from_env) + for name in type(settings).model_fields ] return ModuleSettingsView( module_name=module_name, diff --git a/modules/settings/settings/_module_settings_props.py b/modules/settings/settings/_module_settings_props.py index 311f55a09..0d971706f 100644 --- a/modules/settings/settings/_module_settings_props.py +++ b/modules/settings/settings/_module_settings_props.py @@ -49,6 +49,8 @@ def serialize(views: list[ModuleSettingsView]) -> list[dict[str, Any]]: # Turns a pattern-constrained string into a select rather # than a text box whose only feedback on a typo is a 422. "choices": f.choices, + "input_type": f.input_type or f.type, + "nullable": f.nullable, } for f in v.fields ], diff --git a/modules/settings/settings/_secrets.py b/modules/settings/settings/_secrets.py index 82e4e659b..a29fb967a 100644 --- a/modules/settings/settings/_secrets.py +++ b/modules/settings/settings/_secrets.py @@ -9,9 +9,11 @@ from __future__ import annotations import re -from typing import Any +from typing import Any, get_args, get_origin from urllib.parse import urlsplit +from pydantic import BaseModel, SecretBytes, SecretStr + # We intentionally DON'T match the bare word "token" (would mask # `verification_token_lifetime_seconds` — just an int) or "key" alone (would # mask `s3_bucket_key_prefix`). Only fragments that actually indicate material. @@ -25,6 +27,41 @@ _NEVER_SECRET_TYPES = frozenset({"int", "float", "bool"}) +# ``.`` keys whose declared type holds a ``SecretStr`` / +# ``SecretBytes``. Filled by ``ModuleSettingsRegistry.register`` so the row +# browser, tenant view and known-keys list — which see only a key and a stored +# string — mask them too. Only ever grows; keys are package-qualified. +_TYPED_SECRET_KEYS: set[str] = set() + + +def annotation_holds_secret(annotation: Any, _seen: frozenset[type] = frozenset()) -> bool: + """True if a ``SecretStr``/``SecretBytes`` sits anywhere in ``annotation``. + + Walks unions (``SecretStr | None``), containers (``dict[str, SecretStr]``) + and nested models. Since json fields store the real secret (GH #487 + review), a field typed this way must be masked whatever its name. + """ + if get_origin(annotation) is None and isinstance(annotation, type): + if issubclass(annotation, SecretStr | SecretBytes): + return True + if issubclass(annotation, BaseModel) and annotation not in _seen: + seen = _seen | {annotation} + return any( + annotation_holds_secret(f.annotation, seen) + for f in annotation.model_fields.values() + ) + return any(annotation_holds_secret(arg, _seen) for arg in get_args(annotation)) + + +def register_typed_secrets(package: str, cls: type[BaseModel]) -> None: + """Record which of ``cls``'s fields are secrets by type.""" + _TYPED_SECRET_KEYS.update( + f"{package}.{name}" + for name, info in cls.model_fields.items() + if annotation_holds_secret(info.annotation) + ) + + def is_secret_field(name: str) -> bool: """True if a field name suggests it holds credential material.""" return bool(_SECRET_PATTERNS.search(name)) @@ -49,11 +86,14 @@ def is_named_secret(name: str, value_type: str | None = None) -> bool: def conceals_secret(name: str, value: Any, value_type: str | None = None) -> bool: """True if this name/value pair must be masked on the way out. - The one rule every read path shares: mask per *value* as well as per name, + The one rule every read path shares: mask per *type* (a registered + ``.`` declared as a secret), per *value* and per name, so a DSN carrying a password is hidden even on a field called ``broker_url``, while that field's password-free default is still shown. """ - return is_named_secret(name, value_type) or embeds_credential(value) + return ( + name in _TYPED_SECRET_KEYS or is_named_secret(name, value_type) or embeds_credential(value) + ) def embeds_credential(value: Any) -> bool: diff --git a/modules/settings/settings/endpoints/module_api.py b/modules/settings/settings/endpoints/module_api.py index 08568e0d4..8e5f700b7 100644 --- a/modules/settings/settings/endpoints/module_api.py +++ b/modules/settings/settings/endpoints/module_api.py @@ -11,6 +11,7 @@ from fastapi import APIRouter, Depends, FastAPI, HTTPException, Request, Response, status from pydantic import ValidationError +from simple_module_hosting.i18n_deps import TranslatorDep from simple_module_hosting.permissions import RequiresPermission from settings._module_settings import ( @@ -23,7 +24,7 @@ from settings.contracts.events import SettingsReloaded from settings.deps import get_setting_service from settings.hydrate import hydrate_settings -from settings.reload import apply_changes_and_reload +from settings.reload import EnvPinnedError, apply_changes_and_reload from settings.service import SettingService from settings.store import SettingsStore @@ -90,6 +91,7 @@ async def update_module( package: str, changes: dict[str, Any], request: Request, + t: TranslatorDep, service: SettingService = Depends(get_setting_service), ) -> dict[str, Any]: registry = getattr(request.app.state, MODULE_PACKAGE).module_registry @@ -116,9 +118,21 @@ async def update_module( except ValidationError as exc: # ``exc.errors()`` still leaves ``ctx.error`` holding the raw Python # exception (not JSON-serializable). Round-trip through pydantic's - # own JSON encoder to get clean, serializable errors. - clean = json.loads(exc.json(include_url=False)) + # own JSON encoder to get clean, serializable errors. ``input`` is + # dropped: it echoes the submitted value, which may be a secret. + clean = json.loads(exc.json(include_url=False, include_input=False)) raise HTTPException(status_code=422, detail=clean) from exc + except EnvPinnedError as exc: + # Shaped like a pydantic 422 so the editor shows it on the field. + detail = [ + { + "loc": ["body", name], + "msg": t.t("settings.modules_form.env_pinned"), + "type": "env_pinned", + } + for name in exc.fields + ] + raise HTTPException(status_code=422, detail=detail) from exc except KeyError as exc: raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(exc)) from exc diff --git a/modules/settings/settings/hydrate.py b/modules/settings/settings/hydrate.py index 9662acb8b..dfd733778 100644 --- a/modules/settings/settings/hydrate.py +++ b/modules/settings/settings/hydrate.py @@ -11,16 +11,25 @@ import json import logging -from typing import get_origin - -from pydantic import ValidationError -from pydantic_settings import BaseSettings +import types +from typing import Union, get_args, get_origin + +from pydantic import BaseModel, ValidationError +from pydantic_settings import ( + BaseSettings, + DotEnvSettingsSource, + EnvSettingsSource, + InitSettingsSource, + SecretsSettingsSource, +) from sqlalchemy.orm import Session from settings.store import SettingsStore, get_overrides_sync logger = logging.getLogger(__name__) +_SCALAR_TYPES = {bool: "bool", int: "int", float: "float"} + def value_type_for_field(cls: type[BaseSettings], field_name: str) -> str: """Return the ``value_type`` label for a field based on its annotation. @@ -36,13 +45,36 @@ def value_type_for_field(cls: type[BaseSettings], field_name: str) -> str: origin = get_origin(ann) if origin is not None: return "json" - if ann is bool: - return "bool" - if ann is int: - return "int" - if ann is float: - return "float" - return "string" + return _SCALAR_TYPES.get(ann, "string") + + +def _holds_structure(ann: object) -> bool: + """A bare ``dict``/``list``/model: not editable as a single-line string.""" + return isinstance(ann, type) and issubclass( + ann, dict | list | set | frozenset | tuple | BaseModel + ) + + +def input_type_for_field(cls: type[BaseSettings], field_name: str) -> tuple[str, bool]: + """``(input_type, nullable)`` — which editor control a field needs. + + Storage stays on :func:`value_type_for_field`, which files every union + under ``json``. That is right for the store and wrong for the form: + ``int | None`` wants a number box where empty means ``None``, not a JSON + textarea that shows the literal text ``null`` (GH #487). + """ + ann = cls.model_fields[field_name].annotation + if get_origin(ann) not in (Union, types.UnionType): + return value_type_for_field(cls, field_name), False + args = get_args(ann) + rest = [a for a in args if a is not type(None)] + nullable = len(rest) < len(args) + if len(rest) == 1 and get_origin(rest[0]) is None: + only = rest[0] + if _holds_structure(only): + return "json", nullable + return _SCALAR_TYPES.get(only, "string"), nullable + return "json", nullable def _parse(raw: str, value_type: str): @@ -57,6 +89,49 @@ def _parse(raw: str, value_type: str): return raw +def env_supplied_fields(cls: type[BaseSettings]) -> frozenset[str]: + """Fields the class's own non-init sources (env, ``.env``, secrets) answer. + + Asks the sources ``cls`` actually installs, so a ``DbBackedSettings`` + (init-only) reports nothing and an ``env_prefix`` class reports exactly the + ```` names that are set. Deliberately not ``cls()``: a + model validator can reject the env-only view (a production broker check) + that the DB-merged one would pass. + + A class with no ``env_prefix`` reports nothing from its sources: a plain + ``BaseSettings`` reads bare, case-insensitive names, so a field called + ``user``, ``home`` or ``path`` would be pinned by the process environment + and every admin edit refused. Only its ``env_seeded_fields`` count. + """ + found: set[str] = set() + if cls.model_config.get("env_prefix"): + found.update(_source_supplied_fields(cls)) + # A class may also read env through a ``default_factory`` (one shared + # variable seeding several fields), which no source can see. It names + # those fields itself, so env still beats the DB for them. + seeded = getattr(cls, "env_seeded_fields", None) + if callable(seeded): + found.update(seeded()) + return frozenset(found) & cls.model_fields.keys() + + +def _source_supplied_fields(cls: type[BaseSettings]) -> set[str]: + sources = cls.settings_customise_sources( + cls, + init_settings=InitSettingsSource(cls, init_kwargs={}), + env_settings=EnvSettingsSource(cls), + dotenv_settings=DotEnvSettingsSource(cls), + file_secret_settings=SecretsSettingsSource(cls), + ) + by_key = {(info.alias or name): name for name, info in cls.model_fields.items()} + found: set[str] = set() + for source in sources: + if isinstance(source, InitSettingsSource): + continue + found.update(by_key.get(key, key) for key in source()) + return found + + def build_settings[T: BaseSettings]( cls: type[T], raw_overrides: dict[str, tuple[str, str]], *, lenient: bool = False ) -> T: @@ -69,10 +144,15 @@ def build_settings[T: BaseSettings]( lifespan, workers): a row that was stored before a validator tightened must not take the app down, nor discard the module's other, valid overrides. Each offending field is logged and ignored (it falls back to env/default). + + Precedence is env → DB → default (GH #486). pydantic-settings ranks init + kwargs above its env source, so a stored override for a field the + environment sets is dropped here rather than passed in. """ + from_env = env_supplied_fields(cls) parsed: dict[str, object] = {} for field_name, (raw, vtype) in raw_overrides.items(): - if field_name not in cls.model_fields: + if field_name not in cls.model_fields or field_name in from_env: continue try: parsed[field_name] = _parse(raw, vtype) diff --git a/modules/settings/settings/locales/en.json b/modules/settings/settings/locales/en.json index 274a33ea2..0c84b116c 100644 --- a/modules/settings/settings/locales/en.json +++ b/modules/settings/settings/locales/en.json @@ -119,7 +119,7 @@ "env_var_hint": "Environment variable name for this field, used by \"smpy settings import-from-env\"", "env_var_not_read_hint": "{env_var} is a label only — this module reads its settings from the database, never from the environment", "source_db": "overridden in DB", - "source_db_over_env": "overridden in DB · shadows env", + "source_db_ignored_for_env": "stored in DB · env var wins", "secret_write_only": "write-only · never returned", "head_title": "Module settings", "managed_title": "Managed on its own page", @@ -134,7 +134,12 @@ "save_failed": "Saving failed. Try again.", "requires_restart": "Requires restart", "reset_to_default": "Revert", - "set_new_value": "Set new value" + "set_new_value": "Set new value", + "invalid_json": "Not valid JSON — it will be rejected on save.", + "not_set": "Not set (default)", + "option_on": "On", + "option_off": "Off", + "env_pinned": "Set by an environment variable, which takes precedence. Change it there." }, "nav": { "settings": "Settings" diff --git a/modules/settings/settings/module_registry.py b/modules/settings/settings/module_registry.py index 4a80147bd..1084fefd5 100644 --- a/modules/settings/settings/module_registry.py +++ b/modules/settings/settings/module_registry.py @@ -11,6 +11,8 @@ from pydantic_settings import BaseSettings +from settings._secrets import register_typed_secrets + @dataclass(slots=True) class ModuleSettingsRegistry: @@ -28,6 +30,7 @@ def register( if package in self._classes: raise ValueError(f"{package!r} already registered") self._classes[package] = cls + register_typed_secrets(package, cls) if manage_url: self._manage_urls[package] = manage_url diff --git a/modules/settings/settings/pages/components/FieldInput.tsx b/modules/settings/settings/pages/components/FieldInput.tsx index 8dddb05e8..c88382bdb 100644 --- a/modules/settings/settings/pages/components/FieldInput.tsx +++ b/modules/settings/settings/pages/components/FieldInput.tsx @@ -8,8 +8,8 @@ import { SelectValue, } from '@simple-module-py/ui/components/ui/select'; import { Switch } from '@simple-module-py/ui/components/ui/switch'; -import { Textarea } from '@simple-module-py/ui/components/ui/textarea'; import { useState } from 'react'; +import { JsonInput } from './JsonInput'; export type FieldType = 'bool' | 'int' | 'float' | 'string' | 'json'; @@ -33,8 +33,14 @@ export type FieldMeta = { source?: 'db' | 'env' | 'default'; /** Closed set of accepted values, or null when the field is free text. */ choices?: string[] | null; + /** Editor control: `type` with an `X | None` unwrapped to `X`. */ + input_type?: FieldType; + /** `null` is a valid value; an empty input sends it. */ + nullable?: boolean; }; +const UNSET = '__unset__'; + type Props = { field: FieldMeta; onChange: (name: string, value: unknown) => void; @@ -49,6 +55,8 @@ export function FieldInput({ field, onChange, value, id }: Props) { const { t } = useT(); const [revealed, setRevealed] = useState(false); const highlight = field.db_override ? OVERRIDDEN : ''; + const kind = field.input_type ?? field.type; + const nullable = !!field.nullable; if (field.is_secret && !revealed) { // Not "Reveal": the server never returns the stored value, so there is @@ -83,7 +91,27 @@ export function FieldInput({ field, onChange, value, id }: Props) { ); } - if (field.type === 'bool') { + if (kind === 'bool' && nullable) { + // `bool | None`: a switch cannot say "not set", which is the default. + const current = value === null || value === undefined ? UNSET : String(value); + return ( + + ); + } + + if (kind === 'bool') { return ( onChange(field.name, e.target.value === '' ? null : Number(e.target.value)) @@ -128,14 +156,15 @@ export function FieldInput({ field, onChange, value, id }: Props) { ); } - if (field.type === 'json') { + if (kind === 'json') { return ( -