From 110b72016b3a1004717d784a2eb4118d5833aefd Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 14:38:37 +0200 Subject: [PATCH 1/6] users: refresh tokens are consumed atomically; reusing a spent token revokes its family (#625) UPDATE ... WHERE revoked_at IS NULL RETURNING picks exactly one winner under concurrency; presenting an already-revoked token revokes every live token in its family (new family_id column, migration a625f0b7c912, branch label users). Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 2 + docs/database/migrations.md | 4 +- ...25f0b7c912_users_refresh_token_families.py | 32 ++++++++ host/tests/test_migration_graph.py | 5 +- .../users/tests/test_users_refresh_reuse.py | 74 +++++++++++++++++++ modules/users/users/auth_local/token_api.py | 42 +++++++---- modules/users/users/models/refresh_token.py | 1 + 7 files changed, 144 insertions(+), 16 deletions(-) create mode 100644 host/migrations/versions/a625f0b7c912_users_refresh_token_families.py create mode 100644 modules/users/tests/test_users_refresh_reuse.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d523100a..07c28d4e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,8 @@ All notable changes to this project are documented in this file. The format is b so an account verified after signing in can accept without signing in again. **Behaviour change:** with `users.require_verification` off, self-registered accounts must verify their email (or be marked verified by an admin) before accepting an invitation. +- Users: atomically consume refresh tokens and revoke their refresh-token family on reuse + (#625). Migration `a625f0b7c912` adds family tracking; existing tokens start separate families. - DB: sessions bound to a SQLAlchemy `Connection` now inherit its engine's tenant policy and audit configuration, preserving strict tenant isolation and audit callbacks (#630). - Keycloak JWKS fetches now use a per-cache lock and a 30-second minimum interval diff --git a/docs/database/migrations.md b/docs/database/migrations.md index f9c2f26d7..64daeee63 100644 --- a/docs/database/migrations.md +++ b/docs/database/migrations.md @@ -21,7 +21,7 @@ The history is **not** a single linear chain. It has several roots and several h | Head | Owning module | Label Alembic reports | |---|---|---| -| `c8e4f1a2b6d9` | users (login lockout) | `api_tokens` | +| `a625f0b7c912` | users (refresh-token families) | `api_tokens`, `background_tasks`, `users` | | `a7c2e91d4b58` | audit_log | `audit_log` | | `f1a9c3d7b2e4` | tenants | `tenants` | | `d263ea727663` | framework (`sm_lease`) | `leases` | @@ -34,7 +34,7 @@ A module's first migration sets `branch_labels = ("",)`. Alembic ** ### Known mislabels -Only six of the revisions set `branch_labels` (`audit_log`, `api_tokens`, `keycloak`, `tenants`, `sharing`, `leases`). The graph carries these defects; they are documented rather than rewritten, because changing labels on revisions that installed databases already applied is not worth the risk: +The declared `branch_labels` are `audit_log`, `api_tokens`, `background_tasks`, `keycloak`, `tenants`, `sharing`, `leases` and `users` (added by `a625f0b7c912`). The graph carries these defects; they are documented rather than rewritten, because changing labels on revisions that installed databases already applied is not worth the risk: - `51b2058d8afd_api_tokens_initial` sits in the middle of the trunk, so every revision above it (file_storage, background_tasks including the outbox, and the users revisions through `c8e4f1a2b6d9`) reports the `api_tokens` label. The `api_tokens` head is therefore really a users revision. - `keycloak` branches off a users revision, so a Keycloak-only install still builds the users schema. diff --git a/host/migrations/versions/a625f0b7c912_users_refresh_token_families.py b/host/migrations/versions/a625f0b7c912_users_refresh_token_families.py new file mode 100644 index 000000000..f46a7520f --- /dev/null +++ b/host/migrations/versions/a625f0b7c912_users_refresh_token_families.py @@ -0,0 +1,32 @@ +"""users: refresh-token families (#625). + +Revision ID: a625f0b7c912 +Revises: 1f5985daa10b +""" + +import sqlalchemy as sa +from alembic import op + +revision = "a625f0b7c912" +down_revision = "1f5985daa10b" +branch_labels = ("users",) +depends_on = None + +_TABLE = "users_refresh_token" +_INDEX = "ix_users_refresh_token_family_id" + + +def upgrade() -> None: + op.add_column(_TABLE, sa.Column("family_id", sa.Uuid(), nullable=True)) + tokens = sa.table(_TABLE, sa.column("token", sa.Uuid()), sa.column("family_id", sa.Uuid())) + # Historical rotations have no lineage; each existing token starts a family. + op.execute(tokens.update().values(family_id=tokens.c.token)) + with op.batch_alter_table(_TABLE) as batch: + batch.alter_column("family_id", existing_type=sa.Uuid(), nullable=False) + batch.create_index(_INDEX, ["family_id"]) + + +def downgrade() -> None: + with op.batch_alter_table(_TABLE) as batch: + batch.drop_index(_INDEX) + batch.drop_column("family_id") diff --git a/host/tests/test_migration_graph.py b/host/tests/test_migration_graph.py index f3b8861f8..521b27348 100644 --- a/host/tests/test_migration_graph.py +++ b/host/tests/test_migration_graph.py @@ -39,6 +39,7 @@ "e878882924e7": "background_tasks", "bb906e1a1089": "background_tasks", # task progress + owner (#536) "1f5985daa10b": "background_tasks", # retained log records (#537) + "a625f0b7c912": "users", # refresh-token families (#625) "51b2058d8afd": "api_tokens", "168a2882f443": "keycloak", "f6a3b9d2e8c4": "keycloak", @@ -55,7 +56,7 @@ # head revision -> module owning the tables it creates or alters. _HEADS = { "a7c2e91d4b58": "audit_log", - "1f5985daa10b": "background_tasks", # also carries the inherited ``api_tokens`` + "a625f0b7c912": "users", # also carries ``api_tokens`` and ``background_tasks`` "f1a9c3d7b2e4": "tenants", "f6a3b9d2e8c4": "keycloak", "f733c7938fd1": "sharing", @@ -71,6 +72,7 @@ "f733c7938fd1": "sharing", "bb906e1a1089": "background_tasks", "d263ea727663": "leases", + "a625f0b7c912": "users", } # Known mislabels: revisions that inherit a label from an ancestor owned by a @@ -93,6 +95,7 @@ "d7b3a9e5c1f2": "api_tokens", "bb906e1a1089": "api_tokens", # also its own ``background_tasks`` "1f5985daa10b": "api_tokens", # also inherits ``background_tasks`` + "a625f0b7c912": "api_tokens", # also inherits ``background_tasks`` "51b2058d8afd": "api_tokens", # ancestors of a labelled revision inherit its label too } diff --git a/modules/users/tests/test_users_refresh_reuse.py b/modules/users/tests/test_users_refresh_reuse.py new file mode 100644 index 000000000..961a9ab2d --- /dev/null +++ b/modules/users/tests/test_users_refresh_reuse.py @@ -0,0 +1,74 @@ +"""Refresh rotation must consume a token once and detect reuse (#625).""" + +import asyncio + +from _users_app_builders import _build_users_app, _client, _make_standard_user +from sqlalchemy.ext.asyncio import AsyncSession +from sqlalchemy.sql import Select +from users.models.refresh_token import RefreshToken + + +async def _login(client): + response = await client.post( + "/api/users/auth/token", + json={"email": "user@example.com", "password": "UserPass1!"}, + ) + assert response.status_code == 200 + return response.json()["refresh_token"] + + +async def _refresh(client, token): + return await client.post("/api/users/auth/token/refresh", json={"refresh_token": token}) + + +async def test_users_refresh_reuse_revokes_descendants_only(anon_client, users_app): + await _make_standard_user(users_app) + original = await _login(anon_client) + separate_login = await _login(anon_client) + child = await _refresh(anon_client, original) + assert child.status_code == 200 + grandchild = await _refresh(anon_client, child.json()["refresh_token"]) + assert grandchild.status_code == 200 + + assert (await _refresh(anon_client, original)).status_code == 401 + assert (await _refresh(anon_client, grandchild.json()["refresh_token"])).status_code == 401 + assert (await _refresh(anon_client, separate_login)).status_code == 200 + + +async def test_users_concurrent_refresh_has_one_winner(monkeypatch, tmp_path): + # Separate connections: an in-memory SQLite StaticPool shares a transaction. + app, lifespan = await _build_users_app( + monkeypatch, + allow_signup=False, + database_url=f"sqlite+aiosqlite:///{tmp_path / 'refresh.db'}", + ) + try: + await _make_standard_user(app) + async with _client(app, "127.0.0.1") as client: + token = await _login(client) + execute = AsyncSession.execute + readers = asyncio.Barrier(2) + + async def synchronized_read(session, statement, *args, **kwargs): + result = await execute(session, statement, *args, **kwargs) + if isinstance(statement, Select) and any( + desc.get("entity") is RefreshToken for desc in statement.column_descriptions + ): + frozen = result.freeze() + row = frozen().scalar_one_or_none() + if row is not None and row.revoked_at is None: + # Force both stale reads before either request can burn the token. + await readers.wait() + return frozen() + return result + + monkeypatch.setattr(AsyncSession, "execute", synchronized_read) + responses = await asyncio.wait_for( + asyncio.gather(_refresh(client, token), _refresh(client, token)), + timeout=15, + ) + assert sorted(response.status_code for response in responses) == [200, 401] + winner = next(response for response in responses if response.status_code == 200) + assert (await _refresh(client, winner.json()["refresh_token"])).status_code == 401 + finally: + await lifespan.__aexit__(None, None, None) diff --git a/modules/users/users/auth_local/token_api.py b/modules/users/users/auth_local/token_api.py index 36fed3e6d..bc084f545 100644 --- a/modules/users/users/auth_local/token_api.py +++ b/modules/users/users/auth_local/token_api.py @@ -13,9 +13,10 @@ from fastapi import APIRouter, Depends, HTTPException, Request from simple_module_db.deps import get_db from simple_module_db.transaction import rollback_session -from sqlalchemy import func, select +from sqlalchemy import func, select, update from sqlalchemy.ext.asyncio import AsyncSession from sqlmodel import SQLModel, col +from starlette.responses import JSONResponse from users.auth_local.lockout import ( admit, @@ -136,19 +137,32 @@ async def token_refresh( raise HTTPException(status_code=401, detail="Invalid refresh token") from None now = datetime.now(UTC) - stmt = select(RefreshToken).where( - col(RefreshToken.token) == token_uuid, - RefreshToken.revoked_at.is_(None), - col(RefreshToken.expires_at) > now, + stmt = ( + update(RefreshToken) + .where( + col(RefreshToken.token) == token_uuid, + col(RefreshToken.revoked_at).is_(None), + col(RefreshToken.expires_at) > now, + ) + .values(revoked_at=now) + .returning(RefreshToken) ) rt = (await db.execute(stmt)).scalar_one_or_none() if rt is None: - raise HTTPException(status_code=401, detail="Invalid or expired refresh token") - - # Burn the presented token first: it is committed even when the account - # check below answers 401, so a deactivated account's token stays dead. - rt.revoked_at = now - await db.flush() + reused = ( + await db.execute(select(RefreshToken).where(col(RefreshToken.token) == token_uuid)) + ).scalar_one_or_none() + if reused is not None and reused.revoked_at is not None: + await db.execute( + update(RefreshToken) + .where( + col(RefreshToken.family_id) == reused.family_id, + col(RefreshToken.revoked_at).is_(None), + ) + .values(revoked_at=now) + ) + # Returning a response lets get_db commit revocations; raising rolls them back. + return JSONResponse({"detail": "Invalid or expired refresh token"}, status_code=401) # The account is re-read rather than trusted from the row: a refresh token # outlives the access tokens it mints, so between two refreshes the user may @@ -157,14 +171,14 @@ async def token_refresh( # longer has one. user = (await db.execute(select(User).where(col(User.id) == rt.user_id))).scalar_one_or_none() if user is None or not user.is_active or user.disabled_at is not None: - raise HTTPException(status_code=401, detail="Invalid or expired refresh token") + return JSONResponse({"detail": "Invalid or expired refresh token"}, status_code=401) if is_read_only_demo(request.app, user.id): # A refusal must not consume the token: undo the revocation above. 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) + return await _create_token_pair(db, user.id, settings, user.session_version, rt.family_id) @router.delete("/token") @@ -191,6 +205,7 @@ async def _create_token_pair( user_id: uuid_mod.UUID, settings, session_version: int = 0, + family_id: uuid_mod.UUID | None = None, ) -> TokenResponse: """Mint a new access token + refresh token pair and persist both. @@ -215,6 +230,7 @@ async def _create_token_pair( refresh = RefreshToken( token=uuid_mod.uuid4(), + family_id=family_id or uuid_mod.uuid4(), user_id=user_id, created_at=now, expires_at=now + timedelta(seconds=settings.refresh_token_lifetime_seconds), diff --git a/modules/users/users/models/refresh_token.py b/modules/users/users/models/refresh_token.py index 01fc1cd31..dd8827172 100644 --- a/modules/users/users/models/refresh_token.py +++ b/modules/users/users/models/refresh_token.py @@ -23,6 +23,7 @@ class RefreshToken(Base, table=True): # ty: ignore[unsupported-base] # never matched a parent row and any SQL join across it returned nothing. # Invisible until ``PRAGMA foreign_keys=ON`` (GH #339). user_id: uuid_mod.UUID = Field(sa_type=GUID, foreign_key="users_user.id", index=True) + family_id: uuid_mod.UUID = Field(default_factory=uuid_mod.uuid4, index=True) created_at: datetime = Field(default_factory=lambda: datetime.now(UTC)) expires_at: datetime revoked_at: datetime | None = None From 1d81355712250c8de5be67548f78da32b7c290c4 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 14:59:50 +0200 Subject: [PATCH 2/6] users: hash access/refresh tokens at rest, prune spent ones, revoke the cookie token on logout (#626) SHA-256 digests replace raw bearer secrets (migration a626b1c8d023 clears existing rows: one re-login). Logins/refreshes prune the user's expired/revoked tokens in bounded batches, keeping reuse evidence for live families (#625). Logout now deletes the presented access token instead of leaving it valid until expiry. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 6 + docs/database/migrations.md | 13 +- ...c8d023_users_hash_bearer_tokens_at_rest.py | 41 ++++ host/tests/test_migration_graph.py | 4 +- modules/users/tests/test_bearer_token_age.py | 8 +- .../users/tests/test_bearer_token_expiry.py | 6 +- .../users/tests/test_service_admin_crud.py | 6 +- modules/users/tests/test_sessions_revoke.py | 1 + .../tests/test_users_token_hash_migration.py | 60 ++++++ .../users/tests/test_users_token_pruning.py | 176 ++++++++++++++++++ .../users/tests/test_users_token_storage.py | 104 +++++++++++ modules/users/users/auth_local/api.py | 6 +- modules/users/users/auth_local/token_api.py | 20 +- modules/users/users/auth_local/views.py | 4 +- modules/users/users/db_adapter.py | 12 +- modules/users/users/models/access_token.py | 2 +- modules/users/users/models/refresh_token.py | 2 +- modules/users/users/token_storage.py | 50 +++++ modules/users/users/token_strategy.py | 15 +- 19 files changed, 513 insertions(+), 23 deletions(-) create mode 100644 host/migrations/versions/a626b1c8d023_users_hash_bearer_tokens_at_rest.py create mode 100644 modules/users/tests/test_users_token_hash_migration.py create mode 100644 modules/users/tests/test_users_token_pruning.py create mode 100644 modules/users/tests/test_users_token_storage.py create mode 100644 modules/users/users/token_storage.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 07c28d4e0..297885143 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,12 @@ All notable changes to this project are documented in this file. The format is b so an account verified after signing in can accept without signing in again. **Behaviour change:** with `users.require_verification` off, self-registered accounts must verify their email (or be marked verified by an admin) before accepting an invitation. +- Users: store access and refresh bearer tokens as SHA-256 digests and prune spent + credentials in bounded batches on login/refresh, retaining active-family + reuse evidence (#626). **Behaviour change:** migration `a626b1c8d023` deletes + existing access/refresh tokens; affected clients must log in once again. Logging out + (`POST /users/logout`, `POST /api/users/auth/logout`) now deletes the presented access + token server-side instead of only clearing the cookie. - Users: atomically consume refresh tokens and revoke their refresh-token family on reuse (#625). Migration `a625f0b7c912` adds family tracking; existing tokens start separate families. - DB: sessions bound to a SQLAlchemy `Connection` now inherit its engine's tenant diff --git a/docs/database/migrations.md b/docs/database/migrations.md index 64daeee63..d84a41e0d 100644 --- a/docs/database/migrations.md +++ b/docs/database/migrations.md @@ -21,7 +21,7 @@ The history is **not** a single linear chain. It has several roots and several h | Head | Owning module | Label Alembic reports | |---|---|---| -| `a625f0b7c912` | users (refresh-token families) | `api_tokens`, `background_tasks`, `users` | +| `a626b1c8d023` | users (hashed bearer tokens) | `api_tokens`, `background_tasks`, `users` | | `a7c2e91d4b58` | audit_log | `audit_log` | | `f1a9c3d7b2e4` | tenants | `tenants` | | `d263ea727663` | framework (`sm_lease`) | `leases` | @@ -44,6 +44,17 @@ Guidance: give an unlabelled module a `branch_labels` on its **next** revision. `host/tests/test_migration_graph.py` pins the roots, heads, each revision's owning module and the mislabels above. Adding a root, head or revision fails it until the table is updated. +## Users token storage upgrade (#626) + +Migration `a626b1c8d023` follows `a625f0b7c912` on `users@head` and changes +refresh and access token keys to SHA-256 digests. **Behaviour change:** upgrade +deletes existing refresh/access token rows, requiring one re-login for clients +using them (including `sm_auth` cookies). Other session cookies are unaffected. +Downgrade also deletes token rows: hashes cannot be restored to bearer secrets. +Cookie login, token login and refresh prune up to 100 spent rows per table for +that user. Refresh reuse evidence is retained while the family has a live token, including when +an ancestor has expired; inactive users' rows wait until their next login. + ## Day-to-day workflow ### Create a migration diff --git a/host/migrations/versions/a626b1c8d023_users_hash_bearer_tokens_at_rest.py b/host/migrations/versions/a626b1c8d023_users_hash_bearer_tokens_at_rest.py new file mode 100644 index 000000000..9be50da0f --- /dev/null +++ b/host/migrations/versions/a626b1c8d023_users_hash_bearer_tokens_at_rest.py @@ -0,0 +1,41 @@ +"""users: hash bearer tokens at rest (#626). + +Revision ID: a626b1c8d023 +Revises: a625f0b7c912 + +Existing credentials are invalidated: clients must log in again once. +""" + +import sqlalchemy as sa +from alembic import op + +revision = "a626b1c8d023" +down_revision = "a625f0b7c912" +branch_labels = None +depends_on = None + + +def _clear_tokens() -> None: + for table in ("users_refresh_token", "users_access_token"): + op.execute(sa.table(table).delete()) + + +def upgrade() -> None: + _clear_tokens() + with op.batch_alter_table("users_refresh_token") as batch: + batch.alter_column( + "token", existing_type=sa.Uuid(), type_=sa.String(64), postgresql_using="token::text" + ) + with op.batch_alter_table("users_access_token") as batch: + batch.alter_column("token", existing_type=sa.String(43), type_=sa.String(64)) + + +def downgrade() -> None: + # Digests cannot be restored to UUIDs or bearer secrets. + _clear_tokens() + with op.batch_alter_table("users_refresh_token") as batch: + batch.alter_column( + "token", existing_type=sa.String(64), type_=sa.Uuid(), postgresql_using="token::uuid" + ) + with op.batch_alter_table("users_access_token") as batch: + batch.alter_column("token", existing_type=sa.String(64), type_=sa.String(43)) diff --git a/host/tests/test_migration_graph.py b/host/tests/test_migration_graph.py index 521b27348..b82b00031 100644 --- a/host/tests/test_migration_graph.py +++ b/host/tests/test_migration_graph.py @@ -40,6 +40,7 @@ "bb906e1a1089": "background_tasks", # task progress + owner (#536) "1f5985daa10b": "background_tasks", # retained log records (#537) "a625f0b7c912": "users", # refresh-token families (#625) + "a626b1c8d023": "users", # hashed bearer tokens (#626) "51b2058d8afd": "api_tokens", "168a2882f443": "keycloak", "f6a3b9d2e8c4": "keycloak", @@ -56,7 +57,7 @@ # head revision -> module owning the tables it creates or alters. _HEADS = { "a7c2e91d4b58": "audit_log", - "a625f0b7c912": "users", # also carries ``api_tokens`` and ``background_tasks`` + "a626b1c8d023": "users", # also carries ``api_tokens`` and ``background_tasks`` "f1a9c3d7b2e4": "tenants", "f6a3b9d2e8c4": "keycloak", "f733c7938fd1": "sharing", @@ -96,6 +97,7 @@ "bb906e1a1089": "api_tokens", # also its own ``background_tasks`` "1f5985daa10b": "api_tokens", # also inherits ``background_tasks`` "a625f0b7c912": "api_tokens", # also inherits ``background_tasks`` + "a626b1c8d023": "api_tokens", # also inherits ``background_tasks`` "51b2058d8afd": "api_tokens", # ancestors of a labelled revision inherit its label too } diff --git a/modules/users/tests/test_bearer_token_age.py b/modules/users/tests/test_bearer_token_age.py index 53fe0db97..37a136ec7 100644 --- a/modules/users/tests/test_bearer_token_age.py +++ b/modules/users/tests/test_bearer_token_age.py @@ -9,6 +9,7 @@ from __future__ import annotations +import hashlib import uuid from datetime import UTC, datetime, timedelta @@ -27,17 +28,18 @@ async def _mint(users_app) -> str: user_id = ( await session.execute(select(User.id).where(User.email == "admin@example.com")) ).scalar_one() - token = UserAccessToken(token=uuid.uuid4().hex, user_id=user_id) + secret = uuid.uuid4().hex + token = UserAccessToken(token=hashlib.sha256(secret.encode()).hexdigest(), user_id=user_id) session.add(token) await session.commit() - return token.token + return secret async def _backdate(users_app, token: str, *, seconds: int) -> None: async with users_app.state.sm.db.session_factory() as session: await session.execute( update(UserAccessToken) - .where(UserAccessToken.token == token) + .where(UserAccessToken.token == hashlib.sha256(token.encode()).hexdigest()) .values(created_at=datetime.now(UTC) - timedelta(seconds=seconds)) ) await session.commit() diff --git a/modules/users/tests/test_bearer_token_expiry.py b/modules/users/tests/test_bearer_token_expiry.py index 67a4fb4ba..d83fbe706 100644 --- a/modules/users/tests/test_bearer_token_expiry.py +++ b/modules/users/tests/test_bearer_token_expiry.py @@ -17,6 +17,7 @@ from __future__ import annotations +import hashlib import uuid from datetime import UTC, datetime, timedelta @@ -78,7 +79,7 @@ async def _expire(app, token: str) -> None: out. """ async with app.state.sm.db.session_factory() as session: - row = await session.get(UserAccessToken, token) + row = await session.get(UserAccessToken, hashlib.sha256(token.encode()).hexdigest()) row.expires_at = datetime.now(UTC) - timedelta(seconds=1) await session.commit() @@ -234,7 +235,8 @@ async def test_a_refresh_carries_the_accounts_counter(self, users_app, anon_clie row = ( await session.execute( select(UserAccessToken).where( - UserAccessToken.token == rotated.json()["access_token"] + UserAccessToken.token + == hashlib.sha256(rotated.json()["access_token"].encode()).hexdigest() ) ) ).scalar_one() diff --git a/modules/users/tests/test_service_admin_crud.py b/modules/users/tests/test_service_admin_crud.py index 6e0cd02ee..0360d3daa 100644 --- a/modules/users/tests/test_service_admin_crud.py +++ b/modules/users/tests/test_service_admin_crud.py @@ -144,7 +144,11 @@ async def test_delete_user_removes_user_and_all_child_rows(users_app): account_email="todelete@example.com", ) ) - session.add(RefreshToken(user_id=user.id, expires_at=datetime.now(UTC) + timedelta(days=1))) + session.add( + RefreshToken( + token="a" * 64, user_id=user.id, expires_at=datetime.now(UTC) + timedelta(days=1) + ) + ) await session.flush() svc = _build_service(session, users_app) diff --git a/modules/users/tests/test_sessions_revoke.py b/modules/users/tests/test_sessions_revoke.py index 226c0705d..690a9d36c 100644 --- a/modules/users/tests/test_sessions_revoke.py +++ b/modules/users/tests/test_sessions_revoke.py @@ -104,6 +104,7 @@ async def test_refresh_tokens_are_revoked(self, users_app, anon_client, signed_i async with users_app.state.sm.db.session_factory() as session: token = RefreshToken( + token="a" * 64, user_id=signed_in_admin, expires_at=datetime.now(UTC) + timedelta(days=1), ) diff --git a/modules/users/tests/test_users_token_hash_migration.py b/modules/users/tests/test_users_token_hash_migration.py new file mode 100644 index 000000000..62e295569 --- /dev/null +++ b/modules/users/tests/test_users_token_hash_migration.py @@ -0,0 +1,60 @@ +"""The hash migration invalidates plaintext credentials and round-trips (#626).""" + +import importlib.util +import uuid +from pathlib import Path + +import sqlalchemy as sa +from alembic.migration import MigrationContext +from alembic.operations import Operations + + +def test_users_hash_migration_invalidates_tokens_in_both_directions(): + path = Path(__file__).resolve().parents[3] / "host/migrations/versions" + spec = importlib.util.spec_from_file_location( + "hash_migration", path / "a626b1c8d023_users_hash_bearer_tokens_at_rest.py" + ) + migration = importlib.util.module_from_spec(spec) + spec.loader.exec_module(migration) + engine = sa.create_engine("sqlite://") + metadata = sa.MetaData() + refresh = sa.Table( + "users_refresh_token", + metadata, + sa.Column("token", sa.Uuid(), primary_key=True), + sa.Column("family_id", sa.Uuid(), nullable=False, index=True), + ) + access = sa.Table( + "users_access_token", metadata, sa.Column("token", sa.String(43), primary_key=True) + ) + try: + with engine.begin() as connection: + metadata.create_all(connection) + connection.execute(refresh.insert().values(token=uuid.uuid4(), family_id=uuid.uuid4())) + connection.execute(access.insert().values(token="plaintext-access")) + with Operations.context(MigrationContext.configure(connection)): + migration.upgrade() + for table in (refresh, access): + assert connection.scalar(sa.select(sa.func.count()).select_from(table)) == 0 + column = sa.inspect(connection).get_columns(table.name)[0] + assert column["type"].length == 64 + connection.execute( + sa.text( + "INSERT INTO users_refresh_token (token, family_id) " + "VALUES (:token, :family)" + ), + {"token": "a" * 64, "family": uuid.uuid4().hex}, + ) + connection.execute( + sa.text("INSERT INTO users_access_token (token) VALUES (:token)"), + {"token": "b" * 64}, + ) + migration.downgrade() + for table in (refresh, access): + assert connection.scalar(sa.select(sa.func.count()).select_from(table)) == 0 + connection.execute( + refresh.insert().values(token=uuid.uuid4(), family_id=uuid.uuid4()) + ) + connection.execute(access.insert().values(token="new-plaintext")) + finally: + engine.dispose() diff --git a/modules/users/tests/test_users_token_pruning.py b/modules/users/tests/test_users_token_pruning.py new file mode 100644 index 000000000..b9df0563f --- /dev/null +++ b/modules/users/tests/test_users_token_pruning.py @@ -0,0 +1,176 @@ +"""Exercise bounded pruning SQL without the async SQLite driver (#626).""" + +import asyncio +import hashlib +import uuid +from datetime import UTC, datetime, timedelta +from types import SimpleNamespace + +import sqlalchemy as sa +from fastapi_users_db_sqlalchemy.generics import GUID +from sqlalchemy.orm import Session +from users.models import RefreshToken, UserAccessToken +from users.token_storage import prune_tokens + + +def test_users_pruning_is_bounded_scoped_and_keeps_active_family_evidence(): + engine = sa.create_engine("sqlite://") + metadata = sa.MetaData() + users = sa.Table("users_user", metadata, sa.Column("id", GUID(), primary_key=True)) + refresh = RefreshToken.__table__.to_metadata(metadata) + access = UserAccessToken.__table__.to_metadata(metadata) + now = datetime.now(UTC) + user_id, other_id, family = uuid.uuid4(), uuid.uuid4(), uuid.uuid4() + + try: + with engine.begin() as connection: + metadata.create_all(connection) + connection.execute(users.insert(), [{"id": user_id}, {"id": other_id}]) + + def add_refresh(token, owner, *, active=False, family_id=None): + connection.execute( + refresh.insert().values( + token=token, + user_id=owner, + family_id=family_id or uuid.uuid4(), + created_at=now, + expires_at=now + timedelta(days=1) if active else now, + revoked_at=None if active else now, + ) + ) + + def add_access(token, owner, *, expired=True): + connection.execute( + access.insert().values( + token=token, + user_id=owner, + created_at=now, + expires_at=now if expired else now + timedelta(days=1), + session_version=0, + ) + ) + + for i in range(101): + add_refresh(f"spent-{i}", user_id) + add_access(f"expired-{i}", user_id) + add_refresh("other-user", other_id) + add_access("other-user", other_id) + add_refresh("live-child", user_id, active=True, family_id=family) + add_refresh("expired-ancestor", user_id, family_id=family) + add_access("live-access", user_id, expired=False) + + class ExecuteAsync: + async def execute(self, statement): + return connection.execute(statement) + + db = ExecuteAsync() + asyncio.run(prune_tokens(db, user_id, now)) + for table, retained in ( + (refresh, {"other-user", "live-child", "expired-ancestor"}), + (access, {"other-user", "live-access"}), + ): + tokens = set(connection.scalars(sa.select(table.c.token))) + assert retained <= tokens + assert len(tokens - retained) == 1 # only 100 spent rows per call + asyncio.run(prune_tokens(db, user_id, now)) + assert set(connection.scalars(sa.select(refresh.c.token))) == { + "other-user", + "live-child", + "expired-ancestor", + } + assert set(connection.scalars(sa.select(access.c.token))) == { + "other-user", + "live-access", + } + finally: + engine.dispose() + + +def test_users_token_pair_persists_only_digests_with_sync_sqlite(): + from users.auth_local.token_api import _create_token_pair + + engine = sa.create_engine("sqlite://") + metadata = sa.MetaData() + users = sa.Table("users_user", metadata, sa.Column("id", GUID(), primary_key=True)) + refresh = RefreshToken.__table__.to_metadata(metadata) + access = UserAccessToken.__table__.to_metadata(metadata) + user_id = uuid.uuid4() + try: + metadata.create_all(engine) + with Session(engine) as session: + session.execute(users.insert().values(id=user_id)) + + pair = asyncio.run( + _create_token_pair( + _SyncSession(session), + user_id, + SimpleNamespace( + bearer_token_lifetime_seconds=900, refresh_token_lifetime_seconds=3600 + ), + ) + ) + for table, secret in ((access, pair.access_token), (refresh, pair.refresh_token)): + stored = session.scalar(sa.select(table.c.token)) + assert stored == hashlib.sha256(secret.encode()).hexdigest() + finally: + engine.dispose() + + +class _SyncSession: + """Run real SQL through async interfaces without the sandbox's aiosqlite hang.""" + + def __init__(self, session): + self.session = session + + def add(self, row): + self.session.add(row) + + async def flush(self): + self.session.flush() + + async def execute(self, statement): + return self.session.execute(statement) + + async def commit(self): + self.session.commit() + + async def refresh(self, row): + self.session.refresh(row) + + async def delete(self, row): + self.session.delete(row) + + +def test_users_cookie_strategy_hashes_prunes_and_destroys_with_sync_sqlite(): + from users.backend import build_strategy + from users.db_adapter import PruningAccessTokenDatabase + + engine = sa.create_engine("sqlite://") + metadata = sa.MetaData() + users = sa.Table("users_user", metadata, sa.Column("id", GUID(), primary_key=True)) + RefreshToken.__table__.to_metadata(metadata) + access = UserAccessToken.__table__.to_metadata(metadata) + user_id = uuid.uuid4() + try: + metadata.create_all(engine) + with Session(engine) as session: + session.execute(users.insert().values(id=user_id)) + session.add( + UserAccessToken(token="expired", user_id=user_id, expires_at=datetime.now(UTC)) + ) + session.commit() + database = PruningAccessTokenDatabase(_SyncSession(session), UserAccessToken) + strategy = build_strategy(database, 900) + secret = asyncio.run( + strategy.write_token(SimpleNamespace(id=user_id, session_version=2)) + ) + digest = hashlib.sha256(secret.encode()).hexdigest() + assert list(session.scalars(sa.select(access.c.token))) == [digest] + assert asyncio.run(database.get_by_token(secret)) is None + assert asyncio.run(database.get_by_token(digest)).session_version == 2 + asyncio.run(strategy.destroy_token(digest, SimpleNamespace(id=user_id))) + assert session.scalar(sa.select(sa.func.count()).select_from(access)) == 1 + asyncio.run(strategy.destroy_token(secret, SimpleNamespace(id=user_id))) + assert session.scalar(sa.select(sa.func.count()).select_from(access)) == 0 + finally: + engine.dispose() diff --git a/modules/users/tests/test_users_token_storage.py b/modules/users/tests/test_users_token_storage.py new file mode 100644 index 000000000..e93929256 --- /dev/null +++ b/modules/users/tests/test_users_token_storage.py @@ -0,0 +1,104 @@ +"""Bearer secrets stay out of storage and spent credentials are pruned (#626).""" + +import hashlib +from datetime import UTC, datetime, timedelta + +import pytest +from sqlalchemy import select +from users.models import RefreshToken, UserAccessToken + + +async def _login(client): + response = await client.post( + "/api/users/auth/token", + json={"email": "admin@example.com", "password": "AdminPass1!"}, + ) + assert response.status_code == 200, response.text + return response.json() + + +@pytest.mark.anyio +async def test_users_pair_stores_digests_and_accepts_only_presented_secrets(anon_client, users_db): + pair = await _login(anon_client) + for model, key in ((UserAccessToken, "access_token"), (RefreshToken, "refresh_token")): + row = (await users_db.execute(select(model))).scalar_one() + assert row.token == hashlib.sha256(pair[key].encode()).hexdigest() + assert ( + await anon_client.get( + "/admin/users/", headers={"Authorization": f"Bearer {pair['access_token']}"} + ) + ).status_code == 200 + digest = hashlib.sha256(pair["access_token"].encode()).hexdigest() + assert ( + await anon_client.get("/admin/users/", headers={"Authorization": f"Bearer {digest}"}) + ).status_code in (302, 401) + rotated = await anon_client.post( + "/api/users/auth/token/refresh", json={"refresh_token": pair["refresh_token"]} + ) + assert rotated.status_code == 200 + + +@pytest.mark.anyio +async def test_users_cookie_token_is_hashed_and_logout_deletes_it(anon_client, users_db): + response = await anon_client.post( + "/api/users/auth/login", + data={"username": "admin@example.com", "password": "AdminPass1!"}, + ) + assert response.status_code == 204 + secret = anon_client.cookies.get("sm_auth") + row = (await users_db.execute(select(UserAccessToken))).scalar_one() + assert row.token == hashlib.sha256(secret.encode()).hexdigest() + assert (await anon_client.get("/api/users/me")).status_code == 200 + row.expires_at = datetime.now(UTC) - timedelta(seconds=1) + await users_db.commit() + assert ( + await anon_client.post( + "/api/users/auth/login", + data={"username": "admin@example.com", "password": "AdminPass1!"}, + ) + ).status_code == 204 + assert await users_db.get(UserAccessToken, row.token, populate_existing=True) is None + assert (await anon_client.post("/api/users/auth/logout")).status_code == 204 + assert (await users_db.execute(select(UserAccessToken))).scalars().all() == [] + + +@pytest.mark.anyio +async def test_users_login_prunes_expired_and_revoked_inactive_families(anon_client, users_db): + await _login(anon_client) + refresh = (await users_db.execute(select(RefreshToken))).scalar_one() + access = (await users_db.execute(select(UserAccessToken))).scalar_one() + refresh.revoked_at = datetime.now(UTC) + access.expires_at = datetime.now(UTC) - timedelta(seconds=1) + old_refresh, old_access = refresh.token, access.token + await users_db.commit() + await _login(anon_client) + assert await users_db.get(RefreshToken, old_refresh, populate_existing=True) is None + assert await users_db.get(UserAccessToken, old_access, populate_existing=True) is None + + +@pytest.mark.anyio +async def test_users_pruning_keeps_reuse_evidence_even_for_expired_ancestors( + anon_client, + users_db, +): + pair = await _login(anon_client) + child = await anon_client.post( + "/api/users/auth/token/refresh", json={"refresh_token": pair["refresh_token"]} + ) + assert child.status_code == 200 + ancestor = await users_db.get( + RefreshToken, hashlib.sha256(pair["refresh_token"].encode()).hexdigest() + ) + assert ancestor is not None + ancestor.expires_at = datetime.now(UTC) - timedelta(seconds=1) + await users_db.commit() + await _login(anon_client) # cleanup must preserve this family's reuse evidence + replay = await anon_client.post( + "/api/users/auth/token/refresh", json={"refresh_token": pair["refresh_token"]} + ) + assert replay.status_code == 401 + assert ( + await anon_client.post( + "/api/users/auth/token/refresh", json={"refresh_token": child.json()["refresh_token"]} + ) + ).status_code == 401 diff --git a/modules/users/users/auth_local/api.py b/modules/users/users/auth_local/api.py index 0f77c672b..b9b875867 100644 --- a/modules/users/users/auth_local/api.py +++ b/modules/users/users/auth_local/api.py @@ -48,6 +48,7 @@ get_user_manager, ) from users.manager import UserManager +from users.token_storage import revoke_access_token logger = logging.getLogger(__name__) router = APIRouter() @@ -165,10 +166,11 @@ async def login( @router.post("/auth/logout", status_code=204) -async def api_logout(request: Request): - """API logout — clears both the access-token cookie and the session.""" +async def api_logout(request: Request, db: AsyncSession = Depends(get_db)): + """API logout — revokes the access token, clears its cookie and the session.""" request.session.clear() cookie_name = request.app.state.users.settings.cookie_name + await revoke_access_token(db, request.cookies.get(cookie_name)) response = Response(status_code=204) response.delete_cookie(cookie_name, path="/") return response diff --git a/modules/users/users/auth_local/token_api.py b/modules/users/users/auth_local/token_api.py index bc084f545..7181b1016 100644 --- a/modules/users/users/auth_local/token_api.py +++ b/modules/users/users/auth_local/token_api.py @@ -28,6 +28,7 @@ from users.demo.guard import DEMO_READ_ONLY_DETAIL, is_read_only_demo from users.models import User from users.models.refresh_token import RefreshToken +from users.token_storage import hash_token, prune_tokens # Pre-computed bcrypt hash used when a login attempt targets a non-existent # user. Running verify against this takes the same time as a real check, @@ -140,7 +141,7 @@ async def token_refresh( stmt = ( update(RefreshToken) .where( - col(RefreshToken.token) == token_uuid, + col(RefreshToken.token) == hash_token(str(token_uuid)), col(RefreshToken.revoked_at).is_(None), col(RefreshToken.expires_at) > now, ) @@ -150,7 +151,9 @@ async def token_refresh( rt = (await db.execute(stmt)).scalar_one_or_none() if rt is None: reused = ( - await db.execute(select(RefreshToken).where(col(RefreshToken.token) == token_uuid)) + await db.execute( + select(RefreshToken).where(col(RefreshToken.token) == hash_token(str(token_uuid))) + ) ).scalar_one_or_none() if reused is not None and reused.revoked_at is not None: await db.execute( @@ -192,7 +195,7 @@ async def token_revoke( except (ValueError, TypeError): raise HTTPException(status_code=400, detail="Invalid token format") from None - stmt = select(RefreshToken).where(col(RefreshToken.token) == token_uuid) + stmt = select(RefreshToken).where(col(RefreshToken.token) == hash_token(str(token_uuid))) rt = (await db.execute(stmt)).scalar_one_or_none() if rt and rt.revoked_at is None: rt.revoked_at = datetime.now(UTC) @@ -219,8 +222,10 @@ async def _create_token_pair( now = datetime.now(UTC) + access_secret = str(uuid_mod.uuid4()) + refresh_secret = str(uuid_mod.uuid4()) access_token = UserAccessToken( - token=str(uuid_mod.uuid4()), + token=hash_token(access_secret), user_id=user_id, created_at=now, expires_at=now + timedelta(seconds=settings.bearer_token_lifetime_seconds), @@ -229,7 +234,7 @@ async def _create_token_pair( db.add(access_token) refresh = RefreshToken( - token=uuid_mod.uuid4(), + token=hash_token(refresh_secret), family_id=family_id or uuid_mod.uuid4(), user_id=user_id, created_at=now, @@ -237,10 +242,11 @@ async def _create_token_pair( ) db.add(refresh) await db.flush() + await prune_tokens(db, user_id, now) return TokenResponse( - access_token=access_token.token, - refresh_token=str(refresh.token), + access_token=access_secret, + refresh_token=refresh_secret, token_type="Bearer", expires_in=settings.bearer_token_lifetime_seconds, ) diff --git a/modules/users/users/auth_local/views.py b/modules/users/users/auth_local/views.py index 56ba1fb2b..c39d9bc66 100644 --- a/modules/users/users/auth_local/views.py +++ b/modules/users/users/auth_local/views.py @@ -20,6 +20,7 @@ from users.mailer import mailer_delivers from users.manager import UserManager, get_user_manager from users.models import User +from users.token_storage import revoke_access_token router = APIRouter() @@ -116,11 +117,12 @@ async def login_page(request: Request, inertia: InertiaDep) -> InertiaResponse: @router.post("/logout", response_model=None) -async def logout(request: Request) -> RedirectResponse: +async def logout(request: Request, db: AsyncSession = Depends(get_db)) -> RedirectResponse: """POST-only to resist cross-site `` logout attacks — the menu's logout link submits this as an Inertia form.""" request.session.clear() cookie_name = request.app.state.users.settings.cookie_name + await revoke_access_token(db, request.cookies.get(cookie_name)) # 303 forces the follow-up to GET — Inertia treats the redirect as a full # navigation rather than replaying the POST. response = RedirectResponse("/", status_code=303) diff --git a/modules/users/users/db_adapter.py b/modules/users/users/db_adapter.py index 909897efe..a41d97a1a 100644 --- a/modules/users/users/db_adapter.py +++ b/modules/users/users/db_adapter.py @@ -3,6 +3,7 @@ from __future__ import annotations from collections.abc import AsyncGenerator +from datetime import UTC, datetime from fastapi import Depends from fastapi_users_db_sqlalchemy import SQLAlchemyUserDatabase @@ -13,6 +14,7 @@ from sqlalchemy.orm import noload, selectinload from users.models import OAuthAccount, User, UserAccessToken +from users.token_storage import prune_tokens class UserDatabaseWithRoles(SQLAlchemyUserDatabase): @@ -58,7 +60,15 @@ async def get_user_db( yield UserDatabaseWithRoles(session, User, OAuthAccount) +class PruningAccessTokenDatabase(SQLAlchemyAccessTokenDatabase[UserAccessToken]): + """Clean up spent credentials when a cookie login mints an access token.""" + + async def create(self, create_dict): + await prune_tokens(self.session, create_dict["user_id"], datetime.now(UTC)) + return await super().create(create_dict) + + async def get_access_token_db( session: AsyncSession = Depends(get_db), ) -> AsyncGenerator[SQLAlchemyAccessTokenDatabase[UserAccessToken], None]: - yield SQLAlchemyAccessTokenDatabase(session, UserAccessToken) + yield PruningAccessTokenDatabase(session, UserAccessToken) diff --git a/modules/users/users/models/access_token.py b/modules/users/users/models/access_token.py index ff74e70ee..2161234a0 100644 --- a/modules/users/users/models/access_token.py +++ b/modules/users/users/models/access_token.py @@ -32,7 +32,7 @@ class UserAccessToken(Base, table=True): # ty: ignore[unsupported-base] __tablename__ = "users_access_token" - token: str = Field(max_length=43, primary_key=True) + token: str = Field(max_length=64, primary_key=True) created_at: datetime = Field( default_factory=now_utc, sa_type=TIMESTAMPAware(timezone=True), diff --git a/modules/users/users/models/refresh_token.py b/modules/users/users/models/refresh_token.py index dd8827172..34f77f774 100644 --- a/modules/users/users/models/refresh_token.py +++ b/modules/users/users/models/refresh_token.py @@ -16,7 +16,7 @@ class RefreshToken(Base, table=True): # ty: ignore[unsupported-base] __tablename__ = "users_refresh_token" - token: uuid_mod.UUID = Field(default_factory=uuid_mod.uuid4, primary_key=True) + token: str = Field(primary_key=True, max_length=64) # ``sa_type=GUID`` to match ``User.id``, not SQLModel's default ``Uuid``. # The two render identically on Postgres (UUID) but not on SQLite, where # ``Uuid`` stores the bare hex and ``GUID`` the dashed form — so the FK diff --git a/modules/users/users/token_storage.py b/modules/users/users/token_storage.py new file mode 100644 index 000000000..885dae5bd --- /dev/null +++ b/modules/users/users/token_storage.py @@ -0,0 +1,50 @@ +"""Hash bearer secrets and bound opportunistic credential cleanup.""" + +import hashlib +import uuid +from datetime import datetime + +from sqlalchemy import delete, or_, select +from sqlalchemy.ext.asyncio import AsyncSession +from sqlmodel import col + +from users.models import RefreshToken, UserAccessToken + +_PRUNE_BATCH_SIZE = 100 + + +def hash_token(secret: str) -> str: + return hashlib.sha256(secret.encode()).hexdigest() + + +async def revoke_access_token(db: AsyncSession, secret: str | None) -> None: + """Delete the access-token row behind a presented cookie, so logging out + ends that credential server-side instead of leaving it valid until expiry.""" + if secret: + digest = hash_token(secret) + await db.execute(delete(UserAccessToken).where(col(UserAccessToken.token) == digest)) + + +async def prune_tokens(db: AsyncSession, user_id: uuid.UUID, now: datetime) -> None: + """Prune this user's spent rows, preserving active families' reuse evidence.""" + active_families = select(RefreshToken.family_id).where( + col(RefreshToken.user_id) == user_id, + col(RefreshToken.revoked_at).is_(None), + col(RefreshToken.expires_at) > now, + ) + refresh_ids = ( + select(RefreshToken.token) + .where( + col(RefreshToken.user_id) == user_id, + or_(col(RefreshToken.expires_at) <= now, col(RefreshToken.revoked_at).is_not(None)), + col(RefreshToken.family_id).not_in(active_families), + ) + .limit(_PRUNE_BATCH_SIZE) + ) + await db.execute(delete(RefreshToken).where(col(RefreshToken.token).in_(refresh_ids))) + access_ids = ( + select(UserAccessToken.token) + .where(col(UserAccessToken.user_id) == user_id, col(UserAccessToken.expires_at) <= now) + .limit(_PRUNE_BATCH_SIZE) + ) + await db.execute(delete(UserAccessToken).where(col(UserAccessToken.token).in_(access_ids))) diff --git a/modules/users/users/token_strategy.py b/modules/users/users/token_strategy.py index e871e2676..ecd5a38fe 100644 --- a/modules/users/users/token_strategy.py +++ b/modules/users/users/token_strategy.py @@ -30,6 +30,7 @@ from sqlmodel import col from users.models import UserAccessToken +from users.token_storage import hash_token logger = logging.getLogger(__name__) @@ -77,6 +78,16 @@ def _create_access_token_dict(self, user: models.UP) -> dict[str, Any]: data["session_version"] = int(getattr(user, "session_version", 0) or 0) return data + async def write_token(self, user: models.UP) -> str: + data = self._create_access_token_dict(user) + secret = data["token"] + data["token"] = hash_token(secret) + await self.database.create(data) + return secret + + async def destroy_token(self, token: str, user: models.UP) -> None: + await super().destroy_token(hash_token(token), user) + async def read_token( self, token: str | None, user_manager: BaseUserManager[models.UP, models.ID] ) -> models.UP | None: @@ -93,7 +104,7 @@ async def read_token( if self.lifetime_seconds: max_age = datetime.now(UTC) - timedelta(seconds=self.lifetime_seconds) - access_token = await self.database.get_by_token(token, max_age) + access_token = await self.database.get_by_token(hash_token(token), max_age) if access_token is None: return None @@ -136,7 +147,7 @@ async def resolve_bearer(scope, token: str) -> UserContext | None: now = datetime.now(UTC) cutoff = now - timedelta(seconds=_TOKEN_LIFETIME_SECONDS) stmt = select(UserAccessToken).where( - col(UserAccessToken.token) == token, + col(UserAccessToken.token) == hash_token(token), col(UserAccessToken.created_at) > cutoff, col(UserAccessToken.expires_at) > now, ) From da7f125828561ac454ec4a36ec6d44fe21cc08f5 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 19:15:16 +0200 Subject: [PATCH 3/6] Clean up tenant memberships, shares and grants on user deletion; publish user events after commit (#627) UserDeleted, UserDisabled and RoleAssigned are published durably (or inline in the request transaction without background_tasks); ordinary subscribers run only after commit. tenants, sharing and permissions purge the deleted user's memberships, shares and direct grants; a tenant left without an owner is suspended. RoleAssigned fires only for newly added roles, and set_roles now reloads roles with populate_existing so a user already in the identity map no longer comes back with an empty roles list. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 4 + modules/permissions/permissions/module.py | 8 + .../permissions/permissions/user_cleanup.py | 17 ++ modules/sharing/sharing/module.py | 9 + modules/sharing/sharing/user_cleanup.py | 25 +++ modules/tenants/tenants/module.py | 9 + modules/tenants/tenants/user_cleanup.py | 52 +++++ .../users/tests/test_users_deletion_events.py | 196 ++++++++++++++++++ modules/users/users/admin/api.py | 17 +- modules/users/users/admin/events.py | 11 + modules/users/users/admin/queries.py | 2 + modules/users/users/admin/service.py | 9 +- 12 files changed, 350 insertions(+), 9 deletions(-) create mode 100644 modules/permissions/permissions/user_cleanup.py create mode 100644 modules/sharing/sharing/user_cleanup.py create mode 100644 modules/tenants/tenants/user_cleanup.py create mode 100644 modules/users/tests/test_users_deletion_events.py create mode 100644 modules/users/users/admin/events.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 297885143..7b02c7d91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,10 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] ### Fixed +- 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 + an owner are suspended. Existing roles no longer emit `RoleAssigned` (#627). - Database: discard `on_commit` callbacks registered inside a rolled-back savepoint, including released child savepoints, while preserving callbacks from surviving work (#631). - Role-permission edits, module settings updates/clears and system/tenant feature-flag diff --git a/modules/permissions/permissions/module.py b/modules/permissions/permissions/module.py index 76bf2cdd2..928fc81d3 100644 --- a/modules/permissions/permissions/module.py +++ b/modules/permissions/permissions/module.py @@ -17,6 +17,7 @@ if TYPE_CHECKING: from fastapi import FastAPI + from simple_module_core.events import EventBus from simple_module_core.invalidation import InvalidationBus from sqlalchemy.ext.asyncio import AsyncSession @@ -50,6 +51,13 @@ class PermissionsModule(ModuleBase): i18n_audience="admin", ) + def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) -> None: + from users.contracts.events import UserDeleted + + from permissions.user_cleanup import purge_user_grants + + bus.subscribe_durable(UserDeleted, purge_user_grants) + def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: from permissions.endpoints.api import router as api from permissions.endpoints.views import router as views diff --git a/modules/permissions/permissions/user_cleanup.py b/modules/permissions/permissions/user_cleanup.py new file mode 100644 index 000000000..820a7105d --- /dev/null +++ b/modules/permissions/permissions/user_cleanup.py @@ -0,0 +1,17 @@ +"""Revoke a deleted user's direct permission grants.""" + +from simple_module_core.durable_events import DurableContext +from simple_module_core.invalidation import Invalidation +from simple_module_db.callbacks import register_on_commit +from sqlmodel import col, delete +from users.contracts.events import UserDeleted + +from permissions.grants import CHANNEL, apply_invalidation +from permissions.models import UserPermission + + +async def purge_user_grants(event: UserDeleted, ctx: DurableContext) -> None: + await ctx.db.execute(delete(UserPermission).where(col(UserPermission.user_id) == event.user_id)) + register_on_commit( + ctx.db, lambda: apply_invalidation(Invalidation(channel=CHANNEL, key=str(event.user_id))) + ) diff --git a/modules/sharing/sharing/module.py b/modules/sharing/sharing/module.py index c1c8c4e94..52ec02b93 100644 --- a/modules/sharing/sharing/module.py +++ b/modules/sharing/sharing/module.py @@ -3,6 +3,7 @@ from __future__ import annotations import importlib.resources +import importlib.util from pathlib import Path from typing import TYPE_CHECKING @@ -35,6 +36,14 @@ def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) -> bus.subscribe_durable(TenantDeleted, purge_tenant_data) + # Users is optional: Keycloak-only installations have no local users package. + if importlib.util.find_spec("users") is not None: + from users.contracts.events import UserDeleted + + from sharing.user_cleanup import purge_user_shares + + bus.subscribe_durable(UserDeleted, purge_user_shares) + def register_settings(self, app: FastAPI) -> None: from sharing.services import SharingServices from sharing.settings import SharingSettings diff --git a/modules/sharing/sharing/user_cleanup.py b/modules/sharing/sharing/user_cleanup.py new file mode 100644 index 000000000..455642870 --- /dev/null +++ b/modules/sharing/sharing/user_cleanup.py @@ -0,0 +1,25 @@ +"""Revoke shares addressed to a deleted principal across tenants.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from simple_module_core.durable_events import DurableContext +from simple_module_db import all_tenants +from sqlmodel import col, delete + +from sharing.constants import SUBJECT_USER +from sharing.models import ResourceShare + +if TYPE_CHECKING: + from users.contracts.events import UserDeleted + + +async def purge_user_shares(event: UserDeleted, ctx: DurableContext) -> None: + with all_tenants(): + await ctx.db.execute( + delete(ResourceShare).where( + col(ResourceShare.subject_type) == SUBJECT_USER, + col(ResourceShare.subject_id) == str(event.user_id), + ) + ) diff --git a/modules/tenants/tenants/module.py b/modules/tenants/tenants/module.py index c857c1a01..08e672c5d 100644 --- a/modules/tenants/tenants/module.py +++ b/modules/tenants/tenants/module.py @@ -10,6 +10,7 @@ import importlib import importlib.resources +import importlib.util import logging from pathlib import Path from typing import TYPE_CHECKING, Any @@ -87,6 +88,14 @@ def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) -> InvitationCreated, send_invitation_email, name=c.INVITATION_MAIL_HANDLER ) + # Users is optional: Keycloak-only installations have no local users package. + if importlib.util.find_spec("users") is not None: + from users.contracts.events import UserDeleted + + from tenants.user_cleanup import purge_user_memberships + + bus.subscribe_durable(UserDeleted, purge_user_memberships) + def register_settings(self, app: FastAPI) -> None: from simple_module_hosting.shared_props import register_inertia_shared_provider diff --git a/modules/tenants/tenants/user_cleanup.py b/modules/tenants/tenants/user_cleanup.py new file mode 100644 index 000000000..082dc0cc9 --- /dev/null +++ b/modules/tenants/tenants/user_cleanup.py @@ -0,0 +1,52 @@ +"""Remove a deleted principal from every organisation (#627).""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from simple_module_core.durable_events import DurableContext +from simple_module_db.callbacks import register_on_commit +from sqlmodel import col, delete, select + +from tenants.constants import MembershipRole, TenantStatus +from tenants.models import Membership, Tenant +from tenants.resolver import forget + +if TYPE_CHECKING: + from users.contracts.events import UserDeleted + + +async def purge_user_memberships(event: UserDeleted, ctx: DurableContext) -> None: + user_id = str(event.user_id) + # Use the same tenant locks as membership edits; lock in a stable order. + tenants = ( + ( + await ctx.db.execute( + select(Tenant) + .where( + col(Tenant.id).in_( + select(Membership.tenant_id).where( + col(Membership.user_id) == user_id, + Membership.role == MembershipRole.OWNER, + ) + ) + ) + .order_by(Tenant.id) + .with_for_update() + ) + ) + .scalars() + .all() + ) + await ctx.db.execute(delete(Membership).where(col(Membership.user_id) == user_id)) + for tenant in tenants: + owner = await ctx.db.scalar( + select(Membership.id) + .where(Membership.tenant_id == tenant.id, Membership.role == MembershipRole.OWNER) + .limit(1) + ) + if owner is None: + # Flag the orphan for platform administration without granting ownership. + tenant.status = TenantStatus.SUSPENDED + await ctx.db.flush() + register_on_commit(ctx.db, lambda: forget(None)) diff --git a/modules/users/tests/test_users_deletion_events.py b/modules/users/tests/test_users_deletion_events.py new file mode 100644 index 000000000..17c47cf30 --- /dev/null +++ b/modules/users/tests/test_users_deletion_events.py @@ -0,0 +1,196 @@ +"""User lifecycle events and cross-module cleanup are transactional (#627).""" + +import uuid + +import pytest +from background_tasks.models import OutboxMessage +from background_tasks.outbox_transport import _OutboxWriter +from permissions.models import UserPermission +from permissions.module import PermissionsModule +from sharing.constants import SUBJECT_USER +from sharing.models import ResourceShare +from sharing.module import SharingModule +from simple_module_core.durable_events import event_key +from simple_module_core.events import EventBus +from simple_module_db import all_tenants, tenant_context +from simple_module_db.transaction import finalize_session, rollback_session +from sqlmodel import select +from starlette.requests import Request +from tenants.constants import MembershipRole, TenantStatus +from tenants.models import Membership, Tenant +from tenants.module import TenantsModule +from tenants.service import TenantService +from users.admin.api import admin_delete_user, admin_disable_user, admin_set_roles +from users.admin.service import UserService +from users.contracts.events import RoleAssigned, UserDeleted, UserDisabled +from users.contracts.schemas import RoleAssignment +from users.models import Role, User, UserRole + + +class PausedUserTransport(_OutboxWriter): + def schedule(self, db, ids): + pass + + +async def record_user_event(event, ctx): + pass + + +def cleanup_bus(durable=False): + bus = EventBus() + for module in (TenantsModule(), SharingModule(), PermissionsModule()): + module.register_event_handlers(bus) + if durable: + bus.durable_transport = PausedUserTransport() + return bus + + +def request(): + return Request({"type": "http"}) + + +async def seed_cleanup(db, user_id, other_id): + db.add(User(id=user_id, email="deleted@example.com", hashed_password="unused")) + for tenant_id in ("sole", "coowned", "unrelated"): + db.add(Tenant(id=tenant_id, slug=tenant_id, name=tenant_id)) + await db.flush() + for tenant_id in ("sole", "coowned"): + db.add( + Membership( + tenant_id=tenant_id, + user_id=str(user_id), + role=MembershipRole.OWNER, + email="deleted@example.com", + ) + ) + for tenant_id in ("coowned", "unrelated"): + db.add(Membership(tenant_id=tenant_id, user_id=str(other_id), role=MembershipRole.OWNER)) + for tenant_id in ("sole", "coowned"): + with tenant_context(tenant_id): + for subject_id in (str(user_id), str(other_id)): + db.add( + ResourceShare( + resource_type="test.note", + resource_id="1", + subject_type=SUBJECT_USER, + subject_id=subject_id, + role="view", + granted_by=str(other_id), + ) + ) + await db.flush() + for uid in (user_id, other_id): + db.add(UserPermission(user_id=uid, permission_key="test.view")) + await finalize_session(db) + + +@pytest.mark.parametrize("rollback", [False, True]) +async def test_user_deleted_purges_all_tenants_and_grants(db_session, rollback): + user_id, other_id = uuid.uuid4(), uuid.uuid4() + await seed_cleanup(db_session, user_id, other_id) + bus = cleanup_bus() + # A deletion made while acting in one tenant must still clean every tenant. + with tenant_context("sole"): + await admin_delete_user(user_id, request(), bus, UserService(db_session, None), db_session) + assert not await TenantService(db_session).has_member_email("sole", "deleted@example.com") + assert (await db_session.get(Tenant, "sole")).status == TenantStatus.SUSPENDED + assert (await db_session.get(Tenant, "coowned")).status == TenantStatus.ACTIVE + assert (await db_session.get(Tenant, "unrelated")).status == TenantStatus.ACTIVE + with all_tenants(): + shares = (await db_session.execute(select(ResourceShare))).scalars().all() + assert len(shares) == 2 + assert {s.subject_id for s in shares} == {str(other_id)} + assert (await db_session.execute(select(UserPermission.user_id))).scalars().all() == [other_id] + # Re-delivery is harmless, including when the user no longer exists. + await bus.publish_durable_or_inline(UserDeleted(user_id), db_session) + if rollback: + await rollback_session(db_session) + assert await db_session.get(User, user_id) is not None + assert (await db_session.get(Tenant, "sole")).status == TenantStatus.ACTIVE + assert len((await db_session.execute(select(Membership))).scalars().all()) == 4 + with all_tenants(): + assert len((await db_session.execute(select(ResourceShare))).scalars().all()) == 4 + assert len((await db_session.execute(select(UserPermission))).scalars().all()) == 2 + else: + await finalize_session(db_session) + assert await db_session.get(User, user_id) is None + + +@pytest.mark.parametrize( + "operation,event_type", + [("delete", UserDeleted), ("disable", UserDisabled), ("roles", RoleAssigned)], +) +async def test_admin_events_outbox_rolls_back_with_user_write(db_session, operation, event_type): + uid = uuid.uuid4() + db_session.add(User(id=uid, email="events@example.com", hashed_password="unused")) + db_session.add(Role(name="new-role")) + await finalize_session(db_session) + bus = cleanup_bus(durable=True) + bus.subscribe_durable(event_type, record_user_event) + observed = [] + + async def observe(event): + observed.append(event) + + bus.subscribe(event_type, observe) + service = UserService(db_session, None) + if operation == "delete": + await admin_delete_user(uid, request(), bus, service, db_session) + elif operation == "disable": + await admin_disable_user(uid, bus, service, db_session) + else: + await admin_set_roles( + uid, RoleAssignment(role_names=["new-role"]), request(), bus, service, db_session + ) + rows = (await db_session.execute(select(OutboxMessage))).scalars().all() + assert rows + assert {row.event_key for row in rows} == {event_key(event_type)} + assert observed == [] + await rollback_session(db_session) + assert (await db_session.execute(select(OutboxMessage))).scalars().all() == [] + user = await db_session.get(User, uid) + assert user is not None and user.is_active + assert (await db_session.execute(select(UserRole))).scalars().all() == [] + assert observed == [] + + +async def test_role_assigned_only_for_new_resolved_roles(db_session): + uid = uuid.uuid4() + user = User(id=uid, email="roles@example.com", hashed_password="unused") + held, added = Role(name="held"), Role(name="added") + db_session.add_all([user, held, added]) + await db_session.flush() + db_session.add(UserRole(user_id=uid, role_id=held.id)) + await finalize_session(db_session) + bus = EventBus() + bus.durable_transport = PausedUserTransport() + bus.subscribe_durable(RoleAssigned, record_user_event) + for _ in range(2): + await admin_set_roles( + uid, + RoleAssignment(role_names=["held", "added", "added", "missing"]), + request(), + bus, + UserService(db_session, None), + db_session, + ) + rows = (await db_session.execute(select(OutboxMessage))).scalars().all() + assert [row.payload["role_name"] for row in rows] == ["added"] + + +async def test_ordinary_user_events_wait_for_commit(db_session): + from users.admin.events import publish_admin_event + + bus = EventBus() + observed = [] + + async def observe(event): + observed.append(event.user_id) + + bus.subscribe(UserDisabled, observe) + uid = uuid.uuid4() + db_session.add(User(id=uid, email="committed@example.com", hashed_password="unused")) + await publish_admin_event(bus, UserDisabled(uid), db_session) + assert observed == [] + await finalize_session(db_session) + assert observed == [uid] diff --git a/modules/users/users/admin/api.py b/modules/users/users/admin/api.py index 590069a09..30046a9c5 100644 --- a/modules/users/users/admin/api.py +++ b/modules/users/users/admin/api.py @@ -8,10 +8,13 @@ from fastapi import status as http_status from fastapi_users import exceptions as fa_exceptions from simple_module_core.events import EventBus +from simple_module_db.deps import get_db from simple_module_db.pagination import PageParams from simple_module_hosting.pagination import page_params from simple_module_hosting.permissions import RequiresPermission +from sqlalchemy.ext.asyncio import AsyncSession +from users.admin.events import publish_admin_event from users.admin.links import admin_link_base from users.admin.service import UserService from users.constants import PERM_USERS_MANAGE, sanitize_list_filters @@ -164,6 +167,7 @@ async def admin_delete_user( request: Request, bus: EventBus = Depends(get_event_bus), service: UserService = Depends(get_user_service), + db: AsyncSession = Depends(get_db), ): """Hard-delete a user. An admin cannot delete their own account.""" actor = getattr(request.state, "user", None) @@ -176,7 +180,7 @@ async def admin_delete_user( await service.delete_user(user_id) except UserNotFoundError: raise HTTPException(status_code=404, detail="User not found") from None - await bus.publish(UserDeleted(user_id=user_id)) + await publish_admin_event(bus, UserDeleted(user_id=user_id), db) return Response(status_code=http_status.HTTP_204_NO_CONTENT) @@ -185,13 +189,14 @@ async def admin_disable_user( user_id: uuid.UUID, bus: EventBus = Depends(get_event_bus), service: UserService = Depends(get_user_service), + db: AsyncSession = Depends(get_db), ): """Disable a user account (sets is_active=False and disabled_at).""" try: user = await service.disable(user_id) except UserNotFoundError: raise HTTPException(status_code=404, detail="User not found") from None - await bus.publish(UserDisabled(user_id=user.id)) + await publish_admin_event(bus, UserDisabled(user_id=user.id), db) return service.to_list_item(user) @@ -228,10 +233,13 @@ async def admin_set_roles( request: Request, bus: EventBus = Depends(get_event_bus), service: UserService = Depends(get_user_service), + db: AsyncSession = Depends(get_db), ): """Replace a user's role assignments.""" assigned_by = getattr(request.state, "user", None) try: + previous = await service.get_with_roles(user_id) + previous_roles = {r.name for r in previous.roles} if previous else set() user = await service.set_roles( user_id, data.role_names, @@ -239,8 +247,9 @@ async def admin_set_roles( ) except UserNotFoundError: raise HTTPException(status_code=404, detail="User not found") from None - for role in data.role_names: - await bus.publish(RoleAssigned(user_id=user.id, role_name=role)) + for role in user.roles: + if role.name not in previous_roles: + await publish_admin_event(bus, RoleAssigned(user_id=user.id, role_name=role.name), db) return service.to_list_item(user) diff --git a/modules/users/users/admin/events.py b/modules/users/users/admin/events.py new file mode 100644 index 000000000..28b7e6008 --- /dev/null +++ b/modules/users/users/admin/events.py @@ -0,0 +1,11 @@ +"""Transactional publication of admin lifecycle events.""" + +from simple_module_core.events import Event, EventBus +from simple_module_db.callbacks import register_on_commit +from sqlalchemy.ext.asyncio import AsyncSession + + +async def publish_admin_event(bus: EventBus, event: Event, db: AsyncSession) -> None: + await bus.publish_durable_or_inline(event, db) + # Preserve ordinary subscribers, but only after the user write commits. + register_on_commit(db, lambda: bus.publish(event)) diff --git a/modules/users/users/admin/queries.py b/modules/users/users/admin/queries.py index 28e447041..dbf02afb2 100644 --- a/modules/users/users/admin/queries.py +++ b/modules/users/users/admin/queries.py @@ -115,6 +115,8 @@ async def list_roles(self) -> list[RoleListItem]: ] async def _get_user_with_roles(self, user_id: uuid.UUID) -> User | None: + # populate_existing: ``roles`` is ``noload``, so an instance already in + # the identity map would otherwise keep its stale (or empty) collection. result = await self._db.execute( select(User) .where(col(User.id) == user_id) diff --git a/modules/users/users/admin/service.py b/modules/users/users/admin/service.py index 0d5ecbb6b..686deb108 100644 --- a/modules/users/users/admin/service.py +++ b/modules/users/users/admin/service.py @@ -108,9 +108,9 @@ async def delete_user(self, user_id: uuid.UUID) -> None: """Hard-delete a user and all of its dependent rows. Every child table is cleared with an explicit bulk ``DELETE`` so the - result is deterministic and identical on Postgres and SQLite (SQLite - only honours FK cascade with a per-connection pragma we don't set, and - ``RefreshToken`` has no DB cascade at all). + result is deterministic and identical on Postgres and SQLite, including + tables such as ``RefreshToken`` without DB cascade. SQLite foreign keys + are enabled by the framework. Other modules clean up via ``UserDeleted``. The user is loaded with ``roles`` and ``oauth_accounts`` forced to ``noload``. ``noload`` returns an empty collection *without* emitting a @@ -282,8 +282,7 @@ async def set_roles( await self._revoke_sessions(user) await self._db.flush() - await self._db.refresh(user, attribute_names=["roles"]) - return user + return await self._require_user(user_id) async def generate_reset_link(self, user_id: uuid.UUID, base_url: str) -> str: """Build an admin-copyable password-reset URL. No email side-effect. From 24897942f16cc0809745f6d1a7ee19af6c6a0a77 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 19:18:48 +0200 Subject: [PATCH 4/6] refactor(optimize): hash refresh token once, use session.scalar(s) helpers --- modules/tenants/tenants/user_cleanup.py | 22 ++++----------------- modules/users/users/auth_local/token_api.py | 9 +++------ modules/users/users/token_storage.py | 5 +++-- 3 files changed, 10 insertions(+), 26 deletions(-) diff --git a/modules/tenants/tenants/user_cleanup.py b/modules/tenants/tenants/user_cleanup.py index 082dc0cc9..79edced06 100644 --- a/modules/tenants/tenants/user_cleanup.py +++ b/modules/tenants/tenants/user_cleanup.py @@ -19,25 +19,11 @@ async def purge_user_memberships(event: UserDeleted, ctx: DurableContext) -> None: user_id = str(event.user_id) # Use the same tenant locks as membership edits; lock in a stable order. - tenants = ( - ( - await ctx.db.execute( - select(Tenant) - .where( - col(Tenant.id).in_( - select(Membership.tenant_id).where( - col(Membership.user_id) == user_id, - Membership.role == MembershipRole.OWNER, - ) - ) - ) - .order_by(Tenant.id) - .with_for_update() - ) - ) - .scalars() - .all() + owned = select(Membership.tenant_id).where( + col(Membership.user_id) == user_id, Membership.role == MembershipRole.OWNER ) + stmt = select(Tenant).where(col(Tenant.id).in_(owned)).order_by(Tenant.id).with_for_update() + tenants = (await ctx.db.scalars(stmt)).all() await ctx.db.execute(delete(Membership).where(col(Membership.user_id) == user_id)) for tenant in tenants: owner = await ctx.db.scalar( diff --git a/modules/users/users/auth_local/token_api.py b/modules/users/users/auth_local/token_api.py index 7181b1016..235a7f22d 100644 --- a/modules/users/users/auth_local/token_api.py +++ b/modules/users/users/auth_local/token_api.py @@ -138,10 +138,11 @@ async def token_refresh( raise HTTPException(status_code=401, detail="Invalid refresh token") from None now = datetime.now(UTC) + digest = hash_token(str(token_uuid)) stmt = ( update(RefreshToken) .where( - col(RefreshToken.token) == hash_token(str(token_uuid)), + col(RefreshToken.token) == digest, col(RefreshToken.revoked_at).is_(None), col(RefreshToken.expires_at) > now, ) @@ -150,11 +151,7 @@ async def token_refresh( ) rt = (await db.execute(stmt)).scalar_one_or_none() if rt is None: - reused = ( - await db.execute( - select(RefreshToken).where(col(RefreshToken.token) == hash_token(str(token_uuid))) - ) - ).scalar_one_or_none() + reused = await db.scalar(select(RefreshToken).where(col(RefreshToken.token) == digest)) if reused is not None and reused.revoked_at is not None: await db.execute( update(RefreshToken) diff --git a/modules/users/users/token_storage.py b/modules/users/users/token_storage.py index 885dae5bd..e2afeaecd 100644 --- a/modules/users/users/token_storage.py +++ b/modules/users/users/token_storage.py @@ -21,8 +21,9 @@ async def revoke_access_token(db: AsyncSession, secret: str | None) -> None: """Delete the access-token row behind a presented cookie, so logging out ends that credential server-side instead of leaving it valid until expiry.""" if secret: - digest = hash_token(secret) - await db.execute(delete(UserAccessToken).where(col(UserAccessToken.token) == digest)) + await db.execute( + delete(UserAccessToken).where(col(UserAccessToken.token) == hash_token(secret)) + ) async def prune_tokens(db: AsyncSession, user_id: uuid.UUID, now: datetime) -> None: From 3f22746ee730d70b90bbffc3fe4160b8e10cc8c7 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 19:29:31 +0200 Subject: [PATCH 5/6] fix: address code review findings (round 1, pass 1) UserDeleted moves to simple_module_core.user_events so tenants/sharing subscribe without importing users; docs describe hashed tokens and the users migration label. --- CHANGELOG.md | 2 ++ CLAUDE.md | 3 ++- docs/modules/users.md | 10 +++++----- framework/core/simple_module_core/user_events.py | 16 ++++++++++++++++ modules/permissions/permissions/module.py | 2 +- modules/permissions/permissions/user_cleanup.py | 2 +- modules/sharing/sharing/module.py | 9 +++------ modules/sharing/sharing/user_cleanup.py | 2 +- modules/tenants/tenants/module.py | 9 +++------ modules/tenants/tenants/user_cleanup.py | 2 +- modules/users/users/contracts/events.py | 15 ++++++++++----- 11 files changed, 45 insertions(+), 27 deletions(-) create mode 100644 framework/core/simple_module_core/user_events.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b02c7d91..b58962012 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ All notable changes to this project are documented in this file. The format is b the durable outbox (inline cleanup without background tasks). Deletion purges tenant memberships, shares to the user and direct permission grants; tenants left without an owner are suspended. Existing roles no longer emit `RoleAssigned` (#627). + `UserDeleted` now lives in `simple_module_core.user_events` (still re-exported from + `users.contracts.events`) so modules can subscribe without depending on `users`. - Database: discard `on_commit` callbacks registered inside a rolled-back savepoint, including released child savepoints, while preserving callbacks from surviving work (#631). - Role-permission edits, module settings updates/clears and system/tenant feature-flag diff --git a/CLAUDE.md b/CLAUDE.md index 67c4af721..265690d07 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -329,7 +329,8 @@ development and testing (`session_cookie_secure`). included. Known mislabels (existing files are left alone, applied databases stay valid): `51b2058d8afd` puts `api_tokens` on the trunk, so users, file_storage and background_tasks revisions report it, and keycloak's - `168a2882f443` branches off the users revision `873ca2015033`. Give an + `168a2882f443` branches off the users revision `873ca2015033`. The users + trunk also carries a `users` label since `a625f0b7c912`. Give an unlabelled module a label on its next revision. `host/tests/test_migration_graph.py` pins the roots, heads and owners; update it with any new root or head. `docs/database/migrations.md`. diff --git a/docs/modules/users.md b/docs/modules/users.md index 679506316..4f04a1645 100644 --- a/docs/modules/users.md +++ b/docs/modules/users.md @@ -36,7 +36,7 @@ The module is built on [`fastapi-users`](https://fastapi-users.github.io/) for p | `POST /api/users/auth/accept-invite` | `AcceptInviteRequest` | sets password + signs the user in | | `POST /api/users/auth/demo/{role}` | — | one-click sign-in as a shared demo account (`role` ∈ `admin`, `user`); `404` unless that account is configured; rate-limited per role — see [Demo mode](#demo-mode-hosting-a-showcase-instance) | | `POST /api/users/auth/token` | `TokenRequest` (email + password) | bearer login for mobile / API clients → `{access_token, refresh_token, token_type, expires_in}`; `401` for bad credentials and external/SSO users; `429` + `Retry-After` while locked — see [Login lockout](#login-lockout) | -| `POST /api/users/auth/token/refresh` | `RefreshRequest` (refresh_token) | rotates a refresh token into a new pair (old one revoked) | +| `POST /api/users/auth/token/refresh` | `RefreshRequest` (refresh_token) | rotates a refresh token into a new pair (old one revoked atomically; reuse of a revoked token revokes its family) | | `DELETE /api/users/auth/token` | `RefreshRequest` (refresh_token) | revokes a refresh token (idempotent) | | `GET /api/users/auth/{provider}/login` | — | OAuth: redirect to the IdP (`provider` ∈ configured set) | | `GET /api/users/auth/{provider}/callback` | `?code=&state=` | OAuth: find-or-create user, set cookie, 303 to `login_redirect_url` | @@ -161,11 +161,11 @@ permissions role pages. The edit page's *Recent activity* card reads the core `UserRole` (table `users_user_role`) — composite-PK join table with `assigned_at`, `assigned_by`. The `(user_id, role_id)` PK is user-id-first; a separate index covers reverse lookups by `role_id`. -`UserAccessToken` (table `users_access_token`) — bearer API tokens; resolved by `UsersAuthProvider` on `Authorization: Bearer` requests (sessions remain the primary browser auth). +`UserAccessToken` (table `users_access_token`) — bearer API tokens, stored as SHA-256 digests (the secret is shown once at login); resolved by `UsersAuthProvider` on `Authorization: Bearer` requests (sessions remain the primary browser auth). `OAuthAccount` (table `users_oauth_account`) — linked OAuth/OIDC accounts: `oauth_name`, `account_id`, `account_email`, `access_token`, `refresh_token`, `expires_at`. -`RefreshToken` (table `users_refresh_token`) — opaque refresh tokens for the bearer flow: `token` (PK), `user_id`, `created_at`, `expires_at`, `revoked_at`. +`RefreshToken` (table `users_refresh_token`) — opaque refresh tokens for the bearer flow: `token` (PK, SHA-256 digest of the secret), `family_id` (rotation lineage; presenting an already-revoked token revokes the whole family), `user_id`, `created_at`, `expires_at`, `revoked_at`. Two pre-seeded roles get fixed UUIDs so other modules can reference them safely: @@ -242,7 +242,7 @@ Everything else is DB-backed (initial values are pydantic defaults; edit under U | `UserRegistered` | `user_id`, `email` | on signup | | `UserInvited` | `user_id`, `email`, `invited_by` | on admin invite | | `UserCreated` | `user_id`, `email`, `created_by` | on admin create (`POST /api/users/admin`) | -| `UserDeleted` | `user_id` | on admin delete | +| `UserDeleted` | `user_id` | on admin delete; durable (outbox, or inline in the request transaction). Defined in `simple_module_core.user_events` so modules subscribe without depending on `users` | | `UserDisabled` | `user_id` | on admin disable | | `RoleAssigned` | `user_id`, `role_name` | once per role on `PUT /admin/{user_id}/roles` | @@ -446,7 +446,7 @@ In the admin UI, the user list and edit page show an **"External · SSO"** badge `UsersAuthProvider` (registered on `app.state.auth.auth_provider` in `register_settings`) is the `AuthProvider` the auth module's `AuthMiddleware` delegates to. On each request `resolve_user`: -- For a `Bearer` token, looks the token up in `users_access_token` and returns the active user. +- For a `Bearer` token, looks the token's SHA-256 digest up in `users_access_token` and returns the active user. - Otherwise reads `session["user_id"]`, returns the cached context from `session["user_ctx"]` on a hit, else loads the User row with eagerly-loaded roles, builds a `UserContext`, caches it in `session["user_ctx"]`, and clears stale session keys when the user is missing/disabled. `get_login_url()` → `/users/login`, `get_logout_url()` → `/users/logout`. `UsersModule.register_public_routes` declares these prefixes anonymous (in addition to the framework defaults `/health`, `/static/`, `/api/docs`, `/api/redoc`, `/api/openapi.json` (only when served), `/i18n/`, `/`): diff --git a/framework/core/simple_module_core/user_events.py b/framework/core/simple_module_core/user_events.py new file mode 100644 index 000000000..5cd478eea --- /dev/null +++ b/framework/core/simple_module_core/user_events.py @@ -0,0 +1,16 @@ +"""User lifecycle contracts shared without a dependency on the users module.""" + +import uuid +from dataclasses import dataclass + +from simple_module_core.events import Event + + +@dataclass +class UserDeleted(Event): + """The account is gone; owners of per-user rows must durably remove them. + + Delivery is at least once, so handlers must be idempotent. + """ + + user_id: uuid.UUID diff --git a/modules/permissions/permissions/module.py b/modules/permissions/permissions/module.py index 928fc81d3..d5be09c31 100644 --- a/modules/permissions/permissions/module.py +++ b/modules/permissions/permissions/module.py @@ -52,7 +52,7 @@ class PermissionsModule(ModuleBase): ) def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) -> None: - from users.contracts.events import UserDeleted + from simple_module_core.user_events import UserDeleted from permissions.user_cleanup import purge_user_grants diff --git a/modules/permissions/permissions/user_cleanup.py b/modules/permissions/permissions/user_cleanup.py index 820a7105d..ce67e07ad 100644 --- a/modules/permissions/permissions/user_cleanup.py +++ b/modules/permissions/permissions/user_cleanup.py @@ -2,9 +2,9 @@ from simple_module_core.durable_events import DurableContext from simple_module_core.invalidation import Invalidation +from simple_module_core.user_events import UserDeleted from simple_module_db.callbacks import register_on_commit from sqlmodel import col, delete -from users.contracts.events import UserDeleted from permissions.grants import CHANNEL, apply_invalidation from permissions.models import UserPermission diff --git a/modules/sharing/sharing/module.py b/modules/sharing/sharing/module.py index 52ec02b93..99e2b8469 100644 --- a/modules/sharing/sharing/module.py +++ b/modules/sharing/sharing/module.py @@ -3,7 +3,6 @@ from __future__ import annotations import importlib.resources -import importlib.util from pathlib import Path from typing import TYPE_CHECKING @@ -36,13 +35,11 @@ def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) -> bus.subscribe_durable(TenantDeleted, purge_tenant_data) - # Users is optional: Keycloak-only installations have no local users package. - if importlib.util.find_spec("users") is not None: - from users.contracts.events import UserDeleted + from simple_module_core.user_events import UserDeleted - from sharing.user_cleanup import purge_user_shares + from sharing.user_cleanup import purge_user_shares - bus.subscribe_durable(UserDeleted, purge_user_shares) + bus.subscribe_durable(UserDeleted, purge_user_shares) def register_settings(self, app: FastAPI) -> None: from sharing.services import SharingServices diff --git a/modules/sharing/sharing/user_cleanup.py b/modules/sharing/sharing/user_cleanup.py index 455642870..3626161e9 100644 --- a/modules/sharing/sharing/user_cleanup.py +++ b/modules/sharing/sharing/user_cleanup.py @@ -12,7 +12,7 @@ from sharing.models import ResourceShare if TYPE_CHECKING: - from users.contracts.events import UserDeleted + from simple_module_core.user_events import UserDeleted async def purge_user_shares(event: UserDeleted, ctx: DurableContext) -> None: diff --git a/modules/tenants/tenants/module.py b/modules/tenants/tenants/module.py index 08e672c5d..422576cc7 100644 --- a/modules/tenants/tenants/module.py +++ b/modules/tenants/tenants/module.py @@ -10,7 +10,6 @@ import importlib import importlib.resources -import importlib.util import logging from pathlib import Path from typing import TYPE_CHECKING, Any @@ -88,13 +87,11 @@ def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) -> InvitationCreated, send_invitation_email, name=c.INVITATION_MAIL_HANDLER ) - # Users is optional: Keycloak-only installations have no local users package. - if importlib.util.find_spec("users") is not None: - from users.contracts.events import UserDeleted + from simple_module_core.user_events import UserDeleted - from tenants.user_cleanup import purge_user_memberships + from tenants.user_cleanup import purge_user_memberships - bus.subscribe_durable(UserDeleted, purge_user_memberships) + bus.subscribe_durable(UserDeleted, purge_user_memberships) def register_settings(self, app: FastAPI) -> None: from simple_module_hosting.shared_props import register_inertia_shared_provider diff --git a/modules/tenants/tenants/user_cleanup.py b/modules/tenants/tenants/user_cleanup.py index 79edced06..33bedf285 100644 --- a/modules/tenants/tenants/user_cleanup.py +++ b/modules/tenants/tenants/user_cleanup.py @@ -13,7 +13,7 @@ from tenants.resolver import forget if TYPE_CHECKING: - from users.contracts.events import UserDeleted + from simple_module_core.user_events import UserDeleted async def purge_user_memberships(event: UserDeleted, ctx: DurableContext) -> None: diff --git a/modules/users/users/contracts/events.py b/modules/users/users/contracts/events.py index 4dc4d3796..6c5e427a1 100644 --- a/modules/users/users/contracts/events.py +++ b/modules/users/users/contracts/events.py @@ -6,6 +6,16 @@ from dataclasses import dataclass from simple_module_core.events import Event +from simple_module_core.user_events import UserDeleted + +__all__ = [ + "RoleAssigned", + "UserCreated", + "UserDeleted", + "UserDisabled", + "UserInvited", + "UserRegistered", +] @dataclass @@ -28,11 +38,6 @@ class UserCreated(Event): created_by: str | None -@dataclass -class UserDeleted(Event): - user_id: uuid.UUID - - @dataclass class UserDisabled(Event): user_id: uuid.UUID From d9fbcdb2dc0a15eda18739115fc2dd0ccea40662 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 19:39:02 +0200 Subject: [PATCH 6/6] chore: regenerate users api types --- modules/users/users/pages/api.generated.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/users/users/pages/api.generated.ts b/modules/users/users/pages/api.generated.ts index 9b7410ca4..bc65a7ebd 100644 --- a/modules/users/users/pages/api.generated.ts +++ b/modules/users/users/pages/api.generated.ts @@ -38,7 +38,7 @@ export interface paths { put?: never; /** * Api Logout - * @description API logout — clears both the access-token cookie and the session. + * @description API logout — revokes the access token, clears its cookie and the session. */ post: operations["users_api_logout"]; delete?: never;