From 276f4d61c51c3141c578e8bb3484f3e20d39950d Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 13:02:43 +0200 Subject: [PATCH 1/2] feat(users): retire legacy users_user.tenant_id (#381) UserContext.from_user no longer copies a tenant from the user row, UserRead drops tenant_id, and an Alembic migration drops the column and its index. Adds a regression test that membership add/remove/role change/switch apply on the next request without re-login. Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- docs/framework/multi-tenancy.md | 4 ++ .../diagnostics/_tenancy.py | 4 +- ...4e8a1b6c392_users_drop_legacy_tenant_id.py | 36 +++++++++++ modules/auth/auth/contracts/schemas.py | 6 +- modules/auth/tests/test_user_context.py | 4 +- .../tenants/tests/test_membership_changes.py | 64 +++++++++++++++++++ modules/users/tests/_middleware_support.py | 2 - modules/users/tests/test_user_model.py | 1 - modules/users/tests/test_users_middleware.py | 1 - modules/users/users/contracts/schemas.py | 1 - modules/users/users/models/user.py | 1 - tests/loadtest/seed.py | 1 - 12 files changed, 112 insertions(+), 13 deletions(-) create mode 100644 host/migrations/versions/d4e8a1b6c392_users_drop_legacy_tenant_id.py create mode 100644 modules/tenants/tests/test_membership_changes.py diff --git a/docs/framework/multi-tenancy.md b/docs/framework/multi-tenancy.md index 68500038..b1110f3f 100644 --- a/docs/framework/multi-tenancy.md +++ b/docs/framework/multi-tenancy.md @@ -159,6 +159,10 @@ for anonymous visitors, on public routes), the tenant header (members only), then the session's choice validated against a membership (#363). Without one it falls back to the principal's `tenant_id` claim, and for **anonymous** requests only, the configured `tenant_header`. An authenticated user can never pick a tenant by header. +The `users` module's `User` is platform-global and has no `tenant_id` +column (dropped in #381): a user belongs to tenants only through +`tenants_membership`, and a membership change applies on their next request, +no re-login needed. Most auth providers set no `tenant_id` claim, so `multi_tenant` with no resolver fails every tenant-scoped query closed; the boot reports that as `SM025`. diff --git a/framework/core/simple_module_core/diagnostics/_tenancy.py b/framework/core/simple_module_core/diagnostics/_tenancy.py index b2052cba..8132fe4d 100644 --- a/framework/core/simple_module_core/diagnostics/_tenancy.py +++ b/framework/core/simple_module_core/diagnostics/_tenancy.py @@ -9,8 +9,8 @@ Duck-typed on SQLAlchemy ``Table`` objects (core does not depend on SQLAlchemy). Only tables of models that inherit ``MultiTenantMixin`` count: a -plain ``tenant_id`` column (``users_user``'s legacy one, the ``tenants`` -registry's own tables) carries no isolation and no per-tenant key rule. +plain ``tenant_id`` column (the ``tenants`` registry's own tables) carries no +isolation and no per-tenant key rule. SM025: ``multi_tenant`` is on but no module registered ``app.state.tenant_resolver`` — see :func:`check_tenant_resolver`. diff --git a/host/migrations/versions/d4e8a1b6c392_users_drop_legacy_tenant_id.py b/host/migrations/versions/d4e8a1b6c392_users_drop_legacy_tenant_id.py new file mode 100644 index 00000000..96ac5f26 --- /dev/null +++ b/host/migrations/versions/d4e8a1b6c392_users_drop_legacy_tenant_id.py @@ -0,0 +1,36 @@ +"""users_user: drop the legacy tenant_id column + +Tenant membership lives in ``tenants_membership`` and the active tenant is +resolved per request, so the copy on the user row was dead weight that could +only go stale. ``User`` stays platform-global. See GH #381. + +Revision ID: d4e8a1b6c392 +Revises: c3a1d7e45f20 +Create Date: 2026-10-01 10:00:00.000000 +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "d4e8a1b6c392" +down_revision: str | None = "c3a1d7e45f20" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +_INDEX = "ix_users_user_tenant_id" + + +def upgrade() -> None: + # batch mode: SQLite cannot drop an indexed column in place. + with op.batch_alter_table("users_user") as batch: + batch.drop_index(_INDEX) + batch.drop_column("tenant_id") + + +def downgrade() -> None: + with op.batch_alter_table("users_user") as batch: + batch.add_column(sa.Column("tenant_id", sa.String(length=50), nullable=True)) + batch.create_index(_INDEX, ["tenant_id"], unique=False) diff --git a/modules/auth/auth/contracts/schemas.py b/modules/auth/auth/contracts/schemas.py index 34b7b0c1..b6d0187f 100644 --- a/modules/auth/auth/contracts/schemas.py +++ b/modules/auth/auth/contracts/schemas.py @@ -26,7 +26,10 @@ def from_user(cls, user: User | Any) -> UserContext: """Build a UserContext from a users.models.User with eagerly-loaded roles. Duck-typed to avoid importing users.models at runtime — any object - exposing .id, .email, .full_name, .roles[*].name, .tenant_id works. + exposing .id, .email, .full_name, .roles[*].name works. The user row + carries no tenant: the active tenant is resolved per request from + ``tenants_membership``, so ``tenant_id`` stays ``None`` here (only a + generic provider's claim path sets it). The caller is responsible for eager-loading roles (selectinload). """ return cls( @@ -34,7 +37,6 @@ def from_user(cls, user: User | Any) -> UserContext: email=user.email, name=user.full_name or user.email, roles=[r.name for r in user.roles], - tenant_id=user.tenant_id, ) def to_session_dict(self) -> dict[str, Any]: diff --git a/modules/auth/tests/test_user_context.py b/modules/auth/tests/test_user_context.py index 00b238fd..cb6df56b 100644 --- a/modules/auth/tests/test_user_context.py +++ b/modules/auth/tests/test_user_context.py @@ -18,14 +18,14 @@ async def test_from_user_basic(self): email="charlie@example.com", full_name="Charlie Brown", roles=[role_a, role_b], - tenant_id="tenant-42", ) ctx = UserContext.from_user(fake_user) assert ctx.id == "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" assert ctx.email == "charlie@example.com" assert ctx.name == "Charlie Brown" assert ctx.roles == ["admin", "editor"] - assert ctx.tenant_id == "tenant-42" + # The user row carries no tenant; the resolver supplies it per request. + assert ctx.tenant_id is None async def test_from_user_name_fallback_to_email(self): """When full_name is None, ctx.name falls back to the user's email.""" diff --git a/modules/tenants/tests/test_membership_changes.py b/modules/tenants/tests/test_membership_changes.py new file mode 100644 index 00000000..e17729af --- /dev/null +++ b/modules/tenants/tests/test_membership_changes.py @@ -0,0 +1,64 @@ +"""A membership change reaches the user's NEXT request, with no re-login (#381, #362). + +``users_user.tenant_id`` used to be copied into the session at login, so moving +a user between tenants only took effect after they signed in again. The active +tenant now comes from ``tenants_membership`` on every request; these tests +keep the member's signed session cookie fixed throughout and change membership +underneath it. +""" + +from __future__ import annotations + + +async def test_removal_takes_effect_on_the_next_request(tenant_client): + async with ( + tenant_client() as owner, + tenant_client("member", tenant_id=owner.tenant_id) as member, + ): + assert (await member.client.get("/api/tenants/current/members")).status_code == 200 + + removed = await owner.client.delete(f"/api/tenants/current/members/{member.user_id}") + assert removed.status_code == 204 + + assert (await member.client.get("/api/tenants/current/members")).status_code != 200 + assert (await member.client.get("/api/tenants/")).json() == [] + + +async def test_role_change_takes_effect_on_the_next_request(tenant_client): + async with ( + tenant_client() as owner, + tenant_client("member", tenant_id=owner.tenant_id) as member, + ): + invite = {"email": "new@example.com"} + before = await member.client.post("/api/tenants/current/invitations", json=invite) + assert before.status_code == 403 + + promoted = await owner.client.patch( + f"/api/tenants/current/members/{member.user_id}", json={"role": "admin"} + ) + assert promoted.status_code == 200 + after = await member.client.post("/api/tenants/current/invitations", json=invite) + assert after.status_code == 201 + + await owner.client.patch( + f"/api/tenants/current/members/{member.user_id}", json={"role": "member"} + ) + demoted = await member.client.post( + "/api/tenants/current/invitations", json={"email": "other@example.com"} + ) + assert demoted.status_code == 403 + + +async def test_joining_and_switching_tenants_needs_no_relogin(tenant_client): + async with tenant_client() as user: + first = user.tenant_id + created = await user.client.post("/api/tenants/", json={"name": "Second", "slug": "second"}) + assert created.status_code == 201 + second = created.json()["id"] + + mine = {t["id"] for t in (await user.client.get("/api/tenants/")).json()} + assert mine == {first, second} + + assert (await user.client.post(f"/api/tenants/{first}/switch")).status_code == 204 + members = (await user.client.get("/api/tenants/current/members")).json() + assert [m["user_id"] for m in members] == [user.user_id] diff --git a/modules/users/tests/_middleware_support.py b/modules/users/tests/_middleware_support.py index 0744746a..ac4cec89 100644 --- a/modules/users/tests/_middleware_support.py +++ b/modules/users/tests/_middleware_support.py @@ -49,7 +49,6 @@ async def _default_handler(request: Request): "email": user.email, "name": user.name, "roles": user.roles, - "tenant_id": user.tenant_id, } if user is not None else None @@ -106,7 +105,6 @@ async def mw_active_user(db_session, _mw_seed_roles): is_superuser=False, is_verified=True, full_name="Middleware Tester", - tenant_id="acme", ) link = UserRole(user_id=user_id, role_id=ADMIN_ROLE_ID) db_session.add_all([user, link]) diff --git a/modules/users/tests/test_user_model.py b/modules/users/tests/test_user_model.py index 5092d0c5..ff319b60 100644 --- a/modules/users/tests/test_user_model.py +++ b/modules/users/tests/test_user_model.py @@ -35,7 +35,6 @@ def test_required_columns(self): "is_superuser", "is_verified", "full_name", - "tenant_id", "disabled_at", "last_login_at", # AuditMixin columns diff --git a/modules/users/tests/test_users_middleware.py b/modules/users/tests/test_users_middleware.py index 43b77c6d..155f9739 100644 --- a/modules/users/tests/test_users_middleware.py +++ b/modules/users/tests/test_users_middleware.py @@ -81,7 +81,6 @@ async def test_authenticated_request_sets_user_context(db_state, mw_active_user) assert data["user"]["email"] == "middleware-test@example.com" assert data["user"]["name"] == "Middleware Tester" assert data["user"]["roles"] == ["admin"] - assert data["user"]["tenant_id"] == "acme" # --------------------------------------------------------------------------- diff --git a/modules/users/users/contracts/schemas.py b/modules/users/users/contracts/schemas.py index 754ad8a8..ea667166 100644 --- a/modules/users/users/contracts/schemas.py +++ b/modules/users/users/contracts/schemas.py @@ -47,7 +47,6 @@ class UserRead(CreateUpdateDictModel, SQLModel): is_verified: bool = False is_external: bool = False full_name: str | None = None - tenant_id: str | None = None disabled_at: datetime | None = None last_login_at: datetime | None = None diff --git a/modules/users/users/models/user.py b/modules/users/users/models/user.py index 3be186f7..347bcbae 100644 --- a/modules/users/users/models/user.py +++ b/modules/users/users/models/user.py @@ -56,7 +56,6 @@ class User(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] ) full_name: str | None = Field(default=None, max_length=255) - tenant_id: str | None = Field(default=None, max_length=50, index=True) disabled_at: datetime | None = Field( default=None, sa_type=DateTime(timezone=True), diff --git a/tests/loadtest/seed.py b/tests/loadtest/seed.py index 064292fc..78907f6a 100644 --- a/tests/loadtest/seed.py +++ b/tests/loadtest/seed.py @@ -110,7 +110,6 @@ async def main() -> None: "is_superuser": False, "is_verified": i % 3 != 0, "full_name": fake.name(), - "tenant_id": None, "disabled_at": NOW if disabled else None, "last_login_at": NOW - timedelta(days=i % 90) if i % 4 else None, "created_at": NOW - timedelta(days=i % 365), From b4ffe679c3d64784fc01fa6191c79a070f73f8b0 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 14:54:57 +0200 Subject: [PATCH 2/2] fix(users): ignore the tenant_id cached in pre-upgrade session cookies (review of #381) Cookies signed before the users_user.tenant_id column was retired still carry it inside user_ctx, and the cached path returned it verbatim. The users provider now drops it there; the active tenant comes from memberships only. Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- .../users/tests/test_legacy_cached_tenant.py | 54 +++++++++++++++++++ modules/users/users/provider.py | 3 ++ 2 files changed, 57 insertions(+) create mode 100644 modules/users/tests/test_legacy_cached_tenant.py diff --git a/modules/users/tests/test_legacy_cached_tenant.py b/modules/users/tests/test_legacy_cached_tenant.py new file mode 100644 index 00000000..c5d65bfd --- /dev/null +++ b/modules/users/tests/test_legacy_cached_tenant.py @@ -0,0 +1,54 @@ +"""A pre-#381 session cookie's cached ``tenant_id`` is ignored. + +``users_user.tenant_id`` is gone and the active tenant is resolved per request +from memberships, but cookies signed before the upgrade still carry the old +value inside ``user_ctx``. Honouring it would keep acting for a tenant the +user may have left. +""" + +from __future__ import annotations + +import time + +import pytest +from auth.contracts.schemas import UserContext +from simple_module_hosting.session import SESSION_EXPIRES_AT_KEY +from sqlalchemy import select +from starlette.requests import Request +from users.models import User +from users.provider import UsersAuthProvider + + +async def _admin_id(users_app) -> str: + async with users_app.state.sm.db.session_factory() as session: + stmt = select(User.id).where(User.email == "admin@example.com") + return str((await session.execute(stmt)).scalar_one()) + + +@pytest.mark.anyio +async def test_old_cookie_tenant_id_is_dropped_on_the_cached_path(users_app): + user_id = await _admin_id(users_app) + legacy_ctx = UserContext( + id=user_id, email="admin@example.com", name="Admin", roles=["admin"], tenant_id="acme" + ).to_session_dict() + session = { + "user_id": user_id, + "user_ctx": legacy_ctx, + "session_version": 0, + SESSION_EXPIRES_AT_KEY: int(time.time()) + 3600, + } + scope = { + "type": "http", + "method": "GET", + "path": "/", + "headers": [], + "app": users_app, + "session": session, + } + + ctx = await UsersAuthProvider().resolve_user(Request(scope)) + + assert ctx is not None + assert ctx.id == user_id + assert ctx.name == "Admin" # the cached context was used, not a DB reload + assert ctx.tenant_id is None diff --git a/modules/users/users/provider.py b/modules/users/users/provider.py index 354f4db7..5207a126 100644 --- a/modules/users/users/provider.py +++ b/modules/users/users/provider.py @@ -129,6 +129,9 @@ async def resolve_user(self, request: Request) -> UserContext | None: # sign out a browser this process has never seen — the alternative # is a button that only logs out the person pressing it. if await self._version_still_current(request.scope, user_uuid, session): + # Cookies minted before #381 still cache the retired user-row + # tenant; the active tenant now comes from memberships alone. + cached.tenant_id = None return cached _forget(session) return None