Tech-debt batch 15: users — atomic refresh rotation, hashed tokens, user-deletion cleanup (#625 #626 #627)

CHANGELOG.md

@@ -15,6 +15,12 @@ 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).
+  `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
@@ -55,6 +61,14 @@ 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
   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

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

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` |
+| `a626b1c8d023` | users (hashed bearer tokens) | `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 = ("<module_name>",)`. 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.
@@ -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

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/`, `/`):

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

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

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

host/tests/test_migration_graph.py

@@ -39,6 +39,8 @@
     "e878882924e7": "background_tasks",
     "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",
@@ -55,7 +57,7 @@
 # head revision -> module owning the tables it creates or alters.
 _HEADS = {
     "a7c2e91d4b58": "audit_log",
-    "1f5985daa10b": "background_tasks",  # also carries the inherited ``api_tokens``
+    "a626b1c8d023": "users",  # also carries ``api_tokens`` and ``background_tasks``
     "f1a9c3d7b2e4": "tenants",
     "f6a3b9d2e8c4": "keycloak",
     "f733c7938fd1": "sharing",
@@ -71,6 +73,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 +96,8 @@
     "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``
+    "a626b1c8d023": "api_tokens",  # also inherits ``background_tasks``
     "51b2058d8afd": "api_tokens",  # ancestors of a labelled revision inherit its label too
 }
 

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 simple_module_core.user_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

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_core.user_events import UserDeleted
+from simple_module_db.callbacks import register_on_commit
+from sqlmodel import col, delete
+
+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)))
+    )

modules/sharing/sharing/module.py

@@ -35,6 +35,12 @@ def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) ->
 
         bus.subscribe_durable(TenantDeleted, purge_tenant_data)
 
+        from simple_module_core.user_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

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 simple_module_core.user_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),
+            )
+        )

modules/tenants/tenants/module.py

@@ -87,6 +87,12 @@ def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) ->
             InvitationCreated, send_invitation_email, name=c.INVITATION_MAIL_HANDLER
         )
 
+        from simple_module_core.user_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
 

modules/tenants/tenants/user_cleanup.py

@@ -0,0 +1,38 @@
+"""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 simple_module_core.user_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.
+    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(
+            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))