From f994e7bc3454fde60080bd3191546b5f63ec47a3 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 14:20:31 +0200 Subject: [PATCH 1/9] feat(core): UserDirectory and ActivityFeed protocols (#501) Core protocols so modules read users and recent activity without importing a provider module: DirectoryUser/DirectoryStats, the UserDirectory protocol with a NullUserDirectory fallback, the shared naming rule (DirectoryUser.label), id parsing/chunking helpers, and the ActivityFeed protocol, each registered on app.state. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../specs/2026-10-21-user-directory-design.md | 102 ++++++++++ .../core/simple_module_core/activity_feed.py | 75 +++++++ .../core/simple_module_core/user_directory.py | 187 ++++++++++++++++++ .../core/tests/test_core_user_directory.py | 143 ++++++++++++++ 4 files changed, 507 insertions(+) create mode 100644 docs/superpowers/specs/2026-10-21-user-directory-design.md create mode 100644 framework/core/simple_module_core/activity_feed.py create mode 100644 framework/core/simple_module_core/user_directory.py create mode 100644 framework/core/tests/test_core_user_directory.py diff --git a/docs/superpowers/specs/2026-10-21-user-directory-design.md b/docs/superpowers/specs/2026-10-21-user-directory-design.md new file mode 100644 index 00000000..fe7bb843 --- /dev/null +++ b/docs/superpowers/specs/2026-10-21-user-directory-design.md @@ -0,0 +1,102 @@ +# User directory protocol (#501) + +Approved 2026-10-21. + +## Problem + +`audit_log`, `dashboard`, `file_storage`, `permissions`, `api_tokens` and `auth` +import `users.models` directly, so on a Keycloak install (where +`select_auth_provider` drops `Users`) they silently query empty or stale +`users_*` tables. `users` ↔ `audit_log` import each other (cycle). +`topological_sort` only warns on a missing `depends_on` target. No module has a +foreign key to `users_user`; the only thing that genuinely needs the users +schema is the `Role` table (permissions role pages, `/roles/{role_id}`). + +## Design + +### Core: `simple_module_core/user_directory.py` + +```python +@dataclass(frozen=True) +class DirectoryUser: + id: str + email: str + full_name: str | None = None + is_active: bool = True + roles: tuple[str, ...] | None = None # None = provider doesn't know + + @property + def label(self) -> str: # the one naming rule + return self.full_name or self.email + + +@dataclass(frozen=True) +class DirectoryStats: + total: int | None + active_since: int | None # last login within the window + created_this_month: int | None + + +class UserDirectory(Protocol): + entity_type: str | None # audit-link key ("User"); None = unlinked + + async def get_many( + self, db, ids: Collection[str], *, with_roles: bool = False + ) -> dict[str, DirectoryUser]: ... + async def search(self, db, term: str | None, *, limit: int) -> list[DirectoryUser]: ... + def match_ids(self, term: str) -> Any | None: ... # SELECT of text ids, or None + async def stats(self, db, *, active_window: timedelta) -> DirectoryStats: ... +``` + +- `get_many`: keyed by the caller's id strings; non-UUID or unknown ids are + omitted, never raise; implementations chunk (≤500 ids per query). +- Registration: `set_user_directory(app, d)` / `get_user_directory(app)` on + `app.state`. The active auth provider registers it in `register_settings`. + No provider → `NullUserDirectory` (empty results, `None` stats, `match_ids` + → `None`); callers fall back to raw ids. +- `db` typed `Any` (core has no SQLAlchemy dependency). + +### Activity feed (breaks the users ↔ audit_log cycle) + +Core `ActivityFeed` protocol: `async recent_for_actor(db, actor_id, *, limit, +permissions) -> list[ActivityItem]` (`at`, `action`, `entity_type`, +`entity_id`, `entity_label | None`); `set_activity_feed` / `get_activity_feed` +on `app.state`. `audit_log` registers it (reusing `AuditLogService` and +`resolve_entity_labels` with its permission gating unchanged, GH #300). +`users/admin/recent_activity.py` drops its lazy `audit_log` imports and module +name check; a missing feed returns `None` (keeps "no card" vs "no activity"). + +### Implementations + +- `users/directory.py`; `users/audit.py`'s `resolve_user_labels` delegates to it. +- `keycloak`: over `KeycloakUserCache` (users who have signed in at least + once); roles `None`; `created_this_month` `None`. + +### Ports + +- `audit_log`: `resolve_actors`, `actor_filter` (stays an unbounded subquery via + `match_ids`; `None` → exact id match only), `USER_ENTITY_TYPE` from the + directory; drop `depends_on=[Users]`. +- `dashboard`: tiles from `stats()`, a `None` tile is hidden; drop `Users` dep. +- `file_storage`: delete `resolve.py`'s copy, use the directory. +- `api_tokens`: owner existence/active and emails via `get_many`; PAT + resolution via `get_many(with_roles=True)`. Personal tokens stay refused + when roles are unknown (Keycloak). Keeps `depends_on=[Users]`. +- `permissions`: user side (`get_many(with_roles=True)`, `search`); role pages + keep `Users` (import `users.contracts`, not `users.models`); `ADMIN_ROLE` + from core instead of `users.constants`. +- `auth`: structural `Protocol` replaces the `users.User` type hint. +- `background_tasks`: drop the unused `depends_on=[Users]` (test that ordering + doesn't matter). + +### Strict discovery (last commit) + +`topological_sort(modules, *, strict=False)` collects all missing pairs; strict +raises one `MissingDependencyError(InvalidModuleError)` naming each. +`app_builder` passes `strict=not is_development`; doctor reports a new SM +diagnostic; the outbox worker stays lenient. + +## Not in scope + +Role pages / personal API tokens for Keycloak (follow-up: key roles by name). +`framework/testing` importing `users.bootstrap`. No migrations. diff --git a/framework/core/simple_module_core/activity_feed.py b/framework/core/simple_module_core/activity_feed.py new file mode 100644 index 00000000..ec82492a --- /dev/null +++ b/framework/core/simple_module_core/activity_feed.py @@ -0,0 +1,75 @@ +"""The activity feed: "what has this person done lately", without the audit log. + +The users edit page shows a card of the account's recent actions. The audit log +is what records them, but ``users`` must not import ``audit_log`` (and +``audit_log`` must not import ``users``): each is optional to the other, and +importing both ways was a cycle (GH #501). + +So ``audit_log`` registers an :class:`ActivityFeed` with +:func:`set_activity_feed` and the card asks :func:`get_activity_feed`. ``None`` +means nothing records activity on this install — a card that is absent, which +is a different claim from an empty one ("nobody did anything"). +""" + +from __future__ import annotations + +from collections.abc import Collection +from dataclasses import dataclass +from datetime import datetime +from typing import Any, Protocol, runtime_checkable + +ACTIVITY_FEED_STATE_ATTR = "activity_feed" +"""``app.state`` attribute holding the active :class:`ActivityFeed`.""" + + +@dataclass(frozen=True) +class ActivityItem: + """One thing an actor did.""" + + at: datetime + action: str + entity_type: str + entity_id: str + #: The row's display name, when its owner can name it and the reader may + #: see that name; ``None`` otherwise (the caller shows a short id). + entity_label: str | None = None + #: The recorded change set, whatever its shape, for summarising which + #: fields moved. ``None`` when the recorder kept none. + changes: Any = None + + +@runtime_checkable +class ActivityFeed(Protocol): + async def recent_for_actor( + self, + db: Any, + actor_id: str, + *, + limit: int, + permissions: Collection[str], + ) -> list[ActivityItem]: + """The latest *limit* actions by *actor_id*, newest first. + + *permissions* are the reader's grants: labels whose owner gated them + behind a permission the reader lacks come back as ``None``. + """ + ... + + +def set_activity_feed(app: Any, feed: ActivityFeed | None) -> None: + """Install (or, with ``None``, remove) the app's activity feed.""" + setattr(app.state, ACTIVITY_FEED_STATE_ATTR, feed) + + +def get_activity_feed(app: Any) -> ActivityFeed | None: + """The app's activity feed, or ``None`` when nothing records activity.""" + return getattr(app.state, ACTIVITY_FEED_STATE_ATTR, None) + + +__all__ = [ + "ACTIVITY_FEED_STATE_ATTR", + "ActivityFeed", + "ActivityItem", + "get_activity_feed", + "set_activity_feed", +] diff --git a/framework/core/simple_module_core/user_directory.py b/framework/core/simple_module_core/user_directory.py new file mode 100644 index 00000000..d04fc6ea --- /dev/null +++ b/framework/core/simple_module_core/user_directory.py @@ -0,0 +1,187 @@ +"""The user directory: how modules read *about* users without owning them. + +Which module holds the user records depends on the auth provider: ``users`` +keeps them in its own tables, ``keycloak`` keeps a cache of everyone who has +signed in. Code that only needs to *name* a user, list a few, or count them +(the audit log, the dashboard, file storage, API tokens, permissions) asks +through this protocol instead of importing either module's models, so it keeps +working whichever provider is installed (GH #501). + +The active auth provider registers its directory with +:func:`set_user_directory` in ``register_settings``. With none registered +:func:`get_user_directory` returns :class:`NullUserDirectory`, which knows +nobody: callers fall back to the raw id, which is still the truthful record. + +The ``db`` arguments are typed ``Any`` because ``simple_module_core`` must not +depend on SQLAlchemy; in practice it is the request's ``AsyncSession``. +""" + +from __future__ import annotations + +import uuid +from collections.abc import Collection, Iterable, Iterator, Sequence +from dataclasses import dataclass +from datetime import timedelta +from typing import Any, Protocol, runtime_checkable + +USER_DIRECTORY_STATE_ATTR = "user_directory" +"""``app.state`` attribute holding the active :class:`UserDirectory`.""" + +GET_MANY_CHUNK = 500 +"""Most ids an implementation puts in one ``IN`` list. + +A page of audit rows is far below it; an export or a bulk lookup is not, and +Postgres caps bind parameters per statement, so implementations chunk.""" + + +@dataclass(frozen=True) +class DirectoryUser: + """One user, as much as the directory knows about them.""" + + id: str + email: str + full_name: str | None = None + is_active: bool = True + #: Role names, or ``None`` when the provider does not know them (Keycloak + #: keeps roles in the IdP). Only filled when ``with_roles=True`` was asked. + roles: tuple[str, ...] | None = None + + @property + def label(self) -> str: + """The one naming rule: the full name, else the email. + + An invited account has no name until it is accepted, so the email is + what identifies it until then. Every screen that names a user uses + this, so one row is never called two things on one page. + """ + return self.full_name or self.email + + +@dataclass(frozen=True) +class DirectoryStats: + """Headline counts. ``None`` means the provider cannot say, not zero.""" + + total: int | None + #: Users whose last sign-in falls inside the requested window. + active_since: int | None + created_this_month: int | None + + +@runtime_checkable +class UserDirectory(Protocol): + #: The audit-link key user rows are recorded under (``"User"``), or + #: ``None`` when the provider's users have no audited model to link to. + entity_type: str | None + + async def get_many( + self, db: Any, ids: Collection[str], *, with_roles: bool = False + ) -> dict[str, DirectoryUser]: + """Map each known id to its user. + + Keyed by the caller's own id strings. Ids that are not uuids, or name + nobody, are absent from the result — never an error. + """ + ... + + async def search(self, db: Any, term: str | None, *, limit: int) -> list[DirectoryUser]: + """Users whose name or email contains *term* (all users when empty).""" + ... + + def match_ids(self, term: str) -> Any | None: + """A SQL ``SELECT`` of the text ids whose name or email contains *term*. + + Returned unexecuted so a caller can embed it as a subquery with no + ceiling on how many ids match. ``None`` when the provider cannot express + the match in SQL; the caller then matches the term as an id. + """ + ... + + async def stats(self, db: Any, *, active_window: timedelta) -> DirectoryStats: + """Headline counts for the dashboard.""" + ... + + +class NullUserDirectory: + """The directory when no auth provider registered one: it knows nobody.""" + + entity_type: str | None = None + + async def get_many( + self, db: Any, ids: Collection[str], *, with_roles: bool = False + ) -> dict[str, DirectoryUser]: + return {} + + async def search(self, db: Any, term: str | None, *, limit: int) -> list[DirectoryUser]: + return [] + + def match_ids(self, term: str) -> Any | None: + return None + + async def stats(self, db: Any, *, active_window: timedelta) -> DirectoryStats: + return DirectoryStats(total=None, active_since=None, created_this_month=None) + + +_NULL_DIRECTORY = NullUserDirectory() + + +def set_user_directory(app: Any, directory: UserDirectory) -> None: + """Install *directory* as the app's user directory (the auth provider does).""" + setattr(app.state, USER_DIRECTORY_STATE_ATTR, directory) + + +def get_user_directory(app: Any) -> UserDirectory: + """The app's user directory, or a :class:`NullUserDirectory`.""" + directory = getattr(app.state, USER_DIRECTORY_STATE_ATTR, None) + return directory if directory is not None else _NULL_DIRECTORY + + +def parse_user_ids(ids: Collection[str]) -> dict[uuid.UUID, str]: + """Map each id that parses as a uuid to the caller's own spelling of it. + + Implementations key their answer by the caller's strings, so an uppercase + or unhyphenated id still finds its row. Anything else (a worker name, an + ``svc:`` principal, ``None``) belongs to another id space and is dropped. + """ + parsed: dict[uuid.UUID, str] = {} + for raw in ids: + try: + parsed[uuid.UUID(raw)] = raw + except (ValueError, AttributeError, TypeError): + continue + return parsed + + +def chunked[T](items: Sequence[T], size: int = GET_MANY_CHUNK) -> Iterator[Sequence[T]]: + """*items* in slices of at most *size*, for bounded ``IN`` lists.""" + for start in range(0, len(items), size): + yield items[start : start + size] + + +async def resolve_user_labels( + directory: UserDirectory, db: Any, ids: Iterable[str | None] +) -> dict[str, str]: + """Map id -> :attr:`DirectoryUser.label` for every id the directory knows. + + Empty and ``None`` ids (a row with no recorded user) are skipped; ids the + directory cannot name are absent, and callers show the stored id instead. + """ + wanted = {uid for uid in ids if uid} + if not wanted: + return {} + found = await directory.get_many(db, wanted) + return {uid: user.label for uid, user in found.items()} + + +__all__ = [ + "GET_MANY_CHUNK", + "USER_DIRECTORY_STATE_ATTR", + "DirectoryStats", + "DirectoryUser", + "NullUserDirectory", + "UserDirectory", + "chunked", + "get_user_directory", + "parse_user_ids", + "resolve_user_labels", + "set_user_directory", +] diff --git a/framework/core/tests/test_core_user_directory.py b/framework/core/tests/test_core_user_directory.py new file mode 100644 index 00000000..254bbac2 --- /dev/null +++ b/framework/core/tests/test_core_user_directory.py @@ -0,0 +1,143 @@ +"""The core user-directory protocol, its null object and the shared helpers (GH #501).""" + +from __future__ import annotations + +import uuid +from datetime import UTC, datetime, timedelta +from types import SimpleNamespace + +from simple_module_core.activity_feed import ( + ActivityFeed, + ActivityItem, + get_activity_feed, + set_activity_feed, +) +from simple_module_core.user_directory import ( + GET_MANY_CHUNK, + DirectoryStats, + DirectoryUser, + NullUserDirectory, + UserDirectory, + chunked, + get_user_directory, + parse_user_ids, + resolve_user_labels, + set_user_directory, +) + + +def _app() -> SimpleNamespace: + return SimpleNamespace(state=SimpleNamespace()) + + +class _FakeDirectory: + entity_type: str | None = "User" + + def __init__(self, users: list[DirectoryUser]) -> None: + self.users = {u.id: u for u in users} + self.calls: list[set[str]] = [] + + async def get_many(self, db, ids, *, with_roles=False): + self.calls.append(set(ids)) + return {i: self.users[i] for i in ids if i in self.users} + + async def search(self, db, term, *, limit): + return list(self.users.values())[:limit] + + def match_ids(self, term): + return None + + async def stats(self, db, *, active_window): + return DirectoryStats(total=len(self.users), active_since=None, created_this_month=None) + + +class TestDirectoryUser: + def test_label_prefers_the_full_name(self): + assert DirectoryUser(id="1", email="a@x", full_name="Ada").label == "Ada" + + def test_label_falls_back_to_the_email(self): + assert DirectoryUser(id="1", email="a@x").label == "a@x" + assert DirectoryUser(id="1", email="a@x", full_name="").label == "a@x" + + def test_roles_default_to_unknown_not_none_held(self): + assert DirectoryUser(id="1", email="a@x").roles is None + + +class TestNullUserDirectory: + async def test_knows_nobody(self): + d = NullUserDirectory() + assert d.entity_type is None + assert await d.get_many(None, [str(uuid.uuid4())], with_roles=True) == {} + assert await d.search(None, "ada", limit=10) == [] + assert d.match_ids("ada") is None + + async def test_stats_are_unknown_not_zero(self): + stats = await NullUserDirectory().stats(None, active_window=timedelta(days=7)) + assert stats == DirectoryStats(total=None, active_since=None, created_this_month=None) + + def test_satisfies_the_protocol(self): + assert isinstance(NullUserDirectory(), UserDirectory) + + +class TestRegistration: + def test_without_a_provider_the_null_directory_answers(self): + assert isinstance(get_user_directory(_app()), NullUserDirectory) + + def test_the_registered_directory_wins(self): + app = _app() + directory = _FakeDirectory([]) + set_user_directory(app, directory) + assert get_user_directory(app) is directory + + +class TestHelpers: + def test_parse_keeps_the_callers_spelling(self): + uid = uuid.uuid4() + upper = str(uid).upper() + assert parse_user_ids([upper, "celery-worker", "", "svc:1"]) == {uid: upper} + + def test_parse_tolerates_non_strings(self): + assert parse_user_ids([None, 3]) == {} + + def test_chunked_bounds_every_slice(self): + items = list(range(GET_MANY_CHUNK * 2 + 1)) + slices = list(chunked(items)) + assert [len(s) for s in slices] == [GET_MANY_CHUNK, GET_MANY_CHUNK, 1] + assert [x for s in slices for x in s] == items + + async def test_resolve_labels_uses_the_one_naming_rule(self): + directory = _FakeDirectory( + [DirectoryUser(id="a", email="a@x", full_name="Ada"), DirectoryUser("b", "b@x")] + ) + labels = await resolve_user_labels(directory, None, ["a", "b", "gone", "", "a"]) + assert labels == {"a": "Ada", "b": "b@x"} + assert directory.calls == [{"a", "b", "gone"}] + + async def test_resolve_labels_short_circuits_without_ids(self): + directory = _FakeDirectory([]) + assert await resolve_user_labels(directory, None, ["", ""]) == {} + assert directory.calls == [] + + +class _Feed: + async def recent_for_actor(self, db, actor_id, *, limit, permissions): + return [ActivityItem(datetime.now(UTC), "created", "Setting", "k")] + + +class TestActivityFeed: + def test_absent_until_something_records_activity(self): + assert get_activity_feed(_app()) is None + + def test_set_and_remove(self): + app = _app() + feed = _Feed() + set_activity_feed(app, feed) + assert get_activity_feed(app) is feed + assert isinstance(feed, ActivityFeed) + set_activity_feed(app, None) + assert get_activity_feed(app) is None + + def test_item_label_and_changes_are_optional(self): + item = ActivityItem(datetime.now(UTC), "updated", "User", "1") + assert item.entity_label is None + assert item.changes is None From a5d38a275e97c2369e38f89a68fc7ce787ac515e Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 14:20:31 +0200 Subject: [PATCH 2/9] feat(users,keycloak): user directory implementations (#501) UsersDirectory over users_user (roles via users_user_role, disabled accounts inactive); users.audit's label resolver delegates to it. KeycloakDirectory over keycloak_user_cache: users who have signed in, roles unknown, no creation date; match_ids renders hyphenated ids on SQLite where Uuid is bare hex. Each provider registers its directory in register_settings. users.contracts.roles re-exports Role for the permissions role pages. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- modules/keycloak/keycloak/directory.py | 109 ++++++++++++++++ modules/keycloak/keycloak/module.py | 7 + .../keycloak/tests/test_keycloak_directory.py | 107 +++++++++++++++ modules/users/tests/test_users_directory.py | 123 ++++++++++++++++++ modules/users/users/audit.py | 32 ++--- modules/users/users/contracts/roles.py | 11 ++ modules/users/users/directory.py | 115 ++++++++++++++++ modules/users/users/module.py | 8 ++ 8 files changed, 489 insertions(+), 23 deletions(-) create mode 100644 modules/keycloak/keycloak/directory.py create mode 100644 modules/keycloak/tests/test_keycloak_directory.py create mode 100644 modules/users/tests/test_users_directory.py create mode 100644 modules/users/users/contracts/roles.py create mode 100644 modules/users/users/directory.py diff --git a/modules/keycloak/keycloak/directory.py b/modules/keycloak/keycloak/directory.py new file mode 100644 index 00000000..6a3a2ae7 --- /dev/null +++ b/modules/keycloak/keycloak/directory.py @@ -0,0 +1,109 @@ +"""The keycloak module's :class:`~simple_module_core.user_directory.UserDirectory`. + +Keycloak owns the accounts; this app only learns about a user when they sign +in, and caches the stable id it hands out in ``keycloak_user_cache``. So the +directory knows **users who have signed in at least once**, and nothing about +their roles (those live in the IdP and arrive as token claims, never stored) +or when the IdP account was created. +""" + +from __future__ import annotations + +from collections.abc import Collection +from datetime import UTC, datetime, timedelta +from typing import Any + +from simple_module_core.user_directory import ( + DirectoryStats, + DirectoryUser, + chunked, + parse_user_ids, +) +from simple_module_db import LIKE_ESCAPE_CHAR, like_contains_pattern +from sqlalchemy import String, func, or_, select +from sqlalchemy.ext.compiler import compiles +from sqlalchemy.sql.expression import FunctionElement + +from keycloak.models import KeycloakUserCache + + +class _UuidText(FunctionElement): + """A uuid column as the canonical hyphenated text every text id uses. + + ``keycloak_user_cache.id`` is SQLAlchemy's ``Uuid``: native on Postgres, + where a plain cast already renders hyphenated, but 32 bare hex characters + on SQLite — which would never equal the ``str(uuid)`` an audit row stored. + """ + + type = String() + inherit_cache = True + + +@compiles(_UuidText) +def _uuid_text_default(element: Any, compiler: Any, **kw: Any) -> str: + return f"CAST({compiler.process(element.clauses, **kw)} AS VARCHAR)" + + +@compiles(_UuidText, "sqlite") +def _uuid_text_sqlite(element: Any, compiler: Any, **kw: Any) -> str: + col = compiler.process(element.clauses, **kw) + parts = [(1, 8), (9, 4), (13, 4), (17, 4), (21, 12)] + return " || '-' || ".join(f"substr({col}, {start}, {length})" for start, length in parts) + + +_COLUMNS = (KeycloakUserCache.id, KeycloakUserCache.email, KeycloakUserCache.full_name) + + +def _to_directory_user(row: Any, key: str) -> DirectoryUser: + # ``roles`` stays ``None``: unknown, which callers must not read as "none". + return DirectoryUser(id=key, email=row.email, full_name=row.full_name) + + +def _name_or_email_contains(term: str) -> Any: + pattern = like_contains_pattern(term) + return or_( + KeycloakUserCache.full_name.ilike(pattern, escape=LIKE_ESCAPE_CHAR), + KeycloakUserCache.email.ilike(pattern, escape=LIKE_ESCAPE_CHAR), + ) + + +class KeycloakDirectory: + """Directory over ``keycloak_user_cache``.""" + + #: Cache rows are not an audited model with a screen of their own, so actor + #: cells stay unlinked rather than pointing at a users page that is absent. + entity_type: str | None = None + + async def get_many( + self, db: Any, ids: Collection[str], *, with_roles: bool = False + ) -> dict[str, DirectoryUser]: + parsed = parse_user_ids(ids) + found: dict[str, DirectoryUser] = {} + for chunk in chunked(list(parsed)): + stmt = select(*_COLUMNS).where(KeycloakUserCache.id.in_(chunk)) + for row in (await db.execute(stmt)).all(): + key = parsed[row.id] + found[key] = _to_directory_user(row, key) + return found + + async def search(self, db: Any, term: str | None, *, limit: int) -> list[DirectoryUser]: + stmt = select(*_COLUMNS).order_by(KeycloakUserCache.email).limit(limit) + if term and term.strip(): + stmt = stmt.where(_name_or_email_contains(term.strip())) + return [_to_directory_user(row, str(row.id)) for row in (await db.execute(stmt)).all()] + + def match_ids(self, term: str) -> Any | None: + return select(_UuidText(KeycloakUserCache.id)).where(_name_or_email_contains(term)) + + async def stats(self, db: Any, *, active_window: timedelta) -> DirectoryStats: + count = select(func.count()).select_from(KeycloakUserCache) + total = (await db.execute(count)).scalar_one() + cutoff = datetime.now(UTC) - active_window + active = ( + await db.execute(count.where(KeycloakUserCache.last_login_at >= cutoff)) + ).scalar_one() + # The cache row is written at first sign-in, not account creation. + return DirectoryStats(total=total, active_since=active, created_this_month=None) + + +__all__ = ["KeycloakDirectory"] diff --git a/modules/keycloak/keycloak/module.py b/modules/keycloak/keycloak/module.py index 9a9df10f..e5d08541 100644 --- a/modules/keycloak/keycloak/module.py +++ b/modules/keycloak/keycloak/module.py @@ -42,6 +42,13 @@ def register_settings(self, app: FastAPI) -> None: app.state.keycloak.settings, state=app.state.keycloak ) + # Other modules read accounts through the directory (GH #501). + from simple_module_core.user_directory import set_user_directory + + from keycloak.directory import KeycloakDirectory + + set_user_directory(app, KeycloakDirectory()) + def register_menu_items(self, registry: MenuRegistry) -> None: registry.add( MenuItem( diff --git a/modules/keycloak/tests/test_keycloak_directory.py b/modules/keycloak/tests/test_keycloak_directory.py new file mode 100644 index 00000000..e9f0115e --- /dev/null +++ b/modules/keycloak/tests/test_keycloak_directory.py @@ -0,0 +1,107 @@ +"""``KeycloakDirectory``: users who have signed in, over the cache (GH #501).""" + +from __future__ import annotations + +import uuid +from datetime import UTC, datetime, timedelta + +from audit_log.models import AuditEntry +from audit_log.resolve import actor_filter, actor_link, resolve_actors +from keycloak.directory import KeycloakDirectory +from keycloak.models import KeycloakUserCache +from simple_module_core.audit_links import AuditLink, AuditLinkRegistry +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +_D = KeycloakDirectory() + + +async def _cached(db: AsyncSession, email: str, **kw) -> KeycloakUserCache: + row = KeycloakUserCache(keycloak_sub=f"sub-{uuid.uuid4().hex}", email=email, **kw) + db.add(row) + await db.flush() + return row + + +class TestGetMany: + async def test_names_a_cached_user(self, db_session: AsyncSession): + row = await _cached(db_session, "kc@example.com", full_name="Kay Cee") + found = await _D.get_many(db_session, [str(row.id), "svc:1", str(uuid.uuid4())]) + assert list(found) == [str(row.id)] + user = found[str(row.id)] + assert user.label == "Kay Cee" + assert user.is_active + + async def test_roles_are_unknown_even_when_asked(self, db_session: AsyncSession): + """Roles live in the IdP; an empty tuple would claim the user holds none.""" + row = await _cached(db_session, "roles@example.com") + found = await _D.get_many(db_session, [str(row.id)], with_roles=True) + assert found[str(row.id)].roles is None + + +class TestSearchAndMatch: + async def test_search(self, db_session: AsyncSession): + await _cached(db_session, "b-okafor@example.com") + await _cached(db_session, "a@example.com", full_name="Okafor A") + await _cached(db_session, "nobody@example.com") + found = await _D.search(db_session, "okafor", limit=10) + assert [u.email for u in found] == ["a@example.com", "b-okafor@example.com"] + + async def test_match_ids_renders_hyphenated_ids_on_sqlite(self, db_session: AsyncSession): + """``Uuid`` is 32 bare hex characters on SQLite; the audit trail stores + ``str(uuid)``. Without the hyphenating expression nothing would match.""" + row = await _cached(db_session, "hyphen@example.com", full_name="Hyphen") + ids = (await db_session.execute(_D.match_ids("hyphen"))).scalars().all() + assert ids == [str(row.id)] + + +class TestStats: + async def test_counts_signed_in_users_and_cannot_date_creation(self, db_session: AsyncSession): + await _cached(db_session, "now@example.com", last_login_at=datetime.now(UTC)) + await _cached( + db_session, "old@example.com", last_login_at=datetime.now(UTC) - timedelta(days=60) + ) + stats = await _D.stats(db_session, active_window=timedelta(days=7)) + assert stats.total == 2 + assert stats.active_since == 1 + assert stats.created_this_month is None + + +class TestAuditLogUnderKeycloak: + """The audit log names and finds Keycloak actors — it used to query the + empty ``users_user`` table on a Keycloak install.""" + + async def test_actor_names_and_search(self, db_session: AsyncSession): + row = await _cached(db_session, "auditor@example.com", full_name="Audrey") + db_session.add( + AuditEntry( + entity_type="Setting", + entity_id="k", + action="updated", + changes=[], + user_id=str(row.id), + ) + ) + db_session.add( + AuditEntry( + entity_type="Setting", + entity_id="k", + action="updated", + changes=[], + user_id=str(uuid.uuid4()), + ) + ) + await db_session.flush() + + assert await resolve_actors(_D, db_session, [str(row.id)]) == {str(row.id): "Audrey"} + matched = ( + (await db_session.execute(select(AuditEntry.user_id).where(actor_filter(_D, "audr")))) + .scalars() + .all() + ) + assert matched == [str(row.id)] + + def test_actor_cells_are_unlinked(self): + registry = AuditLinkRegistry() + registry.register(AuditLink(entity_type="User", url_template="/admin/users/{id}")) + assert actor_link(registry, _D, "anything") is None diff --git a/modules/users/tests/test_users_directory.py b/modules/users/tests/test_users_directory.py new file mode 100644 index 00000000..b666217b --- /dev/null +++ b/modules/users/tests/test_users_directory.py @@ -0,0 +1,123 @@ +"""``UsersDirectory``: the users module's answer to the core directory (GH #501).""" + +from __future__ import annotations + +import uuid +from datetime import UTC, datetime, timedelta + +from simple_module_core.user_directory import GET_MANY_CHUNK, get_user_directory +from sqlalchemy.ext.asyncio import AsyncSession +from users.directory import UsersDirectory +from users.models import Role, User, UserRole + +_D = UsersDirectory() + + +async def _user(db: AsyncSession, email: str, **kw) -> User: + user = User(email=email, hashed_password="x", is_active=True, **kw) + db.add(user) + await db.flush() + return user + + +class TestGetMany: + async def test_keys_by_the_callers_spelling(self, db_session: AsyncSession): + user = await _user(db_session, "ada@example.com", full_name="Ada") + upper = str(user.id).upper() + found = await _D.get_many(db_session, [upper]) + assert list(found) == [upper] + assert found[upper].label == "Ada" + assert found[upper].id == upper + + async def test_unknown_and_foreign_ids_are_absent(self, db_session: AsyncSession): + found = await _D.get_many(db_session, [str(uuid.uuid4()), "celery-worker-1", ""]) + assert found == {} + + async def test_roles_only_when_asked(self, db_session: AsyncSession): + user = await _user(db_session, "roles@example.com") + role = Role(name=f"editor-{uuid.uuid4().hex[:6]}") + db_session.add(role) + await db_session.flush() + db_session.add(UserRole(user_id=user.id, role_id=role.id)) + await db_session.flush() + + plain = await _D.get_many(db_session, [str(user.id)]) + assert plain[str(user.id)].roles is None + with_roles = await _D.get_many(db_session, [str(user.id)], with_roles=True) + assert with_roles[str(user.id)].roles == (role.name,) + + async def test_a_user_without_roles_holds_none_rather_than_unknown( + self, db_session: AsyncSession + ): + user = await _user(db_session, "bare@example.com") + found = await _D.get_many(db_session, [str(user.id)], with_roles=True) + assert found[str(user.id)].roles == () + + async def test_disabled_or_inactive_is_not_active(self, db_session: AsyncSession): + disabled = await _user(db_session, "off@example.com", disabled_at=datetime.now(UTC)) + inactive = await _user(db_session, "inactive@example.com") + inactive.is_active = False + await db_session.flush() + found = await _D.get_many(db_session, [str(disabled.id), str(inactive.id)]) + assert not found[str(disabled.id)].is_active + assert not found[str(inactive.id)].is_active + + async def test_more_ids_than_one_chunk(self, db_session: AsyncSession): + user = await _user(db_session, "chunk@example.com") + ids = [str(uuid.uuid4()) for _ in range(GET_MANY_CHUNK + 10)] + [str(user.id)] + found = await _D.get_many(db_session, ids) + assert list(found) == [str(user.id)] + + +class TestSearchAndMatch: + async def test_search_by_name_or_email(self, db_session: AsyncSession): + await _user(db_session, "zed@example.com", full_name="Grace Okafor") + await _user(db_session, "okafor.b@example.com") + await _user(db_session, "other@example.com") + found = await _D.search(db_session, "okafor", limit=10) + assert [u.email for u in found] == ["okafor.b@example.com", "zed@example.com"] + + async def test_empty_term_lists_everyone_up_to_the_limit(self, db_session: AsyncSession): + for n in range(3): + await _user(db_session, f"all{n}@example.com") + assert len(await _D.search(db_session, None, limit=2)) == 2 + assert len(await _D.search(db_session, " ", limit=10)) >= 3 + + async def test_match_ids_selects_the_text_form_audit_rows_store(self, db_session: AsyncSession): + user = await _user(db_session, "match@example.com", full_name="Matchable") + ids = (await db_session.execute(_D.match_ids("matchab"))).scalars().all() + assert ids == [str(user.id)] + + async def test_like_metacharacters_are_literal(self, db_session: AsyncSession): + await _user(db_session, "dana_x@example.com") + await _user(db_session, "danaYx@example.com") + found = await _D.search(db_session, "dana_x", limit=10) + assert [u.email for u in found] == ["dana_x@example.com"] + + +class TestStats: + async def test_counts(self, db_session: AsyncSession): + before = await _D.stats(db_session, active_window=timedelta(days=7)) + await _user(db_session, "recent@example.com", last_login_at=datetime.now(UTC)) + await _user( + db_session, + "stale@example.com", + last_login_at=datetime.now(UTC) - timedelta(days=30), + ) + after = await _D.stats(db_session, active_window=timedelta(days=7)) + assert after.total == (before.total or 0) + 2 + assert after.active_since == (before.active_since or 0) + 1 + assert after.created_this_month == (before.created_this_month or 0) + 2 + + +class TestRegistration: + async def test_users_module_installs_its_directory(self, app): + assert isinstance(get_user_directory(app), UsersDirectory) + + async def test_audit_label_resolver_delegates(self, db_session: AsyncSession): + from users.audit import resolve_user_labels + + user = await _user(db_session, "label@example.com") + assert await resolve_user_labels(db_session, [str(user.id)]) == { + str(user.id): "label@example.com" + } diff --git a/modules/users/users/audit.py b/modules/users/users/audit.py index 54804245..10fa9187 100644 --- a/modules/users/users/audit.py +++ b/modules/users/users/audit.py @@ -13,45 +13,31 @@ from __future__ import annotations -import uuid +from typing import Any from simple_module_core.audit_links import AuditLink -from sqlalchemy import select -from sqlalchemy.ext.asyncio import AsyncSession +from simple_module_core.user_directory import resolve_user_labels as _labels_via from users.constants import PERM_USERS_MANAGE +from users.directory import UsersDirectory from users.models import User _LABEL = "User" _LABEL_KEY = "users.audit.user" +_DIRECTORY = UsersDirectory() -async def resolve_user_labels(db: AsyncSession, ids: list[str]) -> dict[str, str]: + +async def resolve_user_labels(db: Any, ids: list[str]) -> dict[str, str]: """Map user id -> display name for every id given. Ids that do not parse as uuids belong to some other id space (a worker name, say) and ids that no longer resolve belong to deleted accounts. Both are simply absent from the result rather than an error: the caller - falls back to the raw id, which is still the truthful record. + falls back to the raw id, which is still the truthful record. The naming + rule itself is ``DirectoryUser.label``, shared with every other screen. """ - parsed: dict[uuid.UUID, str] = {} - for raw in ids: - try: - parsed[uuid.UUID(raw)] = raw - except (ValueError, AttributeError, TypeError): - continue - if not parsed: - return {} - - rows = ( - await db.execute( - select(User.id, User.email, User.full_name).where(User.id.in_(list(parsed))) - ) - ).all() - return { - parsed.get(user_id, str(user_id)): (full_name or email) - for user_id, email, full_name in rows - } + return await _labels_via(_DIRECTORY, db, ids) def build_user_audit_link(admin_url_prefix: str) -> AuditLink: diff --git a/modules/users/users/contracts/roles.py b/modules/users/users/contracts/roles.py new file mode 100644 index 00000000..eb1f06f6 --- /dev/null +++ b/modules/users/users/contracts/roles.py @@ -0,0 +1,11 @@ +"""The role table, as other modules may read it. + +Users are read through the app's user directory (GH #501); roles have no such +protocol yet, so the ``permissions`` role pages read this table directly and +keep ``Users`` in their ``depends_on``. Import it from here, not from +``users.models``, so the dependency stays on the published surface. +""" + +from users.models.role import Role + +__all__ = ["Role"] diff --git a/modules/users/users/directory.py b/modules/users/users/directory.py new file mode 100644 index 00000000..fd17248b --- /dev/null +++ b/modules/users/users/directory.py @@ -0,0 +1,115 @@ +"""The users module's :class:`~simple_module_core.user_directory.UserDirectory`. + +Other modules read user records through ``get_user_directory(app)`` rather than +importing ``users.models`` (GH #501), so the same audit log, dashboard and +permissions screens work whether ``users`` or ``keycloak`` holds the accounts. +This is the implementation over ``users_user``; it is installed in +``UsersModule.register_settings``. +""" + +from __future__ import annotations + +import uuid +from collections.abc import Collection +from datetime import UTC, datetime, timedelta +from typing import Any + +from simple_module_core.user_directory import ( + DirectoryStats, + DirectoryUser, + chunked, + parse_user_ids, +) +from simple_module_db import LIKE_ESCAPE_CHAR, like_contains_pattern +from sqlalchemy import String, cast, func, or_, select + +from users.models import Role, User, UserRole + +# Columns only: loading ``User`` itself would also selectin-load its OAuth +# accounts, a second query per lookup that nothing here reads. +_COLUMNS = (User.id, User.email, User.full_name, User.is_active, User.disabled_at) + + +def _to_directory_user(row: Any, roles: tuple[str, ...] | None, key: str) -> DirectoryUser: + return DirectoryUser( + id=key, + email=row.email, + full_name=row.full_name, + # A disabled account is still ``is_active`` in fastapi-users' sense; + # to every other module it is simply not usable. + is_active=bool(row.is_active) and row.disabled_at is None, + roles=roles, + ) + + +def _name_or_email_contains(term: str) -> Any: + pattern = like_contains_pattern(term) + return or_( + User.full_name.ilike(pattern, escape=LIKE_ESCAPE_CHAR), + User.email.ilike(pattern, escape=LIKE_ESCAPE_CHAR), + ) + + +class UsersDirectory: + """Directory over the local ``users_user`` table.""" + + #: Audit entries record ``type(obj).__name__``; the users module registers + #: its audit link under the same class name. + entity_type: str | None = User.__name__ + + async def get_many( + self, db: Any, ids: Collection[str], *, with_roles: bool = False + ) -> dict[str, DirectoryUser]: + parsed = parse_user_ids(ids) + if not parsed: + return {} + found: dict[str, DirectoryUser] = {} + for chunk in chunked(list(parsed)): + rows = (await db.execute(select(*_COLUMNS).where(User.id.in_(chunk)))).all() + roles = await self._roles_for(db, chunk) if with_roles else {} + for row in rows: + key = parsed[row.id] + user_roles = tuple(sorted(roles.get(row.id, ()))) if with_roles else None + found[key] = _to_directory_user(row, user_roles, key) + return found + + async def _roles_for( + self, db: Any, user_ids: Collection[uuid.UUID] + ) -> dict[uuid.UUID, list[str]]: + rows = await db.execute( + select(UserRole.user_id, Role.name) + .join(Role, Role.id == UserRole.role_id) + .where(UserRole.user_id.in_(list(user_ids))) + ) + roles: dict[uuid.UUID, list[str]] = {} + for user_id, name in rows.all(): + roles.setdefault(user_id, []).append(name) + return roles + + async def search(self, db: Any, term: str | None, *, limit: int) -> list[DirectoryUser]: + stmt = select(*_COLUMNS).order_by(User.email).limit(limit) + if term and term.strip(): + stmt = stmt.where(_name_or_email_contains(term.strip())) + rows = (await db.execute(stmt)).all() + return [_to_directory_user(row, None, str(row.id)) for row in rows] + + def match_ids(self, term: str) -> Any | None: + # ``CAST`` renders the canonical lowercase hyphenated form on both + # dialects (Postgres from ``uuid``, SQLite from GUID's ``CHAR(36)``), + # which is the form the audit trail and every other text id stores. + return select(cast(User.id, String)).where(_name_or_email_contains(term)) + + async def stats(self, db: Any, *, active_window: timedelta) -> DirectoryStats: + now = datetime.now(UTC) + # Calendar month, not a rolling 30 days: the tile reads "+6 this month". + month_start = now.replace(day=1, hour=0, minute=0, second=0, microsecond=0) + count = select(func.count()).select_from(User) + total = (await db.execute(count)).scalar_one() + active = ( + await db.execute(count.where(User.last_login_at >= now - active_window)) + ).scalar_one() + created = (await db.execute(count.where(User.created_at >= month_start))).scalar_one() + return DirectoryStats(total=total, active_since=active, created_this_month=created) + + +__all__ = ["UsersDirectory"] diff --git a/modules/users/users/module.py b/modules/users/users/module.py index d8d6a1b9..5af86270 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -65,6 +65,14 @@ def register_settings(self, app: FastAPI) -> None: app.state.auth.auth_provider = UsersAuthProvider() + # Other modules read accounts through the directory, never + # ``users.models`` (GH #501). + from simple_module_core.user_directory import set_user_directory + + from users.directory import UsersDirectory + + set_user_directory(app, UsersDirectory()) + # The public shell renders a "Sign up" link only when signup is open — # /users/register 404s otherwise. Registered here (not in the view # layer) so it is set before the first request is served. From 43798f81829f28fc527bdb91a66acb1731a70280 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 14:20:59 +0200 Subject: [PATCH 3/9] refactor(audit_log,users): read actors via the user directory; activity feed breaks the cycle (#501) audit_log no longer imports users: actors are named and searched through the app's user directory (actor_filter stays an unbounded SQL subquery via match_ids, and falls back to an exact id match without one), actor links key off the directory's entity_type, and depends_on=[Users] is gone. audit_log registers an ActivityFeed; the users edit page's Recent activity card reads it instead of lazily importing audit_log, keeping 'no feed' (no card) distinct from 'no activity' (empty card). Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- modules/audit_log/audit_log/activity.py | 58 ++++++++++ modules/audit_log/audit_log/endpoints/api.py | 5 +- .../audit_log/audit_log/endpoints/views.py | 8 +- modules/audit_log/audit_log/export.py | 4 +- modules/audit_log/audit_log/module.py | 11 +- modules/audit_log/audit_log/resolve.py | 100 ++++++++---------- .../tests/test_actor_filter_by_name.py | 14 ++- .../tests/test_audit_log_activity_feed.py | 60 +++++++++++ .../audit_log/tests/test_audit_log_resolve.py | 33 +++--- .../users/tests/test_edit_recent_activity.py | 20 ++-- modules/users/users/admin/recent_activity.py | 87 +++++---------- 11 files changed, 253 insertions(+), 147 deletions(-) create mode 100644 modules/audit_log/audit_log/activity.py create mode 100644 modules/audit_log/tests/test_audit_log_activity_feed.py diff --git a/modules/audit_log/audit_log/activity.py b/modules/audit_log/audit_log/activity.py new file mode 100644 index 00000000..eb79e196 --- /dev/null +++ b/modules/audit_log/audit_log/activity.py @@ -0,0 +1,58 @@ +"""The audit log as the app's :class:`~simple_module_core.activity_feed.ActivityFeed`. + +The users edit page shows what an account did lately. It asks the core +activity feed rather than importing this module (GH #501): ``users`` and +``audit_log`` are each optional to the other, and importing both ways was a +cycle. Installed in :meth:`AuditLogModule.register_settings`. +""" + +from __future__ import annotations + +from collections.abc import Collection +from typing import Any + +from simple_module_core.activity_feed import ActivityItem + +from audit_log.resolve import resolve_entity_labels +from audit_log.service import AuditLogService + + +class AuditActivityFeed: + """Recent audit entries by one actor, with their rows named.""" + + def __init__(self, app: Any) -> None: + # The app, not its registry: the audit-link registry is assembled after + # ``register_settings`` runs, so it is read per call. + self._app = app + + async def recent_for_actor( + self, + db: Any, + actor_id: str, + *, + limit: int, + permissions: Collection[str], + ) -> list[ActivityItem]: + page = await AuditLogService(db).list_entries(user_id=actor_id, page_size=limit) + refs = [(entry.entity_type, entry.entity_id) for entry in page.items] + # The reader's grants reach the resolvers, which skip entity types + # whose owner gated their names (GH #300), exactly as the audit screen. + labels = ( + await resolve_entity_labels(db, self._app.state.sm.audit_links, refs, permissions) + if refs + else {} + ) + return [ + ActivityItem( + at=entry.created_at, + action=entry.action, + entity_type=entry.entity_type, + entity_id=entry.entity_id, + entity_label=labels.get((entry.entity_type, entry.entity_id)), + changes=entry.changes, + ) + for entry in page.items + ] + + +__all__ = ["AuditActivityFeed"] diff --git a/modules/audit_log/audit_log/endpoints/api.py b/modules/audit_log/audit_log/endpoints/api.py index a2d53d3a..ba53042d 100644 --- a/modules/audit_log/audit_log/endpoints/api.py +++ b/modules/audit_log/audit_log/endpoints/api.py @@ -6,6 +6,7 @@ from fastapi import APIRouter, Depends, Query, Request from fastapi.responses import StreamingResponse +from simple_module_core.user_directory import get_user_directory from simple_module_db.deps import get_db from simple_module_hosting.permissions import RequiresPermission, resolved_permissions_for from sqlalchemy.ext.asyncio import AsyncSession @@ -75,11 +76,12 @@ async def export_audit_entries( resolved through names, both exactly as the screen does them, so the file and the page always agree about which rows matched. """ + directory = get_user_directory(request.app) filters = EntryFilters.for_date_only_range( entity_type=entity_type, entity_id=entity_id, action=action, - actor_match=actor_filter(user_id or ""), + actor_match=actor_filter(directory, user_id or ""), correlation_id=correlation_id, from_date=from_date, to_date=to_date, @@ -94,6 +96,7 @@ async def export_audit_entries( service, db, request.app.state.sm.audit_links, + directory, filters, resolved_permissions_for(request), ), diff --git a/modules/audit_log/audit_log/endpoints/views.py b/modules/audit_log/audit_log/endpoints/views.py index 0c546f68..9d16dc81 100644 --- a/modules/audit_log/audit_log/endpoints/views.py +++ b/modules/audit_log/audit_log/endpoints/views.py @@ -7,6 +7,7 @@ from fastapi import APIRouter, Depends, Query, Request from simple_module_core.audit_links import AuditLinkRegistry +from simple_module_core.user_directory import get_user_directory from simple_module_db.deps import get_db from simple_module_hosting.i18n_deps import TranslatorDep from simple_module_hosting.inertia_deps import InertiaDep @@ -84,6 +85,7 @@ async def browse( page: str | None = Query(default=None), page_size: str | None = Query(default=None), ) -> InertiaResponse: + directory = get_user_directory(request.app) # Sanitize pagination — never raise a validation error for bad values. page_int = max(_safe_int(page, 1), 1) page_size_int = max(1, min(_safe_int(page_size, DEFAULT_PAGE_SIZE), MAX_PAGE_SIZE)) @@ -95,7 +97,7 @@ async def browse( entity_type=entity_type, entity_id=entity_id, action=action, - actor_match=actor_filter(actor_term), + actor_match=actor_filter(directory, actor_term), correlation_id=correlation_id, from_date=from_date, to_date=to_date, @@ -117,7 +119,7 @@ async def browse( # is what makes it a durable record. links = request.app.state.sm.audit_links entity_types = _type_options(await service.distinct_entity_types(), links) - actors = await resolve_actors(db, [item.user_id for item in result.items]) + actors = await resolve_actors(directory, db, [item.user_id for item in result.items]) # Entity names are resolved against what *this* reader may see: naming the # row is a read of the row, and `audit_log.view` alone must not be a way # round the permission its owning module puts on that (GH #300). @@ -137,7 +139,7 @@ async def browse( # registry means this link cannot drift from the entity links in the # next column, and it degrades to plain text if no module claims the # users table. - payload["actor_url"] = actor_link(links, item.user_id) if item.user_id else None + payload["actor_url"] = actor_link(links, directory, item.user_id) if item.user_id else None payload["entity"] = entity_link( links, item.entity_type, diff --git a/modules/audit_log/audit_log/export.py b/modules/audit_log/audit_log/export.py index e2d48b73..9835ee32 100644 --- a/modules/audit_log/audit_log/export.py +++ b/modules/audit_log/audit_log/export.py @@ -19,6 +19,7 @@ from typing import Any from simple_module_core.audit_links import AuditLinkRegistry +from simple_module_core.user_directory import UserDirectory from sqlalchemy.ext.asyncio import AsyncSession from audit_log.contracts.schemas import AuditEntryRead @@ -109,6 +110,7 @@ async def stream_csv( service: AuditLogService, db: AsyncSession, registry: AuditLinkRegistry, + directory: UserDirectory, filters: EntryFilters, permissions: Collection[str], ) -> AsyncIterator[str]: @@ -137,7 +139,7 @@ def flush() -> str: yield flush() async for batch in service.iter_entries(filters): - actors = await resolve_actors(db, [entry.user_id for entry in batch]) + actors = await resolve_actors(directory, db, [entry.user_id for entry in batch]) labels = await resolve_entity_labels( db, registry, diff --git a/modules/audit_log/audit_log/module.py b/modules/audit_log/audit_log/module.py index a557670b..f1576211 100644 --- a/modules/audit_log/audit_log/module.py +++ b/modules/audit_log/audit_log/module.py @@ -24,18 +24,23 @@ VIEW_PREFIX, ) -_MODULE_USERS = "Users" - class AuditLogModule(ModuleBase): meta = ModuleMeta( name=MODULE_NAME, route_prefix=API_PREFIX, view_prefix=VIEW_PREFIX, - depends_on=[_MODULE_USERS], i18n_audience="admin", ) + def register_settings(self, app: FastAPI) -> None: + """Offer the audit trail as the app's activity feed (GH #501).""" + from simple_module_core.activity_feed import set_activity_feed + + from audit_log.activity import AuditActivityFeed + + set_activity_feed(app, AuditActivityFeed(app)) + def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: from audit_log.endpoints.api import router as api from audit_log.endpoints.views import router as views diff --git a/modules/audit_log/audit_log/resolve.py b/modules/audit_log/audit_log/resolve.py index b17cc3ab..a6436bc7 100644 --- a/modules/audit_log/audit_log/resolve.py +++ b/modules/audit_log/audit_log/resolve.py @@ -6,7 +6,7 @@ This module resolves both at render time: -* ``resolve_actors`` batches one query for every user id on the page. +* ``resolve_actors`` asks the user directory once for every user id on the page. * ``resolve_entity_labels`` asks each owning module what its rows are *called*, one batched call per entity type on the page. * ``entity_link`` consults the host's :class:`AuditLinkRegistry`, which each @@ -25,29 +25,29 @@ from simple_module_core.audit_links import AuditLinkRegistry from simple_module_core.permissions import grants -from simple_module_db import LIKE_ESCAPE_CHAR, like_contains_pattern -from sqlalchemy import String, cast, or_, select +from simple_module_core.user_directory import UserDirectory, resolve_user_labels from sqlalchemy.ext.asyncio import AsyncSession -from users.audit import resolve_user_labels -from users.models import User from audit_log.models import AuditEntry logger = logging.getLogger(__name__) -async def resolve_actors(db: AsyncSession, user_ids: list[str | None]) -> dict[str, str]: +async def resolve_actors( + directory: UserDirectory, db: AsyncSession, user_ids: list[str | None] +) -> dict[str, str]: """Map user id -> display label for every id on the page. - One query for the whole page rather than one per row: an audit page is 50 + One lookup for the whole page rather than one per row: an audit page is 50 entries and the same handful of admins tend to appear on all of them. - Delegates the naming rule itself to ``users.audit`` — the actor column and - the entity column both point at user rows, and two copies of "full_name or - email" is two chances for the same row to be called two things on one - screen. Ids that no longer resolve (deleted accounts) or never named an - account at all are simply absent from the result — the caller falls back to - showing the raw id, which is still the truthful record of who acted. + Asks the app's user directory (GH #501) rather than any one module's + tables, so actors are named whichever auth provider holds the accounts, + and by ``DirectoryUser.label`` — the same rule the entity column uses, so + one row is never called two things on one screen. Ids that no longer + resolve (deleted accounts) or never named an account at all are simply + absent from the result — the caller falls back to showing the raw id, + which is still the truthful record of who acted. Deliberately **not** gated on ``users.manage`` the way the entity column is (``resolve_entity_labels``). The two columns disclose the same field and @@ -57,13 +57,10 @@ async def resolve_actors(db: AsyncSession, user_ids: list[str | None]) -> dict[s ``audit_log.view`` therefore does imply "may see who acted"; that is stated in docs/modules/audit_log.md so it is granted knowingly. See GH #300. """ - wanted = sorted({uid for uid in user_ids if uid}) - if not wanted: - return {} - return await resolve_user_labels(db, wanted) + return await resolve_user_labels(directory, db, [uid for uid in user_ids if uid]) -def actor_filter(term: str) -> Any | None: +def actor_filter(directory: UserDirectory, term: str) -> Any | None: """A predicate selecting the audit rows an Actor search should show. A uuid means that account and only that account — an id is unambiguous, and @@ -71,24 +68,22 @@ def actor_filter(term: str) -> Any | None: substring of a name or an email, which is what someone investigating an incident actually has to hand. - The name case is a **subquery**, not a list of ids resolved in Python - first. Materialising the matches means choosing a ceiling: without one, a - common substring on a large install builds an ``IN`` list with more bind - parameters than Postgres accepts; with one, the same search silently - returns an arbitrary slice of the matching accounts, on the page *and* in - the CSV, with nothing to say so. An audit log that quietly answers a - different question than the one asked is worse than one that is slow, so - the match stays in the database where it has no ceiling. + The name case is a **subquery** (the directory's ``match_ids``), not a list + of ids resolved in Python first. Materialising the matches means choosing a + ceiling: without one, a common substring on a large install builds an + ``IN`` list with more bind parameters than Postgres accepts; with one, the + same search silently returns an arbitrary slice of the matching accounts, + on the page *and* in the CSV, with nothing to say so. An audit log that + quietly answers a different question than the one asked is worse than one + that is slow, so the match stays in the database where it has no ceiling. + + A directory that cannot express the match in SQL (none installed) answers + ``None``, and the term is then matched as an exact actor id — a worker + name, say — rather than silently matching nothing. ``None`` means the box was empty. It never means "nobody is called that" — that answer is a subquery matching no rows, which is not the same thing. - ``user_id`` is text and ``User.id`` is a uuid, which is why the ids were - materialised in the first place; ``CAST`` bridges it. Both dialects render - the canonical lowercase, hyphenated form the audit trail stored (Postgres - from its native ``uuid``, SQLite from the ``CHAR(36)`` the GUID type - writes). - Typed ``Any`` because SQLModel declares columns with their plain Python types, so a comparison reads statically as ``bool`` while being a SQL expression at runtime — the same reason ``EntryFilters.conditions`` does. @@ -101,15 +96,10 @@ def actor_filter(term: str) -> Any | None: except (ValueError, AttributeError, TypeError): pass - pattern = like_contains_pattern(term) - return AuditEntry.user_id.in_( - select(cast(User.id, String)).where( - or_( - User.full_name.ilike(pattern, escape=LIKE_ESCAPE_CHAR), - User.email.ilike(pattern, escape=LIKE_ESCAPE_CHAR), - ) - ) - ) + matching = directory.match_ids(term) + if matching is None: + return AuditEntry.user_id == term + return AuditEntry.user_id.in_(matching) async def resolve_entity_labels( @@ -188,30 +178,26 @@ def table_name_for(entity_type: str) -> str: return _CAMEL_BOUNDARY.sub("_", entity_type).lower() -USER_ENTITY_TYPE = User.__name__ -"""The model an audit actor is a row of. - -The audit trail records ``type(obj).__name__``, so registry keys are class -names — not ``__tablename__``. Actors are user ids, so their link comes from -the same registry entry as any other reference to that model. - -Derived from the class rather than spelled ``"User"``: the users module keys -its own registration off ``User.__name__`` too, so a rename moves both ends -together instead of silently unlinking every actor cell.""" - - -def actor_link(registry: AuditLinkRegistry, user_id: str) -> str | None: +def actor_link(registry: AuditLinkRegistry, directory: UserDirectory, user_id: str) -> str | None: """URL for the acting user's record, or ``None`` when nothing claims it. + Actors are user ids, so their link comes from the same registry entry as + any other reference to the directory's user model — its ``entity_type`` + (``"User"`` under the users module). The audit trail records + ``type(obj).__name__``, so registry keys are class names, never + ``__tablename__``. + Deliberately routed through the registry rather than hardcoding the users module's route: the users module already declares where its rows live, and duplicating that here would 404 the moment it moves its prefix, or silently disagree with the entity link rendered in the next column. - ``None`` when nothing claims the users table — this module depends on - ``users`` for its models, but not on ``users`` having registered a link. + ``None`` when the directory's users have no audited model (Keycloak, or no + provider at all) or nothing registered a link for it. """ - link = registry.get(USER_ENTITY_TYPE) + if directory.entity_type is None: + return None + link = registry.get(directory.entity_type) return link.url_for(user_id) if link else None diff --git a/modules/audit_log/tests/test_actor_filter_by_name.py b/modules/audit_log/tests/test_actor_filter_by_name.py index 993415e7..2d831aa9 100644 --- a/modules/audit_log/tests/test_actor_filter_by_name.py +++ b/modules/audit_log/tests/test_actor_filter_by_name.py @@ -154,12 +154,24 @@ async def test_the_filter_is_a_subquery_not_a_list_of_ids(self) -> None: that materialises ids passes the count test right up until the install is big enough to break it.""" from audit_log.resolve import actor_filter + from users.directory import UsersDirectory - rendered = str(actor_filter("okafor")) + rendered = str(actor_filter(UsersDirectory(), "okafor")) assert "SELECT" in rendered.upper() assert "users_user" in rendered + async def test_without_a_directory_the_term_is_an_exact_actor_id(self) -> None: + """No directory can say who is called that, so the box falls back to + an id match (a worker name) instead of silently matching nobody.""" + from audit_log.resolve import actor_filter + from simple_module_core.user_directory import NullUserDirectory + + rendered = str(actor_filter(NullUserDirectory(), "celery-worker-1")) + + assert "SELECT" not in rendered.upper() + assert "user_id" in rendered + async def test_every_matching_actor_is_returned( self, app, authenticated_client: httpx.AsyncClient ) -> None: diff --git a/modules/audit_log/tests/test_audit_log_activity_feed.py b/modules/audit_log/tests/test_audit_log_activity_feed.py new file mode 100644 index 00000000..108f0a1e --- /dev/null +++ b/modules/audit_log/tests/test_audit_log_activity_feed.py @@ -0,0 +1,60 @@ +"""The audit log as the core activity feed the users edit page reads (GH #501).""" + +from __future__ import annotations + +import uuid +from types import SimpleNamespace + +from audit_log.activity import AuditActivityFeed +from audit_log.models import AuditEntry +from simple_module_core.activity_feed import get_activity_feed +from simple_module_core.audit_links import AuditLink, AuditLinkRegistry +from simple_module_core.permissions import WILDCARD + +_GATE = "secrets.view" + + +async def test_audit_log_installs_the_feed(app): + assert isinstance(get_activity_feed(app), AuditActivityFeed) + + +async def test_recent_entries_by_one_actor_with_gated_labels(app): + actor, other = str(uuid.uuid4()), str(uuid.uuid4()) + registry = AuditLinkRegistry() + + async def _names(db, ids): + return {i: f"name-{i}" for i in ids} + + registry.register( + AuditLink( + entity_type="Secret", url_template="", label_resolver=_names, label_permission=_GATE + ) + ) + registry.register(AuditLink(entity_type="Note", url_template="", label_resolver=_names)) + app_like = SimpleNamespace(state=SimpleNamespace(sm=SimpleNamespace(audit_links=registry))) + + async with app.state.sm.db.session_factory() as db: + for n, (etype, who) in enumerate([("Note", actor), ("Secret", actor), ("Note", other)]): + db.add( + AuditEntry( + entity_type=etype, + entity_id=f"e{n}", + action="updated", + changes=[{"field": "body"}], + user_id=who, + tenant_id="default", + ) + ) + await db.flush() + + feed = AuditActivityFeed(app_like) + items = await feed.recent_for_actor(db, actor, limit=10, permissions=[]) + assert sorted((i.entity_type, i.entity_label) for i in items) == [ + ("Note", "name-e0"), + # Gated label withheld from a reader without the permission (GH #300). + ("Secret", None), + ] + assert all(i.changes == [{"field": "body"}] for i in items) + + admin_view = await feed.recent_for_actor(db, actor, limit=1, permissions=[WILDCARD]) + assert len(admin_view) == 1 diff --git a/modules/audit_log/tests/test_audit_log_resolve.py b/modules/audit_log/tests/test_audit_log_resolve.py index 55c0cda6..f0125d08 100644 --- a/modules/audit_log/tests/test_audit_log_resolve.py +++ b/modules/audit_log/tests/test_audit_log_resolve.py @@ -10,9 +10,13 @@ from audit_log.resolve import actor_link, entity_link, resolve_actors from simple_module_core.audit_links import AuditLink, AuditLinkRegistry +from simple_module_core.user_directory import NullUserDirectory from sqlalchemy.ext.asyncio import AsyncSession +from users.directory import UsersDirectory from users.models import User +_DIRECTORY = UsersDirectory() + def _registry() -> AuditLinkRegistry: reg = AuditLinkRegistry() @@ -97,18 +101,21 @@ class TestActorLink: def test_uses_the_registered_users_route(self): """Hardcoding /users/admin/{id} here would 404 the moment the users module moved its prefix, and duplicate what the registry owns.""" - assert actor_link(_registry(), "a91") == "/admin/users/a91" + assert actor_link(_registry(), _DIRECTORY, "a91") == "/admin/users/a91" def test_none_when_the_users_table_is_unclaimed(self): """A registry without a User entry yields no link rather than a guessed one. - Not a claim that audit_log runs without the users module — it imports - `users.models` at module scope and declares the dependency. This covers - a users module that ships no audit link, and any registry built before - registration has run. + Covers a users module that ships no audit link, and any registry built + before registration has run. """ - assert actor_link(AuditLinkRegistry(), "a91") is None + assert actor_link(AuditLinkRegistry(), _DIRECTORY, "a91") is None + + def test_none_when_the_directory_has_no_user_model(self): + """Keycloak (or no provider): no audited user rows, so no link — even + when some registry entry happens to be keyed ``User``.""" + assert actor_link(_registry(), NullUserDirectory(), "a91") is None class TestResolveActors: @@ -122,7 +129,7 @@ async def test_resolves_full_name_when_present(self, db_session: AsyncSession): db_session.add(user) await db_session.flush() - resolved = await resolve_actors(db_session, [str(user.id)]) + resolved = await resolve_actors(_DIRECTORY, db_session, [str(user.id)]) assert resolved[str(user.id)] == "Sam Carter" async def test_falls_back_to_email_without_a_name(self, db_session: AsyncSession): @@ -130,21 +137,21 @@ async def test_falls_back_to_email_without_a_name(self, db_session: AsyncSession db_session.add(user) await db_session.flush() - resolved = await resolve_actors(db_session, [str(user.id)]) + resolved = await resolve_actors(_DIRECTORY, db_session, [str(user.id)]) assert resolved[str(user.id)] == "noname@example.com" async def test_unknown_ids_are_simply_absent(self, db_session: AsyncSession): """A deleted account must not blank the row — the caller shows the id.""" missing = str(uuid.uuid4()) - assert await resolve_actors(db_session, [missing]) == {} + assert await resolve_actors(_DIRECTORY, db_session, [missing]) == {} async def test_empty_and_none_ids_short_circuit(self, db_session: AsyncSession): - assert await resolve_actors(db_session, [None, None]) == {} - assert await resolve_actors(db_session, []) == {} + assert await resolve_actors(_DIRECTORY, db_session, [None, None]) == {} + assert await resolve_actors(_DIRECTORY, db_session, []) == {} async def test_unparseable_ids_do_not_fail_the_page(self, db_session: AsyncSession): """System actors from another id space must not 500 the audit log.""" - assert await resolve_actors(db_session, ["celery-worker-1"]) == {} + assert await resolve_actors(_DIRECTORY, db_session, ["celery-worker-1"]) == {} async def test_batches_the_whole_page_in_one_pass(self, db_session: AsyncSession): users = [ @@ -156,5 +163,5 @@ async def test_batches_the_whole_page_in_one_pass(self, db_session: AsyncSession ids = [str(u.id) for u in users] # Repeats are normal: one admin usually authors most of a page. - resolved = await resolve_actors(db_session, ids + ids) + resolved = await resolve_actors(_DIRECTORY, db_session, ids + ids) assert set(resolved) == set(ids) diff --git a/modules/users/tests/test_edit_recent_activity.py b/modules/users/tests/test_edit_recent_activity.py index 59dfc7ff..4fc37342 100644 --- a/modules/users/tests/test_edit_recent_activity.py +++ b/modules/users/tests/test_edit_recent_activity.py @@ -147,17 +147,19 @@ async def test_a_row_nothing_can_name_falls_back_to_a_short_id(self, admin_clien @pytest.mark.anyio async def test_prop_is_none_when_audit_log_is_not_installed(self, admin_client, users_app): - """Absent module, absent card — not an empty one claiming no activity.""" - sm = users_app.state.sm - loaded = sm.modules - trimmed = type(loaded)(m for m in loaded if getattr(m.meta, "name", "") != "AuditLog") - assert len(trimmed) < len(loaded), "audit_log was not loaded, so this proves nothing" - # ``sm`` is a frozen dataclass, so this is the only way to model an app - # built without the module short of building a second one. - object.__setattr__(sm, "modules", trimmed) + """No activity feed, absent card — not an empty one claiming no activity. + + The card reads the core activity feed that ``audit_log`` installs + (GH #501); an app built without the module has none. + """ + from simple_module_core.activity_feed import get_activity_feed, set_activity_feed + + feed = get_activity_feed(users_app) + assert feed is not None, "audit_log installed no feed, so this proves nothing" + set_activity_feed(users_app, None) try: user_id = await _admin_id(users_app) props = await _edit_props(admin_client, user_id) assert props["recent_activity"] is None finally: - object.__setattr__(sm, "modules", loaded) + set_activity_feed(users_app, feed) diff --git a/modules/users/users/admin/recent_activity.py b/modules/users/users/admin/recent_activity.py index c853f010..97bee770 100644 --- a/modules/users/users/admin/recent_activity.py +++ b/modules/users/users/admin/recent_activity.py @@ -1,12 +1,12 @@ """The last few things this person did, for the edit page's activity card. The audit log already records every write with an actor, so the card is a read -rather than new bookkeeping. Users must not *depend* on ``audit_log`` though — -it is an optional module, and a hard import would make the users screens fail -to load on an install that never enabled it. Hence the lazy import inside -``try/except ImportError`` and the ``None`` return, which the page renders as -"no card" rather than "no activity": an install that records nothing and a -person who did nothing are different claims. +rather than new bookkeeping. Users must not *import* ``audit_log`` though — it +is an optional module, and the two importing each other was a cycle (GH #501). +So the card reads the core activity feed, which ``audit_log`` installs. No feed +means a ``None`` return, which the page renders as "no card" rather than "no +activity": an install that records nothing and a person who did nothing are +different claims. Every line is a *translated* sentence, not an assembled one. The rows are rendered server-side, so this reaches for the request's ``Translator`` the way @@ -19,10 +19,10 @@ import logging import uuid -from collections.abc import Collection from typing import Any from fastapi import Request +from simple_module_core.activity_feed import get_activity_feed from simple_module_core.i18n import Translator from simple_module_hosting.permissions import resolved_permissions_for from sqlalchemy.ext.asyncio import AsyncSession @@ -132,61 +132,35 @@ def _kind_of(t: Translator, registry: Any, entity_type: str) -> str: return label.lower() -async def _labels_for( - db: AsyncSession, registry: Any, entries: list[Any], permissions: Collection[str] -) -> dict[tuple[str, str], str]: - """Display labels for every row an entry page refers to, by entity type. - - Delegates to the audit-links registry so each module names its own rows — - a ``Setting`` by its key, a ``StoredFile`` by its filename — batched to one - query per entity type rather than one per row. Types whose owner registered - no resolver are simply absent, and the caller shows a short id. - - *permissions* carries the reader's grants through to the resolvers, which - skip entity types their owner gated (GH #300). This card lives behind - ``users.manage`` so in practice nothing is withheld here — passing the real - set rather than assuming that keeps it true if the page's own guard ever - loosens. - """ - from audit_log.resolve import resolve_entity_labels - - refs = [(entry.entity_type, entry.entity_id) for entry in entries] - if not refs: - return {} - return await resolve_entity_labels(db, registry, refs, permissions) - - async def recent_activity_for( request: Request, user_id: uuid.UUID, db: AsyncSession, t: Translator, ) -> list[dict[str, str]] | None: - """Recent audit entries where *user_id* was the actor, or ``None``. + """Recent activity where *user_id* was the actor, or ``None``. Filtered by actor rather than by subject because the deck's card reads as a record of what this person has been doing, which is what an admin reviewing an account wants to know. - """ - try: - from audit_log.constants import MODULE_NAME - from audit_log.service import AuditLogService - except ImportError: - return None - # Installed as a package but not loaded into this app is a real state - # (a trimmed preset, a disabled module), and its table may not exist. - # - # Compared against the module's own constant rather than a literal: the - # registered name is ``"AuditLog"`` while the menu label is ``"Audit log"``, - # and hardcoding the readable one silently disabled this card entirely. - if not any(getattr(m.meta, "name", "") == MODULE_NAME for m in request.app.state.sm.modules): + The reader's grants travel to the feed, which withholds the names of rows + whose owner gated them (GH #300). This card lives behind ``users.manage`` + so in practice nothing is withheld here — passing the real set rather than + assuming that keeps it true if the page's own guard ever loosens. + """ + feed = get_activity_feed(request.app) + if feed is None: return None registry = request.app.state.sm.audit_links try: - page = await AuditLogService(db).list_entries(user_id=str(user_id), page_size=RECENT_LIMIT) - labels = await _labels_for(db, registry, page.items, resolved_permissions_for(request)) + items = await feed.recent_for_actor( + db, + str(user_id), + limit=RECENT_LIMIT, + permissions=resolved_permissions_for(request), + ) except Exception: # A card that cannot load is not a reason to 500 the whole edit page. logger.exception("Could not read recent activity for %s", user_id) @@ -194,20 +168,15 @@ async def recent_activity_for( return [ { - "at": entry.created_at.isoformat(), + "at": item.at.isoformat(), "summary": _summarise( t, - entry.action, - _kind_of(t, registry, entry.entity_type), - labels.get( - (entry.entity_type, entry.entity_id), - entry.entity_id[:_SHORT_ID_CHARS], - ), - entry.changes, - ), - "href": ( - f"{AUDIT_LOG_URL}?entity_type={entry.entity_type}&entity_id={entry.entity_id}" + item.action, + _kind_of(t, registry, item.entity_type), + item.entity_label or item.entity_id[:_SHORT_ID_CHARS], + item.changes, ), + "href": (f"{AUDIT_LOG_URL}?entity_type={item.entity_type}&entity_id={item.entity_id}"), } - for entry in page.items + for item in items ] From 0983481b313bafcf9bbaf2108ca03f9a2c463ae2 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 14:20:59 +0200 Subject: [PATCH 4/9] refactor: port dashboard, file_storage, api_tokens, permissions, auth onto the user directory (#501) - dashboard: user tiles from UserDirectory.stats(); a None figure hides its tile; Users dependency (meta and package) dropped. - file_storage: uploader names via the directory; the copied users query is gone. - api_tokens: owner checks and emails via get_many; PAT resolution via get_many(with_roles=True), refusing owners whose roles are unknown. Keeps depends_on Users. - permissions: user side via get_many/search; role pages keep Users through users.contracts.roles; admin sync uses core ADMIN_ROLE. - auth: a structural AccountLike protocol replaces the users.User hint. - background_tasks: drop the unused Users dependency. - smpy catalog: requires now mirrors depends_on (users needs settings, background_tasks needs settings, dashboard needs nothing), pinned by a test. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/cli/simple_module_cli/catalog.py | 14 ++-- framework/cli/tests/test_cli_catalog.py | 8 +- .../cli/tests/test_cli_catalog_depends_on.py | 49 +++++++++++ framework/cli/tests/test_cli_wizard.py | 10 +-- modules/api_tokens/api_tokens/deps.py | 7 +- modules/api_tokens/api_tokens/resolver.py | 39 +++++---- modules/api_tokens/api_tokens/service.py | 29 +++---- .../tests/test_api_tokens_directory.py | 80 ++++++++++++++++++ .../tests/test_api_tokens_hardening.py | 5 +- modules/audit_log/pyproject.toml | 5 -- modules/auth/auth/contracts/schemas.py | 25 ++++-- .../background_tasks/constants.py | 1 - .../background_tasks/module.py | 3 +- .../tests/test_bg_users_ordering.py | 30 +++++++ modules/dashboard/dashboard/module.py | 2 - modules/dashboard/dashboard/pages/Home.tsx | 47 +++++++---- modules/dashboard/dashboard/stats.py | 48 +++-------- modules/dashboard/pyproject.toml | 2 - modules/dashboard/tests/test_dashboard.py | 4 +- .../tests/test_dashboard_user_directory.py | 69 ++++++++++++++++ .../file_storage/endpoints/views.py | 8 +- modules/file_storage/file_storage/resolve.py | 62 ++------------ .../test_file_storage_uploader_directory.py | 25 ++++++ modules/permissions/permissions/deps.py | 4 +- modules/permissions/permissions/service.py | 82 +++++++++---------- .../tests/test_permissions_directory.py | 66 +++++++++++++++ .../tests/test_permissions_grant_search.py | 3 +- .../test_permissions_inheritance_sources.py | 3 +- .../tests/test_permissions_module.py | 27 +++--- uv.lock | 4 - 30 files changed, 519 insertions(+), 242 deletions(-) create mode 100644 framework/cli/tests/test_cli_catalog_depends_on.py create mode 100644 modules/api_tokens/tests/test_api_tokens_directory.py create mode 100644 modules/background_tasks/tests/test_bg_users_ordering.py create mode 100644 modules/dashboard/tests/test_dashboard_user_directory.py create mode 100644 modules/file_storage/tests/test_file_storage_uploader_directory.py create mode 100644 modules/permissions/tests/test_permissions_directory.py diff --git a/framework/cli/simple_module_cli/catalog.py b/framework/cli/simple_module_cli/catalog.py index dfaf0660..721a9103 100644 --- a/framework/cli/simple_module_cli/catalog.py +++ b/framework/cli/simple_module_cli/catalog.py @@ -29,19 +29,17 @@ class ModuleEntry: CATALOG: dict[str, ModuleEntry] = { "auth": ModuleEntry("auth", "simple_module_auth", "Auth"), - "users": ModuleEntry("users", "simple_module_users", "Users", requires=("auth",)), + # ``requires`` mirrors each module's ``depends_on``: outside development a + # missing dependency refuses to boot (GH #501), so a scaffold that left + # one out would build an app that cannot start in production. + "users": ModuleEntry("users", "simple_module_users", "Users", requires=("auth", "settings")), "permissions": ModuleEntry( "permissions", "simple_module_permissions", "Permissions", requires=("auth", "users"), ), - "dashboard": ModuleEntry( - "dashboard", - "simple_module_dashboard", - "Dashboard", - requires=("users",), - ), + "dashboard": ModuleEntry("dashboard", "simple_module_dashboard", "Dashboard"), "settings": ModuleEntry("settings", "simple_module_settings", "Settings"), "feature_flags": ModuleEntry("feature_flags", "simple_module_feature_flags", "Feature Flags"), "file_storage": ModuleEntry( @@ -54,7 +52,7 @@ class ModuleEntry: "background_tasks", "simple_module_background_tasks", "Background Tasks", - requires=("users",), + requires=("settings",), recipe="background_tasks", ), "site_lock": ModuleEntry( diff --git a/framework/cli/tests/test_cli_catalog.py b/framework/cli/tests/test_cli_catalog.py index f3eb528f..b9efe4ac 100644 --- a/framework/cli/tests/test_cli_catalog.py +++ b/framework/cli/tests/test_cli_catalog.py @@ -36,15 +36,15 @@ def test_expand_deps_returns_input_when_no_requires() -> None: def test_expand_deps_pulls_in_transitive_dep() -> None: resolved, added = expand_deps(["users"]) - assert set(resolved) == {"auth", "users"} - assert added == [("auth", "users")] + assert set(resolved) == {"auth", "settings", "users"} + assert added == [("auth", "users"), ("settings", "users")] def test_expand_deps_pulls_in_chain() -> None: resolved, added = expand_deps(["permissions"]) - assert set(resolved) == {"permissions", "users", "auth"} + assert set(resolved) == {"permissions", "users", "auth", "settings"} added_names = {a for a, _ in added} - assert added_names == {"users", "auth"} + assert added_names == {"users", "auth", "settings"} def test_expand_deps_idempotent_when_input_already_complete() -> None: diff --git a/framework/cli/tests/test_cli_catalog_depends_on.py b/framework/cli/tests/test_cli_catalog_depends_on.py new file mode 100644 index 00000000..71af5355 --- /dev/null +++ b/framework/cli/tests/test_cli_catalog_depends_on.py @@ -0,0 +1,49 @@ +"""The catalog's ``requires`` must cover every module's ``depends_on``. + +Outside development a missing ``depends_on`` target refuses to boot (GH #501), +so a catalog entry that under-declares builds an app — ``smpy new`` with a +preset, or the wizard — that starts in development and fails in production. +""" + +from __future__ import annotations + +from importlib.metadata import entry_points + +import pytest +from simple_module_cli.catalog import CATALOG + + +def _module_classes_by_package() -> dict[str, type]: + found: dict[str, type] = {} + for ep in entry_points(group="simple_module"): + if ep.dist is not None: + found[ep.dist.name.replace("-", "_")] = ep.load() + return found + + +def _closure(key: str) -> set[str]: + seen: set[str] = set() + stack = [key] + while stack: + for dep in CATALOG[stack.pop()].requires: + if dep not in seen: + seen.add(dep) + stack.append(dep) + return seen + + +def test_requires_covers_depends_on() -> None: + classes = _module_classes_by_package() + missing_pkgs = [e.package for e in CATALOG.values() if e.package not in classes] + if missing_pkgs: + pytest.skip(f"catalog packages not installed: {missing_pkgs}") + key_by_meta = {classes[e.package].meta.name: key for key, e in CATALOG.items()} + + gaps = [] + for key, entry in CATALOG.items(): + required = _closure(key) + for dep in classes[entry.package].meta.depends_on: + dep_key = key_by_meta.get(dep) + if dep_key is None or dep_key not in required: + gaps.append(f"{key} depends_on {dep!r} but the catalog does not require it") + assert not gaps, gaps diff --git a/framework/cli/tests/test_cli_wizard.py b/framework/cli/tests/test_cli_wizard.py index d4c4bc48..da7558ed 100644 --- a/framework/cli/tests/test_cli_wizard.py +++ b/framework/cli/tests/test_cli_wizard.py @@ -43,7 +43,7 @@ def test_wizard_postgres_with_tenancy() -> None: def test_wizard_minimal_preset() -> None: _, _, selected, _ = _drive(["", "", "1", "", ""]) - assert set(selected) == {"users", "auth"} + assert set(selected) == {"users", "auth", "settings"} def test_wizard_full_preset_includes_background_tasks() -> None: @@ -57,12 +57,12 @@ def test_wizard_custom_picks_only_yes_answers() -> None: # Derive the answers from CATALOG rather than hardcoding a count: a # hardcoded sequence silently shifts onto the wrong module the next time # one is added, which is how this test broke when `site_lock` landed. - picks = ["y" if name == "background_tasks" else "n" for name in CATALOG] + picks = ["y" if name == "permissions" else "n" for name in CATALOG] answers = ["", "", "4", *picks, "", ""] _, _, selected, out = _drive(answers) - assert set(selected) == {"background_tasks", "users", "auth"} - assert "Added users (required by background_tasks)" in out - assert "Added auth (required by users)" in out + assert set(selected) == {"permissions", "users", "auth", "settings"} + assert "Added users (required by permissions)" in out + assert "Added settings (required by users)" in out def test_wizard_aborts_on_confirm_no() -> None: diff --git a/modules/api_tokens/api_tokens/deps.py b/modules/api_tokens/api_tokens/deps.py index e2669d1e..b935a12b 100644 --- a/modules/api_tokens/api_tokens/deps.py +++ b/modules/api_tokens/api_tokens/deps.py @@ -8,6 +8,7 @@ from auth.contracts.schemas import UserContext from auth.contracts.tokens import AUTH_KIND_SESSION from fastapi import Depends, HTTPException, Request +from simple_module_core.user_directory import get_user_directory from simple_module_db.deps import get_db from simple_module_hosting.i18n_deps import TranslatorDep from sqlalchemy.ext.asyncio import AsyncSession @@ -16,8 +17,10 @@ from api_tokens.settings import ApiTokensSettings -async def get_api_token_service(db: AsyncSession = Depends(get_db)) -> ApiTokenService: - return ApiTokenService(db) +async def get_api_token_service( + request: Request, db: AsyncSession = Depends(get_db) +) -> ApiTokenService: + return ApiTokenService(db, get_user_directory(request.app)) def get_settings(request: Request) -> ApiTokensSettings: diff --git a/modules/api_tokens/api_tokens/resolver.py b/modules/api_tokens/api_tokens/resolver.py index fbeb69fb..9741cc54 100644 --- a/modules/api_tokens/api_tokens/resolver.py +++ b/modules/api_tokens/api_tokens/resolver.py @@ -5,10 +5,11 @@ hands the request to this resolver. Per request: a regex check, one indexed lookup by hash (no cache, so a revoke -is effective on the very next request), and for a personal token one primary -key read of the owner with their roles. Everything that is not a usable token -— unknown, revoked, expired, an inactive or disabled owner — is ``None``, -which the middleware turns into a 401. +is effective on the very next request), and for a personal token one read of +the owner with their roles through the app's user directory. Everything that +is not a usable token — unknown, revoked, expired, an inactive or disabled +owner, an owner whose roles the directory cannot read — is ``None``, which the +middleware turns into a 401. """ from __future__ import annotations @@ -20,9 +21,9 @@ from auth.contracts.schemas import UserContext from auth.contracts.tokens import AUTH_KIND_PAT, AUTH_KIND_SERVICE, is_api_token, parse_bearer from simple_module_core.tenancy import is_tenant_role +from simple_module_core.user_directory import UserDirectory, get_user_directory from simple_module_db import ALL_TENANTS_OPTION from sqlalchemy import or_, select, update -from sqlalchemy.orm import noload, selectinload from starlette.requests import Request from api_tokens.constants import ( @@ -52,20 +53,24 @@ def _usable(token: ApiToken, now: datetime) -> bool: return token.revoked_at is None and token.expires_at > now -async def _personal(db: Any, token: ApiToken) -> UserContext | None: - from users.models import User - +async def _personal(db: Any, directory: UserDirectory, token: ApiToken) -> UserContext | None: if token.owner_user_id is None: return None - stmt = ( - select(User) - .where(User.id == token.owner_user_id) # ty: ignore[invalid-argument-type] - .options(selectinload(User.roles), noload(User.oauth_accounts)) # ty: ignore[invalid-argument-type] - ) - user = (await db.execute(stmt)).scalar_one_or_none() - if user is None or not user.is_active or user.disabled_at is not None: + owner_id = str(token.owner_user_id) + user = (await directory.get_many(db, [owner_id], with_roles=True)).get(owner_id) + # Unknown roles (an IdP that keeps them, like Keycloak) refuse the token + # rather than authenticate an owner with none: the roles are what the + # scopes are intersected with, and "none" is not what the owner holds. + if user is None or not user.is_active or user.roles is None: return None - principal = UserContext.from_user(user, auth_kind=AUTH_KIND_PAT, scopes=list(token.scopes)) + principal = UserContext( + id=owner_id, + email=user.email, + name=user.label, + roles=list(user.roles), + auth_kind=AUTH_KIND_PAT, + scopes=list(token.scopes), + ) # Pinned to the tenant it was created in, like a service key: the tenant # resolver acts for this tenant only (and only while the owner is still an # active member of it), never one named by a header, subdomain or session. @@ -141,7 +146,7 @@ async def resolve_api_token(request: Request) -> UserContext | None: if token is None or not _usable(token, now): return None if token.kind == TokenKind.PERSONAL: - principal = await _personal(db, token) + principal = await _personal(db, get_user_directory(app), token) elif token.kind == TokenKind.SERVICE: principal = _service(token) else: diff --git a/modules/api_tokens/api_tokens/service.py b/modules/api_tokens/api_tokens/service.py index 45a087a1..13f531eb 100644 --- a/modules/api_tokens/api_tokens/service.py +++ b/modules/api_tokens/api_tokens/service.py @@ -13,6 +13,7 @@ from collections.abc import Sequence from datetime import UTC, datetime, timedelta +from simple_module_core.user_directory import NullUserDirectory, UserDirectory from simple_module_db import ALL_TENANTS_OPTION, all_tenants from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession @@ -37,8 +38,11 @@ def _utcnow() -> datetime: class ApiTokenService: - def __init__(self, db: AsyncSession) -> None: + def __init__(self, db: AsyncSession, directory: UserDirectory | None = None) -> None: self.db = db + # Owners are read through the app's user directory (GH #501); without + # one, nobody has a usable account and owners show unnamed. + self.directory: UserDirectory = directory or NullUserDirectory() async def create( self, @@ -81,17 +85,16 @@ async def create( return token, new.plaintext async def has_usable_account(self, user_id: uuid.UUID) -> bool: - """Whether *user_id* is an active ``users_user`` row the resolver can load. + """Whether *user_id* is an active account the resolver can turn into a principal. - Under an auth provider that keeps no local account (Keycloak without a - cached user), a token minted for the id would be dead on arrival. + That takes a directory entry that is active **and** whose roles are + known: under a provider that keeps roles in the IdP (Keycloak), a token + minted for the id would be dead on arrival, since the resolver refuses + a personal token whose owner's roles it cannot read. """ - from users.models import User - - stmt = select(User.id).where( - User.id == user_id, User.is_active.is_(True), User.disabled_at.is_(None) - ) - return (await self.db.execute(stmt)).first() is not None + found = await self.directory.get_many(self.db, [str(user_id)], with_roles=True) + owner = found.get(str(user_id)) + return owner is not None and owner.is_active and owner.roles is not None async def revoke_all_for_owner(self, owner_user_id: uuid.UUID) -> int: """Revoke every live personal token *owner_user_id* holds, in any tenant.""" @@ -157,13 +160,11 @@ async def revoke(self, token: ApiToken, *, cross_tenant: bool = False) -> ApiTok return token async def owner_emails(self, tokens: Sequence[ApiToken]) -> dict[uuid.UUID, str]: - from users.models import User - ids = {t.owner_user_id for t in tokens if t.owner_user_id is not None} if not ids: return {} - rows = (await self.db.execute(select(User.id, User.email).where(User.id.in_(ids)))).all() - return {row[0]: row[1] for row in rows} + found = await self.directory.get_many(self.db, [str(i) for i in ids]) + return {i: found[str(i)].email for i in ids if str(i) in found} @staticmethod def to_view(token: ApiToken, owner_email: str | None = None) -> TokenView: diff --git a/modules/api_tokens/tests/test_api_tokens_directory.py b/modules/api_tokens/tests/test_api_tokens_directory.py new file mode 100644 index 00000000..5c00f375 --- /dev/null +++ b/modules/api_tokens/tests/test_api_tokens_directory.py @@ -0,0 +1,80 @@ +"""Token owners are read through the user directory (GH #501).""" + +from __future__ import annotations + +import uuid +from datetime import UTC, datetime, timedelta + +from api_tokens.constants import PERM_USE, TokenKind +from api_tokens.models import ApiToken +from api_tokens.resolver import _personal +from api_tokens.service import ApiTokenService +from auth.contracts.tokens import AUTH_KIND_PAT +from simple_module_core.user_directory import DirectoryUser, NullUserDirectory + +_OWNER = uuid.uuid4() + + +class _Directory(NullUserDirectory): + def __init__(self, user: DirectoryUser | None) -> None: + self.user = user + self.asked_roles: list[bool] = [] + + async def get_many(self, db, ids, *, with_roles=False): + self.asked_roles.append(with_roles) + return {self.user.id: self.user} if self.user and self.user.id in ids else {} + + +def _token(**kw) -> ApiToken: + return ApiToken( + kind=TokenKind.PERSONAL, + name="t", + prefix="smpat_x", + token_hash="h", + owner_user_id=_OWNER, + scopes=["feature_flags.view"], + expires_at=datetime.now(UTC) + timedelta(days=1), + tenant_id="default", + **kw, + ) + + +def _owner(**kw) -> DirectoryUser: + return DirectoryUser(id=str(_OWNER), email="o@example.com", **kw) + + +class TestPersonalTokenResolution: + async def test_an_active_owner_with_known_roles_authenticates(self): + directory = _Directory(_owner(full_name="Owen", roles=("editor",))) + principal = await _personal(None, directory, _token()) + assert principal is not None + assert (principal.id, principal.name, principal.roles) == (str(_OWNER), "Owen", ["editor"]) + assert principal.auth_kind == AUTH_KIND_PAT + assert principal.scopes == ["feature_flags.view"] + assert principal.required_permission == PERM_USE + assert principal.tenant_id == "default" + assert directory.asked_roles == [True] + + async def test_unknown_roles_refuse_the_token(self): + """Keycloak keeps roles in the IdP: refuse rather than act with none.""" + assert await _personal(None, _Directory(_owner(roles=None)), _token()) is None + + async def test_an_inactive_or_missing_owner_refuses(self): + inactive = _Directory(_owner(roles=(), is_active=False)) + assert await _personal(None, inactive, _token()) is None + assert await _personal(None, _Directory(None), _token()) is None + + +class TestServiceOwners: + async def test_usable_account_needs_known_roles(self): + known = ApiTokenService(None, _Directory(_owner(roles=()))) + unknown = ApiTokenService(None, _Directory(_owner(roles=None))) + assert await known.has_usable_account(_OWNER) is True + assert await unknown.has_usable_account(_OWNER) is False + + async def test_owner_emails(self): + service = ApiTokenService(None, _Directory(_owner())) + other = _token() + other.owner_user_id = uuid.uuid4() + emails = await service.owner_emails([_token(), other]) + assert emails == {_OWNER: "o@example.com"} diff --git a/modules/api_tokens/tests/test_api_tokens_hardening.py b/modules/api_tokens/tests/test_api_tokens_hardening.py index b31654ec..a4095e01 100644 --- a/modules/api_tokens/tests/test_api_tokens_hardening.py +++ b/modules/api_tokens/tests/test_api_tokens_hardening.py @@ -186,12 +186,15 @@ async def missing(self, user_id) -> bool: async def test_has_usable_account(app): from api_tokens.service import ApiTokenService + from simple_module_core.user_directory import get_user_directory user_id = await create_user(app, f"u-{uuid.uuid4().hex[:6]}@example.com") async with app.state.sm.db.session_factory() as session: - service = ApiTokenService(session) + service = ApiTokenService(session, get_user_directory(app)) assert await service.has_usable_account(uuid.UUID(user_id)) is True assert await service.has_usable_account(uuid.uuid4()) is False + # Without a directory nobody has a usable account. + assert await ApiTokenService(session).has_usable_account(uuid.UUID(user_id)) is False async def test_revoke_all_ends_every_token(actor, bearer): diff --git a/modules/audit_log/pyproject.toml b/modules/audit_log/pyproject.toml index cd542cb1..ac708265 100644 --- a/modules/audit_log/pyproject.toml +++ b/modules/audit_log/pyproject.toml @@ -24,10 +24,6 @@ dependencies = [ "simple_module_core==0.0.37", "simple_module_db==0.0.37", "simple_module_hosting==0.0.37", - # ``audit_log.resolve`` imports ``users.models`` to turn a stored actor id - # into a name, so the users package is a hard install requirement — not - # just the runtime ``depends_on`` the module meta already declares. - "simple_module_users==0.0.37", ] [project.entry-points.simple_module] @@ -58,4 +54,3 @@ packages = ["audit_log"] simple_module_core = { workspace = true } simple_module_db = { workspace = true } simple_module_hosting = { workspace = true } -simple_module_users = { workspace = true } diff --git a/modules/auth/auth/contracts/schemas.py b/modules/auth/auth/contracts/schemas.py index 12415b44..840407a0 100644 --- a/modules/auth/auth/contracts/schemas.py +++ b/modules/auth/auth/contracts/schemas.py @@ -2,15 +2,28 @@ from __future__ import annotations +from collections.abc import Iterable from dataclasses import dataclass, field -from typing import TYPE_CHECKING, Any +from typing import Any, Protocol from auth.contracts.tokens import AUTH_KIND_SESSION -if TYPE_CHECKING: - # Runtime import would be circular: auth -> users -> auth. - # Only imported for type-hints, never at runtime. - from users.models import User + +class NamedRole(Protocol): + name: str + + +class AccountLike(Protocol): + """What :meth:`UserContext.from_user` reads from an account row. + + Structural, so ``auth`` names no provider's model: ``users`` depends on + ``auth``, never the other way round (GH #501). + """ + + id: Any + email: str + full_name: str | None + roles: Iterable[NamedRole] @dataclass @@ -53,7 +66,7 @@ def is_scoped(self) -> bool: @classmethod def from_user( cls, - user: User | Any, + user: AccountLike, *, auth_kind: str = AUTH_KIND_SESSION, scopes: list[str] | None = None, diff --git a/modules/background_tasks/background_tasks/constants.py b/modules/background_tasks/background_tasks/constants.py index 3de27853..e2d78bcc 100644 --- a/modules/background_tasks/background_tasks/constants.py +++ b/modules/background_tasks/background_tasks/constants.py @@ -18,7 +18,6 @@ TABLE_OUTBOX = f"{TABLE_PREFIX}outbox" # ── Module dependencies ───────────────────────────────────────── -_MODULE_USERS = "Users" # Sorted after Settings for its admin editor and reload path; registering # settings itself no longer needs it (the framework owns the registry, #500). _MODULE_SETTINGS = "Settings" diff --git a/modules/background_tasks/background_tasks/module.py b/modules/background_tasks/background_tasks/module.py index 088c83b1..3b0d6c5f 100644 --- a/modules/background_tasks/background_tasks/module.py +++ b/modules/background_tasks/background_tasks/module.py @@ -17,7 +17,6 @@ from background_tasks.constants import ( _MODULE_SETTINGS, - _MODULE_USERS, API_PREFIX, MENU_ICON, MENU_LABEL, @@ -74,7 +73,7 @@ class BackgroundTasksModule(ModuleBase): name=MODULE_DISPLAY_NAME, route_prefix=API_PREFIX, view_prefix=VIEW_PREFIX, - depends_on=[_MODULE_USERS, _MODULE_SETTINGS], + depends_on=[_MODULE_SETTINGS], i18n_audience="admin", ) diff --git a/modules/background_tasks/tests/test_bg_users_ordering.py b/modules/background_tasks/tests/test_bg_users_ordering.py new file mode 100644 index 00000000..d2f8d21a --- /dev/null +++ b/modules/background_tasks/tests/test_bg_users_ordering.py @@ -0,0 +1,30 @@ +"""``background_tasks`` no longer depends on ``users``, and order does not matter. + +The dependency was declared but never used (GH #501). The one thing this module +reads that ``users`` contributes is the invalidation channel list, which every +module registers in ``register_invalidations`` before any ``on_startup`` runs, +so sorting ``BackgroundTasks`` ahead of ``Users`` changes nothing. +""" + +from __future__ import annotations + +from background_tasks.module import BackgroundTasksModule +from simple_module_core.discovery import topological_sort +from users.module import UsersModule +from users.session_revocation import CHANNEL + + +def test_users_is_not_a_dependency(): + assert "Users" not in BackgroundTasksModule.meta.depends_on + + +def test_sorts_without_users_installed(): + ordered = topological_sort([BackgroundTasksModule()]) + assert [m.meta.name for m in ordered] == ["BackgroundTasks"] + + +async def test_users_channel_is_subscribed_when_background_tasks_sorts_first(app): + names = [m.meta.name for m in app.state.sm.modules] + assert names.index("BackgroundTasks") < names.index(UsersModule.meta.name) + # What the Redis invalidation transport fans out at on_startup. + assert CHANNEL in app.state.sm.invalidation.channels diff --git a/modules/dashboard/dashboard/module.py b/modules/dashboard/dashboard/module.py index 9b5366ef..8582f19a 100644 --- a/modules/dashboard/dashboard/module.py +++ b/modules/dashboard/dashboard/module.py @@ -12,7 +12,6 @@ from dashboard.constants import PERM_GROUP, PERM_VIEW -_MODULE_USERS = "Users" _URL_DASHBOARD = "/dashboard/" _URL_DOCTOR = "/admin/doctor/" _ICON_DASHBOARD = "home" @@ -26,7 +25,6 @@ class DashboardModule(ModuleBase): view_prefix="/dashboard", # The dashboard itself is an app screen; Doctor is an admin one. admin_view_prefix="/admin/doctor", - depends_on=[_MODULE_USERS], i18n_audience="admin", ) diff --git a/modules/dashboard/dashboard/pages/Home.tsx b/modules/dashboard/dashboard/pages/Home.tsx index 92e56bc3..7322af59 100644 --- a/modules/dashboard/dashboard/pages/Home.tsx +++ b/modules/dashboard/dashboard/pages/Home.tsx @@ -38,9 +38,12 @@ interface Props { welcome: string; /** False for a viewer without `dashboard.view`: the stats props are absent. */ can_view_stats: boolean; - total_users: number; - active_users_7d: number; - users_created_this_month: number; + /** User counts come from the auth provider's directory; `null` means it + * cannot say (no provider, or Keycloak for the monthly figure), and the + * tile is hidden rather than showing a zero that would be a guess. */ + total_users: number | null; + active_users_7d: number | null; + users_created_this_month: number | null; module_count: number; system_info: SystemInfo; } @@ -97,21 +100,29 @@ function Home() { description={t(keys.dashboard.home.description)} >
- - + {props.total_users !== null && ( + + )} + {props.active_users_7d !== null && ( + + )} dict: if hit is not None: return hit - total_users = await _count_users(db) - active_users_7d = await _count_active_users(db, days=7) - created_this_month = await _count_users_created_this_month(db) + # Counted by whichever auth provider holds the accounts (GH #501); a + # ``None`` is a figure it cannot give, and the page hides that tile. + user_stats = await get_user_directory(app).stats( + db, active_window=timedelta(days=_ACTIVE_WINDOW_DAYS) + ) health_checks = await _run_health_checks(app) modules_list = _get_module_info(app, health_checks) result = { - "total_users": total_users, - "active_users_7d": active_users_7d, - "users_created_this_month": created_this_month, + "total_users": user_stats.total, + "active_users_7d": user_stats.active_since, + "users_created_this_month": user_stats.created_this_month, "module_count": len(modules_list), "system_info": { "modules": modules_list, @@ -72,34 +74,6 @@ def invalidate_stats_cache() -> None: _cache_ts = 0.0 -async def _count_users(db: AsyncSession) -> int: - result = await db.execute(select(func.count()).select_from(User)) - return result.scalar_one() - - -async def _count_users_created_this_month(db: AsyncSession) -> int: - """Accounts created since the first of the current month, UTC. - - Calendar month rather than a rolling 30 days: the tile reads "+6 this - month", and a rolling window would make that sentence a lie every day of - the month except the first. - """ - now = datetime.now(UTC) - start = now.replace(day=1, hour=0, minute=0, second=0, microsecond=0) - result = await db.execute( - select(func.count()).select_from(User).where(User.created_at >= start) - ) - return result.scalar_one() - - -async def _count_active_users(db: AsyncSession, *, days: int) -> int: - cutoff = datetime.now(UTC) - timedelta(days=days) - result = await db.execute( - select(func.count()).select_from(User).where(User.last_login_at >= cutoff) - ) - return result.scalar_one() - - # Worst-first: a module's tile reports the worst state among its checks, so a # single unhealthy check is never hidden behind a healthy sibling. _HEALTH_SEVERITY = { diff --git a/modules/dashboard/pyproject.toml b/modules/dashboard/pyproject.toml index 272f255c..e0bc10f3 100644 --- a/modules/dashboard/pyproject.toml +++ b/modules/dashboard/pyproject.toml @@ -24,7 +24,6 @@ dependencies = [ "simple_module_core==0.0.37", "simple_module_db==0.0.37", "simple_module_hosting==0.0.37", - "simple_module_users==0.0.37", ] [project.entry-points.simple_module] @@ -52,4 +51,3 @@ packages = ["dashboard"] simple_module_core = { workspace = true } simple_module_db = { workspace = true } simple_module_hosting = { workspace = true } -simple_module_users = { workspace = true } diff --git a/modules/dashboard/tests/test_dashboard.py b/modules/dashboard/tests/test_dashboard.py index 040efd9a..93f6d272 100644 --- a/modules/dashboard/tests/test_dashboard.py +++ b/modules/dashboard/tests/test_dashboard.py @@ -24,7 +24,9 @@ async def test_module_meta(self): mod = DashboardModule() assert mod.meta.name == "Dashboard" assert mod.meta.route_prefix == "/api/dashboard" - assert "Users" in mod.meta.depends_on + # User counts come through the user directory (GH #501), so the + # dashboard runs under any auth provider. + assert "Users" not in mod.meta.depends_on # ── Stats function unit tests ──────────────────────────────────────── diff --git a/modules/dashboard/tests/test_dashboard_user_directory.py b/modules/dashboard/tests/test_dashboard_user_directory.py new file mode 100644 index 00000000..d485469e --- /dev/null +++ b/modules/dashboard/tests/test_dashboard_user_directory.py @@ -0,0 +1,69 @@ +"""Dashboard user tiles come from the user directory (GH #501).""" + +from __future__ import annotations + +from datetime import timedelta + +import pytest +from simple_module_core.user_directory import ( + DirectoryStats, + NullUserDirectory, + get_user_directory, + set_user_directory, +) + + +@pytest.fixture(autouse=True) +def _fresh_cache(): + from dashboard.stats import invalidate_stats_cache + + invalidate_stats_cache() + yield + invalidate_stats_cache() + + +class _KeycloakLike(NullUserDirectory): + """Knows how many signed in, not when the accounts were created.""" + + def __init__(self) -> None: + self.windows: list[timedelta] = [] + + async def stats(self, db, *, active_window): + self.windows.append(active_window) + return DirectoryStats(total=4, active_since=2, created_this_month=None) + + +async def _stats(app) -> dict: + from dashboard.stats import fetch_dashboard_stats + + async with app.state.sm.db.session_factory() as db: + return await fetch_dashboard_stats(db, app) + + +async def test_counts_come_from_the_directory(app): + original = get_user_directory(app) + directory = _KeycloakLike() + set_user_directory(app, directory) + try: + stats = await _stats(app) + finally: + set_user_directory(app, original) + assert stats["total_users"] == 4 + assert stats["active_users_7d"] == 2 + # Unknown, not zero: the page hides the "+N this month" delta. + assert stats["users_created_this_month"] is None + assert directory.windows == [timedelta(days=7)] + + +async def test_no_directory_hides_every_user_tile(app): + original = get_user_directory(app) + set_user_directory(app, NullUserDirectory()) + try: + stats = await _stats(app) + finally: + set_user_directory(app, original) + assert stats["total_users"] is None + assert stats["active_users_7d"] is None + assert stats["users_created_this_month"] is None + # The rest of the dashboard is unaffected. + assert stats["module_count"] >= 1 diff --git a/modules/file_storage/file_storage/endpoints/views.py b/modules/file_storage/file_storage/endpoints/views.py index 1b63a9ed..f938b5db 100644 --- a/modules/file_storage/file_storage/endpoints/views.py +++ b/modules/file_storage/file_storage/endpoints/views.py @@ -4,7 +4,8 @@ import math -from fastapi import APIRouter, Depends, Query +from fastapi import APIRouter, Depends, Query, Request +from simple_module_core.user_directory import get_user_directory from simple_module_db.deps import get_db from simple_module_hosting.inertia_deps import InertiaDep from simple_module_hosting.permissions import RequiresPermission @@ -29,6 +30,7 @@ dependencies=[Depends(RequiresPermission(constants.Permission.DOWNLOAD))], ) async def browse( + request: Request, inertia: InertiaDep, page: int = Query(default=1), q: str = "", @@ -71,7 +73,9 @@ async def browse( # One lookup covers the rows on screen and the filter's options, so # picking someone from the dropdown never renames them. labels = await resolve_uploader_labels( - db, [item.uploaded_by for item in items] + [u["id"] for u in uploaders] + get_user_directory(request.app), + db, + [item.uploaded_by for item in items] + [u["id"] for u in uploaders], ) settings = service.settings # The page name is hard-coded as a literal here (rather than via diff --git a/modules/file_storage/file_storage/resolve.py b/modules/file_storage/file_storage/resolve.py index 7a87a4fd..b3379ad6 100644 --- a/modules/file_storage/file_storage/resolve.py +++ b/modules/file_storage/file_storage/resolve.py @@ -4,68 +4,19 @@ permanent record — display names change, ids do not — and useless in a table: a column of uuids names nobody. -The users module is imported lazily and optionally. ``file_storage`` does not -depend on it: an install can run an external identity provider instead, in -which case the ids belong to another id space and the screen falls back to -showing what it stored rather than failing the page. Precedent: -``modules/audit_log/audit_log/resolve.py``. +Names come from the app's user directory (``simple_module_core.user_directory``), +whichever auth provider installed it; ``file_storage`` imports no user module. +With no directory, or ids from another id space, the screen falls back to +showing what it stored rather than failing the page (GH #501). """ from __future__ import annotations -import uuid -from collections.abc import Iterable - -from sqlalchemy import select -from sqlalchemy.ext.asyncio import AsyncSession +from simple_module_core.user_directory import resolve_user_labels as resolve_uploader_labels from file_storage import constants -async def resolve_uploader_labels( - db: AsyncSession, user_ids: Iterable[str | None] -) -> dict[str, str]: - """Map user id -> display label for every id on the page. - - One query for the whole page rather than one per row: a file listing is 20 - rows and the same handful of people tend to have uploaded all of them. - - Ids that no longer resolve — deleted accounts, or another provider's id - space — are simply absent from the result; the caller falls back to the raw - id, which is still the truthful record of who uploaded the file. - """ - wanted = {uid for uid in user_ids if uid} - if not wanted: - return {} - - try: - from users.models import User - except ImportError: - return {} - - # File rows store the id as text; the users table keys on UUID. Ids that - # aren't parseable belong to some other id space, so skip rather than fail. - parsed: dict[uuid.UUID, str] = {} - for raw in wanted: - try: - parsed[uuid.UUID(raw)] = raw - except (ValueError, AttributeError, TypeError): - continue - if not parsed: - return {} - - rows = ( - await db.execute( - select(User.id, User.email, User.full_name).where(User.id.in_(list(parsed))) - ) - ).all() - - return { - (parsed.get(user_id) or str(user_id)): (full_name or email) - for user_id, email, full_name in rows - } - - def label_for(user_id: str | None, labels: dict[str, str]) -> str: """Display label for one row's uploader. @@ -75,3 +26,6 @@ def label_for(user_id: str | None, labels: dict[str, str]) -> str: if not user_id: return constants.UNKNOWN_UPLOADER return labels.get(user_id, user_id) + + +__all__ = ["label_for", "resolve_uploader_labels"] diff --git a/modules/file_storage/tests/test_file_storage_uploader_directory.py b/modules/file_storage/tests/test_file_storage_uploader_directory.py new file mode 100644 index 00000000..e64b2e18 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_uploader_directory.py @@ -0,0 +1,25 @@ +"""Uploader names come from the user directory, not ``users.models`` (GH #501).""" + +from __future__ import annotations + +from file_storage.resolve import label_for, resolve_uploader_labels +from simple_module_core.user_directory import DirectoryUser, NullUserDirectory + + +class _Directory(NullUserDirectory): + async def get_many(self, db, ids, *, with_roles=False): + known = {"u1": DirectoryUser(id="u1", email="u1@example.com", full_name="Uma")} + return {i: known[i] for i in ids if i in known} + + +async def test_known_uploaders_are_named_and_others_keep_their_id(): + labels = await resolve_uploader_labels(_Directory(), None, ["u1", "gone", None]) + assert labels == {"u1": "Uma"} + assert label_for("u1", labels) == "Uma" + assert label_for("gone", labels) == "gone" + + +async def test_without_a_directory_the_stored_id_is_shown(): + labels = await resolve_uploader_labels(NullUserDirectory(), None, ["u1"]) + assert labels == {} + assert label_for("u1", labels) == "u1" diff --git a/modules/permissions/permissions/deps.py b/modules/permissions/permissions/deps.py index 5447e796..81c25f88 100644 --- a/modules/permissions/permissions/deps.py +++ b/modules/permissions/permissions/deps.py @@ -4,6 +4,7 @@ from fastapi import Depends, Request from simple_module_core.permissions import PermissionRegistry +from simple_module_core.user_directory import get_user_directory from simple_module_db.deps import get_db from simple_module_hosting.permissions import RequiresPermission as _HostingRequiresPermission from sqlalchemy.ext.asyncio import AsyncSession @@ -25,10 +26,11 @@ async def get_permission_registry(request: Request) -> PermissionRegistry: async def get_permission_service( + request: Request, db: AsyncSession = Depends(get_db), registry: PermissionRegistry = Depends(get_permission_registry), ) -> PermissionService: - return PermissionService(db, registry) + return PermissionService(db, registry, get_user_directory(request.app)) def assigned_by(request: Request) -> str | None: diff --git a/modules/permissions/permissions/service.py b/modules/permissions/permissions/service.py index d370d7a9..718750e7 100644 --- a/modules/permissions/permissions/service.py +++ b/modules/permissions/permissions/service.py @@ -5,11 +5,10 @@ import uuid from typing import TYPE_CHECKING -from simple_module_core.permissions import WILDCARD -from simple_module_db import LIKE_ESCAPE_CHAR, like_contains_pattern +from simple_module_core.permissions import ADMIN_ROLE, WILDCARD +from simple_module_core.user_directory import DirectoryUser, NullUserDirectory, UserDirectory from sqlalchemy import delete, func, select from sqlalchemy.ext.asyncio import AsyncSession -from sqlalchemy.orm import selectinload from permissions.contracts.schemas import ( PermissionGroupOut, @@ -22,7 +21,16 @@ if TYPE_CHECKING: from simple_module_core.permissions import PermissionRegistry - from users.models import Role, User + from users.contracts.roles import Role + + +def _user_out(user: DirectoryUser) -> UserOut: + return UserOut(id=uuid.UUID(user.id), email=user.email, full_name=user.full_name) + + +def _role_names(user: DirectoryUser) -> list[str]: + # ``None``: the provider keeps roles elsewhere, so nothing is inherited here. + return list(user.roles or ()) class PermissionService: @@ -33,12 +41,19 @@ class PermissionService: hook). The DB stores the mutable mapping of those keys onto: * roles — applied to every user holding that role - * users — granted directly on top of (or independent of) roles + * users — granted directly on top of (or independent of) roles, read + through the app's user directory (GH #501); roles are ``users``' table. """ - def __init__(self, db: AsyncSession, registry: PermissionRegistry) -> None: + def __init__( + self, + db: AsyncSession, + registry: PermissionRegistry, + directory: UserDirectory | None = None, + ) -> None: self.db = db self.registry = registry + self.directory: UserDirectory = directory or NullUserDirectory() # ── Registry read-outs ───────────────────────────────────── @@ -58,7 +73,7 @@ def _registered_keys(self) -> set[str]: # ── Role helpers ─────────────────────────────────────────── async def _load_roles(self) -> list[Role]: - from users.models import Role + from users.contracts.roles import Role result = await self.db.execute(select(Role).order_by(Role.name)) return list(result.scalars().all()) @@ -72,7 +87,7 @@ async def list_roles_with_counts(self) -> list[tuple[RoleOut, int]]: return [(RoleOut.model_validate(r), counts.get(r.name, 0)) for r in roles] async def get_role(self, role_id: uuid.UUID) -> RoleOut | None: - from users.models import Role + from users.contracts.roles import Role role = await self.db.get(Role, role_id) return RoleOut.model_validate(role) if role is not None else None @@ -125,13 +140,9 @@ async def set_role_permissions( # ── User helpers ─────────────────────────────────────────── - async def _get_user(self, user_id: uuid.UUID) -> User | None: - from users.models import User - - result = await self.db.execute( - select(User).where(User.id == user_id).options(selectinload(User.roles)) # ty: ignore[invalid-argument-type] - ) - return result.scalar_one_or_none() + async def _get_user(self, user_id: uuid.UUID) -> DirectoryUser | None: + found = await self.directory.get_many(self.db, [str(user_id)], with_roles=True) + return found.get(str(user_id)) async def get_user_direct_keys(self, user_id: uuid.UUID) -> list[str]: """Keys granted directly to *user_id* (roles excluded).""" @@ -144,16 +155,7 @@ async def list_users_with_counts( self, *, limit: int = 50, search: str | None = None ) -> list[tuple[UserOut, int]]: """Users who have at least one direct grant, plus an optional search.""" - from users.models import User - - stmt = select(User).limit(limit) - if search: - p, esc = like_contains_pattern(search), LIKE_ESCAPE_CHAR - stmt = stmt.where(User.email.ilike(p, esc) | User.full_name.ilike(p, esc)) - stmt = stmt.order_by(User.email) - - users_result = await self.db.execute(stmt) - users = list(users_result.scalars().all()) + users = [_user_out(u) for u in await self.directory.search(self.db, search, limit=limit)] if not users: return [] @@ -164,17 +166,17 @@ async def list_users_with_counts( .group_by(UserPermission.user_id) ) counts = dict(counts_result.all()) - return [(UserOut.model_validate(u), counts.get(u.id, 0)) for u in users] + return [(u, counts.get(u.id, 0)) for u in users] async def get_user_permissions(self, user_id: uuid.UUID) -> UserPermissionsOut | None: user = await self._get_user(user_id) if user is None: return None - direct = set(await self.get_user_direct_keys(user.id)) - role_names = [r.name for r in user.roles] + direct = set(await self.get_user_direct_keys(user_id)) + role_names = _role_names(user) inherited = self._resolve_role_permissions(role_names) - direct return UserPermissionsOut( - user=UserOut.model_validate(user), + user=_user_out(user), roles=sorted(role_names), direct=sorted(direct), inherited=sorted(inherited), @@ -193,26 +195,26 @@ async def set_user_permissions( registered = self._registered_keys() wanted = {k for k in keys if k in registered} - existing = set(await self.get_user_direct_keys(user.id)) + existing = set(await self.get_user_direct_keys(user_id)) to_remove = (existing - wanted) & registered if to_remove: await self.db.execute( delete(UserPermission).where( - UserPermission.user_id == user.id, # ty: ignore[invalid-argument-type] + UserPermission.user_id == user_id, # ty: ignore[invalid-argument-type] UserPermission.permission_key.in_(to_remove), ) ) self.db.add_all( - UserPermission(user_id=user.id, permission_key=key, assigned_by=assigned_by) + UserPermission(user_id=user_id, permission_key=key, assigned_by=assigned_by) for key in wanted - existing ) await self.db.flush() - role_names = [r.name for r in user.roles] + role_names = _role_names(user) inherited = self._resolve_role_permissions(role_names) - wanted return UserPermissionsOut( - user=UserOut.model_validate(user), + user=_user_out(user), roles=sorted(role_names), direct=sorted(wanted), inherited=sorted(inherited), @@ -253,8 +255,8 @@ async def resolve_effective_permissions(self, user_id: uuid.UUID) -> set[str]: user = await self._get_user(user_id) if user is None: return set() - direct = set(await self.get_user_direct_keys(user.id)) - inherited = self._resolve_role_permissions([r.name for r in user.roles]) + direct = set(await self.get_user_direct_keys(user_id)) + inherited = self._resolve_role_permissions(_role_names(user)) return direct | inherited # ── Startup sync ─────────────────────────────────────────── @@ -284,16 +286,14 @@ async def sync_admin_all_permissions(self, assigned_by: str | None = None) -> No UI — without this sync it would show admin missing newly registered keys until someone clicked Save. """ - from users.constants import ADMIN_ROLE_NAME - registered = set(self.registry.all_permissions) - existing = set(await self._get_role_keys(ADMIN_ROLE_NAME)) + existing = set(await self._get_role_keys(ADMIN_ROLE)) missing = registered - existing if not missing: return self.db.add_all( - RolePermission(role_name=ADMIN_ROLE_NAME, permission_key=key, assigned_by=assigned_by) + RolePermission(role_name=ADMIN_ROLE, permission_key=key, assigned_by=assigned_by) for key in missing ) await self.db.flush() - self.registry.set_role_overlay(ADMIN_ROLE_NAME, registered) + self.registry.set_role_overlay(ADMIN_ROLE, registered) diff --git a/modules/permissions/tests/test_permissions_directory.py b/modules/permissions/tests/test_permissions_directory.py new file mode 100644 index 00000000..f5a0d96e --- /dev/null +++ b/modules/permissions/tests/test_permissions_directory.py @@ -0,0 +1,66 @@ +"""The user side of the permissions screens reads the user directory (GH #501).""" + +from __future__ import annotations + +import uuid + +from permissions.service import PermissionService +from simple_module_core.permissions import ADMIN_ROLE, PermissionRegistry +from simple_module_core.user_directory import DirectoryUser, NullUserDirectory +from sqlalchemy.ext.asyncio import AsyncSession + +_UID = uuid.uuid4() + + +class _IdpDirectory(NullUserDirectory): + """An IdP-backed directory: knows the user, not their roles.""" + + def __init__(self) -> None: + self.searches: list[tuple[str | None, int]] = [] + + async def get_many(self, db, ids, *, with_roles=False): + user = DirectoryUser(id=str(_UID), email="idp@example.com", full_name="Ida") + return {str(_UID): user} if str(_UID) in ids else {} + + async def search(self, db, term, *, limit): + self.searches.append((term, limit)) + return [DirectoryUser(id=str(_UID), email="idp@example.com")] + + +def _registry() -> PermissionRegistry: + reg = PermissionRegistry() + reg.add_group("Products", ["products.view", "products.create"]) + reg.map_role("editor", ["products.view"]) + return reg + + +async def test_direct_grants_work_when_roles_are_unknown(db_session: AsyncSession): + svc = PermissionService(db_session, _registry(), _IdpDirectory()) + updated = await svc.set_user_permissions(_UID, ["products.create"]) + assert updated is not None + assert updated.user.email == "idp@example.com" + # Unknown roles are not read as roles held: nothing is inherited. + assert (updated.roles, updated.inherited) == ([], []) + assert await svc.resolve_effective_permissions(_UID) == {"products.create"} + + +async def test_list_users_searches_the_directory(db_session: AsyncSession): + directory = _IdpDirectory() + rows = await PermissionService(db_session, _registry(), directory).list_users_with_counts( + search="idp", limit=5 + ) + assert [(u.email, n) for u, n in rows] == [("idp@example.com", 0)] + assert directory.searches == [("idp", 5)] + + +async def test_without_a_directory_nobody_is_found(db_session: AsyncSession): + svc = PermissionService(db_session, _registry()) + assert await svc.get_user_permissions(_UID) is None + assert await svc.list_users_with_counts() == [] + + +def test_admin_role_name_comes_from_core(): + """The admin sync keys on core's ``ADMIN_ROLE``, not ``users.constants``.""" + from users.constants import ADMIN_ROLE_NAME + + assert ADMIN_ROLE == ADMIN_ROLE_NAME diff --git a/modules/permissions/tests/test_permissions_grant_search.py b/modules/permissions/tests/test_permissions_grant_search.py index a9c2bd80..13926ca5 100644 --- a/modules/permissions/tests/test_permissions_grant_search.py +++ b/modules/permissions/tests/test_permissions_grant_search.py @@ -6,6 +6,7 @@ from permissions.service import PermissionService from simple_module_core.permissions import PermissionRegistry from test_permissions_module import _seed_user +from users.directory import UsersDirectory class TestGrantSearchEscaping: @@ -17,7 +18,7 @@ async def test_search_matches_like_metacharacters_literally(self, db_session): await _seed_user(db_session, email="danaYx@test") registry = PermissionRegistry() registry.add_group(PERMISSION_GROUP, [PERM_VIEW, PERM_MANAGE]) - svc = PermissionService(db_session, registry) + svc = PermissionService(db_session, registry, UsersDirectory()) rows = await svc.list_users_with_counts(search="dana_x") diff --git a/modules/permissions/tests/test_permissions_inheritance_sources.py b/modules/permissions/tests/test_permissions_inheritance_sources.py index 5eca8c49..802aa7ba 100644 --- a/modules/permissions/tests/test_permissions_inheritance_sources.py +++ b/modules/permissions/tests/test_permissions_inheritance_sources.py @@ -11,6 +11,7 @@ from permissions.service import PermissionService from simple_module_core.permissions import WILDCARD, PermissionRegistry from sqlalchemy.ext.asyncio import AsyncSession +from users.directory import UsersDirectory def _registry() -> PermissionRegistry: @@ -21,7 +22,7 @@ def _registry() -> PermissionRegistry: def _service(db_session: AsyncSession, reg: PermissionRegistry) -> PermissionService: - return PermissionService(db_session, reg) + return PermissionService(db_session, reg, UsersDirectory()) class TestResolveRoleSources: diff --git a/modules/permissions/tests/test_permissions_module.py b/modules/permissions/tests/test_permissions_module.py index de238616..3debeb5f 100644 --- a/modules/permissions/tests/test_permissions_module.py +++ b/modules/permissions/tests/test_permissions_module.py @@ -12,6 +12,7 @@ from permissions.service import PermissionService from simple_module_core.permissions import PermissionRegistry from sqlalchemy.ext.asyncio import AsyncSession +from users.directory import UsersDirectory # ── register_permissions hook ────────────────────────────────── @@ -75,21 +76,21 @@ class TestRoleService: async def test_list_registered_groups( self, db_session: AsyncSession, registry: PermissionRegistry ): - svc = PermissionService(db_session, registry) + svc = PermissionService(db_session, registry, UsersDirectory()) names = {g.name for g in svc.list_registered_groups()} assert {"Products", PERMISSION_GROUP} <= names async def test_get_role_permissions_not_found( self, db_session: AsyncSession, registry: PermissionRegistry ): - svc = PermissionService(db_session, registry) + svc = PermissionService(db_session, registry, UsersDirectory()) assert await svc.get_role_permissions(uuid.uuid4()) is None async def test_set_and_get_role_permissions( self, db_session: AsyncSession, registry: PermissionRegistry ): role_id = await _seed_role(db_session) - svc = PermissionService(db_session, registry) + svc = PermissionService(db_session, registry, UsersDirectory()) updated = await svc.set_role_permissions(role_id, ["products.view", "products.create"]) assert updated is not None @@ -103,7 +104,7 @@ async def test_set_filters_unregistered_keys( self, db_session: AsyncSession, registry: PermissionRegistry ): role_id = await _seed_role(db_session) - svc = PermissionService(db_session, registry) + svc = PermissionService(db_session, registry, UsersDirectory()) updated = await svc.set_role_permissions(role_id, ["products.view", "bogus.key"]) assert updated is not None assert updated.permissions == ["products.view"] @@ -112,7 +113,7 @@ async def test_set_is_replace_semantics( self, db_session: AsyncSession, registry: PermissionRegistry ): role_id = await _seed_role(db_session) - svc = PermissionService(db_session, registry) + svc = PermissionService(db_session, registry, UsersDirectory()) await svc.set_role_permissions(role_id, ["products.view", "products.create"]) await svc.set_role_permissions(role_id, ["products.view"]) fetched = await svc.get_role_permissions(role_id) @@ -123,7 +124,7 @@ async def test_set_updates_registry_role_map( self, db_session: AsyncSession, registry: PermissionRegistry ): role_id = await _seed_role(db_session) - svc = PermissionService(db_session, registry) + svc = PermissionService(db_session, registry, UsersDirectory()) await svc.set_role_permissions(role_id, ["products.view"]) assert "products.view" in registry.role_map.get("user", []) @@ -131,7 +132,7 @@ async def test_load_all_into_registry( self, db_session: AsyncSession, registry: PermissionRegistry ): role_id = await _seed_role(db_session) - await PermissionService(db_session, registry).set_role_permissions( + await PermissionService(db_session, registry, UsersDirectory()).set_role_permissions( role_id, ["products.view"] ) fresh = PermissionRegistry() @@ -144,14 +145,14 @@ class TestUserService: async def test_get_user_permissions_not_found( self, db_session: AsyncSession, registry: PermissionRegistry ): - svc = PermissionService(db_session, registry) + svc = PermissionService(db_session, registry, UsersDirectory()) assert await svc.get_user_permissions(uuid.uuid4()) is None async def test_set_and_get_user_permissions( self, db_session: AsyncSession, registry: PermissionRegistry ): user_id = await _seed_user(db_session) - svc = PermissionService(db_session, registry) + svc = PermissionService(db_session, registry, UsersDirectory()) updated = await svc.set_user_permissions(user_id, ["products.view"]) assert updated is not None @@ -166,7 +167,7 @@ async def test_set_filters_unregistered_keys_for_user( self, db_session: AsyncSession, registry: PermissionRegistry ): user_id = await _seed_user(db_session) - svc = PermissionService(db_session, registry) + svc = PermissionService(db_session, registry, UsersDirectory()) updated = await svc.set_user_permissions(user_id, ["products.view", "bogus"]) assert updated is not None assert updated.direct == ["products.view"] @@ -176,7 +177,7 @@ async def test_inherited_deduplicated_from_direct( ): role_id = await _seed_role(db_session) user_id = await _seed_user(db_session, roles=[role_id]) - svc = PermissionService(db_session, registry) + svc = PermissionService(db_session, registry, UsersDirectory()) await svc.set_role_permissions(role_id, ["products.view"]) await svc.set_user_permissions(user_id, ["products.view", "products.create"]) @@ -194,7 +195,7 @@ async def test_resolve_effective_permissions_union( ): role_id = await _seed_role(db_session) user_id = await _seed_user(db_session, roles=[role_id]) - svc = PermissionService(db_session, registry) + svc = PermissionService(db_session, registry, UsersDirectory()) await svc.set_role_permissions(role_id, ["products.view"]) await svc.set_user_permissions(user_id, ["products.create"]) @@ -205,7 +206,7 @@ async def test_resolve_effective_permissions_union( async def test_resolve_effective_permissions_missing_user( self, db_session: AsyncSession, registry: PermissionRegistry ): - svc = PermissionService(db_session, registry) + svc = PermissionService(db_session, registry, UsersDirectory()) assert await svc.resolve_effective_permissions(uuid.uuid4()) == set() diff --git a/uv.lock b/uv.lock index 12d8fe50..d9512bcf 100644 --- a/uv.lock +++ b/uv.lock @@ -3065,7 +3065,6 @@ dependencies = [ { name = "simple-module-core" }, { name = "simple-module-db" }, { name = "simple-module-hosting" }, - { name = "simple-module-users" }, ] [package.metadata] @@ -3073,7 +3072,6 @@ requires-dist = [ { name = "simple-module-core", editable = "framework/core" }, { name = "simple-module-db", editable = "framework/db" }, { name = "simple-module-hosting", editable = "framework/hosting" }, - { name = "simple-module-users", editable = "modules/users" }, ] [[package]] @@ -3183,7 +3181,6 @@ dependencies = [ { name = "simple-module-core" }, { name = "simple-module-db" }, { name = "simple-module-hosting" }, - { name = "simple-module-users" }, ] [package.metadata] @@ -3191,7 +3188,6 @@ requires-dist = [ { name = "simple-module-core", editable = "framework/core" }, { name = "simple-module-db", editable = "framework/db" }, { name = "simple-module-hosting", editable = "framework/hosting" }, - { name = "simple-module-users", editable = "modules/users" }, ] [[package]] From 0a51fae1ed06955f92bb10dda5e8cd836548c881 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 14:22:28 +0200 Subject: [PATCH 5/9] docs: user directory and activity feed (#501) New docs/framework/user-directory.md (sidebar entry), module docs for audit_log, dashboard, background_tasks, api_tokens, permissions, users and keycloak, a CLAUDE.md architecture note, the conventions skill rule, and CHANGELOG entries. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 29 ++++++++ CLAUDE.md | 6 ++ docs/.vitepress/config.ts | 1 + docs/framework/user-directory.md | 89 +++++++++++++++++++++++ docs/modules/api_tokens.md | 2 +- docs/modules/audit_log.md | 6 +- docs/modules/background_tasks.md | 2 +- docs/modules/dashboard.md | 5 +- docs/modules/keycloak.md | 10 +++ docs/modules/permissions.md | 5 ++ docs/modules/users.md | 8 ++ skills/simple-module-conventions/SKILL.md | 4 + 12 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 docs/framework/user-directory.md diff --git a/CHANGELOG.md b/CHANGELOG.md index a8cd8004..ae3883ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,8 +23,37 @@ All notable changes to this project are documented in this file. The format is b emailed when `public_base_url` is set. The invite API response gains `emailed`, and the invite form says whether the link was emailed. - `captured_mail` pytest fixture in `simple_module_test` records sent mail. +- User directory protocol (#501): `simple_module_core.user_directory` + (`UserDirectory`, `DirectoryUser`, `DirectoryStats`, `NullUserDirectory`, + `get_user_directory` / `set_user_directory`). `users` installs + `UsersDirectory`, `keycloak` installs `KeycloakDirectory` (users who have + signed in; roles unknown). `audit_log`, `dashboard`, `file_storage`, + `api_tokens` and `permissions` read users through it instead of importing + `users.models`, so actor names and search, dashboard tiles and uploader + names now work under Keycloak instead of querying the empty `users_user` + table. See `docs/framework/user-directory.md`. +- Activity feed protocol (#501): `simple_module_core.activity_feed`. + `audit_log` installs it; the users edit page's *Recent activity* card reads + it, which removes the `users` ↔ `audit_log` import cycle. ### Changed +- `audit_log` and `dashboard` no longer depend on `Users` (meta `depends_on` + and package dependency); `background_tasks` drops its unused `Users` + dependency (#501). `permissions` (role pages) and `api_tokens` still do. +- **Behaviour change (#501):** dashboard user figures the auth provider cannot + give are `null` in `/api/dashboard/stats` and their tile is hidden (with no + provider, all three; under Keycloak, `users_created_this_month`). Personal + API tokens are refused for an owner whose roles the directory cannot read + (Keycloak), as before. +- `audit_log.resolve`: `resolve_actors`, `actor_filter` and `actor_link` take + the `UserDirectory`; `USER_ENTITY_TYPE` is gone (use the directory's + `entity_type`). `audit_log.export.stream_csv` takes the directory too. + `PermissionService` and `ApiTokenService` accept a directory (defaults to + the null directory). With no directory, the audit Actor search matches the + term as an exact actor id (#501). +- `smpy` module catalog: `users` now requires `settings`, `background_tasks` + requires `settings` instead of `users`, `dashboard` requires nothing, + mirroring each module's `depends_on` (#501). - The module-settings mechanism moved from the `settings` plugin into the framework (#500): `register_module_settings` / `ModuleSettingsRegistry` to `simple_module_core.module_settings`, hydration (`hydrate_settings`, diff --git a/CLAUDE.md b/CLAUDE.md index d499391e..9376f9d2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -195,6 +195,12 @@ every Tailwind utility, so keep tokens in `theme.css` and rules in `styles.css` delivery (outbox via `background_tasks`): `bus.subscribe_durable` + `await bus.publish_durable(event, db)` inside the request transaction; a durable handler must not commit `ctx.db`. `docs/framework/events.md`. +- Users: read accounts through `get_user_directory(app)` + (`simple_module_core.user_directory`; `get_many`, `search`, `match_ids`, + `stats`), never `users.models` outside `users`; the auth provider installs + it in `register_settings`. `DirectoryUser.roles is None` means unknown + (Keycloak), not none. Recent activity: `get_activity_feed(app)` (audit_log + installs it). `docs/framework/user-directory.md`. - Menus: `MenuRegistry.add_provider(fn)` for per-request items; permissions: `PermissionRegistry.add_source(name, provider)` + `invalidate_source(name)`. `docs/framework/permissions.md`. diff --git a/docs/.vitepress/config.ts b/docs/.vitepress/config.ts index c0bbebb2..69dc90d1 100644 --- a/docs/.vitepress/config.ts +++ b/docs/.vitepress/config.ts @@ -123,6 +123,7 @@ export default defineConfig({ { text: "Settings & app.state", link: "/framework/settings" }, { text: "Permissions", link: "/framework/permissions" }, { text: "Principal resolvers", link: "/framework/principal-resolvers" }, + { text: "User directory", link: "/framework/user-directory" }, { text: "Public routes", link: "/framework/public-routes" }, { text: "Events", link: "/framework/events" }, { text: "Cache invalidation", link: "/framework/invalidation" }, diff --git a/docs/framework/user-directory.md b/docs/framework/user-directory.md new file mode 100644 index 00000000..0872dd75 --- /dev/null +++ b/docs/framework/user-directory.md @@ -0,0 +1,89 @@ +# User directory and activity feed + +Which module holds the user accounts depends on the auth provider: `users` +keeps them in `users_user`, `keycloak` keeps a cache of everyone who has signed +in. A module that only needs to *name* a user, look a few up, search them or +count them reads them through the **user directory**, a protocol in +`simple_module_core.user_directory`, and never imports `users.models`. That +keeps it working under either provider instead of silently querying an empty +table on a Keycloak install (GH #501). + +## Reading users + +```python +from simple_module_core.user_directory import get_user_directory + +directory = get_user_directory(request.app) +found = await directory.get_many(db, ["3f0c…", "celery-worker"]) # {id: DirectoryUser} +``` + +| Member | Returns | +|---|---| +| `get_many(db, ids, *, with_roles=False)` | `{id: DirectoryUser}` keyed by **your** id strings. Ids that are not uuids or name nobody are absent, never an error. Implementations chunk (`GET_MANY_CHUNK`, 500 ids per query). | +| `search(db, term, *, limit)` | Users whose name or email contains `term`, ordered by email; everyone up to `limit` when `term` is empty. | +| `match_ids(term)` | An **unexecuted** `SELECT` of the matching text ids, for use as an unbounded subquery (the audit log's Actor filter). `None` when the provider cannot express it. | +| `stats(db, *, active_window)` | `DirectoryStats(total, active_since, created_this_month)`; a `None` field means "cannot say", not zero. | +| `entity_type` | The audit-link key user rows are recorded under (`"User"`), or `None` when they have no audited model to link to. | + +`DirectoryUser` carries `id`, `email`, `full_name`, `is_active` (false for a +disabled account too) and `roles`: a tuple of role names when `with_roles=True` +and the provider knows them, otherwise `None`. **`None` means unknown, not "no +roles"**: Keycloak keeps roles in the IdP, so code that would act on them +(personal API tokens) must refuse rather than treat the user as role-less. + +Name a user with `DirectoryUser.label` (the full name, else the email). It is +the one naming rule, so a row is never called two things on one screen. +`resolve_user_labels(directory, db, ids)` maps a page of ids to labels in one +call; `parse_user_ids` and `chunked` help an implementation key and bound its +queries. + +## Providers + +The active auth provider installs its directory in `register_settings` with +`set_user_directory(app, directory)`: + +| Provider | Directory | Knows | +|---|---|---| +| `users` | `users.directory.UsersDirectory` | every local account, roles via `users_user_role`, all three stats | +| `keycloak` | `keycloak.directory.KeycloakDirectory` | users who have signed in at least once (`keycloak_user_cache`); roles `None`; `created_this_month` `None`; `entity_type` `None` | +| none | `NullUserDirectory` | nobody; every stat `None`; `match_ids` `None` | + +A new provider implements the four methods and `entity_type`, and registers +it. Ids must come back in the canonical hyphenated text form the rest of the +app stores (`str(uuid)`); `KeycloakDirectory` shows how to render that on +SQLite, where SQLAlchemy's `Uuid` is 32 bare hex characters. + +## Who uses it + +| Module | Reads | +|---|---| +| `audit_log` | actor names (`resolve_actors`), the Actor search (`actor_filter`, a subquery via `match_ids`, an exact id match without one) and actor links (keyed by `entity_type`) | +| `dashboard` | the user tiles from `stats()`; a `None` tile is hidden | +| `file_storage` | uploader names | +| `api_tokens` | owner checks and emails; personal-token resolution via `get_many(with_roles=True)` | +| `permissions` | the user side of the grants screens (`get_many(with_roles=True)`, `search`) | + +`permissions` role pages and `api_tokens` still depend on `Users`: roles have +no directory protocol yet (the `Role` table is read from `users.contracts.roles`), +and a personal token needs the owner's roles. + +## Activity feed + +The users edit page shows what an account did lately. That comes from the +audit log, but `users` and `audit_log` must not import each other (each is +optional to the other, and importing both ways was a cycle). So +`simple_module_core.activity_feed` defines an `ActivityFeed` protocol: + +```python +from simple_module_core.activity_feed import get_activity_feed + +feed = get_activity_feed(request.app) # None: nothing records activity +items = await feed.recent_for_actor(db, user_id, limit=6, permissions=granted) +``` + +Each `ActivityItem` has `at`, `action`, `entity_type`, `entity_id`, +`entity_label` (or `None` when the owner cannot name the row, or the reader +lacks its `AuditLink.label_permission`) and the raw `changes`. `audit_log` +installs the feed in `register_settings`; `set_activity_feed(app, None)` +removes it. A missing feed renders no card, which is a different claim from +an empty one. diff --git a/docs/modules/api_tokens.md b/docs/modules/api_tokens.md index 83a4eb85..ddff6719 100644 --- a/docs/modules/api_tokens.md +++ b/docs/modules/api_tokens.md @@ -104,7 +104,7 @@ everywhere) authenticate through the session cookie only and never see a token. | Expiry is required and at most `max_lifetime_days` (setting, default 365) | service | | Service key roles: platform roles only — never `admin`, never `tenant:*` | admin endpoint + service + resolver | | Only a **cookie session** may create, list or revoke tokens: `auth_kind == "session"` and no `Authorization` header. A token, a `users` access token or a Keycloak JWT gets 403 | every endpoint and page | -| The creator must have a local `users_user` row the resolver can load (under Keycloak, a cached user); otherwise 400 `api_tokens.errors.account_required` | personal create endpoint | +| The creator must be an active account in the app's [user directory](/framework/user-directory) whose roles are known; otherwise 400 `api_tokens.errors.account_required`. Under Keycloak roles live in the IdP, so personal tokens are refused there | personal create endpoint | | Cookie-authenticated mutations require `X-CSRF-Token` (`RequiresCsrf`) | every mutation | ## Tenancy diff --git a/docs/modules/audit_log.md b/docs/modules/audit_log.md index 339a36cd..85fe016c 100644 --- a/docs/modules/audit_log.md +++ b/docs/modules/audit_log.md @@ -9,7 +9,7 @@ Automatic field-level audit trail for every SQLModel entity in the app. Each cre | `name` | `AuditLog` | | `route_prefix` | `/api/audit_log` | | `view_prefix` | `/admin/audit-log` | -| `depends_on` | `["Users"]` | +| `depends_on` | `[]` | ## How capture works @@ -155,6 +155,8 @@ An entry stores a model class name and a primary key, which proves what happened **The entity column's names are permission-gated.** An `AuditLink` may declare a `label_permission`, and a reader who does not hold it gets the id instead of the name. `users` declares `users.manage`, because a user row is named by `full_name or email` — so without the gate, `audit_log.view` alone was a way to read the display name of every account that appears in the log, and the email address of every account still holding an unaccepted invite (those have no `full_name` yet). The entry itself is never withheld: the action, the type, the timestamp and the id are the audit trail. Links whose owner declares nothing — setting keys, filenames, task names, flag names — are shown to anyone who may read the trail. +Actor names come from the [user directory](/framework/user-directory) (`DirectoryUser.label`), so they follow whichever auth provider holds the accounts. The Actor search box matches a uuid exactly and anything else against names and emails as a database subquery (`match_ids`), with no ceiling on how many accounts match; with no directory it matches the term as an exact actor id. + **The actor column's names are not.** `audit_log.view` does imply "may see who acted", and that is deliberate: an audit trail that will not say who performed a change is not one. The two columns disclose the same field and answer different questions — the entity column names people who were merely *edited*, which reviewing the trail does not require. Grant `audit_log.view` knowing it discloses the display name (or email) of everyone who has ever made a change. The `changes` column is a third surface, and a wider one: it records the values that were written, so a `User` create entry contains the email that was set. Use `__audit_exclude_fields__` on a model to keep a column out of the diff (see § Opting out). @@ -192,4 +194,4 @@ The module is discovered automatically once `simple_module_audit_log` is install pip install simple_module_audit_log ``` -It depends on the [`users`](/modules/users) module (for the menu/permission wiring and the request-scoped `user_id`). +It depends on no other module. Actor names, the Actor search and actor links come from the app's [user directory](/framework/user-directory), so they work under `users`, `keycloak` (users who have signed in) or no provider at all (raw ids). The module also installs the core [activity feed](/framework/user-directory#activity-feed) the users edit page reads. diff --git a/docs/modules/background_tasks.md b/docs/modules/background_tasks.md index 38fc21d5..2b124ca1 100644 --- a/docs/modules/background_tasks.md +++ b/docs/modules/background_tasks.md @@ -9,7 +9,7 @@ Celery + Redis task queue with persistent history, an admin UI for monitoring + | `name` | `BackgroundTasks` | | `route_prefix` | `/api/background_tasks` | | `view_prefix` | `/admin/background-tasks` | -| `depends_on` | `["Users"]` | +| `depends_on` | `["Settings"]` | ## Writing a task diff --git a/docs/modules/dashboard.md b/docs/modules/dashboard.md index d32d4216..0e10193c 100644 --- a/docs/modules/dashboard.md +++ b/docs/modules/dashboard.md @@ -11,7 +11,7 @@ It's intentionally simple — a place for new installs to land that proves the p | `name` | `Dashboard` | | `route_prefix` | `/api/dashboard` | | `view_prefix` | `/dashboard` | -| `depends_on` | `["Users"]` | +| `depends_on` | `[]` | ## Routes @@ -47,6 +47,7 @@ the role editor to change that. { "total_users": 12, "active_users_7d": 3, + "users_created_this_month": 2, "module_count": 8, "system_info": { "python_version": "3.12.4", @@ -56,6 +57,8 @@ the role editor to change that. } ``` +The three user figures come from the app's [user directory](/framework/user-directory) (`stats()`), so they count whichever auth provider holds the accounts. A figure the provider cannot give is `null` and its tile is hidden: under `keycloak` `users_created_this_month` is `null` (the cache only learns of a user at first sign-in), and with no auth provider all three are. + The result is cached process-wide for 30 seconds. If you mutate something that should change the numbers (e.g. seed users from a script), call `dashboard.stats.invalidate_stats_cache()` to clear it on the next call. ## Menu diff --git a/docs/modules/keycloak.md b/docs/modules/keycloak.md index 8b093369..bf0841b5 100644 --- a/docs/modules/keycloak.md +++ b/docs/modules/keycloak.md @@ -105,6 +105,16 @@ Keycloak registers no setup steps: its local `users` table is legitimately empty | `full_name` | `str \| None` | from `preferred_username` | | `last_login_at` | `datetime \| None` | refreshed on each resolve | +The cache backs this module's [user directory](/framework/user-directory) +(`keycloak.directory.KeycloakDirectory`, installed in `register_settings`), so +the audit log, dashboard, file storage and the permissions grants screen name +and count Keycloak users. It knows only users who have signed in at least once; +roles are `None` (they live in the IdP) and `created_this_month` is `None`. +Modules that still need the `users` tables — `permissions` (role pages) and +`api_tokens` (personal tokens need the owner's roles) — declare +`depends_on=["Users"]`, so leave them out of a Keycloak install +(`SM_MODULES_ENABLED`). + ## Menu | Label | URL | Icon | Section | Order | Method | diff --git a/docs/modules/permissions.md b/docs/modules/permissions.md index e0705491..667e2c04 100644 --- a/docs/modules/permissions.md +++ b/docs/modules/permissions.md @@ -17,6 +17,11 @@ There is one `RequiresPermission`, in `simple_module_hosting.permissions`. With | `view_prefix` | `/admin/permissions` | | `depends_on` | `["Auth", "Users"]` | +The user side of the screens (looking a user up, searching, their roles) goes +through the [user directory](/framework/user-directory). The role pages read +the `users` module's `Role` table (`users.contracts.roles`), which is why the +module still depends on `Users`. + ## Routes ### API diff --git a/docs/modules/users.md b/docs/modules/users.md index d9077327..d952bff6 100644 --- a/docs/modules/users.md +++ b/docs/modules/users.md @@ -129,6 +129,14 @@ from users.contracts.events import ( | `RoleListItem` | `id`, `name`, `description`, `user_count`. | | `UserRegistered`, `UserInvited`, `UserCreated`, `UserDeleted`, `UserDisabled`, `RoleAssigned` | Events — see [Events](#events). | +Other modules read accounts through the [user directory](/framework/user-directory), +which this module installs as `users.directory.UsersDirectory` in +`register_settings`; they should not import `users.models`. The one table read +directly is `Role`, re-exported as `users.contracts.roles.Role` for the +permissions role pages. The edit page's *Recent activity* card reads the core +[activity feed](/framework/user-directory#activity-feed) rather than importing +`audit_log`. + ## Models `User` (table `users_user`) diff --git a/skills/simple-module-conventions/SKILL.md b/skills/simple-module-conventions/SKILL.md index 8b0f028a..6eca3b89 100644 --- a/skills/simple-module-conventions/SKILL.md +++ b/skills/simple-module-conventions/SKILL.md @@ -116,6 +116,10 @@ The host CSP blocks any origin you don't declare — a font CDN or tile server y Build a `MailMessage(to, subject, text)` and send it with `current_mail_sender()` (request code) or `await resolve_mail_sender(db)` (durable handlers, which run in the outbox worker without an app). With no transport installed both return a logging sender, so never guard on "is mail configured"; read `sender.delivers_email` when a screen must hand the link back instead. The `mail` module owns SMTP and its settings. In tests, request the `captured_mail` fixture. +### 12. Read users through the user directory, never `users.models` + +Which module holds the accounts depends on the auth provider (`users` or `keycloak`). To name, look up, search or count users, use `get_user_directory(app)` from `simple_module_core.user_directory`: `get_many(db, ids, with_roles=...)`, `search(db, term, limit=...)`, `match_ids(term)` (a subquery) and `stats(db, active_window=...)`. Name a user with `DirectoryUser.label`. `roles is None` means the provider does not know them (Keycloak) — refuse anything that needs roles rather than treating the user as role-less. Importing `users.models` from another module silently queries an empty table under Keycloak. + ## Related skills - **simple-module-database** — per-module Base, mixins, session lifecycle From 76b7cddcd8ec5329f99c0a4325b8d7cbfad1f5c0 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 14:35:14 +0200 Subject: [PATCH 6/9] feat(discovery): refuse missing depends_on outside development (#501) topological_sort(modules, strict=...) collects every (module, dependency) pair whose dependency is not loaded; strict raises one MissingDependencyError (an InvalidModuleError) naming them all. create_app passes strict=not is_development; development keeps logging per pair and the new SM030 diagnostic reports it (warning at a dev boot, error in make doctor). The outbox worker stays lenient. ModuleMeta.optional_depends_on orders after a module when it is loaded and is never missing. The ordering-only Settings entries (users, tenants, file_storage, background_tasks, site_lock, api_tokens, keycloak) move there, keeping the boot order unchanged and the settings plugin optional (#500). The smpy catalog no longer makes users require settings. Keycloak test apps leave out modules that need Users (permissions, api_tokens) instead of booting leniently; a test pins that the full set fails a non-development boot naming both. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 19 ++- CLAUDE.md | 10 +- docs/framework/discovery.md | 10 +- docs/modules/api_tokens.md | 2 +- docs/modules/background_tasks.md | 2 +- docs/modules/file_storage.md | 2 +- docs/modules/keycloak.md | 2 +- docs/modules/site_lock.md | 2 +- docs/reference/diagnostic-codes.md | 3 +- framework/cli/simple_module_cli/catalog.py | 6 +- framework/cli/tests/test_cli_catalog.py | 8 +- framework/cli/tests/test_cli_wizard.py | 6 +- framework/core/simple_module_core/__init__.py | 4 + framework/core/simple_module_core/__main__.py | 6 + .../diagnostics/_dependencies.py | 42 +++++++ .../simple_module_core/diagnostics/_module.py | 2 + .../core/simple_module_core/discovery.py | 53 ++++++--- .../core/simple_module_core/exceptions.py | 19 +++ .../core/simple_module_core/module_meta.py | 10 ++ .../core/tests/test_discovery_missing_deps.py | 108 ++++++++++++++++++ .../simple_module_hosting/app_builder.py | 2 +- modules/api_tokens/api_tokens/module.py | 6 +- .../background_tasks/module.py | 2 +- .../background_tasks/outbox_worker.py | 2 + modules/file_storage/file_storage/module.py | 2 +- modules/keycloak/keycloak/module.py | 4 +- .../keycloak/tests/keycloak_test_modules.py | 19 +++ .../keycloak/tests/test_keycloak_module.py | 3 +- .../tests/test_keycloak_strict_boot.py | 56 +++++++++ .../tests/test_keycloak_tenant_hardening.py | 2 + modules/keycloak/tests/test_keycloak_views.py | 2 + modules/site_lock/README.md | 3 +- modules/site_lock/site_lock/module.py | 3 +- modules/tenants/tenants/module.py | 6 +- modules/users/users/module.py | 3 +- skills/simple-module-creating/SKILL.md | 3 +- skills/simple-module-doctor/SKILL.md | 5 +- 37 files changed, 385 insertions(+), 54 deletions(-) create mode 100644 framework/core/simple_module_core/diagnostics/_dependencies.py create mode 100644 framework/core/tests/test_discovery_missing_deps.py create mode 100644 modules/keycloak/tests/keycloak_test_modules.py create mode 100644 modules/keycloak/tests/test_keycloak_strict_boot.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ae3883ad..c31dfddc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,9 +51,22 @@ All notable changes to this project are documented in this file. The format is b `PermissionService` and `ApiTokenService` accept a directory (defaults to the null directory). With no directory, the audit Actor search matches the term as an exact actor id (#501). -- `smpy` module catalog: `users` now requires `settings`, `background_tasks` - requires `settings` instead of `users`, `dashboard` requires nothing, - mirroring each module's `depends_on` (#501). +- `smpy` module catalog: `background_tasks` requires `settings` instead of + `users` and `dashboard` requires nothing; a test pins that every entry's + `requires` covers its module's `depends_on` (#501). +- **Behaviour change (#501):** outside development, a module whose + `depends_on` names a module that is not loaded now refuses the boot with one + `MissingDependencyError` (an `InvalidModuleError`) listing every + `(module, dependency)` pair, instead of a warning and a module running + against tables nobody owns. Development still logs it; `make doctor` + reports it as the new `SM030` error. Selecting `keycloak` drops `Users`, so + a Keycloak install must leave out `permissions` and `api_tokens` + (`SM_MODULES_ENABLED`). The outbox worker still sorts leniently. +- New `ModuleMeta.optional_depends_on`: modules to sort after when loaded, + never required (#501). `users`, `tenants`, `file_storage`, + `background_tasks`, `site_lock`, `api_tokens` and `keycloak` move their + ordering-only `Settings` entry there, so the boot order is unchanged and + the `settings` plugin can still be disabled (#500). - The module-settings mechanism moved from the `settings` plugin into the framework (#500): `register_module_settings` / `ModuleSettingsRegistry` to `simple_module_core.module_settings`, hydration (`hydrate_settings`, diff --git a/CLAUDE.md b/CLAUDE.md index 9376f9d2..fdc1e920 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -118,8 +118,11 @@ until added. **Discovery.** Modules declare `[project.entry-points.simple_module]` pointing at a `ModuleBase` subclass with `meta = ModuleMeta(...)`. `simple_module_core.discovery.discover_modules()` loads them, topologically sorts -by `ModuleMeta.depends_on`, and the host calls each `register_*` hook in that -order (`docs/framework/discovery.md`). +by `ModuleMeta.depends_on` (plus `optional_depends_on`, ordering only, may be +absent), and the host calls each `register_*` hook in that order +(`docs/framework/discovery.md`). Outside development a missing `depends_on` +target fails the boot (`MissingDependencyError`, `SM030`); selecting +`keycloak` drops `Users`, so modules needing it must be left out. **Two names per module.** `ModuleMeta.name` (scaffold default: PascalCase, `FeatureFlags`) keys Inertia render paths, `depends_on` and `SM_MODULES_ENABLED`. @@ -432,7 +435,8 @@ Unlisted test directories are silently never collected. `make doctor` and a development boot run the `SM0xx` checks; the table with triggers and fixes is `docs/reference/diagnostic-codes.md`. **Production does not run the suite** (only `SM025` is reported there). What fails a production -boot is strict discovery (a bad entry point or missing `meta`), the +boot is strict discovery (a bad entry point, missing `meta`, or a missing +`depends_on` target), the auth-provider check, placeholder secrets (`SM_SECRET_KEY`, users token secrets), a localhost Celery broker, and the `SM010` migration check (except on a never-migrated first run, which goes to `/setup`). Catch everything else diff --git a/docs/framework/discovery.md b/docs/framework/discovery.md index 48c17720..c96761d0 100644 --- a/docs/framework/discovery.md +++ b/docs/framework/discovery.md @@ -32,6 +32,7 @@ Failures: - **Missing `meta`** — emits `SM001`. Strict mode: raises. - **Duplicate `meta.name`** — emits `SM008` (error). Always fails boot, even in dev, because names are used as table-name prefixes. - **Cycle in `depends_on`** — raises `CircularDependencyError`. Always fails boot. +- **Missing `depends_on` target** (a module that is not installed, not enabled, or dropped because another auth provider was selected) — outside development, `topological_sort(strict=True)` raises one `MissingDependencyError` (an `InvalidModuleError`) naming every `(module, dependency)` pair, and the boot fails. In development it is logged per pair and reported as `SM030` (a warning at boot, an error in `make doctor`). The outbox worker sorts leniently. GH #501. ## ModuleMeta @@ -42,7 +43,8 @@ meta = ModuleMeta( name="Orders", # PascalCase, globally unique route_prefix="/api/orders", # where the API router mounts view_prefix="/orders", # where the view router mounts - depends_on=["Products"], # hard ordering requirements + depends_on=["Products"], # required, and sorted after + optional_depends_on=["Settings"], # sorted after when loaded; may be absent version="1.0.0", # semver for the module i18n_audience="public", # who gets this module's locale catalog ) @@ -67,7 +69,11 @@ A list of other modules' `meta.name` values that **must be loaded first**. The f - Your module's `register_routes` reads another module's service to compose endpoints. - Your module subscribes to events published by another module. -`depends_on` is **not** a dependency-injection mechanism. It only controls order — you still `import` the services you need. +`depends_on` is **not** a dependency-injection mechanism. It controls order and presence — you still `import` the services you need. Outside development a missing entry refuses the boot (see Failures above), so list only modules yours cannot work without. + +### `optional_depends_on` + +Modules to sort after **when they are loaded**, for an ordering-only relationship: your middleware wraps outside theirs, or you subscribe to an event they publish, but your module works without them. Present modules order exactly as `depends_on` would, so moving an entry here never changes the boot order; an absent one is fine in every environment. The modules that sorted after `Settings` only for ordering (`users`, `tenants`, `file_storage`, `background_tasks`, `site_lock`, `api_tokens`, `keycloak`) declare it here, because the settings mechanism lives in the framework (#500) and the `settings` plugin may be disabled. ### `version` diff --git a/docs/modules/api_tokens.md b/docs/modules/api_tokens.md index ddff6719..c98cf251 100644 --- a/docs/modules/api_tokens.md +++ b/docs/modules/api_tokens.md @@ -13,7 +13,7 @@ rest, shown once, and revocable with effect on the next request. | `route_prefix` | `/api/api_tokens` | | `view_prefix` | `/api-tokens` | | `admin_view_prefix` | `/admin/api-tokens` | -| `depends_on` | `Auth`, `Users`, `Settings` | +| `depends_on` | `Auth`, `Users` (optional: `Settings`) | Installed in this repository's host. It is **not** part of the `smpy new` scaffold; add `simple_module_api_tokens` to a host's dependencies to enable it, diff --git a/docs/modules/background_tasks.md b/docs/modules/background_tasks.md index 2b124ca1..a68f0783 100644 --- a/docs/modules/background_tasks.md +++ b/docs/modules/background_tasks.md @@ -9,7 +9,7 @@ Celery + Redis task queue with persistent history, an admin UI for monitoring + | `name` | `BackgroundTasks` | | `route_prefix` | `/api/background_tasks` | | `view_prefix` | `/admin/background-tasks` | -| `depends_on` | `["Settings"]` | +| `depends_on` | `[]` (optional: `["Settings"]`) | ## Writing a task diff --git a/docs/modules/file_storage.md b/docs/modules/file_storage.md index 2f56bd42..6bacb022 100644 --- a/docs/modules/file_storage.md +++ b/docs/modules/file_storage.md @@ -9,7 +9,7 @@ Pluggable file storage with two shipped backends — local filesystem and S3-com | `name` | `FileStorage` | | `route_prefix` | `/api/file-storage` | | `view_prefix` | `/file-storage` | -| `depends_on` | `["Settings"]` | +| `depends_on` | `[]` (optional: `["Settings"]`) | ## Routes diff --git a/docs/modules/keycloak.md b/docs/modules/keycloak.md index bf0841b5..3b653b1d 100644 --- a/docs/modules/keycloak.md +++ b/docs/modules/keycloak.md @@ -9,7 +9,7 @@ A pluggable **OIDC auth provider** backed by Keycloak. Install it *instead of* [ | `name` | `Keycloak` | | `route_prefix` | `/api/keycloak` | | `view_prefix` | `/keycloak` | -| `depends_on` | `["Auth", "Settings"]` | +| `depends_on` | `["Auth"]` (optional: `["Settings"]`) | The module sets the class flag `_is_auth_provider = True`, which is what the auth-provider diagnostics count (see [below](#auth-provider-diagnostics)). diff --git a/docs/modules/site_lock.md b/docs/modules/site_lock.md index e297bc9d..9fa87df5 100644 --- a/docs/modules/site_lock.md +++ b/docs/modules/site_lock.md @@ -13,7 +13,7 @@ This is not user authentication — that is what [`auth`](/modules/auth), [`user | `name` | `SiteLock` | | `route_prefix` | *(none)* | | `view_prefix` | *(none)* | -| `depends_on` | `["Settings", "Auth"]` | +| `depends_on` | `["Auth"]` (optional: `["Settings"]`) | Both dependencies are load-bearing. `Settings` lets `register_module_settings` reach the module registry. `Auth` makes this module sort *after* the auth module, and since middleware is installed in topological order while Starlette's `add_middleware` is LIFO, sorting later means wrapping **outermost** — so `SiteLockMiddleware` executes *before* `AuthMiddleware`. diff --git a/docs/reference/diagnostic-codes.md b/docs/reference/diagnostic-codes.md index add65311..1876cb81 100644 --- a/docs/reference/diagnostic-codes.md +++ b/docs/reference/diagnostic-codes.md @@ -1,6 +1,6 @@ # Diagnostic codes -The framework runs a set of static checks over installed modules at app boot. The full structural/page/i18n suite runs in development (`SM_ENVIRONMENT=development`), prints to stderr, and aborts boot on any ERROR. In non-development environments (anything other than `development`) module discovery is **strict** — entry-point failures and missing/invalid `meta` (the SM001 class of problems) raise at boot — and the migration check (SM010) fails boot in every environment. +The framework runs a set of static checks over installed modules at app boot. The full structural/page/i18n suite runs in development (`SM_ENVIRONMENT=development`), prints to stderr, and aborts boot on any ERROR. In non-development environments (anything other than `development`) module discovery is **strict** — entry-point failures, missing/invalid `meta` (the SM001 class of problems) and missing `depends_on` targets (SM030) raise at boot — and the migration check (SM010) fails boot in every environment. ## Levels @@ -38,6 +38,7 @@ The framework runs a set of static checks over installed modules at app boot. Th | `SM027` | WARNING | A key in `host/locales/overrides/.json` matches no catalog key, so the override was skipped. Reported by `make doctor` and at dev boot. | Fix the key: overrides replace an existing key by its full dotted path (e.g. `users.login.aside_heading`). See [i18n](/framework/i18n#overriding-module-copy). | | `SM028` | WARNING | A module's `tasks.py` imports `torch`, `tensorflow`, `ultralytics`, `osgeo`, `rasterio`, `cv2` or `sklearn` at module level, so every process that loads the module's tasks (web, each worker, beat) pays for it and a queue-restricted worker cannot avoid it. | Import the package inside the task function. See [background_tasks](/modules/background_tasks#running-a-worker-for-a-subset-of-modules). | | `SM029` | INFO | A module overrides `db_extensions()` (Postgres extensions such as `ltree`) and the configured database is SQLite, which has none. `ensure_db_extensions` raises `UnsupportedDatabaseError` when migrations run. | Point `SM_DATABASE_URL` at PostgreSQL. See [Postgres extensions and column types](/database/extensions). | +| `SM030` | WARNING at a dev boot; ERROR in `make doctor` | A module's `depends_on` names a module that is not loaded (not installed, left out of `SM_MODULES_ENABLED`, or dropped because another auth provider was selected — `keycloak` drops `Users`). Outside development the boot raises `MissingDependencyError` listing every pair. | Install and enable the dependency, or leave the dependent module out. A relationship that is only about order belongs in `optional_depends_on`. See [Discovery](/framework/discovery#modulemeta). | `SM022`/`SM023` are the two halves of the same invariant: a module's optional [`theme.css` is imported unlayered and `styles.css` into `layer(components)`](/module-authoring#styling), and CSS put in the wrong one silently does nothing (or silently wins everything). diff --git a/framework/cli/simple_module_cli/catalog.py b/framework/cli/simple_module_cli/catalog.py index 721a9103..d26bc95d 100644 --- a/framework/cli/simple_module_cli/catalog.py +++ b/framework/cli/simple_module_cli/catalog.py @@ -29,10 +29,8 @@ class ModuleEntry: CATALOG: dict[str, ModuleEntry] = { "auth": ModuleEntry("auth", "simple_module_auth", "Auth"), - # ``requires`` mirrors each module's ``depends_on``: outside development a - # missing dependency refuses to boot (GH #501), so a scaffold that left - # one out would build an app that cannot start in production. - "users": ModuleEntry("users", "simple_module_users", "Users", requires=("auth", "settings")), + # ``requires`` covers each module's ``depends_on`` (pinned by a test). + "users": ModuleEntry("users", "simple_module_users", "Users", requires=("auth",)), "permissions": ModuleEntry( "permissions", "simple_module_permissions", diff --git a/framework/cli/tests/test_cli_catalog.py b/framework/cli/tests/test_cli_catalog.py index b9efe4ac..f3eb528f 100644 --- a/framework/cli/tests/test_cli_catalog.py +++ b/framework/cli/tests/test_cli_catalog.py @@ -36,15 +36,15 @@ def test_expand_deps_returns_input_when_no_requires() -> None: def test_expand_deps_pulls_in_transitive_dep() -> None: resolved, added = expand_deps(["users"]) - assert set(resolved) == {"auth", "settings", "users"} - assert added == [("auth", "users"), ("settings", "users")] + assert set(resolved) == {"auth", "users"} + assert added == [("auth", "users")] def test_expand_deps_pulls_in_chain() -> None: resolved, added = expand_deps(["permissions"]) - assert set(resolved) == {"permissions", "users", "auth", "settings"} + assert set(resolved) == {"permissions", "users", "auth"} added_names = {a for a, _ in added} - assert added_names == {"users", "auth", "settings"} + assert added_names == {"users", "auth"} def test_expand_deps_idempotent_when_input_already_complete() -> None: diff --git a/framework/cli/tests/test_cli_wizard.py b/framework/cli/tests/test_cli_wizard.py index da7558ed..41860ac4 100644 --- a/framework/cli/tests/test_cli_wizard.py +++ b/framework/cli/tests/test_cli_wizard.py @@ -43,7 +43,7 @@ def test_wizard_postgres_with_tenancy() -> None: def test_wizard_minimal_preset() -> None: _, _, selected, _ = _drive(["", "", "1", "", ""]) - assert set(selected) == {"users", "auth", "settings"} + assert set(selected) == {"users", "auth"} def test_wizard_full_preset_includes_background_tasks() -> None: @@ -60,9 +60,9 @@ def test_wizard_custom_picks_only_yes_answers() -> None: picks = ["y" if name == "permissions" else "n" for name in CATALOG] answers = ["", "", "4", *picks, "", ""] _, _, selected, out = _drive(answers) - assert set(selected) == {"permissions", "users", "auth", "settings"} + assert set(selected) == {"permissions", "users", "auth"} assert "Added users (required by permissions)" in out - assert "Added settings (required by users)" in out + assert "Added auth (required by permissions)" in out def test_wizard_aborts_on_confirm_no() -> None: diff --git a/framework/core/simple_module_core/__init__.py b/framework/core/simple_module_core/__init__.py index ff9f6d21..baa9f3cc 100644 --- a/framework/core/simple_module_core/__init__.py +++ b/framework/core/simple_module_core/__init__.py @@ -14,6 +14,7 @@ DEFAULT_AUTH_PROVIDER, discover_modules, get_module_package_name, + missing_dependencies, resolve_auth_provider, select_auth_provider, topological_sort, @@ -28,6 +29,7 @@ CircularDependencyError, FrameworkVersionError, InvalidModuleError, + MissingDependencyError, ModuleError, NotFoundError, ValidationError, @@ -91,6 +93,7 @@ "MenuRegistry", "MenuSection", "MigrationDiagnostics", + "MissingDependencyError", "ModuleBase", "ModuleError", "ModuleMeta", @@ -113,6 +116,7 @@ "get_module_package_name", "is_flag_enabled", "is_tenant_role", + "missing_dependencies", "print_diagnostics", "require_flag", "resolve_auth_provider", diff --git a/framework/core/simple_module_core/__main__.py b/framework/core/simple_module_core/__main__.py index 98a1cec5..6c437310 100644 --- a/framework/core/simple_module_core/__main__.py +++ b/framework/core/simple_module_core/__main__.py @@ -30,6 +30,7 @@ print_diagnostics, run_diagnostics, ) +from simple_module_core.diagnostics._dependencies import CODE as MISSING_DEPENDENCY_CODE from simple_module_core.diagnostics._i18n import unknown_overrides_for from simple_module_core.discovery import ( discover_modules, @@ -181,6 +182,11 @@ def main() -> int: database_dialect=_database_dialect(), ) diagnostics.extend(override_findings) + # A missing dependency only warns at a dev boot, but production refuses + # to start with it (GH #501), so doctor reports it as what it will be. + for finding in diagnostics: + if finding.code == MISSING_DEPENDENCY_CODE: + finding.level = DiagnosticLevel.ERROR print_diagnostics(diagnostics) errors = [d for d in diagnostics if d.level == DiagnosticLevel.ERROR] diff --git a/framework/core/simple_module_core/diagnostics/_dependencies.py b/framework/core/simple_module_core/diagnostics/_dependencies.py new file mode 100644 index 00000000..fede89b9 --- /dev/null +++ b/framework/core/simple_module_core/diagnostics/_dependencies.py @@ -0,0 +1,42 @@ +"""SM030 — a module ``depends_on`` a module that is not loaded (GH #501). + +A development boot logs it and carries on; ``make doctor`` reports it as an +error; any other environment refuses to boot (``topological_sort(strict=True)`` +raises ``MissingDependencyError``). The usual cause is an auth provider other +than ``users``: selecting it drops ``Users``, and every module that still +depends on it (``permissions``, ``api_tokens``) has to be left out too. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from simple_module_core.diagnostics._types import Diagnostic, DiagnosticLevel + +if TYPE_CHECKING: + from simple_module_core.module import ModuleBase + +CODE = "SM030" + + +def check_missing_dependencies( + modules: list[ModuleBase], level: DiagnosticLevel = DiagnosticLevel.WARNING +) -> list[Diagnostic]: + names = {m.meta.name for m in modules} + return [ + Diagnostic( + level=level, + code=CODE, + message=( + f"Module '{mod.meta.name}' depends on '{dep}', which is not loaded; " + "a non-development boot refuses to start" + ), + module_name=mod.meta.name, + suggestion=( + f"Install and enable '{dep}', or leave '{mod.meta.name}' out (SM_MODULES_ENABLED)" + ), + ) + for mod in modules + for dep in mod.meta.depends_on + if dep not in names + ] diff --git a/framework/core/simple_module_core/diagnostics/_module.py b/framework/core/simple_module_core/diagnostics/_module.py index 701ed96d..97243630 100644 --- a/framework/core/simple_module_core/diagnostics/_module.py +++ b/framework/core/simple_module_core/diagnostics/_module.py @@ -8,6 +8,7 @@ from simple_module_core.diagnostics._coupling import check_framework_module_coupling from simple_module_core.diagnostics._css import check_module_css +from simple_module_core.diagnostics._dependencies import check_missing_dependencies from simple_module_core.diagnostics._heavy_task_imports import check_heavy_task_imports from simple_module_core.diagnostics._inertia_api import check_inertia_api_calls from simple_module_core.diagnostics._js_workspace import check_js_workspace_files @@ -30,6 +31,7 @@ def run(self, modules: list[ModuleBase]) -> list[Diagnostic]: diagnostics.extend(self._check_missing_meta(modules)) diagnostics.extend(self._check_views_without_menu(modules)) diagnostics.extend(self._check_auth_provider_conflict(modules)) + diagnostics.extend(check_missing_dependencies(modules)) diagnostics.extend(check_framework_module_coupling(modules)) for mod in modules: diagnostics.extend(check_tenant_unique_keys(module_tables(mod), mod.meta.name)) diff --git a/framework/core/simple_module_core/discovery.py b/framework/core/simple_module_core/discovery.py index b2b2568c..744e68ee 100644 --- a/framework/core/simple_module_core/discovery.py +++ b/framework/core/simple_module_core/discovery.py @@ -7,7 +7,11 @@ 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.exceptions import ( + CircularDependencyError, + InvalidModuleError, + MissingDependencyError, +) from simple_module_core.module import ModuleBase, ModuleMeta logger = logging.getLogger(__name__) @@ -186,28 +190,49 @@ def select_auth_provider( 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]: +def missing_dependencies(modules: Sequence[ModuleBase]) -> list[tuple[str, str]]: + """Every ``(module, dependency)`` pair whose dependency is not in *modules*.""" + names = {m.meta.name for m in modules} + return [ + (mod.meta.name, dep) for mod in modules for dep in mod.meta.depends_on if dep not in names + ] + + +def _ordering_deps(mod: ModuleBase, loaded: dict[str, ModuleBase]) -> list[str]: + """The loaded modules *mod* must sort after: required and optional alike.""" + wanted = [*mod.meta.depends_on, *mod.meta.optional_depends_on] + return [dep for dep in dict.fromkeys(wanted) if dep in loaded] + + +def topological_sort(modules: Sequence[ModuleBase], *, strict: bool = False) -> list[ModuleBase]: """Sort modules so dependencies come before dependents. + ``optional_depends_on`` orders the same way when present and is never + missing. A ``depends_on`` entry naming a module that is not loaded is + logged and ignored; with ``strict`` every such pair is collected and raised at once as + :class:`MissingDependencyError`. The host passes ``strict`` outside + development: a module whose dependency is absent would otherwise boot and + fail later, or silently query another module's empty tables (GH #501). + Raises ``CircularDependencyError`` if a cycle is detected. """ + missing = missing_dependencies(modules) + if missing and strict: + raise MissingDependencyError(missing) + for name, dep in missing: + logger.warning("Module '%s' depends on '%s' which is not installed — ignoring", name, dep) + by_name: dict[str, ModuleBase] = {m.meta.name: m for m in modules} # Kahn's algorithm in_degree: dict[str, int] = dict.fromkeys(by_name, 0) dependents: dict[str, list[str]] = {name: [] for name in by_name} - for mod in modules: - for dep_name in mod.meta.depends_on: - if dep_name not in by_name: - logger.warning( - "Module '%s' depends on '%s' which is not installed — ignoring", - mod.meta.name, - dep_name, - ) - continue - in_degree[mod.meta.name] += 1 - dependents[dep_name].append(mod.meta.name) + edges = {m.meta.name: _ordering_deps(m, by_name) for m in modules} + for name, deps in edges.items(): + for dep_name in deps: + in_degree[name] += 1 + dependents[dep_name].append(name) queue: list[str] = [name for name, deg in in_degree.items() if deg == 0] result: list[str] = [] @@ -225,7 +250,7 @@ def topological_sort(modules: Sequence[ModuleBase]) -> list[ModuleBase]: if len(result) != len(by_name): # Find the cycle for a helpful error message remaining = set(by_name.keys()) - set(result) - cycle = _find_cycle(remaining, {n: m.meta.depends_on for n, m in by_name.items()}) + cycle = _find_cycle(remaining, edges) raise CircularDependencyError(cycle) return [by_name[name] for name in result] diff --git a/framework/core/simple_module_core/exceptions.py b/framework/core/simple_module_core/exceptions.py index 92f86641..d2b37563 100644 --- a/framework/core/simple_module_core/exceptions.py +++ b/framework/core/simple_module_core/exceptions.py @@ -14,6 +14,25 @@ class InvalidModuleError(ModuleError): """ +class MissingDependencyError(InvalidModuleError): + """Raised by strict ordering when modules ``depends_on`` modules not loaded. + + ``missing`` holds every ``(module, dependency)`` pair, so one boot reports + the whole gap instead of the first module found (GH #501). + """ + + def __init__(self, missing: list[tuple[str, str]]) -> None: + self.missing = missing + lines = [f" - {name} depends on {dep}, which is not loaded" for name, dep in missing] + super().__init__( + "Module dependencies are missing:\n" + + "\n".join(lines) + + "\n\nInstall and enable the dependencies, or remove the modules that " + "need them (SM_MODULES_ENABLED). An auth provider other than 'users' " + "drops the Users module, so modules depending on it cannot load." + ) + + class CircularDependencyError(ModuleError): """Raised when a circular dependency is detected between modules.""" diff --git a/framework/core/simple_module_core/module_meta.py b/framework/core/simple_module_core/module_meta.py index e3874aee..6ff9d870 100644 --- a/framework/core/simple_module_core/module_meta.py +++ b/framework/core/simple_module_core/module_meta.py @@ -34,6 +34,16 @@ class ModuleMeta: ``view_prefix`` at ``/admin/...`` directly. """ depends_on: list[str] = field(default_factory=list) + """Modules this one needs. Outside development a missing one refuses the + boot (``MissingDependencyError``, GH #501); in development it is logged.""" + optional_depends_on: list[str] = field(default_factory=list) + """Modules to sort after **when they are loaded**; absence is fine. + + For an ordering-only relationship — subscribe after the module that + publishes an event, wrap middleware outside another's — where the module + works without the other one. Sorted exactly like ``depends_on`` when + present, so moving an entry here does not change the boot order. + """ version: str = "1.0.0" requires_framework: str | None = None """PEP 440 specifier for the framework API version this module supports. diff --git a/framework/core/tests/test_discovery_missing_deps.py b/framework/core/tests/test_discovery_missing_deps.py new file mode 100644 index 00000000..3d899352 --- /dev/null +++ b/framework/core/tests/test_discovery_missing_deps.py @@ -0,0 +1,108 @@ +"""Missing ``depends_on`` targets: strict raises them all, lenient warns (GH #501).""" + +from __future__ import annotations + +import logging + +import pytest +from simple_module_core import __main__ as doctor +from simple_module_core.diagnostics import DiagnosticLevel, ModuleDiagnostics +from simple_module_core.discovery import missing_dependencies, topological_sort +from simple_module_core.exceptions import InvalidModuleError, MissingDependencyError +from simple_module_core.module import ModuleBase, ModuleMeta + + +class _Base(ModuleBase): + meta = ModuleMeta(name="Base") + + +class _NeedsUsers(ModuleBase): + meta = ModuleMeta(name="NeedsUsers", depends_on=["Base", "Users"]) + + +class _NeedsTwo(ModuleBase): + meta = ModuleMeta(name="NeedsTwo", depends_on=["Users", "Settings"]) + + +def _set() -> list[ModuleBase]: + return [_NeedsTwo(), _Base(), _NeedsUsers()] + + +class TestMissingDependencies: + def test_lists_every_pair(self): + assert missing_dependencies(_set()) == [ + ("NeedsTwo", "Users"), + ("NeedsTwo", "Settings"), + ("NeedsUsers", "Users"), + ] + + def test_empty_when_complete(self): + assert missing_dependencies([_Base()]) == [] + + +class TestStrictSort: + def test_raises_once_naming_every_pair(self): + with pytest.raises(MissingDependencyError) as exc: + topological_sort(_set(), strict=True) + assert exc.value.missing == missing_dependencies(_set()) + text = str(exc.value) + for line in ( + "NeedsTwo depends on Users", + "NeedsTwo depends on Settings", + "NeedsUsers depends on Users", + ): + assert line in text + + def test_is_an_invalid_module_error(self): + """Callers that already handle strict discovery failures catch it too.""" + assert issubclass(MissingDependencyError, InvalidModuleError) + + def test_complete_sets_sort_normally(self): + assert [m.meta.name for m in topological_sort([_Base()], strict=True)] == ["Base"] + + +class TestLenientSort: + def test_warns_per_pair_and_still_sorts(self, caplog): + with caplog.at_level(logging.WARNING, logger="simple_module_core.discovery"): + ordered = topological_sort(_set()) + names = [m.meta.name for m in ordered] + assert names.index("Base") < names.index("NeedsUsers") + warnings = [r.getMessage() for r in caplog.records] + assert len(warnings) == 3 + assert any("'NeedsUsers' depends on 'Users'" in w for w in warnings) + + +class TestDiagnostic: + def test_sm030_is_a_warning_at_a_dev_boot(self): + findings = [d for d in ModuleDiagnostics().run(_set()) if d.code == "SM030"] + assert len(findings) == 3 + assert {d.level for d in findings} == {DiagnosticLevel.WARNING} + assert {d.module_name for d in findings} == {"NeedsTwo", "NeedsUsers"} + + def test_doctor_reports_it_as_an_error(self, tmp_path, monkeypatch, capsys): + monkeypatch.setattr(doctor, "discover_modules", lambda strict=True: _set()) + monkeypatch.setattr(doctor, "find_env_file", lambda: tmp_path / ".env") + monkeypatch.setattr(doctor, "parse_dotenv", dict) + assert doctor.main() == 1 + err = capsys.readouterr().err + assert "SM030 [ERROR] NeedsUsers" in err + + +class _OrdersAfterBase(ModuleBase): + meta = ModuleMeta(name="Aardvark", optional_depends_on=["Base", "Absent"]) + + +class TestOptionalDependsOn: + def test_orders_after_a_loaded_module(self): + """Alphabetically 'Aardvark' would sort first; the optional edge wins.""" + ordered = [ + m.meta.name for m in topological_sort([_OrdersAfterBase(), _Base()], strict=True) + ] + assert ordered == ["Base", "Aardvark"] + + def test_absence_is_never_missing(self, caplog): + with caplog.at_level(logging.WARNING, logger="simple_module_core.discovery"): + ordered = topological_sort([_OrdersAfterBase()], strict=True) + assert [m.meta.name for m in ordered] == ["Aardvark"] + assert missing_dependencies([_OrdersAfterBase()]) == [] + assert not caplog.records diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index e863ac3b..943fa282 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -109,7 +109,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: modules = select_auth_provider( installed_modules, settings.auth_provider, strict=not settings.is_development ) - modules = topological_sort(modules) + modules = topological_sort(modules, strict=not settings.is_development) logger.info( "Loaded %d module(s): %s", len(modules), diff --git a/modules/api_tokens/api_tokens/module.py b/modules/api_tokens/api_tokens/module.py index 227b981f..33640a04 100644 --- a/modules/api_tokens/api_tokens/module.py +++ b/modules/api_tokens/api_tokens/module.py @@ -27,8 +27,10 @@ class ApiTokensModule(ModuleBase): view_prefix=c.VIEW_PREFIX, admin_view_prefix=c.ADMIN_VIEW_PREFIX, # Auth owns the resolver chain this module appends to; Users owns the - # accounts a personal token acts for; Settings stores the lifetime cap. - depends_on=[c.MODULE_AUTH, c.MODULE_USERS, c.MODULE_SETTINGS], + # accounts a personal token acts for. Settings (the lifetime cap's + # editor) is ordering only: the framework hydrates settings (#500). + depends_on=[c.MODULE_AUTH, c.MODULE_USERS], + optional_depends_on=[c.MODULE_SETTINGS], ) def register_settings(self, app: FastAPI) -> None: diff --git a/modules/background_tasks/background_tasks/module.py b/modules/background_tasks/background_tasks/module.py index 3b0d6c5f..318bedd8 100644 --- a/modules/background_tasks/background_tasks/module.py +++ b/modules/background_tasks/background_tasks/module.py @@ -73,7 +73,7 @@ class BackgroundTasksModule(ModuleBase): name=MODULE_DISPLAY_NAME, route_prefix=API_PREFIX, view_prefix=VIEW_PREFIX, - depends_on=[_MODULE_SETTINGS], + optional_depends_on=[_MODULE_SETTINGS], i18n_audience="admin", ) diff --git a/modules/background_tasks/background_tasks/outbox_worker.py b/modules/background_tasks/background_tasks/outbox_worker.py index d7541fef..bd8c26a0 100644 --- a/modules/background_tasks/background_tasks/outbox_worker.py +++ b/modules/background_tasks/background_tasks/outbox_worker.py @@ -109,6 +109,8 @@ def build_worker_bus() -> EventBus: bus = EventBus() bus.durable_transport = WorkerOutboxTransport() + # Lenient on purpose: the worker delivers whatever is installed, and a + # module set the web process refuses is reported there, not here. for module in topological_sort(discover_modules()): try: _register(module, bus) diff --git a/modules/file_storage/file_storage/module.py b/modules/file_storage/file_storage/module.py index 14e7900d..268737e1 100644 --- a/modules/file_storage/file_storage/module.py +++ b/modules/file_storage/file_storage/module.py @@ -66,7 +66,7 @@ class FileStorageModule(ModuleBase): view_prefix=constants.ROUTE_PREFIX_VIEW, # Kept after Settings so the module order is unchanged; registering # settings no longer needs it (the framework owns the registry, #500). - depends_on=[constants._MODULE_SETTINGS], + optional_depends_on=[constants._MODULE_SETTINGS], i18n_audience="admin", ) diff --git a/modules/keycloak/keycloak/module.py b/modules/keycloak/keycloak/module.py index e5d08541..ab7a1754 100644 --- a/modules/keycloak/keycloak/module.py +++ b/modules/keycloak/keycloak/module.py @@ -22,7 +22,9 @@ class KeycloakModule(ModuleBase): name="Keycloak", route_prefix="/api/keycloak", view_prefix="/keycloak", - depends_on=[_MODULE_DEPENDENCY_AUTH, _MODULE_DEPENDENCY_SETTINGS], + depends_on=[_MODULE_DEPENDENCY_AUTH], + # Ordering only: settings registration is the framework's (#500). + optional_depends_on=[_MODULE_DEPENDENCY_SETTINGS], ) _is_auth_provider = True diff --git a/modules/keycloak/tests/keycloak_test_modules.py b/modules/keycloak/tests/keycloak_test_modules.py new file mode 100644 index 00000000..1d292756 --- /dev/null +++ b/modules/keycloak/tests/keycloak_test_modules.py @@ -0,0 +1,19 @@ +"""The module set a Keycloak test app boots with. + +Selecting ``keycloak`` drops ``Users``, and outside development a module whose +``depends_on`` is not loaded refuses to boot (GH #501). ``permissions`` (role +pages) and ``api_tokens`` (personal tokens need the owner's roles) still depend +on ``Users``, so a Keycloak install leaves them out — and so do these tests, +rather than booting in a lenient mode production never uses. +""" + +from __future__ import annotations + +from simple_module_core.discovery import discover_modules + +_USERS = "Users" + + +def keycloak_modules_enabled() -> list[str]: + """Every installed module except those that need ``Users``.""" + return [m.meta.name for m in discover_modules() if _USERS not in m.meta.depends_on] diff --git a/modules/keycloak/tests/test_keycloak_module.py b/modules/keycloak/tests/test_keycloak_module.py index 6d715039..98492d48 100644 --- a/modules/keycloak/tests/test_keycloak_module.py +++ b/modules/keycloak/tests/test_keycloak_module.py @@ -10,7 +10,8 @@ def test_keycloak_module_meta(): mod = KeycloakModule() assert mod.meta.name == "Keycloak" - assert mod.meta.depends_on == ["Auth", "Settings"] + assert mod.meta.depends_on == ["Auth"] + assert mod.meta.optional_depends_on == ["Settings"] assert mod._is_auth_provider is True diff --git a/modules/keycloak/tests/test_keycloak_strict_boot.py b/modules/keycloak/tests/test_keycloak_strict_boot.py new file mode 100644 index 00000000..3431c4ce --- /dev/null +++ b/modules/keycloak/tests/test_keycloak_strict_boot.py @@ -0,0 +1,56 @@ +"""A Keycloak app boots with its directory, and refuses modules that need Users. + +Selecting ``keycloak`` drops ``Users``. Outside development the host sorts +modules strictly, so ``permissions`` and ``api_tokens`` (which still depend on +``Users``) fail the boot in one error naming both, instead of booting against +empty ``users_*`` tables (GH #501). +""" + +from __future__ import annotations + +import pytest +from keycloak.directory import KeycloakDirectory +from keycloak_test_modules import keycloak_modules_enabled +from simple_module_core.exceptions import MissingDependencyError +from simple_module_core.user_directory import get_user_directory +from simple_module_hosting.app_builder import create_app +from simple_module_hosting.settings import Settings + + +def _settings(environment: str, modules_enabled: list[str] | None = None) -> Settings: + return Settings( + database_url="sqlite+aiosqlite:///:memory:", + environment=environment, + secret_key="test-secret-key", + multi_tenant=False, + auth_provider="keycloak", + modules_enabled=modules_enabled, + ) + + +def test_non_development_refuses_every_module_that_needs_users(): + with pytest.raises(MissingDependencyError) as exc: + create_app(_settings("testing")) + assert ("Permissions", "Users") in exc.value.missing + assert ("ApiTokens", "Users") in exc.value.missing + assert {dep for _name, dep in exc.value.missing} == {"Users"} + + +def test_without_them_the_app_boots_with_the_keycloak_directory(): + app = create_app(_settings("testing", keycloak_modules_enabled())) + assert isinstance(get_user_directory(app), KeycloakDirectory) + names = {m.meta.name for m in app.state.sm.modules} + assert {"AuditLog", "Dashboard", "FileStorage"} <= names + assert not names & {"Users", "Permissions", "ApiTokens"} + + +def test_development_only_warns(monkeypatch, caplog): + # The dev-boot pass writes the frontend's generated files into the source + # tree; with this module set that would clobber the workspace's copies. + import simple_module_hosting.app_builder as app_builder + + monkeypatch.setattr(app_builder, "run_dev_boot", lambda *a, **kw: None) + app = create_app(_settings("development")) + names = {m.meta.name for m in app.state.sm.modules} + assert "Permissions" in names + assert any("'Permissions' depends on 'Users'" in r.getMessage() for r in caplog.records) diff --git a/modules/keycloak/tests/test_keycloak_tenant_hardening.py b/modules/keycloak/tests/test_keycloak_tenant_hardening.py index 0e578093..99a7e440 100644 --- a/modules/keycloak/tests/test_keycloak_tenant_hardening.py +++ b/modules/keycloak/tests/test_keycloak_tenant_hardening.py @@ -15,6 +15,7 @@ from keycloak.provider import KeycloakAuthProvider from keycloak.settings import KeycloakSettings from keycloak.state import KeycloakState +from keycloak_test_modules import keycloak_modules_enabled from starlette.requests import Request @@ -105,6 +106,7 @@ def test_the_host_wires_the_provider_to_the_live_settings(): secret_key="test-secret-key", multi_tenant=False, auth_provider="keycloak", + modules_enabled=keycloak_modules_enabled(), ) ) provider = app.state.auth.auth_provider diff --git a/modules/keycloak/tests/test_keycloak_views.py b/modules/keycloak/tests/test_keycloak_views.py index beff27cc..cabdb77e 100644 --- a/modules/keycloak/tests/test_keycloak_views.py +++ b/modules/keycloak/tests/test_keycloak_views.py @@ -13,6 +13,7 @@ import httpx import pytest +from keycloak_test_modules import keycloak_modules_enabled from simple_module_hosting.settings import Settings _INERTIA_HEADERS = {"X-Inertia": "true", "X-Inertia-Version": "1.0"} @@ -39,6 +40,7 @@ async def _build_keycloak_app(): secret_key="test-secret-key", multi_tenant=False, auth_provider="keycloak", + modules_enabled=keycloak_modules_enabled(), ) application = create_app(settings) diff --git a/modules/site_lock/README.md b/modules/site_lock/README.md index fd6eabc8..0f50d69f 100644 --- a/modules/site_lock/README.md +++ b/modules/site_lock/README.md @@ -47,7 +47,8 @@ That ordering is the whole point — an anonymous visitor gets the gate rather than a redirect to the login page, so a locked site never reveals that it has one. -It achieves that ordering by declaring `depends_on=["Settings", "Auth"]`: +It achieves that ordering by declaring `depends_on=["Auth"]` (and +`optional_depends_on=["Settings"]`, which keeps the boot order unchanged): modules are installed in topological order and Starlette's `add_middleware` is LIFO, so sorting after `Auth` makes this middleware wrap outermost. diff --git a/modules/site_lock/site_lock/module.py b/modules/site_lock/site_lock/module.py index 01d27cca..8395aac1 100644 --- a/modules/site_lock/site_lock/module.py +++ b/modules/site_lock/site_lock/module.py @@ -20,7 +20,8 @@ class SiteLockModule(ModuleBase): # no longer needs it, #500); ``Auth`` so this module sorts *after* auth and its # middleware therefore wraps outermost, executing before # AuthMiddleware. Both are load-bearing — see the module README. - depends_on=[c.MODULE_SETTINGS, c.MODULE_AUTH], + depends_on=[c.MODULE_AUTH], + optional_depends_on=[c.MODULE_SETTINGS], ) def register_settings(self, app: FastAPI) -> None: diff --git a/modules/tenants/tenants/module.py b/modules/tenants/tenants/module.py index fe17ac00..c1ba6270 100644 --- a/modules/tenants/tenants/module.py +++ b/modules/tenants/tenants/module.py @@ -48,8 +48,10 @@ class TenantsModule(ModuleBase): view_prefix="/tenants", admin_view_prefix="/admin/tenants", # Auth: its middleware must have set request.state.user before the - # resolver runs. Settings: register_module_settings. - depends_on=[c._MODULE_AUTH, c._MODULE_SETTINGS], + # resolver runs. Settings: ordering only, since registering settings + # is the framework's (#500). + depends_on=[c._MODULE_AUTH], + optional_depends_on=[c._MODULE_SETTINGS], ) def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) -> None: diff --git a/modules/users/users/module.py b/modules/users/users/module.py index 5af86270..8fc3386b 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -51,7 +51,8 @@ class UsersModule(ModuleBase): # belongs with the other admin screens. One view_prefix cannot # express both, hence the second router. admin_view_prefix="/admin/users", - depends_on=[_MODULE_DEPENDENCY_AUTH, _MODULE_DEPENDENCY_SETTINGS], + depends_on=[_MODULE_DEPENDENCY_AUTH], + optional_depends_on=[_MODULE_DEPENDENCY_SETTINGS], ) _is_auth_provider = True diff --git a/skills/simple-module-creating/SKILL.md b/skills/simple-module-creating/SKILL.md index ef1a82e3..4f997610 100644 --- a/skills/simple-module-creating/SKILL.md +++ b/skills/simple-module-creating/SKILL.md @@ -78,7 +78,8 @@ class OrdersModule(ModuleBase): name="Orders", # PascalCase, must be unique route_prefix="/api/orders", view_prefix="/orders", - depends_on=[], # other module names (PascalCase) + depends_on=[], # modules this one needs (PascalCase); missing → prod boot fails + optional_depends_on=[], # ordering only: sort after these when loaded ) ``` diff --git a/skills/simple-module-doctor/SKILL.md b/skills/simple-module-doctor/SKILL.md index c9d679bf..cbc0e0df 100644 --- a/skills/simple-module-doctor/SKILL.md +++ b/skills/simple-module-doctor/SKILL.md @@ -1,13 +1,13 @@ --- name: simple-module-doctor -description: Use when interpreting a simple_module_python diagnostic code (SM001–SM029) printed at boot or by the diagnostics runner — what the code means, whether it's blocking, and the concrete fix. Triggers on any "SMnnn" code in logs, "InvalidModuleError", "module silently doesn't load", or "production failed boot". +description: Use when interpreting a simple_module_python diagnostic code (SM001–SM030) printed at boot or by the diagnostics runner — what the code means, whether it's blocking, and the concrete fix. Triggers on any "SMnnn" code in logs, "InvalidModuleError", "module silently doesn't load", or "production failed boot". --- # simple_module_python: diagnostics `make doctor` runs `python -m simple_module_core`: it discovers modules **strictly**, runs the diagnostics, prints findings to stderr (`✓ No module diagnostics issues found` when clean), and exits `1` if any ERROR-level finding was reported. -The same diagnostics also run at host boot, but **only in development** (`SM_ENVIRONMENT == development`) — they print to the boot logs and `raise SystemExit` if there's any ERROR. **In production the diagnostics pass is skipped**; what protects prod is that module *discovery* runs strictly (any bad entry point — broken import, missing `meta`, non-`ModuleBase` class — raises and fails the boot). So "errors fail prod boot" is true for discovery-level breakage (surfaced as **SM001**), not for the per-finding checks below, which you'll catch via `make doctor` / dev boot before you ever deploy. +The same diagnostics also run at host boot, but **only in development** (`SM_ENVIRONMENT == development`) — they print to the boot logs and `raise SystemExit` if there's any ERROR. **In production the diagnostics pass is skipped**; what protects prod is that module *discovery* runs strictly (any bad entry point — broken import, missing `meta`, non-`ModuleBase` class — or a missing `depends_on` target raises and fails the boot). So "errors fail prod boot" is true for discovery-level breakage (surfaced as **SM001**), not for the per-finding checks below, which you'll catch via `make doctor` / dev boot before you ever deploy. You can also call `run_diagnostics(...)` from `simple_module_core` programmatically (e.g. in a CI job) to surface issues without booting the full app — pass `migration_state=` / `module_tables=` / `migrated_tables=` to additionally get **SM010/SM011** (the migration checks below), and `i18n_supported_locales=` / `i18n_default_locale=` to get the locale checks. @@ -35,6 +35,7 @@ You can also call `run_diagnostics(...)` from `simple_module_core` programmatica | **SM021** | WARNING | No auth provider module is installed | Install an auth provider module (e.g. `simple-module-users` or `simple-module-keycloak`) | | **SM028** | WARNING | A module's `tasks.py` imports `torch`, `tensorflow`, `ultralytics`, `osgeo`, `rasterio`, `cv2` or `sklearn` at module level | Import the package inside the task function, so only a worker that runs the task loads it | | **SM029** | INFO | A module overrides `db_extensions()` (Postgres extensions like `ltree`) but the database is SQLite, which has none; `ensure_db_extensions` raises `UnsupportedDatabaseError` at migrate time | Point `SM_DATABASE_URL` at PostgreSQL | +| **SM030** | WARNING (dev boot) / ERROR (`make doctor`) | A module's `depends_on` names a module that is not loaded — often `Users` under the `keycloak` provider. Outside development the boot raises `MissingDependencyError` listing every pair | Install/enable the dependency or leave the dependent module out; move an ordering-only entry to `optional_depends_on` | `SM002`, `SM005`, `SM006` don't exist in the implementation — they're reserved/retired and never emitted. `SM012` is the one code that doesn't come from the core diagnostics runner: it's raised from the hosting layer (`check_settings_registration`, after Phase 4 settings registration) and so is only ever seen at **dev boot**, not via `make doctor`. From 78b6f3873f20d38f31e9bd4ed6395a3b2e4e3f28 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 14:40:56 +0200 Subject: [PATCH 7/9] refactor(optimize): reuse missing_dependencies, share the api_tokens owner check, trim stale docstrings Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../diagnostics/_dependencies.py | 20 +++++++----------- modules/api_tokens/api_tokens/owner.py | 21 +++++++++++++++++++ modules/api_tokens/api_tokens/resolver.py | 10 ++++----- modules/api_tokens/api_tokens/service.py | 5 ++--- modules/audit_log/audit_log/activity.py | 6 +----- modules/audit_log/audit_log/resolve.py | 2 +- modules/auth/auth/contracts/schemas.py | 2 +- modules/users/users/audit.py | 4 +--- modules/users/users/directory.py | 2 -- 9 files changed, 38 insertions(+), 34 deletions(-) create mode 100644 modules/api_tokens/api_tokens/owner.py diff --git a/framework/core/simple_module_core/diagnostics/_dependencies.py b/framework/core/simple_module_core/diagnostics/_dependencies.py index fede89b9..0da9b3d3 100644 --- a/framework/core/simple_module_core/diagnostics/_dependencies.py +++ b/framework/core/simple_module_core/diagnostics/_dependencies.py @@ -12,6 +12,7 @@ from typing import TYPE_CHECKING from simple_module_core.diagnostics._types import Diagnostic, DiagnosticLevel +from simple_module_core.discovery import missing_dependencies if TYPE_CHECKING: from simple_module_core.module import ModuleBase @@ -19,24 +20,17 @@ CODE = "SM030" -def check_missing_dependencies( - modules: list[ModuleBase], level: DiagnosticLevel = DiagnosticLevel.WARNING -) -> list[Diagnostic]: - names = {m.meta.name for m in modules} +def check_missing_dependencies(modules: list[ModuleBase]) -> list[Diagnostic]: return [ Diagnostic( - level=level, + level=DiagnosticLevel.WARNING, code=CODE, message=( - f"Module '{mod.meta.name}' depends on '{dep}', which is not loaded; " + f"Module '{name}' depends on '{dep}', which is not loaded; " "a non-development boot refuses to start" ), - module_name=mod.meta.name, - suggestion=( - f"Install and enable '{dep}', or leave '{mod.meta.name}' out (SM_MODULES_ENABLED)" - ), + module_name=name, + suggestion=f"Install and enable '{dep}', or leave '{name}' out (SM_MODULES_ENABLED)", ) - for mod in modules - for dep in mod.meta.depends_on - if dep not in names + for name, dep in missing_dependencies(modules) ] diff --git a/modules/api_tokens/api_tokens/owner.py b/modules/api_tokens/api_tokens/owner.py new file mode 100644 index 00000000..b0847958 --- /dev/null +++ b/modules/api_tokens/api_tokens/owner.py @@ -0,0 +1,21 @@ +"""The one definition of an owner a personal token can act for.""" + +from __future__ import annotations + +from typing import Any + +from simple_module_core.user_directory import DirectoryUser, UserDirectory + + +async def usable_owner(directory: UserDirectory, db: Any, owner_id: str) -> DirectoryUser | None: + """The directory entry for *owner_id*, or ``None`` when it cannot back a token. + + Usable means found, active **and** with roles known: under a provider that + keeps roles in the IdP (Keycloak) a token would be dead on arrival, since + the roles are what its scopes are intersected with and "none" is not what + the owner holds. + """ + user = (await directory.get_many(db, [owner_id], with_roles=True)).get(owner_id) + if user is None or not user.is_active or user.roles is None: + return None + return user diff --git a/modules/api_tokens/api_tokens/resolver.py b/modules/api_tokens/api_tokens/resolver.py index 9741cc54..883a4426 100644 --- a/modules/api_tokens/api_tokens/resolver.py +++ b/modules/api_tokens/api_tokens/resolver.py @@ -33,6 +33,7 @@ TokenKind, ) from api_tokens.models import ApiToken +from api_tokens.owner import usable_owner from api_tokens.tokens import hash_token, is_well_formed logger = logging.getLogger(__name__) @@ -57,17 +58,14 @@ async def _personal(db: Any, directory: UserDirectory, token: ApiToken) -> UserC if token.owner_user_id is None: return None owner_id = str(token.owner_user_id) - user = (await directory.get_many(db, [owner_id], with_roles=True)).get(owner_id) - # Unknown roles (an IdP that keeps them, like Keycloak) refuse the token - # rather than authenticate an owner with none: the roles are what the - # scopes are intersected with, and "none" is not what the owner holds. - if user is None or not user.is_active or user.roles is None: + user = await usable_owner(directory, db, owner_id) + if user is None: return None principal = UserContext( id=owner_id, email=user.email, name=user.label, - roles=list(user.roles), + roles=list(user.roles or ()), auth_kind=AUTH_KIND_PAT, scopes=list(token.scopes), ) diff --git a/modules/api_tokens/api_tokens/service.py b/modules/api_tokens/api_tokens/service.py index 13f531eb..4aa0e36e 100644 --- a/modules/api_tokens/api_tokens/service.py +++ b/modules/api_tokens/api_tokens/service.py @@ -21,6 +21,7 @@ from api_tokens.constants import FORBIDDEN_SERVICE_ROLES, TokenKind from api_tokens.contracts.schemas import TokenView from api_tokens.models import ApiToken +from api_tokens.owner import usable_owner from api_tokens.tokens import generate @@ -92,9 +93,7 @@ async def has_usable_account(self, user_id: uuid.UUID) -> bool: minted for the id would be dead on arrival, since the resolver refuses a personal token whose owner's roles it cannot read. """ - found = await self.directory.get_many(self.db, [str(user_id)], with_roles=True) - owner = found.get(str(user_id)) - return owner is not None and owner.is_active and owner.roles is not None + return await usable_owner(self.directory, self.db, str(user_id)) is not None async def revoke_all_for_owner(self, owner_user_id: uuid.UUID) -> int: """Revoke every live personal token *owner_user_id* holds, in any tenant.""" diff --git a/modules/audit_log/audit_log/activity.py b/modules/audit_log/audit_log/activity.py index eb79e196..8ec062d7 100644 --- a/modules/audit_log/audit_log/activity.py +++ b/modules/audit_log/audit_log/activity.py @@ -37,11 +37,7 @@ async def recent_for_actor( refs = [(entry.entity_type, entry.entity_id) for entry in page.items] # The reader's grants reach the resolvers, which skip entity types # whose owner gated their names (GH #300), exactly as the audit screen. - labels = ( - await resolve_entity_labels(db, self._app.state.sm.audit_links, refs, permissions) - if refs - else {} - ) + labels = await resolve_entity_labels(db, self._app.state.sm.audit_links, refs, permissions) return [ ActivityItem( at=entry.created_at, diff --git a/modules/audit_log/audit_log/resolve.py b/modules/audit_log/audit_log/resolve.py index a6436bc7..c5005177 100644 --- a/modules/audit_log/audit_log/resolve.py +++ b/modules/audit_log/audit_log/resolve.py @@ -57,7 +57,7 @@ async def resolve_actors( ``audit_log.view`` therefore does imply "may see who acted"; that is stated in docs/modules/audit_log.md so it is granted knowingly. See GH #300. """ - return await resolve_user_labels(directory, db, [uid for uid in user_ids if uid]) + return await resolve_user_labels(directory, db, user_ids) def actor_filter(directory: UserDirectory, term: str) -> Any | None: diff --git a/modules/auth/auth/contracts/schemas.py b/modules/auth/auth/contracts/schemas.py index 840407a0..b2842344 100644 --- a/modules/auth/auth/contracts/schemas.py +++ b/modules/auth/auth/contracts/schemas.py @@ -71,7 +71,7 @@ def from_user( auth_kind: str = AUTH_KIND_SESSION, scopes: list[str] | None = None, ) -> UserContext: - """Build a UserContext from a users.models.User with eagerly-loaded roles. + """Build a UserContext from an :class:`AccountLike` with eagerly-loaded roles. Duck-typed to avoid importing users.models at runtime — any object exposing .id, .email, .full_name, .roles[*].name works. The user row diff --git a/modules/users/users/audit.py b/modules/users/users/audit.py index 10fa9187..b426afd5 100644 --- a/modules/users/users/audit.py +++ b/modules/users/users/audit.py @@ -2,9 +2,7 @@ An audit entry stores ``User`` and a uuid. Turning that back into something a reader can act on has two halves and both belong here: *where* the record -lives (the admin route) and *what it is called*. Only this module knows the -second — a user is named by ``full_name``, or by the email while an invite is -still outstanding and no name exists yet. +lives (the admin route) and *what it is called* (``DirectoryUser.label``). The resolver is deliberately batched: an audit page is 50 rows and a busy afternoon is 50 edits to the same handful of accounts, so it takes every id on diff --git a/modules/users/users/directory.py b/modules/users/users/directory.py index fd17248b..0c573508 100644 --- a/modules/users/users/directory.py +++ b/modules/users/users/directory.py @@ -61,8 +61,6 @@ async def get_many( self, db: Any, ids: Collection[str], *, with_roles: bool = False ) -> dict[str, DirectoryUser]: parsed = parse_user_ids(ids) - if not parsed: - return {} found: dict[str, DirectoryUser] = {} for chunk in chunked(list(parsed)): rows = (await db.execute(select(*_COLUMNS).where(User.id.in_(chunk)))).all() From 82f15e0ad5995e1ad37f4fc0fac7ceb462a22780 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 14:51:22 +0200 Subject: [PATCH 8/9] docs: review round 1 fixes (changelog entry, docstring wrap) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 3 +++ framework/core/simple_module_core/discovery.py | 9 +++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c31dfddc..634b5dc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,9 @@ All notable changes to this project are documented in this file. The format is b `PermissionService` and `ApiTokenService` accept a directory (defaults to the null directory). With no directory, the audit Actor search matches the term as an exact actor id (#501). +- `file_storage.resolve.resolve_uploader_labels` is now core's + `resolve_user_labels(directory, db, ids)` (the directory is the first + argument), and `users.audit.resolve_user_labels` delegates to it (#501). - `smpy` module catalog: `background_tasks` requires `settings` instead of `users` and `dashboard` requires nothing; a test pins that every entry's `requires` covers its module's `depends_on` (#501). diff --git a/framework/core/simple_module_core/discovery.py b/framework/core/simple_module_core/discovery.py index 744e68ee..82e44219 100644 --- a/framework/core/simple_module_core/discovery.py +++ b/framework/core/simple_module_core/discovery.py @@ -209,10 +209,11 @@ def topological_sort(modules: Sequence[ModuleBase], *, strict: bool = False) -> ``optional_depends_on`` orders the same way when present and is never missing. A ``depends_on`` entry naming a module that is not loaded is - logged and ignored; with ``strict`` every such pair is collected and raised at once as - :class:`MissingDependencyError`. The host passes ``strict`` outside - development: a module whose dependency is absent would otherwise boot and - fail later, or silently query another module's empty tables (GH #501). + logged and ignored; with ``strict`` every such pair is collected and + raised at once as :class:`MissingDependencyError`. The host passes + ``strict`` outside development: a module whose dependency is absent would + otherwise boot and fail later, or silently query another module's empty + tables (GH #501). Raises ``CircularDependencyError`` if a cycle is detected. """ From cb480e67da894745bc212cd8505c817317bfe314 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 21 Oct 2026 14:58:15 +0200 Subject: [PATCH 9/9] fix: address code review findings (round 1, pass 1) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 13 ++- docs/framework/user-directory.md | 12 ++- docs/modules/api_tokens.md | 2 +- docs/modules/keycloak.md | 9 +- .../specs/2026-10-21-user-directory-design.md | 5 +- .../core/simple_module_core/user_directory.py | 15 ++-- .../core/tests/test_core_user_directory.py | 7 +- modules/api_tokens/README.md | 4 +- modules/api_tokens/api_tokens/constants.py | 6 +- modules/api_tokens/api_tokens/module.py | 12 +-- modules/api_tokens/pyproject.toml | 8 +- modules/keycloak/keycloak/directory.py | 4 +- .../keycloak/tests/keycloak_test_modules.py | 5 +- .../tests/test_keycloak_api_tokens.py | 88 +++++++++++++++++++ .../keycloak/tests/test_keycloak_directory.py | 7 ++ .../tests/test_keycloak_strict_boot.py | 11 +-- modules/users/tests/test_users_directory.py | 8 ++ modules/users/users/directory.py | 4 +- uv.lock | 4 +- 19 files changed, 176 insertions(+), 48 deletions(-) create mode 100644 modules/keycloak/tests/test_keycloak_api_tokens.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 634b5dc8..16031885 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,7 +31,9 @@ All notable changes to this project are documented in this file. The format is b `api_tokens` and `permissions` read users through it instead of importing `users.models`, so actor names and search, dashboard tiles and uploader names now work under Keycloak instead of querying the empty `users_user` - table. See `docs/framework/user-directory.md`. + table. `get_many` returns an entry for every caller spelling of a known id + (`parse_user_ids` maps a uuid to all of them). See + `docs/framework/user-directory.md`. - Activity feed protocol (#501): `simple_module_core.activity_feed`. `audit_log` installs it; the users edit page's *Recent activity* card reads it, which removes the `users` ↔ `audit_log` import cycle. @@ -39,7 +41,11 @@ All notable changes to this project are documented in this file. The format is b ### Changed - `audit_log` and `dashboard` no longer depend on `Users` (meta `depends_on` and package dependency); `background_tasks` drops its unused `Users` - dependency (#501). `permissions` (role pages) and `api_tokens` still do. + dependency (#501). `api_tokens` moves `Users` to `optional_depends_on` and + its package depends on `fastapi-users-db-sqlalchemy` instead of + `simple_module_users`, so a Keycloak install keeps service keys (personal + tokens are refused there). `permissions` (role pages) still depends on + `Users`. - **Behaviour change (#501):** dashboard user figures the auth provider cannot give are `null` in `/api/dashboard/stats` and their tile is hidden (with no provider, all three; under Keycloak, `users_created_this_month`). Personal @@ -63,8 +69,7 @@ All notable changes to this project are documented in this file. The format is b `(module, dependency)` pair, instead of a warning and a module running against tables nobody owns. Development still logs it; `make doctor` reports it as the new `SM030` error. Selecting `keycloak` drops `Users`, so - a Keycloak install must leave out `permissions` and `api_tokens` - (`SM_MODULES_ENABLED`). The outbox worker still sorts leniently. + a Keycloak install must leave out `permissions` (`SM_MODULES_ENABLED`). The outbox worker still sorts leniently. - New `ModuleMeta.optional_depends_on`: modules to sort after when loaded, never required (#501). `users`, `tenants`, `file_storage`, `background_tasks`, `site_lock`, `api_tokens` and `keycloak` move their diff --git a/docs/framework/user-directory.md b/docs/framework/user-directory.md index 0872dd75..488a6f57 100644 --- a/docs/framework/user-directory.md +++ b/docs/framework/user-directory.md @@ -35,7 +35,9 @@ Name a user with `DirectoryUser.label` (the full name, else the email). It is the one naming rule, so a row is never called two things on one screen. `resolve_user_labels(directory, db, ids)` maps a page of ids to labels in one call; `parse_user_ids` and `chunked` help an implementation key and bound its -queries. +queries. `get_many` is keyed by the caller's strings: `parse_user_ids` maps each +uuid to every spelling the caller passed (`ABC…`, `abc…`, unhyphenated), and an +implementation returns an entry under each one. ## Providers @@ -63,9 +65,11 @@ SQLite, where SQLAlchemy's `Uuid` is 32 bare hex characters. | `api_tokens` | owner checks and emails; personal-token resolution via `get_many(with_roles=True)` | | `permissions` | the user side of the grants screens (`get_many(with_roles=True)`, `search`) | -`permissions` role pages and `api_tokens` still depend on `Users`: roles have -no directory protocol yet (the `Role` table is read from `users.contracts.roles`), -and a personal token needs the owner's roles. +`permissions` role pages still depend on `Users`: roles have no directory +protocol yet (the `Role` table is read from `users.contracts.roles`). +`api_tokens` lists `Users` in `optional_depends_on` only: a personal token +needs the owner's roles, so under Keycloak it is refused while service keys +keep working. ## Activity feed diff --git a/docs/modules/api_tokens.md b/docs/modules/api_tokens.md index c98cf251..4615d1b6 100644 --- a/docs/modules/api_tokens.md +++ b/docs/modules/api_tokens.md @@ -13,7 +13,7 @@ rest, shown once, and revocable with effect on the next request. | `route_prefix` | `/api/api_tokens` | | `view_prefix` | `/api-tokens` | | `admin_view_prefix` | `/admin/api-tokens` | -| `depends_on` | `Auth`, `Users` (optional: `Settings`) | +| `depends_on` | `Auth` (optional: `Users`, `Settings`) | Installed in this repository's host. It is **not** part of the `smpy new` scaffold; add `simple_module_api_tokens` to a host's dependencies to enable it, diff --git a/docs/modules/keycloak.md b/docs/modules/keycloak.md index 3b653b1d..28d97d13 100644 --- a/docs/modules/keycloak.md +++ b/docs/modules/keycloak.md @@ -110,10 +110,11 @@ The cache backs this module's [user directory](/framework/user-directory) the audit log, dashboard, file storage and the permissions grants screen name and count Keycloak users. It knows only users who have signed in at least once; roles are `None` (they live in the IdP) and `created_this_month` is `None`. -Modules that still need the `users` tables — `permissions` (role pages) and -`api_tokens` (personal tokens need the owner's roles) — declare -`depends_on=["Users"]`, so leave them out of a Keycloak install -(`SM_MODULES_ENABLED`). +`permissions` (role pages) still needs the `users` tables and declares +`depends_on=["Users"]`, so leave it out of a Keycloak install +(`SM_MODULES_ENABLED`). `api_tokens` only orders after `Users`: under Keycloak +its service keys work and personal tokens are refused (the owner's roles live +in the IdP). ## Menu diff --git a/docs/superpowers/specs/2026-10-21-user-directory-design.md b/docs/superpowers/specs/2026-10-21-user-directory-design.md index fe7bb843..f246f2e3 100644 --- a/docs/superpowers/specs/2026-10-21-user-directory-design.md +++ b/docs/superpowers/specs/2026-10-21-user-directory-design.md @@ -81,7 +81,10 @@ name check; a missing feed returns `None` (keeps "no card" vs "no activity"). - `file_storage`: delete `resolve.py`'s copy, use the directory. - `api_tokens`: owner existence/active and emails via `get_many`; PAT resolution via `get_many(with_roles=True)`. Personal tokens stay refused - when roles are unknown (Keycloak). Keeps `depends_on=[Users]`. + when roles are unknown (Keycloak). `Users` moves to `optional_depends_on` + (ordering only), so a strict Keycloak boot keeps `api_tokens` and its + service keys; the package depends on `fastapi-users-db-sqlalchemy` (for + `GUID`) instead of `simple_module_users`. - `permissions`: user side (`get_many(with_roles=True)`, `search`); role pages keep `Users` (import `users.contracts`, not `users.models`); `ADMIN_ROLE` from core instead of `users.constants`. diff --git a/framework/core/simple_module_core/user_directory.py b/framework/core/simple_module_core/user_directory.py index d04fc6ea..b4b6ceea 100644 --- a/framework/core/simple_module_core/user_directory.py +++ b/framework/core/simple_module_core/user_directory.py @@ -135,19 +135,22 @@ def get_user_directory(app: Any) -> UserDirectory: return directory if directory is not None else _NULL_DIRECTORY -def parse_user_ids(ids: Collection[str]) -> dict[uuid.UUID, str]: - """Map each id that parses as a uuid to the caller's own spelling of it. +def parse_user_ids(ids: Collection[str]) -> dict[uuid.UUID, list[str]]: + """Map each id that parses as a uuid to every caller spelling of it. Implementations key their answer by the caller's strings, so an uppercase - or unhyphenated id still finds its row. Anything else (a worker name, an - ``svc:`` principal, ``None``) belongs to another id space and is dropped. + or unhyphenated id still finds its row, and two spellings of one id each + get an entry. Anything else (a worker name, an ``svc:`` principal, + ``None``) belongs to another id space and is dropped. """ - parsed: dict[uuid.UUID, str] = {} + parsed: dict[uuid.UUID, list[str]] = {} for raw in ids: try: - parsed[uuid.UUID(raw)] = raw + keys = parsed.setdefault(uuid.UUID(raw), []) except (ValueError, AttributeError, TypeError): continue + if raw not in keys: + keys.append(raw) return parsed diff --git a/framework/core/tests/test_core_user_directory.py b/framework/core/tests/test_core_user_directory.py index 254bbac2..5fbce939 100644 --- a/framework/core/tests/test_core_user_directory.py +++ b/framework/core/tests/test_core_user_directory.py @@ -94,7 +94,12 @@ class TestHelpers: def test_parse_keeps_the_callers_spelling(self): uid = uuid.uuid4() upper = str(uid).upper() - assert parse_user_ids([upper, "celery-worker", "", "svc:1"]) == {uid: upper} + assert parse_user_ids([upper, "celery-worker", "", "svc:1"]) == {uid: [upper]} + + def test_parse_keeps_every_spelling_of_one_id(self): + uid = uuid.uuid4() + spellings = [str(uid), str(uid).upper(), uid.hex, f"{{{uid}}}", str(uid)] + assert parse_user_ids(spellings) == {uid: spellings[:4]} def test_parse_tolerates_non_strings(self): assert parse_user_ids([None, 3]) == {} diff --git a/modules/api_tokens/README.md b/modules/api_tokens/README.md index 8545e53e..7fddd4f3 100644 --- a/modules/api_tokens/README.md +++ b/modules/api_tokens/README.md @@ -19,7 +19,9 @@ pip install simple_module_api_tokens ``` Add `simple_module_api_tokens` to your host's dependencies and generate a -migration. It needs the `auth`, `users` and `settings` modules. +migration. It needs the `auth` module. Under `keycloak` (no `users`) service +keys work and personal tokens are refused, since the owner's roles live in the +IdP. ## Usage diff --git a/modules/api_tokens/api_tokens/constants.py b/modules/api_tokens/api_tokens/constants.py index fcc76e99..9df6c2bb 100644 --- a/modules/api_tokens/api_tokens/constants.py +++ b/modules/api_tokens/api_tokens/constants.py @@ -11,9 +11,9 @@ MODULE_PACKAGE: Final = "api_tokens" LOCALE_NAMESPACE: Final = "api_tokens" -# Modules this one needs registered first: Auth owns the resolver chain, -# Users owns the accounts a personal token acts for, Settings stores -# ``max_lifetime_days``. +# Modules this one is ordered after: Auth owns the resolver chain (required); +# Users, when installed, owns the accounts a personal token acts for; Settings +# stores ``max_lifetime_days``. MODULE_AUTH: Final = "Auth" MODULE_USERS: Final = "Users" MODULE_SETTINGS: Final = "Settings" diff --git a/modules/api_tokens/api_tokens/module.py b/modules/api_tokens/api_tokens/module.py index 33640a04..8bc6bb62 100644 --- a/modules/api_tokens/api_tokens/module.py +++ b/modules/api_tokens/api_tokens/module.py @@ -26,11 +26,13 @@ class ApiTokensModule(ModuleBase): route_prefix=c.API_PREFIX, view_prefix=c.VIEW_PREFIX, admin_view_prefix=c.ADMIN_VIEW_PREFIX, - # Auth owns the resolver chain this module appends to; Users owns the - # accounts a personal token acts for. Settings (the lifetime cap's - # editor) is ordering only: the framework hydrates settings (#500). - depends_on=[c.MODULE_AUTH, c.MODULE_USERS], - optional_depends_on=[c.MODULE_SETTINGS], + # Auth owns the resolver chain this module appends to. Owners are read + # through the app's user directory, so Users is ordering only: under + # Keycloak service keys work and personal tokens are refused (roles + # unknown). Settings (the lifetime cap's editor) is ordering only too: + # the framework hydrates settings (#500). + depends_on=[c.MODULE_AUTH], + optional_depends_on=[c.MODULE_USERS, c.MODULE_SETTINGS], ) def register_settings(self, app: FastAPI) -> None: diff --git a/modules/api_tokens/pyproject.toml b/modules/api_tokens/pyproject.toml index efabf0d6..a8e59eb5 100644 --- a/modules/api_tokens/pyproject.toml +++ b/modules/api_tokens/pyproject.toml @@ -27,9 +27,10 @@ dependencies = [ "simple_module_inertia==0.0.37", "simple_module_auth==0.0.37", "simple_module_settings==0.0.37", - # A personal token acts for a ``users`` account: the resolver loads the - # owner row and the table's FK points at ``users_user``. - "simple_module_users==0.0.37", + # Owners are read through the app's user directory, so ``users`` is not + # required. ``GUID`` (the owner column's type, as ``users_user.id`` has it) + # comes from here. + "fastapi-users-db-sqlalchemy>=7,<8", ] [project.entry-points.simple_module] @@ -63,4 +64,3 @@ simple_module_hosting = { workspace = true } simple_module_inertia = { workspace = true } simple_module_auth = { workspace = true } simple_module_settings = { workspace = true } -simple_module_users = { workspace = true } diff --git a/modules/keycloak/keycloak/directory.py b/modules/keycloak/keycloak/directory.py index 6a3a2ae7..b7bdd664 100644 --- a/modules/keycloak/keycloak/directory.py +++ b/modules/keycloak/keycloak/directory.py @@ -82,8 +82,8 @@ async def get_many( for chunk in chunked(list(parsed)): stmt = select(*_COLUMNS).where(KeycloakUserCache.id.in_(chunk)) for row in (await db.execute(stmt)).all(): - key = parsed[row.id] - found[key] = _to_directory_user(row, key) + for key in parsed[row.id]: + found[key] = _to_directory_user(row, key) return found async def search(self, db: Any, term: str | None, *, limit: int) -> list[DirectoryUser]: diff --git a/modules/keycloak/tests/keycloak_test_modules.py b/modules/keycloak/tests/keycloak_test_modules.py index 1d292756..14af58c0 100644 --- a/modules/keycloak/tests/keycloak_test_modules.py +++ b/modules/keycloak/tests/keycloak_test_modules.py @@ -2,9 +2,8 @@ Selecting ``keycloak`` drops ``Users``, and outside development a module whose ``depends_on`` is not loaded refuses to boot (GH #501). ``permissions`` (role -pages) and ``api_tokens`` (personal tokens need the owner's roles) still depend -on ``Users``, so a Keycloak install leaves them out — and so do these tests, -rather than booting in a lenient mode production never uses. +pages) still depends on ``Users``, so a Keycloak install leaves it out — and so +do these tests, rather than booting in a lenient mode production never uses. """ from __future__ import annotations diff --git a/modules/keycloak/tests/test_keycloak_api_tokens.py b/modules/keycloak/tests/test_keycloak_api_tokens.py new file mode 100644 index 00000000..c478f1bf --- /dev/null +++ b/modules/keycloak/tests/test_keycloak_api_tokens.py @@ -0,0 +1,88 @@ +"""``api_tokens`` under Keycloak: service keys work, personal tokens are refused. + +``Users`` is only an ordering hint for ``api_tokens`` (GH #501), so a strict +(non-development) Keycloak boot keeps it. A service key needs no account; a +personal token needs its owner's roles, which live in the IdP, so it is +refused rather than acting with none. +""" + +from __future__ import annotations + +from datetime import UTC, datetime, timedelta + +from api_tokens.constants import TokenKind +from api_tokens.models import ApiToken +from api_tokens.resolver import SERVICE_PRINCIPAL_PREFIX, resolve_api_token +from api_tokens.tokens import generate +from auth.contracts.tokens import AUTH_KIND_SERVICE +from keycloak.models import KeycloakUserCache +from keycloak_test_modules import keycloak_modules_enabled +from simple_module_core.user_directory import get_user_directory +from simple_module_hosting.app_builder import create_app +from simple_module_hosting.settings import Settings +from simple_module_test._schema import _create_all_tables +from starlette.requests import Request + + +def _app(): + return create_app( + Settings( + database_url="sqlite+aiosqlite:///:memory:", + environment="testing", + secret_key="test-secret-key", + multi_tenant=False, + auth_provider="keycloak", + modules_enabled=keycloak_modules_enabled(), + ) + ) + + +def _request(app, credential: str) -> Request: + headers = [(b"authorization", f"Bearer {credential}".encode())] + return Request({"type": "http", "app": app, "headers": headers, "method": "GET", "path": "/"}) + + +async def _issue(app, kind: TokenKind, **kw) -> str: + issued = generate(kind) + async with app.state.sm.db.session_factory() as db: + db.add( + ApiToken( + kind=kind, + name=f"{kind}-key", + prefix=issued.prefix, + token_hash=issued.token_hash, + scopes=["feature_flags.view"], + expires_at=datetime.now(UTC) + timedelta(days=1), + **kw, + ) + ) + await db.commit() + return issued.plaintext + + +async def test_api_tokens_boot_strictly_with_service_keys_only(): + app = _app() + assert "ApiTokens" in {m.meta.name for m in app.state.sm.modules} + await _create_all_tables(app.state.sm.db.engine) + try: + async with app.state.sm.db.session_factory() as db: + owner = KeycloakUserCache(keycloak_sub="sub-owner", email="owner@example.com") + db.add(owner) + await db.commit() + owner_id = owner.id + + service = await _issue(app, TokenKind.SERVICE, roles=["reader"]) + principal = await resolve_api_token(_request(app, service)) + assert principal is not None + assert principal.auth_kind == AUTH_KIND_SERVICE + assert principal.id.startswith(SERVICE_PRINCIPAL_PREFIX) + assert principal.roles == ["reader"] + + # The owner is known to the directory; only their roles are not. + async with app.state.sm.db.session_factory() as db: + known = await get_user_directory(app).get_many(db, [str(owner_id)], with_roles=True) + assert known[str(owner_id)].roles is None + personal = await _issue(app, TokenKind.PERSONAL, owner_user_id=owner_id) + assert await resolve_api_token(_request(app, personal)) is None + finally: + await app.state.sm.db.engine.dispose() diff --git a/modules/keycloak/tests/test_keycloak_directory.py b/modules/keycloak/tests/test_keycloak_directory.py index e9f0115e..977c0cb7 100644 --- a/modules/keycloak/tests/test_keycloak_directory.py +++ b/modules/keycloak/tests/test_keycloak_directory.py @@ -32,6 +32,13 @@ async def test_names_a_cached_user(self, db_session: AsyncSession): assert user.label == "Kay Cee" assert user.is_active + async def test_every_spelling_of_one_id_gets_an_entry(self, db_session: AsyncSession): + row = await _cached(db_session, "two@example.com") + spellings = [str(row.id), str(row.id).upper(), row.id.hex] + found = await _D.get_many(db_session, spellings) + assert sorted(found) == sorted(spellings) + assert {u.email for u in found.values()} == {"two@example.com"} + async def test_roles_are_unknown_even_when_asked(self, db_session: AsyncSession): """Roles live in the IdP; an empty tuple would claim the user holds none.""" row = await _cached(db_session, "roles@example.com") diff --git a/modules/keycloak/tests/test_keycloak_strict_boot.py b/modules/keycloak/tests/test_keycloak_strict_boot.py index 3431c4ce..f9eb1f8c 100644 --- a/modules/keycloak/tests/test_keycloak_strict_boot.py +++ b/modules/keycloak/tests/test_keycloak_strict_boot.py @@ -1,9 +1,9 @@ """A Keycloak app boots with its directory, and refuses modules that need Users. Selecting ``keycloak`` drops ``Users``. Outside development the host sorts -modules strictly, so ``permissions`` and ``api_tokens`` (which still depend on -``Users``) fail the boot in one error naming both, instead of booting against -empty ``users_*`` tables (GH #501). +modules strictly, so ``permissions`` (role pages still depend on ``Users``) +fails the boot instead of booting against empty ``users_*`` tables (GH #501). +``api_tokens`` only orders after ``Users``, so it boots (service keys work). """ from __future__ import annotations @@ -32,7 +32,7 @@ def test_non_development_refuses_every_module_that_needs_users(): with pytest.raises(MissingDependencyError) as exc: create_app(_settings("testing")) assert ("Permissions", "Users") in exc.value.missing - assert ("ApiTokens", "Users") in exc.value.missing + assert all(name != "ApiTokens" for name, _dep in exc.value.missing) assert {dep for _name, dep in exc.value.missing} == {"Users"} @@ -41,7 +41,8 @@ def test_without_them_the_app_boots_with_the_keycloak_directory(): assert isinstance(get_user_directory(app), KeycloakDirectory) names = {m.meta.name for m in app.state.sm.modules} assert {"AuditLog", "Dashboard", "FileStorage"} <= names - assert not names & {"Users", "Permissions", "ApiTokens"} + assert "ApiTokens" in names + assert not names & {"Users", "Permissions"} def test_development_only_warns(monkeypatch, caplog): diff --git a/modules/users/tests/test_users_directory.py b/modules/users/tests/test_users_directory.py index b666217b..ae968749 100644 --- a/modules/users/tests/test_users_directory.py +++ b/modules/users/tests/test_users_directory.py @@ -29,6 +29,14 @@ async def test_keys_by_the_callers_spelling(self, db_session: AsyncSession): assert found[upper].label == "Ada" assert found[upper].id == upper + async def test_every_spelling_of_one_id_gets_an_entry(self, db_session: AsyncSession): + """Two spellings of one id must not collapse: each caller key is labelled.""" + user = await _user(db_session, "twice@example.com", full_name="Twice") + spellings = [str(user.id), str(user.id).upper()] + found = await _D.get_many(db_session, spellings, with_roles=True) + assert sorted(found) == sorted(spellings) + assert [found[k].label for k in spellings] == ["Twice", "Twice"] + async def test_unknown_and_foreign_ids_are_absent(self, db_session: AsyncSession): found = await _D.get_many(db_session, [str(uuid.uuid4()), "celery-worker-1", ""]) assert found == {} diff --git a/modules/users/users/directory.py b/modules/users/users/directory.py index 0c573508..518b983c 100644 --- a/modules/users/users/directory.py +++ b/modules/users/users/directory.py @@ -66,9 +66,9 @@ async def get_many( rows = (await db.execute(select(*_COLUMNS).where(User.id.in_(chunk)))).all() roles = await self._roles_for(db, chunk) if with_roles else {} for row in rows: - key = parsed[row.id] user_roles = tuple(sorted(roles.get(row.id, ()))) if with_roles else None - found[key] = _to_directory_user(row, user_roles, key) + for key in parsed[row.id]: + found[key] = _to_directory_user(row, user_roles, key) return found async def _roles_for( diff --git a/uv.lock b/uv.lock index d9512bcf..de9f41c7 100644 --- a/uv.lock +++ b/uv.lock @@ -3037,24 +3037,24 @@ name = "simple-module-api-tokens" version = "0.0.37" source = { editable = "modules/api_tokens" } dependencies = [ + { name = "fastapi-users-db-sqlalchemy" }, { name = "simple-module-auth" }, { name = "simple-module-core" }, { name = "simple-module-db" }, { name = "simple-module-hosting" }, { name = "simple-module-inertia" }, { name = "simple-module-settings" }, - { name = "simple-module-users" }, ] [package.metadata] requires-dist = [ + { name = "fastapi-users-db-sqlalchemy", specifier = ">=7,<8" }, { name = "simple-module-auth", editable = "modules/auth" }, { name = "simple-module-core", editable = "framework/core" }, { name = "simple-module-db", editable = "framework/db" }, { name = "simple-module-hosting", editable = "framework/hosting" }, { name = "simple-module-inertia", editable = "framework/inertia" }, { name = "simple-module-settings", editable = "modules/settings" }, - { name = "simple-module-users", editable = "modules/users" }, ] [[package]]