Skip to content

Sprint 4 PR 4.5c — close /calendar/org/export auth gap - #228

Merged
tomqwu merged 1 commit into
mainfrom
sprint-4-pr-4-5c-calendar-org-export-auth
Jun 25, 2026
Merged

tomqwu merged 1 commit into
mainfrom
sprint-4-pr-4-5c-calendar-org-export-auth

Conversation

@tomqwu

@tomqwu tomqwu commented Jun 25, 2026

Copy link
Copy Markdown
Owner

Why

PR #226 (4.5b) closed three of the four calendar auth gaps but explicitly left this one open as a follow-up:

GET /api/v1/calendar/org/export still uses the spoofable person_id-as-auth proxy: it accepts person_id as a query parameter and looks up the admin record by that id instead of by the JWT.

Person ids are deterministic (often email-derived slugs), so anyone who can enumerate or guess an admin's id can download the entire organization's events — a PII / org-data leak. This PR closes that gap.

What changed

  • api/routers/calendar.py — export_organization_events now takes current_admin: Person = Depends(get_current_admin_user) and delegates the same-org check to verify_org_member(current_admin, org_id). The person_id query parameter (which was the spoof vector) has been removed from the signature. FastAPI silently ignores extra query params, so legacy clients still sending ?person_id=... won't 422.
  • tests/api/test_calendar_auth.py — new TestOrgExportAuth class with 5 cases:
    1. No auth header → 401/403
    2. Volunteer in same org → 403
    3. Admin in org A asking for org B → 403
    4. Admin in same org, no events seeded → 404 (auth passes; the empty-events 404 is the next gate)
    5. Explicit regression test: a volunteer trying to escalate by passing an admin's id in ?person_id=... → 403 (the legacy spoof vector no longer works)
  • tests/conftest.py — extend the unit-tier verify_org_member monkey-patch list to include api.routers.calendar, since that router now imports the symbol directly.
  • 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 admin so the case isn't 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, description updated.

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


Generated by Claude Code.


Generated by Claude Code

@tomqwu
tomqwu force-pushed the sprint-4-pr-4-5c-calendar-org-export-auth branch from 194f159 to f645388 Compare June 25, 2026 13:47
@tomqwu
tomqwu marked this pull request as ready for review June 25, 2026 14:17
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

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.
@tomqwu
tomqwu force-pushed the sprint-4-pr-4-5c-calendar-org-export-auth branch from f645388 to 9ae7d46 Compare June 25, 2026 14:21
@tomqwu
tomqwu merged commit a2e483b into main Jun 25, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants