From 9d7f2fa3e05f520eae9e612bc6edfea1fb279669 Mon Sep 17 00:00:00 2001 From: Toni Barth Date: Sat, 26 Sep 2026 13:14:18 +0200 Subject: [PATCH 1/3] Say a refused update is a refusal, in every adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since #97 a split creates its new series first and takes it back only when the truncate certainly wrote nothing (`writeNeverLanded`: a refusal token or a refusing code). Several certain refusals arrived as `protocol`: EWS SOAP error answers to the one `UpdateItem`, Google and Graph HTTP 400 and 429, and CalDAV's own checks before the PUT. Both series then stayed with the warning that the series may show twice, although it certainly did (decision 147). - cal-core: `WriteRefusal::refused_status` names the statuses with which a server turned a write down whole (400, 413, 415, 422, 429, 507), and the new `UnsafeToWrite` refusal says Aperio did not write because it could not do so safely. `parse` now knows every refusal (`occurrence-not-writable` was missing). - Each adapter's `update_event` maps such an answer to `Forbidden("server-refused: …")`; EWS also every SOAP error answer to the update except internal and timeout errors; CalDAV's own checks say `unsafe-to-write` with a log token. `forbidden` reaches JS on the phone. - Graph: a DELETE that finds nothing after a successful `/cancel` is done: the cancel moved the event to Deleted Items, which gives it a new id. - Frontend: `unsafe-to-write` has its sentence and reason in EN and DE. - DESIGN.md and TODO.md follow. Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 9 +- TODO.md | 34 +++-- crates/adapter-caldav/src/events.rs | 69 +++++++++- crates/adapter-caldav/src/lib.rs | 52 +++++++- crates/adapter-ews/src/lib.rs | 110 +++++++++++++++- crates/adapter-google/src/lib.rs | 104 ++++++++++++++- crates/adapter-microsoft-graph/src/api.rs | 12 +- crates/adapter-microsoft-graph/src/lib.rs | 152 +++++++++++++++++++++- crates/cal-core/bindings/WriteRefusal.ts | 2 +- crates/cal-core/src/write_refusal.rs | 59 +++++++++ locales/de/translation.json | 2 + locales/en/translation.json | 2 + shared/eventWriteError.ts | 2 + shared/generated/WriteRefusal.ts | 2 +- src/state/eventWriteError.test.ts | 21 +++ 15 files changed, 595 insertions(+), 37 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index f559b889f..fd7995d7e 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -1404,9 +1404,12 @@ Besprechung sagte sie dem Gast ab. Jetzt: folgenden“ wird nicht angeboten: Das schriebe die Regel um, die nur der Organisator ändern darf. - Eine Ablehnung reist als Marke (`cal_core::WriteRefusal`: - `reply-only-invitation:`, `server-refused:`, `identity-unknown:`), und - beide Oberflächen sagen daraus einen Satz in der Sprache des Nutzers - (`shared/eventWriteError.ts`). `CACHE_GENERATION` 4, damit eine + `reply-only-invitation:`, `server-refused:`, `identity-unknown:`, + `occurrence-not-writable:`, `unsafe-to-write:`), und beide Oberflächen + sagen daraus einen Satz in der Sprache des Nutzers + (`shared/eventWriteError.ts`). Eine Marke heißt auch: Es wurde sicher + nichts geschrieben (`writeNeverLanded`); beim Ändern eines Termins melden + alle Adapter eine Ablehnung des Servers so (147). `CACHE_GENERATION` 4, damit eine gespeicherte Kalenderliste ohne das neue Merkmal neu gelesen wird. **Free/Busy-Abfrage (implementiert).** Im Termin-Dialog prüft „Verfügbarkeit diff --git a/TODO.md b/TODO.md index 53385b1bd..4edc195cb 100644 --- a/TODO.md +++ b/TODO.md @@ -2409,22 +2409,32 @@ Siehe DESIGN §4.2. „weg oder nie hier“, die Suche über die Kalender geht weiter und zählt es nur als gelöscht, wenn kein anderer Kalender den Termin hat (`DeleteWalk`). Nach einem gescheiterten Verbindungsaufbau gilt nichts - davon: da ging nichts hinaus. + davon: da ging nichts hinaus. Am Handy kommen nur `forbidden`, + `conflict` und `network` mit Code an (B9); eine Ablehnung mit Marke + (`server-refused`, `unsafe-to-write`) reist aber als `forbidden` und + wird erkannt, jede andere gilt dort vorsichtig als unklar. Handy ohne + Testläufer — ↻ im Test. - 🚩 CalDAV `create_task_list`: MKCALENDAR läuft über `send_retrying`. Kam der erste Versuch an und brach dann die Verbindung ab, antwortet die Wiederholung 405 (die Liste gibt es schon): gemeldet als Fehler, ein erneuter Versuch legt eine zweite Liste an. Vor #97 schon so. - - 🚩 **Sichere Ablehnungen als Ablehnung kennzeichnen** (147, eigener PR - direkt nach #97): EWS-Fehlerantworten, HTTP 400/429 bei Google und - Microsoft, CalDAV-Prüfungen „nothing was saved“ kommen als `protocol` an; - beim Teilen bleiben dann beide Serien mit Warnung stehen, obwohl sicher - nichts gekürzt wurde. Die Adapter sollen sie mit `server-refused` - kennzeichnen. Dazu Graph: nach erfolgreichem `/cancel` liefert das - folgende DELETE womöglich 404 (die Absage verschiebt das Ereignis), das - Löschen gilt dann als gescheitert. Am Handy kommen - nur `forbidden`, `conflict` und `network` mit Code an (B9), jede andere - Ablehnung gilt dort also vorsichtig als unklar. Handy ohne Testläufer — - ↻ im Test. + - ✅ **Sichere Ablehnungen als Ablehnung gekennzeichnet** (147). EWS- + Fehlerantworten, HTTP 400/429 bei Google und Microsoft und CalDAVs eigene + Prüfungen „nothing was saved“ kamen als `protocol` an; beim Teilen + blieben dann beide Serien mit Warnung stehen, obwohl sicher nichts + gekürzt wurde. Jetzt meldet jeder Adapter beim Ändern eines Termins + (`update_event`) eine Ablehnung als solche: ein HTTP-Status, mit dem der + Server das Schreiben ganz abgelehnt hat + (`WriteRefusal::refused_status`: 400, 413, 415, 422, 429, 507), und bei + EWS jede SOAP-Fehlerantwort auf das eine `UpdateItem` außer internen + Server- und Zeitüberschreitungsfehlern, als `server-refused`; CalDAVs + eigene Prüfungen als neue Marke `unsafe-to-write` („Aperio kann diesen + Termin nicht sicher ändern“). Beide reisen als `forbidden` und kommen so + auch am Handy an. Graph: ein DELETE, das nach erfolgreichem `/cancel` + nichts mehr findet, gilt als erledigt (die Absage verschiebt das + Ereignis nach „Gelöschte Elemente“, mit neuer Id). Nicht hier: Anlegen + und Löschen melden solche Ablehnungen weiter als `protocol` (für das + Teilen zählt nur das Kürzen). - 🚩 **Weckerberechnung bei einem Ende vor dem Beginn** (124: jetzt nicht). `expand_on` nimmt bei einer Regel, die vor ihrem Beginn endet, weiter den Serienbeginn. Aperio schreibt solche Regeln nicht mehr, aber ein Enddatum, diff --git a/crates/adapter-caldav/src/events.rs b/crates/adapter-caldav/src/events.rs index c9f9dc3de..4e8ba4c5b 100644 --- a/crates/adapter-caldav/src/events.rs +++ b/crates/adapter-caldav/src/events.rs @@ -331,20 +331,32 @@ async fn update_master( .map_err(|e| CaldavError::Config(format!("event.calendar_id is not a URL: {e}")))?; let resource = resource_url_for_event(&cal_url, &event.id)?; let (body, server_etag) = get_resource(client, &resource, credentials).await?; - let refused = - |why: &str| CaldavError::Protocol(format!("{}: {why}; nothing was saved", event.id)); + // Nothing is written, and the caller must know it for certain: splitting a + // series takes its new part back only after a refusal like this one + // (decision 144), so it travels as one, not as a protocol error. + let refused = |token: &str, why: &str| { + tracing::warn!(event_id = %event.id, why, "refusing to write the event; nothing was saved"); + CaldavError::Forbidden(WriteRefusal::UnsafeToWrite.message(token)) + }; let own = ctx.identity.clone().unwrap_or_default(); - let blocks = vevent_blocks(&body, cal_url.as_str(), &own) - .ok_or_else(|| refused("its resource cannot be read block by block"))?; + let blocks = vevent_blocks(&body, cal_url.as_str(), &own).ok_or_else(|| { + refused( + "unreadable-blocks", + "its resource cannot be read block by block", + ) + })?; let (_, uid) = decode_event_id(&event.id); let master = blocks .iter() .find(|b| { override_recurrence_id(&b.event.id).is_none() && decode_event_id(&b.event.id).1 == uid }) - .ok_or_else(|| refused("its resource holds no such event"))?; + .ok_or_else(|| refused("no-such-event", "its resource holds no such event"))?; if !one_organizer(&body, &blocks) { - return Err(refused("its components name different organizers")); + return Err(refused( + "mixed-organizers", + "its components name different organizers", + )); } if ctx.schedules && attendee_copy(&body[master.range.clone()], ctx)? { return write_attendee_copy( @@ -381,7 +393,12 @@ async fn update_master( |rid| cutoff.is_none_or(|until| rid <= until), |block| apply_to_block(block, &plan.change), ) - .ok_or_else(|| refused("its resource cannot be read block by block"))?; + .ok_or_else(|| { + refused( + "unreadable-blocks", + "its resource cannot be read block by block", + ) + })?; let if_match = event.etag.as_deref().or(server_etag.as_deref()); let new_etag = put_resource(client, &resource, new_body, if_match, credentials).await?; @@ -2206,6 +2223,44 @@ END:VCALENDAR\r } } + /// A resource whose blocks name different organizers is not written: a + /// refusal of Aperio's own, said as one (decision 144), not a protocol + /// error the caller has to treat as maybe written. + #[tokio::test] + async fn an_update_aperio_will_not_write_is_a_refusal() { + let mut server = Server::new_async().await; + let body = "BEGIN:VCALENDAR\r\nVERSION:2.0\r\n\ +BEGIN:VEVENT\r\nUID:abc-123@aperio\r\nDTSTAMP:20260520T060000Z\r\nSUMMARY:Standup\r\n\ +DTSTART:20260520T080000Z\r\nDTEND:20260520T083000Z\r\nRRULE:FREQ=DAILY\r\n\ +ORGANIZER:mailto:alice@example.org\r\nEND:VEVENT\r\n\ +BEGIN:VEVENT\r\nUID:abc-123@aperio\r\nDTSTAMP:20260520T060000Z\r\nSUMMARY:Standup\r\n\ +RECURRENCE-ID:20260521T080000Z\r\nDTSTART:20260521T090000Z\r\nDTEND:20260521T093000Z\r\n\ +ORGANIZER:mailto:bob@example.org\r\nEND:VEVENT\r\nEND:VCALENDAR\r\n"; + let _get = serve_copy(&mut server, body.to_string()).await; + let put = server + .mock( + "PUT", + mockito::Matcher::Regex(r"^/calendars/alice/work/.+\.ics$".into()), + ) + .expect(0) + .create_async() + .await; + let cal_url = Url::parse(&format!("{}/calendars/alice/work/", server.url())).unwrap(); + let err = update_event( + &client(), + sample_existing_event(&cal_url), + &creds(&server.url()), + &WriteCtx::default(), + ) + .await + .unwrap_err(); + match err { + CaldavError::Forbidden(msg) => assert_eq!(msg, "unsafe-to-write: mixed-organizers"), + other => panic!("expected a refusal, got {other:?}"), + } + put.assert_async().await; + } + #[tokio::test] async fn update_event_412_surfaces_as_conflict() { let mut server = Server::new_async().await; diff --git a/crates/adapter-caldav/src/lib.rs b/crates/adapter-caldav/src/lib.rs index c443c9f27..8c1e4a90a 100644 --- a/crates/adapter-caldav/src/lib.rs +++ b/crates/adapter-caldav/src/lib.rs @@ -928,6 +928,23 @@ impl CaldavAdapter { /// Translate a CalDAV-specific error into the shared `cal_core::Error` /// shape so the rest of the app can pattern-match it uniformly. +/// [`to_core_error`] for an update. A status with which the server turned the +/// write down whole ([`cal_core::WriteRefusal::refused_status`]) is a refusal, +/// not a protocol error: a caller deciding whether the write may have landed — +/// splitting a series undoes its new part only when the cut certainly did not +/// (decision 144) — must be told that nothing was written. +fn to_update_error(err: CaldavError) -> CoreError { + match err { + CaldavError::Http { status, message } if cal_core::WriteRefusal::refused_status(status) => { + tracing::warn!(status, %message, "the server refused the update"); + CoreError::Forbidden( + cal_core::WriteRefusal::ServerRefused.message(&format!("HTTP {status}")), + ) + } + other => to_core_error(other), + } +} + fn to_core_error(err: CaldavError) -> CoreError { match err { CaldavError::Network(msg) => CoreError::Network(msg), @@ -1091,7 +1108,7 @@ impl CalendarFeature for CaldavAdapter { let ctx = self.write_ctx().await?; events::update_event(&self.http, event, &self.credentials, &ctx) .await - .map_err(to_core_error) + .map_err(to_update_error) } async fn add_event_exdate( @@ -1693,6 +1710,39 @@ fn contact_matches(c: &Contact, needle_lower: &str) -> bool { .any(|e| e.value.to_lowercase().contains(needle_lower)) } +#[cfg(test)] +mod update_refusal_tests { + use super::*; + + /// A status with which the server turned the update down whole is a + /// refusal (decision 144); a server failure and the named statuses keep + /// their error. + #[test] + fn an_update_the_server_turned_down_is_a_refusal() { + for status in [400, 413, 415, 422, 429, 507] { + match to_update_error(CaldavError::Http { + status, + message: "no".into(), + }) { + CoreError::Forbidden(msg) => { + assert_eq!(msg, format!("server-refused: HTTP {status}")) + } + other => panic!("{status}: {other:?}"), + } + } + let http = |status| CaldavError::Http { + status, + message: "no".into(), + }; + assert!(matches!(to_update_error(http(500)), CoreError::Protocol(_))); + assert!(matches!(to_update_error(http(412)), CoreError::Conflict(_))); + assert!(matches!( + to_update_error(CaldavError::Network("reset".into())), + CoreError::Network(_) + )); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/adapter-ews/src/lib.rs b/crates/adapter-ews/src/lib.rs index 68935dd57..5c764b4e1 100644 --- a/crates/adapter-ews/src/lib.rs +++ b/crates/adapter-ews/src/lib.rs @@ -1000,7 +1000,7 @@ impl CalendarFeature for EwsAdapter { }; api::update_event(&self.client, &event, zones.as_ref()) .await - .map_err(to_core_error) + .map_err(to_update_error) } async fn delete_event(&self, event_id: &str, send_cancellations: bool) -> CoreResult<()> { @@ -1384,6 +1384,48 @@ impl ContactsFeature for EwsAdapter { } } +/// [`to_core_error`] for an update. A status with which the server turned the +/// write down whole ([`cal_core::WriteRefusal::refused_status`]) is a refusal, +/// not a protocol error: a caller deciding whether the write may have landed — +/// splitting a series undoes its new part only when the cut certainly did not +/// (decision 144) — must be told that nothing was written. +/// +/// An update is one `UpdateItem` of one item, so a SOAP error answer to it +/// wrote nothing, whatever its code — except the server's own internal and +/// timeout errors, which may come after part of the work was done. The codes +/// [`to_core_error`] already names (sign-in, not found) keep their error. +fn to_update_error(err: EwsError) -> CoreError { + match err { + EwsError::Http { status, message } if cal_core::WriteRefusal::refused_status(status) => { + tracing::warn!(status, %message, "Exchange refused the update"); + CoreError::Forbidden( + cal_core::WriteRefusal::ServerRefused.message(&format!("HTTP {status}")), + ) + } + EwsError::Soap { code, message } if soap_refused_update(&code) => { + tracing::warn!(%code, %message, "Exchange refused the update"); + CoreError::Forbidden(cal_core::WriteRefusal::ServerRefused.message(&code)) + } + other => to_core_error(other), + } +} + +/// Whether a SOAP error answer to one item's `UpdateItem` certainly wrote +/// nothing; see [`to_update_error`]. +fn soap_refused_update(code: &str) -> bool { + let named = matches!( + code, + "ErrorAccessDenied" + | "ErrorInvalidAccessToken" + | "ErrorPasswordExpired" + | "ErrorADUnavailable" + | "ErrorNoFreeBusyAccess" + | "ErrorItemNotFound" + | "ErrorFolderNotFound" + ); + !named && !code.contains("InternalServer") && !code.contains("Timeout") +} + fn to_core_error(err: EwsError) -> CoreError { use EwsError::*; match err { @@ -1414,6 +1456,72 @@ fn to_core_error(err: EwsError) -> CoreError { } } +#[cfg(test)] +mod update_refusal_tests { + use super::*; + + fn soap(code: &str) -> EwsError { + EwsError::Soap { + code: code.into(), + message: "no".into(), + } + } + + /// An update is one `UpdateItem` of one item: a SOAP error answer wrote + /// nothing, and is a refusal (decision 144). + #[test] + fn a_soap_error_answer_to_an_update_is_a_refusal() { + for code in [ + "ErrorServerBusy", + "ErrorInvalidPropertySet", + "ErrorCalendarInvalidRecurrence", + ] { + match to_update_error(soap(code)) { + CoreError::Forbidden(msg) => assert_eq!(msg, format!("server-refused: {code}")), + other => panic!("{code}: {other:?}"), + } + } + match to_update_error(EwsError::Http { + status: 429, + message: "slow down".into(), + }) { + CoreError::Forbidden(msg) => assert_eq!(msg, "server-refused: HTTP 429"), + other => panic!("{other:?}"), + } + } + + /// ...except where the server's own failure may come after part of the + /// work, and where the error is already named. + #[test] + fn a_server_failure_stays_unsure_and_a_named_error_keeps_its_name() { + for code in [ + "ErrorInternalServerError", + "ErrorInternalServerTransientError", + "ErrorTimeoutExpired", + ] { + assert!( + matches!(to_update_error(soap(code)), CoreError::Protocol(_)), + "{code}" + ); + } + assert!(matches!( + to_update_error(soap("ErrorAccessDenied")), + CoreError::Authentication(_) + )); + assert!(matches!( + to_update_error(soap("ErrorItemNotFound")), + CoreError::NotFound(_) + )); + assert!(matches!( + to_update_error(EwsError::Http { + status: 503, + message: "busy".into(), + }), + CoreError::Protocol(_) + )); + } +} + #[cfg(test)] mod state_persistence_tests { use super::*; diff --git a/crates/adapter-google/src/lib.rs b/crates/adapter-google/src/lib.rs index fc80e48f4..a80b2aa2e 100644 --- a/crates/adapter-google/src/lib.rs +++ b/crates/adapter-google/src/lib.rs @@ -353,7 +353,7 @@ impl CalendarFeature for GoogleAdapter { async fn update_event(&self, event: Event) -> CoreResult { api::update_event(&self.state, &event) .await - .map_err(to_core_error) + .map_err(to_update_error) } async fn delete_event(&self, event_id: &str, send_cancellations: bool) -> CoreResult<()> { @@ -720,6 +720,23 @@ fn is_read_only_google_list(list_id: &str) -> bool { || list_id == contacts::GOOGLE_DIRECTORY_LIST_ID } +/// [`to_core_error`] for an update. A status with which the server turned the +/// write down whole ([`cal_core::WriteRefusal::refused_status`]) is a refusal, +/// not a protocol error: a caller deciding whether the write may have landed — +/// splitting a series undoes its new part only when the cut certainly did not +/// (decision 144) — must be told that nothing was written. +fn to_update_error(err: GoogleError) -> CoreError { + match err { + GoogleError::Http { status, message } if cal_core::WriteRefusal::refused_status(status) => { + tracing::warn!(status, %message, "Google refused the update"); + CoreError::Forbidden( + cal_core::WriteRefusal::ServerRefused.message(&format!("HTTP {status}")), + ) + } + other => to_core_error(other), + } +} + fn to_core_error(err: GoogleError) -> CoreError { use GoogleError::*; match err { @@ -746,6 +763,91 @@ mod delta_tests { use chrono::TimeZone; use mockito::{Matcher, Server}; + /// A series head, cut short: what splitting a series writes. + fn truncated_master() -> Event { + Event { + keep_attendees: false, + keep_fields: Vec::new(), + clear_attendees: false, + organized_elsewhere: false, + id: "master-1".into(), + calendar_id: "primary".into(), + title: "Teamrunde".into(), + description: None, + location: None, + start: chrono::Utc.with_ymd_and_hms(2026, 6, 1, 7, 0, 0).unwrap(), + end: chrono::Utc.with_ymd_and_hms(2026, 6, 1, 8, 0, 0).unwrap(), + all_day: false, + recurrence: Some(cal_core::EventRecurrence { + rrule: "FREQ=WEEKLY;UNTIL=20260824T065959Z".into(), + exceptions: vec![], + tzid: None, + }), + color_label: None, + color_hex: None, + reminders: vec![], + sound: None, + attendees: vec![], + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + etag: None, + organizer: None, + attendee_responses: vec![], + send_invitations: false, + truncate_tail_overrides: false, + cancelled: false, + scheduling_silenced: false, + } + } + + /// Google read the truncate and turned it down whole: nothing was written, + /// and the caller must know that for certain (decision 144), not as a + /// protocol error it has to treat as maybe landed. + #[tokio::test] + async fn an_update_google_turned_down_is_a_refusal() { + for status in [400, 429] { + let mut server = Server::new_async().await; + let _patch = server + .mock( + "PATCH", + Matcher::Regex(r"^/calendars/primary/events/master-1".into()), + ) + .with_status(status) + .with_body(r#"{"error":{"message":"no"}}"#) + .create_async() + .await; + let err = adapter_for(&server) + .update_event(truncated_master()) + .await + .unwrap_err(); + match err { + CoreError::Forbidden(msg) => { + assert_eq!(msg, format!("server-refused: HTTP {status}")) + } + other => panic!("{status}: {other:?}"), + } + } + } + + /// A server error says nothing about what was written: unsure, as before. + #[tokio::test] + async fn an_update_that_failed_in_the_server_is_not_a_refusal() { + let mut server = Server::new_async().await; + let _patch = server + .mock( + "PATCH", + Matcher::Regex(r"^/calendars/primary/events/master-1".into()), + ) + .with_status(503) + .create_async() + .await; + let err = adapter_for(&server) + .update_event(truncated_master()) + .await + .unwrap_err(); + assert!(matches!(err, CoreError::Protocol(_)), "{err:?}"); + } + /// Build an adapter whose API + token endpoints point at the mock /// server. The access token is valid for an hour so no refresh fires. fn adapter_for(server: &Server) -> GoogleAdapter { diff --git a/crates/adapter-microsoft-graph/src/api.rs b/crates/adapter-microsoft-graph/src/api.rs index d73a58ec1..39e051bf2 100644 --- a/crates/adapter-microsoft-graph/src/api.rs +++ b/crates/adapter-microsoft-graph/src/api.rs @@ -527,11 +527,13 @@ pub async fn delete_event(state: &ApiState, event_id: &str) -> GraphResult<()> { } /// `POST /me/events/{id}/cancel` — the ORGANIZER cancels a meeting: Graph -/// emails a cancellation to every attendee and marks the event cancelled -/// server-side (it stays on the organizer's calendar as `isCancelled` until -/// deleted). Organizer-only; Graph rejects it for a non-organizer or a -/// non-meeting. The caller (`delete_event` with `send_cancellations`) pairs it -/// with a follow-up DELETE to actually remove the row. +/// emails a cancellation to every attendee and marks the event cancelled. +/// Microsoft documents that the action moves the event to Deleted Items, and a +/// move gives an Outlook item a new id unless immutable ids are asked for +/// (this adapter does not), so the id may be gone afterwards. Organizer-only; +/// Graph rejects it for a non-organizer or a non-meeting. The caller +/// (`delete_event` with `send_cancellations`) pairs it with a follow-up DELETE +/// to remove the row wherever it still is, and takes a 404 there as done. pub async fn cancel_event(state: &ApiState, event_id: &str) -> GraphResult<()> { let id_enc = urlencoding(event_id); let path = format!("/me/events/{id_enc}/cancel"); diff --git a/crates/adapter-microsoft-graph/src/lib.rs b/crates/adapter-microsoft-graph/src/lib.rs index 6e73cc41f..dd0380a7b 100644 --- a/crates/adapter-microsoft-graph/src/lib.rs +++ b/crates/adapter-microsoft-graph/src/lib.rs @@ -350,12 +350,13 @@ impl CalendarFeature for MicrosoftGraphAdapter { async fn update_event(&self, event: Event) -> CoreResult { api::update_event(&self.state, &event) .await - .map_err(to_core_error) + .map_err(to_update_error) } async fn delete_event(&self, event_id: &str, send_cancellations: bool) -> CoreResult<()> { // Graph's event-id is mailbox-wide unique — no calendar // walk required, unlike Google. + let mut cancelled = false; if send_cancellations { // Organizer cancellation: notify attendees first (Graph marks the // event cancelled), then remove it. Graph's `/cancel` is @@ -371,7 +372,7 @@ impl CalendarFeature for MicrosoftGraphAdapter { // transient failure doesn't silently drop the cancellation and // delete anyway. match api::cancel_event(&self.state, event_id).await { - Ok(()) => {} + Ok(()) => cancelled = true, Err(GraphError::Http { status, .. }) if status == 400 || status == 403 => { tracing::debug!( status, @@ -382,9 +383,14 @@ impl CalendarFeature for MicrosoftGraphAdapter { Err(err) => return Err(to_core_error(err)), } } - api::delete_event(&self.state, event_id) - .await - .map_err(to_core_error) + match api::delete_event(&self.state, event_id).await { + // `/cancel` moved the event to Deleted Items, and a move gives an + // Outlook item a new id: the old one is gone, which is what was + // asked. Reported as a failure, undoing a split's new series said + // it could not, while it was cancelled and off the calendar. + Err(GraphError::Http { status: 404, .. }) if cancelled => Ok(()), + other => other.map_err(to_core_error), + } } async fn get_free_busy(&self, emails: &[&str], range: DateRange) -> CoreResult> { @@ -732,6 +738,23 @@ fn urlencoding(s: &str) -> String { out } +/// [`to_core_error`] for an update. A status with which the server turned the +/// write down whole ([`cal_core::WriteRefusal::refused_status`]) is a refusal, +/// not a protocol error: a caller deciding whether the write may have landed — +/// splitting a series undoes its new part only when the cut certainly did not +/// (decision 144) — must be told that nothing was written. +fn to_update_error(err: GraphError) -> CoreError { + match err { + GraphError::Http { status, message } if cal_core::WriteRefusal::refused_status(status) => { + tracing::warn!(status, %message, "Graph refused the update"); + CoreError::Forbidden( + cal_core::WriteRefusal::ServerRefused.message(&format!("HTTP {status}")), + ) + } + other => to_core_error(other), + } +} + fn to_core_error(err: GraphError) -> CoreError { use GraphError::*; match err { @@ -758,6 +781,125 @@ mod delta_tests { use chrono::TimeZone; use mockito::{Matcher, Server}; + /// A series head, cut short: what splitting a series writes. + fn truncated_master() -> Event { + Event { + keep_attendees: false, + keep_fields: Vec::new(), + clear_attendees: false, + organized_elsewhere: false, + id: "ev-1".into(), + calendar_id: "cal-1".into(), + title: "Teamrunde".into(), + description: None, + location: None, + start: chrono::Utc.with_ymd_and_hms(2026, 6, 1, 7, 0, 0).unwrap(), + end: chrono::Utc.with_ymd_and_hms(2026, 6, 1, 8, 0, 0).unwrap(), + all_day: false, + recurrence: Some(cal_core::EventRecurrence { + rrule: "FREQ=WEEKLY;UNTIL=20260824T065959Z".into(), + exceptions: vec![], + tzid: None, + }), + color_label: None, + color_hex: None, + reminders: vec![], + sound: None, + attendees: vec![], + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + etag: None, + organizer: None, + attendee_responses: vec![], + send_invitations: false, + truncate_tail_overrides: false, + cancelled: false, + scheduling_silenced: false, + } + } + + /// Graph read the truncate and turned it down whole: a refusal, so a split + /// takes its new part back (decision 144). + #[tokio::test] + async fn an_update_graph_turned_down_is_a_refusal() { + let mut server = Server::new_async().await; + // The read before the write may fail; it is only for the body. + let _get = server + .mock("GET", "/me/events/ev-1") + .with_status(404) + .create_async() + .await; + let _patch = server + .mock("PATCH", "/me/events/ev-1") + .with_status(400) + .with_body(r#"{"error":{"code":"ErrorInvalidRequest"}}"#) + .create_async() + .await; + let err = adapter_for(&server) + .update_event(truncated_master()) + .await + .unwrap_err(); + match err { + CoreError::Forbidden(msg) => assert_eq!(msg, "server-refused: HTTP 400"), + other => panic!("{other:?}"), + } + } + + /// `/cancel` moves the event to Deleted Items, which gives it a new id: the + /// DELETE that follows may find nothing, and that is the event gone. + #[tokio::test] + async fn a_delete_that_finds_nothing_after_the_cancel_is_done() { + let mut server = Server::new_async().await; + let _cancel = server + .mock("POST", "/me/events/ev-1/cancel") + .with_status(202) + .create_async() + .await; + let _delete = server + .mock("DELETE", "/me/events/ev-1") + .with_status(404) + .create_async() + .await; + adapter_for(&server) + .delete_event("ev-1", true) + .await + .expect("cancelled, and gone"); + } + + /// ...but only after a cancel: without one, a 404 is an event not found, + /// and after one, any other failure is still a failure. + #[tokio::test] + async fn a_delete_that_finds_nothing_without_a_cancel_is_not_found() { + let mut server = Server::new_async().await; + let _delete = server + .mock("DELETE", "/me/events/ev-1") + .with_status(404) + .create_async() + .await; + let err = adapter_for(&server) + .delete_event("ev-1", false) + .await + .unwrap_err(); + assert!(matches!(err, CoreError::NotFound(_)), "{err:?}"); + + let mut server = Server::new_async().await; + let _cancel = server + .mock("POST", "/me/events/ev-1/cancel") + .with_status(202) + .create_async() + .await; + let _delete = server + .mock("DELETE", "/me/events/ev-1") + .with_status(500) + .create_async() + .await; + let err = adapter_for(&server) + .delete_event("ev-1", true) + .await + .unwrap_err(); + assert!(matches!(err, CoreError::Protocol(_)), "{err:?}"); + } + fn adapter_for(server: &Server) -> MicrosoftGraphAdapter { let mut adapter = MicrosoftGraphAdapter::new( "client".into(), diff --git a/crates/cal-core/bindings/WriteRefusal.ts b/crates/cal-core/bindings/WriteRefusal.ts index b57074c33..d43ff64dc 100644 --- a/crates/cal-core/bindings/WriteRefusal.ts +++ b/crates/cal-core/bindings/WriteRefusal.ts @@ -3,4 +3,4 @@ /** * The reason a write did not happen. */ -export type WriteRefusal = "reply-only-invitation" | "server-refused" | "identity-unknown" | "occurrence-not-writable"; +export type WriteRefusal = "reply-only-invitation" | "server-refused" | "identity-unknown" | "occurrence-not-writable" | "unsafe-to-write"; diff --git a/crates/cal-core/src/write_refusal.rs b/crates/cal-core/src/write_refusal.rs index 3100b4b09..1062ad211 100644 --- a/crates/cal-core/src/write_refusal.rs +++ b/crates/cal-core/src/write_refusal.rs @@ -41,6 +41,12 @@ pub enum WriteRefusal { /// the same thing: this occurrence could not be saved on its own. Carving /// it out instead is never done behind their back (decision 92). OccurrenceNotWritable, + /// Aperio did not write, because the event could not be written without + /// risking what else its resource holds: the resource cannot be read + /// block by block, it holds no such event, or its components name + /// different organizers. The detail is a machine token for the log, as + /// for [`Self::OccurrenceNotWritable`]. + UnsafeToWrite, } impl WriteRefusal { @@ -51,9 +57,26 @@ impl WriteRefusal { Self::ServerRefused => "server-refused", Self::IdentityUnknown => "identity-unknown", Self::OccurrenceNotWritable => "occurrence-not-writable", + Self::UnsafeToWrite => "unsafe-to-write", } } + /// Whether a server that answered a write with this HTTP status turned the + /// write down whole: it read the request and wrote nothing (bad request, + /// too large, wrong media type, unprocessable, too many requests, no + /// storage). The statuses the adapters already name — 401, 403, 404, 409, + /// 412 — carry their own error; a 5xx may come after a part was written + /// and says nothing either way. + /// + /// Where a write is refused this way, an adapter reports + /// [`Self::ServerRefused`] rather than a protocol error, so that a caller + /// deciding whether the write may have landed (splitting a series undoes + /// its new part only when the cut certainly did not, decision 144) is told + /// the truth. + pub const fn refused_status(status: u16) -> bool { + matches!(status, 400 | 413 | 415 | 422 | 429 | 507) + } + /// The message: the token, and the detail behind a colon when there is one. pub fn message(self, detail: &str) -> String { let detail = detail.trim(); @@ -74,6 +97,8 @@ impl WriteRefusal { Self::ReplyOnlyInvitation, Self::ServerRefused, Self::IdentityUnknown, + Self::OccurrenceNotWritable, + Self::UnsafeToWrite, ] { let token = refusal.token(); let rest = match message.strip_prefix(token) { @@ -117,6 +142,40 @@ mod tests { ); } + #[test] + fn every_refusal_is_read_back() { + for refusal in [ + WriteRefusal::ReplyOnlyInvitation, + WriteRefusal::ServerRefused, + WriteRefusal::IdentityUnknown, + WriteRefusal::OccurrenceNotWritable, + WriteRefusal::UnsafeToWrite, + ] { + let msg = refusal.message("detail"); + assert_eq!( + WriteRefusal::parse(&msg), + Some((refusal, "detail")), + "{msg}" + ); + // The token is the serialized name the surfaces look up. + assert_eq!( + serde_json::to_value(refusal).unwrap(), + serde_json::json!(refusal.token()), + ); + } + } + + #[test] + fn a_refusing_status_is_one_that_wrote_nothing() { + for status in [400, 413, 415, 422, 429, 507] { + assert!(WriteRefusal::refused_status(status), "{status}"); + } + // Named elsewhere (401, 403, 404, 409, 412), or unknown either way. + for status in [200, 401, 403, 404, 409, 412, 500, 502, 503, 504] { + assert!(!WriteRefusal::refused_status(status), "{status}"); + } + } + #[test] fn a_message_that_only_starts_like_one_is_not_a_refusal() { assert_eq!(WriteRefusal::parse("server-refused-by-proxy: x"), None); diff --git a/locales/de/translation.json b/locales/de/translation.json index e071f1d46..9862903a2 100644 --- a/locales/de/translation.json +++ b/locales/de/translation.json @@ -1640,6 +1640,7 @@ "serverRefused": "Der Kalenderserver hat diese Änderung abgelehnt ({{detail}}). Es wurde nichts geändert.", "identityUnknown": "Aperio konnte die eigenen Adressen dieses Kontos auf dem Kalenderserver nicht lesen und kann dort deshalb keine Besprechung ändern. Es wurde nichts geändert.", "occurrenceNotWritable": "Dieser eine Termin lässt sich in seiner Serie nicht einzeln ändern. Es wurde nichts geändert.", + "unsafeToWrite": "Aperio kann diesen Termin nicht sicher ändern: Was auf dem Server noch mit ihm gespeichert ist, stünde auf dem Spiel. Es wurde nichts geändert.", "forbidden": "Der Anbieter erlaubt diese Änderung nicht ({{detail}}). Es wurde nichts geändert.", "changedOnServer": "Dieser Termin wurde auf dem Server geändert, seit du ihn geöffnet hast. Es wurde nichts geändert. Öffne ihn erneut, um den aktuellen Stand zu sehen.", "reason": { @@ -1647,6 +1648,7 @@ "serverRefused": "der Kalenderserver hat es abgelehnt ({{detail}})", "identityUnknown": "Aperio konnte die eigenen Adressen dieses Kontos auf dem Kalenderserver nicht lesen", "occurrenceNotWritable": "dieser eine Termin lässt sich in seiner Serie nicht einzeln ändern", + "unsafeToWrite": "Aperio kann ihn auf dem Server nicht sicher ändern", "forbidden": "der Anbieter erlaubt es nicht ({{detail}})", "changedOnServer": "sie wurde auf dem Server geändert, seit du sie geöffnet hast" } diff --git a/locales/en/translation.json b/locales/en/translation.json index 736f4bd0c..64605f786 100644 --- a/locales/en/translation.json +++ b/locales/en/translation.json @@ -1640,6 +1640,7 @@ "serverRefused": "The calendar server refused this change ({{detail}}). Nothing was changed.", "identityUnknown": "Aperio could not read this account's own addresses on the calendar server, so it cannot change a meeting there. Nothing was changed.", "occurrenceNotWritable": "This one occurrence cannot be changed on its own inside its series. Nothing was changed.", + "unsafeToWrite": "Aperio cannot change this event safely: what else is stored with it on the server would be put at risk. Nothing was changed.", "forbidden": "The provider does not allow this change ({{detail}}). Nothing was changed.", "changedOnServer": "This event changed on the server since you opened it. Nothing was changed. Open it again to see the current version.", "reason": { @@ -1647,6 +1648,7 @@ "serverRefused": "the calendar server refused it ({{detail}})", "identityUnknown": "Aperio could not read this account's own addresses on the calendar server", "occurrenceNotWritable": "this occurrence cannot be changed on its own inside its series", + "unsafeToWrite": "Aperio cannot change it safely on the server", "forbidden": "the provider does not allow it ({{detail}})", "changedOnServer": "it changed on the server since you opened it" } diff --git a/shared/eventWriteError.ts b/shared/eventWriteError.ts index 165aff454..f4f4cbff9 100644 --- a/shared/eventWriteError.ts +++ b/shared/eventWriteError.ts @@ -17,6 +17,7 @@ const REFUSAL_KEYS: Record = { 'server-refused': 'dialogs.event.writeError.serverRefused', 'identity-unknown': 'dialogs.event.writeError.identityUnknown', 'occurrence-not-writable': 'dialogs.event.writeError.occurrenceNotWritable', + 'unsafe-to-write': 'dialogs.event.writeError.unsafeToWrite', }; const TOKENS = Object.keys(REFUSAL_KEYS) as WriteRefusal[]; @@ -28,6 +29,7 @@ const REFUSAL_REASON_KEYS: Record = { 'server-refused': 'dialogs.event.writeError.reason.serverRefused', 'identity-unknown': 'dialogs.event.writeError.reason.identityUnknown', 'occurrence-not-writable': 'dialogs.event.writeError.reason.occurrenceNotWritable', + 'unsafe-to-write': 'dialogs.event.writeError.reason.unsafeToWrite', }; /** An error as the hosts hand it over: a code and a message. */ diff --git a/shared/generated/WriteRefusal.ts b/shared/generated/WriteRefusal.ts index b57074c33..d43ff64dc 100644 --- a/shared/generated/WriteRefusal.ts +++ b/shared/generated/WriteRefusal.ts @@ -3,4 +3,4 @@ /** * The reason a write did not happen. */ -export type WriteRefusal = "reply-only-invitation" | "server-refused" | "identity-unknown" | "occurrence-not-writable"; +export type WriteRefusal = "reply-only-invitation" | "server-refused" | "identity-unknown" | "occurrence-not-writable" | "unsafe-to-write"; diff --git a/src/state/eventWriteError.test.ts b/src/state/eventWriteError.test.ts index 69dc6dd57..41477cb9b 100644 --- a/src/state/eventWriteError.test.ts +++ b/src/state/eventWriteError.test.ts @@ -158,3 +158,24 @@ describe('eventWriteFailureReason', () => { expect(eventWriteFailureReason(err, t)).toBe(eventWriteErrorMessage(err, t)); }); }); + +describe('a write Aperio will not risk', () => { + it('reads as its own refusal, on either surface, and as nothing written', () => { + // A CalDAV resource whose blocks name different organizers is not + // written: the adapter says so with a token of its own. + const err = command('forbidden', 'unsafe-to-write: mixed-organizers'); + expect(eventWriteRefusal(err)?.refusal).toBe('unsafe-to-write'); + expect(eventWriteErrorMessage(err, t)).toMatch(/nicht sicher ändern/); + expect(eventWriteErrorMessage(err, t)).not.toMatch(/mixed-organizers/); + expect(eventWriteFailureReason(err, t)).not.toMatch(/nichts geändert/); + expect(writeNeverLanded(err)).toBe(true); + }); + + it('counts a server that turned the write down as nothing written', () => { + // 400, 429 and their kin, from every adapter's update (decision 147). + expect(writeNeverLanded(command('forbidden', 'server-refused: HTTP 429'))).toBe(true); + expect(eventWriteErrorMessage(command('forbidden', 'server-refused: HTTP 429'), t)).toMatch( + /abgelehnt \(HTTP 429\)/, + ); + }); +}); From 6ff44b3cec156222cde5edd207270a43d107dcfd Mon Sep 17 00:00:00 2001 From: Toni Barth Date: Sat, 26 Sep 2026 13:42:56 +0200 Subject: [PATCH 2/3] Review of #98: only what certainly wrote nothing is a refusal - EWS: the denylist ("every SOAP error but internal and timeout") took in codes a meeting update raises after it saved the item and while it sends it (send-as denied, quotas, message size, the store), `Unknown` and generic faults. `REFUSED_UPDATE_CODES` is now an allowlist of the codes Exchange raises while it checks or throttles the request; any other code stays unsure. The code is also read from a SOAP fault sent with HTTP 500, the usual shape of `ErrorServerBusy`, without its namespace prefix. - CalDAV: any failure of a replayed PUT whose first attempt may have landed is unsure, not only a 412: a 429 or 507 may be about the first attempt. - Google and Graph: a token endpoint that refuses the refresh (400/401, `invalid_grant`) is a sign-in failure, reported as a 401, not the calendar server refusing the change. A refused update carries the server's own reason after the status. - CalDAV: `to_core_error`'s doc comment is back on it. - TODO.md follows. Co-Authored-By: Claude Opus 5.5 --- TODO.md | 16 ++- crates/adapter-caldav/src/events.rs | 37 +++++-- crates/adapter-caldav/src/lib.rs | 4 +- crates/adapter-ews/src/lib.rs | 120 +++++++++++++++++----- crates/adapter-google/src/api.rs | 20 +++- crates/adapter-google/src/lib.rs | 54 +++++++++- crates/adapter-microsoft-graph/src/api.rs | 20 +++- crates/adapter-microsoft-graph/src/lib.rs | 57 +++++++++- 8 files changed, 279 insertions(+), 49 deletions(-) diff --git a/TODO.md b/TODO.md index 4edc195cb..f10e5d134 100644 --- a/TODO.md +++ b/TODO.md @@ -2425,11 +2425,17 @@ Siehe DESIGN §4.2. gekürzt wurde. Jetzt meldet jeder Adapter beim Ändern eines Termins (`update_event`) eine Ablehnung als solche: ein HTTP-Status, mit dem der Server das Schreiben ganz abgelehnt hat - (`WriteRefusal::refused_status`: 400, 413, 415, 422, 429, 507), und bei - EWS jede SOAP-Fehlerantwort auf das eine `UpdateItem` außer internen - Server- und Zeitüberschreitungsfehlern, als `server-refused`; CalDAVs - eigene Prüfungen als neue Marke `unsafe-to-write` („Aperio kann diesen - Termin nicht sicher ändern“). Beide reisen als `forbidden` und kommen so + (`WriteRefusal::refused_status`: 400, 413, 415, 422, 429, 507), samt dem + Grund, den Google und Microsoft nennen, und bei EWS die bekannten Prüf- + und Drosselungsfehler (`REFUSED_UPDATE_CODES`, auch als SOAP-Fault mit + HTTP 500), als `server-refused`; jeder andere EWS-Code bleibt unklar, + weil eine Besprechung erst gespeichert und dann verschickt wird. CalDAVs + eigene Prüfungen kommen als neue Marke `unsafe-to-write` („Aperio kann + diesen Termin nicht sicher ändern“). Antwortet der Server auf eine + CalDAV-Wiederholung mit einem Fehler, bleibt es unklar: der erste + Versuch kann angekommen sein. Scheitert bei Google oder Microsoft die + Erneuerung des Tokens (400/401 am Token-Endpunkt), ist es ein + Anmeldefehler, keine Ablehnung des Kalenderservers. Beide reisen als `forbidden` und kommen so auch am Handy an. Graph: ein DELETE, das nach erfolgreichem `/cancel` nichts mehr findet, gilt als erledigt (die Absage verschiebt das Ereignis nach „Gelöschte Elemente“, mit neuer Id). Nicht hier: Anlegen diff --git a/crates/adapter-caldav/src/events.rs b/crates/adapter-caldav/src/events.rs index 4e8ba4c5b..a8bccaffd 100644 --- a/crates/adapter-caldav/src/events.rs +++ b/crates/adapter-caldav/src/events.rs @@ -810,15 +810,14 @@ async fn put_resource( .body(body) .send_retrying_marked() .await?; - // A 412 on the replay of a guarded write: the connection died after the - // first PUT went out, and that one may have landed — its new ETag is what - // refuses the replay. Read as a refusal, a caller would undo around a write - // that went through: splitting a series deleted its new part while the old - // part was already cut short (decision 144). So it is what it is, unsure. - if first_may_have_landed - && if_match.is_some() - && response.status() == StatusCode::PRECONDITION_FAILED - { + // Any refusal of a replay whose first attempt may have landed: the + // connection died after the first PUT went out, and the answer may be + // about that one — a 412 because its new ETag refuses the replay, a 429 or + // a 507 because it was taken. Read as a refusal, a caller would undo + // around a write that went through: splitting a series deleted its new + // part while the old part was already cut short (decision 144). So it is + // what it is, unsure. + if first_may_have_landed && !response.status().is_success() { return Err(CaldavError::Network(format!( "the connection to '{resource}' broke after the change was sent; \ it may have been saved" @@ -1869,6 +1868,26 @@ END:VCALENDAR assert!(matches!(err, CaldavError::Network(_)), "{err:?}"); } + /// ...and so is any other refusal of that replay: a 429 may be the server + /// throttling a second copy of what it already stored. + #[tokio::test] + async fn any_refusal_of_a_replayed_put_is_unsure() { + let base = + first_attempt_lost_then(b"HTTP/1.1 429 Too Many Requests\r\ncontent-length: 0\r\n\r\n") + .await; + let resource = Url::parse(&format!("{base}/calendars/alice/work/abc.ics")).unwrap(); + let err = put_resource( + &client(), + &resource, + standup_body("Cut short"), + None, + &creds(&base), + ) + .await + .expect_err("the replay was refused"); + assert!(matches!(err, CaldavError::Network(_)), "{err:?}"); + } + /// Without a replay, a 412 is what it says: the copy moved on. #[tokio::test] async fn a_412_without_a_replay_is_a_conflict() { diff --git a/crates/adapter-caldav/src/lib.rs b/crates/adapter-caldav/src/lib.rs index 8c1e4a90a..7318cf382 100644 --- a/crates/adapter-caldav/src/lib.rs +++ b/crates/adapter-caldav/src/lib.rs @@ -926,8 +926,6 @@ impl CaldavAdapter { } } -/// Translate a CalDAV-specific error into the shared `cal_core::Error` -/// shape so the rest of the app can pattern-match it uniformly. /// [`to_core_error`] for an update. A status with which the server turned the /// write down whole ([`cal_core::WriteRefusal::refused_status`]) is a refusal, /// not a protocol error: a caller deciding whether the write may have landed — @@ -945,6 +943,8 @@ fn to_update_error(err: CaldavError) -> CoreError { } } +/// Translate a CalDAV-specific error into the shared `cal_core::Error` +/// shape so the rest of the app can pattern-match it uniformly. fn to_core_error(err: CaldavError) -> CoreError { match err { CaldavError::Network(msg) => CoreError::Network(msg), diff --git a/crates/adapter-ews/src/lib.rs b/crates/adapter-ews/src/lib.rs index 5c764b4e1..2a3fccedc 100644 --- a/crates/adapter-ews/src/lib.rs +++ b/crates/adapter-ews/src/lib.rs @@ -1390,10 +1390,13 @@ impl ContactsFeature for EwsAdapter { /// splitting a series undoes its new part only when the cut certainly did not /// (decision 144) — must be told that nothing was written. /// -/// An update is one `UpdateItem` of one item, so a SOAP error answer to it -/// wrote nothing, whatever its code — except the server's own internal and -/// timeout errors, which may come after part of the work was done. The codes -/// [`to_core_error`] already names (sign-in, not found) keep their error. +/// Only the SOAP codes with which Exchange turns an `UpdateItem` down while it +/// checks or throttles the request count ([`REFUSED_UPDATE_CODES`]): a meeting +/// update may save the item and then fail while sending it (send-as denied, a +/// quota, the store going away), so any other code says nothing either way. +/// Such a code is read from a SOAP fault sent with HTTP 500 too — the usual +/// shape of `ErrorServerBusy`. The codes [`to_core_error`] already names +/// (sign-in, not found) keep their error. fn to_update_error(err: EwsError) -> CoreError { match err { EwsError::Http { status, message } if cal_core::WriteRefusal::refused_status(status) => { @@ -1402,28 +1405,51 @@ fn to_update_error(err: EwsError) -> CoreError { cal_core::WriteRefusal::ServerRefused.message(&format!("HTTP {status}")), ) } - EwsError::Soap { code, message } if soap_refused_update(&code) => { + EwsError::Http { + status: 500, + message, + } => match soap::check_for_fault(&message) { + Err(EwsError::Soap { code, message: why }) if refused_update_code(&code).is_some() => { + let code = refused_update_code(&code).unwrap_or_default(); + tracing::warn!(%code, %why, "Exchange refused the update"); + CoreError::Forbidden(cal_core::WriteRefusal::ServerRefused.message(code)) + } + _ => to_core_error(EwsError::Http { + status: 500, + message, + }), + }, + EwsError::Soap { code, message } if refused_update_code(&code).is_some() => { + let code = refused_update_code(&code).unwrap_or_default(); tracing::warn!(%code, %message, "Exchange refused the update"); - CoreError::Forbidden(cal_core::WriteRefusal::ServerRefused.message(&code)) + CoreError::Forbidden(cal_core::WriteRefusal::ServerRefused.message(code)) } other => to_core_error(other), } } -/// Whether a SOAP error answer to one item's `UpdateItem` certainly wrote -/// nothing; see [`to_update_error`]. -fn soap_refused_update(code: &str) -> bool { - let named = matches!( - code, - "ErrorAccessDenied" - | "ErrorInvalidAccessToken" - | "ErrorPasswordExpired" - | "ErrorADUnavailable" - | "ErrorNoFreeBusyAccess" - | "ErrorItemNotFound" - | "ErrorFolderNotFound" - ); - !named && !code.contains("InternalServer") && !code.contains("Timeout") +/// The SOAP codes with which Exchange turns an `UpdateItem` down before it +/// saves anything: the request does not validate, the item moved on +/// meanwhile, or the server throttles. See [`to_update_error`]. +const REFUSED_UPDATE_CODES: &[&str] = &[ + "ErrorServerBusy", + "ErrorInvalidRequest", + "ErrorSchemaValidation", + "ErrorInvalidPropertySet", + "ErrorInvalidPropertyDelete", + "ErrorInvalidPropertyUpdateSentMessage", + "ErrorCalendarInvalidRecurrence", + "ErrorInvalidIdMalformed", + "ErrorInvalidChangeKey", + "ErrorIrresolvableConflict", + "ErrorStaleObject", +]; + +/// The code, without the namespace prefix a fault's `faultcode` carries +/// (`a:ErrorServerBusy`), when it is one of [`REFUSED_UPDATE_CODES`]. +fn refused_update_code(code: &str) -> Option<&str> { + let bare = code.rsplit(':').next().unwrap_or(code); + REFUSED_UPDATE_CODES.contains(&bare).then_some(bare) } fn to_core_error(err: EwsError) -> CoreError { @@ -1467,8 +1493,8 @@ mod update_refusal_tests { } } - /// An update is one `UpdateItem` of one item: a SOAP error answer wrote - /// nothing, and is a refusal (decision 144). + /// Exchange turned the update down while checking or throttling it: + /// nothing was saved, and it is a refusal (decision 144). #[test] fn a_soap_error_answer_to_an_update_is_a_refusal() { for code in [ @@ -1490,14 +1516,22 @@ mod update_refusal_tests { } } - /// ...except where the server's own failure may come after part of the - /// work, and where the error is already named. + /// ...but any other code may come after the item was saved — a meeting + /// update saves, then sends — and stays unsure; a named error keeps its + /// name. #[test] fn a_server_failure_stays_unsure_and_a_named_error_keeps_its_name() { for code in [ "ErrorInternalServerError", "ErrorInternalServerTransientError", "ErrorTimeoutExpired", + "ErrorSendAsDenied", + "ErrorQuotaExceeded", + "ErrorSubmissionQuotaExceeded", + "ErrorMessageSizeExceeded", + "ErrorMailboxStoreUnavailable", + "Unknown", + "s:Server", ] { assert!( matches!(to_update_error(soap(code)), CoreError::Protocol(_)), @@ -1520,6 +1554,44 @@ mod update_refusal_tests { CoreError::Protocol(_) )); } + + fn fault_500(code: &str) -> EwsError { + EwsError::Http { + status: 500, + message: format!( + r#" + + + + a:{code} + The server cannot service this request right now. Try again later. + + +"# + ), + } + } + + /// Exchange throttles with a SOAP fault sent as HTTP 500: read from the + /// fault, it is the refusal it names; any other 500 stays unsure. + #[test] + fn a_throttling_fault_sent_as_500_is_a_refusal() { + match to_update_error(fault_500("ErrorServerBusy")) { + CoreError::Forbidden(msg) => assert_eq!(msg, "server-refused: ErrorServerBusy"), + other => panic!("{other:?}"), + } + assert!(matches!( + to_update_error(fault_500("ErrorInternalServerError")), + CoreError::Protocol(_) + )); + assert!(matches!( + to_update_error(EwsError::Http { + status: 500, + message: "Server Error".into(), + }), + CoreError::Protocol(_) + )); + } } #[cfg(test)] diff --git a/crates/adapter-google/src/api.rs b/crates/adapter-google/src/api.rs index 56fb88594..2f03d4bca 100644 --- a/crates/adapter-google/src/api.rs +++ b/crates/adapter-google/src/api.rs @@ -215,7 +215,7 @@ impl ApiState { .await .map_err(|err| { warn!(?err, "refresh-token grant failed"); - err + refresh_refused(err) })?; let mut guard = self.tokens.lock().await; guard.access_token = fresh.access_token; @@ -229,6 +229,24 @@ impl ApiState { } } +/// A token endpoint that answered the refresh with 400 or 401 — a revoked or +/// expired grant (`invalid_grant`), a client it does not know — refused the +/// sign-in, not the request that needed it: said as a 401, the write that was +/// waiting for the token reads as a sign-in failure. As its own status, an +/// update read it as the calendar server refusing the change (decision 147). +/// Anything else keeps its error. +fn refresh_refused(err: GoogleError) -> GoogleError { + match err { + GoogleError::Http { status, message } if status == 400 || status == 401 => { + GoogleError::Http { + status: 401, + message: format!("token refresh refused (HTTP {status}): {message}"), + } + } + other => other, + } +} + /// The one non-success answer that is not a failure: Google refusing a /// DIRECTORY listing to an account that has none. /// diff --git a/crates/adapter-google/src/lib.rs b/crates/adapter-google/src/lib.rs index a80b2aa2e..a5a32f1bf 100644 --- a/crates/adapter-google/src/lib.rs +++ b/crates/adapter-google/src/lib.rs @@ -729,14 +729,29 @@ fn to_update_error(err: GoogleError) -> CoreError { match err { GoogleError::Http { status, message } if cal_core::WriteRefusal::refused_status(status) => { tracing::warn!(status, %message, "Google refused the update"); - CoreError::Forbidden( - cal_core::WriteRefusal::ServerRefused.message(&format!("HTTP {status}")), - ) + CoreError::Forbidden(cal_core::WriteRefusal::ServerRefused.message( + &match server_reason(&message) { + Some(reason) => format!("HTTP {status}: {reason}"), + None => format!("HTTP {status}"), + }, + )) } other => to_core_error(other), } } +/// The reason a JSON error answer gives (`{"error":{"message":"…"}}`), +/// trimmed, for the sentence that says the server refused: "HTTP 400" alone +/// tells the user nothing they can act on. +fn server_reason(body: &str) -> Option { + let value: serde_json::Value = serde_json::from_str(body).ok()?; + let reason = value.get("error")?.get("message")?.as_str()?.trim(); + if reason.is_empty() { + return None; + } + Some(reason.chars().take(160).collect()) +} + fn to_core_error(err: GoogleError) -> CoreError { use GoogleError::*; match err { @@ -820,15 +835,46 @@ mod delta_tests { .update_event(truncated_master()) .await .unwrap_err(); + // The server's own reason travels with the status. match err { CoreError::Forbidden(msg) => { - assert_eq!(msg, format!("server-refused: HTTP {status}")) + assert_eq!(msg, format!("server-refused: HTTP {status}: no")) } other => panic!("{status}: {other:?}"), } } } + const PATCH_PATH: &str = r"^/calendars/primary/events/master-1"; + + /// The save met an expired access token, and the token endpoint refused + /// the refresh (a revoked grant answers 400 `invalid_grant`): a sign-in + /// failure, as the error the user can act on — not the calendar server + /// refusing the change. + #[tokio::test] + async fn a_refused_token_refresh_during_an_update_is_a_sign_in_failure() { + let mut server = Server::new_async().await; + let _write = server + .mock("PATCH", Matcher::Regex(PATCH_PATH.into())) + .with_status(401) + .create_async() + .await; + let _token = server + .mock("POST", "/token") + .with_status(400) + .with_body(r#"{"error":"invalid_grant","error_description":"Token has been expired or revoked."}"#) + .create_async() + .await; + let err = adapter_for(&server) + .update_event(truncated_master()) + .await + .unwrap_err(); + match err { + CoreError::Authentication(msg) => assert!(msg.contains("invalid_grant"), "{msg}"), + other => panic!("expected a sign-in failure, got {other:?}"), + } + } + /// A server error says nothing about what was written: unsure, as before. #[tokio::test] async fn an_update_that_failed_in_the_server_is_not_a_refusal() { diff --git a/crates/adapter-microsoft-graph/src/api.rs b/crates/adapter-microsoft-graph/src/api.rs index 39e051bf2..2c4a463f5 100644 --- a/crates/adapter-microsoft-graph/src/api.rs +++ b/crates/adapter-microsoft-graph/src/api.rs @@ -240,7 +240,7 @@ impl ApiState { .await .map_err(|err| { warn!(?err, "refresh-token grant failed"); - err + refresh_refused(err) })?; let mut guard = self.tokens.lock().await; guard.access_token = fresh.access_token; @@ -252,6 +252,24 @@ impl ApiState { } } +/// A token endpoint that answered the refresh with 400 or 401 — a revoked or +/// expired grant (`invalid_grant`), a client it does not know — refused the +/// sign-in, not the request that needed it: said as a 401, the write that was +/// waiting for the token reads as a sign-in failure. As its own status, an +/// update read it as the calendar server refusing the change (decision 147). +/// Anything else keeps its error. +fn refresh_refused(err: GraphError) -> GraphError { + match err { + GraphError::Http { status, message } if status == 400 || status == 401 => { + GraphError::Http { + status: 401, + message: format!("token refresh refused (HTTP {status}): {message}"), + } + } + other => other, + } +} + async fn decode_json(response: reqwest::Response) -> GraphResult { let status = response.status(); let text = response.text().await.unwrap_or_default(); diff --git a/crates/adapter-microsoft-graph/src/lib.rs b/crates/adapter-microsoft-graph/src/lib.rs index dd0380a7b..cfeabdcd3 100644 --- a/crates/adapter-microsoft-graph/src/lib.rs +++ b/crates/adapter-microsoft-graph/src/lib.rs @@ -747,14 +747,29 @@ fn to_update_error(err: GraphError) -> CoreError { match err { GraphError::Http { status, message } if cal_core::WriteRefusal::refused_status(status) => { tracing::warn!(status, %message, "Graph refused the update"); - CoreError::Forbidden( - cal_core::WriteRefusal::ServerRefused.message(&format!("HTTP {status}")), - ) + CoreError::Forbidden(cal_core::WriteRefusal::ServerRefused.message( + &match server_reason(&message) { + Some(reason) => format!("HTTP {status}: {reason}"), + None => format!("HTTP {status}"), + }, + )) } other => to_core_error(other), } } +/// The reason a JSON error answer gives (`{"error":{"message":"…"}}`), +/// trimmed, for the sentence that says the server refused: "HTTP 400" alone +/// tells the user nothing they can act on. +fn server_reason(body: &str) -> Option { + let value: serde_json::Value = serde_json::from_str(body).ok()?; + let reason = value.get("error")?.get("message")?.as_str()?.trim(); + if reason.is_empty() { + return None; + } + Some(reason.chars().take(160).collect()) +} + fn to_core_error(err: GraphError) -> CoreError { use GraphError::*; match err { @@ -839,12 +854,48 @@ mod delta_tests { .update_event(truncated_master()) .await .unwrap_err(); + // An answer without a message of its own gives the status alone. match err { CoreError::Forbidden(msg) => assert_eq!(msg, "server-refused: HTTP 400"), other => panic!("{other:?}"), } } + const PATCH_PATH: &str = r"^/me/events/ev-1$"; + + /// The save met an expired access token, and the token endpoint refused + /// the refresh (a revoked grant answers 400 `invalid_grant`): a sign-in + /// failure, as the error the user can act on — not the calendar server + /// refusing the change. + #[tokio::test] + async fn a_refused_token_refresh_during_an_update_is_a_sign_in_failure() { + let mut server = Server::new_async().await; + let _get = server + .mock("GET", "/me/events/ev-1") + .with_status(404) + .create_async() + .await; + let _write = server + .mock("PATCH", Matcher::Regex(PATCH_PATH.into())) + .with_status(401) + .create_async() + .await; + let _token = server + .mock("POST", "/token") + .with_status(400) + .with_body(r#"{"error":"invalid_grant","error_description":"Token has been expired or revoked."}"#) + .create_async() + .await; + let err = adapter_for(&server) + .update_event(truncated_master()) + .await + .unwrap_err(); + match err { + CoreError::Authentication(msg) => assert!(msg.contains("invalid_grant"), "{msg}"), + other => panic!("expected a sign-in failure, got {other:?}"), + } + } + /// `/cancel` moves the event to Deleted Items, which gives it a new id: the /// DELETE that follows may find nothing, and that is the event gone. #[tokio::test] From 3084ad21b76a2380c698048ab0739613db72ed0e Mon Sep 17 00:00:00 2001 From: Toni Barth Date: Sat, 26 Sep 2026 14:19:37 +0200 Subject: [PATCH 3/3] Second review of #98: conflict codes stay unsure, reasons survive a cut - EWS: `ErrorIrresolvableConflict` and `ErrorStaleObject` are off the refusal list. The update is sent with `AlwaysOverwrite`, so no ChangeKey is checked before the save, and a conflict code comes from somewhere else; it stays unsure. - CalDAV: a replayed PUT answered with a failure keeps its answer: the status goes into the error and status and body into the log, where the replay rule had dropped both. - Google and Graph: the error keeps only the first 300 characters of an answer, so a realistic envelope arrived as broken JSON and its reason was dropped. `server_reason` now reads the first "message" string of a cut answer as far as it goes. - host-core: `is_auth_shaped`'s doc and test name the shape Google and Graph now report for a refused refresh, beside the one other OAuth adapters still send. - TODO.md names the two tokens that travel as `forbidden`, and where the replay and refresh failures travel. Co-Authored-By: Claude Opus 5.5 --- TODO.md | 13 ++++-- crates/adapter-caldav/src/events.rs | 20 ++++++-- crates/adapter-ews/src/lib.rs | 11 +++-- crates/adapter-google/src/lib.rs | 56 ++++++++++++++++++++++- crates/adapter-microsoft-graph/src/lib.rs | 56 ++++++++++++++++++++++- crates/host-core/src/cache/mod.rs | 10 ++-- crates/host-core/src/cache/tests.rs | 9 ++-- 7 files changed, 153 insertions(+), 22 deletions(-) diff --git a/TODO.md b/TODO.md index f10e5d134..9cae6be92 100644 --- a/TODO.md +++ b/TODO.md @@ -2429,14 +2429,17 @@ Siehe DESIGN §4.2. Grund, den Google und Microsoft nennen, und bei EWS die bekannten Prüf- und Drosselungsfehler (`REFUSED_UPDATE_CODES`, auch als SOAP-Fault mit HTTP 500), als `server-refused`; jeder andere EWS-Code bleibt unklar, - weil eine Besprechung erst gespeichert und dann verschickt wird. CalDAVs + weil eine Besprechung erst gespeichert und dann verschickt wird; die + Konfliktcodes auch, denn Aperio schreibt mit `AlwaysOverwrite`. CalDAVs eigene Prüfungen kommen als neue Marke `unsafe-to-write` („Aperio kann - diesen Termin nicht sicher ändern“). Antwortet der Server auf eine - CalDAV-Wiederholung mit einem Fehler, bleibt es unklar: der erste + diesen Termin nicht sicher ändern“). `server-refused` und + `unsafe-to-write` reisen als `forbidden` und kommen so auch am Handy an. + Antwortet der Server auf eine CalDAV-Wiederholung mit einem Fehler, + bleibt es unklar (`network`, die Antwort steht im Protokoll): der erste Versuch kann angekommen sein. Scheitert bei Google oder Microsoft die Erneuerung des Tokens (400/401 am Token-Endpunkt), ist es ein - Anmeldefehler, keine Ablehnung des Kalenderservers. Beide reisen als `forbidden` und kommen so - auch am Handy an. Graph: ein DELETE, das nach erfolgreichem `/cancel` + Anmeldefehler (`auth`), keine Ablehnung des Kalenderservers; am Handy + kommt er ohne Code an und bleibt dort unklar. Graph: ein DELETE, das nach erfolgreichem `/cancel` nichts mehr findet, gilt als erledigt (die Absage verschiebt das Ereignis nach „Gelöschte Elemente“, mit neuer Id). Nicht hier: Anlegen und Löschen melden solche Ablehnungen weiter als `protocol` (für das diff --git a/crates/adapter-caldav/src/events.rs b/crates/adapter-caldav/src/events.rs index a8bccaffd..523c9e301 100644 --- a/crates/adapter-caldav/src/events.rs +++ b/crates/adapter-caldav/src/events.rs @@ -818,9 +818,19 @@ async fn put_resource( // part while the old part was already cut short (decision 144). So it is // what it is, unsure. if first_may_have_landed && !response.status().is_success() { + // The answer itself is kept for the log: it may be the first + // attempt's, or a refusal the replay met on its own. + let status = response.status().as_u16(); + let body = response.text().await.unwrap_or_default(); + tracing::warn!( + %resource, + status, + body = %body.chars().take(200).collect::(), + "a replayed PUT was answered with a failure; the first attempt may have been saved", + ); return Err(CaldavError::Network(format!( - "the connection to '{resource}' broke after the change was sent; \ - it may have been saved" + "the connection to '{resource}' broke after the change was sent \ + (the replay was answered HTTP {status}); it may have been saved" ))); } check_write(response).await @@ -1885,7 +1895,11 @@ END:VCALENDAR ) .await .expect_err("the replay was refused"); - assert!(matches!(err, CaldavError::Network(_)), "{err:?}"); + match err { + // The replay's answer is kept, for whoever reads the error. + CaldavError::Network(msg) => assert!(msg.contains("HTTP 429"), "{msg}"), + other => panic!("expected unsure, got {other:?}"), + } } /// Without a replay, a 412 is what it says: the copy moved on. diff --git a/crates/adapter-ews/src/lib.rs b/crates/adapter-ews/src/lib.rs index 2a3fccedc..9c6ad73c3 100644 --- a/crates/adapter-ews/src/lib.rs +++ b/crates/adapter-ews/src/lib.rs @@ -1429,8 +1429,11 @@ fn to_update_error(err: EwsError) -> CoreError { } /// The SOAP codes with which Exchange turns an `UpdateItem` down before it -/// saves anything: the request does not validate, the item moved on -/// meanwhile, or the server throttles. See [`to_update_error`]. +/// saves anything: the request does not validate, or the server throttles. +/// See [`to_update_error`]. The update is sent with `AlwaysOverwrite`, so no +/// ChangeKey is checked before the save, and a conflict code +/// (`ErrorIrresolvableConflict`, `ErrorStaleObject`) comes from somewhere +/// else: it is not on this list and stays unsure. const REFUSED_UPDATE_CODES: &[&str] = &[ "ErrorServerBusy", "ErrorInvalidRequest", @@ -1441,8 +1444,6 @@ const REFUSED_UPDATE_CODES: &[&str] = &[ "ErrorCalendarInvalidRecurrence", "ErrorInvalidIdMalformed", "ErrorInvalidChangeKey", - "ErrorIrresolvableConflict", - "ErrorStaleObject", ]; /// The code, without the namespace prefix a fault's `faultcode` carries @@ -1532,6 +1533,8 @@ mod update_refusal_tests { "ErrorMailboxStoreUnavailable", "Unknown", "s:Server", + "ErrorIrresolvableConflict", + "ErrorStaleObject", ] { assert!( matches!(to_update_error(soap(code)), CoreError::Protocol(_)), diff --git a/crates/adapter-google/src/lib.rs b/crates/adapter-google/src/lib.rs index a5a32f1bf..2f22f59ac 100644 --- a/crates/adapter-google/src/lib.rs +++ b/crates/adapter-google/src/lib.rs @@ -743,9 +743,35 @@ fn to_update_error(err: GoogleError) -> CoreError { /// The reason a JSON error answer gives (`{"error":{"message":"…"}}`), /// trimmed, for the sentence that says the server refused: "HTTP 400" alone /// tells the user nothing they can act on. +/// +/// The error keeps only the first 300 characters of the answer, so a longer +/// envelope arrives cut and is no JSON any more. The first `"message"` +/// string is then read as far as it goes: it is the error's own, before the +/// details and the request ids that make an envelope long. fn server_reason(body: &str) -> Option { - let value: serde_json::Value = serde_json::from_str(body).ok()?; - let reason = value.get("error")?.get("message")?.as_str()?.trim(); + let reason = match serde_json::from_str::(body) { + Ok(value) => value.get("error")?.get("message")?.as_str()?.to_string(), + Err(_) => { + let rest = &body[body.find("\"message\"")? + "\"message\"".len()..]; + let rest = rest.trim_start().strip_prefix(':')?.trim_start(); + let rest = rest.strip_prefix('"')?; + let mut out = String::new(); + let mut escaped = false; + for c in rest.chars() { + match (escaped, c) { + (true, _) => { + out.push(c); + escaped = false; + } + (false, '\\') => escaped = true, + (false, '"') => break, + (false, _) => out.push(c), + } + } + out + } + }; + let reason = reason.trim(); if reason.is_empty() { return None; } @@ -845,6 +871,32 @@ mod delta_tests { } } + /// The error keeps only the first 300 characters of an answer, so a + /// realistic envelope arrives cut: its reason is still read. + #[test] + fn the_reason_is_read_from_a_cut_answer() { + let long = "The recurrence rule does not generate an occurrence on the start date \ + of the event, so the event cannot be saved as it is."; + let body = format!( + r#"{{"error":{{"code":400,"message":"{long}","errors":[{{"domain":"global","reason":"invalid","message":"{long}"}}],"innerError":{{"date":"2026-09-26T10:00:00","request-id":"0f6c1d2e-9a8b-4c3d-8e7f-6a5b4c3d2e1f"}}}}}}"# + ); + let cut: String = body.chars().take(300).collect(); + assert!( + serde_json::from_str::(&cut).is_err(), + "the test must cut" + ); + assert_eq!(server_reason(&cut).as_deref(), Some(long)); + // A reason cut itself is read as far as it goes. + let cut_early: String = body.chars().take(60).collect(); + assert!(server_reason(&cut_early).is_some_and(|r| long.starts_with(&r))); + // No message, no reason. + assert_eq!( + server_reason(r#"{"error":{"code":"ErrorInvalidRequest"}}"#), + None + ); + assert_eq!(server_reason("Bad Request"), None); + } + const PATCH_PATH: &str = r"^/calendars/primary/events/master-1"; /// The save met an expired access token, and the token endpoint refused diff --git a/crates/adapter-microsoft-graph/src/lib.rs b/crates/adapter-microsoft-graph/src/lib.rs index cfeabdcd3..6ae8f30ac 100644 --- a/crates/adapter-microsoft-graph/src/lib.rs +++ b/crates/adapter-microsoft-graph/src/lib.rs @@ -761,9 +761,35 @@ fn to_update_error(err: GraphError) -> CoreError { /// The reason a JSON error answer gives (`{"error":{"message":"…"}}`), /// trimmed, for the sentence that says the server refused: "HTTP 400" alone /// tells the user nothing they can act on. +/// +/// The error keeps only the first 300 characters of the answer, so a longer +/// envelope arrives cut and is no JSON any more. The first `"message"` +/// string is then read as far as it goes: it is the error's own, before the +/// details and the request ids that make an envelope long. fn server_reason(body: &str) -> Option { - let value: serde_json::Value = serde_json::from_str(body).ok()?; - let reason = value.get("error")?.get("message")?.as_str()?.trim(); + let reason = match serde_json::from_str::(body) { + Ok(value) => value.get("error")?.get("message")?.as_str()?.to_string(), + Err(_) => { + let rest = &body[body.find("\"message\"")? + "\"message\"".len()..]; + let rest = rest.trim_start().strip_prefix(':')?.trim_start(); + let rest = rest.strip_prefix('"')?; + let mut out = String::new(); + let mut escaped = false; + for c in rest.chars() { + match (escaped, c) { + (true, _) => { + out.push(c); + escaped = false; + } + (false, '\\') => escaped = true, + (false, '"') => break, + (false, _) => out.push(c), + } + } + out + } + }; + let reason = reason.trim(); if reason.is_empty() { return None; } @@ -861,6 +887,32 @@ mod delta_tests { } } + /// The error keeps only the first 300 characters of an answer, so a + /// realistic envelope arrives cut: its reason is still read. + #[test] + fn the_reason_is_read_from_a_cut_answer() { + let long = "The recurrence rule does not generate an occurrence on the start date \ + of the event, so the event cannot be saved as it is."; + let body = format!( + r#"{{"error":{{"code":400,"message":"{long}","errors":[{{"domain":"global","reason":"invalid","message":"{long}"}}],"innerError":{{"date":"2026-09-26T10:00:00","request-id":"0f6c1d2e-9a8b-4c3d-8e7f-6a5b4c3d2e1f"}}}}}}"# + ); + let cut: String = body.chars().take(300).collect(); + assert!( + serde_json::from_str::(&cut).is_err(), + "the test must cut" + ); + assert_eq!(server_reason(&cut).as_deref(), Some(long)); + // A reason cut itself is read as far as it goes. + let cut_early: String = body.chars().take(60).collect(); + assert!(server_reason(&cut_early).is_some_and(|r| long.starts_with(&r))); + // No message, no reason. + assert_eq!( + server_reason(r#"{"error":{"code":"ErrorInvalidRequest"}}"#), + None + ); + assert_eq!(server_reason("Bad Request"), None); + } + const PATCH_PATH: &str = r"^/me/events/ev-1$"; /// The save met an expired access token, and the token endpoint refused diff --git a/crates/host-core/src/cache/mod.rs b/crates/host-core/src/cache/mod.rs index f4cc3c4aa..154f9dd8b 100644 --- a/crates/host-core/src/cache/mod.rs +++ b/crates/host-core/src/cache/mod.rs @@ -281,9 +281,13 @@ const CONFIRM_THRESHOLD: i64 = 2; /// opposed to a network blip)? Substring match over the usual suspects — /// conservative on purpose: a false "auth" only makes the UI suggest /// re-checking the password. The OAuth needles matter because a revoked -/// Google/Graph grant surfaces as the TOKEN endpoint's HTTP 400 body -/// (`{"error":"invalid_grant",...}`) embedded in a protocol error, not -/// as a 401 — exactly the case where re-authenticating is the fix. +/// grant's text carries the TOKEN endpoint's body +/// (`{"error":"invalid_grant",...}`): Google and Graph now report such a +/// refused refresh as an authentication error ("token refresh refused (HTTP +/// 400): …"), but the builtin-oauth adapters (Drive, Dropbox, Webex), plugin +/// messages passed through verbatim and older plugin builds still carry it +/// as "protocol error: … HTTP 400: …" — exactly the case where +/// re-authenticating is the fix. pub fn is_auth_shaped(error: &str) -> bool { let lower = error.to_lowercase(); [ diff --git a/crates/host-core/src/cache/tests.rs b/crates/host-core/src/cache/tests.rs index 16debd609..dbdc09300 100644 --- a/crates/host-core/src/cache/tests.rs +++ b/crates/host-core/src/cache/tests.rs @@ -2628,13 +2628,16 @@ fn auth_shaped_heuristic() { assert!(super::is_auth_shaped("HTTP 401 Unauthorized")); assert!(super::is_auth_shaped("server said: invalid credentials")); assert!(super::is_auth_shaped("403 Forbidden")); - // Revoked OAuth grant: the token endpoint's HTTP 400 body embedded - // in a protocol error — the exact string shape both OAuth adapters - // record (Google/Graph map token failures to a 400, not a 401). + // Revoked OAuth grant: the token endpoint's HTTP 400 body embedded in + // a protocol error — the shape builtin-oauth adapters and older plugin + // builds record — and as Google and Graph report it now. assert!(super::is_auth_shaped( "protocol error: Google HTTP 400: {\"error\":\"invalid_grant\",\ \"error_description\":\"Token has been expired or revoked.\"}" )); + assert!(super::is_auth_shaped( + "authentication failed: token refresh refused (HTTP 400): {\"error\":\"invalid_grant\"}" + )); assert!(!super::is_auth_shaped("connection reset by peer")); assert!(!super::is_auth_shaped("timeout after 30s")); }