diff --git a/docs/framework/multi-tenancy.md b/docs/framework/multi-tenancy.md index 364a6e9f..b0e301c6 100644 --- a/docs/framework/multi-tenancy.md +++ b/docs/framework/multi-tenancy.md @@ -239,6 +239,15 @@ overrides with no tenant bound — deliberately cross-tenant (the table is not `MultiTenantMixin`), so keep it that way. No tenant role holds `feature_flags.manage`; the tenant-override admin screens are platform-only. +## Settings + +`settings` keeps its explicit `(scope, scope_id, key)` rows — no mixin. A key +declared `tenant_overridable` can be changed by a tenant owner/admin for their +active tenant (`/api/settings/tenant/current/{key}`, `settings.tenant.edit`); +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). + 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/settings.md b/docs/modules/settings.md index 168f153f..280e9594 100644 --- a/docs/modules/settings.md +++ b/docs/modules/settings.md @@ -72,11 +72,52 @@ class OrdersModule(ModuleBase): The browse UI uses these definitions to render meaningful empty states for unset keys. +### Tenant-overridable keys + +A definition with `tenant_overridable=True` can be changed by a tenant for +itself (#382). Tenant owners and admins hold `settings.tenant.edit` (mapped onto +`tenant:owner` / `tenant:admin`; members get nothing) and write through +`/api/settings/tenant/current/{key}`, which acts on `request.state.tenant_id` +only — the tenant never comes from the URL. Keys without the flag answer 422 +there; a request acting for no tenant gets 403. The `tenants` module renders +these keys at `/tenants/settings`. + +```python +async def check_logo(request, tenant_id: str, value: str) -> None: + if value and not await tenant_owns_file(request.app, tenant_id, value): + raise LookupError("unknown file") # -> 404; ValueError -> 422 + + +registry.add( + SettingDefinition( + key="orders.checkout_note", + tenant_overridable=True, + check=check_logo, + ) +) +``` + +`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. + +Runtime reads need nothing new: `SettingsDep` is already bound to the active +tenant, so `await settings.get(key)` resolves **tenant → system → default**. + +### Cache invalidation + +Every SYSTEM / TENANT write publishes on the `settings.values` +[invalidation](/framework/invalidation) channel after commit, keyed per +(tenant, key): `"|"`, or `"|"` for a system write (every +tenant inheriting it is affected). `settings.contracts.invalidation` builds and +parses the key. Settings caches nothing itself; a module that caches a resolved +per-tenant value subscribes and forgets. + ## Routes ### Generic K/V API (`/api/settings/...`) -All write endpoints require `settings.edit` / `settings.create` / `settings.delete`; reads need `settings.view`. +All write endpoints require `settings.edit` / `settings.create` / `settings.delete`; reads need `settings.view`. These are platform-operator permissions: no tenant role holds them, and the holder may write any key at system scope or at any tenant's scope. A TENANT `scope_id` must name a real tenant when a tenant-owning module is installed (`simple_module_core.tenancy.tenant_exists`): 404 on the `/tenant/{scope_id}/…` GET/PUT, 422 for a `POST /` body. `DELETE` stays unvalidated so a deleted tenant's leftovers can be cleared. A key declaring `SettingDefinition.clear_via` (a file set through an upload route, which reaps the stored file) is protected inside `SettingService.delete` / `delete_scoped`, so it holds for every scope (SYSTEM, TENANT, USER) and every caller: a generic delete raises `ManagedKeyError`, which the API maps to 422 naming that route (`clear_via` may be a `{SettingScope: route}` mapping when system and tenant rows clear differently; the 422 names the route for the row's scope, via `clear_route`) and the Inertia store screen shows as a toast. Two deliberate ways through: the owner of the key passes `as_owner=True` after taking responsibility for the file (branding's tenant clear route reaps it), and a TENANT row whose tenant no longer exists may be cleared by a platform operator. The guard needs the registry, which `get_setting_service` supplies; a bare `SettingService(db)` guards nothing. | Method + path | Purpose | |---|---| @@ -85,6 +126,8 @@ All write endpoints require `settings.edit` / `settings.create` / `settings.dele | `GET / PUT / DELETE /api/settings/system/{key}` | system-scope CRUD | | `GET / PUT / DELETE /api/settings/tenant/{scope_id}/{key}` | tenant-scope CRUD | | `GET / PUT / DELETE /api/settings/user/{scope_id}/{key}` | user-scope CRUD | +| `GET /api/settings/tenant/current` | the active tenant's overridable keys: `inherited`, own `value`, `effective` (`settings.tenant.edit`) | +| `GET / PUT / DELETE /api/settings/tenant/current/{key}` | the active tenant's override of an overridable key (`settings.tenant.edit`) | | `POST /api/settings/` | create with explicit `scope` + `scope_id` (`SettingCreate`) | | `GET / PUT / DELETE /api/settings/{setting_id}` | by-id CRUD | diff --git a/docs/modules/tenants.md b/docs/modules/tenants.md index f54317ac..17f46504 100644 --- a/docs/modules/tenants.md +++ b/docs/modules/tenants.md @@ -34,6 +34,7 @@ scoped to them. |---|---|---| | `GET /tenants/` | signed in | My organisations: switch, create | | `GET /tenants/members` | `tenants.members.view` | Members and invitations of the active tenant | +| `GET /tenants/settings` | `settings.tenant.edit` | The active tenant's overrides of `tenant_overridable` settings | | `GET /tenants/invitations/accept?token=` | signed in | Accept an invitation | | `GET /admin/tenants/` | `tenants.platform.view` | Platform list of all tenants | | `GET/POST /api/tenants/` | signed in | List mine / create | @@ -47,6 +48,12 @@ scoped to them. Tenant-level routes act on the *active* tenant (`/current`), never on an id from the URL. +Organisation settings use `settings.tenant.edit`, which the `settings` module +owns and maps onto owner and admin (#382); the page writes through +`/api/settings/tenant/current/{key}`. The former `tenants.settings.manage` +permission — declared, owner-only, and checked by nothing — is retired, so +there is one permission for the one surface. + ## Configuration DB-backed (Settings screen): diff --git a/modules/settings/settings/_announce.py b/modules/settings/settings/_announce.py new file mode 100644 index 00000000..8db64c1f --- /dev/null +++ b/modules/settings/settings/_announce.py @@ -0,0 +1,36 @@ +"""Publish a per-(tenant, key) invalidation after a settings write commits. + +Kept out of ``service.py`` (and its 300-line budget). See +``settings.contracts.invalidation`` for the key format. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from simple_module_db.callbacks import register_on_commit + +from settings.constants import INVALIDATION_CHANNEL, SCOPE_SYSTEM, SCOPE_TENANT +from settings.contracts.invalidation import invalidation_key + +if TYPE_CHECKING: + from simple_module_core.invalidation import InvalidationBus + from sqlalchemy.ext.asyncio import AsyncSession + + +def announce( + db: AsyncSession, bus: InvalidationBus | None, scope: str, scope_id: str, key: str +) -> None: + """Queue the notice for this session's commit; user-scope writes have no consumer. + + After commit, not inline: a consumer that drops its entry before the row is + durable re-reads the *old* value and caches it for a full TTL. + """ + if bus is None or scope not in (SCOPE_SYSTEM, SCOPE_TENANT): + return + wire_key = invalidation_key(scope_id if scope == SCOPE_TENANT else None, key) + + async def publish() -> None: + await bus.publish(INVALIDATION_CHANNEL, key=wire_key) + + register_on_commit(db, publish) diff --git a/modules/settings/settings/_listing.py b/modules/settings/settings/_listing.py new file mode 100644 index 00000000..b9d15d80 --- /dev/null +++ b/modules/settings/settings/_listing.py @@ -0,0 +1,121 @@ +"""Read-side listing queries of :class:`SettingService`, split out for size.""" + +from __future__ import annotations + +from simple_module_db import LIKE_ESCAPE_CHAR, like_contains_pattern +from sqlalchemy import func, select +from sqlalchemy.ext.asyncio import AsyncSession + +from settings._row_masking import out +from settings.constants import ( + ALL_SCOPES, + DEFAULT_PER_PAGE, + SCOPE_ALL, + SYSTEM_SCOPE_ID, +) +from settings.contracts.schemas import SettingOut, SettingScope +from settings.models import Setting + + +class SettingListing: + """Mixin: the ``list_*`` / ``count_by_scope`` queries. Needs ``self.db``.""" + + db: AsyncSession + + # ── Listing ───────────────────────────────────────────────────── + + async def list_all(self) -> list[SettingOut]: + result = await self.db.execute( + select(Setting).order_by(Setting.scope, Setting.scope_id, Setting.key) + ) + return [out(row) for row in result.scalars()] + + async def list_filtered( + self, + scope: SettingScope | None = None, + q: str | None = None, + page: int = 1, + per_page: int = DEFAULT_PER_PAGE, + ) -> tuple[list[SettingOut], int]: + """One page of rows plus the unpaged total for the same filters. + + The browse screen used to receive every row and filter in the browser, + which made the payload, the render and find-in-page all scale with the + whole table instead of with what was asked for. ``q`` matches the key + only — the search box says "Search keys…", and quietly matching values + would surface rows whose key has nothing to do with the query. + """ + conditions = self._filter_conditions(scope, q) + total = await self.db.scalar(select(func.count()).select_from(Setting).where(*conditions)) + stmt = ( + select(Setting) + .where(*conditions) + .order_by(Setting.scope, Setting.scope_id, Setting.key) + .offset(max(page - 1, 0) * per_page) + .limit(per_page) + ) + result = await self.db.execute(stmt) + return [out(row) for row in result.scalars()], int(total or 0) + + async def count_by_scope(self, q: str | None = None) -> dict[str, int]: + """Per-scope tallies for the filter tabs, plus ``all``. + + Every scope is named even at zero: a tab that disappears when its count + drops to nothing moves the other tabs under the cursor mid-search. + The scope filter itself is deliberately not applied — the tabs describe + what each of them *would* show, so selecting one must not zero the rest. + """ + conditions = self._filter_conditions(None, q) + stmt = select(Setting.scope, func.count()).where(*conditions).group_by(Setting.scope) + result = await self.db.execute(stmt) + tallies = {str(scope): int(count) for scope, count in result.all()} + counts = {name: tallies.get(name, 0) for name in ALL_SCOPES} + return {SCOPE_ALL: sum(counts.values()), **counts} + + @staticmethod + def _filter_conditions(scope: SettingScope | None, q: str | None) -> list: + conditions = [] + if scope is not None: + conditions.append(Setting.scope == scope.value) + needle = (q or "").strip() + if needle: + # Setting keys are full of underscores, and `_` is a LIKE wildcard: + # unescaped, a search for "smtp_host" also matches "smtpXhost", and + # a stray "%" matches the entire table. ``ilike`` is emulated by + # SQLAlchemy on SQLite (lower() on both sides), so one expression + # is case-insensitive on both databases. + conditions.append( + Setting.key.ilike(like_contains_pattern(needle), escape=LIKE_ESCAPE_CHAR) + ) + return conditions + + async def list_by_scope( + self, scope: SettingScope, scope_id: str = SYSTEM_SCOPE_ID + ) -> list[SettingOut]: + result = await self.db.execute(self._scope_stmt(scope, scope_id)) + return [out(row) for row in result.scalars()] + + async def list_by_scope_unmasked( + self, scope: SettingScope, scope_id: str = SYSTEM_SCOPE_ID + ) -> list[SettingOut]: + """The same rows with their real values, for code that *applies* them. + + The masking in :func:`_out` is for the screens. Hydration is not a + screen: ``SettingsStore`` feeds these values back into the live module + settings objects at boot, so a masked read writes a row of dots over the + real secret — a mailer that cannot authenticate, and a + ``reset_password_token_secret`` that no longer verifies the tokens it + signed. This is the one read that must see through the mask, and it is + spelled out rather than reached by passing a flag so that every caller + of it is one grep away. + """ + result = await self.db.execute(self._scope_stmt(scope, scope_id)) + return [SettingOut.model_validate(row) for row in result.scalars()] + + @staticmethod + def _scope_stmt(scope: SettingScope, scope_id: str): + return ( + select(Setting) + .where(Setting.scope == scope.value, Setting.scope_id == scope_id) + .order_by(Setting.key) + ) diff --git a/modules/settings/settings/_managed_keys.py b/modules/settings/settings/_managed_keys.py new file mode 100644 index 00000000..233d9fe1 --- /dev/null +++ b/modules/settings/settings/_managed_keys.py @@ -0,0 +1,59 @@ +"""Rows of a key set by upload are not deleted by a bare row delete. + +A key declaring ``SettingDefinition.clear_via`` is a file the owner's upload +route stores and reaps. Deleting the row any other way leaves the stored file +behind, so :meth:`SettingService.delete` / ``delete_scoped`` refuse it with +:class:`ManagedKeyError` **for every scope** — the rule lives in the service so +a route (or a module calling the service directly) cannot forget it. + +Two ways through, both deliberate: + +* the owner passes ``as_owner=True`` after taking responsibility for the file + (branding's clear routes reap it); +* a TENANT row whose tenant no longer exists has no live owner left to clear + it, so a platform operator may delete it by hand. SYSTEM and USER rows get no + such exception: a SYSTEM row always has a live owner (the platform's own + upload route), and a USER row of a managed key is cleared through the owner + too. The guard is only as wide as the registry the service was built with; + a service built without one (``SettingService(db)``) guards nothing. +""" + +from __future__ import annotations + +from collections.abc import Awaitable, Callable + +from settings.constants import ERR_MANAGED_KEY_DELETE +from settings.contracts.registry import SettingsRegistry, clear_route +from settings.contracts.schemas import SettingScope + +TenantIsLive = Callable[[str], Awaitable[bool]] + + +class ManagedKeyError(Exception): + """The key is cleared through ``clear_via``, not by deleting its row.""" + + def __init__(self, key: str, clear_via: str) -> None: + self.key = key + self.clear_via = clear_via + super().__init__(ERR_MANAGED_KEY_DELETE.format(clear_via=clear_via)) + + +async def ensure_deletable( + registry: SettingsRegistry | None, + tenant_is_live: TenantIsLive | None, + scope: str, + scope_id: str, + key: str, +) -> None: + """Raise :class:`ManagedKeyError` unless the row may be deleted generically.""" + definition = registry.get(key) if registry is not None else None + if definition is None or not definition.clear_via: + return + orphaned = ( + scope == SettingScope.TENANT.value + and tenant_is_live is not None + and not await tenant_is_live(scope_id) + ) + if orphaned: + return + raise ManagedKeyError(key, clear_route(definition, scope)) diff --git a/modules/settings/settings/_module_settings_props.py b/modules/settings/settings/_module_settings_props.py index fd0d7884..311f55a0 100644 --- a/modules/settings/settings/_module_settings_props.py +++ b/modules/settings/settings/_module_settings_props.py @@ -9,9 +9,10 @@ from typing import Any +from fastapi import FastAPI from fastapi.encoders import jsonable_encoder -from settings._module_settings import ModuleSettingsView +from settings._module_settings import ModuleSettingsView, _package_of def serialize(views: list[ModuleSettingsView]) -> list[dict[str, Any]]: @@ -54,3 +55,24 @@ def serialize(views: list[ModuleSettingsView]) -> list[dict[str, Any]]: } for v in views ] + + +def testable_packages(app: FastAPI) -> dict[str, list[str]]: + """Package -> the names of the health checks its module registered. + + "Test connection" is just that module's health checks run on demand — + reusing the registry means settings never learns what an SMTP or an S3 + connection is. The names come back with the packages so the button can say + what it is about to dial ("Test mailer connection") instead of the useless + "Test connection" a bare package list can produce. + """ + checks_by_owner: dict[str, list[str]] = {} + for check in app.state.sm.health_registry.all_checks: + if check.module: + checks_by_owner.setdefault(check.module, []).append(check.name) + + return { + _package_of(mod): sorted(checks_by_owner[mod.meta.name]) + for mod in getattr(app.state.sm, "modules", ()) + if mod.meta.name in checks_by_owner + } diff --git a/modules/settings/settings/_unique_write.py b/modules/settings/settings/_unique_write.py new file mode 100644 index 00000000..8dc3c0b7 --- /dev/null +++ b/modules/settings/settings/_unique_write.py @@ -0,0 +1,29 @@ +"""Race-safe inserts for the (scope, scope_id, key) unique key. + +A read-then-insert cannot be made safe by reading harder: two requests can both +see "no row" and both insert. The unique constraint is the arbiter, so the +insert runs in a savepoint — a loser's ``IntegrityError`` rolls back only the +savepoint, leaving the request's transaction (and its session) usable. +""" + +from __future__ import annotations + +from sqlalchemy.exc import IntegrityError +from sqlalchemy.ext.asyncio import AsyncSession + +from settings.models import Setting + + +class DuplicateSettingError(Exception): + """A row with this (scope, scope_id, key) already exists.""" + + +async def insert_if_free(db: AsyncSession, entity: Setting) -> bool: + """Insert ``entity``; ``False`` when its (scope, scope_id, key) is already taken.""" + try: + async with db.begin_nested(): + db.add(entity) + await db.flush() + except IntegrityError: + return False + return True diff --git a/modules/settings/settings/constants.py b/modules/settings/settings/constants.py index 8032a101..ded7d293 100644 --- a/modules/settings/settings/constants.py +++ b/modules/settings/settings/constants.py @@ -70,6 +70,11 @@ API_SYSTEM_PATH: Final = "/system/{key}" API_TENANT_PATH: Final = "/tenant/{scope_id}/{key}" API_USER_PATH: Final = "/user/{scope_id}/{key}" +# The active tenant's own overrides (#382). The tenant comes from +# ``request.state.tenant_id`` — never from the URL — so "current" is a literal +# segment, registered ahead of ``API_TENANT_PATH`` so it is not read as an id. +API_TENANT_CURRENT_PATH: Final = "/tenant/current" +API_TENANT_CURRENT_KEY_PATH: Final = "/tenant/current/{key}" # ── Menu ───────────────────────────────────────────────────────────── MENU_LABEL: Final = MODULE_NAME @@ -85,7 +90,17 @@ PERM_CREATE: Final = "settings.create" PERM_EDIT: Final = "settings.edit" PERM_DELETE: Final = "settings.delete" -ALL_PERMISSIONS: Final = (PERM_VIEW, PERM_CREATE, PERM_EDIT, PERM_DELETE) +# Edit the *active* tenant's overrides of ``tenant_overridable`` keys — and +# nothing else. Mapped onto ``tenant:owner`` / ``tenant:admin``; the four +# above stay platform-operator permissions (system scope, any tenant's scope). +PERM_TENANT_EDIT: Final = "settings.tenant.edit" +ALL_PERMISSIONS: Final = (PERM_VIEW, PERM_CREATE, PERM_EDIT, PERM_DELETE, PERM_TENANT_EDIT) + +# ── Cache invalidation ─────────────────────────────────────────────── +# Published (after commit) for every SYSTEM / TENANT write, keyed per +# (tenant, key) — see ``settings.contracts.invalidation``. Settings caches +# nothing itself; consumers holding per-tenant resolved values subscribe. +INVALIDATION_CHANNEL: Final = "settings.values" # ── Database ───────────────────────────────────────────────────────── DB_SCHEMA: Final = MODULE_PACKAGE @@ -130,16 +145,23 @@ # ── User-facing error messages ─────────────────────────────────────── ERR_SETTING_NOT_FOUND: Final = "Setting not found" ERR_KEY_ALREADY_EXISTS: Final = "Setting key already exists" +ERR_SETTING_EXISTS: Final = "A setting with this scope and key already exists" ERR_SYSTEM_SCOPE_NO_ID: Final = "system scope must not have a scope_id" ERR_SCOPED_REQUIRES_ID: Final = "tenant/user scope requires a scope_id" ERR_UNKNOWN_SCOPE: Final = "unknown scope" ERR_VALUE_MISMATCH: Final = "value does not parse as declared value_type" +ERR_UNKNOWN_TENANT: Final = "Unknown tenant" +ERR_NO_ACTIVE_TENANT: Final = "No active organisation for this request" +ERR_MANAGED_KEY_DELETE: Final = "This setting is managed elsewhere; clear it at {clear_via}" +ERR_NOT_TENANT_OVERRIDABLE: Final = "This setting cannot be changed per organisation" # ── HTTP ───────────────────────────────────────────────────────────── STATUS_CREATED: Final = 201 STATUS_NO_CONTENT: Final = 204 +STATUS_FORBIDDEN: Final = 403 STATUS_NOT_FOUND: Final = 404 STATUS_CONFLICT: Final = 409 +STATUS_UNPROCESSABLE: Final = 422 # ── Query parameter names ──────────────────────────────────────────── QP_USER_ID: Final = "user_id" diff --git a/modules/settings/settings/contracts/__init__.py b/modules/settings/settings/contracts/__init__.py index f0957b23..a4fdbab7 100644 --- a/modules/settings/settings/contracts/__init__.py +++ b/modules/settings/settings/contracts/__init__.py @@ -1,7 +1,7 @@ """Settings contracts — public interface for other modules.""" from settings.contracts.accessor import SettingsAccessor -from settings.contracts.registry import SettingDefinition, SettingsRegistry +from settings.contracts.registry import SettingDefinition, SettingsRegistry, clear_route from settings.contracts.schemas import ( SettingCreate, SettingOut, @@ -21,4 +21,5 @@ "SettingValueType", "SettingsAccessor", "SettingsRegistry", + "clear_route", ] diff --git a/modules/settings/settings/contracts/invalidation.py b/modules/settings/settings/contracts/invalidation.py new file mode 100644 index 00000000..aadc4e64 --- /dev/null +++ b/modules/settings/settings/contracts/invalidation.py @@ -0,0 +1,39 @@ +"""Per-(tenant, key) invalidation notices for settings writes (#382). + +``settings`` keeps no cache of its own, but consumers resolving a value per +tenant do (branding's per-tenant theme). Every SYSTEM / TENANT write publishes +on :data:`INVALIDATION_CHANNEL` after commit, with the key built here: + +* ``"|"`` — one tenant's override of ``key`` changed; +* ``"|"`` — the system value changed, so every tenant's view of ``key`` + may have (tenants without an override inherit it). + +``|`` cannot appear in a tenant id (``simple_module_db.TENANT_ID_PATTERN``), so +the first one always separates the two halves. Handlers may only forget. +""" + +from __future__ import annotations + +from settings.constants import INVALIDATION_CHANNEL + +_SEP = "|" + + +def invalidation_key(tenant_id: str | None, key: str) -> str: + """The wire key for a write of ``key`` at ``tenant_id`` (``None`` = system).""" + return f"{tenant_id or ''}{_SEP}{key}" + + +def parse_invalidation_key(raw: str | None) -> tuple[str | None, str | None]: + """``(tenant_id, key)``; ``tenant_id`` is ``None`` for a system write. + + ``(None, None)`` for a whole-channel clear (``raw is None``) or a key this + module did not build — both mean "forget everything". + """ + if raw is None or _SEP not in raw: + return None, None + tenant_id, _, key = raw.partition(_SEP) + return (tenant_id or None), key + + +__all__ = ["INVALIDATION_CHANNEL", "invalidation_key", "parse_invalidation_key"] diff --git a/modules/settings/settings/contracts/registry.py b/modules/settings/settings/contracts/registry.py index 237fc9a8..47b0b952 100644 --- a/modules/settings/settings/contracts/registry.py +++ b/modules/settings/settings/contracts/registry.py @@ -25,21 +25,64 @@ def on_startup(self, app): from __future__ import annotations +from collections.abc import Awaitable, Callable, Mapping from dataclasses import dataclass, field +from typing import TYPE_CHECKING from settings.constants import ERR_KEY_ALREADY_EXISTS from settings.contracts.schemas import SettingScope, SettingValueType +if TYPE_CHECKING: + from starlette.requests import Request + +TenantValueCheck = Callable[["Request", str, str], Awaitable[None]] +"""``async (request, tenant_id, value)`` run before a TENANT-scope write. + +Raise ``ValueError`` to reject the value (422) or ``LookupError`` when it names +something the tenant does not have (404) — e.g. a file id owned by another +tenant. ``tenant_id`` is the scope being written, which on the platform routes +is not the caller's own active tenant.""" + @dataclass(frozen=True, slots=True) class SettingDefinition: - """Declared metadata for a setting key.""" + """Declared metadata for a setting key. + + ``tenant_overridable`` opts the key into tenant self-service (#382): a + tenant owner/admin may write it for their *own* active tenant through + ``/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. + ``clear_via`` names the route that owns clearing the key (e.g. an upload + route that also reaps the stored file): the generic DELETE routes answer + 422 pointing there while the tenant exists. A platform operator may still + delete the row left behind by a tenant that no longer exists. A key whose + system and tenant rows are cleared through different routes passes a + ``{SettingScope: route}`` mapping; read it with :func:`clear_route`. + """ key: str default: str = "" description: str = "" scope: SettingScope = SettingScope.SYSTEM value_type: SettingValueType = SettingValueType.STRING + tenant_overridable: bool = False + check: TenantValueCheck | None = field(default=None, compare=False) + clear_via: str | Mapping[SettingScope, str] = "" + + +def clear_route(definition: SettingDefinition, scope: SettingScope | str) -> str: + """The route that clears ``definition`` at ``scope`` ("" if unmanaged). + + A scope the mapping does not name falls back to its first route, so the + refusal still points somewhere rather than at nothing. + """ + via = definition.clear_via + if isinstance(via, str): + return via + if not via: + return "" + return via.get(SettingScope(scope)) or next(iter(via.values())) @dataclass(slots=True) @@ -62,5 +105,10 @@ def get(self, key: str) -> SettingDefinition | None: def all_definitions(self) -> list[SettingDefinition]: return list(self._defs.values()) + @property + def tenant_overridable(self) -> list[SettingDefinition]: + """Definitions a tenant may override for itself, sorted by key.""" + return sorted((d for d in self._defs.values() if d.tenant_overridable), key=lambda d: d.key) + def __contains__(self, key: str) -> bool: return key in self._defs diff --git a/modules/settings/settings/deps.py b/modules/settings/settings/deps.py index 7ea13394..5cff6a08 100644 --- a/modules/settings/settings/deps.py +++ b/modules/settings/settings/deps.py @@ -18,12 +18,21 @@ from settings.contracts.accessor import SettingsAccessor from settings.contracts.registry import SettingsRegistry from settings.service import SettingService +from settings.tenant_scope import is_known_tenant, registry_of async def get_setting_service( + request: Request, db: AsyncSession = Depends(get_db), ) -> SettingService: - return SettingService(db) + bus = getattr(getattr(request.app.state, "sm", None), "invalidation", None) + + async def tenant_is_live(tenant_id: str) -> bool: + return await is_known_tenant(request, tenant_id) + + return SettingService( + db, invalidation=bus, registry=registry_of(request), tenant_is_live=tenant_is_live + ) def get_settings_registry(request: Request) -> SettingsRegistry: diff --git a/modules/settings/settings/endpoints/_create_form.py b/modules/settings/settings/endpoints/_create_form.py new file mode 100644 index 00000000..5dbc78bb --- /dev/null +++ b/modules/settings/settings/endpoints/_create_form.py @@ -0,0 +1,34 @@ +"""The create-setting form action, split out of ``views`` to keep it under the file cap.""" + +from __future__ import annotations + +from fastapi import Request +from pydantic import ValidationError +from simple_module_hosting.inertia_utils import redirect_back_with_errors, validation_errors_to_dict +from starlette.responses import RedirectResponse + +from settings._unique_write import DuplicateSettingError +from settings.constants import ERR_SETTING_EXISTS +from settings.contracts.schemas import SettingCreate, SettingScope +from settings.service import SettingService +from settings.tenant_scope import tenant_write_error + + +async def create_from_form( + request: Request, service: SettingService, redirect_to: str +) -> RedirectResponse: + """Validate and store a posted setting; field errors go back to the form.""" + body = await request.json() + try: + data = SettingCreate(**body) + except ValidationError as exc: + return redirect_back_with_errors(request, validation_errors_to_dict(exc)) + if data.scope is SettingScope.TENANT and ( + error := await tenant_write_error(request, data.scope_id, data.key, data.value) + ): + return redirect_back_with_errors(request, {"scope_id": error}) + try: + await service.create(data) + except DuplicateSettingError: + return redirect_back_with_errors(request, {"key": ERR_SETTING_EXISTS}) + return RedirectResponse(redirect_to, status_code=303) diff --git a/modules/settings/settings/endpoints/api.py b/modules/settings/settings/endpoints/api.py index 2e60cee6..7b2e71b8 100644 --- a/modules/settings/settings/endpoints/api.py +++ b/modules/settings/settings/endpoints/api.py @@ -2,16 +2,19 @@ from __future__ import annotations -from fastapi import APIRouter, Depends, HTTPException, Query +from fastapi import APIRouter, Depends, HTTPException, Query, Request from simple_module_hosting.permissions import RequiresPermission +from settings._unique_write import DuplicateSettingError from settings.constants import ( API_BY_ID_PATH, API_RESOLVE_PATH, API_SYSTEM_PATH, API_TENANT_PATH, API_USER_PATH, + ERR_SETTING_EXISTS, ERR_SETTING_NOT_FOUND, + ERR_UNKNOWN_TENANT, PERM_CREATE, PERM_DELETE, PERM_EDIT, @@ -20,9 +23,11 @@ QP_SCOPE_ID, QP_TENANT_ID, QP_USER_ID, + STATUS_CONFLICT, STATUS_CREATED, STATUS_NO_CONTENT, STATUS_NOT_FOUND, + STATUS_UNPROCESSABLE, SYSTEM_SCOPE_ID, ) from settings.contracts.schemas import ( @@ -34,6 +39,11 @@ ) from settings.deps import get_setting_service from settings.service import SettingService +from settings.tenant_scope import ( + is_known_tenant, + require_known_tenant, + run_check, +) router = APIRouter() @@ -112,12 +122,20 @@ async def delete_system_setting( raise _not_found() +# Platform-operator routes: the tenant comes from the URL, so it must name a +# real tenant (#382). DELETE stays unvalidated so a row left behind by a +# deleted tenant can still be cleared — but while the tenant exists, a key set +# by upload is cleared through its upload route, which reaps the file. + + @router.get(API_TENANT_PATH, response_model=SettingOut, dependencies=_VIEW) async def get_tenant_setting( scope_id: str, key: str, + request: Request, service: SettingService = Depends(get_setting_service), ) -> SettingOut: + await require_known_tenant(request, scope_id) result = await service.get_scoped(SettingScope.TENANT, scope_id, key) if result is None: raise _not_found() @@ -129,8 +147,11 @@ async def upsert_tenant_setting( scope_id: str, key: str, data: SettingUpsert, + request: Request, service: SettingService = Depends(get_setting_service), ) -> SettingOut: + await require_known_tenant(request, scope_id) + await run_check(request, scope_id, key, data.value) return await service.upsert_scoped(SettingScope.TENANT, scope_id, key, data) @@ -182,9 +203,18 @@ async def delete_user_setting( @router.post("/", response_model=SettingOut, status_code=STATUS_CREATED, dependencies=_CREATE) async def create_setting( data: SettingCreate, + request: Request, service: SettingService = Depends(get_setting_service), ) -> SettingOut: - return await service.create(data) + if data.scope is SettingScope.TENANT: + # A body field, not a path segment: an unknown tenant is invalid input. + if not await is_known_tenant(request, data.scope_id): + raise HTTPException(status_code=STATUS_UNPROCESSABLE, detail=ERR_UNKNOWN_TENANT) + await run_check(request, data.scope_id, data.key, data.value) + try: + return await service.create(data) + except DuplicateSettingError as exc: + raise HTTPException(status_code=STATUS_CONFLICT, detail=ERR_SETTING_EXISTS) from exc @router.get(API_BY_ID_PATH, response_model=SettingOut, dependencies=_VIEW) @@ -201,8 +231,12 @@ async def get_setting( async def update_setting( setting_id: int, data: SettingUpdate, + request: Request, service: SettingService = Depends(get_setting_service), ) -> SettingOut: + current = await service.get_by_id(setting_id) + if current is not None and current.scope is SettingScope.TENANT and data.value is not None: + await run_check(request, current.scope_id, current.key, data.value) result = await service.update(setting_id, data) if result is None: raise _not_found() diff --git a/modules/settings/settings/endpoints/tenant_api.py b/modules/settings/settings/endpoints/tenant_api.py new file mode 100644 index 00000000..5e2d6aef --- /dev/null +++ b/modules/settings/settings/endpoints/tenant_api.py @@ -0,0 +1,88 @@ +"""Self-service settings for the active tenant (#382). + +``/api/settings/tenant/current[/{key}]`` reads and writes the TENANT-scope rows +of ``request.state.tenant_id`` — never a tenant named in the URL — and only for +keys declared ``tenant_overridable``. Guarded by ``settings.tenant.edit``, +which tenant owners and admins hold; members get 403. A request acting for no +tenant gets 403 too: there is nothing for it to edit. + +Registered ahead of the platform router, whose ``/tenant/{scope_id}/{key}`` +would otherwise read ``current`` as a tenant id. +""" + +from __future__ import annotations + +from fastapi import APIRouter, Depends, HTTPException, Request +from simple_module_hosting.permissions import RequiresPermission + +from settings.constants import ( + API_TENANT_CURRENT_KEY_PATH, + API_TENANT_CURRENT_PATH, + ERR_SETTING_NOT_FOUND, + PERM_TENANT_EDIT, + STATUS_NO_CONTENT, + STATUS_NOT_FOUND, + STATUS_UNPROCESSABLE, +) +from settings.contracts.schemas import SettingOut, SettingScope, SettingUpsert +from settings.deps import get_setting_service +from settings.service import SettingService +from settings.tenant_scope import ( + active_tenant, + overridable_definition, + registry_of, + run_check, +) +from settings.tenant_view import TenantSettingView, list_for_tenant + +router = APIRouter(dependencies=[Depends(RequiresPermission(PERM_TENANT_EDIT))]) + + +@router.get(API_TENANT_CURRENT_PATH, response_model=list[TenantSettingView]) +async def list_current( + request: Request, service: SettingService = Depends(get_setting_service) +) -> list[TenantSettingView]: + return await list_for_tenant(service, registry_of(request), active_tenant(request)) + + +@router.get(API_TENANT_CURRENT_KEY_PATH, response_model=SettingOut) +async def get_current( + key: str, request: Request, service: SettingService = Depends(get_setting_service) +) -> SettingOut: + tenant_id = active_tenant(request) + overridable_definition(request, key) + result = await service.get_scoped(SettingScope.TENANT, tenant_id, key) + if result is None: + raise HTTPException(status_code=STATUS_NOT_FOUND, detail=ERR_SETTING_NOT_FOUND) + return result + + +@router.put(API_TENANT_CURRENT_KEY_PATH, response_model=SettingOut) +async def put_current( + key: str, + data: SettingUpsert, + request: Request, + service: SettingService = Depends(get_setting_service), +) -> SettingOut: + tenant_id = active_tenant(request) + definition = overridable_definition(request, key) + # The declared type wins over whatever the client sent: a tenant must not + # turn the platform's int into a string its readers cannot parse. + try: + upsert = SettingUpsert( + value=data.value, value_type=definition.value_type, description=data.description + ) + except ValueError as exc: + raise HTTPException(status_code=STATUS_UNPROCESSABLE, detail=str(exc)) from exc + await run_check(request, tenant_id, key, upsert.value) + return await service.upsert_scoped(SettingScope.TENANT, tenant_id, key, upsert) + + +@router.delete(API_TENANT_CURRENT_KEY_PATH, status_code=STATUS_NO_CONTENT) +async def delete_current( + key: str, request: Request, service: SettingService = Depends(get_setting_service) +) -> None: + tenant_id = active_tenant(request) + overridable_definition(request, key) + if not await service.delete_scoped(SettingScope.TENANT, tenant_id, key): + raise HTTPException(status_code=STATUS_NOT_FOUND, detail=ERR_SETTING_NOT_FOUND) diff --git a/modules/settings/settings/endpoints/views.py b/modules/settings/settings/endpoints/views.py index cf3c6331..55b60d1f 100644 --- a/modules/settings/settings/endpoints/views.py +++ b/modules/settings/settings/endpoints/views.py @@ -14,6 +14,7 @@ from fastapi import APIRouter, Depends, HTTPException, Request from pydantic import ValidationError +from simple_module_hosting.i18n_deps import TranslatorDep from simple_module_hosting.inertia_deps import InertiaDep from simple_module_hosting.inertia_utils import redirect_back_with_errors, validation_errors_to_dict from simple_module_hosting.permissions import RequiresPermission @@ -21,12 +22,13 @@ from starlette.responses import RedirectResponse from settings import browse_query, known_keys +from settings._managed_keys import ManagedKeyError from settings._module_settings import ( _package_of, collect_module_settings, overrides_by_package, ) -from settings._module_settings_props import serialize +from settings._module_settings_props import serialize, testable_packages from settings.constants import ( DEFAULT_PER_PAGE, ERR_SETTING_NOT_FOUND, @@ -50,9 +52,11 @@ VIEW_PREFIX, VIEW_STORE_PATH, ) -from settings.contracts.schemas import SettingCreate, SettingUpdate +from settings.contracts.schemas import SettingUpdate from settings.deps import get_setting_service +from settings.endpoints._create_form import create_from_form from settings.service import SettingService +from settings.tenant_scope import tenant_update_error _PAGE_BROWSE = "Settings/Browse" _PAGE_CREATE = "Settings/Create" @@ -157,13 +161,7 @@ async def create_action( request: Request, service: SettingService = Depends(get_setting_service), ) -> RedirectResponse: - body = await request.json() - try: - data = SettingCreate(**body) - except ValidationError as exc: - return redirect_back_with_errors(request, validation_errors_to_dict(exc)) - await service.create(data) - return RedirectResponse(_REDIRECT_SETTINGS, status_code=303) + return await create_from_form(request, service, _REDIRECT_SETTINGS) @router.put( @@ -181,6 +179,8 @@ async def update_action( data = SettingUpdate(**body) except ValidationError as exc: return redirect_back_with_errors(request, validation_errors_to_dict(exc)) + if error := await tenant_update_error(request, await service.get_by_id(setting_id), data.value): + return redirect_back_with_errors(request, {"value": error}) await service.update(setting_id, data) return RedirectResponse(_REDIRECT_SETTINGS, status_code=303) @@ -192,9 +192,18 @@ async def update_action( ) async def delete_action( setting_id: int, + request: Request, + t: TranslatorDep, service: SettingService = Depends(get_setting_service), ) -> RedirectResponse: - await service.delete(setting_id) + try: + await service.delete(setting_id) + except ManagedKeyError as exc: + # Shown by the page as a toast; a bare row delete would orphan the file. + message = t.t( + "settings.browse.delete_managed_error", setting=exc.key, clear_via=exc.clear_via + ) + return redirect_back_with_errors(request, {"delete": message}) return RedirectResponse(_REDIRECT_SETTINGS, status_code=303) @@ -219,32 +228,11 @@ async def modules_view( PROP_MODULES: serialize(views), # Which packages can be connection-tested, so the page only offers # the button where something is actually reachable. - PROP_TESTABLE: _testable_packages(request), + PROP_TESTABLE: testable_packages(request.app), }, ) -def _testable_packages(request: Request) -> dict[str, list[str]]: - """Package -> the names of the health checks its module registered. - - "Test connection" is just that module's health checks run on demand — - reusing the registry means settings never learns what an SMTP or an S3 - connection is. The names come back with the packages so the button can say - what it is about to dial ("Test mailer connection") instead of the useless - "Test connection" a bare package list can produce. - """ - checks_by_owner: dict[str, list[str]] = {} - for check in request.app.state.sm.health_registry.all_checks: - if check.module: - checks_by_owner.setdefault(check.module, []).append(check.name) - - return { - _package_of(mod): sorted(checks_by_owner[mod.meta.name]) - for mod in getattr(request.app.state.sm, "modules", ()) - if mod.meta.name in checks_by_owner - } - - @router.post( "/test-connection/{package}", response_model=None, diff --git a/modules/settings/settings/errors.py b/modules/settings/settings/errors.py new file mode 100644 index 00000000..368bf558 --- /dev/null +++ b/modules/settings/settings/errors.py @@ -0,0 +1,20 @@ +"""HTTP mapping of settings domain errors.""" + +from __future__ import annotations + +from fastapi import FastAPI, Request +from fastapi.responses import JSONResponse, Response + +from settings._managed_keys import ManagedKeyError +from settings.constants import STATUS_UNPROCESSABLE + + +async def _managed_key(request: Request, exc: Exception) -> Response: + assert isinstance(exc, ManagedKeyError) + return JSONResponse( + {"detail": str(exc), "clear_via": exc.clear_via}, status_code=STATUS_UNPROCESSABLE + ) + + +def install_exception_handlers(app: FastAPI) -> None: + app.add_exception_handler(ManagedKeyError, _managed_key) diff --git a/modules/settings/settings/locales/en.json b/modules/settings/settings/locales/en.json index 8c255675..73d82314 100644 --- a/modules/settings/settings/locales/en.json +++ b/modules/settings/settings/locales/en.json @@ -11,6 +11,8 @@ "delete_title": "Delete override", "delete_description": "\"{key}\" will be removed and the key falls back to the next scope down. This cannot be undone.", "delete_confirm_button": "Delete override", + "delete_failed": "The override could not be deleted.", + "delete_managed_error": "\"{setting}\" is managed elsewhere and cannot be deleted here. Clear it at {clear_via}.", "description": "Database overrides. Precedence: user beats tenant beats system beats env default.", "search_placeholder": "Search keys…", "scope_filter_label": "Filter overrides by scope", diff --git a/modules/settings/settings/module.py b/modules/settings/settings/module.py index 3f6df5c7..81431818 100644 --- a/modules/settings/settings/module.py +++ b/modules/settings/settings/module.py @@ -23,6 +23,7 @@ MODULE_NAME, MODULE_PACKAGE, PERM_GROUP, + PERM_TENANT_EDIT, PERM_VIEW, VIEW_PREFIX, ) @@ -76,12 +77,21 @@ def register_settings(self, app: FastAPI) -> None: # Self-register so the UI lists our own settings alongside other modules. services.module_registry.register("settings", SettingsSettings) + def register_exception_handlers(self, app: FastAPI) -> None: + from settings.errors import install_exception_handlers + + install_exception_handlers(app) + def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: from settings.endpoints.api import router as api from settings.endpoints.module_api import router as module_api + from settings.endpoints.tenant_api import router as tenant_api from settings.endpoints.views import router as views api_router.include_router(module_api) + # Before ``api``: its ``/tenant/{scope_id}/{key}`` would capture + # ``/tenant/current/{key}`` with ``scope_id="current"``. + api_router.include_router(tenant_api) api_router.include_router(api) view_router.include_router(views) @@ -103,7 +113,14 @@ def register_menu_items(self, registry: MenuRegistry) -> None: ) def register_permissions(self, registry: PermissionRegistry) -> None: + from simple_module_core.tenancy import TenantRole, tenant_role + registry.add_group(PERM_GROUP, list(ALL_PERMISSIONS)) + # Tenant owners and admins edit their own tenant's overridable keys + # (#382). The one permission for it — ``tenants`` no longer ships its + # own unused ``tenants.settings.manage``. Members get nothing here. + for role in (TenantRole.OWNER, TenantRole.ADMIN): + registry.map_role(tenant_role(role), [PERM_TENANT_EDIT]) def register_audit_links(self, registry: AuditLinkRegistry) -> None: from settings.models import Setting diff --git a/modules/settings/settings/pages/Browse.tsx b/modules/settings/settings/pages/Browse.tsx index 9b6ca1d7..178829e4 100644 --- a/modules/settings/settings/pages/Browse.tsx +++ b/modules/settings/settings/pages/Browse.tsx @@ -9,6 +9,7 @@ import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import { Plus, Search, Settings as SettingsIcon, Trash2 } from 'lucide-react'; import type React from 'react'; import { useCallback, useEffect, useState } from 'react'; +import { toast } from 'sonner'; import { type ScopeCounts, type ScopeFilter, ScopeTabs } from './components/ScopeTabs'; import { StoreCards } from './components/StoreCards'; import { StoreTable } from './components/StoreTable'; @@ -57,7 +58,9 @@ function Browse({ settings, pagination, counts, filters }: Props) { function confirmDelete() { if (!pendingDelete) return; - router.delete(ROUTES.byId(pendingDelete.id)); + router.delete(ROUTES.byId(pendingDelete.id), { + onError: (errors) => toast.error(errors.delete ?? t(keys.settings.browse.delete_failed)), + }); setPendingDelete(null); } diff --git a/modules/settings/settings/service.py b/modules/settings/settings/service.py index 00285a3f..5364faf5 100644 --- a/modules/settings/settings/service.py +++ b/modules/settings/settings/service.py @@ -2,15 +2,17 @@ from __future__ import annotations -from simple_module_db import LIKE_ESCAPE_CHAR, like_contains_pattern -from sqlalchemy import func, select +from typing import TYPE_CHECKING + +from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession +from settings._announce import announce +from settings._listing import SettingListing +from settings._managed_keys import TenantIsLive, ensure_deletable from settings._row_masking import drop_placeholder_write, is_placeholder_write, out +from settings._unique_write import DuplicateSettingError, insert_if_free from settings.constants import ( - ALL_SCOPES, - DEFAULT_PER_PAGE, - SCOPE_ALL, SYSTEM_SCOPE_ID, VALUE_TYPE_STRING, ) @@ -23,114 +25,38 @@ ) from settings.models import Setting +if TYPE_CHECKING: + from simple_module_core.invalidation import InvalidationBus + + from settings.contracts.registry import SettingsRegistry -class SettingService: + +class SettingService(SettingListing): """Async CRUD + scope resolution for key/value settings. Resolution precedence when calling ``resolve`` / ``get_resolved_value``: USER > TENANT > SYSTEM. The first match in that chain is returned. """ - def __init__(self, db: AsyncSession) -> None: - self.db = db - - # ── Listing ───────────────────────────────────────────────────── - - async def list_all(self) -> list[SettingOut]: - result = await self.db.execute( - select(Setting).order_by(Setting.scope, Setting.scope_id, Setting.key) - ) - return [out(row) for row in result.scalars()] - - async def list_filtered( + def __init__( self, - scope: SettingScope | None = None, - q: str | None = None, - page: int = 1, - per_page: int = DEFAULT_PER_PAGE, - ) -> tuple[list[SettingOut], int]: - """One page of rows plus the unpaged total for the same filters. - - The browse screen used to receive every row and filter in the browser, - which made the payload, the render and find-in-page all scale with the - whole table instead of with what was asked for. ``q`` matches the key - only — the search box says "Search keys…", and quietly matching values - would surface rows whose key has nothing to do with the query. - """ - conditions = self._filter_conditions(scope, q) - total = await self.db.scalar(select(func.count()).select_from(Setting).where(*conditions)) - stmt = ( - select(Setting) - .where(*conditions) - .order_by(Setting.scope, Setting.scope_id, Setting.key) - .offset(max(page - 1, 0) * per_page) - .limit(per_page) - ) - result = await self.db.execute(stmt) - return [out(row) for row in result.scalars()], int(total or 0) - - async def count_by_scope(self, q: str | None = None) -> dict[str, int]: - """Per-scope tallies for the filter tabs, plus ``all``. - - Every scope is named even at zero: a tab that disappears when its count - drops to nothing moves the other tabs under the cursor mid-search. - The scope filter itself is deliberately not applied — the tabs describe - what each of them *would* show, so selecting one must not zero the rest. - """ - conditions = self._filter_conditions(None, q) - stmt = select(Setting.scope, func.count()).where(*conditions).group_by(Setting.scope) - result = await self.db.execute(stmt) - tallies = {str(scope): int(count) for scope, count in result.all()} - counts = {name: tallies.get(name, 0) for name in ALL_SCOPES} - return {SCOPE_ALL: sum(counts.values()), **counts} - - @staticmethod - def _filter_conditions(scope: SettingScope | None, q: str | None) -> list: - conditions = [] - if scope is not None: - conditions.append(Setting.scope == scope.value) - needle = (q or "").strip() - if needle: - # Setting keys are full of underscores, and `_` is a LIKE wildcard: - # unescaped, a search for "smtp_host" also matches "smtpXhost", and - # a stray "%" matches the entire table. ``ilike`` is emulated by - # SQLAlchemy on SQLite (lower() on both sides), so one expression - # is case-insensitive on both databases. - conditions.append( - Setting.key.ilike(like_contains_pattern(needle), escape=LIKE_ESCAPE_CHAR) - ) - return conditions - - async def list_by_scope( - self, scope: SettingScope, scope_id: str = SYSTEM_SCOPE_ID - ) -> list[SettingOut]: - result = await self.db.execute(self._scope_stmt(scope, scope_id)) - return [out(row) for row in result.scalars()] - - async def list_by_scope_unmasked( - self, scope: SettingScope, scope_id: str = SYSTEM_SCOPE_ID - ) -> list[SettingOut]: - """The same rows with their real values, for code that *applies* them. - - The masking in :func:`_out` is for the screens. Hydration is not a - screen: ``SettingsStore`` feeds these values back into the live module - settings objects at boot, so a masked read writes a row of dots over the - real secret — a mailer that cannot authenticate, and a - ``reset_password_token_secret`` that no longer verifies the tokens it - signed. This is the one read that must see through the mask, and it is - spelled out rather than reached by passing a flag so that every caller - of it is one grep away. - """ - result = await self.db.execute(self._scope_stmt(scope, scope_id)) - return [SettingOut.model_validate(row) for row in result.scalars()] - - @staticmethod - def _scope_stmt(scope: SettingScope, scope_id: str): - return ( - select(Setting) - .where(Setting.scope == scope.value, Setting.scope_id == scope_id) - .order_by(Setting.key) - ) + db: AsyncSession, + invalidation: InvalidationBus | None = None, + registry: SettingsRegistry | None = None, + tenant_is_live: TenantIsLive | None = None, + ) -> None: + self.db = db + # With a registry, ``delete``/``delete_scoped`` refuse a ``clear_via`` + # key (see ``_managed_keys``); ``tenant_is_live`` lets a deleted + # tenant's leftover row through. + self.registry = registry + self.tenant_is_live = tenant_is_live + # When given, SYSTEM/TENANT writes announce themselves after commit so + # per-tenant caches elsewhere drop the (tenant, key) they hold. + self.invalidation = invalidation + + def _changed(self, entity: Setting) -> None: + announce(self.db, self.invalidation, entity.scope, entity.scope_id, entity.key) # ── Lookup ────────────────────────────────────────────────────── @@ -193,9 +119,10 @@ async def get_resolved_value( async def create(self, data: SettingCreate) -> SettingOut: entity = Setting(**data.model_dump()) - self.db.add(entity) - await self.db.flush() + if not await insert_if_free(self.db, entity): + raise DuplicateSettingError(f"{data.scope.value}/{data.scope_id}/{data.key}") await self.db.refresh(entity) + self._changed(entity) return out(entity) async def update(self, setting_id: int, data: SettingUpdate) -> SettingOut | None: @@ -208,6 +135,7 @@ async def update(self, setting_id: int, data: SettingUpdate) -> SettingOut | Non setattr(entity, field, value) await self.db.flush() await self.db.refresh(entity) + self._changed(entity) return out(entity) async def upsert_scoped( @@ -229,34 +157,52 @@ async def upsert_scoped( ), description=data.description, ) - self.db.add(entity) - else: - if not is_placeholder_write(entity, data.value): - entity.value = data.value - if data.value_type is not None: - entity.value_type = data.value_type.value - # Honor explicit description=None as "clear"; skip only when unset. - if "description" in data.model_fields_set: - entity.description = data.description + # Two first-time writers can both get here; the loser's insert is + # refused by the unique key, and it becomes an update of the winner's row. + if await insert_if_free(self.db, entity): + await self.db.refresh(entity) + self._changed(entity) + return out(entity) + entity = await self._find(scope, scope_id, key) + if entity is None: # deleted again between the two statements + raise DuplicateSettingError(f"{scope.value}/{scope_id}/{key}") + if not is_placeholder_write(entity, data.value): + entity.value = data.value + if data.value_type is not None: + entity.value_type = data.value_type.value + # Honor explicit description=None as "clear"; skip only when unset. + if "description" in data.model_fields_set: + entity.description = data.description await self.db.flush() await self.db.refresh(entity) + self._changed(entity) return out(entity) - async def delete(self, setting_id: int) -> bool: + async def delete(self, setting_id: int, *, as_owner: bool = False) -> bool: entity = await self.db.get(Setting, setting_id) if entity is None: return False - await self.db.delete(entity) - await self.db.flush() + await self._remove(entity, as_owner) return True - async def delete_scoped(self, scope: SettingScope, scope_id: str, key: str) -> bool: + async def delete_scoped( + self, scope: SettingScope, scope_id: str, key: str, *, as_owner: bool = False + ) -> bool: + """Delete one row; ``as_owner`` is for the module that reaps a ``clear_via`` key's file.""" entity = await self._find(scope, scope_id, key) if entity is None: return False + await self._remove(entity, as_owner) + return True + + async def _remove(self, entity: Setting, as_owner: bool) -> None: + if not as_owner: + await ensure_deletable( + self.registry, self.tenant_is_live, entity.scope, entity.scope_id, entity.key + ) + self._changed(entity) await self.db.delete(entity) await self.db.flush() - return True # ── Internals ─────────────────────────────────────────────────── diff --git a/modules/settings/settings/tenant_scope.py b/modules/settings/settings/tenant_scope.py new file mode 100644 index 00000000..040600a0 --- /dev/null +++ b/modules/settings/settings/tenant_scope.py @@ -0,0 +1,130 @@ +"""Guards for TENANT-scope writes (#382). + +Two surfaces write tenant-scope rows, and they trust different things: + +* the **platform** routes (``/api/settings/tenant/{scope_id}/…``, guarded by + ``settings.*``) take the tenant from the URL, because a platform operator + legitimately edits any tenant. The id must name a real tenant + (:func:`require_known_tenant`) — a typo used to create a row no tenant would + ever read. +* the **self-service** routes (``/api/settings/tenant/current/…``, guarded by + ``settings.tenant.edit``) take the tenant from ``request.state.tenant_id`` + only (:func:`active_tenant`), and only for keys declared + ``tenant_overridable`` (:func:`overridable_definition`). + +A key with a ``clear_via`` is protected from generic deletes by +``SettingService`` itself (``_managed_keys``), not by anything here. + +Both run the definition's ``check`` (:func:`run_check`) before writing. +""" + +from __future__ import annotations + +from fastapi import HTTPException, Request +from simple_module_core.tenancy import tenant_exists + +from settings.constants import ( + ERR_NO_ACTIVE_TENANT, + ERR_NOT_TENANT_OVERRIDABLE, + ERR_UNKNOWN_TENANT, + MODULE_PACKAGE, + STATUS_FORBIDDEN, + STATUS_NOT_FOUND, + STATUS_UNPROCESSABLE, +) +from settings.contracts.registry import SettingDefinition, SettingsRegistry +from settings.contracts.schemas import SettingOut, SettingScope + + +def registry_of(request: Request) -> SettingsRegistry | None: + services = getattr(request.app.state, MODULE_PACKAGE, None) + return getattr(services, "registry", None) + + +async def is_known_tenant(request: Request, tenant_id: str) -> bool: + """False only when a tenant-owning module says the id is unknown. + + With no such module installed (``tenant_exists`` answers ``None``) every id + is accepted, which is the pre-#382 behaviour single-tenant installs rely on. + """ + return await tenant_exists(request.app, tenant_id) is not False + + +async def require_known_tenant(request: Request, tenant_id: str) -> None: + if not await is_known_tenant(request, tenant_id): + raise HTTPException(status_code=STATUS_NOT_FOUND, detail=ERR_UNKNOWN_TENANT) + + +def active_tenant(request: Request) -> str: + """The request's resolved tenant; 403 when it acts for none.""" + tenant_id = getattr(request.state, "tenant_id", None) + if not tenant_id: + raise HTTPException(status_code=STATUS_FORBIDDEN, detail=ERR_NO_ACTIVE_TENANT) + return tenant_id + + +def overridable_definition(request: Request, key: str) -> SettingDefinition: + """The key's definition when a tenant may override it; 422 otherwise.""" + registry = registry_of(request) + definition = registry.get(key) if registry is not None else None + if definition is None or not definition.tenant_overridable: + raise HTTPException(status_code=STATUS_UNPROCESSABLE, detail=ERR_NOT_TENANT_OVERRIDABLE) + return definition + + +async def run_check(request: Request, tenant_id: str, key: str, value: str) -> None: + """Run the key's declared ``check`` for a write at ``tenant_id``, if any.""" + registry = registry_of(request) + definition = registry.get(key) if registry is not None else None + if definition is None or definition.check is None: + return + try: + await definition.check(request, tenant_id, value) + except LookupError as exc: + raise HTTPException(status_code=STATUS_NOT_FOUND, detail=str(exc) or key) from exc + except ValueError as exc: + raise HTTPException(status_code=STATUS_UNPROCESSABLE, detail=str(exc) or key) from exc + + +async def tenant_write_error(request: Request, tenant_id: str, key: str, value: str) -> str | None: + """The reason a platform form may not write ``key`` at ``tenant_id``, or ``None``. + + For the Inertia store form, which reports errors on the field rather than + as an HTTP status. + """ + if not await is_known_tenant(request, tenant_id): + return ERR_UNKNOWN_TENANT + try: + await run_check(request, tenant_id, key, value) + except HTTPException as exc: + return str(exc.detail) + return None + + +async def tenant_update_error( + request: Request, current: SettingOut | None, value: str | None +) -> str | None: + """:func:`tenant_write_error` for the edit form (``PUT /settings/{id}``). + + The row's own scope, tenant and key decide — an update cannot move a row. + Only a TENANT row whose value actually changes is checked: a + description-only edit, or the masked echo of an unchanged secret, writes + nothing the check could object to. + """ + if current is None or current.scope != SettingScope.TENANT: + return None + if value is None or value == current.value: + return None + return await tenant_write_error(request, current.scope_id, current.key, value) + + +__all__ = [ + "active_tenant", + "is_known_tenant", + "overridable_definition", + "registry_of", + "require_known_tenant", + "run_check", + "tenant_update_error", + "tenant_write_error", +] diff --git a/modules/settings/settings/tenant_view.py b/modules/settings/settings/tenant_view.py new file mode 100644 index 00000000..67098ea1 --- /dev/null +++ b/modules/settings/settings/tenant_view.py @@ -0,0 +1,76 @@ +"""What a tenant sees of its overridable settings (#382). + +One row per ``tenant_overridable`` definition: the value the tenant inherits +(system override, else the declared default), its own override if any, and the +effective value — the same precedence ``SettingsAccessor`` applies at runtime. +Used by the self-service list endpoint and by the tenant settings page. +""" + +from __future__ import annotations + +from sqlmodel import SQLModel + +from settings._secrets import conceals_secret, mask +from settings.constants import SYSTEM_SCOPE_ID +from settings.contracts.registry import SettingsRegistry +from settings.contracts.schemas import SettingScope, SettingValueType +from settings.service import SettingService + + +class TenantSettingView(SQLModel): + """One overridable key, as the active tenant sees it.""" + + key: str + description: str + value_type: SettingValueType + inherited: str + """What applies without a tenant override: the system value, else the default.""" + value: str | None + """The tenant's own override, ``None`` while it inherits.""" + effective: str + + +def _shown(key: str, value: str | None, value_type: str) -> str | None: + if value is None: + return None + return mask(value) if conceals_secret(key, value, value_type) else value + + +async def list_for_tenant( + service: SettingService, registry: SettingsRegistry | None, tenant_id: str +) -> list[TenantSettingView]: + if registry is None: + return [] + definitions = registry.tenant_overridable + if not definitions: + return [] + keys = {d.key for d in definitions} + system = { + row.key: row.value + for row in await service.list_by_scope_unmasked(SettingScope.SYSTEM, SYSTEM_SCOPE_ID) + if row.key in keys + } + own = { + row.key: row.value + for row in await service.list_by_scope_unmasked(SettingScope.TENANT, tenant_id) + if row.key in keys + } + views = [] + for d in definitions: + inherited = system.get(d.key, d.default) + value = own.get(d.key) + views.append( + TenantSettingView( + key=d.key, + description=d.description, + value_type=d.value_type, + inherited=_shown(d.key, inherited, d.value_type) or "", + value=_shown(d.key, value, d.value_type), + effective=_shown(d.key, value if value is not None else inherited, d.value_type) + or "", + ) + ) + return views + + +__all__ = ["TenantSettingView", "list_for_tenant"] diff --git a/modules/settings/tests/test_clear_route_per_scope.py b/modules/settings/tests/test_clear_route_per_scope.py new file mode 100644 index 00000000..0b1de18a --- /dev/null +++ b/modules/settings/tests/test_clear_route_per_scope.py @@ -0,0 +1,64 @@ +"""``clear_via`` may differ per scope, and the refusal names the row's own route.""" + +from __future__ import annotations + +import pytest +from settings._managed_keys import ManagedKeyError +from settings.constants import SYSTEM_SCOPE_ID +from settings.contracts.registry import SettingDefinition, clear_route +from settings.contracts.schemas import SettingScope, SettingUpsert +from settings.service import SettingService + +KEY = "demo.scoped_logo" +SYSTEM_ROUTE = "/api/demo/logo" +TENANT_ROUTE = "/api/demo/tenant/logo" + + +def test_clear_route_resolution(): + plain = SettingDefinition(key="a", clear_via=TENANT_ROUTE) + mapped = SettingDefinition( + key="b", + clear_via={SettingScope.SYSTEM: SYSTEM_ROUTE, SettingScope.TENANT: TENANT_ROUTE}, + ) + assert clear_route(plain, SettingScope.SYSTEM) == TENANT_ROUTE + assert clear_route(mapped, SettingScope.SYSTEM) == SYSTEM_ROUTE + assert clear_route(mapped, "tenant") == TENANT_ROUTE + assert clear_route(mapped, SettingScope.USER) == SYSTEM_ROUTE + assert clear_route(SettingDefinition(key="c"), SettingScope.SYSTEM) == "" + + +@pytest.fixture(autouse=True) +def definitions(app): + app.state.settings.registry.add( + SettingDefinition( + key=KEY, + tenant_overridable=True, + clear_via={SettingScope.SYSTEM: SYSTEM_ROUTE, SettingScope.TENANT: TENANT_ROUTE}, + ) + ) + + +async def test_system_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="f") + ) + await db.commit() + async with app.state.sm.db.session_factory() as db: + service = SettingService(db, registry=app.state.settings.registry) + with pytest.raises(ManagedKeyError) as err: + await service.delete_scoped(SettingScope.SYSTEM, SYSTEM_SCOPE_ID, KEY) + assert err.value.clear_via == SYSTEM_ROUTE + + +async def test_tenant_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="f") + ) + await db.commit() + resp = await t.client.delete(f"/api/settings/tenant/current/{KEY}") + assert resp.status_code == 422 + assert TENANT_ROUTE in resp.json()["detail"] + assert SYSTEM_ROUTE not in resp.json()["detail"] diff --git a/modules/settings/tests/test_duplicate_and_race.py b/modules/settings/tests/test_duplicate_and_race.py new file mode 100644 index 00000000..dbe40f61 --- /dev/null +++ b/modules/settings/tests/test_duplicate_and_race.py @@ -0,0 +1,113 @@ +"""qa BUG-001 / BUG-002: the (scope, scope_id, key) unique key must never surface as a 500. + +* two first-time writers of one key: one inserts, the other becomes an update; +* creating a key that already exists: 409 from the API, an inline field error + from the admin form. +""" + +from __future__ import annotations + +import asyncio + +import pytest +from settings.contracts.schemas import SettingScope, SettingUpsert +from settings.models import Base, Setting +from settings.service import SettingService +from simple_module_db.listeners import register_listeners +from simple_module_db.session import init_db +from simple_module_test.database import ( + database_url_for_tests, + init_db_kwargs, + is_sqlite, + reset_schema, +) +from sqlalchemy import func, select + +BODY = {"scope": "system", "scope_id": "", "key": "dup.key", "value": "1"} + + +def _backends() -> list[str]: + backends = ["sqlite-file"] + if not is_sqlite(database_url_for_tests()): + backends.append("postgres") + return backends + + +@pytest.mark.parametrize("backend", _backends()) +async def test_concurrent_first_time_upserts_all_succeed(tmp_path, backend: str): + if backend == "postgres": + url = database_url_for_tests() + state = init_db(url, **init_db_kwargs(url)) + await reset_schema(state.engine) + else: + state = init_db(f"sqlite+aiosqlite:///{tmp_path}/race.db") + register_listeners(state) + try: + async with state.engine.begin() as conn: + await conn.run_sync(Base.metadata.create_all) + + async def put(i: int) -> str: + async with state.session_factory() as db: + try: + await SettingService(db).upsert_scoped( + SettingScope.TENANT, "t1", "race.key", SettingUpsert(value=f"v{i}") + ) + await db.commit() + return "ok" + except Exception as exc: + await db.rollback() + # SQLite may refuse a stale snapshot outright; that is a + # retryable "busy", never the unique-key error being fixed. + return type(exc).__name__ + + outcomes = await asyncio.gather(*(put(i) for i in range(5))) + assert "IntegrityError" not in outcomes, outcomes + assert "ok" in outcomes + async with state.session_factory() as db: + count = await db.scalar(select(func.count()).select_from(Setting)) + assert count == 1 + finally: + await state.engine.dispose() + + +async def test_api_create_duplicate_is_409(authenticated_client): + first = await authenticated_client.post("/api/settings/", json=BODY) + assert first.status_code == 201, first.text + again = await authenticated_client.post("/api/settings/", json=BODY) + assert again.status_code == 409, again.text + + +async def test_form_create_duplicate_is_an_inline_error(authenticated_client): + first = await authenticated_client.post("/api/settings/", json=BODY) + assert first.status_code == 201, first.text + resp = await authenticated_client.post( + "/admin/settings/store", + json=BODY, + headers={"Referer": "http://test/admin/settings/create", "X-Inertia": "true"}, + ) + assert resp.status_code == 303, resp.text + shown = await authenticated_client.get( + "/admin/settings/create", headers={"X-Inertia": "true", "Accept": "application/json"} + ) + assert shown.status_code == 200 + assert "already exists" in shown.json()["props"]["errors"]["key"] + + +async def test_upsert_that_loses_the_insert_race_becomes_an_update(db_session, monkeypatch): + """Deterministic stand-in for the race: the row lands after the lookup said "none".""" + service = SettingService(db_session) + await service.upsert_scoped(SettingScope.TENANT, "t1", "race.key", SettingUpsert(value="first")) + real_find = service._find + calls = 0 + + async def blind_once(*args, **kwargs): + nonlocal calls + calls += 1 + return None if calls == 1 else await real_find(*args, **kwargs) + + monkeypatch.setattr(service, "_find", blind_once) + result = await service.upsert_scoped( + SettingScope.TENANT, "t1", "race.key", SettingUpsert(value="second") + ) + assert result.value == "second" + assert await db_session.scalar(select(func.count()).select_from(Setting)) == 1 diff --git a/modules/settings/tests/test_settings_api.py b/modules/settings/tests/test_settings_api.py index 9ed006fa..52c58587 100644 --- a/modules/settings/tests/test_settings_api.py +++ b/modules/settings/tests/test_settings_api.py @@ -3,6 +3,7 @@ from __future__ import annotations import httpx +import pytest from settings.constants import ( API_PREFIX, STATUS_CREATED, @@ -11,6 +12,17 @@ ) +@pytest.fixture(autouse=True) +async def _known_tenants(app): + """The tenant ids these tests write to must name real tenants (#382).""" + from tenants.models import Tenant + + async with app.state.sm.db.session_factory() as db: + for tenant_id in ("acme", "t1", "t2"): + db.add(Tenant(id=tenant_id, slug=tenant_id, name=tenant_id)) + await db.commit() + + def _url(path: str = "") -> str: return f"{API_PREFIX}/{path.lstrip('/')}" if path else f"{API_PREFIX}/" diff --git a/modules/settings/tests/test_tenant_settings.py b/modules/settings/tests/test_tenant_settings.py new file mode 100644 index 00000000..ba0f61b1 --- /dev/null +++ b/modules/settings/tests/test_tenant_settings.py @@ -0,0 +1,255 @@ +"""Tenant self-service settings and TENANT ``scope_id`` validation (#382). + +A tenant owner/admin edits *their own* active tenant's ``tenant_overridable`` +keys through ``/api/settings/tenant/current/{key}``; the tenant never comes +from the URL. Platform routes keep editing any scope, but a TENANT ``scope_id`` +must name a real tenant. +""" + +from __future__ import annotations + +import pytest +from fastapi import Request +from settings.constants import INVALIDATION_CHANNEL, PERM_TENANT_EDIT +from settings.contracts.invalidation import parse_invalidation_key +from settings.contracts.registry import SettingDefinition +from settings.contracts.schemas import SettingScope, SettingUpsert, SettingValueType +from settings.deps import SettingsDep +from settings.service import SettingService +from simple_module_core.tenancy import TenantRole, tenant_role + +CURRENT = "/api/settings/tenant/current" +KEY = "demo.greeting" +LOCKED = "demo.locked" +COUNT = "demo.count" +CHECKED = "demo.checked" + + +@pytest.fixture(autouse=True) +def definitions(app): + registry = app.state.settings.registry + + async def check(request, tenant_id, value): + if value == "missing": + raise LookupError("no such thing for this tenant") + if value == "bad": + raise ValueError("bad value") + + registry.add(SettingDefinition(key=KEY, default="hello", tenant_overridable=True)) + registry.add(SettingDefinition(key=LOCKED, default="x")) + registry.add( + SettingDefinition( + key=COUNT, default="1", value_type=SettingValueType.INT, tenant_overridable=True + ) + ) + registry.add(SettingDefinition(key=CHECKED, tenant_overridable=True, check=check)) + + +async def _system(app, key: str, value: str) -> None: + async with app.state.sm.db.session_factory() as db: + await SettingService(db).upsert_scoped( + SettingScope.SYSTEM, "", key, SettingUpsert(value=value) + ) + await db.commit() + + +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 + + +def _row(listing: list[dict], key: str) -> dict: + return next(r for r in listing if r["key"] == key) + + +class TestPermissionMapping: + def test_owner_and_admin_hold_it_member_does_not(self, app): + role_map = app.state.sm.permissions.role_map + assert PERM_TENANT_EDIT in role_map[tenant_role(TenantRole.OWNER)] + assert PERM_TENANT_EDIT in role_map[tenant_role(TenantRole.ADMIN)] + assert PERM_TENANT_EDIT not in role_map.get(tenant_role(TenantRole.MEMBER), []) + + def test_the_unused_tenants_permission_is_retired(self, app): + names = {p for g in app.state.sm.permissions.groups for p in g.permissions} + assert "tenants.settings.manage" not in names + + +class TestSelfService: + @pytest.mark.parametrize("role", ["owner", "admin"]) + async def test_manager_edits_own_overridable_key(self, app, tenant_client, role): + async with tenant_client(role) as me: + resp = await me.client.put(f"{CURRENT}/{KEY}", json={"value": "hi"}) + assert resp.status_code == 200, resp.text + assert resp.json()["scope"] == "tenant" + assert resp.json()["scope_id"] == me.tenant_id + assert (await me.client.get(f"{CURRENT}/{KEY}")).json()["value"] == "hi" + assert await _tenant_value(app, me.tenant_id, KEY) == "hi" + + async def test_reset_falls_back_and_second_reset_404s(self, tenant_client): + async with tenant_client() as me: + await me.client.put(f"{CURRENT}/{KEY}", json={"value": "hi"}) + assert (await me.client.delete(f"{CURRENT}/{KEY}")).status_code == 204 + assert (await me.client.delete(f"{CURRENT}/{KEY}")).status_code == 404 + assert (await me.client.get(f"{CURRENT}/{KEY}")).status_code == 404 + + async def test_writes_land_on_the_active_tenant_only(self, app, tenant_client): + async with tenant_client() as a, tenant_client() as b: + await a.client.put(f"{CURRENT}/{KEY}", json={"value": "from-a"}) + listing = (await b.client.get(CURRENT)).json() + assert _row(listing, KEY)["value"] is None + assert await _tenant_value(app, b.tenant_id, KEY) is None + + async def test_cannot_reach_another_tenant_through_the_platform_routes(self, tenant_client): + async with tenant_client() as a, tenant_client() as b: + url = f"/api/settings/tenant/{b.tenant_id}/{KEY}" + assert (await a.client.put(url, json={"value": "x"})).status_code == 403 + assert (await a.client.delete(url)).status_code == 403 + assert (await a.client.get(url)).status_code == 403 + + async def test_cannot_edit_system_scope(self, tenant_client): + async with tenant_client() as me: + resp = await me.client.put(f"/api/settings/system/{KEY}", json={"value": "x"}) + assert resp.status_code == 403 + + async def test_non_overridable_and_unknown_keys_are_refused(self, tenant_client): + async with tenant_client() as me: + for key in (LOCKED, "never.declared"): + assert ( + await me.client.put(f"{CURRENT}/{key}", json={"value": "x"}) + ).status_code == 422 + assert (await me.client.get(f"{CURRENT}/{key}")).status_code == 422 + + async def test_member_is_forbidden(self, tenant_client): + async with tenant_client("member") as me: + assert (await me.client.get(CURRENT)).status_code == 403 + assert (await me.client.put(f"{CURRENT}/{KEY}", json={"value": "x"})).status_code == 403 + + async def test_no_active_tenant_is_forbidden(self, authenticated_client): + # The platform admin passes the permission guard but acts for no tenant. + assert (await authenticated_client.get(CURRENT)).status_code == 403 + resp = await authenticated_client.put(f"{CURRENT}/{KEY}", json={"value": "x"}) + assert resp.status_code == 403 + + async def test_declared_type_wins(self, tenant_client): + async with tenant_client() as me: + bad = await me.client.put( + f"{CURRENT}/{COUNT}", json={"value": "abc", "value_type": "string"} + ) + ok = await me.client.put(f"{CURRENT}/{COUNT}", json={"value": "5"}) + assert bad.status_code == 422 + assert ok.json()["value_type"] == "int" + + async def test_check_hook(self, tenant_client): + async with tenant_client() as me: + url = f"{CURRENT}/{CHECKED}" + assert (await me.client.put(url, json={"value": "missing"})).status_code == 404 + assert (await me.client.put(url, json={"value": "bad"})).status_code == 422 + assert (await me.client.put(url, json={"value": "fine"})).status_code == 200 + + +class TestEffectiveValue: + async def test_precedence_default_then_system_then_tenant(self, app, tenant_client): + async with tenant_client() as me: + row = _row((await me.client.get(CURRENT)).json(), KEY) + assert (row["inherited"], row["value"], row["effective"]) == ("hello", None, "hello") + + await _system(app, KEY, "sys") + row = _row((await me.client.get(CURRENT)).json(), KEY) + assert (row["inherited"], row["effective"]) == ("sys", "sys") + + await me.client.put(f"{CURRENT}/{KEY}", json={"value": "mine"}) + row = _row((await me.client.get(CURRENT)).json(), KEY) + assert (row["inherited"], row["value"], row["effective"]) == ("sys", "mine", "mine") + + async def test_listing_offers_only_overridable_keys(self, tenant_client): + async with tenant_client() as me: + keys = {r["key"] for r in (await me.client.get(CURRENT)).json()} + assert {KEY, COUNT, CHECKED} <= keys + assert LOCKED not in keys + + async def test_accessor_reads_the_active_tenant(self, app, tenant_client): + @app.get("/__greeting") + async def greeting(request: Request, settings: SettingsDep): + return {"value": await settings.get(KEY)} + + await _system(app, KEY, "sys") + async with tenant_client() as a, tenant_client() as b: + await a.client.put(f"{CURRENT}/{KEY}", json={"value": "mine"}) + assert (await a.client.get("/__greeting")).json() == {"value": "mine"} + assert (await b.client.get("/__greeting")).json() == {"value": "sys"} + + +class TestPlatformScopeIdValidation: + async def test_unknown_tenant_is_404_on_get_and_put(self, authenticated_client): + url = f"/api/settings/tenant/no-such-tenant/{KEY}" + assert (await authenticated_client.put(url, json={"value": "x"})).status_code == 404 + assert (await authenticated_client.get(url)).status_code == 404 + + async def test_unknown_tenant_in_a_create_body_is_422(self, authenticated_client): + resp = await authenticated_client.post( + "/api/settings/", + json={"scope": "tenant", "scope_id": "no-such-tenant", "key": "k", "value": "v"}, + ) + assert resp.status_code == 422 + + async def test_delete_stays_unvalidated(self, authenticated_client): + # Nothing stored, so 404 for the *row* — not a refusal of the id. + resp = await authenticated_client.delete(f"/api/settings/tenant/gone/{KEY}") + assert resp.status_code == 404 + assert resp.json()["detail"] == "Setting not found" + + async def test_real_tenant_is_accepted_and_check_runs( + self, authenticated_client, tenant_client + ): + async with tenant_client() as t: + url = f"/api/settings/tenant/{t.tenant_id}" + ok = await authenticated_client.put(f"{url}/{LOCKED}", json={"value": "v"}) + refused = await authenticated_client.put(f"{url}/{CHECKED}", json={"value": "missing"}) + # Platform operators may write non-overridable keys at tenant scope. + assert ok.status_code == 200 + assert refused.status_code == 404 + + async def test_the_edit_form_runs_the_check_on_a_tenant_row( + self, app, authenticated_client, tenant_client + ): + """``PUT /admin/settings/{id}`` is a second door onto a TENANT row: it + must refuse what the create form and the JSON routes refuse.""" + async with tenant_client() as t: + async with app.state.sm.db.session_factory() as db: + row = await SettingService(db).upsert_scoped( + SettingScope.TENANT, t.tenant_id, CHECKED, SettingUpsert(value="fine") + ) + await db.commit() + url = f"/admin/settings/{row.id}" + back = {"Referer": f"http://testserver{url}/edit"} + + refused = await authenticated_client.put(url, json={"value": "bad"}, headers=back) + assert refused.status_code == 303 + assert refused.headers["location"].endswith(f"{url}/edit") + assert await _tenant_value(app, t.tenant_id, CHECKED) == "fine" + + ok = await authenticated_client.put(url, json={"value": "better"}, headers=back) + assert ok.headers["location"].endswith("/admin/settings/store") + assert await _tenant_value(app, t.tenant_id, CHECKED) == "better" + + +class TestInvalidation: + async def test_writes_publish_per_tenant_and_key_after_commit(self, app, tenant_client): + seen: list[tuple[str | None, str | None]] = [] + app.state.sm.invalidation.subscribe( + INVALIDATION_CHANNEL, lambda inv: seen.append(parse_invalidation_key(inv.key)) + ) + async with tenant_client() as me: + await me.client.put(f"{CURRENT}/{KEY}", json={"value": "x"}) + await me.client.delete(f"{CURRENT}/{KEY}") + await me.client.put(f"{CURRENT}/{LOCKED}", json={"value": "x"}) # 422: no write + assert seen == [(me.tenant_id, KEY), (me.tenant_id, KEY)] + + async def test_system_writes_publish_without_a_tenant(self, 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)) + ) + await authenticated_client.put(f"/api/settings/system/{KEY}", json={"value": "x"}) + assert seen == [(None, KEY)] diff --git a/modules/settings/tests/test_upload_key_delete.py b/modules/settings/tests/test_upload_key_delete.py new file mode 100644 index 00000000..e0f4e702 --- /dev/null +++ b/modules/settings/tests/test_upload_key_delete.py @@ -0,0 +1,152 @@ +"""A key set by upload is not cleared by a bare row delete (ship review). + +A generic delete would drop the setting row but leave the stored file behind; +the key's ``clear_via`` route is what reaps it. The rule lives in +``SettingService`` and holds for every scope and every caller: the API routes, +the Inertia admin route and a direct service call. The one exception is a +TENANT row left by a deleted tenant, which a platform operator may clear. +""" + +from __future__ import annotations + +import pytest +from settings._managed_keys import ManagedKeyError +from settings.constants import SYSTEM_SCOPE_ID +from settings.contracts.registry import SettingDefinition +from settings.contracts.schemas import SettingScope, SettingUpsert +from settings.service import SettingService + +KEY = "demo.logo" +UPLOAD = "/api/demo/tenant/logo" +INERTIA = {"X-Inertia": "true", "X-Inertia-Version": ""} + + +@pytest.fixture(autouse=True) +def definitions(app): + app.state.settings.registry.add( + SettingDefinition(key=KEY, tenant_overridable=True, clear_via=UPLOAD) + ) + + +async def _seed(app, scope: SettingScope, scope_id: str) -> int: + async with app.state.sm.db.session_factory() as db: + row = await SettingService(db).upsert_scoped( + scope, scope_id, KEY, SettingUpsert(value="file-1") + ) + await db.commit() + return row.id + + +async def _exists(app, scope: SettingScope, scope_id: str) -> bool: + async with app.state.sm.db.session_factory() as db: + return await SettingService(db).get_scoped(scope, scope_id, KEY) is not None + + +async def test_self_service_delete_is_refused_and_row_kept(app, tenant_client): + async with tenant_client() as t: + await _seed(app, SettingScope.TENANT, t.tenant_id) + resp = await t.client.delete(f"/api/settings/tenant/current/{KEY}") + assert resp.status_code == 422 + assert UPLOAD in resp.json()["detail"] + assert await _exists(app, SettingScope.TENANT, t.tenant_id) + + +async def test_platform_delete_is_refused_for_a_live_tenant( + app, authenticated_client, tenant_client +): + async with tenant_client() as t: + await _seed(app, SettingScope.TENANT, t.tenant_id) + resp = await authenticated_client.delete(f"/api/settings/tenant/{t.tenant_id}/{KEY}") + assert resp.status_code == 422 + assert await _exists(app, SettingScope.TENANT, t.tenant_id) + + +async def test_api_delete_by_id_is_refused_for_tenant_and_system_rows( + app, authenticated_client, tenant_client +): + async with tenant_client() as t: + tenant_row = await _seed(app, SettingScope.TENANT, t.tenant_id) + system_row = await _seed(app, SettingScope.SYSTEM, SYSTEM_SCOPE_ID) + for row_id in (tenant_row, system_row): + resp = await authenticated_client.delete(f"/api/settings/{row_id}") + assert resp.status_code == 422 + assert UPLOAD in resp.json()["detail"] + assert await _exists(app, SettingScope.TENANT, t.tenant_id) + assert await _exists(app, SettingScope.SYSTEM, SYSTEM_SCOPE_ID) + + +async def test_inertia_admin_delete_is_refused_for_tenant_and_system_rows( + app, authenticated_client, tenant_client +): + async with tenant_client() as t: + tenant_row = await _seed(app, SettingScope.TENANT, t.tenant_id) + system_row = await _seed(app, SettingScope.SYSTEM, SYSTEM_SCOPE_ID) + for row_id in (tenant_row, system_row): + resp = await authenticated_client.delete( + f"/admin/settings/{row_id}", headers=INERTIA, follow_redirects=False + ) + assert resp.status_code in (302, 303) + assert await _exists(app, SettingScope.TENANT, t.tenant_id) + assert await _exists(app, SettingScope.SYSTEM, SYSTEM_SCOPE_ID) + + +async def test_inertia_refusal_carries_a_translated_error(app, authenticated_client): + row_id = await _seed(app, SettingScope.SYSTEM, SYSTEM_SCOPE_ID) + resp = await authenticated_client.delete( + f"/admin/settings/{row_id}", + headers={**INERTIA, "Referer": "http://test/admin/settings/store"}, + ) + # The error rides the session to the next page render, as for any form. + page = await authenticated_client.get("/admin/settings/store") + assert "is managed elsewhere" in page.text, resp.status_code + + +async def test_direct_service_call_is_refused(app, tenant_client): + async with tenant_client() as t: + tenant_row = await _seed(app, SettingScope.TENANT, t.tenant_id) + await _seed(app, SettingScope.SYSTEM, SYSTEM_SCOPE_ID) + registry = app.state.settings.registry + async with app.state.sm.db.session_factory() as db: + service = SettingService(db, registry=registry) + with pytest.raises(ManagedKeyError) as err: + await service.delete(tenant_row) + assert err.value.clear_via == UPLOAD + with pytest.raises(ManagedKeyError): + await service.delete_scoped(SettingScope.SYSTEM, SYSTEM_SCOPE_ID, KEY) + # The owner, having a reap to run, may stand the guard down. + assert await service.delete_scoped( + SettingScope.SYSTEM, SYSTEM_SCOPE_ID, KEY, as_owner=True + ) + + +async def test_platform_may_clear_a_row_left_by_a_deleted_tenant(app, authenticated_client): + row_id = await _seed(app, SettingScope.TENANT, "gone-tenant") + resp = await authenticated_client.delete(f"/api/settings/tenant/gone-tenant/{KEY}") + assert resp.status_code == 204 + assert not await _exists(app, SettingScope.TENANT, "gone-tenant") + row_id = await _seed(app, SettingScope.TENANT, "gone-tenant") + resp = await authenticated_client.delete(f"/api/settings/{row_id}") + assert resp.status_code == 204 + + +async def test_plain_keys_still_delete(app, authenticated_client, tenant_client): + app.state.settings.registry.add(SettingDefinition(key="demo.plain", tenant_overridable=True)) + async with tenant_client() as t: + await t.client.put("/api/settings/tenant/current/demo.plain", json={"value": "x"}) + assert (await t.client.delete("/api/settings/tenant/current/demo.plain")).status_code == 204 + async with app.state.sm.db.session_factory() as db: + service = SettingService(db) + first = await service.upsert_scoped( + SettingScope.SYSTEM, SYSTEM_SCOPE_ID, "demo.plain", SettingUpsert(value="y") + ) + second = await service.upsert_scoped( + SettingScope.USER, "u1", "demo.plain", SettingUpsert(value="z") + ) + await db.commit() + assert (await authenticated_client.delete(f"/api/settings/{first.id}")).status_code == 204 + resp = await authenticated_client.delete( + f"/admin/settings/{second.id}", headers=INERTIA, follow_redirects=False + ) + assert resp.status_code in (302, 303) + async with app.state.sm.db.session_factory() as db: + assert await SettingService(db).get_by_id(second.id) is None diff --git a/modules/tenants/tenants/components/TenantSettingRow.tsx b/modules/tenants/tenants/components/TenantSettingRow.tsx new file mode 100644 index 00000000..05b0fa7f --- /dev/null +++ b/modules/tenants/tenants/components/TenantSettingRow.tsx @@ -0,0 +1,97 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { Badge } from '@simple-module-py/ui/components/ui/badge'; +import { Button } from '@simple-module-py/ui/components/ui/button'; +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 { detail } from './apiDetail'; + +export interface TenantSetting { + key: string; + description: string; + value_type: 'string' | 'bool' | 'int' | 'float' | 'json'; + inherited: string; + value: string | null; + effective: string; +} + +interface Props { + setting: TenantSetting; + onChanged: () => void; +} + +const API = '/api/settings/tenant/current'; + +/** One overridable key: its inherited value, the organisation's override, save/reset. */ +export function TenantSettingRow({ setting, onChanged }: Props) { + const { t } = useT(); + const [draft, setDraft] = useState(setting.value ?? ''); + const [busy, setBusy] = useState(false); + const overridden = setting.value !== null; + const inputId = `tenant-setting-${setting.key}`; + + async function send(method: 'PUT' | 'DELETE') { + setBusy(true); + try { + const response = await fetch(`${API}/${encodeURIComponent(setting.key)}`, { + method, + credentials: 'same-origin', + headers: method === 'PUT' ? { 'Content-Type': 'application/json' } : undefined, + body: method === 'PUT' ? JSON.stringify({ value: draft }) : undefined, + }); + if (!response.ok) { + toast.error((await detail(response)) ?? t(keys.tenants.settings.toast_failed)); + return; + } + toast.success( + t(method === 'PUT' ? keys.tenants.settings.toast_saved : keys.tenants.settings.toast_reset), + ); + onChanged(); + } catch { + toast.error(t(keys.tenants.settings.toast_failed)); + } finally { + setBusy(false); + } + } + + return ( +
+
+ + + {t(overridden ? keys.tenants.settings.overridden : keys.tenants.settings.inherited)} + +
+ {setting.description && ( +

{setting.description}

+ )} +
+ setDraft(event.target.value)} + disabled={busy} + /> +
+ + {overridden && ( + + )} +
+
+

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

+
+ ); +} diff --git a/modules/tenants/tenants/components/apiDetail.ts b/modules/tenants/tenants/components/apiDetail.ts new file mode 100644 index 00000000..32eb32db --- /dev/null +++ b/modules/tenants/tenants/components/apiDetail.ts @@ -0,0 +1,5 @@ +/** The `detail` string of a FastAPI error response, or `null` when it has none. */ +export async function detail(response: Response): Promise { + const data = await response.json().catch(() => ({}) as Record); + return typeof data.detail === 'string' ? data.detail : null; +} diff --git a/modules/tenants/tenants/constants.py b/modules/tenants/tenants/constants.py index b133702b..640a1872 100644 --- a/modules/tenants/tenants/constants.py +++ b/modules/tenants/tenants/constants.py @@ -43,18 +43,23 @@ class TenantStatus(StrEnum): # (wildcard) or an explicit role grant. PERM_MEMBERS_VIEW = "tenants.members.view" PERM_MEMBERS_MANAGE = "tenants.members.manage" -PERM_SETTINGS_MANAGE = "tenants.settings.manage" +# Owned and mapped by ``settings`` (#382); named here for the page guard and +# the sidebar entry only. +PERM_TENANT_SETTINGS = "settings.tenant.edit" PERM_PLATFORM_VIEW = "tenants.platform.view" PERM_PLATFORM_MANAGE = "tenants.platform.manage" ROLE_PERMISSIONS: dict[MembershipRole, list[str]] = { - MembershipRole.OWNER: [PERM_MEMBERS_VIEW, PERM_MEMBERS_MANAGE, PERM_SETTINGS_MANAGE], + # Tenant settings are ``settings.tenant.edit``, which the settings module + # owns and maps onto owner + admin itself (#382). + MembershipRole.OWNER: [PERM_MEMBERS_VIEW, PERM_MEMBERS_MANAGE], MembershipRole.ADMIN: [PERM_MEMBERS_VIEW, PERM_MEMBERS_MANAGE], MembershipRole.MEMBER: [PERM_MEMBERS_VIEW], } PAGE_INDEX = "Tenants/Index" PAGE_MEMBERS = "Tenants/Members" +PAGE_SETTINGS = "Tenants/Settings" PAGE_ACCEPT = "Tenants/AcceptInvitation" PAGE_ADMIN = "Tenants/AdminBrowse" diff --git a/modules/tenants/tenants/endpoints/views.py b/modules/tenants/tenants/endpoints/views.py index 40b728cf..9a388942 100644 --- a/modules/tenants/tenants/endpoints/views.py +++ b/modules/tenants/tenants/endpoints/views.py @@ -19,9 +19,11 @@ PAGE_ACCEPT, PAGE_INDEX, PAGE_MEMBERS, + PAGE_SETTINGS, PERM_MEMBERS_MANAGE, PERM_MEMBERS_VIEW, PERM_PLATFORM_MANAGE, + PERM_TENANT_SETTINGS, ) from tenants.deps import InvitationServiceDep, TenantServiceDep, UserIdDep from tenants.resolver import memberships_for @@ -95,6 +97,39 @@ async def members( ) +@router.get( + "/settings", + response_model=None, + dependencies=[Depends(RequiresPermission(PERM_TENANT_SETTINGS))], +) +async def tenant_settings( + request: Request, inertia: InertiaDep, service: TenantServiceDep +) -> InertiaResponse | RedirectResponse: + """The active organisation's overridable settings (#382). + + Rendered here, next to Members, because it is the organisation's page; the + rows and the writes belong to ``settings`` (``/api/settings/tenant/current``), + which acts on the active tenant only. + """ + from settings.deps import get_setting_service + from settings.tenant_scope import registry_of + from settings.tenant_view import list_for_tenant + + tenant_id = getattr(request.state, "tenant_id", None) + tenant = await service.get(tenant_id) if tenant_id else None + if tenant is None: + return RedirectResponse("/tenants/?reason=tenant_required", status_code=303) + settings = await get_setting_service(request, service.db) + rows = await list_for_tenant(settings, registry_of(request), tenant.id) + return await inertia.render( + PAGE_SETTINGS, + { + "tenant": {"id": tenant.id, "name": tenant.name, "slug": tenant.slug}, + "settings": [row.model_dump(mode="json") for row in rows], + }, + ) + + @router.get("/invitations/accept", response_model=None) async def accept_invitation( request: Request, inertia: InertiaDep, invitations: InvitationServiceDep, token: str = "" diff --git a/modules/tenants/tenants/locales/en.json b/modules/tenants/tenants/locales/en.json index 89894da5..158bbb04 100644 --- a/modules/tenants/tenants/locales/en.json +++ b/modules/tenants/tenants/locales/en.json @@ -2,7 +2,8 @@ "nav": { "organisations": "Organisations", "members": "Members", - "tenants": "Tenants" + "tenants": "Tenants", + "settings": "Organisation settings" }, "roles": { "owner": "Owner", @@ -135,5 +136,21 @@ "already_member": "That person is already a member of this organisation.", "tenant_manager_required": "Only an owner or admin of this organisation can do that.", "tenant_isolation": "That action is not allowed across organisations." + }, + "settings": { + "head_title": "Organisation settings", + "title": "Organisation settings", + "description": "Settings {name} can change for itself. Anything you leave alone keeps the platform's value.", + "empty_title": "Nothing to configure", + "empty_description": "The platform does not let organisations change any settings yet.", + "overridden": "Overridden", + "inherited": "Platform value", + "inherited_value": "Platform value: {value}", + "none": "(empty)", + "save": "Save", + "reset": "Reset to platform value", + "toast_saved": "Setting saved", + "toast_reset": "Setting reset to the platform value", + "toast_failed": "Could not save the setting" } } diff --git a/modules/tenants/tenants/module.py b/modules/tenants/tenants/module.py index 747d2875..0190c370 100644 --- a/modules/tenants/tenants/module.py +++ b/modules/tenants/tenants/module.py @@ -115,6 +115,17 @@ def register_menu_items(self, registry: MenuRegistry) -> None: permissions=[c.PERM_MEMBERS_VIEW], ) ) + registry.add( + MenuItem( + label="Organisation settings", + label_key="tenants.nav.settings", + url="/tenants/settings", + icon="settings", + order=92, + section=MenuSection.SIDEBAR, + permissions=[c.PERM_TENANT_SETTINGS], + ) + ) registry.add( MenuItem( label="Tenants", @@ -135,7 +146,6 @@ def register_permissions(self, registry: PermissionRegistry) -> None: [ c.PERM_MEMBERS_VIEW, c.PERM_MEMBERS_MANAGE, - c.PERM_SETTINGS_MANAGE, c.PERM_PLATFORM_VIEW, c.PERM_PLATFORM_MANAGE, ], diff --git a/modules/tenants/tenants/pages/Settings.tsx b/modules/tenants/tenants/pages/Settings.tsx new file mode 100644 index 00000000..431c6b4a --- /dev/null +++ b/modules/tenants/tenants/pages/Settings.tsx @@ -0,0 +1,51 @@ +import { Head, router, usePage } from '@inertiajs/react'; +import { keys, useT } from '@simple-module-py/i18n'; +import { EmptyState } from '@simple-module-py/ui/components/EmptyState'; +import { PageShell } from '@simple-module-py/ui/components/PageShell'; +import { Card } from '@simple-module-py/ui/components/ui/card'; +import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import { Settings2 } from 'lucide-react'; +import { type TenantSetting, TenantSettingRow } from '../components/TenantSettingRow'; + +interface Props { + tenant: { id: string; name: string; slug: string }; + settings: TenantSetting[]; +} + +/** The active organisation's overrides of the keys the platform lets it change. */ +function Settings() { + const { tenant, settings } = usePage<{ props: Props }>().props as unknown as Props; + const { t } = useT(); + + return ( + <> + + + {settings.length === 0 ? ( + + ) : ( + + {settings.map((setting) => ( + router.reload()} + /> + ))} + + )} + + + ); +} + +Settings.layout = [AuthenticatedLayout]; +export default Settings; diff --git a/modules/tenants/tests-js/TenantSettingRow.test.tsx b/modules/tenants/tests-js/TenantSettingRow.test.tsx new file mode 100644 index 00000000..d852051b --- /dev/null +++ b/modules/tenants/tests-js/TenantSettingRow.test.tsx @@ -0,0 +1,66 @@ +/** + * The organisation settings row writes through the active-tenant endpoint — + * never a URL carrying a tenant id — and only offers "reset" when the tenant + * actually holds an override (#382). + */ +import '@testing-library/jest-dom/vitest'; +import { configureI18n } from '@simple-module-py/i18n'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { afterEach, describe, expect, test, vi } from 'vitest'; +import { type TenantSetting, TenantSettingRow } from '../tenants/components/TenantSettingRow'; + +configureI18n({ + locale: 'en', + messages: { + 'tenants.settings.save': 'Save', + 'tenants.settings.reset': 'Reset to platform value', + 'tenants.settings.overridden': 'Overridden', + 'tenants.settings.inherited': 'Platform value', + 'tenants.settings.inherited_value': 'Platform value: {value}', + 'tenants.settings.none': '(empty)', + 'tenants.settings.toast_saved': 'Setting saved', + 'tenants.settings.toast_reset': 'Setting reset', + 'tenants.settings.toast_failed': 'Could not save', + }, +}); + +const base: TenantSetting = { + key: 'demo.motto', + description: 'A motto', + value_type: 'string', + inherited: 'hi', + value: null, + effective: 'hi', +}; + +afterEach(() => vi.restoreAllMocks()); + +describe('TenantSettingRow', () => { + test('saving PUTs the draft to the current-tenant endpoint', async () => { + const fetchMock = vi + .spyOn(globalThis, 'fetch') + .mockResolvedValue(new Response('{}', { status: 200 })); + const onChanged = vi.fn(); + render(); + + await userEvent.type(screen.getByLabelText('demo.motto'), 'ours'); + await userEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(fetchMock).toHaveBeenCalledWith( + '/api/settings/tenant/current/demo.motto', + expect.objectContaining({ method: 'PUT', body: JSON.stringify({ value: 'ours' }) }), + ); + expect(onChanged).toHaveBeenCalled(); + }); + + test('reset is offered only for an override', () => { + const { rerender } = render( {}} />); + expect(screen.queryByRole('button', { name: 'Reset to platform value' })).toBeNull(); + expect(screen.getByText('Platform value: hi')).toBeInTheDocument(); + + rerender( {}} />); + expect(screen.getByRole('button', { name: 'Reset to platform value' })).toBeInTheDocument(); + expect(screen.getByText('Overridden')).toBeInTheDocument(); + }); +}); diff --git a/modules/tenants/tests-js/apiDetail.test.ts b/modules/tenants/tests-js/apiDetail.test.ts new file mode 100644 index 00000000..fb9f9908 --- /dev/null +++ b/modules/tenants/tests-js/apiDetail.test.ts @@ -0,0 +1,14 @@ +import { describe, expect, test } from 'vitest'; +import { detail } from '../tenants/components/apiDetail'; + +describe('detail', () => { + test('returns a string detail', async () => { + const response = new Response(JSON.stringify({ detail: 'nope' }), { status: 422 }); + expect(await detail(response)).toBe('nope'); + }); + + test('is null for a non-string detail or a non-JSON body', async () => { + expect(await detail(new Response(JSON.stringify({ detail: [1] })))).toBeNull(); + expect(await detail(new Response(''))).toBeNull(); + }); +}); diff --git a/modules/tenants/tests/test_settings_page.py b/modules/tenants/tests/test_settings_page.py new file mode 100644 index 00000000..209103b6 --- /dev/null +++ b/modules/tenants/tests/test_settings_page.py @@ -0,0 +1,45 @@ +"""``/tenants/settings``: the active organisation's overridable settings (#382).""" + +from __future__ import annotations + +import pytest +from settings.contracts.registry import SettingDefinition + +INERTIA = {"X-Inertia": "true", "Accept": "application/json"} + + +@pytest.fixture(autouse=True) +def overridable(app): + app.state.settings.registry.add( + SettingDefinition(key="demo.motto", default="hi", tenant_overridable=True) + ) + + +async def test_owner_sees_the_overridable_keys(tenant_client): + async with tenant_client() as me: + await me.client.put("/api/settings/tenant/current/demo.motto", json={"value": "ours"}) + resp = await me.client.get("/tenants/settings", headers=INERTIA) + assert resp.status_code == 200, resp.text + page = resp.json() + assert page["component"] == "Tenants/Settings" + assert page["props"]["tenant"]["id"] == me.tenant_id + row = next(r for r in page["props"]["settings"] if r["key"] == "demo.motto") + assert (row["inherited"], row["value"], row["effective"]) == ("hi", "ours", "ours") + + +async def test_member_is_forbidden(tenant_client): + async with tenant_client("member") as me: + resp = await me.client.get("/tenants/settings", headers=INERTIA) + assert resp.status_code == 403 + + +async def test_sidebar_entry_follows_the_permission(tenant_client): + async with tenant_client("admin") as admin, tenant_client("member") as member: + admin_menu = (await admin.client.get("/tenants/", headers=INERTIA)).json() + member_menu = (await member.client.get("/tenants/", headers=INERTIA)).json() + + def urls(page: dict) -> set[str]: + return {item["url"] for item in page["props"]["menus"].get("sidebar", [])} + + assert "/tenants/settings" in urls(admin_menu) + assert "/tenants/settings" not in urls(member_menu) diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index c7f94503..26e45c74 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -589,7 +589,9 @@ export default { 'settings.audit.setting': '', 'settings.browse.delete_confirm_button': '', 'settings.browse.delete_description': '', + 'settings.browse.delete_failed': '', 'settings.browse.delete_link': '', + 'settings.browse.delete_managed_error': '', 'settings.browse.delete_title': '', 'settings.browse.description': '', 'settings.browse.edit_link': '', @@ -812,10 +814,25 @@ export default { 'tenants.members.you_suffix': '', 'tenants.nav.members': '', 'tenants.nav.organisations': '', + 'tenants.nav.settings': '', 'tenants.nav.tenants': '', 'tenants.roles.admin': '', 'tenants.roles.member': '', 'tenants.roles.owner': '', + 'tenants.settings.description': '', + 'tenants.settings.empty_description': '', + 'tenants.settings.empty_title': '', + 'tenants.settings.head_title': '', + 'tenants.settings.inherited': '', + 'tenants.settings.inherited_value': '', + 'tenants.settings.none': '', + 'tenants.settings.overridden': '', + 'tenants.settings.reset': '', + 'tenants.settings.save': '', + 'tenants.settings.title': '', + 'tenants.settings.toast_failed': '', + 'tenants.settings.toast_reset': '', + 'tenants.settings.toast_saved': '', '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 2db30f53..ecd037da 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -765,7 +765,9 @@ export const keys = { browse: { delete_confirm_button: 'settings.browse.delete_confirm_button', delete_description: 'settings.browse.delete_description', + delete_failed: 'settings.browse.delete_failed', delete_link: 'settings.browse.delete_link', + delete_managed_error: 'settings.browse.delete_managed_error', delete_title: 'settings.browse.delete_title', description: 'settings.browse.description', edit_link: 'settings.browse.edit_link', @@ -1029,6 +1031,7 @@ export const keys = { nav: { members: 'tenants.nav.members', organisations: 'tenants.nav.organisations', + settings: 'tenants.nav.settings', tenants: 'tenants.nav.tenants', }, roles: { @@ -1036,6 +1039,22 @@ export const keys = { member: 'tenants.roles.member', owner: 'tenants.roles.owner', }, + settings: { + description: 'tenants.settings.description', + empty_description: 'tenants.settings.empty_description', + empty_title: 'tenants.settings.empty_title', + head_title: 'tenants.settings.head_title', + inherited: 'tenants.settings.inherited', + inherited_value: 'tenants.settings.inherited_value', + none: 'tenants.settings.none', + overridden: 'tenants.settings.overridden', + reset: 'tenants.settings.reset', + save: 'tenants.settings.save', + title: 'tenants.settings.title', + toast_failed: 'tenants.settings.toast_failed', + toast_reset: 'tenants.settings.toast_reset', + toast_saved: 'tenants.settings.toast_saved', + }, status: { active: 'tenants.status.active', suspended: 'tenants.status.suspended', diff --git a/packages/ui/src/layouts/AuthenticatedLayout.tsx b/packages/ui/src/layouts/AuthenticatedLayout.tsx index a823ed79..3086e768 100644 --- a/packages/ui/src/layouts/AuthenticatedLayout.tsx +++ b/packages/ui/src/layouts/AuthenticatedLayout.tsx @@ -1,4 +1,3 @@ -import { Toaster } from '@simple-module-py/ui/components/ui/sonner'; import type React from 'react'; import { DEFAULT_SIDEBAR_THEME, SidebarLayout } from './SidebarLayout'; @@ -14,7 +13,6 @@ export function AuthenticatedLayout({ children }: { children: React.ReactNode }) // the wordmark, where it read as part of the branding rather than a setting. {children} - ); } diff --git a/packages/ui/src/layouts/SidebarLayout.toast.test.tsx b/packages/ui/src/layouts/SidebarLayout.toast.test.tsx new file mode 100644 index 00000000..8e64e7b8 --- /dev/null +++ b/packages/ui/src/layouts/SidebarLayout.toast.test.tsx @@ -0,0 +1,52 @@ +import { act, render, screen } from '@testing-library/react'; +import { describe, expect, it, vi } from 'vitest'; + +vi.mock('@inertiajs/react', () => ({ + usePage: () => ({ url: '/admin/settings', props: {} }), + Link: ({ + href, + children, + ...rest + }: { href: string; children: React.ReactNode } & Record) => ( + + {children} + + ), +})); + +vi.mock('next-themes', () => ({ useTheme: () => ({ theme: 'light' }) })); + +vi.mock('@simple-module-py/i18n', () => { + const keys = new Proxy({}, { get: () => new Proxy({}, { get: () => 'k' }) }); + return { useT: () => ({ t: (k: string) => k }), t: (k: string) => k, keys }; +}); + +vi.mock('../components/AppTopbar', () => ({ + AppTopbar: () => null, + activeSection: () => null, + findSection: () => null, +})); +vi.mock('../components/BrandingFooter', () => ({ BrandingFooter: () => null })); +vi.mock('../components/BrandingHead', () => ({ BrandingHead: () => null })); +vi.mock('../components/BrandingBanner', () => ({ BrandingBanner: () => null })); +vi.mock('../components/DemoBanner', () => ({ DemoBanner: () => null })); +vi.mock('./MobileBar', () => ({ MobileBar: () => null })); +vi.mock('./SidebarUserMenu', () => ({ SidebarUserMenu: () => null })); + +import { toast } from 'sonner'; +import { AdminLayout } from './AdminLayout'; + +describe('AdminLayout toasts', () => { + it('renders a toast fired from an admin page', async () => { + render( + +

page

+ , + ); + await act(async () => { + toast.error('Managed key refused'); + }); + expect(await screen.findByText('Managed key refused')).toBeTruthy(); + expect(document.querySelectorAll('[data-sonner-toaster]').length).toBe(1); + }); +}); diff --git a/packages/ui/src/layouts/SidebarLayout.tsx b/packages/ui/src/layouts/SidebarLayout.tsx index 000c7e72..457089e8 100644 --- a/packages/ui/src/layouts/SidebarLayout.tsx +++ b/packages/ui/src/layouts/SidebarLayout.tsx @@ -1,6 +1,7 @@ import { Link, usePage } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; import { Button } from '@simple-module-py/ui/components/ui/button'; +import { Toaster } from '@simple-module-py/ui/components/ui/sonner'; import { Tooltip, TooltipContent, @@ -268,6 +269,8 @@ function SidebarShell({ children, menuKey, theme, headerSlot, footerNavSlot }: S /> + {/* Mounted here, not in a concrete layout, so every sidebar shell (app and admin) can toast. */} + ); }