From fae26f151ed7dd1b491021b336dac750ecfbca23 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 13:56:41 +0000 Subject: [PATCH 1/3] feat(core): activate one auth provider instead of failing on SM020 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fresh clone could not boot. `uv sync --all-packages` installs every workspace member, so both `users` and `keycloak` register auth-provider entry points, SM020 fires, and `create_app` exits before serving a request — `make dev`, `python -m host.main`, and `make doctor` all died on a clean checkout. Add `SM_AUTH_PROVIDER` (default `users`) and `select_auth_provider()`, which drops the non-selected providers at discovery when more than one is installed. Keycloak stays installed and testable but inert until asked for. Nothing is dropped when only one provider is present, so a keycloak-only host is unaffected, and an unrecognised name leaves the conflict for SM020 to report rather than silently picking a winner. Applied in `create_app`, `make doctor`, and `smpy host gen-pages` so the manifest, the diagnostics, and the running app agree on the module set. Also: - `users` and `background_tasks` call `register_module_settings`, which reads `app.state.settings`, but neither declared a dependency on `Settings` — they were ordered correctly only by way of a transitive edge. Selecting keycloak removed that edge and `background_tasks` crashed at boot. Declare the dependency they actually have. - Fix the one Biome warning (`useOptionalChain` in ModuleForm.tsx). Claude-Session: https://claude.ai/code/session_01RtK7xkMUveHaw6TdK1DF7L --- .env.example | 7 ++ README.md | 42 +++++++ framework/core/simple_module_core/__init__.py | 6 + framework/core/simple_module_core/__main__.py | 11 +- .../core/simple_module_core/discovery.py | 48 ++++++++ .../tests/test_auth_provider_selection.py | 110 ++++++++++++++++++ .../simple_module_hosting/app_builder.py | 5 +- .../bootstrap_settings.py | 9 ++ .../hosting/simple_module_hosting/host_cli.py | 8 +- framework/hosting/tests/test_app.py | 12 +- .../background_tasks/constants.py | 3 + .../background_tasks/module.py | 3 +- .../settings/pages/components/ModuleForm.tsx | 2 +- modules/users/users/module.py | 5 +- 14 files changed, 256 insertions(+), 15 deletions(-) create mode 100644 framework/core/tests/test_auth_provider_selection.py 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..ea773a0c 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,50 @@ 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, +) -> 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 — nor when + ``preferred`` names none of them, since that's real misconfiguration and + SM020 states it better than a silent pick would. + """ + providers = [m for m in modules if getattr(m, "_is_auth_provider", False)] + if len(providers) < 2: + return list(modules) + + keep = next((m for m in providers if m.meta.name.lower() == preferred.lower()), None) + if keep is None: + 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..0a0ef7b9 --- /dev/null +++ b/framework/core/tests/test_auth_provider_selection.py @@ -0,0 +1,110 @@ +"""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.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_leaves_the_conflict_for_sm020(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_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..48994f08 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,9 @@ 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. + modules = select_auth_provider(modules, settings.auth_provider) 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..fa843707 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``). 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..6be967a3 100644 --- a/framework/hosting/tests/test_app.py +++ b/framework/hosting/tests/test_app.py @@ -168,15 +168,13 @@ 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)) + # Every installed module, Keycloak included: both auth providers are + # entry points in the dev workspace and create_app is expected to + # activate one (SM_AUTH_PROVIDER) rather than fail the boot on SM020. + app = create_app(Settings()) sm = app.state.sm + assert [m.meta.name for m in sm.modules].count("Keycloak") == 0 assert isinstance(sm, Services) assert sm.settings is not None assert sm.db is not None 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 From a6c5864c5b3f993e5714f93da5a167e6e6b1376a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 14:07:04 +0000 Subject: [PATCH 2/3] fix(ui): map the menu icons that rendered as blank spacers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Branding declares `palette`, Audit Log declares `scroll-text`, and the Doctor page declares `stethoscope` — none were in NavIcon's ICON_MAP, so those three sidebar entries rendered an empty 5x5 span where the icon should be. NavIcon falls back to that spacer for an unknown name instead of throwing, so nothing surfaced the gap: no console warning, no failing test, just a missing icon. Add the three lucide imports and their map entries. Guard it with host/tests/test_nav_icons.py, which walks every module's register_menu_items(), collects the declared icon names, and asserts each one has an ICON_MAP entry. It found `stethoscope`, which a manual grep of the module sources had missed. Claude-Session: https://claude.ai/code/session_01RtK7xkMUveHaw6TdK1DF7L --- host/tests/test_nav_icons.py | 64 ++++++++++++++++++++++++++ packages/ui/src/components/NavIcon.tsx | 6 +++ 2 files changed, 70 insertions(+) create mode 100644 host/tests/test_nav_icons.py 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/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, From cb416cc47761ae368793a7033210ebafef81d3fe Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 15:21:26 +0000 Subject: [PATCH 3/3] fix(core): fail loudly on an unrecognised SM_AUTH_PROVIDER; isolate tests from it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four gaps in the auth-provider selection, all found by review: 1. An unrecognised name (typo) kept every provider mounted and said nothing. The docstring claimed SM020 would report it, but diagnostics run in development only (app_builder gates run_diagnostics on is_development), so a deployed app silently mounted both providers, rendered two "Logout" entries, and let topological order decide which one won app.state.auth.auth_provider. select_auth_provider now always warns, and takes strict= (passed as `not is_development`) to raise InvalidModuleError in production — matching how discover_modules already treats strictness. 2. The shared `settings` fixture pinned every other env-sensitive knob but not auth_provider, so `SM_AUTH_PROVIDER=keycloak` in a .env — exactly what the new README tells Keycloak users to set — reshaped the suite. Pinning the fixture alone wasn't enough: module conftests build their own Settings. A session-scoped autouse fixture in the plugin pins the env var itself, which outranks .env and so covers every construction site. With keycloak in .env the suite went from 152 failures to 0. 3. test_app_state_has_sm_services read SM_AUTH_PROVIDER from the ambient environment while hard-asserting Keycloak was absent. It now passes auth_provider explicitly, and asserts the Services type before the module-list detail. 4. Blank `SM_AUTH_PROVIDER=` gave auth_provider == '', which matches no provider, while resolve_auth_provider() treated blank as the default — so the host and `make doctor` / gen-pages disagreed about the active provider. A field validator normalises blank to the default, mirroring _normalize_trusted_proxy in the same file. Claude-Session: https://claude.ai/code/session_01RtK7xkMUveHaw6TdK1DF7L --- .../core/simple_module_core/discovery.py | 23 +++++++- .../tests/test_auth_provider_selection.py | 25 +++++++- .../simple_module_hosting/app_builder.py | 6 +- .../bootstrap_settings.py | 13 ++++ framework/hosting/tests/test_app.py | 12 ++-- .../tests/test_auth_provider_setting.py | 59 +++++++++++++++++++ .../testing/simple_module_test/fixtures.py | 40 ++++++++++++- .../testing/simple_module_test/plugin.py | 1 + 8 files changed, 167 insertions(+), 12 deletions(-) create mode 100644 framework/hosting/tests/test_auth_provider_setting.py diff --git a/framework/core/simple_module_core/discovery.py b/framework/core/simple_module_core/discovery.py index ea773a0c..b2b2568c 100644 --- a/framework/core/simple_module_core/discovery.py +++ b/framework/core/simple_module_core/discovery.py @@ -139,6 +139,8 @@ def resolve_auth_provider() -> str: 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. @@ -149,16 +151,31 @@ def select_auth_provider( 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 — nor when - ``preferred`` names none of them, since that's real misconfiguration and - SM020 states it better than a silent pick would. + 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( diff --git a/framework/core/tests/test_auth_provider_selection.py b/framework/core/tests/test_auth_provider_selection.py index 0a0ef7b9..bc9e41d0 100644 --- a/framework/core/tests/test_auth_provider_selection.py +++ b/framework/core/tests/test_auth_provider_selection.py @@ -11,6 +11,7 @@ resolve_auth_provider, select_auth_provider, ) +from simple_module_core.exceptions import InvalidModuleError from simple_module_core.module import ModuleBase, ModuleMeta @@ -59,11 +60,33 @@ 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_leaves_the_conflict_for_sm020(self): + 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()]) diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index 48994f08..d712a5e0 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -122,7 +122,11 @@ def create_app(settings: Settings | None = None) -> FastAPI: ) # Two auth providers can be installed at once (they are in this workspace); # only the configured one is activated. See select_auth_provider / SM020. - modules = select_auth_provider(modules, settings.auth_provider) + # 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 fa843707..142103e5 100644 --- a/framework/hosting/simple_module_hosting/bootstrap_settings.py +++ b/framework/hosting/simple_module_hosting/bootstrap_settings.py @@ -64,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/tests/test_app.py b/framework/hosting/tests/test_app.py index 6be967a3..b7bc31ca 100644 --- a/framework/hosting/tests/test_app.py +++ b/framework/hosting/tests/test_app.py @@ -168,14 +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("") - # Every installed module, Keycloak included: both auth providers are - # entry points in the dev workspace and create_app is expected to - # activate one (SM_AUTH_PROVIDER) rather than fail the boot on SM020. - app = create_app(Settings()) + # 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 [m.meta.name for m in sm.modules].count("Keycloak") == 0 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, )