From c391777716c016648b35764e82e6a2774bbf1f52 Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Thu, 20 Aug 2026 21:25:14 -0700 Subject: [PATCH] Make volunteer approval server-authoritative; unify identity resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the bug where an approved mentor/judge/volunteer/sponsor editing their application was silently reset to isSelected=false (and could lose check-in / refund state the same way). - STAFF_OWNED_VOLUNTEER_FIELDS stripped from every self-service submit/update in create_or_update_volunteer: isSelected, check-in fields, refund bookkeeping, certificates, sent_emails. Deposit payment fields (stripe_payment_intent_id, deposit_amount_cents, deposit_disposition) deliberately excluded — the hacker Stripe return sets those on /update. Also closes authenticated self-approval on the create path (payload could override the isSelected=False seed). - find_volunteer_by_caller_identity(): shared 3-way resolver (propel UUID -> PropelAuth email -> OAuth user_id) now used by handle_get, create_or_update_volunteer, and mentors' _find_mentor_volunteer. Read and write matching the same docs stops edits from falling into the create branch and spawning duplicate isSelected=False docs. - All ten /api/{type}/application//{submit,update} routes are @auth.require_user (were optional_user); identity comes from the verified token only — the body user_id fallback is gone. - _notifications_disabled(): ENVIRONMENT=test suppresses the Slack/Resend fan-out — unit tests were posting real Slack messages and attempting real Resend sends. - Regression tests for all of the above (each verified to fail against the old code); fixed test_update_volunteer's stale .update() assert (code calls .set(merge=True)). Co-Authored-By: Claude Fable 5 --- CLAUDE.md | 3 + api/mentors/mentors_service.py | 48 +--- api/volunteers/README.md | 12 +- .../tests/test_volunteers_service.py | 232 +++++++++++++++++- api/volunteers/volunteers_views.py | 65 ++--- services/volunteers_service.py | 124 +++++++++- 6 files changed, 387 insertions(+), 97 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3b62d1d..2e86f74 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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//{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//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: `` 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. diff --git a/api/mentors/mentors_service.py b/api/mentors/mentors_service.py index d25e1e1..04d9aa8 100644 --- a/api/mentors/mentors_service.py +++ b/api/mentors/mentors_service.py @@ -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") @@ -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: diff --git a/api/volunteers/README.md b/api/volunteers/README.md index d37a077..2831724 100644 --- a/api/volunteers/README.md +++ b/api/volunteers/README.md @@ -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//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 diff --git a/api/volunteers/tests/test_volunteers_service.py b/api/volunteers/tests/test_volunteers_service.py index 6a4835b..c2a66d2 100644 --- a/api/volunteers/tests/test_volunteers_service.py +++ b/api/volunteers/tests/test_volunteers_service.py @@ -47,8 +47,11 @@ def test_create_volunteer(mock_get_db): mock_db.collection.return_value = mock_collection mock_collection.document.return_value = mock_doc - # Mock get_volunteer_by_user_id to return None (new volunteer) - with patch('services.volunteers_service.get_volunteer_by_user_id', return_value=None): + # Mock get_volunteer_by_user_id to return None (new volunteer). The shared + # identity resolver then falls back to PropelAuth — stub that out too so + # tests never make a network call. + with patch('services.volunteers_service.get_volunteer_by_user_id', return_value=None), \ + patch('services.users_service.get_propel_user_details_by_id', return_value=None): # Mock get_slack_user_by_email with patch('services.volunteers_service.get_slack_user_by_email', return_value={"id": "slack-123"}): # Call the function @@ -102,10 +105,231 @@ def test_update_volunteer(mock_get_db): assert result["id"] == "abc-123" assert result["shortBio"] == "Updated bio" - # Verify database calls + # Verify database calls. The update path writes via set(..., merge=True) + # so that brand-new fields land too. mock_db.collection.assert_called_with('volunteers') mock_collection.document.assert_called_once_with("abc-123") - mock_doc.update.assert_called_once() + mock_doc.set.assert_called_once() + + +# --------------------------------------------------------------------------- +# Staff-owned fields (STAFF_OWNED_VOLUNTEER_FIELDS) +# +# An applicant's own submit/update must never write approval, check-in or +# refund state. Regression cover for the bug where every application edit sent +# `isSelected: false` from the form's initial state and silently un-approved +# an already-approved mentor/judge/volunteer/sponsor. +# --------------------------------------------------------------------------- + +# The notification fan-out at the end of create/update talks to Slack and +# Resend for real; these tests only care about what gets written to Firestore. +def _silence_notifications(): + return [ + patch('services.volunteers_service.send_admin_notification_email'), + patch('services.volunteers_service.send_slack_volunteer_notification'), + patch('services.volunteers_service.send_volunteer_confirmation_email'), + patch('services.volunteers_service.get_slack_user_by_email', return_value=None), + ] + + +def _written_doc(mock_doc): + """The dict handed to Firestore by whichever write path ran.""" + if mock_doc.set.called: + return mock_doc.set.call_args[0][0] + return mock_doc.update.call_args[0][0] + + +@patch('services.volunteers_service.get_db') +def test_update_does_not_let_applicant_reset_isSelected(mock_get_db): + """An approved volunteer editing their application stays approved.""" + mock_db = MagicMock() + mock_get_db.return_value = mock_db + mock_collection = MagicMock() + mock_doc = MagicMock() + mock_db.collection.return_value = mock_collection + mock_collection.document.return_value = mock_doc + + approved = {**MOCK_VOLUNTEER_DOC, "isSelected": True} + + with patch('services.volunteers_service.get_volunteer_by_user_id', return_value=approved): + for p in _silence_notifications(): + p.start() + try: + result = create_or_update_volunteer( + user_id=MOCK_USER_ID, + email=MOCK_EMAIL, + event_id=MOCK_EVENT_ID, + volunteer_data={ + **MOCK_MENTOR_DATA, + "shortBio": "Updated bio", + # What the mentor/volunteer/sponsor forms actually send. + "isSelected": False, + }, + ) + finally: + patch.stopall() + + written = _written_doc(mock_doc) + assert written["isSelected"] is True, "applicant payload must not un-approve" + assert result["isSelected"] is True + # The rest of the edit still goes through. + assert written["shortBio"] == "Updated bio" + + +@patch('services.volunteers_service.get_db') +def test_update_does_not_let_applicant_clobber_checkin_or_refund_state(mock_get_db): + """Check-in and refund bookkeeping are staff/system-owned.""" + mock_db = MagicMock() + mock_get_db.return_value = mock_db + mock_collection = MagicMock() + mock_doc = MagicMock() + mock_db.collection.return_value = mock_collection + mock_collection.document.return_value = mock_doc + + existing = { + **MOCK_VOLUNTEER_DOC, + "isSelected": True, + "checkInTime": "2026-10-12T09:00:00", + "isCheckedIn": True, + "deposit_status": "refunded", + } + + with patch('services.volunteers_service.get_volunteer_by_user_id', return_value=existing): + for p in _silence_notifications(): + p.start() + try: + create_or_update_volunteer( + user_id=MOCK_USER_ID, + email=MOCK_EMAIL, + event_id=MOCK_EVENT_ID, + volunteer_data={ + **MOCK_MENTOR_DATA, + "checkInTime": None, + "isCheckedIn": False, + "deposit_status": "paid", + "deposit_refund_id": "re_attacker", + }, + ) + finally: + patch.stopall() + + written = _written_doc(mock_doc) + # Omitted from the merge write, so Firestore keeps the stored values. + for field in ("checkInTime", "isCheckedIn", "deposit_status", "deposit_refund_id"): + assert field not in written, f"{field} must not be writable by an applicant" + + +@patch('services.volunteers_service.get_db') +def test_create_ignores_applicant_supplied_isSelected(mock_get_db): + """A first-time applicant cannot self-approve via the payload.""" + mock_db = MagicMock() + mock_get_db.return_value = mock_db + mock_collection = MagicMock() + mock_doc = MagicMock() + mock_db.collection.return_value = mock_collection + mock_collection.document.return_value = mock_doc + + with patch('services.volunteers_service.get_volunteer_by_user_id', return_value=None), \ + patch('services.users_service.get_propel_user_details_by_id', return_value=None): + for p in _silence_notifications(): + p.start() + try: + result = create_or_update_volunteer( + user_id=MOCK_USER_ID, + email=MOCK_EMAIL, + event_id=MOCK_EVENT_ID, + volunteer_data={**MOCK_MENTOR_DATA, "isSelected": True}, + ) + finally: + patch.stopall() + + assert _written_doc(mock_doc)["isSelected"] is False + assert result["isSelected"] is False + + +@patch('services.volunteers_service.get_db') +def test_update_still_accepts_deposit_payment_fields(mock_get_db): + """The hacker Stripe return sets these on /update — must not be stripped.""" + mock_db = MagicMock() + mock_get_db.return_value = mock_db + mock_collection = MagicMock() + mock_doc = MagicMock() + mock_db.collection.return_value = mock_collection + mock_collection.document.return_value = mock_doc + + existing = {**MOCK_VOLUNTEER_DOC, "volunteer_type": "hacker", "type": "hackers"} + + with patch('services.volunteers_service.get_volunteer_by_user_id', return_value=existing): + for p in _silence_notifications(): + p.start() + try: + create_or_update_volunteer( + user_id=MOCK_USER_ID, + email=MOCK_EMAIL, + event_id=MOCK_EVENT_ID, + volunteer_data={ + "volunteer_type": "hacker", + "type": "hackers", + "stripe_payment_intent_id": "pi_123", + "deposit_amount_cents": 2500, + "deposit_disposition": "refund", + }, + ) + finally: + patch.stopall() + + written = _written_doc(mock_doc) + assert written["stripe_payment_intent_id"] == "pi_123" + assert written["deposit_amount_cents"] == 2500 + assert written["deposit_disposition"] == "refund" + + +@patch('services.volunteers_service.get_db') +def test_update_resolves_existing_doc_by_email_when_user_id_differs(mock_get_db): + """The duplicate-doc bug: a volunteer doc stored under a different identity + shape (email match, OAuth user_id, admin-created) must hit the UPDATE + branch, not spawn a second isSelected=False doc. The write path uses the + same 3-way resolver as the GET route (find_volunteer_by_caller_identity).""" + mock_db = MagicMock() + mock_get_db.return_value = mock_db + mock_collection = MagicMock() + mock_doc = MagicMock() + mock_db.collection.return_value = mock_collection + mock_collection.document.return_value = mock_doc + + # Doc was stored with an OAuth-shaped user_id, so the propel-UUID lookup + # misses; the PropelAuth-email lookup is what finds it. + approved_legacy_doc = { + **MOCK_VOLUNTEER_DOC, + "user_id": "oauth2|slack|T1Q7936BH-U123ABC", + "isSelected": True, + } + + with patch('services.volunteers_service.get_volunteer_by_user_id', return_value=None), \ + patch('services.users_service.get_propel_user_details_by_id', + return_value=(MOCK_EMAIL, "oauth2|slack|T1Q7936BH-U123ABC", None, None, "Test Mentor", "tm")), \ + patch('services.volunteers_service.get_volunteer_by_email', return_value=approved_legacy_doc): + for p in _silence_notifications(): + p.start() + try: + result = create_or_update_volunteer( + user_id=MOCK_USER_ID, # propel UUID != stored user_id + email=MOCK_EMAIL, + event_id=MOCK_EVENT_ID, + volunteer_data={**MOCK_MENTOR_DATA, "shortBio": "Edited bio"}, + ) + finally: + patch.stopall() + + # UPDATE branch: writes to the EXISTING doc id — no new doc created. + mock_collection.document.assert_called_once_with("abc-123") + written = _written_doc(mock_doc) + assert written["shortBio"] == "Edited bio" + # Approval and the doc's original identity survive the edit. + assert written["isSelected"] is True + assert written["user_id"] == "oauth2|slack|T1Q7936BH-U123ABC" + assert result["isSelected"] is True + @patch('services.volunteers_service.get_db') def test_get_volunteers_by_event(mock_get_db): diff --git a/api/volunteers/volunteers_views.py b/api/volunteers/volunteers_views.py index c8a97af..1d86783 100644 --- a/api/volunteers/volunteers_views.py +++ b/api/volunteers/volunteers_views.py @@ -6,8 +6,7 @@ from common.exceptions import InvalidUsageError from common.utils.slack import send_slack_audit from services.volunteers_service import ( - get_volunteer_by_user_id, - get_volunteer_by_email, + find_volunteer_by_caller_identity, get_volunteers_by_event, create_or_update_volunteer, update_volunteer_selection, @@ -97,13 +96,13 @@ def handle_submit(user, event_id: str, volunteer_type: str) -> Tuple[Dict[str, A if not isinstance(email, str) or '@' not in email: return _error_response("Invalid email format", 400) - # Set user_id if provided in user, otherwise see if it's in volunteer_data, otherwise use None - user_id = None - if hasattr(user, 'user_id'): - user_id = user.user_id - elif 'user_id' in volunteer_data: - user_id = volunteer_data['user_id'] - + # Identity comes from the verified token only. These routes are + # @auth.require_user, so never trust a user_id supplied in the body — + # that let an anonymous caller submit an application as someone else. + user_id = getattr(user, 'user_id', None) + if not user_id: + return _error_response("Authentication required", 401) + # Set appropriate type field based on volunteer_type type_mapping = { 'mentor': 'mentors', @@ -143,29 +142,11 @@ def handle_get(user, event_id: str, volunteer_type: str) -> Tuple[Dict[str, Any] try: uid = getattr(user, "user_id", None) - # 1. Direct match on the id we were handed. For self-submitted apps this - # is the PropelAuth UUID (handle_submit stores auth_user.user_id), and - # for the ?userId= query-param path it's whatever the caller passed. - volunteer = get_volunteer_by_user_id(uid, event_id, volunteer_type) if uid else None - - # 2. Fall back to email / OAuth user_id. Volunteer docs created through - # flows that stored the OAuth identity (oauth2|slack|...) rather than - # the PropelAuth UUID won't match step 1, so resolve the caller the - # same way the mentor self-check does: email first, then OAuth user_id. - if volunteer is None and uid: - email = oauth_user_id = None - try: - from services.users_service import get_propel_user_details_by_id - details = get_propel_user_details_by_id(uid) or () - email = details[0] if len(details) > 0 else None - oauth_user_id = details[1] if len(details) > 1 else None - except Exception as resolve_err: - logger.warning(f"handle_get: could not resolve caller {uid}: {resolve_err}") - - if email: - volunteer = get_volunteer_by_email(email, event_id, volunteer_type) - if volunteer is None and oauth_user_id and oauth_user_id != uid: - volunteer = get_volunteer_by_user_id(oauth_user_id, event_id, volunteer_type) + # Shared 3-way resolver (propel UUID -> PropelAuth email -> OAuth + # user_id). The submit/update path uses the SAME resolver, so any doc + # this route can show, an update can also find — keeping edits from + # falling into the create branch and duplicating the application. + volunteer = find_volunteer_by_caller_identity(uid, event_id, volunteer_type) if volunteer: logger.info(f"Retrieved {volunteer_type} application for event {event_id}") @@ -194,7 +175,7 @@ def handle_admin_list(user, event_id: str, volunteer_type: str) -> Tuple[Dict[st # Mentor routes @bp.route('/mentor/application//submit', methods=['POST']) -@auth.optional_user +@auth.require_user def submit_mentor_application(event_id): """Submit a mentor application for a specific event.""" user = auth_user @@ -205,7 +186,7 @@ def submit_mentor_application(event_id): return handle_submit(user, event_id, 'mentor') @bp.route('/mentor/application//update', methods=['POST']) -@auth.optional_user +@auth.require_user def update_mentor_application(event_id): """Update a mentor application for a specific event.""" user = auth_user @@ -260,7 +241,7 @@ def get_my_volunteer_status_for_event(event_id): # Sponsor routes @bp.route('/sponsor/application//submit', methods=['POST']) -@auth.optional_user +@auth.require_user def submit_sponsor_application(event_id): """Submit a sponsor application for a specific event.""" user = auth_user @@ -271,7 +252,7 @@ def submit_sponsor_application(event_id): return handle_submit(user, event_id, 'sponsor') @bp.route('/sponsor/application//update', methods=['POST']) -@auth.optional_user +@auth.require_user def update_sponsor_application(event_id): """Update a sponsor application for a specific event.""" user = auth_user @@ -307,7 +288,7 @@ def admin_list_sponsors(user, org, event_id): # Judge routes @bp.route('/judge/application//submit', methods=['POST']) -@auth.optional_user +@auth.require_user def submit_judge_application(event_id): """Submit a judge application for a specific event.""" user = auth_user @@ -318,7 +299,7 @@ def submit_judge_application(event_id): return handle_submit(user, event_id, 'judge') @bp.route('/judge/application//update', methods=['POST']) -@auth.optional_user +@auth.require_user def update_judge_application(event_id): """Update a judge application for a specific event.""" user = auth_user @@ -354,7 +335,7 @@ def admin_list_judges(user, org, event_id): # Generic volunteer routes @bp.route('/volunteer/application//submit', methods=['POST']) -@auth.optional_user +@auth.require_user def submit_volunteer_application(event_id): """Submit a general volunteer application for a specific event.""" user = auth_user @@ -365,7 +346,7 @@ def submit_volunteer_application(event_id): return handle_submit(user, event_id, 'volunteer') @bp.route('/volunteer/application//update', methods=['POST']) -@auth.optional_user +@auth.require_user def update_volunteer_application(event_id): """Update a general volunteer application for a specific event.""" user = auth_user @@ -535,7 +516,7 @@ def stripe_webhook_hacker_deposit(): # Generic hacker routes @bp.route('/hacker/application//submit', methods=['POST']) -@auth.optional_user +@auth.require_user def submit_hacker_application(event_id): """Submit a hacker application for a specific event.""" user = auth_user @@ -546,7 +527,7 @@ def submit_hacker_application(event_id): return handle_submit(user, event_id, 'hacker') @bp.route('/hacker/application//update', methods=['POST']) -@auth.optional_user +@auth.require_user def update_hacker_application(event_id): """Update a hacker application for a specific event.""" user = auth_user diff --git a/services/volunteers_service.py b/services/volunteers_service.py index dd941b5..634dd4e 100644 --- a/services/volunteers_service.py +++ b/services/volunteers_service.py @@ -26,6 +26,34 @@ logger = get_logger("services.volunteers_service") +# Volunteer-doc fields that only staff / system flows may write. A self-service +# application submit or update must never change these — the applicant's own +# payload is not authoritative. Owners: approval -> update_volunteer_selection(); +# check-in/out -> mentor_checkin()/mentor_checkout(); refund bookkeeping -> +# refund_hacker_deposit() and the Stripe webhook handlers. +# +# Deliberately NOT listed: deposit_amount_cents, deposit_disposition and +# stripe_payment_intent_id — the hacker form legitimately sets those when it +# returns from Stripe Checkout, which happens on the /update path. +STAFF_OWNED_VOLUNTEER_FIELDS = frozenset({ + 'isSelected', + 'isCheckedIn', 'checkedIn', 'checkInTime', 'checkInTimeList', + 'checkOutTime', 'checkoutTimeList', + 'deposit_status', 'deposit_refund_id', 'deposit_refund_amount_cents', + 'deposit_refunded_at', 'deposit_refunded_by', 'deposit_refund_status_msg', + 'certificates', 'sent_emails', +}) + +def _notifications_disabled() -> bool: + """ + True when outbound notifications (Slack, Resend email) must be suppressed. + Mirrors the ENVIRONMENT=test gate get_db() uses for MockFirestore — without + this, unit tests exercising create_or_update_volunteer posted REAL Slack + messages and attempted REAL Resend sends. + """ + return os.environ.get("ENVIRONMENT") == "test" + + def _generate_volunteer_id() -> str: """Generate a unique ID for a volunteer.""" return str(uuid.uuid4()) @@ -84,11 +112,57 @@ def get_volunteer_by_email(email: str, event_id: str, volunteer_type: str) -> Op .where('event_id', '==', event_id) \ .where('volunteer_type', '==', volunteer_type) \ .limit(1).stream() - + for volunteer in volunteers: return volunteer.to_dict() return None + +def find_volunteer_by_caller_identity(propel_user_id: str, event_id: str, volunteer_type: str) -> Optional[Dict[str, Any]]: + """ + Resolve the calling user's volunteer doc for an event, trying every + identity shape a doc may have been stored under: + + 1. raw PropelAuth UUID — self-submitted apps store auth_user.user_id + (the propel UUID) in the doc's `user_id` field. The common case. + 2. PropelAuth email — docs created by admin/import flows, or legacy + self-submits, where `user_id` holds something else. The email comes + from the VERIFIED token identity (never the form payload, which would + let a caller hijack someone else's application by typing their email). + 3. OAuth user_id — legacy docs that stored oauth2|slack|... . + + This is the single resolver shared by the GET application route + (handle_get), the submit/update path (create_or_update_volunteer), and the + mentor gate (_find_mentor_volunteer). Keeping read and write on the SAME + resolver matters: when the read path found a doc the write path couldn't, + an application edit fell into the create branch and spawned a duplicate + isSelected=False doc, orphaning the approved one. + """ + if not propel_user_id: + return None + + volunteer = get_volunteer_by_user_id(propel_user_id, event_id, volunteer_type) + if volunteer: + return volunteer + + email = oauth_user_id = None + try: + # Lazy import: users_service lazily imports this module, so a + # top-level import here would risk a cycle. + from services.users_service import get_propel_user_details_by_id + details = get_propel_user_details_by_id(propel_user_id) or () + email = details[0] if len(details) > 0 else None + oauth_user_id = details[1] if len(details) > 1 else None + except Exception as e: + warning(logger, "Could not resolve caller identity via PropelAuth", + propel_user_id=propel_user_id, exc_info=e) + + if email: + volunteer = get_volunteer_by_email(email, event_id, volunteer_type) + if volunteer is None and oauth_user_id and oauth_user_id != propel_user_id: + volunteer = get_volunteer_by_user_id(oauth_user_id, event_id, volunteer_type) + return volunteer + # Function to clear all caches related to a volunteer def _clear_volunteer_caches(user_id: str, email: str, event_id: str, volunteer_type: str): """Clear all caches related to a specific volunteer.""" @@ -197,13 +271,17 @@ def send_volunteer_confirmation_email(first_name: str, last_name: str, email: st Returns: True if email was sent successfully, False otherwise """ + if _notifications_disabled(): + info(logger, "ENVIRONMENT=test — skipping volunteer confirmation email", email=email) + return None + resend_api_key = os.environ.get('RESEND_WELCOME_EMAIL_KEY') if not resend_api_key: error(logger, "Missing required environment variable", var_name="RESEND_WELCOME_EMAIL_KEY") return False - + resend.api_key = resend_api_key - + try: volunteer_type_readable = volunteer_type.capitalize() is_reviewed_role = volunteer_type.lower() in ("mentor", "judge") @@ -341,11 +419,15 @@ def send_admin_notification_email(volunteer_data: Dict[str, Any], is_update: boo Returns: True if email was sent successfully, False otherwise """ + if _notifications_disabled(): + info(logger, "ENVIRONMENT=test — skipping admin notification email") + return False + resend_api_key = os.environ.get('RESEND_WELCOME_EMAIL_KEY') if not resend_api_key: error(logger, "Missing required environment variable", var_name="RESEND_WELCOME_EMAIL_KEY") return False - + resend.api_key = resend_api_key try: @@ -390,6 +472,10 @@ def send_slack_volunteer_notification(volunteer_data: Dict[str, Any], is_update: Returns: True if notification was sent successfully, False otherwise """ + if _notifications_disabled(): + info(logger, "ENVIRONMENT=test — skipping Slack volunteer notification") + return False + import datetime as _dt # --- Derive display values --- @@ -903,9 +989,23 @@ def create_or_update_volunteer( db = get_db() volunteer_type = volunteer_data.get('volunteer_type') - - # Check if volunteer already exists - existing = get_volunteer_by_user_id(user_id, event_id, volunteer_type) + + # Applicants may not write staff-owned fields. Several application forms ship + # a stale `isSelected: false` from their initial form state, which used to + # silently un-approve the applicant on every edit. Stripping here (before the + # create/update branch) covers both paths: on update, set(merge=True) omits + # the keys so the stored values survive; on create, it stops a crafted + # payload from overriding the isSelected=False seed to self-approve. + volunteer_data = { + k: v for k, v in volunteer_data.items() + if k not in STAFF_OWNED_VOLUNTEER_FIELDS + } + + # Check if volunteer already exists. Use the SAME 3-way identity resolver + # as the GET route — matching by propel UUID only made a user whose doc was + # stored under another identity shape (email / OAuth id) fall into the + # create branch on edit, spawning a duplicate isSelected=False doc. + existing = find_volunteer_by_caller_identity(user_id, event_id, volunteer_type) if existing: # Update existing record @@ -927,9 +1027,9 @@ def create_or_update_volunteer( update_data['timestamp'] = existing.get('timestamp') update_data['status'] = existing.get('status', 'active') - # Keep existing isSelected status if not explicitly provided - if 'isSelected' not in volunteer_data: - update_data['isSelected'] = existing.get('isSelected', False) + # isSelected is staff-owned (stripped from volunteer_data above) — carry + # the stored value forward so docs predating the field still get one. + update_data['isSelected'] = existing.get('isSelected', False) # Log volunteer_data logger.debug(f"volunteer_data: {volunteer_data}") @@ -1864,6 +1964,10 @@ def send_mentor_checkin_notification(volunteer: Dict[str, Any], time_slot: Optio Returns True if notification was sent, False if skipped (no Slack account) or on error. """ + if _notifications_disabled(): + info(logger, "ENVIRONMENT=test — skipping mentor check-in notification") + return False + email = volunteer.get('email', '') areas_of_expertise = volunteer.get('expertise', []) specialties = volunteer.get('softwareEngineeringSpecifics', [])