diff --git a/.env.example b/.env.example index c088a6fd..e2fc5ac7 100644 --- a/.env.example +++ b/.env.example @@ -27,6 +27,13 @@ SM_DATABASE_URL=sqlite+aiosqlite:///./app.db SM_ENVIRONMENT=development SM_SECRET_KEY=change-me-in-production +# Auth provider: `users` (local accounts, the default) or `keycloak` (OIDC). +# Only one can be active at a time — they claim the same auth slot. A dev +# workspace has both installed because `uv sync --all-packages` installs every +# member, so the non-selected one is skipped at discovery instead of failing +# the boot with SM020. Ignored when only one provider is installed. +# SM_AUTH_PROVIDER=keycloak + # Dev-only: Vite asset URL (ignored in production builds) SM_VITE_DEV_URL=http://localhost:5050 diff --git a/README.md b/README.md index 41285156..b8acc8d1 100644 --- a/README.md +++ b/README.md @@ -117,6 +117,7 @@ Local deployments only need one env var — everything else has sensible default | `SM_ENVIRONMENT` | `development` | No — any value other than `development`, `test`, `testing` triggers strict discovery and placeholder-secret checks | | `SM_SECRET_KEY` | `change-me-in-production` | No in dev; **must** be overridden in production | | `SM_VITE_DEV_URL` | `http://localhost:5050` | Dev only — Vite HMR origin | +| `SM_AUTH_PROVIDER` | `users` | No — `users` or `keycloak`. Only read when both are installed; see [Auth providers](#auth-providers-users-or-keycloak) | Power users can still override the following bootstrap knobs via env if needed: `SM_DB_POOL_SIZE`, `SM_DB_MAX_OVERFLOW`, `SM_DB_POOL_PRE_PING`, `SM_DB_POOL_RECYCLE`, `SM_DEBUG`, `SM_LOG_LEVEL`, `SM_LOG_FORMAT`, `SM_MODULES_ENABLED`. These are needed before the DB connection is open. @@ -160,6 +161,47 @@ The 300-line file cap (enforced by CI) usually pushes you to factor row-level co ## User management +### Auth providers: users **or** keycloak, never both + +This repo ships two authentication providers, and **exactly one can be active at a time**: + +| Module | What it does | +|---|---| +| `users` (default) | Local accounts — password login, invites, signup, roles, an admin UI | +| `keycloak` | Delegates authentication to a Keycloak realm over OIDC | + +Both claim the same slot (`app.state.auth.auth_provider`), so running them together +is a misconfiguration, not a supported combination. Installing both and activating +neither is reported as `SM020` (error — fails boot); installing neither is `SM021`. + +`uv sync --all-packages` installs *every* workspace member, so a dev clone has both +packages on disk. The host therefore activates one and skips the other rather than +failing on `SM020`, and **`users` is the default** — `keycloak` is installed but inert +until you ask for it. To switch: + +```bash +# .env +SM_AUTH_PROVIDER=keycloak +``` + +Then re-run `make gen-pages` so the frontend manifest picks up the active provider's +pages (`make dev` does this for you), and configure the realm under +`/settings/modules`. Switching back is the same knob set to `users`. + +Two caveats when running Keycloak: + +- `dashboard`, `permissions`, `audit_log`, and `background_tasks` declare a + dependency on the `Users` **module** and import from the `users` package, so + `simple_module_users` still has to be installed even though it is inactive. A + host that wants Keycloak *and* none of the local-account machinery should leave + those modules out of its own dependency list. +- The local-account flows (`/users/login`, invites, signup, the sections below) + belong to the `users` module and are gone while Keycloak is active — Keycloak's + realm owns login, logout, and user administration instead. + +Only one provider is ever discovered, so nothing here changes if you install just one +of the two: a host that ships only `keycloak` keeps it regardless of `SM_AUTH_PROVIDER`. + ### Creating the first admin Either use the CLI: diff --git a/framework/core/simple_module_core/__init__.py b/framework/core/simple_module_core/__init__.py index f01de2f9..d43e1bfa 100644 --- a/framework/core/simple_module_core/__init__.py +++ b/framework/core/simple_module_core/__init__.py @@ -8,8 +8,11 @@ run_diagnostics, ) from simple_module_core.discovery import ( + DEFAULT_AUTH_PROVIDER, discover_modules, get_module_package_name, + resolve_auth_provider, + select_auth_provider, topological_sort, ) from simple_module_core.events import Event, EventBus @@ -39,6 +42,7 @@ from simple_module_core.versioning import FRAMEWORK_API_VERSION, check_framework_compatibility __all__ = [ + "DEFAULT_AUTH_PROVIDER", "FRAMEWORK_API_VERSION", "CircularDependencyError", "DesignPack", @@ -77,6 +81,8 @@ "is_flag_enabled", "print_diagnostics", "require_flag", + "resolve_auth_provider", "run_diagnostics", + "select_auth_provider", "topological_sort", ] diff --git a/framework/core/simple_module_core/__main__.py b/framework/core/simple_module_core/__main__.py index 9c5cd9be..0f122ab2 100644 --- a/framework/core/simple_module_core/__main__.py +++ b/framework/core/simple_module_core/__main__.py @@ -25,7 +25,12 @@ print_diagnostics, run_diagnostics, ) -from simple_module_core.discovery import discover_modules, topological_sort +from simple_module_core.discovery import ( + discover_modules, + resolve_auth_provider, + select_auth_provider, + topological_sort, +) from simple_module_core.dotenv import parse_dotenv from simple_module_core.exceptions import InvalidModuleError @@ -100,6 +105,10 @@ def main() -> int: print("No modules discovered. Is the project installed (`uv sync --all-packages`)?") return 0 + # Mirror the host: only the configured auth provider is active, so doctor + # reports on the same module set the app actually boots with. + modules = select_auth_provider(modules, resolve_auth_provider()) + # Topological sort surfaces CircularDependencyError early. modules = topological_sort(modules) diff --git a/framework/core/simple_module_core/discovery.py b/framework/core/simple_module_core/discovery.py index 5ecf1188..b2b2568c 100644 --- a/framework/core/simple_module_core/discovery.py +++ b/framework/core/simple_module_core/discovery.py @@ -6,6 +6,7 @@ from collections.abc import Sequence from importlib.metadata import entry_points +from simple_module_core.dotenv import env_str, load_dotenv_into_environ from simple_module_core.exceptions import CircularDependencyError, InvalidModuleError from simple_module_core.module import ModuleBase, ModuleMeta @@ -13,6 +14,9 @@ ENTRY_POINT_GROUP = "simple_module" +DEFAULT_AUTH_PROVIDER = "users" +"""Auth provider activated when several are installed — see select_auth_provider.""" + def get_module_package_name(module: ModuleBase) -> str: """Return the top-level Python package a module instance belongs to. @@ -121,6 +125,67 @@ def fail(msg: str, exc: BaseException | None = None) -> None: return modules +def resolve_auth_provider() -> str: + """Return the configured auth provider name, honoring ``.env``. + + For tools that run outside the host process (``make doctor``, ``smpy host + gen-pages``) and so have no ``BootstrapSettings`` to read from. The host + passes ``settings.auth_provider`` to :func:`select_auth_provider` instead. + """ + load_dotenv_into_environ() + return env_str("SM_AUTH_PROVIDER", DEFAULT_AUTH_PROVIDER) + + +def select_auth_provider( + modules: Sequence[ModuleBase], + preferred: str = DEFAULT_AUTH_PROVIDER, + *, + strict: bool = False, +) -> list[ModuleBase]: + """Drop every auth provider except ``preferred`` when several are installed. + + ``users`` and ``keycloak`` both claim ``app.state.auth.auth_provider``, so + only one can be active — that's what SM020 reports. A dev workspace has + both installed (``uv sync --all-packages`` installs every member), which + would otherwise fail the boot on a fresh clone. Selecting one here keeps + the alternative provider installed and testable but inert. + + Nothing is dropped when fewer than two providers are present — a host that + installs only ``keycloak`` keeps it whatever ``preferred`` says. + + A ``preferred`` that names none of the installed providers is a + misconfiguration (typically a typo). Leaving every provider mounted means + two modules write ``app.state.auth.auth_provider`` and the topological + order silently decides the winner, so this always warns, and with + ``strict`` raises :class:`InvalidModuleError`. Callers in production + should pass ``strict=True``: SM020 would catch this, but diagnostics run + in development only, so nothing else reports it in a deployed app. + """ + providers = [m for m in modules if getattr(m, "_is_auth_provider", False)] + if len(providers) < 2: + return list(modules) + + installed = ", ".join(m.meta.name for m in providers) + keep = next((m for m in providers if m.meta.name.lower() == preferred.lower()), None) + if keep is None: + msg = ( + f"SM_AUTH_PROVIDER={preferred!r} matches none of the installed auth " + f"providers ({installed}); all of them stay mounted and the last one " + "registered wins" + ) + if strict: + raise InvalidModuleError(msg) + logger.warning("%s — set it to one of the names above", msg) + return list(modules) + + logger.info( + "Auth provider '%s' active (SM_AUTH_PROVIDER); skipping also-installed: %s", + keep.meta.name, + ", ".join(m.meta.name for m in providers if m is not keep), + ) + return [m for m in modules if m is keep or not getattr(m, "_is_auth_provider", False)] + + def topological_sort(modules: Sequence[ModuleBase]) -> list[ModuleBase]: """Sort modules so dependencies come before dependents. diff --git a/framework/core/tests/test_auth_provider_selection.py b/framework/core/tests/test_auth_provider_selection.py new file mode 100644 index 00000000..bc9e41d0 --- /dev/null +++ b/framework/core/tests/test_auth_provider_selection.py @@ -0,0 +1,133 @@ +"""Tests for select_auth_provider — picking one of several installed providers.""" + +from __future__ import annotations + +import logging +import os + +import pytest +from simple_module_core.discovery import ( + DEFAULT_AUTH_PROVIDER, + resolve_auth_provider, + select_auth_provider, +) +from simple_module_core.exceptions import InvalidModuleError +from simple_module_core.module import ModuleBase, ModuleMeta + + +class FakeUsers(ModuleBase): + meta = ModuleMeta(name="Users") + _is_auth_provider = True + + +class FakeKeycloak(ModuleBase): + meta = ModuleMeta(name="Keycloak") + _is_auth_provider = True + + +class FakeDashboard(ModuleBase): + meta = ModuleMeta(name="Dashboard") + + +def _names(modules): + return [m.meta.name for m in modules] + + +class TestSelectAuthProvider: + def test_default_keeps_users_and_drops_keycloak(self): + """Both installed (the dev workspace) → the default provider wins, no SM020.""" + result = select_auth_provider([FakeUsers(), FakeKeycloak(), FakeDashboard()]) + assert _names(result) == ["Users", "Dashboard"] + + def test_preferred_keycloak_drops_users(self): + result = select_auth_provider([FakeUsers(), FakeKeycloak()], "keycloak") + assert _names(result) == ["Keycloak"] + + def test_match_is_case_insensitive(self): + result = select_auth_provider([FakeUsers(), FakeKeycloak()], "KeyCloak") + assert _names(result) == ["Keycloak"] + + def test_non_provider_modules_keep_their_order(self): + modules = [FakeDashboard(), FakeKeycloak(), FakeUsers()] + assert _names(select_auth_provider(modules)) == ["Dashboard", "Users"] + + def test_lone_provider_survives_a_mismatched_preference(self): + """A keycloak-only host keeps keycloak even at the default preference.""" + result = select_auth_provider([FakeKeycloak(), FakeDashboard()], "users") + assert _names(result) == ["Keycloak", "Dashboard"] + + def test_no_providers_is_untouched(self): + """SM021 territory — nothing to select, and nothing to drop.""" + assert _names(select_auth_provider([FakeDashboard()])) == ["Dashboard"] + + def test_unknown_preference_keeps_every_provider(self): + """Naming a provider that isn't installed must not silently pick one.""" + result = select_auth_provider([FakeUsers(), FakeKeycloak()], "oidc") + assert _names(result) == ["Users", "Keycloak"] + + def test_unknown_preference_warns(self, caplog): + """SM020 only runs in development — a typo must not pass in silence.""" + with caplog.at_level(logging.WARNING, logger="simple_module_core.discovery"): + select_auth_provider([FakeUsers(), FakeKeycloak()], "keycloack") + warnings = [r for r in caplog.records if r.levelno >= logging.WARNING] + assert warnings, "an unrecognised provider name logged nothing" + assert "keycloack" in warnings[0].getMessage() + + def test_unknown_preference_raises_when_strict(self): + """Production boots strict, where mounting both providers is not viable.""" + with pytest.raises(InvalidModuleError, match="keycloack"): + select_auth_provider([FakeUsers(), FakeKeycloak()], "keycloack", strict=True) + + def test_strict_is_quiet_on_a_valid_selection(self): + result = select_auth_provider([FakeUsers(), FakeKeycloak()], "users", strict=True) + assert _names(result) == ["Users"] + + def test_strict_ignores_a_mismatch_when_only_one_is_installed(self): + """Nothing is ambiguous with a single provider, so strict must not raise.""" + result = select_auth_provider([FakeKeycloak()], "oidc", strict=True) + assert _names(result) == ["Keycloak"] + + def test_selection_is_logged(self, caplog): + with caplog.at_level(logging.INFO, logger="simple_module_core.discovery"): + select_auth_provider([FakeUsers(), FakeKeycloak()]) + assert any("Keycloak" in rec.getMessage() for rec in caplog.records) + + def test_input_is_not_mutated(self): + modules = [FakeUsers(), FakeKeycloak()] + select_auth_provider(modules) + assert len(modules) == 2 + + +class TestResolveAuthProvider: + @pytest.fixture(autouse=True) + def _isolated_environ(self, monkeypatch, tmp_path): + """Swap in a throwaway ``os.environ``. + + ``resolve_auth_provider`` merges ``.env`` into the real environment via + ``setdefault``, which ``monkeypatch.delenv(raising=False)`` can't undo — + it records nothing for a key that was already absent. Without this the + provider chosen here would leak into every later test in the session. + """ + monkeypatch.setattr(os, "environ", dict(os.environ)) + monkeypatch.setenv("SM_PROJECT_ROOT", str(tmp_path)) + monkeypatch.delenv("SM_AUTH_PROVIDER", raising=False) + + def test_defaults_to_users(self): + assert resolve_auth_provider() == DEFAULT_AUTH_PROVIDER + + def test_reads_the_env_var(self, monkeypatch): + monkeypatch.setenv("SM_AUTH_PROVIDER", "keycloak") + assert resolve_auth_provider() == "keycloak" + + def test_blank_falls_back_to_the_default(self, monkeypatch): + monkeypatch.setenv("SM_AUTH_PROVIDER", " ") + assert resolve_auth_provider() == DEFAULT_AUTH_PROVIDER + + def test_reads_dotenv_when_env_is_unset(self, tmp_path): + (tmp_path / ".env").write_text("SM_AUTH_PROVIDER=keycloak\n", encoding="utf-8") + assert resolve_auth_provider() == "keycloak" + + def test_real_environment_wins_over_dotenv(self, monkeypatch, tmp_path): + (tmp_path / ".env").write_text("SM_AUTH_PROVIDER=keycloak\n", encoding="utf-8") + monkeypatch.setenv("SM_AUTH_PROVIDER", "users") + assert resolve_auth_provider() == "users" diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index c2d57815..d712a5e0 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -12,7 +12,7 @@ from fastapi import FastAPI 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, topological_sort +from simple_module_core.discovery import discover_modules, select_auth_provider, topological_sort from simple_module_core.events import EventBus from simple_module_core.feature_flags import FeatureFlagRegistry from simple_module_core.health import HealthRegistry @@ -120,6 +120,13 @@ def create_app(settings: Settings | None = None) -> FastAPI: enabled=settings.modules_enabled, strict=not settings.is_development, ) + # Two auth providers can be installed at once (they are in this workspace); + # only the configured one is activated. See select_auth_provider / SM020. + # Strict outside development: diagnostics don't run there, so an + # unrecognised name would otherwise mount both providers unreported. + modules = select_auth_provider( + modules, settings.auth_provider, strict=not settings.is_development + ) modules = topological_sort(modules) logger.info( "Loaded %d module(s): %s", diff --git a/framework/hosting/simple_module_hosting/bootstrap_settings.py b/framework/hosting/simple_module_hosting/bootstrap_settings.py index ff501e5a..142103e5 100644 --- a/framework/hosting/simple_module_hosting/bootstrap_settings.py +++ b/framework/hosting/simple_module_hosting/bootstrap_settings.py @@ -11,6 +11,7 @@ from pydantic import field_validator, model_validator from pydantic_settings import BaseSettings, SettingsConfigDict +from simple_module_core.discovery import DEFAULT_AUTH_PROVIDER from simple_module_core.environments import NON_PROD_ENVIRONMENTS _PLACEHOLDER_SECRET_KEY = "change-me-in-production" @@ -46,6 +47,14 @@ class BootstrapSettings(BaseSettings): modules_enabled: list[str] | None = None + auth_provider: str = DEFAULT_AUTH_PROVIDER + """Which auth provider module to activate (``SM_AUTH_PROVIDER``). + + ``users`` and ``keycloak`` both provide authentication and only one may be + active at a time. When both are installed this picks the winner; the other + is skipped at discovery. Ignored when only one is installed. + """ + auth_public_paths: list[str] = [] """Host-level anonymous-access path prefixes (``SM_AUTH_PUBLIC_PATHS``). @@ -55,6 +64,19 @@ class BootstrapSettings(BaseSettings): ``register_public_routes`` hook, which is method-aware. """ + @field_validator("auth_provider", mode="after") + @classmethod + def _normalize_auth_provider(cls, value: str) -> str: + """Strip whitespace; treat blank as unset. + + ``SM_AUTH_PROVIDER=`` in a ``.env`` yields ``''``, which matches no + installed provider and would mount all of them. The out-of-process + readers (``make doctor``, ``gen-pages``) go through + ``resolve_auth_provider``, which already falls back on blank — without + this they and the host would disagree about the active provider. + """ + return value.strip() or DEFAULT_AUTH_PROVIDER + @field_validator("trusted_proxy", mode="after") @classmethod def _normalize_trusted_proxy(cls, value: str | None) -> str | None: diff --git a/framework/hosting/simple_module_hosting/host_cli.py b/framework/hosting/simple_module_hosting/host_cli.py index 601ecd58..64054af4 100644 --- a/framework/hosting/simple_module_hosting/host_cli.py +++ b/framework/hosting/simple_module_hosting/host_cli.py @@ -13,7 +13,7 @@ from typing import Annotated import typer -from simple_module_core import discover_modules +from simple_module_core import discover_modules, resolve_auth_provider, select_auth_provider from simple_module_hosting.manifest import ( collect_module_js_deps, @@ -42,7 +42,9 @@ def gen_pages( if not host_dir.is_dir(): typer.echo(f"ERROR: client_app directory not found at {host_dir}", err=True) raise typer.Exit(code=1) - modules = discover_modules() + # Skip the auth provider the host won't boot — its pages would otherwise + # ship in the bundle with no view endpoint able to render them. + modules = select_auth_provider(discover_modules(), resolve_auth_provider()) written = write_module_pages_manifest(modules, host_dir) typer.echo( f"Module pages manifest: {len(modules)} module(s) " @@ -70,7 +72,7 @@ def sync_js_deps( typer.echo(f"ERROR: client_app directory not found at {host_client_app}", err=True) raise typer.Exit(code=1) - modules = discover_modules() + modules = select_auth_provider(discover_modules(), resolve_auth_provider()) by_module = collect_module_js_deps(modules) if not by_module: typer.echo("No module JS dependencies declared.") diff --git a/framework/hosting/tests/test_app.py b/framework/hosting/tests/test_app.py index 6066daff..b7bc31ca 100644 --- a/framework/hosting/tests/test_app.py +++ b/framework/hosting/tests/test_app.py @@ -168,16 +168,16 @@ async def test_app_state_has_sm_services( (tmp_path / "host" / "templates").mkdir(parents=True) (tmp_path / "host" / "templates" / "index.html").write_text("") - # Exclude Keycloak — both ``users`` and ``keycloak`` are installed as - # entry points in the dev workspace. SM020 fires if both are present - # because the app is only meant to run with one auth provider. - from simple_module_core.discovery import discover_modules - - all_names = [m.meta.name for m in discover_modules() if m.meta.name != "Keycloak"] - app = create_app(Settings(modules_enabled=all_names)) + # No modules_enabled allowlist: both auth providers are entry points in + # the dev workspace, and create_app is expected to activate the named + # one rather than fail the boot on SM020. auth_provider is passed + # explicitly so an SM_AUTH_PROVIDER in the developer's .env can't + # decide which provider this asserts on. + app = create_app(Settings(auth_provider="users")) sm = app.state.sm assert isinstance(sm, Services) + assert [m.meta.name for m in sm.modules].count("Keycloak") == 0 assert sm.settings is not None assert sm.db is not None assert sm.event_bus is not None diff --git a/framework/hosting/tests/test_auth_provider_setting.py b/framework/hosting/tests/test_auth_provider_setting.py new file mode 100644 index 00000000..bc7c3e9b --- /dev/null +++ b/framework/hosting/tests/test_auth_provider_setting.py @@ -0,0 +1,59 @@ +"""``SM_AUTH_PROVIDER`` normalisation on BootstrapSettings. + +The host reads this through ``Settings``; ``make doctor`` and ``smpy host +gen-pages`` read it through ``simple_module_core.resolve_auth_provider``. +The two must agree on every input, or the tools report on a different module +set than the app boots with. +""" + +from __future__ import annotations + +import os + +import pytest +from simple_module_core.discovery import DEFAULT_AUTH_PROVIDER, resolve_auth_provider +from simple_module_hosting.settings import Settings + + +def _settings(**overrides) -> Settings: + return Settings( + database_url="sqlite+aiosqlite:///:memory:", + environment="testing", + secret_key="test-secret-key", + **overrides, + ) + + +class TestAuthProviderSetting: + @pytest.fixture(autouse=True) + def _isolated_env(self, monkeypatch, tmp_path): + """Run against an empty ``.env`` and a throwaway environment. + + ``Settings`` resolves ``env_file=".env"`` relative to the working + directory, so without the chdir a developer running Keycloak locally + would fail the default-value assertions below — the very coupling + these tests exist to pin down. + """ + monkeypatch.setattr(os, "environ", dict(os.environ)) + monkeypatch.delenv("SM_AUTH_PROVIDER", raising=False) + monkeypatch.chdir(tmp_path) + + def test_defaults_to_users(self): + assert _settings().auth_provider == DEFAULT_AUTH_PROVIDER + + def test_explicit_value_passes_through(self): + assert _settings(auth_provider="keycloak").auth_provider == "keycloak" + + @pytest.mark.parametrize("raw", ["", " ", "\t"]) + def test_blank_falls_back_to_the_default(self, raw: str): + """``SM_AUTH_PROVIDER=`` in .env yielded '', which matches no provider.""" + assert _settings(auth_provider=raw).auth_provider == DEFAULT_AUTH_PROVIDER + + def test_surrounding_whitespace_stripped(self): + assert _settings(auth_provider=" keycloak ").auth_provider == "keycloak" + + @pytest.mark.parametrize("raw", ["", " ", "keycloak", " keycloak "]) + def test_agrees_with_resolve_auth_provider(self, raw: str, monkeypatch): + """Host and out-of-process readers must land on the same name.""" + monkeypatch.setenv("SM_AUTH_PROVIDER", raw) + assert _settings().auth_provider == resolve_auth_provider() diff --git a/framework/testing/simple_module_test/fixtures.py b/framework/testing/simple_module_test/fixtures.py index c827adc9..caa48ce1 100644 --- a/framework/testing/simple_module_test/fixtures.py +++ b/framework/testing/simple_module_test/fixtures.py @@ -22,12 +22,13 @@ import contextlib import importlib -from collections.abc import AsyncGenerator +import os +from collections.abc import AsyncGenerator, Iterator from functools import lru_cache import httpx import pytest -from simple_module_core.discovery import discover_modules +from simple_module_core.discovery import DEFAULT_AUTH_PROVIDER, discover_modules from simple_module_db.base import all_module_bases from simple_module_db.session import DatabaseState, init_db from simple_module_hosting.settings import Settings @@ -35,6 +36,35 @@ from simple_module_test.session_cookie import forge_session_cookie +_AUTH_PROVIDER_ENV = "SM_AUTH_PROVIDER" + + +@pytest.fixture(scope="session", autouse=True) +def pinned_auth_provider() -> Iterator[str]: + """Pin ``SM_AUTH_PROVIDER`` to the local-account provider for the suite. + + ``Settings`` reads the repo's ``.env``, and the README tells anyone running + Keycloak locally to put ``SM_AUTH_PROVIDER=keycloak`` there. Nearly every + test that boots an app expects the ``users`` provider — ``/users/login``, + the admin pages, the seeded admin in ``authenticated_client`` — and the + ``Settings(...)`` construction sites are spread across module conftests, + so pinning it on each one would be a losing game. + + The real environment outranks ``.env`` in pydantic-settings, so setting it + here covers every construction site at once. A test that wants a different + provider can still ``monkeypatch.setenv`` or pass ``auth_provider=`` to + ``Settings`` directly. + """ + previous = os.environ.get(_AUTH_PROVIDER_ENV) + os.environ[_AUTH_PROVIDER_ENV] = DEFAULT_AUTH_PROVIDER + try: + yield DEFAULT_AUTH_PROVIDER + finally: + if previous is None: + os.environ.pop(_AUTH_PROVIDER_ENV, None) + else: + os.environ[_AUTH_PROVIDER_ENV] = previous + @pytest.fixture def settings() -> Settings: @@ -44,6 +74,11 @@ def settings() -> Settings: (and the ``X-Tenant-ID`` header paths they rely on) keep working. Individual tests that want the tenant middleware absent construct their own ``Settings(multi_tenant=False, ...)`` in the test body. + + ``auth_provider`` is pinned for the same reason as the rest: a developer + running Keycloak locally has ``SM_AUTH_PROVIDER=keycloak`` in ``.env``, + which would otherwise swap the auth provider out from under every test + that expects ``/users/login``. """ return Settings( database_url="sqlite+aiosqlite:///:memory:", @@ -51,6 +86,7 @@ def settings() -> Settings: secret_key="test-secret-key", multi_tenant=True, tenant_header="X-Tenant-ID", + auth_provider="users", ) diff --git a/framework/testing/simple_module_test/plugin.py b/framework/testing/simple_module_test/plugin.py index 85a9328f..342da876 100644 --- a/framework/testing/simple_module_test/plugin.py +++ b/framework/testing/simple_module_test/plugin.py @@ -27,6 +27,7 @@ db_session, db_state, engine, + pinned_auth_provider, settings, ) diff --git a/host/tests/test_nav_icons.py b/host/tests/test_nav_icons.py new file mode 100644 index 00000000..c44a1431 --- /dev/null +++ b/host/tests/test_nav_icons.py @@ -0,0 +1,64 @@ +"""Every menu icon a module declares must exist in the frontend's ICON_MAP. + +``NavIcon`` renders an empty spacer for an unknown name rather than throwing, +so a typo — or a new lucide icon nobody added to the map — costs a sidebar +entry its icon with nothing in the logs to say so. Branding (``palette``), +Audit Log (``scroll-text``), and the Doctor page (``stethoscope``) all +shipped that way. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest +from simple_module_core.discovery import discover_modules +from simple_module_core.menu import MenuRegistry + +_NAV_ICON = Path(__file__).resolve().parents[2] / "packages/ui/src/components/NavIcon.tsx" + +# Matches both quoted and bare keys: ``'log-out': LogOut,`` and ``home: Home,``. +_ICON_KEY = re.compile(r"^\s*'?([a-z0-9-]+)'?:\s*[A-Z]\w*,\s*$", re.MULTILINE) + + +def _mapped_icon_names() -> set[str]: + source = _NAV_ICON.read_text(encoding="utf-8") + body = source.split("const ICON_MAP = {", 1)[1].split("} as const;", 1)[0] + return set(_ICON_KEY.findall(body)) + + +def _declared_icon_names() -> dict[str, str]: + """Return ``{icon_name: "Module/label"}`` for every registered menu item.""" + declared: dict[str, str] = {} + for module in discover_modules(): + registry = MenuRegistry() + module.register_menu_items(registry) + for item in registry.all_items: + if item.icon: + declared[item.icon] = f"{module.meta.name}/{item.label}" + return declared + + +class TestNavIcons: + def test_icon_map_parses(self): + """Guard the regex itself — a silently empty map would pass every check.""" + mapped = _mapped_icon_names() + assert "home" in mapped + assert "log-out" in mapped + assert len(mapped) > 50 + + def test_every_declared_icon_is_mapped(self): + declared = _declared_icon_names() + assert declared, "no module declared a menu icon — discovery is probably broken" + + mapped = _mapped_icon_names() + missing = {name: owner for name, owner in declared.items() if name not in mapped} + assert not missing, ( + f"menu icons with no ICON_MAP entry (they render as blank spacers): {missing}. " + f"Add them to {_NAV_ICON.name}." + ) + + @pytest.mark.parametrize("icon", ["palette", "scroll-text", "stethoscope"]) + def test_previously_missing_icons_stay_mapped(self, icon: str): + assert icon in _mapped_icon_names() diff --git a/modules/background_tasks/background_tasks/constants.py b/modules/background_tasks/background_tasks/constants.py index 995ffc8f..d567f240 100644 --- a/modules/background_tasks/background_tasks/constants.py +++ b/modules/background_tasks/background_tasks/constants.py @@ -18,6 +18,9 @@ # ── Module dependencies ───────────────────────────────────────── _MODULE_USERS = "Users" +# register_settings() calls settings.registration.register_module_settings, +# which reads app.state.settings — so Settings must register first. +_MODULE_SETTINGS = "Settings" # ── Env / settings ────────────────────────────────────────────── ENV_PREFIX = "SM_BG_TASKS_" diff --git a/modules/background_tasks/background_tasks/module.py b/modules/background_tasks/background_tasks/module.py index a8c217ac..7c7c934c 100644 --- a/modules/background_tasks/background_tasks/module.py +++ b/modules/background_tasks/background_tasks/module.py @@ -13,6 +13,7 @@ from simple_module_core.permissions import PermissionRegistry from background_tasks.constants import ( + _MODULE_SETTINGS, _MODULE_USERS, API_PREFIX, MENU_ICON, @@ -40,7 +41,7 @@ class BackgroundTasksModule(ModuleBase): name=MODULE_DISPLAY_NAME, route_prefix=API_PREFIX, view_prefix=VIEW_PREFIX, - depends_on=[_MODULE_USERS], + depends_on=[_MODULE_USERS, _MODULE_SETTINGS], i18n_audience="admin", ) diff --git a/modules/settings/settings/pages/components/ModuleForm.tsx b/modules/settings/settings/pages/components/ModuleForm.tsx index ecdd8fb1..290f3dda 100644 --- a/modules/settings/settings/pages/components/ModuleForm.tsx +++ b/modules/settings/settings/pages/components/ModuleForm.tsx @@ -78,7 +78,7 @@ export function ModuleForm({ module: m }: Props) { const body = await resp.json(); const fieldErrs: Record = {}; for (const d of body.detail ?? []) { - if (d.loc && d.loc.length) fieldErrs[d.loc[d.loc.length - 1]] = d.msg; + if (d.loc?.length) fieldErrs[d.loc[d.loc.length - 1]] = d.msg; } setErrors(fieldErrs); } else if (resp.ok) { diff --git a/modules/users/users/module.py b/modules/users/users/module.py index 754cf475..dd56d200 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -21,6 +21,9 @@ from simple_module_core.events import EventBus _MODULE_DEPENDENCY_AUTH = "Auth" +# register_settings() goes through settings.registration.register_module_settings, +# which reads app.state.settings — so Settings must register first. +_MODULE_DEPENDENCY_SETTINGS = "Settings" # Menu URLs _URL_USERS_ADMIN = "/users/admin" @@ -38,7 +41,7 @@ class UsersModule(ModuleBase): name="Users", route_prefix="/api/users", view_prefix="/users", - depends_on=[_MODULE_DEPENDENCY_AUTH], + depends_on=[_MODULE_DEPENDENCY_AUTH, _MODULE_DEPENDENCY_SETTINGS], ) _is_auth_provider = True diff --git a/packages/ui/src/components/NavIcon.tsx b/packages/ui/src/components/NavIcon.tsx index 7bd90a39..5726e7be 100644 --- a/packages/ui/src/components/NavIcon.tsx +++ b/packages/ui/src/components/NavIcon.tsx @@ -48,10 +48,12 @@ import { Menu, MessageSquare, Package, + Palette, Pencil, Plus, RefreshCw, Save, + ScrollText, Search, Send, Server, @@ -63,6 +65,7 @@ import { ShoppingCart, Sparkles, Star, + Stethoscope, Tag, Terminal, Trash, @@ -123,10 +126,12 @@ const ICON_MAP = { menu: Menu, 'message-square': MessageSquare, package: Package, + palette: Palette, pencil: Pencil, plus: Plus, 'refresh-cw': RefreshCw, save: Save, + 'scroll-text': ScrollText, search: Search, send: Send, server: Server, @@ -138,6 +143,7 @@ const ICON_MAP = { 'shopping-cart': ShoppingCart, sparkles: Sparkles, star: Star, + stethoscope: Stethoscope, tag: Tag, terminal: Terminal, trash: Trash,