fix: settings precedence, settings editor types, signup role, S3 download names, scaffolded worker (#486–#490)

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).

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 |
 |---|---|---|

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
 

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 |
 |---|---|---|

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")

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:

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

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 == []

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())

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,