From 6282ef17748428bf00e77ec73181e3d81520ca76 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 22:04:18 +0200 Subject: [PATCH 1/7] background_tasks: serialize eager outbox dispatches per transport; tenant teardown test covers both module orders (#655) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 3 + .../background_tasks/outbox_transport.py | 7 ++- .../test_bg_outbox_eager_serialization.py | 56 +++++++++++++++++++ .../test_tenants_teardown_integration.py | 11 ++++ 4 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 modules/background_tasks/tests/test_bg_outbox_eager_serialization.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 844ffbeea..48cd1e83a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,9 @@ All notable changes to this project are documented in this file. The format is b across pages, and recover concurrent duplicate share grants without a 500 (#640). - Share UTC timestamp helpers across sharing, tenants, background tasks, API tokens and users (#640). +- Background tasks: serialize eager outbox dispatches so chained deliveries do not + overlap on shared in-memory SQLite connections or trip another handler's commit + guard during tenant teardown (#655). - Users: publish deletion, disabling and new role assignments transactionally through the durable outbox (inline cleanup without background tasks). Deletion purges tenant memberships, shares to the user and direct permission grants; tenants left without diff --git a/modules/background_tasks/background_tasks/outbox_transport.py b/modules/background_tasks/background_tasks/outbox_transport.py index aaecd5dde..c8392c1a3 100644 --- a/modules/background_tasks/background_tasks/outbox_transport.py +++ b/modules/background_tasks/background_tasks/outbox_transport.py @@ -132,6 +132,8 @@ def __init__( # Eager in-process deliveries still running (kept so they aren't # garbage-collected mid-flight, and so ``drain`` can await them). self._tasks: set[asyncio.Task[Any]] = set() + # Chained eager publishes must not share a StaticPool transaction (#655). + self._dispatch_lock = asyncio.Lock() def policy(self) -> OutboxPolicy: s = self._services.settings @@ -155,8 +157,9 @@ def schedule(self, db: AsyncSession | Session, ids: Sequence[uuid.UUID]) -> None id_strs = [str(i) for i in id_list] after_commit(db, [lambda: self._send(id_strs)]) - def _dispatch(self, ids: list[uuid.UUID]) -> Any: - return dispatch(self._db_state.session_factory, self._bus, self.policy(), ids=ids) + async def _dispatch(self, ids: list[uuid.UUID]) -> Any: + async with self._dispatch_lock: + return await dispatch(self._db_state.session_factory, self._bus, self.policy(), ids=ids) def _dispatch_soon(self, ids: list[uuid.UUID]) -> None: """Eager delivery from a session's ``after_commit`` event. diff --git a/modules/background_tasks/tests/test_bg_outbox_eager_serialization.py b/modules/background_tasks/tests/test_bg_outbox_eager_serialization.py new file mode 100644 index 000000000..20729fbc9 --- /dev/null +++ b/modules/background_tasks/tests/test_bg_outbox_eager_serialization.py @@ -0,0 +1,56 @@ +"""Chained eager deliveries must not overlap on in-memory SQLite (#655).""" + +import asyncio +from dataclasses import dataclass + +from background_tasks.constants import OutboxStatus +from background_tasks.models import TaskExecution +from simple_module_core import DurableContext +from simple_module_core.events import Event + + +@dataclass +class EagerParent(Event): + pass + + +@dataclass +class EagerChild(Event): + pass + + +async def test_chained_eager_delivery_waits_for_parent_dispatch(app, outbox_rows, effect_names): + app.state.background_tasks.settings.task_always_eager = True + bus = app.state.sm.event_bus + active = False + + async def publish_child(event: EagerParent, ctx: DurableContext) -> None: + await bus.publish_durable(EagerChild(), ctx.db) + + async def effect(event: Event, ctx: DurableContext) -> None: + nonlocal active + assert not active, "eager dispatchers overlapped" + active = True + try: + await ctx.db.connection() + # intentional-sleep: let the child dispatcher attempt its claim while guarded + await asyncio.sleep(0.05) + ctx.db.add(TaskExecution(task_name=f"effect:{type(event).__name__}")) + await ctx.db.flush() + finally: + active = False + + bus.subscribe_durable(EagerParent, publish_child, name="eager-publish-child") + bus.subscribe_durable(EagerParent, effect, name="eager-parent-effect") + bus.subscribe_durable(EagerChild, effect, name="eager-child-effect") + async with app.state.sm.db.session_factory() as db: + await bus.publish_durable(EagerParent(), db) + await db.commit() + + await bus.durable_transport.drain() + + rows = await outbox_rows() + assert len(rows) == 3 + assert all(row.status == OutboxStatus.DELIVERED for row in rows) + assert all(row.attempts == 1 for row in rows) + assert await effect_names() == ["effect:EagerChild", "effect:EagerParent"] diff --git a/modules/tenants/tests/test_tenants_teardown_integration.py b/modules/tenants/tests/test_tenants_teardown_integration.py index 8649df4b3..17c66de86 100644 --- a/modules/tenants/tests/test_tenants_teardown_integration.py +++ b/modules/tenants/tests/test_tenants_teardown_integration.py @@ -1,7 +1,9 @@ """A real tenant deletion delivers every installed owner's durable teardown.""" +from dataclasses import replace from datetime import UTC, datetime, timedelta +import pytest from api_tokens.models import ApiToken from audit_log.models import AuditEntry from background_tasks.outbox_claim import OutboxPolicy @@ -9,12 +11,21 @@ from feature_flags.models import FeatureFlagOverride from file_storage.backends.filesystem import FilesystemBackend from file_storage.models import StoredFile +from sharing.module import SharingModule from simple_module_db import all_tenants, tenant_context from simple_module_db.stored_settings import SCOPE_TENANT, Setting from sqlmodel import select from tenants.service import TenantService +@pytest.fixture(autouse=True, params=[False, True], ids=["default-order", "sharing-after-users"]) +def _sharing_load_order(request, monkeypatch): + if request.param: + monkeypatch.setattr( + SharingModule, "meta", replace(SharingModule.meta, optional_depends_on=["Users"]) + ) + + async def test_tenants_delete_removes_owned_data_but_retains_audit( app, tenant_client, client, tmp_path, monkeypatch ): From ad84d9707067ca95f7b06dc1aa7e2178089824a1 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 22:05:41 +0200 Subject: [PATCH 2/7] api_tokens: purge a deleted user's personal tokens on UserDeleted (#657) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 2 + modules/api_tokens/api_tokens/module.py | 3 + modules/api_tokens/api_tokens/user_cleanup.py | 19 ++++++ .../tests/test_api_tokens_user_cleanup.py | 61 +++++++++++++++++++ 4 files changed, 85 insertions(+) create mode 100644 modules/api_tokens/api_tokens/user_cleanup.py create mode 100644 modules/api_tokens/tests/test_api_tokens_user_cleanup.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 48cd1e83a..9eec991f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,8 @@ All notable changes to this project are documented in this file. The format is b - Background tasks: serialize eager outbox dispatches so chained deliveries do not overlap on shared in-memory SQLite connections or trip another handler's commit guard during tenant teardown (#655). +- Api tokens: durably purge a deleted user's personal tokens across all tenants + on `UserDeleted`, preserving service keys and other users' tokens (#657). - Users: publish deletion, disabling and new role assignments transactionally through the durable outbox (inline cleanup without background tasks). Deletion purges tenant memberships, shares to the user and direct permission grants; tenants left without diff --git a/modules/api_tokens/api_tokens/module.py b/modules/api_tokens/api_tokens/module.py index 32a09bbe6..581c8532f 100644 --- a/modules/api_tokens/api_tokens/module.py +++ b/modules/api_tokens/api_tokens/module.py @@ -38,10 +38,13 @@ class ApiTokensModule(ModuleBase): def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) -> None: from simple_module_core.tenant_events import TenantDeleted + from simple_module_core.user_events import UserDeleted from api_tokens.tenant_cleanup import purge_tenant_data + from api_tokens.user_cleanup import purge_user_tokens bus.subscribe_durable(TenantDeleted, purge_tenant_data) + bus.subscribe_durable(UserDeleted, purge_user_tokens) def register_settings(self, app: FastAPI) -> None: from api_tokens.services import ApiTokensServices diff --git a/modules/api_tokens/api_tokens/user_cleanup.py b/modules/api_tokens/api_tokens/user_cleanup.py new file mode 100644 index 000000000..82d696eb7 --- /dev/null +++ b/modules/api_tokens/api_tokens/user_cleanup.py @@ -0,0 +1,19 @@ +"""Durable cleanup of a deleted user's personal tokens.""" + +from simple_module_core.durable_events import DurableContext +from simple_module_core.user_events import UserDeleted +from simple_module_db.tenancy import all_tenants +from sqlmodel import col, delete + +from api_tokens.constants import TokenKind +from api_tokens.models import ApiToken + + +async def purge_user_tokens(event: UserDeleted, ctx: DurableContext) -> None: + with all_tenants(): + await ctx.db.execute( + delete(ApiToken).where( + col(ApiToken.kind) == TokenKind.PERSONAL, + col(ApiToken.owner_user_id) == event.user_id, + ) + ) diff --git a/modules/api_tokens/tests/test_api_tokens_user_cleanup.py b/modules/api_tokens/tests/test_api_tokens_user_cleanup.py new file mode 100644 index 000000000..89510147e --- /dev/null +++ b/modules/api_tokens/tests/test_api_tokens_user_cleanup.py @@ -0,0 +1,61 @@ +"""User deletion purges personal tokens across tenants transactionally (#657).""" + +import uuid +from datetime import UTC, datetime, timedelta + +import pytest +from api_tokens.constants import TokenKind +from api_tokens.models import ApiToken +from api_tokens.module import ApiTokensModule +from simple_module_core.events import EventBus +from simple_module_core.user_events import UserDeleted +from simple_module_db import all_tenants, tenant_context +from simple_module_db.transaction import finalize_session, rollback_session +from sqlmodel import select + + +@pytest.mark.parametrize("rollback", [False, True]) +async def test_api_tokens_user_deleted_cleanup_is_scoped_idempotent_and_transactional( + db_session, rollback +): + deleted_id, other_id = uuid.uuid4(), uuid.uuid4() + kept_ids = set() + for tenant_id in ("first", "second"): + with tenant_context(tenant_id): + for kind, owner in ( + (TokenKind.PERSONAL, deleted_id), + (TokenKind.PERSONAL, other_id), + (TokenKind.SERVICE, deleted_id), + ): + token = ApiToken( + kind=kind, + owner_user_id=owner, + name="token", + prefix="test", + token_hash=uuid.uuid4().hex, + expires_at=datetime.now(UTC) + timedelta(days=1), + ) + db_session.add(token) + if kind == TokenKind.SERVICE or owner == other_id: + kept_ids.add(token.id) + await db_session.flush() + await finalize_session(db_session) + bus = EventBus() + # Workers register without an app; inline delivery uses the same durable subscription. + ApiTokensModule().register_event_handlers(bus) + with tenant_context("first"): + for _ in range(2): + await bus.publish_durable_or_inline(UserDeleted(deleted_id), db_session) + with all_tenants(): + remaining = (await db_session.execute(select(ApiToken.id))).scalars().all() + assert set(remaining) == kept_ids + if rollback: + await rollback_session(db_session) + else: + await finalize_session(db_session) + with all_tenants(): + remaining = (await db_session.execute(select(ApiToken.id))).scalars().all() + if rollback: + assert len(remaining) == 6 + else: + assert set(remaining) == kept_ids From 8ac6514a3044edc0c43784728190fb4f3c49bb3c Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 22:09:17 +0200 Subject: [PATCH 3/7] users: refresh-token families get an absolute lifetime (refresh_token_family_max_age_days, default 90); pruning removes expired families (#656) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 2 + docs/modules/users.md | 1 + .../test_users_refresh_family_max_age.py | 178 ++++++++++++++++++ .../users/tests/test_users_refresh_reuse.py | 2 +- .../users/tests/test_users_token_pruning.py | 4 +- modules/users/users/auth_local/token_api.py | 27 ++- modules/users/users/db_adapter.py | 27 ++- modules/users/users/settings.py | 4 + modules/users/users/token_storage.py | 29 ++- 9 files changed, 261 insertions(+), 13 deletions(-) create mode 100644 modules/users/tests/test_users_refresh_family_max_age.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9eec991f8..6bc990f35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,8 @@ All notable changes to this project are documented in this file. The format is b guard during tenant teardown (#655). - Api tokens: durably purge a deleted user's personal tokens across all tenants on `UserDeleted`, preserving service keys and other users' tokens (#657). +- Users: cap refresh-token families at a DB-configurable 90 days, refusing refreshes + and revoking/pruning aged families while preserving reuse evidence for live families (#656). - Users: publish deletion, disabling and new role assignments transactionally through the durable outbox (inline cleanup without background tasks). Deletion purges tenant memberships, shares to the user and direct permission grants; tenants left without diff --git a/docs/modules/users.md b/docs/modules/users.md index 4f04a1645..8b12226c5 100644 --- a/docs/modules/users.md +++ b/docs/modules/users.md @@ -196,6 +196,7 @@ Everything else is DB-backed (initial values are pydantic defaults; edit under U | `verification_token_lifetime_seconds` | `604_800` (7 days) | | `bearer_token_lifetime_seconds` | `900` (15 min) | | `refresh_token_lifetime_seconds` | `2_592_000` (30 days) | +| `refresh_token_family_max_age_days` | `90` (≥ 1) — absolute age from the first token; requires a new login after expiry | | `cookie_name` | `"sm_auth"` | | `cookie_max_age_seconds` | `1_209_600` (14 days) | | `cookie_secure` | `True` (flipped to `False` in dev at startup) | diff --git a/modules/users/tests/test_users_refresh_family_max_age.py b/modules/users/tests/test_users_refresh_family_max_age.py new file mode 100644 index 000000000..e169e15a1 --- /dev/null +++ b/modules/users/tests/test_users_refresh_family_max_age.py @@ -0,0 +1,178 @@ +"""Absolute refresh-family age bounds retained rotation history (#656).""" + +import asyncio +import uuid +from datetime import UTC, datetime, timedelta +from types import SimpleNamespace + +import pytest +import sqlalchemy as sa +from pydantic import ValidationError +from sqlalchemy.orm import Session +from starlette.responses import JSONResponse +from users.auth_local.token_api import RefreshRequest, TokenResponse, token_refresh +from users.db_adapter import get_access_token_db +from users.models import Base, RefreshToken, User, UserAccessToken +from users.settings import UsersSettings +from users.token_storage import hash_token, prune_tokens + + +@pytest.fixture +def family_db(): + engine = sa.create_engine("sqlite://") + Base.metadata.create_all(engine) + try: + with Session(engine) as session: + owner = uuid.uuid4() + session.add(User(id=owner, email="family@example.com")) + session.flush() + yield session, owner + finally: + engine.dispose() + + +class _AsyncSession: + def __init__(self, session): + self.session = session + + def add(self, row): + self.session.add(row) + + async def flush(self): + self.session.flush() + + async def commit(self): + self.session.commit() + + async def refresh(self, row): + self.session.refresh(row) + + async def scalar(self, statement): + return self.session.scalar(statement) + + async def execute(self, statement): + return self.session.execute(statement) + + +def _family(session, owner, now, age_days, *, children=1): + family = uuid.uuid4() + session.add( + RefreshToken( + token=str(uuid.uuid4()), + user_id=owner, + family_id=family, + created_at=now - timedelta(days=age_days), + expires_at=now - timedelta(days=1), + revoked_at=now - timedelta(days=1), + ) + ) + secrets = [str(uuid.uuid4()) for _ in range(children)] + for secret in secrets: + session.add( + RefreshToken( + token=hash_token(secret), + user_id=owner, + family_id=family, + created_at=now, + expires_at=now + timedelta(days=30), + ) + ) + session.flush() + return secrets + + +@pytest.mark.parametrize("age_days", [7, 8, 91]) +@pytest.mark.parametrize("expired", [False, True]) +def test_users_refresh_refuses_old_family_and_revokes_all_live_children( + family_db, expired, age_days +): + session, owner = family_db + now = datetime.now(UTC) + secrets = _family(session, owner, now, age_days, children=2) + if expired: + session.get(RefreshToken, hash_token(secrets[0])).expires_at = now - timedelta(seconds=1) + session.flush() + settings = ( + UsersSettings() if age_days == 91 else UsersSettings(refresh_token_family_max_age_days=7) + ) + request = _request(settings) + response = asyncio.run( + token_refresh(RefreshRequest(refresh_token=secrets[0]), request, _AsyncSession(session)) + ) + assert isinstance(response, JSONResponse) + assert response.status_code == 401 + session.expire_all() + assert all(row.revoked_at is not None for row in session.scalars(sa.select(RefreshToken))) + assert session.scalar(sa.select(sa.func.count()).select_from(UserAccessToken)) == 0 + + +def test_users_prune_revokes_old_family_before_bounded_deletion(family_db): + session, owner = family_db + now = datetime.now(UTC) + _family(session, owner, now, 8, children=101) + recent = _family(session, owner, now, 6) + other = uuid.uuid4() + session.add(User(id=other, email="other@example.com")) + session.flush() + other_old = _family(session, other, now, 8) + + for _ in range(2): + asyncio.run(prune_tokens(_AsyncSession(session), owner, now, family_max_age_days=7)) + session.expire_all() + # Even if the oldest ancestor was in the first batch, no child becomes live again. + remaining = list( + session.scalars( + sa.select(RefreshToken).where( + RefreshToken.user_id == owner, RefreshToken.token != hash_token(recent[0]) + ) + ) + ) + assert all(row.revoked_at is not None for row in remaining) + assert session.scalar(sa.select(sa.func.count()).select_from(RefreshToken)) == 4 + assert session.get(RefreshToken, hash_token(recent[0])).revoked_at is None + assert session.get(RefreshToken, hash_token(other_old[0])).revoked_at is None + + +def test_users_family_max_age_rejects_zero(): + with pytest.raises(ValidationError): + UsersSettings(refresh_token_family_max_age_days=0) + + +def _request(settings): + return SimpleNamespace( + app=SimpleNamespace( + state=SimpleNamespace(users=SimpleNamespace(settings=settings, demo_user_ids=set())) + ) + ) + + +def test_users_recent_family_rotates_and_keeps_expired_ancestor_reuse_evidence(family_db): + session, owner = family_db + now = datetime.now(UTC) + secrets = _family(session, owner, now, 6) + ancestor = session.scalar(sa.select(RefreshToken).where(RefreshToken.revoked_at.is_not(None))) + ancestor_secret = str(uuid.uuid4()) + ancestor.token = hash_token(ancestor_secret) + session.flush() + request = _request(UsersSettings(refresh_token_family_max_age_days=7)) + db = _AsyncSession(session) + rotated = asyncio.run(token_refresh(RefreshRequest(refresh_token=secrets[0]), request, db)) + assert isinstance(rotated, TokenResponse) + assert session.get(RefreshToken, hash_token(ancestor_secret)) is not None + for secret in (ancestor_secret, rotated.refresh_token): + refused = asyncio.run(token_refresh(RefreshRequest(refresh_token=secret), request, db)) + assert refused.status_code == 401 + + +def test_users_cookie_cleanup_uses_configured_family_max_age(family_db): + session, owner = family_db + now = datetime.now(UTC) + _family(session, owner, now, 8) + request = _request(UsersSettings(refresh_token_family_max_age_days=7)) + + async def login(): + async for database in get_access_token_db(request, _AsyncSession(session)): + await database.create({"token": "cookie", "user_id": owner}) + + asyncio.run(login()) + assert session.scalar(sa.select(sa.func.count()).select_from(RefreshToken)) == 0 diff --git a/modules/users/tests/test_users_refresh_reuse.py b/modules/users/tests/test_users_refresh_reuse.py index 961a9ab2d..6c08227ef 100644 --- a/modules/users/tests/test_users_refresh_reuse.py +++ b/modules/users/tests/test_users_refresh_reuse.py @@ -56,7 +56,7 @@ async def synchronized_read(session, statement, *args, **kwargs): ): frozen = result.freeze() row = frozen().scalar_one_or_none() - if row is not None and row.revoked_at is None: + if isinstance(row, RefreshToken) and row.revoked_at is None: # Force both stale reads before either request can burn the token. await readers.wait() return frozen() diff --git a/modules/users/tests/test_users_token_pruning.py b/modules/users/tests/test_users_token_pruning.py index b9df0563f..62b1c842b 100644 --- a/modules/users/tests/test_users_token_pruning.py +++ b/modules/users/tests/test_users_token_pruning.py @@ -105,7 +105,9 @@ def test_users_token_pair_persists_only_digests_with_sync_sqlite(): _SyncSession(session), user_id, SimpleNamespace( - bearer_token_lifetime_seconds=900, refresh_token_lifetime_seconds=3600 + bearer_token_lifetime_seconds=900, + refresh_token_lifetime_seconds=3600, + refresh_token_family_max_age_days=90, ), ) ) diff --git a/modules/users/users/auth_local/token_api.py b/modules/users/users/auth_local/token_api.py index 235a7f22d..45fececaa 100644 --- a/modules/users/users/auth_local/token_api.py +++ b/modules/users/users/auth_local/token_api.py @@ -150,13 +150,29 @@ async def token_refresh( .returning(RefreshToken) ) rt = (await db.execute(stmt)).scalar_one_or_none() + rotated = rt is not None if rt is None: - reused = await db.scalar(select(RefreshToken).where(col(RefreshToken.token) == digest)) - if reused is not None and reused.revoked_at is not None: + rt = await db.scalar(select(RefreshToken).where(col(RefreshToken.token) == digest)) + if rt is None: + return JSONResponse({"detail": "Invalid or expired refresh token"}, status_code=401) + + settings = request.app.state.users.settings + family_start = ( + await db.execute( + select(func.min(RefreshToken.created_at)).where( + col(RefreshToken.family_id) == rt.family_id, + ) + ) + ).scalar_one() + family_expired = family_start <= now - timedelta( + days=settings.refresh_token_family_max_age_days + ) + if not rotated or family_expired: + if rt.revoked_at is not None or family_expired: await db.execute( update(RefreshToken) .where( - col(RefreshToken.family_id) == reused.family_id, + col(RefreshToken.family_id) == rt.family_id, col(RefreshToken.revoked_at).is_(None), ) .values(revoked_at=now) @@ -177,7 +193,6 @@ async def token_refresh( await rollback_session(db) raise HTTPException(status_code=403, detail=DEMO_READ_ONLY_DETAIL) - settings = request.app.state.users.settings return await _create_token_pair(db, user.id, settings, user.session_version, rt.family_id) @@ -239,7 +254,9 @@ async def _create_token_pair( ) db.add(refresh) await db.flush() - await prune_tokens(db, user_id, now) + await prune_tokens( + db, user_id, now, family_max_age_days=settings.refresh_token_family_max_age_days + ) return TokenResponse( access_token=access_secret, diff --git a/modules/users/users/db_adapter.py b/modules/users/users/db_adapter.py index a41d97a1a..b0919ce38 100644 --- a/modules/users/users/db_adapter.py +++ b/modules/users/users/db_adapter.py @@ -5,7 +5,7 @@ from collections.abc import AsyncGenerator from datetime import UTC, datetime -from fastapi import Depends +from fastapi import Depends, Request from fastapi_users_db_sqlalchemy import SQLAlchemyUserDatabase from fastapi_users_db_sqlalchemy.access_token import SQLAlchemyAccessTokenDatabase from simple_module_db.deps import get_db @@ -14,6 +14,7 @@ from sqlalchemy.orm import noload, selectinload from users.models import OAuthAccount, User, UserAccessToken +from users.settings import DEFAULT_REFRESH_TOKEN_FAMILY_MAX_AGE_DAYS from users.token_storage import prune_tokens @@ -63,12 +64,32 @@ async def get_user_db( class PruningAccessTokenDatabase(SQLAlchemyAccessTokenDatabase[UserAccessToken]): """Clean up spent credentials when a cookie login mints an access token.""" + def __init__( + self, + session, + access_token_table, + *, + family_max_age_days: int = DEFAULT_REFRESH_TOKEN_FAMILY_MAX_AGE_DAYS, + ): + super().__init__(session, access_token_table) + self.family_max_age_days = family_max_age_days + async def create(self, create_dict): - await prune_tokens(self.session, create_dict["user_id"], datetime.now(UTC)) + await prune_tokens( + self.session, + create_dict["user_id"], + datetime.now(UTC), + family_max_age_days=self.family_max_age_days, + ) return await super().create(create_dict) async def get_access_token_db( + request: Request, session: AsyncSession = Depends(get_db), ) -> AsyncGenerator[SQLAlchemyAccessTokenDatabase[UserAccessToken], None]: - yield PruningAccessTokenDatabase(session, UserAccessToken) + yield PruningAccessTokenDatabase( + session, + UserAccessToken, + family_max_age_days=request.app.state.users.settings.refresh_token_family_max_age_days, + ) diff --git a/modules/users/users/settings.py b/modules/users/users/settings.py index 83bb1d5ae..f53cbae6c 100644 --- a/modules/users/users/settings.py +++ b/modules/users/users/settings.py @@ -29,6 +29,8 @@ _PLACEHOLDER_RESET_SECRET = "dev-reset-token-secret-change-me" _PLACEHOLDER_VERIFY_SECRET = "dev-verify-token-secret-change-me" +DEFAULT_REFRESH_TOKEN_FAMILY_MAX_AGE_DAYS = 90 + DEFAULT_LOGIN_REDIRECT_URL = "/dashboard/" # Groups the demo fields together in the module-settings editor. @@ -84,6 +86,8 @@ def _non_empty_redirect(cls, value: str) -> str: bearer_token_lifetime_seconds: int = 60 * 15 # 15 minutes refresh_token_lifetime_seconds: int = 60 * 60 * 24 * 30 # 30 days + refresh_token_family_max_age_days: int = Field(DEFAULT_REFRESH_TOKEN_FAMILY_MAX_AGE_DAYS, ge=1) + # Cookie (fastapi-users AuthenticationBackend) cookie_name: str = "sm_auth" cookie_max_age_seconds: int = 60 * 60 * 24 * 14 # 14 days diff --git a/modules/users/users/token_storage.py b/modules/users/users/token_storage.py index e2afeaecd..817e4bed7 100644 --- a/modules/users/users/token_storage.py +++ b/modules/users/users/token_storage.py @@ -2,13 +2,14 @@ import hashlib import uuid -from datetime import datetime +from datetime import datetime, timedelta -from sqlalchemy import delete, or_, select +from sqlalchemy import delete, func, or_, select, update from sqlalchemy.ext.asyncio import AsyncSession from sqlmodel import col from users.models import RefreshToken, UserAccessToken +from users.settings import DEFAULT_REFRESH_TOKEN_FAMILY_MAX_AGE_DAYS _PRUNE_BATCH_SIZE = 100 @@ -26,8 +27,30 @@ async def revoke_access_token(db: AsyncSession, secret: str | None) -> None: ) -async def prune_tokens(db: AsyncSession, user_id: uuid.UUID, now: datetime) -> None: +async def prune_tokens( + db: AsyncSession, + user_id: uuid.UUID, + now: datetime, + *, + family_max_age_days: int = DEFAULT_REFRESH_TOKEN_FAMILY_MAX_AGE_DAYS, +) -> None: """Prune this user's spent rows, preserving active families' reuse evidence.""" + expired_families = ( + select(RefreshToken.family_id) + .where(col(RefreshToken.user_id) == user_id) + .group_by(RefreshToken.family_id) + .having(func.min(RefreshToken.created_at) <= now - timedelta(days=family_max_age_days)) + ) + # Revoke before bounded deletion: removing the oldest row must not renew the family. + await db.execute( + update(RefreshToken) + .where( + col(RefreshToken.user_id) == user_id, + col(RefreshToken.family_id).in_(expired_families), + col(RefreshToken.revoked_at).is_(None), + ) + .values(revoked_at=now) + ) active_families = select(RefreshToken.family_id).where( col(RefreshToken.user_id) == user_id, col(RefreshToken.revoked_at).is_(None), From 3c2ea7458eeb6c11ff309ec9ecb469fb74eb85ee Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 22:10:14 +0200 Subject: [PATCH 4/7] users admin: refusal toasts show the server's reason, translated (#653) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 2 + .../users_admin_error_details.test.tsx | 112 ++++++++++++++++++ .../admin/components/useUserRowActions.ts | 14 ++- modules/users/users/locales/de.json | 12 +- modules/users/users/locales/en.json | 12 +- .../pages/Users/components/DangerZone.tsx | 6 +- .../pages/Users/components/useEditUserForm.ts | 24 ++-- .../pages/Users/components/useUserActions.ts | 10 +- modules/users/users/pages/failureMessage.ts | 25 ++++ packages/i18n/src/generated-resources.ts | 8 ++ packages/i18n/src/keys.generated.ts | 10 ++ 11 files changed, 214 insertions(+), 21 deletions(-) create mode 100644 modules/users/tests-js/users_admin_error_details.test.tsx diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bc990f35..74504fc0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,8 @@ All notable changes to this project are documented in this file. The format is b on `UserDeleted`, preserving service keys and other users' tokens (#657). - Users: cap refresh-token families at a DB-configurable 90 days, refusing refreshes and revoking/pruning aged families while preserving reuse evidence for live families (#656). +- Users admin actions show translated server refusal details instead of generic errors; + unknown details retain the existing fallback (#653). - Users: publish deletion, disabling and new role assignments transactionally through the durable outbox (inline cleanup without background tasks). Deletion purges tenant memberships, shares to the user and direct permission grants; tenants left without diff --git a/modules/users/tests-js/users_admin_error_details.test.tsx b/modules/users/tests-js/users_admin_error_details.test.tsx new file mode 100644 index 000000000..968d0e9f2 --- /dev/null +++ b/modules/users/tests-js/users_admin_error_details.test.tsx @@ -0,0 +1,112 @@ +import { configureI18n, t } from '@simple-module-py/i18n'; +import { act, renderHook, waitFor } from '@testing-library/react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('@inertiajs/react', () => ({ router: { reload: vi.fn(), on: vi.fn(() => vi.fn()) } })); +vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +import { toast } from 'sonner'; +import { useUserRowActions } from '../users/admin/components/useUserRowActions'; +import de from '../users/locales/de.json'; +import en from '../users/locales/en.json'; +import { adminFailureMessage } from '../users/pages/failureMessage'; +import { useEditUserForm } from '../users/pages/Users/components/useEditUserForm'; +import { useUserActions } from '../users/pages/Users/components/useUserActions'; +import { json, stubFetch } from './users_fixtures'; + +afterEach(() => { + vi.unstubAllGlobals(); + vi.clearAllMocks(); +}); + +describe.each(['en', 'de'])('users admin errors (%s)', (locale) => { + function configure() { + const messages: Record = {}; + function flatten(value: object, prefix = 'users') { + for (const [key, child] of Object.entries(value)) { + if (typeof child === 'string') messages[`${prefix}.${key}`] = child; + else flatten(child, `${prefix}.${key}`); + } + } + flatten(locale === 'en' ? en : de); + configureI18n({ locale, messages }); + } + + const privileged = () => + locale === 'en' + ? 'You cannot manage a more privileged account.' + : 'Sie können kein Konto mit höheren Berechtigungen verwalten.'; + + it.each(['row', 'edit'])('translates the server refusal in the %s action toast', async (page) => { + configure(); + stubFetch(() => json(403, { detail: 'You cannot manage a more privileged account.' })); + const { result } = renderHook(() => { + const row = useUserRowActions(); + const edit = useUserActions('target', true, false, null); + return { row, edit }; + }); + await act(async () => { + if (page === 'row') await result.current.row.setActive('target', false); + else result.current.edit.disable(); + }); + await waitFor(() => expect(toast.error).toHaveBeenCalledWith(privileged())); + expect(result.current.edit.isActive).toBe(true); + }); + + it.each([false, true])( + 'explains a refused role grant after details saved: %s', + async (detailsDirty) => { + configure(); + stubFetch((request) => + request.method === 'PATCH' + ? json(200, {}) + : json(403, { detail: 'You cannot assign roles you do not hold.' }), + ); + const user = { id: 'target', email: 'a@example.com', full_name: null, roles: [] }; + const { result } = renderHook(() => useEditUserForm(user)); + act(() => { + result.current.toggleRole('Admin'); + if (detailsDirty) result.current.setForm((prev) => ({ ...prev, fullName: 'New name' })); + }); + await act(async () => result.current.save()); + const reason = + locale === 'en' + ? 'You cannot assign roles you do not hold.' + : 'Sie können keine Rollen zuweisen, die Sie selbst nicht innehaben.'; + expect(result.current.error).toBe( + detailsDirty ? `${(locale === 'en' ? en : de).edit.error_partial} ${reason}` : reason, + ); + expect(result.current.dirtyCount).toBe(1); + }, + ); + + it.each(Object.entries(en.admin_errors))('translates the known refusal %s', (key, detail) => { + configure(); + expect(adminFailureMessage({ status: 400, detail }, 'fallback', 'network', t)).toBe( + (locale === 'en' ? en : de).admin_errors[key as keyof typeof en.admin_errors], + ); + }); + + it.each([null, '', 'constructor', 'unexpected server internals', [{ msg: 'invalid' }]])( + 'keeps the fallback for unknown detail %s', + async (detail) => { + configure(); + stubFetch(() => json(400, { detail })); + const { result } = renderHook(() => useUserRowActions()); + await act(async () => result.current.setActive('target', false)); + expect(toast.error).toHaveBeenCalledWith( + (locale === 'en' ? en : de).user_row.toast_status_failed, + ); + }, + ); + + it('keeps the network failure message', async () => { + configure(); + stubFetch(() => { + throw new TypeError('offline'); + }); + const { result } = renderHook(() => useUserRowActions()); + await act(async () => result.current.setActive('target', false)); + expect(toast.error).toHaveBeenCalledWith((locale === 'en' ? en : de).common.error_occurred); + }); +}); diff --git a/modules/users/users/admin/components/useUserRowActions.ts b/modules/users/users/admin/components/useUserRowActions.ts index 073f25d59..acee87670 100644 --- a/modules/users/users/admin/components/useUserRowActions.ts +++ b/modules/users/users/admin/components/useUserRowActions.ts @@ -4,7 +4,7 @@ import { apiResult } from '@simple-module-py/ui/lib/api'; import { useState } from 'react'; import { toast } from 'sonner'; import { api } from '../../pages/api'; -import { failureMessage } from '../../pages/failureMessage'; +import { adminFailureMessage } from '../../pages/failureMessage'; /** * The per-row actions on the users table — resend, copy a reset link, and @@ -41,7 +41,9 @@ export function useUserRowActions() { }), ); if (!resp.ok) { - toast.error(failureMessage(resp, t(keys.users.user_row.toast_resend_failed), network)); + toast.error( + adminFailureMessage(resp, t(keys.users.user_row.toast_resend_failed), network, t), + ); return; } const body = resp.data; @@ -74,7 +76,9 @@ export function useUserRowActions() { }), ); if (!resp.ok) { - toast.error(failureMessage(resp, t(keys.users.user_row.toast_reset_failed), network)); + toast.error( + adminFailureMessage(resp, t(keys.users.user_row.toast_reset_failed), network, t), + ); return; } const copied = await copyToClipboard(resp.data?.link ?? ''); @@ -97,7 +101,9 @@ export function useUserRowActions() { : api.PATCH('/api/users/admin/{user_id}/disable', params), ); if (!resp.ok) { - toast.error(failureMessage(resp, t(keys.users.user_row.toast_status_failed), network)); + toast.error( + adminFailureMessage(resp, t(keys.users.user_row.toast_status_failed), network, t), + ); return; } toast.success( diff --git a/modules/users/users/locales/de.json b/modules/users/users/locales/de.json index 0f6e31140..0c8eb2a7e 100644 --- a/modules/users/users/locales/de.json +++ b/modules/users/users/locales/de.json @@ -415,5 +415,15 @@ "incomplete_description": "Dieser Link ist unvollständig. Öffnen Sie ihn erneut aus der E-Mail.", "resend_failed": "Senden ist derzeit nicht möglich. Versuchen Sie es in einer Minute erneut." }, - "permission_group": "Benutzer" + "permission_group": "Benutzer", + "admin_errors": { + "more_privileged": "Sie können kein Konto mit höheren Berechtigungen verwalten.", + "roles_not_held": "Sie können keine Rollen zuweisen, die Sie selbst nicht innehaben.", + "own_account": "Sie können Ihr eigenes Konto nicht deaktivieren oder herabstufen.", + "last_admin": "Sie können den letzten aktiven Administrator nicht entfernen.", + "delete_self": "Sie können Ihr eigenes Konto nicht löschen.", + "email_exists": "Ein Benutzer mit dieser E-Mail-Adresse existiert bereits.", + "not_found": "Benutzer nicht gefunden", + "external_password": "Externe Benutzer (SSO) haben kein Passwort zum Zurücksetzen." + } } diff --git a/modules/users/users/locales/en.json b/modules/users/users/locales/en.json index cd1080160..4b7e1bf0e 100644 --- a/modules/users/users/locales/en.json +++ b/modules/users/users/locales/en.json @@ -415,5 +415,15 @@ "incomplete_description": "This link is incomplete — open it from the email again.", "resend_failed": "We could not send it just now. Try again in a minute." }, - "permission_group": "Users" + "permission_group": "Users", + "admin_errors": { + "more_privileged": "You cannot manage a more privileged account.", + "roles_not_held": "You cannot assign roles you do not hold.", + "own_account": "You cannot disable or demote your own account.", + "last_admin": "You cannot remove the last active administrator.", + "delete_self": "You cannot delete your own account.", + "email_exists": "A user with this email already exists.", + "not_found": "User not found", + "external_password": "External (SSO) users have no password to reset" + } } diff --git a/modules/users/users/pages/Users/components/DangerZone.tsx b/modules/users/users/pages/Users/components/DangerZone.tsx index b457e7d1c..4a7945361 100644 --- a/modules/users/users/pages/Users/components/DangerZone.tsx +++ b/modules/users/users/pages/Users/components/DangerZone.tsx @@ -8,7 +8,7 @@ import { Trash2, TriangleAlert } from 'lucide-react'; import { useState } from 'react'; import { toast } from 'sonner'; import { api } from '../../api'; -import { failureMessage } from '../../failureMessage'; +import { adminFailureMessage } from '../../failureMessage'; interface Props { userId: string; @@ -43,11 +43,11 @@ export function DangerZone({ userId, email, isSelf }: Props) { return; // navigating away — leave `deleting` set } toast.error( - failureMessage( + adminFailureMessage( res, t(keys.users.danger_zone.toast_failed), t(keys.users.common.error_occurred), - true, + t, ), ); setDeleting(false); diff --git a/modules/users/users/pages/Users/components/useEditUserForm.ts b/modules/users/users/pages/Users/components/useEditUserForm.ts index 17af47957..828c8c90b 100644 --- a/modules/users/users/pages/Users/components/useEditUserForm.ts +++ b/modules/users/users/pages/Users/components/useEditUserForm.ts @@ -5,6 +5,7 @@ import { apiResult, NETWORK_FAILURE } from '@simple-module-py/ui/lib/api'; import { useEffect, useMemo, useRef, useState } from 'react'; import { toast } from 'sonner'; import { api } from '../../api'; +import { adminFailureMessage } from '../../failureMessage'; export interface EditableUser { id: string; @@ -104,9 +105,12 @@ export function useEditUserForm(user: EditableUser) { ); if (!resp.ok) { setError( - resp.status === NETWORK_FAILURE - ? t(keys.users.common.error_occurred) - : (resp.detail ?? t(keys.users.edit.error_details)), + adminFailureMessage( + resp, + t(keys.users.edit.error_details), + t(keys.users.common.error_occurred), + t, + ), ); return; } @@ -120,12 +124,16 @@ export function useEditUserForm(user: EditableUser) { }), ); if (!resp.ok) { + const message = adminFailureMessage( + resp, + t(keys.users.edit.error_roles), + t(keys.users.common.error_occurred), + t, + ); setError( - resp.status === NETWORK_FAILURE - ? t(keys.users.common.error_occurred) - : detailsDirty - ? t(keys.users.edit.error_partial) - : t(keys.users.edit.error_roles), + detailsDirty && resp.status !== NETWORK_FAILURE + ? `${t(keys.users.edit.error_partial)} ${message}` + : message, ); return; } diff --git a/modules/users/users/pages/Users/components/useUserActions.ts b/modules/users/users/pages/Users/components/useUserActions.ts index 466066031..c4730ca33 100644 --- a/modules/users/users/pages/Users/components/useUserActions.ts +++ b/modules/users/users/pages/Users/components/useUserActions.ts @@ -3,7 +3,7 @@ import { apiResult } from '@simple-module-py/ui/lib/api'; import { useState } from 'react'; import { toast } from 'sonner'; import { api } from '../../api'; -import { failureMessage } from '../../failureMessage'; +import { adminFailureMessage } from '../../failureMessage'; /** * The immediate account actions on the edit page — disable/enable, mark @@ -39,7 +39,7 @@ export function useUserActions( onSuccess(); toast.success(okMsg); } else { - toast.error(failureMessage(res, failMsg, t(keys.users.common.error_occurred))); + toast.error(adminFailureMessage(res, failMsg, t(keys.users.common.error_occurred), t)); } }) .finally(() => setSavingStatus(false)); @@ -80,10 +80,11 @@ export function useUserActions( toast.success(t(keys.users.edit.toast_verified)); } else { toast.error( - failureMessage( + adminFailureMessage( res, t(keys.users.edit.toast_verify_failed), t(keys.users.common.error_occurred), + t, ), ); } @@ -99,10 +100,11 @@ export function useUserActions( toast.success(t(keys.users.edit.toast_reset_copied)); } else { toast.error( - failureMessage( + adminFailureMessage( res, t(keys.users.edit.toast_reset_failed), t(keys.users.common.error_occurred), + t, ), ); } diff --git a/modules/users/users/pages/failureMessage.ts b/modules/users/users/pages/failureMessage.ts index b783d9eef..959541166 100644 --- a/modules/users/users/pages/failureMessage.ts +++ b/modules/users/users/pages/failureMessage.ts @@ -1,5 +1,30 @@ +import { keys, type TranslationKey } from '@simple-module-py/i18n'; import { type ApiResult, NETWORK_FAILURE } from '@simple-module-py/ui/lib/api'; +// The API currently uses English messages as detail codes. +const detailKeys = new Map([ + ['You cannot manage a more privileged account.', keys.users.admin_errors.more_privileged], + ['You cannot assign roles you do not hold.', keys.users.admin_errors.roles_not_held], + ['You cannot disable or demote your own account.', keys.users.admin_errors.own_account], + ['You cannot remove the last active administrator.', keys.users.admin_errors.last_admin], + ['You cannot delete your own account.', keys.users.admin_errors.delete_self], + ['A user with this email already exists.', keys.users.admin_errors.email_exists], + ['User not found', keys.users.admin_errors.not_found], + ['External (SSO) users have no password to reset', keys.users.admin_errors.external_password], +]); + +/** Translate known admin refusals; unknown details keep the action's fallback. */ +export function adminFailureMessage( + res: Pick, 'status' | 'detail'>, + fallback: string, + network: string, + t: (key: TranslationKey) => string, +): string { + if (res.status === NETWORK_FAILURE) return network; + const key = res.detail ? detailKeys.get(res.detail) : undefined; + return key ? t(key) : fallback; +} + /** * The text for a failed call: `network` when the request never reached the * server, else the server's string `detail` when `useDetail` is set and it sent diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index 7383d4b81..d42060b71 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -1147,6 +1147,14 @@ export default { 'users.add_people.toast_invites_sent_one': '', 'users.add_people.toast_invites_sent_other': '', 'users.add_people.toast_user_created': '', + 'users.admin_errors.delete_self': '', + 'users.admin_errors.email_exists': '', + 'users.admin_errors.external_password': '', + 'users.admin_errors.last_admin': '', + 'users.admin_errors.more_privileged': '', + 'users.admin_errors.not_found': '', + 'users.admin_errors.own_account': '', + 'users.admin_errors.roles_not_held': '', 'users.audit.user': '', 'users.common.back_to_sign_in': '', 'users.common.back_to_users': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index b20ddb20c..cf4fe3c4e 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -1461,6 +1461,16 @@ export const keys = { toast_invites_sent_other: 'users.add_people.toast_invites_sent_other', toast_user_created: 'users.add_people.toast_user_created', }, + admin_errors: { + delete_self: 'users.admin_errors.delete_self', + email_exists: 'users.admin_errors.email_exists', + external_password: 'users.admin_errors.external_password', + last_admin: 'users.admin_errors.last_admin', + more_privileged: 'users.admin_errors.more_privileged', + not_found: 'users.admin_errors.not_found', + own_account: 'users.admin_errors.own_account', + roles_not_held: 'users.admin_errors.roles_not_held', + }, audit: { user: 'users.audit.user', }, From b1bc5e21aba3cbd8bb7b5810a1a8fbed012ca268 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 22:17:06 +0200 Subject: [PATCH 5/7] users: /api/users/me accepts the request principal (session or bearer) (#658) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 2 + .../users/tests/test_bearer_token_expiry.py | 33 +++++++++++-- modules/users/tests/test_remember_me.py | 31 +++++++++++-- .../users/tests/test_users_me_credentials.py | 46 +++++++++++++++++++ modules/users/users/auth_local/api.py | 6 +-- modules/users/users/deps.py | 17 +++++++ 6 files changed, 123 insertions(+), 12 deletions(-) create mode 100644 modules/users/tests/test_users_me_credentials.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 74504fc0a..14248f30d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,8 @@ All notable changes to this project are documented in this file. The format is b and revoking/pruning aged families while preserving reuse evidence for live families (#656). - Users admin actions show translated server refusal details instead of generic errors; unknown details retain the existing fallback (#653). +- Users: GET and PATCH `/api/users/me` now accept bearer access tokens from + `/api/users/auth/token` alongside browser sessions (#658). - Users: publish deletion, disabling and new role assignments transactionally through the durable outbox (inline cleanup without background tasks). Deletion purges tenant memberships, shares to the user and direct permission grants; tenants left without diff --git a/modules/users/tests/test_bearer_token_expiry.py b/modules/users/tests/test_bearer_token_expiry.py index d83fbe706..4a6f89e79 100644 --- a/modules/users/tests/test_bearer_token_expiry.py +++ b/modules/users/tests/test_bearer_token_expiry.py @@ -32,6 +32,30 @@ _CREDS = {"username": "admin@example.com", "password": "AdminPass1!"} +async def _cookie_strategy_user(app, client): + """Resolve the ``sm_auth`` cookie through the cookie strategy itself. + + ``/api/users/me`` now resolves the request principal (session or bearer, + #658), so it no longer exercises ``ExpiringDatabaseStrategy``; read the row + the way ``fastapi_users.current_user`` does instead. + """ + from users.backend import build_strategy + from users.db_adapter import PruningAccessTokenDatabase + from users.models import UserAccessToken + + async with app.state.sm.db.session_factory() as session: + + class _Manager: + def parse_id(self, value): + return value if isinstance(value, uuid.UUID) else uuid.UUID(str(value)) + + async def get(self, user_id): + return await session.get(User, user_id) + + strategy = build_strategy(PruningAccessTokenDatabase(session, UserAccessToken), 900) + return await strategy.read_token(client.cookies.get("sm_auth"), _Manager()) + + async def _tokens(app) -> list[UserAccessToken]: async with app.state.sm.db.session_factory() as session: return list((await session.execute(select(UserAccessToken))).scalars()) @@ -129,15 +153,14 @@ class TestAnExpiredRowStopsAuthenticating: async def test_the_cookie_strategy_path_refuses_it(self, users_app, anon_client): """`fastapi_users.current_user`, via ExpiringDatabaseStrategy. - ``/api/users/me`` is the probe because it depends on ``current_user``, - which is what reads the ``sm_auth`` row. The row stays well inside the - thirty-day read ceiling — only the deadline it was issued for has passed. + The row stays well inside the thirty-day read ceiling — only the + deadline it was issued for has passed. """ assert (await anon_client.post(_LOGIN, data=_CREDS)).status_code == 204 - assert (await anon_client.get(_ME)).status_code == 200 + assert await _cookie_strategy_user(users_app, anon_client) is not None await _expire_all(users_app) - assert (await anon_client.get(_ME)).status_code == 401 + assert await _cookie_strategy_user(users_app, anon_client) is None @pytest.mark.anyio async def test_the_provider_path_refuses_it(self, users_app, anon_client): diff --git a/modules/users/tests/test_remember_me.py b/modules/users/tests/test_remember_me.py index e681ca23c..4f23bbd2c 100644 --- a/modules/users/tests/test_remember_me.py +++ b/modules/users/tests/test_remember_me.py @@ -77,6 +77,30 @@ async def _age_access_tokens(app, *, days: int) -> None: await session.commit() +async def _cookie_strategy_user(app, client): + """Resolve the ``sm_auth`` cookie through the cookie strategy itself. + + ``/api/users/me`` now resolves the request principal (session or bearer, + #658), so it no longer exercises ``ExpiringDatabaseStrategy``; read the row + the way ``fastapi_users.current_user`` does instead. + """ + from users.backend import build_strategy + from users.db_adapter import PruningAccessTokenDatabase + from users.models import UserAccessToken + + async with app.state.sm.db.session_factory() as session: + + class _Manager: + def parse_id(self, value): + return value if isinstance(value, uuid.UUID) else uuid.UUID(str(value)) + + async def get(self, user_id): + return await session.get(User, user_id) + + strategy = build_strategy(PruningAccessTokenDatabase(session, UserAccessToken), 900) + return await strategy.read_token(client.cookies.get("sm_auth"), _Manager()) + + class TestRemembered: @pytest.mark.anyio async def test_the_auth_cookie_lasts_thirty_days(self, anon_client, users_db): @@ -104,14 +128,13 @@ async def test_a_token_inside_the_promised_window_is_still_read( know which sign-ins asked to be remembered — a row minted to last thirty days therefore has to be accepted for thirty, or the cookie outlives the credential it carries and the checkbox starts lying on - day fourteen. ``/api/users/me`` is the probe because it depends on - ``current_user``, which is the code path that reads the row. + day fourteen. """ await _make_user(users_db, "remember-token@example.com") await _login(anon_client, "remember-token@example.com", remember=True) await _age_access_tokens(users_app, days=29) - assert (await anon_client.get("/api/users/me")).status_code == 200 + assert await _cookie_strategy_user(users_app, anon_client) is not None @pytest.mark.anyio async def test_a_token_past_the_window_stops_being_read(self, anon_client, users_db, users_app): @@ -121,7 +144,7 @@ async def test_a_token_past_the_window_stops_being_read(self, anon_client, users await _login(anon_client, "stale-token@example.com", remember=True) await _age_access_tokens(users_app, days=31) - assert (await anon_client.get("/api/users/me")).status_code == 401 + assert await _cookie_strategy_user(users_app, anon_client) is None class TestTheWindowSurvivesLaterRequests: diff --git a/modules/users/tests/test_users_me_credentials.py b/modules/users/tests/test_users_me_credentials.py new file mode 100644 index 000000000..e22d38970 --- /dev/null +++ b/modules/users/tests/test_users_me_credentials.py @@ -0,0 +1,46 @@ +"""Self-profile routes accept local bearer tokens as well as browser cookies.""" + +import pytest +from _users_app_builders import user_row + + +@pytest.mark.parametrize("credential", ["bearer", "cookie"]) +async def test_users_me_reads_and_updates_profile(anon_client, alice, users_app, credential): + if credential == "bearer": + login = await anon_client.post( + "/api/users/auth/token", + json={"email": alice.email, "password": "UserPass1!"}, + ) + assert login.status_code == 200 + anon_client.cookies.clear() + headers = {"Authorization": f"Bearer {login.json()['access_token']}"} + else: + login = await anon_client.post( + "/api/users/auth/login", + data={"username": alice.email, "password": "UserPass1!"}, + ) + assert login.status_code == 204 + headers = {} + + profile = await anon_client.get("/api/users/me", headers=headers) + assert profile.status_code == 200 + assert profile.json()["id"] == str(alice.id) + assert profile.json()["email"] == alice.email + assert "hashed_password" not in profile.json() + + updated = await anon_client.patch( + "/api/users/me", headers=headers, json={"full_name": "Updated Profile"} + ) + assert updated.status_code == 200 + assert updated.json()["id"] == str(alice.id) + assert updated.json()["full_name"] == "Updated Profile" + assert (await user_row(users_app, alice.email)).full_name == "Updated Profile" + + +@pytest.mark.parametrize("method", ["GET", "PATCH"]) +@pytest.mark.parametrize("headers", [{}, {"Authorization": "Bearer invalid-token"}]) +async def test_users_me_requires_valid_credentials(anon_client, method, headers): + response = await anon_client.request( + method, "/api/users/me", headers=headers, json={"full_name": "Unauthorized"} + ) + assert response.status_code == 401 diff --git a/modules/users/users/auth_local/api.py b/modules/users/users/auth_local/api.py index b9b875867..3fb90c074 100644 --- a/modules/users/users/auth_local/api.py +++ b/modules/users/users/auth_local/api.py @@ -44,7 +44,7 @@ from users.db_adapter import get_access_token_db from users.deps import ( auth_backend, - fastapi_users, + get_current_local_user, get_user_manager, ) from users.manager import UserManager @@ -227,7 +227,7 @@ async def accept_invite( @router.get("/me", response_model=UserRead, dependencies=[Depends(Authenticated())]) -async def read_me(user=Depends(fastapi_users.current_user(active=True))): +async def read_me(user=Depends(get_current_local_user)): """Return the currently authenticated user's profile.""" return user @@ -236,7 +236,7 @@ async def read_me(user=Depends(fastapi_users.current_user(active=True))): async def update_me( data: SelfProfileUpdate, request: Request, - user=Depends(fastapi_users.current_user(active=True)), + user=Depends(get_current_local_user), user_manager: UserManager = Depends(get_user_manager), ): """Update the currently authenticated user's profile.""" diff --git a/modules/users/users/deps.py b/modules/users/users/deps.py index 9e8147c31..f9f310f23 100644 --- a/modules/users/users/deps.py +++ b/modules/users/users/deps.py @@ -15,8 +15,10 @@ import uuid from typing import TYPE_CHECKING +from auth.deps import CurrentUser from fastapi import Depends, HTTPException, Request from fastapi_users import FastAPIUsers +from fastapi_users import exceptions as fu_exceptions from simple_module_core.events import EventBus from simple_module_db.deps import get_db from sqlalchemy.ext.asyncio import AsyncSession @@ -48,6 +50,20 @@ current_superuser = fastapi_users.current_user(active=True, superuser=True) +async def get_current_local_user( + principal: CurrentUser, + user_manager: UserManager = Depends(get_user_manager), +) -> User: + """Load the local account authenticated by the session or bearer provider.""" + try: + user = await user_manager.get(user_manager.parse_id(principal.id)) + except (fu_exceptions.UserNotExists, fu_exceptions.InvalidID): + raise HTTPException(status_code=401, detail="Not authenticated") from None + if not user.is_active or user.disabled_at is not None: + raise HTTPException(status_code=401, detail="Not authenticated") + return user + + async def get_mailer(request: Request): """Return the mailer from app.state.users (built in UsersModule.on_startup).""" return request.app.state.users.mailer @@ -79,6 +95,7 @@ async def get_user_service( "current_superuser", "fastapi_users", "get_access_token_db", + "get_current_local_user", "get_event_bus", "get_mailer", "get_user_db", From 8e6ac628eb59a16be03e33462166db76439650ce Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 22:20:13 +0200 Subject: [PATCH 6/7] users: OAuth sign-in refusals redirect to the login page with a translated message (#654) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 2 + modules/users/tests-js/users_Login.test.tsx | 16 +++ .../tests/test_users_oauth_error_responses.py | 111 ++++++++++++++++++ .../tests/test_users_oauth_login_errors.py | 51 ++++++++ modules/users/users/auth_local/views.py | 3 + modules/users/users/constants.py | 2 + modules/users/users/locales/de.json | 3 + modules/users/users/locales/en.json | 3 + modules/users/users/oauth/api.py | 43 +++---- modules/users/users/oauth/errors.py | 27 +++++ modules/users/users/pages/Login.tsx | 12 +- packages/i18n/src/generated-resources.ts | 3 + packages/i18n/src/keys.generated.ts | 3 + 13 files changed, 254 insertions(+), 25 deletions(-) create mode 100644 modules/users/tests/test_users_oauth_error_responses.py create mode 100644 modules/users/tests/test_users_oauth_login_errors.py create mode 100644 modules/users/users/oauth/errors.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 14248f30d..e0bfd2339 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,8 @@ All notable changes to this project are documented in this file. The format is b unknown details retain the existing fallback (#653). - Users: GET and PATCH `/api/users/me` now accept bearer access tokens from `/api/users/auth/token` alongside browser sessions (#658). +- Users: redirect refused OAuth/OIDC browser sign-ins to login with translated English + and German messages; preserve JSON errors for API callers (#654). - Users: publish deletion, disabling and new role assignments transactionally through the durable outbox (inline cleanup without background tasks). Deletion purges tenant memberships, shares to the user and direct permission grants; tenants left without diff --git a/modules/users/tests-js/users_Login.test.tsx b/modules/users/tests-js/users_Login.test.tsx index 910066275..b7fbb71e7 100644 --- a/modules/users/tests-js/users_Login.test.tsx +++ b/modules/users/tests-js/users_Login.test.tsx @@ -42,6 +42,22 @@ afterEach(() => { }); describe('users Login', () => { + it.each([ + ['account_not_verified', keys.users.login.oauth_account_not_verified], + ['signup_disabled', keys.users.login.oauth_signup_disabled], + ['signin_failed', keys.users.login.oauth_signin_failed], + ])('shows the translated OAuth message for %s', (code, key) => { + configureI18n({ locale: 'en', messages: { [key]: 'Sign-in was refused.' } }); + render(); + expect(screen.getByRole('alert')).toHaveTextContent('Sign-in was refused.'); + configureI18n({ locale: 'en', messages: {} }); + }); + + it('ignores unknown OAuth codes instead of displaying query text', () => { + render(); + expect(screen.queryByRole('alert')).toBeNull(); + }); + it('posts the form-encoded credentials and follows the server redirect on 204', async () => { const mock = stubFetch(() => new Response(null, { status: 204 })); render(); diff --git a/modules/users/tests/test_users_oauth_error_responses.py b/modules/users/tests/test_users_oauth_error_responses.py new file mode 100644 index 000000000..8422c851a --- /dev/null +++ b/modules/users/tests/test_users_oauth_error_responses.py @@ -0,0 +1,111 @@ +"""Exercise callback error responses without a database or provider network.""" + +from types import SimpleNamespace +from unittest.mock import AsyncMock + +import httpx +import pytest +from fastapi import APIRouter, FastAPI +from fastapi_users.exceptions import UserNotExists +from httpx_oauth.exceptions import GetProfileError +from starlette.middleware.sessions import SessionMiddleware +from users.deps import auth_backend, get_user_manager +from users.oauth.api import register_oauth_routes +from users.oauth.providers import OAuthProvider + + +@pytest.mark.anyio +@pytest.mark.parametrize( + ("failure", "detail", "public_code", "status"), + [ + ("unverified", "OAUTH_ACCOUNT_NOT_VERIFIED", "account_not_verified", 403), + ("signup", "OAUTH_SIGNUP_DISABLED", "signup_disabled", 403), + ("provider", "OAUTH_PROVIDER_ERROR", "signin_failed", 400), + ("profile", "OAUTH_PROVIDER_ERROR", "signin_failed", 400), + ("token", "OAUTH_PROVIDER_ERROR", "signin_failed", 400), + ("state", "OAUTH_INVALID_STATE", "signin_failed", 400), + ("missing_code", "OAUTH_MISSING_CODE", "signin_failed", 400), + ], +) +@pytest.mark.parametrize("accept", ["text/html", "application/json", "*/*", "xhr"]) +async def test_oauth_callback_error_response(failure, detail, public_code, status, accept): + app = FastAPI() + app.add_middleware(SessionMiddleware, secret_key="test-secret") + provider_client = AsyncMock() + provider_client.get_authorization_url.side_effect = lambda url, state: ( + f"https://idp/?state={state}" + ) + provider_client.get_access_token.return_value = {"access_token": "test-token"} + provider_client.get_profile.return_value = {"sub": "test-id", "email": "test@example.com"} + if failure == "token": + provider_client.get_access_token.side_effect = httpx.ConnectError("private details") + if failure == "profile": + provider_client.get_profile.side_effect = GetProfileError("private provider details") + app.state.users = SimpleNamespace( + oauth_clients={"oidc": OAuthProvider("oidc", "OIDC", provider_client)}, + settings=SimpleNamespace(allow_signup=False), + ) + manager = AsyncMock() + manager.get_by_oauth_account.side_effect = UserNotExists() + if failure == "unverified": + manager.get_by_email.return_value = SimpleNamespace(is_verified=False, invited_at=None) + else: + manager.get_by_email.side_effect = UserNotExists() + + async def manager_dep(): + return manager + + async def strategy_dep(): + return None + + app.dependency_overrides[get_user_manager] = manager_dep + app.dependency_overrides[auth_backend.get_strategy] = strategy_dep + router = APIRouter(prefix="/api/users") + register_oauth_routes(router) + app.include_router(router) + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), base_url="http://test" + ) as client: + login = await client.get("/api/users/auth/oidc/login") + state = httpx.URL(login.headers["location"]).params["state"] + params = {"state": "bad" if failure == "state" else state} + if failure == "provider": + params["error"] = "" + elif failure != "missing_code": + params["code"] = "test-code" + headers = {"accept": accept} + if accept == "xhr": + headers = {"accept": "text/html", "x-requested-with": "XMLHttpRequest"} + response = await client.get("/api/users/auth/oidc/callback", params=params, headers=headers) + if accept == "text/html": + assert response.status_code == 303 + assert response.headers["location"] == f"/users/login?oauth_error={public_code}" + else: + assert response.status_code == status + assert response.json() == {"detail": detail} + + +@pytest.mark.anyio +@pytest.mark.parametrize( + "code", ["account_not_verified", "signup_disabled", "signin_failed", ""}, + headers={"accept": "text/html" if browser else "application/json"}, + follow_redirects=False, + ) + if browser: + assert response.status_code == 303 + assert response.headers["location"] == "/users/login?oauth_error=signin_failed" + else: + assert response.status_code == 400 + assert response.json()["detail"] == "OAUTH_PROVIDER_ERROR" + + +@pytest.mark.anyio +@pytest.mark.parametrize( + "code", ["account_not_verified", "signup_disabled", "signin_failed", "