Repository navigation
Sprint 4 PR 4.5b — close /calendar/export auth gap - #226
Merged
Merged
Conversation
This was referenced Jun 25, 2026
tomqwu
pushed a commit
that referenced
this pull request
Jun 25, 2026
Summary: `GET /api/v1/calendar/org/export` previously accepted `person_id` as a query parameter and used it as the auth proxy — "must refer to an admin in this org." Person ids are deterministic (often email-derived slugs), so anyone who could enumerate or guess an admin's id could download the entire org's events. Auth is now JWT-only via Depends(get_current_admin_user), with the same-org check delegated to verify_org_member. This closes the follow-up explicitly named in PR #226 (4.5b). Changed files: - api/routers/calendar.py — replace spoofable person_id query param with Depends(get_current_admin_user) + verify_org_member(current_admin, org_id). The `person_id` query param is dropped from the signature; any extra query params remaining on legacy callers are ignored by FastAPI. - tests/api/test_calendar_auth.py — new TestOrgExportAuth class with 5 cases: unauth → 401/403, volunteer-in-same-org → 403, admin-cross-org → 403, admin-same-org-no-events → 404, and an explicit regression test that a volunteer cannot escalate by passing an admin's id as the legacy ?person_id= spoof param. - tests/conftest.py — extend the unit-tier verify_org_member monkey-patch list to include api.routers.calendar (it now imports the symbol directly so the patch must rebind there too). - tests/unit/test_calendar.py — drop the now-dead ?person_id= query param from the URLs; remove `test_org_export_as_volunteer_denied` (mocked auth always returns an admin so the case is not representable in this tier — the equivalent assertion lives in TestOrgExportAuth). - tests/contract/openapi.snapshot.json — refreshed via make update-openapi-snapshot: person_id parameter removed, HTTPBearer security added to the operation, description bumped. Validation: - poetry run pytest tests/api/test_calendar_auth.py tests/unit/test_calendar.py → 32 passed - make test-unit-fast → 338 passed, 21 skipped - poetry run pytest tests/api tests/contract → 316 passed - poetry run pytest tests/cli tests/integration → 59 passed - poetry run black api tests — clean - poetry run ruff check api tests — clean Follow-ups: - E2E lane will only go green once #227 (fix-e2e-stale-dates) lands and this branch is rebased onto the updated main. The stale-date flake is unrelated to this change and pre-exists in the suite.
Summary:
PR 4.5 added auth to /calendar/subscribe and /reset-token but missed
/calendar/export, which was still fully unauthenticated. Any caller could
download anyone's personal schedule as ICS by guessing or scraping
person_id values — a PII leak. Apply the same self-or-admin-in-same-org
rule already used by the sibling endpoints.
Changed files:
- api/routers/calendar.py:
export_personal_schedule now takes current_user via Depends(get_current_user)
and runs _ensure_self_or_same_org_admin after the person lookup.
- tests/api/test_calendar_auth.py:
New TestExportAuth class with 5 cases covering no-auth, cross-user,
cross-org admin, self, and admin-in-same-org. Allowed cases land on
the existing 404 "No assignments found" body — proving auth cleared
without having to seed an Assignment row.
- tests/contract/openapi.snapshot.json:
Refreshed; only adds HTTPBearer security on /api/v1/calendar/export
and the new sentence in its description.
Validation:
- poetry run pytest tests/api/test_calendar_auth.py -v -> 15 passed
- poetry run pytest tests/unit/test_calendar.py -v -> 18 passed (mocked
auth satisfies the new dep, no test changes needed)
- make test-unit-fast -> 339 passed, 21 skipped
- poetry run pytest tests/api -> 315 passed
- poetry run pytest tests/contract -> 1 passed
- black and ruff clean on touched files
Follow-ups:
- /calendar/org/export still uses the spoofable person_id-as-auth proxy
pattern (passes person_id in query, looks up the admin record by id
instead of by JWT). Worth a separate PR to switch it to
Depends(get_current_admin_user) + verify_org_member.
tomqwu
force-pushed
the
sprint-4-pr-4-5b-calendar-export-auth
branch
from
June 25, 2026 13:47
ac58119 to
3b67a6d
Compare
tomqwu
marked this pull request as ready for review
June 25, 2026 14:17
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The original Sprint 4 PR 4.5 added auth to
/calendar/subscribeand/calendar/reset-tokenand shipped the admin-only/calendar/{person_id}/admin-resetendpoint — but it missed a sibling endpoint:GET /api/v1/calendar/export?person_id=.... That route was still wired with onlyDepends(get_db), so any unauthenticated caller could download anyone's personal schedule as an ICS file by guessing or scrapingperson_idvalues.This is a PII leak, not a feature gap, so closing it under the same rule the other calendar endpoints already use: caller must be the target person or an admin in the same organization.
What changed
api/routers/calendar.py—export_personal_schedulenow takescurrent_user: Person = Depends(get_current_user)and runs_ensure_self_or_same_org_admin(current_user, person)right after the person lookup, matching the existing pattern in/subscribeand/reset-token.tests/api/test_calendar_auth.py— newTestExportAuthclass with 5 cases:401/403403403404 "No assignments found"(allowed cases reach the existing body logic without needing seeded assignments)404 "No assignments found"tests/contract/openapi.snapshot.json— refreshed viamake update-openapi-snapshot. The diff is small: addsHTTPBearersecurity on the export operation and one extra sentence in its description.Validation
poetry run pytest tests/api/test_calendar_auth.py -v→ 15 passed (5 new + 10 existing)poetry run pytest tests/unit/test_calendar.py -v→ 18 passed (existing tests use mocked auth, no changes needed)make test-unit-fast→ 339 passed, 21 skippedpoetry run pytest tests/api→ 315 passedpoetry run pytest tests/contract→ 1 passed (snapshot matches)poetry run black api testsclean;poetry run ruff check api testsclean on touched filesFollow-ups
GET /api/v1/calendar/org/exportstill uses the spoofableperson_id-as-auth proxy: it acceptsperson_idas a query parameter and looks up the admin record by that id instead of by the JWT. Worth its own PR to switch toDepends(get_current_admin_user)+verify_org_member, but kept out of scope here to keep this fix small and reviewable.Generated by Claude Code