From 7528d3543b06ce11afb5c5fbb755590805f81fa8 Mon Sep 17 00:00:00 2001 From: Sylvain Zimmer Date: Thu, 28 May 2026 17:56:25 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(caldav)=20use=20the=20OIDC=20email?= =?UTF-8?q?=20instead=20of=20the=20mailbox=20email=20(#679)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This allows the user to see the exact list of calendars seen on the other Calendars app --- src/backend/core/api/openapi.json | 15 + src/backend/core/api/viewsets/calendar.py | 80 +++-- .../core/services/calendar/ics_rebuild.py | 259 ++++++++++++++++ src/backend/core/services/calendar/service.py | 291 ++---------------- src/backend/core/services/calendar/tasks.py | 30 +- src/backend/core/tests/api/test_calendar.py | 90 ++++-- src/backend/messages/settings.py | 21 +- .../src/features/api/gen/calendar/calendar.ts | 46 ++- .../src/features/api/gen/models/index.ts | 1 + ...ailboxes_calendar_calendars_retrieve403.ts | 11 + 10 files changed, 505 insertions(+), 339 deletions(-) create mode 100644 src/backend/core/services/calendar/ics_rebuild.py create mode 100644 src/frontend/src/features/api/gen/models/mailboxes_calendar_calendars_retrieve403.ts diff --git a/src/backend/core/api/openapi.json b/src/backend/core/api/openapi.json index 854af885..80d9f4ad 100644 --- a/src/backend/core/api/openapi.json +++ b/src/backend/core/api/openapi.json @@ -2700,6 +2700,21 @@ }, "description": "" }, + "403": { + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "detail": { + "type": "string" + } + } + } + } + }, + "description": "Per-mailbox CalDAV channel denied access (upstream 403). Not returned for the deployment-default config, where a 403 is treated as an empty calendar list." + }, "502": { "content": { "application/json": { diff --git a/src/backend/core/api/viewsets/calendar.py b/src/backend/core/api/viewsets/calendar.py index b51f0e58..2369951e 100644 --- a/src/backend/core/api/viewsets/calendar.py +++ b/src/backend/core/api/viewsets/calendar.py @@ -56,14 +56,20 @@ class CalDAVChannelMixin: Priority: per-mailbox Channel (user-configured, pointing at any CalDAV provider) > deployment-default config (``CALDAV_DEFAULT_*`` - env vars). For the default path, the mailbox email is sent as the - Basic Auth username so the CalDAV server can route to the user's - calendars via principal discovery — see - ``CalDAVService.from_instance_config`` for the trust model. - Returns None if neither is configured. + env vars). For the default path, the *requesting user's OIDC + identity email* is sent as the Basic Auth username — not the + mailbox email — because the CalDAV server (e.g. suitenumerique + /calendars) keys principals on the OIDC ``email`` claim, and the + two can diverge (a Mailbox's ``local_part@domain.name`` is not + always the human's primary identity address). Using the mailbox + email here returns 403 Unknown User for those cases and silently + hides the calendar UI; using the OIDC email routes to the + principal the calendar provider provisioned on first login. + See ``CalDAVService.from_instance_config`` for the trust model. + Returns None if neither config path is available. """ return CalDAVService.from_channel_or_instance( - self.caldav_channel, str(self.mailbox) + self.caldav_channel, self.request.user.email ) def require_caldav_service(self): @@ -142,7 +148,7 @@ class CalendarRsvpView(CalDAVChannelMixin, APIView): try: task = calendar_rsvp_task.delay( channel_id=str(channel.id) if channel else None, - mailbox_email=mailbox_email, + user_email=request.user.email, ics_data=ics_data, response=response_type, attendee_email=mailbox_email, @@ -215,7 +221,7 @@ class CalendarAddEventView(CalDAVChannelMixin, APIView): try: task = calendar_add_event_task.delay( channel_id=str(channel.id) if channel else None, - mailbox_email=str(self.mailbox), + user_email=request.user.email, ics_data=ics_data, calendar_id=calendar_id, ) @@ -389,6 +395,14 @@ class CalendarListView(CalDAVChannelMixin, APIView): ), }, ), + 403: OpenApiResponse( + response=_ERROR_SCHEMA, + description=( + "Per-mailbox CalDAV channel denied access (upstream 403). " + "Not returned for the deployment-default config, where a " + "403 is treated as an empty calendar list." + ), + ), 502: OpenApiResponse( response=_ERROR_SCHEMA, description="CalDAV server error while listing calendars.", @@ -411,22 +425,50 @@ class CalendarListView(CalDAVChannelMixin, APIView): # calendars would fail at PUT time. calendars = service.list_calendars(writable_only=True) except CalDAVError as e: - # 403 from the CalDAV proxy means "we authenticated the - # service credential but the mailbox email is not a known - # user upstream" — effectively this mailbox has no calendar - # account. Surface it like an unconfigured integration so - # the UI hides the footer rather than showing a misleading - # "service unavailable" message. - if e.status_code == 403: + # 403 on the *instance-level* path means "we authenticated the + # service credential but this OIDC identity is not yet a + # principal upstream" — calendars (and similar providers) + # provision a principal on first login, so this is the + # "user has no calendars *yet*" state, not "integration + # disabled". Surface it as configured=True with an empty + # list so the UI shows the create-a-calendar CTA. + # + # This rationale only holds for the deployment-default config: + # a per-mailbox Channel uses the user's own credentials against + # a CalDAV provider of their choice, so a 403 there is a genuine + # ACL/auth failure that must surface — not be hidden behind a + # spurious empty list. + if e.status_code == 403 and self.caldav_channel is None: logger.info( - "CalDAV reports mailbox %s has no calendar account (HTTP 403).", - mailbox_id, + "CalDAV reports user %s has no calendar account yet (HTTP 403); " + "treating as empty calendar list.", + request.user.id, ) return Response( - {"calendars": [], "web_url": web_url, "configured": False}, + {"calendars": [], "web_url": web_url, "configured": True}, status=status.HTTP_200_OK, ) - logger.warning("CalDAV upstream failed during list_calendars: %s", e) + if e.status_code == 403: + # Log only the channel id + status — the CalDAVError message + # embeds the (user-supplied) upstream URL, which must not + # reach the logs. + logger.warning( + "CalDAV channel %s denied access (HTTP 403) during list_calendars.", + self.caldav_channel.id, + ) + return Response( + {"detail": "CalDAV server denied access while listing calendars."}, + status=status.HTTP_403_FORBIDDEN, + ) + # Same redaction: never log ``e`` — its message embeds the + # upstream URL (and, for network errors, the raw requests + # exception). Channel id + HTTP status are enough to triage. + logger.warning( + "CalDAV upstream failed during list_calendars " + "(channel=%s, HTTP status=%s).", + self.caldav_channel.id if self.caldav_channel else None, + e.status_code, + ) return Response( {"detail": "CalDAV server returned an error while listing calendars."}, status=status.HTTP_502_BAD_GATEWAY, diff --git a/src/backend/core/services/calendar/ics_rebuild.py b/src/backend/core/services/calendar/ics_rebuild.py new file mode 100644 index 00000000..abb2d76b --- /dev/null +++ b/src/backend/core/services/calendar/ics_rebuild.py @@ -0,0 +1,259 @@ +"""Storage-safe ICS rebuild. + +Inbound .ics is attacker-controlled (it arrives as an email attachment). +Rather than try to enumerate every dangerous extension to strip — METHOD, +VALARM with ACTION:EMAIL (Apple amplification), X-MS-OLK-*, X-ALT-DESC +HTML, ATTACH with data: URIs, future iCal extensions we haven't heard +of — we rebuild a fresh VCALENDAR from a tight allowlist of RFC 5545 +properties before PUTing to the CalDAV server. +""" + +import copy +import re +from datetime import datetime, timezone + +from icalendar import Calendar as ICalendar +from icalendar import Event as ICalEvent + +# VEVENT properties kept on the rebuilt event. Everything else is dropped: +# notable exclusions are ATTACH (size + scheme abuse), GEO, RESOURCES, +# RELATED-TO, REQUEST-STATUS, COMMENT, any X-* extension. +_VEVENT_KEEP = frozenset( + { + # Identity / iTIP versioning + "UID", + "DTSTAMP", + "SEQUENCE", + "CREATED", + "LAST-MODIFIED", + # Time / recurrence + "DTSTART", + "DTEND", + "DURATION", + "RRULE", + "RDATE", + "EXDATE", + "RECURRENCE-ID", + # Display + "SUMMARY", + "DESCRIPTION", + "LOCATION", + "URL", + "CATEGORIES", + # Semantics + "STATUS", + "TRANSP", + "CLASS", + "PRIORITY", + "ORGANIZER", + "ATTENDEE", + } +) + +# Per-property parameter allowlist. Anything else (X-*, unknown future +# params, attacker-crafted noise) is dropped. Properties not in this map +# get all parameters stripped. +_PARAM_KEEP = { + "DTSTART": frozenset({"TZID", "VALUE"}), + "DTEND": frozenset({"TZID", "VALUE"}), + "DURATION": frozenset(), + "RECURRENCE-ID": frozenset({"TZID", "VALUE", "RANGE"}), + "RDATE": frozenset({"TZID", "VALUE"}), + "EXDATE": frozenset({"TZID", "VALUE"}), + "ATTENDEE": frozenset( + { + "CN", + "PARTSTAT", + "ROLE", + "CUTYPE", + "RSVP", + "MEMBER", + "DELEGATED-TO", + "DELEGATED-FROM", + "SENT-BY", + "DIR", + "LANGUAGE", + "SCHEDULE-AGENT", + "SCHEDULE-STATUS", + } + ), + "ORGANIZER": frozenset( + { + "CN", + "DIR", + "SENT-BY", + "LANGUAGE", + "SCHEDULE-AGENT", + "SCHEDULE-STATUS", + } + ), + "SUMMARY": frozenset({"LANGUAGE"}), + "DESCRIPTION": frozenset({"LANGUAGE"}), + "LOCATION": frozenset({"LANGUAGE"}), + "CATEGORIES": frozenset({"LANGUAGE"}), +} + +# URL property must start with http:// or https://. We don't use +# ``urlparse`` here — browsers tolerate whitespace, control chars and +# weird Unicode that urlparse rejects, so a string we'd consider "safe" +# (because urlparse couldn't extract a dangerous scheme) might still +# resolve to ``javascript:`` in the browser. Be stricter than urlparse: +# the value must literally start with ``http://`` or ``https://`` +# (case-insensitive, no leading whitespace). +_SAFE_URL_RE = re.compile(r"^https?://", re.IGNORECASE) + +# RRULE frequencies that produce ruinous expansion if unbounded. +# Thunderbird hard-froze on a SECONDLY-frequency invite without +# COUNT/UNTIL (Mozilla bug 1770984). Reject these unless explicitly +# bounded. +_RRULE_FREQ_REQUIRES_BOUND = frozenset({"SECONDLY", "MINUTELY"}) + + +def rebuild_for_storage(cal): + """Return a fresh VCALENDAR containing only allowlisted properties. + + Default-deny posture: inbound .ics is attacker-controlled (it + arrives as an email attachment), so rather than try to enumerate + every dangerous extension to strip — METHOD, VALARM with + ACTION:EMAIL (Apple amplification), X-MS-OLK-*, X-ALT-DESC HTML, + ATTACH with data: URIs, future iCal extensions we haven't heard + of — we rebuild a fresh calendar from a small allowlist of + well-understood RFC 5545 properties. + + VTIMEZONE blocks referenced by a kept event's ``TZID`` parameter + are preserved so events authored in non-UTC zones still render + correctly. VTIMEZONEs not referenced by any kept event are + dropped. + + See _VEVENT_KEEP / _PARAM_KEEP for the exact allowlist. + """ + # Work on a deep copy throughout: ``_filter_params`` mutates the + # value objects' ``params`` dicts, and we add components by + # reference to ``fresh``. Without the copy the caller's parsed + # cal would be silently mutilated after this returns. + cal = copy.deepcopy(cal) + + fresh = ICalendar() + # Always stamp our own PRODID. Preserving the input's would echo + # attacker branding ("Created by Evil Corp") into the user's + # calendar and is informationally useless for storage. VERSION + # is fixed at 2.0 (RFC 5545); CALSCALE is preserved when present. + fresh.add("PRODID", "-//messages//CalDAV interop//EN") + fresh.add("VERSION", "2.0") + if cal.get("CALSCALE"): + fresh.add("CALSCALE", str(cal["CALSCALE"])) + + # Pass 1: rebuild every VEVENT and learn which TZIDs they + # actually reference. + rebuilt_events = [] + referenced_tzids = set() + for vevent in cal.walk("VEVENT"): + clean = _rebuild_event(vevent) + if clean is None: + continue + for prop in ("DTSTART", "DTEND", "RECURRENCE-ID", "RDATE", "EXDATE"): + val = clean.get(prop) + if val is None: + continue + for v in val if isinstance(val, list) else [val]: + tzid = getattr(v, "params", {}).get("TZID") + if tzid: + referenced_tzids.add(str(tzid)) + rebuilt_events.append(clean) + + # Pass 2: preserve only the VTIMEZONEs our kept events use. + for vtz in cal.walk("VTIMEZONE"): + if str(vtz.get("TZID") or "") in referenced_tzids: + fresh.add_component(vtz) + + for evt in rebuilt_events: + fresh.add_component(evt) + + return fresh + + +def _rebuild_event(src): + """Build a fresh VEVENT containing only allowlisted properties. + + Returns ``None`` if the source has no UID (malformed event per + RFC 5545 — skip rather than store). A missing DTSTAMP is + synthesized (see fallback chain below). + + DTSTAMP caveat: iTIP sequencing (RFC 5546 §3.2.6) compares + SEQUENCE first, then DTSTAMP, to decide whether an inbound + update supersedes what's already stored. The mainstream + generators (Google, Outlook, Apple, …) all emit DTSTAMP on + every iTIP message, so this fallback only fires for minimal / + hand-crafted invites. When it does fire and we synthesize + ``now``, a later legitimate UPDATE that carries an *older* + DTSTAMP (because the organizer authored it before our store + observed the original) can be misread as outdated and dropped + — the user appears to be on a stale copy of the event. The + LAST-MODIFIED → CREATED preference makes this less likely by + preserving the authoring order when those are present; ``now`` + is a true last-resort that we accept can cause sequence + inversion on follow-up updates. + """ + fresh = ICalEvent() + for key in list(src.keys()): + upper = key.upper() + if upper not in _VEVENT_KEEP: + continue + value = src[key] + for item in value if isinstance(value, list) else [value]: + cleaned = _clean_property_value(upper, item) + if cleaned is None: + continue + _filter_params(upper, cleaned) + # ``encode=0``: the value is already a typed icalendar + # object (vText / vDDDTypes / vCalAddress / vRecur); we + # don't want add() to re-encode and lose the params. + fresh.add(key, cleaned, encode=0) + + if "UID" not in fresh: + return None + if "DTSTAMP" not in fresh: + # Prefer iTIP versioning info already present on the event + # (LAST-MODIFIED → CREATED) over server-now. Setting DTSTAMP + # to "now" effectively claims this event was authored this + # second, which breaks iTIP sequencing if the organizer ever + # sends an update; the original timestamps preserve order. + fallback = fresh.get("LAST-MODIFIED") or fresh.get("CREATED") + if fallback is not None and hasattr(fallback, "dt"): + fresh.add("DTSTAMP", fallback.dt) + else: + fresh.add("DTSTAMP", datetime.now(tz=timezone.utc)) + return fresh + + +def _clean_property_value(prop, value): + """Per-property validation. Returns ``None`` to drop the value.""" + if prop == "URL": + if not _SAFE_URL_RE.match(str(value)): + return None + elif prop == "RRULE": + # icalendar represents RRULE as a vRecur (dict-like) where + # each entry is a list (``{"FREQ": ["SECONDLY"]}``). + try: + freq = (value.get("FREQ") or [""])[0] + except (AttributeError, TypeError): + return None + if str(freq).upper() in _RRULE_FREQ_REQUIRES_BOUND: + if not (value.get("COUNT") or value.get("UNTIL")): + return None + return value + + +def _filter_params(prop, value): + """Drop X-* / unknown params from a property value, in place. + + Properties not listed in ``_PARAM_KEEP`` get all their parameters + stripped — default-deny matches the rebuild posture. + """ + params = getattr(value, "params", None) + if not params: + return + allowed = _PARAM_KEEP.get(prop, frozenset()) + for k in list(params.keys()): + if k.upper() not in allowed: + del params[k] diff --git a/src/backend/core/services/calendar/service.py b/src/backend/core/services/calendar/service.py index f241e5ac..c985285c 100644 --- a/src/backend/core/services/calendar/service.py +++ b/src/backend/core/services/calendar/service.py @@ -6,7 +6,6 @@ events, add event, RSVP) directly over HTTP using ``requests`` and library's dependency surface. """ -import copy import logging import re import uuid @@ -19,8 +18,8 @@ import defusedxml.ElementTree as ET import requests from defusedxml.ElementTree import ParseError as DefusedParseError from icalendar import Calendar as ICalendar -from icalendar import Event as ICalEvent +from core.services.calendar.ics_rebuild import rebuild_for_storage from core.services.ssrf import ( SSRFProtectedAdapter, SSRFValidationError, @@ -41,113 +40,6 @@ APPLE_ICAL_NS = "http://apple.com/ns/ical/" _HEX_COLOR_RE = re.compile(r"^#(?:[0-9a-fA-F]{3}|[0-9a-fA-F]{6})$") -# --- Storage-safe ICS rebuild --------------------------------------------- -# -# Inbound .ics is attacker-controlled (it arrives as an email attachment). -# Rather than try to enumerate every dangerous extension to strip — METHOD, -# VALARM with ACTION:EMAIL (Apple amplification), X-MS-OLK-*, X-ALT-DESC -# HTML, ATTACH with data: URIs, future iCal extensions we haven't heard -# of — we rebuild a fresh VCALENDAR from a tight allowlist of RFC 5545 -# properties before PUTing to the CalDAV server. - -# VCALENDAR-level properties to preserve. ``METHOD`` is deliberately -# excluded: RFC 4791 §4.1 forbids it in stored calendar objects. -_VCALENDAR_KEEP = frozenset({"VERSION", "PRODID", "CALSCALE"}) - -# VEVENT properties kept on the rebuilt event. Everything else is dropped: -# notable exclusions are ATTACH (size + scheme abuse), GEO, RESOURCES, -# RELATED-TO, REQUEST-STATUS, COMMENT, any X-* extension. -_VEVENT_KEEP = frozenset( - { - # Identity / iTIP versioning - "UID", - "DTSTAMP", - "SEQUENCE", - "CREATED", - "LAST-MODIFIED", - # Time / recurrence - "DTSTART", - "DTEND", - "DURATION", - "RRULE", - "RDATE", - "EXDATE", - "RECURRENCE-ID", - # Display - "SUMMARY", - "DESCRIPTION", - "LOCATION", - "URL", - "CATEGORIES", - # Semantics - "STATUS", - "TRANSP", - "CLASS", - "PRIORITY", - "ORGANIZER", - "ATTENDEE", - } -) - -# Per-property parameter allowlist. Anything else (X-*, unknown future -# params, attacker-crafted noise) is dropped. Properties not in this map -# get all parameters stripped. -_PARAM_KEEP = { - "DTSTART": frozenset({"TZID", "VALUE"}), - "DTEND": frozenset({"TZID", "VALUE"}), - "DURATION": frozenset(), - "RECURRENCE-ID": frozenset({"TZID", "VALUE", "RANGE"}), - "RDATE": frozenset({"TZID", "VALUE"}), - "EXDATE": frozenset({"TZID", "VALUE"}), - "ATTENDEE": frozenset( - { - "CN", - "PARTSTAT", - "ROLE", - "CUTYPE", - "RSVP", - "MEMBER", - "DELEGATED-TO", - "DELEGATED-FROM", - "SENT-BY", - "DIR", - "LANGUAGE", - "SCHEDULE-AGENT", - "SCHEDULE-STATUS", - } - ), - "ORGANIZER": frozenset( - { - "CN", - "DIR", - "SENT-BY", - "LANGUAGE", - "SCHEDULE-AGENT", - "SCHEDULE-STATUS", - } - ), - "SUMMARY": frozenset({"LANGUAGE"}), - "DESCRIPTION": frozenset({"LANGUAGE"}), - "LOCATION": frozenset({"LANGUAGE"}), - "CATEGORIES": frozenset({"LANGUAGE"}), -} - -# URL property must start with http:// or https://. We don't use -# ``urlparse`` here — browsers tolerate whitespace, control chars and -# weird Unicode that urlparse rejects, so a string we'd consider "safe" -# (because urlparse couldn't extract a dangerous scheme) might still -# resolve to ``javascript:`` in the browser. Be stricter than urlparse: -# the value must literally start with ``http://`` or ``https://`` -# (case-insensitive, no leading whitespace). -_SAFE_URL_RE = re.compile(r"^https?://", re.IGNORECASE) - -# RRULE frequencies that produce ruinous expansion if unbounded. -# Thunderbird hard-froze on a SECONDLY-frequency invite without -# COUNT/UNTIL (Mozilla bug 1770984). Reject these unless explicitly -# bounded. -_RRULE_FREQ_REQUIRES_BOUND = frozenset({"SECONDLY", "MINUTELY"}) - - def _q(ns, tag): return f"{{{ns}}}{tag}" @@ -568,7 +460,7 @@ class CalDAVService: # pylint: disable=too-many-instance-attributes def add_event(self, ics_data, calendar_id=None): """Store an event on the selected calendar (or the default one).""" cal = ICalendar.from_ical(ics_data) - cal = self._rebuild_for_storage(cal) + cal = rebuild_for_storage(cal) # Suppress server-side iTIP REQUEST fan-out. sabre/dav's Schedule # plugin (used by suitenumerique/calendars) auto-dispatches one # iTIP REQUEST per ATTENDEE on any PUT where the calendar owner is @@ -613,7 +505,7 @@ class CalDAVService: # pylint: disable=too-many-instance-attributes "RSVP would not notify the organizer." ) - cal = self._rebuild_for_storage(cal) + cal = rebuild_for_storage(cal) self._put_event( self._pick_calendar_url(calendar_id), @@ -621,156 +513,6 @@ class CalDAVService: # pylint: disable=too-many-instance-attributes ) return True - @classmethod - def _rebuild_for_storage(cls, cal): - """Return a fresh VCALENDAR containing only allowlisted properties. - - Default-deny posture: inbound .ics is attacker-controlled (it - arrives as an email attachment), so rather than try to enumerate - every dangerous extension to strip — METHOD, VALARM with - ACTION:EMAIL (Apple amplification), X-MS-OLK-*, X-ALT-DESC HTML, - ATTACH with data: URIs, future iCal extensions we haven't heard - of — we rebuild a fresh calendar from a small allowlist of - well-understood RFC 5545 properties. - - VTIMEZONE blocks referenced by a kept event's ``TZID`` parameter - are preserved so events authored in non-UTC zones still render - correctly. VTIMEZONEs not referenced by any kept event are - dropped. - - See _VEVENT_KEEP / _PARAM_KEEP for the exact allowlist. - """ - # Work on a deep copy throughout: ``_filter_params`` mutates the - # value objects' ``params`` dicts, and we add components by - # reference to ``fresh``. Without the copy the caller's parsed - # cal would be silently mutilated after this returns. - cal = copy.deepcopy(cal) - - fresh = ICalendar() - # Always stamp our own PRODID. Preserving the input's would echo - # attacker branding ("Created by Evil Corp") into the user's - # calendar and is informationally useless for storage. VERSION - # is fixed at 2.0 (RFC 5545); CALSCALE is preserved when present. - fresh.add("PRODID", "-//messages//CalDAV interop//EN") - fresh.add("VERSION", "2.0") - if cal.get("CALSCALE"): - fresh.add("CALSCALE", str(cal["CALSCALE"])) - - # Pass 1: rebuild every VEVENT and learn which TZIDs they - # actually reference. - rebuilt_events = [] - referenced_tzids = set() - for vevent in cal.walk("VEVENT"): - clean = cls._rebuild_event(vevent) - if clean is None: - continue - for prop in ("DTSTART", "DTEND", "RECURRENCE-ID", "RDATE", "EXDATE"): - val = clean.get(prop) - if val is None: - continue - for v in val if isinstance(val, list) else [val]: - tzid = getattr(v, "params", {}).get("TZID") - if tzid: - referenced_tzids.add(str(tzid)) - rebuilt_events.append(clean) - - # Pass 2: preserve only the VTIMEZONEs our kept events use. - for vtz in cal.walk("VTIMEZONE"): - if str(vtz.get("TZID") or "") in referenced_tzids: - fresh.add_component(vtz) - - for evt in rebuilt_events: - fresh.add_component(evt) - - return fresh - - @classmethod - def _rebuild_event(cls, src): - """Build a fresh VEVENT containing only allowlisted properties. - - Returns ``None`` if the source has no UID (malformed event per - RFC 5545 — skip rather than store). A missing DTSTAMP is - synthesized (see fallback chain below). - - DTSTAMP caveat: iTIP sequencing (RFC 5546 §3.2.6) compares - SEQUENCE first, then DTSTAMP, to decide whether an inbound - update supersedes what's already stored. The mainstream - generators (Google, Outlook, Apple, …) all emit DTSTAMP on - every iTIP message, so this fallback only fires for minimal / - hand-crafted invites. When it does fire and we synthesize - ``now``, a later legitimate UPDATE that carries an *older* - DTSTAMP (because the organizer authored it before our store - observed the original) can be misread as outdated and dropped - — the user appears to be on a stale copy of the event. The - LAST-MODIFIED → CREATED preference makes this less likely by - preserving the authoring order when those are present; ``now`` - is a true last-resort that we accept can cause sequence - inversion on follow-up updates. - """ - fresh = ICalEvent() - for key in list(src.keys()): - upper = key.upper() - if upper not in _VEVENT_KEEP: - continue - value = src[key] - for item in value if isinstance(value, list) else [value]: - cleaned = cls._clean_property_value(upper, item) - if cleaned is None: - continue - cls._filter_params(upper, cleaned) - # ``encode=0``: the value is already a typed icalendar - # object (vText / vDDDTypes / vCalAddress / vRecur); we - # don't want add() to re-encode and lose the params. - fresh.add(key, cleaned, encode=0) - - if "UID" not in fresh: - return None - if "DTSTAMP" not in fresh: - # Prefer iTIP versioning info already present on the event - # (LAST-MODIFIED → CREATED) over server-now. Setting DTSTAMP - # to "now" effectively claims this event was authored this - # second, which breaks iTIP sequencing if the organizer ever - # sends an update; the original timestamps preserve order. - fallback = fresh.get("LAST-MODIFIED") or fresh.get("CREATED") - if fallback is not None and hasattr(fallback, "dt"): - fresh.add("DTSTAMP", fallback.dt) - else: - fresh.add("DTSTAMP", datetime.now(tz=timezone.utc)) - return fresh - - @staticmethod - def _clean_property_value(prop, value): - """Per-property validation. Returns ``None`` to drop the value.""" - if prop == "URL": - if not _SAFE_URL_RE.match(str(value)): - return None - elif prop == "RRULE": - # icalendar represents RRULE as a vRecur (dict-like) where - # each entry is a list (``{"FREQ": ["SECONDLY"]}``). - try: - freq = (value.get("FREQ") or [""])[0] - except (AttributeError, TypeError): - return None - if str(freq).upper() in _RRULE_FREQ_REQUIRES_BOUND: - if not (value.get("COUNT") or value.get("UNTIL")): - return None - return value - - @staticmethod - def _filter_params(prop, value): - """Drop X-* / unknown params from a property value, in place. - - Properties not listed in ``_PARAM_KEEP`` get all their parameters - stripped — default-deny matches the rebuild posture. - """ - params = getattr(value, "params", None) - if not params: - return - allowed = _PARAM_KEEP.get(prop, frozenset()) - for k in list(params.keys()): - if k.upper() not in allowed: - del params[k] - @staticmethod def _set_schedule_agent_client(cal): """Stamp ``SCHEDULE-AGENT=CLIENT`` on every ORGANIZER (RFC 6638 §7.1). @@ -948,19 +690,24 @@ class CalDAVService: # pylint: disable=too-many-instance-attributes can point a Channel at any CalDAV provider; see ``from_channel_or_instance``). - Authenticates with HTTP Basic Auth: ``username`` is the acting - mailbox email, passed per-request, so the CalDAV server can - resolve the user's calendars via principal discovery. The password - is the single ``CALDAV_DEFAULT_PASSWORD`` value — at the protocol - level it is just an HTTP Basic password, but the same value is - sent for every mailbox, so it effectively authenticates + Authenticates with HTTP Basic Auth: ``username`` is the requesting + user's *OIDC identity email* (``User.email``) — NOT the mailbox + address. The companion CalDAV provider (suitenumerique/calendars) + keys principals on the OIDC email claim, and provisions a + principal on first request, so the right addressing identity is + the human's OIDC email even when they are acting on a mailbox + whose ``local_part@domain.name`` differs. + + The password is the single ``CALDAV_DEFAULT_PASSWORD`` value — at + the protocol level it is just an HTTP Basic password, but the same + value is sent for every user, so it effectively authenticates messages-as-a-service rather than any individual user. Trust model: see the comment block on ``CALDAV_DEFAULT_PASSWORD`` in ``messages/settings.py``. In short: the CalDAV server trusts whichever email messages claims to act as, so the load-bearing - safety property is that mailbox creation cannot mint a Mailbox - whose email belongs to another user on the CalDAV side. + safety property is that the OIDC identity provider does not let + one human assert another human's email claim. """ url = django_settings.CALDAV_DEFAULT_URL password = django_settings.CALDAV_DEFAULT_PASSWORD @@ -981,8 +728,10 @@ class CalDAVService: # pylint: disable=too-many-instance-attributes deployment-wide fallback — see ``from_instance_config`` for its trust model. - ``username`` is the acting mailbox email; it is only used by the - default path (per-channel credentials are self-contained). + ``username`` is the requesting user's OIDC identity email; it is + only used by the default path (per-channel credentials are + self-contained). See ``from_instance_config`` for why it must + be the OIDC email rather than the mailbox address. Returns None if neither is available. """ # TODO(caldav-per-channel): no DRF write path exists for CalDAV diff --git a/src/backend/core/services/calendar/tasks.py b/src/backend/core/services/calendar/tasks.py index 9d44510d..dbb728ae 100644 --- a/src/backend/core/services/calendar/tasks.py +++ b/src/backend/core/services/calendar/tasks.py @@ -21,25 +21,27 @@ _RSVP_FAILURE = "Failed to send the RSVP." _ADD_FAILURE = "Failed to add the event to the calendar." -def _get_caldav_service(channel_id: str | None, mailbox_email: str): +def _get_caldav_service(channel_id: str | None, user_email: str): """Build a CalDAVService from a channel ID or instance-level config. - ``mailbox_email`` is the acting mailbox's email. It is the Basic Auth - username for the instance-level path; per-channel auth is + ``user_email`` is the requesting user's OIDC identity email — NOT + the acting mailbox's email. It is the Basic Auth username for the + instance-level path because the calendars CalDAV provider keys + principals on the OIDC ``email`` claim. Per-channel auth is self-contained so the email is ignored there. """ if channel_id: channel = Channel.objects.get(id=channel_id, type=ChannelTypes.CALDAV) return CalDAVService.from_channel(channel) - return CalDAVService.from_instance_config(mailbox_email) + return CalDAVService.from_instance_config(user_email) @celery_app.task(bind=True) def calendar_rsvp_task( self, # pylint: disable=unused-argument channel_id: str | None, - mailbox_email: str, + user_email: str, ics_data: str, response: str, attendee_email: str, @@ -50,14 +52,18 @@ def calendar_rsvp_task( Args: channel_id: UUID of the CalDAV channel, or None for instance config - mailbox_email: Acting mailbox email (Basic Auth user for instance config) + user_email: Requesting user's OIDC identity email (Basic Auth user + for instance config — addresses the user's principal on the + CalDAV server, which keys on the OIDC email claim). ics_data: Raw ICS content response: ACCEPTED, DECLINED, or TENTATIVE - attendee_email: Email of the responding attendee + attendee_email: Email of the responding attendee in the .ics + ATTENDEE list (the mailbox address the invitation was sent + to — this is what iTIP matches on, NOT the user's OIDC email). calendar_id: Optional specific calendar URL to use """ try: - service = _get_caldav_service(channel_id, mailbox_email) + service = _get_caldav_service(channel_id, user_email) except Channel.DoesNotExist as e: # Race: the row existed when the viewset enqueued the task, gone # by the time the worker ran. Worth a Sentry breadcrumb so we @@ -123,7 +129,7 @@ def calendar_rsvp_task( def calendar_add_event_task( self, # pylint: disable=unused-argument channel_id: str | None, - mailbox_email: str, + user_email: str, ics_data: str, calendar_id: str | None = None, ) -> Dict[str, Any]: @@ -132,12 +138,14 @@ def calendar_add_event_task( Args: channel_id: UUID of the CalDAV channel, or None for instance config - mailbox_email: Acting mailbox email (Basic Auth user for instance config) + user_email: Requesting user's OIDC identity email (Basic Auth user + for instance config). The event is stored on a calendar owned + by this user's CalDAV principal, not on the mailbox's. ics_data: Raw ICS content calendar_id: Optional specific calendar URL to use """ try: - service = _get_caldav_service(channel_id, mailbox_email) + service = _get_caldav_service(channel_id, user_email) except Channel.DoesNotExist as e: # Race: the row existed when the viewset enqueued the task, gone # by the time the worker ran. Worth a Sentry breadcrumb so we diff --git a/src/backend/core/tests/api/test_calendar.py b/src/backend/core/tests/api/test_calendar.py index b4194201..f6648503 100644 --- a/src/backend/core/tests/api/test_calendar.py +++ b/src/backend/core/tests/api/test_calendar.py @@ -17,6 +17,7 @@ from icalendar import Calendar as ICalendar from core import factories from core.enums import ChannelTypes, MailboxRoleChoices +from core.services.calendar.ics_rebuild import rebuild_for_storage from core.services.calendar.service import CalDAVError, CalDAVService @@ -353,6 +354,53 @@ class TestCalendarListView: names = [c["name"] for c in resp.json()["calendars"]] assert names == ["Writable"] + def test_list_calendars_403_instance_config_is_empty_list( + self, + api_client, + mailbox, + user_with_mailbox, + instance_caldav_config, + ): + """On the instance-level path, a 403 means the OIDC identity has no + principal upstream yet (provisioned on first login) — surface it as + configured=True with an empty list, not an error.""" + api_client.force_authenticate(user=user_with_mailbox) + with mock.patch.object( + CalDAVService, + "list_calendars", + side_effect=CalDAVError("Forbidden", status_code=403), + ): + resp = api_client.get( + f"/api/v1.0/mailboxes/{mailbox.id}/calendar/calendars/" + ) + + assert resp.status_code == 200 + body = resp.json() + assert body["calendars"] == [] + assert body["configured"] is True + + def test_list_calendars_403_per_channel_surfaces_error( + self, + api_client, + mailbox, + caldav_channel, + user_with_mailbox, + ): + """On a per-mailbox channel the user supplies their own credentials, + so a 403 is a genuine ACL/auth failure and must surface — not be + masked as an empty calendar list.""" + api_client.force_authenticate(user=user_with_mailbox) + with mock.patch.object( + CalDAVService, + "list_calendars", + side_effect=CalDAVError("Forbidden", status_code=403), + ): + resp = api_client.get( + f"/api/v1.0/mailboxes/{mailbox.id}/calendar/calendars/" + ) + + assert resp.status_code == 403 + # --------------------------------------------------------------------------- # Conflicts @@ -1001,24 +1049,27 @@ def instance_caldav_config(radicale_server, settings): """Configure instance-level CalDAV settings pointing at Radicale. CALDAV_DEFAULT_URL is the CalDAV server root — the service resolves the - per-user calendar-home-set via principal discovery, using the mailbox - email as the Basic Auth username. + per-user calendar-home-set via principal discovery, using the requesting + user's OIDC identity email as the Basic Auth username (see + ``CalDAVService.from_instance_config``). """ settings.CALDAV_DEFAULT_URL = f"{radicale_server}/" settings.CALDAV_DEFAULT_PASSWORD = INSTANCE_CALDAV_PASSWORD @pytest.fixture() -def instance_calendar(radicale_server, mailbox): - """Create a calendar under the mailbox's principal on Radicale. +def instance_calendar(radicale_server, user_with_mailbox): + """Create a calendar under the requesting user's principal on Radicale. Radicale with ``auth.type=none`` treats the Basic Auth username as the - principal name and serves calendars under ``/{username}/``. Since the - service now uses the mailbox email as the Basic Auth username, the - calendar must live under that path. + principal name and serves calendars under ``/{username}/``. The + instance-level service authenticates as the requesting user's OIDC + identity email (``CalDAVService.from_instance_config``), so the calendar + must live under that user's path — not the mailbox's. """ - cal_url = f"{radicale_server}/{mailbox}/instance-cal/" - _mkcalendar(cal_url, "Instance Calendar", auth=(str(mailbox), "ignored")) + user_email = user_with_mailbox.email + cal_url = f"{radicale_server}/{user_email}/instance-cal/" + _mkcalendar(cal_url, "Instance Calendar", auth=(str(user_email), "ignored")) return radicale_server @@ -1313,7 +1364,7 @@ class TestRebuildForStorage: ) def _rebuild(self, ics): - return CalDAVService._rebuild_for_storage(ICalendar.from_ical(ics)) + return rebuild_for_storage(ICalendar.from_ical(ics)) def test_drops_method(self): out = self._rebuild(self._hostile_ics()) @@ -1461,7 +1512,7 @@ class TestRebuildForStorage: ics = self._hostile_ics() cal = ICalendar.from_ical(ics) before = cal.to_ical() - _ = CalDAVService._rebuild_for_storage(cal) + _ = rebuild_for_storage(cal) # Input must be byte-identical after rebuild. assert cal.to_ical() == before @@ -1957,19 +2008,22 @@ class TestListCalendarsWritableFilter: # --------------------------------------------------------------------------- -# Credential contract: Basic Auth user = mailbox email, password = setting +# Credential contract: Basic Auth user = OIDC identity email, password = setting # --------------------------------------------------------------------------- @pytest.mark.django_db() -def test_instance_config_sends_mailbox_email_as_basic_auth_user(settings, mailbox): - """Instance-level auth: Basic Auth user must be the mailbox email and - the password must be CALDAV_DEFAULT_PASSWORD verbatim.""" +def test_instance_config_sends_oidc_email_as_basic_auth_user(settings): + """Instance-level auth: Basic Auth user must be the requesting user's + OIDC identity email (NOT the mailbox address — the CalDAV provider + keys principals on the OIDC ``email`` claim) and the password must be + CALDAV_DEFAULT_PASSWORD verbatim.""" settings.CALDAV_DEFAULT_URL = "https://caldav.example.com/" settings.CALDAV_DEFAULT_PASSWORD = "shared-secret-xyz" - service = CalDAVService.from_instance_config(str(mailbox)) + oidc_email = "alice@identity.example" + service = CalDAVService.from_instance_config(oidc_email) - assert service.username == str(mailbox) + assert service.username == oidc_email assert service.password == "shared-secret-xyz" - assert service.session.auth == (str(mailbox), "shared-secret-xyz") + assert service.session.auth == (oidc_email, "shared-secret-xyz") diff --git a/src/backend/messages/settings.py b/src/backend/messages/settings.py index f1966d75..039a60a0 100644 --- a/src/backend/messages/settings.py +++ b/src/backend/messages/settings.py @@ -375,17 +375,20 @@ class Base(Configuration): # ``CALDAV_DEFAULT_URL`` is the CalDAV server root. # ``CALDAV_DEFAULT_PASSWORD`` is a single secret sent as the HTTP Basic # Auth *password* on every outbound request. The Basic Auth *username* - # is the acting mailbox's email, computed per request from the URL. + # is the requesting user's OIDC identity email (``User.email``), which + # is what providers like suitenumerique/calendars key principals on — + # NOT the mailbox's ``local_part@domain.name`` (which can diverge from + # the user's OIDC email). # # That means the same secret authenticates messages-as-a-service for - # all mailboxes — the CalDAV server is then responsible for whatever - # per-user authorization it wants to layer on top. Concretely: any user - # who can choose what email lands on a Mailbox row can cause messages - # to authenticate to the CalDAV server *as* that email. The load-bearing - # safety property is therefore that mailbox creation does not let one - # user mint a Mailbox whose email matches another user on the CalDAV - # side. Operators wiring up this integration must verify that property - # against their domain/identity ownership rules. + # all users — the CalDAV server is then responsible for whatever + # per-user authorization it wants to layer on top. Concretely: any + # user authenticated via OIDC can cause messages to authenticate to + # the CalDAV server as their OIDC email. The load-bearing safety + # property is therefore that the OIDC identity provider does not let + # one human assert another human's ``email`` claim. Operators wiring + # up this integration must verify that property holds for their IdP + # configuration (e.g. that ``email`` is a verified claim). CALDAV_DEFAULT_URL = values.Value( None, environ_name="CALDAV_DEFAULT_URL", environ_prefix=None ) diff --git a/src/frontend/src/features/api/gen/calendar/calendar.ts b/src/frontend/src/features/api/gen/calendar/calendar.ts index 2fe11a35..bef16a82 100644 --- a/src/frontend/src/features/api/gen/calendar/calendar.ts +++ b/src/frontend/src/features/api/gen/calendar/calendar.ts @@ -31,6 +31,7 @@ import type { CalendarRsvpResponse, MailboxesCalendarAddCreate400, MailboxesCalendarAddCreate503, + MailboxesCalendarCalendarsRetrieve403, MailboxesCalendarCalendarsRetrieve502, MailboxesCalendarConflictsCreate400, MailboxesCalendarConflictsCreate502, @@ -179,6 +180,11 @@ export type mailboxesCalendarCalendarsRetrieveResponse200 = { status: 200; }; +export type mailboxesCalendarCalendarsRetrieveResponse403 = { + data: MailboxesCalendarCalendarsRetrieve403; + status: 403; +}; + export type mailboxesCalendarCalendarsRetrieveResponse502 = { data: MailboxesCalendarCalendarsRetrieve502; status: 502; @@ -188,10 +194,12 @@ export type mailboxesCalendarCalendarsRetrieveResponseSuccess = mailboxesCalendarCalendarsRetrieveResponse200 & { headers: Headers; }; -export type mailboxesCalendarCalendarsRetrieveResponseError = - mailboxesCalendarCalendarsRetrieveResponse502 & { - headers: Headers; - }; +export type mailboxesCalendarCalendarsRetrieveResponseError = ( + | mailboxesCalendarCalendarsRetrieveResponse403 + | mailboxesCalendarCalendarsRetrieveResponse502 +) & { + headers: Headers; +}; export type mailboxesCalendarCalendarsRetrieveResponse = | mailboxesCalendarCalendarsRetrieveResponseSuccess @@ -222,7 +230,10 @@ export const getMailboxesCalendarCalendarsRetrieveQueryKey = ( export const getMailboxesCalendarCalendarsRetrieveQueryOptions = < TData = Awaited>, - TError = ErrorType, + TError = ErrorType< + | MailboxesCalendarCalendarsRetrieve403 + | MailboxesCalendarCalendarsRetrieve502 + >, >( mailboxId: string, options?: { @@ -265,12 +276,16 @@ export const getMailboxesCalendarCalendarsRetrieveQueryOptions = < export type MailboxesCalendarCalendarsRetrieveQueryResult = NonNullable< Awaited> >; -export type MailboxesCalendarCalendarsRetrieveQueryError = - ErrorType; +export type MailboxesCalendarCalendarsRetrieveQueryError = ErrorType< + MailboxesCalendarCalendarsRetrieve403 | MailboxesCalendarCalendarsRetrieve502 +>; export function useMailboxesCalendarCalendarsRetrieve< TData = Awaited>, - TError = ErrorType, + TError = ErrorType< + | MailboxesCalendarCalendarsRetrieve403 + | MailboxesCalendarCalendarsRetrieve502 + >, >( mailboxId: string, options: { @@ -297,7 +312,10 @@ export function useMailboxesCalendarCalendarsRetrieve< }; export function useMailboxesCalendarCalendarsRetrieve< TData = Awaited>, - TError = ErrorType, + TError = ErrorType< + | MailboxesCalendarCalendarsRetrieve403 + | MailboxesCalendarCalendarsRetrieve502 + >, >( mailboxId: string, options?: { @@ -324,7 +342,10 @@ export function useMailboxesCalendarCalendarsRetrieve< }; export function useMailboxesCalendarCalendarsRetrieve< TData = Awaited>, - TError = ErrorType, + TError = ErrorType< + | MailboxesCalendarCalendarsRetrieve403 + | MailboxesCalendarCalendarsRetrieve502 + >, >( mailboxId: string, options?: { @@ -344,7 +365,10 @@ export function useMailboxesCalendarCalendarsRetrieve< export function useMailboxesCalendarCalendarsRetrieve< TData = Awaited>, - TError = ErrorType, + TError = ErrorType< + | MailboxesCalendarCalendarsRetrieve403 + | MailboxesCalendarCalendarsRetrieve502 + >, >( mailboxId: string, options?: { diff --git a/src/frontend/src/features/api/gen/models/index.ts b/src/frontend/src/features/api/gen/models/index.ts index a8a7b4e0..59c5fc6e 100644 --- a/src/frontend/src/features/api/gen/models/index.ts +++ b/src/frontend/src/features/api/gen/models/index.ts @@ -85,6 +85,7 @@ export * from "./mailbox_role_choices"; export * from "./mailboxes_accesses_list_params"; export * from "./mailboxes_calendar_add_create400"; export * from "./mailboxes_calendar_add_create503"; +export * from "./mailboxes_calendar_calendars_retrieve403"; export * from "./mailboxes_calendar_calendars_retrieve502"; export * from "./mailboxes_calendar_conflicts_create400"; export * from "./mailboxes_calendar_conflicts_create502"; diff --git a/src/frontend/src/features/api/gen/models/mailboxes_calendar_calendars_retrieve403.ts b/src/frontend/src/features/api/gen/models/mailboxes_calendar_calendars_retrieve403.ts new file mode 100644 index 00000000..eb1dbb40 --- /dev/null +++ b/src/frontend/src/features/api/gen/models/mailboxes_calendar_calendars_retrieve403.ts @@ -0,0 +1,11 @@ +/** + * Generated by orval 🍺 + * Do not edit manually. + * messages API + * This is the messages API schema. + * OpenAPI spec version: 1.0.0 (v1.0) + */ + +export type MailboxesCalendarCalendarsRetrieve403 = { + detail?: string; +};