From bbf503a7d750b7e98b2bd1c04619b6f6b3577c8c Mon Sep 17 00:00:00 2001 From: Albrecht Degering Date: Mon, 20 Jul 2026 18:35:57 +0200 Subject: [PATCH] fix(responses): reject stale scheduling answers --- src/govoplan_scheduling/backend/router.py | 3 + src/govoplan_scheduling/backend/schemas.py | 29 +++- src/govoplan_scheduling/backend/service.py | 159 +++++++++++++++++---- tests/test_response_editing.py | 135 ++++++++++++++++- tests/test_service.py | 24 +++- 5 files changed, 311 insertions(+), 39 deletions(-) diff --git a/src/govoplan_scheduling/backend/router.py b/src/govoplan_scheduling/backend/router.py index 08700b8..92d966a 100644 --- a/src/govoplan_scheduling/backend/router.py +++ b/src/govoplan_scheduling/backend/router.py @@ -31,6 +31,7 @@ from govoplan_scheduling.backend.schemas import ( ) from govoplan_scheduling.backend.runtime import get_registry from govoplan_scheduling.backend.service import ( + SchedulingConflictError, SchedulingError, SchedulingPermissionError, cancel_scheduling_request, @@ -115,6 +116,8 @@ def _can_manage_scheduling(principal: ApiPrincipal) -> bool: def _scheduling_http_error(exc: SchedulingError) -> HTTPException: + if isinstance(exc, SchedulingConflictError): + return HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(exc)) if isinstance(exc, SchedulingPermissionError): return HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=str(exc)) if str(exc) == "Scheduling request not found": diff --git a/src/govoplan_scheduling/backend/schemas.py b/src/govoplan_scheduling/backend/schemas.py index bdbcb6c..c58e0aa 100644 --- a/src/govoplan_scheduling/backend/schemas.py +++ b/src/govoplan_scheduling/backend/schemas.py @@ -2,8 +2,9 @@ from __future__ import annotations from datetime import datetime from typing import Any, Literal +from zoneinfo import ZoneInfo, ZoneInfoNotFoundError -from pydantic import BaseModel, ConfigDict, Field, model_validator +from pydantic import AwareDatetime, BaseModel, ConfigDict, Field, field_validator, model_validator SchedulingStatus = Literal["draft", "collecting", "closed", "decided", "handed_off", "cancelled", "archived"] @@ -13,6 +14,16 @@ SchedulingResultVisibility = Literal["organizer", "after_response", "after_close SchedulingAvailabilityValue = Literal["available", "maybe", "unavailable"] +def _known_timezone(value: str | None) -> str | None: + if value is None: + return None + try: + ZoneInfo(value) + except ZoneInfoNotFoundError as exc: + raise ValueError("timezone must be a valid IANA timezone") from exc + return value + + class SchedulingCalendarPreferences(BaseModel): model_config = ConfigDict(extra="forbid") @@ -28,12 +39,14 @@ class SchedulingCandidateSlotInput(BaseModel): label: str | None = Field(default=None, max_length=500) description: str | None = None - start_at: datetime - end_at: datetime + start_at: AwareDatetime + end_at: AwareDatetime timezone: str | None = Field(default=None, max_length=100) location: str | None = Field(default=None, max_length=500) metadata: dict[str, Any] = Field(default_factory=dict) + _validate_timezone = field_validator("timezone")(_known_timezone) + @model_validator(mode="after") def validate_range(self) -> "SchedulingCandidateSlotInput": if self.end_at <= self.start_at: @@ -52,12 +65,14 @@ class SchedulingCandidateSlotUpdateRequest(BaseModel): label: str | None = Field(default=None, min_length=1, max_length=500) description: str | None = None - start_at: datetime | None = None - end_at: datetime | None = None + start_at: AwareDatetime | None = None + end_at: AwareDatetime | None = None timezone: str | None = Field(default=None, min_length=1, max_length=100) location: str | None = Field(default=None, max_length=500) metadata: dict[str, Any] | None = None + _validate_timezone = field_validator("timezone")(_known_timezone) + @model_validator(mode="after") def validate_range(self) -> "SchedulingCandidateSlotUpdateRequest": if self.start_at is not None and self.end_at is not None and self.end_at <= self.start_at: @@ -94,6 +109,8 @@ class SchedulingRequestCreateRequest(BaseModel): create_participant_invitations: bool = True metadata: dict[str, Any] = Field(default_factory=dict) + _validate_timezone = field_validator("timezone")(_known_timezone) + class SchedulingRequestUpdateRequest(BaseModel): model_config = ConfigDict(extra="forbid") @@ -119,6 +136,7 @@ class SchedulingCandidateSlotResponse(BaseModel): timezone: str location: str | None = None position: int + revision: str freebusy_checked_at: datetime | None = None freebusy_status: str | None = None freebusy_conflicts: list[dict[str, Any]] = Field(default_factory=list) @@ -192,6 +210,7 @@ class SchedulingAvailabilityAnswerInput(BaseModel): slot_id: str value: SchedulingAvailabilityValue + option_revision: str = Field(min_length=64, max_length=64, pattern=r"^[0-9a-f]{64}$") class SchedulingAvailabilityResponseRequest(BaseModel): diff --git a/src/govoplan_scheduling/backend/service.py b/src/govoplan_scheduling/backend/service.py index 3e82187..b723fe7 100644 --- a/src/govoplan_scheduling/backend/service.py +++ b/src/govoplan_scheduling/backend/service.py @@ -1,5 +1,7 @@ from __future__ import annotations +import hashlib +import json from datetime import datetime, timezone from typing import Any @@ -19,6 +21,7 @@ from govoplan_core.core.poll import ( PollInvitationCommand, PollOptionUpdateCommand, PollOptionRequest, + PollResponseRef, PollResponseSubmissionProvider, PollSchedulingProvider, PollSubmitResponseCommand, @@ -46,6 +49,10 @@ class SchedulingPermissionError(SchedulingError): pass +class SchedulingConflictError(SchedulingError): + pass + + WORKFLOW_DRAFT = "draft" WORKFLOW_COLLECTING = "collecting_availability" WORKFLOW_CLOSED = "availability_closed" @@ -74,6 +81,27 @@ def _jsonable(value: Any) -> Any: return value +def scheduling_slot_revision(slot: SchedulingCandidateSlot) -> str: + """Stable semantic revision used to reject responses from stale forms.""" + + snapshot = { + "label": slot.label, + "description": slot.description, + "start_at": response_datetime(slot.start_at), + "end_at": response_datetime(slot.end_at), + "timezone": slot.timezone, + "location": slot.location, + "metadata": slot.metadata_ or {}, + } + encoded = json.dumps( + _jsonable(snapshot), + ensure_ascii=False, + sort_keys=True, + separators=(",", ":"), + ).encode("utf-8") + return hashlib.sha256(encoded).hexdigest() + + def _now() -> datetime: return utcnow() @@ -151,6 +179,28 @@ def _participant_respondent_id( raise SchedulingPermissionError("An authenticated account is required to respond") +def _participant_poll_response( + session: Session, + *, + request: SchedulingRequest, + participant: SchedulingParticipant, + actor_ids: tuple[str, ...], + canonical_respondent_id: str, +) -> PollResponseRef | None: + if request.poll_id is None: + raise SchedulingError("Scheduling request has no backing poll") + try: + return _poll_provider().get_response( + session, + tenant_id=request.tenant_id, + poll_id=request.poll_id, + respondent_ids=_actor_ids((*actor_ids, canonical_respondent_id)), + invitation_id=participant.poll_invitation_id, + ) + except PollCapabilityError as exc: + raise SchedulingError(str(exc)) from exc + + def _poll_option_inputs(request: SchedulingRequest) -> list[PollOptionRequest]: options: list[PollOptionRequest] = [] for slot in _active_slots(request): @@ -737,21 +787,22 @@ def refresh_participant_response_state( *, request: SchedulingRequest, actor_ids: tuple[str, ...] = (), + reset_missing: bool = False, ) -> None: if request.poll_id is None: return + participants = _active_participants(request) by_invitation_id = { participant.poll_invitation_id: participant - for participant in _active_participants(request) + for participant in participants if participant.poll_invitation_id is not None } - by_respondent_id = { - participant.respondent_id: participant - for participant in _active_participants(request) - if participant.respondent_id is not None - } - if not by_invitation_id and not by_respondent_id: - return + by_respondent_id: dict[str, SchedulingParticipant] = {} + for participant in participants: + stored_identity = (participant.metadata_ or {}).get("poll_response_respondent_id") + for identity in (participant.respondent_id, stored_identity): + if isinstance(identity, str) and identity: + by_respondent_id[identity] = participant try: responses = _poll_provider().list_responses( session, @@ -764,9 +815,11 @@ def refresh_participant_response_state( ids = _actor_ids(actor_ids) actor_participants = [ participant - for participant in _active_participants(request) + for participant in participants if participant.respondent_id in ids or participant.email in ids ] + matched_participant_ids: set[str] = set() + unmatched_response_count = 0 for response in responses: participant = by_invitation_id.get(response.invitation_id) if participant is None and response.respondent_id is not None: @@ -781,7 +834,19 @@ def refresh_participant_response_state( and len(actor_participants) == 1 ): participant = actor_participants[0] - if participant is not None and participant.status != "responded": + if participant is None: + unmatched_response_count += 1 + continue + matched_participant_ids.add(participant.id) + if response.respondent_id: + metadata = participant.metadata_ or {} + if metadata.get("poll_response_respondent_id") != response.respondent_id: + participant.metadata_ = { + **metadata, + "poll_response_respondent_id": response.respondent_id, + } + changed = True + if participant.status != "responded": participant.status = "responded" participant.responded_at = response_datetime(response.submitted_at) _emit_scheduling_center_notification( @@ -793,6 +858,24 @@ def refresh_participant_response_state( body_text=f"{participant.display_name or participant.email or 'A participant'} responded to {request.title}.", ) changed = True + if reset_missing: + for participant in participants: + if participant.status != "responded" or participant.id in matched_participant_ids: + continue + stored_identity = (participant.metadata_ or {}).get("poll_response_respondent_id") + if not isinstance(stored_identity, str) and unmatched_response_count: + # Do not reset an older projection while an active response + # exists that cannot yet be mapped safely to a participant. + continue + participant.status = "invited" if participant.poll_invitation_id else "draft" + participant.responded_at = None + if isinstance(stored_identity, str): + participant.metadata_ = { + key: value + for key, value in (participant.metadata_ or {}).items() + if key != "poll_response_respondent_id" + } + changed = True if changed: session.flush() @@ -1073,11 +1156,24 @@ def submit_scheduling_availability( participant, fallback_respondent_id=respondent_id, ) + existing_response = _participant_poll_response( + session, + request=request, + participant=participant, + actor_ids=actor_ids, + canonical_respondent_id=canonical_respondent_id, + ) + if existing_response is not None and existing_response.respondent_id: + canonical_respondent_id = existing_response.respondent_id answers: list[PollAnswerRequest] = [] for answer in payload.answers: slot = _selected_slot(request, slot_id=answer.slot_id) if slot.poll_option_id is None: raise SchedulingError("Scheduling slot has no backing poll option") + if answer.option_revision != scheduling_slot_revision(slot): + raise SchedulingConflictError( + "Scheduling options changed after this response form was loaded; reload before responding" + ) answers.append( PollAnswerRequest( option_id=slot.poll_option_id, @@ -1108,6 +1204,11 @@ def submit_scheduling_availability( first_response = participant.status != "responded" participant.status = "responded" participant.responded_at = response_datetime(response.submitted_at) + if response.respondent_id: + participant.metadata_ = { + **(participant.metadata_ or {}), + "poll_response_respondent_id": response.respondent_id, + } if first_response: _emit_scheduling_center_notification( session, @@ -1147,33 +1248,26 @@ def get_scheduling_availability_response( participant, fallback_respondent_id=respondent_id, ) - try: - responses = _poll_provider().list_responses( - session, - tenant_id=tenant_id, - poll_id=request.poll_id, - ) - except PollCapabilityError as exc: - raise SchedulingError(str(exc)) from exc - matching = [ - response - for response in responses - if response.respondent_id == canonical_respondent_id - or ( - participant.poll_invitation_id is not None - and response.invitation_id == participant.poll_invitation_id - ) - ] - response = max(matching, key=lambda item: response_datetime(item.submitted_at)) if matching else None + response = _participant_poll_response( + session, + request=request, + participant=participant, + actor_ids=actor_ids, + canonical_respondent_id=canonical_respondent_id, + ) slot_by_option_id = { slot.poll_option_id: slot for slot in _active_slots(request) if slot.poll_option_id is not None } + slot_by_option_key = { + f"slot-{slot.position + 1}": slot + for slot in _active_slots(request) + } answers: list[dict[str, str]] = [] if response is not None: for answer in response.answers: - slot = slot_by_option_id.get(answer.option_id) + slot = slot_by_option_id.get(answer.option_id) or slot_by_option_key.get(answer.option_key) if slot is None or answer.value not in {"available", "maybe", "unavailable"}: continue answers.append({"slot_id": slot.id, "value": str(answer.value)}) @@ -1337,6 +1431,12 @@ def update_scheduling_candidate_slot( except PollCapabilityError as exc: raise SchedulingError(str(exc)) from exc + refresh_participant_response_state( + session, + request=request, + reset_missing=True, + ) + slot.label = label slot.description = description slot.start_at = start_at @@ -1532,6 +1632,7 @@ def scheduling_slot_response(slot: SchedulingCandidateSlot) -> dict[str, Any]: "timezone": slot.timezone, "location": slot.location, "position": slot.position, + "revision": scheduling_slot_revision(slot), "freebusy_checked_at": response_datetime(slot.freebusy_checked_at), "freebusy_status": slot.freebusy_status, "freebusy_conflicts": slot.freebusy_conflicts or [], diff --git a/tests/test_response_editing.py b/tests/test_response_editing.py index bfbe8f0..ff1159e 100644 --- a/tests/test_response_editing.py +++ b/tests/test_response_editing.py @@ -4,6 +4,8 @@ import unittest from datetime import datetime, timedelta, timezone from types import SimpleNamespace +from fastapi import HTTPException +from pydantic import ValidationError from sqlalchemy import create_engine from sqlalchemy.orm import Session, sessionmaker @@ -40,6 +42,8 @@ from govoplan_scheduling.backend.service import ( SchedulingError, cancel_scheduling_request, create_scheduling_request, + scheduling_request_summary, + scheduling_slot_revision, update_scheduling_candidate_slot, ) @@ -99,7 +103,12 @@ class SchedulingResponseEditingTests(unittest.TestCase): user=SimpleNamespace(id=f"membership-{account_id}"), ) - def _request(self, *, status: str = "collecting") -> SchedulingRequest: + def _request( + self, + *, + status: str = "collecting", + allow_participant_updates: bool = True, + ) -> SchedulingRequest: start = datetime(2026, 7, 20, 9, tzinfo=timezone.utc) request, _tokens = create_scheduling_request( self.session, @@ -108,6 +117,7 @@ class SchedulingResponseEditingTests(unittest.TestCase): payload=SchedulingRequestCreateRequest( title="Response editing", status=status, + allow_participant_updates=allow_participant_updates, calendar=SchedulingCalendarPreferences(), slots=[ SchedulingCandidateSlotInput( @@ -136,8 +146,16 @@ class SchedulingResponseEditingTests(unittest.TestCase): request.id, SchedulingAvailabilityResponseRequest( answers=[ - SchedulingAvailabilityAnswerInput(slot_id=request.slots[0].id, value="available"), - SchedulingAvailabilityAnswerInput(slot_id=request.slots[1].id, value="maybe"), + SchedulingAvailabilityAnswerInput( + slot_id=request.slots[0].id, + value="available", + option_revision=scheduling_slot_revision(request.slots[0]), + ), + SchedulingAvailabilityAnswerInput( + slot_id=request.slots[1].id, + value="maybe", + option_revision=scheduling_slot_revision(request.slots[1]), + ), ] ), session=self.session, @@ -227,7 +245,7 @@ class SchedulingResponseEditingTests(unittest.TestCase): request_id=request.id, slot_id=slot.id, payload=SchedulingCandidateSlotUpdateRequest( - start_at=original_start + timedelta(minutes=15), + start_at=original_start.replace(tzinfo=timezone.utc) + timedelta(minutes=15), ), ) @@ -249,6 +267,115 @@ class SchedulingResponseEditingTests(unittest.TestCase): self.assertIsNotNone(cancelled.cancelled_at) self.assertEqual(poll.status, "draft") + def test_fully_invalidated_response_becomes_unanswered(self) -> None: + request = self._request() + alice = self._principal( + "alice-account", + email="alice@example.test", + scopes={RESPOND_SCOPE}, + ) + api_submit_scheduling_availability( + request.id, + SchedulingAvailabilityResponseRequest( + answers=[ + SchedulingAvailabilityAnswerInput( + slot_id=request.slots[0].id, + value="available", + option_revision=scheduling_slot_revision(request.slots[0]), + ) + ] + ), + session=self.session, + principal=alice, + ) + + api_update_scheduling_candidate_slot( + request.id, + request.slots[0].id, + SchedulingCandidateSlotUpdateRequest(label="Monday, revised"), + session=self.session, + principal=self._principal("organizer-1", email=None, scopes={WRITE_SCOPE}), + ) + current = api_get_my_scheduling_availability( + request.id, + session=self.session, + principal=alice, + ) + + self.assertFalse(current.has_response) + self.assertEqual(current.answers, []) + self.assertEqual(request.participants[0].status, "invited") + self.assertIsNone(request.participants[0].responded_at) + self.assertEqual( + scheduling_request_summary(self.session, tenant_id="tenant-1", request_id=request.id)["response_count"], + 0, + ) + + def test_option_change_is_blocked_when_response_updates_are_disabled(self) -> None: + request = self._request(allow_participant_updates=False) + self._submit_both(request) + original_label = request.slots[0].label + + with self.assertRaisesRegex(SchedulingError, "response updates are disabled"): + update_scheduling_candidate_slot( + self.session, + tenant_id="tenant-1", + request_id=request.id, + slot_id=request.slots[0].id, + payload=SchedulingCandidateSlotUpdateRequest(label="Monday, revised"), + ) + + self.assertEqual(request.slots[0].label, original_label) + response = self.session.query(PollResponse).filter(PollResponse.poll_id == request.poll_id).one() + self.assertEqual(len(response.answers), 2) + + def test_stale_option_revision_is_rejected_without_writing_a_response(self) -> None: + request = self._request() + stale_revision = scheduling_slot_revision(request.slots[0]) + api_update_scheduling_candidate_slot( + request.id, + request.slots[0].id, + SchedulingCandidateSlotUpdateRequest(label="Monday, revised"), + session=self.session, + principal=self._principal("organizer-1", email=None, scopes={WRITE_SCOPE}), + ) + + with self.assertRaises(HTTPException) as stale: + api_submit_scheduling_availability( + request.id, + SchedulingAvailabilityResponseRequest( + answers=[ + SchedulingAvailabilityAnswerInput( + slot_id=request.slots[0].id, + value="available", + option_revision=stale_revision, + ) + ] + ), + session=self.session, + principal=self._principal( + "alice-account", + email="alice@example.test", + scopes={RESPOND_SCOPE}, + ), + ) + + self.assertEqual(stale.exception.status_code, 409) + self.assertEqual(self.session.query(PollResponse).filter(PollResponse.poll_id == request.poll_id).count(), 0) + + def test_slot_inputs_require_aware_datetimes_and_known_timezones(self) -> None: + with self.assertRaises(ValidationError): + SchedulingCandidateSlotInput( + start_at=datetime(2026, 7, 20, 9), + end_at=datetime(2026, 7, 20, 10), + ) + with self.assertRaisesRegex(ValidationError, "valid IANA timezone"): + SchedulingCandidateSlotInput( + start_at=datetime(2026, 7, 20, 9, tzinfo=timezone.utc), + end_at=datetime(2026, 7, 20, 10, tzinfo=timezone.utc), + timezone="Mars/Olympus_Mons", + ) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_service.py b/tests/test_service.py index 7a963a3..ed8041c 100644 --- a/tests/test_service.py +++ b/tests/test_service.py @@ -44,6 +44,7 @@ from govoplan_scheduling.backend.router import ( api_create_tentative_calendar_holds, api_decide_scheduling_request, api_evaluate_calendar_freebusy, + api_get_my_scheduling_availability, api_get_scheduling_request, api_list_scheduling_requests, api_scheduling_summary, @@ -66,6 +67,7 @@ from govoplan_scheduling.backend.service import ( open_scheduling_request, require_visible_scheduling_results, scheduling_request_summary, + scheduling_slot_revision, update_scheduling_request, ) from govoplan_scheduling.backend.runtime import configure_runtime @@ -825,7 +827,12 @@ class SchedulingServiceTests(unittest.TestCase): "attacker", email="alice@example.test", membership_id="alice-membership", - scopes={SCHEDULING_READ_SCOPE, "poll:poll:read", "poll:response:write"}, + scopes={ + SCHEDULING_READ_SCOPE, + SCHEDULING_RESPOND_SCOPE, + "poll:poll:read", + "poll:response:write", + }, ) response = api_submit_poll_response( @@ -842,9 +849,19 @@ class SchedulingServiceTests(unittest.TestCase): session=self.session, principal=attacker, ) + current = api_get_my_scheduling_availability( + request.id, + session=self.session, + principal=attacker, + ) self.assertNotIn("invitation_id", response.metadata) self.assertEqual([request.id], [item.id for item in listed.requests]) + self.assertTrue(current.has_response) + self.assertEqual( + [(answer.slot_id, answer.value) for answer in current.answers], + [(request.slots[0].id, "available")], + ) alice = next(participant for participant in request.participants if participant.display_name == "Alice") bob = next(participant for participant in request.participants if participant.display_name == "Bob") self.assertEqual(alice.status, "responded") @@ -883,10 +900,12 @@ class SchedulingServiceTests(unittest.TestCase): SchedulingAvailabilityAnswerInput( slot_id=request.slots[0].id, value="available", + option_revision=scheduling_slot_revision(request.slots[0]), ), SchedulingAvailabilityAnswerInput( slot_id=request.slots[1].id, value="maybe", + option_revision=scheduling_slot_revision(request.slots[1]), ), ] ), @@ -917,6 +936,7 @@ class SchedulingServiceTests(unittest.TestCase): SchedulingAvailabilityAnswerInput( slot_id=request.slots[0].id, value="unavailable", + option_revision=scheduling_slot_revision(request.slots[0]), ) ] ), @@ -969,10 +989,12 @@ class SchedulingServiceTests(unittest.TestCase): SchedulingAvailabilityAnswerInput( slot_id=request.slots[0].id, value="unavailable", + option_revision=scheduling_slot_revision(request.slots[0]), ), SchedulingAvailabilityAnswerInput( slot_id=request.slots[1].id, value="maybe", + option_revision=scheduling_slot_revision(request.slots[1]), ), ] ),