From 3d6d2291d20c64a0f640f05b6d7e8be70eb47593 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 12:24:51 +0200 Subject: [PATCH 1/6] feat(tenancy): adoption prerequisites (#380) Groundwork so core modules can adopt MultiTenantMixin without breaking single-tenant installs or leaving tenant members without permissions. - DEFAULT_TENANT_ID = "default" (simple_module_db). With tenancy not strict and no tenant bound, MultiTenantMixin inserts (session.add, ORM/Core insert, multi-VALUES, and inside all_tenants()) are stamped with it instead of hitting the NOT NULL constraint. Strict mode is unchanged: an unbound insert still raises MissingTenantError. HostSettings.default_tenant is untouched. Reads stay unfiltered when unbound and not strict: on a single-tenant install every row belongs to the install whatever tenant_id it carries, and narrowing to "default" would hide rows written under default_tenant or while multi_tenant was on. - Tenant-role vocabulary in core (simple_module_core.tenancy): TenantRole (owner/admin/member), TENANT_ROLE_PREFIX, tenant_role(), is_tenant_role(). tenants re-exports them (MembershipRole is TenantRole). - tenant_client fixture in simple_module_test: factory tenant_client(role="owner", *, tenant_id=None, email=None) yielding TenantClient(client, tenant_id, user_id); lazily imports users/tenants. tenants tests use it where it was a straight swap. - SM025: multi_tenant on with no app.state.tenant_resolver, reported at boot after module registration (needs the built app, so not in the doctor CLI) in every environment, and folded into the Doctor screen's results. - tenants: depends_on uses _MODULE_* constants (ci-check-hardcoded-strings). - Docs: multi-tenancy (default tenant, roles, testing), diagnostic codes, CLAUDE.md. Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- CLAUDE.md | 4 +- docs/framework/multi-tenancy.md | 85 ++++++++++-- docs/reference/diagnostic-codes.md | 3 +- framework/core/README.md | 5 +- framework/core/simple_module_core/__init__.py | 5 + .../diagnostics/__init__.py | 2 + .../diagnostics/_tenancy.py | 37 +++++- framework/core/simple_module_core/tenancy.py | 49 +++++++ .../core/tests/test_diagnostics_tenancy.py | 22 ++++ framework/core/tests/test_tenant_roles.py | 33 +++++ framework/db/simple_module_db/__init__.py | 2 + framework/db/simple_module_db/flush_guard.py | 16 ++- framework/db/simple_module_db/insert_guard.py | 32 ++--- framework/db/simple_module_db/mixins.py | 6 +- framework/db/simple_module_db/query_filter.py | 8 +- framework/db/simple_module_db/tenancy.py | 15 ++- framework/db/tests/conftest.py | 20 ++- framework/db/tests/test_default_tenant.py | 86 ++++++++++++ framework/db/tests/test_multi_tenancy.py | 11 +- framework/db/tests/test_tenant_strict.py | 25 +--- .../simple_module_hosting/_boot_checks.py | 47 +++++++ .../simple_module_hosting/app_builder.py | 2 + .../tests/test_tenant_resolver_diagnostic.py | 47 +++++++ framework/testing/README.md | 1 + .../testing/simple_module_test/__init__.py | 5 + .../testing/simple_module_test/plugin.py | 1 + .../simple_module_test/tenant_client.py | 124 ++++++++++++++++++ modules/tenants/tenants/constants.py | 22 ++-- modules/tenants/tenants/module.py | 4 +- modules/tenants/tenants/resolver.py | 3 +- modules/tenants/tests/conftest.py | 41 ++---- modules/tenants/tests/test_invitations.py | 9 +- modules/tenants/tests/test_isolation_e2e.py | 31 ++--- .../tests/test_tenant_client_fixture.py | 35 +++++ modules/tenants/tests/test_tenants_api.py | 10 +- 35 files changed, 699 insertions(+), 149 deletions(-) create mode 100644 framework/core/simple_module_core/tenancy.py create mode 100644 framework/core/tests/test_tenant_roles.py create mode 100644 framework/db/tests/test_default_tenant.py create mode 100644 framework/hosting/simple_module_hosting/_boot_checks.py create mode 100644 framework/hosting/tests/test_tenant_resolver_diagnostic.py create mode 100644 framework/testing/simple_module_test/tenant_client.py create mode 100644 modules/tenants/tests/test_tenant_client_fixture.py diff --git a/CLAUDE.md b/CLAUDE.md index 7b414fa8..e34086f9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -82,7 +82,7 @@ hence `SM022`/`SM023`. See `docs/module-authoring.md` § Styling. **Database**: per-module `Base` via `create_module_base("")`. On SQLite `init_db` enables WAL, an explicit `busy_timeout`, and **`foreign_keys=ON`** — so both sides of an FK must use the same column type (`sa.Uuid` and fastapi-users' `GUID` are identical on Postgres but not on SQLite), and parents must be flushed before children. Every module owns its own `MetaData` (so Alembic autogenerate can attribute tables to a module), but all tables live in the host's single schema. `__tablename__` must be prefixed with the module name to avoid collisions (`orders_order`). Postgres and SQLite share the same layout. -Standard mixins in `simple_module_db.mixins`: `AuditMixin`, `SoftDeleteMixin` (bypass the read filter with `stmt.execution_options(include_deleted=True)`; purge by deleting an already-trashed row, or with `hard_delete(session, obj)`), `MultiTenantMixin`, `VersionedMixin`. The soft-delete/tenant filters cover every statement shape, not only selects that name the entity — joins, ORM subqueries, `select(func.count()).select_from(Model)`, and Core statements over `Model.__table__` (GH #332). **Tenancy fails closed**: with `multi_tenant` on, a query or insert on a `MultiTenantMixin` model with no `current_tenant_id` raises `TenantIsolationError` instead of reading every tenant; cross-tenant code says so with `all_tenants()` / `execution_options(all_tenants=True)`, and jobs/CLI act for one tenant with `tenant_context(id)`. Unique keys on such tables must include `tenant_id` (`SM024`). The `tenants` module owns organisations, memberships and `app.state.tenant_resolver`; tenant-level routes act on the *active* tenant, never a tenant id from the URL. See [docs/framework/multi-tenancy.md](docs/framework/multi-tenancy.md). The per-request session (`get_db`) auto-commits **only if** there are pending writes (via `after_flush` listener); otherwise rollback. Service code should **not** call `session.commit()` — flush if you need DB-assigned values. DML executed through the session (`session.execute(update(Model)...)`) counts as a write; a raw `text("UPDATE ...")` does not, and needs `mark_written(session)`. The commit fires in `CommitBeforeResponseMiddleware`, at the ASGI `http.response.start` message, so a client that creates a row and immediately reads it back in a second request sees it — FastAPI runs a `yield` dependency's exit code *after* the response is delivered, which used to make that a deterministic 404 (GH #257). `get_db` keeps the same commit in its own exit code as a fallback for when the middleware isn't in the stack; whichever runs first wins. +Standard mixins in `simple_module_db.mixins`: `AuditMixin`, `SoftDeleteMixin` (bypass the read filter with `stmt.execution_options(include_deleted=True)`; purge by deleting an already-trashed row, or with `hard_delete(session, obj)`), `MultiTenantMixin`, `VersionedMixin`. The soft-delete/tenant filters cover every statement shape, not only selects that name the entity — joins, ORM subqueries, `select(func.count()).select_from(Model)`, and Core statements over `Model.__table__` (GH #332). **Tenancy fails closed**: with `multi_tenant` on, a query or insert on a `MultiTenantMixin` model with no `current_tenant_id` raises `TenantIsolationError` instead of reading every tenant; cross-tenant code says so with `all_tenants()` / `execution_options(all_tenants=True)`, and jobs/CLI act for one tenant with `tenant_context(id)`. With `multi_tenant` off and no tenant bound, inserts are stamped `DEFAULT_TENANT_ID` (`"default"`, from `simple_module_db`) and reads stay unfiltered; adoption migrations backfill with the same constant. Tenant roles reach the principal as `tenant:` — map them with `tenant_role(TenantRole.MEMBER)` from `simple_module_core.tenancy`, never by importing `tenants`; tests use the `tenant_client(role)` fixture. Unique keys on such tables must include `tenant_id` (`SM024`). The `tenants` module owns organisations, memberships and `app.state.tenant_resolver`; tenant-level routes act on the *active* tenant, never a tenant id from the URL. See [docs/framework/multi-tenancy.md](docs/framework/multi-tenancy.md). The per-request session (`get_db`) auto-commits **only if** there are pending writes (via `after_flush` listener); otherwise rollback. Service code should **not** call `session.commit()` — flush if you need DB-assigned values. DML executed through the session (`session.execute(update(Model)...)`) counts as a write; a raw `text("UPDATE ...")` does not, and needs `mark_written(session)`. The commit fires in `CommitBeforeResponseMiddleware`, at the ASGI `http.response.start` message, so a client that creates a row and immediately reads it back in a second request sees it — FastAPI runs a `yield` dependency's exit code *after* the response is delivered, which used to make that a deterministic 404 (GH #257). `get_db` keeps the same commit in its own exit code as a fallback for when the middleware isn't in the stack; whichever runs first wins. **Migrations** live in `host/migrations/versions/` — not in module packages. `host/alembic/env.py` calls `build_module_metadata()` + `make_include_object()` so autogenerate covers every installed module and ignores host-owned tables. First migration of each module should set `branch_labels = ("",)` to enable per-module `downgrade @base`. @@ -110,7 +110,7 @@ Standard mixins in `simple_module_db.mixins`: `AuditMixin`, `SoftDeleteMixin` (b ## Diagnostic codes -Meaningful codes when reading `make doctor` output: `SM001` missing meta (error), `SM003` orphan page / `SM004` phantom render (warn), `SM007` module overrides no hooks (info), `SM008` duplicate name (error), `SM009` framework→plugin import (error), `SM010` DB revision behind head (error), `SM011` module table not in migration history (warn), `SM012` `register_settings` overridden but nothing on `app.state.` (warn, fires at dev boot only), `SM013`–`SM016` locale issues, `SM017` module ships `.tsx` pages but is missing `package.json`/`tsconfig.json` (warn), `SM018` Inertia `router.{post,patch,put,delete}()` in a page targets a JSON `/api/*` endpoint (warn — Inertia rejects non-Inertia responses), `SM019` module registers view routes (non-empty `view_prefix` + overrides `register_routes`) but overrides neither `register_menu_items` nor `register_permissions` (warn — pages exist with no sidebar entry and no role-editor visibility; admins can't reach them through the UI). Modules whose views are sub-pages of another module typically register permissions to stay discoverable in the role editor without needing their own sidebar entry. `SM020` multiple auth provider modules installed (error), `SM021` no auth provider module installed (warn), `SM022` `@theme`/`@custom-variant`/`@utility` in a module's `styles.css`, where `layer(components)` makes them inert (warn), `SM023` an unlayered rule in a module's `theme.css`, which outranks every Tailwind utility (warn). `SM024` a unique key on a `MultiTenantMixin` table that omits `tenant_id` (warn). In production, errors fail boot. +Meaningful codes when reading `make doctor` output: `SM001` missing meta (error), `SM003` orphan page / `SM004` phantom render (warn), `SM007` module overrides no hooks (info), `SM008` duplicate name (error), `SM009` framework→plugin import (error), `SM010` DB revision behind head (error), `SM011` module table not in migration history (warn), `SM012` `register_settings` overridden but nothing on `app.state.` (warn, fires at dev boot only), `SM013`–`SM016` locale issues, `SM017` module ships `.tsx` pages but is missing `package.json`/`tsconfig.json` (warn), `SM018` Inertia `router.{post,patch,put,delete}()` in a page targets a JSON `/api/*` endpoint (warn — Inertia rejects non-Inertia responses), `SM019` module registers view routes (non-empty `view_prefix` + overrides `register_routes`) but overrides neither `register_menu_items` nor `register_permissions` (warn — pages exist with no sidebar entry and no role-editor visibility; admins can't reach them through the UI). Modules whose views are sub-pages of another module typically register permissions to stay discoverable in the role editor without needing their own sidebar entry. `SM020` multiple auth provider modules installed (error), `SM021` no auth provider module installed (warn), `SM022` `@theme`/`@custom-variant`/`@utility` in a module's `styles.css`, where `layer(components)` makes them inert (warn), `SM023` an unlayered rule in a module's `theme.css`, which outranks every Tailwind utility (warn). `SM024` a unique key on a `MultiTenantMixin` table that omits `tenant_id` (warn). `SM025` `multi_tenant` is on but no module registered `app.state.tenant_resolver` (warn — checked at boot after module registration, in every environment, not by the `make doctor` CLI). In production, errors fail boot. ## Tests & fixtures diff --git a/docs/framework/multi-tenancy.md b/docs/framework/multi-tenancy.md index 9ce0f2cb..2e2b5c41 100644 --- a/docs/framework/multi-tenancy.md +++ b/docs/framework/multi-tenancy.md @@ -17,8 +17,8 @@ tenant a request acts for. |---|---|---|---| | `SELECT` | filtered to the tenant | `MissingTenantError` | unfiltered | | ORM `update()` / `delete()` | filtered to the tenant; `update().values(tenant_id=…)` raises | `MissingTenantError` | unfiltered | -| `session.add` + flush | `tenant_id` filled in; a different explicit value raises | `MissingTenantError` unless `tenant_id` is set explicitly | DB `NOT NULL` error unless set | -| ORM `insert(Model)` (bulk / `.values()`) | `tenant_id` filled in; a different explicit value raises | `MissingTenantError` unless every row sets `tenant_id` | DB `NOT NULL` error unless set | +| `session.add` + flush | `tenant_id` filled in; a different explicit value raises | `MissingTenantError` unless `tenant_id` is set explicitly | `tenant_id` filled in with `DEFAULT_TENANT_ID` unless set | +| ORM `insert(Model)` (bulk / `.values()`) | `tenant_id` filled in; a different explicit value raises | `MissingTenantError` unless every row sets `tenant_id` | `tenant_id` filled in with `DEFAULT_TENANT_ID` unless set | | Flushing a change to, or a delete of, a loaded object | only if it belongs to the bound tenant | `MissingTenantError` | allowed | | Changing `tenant_id` | raises | raises | raises (only an `all_tenants()` block may move a row) | @@ -103,12 +103,35 @@ the call, sync or async (#364). ## Single-tenant hosts -A host with `multi_tenant` off can still install modules whose tables use the -mixin: set `default_tenant` (a `HostSettings` field, e.g. `main`) and every -request, and every background task with no tenant on its message, acts as -that tenant (#359). CLI commands and scripts use -`tenant_context(settings.default_tenant)`. It is ignored when `multi_tenant` -is on — a multi-tenant install never falls back to a shared tenant. +A host with `multi_tenant` off can install modules whose tables use the mixin +with no configuration at all. With no tenant bound, an insert is stamped with +`DEFAULT_TENANT_ID` (`"default"`, exported by `simple_module_db`) — inside an +`all_tenants()` block too — and reads stay unfiltered, so the install behaves +as one tenant that owns every row (#380). A module adopting the mixin backfills +its existing rows with the same constant in its migration: + +```python +from simple_module_db import DEFAULT_TENANT_ID + +op.add_column("files_file", sa.Column("tenant_id", sa.String(50), nullable=True)) +op.execute(sa.text("UPDATE files_file SET tenant_id = :t").bindparams(t=DEFAULT_TENANT_ID)) +op.alter_column("files_file", "tenant_id", nullable=False) +``` + +Unbound reads are deliberately *not* narrowed to `DEFAULT_TENANT_ID`: on a +single-tenant install every row is the install's, whatever `tenant_id` it +carries — rows written under `default_tenant`, or while `multi_tenant` was +briefly on, must not vanish. Strict mode never uses the constant; it raises. + +To give the single tenant a name of your choosing instead, set +`default_tenant` (a `HostSettings` field, e.g. `main`): every request, and +every background task with no tenant on its message, then *binds* that tenant +(#359), and inserts are stamped with it rather than the constant. Reads are +then scoped to it as well, so rows already stamped `DEFAULT_TENANT_ID` need +re-stamping when an existing install adopts `default_tenant`. CLI commands +and scripts use `tenant_context(settings.default_tenant)`. It is ignored when +`multi_tenant` is on — a multi-tenant install never falls back to a shared +tenant. ## Background jobs @@ -136,6 +159,52 @@ 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. +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`. + +## Tenant roles + +A membership role — `owner`, `admin` or `member` — reaches the request +principal as `tenant:`, for the active tenant only, so a tenant `admin` +is never the platform `admin`. The vocabulary lives in core, so a module maps +these onto its own permissions without depending on `tenants`: + +```python +from simple_module_core.tenancy import TenantRole, tenant_role + + +def register_permissions(self, registry): + registry.map_role(tenant_role(TenantRole.MEMBER), ["files.view", "files.upload"]) + registry.map_role(tenant_role(TenantRole.ADMIN), ["files.manage"]) +``` + +`tenant_role()` rejects a name outside `TenantRole`, so a typo fails at boot +rather than granting nothing. `TENANT_ROLE_PREFIX` and `is_tenant_role()` are +there for code that inspects a principal's roles. A role maps only what it is +given — map `owner` and `admin` too if they should hold a member's +permissions. + +## Testing + +The `simple_module_test` plugin ships `tenant_client` (needs the `users` and +`tenants` modules): a factory yielding a client signed in as a fresh user with +`role` in a new tenant — or in `tenant_id=` — with that tenant active. + +```python +async def test_isolation(tenant_client): + async with tenant_client() as a, tenant_client("member") as b: + await a.client.post("/api/things", json={"name": "x"}) + assert (await b.client.get("/api/things")).json() == [] + + +async def test_same_tenant(tenant_client): + async with ( + tenant_client("owner") as (owner, tenant_id, _), + tenant_client("member", tenant_id=tenant_id) as (member, _, member_id), + ): + ... +``` ## Unique keys diff --git a/docs/reference/diagnostic-codes.md b/docs/reference/diagnostic-codes.md index fca4721c..7d60ffe3 100644 --- a/docs/reference/diagnostic-codes.md +++ b/docs/reference/diagnostic-codes.md @@ -33,6 +33,7 @@ The framework runs a set of static checks over installed modules at app boot. Th | `SM022` | WARNING | A module's `styles.css` contains a top-level `@theme`, `@custom-variant` or `@utility` block. That file is imported into `layer(components)`, where those at-rules are inert. | Move the block to the module's `theme.css`, which is imported unlayered so its tokens actually register. | | `SM023` | WARNING | A module's `theme.css` contains an unlayered plain rule (anything but an at-rule or a `:root`-style selector). Unlayered CSS outranks every Tailwind utility. | Move the rule to the module's `styles.css`, which is imported into `layer(components)` so utilities still win. | | `SM024` | WARNING | A unique column, constraint or index on a `MultiTenantMixin` table does not include `tenant_id`, so the first tenant to claim a value locks every other tenant out of it. | Make the key per tenant: add `tenant_id` to it (`Index(..., "tenant_id", "slug", unique=True)`). | +| `SM025` | WARNING | `multi_tenant` is on but no module registered `app.state.tenant_resolver`, so only the principal's `tenant_id` claim can bind a tenant — with most auth providers every tenant-scoped query then fails closed. Checked at boot (it needs the built app), in every environment, not by `make doctor`. | Install the `tenants` module, or register your own `async (Request) -> str \| None` resolver on `app.state.tenant_resolver`. | `SM022`/`SM023` are the two halves of the same invariant: a module's optional [`theme.css` is imported unlayered and `styles.css` into `layer(components)`](/module-authoring#styling), and CSS put in the wrong one silently does nothing (or silently wins everything). @@ -41,7 +42,7 @@ The framework runs a set of static checks over installed modules at app boot. Th | Context | What runs | |---|---| | App boot in development | Full structural/page/i18n suite, results logged to stderr. ERRORS abort boot. | -| App boot in non-development | Strict module discovery (raises on SM001-class failures) + the migration check (SM010). The page/locale static suite is dev-only. | +| App boot in non-development | Strict module discovery (raises on SM001-class failures) + the migration check (SM010) + SM025 (logged as a warning). The page/locale static suite is dev-only. | Sample dev-mode output: diff --git a/framework/core/README.md b/framework/core/README.md index 682f5f42..230b2f47 100644 --- a/framework/core/README.md +++ b/framework/core/README.md @@ -2,7 +2,7 @@ Module-system primitives for the [simple_module](https://github.com/antosubash/simple_module_python) framework — a modular-monolith for Python/FastAPI where each feature is a plugin package discovered at boot. -This package defines `ModuleBase`, the `ModuleMeta` descriptor, the `discover_modules()` entry-point loader, topological dependency sorting, event bus primitives, and the diagnostic codes (`SM001`–`SM021`) used by `make doctor`. +This package defines `ModuleBase`, the `ModuleMeta` descriptor, the `discover_modules()` entry-point loader, topological dependency sorting, event bus primitives, and the diagnostic codes (`SM001`–`SM025`) used by `make doctor`. ## Install @@ -17,7 +17,8 @@ You usually don't install this directly — it's pulled in by `simple_module_hos - `ModuleBase` — the subclass every module extends to opt into lifecycle hooks. - `ModuleMeta` — required `meta = ModuleMeta(name=..., depends_on=...)` attribute on each module. - `discover_modules()` — loads all `[project.entry-points.simple_module]` modules, topologically sorts by `depends_on`. -- Diagnostic registry — `SM001` missing meta, `SM003` orphan page, `SM008` duplicate name, `SM009` framework→plugin coupling violation, and the rest of the `SM0xx` set through `SM021`. +- Diagnostic registry — `SM001` missing meta, `SM003` orphan page, `SM008` duplicate name, `SM009` framework→plugin coupling violation, and the rest of the `SM0xx` set through `SM025`. +- Tenant-role vocabulary (`simple_module_core.tenancy`) — `TenantRole`, `TENANT_ROLE_PREFIX`, `tenant_role()`, so any module can map `tenant:` onto its permissions without depending on `tenants`. - Tiny event-bus (`pyee`) for decoupled module-to-module communication. ## Usage diff --git a/framework/core/simple_module_core/__init__.py b/framework/core/simple_module_core/__init__.py index 9db8fbd7..fa1395b6 100644 --- a/framework/core/simple_module_core/__init__.py +++ b/framework/core/simple_module_core/__init__.py @@ -47,11 +47,13 @@ from simple_module_core.public_routes import PublicRoute, PublicRouteRegistry from simple_module_core.services import Services from simple_module_core.setup_steps import SetupRegistry, SetupStep +from simple_module_core.tenancy import TENANT_ROLE_PREFIX, TenantRole, is_tenant_role, tenant_role from simple_module_core.versioning import FRAMEWORK_API_VERSION, check_framework_compatibility __all__ = [ "DEFAULT_AUTH_PROVIDER", "FRAMEWORK_API_VERSION", + "TENANT_ROLE_PREFIX", "AuditLink", "AuditLinkRegistry", "CircularDependencyError", @@ -89,6 +91,7 @@ "Services", "SetupRegistry", "SetupStep", + "TenantRole", "Translator", "ValidationError", "check_framework_compatibility", @@ -97,10 +100,12 @@ "flag_enabled", "get_module_package_name", "is_flag_enabled", + "is_tenant_role", "print_diagnostics", "require_flag", "resolve_auth_provider", "run_diagnostics", "select_auth_provider", + "tenant_role", "topological_sort", ] diff --git a/framework/core/simple_module_core/diagnostics/__init__.py b/framework/core/simple_module_core/diagnostics/__init__.py index 80d4249f..b163e20a 100644 --- a/framework/core/simple_module_core/diagnostics/__init__.py +++ b/framework/core/simple_module_core/diagnostics/__init__.py @@ -12,6 +12,7 @@ from simple_module_core.diagnostics._module import ModuleDiagnostics from simple_module_core.diagnostics._pages import collect_tsx_pages from simple_module_core.diagnostics._runner import print_diagnostics, run_diagnostics +from simple_module_core.diagnostics._tenancy import check_tenant_resolver from simple_module_core.diagnostics._types import Diagnostic, DiagnosticLevel __all__ = [ @@ -20,6 +21,7 @@ "I18nDiagnostics", "MigrationDiagnostics", "ModuleDiagnostics", + "check_tenant_resolver", "collect_tsx_pages", "print_diagnostics", "run_diagnostics", diff --git a/framework/core/simple_module_core/diagnostics/_tenancy.py b/framework/core/simple_module_core/diagnostics/_tenancy.py index 64066e0e..b2052cba 100644 --- a/framework/core/simple_module_core/diagnostics/_tenancy.py +++ b/framework/core/simple_module_core/diagnostics/_tenancy.py @@ -1,4 +1,6 @@ -"""SM024: a unique key on a tenant-scoped table that ignores the tenant. +"""Tenancy diagnostics. + +SM024: a unique key on a tenant-scoped table that ignores the tenant. On a ``MultiTenantMixin`` table every business key is per tenant. A unique constraint without ``tenant_id`` in it means the first tenant to claim a value @@ -9,6 +11,9 @@ 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. + +SM025: ``multi_tenant`` is on but no module registered +``app.state.tenant_resolver`` — see :func:`check_tenant_resolver`. """ from __future__ import annotations @@ -105,4 +110,32 @@ def module_tables(mod: ModuleBase) -> list[Any]: return list(seen.values()) -__all__ = ["check_tenant_unique_keys", "module_tables"] +def check_tenant_resolver(*, multi_tenant: bool, resolver: object | None) -> list[Diagnostic]: + """SM025: strict tenancy with nothing but the legacy claim path to feed it. + + Without a resolver ``TenantMiddleware`` binds only the principal's + ``tenant_id`` claim (or, for anonymous requests, the tenant header). Most + providers set no such claim, so every tenant-scoped query fails closed with + ``MissingTenantError``. Needs the built app, so it runs at boot (after the + module registrations), not from the ``make doctor`` CLI. + """ + if not multi_tenant or resolver is not None: + return [] + return [ + Diagnostic( + level=DiagnosticLevel.WARNING, + code="SM025", + message=( + "multi_tenant is on but no module registered app.state.tenant_resolver — " + "only the principal's tenant_id claim can bind a tenant" + ), + module_name="", + suggestion=( + "Install the 'tenants' module (or register your own async " + "(Request) -> str | None resolver on app.state.tenant_resolver)" + ), + ) + ] + + +__all__ = ["check_tenant_resolver", "check_tenant_unique_keys", "module_tables"] diff --git a/framework/core/simple_module_core/tenancy.py b/framework/core/simple_module_core/tenancy.py new file mode 100644 index 00000000..11323388 --- /dev/null +++ b/framework/core/simple_module_core/tenancy.py @@ -0,0 +1,49 @@ +"""Tenant-role vocabulary shared by every module (#380). + +A tenant membership role (``owner`` / ``admin`` / ``member``) reaches the +request principal as ``tenant:`` for the *active* tenant only, so a +tenant ``admin`` can never be mistaken for the platform ``admin`` role. The +``tenants`` module assigns these; any other module maps them onto its own +permissions without depending on ``tenants``:: + + from simple_module_core.tenancy import TenantRole, tenant_role + + registry.map_role(tenant_role(TenantRole.MEMBER), ["files.view"]) +""" + +from __future__ import annotations + +from enum import StrEnum + +TENANT_ROLE_PREFIX = "tenant:" + + +class TenantRole(StrEnum): + """A member's role within one tenant.""" + + OWNER = "owner" + ADMIN = "admin" + MEMBER = "member" + + +def tenant_role(name: str) -> str: + """The effective principal role for membership role ``name``: ``tenant:``. + + Raises ``ValueError`` for a name that is not a :class:`TenantRole`, so a + typo in a ``map_role`` call fails at boot instead of granting nothing. + """ + try: + role = TenantRole(name) + except ValueError: + raise ValueError( + f"unknown tenant role {name!r}; expected one of {list(TenantRole)}" + ) from None + return f"{TENANT_ROLE_PREFIX}{role.value}" + + +def is_tenant_role(role: str) -> bool: + """True for an effective tenant role (``tenant:…``) on a principal.""" + return role.startswith(TENANT_ROLE_PREFIX) + + +__all__ = ["TENANT_ROLE_PREFIX", "TenantRole", "is_tenant_role", "tenant_role"] diff --git a/framework/core/tests/test_diagnostics_tenancy.py b/framework/core/tests/test_diagnostics_tenancy.py index 60bab9a3..f9fa8c57 100644 --- a/framework/core/tests/test_diagnostics_tenancy.py +++ b/framework/core/tests/test_diagnostics_tenancy.py @@ -113,3 +113,25 @@ class MultiTenantMixin: # stands in for simple_module_db's, matched by name mod_cls = type("DemoModule", (), {"__module__": "sm024pkg.module"}) assert [t.name for t in module_tables(mod_cls())] == ["demo_scoped"] + + +# ── SM025: multi_tenant on, but nothing resolves the tenant ───────────── + + +def test_multi_tenant_without_resolver_warns(): + from simple_module_core.diagnostics import DiagnosticLevel, check_tenant_resolver + + diags = check_tenant_resolver(multi_tenant=True, resolver=None) + assert [d.code for d in diags] == ["SM025"] + assert diags[0].level == DiagnosticLevel.WARNING + assert "tenant_resolver" in diags[0].message + + +def test_resolver_registered_or_tenancy_off_is_fine(): + from simple_module_core.diagnostics import check_tenant_resolver + + async def resolver(request): # pragma: no cover - never called + return None + + assert check_tenant_resolver(multi_tenant=True, resolver=resolver) == [] + assert check_tenant_resolver(multi_tenant=False, resolver=None) == [] diff --git a/framework/core/tests/test_tenant_roles.py b/framework/core/tests/test_tenant_roles.py new file mode 100644 index 00000000..e211ae73 --- /dev/null +++ b/framework/core/tests/test_tenant_roles.py @@ -0,0 +1,33 @@ +"""Shared tenant-role vocabulary (#380): modules map these without importing ``tenants``.""" + +from __future__ import annotations + +import pytest +from simple_module_core import TENANT_ROLE_PREFIX, TenantRole, is_tenant_role, tenant_role + + +def test_prefix_and_names(): + assert TENANT_ROLE_PREFIX == "tenant:" + assert [r.value for r in TenantRole] == ["owner", "admin", "member"] + + +@pytest.mark.parametrize( + ("name", "expected"), + [ + (TenantRole.OWNER, "tenant:owner"), + (TenantRole.ADMIN, "tenant:admin"), + ("member", "tenant:member"), + ], +) +def test_tenant_role_builds_the_effective_role(name, expected): + assert tenant_role(name) == expected + + +def test_tenant_role_rejects_unknown_names(): + with pytest.raises(ValueError, match="tenant role"): + tenant_role("superuser") + + +def test_is_tenant_role(): + assert is_tenant_role("tenant:member") + assert not is_tenant_role("admin") diff --git a/framework/db/simple_module_db/__init__.py b/framework/db/simple_module_db/__init__.py index dc28919f..2929f334 100644 --- a/framework/db/simple_module_db/__init__.py +++ b/framework/db/simple_module_db/__init__.py @@ -16,6 +16,7 @@ from simple_module_db.session import DatabaseState, RequestSession, init_db from simple_module_db.tenancy import ( ALL_TENANTS_OPTION, + DEFAULT_TENANT_ID, TENANT_ID_PATTERN, MissingTenantError, TenantIsolationError, @@ -30,6 +31,7 @@ __all__ = [ "ALL_TENANTS_OPTION", + "DEFAULT_TENANT_ID", "LIKE_ESCAPE_CHAR", "TENANT_ID_PATTERN", "AuditMixin", diff --git a/framework/db/simple_module_db/flush_guard.py b/framework/db/simple_module_db/flush_guard.py index cd70068f..ff960c69 100644 --- a/framework/db/simple_module_db/flush_guard.py +++ b/framework/db/simple_module_db/flush_guard.py @@ -13,8 +13,9 @@ from sqlalchemy.orm import Session from simple_module_db.mixins import MultiTenantMixin -from simple_module_db.query_filter import is_strict +from simple_module_db.query_filter import is_strict, strict_configured from simple_module_db.tenancy import ( + DEFAULT_TENANT_ID, TenantIsolationError, current_tenant_id, is_all_tenants, @@ -30,11 +31,24 @@ def _owner(obj: MultiTenantMixin) -> str | None: return obj.tenant_id +def _stamp_unbound(session: Session) -> None: + """No tenant bound on a non-strict install: new rows go to the default tenant.""" + for obj in session.new: + if isinstance(obj, MultiTenantMixin) and obj.tenant_id is None: + obj.tenant_id = DEFAULT_TENANT_ID + + def guard_flush(session: Session) -> None: if is_all_tenants(): + # A bypass block is never scoped; on a single-tenant install its new + # rows still need a tenant to satisfy NOT NULL (strict: left to the DB). + if not strict_configured(session): + _stamp_unbound(session) return tenant_id = current_tenant_id.get() strict = is_strict(session) + if tenant_id is None and not strict: + _stamp_unbound(session) for obj in session.new: if not isinstance(obj, MultiTenantMixin): diff --git a/framework/db/simple_module_db/insert_guard.py b/framework/db/simple_module_db/insert_guard.py index c575bf1e..d300a4a3 100644 --- a/framework/db/simple_module_db/insert_guard.py +++ b/framework/db/simple_module_db/insert_guard.py @@ -1,7 +1,8 @@ """INSERT guard and statement-value helpers for the tenant query filter. Explicit ``tenant_id`` values in an insert must match the bound tenant, and -missing ones are stamped with it (#357); the same value readers let the +missing ones are stamped with it (#357) — or, unbound on a non-strict +install, with ``DEFAULT_TENANT_ID`` (#380); the same value readers let the query filter refuse an UPDATE that assigns ``tenant_id`` (#356). """ @@ -14,6 +15,7 @@ from simple_module_db import model_registry as registry from simple_module_db.tenancy import ( ALL_TENANTS_OPTION, + DEFAULT_TENANT_ID, TenantIsolationError, current_tenant_id, is_all_tenants, @@ -63,11 +65,14 @@ def guard_insert(execute_state: ORMExecuteState, *, strict: bool) -> Any: target = getattr(execute_state.statement, "table", None) if not mappers and getattr(target, "name", None) in registry.tenant_table_names: mappers = [target.name] # Core insert(Model.__table__) - if not mappers or execute_state.execution_options.get(ALL_TENANTS_OPTION, False): + if not mappers: return None - if is_all_tenants(): + bypass = execute_state.execution_options.get(ALL_TENANTS_OPTION, False) or is_all_tenants() + if bypass and strict: return None - tenant_id = current_tenant_id.get() + # Unbound on a non-strict install (bypassed or not): nothing to check + # explicit values against, missing ones get DEFAULT_TENANT_ID (#380). + tenant_id = None if bypass else current_tenant_id.get() if getattr(execute_state.statement, "_multi_values", None): return _guard_multi_values(execute_state, tenant_id, mappers[0], strict) for value in assigned_tenant_ids(execute_state): @@ -81,15 +86,14 @@ def guard_insert(execute_state: ORMExecuteState, *, strict: bool) -> Any: ) if not missing: return None - if tenant_id is None: - if strict: - raise missing_tenant_error(mappers[0], "INSERT") - return None + if tenant_id is None and strict: + raise missing_tenant_error(mappers[0], "INSERT") + stamp = tenant_id or DEFAULT_TENANT_ID if rows: - stamped = [{**r, TENANT_COLUMN: r.get(TENANT_COLUMN, tenant_id)} for r in rows] + stamped = [{**r, TENANT_COLUMN: r.get(TENANT_COLUMN, stamp)} for r in rows] payload = stamped if isinstance(execute_state.parameters, list) else stamped[0] return execute_state.invoke_statement(params=payload) - execute_state.statement = execute_state.statement.values(**{TENANT_COLUMN: tenant_id}) + execute_state.statement = execute_state.statement.values(**{TENANT_COLUMN: stamp}) return None @@ -116,15 +120,13 @@ def _guard_multi_values( raise _foreign_insert(value, tenant_id) else: missing = True - mapping[tenant_col] = tenant_id + mapping[tenant_col] = tenant_id or DEFAULT_TENANT_ID rows.append(mapping) groups.append(rows) if not missing: return None - if tenant_id is None: - if strict: - raise missing_tenant_error(entity, "INSERT") - return None + if tenant_id is None and strict: + raise missing_tenant_error(entity, "INSERT") stamped = stmt._generate() stamped._multi_values = tuple(groups) execute_state.statement = stamped diff --git a/framework/db/simple_module_db/mixins.py b/framework/db/simple_module_db/mixins.py index a08a9fcd..3db76f87 100644 --- a/framework/db/simple_module_db/mixins.py +++ b/framework/db/simple_module_db/mixins.py @@ -67,9 +67,9 @@ class MultiTenantMixin(SQLModel): inside a request scope without explicitly threading the tenant through — the ``_before_flush_listener`` in :mod:`simple_module_db.listeners` populates it from the ``current_tenant_id`` contextvar before the row - reaches the DB. The column itself is non-nullable, so a row inserted - outside any tenant context fails loudly at the DB rather than silently - leaking across tenants. + reaches the DB. The column itself is non-nullable. With no tenant bound, + strict mode (``multi_tenant`` on) raises ``MissingTenantError``; a + single-tenant install stamps ``DEFAULT_TENANT_ID`` instead (#380). """ tenant_id: str | None = Field( diff --git a/framework/db/simple_module_db/query_filter.py b/framework/db/simple_module_db/query_filter.py index 264254c9..455f3755 100644 --- a/framework/db/simple_module_db/query_filter.py +++ b/framework/db/simple_module_db/query_filter.py @@ -51,7 +51,7 @@ def bind_engine_policy(engine: Engine, policy: Any) -> None: _engine_policy[engine] = policy -def _strict_configured(session: Session) -> bool: +def strict_configured(session: Session) -> bool: bind = session.bind policy = _engine_policy.get(bind) if isinstance(bind, Engine) else None return bool(policy is not None and policy.tenant_strict) @@ -59,7 +59,7 @@ def _strict_configured(session: Session) -> bool: def is_strict(session: Session) -> bool: """Strict isolation is on for this session's engine and not waived.""" - return _strict_configured(session) and not is_all_tenants() + return strict_configured(session) and not is_all_tenants() def filter_statements(execute_state: ORMExecuteState) -> Any: @@ -76,7 +76,7 @@ def filter_statements(execute_state: ORMExecuteState) -> Any: ``all_tenants()``; soft-delete bypass: ``include_deleted=True``. """ if execute_state.is_insert: - return guard_insert(execute_state, strict=_strict_configured(execute_state.session)) + return guard_insert(execute_state, strict=strict_configured(execute_state.session)) is_select = execute_state.is_select if not (is_select or execute_state.is_update or execute_state.is_delete): return None @@ -85,7 +85,7 @@ def filter_statements(execute_state: ORMExecuteState) -> Any: skip_soft_delete = not is_select or options_in.get("include_deleted", False) tenant_id = current_tenant_id.get() skip_tenant = options_in.get(ALL_TENANTS_OPTION, False) or is_all_tenants() - strict = _strict_configured(execute_state.session) + strict = strict_configured(execute_state.session) if execute_state.is_update and not skip_tenant: # Before the early return: moving rows between tenants is refused # bound or not (#356), like the unit-of-work rule in flush_guard. diff --git a/framework/db/simple_module_db/tenancy.py b/framework/db/simple_module_db/tenancy.py index 71549ced..f6a98030 100644 --- a/framework/db/simple_module_db/tenancy.py +++ b/framework/db/simple_module_db/tenancy.py @@ -6,7 +6,8 @@ ``DatabaseState.tenant_strict``: * **not strict** (single-tenant installs, the historical default): no filter - is applied — the query sees every tenant's rows. + is applied — the query sees every tenant's rows — and an insert is stamped + with :data:`DEFAULT_TENANT_ID`. * **strict** (enabled whenever the host runs with ``multi_tenant``): the query raises :class:`TenantIsolationError` instead. A request, background job or CLI command that forgot to establish a tenant fails loudly rather than @@ -45,6 +46,17 @@ (a header, a path) must pass this before it is bound.""" +DEFAULT_TENANT_ID = "default" +"""The tenant a row lands in when tenancy is not strict and none is bound. + +A single-tenant install (``multi_tenant`` off) has no tenant to bind, but +``MultiTenantMixin.tenant_id`` is NOT NULL, so inserts are stamped with this. +Adoption migrations backfill existing rows with the same value. It is not the +``default_tenant`` host setting: that one *binds* a tenant per request; this +is only the fallback when nothing is bound. Strict mode never uses it. +""" + + def is_valid_tenant_id(value: object) -> bool: return isinstance(value, str) and TENANT_ID_PATTERN.fullmatch(value) is not None @@ -156,6 +168,7 @@ def missing_tenant_error(entity: str, operation: str) -> MissingTenantError: __all__ = [ "ALL_TENANTS_OPTION", + "DEFAULT_TENANT_ID", "TENANT_ID_PATTERN", "MissingTenantError", "TenantIsolationError", diff --git a/framework/db/tests/conftest.py b/framework/db/tests/conftest.py index 28998fe1..3e06bb26 100644 --- a/framework/db/tests/conftest.py +++ b/framework/db/tests/conftest.py @@ -14,10 +14,9 @@ _URL = database_url_for_tests() -@pytest.fixture -async def tenant_session() -> AsyncGenerator[AsyncSession, None]: - """Session backed by in-memory SQLite with tenant listeners registered.""" +async def _tenant_session(*, strict: bool) -> AsyncGenerator[AsyncSession, None]: db_state = init_db(_URL, **init_db_kwargs(_URL)) + db_state.tenant_strict = strict try: register_listeners(db_state) await reset_schema(db_state.engine) @@ -26,4 +25,19 @@ async def tenant_session() -> AsyncGenerator[AsyncSession, None]: async with db_state.session_factory() as session: yield session finally: + db_state.tenant_strict = False await db_state.engine.dispose() + + +@pytest.fixture +async def tenant_session() -> AsyncGenerator[AsyncSession, None]: + """Session backed by in-memory SQLite with tenant listeners registered.""" + async for session in _tenant_session(strict=False): + yield session + + +@pytest.fixture +async def strict_session() -> AsyncGenerator[AsyncSession, None]: + """Like ``tenant_session``, with fail-closed isolation (``multi_tenant`` on).""" + async for session in _tenant_session(strict=True): + yield session diff --git a/framework/db/tests/test_default_tenant.py b/framework/db/tests/test_default_tenant.py new file mode 100644 index 00000000..dcad54c6 --- /dev/null +++ b/framework/db/tests/test_default_tenant.py @@ -0,0 +1,86 @@ +"""Single-tenant installs: an unbound insert lands in ``DEFAULT_TENANT_ID`` (#380). + +``MultiTenantMixin.tenant_id`` is NOT NULL, so before #380 the first module to +adopt the mixin broke every install with ``multi_tenant`` off. Strict mode must +keep failing closed — see ``test_tenant_strict.py``. +""" + +from __future__ import annotations + +import pytest +from _models import _TenantItem +from simple_module_db import ( + DEFAULT_TENANT_ID, + TenantIsolationError, + all_tenants, + tenant_context, +) +from sqlalchemy import insert, select +from sqlalchemy.ext.asyncio import AsyncSession + + +def test_default_tenant_id_is_a_valid_tenant_id(): + from simple_module_db import is_valid_tenant_id + + assert DEFAULT_TENANT_ID == "default" + assert is_valid_tenant_id(DEFAULT_TENANT_ID) + + +async def test_session_add_without_tenant_stamps_default(tenant_session: AsyncSession): + item = _TenantItem(name="single") + tenant_session.add(item) + await tenant_session.flush() + assert item.tenant_id == DEFAULT_TENANT_ID + + +async def test_explicit_tenant_id_is_kept_when_unbound(tenant_session: AsyncSession): + item = _TenantItem(name="explicit", tenant_id="other") + tenant_session.add(item) + await tenant_session.flush() + assert item.tenant_id == "other" + + +async def test_bound_tenant_still_wins(tenant_session: AsyncSession): + with tenant_context("acme"): + item = _TenantItem(name="bound") + tenant_session.add(item) + await tenant_session.flush() + assert item.tenant_id == "acme" + + +async def test_all_tenants_block_on_single_tenant_install_stamps_default( + tenant_session: AsyncSession, +): + with all_tenants(): + item = _TenantItem(name="platform") + tenant_session.add(item) + await tenant_session.flush() + assert item.tenant_id == DEFAULT_TENANT_ID + + +async def test_orm_insert_values_stamps_default(tenant_session: AsyncSession): + await tenant_session.execute(insert(_TenantItem).values(name="core-one")) + await tenant_session.execute(insert(_TenantItem), [{"name": "bulk-a"}, {"name": "bulk-b"}]) + await tenant_session.execute(insert(_TenantItem).values([{"name": "mv-a"}, {"name": "mv-b"}])) + await tenant_session.execute(insert(_TenantItem.__table__).values(name="table")) + rows = (await tenant_session.execute(select(_TenantItem))).scalars().all() + assert len(rows) == 6 + assert {r.tenant_id for r in rows} == {DEFAULT_TENANT_ID} + + +async def test_unbound_reads_stay_unfiltered(tenant_session: AsyncSession): + """Not strict + no tenant reads every row — the single-tenant semantics.""" + tenant_session.add(_TenantItem(name="default-row")) + tenant_session.add(_TenantItem(name="other-row", tenant_id="other")) + await tenant_session.flush() + rows = (await tenant_session.execute(select(_TenantItem))).scalars().all() + assert {r.name for r in rows} == {"default-row", "other-row"} + + +async def test_strict_mode_does_not_fall_back(strict_session: AsyncSession): + strict_session.add(_TenantItem(name="orphan")) + with pytest.raises(TenantIsolationError, match="INSERT"): + await strict_session.flush() + await strict_session.rollback() + with pytest.raises(TenantIsolationError, match="INSERT"): + await strict_session.execute(insert(_TenantItem).values(name="orphan")) diff --git a/framework/db/tests/test_multi_tenancy.py b/framework/db/tests/test_multi_tenancy.py index c5d5cacc..518a778d 100644 --- a/framework/db/tests/test_multi_tenancy.py +++ b/framework/db/tests/test_multi_tenancy.py @@ -6,9 +6,9 @@ import pytest from _models import _TenantItem, _TenantSoftItem +from simple_module_db import DEFAULT_TENANT_ID from simple_module_db.listeners import TenantIsolationError, current_tenant_id from sqlalchemy import select -from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession @@ -231,15 +231,14 @@ async def test_cross_tenant_violation_does_not_leak_state(self, tenant_session: finally: current_tenant_id.reset(token) - async def test_creation_without_tenant_or_context_fails_at_db( + async def test_creation_without_tenant_or_context_uses_default( self, tenant_session: AsyncSession ): - """No tenant context and no explicit tenant_id → NOT NULL constraint fires.""" + """Not strict, no tenant, no explicit tenant_id → the default tenant (#380).""" item = _TenantItem(name="Orphan") tenant_session.add(item) - with pytest.raises(IntegrityError): - await tenant_session.flush() - await tenant_session.rollback() + await tenant_session.flush() + assert item.tenant_id == DEFAULT_TENANT_ID async def test_system_operation_sees_all_tenants(self, tenant_session: AsyncSession): """Without a tenant context, a system query can read across all tenants.""" diff --git a/framework/db/tests/test_tenant_strict.py b/framework/db/tests/test_tenant_strict.py index 10a4bc0c..fdfc3803 100644 --- a/framework/db/tests/test_tenant_strict.py +++ b/framework/db/tests/test_tenant_strict.py @@ -2,35 +2,12 @@ from __future__ import annotations -from collections.abc import AsyncGenerator - import pytest -from _models import _TenantBase, _TenantItem +from _models import _TenantItem from simple_module_db import TenantIsolationError, all_tenants, current_tenant_id, tenant_context -from simple_module_db.listeners import register_listeners -from simple_module_db.session import init_db -from simple_module_test.database import database_url_for_tests, init_db_kwargs, reset_schema from sqlalchemy import delete, func, select, update from sqlalchemy.ext.asyncio import AsyncSession -_URL = database_url_for_tests() - - -@pytest.fixture -async def strict_session() -> AsyncGenerator[AsyncSession, None]: - db_state = init_db(_URL, **init_db_kwargs(_URL)) - db_state.tenant_strict = True - try: - register_listeners(db_state) - await reset_schema(db_state.engine) - async with db_state.engine.begin() as conn: - await conn.run_sync(_TenantBase.metadata.create_all) - async with db_state.session_factory() as session: - yield session - finally: - db_state.tenant_strict = False - await db_state.engine.dispose() - async def _seed(session: AsyncSession) -> None: for tenant in ("a", "b"): diff --git a/framework/hosting/simple_module_hosting/_boot_checks.py b/framework/hosting/simple_module_hosting/_boot_checks.py new file mode 100644 index 00000000..bd9aae65 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_boot_checks.py @@ -0,0 +1,47 @@ +"""Diagnostics that need the built app, so cannot run from ``make doctor``. + +The module diagnostics in Phase 2 see only module classes and the source tree. +A check about what modules *registered* has to wait until after Phase 5, and +unlike the source-tree checks it is meaningful in production too. +""" + +from __future__ import annotations + +import logging +from typing import TYPE_CHECKING, Any + +from simple_module_core.diagnostics import Diagnostic, check_tenant_resolver, print_diagnostics + +if TYPE_CHECKING: + from fastapi import FastAPI + from simple_module_core.services import DiagnosticsState + +logger = logging.getLogger(__name__) + + +def _tenant_checks(app: FastAPI, settings: Any) -> list[Diagnostic]: + return check_tenant_resolver( + multi_tenant=bool(getattr(settings, "multi_tenant", False)), + resolver=getattr(app.state, "tenant_resolver", None), + ) + + +def report_tenant_resolution( + app: FastAPI, settings: Any, diagnostics_state: DiagnosticsState +) -> list[Diagnostic]: + """SM025: report, and fold into the Doctor screen's results when it has a runner.""" + found = _tenant_checks(app, settings) + if found: + if settings.is_development: + print_diagnostics(found) + else: + for diag in found: + logger.warning("%s", diag) + runner = diagnostics_state.runner + if runner is not None: + diagnostics_state.runner = lambda: [*runner(), *_tenant_checks(app, settings)] + diagnostics_state.results = [*diagnostics_state.results, *found] + return found + + +__all__ = ["report_tenant_resolution"] diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index 9402675b..def295a6 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -24,6 +24,7 @@ from simple_module_db.listeners import register_listeners from simple_module_db.session import init_db +from simple_module_hosting._boot_checks import report_tenant_resolution from simple_module_hosting._db_health import register_database_check from simple_module_hosting._dev_boot import run_dev_boot from simple_module_hosting._inertia_setup import setup_inertia @@ -210,6 +211,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: ) attach_public_routes(app, settings, public_route_registry) + report_tenant_resolution(app, settings, diagnostics_state) # SM025 # Branding reads the packs off app.state directly: its API validates a # submitted slug before persisting it, and its view builds the dropdown. diff --git a/framework/hosting/tests/test_tenant_resolver_diagnostic.py b/framework/hosting/tests/test_tenant_resolver_diagnostic.py new file mode 100644 index 00000000..9414e472 --- /dev/null +++ b/framework/hosting/tests/test_tenant_resolver_diagnostic.py @@ -0,0 +1,47 @@ +"""SM025 at boot: ``multi_tenant`` on with no ``app.state.tenant_resolver`` (#380).""" + +from __future__ import annotations + +from types import SimpleNamespace + +from fastapi import FastAPI +from simple_module_core.services import DiagnosticsState +from simple_module_hosting._boot_checks import report_tenant_resolution + + +def _settings(*, multi_tenant: bool, dev: bool = True) -> SimpleNamespace: + return SimpleNamespace(multi_tenant=multi_tenant, is_development=dev) + + +def test_missing_resolver_is_reported_and_kept_for_the_doctor_screen(): + app = FastAPI() + state = DiagnosticsState(runner=list) + state.rerun() + + found = report_tenant_resolution(app, _settings(multi_tenant=True), state) + + assert [d.code for d in found] == ["SM025"] + assert [d.code for d in state.results] == ["SM025"] + # "Re-run checks" re-evaluates against the live app state. + app.state.tenant_resolver = lambda request: None + assert state.rerun() == [] + + +def test_registered_resolver_is_clean(): + app = FastAPI() + app.state.tenant_resolver = lambda request: None + assert report_tenant_resolution(app, _settings(multi_tenant=True), DiagnosticsState()) == [] + + +def test_production_logs_instead_of_printing(caplog): + found = report_tenant_resolution( + FastAPI(), _settings(multi_tenant=True, dev=False), DiagnosticsState() + ) + assert [d.code for d in found] == ["SM025"] + assert "SM025" in caplog.text + + +def test_single_tenant_install_is_clean(): + assert ( + report_tenant_resolution(FastAPI(), _settings(multi_tenant=False), DiagnosticsState()) == [] + ) diff --git a/framework/testing/README.md b/framework/testing/README.md index 9425f5a8..fc8f7f79 100644 --- a/framework/testing/README.md +++ b/framework/testing/README.md @@ -23,6 +23,7 @@ All fixtures below are auto-registered via the `pytest11` entry point — instal - `app` — a `create_app(settings)` instance with `lifespan` started and stopped. - `client` — an `httpx.AsyncClient` bound to the test app. - `authenticated_client` — same but with an admin user seeded and a forged session cookie attached. **Requires the `users` module** to be installed (it seeds the admin via `users.bootstrap`); apps scaffolded by `smpy` include it. +- `tenant_client` — factory: `async with tenant_client("member") as (client, tenant_id, user_id)` signs in a fresh user who is an `owner` / `admin` / `member` (default `owner`) of a new tenant — or of `tenant_id=` when given — with that tenant active. **Requires the `users` and `tenants` modules.** ## Usage diff --git a/framework/testing/simple_module_test/__init__.py b/framework/testing/simple_module_test/__init__.py index d8e557df..934cc16f 100644 --- a/framework/testing/simple_module_test/__init__.py +++ b/framework/testing/simple_module_test/__init__.py @@ -20,16 +20,21 @@ See ``simple_module_test.plugin`` (and ``simple_module_test.fixtures``) for their definitions. ``authenticated_client`` additionally requires the ``users`` module to be installed — it seeds an admin via ``users.bootstrap``. +``tenant_client`` (``users`` + ``tenants``) is a factory yielding a client signed +in as an ``owner``/``admin``/``member`` of a real tenant — see +``simple_module_test.tenant_client``. """ from simple_module_test.app_factory import build_test_app from simple_module_test.fake_events import FakeEventBus, RecordedEvent from simple_module_test.routes import effective_route_paths from simple_module_test.session_cookie import forge_session_cookie +from simple_module_test.tenant_client import TenantClient __all__ = [ "FakeEventBus", "RecordedEvent", + "TenantClient", "build_test_app", "effective_route_paths", "forge_session_cookie", diff --git a/framework/testing/simple_module_test/plugin.py b/framework/testing/simple_module_test/plugin.py index 45ad80f0..4543958b 100644 --- a/framework/testing/simple_module_test/plugin.py +++ b/framework/testing/simple_module_test/plugin.py @@ -34,6 +34,7 @@ setup_pending_app, ) from simple_module_test.redis_server import redis_server # noqa: F401 +from simple_module_test.tenant_client import tenant_client # noqa: F401 def _bootstrap_eager_celery() -> None: diff --git a/framework/testing/simple_module_test/tenant_client.py b/framework/testing/simple_module_test/tenant_client.py new file mode 100644 index 00000000..430f1192 --- /dev/null +++ b/framework/testing/simple_module_test/tenant_client.py @@ -0,0 +1,124 @@ +"""``tenant_client``: a signed-in member of a real tenant, with a chosen role. + +For a module adopting ``MultiTenantMixin``: its tests need requests that act +for one tenant, as an ``owner``, ``admin`` or ``member`` there — the role that +reaches the principal as ``tenant:``:: + + async def test_members_can_read(tenant_client): + async with tenant_client("member") as (client, tenant_id, user_id): + resp = await client.get("/api/things") + + async def test_isolation(tenant_client): + async with tenant_client() as a, tenant_client() as b: + ... # two tenants, two owners + + async with tenant_client("owner") as owner, tenant_client( + "member", tenant_id=owner.tenant_id + ) as member: + ... # two users of the same tenant + +Requires the ``users`` and ``tenants`` modules. Both are imported inside the +fixture body, never at module scope, so this plugin still imports cleanly in +an app that has neither — the same rule ``authenticated_client`` follows. +""" + +from __future__ import annotations + +import uuid +from collections.abc import AsyncIterator, Callable +from contextlib import AbstractAsyncContextManager, asynccontextmanager +from typing import Any, NamedTuple + +import httpx +import pytest +from simple_module_core.tenancy import TenantRole +from sqlalchemy import select + +from simple_module_test.session_cookie import forge_session_cookie + + +class TenantClient(NamedTuple): + client: httpx.AsyncClient + tenant_id: str + user_id: str + + +async def create_user(app: Any, email: str) -> str: + """A real, active, non-superuser ``users`` row with the ``user`` role; its id.""" + from users.models import Role, User, UserRole + + async with app.state.sm.db.session_factory() as session: + user = User( + id=uuid.uuid4(), + email=email, + hashed_password="x", + is_active=True, + is_superuser=False, + is_verified=True, + ) + session.add(user) + await session.flush() + role = (await session.execute(select(Role).where(Role.name == "user"))).scalar_one_or_none() + if role is not None: + session.add(UserRole(user_id=user.id, role_id=role.id)) + await session.commit() + return str(user.id) + + +async def _join(app: Any, user_id: str, email: str, role: TenantRole, tenant_id: str | None) -> str: + from tenants.models import Membership, Tenant + from tenants.resolver import forget + + async with app.state.sm.db.session_factory() as session: + if tenant_id is None: + tenant = Tenant(slug=f"t-{uuid.uuid4().hex[:12]}", name="Test Org") + session.add(tenant) + await session.flush() + tenant_id = tenant.id + session.add(Membership(tenant_id=tenant_id, user_id=user_id, role=role, email=email)) + await session.commit() + forget(user_id) # the resolver caches memberships per user + return tenant_id + + +@asynccontextmanager +async def session_client(app: Any, data: dict[str, Any]) -> AsyncIterator[httpx.AsyncClient]: + """An ``httpx`` client for ``app`` whose signed session cookie holds ``data``.""" + cookie = forge_session_cookie(app.state.sm.settings.secret_key, data) + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), + base_url="http://testserver", + cookies={"session": cookie}, + ) as client: + yield client + + +@pytest.fixture +def tenant_client(app: Any) -> Callable[..., AbstractAsyncContextManager[TenantClient]]: + """Factory: ``tenant_client(role="owner", *, tenant_id=None, email=None)``. + + Creates a user and a membership with ``role`` — in a new tenant, or in + ``tenant_id`` when given — and yields a :class:`TenantClient` whose session + has that tenant active. + """ + from tenants.constants import SESSION_ACTIVE_TENANT + + @asynccontextmanager + async def factory( + role: str = TenantRole.OWNER, + *, + tenant_id: str | None = None, + email: str | None = None, + ) -> AsyncIterator[TenantClient]: + member_role = TenantRole(role) + email = email or f"{member_role}-{uuid.uuid4().hex[:8]}@example.com" + user_id = await create_user(app, email) + tenant_id = await _join(app, user_id, email, member_role, tenant_id) + data = {"user_id": user_id, SESSION_ACTIVE_TENANT: tenant_id} + async with session_client(app, data) as client: + yield TenantClient(client, tenant_id, user_id) + + return factory + + +__all__ = ["TenantClient", "create_user", "session_client", "tenant_client"] diff --git a/modules/tenants/tenants/constants.py b/modules/tenants/tenants/constants.py index 92e6c058..b133702b 100644 --- a/modules/tenants/tenants/constants.py +++ b/modules/tenants/tenants/constants.py @@ -4,18 +4,22 @@ from enum import StrEnum +# The role vocabulary lives in core so other modules can map ``tenant:`` +# onto their permissions without depending on this module (#380). +from simple_module_core.tenancy import TENANT_ROLE_PREFIX, tenant_role +from simple_module_core.tenancy import TenantRole as MembershipRole + MODULE_PACKAGE = "tenants" DISPLAY_NAME = "Tenants" +# Names of the modules this one depends on (``ModuleMeta.depends_on``). +_MODULE_AUTH = "Auth" +_MODULE_SETTINGS = "Settings" + # Session key holding the tenant the signed-in user last switched to. Only a # preference: the resolver re-validates it against a membership every request. SESSION_ACTIVE_TENANT = "sm_active_tenant" -# Effective-role prefix: a membership role becomes ``tenant:`` on the -# request principal for the active tenant only, so a tenant ``admin`` can -# never be confused with the platform ``admin`` role. -TENANT_ROLE_PREFIX = "tenant:" - INVALIDATION_CHANNEL = "tenants.membership" # Entitlement keys this module enforces (see contracts.entitlements). @@ -32,12 +36,6 @@ class TenantStatus(StrEnum): SUSPENDED = "suspended" -class MembershipRole(StrEnum): - OWNER = "owner" - ADMIN = "admin" - MEMBER = "member" - - MANAGER_ROLES = frozenset({MembershipRole.OWNER, MembershipRole.ADMIN}) # Permissions. ``tenants.*`` within the active tenant; ``tenants.platform.*`` @@ -59,3 +57,5 @@ class MembershipRole(StrEnum): PAGE_MEMBERS = "Tenants/Members" PAGE_ACCEPT = "Tenants/AcceptInvitation" PAGE_ADMIN = "Tenants/AdminBrowse" + +__all__ = ["TENANT_ROLE_PREFIX", "MembershipRole", "tenant_role"] diff --git a/modules/tenants/tenants/module.py b/modules/tenants/tenants/module.py index f096bf8c..38545fdf 100644 --- a/modules/tenants/tenants/module.py +++ b/modules/tenants/tenants/module.py @@ -35,7 +35,7 @@ class TenantsModule(ModuleBase): admin_view_prefix="/admin/tenants", # Auth: its middleware must have set request.state.user before the # resolver runs. Settings: register_module_settings. - depends_on=["Auth", "Settings"], + depends_on=[c._MODULE_AUTH, c._MODULE_SETTINGS], ) def register_settings(self, app: FastAPI) -> None: @@ -128,7 +128,7 @@ def register_permissions(self, registry: PermissionRegistry) -> None: ], ) for role, perms in c.ROLE_PERMISSIONS.items(): - registry.map_role(f"{c.TENANT_ROLE_PREFIX}{role}", perms) + registry.map_role(c.tenant_role(role), perms) async def on_startup(self, app: FastAPI) -> None: if not getattr(app.state.sm.settings, "multi_tenant", False): diff --git a/modules/tenants/tenants/resolver.py b/modules/tenants/tenants/resolver.py index e391907d..37fb4e99 100644 --- a/modules/tenants/tenants/resolver.py +++ b/modules/tenants/tenants/resolver.py @@ -21,6 +21,7 @@ from cachetools import TTLCache from fastapi import FastAPI from simple_module_core.invalidation import Invalidation, InvalidationBus +from simple_module_core.tenancy import is_tenant_role from simple_module_db import is_valid_tenant_id from starlette.requests import Request @@ -117,7 +118,7 @@ def pick_active(memberships: list[MyTenantView], preferred: str | None) -> MyTen def _with_tenant_role(user: Any, tenant_id: str, role: str) -> Any: if not dataclasses.is_dataclass(user) or isinstance(user, type): return user - roles = [r for r in getattr(user, "roles", []) if not r.startswith(TENANT_ROLE_PREFIX)] + roles = [r for r in getattr(user, "roles", []) if not is_tenant_role(r)] changes: dict[str, Any] = {"roles": [*roles, f"{TENANT_ROLE_PREFIX}{role}"]} if "tenant_id" in {f.name for f in dataclasses.fields(user)}: changes["tenant_id"] = tenant_id diff --git a/modules/tenants/tests/conftest.py b/modules/tenants/tests/conftest.py index adeebbbf..9c02a5ab 100644 --- a/modules/tenants/tests/conftest.py +++ b/modules/tenants/tests/conftest.py @@ -1,15 +1,18 @@ -"""Helpers: real users with their own signed-session clients.""" +"""Helpers: real users with their own signed-session clients. + +``tenant_client`` (a member of a tenant, with a role) comes from the +``simple_module_test`` plugin; ``user_client`` here is a user with *no* tenant, +which the tests that exercise creating and joining organisations start from. +""" from __future__ import annotations -import uuid from collections.abc import AsyncGenerator, Callable from contextlib import asynccontextmanager import httpx import pytest -from simple_module_test.session_cookie import forge_session_cookie -from sqlalchemy import select +from simple_module_test.tenant_client import create_user, session_client from tenants.resolver import forget @@ -20,40 +23,14 @@ def _fresh_membership_cache(): forget(None) -async def _make_user(app, email: str) -> str: - from users.models import Role, User, UserRole - - async with app.state.sm.db.session_factory() as session: - user = User( - id=uuid.uuid4(), - email=email, - hashed_password="x", - is_active=True, - is_superuser=False, - is_verified=True, - ) - session.add(user) - await session.flush() - role = (await session.execute(select(Role).where(Role.name == "user"))).scalar_one_or_none() - if role is not None: - session.add(UserRole(user_id=user.id, role_id=role.id)) - await session.commit() - return str(user.id) - - @pytest.fixture def user_client(app) -> Callable: """``async with user_client("a@x.io") as (client, user_id): ...``""" @asynccontextmanager async def factory(email: str) -> AsyncGenerator[tuple[httpx.AsyncClient, str], None]: - user_id = await _make_user(app, email) - cookie = forge_session_cookie(app.state.sm.settings.secret_key, {"user_id": user_id}) - async with httpx.AsyncClient( - transport=httpx.ASGITransport(app=app), - base_url="http://testserver", - cookies={"session": cookie}, - ) as client: + user_id = await create_user(app, email) + async with session_client(app, {"user_id": user_id}) as client: yield client, user_id return factory diff --git a/modules/tenants/tests/test_invitations.py b/modules/tenants/tests/test_invitations.py index 127263b3..1958644a 100644 --- a/modules/tenants/tests/test_invitations.py +++ b/modules/tenants/tests/test_invitations.py @@ -61,13 +61,8 @@ async def test_invitations_cannot_grant_owner(user_client): assert resp.status_code == 422 -async def test_plain_member_cannot_invite(user_client): - async with user_client("owner@x.io") as (owner, _), user_client("m@x.io") as (member, _): - await _setup(owner) - token = ( - await owner.post("/api/tenants/current/invitations", json={"email": "m@x.io"}) - ).json()["token"] - await member.post("/api/tenants/invitations/accept", json={"token": token}) +async def test_plain_member_cannot_invite(tenant_client): + async with tenant_client("member") as (member, _, _): resp = await member.post("/api/tenants/current/invitations", json={"email": "z@x.io"}) assert resp.status_code == 403 diff --git a/modules/tenants/tests/test_isolation_e2e.py b/modules/tenants/tests/test_isolation_e2e.py index d23ec97c..599f645d 100644 --- a/modules/tenants/tests/test_isolation_e2e.py +++ b/modules/tenants/tests/test_isolation_e2e.py @@ -43,15 +43,13 @@ async def _org(client, name): return (await client.post("/api/tenants/", json={"name": name})).json() -async def test_each_tenant_sees_only_its_rows(notes_app, user_client): - async with user_client("a@x.io") as (a, _), user_client("b@x.io") as (b, _): - ta = await _org(a, "Alpha") - tb = await _org(b, "Beta") +async def test_each_tenant_sees_only_its_rows(notes_app, tenant_client): + async with tenant_client() as (a, ta, _), tenant_client() as (b, tb, _): assert (await a.post("/api/e2e/notes", json={"body": "a1"})).status_code == 200 assert (await b.post("/api/e2e/notes", json={"body": "b1"})).status_code == 200 - assert (await a.get("/api/e2e/notes")).json() == [{"body": "a1", "tenant_id": ta["id"]}] - assert (await b.get("/api/e2e/notes")).json() == [{"body": "b1", "tenant_id": tb["id"]}] + assert (await a.get("/api/e2e/notes")).json() == [{"body": "a1", "tenant_id": ta}] + assert (await b.get("/api/e2e/notes")).json() == [{"body": "b1", "tenant_id": tb}] async def test_switching_changes_the_visible_data(notes_app, user_client): @@ -74,13 +72,11 @@ async def test_no_tenant_fails_closed(notes_app, user_client): assert page.headers["location"].startswith("/tenants/?reason=tenant_required") -async def test_removed_member_loses_access_immediately(notes_app, user_client): - async with user_client("o@x.io") as (owner, _), user_client("m@x.io") as (member, member_id): - await _org(owner, "Acme") - token = ( - await owner.post("/api/tenants/current/invitations", json={"email": "m@x.io"}) - ).json()["token"] - await member.post("/api/tenants/invitations/accept", json={"token": token}) +async def test_removed_member_loses_access_immediately(notes_app, tenant_client): + async with ( + tenant_client() as (owner, tenant_id, _), + tenant_client("member", tenant_id=tenant_id) as (member, _, member_id), + ): await owner.post("/api/e2e/notes", json={"body": "secret"}) assert len((await member.get("/api/e2e/notes")).json()) == 1 @@ -88,18 +84,17 @@ async def test_removed_member_loses_access_immediately(notes_app, user_client): assert (await member.get("/api/e2e/notes")).status_code == 403 -async def test_suspended_tenant_is_not_resolved(notes_app, user_client, authenticated_client): - async with user_client("o@x.io") as (owner, _): - tenant = await _org(owner, "Late Payer") +async def test_suspended_tenant_is_not_resolved(notes_app, tenant_client, authenticated_client): + async with tenant_client() as (owner, tenant_id, _): assert (await owner.get("/api/e2e/notes")).status_code == 200 - resp = await authenticated_client.post(f"/api/tenants/admin/{tenant['id']}/suspend") + resp = await authenticated_client.post(f"/api/tenants/admin/{tenant_id}/suspend") assert resp.status_code == 200 and resp.json()["status"] == "suspended" assert (await owner.get("/api/e2e/notes")).status_code == 403 page = await owner.get("/tenants/", headers={"X-Inertia": "true"}) assert page.json()["props"]["suspended"] is True - await authenticated_client.post(f"/api/tenants/admin/{tenant['id']}/reactivate") + await authenticated_client.post(f"/api/tenants/admin/{tenant_id}/reactivate") assert (await owner.get("/api/e2e/notes")).status_code == 200 diff --git a/modules/tenants/tests/test_tenant_client_fixture.py b/modules/tenants/tests/test_tenant_client_fixture.py new file mode 100644 index 00000000..c8f09961 --- /dev/null +++ b/modules/tenants/tests/test_tenant_client_fixture.py @@ -0,0 +1,35 @@ +"""The shared ``tenant_client`` fixture and tenant-role vocabulary (#380).""" + +from __future__ import annotations + +import pytest +from simple_module_core.tenancy import TenantRole, tenant_role + + +@pytest.mark.parametrize("role", list(TenantRole)) +async def test_fixture_creates_a_member_with_the_role(tenant_client, role): + async with tenant_client(role) as (client, tenant_id, user_id): + members = (await client.get("/api/tenants/current/members")).json() + assert [(m["user_id"], m["role"]) for m in members] == [(user_id, role)] + mine = (await client.get("/api/tenants/")).json() + assert [t["id"] for t in mine] == [tenant_id] + + +async def test_second_member_joins_an_existing_tenant(tenant_client): + async with ( + tenant_client() as owner, + tenant_client("admin", tenant_id=owner.tenant_id) as admin, + ): + assert admin.tenant_id == owner.tenant_id + resp = await admin.client.post("/api/tenants/current/invitations", json={"email": "n@x.io"}) + assert resp.status_code == 201 + + +async def test_tenant_roles_reach_the_permission_registry(app): + from tenants.constants import ROLE_PERMISSIONS, TENANT_ROLE_PREFIX, MembershipRole + + assert MembershipRole is TenantRole + assert TENANT_ROLE_PREFIX == "tenant:" + role_map = app.state.sm.permissions.role_map + for role, perms in ROLE_PERMISSIONS.items(): + assert set(perms) <= set(role_map[tenant_role(role)]) diff --git a/modules/tenants/tests/test_tenants_api.py b/modules/tenants/tests/test_tenants_api.py index 1e44500c..1e48d4bb 100644 --- a/modules/tenants/tests/test_tenants_api.py +++ b/modules/tenants/tests/test_tenants_api.py @@ -62,17 +62,15 @@ async def test_member_without_tenant_gets_403_on_tenant_api(user_client): assert resp.status_code == 403 -async def test_last_owner_cannot_leave_or_be_demoted(user_client): - async with user_client("solo@x.io") as (client, user_id): - await _create(client, "Solo") +async def test_last_owner_cannot_leave_or_be_demoted(tenant_client): + async with tenant_client("owner") as (client, _, user_id): assert (await client.delete("/api/tenants/current/membership")).status_code == 409 resp = await client.patch(f"/api/tenants/current/members/{user_id}", json={"role": "admin"}) assert resp.status_code == 409 -async def test_tenant_admin_is_not_platform_admin(user_client): - async with user_client("owner@x.io") as (client, _): - await _create(client, "Mine") +async def test_tenant_admin_is_not_platform_admin(tenant_client): + async with tenant_client("owner") as (client, _, _): assert (await client.get("/api/tenants/admin/")).status_code == 403 assert (await client.get("/admin/tenants/")).status_code in (302, 303, 403) From 8d187cae02deb9c2d0592fef9929ea6d837b44c4 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 14:48:20 +0200 Subject: [PATCH 2/6] fix(db): stamp the bound tenant, else the install's default_tenant, on unscoped writes (review of #380) Non-strict bypass and unbound writes always fell back to DEFAULT_TENANT_ID: an insert with execution_options(all_tenants=True) inside a request dropped the request's tenant, and on a default_tenant install every CLI/all_tenants() write landed in "default", invisible to the install's own scoped requests. Both guards now stamp current_tenant_id if bound, else the engine policy's default_tenant_id, which the host publishes from Settings.default_tenant (single-tenant installs only). Strict mode is unchanged. Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- docs/framework/multi-tenancy.md | 30 +++++-- framework/db/simple_module_db/flush_guard.py | 13 +++- framework/db/simple_module_db/insert_guard.py | 21 ++--- framework/db/simple_module_db/query_filter.py | 34 ++++++-- framework/db/simple_module_db/session.py | 4 + framework/db/simple_module_db/tenancy.py | 5 +- framework/db/tests/conftest.py | 13 +++- framework/db/tests/test_default_tenant.py | 78 +++++++++++++++++++ .../simple_module_hosting/app_builder.py | 4 + .../tests/test_tenant_scope_helpers.py | 10 +++ 10 files changed, 184 insertions(+), 28 deletions(-) diff --git a/docs/framework/multi-tenancy.md b/docs/framework/multi-tenancy.md index 2e2b5c41..2bdf7dc6 100644 --- a/docs/framework/multi-tenancy.md +++ b/docs/framework/multi-tenancy.md @@ -17,8 +17,8 @@ tenant a request acts for. |---|---|---|---| | `SELECT` | filtered to the tenant | `MissingTenantError` | unfiltered | | ORM `update()` / `delete()` | filtered to the tenant; `update().values(tenant_id=…)` raises | `MissingTenantError` | unfiltered | -| `session.add` + flush | `tenant_id` filled in; a different explicit value raises | `MissingTenantError` unless `tenant_id` is set explicitly | `tenant_id` filled in with `DEFAULT_TENANT_ID` unless set | -| ORM `insert(Model)` (bulk / `.values()`) | `tenant_id` filled in; a different explicit value raises | `MissingTenantError` unless every row sets `tenant_id` | `tenant_id` filled in with `DEFAULT_TENANT_ID` unless set | +| `session.add` + flush | `tenant_id` filled in; a different explicit value raises | `MissingTenantError` unless `tenant_id` is set explicitly | `tenant_id` filled in with the install's fallback tenant unless set | +| ORM `insert(Model)` (bulk / `.values()`) | `tenant_id` filled in; a different explicit value raises | `MissingTenantError` unless every row sets `tenant_id` | `tenant_id` filled in with the install's fallback tenant unless set | | Flushing a change to, or a delete of, a loaded object | only if it belongs to the bound tenant | `MissingTenantError` | allowed | | Changing `tenant_id` | raises | raises | raises (only an `all_tenants()` block may move a row) | @@ -126,13 +126,31 @@ briefly on, must not vanish. Strict mode never uses the constant; it raises. To give the single tenant a name of your choosing instead, set `default_tenant` (a `HostSettings` field, e.g. `main`): every request, and every background task with no tenant on its message, then *binds* that tenant -(#359), and inserts are stamped with it rather than the constant. Reads are -then scoped to it as well, so rows already stamped `DEFAULT_TENANT_ID` need -re-stamping when an existing install adopts `default_tenant`. CLI commands -and scripts use `tenant_context(settings.default_tenant)`. It is ignored when +(#359), and inserts are stamped with it rather than the constant. The host +also publishes it as the install's fallback (`DatabaseState.default_tenant_id`), +so writes with nothing bound — a CLI command, an `all_tenants()` block — land +in `main` too instead of in `DEFAULT_TENANT_ID`, where the install's own +(scoped) requests would never see them. CLI commands and scripts should still +prefer `tenant_context(settings.default_tenant)`. It is ignored when `multi_tenant` is on — a multi-tenant install never falls back to a shared tenant. +The fallback only fills a *missing* `tenant_id`, and only when nothing is +bound: a statement run with `execution_options(all_tenants=True)` inside a +request keeps the request's tenant on the rows it inserts. + +Reads under `default_tenant` are scoped to it, so an existing install that +adopts the setting must re-stamp the rows it already wrote under +`DEFAULT_TENANT_ID` (and any it wrote while `multi_tenant` was briefly on), +in a migration or a one-off script, before switching it on: + +```python +op.execute( + sa.text("UPDATE files_file SET tenant_id = :new WHERE tenant_id = :old") + .bindparams(new="main", old=DEFAULT_TENANT_ID) +) +``` + ## Background jobs `background_tasks` stamps the enqueuing request's tenant onto the Celery diff --git a/framework/db/simple_module_db/flush_guard.py b/framework/db/simple_module_db/flush_guard.py index ff960c69..743115af 100644 --- a/framework/db/simple_module_db/flush_guard.py +++ b/framework/db/simple_module_db/flush_guard.py @@ -13,9 +13,8 @@ from sqlalchemy.orm import Session from simple_module_db.mixins import MultiTenantMixin -from simple_module_db.query_filter import is_strict, strict_configured +from simple_module_db.query_filter import fallback_tenant_id, is_strict, strict_configured from simple_module_db.tenancy import ( - DEFAULT_TENANT_ID, TenantIsolationError, current_tenant_id, is_all_tenants, @@ -32,10 +31,16 @@ def _owner(obj: MultiTenantMixin) -> str | None: def _stamp_unbound(session: Session) -> None: - """No tenant bound on a non-strict install: new rows go to the default tenant.""" + """Non-strict and unscoped (unbound, or an ``all_tenants()`` block). + + New rows go to the bound tenant if there is one — a bypass widens what a + block may *read*, it does not move its writes elsewhere — else to the + install's fallback (``default_tenant`` or ``DEFAULT_TENANT_ID``). + """ + stamp = current_tenant_id.get() or fallback_tenant_id(session) for obj in session.new: if isinstance(obj, MultiTenantMixin) and obj.tenant_id is None: - obj.tenant_id = DEFAULT_TENANT_ID + obj.tenant_id = stamp def guard_flush(session: Session) -> None: diff --git a/framework/db/simple_module_db/insert_guard.py b/framework/db/simple_module_db/insert_guard.py index d300a4a3..57f9343d 100644 --- a/framework/db/simple_module_db/insert_guard.py +++ b/framework/db/simple_module_db/insert_guard.py @@ -2,7 +2,7 @@ Explicit ``tenant_id`` values in an insert must match the bound tenant, and missing ones are stamped with it (#357) — or, unbound on a non-strict -install, with ``DEFAULT_TENANT_ID`` (#380); the same value readers let the +install, with the install's fallback tenant (#380); the same value readers let the query filter refuse an UPDATE that assigns ``tenant_id`` (#356). """ @@ -60,7 +60,9 @@ def _foreign_insert(value: Any, tenant_id: str) -> TenantIsolationError: ) -def guard_insert(execute_state: ORMExecuteState, *, strict: bool) -> Any: +def guard_insert( + execute_state: ORMExecuteState, *, strict: bool, fallback: str = DEFAULT_TENANT_ID +) -> Any: mappers = [m.class_.__name__ for m in execute_state.all_mappers if registry.flags(m.class_)[1]] target = getattr(execute_state.statement, "table", None) if not mappers and getattr(target, "name", None) in registry.tenant_table_names: @@ -70,11 +72,13 @@ def guard_insert(execute_state: ORMExecuteState, *, strict: bool) -> Any: bypass = execute_state.execution_options.get(ALL_TENANTS_OPTION, False) or is_all_tenants() if bypass and strict: return None - # Unbound on a non-strict install (bypassed or not): nothing to check - # explicit values against, missing ones get DEFAULT_TENANT_ID (#380). - tenant_id = None if bypass else current_tenant_id.get() + # A non-strict bypass checks no explicit values; missing ones still get + # the bound tenant, else the install's fallback (#380). + bound = current_tenant_id.get() + tenant_id = None if bypass else bound + stamp = bound or fallback if getattr(execute_state.statement, "_multi_values", None): - return _guard_multi_values(execute_state, tenant_id, mappers[0], strict) + return _guard_multi_values(execute_state, tenant_id, stamp, mappers[0], strict) for value in assigned_tenant_ids(execute_state): if tenant_id is not None and value != tenant_id: raise _foreign_insert(value, tenant_id) @@ -88,7 +92,6 @@ def guard_insert(execute_state: ORMExecuteState, *, strict: bool) -> Any: return None if tenant_id is None and strict: raise missing_tenant_error(mappers[0], "INSERT") - stamp = tenant_id or DEFAULT_TENANT_ID if rows: stamped = [{**r, TENANT_COLUMN: r.get(TENANT_COLUMN, stamp)} for r in rows] payload = stamped if isinstance(execute_state.parameters, list) else stamped[0] @@ -98,7 +101,7 @@ def guard_insert(execute_state: ORMExecuteState, *, strict: bool) -> Any: def _guard_multi_values( - execute_state: ORMExecuteState, tenant_id: str | None, entity: str, strict: bool + execute_state: ORMExecuteState, tenant_id: str | None, stamp: str, entity: str, strict: bool ) -> Any: """``insert(M).values([{...}, {...}])``: check and stamp each row. @@ -120,7 +123,7 @@ def _guard_multi_values( raise _foreign_insert(value, tenant_id) else: missing = True - mapping[tenant_col] = tenant_id or DEFAULT_TENANT_ID + mapping[tenant_col] = stamp rows.append(mapping) groups.append(rows) if not missing: diff --git a/framework/db/simple_module_db/query_filter.py b/framework/db/simple_module_db/query_filter.py index 455f3755..ef6d0004 100644 --- a/framework/db/simple_module_db/query_filter.py +++ b/framework/db/simple_module_db/query_filter.py @@ -23,6 +23,7 @@ from simple_module_db.subquery_guard import scope_exists_subqueries from simple_module_db.tenancy import ( ALL_TENANTS_OPTION, + DEFAULT_TENANT_ID, TenantIsolationError, current_tenant_id, is_all_tenants, @@ -34,16 +35,18 @@ # Per-engine tenancy policy. Keyed by engine rather than held in a module # global so two ``DatabaseState``s in one process (tests, a CLI next to an # app) cannot switch each other's strict mode off. Values expose -# ``tenant_strict`` — a ``DatabaseState``, or an ``EngineTenancy`` for a bare -# sync engine such as the Celery worker's. +# ``tenant_strict`` (and optionally ``default_tenant_id``) — a +# ``DatabaseState``, or an ``EngineTenancy`` for a bare sync engine such as +# the Celery worker's. _engine_policy: WeakKeyDictionary[Engine, Any] = WeakKeyDictionary() class EngineTenancy: """Tenancy policy for an engine that has no ``DatabaseState``.""" - def __init__(self, *, tenant_strict: bool) -> None: + def __init__(self, *, tenant_strict: bool, default_tenant_id: str = DEFAULT_TENANT_ID) -> None: self.tenant_strict = tenant_strict + self.default_tenant_id = default_tenant_id def bind_engine_policy(engine: Engine, policy: Any) -> None: @@ -51,12 +54,26 @@ def bind_engine_policy(engine: Engine, policy: Any) -> None: _engine_policy[engine] = policy -def strict_configured(session: Session) -> bool: +def _policy(session: Session) -> Any: bind = session.bind - policy = _engine_policy.get(bind) if isinstance(bind, Engine) else None + return _engine_policy.get(bind) if isinstance(bind, Engine) else None + + +def strict_configured(session: Session) -> bool: + policy = _policy(session) return bool(policy is not None and policy.tenant_strict) +def fallback_tenant_id(session: Session) -> str: + """The tenant a non-strict write lands in when none is bound. + + The install's ``default_tenant`` when the host set one (published as + ``DatabaseState.default_tenant_id``), else :data:`DEFAULT_TENANT_ID`. + """ + policy = _policy(session) + return getattr(policy, "default_tenant_id", None) or DEFAULT_TENANT_ID + + def is_strict(session: Session) -> bool: """Strict isolation is on for this session's engine and not waived.""" return strict_configured(session) and not is_all_tenants() @@ -76,7 +93,12 @@ def filter_statements(execute_state: ORMExecuteState) -> Any: ``all_tenants()``; soft-delete bypass: ``include_deleted=True``. """ if execute_state.is_insert: - return guard_insert(execute_state, strict=strict_configured(execute_state.session)) + session = execute_state.session + return guard_insert( + execute_state, + strict=strict_configured(session), + fallback=fallback_tenant_id(session), + ) is_select = execute_state.is_select if not (is_select or execute_state.is_update or execute_state.is_delete): return None diff --git a/framework/db/simple_module_db/session.py b/framework/db/simple_module_db/session.py index 58fbcd8e..6971732f 100644 --- a/framework/db/simple_module_db/session.py +++ b/framework/db/simple_module_db/session.py @@ -16,6 +16,7 @@ from simple_module_db.callbacks import OnCommitCallback, register_on_commit from simple_module_db.provider import DatabaseProvider, detect_provider +from simple_module_db.tenancy import DEFAULT_TENANT_ID class RequestSession(AsyncSession): @@ -42,6 +43,9 @@ class DatabaseState: # raises instead of reading every tenant. The host turns this on whenever # it runs with ``multi_tenant``; see ``simple_module_db.tenancy``. tenant_strict: bool = False + # Where a non-strict write with no tenant bound (or inside ``all_tenants()``) + # lands: the host's ``default_tenant``, else ``DEFAULT_TENANT_ID``. + default_tenant_id: str = DEFAULT_TENANT_ID _listeners_registered: bool = field(default=False, repr=False) diff --git a/framework/db/simple_module_db/tenancy.py b/framework/db/simple_module_db/tenancy.py index f6a98030..df7226b4 100644 --- a/framework/db/simple_module_db/tenancy.py +++ b/framework/db/simple_module_db/tenancy.py @@ -52,8 +52,9 @@ A single-tenant install (``multi_tenant`` off) has no tenant to bind, but ``MultiTenantMixin.tenant_id`` is NOT NULL, so inserts are stamped with this. Adoption migrations backfill existing rows with the same value. It is not the -``default_tenant`` host setting: that one *binds* a tenant per request; this -is only the fallback when nothing is bound. Strict mode never uses it. +``default_tenant`` host setting: that one *binds* a tenant per request, and +when set it replaces this constant as the fallback +(``DatabaseState.default_tenant_id``). Strict mode never uses either. """ diff --git a/framework/db/tests/conftest.py b/framework/db/tests/conftest.py index 3e06bb26..55191436 100644 --- a/framework/db/tests/conftest.py +++ b/framework/db/tests/conftest.py @@ -14,9 +14,13 @@ _URL = database_url_for_tests() -async def _tenant_session(*, strict: bool) -> AsyncGenerator[AsyncSession, None]: +async def _tenant_session( + *, strict: bool, default_tenant: str | None = None +) -> AsyncGenerator[AsyncSession, None]: db_state = init_db(_URL, **init_db_kwargs(_URL)) db_state.tenant_strict = strict + if default_tenant: + db_state.default_tenant_id = default_tenant try: register_listeners(db_state) await reset_schema(db_state.engine) @@ -41,3 +45,10 @@ async def strict_session() -> AsyncGenerator[AsyncSession, None]: """Like ``tenant_session``, with fail-closed isolation (``multi_tenant`` on).""" async for session in _tenant_session(strict=True): yield session + + +@pytest.fixture +async def acme_default_session() -> AsyncGenerator[AsyncSession, None]: + """Single-tenant install with ``default_tenant="acme"``.""" + async for session in _tenant_session(strict=False, default_tenant="acme"): + yield session diff --git a/framework/db/tests/test_default_tenant.py b/framework/db/tests/test_default_tenant.py index dcad54c6..a5a48f3b 100644 --- a/framework/db/tests/test_default_tenant.py +++ b/framework/db/tests/test_default_tenant.py @@ -13,9 +13,11 @@ DEFAULT_TENANT_ID, TenantIsolationError, all_tenants, + current_tenant_id, tenant_context, ) from sqlalchemy import insert, select +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession @@ -84,3 +86,79 @@ async def test_strict_mode_does_not_fall_back(strict_session: AsyncSession): await strict_session.rollback() with pytest.raises(TenantIsolationError, match="INSERT"): await strict_session.execute(insert(_TenantItem).values(name="orphan")) + + +async def _insert_all_shapes(session: AsyncSession) -> set[str]: + session.add(_TenantItem(name="added")) + await session.flush() + await session.execute(insert(_TenantItem).values(name="core-one")) + await session.execute(insert(_TenantItem), [{"name": "bulk-a"}]) + await session.execute(insert(_TenantItem).values([{"name": "mv-a"}])) + await session.execute(insert(_TenantItem.__table__).values(name="table")) + stmt = select(_TenantItem).execution_options(all_tenants=True) + return {r.tenant_id for r in (await session.execute(stmt)).scalars().all()} + + +async def test_bypassed_insert_with_a_bound_tenant_keeps_the_bound_tenant( + tenant_session: AsyncSession, +): + """``execution_options(all_tenants=True)`` widens a statement's scope; a + missing ``tenant_id`` still goes to the bound tenant, not the fallback.""" + with tenant_context("acme"): + for stmt in ( + insert(_TenantItem).values(name="core"), + insert(_TenantItem).values([{"name": "mv"}]), + insert(_TenantItem.__table__).values(name="table"), + ): + await tenant_session.execute(stmt.execution_options(all_tenants=True)) + await tenant_session.execute( + insert(_TenantItem).execution_options(all_tenants=True), [{"name": "bulk"}] + ) + stmt = select(_TenantItem).execution_options(all_tenants=True) + rows = (await tenant_session.execute(stmt)).scalars().all() + assert len(rows) == 4 + assert {r.tenant_id for r in rows} == {"acme"} + + +async def test_flush_in_bypass_with_a_bound_tenant_keeps_the_bound_tenant( + tenant_session: AsyncSession, +): + """``all_tenants()`` clears the tenant, but code that binds one inside the + bypass (a restored context, ``bind_current_tenant``) keeps its writes.""" + with all_tenants(): + token = current_tenant_id.set("acme") + try: + item = _TenantItem(name="added") + tenant_session.add(item) + await tenant_session.flush() + finally: + current_tenant_id.reset(token) + assert item.tenant_id == "acme" + + +async def test_default_tenant_install_stamps_it_when_unbound( + acme_default_session: AsyncSession, +): + assert await _insert_all_shapes(acme_default_session) == {"acme"} + + +async def test_default_tenant_install_stamps_it_inside_all_tenants( + acme_default_session: AsyncSession, +): + with all_tenants(): + assert await _insert_all_shapes(acme_default_session) == {"acme"} + + +async def test_plain_install_stamps_default_inside_all_tenants(tenant_session: AsyncSession): + with all_tenants(): + assert await _insert_all_shapes(tenant_session) == {DEFAULT_TENANT_ID} + + +async def test_strict_bypass_leaves_missing_tenant_to_the_database( + strict_session: AsyncSession, +): + """Strict is unchanged: an all_tenants() insert is not stamped.""" + with all_tenants(): + strict_session.add(_TenantItem(name="orphan")) + with pytest.raises(IntegrityError): + await strict_session.flush() diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index def295a6..fc0fb7a5 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -240,6 +240,10 @@ def create_app(settings: Settings | None = None) -> FastAPI: # Multi-tenant installs fail closed: a tenant-scoped query with no tenant # context raises instead of reading every tenant's rows. db_state.tenant_strict = settings.multi_tenant + # Where unbound / all_tenants() writes land on a single-tenant install: + # its ``default_tenant`` when set, so they stay visible to its requests. + if not settings.multi_tenant and settings.default_tenant: + db_state.default_tenant_id = settings.default_tenant register_listeners(db_state) # The host's own readiness signal, and the only probe-safe check in a # default install — module checks reach third parties and are on-demand. diff --git a/framework/hosting/tests/test_tenant_scope_helpers.py b/framework/hosting/tests/test_tenant_scope_helpers.py index d4314178..90000208 100644 --- a/framework/hosting/tests/test_tenant_scope_helpers.py +++ b/framework/hosting/tests/test_tenant_scope_helpers.py @@ -40,6 +40,16 @@ def test_default_tenant_is_ignored_when_multi_tenant(): assert "fixed" not in mw.kwargs +@pytest.mark.parametrize( + ("multi_tenant", "default_tenant", "expected"), + [(False, "main", "main"), (False, "", "default"), (True, "main", "default")], +) +def test_default_tenant_is_published_as_the_write_fallback(multi_tenant, default_tenant, expected): + """Unbound / all_tenants() writes land in the install's own tenant.""" + app = create_app(_settings(multi_tenant=multi_tenant, default_tenant=default_tenant)) + assert app.state.sm.db.default_tenant_id == expected + + def test_no_tenant_middleware_without_either(): assert _tenant_middleware(create_app(_settings(multi_tenant=False))) == [] From 1a6c08c317b40f9a539930e8ee89bb4e501e354d Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 15:04:14 +0200 Subject: [PATCH 3/6] docs(tenancy): ruff-format the re-stamp example (review of #380) Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- docs/framework/multi-tenancy.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/framework/multi-tenancy.md b/docs/framework/multi-tenancy.md index 2bdf7dc6..0d616cd1 100644 --- a/docs/framework/multi-tenancy.md +++ b/docs/framework/multi-tenancy.md @@ -146,8 +146,9 @@ in a migration or a one-off script, before switching it on: ```python op.execute( - sa.text("UPDATE files_file SET tenant_id = :new WHERE tenant_id = :old") - .bindparams(new="main", old=DEFAULT_TENANT_ID) + sa.text("UPDATE files_file SET tenant_id = :new WHERE tenant_id = :old").bindparams( + new="main", old=DEFAULT_TENANT_ID + ) ) ``` From 80686d4b16d64f7cfce2d0a074002caa020b6942 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 15:14:19 +0200 Subject: [PATCH 4/6] fix(testing): tenant_client seeds and attaches the user role (review of #380) create_user looked the 'user' Role up and silently skipped the UserRole when the row was absent, which it always is under create_all. Extract users.bootstrap.ensure_user_role (shared with create_standard_user) and use it so the fixture user really holds the platform role. Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- .../simple_module_test/tenant_client.py | 11 +++--- .../tests/test_tenant_client_fixture.py | 19 +++++++++++ modules/users/users/bootstrap.py | 34 ++++++++++++------- 3 files changed, 47 insertions(+), 17 deletions(-) diff --git a/framework/testing/simple_module_test/tenant_client.py b/framework/testing/simple_module_test/tenant_client.py index 430f1192..829fc58c 100644 --- a/framework/testing/simple_module_test/tenant_client.py +++ b/framework/testing/simple_module_test/tenant_client.py @@ -32,7 +32,6 @@ async def test_isolation(tenant_client): import httpx import pytest from simple_module_core.tenancy import TenantRole -from sqlalchemy import select from simple_module_test.session_cookie import forge_session_cookie @@ -45,7 +44,8 @@ class TenantClient(NamedTuple): async def create_user(app: Any, email: str) -> str: """A real, active, non-superuser ``users`` row with the ``user`` role; its id.""" - from users.models import Role, User, UserRole + from users.bootstrap import ensure_user_role + from users.models import User, UserRole async with app.state.sm.db.session_factory() as session: user = User( @@ -58,9 +58,10 @@ async def create_user(app: Any, email: str) -> str: ) session.add(user) await session.flush() - role = (await session.execute(select(Role).where(Role.name == "user"))).scalar_one_or_none() - if role is not None: - session.add(UserRole(user_id=user.id, role_id=role.id)) + # Seeded lazily: test schemas come from ``create_all`` with no seed rows, + # and a missing role must not silently leave the user role-less. + role = await ensure_user_role(session) + session.add(UserRole(user_id=user.id, role_id=role.id)) await session.commit() return str(user.id) diff --git a/modules/tenants/tests/test_tenant_client_fixture.py b/modules/tenants/tests/test_tenant_client_fixture.py index c8f09961..3d3f5b6e 100644 --- a/modules/tenants/tests/test_tenant_client_fixture.py +++ b/modules/tenants/tests/test_tenant_client_fixture.py @@ -33,3 +33,22 @@ async def test_tenant_roles_reach_the_permission_registry(app): role_map = app.state.sm.permissions.role_map for role, perms in ROLE_PERMISSIONS.items(): assert set(perms) <= set(role_map[tenant_role(role)]) + + +async def test_fixture_user_holds_the_platform_user_role(app, tenant_client): + """The ``user`` Role row is seeded on demand, never silently skipped.""" + from sqlalchemy import select + from users.constants import USER_ROLE_NAME + from users.models import Role, UserRole + + async with tenant_client("member") as (_, _, user_id): + pass + async with app.state.sm.db.session_factory() as session: + names = ( + await session.execute( + select(Role.name) + .join(UserRole, UserRole.role_id == Role.id) + .where(UserRole.user_id == user_id) + ) + ).scalars() + assert list(names) == [USER_ROLE_NAME] diff --git a/modules/users/users/bootstrap.py b/modules/users/users/bootstrap.py index 5e6883fb..d6c7ba8d 100644 --- a/modules/users/users/bootstrap.py +++ b/modules/users/users/bootstrap.py @@ -134,6 +134,27 @@ async def create_admin( return CreateAdminResult(user=existing, created=False) +async def ensure_user_role(db: AsyncSession) -> Role: + """The standard ``user`` Role row, created (and flushed) if it is missing. + + The seed migration normally inserts it; tests build the schema with + ``create_all`` and get no seed rows, so callers that attach the role must + not assume it exists. + """ + user_role = ( + await db.execute(select(Role).where(Role.name == USER_ROLE_NAME)) + ).scalar_one_or_none() + if user_role is None: + user_role = ( + await db.execute(select(Role).where(Role.id == USER_ROLE_ID)) + ).scalar_one_or_none() + if user_role is None: + user_role = Role(id=USER_ROLE_ID, name=USER_ROLE_NAME, description=USER_ROLE_DESCRIPTION) + db.add(user_role) + await db.flush() + return user_role + + async def create_standard_user( db: AsyncSession, *, @@ -153,18 +174,7 @@ async def create_standard_user( logger.info(_EVT_USER_NOOP, extra={"email": email, "id": str(existing.id)}) return CreateAdminResult(user=existing, created=False) - user_role = ( - await db.execute(select(Role).where(Role.name == USER_ROLE_NAME)) - ).scalar_one_or_none() - if user_role is None: - user_role = ( - await db.execute(select(Role).where(Role.id == USER_ROLE_ID)) - ).scalar_one_or_none() - if user_role is None: - # Safety net — the seed migration normally inserts this row. - user_role = Role(id=USER_ROLE_ID, name=USER_ROLE_NAME, description=USER_ROLE_DESCRIPTION) - db.add(user_role) - await db.flush() + user_role = await ensure_user_role(db) user = User( email=email, From f816296d4117cbb3a834da8eaebaab54be7854d6 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 16:55:50 +0200 Subject: [PATCH 5/6] fix(tenants): validate invite emails and enforce one open invite per address in the DB (qa BUG-003, BUG-004) Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- ...e4_tenants_invitation_open_email_unique.py | 44 +++++++++++++++++++ modules/tenants/tenants/contracts/schemas.py | 12 +++-- modules/tenants/tenants/invitations.py | 21 ++++++++- modules/tenants/tenants/models.py | 16 ++++++- modules/tenants/tests/test_invitations.py | 28 ++++++++++++ modules/tenants/tests/test_owner_race.py | 34 +++++++++++++- 6 files changed, 147 insertions(+), 8 deletions(-) create mode 100644 host/migrations/versions/f1a9c3d7b2e4_tenants_invitation_open_email_unique.py diff --git a/host/migrations/versions/f1a9c3d7b2e4_tenants_invitation_open_email_unique.py b/host/migrations/versions/f1a9c3d7b2e4_tenants_invitation_open_email_unique.py new file mode 100644 index 00000000..354e21b1 --- /dev/null +++ b/host/migrations/versions/f1a9c3d7b2e4_tenants_invitation_open_email_unique.py @@ -0,0 +1,44 @@ +"""tenants_invitation: one open invitation per (tenant, email) + +Concurrent invites to one address each passed the service's duplicate check and +each inserted a row. A partial unique index makes the database the arbiter. +Existing duplicates are collapsed first (newest kept), or the index would fail. + +Revision ID: f1a9c3d7b2e4 +Revises: e36ab09c4a92 +Create Date: 2026-10-01 18:00:00.000000 +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +revision: str = "f1a9c3d7b2e4" +down_revision: str | None = "e36ab09c4a92" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +_INDEX = "uq_tenants_invitation_open_email" + + +def upgrade() -> None: + op.execute( + sa.text( + "DELETE FROM tenants_invitation WHERE accepted_at IS NULL AND id NOT IN (" + " SELECT MAX(id) FROM tenants_invitation WHERE accepted_at IS NULL" + " GROUP BY tenant_id, email)" + ) + ) + op.create_index( + _INDEX, + "tenants_invitation", + ["tenant_id", "email"], + unique=True, + sqlite_where=sa.text("accepted_at IS NULL"), + postgresql_where=sa.text("accepted_at IS NULL"), + ) + + +def downgrade() -> None: + op.drop_index(_INDEX, table_name="tenants_invitation") diff --git a/modules/tenants/tenants/contracts/schemas.py b/modules/tenants/tenants/contracts/schemas.py index 8e225ec2..9fd65205 100644 --- a/modules/tenants/tenants/contracts/schemas.py +++ b/modules/tenants/tenants/contracts/schemas.py @@ -5,6 +5,7 @@ import re from datetime import datetime +from email_validator import EmailNotValidError, validate_email from pydantic import field_validator from sqlmodel import Field, SQLModel @@ -67,10 +68,13 @@ class InvitationCreate(SQLModel): @field_validator("email") @classmethod def _normalise(cls, value: str) -> str: - value = value.strip().lower() - if "@" not in value: - raise ValueError("invalid email") - return value + try: + checked = validate_email( + value.strip(), check_deliverability=False, test_environment=True + ) + except EmailNotValidError as exc: + raise ValueError("invalid email") from exc + return checked.normalized.lower() @field_validator("role") @classmethod diff --git a/modules/tenants/tenants/invitations.py b/modules/tenants/tenants/invitations.py index a5fc79b3..a57e80e1 100644 --- a/modules/tenants/tenants/invitations.py +++ b/modules/tenants/tenants/invitations.py @@ -11,7 +11,8 @@ import secrets from datetime import UTC, datetime, timedelta -from sqlalchemy import func, select +from sqlalchemy import delete, func, select +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from tenants.constants import MembershipRole, TenantStatus @@ -63,6 +64,16 @@ async def create( tenant = await self.tenants.lock(tenant_id) if await self.tenants.has_member_email(tenant_id, data.email): raise TenantError("already_member", status_code=409) + # An expired, unaccepted invite no longer counts as pending but would + # still occupy the unique index; drop it so the address can be re-invited. + await self.db.execute( + delete(Invitation).where( + Invitation.tenant_id == tenant_id, + Invitation.email == data.email, + Invitation.accepted_at.is_(None), + Invitation.expires_at <= _now(), + ) + ) duplicate = await self.db.scalar( select(Invitation.id).where(_pending_clause(tenant_id), Invitation.email == data.email) ) @@ -78,7 +89,13 @@ async def create( expires_at=_now() + self.ttl, ) self.db.add(invitation) - await self.db.flush() + try: + await self.db.flush() + except IntegrityError as exc: + # A concurrent invite for the same address won the race past the + # read above; the partial unique index caught it. The request + # fails as a whole, so no savepoint is needed. + raise TenantError("already_invited", status_code=409) from exc accept_url = f"{base_url.rstrip('/')}{ACCEPT_PATH}?token={token}" self.tenants._after_commit( InvitationCreated(tenant_id, tenant.name, data.email, data.role, accept_url) diff --git a/modules/tenants/tenants/models.py b/modules/tenants/tenants/models.py index 92fbcd32..4d572437 100644 --- a/modules/tenants/tenants/models.py +++ b/modules/tenants/tenants/models.py @@ -13,7 +13,7 @@ from simple_module_db.base import create_module_base from simple_module_db.mixins import AuditMixin -from sqlalchemy import Column, DateTime, Index, UniqueConstraint +from sqlalchemy import Column, DateTime, Index, UniqueConstraint, text from sqlmodel import Field from tenants.constants import ( @@ -71,6 +71,20 @@ class Invitation(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] """A pending invitation. Only the token's SHA-256 is stored.""" __tablename__ = "tenants_invitation" + __table_args__ = ( + # One open invitation per address: the service's duplicate check is a + # read, so concurrent requests need the database to say no (SQLite + # has no row lock to serialise them). Expired rows are reaped by the + # service before a re-invite, so only ``accepted_at`` is in the predicate. + Index( + "uq_tenants_invitation_open_email", + "tenant_id", + "email", + unique=True, + sqlite_where=text("accepted_at IS NULL"), + postgresql_where=text("accepted_at IS NULL"), + ), + ) id: int | None = Field(default=None, primary_key=True) tenant_id: str = Field( diff --git a/modules/tenants/tests/test_invitations.py b/modules/tenants/tests/test_invitations.py index 1958644a..753ee3d4 100644 --- a/modules/tenants/tests/test_invitations.py +++ b/modules/tenants/tests/test_invitations.py @@ -93,3 +93,31 @@ async def test_revoked_invitation_cannot_be_accepted(user_client): ).status_code == 204 resp = await new.post("/api/tenants/invitations/accept", json={"token": issued["token"]}) assert resp.status_code == 404 + + +async def test_malformed_invite_emails_are_rejected(user_client): + """qa BUG-003: only an "@" was checked, so junk reached the table.""" + async with user_client("owner@x.io") as (owner, _): + await _setup(owner) + for bad in ("a b@x.com", "@x.com", "@x.com", "nodomain@", "a@b"): + r = await owner.post("/api/tenants/current/invitations", json={"email": bad}) + assert r.status_code == 422, (bad, r.text) + + +async def test_open_invitation_is_unique_in_the_database(db_session): + """The partial index itself, independent of the service pre-check.""" + from datetime import UTC, datetime, timedelta + + import pytest + from sqlalchemy.exc import IntegrityError + from tenants.models import Invitation, Tenant + + tenant = Tenant(name="T", slug="t-uniq") + db_session.add(tenant) + await db_session.flush() + exp = datetime.now(UTC) + timedelta(days=1) + db_session.add(Invitation(tenant_id=tenant.id, email="a@x.io", token_hash="h1", expires_at=exp)) + await db_session.flush() + db_session.add(Invitation(tenant_id=tenant.id, email="a@x.io", token_hash="h2", expires_at=exp)) + with pytest.raises(IntegrityError): + await db_session.flush() diff --git a/modules/tenants/tests/test_owner_race.py b/modules/tenants/tests/test_owner_race.py index d382c742..97664220 100644 --- a/modules/tenants/tests/test_owner_race.py +++ b/modules/tenants/tests/test_owner_race.py @@ -23,8 +23,9 @@ reset_schema, ) from tenants.constants import MembershipRole -from tenants.contracts.schemas import TenantCreate +from tenants.contracts.schemas import InvitationCreate, TenantCreate from tenants.errors import TenantError +from tenants.invitations import InvitationService from tenants.models import Base from tenants.service import TenantService @@ -100,3 +101,34 @@ async def demote(user_id: str) -> str: owners = await TenantService(db)._owner_count(tenant.id) assert sorted(outcomes) == ["last_owner", "ok"] assert owners == 1 + + +@pytest.mark.parametrize("backend", _backends()) +async def test_concurrent_invites_to_one_address_give_one(tmp_path, backend: str): + """qa BUG-004: the loser of an invite race gets ``already_invited``, not a second row.""" + async with _database(tmp_path, backend) as state: + async with state.session_factory() as db: + tenant = await TenantService(db).create_tenant( + TenantCreate(name="Inv"), owner_user_id="a" + ) + await db.commit() + + async def invite() -> str: + async with state.session_factory() as db: + try: + await InvitationService(TenantService(db), ttl_hours=1).create( + tenant.id, InvitationCreate(email="dup@x.io"), base_url="http://t" + ) + await db.commit() + return "ok" + except TenantError as exc: + await db.rollback() + return exc.code + + outcomes = await asyncio.gather(*(invite() for _ in range(5))) + assert sorted(outcomes) == ["already_invited"] * 4 + ["ok"] + async with state.session_factory() as db: + pending = await InvitationService(TenantService(db), ttl_hours=1).list_pending( + tenant.id + ) + assert len(pending) == 1 From 7ad5870b7db63ce1c82986f18754ab8a178a3d74 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 17:13:54 +0200 Subject: [PATCH 6/6] fix(tenants): declare email-validator, imported by the invite schema (qa review) Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- modules/tenants/pyproject.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/tenants/pyproject.toml b/modules/tenants/pyproject.toml index 1b04f9e4..1fc5f652 100644 --- a/modules/tenants/pyproject.toml +++ b/modules/tenants/pyproject.toml @@ -16,6 +16,7 @@ dependencies = [ "simple_module_settings==0.0.35", "simple_module_auth==0.0.35", "cachetools>=5.3", + "email-validator>=2.1", ] [project.entry-points.simple_module]