diff --git a/.gitignore b/.gitignore index e62553ec..a878eda3 100644 --- a/.gitignore +++ b/.gitignore @@ -63,3 +63,4 @@ Thumbs.db host/client_app/.playwright-cli/* .superpowers/ .qa/ +.verify/ diff --git a/framework/core/simple_module_core/__init__.py b/framework/core/simple_module_core/__init__.py index d43e1bfa..65ba4730 100644 --- a/framework/core/simple_module_core/__init__.py +++ b/framework/core/simple_module_core/__init__.py @@ -1,5 +1,6 @@ """SimpleModule Core - Module system, menu, permissions, events, and diagnostics.""" +from simple_module_core.audit_links import AuditLink, AuditLinkRegistry from simple_module_core.design_packs import DesignPack, DesignPackRegistry from simple_module_core.diagnostics import ( DiagnosticLevel, @@ -44,6 +45,8 @@ __all__ = [ "DEFAULT_AUTH_PROVIDER", "FRAMEWORK_API_VERSION", + "AuditLink", + "AuditLinkRegistry", "CircularDependencyError", "DesignPack", "DesignPackRegistry", diff --git a/framework/core/simple_module_core/audit_links.py b/framework/core/simple_module_core/audit_links.py new file mode 100644 index 00000000..9c61b1f9 --- /dev/null +++ b/framework/core/simple_module_core/audit_links.py @@ -0,0 +1,84 @@ +"""Audit-link registry — modules teach the audit log how to reach their records. + +An audit entry stores the *model class name* and primary key of the row that +changed (``StoredFile``, ``a91f3c2b…`` — ``snapshot_changes`` records +``type(obj).__name__``, never ``__tablename__``). That is enough to prove what +happened and useless for doing anything about it: the reader has an id and no +way to open the record it names. + +A module declares where its rows live via +:meth:`~simple_module_core.module.ModuleBase.register_audit_links`; the host +collects them into one registry at boot and stores it on +``app.state.sm.audit_links``. + +**The registry maps model class names to URL templates, nothing more.** It does +not verify the row exists or that the reader may open it — following a link to a +deleted record lands on that screen's own 404, and permissions are enforced by +the target route as usual. +""" + +from __future__ import annotations + +from dataclasses import dataclass + +_ID_PLACEHOLDER = "{id}" + + +@dataclass(frozen=True) +class AuditLink: + """Where the records of one audited model can be viewed. + + Args: + entity_type: The **model class name**, matching + ``AuditEntry.entity_type`` (e.g. ``"User"``, not ``"users_user"``). + ``snapshot_changes`` records ``type(obj).__name__``, so keying this + off ``__tablename__`` silently never matches — and because an + unmatched lookup falls back to showing ``entity_type`` as the + label, a table-name key looks like it worked. + url_template: Path containing ``{id}``, substituted with the entity id + (e.g. ``"/admin/users/{id}/edit"``). + label: Human-readable name for the entity kind, shown instead of the + raw class name (e.g. ``"User account"``). + """ + + entity_type: str + url_template: str + label: str = "" + + def __post_init__(self) -> None: + if _ID_PLACEHOLDER not in self.url_template: + raise ValueError( + f"AuditLink for {self.entity_type!r} has url_template " + f"{self.url_template!r}, which contains no {_ID_PLACEHOLDER} — " + f"every row would link to the same page" + ) + + def url_for(self, entity_id: str) -> str: + return self.url_template.replace(_ID_PLACEHOLDER, entity_id) + + +class AuditLinkRegistry: + """Aggregates every module's :class:`AuditLink` declarations. + + Populated once during boot (``register_audit_links`` hook) and read + thereafter by the audit log view when it renders each row. + """ + + def __init__(self) -> None: + self._links: dict[str, AuditLink] = {} + + def register(self, link: AuditLink) -> None: + existing = self._links.get(link.entity_type) + if existing is not None and existing != link: + raise ValueError( + f"Two modules claim audit links for {link.entity_type!r}: " + f"{existing.url_template!r} and {link.url_template!r}" + ) + self._links[link.entity_type] = link + + def get(self, entity_type: str) -> AuditLink | None: + return self._links.get(entity_type) + + @property + def all_links(self) -> dict[str, AuditLink]: + return dict(self._links) diff --git a/framework/core/simple_module_core/health.py b/framework/core/simple_module_core/health.py index 4c6f3f39..6999f690 100644 --- a/framework/core/simple_module_core/health.py +++ b/framework/core/simple_module_core/health.py @@ -30,6 +30,22 @@ class HealthCheck: name: str check: HealthCheckFn + module: str = "" + """Module that contributed the check. Stamped by the registry during + ``register_health_checks``; module authors never set it by hand.""" + probe: bool = True + """Whether automatic pollers may run this check. + + Set ``False`` for checks that reach a third party — an SMTP login, an S3 + request. Readiness is asked on a timer (a Kubernetes probe every 10s), and + a check that authenticates against a mail provider on that schedule earns + a rate-limit and binds probe latency to someone else's uptime. Such a + dependency is also not a readiness signal: the app serves pages fine while + its mailer is down. + + ``False`` checks still run when explicitly requested — that is what the + "Test connection" action on the module-settings screen invokes. + """ class HealthRegistry: @@ -37,10 +53,32 @@ class HealthRegistry: def __init__(self) -> None: self._checks: list[HealthCheck] = [] + self._current_owner: str = "" + + def set_owner(self, module_name: str) -> None: + """Attribute subsequently-added checks to ``module_name``. + + The host calls this around each ``register_health_checks`` hook so a + check knows which module it belongs to without changing the ``add`` + signature module authors already use. Attribution is what lets the + dashboard show health per module rather than one global number. + """ + self._current_owner = module_name def add(self, check: HealthCheck) -> None: + if not check.module: + check.module = self._current_owner self._checks.append(check) @property def all_checks(self) -> list[HealthCheck]: + """Every registered check, including on-demand ones. + + Callers that poll on a timer want :attr:`probe_checks` instead. + """ return list(self._checks) + + @property + def probe_checks(self) -> list[HealthCheck]: + """Checks safe to run automatically, on a timer.""" + return [c for c in self._checks if c.probe] diff --git a/framework/core/simple_module_core/menu.py b/framework/core/simple_module_core/menu.py index 648e9261..e4c0869b 100644 --- a/framework/core/simple_module_core/menu.py +++ b/framework/core/simple_module_core/menu.py @@ -30,6 +30,15 @@ class MenuItem: requires_auth: bool = True roles: list[str] = field(default_factory=list) """Empty list = visible to all authenticated users.""" + permissions: list[str] = field(default_factory=list) + """Permission keys required to see this entry. Empty = no permission check. + + Declare the same permission the target route enforces. Roles alone cannot + express this: a custom role holding ``settings.view`` should see Settings, + and hard-coding ``roles=["admin"]`` would hide it from them while still + showing it to any admin-adjacent role that cannot actually open it. An + entry that 403s on click is worse than no entry at all. + """ method: MenuItemMethod = "get" """HTTP method used when the item is activated. ``"post"`` renders as an Inertia form submission so the target endpoint can be POST-only (e.g. logout).""" @@ -69,12 +78,18 @@ def get_for_user( *, is_authenticated: bool, roles: list[str] | None = None, + permissions: list[str] | None = None, ) -> dict[str, list[dict]]: - """Return menu items grouped by section, filtered by auth/roles. + """Return menu items grouped by section, filtered by auth/roles/permissions. + + ``permissions`` is the caller's already-expanded permission list (no + wildcards). Items declaring permissions the caller lacks are dropped, + so the sidebar never offers a screen that will 403 on click. Returns a dict ready to be serialized into Inertia shared props. """ roles = roles or [] + granted = set(permissions or []) result: dict[str, list[dict]] = {s.value: [] for s in MenuSection} for item in self.all_items: @@ -82,6 +97,10 @@ def get_for_user( continue if item.roles and not any(r in item.roles for r in roles): continue + # All declared permissions must be held: an entry naming several is + # asking for all of them, matching how route guards compose. + if item.permissions and not granted.issuperset(item.permissions): + continue result[item.section.value].append( { "label": item.label, diff --git a/framework/core/simple_module_core/module.py b/framework/core/simple_module_core/module.py index 703c04e6..04d8d233 100644 --- a/framework/core/simple_module_core/module.py +++ b/framework/core/simple_module_core/module.py @@ -10,6 +10,7 @@ if TYPE_CHECKING: from fastapi import APIRouter, FastAPI + from simple_module_core.audit_links import AuditLinkRegistry from simple_module_core.design_packs import DesignPackRegistry from simple_module_core.events import EventBus from simple_module_core.feature_flags import FeatureFlagRegistry @@ -163,6 +164,28 @@ def register_design_packs(self, registry): order. """ + def register_audit_links(self, registry: AuditLinkRegistry) -> None: + """Declare where this module's audited records can be viewed. + + An audit entry stores a table name and a primary key, which tells the + reader what changed but gives them no way to open it. Override this + hook to make your module's rows reachable from the audit log:: + + def register_audit_links(self, registry): + registry.register( + AuditLink( + entity_type="users_user", + url_template="/admin/users/{id}/edit", + label="User", + ) + ) + + ``entity_type`` is the ``__tablename__`` the rows are audited under. + Registering only supplies the URL — the target route still enforces + its own permissions, so linking never widens access. Called once at + boot, in dependency order. + """ + def register_middleware(self, app: FastAPI) -> None: """Add middleware to the application. diff --git a/framework/core/simple_module_core/services.py b/framework/core/simple_module_core/services.py index cb86c11c..4df8860c 100644 --- a/framework/core/simple_module_core/services.py +++ b/framework/core/simple_module_core/services.py @@ -20,6 +20,7 @@ from simple_module_db.session import DatabaseState from simple_module_hosting.settings import Settings + from simple_module_core.audit_links import AuditLinkRegistry from simple_module_core.design_packs import DesignPackRegistry from simple_module_core.events import EventBus from simple_module_core.feature_flags import FeatureFlagRegistry @@ -44,6 +45,7 @@ class Services: health_registry: HealthRegistry public_routes: PublicRouteRegistry design_packs: DesignPackRegistry + audit_links: AuditLinkRegistry i18n_registry: I18nRegistry inertia_config: InertiaConfig modules: tuple[ModuleBase, ...] diff --git a/framework/core/tests/test_audit_links.py b/framework/core/tests/test_audit_links.py new file mode 100644 index 00000000..ba8fdc15 --- /dev/null +++ b/framework/core/tests/test_audit_links.py @@ -0,0 +1,57 @@ +"""Tests for AuditLink / AuditLinkRegistry. + +The audit log stores a table name and a primary key. This registry is how a +module says where those rows can be opened, so the log stops being a wall of +unactionable uuids. +""" + +from __future__ import annotations + +import pytest +from simple_module_core.audit_links import AuditLink, AuditLinkRegistry + + +class TestAuditLink: + def test_url_for_substitutes_the_id(self): + link = AuditLink(entity_type="User", url_template="/users/admin/{id}") + assert link.url_for("a91f3c2b") == "/users/admin/a91f3c2b" + + def test_template_without_placeholder_is_rejected(self): + """Every row would otherwise link to the same page.""" + with pytest.raises(ValueError, match="contains no"): + AuditLink(entity_type="User", url_template="/users/admin") + + def test_label_defaults_to_empty(self): + assert AuditLink(entity_type="t", url_template="/t/{id}").label == "" + + +class TestAuditLinkRegistry: + def test_get_returns_none_for_unclaimed_tables(self): + """Join tables and blob stores have no screen; that is not an error.""" + assert AuditLinkRegistry().get("UserPermission") is None + + def test_register_and_get(self): + reg = AuditLinkRegistry() + link = AuditLink(entity_type="User", url_template="/users/admin/{id}", label="User") + reg.register(link) + assert reg.get("User") is link + + def test_conflicting_claims_raise(self): + """Two modules mapping one table means one of them silently loses.""" + reg = AuditLinkRegistry() + reg.register(AuditLink(entity_type="User", url_template="/a/{id}")) + with pytest.raises(ValueError, match="Two modules claim"): + reg.register(AuditLink(entity_type="User", url_template="/b/{id}")) + + def test_registering_the_same_link_twice_is_allowed(self): + """Idempotent re-registration must not break a re-entrant boot.""" + reg = AuditLinkRegistry() + for _ in range(2): + reg.register(AuditLink(entity_type="User", url_template="/a/{id}")) + assert len(reg.all_links) == 1 + + def test_all_links_is_a_copy(self): + reg = AuditLinkRegistry() + reg.register(AuditLink(entity_type="User", url_template="/a/{id}")) + reg.all_links.clear() + assert reg.get("User") is not None diff --git a/framework/core/tests/test_health_registry.py b/framework/core/tests/test_health_registry.py index 1c0fb390..cbbb86b6 100644 --- a/framework/core/tests/test_health_registry.py +++ b/framework/core/tests/test_health_registry.py @@ -33,6 +33,41 @@ async def check_b() -> HealthCheckResult: reg.add(HealthCheck(name="b", check=check_b)) assert len(reg.all_checks) == 2 + async def test_checks_are_attributed_to_the_owning_module(self): + """The dashboard shows health per module, which needs this attribution.""" + reg = HealthRegistry() + + async def check() -> HealthCheckResult: + return HealthCheckResult(status=HealthStatus.HEALTHY) + + reg.set_owner("FileStorage") + reg.add(HealthCheck(name="s3", check=check)) + reg.set_owner("BackgroundTasks") + reg.add(HealthCheck(name="broker", check=check)) + + owners = {c.name: c.module for c in reg.all_checks} + assert owners == {"s3": "FileStorage", "broker": "BackgroundTasks"} + + async def test_explicit_module_survives_the_current_owner(self): + reg = HealthRegistry() + + async def check() -> HealthCheckResult: + return HealthCheckResult(status=HealthStatus.HEALTHY) + + reg.set_owner("Dashboard") + reg.add(HealthCheck(name="db", check=check, module="Users")) + assert reg.all_checks[0].module == "Users" + + async def test_unowned_checks_have_no_module(self): + """Checks added outside a register_health_checks hook belong to nobody.""" + reg = HealthRegistry() + + async def check() -> HealthCheckResult: + return HealthCheckResult(status=HealthStatus.HEALTHY) + + reg.add(HealthCheck(name="db", check=check)) + assert reg.all_checks[0].module == "" + async def test_check_result_defaults(self): result = HealthCheckResult(status=HealthStatus.HEALTHY) assert result.detail is None diff --git a/framework/core/tests/test_menu.py b/framework/core/tests/test_menu.py index cd9cdcfc..04db0bdc 100644 --- a/framework/core/tests/test_menu.py +++ b/framework/core/tests/test_menu.py @@ -114,3 +114,52 @@ async def test_group_serialized(self): result = reg.get_for_user(is_authenticated=True) groups = [i["group"] for i in result["sidebar"]] assert groups == ["Administration", "System"] + + +class TestPermissionFiltering: + """An entry that 403s on click is worse than no entry at all. + + Roles alone could not express this: hard-coding ``roles=["admin"]`` hides + the screen from a custom role that legitimately holds the permission, while + still showing it to admin-adjacent roles that cannot open it. + """ + + async def test_entry_hidden_without_the_permission(self): + reg = MenuRegistry() + reg.add(MenuItem(label="Settings", url="/settings/", permissions=["settings.view"])) + result = reg.get_for_user(is_authenticated=True, permissions=["users.manage"]) + assert result["sidebar"] == [] + + async def test_entry_shown_with_the_permission(self): + reg = MenuRegistry() + reg.add(MenuItem(label="Settings", url="/settings/", permissions=["settings.view"])) + result = reg.get_for_user(is_authenticated=True, permissions=["settings.view"]) + assert [i["label"] for i in result["sidebar"]] == ["Settings"] + + async def test_permission_granted_by_a_custom_role_is_enough(self): + """No role check involved — holding the key is what matters.""" + reg = MenuRegistry() + reg.add(MenuItem(label="Settings", url="/settings/", permissions=["settings.view"])) + result = reg.get_for_user( + is_authenticated=True, roles=["auditor"], permissions=["settings.view"] + ) + assert len(result["sidebar"]) == 1 + + async def test_all_declared_permissions_are_required(self): + reg = MenuRegistry() + reg.add(MenuItem(label="X", url="/x", permissions=["a.view", "a.manage"])) + assert reg.get_for_user(is_authenticated=True, permissions=["a.view"])["sidebar"] == [] + both = reg.get_for_user(is_authenticated=True, permissions=["a.view", "a.manage"]) + assert len(both["sidebar"]) == 1 + + async def test_no_declared_permissions_means_no_check(self): + """Existing entries must keep working without opting in.""" + reg = MenuRegistry() + reg.add(MenuItem(label="Home", url="/")) + assert len(reg.get_for_user(is_authenticated=True, permissions=[])["sidebar"]) == 1 + + async def test_omitting_permissions_entirely_hides_gated_entries(self): + """A caller that passes no permissions holds none — fail closed.""" + reg = MenuRegistry() + reg.add(MenuItem(label="Settings", url="/settings/", permissions=["settings.view"])) + assert reg.get_for_user(is_authenticated=True)["sidebar"] == [] diff --git a/framework/core/tests/test_services.py b/framework/core/tests/test_services.py index 384a598d..15e9da67 100644 --- a/framework/core/tests/test_services.py +++ b/framework/core/tests/test_services.py @@ -31,6 +31,7 @@ async def test_services_round_trip_field_access(self) -> None: assert s.health_registry is _SENTINEL_HEALTH assert s.public_routes is _SENTINEL_PUBLIC_ROUTES assert s.design_packs is _SENTINEL_DESIGN_PACKS + assert s.audit_links is _SENTINEL_AUDIT_LINKS assert s.i18n_registry is _SENTINEL_I18N assert s.inertia_config is _SENTINEL_INERTIA assert s.modules == () @@ -45,6 +46,7 @@ async def test_services_round_trip_field_access(self) -> None: _SENTINEL_HEALTH = object() _SENTINEL_PUBLIC_ROUTES = object() _SENTINEL_DESIGN_PACKS = object() +_SENTINEL_AUDIT_LINKS = object() _SENTINEL_I18N = object() _SENTINEL_INERTIA = object() @@ -61,6 +63,7 @@ def _make_services() -> Services: health_registry=_SENTINEL_HEALTH, # type: ignore[arg-type] public_routes=_SENTINEL_PUBLIC_ROUTES, # type: ignore[arg-type] design_packs=_SENTINEL_DESIGN_PACKS, # type: ignore[arg-type] + audit_links=_SENTINEL_AUDIT_LINKS, # type: ignore[arg-type] i18n_registry=_SENTINEL_I18N, # type: ignore[arg-type] inertia_config=_SENTINEL_INERTIA, # type: ignore[arg-type] modules=(), diff --git a/framework/hosting/simple_module_hosting/_db_health.py b/framework/hosting/simple_module_hosting/_db_health.py new file mode 100644 index 00000000..628248b8 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_db_health.py @@ -0,0 +1,56 @@ +"""The host's own readiness check: can we reach the database? + +Registered by ``create_app`` rather than by a module, because it is the one +dependency no request can do without — unlike a mailer or an object store, +which a page load never touches. + +It also gives ``/health/ready`` something to actually report. Modules +contribute checks for third-party services, and those are ``probe=False`` +(polling an SMTP login every 10s earns a rate-limit), so without this the +readiness endpoint would answer "healthy" from an empty check set — a green +light that proves nothing. + +It is owned by the host, not a module, so it deliberately does not light up any +tile on the dashboard's per-module health strip — those are keyed by +``HealthCheck.module``. +""" + +from __future__ import annotations + +from simple_module_core.health import HealthCheck, HealthCheckResult, HealthStatus +from sqlalchemy import text + +CHECK_DATABASE = "host.database" + +_MODULE = "Host" + + +def build_database_check(db_state): + """Return an async check issuing the cheapest possible round trip.""" + + async def check() -> HealthCheckResult: + try: + async with db_state.session_factory() as session: + await session.execute(text("SELECT 1")) + except Exception as exc: + return HealthCheckResult(status=HealthStatus.UNHEALTHY, detail=str(exc)) + return HealthCheckResult(status=HealthStatus.HEALTHY, detail="Database reachable") + + return check + + +def register_database_check(health_registry, db_state) -> None: + """Add the database check to *health_registry*. + + ``probe=True``: a ``SELECT 1`` against an already-pooled connection is + cheap enough to run on a probe timer, and it is exactly what a readiness + probe should be asking. + """ + health_registry.add( + HealthCheck( + name=CHECK_DATABASE, + check=build_database_check(db_state), + module=_MODULE, + probe=True, + ) + ) diff --git a/framework/hosting/simple_module_hosting/_error_handlers.py b/framework/hosting/simple_module_hosting/_error_handlers.py index e7c280b2..4737562b 100644 --- a/framework/hosting/simple_module_hosting/_error_handlers.py +++ b/framework/hosting/simple_module_hosting/_error_handlers.py @@ -34,7 +34,18 @@ async def render_error_page(request: Request, status_code: int, message: str) -> shared = getattr(request.state, "inertia_shared", None) if shared: inertia.share(**shared) - response = await inertia.render("Error", {"status": status_code, "message": message}) + # The correlation id is the only handle a user has on their own failed + # request — without it a support report is just "it broke". It is already + # on every log line for this request, so quoting it back makes the page + # and the logs joinable. + response = await inertia.render( + "Error", + { + "status": status_code, + "message": message, + "correlation_id": getattr(request.state, "correlation_id", "") or "", + }, + ) response.status_code = status_code return response except InertiaVersionConflictException as exc: diff --git a/framework/hosting/simple_module_hosting/_registrations.py b/framework/hosting/simple_module_hosting/_registrations.py new file mode 100644 index 00000000..b1372e1c --- /dev/null +++ b/framework/hosting/simple_module_hosting/_registrations.py @@ -0,0 +1,65 @@ +"""Phase 5 of boot — every module's declarative registration hooks. + +Extracted from ``app_builder.py`` to keep that file readable; this is the one +place that knows the full set of hooks a module may implement, and the order +they run in. ``app_builder.create_app`` is the only intended caller. +""" + +from __future__ import annotations + +import inspect +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from fastapi import FastAPI + + +def run_module_registrations( + modules: list, + *, + app: FastAPI, + event_bus, + menu_registry, + perm_registry, + ff_registry, + health_registry, + public_route_registry, + design_pack_registry, + audit_link_registry, +) -> None: + """Invoke each module's registration hooks, in dependency order. + + Health checks are attributed to the module that registers them, so the + dashboard can report health per module rather than one global number. The + owner is cleared afterwards: anything registered later — a module's + ``on_startup``, say — belongs to no module in this loop, and inheriting + the last one's name would be a lie. + """ + for mod in modules: + mod.register_menu_items(menu_registry) + mod.register_permissions(perm_registry) + mod.register_feature_flags(ff_registry) + dispatch_event_handlers(mod, event_bus, app) + health_registry.set_owner(mod.meta.name) + mod.register_health_checks(health_registry) + mod.register_public_routes(public_route_registry) + mod.register_design_packs(design_pack_registry) + mod.register_audit_links(audit_link_registry) + + health_registry.set_owner("") + + +def dispatch_event_handlers(mod, event_bus, app: FastAPI) -> None: + """Call ``mod.register_event_handlers`` with or without ``app``. + + Back-compat shim for modules that still override the one-arg form + ``(self, bus)``; passing ``app=`` to those crashes. + """ + sig = inspect.signature(mod.register_event_handlers) + accepts_app = "app" in sig.parameters or any( + p.kind is inspect.Parameter.VAR_KEYWORD for p in sig.parameters.values() + ) + if accepts_app: + mod.register_event_handlers(event_bus, app=app) + else: + mod.register_event_handlers(event_bus) diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index d712a5e0..15ade822 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -2,7 +2,6 @@ from __future__ import annotations -import inspect import logging import os from collections.abc import AsyncGenerator @@ -10,6 +9,7 @@ from pathlib import Path from fastapi import FastAPI +from simple_module_core.audit_links import AuditLinkRegistry from simple_module_core.design_packs import DesignPackRegistry from simple_module_core.diagnostics import DiagnosticLevel, print_diagnostics, run_diagnostics from simple_module_core.discovery import discover_modules, select_auth_provider, topological_sort @@ -23,6 +23,7 @@ from simple_module_db.listeners import register_listeners from simple_module_db.session import init_db +from simple_module_hosting._db_health import register_database_check from simple_module_hosting._inertia_setup import setup_inertia from simple_module_hosting._phase_helpers import ( attach_public_routes, @@ -33,6 +34,7 @@ register_host_settings, wire_module_routes, ) +from simple_module_hosting._registrations import run_module_registrations from simple_module_hosting.health import router as health_router from simple_module_hosting.i18n_manifest import build_i18n_registry, emit_frontend_types from simple_module_hosting.migrations import check_migrations @@ -80,22 +82,6 @@ def _resolve_project_root() -> Path: _PROJECT_ROOT = _resolve_project_root() -def _register_event_handlers(mod, event_bus: EventBus, app: FastAPI) -> None: - """Dispatch to ``mod.register_event_handlers`` with or without ``app``. - - Back-compat shim for modules that still override the one-arg form - ``(self, bus)``; passing ``app=`` to those crashes. - """ - sig = inspect.signature(mod.register_event_handlers) - accepts_app = "app" in sig.parameters or any( - p.kind is inspect.Parameter.VAR_KEYWORD for p in sig.parameters.values() - ) - if accepts_app: - mod.register_event_handlers(event_bus, app=app) - else: - mod.register_event_handlers(event_bus) - - def create_app(settings: Settings | None = None) -> FastAPI: """Build and configure the full FastAPI application. @@ -173,6 +159,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: health_registry = HealthRegistry() public_route_registry = PublicRouteRegistry() design_pack_registry = DesignPackRegistry() + audit_link_registry = AuditLinkRegistry() @asynccontextmanager async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: @@ -219,14 +206,18 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: print_diagnostics(settings_diagnostics) # ── Phase 5: Module registrations ────────────────────── - for mod in modules: - mod.register_menu_items(menu_registry) - mod.register_permissions(perm_registry) - mod.register_feature_flags(ff_registry) - _register_event_handlers(mod, event_bus, app) - mod.register_health_checks(health_registry) - mod.register_public_routes(public_route_registry) - mod.register_design_packs(design_pack_registry) + run_module_registrations( + modules, + app=app, + event_bus=event_bus, + menu_registry=menu_registry, + perm_registry=perm_registry, + ff_registry=ff_registry, + health_registry=health_registry, + public_route_registry=public_route_registry, + design_pack_registry=design_pack_registry, + audit_link_registry=audit_link_registry, + ) attach_public_routes(app, settings, public_route_registry) @@ -255,6 +246,9 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: pool_recycle=settings.db_pool_recycle, ) register_listeners(db_state) + # The host's own readiness signal, and the only probe-safe check in a + # default install — module checks reach third parties and are on-demand. + register_database_check(health_registry, db_state) # ── Phase 7: Inertia + exception handlers ────────────── inertia_config = setup_inertia(app, settings, modules, _PROJECT_ROOT) @@ -288,6 +282,7 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: health_registry=health_registry, public_routes=public_route_registry, design_packs=design_pack_registry, + audit_links=audit_link_registry, i18n_registry=i18n_registry, inertia_config=inertia_config, modules=tuple(modules), diff --git a/framework/hosting/simple_module_hosting/health.py b/framework/hosting/simple_module_hosting/health.py index 3abc047a..5e90b875 100644 --- a/framework/hosting/simple_module_hosting/health.py +++ b/framework/hosting/simple_module_hosting/health.py @@ -44,7 +44,11 @@ async def liveness() -> dict: @router.get("/health/ready") async def readiness(request: Request) -> dict: registry: HealthRegistry = request.app.state.sm.health_registry - checks = registry.all_checks + # Probe-safe checks only. Readiness is polled on a timer, and a check that + # opens an SMTP session or hits S3 on that schedule gets rate-limited and + # makes this endpoint's latency someone else's problem. Those dependencies + # are not readiness signals anyway — the app serves pages without them. + checks = registry.probe_checks if not checks: return {_KEY_STATUS: _STATUS_HEALTHY, _KEY_CHECKS: {}} diff --git a/framework/hosting/simple_module_hosting/middleware.py b/framework/hosting/simple_module_hosting/middleware.py index 7d917f50..97b49b1f 100644 --- a/framework/hosting/simple_module_hosting/middleware.py +++ b/framework/hosting/simple_module_hosting/middleware.py @@ -274,6 +274,9 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: "menus": self.menu_registry.get_for_user( is_authenticated=is_authenticated, roles=roles, + # Already expanded above (wildcards resolved), which is exactly + # what the menu filter needs to drop entries that would 403. + permissions=frontend_permissions, ), "i18n": i18n_block, } diff --git a/framework/hosting/tests/test_db_health_check.py b/framework/hosting/tests/test_db_health_check.py new file mode 100644 index 00000000..0499abad --- /dev/null +++ b/framework/hosting/tests/test_db_health_check.py @@ -0,0 +1,40 @@ +"""The host's database readiness check. + +Module-contributed checks reach third parties and are on-demand, so without +this one `/health/ready` would answer "healthy" from an empty check set — a +green light proving nothing. + +It does *not* feed the dashboard's per-module health dot: that maps a check to +a tile by `HealthCheck.module`, and this one is owned by the host ("Host"), +which names no module. With every bundled module check now `probe=False`, the +dots are blank by design — nothing is polling those dependencies. +""" + +from __future__ import annotations + +import httpx +from simple_module_core.health import HealthStatus +from simple_module_hosting._db_health import CHECK_DATABASE + + +class TestDatabaseHealthCheck: + async def test_registered_and_probe_safe(self, app) -> None: + checks = {c.name: c for c in app.state.sm.health_registry.all_checks} + assert CHECK_DATABASE in checks, sorted(checks) + assert checks[CHECK_DATABASE].probe is True + + async def test_readiness_reports_a_real_check(self, client: httpx.AsyncClient) -> None: + """An empty `checks` block was the symptom worth preventing.""" + resp = await client.get("/health/ready") + assert resp.status_code == 200 + body = resp.json() + assert CHECK_DATABASE in body["checks"], body + assert body["checks"][CHECK_DATABASE]["status"] == HealthStatus.HEALTHY.value + + async def test_passes_against_a_live_database(self, app) -> None: + check = next(c for c in app.state.sm.health_registry.all_checks if c.name == CHECK_DATABASE) + assert (await check.check()).status is HealthStatus.HEALTHY + + async def test_probe_checks_is_not_empty(self, app) -> None: + """Readiness reads probe_checks; an always-empty list makes it inert.""" + assert app.state.sm.health_registry.probe_checks diff --git a/framework/hosting/tests/test_error_page_shared_props.py b/framework/hosting/tests/test_error_page_shared_props.py index 68d0efa7..f4ce69e1 100644 --- a/framework/hosting/tests/test_error_page_shared_props.py +++ b/framework/hosting/tests/test_error_page_shared_props.py @@ -61,6 +61,19 @@ async def test_error_page_keeps_its_own_props( props = _inertia_page((await authenticated_client.get(_MISSING_PATH)).text)["props"] assert props["status"] == _NOT_FOUND + async def test_error_page_carries_correlation_id( + self, authenticated_client: httpx.AsyncClient + ) -> None: + """The page shows this id so a support report can be joined to the logs.""" + resp = await authenticated_client.get(_MISSING_PATH) + props = _inertia_page(resp.text)["props"] + assert props.get("correlation_id"), ( + f"no correlation_id on error page; props={sorted(props)}" + ) + # Must be the same id the response header advertises, or quoting it + # back would point support at a different request. + assert props["correlation_id"] == resp.headers.get("x-correlation-id") + async def test_anonymous_error_page_still_renders(self, client: httpx.AsyncClient) -> None: """An unauthenticated 404 must not blow up on missing shared state.""" resp = await client.get("/health/definitely-not-real") diff --git a/host/client_app/i18n.ts b/host/client_app/i18n.ts index 4071b1ab..a2208f50 100644 --- a/host/client_app/i18n.ts +++ b/host/client_app/i18n.ts @@ -2,8 +2,8 @@ * Initial wiring for @simple-module-py/i18n inside the Inertia app. * * Reads {locale, messages} from Inertia shared props and calls - * configureI18n on boot; on every successful navigation, checks whether - * the active locale changed and updates the i18next resources. + * configureI18n on boot; on every successful navigation, adopts whatever + * catalog the server chose to send (see `subscribeI18nToNavigation`). */ import type { PageProps } from '@inertiajs/core'; @@ -25,18 +25,20 @@ export function bootI18nFromInitialPage(props: PageProps): void { return; } configureI18n({ locale: i18n.locale, messages: i18n.messages ?? {} }); - activeLocale = i18n.locale; } -let activeLocale: string | null = null; - export function subscribeI18nToNavigation(): () => void { return router.on('success', (event) => { const i18n = (event.detail.page.props as unknown as { i18n?: I18nSharedProps }).i18n; if (!i18n) return; - if (i18n.locale !== activeLocale && i18n.messages) { + // A non-null `messages` payload IS the server's signal that the client + // needs it — it sends `null` whenever the cached catalog is still good. + // Gating on a locale change instead drops the catalog that arrives when + // the *audience* changes: logging in swaps the public snapshot for one + // including admin-only modules, at the same locale, so every admin screen + // rendered raw keys ("dashboard.home.title") until a hard refresh. + if (i18n.messages) { updateI18n({ locale: i18n.locale, messages: i18n.messages }); - activeLocale = i18n.locale; } }); } diff --git a/host/client_app/pages/Error.tsx b/host/client_app/pages/Error.tsx index 4f40bcab..927e5b4d 100644 --- a/host/client_app/pages/Error.tsx +++ b/host/client_app/pages/Error.tsx @@ -1,5 +1,6 @@ import { Head, Link } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; +import { CopyableId } from '@simple-module-py/ui/components/CopyableId'; import { ErrorScreen } from '@simple-module-py/ui/components/ErrorScreen'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { Home, LifeBuoy } from 'lucide-react'; @@ -7,9 +8,10 @@ import { Home, LifeBuoy } from 'lucide-react'; interface Props { status: number; message: string; + correlation_id?: string; } -function ErrorPage({ status, message }: Props) { +function ErrorPage({ status, message, correlation_id }: Props) { const { t } = useT(); const titles: Record = { @@ -36,7 +38,26 @@ function ErrorPage({ status, message }: Props) { return ( <> - + + + {t(keys.host.error.correlation_id_label)} + + + + ) : undefined + } + > + ); + })} + + ); +} diff --git a/modules/background_tasks/background_tasks/service.py b/modules/background_tasks/background_tasks/service.py index b3711cd0..c5693194 100644 --- a/modules/background_tasks/background_tasks/service.py +++ b/modules/background_tasks/background_tasks/service.py @@ -82,6 +82,26 @@ async def list( task_name=task_name, ) + async def status_counts(self, *, task_name: str | None = None) -> dict[str, int]: + """Count executions per status for the ops strip above the table. + + Deliberately ignores the status filter — the strip is how the operator + picks a status, so it has to keep showing the ones they aren't looking + at. It does honour ``task_name`` so the counts describe the same + result set the table is paging through. + + Statuses with no rows are omitted; callers fill in zeros. + """ + query = select(TaskExecution.status, func.count().label("n")) + if task_name: + query = query.where(TaskExecution.task_name.ilike(f"%{task_name}%")) + query = query.group_by(TaskExecution.status) + + rows = (await self.db.execute(query)).all() + # `status` is a TaskStatus (StrEnum) on Postgres but comes back as a + # plain str on SQLite; normalise so the JSON keys match either way. + return {str(getattr(row[0], "value", row[0])): int(row[1]) for row in rows} + async def get(self, execution_id: uuid.UUID) -> TaskExecutionDetail | None: row = await self.db.get(TaskExecution, execution_id) if row is None: diff --git a/modules/background_tasks/tests/test_bg_service.py b/modules/background_tasks/tests/test_bg_service.py index 8614cdcf..3a33a404 100644 --- a/modules/background_tasks/tests/test_bg_service.py +++ b/modules/background_tasks/tests/test_bg_service.py @@ -99,6 +99,46 @@ async def test_filters_by_task_name_substring( assert [i.task_name for i in resp.items] == ["orders.send_receipt"] +class TestStatusCounts: + """Feeds the failed/stuck ops strip above the executions table.""" + + async def test_counts_are_grouped_by_status( + self, db_session: AsyncSession, service: BackgroundTaskService + ): + for status in (TaskStatus.FAILED, TaskStatus.FAILED, TaskStatus.SUCCESS): + db_session.add(_make_row(status=status)) + await db_session.flush() + + counts = await service.status_counts() + assert counts[TaskStatus.FAILED.value] == 2 + assert counts[TaskStatus.SUCCESS.value] == 1 + + async def test_empty_table_yields_no_counts(self, service: BackgroundTaskService): + assert await service.status_counts() == {} + + async def test_search_narrows_the_counts( + self, db_session: AsyncSession, service: BackgroundTaskService + ): + """The strip must describe the same rows the table is paging through.""" + db_session.add(_make_row(task_name="orders.send_receipt", status=TaskStatus.FAILED)) + db_session.add(_make_row(task_name="users.sync", status=TaskStatus.FAILED)) + await db_session.flush() + + counts = await service.status_counts(task_name="receipt") + assert counts == {TaskStatus.FAILED.value: 1} + + async def test_counts_keys_are_plain_strings( + self, db_session: AsyncSession, service: BackgroundTaskService + ): + """Postgres hands back the enum, SQLite a str — the page needs one shape.""" + db_session.add(_make_row(status=TaskStatus.STUCK)) + await db_session.flush() + + counts = await service.status_counts() + assert all(type(key) is str for key in counts) + assert "stuck" in counts + + class TestGet: async def test_returns_none_for_missing_id(self, service: BackgroundTaskService): assert await service.get(uuid.uuid4()) is None diff --git a/modules/branding/branding/components/BrandingPreview.tsx b/modules/branding/branding/components/BrandingPreview.tsx new file mode 100644 index 00000000..6558cf29 --- /dev/null +++ b/modules/branding/branding/components/BrandingPreview.tsx @@ -0,0 +1,153 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { Card, CardContent, CardHeader, CardTitle } from '@simple-module-py/ui/components/ui/card'; +import type { MenuItem } from '@simple-module-py/ui/types'; + +export type PreviewSeverity = 'info' | 'warning' | 'danger'; + +interface Props { + appName: string; + /** Hex colour from the form, or '' to fall back to the default swatch. */ + color: string; + defaultColor: string; + logoUrl: string | null; + /** Dark-surface logo variant; falls back to `logoUrl` like the real sidebar. */ + logoDarkUrl: string | null; + bannerMessage: string; + bannerSeverity: PreviewSeverity; + /** The viewer's own sidebar entries, so the preview shows a real nav. */ + menuItems: MenuItem[]; +} + +/** + * Mirrors BrandingBanner's severity map. Duplicated rather than imported + * because that component reads the *saved* banner from shared props — the + * whole point here is to render the unsaved form state. + */ +const SEVERITY_CLASS: Record = { + info: 'bg-sky-600 text-white', + warning: 'bg-amber-500 text-black', + danger: 'bg-red-600 text-white', +}; + +const MAX_NAV_ROWS = 5; + +/** + * Live preview of the sidebar and banner as the form is edited. + * + * Previously the preview was a logo tile and the app name, so the two places + * branding is actually most visible — the sidebar every authenticated page + * carries, and the site-wide banner — could only be checked by saving and + * waiting for a full reload. Everything here is driven by form state, so it + * updates as you type and never needs a round trip. + */ +export function BrandingPreview({ + appName, + color, + defaultColor, + logoUrl, + logoDarkUrl, + bannerMessage, + bannerSeverity, + menuItems, +}: Props) { + const { t } = useT(); + const accent = color || defaultColor; + const name = appName || 'SimpleModule'; + const initial = name.trim()[0]?.toUpperCase() ?? 'S'; + // Same fallback the real sidebar uses: no dark variant means the primary + // logo has to hold up against the near-black surface. + const darkLogo = logoDarkUrl ?? logoUrl; + const rows = menuItems.slice(0, MAX_NAV_ROWS); + + return ( + + + {t(keys.branding.manage.preview_title)} + + +
+ {bannerMessage ? ( +
+ {bannerMessage} +
+ ) : ( +
+ {t(keys.branding.manage.preview_no_banner)} +
+ )} + +
+ {/* Sidebar. `bg-app-sidebar` is the same near-black token the real + shell uses, so the dark-variant logo is judged against the + surface it will actually sit on. */} +
+ {/* Rendered inline rather than via BrandingMark: that component + takes its badge colour as a Tailwind class, and the point + here is to show the hex currently in the colour field. */} +
+ {darkLogo ? ( + {name} + ) : ( + + {initial} + + )} + {name} +
+
+ {rows.map((item) => ( +
+ {item.label} +
+ ))} + {rows.length === 0 && ( +
—
+ )} +
+
+ +
+
+
+
+
+ {t(keys.branding.manage.preview_button)} +
+
+
+
+ + {/* The original logo-tile preview: the light-surface logo, which the + sidebar above cannot show because it renders the dark variant. */} +
+
+ {logoUrl ? ( + {name} + ) : ( + {name.trim()[0]?.toUpperCase() ?? 'S'} + )} +
+ {name} +
+ + + ); +} diff --git a/modules/branding/branding/locales/en.json b/modules/branding/branding/locales/en.json index 5b996d6d..2fab0c2d 100644 --- a/modules/branding/branding/locales/en.json +++ b/modules/branding/branding/locales/en.json @@ -47,6 +47,8 @@ "preview_title": "Preview", "saved_toast": "Branding updated", "error_toast": "Could not update branding", - "upload_error_toast": "Could not upload image" + "upload_error_toast": "Could not upload image", + "preview_no_banner": "No banner set", + "preview_button": "Action" } } diff --git a/modules/branding/branding/pages/Manage.tsx b/modules/branding/branding/pages/Manage.tsx index f379faa2..616c2b34 100644 --- a/modules/branding/branding/pages/Manage.tsx +++ b/modules/branding/branding/pages/Manage.tsx @@ -16,6 +16,7 @@ import type { SharedProps } from '@simple-module-py/ui/types'; import { useEffect, useState } from 'react'; import { toast } from 'sonner'; import { BannerField, type BannerSeverity } from '../components/BannerField'; +import { BrandingPreview } from '../components/BrandingPreview'; import { DesignPackField, type DesignPackOption } from '../components/DesignPackField'; import { FooterCard, type FooterPayload } from '../components/FooterCard'; import { ImageField } from '../components/ImageField'; @@ -264,30 +265,16 @@ function Manage() { onSave={saveFooter} /> - - - {t(keys.branding.manage.preview_title)} - - -
-
- {branding?.logoUrl ? ( - {appName} - ) : ( - {(appName.trim()[0] ?? 'S').toUpperCase()} - )} -
- {appName || 'SimpleModule'} -
- - +
diff --git a/modules/dashboard/dashboard/locales/en.json b/modules/dashboard/dashboard/locales/en.json index c422fed4..9b63ed48 100644 --- a/modules/dashboard/dashboard/locales/en.json +++ b/modules/dashboard/dashboard/locales/en.json @@ -15,6 +15,11 @@ }, "welcome_card_title": "Welcome", "welcome_message": "Welcome to SimpleModule", - "description_body": "This is a modular monolith built with FastAPI, Inertia.js, and React. Each module provides its own pages, API endpoints, and database schema." + "description_body": "This is a modular monolith built with FastAPI, Inertia.js, and React. Each module provides its own pages, API endpoints, and database schema.", + "health": { + "healthy": "Healthy", + "degraded": "Degraded", + "unhealthy": "Unhealthy" + } } } diff --git a/modules/dashboard/dashboard/locales/es.json b/modules/dashboard/dashboard/locales/es.json index 117b300c..8c696b99 100644 --- a/modules/dashboard/dashboard/locales/es.json +++ b/modules/dashboard/dashboard/locales/es.json @@ -15,6 +15,11 @@ }, "welcome_card_title": "Bienvenido", "welcome_message": "Bienvenido a SimpleModule", - "description_body": "Este es un monolito modular construido con FastAPI, Inertia.js y React. Cada módulo proporciona sus propias páginas, endpoints de API y esquema de base de datos." + "description_body": "Este es un monolito modular construido con FastAPI, Inertia.js y React. Cada módulo proporciona sus propias páginas, endpoints de API y esquema de base de datos.", + "health": { + "healthy": "Correcto", + "degraded": "Degradado", + "unhealthy": "Con fallos" + } } } diff --git a/modules/dashboard/dashboard/pages/Home.tsx b/modules/dashboard/dashboard/pages/Home.tsx index ca5b9df3..d8c44c8a 100644 --- a/modules/dashboard/dashboard/pages/Home.tsx +++ b/modules/dashboard/dashboard/pages/Home.tsx @@ -5,12 +5,18 @@ import { SectionTitle } from '@simple-module-py/ui/components/SectionTitle'; import { StatCard } from '@simple-module-py/ui/components/StatCard'; import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; +import type { SharedProps } from '@simple-module-py/ui/types'; import { Activity, Box, Stethoscope, Users } from 'lucide-react'; import { DemoPlaceholders } from './components/DemoPlaceholders'; +import { ModuleTile } from './components/ModuleTile'; interface SystemModule { name: string; status: 'loaded'; + /** The module's own screen, or '' when it ships no views. */ + url: string; + /** Worst health status across the module's checks; '' when it registers none. */ + health: '' | 'healthy' | 'degraded' | 'unhealthy'; } interface HealthCheck { @@ -24,6 +30,12 @@ interface SystemInfo { health_checks: HealthCheck[]; } +/** Does `menuUrl` sit at, or below, the route prefix `owner`? */ +function isUnder(menuUrl: string, owner: string): boolean { + const normalized = menuUrl.replace(/\/+$/, ''); + return normalized === owner || normalized.startsWith(`${owner}/`); +} + interface Props { total_users: number; active_users_7d: number; @@ -32,11 +44,66 @@ interface Props { } function Home() { - const props = usePage<{ props: Props }>().props as unknown as Props; + const page = usePage(); + const props = page.props as unknown as Props; + const { menus } = page.props as unknown as SharedProps; const { t } = useT(); const unhealthy = props.system_info.health_checks.filter((c) => c.status !== 'healthy').length; + // The server cannot filter these links per user — the stats payload is + // process-wide cached — so reachability is decided here against the menus + // the middleware already filtered for this session. + // POST entries (Logout) are excluded: the tile renders a GET link, so + // adopting one as a module's target hands the user a 405. + const menuUrls = [ + ...(menus?.sidebar ?? []), + ...(menus?.adminSidebar ?? []), + ...(menus?.navbar ?? []), + ...(menus?.userDropdown ?? []), + ] + .filter((item) => (item.method ?? 'get') === 'get') + .map((item) => item.url); + + // Every module's own prefix, so the fallback below can tell "this entry is + // mine" from "this entry belongs to a module mounted deeper than me". + const modulePrefixes = props.system_info.modules + .map((m) => m.url.replace(/\/+$/, '')) + .filter(Boolean); + + /** + * The menu entry this module's tile should open, or '' when the user has + * none. + * + * Matching the view prefix exactly is not enough: a module often mounts its + * landing screen below its own prefix (Users is `/users`, its menu entry is + * `/users/admin`), and an exact match leaves those tiles permanently inert + * for admins who can in fact open them. So fall back to the first menu entry + * that lives under the prefix — but only if no *other* module owns a longer + * prefix of that entry, or a module mounted at `/admin` would adopt the + * background-tasks entry at `/admin/background-tasks` and link its tile to + * somebody else's screen. + */ + function menuTarget(url: string): string { + if (!url) return ''; + const prefix = url.replace(/\/+$/, ''); + const exact = menuUrls.find((menuUrl) => menuUrl.replace(/\/+$/, '') === prefix); + if (exact) return exact; + return ( + menuUrls.find( + (menuUrl) => + isUnder(menuUrl, prefix) && + !modulePrefixes.some((other) => other.length > prefix.length && isUnder(menuUrl, other)), + ) ?? '' + ); + } + + const healthLabels: Record = { + healthy: t(keys.dashboard.home.health.healthy), + degraded: t(keys.dashboard.home.health.degraded), + unhealthy: t(keys.dashboard.home.health.unhealthy), + }; + return ( <> @@ -84,17 +151,19 @@ function Home() { System
- {props.system_info.modules.map((m) => ( -
- - - {m.name} -
- ))} + {props.system_info.modules.map((m) => { + const target = menuTarget(m.url); + return ( + + ); + })}
diff --git a/modules/dashboard/dashboard/pages/components/ModuleTile.tsx b/modules/dashboard/dashboard/pages/components/ModuleTile.tsx new file mode 100644 index 00000000..9972e525 --- /dev/null +++ b/modules/dashboard/dashboard/pages/components/ModuleTile.tsx @@ -0,0 +1,65 @@ +import { Link } from '@inertiajs/react'; +import { Box, ChevronRight } from 'lucide-react'; + +export type ModuleHealth = '' | 'healthy' | 'degraded' | 'unhealthy'; + +interface Props { + name: string; + /** The module's own screen. Empty, or not in the user's menus, renders inert. */ + url: string; + health: ModuleHealth; + /** False when the module ships no screen this user is allowed to open. */ + reachable: boolean; + healthLabel?: string; +} + +const DOT: Record, string> = { + healthy: 'bg-primary', + degraded: 'bg-amber-500', + unhealthy: 'bg-red-500', +}; + +const BASE = + 'flex items-center gap-2 rounded-lg border border-border bg-card px-3 py-2 text-left w-full'; + +export function ModuleTile({ name, url, health, reachable, healthLabel }: Props) { + const body = ( + <> + + + {name} + {health ? ( + <> +
- {group.permissions.map((key, i) => { - const fromRole = inheritedSet.has(key); - const checked = directSet.has(key); - return ( - - ); - })} + {group.permissions.map((key, i) => ( + + ))}
); diff --git a/modules/permissions/permissions/pages/components/PermissionRow.tsx b/modules/permissions/permissions/pages/components/PermissionRow.tsx new file mode 100644 index 00000000..1a600343 --- /dev/null +++ b/modules/permissions/permissions/pages/components/PermissionRow.tsx @@ -0,0 +1,83 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { Badge } from '@simple-module-py/ui/components/ui/badge'; +import { Switch } from '@simple-module-py/ui/components/ui/switch'; +import { Check, Minus } from 'lucide-react'; + +interface Props { + permissionKey: string; + /** Granted directly to this user — the only thing the switch controls. */ + direct: boolean; + /** Roles granting this key, empty when none do. */ + viaRoles: string[]; + onToggle: (key: string, checked: boolean) => void; + className?: string; +} + +/** + * One permission, showing *effective* access separately from the direct grant. + * + * The switch reflects only the direct grant, which is correct — it is the only + * thing this form can change. Previously that was also the row's only signal, + * so a permission the user genuinely holds through a role rendered as "off". + * The leading indicator now answers "does this user have it?" and the switch + * answers "is it granted here?", which are different questions. + */ +export function PermissionRow({ + permissionKey, + direct, + viaRoles, + onToggle, + className = '', +}: Props) { + const { t } = useT(); + const inherited = viaRoles.length > 0; + const effective = direct || inherited; + const switchId = `perm-${permissionKey}`; + + return ( +
+ + + + {permissionKey} + + + {inherited && ( + + {t(keys.permissions.user_edit.via_role, { role: viaRoles[0] })} + {viaRoles.length > 1 ? ` +${viaRoles.length - 1}` : ''} + + )} + + onToggle(permissionKey, c === true)} + aria-label={t(keys.permissions.user_edit.direct_toggle_label, { key: permissionKey })} + title={t(keys.permissions.user_edit.direct_toggle_label, { key: permissionKey })} + /> +
+ ); +} diff --git a/modules/permissions/permissions/service.py b/modules/permissions/permissions/service.py index 7601618c..5755440a 100644 --- a/modules/permissions/permissions/service.py +++ b/modules/permissions/permissions/service.py @@ -176,6 +176,7 @@ async def get_user_permissions(self, user_id: uuid.UUID) -> UserPermissionsOut | roles=sorted(role_names), direct=sorted(direct), inherited=sorted(inherited), + inherited_by=self._resolve_role_sources(role_names), ) async def set_user_permissions( @@ -212,6 +213,7 @@ async def set_user_permissions( roles=sorted(role_names), direct=sorted(wanted), inherited=sorted(inherited), + inherited_by=self._resolve_role_sources(role_names), ) # ── Effective-permissions resolution ─────────────────────── @@ -229,6 +231,24 @@ def _resolve_role_permissions(self, role_names: list[str]) -> set[str]: resolved.update(perms) return resolved + def _resolve_role_sources(self, role_names: list[str]) -> dict[str, list[str]]: + """Map each inherited permission key to the roles that grant it. + + "Inherited" alone doesn't tell an admin what to change — they need to + know *which* role to edit. Two roles can grant the same key, so the + value is a list. + """ + from simple_module_core.permissions import WILDCARD + + role_map = self.registry.role_map + sources: dict[str, list[str]] = {} + for name in sorted(role_names): + perms = role_map.get(name, []) + keys = self.registry.all_permissions if WILDCARD in perms else perms + for key in keys: + sources.setdefault(key, []).append(name) + return sources + async def resolve_effective_permissions(self, user_id: uuid.UUID) -> set[str]: """Return every permission key the user holds (role-inherited + direct).""" user = await self._get_user(user_id) diff --git a/modules/permissions/tests/test_permissions_inheritance_sources.py b/modules/permissions/tests/test_permissions_inheritance_sources.py new file mode 100644 index 00000000..5eca8c49 --- /dev/null +++ b/modules/permissions/tests/test_permissions_inheritance_sources.py @@ -0,0 +1,92 @@ +"""Naming which role grants an inherited permission. + +The user-grants screen drove its switch off `direct` alone, so a permission +the user genuinely holds through a role rendered exactly like one they did +not hold. `inherited_by` gives each row its source, which is also what tells +an admin which role to edit if they want the permission gone. +""" + +from __future__ import annotations + +from permissions.service import PermissionService +from simple_module_core.permissions import WILDCARD, PermissionRegistry +from sqlalchemy.ext.asyncio import AsyncSession + + +def _registry() -> PermissionRegistry: + reg = PermissionRegistry() + reg.add_group("Products", ["products.view", "products.create"]) + reg.add_group("Settings", ["settings.read", "settings.manage"]) + return reg + + +def _service(db_session: AsyncSession, reg: PermissionRegistry) -> PermissionService: + return PermissionService(db_session, reg) + + +class TestResolveRoleSources: + def test_maps_each_key_to_its_granting_role(self, db_session: AsyncSession): + reg = _registry() + reg.map_role("editor", ["products.view", "products.create"]) + svc = _service(db_session, reg) + + sources = svc._resolve_role_sources(["editor"]) + assert sources["products.view"] == ["editor"] + assert sources["products.create"] == ["editor"] + + def test_two_roles_granting_one_key_both_appear(self, db_session: AsyncSession): + """Revoking via one role would not be enough; the admin needs both.""" + reg = _registry() + reg.map_role("editor", ["products.view"]) + reg.map_role("viewer", ["products.view"]) + svc = _service(db_session, reg) + + assert svc._resolve_role_sources(["viewer", "editor"])["products.view"] == [ + "editor", + "viewer", + ] + + def test_wildcard_role_claims_every_registered_key(self, db_session: AsyncSession): + """An admin role holds everything — each row must still say why.""" + reg = _registry() + reg.map_role("admin", [WILDCARD]) + svc = _service(db_session, reg) + + sources = svc._resolve_role_sources(["admin"]) + assert set(sources) == set(reg.all_permissions) + assert sources["settings.manage"] == ["admin"] + + def test_no_roles_yields_nothing(self, db_session: AsyncSession): + assert _service(db_session, _registry())._resolve_role_sources([]) == {} + + def test_unknown_role_contributes_nothing(self, db_session: AsyncSession): + assert _service(db_session, _registry())._resolve_role_sources(["ghost"]) == {} + + +class TestUserPermissionsPayload: + async def test_inherited_by_covers_keys_that_are_also_direct( + self, db_session: AsyncSession + ) -> None: + """`inherited` drops direct duplicates; `inherited_by` must not, or a + key granted both ways looks purely direct and revoking it silently + leaves the role grant in place.""" + from users.constants import USER_ROLE_ID + from users.models import Role, User + + reg = _registry() + reg.map_role("editor", ["products.view"]) + + role = Role(id=USER_ROLE_ID, name="editor", description="") + user = User(email="e@example.com", hashed_password="x", is_active=True) + user.roles = [role] + db_session.add_all([role, user]) + await db_session.flush() + + svc = _service(db_session, reg) + await svc.set_user_permissions(user.id, ["products.view"]) + out = await svc.get_user_permissions(user.id) + + assert out is not None + assert "products.view" in out.direct + assert "products.view" not in out.inherited + assert out.inherited_by["products.view"] == ["editor"] diff --git a/modules/settings/settings/_module_settings.py b/modules/settings/settings/_module_settings.py index ad3c8176..5940396a 100644 --- a/modules/settings/settings/_module_settings.py +++ b/modules/settings/settings/_module_settings.py @@ -6,6 +6,7 @@ from __future__ import annotations +import os import re from dataclasses import dataclass from typing import Any @@ -41,6 +42,37 @@ class ModuleSettingField: type: str requires_restart: bool group: str | None + env_set: bool = False + """This field is genuinely env-readable *and* its env var is set. + + Deliberately not "the ``SM_*`` label below is present in ``os.environ``". + The bundled module settings classes declare no ``env_prefix`` — they are + constructed from pydantic defaults and hydrated from the DB — so their + ``SM_*`` vars are never consulted. Reporting a leftover + ``SM_USERS_SMTP_HOST`` as the live source would invert the very question + this screen answers. Classes that *do* declare one (the host's ``Settings`` + with ``SM_``, and anything from the module scaffold) report ``env`` for + real, because for them pydantic really does read it. + """ + db_override: bool = False + """A stored setting overrides this field.""" + + @property + def source(self) -> str: + """Where the live value came from: ``db``, ``env`` or ``default``. + + Mirrors the precedence in ``hydrate_settings``: DB overrides are passed + to the constructor explicitly, so they beat anything pydantic reads for + fields left unset, which in turn beats the field default. ``env`` only + appears for settings classes that actually declare an ``env_prefix``; + see :attr:`env_set`. Showing this is the difference between "why is + this not taking effect" being a five-minute question and an afternoon. + """ + if self.db_override: + return "db" + if self.env_set: + return "env" + return "default" @dataclass(frozen=True, slots=True) @@ -93,16 +125,43 @@ def _resolve_default(info) -> Any: return None -def _field_view(name: str, settings: BaseSettings, prefix: str) -> ModuleSettingField: +def _env_readable_var(settings: BaseSettings, name: str) -> str | None: + """Env var pydantic would actually read for ``name``, or ``None``. + + ``env_var`` on the view is a *label* — the ``SM__`` name the + ``smpy settings import-from-env`` CLI looks for, kept from before settings + moved into the DB. It is not evidence that pydantic reads it: the bundled + module settings classes declare ``SettingsConfigDict(extra="ignore")`` with + no ``env_prefix``, so ``SM_FILE_STORAGE_BACKEND`` has no effect on + ``FileStorageSettings()``. Deriving env-readability from the class's own + ``env_prefix`` keeps the "From environment" badge honest, and works as-is + for the classes that do declare one — the host's ``Settings`` (``SM_``) and + every module built from the scaffold, whose template ships + ``env_prefix="SM__"``. + """ + env_prefix = str(type(settings).model_config.get("env_prefix") or "") + if not env_prefix: + return None + return f"{env_prefix}{name.upper()}" + + +def _field_view( + name: str, + settings: BaseSettings, + prefix: str, + overridden: frozenset[str] = frozenset(), +) -> ModuleSettingField: cls = type(settings) info = cls.model_fields[name] raw_value = getattr(settings, name) secret = is_secret_field(name) extra = info.json_schema_extra if isinstance(info.json_schema_extra, dict) else {} default = _resolve_default(info) + env_var = f"{prefix}{name.upper()}" + live_env_var = _env_readable_var(settings, name) return ModuleSettingField( name=name, - env_var=f"{prefix}{name.upper()}", + env_var=env_var, value=_mask(raw_value) if secret else raw_value, default=_mask(default) if secret else default, description=info.description or "", @@ -110,16 +169,26 @@ def _field_view(name: str, settings: BaseSettings, prefix: str) -> ModuleSetting type=value_type_for_field(cls, name), requires_restart=bool(extra.get("requires_restart", False)), group=extra.get("group"), + env_set=live_env_var is not None and live_env_var in os.environ, + db_override=name in overridden, ) -def collect_module_settings(app: FastAPI) -> list[ModuleSettingsView]: +def collect_module_settings( + app: FastAPI, + overrides: dict[str, frozenset[str]] | None = None, +) -> list[ModuleSettingsView]: """Return a sorted, serializable view of every module's BaseSettings. Folds in both ``app.state.sm.modules`` (plugin modules) and additional packages registered via ``app.state.settings.module_registry`` (e.g. ``"host"``) that aren't backed by a ``ModuleBase`` instance. + + ``overrides`` maps package -> field names carrying a stored override. It + is passed in rather than read here because fetching it is async and this + function is not; callers without it get ``db_override=False`` throughout. """ + by_package = overrides or {} views: list[ModuleSettingsView] = [] seen: set[str] = set() @@ -128,7 +197,7 @@ def collect_module_settings(app: FastAPI) -> list[ModuleSettingsView]: settings = _extract_settings(app, package) if settings is None: continue - views.append(_build_view(mod.meta.name, package, settings)) + views.append(_build_view(mod.meta.name, package, settings, by_package)) seen.add(package) settings_services = getattr(app.state, "settings", None) @@ -140,16 +209,24 @@ def collect_module_settings(app: FastAPI) -> list[ModuleSettingsView]: settings = _extract_settings(app, package) if settings is None: continue - views.append(_build_view(package.title(), package, settings)) + views.append(_build_view(package.title(), package, settings, by_package)) seen.add(package) views.sort(key=lambda v: v.module_name) return views -def _build_view(module_name: str, package: str, settings: BaseSettings) -> ModuleSettingsView: +def _build_view( + module_name: str, + package: str, + settings: BaseSettings, + overrides: dict[str, frozenset[str]] | None = None, +) -> ModuleSettingsView: prefix = env_prefix_for(package) - fields = [_field_view(name, settings, prefix) for name in type(settings).model_fields] + overridden = (overrides or {}).get(package, frozenset()) + fields = [ + _field_view(name, settings, prefix, overridden) for name in type(settings).model_fields + ] return ModuleSettingsView( module_name=module_name, package=package, @@ -178,6 +255,9 @@ def serialize(views: list[ModuleSettingsView]) -> list[dict[str, Any]]: "type": f.type, "requires_restart": f.requires_restart, "group": f.group, + "env_set": f.env_set, + "db_override": f.db_override, + "source": f.source, } for f in v.fields ], diff --git a/modules/settings/settings/constants.py b/modules/settings/settings/constants.py index b389f628..69dc9cf1 100644 --- a/modules/settings/settings/constants.py +++ b/modules/settings/settings/constants.py @@ -48,6 +48,12 @@ VIEW_CREATE_PATH: Final = "/create" VIEW_EDIT_PATH: Final = "/{setting_id}/edit" VIEW_MODULES_PATH: Final = "/modules" +"""Legacy path for the per-module forms. Those now live at the section root; +this redirects, so existing links and bookmarks keep working.""" + +VIEW_STORE_PATH: Final = "/store" +"""Raw key/value store. Demoted from the section root: it is a database view, +and an admin looking for "settings" almost always wants the module forms.""" API_BY_ID_PATH: Final = "/{setting_id}" API_BY_KEY_PATH: Final = "/by-key/{key}" API_RESOLVE_PATH: Final = "/resolve/{key}" diff --git a/modules/settings/settings/endpoints/views.py b/modules/settings/settings/endpoints/views.py index 4de333f9..29b8d3e3 100644 --- a/modules/settings/settings/endpoints/views.py +++ b/modules/settings/settings/endpoints/views.py @@ -9,16 +9,21 @@ from __future__ import annotations -from fastapi import APIRouter, Depends, Request +from fastapi import APIRouter, Depends, HTTPException, Request from inertia import InertiaResponse from pydantic import ValidationError 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 from starlette.responses import RedirectResponse -from settings._module_settings import collect_module_settings, serialize +from settings._module_settings import _package_of, collect_module_settings, serialize from settings.constants import ( ERR_SETTING_NOT_FOUND, + PERM_CREATE, + PERM_DELETE, + PERM_EDIT, + PERM_VIEW, PROP_ERROR, PROP_MODULES, PROP_SETTING, @@ -26,6 +31,7 @@ VIEW_CREATE_PATH, VIEW_EDIT_PATH, VIEW_MODULES_PATH, + VIEW_STORE_PATH, ) from settings.contracts.schemas import SettingCreate, SettingUpdate from settings.deps import get_setting_service @@ -36,16 +42,30 @@ _PAGE_EDIT = "Settings/Edit" _PAGE_MODULES_EDIT = "Settings/ModulesEdit" -_REDIRECT_SETTINGS = "/settings" +# Row-level actions return to the raw store they were performed in, not to +# the module forms that now own the section root. +_REDIRECT_SETTINGS = "/settings/store" +_REDIRECT_MODULES = "/settings/" -router = APIRouter() +# Every screen in this section reads configuration: module field values, +# their env var names, and now which of the two is in force. The matching JSON +# API (``/api/settings/...``) has always required ``settings.view``, so leaving +# these unguarded let any signed-in account read the same data by asking for +# the page instead. Mutating routes add their own stricter guard on top. +router = APIRouter(dependencies=[Depends(RequiresPermission(PERM_VIEW))]) -@router.get("/", response_model=None) +@router.get(VIEW_STORE_PATH, response_model=None) async def browse( inertia: InertiaDep, service: SettingService = Depends(get_setting_service), ) -> InertiaResponse: + """The raw key/value store. + + Moved off the section root: it is a database view, and an admin who clicks + "Settings" is nearly always after a module's form, not a table of rows + keyed by dotted strings. + """ items = await service.list_all() return await inertia.render( _PAGE_BROWSE, @@ -53,9 +73,38 @@ async def browse( ) +@router.get(VIEW_MODULES_PATH, response_model=None) +async def modules_redirect() -> RedirectResponse: + """The per-module forms moved to the section root; keep old links alive.""" + return RedirectResponse(_REDIRECT_MODULES, status_code=308) + + @router.get(VIEW_CREATE_PATH, response_model=None) -async def create_view(inertia: InertiaDep) -> InertiaResponse: - return await inertia.render(_PAGE_CREATE) +async def create_view(request: Request, inertia: InertiaDep) -> InertiaResponse: + return await inertia.render(_PAGE_CREATE, {"known_keys": _known_keys(request)}) + + +def _known_keys(request: Request) -> list[dict[str, str]]: + """Every ``.`` a module actually reads, for autocomplete. + + 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. + """ + suggestions: list[dict[str, str]] = [] + for view in collect_module_settings(request.app): + for field in view.fields: + suggestions.append( + { + "key": f"{view.package}.{field.name}", + "type": field.type, + "description": field.description, + "module": view.module_name, + } + ) + return sorted(suggestions, key=lambda s: s["key"]) @router.get(VIEW_EDIT_PATH, response_model=None) @@ -73,7 +122,13 @@ async def edit_view( # ── Form actions (POST/PUT/DELETE → redirect) ───────────────── -@router.post("/", response_model=None) +# Posts to the store collection, which is where the rows live now that the +# section root renders the module forms. +@router.post( + VIEW_STORE_PATH, + response_model=None, + dependencies=[Depends(RequiresPermission(PERM_CREATE))], +) async def create_action( request: Request, service: SettingService = Depends(get_setting_service), @@ -87,7 +142,11 @@ async def create_action( return RedirectResponse(_REDIRECT_SETTINGS, status_code=303) -@router.put("/{setting_id}", response_model=None) +@router.put( + "/{setting_id}", + response_model=None, + dependencies=[Depends(RequiresPermission(PERM_EDIT))], +) async def update_action( setting_id: int, request: Request, @@ -102,7 +161,11 @@ async def update_action( return RedirectResponse(_REDIRECT_SETTINGS, status_code=303) -@router.delete("/{setting_id}", response_model=None) +@router.delete( + "/{setting_id}", + response_model=None, + dependencies=[Depends(RequiresPermission(PERM_DELETE))], +) async def delete_action( setting_id: int, service: SettingService = Depends(get_setting_service), @@ -111,14 +174,96 @@ async def delete_action( return RedirectResponse(_REDIRECT_SETTINGS, status_code=303) -@router.get(VIEW_MODULES_PATH, response_model=None) -async def modules_view(request: Request, inertia: InertiaDep) -> InertiaResponse: +@router.get("/", response_model=None) +async def modules_view( + request: Request, + inertia: InertiaDep, + service: SettingService = Depends(get_setting_service), +) -> InertiaResponse: """Read-only view of every module's pydantic ``BaseSettings`` instance. Auto-discovered from ``app.state.sm.modules``; secrets are masked server-side. + Each field also reports where its live value came from — a stored override, + an ``SM_*`` env var, or the field default — so a setting that "isn't taking + effect" explains itself. """ - views = collect_module_settings(request.app) + overrides = await _overrides_by_package(service) + views = collect_module_settings(request.app, overrides) return await inertia.render( _PAGE_MODULES_EDIT, - {PROP_MODULES: serialize(views)}, + { + PROP_MODULES: serialize(views), + # Which packages can be connection-tested, so the page only offers + # the button where something is actually reachable. + "testable": _testable_packages(request), + }, + ) + + +async def _overrides_by_package(service: SettingService) -> dict[str, frozenset[str]]: + """Map package -> field names carrying a stored override. + + Reads the SYSTEM scope once and buckets by key prefix. Packages with no + overrides are simply absent, which ``collect_module_settings`` already + treats as "nothing overridden". + """ + from settings.store import SettingsStore + + return await SettingsStore(service).all_override_fields() + + +def _testable_packages(request: Request) -> list[str]: + """Packages whose module registered at least one health check. + + "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. + """ + registry = request.app.state.sm.health_registry + owners = {c.module for c in registry.all_checks if c.module} + return sorted( + { + _package_of(mod) + for mod in getattr(request.app.state.sm, "modules", ()) + if mod.meta.name in owners + } ) + + +@router.post( + "/test-connection/{package}", + response_model=None, + # Guarded, unlike the read-only view routes around it: this one makes the + # server open outbound connections on demand (SMTP AUTH, S3) and hands the + # raw failure text — hostnames, bucket names, auth errors — back to the + # caller. Only someone allowed to change these settings should be able to. + dependencies=[Depends(RequiresPermission(PERM_EDIT))], +) +async def test_connection(package: str, request: Request) -> dict: + """Run one module's health checks now and report each result. + + Returns 200 with per-check results even when a check fails: an admin + testing a connection expects to read the failure, not to get an error + status with the reason buried. + """ + modules = getattr(request.app.state.sm, "modules", ()) + owner = next((m for m in modules if _package_of(m) == package), None) + if owner is None: + raise HTTPException(status_code=404, detail=f"Unknown module package: {package}") + + checks = [ + c for c in request.app.state.sm.health_registry.all_checks if c.module == owner.meta.name + ] + if not checks: + raise HTTPException(status_code=404, detail=f"{owner.meta.name} has no connection to test") + + results = [] + for check in checks: + try: + outcome = await check.check() + results.append( + {"name": check.name, "status": outcome.status.value, "detail": outcome.detail or ""} + ) + except Exception as exc: + results.append({"name": check.name, "status": "unhealthy", "detail": str(exc)}) + return {"module": owner.meta.name, "checks": results} diff --git a/modules/settings/settings/locales/en.json b/modules/settings/settings/locales/en.json index 37f018de..a06cdd7e 100644 --- a/modules/settings/settings/locales/en.json +++ b/modules/settings/settings/locales/en.json @@ -39,7 +39,8 @@ "value_label": "Value", "value_placeholder": "Enter a value", "description_label": "Description", - "description_placeholder": "Optional description" + "description_placeholder": "Optional description", + "key_unknown_warning": "No installed module declares this key. It will be stored, but nothing will read it unless a module looks it up." }, "create": { "title": "New Setting", @@ -66,6 +67,16 @@ "value": "Value", "default": "Default", "description": "Description" - } + }, + "test_connection": "Test connection", + "testing": "Testing…", + "env_var_hint": "Environment variable name for this field, used by \"smpy settings import-from-env\"", + "source_db": "Stored override", + "source_db_hint": "A stored setting supplies this value", + "source_db_over_env": "Stored (shadows env)", + "source_db_shadows_env": "A stored setting overrides {env_var}; the environment value is ignored", + "source_env": "From environment", + "source_env_hint": "{env_var} is set in this deployment", + "source_default": "Default" } } diff --git a/modules/settings/settings/module.py b/modules/settings/settings/module.py index b60e0c97..c9e64187 100644 --- a/modules/settings/settings/module.py +++ b/modules/settings/settings/module.py @@ -6,6 +6,7 @@ from pathlib import Path from fastapi import APIRouter, FastAPI +from simple_module_core.audit_links import AuditLink, AuditLinkRegistry from simple_module_core.menu import MenuItem, MenuRegistry, MenuSection from simple_module_core.module import ModuleBase, ModuleMeta from simple_module_core.permissions import PermissionRegistry @@ -21,6 +22,7 @@ MODULE_NAME, MODULE_PACKAGE, PERM_GROUP, + PERM_VIEW, VIEW_PREFIX, ) @@ -67,12 +69,27 @@ def register_menu_items(self, registry: MenuRegistry) -> None: order=MENU_ORDER, section=MenuSection.SIDEBAR, group="System", + # Mirrors the view router's guard, so the entry is not offered + # to accounts whose click would 403. + permissions=[PERM_VIEW], ) ) def register_permissions(self, registry: PermissionRegistry) -> None: registry.add_group(PERM_GROUP, list(ALL_PERMISSIONS)) + def register_audit_links(self, registry: AuditLinkRegistry) -> None: + from settings.models import Setting + + registry.register( + AuditLink( + # Class name, not __tablename__ — see AuditLink.entity_type. + entity_type=Setting.__name__, + url_template=f"{VIEW_PREFIX}/{{id}}/edit", + label="Setting", + ) + ) + def locale_dirs(self) -> dict[str, Path]: base = Path(str(importlib.resources.files(__package__) / "locales")) return {LOCALE_NAMESPACE: base} diff --git a/modules/settings/settings/pages/Create.tsx b/modules/settings/settings/pages/Create.tsx index e4def08d..3e553961 100644 --- a/modules/settings/settings/pages/Create.tsx +++ b/modules/settings/settings/pages/Create.tsx @@ -15,13 +15,16 @@ import { import { Textarea } from '@simple-module-py/ui/components/ui/textarea'; import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; import type React from 'react'; +import { KeyField, type KnownKey } from './components/KeyField'; import ValueInput, { VALUE_TYPES, type ValueType } from './components/ValueInput'; import { ROUTES } from './routes'; const SCOPES = ['system', 'tenant', 'user'] as const; type Scope = (typeof SCOPES)[number]; -function Create() { +type Props = { known_keys?: KnownKey[] }; + +function Create({ known_keys }: Props) { const { t } = useT(); const { data, setData, post, processing, errors } = useForm({ scope: 'system' as Scope, @@ -83,19 +86,18 @@ function Create() { {errors.scope_id &&

{errors.scope_id}

}
-
- - setData('key', e.target.value)} - required - placeholder={t(keys.settings.form.key_placeholder)} - className="font-mono" - /> - {errors.key &&

{errors.key}

} -
+ { + setData((prev) => ({ + ...prev, + key, + ...(type ? { value_type: type as ValueType } : {}), + })); + }} + />
- +
+ {testable && } + +
{Object.entries(grouped).map(([group, fields]) => ( @@ -128,6 +137,7 @@ export function ModuleForm({ module: m }: Props) { Requires restart )} +
(null); + const [error, setError] = useState(null); + + async function run() { + setBusy(true); + setError(null); + setResults(null); + try { + const resp = await fetch(ROUTES.testConnection(pkg), { method: 'POST' }); + if (!resp.ok) throw new Error(resp.statusText); + const body = await resp.json(); + setResults(body.checks ?? []); + } catch (err) { + setError((err as Error).message); + } finally { + setBusy(false); + } + } + + return ( +
+ + + {error &&

{error}

} + + {results?.map((result) => { + const ok = result.status === 'healthy'; + return ( +

+ {ok ? : } + {/* The reason is the whole point: "connection refused" and + "authentication failed" need different fixes. */} + {result.detail || result.status} +

+ ); + })} +
+ ); +} diff --git a/modules/settings/settings/pages/routes.ts b/modules/settings/settings/pages/routes.ts index 18a9ba0e..1c76cb1e 100644 --- a/modules/settings/settings/pages/routes.ts +++ b/modules/settings/settings/pages/routes.ts @@ -1,7 +1,10 @@ export const ROUTES = { - browse: '/settings', - modules: '/settings/modules', + /** Per-module forms — the section root, and where "Settings" now lands. */ + modules: '/settings/', + /** Raw key/value store, demoted from the root. */ + browse: '/settings/store', create: '/settings/create', edit: (id: number) => `/settings/${id}/edit`, byId: (id: number) => `/settings/${id}`, + testConnection: (pkg: string) => `/settings/test-connection/${pkg}`, } as const; diff --git a/modules/settings/settings/store.py b/modules/settings/settings/store.py index a7d9f7d3..9940bdc0 100644 --- a/modules/settings/settings/store.py +++ b/modules/settings/settings/store.py @@ -35,6 +35,22 @@ async def get_overrides(self, package: str) -> dict[str, tuple[str, str]]: out[field_name] = (item.value, item.value_type) return out + async def all_override_fields(self) -> dict[str, frozenset[str]]: + """Return ``{package: {field_name, ...}}`` for every stored override. + + One query for the whole screen. ``get_overrides`` re-reads the entire + SYSTEM scope per package, so calling it in a loop over the installed + modules is one full read per module for the same rows. + """ + items = await self._service.list_by_scope(SettingScope.SYSTEM, SYSTEM_SCOPE_ID) + out: dict[str, set[str]] = {} + for item in items: + package, sep, field_name = item.key.partition(".") + if not sep or not field_name or "." in field_name: + continue + out.setdefault(package, set()).add(field_name) + return {package: frozenset(fields) for package, fields in out.items()} + async def set_override(self, package: str, field: str, value: str, value_type: str) -> None: await self._service.upsert_scoped( SettingScope.SYSTEM, diff --git a/modules/settings/tests/test_module_settings.py b/modules/settings/tests/test_module_settings.py index ab71e154..29c9410c 100644 --- a/modules/settings/tests/test_module_settings.py +++ b/modules/settings/tests/test_module_settings.py @@ -49,6 +49,7 @@ def test_collect_exposes_type_requires_restart_group(): health_registry=None, # type: ignore[arg-type] public_routes=None, # type: ignore[arg-type] design_packs=None, # type: ignore[arg-type] + audit_links=None, # type: ignore[arg-type] i18n_registry=None, # type: ignore[arg-type] inertia_config=None, # type: ignore[arg-type] modules=(_DemoModule(),), # type: ignore[arg-type] diff --git a/modules/settings/tests/test_settings_field_sources.py b/modules/settings/tests/test_settings_field_sources.py new file mode 100644 index 00000000..0bcaaca5 --- /dev/null +++ b/modules/settings/tests/test_settings_field_sources.py @@ -0,0 +1,122 @@ +"""Where a module setting's live value actually came from. + +The module-settings screen listed a value and its env var name but never said +which one was in force, so "I set SM_USERS_SMTP_HOST and nothing changed" +(because a stored override shadowed it) was invisible on the screen. +""" + +from __future__ import annotations + +import pytest +from settings._module_settings import ModuleSettingField + + +def _first_field(instance) -> str: + """First declared field name. Kept out of the async tests — a bare next() + raising StopIteration inside a coroutine surfaces as an unrelated + RuntimeError.""" + names = list(type(instance).model_fields) + assert names, f"{type(instance).__name__} declares no fields" + return names[0] + + +def _field(**overrides) -> ModuleSettingField: + base = { + "name": "smtp_host", + "env_var": "SM_USERS_SMTP_HOST", + "value": "mail.example.com", + "default": "localhost", + "description": "", + "is_secret": False, + "type": "string", + "requires_restart": False, + "group": None, + } + return ModuleSettingField(**{**base, **overrides}) + + +class TestFieldSource: + def test_plain_field_reports_default(self): + assert _field().source == "default" + + def test_env_var_present_reports_env(self): + assert _field(env_set=True).source == "env" + + def test_stored_override_reports_db(self): + assert _field(db_override=True).source == "db" + + def test_db_override_beats_env(self): + """Mirrors hydrate_settings: DB values are passed to the constructor, + so pydantic never consults the environment for that field.""" + assert _field(env_set=True, db_override=True).source == "db" + + +class TestModulesView: + async def test_fields_carry_their_source(self, authenticated_client): + resp = await authenticated_client.get("/settings/", follow_redirects=False) + assert resp.status_code == 200 + + def test_env_var_presence_is_detected(self, monkeypatch: pytest.MonkeyPatch): + """A class that really reads env must not read as 'Default'.""" + from pydantic_settings import BaseSettings, SettingsConfigDict + from settings._module_settings import _field_view + + class _EnvBacked(BaseSettings): + model_config = SettingsConfigDict(env_prefix="SM_DEMO_", extra="ignore") + + host: str = "localhost" + + assert _field_view("host", _EnvBacked(), "SM_DEMO_").env_set is False + monkeypatch.setenv("SM_DEMO_HOST", "mail.example.com") + view = _field_view("host", _EnvBacked(), "SM_DEMO_") + assert view.env_set is True + assert view.source == "env" + # The claim has to be true, not just consistent with the label. + assert view.value == "mail.example.com" + + def test_unread_env_var_is_not_claimed_as_the_source(self, monkeypatch: pytest.MonkeyPatch): + """Module settings declare no ``env_prefix`` — they come from defaults + plus DB overrides — so a leftover ``SM_*`` var changes nothing. Badging + it "From environment" would invert the question this screen answers.""" + from file_storage.settings import FileStorageSettings + from settings._module_settings import _field_view + + name = _first_field(FileStorageSettings()) + monkeypatch.setenv(f"SM_FILE_STORAGE_{name.upper()}", "definitely-not-a-backend") + + instance = FileStorageSettings() + assert getattr(instance, name) != "definitely-not-a-backend" + assert _field_view(name, instance, "SM_FILE_STORAGE_").env_set is False + assert _field_view(name, instance, "SM_FILE_STORAGE_").source == "default" + + def test_overrides_mark_their_fields(self): + from file_storage.settings import FileStorageSettings + from settings._module_settings import _field_view + + instance = FileStorageSettings() + name = _first_field(instance) + view = _field_view(name, instance, "SM_FILE_STORAGE_", frozenset({name})) + assert view.db_override is True + assert view.source == "db" + + +class TestTestConnectionEndpoint: + async def test_unknown_package_is_a_404(self, authenticated_client): + resp = await authenticated_client.post("/settings/test-connection/nosuchmodule") + assert resp.status_code == 404 + + async def test_module_without_checks_is_a_404(self, authenticated_client): + """Only modules that registered a check can be tested.""" + resp = await authenticated_client.post("/settings/test-connection/settings") + assert resp.status_code == 404 + + async def test_failing_check_still_returns_200_with_the_reason(self, authenticated_client): + """An admin testing a connection needs to read the failure, not get an + error status with the reason buried.""" + resp = await authenticated_client.post("/settings/test-connection/file_storage") + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["checks"], body + for check in body["checks"]: + assert check["status"] in ("healthy", "degraded", "unhealthy") + assert "detail" in check diff --git a/modules/settings/tests/test_settings_view_authz.py b/modules/settings/tests/test_settings_view_authz.py new file mode 100644 index 00000000..ccac1cbc --- /dev/null +++ b/modules/settings/tests/test_settings_view_authz.py @@ -0,0 +1,61 @@ +"""The settings screens must be as guarded as the API behind them. + +``/api/settings/modules`` has always required ``settings.view``, but the Inertia +screens rendering the same data carried no permission dependency — so any +signed-in account could read every module's configuration (values, env var +names, and which of the two is in force) by asking for the page instead. +""" + +from __future__ import annotations + +import httpx +import pytest +from simple_module_test.fixtures import forge_session_cookie + +_VIEW_ROUTES = ["/settings/", "/settings/store", "/settings/create"] + + +@pytest.fixture +async def plain_user_client(app): + """A signed-in account holding no settings permission.""" + from users.models import User + + async with app.state.sm.db.session_factory() as session: + user = User( + email="plain@example.com", + hashed_password="x", + is_active=True, + is_verified=True, + ) + session.add(user) + await session.commit() + user_id = str(user.id) + + signed = forge_session_cookie(app.state.sm.settings.secret_key, {"user_id": user_id}) + transport = httpx.ASGITransport(app=app) + async with httpx.AsyncClient( + transport=transport, base_url="http://testserver", cookies={"session": signed} + ) as client: + yield client + + +@pytest.mark.parametrize("path", _VIEW_ROUTES) +async def test_view_routes_reject_a_user_without_settings_view( + plain_user_client: httpx.AsyncClient, path: str +): + resp = await plain_user_client.get(path, follow_redirects=False) + assert resp.status_code in (302, 401, 403), resp.text[:400] + + +async def test_the_api_and_the_screen_agree(plain_user_client: httpx.AsyncClient): + """Same data, same answer — the gap between them was the bug.""" + api = await plain_user_client.get("/api/settings/modules", follow_redirects=False) + view = await plain_user_client.get("/settings/", follow_redirects=False) + assert api.status_code in (302, 401, 403) + assert view.status_code in (302, 401, 403) + + +@pytest.mark.parametrize("path", _VIEW_ROUTES) +async def test_admins_still_reach_every_screen(authenticated_client: httpx.AsyncClient, path: str): + resp = await authenticated_client.get(path, follow_redirects=False) + assert resp.status_code == 200, resp.text[:400] diff --git a/modules/users/tests/test_users_bulk_invite.py b/modules/users/tests/test_users_bulk_invite.py new file mode 100644 index 00000000..7bc3b9e5 --- /dev/null +++ b/modules/users/tests/test_users_bulk_invite.py @@ -0,0 +1,229 @@ +"""Bulk invite — many addresses per submit, with per-address outcomes. + +The invite form took one address at a time, so onboarding a team meant +repeating the form once per person. Partial success is the normal case here: +one already-registered address must not discard the rest. +""" + +from __future__ import annotations + +import httpx +import pytest + +_URL = "/api/users/admin/invite/bulk" + + +class TestBulkInvite: + async def test_invites_every_address(self, authenticated_client: httpx.AsyncClient): + resp = await authenticated_client.post( + _URL, + json={"emails": ["a@example.com", "b@example.com"], "role_names": []}, + ) + assert resp.status_code == 200, resp.text + results = resp.json()["results"] + assert [r["email"] for r in results] == ["a@example.com", "b@example.com"] + + async def test_duplicate_addresses_are_invited_once( + self, authenticated_client: httpx.AsyncClient + ): + """Pasting a list with a repeat should not mint two invites for it.""" + resp = await authenticated_client.post( + _URL, + json={"emails": ["dup@example.com", "DUP@example.com"], "role_names": []}, + ) + assert len(resp.json()["results"]) == 1 + + async def test_addresses_are_normalised_to_lowercase( + self, authenticated_client: httpx.AsyncClient + ): + resp = await authenticated_client.post( + _URL, json={"emails": ["Mixed@Example.com"], "role_names": []} + ) + assert resp.json()["results"][0]["email"] == "mixed@example.com" + + async def test_one_failure_does_not_discard_the_others( + self, authenticated_client: httpx.AsyncClient + ): + """A duplicate in a pasted list of twenty must not lose the other 19.""" + await authenticated_client.post( + _URL, json={"emails": ["taken@example.com"], "role_names": []} + ) + resp = await authenticated_client.post( + _URL, + json={"emails": ["taken@example.com", "fresh@example.com"], "role_names": []}, + ) + assert resp.status_code == 200, resp.text + by_email = {r["email"]: r for r in resp.json()["results"]} + assert by_email["taken@example.com"]["status"] == "failed" + assert by_email["fresh@example.com"]["status"] in ("sent", "link") + + async def test_a_failure_always_carries_a_reason(self, authenticated_client: httpx.AsyncClient): + """Every failed row must say why. + + ``str(UserAlreadyExists())`` is empty — the reason lives in the type, not + the message — so passing it through rendered an address in red with a + blank reason beside it. Asserting the status alone is what let that ship. + """ + await authenticated_client.post( + _URL, json={"emails": ["why@example.com"], "role_names": []} + ) + resp = await authenticated_client.post( + _URL, json={"emails": ["why@example.com"], "role_names": []} + ) + result = resp.json()["results"][0] + assert result["status"] == "failed" + assert result["detail"], "a failed address must explain itself" + assert "already" in result["detail"].lower() + + async def test_console_mailer_hands_back_a_copyable_link( + self, authenticated_client: httpx.AsyncClient + ): + """The test app uses the console mailer, which delivers nothing — the + admin needs the link or the invite is undeliverable.""" + resp = await authenticated_client.post( + _URL, json={"emails": ["linkme@example.com"], "role_names": []} + ) + result = resp.json()["results"][0] + assert result["status"] == "link" + assert "/users/invite/accept?token=" in result["link"] + + async def test_roles_apply_to_every_address(self, authenticated_client: httpx.AsyncClient): + resp = await authenticated_client.post( + _URL, + json={"emails": ["r1@example.com", "r2@example.com"], "role_names": ["user"]}, + ) + assert resp.status_code == 200, resp.text + assert all(r["status"] in ("sent", "link") for r in resp.json()["results"]) + + async def test_roles_survive_a_later_failure(self, authenticated_client: httpx.AsyncClient): + """A failure mid-list must not roll back an earlier invite's roles. + + ``invite`` only flushes its role rows; the rollback that clears failed + transaction state used to take them with it, so the person invited just + before a duplicate address ended up with none of the chosen roles. + """ + await authenticated_client.post( + _URL, json={"emails": ["dupe@example.com"], "role_names": []} + ) + resp = await authenticated_client.post( + _URL, + json={"emails": ["kept@example.com", "dupe@example.com"], "role_names": ["admin"]}, + ) + assert resp.status_code == 200, resp.text + + listing = await authenticated_client.get("/api/users/admin") + users = {u["email"]: u for u in listing.json()} + assert users["kept@example.com"]["roles"] == ["admin"] + + async def test_a_malformed_address_does_not_reject_the_submit( + self, authenticated_client: httpx.AsyncClient + ): + """A typo in one line of a pasted column used to 422 the whole body, + leaving the admin with a generic error and no idea which line.""" + resp = await authenticated_client.post( + _URL, + json={"emails": ["good@example.com", "not-an-address"], "role_names": []}, + ) + assert resp.status_code == 200, resp.text + by_email = {r["email"]: r for r in resp.json()["results"]} + assert by_email["good@example.com"]["status"] in ("sent", "link") + assert by_email["not-an-address"]["status"] == "failed" + assert "valid email" in by_email["not-an-address"]["detail"] + + async def test_empty_list_is_accepted_and_does_nothing( + self, authenticated_client: httpx.AsyncClient + ): + resp = await authenticated_client.post(_URL, json={"emails": [], "role_names": []}) + assert resp.status_code == 200 + assert resp.json()["results"] == [] + + async def test_address_count_is_capped(self, authenticated_client: httpx.AsyncClient): + """One submit must not be able to mint unbounded live invite tokens.""" + from users.admin.bulk_invite import MAX_ADDRESSES + + emails = [f"bulk{i}@example.com" for i in range(MAX_ADDRESSES + 5)] + resp = await authenticated_client.post(_URL, json={"emails": emails, "role_names": []}) + invited = [r for r in resp.json()["results"] if r["status"] != "failed"] + assert len(invited) == MAX_ADDRESSES + + async def test_addresses_over_the_cap_are_reported_not_dropped( + self, authenticated_client: httpx.AsyncClient + ): + """Silent truncation reports "100 invites sent" while 5 people are + never contacted, with nothing on screen saying so.""" + from users.admin.bulk_invite import MAX_ADDRESSES + + emails = [f"over{i}@example.com" for i in range(MAX_ADDRESSES + 5)] + resp = await authenticated_client.post(_URL, json={"emails": emails, "role_names": []}) + results = resp.json()["results"] + + assert len(results) == MAX_ADDRESSES + 5, "every submitted address needs an outcome" + overflow = {r["email"]: r for r in results[-5:]} + assert set(overflow) == { + f"over{i}@example.com" for i in range(MAX_ADDRESSES, MAX_ADDRESSES + 5) + } + for result in overflow.values(): + assert result["status"] == "failed" + assert "limit" in result["detail"] + + async def test_requires_authentication(self, client: httpx.AsyncClient): + resp = await client.post( + _URL, json={"emails": ["x@example.com"], "role_names": []}, follow_redirects=False + ) + assert resp.status_code in (302, 401, 403) + + +class TestInvitePreview: + async def test_accept_page_names_the_invitee( + self, authenticated_client: httpx.AsyncClient, client: httpx.AsyncClient + ): + """The card asked for a password while identifying nobody.""" + created = await authenticated_client.post( + _URL, json={"emails": ["preview@example.com"], "role_names": []} + ) + link = created.json()["results"][0]["link"] + token = link.split("token=")[1] + + resp = await client.get( + f"/users/invite/accept?token={token}", + headers={"X-Inertia": "true", "Accept": "application/json"}, + ) + assert resp.status_code == 200, resp.text + invite = resp.json()["props"]["invite"] + assert invite["email"] == "preview@example.com" + assert invite["already_accepted"] is False + + @pytest.mark.parametrize("token", ["", "not-a-jwt", "a.b.c"]) + async def test_unreadable_tokens_yield_no_preview(self, client: httpx.AsyncClient, token: str): + """Expired, tampered and absent all look the same here on purpose — + the reason belongs to the accept attempt, which validates properly.""" + resp = await client.get( + f"/users/invite/accept?token={token}", + headers={"X-Inertia": "true", "Accept": "application/json"}, + ) + assert resp.status_code == 200 + assert resp.json()["props"]["invite"] is None + + async def test_preview_does_not_consume_the_invite( + self, authenticated_client: httpx.AsyncClient, client: httpx.AsyncClient + ): + """Viewing the page must leave the token usable — UserManager.verify + marks the account verified as a side effect, so the preview cannot + route through it.""" + created = await authenticated_client.post( + _URL, json={"emails": ["unspent@example.com"], "role_names": []} + ) + token = created.json()["results"][0]["link"].split("token=")[1] + + for _ in range(2): + resp = await client.get( + f"/users/invite/accept?token={token}", + headers={"X-Inertia": "true", "Accept": "application/json"}, + ) + assert resp.json()["props"]["invite"]["already_accepted"] is False + + accepted = await client.post( + "/api/users/auth/accept-invite", + json={"token": token, "password": "a-good-password-123"}, + ) + assert accepted.status_code in (200, 204), accepted.text diff --git a/modules/users/tests/test_views_admin.py b/modules/users/tests/test_views_admin.py index 82391419..5162f27d 100644 --- a/modules/users/tests/test_views_admin.py +++ b/modules/users/tests/test_views_admin.py @@ -129,23 +129,58 @@ async def test_flag_false_when_not_installed(self, admin_client, users_app, user # --------------------------------------------------------------------------- -# Admin create page +# Admin add-people page (create + invite merged behind a mode switch) # --------------------------------------------------------------------------- -class TestAdminCreatePage: +class TestAdminAddPeoplePage: @pytest.mark.anyio - async def test_create_page_renders_with_roles(self, admin_client): + async def test_add_page_renders_with_roles(self, admin_client): resp = await admin_client.get( - "/users/admin/create", + "/users/admin/add", headers={"X-Inertia": "true", "Accept": "application/json"}, ) assert resp.status_code == 200 data = resp.json() - assert data["component"] == "Users/Users/Create" + assert data["component"] == "Users/Users/AddPeople" assert "roles" in data["props"] @pytest.mark.anyio - async def test_create_page_requires_auth(self, anon_client): - resp = await anon_client.get("/users/admin/create", follow_redirects=False) + async def test_add_page_reports_whether_mail_can_be_delivered(self, admin_client): + """Drives the copy-link panel — the page has to know before submitting.""" + resp = await admin_client.get( + "/users/admin/add", + headers={"X-Inertia": "true", "Accept": "application/json"}, + ) + assert "mailer_delivers" in resp.json()["props"] + + @pytest.mark.anyio + async def test_no_mailer_does_not_promise_delivery(self, admin_client, app): + """With nothing able to send, the page must offer the copy-link panel — + claiming delivery is the one answer that is certainly wrong.""" + original = app.state.users.mailer + app.state.users.mailer = None + try: + resp = await admin_client.get( + "/users/admin/add", + headers={"X-Inertia": "true", "Accept": "application/json"}, + ) + assert resp.json()["props"]["mailer_delivers"] is False + finally: + app.state.users.mailer = original + + @pytest.mark.anyio + async def test_add_page_requires_auth(self, anon_client): + resp = await anon_client.get("/users/admin/add", follow_redirects=False) assert resp.status_code == 302 + + @pytest.mark.anyio + @pytest.mark.parametrize( + ("old_path", "mode"), + [("/users/admin/create", "create"), ("/users/admin/invite", "invite")], + ) + async def test_old_urls_redirect_into_the_right_mode(self, admin_client, old_path, mode): + """Existing links must land on the merged form with their mode preselected.""" + resp = await admin_client.get(old_path, follow_redirects=False) + assert resp.status_code == 307 + assert resp.headers["location"] == f"/users/admin/add?mode={mode}" diff --git a/modules/users/users/admin/api.py b/modules/users/users/admin/api.py index 2a8e2b14..bb68f1af 100644 --- a/modules/users/users/admin/api.py +++ b/modules/users/users/admin/api.py @@ -10,6 +10,7 @@ from simple_module_core.events import EventBus from simple_module_hosting.permissions import RequiresPermission +from users.admin.bulk_invite import bulk_router from users.admin.service import UserService from users.constants import PERM_USERS_MANAGE, sanitize_list_filters from users.contracts.events import ( @@ -40,6 +41,10 @@ tags=["users-admin"], ) +# Bulk invite lives in its own module (this file is near the 300-line cap) but +# mounts here so it inherits the users.manage guard above. +admin_router.include_router(bulk_router) + @admin_router.get("", response_model=list[UserListItem]) async def admin_list_users( diff --git a/modules/users/users/admin/bulk_invite.py b/modules/users/users/admin/bulk_invite.py new file mode 100644 index 00000000..e45938b1 --- /dev/null +++ b/modules/users/users/admin/bulk_invite.py @@ -0,0 +1,219 @@ +"""Bulk invite — one submit, many addresses. + +The invite form took a single address, so onboarding a team meant repeating +the same form once per person. This accepts a pasted list and reports each +address separately: one already-registered address in a list of twenty must +not discard the other nineteen. +""" + +from __future__ import annotations + +import contextlib +import logging + +from fastapi import APIRouter, Depends, Request +from fastapi_users.exceptions import UserAlreadyExists +from pydantic import EmailStr, TypeAdapter, ValidationError +from simple_module_core.events import EventBus +from simple_module_db.deps import get_db +from sqlalchemy.ext.asyncio import AsyncSession + +from users.admin.service import UserService +from users.contracts.events import UserInvited +from users.contracts.schemas import BulkInviteResponse, BulkInviteResult, UserBulkInvite +from users.deps import get_event_bus, get_mailer, get_user_service + +logger = logging.getLogger(__name__) + +bulk_router = APIRouter() + +STATUS_SENT = "sent" +STATUS_LINK = "link" +STATUS_FAILED = "failed" + +MAX_ADDRESSES = 100 +"""Enough for a team, small enough that one submit cannot mint an unbounded +number of live invite tokens.""" + +_EMAIL = TypeAdapter(EmailStr) +"""Per-address validation. Deliberately not a ``list[EmailStr]`` on the request +model: pydantic would reject the whole body over one typo, and the caller would +get a 422 naming an index rather than the per-address outcomes this endpoint +exists to produce.""" + + +def _failure_detail(exc: Exception) -> str: + """Human-readable reason for one address failing. + + ``str(UserAlreadyExists())`` is the empty string — fastapi-users carries the + meaning in the exception *type*, not its message. Passing that straight + through renders the single most common failure as a bare red address with no + reason beside it, which is exactly the question the per-address results exist + to answer. The final fallback names the exception class rather than leaving + the cell blank: a class name is a poor message, but it is still a lead. + """ + if isinstance(exc, UserAlreadyExists): + return "Already registered" + return str(exc) or type(exc).__name__ + + +def _invite_link(request: Request, token: str) -> str: + """Build the accept URL the admin will hand to the invitee. + + Uses the module's configured ``base_url`` — the same value both mailers + build their links from — rather than ``request.base_url``. Behind a reverse + proxy without ``SM_TRUSTED_PROXY``, the request's own base URL is the + internal origin (``http://10.0.0.5:8000``), and this link is *only* ever + surfaced when mail could not be delivered, i.e. exactly when the admin has + to pass it on by hand. Falls back to the request when the module's settings + are unavailable, so the link is never simply missing. + """ + services = getattr(request.app.state, "users", None) + configured = getattr(getattr(services, "settings", None), "base_url", "") + base = str(configured or request.base_url).rstrip("/") + return f"{base}/users/invite/accept?token={token}" + + +@bulk_router.post("/invite/bulk", response_model=BulkInviteResponse) +async def admin_bulk_invite( + data: UserBulkInvite, + request: Request, + bus: EventBus = Depends(get_event_bus), + service: UserService = Depends(get_user_service), + # The same request-scoped session the service was built from, taken through + # the dependency rather than off the service's private attribute. + db: AsyncSession = Depends(get_db), + mailer=Depends(get_mailer), +) -> BulkInviteResponse: + """Invite every address in *data*, all sharing the same roles.""" + invited_by = getattr(request.state, "user", None) + invited_by_name = invited_by.name if invited_by else "Administrator" + + # Absent attribute means "assume it delivers" — a third-party mailer must + # never leak invite tokens into the response just by not declaring itself. + # No mailer at all delivers nothing, though: defaulting that to True sends + # every address down the send path, where the AttributeError surfaces to + # the admin as a raw "'NoneType' object has no attribute 'send_invite'". + # This matches what the page's ``mailer_delivers`` prop already reports. + delivers = mailer is not None and getattr(mailer, "delivers_email", True) + + # Preserve submit order but drop repeats: pasting a list with the same + # address twice should not create two invites for it. A malformed address + # is reported like any other per-address failure rather than rejecting the + # submit — a typo in one line of a pasted column must not lose the column. + seen: set[str] = set() + ordered: list[str] = [] + malformed: list[str] = [] + for raw in data.emails: + email = str(raw).strip().lower() + if not email or email in seen: + continue + seen.add(email) + try: + _EMAIL.validate_python(email) + except ValidationError: + malformed.append(email) + continue + ordered.append(email) + + # Anything past the cap is reported, never silently dropped: truncating in + # silence tells the admin "100 invites sent" while 50 people are never + # contacted, and nothing on screen says otherwise. + accepted, overflow = ordered[:MAX_ADDRESSES], ordered[MAX_ADDRESSES:] + + results: list[BulkInviteResult] = [] + for email in accepted: + try: + user, token = await service.invite(email, None, data.role_names, invited_by=invited_by) + # Make this invite durable before touching the session again. The + # failure path below rolls back, and the role rows the service + # flushed but did not commit would go with it — leaving the person + # invited with none of the roles the admin picked. + await db.commit() + except Exception as exc: + # Already-registered is the common case and reads fine as-is; + # anything else is logged so the admin's summary stays short. + logger.info("bulk invite failed for %s: %s", email, exc) + results.append( + BulkInviteResult(email=email, status=STATUS_FAILED, detail=_failure_detail(exc)) + ) + # Clear any failed transaction state before touching the session + # again. A real DB error (an IntegrityError from a concurrent + # signup, say) otherwise leaves every remaining address dying with + # PendingRollbackError — one bad row turning into total failure, + # the opposite of the partial success this endpoint promises. + # Safe to discard: every successful invite above is committed + # before the next one starts, so nothing durable is pending here. + with contextlib.suppress(Exception): + await db.rollback() + continue + + if delivers: + try: + await mailer.send_invite(user.email, token, invited_by_name) + except Exception as exc: + # The account exists and the token is valid — the delivery + # failed. Handing back the link turns a dead end into a + # copy-paste, rather than stranding a half-finished invite. + logger.warning("invite mail failed for %s: %s", email, exc) + results.append( + BulkInviteResult( + email=email, + status=STATUS_LINK, + detail=str(exc), + link=_invite_link(request, token), + ) + ) + else: + results.append(BulkInviteResult(email=email, status=STATUS_SENT)) + else: + results.append( + BulkInviteResult( + email=email, + status=STATUS_LINK, + link=_invite_link(request, token), + ) + ) + + await bus.publish( + UserInvited( + user_id=user.id, + email=user.email, + invited_by=(str(invited_by.id) if invited_by else None), + ) + ) + # Handlers are awaited inline and may write through this same + # request-scoped session. Commit again so a *later* address failing — + # and rolling back to clear the transaction — cannot take their work + # with it, which is what makes the "nothing durable is pending" + # assumption in the failure branch above actually hold. + # + # Guarded: the invite itself is already committed, so a handler writing + # something the DB rejects must not 500 the request and discard every + # per-address result collected so far — that is exactly the total + # failure this endpoint exists to avoid. + try: + await db.commit() + except Exception as exc: + logger.warning("post-invite handler commit failed for %s: %s", email, exc) + with contextlib.suppress(Exception): + await db.rollback() + + results.extend( + BulkInviteResult( + email=email, + status=STATUS_FAILED, + detail="Not a valid email address", + ) + for email in malformed + ) + results.extend( + BulkInviteResult( + email=email, + status=STATUS_FAILED, + detail=f"Not attempted — over the {MAX_ADDRESSES}-address limit for one submit", + ) + for email in overflow + ) + + return BulkInviteResponse(results=results) diff --git a/modules/users/users/admin/views.py b/modules/users/users/admin/views.py index 5b070c5d..4a0013d3 100644 --- a/modules/users/users/admin/views.py +++ b/modules/users/users/admin/views.py @@ -8,6 +8,7 @@ from inertia import InertiaResponse from simple_module_hosting.inertia_deps import InertiaDep from simple_module_hosting.permissions import RequiresPermission +from starlette.responses import RedirectResponse from users.admin.service import UserService from users.constants import PERM_USERS_MANAGE, sanitize_list_filters @@ -18,8 +19,7 @@ router = APIRouter() _PAGE_ADMIN_INDEX = "Users/Users/Index" -_PAGE_ADMIN_INVITE = "Users/Users/Invite" -_PAGE_ADMIN_CREATE = "Users/Users/Create" +_PAGE_ADMIN_ADD = "Users/Users/AddPeople" _PAGE_ADMIN_EDIT = "Users/Users/Edit" @@ -81,37 +81,53 @@ async def admin_index( @router.get( - "/admin/invite", + "/admin/add", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) -async def admin_invite_page( +async def admin_add_people_page( request: Request, inertia: InertiaDep, ) -> InertiaResponse: + """One screen for both ways of adding people, chosen by a mode switch. + + Create and invite were separate pages reached from separate buttons, which + made an admin decide between them before seeing what either involved. They + take almost the same inputs and differ in one respect — who sets the + password — so the choice belongs inside the form. + """ + mailer = getattr(getattr(request.app.state, "users", None), "mailer", None) return await inertia.render( - _PAGE_ADMIN_INVITE, + _PAGE_ADMIN_ADD, { "roles": await _roles_payload(request.app), + # Drives the copy-link panel: when nothing can be delivered, the + # invite mode has to hand the link back instead. No mailer at all + # delivers nothing — promising delivery there would be the one + # answer that is certainly wrong. + "mailer_delivers": bool(mailer is not None and getattr(mailer, "delivers_email", True)), }, ) +@router.get( + "/admin/invite", + response_model=None, + dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], +) +async def admin_invite_redirect() -> RedirectResponse: + """Old invite URL — the flow merged into /users/admin/add.""" + return RedirectResponse("/users/admin/add?mode=invite", status_code=307) + + @router.get( "/admin/create", response_model=None, dependencies=[Depends(RequiresPermission(PERM_USERS_MANAGE))], ) -async def admin_create_page( - request: Request, - inertia: InertiaDep, -) -> InertiaResponse: - return await inertia.render( - _PAGE_ADMIN_CREATE, - { - "roles": await _roles_payload(request.app), - }, - ) +async def admin_create_redirect() -> RedirectResponse: + """Old create URL — the flow merged into /users/admin/add.""" + return RedirectResponse("/users/admin/add?mode=create", status_code=307) @router.get( diff --git a/modules/users/users/auth_local/invite_preview.py b/modules/users/users/auth_local/invite_preview.py new file mode 100644 index 00000000..3e916d8f --- /dev/null +++ b/modules/users/users/auth_local/invite_preview.py @@ -0,0 +1,64 @@ +"""Read an invite token without spending it. + +The accept-invite card asked for a password while showing neither who the +invite was for nor what access it grants. Someone forwarded a link, or holding +two invites to different deployments, had no way to tell them apart — and no +way to notice an invite addressed to the wrong person before accepting it. + +``UserManager.verify`` cannot answer this: it marks the account verified as a +side effect, so calling it to peek would consume the invite. The verification +token is a JWT carrying ``sub`` and ``email``, so decoding it read-only gives +the same facts with no side effects. +""" + +from __future__ import annotations + +import logging +from typing import Any + +import jwt +from fastapi_users.jwt import decode_jwt + +logger = logging.getLogger(__name__) + + +async def preview_invite(token: str, user_manager: Any) -> dict[str, Any] | None: + """Return ``{"email", "roles"}`` for *token*, or ``None`` if unreadable. + + ``None`` covers expired, tampered, and wrong-audience tokens alike. The + page deliberately does not distinguish them: the reason belongs to the + accept attempt, which validates properly. + """ + if not token: + return None + + try: + data = decode_jwt( + token, + user_manager.verification_token_secret, + [user_manager.verification_token_audience], + ) + except jwt.PyJWTError: + return None + + email = data.get("email") + if not email: + return None + + roles: list[str] = [] + try: + user = await user_manager.get_by_email(email) + except Exception: + # The token decoded but the account is gone. Showing the address it + # was issued for is still more useful than showing nothing; accepting + # will fail with a proper message. + return {"email": email, "roles": roles, "already_accepted": False} + + roles = sorted(role.name for role in getattr(user, "roles", []) or []) + return { + "email": email, + "roles": roles, + # An invite that has already been used should say so, rather than + # presenting a password form that is guaranteed to fail. + "already_accepted": bool(getattr(user, "is_verified", False)), + } diff --git a/modules/users/users/auth_local/views.py b/modules/users/users/auth_local/views.py index eafaec1f..cf694526 100644 --- a/modules/users/users/auth_local/views.py +++ b/modules/users/users/auth_local/views.py @@ -2,12 +2,14 @@ from __future__ import annotations -from fastapi import APIRouter, HTTPException, Request +from fastapi import APIRouter, Depends, HTTPException, Request from inertia import InertiaResponse from simple_module_hosting.inertia_deps import InertiaDep from starlette.responses import RedirectResponse +from users.auth_local.invite_preview import preview_invite from users.bootstrap import resolve_bootstrap_credentials +from users.manager import UserManager, get_user_manager router = APIRouter() @@ -95,8 +97,20 @@ async def verify_page(inertia: InertiaDep, token: str = "") -> InertiaResponse: @router.get("/invite/accept", response_model=None) -async def accept_invite_page(inertia: InertiaDep, token: str = "") -> InertiaResponse: - return await inertia.render(_PAGE_ACCEPT_INVITE, {"token": token}) +async def accept_invite_page( + inertia: InertiaDep, + user_manager: UserManager = Depends(get_user_manager), + token: str = "", +) -> InertiaResponse: + """Show who the invite is for and what it grants, before asking for a password.""" + invite = await preview_invite(token, user_manager) + return await inertia.render( + _PAGE_ACCEPT_INVITE, + { + "token": token, + "invite": invite, + }, + ) @router.get("/me", response_model=None) diff --git a/modules/users/users/contracts/schemas.py b/modules/users/users/contracts/schemas.py index 31e9d27c..f0b4a0d1 100644 --- a/modules/users/users/contracts/schemas.py +++ b/modules/users/users/contracts/schemas.py @@ -7,7 +7,14 @@ from fastapi_users.schemas import CreateUpdateDictModel from pydantic import ConfigDict, EmailStr -from sqlmodel import SQLModel +from sqlmodel import Field, SQLModel + +MAX_BULK_INVITE_BODY_ADDRESSES = 1000 +"""Hard ceiling on the address list one bulk-invite body may carry. + +Distinct from ``bulk_invite.MAX_ADDRESSES`` (how many invites one submit may +actually mint): this bounds the work and the response, both of which are one +entry per submitted address.""" # NOTE on EmailStr: only *input* schemas (UserCreate/UserUpdate/UserInvite) use # EmailStr — that is where an email must be format-validated. Response schemas @@ -57,6 +64,45 @@ class UserInvite(SQLModel): role_names: list[str] = [] +class UserBulkInvite(SQLModel): + """Invite several addresses in one submit, all sharing the same roles.""" + + emails: list[str] = Field(max_length=MAX_BULK_INVITE_BODY_ADDRESSES) + """Raw addresses, validated one at a time by the endpoint rather than by + ``list[EmailStr]`` here: a single typo in a pasted column would otherwise + 422 the whole submit, and the caller would get an error naming a list index + instead of the per-address outcomes this endpoint exists to report. + + The length bound is on the *body*, not the invite cap: the endpoint reports + an outcome for every address it is handed, so an unbounded list means + unbounded per-address validation and an equally unbounded response. Set far + above the invite cap so the "over the limit" outcomes stay visible for any + plausible paste.""" + role_names: list[str] = [] + + +class BulkInviteResult(SQLModel): + """Outcome for a single address in a bulk invite. + + Per-address rather than all-or-nothing: one already-registered address in + a pasted list of twenty should not discard the other nineteen. + """ + + email: str + status: str + """``"sent"`` — mail dispatched. ``"link"`` — created, but the configured + mailer cannot deliver, so ``link`` carries the URL. ``"failed"`` — see + ``detail``.""" + detail: str = "" + link: str | None = None + """One-time accept URL. Populated only when the mailer cannot deliver; + otherwise the token stays out of the response entirely.""" + + +class BulkInviteResponse(SQLModel): + results: list[BulkInviteResult] + + class UserAdminCreate(SQLModel): email: EmailStr password: str diff --git a/modules/users/users/health.py b/modules/users/users/health.py new file mode 100644 index 00000000..7af1f420 --- /dev/null +++ b/modules/users/users/health.py @@ -0,0 +1,47 @@ +"""Health check for the configured mailer. + +Doubles as the "Test connection" action on the module-settings screen: an +admin who has just typed SMTP credentials needs a way to find out they are +wrong that is cheaper than triggering a password reset and waiting. +""" + +from __future__ import annotations + +from fastapi import FastAPI +from simple_module_core.health import HealthCheckResult, HealthStatus + +CHECK_MAILER = "users.mailer" + + +def build_mailer_check(app: FastAPI): + """Return an async check closing over *app* so it re-reads live settings. + + Bound to the app rather than a mailer instance because settings are + hydrated from the DB and can change after boot — a check pinned to the + boot-time mailer would keep testing credentials the admin has replaced. + """ + + async def check() -> HealthCheckResult: + services = getattr(app.state, "users", None) + mailer = getattr(services, "mailer", None) + if mailer is None: + return HealthCheckResult(status=HealthStatus.UNHEALTHY, detail="No mailer configured") + + verify = getattr(mailer, "verify_connection", None) + if verify is None: + # The console mailer writes links to the log; there is nothing to + # reach, so it is healthy by construction rather than untested. + return HealthCheckResult( + status=HealthStatus.HEALTHY, + detail=f"{type(mailer).__name__} needs no connection", + ) + + try: + await verify() + except Exception as exc: + # The reason matters more than the traceback: "authentication + # failed" and "connection refused" call for different fixes. + return HealthCheckResult(status=HealthStatus.UNHEALTHY, detail=str(exc)) + return HealthCheckResult(status=HealthStatus.HEALTHY, detail="SMTP reachable") + + return check diff --git a/modules/users/users/mailer/console.py b/modules/users/users/mailer/console.py index b5a434fc..65b32d3b 100644 --- a/modules/users/users/mailer/console.py +++ b/modules/users/users/mailer/console.py @@ -12,6 +12,15 @@ class ConsoleMailer: + delivers_email = False + """Nothing leaves the process — links only reach the log. + + Callers that need the recipient to actually receive something (the bulk + invite screen) read this to decide whether to surface the one-time link + in the UI instead. Absence of the attribute means "assume it delivers", + so a third-party mailer never leaks tokens by omission. + """ + def __init__(self, base_url: str, app_name_provider: AppNameProvider | None = None) -> None: self._base = base_url.rstrip("/") from users.mailer import default_app_name diff --git a/modules/users/users/mailer/smtp.py b/modules/users/users/mailer/smtp.py index a6a065e7..b6121617 100644 --- a/modules/users/users/mailer/smtp.py +++ b/modules/users/users/mailer/smtp.py @@ -2,6 +2,7 @@ from __future__ import annotations +import contextlib import importlib.resources from email.message import EmailMessage from typing import TYPE_CHECKING @@ -71,6 +72,27 @@ async def send_invite(self, email: str, token: str, invited_by_name: str) -> Non body = template.render(link=link, invited_by_name=invited_by_name, app_name=app) await self._send(email, f"{invited_by_name} invited you to {app}", body) + async def verify_connection(self) -> None: + """Open an SMTP session and authenticate, then hang up. + + Deliberately stops short of sending anything: an admin checking their + mailer config should not put a stray message in someone's inbox. This + catches the failures that actually happen — wrong host or port, TLS + mismatch, bad credentials — and raises whatever aiosmtplib raises so + the caller can show the real reason. + """ + client = aiosmtplib.SMTP(hostname=self._host, port=self._port, use_tls=self._use_tls) + await client.connect() + try: + if self._username: + await client.login(self._username, self._password or "") + finally: + # A failure hanging up says nothing about whether the credentials + # work, which is the only question being asked — so it must not + # mask the login error this block is unwinding. + with contextlib.suppress(Exception): + await client.quit() + async def _send(self, to: str, subject: str, body: str) -> None: message = EmailMessage() message["From"] = self._from diff --git a/modules/users/users/module.py b/modules/users/users/module.py index dd56d200..e147fea5 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -5,6 +5,7 @@ from typing import TYPE_CHECKING from fastapi import APIRouter, Depends +from simple_module_core.audit_links import AuditLink, AuditLinkRegistry from simple_module_core.menu import MenuItem, MenuRegistry, MenuSection from simple_module_core.module import ModuleBase, ModuleMeta from simple_module_core.permissions import PermissionRegistry @@ -96,6 +97,19 @@ def register_permissions(self, registry: PermissionRegistry) -> None: ) registry.map_role(USER_ROLE_NAME, [PERM_USERS_SELF_PROFILE]) + def register_audit_links(self, registry: AuditLinkRegistry) -> None: + from users.models import User + + registry.register( + AuditLink( + # The model class name — what snapshot_changes records. Keying + # this off __tablename__ ("users_user") silently never matches. + entity_type=User.__name__, + url_template=f"{_URL_USERS_ADMIN}/{{id}}", + label="User", + ) + ) + def register_menu_items(self, registry: MenuRegistry) -> None: # Admin-only user management registry.add( @@ -194,6 +208,25 @@ def _app_name() -> str: return name or default_app_name() state.mailer = build_mailer(s, _app_name) + + # Registered here rather than in register_health_checks because the + # check needs the app to re-read DB-hydrated settings on every run. + # The owner is passed explicitly since the boot-time set_owner window + # has long closed by startup. + from simple_module_core.health import HealthCheck + + from users.health import CHECK_MAILER, build_mailer_check + + app.state.sm.health_registry.add( + HealthCheck( + name=CHECK_MAILER, + check=build_mailer_check(app), + module=self.meta.name, + # On demand only: this authenticates against the mail provider, + # which must not happen on a readiness-probe timer. + probe=False, + ) + ) state.rate_limiter = LoginRateLimiter( max_failures=s.login_rate_limit_failures, window_seconds=s.login_rate_limit_window_seconds, diff --git a/modules/users/users/pages/AcceptInvite.tsx b/modules/users/users/pages/AcceptInvite.tsx index aa5f928a..e2e2e18b 100644 --- a/modules/users/users/pages/AcceptInvite.tsx +++ b/modules/users/users/pages/AcceptInvite.tsx @@ -6,12 +6,20 @@ import { AuthCardShell } from '@simple-module-py/ui/layouts/AuthCardShell'; import { CheckCircle2 } from 'lucide-react'; import { useState } from 'react'; +interface InvitePreview { + email: string; + roles: string[]; + already_accepted: boolean; +} + interface Props { token: string; + /** null when the token cannot be read — expired, tampered, or absent. */ + invite: InvitePreview | null; } function AcceptInvite() { - const { token: initialToken } = usePage<{ props: Props }>().props as unknown as Props; + const { token: initialToken, invite } = usePage<{ props: Props }>().props as unknown as Props; const urlToken = typeof window !== 'undefined' ? (new URLSearchParams(window.location.search).get('token') ?? '') @@ -56,13 +64,39 @@ function AcceptInvite() { return ( + {/* Who the invite is for, and what it grants. Without this the card asks + for a password while identifying neither — a forwarded link, or an + invite addressed to the wrong person, is indistinguishable from the + right one. */}
diff --git a/modules/users/users/pages/Users/Index.tsx b/modules/users/users/pages/Users/Index.tsx index dc30b4c8..9a9410af 100644 --- a/modules/users/users/pages/Users/Index.tsx +++ b/modules/users/users/pages/Users/Index.tsx @@ -128,20 +128,14 @@ function Index() { title="Users" description="People with access to this workspace. Invites use the configured mailer." actions={ -
- - -
+ // One entry point: invite-vs-create is a choice inside the form, not + // a choice between two buttons made before seeing either. + } >
diff --git a/modules/users/users/pages/Users/Invite.tsx b/modules/users/users/pages/Users/Invite.tsx deleted file mode 100644 index f0caf4a4..00000000 --- a/modules/users/users/pages/Users/Invite.tsx +++ /dev/null @@ -1,147 +0,0 @@ -import { Link, router, usePage } from '@inertiajs/react'; -import { PageShell } from '@simple-module-py/ui/components/PageShell'; -import { Button } from '@simple-module-py/ui/components/ui/button'; -import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; -import { Input } from '@simple-module-py/ui/components/ui/input'; -import { Label } from '@simple-module-py/ui/components/ui/label'; -import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; -import { Mail, Send } from 'lucide-react'; -import { useState } from 'react'; -import { toast } from 'sonner'; - -interface Role { - id: string; - name: string; -} - -interface Props { - roles: Role[]; -} - -function Invite() { - const { roles } = usePage<{ props: Props }>().props as unknown as Props; - - const [email, setEmail] = useState(''); - const [fullName, setFullName] = useState(''); - const [selectedRoles, setSelectedRoles] = useState([]); - const [error, setError] = useState(null); - const [loading, setLoading] = useState(false); - - const toggleRole = (roleName: string) => { - setSelectedRoles((prev) => - prev.includes(roleName) ? prev.filter((r) => r !== roleName) : [...prev, roleName], - ); - }; - - const handleSubmit = (e: React.FormEvent) => { - e.preventDefault(); - setError(null); - setLoading(true); - fetch('/api/users/admin/invite', { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ email, full_name: fullName || null, role_names: selectedRoles }), - }) - .then(async (res) => { - if (res.ok) { - toast.success('Invite sent'); - router.visit('/users/admin'); - } else { - const data = await res.json().catch(() => ({})); - setError(typeof data?.detail === 'string' ? data.detail : 'Failed to send invite'); - } - }) - .catch(() => setError('An error occurred. Please try again.')) - .finally(() => setLoading(false)); - }; - - return ( - - Back to Users - - } - > - - -
-
- -
- - setEmail(e.target.value)} - placeholder="teammate@example.com" - required - autoComplete="off" - className="pl-9" - /> -
-
- -
- - setFullName(e.target.value)} - placeholder="Jane Doe" - /> -
- - {roles.length > 0 && ( -
- -
- {roles.map((role) => { - const active = selectedRoles.includes(role.name); - return ( - - ); - })} -
-
- )} - - {error &&

{error}

} - -
- - -
-
- - - - ); -} - -Invite.layout = (page: React.ReactNode) => {page}; -export default Invite; diff --git a/modules/users/users/pages/Users/components/CreateUserFields.tsx b/modules/users/users/pages/Users/components/CreateUserFields.tsx new file mode 100644 index 00000000..dbef6310 --- /dev/null +++ b/modules/users/users/pages/Users/components/CreateUserFields.tsx @@ -0,0 +1,78 @@ +import { Input } from '@simple-module-py/ui/components/ui/input'; +import { Label } from '@simple-module-py/ui/components/ui/label'; +import { Lock, Mail } from 'lucide-react'; + +interface Props { + email: string; + fullName: string; + password: string; + onEmailChange: (value: string) => void; + onFullNameChange: (value: string) => void; + onPasswordChange: (value: string) => void; +} + +export function CreateUserFields({ + email, + fullName, + password, + onEmailChange, + onFullNameChange, + onPasswordChange, +}: Props) { + return ( + <> +
+ +
+ + onEmailChange(e.target.value)} + placeholder="teammate@example.com" + required + autoComplete="off" + className="pl-9" + /> +
+
+ +
+ + onFullNameChange(e.target.value)} + placeholder="Jane Doe" + /> +
+ +
+ +
+ + onPasswordChange(e.target.value)} + required + autoComplete="new-password" + className="pl-9" + /> +
+

+ The account is active and verified immediately — share the password securely. +

+
+ + ); +} diff --git a/modules/users/users/pages/Users/components/DetailsCard.tsx b/modules/users/users/pages/Users/components/DetailsCard.tsx index bd1de671..6fa82d94 100644 --- a/modules/users/users/pages/Users/components/DetailsCard.tsx +++ b/modules/users/users/pages/Users/components/DetailsCard.tsx @@ -1,53 +1,25 @@ import { SectionTitle } from '@simple-module-py/ui/components/SectionTitle'; -import { Button } from '@simple-module-py/ui/components/ui/button'; import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; 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'; interface Props { - user: { id: string; email: string; full_name: string | null }; + email: string; + fullName: string; + onEmailChange: (value: string) => void; + onFullNameChange: (value: string) => void; + error?: string | null; } -export function DetailsCard({ user }: Props) { - const [email, setEmail] = useState(user.email); - const [fullName, setFullName] = useState(user.full_name ?? ''); - const [saving, setSaving] = useState(false); - const [error, setError] = useState(null); - - const handleSave = () => { - setSaving(true); - setError(null); - fetch(`/api/users/admin/${user.id}`, { - method: 'PATCH', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ email, full_name: fullName || null }), - }) - .then(async (res) => { - if (res.ok) { - toast.success('Details updated'); - } else { - const data = await res.json().catch(() => ({})); - setError(typeof data?.detail === 'string' ? data.detail : 'Failed to update details'); - } - }) - .catch(() => setError('An error occurred')) - .finally(() => setSaving(false)); - }; - +/** + * Editable account details. Fully controlled and without a save button of its + * own — the page owns one dirty state covering details and roles together. + */ +export function DetailsCard({ email, fullName, onEmailChange, onFullNameChange, error }: Props) { return ( - - {saving ? 'Saving…' : 'Save details'} - - } - > - Details - + Details
@@ -69,7 +41,7 @@ export function DetailsCard({ user }: Props) { id="edit-full-name" type="text" value={fullName} - onChange={(e) => setFullName(e.target.value)} + onChange={(e) => onFullNameChange(e.target.value)} placeholder="Jane Doe" />
diff --git a/modules/users/users/pages/Users/components/InviteFields.tsx b/modules/users/users/pages/Users/components/InviteFields.tsx new file mode 100644 index 00000000..508fda28 --- /dev/null +++ b/modules/users/users/pages/Users/components/InviteFields.tsx @@ -0,0 +1,46 @@ +import { Label } from '@simple-module-py/ui/components/ui/label'; +import { Textarea } from '@simple-module-py/ui/components/ui/textarea'; +import { Info } from 'lucide-react'; + +interface Props { + emails: string; + onEmailsChange: (value: string) => void; + /** Addresses parsed out of the box so far. */ + count: number; + mailerDelivers: boolean; +} + +export function InviteFields({ emails, onEmailsChange, count, mailerDelivers }: Props) { + return ( +
+ +