From 6d1c2c2c2ec3c1d23732923158d23f648b934c4b Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 07:04:31 +0200 Subject: [PATCH 01/19] fix(core): don't cache a failed permission source as empty (#517) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../core/simple_module_core/permissions.py | 17 ++++++++++++++--- framework/core/tests/test_permission_sources.py | 16 ++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/framework/core/simple_module_core/permissions.py b/framework/core/simple_module_core/permissions.py index baaea6f21..004304c29 100644 --- a/framework/core/simple_module_core/permissions.py +++ b/framework/core/simple_module_core/permissions.py @@ -106,6 +106,9 @@ 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]]] = {} + # Set when a source raised during the current recompute, so the derived + # ``all_permissions`` snapshot is not cached with that source missing. + self._source_failed = False def _invalidate(self) -> None: self._all_permissions_cache = None @@ -120,7 +123,8 @@ 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. + and contributes nothing for that read; the empty result is not cached, + so the next read retries it. """ self._sources[name] = provider self._source_cache.pop(name, None) @@ -147,7 +151,10 @@ def _source_output(self, name: str) -> tuple[list[str], dict[str, str]]: labels[key] = label except Exception: logger.exception("Permission source %r failed; contributing nothing", name) - keys, labels = [], {} + # Not cached: a transient failure must not hide the source until + # someone invalidates it. + self._source_failed = True + return [], {} out = (sorted(set(keys)), labels) self._source_cache[name] = out return out @@ -177,10 +184,14 @@ def add(self, permission: str) -> None: def all_permissions(self) -> list[str]: """All registered permission strings, sorted.""" if self._all_permissions_cache is None: + self._source_failed = False perms: set[str] = set() for group in self.groups: perms.update(group.permissions) - self._all_permissions_cache = sorted(perms) + result = sorted(perms) + if self._source_failed: + return result + self._all_permissions_cache = result 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..d7047c613 100644 --- a/framework/core/tests/test_permission_sources.py +++ b/framework/core/tests/test_permission_sources.py @@ -50,3 +50,19 @@ 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"]) + + +def test_failed_source_is_not_cached_as_empty() -> None: + calls = {"n": 0} + + def flaky() -> list[str]: + calls["n"] += 1 + if calls["n"] == 1: + raise RuntimeError("db not ready") + return ["flaky.read"] + + reg = PermissionRegistry() + reg.add_source("flaky", flaky) + assert reg.all_permissions == [] + assert reg.has("flaky.read") + assert "flaky.read" in reg.all_permissions From 0037d02b310936ad87da88a9a6db679358ddd7fb Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 07:05:13 +0200 Subject: [PATCH 02/19] fix(background_tasks): don't share the sync engine across a fork (#532) Record the creating PID; rebuild with dispose(close=False) on mismatch and drop inherited engines on worker_process_init. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../background_tasks/sync_db.py | 36 ++++++-- .../tests/test_sync_db_fork.py | 87 +++++++++++++++++++ 2 files changed, 116 insertions(+), 7 deletions(-) create mode 100644 modules/background_tasks/tests/test_sync_db_fork.py diff --git a/modules/background_tasks/background_tasks/sync_db.py b/modules/background_tasks/background_tasks/sync_db.py index a8cdf5039..304537132 100644 --- a/modules/background_tasks/background_tasks/sync_db.py +++ b/modules/background_tasks/background_tasks/sync_db.py @@ -21,6 +21,7 @@ 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 +from celery.signals import worker_process_init from sqlalchemy import create_engine from sqlalchemy.engine import Engine from sqlalchemy.orm import Session, sessionmaker @@ -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 @@ -72,14 +77,24 @@ def set_database_url( install's own scoped reads never see it. """ global _url_override, _engine, _session_factory, _tenant_strict, _default_tenant + global _engine_pid 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() + _engine_pid = None + + +def _drop_engine() -> None: + """Dispose the cached engine, never touching sockets inherited via fork.""" + global _engine, _session_factory 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 @@ -127,9 +142,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 +159,20 @@ 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, _engine_pid + _drop_engine() + _engine_pid = None _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_sync_db_fork.py b/modules/background_tasks/tests/test_sync_db_fork.py new file mode 100644 index 000000000..2cba39f18 --- /dev/null +++ b/modules/background_tasks/tests/test_sync_db_fork.py @@ -0,0 +1,87 @@ +"""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 From a0dfbaf7f1c93961ce8fd57ae169531329f2cf09 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 07:05:41 +0200 Subject: [PATCH 03/19] fix(background_tasks): require task_soft_time_limit < task_time_limit (#548) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../background_tasks/settings.py | 16 ++++++++++ .../tests/test_background_tasks_settings.py | 30 +++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/modules/background_tasks/background_tasks/settings.py b/modules/background_tasks/background_tasks/settings.py index 220c36689..fda35a578 100644 --- a/modules/background_tasks/background_tasks/settings.py +++ b/modules/background_tasks/background_tasks/settings.py @@ -239,6 +239,22 @@ 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. + """ + soft = info.data.get("task_soft_time_limit") + if hard is not None and soft is not None and soft >= hard: + raise ValueError( + f"task_time_limit ({hard}) must be greater than task_soft_time_limit ({soft}); " + "otherwise the soft limit can never fire" + ) + return hard + @field_validator("outbox_handler_timeout_seconds") @classmethod def _handler_timeout_fits_the_lease( diff --git a/modules/background_tasks/tests/test_background_tasks_settings.py b/modules/background_tasks/tests/test_background_tasks_settings.py index d488fa410..42457567a 100644 --- a/modules/background_tasks/tests/test_background_tasks_settings.py +++ b/modules/background_tasks/tests/test_background_tasks_settings.py @@ -25,3 +25,33 @@ 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 From 2d1d5978075d2c891c4907cc9c35d220e3b8901c Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 07:06:00 +0200 Subject: [PATCH 04/19] fix(auth): parse the Bearer scheme case-insensitively (#533) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add auth.contracts.tokens.parse_bearer(), the one reader of an Authorization: Bearer header: the scheme is matched case-insensitively (RFC 9110 §11.1) and surrounding whitespace is tolerated. The users and keycloak providers and the api_tokens resolver all use it, so a client that builds its header from /auth/token's token_type authenticates. /auth/token now returns token_type="Bearer". Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- modules/api_tokens/api_tokens/resolver.py | 9 ++-- .../tests/test_api_tokens_resolver.py | 11 ++++ modules/auth/auth/contracts/tokens.py | 17 ++++++ modules/auth/tests/test_auth_parse_bearer.py | 21 ++++++++ modules/keycloak/keycloak/provider.py | 9 ++-- .../tests/test_keycloak_bearer_kind.py | 7 ++- modules/users/tests/test_token_api.py | 2 +- .../users/tests/test_users_bearer_scheme.py | 54 +++++++++++++++++++ modules/users/users/auth_local/token_api.py | 4 +- modules/users/users/provider.py | 9 ++-- 10 files changed, 122 insertions(+), 21 deletions(-) create mode 100644 modules/auth/tests/test_auth_parse_bearer.py create mode 100644 modules/users/tests/test_users_bearer_scheme.py 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/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/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_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/users/auth_local/token_api.py b/modules/users/users/auth_local/token_api.py index 9666f264d..1a8643e7d 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 @@ -189,6 +189,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/provider.py b/modules/users/users/provider.py index 61df970d4..e75a01f58 100644 --- a/modules/users/users/provider.py +++ b/modules/users/users/provider.py @@ -13,7 +13,7 @@ from typing import Any from auth.contracts.schemas import UserContext -from auth.contracts.tokens import is_api_token +from auth.contracts.tokens import is_api_token, parse_bearer from simple_module_hosting.session import ( SESSION_COOKIE_MAX_AGE, SESSION_EXPIRES_AT_KEY, @@ -89,9 +89,8 @@ async def resolve_user(self, request: Request) -> UserContext | None: # already had. Pinned by ``test_a_bad_bearer_is_not_rescued_by_a_valid # _session``, which asserted the opposite for as long as it went # uncollected. - 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 ours: answering None at once # saves a pointless ``users_access_token`` lookup and leaves the # token to the principal resolver that owns it. Still no fallback @@ -243,7 +242,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, scope, token: str) -> UserContext | None: """Resolve an ``Authorization: Bearer`` token to its user. From 679df2352195ae5527f87309373ab1c8c383dae9 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 07:07:53 +0200 Subject: [PATCH 05/19] fix(users): OAuth sign-in creates accounts only when allow_signup is on (#543) The OAuth callback now checks, before find-or-create, whether the identity already belongs to an account (linked OAuth account, then email association). With allow_signup off and no such account it answers 403 OAUTH_SIGNUP_DISABLED, the same error-code shape as the callback's other refusals; existing accounts still link and sign in. The fake OAuth provider used by test_external_users moves to a shared _users_oauth_support helper. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- modules/users/tests/_users_oauth_support.py | 52 +++++++++++++ modules/users/tests/test_external_users.py | 51 ++---------- .../tests/test_users_oauth_signup_gate.py | 78 +++++++++++++++++++ modules/users/users/manager.py | 8 +- modules/users/users/oauth/api.py | 31 ++++++++ 5 files changed, 171 insertions(+), 49 deletions(-) create mode 100644 modules/users/tests/_users_oauth_support.py create mode 100644 modules/users/tests/test_users_oauth_signup_gate.py diff --git a/modules/users/tests/_users_oauth_support.py b/modules/users/tests/_users_oauth_support.py new file mode 100644 index 000000000..3621b04f6 --- /dev/null +++ b/modules/users/tests/_users_oauth_support.py @@ -0,0 +1,52 @@ +"""A fake OAuth provider (no network) to drive the ``/login`` + ``/callback`` flow.""" + +from __future__ import annotations + +from urllib.parse import parse_qs, urlparse + +import httpx + + +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_allow_signup(app, allowed: bool) -> None: + """Flip ``allow_signup`` on the live settings, as an admin edit would.""" + settings = app.state.users.settings + app.state.users.settings = settings.model_copy(update={"allow_signup": allowed}) + + +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_users_oauth_signup_gate.py b/modules/users/tests/test_users_oauth_signup_gate.py new file mode 100644 index 000000000..406c4fbef --- /dev/null +++ b/modules/users/tests/test_users_oauth_signup_gate.py @@ -0,0 +1,78 @@ +"""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 uuid + +import pytest +from _users_oauth_support import install_fake_provider, oauth_callback, set_allow_signup +from fastapi_users.password import PasswordHelper +from sqlalchemy import select +from users.models import User + + +async def _find(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() + + +@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(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(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): + async with users_app.state.sm.db.session_factory() as session: + session.add( + User( + id=uuid.uuid4(), + email="staff@example.com", + hashed_password=PasswordHelper().hash("SecurePass1!"), + is_active=True, + is_verified=True, + ) + ) + await session.commit() + 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(users_app, "after@example.com") is None diff --git a/modules/users/users/manager.py b/modules/users/users/manager.py index 5911b83b4..20fd5c6f6 100644 --- a/modules/users/users/manager.py +++ b/modules/users/users/manager.py @@ -125,10 +125,10 @@ 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) diff --git a/modules/users/users/oauth/api.py b/modules/users/users/oauth/api.py index cc475c987..ac406d5ba 100644 --- a/modules/users/users/oauth/api.py +++ b/modules/users/users/oauth/api.py @@ -47,6 +47,26 @@ def _resolve_provider(request: Request, provider: str) -> OAuthProvider: return found +async def _is_known_account( + user_manager: UserManager, provider: str, account_id: str, account_email: str +) -> bool: + """Whether ``oauth_callback`` would sign in an existing account, not create one. + + Mirrors its two lookups: the linked OAuth account first, then — because + the callback passes ``associate_by_email=True`` — an account by email. + """ + try: + await user_manager.get_by_oauth_account(provider, account_id) + return True + except fu_exceptions.UserNotExists: + pass + try: + await user_manager.get_by_email(account_email) + return True + except fu_exceptions.UserNotExists: + return False + + 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 +109,17 @@ async def callback( if account_email is None: raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="OAUTH_NO_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 not request.app.state.users.settings.allow_signup and not await _is_known_account( + user_manager, provider, account_id, account_email + ): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, detail="OAUTH_SIGNUP_DISABLED" + ) + # 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). From ab983baebdf4c7365ca355e707a37dce8ad113d9 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 07:10:37 +0200 Subject: [PATCH 06/19] fix(users): honour require_verification on self-registration (#547) With users.require_verification off, on_after_register marks a self-registered account verified and sends no verification email, so the new user can sign in at once. Request-less creates (admin, invite) and OAuth provisioning are unchanged; with the setting on, behaviour is as before. The register view passes require_verification to the page, whose aside bullet and success screen now follow it (new users.register keys, i18n keys regenerated). attach_default_role moves to users/registration.py to keep manager.py under the 300-line cap. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- modules/users/tests-js/Register.test.tsx | 56 +++++++++++--- modules/users/tests/test_user_manager.py | 2 +- .../tests/test_users_require_verification.py | 76 +++++++++++++++++++ modules/users/users/auth_local/views.py | 9 ++- modules/users/users/locales/en.json | 3 + modules/users/users/manager.py | 50 ++++-------- modules/users/users/pages/Register.tsx | 27 +++++-- modules/users/users/registration.py | 41 ++++++++++ packages/i18n/src/generated-resources.ts | 3 + packages/i18n/src/keys.generated.ts | 3 + 10 files changed, 216 insertions(+), 54 deletions(-) create mode 100644 modules/users/tests/test_users_require_verification.py create mode 100644 modules/users/users/registration.py 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/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_require_verification.py b/modules/users/tests/test_users_require_verification.py new file mode 100644 index 000000000..fdb93776d --- /dev/null +++ b/modules/users/tests/test_users_require_verification.py @@ -0,0 +1,76 @@ +"""``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 sqlalchemy import select +from users.models import User + +_EMAIL = "signup@example.com" +_PASSWORD = "SecurePass1!" +_INERTIA_HEADERS = {"X-Inertia": "true", "X-Inertia-Version": "1.0"} + + +def _configure(app, *, require_verification: bool) -> AsyncMock: + users = app.state.users + users.settings = users.settings.model_copy( + update={"require_verification": require_verification} + ) + 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} + ) + + +async def _stored(app) -> User: + async with app.state.sm.db.session_factory() as session: + return (await session.execute(select(User).where(User.email == _EMAIL))).scalar_one() + + +@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 _stored(users_app_signup)).is_verified is False + + +@pytest.mark.anyio +async def test_not_required_verifies_and_allows_login(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() + assert (await _stored(users_app_signup)).is_verified is True + + +@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 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 20fd5c6f6..90697684d 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 ────────────────────────────────────── @@ -131,41 +131,19 @@ async def on_after_register(self, user: User, request: Request | None = None) -> # 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): the + # account is usable at once and no email is sent. Request-less + # creates (admin, invite) keep their verify-by-link flow. + user.is_verified = True + await self.user_db.update(user, {"is_verified": True}) 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/pages/Register.tsx b/modules/users/users/pages/Register.tsx index b94a0b918..de52b7be1 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(''); @@ -93,10 +98,18 @@ function Register() {