fix: restore main's gates — file-size cap, and a fixture that ignored its own contract (#315)

Two failures on main, neither of which any single PR's CI could have caught.

**`provider.py` at 305 lines.** #309 and #314 were each green against the main
they branched from; squash-merging both put the file over the 300-line cap. Split
on the seam already there: `_resolve_bearer` moves to `token_strategy`, which is
where `ExpiringDatabaseStrategy` lives. Those two are the only readers of
`users_access_token` and they have to apply the same deadline and
`session_version` rules — keeping them in one file is what stops them drifting.
`provider.py` keeps a one-line delegate so the method stays on the provider's
surface. 305 → 265, and `token_strategy` → 164.

**`setup_pending_app` boots *with* an administrator.** `UsersModule.on_startup`
seeds one from `SM_USERS_BOOTSTRAP_*`, read from the environment *and* from a
`.env` on disk. A developer who followed `.env.example` has those set, so the
fixture whose entire contract is "an app with no administrator" hands back an app
that has two — the setup gate releases, the wizard routes 404, and eleven tests
in `framework/hosting/tests` fail. CI has no `.env`, so it never saw this: the
failure was local-only, which is the worst shape for a fixture to be wrong in.
It also looked like test-ordering noise, because whether it reproduced depended
on what else had booted an app first.

The fixture now scrubs the bootstrap vars and stubs the dotenv reader for the
app it builds, the same way `modules/users/tests/conftest.py` does for its own.
Adding that pushed `fixtures.py` over the cap too, so the schema machinery
(model imports, alembic heads, table creation) moves to `_schema.py` — that
module declares fixtures, this one is what they stand on.

Author: antosubash

framework/testing/simple_module_test/_schema.py

@@ -0,0 +1,77 @@
+"""Schema construction for the plugin's app fixtures.
+
+Split out of :mod:`simple_module_test.fixtures` so that module declares
+fixtures and this one holds the machinery underneath them: importing every
+installed module's models, resolving the migration heads, creating the tables
+and stamping ``alembic_version`` so the boot-time migration check passes.
+"""
+
+from __future__ import annotations
+
+import contextlib
+import importlib
+from functools import lru_cache
+
+from simple_module_core.discovery import discover_modules
+from simple_module_db.base import all_module_bases
+
+
+@lru_cache(maxsize=1)
+def _ensure_models_imported() -> list:
+    """Import all module models so all_module_bases is populated (cached)."""
+    for mod in discover_modules():
+        pkg = type(mod).__module__.split(".")[0]
+        with contextlib.suppress(ModuleNotFoundError):
+            importlib.import_module(f"{pkg}.models")
+    return list(all_module_bases)
+
+
+@lru_cache(maxsize=1)
+def _alembic_heads() -> tuple[str, ...]:
+    """Cached head revisions — cannot change within a pytest run.
+
+    Plural: each module's first migration sets its own ``branch_labels``, so
+    the history has one head per module and a real upgraded database carries
+    an ``alembic_version`` row for each. Stamping only one leaves the rest
+    looking un-applied, which now reads as a behind-head schema and puts every
+    test app behind the setup gate.
+    """
+    from simple_module_hosting.migrations import resolve_head_revisions
+
+    return resolve_head_revisions()
+
+
+async def _create_all_tables(engine) -> None:
+    """Create all module tables in a single connection.
+
+    Also stamps the alembic_version table at heads so the app's startup
+    migration check (``check_migrations``) treats the test DB as current.
+    Without the stamp the check would raise because ``create_all`` doesn't
+    touch alembic_version.
+    """
+    from sqlalchemy import text
+
+    bases = _ensure_models_imported()
+    heads = _alembic_heads()
+
+    async with engine.begin() as conn:
+
+        def _sync_create_all(sync_conn):
+            for base in bases:
+                base.metadata.create_all(sync_conn)
+
+        await conn.run_sync(_sync_create_all)
+
+        if heads:
+            await conn.execute(
+                text(
+                    "CREATE TABLE IF NOT EXISTS alembic_version "
+                    "(version_num VARCHAR(32) NOT NULL PRIMARY KEY)"
+                )
+            )
+            await conn.execute(text("DELETE FROM alembic_version"))
+            for head in heads:
+                await conn.execute(
+                    text("INSERT INTO alembic_version (version_num) VALUES (:v)"),
+                    {"v": head},
+                )

framework/testing/simple_module_test/fixtures.py

@@ -20,20 +20,17 @@
 
 from __future__ import annotations
 
-import contextlib
-import importlib
 import os
 from collections.abc import AsyncGenerator, Iterator
-from functools import lru_cache
 
 import httpx
 import pytest
-from simple_module_core.discovery import DEFAULT_AUTH_PROVIDER, discover_modules
-from simple_module_db.base import all_module_bases
+from simple_module_core.discovery import DEFAULT_AUTH_PROVIDER
 from simple_module_db.session import DatabaseState, init_db
 from simple_module_hosting.settings import Settings
 from sqlalchemy.ext.asyncio import AsyncEngine, AsyncSession
 
+from simple_module_test._schema import _create_all_tables
 from simple_module_test.session_cookie import forge_session_cookie
 
 _AUTH_PROVIDER_ENV = "SM_AUTH_PROVIDER"
@@ -107,67 +104,6 @@ async def engine(db_state: DatabaseState) -> AsyncEngine:
     return db_state.engine
 
 
-@lru_cache(maxsize=1)
-def _ensure_models_imported() -> list:
-    """Import all module models so all_module_bases is populated (cached)."""
-    for mod in discover_modules():
-        pkg = type(mod).__module__.split(".")[0]
-        with contextlib.suppress(ModuleNotFoundError):
-            importlib.import_module(f"{pkg}.models")
-    return list(all_module_bases)
-
-
-@lru_cache(maxsize=1)
-def _alembic_heads() -> tuple[str, ...]:
-    """Cached head revisions — cannot change within a pytest run.
-
-    Plural: each module's first migration sets its own ``branch_labels``, so
-    the history has one head per module and a real upgraded database carries
-    an ``alembic_version`` row for each. Stamping only one leaves the rest
-    looking un-applied, which now reads as a behind-head schema and puts every
-    test app behind the setup gate.
-    """
-    from simple_module_hosting.migrations import resolve_head_revisions
-
-    return resolve_head_revisions()
-
-
-async def _create_all_tables(engine) -> None:
-    """Create all module tables in a single connection.
-
-    Also stamps the alembic_version table at heads so the app's startup
-    migration check (``check_migrations``) treats the test DB as current.
-    Without the stamp the check would raise because ``create_all`` doesn't
-    touch alembic_version.
-    """
-    from sqlalchemy import text
-
-    bases = _ensure_models_imported()
-    heads = _alembic_heads()
-
-    async with engine.begin() as conn:
-
-        def _sync_create_all(sync_conn):
-            for base in bases:
-                base.metadata.create_all(sync_conn)
-
-        await conn.run_sync(_sync_create_all)
-
-        if heads:
-            await conn.execute(
-                text(
-                    "CREATE TABLE IF NOT EXISTS alembic_version "
-                    "(version_num VARCHAR(32) NOT NULL PRIMARY KEY)"
-                )
-            )
-            await conn.execute(text("DELETE FROM alembic_version"))
-            for head in heads:
-                await conn.execute(
-                    text("INSERT INTO alembic_version (version_num) VALUES (:v)"),
-                    {"v": head},
-                )
-
-
 @pytest.fixture
 async def db_session(db_state: DatabaseState) -> AsyncGenerator[AsyncSession, None]:
     """Yield an async session backed by in-memory SQLite."""
@@ -236,12 +172,38 @@ async def app(settings: Settings):
         yield application
 
 
+def _disable_env_bootstrap(monkeypatch: pytest.MonkeyPatch) -> None:
+    """Stop ``bootstrap_admin_from_env`` seeding the app being built.
+
+    ``UsersModule.on_startup`` creates an administrator from
+    ``SM_USERS_BOOTSTRAP_*``, read from the environment *and* from a ``.env`` on
+    disk. A developer who followed ``.env.example`` has those set, so every app
+    this plugin builds gets an admin whether the fixture asked for one or not.
+    For ``setup_pending_app`` that is not a nuisance but the opposite of its
+    contract: the setup gate releases the moment an administrator exists, so the
+    wizard routes 404 and every test using it fails.
+
+    They fail *locally only* — CI has no ``.env`` — which is the worst shape for
+    a fixture to be wrong in, and left the failure looking like test-ordering
+    noise rather than a fixture that does not do what it says.
+    """
+    for key in list(os.environ):
+        if key.startswith("SM_USERS_BOOTSTRAP_"):
+            monkeypatch.delenv(key, raising=False)
+    try:
+        from users import bootstrap as bootstrap_module
+    except ImportError:
+        return  # No local-accounts provider installed; nothing bootstraps.
+    monkeypatch.setattr(bootstrap_module, "_read_dotenv_bootstrap_vars", dict)
+
+
 @pytest.fixture
-async def setup_pending_app(settings: Settings):
+async def setup_pending_app(settings: Settings, monkeypatch: pytest.MonkeyPatch):
     """An app with no administrator, so the first-run setup gate is engaged.
 
     The counterpart to ``app``: use this to assert on setup-mode behaviour.
     """
+    _disable_env_bootstrap(monkeypatch)
     async for application in _build_app(settings, seed_admin=False):
         yield application
 

modules/users/users/provider.py

@@ -10,7 +10,6 @@
 import logging
 import uuid as uuid_mod
 from collections.abc import Mapping
-from datetime import UTC, datetime, timedelta
 from typing import Any
 
 from auth.contracts.schemas import UserContext
@@ -221,55 +220,16 @@ def is_bearer_request(self, request: Request | None) -> bool:
         return request.headers.get("authorization", "").startswith("Bearer ")
 
     async def _resolve_bearer(self, scope, token: str) -> UserContext | None:
-        """Look up an access token in users_access_token and return the user."""
-        try:
-            from sqlalchemy import select
-            from sqlalchemy.orm import noload, selectinload
+        """Resolve an ``Authorization: Bearer`` token to its user.
 
-            from users.backend import _TOKEN_LIFETIME_SECONDS
-            from users.models import User, UserAccessToken
-            from users.token_strategy import token_is_live
+        Delegates to :func:`users.token_strategy.resolve_bearer`, which is where
+        ``ExpiringDatabaseStrategy`` also lives — the two readers of
+        ``users_access_token`` apply the same deadline and ``session_version``
+        rules, and keeping them in one file is what stops them drifting apart.
+        """
+        from users.token_strategy import resolve_bearer
 
-            session_factory = scope["app"].state.sm.db.session_factory
-            async with session_factory() as db_session:
-                # Neither clause is optional: this path bypasses fastapi-users'
-                # DatabaseStrategy, which is where a lifetime is normally
-                # applied, so without them a row authenticated forever. The
-                # ceiling is the same constant the strategy reads with, and
-                # ``expires_at`` is the row's own deadline — an ordinary
-                # sign-in's fourteen days, or ``/auth/token``'s fifteen
-                # minutes, rather than the thirty-day ceiling for all of them.
-                now = datetime.now(UTC)
-                cutoff = now - timedelta(seconds=_TOKEN_LIFETIME_SECONDS)
-                stmt = select(UserAccessToken).where(
-                    UserAccessToken.token == token,
-                    UserAccessToken.created_at > cutoff,
-                    UserAccessToken.expires_at > now,
-                )
-                access = (await db_session.execute(stmt)).scalar_one_or_none()
-                if access is None:
-                    return None
-                # noload oauth_accounts: lazy="selectin" on the model would
-                # otherwise fire an extra query the UserContext never reads.
-                stmt = (
-                    select(User)
-                    .where(User.id == access.user_id)
-                    .options(selectinload(User.roles), noload(User.oauth_accounts))
-                )
-                user = (await db_session.execute(stmt)).scalar_one_or_none()
-                if user is None or not user.is_active or user.disabled_at is not None:
-                    return None
-                # The revocation check the session path has had all along. A
-                # password change bumps ``session_version`` and strands every
-                # session; without this the bearer tokens minted before it kept
-                # working, including any an attacker who knew the old password
-                # had already collected. Free here — the row is already loaded.
-                if not token_is_live(access, user, now):
-                    return None
-                return UserContext.from_user(user)
-        except Exception:
-            logger.exception("Bearer token resolution failed")
-            return None
+        return await resolve_bearer(scope, token)
 
     async def _load_user(self, scope, user_id: uuid_mod.UUID, session=None) -> UserContext | None:
         try:

modules/users/users/token_strategy.py

@@ -18,15 +18,19 @@
 
 from __future__ import annotations
 
+import logging
 from datetime import UTC, datetime, timedelta
 from typing import Any
 
+from auth.contracts.schemas import UserContext
 from fastapi_users import exceptions, models
 from fastapi_users.authentication.strategy.db import AccessTokenDatabase, DatabaseStrategy
 from fastapi_users.manager import BaseUserManager
 
 from users.models import UserAccessToken
 
+logger = logging.getLogger(__name__)
+
 
 def token_is_live(access_token: UserAccessToken, user: Any, now: datetime | None = None) -> bool:
     """Whether this row still authenticates ``user``.
@@ -100,3 +104,61 @@ async def read_token(
         if not token_is_live(access_token, user):
             return None
         return user
+
+
+async def resolve_bearer(scope, token: str) -> UserContext | None:
+    """Look up an access token in ``users_access_token`` and return the user.
+
+    The provider's half of the pair. ``AuthMiddleware`` reaches this through
+    ``UsersAuthProvider.resolve_user``; ``fastapi_users.current_user`` reaches
+    :class:`ExpiringDatabaseStrategy` instead. Same two bounds either way,
+    written once as SQL and once in Python because one path can push them into
+    the query and the other cannot.
+    """
+    try:
+        from sqlalchemy import select
+        from sqlalchemy.orm import noload, selectinload
+
+        from users.backend import _TOKEN_LIFETIME_SECONDS
+        from users.models import User
+
+        session_factory = scope["app"].state.sm.db.session_factory
+        async with session_factory() as db_session:
+            # Neither clause is optional: this path bypasses fastapi-users'
+            # DatabaseStrategy, which is where a lifetime is normally applied,
+            # so without them a row authenticated forever. The ceiling is the
+            # same constant the strategy reads with, and ``expires_at`` is the
+            # row's own deadline — an ordinary sign-in's fourteen days, or
+            # ``/auth/token``'s fifteen minutes, rather than the thirty-day
+            # ceiling for all of them.
+            now = datetime.now(UTC)
+            cutoff = now - timedelta(seconds=_TOKEN_LIFETIME_SECONDS)
+            stmt = select(UserAccessToken).where(
+                UserAccessToken.token == token,
+                UserAccessToken.created_at > cutoff,
+                UserAccessToken.expires_at > now,
+            )
+            access = (await db_session.execute(stmt)).scalar_one_or_none()
+            if access is None:
+                return None
+            # noload oauth_accounts: lazy="selectin" on the model would
+            # otherwise fire an extra query the UserContext never reads.
+            stmt = (
+                select(User)
+                .where(User.id == access.user_id)
+                .options(selectinload(User.roles), noload(User.oauth_accounts))
+            )
+            user = (await db_session.execute(stmt)).scalar_one_or_none()
+            if user is None or not user.is_active or user.disabled_at is not None:
+                return None
+            # The revocation check the session path has had all along. A
+            # password change bumps ``session_version`` and strands every
+            # session; without this the bearer tokens minted before it kept
+            # working, including any an attacker who knew the old password had
+            # already collected. Free here — the row is already loaded.
+            if not token_is_live(access, user, now):
+                return None
+            return UserContext.from_user(user)
+    except Exception:
+        logger.exception("Bearer token resolution failed")
+        return None