diff --git a/docs/framework/multi-tenancy.md b/docs/framework/multi-tenancy.md index 274e5e53..364a6e9f 100644 --- a/docs/framework/multi-tenancy.md +++ b/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 diff --git a/docs/modules/branding.md b/docs/modules/branding.md index 7f863fa2..2fc697e3 100644 --- a/docs/modules/branding.md +++ b/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 `` 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 (``, ``) 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. diff --git a/docs/modules/file_storage.md b/docs/modules/file_storage.md index efb4eab6..7b196c56 100644 --- a/docs/modules/file_storage.md +++ b/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/`, 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 diff --git a/framework/db/simple_module_db/__init__.py b/framework/db/simple_module_db/__init__.py index 2929f334..3148900f 100644 --- a/framework/db/simple_module_db/__init__.py +++ b/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", diff --git a/framework/db/simple_module_db/tenancy.py b/framework/db/simple_module_db/tenancy.py index df7226b4..c8763f3e 100644 --- a/framework/db/simple_module_db/tenancy.py +++ b/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", diff --git a/framework/hosting/simple_module_hosting/host_settings.py b/framework/hosting/simple_module_hosting/host_settings.py index 529de771..17f3dc4d 100644 --- a/framework/hosting/simple_module_hosting/host_settings.py +++ b/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 diff --git a/framework/hosting/tests/test_tenant_scope_helpers.py b/framework/hosting/tests/test_tenant_scope_helpers.py index 90000208..6a1e9075 100644 --- a/framework/hosting/tests/test_tenant_scope_helpers.py +++ b/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 = {} diff --git a/host/migrations/versions/c7f2d9a41e83_file_storage_stored_file_tenant_id.py b/host/migrations/versions/c7f2d9a41e83_file_storage_stored_file_tenant_id.py new file mode 100644 index 00000000..9cb5dd4b --- /dev/null +++ b/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") diff --git a/modules/audit_log/tests/test_entity_labels.py b/modules/audit_log/tests/test_entity_labels.py index df5e64f1..36f9ee88 100644 --- a/modules/audit_log/tests/test_entity_labels.py +++ b/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) diff --git a/modules/branding/branding/endpoints/api.py b/modules/branding/branding/endpoints/api.py index a7458b99..e1bf9187 100644 --- a/modules/branding/branding/endpoints/api.py +++ b/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)) diff --git a/modules/branding/branding/endpoints/assets.py b/modules/branding/branding/endpoints/assets.py index 014fe444..f5b81ed9 100644 --- a/modules/branding/branding/endpoints/assets.py +++ b/modules/branding/branding/endpoints/assets.py @@ -4,6 +4,12 @@ file id from ``app.state.branding.settings`` and stream that one file, so they expose exactly the two images an administrator designated as public branding and nothing else in ``file_storage``. + +They run with no tenant bound (anonymous visitors), so the file is read as a +*platform* file (``platform=True``): an ``all_tenants()`` lookup restricted to +rows owned by ``file_storage.scope.PLATFORM_TENANT_ID``. That is safe because +the id comes from SYSTEM-scope settings, never from the request, and the owner +condition means even a setting pointed at a tenant's file id cannot publish it. """ from __future__ import annotations @@ -55,7 +61,7 @@ async def _serve( ) -> RedirectResponse | StreamingResponse: file_id = _configured_file_id(request, field) try: - download = await storage.download(file_id) + download = await storage.download(file_id, platform=True) except StoredFileNotFoundError as exc: # Referenced file went away underneath us. 404 uncached, so the next # request retries once the setting is fixed rather than caching a miss. diff --git a/modules/branding/branding/service.py b/modules/branding/branding/service.py index 66939aa9..063293ca 100644 --- a/modules/branding/branding/service.py +++ b/modules/branding/branding/service.py @@ -99,7 +99,7 @@ async def _reap(self, file_id: str) -> None: if self.storage is None: return try: - await self.storage.delete(uuid.UUID(file_id)) + await self.storage.delete(uuid.UUID(file_id), platform=True) except Exception: # Deliberately broad: any failure here is a cleanup problem, never # a reason to reject a rebrand the admin already succeeded at. diff --git a/modules/branding/tests/test_asset_lifecycle.py b/modules/branding/tests/test_asset_lifecycle.py index 6d026618..0f1880e5 100644 --- a/modules/branding/tests/test_asset_lifecycle.py +++ b/modules/branding/tests/test_asset_lifecycle.py @@ -30,7 +30,10 @@ def __init__(self) -> None: def install(self, monkeypatch: pytest.MonkeyPatch) -> None: store = self - async def fake_upload(self: FileStorageService, upload: Any) -> StoredFile: + async def fake_upload( + self: FileStorageService, upload: Any, *, platform: bool = False + ) -> StoredFile: + assert platform, "branding must act on platform-owned files" row = StoredFile( id=uuid.uuid4(), key=f"2026/06/{uuid.uuid4()}.png", @@ -43,7 +46,10 @@ async def fake_upload(self: FileStorageService, upload: Any) -> StoredFile: store.uploaded.append(row.id) return row - async def fake_delete(self: FileStorageService, file_id: uuid.UUID) -> StoredFile: + async def fake_delete( + self: FileStorageService, file_id: uuid.UUID, *, platform: bool = False + ) -> StoredFile: + assert platform, "branding must act on platform-owned files" if file_id in store.deleted: raise StoredFileNotFoundError(str(file_id)) store.deleted.append(file_id) @@ -136,7 +142,9 @@ async def test_a_failed_cleanup_does_not_fail_the_rebrand( # reach by design; see BrandingService._reap. await _upload(authenticated_client, "logo") - async def boom(self: FileStorageService, file_id: uuid.UUID) -> StoredFile: + async def boom( + self: FileStorageService, file_id: uuid.UUID, *, platform: bool = False + ) -> StoredFile: raise RuntimeError("backend unavailable") monkeypatch.setattr(FileStorageService, "delete", boom, raising=True) diff --git a/modules/branding/tests/test_branding.py b/modules/branding/tests/test_branding.py index 934bac9c..64b13d94 100644 --- a/modules/branding/tests/test_branding.py +++ b/modules/branding/tests/test_branding.py @@ -195,7 +195,8 @@ async def test_logo_upload_sets_logo_url( ) -> None: fake_id = uuid.uuid4() - async def fake_upload(self, upload): + async def fake_upload(self, upload, *, platform: bool = False): + assert platform, "branding must act on platform-owned files" return SimpleNamespace(id=fake_id) monkeypatch.setattr("file_storage.service.FileStorageService.upload", fake_upload, raising=True) diff --git a/modules/branding/tests/test_branding_platform_files.py b/modules/branding/tests/test_branding_platform_files.py new file mode 100644 index 00000000..8fec72b3 --- /dev/null +++ b/modules/branding/tests/test_branding_platform_files.py @@ -0,0 +1,115 @@ +"""Branding images are *platform* files, against the real file_storage (#383). + +``StoredFile`` is tenant-scoped and the suite runs strict, yet the logo must +load for an anonymous visitor (no tenant bound at all) and must not land in — +or be deletable from — whichever organisation the uploading admin has active. +Branding therefore uploads, serves and reaps with ``platform=True``: rows owned +by ``PLATFORM_TENANT_ID``, read under ``all_tenants()`` but only ever matching +that owner. +""" + +from __future__ import annotations + +import uuid + +import httpx +import pytest +from branding.constants import LOGO_URL +from file_storage.models import StoredFile +from file_storage.scope import PLATFORM_TENANT_ID +from simple_module_db import all_tenants +from sqlalchemy import select + +_PNG = b"\x89PNG\r\n\x1a\n" + b"\x00" * 32 + + +async def _upload_logo(client: httpx.AsyncClient, body: bytes = _PNG) -> None: + resp = await client.post("/api/branding/logo", files={"file": ("logo.png", body, "image/png")}) + assert resp.status_code == 200, resp.text + + +async def _rows(app) -> list[StoredFile]: + with all_tenants(): + async with app.state.sm.db.session_factory() as session: + stmt = select(StoredFile).execution_options(include_deleted=True) + return list((await session.execute(stmt)).scalars().all()) + + +async def _give_admin_a_tenant(app) -> str: + from simple_module_test.fixtures import SETUP_ADMIN_EMAIL + from tenants.models import Membership, Tenant + from tenants.resolver import forget + from users.models import User + + async with app.state.sm.db.session_factory() as session: + admin = ( + await session.execute(select(User).where(User.email == SETUP_ADMIN_EMAIL)) + ).scalar_one() + tenant = Tenant(slug="admin-org", name="Admin Org") + session.add(tenant) + await session.flush() + session.add(Membership(tenant_id=tenant.id, user_id=str(admin.id), role="owner")) + await session.commit() + forget(str(admin.id)) + return tenant.id + + +async def test_a_platform_admin_with_no_tenant_can_brand_and_guests_see_it( + app, authenticated_client: httpx.AsyncClient, client: httpx.AsyncClient +): + await _upload_logo(authenticated_client) + + [row] = await _rows(app) + assert row.tenant_id == PLATFORM_TENANT_ID + assert row.key.startswith(f"{PLATFORM_TENANT_ID}/") + + resp = await client.get(LOGO_URL) # anonymous, no tenant bound + assert resp.status_code == 200, resp.text + assert resp.content == _PNG + + +async def test_an_admin_inside_a_tenant_still_uploads_a_platform_file( + app, authenticated_client: httpx.AsyncClient, client: httpx.AsyncClient +): + await _give_admin_a_tenant(app) + await _upload_logo(authenticated_client) + + [row] = await _rows(app) + assert row.tenant_id == PLATFORM_TENANT_ID + # Not one of the organisation's files: absent from its listing... + listed = (await authenticated_client.get("/api/file-storage/files")).json() + assert listed["items"] == [] + # ...and unreachable through its routes. + resp = await authenticated_client.get(f"/api/file-storage/files/{row.id}") + assert resp.status_code == 404 + assert (await client.get(LOGO_URL)).status_code == 200 + + +async def test_replacing_the_logo_reaps_the_old_platform_file( + app, authenticated_client: httpx.AsyncClient +): + await _give_admin_a_tenant(app) + await _upload_logo(authenticated_client, _PNG) + await _upload_logo(authenticated_client, _PNG + b"\x01") + + rows = sorted(await _rows(app), key=lambda r: r.created_at) + assert [r.is_deleted for r in rows] == [True, False] + + +async def test_a_setting_naming_a_tenant_file_cannot_publish_it( + app, tenant_client, client: httpx.AsyncClient, monkeypatch: pytest.MonkeyPatch +): + """The settings store is editable; pointing ``logo_file_id`` at a tenant's + upload must not turn the anonymous logo route into a download link for it.""" + async with tenant_client() as tenant: + resp = await tenant.client.post( + "/api/file-storage/upload", files={"file": ("secret.png", _PNG, "image/png")} + ) + assert resp.status_code == 201, resp.text + file_id = resp.json()["id"] + + monkeypatch.setattr(app.state.branding.settings, "logo_file_id", file_id) + + assert (await client.get(LOGO_URL)).status_code == 404 + monkeypatch.setattr(app.state.branding.settings, "logo_file_id", str(uuid.uuid4())) + assert (await client.get(LOGO_URL)).status_code == 404 diff --git a/modules/branding/tests/test_logo_dark.py b/modules/branding/tests/test_logo_dark.py index 5e9e07d3..c4580afc 100644 --- a/modules/branding/tests/test_logo_dark.py +++ b/modules/branding/tests/test_logo_dark.py @@ -31,7 +31,10 @@ def stored(monkeypatch: pytest.MonkeyPatch) -> dict[uuid.UUID, StoredFile]: rows: dict[uuid.UUID, StoredFile] = {} - async def fake_upload(self: FileStorageService, upload: Any) -> StoredFile: + async def fake_upload( + self: FileStorageService, upload: Any, *, platform: bool = False + ) -> StoredFile: + assert platform, "branding must act on platform-owned files" row = StoredFile( id=uuid.uuid4(), key=f"2026/06/{uuid.uuid4()}.png", @@ -44,13 +47,20 @@ async def fake_upload(self: FileStorageService, upload: Any) -> StoredFile: rows[row.id] = row return row - async def fake_download(self: FileStorageService, file_id: uuid.UUID) -> StreamDownload: + async def fake_download( + self: FileStorageService, file_id: uuid.UUID, *, platform: bool = False + ) -> StreamDownload: + assert platform, "branding must act on platform-owned files" + async def body() -> AsyncIterator[bytes]: yield _PNG return StreamDownload(file=rows[file_id], body=body()) - async def fake_delete(self: FileStorageService, file_id: uuid.UUID) -> StoredFile: + async def fake_delete( + self: FileStorageService, file_id: uuid.UUID, *, platform: bool = False + ) -> StoredFile: + assert platform, "branding must act on platform-owned files" return rows.pop(file_id) monkeypatch.setattr(FileStorageService, "upload", fake_upload, raising=True) diff --git a/modules/branding/tests/test_public_assets.py b/modules/branding/tests/test_public_assets.py index 0a0173e6..ee84b7c7 100644 --- a/modules/branding/tests/test_public_assets.py +++ b/modules/branding/tests/test_public_assets.py @@ -36,10 +36,16 @@ def stored_logo(monkeypatch: pytest.MonkeyPatch) -> StoredFile: checksum_sha256="a" * 64, ) - async def fake_upload(self: FileStorageService, upload: Any) -> StoredFile: + async def fake_upload( + self: FileStorageService, upload: Any, *, platform: bool = False + ) -> StoredFile: + assert platform, "branding must act on platform-owned files" return row - async def fake_download(self: FileStorageService, file_id: uuid.UUID) -> StreamDownload: + async def fake_download( + self: FileStorageService, file_id: uuid.UUID, *, platform: bool = False + ) -> StreamDownload: + assert platform, "branding must act on platform-owned files" assert file_id == row.id, f"asked for {file_id}, only {row.id} is stored" async def body() -> AsyncIterator[bytes]: diff --git a/modules/file_storage/file_storage/aggregates.py b/modules/file_storage/file_storage/aggregates.py index 65ebf231..df99006c 100644 --- a/modules/file_storage/file_storage/aggregates.py +++ b/modules/file_storage/file_storage/aggregates.py @@ -24,7 +24,10 @@ The cache is per-app (held on ``FileStorageServices``), not per-process: a process running two apps — the test suite does — must not serve one app's -totals from the other's database. +totals from the other's database. Within an app it is keyed by tenant (#383): +the scan is tenant-filtered, so one tenant's totals must never answer another +tenant's render. A write drops only the slots it could have changed — the +tenants of the rows it wrote, plus the unscoped slot that counts everyone. """ from __future__ import annotations @@ -34,6 +37,7 @@ from functools import partial from typing import TYPE_CHECKING +from simple_module_db import current_tenant_id from sqlalchemy import event, func, select from sqlalchemy.ext.asyncio import AsyncSession @@ -51,7 +55,7 @@ """ _WROTE_FILES_KEY = "file_storage_wrote_files" -"""``Session.info`` flag set at flush and read at commit. +"""``Session.info`` set of written tenants, filled at flush and read at commit. Stamped in ``before_flush`` because that is the last point at which ``session.new``/``.dirty``/``.deleted`` still name the objects being written; @@ -132,63 +136,84 @@ def _facets(counts: dict[str, int]) -> tuple[Facet, ...]: return tuple(Facet(value=value, count=count) for value, count in sorted(counts.items())) +_Slot = tuple[StorageAggregates, float] +"""Cached totals and the monotonic time they expire at.""" + + @dataclass class AggregateCache: - """One slot's worth of memoised totals, with an expiry and a manual drop. + """Memoised totals per tenant, each with an expiry, and a manual drop. - Deliberately not ``cachetools``: a single entry with one expiry is two - fields, and ``file_storage`` would otherwise grow a dependency to hold them. + Keyed by the bound tenant id — ``None`` for an unscoped read (a + single-tenant install, or an ``all_tenants()`` block), which counts every + tenant's rows and so is a slot of its own. Expired slots are pruned on + store, so the map holds at most the tenants seen within one TTL. + + Deliberately not ``cachetools``: a dict of (value, expiry) pairs is all + this needs, and ``file_storage`` would otherwise grow a dependency for it. """ ttl_seconds: float = AGGREGATE_TTL_SECONDS - _value: StorageAggregates | None = field(default=None, init=False, repr=False) - _expires_at: float = field(default=0.0, init=False, repr=False) + _slots: dict[str | None, _Slot] = field(default_factory=dict, init=False, repr=False) _wired: bool = field(default=False, init=False, repr=False) """Whether :func:`register_invalidation` has already attached this cache.""" - def invalidate(self) -> None: - """Forget the cached totals — the next read re-scans.""" - self._value = None - self._expires_at = 0.0 + def invalidate(self, tenant_ids: set[str | None] | None = None) -> None: + """Forget cached totals — for ``tenant_ids``, or every slot if ``None``.""" + if tenant_ids is None: + self._slots.clear() + return + for tenant_id in tenant_ids: + self._slots.pop(tenant_id, None) def peek(self) -> StorageAggregates | None: - """The cached totals if still fresh, else ``None``. No DB access.""" - if self._value is None or time.monotonic() >= self._expires_at: + """The bound tenant's cached totals if still fresh, else ``None``.""" + slot = self._slots.get(current_tenant_id.get()) + if slot is None or time.monotonic() >= slot[1]: return None - return self._value + return slot[0] async def get(self, db: AsyncSession) -> StorageAggregates: - """Cached totals, computing them on a miss.""" + """Cached totals for the bound tenant, computing them on a miss.""" cached = self.peek() if cached is not None: return cached + # Read the key before the await: the scan is filtered by the tenant + # bound *now*, and that is the slot its answer belongs in. + tenant_id = current_tenant_id.get() value = await compute(db) - self._value = value - self._expires_at = time.monotonic() + self.ttl_seconds + now = time.monotonic() + self._slots = {k: v for k, v in self._slots.items() if v[1] > now} + self._slots[tenant_id] = (value, now + self.ttl_seconds) return value def _mark_stored_file_writes(session, flush_context, instances) -> None: - """Flag the session if this flush touches a stored file.""" - if session.info.get(_WROTE_FILES_KEY): - return + """Record which tenants' stored files this flush writes. + + The flush guard runs in its own ``before_flush`` and may stamp + ``tenant_id`` after this one, so the bound tenant is recorded alongside: + an unstamped new row is about to become the bound tenant's. + """ + written: set[str | None] = session.info.setdefault(_WROTE_FILES_KEY, set()) for obj in (*session.new, *session.dirty, *session.deleted): if isinstance(obj, StoredFile): - session.info[_WROTE_FILES_KEY] = True - return + written.add(obj.tenant_id or current_tenant_id.get()) def _drop_on_commit(cache: AggregateCache, session) -> None: - """Drop the cached totals once a file-writing session has committed. + """Drop the written tenants' totals once a file-writing session committed. - The flag is read, not consumed: a session can commit more than once — the + The unscoped slot (``None``) counts every tenant, so any write drops it. + The set is read, not consumed: a session can commit more than once — the commit-before-response middleware is deliberately re-armable — and a - listener that ate the flag would leave a second cache on the same session + listener that ate the set would leave a second cache on the same session class, or a second commit, with nothing to act on. Re-dropping an already dropped cache costs nothing; missing a drop serves a stale number. """ - if session.info.get(_WROTE_FILES_KEY): - cache.invalidate() + written = session.info.get(_WROTE_FILES_KEY) + if written: + cache.invalidate({*written, None}) def register_invalidation(db_state: DatabaseState, cache: AggregateCache) -> None: diff --git a/modules/file_storage/file_storage/constants.py b/modules/file_storage/file_storage/constants.py index f558d296..935b8824 100644 --- a/modules/file_storage/file_storage/constants.py +++ b/modules/file_storage/file_storage/constants.py @@ -110,6 +110,5 @@ class I18nKey: # ── Menu ───────────────────────────────────────────────────────────── MENU_ICON: Final = "files" MENU_ORDER: Final = 40 -MENU_ROLES: Final = ("admin",) ADMIN_ROLE: Final = "admin" USER_ROLE: Final = "user" diff --git a/modules/file_storage/file_storage/models.py b/modules/file_storage/file_storage/models.py index 543a81fd..b0a67961 100644 --- a/modules/file_storage/file_storage/models.py +++ b/modules/file_storage/file_storage/models.py @@ -6,7 +6,7 @@ import sqlalchemy as sa from simple_module_db.base import create_module_base -from simple_module_db.mixins import AuditMixin, SoftDeleteMixin +from simple_module_db.mixins import AuditMixin, MultiTenantMixin, SoftDeleteMixin from sqlalchemy import Index from sqlmodel import Field @@ -15,12 +15,19 @@ Base = create_module_base(constants.MODULE_NAME) -class StoredFile(Base, AuditMixin, SoftDeleteMixin, table=True): # ty: ignore[unsupported-base] +class StoredFile(Base, AuditMixin, SoftDeleteMixin, MultiTenantMixin, table=True): # ty: ignore[unsupported-base] """A file persisted to a configured storage backend. The ``backend`` column records which provider holds the bytes — important if the active backend is changed after ingest, since old rows still need to be located on their original provider until they're migrated. + + Tenant-scoped (#383): every read, update and delete is filtered to the + bound tenant, so a file id from another tenant simply does not resolve. + ``key`` is unique per tenant — new keys start with ``{tenant_id}/`` so the + backend namespaces are disjoint too, while rows written before adoption + keep the un-prefixed key they were stored under (the column holds the full + object path; nothing derives it). """ __tablename__ = constants.TABLE_STORED_FILE @@ -38,7 +45,7 @@ class StoredFile(Base, AuditMixin, SoftDeleteMixin, table=True): # ty: ignore[u ) __table_args__ = ( - Index(f"ix_{constants.TABLE_STORED_FILE}_key", "key", unique=True), + Index(f"ix_{constants.TABLE_STORED_FILE}_tenant_key", "tenant_id", "key", unique=True), Index(f"ix_{constants.TABLE_STORED_FILE}_created_by", "created_by"), Index(f"ix_{constants.TABLE_STORED_FILE}_is_deleted", "is_deleted"), ) diff --git a/modules/file_storage/file_storage/module.py b/modules/file_storage/file_storage/module.py index f739bcd1..73635c1a 100644 --- a/modules/file_storage/file_storage/module.py +++ b/modules/file_storage/file_storage/module.py @@ -13,6 +13,7 @@ from simple_module_core.menu import MenuItem, MenuRegistry, MenuSection from simple_module_core.module import ModuleBase, ModuleMeta from simple_module_core.permissions import PermissionRegistry +from simple_module_core.tenancy import TenantRole, tenant_role from file_storage import constants @@ -43,11 +44,13 @@ async def _resolve_file_labels(db: AsyncSession, ids: list[str]) -> dict[str, st continue if not wanted: return {} - rows = ( - await db.execute( - select(StoredFile.id, StoredFile.filename).where(StoredFile.id.in_(list(wanted))) - ) - ).all() + # Deliberately cross-tenant: the audit log is a platform screen listing + # every tenant's entries, and the entry it labels already records the + # filename among its captured fields, so naming it discloses nothing new. + # Scoped to the bound tenant instead, it would fail closed for a platform + # admin with no organisation and leave every other tenant's file a uuid. + stmt = select(StoredFile.id, StoredFile.filename).where(StoredFile.id.in_(list(wanted))) + rows = (await db.execute(stmt.execution_options(all_tenants=True))).all() return {wanted.get(file_id, str(file_id)): filename for file_id, filename in rows} @@ -123,14 +126,14 @@ def register_permissions(self, registry: PermissionRegistry) -> None: constants.Permission.MANAGE, ], ) - registry.map_role( - constants.USER_ROLE, - [ - constants.Permission.UPLOAD, - constants.Permission.DOWNLOAD, - constants.Permission.DELETE, - ], - ) + read_write = [constants.Permission.UPLOAD, constants.Permission.DOWNLOAD] + # Deleting is an organisation-admin act (#383). The platform ``user`` + # role used to carry it too, which — every account holds ``user`` — + # would hand it straight back to each tenant member. + registry.map_role(constants.USER_ROLE, read_write) + registry.map_role(tenant_role(TenantRole.MEMBER), read_write) + for role in (TenantRole.ADMIN, TenantRole.OWNER): + registry.map_role(tenant_role(role), [*read_write, constants.Permission.DELETE]) def register_menu_items(self, registry: MenuRegistry) -> None: registry.add( @@ -141,7 +144,9 @@ def register_menu_items(self, registry: MenuRegistry) -> None: icon=constants.MENU_ICON, order=constants.MENU_ORDER, section=MenuSection.SIDEBAR, - roles=list(constants.MENU_ROLES), + # Platform admins, and every member of the active organisation + # (the files screen is where a tenant's own uploads live). + roles=[constants.ADMIN_ROLE, *(tenant_role(r) for r in TenantRole)], group="Content", group_key="ui.nav_groups.content", ) @@ -171,6 +176,13 @@ async def on_startup(self, app: FastAPI) -> None: from file_storage.aggregates import register_invalidation from file_storage.backends import build_backend + # With multi_tenant off the install is the only tenant, so ordinary + # users keep deleting their files. With it on every tenant member also + # holds ``user``, so the grant stays on the organisation-admin roles. + # Done here because register_permissions cannot see the settings. + if not getattr(app.state.sm.settings, "multi_tenant", False): + app.state.sm.permissions.map_role(constants.USER_ROLE, [constants.Permission.DELETE]) + services = app.state.file_storage settings = services.settings services.backend = build_backend(settings) diff --git a/modules/file_storage/file_storage/scope.py b/modules/file_storage/file_storage/scope.py new file mode 100644 index 00000000..0c5edfd2 --- /dev/null +++ b/modules/file_storage/file_storage/scope.py @@ -0,0 +1,72 @@ +"""Which tenant a stored file belongs to — and the one way to act as the platform. + +``StoredFile`` is ``MultiTenantMixin`` (#383): request code reads, writes and +deletes only the bound tenant's files, and strict mode fails closed with no +tenant bound. Two callers need something else, and both go through here: + +* **Upload** must know the owning tenant *before* the row exists, because the + storage key is prefixed with it and the bytes are written to the backend + before the row is flushed. :func:`owning_tenant` answers that the same way + the flush guard will, so a strict install with no tenant fails before any + object is written rather than after. +* **Platform-owned files** — today, branding's logo and favicon — belong to no + tenant and must stay readable from anonymous requests, where no tenant is + bound at all. They are stamped :data:`PLATFORM_TENANT_ID` and read under + :func:`platform_scope`, restricted to that owner. The id is reserved in + ``simple_module_db`` — ``is_valid_tenant_id`` refuses it — so no request, + job or tenant can ever be bound to it, and it is distinct from + ``DEFAULT_TENANT_ID``, which owns a single-tenant install's ordinary rows. +""" + +from __future__ import annotations + +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager + +from simple_module_db import PLATFORM_TENANT_ID, all_tenants, current_tenant_id +from simple_module_db.query_filter import fallback_tenant_id, is_strict +from simple_module_db.tenancy import missing_tenant_error +from sqlalchemy.ext.asyncio import AsyncSession + + +def owning_tenant(db: AsyncSession, *, platform: bool = False) -> str: + """The tenant a new upload is stamped with, decided before the bytes move. + + Bound tenant if there is one; ``PLATFORM_TENANT_ID`` for a platform upload; + on a non-strict install with nothing bound, the install's fallback + (``default_tenant`` or ``DEFAULT_TENANT_ID``) — the flush guard's own + choice. Strict with nothing bound raises ``MissingTenantError``. + """ + if platform: + return PLATFORM_TENANT_ID + bound = current_tenant_id.get() + if bound: + return bound + if is_strict(db.sync_session): + raise missing_tenant_error("StoredFile", "INSERT") + return fallback_tenant_id(db.sync_session) + + +@asynccontextmanager +async def platform_scope(db: AsyncSession, platform: bool) -> AsyncIterator[None]: + """``all_tenants()`` for platform-file access, a no-op otherwise. + + Callers pair it with a ``tenant_id == PLATFORM_TENANT_ID`` condition, so + the bypass widens *which context* may read a platform file without letting + a platform caller reach any tenant's rows. + + The bypass covers the session's whole flush, not one statement, so it must + never carry the caller's unrelated pending writes along: those are flushed + first, under the normal guard (stamped with, and checked against, the bound + tenant), and autoflush is off inside — only what the block itself adds or + changes is flushed with isolation lifted. + """ + if not platform: + yield + return + await db.flush() + with db.no_autoflush, all_tenants(): + yield + + +__all__ = ["PLATFORM_TENANT_ID", "owning_tenant", "platform_scope"] diff --git a/modules/file_storage/file_storage/service.py b/modules/file_storage/file_storage/service.py index f4289291..b969856f 100644 --- a/modules/file_storage/file_storage/service.py +++ b/modules/file_storage/file_storage/service.py @@ -26,6 +26,7 @@ from file_storage.contracts.service import StorageNotFoundError from file_storage.models import StoredFile from file_storage.reads import FileStorageReads +from file_storage.scope import PLATFORM_TENANT_ID, owning_tenant, platform_scope if TYPE_CHECKING: from file_storage.aggregates import AggregateCache @@ -89,10 +90,18 @@ def __init__( # ── Upload ─────────────────────────────────────────────────────── - async def upload(self, upload: UploadFile) -> StoredFileOut: - """Validate, stream-hash, persist to backend, and record metadata.""" + async def upload(self, upload: UploadFile, *, platform: bool = False) -> StoredFileOut: + """Validate, stream-hash, persist to backend, and record metadata. + + The row belongs to the bound tenant, or — with ``platform=True`` — to + no tenant (:data:`~file_storage.scope.PLATFORM_TENANT_ID`), for files + the whole install serves, such as branding images. The owner is + settled first: the key is prefixed with it, and a strict install with + no tenant must fail before an object is written, not after. + """ content_type = upload.content_type or "application/octet-stream" self._check_content_type(content_type) + tenant_id = owning_tenant(self.db, platform=platform) size = 0 sha = hashlib.sha256() @@ -110,7 +119,7 @@ async def _hashing_stream() -> AsyncIterator[bytes]: sha.update(chunk) yield chunk - key = _generate_key(upload.filename or "file") + key = _generate_key(tenant_id, upload.filename or "file") await self.backend.put( key, _hashing_stream(), @@ -125,6 +134,7 @@ async def _hashing_stream() -> AsyncIterator[bytes]: # by a janitor sweep. try: row = StoredFile( + tenant_id=tenant_id, key=key, filename=upload.filename or key, content_type=content_type, @@ -132,9 +142,10 @@ async def _hashing_stream() -> AsyncIterator[bytes]: backend=self.backend.backend_id, checksum_sha256=sha.hexdigest(), ) - self.db.add(row) - await self.db.flush() - await self.db.refresh(row) + async with platform_scope(self.db, platform): + self.db.add(row) + await self.db.flush() + await self.db.refresh(row) except Exception: try: await self.backend.delete(key) @@ -152,20 +163,31 @@ def _check_content_type(self, content_type: str) -> None: if allowed is not None and content_type not in allowed: raise ContentTypeNotAllowedError(f"Content-Type {content_type!r} not in allow-list.") - async def get(self, file_id: uuid.UUID) -> StoredFile: - row = await self.db.get(StoredFile, file_id) + async def get(self, file_id: uuid.UUID, *, platform: bool = False) -> StoredFile: + """The bound tenant's file, or with ``platform=True`` a platform file. + + Another tenant's id misses exactly like an unknown one. ``platform`` + needs no tenant bound (anonymous branding requests have none) and only + ever matches a platform-owned row, so a tenant's file id handed to a + platform caller — say, pasted into a branding setting — still misses. + """ + stmt = select(StoredFile).where(StoredFile.id == file_id) + if platform: + stmt = stmt.where(StoredFile.tenant_id == PLATFORM_TENANT_ID) + async with platform_scope(self.db, platform): + row = (await self.db.execute(stmt)).scalar_one_or_none() if row is None: raise StoredFileNotFoundError(str(file_id)) return row - async def download(self, file_id: uuid.UUID) -> Download: + async def download(self, file_id: uuid.UUID, *, platform: bool = False) -> Download: """Return either a streamed body or a redirect URL. Dispatch on ``backend.supports_presigned_url`` so the service stays provider-agnostic — adding a new backend that supports presigning works without touching this method. """ - row = await self.get(file_id) + row = await self.get(file_id, platform=platform) if self.backend.supports_presigned_url: url = await self.backend.presigned_get_url( row.key, self.settings.s3_presign_ttl_seconds @@ -221,21 +243,30 @@ async def delete_many(self, file_ids: Sequence[uuid.UUID]) -> list[StoredFile]: ) return rows - async def delete(self, file_id: uuid.UUID) -> StoredFile: - row = await self.get(file_id) - # Soft-delete in DB first; if the backend delete fails afterwards we - # still have a row marked deleted that can be reaped by a janitor. - row.is_deleted = True - row.deleted_at = datetime.now(UTC) - await self.db.flush() + async def delete(self, file_id: uuid.UUID, *, platform: bool = False) -> StoredFile: + # One scope around read + write: its opening flush runs before the + # platform row is touched, so the guard never sees that change. + async with platform_scope(self.db, platform): + row = await self.get(file_id, platform=platform) + # Soft-delete in DB first; if the backend delete fails afterwards we + # still have a row marked deleted that can be reaped by a janitor. + row.is_deleted = True + row.deleted_at = datetime.now(UTC) + await self.db.flush() # Object is acceptably absent — eg. a previous delete partially succeeded. with contextlib.suppress(StorageNotFoundError): await self.backend.delete(row.key) return row -def _generate_key(filename: str) -> str: - """Build a date-sharded, collision-proof key from the original filename.""" +def _generate_key(tenant_id: str, filename: str) -> str: + """Build a tenant-prefixed, date-sharded, collision-proof key. + + The ``{tenant_id}/`` prefix keeps each tenant's objects in a disjoint + namespace on the backend (one prefix to export, purge or bill per tenant). + Tenant ids are validated to ``TENANT_ID_PATTERN``, which has no ``/`` and + cannot be ``..``, so the prefix is always exactly one path segment. + """ today = datetime.now(UTC) suffix = Path(filename).suffix - return f"{today:%Y/%m/%d}/{uuid.uuid4().hex}{suffix}" + return f"{tenant_id}/{today:%Y/%m/%d}/{uuid.uuid4().hex}{suffix}" diff --git a/modules/file_storage/tests/conftest.py b/modules/file_storage/tests/conftest.py index c553df6d..27c74056 100644 --- a/modules/file_storage/tests/conftest.py +++ b/modules/file_storage/tests/conftest.py @@ -47,3 +47,39 @@ def _sync_engine(target) -> Engine: """Accept an app, an ``AsyncEngine`` or a sync ``Engine`` interchangeably.""" engine = target.state.sm.db.engine if hasattr(target, "state") else target return getattr(engine, "sync_engine", engine) + + +@pytest.fixture +async def admin_tenant_id(app) -> str: + """Give the seeded admin an organisation it owns; that tenant's id. + + ``StoredFile`` is tenant-scoped and the suite runs strict, so a request + with no active tenant fails closed (403 ``tenant_required``). The tenants + resolver falls back to a user's first membership, so one membership is + enough to make every ``authenticated_client`` request act for this tenant. + """ + from simple_module_test.fixtures import SETUP_ADMIN_EMAIL + from sqlalchemy import select + from tenants.models import Membership, Tenant + from tenants.resolver import forget + from users.models import User + + async with app.state.sm.db.session_factory() as session: + admin = ( + await session.execute(select(User).where(User.email == SETUP_ADMIN_EMAIL)) + ).scalar_one() + tenant = Tenant(slug="admin-org", name="Admin Org") + session.add(tenant) + await session.flush() + session.add( + Membership(tenant_id=tenant.id, user_id=str(admin.id), role="owner", email=admin.email) + ) + await session.commit() + forget(str(admin.id)) + return tenant.id + + +@pytest.fixture +async def authenticated_client(authenticated_client, admin_tenant_id): + """The plugin's admin client, acting for the admin's own organisation.""" + return authenticated_client diff --git a/modules/file_storage/tests/test_browse_props.py b/modules/file_storage/tests/test_browse_props.py index d49267b6..1d8f205e 100644 --- a/modules/file_storage/tests/test_browse_props.py +++ b/modules/file_storage/tests/test_browse_props.py @@ -16,22 +16,24 @@ UNKNOWN_UPLOADER = constants.UNKNOWN_UPLOADER -async def _seed_orphan_row(app) -> None: +async def _seed_orphan_row(app, tenant_id: str) -> None: """Insert a file with no uploader, the way pre-audit rows look.""" from file_storage.models import StoredFile - - async with app.state.sm.db.session_factory() as session: - session.add( - StoredFile( - key="2026/01/01/orphan.txt", - filename="orphan.txt", - content_type="text/plain", - size_bytes=3, - backend=constants.BackendId.FILESYSTEM, - checksum_sha256="0" * 64, + from simple_module_db import tenant_context + + with tenant_context(tenant_id): + async with app.state.sm.db.session_factory() as session: + session.add( + StoredFile( + key="2026/01/01/orphan.txt", + filename="orphan.txt", + content_type="text/plain", + size_bytes=3, + backend=constants.BackendId.FILESYSTEM, + checksum_sha256="0" * 64, + ) ) - ) - await session.commit() + await session.commit() async def _upload(client: httpx.AsyncClient, name: str, body: bytes = b"hi") -> str: @@ -129,10 +131,10 @@ async def test_rows_carry_a_resolved_uploader_label( assert [f["uploaded_by_label"] for f in props["files"]] == ["Test Admin"] async def test_unknown_uploader_falls_back_to_a_dash( - self, app, authenticated_client: httpx.AsyncClient + self, app, authenticated_client: httpx.AsyncClient, admin_tenant_id: str ): """Rows predating authenticated uploads carry no ``created_by``.""" - await _seed_orphan_row(app) + await _seed_orphan_row(app, admin_tenant_id) props = await _browse(authenticated_client) diff --git a/modules/file_storage/tests/test_browse_query_shape.py b/modules/file_storage/tests/test_browse_query_shape.py index 36e55a2e..ae86b68e 100644 --- a/modules/file_storage/tests/test_browse_query_shape.py +++ b/modules/file_storage/tests/test_browse_query_shape.py @@ -140,25 +140,27 @@ async def test_a_bulk_delete_is_reflected_on_the_next_render( assert props["uploaders"] == [] async def test_a_write_outside_the_request_path_still_drops_the_cache( - self, app, authenticated_client: httpx.AsyncClient + self, app, authenticated_client: httpx.AsyncClient, admin_tenant_id: str ): """Invalidation hangs off the commit, not off ``FileUploaded`` — so a seed script or a fix-up in the shell is seen too.""" from file_storage.models import StoredFile + from simple_module_db import tenant_context assert (await _browse(authenticated_client))["used_bytes"] == 0 - async with app.state.sm.db.session_factory() as session: - session.add( - StoredFile( - key="2026/01/01/seeded.txt", - filename="seeded.txt", - content_type="text/plain", - size_bytes=42, - backend=constants.BackendId.FILESYSTEM, - checksum_sha256="0" * 64, + with tenant_context(admin_tenant_id): + async with app.state.sm.db.session_factory() as session: + session.add( + StoredFile( + key="2026/01/01/seeded.txt", + filename="seeded.txt", + content_type="text/plain", + size_bytes=42, + backend=constants.BackendId.FILESYSTEM, + checksum_sha256="0" * 64, + ) ) - ) - await session.commit() + await session.commit() assert (await _browse(authenticated_client))["used_bytes"] == 42 diff --git a/modules/file_storage/tests/test_bulk_delete.py b/modules/file_storage/tests/test_bulk_delete.py index 9401d343..a43ca481 100644 --- a/modules/file_storage/tests/test_bulk_delete.py +++ b/modules/file_storage/tests/test_bulk_delete.py @@ -15,7 +15,6 @@ from fastapi import UploadFile from file_storage import constants from file_storage.contracts.events import FileDeleted -from simple_module_test import forge_session_cookie BULK_DELETE = f"{constants.ROUTE_PREFIX_API}{constants.PATH_FILES_BULK_DELETE}" LIST_FILES = f"{constants.ROUTE_PREFIX_API}{constants.PATH_FILES}" @@ -157,14 +156,15 @@ async def test_unauthenticated_request_is_rejected(self, client: httpx.AsyncClie assert resp.status_code in {302, 401, 403} - async def test_download_only_caller_cannot_delete(self, app): - """Reading the bucket and emptying it are separate grants. + async def test_tenant_member_can_read_but_not_delete(self, tenant_client): + """Reading the bucket and emptying it are separate grants (#383). - The module maps its own ``user`` role to upload+download+delete, so a - plain account proves nothing here — this caller holds exactly - ``file_storage.download`` and must be refused. + A tenant ``member`` holds upload and download; delete is mapped onto + the organisation's ``admin``/``owner`` only — and no longer onto the + platform ``user`` role every account carries, which would have handed + it straight back to the member. """ - async with await _reader_client(app) as reader: + async with tenant_client("member") as (reader, _, _): listed = await reader.get(LIST_FILES) forbidden = await reader.post(BULK_DELETE, json={"ids": []}) @@ -172,44 +172,3 @@ async def test_download_only_caller_cannot_delete(self, app): # permission rather than a caller who never authenticated at all. assert listed.status_code == 200, listed.text assert forbidden.status_code == 403 - - -READER_ROLE = "file_storage_reader" - - -async def _reader_client(app) -> httpx.AsyncClient: - """A signed-in caller holding ``file_storage.download`` and nothing else. - - Built rather than reused: the module maps its own ``user`` role to - upload+download+delete, so no seeded account can stand in for "may read the - bucket, may not empty it". - """ - from users.models import Role, User, UserRole - - # Role → permission mapping lives in the registry, not the DB. - app.state.sm.permissions.map_role(READER_ROLE, [constants.Permission.DOWNLOAD]) - - async with app.state.sm.db.session_factory() as session: - role = Role(name=READER_ROLE, description="Download only") - user = User( - email="reader@test", - hashed_password="not-a-real-hash", - is_active=True, - is_verified=True, - is_superuser=False, - ) - session.add_all([role, user]) - await session.flush() - session.add(UserRole(user_id=user.id, role_id=role.id)) - await session.commit() - user_id = str(user.id) - - return httpx.AsyncClient( - transport=httpx.ASGITransport(app=app), - base_url="http://testserver", - cookies={ - "session": forge_session_cookie( - str(app.state.sm.settings.secret_key), {"user_id": user_id} - ) - }, - ) diff --git a/modules/file_storage/tests/test_delete_permission_modes.py b/modules/file_storage/tests/test_delete_permission_modes.py new file mode 100644 index 00000000..bbe16553 --- /dev/null +++ b/modules/file_storage/tests/test_delete_permission_modes.py @@ -0,0 +1,21 @@ +"""file_storage.delete on the platform ``user`` role follows ``multi_tenant``.""" + +from __future__ import annotations + +import pytest +from file_storage.constants import Permission +from simple_module_hosting.settings import Settings + + +def test_user_lacks_delete_when_multi_tenant(app): + assert app.state.sm.settings.multi_tenant + assert Permission.DELETE not in app.state.sm.permissions.role_map.get("user", []) + + +class TestSingleTenant: + @pytest.fixture + def settings(self, settings: Settings) -> Settings: + return settings.model_copy(update={"multi_tenant": False, "tenant_header": ""}) + + def test_user_keeps_delete(self, app): + assert Permission.DELETE in app.state.sm.permissions.role_map["user"] diff --git a/modules/file_storage/tests/test_file_storage_tenancy.py b/modules/file_storage/tests/test_file_storage_tenancy.py new file mode 100644 index 00000000..0319c46d --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_tenancy.py @@ -0,0 +1,210 @@ +"""``StoredFile`` is tenant-scoped (#383): one organisation never sees another's files. + +Every route answers for the active tenant only — another tenant's file id is +indistinguishable from an unknown one (404, never 403, so ids cannot be probed), +listings and bucket totals count only the tenant's own rows, and the backend +namespaces are disjoint because new keys start with ``{tenant_id}/``. +""" + +from __future__ import annotations + +import httpx +import pytest +from file_storage import constants +from file_storage.models import StoredFile +from simple_module_db import tenant_context +from sqlalchemy import select +from sqlalchemy.exc import IntegrityError + +API = constants.ROUTE_PREFIX_API +VIEW = f"{constants.ROUTE_PREFIX_VIEW}/" +INERTIA = {"X-Inertia": "true", "Accept": "application/json"} + + +async def _upload(client: httpx.AsyncClient, name: str = "a.txt", body: bytes = b"hi") -> dict: + resp = await client.post(f"{API}/upload", files={"file": (name, body, "text/plain")}) + assert resp.status_code == 201, resp.text + return resp.json() + + +async def _browse(client: httpx.AsyncClient) -> dict: + resp = await client.get(VIEW, headers=INERTIA) + assert resp.status_code == 200, resp.text + return resp.json()["props"] + + +class TestCrossTenantAccess: + async def test_another_tenants_file_is_not_found(self, tenant_client): + async with tenant_client() as a, tenant_client() as b: + file_id = (await _upload(a.client))["id"] + + assert (await b.client.get(f"{API}/files/{file_id}")).status_code == 404 + assert (await b.client.get(f"{API}/files/{file_id}/download")).status_code == 404 + assert (await b.client.delete(f"{API}/files/{file_id}")).status_code == 404 + + # ...and the owner still has it, untouched by the attempts. + assert (await a.client.get(f"{API}/files/{file_id}")).status_code == 200 + download = await a.client.get(f"{API}/files/{file_id}/download") + assert download.status_code == 200 + assert download.content == b"hi" + + async def test_bulk_delete_skips_another_tenants_ids(self, tenant_client): + async with tenant_client() as a, tenant_client() as b: + file_id = (await _upload(a.client))["id"] + + resp = await b.client.post(f"{API}/files/bulk-delete", json={"ids": [file_id]}) + + assert resp.status_code == 200, resp.text + assert resp.json() == {"deleted": 0, "ids": []} + assert (await a.client.get(f"{API}/files/{file_id}")).status_code == 200 + + async def test_listing_shows_only_the_tenants_own_files(self, tenant_client): + async with tenant_client() as a, tenant_client() as b: + await _upload(a.client, "a.txt") + await _upload(b.client, "b.txt") + + a_list = (await a.client.get(f"{API}/files")).json() + b_list = (await b.client.get(f"{API}/files")).json() + + assert [f["filename"] for f in a_list["items"]] == ["a.txt"] + assert [f["filename"] for f in b_list["items"]] == ["b.txt"] + assert a_list["total"] == b_list["total"] == 1 + + +class TestKeys: + async def test_new_keys_are_prefixed_with_the_tenant(self, app, tenant_client): + async with tenant_client() as a: + out = await _upload(a.client) + + assert out["key"].startswith(f"{a.tenant_id}/") + # The bytes live under that prefix on the backend, not just in the row. + assert await app.state.file_storage.backend.exists(out["key"]) + + async def test_the_same_key_can_exist_in_two_tenants(self, app): + def row(tenant_id: str) -> StoredFile: + return StoredFile( + tenant_id=tenant_id, + key="shared/key.txt", + filename="key.txt", + content_type="text/plain", + size_bytes=1, + backend=constants.BackendId.FILESYSTEM, + checksum_sha256="0" * 64, + ) + + for tenant_id in ("t-one", "t-two"): + with tenant_context(tenant_id): + async with app.state.sm.db.session_factory() as session: + session.add(row(tenant_id)) + await session.commit() + + # Still unique *within* a tenant. + with tenant_context("t-one"), pytest.raises(IntegrityError): + async with app.state.sm.db.session_factory() as session: + session.add(row("t-one")) + await session.commit() + + with tenant_context("t-two"): + async with app.state.sm.db.session_factory() as session: + rows = (await session.execute(select(StoredFile))).scalars().all() + assert [(r.tenant_id, r.key) for r in rows] == [("t-two", "shared/key.txt")] + + +class TestAggregates: + async def test_totals_and_facets_are_per_tenant(self, tenant_client): + async with tenant_client() as a, tenant_client() as b: + await _upload(a.client, "a.txt", b"12345") + + a_props = await _browse(a.client) + b_props = await _browse(b.client) + + assert a_props["used_bytes"] == 5 + assert [f["value"] for f in a_props["content_types"]] == ["text/plain"] + assert [u["id"] for u in a_props["uploaders"]] == [a.user_id] + assert b_props["used_bytes"] == 0 + assert b_props["content_types"] == [] + assert b_props["uploaders"] == [] + + async def test_a_cached_tenant_total_never_answers_another_tenant(self, tenant_client): + """The cache is warm for A before B renders — B must still get its own.""" + async with tenant_client() as a, tenant_client() as b: + await _upload(a.client, "a.txt", b"123") + await _upload(b.client, "b.txt", b"1234567") + + assert (await _browse(a.client))["used_bytes"] == 3 + assert (await _browse(b.client))["used_bytes"] == 7 + assert (await _browse(a.client))["used_bytes"] == 3 + + # B's write drops B's slot; A's next render is still A's number. + await _upload(b.client, "c.txt", b"1") + assert (await _browse(b.client))["used_bytes"] == 8 + assert (await _browse(a.client))["used_bytes"] == 3 + + +class TestTenantRoles: + async def test_a_member_can_upload_and_download_but_not_delete(self, tenant_client): + async with tenant_client("member") as member: + file_id = (await _upload(member.client))["id"] + + assert (await member.client.get(f"{API}/files/{file_id}/download")).status_code == 200 + assert (await member.client.delete(f"{API}/files/{file_id}")).status_code == 403 + + @pytest.mark.parametrize("role", ["admin", "owner"]) + async def test_admins_and_owners_can_delete(self, tenant_client, role): + async with tenant_client(role) as boss: + file_id = (await _upload(boss.client))["id"] + + assert (await boss.client.delete(f"{API}/files/{file_id}")).status_code == 204 + assert (await boss.client.get(f"{API}/files/{file_id}")).status_code == 404 + + async def test_members_see_the_files_menu_entry(self, tenant_client): + async with tenant_client("member") as member: + props = await _browse(member.client) + + urls = [item["url"] for item in props["menus"]["sidebar"]] + assert f"{constants.ROUTE_PREFIX_VIEW}/" in urls + + +class TestCacheSlots: + async def test_a_write_drops_only_its_own_tenants_slot(self, db_session, db_state): + from file_storage.aggregates import AggregateCache, register_invalidation + + cache = AggregateCache() + register_invalidation(db_state, cache) + for tenant_id in ("t-one", "t-two"): + with tenant_context(tenant_id): + await cache.get(db_session) + + with tenant_context("t-one"): + async with db_state.session_factory() as other: + other.add( + StoredFile( + key="k", + filename="k.txt", + content_type="text/plain", + size_bytes=4, + backend=constants.BackendId.FILESYSTEM, + checksum_sha256="0" * 64, + ) + ) + await other.commit() + assert cache.peek() is None + with tenant_context("t-two"): + assert cache.peek() is not None + + +class TestAuditLabels: + async def test_the_platform_audit_log_names_every_tenants_files(self, app, tenant_client): + """The audit log lists every tenant's entries (and already records the + filename in each), so its resolver names files across tenants — and + works for a platform admin with no tenant bound.""" + from file_storage.module import _resolve_file_labels + + async with tenant_client() as a, tenant_client() as b: + a_id = (await _upload(a.client, "a.txt"))["id"] + b_id = (await _upload(b.client, "b.txt"))["id"] + + async with app.state.sm.db.session_factory() as session: + labels = await _resolve_file_labels(session, [a_id, b_id]) + + assert labels == {a_id: "a.txt", b_id: "b.txt"} diff --git a/modules/file_storage/tests/test_file_storage_tenancy_unbound.py b/modules/file_storage/tests/test_file_storage_tenancy_unbound.py new file mode 100644 index 00000000..cb80d165 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_tenancy_unbound.py @@ -0,0 +1,108 @@ +"""File storage with no tenant bound: single-tenant installs, and strict fail-closed. + +A host with ``multi_tenant`` off binds no tenant; uploads must keep working and +land in ``DEFAULT_TENANT_ID`` (the same value the adoption migration +back-filled). With ``multi_tenant`` on, a caller that has no organisation is +refused *before* any bytes are written, so a failed upload leaves no orphan. +""" + +from __future__ import annotations + +import pytest +from file_storage import constants +from file_storage.models import StoredFile +from simple_module_db import DEFAULT_TENANT_ID, MissingTenantError +from simple_module_hosting.settings import Settings +from simple_module_test.database import database_url_for_tests +from sqlalchemy import select + +API = constants.ROUTE_PREFIX_API + + +class TestSingleTenantInstall: + @pytest.fixture + def settings(self) -> Settings: + return Settings( + database_url=database_url_for_tests(), + environment="testing", + secret_key="test-secret-key", + multi_tenant=False, + auth_provider="users", + ) + + async def test_upload_is_stamped_with_the_default_tenant(self, app, authenticated_client): + resp = await authenticated_client.post( + f"{API}/upload", files={"file": ("a.txt", b"hi", "text/plain")} + ) + + assert resp.status_code == 201, resp.text + out = resp.json() + assert out["key"].startswith(f"{DEFAULT_TENANT_ID}/") + async with app.state.sm.db.session_factory() as session: + row = (await session.execute(select(StoredFile))).scalar_one() + assert row.tenant_id == DEFAULT_TENANT_ID + + download = await authenticated_client.get(f"{API}/files/{out['id']}/download") + assert download.status_code == 200 + assert download.content == b"hi" + listed = (await authenticated_client.get(f"{API}/files")).json() + assert listed["total"] == 1 + + async def test_rows_from_any_tenant_stay_visible(self, app, authenticated_client): + """Unbound reads are not narrowed to the default tenant: a row written + while ``multi_tenant`` was briefly on is still the install's.""" + async with app.state.sm.db.session_factory() as session: + session.add( + StoredFile( + tenant_id="was-a-tenant", + key="old/key.txt", + filename="old.txt", + content_type="text/plain", + size_bytes=1, + backend=constants.BackendId.FILESYSTEM, + checksum_sha256="0" * 64, + ) + ) + await session.commit() + + listed = (await authenticated_client.get(f"{API}/files")).json() + + assert [f["filename"] for f in listed["items"]] == ["old.txt"] + + +class TestStrictWithNoTenant: + async def test_upload_fails_closed_before_writing_bytes(self, app): + """No tenant bound under strict mode: refused, and nothing stored — + the owner is settled before the backend sees a byte.""" + from io import BytesIO + + from fastapi import UploadFile + from file_storage.service import FileStorageService + + services = app.state.file_storage + written: list[str] = [] + original_put = services.backend.put + + async def spy_put(key, *args, **kwargs): + written.append(key) + return await original_put(key, *args, **kwargs) + + services.backend.put = spy_put + try: + async with app.state.sm.db.session_factory() as session: + service = FileStorageService(session, services.backend, services.settings) + with pytest.raises(MissingTenantError): + await service.upload(UploadFile(BytesIO(b"hi"), filename="a.txt")) + finally: + services.backend.put = original_put + + assert written == [] + + async def test_service_reads_raise_without_a_tenant(self, app): + from file_storage.service import FileStorageService + + services = app.state.file_storage + async with app.state.sm.db.session_factory() as session: + service = FileStorageService(session, services.backend, services.settings) + with pytest.raises(MissingTenantError): + await service.list_files() diff --git a/modules/file_storage/tests/test_file_storage_tenant_migration.py b/modules/file_storage/tests/test_file_storage_tenant_migration.py new file mode 100644 index 00000000..26b3c99f --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_tenant_migration.py @@ -0,0 +1,133 @@ +"""Migration ``c7f2d9a41e83``: back-fill existing files into the default tenant or the platform. + +Runs the real Alembic chain against a throwaway SQLite file: rows written +before the migration keep their key and land in ``DEFAULT_TENANT_ID``, except +the files the system branding settings reference, which become platform files +(``PLATFORM_TENANT_ID``) so they stay servable; the unique key widens to +``(tenant_id, key)``, and the downgrade puts it all back. +""" + +from __future__ import annotations + +import uuid +from pathlib import Path + +import pytest +import sqlalchemy as sa +from alembic import command +from alembic.config import Config +from simple_module_db import DEFAULT_TENANT_ID, PLATFORM_TENANT_ID +from sqlalchemy.exc import IntegrityError + +BEFORE = "b5d3f08a6e17" +REVISION = "c7f2d9a41e83" +TABLE = "file_storage_stored_file" +SCRIPTS = Path(__file__).resolve().parents[3] / "host" / "migrations" + + +@pytest.fixture +def migrate(tmp_path, monkeypatch): + db_file = tmp_path / "migrate.db" + monkeypatch.setenv("SM_DATABASE_URL", f"sqlite+aiosqlite:///{db_file}") + # No ini file on purpose: env.py runs ``fileConfig`` for one, which + # disables every logger already created and breaks later caplog tests. + config = Config() + config.set_main_option("script_location", str(SCRIPTS)) + engine = sa.create_engine(f"sqlite:///{db_file}") + yield config, engine + engine.dispose() + + +def _insert(conn, key: str, **extra) -> str: + file_id = uuid.uuid4() + conn.execute( + sa.text( + f"INSERT INTO {TABLE} (id, key, filename, content_type, size_bytes, backend," + " checksum_sha256, extra_metadata, is_deleted" + + "".join(f", {k}" for k in extra) + + ") VALUES (:id, :key, 'f.txt', 'text/plain', 1, 'filesystem', :sum, '{}', 0" + + "".join(f", :{k}" for k in extra) + + ")" + ), + {"id": file_id.hex, "key": key, "sum": "0" * 64, **extra}, + ) + return str(file_id) + + +def _setting(conn, key: str, value: str, *, scope: str = "system", scope_id: str = "") -> None: + conn.execute( + sa.text( + "INSERT INTO settings_setting (scope, scope_id, key, value, value_type, created_at," + " updated_at) VALUES (:scope, :scope_id, :key, :value, 'string'," + " CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" + ), + {"scope": scope, "scope_id": scope_id, "key": key, "value": value}, + ) + + +def _unique_indexes(engine) -> list[list[str]]: + return sorted( + ix["column_names"] for ix in sa.inspect(engine).get_indexes(TABLE) if ix["unique"] + ) + + +# An earlier revision in the chain reflects ``users_user`` under batch mode. +@pytest.mark.filterwarnings("ignore:Skipped unsupported reflection") +def test_upgrade_backfills_and_downgrade_restores(migrate): + config, engine = migrate + command.upgrade(config, BEFORE) + with engine.begin() as conn: + _insert(conn, "2026/01/01/old.txt") + + command.upgrade(config, REVISION) + + with engine.begin() as conn: + rows = conn.execute(sa.text(f"SELECT key, tenant_id FROM {TABLE}")).all() + assert [tuple(r) for r in rows] == [("2026/01/01/old.txt", DEFAULT_TENANT_ID)] + # The same key is now fine in another tenant... + _insert(conn, "2026/01/01/old.txt", tenant_id="other") + assert _unique_indexes(engine) == [["tenant_id", "key"]] + columns = {c["name"]: c for c in sa.inspect(engine).get_columns(TABLE)} + assert columns["tenant_id"]["nullable"] is False + # ...but not twice in the same one. + with pytest.raises(IntegrityError), engine.begin() as conn: + _insert(conn, "2026/01/01/old.txt", tenant_id="other") + + with engine.begin() as conn: + conn.execute(sa.text(f"DELETE FROM {TABLE} WHERE tenant_id = 'other'")) + command.downgrade(config, BEFORE) + + assert "tenant_id" not in {c["name"] for c in sa.inspect(engine).get_columns(TABLE)} + assert _unique_indexes(engine) == [["key"]] + with engine.begin() as conn: + assert conn.execute(sa.text(f"SELECT key FROM {TABLE}")).scalars().all() == [ + "2026/01/01/old.txt" + ] + + +@pytest.mark.filterwarnings("ignore:Skipped unsupported reflection") +def test_branding_referenced_files_become_platform_files(migrate): + config, engine = migrate + command.upgrade(config, BEFORE) + with engine.begin() as conn: + logo = _insert(conn, "2026/01/01/logo.png") + favicon = _insert(conn, "2026/01/01/favicon.ico") + _insert(conn, "2026/01/01/report.pdf") + unrelated = _insert(conn, "2026/01/01/other.png") + _setting(conn, "branding.logo_file_id", logo) + _setting(conn, "branding.favicon_file_id", favicon) + _setting(conn, "branding.logo_dark_file_id", "not-a-uuid") + # Only SYSTEM scope names platform files. + _setting(conn, "branding.logo_file_id", unrelated, scope="user", scope_id="u1") + + command.upgrade(config, REVISION) + + with engine.begin() as conn: + owners = dict(conn.execute(sa.text(f"SELECT key, tenant_id FROM {TABLE}")).all()) + assert owners == { + "2026/01/01/logo.png": PLATFORM_TENANT_ID, + "2026/01/01/favicon.ico": PLATFORM_TENANT_ID, + "2026/01/01/report.pdf": DEFAULT_TENANT_ID, + "2026/01/01/other.png": DEFAULT_TENANT_ID, + } + assert PLATFORM_TENANT_ID != DEFAULT_TENANT_ID diff --git a/modules/file_storage/tests/test_platform_scope.py b/modules/file_storage/tests/test_platform_scope.py new file mode 100644 index 00000000..82094abd --- /dev/null +++ b/modules/file_storage/tests/test_platform_scope.py @@ -0,0 +1,106 @@ +"""``platform=True`` lifts isolation for the platform row only (#383). + +``platform_scope`` is an ``all_tenants()`` block, and a flush inside it flushes +the whole session. The caller's unrelated pending writes must therefore be +flushed *before* the bypass — stamped with, and checked against, the bound +tenant — never carried through it. +""" + +from __future__ import annotations + +import uuid +from io import BytesIO + +import pytest +from fastapi import UploadFile +from file_storage import constants +from file_storage.models import StoredFile +from file_storage.service import FileStorageService +from simple_module_db import ( + DEFAULT_TENANT_ID, + PLATFORM_TENANT_ID, + TenantIsolationError, + all_tenants, + is_valid_tenant_id, + tenant_context, +) +from sqlalchemy import select + + +def _row(**extra) -> StoredFile: + return StoredFile( + key=f"pending/{uuid.uuid4().hex}.txt", + filename="pending.txt", + content_type="text/plain", + size_bytes=1, + backend=constants.BackendId.FILESYSTEM, + checksum_sha256="0" * 64, + **extra, + ) + + +def _service(app, session) -> FileStorageService: + services = app.state.file_storage + return FileStorageService(session, services.backend, services.settings) + + +def _logo() -> UploadFile: + return UploadFile(BytesIO(b"\x89PNG\r\n\x1a\n"), filename="logo.png") + + +def test_the_platform_owner_is_reserved_and_distinct(): + assert PLATFORM_TENANT_ID != DEFAULT_TENANT_ID + assert not is_valid_tenant_id(PLATFORM_TENANT_ID) + with pytest.raises(ValueError), tenant_context(PLATFORM_TENANT_ID): + pass + + +async def test_a_pending_tenant_row_is_stamped_with_the_bound_tenant(app): + async with app.state.sm.db.session_factory() as session: + with tenant_context("acme"): + pending = _row() + session.add(pending) + platform = await _service(app, session).upload(_logo(), platform=True) + await session.commit() + + async with app.state.sm.db.session_factory() as session: + with all_tenants(): + rows = await session.execute(select(StoredFile.id, StoredFile.tenant_id)) + owners = dict(rows.all()) + assert owners == {pending.id: "acme", platform.id: PLATFORM_TENANT_ID} + + +async def test_the_pending_rows_audit_entry_keeps_the_bound_tenant(app): + from audit_log.models import AuditEntry + + async with app.state.sm.db.session_factory() as session: + with tenant_context("acme"): + pending = _row() + session.add(pending) + await _service(app, session).upload(_logo(), platform=True) + await session.commit() + + async with app.state.sm.db.session_factory() as session: + tenants = ( + await session.execute( + select(AuditEntry.tenant_id).where(AuditEntry.entity_id == str(pending.id)) + ) + ).scalars() + assert list(tenants) == ["acme"] + + +async def test_a_pending_cross_tenant_write_is_still_refused(app): + """Before the fix the platform flush ran it under ``all_tenants()``.""" + async with app.state.sm.db.session_factory() as session: + with tenant_context("acme"): + session.add(_row(tenant_id="globex")) + with pytest.raises(TenantIsolationError): + await _service(app, session).upload(_logo(), platform=True) + + +async def test_a_platform_read_does_not_autoflush_pending_writes_unguarded(app): + async with app.state.sm.db.session_factory() as session: + with tenant_context("acme"): + session.add(_row(tenant_id="globex")) + with pytest.raises(TenantIsolationError): + await _service(app, session).get(uuid.uuid4(), platform=True) diff --git a/modules/file_storage/tests/test_uploader_filter.py b/modules/file_storage/tests/test_uploader_filter.py index 8350102f..439314e9 100644 --- a/modules/file_storage/tests/test_uploader_filter.py +++ b/modules/file_storage/tests/test_uploader_filter.py @@ -16,6 +16,7 @@ from file_storage.models import StoredFile from file_storage.service import FileStorageService from file_storage.settings import FileStorageSettings +from simple_module_db import tenant_context from sqlalchemy.ext.asyncio import AsyncSession VIEW_BASE = f"{constants.ROUTE_PREFIX_VIEW}/" @@ -91,11 +92,12 @@ async def test_skips_rows_with_no_uploader(self, tmp_path, db_session: AsyncSess class TestBrowseView: async def test_filters_the_table_by_uploader( - self, app, authenticated_client: httpx.AsyncClient + self, app, authenticated_client: httpx.AsyncClient, admin_tenant_id: str ): - async with app.state.sm.db.session_factory() as session: - await _seed(session, ("mine.txt", ALICE), ("theirs.txt", BOB)) - await session.commit() + with tenant_context(admin_tenant_id): + async with app.state.sm.db.session_factory() as session: + await _seed(session, ("mine.txt", ALICE), ("theirs.txt", BOB)) + await session.commit() resp = await authenticated_client.get( VIEW_BASE, params={"uploaded_by": ALICE}, headers=INERTIA_HEADERS @@ -107,12 +109,13 @@ async def test_filters_the_table_by_uploader( assert props["pagination"]["total"] == 1 async def test_uploader_facet_is_not_narrowed_by_the_active_filter( - self, app, authenticated_client: httpx.AsyncClient + self, app, authenticated_client: httpx.AsyncClient, admin_tenant_id: str ): """A filter that hides its own alternatives is a dead end.""" - async with app.state.sm.db.session_factory() as session: - await _seed(session, ("mine.txt", ALICE), ("theirs.txt", BOB)) - await session.commit() + with tenant_context(admin_tenant_id): + async with app.state.sm.db.session_factory() as session: + await _seed(session, ("mine.txt", ALICE), ("theirs.txt", BOB)) + await session.commit() resp = await authenticated_client.get( VIEW_BASE, params={"uploaded_by": ALICE}, headers=INERTIA_HEADERS diff --git a/modules/tenants/tenants/contracts/schemas.py b/modules/tenants/tenants/contracts/schemas.py index 9fd65205..368c4961 100644 --- a/modules/tenants/tenants/contracts/schemas.py +++ b/modules/tenants/tenants/contracts/schemas.py @@ -7,6 +7,7 @@ from email_validator import EmailNotValidError, validate_email from pydantic import field_validator +from simple_module_db import PLATFORM_TENANT_ID from sqlmodel import Field, SQLModel from tenants.constants import MAX_EMAIL_LEN, MAX_NAME_LEN, MembershipRole @@ -33,6 +34,8 @@ def _strip(cls, value: str) -> str: def _slug_shape(cls, value: str | None) -> str | None: if value is not None and not _SLUG_RE.fullmatch(value): raise ValueError("slug must be 1-50 lowercase letters, digits or inner dashes") + if value == PLATFORM_TENANT_ID: + raise ValueError(f"slug {value!r} is reserved") return value diff --git a/modules/tenants/tenants/models.py b/modules/tenants/tenants/models.py index 4d572437..7375a51e 100644 --- a/modules/tenants/tenants/models.py +++ b/modules/tenants/tenants/models.py @@ -11,9 +11,11 @@ import uuid from datetime import datetime +from simple_module_db import is_valid_tenant_id from simple_module_db.base import create_module_base from simple_module_db.mixins import AuditMixin from sqlalchemy import Column, DateTime, Index, UniqueConstraint, text +from sqlalchemy.orm import validates from sqlmodel import Field from tenants.constants import ( @@ -42,6 +44,14 @@ class Tenant(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] name: str = Field(max_length=MAX_NAME_LEN) status: str = Field(default=TenantStatus.ACTIVE, max_length=20, index=True) + @validates("id") + def _check_id(self, _key: str, value: str) -> str: + # Refuses the reserved ``PLATFORM_TENANT_ID`` (and any malformed id): + # a tenant with that id would own every platform file. + if not is_valid_tenant_id(value): + raise ValueError(f"{value!r} cannot be a tenant id") + return value + class Membership(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] """A user's membership in a tenant, with their role there. diff --git a/modules/tenants/tenants/service.py b/modules/tenants/tenants/service.py index 8492f78b..f646a8d4 100644 --- a/modules/tenants/tenants/service.py +++ b/modules/tenants/tenants/service.py @@ -17,6 +17,7 @@ from typing import Any from simple_module_core.events import Event, EventBus +from simple_module_db import PLATFORM_TENANT_ID from sqlalchemy import func, select from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession @@ -93,7 +94,10 @@ async def get(self, tenant_id: str) -> Tenant | None: async def _free_slug(self, wanted: str) -> str: slug = wanted - while await self.db.scalar(select(Tenant.id).where(Tenant.slug == slug)): + # The reserved platform owner is never handed out, even as a slug. + while slug == PLATFORM_TENANT_ID or await self.db.scalar( + select(Tenant.id).where(Tenant.slug == slug) + ): suffix = secrets.token_hex(2) slug = f"{wanted[: MAX_SLUG_LEN - len(suffix) - 1]}-{suffix}" return slug diff --git a/modules/tenants/tests/test_hardening.py b/modules/tenants/tests/test_hardening.py index f0a86a4a..f386112c 100644 --- a/modules/tenants/tests/test_hardening.py +++ b/modules/tenants/tests/test_hardening.py @@ -124,6 +124,21 @@ async def test_slug_shape_is_enforced(user_client): assert ok.status_code == 201 +async def test_the_platform_owner_is_never_a_tenant(user_client): + """``PLATFORM_TENANT_ID`` owns platform files; no tenant may take it.""" + import pytest + from simple_module_db import PLATFORM_TENANT_ID + from tenants.models import Tenant + + async with user_client("o@x.io") as (owner, _): + resp = await owner.post("/api/tenants/", json={"name": "X", "slug": PLATFORM_TENANT_ID}) + assert resp.status_code == 422 + derived = (await owner.post("/api/tenants/", json={"name": "Platform"})).json() + assert derived["slug"] != PLATFORM_TENANT_ID + with pytest.raises(ValueError): + Tenant(id=PLATFORM_TENANT_ID, slug="p", name="P") + + async def test_concurrent_accepts_of_one_invitation_give_200_and_409(user_client): async with user_client("o@x.io") as (owner, _), user_client("n@x.io") as (new, _): await _org(owner, "Acme") diff --git a/modules/tenants/tests/test_tenant_role_permissions.py b/modules/tenants/tests/test_tenant_role_permissions.py index 81f15a29..89a8cf89 100644 --- a/modules/tenants/tests/test_tenant_role_permissions.py +++ b/modules/tenants/tests/test_tenant_role_permissions.py @@ -56,7 +56,9 @@ def test_tenant_roles_hold_no_wildcard_and_are_distinct_from_admin(self, app): key = tenant_role(role) assert key != ADMIN_ROLE and is_tenant_role(key) assert WILDCARD not in role_map[key] - assert set(role_map[key]) == set(ROLE_PERMISSIONS[role]) + # Other modules map their own permissions onto tenant roles too + # (file_storage, #383), so tenants' own grants are a floor. + assert set(ROLE_PERMISSIONS[role]) <= set(role_map[key]) assert not any(".platform." in p for p in role_map[key]) async def test_tenant_admin_is_refused_platform_routes(self, tenant_client):