feat(file_storage): adopt MultiTenantMixin on StoredFile (#383) [stack 9/11]

docs/framework/multi-tenancy.md

@@ -118,6 +118,12 @@ op.execute(sa.text("UPDATE files_file SET tenant_id = :t").bindparams(t=DEFAULT_
 op.alter_column("files_file", "tenant_id", nullable=False)
 ```
 
+`PLATFORM_TENANT_ID` (`"platform"`) is the other reserved value: the owner of
+rows that belong to the install rather than to a tenant (`file_storage`'s
+platform files). `is_valid_tenant_id` refuses it, so nothing can be bound to it
+and no organisation or `default_tenant` can take it; a re-stamp script must
+leave those rows alone.
+
 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

docs/modules/branding.md

@@ -50,6 +50,14 @@ Registered through the [`register_public_routes`](/framework/public-routes) hook
 
 Branding serves these itself rather than linking `file_storage`'s download route, which is gated by `file_storage.download` — no logged-out visitor carries that permission, and the sign-in page, the public landing page and every `<link rel="icon">` are exactly where the logo has to appear. Each route resolves **only** the id currently held in branding settings and streams that one file, so it is not a way to read arbitrary files out of `file_storage`.
 
+Branding images are **platform files** (`platform=True` in `file_storage`):
+uploaded as the install rather than as the admin's active organisation, and
+served to anonymous visitors — who have no tenant bound — by a lookup that only
+ever matches platform-owned rows. A setting pointed at a tenant's upload
+therefore `404`s instead of publishing it. Images uploaded before
+`file_storage` adopted tenancy were back-filled into the platform owner and
+keep working.
+
 Responses carry `Content-Disposition: attachment` and `X-Content-Type-Options: nosniff`. Both are ignored for subresource loads (`<img>`, `<link rel="icon">`) but stop a direct visit rendering the bytes as a document at the app's own origin.
 
 When `file_storage` is backed by S3-compatible storage, the route returns a `302` to a presigned URL. That redirect is deliberately **uncached** — the target expires, so caching it would hand out a dead link after the TTL.

docs/modules/file_storage.md

@@ -62,7 +62,8 @@ from file_storage.contracts import (
 | Column | Type | Notes |
 |---|---|---|
 | `id` | `UUID` | PK |
-| `key` | `str(512)` | backend-relative path; **unique** |
+| `tenant_id` | `str(50)` | owning tenant, from `MultiTenantMixin` |
+| `key` | `str(512)` | backend-relative path; **unique per tenant** |
 | `filename` | `str(255)` | original upload filename |
 | `content_type` | `str(128)` | sniffed / declared MIME type |
 | `size_bytes` | `int` | |
@@ -71,7 +72,56 @@ from file_storage.contracts import (
 | `extra_metadata` | `dict` | per-backend extras |
 | audit + soft-delete | from `AuditMixin` + `SoftDeleteMixin` | |
 
-Indexes on `key` (unique), `created_by`, `is_deleted`.
+Indexes on `(tenant_id, key)` (unique), `tenant_id`, `created_by`, `is_deleted`.
+
+## Multi-tenancy
+
+`StoredFile` is [`MultiTenantMixin`](/framework/multi-tenancy) (#383). Every
+route acts for the request's active tenant: another tenant's file id answers
+`404` exactly like an unknown one, listings and the browse screen's totals and
+facets count only the tenant's rows, and bulk delete skips ids it cannot see.
+The aggregate cache is keyed per tenant, and a write drops only the slots of
+the tenants it wrote (plus the unscoped slot).
+
+New storage keys are `{tenant_id}/YYYY/MM/DD/<uuid><ext>`, so each tenant's
+objects live under their own backend prefix. Rows written before the adoption
+migration keep their un-prefixed key (the column stores the full path) and were
+back-filled into `DEFAULT_TENANT_ID` (branding's system images into the
+platform owner, below). A single-tenant install (`multi_tenant`
+off) stamps uploads with `DEFAULT_TENANT_ID` and reads every row; with
+`multi_tenant` on and no tenant bound, an upload fails closed *before* any
+bytes reach the backend.
+
+**Platform files.** Files that belong to the install rather than to a tenant —
+branding's logo and favicon — are written and read with `platform=True` on
+`FileStorageService.upload` / `get` / `download` / `delete`. They are owned by
+`PLATFORM_TENANT_ID` (`"platform"`, exported by `simple_module_db`) and looked
+up under `all_tenants()` **restricted to that owner**, so they resolve from
+anonymous requests while no tenant's file can be reached that way. The id is
+reserved — `is_valid_tenant_id` refuses it, so no request, header, claim, task
+message, `default_tenant` setting or `tenants` organisation can ever be bound
+to it — and it is distinct from `DEFAULT_TENANT_ID`, the owner of a
+single-tenant install's ordinary rows. The adoption migration made only the
+files the system branding settings referenced platform files; every other
+existing row went to `DEFAULT_TENANT_ID`. **Never re-stamp platform rows**: a
+script adopting `default_tenant` must update `WHERE tenant_id = 'default'`,
+never every row.
+
+The bypass covers only the platform row. `platform_scope` flushes the
+session's other pending writes *before* lifting isolation — so they are
+stamped with, and checked against, the bound tenant as usual — and turns
+autoflush off inside, so a platform read cannot carry them through unguarded.
+
+Platform files are not listed on any tenant's Files screen.
+
+The audit-log label resolver names files across tenants on purpose: the audit
+log is a platform screen over every tenant's entries, each of which already
+records the filename.
+
+The module has no background jobs, sweeps or CLI commands. Anything added
+later that touches `StoredFile` outside a request must run under
+`tenant_context(row.tenant_id)` for per-tenant work, or `all_tenants()` for a
+deliberate platform-wide sweep.
 
 ## Settings
 
@@ -125,16 +175,22 @@ Register it in your module's `register_settings` so the file_storage service can
 
 | Code | Granted to | Purpose |
 |---|---|---|
-| `file_storage.upload` | `user`, `admin` | upload files |
-| `file_storage.download` | `user`, `admin` | list / get / download |
-| `file_storage.delete` | `user`, `admin` | delete |
+| `file_storage.upload` | `user`, `admin`, `tenant:member`/`admin`/`owner` | upload files |
+| `file_storage.download` | `user`, `admin`, `tenant:member`/`admin`/`owner` | list / get / download |
+| `file_storage.delete` | `admin`, `tenant:admin`, `tenant:owner`; also `user` when `multi_tenant` is off | delete |
 | `file_storage.manage` | `admin` | reserved for future admin operations |
 
+With `multi_tenant` on, delete is an organisation-admin act: the platform
+`user` role, which every account holds, does not carry it (it would hand it to
+every tenant member). With `multi_tenant` off the install is the only tenant,
+so `on_startup` maps `file_storage.delete` onto `user` and ordinary users keep
+deleting their files.
+
 ## Menu
 
 | Label | URL | Icon | Section | Group | Order | Roles |
 |---|---|---|---|---|---|---|
-| `Files` | `/file-storage` | `files` | `SIDEBAR` | `Content` | `40` | `["admin"]` |
+| `Files` | `/file-storage` | `files` | `SIDEBAR` | `Content` | `40` | `["admin", "tenant:owner", "tenant:admin", "tenant:member"]` |
 
 ## Events
 

framework/db/simple_module_db/__init__.py

@@ -17,6 +17,7 @@
 from simple_module_db.tenancy import (
     ALL_TENANTS_OPTION,
     DEFAULT_TENANT_ID,
+    PLATFORM_TENANT_ID,
     TENANT_ID_PATTERN,
     MissingTenantError,
     TenantIsolationError,
@@ -33,6 +34,7 @@
     "ALL_TENANTS_OPTION",
     "DEFAULT_TENANT_ID",
     "LIKE_ESCAPE_CHAR",
+    "PLATFORM_TENANT_ID",
     "TENANT_ID_PATTERN",
     "AuditMixin",
     "AuditRecord",

framework/db/simple_module_db/tenancy.py

@@ -58,8 +58,25 @@
 """
 
 
+PLATFORM_TENANT_ID = "platform"
+"""Reserved owner of rows that belong to the install, not to any tenant.
+
+``file_storage`` stamps platform files (branding's system logo and favicon)
+with it and reads them under ``all_tenants()`` restricted to this owner. It is
+distinct from :data:`DEFAULT_TENANT_ID` — the single-tenant fallback every
+unbound insert lands in — so a platform lookup can never reach an ordinary
+single-tenant row. :func:`is_valid_tenant_id` refuses it: no request, header,
+claim, task message or ``default_tenant`` setting can bind it, and no tenant
+can be created with it.
+"""
+
+
 def is_valid_tenant_id(value: object) -> bool:
-    return isinstance(value, str) and TENANT_ID_PATTERN.fullmatch(value) is not None
+    return (
+        isinstance(value, str)
+        and value != PLATFORM_TENANT_ID
+        and TENANT_ID_PATTERN.fullmatch(value) is not None
+    )
 
 
 class TenantIsolationError(Exception):
@@ -170,6 +187,7 @@ def missing_tenant_error(entity: str, operation: str) -> MissingTenantError:
 __all__ = [
     "ALL_TENANTS_OPTION",
     "DEFAULT_TENANT_ID",
+    "PLATFORM_TENANT_ID",
     "TENANT_ID_PATTERN",
     "MissingTenantError",
     "TenantIsolationError",

framework/hosting/simple_module_hosting/host_settings.py

@@ -132,9 +132,11 @@ def _normalize_trusted_proxy(cls, value: str | None) -> str | None:
     @field_validator("default_tenant", mode="after")
     @classmethod
     def _check_default_tenant(cls, value: str) -> str:
-        from simple_module_db import is_valid_tenant_id
+        from simple_module_db import PLATFORM_TENANT_ID, is_valid_tenant_id
 
         value = value.strip()
+        if value == PLATFORM_TENANT_ID:
+            raise ValueError(f"default_tenant {value!r} is reserved for platform-owned rows")
         if value and not is_valid_tenant_id(value):
             raise ValueError(f"default_tenant {value!r} is not a valid tenant id")
         return value

framework/hosting/tests/test_tenant_scope_helpers.py

@@ -59,6 +59,13 @@ def test_default_tenant_must_be_a_valid_id():
         HostSettings(default_tenant="has space")
 
 
+def test_default_tenant_cannot_be_the_platform_owner():
+    from simple_module_db import PLATFORM_TENANT_ID
+
+    with pytest.raises(ValidationError, match="reserved"):
+        HostSettings(default_tenant=PLATFORM_TENANT_ID)
+
+
 async def test_fixed_tenant_middleware_binds_every_request():
     seen = {}
 

host/migrations/versions/c7f2d9a41e83_file_storage_stored_file_tenant_id.py

@@ -0,0 +1,108 @@
+"""file_storage_stored_file: adopt MultiTenantMixin
+
+Adds ``tenant_id`` (nullable → back-filled with ``DEFAULT_TENANT_ID`` → NOT
+NULL), indexes it, and widens the unique ``key`` index to ``(tenant_id, key)``
+so two tenants can hold the same key (SM024). Existing rows keep their key:
+the column stores the full object path, so their bytes are still found where
+they were written; only new uploads get the ``{tenant_id}/`` prefix. See
+GH #383.
+
+Back-filled rows land in ``DEFAULT_TENANT_ID`` (the single-tenant fallback),
+except the files the SYSTEM-scope branding settings point at — the system logo,
+dark logo and favicon — which become *platform* files (``PLATFORM_TENANT_ID``),
+so they stay readable from the anonymous asset routes. The two owners are
+distinct on purpose: a platform lookup must never reach an ordinary row.
+Platform rows are never re-stamped by a later ``default_tenant`` adoption.
+
+Edited in place after review (before any release ran it): the first version
+back-filled everything, branding images included, into ``DEFAULT_TENANT_ID``.
+
+file_storage has no migration chain of its own (its table came in with the
+host's initial schema), so this extends the mainline head.
+
+Revision ID: c7f2d9a41e83
+Revises: b5d3f08a6e17
+Create Date: 2026-10-01 14:00:00.000000
+"""
+
+import uuid
+from collections.abc import Sequence
+
+import sqlalchemy as sa
+from alembic import op
+from simple_module_db import DEFAULT_TENANT_ID, PLATFORM_TENANT_ID
+
+# revision identifiers, used by Alembic.
+revision: str = "c7f2d9a41e83"
+down_revision: str | None = "b5d3f08a6e17"
+branch_labels: str | Sequence[str] | None = None
+depends_on: str | Sequence[str] | None = None
+
+_TABLE = "file_storage_stored_file"
+_OLD_KEY = "ix_file_storage_stored_file_key"
+_TENANT_KEY = "ix_file_storage_stored_file_tenant_key"
+_TENANT = "ix_file_storage_stored_file_tenant_id"
+_SETTINGS = "settings_setting"
+# SYSTEM-scope settings whose value is a platform file id (branding images).
+_PLATFORM_FILE_KEYS = (
+    "branding.logo_file_id",
+    "branding.logo_dark_file_id",
+    "branding.favicon_file_id",
+)
+
+
+def _platform_file_ids(bind: sa.engine.Connection) -> list[uuid.UUID]:
+    """File ids the system branding settings reference (malformed values skipped)."""
+    if not sa.inspect(bind).has_table(_SETTINGS):
+        return []
+    settings = sa.table(
+        _SETTINGS,
+        sa.column("scope", sa.String),
+        sa.column("scope_id", sa.String),
+        sa.column("key", sa.String),
+        sa.column("value", sa.String),
+    )
+    rows = bind.execute(
+        sa.select(settings.c.value).where(
+            settings.c.scope == "system",
+            settings.c.scope_id == "",
+            settings.c.key.in_(_PLATFORM_FILE_KEYS),
+        )
+    ).scalars()
+    ids: list[uuid.UUID] = []
+    for raw in rows:
+        try:
+            ids.append(uuid.UUID(str(raw).strip().strip('"')))
+        except ValueError:
+            continue
+    return ids
+
+
+def upgrade() -> None:
+    op.add_column(_TABLE, sa.Column("tenant_id", sa.String(length=50), nullable=True))
+    files = sa.table(_TABLE, sa.column("id", sa.Uuid), sa.column("tenant_id", sa.String))
+    bind = op.get_bind()
+    if platform_ids := _platform_file_ids(bind):
+        bind.execute(
+            sa.update(files)
+            .where(files.c.id.in_(platform_ids))
+            .values(tenant_id=PLATFORM_TENANT_ID)
+        )
+    bind.execute(
+        sa.update(files).where(files.c.tenant_id.is_(None)).values(tenant_id=DEFAULT_TENANT_ID)
+    )
+    with op.batch_alter_table(_TABLE) as batch:
+        batch.alter_column("tenant_id", existing_type=sa.String(length=50), nullable=False)
+        batch.drop_index(_OLD_KEY)
+        batch.create_index(_TENANT_KEY, ["tenant_id", "key"], unique=True)
+        batch.create_index(_TENANT, ["tenant_id"], unique=False)
+
+
+def downgrade() -> None:
+    # Fails if two tenants hold the same key — only possible for keys written
+    # by hand, since generated keys carry a uuid. Resolve those rows first.
+    with op.batch_alter_table(_TABLE) as batch:
+        batch.drop_index(_TENANT)
+        batch.drop_index(_TENANT_KEY)
+        batch.create_index(_OLD_KEY, ["key"], unique=True)
+        batch.drop_column("tenant_id")

modules/audit_log/tests/test_entity_labels.py

@@ -139,19 +139,23 @@ async def test_a_stored_file_is_named_by_its_filename(
         that owns the table is still the only thing that can name the row.
         """
         from file_storage.models import StoredFile
-
-        async with app.state.sm.db.session_factory() as session:
-            stored = StoredFile(
-                key="k/q3-report.pdf",
-                filename="q3-report.pdf",
-                content_type="application/pdf",
-                size_bytes=12,
-                backend="filesystem",
-                checksum_sha256="0" * 64,
-            )
-            session.add(stored)
-            await session.commit()
-            file_id = str(stored.id)
+        from simple_module_db import tenant_context
+
+        # StoredFile is tenant-scoped (#383); the platform audit log still
+        # names it, read by an admin with no tenant bound.
+        with tenant_context("files-tenant"):
+            async with app.state.sm.db.session_factory() as session:
+                stored = StoredFile(
+                    key="k/q3-report.pdf",
+                    filename="q3-report.pdf",
+                    content_type="application/pdf",
+                    size_bytes=12,
+                    backend="filesystem",
+                    checksum_sha256="0" * 64,
+                )
+                session.add(stored)
+                await session.commit()
+                file_id = str(stored.id)
 
         await _seed_entry(app, entity_type="StoredFile", entity_id=file_id)
 

modules/branding/branding/endpoints/api.py

@@ -15,6 +15,10 @@
 
 router = APIRouter()
 
+# Branding images are uploaded with ``platform=True``: they are the install's,
+# not the uploading admin's active organisation's, and the anonymous asset
+# routes must find them with no tenant bound (see ``file_storage.scope``).
+
 _MANAGE = Depends(RequiresPermission(constants.PERM_MANAGE))
 
 
@@ -63,7 +67,7 @@ async def upload_logo(
     storage: FileStorageService = Depends(get_file_storage_service),
 ) -> BrandingOut:
     await validate_image(file)
-    stored = await storage.upload(file)
+    stored = await storage.upload(file, platform=True)
     return await service.set_logo(str(stored.id))
 
 
@@ -87,7 +91,7 @@ async def upload_logo_dark(
     storage: FileStorageService = Depends(get_file_storage_service),
 ) -> BrandingOut:
     await validate_image(file)
-    stored = await storage.upload(file)
+    stored = await storage.upload(file, platform=True)
     return await service.set_logo_dark(str(stored.id))
 
 
@@ -103,7 +107,7 @@ async def upload_favicon(
     storage: FileStorageService = Depends(get_file_storage_service),
 ) -> BrandingOut:
     await validate_image(file)
-    stored = await storage.upload(file)
+    stored = await storage.upload(file, platform=True)
     return await service.set_favicon(str(stored.id))