From ab75018a2521d3fdd87a4cb3e6c2acfefd3a0fb0 Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Wed, 8 Jul 2026 06:26:19 +0200 Subject: [PATCH 1/5] refactor(api/auth): move OAuthClient and session key into auth/oauth --- api/src/damnit_api/auth/dependencies.py | 2 +- api/src/damnit_api/auth/oauth.py | 46 +++++++++++++++++++++++++ api/src/damnit_api/auth/routers.py | 2 +- api/src/damnit_api/main.py | 3 +- api/src/damnit_api/state.py | 38 ++------------------ api/tests/test_auth_routers.py | 2 +- api/tests/test_errors.py | 4 ++- api/tests/test_state.py | 3 +- 8 files changed, 57 insertions(+), 43 deletions(-) create mode 100644 api/src/damnit_api/auth/oauth.py diff --git a/api/src/damnit_api/auth/dependencies.py b/api/src/damnit_api/auth/dependencies.py index bca3b2e9..c1a5338f 100644 --- a/api/src/damnit_api/auth/dependencies.py +++ b/api/src/damnit_api/auth/dependencies.py @@ -9,9 +9,9 @@ from sqlmodel.ext.asyncio.session import AsyncSession from .._mymdc.dependencies import MyMdCClient -from ..state import OAuthClient from .models import OAuthUserInfo as _OAuthUserInfo from .models import User as _User +from .oauth import OAuthClient from .token_store import TokenStore diff --git a/api/src/damnit_api/auth/oauth.py b/api/src/damnit_api/auth/oauth.py new file mode 100644 index 00000000..59d9dcf0 --- /dev/null +++ b/api/src/damnit_api/auth/oauth.py @@ -0,0 +1,46 @@ +"""OAuth2/OIDC client configuration. + +Owned by the auth slice; the composition root builds it via +`create_oauth_client` and holds it on `AppState`. +""" + +from dataclasses import dataclass, field +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from ..shared.settings import Settings + +# Session cookie name, shared by `main.py`'s session config and the logout +# handlers in `auth/routers.py` so the two cannot drift. +SESSION_COOKIE_KEY = "session" + + +@dataclass +class OAuthClient: + """OAuth2/OIDC client configuration with lazily loaded server metadata.""" + + client_id: str + client_secret: str + scope: str + server_metadata_url: str + server_metadata: dict = field(default_factory=dict) + + async def load_server_metadata(self) -> None: + import httpx + + async with httpx.AsyncClient() as http: + resp = await http.get(self.server_metadata_url) + resp.raise_for_status() + self.server_metadata = resp.json() + + +def create_oauth_client(settings: "Settings") -> OAuthClient | None: + if settings.auth is None: + return None + + return OAuthClient( + client_id=settings.auth.client_id, + client_secret=settings.auth.client_secret.get_secret_value(), + scope="openid email groups", + server_metadata_url=str(settings.auth.server_metadata_url), + ) diff --git a/api/src/damnit_api/auth/routers.py b/api/src/damnit_api/auth/routers.py index d450a990..89f0134b 100644 --- a/api/src/damnit_api/auth/routers.py +++ b/api/src/damnit_api/auth/routers.py @@ -16,8 +16,8 @@ from .._db.dependencies import DBSession from .._mymdc.dependencies import MyMdCClient from ..runs.dependencies import Repositories -from ..state import SESSION_COOKIE_KEY, OAuthClient from . import dependencies, models +from .oauth import SESSION_COOKIE_KEY, OAuthClient from .token_store import TokenStore logger = get_logger() diff --git a/api/src/damnit_api/main.py b/api/src/damnit_api/main.py index b6b7d6c4..53dfedf2 100644 --- a/api/src/damnit_api/main.py +++ b/api/src/damnit_api/main.py @@ -22,18 +22,17 @@ def create_app(): from litestar.response import Redirect from . import _logging, auth, get_logger + from .auth.oauth import SESSION_COOKIE_KEY, create_oauth_client from .graphql.dependencies import get_subscription_cursors from .runs.dependencies import get_repositories from .shared.errors import DamnitWebError from .shared.gql import get_gql_controller from .shared.settings import settings from .state import ( - SESSION_COOKIE_KEY, AppState, create_db_engine, create_db_sessionmaker, create_mymdc_client, - create_oauth_client, create_repositories, create_subscription_cursors, create_token_store, diff --git a/api/src/damnit_api/state.py b/api/src/damnit_api/state.py index 4917c5e3..9a64eeb9 100644 --- a/api/src/damnit_api/state.py +++ b/api/src/damnit_api/state.py @@ -8,7 +8,7 @@ from __future__ import annotations -from dataclasses import dataclass, field +from dataclasses import dataclass from typing import TYPE_CHECKING from litestar.datastructures import ( @@ -19,34 +19,12 @@ if TYPE_CHECKING: from ._mymdc.clients import MyMdCClient + from .auth.oauth import OAuthClient from .auth.token_store import TokenStore from .graphql.subscriptions import SubscriptionCursors from .runs.repository import DamnitRepositoryRegistry from .shared.settings import Settings -# Session cookie name, shared by `main.py`'s `CookieBackendConfig` and the -# logout handlers in `auth/routers.py` so the two cannot drift. -SESSION_COOKIE_KEY = "session" - - -@dataclass -class OAuthClient: - """OAuth2/OIDC client configuration with lazily loaded server metadata.""" - - client_id: str - client_secret: str - scope: str - server_metadata_url: str - server_metadata: dict = field(default_factory=dict) - - async def load_server_metadata(self) -> None: - import httpx - - async with httpx.AsyncClient() as http: - resp = await http.get(self.server_metadata_url) - resp.raise_for_status() - self.server_metadata = resp.json() - @dataclass(frozen=True) class AppState: @@ -83,18 +61,6 @@ def create_mymdc_client(settings: Settings) -> MyMdCClient: raise ValueError(msg) -def create_oauth_client(settings: Settings) -> OAuthClient | None: - if settings.auth is None: - return None - - return OAuthClient( - client_id=settings.auth.client_id, - client_secret=settings.auth.client_secret.get_secret_value(), - scope="openid email groups", - server_metadata_url=str(settings.auth.server_metadata_url), - ) - - def create_token_store() -> TokenStore: from .auth.token_store import InMemoryTokenStore diff --git a/api/tests/test_auth_routers.py b/api/tests/test_auth_routers.py index 2fd0a9f8..5c09b8b6 100644 --- a/api/tests/test_auth_routers.py +++ b/api/tests/test_auth_routers.py @@ -9,7 +9,7 @@ from damnit_api.auth.routers import OAuthController from damnit_api.auth.token_store import InMemoryTokenStore -from damnit_api.state import SESSION_COOKIE_KEY, OAuthClient +from damnit_api.auth.oauth import SESSION_COOKIE_KEY, OAuthClient SERVER_METADATA = { "authorization_endpoint": "https://idp.example/authorize", diff --git a/api/tests/test_errors.py b/api/tests/test_errors.py index 7b906cd5..059eb9db 100644 --- a/api/tests/test_errors.py +++ b/api/tests/test_errors.py @@ -86,7 +86,9 @@ def app(monkeypatch): async def noop_load(self): pass - monkeypatch.setattr("damnit_api.state.OAuthClient.load_server_metadata", noop_load) + monkeypatch.setattr( + "damnit_api.auth.oauth.OAuthClient.load_server_metadata", noop_load + ) return create_app() diff --git a/api/tests/test_state.py b/api/tests/test_state.py index 0f7dbd02..dafd75bb 100644 --- a/api/tests/test_state.py +++ b/api/tests/test_state.py @@ -6,11 +6,12 @@ import pytest +from damnit_api.auth.oauth import create_oauth_client from damnit_api.auth.token_store import InMemoryTokenStore from damnit_api.runs.repository import DamnitRepositoryRegistry from damnit_api.shared.models import ProposalNumber from damnit_api.shared.settings import Settings -from damnit_api.state import create_mymdc_client, create_oauth_client +from damnit_api.state import create_mymdc_client def test_appstate_only_imported_by_composition_root(): From 31be4ae8726be5cdbe63b1e2e04b1fce37a12450 Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Wed, 8 Jul 2026 06:36:18 +0200 Subject: [PATCH 2/5] feat(api): server-side sessions on a litestar store registry --- api/src/damnit_api/main.py | 42 +++++++++++++------- api/src/damnit_api/shared/settings.py | 4 ++ api/tests/refactor/e2e/conftest.py | 56 ++++++++++++++------------- api/tests/test_auth_routers.py | 6 +-- 4 files changed, 63 insertions(+), 45 deletions(-) diff --git a/api/src/damnit_api/main.py b/api/src/damnit_api/main.py index 53dfedf2..cfab19d2 100644 --- a/api/src/damnit_api/main.py +++ b/api/src/damnit_api/main.py @@ -14,12 +14,13 @@ def create_app(): - import hashlib - from litestar import Request, Response - from litestar.middleware.session.client_side import CookieBackendConfig + from litestar.middleware.session.server_side import ServerSideSessionConfig from litestar.openapi import OpenAPIConfig from litestar.response import Redirect + from litestar.stores.file import FileStore + from litestar.stores.memory import MemoryStore + from litestar.stores.registry import StoreRegistry from . import _logging, auth, get_logger from .auth.oauth import SESSION_COOKIE_KEY, create_oauth_client @@ -41,14 +42,12 @@ def create_app(): logger = get_logger("lifespan") - # ── Session middleware ──────────────────────────────────────────────────── - # Derive a 32-byte AES key from the session secret via SHA-256. - session_secret = settings.session_secret - assert session_secret is not None # enforced by Settings validator # noqa: S101 - session_config = CookieBackendConfig( - secret=hashlib.sha256(session_secret.get_secret_value().encode()).digest(), - key=SESSION_COOKIE_KEY, - ) + # ── Stores + server-side sessions ───────────────────────────────────────── + # Sessions are server-side: the cookie carries only an opaque session id; + # session data lives in a Litestar store. The same registry backs every + # named store (sessions, response cache); the backend is mode-dependent + # (in-memory locally, file-backed otherwise) and selected below. + session_config = ServerSideSessionConfig(key=SESSION_COOKIE_KEY) # ── Exception handlers ──────────────────────────────────────────────────── def dw_error_handler(request: Request, exc: DamnitWebError) -> Response: @@ -109,10 +108,22 @@ async def lifespan(app: Litestar): finally: await engine.dispose() - # ── Auth controller (mode-dependent, ADR-008 composition) ─────────────── - auth_controller = ( - auth.NoAuthOAuthController if settings.is_local else auth.OAuthController - ) + def _file_store(name: str) -> FileStore: + # FileStore does not create its directory on write; the session read + # path (and forged test writes) need it to exist up front. + path = settings.store_path / name + path.mkdir(parents=True, exist_ok=True) + return FileStore(path) + + # ── Mode-dependent composition: controller and stores ─────────────────── + # In-memory stores are process-local: local mode is single-worker, and the + # file-backed stores serve the deployed (potentially multi-worker) case. + if settings.is_local: + auth_controller = auth.NoAuthOAuthController + stores = StoreRegistry(default_factory=lambda name: MemoryStore()) + else: + auth_controller = auth.OAuthController + stores = StoreRegistry(default_factory=_file_store) # ── GraphQL controller ──────────────────────────────────────────────────── gql_controller = get_gql_controller() @@ -143,6 +154,7 @@ async def lifespan(app: Litestar): ), "repositories": Provide(get_repositories, sync_to_thread=False), }, + stores=stores, middleware=[ session_config.middleware, _logging.RequestLoggingMiddleware, diff --git a/api/src/damnit_api/shared/settings.py b/api/src/damnit_api/shared/settings.py index 9aeae7f0..36bdc434 100644 --- a/api/src/damnit_api/shared/settings.py +++ b/api/src/damnit_api/shared/settings.py @@ -52,6 +52,10 @@ class Settings(BaseSettings): db_path: Path = Path(__file__).parents[3] / "dw_api.sqlite" + # Directory for file-backed Litestar stores (server-side sessions, response + # cache) outside local mode; each named store gets a subdirectory. + store_path: Path = Path(__file__).parents[3] / "stores" + debug: bool = True log_level: str = "DEBUG" diff --git a/api/tests/refactor/e2e/conftest.py b/api/tests/refactor/e2e/conftest.py index 7e2c89e3..c491564a 100644 --- a/api/tests/refactor/e2e/conftest.py +++ b/api/tests/refactor/e2e/conftest.py @@ -107,10 +107,13 @@ def _fresh_app_db(tmp_path): db_path = tmp_path / "dw_api.sqlite" SQLModel.metadata.create_all(create_engine(f"sqlite:///{db_path}")) - original = settings.db_path + original_db, original_store = settings.db_path, settings.store_path settings.db_path = db_path + # Isolate the file-backed stores (server-side sessions, response cache) into + # the tmp dir so tests never write into the repo's `stores/`. + settings.store_path = tmp_path / "stores" yield - settings.db_path = original + settings.db_path, settings.store_path = original_db, original_store @pytest_asyncio.fixture @@ -128,44 +131,43 @@ async def e2e_client(vcr, _fresh_app_db): async with httpx.AsyncClient( transport=transport, base_url="http://testserver" ) as client: + # Expose the Litestar app so `mint_session_cookie` can write the + # forged session into the app's own server-side store. + client.app = app yield client @pytest_asyncio.fixture -async def logged_in_client(e2e_client): # noqa: RUF029 - must be async to receive the async fixture - """ASGI client with new login session cookie.""" - e2e_client.cookies.update(mint_session_cookie()) +async def logged_in_client(e2e_client): + """ASGI client with a forged server-side login session.""" + cookie = await mint_session_cookie(e2e_client.app) + e2e_client.cookies.update(cookie) return e2e_client -def mint_session_cookie(user: dict | None = None) -> dict[str, str]: +async def mint_session_cookie(app, user: dict | None = None) -> dict[str, str]: """Forge the session cookie a completed OAuth login would set. !!! warning This bypasses awkward OAuth internals (redirects, callbacks, etc...) while - keeping the real session path: the value is encrypted with the app's own - session secret, exactly as Litestar's client-side `CookieBackendConfig` does - (see `main.py`). + keeping the real session path: the session data is written into the app's + own server-side store and the returned cookie carries only the opaque + session id, exactly as Litestar's `ServerSideSessionConfig` does (see + `main.py`). """ - import hashlib - - from litestar.middleware.session.client_side import ( - ClientSideSessionBackend, - CookieBackendConfig, + from litestar.middleware.session.server_side import ( + ServerSideSessionBackend, + ServerSideSessionConfig, ) - from damnit_api.shared.settings import settings - - secret = settings.session_secret.get_secret_value() # ty: ignore[unresolved-attribute] # pyright: ignore[reportOptionalMemberAccess] - config = CookieBackendConfig( - secret=hashlib.sha256(secret.encode()).digest(), key=SESSION_COOKIE + config = ServerSideSessionConfig(key=SESSION_COOKIE) + backend = ServerSideSessionBackend(config=config) + store = config.get_store_from_app(app) + session_id = backend.generate_session_id() + await backend.set( + session_id=session_id, + data=backend.serialize_data({"user": user or TEST_USER}), + store=store, ) - backend = ClientSideSessionBackend(config=config) - chunks = backend.dump_data({"user": user or TEST_USER}) - if len(chunks) == 1: - return {SESSION_COOKIE: chunks[0].decode("utf-8")} - return { - f"{SESSION_COOKIE}-{i}": chunk.decode("utf-8") - for i, chunk in enumerate(chunks) - } + return {SESSION_COOKIE: session_id} diff --git a/api/tests/test_auth_routers.py b/api/tests/test_auth_routers.py index 5c09b8b6..bc075423 100644 --- a/api/tests/test_auth_routers.py +++ b/api/tests/test_auth_routers.py @@ -4,12 +4,12 @@ import pytest from litestar.di import Provide -from litestar.middleware.session.client_side import CookieBackendConfig +from litestar.middleware.session.server_side import ServerSideSessionConfig from litestar.testing import create_test_client +from damnit_api.auth.oauth import SESSION_COOKIE_KEY, OAuthClient from damnit_api.auth.routers import OAuthController from damnit_api.auth.token_store import InMemoryTokenStore -from damnit_api.auth.oauth import SESSION_COOKIE_KEY, OAuthClient SERVER_METADATA = { "authorization_endpoint": "https://idp.example/authorize", @@ -47,7 +47,7 @@ def token_store(): @pytest.fixture def session_config(): - return CookieBackendConfig(secret=b"0" * 32, key=SESSION_COOKIE_KEY) + return ServerSideSessionConfig(key=SESSION_COOKIE_KEY) @pytest.fixture From 351eaa0b576e429776f800cc8dbc4f12631a6dd2 Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Wed, 8 Jul 2026 06:42:40 +0200 Subject: [PATCH 3/5] feat(api/auth): fold the token store into the server-side session --- .gitignore | 3 ++ api/src/damnit_api/auth/dependencies.py | 5 --- api/src/damnit_api/auth/routers.py | 14 +++---- api/src/damnit_api/auth/token_store.py | 29 -------------- api/src/damnit_api/main.py | 5 --- api/src/damnit_api/state.py | 8 ---- api/tests/test_auth_routers.py | 53 ++++++++++--------------- api/tests/test_state.py | 19 --------- 8 files changed, 32 insertions(+), 104 deletions(-) delete mode 100644 api/src/damnit_api/auth/token_store.py diff --git a/.gitignore b/.gitignore index 14229b43..21fd6521 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,6 @@ certs/ *.crt *.key *.pem + +# Litestar file-backed stores (server-side sessions, response cache) +/api/stores/ diff --git a/api/src/damnit_api/auth/dependencies.py b/api/src/damnit_api/auth/dependencies.py index c1a5338f..e3ff02e5 100644 --- a/api/src/damnit_api/auth/dependencies.py +++ b/api/src/damnit_api/auth/dependencies.py @@ -12,7 +12,6 @@ from .models import OAuthUserInfo as _OAuthUserInfo from .models import User as _User from .oauth import OAuthClient -from .token_store import TokenStore def get_oauth_client(state: State) -> OAuthClient: @@ -41,10 +40,6 @@ async def get_oauth_http_client( await client.aclose() -def get_token_store(state: State) -> TokenStore: - return state.app_state.token_store # type: ignore[attr-defined] - - def get_oauth_user_info(request: Request) -> _OAuthUserInfo: """Litestar dependency: resolve OAuthUserInfo from the session.""" return _OAuthUserInfo.from_connection(request) # type: ignore[arg-type] diff --git a/api/src/damnit_api/auth/routers.py b/api/src/damnit_api/auth/routers.py index 89f0134b..fac8ff85 100644 --- a/api/src/damnit_api/auth/routers.py +++ b/api/src/damnit_api/auth/routers.py @@ -18,7 +18,6 @@ from ..runs.dependencies import Repositories from . import dependencies, models from .oauth import SESSION_COOKIE_KEY, OAuthClient -from .token_store import TokenStore logger = get_logger() @@ -122,7 +121,6 @@ async def callback( self, request: Request, oauth_config: OAuthClient, - token_store: TokenStore, oauth_http_client: Annotated[ AsyncOAuth2Client, Dependency(skip_validation=True) ], @@ -159,8 +157,10 @@ async def callback( # session and is re-validated against the relative-path allow-list. target = _sanitize_redirect_target(request.session.pop("_login_redirect", None)) + # Tokens live in the session: server-side only, keyed by the session + # id, with no parallel store to keep consistent. request.session["user"] = user - token_store.store(str(user["sub"]), token) + request.session["tokens"] = token return Redirect(path=target) @@ -169,15 +169,15 @@ async def logout( self, request: Request, oauth_config: OAuthClient, - token_store: TokenStore, ) -> Response: """Clear the session; revoke tokens in the background.""" - user_sub = request.session.get("user", {}).get("sub") revocation_endpoint = oauth_config.server_metadata.get("revocation_endpoint") end_session_endpoint = oauth_config.server_metadata.get( "end_session_endpoint" ) + tokens = request.session.pop("tokens", None) or {} + tokens_to_revoke: list[tuple[str, str]] = [] if ( revocation_endpoint @@ -187,10 +187,10 @@ async def logout( tokens_to_revoke = [ (k, token) for k in ("refresh_token", "access_token") - if (token := token_store.pop_token_field(user_sub, k)) + if (token := tokens.get(k)) ] - token_id = token_store.pop_token_field(user_sub, "id_token") + token_id = tokens.get("id_token") logout_url = None if token_id and end_session_endpoint: params = {"id_token_hint": token_id} diff --git a/api/src/damnit_api/auth/token_store.py b/api/src/damnit_api/auth/token_store.py deleted file mode 100644 index ddf64334..00000000 --- a/api/src/damnit_api/auth/token_store.py +++ /dev/null @@ -1,29 +0,0 @@ -"""TokenStore protocol and implementations.""" - -from typing import Any, Protocol, runtime_checkable - - -# `runtime_checkable` is required so Litestar's msgspec-based signature -# validation (which does `isinstance(value, TokenStore)`) doesn't raise a -# TypeError on every request that injects this dependency. -@runtime_checkable -class TokenStore(Protocol): - def store(self, sub: str, token: dict[str, Any]) -> None: ... - def pop_token_field(self, sub: str, field: str) -> str | None: ... - - -class InMemoryTokenStore: - def __init__(self) -> None: - self._tokens: dict[str, dict[str, Any]] = {} - - def store(self, sub: str, token: dict[str, Any]) -> None: - self._tokens[sub] = token - - def pop_token_field(self, sub: str, field: str) -> str | None: - token = self._tokens.get(sub) - if token is None: - return None - value = token.pop(field, None) - if not token: - del self._tokens[sub] - return value diff --git a/api/src/damnit_api/main.py b/api/src/damnit_api/main.py index cfab19d2..529c9ce5 100644 --- a/api/src/damnit_api/main.py +++ b/api/src/damnit_api/main.py @@ -36,7 +36,6 @@ def create_app(): create_mymdc_client, create_repositories, create_subscription_cursors, - create_token_store, provide_app_state, ) @@ -98,7 +97,6 @@ async def lifespan(app: Litestar): db_sessionmaker=create_db_sessionmaker(engine), mymdc_client=create_mymdc_client(settings), oauth_client=oauth_client, - token_store=create_token_store(), repositories=create_repositories(), subscription_cursors=create_subscription_cursors(), ) @@ -142,9 +140,6 @@ def _file_store(name: str) -> FileStore: "oauth_config": Provide( auth.dependencies.get_oauth_client, sync_to_thread=False ), - "token_store": Provide( - auth.dependencies.get_token_store, sync_to_thread=False - ), "session": Provide(get_session), "mymdc": Provide(get_mymdc_client, sync_to_thread=False), "user": Provide(get_user), diff --git a/api/src/damnit_api/state.py b/api/src/damnit_api/state.py index 9a64eeb9..a73b7f5d 100644 --- a/api/src/damnit_api/state.py +++ b/api/src/damnit_api/state.py @@ -20,7 +20,6 @@ if TYPE_CHECKING: from ._mymdc.clients import MyMdCClient from .auth.oauth import OAuthClient - from .auth.token_store import TokenStore from .graphql.subscriptions import SubscriptionCursors from .runs.repository import DamnitRepositoryRegistry from .shared.settings import Settings @@ -32,7 +31,6 @@ class AppState: db_sessionmaker: async_sessionmaker[AsyncSession] mymdc_client: MyMdCClient oauth_client: OAuthClient | None # None when auth is disabled - token_store: TokenStore repositories: DamnitRepositoryRegistry subscription_cursors: SubscriptionCursors @@ -61,12 +59,6 @@ def create_mymdc_client(settings: Settings) -> MyMdCClient: raise ValueError(msg) -def create_token_store() -> TokenStore: - from .auth.token_store import InMemoryTokenStore - - return InMemoryTokenStore() - - def create_repositories() -> DamnitRepositoryRegistry: """Registry of per-proposal `DamnitRepository` objects (ADR-005). diff --git a/api/tests/test_auth_routers.py b/api/tests/test_auth_routers.py index bc075423..568ae8b0 100644 --- a/api/tests/test_auth_routers.py +++ b/api/tests/test_auth_routers.py @@ -9,7 +9,6 @@ from damnit_api.auth.oauth import SESSION_COOKIE_KEY, OAuthClient from damnit_api.auth.routers import OAuthController -from damnit_api.auth.token_store import InMemoryTokenStore SERVER_METADATA = { "authorization_endpoint": "https://idp.example/authorize", @@ -40,25 +39,19 @@ def _oauth_config() -> OAuthClient: ) -@pytest.fixture -def token_store(): - return InMemoryTokenStore() - - @pytest.fixture def session_config(): return ServerSideSessionConfig(key=SESSION_COOKIE_KEY) @pytest.fixture -def client(token_store, session_config): - # oauth_config/token_store are app-level dependencies in the composition - # root (main.py); provide fakes at the same layer here. +def client(session_config): + # oauth_config is an app-level dependency in the composition root + # (main.py); provide a fake at the same layer here. with create_test_client( route_handlers=[OAuthController], dependencies={ "oauth_config": Provide(_oauth_config, sync_to_thread=False), - "token_store": Provide(lambda: token_store, sync_to_thread=False), }, session_config=session_config, middleware=[session_config.middleware], @@ -102,9 +95,7 @@ def test_callback_state_mismatch_returns_401(client): assert resp.status_code == 401 -def test_callback_success_stores_session_user_and_token( - client, token_store, mock_oauth_client -): +def test_callback_success_stores_session_user_and_token(client, mock_oauth_client): client.set_session_data({"_oauth_state": "csrf-state"}) resp = client.get( @@ -119,38 +110,40 @@ def test_callback_success_stores_session_user_and_token( session = client.get_session_data() assert session["user"]["sub"] == "user-1" - assert token_store.pop_token_field("user-1", "access_token") == "tok123" - assert token_store.pop_token_field("user-1", "refresh_token") == "ref456" + # Tokens are stored server-side as part of the session data. + assert session["tokens"]["access_token"] == "tok123" # noqa: S105 + assert session["tokens"]["refresh_token"] == "ref456" # noqa: S105 # ── /oauth/logout ──────────────────────────────────────────────────────────── -def test_logout_revokes_tokens_and_clears_session_cookie( - client, token_store, mock_oauth_client -): - token_store.store( - "user-1", - {"access_token": "tok123", "refresh_token": "ref456", "id_token": "idtok789"}, - ) - client.set_session_data({"user": {"sub": "user-1"}}) +def test_logout_revokes_tokens_and_clears_session_cookie(client, mock_oauth_client): + client.set_session_data({ + "user": {"sub": "user-1"}, + "tokens": { + "access_token": "tok123", + "refresh_token": "ref456", + "id_token": "idtok789", + }, + }) resp = client.post("/oauth/logout") assert resp.status_code == 201 assert resp.json()["logout_url"].startswith("https://idp.example/logout") - # Both refresh and access tokens are revoked via the TokenStore. + # Both refresh and access tokens are revoked. assert mock_oauth_client.post.await_count == 2 - assert token_store.pop_token_field("user-1", "access_token") is None - assert token_store.pop_token_field("user-1", "refresh_token") is None # The session cookie is cleared, keyed by the shared session cookie name. set_cookie = resp.headers.get("set-cookie", "") assert f"{SESSION_COOKIE_KEY}=" in set_cookie assert "Max-Age=0" in set_cookie - assert "user" not in client.get_session_data() + session = client.get_session_data() + assert "user" not in session + assert "tokens" not in session # ── x-forwarded-host trust gate ────────────────────────────────────────────── @@ -220,7 +213,7 @@ def test_login_rejects_protocol_relative_redirect_target(client, mock_oauth_clie def test_relative_redirect_carried_through_login_and_callback( - client, token_store, mock_oauth_client + client, mock_oauth_client ): resp = client.get( "/oauth/login", @@ -241,9 +234,7 @@ def test_relative_redirect_carried_through_login_and_callback( assert resp.headers["location"] == "/proposal/1234" -def test_callback_sanitizes_redirect_carried_in_session( - client, token_store, mock_oauth_client -): +def test_callback_sanitizes_redirect_carried_in_session(client, mock_oauth_client): client.set_session_data( { "_oauth_state": "csrf-state", diff --git a/api/tests/test_state.py b/api/tests/test_state.py index dafd75bb..e6bca79b 100644 --- a/api/tests/test_state.py +++ b/api/tests/test_state.py @@ -7,7 +7,6 @@ import pytest from damnit_api.auth.oauth import create_oauth_client -from damnit_api.auth.token_store import InMemoryTokenStore from damnit_api.runs.repository import DamnitRepositoryRegistry from damnit_api.shared.models import ProposalNumber from damnit_api.shared.settings import Settings @@ -79,21 +78,3 @@ def __init__(self, proposal): assert registry.get(ProposalNumber(1234)) is first assert registry.get(ProposalNumber(5678)) is not first assert created == [ProposalNumber(1234), ProposalNumber(5678)] - - -def test_token_store_stores_and_pops_fields(): - store = InMemoryTokenStore() - store.store("sub-1", {"access_token": "a", "id_token": "i"}) - - assert store.pop_token_field("sub-1", "access_token") == "a" - assert store.pop_token_field("sub-1", "access_token") is None # popped - assert store.pop_token_field("unknown-sub", "id_token") is None - - -def test_token_store_drops_entry_once_empty(): - store = InMemoryTokenStore() - store.store("sub-1", {"access_token": "a"}) - - assert store.pop_token_field("sub-1", "access_token") == "a" - # The now-empty entry is removed, not left as a dangling empty dict. - assert "sub-1" not in store._tokens From bfb60ee8c08cc7247798b3bbfd88f55425d12a0f Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Wed, 8 Jul 2026 06:45:42 +0200 Subject: [PATCH 4/5] refactor(api/contextfile): replace alru_cache with response caching --- api/src/damnit_api/contextfile/models.py | 3 - api/src/damnit_api/contextfile/routers.py | 12 +++- api/tests/test_contextfile.py | 69 ++++++++++------------- 3 files changed, 38 insertions(+), 46 deletions(-) diff --git a/api/src/damnit_api/contextfile/models.py b/api/src/damnit_api/contextfile/models.py index 0eeb6cf0..e86d3644 100644 --- a/api/src/damnit_api/contextfile/models.py +++ b/api/src/damnit_api/contextfile/models.py @@ -1,6 +1,5 @@ from typing import Self -import async_lru from anyio import Path as APath from pydantic import BaseModel @@ -9,7 +8,6 @@ class ModifiedTime(BaseModel): lastModified: float # noqa: N815 @classmethod - @async_lru.alru_cache(ttl=5) async def from_file(cls, path: APath) -> Self: stat = await path.stat() return cls(lastModified=stat.st_mtime) @@ -20,7 +18,6 @@ class ContextFile(BaseModel): fileContent: str # noqa: N815 @classmethod - @async_lru.alru_cache(ttl=5) async def from_file(cls, path: APath) -> Self: content = await path.read_text() modified_timestamp = await ModifiedTime.from_file(path) diff --git a/api/src/damnit_api/contextfile/routers.py b/api/src/damnit_api/contextfile/routers.py index f8205b52..252c2630 100644 --- a/api/src/damnit_api/contextfile/routers.py +++ b/api/src/damnit_api/contextfile/routers.py @@ -1,5 +1,5 @@ from anyio import Path as APath -from litestar import Router, get +from litestar import Request, Router, get from litestar.di import Provide from ..metadata.models import ProposalMeta @@ -7,7 +7,13 @@ from . import models -@get("/content") +def _proposal_cache_key(request: Request) -> str: + """Key response-cache entries per proposal so entries stay isolated.""" + proposal_number = request.query_params.get("proposal_number", "") + return f"{request.url.path}:{proposal_number}" + + +@get("/content", cache=5, cache_key_builder=_proposal_cache_key) async def get_content(proposal: ProposalMeta) -> models.ContextFile | None: if proposal.damnit_path is None: return None @@ -16,7 +22,7 @@ async def get_content(proposal: ProposalMeta) -> models.ContextFile | None: ) -@get("/last_modified") +@get("/last_modified", cache=5, cache_key_builder=_proposal_cache_key) async def get_modified(proposal: ProposalMeta) -> models.ModifiedTime | None: if proposal.damnit_path is None: return None diff --git a/api/tests/test_contextfile.py b/api/tests/test_contextfile.py index e1fd0cd5..e7701a06 100644 --- a/api/tests/test_contextfile.py +++ b/api/tests/test_contextfile.py @@ -1,12 +1,10 @@ -import asyncio -import time +import os import pytest from litestar import Router from litestar.di import Provide from litestar.testing import create_test_client -from damnit_api.contextfile import models from damnit_api.contextfile.routers import get_content, get_modified from damnit_api.metadata.models import ProposalMeta @@ -27,6 +25,13 @@ def _stub_proposal(damnit_path: str) -> ProposalMeta: ) +@pytest.fixture +def temp_dir(tmp_path): + file_path = tmp_path / "context.py" + file_path.write_text("initial content") + return tmp_path + + @pytest.fixture def client(temp_dir): test_router = Router( @@ -43,51 +48,35 @@ def client(temp_dir): yield c -@pytest.fixture -def temp_dir(tmp_path): - file_path = tmp_path / "context.py" - file_path.write_text("initial content") - return tmp_path - +def _bump_mtime(path, seconds: float = 60.0) -> None: + stat = path.stat() + os.utime(path, times=(stat.st_atime, stat.st_mtime + seconds)) -@pytest.fixture(autouse=True) -def clear_cache(): - yield - models.ModifiedTime.from_file.cache_clear() - models.ContextFile.from_file.cache_clear() +def test_file_fetching(client): + resp = client.get("/contextfile/content", params={"proposal_number": 1}) + assert resp.status_code == 200 + assert resp.json()["fileContent"] == "initial content" -@pytest.mark.asyncio -async def test_watcher_detects_change(client, temp_dir): - temp_path = temp_dir / "context.py" - resp = client.get("/contextfile/last_modified") +def test_response_cached_within_ttl(client, temp_dir): + resp = client.get("/contextfile/last_modified", params={"proposal_number": 1}) assert resp.status_code == 200 - initial_modified = resp.json()["lastModified"] + initial = resp.json()["lastModified"] - await asyncio.to_thread(temp_path.write_text, "new content") + _bump_mtime(temp_dir / "context.py") - assert await wait_for_change(client, "/contextfile/last_modified", initial_modified) + # Same proposal within the TTL: the cached response is served. + resp = client.get("/contextfile/last_modified", params={"proposal_number": 1}) + assert resp.json()["lastModified"] == initial -@pytest.mark.filterwarnings("ignore::async_lru.AlruCacheLoopResetWarning") -def test_file_fetching(client): - resp = client.get("/contextfile/content") - assert resp.status_code == 200 - assert resp.json()["fileContent"] == "initial content" +def test_cache_entries_isolated_per_proposal(client, temp_dir): + resp = client.get("/contextfile/last_modified", params={"proposal_number": 1}) + initial = resp.json()["lastModified"] + _bump_mtime(temp_dir / "context.py") -async def wait_for_change( - client, - url, - initial_value, - timeout: float = 5.0, # noqa: ASYNC109 -): - start = time.time() - while time.time() - start < timeout: - models.ModifiedTime.from_file.cache_clear() - resp = client.get(url) - if resp.json()["lastModified"] > initial_value: - return True - await asyncio.sleep(0.1) - return False + # A different proposal key misses the cache and sees the new mtime. + resp = client.get("/contextfile/last_modified", params={"proposal_number": 2}) + assert resp.json()["lastModified"] > initial From 107a031ab0733209d11fea80eda95ce81fea5db6 Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Wed, 8 Jul 2026 07:22:54 +0200 Subject: [PATCH 5/5] docs(api/adr): drop token store from ADR-002 after server-side sessions --- api/docs/adr/002-no-global-mutable-state.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/docs/adr/002-no-global-mutable-state.md b/api/docs/adr/002-no-global-mutable-state.md index b716ab02..0b4ed995 100644 --- a/api/docs/adr/002-no-global-mutable-state.md +++ b/api/docs/adr/002-no-global-mutable-state.md @@ -33,7 +33,7 @@ Chosen option: a single frozen `AppState` dataclass built once in the lifespan. - Good: initialisation order and the full dependency set are explicit in one place - Good: missing dependency results in construction error at startup, not a `None` at request time. -- Good: tests build `AppState` with fakes (a mock MyMdC client, an in-memory token store) +- Good: tests build `AppState` with fakes (a mock MyMdC client, a stub OAuth client) - Good: removes/reduces need to monkeypatch modules and clear cache between tests. - Bad: (ish?) dependencies must be specified through signatures instead of imported where needed - This is kind of the whole point, but it does mean there is more code to do the same thing (importing a global is easier/shorter) @@ -43,7 +43,7 @@ Chosen option: a single frozen `AppState` dataclass built once in the lifespan. ### The rules 1. All runtime dependencies stored in a single frozen `AppState` dataclass (`state.py`) which is constructed once in the application lifespan and attached to `app.state`. This currently contains: - - App DB engine/sessionmaker, MyMdC client, OAuth client (`None` when auth is disabled), DAMNIT DB registry, token store, subscription cursors. + - App DB engine/sessionmaker, MyMdC client, OAuth client (`None` when auth is disabled), DAMNIT DB registry, subscription cursors. 2. Each field is built by a pure factory function (`create_*`) taking `Settings` as explicit arguments. No factory reads module state or has side effects beyond constructing its object. 3. There is exactly one composition/setup root: the app entrypoint and its lifespan (target shape: `create_app(settings)`, see the ADR-000 layout). - This is the only place that reads settings to select implementations.