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/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. diff --git a/api/src/damnit_api/auth/dependencies.py b/api/src/damnit_api/auth/dependencies.py index bca3b2e9..e3ff02e5 100644 --- a/api/src/damnit_api/auth/dependencies.py +++ b/api/src/damnit_api/auth/dependencies.py @@ -9,10 +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 .token_store import TokenStore +from .oauth import OAuthClient 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/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..fac8ff85 100644 --- a/api/src/damnit_api/auth/routers.py +++ b/api/src/damnit_api/auth/routers.py @@ -16,9 +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 .token_store import TokenStore +from .oauth import SESSION_COOKIE_KEY, OAuthClient 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/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/src/damnit_api/main.py b/api/src/damnit_api/main.py index b6b7d6c4..529c9ce5 100644 --- a/api/src/damnit_api/main.py +++ b/api/src/damnit_api/main.py @@ -14,42 +14,39 @@ 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 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, provide_app_state, ) 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: @@ -100,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(), ) @@ -110,10 +106,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() @@ -132,9 +140,6 @@ async def lifespan(app: Litestar): "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), @@ -144,6 +149,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/src/damnit_api/state.py b/api/src/damnit_api/state.py index 4917c5e3..a73b7f5d 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,11 @@ if TYPE_CHECKING: from ._mymdc.clients import MyMdCClient - from .auth.token_store import TokenStore + from .auth.oauth import OAuthClient 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: @@ -54,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 @@ -83,24 +59,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 - - return InMemoryTokenStore() - - def create_repositories() -> DamnitRepositoryRegistry: """Registry of per-proposal `DamnitRepository` objects (ADR-005). 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 2fd0a9f8..568ae8b0 100644 --- a/api/tests/test_auth_routers.py +++ b/api/tests/test_auth_routers.py @@ -4,12 +4,11 @@ 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.state import SESSION_COOKIE_KEY, OAuthClient 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 CookieBackendConfig(secret=b"0" * 32, key=SESSION_COOKIE_KEY) + 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_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 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..e6bca79b 100644 --- a/api/tests/test_state.py +++ b/api/tests/test_state.py @@ -6,11 +6,11 @@ import pytest -from damnit_api.auth.token_store import InMemoryTokenStore +from damnit_api.auth.oauth import create_oauth_client 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(): @@ -78,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