Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ To send a Slack DM, pass the Slack user ID as `channel`: `send_slack(message=...

These are DIFFERENT VALUES. When bundling user data for the frontend, include both: `{user_id: propel_id, db_id: firestore_doc_id, name, profile_image}`. The frontend needs `db_id` to build profile links and `user_id` (propel) for matching against assignees/editors/mentions.

## Volunteer applications (`volunteers` collection) — staff-owned fields
`create_or_update_volunteer` (`services/volunteers_service.py`) has exactly ONE production caller: `handle_submit` (`api/volunteers/volunteers_views.py`), backing only the self-service `/api/{mentor,judge,hacker,volunteer,sponsor}/application/<event_id>/{submit,update}` routes. It still persists the whole `volunteer_data` dict (no allowlist — new form fields flow through for free) **except** `STAFF_OWNED_VOLUNTEER_FIELDS`, which is stripped from the payload once, before the create/update branch. That covers both paths: on update `set(merge=True)` omits the keys so stored values survive; on create it stops a payload from overriding the `isSelected: False` seed to self-approve. The set covers approval (`isSelected`), check-in (`isCheckedIn`/`checkedIn`/`checkInTime`/`checkInTimeList`/`checkOutTime`/`checkoutTimeList`), refund bookkeeping (`deposit_status`, `deposit_refund_*`), `certificates` and `sent_emails`. **Deliberately NOT in the set:** `stripe_payment_intent_id`/`deposit_amount_cents`/`deposit_disposition` — the hacker Stripe Checkout return legitimately writes those via `/update`; adding them breaks deposits. The bug that motivated this (Aug 2026): all five frontend forms shipped `isSelected: false` from their `initialFormData` (sponsor hardcoded it), and the old guard only preserved the stored flag when the key was *absent* — so every application edit silently un-approved an approved mentor/judge/volunteer/sponsor. The same strip also closes a **privilege-escalation hole that was reachable**: before this, ANY logged-in user could POST `isSelected: true` to `/submit` and land an already-approved mentor or judge doc (the create path seeds `isSelected: False` then does `volunteer_doc.update(volunteer_data)`), granting `MentorTeamPanel` write access via `user_is_mentor_for_event` and survey-trust via `get_user_event_roles`. The submit/update routes were also `@auth.optional_user` and `handle_submit` had an `elif 'user_id' in volunteer_data` identity fallback — but the *anonymous* variant was NOT actually exploitable: `send_slack_audit` interpolates `user.user_id` before that fallback, and PropelAuth's `LoggedOutUser` has no `user_id`, so an unauthenticated POST raised AttributeError → caught → 400. That's an accident, not a control, so the routes are now `@auth.require_user` and `handle_submit` takes identity from the token ONLY — never a body `user_id`. Approval changes only via `update_volunteer_selection` (`POST /api/admin/volunteer/<volunteer_id>/select`). Regression tests: `api/volunteers/tests/test_volunteers_service.py`. **Identity resolution is shared:** `find_volunteer_by_caller_identity(propel_user_id, event_id, volunteer_type)` (`services/volunteers_service.py`) is THE 3-way resolver — propel UUID → PropelAuth email → OAuth `user_id` — used by `handle_get` (the GET route), `create_or_update_volunteer` (the write path), and `api/mentors/mentors_service.py::_find_mentor_volunteer` (delegates). Keeping read and write on the same resolver is load-bearing: when the write path matched propel UUID only, a user whose doc was stored under another identity shape saw their app on read, edited it, missed the write lookup, and fell into the CREATE branch — spawning a duplicate `isSelected: False` doc and orphaning the approved one. The email step uses the **verified PropelAuth email only** — never the form-payload email, which would let a caller hijack someone else's application by typing their address. Don't add a fourth copy of this lookup; delegate. (Surveys' `get_user_event_roles` is intentionally separate — it scans all volunteer_types in one pass.) **Notification gate:** `_notifications_disabled()` in `volunteers_service.py` suppresses the Slack/Resend fan-out (`send_admin_notification_email`, `send_slack_volunteer_notification`, `send_volunteer_confirmation_email`, `send_mentor_checkin_notification`) when `ENVIRONMENT=test` — before this, unit tests exercising `create_or_update_volunteer` posted REAL Slack messages and attempted REAL Resend sends. Mirror this gate on any new outbound-notification function in this service.

## Volunteer time tracking (`/api/users/volunteering`)
GET/POST in `api/users/users_views.py` → `services/users_service.py`. Both resolve identity through `_resolve_and_ensure_user(propel_id)`, in this order so a broken OAuth provider token can NEVER block volunteering: **(1) `fetch_user_by_propel_id(propel_id)` — direct Firestore lookup on the stored `propel_id` field, NO external call (covers everyone who has saved a profile); (2) the OAuth provider round-trip (`get_oauth_user_from_propel_user_id` → `sub` → `fetch_user_by_user_id`), the best source for the OAuth-format `user_id` + avatar, lazily creating a doc for new users; (3) the PropelAuth user-metadata fallback (`_fetch_propel_metadata` → `auth.fetch_user_metadata_by_user_id`) — RELIABLE, does NOT depend on the provider token — which resolves an existing doc by email (backfilling `propel_id`) or lazily creates one from the metadata (`user_id` set to the propel UUID since we lack the oauth-format id without the provider call; `propel_id` is the canonical match so step 1 hits forever after).** The bug this fixes: the WRITE used to depend SOLELY on step 2; when `get_oauth_user_from_propel_user_id` returns None (expired/unavailable provider token, PropelAuth hiccup, or its 5-min negative cache) the write 404'd ("Couldn't log that time") while the read masked it by returning empty. **Critical:** `get_profile_metadata` (which creates the doc) ALSO depends on the OAuth round-trip, so a user whose OAuth has always failed may have NO doc at all — step 3 (metadata) is what resolves/creates them. `fetch_user_by_propel_id`/`fetch_user_by_email` live in `db/{db,firestore,mem}.py` (single-field equality queries — auto-indexed, no composite index). **Logging:** `get_oauth_user_from_propel_user_id` now logs the PropelAuth response BODY (truncated) on non-200 and a debug line when serving a cached miss — previously the root cause (e.g. "no linked OAuth connection", wrong `PROPEL_AUTH_URL`/`KEY`) was invisible during a tight retry window. Tests: `api/users/tests/test_volunteer_resolve.py` (6 cases). NOTE — date/locale is NOT a factor: `<input type=date>` always yields an ISO `yyyy-MM-dd` value regardless of the user's locale. `get_volunteering_time` now returns `([], 0, 0)` (never None/404) so the page shows a clean zero-state, and filters in a SINGLE pass — an entry may carry `commitmentHours`, `finalHours`, or BOTH (manual logs send both), no concat/duplicate. `save_volunteering_time` accepts an optional `timestamp` (backdated manual logs) + `manual:true` flag; hours are float-cleaned, non-negative, capped at 1000.

Expand Down
48 changes: 8 additions & 40 deletions api/mentors/mentors_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
from common.utils.slack import send_slack, send_slack_audit
from common.utils.firebase import get_hackathon_by_event_id
from services.users_service import get_propel_user_details_by_id
from services.volunteers_service import get_volunteer_by_email, get_volunteer_by_user_id
from services.volunteers_service import find_volunteer_by_caller_identity
from services.teams_service import get_team

logger = logging.getLogger("myapp")
Expand Down Expand Up @@ -105,52 +105,20 @@ def _resolve_caller(propel_user_id):

def _find_mentor_volunteer(propel_user_id, event_id):
"""
Resolve the caller's mentor volunteer doc for THIS event, trying every
identity shape a doc may have been stored under. Mirrors handle_get
(volunteers_views.py) so the panel gate matches the same docs the
GET-application path already matches:

1. raw PropelAuth UUID — self-submitted apps store auth_user.user_id
(the propel UUID) in the doc's `user_id` field. THIS is the common
case and was the missing lookup that caused approved mentors with a
form-email != login-email to 403.
2. PropelAuth email — form-entered email == login email.
3. OAuth user_id — legacy docs that stored oauth2|slack|... .
Resolve the caller's mentor volunteer doc for THIS event via the shared
3-way resolver (propel UUID -> PropelAuth email -> OAuth user_id) in
volunteers_service — the same one the GET-application route and the
submit/update path use, so the panel gate matches the same docs.

Returns the first volunteer doc found (regardless of isSelected) or None.
"""
if not propel_user_id or not event_id:
return None

# 1. Direct match on the raw propel UUID (how handle_submit stores user_id).
try:
v = get_volunteer_by_user_id(propel_user_id, event_id, "mentor")
if v:
return v
return find_volunteer_by_caller_identity(propel_user_id, event_id, "mentor")
except Exception as e:
logger.warning("_find_mentor_volunteer: propel_id lookup failed: %s", e)

email, oauth_user_id, _ = _resolve_caller(propel_user_id)

# 2. Email match.
if email:
try:
v = get_volunteer_by_email(email, event_id, "mentor")
if v:
return v
except Exception as e:
logger.warning("_find_mentor_volunteer: email lookup failed: %s", e)

# 3. OAuth user_id match (legacy docs), only if it differs from the propel UUID.
if oauth_user_id and oauth_user_id != propel_user_id:
try:
v = get_volunteer_by_user_id(oauth_user_id, event_id, "mentor")
if v:
return v
except Exception as e:
logger.warning("_find_mentor_volunteer: user_id lookup failed: %s", e)

return None
logger.warning("_find_mentor_volunteer: lookup failed: %s", e)
return None


def user_is_mentor_for_event(propel_user_id, event_id) -> bool:
Expand Down
12 changes: 11 additions & 1 deletion api/volunteers/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,17 @@ Submit or update volunteer applications for specific events.
- `event_id`: Event ID

#### Authentication
- Optional user authentication
- **Required.** A valid bearer token is mandatory; identity is taken from the
token only, never from a `user_id` in the request body.

#### Staff-owned fields (accepted but ignored)
Approval, check-in and refund state are server-authoritative — an applicant's
payload can never set them. See `STAFF_OWNED_VOLUNTEER_FIELDS` in
`services/volunteers_service.py`. Notably `isSelected` is ignored here; approval
changes only via `POST /api/admin/volunteer/<volunteer_id>/select`. Deposit
*payment* fields (`stripe_payment_intent_id`, `deposit_amount_cents`,
`deposit_disposition`) are NOT in that set — the hacker Stripe return flow sets
them on `/update`.

#### Request Body (JSON)
```json
Expand Down
Loading
Loading