diff --git a/CHANGELOG.md b/CHANGELOG.md index c436a0ea2..88ff6a74d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,30 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] ### Changed +- **Behaviour change (#543):** with `allow_signup` off, an OAuth sign-in no + longer creates an account. It answers 403 `OAUTH_SIGNUP_DISABLED` unless the + identity is already linked or its email matches an existing account, which + still link and sign in. +- **Behaviour change (#547):** `users.require_verification` is honoured. With + it off, no verification email is sent, the register page says so, and + password login no longer requires a verified email — including for accounts + registered while it was on. Such accounts stay unverified, and an OAuth + sign-in whose email matches an unverified password account is refused with + 403 `OAUTH_ACCOUNT_NOT_VERIFIED` instead of linking to it (an account already + linked to that OAuth identity still signs in). With it on, + `POST /api/users/auth/token` now refuses an unverified account with 400 + `LOGIN_USER_NOT_VERIFIED`, as the session login already did; it used to mint + bearer tokens for one. +- **Behaviour change (#544):** `file_storage` callers without + `file_storage.manage` (granted to admins and to `tenant:admin`/`tenant:owner`) + see, download, change and delete only files they uploaded; anyone else's + file is a 404. Files with no recorded uploader are visible to managers only. +- **Behaviour change (#545):** `file_storage` serves a stored type inline only + from an allow-list (raster images, PDF, JSON/GeoJSON, plain text, CSV, opaque + binaries, audio/video/font). Everything else, any `*+xml` and any malformed + value included, is a sandboxed attachment. +- `POST /api/users/auth/token` returns `token_type="Bearer"`, and every bearer + resolver (users, keycloak, api_tokens) accepts the scheme in any case (#533). - **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 @@ -312,6 +336,16 @@ 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 +- A runtime permission source that raises is no longer cached as empty until + invalidated: it contributes nothing for 30 seconds (one logged traceback), + then is retried; `invalidate_source` retries it at once (#517). +- `background_tasks`: a prefork Celery child no longer reuses the sync engine + (and Postgres connections) built in the parent (#532). +- Settings validation: `file_storage.quota_bytes` must be positive (and at most + 2^53-1), and `background_tasks.task_time_limit` must exceed + `task_soft_time_limit` when both are set (#548). With the hard limit set by + env, an offending stored soft limit is dropped (and logged) at boot instead + of discarding every background_tasks override and stopping the worker. - 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 diff --git a/docs/modules/file_storage.md b/docs/modules/file_storage.md index 7f4544e42..2f56bd429 100644 --- a/docs/modules/file_storage.md +++ b/docs/modules/file_storage.md @@ -78,10 +78,17 @@ Serving rules: `If-None-Match`) and `X-Content-Type-Options: nosniff`. - Every public response carries `Content-Security-Policy: default-src 'none'; style-src 'unsafe-inline'; sandbox` (the security-headers middleware now - keeps a CSP the response already set). Active content — HTML, XHTML, SVG, - XML, JavaScript — is forced to `Content-Disposition: attachment` and is - always streamed, so stored XSS on the app origin is not possible. Other - types are `inline`. + keeps a CSP the response already set). Only an allow-list of types is + served `inline` (and redirected on S3): common raster images, PDF, JSON, + GeoJSON, plain text, CSV, opaque binaries (`application/octet-stream`, zip, + gzip, protobuf / vector tiles) and `audio/*` / `video/*` / `font/*` minus + any XML or HTML subtype + (`INLINE_SAFE_CONTENT_TYPES` / `INLINE_SAFE_FAMILIES`). Everything else — + HTML, SVG, any XML, scripts, unknown types, and malformed values such as + `text/html,` — is forced to `Content-Disposition: attachment` and always + streamed, so stored XSS on the app origin is not possible. The stored type + is lower-cased and stripped of parameters before the check + (`media_types.is_active_content`). - Presigning backends (S3) answer with a `302` to the presigned URL, cached for at most half the signature's TTL; filesystem backends stream. @@ -311,7 +318,7 @@ Register it in your module's `register_settings` so the file_storage service can | `file_storage.upload` | `user`, `admin`, `tenant:member`/`admin`/`owner` | upload files | | `file_storage.download` | `user`, `admin`, `tenant:member`/`admin`/`owner` | list / get / download | | `file_storage.delete` | `admin`, `tenant:admin`, `tenant:owner`; also `user` when `multi_tenant` is off | delete | -| `file_storage.manage` | `admin` | reserved for future admin operations | +| `file_storage.manage` | `admin`, `tenant:admin`, `tenant:owner` | act on every file in the active tenant, not only one's own | With `multi_tenant` on, delete is an organisation-admin act: the platform `user` role, which every account holds, does not carry it (it would hand it to @@ -319,6 +326,16 @@ every tenant member). With `multi_tenant` off the install is the only tenant, so `on_startup` maps `file_storage.delete` onto `user` and ordinary users keep deleting their files. +**Ownership.** Every authenticated route — list, get, download, thumbnail, +publish/unpublish, delete, bulk delete and the browse screen — acts only on the +files the caller uploaded (`created_by` is their user id), unless they hold +`file_storage.manage`. Someone else's file answers `404`, exactly like an +unknown id, and the browse screen's type and uploader filters list only the +caller's own rows (the usage figure stays bucket-wide, as the quota is). The +scope is passed explicitly by the endpoints (`deps.get_owner_scope`) to the +service's `owner=` argument; internal callers such as branding and reapers +pass none and stay unscoped, and anonymous `public` serving is unaffected. + ## Menu | Label | URL | Icon | Section | Group | Order | Roles | diff --git a/framework/core/simple_module_core/permissions.py b/framework/core/simple_module_core/permissions.py index baaea6f21..62c0363d1 100644 --- a/framework/core/simple_module_core/permissions.py +++ b/framework/core/simple_module_core/permissions.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging +import time from collections.abc import Awaitable, Callable, Collection, Iterable from dataclasses import dataclass, field from typing import Any @@ -95,7 +96,11 @@ class PermissionRegistry: every mutation. """ - def __init__(self) -> None: + #: Seconds a failed source is answered as empty before it is called again. + SOURCE_RETRY_SECONDS = 30.0 + + def __init__(self, *, clock: Callable[[], float] = time.monotonic) -> None: + self._clock = clock self._groups: dict[str, PermissionGroup] = {} self._role_map: dict[str, set[str]] = {} # Admin-editor grants (DB-backed). Kept apart from ``_role_map`` (code @@ -106,6 +111,8 @@ def __init__(self) -> None: self._grant_sources: list[GrantSource] = [] self._sources: dict[str, PermissionSourceProvider] = {} self._source_cache: dict[str, tuple[list[str], dict[str, str]]] = {} + self._source_failed_at: dict[str, float] = {} # name → clock() of last failure + self._all_permissions_expires: float | None = None def _invalidate(self) -> None: self._all_permissions_cache = None @@ -120,21 +127,28 @@ def add_source(self, name: str, provider: PermissionSourceProvider) -> None: is **sync** and should read a module-maintained in-memory cache — the registry is consulted on every request. Its output is cached here until :meth:`invalidate_source` is called. A provider that raises is logged - and contributes nothing. + once and contributes nothing for ``SOURCE_RETRY_SECONDS``, then is + retried — a transient failure recovers on its own (GH #517). """ self._sources[name] = provider self._source_cache.pop(name, None) + self._source_failed_at.pop(name, None) self._invalidate() def invalidate_source(self, name: str) -> None: - """Drop the cached output of source *name*; re-read on next access.""" + """Drop the cached output (or failure) of source *name*; re-read on next access.""" self._source_cache.pop(name, None) + self._source_failed_at.pop(name, None) self._invalidate() def _source_output(self, name: str) -> tuple[list[str], dict[str, str]]: cached = self._source_cache.get(name) if cached is not None: return cached + failed_at = self._source_failed_at.get(name) + if failed_at is not None and self._clock() - failed_at < self.SOURCE_RETRY_SECONDS: + # In the retry window: empty, uncalled, no traceback per request. + return [], {} keys: list[str] = [] labels: dict[str, str] = {} try: @@ -146,8 +160,12 @@ def _source_output(self, name: str) -> tuple[list[str], dict[str, str]]: keys.append(key) labels[key] = label except Exception: - logger.exception("Permission source %r failed; contributing nothing", name) - keys, labels = [], {} + logger.exception("Permission source %r failed; contributing nothing until retry", name) + # Remembered for the retry window only: a transient failure must + # not hide the source until someone invalidates it. + self._source_failed_at[name] = self._clock() + return [], {} + self._source_failed_at.pop(name, None) out = (sorted(set(keys)), labels) self._source_cache[name] = out return out @@ -176,11 +194,18 @@ def add(self, permission: str) -> None: @property def all_permissions(self) -> list[str]: """All registered permission strings, sorted.""" - if self._all_permissions_cache is None: + expires = self._all_permissions_expires + if self._all_permissions_cache is None or (expires and self._clock() >= expires): perms: set[str] = set() for group in self.groups: perms.update(group.permissions) self._all_permissions_cache = sorted(perms) + # A snapshot taken while a source is failing lives only until that + # source's retry, so a recovery shows up without an invalidation. + failed = self._source_failed_at.values() + self._all_permissions_expires = ( + min(failed) + self.SOURCE_RETRY_SECONDS if failed else None + ) return self._all_permissions_cache @property diff --git a/framework/core/tests/test_permission_sources.py b/framework/core/tests/test_permission_sources.py index ec2d9ff1e..68f13c50c 100644 --- a/framework/core/tests/test_permission_sources.py +++ b/framework/core/tests/test_permission_sources.py @@ -50,3 +50,84 @@ def test_admin_implicit_all_includes_sources() -> None: reg = PermissionRegistry() reg.add_source("records", lambda: ["records.product.edit"]) assert "records.product.edit" in reg.get_permissions_for_roles(["admin"]) + + +class _Clock: + def __init__(self) -> None: + self.now = 1000.0 + + def __call__(self) -> float: + return self.now + + +def test_failed_source_recovers_after_the_retry_window() -> None: + calls = {"n": 0} + + def flaky() -> list[str]: + calls["n"] += 1 + if calls["n"] == 1: + raise RuntimeError("db not ready") + return ["flaky.read"] + + clock = _Clock() + reg = PermissionRegistry(clock=clock) + reg.add_source("flaky", flaky) + assert reg.all_permissions == [] + assert not reg.has("flaky.read") # inside the window: not called again + assert calls["n"] == 1 + clock.now += PermissionRegistry.SOURCE_RETRY_SECONDS + assert reg.has("flaky.read") # no invalidate_source needed + assert "flaky.read" in reg.all_permissions + assert calls["n"] == 2 + + +def test_permanent_failure_logs_once_per_window(caplog) -> None: + calls = {"n": 0} + + def broken() -> list[str]: + calls["n"] += 1 + raise RuntimeError("misconfigured") + + clock = _Clock() + reg = PermissionRegistry(clock=clock) + reg.add_source("broken", broken) + with caplog.at_level(logging.ERROR): + for _ in range(50): + assert reg.all_permissions == [] + assert (calls["n"], len(caplog.records)) == (1, 1) + assert caplog.records[0].exc_info is not None + clock.now += PermissionRegistry.SOURCE_RETRY_SECONDS + 1 + for _ in range(50): + assert reg.all_permissions == [] + assert (calls["n"], len(caplog.records)) == (2, 2) + + +def test_invalidate_source_retries_a_failed_source_at_once() -> None: + state = {"ok": False} + + def source() -> list[str]: + if not state["ok"]: + raise RuntimeError("not yet") + return ["late.read"] + + reg = PermissionRegistry(clock=_Clock()) + reg.add_source("late", source) + assert reg.all_permissions == [] + state["ok"] = True + reg.invalidate_source("late") + assert reg.all_permissions == ["late.read"] + + +def test_snapshot_is_reused_while_a_source_is_failing() -> None: + def broken() -> list[str]: + raise RuntimeError("down") + + clock = _Clock() + reg = PermissionRegistry(clock=clock) + reg.add("static.read") + reg.add_source("broken", broken) + first = reg.all_permissions + assert first == ["static.read"] + assert reg.all_permissions is first # not rebuilt on every has() + clock.now += PermissionRegistry.SOURCE_RETRY_SECONDS + assert reg.all_permissions is not first # rebuilt once the retry is due diff --git a/modules/api_tokens/api_tokens/resolver.py b/modules/api_tokens/api_tokens/resolver.py index e82c6d93b..7dec894e4 100644 --- a/modules/api_tokens/api_tokens/resolver.py +++ b/modules/api_tokens/api_tokens/resolver.py @@ -18,7 +18,7 @@ from typing import Any from auth.contracts.schemas import UserContext -from auth.contracts.tokens import AUTH_KIND_PAT, AUTH_KIND_SERVICE, is_api_token +from auth.contracts.tokens import AUTH_KIND_PAT, AUTH_KIND_SERVICE, is_api_token, parse_bearer from simple_module_core.tenancy import is_tenant_role from simple_module_db import ALL_TENANTS_OPTION from sqlalchemy import or_, select, update @@ -42,11 +42,8 @@ def _bearer(request: Request) -> str | None: - header = request.headers.get("authorization", "") - if not header.startswith("Bearer "): - return None - credential = header[7:] - if not is_api_token(credential) or not is_well_formed(credential): + credential = parse_bearer(request.headers.get("authorization")) + if credential is None or not is_api_token(credential) or not is_well_formed(credential): return None return credential diff --git a/modules/api_tokens/tests/test_api_tokens_resolver.py b/modules/api_tokens/tests/test_api_tokens_resolver.py index a54835299..ccb90ad82 100644 --- a/modules/api_tokens/tests/test_api_tokens_resolver.py +++ b/modules/api_tokens/tests/test_api_tokens_resolver.py @@ -5,6 +5,7 @@ import uuid from datetime import UTC, datetime, timedelta +import pytest from _api_tokens_support import FLAGS_URL, create_pat, load_token, update_token from api_tokens.constants import TokenKind from api_tokens.tokens import generate @@ -21,6 +22,16 @@ async def test_valid_pat_reads_but_cannot_write(actor, bearer): assert put.status_code == 403 +@pytest.mark.parametrize("scheme", ["Bearer", "bearer", "BEARER"]) +async def test_scheme_is_case_insensitive(actor, bearer, scheme): + """GH #533: RFC 9110 auth schemes are case-insensitive.""" + async with actor(admin=True) as a: + token = (await create_pat(a.client, ["feature_flags.view"]))["token"] + + resp = await bearer("GET", FLAGS_URL, token, headers={"Authorization": f"{scheme} {token}"}) + assert resp.status_code == 200 + + async def test_revoke_takes_effect_on_the_next_request(actor, bearer): async with actor(admin=True) as a: created = await create_pat(a.client, ["feature_flags.view"]) diff --git a/modules/auth/auth/contracts/tokens.py b/modules/auth/auth/contracts/tokens.py index 446ae3ce8..f6687904d 100644 --- a/modules/auth/auth/contracts/tokens.py +++ b/modules/auth/auth/contracts/tokens.py @@ -31,6 +31,22 @@ API_TOKEN_PREFIXES: tuple[str, ...] = (PAT_PREFIX, SERVICE_KEY_PREFIX) +def parse_bearer(header: str | None) -> str | None: + """The credential of an ``Authorization: Bearer `` header, else None. + + The auth scheme is case-insensitive (RFC 9110 §11.1): clients that build + the header from ``/auth/token``'s ``token_type`` send ``bearer``, and must + authenticate exactly like ``Bearer``. Surrounding and separating whitespace + is tolerated; a header with no credential, or a different scheme, is None. + """ + if not header: + return None + parts = header.split(None, 1) + if len(parts) != 2 or parts[0].lower() != "bearer": + return None + return parts[1].strip() or None + + def is_api_token(credential: str) -> bool: """Whether a bearer value is an ``api_tokens`` credential (by prefix only).""" return credential.startswith(API_TOKEN_PREFIXES) @@ -45,4 +61,5 @@ def is_api_token(credential: str) -> bool: "PAT_PREFIX", "SERVICE_KEY_PREFIX", "is_api_token", + "parse_bearer", ] diff --git a/modules/auth/tests/test_auth_parse_bearer.py b/modules/auth/tests/test_auth_parse_bearer.py new file mode 100644 index 000000000..d2ba7e847 --- /dev/null +++ b/modules/auth/tests/test_auth_parse_bearer.py @@ -0,0 +1,21 @@ +"""``parse_bearer``: the one place an ``Authorization: Bearer`` header is read (GH #533).""" + +from __future__ import annotations + +import pytest +from auth.contracts.tokens import parse_bearer + + +@pytest.mark.parametrize( + "header", + ["Bearer abc", "bearer abc", "BEARER abc", "BeArEr abc", " Bearer abc ", "bearer\tabc"], +) +def test_scheme_is_case_insensitive_and_whitespace_tolerant(header): + assert parse_bearer(header) == "abc" + + +@pytest.mark.parametrize( + "header", [None, "", "Bearer", "Bearer ", "Basic abc", "Bearerabc", "Token abc"] +) +def test_anything_else_is_not_a_bearer_credential(header): + assert parse_bearer(header) is None diff --git a/modules/background_tasks/background_tasks/settings.py b/modules/background_tasks/background_tasks/settings.py index 220c36689..98fac859f 100644 --- a/modules/background_tasks/background_tasks/settings.py +++ b/modules/background_tasks/background_tasks/settings.py @@ -41,7 +41,9 @@ from typing import Any from pydantic import Field, ValidationInfo, field_validator, model_validator +from pydantic_core import PydanticCustomError from pydantic_settings import BaseSettings, SettingsConfigDict +from settings.hydrate import RELATED_FIELDS_CTX from background_tasks.config_guards import parse_task_routes from background_tasks.constants import ( @@ -239,6 +241,26 @@ def _vet_task_routes(cls, value: object) -> object: default=None, ge=1, description="Seconds a durable handler may run per attempt." ) + @field_validator("task_time_limit") + @classmethod + def _soft_limit_below_hard(cls, hard: int | None, info: ValidationInfo) -> int | None: + """A soft limit at or above the hard one can never fire (#548). + + Attached to the later-declared hard limit so the error carries that + field's ``loc`` and the settings form shows it under the field. It names + the soft limit as related so boot hydration can drop a stored soft limit + when the hard one is pinned by env. + """ + soft = info.data.get("task_soft_time_limit") + if hard is not None and soft is not None and soft >= hard: + raise PydanticCustomError( + "time_limit_order", + "task_time_limit ({hard}) must be greater than task_soft_time_limit ({soft}); " + "otherwise the soft limit can never fire", + {"hard": hard, "soft": soft, RELATED_FIELDS_CTX: ["task_soft_time_limit"]}, + ) + return hard + @field_validator("outbox_handler_timeout_seconds") @classmethod def _handler_timeout_fits_the_lease( diff --git a/modules/background_tasks/background_tasks/sync_db.py b/modules/background_tasks/background_tasks/sync_db.py index a8cdf5039..76056ea79 100644 --- a/modules/background_tasks/background_tasks/sync_db.py +++ b/modules/background_tasks/background_tasks/sync_db.py @@ -18,6 +18,7 @@ from collections.abc import Generator from contextlib import contextmanager +from celery.signals import worker_process_init from simple_module_db import DEFAULT_TENANT_ID from simple_module_db.listeners import attach_session_listeners from simple_module_db.query_filter import EngineTenancy, bind_engine_policy @@ -28,6 +29,10 @@ logger = logging.getLogger(__name__) _engine: Engine | None = None +# PID that built ``_engine``. A prefork Celery child inherits the parent's +# engine (and its sockets) across fork(); using them would interleave two +# processes on one Postgres connection. +_engine_pid: int | None = None _session_factory: sessionmaker[Session] | None = None _url_override: str | None = None _tenant_strict: bool = False @@ -71,17 +76,26 @@ def set_database_url( in the web process, instead of landing in ``DEFAULT_TENANT_ID`` where the install's own scoped reads never see it. """ - global _url_override, _engine, _session_factory, _tenant_strict, _default_tenant + global _url_override, _tenant_strict, _default_tenant default_tenant = None if tenant_strict else (default_tenant or None) if (_url_override, _tenant_strict, _default_tenant) == (url, tenant_strict, default_tenant): return _url_override = url _tenant_strict = tenant_strict _default_tenant = default_tenant + _drop_engine() + + +def _drop_engine() -> None: + """Dispose the cached engine, never touching sockets inherited via fork.""" + global _engine, _session_factory, _engine_pid if _engine is not None: - _engine.dispose() + # In a forked child the pooled sockets belong to the parent: close=False + # forgets them without sending a protocol-level close down the shared fd. + _engine.dispose(close=_engine_pid == os.getpid()) _engine = None _session_factory = None + _engine_pid = None def _resolve_url() -> str: @@ -127,9 +141,12 @@ def _build_engine() -> Engine: def get_sync_session_factory() -> sessionmaker[Session]: """Return the process-global sync session factory, building it once.""" - global _engine, _session_factory + global _engine, _session_factory, _engine_pid + if _session_factory is not None and _engine_pid != os.getpid(): + _drop_engine() if _session_factory is None: _engine = _build_engine() + _engine_pid = os.getpid() _session_factory = sessionmaker(bind=_engine, class_=WorkerSession, expire_on_commit=False) return _session_factory @@ -141,16 +158,19 @@ def dispose_sync_engine() -> None: restarts within one process (test runners, uvicorn dev reload) don't accumulate engines against the old DB URL. """ - global _engine, _session_factory, _url_override, _tenant_strict, _default_tenant - if _engine is not None: - _engine.dispose() - _engine = None - _session_factory = None + global _url_override, _tenant_strict, _default_tenant + _drop_engine() _url_override = None _tenant_strict = False _default_tenant = None +@worker_process_init.connect +def _dispose_inherited_engine(**_: object) -> None: + """Forget the parent's engine in a fresh prefork child (rebuilt lazily).""" + _drop_engine() + + @contextmanager def sync_session() -> Generator[Session]: """Open a short-lived sync session; commit on success, rollback on error.""" diff --git a/modules/background_tasks/tests/test_background_tasks_settings.py b/modules/background_tasks/tests/test_background_tasks_settings.py index d488fa410..9c882fbfa 100644 --- a/modules/background_tasks/tests/test_background_tasks_settings.py +++ b/modules/background_tasks/tests/test_background_tasks_settings.py @@ -1,5 +1,7 @@ from __future__ import annotations +import logging + import pytest from background_tasks.settings import BackgroundTasksSettings from pydantic import ValidationError @@ -25,3 +27,70 @@ def test_max_retries_accepts_supported_boundaries(max_retries: int) -> None: def test_max_retries_rejects_values_outside_supported_range(max_retries: int) -> None: with pytest.raises(ValidationError): BackgroundTasksSettings(max_retries=max_retries) + + +def test_soft_time_limit_must_be_below_hard_limit() -> None: + ok = BackgroundTasksSettings(task_soft_time_limit=50, task_time_limit=100) + assert (ok.task_soft_time_limit, ok.task_time_limit) == (50, 100) + assert BackgroundTasksSettings(task_soft_time_limit=50).task_time_limit is None + assert BackgroundTasksSettings(task_time_limit=50).task_soft_time_limit is None + + for soft, hard in ((100, 50), (50, 50)): + with pytest.raises(ValidationError) as exc: + BackgroundTasksSettings(task_soft_time_limit=soft, task_time_limit=hard) + err = exc.value.errors()[0] + assert err["loc"] == ("task_time_limit",) + assert "task_soft_time_limit" in err["msg"] + + +@pytest.mark.asyncio +async def test_stored_invalid_limit_pair_does_not_break_lenient_hydration(db_session) -> None: + from settings.hydrate import hydrate_settings + from settings.service import SettingService + from settings.store import SettingsStore + + store = SettingsStore(SettingService(db_session)) + await store.set_override("background_tasks", "task_soft_time_limit", "100", "int") + await store.set_override("background_tasks", "task_time_limit", "50", "int") + cfg = await hydrate_settings( + BackgroundTasksSettings, store, package="background_tasks", lenient=True + ) + assert cfg.task_soft_time_limit == 100 + assert cfg.task_time_limit is None + + +@pytest.mark.asyncio +async def test_hydration_drops_the_stored_soft_limit(db_session, caplog, monkeypatch) -> None: + """With the hard limit pinned by env, the pair rule reported on + ``task_time_limit`` cannot be satisfied by dropping it; the stored soft + limit is the half to drop (#548).""" + from settings.hydrate import hydrate_settings + from settings.service import SettingService + from settings.store import SettingsStore + + monkeypatch.setenv("SM_BG_TASKS_TASK_TIME_LIMIT", "50") + store = SettingsStore(SettingService(db_session)) + await store.set_override("background_tasks", "task_soft_time_limit", "100", "int") + await store.set_override("background_tasks", "max_retries", "7", "int") + + with caplog.at_level(logging.ERROR, logger="settings.hydrate"): + cfg = await hydrate_settings( + BackgroundTasksSettings, store, "background_tasks", lenient=True + ) + + assert (cfg.task_time_limit, cfg.task_soft_time_limit) == (50, None) + assert cfg.max_retries == 7 # the module's other overrides survive + assert "task_soft_time_limit" in caplog.text + + +@pytest.mark.asyncio +async def test_editor_reports_the_limit_pair_on_the_hard_limit(authenticated_client) -> None: + resp = await authenticated_client.put( + "/api/settings/modules/background_tasks", + json={"task_soft_time_limit": 100, "task_time_limit": 50}, + ) + + assert resp.status_code == 422 + [error] = resp.json()["detail"] + assert error["loc"][-1] == "task_time_limit" + assert "task_soft_time_limit" in error["msg"] diff --git a/modules/background_tasks/tests/test_bg_runner_settings.py b/modules/background_tasks/tests/test_bg_runner_settings.py index 0605e5aa6..58b43050f 100644 --- a/modules/background_tasks/tests/test_bg_runner_settings.py +++ b/modules/background_tasks/tests/test_bg_runner_settings.py @@ -40,6 +40,18 @@ def test_db_overrides_reach_the_worker(db_url: str) -> None: assert load_runner_settings().task_time_limit == 90 +def test_worker_boots_on_its_db_config(db_url: str, monkeypatch) -> None: + """An env-pinned hard limit beside a stored soft limit at or above it (#548): + the stored soft limit is dropped instead of the worker refusing to start.""" + monkeypatch.setenv("SM_BG_TASKS_TASK_TIME_LIMIT", "50") + _put(db_url, "background_tasks.task_soft_time_limit", "100", "int") + _put(db_url, "background_tasks.max_retries", "7", "int") + + cfg = load_runner_settings() + + assert (cfg.task_time_limit, cfg.task_soft_time_limit, cfg.max_retries) == (50, None, 7) + + @pytest.mark.parametrize( ("field", "stored"), [ diff --git a/modules/background_tasks/tests/test_sync_db_fork.py b/modules/background_tasks/tests/test_sync_db_fork.py new file mode 100644 index 000000000..0cbf48fdf --- /dev/null +++ b/modules/background_tasks/tests/test_sync_db_fork.py @@ -0,0 +1,89 @@ +"""The cached sync engine must not survive a fork (#532). + +A prefork Celery child inherits the parent's module globals — including pooled +sockets. Two processes on one Postgres connection interleave protocol frames. +""" + +from __future__ import annotations + +import os +from collections.abc import Iterator + +import pytest +from background_tasks import sync_db +from sqlalchemy import text + + +@pytest.fixture(autouse=True) +def _reset_sync_db() -> Iterator[None]: + sync_db.dispose_sync_engine() + yield + sync_db.dispose_sync_engine() + + +def test_pid_change_rebuilds_engine_without_closing_inherited_sockets( + tmp_path, monkeypatch: pytest.MonkeyPatch +) -> None: + sync_db.set_database_url(f"sqlite:///{tmp_path / 'a.db'}") + sync_db.get_sync_session_factory() + parent = sync_db._engine + assert parent is not None + + closes: list[bool] = [] + real_dispose = parent.dispose + monkeypatch.setattr( + parent, "dispose", lambda close=True: (closes.append(close), real_dispose(close=close)) + ) + + monkeypatch.setattr(os, "getpid", lambda: 999_999) + sync_db.get_sync_session_factory() + + assert sync_db._engine is not parent + assert closes == [False] + assert sync_db._engine_pid == 999_999 + + +def test_same_pid_keeps_engine(tmp_path) -> None: + sync_db.set_database_url(f"sqlite:///{tmp_path / 'a.db'}") + sync_db.get_sync_session_factory() + first = sync_db._engine + sync_db.get_sync_session_factory() + assert sync_db._engine is first + + +def test_worker_process_init_drops_inherited_engine(tmp_path) -> None: + from celery.signals import worker_process_init + + sync_db.set_database_url(f"sqlite:///{tmp_path / 'a.db'}") + sync_db.get_sync_session_factory() + assert sync_db._engine is not None + + worker_process_init.send(sender=None) + + assert sync_db._engine is None + assert sync_db._session_factory is None + + +@pytest.mark.postgres +def test_forked_child_gets_its_own_postgres_connection() -> None: + url = os.environ.get("SM_TEST_DATABASE_URL", "") + if not url.startswith("postgresql"): + pytest.skip("needs SM_TEST_DATABASE_URL on Postgres") + sync_db.set_database_url(url) + + def backend_pid() -> int: + with sync_db.sync_session() as s: + return int(s.execute(text("select pg_backend_pid()")).scalar_one()) + + parent_pid = backend_pid() + r, w = os.pipe() + child = os.fork() + if child == 0: + try: + os.write(w, str(backend_pid()).encode()) + finally: + os._exit(0) + os.waitpid(child, 0) + child_backend = int(os.read(r, 32)) + assert child_backend != parent_pid + assert backend_pid() == parent_pid diff --git a/modules/file_storage/file_storage/constants.py b/modules/file_storage/file_storage/constants.py index 62ff947da..6bcce228f 100644 --- a/modules/file_storage/file_storage/constants.py +++ b/modules/file_storage/file_storage/constants.py @@ -152,6 +152,9 @@ class I18nKey: DEFAULT_CHUNK_SIZE: Final = 64 * 1024 # 64 KB SPOOL_MAX_SIZE_BYTES: Final = 10 * 1024 * 1024 # 10 MB before disk-spill +# Largest byte count a browser's ``Number`` represents exactly (2**53 - 1). +MAX_JS_SAFE_INTEGER: Final = 2**53 - 1 + # ── Resumable uploads (#454) ───────────────────────────────────────── MIB: Final = 1024 * 1024 DEFAULT_DIRECT_UPLOAD_THRESHOLD_BYTES: Final = 64 * MIB @@ -177,21 +180,42 @@ class I18nKey: # Per-IP budget for anonymous public file GETs (its own bucket in the shared # rate limiter, #347), wider than the default because one page embeds many. PUBLIC_FILES_RATE: Final = "600/minute" -# Types a browser would execute or render as a document on our origin. They are -# served as attachments (and sandboxed) so a public upload cannot become stored -# XSS on the app's origin. -ACTIVE_CONTENT_TYPES: Final = frozenset( +# The only types served ``inline`` (and, on S3, by redirect). Everything else — +# HTML, any XML (``*+xml``, ``text/xsl``, SVG), scripts, unknown or malformed +# values — is "active": forced to an attachment and streamed under the sandbox +# CSP, so a public upload cannot become stored XSS on the app's origin (#545). +# An allow-list, because the uploader controls the stored type and a deny-list +# has to anticipate every scriptable document format a browser will render. +INLINE_SAFE_CONTENT_TYPES: Final = frozenset( { - "text/html", - "application/xhtml+xml", - "image/svg+xml", - "text/xml", - "application/xml", - "text/javascript", - "application/javascript", - "application/x-shockwave-flash", + "image/png", + "image/jpeg", + "image/gif", + "image/webp", + "image/avif", + "image/apng", + "image/bmp", + "image/tiff", + "image/x-icon", + "image/vnd.microsoft.icon", + "application/pdf", + "application/json", + "application/geo+json", + "text/plain", + "text/csv", + # Opaque binaries a browser only ever downloads or hands to script — + # kept inline so S3 range readers (PMTiles, archives) still get the + # presigned redirect instead of streaming through the app. + "application/octet-stream", + "application/zip", + "application/gzip", + "application/x-protobuf", + "application/vnd.mapbox-vector-tile", } ) +# Whole families that render as media, never as a document — minus any XML or +# HTML subtype, which ``media_types.is_active_content`` refuses on its own. +INLINE_SAFE_FAMILIES: Final = frozenset({"audio", "video", "font"}) PUBLIC_CSP: Final = "default-src 'none'; style-src 'unsafe-inline'; sandbox" THUMBNAIL_WIDTHS: Final = (64, 128, 256, 512, 1024) diff --git a/modules/file_storage/file_storage/deps.py b/modules/file_storage/file_storage/deps.py index 6259e1bd6..fc0cf4816 100644 --- a/modules/file_storage/file_storage/deps.py +++ b/modules/file_storage/file_storage/deps.py @@ -4,12 +4,19 @@ from fastapi import Depends, Request from simple_module_core.events import EventBus +from simple_module_core.permissions import grants from simple_module_db.deps import get_db +from simple_module_db.listeners import current_user_id +from simple_module_hosting.permissions import ensure_resolved_permissions from sqlalchemy.ext.asyncio import AsyncSession +from file_storage import constants from file_storage.service import FileStorageService from file_storage.services import FileStorageServices +# Matches no row: ``created_by`` is a user id or NULL, never the empty string. +NO_OWNER = "" + async def get_file_storage_services(request: Request) -> FileStorageServices: return request.app.state.file_storage @@ -22,5 +29,28 @@ async def get_file_storage_service( return FileStorageService(db, services.backend, services.settings, services.aggregates) +async def get_owner_scope(request: Request) -> str | None: + """Whose files this request may see and change (#544). + + ``None`` — every file in the active tenant — for a caller holding + ``file_storage.manage`` (platform admins via the wildcard, organisation + admins and owners). Anyone else is confined to the rows they uploaded: the + id is read from the same ``current_user_id`` the audit listener stamps + ``created_by`` with, so the two cannot disagree. A principal with no id + gets :data:`NO_OWNER`, which matches nothing rather than everything. + + Endpoints hand the result to the service, which applies it in the query — + so another user's file misses exactly like an unknown id (404, not 403). + Internal callers (branding, reapers, tasks) never pass it and stay + unscoped. + """ + if grants(await ensure_resolved_permissions(request), constants.Permission.MANAGE): + return None + return current_user_id.get() or NO_OWNER + + +OwnerScope = Depends(get_owner_scope) + + async def get_event_bus(request: Request) -> EventBus: return request.app.state.sm.event_bus diff --git a/modules/file_storage/file_storage/endpoints/api.py b/modules/file_storage/file_storage/endpoints/api.py index 761a58986..1432dcb69 100644 --- a/modules/file_storage/file_storage/endpoints/api.py +++ b/modules/file_storage/file_storage/endpoints/api.py @@ -32,7 +32,7 @@ StoredFileUpdate, ) from file_storage.contracts.service import StorageNotFoundError -from file_storage.deps import get_event_bus, get_file_storage_service +from file_storage.deps import OwnerScope, 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 @@ -122,9 +122,15 @@ async def list_files( default=constants.DEFAULT_SORT ), service: FileStorageService = Depends(get_file_storage_service), + owner: str | None = OwnerScope, ) -> StoredFileListOut: items, total = await service.list_files( - page=page, per_page=per_page, search=q, content_type=content_type, sort=sort + page=page, + per_page=per_page, + created_by=owner, + search=q, + content_type=content_type, + sort=sort, ) return StoredFileListOut(items=items, total=total, page=page, per_page=per_page) @@ -138,9 +144,10 @@ async def get_file( file_id: uuid.UUID, t: TranslatorDep, service: FileStorageService = Depends(get_file_storage_service), + owner: str | None = OwnerScope, ) -> StoredFileOut: try: - row = await service.get(file_id) + row = await service.get(file_id, owner=owner) except StoredFileNotFoundError as exc: raise not_found(t) from exc return StoredFileOut.model_validate(queries.to_out_dict(row)) @@ -156,10 +163,11 @@ async def update_file( body: StoredFileUpdate, t: TranslatorDep, service: FileStorageService = Depends(get_file_storage_service), + owner: str | None = OwnerScope, ) -> StoredFileOut: - """Publish or unpublish a file (anyone allowed to upload may decide).""" + """Publish or unpublish a file the caller uploaded (or any, with manage).""" try: - row = await service.set_public(file_id, body.public) + row = await service.set_public(file_id, body.public, owner=owner) except StoredFileNotFoundError as exc: raise not_found(t) from exc return StoredFileOut.model_validate(queries.to_out_dict(row)) @@ -175,9 +183,10 @@ async def file_thumbnail( t: TranslatorDep, w: int | None = Query(default=None, description="Width in px; clamped and snapped."), service: FileStorageService = Depends(get_file_storage_service), + owner: str | None = OwnerScope, ) -> Response: try: - row = await service.get(file_id) + row = await service.get(file_id, owner=owner) except StoredFileNotFoundError as exc: raise not_found(t) from exc return await thumbnail_response( @@ -195,9 +204,10 @@ async def download_file( request: Request, t: TranslatorDep, service: FileStorageService = Depends(get_file_storage_service), + owner: str | None = OwnerScope, ) -> Response: try: - download = await service.download(file_id, open_body=False) + download = await service.download(file_id, open_body=False, owner=owner) if isinstance(download, RedirectDownload): return RedirectResponse(url=download.url, status_code=status.HTTP_302_FOUND) @@ -227,6 +237,7 @@ async def bulk_delete_files( body: BulkDeleteRequest, service: FileStorageService = Depends(get_file_storage_service), bus: EventBus = Depends(get_event_bus), + owner: str | None = OwnerScope, ) -> BulkDeleteResult: """Delete a selection in one request. @@ -237,7 +248,7 @@ async def bulk_delete_files( subscriber that mirrors or reindexes files cannot tell a bulk delete from a run of single ones. """ - rows = await service.delete_many(body.ids) + rows = await service.delete_many(body.ids, owner=owner) for row in rows: await bus.publish(FileDeleted(file_id=row.id, key=row.key)) return BulkDeleteResult(deleted=len(rows), ids=[row.id for row in rows]) @@ -253,9 +264,10 @@ async def delete_file( t: TranslatorDep, service: FileStorageService = Depends(get_file_storage_service), bus: EventBus = Depends(get_event_bus), + owner: str | None = OwnerScope, ) -> None: try: - row = await service.delete(file_id) + row = await service.delete(file_id, owner=owner) except StoredFileNotFoundError as exc: raise not_found(t) from exc await bus.publish(FileDeleted(file_id=row.id, key=row.key)) diff --git a/modules/file_storage/file_storage/endpoints/views.py b/modules/file_storage/file_storage/endpoints/views.py index c9b0d8804..ca218c1d8 100644 --- a/modules/file_storage/file_storage/endpoints/views.py +++ b/modules/file_storage/file_storage/endpoints/views.py @@ -12,8 +12,9 @@ from sqlalchemy.ext.asyncio import AsyncSession from file_storage import constants +from file_storage.aggregates import StorageAggregates from file_storage.contracts.schemas import StoredFileOut -from file_storage.deps import get_file_storage_service +from file_storage.deps import OwnerScope, get_file_storage_service from file_storage.resolve import label_for, resolve_uploader_labels from file_storage.service import FileStorageService @@ -35,6 +36,7 @@ async def browse( uploaded_by: str = "", db: AsyncSession = Depends(get_db), service: FileStorageService = Depends(get_file_storage_service), + owner: str | None = OwnerScope, ) -> InertiaResponse: # A user-supplied page below 1 (0, negative, a hand-edited ?page=) must # clamp to the first page rather than reject — mirrors the past-the-end @@ -44,7 +46,9 @@ async def browse( filters = { "search": q or None, "content_type": content_type or None, - "created_by": uploaded_by or None, + # Without file_storage.manage the screen is the caller's own uploads + # (#544); the uploader filter cannot widen it. + "created_by": owner if owner is not None else (uploaded_by or None), } # Count before paging, not after. A page requested past the end (reloading # after deleting the last row on that page, or a stale ?page= link) must be @@ -63,11 +67,11 @@ async def browse( # its own alternatives is a dead end) and so the usage figure keeps # describing the bucket rather than the search box. aggregates = await service.storage_aggregates() + content_types, uploaders = await _facets(service, aggregates, owner) # One lookup covers the rows on screen and the filter's options, so # picking someone from the dropdown never renames them. labels = await resolve_uploader_labels( - db, - [item.uploaded_by for item in items] + [f.value for f in aggregates.uploaders], + db, [item.uploaded_by for item in items] + [u["id"] for u in uploaders] ) settings = service.settings # The page name is hard-coded as a literal here (rather than via @@ -81,11 +85,8 @@ async def browse( "files": [_file_payload(item, labels) for item in items], "pagination": {"page": page, "perPage": _PER_PAGE, "total": total}, "filters": {"q": q, "content_type": content_type, "uploaded_by": uploaded_by}, - "content_types": [facet.as_dict() for facet in aggregates.content_types], - "uploaders": [ - {"id": f.value, "label": label_for(f.value, labels), "count": f.count} - for f in aggregates.uploaders - ], + "content_types": content_types, + "uploaders": [{**u, "label": label_for(u["id"], labels)} for u in uploaders], # What the header subtitle states about the bucket. ``quota_bytes`` # stays None until an operator says what the ceiling is — the # screen reports usage rather than inventing a total. @@ -106,6 +107,25 @@ async def browse( ) +async def _facets( + service: FileStorageService, aggregates: StorageAggregates, owner: str | None +) -> tuple[list[dict], list[dict]]: + """The type and uploader dropdowns: bucket-wide, or the caller's own slice. + + A caller confined to their own files must not learn who else uploaded what + from the facet lists either, so theirs come from their rows alone. The + usage figure stays bucket-wide: the quota it is measured against is. + """ + if owner is None: + return ( + [facet.as_dict() for facet in aggregates.content_types], + [{"id": f.value, "count": f.count} for f in aggregates.uploaders], + ) + content_types = await service.content_type_facets(created_by=owner) + total = sum(facet["count"] for facet in content_types) + return content_types, ([{"id": owner, "count": total}] if total else []) + + def _file_payload(item: StoredFileOut, labels: dict[str, str]) -> dict: """One row, with its uploader resolved to something readable.""" payload = item.model_dump(mode="json") diff --git a/modules/file_storage/file_storage/media_types.py b/modules/file_storage/file_storage/media_types.py new file mode 100644 index 000000000..f15447218 --- /dev/null +++ b/modules/file_storage/file_storage/media_types.py @@ -0,0 +1,57 @@ +"""Which stored content types are safe to hand a browser inline (#545). + +The stored type comes from the uploader (``allowed_content_types`` is ``None`` +by default), so this decides from a strict allow-list rather than a list of +known-dangerous types: a deny-list missed ``application/rss+xml``, +``text/xsl`` and the rest of the scriptable XML family, and an exact-match +check let ``Text/HTML`` or ``text/html,`` through. + +A malformed value is never "cleaned up" into a safe-looking one: browsers +disagree on how to read ``image/png,text/html`` (Fetch's MIME extraction takes +the *last* type), so anything that is not exactly one ``type/subtype`` token +pair is treated as active. +""" + +from __future__ import annotations + +import re + +from file_storage import constants +from file_storage.thumbnails import base_type + +# RFC 9110 ``token`` characters, on both sides of a single "/". +_MEDIA_TYPE = re.compile(r"[a-z0-9!#$%&'*+.^_`|~-]+/[a-z0-9!#$%&'*+.^_`|~-]+") + + +def normalise_media_type(content_type: str | None) -> str | None: + """``type/subtype`` lower-cased with parameters dropped; ``None`` if malformed. + + ``Image/PNG; charset=x`` -> ``image/png``. ``text/html,``, a comma or quote + anywhere (even in a parameter), an empty value, or anything with stray + characters around the pair -> ``None``. + """ + # A comma anywhere, parameters included, splits the header for a browser: + # ``image/png;x=1,text/html`` is read as ``text/html`` while the part + # before ";" looks safe. Quotes would let a parameter hide one. + if not content_type or any(c in content_type for c in ',"'): + return None + media = base_type(content_type) + return media if _MEDIA_TYPE.fullmatch(media) else None + + +def is_active_content(content_type: str | None) -> bool: + """Whether the type must be served as a sandboxed attachment, never inline. + + True for everything outside :data:`~file_storage.constants.INLINE_SAFE_CONTENT_TYPES` + and the media families, for any XML subtype even inside those families, + and for any value :func:`normalise_media_type` cannot read. + """ + media = normalise_media_type(content_type) + if media is None: + return True + if media in constants.INLINE_SAFE_CONTENT_TYPES: + return False + family, subtype = media.split("/", 1) + if "xml" in subtype or "html" in subtype: + return True + return family not in constants.INLINE_SAFE_FAMILIES diff --git a/modules/file_storage/file_storage/module.py b/modules/file_storage/file_storage/module.py index 58a6bb1b6..91b491ca2 100644 --- a/modules/file_storage/file_storage/module.py +++ b/modules/file_storage/file_storage/module.py @@ -160,8 +160,14 @@ def register_permissions(self, registry: PermissionRegistry) -> None: # would hand it straight back to each tenant member. registry.map_role(constants.USER_ROLE, read_write) registry.map_role(tenant_role(TenantRole.MEMBER), read_write) + # ``manage`` widens every route from "files I uploaded" to "every file + # in the active tenant" (#544): organisation admins and owners curate + # their members' uploads. Platform admins hold it through ``*``. for role in (TenantRole.ADMIN, TenantRole.OWNER): - registry.map_role(tenant_role(role), [*read_write, constants.Permission.DELETE]) + registry.map_role( + tenant_role(role), + [*read_write, constants.Permission.DELETE, constants.Permission.MANAGE], + ) def register_menu_items(self, registry: MenuRegistry) -> None: registry.add( diff --git a/modules/file_storage/file_storage/queries.py b/modules/file_storage/file_storage/queries.py index 2fbd680bd..9ee747497 100644 --- a/modules/file_storage/file_storage/queries.py +++ b/modules/file_storage/file_storage/queries.py @@ -16,7 +16,7 @@ from urllib.parse import quote from simple_module_db import LIKE_ESCAPE_CHAR, like_contains_pattern, like_prefix_pattern -from sqlalchemy import func, select +from sqlalchemy import Select, func, select from sqlalchemy.ext.asyncio import AsyncSession from file_storage import constants @@ -35,6 +35,11 @@ def order_clauses(sort: str) -> list: return [column.desc() if sort.startswith("-") else column.asc(), StoredFile.id] +def owned_by(stmt: Select, owner: str | None) -> Select: + """Narrow ``stmt`` to ``owner``'s uploads; ``None`` leaves it unscoped (#544).""" + return stmt if owner is None else stmt.where(StoredFile.created_by == owner) + + def filter_clauses( *, created_by: str | None, diff --git a/modules/file_storage/file_storage/service.py b/modules/file_storage/file_storage/service.py index b8de759b0..9003b6d98 100644 --- a/modules/file_storage/file_storage/service.py +++ b/modules/file_storage/file_storage/service.py @@ -153,15 +153,19 @@ def _check_content_type(self, content_type: str) -> None: if allowed is not None and content_type not in allowed: raise ContentTypeNotAllowedError(f"Content-Type {content_type!r} not in allow-list.") - async def get(self, file_id: uuid.UUID, *, platform: bool = False) -> StoredFile: + async def get( + self, file_id: uuid.UUID, *, platform: bool = False, owner: str | None = None + ) -> StoredFile: """The bound tenant's file, or with ``platform=True`` a platform file. Another tenant's id misses exactly like an unknown one. ``platform`` needs no tenant bound (anonymous branding requests have none) and only ever matches a platform-owned row, so a tenant's file id handed to a platform caller — say, pasted into a branding setting — still misses. + ``owner`` (``deps.get_owner_scope``) narrows it to one uploader's rows + the same way: someone else's file misses like an unknown id (#544). """ - stmt = select(StoredFile).where(StoredFile.id == file_id) + stmt = queries.owned_by(select(StoredFile).where(StoredFile.id == file_id), owner) if platform: stmt = stmt.where(StoredFile.tenant_id == PLATFORM_TENANT_ID) async with platform_scope(self.db, platform): @@ -171,7 +175,12 @@ async def get(self, file_id: uuid.UUID, *, platform: bool = False) -> StoredFile return row async def download( - self, file_id: uuid.UUID, *, platform: bool = False, open_body: bool = True + self, + file_id: uuid.UUID, + *, + platform: bool = False, + open_body: bool = True, + owner: str | None = None, ) -> Download: """Return either a streamed body or a redirect URL. @@ -179,7 +188,7 @@ async def download( provider-agnostic — adding a new backend that supports presigning works without touching this method. """ - row = await self.get(file_id, platform=platform) + row = await self.get(file_id, platform=platform, owner=owner) if self.backend.supports_presigned_url: url = await presigned_download_url( self.backend, row, self.settings.s3_presign_ttl_seconds, attachment=True @@ -190,7 +199,9 @@ async def download( # ── Delete ─────────────────────────────────────────────────────── - async def delete_many(self, file_ids: Sequence[uuid.UUID]) -> list[StoredFile]: + async def delete_many( + self, file_ids: Sequence[uuid.UUID], *, owner: str | None = None + ) -> list[StoredFile]: """Soft-delete every id that still resolves, returning the rows removed. Ids that no longer resolve are skipped rather than raising: two admins @@ -200,11 +211,8 @@ async def delete_many(self, file_ids: Sequence[uuid.UUID]) -> list[StoredFile]: """ if not file_ids: return [] - rows = list( - (await self.db.execute(select(StoredFile).where(StoredFile.id.in_(list(file_ids))))) - .scalars() - .all() - ) + stmt = queries.owned_by(select(StoredFile).where(StoredFile.id.in_(list(file_ids))), owner) + rows = list((await self.db.execute(stmt)).scalars().all()) if not rows: return [] @@ -240,7 +248,12 @@ async def delete_many(self, file_ids: Sequence[uuid.UUID]) -> list[StoredFile]: return rows async def delete( - self, file_id: uuid.UUID, *, platform: bool = False, drop_object: bool = True + self, + file_id: uuid.UUID, + *, + platform: bool = False, + drop_object: bool = True, + owner: str | None = None, ) -> StoredFile: """Soft-delete the row and (by default) drop the backend object. @@ -251,7 +264,7 @@ async def delete( # One scope around read + write: its opening flush runs before the # platform row is touched, so the guard never sees that change. async with platform_scope(self.db, platform): - row = await self.get(file_id, platform=platform) + row = await self.get(file_id, platform=platform, owner=owner) # Soft-delete in DB first; if the backend delete fails afterwards we # still have a row marked deleted that can be reaped by a janitor. row.is_deleted = True diff --git a/modules/file_storage/file_storage/serving.py b/modules/file_storage/file_storage/serving.py index f348282e5..08fba06c3 100644 --- a/modules/file_storage/file_storage/serving.py +++ b/modules/file_storage/file_storage/serving.py @@ -9,6 +9,7 @@ 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.media_types import is_active_content from file_storage.models import StoredFile from file_storage.ranges import file_body_response from file_storage.service import FileStorageService @@ -46,10 +47,6 @@ def _not_modified(request: Request, etag: str) -> bool: return etag in {part.strip().removeprefix("W/") for part in sent.split(",")} -def is_active_content(content_type: str) -> bool: - return thumbnails.base_type(content_type) in constants.ACTIVE_CONTENT_TYPES - - async def thumbnail_response( service: FileStorageService, row: StoredFile, diff --git a/modules/file_storage/file_storage/settings.py b/modules/file_storage/file_storage/settings.py index e85b7535f..a8f10c621 100644 --- a/modules/file_storage/file_storage/settings.py +++ b/modules/file_storage/file_storage/settings.py @@ -85,8 +85,12 @@ class FileStorageSettings(DbBackedSettings): # only for installs that have said what 5 GB means to them. Left unset, # the screen reports usage without inventing a ceiling, and uploads are # bounded by ``max_file_size_bytes`` alone as before. + # Positive, and no larger than a browser can hold exactly: the screen and + # the settings editor read it as a JavaScript number (#548). quota_bytes: int | None = Field( default=None, + gt=0, + le=constants.MAX_JS_SAFE_INTEGER, description="Total storage the browse screen measures usage against. None = unmeasured.", ) diff --git a/modules/file_storage/file_storage/visibility.py b/modules/file_storage/file_storage/visibility.py index a91ece7f5..d1dbc2141 100644 --- a/modules/file_storage/file_storage/visibility.py +++ b/modules/file_storage/file_storage/visibility.py @@ -30,7 +30,9 @@ class FileStoragePublic: if TYPE_CHECKING: - async def get(self, file_id: uuid.UUID, *, platform: bool = False) -> StoredFile: ... + async def get( + self, file_id: uuid.UUID, *, platform: bool = False, owner: str | None = None + ) -> StoredFile: ... async def get_public(self, file_id: uuid.UUID) -> StoredFile: """A ``public`` file by id, whichever tenant owns it; anything else misses. @@ -54,9 +56,11 @@ async def get_public(self, file_id: uuid.UUID) -> StoredFile: raise StoredFileNotFoundError(str(file_id)) return row - async def set_public(self, file_id: uuid.UUID, public: bool) -> StoredFile: - """Publish or unpublish one of the bound tenant's files.""" - row = await self.get(file_id) + async def set_public( + self, file_id: uuid.UUID, public: bool, *, owner: str | None = None + ) -> StoredFile: + """Publish or unpublish one of the bound tenant's files (``owner``'s, if given).""" + row = await self.get(file_id, owner=owner) row.public = public await self.db.flush() await self.db.refresh(row) diff --git a/modules/file_storage/tests/file_storage_uploads_helpers.py b/modules/file_storage/tests/file_storage_uploads_helpers.py index 7f0f17525..8b4aec651 100644 --- a/modules/file_storage/tests/file_storage_uploads_helpers.py +++ b/modules/file_storage/tests/file_storage_uploads_helpers.py @@ -1,4 +1,4 @@ -"""Shared helpers for the resumable-upload tests (imported, not collected).""" +"""Shared helpers for the file_storage upload tests (imported, not collected).""" from __future__ import annotations @@ -10,6 +10,14 @@ API = constants.ROUTE_PREFIX_API MIB = constants.MIB PART = 5 * MIB # the smallest part size the settings accept +INERTIA = {"X-Inertia": "true", "Accept": "application/json"} + + +async def upload_text(client: httpx.AsyncClient, name: str, body: bytes = b"hi") -> str: + """Upload ``body`` as a one-shot ``text/plain`` file; return its id.""" + resp = await client.post(f"{API}/upload", files={"file": (name, body, "text/plain")}) + assert resp.status_code == 201, resp.text + return resp.json()["id"] async def open_session( diff --git a/modules/file_storage/tests/test_file_storage_media_types.py b/modules/file_storage/tests/test_file_storage_media_types.py new file mode 100644 index 000000000..070da9ca0 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_media_types.py @@ -0,0 +1,112 @@ +"""Inline serving is an allow-list; everything else is a sandboxed attachment (#545).""" + +from __future__ import annotations + +import pytest +from file_storage import constants +from file_storage.media_types import is_active_content, normalise_media_type + +API = constants.ROUTE_PREFIX_API + +ACTIVE = [ + # The old deny-list's members, still refused. + "text/html", + "application/xhtml+xml", + "image/svg+xml", + "text/xml", + "application/xml", + "text/javascript", + "application/javascript", + "application/x-shockwave-flash", + # Scriptable XML the deny-list missed. + "application/atom+xml", + "application/rss+xml", + "application/mathml+xml", + "text/xsl", + "application/vnd.wap.xhtml+xml", + "application/xslt+xml", + "video/x-something+xml", + # Casing, parameters and whitespace no longer slip past an exact match. + "Text/HTML", + "TEXT/HTML; charset=utf-8", + " text/html ", + "image/SVG+XML;x=y", + # Malformed: never cleaned up into something that looks safe. + "text/html,", + "image/png,text/html", + "image/png;x=1,text/html", + 'image/png; x="a,b"', + "image/png text/html", + "image/", + "/png", + "html", + "", + None, + # Unknown types download rather than render. + "text/markdown", + "application/x-unknown", +] + +INLINE = [ + "image/png", + "IMAGE/PNG", + "image/jpeg; q=0.9", + "image/gif", + "image/webp", + "image/avif", + "image/x-icon", + "application/pdf", + "application/json", + "text/plain; charset=utf-8", + "text/csv", + "video/mp4", + "audio/mpeg", + "font/woff2", + "application/octet-stream", + "application/zip", + "application/geo+json", +] + + +@pytest.mark.parametrize("content_type", ACTIVE) +def test_active(content_type): + assert is_active_content(content_type) is True + + +@pytest.mark.parametrize("content_type", INLINE) +def test_inline_safe(content_type): + assert is_active_content(content_type) is False + + +@pytest.mark.parametrize( + ("raw", "expected"), + [ + ("Image/PNG; charset=x", "image/png"), + (" text/plain ", "text/plain"), + ("application/rss+xml", "application/rss+xml"), + ("text/html,", None), + ("a/b/c", None), + ("", None), + ], +) +def test_normalise_media_type(raw, expected): + assert normalise_media_type(raw) == expected + + +@pytest.mark.parametrize( + ("ctype", "disposition"), + [("application/rss+xml", "attachment"), ("text/xsl", "attachment"), ("image/png", "inline")], +) +async def test_public_route_applies_the_rule(authenticated_client, client, ctype, disposition): + resp = await authenticated_client.post( + f"{API}{constants.PATH_UPLOAD}", + files={"file": ("f.bin", b"", ctype)}, + data={"public": "true"}, + ) + assert resp.status_code == 201, resp.text + + served = await client.get(f"{API}/public/{resp.json()['id']}") + + assert served.status_code == 200 + assert served.headers["content-disposition"].startswith(disposition) + assert "sandbox" in served.headers["content-security-policy"] diff --git a/modules/file_storage/tests/test_file_storage_owner_scope.py b/modules/file_storage/tests/test_file_storage_owner_scope.py new file mode 100644 index 000000000..9ec1a9b5d --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_owner_scope.py @@ -0,0 +1,140 @@ +"""Ordinary users see and change only the files they uploaded (#544). + +``file_storage.download``/``upload`` are held by every account, so without an +owner check any member could list, download and publish everyone's files. +Holding ``file_storage.manage`` — platform admins, organisation admins and +owners — widens the scope back to the whole tenant. Someone else's file id +answers 404, exactly like an unknown one, so ids cannot be probed. +""" + +from __future__ import annotations + +import httpx +import pytest +from file_storage import constants +from file_storage_uploads_helpers import API, INERTIA, upload_text +from simple_module_hosting.settings import Settings +from simple_module_test.tenant_client import create_user, session_client + +VIEW = f"{constants.ROUTE_PREFIX_VIEW}/" + + +async def _names(client: httpx.AsyncClient) -> tuple[list[str], int]: + resp = await client.get(f"{API}/files") + assert resp.status_code == 200, resp.text + body = resp.json() + return sorted(f["filename"] for f in body["items"]), body["total"] + + +class TestMembers: + async def test_member_lists_only_own_files(self, tenant_client): + async with ( + tenant_client("member") as a, + tenant_client("member", tenant_id=a.tenant_id) as b, + ): + await upload_text(a.client, "a.txt") + await upload_text(b.client, "b.txt") + + assert await _names(a.client) == (["a.txt"], 1) + assert await _names(b.client) == (["b.txt"], 1) + + async def test_another_members_file_is_not_found(self, tenant_client): + async with ( + tenant_client("member") as a, + tenant_client("member", tenant_id=a.tenant_id) as b, + ): + file_id = await upload_text(a.client, "a.txt") + + assert (await b.client.get(f"{API}/files/{file_id}")).status_code == 404 + assert (await b.client.get(f"{API}/files/{file_id}/download")).status_code == 404 + assert (await b.client.get(f"{API}/files/{file_id}/thumbnail")).status_code == 404 + patch = await b.client.patch(f"{API}/files/{file_id}", json={"public": True}) + assert patch.status_code == 404 + + # The owner's file is untouched by the attempts, and still theirs. + own = await a.client.get(f"{API}/files/{file_id}") + assert own.status_code == 200 + assert own.json()["public"] is False + download = await a.client.get(f"{API}/files/{file_id}/download") + assert download.status_code == 200 + assert download.content == b"hi" + + async def test_member_can_publish_own_file(self, tenant_client): + async with tenant_client("member") as a: + file_id = await upload_text(a.client, "a.txt") + resp = await a.client.patch(f"{API}/files/{file_id}", json={"public": True}) + assert resp.status_code == 200, resp.text + assert resp.json()["public"] is True + + async def test_browse_shows_only_own_files_and_facets(self, tenant_client): + async with ( + tenant_client("member") as a, + tenant_client("member", tenant_id=a.tenant_id) as b, + ): + await upload_text(a.client, "a.txt") + await b.client.post(f"{API}/upload", files={"file": ("b.csv", b"x,y", "text/csv")}) + + # The uploader filter cannot widen the scope to someone else. + resp = await a.client.get(VIEW, params={"uploaded_by": b.user_id}, headers=INERTIA) + assert resp.status_code == 200, resp.text + props = resp.json()["props"] + + assert [f["filename"] for f in props["files"]] == ["a.txt"] + assert props["pagination"]["total"] == 1 + assert [c["value"] for c in props["content_types"]] == ["text/plain"] + assert [u["id"] for u in props["uploaders"]] == [a.user_id] + + +class TestManagers: + @pytest.mark.parametrize("role", ["admin", "owner"]) + async def test_org_admins_see_and_change_every_members_file(self, tenant_client, role): + async with ( + tenant_client("member") as a, + tenant_client(role, tenant_id=a.tenant_id) as boss, + ): + file_id = await upload_text(a.client, "a.txt") + + assert await _names(boss.client) == (["a.txt"], 1) + assert (await boss.client.get(f"{API}/files/{file_id}/download")).status_code == 200 + patch = await boss.client.patch(f"{API}/files/{file_id}", json={"public": True}) + assert patch.status_code == 200 + assert (await boss.client.delete(f"{API}/files/{file_id}")).status_code == 204 + + async def test_platform_admin_sees_every_file( + self, authenticated_client, admin_tenant_id, tenant_client + ): + async with ( + tenant_client("member", tenant_id=admin_tenant_id) as a, + tenant_client("member", tenant_id=admin_tenant_id) as b, + ): + await upload_text(a.client, "a.txt") + await upload_text(b.client, "b.txt") + + assert await _names(authenticated_client) == (["a.txt", "b.txt"], 2) + props = (await authenticated_client.get(VIEW, headers=INERTIA)).json()["props"] + assert len(props["uploaders"]) == 2 + + +class TestSingleTenant: + """With tenancy off every account is in one tenant and holds ``delete``.""" + + @pytest.fixture + def settings(self, settings: Settings) -> Settings: + return settings.model_copy(update={"multi_tenant": False, "tenant_header": ""}) + + async def test_users_cannot_see_or_delete_each_others_files(self, app): + alice = await create_user(app, "alice@example.com") + bob = await create_user(app, "bob@example.com") + async with ( + session_client(app, {"user_id": alice}) as a, + session_client(app, {"user_id": bob}) as b, + ): + file_id = await upload_text(a, "a.txt") + + assert await _names(b) == ([], 0) + assert (await b.delete(f"{API}/files/{file_id}")).status_code == 404 + bulk = await b.post(f"{API}/files/bulk-delete", json={"ids": [file_id]}) + assert bulk.json() == {"deleted": 0, "ids": []} + + assert await _names(a) == (["a.txt"], 1) + assert (await a.delete(f"{API}/files/{file_id}")).status_code == 204 diff --git a/modules/file_storage/tests/test_file_storage_quota_setting.py b/modules/file_storage/tests/test_file_storage_quota_setting.py new file mode 100644 index 000000000..a533aa053 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_quota_setting.py @@ -0,0 +1,32 @@ +"""``quota_bytes`` must be a positive, JS-safe byte count or unset (#548).""" + +from __future__ import annotations + +import pytest +from file_storage import constants +from file_storage.settings import FileStorageSettings +from pydantic import ValidationError + +_API = "/api/settings/modules/file_storage" + + +@pytest.mark.parametrize("value", [-1, 0, constants.MAX_JS_SAFE_INTEGER + 1, 2**63]) +def test_out_of_range_quota_is_rejected(value): + with pytest.raises(ValidationError): + FileStorageSettings(quota_bytes=value) + + +@pytest.mark.parametrize("value", [None, 1, 5 * 1024**3, constants.MAX_JS_SAFE_INTEGER]) +def test_unset_or_positive_quota_is_accepted(value): + assert FileStorageSettings(quota_bytes=value).quota_bytes == value + + +async def test_settings_api_refuses_a_negative_quota(authenticated_client, app): + resp = await authenticated_client.put(_API, json={"quota_bytes": -1}) + + assert resp.status_code == 422, resp.text + assert app.state.file_storage.settings.quota_bytes is None + + resp = await authenticated_client.put(_API, json={"quota_bytes": 1024}) + assert resp.status_code == 200, resp.text + assert app.state.file_storage.settings.quota_bytes == 1024 diff --git a/modules/keycloak/keycloak/provider.py b/modules/keycloak/keycloak/provider.py index 8e526d0a2..b3092cd13 100644 --- a/modules/keycloak/keycloak/provider.py +++ b/modules/keycloak/keycloak/provider.py @@ -7,7 +7,7 @@ from typing import TYPE_CHECKING, Any from auth.contracts.schemas import UserContext -from auth.contracts.tokens import AUTH_KIND_BEARER, is_api_token +from auth.contracts.tokens import AUTH_KIND_BEARER, is_api_token, parse_bearer from simple_module_core.tenancy import is_tenant_role from simple_module_db import is_valid_tenant_id from starlette.requests import Request @@ -45,9 +45,8 @@ def _settings(self) -> KeycloakSettings | None: return live if live is not None else self._initial_settings async def resolve_user(self, request: Request) -> UserContext | None: - auth_header = request.headers.get("authorization", "") - if auth_header.startswith("Bearer "): - credential = auth_header[7:] + credential = parse_bearer(request.headers.get("authorization")) + if credential is not None: # An ``api_tokens`` credential is not a JWT: skip the parse and # leave it to the principal resolver that owns it. if is_api_token(credential): @@ -87,7 +86,7 @@ def get_public_paths(self) -> tuple[tuple[str, ...], tuple[str, ...]]: def is_bearer_request(self, request: Request | None) -> bool: if request is None: return False - return request.headers.get("authorization", "").startswith("Bearer ") + return parse_bearer(request.headers.get("authorization")) is not None async def _resolve_bearer(self, request: Request, token: str) -> UserContext | None: if self.jwks_cache is None: diff --git a/modules/keycloak/tests/test_keycloak_bearer_kind.py b/modules/keycloak/tests/test_keycloak_bearer_kind.py index 3e43abb87..f9bf2443f 100644 --- a/modules/keycloak/tests/test_keycloak_bearer_kind.py +++ b/modules/keycloak/tests/test_keycloak_bearer_kind.py @@ -6,6 +6,7 @@ from __future__ import annotations +import pytest from auth.contracts.tokens import AUTH_KIND_BEARER from keycloak.provider import KeycloakAuthProvider from keycloak.settings import KeycloakSettings @@ -18,7 +19,8 @@ async def validate_jwt(self, token: str) -> dict: return {"sub": "abc", "email": "kc@example.com", "preferred_username": "kc"} -async def test_jwt_bearer_principal_is_bearer_kind_and_unscoped(monkeypatch): +@pytest.mark.parametrize("scheme", [b"Bearer", b"bearer"]) # GH #533: case-insensitive +async def test_jwt_bearer_principal_is_bearer_kind_and_unscoped(monkeypatch, scheme): state = KeycloakState(settings=KeycloakSettings()) provider = KeycloakAuthProvider(state.settings, state=state) provider.jwks_cache = _Jwks() # type: ignore[assignment] @@ -32,10 +34,11 @@ async def upsert(request, claims) -> str: "type": "http", "method": "GET", "path": "/", - "headers": [(b"authorization", b"Bearer some.jwt.value")], + "headers": [(b"authorization", scheme + b" some.jwt.value")], "session": {}, } ) + assert provider.is_bearer_request(request) ctx = await provider.resolve_user(request) assert ctx is not None assert ctx.auth_kind == AUTH_KIND_BEARER diff --git a/modules/settings/settings/hydrate.py b/modules/settings/settings/hydrate.py index dfd733778..539616cb7 100644 --- a/modules/settings/settings/hydrate.py +++ b/modules/settings/settings/hydrate.py @@ -15,6 +15,7 @@ from typing import Union, get_args, get_origin from pydantic import BaseModel, ValidationError +from pydantic_core import ErrorDetails from pydantic_settings import ( BaseSettings, DotEnvSettingsSource, @@ -30,6 +31,11 @@ _SCALAR_TYPES = {bool: "bool", int: "int", float: "float"} +#: ``ctx`` key on a validation error listing the *other* fields a cross-field +#: rule depends on; raise it with ``PydanticCustomError(type, msg, {key: [...]})``. +#: Lenient hydration drops those when the reported field is not from the DB. +RELATED_FIELDS_CTX = "related_fields" + def value_type_for_field(cls: type[BaseSettings], field_name: str) -> str: """Return the ``value_type`` label for a field based on its annotation. @@ -167,18 +173,34 @@ def build_settings[T: BaseSettings]( try: return cls(**parsed) except ValidationError as exc: - bad = {str(e["loc"][0]) for e in exc.errors() if e["loc"]} & parsed.keys() - if not bad: + culprits = {name: err["type"] for err in exc.errors() for name in _culprits(err, parsed)} + if not culprits: raise - for name in sorted(bad): + for name in sorted(culprits): logger.error( "Ignoring stored value for %s.%s: it no longer validates (%s)", cls.__name__, name, - # error *types* only: a validator's message may echo the value (a secret) - ", ".join(str(e["type"]) for e in exc.errors() if e["loc"][:1] == (name,)), + # error *type* only: a validator's message may echo the value (a secret) + culprits[name], ) - return cls(**{k: v for k, v in parsed.items() if k not in bad}) + return cls(**{k: v for k, v in parsed.items() if k not in culprits}) + + +def _culprits(error: ErrorDetails, parsed: dict[str, object]) -> list[str]: + """The stored keys to drop so that ``error`` goes away. + + The field the error is reported on, when its value came from the DB. + Otherwise — that field is env-pinned or defaulted — the stored fields the + validator names under ``ctx[RELATED_FIELDS_CTX]``: a cross-field rule + reported on one field (so the editor shows it there) whose other half came + from the DB, which is the only half lenient hydration can drop (#548). + """ + loc = str(error["loc"][0]) if error["loc"] else None + if loc in parsed: + return [loc] + related = (error.get("ctx") or {}).get(RELATED_FIELDS_CTX, ()) + return [name for name in related if name in parsed] async def hydrate_settings[T: BaseSettings]( diff --git a/modules/settings/tests/test_hydrate_related_fields.py b/modules/settings/tests/test_hydrate_related_fields.py new file mode 100644 index 000000000..4c769e679 --- /dev/null +++ b/modules/settings/tests/test_hydrate_related_fields.py @@ -0,0 +1,53 @@ +"""Lenient hydration drops the stored half of a cross-field rule (#548).""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError, ValidationInfo, field_validator +from pydantic_core import PydanticCustomError +from pydantic_settings import BaseSettings, SettingsConfigDict +from settings.hydrate import RELATED_FIELDS_CTX, build_settings + + +class _Pair(BaseSettings): + model_config = SettingsConfigDict(env_prefix="SM_PAIRTEST_") + + low: int | None = None + high: int | None = None + other: int = 0 + + @field_validator("high") + @classmethod + def _ordered(cls, high: int | None, info: ValidationInfo) -> int | None: + low = info.data.get("low") + if high is not None and low is not None and low >= high: + raise PydanticCustomError( + "order", "high must exceed low", {RELATED_FIELDS_CTX: ["low"]} + ) + return high + + +_STORED = {"low": ("100", "int"), "other": ("7", "int")} + + +def test_env_pinned_reported_field_drops_the_stored_related_one(monkeypatch) -> None: + monkeypatch.setenv("SM_PAIRTEST_HIGH", "50") + + cfg = build_settings(_Pair, _STORED, lenient=True) + + assert (cfg.low, cfg.high, cfg.other) == (None, 50, 7) + + +def test_both_stored_drops_only_the_reported_field() -> None: + cfg = build_settings(_Pair, {**_STORED, "high": ("50", "int")}, lenient=True) + + assert (cfg.low, cfg.high, cfg.other) == (100, None, 7) + + +def test_strict_still_raises_on_the_reported_field(monkeypatch) -> None: + monkeypatch.setenv("SM_PAIRTEST_HIGH", "50") + + with pytest.raises(ValidationError) as exc: + build_settings(_Pair, _STORED) + + assert exc.value.errors()[0]["loc"] == ("high",) diff --git a/modules/users/tests-js/Register.test.tsx b/modules/users/tests-js/Register.test.tsx index 74888c29f..b4ba3d47c 100644 --- a/modules/users/tests-js/Register.test.tsx +++ b/modules/users/tests-js/Register.test.tsx @@ -1,5 +1,5 @@ import { configureI18n, keys, updateI18n } from '@simple-module-py/i18n'; -import { render, screen } from '@testing-library/react'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; import { act } from 'react'; import { afterEach, describe, expect, it, vi } from 'vitest'; @@ -10,17 +10,23 @@ vi.mock('@inertiajs/react', () => ({ import Register from '../users/pages/Register'; -const note = keys.users.register.bullet_close; -const verification = keys.users.register.bullet_verification; +const r = keys.users.register; +const note = r.bullet_close; +const verification = r.bullet_verification; +const messages = { + [note]: 'Registration may be closed', + [verification]: 'Verify your email', + [r.bullet_no_verification]: 'Sign in right away', + [r.success_title]: 'Check your inbox', + [r.success_ready_title]: 'Account created', +}; // A host locale override is delivered as a new catalog on an Inertia visit. -configureI18n({ - locale: 'en', - messages: { [note]: 'Registration may be closed', [verification]: 'Verify your email' }, -}); +configureI18n({ locale: 'en', messages }); afterEach(() => { - act(() => updateI18n({ locale: 'en', messages: { [note]: 'Registration may be closed' } })); + act(() => updateI18n({ locale: 'en', messages })); + vi.unstubAllGlobals(); }); describe('Register aside', () => { @@ -31,9 +37,41 @@ describe('Register aside', () => { }); it('omits the entire check row when the host overrides the note to empty', () => { - act(() => updateI18n({ locale: 'en', messages: { [note]: ' ' } })); + act(() => updateI18n({ locale: 'en', messages: { ...messages, [note]: ' ' } })); render(); expect(screen.getAllByRole('listitem')).toHaveLength(1); expect(screen.getByText('Verify your email')).toBeInTheDocument(); }); }); + +function submitValidForm() { + vi.stubGlobal( + 'fetch', + vi.fn(async () => new Response(null, { status: 201 })), + ); + fireEvent.change(document.getElementById('email') as HTMLElement, { + target: { value: 'new@example.com' }, + }); + const [password, confirm] = Array.from(document.querySelectorAll('input[type="password"]')); + fireEvent.change(password, { target: { value: 'Str0ng-Passphrase' } }); + fireEvent.change(confirm, { target: { value: 'Str0ng-Passphrase' } }); + fireEvent.submit(document.querySelector('form') as HTMLFormElement); +} + +describe('Register follows users.require_verification (#547)', () => { + it('promises a verification email when verification is required', async () => { + render(); + expect(screen.getByText('Verify your email')).toBeInTheDocument(); + submitValidForm(); + await waitFor(() => expect(screen.getByText('Check your inbox')).toBeInTheDocument()); + }); + + it('says the account is ready when verification is off', async () => { + render(); + expect(screen.queryByText('Verify your email')).toBeNull(); + expect(screen.getByText('Sign in right away')).toBeInTheDocument(); + submitValidForm(); + await waitFor(() => expect(screen.getByText('Account created')).toBeInTheDocument()); + expect(screen.queryByText('Check your inbox')).toBeNull(); + }); +}); diff --git a/modules/users/tests/_users_oauth_support.py b/modules/users/tests/_users_oauth_support.py new file mode 100644 index 000000000..d2e4f5638 --- /dev/null +++ b/modules/users/tests/_users_oauth_support.py @@ -0,0 +1,84 @@ +"""A fake OAuth provider (no network) to drive the ``/login`` + ``/callback`` flow.""" + +from __future__ import annotations + +import uuid +from urllib.parse import parse_qs, urlparse + +import httpx +from fastapi_users.password import PasswordHelper +from sqlalchemy import select +from users.models import User + + +class _FakeOAuthClient: + def __init__(self, account_id: str, account_email: str) -> None: + self._account_id = account_id + self._account_email = account_email + + async def get_authorization_url(self, redirect_uri: str, state: str) -> str: + return f"https://idp.example/authorize?state={state}" + + async def get_access_token(self, code: str, redirect_uri: str) -> dict: + return {"access_token": "fake-token", "expires_at": None, "refresh_token": None} + + async def get_id_email(self, access_token: str) -> tuple[str, str]: + return self._account_id, self._account_email + + +def install_fake_provider(app, account_id: str, account_email: str) -> None: + from users.oauth import OAuthProvider + + app.state.users.oauth_clients["microsoft"] = OAuthProvider( + "microsoft", "Microsoft", _FakeOAuthClient(account_id, account_email) + ) + + +def set_users_settings(app, **update) -> None: + """Change fields on the live users settings, as an admin edit would.""" + settings = app.state.users.settings + app.state.users.settings = settings.model_copy(update=update) + + +def set_allow_signup(app, allowed: bool) -> None: + """Flip ``allow_signup`` on the live settings, as an admin edit would.""" + set_users_settings(app, allow_signup=allowed) + + +async def find_user(app, email: str) -> User | None: + async with app.state.sm.db.session_factory() as session: + stmt = select(User).where(User.email == email) + return (await session.execute(stmt)).scalar_one_or_none() + + +async def seed_password_user(app, email: str, **fields) -> uuid.UUID: + """Insert an active password account for ``email``; return its id.""" + user_id = uuid.uuid4() + async with app.state.sm.db.session_factory() as session: + session.add( + User( + id=user_id, + email=email, + hashed_password=PasswordHelper().hash("SecurePass1!"), + is_active=True, + **fields, + ) + ) + await session.commit() + return user_id + + +async def oauth_callback(client: httpx.AsyncClient) -> httpx.Response: + """Drive /login then /callback; return the callback response.""" + login = await client.get("/api/users/auth/microsoft/login", follow_redirects=False) + assert login.status_code == 302 + state = parse_qs(urlparse(login.headers["location"]).query)["state"][0] + return await client.get( + f"/api/users/auth/microsoft/callback?code=abc&state={state}", + follow_redirects=False, + ) + + +async def run_oauth_login(client: httpx.AsyncClient) -> int: + """Drive /login then /callback; return the callback status code.""" + return (await oauth_callback(client)).status_code diff --git a/modules/users/tests/test_external_users.py b/modules/users/tests/test_external_users.py index 4fd16a375..17f20bf6e 100644 --- a/modules/users/tests/test_external_users.py +++ b/modules/users/tests/test_external_users.py @@ -11,9 +11,9 @@ from __future__ import annotations import uuid -from urllib.parse import parse_qs, urlparse import pytest +from _users_oauth_support import install_fake_provider, run_oauth_login, set_allow_signup from fastapi_users.password import PasswordHelper from sqlalchemy import select from users.models import Role, User @@ -21,46 +21,6 @@ _pw = PasswordHelper() -# --------------------------------------------------------------------------- -# Fake OAuth provider (no network) to drive the /login + /callback flow -# --------------------------------------------------------------------------- - - -class _FakeOAuthClient: - def __init__(self, account_id: str, account_email: str) -> None: - self._account_id = account_id - self._account_email = account_email - - async def get_authorization_url(self, redirect_uri: str, state: str) -> str: - return f"https://idp.example/authorize?state={state}" - - async def get_access_token(self, code: str, redirect_uri: str) -> dict: - return {"access_token": "fake-token", "expires_at": None, "refresh_token": None} - - async def get_id_email(self, access_token: str) -> tuple[str, str]: - return self._account_id, self._account_email - - -def _install_fake_provider(app, account_id: str, account_email: str) -> None: - from users.oauth import OAuthProvider - - app.state.users.oauth_clients["microsoft"] = OAuthProvider( - "microsoft", "Microsoft", _FakeOAuthClient(account_id, account_email) - ) - - -async def _run_oauth_login(client) -> int: - """Drive /login then /callback; return the callback status code.""" - login = await client.get("/api/users/auth/microsoft/login", follow_redirects=False) - assert login.status_code == 302 - state = parse_qs(urlparse(login.headers["location"]).query)["state"][0] - cb = await client.get( - f"/api/users/auth/microsoft/callback?code=abc&state={state}", - follow_redirects=False, - ) - return cb.status_code - - async def _get_user_by_email(db, email: str) -> User: return (await db.execute(select(User).where(User.email == email))).scalar_one() @@ -72,9 +32,10 @@ async def _get_user_by_email(db, email: str) -> User: @pytest.mark.anyio async def test_new_oauth_login_creates_external_user(users_app, anon_client, users_db): - _install_fake_provider(users_app, "ms-oid-1", "sso-new@example.com") + set_allow_signup(users_app, True) # creating an account via OAuth is a signup (#543) + install_fake_provider(users_app, "ms-oid-1", "sso-new@example.com") - assert await _run_oauth_login(anon_client) == 303 + assert await run_oauth_login(anon_client) == 303 user = await _get_user_by_email(users_db, "sso-new@example.com") assert user.is_external is True @@ -96,8 +57,8 @@ async def test_oauth_login_links_existing_password_user_stays_local( users_db.add(existing) await users_db.commit() - _install_fake_provider(users_app, "ms-oid-2", "local@example.com") - assert await _run_oauth_login(anon_client) == 303 + install_fake_provider(users_app, "ms-oid-2", "local@example.com") + assert await run_oauth_login(anon_client) == 303 user = await _get_user_by_email(users_db, "local@example.com") assert user.is_external is False diff --git a/modules/users/tests/test_token_api.py b/modules/users/tests/test_token_api.py index c32afb59d..7e0a2db80 100644 --- a/modules/users/tests/test_token_api.py +++ b/modules/users/tests/test_token_api.py @@ -101,7 +101,7 @@ async def test_valid_credentials_returns_token_pair(self, anon_client, users_db) ) assert resp.status_code == 200 body = resp.json() - assert body["token_type"] == "bearer" + assert body["token_type"] == "Bearer" assert body["access_token"] assert body["refresh_token"] assert body["expires_in"] > 0 diff --git a/modules/users/tests/test_user_manager.py b/modules/users/tests/test_user_manager.py index 47f0ac0e4..c95552f12 100644 --- a/modules/users/tests/test_user_manager.py +++ b/modules/users/tests/test_user_manager.py @@ -127,7 +127,7 @@ async def test_publishes_user_registered(self, manager, fake_user): # Pre-verified so the manager doesn't try to send a verify email. fake_user.is_verified = True - with patch.object(manager, "_attach_default_role", AsyncMock()): + with patch("users.manager.attach_default_role", AsyncMock()): await manager.on_after_register(fake_user, request) (event,) = bus.assert_published(UserRegistered) diff --git a/modules/users/tests/test_users_bearer_scheme.py b/modules/users/tests/test_users_bearer_scheme.py new file mode 100644 index 000000000..b8606a2e5 --- /dev/null +++ b/modules/users/tests/test_users_bearer_scheme.py @@ -0,0 +1,54 @@ +"""The ``Authorization`` scheme is case-insensitive (RFC 9110 §11.1, GH #533). + +``/auth/token`` used to answer ``token_type="bearer"``, and a client that built +its header from that value sent ``bearer `` — which the provider refused. +""" + +from __future__ import annotations + +import pytest +from starlette.requests import Request + +_TOKEN = "/api/users/auth/token" + + +def _request(app, header: str) -> Request: + return Request( + { + "type": "http", + "method": "GET", + "path": "/api/anything", + "headers": [(b"authorization", header.encode())], + "session": {}, + "app": app, + } + ) + + +@pytest.mark.anyio +@pytest.mark.parametrize("scheme", ["Bearer", "bearer", "BEARER", " bearer "]) +async def test_any_scheme_casing_authenticates(users_app, anon_client, scheme): + from users.provider import UsersAuthProvider + + resp = await anon_client.post( + _TOKEN, json={"email": "admin@example.com", "password": "AdminPass1!"} + ) + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["token_type"] == "Bearer" + + provider = UsersAuthProvider() + request = _request(users_app, f"{scheme} {body['access_token']}") + assert provider.is_bearer_request(request) + ctx = await provider.resolve_user(request) + assert ctx is not None + assert ctx.email == "admin@example.com" + + +@pytest.mark.anyio +async def test_another_scheme_is_not_a_bearer_request(users_app): + from users.provider import UsersAuthProvider + + request = _request(users_app, "Basic dXNlcjpwYXNz") + assert not UsersAuthProvider().is_bearer_request(request) + assert await UsersAuthProvider().resolve_user(request) is None diff --git a/modules/users/tests/test_users_oauth_signup_gate.py b/modules/users/tests/test_users_oauth_signup_gate.py new file mode 100644 index 000000000..aa45b51c0 --- /dev/null +++ b/modules/users/tests/test_users_oauth_signup_gate.py @@ -0,0 +1,63 @@ +"""OAuth sign-in creates an account only when ``allow_signup`` is on (GH #543). + +With signups closed, an operator who enables SSO for staff must not open a +signup to the provider's whole user base. Accounts that already exist — linked +by OAuth account or associated by email — still sign in. +""" + +from __future__ import annotations + +import pytest +from _users_oauth_support import ( + find_user, + install_fake_provider, + oauth_callback, + seed_password_user, + set_allow_signup, +) + + +@pytest.mark.anyio +async def test_signup_closed_refuses_a_new_identity(users_app, anon_client): + install_fake_provider(users_app, "ms-new", "stranger@example.com") + + resp = await oauth_callback(anon_client) + + assert resp.status_code == 403 + assert resp.json()["detail"] == "OAUTH_SIGNUP_DISABLED" + assert users_app.state.users.settings.cookie_name not in resp.cookies + assert await find_user(users_app, "stranger@example.com") is None + + +@pytest.mark.anyio +async def test_signup_open_creates_the_account(users_app, anon_client): + set_allow_signup(users_app, True) + install_fake_provider(users_app, "ms-new", "newcomer@example.com") + + assert (await oauth_callback(anon_client)).status_code == 303 + user = await find_user(users_app, "newcomer@example.com") + assert user is not None + assert user.is_external is True + + +@pytest.mark.anyio +async def test_signup_closed_still_associates_an_existing_email(users_app, anon_client): + await seed_password_user(users_app, "staff@example.com", is_verified=True) + install_fake_provider(users_app, "ms-staff", "staff@example.com") + + assert (await oauth_callback(anon_client)).status_code == 303 + + +@pytest.mark.anyio +async def test_signup_closed_still_signs_in_a_linked_oauth_account(users_app, anon_client): + """Matched by the linked OAuth account even when the provider's email moved.""" + set_allow_signup(users_app, True) + install_fake_provider(users_app, "ms-linked", "before@example.com") + assert (await oauth_callback(anon_client)).status_code == 303 + + set_allow_signup(users_app, False) + anon_client.cookies.clear() + install_fake_provider(users_app, "ms-linked", "after@example.com") + + assert (await oauth_callback(anon_client)).status_code == 303 + assert await find_user(users_app, "after@example.com") is None diff --git a/modules/users/tests/test_users_oauth_unverified_link.py b/modules/users/tests/test_users_oauth_unverified_link.py new file mode 100644 index 000000000..1dd11024d --- /dev/null +++ b/modules/users/tests/test_users_oauth_unverified_link.py @@ -0,0 +1,85 @@ +"""OAuth never links by email to an unverified password account (GH #547). + +With ``require_verification`` off anyone can register ``victim@corp.com`` with +a password and sign in. If the real owner later signs in with SSO, associating +by email would hand them — and the attacker — one shared account. +""" + +from __future__ import annotations + +import uuid +from datetime import UTC, datetime + +import pytest +from _users_oauth_support import ( + install_fake_provider, + oauth_callback, + seed_password_user, + set_allow_signup, +) +from fastapi_users.password import PasswordHelper +from sqlalchemy import select +from users.models import OAuthAccount, User + +_EMAIL = "victim@example.com" + + +async def _links(app, user_id: uuid.UUID) -> list[OAuthAccount]: + async with app.state.sm.db.session_factory() as session: + stmt = select(OAuthAccount).where(OAuthAccount.user_id == user_id) + return list((await session.execute(stmt)).scalars()) + + +@pytest.mark.anyio +@pytest.mark.parametrize("allow_signup", [True, False]) +async def test_unverified_password_account_is_not_linked(users_app, anon_client, allow_signup): + set_allow_signup(users_app, allow_signup) + user_id = await seed_password_user(users_app, _EMAIL, is_verified=False) + install_fake_provider(users_app, "ms-victim", _EMAIL) + + resp = await oauth_callback(anon_client) + + assert resp.status_code == 403 + assert resp.json()["detail"] == "OAUTH_ACCOUNT_NOT_VERIFIED" + assert users_app.state.users.settings.cookie_name not in resp.cookies + assert await _links(users_app, user_id) == [] + + +@pytest.mark.anyio +async def test_verified_account_links(users_app, anon_client): + user_id = await seed_password_user(users_app, _EMAIL, is_verified=True) + install_fake_provider(users_app, "ms-owner", _EMAIL) + + assert (await oauth_callback(anon_client)).status_code == 303 + assert [a.account_id for a in await _links(users_app, user_id)] == ["ms-owner"] + + +@pytest.mark.anyio +async def test_pending_invite_links(users_app, anon_client): + """An invite's password is random and unknown, so nobody else holds the account.""" + user_id = await seed_password_user( + users_app, _EMAIL, is_verified=False, invited_at=datetime.now(UTC) + ) + install_fake_provider(users_app, "ms-invitee", _EMAIL) + + assert (await oauth_callback(anon_client)).status_code == 303 + assert len(await _links(users_app, user_id)) == 1 + + +@pytest.mark.anyio +async def test_already_linked_account_signs_in(users_app, anon_client): + """Found by its OAuth link, not by email — verification is not asked for.""" + set_allow_signup(users_app, True) + install_fake_provider(users_app, "ms-linked", "linked@example.com") + assert (await oauth_callback(anon_client)).status_code == 303 + async with users_app.state.sm.db.session_factory() as session: + user = ( + await session.execute(select(User).where(User.email == "linked@example.com")) + ).scalar_one() + user.is_verified = False + user.hashed_password = PasswordHelper().hash("SecurePass1!") + user.is_external = False + await session.commit() + + anon_client.cookies.clear() + assert (await oauth_callback(anon_client)).status_code == 303 diff --git a/modules/users/tests/test_users_require_verification.py b/modules/users/tests/test_users_require_verification.py new file mode 100644 index 000000000..322b95235 --- /dev/null +++ b/modules/users/tests/test_users_require_verification.py @@ -0,0 +1,104 @@ +"""``users.require_verification`` decides whether a signup can sign in at once (GH #547). + +The setting was never read: every self-registered account was sent a +verification email and refused at login until verified, so an install with no +SMTP had signups nobody could use. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock + +import pytest +from _users_oauth_support import find_user, set_users_settings + +_EMAIL = "signup@example.com" +_PASSWORD = "SecurePass1!" +_INERTIA_HEADERS = {"X-Inertia": "true", "X-Inertia-Version": "1.0"} + + +def _configure(app, *, require_verification: bool) -> AsyncMock: + set_users_settings(app, require_verification=require_verification) + users = app.state.users + users.mailer.send_verification = AsyncMock() + return users.mailer.send_verification + + +async def _register_then_login(client): + reg = await client.post( + "/api/users/auth/register", json={"email": _EMAIL, "password": _PASSWORD} + ) + assert reg.status_code == 201, reg.text + return await client.post( + "/api/users/auth/login", data={"username": _EMAIL, "password": _PASSWORD} + ) + + +@pytest.mark.anyio +async def test_required_sends_the_email_and_refuses_login(users_app_signup, anon_client_signup): + sent = _configure(users_app_signup, require_verification=True) + + login = await _register_then_login(anon_client_signup) + + assert login.status_code == 400 + assert login.json()["detail"] == "LOGIN_USER_NOT_VERIFIED" + sent.assert_awaited_once() + assert (await find_user(users_app_signup, _EMAIL)).is_verified is False + + +@pytest.mark.anyio +async def test_not_required_allows_login_without_verifying(users_app_signup, anon_client_signup): + sent = _configure(users_app_signup, require_verification=False) + + login = await _register_then_login(anon_client_signup) + + assert login.status_code == 204, login.text + sent.assert_not_awaited() + # Nobody proved the address, so the account must not claim it was: an SSO + # sign-in for this email would otherwise link to it. + assert (await find_user(users_app_signup, _EMAIL)).is_verified is False + + +@pytest.mark.anyio +async def test_turning_it_off_lets_earlier_signups_in(users_app_signup, anon_client_signup): + _configure(users_app_signup, require_verification=True) + assert (await _register_then_login(anon_client_signup)).status_code == 400 + + _configure(users_app_signup, require_verification=False) + login = await anon_client_signup.post( + "/api/users/auth/login", data={"username": _EMAIL, "password": _PASSWORD} + ) + + assert login.status_code == 204, login.text + + +@pytest.mark.anyio +@pytest.mark.parametrize("required", [True, False]) +async def test_register_page_receives_the_setting(users_app_signup, anon_client_signup, required): + _configure(users_app_signup, require_verification=required) + + resp = await anon_client_signup.get("/users/register", headers=_INERTIA_HEADERS) + + assert resp.status_code == 200, resp.text + assert resp.json()["props"]["require_verification"] is required + + +@pytest.mark.anyio +@pytest.mark.parametrize(("required", "status"), [(True, 400), (False, 200)]) +async def test_bearer_token_login_follows_the_setting( + users_app_signup, anon_client_signup, required, status +): + """``/auth/token`` is a login too: it must not mint tokens for an unverified account.""" + _configure(users_app_signup, require_verification=required) + reg = await anon_client_signup.post( + "/api/users/auth/register", json={"email": _EMAIL, "password": _PASSWORD} + ) + assert reg.status_code == 201, reg.text + + resp = await anon_client_signup.post( + "/api/users/auth/token", json={"email": _EMAIL, "password": _PASSWORD} + ) + + assert resp.status_code == status, resp.text + if required: + assert resp.json()["detail"] == "LOGIN_USER_NOT_VERIFIED" diff --git a/modules/users/users/auth_local/api.py b/modules/users/users/auth_local/api.py index 0b9671d2d..ede5b77a7 100644 --- a/modules/users/users/auth_local/api.py +++ b/modules/users/users/auth_local/api.py @@ -125,8 +125,10 @@ async def login( limiter.record_failure(key) raise HTTPException(status_code=400, detail="LOGIN_BAD_CREDENTIALS") - if not user.is_verified: - # Match fastapi-users' own behavior when requires_verification=True + if not user.is_verified and request.app.state.users.settings.require_verification: + # Match fastapi-users' own behavior when requires_verification=True. + # With the setting off nobody is asked to verify (#547) — including + # accounts that signed up while it was on. raise HTTPException(status_code=400, detail="LOGIN_USER_NOT_VERIFIED") limiter.reset(key) diff --git a/modules/users/users/auth_local/token_api.py b/modules/users/users/auth_local/token_api.py index 9666f264d..61633630a 100644 --- a/modules/users/users/auth_local/token_api.py +++ b/modules/users/users/auth_local/token_api.py @@ -39,7 +39,7 @@ class TokenResponse(SQLModel): access_token: str refresh_token: str - token_type: str = "bearer" + token_type: str = "Bearer" expires_in: int @@ -90,6 +90,10 @@ async def token_login( await db.flush() settings = request.app.state.users.settings + if not user.is_verified and settings.require_verification: + # Same rule as the session login: a self-registered account that never + # proved its address must not mint bearer tokens either (#547). + raise HTTPException(status_code=400, detail="LOGIN_USER_NOT_VERIFIED") return await _create_token_pair(db, user.id, settings, user.session_version) @@ -189,6 +193,6 @@ async def _create_token_pair( return TokenResponse( access_token=access_token.token, refresh_token=str(refresh.token), - token_type="bearer", + token_type="Bearer", expires_in=settings.bearer_token_lifetime_seconds, ) diff --git a/modules/users/users/auth_local/views.py b/modules/users/users/auth_local/views.py index 4be0dbda6..5ee11c1f3 100644 --- a/modules/users/users/auth_local/views.py +++ b/modules/users/users/auth_local/views.py @@ -130,9 +130,14 @@ async def logout(request: Request) -> RedirectResponse: @router.get("/register", response_model=None) async def register_page(request: Request, inertia: InertiaDep) -> InertiaResponse: - if not request.app.state.users.settings.allow_signup: + users_settings = request.app.state.users.settings + if not users_settings.allow_signup: raise HTTPException(status_code=404) - return await inertia.render(_PAGE_REGISTER, {}) + # Drives the "verify your email" bullet and the success screen, so the + # page never promises an email that ``on_after_register`` won't send. + return await inertia.render( + _PAGE_REGISTER, {"require_verification": users_settings.require_verification} + ) @router.get("/forgot-password", response_model=None) diff --git a/modules/users/users/locales/en.json b/modules/users/users/locales/en.json index 23facf03b..65c9ee888 100644 --- a/modules/users/users/locales/en.json +++ b/modules/users/users/locales/en.json @@ -315,6 +315,8 @@ "have_account": "Already have an account?", "success_title": "Check your inbox", "success_body": "We've sent a verification link to {email}.", + "success_ready_title": "Account created", + "success_ready_body": "Your account for {email} is ready. Sign in to continue.", "error_exists": "An account with this email already exists.", "error_password_length": "Password must be at least 8 characters, not counting spaces.", "error_password_contains_email": "Password cannot contain your email.", @@ -324,6 +326,7 @@ "error_failed": "Registration failed. Please try again.", "intro": "Open registration is on for this instance. New accounts get the standard user role; an admin can grant more.", "bullet_verification": "Email verification is required before first sign-in", + "bullet_no_verification": "You can sign in as soon as your account is created", "bullet_close": "Registration may be closed by an administrator", "name_placeholder": "Optional" }, diff --git a/modules/users/users/manager.py b/modules/users/users/manager.py index 5911b83b4..944f547ea 100644 --- a/modules/users/users/manager.py +++ b/modules/users/users/manager.py @@ -12,20 +12,19 @@ from fastapi_users import BaseUserManager, UUIDIDMixin, exceptions from fastapi_users.jwt import generate_jwt from simple_module_hosting.session import stamp_session_expiry -from sqlalchemy import select from users.constants import ( OAUTH_REGISTRATION_REQUEST_FLAG, SESSION_DEMO_KEY, SESSION_USER_ID_KEY, SESSION_VERSION_KEY, - USER_ROLE_NAME, ) from users.contracts.events import UserRegistered from users.db_adapter import UserDatabaseWithRoles, get_user_db from users.exceptions import ExternalUserNoPasswordError from users.mailer import Mailer -from users.models import Role, User, UserRole +from users.models import User +from users.registration import attach_default_role if TYPE_CHECKING: from users.settings import UsersSettings @@ -54,6 +53,7 @@ def __init__( # Read per request: ``get_user_manager`` builds a manager per request # from the live settings, so an admin toggling signup takes effect. self.allow_signup = settings.allow_signup + self.require_verification = settings.require_verification # ── Password policy ────────────────────────────────────── @@ -125,47 +125,25 @@ async def on_after_register(self, user: User, request: Request | None = None) -> user.hashed_password = None user.is_external = True await self.user_db.update(user, {"hashed_password": None, "is_external": True}) - # OAuth provisioning is not gated by ``allow_signup`` the way /register - # is, so with signups closed an operator who only wanted SSO for staff - # would otherwise hand the ``user`` role (file read/write, API tokens) - # to anyone holding an account at the provider. Signups closed means + # The OAuth callback refuses to create accounts with signups closed + # (#543); this is the second line: should one be provisioned anyway, + # it must not get the ``user`` role (file read/write, API tokens) — + # a provider's whole user base would hold it. Signups closed means # an OAuth-provisioned account waits for an admin to grant a role. if request is not None and (self.allow_signup or not oauth_provisioned): - await self._attach_default_role(user) + await attach_default_role(self.user_db.session, user) await self._publish_user_registered(user, request) - if not user.is_verified: - # Kicks off on_after_request_verify, which sends the email - await self.request_verify(user, request) - - async def _attach_default_role(self, user: User) -> None: - """Give a self-registered (or OAuth-provisioned) user the ``user`` role. - - Without it a fresh signup holds no permissions at all — not even - ``api_tokens.use`` — until an admin assigns a role by hand (GH #488). - Only request-bound registrations reach here: the admin create and - invite flows call ``create()`` with no request and assign exactly the - roles the admin picked, which may deliberately be none. - - Looked up, never created: migration ``b5018b617142`` seeds it on a - fresh install, so a missing one means an admin deleted it, and a - public signup must not undo that. It also keeps concurrent signups - from racing to insert it. - """ - session = self.user_db.session - role = ( - await session.execute(select(Role).where(Role.name == USER_ROLE_NAME)) - ).scalar_one_or_none() - if role is None: - logger.warning( - "No '%s' role exists; %s registered with no role", USER_ROLE_NAME, user.id - ) + if user.is_verified: + return + if request is not None and not oauth_provisioned and not self.require_verification: + # Self-registration with verification switched off (#547): no + # email is sent, and password login stops asking for it. The + # account stays *unverified* — nobody has proven the address, so + # an SSO sign-in for it must not link here (see oauth/api.py). + # Request-less creates (admin, invite) keep their verify-by-link flow. return - session.add(UserRole(user_id=user.id, role_id=role.id)) - # Committed, not just flushed: fastapi-users already committed the - # user row in ``create()``, so a failure after this point (event - # publish, the verification email) would otherwise roll the role back - # and leave a committed account with no role that cannot re-register. - await session.commit() + # Kicks off on_after_request_verify, which sends the email + await self.request_verify(user, request) async def _publish_user_registered(self, user: User, request: Request | None) -> None: """Emit ``UserRegistered`` on the app-wide event bus. diff --git a/modules/users/users/oauth/api.py b/modules/users/users/oauth/api.py index cc475c987..41c90bd2b 100644 --- a/modules/users/users/oauth/api.py +++ b/modules/users/users/oauth/api.py @@ -31,6 +31,7 @@ if TYPE_CHECKING: from users.manager import UserManager + from users.models import User from users.oauth.providers import OAuthProvider _SESSION_STATE_KEY_FMT = "oauth_state:{provider}" @@ -47,6 +48,42 @@ def _resolve_provider(request: Request, provider: str) -> OAuthProvider: return found +async def _existing_account( + user_manager: UserManager, provider: str, account_id: str, account_email: str +) -> tuple[User | None, bool]: + """The account ``oauth_callback`` would sign in, and whether it is already linked. + + Mirrors its two lookups: the linked OAuth account first, then — because + the callback passes ``associate_by_email=True`` — an account by email. + ``(None, False)`` means the callback would create a new account. + """ + try: + return await user_manager.get_by_oauth_account(provider, account_id), True + except fu_exceptions.UserNotExists: + pass + try: + return await user_manager.get_by_email(account_email), False + except fu_exceptions.UserNotExists: + return None, False + + +def _unproven_password_account(user: User) -> bool: + """A self-registered password account nobody has proven owns its email. + + Linking an SSO identity to it by email would hand the provider's user an + account someone else may hold the password to — register the victim's + address first, wait for them to sign in with SSO, and share their account + (GH #547). External (SSO-provisioned) accounts have no password, and a + pending invite's password is random and unknown to anyone, so both link. + """ + return ( + not user.is_verified + and not user.is_external + and user.hashed_password is not None + and user.invited_at is None + ) + + def register_oauth_routes(api_router: APIRouter) -> None: """Mount the provider-agnostic OAuth dispatcher under ``/auth``.""" router = APIRouter(prefix="/auth", tags=["users-auth"]) @@ -89,6 +126,22 @@ async def callback( if account_email is None: raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="OAUTH_NO_EMAIL") + existing, linked = await _existing_account( + user_manager, provider, account_id, account_email + ) + # An OAuth sign-in is a signup when nobody holds this identity yet, and + # with ``allow_signup`` off it must not be one: an operator enabling + # SSO for staff would otherwise open signup to the provider's whole + # user base (GH #543). Existing accounts still link and sign in. + if existing is None and not request.app.state.users.settings.allow_signup: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, detail="OAUTH_SIGNUP_DISABLED" + ) + if existing is not None and not linked and _unproven_password_account(existing): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, detail="OAUTH_ACCOUNT_NOT_VERIFIED" + ) + # Mark this request so the manager's on_after_register provisions a # *new* OAuth user as external (null password). Ignored when the login # links to an existing account (on_after_register won't fire). diff --git a/modules/users/users/pages/Register.tsx b/modules/users/users/pages/Register.tsx index b94a0b918..e067263da 100644 --- a/modules/users/users/pages/Register.tsx +++ b/modules/users/users/pages/Register.tsx @@ -4,7 +4,7 @@ import { Button } from '@simple-module-py/ui/components/ui/button'; import { Input } from '@simple-module-py/ui/components/ui/input'; import { AuthCardShell } from '@simple-module-py/ui/layouts/AuthCardShell'; import { LOGIN_PATH } from '@simple-module-py/ui/lib/auth-routes'; -import { MailCheck } from 'lucide-react'; +import { MailCheck, UserCheck } from 'lucide-react'; import { useState } from 'react'; import { AuthField } from '../auth_local/components/AuthField'; import { AuthIntro } from '../auth_local/components/AuthIntro'; @@ -19,7 +19,12 @@ import { type RegisterErrorCode, } from '../auth_local/registerErrors'; -function Register() { +interface RegisterProps { + /** `users.require_verification`: whether a new account must confirm its email. */ + require_verification?: boolean; +} + +function Register({ require_verification = true }: RegisterProps) { const { t } = useT(); const [email, setEmail] = useState(''); const [fullName, setFullName] = useState(''); @@ -29,6 +34,20 @@ function Register() { const [error, setError] = useState(null); const [success, setSuccess] = useState(false); const [loading, setLoading] = useState(false); + // Everything that differs between "check your inbox" and "you can sign in now". + const copy = require_verification + ? { + icon: MailCheck, + title: keys.users.register.success_title, + body: keys.users.register.success_body, + bullet: keys.users.register.bullet_verification, + } + : { + icon: UserCheck, + title: keys.users.register.success_ready_title, + body: keys.users.register.success_ready_body, + bullet: keys.users.register.bullet_no_verification, + }; const message = (code: RegisterErrorCode) => { const r = keys.users.register; @@ -93,10 +112,10 @@ function Register() {