From 0bfd5de5464f7ac389d6d4b2802c25487b7ea0a6 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 14:29:33 +0200 Subject: [PATCH 1/6] feat(branding): resolve branding per tenant over the system theme (#373) - Tenants override app_name, primary_color, design_pack, footer_text and the three images as tenant_overridable settings keys (TENANT scope); banner and footer links stay platform-only. Every tenant-scope write is checked: same validators as the system value (422), and an image id must be a live file owned by the tenant being written (404). - branding.tenant_branding resolves per request: system object as is when multi_tenant is off or no tenant is bound (no lookup), else the tenant's overrides merged on top, in a tenant-keyed 30s TTL cache that forgets on settings.values invalidation notices (all tenants on a system change). - Shared props provider is async and leaves the tenant's theme on request.state.branding for the pre-hydration ; the framework now awaits async shared-prop providers. - Anonymous asset routes resolve the tenant (subdomain or active org) and serve a tenant-set image only as that tenant's own file, system values as platform files; tenant images are Cache-Control private and immutable only when ?v= names the file served. - POST/DELETE /api/branding/tenant/{logo,logo-dark,favicon} upload tenant- owned files (no platform=True) and reap the replaced one in the tenant's scope; the organisation settings page offers uploads for file-id keys (SettingDefinition.upload_url). Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- docs/framework/multi-tenancy.md | 5 + docs/modules/branding.md | 70 ++++++- docs/modules/file_storage.md | 6 +- docs/modules/settings.md | 2 +- .../simple_module_hosting/_inertia_setup.py | 6 +- .../simple_module_hosting/_inertia_shared.py | 8 +- .../simple_module_hosting/middleware.py | 2 +- .../simple_module_hosting/shared_props.py | 8 +- framework/hosting/tests/test_branding_head.py | 16 ++ .../tests/test_inertia_shared_providers.py | 12 ++ modules/branding/branding/constants.py | 26 +++ modules/branding/branding/endpoints/assets.py | 55 +++-- .../branding/branding/endpoints/tenant_api.py | 103 ++++++++++ modules/branding/branding/module.py | 17 ++ modules/branding/branding/service.py | 34 ++-- modules/branding/branding/shared_props.py | 21 +- modules/branding/branding/tenant_branding.py | 186 +++++++++++++++++ .../branding/branding/tenant_definitions.py | 103 ++++++++++ modules/branding/tests/test_branding.py | 10 +- .../branding/tests/test_tenant_branding.py | 188 +++++++++++++++++ .../tests/test_tenant_branding_assets.py | 190 ++++++++++++++++++ .../settings/settings/contracts/registry.py | 4 + modules/settings/settings/tenant_view.py | 3 + .../tenants/components/TenantImageControl.tsx | 73 +++++++ .../tenants/components/TenantSettingRow.tsx | 56 ++++-- modules/tenants/tenants/locales/en.json | 3 +- .../tests-js/TenantSettingRow.test.tsx | 23 +++ packages/i18n/src/generated-resources.ts | 1 + packages/i18n/src/keys.generated.ts | 1 + 29 files changed, 1149 insertions(+), 83 deletions(-) create mode 100644 modules/branding/branding/endpoints/tenant_api.py create mode 100644 modules/branding/branding/tenant_branding.py create mode 100644 modules/branding/branding/tenant_definitions.py create mode 100644 modules/branding/tests/test_tenant_branding.py create mode 100644 modules/branding/tests/test_tenant_branding_assets.py create mode 100644 modules/tenants/tenants/components/TenantImageControl.tsx diff --git a/docs/framework/multi-tenancy.md b/docs/framework/multi-tenancy.md index bf5e0592..a450178c 100644 --- a/docs/framework/multi-tenancy.md +++ b/docs/framework/multi-tenancy.md @@ -216,6 +216,11 @@ the platform routes take a tenant id from the URL and 404 an unknown one. Every write publishes a per-(tenant, key) notice on the `settings.values` invalidation channel. See [settings](/modules/settings#tenant-overridable-keys). +`branding` builds on it: a tenant overrides its name, colour, design pack, +footer caption and images, resolved per request with a tenant-keyed cache that +listens on that channel; anonymous visitors get the subdomain's tenant's theme. +See [branding](/modules/branding#per-tenant-branding). + Screens that take a tenant id from the URL can vet it without importing `tenants`: the module publishes `app.state.tenant_exists`, and `await simple_module_core.tenancy.tenant_exists(app, tenant_id)` answers diff --git a/docs/modules/branding.md b/docs/modules/branding.md index 2fc697e3..6a65d3b5 100644 --- a/docs/modules/branding.md +++ b/docs/modules/branding.md @@ -36,6 +36,17 @@ Every JSON endpoint — including the reads — requires `branding.manage`; they `PUT /` only touches the text fields (`app_name`, `primary_color`, `design_pack`, `banner_message`, `banner_severity`); images are set and cleared through their dedicated upload/delete routes. A `design_pack` slug that no installed module registered is rejected with `422` — accepting it would put `"-root"` on the document with no stylesheet behind it, so the site would look unchanged with nothing in the UI explaining why. +### API (tenant) + +A tenant's own images, for tenant owners and admins (`settings.tenant.edit`). +The tenant is always the request's active one — never an id from the URL. See +[Per-tenant branding](#per-tenant-branding). + +| Method + path | Body / response | +|---|---| +| `POST /api/branding/tenant/{logo,logo-dark,favicon}` | `multipart` (field `file`) → the tenant's effective `BrandingOut` | +| `DELETE /api/branding/tenant/{logo,logo-dark,favicon}` | → `BrandingOut` (the tenant falls back to the system image) | + Uploads are validated **before** the bytes reach `file_storage`: an unsupported or unconvincing type returns `415`, an oversized image `413` (see [Image guard-rails](#image-guard-rails)). ### Public assets (anonymous) @@ -50,13 +61,17 @@ 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. +The **system** images are **platform files** (`platform=True` in +`file_storage`): uploaded as the install rather than as the admin's active +organisation, and served by a lookup that only ever matches platform-owned rows. +Images uploaded before `file_storage` adopted tenancy were back-filled into the +platform owner and keep working. + +Which image a request gets follows its tenant — the subdomain for an anonymous +visitor (`tenants`' `subdomain_base`), the active organisation for a member — +else the system's. A value the **tenant** set is read as the tenant's own file +(under `tenant_context(tenant)`); a system value as a platform file. Either way +a setting pointed at anyone else's upload `404`s instead of publishing it. 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. @@ -76,10 +91,12 @@ The published URL carries `?v=`. Replacing an image stores a **new** `f | Request | `Cache-Control` | |---|---| -| With a `?v=` version | `public, max-age=31536000, immutable` (one year) | -| Without a version | `public, max-age=3600` (one hour) | +| `?v=` naming the file served | `public, max-age=31536000, immutable` (one year) | +| No `?v=`, or one naming another file | `public, max-age=3600` (one hour) | -An unversioned URL can serve new bytes later, so it must never be immutable; the short TTL lets it self-correct. A `404` is never cached, so the next request retries once the setting is fixed. +A **tenant's** image is sent `private` instead of `public`: the same URL serves a different image to another tenant on the same host, so no shared cache may store it. + +An unversioned URL can serve new bytes later, so it must never be immutable; the short TTL lets it self-correct. Since the URL answers per tenant, a `?v=` left over from another tenant's page is treated the same way. A `404` is never cached, so the next request retries once the setting is fixed. ## Public contracts @@ -197,6 +214,37 @@ On startup the module registers a shared-props provider (`register_inertia_share The provider is defensive — it returns `{}` if branding state isn't mounted yet, so a half-booted app never errors a render. Because changes go through the settings store, a save hot-reloads `app.state.branding.settings`; the next render reflects the new values without a restart. +## Per-tenant branding + +With `multi_tenant` on, a tenant can override part of the theme for itself +(#373): `app_name`, `primary_color`, `design_pack`, `footer_text` and the three +images. Each is a settings key (`branding.`) declared +`tenant_overridable`, stored at settings' TENANT scope, and edited by tenant +owners/admins on the organisation settings page (`/tenants/settings`) — scalars +through `/api/settings/tenant/current/{key}`, images through +`/api/branding/tenant/{asset}`. The banner and footer links stay platform-only: +the banner is how the platform announces maintenance, and a tenant must not be +able to silence it. + +Every tenant-scope write is checked first — by the same validators as the system +value (`422`), and for an image id, that the file is a live upload **owned by +the tenant being written** (`404` otherwise; a platform file or another tenant's +upload is refused). Platform operators writing a tenant's keys through the +settings platform routes go through the same check. + +Reads resolve per request (`branding.tenant_branding.resolve`): the request's +tenant overrides on top of the system theme, falling back field by field. The +provider leaves the merged object on `request.state.branding`, which the root +template's pre-hydration `` prefers. A hand-edited invalid override +degrades to the system value for that field. + +**Cost.** With `multi_tenant` off, or no tenant on the request, the system +object is used as is — no lookup. A tenant's overrides are read at most once per +30 s per process (a TTL cache keyed by tenant id); the cache subscribes to +settings' `settings.values` invalidation channel and forgets a tenant when one +of its `branding.*` keys changes — every tenant when a system one does — so +edits show at once, in every worker once a transport is installed. + ## Permissions | Code | Granted to | Purpose | @@ -220,5 +268,5 @@ The provider is defensive — it returns `{}` if branding state isn't mounted ye ## Notes -- Branding is SYSTEM-scoped (one identity per deployment). The settings store already supports tenant/user scope, leaving room for per-tenant branding later. +- Branding has no table: the system identity is SYSTEM-scope settings, a tenant's overrides are TENANT-scope settings (see [Per-tenant branding](#per-tenant-branding)). - The primary colour overrides the `--primary` / `--sidebar-primary` CSS variables from a single hex; the full OKLCH colour scale is not regenerated. diff --git a/docs/modules/file_storage.md b/docs/modules/file_storage.md index b167c476..27d4d38a 100644 --- a/docs/modules/file_storage.md +++ b/docs/modules/file_storage.md @@ -92,12 +92,14 @@ off) stamps uploads with `DEFAULT_TENANT_ID` and reads every row; with 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 +branding's *system* logo and favicon — are written and read with `platform=True` on `FileStorageService.upload` / `get` / `download` / `delete`. They are owned by `file_storage.scope.PLATFORM_TENANT_ID` (`DEFAULT_TENANT_ID`, which tenant ids never collide with) 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. Platform files are not listed on any tenant's Files screen. +reached that way. Platform files are not listed on any tenant's Files screen. A tenant's own +branding images are ordinary tenant files (no `platform=True`), uploaded and +served in that tenant's scope. 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 diff --git a/docs/modules/settings.md b/docs/modules/settings.md index 8a81303f..f9f34758 100644 --- a/docs/modules/settings.md +++ b/docs/modules/settings.md @@ -99,7 +99,7 @@ registry.add( `check` runs before **every** TENANT-scope write of the key — the self-service route and the platform routes alike, with the tenant being written (not the -caller's). The declared `value_type` wins over the one a tenant sends. +caller's). The declared `value_type` wins over the one a tenant sends. `upload_url` marks a file-id key (a logo): the tenant settings page offers an upload to that URL (POST, DELETE to clear) instead of a text box. Runtime reads need nothing new: `SettingsDep` is already bound to the active tenant, so `await settings.get(key)` resolves **tenant → system → default**. diff --git a/framework/hosting/simple_module_hosting/_inertia_setup.py b/framework/hosting/simple_module_hosting/_inertia_setup.py index ed0bff27..fac5bef5 100644 --- a/framework/hosting/simple_module_hosting/_inertia_setup.py +++ b/framework/hosting/simple_module_hosting/_inertia_setup.py @@ -48,7 +48,11 @@ def branding_head(request: Request) -> dict: rather than ``None``, because omitting the tag sends the browser to ``/favicon.ico``, which this app does not serve. """ - services = getattr(request.app.state, "branding", None) + # A per-request object (same duck shape) wins: branding resolves a tenant's + # own theme into ``request.state.branding`` from its shared-props provider. + services = getattr(getattr(request, "state", None), "branding", None) or getattr( + request.app.state, "branding", None + ) settings = getattr(services, "settings", None) app_name = getattr(settings, "app_name", "") or _DEFAULT_APP_NAME accent = getattr(settings, "primary_color", "") or "" diff --git a/framework/hosting/simple_module_hosting/_inertia_shared.py b/framework/hosting/simple_module_hosting/_inertia_shared.py index fa665fa5..4b0f717b 100644 --- a/framework/hosting/simple_module_hosting/_inertia_shared.py +++ b/framework/hosting/simple_module_hosting/_inertia_shared.py @@ -2,6 +2,7 @@ from __future__ import annotations +import inspect import logging from collections.abc import Callable from typing import Any @@ -103,19 +104,22 @@ def build_menu_translator(request: Request) -> Callable[[str], str] | None: return translator.t -def merge_shared_prop_providers(app: Any, request: Request, shared: dict) -> None: +async def merge_shared_prop_providers(app: Any, request: Request, shared: dict) -> None: """Merge module-registered Inertia shared-prop providers into ``shared`` in place. Providers are read off ``app.state.inertia_shared_providers`` (never importing the plugin — preserves SM009). A provider that raises is skipped and logged; a provider may not clobber a framework-owned key (auth/menus/i18n) or an earlier - provider's key. + provider's key. A provider may be ``async`` (its result is awaited) — a + per-tenant lookup cannot be answered from a process-wide object. """ providers = getattr(app.state, "inertia_shared_providers", None) or () for provider in providers: name = getattr(provider, "__name__", provider) try: extra = provider(request) + if inspect.isawaitable(extra): + extra = await extra except Exception: # a bad provider must not break the page render logger.warning("shared-prop provider %r raised; skipping", name, exc_info=True) continue diff --git a/framework/hosting/simple_module_hosting/middleware.py b/framework/hosting/simple_module_hosting/middleware.py index f7184d94..91fdbd24 100644 --- a/framework/hosting/simple_module_hosting/middleware.py +++ b/framework/hosting/simple_module_hosting/middleware.py @@ -234,7 +234,7 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: # Merge module-registered shared-prop providers (e.g. branding) — read off # app.state without importing the plugin (SM009), defensively. - merge_shared_prop_providers(scope["app"], request, shared) + await merge_shared_prop_providers(scope["app"], request, shared) request.state.inertia_shared = shared diff --git a/framework/hosting/simple_module_hosting/shared_props.py b/framework/hosting/simple_module_hosting/shared_props.py index a60160ee..442ad2ab 100644 --- a/framework/hosting/simple_module_hosting/shared_props.py +++ b/framework/hosting/simple_module_hosting/shared_props.py @@ -6,8 +6,8 @@ a registered callable off ``app.state`` rather than reaching into module code, keeping the ``SM009`` framework→plugin import ban intact. -A provider is ``Callable[[Request], dict]``. It must be cheap and total — it runs -for every request. :class:`InertiaLayoutDataMiddleware` merges each provider's +A provider is ``Callable[[Request], dict]`` or an ``async`` one. It must be cheap +and total — it runs for every request. :class:`InertiaLayoutDataMiddleware` merges each provider's returned dict into the ``shared`` payload after the built-in ``auth``/``menus``/ ``i18n`` blocks; a provider that raises is skipped and logged, never failing the request. @@ -15,14 +15,14 @@ from __future__ import annotations -from collections.abc import Callable +from collections.abc import Awaitable, Callable from typing import TYPE_CHECKING if TYPE_CHECKING: from fastapi import FastAPI from starlette.requests import Request -SharedPropsProvider = Callable[["Request"], dict] +SharedPropsProvider = Callable[["Request"], dict | Awaitable[dict]] """A function mapping a request to a dict merged into Inertia shared props.""" _STATE_ATTR = "inertia_shared_providers" diff --git a/framework/hosting/tests/test_branding_head.py b/framework/hosting/tests/test_branding_head.py index 03b4c2a2..42db9e3c 100644 --- a/framework/hosting/tests/test_branding_head.py +++ b/framework/hosting/tests/test_branding_head.py @@ -73,3 +73,19 @@ def test_the_default_follows_the_configured_brand_colour() -> None: favicon = branding_head(_request(services))["favicon_url"] assert favicon == default_favicon_data_uri("Acme", "#1a7dd1") assert favicon != default_favicon_data_uri("Acme") + + +def test_a_per_request_branding_wins_over_the_process_wide_one() -> None: + """A tenant's theme left on ``request.state.branding`` by branding's + provider is what the pre-hydration head shows (#373).""" + system = SimpleNamespace(settings=SimpleNamespace(app_name="Platform", primary_color="")) + tenant = SimpleNamespace( + settings=SimpleNamespace(app_name="Acme", primary_color="#112233"), + favicon_url="/api/branding/favicon?v=t", + ) + request = _request(system) + request.state = SimpleNamespace(branding=tenant) + meta = branding_head(request) + assert meta["app_name"] == "Acme" + assert meta["theme_color"] == "#112233" + assert meta["favicon_url"] == "/api/branding/favicon?v=t" diff --git a/framework/hosting/tests/test_inertia_shared_providers.py b/framework/hosting/tests/test_inertia_shared_providers.py index 19a9549f..0bfaf157 100644 --- a/framework/hosting/tests/test_inertia_shared_providers.py +++ b/framework/hosting/tests/test_inertia_shared_providers.py @@ -89,3 +89,15 @@ def test_provider_cannot_clobber_framework_keys(caplog) -> None: assert isinstance(body["auth"], dict) assert body["branding"] == {"ok": True} assert any("reserved shared-prop" in rec.message for rec in caplog.records) + + +def test_async_provider_is_awaited() -> None: + """A provider may be async — branding resolves the request's tenant (#373).""" + app = _build_app() + + async def provider(_req: Request) -> dict: + return {"branding": {"appName": "Tenant"}} + + register_inertia_shared_provider(app, provider) + + assert TestClient(app).get("/shared").json()["branding"] == {"appName": "Tenant"} diff --git a/modules/branding/branding/constants.py b/modules/branding/branding/constants.py index ddfa3770..44137493 100644 --- a/modules/branding/branding/constants.py +++ b/modules/branding/branding/constants.py @@ -198,3 +198,29 @@ def clean_footer_href(value: str) -> str: if not relative and not lowered.startswith(FOOTER_LINK_SCHEMES): raise ValueError(FOOTER_LINK_HREF_ERROR) return cleaned + + +# ── Per-tenant branding (#373) ───────────────────────────────────────── +# The fields a tenant may override for itself, as ``branding.`` rows at +# settings' TENANT scope. Deliberately not the banner (the platform's channel +# for maintenance notices, which a tenant must not be able to silence) nor the +# footer links (a JSON list with an XSS-relevant allow-list, kept platform-only +# until it has a tenant-facing editor). +TENANT_IMAGE_FIELDS: Final = ("logo_file_id", "logo_dark_file_id", "favicon_file_id") +TENANT_FIELDS: Final = ( + "app_name", + "primary_color", + "design_pack", + "footer_text", + *TENANT_IMAGE_FIELDS, +) +#: ``/api/branding/tenant/{asset}`` — upload (POST) / clear (DELETE) one of the +#: active tenant's own images. +PATH_TENANT_ASSET: Final = "/tenant/{asset}" +TENANT_ASSETS: Final = { + "logo": "logo_file_id", + "logo-dark": "logo_dark_file_id", + "favicon": "favicon_file_id", +} +#: Floor for the per-tenant cache; settings' invalidation notices clear it sooner. +TENANT_CACHE_TTL_SECONDS: Final = 30 diff --git a/modules/branding/branding/endpoints/assets.py b/modules/branding/branding/endpoints/assets.py index f5b81ed9..92df599b 100644 --- a/modules/branding/branding/endpoints/assets.py +++ b/modules/branding/branding/endpoints/assets.py @@ -5,11 +5,18 @@ 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. +Which image, and whose (#373): the request's tenant — from the subdomain for +an anonymous visitor, the active organisation for a member — else the system. +The file id is never taken from the request (``?v=`` is only a cache key): + +* a value the **tenant** set is read as that tenant's file — under + ``tenant_context(tenant)``, so only a row the tenant owns matches; +* a **system** value is read as a *platform* file (``platform=True``): an + ``all_tenants()`` lookup restricted to rows owned by + ``file_storage.scope.PLATFORM_TENANT_ID``. + +Either way a setting pointed at someone else's file id serves a 404, never +their file. """ from __future__ import annotations @@ -26,25 +33,33 @@ StoredFileNotFoundError, StreamDownload, ) +from simple_module_db import tenant_context from branding import constants +from branding.tenant_branding import ResolvedBranding, resolve router = APIRouter() logger = logging.getLogger(__name__) -def _cache_control(request: Request) -> str: - """Long-lived + immutable only for a request that carries a real version.""" +def _cache_control(request: Request, file_id: uuid.UUID, tenant_id: str | None) -> str: + """Long-lived + immutable only when ``?v=`` names the file actually served. + + The same URL answers differently per tenant, so a ``?v=`` left over from + another tenant's (or the system's) page must not pin these bytes for a + year. A tenant's image is ``private``: a shared cache keyed on the URL must + not hand it to a visitor of another tenant on the same host. + """ version = request.query_params.get(constants.ASSET_VERSION_QUERY_KEY) - if version: - return f"public, max-age={constants.ASSET_MAX_AGE_VERSIONED}, immutable" - return f"public, max-age={constants.ASSET_MAX_AGE_UNVERSIONED}" + visibility = "private" if tenant_id else "public" + if version and version == str(file_id): + return f"{visibility}, max-age={constants.ASSET_MAX_AGE_VERSIONED}, immutable" + return f"{visibility}, max-age={constants.ASSET_MAX_AGE_UNVERSIONED}" -def _configured_file_id(request: Request, field: str) -> uuid.UUID: - services = getattr(request.app.state, "branding", None) - raw = getattr(services.settings, field, "") if services is not None else "" +def _configured_file_id(resolved: ResolvedBranding, field: str) -> uuid.UUID: + raw = getattr(resolved.settings, field, "") if not raw: raise HTTPException(status_code=404, detail="No branding image is set.") try: @@ -59,9 +74,17 @@ def _configured_file_id(request: Request, field: str) -> uuid.UUID: async def _serve( request: Request, storage: FileStorageService, field: str ) -> RedirectResponse | StreamingResponse: - file_id = _configured_file_id(request, field) + if getattr(request.app.state, "branding", None) is None: + raise HTTPException(status_code=404, detail="No branding image is set.") + resolved = await resolve(request) + file_id = _configured_file_id(resolved, field) + owner = resolved.owner_of(field) try: - download = await storage.download(file_id, platform=True) + if owner is None: + download = await storage.download(file_id, platform=True) + else: + with tenant_context(owner): + download = await storage.download(file_id) 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. @@ -79,7 +102,7 @@ async def _serve( download.body, media_type=row.content_type, headers={ - "Cache-Control": _cache_control(request), + "Cache-Control": _cache_control(request, file_id, owner), "Content-Length": str(row.size_bytes), "ETag": f'"{row.checksum_sha256}"', # `attachment` is ignored for subresource loads (, `` override at it; ``DELETE`` removes the +override, so the tenant falls back to the system image. The replaced file is +reaped in the tenant's own scope. + +Guarded like every other tenant-settings write: ``settings.tenant.edit`` +(tenant owner/admin) and the tenant comes from ``request.state.tenant_id`` +only. Scalar fields go through settings' generic +``/api/settings/tenant/current/{key}``; only uploads need a route of their own. +""" + +from __future__ import annotations + +import logging +import uuid + +from fastapi import APIRouter, Depends, HTTPException, Request, UploadFile +from file_storage.deps import get_file_storage_service +from file_storage.service import FileStorageService +from settings.constants import PERM_TENANT_EDIT +from settings.contracts.schemas import SettingScope, SettingUpsert +from settings.deps import get_setting_service +from settings.service import SettingService +from settings.tenant_scope import active_tenant +from simple_module_hosting.permissions import RequiresPermission + +from branding.constants import PACKAGE, PATH_TENANT_ASSET, TENANT_ASSETS +from branding.contracts.schemas import BrandingOut +from branding.images import validate_image +from branding.service import to_out +from branding.tenant_branding import merge, overrides_from + +router = APIRouter(dependencies=[Depends(RequiresPermission(PERM_TENANT_EDIT))]) +logger = logging.getLogger(__name__) + + +def _field(asset: str) -> str: + field = TENANT_ASSETS.get(asset) + if field is None: + raise HTTPException(status_code=404, detail=f"Unknown branding image {asset!r}.") + return field + + +async def _swap( + request: Request, + settings: SettingService, + storage: FileStorageService, + tenant_id: str, + field: str, + file_id: str | None, +) -> BrandingOut: + """Point the tenant's ``field`` at ``file_id`` (``None`` = drop the override).""" + key = f"{PACKAGE}.{field}" + previous = await settings.get_scoped(SettingScope.TENANT, tenant_id, key) + if file_id is None: + await settings.delete_scoped(SettingScope.TENANT, tenant_id, key) + else: + await settings.upsert_scoped( + SettingScope.TENANT, tenant_id, key, SettingUpsert(value=file_id) + ) + if previous is not None and previous.value and previous.value != file_id: + try: + # Bound to the tenant already (it is the request's), so this can + # only ever delete the tenant's own file. + await storage.delete(uuid.UUID(previous.value)) + except Exception: + logger.warning( + "Could not delete replaced tenant branding image %s.", previous.value, exc_info=True + ) + # Caches drop this tenant when settings' after-commit notice fires; the + # reply reads through this request's session, which sees the write. + overrides = await overrides_from(settings, tenant_id) + return to_out(merge(request.app.state.branding.settings, tenant_id, overrides).settings) + + +@router.post(PATH_TENANT_ASSET, response_model=BrandingOut) +async def upload_tenant_asset( + asset: str, + file: UploadFile, + request: Request, + settings: SettingService = Depends(get_setting_service), + storage: FileStorageService = Depends(get_file_storage_service), +) -> BrandingOut: + field = _field(asset) + tenant_id = active_tenant(request) + await validate_image(file) + stored = await storage.upload(file) + return await _swap(request, settings, storage, tenant_id, field, str(stored.id)) + + +@router.delete(PATH_TENANT_ASSET, response_model=BrandingOut) +async def clear_tenant_asset( + asset: str, + request: Request, + settings: SettingService = Depends(get_setting_service), + storage: FileStorageService = Depends(get_file_storage_service), +) -> BrandingOut: + field = _field(asset) + tenant_id = active_tenant(request) + return await _swap(request, settings, storage, tenant_id, field, None) diff --git a/modules/branding/branding/module.py b/modules/branding/branding/module.py index 4a0d1477..232c6243 100644 --- a/modules/branding/branding/module.py +++ b/modules/branding/branding/module.py @@ -9,6 +9,7 @@ import importlib.resources from pathlib import Path +from typing import TYPE_CHECKING from fastapi import APIRouter, FastAPI from simple_module_core.menu import MenuItem, MenuRegistry, MenuSection @@ -19,6 +20,9 @@ from branding import constants from branding.constants import MENU_URL +if TYPE_CHECKING: + from simple_module_core.invalidation import InvalidationBus + class BrandingModule(ModuleBase): meta = ModuleMeta( @@ -46,6 +50,16 @@ def register_settings(self, app: FastAPI) -> None: # editor links there instead of double-editing the same fields. manage_url=MENU_URL, ) + # Tenants may override a subset for themselves (#373), through + # settings' tenant surface; resolved per request by tenant_branding. + from branding.tenant_definitions import register_tenant_definitions + + register_tenant_definitions(app) + + def register_invalidations(self, bus: InvalidationBus, app: FastAPI) -> None: + from branding.tenant_branding import subscribe + + subscribe(bus) def register_permissions(self, registry: PermissionRegistry) -> None: registry.add_group( @@ -84,9 +98,12 @@ def register_menu_items(self, registry: MenuRegistry) -> None: def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: from branding.endpoints.api import router as api from branding.endpoints.assets import router as assets + from branding.endpoints.tenant_api import router as tenant_api from branding.endpoints.views import router as views api_router.include_router(api) + # Tenant owners/admins upload their organisation's own images (#373). + api_router.include_router(tenant_api) # Anonymous logo/favicon routes — a guest sees them on the sign-in and # public pages, so they carry no permission dependency. api_router.include_router(assets) diff --git a/modules/branding/branding/service.py b/modules/branding/branding/service.py index 063293ca..77d3a501 100644 --- a/modules/branding/branding/service.py +++ b/modules/branding/branding/service.py @@ -21,9 +21,27 @@ from file_storage.service import FileStorageService from sqlalchemy.ext.asyncio import AsyncSession + from branding.settings import BrandingSettings + logger = logging.getLogger(__name__) +def to_out(settings: BrandingSettings) -> BrandingOut: + """The API view of one branding settings object (system or a tenant's).""" + return BrandingOut( + app_name=settings.app_name, + primary_color=settings.primary_color, + design_pack=settings.design_pack, + logo_url=asset_url(LOGO_URL, settings.logo_file_id), + logo_dark_url=asset_url(LOGO_DARK_URL, settings.logo_dark_file_id), + favicon_url=asset_url(FAVICON_URL, settings.favicon_file_id), + banner_message=settings.banner_message, + banner_severity=settings.banner_severity, + footer_text=settings.footer_text, + footer_links=list(settings.footer_links), + ) + + class BrandingService: """Read/update the application's branding.""" @@ -41,19 +59,9 @@ def __init__( self.storage = storage def current(self) -> BrandingOut: - settings = self.app.state.branding.settings - return BrandingOut( - app_name=settings.app_name, - primary_color=settings.primary_color, - design_pack=settings.design_pack, - logo_url=asset_url(LOGO_URL, settings.logo_file_id), - logo_dark_url=asset_url(LOGO_DARK_URL, settings.logo_dark_file_id), - favicon_url=asset_url(FAVICON_URL, settings.favicon_file_id), - banner_message=settings.banner_message, - banner_severity=settings.banner_severity, - footer_text=settings.footer_text, - footer_links=list(settings.footer_links), - ) + """The *system* branding — what platform admins edit here. A tenant's + effective theme is ``tenant_branding.resolve``.""" + return to_out(self.app.state.branding.settings) async def apply(self, changes: dict[str, Any]) -> BrandingOut: """Persist and hot-swap the given field changes, then return current.""" diff --git a/modules/branding/branding/shared_props.py b/modules/branding/branding/shared_props.py index 492ee803..78277f94 100644 --- a/modules/branding/branding/shared_props.py +++ b/modules/branding/branding/shared_props.py @@ -62,13 +62,24 @@ def branding_payload(settings: BrandingSettings) -> dict: } -def branding_shared_props(request: Request) -> dict: - """Provider: emit ``{"branding": {...}}`` from the live module settings. +async def branding_shared_props(request: Request) -> dict: + """Provider: emit ``{"branding": {...}}`` for the request's tenant (#373). + + The system theme when ``multi_tenant`` is off or no tenant is bound — the + process-wide object, with no lookup. For a tenant, its overrides on top + (``tenant_branding.resolve``), which are also left on + ``request.state.branding`` so the root template's pre-hydration ```` + (title, theme colour, favicon) matches the page. Defensive — returns ``{}`` if the branding state isn't mounted yet, so a half-booted app never errors a page render. """ - services = getattr(request.app.state, "branding", None) - if services is None: + from branding.services import BrandingServices + from branding.tenant_branding import resolve + + if getattr(request.app.state, "branding", None) is None: return {} - return {"branding": branding_payload(services.settings)} + resolved = await resolve(request) + if resolved.tenant_fields: + request.state.branding = BrandingServices(settings=resolved.settings) + return {"branding": branding_payload(resolved.settings)} diff --git a/modules/branding/branding/tenant_branding.py b/modules/branding/branding/tenant_branding.py new file mode 100644 index 00000000..44794f24 --- /dev/null +++ b/modules/branding/branding/tenant_branding.py @@ -0,0 +1,186 @@ +"""Per-tenant branding: the request's tenant's overrides over the system theme (#373). + +Branding has no table. The system theme is ``app.state.branding.settings`` +(SYSTEM-scope settings, hydrated at boot, hot-swapped on save); a tenant's own +values are ``branding.`` rows at settings' TENANT scope, for the fields +in ``TENANT_FIELDS``. :func:`resolve` merges the two for one request. + +Cost model: + +* ``multi_tenant`` off, or no tenant on the request — returns the system + object as is. No lookup, no allocation: a single-tenant install renders + exactly as before. +* a tenant — one read of its override rows per :data:`TENANT_CACHE_TTL_SECONDS` + per process, cached by tenant id. The merged result is cached alongside and + rebuilt (without a read) when the system object is swapped. + +The cache only ever *forgets* on settings' ``settings.values`` invalidation +notices (a tenant's ``branding.*`` row changed, or a system one — which every +tenant inherits); the TTL is the floor when a notice is lost. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass, field +from typing import TYPE_CHECKING, Any + +from cachetools import TTLCache +from pydantic import ValidationError + +from branding.constants import PACKAGE, TENANT_CACHE_TTL_SECONDS, TENANT_FIELDS +from branding.settings import BrandingSettings + +if TYPE_CHECKING: + from fastapi import FastAPI + from settings.service import SettingService + from simple_module_core.invalidation import Invalidation, InvalidationBus + from starlette.requests import Request + +logger = logging.getLogger(__name__) + +_PREFIX = f"{PACKAGE}." + +# tenant id -> (raw overrides, system object merged against, merged result) +_CACHE: TTLCache[str, tuple[dict[str, str], BrandingSettings, ResolvedBranding]] = TTLCache( + maxsize=10_000, ttl=TENANT_CACHE_TTL_SECONDS +) +# Bumped by every forget, so a read that started before an invalidation does +# not store its (possibly stale) result after it. +_epoch = 0 + + +@dataclass(frozen=True, slots=True) +class ResolvedBranding: + """The theme one request sees, and which fields its tenant supplied.""" + + settings: BrandingSettings + tenant_id: str | None = None + tenant_fields: frozenset[str] = field(default_factory=frozenset) + + def owner_of(self, name: str) -> str | None: + """The tenant owning ``name``'s value, or ``None`` for the platform's.""" + return self.tenant_id if name in self.tenant_fields else None + + +def forget(tenant_id: str | None = None) -> None: + """Drop one tenant's entry, or every entry when ``tenant_id`` is ``None``.""" + global _epoch + _epoch += 1 + if tenant_id is None: + _CACHE.clear() + else: + _CACHE.pop(tenant_id, None) + + +def _on_settings_changed(inv: Invalidation) -> None: + from settings.contracts.invalidation import parse_invalidation_key + + tenant_id, key = parse_invalidation_key(inv.key) + if key is not None and not key.startswith(_PREFIX): + return # someone else's setting + # A system value is inherited by every tenant without its own override. + forget(tenant_id) + + +def subscribe(bus: InvalidationBus) -> None: + from settings.constants import INVALIDATION_CHANNEL + + bus.subscribe(INVALIDATION_CHANNEL, _on_settings_changed) + + +def tenancy_active(app: FastAPI) -> bool: + settings = getattr(getattr(app.state, "sm", None), "settings", None) + return bool(getattr(settings, "multi_tenant", False)) + + +def request_tenant(request: Request) -> str | None: + """The tenant whose branding this request sees; ``None`` means the system's.""" + if not tenancy_active(request.app): + return None + return getattr(request.state, "tenant_id", None) or None + + +async def overrides_from(service: SettingService, tenant_id: str) -> dict[str, str]: + """The tenant's ``TENANT_FIELDS`` overrides, as seen by ``service``'s session.""" + from settings.contracts.schemas import SettingScope + + rows = await service.list_by_scope_unmasked(SettingScope.TENANT, tenant_id) + out: dict[str, str] = {} + for row in rows: + name = row.key.removeprefix(_PREFIX) + if row.key.startswith(_PREFIX) and name in TENANT_FIELDS: + out[name] = row.value + return out + + +async def read_overrides(app: FastAPI, tenant_id: str) -> dict[str, str]: + """:func:`overrides_from` on a short-lived session of its own. + + Only for reads ahead of a request's own work (shared props, the asset + routes): a request that has already written must use its own session. + """ + from settings.service import SettingService + + async with app.state.sm.db.session_factory() as db: + return await overrides_from(SettingService(db), tenant_id) + + +def merge(system: BrandingSettings, tenant_id: str, overrides: dict[str, str]) -> ResolvedBranding: + """``system`` with ``overrides`` on top; a value that fails validation is dropped. + + Writes are validated, so a bad value means a hand-edited row — it must + degrade to the system value for that field, not break every page render. + """ + good = dict(overrides) + base: dict[str, Any] = system.model_dump() + while good: + try: + merged = BrandingSettings(**{**base, **good}) + return ResolvedBranding(merged, tenant_id, frozenset(good)) + except ValidationError as exc: + bad = {str(err["loc"][0]) for err in exc.errors() if err.get("loc")} + logger.warning("Tenant %s branding override(s) %s are invalid.", tenant_id, bad) + if not bad & good.keys(): + break + for name in bad: + good.pop(name, None) + return ResolvedBranding(system, tenant_id, frozenset()) + + +async def resolve_for(app: FastAPI, tenant_id: str | None) -> ResolvedBranding: + system: BrandingSettings = app.state.branding.settings + if not tenant_id: + return ResolvedBranding(system) + hit = _CACHE.get(tenant_id) + if hit is not None: + overrides, merged_against, resolved = hit + if merged_against is system: + return resolved + else: + started = _epoch + overrides = await read_overrides(app, tenant_id) + if _epoch != started: + return merge(system, tenant_id, overrides) # don't cache a racing read + resolved = merge(system, tenant_id, overrides) + _CACHE[tenant_id] = (overrides, system, resolved) + return resolved + + +async def resolve(request: Request) -> ResolvedBranding: + """The branding ``request`` sees: its tenant's, falling back to the system's.""" + return await resolve_for(request.app, request_tenant(request)) + + +__all__ = [ + "ResolvedBranding", + "forget", + "merge", + "overrides_from", + "read_overrides", + "request_tenant", + "resolve", + "resolve_for", + "subscribe", + "tenancy_active", +] diff --git a/modules/branding/branding/tenant_definitions.py b/modules/branding/branding/tenant_definitions.py new file mode 100644 index 00000000..cdef3847 --- /dev/null +++ b/modules/branding/branding/tenant_definitions.py @@ -0,0 +1,103 @@ +"""Declare branding's tenant-overridable settings keys (#373). + +Each field in ``TENANT_FIELDS`` becomes a ``SettingDefinition`` with +``tenant_overridable=True``, so a tenant owner/admin can set it for their own +tenant through settings' ``/api/settings/tenant/current/{key}`` and see it on +the organisation settings page. Every TENANT-scope write — self-service or a +platform operator writing on a tenant's behalf — runs :func:`_check` first: + +* a scalar must pass the same validator the system value does (422); +* a design pack must be one an installed module provides (422); +* an image id must name a live file **owned by the tenant being written** + (404 otherwise) — a tenant cannot point its logo at another tenant's upload, + nor at a platform file. +""" + +from __future__ import annotations + +import uuid +from typing import TYPE_CHECKING + +from pydantic import ValidationError +from simple_module_db import all_tenants +from sqlalchemy import select + +from branding.constants import ( + PACKAGE, + ROUTE_PREFIX, + TENANT_ASSETS, + TENANT_FIELDS, + TENANT_IMAGE_FIELDS, +) +from branding.settings import BrandingSettings + +if TYPE_CHECKING: + from fastapi import FastAPI + from starlette.requests import Request + +_DESCRIPTIONS = { + "app_name": "Application name shown in the header, page titles and emails.", + "primary_color": "Accent colour as #rrggbb; empty uses the theme default.", + "design_pack": "Design pack slug; empty uses the base look.", + "footer_text": "Footer caption; empty shows the platform's.", + "logo_file_id": "Logo image.", + "logo_dark_file_id": "Logo for dark surfaces; falls back to the logo.", + "favicon_file_id": "Browser tab icon.", +} +_ASSET_OF = {field: asset for asset, field in TENANT_ASSETS.items()} + + +async def tenant_owns_file(app: FastAPI, tenant_id: str, raw: str) -> bool: + """Whether ``raw`` is the id of a live (not deleted) file owned by ``tenant_id``.""" + from file_storage.models import StoredFile + + try: + file_id = uuid.UUID(raw) + except ValueError: + return False + stmt = select(StoredFile.id).where(StoredFile.id == file_id, StoredFile.tenant_id == tenant_id) + # An explicit owner condition, so the answer does not depend on which + # tenant (if any) the caller happens to be bound to. + with all_tenants(): + async with app.state.sm.db.session_factory() as db: + return (await db.execute(stmt)).first() is not None + + +def _make_check(name: str): + async def check(request: Request, tenant_id: str, value: str) -> None: + if name in TENANT_IMAGE_FIELDS: + if value and not await tenant_owns_file(request.app, tenant_id, value): + raise LookupError("No such image in this organisation.") + return + try: + BrandingSettings(**{name: value}) + except ValidationError as exc: + raise ValueError(exc.errors()[0]["msg"]) from exc + if name == "design_pack" and value: + registry = getattr(request.app.state, "design_packs", None) + if registry is None or not registry.has(value): + raise ValueError(f"Unknown design pack {value!r}.") + + return check + + +def register_tenant_definitions(app: FastAPI) -> None: + from settings.contracts.registry import SettingDefinition + + registry = app.state.settings.registry + defaults = BrandingSettings().model_dump() + for name in TENANT_FIELDS: + asset = _ASSET_OF.get(name) + registry.add( + SettingDefinition( + key=f"{PACKAGE}.{name}", + default=str(defaults[name]), + description=_DESCRIPTIONS[name], + tenant_overridable=True, + check=_make_check(name), + upload_url=f"{ROUTE_PREFIX}/tenant/{asset}" if asset else "", + ) + ) + + +__all__ = ["register_tenant_definitions", "tenant_owns_file"] diff --git a/modules/branding/tests/test_branding.py b/modules/branding/tests/test_branding.py index 64b13d94..63ce858d 100644 --- a/modules/branding/tests/test_branding.py +++ b/modules/branding/tests/test_branding.py @@ -87,15 +87,15 @@ def test_branding_payload_set() -> None: assert payload["faviconUrl"] == "/api/branding/favicon?v=def-456" -def test_provider_returns_empty_when_state_absent() -> None: +async def test_provider_returns_empty_when_state_absent() -> None: request = SimpleNamespace(app=SimpleNamespace(state=SimpleNamespace())) - assert branding_shared_props(request) == {} # type: ignore[arg-type] + assert await branding_shared_props(request) == {} # type: ignore[arg-type] -def test_provider_emits_branding_block() -> None: +async def test_provider_emits_branding_block() -> None: state = SimpleNamespace(branding=SimpleNamespace(settings=BrandingSettings(app_name="Acme"))) - request = SimpleNamespace(app=SimpleNamespace(state=state)) - out = branding_shared_props(request) # type: ignore[arg-type] + request = SimpleNamespace(app=SimpleNamespace(state=state), state=SimpleNamespace()) + out = await branding_shared_props(request) # type: ignore[arg-type] assert out["branding"]["appName"] == "Acme" diff --git a/modules/branding/tests/test_tenant_branding.py b/modules/branding/tests/test_tenant_branding.py new file mode 100644 index 00000000..fc70c0e0 --- /dev/null +++ b/modules/branding/tests/test_tenant_branding.py @@ -0,0 +1,188 @@ +"""Per-tenant branding in shared props, its fallback and its cache (#373). + +A tenant's ``branding.`` overrides (settings, TENANT scope) sit on top +of the system theme for requests acting for that tenant; everyone else — and +every request on a host with ``multi_tenant`` off — sees the system theme from +the process-wide object, with no lookup. +""" + +from __future__ import annotations + +import httpx +import pytest +from branding import tenant_branding +from settings.constants import INVALIDATION_CHANNEL +from settings.contracts.invalidation import invalidation_key +from settings.contracts.schemas import SettingScope, SettingUpsert +from settings.service import SettingService +from simple_module_hosting.settings import Settings +from simple_module_test.database import database_url_for_tests + +INERTIA = {"X-Inertia": "true", "Accept": "application/json"} +CURRENT = "/api/settings/tenant/current" + + +@pytest.fixture(autouse=True) +def _fresh_cache(): + tenant_branding.forget() + yield + tenant_branding.forget() + + +async def _branding(client: httpx.AsyncClient, page: str = "/tenants/") -> dict: + resp = await client.get(page, headers=INERTIA) + assert resp.status_code == 200, resp.text + return resp.json()["props"]["branding"] + + +async def _set(client: httpx.AsyncClient, field: str, value: str) -> httpx.Response: + return await client.put(f"{CURRENT}/branding.{field}", json={"value": value}) + + +async def _write_row(app, tenant_id: str, field: str, value: str) -> None: + """Behind the API's back: no invalidation notice is published.""" + async with app.state.sm.db.session_factory() as db: + await SettingService(db).upsert_scoped( + SettingScope.TENANT, tenant_id, f"branding.{field}", SettingUpsert(value=value) + ) + await db.commit() + + +class TestPerTenantProps: + async def test_two_tenants_see_their_own_branding(self, tenant_client): + async with tenant_client() as a, tenant_client() as b: + assert (await _set(a.client, "app_name", "Acme")).status_code == 200 + assert (await _set(b.client, "app_name", "Beta")).status_code == 200 + await _set(b.client, "primary_color", "#112233") + + got_a, got_b = await _branding(a.client), await _branding(b.client) + assert (got_a["appName"], got_a["primaryColor"]) == ("Acme", None) + assert (got_b["appName"], got_b["primaryColor"]) == ("Beta", "#112233") + + async def test_unset_fields_fall_back_to_the_system_theme( + self, authenticated_client, tenant_client + ): + resp = await authenticated_client.put( + "/api/branding/", json={"app_name": "Platform", "primary_color": "#abcdef"} + ) + assert resp.status_code == 200, resp.text + async with tenant_client() as a, tenant_client() as plain: + await _set(a.client, "app_name", "Acme") + got_a, got_plain = await _branding(a.client), await _branding(plain.client) + assert (got_a["appName"], got_a["primaryColor"]) == ("Acme", "#abcdef") + assert (got_plain["appName"], got_plain["primaryColor"]) == ("Platform", "#abcdef") + # The platform admin acts for no tenant: the system theme. + assert (await _branding(authenticated_client))["appName"] == "Platform" + + async def test_a_system_change_reaches_tenants_without_an_override( + self, authenticated_client, tenant_client + ): + async with tenant_client() as a: + await _branding(a.client) # warm the cache + await authenticated_client.put("/api/branding/", json={"app_name": "Renamed"}) + assert (await _branding(a.client))["appName"] == "Renamed" + + async def test_members_see_it_but_cannot_change_it(self, tenant_client): + async with tenant_client() as owner: + await _set(owner.client, "app_name", "Acme") + async with tenant_client("member", tenant_id=owner.tenant_id) as member: + assert (await _branding(member.client))["appName"] == "Acme" + assert (await _set(member.client, "app_name", "Mine")).status_code == 403 + + @pytest.mark.parametrize( + ("field", "value"), + [("primary_color", "red"), ("app_name", " "), ("design_pack", "no-such-pack")], + ) + async def test_invalid_values_are_refused(self, tenant_client, field, value): + async with tenant_client() as a: + assert (await _set(a.client, field, value)).status_code == 422 + + async def test_platform_only_fields_are_not_overridable(self, tenant_client): + async with tenant_client() as a: + assert (await _set(a.client, "banner_message", "hi")).status_code == 422 + assert (await _set(a.client, "footer_links", "[]")).status_code == 422 + + async def test_a_hand_edited_bad_row_degrades_to_the_system_value(self, app, tenant_client): + async with tenant_client() as a: + await _write_row(app, a.tenant_id, "primary_color", "not-a-colour") + await _write_row(app, a.tenant_id, "app_name", "Fine") + got = await _branding(a.client) + assert (got["appName"], got["primaryColor"]) == ("Fine", None) + + async def test_the_head_shows_the_tenants_name(self, tenant_client): + async with tenant_client() as a: + await _set(a.client, "app_name", "Acme Head") + html = (await a.client.get("/tenants/")).text + assert " 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_branding_is_the_system_theme_with_no_lookup( + self, app, authenticated_client, monkeypatch + ): + async def no_lookup(*_args, **_kwargs): + raise AssertionError("multi_tenant off must not read tenant overrides") + + monkeypatch.setattr(tenant_branding, "read_overrides", no_lookup) + await _write_row(app, "default", "app_name", "Should not show") + await authenticated_client.put("/api/branding/", json={"app_name": "Single"}) + + got = await _branding(authenticated_client, "/admin/branding/") + assert got["appName"] == "Single" diff --git a/modules/branding/tests/test_tenant_branding_assets.py b/modules/branding/tests/test_tenant_branding_assets.py new file mode 100644 index 00000000..7eaafa9b --- /dev/null +++ b/modules/branding/tests/test_tenant_branding_assets.py @@ -0,0 +1,190 @@ +"""Tenant-owned branding images: upload, serve, subdomain, ownership (#373). + +A tenant's logo is a file in *its own* ``file_storage`` namespace (never a +platform file), served by the anonymous asset routes only to requests resolved +to that tenant — a member's session or the tenant's subdomain — and only while +the file is the tenant's. No route takes a file id from the request. +""" + +from __future__ import annotations + +import httpx +import pytest +from branding import tenant_branding +from branding.constants import LOGO_URL +from file_storage.models import StoredFile +from file_storage.scope import PLATFORM_TENANT_ID +from settings.contracts.schemas import SettingScope, SettingUpsert +from settings.service import SettingService +from simple_module_db import all_tenants +from sqlalchemy import select +from tenants.host_resolver import forget_hosts +from tenants.models import Tenant + +_PNG = b"\x89PNG\r\n\x1a\n" + b"\x00" * 32 +TENANT_LOGO = "/api/branding/tenant/logo" +CURRENT = "/api/settings/tenant/current" + + +@pytest.fixture(autouse=True) +def _fresh_cache(): + tenant_branding.forget() + yield + tenant_branding.forget() + + +async def _upload(client: httpx.AsyncClient, body: bytes = _PNG) -> dict: + resp = await client.post(TENANT_LOGO, files={"file": ("logo.png", body, "image/png")}) + assert resp.status_code == 200, resp.text + return resp.json() + + +async def _files(app) -> list[StoredFile]: + with all_tenants(): + async with app.state.sm.db.session_factory() as db: + stmt = select(StoredFile).execution_options(include_deleted=True) + return sorted((await db.execute(stmt)).scalars().all(), key=lambda r: r.created_at) + + +async def _plain_upload(client: httpx.AsyncClient) -> str: + resp = await client.post( + "/api/file-storage/upload", files={"file": ("x.png", _PNG, "image/png")} + ) + assert resp.status_code == 201, resp.text + return resp.json()["id"] + + +class TestUploadAndServe: + async def test_the_upload_is_the_tenants_file_and_only_it_sees_it(self, app, tenant_client): + async with tenant_client() as a, tenant_client() as b: + out = await _upload(a.client) + [row] = await _files(app) + assert row.tenant_id == a.tenant_id + assert out["logo_url"] == f"{LOGO_URL}?v={row.id}" + + resp = await a.client.get(out["logo_url"]) + assert resp.status_code == 200 + assert resp.content == _PNG + # Private: the same URL answers differently for another tenant. + assert resp.headers["cache-control"].startswith("private") + assert "immutable" in resp.headers["cache-control"] + + assert (await b.client.get(LOGO_URL)).status_code == 404 # no system logo + + async def test_a_platform_admin_upload_is_still_a_platform_file( + self, app, authenticated_client, tenant_client + ): + await authenticated_client.post( + "/api/branding/logo", files={"file": ("p.png", _PNG + b"p", "image/png")} + ) + async with tenant_client() as a: + # The tenant has no logo of its own: it inherits the platform's. + resp = await a.client.get(LOGO_URL) + assert resp.content == _PNG + b"p" + assert resp.headers["cache-control"].startswith("public") + await _upload(a.client) + assert (await a.client.get(LOGO_URL)).content == _PNG + owners = {r.tenant_id for r in await _files(app)} + assert owners == {PLATFORM_TENANT_ID, a.tenant_id} + + async def test_replacing_and_clearing_reap_the_tenants_old_file(self, app, tenant_client): + async with tenant_client() as a: + await _upload(a.client, _PNG) + await _upload(a.client, _PNG + b"\x01") + cleared = await a.client.delete(TENANT_LOGO) + assert cleared.status_code == 200 + assert cleared.json()["logo_url"] is None + assert (await a.client.get(LOGO_URL)).status_code == 404 + assert [r.is_deleted for r in await _files(app)] == [True, True] + + async def test_member_cannot_upload(self, tenant_client): + async with ( + tenant_client() as owner, + tenant_client("member", tenant_id=owner.tenant_id) as member, + ): + resp = await member.client.post( + TENANT_LOGO, files={"file": ("l.png", _PNG, "image/png")} + ) + assert resp.status_code == 403 + + async def test_unknown_asset_is_404(self, tenant_client): + async with tenant_client() as a: + resp = await a.client.post( + "/api/branding/tenant/banner", files={"file": ("l.png", _PNG, "image/png")} + ) + assert resp.status_code == 404 + + +class TestSubdomain: + @pytest.fixture + def subdomains(self, app): + app.state.tenants.settings.subdomain_base = "example.com" + forget_hosts() + yield + app.state.tenants.settings.subdomain_base = "" + forget_hosts() + + async def test_an_anonymous_visitor_gets_that_tenants_logo( + self, app, subdomains, tenant_client + ): + async with tenant_client() as a: + await _upload(a.client) + async with app.state.sm.db.session_factory() as db: + slug = (await db.get(Tenant, a.tenant_id)).slug + + def anon(host: str) -> httpx.AsyncClient: + return httpx.AsyncClient(transport=httpx.ASGITransport(app=app), base_url=host) + + async with anon(f"http://{slug}.example.com") as visitor: + resp = await visitor.get(LOGO_URL) + assert resp.status_code == 200 + assert resp.content == _PNG + async with anon("http://example.com") as visitor: + assert (await visitor.get(LOGO_URL)).status_code == 404 # the system's: none + + +class TestOwnership: + async def test_a_tenant_cannot_point_its_logo_at_another_tenants_file(self, tenant_client): + async with tenant_client() as a, tenant_client() as b: + theirs = await _plain_upload(b.client) + ours = await _plain_upload(a.client) + key = f"{CURRENT}/branding.logo_file_id" + assert (await a.client.put(key, json={"value": theirs})).status_code == 404 + assert (await a.client.put(key, json={"value": "not-a-uuid"})).status_code == 404 + assert (await a.client.put(key, json={"value": ours})).status_code == 200 + assert (await a.client.get(LOGO_URL)).status_code == 200 + + async def test_nor_at_a_platform_file(self, app, authenticated_client, tenant_client): + await authenticated_client.post( + "/api/branding/logo", files={"file": ("p.png", _PNG, "image/png")} + ) + platform_id = str(app.state.branding.settings.logo_file_id) + async with tenant_client() as a: + resp = await a.client.put( + f"{CURRENT}/branding.logo_file_id", json={"value": platform_id} + ) + assert resp.status_code == 404 + + async def test_a_platform_operator_writing_for_a_tenant_is_checked_too( + self, authenticated_client, tenant_client + ): + async with tenant_client() as a, tenant_client() as b: + theirs = await _plain_upload(b.client) + url = f"/api/settings/tenant/{a.tenant_id}/branding.logo_file_id" + assert (await authenticated_client.put(url, json={"value": theirs})).status_code == 404 + + async def test_a_hand_edited_row_naming_another_tenants_file_serves_404( + self, app, tenant_client + ): + async with tenant_client() as a, tenant_client() as b: + theirs = await _plain_upload(b.client) + async with app.state.sm.db.session_factory() as db: + await SettingService(db).upsert_scoped( + SettingScope.TENANT, + a.tenant_id, + "branding.logo_file_id", + SettingUpsert(value=theirs), + ) + await db.commit() + assert (await a.client.get(LOGO_URL)).status_code == 404 + assert (await b.client.get(f"/api/file-storage/files/{theirs}")).status_code == 200 diff --git a/modules/settings/settings/contracts/registry.py b/modules/settings/settings/contracts/registry.py index 3a7a9ba4..fc49dc0c 100644 --- a/modules/settings/settings/contracts/registry.py +++ b/modules/settings/settings/contracts/registry.py @@ -53,6 +53,9 @@ class SettingDefinition: ``/api/settings/tenant/current/{key}``. Keys without it are writable at tenant scope only by platform operators (``settings.edit``). ``check`` vets every TENANT-scope write of the key, from either surface. + ``upload_url`` marks a key whose value is a file id set by uploading: the + tenant settings page POSTs the file there and DELETEs it to clear, instead + of offering a text box for an opaque id. """ key: str @@ -62,6 +65,7 @@ class SettingDefinition: value_type: SettingValueType = SettingValueType.STRING tenant_overridable: bool = False check: TenantValueCheck | None = field(default=None, compare=False) + upload_url: str = "" @dataclass(slots=True) diff --git a/modules/settings/settings/tenant_view.py b/modules/settings/settings/tenant_view.py index 67098ea1..6f2bdeaa 100644 --- a/modules/settings/settings/tenant_view.py +++ b/modules/settings/settings/tenant_view.py @@ -28,6 +28,8 @@ class TenantSettingView(SQLModel): value: str | None """The tenant's own override, ``None`` while it inherits.""" effective: str + upload_url: str = "" + """Set for a file-id key: upload here (POST) / clear here (DELETE).""" def _shown(key: str, value: str | None, value_type: str) -> str | None: @@ -68,6 +70,7 @@ async def list_for_tenant( value=_shown(d.key, value, d.value_type), effective=_shown(d.key, value if value is not None else inherited, d.value_type) or "", + upload_url=d.upload_url, ) ) return views diff --git a/modules/tenants/tenants/components/TenantImageControl.tsx b/modules/tenants/tenants/components/TenantImageControl.tsx new file mode 100644 index 00000000..2a6bc404 --- /dev/null +++ b/modules/tenants/tenants/components/TenantImageControl.tsx @@ -0,0 +1,73 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { Button } from '@simple-module-py/ui/components/ui/button'; +import { Input } from '@simple-module-py/ui/components/ui/input'; +import { useState } from 'react'; +import { toast } from 'sonner'; + +interface Props { + inputId: string; + uploadUrl: string; + overridden: boolean; + onChanged: () => void; +} + +async function detail(response: Response): Promise { + const data = await response.json().catch(() => ({}) as Record); + return typeof data.detail === 'string' ? data.detail : null; +} + +/** + * A file-id setting (a logo, a favicon): the value is set by uploading to the + * key's `upload_url` and cleared by DELETE there — the server owns the id, so + * there is no text box for it. + */ +export function TenantImageControl({ inputId, uploadUrl, overridden, onChanged }: Props) { + const { t } = useT(); + const [busy, setBusy] = useState(false); + + async function send(init: RequestInit, success: string) { + setBusy(true); + try { + const response = await fetch(uploadUrl, { credentials: 'same-origin', ...init }); + if (!response.ok) { + toast.error((await detail(response)) ?? t(keys.tenants.settings.toast_failed)); + return; + } + toast.success(success); + onChanged(); + } catch { + toast.error(t(keys.tenants.settings.toast_failed)); + } finally { + setBusy(false); + } + } + + function upload(file: File | undefined) { + if (!file) return; + const body = new FormData(); + body.append('file', file); + void send({ method: 'POST', body }, t(keys.tenants.settings.toast_uploaded)); + } + + return ( +
+ upload(event.target.files?.[0])} + /> + {overridden && ( + + )} +
+ ); +} diff --git a/modules/tenants/tenants/components/TenantSettingRow.tsx b/modules/tenants/tenants/components/TenantSettingRow.tsx index 31b251f8..65a8d9f8 100644 --- a/modules/tenants/tenants/components/TenantSettingRow.tsx +++ b/modules/tenants/tenants/components/TenantSettingRow.tsx @@ -5,6 +5,7 @@ import { Input } from '@simple-module-py/ui/components/ui/input'; import { Label } from '@simple-module-py/ui/components/ui/label'; import { useState } from 'react'; import { toast } from 'sonner'; +import { TenantImageControl } from './TenantImageControl'; export interface TenantSetting { key: string; @@ -13,6 +14,8 @@ export interface TenantSetting { inherited: string; value: string | null; effective: string; + /** Set for a file-id key (a logo): upload/clear there instead of typing an id. */ + upload_url: string; } interface Props { @@ -72,30 +75,41 @@ export function TenantSettingRow({ setting, onChanged }: Props) { {setting.description && (

{setting.description}

)} -
- setDraft(event.target.value)} - disabled={busy} + {setting.upload_url ? ( + -
- - {overridden && ( - - )} + {overridden && ( + + )} +
- -

- {t(keys.tenants.settings.inherited_value, { - value: setting.inherited || t(keys.tenants.settings.none), - })} -

+ )} + {!setting.upload_url && ( +

+ {t(keys.tenants.settings.inherited_value, { + value: setting.inherited || t(keys.tenants.settings.none), + })} +

+ )} ); } diff --git a/modules/tenants/tenants/locales/en.json b/modules/tenants/tenants/locales/en.json index 158bbb04..421106e2 100644 --- a/modules/tenants/tenants/locales/en.json +++ b/modules/tenants/tenants/locales/en.json @@ -151,6 +151,7 @@ "reset": "Reset to platform value", "toast_saved": "Setting saved", "toast_reset": "Setting reset to the platform value", - "toast_failed": "Could not save the setting" + "toast_failed": "Could not save the setting", + "toast_uploaded": "Image uploaded" } } diff --git a/modules/tenants/tests-js/TenantSettingRow.test.tsx b/modules/tenants/tests-js/TenantSettingRow.test.tsx index d852051b..02c7cfdb 100644 --- a/modules/tenants/tests-js/TenantSettingRow.test.tsx +++ b/modules/tenants/tests-js/TenantSettingRow.test.tsx @@ -22,6 +22,7 @@ configureI18n({ 'tenants.settings.toast_saved': 'Setting saved', 'tenants.settings.toast_reset': 'Setting reset', 'tenants.settings.toast_failed': 'Could not save', + 'tenants.settings.toast_uploaded': 'Image uploaded', }, }); @@ -32,6 +33,7 @@ const base: TenantSetting = { inherited: 'hi', value: null, effective: 'hi', + upload_url: '', }; afterEach(() => vi.restoreAllMocks()); @@ -63,4 +65,25 @@ describe('TenantSettingRow', () => { expect(screen.getByRole('button', { name: 'Reset to platform value' })).toBeInTheDocument(); expect(screen.getByText('Overridden')).toBeInTheDocument(); }); + + test('a file-id key offers an upload, not a text box for the id', async () => { + const fetchMock = vi + .spyOn(globalThis, 'fetch') + .mockResolvedValue(new Response('{}', { status: 200 })); + const setting = { + ...base, + key: 'branding.logo_file_id', + upload_url: '/api/branding/tenant/logo', + }; + render( {}} />); + + expect(screen.queryByRole('button', { name: 'Save' })).toBeNull(); + const file = new File(['png'], 'logo.png', { type: 'image/png' }); + await userEvent.upload(screen.getByLabelText('branding.logo_file_id'), file); + + expect(fetchMock).toHaveBeenCalledWith( + '/api/branding/tenant/logo', + expect.objectContaining({ method: 'POST' }), + ); + }); }); diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index a3d66c13..79b0a089 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -831,6 +831,7 @@ export default { 'tenants.settings.toast_failed': '', 'tenants.settings.toast_reset': '', 'tenants.settings.toast_saved': '', + 'tenants.settings.toast_uploaded': '', 'tenants.status.active': '', 'tenants.status.suspended': '', 'ui.admin.back_to_app': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index 3e466775..b1fb3e68 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -1052,6 +1052,7 @@ export const keys = { toast_failed: 'tenants.settings.toast_failed', toast_reset: 'tenants.settings.toast_reset', toast_saved: 'tenants.settings.toast_saved', + toast_uploaded: 'tenants.settings.toast_uploaded', }, status: { active: 'tenants.status.active', From a5438afbe7a0ce8af136d38c347ecf9f407f5f80 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 15:42:23 +0200 Subject: [PATCH 2/6] fix(branding): safer tenant images, reaping, caching and resolution (review of #373) (a)+(b) Image keys are refused (422) on every generic settings route; images are set/cleared only via /api/branding/tenant/{asset}, which validates the bytes. The asset route also refuses to serve a file outside the image allow-list (hand-edited rows). (b) Replaced images (tenant and system) are reaped after commit via register_on_commit (branding.reaper), in their own session, and only when no other image field of the same owner references them - a rollback no longer strands the setting on deleted bytes. (c) Asset Cache-Control: public+immutable only for a tenant-less request whose ?v= names the served file; tenant requests are private (Vary on Cookie and the tenant header); stale/missing ?v= is private, no-cache. (d) The shared-props provider skips /api/, /static/ and non-HTML requests; per-tenant single-flight for concurrent cache misses, and a forget() detaches in-flight reads. (e) Definitions send description_key (branding.tenant_settings.*), which TenantSettingRow translates with the English description as fallback. Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- docs/modules/branding.md | 55 ++++++--- modules/branding/branding/constants.py | 4 + modules/branding/branding/deps.py | 4 +- modules/branding/branding/endpoints/assets.py | 54 +++++++-- .../branding/branding/endpoints/tenant_api.py | 24 ++-- modules/branding/branding/locales/en.json | 9 ++ modules/branding/branding/reaper.py | 83 ++++++++++++++ modules/branding/branding/service.py | 46 +++----- modules/branding/branding/shared_props.py | 24 +++- modules/branding/branding/tenant_branding.py | 30 ++++- .../branding/branding/tenant_definitions.py | 36 ++---- modules/branding/tests/test_branding.py | 7 +- modules/branding/tests/test_branding_reap.py | 100 +++++++++++++++++ modules/branding/tests/test_public_assets.py | 5 +- .../branding/tests/test_tenant_branding.py | 8 ++ .../tests/test_tenant_branding_assets.py | 101 ++++++++++++++--- .../tests/test_tenant_branding_cost.py | 104 ++++++++++++++++++ .../settings/settings/contracts/registry.py | 5 +- modules/settings/settings/tenant_view.py | 3 + .../tenants/components/TenantSettingRow.tsx | 12 +- .../tests-js/TenantSettingRow.test.tsx | 19 ++++ packages/i18n/src/generated-resources.ts | 7 ++ packages/i18n/src/keys.generated.ts | 9 ++ 23 files changed, 625 insertions(+), 124 deletions(-) create mode 100644 modules/branding/branding/reaper.py create mode 100644 modules/branding/tests/test_branding_reap.py create mode 100644 modules/branding/tests/test_tenant_branding_cost.py diff --git a/docs/modules/branding.md b/docs/modules/branding.md index 6a65d3b5..4c6d3a56 100644 --- a/docs/modules/branding.md +++ b/docs/modules/branding.md @@ -89,14 +89,25 @@ Current branding reaches the page through the shared `branding` prop. The endpoi The published URL carries `?v=`. Replacing an image stores a **new** `file_storage` file, so the id doubles as a content address — the URL changes and caches invalidate for free. +The same URL answers per tenant (session, tenant header or subdomain), and a +shared cache keys on the URL alone — so only a URL that pins the bytes by +itself may be shared: + | Request | `Cache-Control` | |---|---| -| `?v=` naming the file served | `public, max-age=31536000, immutable` (one year) | -| No `?v=`, or one naming another file | `public, max-age=3600` (one hour) | +| No tenant on the request, `?v=` naming the file served | `public, max-age=31536000, immutable` (one year) | +| A tenant on the request, `?v=` naming the file served | `private, max-age=31536000, immutable`, `Vary: Cookie` (+ the tenant header when one is configured) | +| No `?v=`, or one naming another file | `private, no-cache` | -A **tenant's** image is sent `private` instead of `public`: the same URL serves a different image to another tenant on the same host, so no shared cache may store it. +A tenant request is `private` even for the platform's image, so a shared cache +never answers the tenant-less URL with it (or the reverse). An unversioned URL +— or a `?v=` left over from another tenant's page, or from before a replace — +can serve other bytes later, so it is never pinned: `no-cache` revalidates on +the next page, which may be in another organisation. A `404` is never cached, +so the next request retries once the setting is fixed. -An unversioned URL can serve new bytes later, so it must never be immutable; the short TTL lets it self-correct. Since the URL answers per tenant, a `?v=` left over from another tenant's page is treated the same way. A `404` is never cached, so the next request retries once the setting is fixed. +The route only ever serves a file whose type is on the image allow-list: a +setting pointed at anything else (a hand-edited row) answers `404`. ## Public contracts @@ -149,7 +160,11 @@ Note the exact property: the bytes must look like **some** allowed image format, ### Asset lifecycle -Replacing or clearing an image deletes the file it stopped referencing, so repeated logo tweaks don't leave orphans in `file_storage`. Cleanup is **best effort**: the setting change has already been persisted, so a storage fault is logged rather than failing an otherwise-successful rebrand. +Replacing or clearing an image deletes the file it stopped referencing, so repeated logo tweaks don't leave orphans in `file_storage` — system and tenant images alike (`branding.reaper`): + +- **After commit.** The delete is queued with `register_on_commit` and runs, in a session of its own, once the settings write is durable. Deleting in the request removed the bytes before that: a late rollback left the setting pointing at a file that was gone. +- **Only when unreferenced.** A file another image field of the same owner still holds — the logo and the dark logo sharing one upload — is kept; the check runs again at reap time. +- **Best effort.** The rebrand already succeeded, so a storage fault is logged and leaves an orphan, never a broken setting. ## Presets @@ -226,11 +241,22 @@ through `/api/settings/tenant/current/{key}`, images through the banner is how the platform announces maintenance, and a tenant must not be able to silence it. -Every tenant-scope write is checked first — by the same validators as the system -value (`422`), and for an image id, that the file is a live upload **owned by -the tenant being written** (`404` otherwise; a platform file or another tenant's -upload is refused). Platform operators writing a tenant's keys through the -settings platform routes go through the same check. +Every tenant-scope write is checked first, by the same validators as the system +value (`422`). The three **image keys are refused on every generic settings +route** (`422` — the self-service route, the platform routes and the admin +forms alike): an image is set and cleared only through +`/api/branding/tenant/{asset}`, which validates the bytes as an image, stores +them as the tenant's own file and reaps the one it replaced. A generic write +could do none of that — it could point the logo at any file the tenant owns +(a PDF) and would never reap what it displaced. (A generic `DELETE` of an +image key is not checked; it drops the override and leaves the file behind for +a janitor rather than reaping it.) + +The organisation settings page shows each key's `description`; definitions +also send a `description_key` (`branding.tenant_settings.`) that the +page translates, with the English text as fallback. Server-side error details +(validator messages) are still shown as sent — they come from pydantic and are +not keyed. Reads resolve per request (`branding.tenant_branding.resolve`): the request's tenant overrides on top of the system theme, falling back field by field. The @@ -238,9 +264,12 @@ provider leaves the merged object on `request.state.branding`, which the root template's pre-hydration `` prefers. A hand-edited invalid override degrades to the system value for that field. -**Cost.** With `multi_tenant` off, or no tenant on the request, the system -object is used as is — no lookup. A tenant's overrides are read at most once per -30 s per process (a TTL cache keyed by tenant id); the cache subscribes to +**Cost.** The shared-props provider only resolves for requests that can render +a page — nothing under `/api/` or `/static/`, and only Inertia visits or +requests accepting HTML. With `multi_tenant` off, or no tenant on the request, +the system object is used as is — no lookup. A tenant's overrides are read at +most once per 30 s per process (a TTL cache keyed by tenant id, with concurrent +misses for one tenant sharing a single read); the cache subscribes to settings' `settings.values` invalidation channel and forgets a tenant when one of its `branding.*` keys changes — every tenant when a system one does — so edits show at once, in every worker once a transport is installed. diff --git a/modules/branding/branding/constants.py b/modules/branding/branding/constants.py index 44137493..748444e2 100644 --- a/modules/branding/branding/constants.py +++ b/modules/branding/branding/constants.py @@ -222,5 +222,9 @@ def clean_footer_href(value: str) -> str: "logo-dark": "logo_dark_file_id", "favicon": "favicon_file_id", } +#: Detail of the 422 a generic settings write of a tenant image key gets. +IMAGE_KEY_GENERIC_WRITE_ERROR: Final = ( + "Branding images are set through /api/branding/tenant/{asset}, not the settings routes." +) #: Floor for the per-tenant cache; settings' invalidation notices clear it sooner. TENANT_CACHE_TTL_SECONDS: Final = 30 diff --git a/modules/branding/branding/deps.py b/modules/branding/branding/deps.py index 98df5730..e0128908 100644 --- a/modules/branding/branding/deps.py +++ b/modules/branding/branding/deps.py @@ -19,8 +19,8 @@ async def get_branding_service( storage: FileStorageService = Depends(get_file_storage_service), ) -> BrandingService: # FastAPI caches dependencies per request, so ``storage`` shares this very - # session: reaping a replaced image commits or rolls back with the settings - # write rather than in a transaction of its own. + # session; a replaced image is reaped only after it commits (see + # ``branding.reaper``), so a rollback never strands the setting. return BrandingService(request.app, db, storage) diff --git a/modules/branding/branding/endpoints/assets.py b/modules/branding/branding/endpoints/assets.py index 92df599b..1f1f1606 100644 --- a/modules/branding/branding/endpoints/assets.py +++ b/modules/branding/branding/endpoints/assets.py @@ -36,6 +36,7 @@ from simple_module_db import tenant_context from branding import constants +from branding.images import ALLOWED_IMAGE_TYPES, normalize_content_type from branding.tenant_branding import ResolvedBranding, resolve router = APIRouter() @@ -43,19 +44,43 @@ logger = logging.getLogger(__name__) -def _cache_control(request: Request, file_id: uuid.UUID, tenant_id: str | None) -> str: - """Long-lived + immutable only when ``?v=`` names the file actually served. +def _cache_headers(request: Request, file_id: uuid.UUID, tenant_id: str | None) -> dict[str, str]: + """Shared caching only where the URL alone determines the bytes. - The same URL answers differently per tenant, so a ``?v=`` left over from - another tenant's (or the system's) page must not pin these bytes for a - year. A tenant's image is ``private``: a shared cache keyed on the URL must - not hand it to a visitor of another tenant on the same host. + ``tenant_id`` is the *request's* tenant. The same URL answers per tenant + (session, tenant header or subdomain), and a shared cache keys on the URL + alone — so only a request with **no** tenant, whose ``?v=`` names the very + file served (a content address), may be stored ``public`` and pinned for a + year. Everything else is ``private``: a request with a tenant is pinned in + the browser only when ``?v=`` matches, varying on what selects the tenant + (the session cookie, the tenant header); an unversioned or stale ``?v=`` is + ``no-cache``, so the next page — maybe in another organisation — revalidates. """ version = request.query_params.get(constants.ASSET_VERSION_QUERY_KEY) - visibility = "private" if tenant_id else "public" - if version and version == str(file_id): - return f"{visibility}, max-age={constants.ASSET_MAX_AGE_VERSIONED}, immutable" - return f"{visibility}, max-age={constants.ASSET_MAX_AGE_UNVERSIONED}" + matches = bool(version) and version == str(file_id) + if tenant_id is None: + if matches: + return { + "Cache-Control": f"public, max-age={constants.ASSET_MAX_AGE_VERSIONED}, immutable" + } + return {"Cache-Control": "private, no-cache"} + if matches: + headers = { + "Cache-Control": f"private, max-age={constants.ASSET_MAX_AGE_VERSIONED}, immutable" + } + else: + headers = {"Cache-Control": "private, no-cache"} + # What selects the tenant: the session cookie and the tenant header (a + # subdomain is part of the cache key already). The session middleware adds + # ``Cookie`` itself when this request read the session, so it is named + # here only otherwise — never twice. + session = request.scope.get("session") + vary = [] if getattr(session, "accessed", False) else ["Cookie"] + if header := getattr(request.app.state.sm.settings, "tenant_header", "") or "": + vary.append(header) + if vary: + headers["Vary"] = ", ".join(vary) + return headers def _configured_file_id(resolved: ResolvedBranding, field: str) -> uuid.UUID: @@ -78,6 +103,7 @@ async def _serve( raise HTTPException(status_code=404, detail="No branding image is set.") resolved = await resolve(request) file_id = _configured_file_id(resolved, field) + request_tenant = resolved.tenant_id owner = resolved.owner_of(field) try: if owner is None: @@ -91,6 +117,12 @@ async def _serve( logger.warning("Branding %s references missing file %s.", field, file_id) raise HTTPException(status_code=404, detail="Branding image is unavailable.") from exc + if normalize_content_type(download.file.content_type) not in ALLOWED_IMAGE_TYPES: + # Uploads are validated, so this is a hand-edited (or pre-validation) + # setting naming some other file: never serve it as branding. + logger.warning("Branding %s references non-image file %s.", field, file_id) + raise HTTPException(status_code=404, detail="Branding image is unavailable.") + if isinstance(download, RedirectDownload): # Deliberately uncached: the target is a presigned URL that expires, so # caching the redirect would hand out a dead link after the TTL. @@ -102,7 +134,7 @@ async def _serve( download.body, media_type=row.content_type, headers={ - "Cache-Control": _cache_control(request, file_id, owner), + **_cache_headers(request, file_id, request_tenant), "Content-Length": str(row.size_bytes), "ETag": f'"{row.checksum_sha256}"', # `attachment` is ignored for subresource loads (, `` override at it; ``DELETE`` removes the override, so the tenant falls back to the system image. The replaced file is -reaped in the tenant's own scope. +reaped in the tenant's own scope once the write commits, unless another of the +tenant's image fields still references it (:mod:`branding.reaper`). Guarded like every other tenant-settings write: ``settings.tenant.edit`` (tenant owner/admin) and the tenant comes from ``request.state.tenant_id`` @@ -14,9 +15,6 @@ from __future__ import annotations -import logging -import uuid - from fastapi import APIRouter, Depends, HTTPException, Request, UploadFile from file_storage.deps import get_file_storage_service from file_storage.service import FileStorageService @@ -27,14 +25,14 @@ from settings.tenant_scope import active_tenant from simple_module_hosting.permissions import RequiresPermission -from branding.constants import PACKAGE, PATH_TENANT_ASSET, TENANT_ASSETS +from branding.constants import PACKAGE, PATH_TENANT_ASSET, TENANT_ASSETS, TENANT_IMAGE_FIELDS from branding.contracts.schemas import BrandingOut from branding.images import validate_image +from branding.reaper import schedule_reap from branding.service import to_out from branding.tenant_branding import merge, overrides_from router = APIRouter(dependencies=[Depends(RequiresPermission(PERM_TENANT_EDIT))]) -logger = logging.getLogger(__name__) def _field(asset: str) -> str: @@ -61,18 +59,14 @@ async def _swap( await settings.upsert_scoped( SettingScope.TENANT, tenant_id, key, SettingUpsert(value=file_id) ) - if previous is not None and previous.value and previous.value != file_id: - try: - # Bound to the tenant already (it is the request's), so this can - # only ever delete the tenant's own file. - await storage.delete(uuid.UUID(previous.value)) - except Exception: - logger.warning( - "Could not delete replaced tenant branding image %s.", previous.value, exc_info=True - ) # Caches drop this tenant when settings' after-commit notice fires; the # reply reads through this request's session, which sees the write. overrides = await overrides_from(settings, tenant_id) + old = previous.value if previous is not None else "" + if old and old != file_id and old not in {overrides.get(f) for f in TENANT_IMAGE_FIELDS}: + # After commit, and only if still unreferenced: a rollback must not + # leave the restored setting pointing at deleted bytes. + schedule_reap(request.app, storage.db, old, tenant_id=tenant_id) return to_out(merge(request.app.state.branding.settings, tenant_id, overrides).settings) diff --git a/modules/branding/branding/locales/en.json b/modules/branding/branding/locales/en.json index 91e978a8..e7fb49b5 100644 --- a/modules/branding/branding/locales/en.json +++ b/modules/branding/branding/locales/en.json @@ -53,5 +53,14 @@ }, "nav": { "branding": "Branding" + }, + "tenant_settings": { + "app_name": "Application name shown in the header, page titles and emails.", + "primary_color": "Accent colour as #rrggbb; empty uses the theme default.", + "design_pack": "Design pack slug; empty uses the base look.", + "footer_text": "Footer caption; empty shows the platform's.", + "logo_file_id": "Logo image.", + "logo_dark_file_id": "Logo for dark surfaces; falls back to the logo.", + "favicon_file_id": "Browser tab icon." } } diff --git a/modules/branding/branding/reaper.py b/modules/branding/branding/reaper.py new file mode 100644 index 00000000..b393fb6a --- /dev/null +++ b/modules/branding/branding/reaper.py @@ -0,0 +1,83 @@ +"""Reap a branding image once nothing references it — after the write commits. + +Every upload mints a new ``file_storage`` id, so the file a setting stops +pointing at would otherwise sit in the store forever. Deleting it in the +request, though, deletes the *bytes* immediately while the settings write is +still uncommitted: a late rollback would leave the setting pointing at a file +that is gone. So the reap is queued with ``register_on_commit`` and runs only +once the new value is durable, in a session of its own. + +A file is reaped only when no image field of the same owner still references +it — the tenant's own overrides for a tenant file, the SYSTEM values for a +platform file — re-checked at reap time, so a value copied to another field +(logo and dark logo sharing one upload) keeps the file alive. + +Best effort throughout: the rebrand already succeeded, so a failure is logged +and leaves an orphan for a janitor, never a broken setting. +""" + +from __future__ import annotations + +import logging +import uuid +from contextlib import nullcontext +from typing import TYPE_CHECKING + +from simple_module_db import tenant_context +from simple_module_db.callbacks import register_on_commit + +from branding.constants import PACKAGE, TENANT_IMAGE_FIELDS + +if TYPE_CHECKING: + from fastapi import FastAPI + from sqlalchemy.ext.asyncio import AsyncSession + +logger = logging.getLogger(__name__) + +_IMAGE_KEYS = frozenset(f"{PACKAGE}.{name}" for name in TENANT_IMAGE_FIELDS) + + +async def referenced_ids(db: AsyncSession, tenant_id: str | None) -> set[str]: + """File ids the owner's image fields hold (``None`` = the system's).""" + from settings.contracts.schemas import SettingScope + from settings.service import SettingService + + scope, scope_id = (SettingScope.TENANT, tenant_id) if tenant_id else (SettingScope.SYSTEM, "") + rows = await SettingService(db).list_by_scope_unmasked(scope, scope_id) + return {row.value for row in rows if row.key in _IMAGE_KEYS and row.value} + + +def schedule_reap(app: FastAPI, db: AsyncSession, file_id: str, *, tenant_id: str | None) -> None: + """Reap ``file_id`` after ``db`` commits, if it is still unreferenced then. + + ``tenant_id`` is the file's owner; ``None`` means a platform file. + """ + if not file_id: + return + register_on_commit(db, lambda: reap(app, file_id, tenant_id=tenant_id)) + + +async def reap(app: FastAPI, file_id: str, *, tenant_id: str | None) -> None: + from file_storage.service import FileStorageService + + try: + parsed = uuid.UUID(file_id) + except ValueError: + logger.warning("Branding image %r is not a file id; nothing to reap.", file_id) + return + try: + with tenant_context(tenant_id) if tenant_id else nullcontext(): + async with app.state.sm.db.session_factory() as db: + if file_id in await referenced_ids(db, tenant_id): + return + services = app.state.file_storage + storage = FileStorageService(db, services.backend, services.settings) + await storage.delete(parsed, platform=tenant_id is None) + await db.commit() + except Exception: + # Deliberately broad: a cleanup failure is never a reason to fail (or, + # running after commit, to misreport) a rebrand that succeeded. + logger.warning("Could not delete replaced branding image %s.", file_id, exc_info=True) + + +__all__ = ["reap", "referenced_ids", "schedule_reap"] diff --git a/modules/branding/branding/service.py b/modules/branding/branding/service.py index 77d3a501..4d01b766 100644 --- a/modules/branding/branding/service.py +++ b/modules/branding/branding/service.py @@ -8,12 +8,11 @@ from __future__ import annotations -import logging -import uuid from typing import TYPE_CHECKING, Any -from branding.constants import FAVICON_URL, LOGO_DARK_URL, LOGO_URL, PACKAGE +from branding.constants import FAVICON_URL, LOGO_DARK_URL, LOGO_URL, PACKAGE, TENANT_IMAGE_FIELDS from branding.contracts.schemas import BrandingOut +from branding.reaper import schedule_reap from branding.shared_props import asset_url if TYPE_CHECKING: @@ -23,8 +22,6 @@ from branding.settings import BrandingSettings -logger = logging.getLogger(__name__) - def to_out(settings: BrandingSettings) -> BrandingOut: """The API view of one branding settings object (system or a tenant's).""" @@ -80,39 +77,22 @@ async def _swap_asset(self, field: str, file_id: str) -> BrandingOut: """Point *field* at *file_id* ("" to clear) and reap what it replaced. Every upload mints a new ``file_storage`` id, so the file we stop - referencing here would otherwise sit in the store forever with nothing - left to reference or reap it. + referencing here would otherwise sit in the store forever. The reap is + queued for after the commit (:mod:`branding.reaper`): deleting the + bytes in the request would leave a rolled-back setting pointing at a + file that is gone. A file another system image field still references + (logo and dark logo sharing one upload) is kept. """ previous = getattr(self.app.state.branding.settings, field, "") out = await self.apply({field: file_id}) - if previous and previous != file_id: - await self._reap(previous) + live = self.app.state.branding.settings + still_used = {getattr(live, name, "") for name in TENANT_IMAGE_FIELDS} + # No storage handed in (a hand-built service): nothing is reaped. + replaced = previous and previous != file_id and previous not in still_used + if replaced and self.storage is not None: + schedule_reap(self.app, self.db, previous, tenant_id=None) return out - async def _reap(self, file_id: str) -> None: - """Delete a no-longer-referenced branding image, best effort. - - The setting change has already been persisted and is what the admin - asked for, so a storage fault (or a hand-edited, non-UUID setting) must - be logged rather than turned into a 500 on a successful rebrand. - - The realistic failures are fully contained: a missing row raises before - any write, and a failed backend delete is already suppressed inside - ``file_storage`` (the row stays flagged for a janitor). Swallowing here - cannot rescue a *flush* failure, though — that leaves the shared request - session dirty and the commit at request end would surface it anyway. - Reaping in the same session is the deliberate trade: it keeps the delete - atomic with the settings write instead of orphaning on a late rollback. - """ - if self.storage is None: - return - try: - 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. - logger.warning("Could not delete replaced branding image %s.", file_id, exc_info=True) - async def set_logo(self, file_id: str) -> BrandingOut: return await self._swap_asset("logo_file_id", file_id) diff --git a/modules/branding/branding/shared_props.py b/modules/branding/branding/shared_props.py index 78277f94..5f7a012c 100644 --- a/modules/branding/branding/shared_props.py +++ b/modules/branding/branding/shared_props.py @@ -62,6 +62,28 @@ def branding_payload(settings: BrandingSettings) -> dict: } +_NO_PAGE_PREFIXES = ("/api/", "/static/") + + +def renders_a_page(request: Request) -> bool: + """Whether ``request`` can render an Inertia page (or the HTML shell). + + Providers run on every request, API and static ones included; resolving a + tenant's branding there is a cache lookup — or a DB read on a miss — for a + prop nobody reads. The same path rule the error handlers use: nothing under + ``/api/`` or ``/static/`` is a page (Inertia views never live under + ``/api/``, SM018). Elsewhere an Inertia visit, or anything a browser could + be navigating with (``text/html``, ``*/*``, or no ``Accept``), counts. + """ + if request.headers.get("x-inertia"): + return True + path = request.url.path + if path == "/api" or path.startswith(_NO_PAGE_PREFIXES): + return False + accept = request.headers.get("accept", "") + return not accept or "text/html" in accept or "*/*" in accept + + async def branding_shared_props(request: Request) -> dict: """Provider: emit ``{"branding": {...}}`` for the request's tenant (#373). @@ -77,7 +99,7 @@ async def branding_shared_props(request: Request) -> dict: from branding.services import BrandingServices from branding.tenant_branding import resolve - if getattr(request.app.state, "branding", None) is None: + if getattr(request.app.state, "branding", None) is None or not renders_a_page(request): return {} resolved = await resolve(request) if resolved.tenant_fields: diff --git a/modules/branding/branding/tenant_branding.py b/modules/branding/branding/tenant_branding.py index 44794f24..48760246 100644 --- a/modules/branding/branding/tenant_branding.py +++ b/modules/branding/branding/tenant_branding.py @@ -21,6 +21,7 @@ from __future__ import annotations +import asyncio import logging from dataclasses import dataclass, field from typing import TYPE_CHECKING, Any @@ -48,6 +49,9 @@ # Bumped by every forget, so a read that started before an invalidation does # not store its (possibly stale) result after it. _epoch = 0 +# tenant id -> (epoch the read started in, the read). Single-flight: concurrent +# misses for one tenant share one read instead of each opening a session. +_INFLIGHT: dict[str, tuple[int, asyncio.Future[dict[str, str]]]] = {} @dataclass(frozen=True, slots=True) @@ -67,10 +71,14 @@ def forget(tenant_id: str | None = None) -> None: """Drop one tenant's entry, or every entry when ``tenant_id`` is ``None``.""" global _epoch _epoch += 1 + # A read already in flight may predate the change: later misses must start + # a fresh one rather than join it (its own waiters still get their answer). if tenant_id is None: _CACHE.clear() + _INFLIGHT.clear() else: _CACHE.pop(tenant_id, None) + _INFLIGHT.pop(tenant_id, None) def _on_settings_changed(inv: Invalidation) -> None: @@ -148,6 +156,23 @@ def merge(system: BrandingSettings, tenant_id: str, overrides: dict[str, str]) - return ResolvedBranding(system, tenant_id, frozenset()) +def _shared_read(app: FastAPI, tenant_id: str) -> tuple[int, asyncio.Future[dict[str, str]]]: + """The in-flight override read for ``tenant_id``, started if there is none.""" + entry = _INFLIGHT.get(tenant_id) + if entry is not None: + return entry + read = asyncio.ensure_future(read_overrides(app, tenant_id)) + entry = (_epoch, read) + _INFLIGHT[tenant_id] = entry + + def _done(_: asyncio.Future[dict[str, str]]) -> None: + if _INFLIGHT.get(tenant_id) is entry: + del _INFLIGHT[tenant_id] + + read.add_done_callback(_done) + return entry + + async def resolve_for(app: FastAPI, tenant_id: str | None) -> ResolvedBranding: system: BrandingSettings = app.state.branding.settings if not tenant_id: @@ -158,8 +183,9 @@ async def resolve_for(app: FastAPI, tenant_id: str | None) -> ResolvedBranding: if merged_against is system: return resolved else: - started = _epoch - overrides = await read_overrides(app, tenant_id) + started, read = _shared_read(app, tenant_id) + # Shielded: one waiter being cancelled must not cancel everyone's read. + overrides = await asyncio.shield(read) if _epoch != started: return merge(system, tenant_id, overrides) # don't cache a racing read resolved = merge(system, tenant_id, overrides) diff --git a/modules/branding/branding/tenant_definitions.py b/modules/branding/branding/tenant_definitions.py index cdef3847..5dd9a386 100644 --- a/modules/branding/branding/tenant_definitions.py +++ b/modules/branding/branding/tenant_definitions.py @@ -8,21 +8,22 @@ * a scalar must pass the same validator the system value does (422); * a design pack must be one an installed module provides (422); -* an image id must name a live file **owned by the tenant being written** - (404 otherwise) — a tenant cannot point its logo at another tenant's upload, - nor at a platform file. +* an image key is refused on every generic settings route (422): images are + set and cleared only through ``/api/branding/tenant/{asset}``, which + validates the bytes as an image, stores them as the tenant's own file and + reaps the file it replaces once the write commits. A generic write could do + none of that — it could point the logo at any file the tenant owns (a PDF), + and the file it displaced would never be reaped. """ from __future__ import annotations -import uuid from typing import TYPE_CHECKING from pydantic import ValidationError -from simple_module_db import all_tenants -from sqlalchemy import select from branding.constants import ( + IMAGE_KEY_GENERIC_WRITE_ERROR, PACKAGE, ROUTE_PREFIX, TENANT_ASSETS, @@ -47,28 +48,10 @@ _ASSET_OF = {field: asset for asset, field in TENANT_ASSETS.items()} -async def tenant_owns_file(app: FastAPI, tenant_id: str, raw: str) -> bool: - """Whether ``raw`` is the id of a live (not deleted) file owned by ``tenant_id``.""" - from file_storage.models import StoredFile - - try: - file_id = uuid.UUID(raw) - except ValueError: - return False - stmt = select(StoredFile.id).where(StoredFile.id == file_id, StoredFile.tenant_id == tenant_id) - # An explicit owner condition, so the answer does not depend on which - # tenant (if any) the caller happens to be bound to. - with all_tenants(): - async with app.state.sm.db.session_factory() as db: - return (await db.execute(stmt)).first() is not None - - def _make_check(name: str): async def check(request: Request, tenant_id: str, value: str) -> None: if name in TENANT_IMAGE_FIELDS: - if value and not await tenant_owns_file(request.app, tenant_id, value): - raise LookupError("No such image in this organisation.") - return + raise ValueError(IMAGE_KEY_GENERIC_WRITE_ERROR.format(asset=_ASSET_OF[name])) try: BrandingSettings(**{name: value}) except ValidationError as exc: @@ -93,6 +76,7 @@ def register_tenant_definitions(app: FastAPI) -> None: key=f"{PACKAGE}.{name}", default=str(defaults[name]), description=_DESCRIPTIONS[name], + description_key=f"{PACKAGE}.tenant_settings.{name}", tenant_overridable=True, check=_make_check(name), upload_url=f"{ROUTE_PREFIX}/tenant/{asset}" if asset else "", @@ -100,4 +84,4 @@ def register_tenant_definitions(app: FastAPI) -> None: ) -__all__ = ["register_tenant_definitions", "tenant_owns_file"] +__all__ = ["register_tenant_definitions"] diff --git a/modules/branding/tests/test_branding.py b/modules/branding/tests/test_branding.py index 63ce858d..7e54e607 100644 --- a/modules/branding/tests/test_branding.py +++ b/modules/branding/tests/test_branding.py @@ -94,7 +94,12 @@ async def test_provider_returns_empty_when_state_absent() -> None: async def test_provider_emits_branding_block() -> None: state = SimpleNamespace(branding=SimpleNamespace(settings=BrandingSettings(app_name="Acme"))) - request = SimpleNamespace(app=SimpleNamespace(state=state), state=SimpleNamespace()) + request = SimpleNamespace( + app=SimpleNamespace(state=state), + state=SimpleNamespace(), + headers={"x-inertia": "true"}, + url=SimpleNamespace(path="/"), + ) out = await branding_shared_props(request) # type: ignore[arg-type] assert out["branding"]["appName"] == "Acme" diff --git a/modules/branding/tests/test_branding_reap.py b/modules/branding/tests/test_branding_reap.py new file mode 100644 index 00000000..332a41f7 --- /dev/null +++ b/modules/branding/tests/test_branding_reap.py @@ -0,0 +1,100 @@ +"""A replaced branding image is reaped after commit, and only when unreferenced. + +Deleting in the request removed the bytes before the settings write was +durable: a rollback left the setting pointing at a deleted file. And a file +another image field of the same owner still holds must survive. +""" + +from __future__ import annotations + +import pytest +from branding import tenant_branding +from branding.endpoints import tenant_api +from branding.service import BrandingService +from file_storage.models import StoredFile +from settings.contracts.schemas import SettingScope, SettingUpsert +from settings.service import SettingService +from simple_module_db import all_tenants +from sqlalchemy import select + +_PNG = b"\x89PNG\r\n\x1a\n" + b"\x00" * 32 +TENANT_LOGO = "/api/branding/tenant/logo" + + +@pytest.fixture(autouse=True) +def _fresh_cache(): + tenant_branding.forget() + yield + tenant_branding.forget() + + +async def _upload(client, url: str = TENANT_LOGO, body: bytes = _PNG) -> None: + resp = await client.post(url, files={"file": ("logo.png", body, "image/png")}) + assert resp.status_code == 200, resp.text + + +async def _deleted(app) -> dict[str, bool]: + with all_tenants(): + async with app.state.sm.db.session_factory() as db: + stmt = select(StoredFile).execution_options(include_deleted=True) + return {str(r.id): r.is_deleted for r in (await db.execute(stmt)).scalars()} + + +async def _tenant_value(app, tenant_id: str, key: str) -> str | None: + async with app.state.sm.db.session_factory() as db: + row = await SettingService(db).get_scoped(SettingScope.TENANT, tenant_id, key) + return row.value if row is not None else None + + +async def test_a_rolled_back_replace_keeps_the_old_file(app, tenant_client, monkeypatch): + async with tenant_client() as a: + await _upload(a.client) + old = await _tenant_value(app, a.tenant_id, "branding.logo_file_id") + + def boom(*_a, **_k): + raise RuntimeError("late failure after the swap") + + monkeypatch.setattr(tenant_api, "merge", boom) + with pytest.raises(RuntimeError): + await _upload(a.client, body=_PNG + b"new") + + assert await _tenant_value(app, a.tenant_id, "branding.logo_file_id") == old + assert (await _deleted(app))[old] is False + monkeypatch.undo() + assert (await a.client.get("/api/branding/logo")).content == _PNG + + +async def test_a_tenant_file_another_field_still_uses_is_kept(app, tenant_client): + async with tenant_client() as a: + await _upload(a.client) + shared = await _tenant_value(app, a.tenant_id, "branding.logo_file_id") + # A hand-edited (or migrated) row sharing the logo's file. + async with app.state.sm.db.session_factory() as db: + await SettingService(db).upsert_scoped( + SettingScope.TENANT, + a.tenant_id, + "branding.logo_dark_file_id", + SettingUpsert(value=shared), + ) + await db.commit() + + await _upload(a.client, body=_PNG + b"new") + assert (await _deleted(app))[shared] is False + + cleared = await a.client.delete("/api/branding/tenant/logo-dark") + assert cleared.status_code == 200 + assert (await _deleted(app))[shared] is True + + +async def test_a_platform_file_another_system_field_still_uses_is_kept(app, authenticated_client): + await _upload(authenticated_client, "/api/branding/logo") + shared = app.state.branding.settings.logo_file_id + async with app.state.sm.db.session_factory() as db: + await BrandingService(app, db).apply({"logo_dark_file_id": shared}) + await db.commit() + + await _upload(authenticated_client, "/api/branding/logo", _PNG + b"new") + assert (await _deleted(app))[shared] is False + + assert (await authenticated_client.delete("/api/branding/logo-dark")).status_code == 200 + assert (await _deleted(app))[shared] is True diff --git a/modules/branding/tests/test_public_assets.py b/modules/branding/tests/test_public_assets.py index ee84b7c7..da5a8d9e 100644 --- a/modules/branding/tests/test_public_assets.py +++ b/modules/branding/tests/test_public_assets.py @@ -141,8 +141,9 @@ async def test_a_request_without_a_usable_version_is_not_immutable( cache_control = (await client.get(f"/api/branding/logo{query}")).headers["cache-control"] - assert "max-age=3600" in cache_control - assert "immutable" not in cache_control + # Nor shared: the same URL answers per tenant, and only a ``?v=`` naming + # the served file makes the URL a content address a shared cache may keep. + assert cache_control == "private, no-cache" async def test_an_unset_image_is_an_uncached_404(client: httpx.AsyncClient) -> None: diff --git a/modules/branding/tests/test_tenant_branding.py b/modules/branding/tests/test_tenant_branding.py index fc70c0e0..40b18a14 100644 --- a/modules/branding/tests/test_tenant_branding.py +++ b/modules/branding/tests/test_tenant_branding.py @@ -186,3 +186,11 @@ async def no_lookup(*_args, **_kwargs): got = await _branding(authenticated_client, "/admin/branding/") assert got["appName"] == "Single" + + +async def test_tenant_rows_carry_a_description_key_for_the_client(tenant_client): + async with tenant_client() as a: + rows = (await a.client.get(CURRENT)).json() + mine = {r["key"]: r for r in rows if r["key"].startswith("branding.")} + assert mine["branding.app_name"]["description_key"] == "branding.tenant_settings.app_name" + assert mine["branding.app_name"]["description"] # the English fallback diff --git a/modules/branding/tests/test_tenant_branding_assets.py b/modules/branding/tests/test_tenant_branding_assets.py index 7eaafa9b..98fe2496 100644 --- a/modules/branding/tests/test_tenant_branding_assets.py +++ b/modules/branding/tests/test_tenant_branding_assets.py @@ -68,6 +68,7 @@ async def test_the_upload_is_the_tenants_file_and_only_it_sees_it(self, app, ten # Private: the same URL answers differently for another tenant. assert resp.headers["cache-control"].startswith("private") assert "immutable" in resp.headers["cache-control"] + assert "Cookie" in resp.headers["vary"] assert (await b.client.get(LOGO_URL)).status_code == 404 # no system logo @@ -81,7 +82,9 @@ async def test_a_platform_admin_upload_is_still_a_platform_file( # The tenant has no logo of its own: it inherits the platform's. resp = await a.client.get(LOGO_URL) assert resp.content == _PNG + b"p" - assert resp.headers["cache-control"].startswith("public") + # Even the platform's file: the request has a tenant, so a shared + # cache must not serve this answer to the tenant-less URL. + assert resp.headers["cache-control"] == "private, no-cache" await _upload(a.client) assert (await a.client.get(LOGO_URL)).content == _PNG owners = {r.tenant_id for r in await _files(app)} @@ -143,18 +146,23 @@ def anon(host: str) -> httpx.AsyncClient: assert (await visitor.get(LOGO_URL)).status_code == 404 # the system's: none -class TestOwnership: - async def test_a_tenant_cannot_point_its_logo_at_another_tenants_file(self, tenant_client): +class TestGenericRoutesRefuseImageKeys: + """Images go through ``/api/branding/tenant/{asset}`` only: a generic + settings write could point the logo at any file (a PDF, another tenant's + upload, a platform file) and would never reap the file it displaced.""" + + async def test_self_service_writes_are_422_whatever_the_value(self, tenant_client): async with tenant_client() as a, tenant_client() as b: theirs = await _plain_upload(b.client) ours = await _plain_upload(a.client) key = f"{CURRENT}/branding.logo_file_id" - assert (await a.client.put(key, json={"value": theirs})).status_code == 404 - assert (await a.client.put(key, json={"value": "not-a-uuid"})).status_code == 404 - assert (await a.client.put(key, json={"value": ours})).status_code == 200 - assert (await a.client.get(LOGO_URL)).status_code == 200 + for value in (theirs, ours, "not-a-uuid", ""): + resp = await a.client.put(key, json={"value": value}) + assert resp.status_code == 422, value + assert "/api/branding/tenant/logo" in resp.json()["detail"] + assert (await a.client.get(LOGO_URL)).status_code == 404 - async def test_nor_at_a_platform_file(self, app, authenticated_client, tenant_client): + async def test_nor_can_it_name_a_platform_file(self, app, authenticated_client, tenant_client): await authenticated_client.post( "/api/branding/logo", files={"file": ("p.png", _PNG, "image/png")} ) @@ -163,16 +171,18 @@ async def test_nor_at_a_platform_file(self, app, authenticated_client, tenant_cl resp = await a.client.put( f"{CURRENT}/branding.logo_file_id", json={"value": platform_id} ) - assert resp.status_code == 404 + assert resp.status_code == 422 - async def test_a_platform_operator_writing_for_a_tenant_is_checked_too( + async def test_a_platform_operator_writing_for_a_tenant_is_refused_too( self, authenticated_client, tenant_client ): - async with tenant_client() as a, tenant_client() as b: - theirs = await _plain_upload(b.client) - url = f"/api/settings/tenant/{a.tenant_id}/branding.logo_file_id" - assert (await authenticated_client.put(url, json={"value": theirs})).status_code == 404 + async with tenant_client() as a: + ours = await _plain_upload(a.client) + url = f"/api/settings/tenant/{a.tenant_id}/branding.favicon_file_id" + assert (await authenticated_client.put(url, json={"value": ours})).status_code == 422 + +class TestOwnership: async def test_a_hand_edited_row_naming_another_tenants_file_serves_404( self, app, tenant_client ): @@ -188,3 +198,66 @@ async def test_a_hand_edited_row_naming_another_tenants_file_serves_404( await db.commit() assert (await a.client.get(LOGO_URL)).status_code == 404 assert (await b.client.get(f"/api/file-storage/files/{theirs}")).status_code == 200 + + +class TestCacheHeaders: + """Only a tenant-less request whose ``?v=`` names the served file is public.""" + + async def test_a_platform_logo_on_a_tenant_request_is_never_public( + self, app, authenticated_client, tenant_client + ): + await authenticated_client.post( + "/api/branding/logo", files={"file": ("p.png", _PNG, "image/png")} + ) + versioned = f"{LOGO_URL}?v={app.state.branding.settings.logo_file_id}" + async with tenant_client() as a: + resp = await a.client.get(versioned) + assert resp.headers["cache-control"].startswith("private") + assert "immutable" in resp.headers["cache-control"] + assert {"Cookie", "X-Tenant-ID"} <= {v.strip() for v in resp.headers["vary"].split(",")} + + async def test_a_stale_version_on_a_tenant_request_is_no_cache(self, tenant_client): + async with tenant_client() as a: + out = await _upload(a.client) + await _upload(a.client, _PNG + b"2") + resp = await a.client.get(out["logo_url"]) # names the replaced file + assert resp.status_code == 200 + assert resp.headers["cache-control"] == "private, no-cache" + + async def test_the_tenant_header_is_part_of_vary(self, app, tenant_client, monkeypatch): + monkeypatch.setattr(app.state.sm.settings, "tenant_header", "X-Tenant") + async with tenant_client() as a: + out = await _upload(a.client) + resp = await a.client.get(out["logo_url"]) + assert "X-Tenant" in {v.strip() for v in resp.headers["vary"].split(",")} + + async def test_an_anonymous_versioned_platform_logo_is_public( + self, app, authenticated_client, client + ): + await authenticated_client.post( + "/api/branding/logo", files={"file": ("p.png", _PNG, "image/png")} + ) + versioned = f"{LOGO_URL}?v={app.state.branding.settings.logo_file_id}" + resp = await client.get(versioned) + assert resp.headers["cache-control"].startswith("public") + assert "X-Tenant-ID" not in resp.headers.get("vary", "") + + +class TestOnlyImagesAreServed: + async def test_a_setting_naming_a_non_image_file_serves_404(self, app, tenant_client): + async with tenant_client() as a: + resp = await a.client.post( + "/api/file-storage/upload", + files={"file": ("x.pdf", b"%PDF-1.4", "application/pdf")}, + ) + pdf = resp.json()["id"] + async with app.state.sm.db.session_factory() as db: + await SettingService(db).upsert_scoped( + SettingScope.TENANT, + a.tenant_id, + "branding.logo_file_id", + SettingUpsert(value=pdf), + ) + await db.commit() + tenant_branding.forget() + assert (await a.client.get(LOGO_URL)).status_code == 404 diff --git a/modules/branding/tests/test_tenant_branding_cost.py b/modules/branding/tests/test_tenant_branding_cost.py new file mode 100644 index 00000000..a0a1acc9 --- /dev/null +++ b/modules/branding/tests/test_tenant_branding_cost.py @@ -0,0 +1,104 @@ +"""Per-tenant branding resolution only runs where a page renders, once per miss. + +Shared-props providers run on every request. Resolving a tenant's branding for +an API call or a static file is a cache lookup — or a DB read on a miss — for a +prop nobody reads; and N concurrent misses for one tenant must share one read. +""" + +from __future__ import annotations + +import asyncio + +import pytest +from branding import tenant_branding +from branding.shared_props import renders_a_page +from starlette.requests import Request + + +@pytest.fixture(autouse=True) +def _fresh_cache(): + tenant_branding.forget() + yield + tenant_branding.forget() + + +def _request(path: str, **headers: str) -> Request: + raw = [(k.lower().replace("_", "-").encode(), v.encode()) for k, v in headers.items()] + return Request({"type": "http", "method": "GET", "path": path, "headers": raw}) + + +@pytest.mark.parametrize( + ("path", "headers", "expected"), + [ + ("/tenants/", {"X-Inertia": "true", "Accept": "application/json"}, True), + ("/tenants/", {"Accept": "text/html,application/xhtml+xml"}, True), + ("/tenants/", {"Accept": "*/*"}, True), + ("/tenants/", {}, True), + ("/tenants/", {"Accept": "application/json"}, False), + ("/api/tenants/", {"Accept": "*/*"}, False), + ("/api/branding/logo", {"Accept": "image/webp,*/*"}, False), + ("/static/dist/app.js", {"Accept": "*/*"}, False), + ], +) +def test_which_requests_render_a_page(path, headers, expected): + assert renders_a_page(_request(path, **headers)) is expected + + +async def test_api_requests_never_read_tenant_overrides(tenant_client, monkeypatch): + reads: list[str] = [] + original = tenant_branding.read_overrides + + async def counting(app, tenant_id): + reads.append(tenant_id) + return await original(app, tenant_id) + + monkeypatch.setattr(tenant_branding, "read_overrides", counting) + async with tenant_client() as a: + assert (await a.client.get("/api/tenants/")).status_code == 200 + assert reads == [] + page = await a.client.get("/tenants/", headers={"X-Inertia": "true"}) + assert page.json()["props"]["branding"]["appName"] + assert reads == [a.tenant_id] + + +async def test_concurrent_misses_share_one_read(app, monkeypatch): + reads = 0 + release = asyncio.Event() + + async def slow(_app, _tenant_id): + nonlocal reads + reads += 1 + await release.wait() + return {"app_name": "Acme"} + + monkeypatch.setattr(tenant_branding, "read_overrides", slow) + waiters = [asyncio.create_task(tenant_branding.resolve_for(app, "acme")) for _ in range(5)] + await asyncio.sleep(0) + release.set() + results = await asyncio.gather(*waiters) + + assert reads == 1 + assert {r.settings.app_name for r in results} == {"Acme"} + + +async def test_a_miss_after_a_forget_does_not_join_the_older_read(app, monkeypatch): + reads: list[str] = [] + release = asyncio.Event() + + async def slow(_app, _tenant_id): + reads.append("read") + await release.wait() + return {"app_name": f"v{len(reads)}"} + + monkeypatch.setattr(tenant_branding, "read_overrides", slow) + first = asyncio.create_task(tenant_branding.resolve_for(app, "acme")) + await asyncio.sleep(0) + tenant_branding.forget("acme") # the row changed while the read was in flight + second = asyncio.create_task(tenant_branding.resolve_for(app, "acme")) + await asyncio.sleep(0) + release.set() + await asyncio.gather(first, second) + + assert len(reads) == 2 + # Only the post-change read is cached. + assert (await tenant_branding.resolve_for(app, "acme")).settings.app_name == "v2" diff --git a/modules/settings/settings/contracts/registry.py b/modules/settings/settings/contracts/registry.py index fc49dc0c..86988158 100644 --- a/modules/settings/settings/contracts/registry.py +++ b/modules/settings/settings/contracts/registry.py @@ -55,7 +55,9 @@ class SettingDefinition: vets every TENANT-scope write of the key, from either surface. ``upload_url`` marks a key whose value is a file id set by uploading: the tenant settings page POSTs the file there and DELETEs it to clear, instead - of offering a text box for an opaque id. + of offering a text box for an opaque id. ``description_key`` is the i18n + key the tenant settings page translates ``description`` with (the English + ``description`` stays the fallback when the key is missing or unset). """ key: str @@ -66,6 +68,7 @@ class SettingDefinition: tenant_overridable: bool = False check: TenantValueCheck | None = field(default=None, compare=False) upload_url: str = "" + description_key: str = "" @dataclass(slots=True) diff --git a/modules/settings/settings/tenant_view.py b/modules/settings/settings/tenant_view.py index 6f2bdeaa..4e1f1410 100644 --- a/modules/settings/settings/tenant_view.py +++ b/modules/settings/settings/tenant_view.py @@ -30,6 +30,8 @@ class TenantSettingView(SQLModel): effective: str upload_url: str = "" """Set for a file-id key: upload here (POST) / clear here (DELETE).""" + description_key: str = "" + """i18n key for ``description``; the client falls back to ``description``.""" def _shown(key: str, value: str | None, value_type: str) -> str | None: @@ -71,6 +73,7 @@ async def list_for_tenant( effective=_shown(d.key, value if value is not None else inherited, d.value_type) or "", upload_url=d.upload_url, + description_key=d.description_key, ) ) return views diff --git a/modules/tenants/tenants/components/TenantSettingRow.tsx b/modules/tenants/tenants/components/TenantSettingRow.tsx index 65a8d9f8..b24caed8 100644 --- a/modules/tenants/tenants/components/TenantSettingRow.tsx +++ b/modules/tenants/tenants/components/TenantSettingRow.tsx @@ -16,6 +16,9 @@ export interface TenantSetting { effective: string; /** Set for a file-id key (a logo): upload/clear there instead of typing an id. */ upload_url: string; + /** i18n key for `description`, owned by the module that declared the key; + * `description` (English) is the fallback when it is unset or missing. */ + description_key?: string; } interface Props { @@ -36,6 +39,11 @@ export function TenantSettingRow({ setting, onChanged }: Props) { const [draft, setDraft] = useState(setting.value ?? ''); const [busy, setBusy] = useState(false); const overridden = setting.value !== null; + // A key declared by another module: not in this file's typed key set, so it + // is looked up dynamically, with the server's English text as the default. + const description = setting.description_key + ? t(setting.description_key as never, { defaultValue: setting.description }) + : setting.description; const inputId = `tenant-setting-${setting.key}`; async function send(method: 'PUT' | 'DELETE') { @@ -72,9 +80,7 @@ export function TenantSettingRow({ setting, onChanged }: Props) { {t(overridden ? keys.tenants.settings.overridden : keys.tenants.settings.inherited)} - {setting.description && ( -

{setting.description}

- )} + {description &&

{description}

} {setting.upload_url ? ( { expect.objectContaining({ method: 'POST' }), ); }); + + test('the description is translated by its key, with the English as fallback', () => { + const { rerender } = render( + {}} + />, + ); + expect(screen.getByText('Nom de l’application')).toBeInTheDocument(); + + rerender( + {}} + />, + ); + expect(screen.getByText('A motto')).toBeInTheDocument(); + }); }); diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index 79b0a089..d1854db0 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -233,6 +233,13 @@ export default { 'branding.manage.unsaved_changes_other': '', 'branding.manage.upload_error_toast': '', 'branding.nav.branding': '', + 'branding.tenant_settings.app_name': '', + 'branding.tenant_settings.design_pack': '', + 'branding.tenant_settings.favicon_file_id': '', + 'branding.tenant_settings.footer_text': '', + 'branding.tenant_settings.logo_dark_file_id': '', + 'branding.tenant_settings.logo_file_id': '', + 'branding.tenant_settings.primary_color': '', 'dashboard.doctor.applied': '', 'dashboard.doctor.apply_pending': '', 'dashboard.doctor.checks.auth_provider': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index b1fb3e68..e72d7902 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -294,6 +294,15 @@ export const keys = { nav: { branding: 'branding.nav.branding', }, + tenant_settings: { + app_name: 'branding.tenant_settings.app_name', + design_pack: 'branding.tenant_settings.design_pack', + favicon_file_id: 'branding.tenant_settings.favicon_file_id', + footer_text: 'branding.tenant_settings.footer_text', + logo_dark_file_id: 'branding.tenant_settings.logo_dark_file_id', + logo_file_id: 'branding.tenant_settings.logo_file_id', + primary_color: 'branding.tenant_settings.primary_color', + }, }, dashboard: { doctor: { From 76f9beea54fd7fbacc0d15c6b0fb6c584f73bc13 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 16:25:39 +0200 Subject: [PATCH 3/6] fix(branding): dark-logo pairing, empty overrides inherit, per-app cache, system saves publish, image keys clear_via (ship review) - a tenant logo without a dark logo no longer shows the platform dark logo - an empty-string override inherits the platform value (clear = delete) - the tenant cache, in-flight reads and epoch live on app.state.branding - the system theme save passes the invalidation bus so other workers forget - image keys declare clear_via so settings' generic deletes refuse them Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- docs/modules/branding.md | 7 + modules/branding/branding/locales/en.json | 8 +- modules/branding/branding/module.py | 2 +- modules/branding/branding/service.py | 5 +- modules/branding/branding/services.py | 5 +- modules/branding/branding/shared_props.py | 4 +- modules/branding/branding/tenant_branding.py | 107 ++++++++----- .../branding/branding/tenant_definitions.py | 29 ++-- modules/branding/tests/test_branding_reap.py | 6 +- modules/branding/tests/test_ship_review.py | 143 ++++++++++++++++++ .../branding/tests/test_tenant_branding.py | 6 +- .../tests/test_tenant_branding_assets.py | 8 +- .../tests/test_tenant_branding_cost.py | 8 +- 13 files changed, 264 insertions(+), 74 deletions(-) create mode 100644 modules/branding/tests/test_ship_review.py diff --git a/docs/modules/branding.md b/docs/modules/branding.md index 4c6d3a56..f53cc8dc 100644 --- a/docs/modules/branding.md +++ b/docs/modules/branding.md @@ -47,6 +47,13 @@ The tenant is always the request's active one — never an id from the URL. See | `POST /api/branding/tenant/{logo,logo-dark,favicon}` | `multipart` (field `file`) → the tenant's effective `BrandingOut` | | `DELETE /api/branding/tenant/{logo,logo-dark,favicon}` | → `BrandingOut` (the tenant falls back to the system image) | +Rules for tenant overrides: + +- **Clearing = deleting the override.** An empty-string override is treated as unset: the tenant inherits the platform value (it never blanks it). +- **Dark logo pairing.** A tenant that overrides the logo but not the dark logo does not get the platform's dark logo; dark surfaces fall back to the tenant's own logo. +- **Images are cleared only here.** The generic settings deletes (`DELETE /api/settings/tenant/current/{key}`, the platform `tenant/{scope_id}/{key}` and by-id routes) answer `422` for image keys, pointing at `DELETE /api/branding/tenant/{asset}`, because only this route reaps the stored file. A platform operator can still delete the leftover row of a tenant that no longer exists. +- **The cache is per app** (`app.state.branding.tenant_cache`), and system theme saves publish `settings.values` so other workers drop their merged tenant entries. + Uploads are validated **before** the bytes reach `file_storage`: an unsupported or unconvincing type returns `415`, an oversized image `413` (see [Image guard-rails](#image-guard-rails)). ### Public assets (anonymous) diff --git a/modules/branding/branding/locales/en.json b/modules/branding/branding/locales/en.json index e7fb49b5..de85cdc6 100644 --- a/modules/branding/branding/locales/en.json +++ b/modules/branding/branding/locales/en.json @@ -56,11 +56,11 @@ }, "tenant_settings": { "app_name": "Application name shown in the header, page titles and emails.", - "primary_color": "Accent colour as #rrggbb; empty uses the theme default.", - "design_pack": "Design pack slug; empty uses the base look.", - "footer_text": "Footer caption; empty shows the platform's.", + "primary_color": "Accent colour as #rrggbb. Reset to use the platform's.", + "design_pack": "Design pack slug. Reset to use the platform's.", + "footer_text": "Footer caption. Reset to show the platform's.", "logo_file_id": "Logo image.", - "logo_dark_file_id": "Logo for dark surfaces; falls back to the logo.", + "logo_dark_file_id": "Logo for dark surfaces; falls back to this organisation's own logo.", "favicon_file_id": "Browser tab icon." } } diff --git a/modules/branding/branding/module.py b/modules/branding/branding/module.py index 232c6243..03ab1906 100644 --- a/modules/branding/branding/module.py +++ b/modules/branding/branding/module.py @@ -59,7 +59,7 @@ def register_settings(self, app: FastAPI) -> None: def register_invalidations(self, bus: InvalidationBus, app: FastAPI) -> None: from branding.tenant_branding import subscribe - subscribe(bus) + subscribe(bus, app) def register_permissions(self, registry: PermissionRegistry) -> None: registry.add_group( diff --git a/modules/branding/branding/service.py b/modules/branding/branding/service.py index 4d01b766..938f5266 100644 --- a/modules/branding/branding/service.py +++ b/modules/branding/branding/service.py @@ -68,7 +68,10 @@ async def apply(self, changes: dict[str, Any]) -> BrandingOut: from settings.service import SettingService from settings.store import SettingsStore - store = SettingsStore(SettingService(self.db)) + # With the bus, so the write publishes ``settings.values`` and other + # workers' merged tenant caches (which inherit system values) drop. + invalidation = getattr(self.app.state.sm, "invalidation", None) + store = SettingsStore(SettingService(self.db, invalidation=invalidation)) bus = self.app.state.sm.event_bus await apply_changes_and_reload(self.app, bus, store, package=PACKAGE, changes=changes) return self.current() diff --git a/modules/branding/branding/services.py b/modules/branding/branding/services.py index 44d59df4..df6f0564 100644 --- a/modules/branding/branding/services.py +++ b/modules/branding/branding/services.py @@ -8,9 +8,10 @@ from __future__ import annotations -from dataclasses import dataclass +from dataclasses import dataclass, field from branding.settings import BrandingSettings +from branding.tenant_branding import TenantCache @dataclass @@ -18,6 +19,8 @@ class BrandingServices: """Branding module singletons.""" settings: BrandingSettings + #: Per-app cache of tenants' merged branding (see ``tenant_branding``). + tenant_cache: TenantCache = field(default_factory=TenantCache) @property def favicon_url(self) -> str | None: diff --git a/modules/branding/branding/shared_props.py b/modules/branding/branding/shared_props.py index 5f7a012c..ac73fe3d 100644 --- a/modules/branding/branding/shared_props.py +++ b/modules/branding/branding/shared_props.py @@ -103,5 +103,7 @@ async def branding_shared_props(request: Request) -> dict: return {} resolved = await resolve(request) if resolved.tenant_fields: - request.state.branding = BrandingServices(settings=resolved.settings) + request.state.branding = BrandingServices( + settings=resolved.settings, tenant_cache=request.app.state.branding.tenant_cache + ) return {"branding": branding_payload(resolved.settings)} diff --git a/modules/branding/branding/tenant_branding.py b/modules/branding/branding/tenant_branding.py index 48760246..38d3be9e 100644 --- a/modules/branding/branding/tenant_branding.py +++ b/modules/branding/branding/tenant_branding.py @@ -42,16 +42,27 @@ _PREFIX = f"{PACKAGE}." -# tenant id -> (raw overrides, system object merged against, merged result) -_CACHE: TTLCache[str, tuple[dict[str, str], BrandingSettings, ResolvedBranding]] = TTLCache( - maxsize=10_000, ttl=TENANT_CACHE_TTL_SECONDS -) -# Bumped by every forget, so a read that started before an invalidation does -# not store its (possibly stale) result after it. -_epoch = 0 -# tenant id -> (epoch the read started in, the read). Single-flight: concurrent -# misses for one tenant share one read instead of each opening a session. -_INFLIGHT: dict[str, tuple[int, asyncio.Future[dict[str, str]]]] = {} + +@dataclass(slots=True) +class TenantCache: + """One app's tenant-branding cache, held as ``app.state.branding.tenant_cache``. + + Per app, not per process: two apps in one process (tests, embedding) must + not serve each other's overrides, and an invalidation heard by one must + not drop the other's. + """ + + # tenant id -> (raw overrides, system object merged against, merged result) + entries: TTLCache[str, tuple[dict[str, str], BrandingSettings, ResolvedBranding]] = field( + default_factory=lambda: TTLCache(maxsize=10_000, ttl=TENANT_CACHE_TTL_SECONDS) + ) + # Bumped by every forget, so a read that started before an invalidation does + # not store its (possibly stale) result after it. + epoch: int = 0 + # tenant id -> (epoch the read started in, the read). Single-flight: + # concurrent misses for one tenant share one read instead of each opening a + # session. + inflight: dict[str, tuple[int, asyncio.Future[dict[str, str]]]] = field(default_factory=dict) @dataclass(frozen=True, slots=True) @@ -67,34 +78,32 @@ def owner_of(self, name: str) -> str | None: return self.tenant_id if name in self.tenant_fields else None -def forget(tenant_id: str | None = None) -> None: - """Drop one tenant's entry, or every entry when ``tenant_id`` is ``None``.""" - global _epoch - _epoch += 1 +def forget(app: FastAPI, tenant_id: str | None = None) -> None: + """Drop one tenant's entry in ``app``, or every entry when ``tenant_id`` is ``None``.""" + cache: TenantCache = app.state.branding.tenant_cache + cache.epoch += 1 # A read already in flight may predate the change: later misses must start # a fresh one rather than join it (its own waiters still get their answer). if tenant_id is None: - _CACHE.clear() - _INFLIGHT.clear() + cache.entries.clear() + cache.inflight.clear() else: - _CACHE.pop(tenant_id, None) - _INFLIGHT.pop(tenant_id, None) + cache.entries.pop(tenant_id, None) + cache.inflight.pop(tenant_id, None) -def _on_settings_changed(inv: Invalidation) -> None: +def subscribe(bus: InvalidationBus, app: FastAPI) -> None: + from settings.constants import INVALIDATION_CHANNEL from settings.contracts.invalidation import parse_invalidation_key - tenant_id, key = parse_invalidation_key(inv.key) - if key is not None and not key.startswith(_PREFIX): - return # someone else's setting - # A system value is inherited by every tenant without its own override. - forget(tenant_id) + def on_settings_changed(inv: Invalidation) -> None: + tenant_id, key = parse_invalidation_key(inv.key) + if key is not None and not key.startswith(_PREFIX): + return # someone else's setting + # A system value is inherited by every tenant without its own override. + forget(app, tenant_id) - -def subscribe(bus: InvalidationBus) -> None: - from settings.constants import INVALIDATION_CHANNEL - - bus.subscribe(INVALIDATION_CHANNEL, _on_settings_changed) + bus.subscribe(INVALIDATION_CHANNEL, on_settings_changed) def tenancy_active(app: FastAPI) -> bool: @@ -139,12 +148,24 @@ def merge(system: BrandingSettings, tenant_id: str, overrides: dict[str, str]) - Writes are validated, so a bad value means a hand-edited row — it must degrade to the system value for that field, not break every page render. + + An empty-string override is *unset*: the field inherits the platform's + value. To go back to inheriting, delete the override; storing ``""`` has + the same effect rather than blanking the platform's value. + + The dark logo is paired with the light one: a tenant that overrides the + logo but not the dark logo must not be shown the *platform's* dark logo + beside its own logo, so the inherited dark logo is dropped and dark + surfaces fall back to the tenant's logo. """ - good = dict(overrides) + good = {name: value for name, value in overrides.items() if value != ""} base: dict[str, Any] = system.model_dump() while good: try: - merged = BrandingSettings(**{**base, **good}) + values = {**base, **good} + if "logo_file_id" in good and "logo_dark_file_id" not in good: + values["logo_dark_file_id"] = "" + merged = BrandingSettings(**values) return ResolvedBranding(merged, tenant_id, frozenset(good)) except ValidationError as exc: bad = {str(err["loc"][0]) for err in exc.errors() if err.get("loc")} @@ -156,18 +177,20 @@ def merge(system: BrandingSettings, tenant_id: str, overrides: dict[str, str]) - return ResolvedBranding(system, tenant_id, frozenset()) -def _shared_read(app: FastAPI, tenant_id: str) -> tuple[int, asyncio.Future[dict[str, str]]]: +def _shared_read( + app: FastAPI, cache: TenantCache, tenant_id: str +) -> tuple[int, asyncio.Future[dict[str, str]]]: """The in-flight override read for ``tenant_id``, started if there is none.""" - entry = _INFLIGHT.get(tenant_id) + entry = cache.inflight.get(tenant_id) if entry is not None: return entry read = asyncio.ensure_future(read_overrides(app, tenant_id)) - entry = (_epoch, read) - _INFLIGHT[tenant_id] = entry + entry = (cache.epoch, read) + cache.inflight[tenant_id] = entry def _done(_: asyncio.Future[dict[str, str]]) -> None: - if _INFLIGHT.get(tenant_id) is entry: - del _INFLIGHT[tenant_id] + if cache.inflight.get(tenant_id) is entry: + del cache.inflight[tenant_id] read.add_done_callback(_done) return entry @@ -177,19 +200,20 @@ async def resolve_for(app: FastAPI, tenant_id: str | None) -> ResolvedBranding: system: BrandingSettings = app.state.branding.settings if not tenant_id: return ResolvedBranding(system) - hit = _CACHE.get(tenant_id) + cache: TenantCache = app.state.branding.tenant_cache + hit = cache.entries.get(tenant_id) if hit is not None: overrides, merged_against, resolved = hit if merged_against is system: return resolved else: - started, read = _shared_read(app, tenant_id) + started, read = _shared_read(app, cache, tenant_id) # Shielded: one waiter being cancelled must not cancel everyone's read. overrides = await asyncio.shield(read) - if _epoch != started: + if cache.epoch != started: return merge(system, tenant_id, overrides) # don't cache a racing read resolved = merge(system, tenant_id, overrides) - _CACHE[tenant_id] = (overrides, system, resolved) + cache.entries[tenant_id] = (overrides, system, resolved) return resolved @@ -200,6 +224,7 @@ async def resolve(request: Request) -> ResolvedBranding: __all__ = [ "ResolvedBranding", + "TenantCache", "forget", "merge", "overrides_from", diff --git a/modules/branding/branding/tenant_definitions.py b/modules/branding/branding/tenant_definitions.py index 5dd9a386..0a5f0940 100644 --- a/modules/branding/branding/tenant_definitions.py +++ b/modules/branding/branding/tenant_definitions.py @@ -8,12 +8,17 @@ * a scalar must pass the same validator the system value does (422); * a design pack must be one an installed module provides (422); -* an image key is refused on every generic settings route (422): images are - set and cleared only through ``/api/branding/tenant/{asset}``, which - validates the bytes as an image, stores them as the tenant's own file and - reaps the file it replaces once the write commits. A generic write could do - none of that — it could point the logo at any file the tenant owns (a PDF), - and the file it displaced would never be reaped. +* an image key is refused on every generic settings write *and delete* route + (422, ``clear_via``): images are set and cleared only through + ``/api/branding/tenant/{asset}``, which validates the bytes as an image, + stores them as the tenant's own file and reaps the file it replaces or + clears once the write commits. A generic write could do none of that — it + could point the logo at any file the tenant owns (a PDF), and a generic + delete would leave the file behind. (A row left by a deleted tenant is the + one thing a platform operator may still delete by hand.) + +An empty override is the same as none: the tenant inherits the platform's +value. To go back to inheriting, delete the override. """ from __future__ import annotations @@ -38,11 +43,11 @@ _DESCRIPTIONS = { "app_name": "Application name shown in the header, page titles and emails.", - "primary_color": "Accent colour as #rrggbb; empty uses the theme default.", - "design_pack": "Design pack slug; empty uses the base look.", - "footer_text": "Footer caption; empty shows the platform's.", + "primary_color": "Accent colour as #rrggbb. Delete the override to use the platform's.", + "design_pack": "Design pack slug. Delete the override to use the platform's.", + "footer_text": "Footer caption. Delete the override to show the platform's.", "logo_file_id": "Logo image.", - "logo_dark_file_id": "Logo for dark surfaces; falls back to the logo.", + "logo_dark_file_id": "Logo for dark surfaces; falls back to the tenant's own logo.", "favicon_file_id": "Browser tab icon.", } _ASSET_OF = {field: asset for asset, field in TENANT_ASSETS.items()} @@ -71,6 +76,7 @@ def register_tenant_definitions(app: FastAPI) -> None: defaults = BrandingSettings().model_dump() for name in TENANT_FIELDS: asset = _ASSET_OF.get(name) + upload_url = f"{ROUTE_PREFIX}/tenant/{asset}" if asset else "" registry.add( SettingDefinition( key=f"{PACKAGE}.{name}", @@ -79,7 +85,8 @@ def register_tenant_definitions(app: FastAPI) -> None: description_key=f"{PACKAGE}.tenant_settings.{name}", tenant_overridable=True, check=_make_check(name), - upload_url=f"{ROUTE_PREFIX}/tenant/{asset}" if asset else "", + upload_url=upload_url, + clear_via=upload_url, ) ) diff --git a/modules/branding/tests/test_branding_reap.py b/modules/branding/tests/test_branding_reap.py index 332a41f7..15bbf6f4 100644 --- a/modules/branding/tests/test_branding_reap.py +++ b/modules/branding/tests/test_branding_reap.py @@ -22,10 +22,10 @@ @pytest.fixture(autouse=True) -def _fresh_cache(): - tenant_branding.forget() +def _fresh_cache(app): + tenant_branding.forget(app) yield - tenant_branding.forget() + tenant_branding.forget(app) async def _upload(client, url: str = TENANT_LOGO, body: bytes = _PNG) -> None: diff --git a/modules/branding/tests/test_ship_review.py b/modules/branding/tests/test_ship_review.py new file mode 100644 index 00000000..c2aa8fa4 --- /dev/null +++ b/modules/branding/tests/test_ship_review.py @@ -0,0 +1,143 @@ +"""Ship-review fixes for tenant branding: merge pairing, empty overrides, the +per-app cache, system-save invalidation and generic deletes of image keys.""" + +from __future__ import annotations + +import httpx +from branding import tenant_branding +from branding.services import BrandingServices +from branding.settings import BrandingSettings +from branding.tenant_branding import TenantCache, merge +from settings.constants import INVALIDATION_CHANNEL +from settings.contracts.invalidation import parse_invalidation_key +from settings.contracts.schemas import SettingScope, SettingUpsert +from settings.service import SettingService +from simple_module_db import all_tenants +from sqlalchemy import select + +_PNG = b"\x89PNG\r\n\x1a\n" + b"\x00" * 32 +TENANT_LOGO = "/api/branding/tenant/logo" +CURRENT = "/api/settings/tenant/current" + + +def _system() -> BrandingSettings: + return BrandingSettings( + app_name="Platform", + primary_color="#112233", + footer_text="Platform footer", + logo_file_id="sys-logo", + logo_dark_file_id="sys-dark", + ) + + +class TestMergePairing: + def test_tenant_logo_without_dark_does_not_inherit_the_platform_dark_logo(self): + got = merge(_system(), "t", {"logo_file_id": "t-logo"}).settings + assert got.logo_file_id == "t-logo" + assert got.logo_dark_file_id == "" # dark surfaces fall back to t-logo + + def test_tenant_dark_logo_alone_keeps_the_platform_light_logo(self): + got = merge(_system(), "t", {"logo_dark_file_id": "t-dark"}).settings + assert (got.logo_file_id, got.logo_dark_file_id) == ("sys-logo", "t-dark") + + def test_both_overridden(self): + got = merge(_system(), "t", {"logo_file_id": "a", "logo_dark_file_id": "b"}).settings + assert (got.logo_file_id, got.logo_dark_file_id) == ("a", "b") + + def test_no_logo_override_inherits_both(self): + got = merge(_system(), "t", {"app_name": "Acme"}).settings + assert (got.logo_file_id, got.logo_dark_file_id) == ("sys-logo", "sys-dark") + + +class TestEmptyOverrideInherits: + def test_empty_string_is_unset(self): + resolved = merge( + _system(), "t", {"footer_text": "", "primary_color": "", "app_name": "Acme"} + ) + assert resolved.settings.footer_text == "Platform footer" + assert resolved.settings.primary_color == "#112233" + assert resolved.settings.app_name == "Acme" + assert resolved.tenant_fields == frozenset({"app_name"}) + + def test_empty_logo_override_does_not_blank_the_dark_pairing(self): + got = merge(_system(), "t", {"logo_file_id": ""}).settings + assert (got.logo_file_id, got.logo_dark_file_id) == ("sys-logo", "sys-dark") + + +class TestPerAppCache: + def test_each_services_object_owns_its_cache(self): + a = BrandingServices(settings=_system()) + b = BrandingServices(settings=_system()) + assert isinstance(a.tenant_cache, TenantCache) + assert a.tenant_cache is not b.tenant_cache + + async def test_forget_touches_only_that_app(self, app): + class _Other: + class state: # noqa: N801 + branding = BrandingServices(settings=_system()) + + other = _Other + for target in (app, other): + cache = target.state.branding.tenant_cache + cache.entries["acme"] = ({}, _system(), None) # type: ignore[assignment] + tenant_branding.forget(app, "acme") + assert "acme" not in app.state.branding.tenant_cache.entries + assert "acme" in other.state.branding.tenant_cache.entries + + +async def test_a_system_theme_save_publishes_the_invalidation(app, authenticated_client): + seen: list[tuple[str | None, str | None]] = [] + app.state.sm.invalidation.subscribe( + INVALIDATION_CHANNEL, lambda inv: seen.append(parse_invalidation_key(inv.key)) + ) + resp = await authenticated_client.put("/api/branding/", json={"app_name": "Published"}) + assert resp.status_code == 200, resp.text + assert (None, "branding.app_name") in seen + + +async def _logo_row(app, tenant_id: str): + async with app.state.sm.db.session_factory() as db: + return await SettingService(db).get_scoped( + SettingScope.TENANT, tenant_id, "branding.logo_file_id" + ) + + +async def _upload(client: httpx.AsyncClient) -> None: + resp = await client.post(TENANT_LOGO, files={"file": ("logo.png", _PNG, "image/png")}) + assert resp.status_code == 200, resp.text + + +async def test_generic_delete_of_an_image_key_is_refused_and_leaks_nothing( + app, authenticated_client, tenant_client +): + from file_storage.models import StoredFile + + async with tenant_client() as a: + await _upload(a.client) + key = "branding.logo_file_id" + refused = await a.client.delete(f"{CURRENT}/{key}") + assert refused.status_code == 422 + assert TENANT_LOGO in refused.json()["detail"] + platform = await authenticated_client.delete(f"/api/settings/tenant/{a.tenant_id}/{key}") + assert platform.status_code == 422 + row = await _logo_row(app, a.tenant_id) + assert row is not None + with all_tenants(): + async with app.state.sm.db.session_factory() as db: + ids = (await db.execute(select(StoredFile.id))).scalars().all() + assert row.value in {str(i) for i in ids} # row and file both still there + + # The branding route is what clears it, and reaps. + assert (await a.client.delete(TENANT_LOGO)).status_code in (200, 204) + assert await _logo_row(app, a.tenant_id) is None + + +async def test_platform_can_clear_a_leftover_row_of_a_deleted_tenant(app, authenticated_client): + async with app.state.sm.db.session_factory() as db: + await SettingService(db).upsert_scoped( + SettingScope.TENANT, "gone", "branding.logo_file_id", SettingUpsert(value="f") + ) + await db.commit() + resp = await authenticated_client.delete("/api/settings/tenant/gone/branding.logo_file_id") + assert resp.status_code == 204 + assert await _logo_row(app, "gone") is None diff --git a/modules/branding/tests/test_tenant_branding.py b/modules/branding/tests/test_tenant_branding.py index 40b18a14..c882909a 100644 --- a/modules/branding/tests/test_tenant_branding.py +++ b/modules/branding/tests/test_tenant_branding.py @@ -23,10 +23,10 @@ @pytest.fixture(autouse=True) -def _fresh_cache(): - tenant_branding.forget() +def _fresh_cache(app): + tenant_branding.forget(app) yield - tenant_branding.forget() + tenant_branding.forget(app) async def _branding(client: httpx.AsyncClient, page: str = "/tenants/") -> dict: diff --git a/modules/branding/tests/test_tenant_branding_assets.py b/modules/branding/tests/test_tenant_branding_assets.py index 98fe2496..4457489c 100644 --- a/modules/branding/tests/test_tenant_branding_assets.py +++ b/modules/branding/tests/test_tenant_branding_assets.py @@ -27,10 +27,10 @@ @pytest.fixture(autouse=True) -def _fresh_cache(): - tenant_branding.forget() +def _fresh_cache(app): + tenant_branding.forget(app) yield - tenant_branding.forget() + tenant_branding.forget(app) async def _upload(client: httpx.AsyncClient, body: bytes = _PNG) -> dict: @@ -259,5 +259,5 @@ async def test_a_setting_naming_a_non_image_file_serves_404(self, app, tenant_cl SettingUpsert(value=pdf), ) await db.commit() - tenant_branding.forget() + tenant_branding.forget(app) assert (await a.client.get(LOGO_URL)).status_code == 404 diff --git a/modules/branding/tests/test_tenant_branding_cost.py b/modules/branding/tests/test_tenant_branding_cost.py index a0a1acc9..85db1480 100644 --- a/modules/branding/tests/test_tenant_branding_cost.py +++ b/modules/branding/tests/test_tenant_branding_cost.py @@ -16,10 +16,10 @@ @pytest.fixture(autouse=True) -def _fresh_cache(): - tenant_branding.forget() +def _fresh_cache(app): + tenant_branding.forget(app) yield - tenant_branding.forget() + tenant_branding.forget(app) def _request(path: str, **headers: str) -> Request: @@ -93,7 +93,7 @@ async def slow(_app, _tenant_id): monkeypatch.setattr(tenant_branding, "read_overrides", slow) first = asyncio.create_task(tenant_branding.resolve_for(app, "acme")) await asyncio.sleep(0) - tenant_branding.forget("acme") # the row changed while the read was in flight + tenant_branding.forget(app, "acme") # the row changed while the read was in flight second = asyncio.create_task(tenant_branding.resolve_for(app, "acme")) await asyncio.sleep(0) release.set() From 37d0622ab4db71b80c35e6d1bacc9443778fae0e Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 17:02:08 +0200 Subject: [PATCH 4/6] fix(branding): serve the platform image when a tenant's logo file is gone (qa BUG-005) Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- modules/branding/branding/endpoints/assets.py | 34 ++++++++++++++----- .../tests/test_tenant_branding_assets.py | 19 +++++++++++ 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/modules/branding/branding/endpoints/assets.py b/modules/branding/branding/endpoints/assets.py index 1f1f1606..24e86c82 100644 --- a/modules/branding/branding/endpoints/assets.py +++ b/modules/branding/branding/endpoints/assets.py @@ -96,6 +96,14 @@ def _configured_file_id(resolved: ResolvedBranding, field: str) -> uuid.UUID: raise HTTPException(status_code=404, detail="No branding image is set.") from exc +async def _download(storage: FileStorageService, file_id: uuid.UUID, owner: str | None): + """The file as its owner sees it: a platform file when ``owner`` is ``None``.""" + if owner is None: + return await storage.download(file_id, platform=True) + with tenant_context(owner): + return await storage.download(file_id) + + async def _serve( request: Request, storage: FileStorageService, field: str ) -> RedirectResponse | StreamingResponse: @@ -106,16 +114,24 @@ async def _serve( request_tenant = resolved.tenant_id owner = resolved.owner_of(field) try: - if owner is None: - download = await storage.download(file_id, platform=True) - else: - with tenant_context(owner): - download = await storage.download(file_id) + download = await _download(storage, file_id, owner) 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. - logger.warning("Branding %s references missing file %s.", field, file_id) - raise HTTPException(status_code=404, detail="Branding image is unavailable.") from exc + if owner is None: + # Referenced file went away underneath us. 404 uncached, so the next + # request retries once the setting is fixed rather than caching a miss. + logger.warning("Branding %s references missing file %s.", field, file_id) + raise HTTPException(status_code=404, detail="Branding image is unavailable.") from exc + # The tenant's own image was deleted (through the Files API, say) while + # its override still names it. Show the platform's image instead of a + # dead one; the override is left for the tenant to replace or reset. + logger.warning("Tenant %s branding %s references missing file %s.", owner, field, file_id) + file_id = _configured_file_id(ResolvedBranding(request.app.state.branding.settings), field) + try: + download = await _download(storage, file_id, None) + except StoredFileNotFoundError as missing: + raise HTTPException( + status_code=404, detail="Branding image is unavailable." + ) from missing if normalize_content_type(download.file.content_type) not in ALLOWED_IMAGE_TYPES: # Uploads are validated, so this is a hand-edited (or pre-validation) diff --git a/modules/branding/tests/test_tenant_branding_assets.py b/modules/branding/tests/test_tenant_branding_assets.py index 4457489c..e89f7c8d 100644 --- a/modules/branding/tests/test_tenant_branding_assets.py +++ b/modules/branding/tests/test_tenant_branding_assets.py @@ -100,6 +100,25 @@ async def test_replacing_and_clearing_reap_the_tenants_old_file(self, app, tenan assert (await a.client.get(LOGO_URL)).status_code == 404 assert [r.is_deleted for r in await _files(app)] == [True, True] + async def test_a_deleted_tenant_logo_falls_back_to_the_platform_logo( + self, app, authenticated_client, tenant_client + ): + """qa BUG-005: deleting the file via Files left a dangling override that 404'd.""" + await authenticated_client.post( + "/api/branding/logo", files={"file": ("p.png", _PNG + b"p", "image/png")} + ) + async with tenant_client() as a: + out = await _upload(a.client) + [_, row] = await _files(app) + gone = await a.client.delete(f"/api/file-storage/files/{row.id}") + assert gone.status_code == 204, gone.text + tenant_branding.forget(app) + + resp = await a.client.get(out["logo_url"]) # the tenant's now-dead URL + assert resp.status_code == 200 + assert resp.content == _PNG + b"p" + assert resp.headers["cache-control"] == "private, no-cache" + async def test_member_cannot_upload(self, tenant_client): async with ( tenant_client() as owner, From fb023840e8d8eb6820e9c5188a2b78878903e755 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 17:38:18 +0200 Subject: [PATCH 5/6] fix(branding): clear tenant images as owner; docs match the every-scope guard (ship review r2) Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- docs/modules/branding.md | 2 +- .../branding/branding/endpoints/tenant_api.py | 4 +++- .../branding/branding/tenant_definitions.py | 19 +++++++++++-------- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/docs/modules/branding.md b/docs/modules/branding.md index f53cc8dc..fbb45892 100644 --- a/docs/modules/branding.md +++ b/docs/modules/branding.md @@ -51,7 +51,7 @@ Rules for tenant overrides: - **Clearing = deleting the override.** An empty-string override is treated as unset: the tenant inherits the platform value (it never blanks it). - **Dark logo pairing.** A tenant that overrides the logo but not the dark logo does not get the platform's dark logo; dark surfaces fall back to the tenant's own logo. -- **Images are cleared only here.** The generic settings deletes (`DELETE /api/settings/tenant/current/{key}`, the platform `tenant/{scope_id}/{key}` and by-id routes) answer `422` for image keys, pointing at `DELETE /api/branding/tenant/{asset}`, because only this route reaps the stored file. A platform operator can still delete the leftover row of a tenant that no longer exists. +- **Images are cleared only here.** `SettingService.delete`/`delete_scoped` refuse an image key at **every** scope (SYSTEM, TENANT and USER rows alike) and for every caller that holds a service built through settings' dependency: the generic settings deletes (`DELETE /api/settings/tenant/current/{key}`, the platform `tenant/{scope_id}/{key}` and by-id routes, the admin store screen) answer `422` pointing at `DELETE /api/branding/tenant/{asset}`, because only this route reaps the stored file. This route deletes with `as_owner=True`. A platform operator can still delete the leftover TENANT row of a tenant that no longer exists. A service constructed without the settings registry (`SettingService(db)`, e.g. branding's own system-scope writes) does not enforce the guard. - **The cache is per app** (`app.state.branding.tenant_cache`), and system theme saves publish `settings.values` so other workers drop their merged tenant entries. Uploads are validated **before** the bytes reach `file_storage`: an unsupported or unconvincing type returns `415`, an oversized image `413` (see [Image guard-rails](#image-guard-rails)). diff --git a/modules/branding/branding/endpoints/tenant_api.py b/modules/branding/branding/endpoints/tenant_api.py index 2e73c6e2..790891fa 100644 --- a/modules/branding/branding/endpoints/tenant_api.py +++ b/modules/branding/branding/endpoints/tenant_api.py @@ -54,7 +54,9 @@ async def _swap( key = f"{PACKAGE}.{field}" previous = await settings.get_scoped(SettingScope.TENANT, tenant_id, key) if file_id is None: - await settings.delete_scoped(SettingScope.TENANT, tenant_id, key) + # The owner of the key (``clear_via``): the reap below is what a bare + # row delete would skip, so the service's guard is stood down here. + await settings.delete_scoped(SettingScope.TENANT, tenant_id, key, as_owner=True) else: await settings.upsert_scoped( SettingScope.TENANT, tenant_id, key, SettingUpsert(value=file_id) diff --git a/modules/branding/branding/tenant_definitions.py b/modules/branding/branding/tenant_definitions.py index 0a5f0940..590f5b74 100644 --- a/modules/branding/branding/tenant_definitions.py +++ b/modules/branding/branding/tenant_definitions.py @@ -8,14 +8,17 @@ * a scalar must pass the same validator the system value does (422); * a design pack must be one an installed module provides (422); -* an image key is refused on every generic settings write *and delete* route - (422, ``clear_via``): images are set and cleared only through - ``/api/branding/tenant/{asset}``, which validates the bytes as an image, - stores them as the tenant's own file and reaps the file it replaces or - clears once the write commits. A generic write could do none of that — it - could point the logo at any file the tenant owns (a PDF), and a generic - delete would leave the file behind. (A row left by a deleted tenant is the - one thing a platform operator may still delete by hand.) +* an image key is refused by ``SettingService.delete``/``delete_scoped`` at + every scope (422, ``clear_via``), whichever route or caller holds a service + built through settings' ``get_setting_service``: images are set only through + ``/api/branding/tenant/{asset}`` and cleared there, which validates the + bytes as an image, stores them as the tenant's own file and reaps the file it + replaces or clears once the write commits. A generic write could do none of + that — it could point the logo at any file the tenant owns (a PDF) — and a + generic delete would leave the file behind. That route deletes with + ``as_owner=True``. A TENANT row left by a deleted tenant is the one thing a + platform operator may still delete by hand. A service built without the + registry (``SettingService(db)``) does not enforce the guard. An empty override is the same as none: the tenant inherits the platform's value. To go back to inheriting, delete the override. From d967b06e72174668fc1d8d35da3805f84a14bed0 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 1 Oct 2026 18:00:45 +0200 Subject: [PATCH 6/6] fix(branding): scope-aware clear_via so the system refusal names the system route (ship qa r2) Claude-Session: https://claude.ai/code/session_01F8RiTBUJQnZmSq56qReZeV --- .../branding/branding/tenant_definitions.py | 13 +++++- .../tests/test_branding_clear_route_scope.py | 42 +++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 modules/branding/tests/test_branding_clear_route_scope.py diff --git a/modules/branding/branding/tenant_definitions.py b/modules/branding/branding/tenant_definitions.py index 590f5b74..2418fd80 100644 --- a/modules/branding/branding/tenant_definitions.py +++ b/modules/branding/branding/tenant_definitions.py @@ -74,12 +74,23 @@ async def check(request: Request, tenant_id: str, value: str) -> None: def register_tenant_definitions(app: FastAPI) -> None: from settings.contracts.registry import SettingDefinition + from settings.contracts.schemas import SettingScope registry = app.state.settings.registry defaults = BrandingSettings().model_dump() for name in TENANT_FIELDS: asset = _ASSET_OF.get(name) upload_url = f"{ROUTE_PREFIX}/tenant/{asset}" if asset else "" + # The system row clears through the platform's own DELETE route; the + # tenant route cannot touch it. + clear_via = ( + { + SettingScope.SYSTEM: f"{ROUTE_PREFIX}/{asset}", + SettingScope.TENANT: upload_url, + } + if asset + else "" + ) registry.add( SettingDefinition( key=f"{PACKAGE}.{name}", @@ -89,7 +100,7 @@ def register_tenant_definitions(app: FastAPI) -> None: tenant_overridable=True, check=_make_check(name), upload_url=upload_url, - clear_via=upload_url, + clear_via=clear_via, ) ) diff --git a/modules/branding/tests/test_branding_clear_route_scope.py b/modules/branding/tests/test_branding_clear_route_scope.py new file mode 100644 index 00000000..ae811580 --- /dev/null +++ b/modules/branding/tests/test_branding_clear_route_scope.py @@ -0,0 +1,42 @@ +"""The managed-key refusal names the route that clears the row's own scope (ship qa r2). + +The system logo is removed through ``DELETE /api/branding/logo``; the tenant +route cannot touch it, so pointing a SYSTEM-scope refusal there was a dead end. +""" + +from __future__ import annotations + +from settings.constants import SYSTEM_SCOPE_ID +from settings.contracts.schemas import SettingScope, SettingUpsert +from settings.service import SettingService + +KEY = "branding.logo_file_id" +SYSTEM_ROUTE = "/api/branding/logo" +TENANT_ROUTE = "/api/branding/tenant/logo" +CURRENT = "/api/settings/tenant/current" + + +async def test_system_scope_refusal_names_the_system_route(app, authenticated_client): + async with app.state.sm.db.session_factory() as db: + await SettingService(db).upsert_scoped( + SettingScope.SYSTEM, SYSTEM_SCOPE_ID, KEY, SettingUpsert(value="1") + ) + await db.commit() + resp = await authenticated_client.delete(f"/api/settings/system/{KEY}") + assert resp.status_code == 422, resp.text + detail = resp.json()["detail"] + assert SYSTEM_ROUTE in detail + assert TENANT_ROUTE not in detail + assert resp.json()["clear_via"] == SYSTEM_ROUTE + + +async def test_tenant_scope_refusal_names_the_tenant_route(app, tenant_client): + async with tenant_client() as t: + async with app.state.sm.db.session_factory() as db: + await SettingService(db).upsert_scoped( + SettingScope.TENANT, t.tenant_id, KEY, SettingUpsert(value="1") + ) + await db.commit() + resp = await t.client.delete(f"{CURRENT}/{KEY}") + assert resp.status_code == 422, resp.text + assert resp.json()["clear_via"] == TENANT_ROUTE