From adb7c8c445a0ebc15222c5efd2b6fc1b47583ed3 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Thu, 1 Oct 2026 21:22:27 +0530 Subject: [PATCH 1/4] Owner-granted data shares: let named users at one user's per-user data Grants say who may open an app; a share says who may act on someone else's data in it. ShareStore (auth/shares.py) keeps (app, owner, grantee) records with an rw/ro access level and a grantee-facing label, beside grants under the platform store. A share names existing accounts only and is removed with either account (admin delete_user cascades). - /auth/shares/{app}: the signed-in owner lists, grants and revokes; a grantee can leave. /_admin/api/shares and `enlace-auth share|list-shares| revoke-share` let an admin manage shares for an owner who cannot (a child). - The per-user store honours ?owner= with an active share (404 otherwise, writes need rw), gains a list route (GET /api/{app}/store?prefix=, walking only the owner's directory, capped), ETags and If-Match / If-None-Match conditional writes (412 with the current value), and CSRF on writes (CSRFMiddleware.enforce_prefixes) although /api/ is otherwise exempt. - An app gets a per-user store when it is protected:user or sets `user_store = true` (needs enlace>=0.1.42), so a public app can keep data for its signed-in visitors. Design and its adversarial review: misc/docs/decisions/0001-owner-granted-data-shares.md. --- README.md | 15 + enlace_auth/__main__.py | 94 +++++++ enlace_auth/admin/routes.py | 52 ++++ enlace_auth/auth/__init__.py | 4 + enlace_auth/auth/middleware.py | 9 +- enlace_auth/auth/share_routes.py | 100 +++++++ enlace_auth/auth/shares.py | 228 +++++++++++++++ enlace_auth/plugin.py | 27 +- enlace_auth/stores/backends.py | 22 ++ enlace_auth/stores/middleware.py | 139 ++++++++-- enlace_auth/stores/prefixed.py | 16 ++ .../0001-owner-granted-data-shares.md | 83 ++++++ pyproject.toml | 2 +- tests/test_shares.py | 260 ++++++++++++++++++ 14 files changed, 1021 insertions(+), 30 deletions(-) create mode 100644 enlace_auth/auth/share_routes.py create mode 100644 enlace_auth/auth/shares.py create mode 100644 misc/docs/decisions/0001-owner-granted-data-shares.md create mode 100644 tests/test_shares.py diff --git a/README.md b/README.md index 1b3ed33..a390639 100644 --- a/README.md +++ b/README.md @@ -214,6 +214,21 @@ rejected — it would have no additive effect and would unintentionally restrict open app. To remove a user listed in `allowed_users`, edit `app.toml` (that layer is intentionally code-managed); the admin panel manages the runtime layer. +## Owner-granted data shares + +Grants say who may *open* an app. A **share** says who may act on *someone else's* data in it: in app `A`, the owner's per-user store is readable (and, by default, writable) by a named grantee, signed in as themselves. Use it for data that belongs to one person and is looked after by others — a child's practice log kept by her parents. + +```bash +enlace-auth share practice kid@example.com parent@example.com --label Kid # admin, from the server +enlace-auth share practice kid@example.com teacher@example.com --access ro +enlace-auth list-shares +enlace-auth revoke-share practice kid@example.com parent@example.com +``` + +A signed-in owner manages their own shares at `/auth/shares/{app}` (`GET`, `PUT …/{grantee}`, `DELETE …/{grantee}`; a grantee leaves with `DELETE …/received/{owner}`); an admin uses `/_admin/api/shares`. A grantee then reads the owner's data with `?owner=` on the per-user store: `GET /api/{app}/store?prefix=…&owner=…` lists it, and every route takes `If-Match` so two devices cannot silently overwrite each other. A share names existing accounts only and is removed with either account. + +An app gets a per-user store when its `access` is `protected:user`, or when its `app.toml` sets `user_store = true` — which lets a **public** app keep data for the visitors who sign in while staying open to everyone else. The design and its review are in [`misc/docs/decisions/0001-owner-granted-data-shares.md`](misc/docs/decisions/0001-owner-granted-data-shares.md). + ## Doctor checks ```python diff --git a/enlace_auth/__main__.py b/enlace_auth/__main__.py index c216e49..725f415 100644 --- a/enlace_auth/__main__.py +++ b/enlace_auth/__main__.py @@ -396,6 +396,97 @@ def revoke_grant(app_id: str, email: str, *, toml: str = "platform.toml"): sys.exit(1) +def _load_share_store(toml_path: Path = Path("platform.toml")): + """Open the platform's owner-granted data shares, checking accounts against the user store.""" + import os + + from enlace_auth.auth.shares import ShareStore + from enlace_auth.stores import make_file_store_factory + + config = PlatformConfig.from_toml(toml_path) + auth = coerce_auth_config(config.auth) + factory = make_file_store_factory(auth.stores.path) + users = factory("users") + root = Path(os.path.expanduser(auth.stores.path)) / "shares" + return ShareStore(factory("shares"), root=root, account_exists=users.__contains__) + + +def share( + app_id: str, + owner: str, + grantee: str, + *, + access: str = "rw", + label: str = None, + expires: str = None, + toml: str = "platform.toml", +): + """Let GRANTEE act on OWNER's per-user data in an app (an owner-granted share, made by an admin). + + Both accounts must exist. Re-sharing the same pair replaces the share. + + Args: + app_id: The app name. + owner: Email of the account whose data is shared. + grantee: Email of the account given access. + access: "rw" (read-write, default) or "ro" (read-only). + label: How the grantee sees this space (e.g. a first name). + expires: Optional expiry — a date (YYYY-MM-DD, end of day UTC) or full + ISO-8601 timestamp. Omit for a non-expiring share. + toml: Path to platform.toml (default: ./platform.toml). + """ + from enlace_auth.auth.grants import GrantError, parse_expires_at + from enlace_auth.auth.shares import ShareError + + store = _load_share_store(Path(toml)) + try: + record = store.share( + app_id, owner, grantee, access=access, label=label, + expires_at=parse_expires_at(expires), granted_by="cli", + ) + except (GrantError, ShareError) as e: + print(str(e), file=sys.stderr) + sys.exit(1) + print( + f"Shared {record['owner']}'s {record['app_id']} data with {record['grantee']} " + f"({record['access']})." + ) + + +def revoke_share(app_id: str, owner: str, grantee: str, *, toml: str = "platform.toml"): + """Revoke the share OWNER → GRANTEE in an app. + + Args: + app_id: The app name. + owner: Email of the account whose data was shared. + grantee: Email of the account that had access. + toml: Path to platform.toml (default: ./platform.toml). + """ + if _load_share_store(Path(toml)).revoke(app_id, owner, grantee): + print(f"Revoked {grantee.lower()} from {owner.lower()}'s {app_id} data.") + else: + print(f"No share {owner.lower()} -> {grantee.lower()} on {app_id}.", file=sys.stderr) + sys.exit(1) + + +def list_shares(*, json: bool = False, toml: str = "platform.toml"): + """List every owner-granted data share. + + Args: + json: Output as JSON. + toml: Path to platform.toml (default: ./platform.toml). + """ + shares = _load_share_store(Path(toml)).list_all() + if json: + print(json_module.dumps(shares, indent=2)) + return + if not shares: + print("No shares.") + return + for r in sorted(shares, key=lambda r: (r["app_id"], r["owner"], r["grantee"])): + print(f"{r['app_id']:<20} {r['owner']} -> {r['grantee']} ({r.get('access')})") + + def list_grants(*, app: str = None, json: bool = False, toml: str = "platform.toml"): """List runtime grants, optionally filtered to a single app. @@ -527,6 +618,9 @@ def revoke_connector_session( grant, revoke_grant, list_grants, + share, + revoke_share, + list_shares, list_connector_sessions, revoke_connector_session, ] diff --git a/enlace_auth/admin/routes.py b/enlace_auth/admin/routes.py index 700eed5..40280f0 100644 --- a/enlace_auth/admin/routes.py +++ b/enlace_auth/admin/routes.py @@ -48,6 +48,7 @@ from pydantic import BaseModel, EmailStr from enlace_auth.auth.grants import GrantError, parse_expires_at +from enlace_auth.auth.shares import ShareError from enlace_auth.auth.passwords import hash_password from enlace_auth.auth.reset_tokens import ( DEFAULT_HANDOFF_TTL, @@ -65,6 +66,15 @@ class _ResetPasswordBody(BaseModel): password: str +class _CreateShareBody(BaseModel): + app_id: str + owner: EmailStr + grantee: EmailStr + access: str = "rw" + label: Optional[str] = None + expires_at: Optional[str] = None + + class _CreateGrantBody(BaseModel): app_id: str email: EmailStr @@ -82,6 +92,8 @@ def make_admin_router( apps: list = (), grant_store=None, # GrantStore — optional; grant endpoints inert without it protected_user_apps: Iterable[str] = (), + share_store=None, # ShareStore — optional; share endpoints inert without it + store_apps: Iterable[str] = (), signing_key: Optional[str] = None, reset_link_ttl: int = DEFAULT_HANDOFF_TTL, resource_allowlist: Optional[Mapping[str, list[str]]] = None, @@ -131,6 +143,7 @@ def make_admin_router( apps_snapshot = list(apps) app_by_name = {a.name: a for a in apps_snapshot} protected_set = frozenset(protected_user_apps) + store_set = frozenset(store_apps) def _oauth_gates() -> dict[str, list[dict]]: """Index the OAuth resource allow-list by the app route it belongs to. @@ -221,6 +234,10 @@ async def delete_user(email: str, request: Request) -> dict[str, Any]: del user_store[target] except KeyError: raise HTTPException(status_code=404, detail="User not found") + # A share never outlives an account it names (ADR 0001 §2): the email is + # free to be registered again by someone else. + if share_store is not None: + share_store.remove_account(target) # Sessions are not checked against the user store on each request, so # a deleted account keeps working until its cookie expires unless its # sessions go too. (An actor deleting themselves is logged out.) @@ -372,6 +389,41 @@ async def create_grant(body: _CreateGrantBody, request: Request) -> dict[str, An raise HTTPException(status_code=422, detail=str(e)) return {"ok": True, "grant": record} + @router.get("/shares") + async def list_shares(request: Request) -> dict[str, Any]: + _require_admin(request) + if share_store is None: + raise HTTPException(status_code=503, detail="Shares store unavailable") + return {"shares": share_store.list_all(), "apps": sorted(store_set)} + + @router.post("/shares") + async def create_share(body: _CreateShareBody, request: Request) -> dict[str, Any]: + actor = _require_admin(request) + if share_store is None: + raise HTTPException(status_code=503, detail="Shares store unavailable") + app_id = body.app_id.strip() + if app_id not in store_set: + raise HTTPException(status_code=404, detail=f"No per-user store for app {app_id!r}") + try: + record = share_store.share( + app_id, body.owner, body.grantee, access=body.access, label=body.label, + expires_at=parse_expires_at(body.expires_at), granted_by=actor, + ) + except ShareError as e: + raise HTTPException(status_code=409 if "No account" in str(e) else 422, detail=str(e)) + except GrantError as e: + raise HTTPException(status_code=422, detail=str(e)) + return {"ok": True, "share": record} + + @router.delete("/shares/{app_id}/{owner}/{grantee}") + async def revoke_share(app_id: str, owner: str, grantee: str, request: Request) -> dict[str, Any]: + _require_admin(request) + if share_store is None: + raise HTTPException(status_code=503, detail="Shares store unavailable") + if not share_store.revoke(app_id, owner, grantee): + raise HTTPException(status_code=404, detail="Share not found") + return {"ok": True} + @router.delete("/grants/{app_id}/{email}") async def revoke_grant(app_id: str, email: str, request: Request) -> dict[str, Any]: _require_admin(request) diff --git a/enlace_auth/auth/__init__.py b/enlace_auth/auth/__init__.py index b6b2654..209181a 100644 --- a/enlace_auth/auth/__init__.py +++ b/enlace_auth/auth/__init__.py @@ -9,6 +9,7 @@ - ``CSRFMiddleware`` — signed double-submit CSRF. - ``SessionStore`` — MutableMapping-backed session storage. - ``GrantStore`` — MutableMapping-backed runtime per-app access grants. +- ``ShareStore`` — owner-granted data shares (who may act on whose per-user data). - ``hash_password`` / ``verify_password`` — argon2id helpers. - ``make_auth_router`` — FastAPI router for ``/auth/*`` endpoints. """ @@ -23,6 +24,7 @@ from enlace_auth.auth.passwords import hash_password, verify_password from enlace_auth.auth.routes import make_auth_router from enlace_auth.auth.sessions import SessionStore +from enlace_auth.auth.shares import ShareError, ShareStore __all__ = [ "AccessRule", @@ -30,6 +32,8 @@ "GrantStore", "PlatformAuthMiddleware", "SessionStore", + "ShareError", + "ShareStore", "hash_password", "make_auth_router", "parse_expires_at", diff --git a/enlace_auth/auth/middleware.py b/enlace_auth/auth/middleware.py index e63c2a2..be8618c 100644 --- a/enlace_auth/auth/middleware.py +++ b/enlace_auth/auth/middleware.py @@ -394,10 +394,15 @@ def __init__( "/auth/login/", "/api/", ), + enforce_prefixes: Iterable[str] = (), ): + """``enforce_prefixes`` are checked even under an exempt prefix: the per-user + store routes (``/api/{app}/store``) sit under ``/api/`` but carry data a + share can expose to someone else's cookie, so they never skip the check.""" self.app = app self._signing_key = signing_key self._cookie = cookie_name + self._enforce = tuple(p.rstrip("/") for p in enforce_prefixes) self._header = header_name.lower().encode("latin-1") self._exempt = tuple(exempt_prefixes) @@ -410,7 +415,9 @@ async def __call__(self, scope, receive, send): path = scope.get("path", "/") # Exempt paths skip the check entirely. - is_exempt = any(path.startswith(p) for p in self._exempt) + is_exempt = any(path.startswith(p) for p in self._exempt) and not any( + path == p or path.startswith(p + "/") for p in self._enforce + ) cookies = _get_cookies(scope) existing = cookies.get(self._cookie) diff --git a/enlace_auth/auth/share_routes.py b/enlace_auth/auth/share_routes.py new file mode 100644 index 0000000..ca3cc83 --- /dev/null +++ b/enlace_auth/auth/share_routes.py @@ -0,0 +1,100 @@ +"""``/auth/shares/*`` — the signed-in user manages the shares of their own data. + +The owner side of a share is always the signed-in user: there is no route by which +a grantee can grant onward, or anyone can share someone else's data (an admin does +that through ``/_admin/api/shares`` or the CLI). These routes live under ``/auth/``, +so the platform's double-submit CSRF check covers every write. + +- ``GET /auth/shares/{app_id}`` → ``{"owner", "granted": [...], "received": [...]}`` +- ``PUT /auth/shares/{app_id}/{grantee}`` (body ``{"access"?, "label"?, "expires_at"?}``) +- ``DELETE /auth/shares/{app_id}/received/{owner}`` — a grantee leaves a share +- ``DELETE /auth/shares/{app_id}/{grantee}`` — the owner revokes one + +See ``misc/docs/decisions/0001-owner-granted-data-shares.md``. +""" + +from __future__ import annotations + +from typing import Any, Callable, Iterable, Optional, Union + +from fastapi import APIRouter, HTTPException, Request +from pydantic import BaseModel + +from enlace_auth.auth.grants import GrantError, parse_expires_at +from enlace_auth.auth.shares import ShareError, ShareStore + + +class _ShareBody(BaseModel): + access: str = "rw" + label: Optional[str] = None + # A date ("YYYY-MM-DD", end of day UTC) or full ISO-8601 timestamp; null = never. + expires_at: Optional[str] = None + + +def make_share_router( + *, + share_store: ShareStore, + store_apps: Union[Iterable[str], Callable[[], Iterable[str]]], +) -> APIRouter: + """Build the ``/auth/shares`` router. + + ``store_apps`` names the apps that have a per-user store (``protected:user`` + apps and ``user_store = true`` apps); a share in any other app is refused, + since there would be nothing to share. A callable is re-read per request. + """ + router = APIRouter(prefix="/auth/shares") + + def _apps() -> set[str]: + return set(store_apps() if callable(store_apps) else store_apps) + + def _me(request: Request) -> str: + user = getattr(request.state, "user_id", None) + if not user or user == "shared": + raise HTTPException(status_code=401, detail="Not authenticated") + return str(user).lower() + + def _app(app_id: str) -> str: + if app_id not in _apps(): + raise HTTPException(status_code=404, detail=f"No per-user store for app {app_id!r}") + return app_id + + @router.get("/{app_id}") + async def list_shares(app_id: str, request: Request) -> dict[str, Any]: + me, app_id = _me(request), _app(app_id) + return { + "owner": me, + "granted": share_store.granted(app_id, me), + "received": share_store.received(app_id, me), + } + + @router.put("/{app_id}/{grantee}") + async def put_share(app_id: str, grantee: str, body: _ShareBody, request: Request) -> dict[str, Any]: + me, app_id = _me(request), _app(app_id) + try: + record = share_store.share( + app_id, me, grantee, access=body.access, label=body.label, + expires_at=parse_expires_at(body.expires_at), granted_by=me, + ) + except ShareError as e: + status = 409 if "No account" in str(e) else 422 + raise HTTPException(status_code=status, detail=str(e)) + except GrantError as e: + raise HTTPException(status_code=422, detail=str(e)) + return {"ok": True, "share": record} + + # Registered before "/{app_id}/{grantee}" so "received" is never read as a grantee. + @router.delete("/{app_id}/received/{owner}") + async def leave_share(app_id: str, owner: str, request: Request) -> dict[str, Any]: + me, app_id = _me(request), _app(app_id) + if not share_store.revoke(app_id, owner, me): + raise HTTPException(status_code=404, detail="Share not found") + return {"ok": True} + + @router.delete("/{app_id}/{grantee}") + async def revoke_share(app_id: str, grantee: str, request: Request) -> dict[str, Any]: + me, app_id = _me(request), _app(app_id) + if not share_store.revoke(app_id, me, grantee): + raise HTTPException(status_code=404, detail="Share not found") + return {"ok": True} + + return router diff --git a/enlace_auth/auth/shares.py b/enlace_auth/auth/shares.py new file mode 100644 index 0000000..2872747 --- /dev/null +++ b/enlace_auth/auth/shares.py @@ -0,0 +1,228 @@ +"""Owner-granted data shares: let named users at one user's per-user data in one app. + +Grants (:mod:`enlace_auth.auth.grants`) answer *may this user open this app?* A +**share** answers a different question: *may this user act on that user's data in +this app?* A share is ``(app_id, owner, grantee)`` plus an access level, and the +per-user store (:mod:`enlace_auth.stores.middleware`) honours it when a request +names an owner with ``?owner=``. The design, and why it is shaped this way, is +``misc/docs/decisions/0001-owner-granted-data-shares.md``. + +Storage mirrors :class:`~enlace_auth.auth.grants.GrantStore`: a thin adapter over a +``MutableMapping`` whose keys are ``"{app_id}/{owner}/{grantee}"`` and whose values +are JSON records:: + + { + "app_id": str, + "owner": str, # normalized email + "grantee": str, # normalized email + "access": "rw" | "ro", # "ro" = read-only (GET) + "label": str | None, # how the grantee sees this space + "granted_at": float, # epoch seconds, UTC + "granted_by": str | None, # who created it (the owner, or an admin) + "expires_at": float | None, # epoch seconds, UTC; None = never + } + +Two rules the store enforces, so no caller can forget them: + +- **A share names existing accounts only** (when ``account_exists`` is given). A + share to an address nobody holds would be claimed by whoever registers it next. +- **A share dies with either account**: :meth:`ShareStore.remove_account` deletes + every share an email is part of, and account deletion calls it. +""" + +from __future__ import annotations + +import time +from collections.abc import Iterator, MutableMapping +from pathlib import Path +from typing import Callable, Optional + +from enlace_auth.auth.grants import ( + GrantError, + _is_active, + _normalize_email, + _validate_app_id, +) + +#: The access levels a share may carry: read-write, or read-only (GET). +ACCESS_LEVELS = ("rw", "ro") + + +class ShareError(ValueError): + """Raised for an invalid share (bad email/app id, unknown account, self-share).""" + + +def _email(value: str) -> str: + try: + return _normalize_email(value) + except GrantError as e: + raise ShareError(str(e).replace("grant", "share")) from None + + +def _app(value: str) -> str: + try: + return _validate_app_id(value) + except GrantError as e: + raise ShareError(str(e).replace("grant", "share")) from None + + +class ShareStore: + """Share semantics over a ``MutableMapping``. + + Args: + backend: the per-name store (e.g. ``factory("shares")``). + root: the filesystem directory backing ``backend``, used only to list one + app's shares without scanning every key (as ``GrantStore`` does). + ``None`` (a dict backend in tests) falls back to filtering all keys. + account_exists: ``email -> bool``. When given, :meth:`share` refuses an + owner or grantee with no account. ``None`` skips the check (tests). + """ + + def __init__( + self, + backend: MutableMapping, + *, + root: Optional[Path] = None, + account_exists: Optional[Callable[[str], bool]] = None, + ): + self._store = backend + self._root = Path(root) if root is not None else None + self._account_exists = account_exists + + @staticmethod + def _key(app_id: str, owner: str, grantee: str) -> str: + return f"{app_id}/{owner}/{grantee}" + + def share( + self, + app_id: str, + owner: str, + grantee: str, + *, + access: str = "rw", + label: Optional[str] = None, + expires_at: Optional[float] = None, + granted_by: Optional[str] = None, + now: Optional[float] = None, + ) -> dict: + """Create or replace the share ``owner`` → ``grantee`` in ``app_id``. + + ``expires_at`` is epoch seconds UTC or ``None``; turn a date string into it + with :func:`enlace_auth.auth.grants.parse_expires_at` first. + """ + app_id, owner, grantee = _app(app_id), _email(owner), _email(grantee) + if owner == grantee: + raise ShareError("An owner cannot share with themselves.") + if access not in ACCESS_LEVELS: + raise ShareError(f"access must be one of {ACCESS_LEVELS}, not {access!r}") + if self._account_exists is not None: + missing = [e for e in (owner, grantee) if not self._account_exists(e)] + if missing: + raise ShareError(f"No account for {', '.join(missing)}; a share names existing accounts only.") + record = { + "app_id": app_id, + "owner": owner, + "grantee": grantee, + "access": access, + "label": label or None, + "granted_at": time.time() if now is None else now, + "granted_by": granted_by or None, + "expires_at": expires_at, + } + self._store[self._key(app_id, owner, grantee)] = record + return record + + def get(self, app_id: str, owner: str, grantee: str) -> Optional[dict]: + try: + value = self._store[self._key(_app(app_id), _email(owner), _email(grantee))] + except (KeyError, ShareError): + return None + return value if isinstance(value, dict) else None + + def revoke(self, app_id: str, owner: str, grantee: str) -> bool: + try: + del self._store[self._key(_app(app_id), _email(owner), _email(grantee))] + return True + except (KeyError, ShareError): + return False + + def access( + self, app_id: str, owner: str, grantee: str, *, now: Optional[float] = None + ) -> Optional[str]: + """``"rw"``, ``"ro"`` or ``None``: what ``grantee`` may do with ``owner``'s data.""" + record = self.get(app_id, owner, grantee) + now = time.time() if now is None else now + if record is None or not _is_active(record, now): + return None + return record.get("access") if record.get("access") in ACCESS_LEVELS else None + + def granted(self, app_id: str, owner: str, *, now: Optional[float] = None) -> list[dict]: + """Every share ``owner`` made in ``app_id``, each with ``"active"``.""" + app_id, owner = _app(app_id), _email(owner) + now = time.time() if now is None else now + return [ + {**rec, "active": _is_active(rec, now)} + for rec in self._records(app_id) + if rec.get("owner") == owner + ] + + def received(self, app_id: str, grantee: str, *, now: Optional[float] = None) -> list[dict]: + """The **active** shares made to ``grantee`` in ``app_id``.""" + app_id, grantee = _app(app_id), _email(grantee) + now = time.time() if now is None else now + return [ + rec + for rec in self._records(app_id) + if rec.get("grantee") == grantee and _is_active(rec, now) + ] + + def list_all(self) -> list[dict]: + """Every share in every app. Admin-only, infrequent.""" + out = [] + for key in list(self._store): + try: + rec = self._store[key] + except KeyError: + continue + if isinstance(rec, dict) and "grantee" in rec: + out.append(rec) + return out + + def remove_account(self, email: str) -> int: + """Delete every share ``email`` is part of, as owner or grantee. Returns how many.""" + try: + email = _email(email) + except ShareError: + return 0 + gone = 0 + for rec in self.list_all(): + if email in (rec.get("owner"), rec.get("grantee")): + gone += self.revoke(rec["app_id"], rec["owner"], rec["grantee"]) + return gone + + def _records(self, app_id: str) -> Iterator[dict]: + for key in self._keys_for_app(app_id): + try: + rec = self._store[key] + except KeyError: + continue + if isinstance(rec, dict): + yield rec + + def _keys_for_app(self, app_id: str) -> Iterator[str]: + """``{app_id}/{owner}/{grantee}`` keys of one app — a directory scan when possible.""" + if self._root is not None: + app_dir = self._root / app_id + if not app_dir.is_dir(): + return + for owner_dir in app_dir.iterdir(): + if not owner_dir.is_dir(): + continue + for child in owner_dir.iterdir(): + if child.is_file() and not child.name.endswith(".tmp"): + yield f"{app_id}/{owner_dir.name}/{child.name}" + return + prefix = f"{app_id}/" + for key in list(self._store): + if isinstance(key, str) and key.startswith(prefix): + yield key diff --git a/enlace_auth/plugin.py b/enlace_auth/plugin.py index 1ed0f8f..034d1c2 100644 --- a/enlace_auth/plugin.py +++ b/enlace_auth/plugin.py @@ -332,6 +332,8 @@ def wire(parent: "FastAPI", config) -> None: from enlace_auth.auth.middleware import AccessRule from enlace_auth.stores import StoreInjectionMiddleware, make_file_store_factory from enlace_auth.stores.middleware import make_store_router + from enlace_auth.auth.share_routes import make_share_router + from enlace_auth.auth.shares import ShareStore platform_factory = make_file_store_factory(auth_cfg.stores.path) session_backend = platform_factory("sessions") @@ -347,6 +349,14 @@ def wire(parent: "FastAPI", config) -> None: grants_root = Path(os.path.expanduser(auth_cfg.stores.path)) / "grants" grant_store = GrantStore(platform_factory("grants"), root=grants_root) + # Owner-granted data shares (ADR 0001): who may act on whose per-user data. + # Beside grants, under the same root; a share may only name existing accounts. + share_store = ShareStore( + platform_factory("shares"), + root=Path(os.path.expanduser(auth_cfg.stores.path)) / "shares", + account_exists=lambda email: email in user_backend, + ) + def grants_resolver(app_id: str) -> set[str]: """Emails with an active grant for ``app_id``; ``now`` read per call.""" return grant_store.active_emails_for_app(app_id, now=time.time()) @@ -369,6 +379,7 @@ def grants_resolver(app_id: str) -> set[str]: shared_hashes: dict[str, str] = {} access_rules: list[AccessRule] = [] protected_user_apps: set[str] = set() + store_apps: set[str] = set() for app in getattr(config, "apps", []): _require_user_gate_for_admins_alias(app) h: Optional[str] = None @@ -379,6 +390,11 @@ def grants_resolver(app_id: str) -> set[str]: shared_hashes[app.name] = h if app.access == "protected:user": protected_user_apps.add(app.name) + if app.access == "protected:user" or getattr(app, "user_store", False) is True: + # Apps with a per-user store: protected:user ones, and any app (public + # included) that opts in with ``user_store = true``. Kept apart from + # protected_user_apps, which the grants admin uses. + store_apps.add(app.name) allowed = tuple( _expand_allowed_users( getattr(app, "allowed_users", ()), admin_emails, app_name=app.name @@ -556,9 +572,11 @@ def grants_resolver(app_id: str) -> set[str]: # Per-user store API. store_router = make_store_router( base_store_getter=lambda: user_data_backend, - protected_apps=protected_user_apps, + protected_apps=store_apps, + share_access=lambda app_id, owner, grantee: share_store.access(app_id, owner, grantee), ) parent.include_router(store_router) + parent.include_router(make_share_router(share_store=share_store, store_apps=store_apps)) # App-metadata overlay: the editable launcher-metadata layer (owner-added # keywords + icon/title overrides). Core reads the overlay + can-edit closure @@ -600,6 +618,8 @@ def grants_resolver(app_id: str) -> set[str]: apps=list(getattr(config, "apps", [])), grant_store=grant_store, protected_user_apps=protected_user_apps, + share_store=share_store, + store_apps=store_apps, signing_key=signing_key, resource_allowlist=auth_cfg.oauth_server.resource_allowlist, public_base_url=_public_base_url(config, auth_cfg), @@ -635,7 +655,10 @@ def grants_resolver(app_id: str) -> set[str]: # reverse insertion order, so the last `add_middleware` call is the # outermost wrapper. We want: auth (outermost) -> store -> csrf -> app. parent.add_middleware( - CSRFMiddleware, signing_key=signing_key, exempt_prefixes=csrf_exempt + CSRFMiddleware, + signing_key=signing_key, + exempt_prefixes=csrf_exempt, + enforce_prefixes=[f"/api/{name}/store" for name in sorted(store_apps)], ) parent.add_middleware(StoreInjectionMiddleware, base_store=user_data_backend) parent.add_middleware( diff --git a/enlace_auth/stores/backends.py b/enlace_auth/stores/backends.py index 5cf06e1..2885fbc 100644 --- a/enlace_auth/stores/backends.py +++ b/enlace_auth/stores/backends.py @@ -102,6 +102,28 @@ def __iter__(self) -> Iterator[str]: def __len__(self) -> int: return sum(1 for _ in iter(self)) + def keys_under(self, prefix: str) -> Iterator[str]: + """The keys starting with ``prefix``, walking only the directory it names. + + ``iter(self)`` walks the whole root; a per-user store holds every user's + every key, so listing one user's collection that way costs the platform. + """ + if not prefix: + yield from iter(self) + return + try: + here = self._path(prefix.rstrip("/")) + except KeyError: + return + start = here if prefix.endswith("/") else here.parent + if not start.is_dir(): + return + for p in start.rglob("*"): + if p.is_file() and not p.name.endswith(".tmp"): + key = str(p.relative_to(self._root)) + if key.startswith(prefix): + yield key + def __contains__(self, key: object) -> bool: if not isinstance(key, str): return False diff --git a/enlace_auth/stores/middleware.py b/enlace_auth/stores/middleware.py index 368ed1d..1c16767 100644 --- a/enlace_auth/stores/middleware.py +++ b/enlace_auth/stores/middleware.py @@ -12,10 +12,13 @@ from __future__ import annotations -from collections.abc import MutableMapping -from typing import Callable, Optional +import hashlib +import json +from collections.abc import Iterable, MutableMapping +from typing import Any, Callable, Optional, Union from fastapi import APIRouter, HTTPException, Request +from fastapi.responses import JSONResponse from enlace_auth.stores.prefixed import PrefixedStore from enlace_auth.stores.validation import sanitize_key @@ -49,55 +52,139 @@ async def __call__(self, scope, receive, send): await self.app(scope, receive, send) +#: How many items the list route returns before it says ``"truncated": true``. +DEFAULT_MAX_ITEMS = 5000 + + +def etag_of(value: Any) -> str: + """A strong ETag for a stored JSON value: a hash of its canonical serialisation.""" + canonical = json.dumps(value, sort_keys=True, separators=(",", ":"), ensure_ascii=False) + return '"' + hashlib.sha256(canonical.encode("utf-8")).hexdigest()[:32] + '"' + + def make_store_router( *, base_store_getter: Callable[[], Optional[MutableMapping]], - protected_apps: set[str], + protected_apps: Union[Iterable[str], Callable[[], Iterable[str]]], + share_access: Optional[Callable[[str, str, str], Optional[str]]] = None, + max_items: int = DEFAULT_MAX_ITEMS, ) -> APIRouter: - """Return a router exposing ``/api/{app_id}/store/{key}`` endpoints. + """Return a router exposing the per-user store of each app that has one. + + Routes: + + - ``GET /api/{app_id}/store?prefix=

`` → ``{"items": {key: {"value", "etag"}}, "truncated"}`` + - ``GET /api/{app_id}/store/{key}`` → ``{"value": v}`` with an ``ETag`` header + - ``PUT /api/{app_id}/store/{key}`` — ``If-Match: `` / ``If-None-Match: *`` make it conditional + - ``DELETE /api/{app_id}/store/{key}`` — ``If-Match`` likewise + + A failed precondition is **412** with the current ``{"value", "etag"}`` (``value`` + null when the key is absent), so the client can resolve and retry. - Only apps whose name is in ``protected_apps`` (i.e. ``protected:user`` - access level) can have their store accessed this way. The router assumes - ``PlatformAuthMiddleware`` has already set ``request.state.user_id``. + ``protected_apps`` names the apps with a per-user store: ``protected:user`` + apps and those whose ``app.toml`` sets ``user_store = true`` (a callable is + re-read per request). ``?owner=`` on any route acts on that user's + data instead of the caller's, when ``share_access(app_id, owner, caller)`` + says so (``"rw"``, or ``"ro"`` for GET only); otherwise 404, as if the owner + had nothing. Without ``share_access`` every ``?owner=`` naming someone else + is 404. See ``misc/docs/decisions/0001-owner-granted-data-shares.md``. + + The router assumes ``PlatformAuthMiddleware`` has set ``request.state.user_id`` + (public apps get it too, when the visitor is signed in). CSRF on writes is the + ``CSRFMiddleware``'s, through its ``enforce_prefixes``. """ router = APIRouter() - def _scoped_store(request: Request, app_id: str) -> PrefixedStore: - if app_id not in protected_apps: + def _apps() -> set[str]: + return set(protected_apps() if callable(protected_apps) else protected_apps) + + def _scoped_store(request: Request, app_id: str, *, write: bool) -> PrefixedStore: + if app_id not in _apps(): raise HTTPException( status_code=404, detail=f"No user store for app '{app_id}'" ) user_id = getattr(request.state, "user_id", None) - if not user_id: + if not user_id or user_id == "shared": raise HTTPException(status_code=401, detail="Not authenticated") base = base_store_getter() if base is None: raise HTTPException(status_code=503, detail="User data store disabled") + owner = (request.query_params.get("owner") or "").strip().lower() + me = str(user_id).lower() + if owner and owner != me: + granted = share_access(app_id, owner, me) if share_access else None + if granted not in ("rw", "ro") or (write and granted != "rw"): + # 404, not 403: a caller probing for owners learns nothing. + raise HTTPException(status_code=404, detail="Not found") + whose = owner + else: + whose = me try: - prefix = f"{sanitize_key(str(user_id))}/{sanitize_key(app_id)}/" + prefix = f"{sanitize_key(whose)}/{sanitize_key(app_id)}/" except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) from e return PrefixedStore(base, prefix) - @router.get("/api/{app_id}/store/{key:path}") - async def get_value(app_id: str, key: str, request: Request): - store = _scoped_store(request, app_id) + def _check_key(key: str) -> None: try: sanitize_key(key) except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) from e + + def _current(store: PrefixedStore, key: str) -> tuple[Any, Optional[str]]: try: - return {"value": store[key]} + value = store[key] except KeyError: + return None, None + return value, etag_of(value) + + def _precondition(request: Request, store: PrefixedStore, key: str) -> None: + """Raise 412 when ``If-Match`` / ``If-None-Match: *`` no longer hold.""" + if_match = request.headers.get("if-match") + if_none_match = request.headers.get("if-none-match") + if if_match is None and if_none_match is None: + return + value, etag = _current(store, key) + failed = if_none_match is not None and if_none_match.strip() == "*" and etag is not None + if if_match is not None: + wanted = if_match.strip() + failed = failed or etag is None or (wanted != "*" and wanted != etag) + # No await between this check and the write, so within one worker the pair is + # atomic. Across workers two writers can still interleave in the window of one + # file write; the conditional write narrows the lost-update race, it does not + # serialise it. # seam candidate: a per-key lock on the base store. + if failed: + raise HTTPException(status_code=412, detail={"value": value, "etag": etag}) + + @router.get("/api/{app_id}/store") + async def list_values(app_id: str, request: Request, prefix: str = ""): + store = _scoped_store(request, app_id, write=False) + if prefix: + _check_key(prefix) + items: dict[str, Any] = {} + truncated = False + for key in store.keys_under(prefix): + if len(items) >= max_items: + truncated = True + break + value, etag = _current(store, key) + if etag is not None: + items[key] = {"value": value, "etag": etag} + return {"items": items, "truncated": truncated} + + @router.get("/api/{app_id}/store/{key:path}") + async def get_value(app_id: str, key: str, request: Request): + store = _scoped_store(request, app_id, write=False) + _check_key(key) + value, etag = _current(store, key) + if etag is None: raise HTTPException(status_code=404, detail=f"Key '{key}' not found") + return JSONResponse({"value": value}, headers={"ETag": etag}) @router.put("/api/{app_id}/store/{key:path}") async def put_value(app_id: str, key: str, request: Request): - store = _scoped_store(request, app_id) - try: - sanitize_key(key) - except ValueError as e: - raise HTTPException(status_code=400, detail=str(e)) from e + store = _scoped_store(request, app_id, write=True) + _check_key(key) try: body = await request.json() except Exception as e: @@ -105,16 +192,16 @@ async def put_value(app_id: str, key: str, request: Request): value = ( body.get("value") if isinstance(body, dict) and "value" in body else body ) + _precondition(request, store, key) store[key] = value - return {"ok": True} + etag = etag_of(value) + return JSONResponse({"ok": True, "etag": etag}, headers={"ETag": etag}) @router.delete("/api/{app_id}/store/{key:path}") async def delete_value(app_id: str, key: str, request: Request): - store = _scoped_store(request, app_id) - try: - sanitize_key(key) - except ValueError as e: - raise HTTPException(status_code=400, detail=str(e)) from e + store = _scoped_store(request, app_id, write=True) + _check_key(key) + _precondition(request, store, key) try: del store[key] except KeyError: diff --git a/enlace_auth/stores/prefixed.py b/enlace_auth/stores/prefixed.py index 7897ff8..00618e0 100644 --- a/enlace_auth/stores/prefixed.py +++ b/enlace_auth/stores/prefixed.py @@ -61,6 +61,22 @@ def __iter__(self) -> Iterator[str]: def __len__(self) -> int: return sum(1 for _ in iter(self)) + def keys_under(self, sub: str = "") -> Iterator[str]: + """Keys (without this store's prefix) that start with ``sub``. + + Uses the base store's own ``keys_under`` when it has one (the file backend + walks only the matching directory), else filters a full iteration. + """ + if sub: + sanitize_key(sub) + full = self._prefix + sub + plen = len(self._prefix) + fast = getattr(self._base, "keys_under", None) + keys = fast(full) if callable(fast) else (k for k in self._base if isinstance(k, str)) + for k in keys: + if k.startswith(full): + yield k[plen:] + def __contains__(self, key: object) -> bool: if not isinstance(key, str): return False diff --git a/misc/docs/decisions/0001-owner-granted-data-shares.md b/misc/docs/decisions/0001-owner-granted-data-shares.md new file mode 100644 index 0000000..6644e98 --- /dev/null +++ b/misc/docs/decisions/0001-owner-granted-data-shares.md @@ -0,0 +1,83 @@ +# ADR 0001 — Owner-granted data shares + +- Status: accepted, 2026-10-01 (revised the same day after an adversarial review; the review's findings and what was done with each are at the end) +- First consumer: a maths-practice app on a family-run enlace platform (a child's practice sessions, shared with her parents) + +## Context + +enlace_auth answers one authorization question today: *may this user open this app?* (`access` levels, `allowed_users`, admin-made **grants** in `auth/grants.py`). Its per-user store (`stores/middleware.py`) then scopes every read and write to `"{user_id}/{app_id}/"`, the signed-in user's own prefix, with no way to reach anyone else's. + +The first real case needs a second question: *may this user read or write that user's data in this app?* A child owns her practice sessions in one app; her two parents, signed in as themselves, need to see and add to them from any device. Nothing in enlace or enlace_auth covers it (no issue, no ADR; only admin grants exist). + +It is a mechanism, not a feature of that app: "let the owner of some per-user data let named people at it" will recur in any per-user app on the platform. So it lives in enlace_auth, beside grants, and that app is its first consumer. + +## Decision + +### 1. A share is (app, owner, grantee), read-write or read-only + +A **share** says: in app `app_id`, `grantee` may act on `owner`'s per-user data. Stored like grants — a thin adapter (`ShareStore`, `auth/shares.py`) over a `MutableMapping` from the platform store factory, key `"{app_id}/{owner}/{grantee}"`, value: + +```json +{"app_id": "practice", "owner": "kid@example.com", "grantee": "parent@example.com", + "access": "rw", "label": "Kid", "granted_at": 1790870000.0, "granted_by": "admin@example.com", + "expires_at": null} +``` + +- `access`: `"rw"` (default) or `"ro"` (GET only). One field now, so a read-only grantee — a teacher, say — never needs a format change. +- `label`: how the **grantee** sees this space ("Kid"). Optional; an app falls back to the owner's email. +- Emails are normalised (lowercased, shape-checked) with the same rules as grants. Expiry works as for grants. One record per pair: granting again replaces it. +- No re-sharing: a grantee cannot grant onward, because the owner side of a share is always the signed-in user (or an admin acting explicitly). + +### 2. A share names an existing account, and dies with it + +Password registration does not prove ownership of an address, and a deleted account frees its email for re-registration. So: + +- **A share may only name accounts that exist** — owner and grantee alike. Otherwise `409` (HTTP) or a refusal (CLI). There are no pending shares. +- **Deleting an account removes every share it is part of**, both ways, in the same operation (`delete_user`). A share never outlives the account it names. + +### 3. Who manages shares + +- **The owner**, signed in: `GET /auth/shares/{app_id}` → `{"owner": me, "granted": [...with "active"], "received": [active shares to me]}`; `PUT /auth/shares/{app_id}/{grantee}` (body `{"access"?, "label"?, "expires_at"?}`); `DELETE /auth/shares/{app_id}/{grantee}`. A grantee can leave: `DELETE /auth/shares/{app_id}/received/{owner}` (registered before the `{grantee}` route). CSRF-protected like every state-changing `/auth/*` route. +- **An admin**, through the existing admin API beside grants (`GET/POST /_admin/api/shares`, `DELETE /_admin/api/shares/{app_id}/{owner}/{grantee}`), and the CLI (`enlace-auth share`, `list-shares`, `revoke-share`). This is the path for an owner who is a child: the parent who runs the platform manages her shares without her password or an ssh session. +- Every route refuses an `app_id` that has no per-user store (see §4), and runs `owner`/`grantee` through both email normalisation and `sanitize_key`; expiry is parsed by the grants module's `parse_expires_at`. + +### 4. How the per-user store honours it + +- **`?owner=`** on `/api/{app_id}/store/...` selects whose prefix to use. Absent, or equal (after normalisation) to the signed-in user: their own, as today. Different: allowed only with an active share `(app_id, owner, user)`, and only for GET when the share is `"ro"`; otherwise **404** — a stranger probing for owners learns nothing. A client must read 404 under `?owner=` as "no access (any more)", not as "empty". +- **A list route**, `GET /api/{app_id}/store?prefix=

` → `{"items": {key: {"value": v, "etag": e}}}`. It lists by walking only the owner's directory under `p` (a `keys_under` fast path on the file backend), never the whole platform store; capped at `max_items` (default 5000) with `"truncated": true` past it. +- **Conditional writes.** Every GET returns an `ETag` (a hash of the stored value; also in the list route's items). `PUT` and `DELETE` accept `If-Match: ` (or `If-None-Match: *` for create-only) and answer **412** with the current value and etag when it no longer matches. Two devices, or two people, writing the same key can then resolve instead of overwriting: the store stays generic, and the app decides what "resolve" means (the first consumer: the newer record by its own revision stamp wins). +- **CSRF on writes.** The store routes are under `/api/`, which the platform's CSRF middleware exempts. For an app with a per-user store, the store router itself requires the double-submit header (`X-CSRF-Token` matching the `enlace_csrf` cookie, the same check `/auth/*` uses) on every `PUT` and `DELETE`. Shares raise the stakes — a grantee's cookie now reaches someone else's data — so cookie SameSite alone is not enough on an origin that hosts many apps. +- **Which apps get a store**: `access = "protected:user"` apps, as today, plus any app whose `app.toml` sets `user_store = true` (`AppConfig` is `extra="allow"`). That lets a **public** app offer a store to visitors who happen to be signed in; anonymous requests get 401 and the app does what it did before. A `user_store` app is **not** added to the `protected:user` set the grants admin uses. + +### 5. Where it sits + +`ShareStore` is built in `plugin.py` beside `GrantStore`, from `platform_factory("shares")`, so shares live under the platform store root (outside the deploy target, surviving redeploys, backed up with it). The store router receives a `share_access(app_id, owner, user) -> "rw" | "ro" | None` callable, not the store. + +## Seams + +| # | Seam | v1 default | Replacement you can point at | +|---|---|---|---| +| 1 | where shares live | `platform_factory("shares")` (JSON files, as grants) | any `MutableMapping` — the factory's existing dol variant (`stores/backends.py`) | +| 2 | how the store router decides | `share_access` built from `ShareStore` | an app-specific policy (e.g. a guardian relation) — same signature | + +NOT seams: groups of grantees, re-sharing, per-key scopes, a `can_manage` delegate flag (the admin path covers the child-owner case), aliases (a short name for the email), a web UI for shares beyond the admin API. Each is "no" on purpose; none has a consumer. + +Surfaces: HTTP (owner routes, admin API, store), CLI (admin). MCP or a frontend would call `ShareStore`'s methods; neither needs the core to change. + +## Consequences + +- Per-user data stops being strictly private to its owner. A request with `?owner=` costs one share lookup (one small file read); without it the hot path is unchanged. +- A public app that enables `user_store` must treat 401 as "no account mode" and 404 under `?owner=` as "access gone". +- Store writes from a browser now need the CSRF header; existing `protected:user` apps that write to the store from a page must send it (none on the first deployment does today: the store is off there). +- The per-user store is still off on a platform without `[stores.user_data]`; enabling it is a platform decision. + +## Alternatives rejected + +- **Extend admin grants with an owner field.** Grants answer "may open the app" and are admin-made; folding owner-made, data-scoped permissions into them would make every grant check reason about two different things. +- **Share the owner's password, or a shared-password app.** No per-person revocation, and a child's password in three heads. +- **A capability link.** Good for a read-only view (the first consumer already has one), but the parents need write access from several devices, signed in as themselves, revocable per person. +- **Impersonation / "acting as".** Broader than needed: the grantee would become the owner everywhere. + +## Review (2026-10-01) and what was done + +An adversarial review of the first draft found: shares to unregistered or deleted emails could be claimed by whoever registers the address next (→ §2); unconditional writes would quietly undo a consumer's merge rule (→ conditional writes, §4); a list over the file backend walked the whole platform store (→ `keys_under`, cap); `/api/` is CSRF-exempt (→ router-level check, §4); share routes lacked input gates (→ §3); a child owner cannot manage her own shares (→ admin API, §3); all-or-nothing access would force a format change for the first read-only grantee (→ `access`); the grantee-facing label was conflated with an owner note (→ `label`); and `user_store` apps must not join the grants admin's `protected:user` set (→ §4). diff --git a/pyproject.toml b/pyproject.toml index 9580e42..0fc07fc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -11,7 +11,7 @@ requires-python = ">=3.10" keywords = ["enlace", "auth", "fastapi", "platform", "argon2", "session", "admin"] authors = [{ name = "Thor Whalen" }] dependencies = [ - "enlace>=0.1.41", # needs enlace.access (shared gate/launcher predicate) + "enlace>=0.1.42", # needs AppConfig.user_store (and enlace.access) "fastapi>=0.100.0", "pydantic>=2.0.0", "itsdangerous>=2.1", diff --git a/tests/test_shares.py b/tests/test_shares.py new file mode 100644 index 0000000..37e4749 --- /dev/null +++ b/tests/test_shares.py @@ -0,0 +1,260 @@ +"""Owner-granted data shares (ADR 0001): the store, the routes, and the per-user store honouring them.""" + +from __future__ import annotations + +import textwrap +import time + +import pytest +from enlace.base import PlatformConfig +from enlace.compose import build_backend +from enlace.discover import discover_apps +from fastapi import FastAPI +from starlette.testclient import TestClient + +from enlace_auth import plugin as auth_plugin +from enlace_auth.auth import ShareError, ShareStore, hash_password +from enlace_auth.auth.cookies import verify_cookie +from enlace_auth.stores import make_store_router +from enlace_auth.stores.backends import make_file_store_factory +from enlace_auth.stores.prefixed import PrefixedStore + +KEY = "shares-key-32bytes-minimumlength!" +KID, MUM, DAD, STRANGER = "kid@example.com", "mum@example.com", "dad@example.com", "x@example.com" + + +# --- ShareStore ------------------------------------------------------------------- + + +def _store(accounts=(KID, MUM, DAD)): + return ShareStore({}, account_exists=set(accounts).__contains__) + + +def test_share_names_existing_accounts_only(): + shares = _store() + with pytest.raises(ShareError, match="No account"): + shares.share("practice", KID, STRANGER) + with pytest.raises(ShareError, match="No account"): + shares.share("practice", STRANGER, KID) + with pytest.raises(ShareError, match="themselves"): + shares.share("practice", KID, KID) + with pytest.raises(ShareError, match="access"): + shares.share("practice", KID, MUM, access="admin") + + +def test_access_levels_expiry_and_normalisation(): + shares = _store() + shares.share("practice", "Kid@Example.com", MUM) + shares.share("practice", KID, DAD, access="ro", label="Kid") + assert shares.access("practice", KID, MUM) == "rw" + assert shares.access("practice", KID, DAD) == "ro" + assert shares.access("practice", MUM, KID) is None, "a share is one-way" + assert shares.access("other", KID, MUM) is None, "a share is per app" + now = time.time() + shares.share("practice", KID, MUM, expires_at=now + 10) + assert shares.access("practice", KID, MUM, now=now + 20) is None + assert [r["grantee"] for r in shares.received("practice", MUM, now=now + 20)] == [] + granted = {r["grantee"]: r["active"] for r in shares.granted("practice", KID, now=now + 20)} + assert granted == {MUM: False, DAD: True} + assert shares.received("practice", DAD)[0]["label"] == "Kid" + + +def test_remove_account_deletes_both_directions(): + shares = _store() + shares.share("practice", KID, MUM) + shares.share("practice", KID, DAD) + shares.share("practice", MUM, DAD) + assert shares.remove_account(MUM) == 2 + assert {(r["owner"], r["grantee"]) for r in shares.list_all()} == {(KID, DAD)} + + +def test_file_backend_lists_one_app_by_directory(tmp_path): + factory = make_file_store_factory(str(tmp_path)) + shares = ShareStore(factory("shares"), root=tmp_path / "shares") + shares.share("practice", KID, MUM) + shares.share("other", KID, DAD) + assert [r["grantee"] for r in shares.received("practice", MUM)] == [MUM] + assert shares.received("practice", DAD) == [] + + +# --- the store router ----------------------------------------------------------- + + +def _router_client(base, shares, *, user): + app = FastAPI() + + @app.middleware("http") + async def _fake_auth(request, call_next): + request.state.user_id = user + return await call_next(request) + + app.include_router( + make_store_router( + base_store_getter=lambda: base, + protected_apps={"practice"}, + share_access=shares.access, + ) + ) + return TestClient(app) + + +def test_owner_param_needs_an_active_share(): + base, shares = {}, _store() + base[f"{KID}/practice/attempts/a1"] = {"id": "a1"} + shares.share("practice", KID, MUM) + shares.share("practice", KID, DAD, access="ro") + + mum = _router_client(base, shares, user=MUM) + assert mum.get(f"/api/practice/store/attempts/a1?owner={KID}").json()["value"] == {"id": "a1"} + assert mum.put(f"/api/practice/store/attempts/a2?owner={KID}", json={"value": {"id": "a2"}}).status_code == 200 + assert base[f"{KID}/practice/attempts/a2"] == {"id": "a2"}, "written into the owner's data" + + dad = _router_client(base, shares, user=DAD) + assert dad.get(f"/api/practice/store/attempts/a1?owner={KID}").status_code == 200 + assert dad.put(f"/api/practice/store/attempts/a3?owner={KID}", json={"value": 1}).status_code == 404, "ro: no writes" + assert dad.delete(f"/api/practice/store/attempts/a1?owner={KID}").status_code == 404 + + stranger = _router_client(base, shares, user=STRANGER) + assert stranger.get(f"/api/practice/store/attempts/a1?owner={KID}").status_code == 404 + assert stranger.get(f"/api/practice/store?owner={KID}").status_code == 404 + # Own data is still the default, and ?owner= naming oneself is the same thing. + assert mum.get("/api/practice/store/attempts/a1").status_code == 404 + assert mum.put(f"/api/practice/store/x?owner={MUM.upper()}", json={"value": 1}).status_code == 200 + assert base[f"{MUM}/practice/x"] == 1 + + shares.revoke("practice", KID, MUM) + assert mum.get(f"/api/practice/store/attempts/a1?owner={KID}").status_code == 404, "revocation is immediate" + + +def test_list_route_and_conditional_writes(): + base, shares = {}, _store() + me = _router_client(base, shares, user=KID) + first = me.put("/api/practice/store/attempts/a1", json={"value": {"rev": 1}}) + etag = first.headers["etag"] + me.put("/api/practice/store/attempts/a2", json={"value": {"rev": 1}}) + me.put("/api/practice/store/marks/removed", json={"value": []}) + + listed = me.get("/api/practice/store?prefix=attempts/").json() + assert set(listed["items"]) == {"attempts/a1", "attempts/a2"} and not listed["truncated"] + assert listed["items"]["attempts/a1"] == {"value": {"rev": 1}, "etag": etag} + assert me.get("/api/practice/store/attempts/a1").headers["etag"] == etag + + # A writer holding the current etag wins; a stale one gets 412 with what is there now. + second = me.put("/api/practice/store/attempts/a1", json={"value": {"rev": 2}}, headers={"If-Match": etag}) + assert second.status_code == 200 + stale = me.put("/api/practice/store/attempts/a1", json={"value": {"rev": 3}}, headers={"If-Match": etag}) + assert stale.status_code == 412 + assert stale.json()["detail"] == {"value": {"rev": 2}, "etag": second.headers["etag"]} + assert base[f"{KID}/practice/attempts/a1"] == {"rev": 2} + # Create-only. + assert me.put("/api/practice/store/attempts/a1", json={"value": 0}, headers={"If-None-Match": "*"}).status_code == 412 + assert me.put("/api/practice/store/attempts/a9", json={"value": 0}, headers={"If-None-Match": "*"}).status_code == 200 + assert me.delete("/api/practice/store/attempts/a9", headers={"If-Match": etag}).status_code == 412 + assert me.delete("/api/practice/store/attempts/a9").status_code == 200 + + +def test_list_is_capped(): + base, shares = {f"{KID}/practice/k{i}": i for i in range(5)}, _store() + app = FastAPI() + + @app.middleware("http") + async def _fake_auth(request, call_next): + request.state.user_id = KID + return await call_next(request) + + app.include_router(make_store_router(base_store_getter=lambda: base, protected_apps={"practice"}, max_items=3)) + body = TestClient(app).get("/api/practice/store").json() + assert len(body["items"]) == 3 and body["truncated"] + + +def test_prefixed_keys_under_walks_only_the_owner(tmp_path): + base = make_file_store_factory(str(tmp_path))("user_data") + base[f"{KID}/practice/attempts/a1"] = 1 + base[f"{MUM}/practice/attempts/b1"] = 2 + store = PrefixedStore(base, f"{KID}/practice/") + assert list(store.keys_under("attempts/")) == ["attempts/a1"] + assert list(base.keys_under(f"{KID}/practice/att")) == [f"{KID}/practice/attempts/a1"] + + +# --- end to end: a PUBLIC app with user_store = true --------------------------------- + + +@pytest.fixture +def platform(tmp_path, monkeypatch): + apps_dir = tmp_path / "apps" + (apps_dir / "practice").mkdir(parents=True) + (apps_dir / "practice" / "server.py").write_text( + textwrap.dedent( + """ + from fastapi import FastAPI + app = FastAPI() + @app.get("/ping") + def ping(): + return {"ok": True} + """ + ).strip() + ) + (apps_dir / "practice" / "app.toml").write_text('access = "public"\nuser_store = true\n') + monkeypatch.setenv("ENLACE_SIGNING_KEY", KEY) + monkeypatch.setenv("ENLACE_ADMIN_EMAILS", DAD) + config = PlatformConfig( + apps_dir=apps_dir, + auth={ + "enabled": True, + "secure_cookies": False, + "registration_open": True, + "stores": {"backend": "file", "path": str(tmp_path / "platform_store")}, + }, + stores={"user_data": {"backend": "file", "path": str(tmp_path / "user_data")}}, + ) + return build_backend(discover_apps(config), plugins=[auth_plugin]) + + +def _signed_in(app, email): + client = TestClient(app) + client.get("/api/practice/ping") + raw = verify_cookie(client.cookies.get("enlace_csrf"), KEY, salt="csrf") + client.headers["X-CSRF-Token"] = raw + r = client.post("/auth/register", json={"email": email, "password": "secretpw123"}) + assert r.status_code == 200, r.text + return client + + +def test_public_app_store_shares_and_csrf(platform): + anonymous = TestClient(platform) + assert anonymous.get("/api/practice/ping").status_code == 200, "the app stays public" + assert anonymous.get("/api/practice/store?prefix=attempts/").status_code == 401 + + kid, mum = _signed_in(platform, KID), _signed_in(platform, MUM) + assert kid.put("/api/practice/store/attempts/a1", json={"value": {"id": "a1"}}).status_code == 200 + + # CSRF is enforced on store writes even though /api/ is otherwise exempt. + bare = kid.put("/api/practice/store/attempts/a2", json={"value": 1}, headers={"X-CSRF-Token": ""}) + assert bare.status_code == 403 + + # No share yet: Mum sees nothing of the kid's, and cannot share to a stranger. + assert mum.get(f"/api/practice/store?prefix=attempts/&owner={KID}").status_code == 404 + assert kid.put(f"/auth/shares/practice/{STRANGER}", json={}).status_code == 409 + assert kid.put(f"/auth/shares/nostore/{MUM}", json={}).status_code == 404 + + r = kid.put(f"/auth/shares/practice/{MUM}", json={"label": "Kid"}) + assert r.status_code == 200, r.text + received = mum.get("/auth/shares/practice").json()["received"] + assert [(s["owner"], s["label"]) for s in received] == [(KID, "Kid")] + items = mum.get(f"/api/practice/store?prefix=attempts/&owner={KID}").json()["items"] + assert list(items) == ["attempts/a1"] + + # Mum leaves; the route for "received" is not mistaken for a grantee. + assert mum.delete(f"/auth/shares/practice/received/{KID}").status_code == 200 + assert kid.get("/auth/shares/practice").json()["granted"] == [] + + +def test_deleting_an_account_removes_its_shares(platform): + kid, mum, dad = _signed_in(platform, KID), _signed_in(platform, MUM), _signed_in(platform, DAD) + # Dad is the admin: he seeds the kid's share without her password. + r = dad.post("/_admin/api/shares", json={"app_id": "practice", "owner": KID, "grantee": MUM}) + assert r.status_code == 200, r.text + assert len(dad.get("/_admin/api/shares").json()["shares"]) == 1 + assert dad.delete(f"/_admin/api/users/{MUM}").status_code == 200 + assert dad.get("/_admin/api/shares").json()["shares"] == [] + _ = kid, mum From 2f03b16aba3c19f02ba85a2a5152b72cef5cade5 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Thu, 1 Oct 2026 21:34:16 +0530 Subject: [PATCH 2/4] Shares: harden the store against unencodable values, tell no_access from no_key, cap labels; correct the ADR (adversarial code review) --- README.md | 2 + enlace_auth/admin/routes.py | 2 +- enlace_auth/auth/share_routes.py | 2 +- enlace_auth/auth/shares.py | 20 ++++++++-- enlace_auth/stores/middleware.py | 40 ++++++++++++++----- .../0001-owner-granted-data-shares.md | 13 ++++-- tests/test_shares.py | 36 +++++++++++++++++ 7 files changed, 96 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index a390639..2d2e2e5 100644 --- a/README.md +++ b/README.md @@ -227,6 +227,8 @@ enlace-auth revoke-share practice kid@example.com parent@example.com A signed-in owner manages their own shares at `/auth/shares/{app}` (`GET`, `PUT …/{grantee}`, `DELETE …/{grantee}`; a grantee leaves with `DELETE …/received/{owner}`); an admin uses `/_admin/api/shares`. A grantee then reads the owner's data with `?owner=` on the per-user store: `GET /api/{app}/store?prefix=…&owner=…` lists it, and every route takes `If-Match` so two devices cannot silently overwrite each other. A share names existing accounts only and is removed with either account. +Upgrading: store writes (`PUT`/`DELETE /api/{app}/store/...`) now require the `X-CSRF-Token` header (from `GET /auth/csrf`), and `/api/{app}/store` is reserved for the platform. A share does not bypass an app's `allowed_users`. + An app gets a per-user store when its `access` is `protected:user`, or when its `app.toml` sets `user_store = true` — which lets a **public** app keep data for the visitors who sign in while staying open to everyone else. The design and its review are in [`misc/docs/decisions/0001-owner-granted-data-shares.md`](misc/docs/decisions/0001-owner-granted-data-shares.md). ## Doctor checks diff --git a/enlace_auth/admin/routes.py b/enlace_auth/admin/routes.py index 40280f0..fe1aa29 100644 --- a/enlace_auth/admin/routes.py +++ b/enlace_auth/admin/routes.py @@ -410,7 +410,7 @@ async def create_share(body: _CreateShareBody, request: Request) -> dict[str, An expires_at=parse_expires_at(body.expires_at), granted_by=actor, ) except ShareError as e: - raise HTTPException(status_code=409 if "No account" in str(e) else 422, detail=str(e)) + raise HTTPException(status_code=409 if e.code == "no_account" else 422, detail=str(e)) except GrantError as e: raise HTTPException(status_code=422, detail=str(e)) return {"ok": True, "share": record} diff --git a/enlace_auth/auth/share_routes.py b/enlace_auth/auth/share_routes.py index ca3cc83..36cdeb4 100644 --- a/enlace_auth/auth/share_routes.py +++ b/enlace_auth/auth/share_routes.py @@ -76,7 +76,7 @@ async def put_share(app_id: str, grantee: str, body: _ShareBody, request: Reques expires_at=parse_expires_at(body.expires_at), granted_by=me, ) except ShareError as e: - status = 409 if "No account" in str(e) else 422 + status = 409 if e.code == "no_account" else 422 raise HTTPException(status_code=status, detail=str(e)) except GrantError as e: raise HTTPException(status_code=422, detail=str(e)) diff --git a/enlace_auth/auth/shares.py b/enlace_auth/auth/shares.py index 2872747..bbb2843 100644 --- a/enlace_auth/auth/shares.py +++ b/enlace_auth/auth/shares.py @@ -48,8 +48,16 @@ ACCESS_LEVELS = ("rw", "ro") +#: The longest ``label`` a share may carry (it is shown to the grantee as a space's name). +MAX_LABEL = 80 + + class ShareError(ValueError): - """Raised for an invalid share (bad email/app id, unknown account, self-share).""" + """Raised for an invalid share. ``code`` says which: ``"no_account"`` or ``"invalid"``.""" + + def __init__(self, message: str, *, code: str = "invalid"): + super().__init__(message) + self.code = code def _email(value: str) -> str: @@ -118,13 +126,19 @@ def share( if self._account_exists is not None: missing = [e for e in (owner, grantee) if not self._account_exists(e)] if missing: - raise ShareError(f"No account for {', '.join(missing)}; a share names existing accounts only.") + raise ShareError( + f"No account for {', '.join(missing)}; a share names existing accounts only.", + code="no_account", + ) + label = (label or "").strip() or None + if label is not None and len(label) > MAX_LABEL: + raise ShareError(f"label is longer than {MAX_LABEL} characters") record = { "app_id": app_id, "owner": owner, "grantee": grantee, "access": access, - "label": label or None, + "label": label, "granted_at": time.time() if now is None else now, "granted_by": granted_by or None, "expires_at": expires_at, diff --git a/enlace_auth/stores/middleware.py b/enlace_auth/stores/middleware.py index 1c16767..ca21f16 100644 --- a/enlace_auth/stores/middleware.py +++ b/enlace_auth/stores/middleware.py @@ -52,13 +52,24 @@ async def __call__(self, scope, receive, send): await self.app(scope, receive, send) +#: The 404 details of the store routes: no share for ``?owner=`` (or it was revoked), or no such key. +NO_ACCESS = "no_access" +NO_KEY = "no_key" + + +def _refuse_constant(name: str): + """JSON's ``NaN``/``Infinity`` are not JSON a browser can read back; refuse them on the way in.""" + raise ValueError(f"non-finite number {name}") + + #: How many items the list route returns before it says ``"truncated": true``. DEFAULT_MAX_ITEMS = 5000 def etag_of(value: Any) -> str: """A strong ETag for a stored JSON value: a hash of its canonical serialisation.""" - canonical = json.dumps(value, sort_keys=True, separators=(",", ":"), ensure_ascii=False) + # ensure_ascii: a lone surrogate (which a browser can send) must hash, not raise. + canonical = json.dumps(value, sort_keys=True, separators=(",", ":"), ensure_ascii=True) return '"' + hashlib.sha256(canonical.encode("utf-8")).hexdigest()[:32] + '"' @@ -114,8 +125,10 @@ def _scoped_store(request: Request, app_id: str, *, write: bool) -> PrefixedStor if owner and owner != me: granted = share_access(app_id, owner, me) if share_access else None if granted not in ("rw", "ro") or (write and granted != "rw"): - # 404, not 403: a caller probing for owners learns nothing. - raise HTTPException(status_code=404, detail="Not found") + # 404, not 403: a caller probing for owners learns nothing. The detail is + # "no_access", so a grantee's client can tell a revoked share from a + # missing key (which says "no_key"); a stranger never gets past here. + raise HTTPException(status_code=404, detail=NO_ACCESS) whose = owner else: whose = me @@ -157,13 +170,14 @@ def _precondition(request: Request, store: PrefixedStore, key: str) -> None: raise HTTPException(status_code=412, detail={"value": value, "etag": etag}) @router.get("/api/{app_id}/store") + @router.get("/api/{app_id}/store/", include_in_schema=False) async def list_values(app_id: str, request: Request, prefix: str = ""): store = _scoped_store(request, app_id, write=False) if prefix: _check_key(prefix) items: dict[str, Any] = {} truncated = False - for key in store.keys_under(prefix): + for key in sorted(store.keys_under(prefix)): if len(items) >= max_items: truncated = True break @@ -178,7 +192,7 @@ async def get_value(app_id: str, key: str, request: Request): _check_key(key) value, etag = _current(store, key) if etag is None: - raise HTTPException(status_code=404, detail=f"Key '{key}' not found") + raise HTTPException(status_code=404, detail=NO_KEY) return JSONResponse({"value": value}, headers={"ETag": etag}) @router.put("/api/{app_id}/store/{key:path}") @@ -186,15 +200,21 @@ async def put_value(app_id: str, key: str, request: Request): store = _scoped_store(request, app_id, write=True) _check_key(key) try: - body = await request.json() - except Exception as e: - raise HTTPException(status_code=400, detail="Body must be JSON") from e + body = json.loads(await request.body(), parse_constant=_refuse_constant) + except ValueError as e: + raise HTTPException(status_code=400, detail="Body must be JSON (no NaN or Infinity)") from e value = ( body.get("value") if isinstance(body, dict) and "value" in body else body ) + try: + # A lone surrogate parses, but no response could ever send it back: refuse it + # here, or one bad value makes every read of the collection fail. + json.dumps(value, ensure_ascii=False).encode("utf-8") + except UnicodeEncodeError as e: + raise HTTPException(status_code=400, detail="Body holds text that is not valid Unicode") from e + etag = etag_of(value) _precondition(request, store, key) store[key] = value - etag = etag_of(value) return JSONResponse({"ok": True, "etag": etag}, headers={"ETag": etag}) @router.delete("/api/{app_id}/store/{key:path}") @@ -205,7 +225,7 @@ async def delete_value(app_id: str, key: str, request: Request): try: del store[key] except KeyError: - raise HTTPException(status_code=404, detail=f"Key '{key}' not found") + raise HTTPException(status_code=404, detail=NO_KEY) return {"ok": True} return router diff --git a/misc/docs/decisions/0001-owner-granted-data-shares.md b/misc/docs/decisions/0001-owner-granted-data-shares.md index 6644e98..b007cb3 100644 --- a/misc/docs/decisions/0001-owner-granted-data-shares.md +++ b/misc/docs/decisions/0001-owner-granted-data-shares.md @@ -43,10 +43,11 @@ Password registration does not prove ownership of an address, and a deleted acco ### 4. How the per-user store honours it -- **`?owner=`** on `/api/{app_id}/store/...` selects whose prefix to use. Absent, or equal (after normalisation) to the signed-in user: their own, as today. Different: allowed only with an active share `(app_id, owner, user)`, and only for GET when the share is `"ro"`; otherwise **404** — a stranger probing for owners learns nothing. A client must read 404 under `?owner=` as "no access (any more)", not as "empty". -- **A list route**, `GET /api/{app_id}/store?prefix=

` → `{"items": {key: {"value": v, "etag": e}}}`. It lists by walking only the owner's directory under `p` (a `keys_under` fast path on the file backend), never the whole platform store; capped at `max_items` (default 5000) with `"truncated": true` past it. +- **`?owner=`** on `/api/{app_id}/store/...` selects whose prefix to use. Absent, or equal (after normalisation) to the signed-in user: their own, as today. Different: allowed only with an active share `(app_id, owner, user)`, and only for GET when the share is `"ro"`; otherwise **404** with detail `"no_access"` — a stranger probing for owners learns nothing more. A missing key is 404 with `"no_key"`, so a grantee's client can tell a revoked share from an absent record. +- **Values must round-trip.** `PUT` refuses JSON's `NaN`/`Infinity` and text with lone surrogates (400): either would be stored and then make every read of the collection fail. +- **A list route**, `GET /api/{app_id}/store?prefix=

` (sorted by key) → `{"items": {key: {"value": v, "etag": e}}}`. It lists by walking only the owner's directory under `p` (a `keys_under` fast path on the file backend), never the whole platform store; capped at `max_items` (default 5000) with `"truncated": true` past it. - **Conditional writes.** Every GET returns an `ETag` (a hash of the stored value; also in the list route's items). `PUT` and `DELETE` accept `If-Match: ` (or `If-None-Match: *` for create-only) and answer **412** with the current value and etag when it no longer matches. Two devices, or two people, writing the same key can then resolve instead of overwriting: the store stays generic, and the app decides what "resolve" means (the first consumer: the newer record by its own revision stamp wins). -- **CSRF on writes.** The store routes are under `/api/`, which the platform's CSRF middleware exempts. For an app with a per-user store, the store router itself requires the double-submit header (`X-CSRF-Token` matching the `enlace_csrf` cookie, the same check `/auth/*` uses) on every `PUT` and `DELETE`. Shares raise the stakes — a grantee's cookie now reaches someone else's data — so cookie SameSite alone is not enough on an origin that hosts many apps. +- **CSRF on writes.** The store routes are under `/api/`, which the platform's CSRF middleware exempts. The plugin therefore passes the store paths of every store-bearing app as `CSRFMiddleware(enforce_prefixes=...)`, so the double-submit header (`X-CSRF-Token` matching the `enlace_csrf` cookie, the same check `/auth/*` uses) is required on every `PUT` and `DELETE` there. A host that mounts `make_store_router` without the plugin must do the same. Shares raise the stakes — a grantee's cookie now reaches someone else's data — so cookie SameSite alone is not enough on an origin that hosts many apps. - **Which apps get a store**: `access = "protected:user"` apps, as today, plus any app whose `app.toml` sets `user_store = true` (`AppConfig` is `extra="allow"`). That lets a **public** app offer a store to visitors who happen to be signed in; anonymous requests get 401 and the app does what it did before. A `user_store` app is **not** added to the `protected:user` set the grants admin uses. ### 5. Where it sits @@ -57,7 +58,7 @@ Password registration does not prove ownership of an address, and a deleted acco | # | Seam | v1 default | Replacement you can point at | |---|---|---|---| -| 1 | where shares live | `platform_factory("shares")` (JSON files, as grants) | any `MutableMapping` — the factory's existing dol variant (`stores/backends.py`) | +| 1 | where shares live | `platform_factory("shares")` (JSON files, as grants) | any `MutableMapping` that takes nested `a/b/c` keys. The factory's dol variant (`stores/backends.py`) does **not** today (it resolves nested keys against the working directory); fixing it is a precondition of using it here | | 2 | how the store router decides | `share_access` built from `ShareStore` | an app-specific policy (e.g. a guardian relation) — same signature | NOT seams: groups of grantees, re-sharing, per-key scopes, a `can_manage` delegate flag (the admin path covers the child-owner case), aliases (a short name for the email), a web UI for shares beyond the admin API. Each is "no" on purpose; none has a consumer. @@ -66,6 +67,10 @@ Surfaces: HTTP (owner routes, admin API, store), CLI (admin). MCP or a frontend ## Consequences +- **Anyone signed in can offer a share to anyone else** — there is no acceptance step, and the `409` for an unknown grantee tells a signed-in user whether an email has an account. On a platform with closed, allow-listed registration both are among people the operator invited; on an open one they are a nuisance channel. So a client shows a received space with its owner's email beside the label (labels are capped at 80 characters), and an `accepted` flag set by the grantee is the additive fix when an open platform needs it. +- **A share does not bypass `allowed_users`.** On a `protected:user` app with an allow-list, a grantee not on it is refused at the gate before the store sees the share; add them to the allow-list (or grant the app) as well. +- **`/api/{app}/store` is reserved** for every app (the router is mounted before the apps), and store writes now need the CSRF header. No app on the first deployment used either. + - Per-user data stops being strictly private to its owner. A request with `?owner=` costs one share lookup (one small file read); without it the hot path is unchanged. - A public app that enables `user_store` must treat 401 as "no account mode" and 404 under `?owner=` as "access gone". - Store writes from a browser now need the CSRF header; existing `protected:user` apps that write to the store from a page must send it (none on the first deployment does today: the store is off there). diff --git a/tests/test_shares.py b/tests/test_shares.py index 37e4749..5f7a125 100644 --- a/tests/test_shares.py +++ b/tests/test_shares.py @@ -258,3 +258,39 @@ def test_deleting_an_account_removes_its_shares(platform): assert dad.delete(f"/_admin/api/users/{MUM}").status_code == 200 assert dad.get("/_admin/api/shares").json()["shares"] == [] _ = kid, mum + + +# --- review fixes ------------------------------------------------------------------- + + +def test_unsafe_values_never_break_a_collection(): + base, shares = {}, _store() + me = _router_client(base, shares, user=KID) + me.put("/api/practice/store/a", json={"value": {"t": "fine"}}) + # A lone surrogate (a browser can send one) is refused, so the collection stays readable. + lone = me.put("/api/practice/store/z", content=b'{"value": {"t": "\\ud800"}}', headers={"Content-Type": "application/json"}) + assert lone.status_code == 400, lone.text + assert me.get("/api/practice/store").status_code == 200 + assert me.get("/api/practice/store/").json()["items"].keys() == {"a"}, "the trailing slash lists too" + # Non-finite numbers are refused before anything is written. + nan = me.put("/api/practice/store/b", content=b'{"value": NaN}', headers={"Content-Type": "application/json"}) + assert nan.status_code == 400 and "b" not in {k.split("/")[-1] for k in base} + + +def test_404_details_tell_no_access_from_no_key(): + base, shares = {}, _store() + shares.share("practice", KID, MUM) + mum = _router_client(base, shares, user=MUM) + assert mum.get(f"/api/practice/store/nope?owner={KID}").json()["detail"] == "no_key" + assert mum.get(f"/api/practice/store/nope?owner={DAD}").json()["detail"] == "no_access" + + +def test_label_is_capped_and_trimmed(): + shares = _store() + assert shares.share("practice", KID, MUM, label=" Kid ")["label"] == "Kid" + assert shares.share("practice", KID, DAD, label=" ")["label"] is None + with pytest.raises(ShareError, match="label"): + shares.share("practice", KID, MUM, label="x" * 81) + with pytest.raises(ShareError) as missing: + shares.share("practice", KID, STRANGER) + assert missing.value.code == "no_account" From 624acb658286a9234d19b7f1143eb2f0e0210188 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Thu, 1 Oct 2026 21:49:23 +0530 Subject: [PATCH 3/4] Lint: wrap long lines, ruff format --- enlace_auth/__main__.py | 18 +++- enlace_auth/admin/routes.py | 23 +++-- enlace_auth/auth/share_routes.py | 24 +++-- enlace_auth/auth/shares.py | 21 ++-- enlace_auth/plugin.py | 12 ++- enlace_auth/stores/middleware.py | 32 +++++-- enlace_auth/stores/prefixed.py | 6 +- tests/test_shares.py | 160 +++++++++++++++++++++++++------ 8 files changed, 227 insertions(+), 69 deletions(-) diff --git a/enlace_auth/__main__.py b/enlace_auth/__main__.py index 725f415..a7b88f4 100644 --- a/enlace_auth/__main__.py +++ b/enlace_auth/__main__.py @@ -397,7 +397,7 @@ def revoke_grant(app_id: str, email: str, *, toml: str = "platform.toml"): def _load_share_store(toml_path: Path = Path("platform.toml")): - """Open the platform's owner-granted data shares, checking accounts against the user store.""" + """Open the platform's data shares, checking accounts against the user store.""" import os from enlace_auth.auth.shares import ShareStore @@ -421,7 +421,7 @@ def share( expires: str = None, toml: str = "platform.toml", ): - """Let GRANTEE act on OWNER's per-user data in an app (an owner-granted share, made by an admin). + """Let GRANTEE act on OWNER's per-user data in an app (a share, made by an admin). Both accounts must exist. Re-sharing the same pair replaces the share. @@ -441,8 +441,13 @@ def share( store = _load_share_store(Path(toml)) try: record = store.share( - app_id, owner, grantee, access=access, label=label, - expires_at=parse_expires_at(expires), granted_by="cli", + app_id, + owner, + grantee, + access=access, + label=label, + expires_at=parse_expires_at(expires), + granted_by="cli", ) except (GrantError, ShareError) as e: print(str(e), file=sys.stderr) @@ -465,7 +470,10 @@ def revoke_share(app_id: str, owner: str, grantee: str, *, toml: str = "platform if _load_share_store(Path(toml)).revoke(app_id, owner, grantee): print(f"Revoked {grantee.lower()} from {owner.lower()}'s {app_id} data.") else: - print(f"No share {owner.lower()} -> {grantee.lower()} on {app_id}.", file=sys.stderr) + print( + f"No share {owner.lower()} -> {grantee.lower()} on {app_id}.", + file=sys.stderr, + ) sys.exit(1) diff --git a/enlace_auth/admin/routes.py b/enlace_auth/admin/routes.py index fe1aa29..3f87183 100644 --- a/enlace_auth/admin/routes.py +++ b/enlace_auth/admin/routes.py @@ -48,13 +48,13 @@ from pydantic import BaseModel, EmailStr from enlace_auth.auth.grants import GrantError, parse_expires_at -from enlace_auth.auth.shares import ShareError from enlace_auth.auth.passwords import hash_password from enlace_auth.auth.reset_tokens import ( DEFAULT_HANDOFF_TTL, mint_reset_token, reset_url, ) +from enlace_auth.auth.shares import ShareError class _CreateUserBody(BaseModel): @@ -403,20 +403,31 @@ async def create_share(body: _CreateShareBody, request: Request) -> dict[str, An raise HTTPException(status_code=503, detail="Shares store unavailable") app_id = body.app_id.strip() if app_id not in store_set: - raise HTTPException(status_code=404, detail=f"No per-user store for app {app_id!r}") + raise HTTPException( + status_code=404, detail=f"No per-user store for app {app_id!r}" + ) try: record = share_store.share( - app_id, body.owner, body.grantee, access=body.access, label=body.label, - expires_at=parse_expires_at(body.expires_at), granted_by=actor, + app_id, + body.owner, + body.grantee, + access=body.access, + label=body.label, + expires_at=parse_expires_at(body.expires_at), + granted_by=actor, ) except ShareError as e: - raise HTTPException(status_code=409 if e.code == "no_account" else 422, detail=str(e)) + raise HTTPException( + status_code=409 if e.code == "no_account" else 422, detail=str(e) + ) except GrantError as e: raise HTTPException(status_code=422, detail=str(e)) return {"ok": True, "share": record} @router.delete("/shares/{app_id}/{owner}/{grantee}") - async def revoke_share(app_id: str, owner: str, grantee: str, request: Request) -> dict[str, Any]: + async def revoke_share( + app_id: str, owner: str, grantee: str, request: Request + ) -> dict[str, Any]: _require_admin(request) if share_store is None: raise HTTPException(status_code=503, detail="Shares store unavailable") diff --git a/enlace_auth/auth/share_routes.py b/enlace_auth/auth/share_routes.py index 36cdeb4..1752bdc 100644 --- a/enlace_auth/auth/share_routes.py +++ b/enlace_auth/auth/share_routes.py @@ -6,7 +6,8 @@ so the platform's double-submit CSRF check covers every write. - ``GET /auth/shares/{app_id}`` → ``{"owner", "granted": [...], "received": [...]}`` -- ``PUT /auth/shares/{app_id}/{grantee}`` (body ``{"access"?, "label"?, "expires_at"?}``) +- ``PUT /auth/shares/{app_id}/{grantee}`` + (body ``{"access"?, "label"?, "expires_at"?}``) - ``DELETE /auth/shares/{app_id}/received/{owner}`` — a grantee leaves a share - ``DELETE /auth/shares/{app_id}/{grantee}`` — the owner revokes one @@ -55,7 +56,9 @@ def _me(request: Request) -> str: def _app(app_id: str) -> str: if app_id not in _apps(): - raise HTTPException(status_code=404, detail=f"No per-user store for app {app_id!r}") + raise HTTPException( + status_code=404, detail=f"No per-user store for app {app_id!r}" + ) return app_id @router.get("/{app_id}") @@ -68,12 +71,19 @@ async def list_shares(app_id: str, request: Request) -> dict[str, Any]: } @router.put("/{app_id}/{grantee}") - async def put_share(app_id: str, grantee: str, body: _ShareBody, request: Request) -> dict[str, Any]: + async def put_share( + app_id: str, grantee: str, body: _ShareBody, request: Request + ) -> dict[str, Any]: me, app_id = _me(request), _app(app_id) try: record = share_store.share( - app_id, me, grantee, access=body.access, label=body.label, - expires_at=parse_expires_at(body.expires_at), granted_by=me, + app_id, + me, + grantee, + access=body.access, + label=body.label, + expires_at=parse_expires_at(body.expires_at), + granted_by=me, ) except ShareError as e: status = 409 if e.code == "no_account" else 422 @@ -91,7 +101,9 @@ async def leave_share(app_id: str, owner: str, request: Request) -> dict[str, An return {"ok": True} @router.delete("/{app_id}/{grantee}") - async def revoke_share(app_id: str, grantee: str, request: Request) -> dict[str, Any]: + async def revoke_share( + app_id: str, grantee: str, request: Request + ) -> dict[str, Any]: me, app_id = _me(request), _app(app_id) if not share_store.revoke(app_id, me, grantee): raise HTTPException(status_code=404, detail="Share not found") diff --git a/enlace_auth/auth/shares.py b/enlace_auth/auth/shares.py index bbb2843..7ecc8ba 100644 --- a/enlace_auth/auth/shares.py +++ b/enlace_auth/auth/shares.py @@ -48,12 +48,12 @@ ACCESS_LEVELS = ("rw", "ro") -#: The longest ``label`` a share may carry (it is shown to the grantee as a space's name). +#: The longest ``label`` a share may carry (the grantee sees it as the space's name). MAX_LABEL = 80 class ShareError(ValueError): - """Raised for an invalid share. ``code`` says which: ``"no_account"`` or ``"invalid"``.""" + """An invalid share. ``code`` says which: ``"no_account"`` or ``"invalid"``.""" def __init__(self, message: str, *, code: str = "invalid"): super().__init__(message) @@ -127,7 +127,8 @@ def share( missing = [e for e in (owner, grantee) if not self._account_exists(e)] if missing: raise ShareError( - f"No account for {', '.join(missing)}; a share names existing accounts only.", + f"No account for {', '.join(missing)}; " + "a share names existing accounts only.", code="no_account", ) label = (label or "").strip() or None @@ -163,14 +164,16 @@ def revoke(self, app_id: str, owner: str, grantee: str) -> bool: def access( self, app_id: str, owner: str, grantee: str, *, now: Optional[float] = None ) -> Optional[str]: - """``"rw"``, ``"ro"`` or ``None``: what ``grantee`` may do with ``owner``'s data.""" + """``"rw"``, ``"ro"`` or ``None``: what ``grantee`` may do with the data.""" record = self.get(app_id, owner, grantee) now = time.time() if now is None else now if record is None or not _is_active(record, now): return None return record.get("access") if record.get("access") in ACCESS_LEVELS else None - def granted(self, app_id: str, owner: str, *, now: Optional[float] = None) -> list[dict]: + def granted( + self, app_id: str, owner: str, *, now: Optional[float] = None + ) -> list[dict]: """Every share ``owner`` made in ``app_id``, each with ``"active"``.""" app_id, owner = _app(app_id), _email(owner) now = time.time() if now is None else now @@ -180,7 +183,9 @@ def granted(self, app_id: str, owner: str, *, now: Optional[float] = None) -> li if rec.get("owner") == owner ] - def received(self, app_id: str, grantee: str, *, now: Optional[float] = None) -> list[dict]: + def received( + self, app_id: str, grantee: str, *, now: Optional[float] = None + ) -> list[dict]: """The **active** shares made to ``grantee`` in ``app_id``.""" app_id, grantee = _app(app_id), _email(grantee) now = time.time() if now is None else now @@ -203,7 +208,7 @@ def list_all(self) -> list[dict]: return out def remove_account(self, email: str) -> int: - """Delete every share ``email`` is part of, as owner or grantee. Returns how many.""" + """Delete every share ``email`` is part of, either side. Returns how many.""" try: email = _email(email) except ShareError: @@ -224,7 +229,7 @@ def _records(self, app_id: str) -> Iterator[dict]: yield rec def _keys_for_app(self, app_id: str) -> Iterator[str]: - """``{app_id}/{owner}/{grantee}`` keys of one app — a directory scan when possible.""" + """The ``{app_id}/{owner}/{grantee}`` keys of one app (a directory scan).""" if self._root is not None: app_dir = self._root / app_id if not app_dir.is_dir(): diff --git a/enlace_auth/plugin.py b/enlace_auth/plugin.py index 034d1c2..569ee99 100644 --- a/enlace_auth/plugin.py +++ b/enlace_auth/plugin.py @@ -330,10 +330,10 @@ def wire(parent: "FastAPI", config) -> None: make_auth_router, ) from enlace_auth.auth.middleware import AccessRule - from enlace_auth.stores import StoreInjectionMiddleware, make_file_store_factory - from enlace_auth.stores.middleware import make_store_router from enlace_auth.auth.share_routes import make_share_router from enlace_auth.auth.shares import ShareStore + from enlace_auth.stores import StoreInjectionMiddleware, make_file_store_factory + from enlace_auth.stores.middleware import make_store_router platform_factory = make_file_store_factory(auth_cfg.stores.path) session_backend = platform_factory("sessions") @@ -573,10 +573,14 @@ def grants_resolver(app_id: str) -> set[str]: store_router = make_store_router( base_store_getter=lambda: user_data_backend, protected_apps=store_apps, - share_access=lambda app_id, owner, grantee: share_store.access(app_id, owner, grantee), + share_access=lambda app_id, owner, grantee: share_store.access( + app_id, owner, grantee + ), ) parent.include_router(store_router) - parent.include_router(make_share_router(share_store=share_store, store_apps=store_apps)) + parent.include_router( + make_share_router(share_store=share_store, store_apps=store_apps) + ) # App-metadata overlay: the editable launcher-metadata layer (owner-added # keywords + icon/title overrides). Core reads the overlay + can-edit closure diff --git a/enlace_auth/stores/middleware.py b/enlace_auth/stores/middleware.py index ca21f16..3a40b62 100644 --- a/enlace_auth/stores/middleware.py +++ b/enlace_auth/stores/middleware.py @@ -52,13 +52,13 @@ async def __call__(self, scope, receive, send): await self.app(scope, receive, send) -#: The 404 details of the store routes: no share for ``?owner=`` (or it was revoked), or no such key. +#: The 404 details of the store routes: no (more) share for ``?owner=``, or no such key. NO_ACCESS = "no_access" NO_KEY = "no_key" def _refuse_constant(name: str): - """JSON's ``NaN``/``Infinity`` are not JSON a browser can read back; refuse them on the way in.""" + """Refuse ``NaN``/``Infinity``: not JSON a browser can read back.""" raise ValueError(f"non-finite number {name}") @@ -69,7 +69,9 @@ def _refuse_constant(name: str): def etag_of(value: Any) -> str: """A strong ETag for a stored JSON value: a hash of its canonical serialisation.""" # ensure_ascii: a lone surrogate (which a browser can send) must hash, not raise. - canonical = json.dumps(value, sort_keys=True, separators=(",", ":"), ensure_ascii=True) + canonical = json.dumps( + value, sort_keys=True, separators=(",", ":"), ensure_ascii=True + ) return '"' + hashlib.sha256(canonical.encode("utf-8")).hexdigest()[:32] + '"' @@ -84,9 +86,11 @@ def make_store_router( Routes: - - ``GET /api/{app_id}/store?prefix=

`` → ``{"items": {key: {"value", "etag"}}, "truncated"}`` + - ``GET /api/{app_id}/store?prefix=

`` + → ``{"items": {key: {"value", "etag"}}, "truncated"}`` - ``GET /api/{app_id}/store/{key}`` → ``{"value": v}`` with an ``ETag`` header - - ``PUT /api/{app_id}/store/{key}`` — ``If-Match: `` / ``If-None-Match: *`` make it conditional + - ``PUT /api/{app_id}/store/{key}`` — conditional with ``If-Match: `` + or ``If-None-Match: *`` - ``DELETE /api/{app_id}/store/{key}`` — ``If-Match`` likewise A failed precondition is **412** with the current ``{"value", "etag"}`` (``value`` @@ -125,7 +129,7 @@ def _scoped_store(request: Request, app_id: str, *, write: bool) -> PrefixedStor if owner and owner != me: granted = share_access(app_id, owner, me) if share_access else None if granted not in ("rw", "ro") or (write and granted != "rw"): - # 404, not 403: a caller probing for owners learns nothing. The detail is + # 404, not 403: a caller probing for owners learns nothing. Detail is # "no_access", so a grantee's client can tell a revoked share from a # missing key (which says "no_key"); a stranger never gets past here. raise HTTPException(status_code=404, detail=NO_ACCESS) @@ -158,7 +162,11 @@ def _precondition(request: Request, store: PrefixedStore, key: str) -> None: if if_match is None and if_none_match is None: return value, etag = _current(store, key) - failed = if_none_match is not None and if_none_match.strip() == "*" and etag is not None + failed = ( + if_none_match is not None + and if_none_match.strip() == "*" + and etag is not None + ) if if_match is not None: wanted = if_match.strip() failed = failed or etag is None or (wanted != "*" and wanted != etag) @@ -202,16 +210,20 @@ async def put_value(app_id: str, key: str, request: Request): try: body = json.loads(await request.body(), parse_constant=_refuse_constant) except ValueError as e: - raise HTTPException(status_code=400, detail="Body must be JSON (no NaN or Infinity)") from e + raise HTTPException( + status_code=400, detail="Body must be JSON (no NaN or Infinity)" + ) from e value = ( body.get("value") if isinstance(body, dict) and "value" in body else body ) try: - # A lone surrogate parses, but no response could ever send it back: refuse it + # A lone surrogate parses, but no response could send it back: refuse it # here, or one bad value makes every read of the collection fail. json.dumps(value, ensure_ascii=False).encode("utf-8") except UnicodeEncodeError as e: - raise HTTPException(status_code=400, detail="Body holds text that is not valid Unicode") from e + raise HTTPException( + status_code=400, detail="Body holds text that is not valid Unicode" + ) from e etag = etag_of(value) _precondition(request, store, key) store[key] = value diff --git a/enlace_auth/stores/prefixed.py b/enlace_auth/stores/prefixed.py index 00618e0..b6813fb 100644 --- a/enlace_auth/stores/prefixed.py +++ b/enlace_auth/stores/prefixed.py @@ -72,7 +72,11 @@ def keys_under(self, sub: str = "") -> Iterator[str]: full = self._prefix + sub plen = len(self._prefix) fast = getattr(self._base, "keys_under", None) - keys = fast(full) if callable(fast) else (k for k in self._base if isinstance(k, str)) + keys = ( + fast(full) + if callable(fast) + else (k for k in self._base if isinstance(k, str)) + ) for k in keys: if k.startswith(full): yield k[plen:] diff --git a/tests/test_shares.py b/tests/test_shares.py index 5f7a125..de3761a 100644 --- a/tests/test_shares.py +++ b/tests/test_shares.py @@ -20,7 +20,12 @@ from enlace_auth.stores.prefixed import PrefixedStore KEY = "shares-key-32bytes-minimumlength!" -KID, MUM, DAD, STRANGER = "kid@example.com", "mum@example.com", "dad@example.com", "x@example.com" +KID, MUM, DAD, STRANGER = ( + "kid@example.com", + "mum@example.com", + "dad@example.com", + "x@example.com", +) # --- ShareStore ------------------------------------------------------------------- @@ -54,7 +59,9 @@ def test_access_levels_expiry_and_normalisation(): shares.share("practice", KID, MUM, expires_at=now + 10) assert shares.access("practice", KID, MUM, now=now + 20) is None assert [r["grantee"] for r in shares.received("practice", MUM, now=now + 20)] == [] - granted = {r["grantee"]: r["active"] for r in shares.granted("practice", KID, now=now + 20)} + granted = { + r["grantee"]: r["active"] for r in shares.granted("practice", KID, now=now + 20) + } assert granted == {MUM: False, DAD: True} assert shares.received("practice", DAD)[0]["label"] == "Kid" @@ -105,25 +112,48 @@ def test_owner_param_needs_an_active_share(): shares.share("practice", KID, DAD, access="ro") mum = _router_client(base, shares, user=MUM) - assert mum.get(f"/api/practice/store/attempts/a1?owner={KID}").json()["value"] == {"id": "a1"} - assert mum.put(f"/api/practice/store/attempts/a2?owner={KID}", json={"value": {"id": "a2"}}).status_code == 200 - assert base[f"{KID}/practice/attempts/a2"] == {"id": "a2"}, "written into the owner's data" + assert mum.get(f"/api/practice/store/attempts/a1?owner={KID}").json()["value"] == { + "id": "a1" + } + assert ( + mum.put( + f"/api/practice/store/attempts/a2?owner={KID}", json={"value": {"id": "a2"}} + ).status_code + == 200 + ) + assert base[f"{KID}/practice/attempts/a2"] == {"id": "a2"}, ( + "written into the owner's data" + ) dad = _router_client(base, shares, user=DAD) assert dad.get(f"/api/practice/store/attempts/a1?owner={KID}").status_code == 200 - assert dad.put(f"/api/practice/store/attempts/a3?owner={KID}", json={"value": 1}).status_code == 404, "ro: no writes" + assert ( + dad.put( + f"/api/practice/store/attempts/a3?owner={KID}", json={"value": 1} + ).status_code + == 404 + ), "ro: no writes" assert dad.delete(f"/api/practice/store/attempts/a1?owner={KID}").status_code == 404 stranger = _router_client(base, shares, user=STRANGER) - assert stranger.get(f"/api/practice/store/attempts/a1?owner={KID}").status_code == 404 + assert ( + stranger.get(f"/api/practice/store/attempts/a1?owner={KID}").status_code == 404 + ) assert stranger.get(f"/api/practice/store?owner={KID}").status_code == 404 # Own data is still the default, and ?owner= naming oneself is the same thing. assert mum.get("/api/practice/store/attempts/a1").status_code == 404 - assert mum.put(f"/api/practice/store/x?owner={MUM.upper()}", json={"value": 1}).status_code == 200 + assert ( + mum.put( + f"/api/practice/store/x?owner={MUM.upper()}", json={"value": 1} + ).status_code + == 200 + ) assert base[f"{MUM}/practice/x"] == 1 shares.revoke("practice", KID, MUM) - assert mum.get(f"/api/practice/store/attempts/a1?owner={KID}").status_code == 404, "revocation is immediate" + assert mum.get(f"/api/practice/store/attempts/a1?owner={KID}").status_code == 404, ( + "revocation is immediate" + ) def test_list_route_and_conditional_writes(): @@ -135,21 +165,54 @@ def test_list_route_and_conditional_writes(): me.put("/api/practice/store/marks/removed", json={"value": []}) listed = me.get("/api/practice/store?prefix=attempts/").json() - assert set(listed["items"]) == {"attempts/a1", "attempts/a2"} and not listed["truncated"] + assert ( + set(listed["items"]) == {"attempts/a1", "attempts/a2"} + and not listed["truncated"] + ) assert listed["items"]["attempts/a1"] == {"value": {"rev": 1}, "etag": etag} assert me.get("/api/practice/store/attempts/a1").headers["etag"] == etag # A writer holding the current etag wins; a stale one gets 412 with what is there now. - second = me.put("/api/practice/store/attempts/a1", json={"value": {"rev": 2}}, headers={"If-Match": etag}) + second = me.put( + "/api/practice/store/attempts/a1", + json={"value": {"rev": 2}}, + headers={"If-Match": etag}, + ) assert second.status_code == 200 - stale = me.put("/api/practice/store/attempts/a1", json={"value": {"rev": 3}}, headers={"If-Match": etag}) + stale = me.put( + "/api/practice/store/attempts/a1", + json={"value": {"rev": 3}}, + headers={"If-Match": etag}, + ) assert stale.status_code == 412 - assert stale.json()["detail"] == {"value": {"rev": 2}, "etag": second.headers["etag"]} + assert stale.json()["detail"] == { + "value": {"rev": 2}, + "etag": second.headers["etag"], + } assert base[f"{KID}/practice/attempts/a1"] == {"rev": 2} # Create-only. - assert me.put("/api/practice/store/attempts/a1", json={"value": 0}, headers={"If-None-Match": "*"}).status_code == 412 - assert me.put("/api/practice/store/attempts/a9", json={"value": 0}, headers={"If-None-Match": "*"}).status_code == 200 - assert me.delete("/api/practice/store/attempts/a9", headers={"If-Match": etag}).status_code == 412 + assert ( + me.put( + "/api/practice/store/attempts/a1", + json={"value": 0}, + headers={"If-None-Match": "*"}, + ).status_code + == 412 + ) + assert ( + me.put( + "/api/practice/store/attempts/a9", + json={"value": 0}, + headers={"If-None-Match": "*"}, + ).status_code + == 200 + ) + assert ( + me.delete( + "/api/practice/store/attempts/a9", headers={"If-Match": etag} + ).status_code + == 412 + ) assert me.delete("/api/practice/store/attempts/a9").status_code == 200 @@ -162,7 +225,11 @@ async def _fake_auth(request, call_next): request.state.user_id = KID return await call_next(request) - app.include_router(make_store_router(base_store_getter=lambda: base, protected_apps={"practice"}, max_items=3)) + app.include_router( + make_store_router( + base_store_getter=lambda: base, protected_apps={"practice"}, max_items=3 + ) + ) body = TestClient(app).get("/api/practice/store").json() assert len(body["items"]) == 3 and body["truncated"] @@ -173,7 +240,9 @@ def test_prefixed_keys_under_walks_only_the_owner(tmp_path): base[f"{MUM}/practice/attempts/b1"] = 2 store = PrefixedStore(base, f"{KID}/practice/") assert list(store.keys_under("attempts/")) == ["attempts/a1"] - assert list(base.keys_under(f"{KID}/practice/att")) == [f"{KID}/practice/attempts/a1"] + assert list(base.keys_under(f"{KID}/practice/att")) == [ + f"{KID}/practice/attempts/a1" + ] # --- end to end: a PUBLIC app with user_store = true --------------------------------- @@ -194,7 +263,9 @@ def ping(): """ ).strip() ) - (apps_dir / "practice" / "app.toml").write_text('access = "public"\nuser_store = true\n') + (apps_dir / "practice" / "app.toml").write_text( + 'access = "public"\nuser_store = true\n' + ) monkeypatch.setenv("ENLACE_SIGNING_KEY", KEY) monkeypatch.setenv("ENLACE_ADMIN_EMAILS", DAD) config = PlatformConfig( @@ -222,18 +293,31 @@ def _signed_in(app, email): def test_public_app_store_shares_and_csrf(platform): anonymous = TestClient(platform) - assert anonymous.get("/api/practice/ping").status_code == 200, "the app stays public" + assert anonymous.get("/api/practice/ping").status_code == 200, ( + "the app stays public" + ) assert anonymous.get("/api/practice/store?prefix=attempts/").status_code == 401 kid, mum = _signed_in(platform, KID), _signed_in(platform, MUM) - assert kid.put("/api/practice/store/attempts/a1", json={"value": {"id": "a1"}}).status_code == 200 + assert ( + kid.put( + "/api/practice/store/attempts/a1", json={"value": {"id": "a1"}} + ).status_code + == 200 + ) # CSRF is enforced on store writes even though /api/ is otherwise exempt. - bare = kid.put("/api/practice/store/attempts/a2", json={"value": 1}, headers={"X-CSRF-Token": ""}) + bare = kid.put( + "/api/practice/store/attempts/a2", + json={"value": 1}, + headers={"X-CSRF-Token": ""}, + ) assert bare.status_code == 403 # No share yet: Mum sees nothing of the kid's, and cannot share to a stranger. - assert mum.get(f"/api/practice/store?prefix=attempts/&owner={KID}").status_code == 404 + assert ( + mum.get(f"/api/practice/store?prefix=attempts/&owner={KID}").status_code == 404 + ) assert kid.put(f"/auth/shares/practice/{STRANGER}", json={}).status_code == 409 assert kid.put(f"/auth/shares/nostore/{MUM}", json={}).status_code == 404 @@ -250,9 +334,15 @@ def test_public_app_store_shares_and_csrf(platform): def test_deleting_an_account_removes_its_shares(platform): - kid, mum, dad = _signed_in(platform, KID), _signed_in(platform, MUM), _signed_in(platform, DAD) + kid, mum, dad = ( + _signed_in(platform, KID), + _signed_in(platform, MUM), + _signed_in(platform, DAD), + ) # Dad is the admin: he seeds the kid's share without her password. - r = dad.post("/_admin/api/shares", json={"app_id": "practice", "owner": KID, "grantee": MUM}) + r = dad.post( + "/_admin/api/shares", json={"app_id": "practice", "owner": KID, "grantee": MUM} + ) assert r.status_code == 200, r.text assert len(dad.get("/_admin/api/shares").json()["shares"]) == 1 assert dad.delete(f"/_admin/api/users/{MUM}").status_code == 200 @@ -268,12 +358,22 @@ def test_unsafe_values_never_break_a_collection(): me = _router_client(base, shares, user=KID) me.put("/api/practice/store/a", json={"value": {"t": "fine"}}) # A lone surrogate (a browser can send one) is refused, so the collection stays readable. - lone = me.put("/api/practice/store/z", content=b'{"value": {"t": "\\ud800"}}', headers={"Content-Type": "application/json"}) + lone = me.put( + "/api/practice/store/z", + content=b'{"value": {"t": "\\ud800"}}', + headers={"Content-Type": "application/json"}, + ) assert lone.status_code == 400, lone.text assert me.get("/api/practice/store").status_code == 200 - assert me.get("/api/practice/store/").json()["items"].keys() == {"a"}, "the trailing slash lists too" + assert me.get("/api/practice/store/").json()["items"].keys() == {"a"}, ( + "the trailing slash lists too" + ) # Non-finite numbers are refused before anything is written. - nan = me.put("/api/practice/store/b", content=b'{"value": NaN}', headers={"Content-Type": "application/json"}) + nan = me.put( + "/api/practice/store/b", + content=b'{"value": NaN}', + headers={"Content-Type": "application/json"}, + ) assert nan.status_code == 400 and "b" not in {k.split("/")[-1] for k in base} @@ -282,7 +382,9 @@ def test_404_details_tell_no_access_from_no_key(): shares.share("practice", KID, MUM) mum = _router_client(base, shares, user=MUM) assert mum.get(f"/api/practice/store/nope?owner={KID}").json()["detail"] == "no_key" - assert mum.get(f"/api/practice/store/nope?owner={DAD}").json()["detail"] == "no_access" + assert ( + mum.get(f"/api/practice/store/nope?owner={DAD}").json()["detail"] == "no_access" + ) def test_label_is_capped_and_trimmed(): From b7dbd4e09421931dca92a67ea2763c29d505780c Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Thu, 1 Oct 2026 21:53:03 +0530 Subject: [PATCH 4/4] Require enlace>=0.1.43, the release that carries AppConfig.user_store --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 0fc07fc..9d573da 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -11,7 +11,7 @@ requires-python = ">=3.10" keywords = ["enlace", "auth", "fastapi", "platform", "argon2", "session", "admin"] authors = [{ name = "Thor Whalen" }] dependencies = [ - "enlace>=0.1.42", # needs AppConfig.user_store (and enlace.access) + "enlace>=0.1.43", # needs AppConfig.user_store (and enlace.access) "fastapi>=0.100.0", "pydantic>=2.0.0", "itsdangerous>=2.1",