From 9bcf302f0ff9f651c23abbca39567d403871d381 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 30 Oct 2026 08:01:20 +0100 Subject: [PATCH 1/3] fix(settings,file_storage): module keys go through their editor; storage settings need a restart; Revert after save (#690, #691, #696) - Generic settings writes (/api/settings/system/{key}, CRUD by id, Browse actions) to a system-scope `.` row skipped validation, the env-pin guard and the hot reload. They now return 422 pointing at the module editor; tenant/user rows stay writable, and managed keys keep their own clear route. Browse no longer suggests module keys. - file_storage builds its backend once at startup; the backend, filesystem root and S3 fields are now marked restart-required instead of silently splitting the web process from the CSP, workers and tenant purge. - Saving a defaulted field in the module form now shows Revert at once. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 6 ++ docs/modules/file_storage.md | 10 +++ docs/modules/settings.md | 7 +- modules/file_storage/file_storage/health.py | 5 +- modules/file_storage/file_storage/settings.py | 24 +++--- .../test_file_storage_restart_settings.py | 73 +++++++++++++++++ modules/settings/settings/_managed_keys.py | 10 ++- modules/settings/settings/constants.py | 1 + modules/settings/settings/deps.py | 10 +++ modules/settings/settings/endpoints/api.py | 32 ++++---- modules/settings/settings/endpoints/views.py | 14 ++-- modules/settings/settings/errors.py | 10 ++- modules/settings/settings/known_keys.py | 59 +++----------- .../settings/pages/components/ModuleForm.tsx | 12 ++- modules/settings/settings/service.py | 23 +++++- .../settings_ModuleFormRevert.test.tsx | 43 ++++++++++ .../settings/tests/test_known_keys_meta.py | 74 +++++------------ .../test_settings_generic_module_guard.py | 80 +++++++++++++++++++ .../tests/test_settings_secret_urls.py | 22 ++--- 19 files changed, 364 insertions(+), 151 deletions(-) create mode 100644 modules/file_storage/tests/test_file_storage_restart_settings.py create mode 100644 modules/settings/tests/test_settings_generic_module_guard.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ed220b4ee..8373db74c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,12 @@ All notable changes to this project are documented in this file. The format is b validate explicit tenant literals and retain strict isolation and source scoping (#693). - Inertia: honor custom once-prop keys in client exclusions, skipping loaders and values on full and partial requests (#694). +- **Behaviour change:** generic settings API and Browse mutations reject registered + module keys and point to their validated editor; stop suggesting them (#690). +- Mark storage backend construction fields as requiring coordinated web/worker + restart; document multipart draining and connection-check behaviour (#691). +- Show Revert immediately after saving a defaulted module field while preserving + drafts and the saved confirmation (#696). ## [0.0.41] — 2026-10-25 diff --git a/docs/modules/file_storage.md b/docs/modules/file_storage.md index b975abfb7..c47c19945 100644 --- a/docs/modules/file_storage.md +++ b/docs/modules/file_storage.md @@ -10,6 +10,16 @@ The extras combine as `simple_module_file_storage[s3,tasks]`. Without `background_tasks`, uploads still work; completed multipart uploads keep `checksum_sha256` null. Streamed uploads compute their checksum inline. +Backend selection, filesystem root, and S3 bucket, region, credentials, endpoints +and signature version require a coordinated restart of web and worker processes +(#691). Saving these fields persists configuration; it does not move files or +rebuild the web backend. Drain or abort multipart uploads and pause storage traffic +and jobs before changing them, then restart every process before resuming. Workers +read stored settings per task and may otherwise use the new store before the web +process restarts. Existing files must be migrated separately when changing roots +or buckets. The connection check probes the currently active web backend, so it +continues to test the old configuration until restart. + ## ModuleMeta | Field | Value | diff --git a/docs/modules/settings.md b/docs/modules/settings.md index e8ff2eca0..3a127625c 100644 --- a/docs/modules/settings.md +++ b/docs/modules/settings.md @@ -7,6 +7,11 @@ Two distinct surfaces: 1. **Generic key/value settings** — anything addressable by a string `key`. Useful for arbitrary config you don't want to wedge into a pydantic class. Edited at `/admin/settings/store`. 2. **Per-module pydantic settings** — each module registers a `BaseSettings` subclass via `register_module_settings`. Hydrated from the DB at boot, edited at `/admin/settings/`, hot-swapped on save with a `SettingsReloaded` event so dependents (SMTP clients, Celery configs, …) can rebuild. +Generic store mutations reject keys prefixed by a registered module package with +HTTP 422 and a link to `/admin/settings/` (or its dedicated management +page). Use the module editor/API for validation, environment pinning and reload; +Browse autocomplete offers only declared generic keys (#690). + ## ModuleMeta | Field | Value | @@ -127,7 +132,7 @@ per-tenant value subscribes and forgets. ### Generic K/V API (`/api/settings/...`) -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. +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 generic keys at system scope or at any tenant's scope; registered module namespaces must use the module editor/API. 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 | |---|---| diff --git a/modules/file_storage/file_storage/health.py b/modules/file_storage/file_storage/health.py index 5a7b83c74..bc1527cfc 100644 --- a/modules/file_storage/file_storage/health.py +++ b/modules/file_storage/file_storage/health.py @@ -21,7 +21,10 @@ def build_backend_check(app: FastAPI): - """Return an async check closing over *app* so it re-reads the live backend.""" + """Return an async check closing over *app* that probes its startup backend. + + Storage settings are restart-required (#691), so that is the backend in use. + """ async def check() -> HealthCheckResult: services = getattr(app.state, "file_storage", None) diff --git a/modules/file_storage/file_storage/settings.py b/modules/file_storage/file_storage/settings.py index e25b5c191..e16f8c79d 100644 --- a/modules/file_storage/file_storage/settings.py +++ b/modules/file_storage/file_storage/settings.py @@ -39,27 +39,33 @@ class FileStorageSettings(DbBackedSettings): # reading each field from its bare name — GH #283. model_config = SettingsConfigDict(extra="ignore") - backend: str = constants.DEFAULT_BACKEND + backend: str = Field( + default=constants.DEFAULT_BACKEND, json_schema_extra={"requires_restart": True} + ) # Filesystem backend fs_root_path: str = Field( default=constants.DEFAULT_FS_ROOT, - json_schema_extra={"group": "Filesystem"}, + json_schema_extra={"group": "Filesystem", "requires_restart": True}, ) # S3-compatible backend (works with AWS S3, MinIO, R2, etc.) - s3_bucket: str = Field(default="", json_schema_extra={"group": "S3"}) - s3_region: str = Field(default="", json_schema_extra={"group": "S3"}) - s3_access_key_id: str = Field(default="", json_schema_extra={"group": "S3"}) - s3_secret_access_key: str = Field(default="", json_schema_extra={"group": "S3"}) + s3_bucket: str = Field(default="", json_schema_extra={"group": "S3", "requires_restart": True}) + s3_region: str = Field(default="", json_schema_extra={"group": "S3", "requires_restart": True}) + s3_access_key_id: str = Field( + default="", json_schema_extra={"group": "S3", "requires_restart": True} + ) + s3_secret_access_key: str = Field( + default="", json_schema_extra={"group": "S3", "requires_restart": True} + ) s3_endpoint_url: str = Field( default="", - json_schema_extra={"group": "S3"}, + json_schema_extra={"group": "S3", "requires_restart": True}, description="Custom endpoint for MinIO / R2. Blank uses AWS default.", ) s3_public_endpoint_url: str = Field( default="", - json_schema_extra={"group": "S3"}, + json_schema_extra={"group": "S3", "requires_restart": True}, description=( "Browser-facing S3 endpoint used to presign direct part uploads, when it " "differs from s3_endpoint_url (e.g. an internal MinIO hostname). Blank " @@ -68,7 +74,7 @@ class FileStorageSettings(DbBackedSettings): ) s3_signature_version: str = Field( default="s3v4", - json_schema_extra={"group": "S3"}, + json_schema_extra={"group": "S3", "requires_restart": True}, description="Presign signature version. Keep s3v4; SigV2 is rejected by most providers.", ) s3_presign_ttl_seconds: int = Field( diff --git a/modules/file_storage/tests/test_file_storage_restart_settings.py b/modules/file_storage/tests/test_file_storage_restart_settings.py new file mode 100644 index 000000000..1d5e152d1 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_restart_settings.py @@ -0,0 +1,73 @@ +"""Storage location changes require coordinated restarts (#691).""" + +import pytest + + +@pytest.mark.parametrize( + "name", + [ + "backend", + "fs_root_path", + "s3_bucket", + "s3_region", + "s3_access_key_id", + "s3_secret_access_key", + "s3_endpoint_url", + "s3_public_endpoint_url", + "s3_signature_version", + ], +) +def test_backend_construction_fields_advertise_restart(name): + from file_storage.settings import FileStorageSettings + from settings._module_settings import _field_view + + field = _field_view(name, FileStorageSettings(), "SM_FILE_STORAGE_") + assert field.requires_restart is True + + +async def test_health_check_reads_current_backend(tmp_path): + from types import SimpleNamespace + + from fastapi import FastAPI + from file_storage.backends.filesystem import FilesystemBackend + from file_storage.health import build_backend_check + from simple_module_core.health import HealthStatus + + app = FastAPI() + app.state.file_storage = SimpleNamespace(backend=None) + check = build_backend_check(app) + assert (await check()).status == HealthStatus.UNHEALTHY + app.state.file_storage.backend = FilesystemBackend(root=tmp_path) + assert (await check()).status == HealthStatus.HEALTHY + + +async def test_root_change_applies_after_backend_restart( + authenticated_client, storage, tmp_path, checksum_calls +): + from file_storage.backends import build_backend + from file_storage.backends.filesystem import FilesystemBackend + + old_root = tmp_path / "old" + new_root = tmp_path / "new" + storage.settings.fs_root_path = str(old_root) + storage.backend = FilesystemBackend(root=old_root) + original_backend = storage.backend + response = await authenticated_client.put( + "/api/settings/modules/file_storage", json={"fs_root_path": str(new_root)} + ) + assert response.status_code == 200, response.text + assert storage.backend is original_backend + response = await authenticated_client.post( + "/api/file-storage/upload", files={"file": ("before.txt", b"before restart", "text/plain")} + ) + assert response.status_code == 201, response.text + assert [p.read_bytes() for p in old_root.rglob("*.txt")] == [b"before restart"] + assert not new_root.exists() + + # Simulate startup constructing the backend from the newly hydrated settings. + storage.backend = build_backend(storage.settings) + response = await authenticated_client.post( + "/api/file-storage/upload", files={"file": ("after.txt", b"after restart", "text/plain")} + ) + assert response.status_code == 201, response.text + assert [p.read_bytes() for p in new_root.rglob("*.txt")] == [b"after restart"] diff --git a/modules/settings/settings/_managed_keys.py b/modules/settings/settings/_managed_keys.py index 391ab1a41..d66111dc2 100644 --- a/modules/settings/settings/_managed_keys.py +++ b/modules/settings/settings/_managed_keys.py @@ -22,7 +22,7 @@ from collections.abc import Awaitable, Callable -from settings.constants import ERR_MANAGED_KEY_DELETE +from settings.constants import ERR_MANAGED_KEY_DELETE, ERR_MODULE_KEY_EDIT from settings.contracts.registry import SettingsRegistry, clear_route from settings.contracts.schemas import SettingScope from settings.exceptions import SettingsError @@ -58,3 +58,11 @@ async def ensure_deletable( if orphaned: return raise ManagedKeyError(key, clear_route(definition, scope)) + + +class ModuleKeyError(SettingsError): + """A generic mutation would bypass the module editor's validation.""" + + def __init__(self, package: str, edit_via: str) -> None: + self.edit_via = edit_via + super().__init__(ERR_MODULE_KEY_EDIT.format(package=package, edit_via=edit_via)) diff --git a/modules/settings/settings/constants.py b/modules/settings/settings/constants.py index 7f7c1777d..bcce41550 100644 --- a/modules/settings/settings/constants.py +++ b/modules/settings/settings/constants.py @@ -145,6 +145,7 @@ 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_MODULE_KEY_EDIT: Final = "Module settings must be edited at {edit_via} (package: {package})" ERR_NOT_TENANT_OVERRIDABLE: Final = "This setting cannot be changed per organisation" # ── HTTP ───────────────────────────────────────────────────────────── diff --git a/modules/settings/settings/deps.py b/modules/settings/settings/deps.py index 85a09261e..a561ec9cf 100644 --- a/modules/settings/settings/deps.py +++ b/modules/settings/settings/deps.py @@ -12,6 +12,7 @@ from fastapi import Depends, Request from simple_module_core.app_state import get_services +from simple_module_core.module_settings import get_module_settings_registry from simple_module_db.deps import get_db from sqlalchemy.ext.asyncio import AsyncSession @@ -36,6 +37,15 @@ async def tenant_is_live(tenant_id: str) -> bool: ) +async def get_generic_setting_service( + request: Request, + service: SettingService = Depends(get_setting_service), +) -> SettingService: + """Raw store routes must send module configuration through its own editor.""" + service.module_registry = get_module_settings_registry(request.app) + return service + + def get_settings_registry(request: Request) -> SettingsRegistry: """Return the app-wide settings registry populated during boot.""" services = getattr(request.app.state, MODULE_PACKAGE) diff --git a/modules/settings/settings/endpoints/api.py b/modules/settings/settings/endpoints/api.py index 4a4e0ada3..18e87cecc 100644 --- a/modules/settings/settings/endpoints/api.py +++ b/modules/settings/settings/endpoints/api.py @@ -37,7 +37,7 @@ SettingUpdate, SettingUpsert, ) -from settings.deps import get_setting_service +from settings.deps import get_generic_setting_service from settings.service import SettingService from settings.tenant_scope import ( is_known_tenant, @@ -69,7 +69,7 @@ def _not_found() -> HTTPException: async def list_settings( scope: SettingScope | None = Query(default=None, alias=QP_SCOPE), scope_id: str = Query(default=SYSTEM_SCOPE_ID, alias=QP_SCOPE_ID), - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> list[SettingOut]: if scope is None: return await service.list_all() @@ -84,7 +84,7 @@ async def resolve_setting( key: str, user_id: str | None = Query(default=None, alias=QP_USER_ID), tenant_id: str | None = Query(default=None, alias=QP_TENANT_ID), - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> SettingOut: result = await service.resolve(key, user_id=user_id, tenant_id=tenant_id) if result is None: @@ -97,7 +97,7 @@ async def resolve_setting( @router.get(API_SYSTEM_PATH, response_model=SettingOut, dependencies=_VIEW) async def get_system_setting( - key: str, service: SettingService = Depends(get_setting_service) + key: str, service: SettingService = Depends(get_generic_setting_service) ) -> SettingOut: result = await service.get_scoped(SettingScope.SYSTEM, SYSTEM_SCOPE_ID, key) if result is None: @@ -109,14 +109,14 @@ async def get_system_setting( async def upsert_system_setting( key: str, data: SettingUpsert, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> SettingOut: return await service.upsert_scoped(SettingScope.SYSTEM, SYSTEM_SCOPE_ID, key, data) @router.delete(API_SYSTEM_PATH, status_code=STATUS_NO_CONTENT, dependencies=_DELETE) async def delete_system_setting( - key: str, service: SettingService = Depends(get_setting_service) + key: str, service: SettingService = Depends(get_generic_setting_service) ) -> None: if not await service.delete_scoped(SettingScope.SYSTEM, SYSTEM_SCOPE_ID, key): raise _not_found() @@ -133,7 +133,7 @@ async def get_tenant_setting( scope_id: str, key: str, request: Request, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> SettingOut: await require_known_tenant(request, scope_id) result = await service.get_scoped(SettingScope.TENANT, scope_id, key) @@ -148,7 +148,7 @@ async def upsert_tenant_setting( key: str, data: SettingUpsert, request: Request, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> SettingOut: await require_known_tenant(request, scope_id) await run_check(request, scope_id, key, data.value) @@ -159,7 +159,7 @@ async def upsert_tenant_setting( async def delete_tenant_setting( scope_id: str, key: str, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> None: if not await service.delete_scoped(SettingScope.TENANT, scope_id, key): raise _not_found() @@ -169,7 +169,7 @@ async def delete_tenant_setting( async def get_user_setting( scope_id: str, key: str, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> SettingOut: result = await service.get_scoped(SettingScope.USER, scope_id, key) if result is None: @@ -182,7 +182,7 @@ async def upsert_user_setting( scope_id: str, key: str, data: SettingUpsert, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> SettingOut: return await service.upsert_scoped(SettingScope.USER, scope_id, key, data) @@ -191,7 +191,7 @@ async def upsert_user_setting( async def delete_user_setting( scope_id: str, key: str, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> None: if not await service.delete_scoped(SettingScope.USER, scope_id, key): raise _not_found() @@ -204,7 +204,7 @@ async def delete_user_setting( async def create_setting( data: SettingCreate, request: Request, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> SettingOut: if data.scope is SettingScope.TENANT: # A body field, not a path segment: an unknown tenant is invalid input. @@ -219,7 +219,7 @@ async def create_setting( @router.get(API_BY_ID_PATH, response_model=SettingOut, dependencies=_VIEW) async def get_setting( - setting_id: int, service: SettingService = Depends(get_setting_service) + setting_id: int, service: SettingService = Depends(get_generic_setting_service) ) -> SettingOut: result = await service.get_by_id(setting_id) if result is None: @@ -232,7 +232,7 @@ async def update_setting( setting_id: int, data: SettingUpdate, request: Request, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_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: @@ -245,7 +245,7 @@ async def update_setting( @router.delete(API_BY_ID_PATH, status_code=STATUS_NO_CONTENT, dependencies=_DELETE) async def delete_setting( - setting_id: int, service: SettingService = Depends(get_setting_service) + setting_id: int, service: SettingService = Depends(get_generic_setting_service) ) -> None: if not await service.delete(setting_id): raise _not_found() diff --git a/modules/settings/settings/endpoints/views.py b/modules/settings/settings/endpoints/views.py index 6b5c35599..3b501a67e 100644 --- a/modules/settings/settings/endpoints/views.py +++ b/modules/settings/settings/endpoints/views.py @@ -53,7 +53,7 @@ VIEW_STORE_PATH, ) from settings.contracts.schemas import SettingUpdate -from settings.deps import get_setting_service +from settings.deps import get_generic_setting_service from settings.endpoints._create_form import create_from_form from settings.service import SettingService from settings.tenant_scope import tenant_update_error @@ -81,7 +81,7 @@ @router.get(VIEW_STORE_PATH, response_model=None) async def browse( inertia: InertiaDep, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), scope: str = SCOPE_ALL, q: str = "", page: browse_query.PageParam = 1, @@ -139,7 +139,7 @@ async def create_view(request: Request, inertia: InertiaDep) -> InertiaResponse: async def edit_view( setting_id: int, inertia: InertiaDep, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> InertiaResponse: item = await service.get_by_id(setting_id) if item is None: @@ -159,7 +159,7 @@ async def edit_view( ) async def create_action( request: Request, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> RedirectResponse: return await create_from_form(request, service, _REDIRECT_SETTINGS) @@ -172,7 +172,7 @@ async def create_action( async def update_action( setting_id: int, request: Request, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> RedirectResponse: body = await request.json() try: @@ -194,7 +194,7 @@ async def delete_action( setting_id: int, request: Request, t: TranslatorDep, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> RedirectResponse: try: await service.delete(setting_id) @@ -211,7 +211,7 @@ async def delete_action( async def modules_view( request: Request, inertia: InertiaDep, - service: SettingService = Depends(get_setting_service), + service: SettingService = Depends(get_generic_setting_service), ) -> InertiaResponse: """Read-only view of every module's pydantic ``BaseSettings`` instance. diff --git a/modules/settings/settings/errors.py b/modules/settings/settings/errors.py index 368bf5581..c19ea2860 100644 --- a/modules/settings/settings/errors.py +++ b/modules/settings/settings/errors.py @@ -5,7 +5,7 @@ from fastapi import FastAPI, Request from fastapi.responses import JSONResponse, Response -from settings._managed_keys import ManagedKeyError +from settings._managed_keys import ManagedKeyError, ModuleKeyError from settings.constants import STATUS_UNPROCESSABLE @@ -16,5 +16,13 @@ async def _managed_key(request: Request, exc: Exception) -> Response: ) +async def _module_key(request: Request, exc: Exception) -> Response: + assert isinstance(exc, ModuleKeyError) + return JSONResponse( + {"detail": str(exc), "edit_via": exc.edit_via}, status_code=STATUS_UNPROCESSABLE + ) + + def install_exception_handlers(app: FastAPI) -> None: app.add_exception_handler(ManagedKeyError, _managed_key) + app.add_exception_handler(ModuleKeyError, _module_key) diff --git a/modules/settings/settings/known_keys.py b/modules/settings/settings/known_keys.py index 2fa88727d..ab7240111 100644 --- a/modules/settings/settings/known_keys.py +++ b/modules/settings/settings/known_keys.py @@ -1,14 +1,6 @@ -"""Suggestions for the free-form key field, with enough meta to resolve them. +"""Autocomplete for declared generic keys; module settings use their own editor. -The key field is free text, and a typo produces a row that looks saved and is -silently never read — the failure gives no feedback at all. Suggesting the -registered keys makes the common case unmissable without forbidding the -uncommon one: keys outside this list stay valid, since a module can read -settings the settings module cannot enumerate. - -Each suggestion also carries what the New override screen's "Resolved value" -panel needs — the env fallback and the module default — so an admin can see -that the override they are about to write is the value the app already has. +Defaults are masked here because Browse renders them in its resolved-value panel. """ from __future__ import annotations @@ -17,37 +9,12 @@ from fastapi import FastAPI from fastapi.encoders import jsonable_encoder +from simple_module_core.module_settings import get_module_settings_registry from simple_module_core.secret_fields import conceals_secret, mask -from settings._module_settings import collect_module_settings - - -def _from_field(package: str, field) -> dict[str, Any]: - return { - "key": f"{package}.{field.name}", - "type": field.type, - "env_var": field.env_var, - # See ``ModuleSettingField.env_readable``: a bundled module's SM_* var - # is a label, not a fallback, and the panel must say so. - "env_readable": field.env_readable, - "env_set": field.env_set, - "env_value": field.env_value, - "default": jsonable_encoder(field.default), - "requires_restart": field.requires_restart, - "is_secret": field.is_secret, - "choices": field.choices, - } - def _from_definition(definition) -> dict[str, Any]: - """A registry declaration, masked on the same rule as a module field. - - ``_from_field`` receives a default that ``_module_settings`` has already - masked; this builder reads the declaration directly, so it has to apply the - rule itself. Hard-coding ``is_secret: False`` and passing ``default`` - straight through put the raw value into the New-override suggestion list, - which renders it verbatim. - """ + """Mask declarations directly, as module fields are no longer suggested.""" value_type = str(definition.value_type) default = jsonable_encoder(definition.default) secret = conceals_secret(definition.key, default, value_type) @@ -66,21 +33,15 @@ def _from_definition(definition) -> dict[str, Any]: def build(app: FastAPI) -> list[dict[str, Any]]: - """Every ``.`` a module reads, plus every declared key. - - Module fields win over registry declarations of the same key: the pydantic - field is the one the app actually reads, while a declaration only records - intent and can go stale without anything failing. - """ + """Declared generic keys only; registered module packages use their editor.""" suggestions: dict[str, dict[str, Any]] = {} - for view in collect_module_settings(app): - for field in view.fields: - entry = _from_field(view.package, field) - suggestions[entry["key"]] = entry - + modules = get_module_settings_registry(app) registry = getattr(getattr(app.state, "settings", None), "registry", None) if registry is not None: for definition in registry.all_definitions: - suggestions.setdefault(definition.key, _from_definition(definition)) + package, sep, _ = definition.key.partition(".") + if sep and modules is not None and modules.get(package) is not None: + continue + suggestions[definition.key] = _from_definition(definition) return [suggestions[key] for key in sorted(suggestions)] diff --git a/modules/settings/settings/pages/components/ModuleForm.tsx b/modules/settings/settings/pages/components/ModuleForm.tsx index ee2ef0479..da0bd2330 100644 --- a/modules/settings/settings/pages/components/ModuleForm.tsx +++ b/modules/settings/settings/pages/components/ModuleForm.tsx @@ -46,6 +46,7 @@ export function ModuleForm({ module: m, checks = [] }: Props) { const [errors, setErrors] = useState>({}); const [busy, setBusy] = useState(false); const [saved, setSaved] = useState(false); + const [savedOverrides, setSavedOverrides] = useState>({}); const [formErrors, setFormErrors] = useState([]); // Refresh saved values without discarding drafts in other fields (#637). @@ -61,6 +62,7 @@ export function ModuleForm({ module: m, checks = [] }: Props) { })); setErrors({}); setSaved(false); + setSavedOverrides({}); }, [initial]); const modifiedFields = useMemo(() => { @@ -113,6 +115,14 @@ export function ModuleForm({ module: m, checks = [] }: Props) { // Keeping the form mounted lets the confirmation remain visible instead // of being discarded by an immediate Inertia reload. setBuffer((prev) => ({ ...prev, baseline: { ...values } })); + setSavedOverrides((prev) => ({ + ...prev, + ...Object.fromEntries( + m.fields + .filter((f) => f.name in changed) + .map((f) => [f.name, notEqual(changed[f.name], f.default)]), + ), + })); setSaved(true); } setBusy(false); @@ -188,7 +198,7 @@ export function ModuleForm({ module: m, checks = [] }: Props) { {fields.map((f) => ( None: self.db = db + self.module_registry: ModuleSettingsRegistry | None = None # 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. @@ -55,6 +58,19 @@ def __init__( def _changed(self, entity: Setting) -> None: announce(self.db, self.invalidation, entity.scope, entity.scope_id, entity.key) + def _ensure_generic_key(self, scope: SettingScope | str, key: str) -> None: + """Refuse a system row for a module field: only its editor validates and reloads. + + Tenant (and user) rows stay writable here: a ``tenant_overridable`` + module key's per-tenant override has no other platform route (#690). + """ + if SettingScope(scope) is not SettingScope.SYSTEM: + return + package, sep, _ = key.partition(".") + registry = self.module_registry + if sep and registry is not None and registry.get(package) is not None: + raise ModuleKeyError(package, registry.manage_url(package) or f"{VIEW_PREFIX}/") + # ── Lookup ────────────────────────────────────────────────────── async def get_by_id(self, setting_id: int) -> SettingOut | None: @@ -115,6 +131,7 @@ async def get_resolved_value( # ── Mutations ─────────────────────────────────────────────────── async def create(self, data: SettingCreate) -> SettingOut: + self._ensure_generic_key(data.scope, data.key) entity = Setting(**data.model_dump()) if not await insert_if_free(self.db, entity): raise DuplicateSettingError(f"{data.scope.value}/{data.scope_id}/{data.key}") @@ -126,6 +143,7 @@ async def update(self, setting_id: int, data: SettingUpdate) -> SettingOut | Non entity = await self.db.get(Setting, setting_id) if entity is None: return None + self._ensure_generic_key(entity.scope, entity.key) changes = data.model_dump(exclude_unset=True) drop_placeholder_write(entity, changes) for field, value in changes.items(): @@ -142,6 +160,7 @@ async def upsert_scoped( key: str, data: SettingUpsert, ) -> SettingOut: + self._ensure_generic_key(scope, key) entity = await self._find(scope, scope_id, key) if entity is None: entity = Setting( @@ -194,9 +213,11 @@ async def delete_scoped( async def _remove(self, entity: Setting, as_owner: bool) -> None: if not as_owner: + # A managed key's own clear route is the more specific refusal. await ensure_deletable( self.registry, self.tenant_is_live, entity.scope, entity.scope_id, entity.key ) + self._ensure_generic_key(entity.scope, entity.key) self._changed(entity) await self.db.delete(entity) await self.db.flush() diff --git a/modules/settings/tests-js/settings_ModuleFormRevert.test.tsx b/modules/settings/tests-js/settings_ModuleFormRevert.test.tsx index 4c2a3c46b..5eee04f24 100644 --- a/modules/settings/tests-js/settings_ModuleFormRevert.test.tsx +++ b/modules/settings/tests-js/settings_ModuleFormRevert.test.tsx @@ -12,6 +12,7 @@ configureI18n({ locale: 'en', messages: { 'settings.modules_form.save': 'Save', + 'settings.modules_form.saved_toast': 'Saved', 'settings.modules_form.reset_to_default': 'Revert', 'settings.modules.unsaved_count_one': '{count} unsaved', }, @@ -86,3 +87,45 @@ test('switching modules starts a fresh edit buffer even when field names match', expect(screen.getByLabelText('sender')).toHaveValue('mail original'); expect(screen.getByRole('button', { name: 'Save' })).toBeDisabled(); }); + +test('saving a defaulted field immediately offers Revert and keeps confirmation visible', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => new Response('{}', { status: 200 })), + ); + render(); + expect(screen.queryByRole('button', { name: 'Revert' })).not.toBeInTheDocument(); + fireEvent.change(screen.getByLabelText('sender'), { target: { value: 'new sender' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + expect(await screen.findByRole('button', { name: 'Revert' })).toBeVisible(); + expect(screen.getByRole('status')).toHaveTextContent('Saved'); +}); + +test('saving the default removes Revert without changing other fields', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => new Response('{}', { status: 200 })), + ); + render( + , + ); + fireEvent.change(screen.getByLabelText('sender'), { target: { value: '' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + await waitFor(() => expect(screen.getByRole('status')).toHaveTextContent('Saved')); + expect(screen.getAllByRole('button', { name: 'Revert' })).toHaveLength(1); +}); + +test('a failed save keeps a defaulted field without Revert', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => new Response('{}', { status: 422 })), + ); + render(); + fireEvent.change(screen.getByLabelText('sender'), { target: { value: 'rejected' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + await waitFor(() => expect(screen.getByRole('button', { name: 'Save' })).toBeEnabled()); + expect(screen.queryByRole('button', { name: 'Revert' })).not.toBeInTheDocument(); + expect(screen.getByLabelText('sender')).toHaveValue('rejected'); +}); diff --git a/modules/settings/tests/test_known_keys_meta.py b/modules/settings/tests/test_known_keys_meta.py index 7784ffd7c..cf39f6504 100644 --- a/modules/settings/tests/test_known_keys_meta.py +++ b/modules/settings/tests/test_known_keys_meta.py @@ -28,13 +28,18 @@ def _by_key(entries: list[dict], key: str) -> dict: class TestFieldMetadata: - async def test_every_suggestion_carries_the_full_meta_shape( - self, authenticated_client: httpx.AsyncClient - ) -> None: + async def test_module_fields_are_not_suggested(self, authenticated_client): entries = await _known_keys(authenticated_client) + assert not any(e["key"].startswith("mail.") for e in entries) + + async def test_generic_suggestions_carry_the_full_meta_shape(self, app, authenticated_client): + from settings.contracts.registry import SettingDefinition - assert entries - expected = { + app.state.settings.registry.add( + SettingDefinition(key="orders.checkout.require_terms", default="true") + ) + entry = _by_key(await _known_keys(authenticated_client), "orders.checkout.require_terms") + assert { "key", "type", "env_var", @@ -45,54 +50,18 @@ async def test_every_suggestion_carries_the_full_meta_shape( "requires_restart", "is_secret", "choices", - } - assert expected <= set(entries[0]) - - async def test_the_env_var_label_matches_the_module_prefix( - self, authenticated_client: httpx.AsyncClient - ) -> None: - mailer = _by_key(await _known_keys(authenticated_client), "mail.backend") - - assert mailer["env_var"] == "SM_MAIL_BACKEND" - assert mailer["default"] == "console" - - async def test_a_db_backed_module_reports_its_env_var_as_not_read( - self, authenticated_client: httpx.AsyncClient - ) -> None: - """``MailSettings`` declares no ``env_prefix``, so ``SM_MAIL_*`` is a - label and not a fallback. Saying otherwise inverts the question the - Resolved value panel exists to answer.""" - mailer = _by_key(await _known_keys(authenticated_client), "mail.backend") + } <= set(entry) - assert mailer["env_readable"] is False - assert mailer["env_set"] is False - assert mailer["env_value"] is None - - async def test_a_pattern_constrained_string_advertises_its_choices( - self, authenticated_client: httpx.AsyncClient - ) -> None: - mailer = _by_key(await _known_keys(authenticated_client), "mail.backend") - - assert mailer["choices"] == ["console", "smtp"] - - async def test_an_unconstrained_string_has_no_choices( - self, authenticated_client: httpx.AsyncClient - ) -> None: - host = _by_key(await _known_keys(authenticated_client), "mail.smtp_host") - - assert host["choices"] is None - - async def test_a_secret_default_is_masked( - self, authenticated_client: httpx.AsyncClient - ) -> None: + async def test_secret_generic_defaults_are_masked(self, app, authenticated_client): from settings._module_settings import SECRET_MASK + from settings.contracts.registry import SettingDefinition - secret = _by_key( - await _known_keys(authenticated_client), "users.reset_password_token_secret" + app.state.settings.registry.add( + SettingDefinition(key="orders.api_key", default="private-credential") ) - - assert secret["is_secret"] is True - assert secret["default"] == SECRET_MASK + entry = _by_key(await _known_keys(authenticated_client), "orders.api_key") + assert entry["is_secret"] is True + assert entry["default"] == SECRET_MASK class TestRegistryDefinitions: @@ -125,10 +94,10 @@ async def test_a_declared_key_is_suggested( assert entry["choices"] is None assert entry["env_readable"] is False - async def test_a_module_field_wins_over_a_declaration_of_the_same_key( + async def test_a_module_declaration_is_not_suggested( self, app: FastAPI, authenticated_client: httpx.AsyncClient ) -> None: - """The pydantic field is the live one; the declaration only records intent.""" + """A stale declaration must not reintroduce a refused module key.""" from settings.contracts.registry import SettingDefinition app.state.settings.registry.add( @@ -137,8 +106,7 @@ async def test_a_module_field_wins_over_a_declaration_of_the_same_key( mailer = [e for e in await _known_keys(authenticated_client) if e["key"] == "mail.backend"] - assert len(mailer) == 1 - assert mailer[0]["default"] == "console" + assert mailer == [] class TestSuggestionsStaySorted: diff --git a/modules/settings/tests/test_settings_generic_module_guard.py b/modules/settings/tests/test_settings_generic_module_guard.py new file mode 100644 index 000000000..94bcc26a8 --- /dev/null +++ b/modules/settings/tests/test_settings_generic_module_guard.py @@ -0,0 +1,80 @@ +"""Generic store mutations cannot bypass module validation (#690).""" + +import pytest +from simple_module_db.stored_settings import Setting + + +@pytest.mark.parametrize( + "method,path,body", + [ + ("PUT", "/api/settings/system/users.allow_signup", {"value": "true"}), + ("POST", "/api/settings/", {"key": "users.allow_signup", "value": "true"}), + ("POST", "/admin/settings/store", {"key": "users.allow_signup", "value": "true"}), + ], +) +async def test_generic_module_writes_are_refused(authenticated_client, app, method, path, body): + original = app.state.users.settings.allow_signup + response = await authenticated_client.request(method, path, json=body) + assert response.status_code == 422, response.text + assert "/admin/settings/" in response.text + assert app.state.users.settings.allow_signup == original + response = await authenticated_client.get("/api/settings/system/users.allow_signup") + assert response.status_code == 404 + + +@pytest.mark.parametrize( + "method,path", + [ + ("PUT", "/api/settings/{id}"), + ("DELETE", "/api/settings/{id}"), + ("DELETE", "/api/settings/system/users.allow_signup"), + ("PUT", "/admin/settings/{id}"), + ("DELETE", "/admin/settings/{id}"), + ], +) +async def test_existing_module_rows_cannot_be_mutated_generically( + authenticated_client, app, method, path +): + async with app.state.sm.db.session_factory() as db: + row = Setting(key="users.allow_signup", value="false", value_type="bool") + db.add(row) + await db.commit() + row_id = row.id + response = await authenticated_client.request( + method, path.format(id=row_id), json={"value": "true"} if method == "PUT" else None + ) + assert response.status_code == 422, response.text + assert "/admin/settings/" in response.text + response = await authenticated_client.get("/api/settings/system/users.allow_signup") + assert response.json()["value"] == "false" + + +def test_browse_does_not_suggest_module_keys(): + from types import SimpleNamespace + + from fastapi import FastAPI + from settings.known_keys import build + from simple_module_core.module_settings import register_module_settings + from users.settings import UsersSettings + + app = FastAPI() + app.state.sm = SimpleNamespace(modules=[]) + register_module_settings(app, "users", UsersSettings, lambda s: SimpleNamespace(settings=s)) + assert not any(entry["key"].startswith("users.") for entry in build(app)) + + +async def test_tenant_row_for_module_key_stays_generically_writable(authenticated_client, app): + """Only system rows are the module's own value; tenant overrides have no other route.""" + async with app.state.sm.db.session_factory() as db: + row = Setting( + key="users.allow_signup", + value="false", + value_type="bool", + scope="tenant", + scope_id="acme", + ) + db.add(row) + await db.commit() + row_id = row.id + response = await authenticated_client.put(f"/api/settings/{row_id}", json={"value": "true"}) + assert response.status_code == 200, response.text diff --git a/modules/settings/tests/test_settings_secret_urls.py b/modules/settings/tests/test_settings_secret_urls.py index 9cc13df7f..32eb1a3f0 100644 --- a/modules/settings/tests/test_settings_secret_urls.py +++ b/modules/settings/tests/test_settings_secret_urls.py @@ -91,21 +91,21 @@ async def test_a_broker_url_with_a_password_is_masked( resp = await authenticated_client.get(_CREATE, headers=_INERTIA) assert resp.status_code == 200, resp.text[:400] entries = resp.json()["props"]["known_keys"] - broker = next(e for e in entries if e["key"] == "background_tasks.broker_url") - - assert broker["is_secret"] is True - assert broker["env_value"] == SECRET_MASK + assert not any(e["key"] == "background_tasks.broker_url" for e in entries) assert "hunter2" not in resp.text - async def test_a_password_free_broker_url_stays_visible( - self, authenticated_client: httpx.AsyncClient - ) -> None: + async def test_declared_generic_dsn_is_masked(self, app, authenticated_client) -> None: + from settings.contracts.registry import SettingDefinition + + app.state.settings.registry.add( + SettingDefinition(key="integration.database_url", default=_WITH_PASSWORD) + ) resp = await authenticated_client.get(_CREATE, headers=_INERTIA) entries = resp.json()["props"]["known_keys"] - broker = next(e for e in entries if e["key"] == "background_tasks.broker_url") - - assert broker["is_secret"] is False - assert broker["default"].startswith("redis://") + entry = next(e for e in entries if e["key"] == "integration.database_url") + assert entry["is_secret"] is True + assert entry["default"] == SECRET_MASK + assert "hunter2" not in resp.text class TestMaskSentinelIsNeverStored: From c95aff54523f95a1045eae364b4d5ec0486962b2 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 30 Oct 2026 08:24:21 +0100 Subject: [PATCH 2/3] fix(settings): Browse flashes the module-key refusal; orphaned module rows stay deletable (#690) Review follow-up: the Inertia Browse create/update/delete routes let ModuleKeyError reach the app-wide JSON handler, breaking the page; they now redirect back with the message. The guard covers only fields a module still declares, so a leftover row for a removed field can be cleared. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../settings/endpoints/_create_form.py | 3 ++ modules/settings/settings/endpoints/views.py | 9 +++-- modules/settings/settings/service.py | 7 ++-- .../test_settings_generic_module_guard.py | 35 +++++++++++++++++-- 4 files changed, 47 insertions(+), 7 deletions(-) diff --git a/modules/settings/settings/endpoints/_create_form.py b/modules/settings/settings/endpoints/_create_form.py index 5dbc78bba..2ab7e6376 100644 --- a/modules/settings/settings/endpoints/_create_form.py +++ b/modules/settings/settings/endpoints/_create_form.py @@ -7,6 +7,7 @@ from simple_module_hosting.inertia_utils import redirect_back_with_errors, validation_errors_to_dict from starlette.responses import RedirectResponse +from settings._managed_keys import ModuleKeyError from settings._unique_write import DuplicateSettingError from settings.constants import ERR_SETTING_EXISTS from settings.contracts.schemas import SettingCreate, SettingScope @@ -31,4 +32,6 @@ async def create_from_form( await service.create(data) except DuplicateSettingError: return redirect_back_with_errors(request, {"key": ERR_SETTING_EXISTS}) + except ModuleKeyError as exc: + return redirect_back_with_errors(request, {"key": str(exc)}) return RedirectResponse(redirect_to, status_code=303) diff --git a/modules/settings/settings/endpoints/views.py b/modules/settings/settings/endpoints/views.py index 3b501a67e..98250c4a2 100644 --- a/modules/settings/settings/endpoints/views.py +++ b/modules/settings/settings/endpoints/views.py @@ -22,7 +22,7 @@ from starlette.responses import RedirectResponse from settings import browse_query, known_keys -from settings._managed_keys import ManagedKeyError +from settings._managed_keys import ManagedKeyError, ModuleKeyError from settings._module_settings import ( _package_of, collect_module_settings, @@ -181,7 +181,10 @@ async def update_action( 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) + try: + await service.update(setting_id, data) + except ModuleKeyError as exc: + return redirect_back_with_errors(request, {"value": str(exc)}) return RedirectResponse(_REDIRECT_SETTINGS, status_code=303) @@ -204,6 +207,8 @@ async def delete_action( "settings.browse.delete_managed_error", setting=exc.key, clear_via=exc.clear_via ) return redirect_back_with_errors(request, {"delete": message}) + except ModuleKeyError as exc: + return redirect_back_with_errors(request, {"delete": str(exc)}) return RedirectResponse(_REDIRECT_SETTINGS, status_code=303) diff --git a/modules/settings/settings/service.py b/modules/settings/settings/service.py index ca34e8c36..a683a3efd 100644 --- a/modules/settings/settings/service.py +++ b/modules/settings/settings/service.py @@ -66,9 +66,12 @@ def _ensure_generic_key(self, scope: SettingScope | str, key: str) -> None: """ if SettingScope(scope) is not SettingScope.SYSTEM: return - package, sep, _ = key.partition(".") + package, sep, field = key.partition(".") registry = self.module_registry - if sep and registry is not None and registry.get(package) is not None: + settings_cls = registry.get(package) if sep and registry is not None else None + # An orphaned row (no such field any more) has no editor to clear it, so + # it stays deletable here. + if settings_cls is not None and field in settings_cls.model_fields: raise ModuleKeyError(package, registry.manage_url(package) or f"{VIEW_PREFIX}/") # ── Lookup ────────────────────────────────────────────────────── diff --git a/modules/settings/tests/test_settings_generic_module_guard.py b/modules/settings/tests/test_settings_generic_module_guard.py index 94bcc26a8..aaaf60dae 100644 --- a/modules/settings/tests/test_settings_generic_module_guard.py +++ b/modules/settings/tests/test_settings_generic_module_guard.py @@ -9,7 +9,6 @@ [ ("PUT", "/api/settings/system/users.allow_signup", {"value": "true"}), ("POST", "/api/settings/", {"key": "users.allow_signup", "value": "true"}), - ("POST", "/admin/settings/store", {"key": "users.allow_signup", "value": "true"}), ], ) async def test_generic_module_writes_are_refused(authenticated_client, app, method, path, body): @@ -28,8 +27,6 @@ async def test_generic_module_writes_are_refused(authenticated_client, app, meth ("PUT", "/api/settings/{id}"), ("DELETE", "/api/settings/{id}"), ("DELETE", "/api/settings/system/users.allow_signup"), - ("PUT", "/admin/settings/{id}"), - ("DELETE", "/admin/settings/{id}"), ], ) async def test_existing_module_rows_cannot_be_mutated_generically( @@ -78,3 +75,35 @@ async def test_tenant_row_for_module_key_stays_generically_writable(authenticate row_id = row.id response = await authenticated_client.put(f"/api/settings/{row_id}", json={"value": "true"}) assert response.status_code == 200, response.text + + +@pytest.mark.parametrize( + "method,path,body", + [ + ("POST", "/admin/settings/store", {"key": "users.allow_signup", "value": "true"}), + ("PUT", "/admin/settings/{id}", {"value": "true"}), + ("DELETE", "/admin/settings/{id}", None), + ], +) +async def test_view_routes_flash_error_instead_of_raw_422( + authenticated_client, app, method, path, body +): + """Inertia form routes redirect back with errors; a raw JSON 422 breaks the page.""" + async with app.state.sm.db.session_factory() as db: + row = Setting(key="users.allow_signup", value="false", value_type="bool") + db.add(row) + await db.commit() + row_id = row.id + response = await authenticated_client.request(method, path.format(id=row_id), json=body) + assert response.status_code == 303, response.text + response = await authenticated_client.get("/api/settings/system/users.allow_signup") + assert response.json()["value"] == "false" + + +async def test_orphaned_module_row_stays_deletable(authenticated_client, app): + """A row for a field the module no longer declares has no editor to clear it.""" + async with app.state.sm.db.session_factory() as db: + db.add(Setting(key="users.no_such_field", value="x", value_type="str")) + await db.commit() + response = await authenticated_client.delete("/api/settings/system/users.no_such_field") + assert response.status_code in (200, 204), response.text From 7b81ed5e09bc82a12734b4478e7a732e371a8503 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 30 Oct 2026 08:49:40 +0100 Subject: [PATCH 3/3] fix(settings): Revert right after a save shows the live value and hides Revert (#696) Found in QA: when no reload happened after the save, the post-revert reload returned props equal to the page's original ones, the [initial] effect did not fire, and the form kept the edited value and the Revert button. Revert now clears the local override flag and takes the field's value from the reload response itself. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../settings/pages/components/ModuleForm.tsx | 19 ++++++++++++++++++- .../settings_ModuleFormRevert.test.tsx | 19 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/modules/settings/settings/pages/components/ModuleForm.tsx b/modules/settings/settings/pages/components/ModuleForm.tsx index da0bd2330..d18231001 100644 --- a/modules/settings/settings/pages/components/ModuleForm.tsx +++ b/modules/settings/settings/pages/components/ModuleForm.tsx @@ -136,11 +136,28 @@ export function ModuleForm({ module: m, checks = [] }: Props) { params: { path: { package: m.package, field: name } }, }), ); + setSavedOverrides((prev) => ({ ...prev, [name]: false })); setBuffer((prev) => ({ ...prev, values: { ...prev.values, [name]: prev.baseline[name] }, })); - router.reload({ only: ['modules'] }); + // Take the field's live value from the reload itself: when the page never + // reloaded after a save, the fresh props equal the old ones and the + // [initial] effect does not fire, leaving the reverted value on screen. + router.reload({ + only: ['modules'], + onSuccess: (page) => { + const modules = (page.props as { modules?: ModuleView[] }).modules ?? []; + const fresh = modules + .find((other) => other.package === m.package) + ?.fields.find((f) => f.name === name); + if (!fresh) return; + setBuffer((prev) => ({ + values: { ...prev.values, [name]: fresh.value }, + baseline: { ...prev.baseline, [name]: fresh.value }, + })); + }, + }); } return ( diff --git a/modules/settings/tests-js/settings_ModuleFormRevert.test.tsx b/modules/settings/tests-js/settings_ModuleFormRevert.test.tsx index 5eee04f24..4da66d352 100644 --- a/modules/settings/tests-js/settings_ModuleFormRevert.test.tsx +++ b/modules/settings/tests-js/settings_ModuleFormRevert.test.tsx @@ -129,3 +129,22 @@ test('a failed save keeps a defaulted field without Revert', async () => { expect(screen.queryByRole('button', { name: 'Revert' })).not.toBeInTheDocument(); expect(screen.getByLabelText('sender')).toHaveValue('rejected'); }); + +test('revert right after a save shows the live default and hides Revert', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => new Response('{}', { status: 200 })), + ); + const fields = [{ ...field('sender', 'default-sender'), default: 'default-sender' }]; + mocks.reload.mockImplementation((options: { onSuccess?: (page: unknown) => void }) => { + options.onSuccess?.({ props: { modules: [view(fields)] } }); + }); + render(); + const input = screen.getByDisplayValue('default-sender'); + fireEvent.change(input, { target: { value: 'custom' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + const revert = await screen.findByRole('button', { name: 'Revert' }); + fireEvent.click(revert); + await waitFor(() => expect(screen.getByDisplayValue('default-sender')).toBeInTheDocument()); + expect(screen.queryByRole('button', { name: 'Revert' })).not.toBeInTheDocument(); +});