From 8c8dab12e38e3393bcae13b49999d55ab49b1cbd Mon Sep 17 00:00:00 2001 From: thethreemagi Date: Fri, 4 Sep 2026 12:09:19 -0400 Subject: [PATCH] P1: gate members documents per unit, session auth on the admin API documents.visible() now takes the viewer's unit set. A members document is served and listed only to a signed-in member of the matching unit; 'both' reaches any member; owner and admin reach everything including unit types added later. Everyone else gets 404, never 403. The gate moved INSIDE find(), so there is one path from a slug to a file and no route can forget to check. listed() and visible() stay separate functions. admin_api takes a session first and falls back to X-Admin-Token as break glass. Still fails closed: no session and no ADMIN_TOKEN is 503. Capability, not role, decides per route. announcements.created_by now comes from the session and ignores any value in the request body. tests/smoke_documents.py, 24 checks, including the invariant that the index can never list something serving would refuse. --- app/admin_api.py | 81 +++++++++++++++++++--------- app/app.py | 23 ++++++-- app/documents.py | 89 ++++++++++++++++++++++++++----- app/identity.py | 2 +- tests/smoke_documents.py | 111 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 262 insertions(+), 44 deletions(-) create mode 100644 tests/smoke_documents.py diff --git a/app/admin_api.py b/app/admin_api.py index 4e2f664..12a4615 100644 --- a/app/admin_api.py +++ b/app/admin_api.py @@ -19,17 +19,31 @@ shows, it comes down. Writing it is the entire feature. The caps that keep the banner from becoming a mess live in store.py, not here, so the panel and any future client inherit them rather than reimplementing them. -Auth: every route requires the X-Admin-Token header to match ADMIN_TOKEN. -If ADMIN_TOKEN is unset the whole router returns 503 - it FAILS CLOSED. These -endpoints expose parent names, emails and phone numbers for minors' families, -so an unconfigured deployment must not serve them. +Auth, two ways, session first. + +A signed-in person holding the required capability is allowed. Otherwise the +X-Admin-Token header must match ADMIN_TOKEN, which is retained as BREAK GLASS: +if the identity layer is broken there has to be a way in that does not depend +on the identity layer. + +Still FAILS CLOSED. With no session and no ADMIN_TOKEN set, every route returns +503. These endpoints expose parent names, emails and phone numbers for minors' +families, so an unconfigured deployment must not serve them. Adding sessions +widened who may read; it did not soften what happens when nothing is +configured. + +Capability, not role, decides. Lead routes need leads:read, announcement routes +need announcements:write, and both are answered by identity.can() against the +one CAPS dictionary - never by a role comparison here. """ import hmac import os -from fastapi import APIRouter, Body, Header, HTTPException, Query +from fastapi import APIRouter, Body, Header, HTTPException, Query, Request +import auth +import identity import store ADMIN_TOKEN = os.environ.get("ADMIN_TOKEN", "").strip() @@ -37,30 +51,42 @@ ADMIN_TOKEN = os.environ.get("ADMIN_TOKEN", "").strip() router = APIRouter(prefix="/api/admin", tags=["admin"]) -def _auth(token): +def _auth(request, token, capability): + """Authorise, and return a label naming who acted, for created_by. + + Order matters: session first, so a normal signed-in leader never depends on + the shared token, and the token stays a fallback rather than the everyday + path. + """ + person = auth.current_person(request) + if person: + if not identity.can(person, capability): + raise HTTPException(403, "your account does not have %s" % capability) + return person["email"] if not ADMIN_TOKEN: - raise HTTPException(503, "admin API disabled: ADMIN_TOKEN is not set") + raise HTTPException(503, "admin API disabled: sign in, or set ADMIN_TOKEN") if not token or not hmac.compare_digest(token, ADMIN_TOKEN): - raise HTTPException(401, "bad or missing X-Admin-Token") + raise HTTPException(401, "sign in, or send a valid X-Admin-Token") + return "admin-token" @router.get("/summary") -def get_summary(x_admin_token: str = Header(None)): - _auth(x_admin_token) +def get_summary(request: Request, x_admin_token: str = Header(None)): + _auth(request, x_admin_token, "leads:read") return store.summary() @router.get("/leads") -def get_leads(since: str = None, q: str = None, +def get_leads(request: Request, since: str = None, q: str = None, limit: int = Query(100, ge=1, le=500), offset: int = 0, x_admin_token: str = Header(None)): - _auth(x_admin_token) + _auth(request, x_admin_token, "leads:read") return {"leads": store.list_leads(since=since, q=q, limit=limit, offset=offset)} @router.get("/leads/{record_id}") -def get_one(record_id: str, x_admin_token: str = Header(None)): - _auth(x_admin_token) +def get_one(request: Request, record_id: str, x_admin_token: str = Header(None)): + _auth(request, x_admin_token, "leads:read") rec = store.get_lead(record_id) if not rec: raise HTTPException(404, "no such lead") @@ -68,16 +94,16 @@ def get_one(record_id: str, x_admin_token: str = Header(None)): @router.get("/mirrors/failed") -def failed(target: str = "google_sheet", x_admin_token: str = Header(None)): - _auth(x_admin_token) +def failed(request: Request, target: str = "google_sheet", x_admin_token: str = Header(None)): + _auth(request, x_admin_token, "leads:read") return {"target": target, "leads": store.failed_mirror_records(target)} @router.post("/mirrors/retry") -def retry(target: str = "google_sheet", x_admin_token: str = Header(None)): +def retry(request: Request, target: str = "google_sheet", x_admin_token: str = Header(None)): """Replay leads whose copy to an external target failed. Idempotent-ish: a lead already marked ok is never retried.""" - _auth(x_admin_token) + _auth(request, x_admin_token, "leads:read") if target != "google_sheet": raise HTTPException(422, "only google_sheet retry is implemented") import app as main_app @@ -106,25 +132,25 @@ def _reject(e): @router.get("/announcements") -def list_announcements(include_expired: bool = False, +def list_announcements(request: Request, include_expired: bool = False, limit: int = Query(100, ge=1, le=500), x_admin_token: str = Header(None)): """Every announcement with its computed state: live, scheduled, expired, revoked, or over_cap. State is returned rather than left to be inferred from what the homepage happens to render.""" - _auth(x_admin_token) + _auth(request, x_admin_token, "announcements:write") return {"announcements": store.list_announcements( include_expired=include_expired, limit=limit)} @router.post("/announcements", status_code=201) -def create_announcement(payload: dict = Body(...), x_admin_token: str = Header(None)): +def create_announcement(request: Request, payload: dict = Body(...), x_admin_token: str = Header(None)): """Post a notice. ends_at is required. 422 if the message is over the character cap or the window is invalid. 409 if the live cap is already reached, listing what is up so you can decide what to revoke.""" - _auth(x_admin_token) + _actor = _auth(request, x_admin_token, "announcements:write") try: return store.create_announcement( message=payload.get("message"), @@ -133,16 +159,21 @@ def create_announcement(payload: dict = Body(...), x_admin_token: str = Header(N level=payload.get("level", "info"), link_url=payload.get("link_url"), link_text=payload.get("link_text"), - created_by=payload.get("created_by"), + # From the session, never from the body. A caller must not be able + # to attribute a public notice to somebody else. Stored as the + # email rather than the uuid: the column is display-facing, people + # are never hard deleted, and a uuid in a banner audit trail helps + # nobody read it. + created_by=_actor, ) except store.AnnouncementRejected as e: raise _reject(e) @router.delete("/announcements/{announcement_id}") -def revoke_announcement(announcement_id: str, x_admin_token: str = Header(None)): +def revoke_announcement(request: Request, announcement_id: str, x_admin_token: str = Header(None)): """Take one down early. Sets revoked_at; never deletes the row.""" - _auth(x_admin_token) + _auth(request, x_admin_token, "announcements:write") if not store.get_announcement(announcement_id): raise HTTPException(404, "no such announcement") if not store.revoke_announcement(announcement_id): diff --git a/app/app.py b/app/app.py index 3d6028e..8d7b77c 100644 --- a/app/app.py +++ b/app/app.py @@ -710,8 +710,10 @@ def doc_row(d): f'{meta}') @app.get("/documents", response_class=HTMLResponse) -def documents_index(): - groups = documents.listing() +def documents_index(request: Request): + viewer = auth.current_person(request) + units = documents.units_for(viewer) + groups = documents.listing(units) if groups: cards = [] for cat, rows in groups: @@ -721,6 +723,14 @@ def documents_index(): inner = "".join(cards) else: inner = '
Nothing posted here yet. Forms and handouts will land on this page as the program year gets going.
' + if units: + signin_note = ('
' + 'Signed in, so anything posted for families is included above. ' + 'Your account
') + else: + signin_note = ('
' + 'Some handouts are posted for registered families only. ' + 'Sign in if you have an account.
') body = f"""
Pack & Troop 73
@@ -730,12 +740,15 @@ def documents_index():
{inner}
Can't find something, or need a form in another format? Message us on Facebook or catch a leader on a {MEETING_DAY} night. -
""" +
{signin_note}""" return page("Forms & Documents ยท Pack & Troop 73", body, "docs") @app.get("/documents/{slug}") -def document_file(slug: str): - d = documents.find(slug) +def document_file(slug: str, request: Request): + # A members document the viewer cannot reach is indistinguishable from one + # that does not exist. That is deliberate: 404, never 403, because a 403 + # confirms the document to someone holding nothing but a guessed slug. + d = documents.find(slug, documents.units_for(auth.current_person(request))) if d is None: body = """
diff --git a/app/documents.py b/app/documents.py index 1d4f4b3..43edef9 100644 --- a/app/documents.py +++ b/app/documents.py @@ -23,9 +23,19 @@ visibility: that need a durable link before member login exists. This is obscurity, not access control: treat an unlisted link as forwardable, because it is. - "members" reserved for the login that does not exist yet. Until it does, - these are hidden AND unservable, returning 404 rather than 403, - because a 403 advertises a document we cannot actually gate yet. + "members" served only to a signed-in member of the right unit, and listed + only for them. Everyone else gets 404, never 403: a 403 + advertises the existence of a document to someone who cannot + have it, and the slug is the only thing they would need. + +Unit scoping. A document's `unit` is "pack", "troop" or "both". A viewer is +resolved to the set of unit types they belong to, and "both" is readable by any +member of any unit. An owner or admin reads everything, including units created +after their role was granted. + +This module deliberately does NOT import identity. It reads a person as a plain +dict, so the coupling is a data shape rather than a module dependency and the +gate stays testable on its own. """ import datetime @@ -137,14 +147,62 @@ def manifest(): return _cache["value"] -def visible(doc): - """The single gate on SERVING. Member login plugs in here and nowhere else.""" - return doc.get("visibility") in ("public", "unlisted") and doc["path"].is_file() +# Every unit type that can appear on a document. A membership in a unit type +# outside this set (a Venturing crew, say) grants no document access until the +# manifest vocabulary is widened to match - failing closed is correct here. +DOC_UNITS = ("pack", "troop") -def listed(doc): - """The separate, weaker question of whether it appears on the index.""" - return doc.get("visibility") == "public" and visible(doc) +def units_for(person): + """The document unit tokens a viewer may read. Empty set for anonymous. + + An owner or admin gets everything, including a unit type added later. That + is the whole reason global_role is a column rather than a membership row. + """ + if not person or person.get("disabled_at"): + return frozenset() + if person.get("global_role") in ("owner", "admin"): + return frozenset(DOC_UNITS) + return frozenset( + m.get("unit_type") for m in person.get("memberships", []) + if m.get("unit_type") in DOC_UNITS) + + +def _member_ok(doc, units): + """Does this viewer's unit set reach this members-only document? + + "both" means the document concerns both units, so any member reaches it. + It does not mean "requires membership of both". + """ + if not units: + return False + return doc.get("unit") == "both" or doc.get("unit") in units + + +def visible(doc, units=None): + """The single gate on SERVING. Login attaches here and nowhere else.""" + if not doc["path"].is_file(): + return False + vis = doc.get("visibility") + if vis in ("public", "unlisted"): + return True + if vis == "members": + return _member_ok(doc, units or frozenset()) + return False + + +def listed(doc, units=None): + """The separate, weaker question of whether it appears on the index. + + Kept separate from visible() on purpose. Collapsing the two is how + "unlisted" quietly becomes public, or "members" quietly becomes servable. + """ + vis = doc.get("visibility") + if vis == "public": + return visible(doc, units) + if vis == "members": + return visible(doc, units) + return False def noindex(doc): @@ -152,10 +210,10 @@ def noindex(doc): return doc.get("visibility") == "unlisted" -def listing(): +def listing(units=None): """Listed documents grouped into their categories, in manifest order.""" m = manifest() - docs = [d for d in m["documents"] if listed(d)] + docs = [d for d in m["documents"] if listed(d, units)] known = {c["id"] for c in m["categories"]} groups = [] for cat in m["categories"]: @@ -168,13 +226,18 @@ def listing(): return groups -def find(slug): +def find(slug, units=None): + """The document at this slug, or None if the viewer cannot have it. + + The gate is applied HERE rather than by the caller, so there is exactly one + path from a slug to a file and no route can forget to check. + """ slug = (slug or "").strip().lower() if not SLUG_RE.match(slug): return None for d in manifest()["documents"]: if d["slug"] == slug: - return d if visible(d) else None + return d if visible(d, units) else None return None diff --git a/app/identity.py b/app/identity.py index 7e7e007..d87659c 100644 --- a/app/identity.py +++ b/app/identity.py @@ -339,7 +339,7 @@ def _person_row(con, r): p = dict(r) p.pop("password_hash", None) p["memberships"] = [dict(m) for m in con.execute( - "SELECT m.unit_id, m.role, m.title, u.slug, u.short_name, u.display_name" + "SELECT m.unit_id, m.role, m.title, u.slug, u.unit_type, u.short_name, u.display_name" " FROM memberships m JOIN units u ON u.id = m.unit_id" " WHERE m.person_id = ? ORDER BY u.sort_order", (p["id"],))] p["capabilities"] = sorted(effective_caps(p)) diff --git a/tests/smoke_documents.py b/tests/smoke_documents.py new file mode 100644 index 0000000..8008371 --- /dev/null +++ b/tests/smoke_documents.py @@ -0,0 +1,111 @@ +""" +smoke_documents.py - the members document gate, per unit. + +Builds a throwaway /docs tree and manifest, then checks who can SEE and who can +LIST each document. Runs in-process, stdlib only. + + python3 tests/smoke_documents.py + +The rules under test are the ones that fail silently: a members document must +be unservable AND unlisted to the wrong viewer, unlisted must stay servable but +off the index, and listing must never be able to show something serving would +refuse. +""" + +import json, os, sys, tempfile + +TMP = tempfile.mkdtemp() +DOCS = os.path.join(TMP, "docs"); os.makedirs(DOCS) +os.environ["DOCS_DIR"] = DOCS +os.environ["STORE_DB"] = os.path.join(TMP, "smoke.db") +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "app")) + +FILES = ["open.pdf", "hidden.pdf", "packonly.pdf", "trooponly.pdf", "bothunits.pdf"] +for f in FILES: + open(os.path.join(DOCS, f), "w").write("x") +json.dump({ + "categories": [{"id": "forms", "title": "Forms", "blurb": ""}], + "documents": [ + {"slug": "open", "file": "open.pdf", "title": "Open", "category": "forms", + "visibility": "public", "unit": "both"}, + {"slug": "hidden", "file": "hidden.pdf", "title": "Hidden", "category": "forms", + "visibility": "unlisted", "unit": "both"}, + {"slug": "packonly", "file": "packonly.pdf", "title": "Pack only", "category": "forms", + "visibility": "members", "unit": "pack"}, + {"slug": "trooponly", "file": "trooponly.pdf", "title": "Troop only", "category": "forms", + "visibility": "members", "unit": "troop"}, + {"slug": "bothunits", "file": "bothunits.pdf", "title": "Both", "category": "forms", + "visibility": "members", "unit": "both"}, + ]}, open(os.path.join(DOCS, "manifest.json"), "w")) + +import documents as D + +PASS = FAIL = 0 + + +def check(label, cond): + global PASS, FAIL + if cond: + PASS += 1; print(" ok %s" % label) + else: + FAIL += 1; print(" FAIL %s" % label) + + +def served(units): + return {s for s in ("open", "hidden", "packonly", "trooponly", "bothunits") + if D.find(s, units) is not None} + + +def listed(units): + return {d["slug"] for _, rows in D.listing(units) for d in rows} + + +anon = frozenset() +pack = D.units_for({"global_role": None, + "memberships": [{"unit_type": "pack", "role": "member"}]}) +troop = D.units_for({"global_role": None, + "memberships": [{"unit_type": "troop", "role": "leader"}]}) +both = D.units_for({"global_role": None, + "memberships": [{"unit_type": "pack", "role": "member"}, + {"unit_type": "troop", "role": "leader"}]}) +admin = D.units_for({"global_role": "admin", "memberships": []}) +crew = D.units_for({"global_role": None, + "memberships": [{"unit_type": "crew", "role": "leader"}]}) +off = D.units_for({"global_role": "owner", "memberships": [], "disabled_at": "2026-01-01T00:00:00+00:00"}) + +print("units_for") +check("anonymous gets nothing", D.units_for(None) == frozenset()) +check("pack member -> {pack}", pack == frozenset({"pack"})) +check("admin -> every unit type", admin == frozenset(D.DOC_UNITS)) +check("crew maps to nothing (manifest has no crew)", crew == frozenset()) +check("disabled owner gets nothing", off == frozenset()) + +print("\nserving") +check("anon: public and unlisted only", served(anon) == {"open", "hidden"}) +check("pack member: + pack and both", served(pack) == {"open", "hidden", "packonly", "bothunits"}) +check("troop leader: + troop and both", served(troop) == {"open", "hidden", "trooponly", "bothunits"}) +check("in both units: everything", served(both) == set(FILES and + {"open", "hidden", "packonly", "trooponly", "bothunits"})) +check("admin: everything", served(admin) == {"open", "hidden", "packonly", "trooponly", "bothunits"}) +check("pack member cannot reach troop doc", D.find("trooponly", pack) is None) +check("crew leader reaches no members doc", served(crew) == {"open", "hidden"}) + +print("\nlisting") +check("anon index is public only", listed(anon) == {"open"}) +check("unlisted never listed, for anyone", "hidden" not in listed(admin)) +check("pack member index", listed(pack) == {"open", "packonly", "bothunits"}) +check("troop leader index", listed(troop) == {"open", "trooponly", "bothunits"}) +check("admin index", listed(admin) == {"open", "packonly", "trooponly", "bothunits"}) + +print("\nlisting can never exceed serving") +for name, u in (("anon", anon), ("pack", pack), ("troop", troop), ("admin", admin), ("crew", crew)): + check("%s: listed is a subset of served" % name, listed(u) <= served(u)) + +print("\nmissing file on disk") +os.remove(os.path.join(DOCS, "packonly.pdf")) +D._cache["key"] = None +check("a members doc with no file is not served", D.find("packonly", pack) is None) +check("and not listed", "packonly" not in listed(pack)) + +print("\n%d passed, %d failed" % (PASS, FAIL)) +sys.exit(1 if FAIL else 0)