diff --git a/.claude/docs/architectural_patterns.md b/.claude/docs/architectural_patterns.md index f7f78fdb..4a5c9c49 100644 --- a/.claude/docs/architectural_patterns.md +++ b/.claude/docs/architectural_patterns.md @@ -83,11 +83,50 @@ Request → Middleware → APIRouter → Endpoint function → Service layer → - All DB access is async: `AsyncSession` injected via `Depends(get_async_session)` - Plugin routes are mounted dynamically by the plugin loader at startup +### Endpoint modules: single file or package + +A module under `sparkth/api/v1/` takes one of two shapes. `sparkth/api/v1/api.py` mounts +each one the same way — `include_router(.router, prefix=...)` — so the shape is +an internal organisation choice and changing it never moves a URL. + +**Single file** (`auth.py`, `llm.py`, `analytics.py`, `file_parser.py`, `user_plugins.py`) +— a module that declares `router` at the top level. The default for a small surface with +no request/response models of its own. + +**Package** (`user/`, `language/`, `permissions/`, `whitelist/`) — reach for this once a +module owns its own pydantic models, or once its routes outgrow one readable file: + +``` +sparkth/api/v1// + __init__.py # exports `router`; registers this domain's exception → HTTP mappings + routes.py # the endpoint functions (a routes/ package when they need splitting) + schemas.py # request/response models owned by this module +``` + +- `__init__.py` re-exports `router` and carries `__all__ = ["router"]`. This is a + deliberate exception to the "avoid re-exports" rule: it keeps `api.py`'s mounting + uniform across both shapes. +- It is also where `register_exception_handler(ExcClass, status_code)` calls live, so a + domain's exception → status mapping sits beside the routes that raise it (see section + 8, and `permissions/__init__.py` / `whitelist/__init__.py`). A package with no domain + exceptions of its own says so in its docstring rather than leaving readers guessing. +- `schemas.py` holds only the models that module owns. Models shared across domains — + `UserBase`, `Token`, `UserLogin`, and `User`, which `auth` returns from register and + login as well as `user/` from `/user/me` — stay in the root `sparkth/schemas.py`; a + package imports them from there rather than duplicating them. Ownership is decided by + who imports the model, not by which routes feel closest to it: pulling a shared model + into a package makes every other domain import that package, and importing any module + from it executes its `__init__.py` and therefore its `routes.py`, which turns a later + import in the other direction into a circular one. +- Route paths come from the prefix in `api.py`, never from the package name, so + converting a file to a package is a pure refactor: the OpenAPI document, the generated + frontend client, and every URL stay byte-identical. + --- ## 4. Dependency Injection via FastAPI `Depends` -**Files:** `sparkth/api/v1/auth.py`, `sparkth/api/v1/user.py`, `sparkth/core/db.py` +**Files:** `sparkth/api/v1/auth.py`, `sparkth/api/v1/user/routes.py`, `sparkth/core/db.py` Auth and DB session are injected uniformly: diff --git a/frontend/lib/api/generated.ts b/frontend/lib/api/generated.ts index a08bbb41..a9d99352 100644 --- a/frontend/lib/api/generated.ts +++ b/frontend/lib/api/generated.ts @@ -544,6 +544,26 @@ export interface paths { patch?: never; trace?: never; }; + "/api/v1/languages": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + /** + * List Languages + * @description Return every supported language and the platform default. + */ + get: operations["list_languages_api_v1_languages_get"]; + put?: never; + post?: never; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/api/v1/llm/configs": { parameters: { query?: never; @@ -1126,11 +1146,9 @@ export interface paths { * Get User * @description Fetch the current authenticated user from the JWT token. * - * ``is_admin`` is derived here from whether the user holds the global ``admin`` - * role; it is not a stored column. - * - * Raises: - * HTTPException: If no user is authenticated. + * ``is_admin`` is derived from whether the user holds the global ``admin`` role; + * it is not a stored column. ``language`` is the raw stored preference — ``None`` + * when the user never chose one. */ get: operations["get_user_api_v1_user_me_get"]; put?: never; @@ -1138,7 +1156,15 @@ export interface paths { delete?: never; options?: never; head?: never; - patch?: never; + /** + * Update User Language + * @description Set or clear the current user's preferred language. + * + * The tag is validated against the supported-language allowlist by + * ``UserLanguageUpdate``, so an unsupported value is a 422 before reaching here. + * An explicit ``null`` clears the preference. + */ + patch: operations["update_user_language_api_v1_user_me_patch"]; trace?: never; }; "/api/v1/whitelist/": { @@ -1939,6 +1965,28 @@ export interface components { /** Team Name */ team_name?: string | null; }; + /** + * SupportedLanguage + * @description One language the platform can generate content in. + */ + SupportedLanguage: { + /** Code */ + code: string; + /** Name */ + name: string; + /** Native Name */ + native_name: string; + }; + /** + * SupportedLanguages + * @description The full allowlist plus the default applied to users who never chose. + */ + SupportedLanguages: { + /** Default */ + default: string; + /** Languages */ + languages: components["schemas"]["SupportedLanguage"][]; + }; /** * SyncFolderRequest * @description Request to sync a Google Drive folder. @@ -2011,6 +2059,8 @@ export interface components { * @default false */ is_admin: boolean; + /** Language */ + language?: string | null; /** Name */ name: string; /** Username */ @@ -2030,6 +2080,18 @@ export interface components { /** Username */ username: string; }; + /** + * UserLanguageUpdate + * @description Body of ``PATCH /user/me``. Validated here so a bad tag is a 422. + * + * ``language`` is required, because ``None`` is a meaningful value here rather + * than a missing one: an explicit ``null`` clears the stored preference, while + * omitting the field is a 422 rather than a no-op. + */ + UserLanguageUpdate: { + /** Language */ + language: string | null; + }; /** UserLogin */ UserLogin: { /** Password */ @@ -3149,6 +3211,26 @@ export interface operations { }; }; }; + list_languages_api_v1_languages_get: { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Successful Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["SupportedLanguages"]; + }; + }; + }; + }; list_llm_configs_api_v1_llm_configs_get: { parameters: { query?: { @@ -4226,6 +4308,39 @@ export interface operations { }; }; }; + update_user_language_api_v1_user_me_patch: { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + requestBody: { + content: { + "application/json": components["schemas"]["UserLanguageUpdate"]; + }; + }; + responses: { + /** @description Successful Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["User"]; + }; + }; + /** @description Validation Error */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["HTTPValidationError"]; + }; + }; + }; + }; list_whitelist_api_v1_whitelist__get: { parameters: { query?: never; diff --git a/sparkth/api/v1/api.py b/sparkth/api/v1/api.py index f62ae093..2f701e89 100644 --- a/sparkth/api/v1/api.py +++ b/sparkth/api/v1/api.py @@ -1,10 +1,11 @@ from fastapi import APIRouter -from sparkth.api.v1 import analytics, auth, file_parser, llm, permissions, user, user_plugins, whitelist +from sparkth.api.v1 import analytics, auth, file_parser, language, llm, permissions, user, user_plugins, whitelist api_router = APIRouter() api_router.include_router(auth.router, prefix="/auth", tags=["auth"]) api_router.include_router(user.router, prefix="/user", tags=["user"]) +api_router.include_router(language.router, prefix="/languages", tags=["Languages"]) api_router.include_router(user_plugins.router, prefix="/user-plugins", tags=["User Plugins"]) api_router.include_router(file_parser.router, prefix="/parser", tags=["File Parser"]) api_router.include_router(whitelist.router, prefix="/whitelist", tags=["Whitelist"]) diff --git a/sparkth/api/v1/language/__init__.py b/sparkth/api/v1/language/__init__.py new file mode 100644 index 00000000..8071df3a --- /dev/null +++ b/sparkth/api/v1/language/__init__.py @@ -0,0 +1,9 @@ +"""Supported-languages API package. + +Exports the router. The package has no domain exceptions of its own, so nothing is +registered with the exception-handler registry here. +""" + +from sparkth.api.v1.language.routes import router + +__all__ = ["router"] diff --git a/sparkth/api/v1/language/routes.py b/sparkth/api/v1/language/routes.py new file mode 100644 index 00000000..6723ca77 --- /dev/null +++ b/sparkth/api/v1/language/routes.py @@ -0,0 +1,31 @@ +"""Read-only endpoint exposing the languages a user may choose from. + +The allowlist is defined once in core config and served from here rather than +duplicated client-side — the frontend language picker and the static-UI +translation layer both read it from this endpoint. + +Unauthenticated, and the only endpoint outside ``auth`` that is: the translation +layer has to render the login, register and password-reset pages, which have no +token yet, and gating it there would force the frontend to carry the duplicate +list this endpoint exists to remove. What it serves is a compile-time constant — +the supported-language table and the platform default — so there is no user data, +no database read, and nothing to enumerate. +""" + +from fastapi import APIRouter + +from sparkth.api.v1.language.schemas import SupportedLanguage, SupportedLanguages +from sparkth.lib.language import SUPPORTED_LANGUAGES +from sparkth.lib.settings import get_settings + +router = APIRouter() + + +@router.get("", response_model=SupportedLanguages) +async def list_languages() -> SupportedLanguages: + """Return every supported language and the platform default.""" + languages = [ + SupportedLanguage(code=code, name=info.name, native_name=info.native_name) + for code, info in SUPPORTED_LANGUAGES.items() + ] + return SupportedLanguages(languages=languages, default=get_settings().DEFAULT_LANGUAGE) diff --git a/sparkth/api/v1/language/schemas.py b/sparkth/api/v1/language/schemas.py new file mode 100644 index 00000000..fc698b33 --- /dev/null +++ b/sparkth/api/v1/language/schemas.py @@ -0,0 +1,18 @@ +"""Pydantic models for the supported-languages API.""" + +from pydantic import BaseModel + + +class SupportedLanguage(BaseModel): + """One language the platform can generate content in.""" + + code: str + name: str + native_name: str + + +class SupportedLanguages(BaseModel): + """The full allowlist plus the default applied to users who never chose.""" + + languages: list[SupportedLanguage] + default: str diff --git a/sparkth/api/v1/user.py b/sparkth/api/v1/user.py deleted file mode 100644 index 96792f3d..00000000 --- a/sparkth/api/v1/user.py +++ /dev/null @@ -1,35 +0,0 @@ -from fastapi import APIRouter, Depends, HTTPException, status -from sqlmodel.ext.asyncio.session import AsyncSession - -from sparkth.core.models.user import User -from sparkth.lib.auth import get_current_user -from sparkth.lib.db import get_async_session -from sparkth.lib.permissions import has_role -from sparkth.lib.permissions.scopes import GLOBAL -from sparkth.schemas import User as UserSchema - -router = APIRouter() - - -@router.get("/me", response_model=UserSchema) -async def get_user( - current_user: User = Depends(get_current_user), - session: AsyncSession = Depends(get_async_session), -) -> UserSchema: - """Fetch the current authenticated user from the JWT token. - - ``is_admin`` is derived here from whether the user holds the global ``admin`` - role; it is not a stored column. - - Raises: - HTTPException: If no user is authenticated. - """ - if not current_user or not current_user.id: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail="No user authenticated.", - ) - - # "global" is the root scope; admin-ness is membership of the admin role there. - is_admin = await has_role(current_user, "admin", GLOBAL, None, session) - return UserSchema.model_validate(current_user).model_copy(update={"is_admin": is_admin}) diff --git a/sparkth/api/v1/user/__init__.py b/sparkth/api/v1/user/__init__.py new file mode 100644 index 00000000..88de6f04 --- /dev/null +++ b/sparkth/api/v1/user/__init__.py @@ -0,0 +1,9 @@ +"""Current-user API package. + +Exports the router. The package has no domain exceptions of its own, so nothing is +registered with the exception-handler registry here. +""" + +from sparkth.api.v1.user.routes import router + +__all__ = ["router"] diff --git a/sparkth/api/v1/user/routes.py b/sparkth/api/v1/user/routes.py new file mode 100644 index 00000000..541abc22 --- /dev/null +++ b/sparkth/api/v1/user/routes.py @@ -0,0 +1,86 @@ +"""The authenticated caller's own profile: ``GET`` and ``PATCH /user/me``. + +Both endpoints serve the caller and nobody else. Neither takes a user id, so a +request can only ever read or write its own row. + +``is_admin`` is not a stored column: it is derived per response from whether the +user holds the ``admin`` role at the global scope, and filled in during +serialization rather than read off the model. + +``PATCH`` carries the preferred language. ``language`` is required in the body +because ``None`` is a meaningful value rather than a missing one — an explicit +``null`` clears a previous choice, so the platform default applies again, while +omitting the field is a 422. +""" + +from fastapi import APIRouter, Depends, HTTPException, status +from sqlmodel.ext.asyncio.session import AsyncSession + +from sparkth.api.v1.user.schemas import UserLanguageUpdate +from sparkth.core.models.user import User +from sparkth.lib.auth import get_current_user +from sparkth.lib.db import get_async_session +from sparkth.lib.permissions import has_role +from sparkth.lib.permissions.scopes import GLOBAL +from sparkth.schemas import User as UserSchema + +router = APIRouter() + + +async def _to_schema(user: User, session: AsyncSession) -> UserSchema: + """Serialize a user, filling in the ``is_admin`` flag the model does not store. + + "global" is the root scope; admin-ness is membership of the admin role there. + """ + is_admin = await has_role(user, "admin", GLOBAL, None, session) + return UserSchema.model_validate(user).model_copy(update={"is_admin": is_admin}) + + +@router.get("/me", response_model=UserSchema) +async def get_user( + current_user: User = Depends(get_current_user), + session: AsyncSession = Depends(get_async_session), +) -> UserSchema: + """Fetch the current authenticated user from the JWT token. + + ``is_admin`` is derived from whether the user holds the global ``admin`` role; + it is not a stored column. ``language`` is the raw stored preference — ``None`` + when the user never chose one. + """ + return await _to_schema(current_user, session) + + +@router.patch("/me", response_model=UserSchema) +async def update_user_language( + update: UserLanguageUpdate, + current_user: User = Depends(get_current_user), + session: AsyncSession = Depends(get_async_session), +) -> UserSchema: + """Set or clear the current user's preferred language. + + The tag is validated against the supported-language allowlist by + ``UserLanguageUpdate``, so an unsupported value is a 422 before reaching here. + An explicit ``null`` clears the preference. + """ + # Load the row this request intends to write instead of mutating the injected + # principal. In production ``get_current_user`` resolves the same request-scoped + # session, so this is a cheap identity-map hit — the re-fetch is a deliberate + # guard, not an optimisation. Its job is to make a write against an instance that + # is *not* attached to ``session`` fail loudly, rather than have ``commit()`` + # persist nothing and the endpoint still answer 200; that failure mode shows up + # neither in the response nor in a test that asserts on it. + user = await session.get(User, current_user.id) + if user is None: + # Unreachable while ``get_current_user`` is the only source of a principal — + # it already answers 401 when the row is gone. ``HTTPException`` here mirrors + # that dependency: an authenticated principal without a row is an + # authentication-boundary condition, not a domain error to route through the + # exception-handler registry. + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="User not found") + + user.language = update.language + # updated_at has a default_factory but no onupdate, so the bump is manual. + user.update_timestamp() + await session.commit() + + return await _to_schema(user, session) diff --git a/sparkth/api/v1/user/schemas.py b/sparkth/api/v1/user/schemas.py new file mode 100644 index 00000000..43f95096 --- /dev/null +++ b/sparkth/api/v1/user/schemas.py @@ -0,0 +1,29 @@ +"""Pydantic models owned by the current-user API. + +The ``User`` response model is deliberately *not* here: ``auth`` returns it from register +and login too, which makes it shared across domains, so it stays in the root +``sparkth.schemas`` alongside ``UserBase`` and ``Token``. Only the request model below, +which nothing outside these routes sends, belongs to this package. +""" + +from pydantic import BaseModel, field_validator + +from sparkth.lib.language import is_supported_language + + +class UserLanguageUpdate(BaseModel): + """Body of ``PATCH /user/me``. Validated here so a bad tag is a 422. + + ``language`` is required, because ``None`` is a meaningful value here rather + than a missing one: an explicit ``null`` clears the stored preference, while + omitting the field is a 422 rather than a no-op. + """ + + language: str | None + + @field_validator("language") + @classmethod + def _check_supported(cls, v: str | None) -> str | None: + if v is not None and not is_supported_language(v): + raise ValueError(f"Unsupported language: {v}") + return v diff --git a/sparkth/schemas.py b/sparkth/schemas.py index d0545a23..8476a3d5 100644 --- a/sparkth/schemas.py +++ b/sparkth/schemas.py @@ -9,18 +9,6 @@ class UserBase(BaseModel): email: EmailStr -class UserCreate(UserBase): - name: str - username: str - password: str - - @field_validator("password") - @classmethod - def _check_password(cls, v: str) -> str: - validate_password_complexity(v) - return v - - class User(UserBase): id: int name: str @@ -30,10 +18,26 @@ class User(UserBase): # register) report a non-admin; /user/me computes and sets the real value. is_admin: bool = False email_verified: bool + # The raw stored preference: None means the user never chose one and the + # platform default applies. Deliberately not resolved here so the frontend can + # tell "never chose" from "chose English". + language: str | None = None model_config = ConfigDict(from_attributes=True) +class UserCreate(UserBase): + name: str + username: str + password: str + + @field_validator("password") + @classmethod + def _check_password(cls, v: str) -> str: + validate_password_complexity(v) + return v + + class Token(BaseModel): access_token: str token_type: str diff --git a/tests/api/v1/test_language.py b/tests/api/v1/test_language.py new file mode 100644 index 00000000..248cc63c --- /dev/null +++ b/tests/api/v1/test_language.py @@ -0,0 +1,40 @@ +"""Tests for the supported-languages endpoint.""" + +from httpx import AsyncClient + +from sparkth.core.models.user import User + + +async def test_lists_every_supported_language(client: AsyncClient, current_user: User) -> None: + response = await client.get("/api/v1/languages") + + assert response.status_code == 200 + codes = [language["code"] for language in response.json()["languages"]] + assert sorted(codes) == ["en", "es", "fr"] + + +async def test_each_entry_carries_both_display_names(client: AsyncClient, current_user: User) -> None: + response = await client.get("/api/v1/languages") + + spanish = next(lang for lang in response.json()["languages"] if lang["code"] == "es") + assert spanish["name"] == "Spanish" + assert spanish["native_name"] == "Español" + + +async def test_reports_the_platform_default(client: AsyncClient, current_user: User) -> None: + response = await client.get("/api/v1/languages") + + assert response.json()["default"] == "en" + + +async def test_does_not_require_authentication(client: AsyncClient) -> None: + """The login, register and password-reset pages need this list before any token exists. + + Deliberately omits the ``current_user`` fixture: without it no + ``get_current_user`` override is installed, so a gate on this route would run + for real and reject the request. + """ + response = await client.get("/api/v1/languages") + + assert response.status_code == 200 + assert response.json()["default"] == "en" diff --git a/tests/api/v1/test_user.py b/tests/api/v1/test_user.py index c308d4ce..a2407f24 100644 --- a/tests/api/v1/test_user.py +++ b/tests/api/v1/test_user.py @@ -1,20 +1,29 @@ -"""Tests for the /user/me endpoint, focused on the computed is_admin flag. +"""Tests for the /user/me endpoint: the computed is_admin flag and the +preferred-language column exposed via GET and PATCH. is_admin is not a stored column — it is derived from whether the user holds the -global ``admin`` role, so these tests seed role assignments and assert the -endpoint reflects them. +global ``admin`` role, so those tests seed role assignments and assert the +endpoint reflects them. The language tests cover reading the raw stored +preference and writing it through PATCH, including the allowlist validation that +keeps unsupported tags out of the database, the auth gate on the write endpoint, +and that a PATCH only ever touches the caller's own row. + +Two of them guard the write itself rather than its result: that the endpoint +refuses a principal with no row instead of reporting success for a write it never +made, and that a successful PATCH advances ``updated_at``. """ from typing import cast -from fastapi import FastAPI +from fastapi import Depends, FastAPI from httpx import ASGITransport, AsyncClient -from sqlalchemy.orm import make_transient +from sqlmodel import select from sqlmodel.ext.asyncio.session import AsyncSession from sparkth.core.models.user import User from sparkth.core.permissions.models import Role from sparkth.lib.auth import get_current_user +from sparkth.lib.db import get_async_session from sparkth.lib.permissions import assign_role from sparkth.lib.permissions.scopes import GLOBAL, PermissionScope @@ -32,19 +41,23 @@ async def _create_user(session: AsyncSession, username: str) -> User: def _override_current_user(client: AsyncClient, user: User) -> None: + """Stand in for ``get_current_user``, resolved on the request's own session. + + Production ``get_current_user`` takes ``session: AsyncSession = + Depends(get_async_session)`` and looks the row up by JWT subject; FastAPI + caches that dependency per request, so the returned user is attached to the + very same session the route body uses. This override takes the same + dependency for the same reason: a value one request writes (e.g. a PATCHed + ``language``) must be visible to a later GET on the same client, and a + detached snapshot frozen at override time would not be. + """ transport = cast(ASGITransport, client._transport) app_instance = cast(FastAPI, transport.app) - snapshot = User( - id=user.id, - name=user.name, - username=user.username, - email=user.email, - hashed_password=user.hashed_password, - ) - make_transient(snapshot) + user_id = user.id - async def override() -> User: - return snapshot + async def override(session: AsyncSession = Depends(get_async_session)) -> User: + result = await session.exec(select(User).where(User.id == user_id)) + return result.one() app_instance.dependency_overrides[get_current_user] = override @@ -87,3 +100,174 @@ async def test_me_admin_role_at_other_scope_does_not_grant_global_admin( assert response.status_code == 200 assert response.json()["is_admin"] is False + + +async def test_me_reports_no_language_for_a_user_who_never_chose(client: AsyncClient, session: AsyncSession) -> None: + user = await _create_user(session, "nolang") + await session.commit() + _override_current_user(client, user) + + response = await client.get("/api/v1/user/me") + + assert response.status_code == 200 + assert response.json()["language"] is None + + +async def test_patch_me_requires_authentication(client: AsyncClient) -> None: + """PATCH is a write endpoint; it must be gated the same as GET. + + Deliberately installs no ``get_current_user`` override, so the real dependency + runs and rejects the unauthenticated request. Nothing has to be popped first: + the autouse ``_clear_dependency_overrides`` fixture clears every override after + each test, so none can leak in from an earlier one. + """ + response = await client.patch("/api/v1/user/me", json={"language": "es"}) + + assert response.status_code == 401 + + +async def test_patch_me_stores_the_chosen_language(client: AsyncClient, session: AsyncSession) -> None: + user = await _create_user(session, "picker") + await session.commit() + _override_current_user(client, user) + + response = await client.patch("/api/v1/user/me", json={"language": "es"}) + + assert response.status_code == 200 + assert response.json()["language"] == "es" + + # Read it back through the API — the choice must have been persisted, not just echoed. + assert (await client.get("/api/v1/user/me")).json()["language"] == "es" + + +async def test_patch_me_refuses_a_principal_with_no_row_instead_of_silently_writing_nothing( + client: AsyncClient, session: AsyncSession, current_user: User +) -> None: + """A detached principal must never produce a silent, successful no-op write. + + The shared ``current_user`` fixture yields a transient ``User`` that was never + added to a session. If the endpoint mutates the injected principal instead of + re-fetching the row it means to write, ``commit()`` persists nothing and the + caller still gets a 200 describing a change that never happened — invisible in + the response and in any test asserting on it. Re-fetching turns that into an + honest 404. + """ + response = await client.patch("/api/v1/user/me", json={"language": "es"}) + + assert response.status_code == 404 + assert (await session.exec(select(User))).all() == [] + + +async def test_patch_me_advances_updated_at(client: AsyncClient, session: AsyncSession) -> None: + """A successful PATCH must bump ``updated_at``. + + ``TimestampedModel.updated_at`` has a ``default_factory`` but no ``onupdate``, + so nothing bumps it unless the write path calls ``update_timestamp()``. Both + timestamps are read back from the database so they are comparable on SQLite, + which drops the UTC offset the in-memory value carries. + """ + user = await _create_user(session, "stamped") + await session.commit() + await session.refresh(user) + before = user.updated_at + _override_current_user(client, user) + + response = await client.patch("/api/v1/user/me", json={"language": "es"}) + + assert response.status_code == 200 + await session.refresh(user) + assert user.updated_at > before + + +async def test_patch_me_only_updates_the_authenticated_users_row(client: AsyncClient, session: AsyncSession) -> None: + """A PATCH must never write to a row other than the caller's own.""" + caller = await _create_user(session, "caller") + bystander = await _create_user(session, "bystander") + await session.commit() + _override_current_user(client, caller) + + response = await client.patch("/api/v1/user/me", json={"language": "fr"}) + + assert response.status_code == 200 + assert response.json()["language"] == "fr" + + _override_current_user(client, bystander) + bystander_response = await client.get("/api/v1/user/me") + + assert bystander_response.json()["language"] is None + + +async def test_patch_me_rejects_an_unsupported_language(client: AsyncClient, session: AsyncSession) -> None: + user = await _create_user(session, "german") + await session.commit() + _override_current_user(client, user) + + response = await client.patch("/api/v1/user/me", json={"language": "de"}) + + assert response.status_code == 422 + assert (await client.get("/api/v1/user/me")).json()["language"] is None + + +async def test_patch_me_rejects_a_wellformed_but_unsupported_tag(client: AsyncClient, session: AsyncSession) -> None: + """A syntactically valid BCP 47 tag outside the allowlist must still be a 422. + + ``is_supported_language`` is an exact, case-sensitive match against the + allowlist keys — it has no subtag or case handling, so a value like + "en-US" or "EN" is not recognised as "en". This guards the write path: a bad + tag must be rejected here rather than silently normalised and stored. + """ + user = await _create_user(session, "regiontag") + await session.commit() + _override_current_user(client, user) + + response = await client.patch("/api/v1/user/me", json={"language": "en-US"}) + + assert response.status_code == 422 + assert (await client.get("/api/v1/user/me")).json()["language"] is None + + response = await client.patch("/api/v1/user/me", json={"language": "EN"}) + + assert response.status_code == 422 + assert (await client.get("/api/v1/user/me")).json()["language"] is None + + +async def test_patch_me_replaces_a_previous_choice(client: AsyncClient, session: AsyncSession) -> None: + user = await _create_user(session, "switcher") + await session.commit() + _override_current_user(client, user) + + await client.patch("/api/v1/user/me", json={"language": "es"}) + response = await client.patch("/api/v1/user/me", json={"language": "fr"}) + + assert response.status_code == 200 + assert (await client.get("/api/v1/user/me")).json()["language"] == "fr" + + +async def test_patch_me_clears_the_language_with_null(client: AsyncClient, session: AsyncSession) -> None: + """Clearing it hands the user back to the platform default at runtime.""" + user = await _create_user(session, "clearer") + await session.commit() + _override_current_user(client, user) + + await client.patch("/api/v1/user/me", json={"language": "fr"}) + response = await client.patch("/api/v1/user/me", json={"language": None}) + + assert response.status_code == 200 + assert response.json()["language"] is None + assert (await client.get("/api/v1/user/me")).json()["language"] is None + + +async def test_patch_me_preserves_the_computed_admin_flag(client: AsyncClient, session: AsyncSession) -> None: + user = await _create_user(session, "adminpicker") + assert user.id is not None + session.add(Role(name="admin")) + await session.flush() + await assign_role(user.id, "admin", GLOBAL, None, session) + await session.commit() + _override_current_user(client, user) + + response = await client.patch("/api/v1/user/me", json={"language": "fr"}) + + assert response.status_code == 200 + assert response.json()["is_admin"] is True + assert response.json()["language"] == "fr" diff --git a/tests/permissions/test_group_resolution.py b/tests/permissions/test_group_resolution.py index 8127eb73..eb0abf72 100644 --- a/tests/permissions/test_group_resolution.py +++ b/tests/permissions/test_group_resolution.py @@ -95,7 +95,7 @@ async def test_soft_deleted_group_assignment_drops_the_grant(session: AsyncSessi async def test_has_role_true_via_group_grant(session: AsyncSession) -> None: - # api/v1/user.py derives is_admin from has_role, so admin-via-group must count. + # api/v1/user/routes.py derives is_admin from has_role, so admin-via-group must count. user = await make_user(session, "alice") role = await make_role(session, "admin", []) await make_group_grant(session, user, role, GLOBAL.name, None)