From 7a14a5beafb8dfe30d59e3cbe0b28fab2cd47e2a Mon Sep 17 00:00:00 2001 From: Distronode <254259913+distronode-com@users.noreply.github.com> Date: Wed, 16 Sep 2026 20:48:36 -0400 Subject: [PATCH 1/2] caldav: move or cancel an event as the account that holds it UpdateEvent and CancelEvent loaded the user's current destination connection and sent its Basic credentials to the stored event URL. A host can connect several CalDAV accounts, so once the destination moved from an account on server A to one on server B, rescheduling or cancelling an older booking sent B's username and app password to server A. A refusal left the event in place and the reconciler retried, resending them every sweep for a cancelled booking and until the end time for a moved one. A 404 was taken as "already gone". Update and cancel now resolve the owning connection from what the booking stored. An account can hold the event only if the event URL lies inside one of its collections (the bound calendar_id or a saved connection_calendars id): same scheme, host and port, path under it at a segment boundary. Among those, the one whose collection is the recorded calendar id wins, else the most specific collection. No match or a tie sends nothing and returns an error for the caller to log. With no CalDAV connection at all it returns nil, as the Provider contract says, and logs a warning. A single account that is still the destination sends exactly the requests it did before. The Service routed update and cancel by the current destination's provider, so after a move to Google or Microsoft the CalDAV event URL went to that provider, which cannot find it, and the event on the CalDAV server was never touched. calendar.EventRecognizer lets a provider claim its own ids; CalDAV claims absolute http(s) URLs, which Google and Microsoft ids never are, and everything else still routes by destination. Tests use two httptest servers that record the credentials on every request: the cross-account move and cancel, an empty calendar id (as reassign passes), an event in a picked sub-calendar, a destination moved to another provider, a disconnected owner, and the ambiguous and no-match cases sending zero requests. Restoring loadConn(ctx, userID, -1, 1) fails three of them with B's password seen on server A. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 26 ++ docs/ARCHITECTURE.md | 14 +- internal/caldav/events.go | 191 ++++++++++- internal/caldav/events_test.go | 593 +++++++++++++++++++++++++++++++++ internal/calendar/calendar.go | 32 +- internal/handler/reassign.go | 2 + 6 files changed, 849 insertions(+), 9 deletions(-) create mode 100644 internal/caldav/events_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f45a54ce..1b2b32ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,32 @@ exact tag (`ghcr.io/calnode/calnode:0.1.0`) if you need stability between upgrad ## [Unreleased] +### Security +- **Moving or cancelling a CalDAV booking no longer sends another account's app password + to the server holding the event.** A host can connect several CalDAV accounts, and + rescheduling or cancelling authenticated as whichever account was the destination at + the time, not the one the event was written to. After a host moved their destination + from an account on one server to an account on another, every reschedule or cancel of + an older booking sent the new account's username and app password to the old account's + server. A server that refused them left the calendar unchanged and the reconciler + retried, sending them again every sweep for a cancelled booking and until the end time + for a moved one. A server that answered 404 instead was taken at its word: the event + counted as already gone and stayed where it was. + + Update and cancel now authenticate as the account that holds the event, found from what + the booking stored: the calendar recorded at creation, or failing that the connected + calendar whose URL contains the event's URL (same scheme, host and port). If no single + account can be established, nothing is sent and the error is logged. Hosts who moved a + CalDAV destination between accounts on different servers should consider rotating the + app password of the account they moved to. + +### Fixed +- **A CalDAV event is moved or deleted even after the destination moves to Google or + Microsoft.** The event was handed to the new destination's provider, which could not + find an id it never issued, so the event stayed on the CalDAV calendar at its old time, + or after its booking was cancelled. A CalDAV event id is the event's URL, which is now + enough to route it back to the CalDAV provider whatever the destination is. + ## [0.9.0] - 2026-09-10 ### Added diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e91d5db6..f56010b5 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -543,8 +543,18 @@ from the current destination. otherwise changing the destination orphans every existing booking: the provider 404s, the booking cancels in Calnode, and the meeting stays on the host's calendar with nothing surfaced. Empty means "resolve the old way", correct for bookings that predate the column. -Known limit: this rescues a change of calendar *within* an account, not a move to a -different account, which would need the account recorded too. +Known limit (Google, Microsoft): this rescues a change of calendar *within* an account, not +a move to a different account, which would need the account recorded too. + +CalDAV does not have that limit, because its event id is the event's absolute URL and so +already names the server. `Service.UpdateEvent`/`CancelEvent` route an id the CalDAV +provider recognizes (`calendar.EventRecognizer`) to it whatever the destination is now, and +it authenticates as the connected account that holds the event (`caldav.eventConn`), never +as the destination: the account whose bound or saved calendar is the recorded calendar id, +else whose calendar URL contains the event URL (same scheme, host and port, path under it; +most specific wins). No match, or a tie, sends nothing and returns an error. This is a +credential boundary, not a routing nicety: accounts can live on different servers, and the +destination's app password sent to an older event's URL goes to someone else's server. --- diff --git a/internal/caldav/events.go b/internal/caldav/events.go index 3de088b3..f0ffa1af 100644 --- a/internal/caldav/events.go +++ b/internal/caldav/events.go @@ -2,9 +2,11 @@ package caldav import ( "context" + "errors" "fmt" "io" "net/http" + "net/url" "strings" "time" @@ -41,8 +43,12 @@ func (c *Client) CreateEvent(ctx context.Context, userID string, p calendar.Crea // UpdateEvent moves an existing event to new start/end times. CalDAV has no partial update, so // it GETs the current object, rewrites DTSTART/DTEND (and bumps SEQUENCE/DTSTAMP), and PUTs it // back — preserving summary, description, location, and attendees. +// +// It authenticates as the account that holds the event (eventConn), not as the current +// destination, and returns an error without sending anything when that account cannot be +// established. func (c *Client) UpdateEvent(ctx context.Context, userID, calendarID, eventID string, start, end time.Time) error { - cn, ok, err := c.loadConn(ctx, userID, -1, 1) + cn, ok, err := c.eventConn(ctx, userID, calendarID, eventID) if err != nil || !ok { return err } @@ -67,9 +73,10 @@ func (c *Client) UpdateEvent(ctx context.Context, userID, calendarID, eventID st return nil } -// CancelEvent deletes the event resource from the destination calendar. +// CancelEvent deletes the event resource from the calendar it was written to, as the account +// that holds it (see UpdateEvent). func (c *Client) CancelEvent(ctx context.Context, userID, calendarID, eventID string) error { - cn, ok, err := c.loadConn(ctx, userID, -1, 1) + cn, ok, err := c.eventConn(ctx, userID, calendarID, eventID) if err != nil || !ok { return err } @@ -83,6 +90,184 @@ func (c *Client) CancelEvent(ctx context.Context, userID, calendarID, eventID st return nil } +// RecognizesEvent reports whether eventID is a CalDAV event id: the absolute http(s) URL of +// the event resource, which is what CreateEvent returns. Google and Microsoft ids are opaque +// tokens and never URLs, so calendar.Service uses this to send an update or cancel here even +// after the user's destination has moved to another provider. +func (c *Client) RecognizesEvent(eventID string) bool { + _, ok := parseDAVURL(eventID) + return ok +} + +var ( + errEventNotURL = errors.New("caldav: event id is not a CalDAV resource URL; nothing was sent") + errNoEventOwner = errors.New("caldav: none of the user's CalDAV accounts holds this event (it is not inside any of their calendars); nothing was sent") + errManyEventOwner = errors.New("caldav: more than one of the user's CalDAV accounts could hold this event; nothing was sent") +) + +// eventAccount is one connected CalDAV account and every collection URL it is known to use: the +// calendar bound at connect, then any calendars saved for the account in connection_calendars. +type eventAccount struct { + id, username, pwEnc string + collections []string +} + +// eventConn resolves the connection an update or cancel of eventID must authenticate as: the +// account that holds the event, never whichever account is the destination now. A host can +// connect several CalDAV accounts on different servers and move the destination between them, +// and an event's URL names the server it lives on, so using the destination's credentials sends +// that account's password to another account's server. +// +// ok=false with a nil error means the user has no CalDAV connection at all, so there is no +// account to act as (the Provider contract's "no matching connection"). Otherwise it returns an +// error, having sent nothing, unless pickEventOwner finds exactly one account. +func (c *Client) eventConn(ctx context.Context, userID, calendarID, eventID string) (conn, bool, error) { + var accounts []eventAccount + rows, err := c.db.QueryContext(ctx, ` + SELECT id, COALESCE(account_email,''), access_token_enc, calendar_id + FROM calendar_connections + WHERE user_id = ? AND provider = 'caldav'`, userID) + if err != nil { + return conn{}, false, fmt.Errorf("caldav: load connections: %w", err) + } + for rows.Next() { + var a eventAccount + var bound string + if err := rows.Scan(&a.id, &a.username, &a.pwEnc, &bound); err != nil { + rows.Close() // #nosec G104 -- already returning the scan error; nothing more actionable + return conn{}, false, fmt.Errorf("caldav: scan connection: %w", err) + } + a.collections = []string{bound} + accounts = append(accounts, a) + } + rows.Close() // #nosec G104 -- rows already fully consumed above; nothing actionable on close error + if err := rows.Err(); err != nil { + return conn{}, false, err + } + if len(accounts) == 0 { + c.logger.Warn("caldav: no CalDAV account is connected to move or cancel an event; it stays on its calendar", + "user_id", userID) + return conn{}, false, nil + } + + // A second query, not a join, and only after the cursor above is closed: the pool is a + // single connection. + rows, err = c.db.QueryContext(ctx, ` + SELECT account_email, calendar_id FROM connection_calendars + WHERE user_id = ? AND provider = 'caldav'`, userID) + if err != nil { + return conn{}, false, fmt.Errorf("caldav: load saved calendars: %w", err) + } + for rows.Next() { + var email, calID string + if err := rows.Scan(&email, &calID); err != nil { + rows.Close() // #nosec G104 -- already returning the scan error; nothing more actionable + return conn{}, false, fmt.Errorf("caldav: scan saved calendar: %w", err) + } + for i := range accounts { + if accounts[i].username == email { + accounts[i].collections = append(accounts[i].collections, calID) + } + } + } + rows.Close() // #nosec G104 -- rows already fully consumed above; nothing actionable on close error + if err := rows.Err(); err != nil { + return conn{}, false, err + } + + i, err := pickEventOwner(accounts, calendarID, eventID) + if err != nil { + return conn{}, false, err + } + a := accounts[i] + pw, err := c.decrypt(a.pwEnc) + if err != nil { + return conn{}, false, fmt.Errorf("caldav: decrypt password: %w", err) + } + return conn{id: a.id, username: a.username, password: string(pw)}, true, nil +} + +// pickEventOwner returns the index of the one account that holds eventID. +// +// An account can hold the event only if the event URL lies inside one of its collections: same +// scheme, host and port, and a path under the collection's at a segment boundary. That is what +// guarantees an account's credentials only go to a server that account is already configured to +// use. Among the accounts that pass, one whose collection is the calendar id recorded at creation +// wins; failing that (no id recorded, as for reassign and bookings that predate the column, or a +// calendar no longer saved) the account with the most specific collection does. A tie is refused +// rather than guessed. +func pickEventOwner(accounts []eventAccount, calendarID, eventID string) (int, error) { + ev, ok := parseDAVURL(eventID) + if !ok { + return -1, errEventNotURL + } + cal, calOK := parseDAVURL(calendarID) + + best, tie := -1, false + var bestExact bool + bestDepth := -1 + for i, a := range accounts { + held, exact, depth := false, false, -1 + for _, raw := range a.collections { + col, ok := parseDAVURL(raw) + if !ok || !col.contains(ev) { + continue + } + held = true + exact = exact || (calOK && col.sameCollection(cal)) + depth = max(depth, len(strings.TrimRight(col.path, "/"))) + } + switch { + case !held: + case best < 0 || (exact && !bestExact) || (exact == bestExact && depth > bestDepth): + best, bestExact, bestDepth, tie = i, exact, depth, false + case exact == bestExact && depth == bestDepth: + tie = true + } + } + switch { + case best < 0: + return -1, errNoEventOwner + case tie: + return -1, errManyEventOwner + } + return best, nil +} + +// davURL is the part of a CalDAV URL that decides where a request, and its credentials, go. +type davURL struct { + scheme, host, port, path string +} + +// parseDAVURL accepts only an absolute http(s) URL with a host. The port is made explicit and the +// host lower-cased, so equivalent spellings of one server compare equal. The path is kept in its +// escaped form, the form the event URL was built from. +func parseDAVURL(raw string) (davURL, bool) { + u, err := url.Parse(raw) + if err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Hostname() == "" { + return davURL{}, false + } + port := u.Port() + if port == "" { + port = map[string]string{"http": "80", "https": "443"}[u.Scheme] + } + return davURL{scheme: u.Scheme, host: strings.ToLower(u.Hostname()), port: port, path: u.EscapedPath()}, true +} + +func (d davURL) sameOrigin(o davURL) bool { + return d.scheme == o.scheme && d.host == o.host && d.port == o.port +} + +// contains reports whether resource lies inside the collection d. +func (d davURL) contains(resource davURL) bool { + dir := strings.TrimRight(d.path, "/") + "/" + return d.sameOrigin(resource) && len(resource.path) > len(dir) && strings.HasPrefix(resource.path, dir) +} + +func (d davURL) sameCollection(o davURL) bool { + return d.sameOrigin(o) && strings.TrimRight(d.path, "/") == strings.TrimRight(o.path, "/") +} + // getICS fetches a calendar resource, returning its body, ETag, and HTTP status. func (c *Client) getICS(ctx context.Context, rawURL, username, password string) (body, etag string, status int, err error) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil) diff --git a/internal/caldav/events_test.go b/internal/caldav/events_test.go new file mode 100644 index 00000000..c4244d72 --- /dev/null +++ b/internal/caldav/events_test.go @@ -0,0 +1,593 @@ +package caldav + +import ( + "context" + "errors" + "io" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "sync" + "testing" + "time" + + "github.com/calnode/calnode/internal/calendar" + "github.com/calnode/calnode/internal/slots" +) + +// davReq is one request a davServer received, with the credentials it carried. +type davReq struct { + Method, Path, User, Pass, IfMatch string +} + +// davServer is a CalDAV server that holds the events of exactly one account. It records every +// request together with the Basic credentials it carried, so a test can assert which account's +// password reached which server, and it refuses any other credentials the way a real server does. +type davServer struct { + *httptest.Server + user, pass string + + mu sync.Mutex + got []davReq +} + +func newDAVServer(t *testing.T, user, pass string) *davServer { + t.Helper() + s := &davServer{user: user, pass: pass} + s.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + u, p, _ := r.BasicAuth() + s.mu.Lock() + s.got = append(s.got, davReq{Method: r.Method, Path: r.URL.Path, User: u, Pass: p, IfMatch: r.Header.Get("If-Match")}) + s.mu.Unlock() + if u != s.user || p != s.pass { + w.WriteHeader(http.StatusUnauthorized) + return + } + switch r.Method { + case http.MethodGet: + w.Header().Set("ETag", `"v1"`) + io.WriteString(w, "BEGIN:VCALENDAR\r\nBEGIN:VEVENT\r\nUID:x@calnode\r\n"+ + "DTSTART:20260701T090000Z\r\nDTEND:20260701T100000Z\r\nSEQUENCE:0\r\nEND:VEVENT\r\nEND:VCALENDAR\r\n") + case http.MethodPut: + w.WriteHeader(http.StatusCreated) + case http.MethodDelete: + w.WriteHeader(http.StatusNoContent) + default: + w.WriteHeader(http.StatusMethodNotAllowed) + } + })) + t.Cleanup(s.Close) + return s +} + +func (s *davServer) requests() []davReq { + s.mu.Lock() + defer s.mu.Unlock() + return append([]davReq(nil), s.got...) +} + +func (s *davServer) reset() { + s.mu.Lock() + defer s.mu.Unlock() + s.got = nil +} + +// onlyOwnCredentials fails the test if any request to s carried credentials other than the +// account s belongs to. +func (s *davServer) onlyOwnCredentials(t *testing.T, name string) { + t.Helper() + for _, r := range s.requests() { + if r.User != s.user || r.Pass != s.pass { + t.Errorf("server %s received %s %s as %q with password %q; only %q's credentials belong there", + name, r.Method, r.Path, r.User, r.Pass, s.user) + } + } +} + +func (s *davServer) noRequests(t *testing.T, name string) { + t.Helper() + if got := s.requests(); len(got) != 0 { + t.Errorf("server %s received %d request(s), want none: %+v", name, len(got), got) + } +} + +var ( + moveStart = time.Date(2026, 7, 2, 14, 0, 0, 0, time.UTC) + moveEnd = moveStart.Add(time.Hour) +) + +func bookingParams() calendar.CreateEventParams { + return calendar.CreateEventParams{ + Summary: "Intro call", Start: time.Date(2026, 7, 1, 9, 0, 0, 0, time.UTC), + End: time.Date(2026, 7, 1, 10, 0, 0, 0, time.UTC), OrganizerEmail: "booker@x.test", + } +} + +// createOn writes a booking's event while accountEmail is the destination, exactly as the +// booking flow does, and returns the ids the flow stores on booking_hosts. +func createOn(t *testing.T, svc *calendar.Service, accountEmail string) (eventID, calendarID string) { + t.Helper() + ctx := context.Background() + if err := svc.SetDestination(ctx, "u1", "caldav", accountEmail); err != nil { + t.Fatalf("set destination %s: %v", accountEmail, err) + } + eventID, _, calendarID, err := svc.CreateEvent(ctx, "u1", bookingParams()) + if err != nil || eventID == "" { + t.Fatalf("CreateEvent on %s: id=%q err=%v", accountEmail, eventID, err) + } + return eventID, calendarID +} + +func newSvc(c *Client) *calendar.Service { + svc := calendar.NewService(c.db) + svc.Register(c) + return svc +} + +// A host books on CalDAV account A, then makes account B (another server) the destination. +// Moving or cancelling the old booking must authenticate to A's server as A. Before the fix +// both operations sent B's password to A's server, which refused it. +func TestUpdateCancel_eventOnPreviousAccountUsesThatAccount(t *testing.T) { + c := newTestClient(t) + ctx := context.Background() + seedUser(t, c.db, "u1") + srvA := newDAVServer(t, "a@a.test", "pw-a") + srvB := newDAVServer(t, "b@b.test", "pw-b") + if err := c.saveConnection(ctx, "u1", "a@a.test", "pw-a", srvA.URL+"/calendars/a/home/"); err != nil { + t.Fatal(err) + } + if err := c.saveConnection(ctx, "u1", "b@b.test", "pw-b", srvB.URL+"/calendars/b/home/"); err != nil { + t.Fatal(err) + } + svc := newSvc(c) + eventID, calendarID := createOn(t, svc, "a@a.test") + if err := svc.SetDestination(ctx, "u1", "caldav", "b@b.test"); err != nil { + t.Fatal(err) + } + srvA.reset() + + if err := svc.UpdateEvent(ctx, "u1", calendarID, eventID, moveStart, moveEnd); err != nil { + t.Errorf("UpdateEvent: %v", err) + } + if err := svc.CancelEvent(ctx, "u1", calendarID, eventID); err != nil { + t.Errorf("CancelEvent: %v", err) + } + + srvA.onlyOwnCredentials(t, "A") + srvB.noRequests(t, "B") + var methods []string + for _, r := range srvA.requests() { + methods = append(methods, r.Method) + } + if want := []string{"GET", "PUT", "DELETE"}; !reflect.DeepEqual(methods, want) { + t.Errorf("server A saw %v, want %v", methods, want) + } +} + +// Reassign cancels with an empty calendar id (it does not load the stored one), and so do +// bookings made before external_calendar_id existed. The owner is then found from the event +// URL alone. +func TestCancel_emptyCalendarIDResolvesOwnerByURL(t *testing.T) { + c := newTestClient(t) + ctx := context.Background() + seedUser(t, c.db, "u1") + srvA := newDAVServer(t, "a@a.test", "pw-a") + srvB := newDAVServer(t, "b@b.test", "pw-b") + if err := c.saveConnection(ctx, "u1", "a@a.test", "pw-a", srvA.URL+"/calendars/a/home/"); err != nil { + t.Fatal(err) + } + if err := c.saveConnection(ctx, "u1", "b@b.test", "pw-b", srvB.URL+"/calendars/b/home/"); err != nil { + t.Fatal(err) + } + svc := newSvc(c) + eventID, _ := createOn(t, svc, "a@a.test") + if err := svc.SetDestination(ctx, "u1", "caldav", "b@b.test"); err != nil { + t.Fatal(err) + } + srvA.reset() + + if err := svc.CancelEvent(ctx, "u1", "", eventID); err != nil { + t.Errorf("CancelEvent with no calendar id: %v", err) + } + if err := svc.UpdateEvent(ctx, "u1", "", eventID, moveStart, moveEnd); err != nil { + t.Errorf("UpdateEvent with no calendar id: %v", err) + } + srvA.onlyOwnCredentials(t, "A") + srvB.noRequests(t, "B") + if n := len(srvA.requests()); n != 3 { + t.Errorf("server A saw %d requests, want 3 (DELETE, GET, PUT): %+v", n, srvA.requests()) + } +} + +// An event written into a calendar picked inside the account (connection_calendars), not the +// account's bound collection, is still found: by its recorded calendar id, and by URL when no +// calendar id was recorded. +func TestUpdate_eventInPickedCalendarResolvesToItsAccount(t *testing.T) { + c := newTestClient(t) + ctx := context.Background() + seedUser(t, c.db, "u1") + srvA := newDAVServer(t, "a@a.test", "pw-a") + srvB := newDAVServer(t, "b@b.test", "pw-b") + if err := c.saveConnection(ctx, "u1", "a@a.test", "pw-a", srvA.URL+"/calendars/a/home/"); err != nil { + t.Fatal(err) + } + if err := c.saveConnection(ctx, "u1", "b@b.test", "pw-b", srvB.URL+"/calendars/b/home/"); err != nil { + t.Fatal(err) + } + svc := newSvc(c) + work := srvA.URL + "/calendars/a/work/" + if err := svc.SetAccountCalendars(ctx, "u1", "caldav", "a@a.test", []calendar.CalendarSelection{ + {CalendarInfo: calendar.CalendarInfo{ID: srvA.URL + "/calendars/a/home/"}, CheckConflicts: true}, + {CalendarInfo: calendar.CalendarInfo{ID: work}, CheckConflicts: true, IsDestination: true}, + }); err != nil { + t.Fatal(err) + } + eventID, _, calendarID, err := svc.CreateEvent(ctx, "u1", bookingParams()) + if err != nil || calendarID != work || !strings.HasPrefix(eventID, work) { + t.Fatalf("CreateEvent: id=%q cal=%q err=%v, want an event in %s", eventID, calendarID, err, work) + } + if err := svc.SetDestination(ctx, "u1", "caldav", "b@b.test"); err != nil { + t.Fatal(err) + } + srvA.reset() + + for _, calID := range []string{calendarID, ""} { + if err := svc.UpdateEvent(ctx, "u1", calID, eventID, moveStart, moveEnd); err != nil { + t.Errorf("UpdateEvent(calendarID=%q): %v", calID, err) + } + } + srvA.onlyOwnCredentials(t, "A") + srvB.noRequests(t, "B") + if n := len(srvA.requests()); n != 4 { + t.Errorf("server A saw %d requests, want 4 (two GET+PUT pairs): %+v", n, srvA.requests()) + } +} + +// otherProvider stands in for Google (the provider column only admits real provider names) as +// the user's destination. Like the real one, it cannot find an event id it never issued; it +// records every id it is asked about. +type otherProvider struct { + mu sync.Mutex + touched []string +} + +func (p *otherProvider) record(eventID string) error { + p.mu.Lock() + defer p.mu.Unlock() + p.touched = append(p.touched, eventID) + return errors.New("other: event not found") +} + +func (p *otherProvider) Name() string { return "google" } +func (p *otherProvider) InvitesGuests() bool { return true } +func (p *otherProvider) AuthURL(string) string { return "" } +func (p *otherProvider) EncryptState(string) (string, error) { return "", nil } +func (p *otherProvider) DecryptState(string) (string, error) { return "", nil } +func (p *otherProvider) Exchange(context.Context, string, string, string) error { return nil } +func (p *otherProvider) Connected(context.Context, string) (bool, error) { return true, nil } +func (p *otherProvider) Disconnect(context.Context, string) error { return nil } +func (p *otherProvider) HasDestination(context.Context, string) (bool, error) { return true, nil } +func (p *otherProvider) ListCalendars(context.Context, string, string) ([]calendar.CalendarInfo, error) { + return nil, nil +} +func (p *otherProvider) FreeBusy(context.Context, string, time.Time, time.Time) ([]slots.Interval, error) { + return nil, nil +} +func (p *otherProvider) CreateEvent(context.Context, string, calendar.CreateEventParams) (string, string, string, error) { + return "other-event-1", "", "primary", nil +} +func (p *otherProvider) UpdateEvent(_ context.Context, _, _, eventID string, _, _ time.Time) error { + return p.record(eventID) +} +func (p *otherProvider) CancelEvent(_ context.Context, _, _, eventID string) error { + return p.record(eventID) +} + +func seedOtherDestination(t *testing.T, c *Client, svc *calendar.Service) { + t.Helper() + ctx := context.Background() + if _, err := c.db.ExecContext(ctx, ` + INSERT INTO calendar_connections + (id, user_id, provider, account_email, access_token_enc, calendar_id, + check_conflicts, is_destination, created_at) + VALUES ('google-1', 'u1', 'google', 'o@o.test', 'enc', 'primary', 1, 0, '2026-01-03T00:00:00Z')`); err != nil { + t.Fatal(err) + } + if err := svc.SetDestination(ctx, "u1", "google", "o@o.test"); err != nil { + t.Fatal(err) + } +} + +// The destination moves from a CalDAV account to another provider. The CalDAV event must +// still be moved and deleted on its own server, as its own account, and the other provider +// must never be handed a CalDAV event URL. Before the fix the Service routed both calls to +// the new destination's provider, so the event on the CalDAV server was never touched. +func TestUpdateCancel_destinationMovedToAnotherProvider(t *testing.T) { + c := newTestClient(t) + ctx := context.Background() + seedUser(t, c.db, "u1") + srvA := newDAVServer(t, "a@a.test", "pw-a") + if err := c.saveConnection(ctx, "u1", "a@a.test", "pw-a", srvA.URL+"/calendars/a/home/"); err != nil { + t.Fatal(err) + } + other := &otherProvider{} + svc := newSvc(c) + svc.Register(other) + eventID, calendarID := createOn(t, svc, "a@a.test") + seedOtherDestination(t, c, svc) + srvA.reset() + + if err := svc.UpdateEvent(ctx, "u1", calendarID, eventID, moveStart, moveEnd); err != nil { + t.Errorf("UpdateEvent: %v", err) + } + if err := svc.CancelEvent(ctx, "u1", calendarID, eventID); err != nil { + t.Errorf("CancelEvent: %v", err) + } + if len(other.touched) != 0 { + t.Errorf("the other provider was asked about CalDAV event(s) %v", other.touched) + } + srvA.onlyOwnCredentials(t, "A") + if n := len(srvA.requests()); n != 3 { + t.Errorf("server A saw %d requests, want 3 (GET, PUT, DELETE): %+v", n, srvA.requests()) + } + + // The other provider's own events still route to it, by destination, as before. + if err := svc.CancelEvent(ctx, "u1", "primary", "other-event-1"); err == nil { + t.Error("CancelEvent of the other provider's event: want its error, got nil") + } + if !reflect.DeepEqual(other.touched, []string{"other-event-1"}) { + t.Errorf("other provider touched %v, want [other-event-1]", other.touched) + } +} + +// The CalDAV account that held the event is gone. There are no credentials left that belong +// to its server, so nothing is sent anywhere, and nothing is retried against a provider that +// never issued the id. +func TestUpdateCancel_owningAccountDisconnectedSendsNothing(t *testing.T) { + c := newTestClient(t) + ctx := context.Background() + seedUser(t, c.db, "u1") + srvA := newDAVServer(t, "a@a.test", "pw-a") + if err := c.saveConnection(ctx, "u1", "a@a.test", "pw-a", srvA.URL+"/calendars/a/home/"); err != nil { + t.Fatal(err) + } + other := &otherProvider{} + svc := newSvc(c) + svc.Register(other) + eventID, calendarID := createOn(t, svc, "a@a.test") + seedOtherDestination(t, c, svc) + if err := svc.DisconnectOne(ctx, "u1", "caldav", "a@a.test"); err != nil { + t.Fatal(err) + } + srvA.reset() + + if err := svc.UpdateEvent(ctx, "u1", calendarID, eventID, moveStart, moveEnd); err != nil { + t.Errorf("UpdateEvent with no CalDAV account left: %v, want nil (no connection to act as)", err) + } + if err := svc.CancelEvent(ctx, "u1", calendarID, eventID); err != nil { + t.Errorf("CancelEvent with no CalDAV account left: %v, want nil (no connection to act as)", err) + } + srvA.noRequests(t, "A") + if len(other.touched) != 0 { + t.Errorf("the other provider was asked about CalDAV event(s) %v", other.touched) + } +} + +// When the stored ids do not identify exactly one of the user's CalDAV accounts, nothing is +// sent: not to the event's server, and not with the destination's credentials. +func TestUpdateCancel_ambiguousOrUnknownOwnerSendsNothing(t *testing.T) { + type setup struct { + c *Client + srvA, srvB *davServer + stranger *davServer // a server none of the user's connections uses + } + newSetup := func(t *testing.T) setup { + c := newTestClient(t) + seedUser(t, c.db, "u1") + s := setup{ + c: c, + srvA: newDAVServer(t, "a@a.test", "pw-a"), + srvB: newDAVServer(t, "b@b.test", "pw-b"), + stranger: newDAVServer(t, "a@a.test", "pw-a"), + } + ctx := context.Background() + if err := c.saveConnection(ctx, "u1", "a@a.test", "pw-a", s.srvA.URL+"/calendars/a/home/"); err != nil { + t.Fatal(err) + } + if err := c.saveConnection(ctx, "u1", "b@b.test", "pw-b", s.srvB.URL+"/calendars/b/home"); err != nil { + t.Fatal(err) + } + return s + } + + cases := []struct { + name string + extra func(t *testing.T, s setup) // optional additional state + ids func(s setup) (calendarID, eventID string) + }{ + { + name: "event on a server no connection uses", + ids: func(s setup) (string, string) { + return "", s.stranger.URL + "/calendars/a/home/ev1.ics" + }, + }, + { + name: "calendar id names an account but the event is on another server", + ids: func(s setup) (string, string) { + return s.srvB.URL + "/calendars/b/home", s.stranger.URL + "/calendars/b/home/ev1.ics" + }, + }, + { + name: "event on a connection's server but outside its collection", + ids: func(s setup) (string, string) { + return "", s.srvA.URL + "/calendars/someone-else/home/ev1.ics" + }, + }, + { + name: "collection path is a prefix only up to a segment boundary", + ids: func(s setup) (string, string) { + return "", s.srvB.URL + "/calendars/b/homework/ev1.ics" + }, + }, + { + name: "event id is not a URL at all (another provider's id)", + ids: func(s setup) (string, string) { + return "primary", "abc123def456" + }, + }, + { + name: "two accounts bound to the same collection", + extra: func(t *testing.T, s setup) { + if err := s.c.saveConnection(context.Background(), "u1", "delegate@a.test", "pw-d", s.srvA.URL+"/calendars/a/home/"); err != nil { + t.Fatal(err) + } + }, + ids: func(s setup) (string, string) { + return s.srvA.URL + "/calendars/a/home/", s.srvA.URL + "/calendars/a/home/ev1.ics" + }, + }, + { + name: "two accounts bound to the same collection, no calendar id", + extra: func(t *testing.T, s setup) { + if err := s.c.saveConnection(context.Background(), "u1", "delegate@a.test", "pw-d", s.srvA.URL+"/calendars/a/home/"); err != nil { + t.Fatal(err) + } + }, + ids: func(s setup) (string, string) { + return "", s.srvA.URL + "/calendars/a/home/ev1.ics" + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := newSetup(t) + if tc.extra != nil { + tc.extra(t, s) + } + ctx := context.Background() + svc := newSvc(s.c) + calendarID, eventID := tc.ids(s) + if err := svc.UpdateEvent(ctx, "u1", calendarID, eventID, moveStart, moveEnd); err == nil { + t.Error("UpdateEvent: want an error when the owning account cannot be established, got nil") + } + if err := svc.CancelEvent(ctx, "u1", calendarID, eventID); err == nil { + t.Error("CancelEvent: want an error when the owning account cannot be established, got nil") + } + s.srvA.noRequests(t, "A") + s.srvB.noRequests(t, "B") + s.stranger.noRequests(t, "stranger") + }) + } +} + +// The common case, one CalDAV account that is still the destination, sends exactly the +// requests it always did, whether or not a calendar id was recorded. +func TestUpdateCancel_singleAccountUnchanged(t *testing.T) { + c := newTestClient(t) + ctx := context.Background() + seedUser(t, c.db, "u1") + srvA := newDAVServer(t, "a@a.test", "pw-a") + if err := c.saveConnection(ctx, "u1", "a@a.test", "pw-a", srvA.URL+"/calendars/a/home/"); err != nil { + t.Fatal(err) + } + svc := newSvc(c) + eventID, calendarID := createOn(t, svc, "a@a.test") + path := strings.TrimPrefix(eventID, srvA.URL) + srvA.reset() + + if err := svc.UpdateEvent(ctx, "u1", calendarID, eventID, moveStart, moveEnd); err != nil { + t.Errorf("UpdateEvent: %v", err) + } + if err := svc.UpdateEvent(ctx, "u1", "", eventID, moveStart, moveEnd); err != nil { + t.Errorf("UpdateEvent (no calendar id): %v", err) + } + if err := svc.CancelEvent(ctx, "u1", calendarID, eventID); err != nil { + t.Errorf("CancelEvent: %v", err) + } + if err := svc.CancelEvent(ctx, "u1", "", eventID); err != nil { + t.Errorf("CancelEvent (no calendar id): %v", err) + } + a := func(method, ifMatch string) davReq { + return davReq{Method: method, Path: path, User: "a@a.test", Pass: "pw-a", IfMatch: ifMatch} + } + want := []davReq{ + a("GET", ""), a("PUT", `"v1"`), + a("GET", ""), a("PUT", `"v1"`), + a("DELETE", ""), + a("DELETE", ""), + } + if got := srvA.requests(); !reflect.DeepEqual(got, want) { + t.Errorf("requests:\n got %+v\nwant %+v", got, want) + } +} + +// pickEventOwner decides which account's credentials an event URL may receive, so the origin and +// path rules are pinned directly, including the ones an httptest server cannot exercise (a scheme +// or default-port change needs a second listener on the same address). +func TestPickEventOwner(t *testing.T) { + acct := func(collections ...string) eventAccount { return eventAccount{collections: collections} } + cases := []struct { + name string + accounts []eventAccount + calendarID string + eventID string + want int + wantErr error + }{ + {"inside the bound collection", []eventAccount{acct("https://a.test/cal/a/"), acct("https://b.test/cal/b/")}, + "", "https://a.test/cal/a/e.ics", 0, nil}, + {"inside a saved calendar", []eventAccount{acct("https://a.test/cal/a/"), acct("https://b.test/cal/b/", "https://b.test/cal/b-work/")}, + "https://b.test/cal/b-work/", "https://b.test/cal/b-work/e.ics", 1, nil}, + {"collection stored without a trailing slash", []eventAccount{acct("https://a.test/cal/a")}, + "https://a.test/cal/a/", "https://a.test/cal/a/e.ics", 0, nil}, + {"host case and an explicit default port are the same origin", []eventAccount{acct("https://A.test:443/cal/a/")}, + "", "https://a.TEST/cal/a/e.ics", 0, nil}, + {"http and https are different origins", []eventAccount{acct("https://a.test/cal/a/")}, + "", "http://a.test/cal/a/e.ics", -1, errNoEventOwner}, + {"a non-default port is a different origin", []eventAccount{acct("https://a.test/cal/a/")}, + "", "https://a.test:8443/cal/a/e.ics", -1, errNoEventOwner}, + {"a sibling path is not inside", []eventAccount{acct("https://a.test/cal/a/")}, + "", "https://a.test/cal/ab/e.ics", -1, errNoEventOwner}, + {"the collection itself is not an event inside it", []eventAccount{acct("https://a.test/cal/a/")}, + "", "https://a.test/cal/a/", -1, errNoEventOwner}, + {"the more specific collection wins", []eventAccount{acct("https://a.test/dav/"), acct("https://a.test/dav/b/home/")}, + "", "https://a.test/dav/b/home/e.ics", 1, nil}, + {"one calendar recorded in two accounts is a tie", []eventAccount{acct("https://a.test/dav/b/home/"), acct("https://a.test/dav/b/home/")}, + "https://a.test/dav/b/home/", "https://a.test/dav/b/home/e.ics", -1, errManyEventOwner}, + {"a recorded calendar id outranks a more specific collection", []eventAccount{acct("https://a.test/dav/"), acct("https://a.test/dav/x/")}, + "https://a.test/dav/", "https://a.test/dav/x/e.ics", 0, nil}, + {"equally specific collections on one server tie", []eventAccount{acct("https://a.test/cal/shared/"), acct("https://a.test/cal/shared")}, + "", "https://a.test/cal/shared/e.ics", -1, errManyEventOwner}, + {"a calendar id outside the event's origin does not count", []eventAccount{acct("https://a.test/cal/a/"), acct("https://b.test/cal/a/")}, + "https://b.test/cal/a/", "https://a.test/cal/a/e.ics", 0, nil}, + {"a Google event id", []eventAccount{acct("https://a.test/cal/a/")}, "primary", "7cbh8rpc10lrc0ckih9tafss99", -1, errEventNotURL}, + {"a Microsoft event id", []eventAccount{acct("https://a.test/cal/a/")}, "", "AAMkAGI2TG93AAA=", -1, errEventNotURL}, + {"no accounts", nil, "", "https://a.test/cal/a/e.ics", -1, errNoEventOwner}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := pickEventOwner(tc.accounts, tc.calendarID, tc.eventID) + if got != tc.want || !errors.Is(err, tc.wantErr) { + t.Errorf("pickEventOwner = (%d, %v), want (%d, %v)", got, err, tc.want, tc.wantErr) + } + }) + } +} + +func TestRecognizesEvent(t *testing.T) { + c := newTestClient(t) + for id, want := range map[string]bool{ + "https://caldav.icloud.com/123/calendars/home/e.ics": true, + "http://nextcloud.lan/remote.php/dav/calendars/u/personal/e.ics": true, + "7cbh8rpc10lrc0ckih9tafss99": false, // Google + "AAMkAGI2TG93AAA=": false, // Microsoft + "": false, + "/calendars/home/e.ics": false, + "mailto:someone@x.test": false, + } { + if got := c.RecognizesEvent(id); got != want { + t.Errorf("RecognizesEvent(%q) = %v, want %v", id, got, want) + } + } +} diff --git a/internal/calendar/calendar.go b/internal/calendar/calendar.go index afaade9c..eda1b5ad 100644 --- a/internal/calendar/calendar.go +++ b/internal/calendar/calendar.go @@ -79,6 +79,18 @@ type Provider interface { CancelEvent(ctx context.Context, userID, calendarID, eventID string) error } +// EventRecognizer is implemented by a provider whose event ids identify the provider by +// themselves. The Service sends an update or cancel of such an event to that provider whatever +// the user's destination is now, so moving the destination to another provider neither strands +// the events written before the move nor hands their ids to a provider that never issued them. +// +// CalDAV implements it: its event id is the absolute URL of the event resource. Google and +// Microsoft ids are opaque and never URLs; they do not implement it and keep routing by +// destination. +type EventRecognizer interface { + RecognizesEvent(eventID string) bool +} + // Service holds the configured providers and dispatches per-user operations to // whichever provider that user has connected. type Service struct { @@ -384,19 +396,31 @@ func (s *Service) CreateEvent(ctx context.Context, userID string, p CreateEventP return "", "", "", nil } +// providerForEvent resolves the provider an existing event belongs to: the one that recognizes +// its id (EventRecognizer), else the user's destination provider. Returns nil if neither. +func (s *Service) providerForEvent(ctx context.Context, userID, eventID string) Provider { + for _, name := range s.ProviderNames() { + if r, ok := s.providers[name].(EventRecognizer); ok && r.RecognizesEvent(eventID) { + return s.providers[name] + } + } + return s.providerForDestination(ctx, userID) +} + // UpdateEvent moves an event. calendarID is the one recorded at creation; empty falls back -// to the user's current destination. +// to the user's current destination. The provider is the one that recognizes eventID, if any +// does, else the destination's (providerForEvent). func (s *Service) UpdateEvent(ctx context.Context, userID, calendarID, eventID string, start, end time.Time) error { - if pr := s.providerForDestination(ctx, userID); pr != nil { + if pr := s.providerForEvent(ctx, userID, eventID); pr != nil { return pr.UpdateEvent(ctx, userID, calendarID, eventID, start, end) } return nil } // CancelEvent deletes an event. calendarID is the one recorded at creation; empty falls -// back to the user's current destination. +// back to the user's current destination. The provider is chosen as for UpdateEvent. func (s *Service) CancelEvent(ctx context.Context, userID, calendarID, eventID string) error { - if pr := s.providerForDestination(ctx, userID); pr != nil { + if pr := s.providerForEvent(ctx, userID, eventID); pr != nil { return pr.CancelEvent(ctx, userID, calendarID, eventID) } return nil diff --git a/internal/handler/reassign.go b/internal/handler/reassign.go index e3d58c1e..3c68d774 100644 --- a/internal/handler/reassign.go +++ b/internal/handler/reassign.go @@ -162,6 +162,8 @@ func (h *Handler) ReassignBooking(w http.ResponseWriter, r *http.Request) { // Reassignment cancels on the OLD host's calendar. Their stored calendar id is not // loaded here, so this falls back to resolving their destination - the same // behaviour as before, and correct unless they also moved their destination. + // A CalDAV event is the exception: its id is its URL, which is enough to find the + // account that holds it wherever the destination is now. if err := gc.CancelEvent(ctx, oldHostID, "", extEventID); err != nil { h.logger.Error("reassign: delete old calendar event", "error", err, "booking_id", bCopy.ID) } From 3c7e662fc938af02e5b07c5f4ac2e4b2a183d7b5 Mon Sep 17 00:00:00 2001 From: Distronode <254259913+distronode-com@users.noreply.github.com> Date: Wed, 16 Sep 2026 22:19:52 -0400 Subject: [PATCH 2/2] caldav: stop retrying an event no single account holds eventConn refuses to send anything when the stored ids do not pick out exactly one CalDAV account: no account holds the event URL, two tie, or the id is not a URL at all. The reconciler treated that refusal like any other failure, leaving needs_sync set (reschedules) or the event id in place (cancellations) and trying again next sweep. Nothing is sent now, but a retry re-reads the same connections and refuses the same way, so the loop only moved: an error line every two minutes until the booking ended, and forever for a cancellation, whose sweep has no date bound. The three refusals now match calendar.ErrEventUnreachable. On it the reconciler logs one warning and lets go: it clears needs_sync for a reschedule (the event keeps its old time and its id) and clears the id for a cancellation (the event stays on its calendar). The cancel warning names the event, since clearing the id drops the only record of it, and logs it redacted because a CalDAV id is a URL that can carry userinfo. A tie is treated the same. needs_sync is surfaced nowhere a person would see it, so leaving it set would reach nobody but the log, and only a change to the host's connections could break the tie. Every other error stays retryable, including a server that is down or refuses the stored password, since reconnecting or waiting fixes those. Tests: the reconciler, with a provider that claims the event, over two sweeps: an unreachable refusal is called once, clears the flag or id and warns exactly once; a transient error is called twice and keeps both. The warning does not log a password embedded in the event URL. On the CalDAV side, all seven no-owner and tie cases now assert the sentinel, and a refused password and an unreachable server assert that they do not match it. Restoring the old reconciler branch fails both reconciler tests; removing the sentinel match fails 14 CalDAV assertions. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 4 +- docs/ARCHITECTURE.md | 7 + internal/caldav/events.go | 16 +- internal/caldav/events_test.go | 50 +++- internal/calendar/calendar.go | 7 + internal/handler/calendar_reconcile.go | 29 ++- .../calendar_reconcile_internal_test.go | 224 ++++++++++++++++++ 7 files changed, 324 insertions(+), 13 deletions(-) create mode 100644 internal/handler/calendar_reconcile_internal_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 5abee573..2dd577d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,9 @@ exact tag (`ghcr.io/calnode/calnode:0.1.0`) if you need stability between upgrad Update and cancel now authenticate as the account that holds the event, found from what the booking stored: the calendar recorded at creation, or failing that the connected calendar whose URL contains the event's URL (same scheme, host and port). If no single - account can be established, nothing is sent and the error is logged. Hosts who moved a + account can be established, nothing is sent, and the reconciler logs one warning and stops + retrying that event rather than refusing it again every sweep; the event stays where it + is. Hosts who moved a CalDAV destination between accounts on different servers should consider rotating the app password of the account they moved to. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index d16328ed..4c41602f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -560,6 +560,13 @@ most specific wins). No match, or a tie, sends nothing and returns an error. Thi credential boundary, not a routing nicety: accounts can live on different servers, and the destination's app password sent to an older event's URL goes to someone else's server. +That refusal matches `calendar.ErrEventUnreachable`, and the reconciler treats it as final: +it logs one warning, clears `needs_sync` (reschedule) or the event id (cancel, logged +redacted since it is the only record), and does not retry. Retrying cannot help, because it +re-reads the same connections and refuses the same way, and the cancellation sweep has no +date bound, so it would repeat forever. Any other error, such as a server that is down or +refuses the stored password, stays retryable. + --- ## 11. Calendar reconciler (self-healing) diff --git a/internal/caldav/events.go b/internal/caldav/events.go index f0ffa1af..81afcb8e 100644 --- a/internal/caldav/events.go +++ b/internal/caldav/events.go @@ -2,7 +2,6 @@ package caldav import ( "context" - "errors" "fmt" "io" "net/http" @@ -100,11 +99,20 @@ func (c *Client) RecognizesEvent(eventID string) bool { } var ( - errEventNotURL = errors.New("caldav: event id is not a CalDAV resource URL; nothing was sent") - errNoEventOwner = errors.New("caldav: none of the user's CalDAV accounts holds this event (it is not inside any of their calendars); nothing was sent") - errManyEventOwner = errors.New("caldav: more than one of the user's CalDAV accounts could hold this event; nothing was sent") + errEventNotURL error = unreachableEventError("caldav: event id is not a CalDAV resource URL; nothing was sent") + errNoEventOwner error = unreachableEventError("caldav: none of the user's CalDAV accounts holds this event (it is not inside any of their calendars); nothing was sent") + errManyEventOwner error = unreachableEventError("caldav: more than one of the user's CalDAV accounts could hold this event; nothing was sent") ) +// unreachableEventError is an update or cancel refused before any request, because the stored +// ids do not pick out one account to send it as. It matches calendar.ErrEventUnreachable, which +// is what tells the reconciler that retrying cannot help. +type unreachableEventError string + +func (e unreachableEventError) Error() string { return string(e) } + +func (e unreachableEventError) Is(target error) bool { return target == calendar.ErrEventUnreachable } + // eventAccount is one connected CalDAV account and every collection URL it is known to use: the // calendar bound at connect, then any calendars saved for the account in connection_calendars. type eventAccount struct { diff --git a/internal/caldav/events_test.go b/internal/caldav/events_test.go index c4244d72..c57cfe09 100644 --- a/internal/caldav/events_test.go +++ b/internal/caldav/events_test.go @@ -468,11 +468,13 @@ func TestUpdateCancel_ambiguousOrUnknownOwnerSendsNothing(t *testing.T) { ctx := context.Background() svc := newSvc(s.c) calendarID, eventID := tc.ids(s) - if err := svc.UpdateEvent(ctx, "u1", calendarID, eventID, moveStart, moveEnd); err == nil { - t.Error("UpdateEvent: want an error when the owning account cannot be established, got nil") + // ErrEventUnreachable specifically, not just an error: it is what stops the + // reconciler retrying a refusal that no later sweep can change. + if err := svc.UpdateEvent(ctx, "u1", calendarID, eventID, moveStart, moveEnd); !errors.Is(err, calendar.ErrEventUnreachable) { + t.Errorf("UpdateEvent = %v, want calendar.ErrEventUnreachable when the owning account cannot be established", err) } - if err := svc.CancelEvent(ctx, "u1", calendarID, eventID); err == nil { - t.Error("CancelEvent: want an error when the owning account cannot be established, got nil") + if err := svc.CancelEvent(ctx, "u1", calendarID, eventID); !errors.Is(err, calendar.ErrEventUnreachable) { + t.Errorf("CancelEvent = %v, want calendar.ErrEventUnreachable when the owning account cannot be established", err) } s.srvA.noRequests(t, "A") s.srvB.noRequests(t, "B") @@ -481,6 +483,46 @@ func TestUpdateCancel_ambiguousOrUnknownOwnerSendsNothing(t *testing.T) { } } +// Once the owning account is established, a failure talking to its server is an ordinary error +// and must not read as ErrEventUnreachable: the server can come back, and the app password can +// be fixed by reconnecting, so the reconciler has to keep retrying these. +func TestUpdateCancel_serverFailureStaysRetryable(t *testing.T) { + cases := []struct { + name string + fail func(t *testing.T, c *Client, s *davServer) + }{ + {"server refuses the stored password", func(t *testing.T, c *Client, s *davServer) { + if err := c.saveConnection(context.Background(), "u1", "a@a.test", "revoked", s.URL+"/calendars/a/home/"); err != nil { + t.Fatal(err) + } + }}, + {"server is unreachable", func(_ *testing.T, _ *Client, s *davServer) { s.Close() }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := newTestClient(t) + ctx := context.Background() + seedUser(t, c.db, "u1") + srvA := newDAVServer(t, "a@a.test", "pw-a") + if err := c.saveConnection(ctx, "u1", "a@a.test", "pw-a", srvA.URL+"/calendars/a/home/"); err != nil { + t.Fatal(err) + } + svc := newSvc(c) + eventID, calendarID := createOn(t, svc, "a@a.test") + tc.fail(t, c, srvA) + + err := svc.UpdateEvent(ctx, "u1", calendarID, eventID, moveStart, moveEnd) + if err == nil || errors.Is(err, calendar.ErrEventUnreachable) { + t.Errorf("UpdateEvent = %v, want a retryable error (not ErrEventUnreachable)", err) + } + err = svc.CancelEvent(ctx, "u1", calendarID, eventID) + if err == nil || errors.Is(err, calendar.ErrEventUnreachable) { + t.Errorf("CancelEvent = %v, want a retryable error (not ErrEventUnreachable)", err) + } + }) + } +} + // The common case, one CalDAV account that is still the destination, sends exactly the // requests it always did, whether or not a calendar id was recorded. func TestUpdateCancel_singleAccountUnchanged(t *testing.T) { diff --git a/internal/calendar/calendar.go b/internal/calendar/calendar.go index eda1b5ad..d9f0df29 100644 --- a/internal/calendar/calendar.go +++ b/internal/calendar/calendar.go @@ -7,6 +7,7 @@ package calendar import ( "context" "database/sql" + "errors" "sort" "time" @@ -91,6 +92,12 @@ type EventRecognizer interface { RecognizesEvent(eventID string) bool } +// ErrEventUnreachable matches an UpdateEvent or CancelEvent error that was refused before +// anything was sent, because the stored ids do not identify one connected account to act as. +// It is a verdict on stored state, not a failed request: a retry re-reads the same connections +// and refuses the same way, so the reconciler stops retrying an event that returns it. +var ErrEventUnreachable = errors.New("calendar: no single connected account holds this event") + // Service holds the configured providers and dispatches per-user operations to // whichever provider that user has connected. type Service struct { diff --git a/internal/handler/calendar_reconcile.go b/internal/handler/calendar_reconcile.go index 5ff52c11..ba67ca85 100644 --- a/internal/handler/calendar_reconcile.go +++ b/internal/handler/calendar_reconcile.go @@ -2,6 +2,8 @@ package handler import ( "context" + "errors" + "net/url" "time" "github.com/calnode/calnode/internal/calendar" @@ -96,8 +98,15 @@ func (h *Handler) reconcileReschedules(ctx context.Context, gc *calendar.Service continue } if err := gc.UpdateEvent(ctx, d.userID, d.calendarID, d.eventID, start, end); err != nil { - h.logger.Error("reconcile: re-apply event time", "error", err, "booking_id", d.bookingID, "host", d.userID) - continue // leave the flag set; retry next sweep + if !errors.Is(err, calendar.ErrEventUnreachable) { + h.logger.Error("reconcile: re-apply event time", "error", err, "booking_id", d.bookingID, "host", d.userID) + continue // leave the flag set; retry next sweep + } + // Nothing was sent, and the next sweep would re-read the same connections and + // refuse the same way, so clear the flag rather than log this every sweep until + // the booking ends. The event keeps its old time. + h.logger.Warn("reconcile: not retrying an event no single account holds; it keeps its old time", + "error", err, "booking_id", d.bookingID, "host", d.userID) } if _, err := h.db.ExecContext(ctx, `UPDATE booking_hosts SET needs_sync = 0 WHERE booking_id = ? AND user_id = ?`, @@ -133,8 +142,20 @@ func (h *Handler) reconcileCancellations(ctx context.Context, gc *calendar.Servi for _, o := range orphans { if err := gc.CancelEvent(ctx, o.userID, o.calendarID, o.eventID); err != nil { - h.logger.Error("reconcile: cancel orphaned event", "error", err, "booking_id", o.bookingID, "host", o.userID) - continue // leave the id in place; retry next sweep + if !errors.Is(err, calendar.ErrEventUnreachable) { + h.logger.Error("reconcile: cancel orphaned event", "error", err, "booking_id", o.bookingID, "host", o.userID) + continue // leave the id in place; retry next sweep + } + // As for reschedules: no retry can succeed, and this query has no date bound, so + // leaving the id would repeat the refusal every sweep forever. The event stays on + // its calendar; its id is logged because clearing it drops the only copy. A CalDAV + // id is a URL and could carry userinfo, so it is logged redacted, or not at all. + eventForLog := "" + if u, perr := url.Parse(o.eventID); perr == nil { + eventForLog = u.Redacted() + } + h.logger.Warn("reconcile: not retrying the delete of an event no single account holds; it stays on its calendar", + "error", err, "booking_id", o.bookingID, "host", o.userID, "event_id", eventForLog) } if _, err := h.db.ExecContext(ctx, `UPDATE booking_hosts SET external_event_id = NULL WHERE booking_id = ? AND user_id = ?`, diff --git a/internal/handler/calendar_reconcile_internal_test.go b/internal/handler/calendar_reconcile_internal_test.go new file mode 100644 index 00000000..a25c2753 --- /dev/null +++ b/internal/handler/calendar_reconcile_internal_test.go @@ -0,0 +1,224 @@ +package handler + +import ( + "bytes" + "context" + "database/sql" + "errors" + "fmt" + "log/slog" + "strings" + "sync" + "testing" + "time" + + "github.com/calnode/calnode/internal/calendar" + "github.com/calnode/calnode/internal/db" + "github.com/calnode/calnode/internal/slots" +) + +// stuckProvider claims every event id (as CalDAV does for its URLs) and fails every update and +// cancel with err, counting the calls so a test can tell a retry from a sweep that let go. +type stuckProvider struct { + err error + + mu sync.Mutex + updates int + cancels int +} + +func (p *stuckProvider) calls() (updates, cancels int) { + p.mu.Lock() + defer p.mu.Unlock() + return p.updates, p.cancels +} + +func (p *stuckProvider) Name() string { return "caldav" } +func (p *stuckProvider) InvitesGuests() bool { return false } +func (p *stuckProvider) RecognizesEvent(string) bool { return true } +func (p *stuckProvider) AuthURL(string) string { return "" } +func (p *stuckProvider) EncryptState(string) (string, error) { return "", nil } +func (p *stuckProvider) DecryptState(string) (string, error) { return "", nil } +func (p *stuckProvider) Exchange(context.Context, string, string, string) error { return nil } +func (p *stuckProvider) Connected(context.Context, string) (bool, error) { return true, nil } +func (p *stuckProvider) Disconnect(context.Context, string) error { return nil } +func (p *stuckProvider) HasDestination(context.Context, string) (bool, error) { return false, nil } +func (p *stuckProvider) ListCalendars(context.Context, string, string) ([]calendar.CalendarInfo, error) { + return nil, nil +} +func (p *stuckProvider) FreeBusy(context.Context, string, time.Time, time.Time) ([]slots.Interval, error) { + return nil, nil +} +func (p *stuckProvider) CreateEvent(context.Context, string, calendar.CreateEventParams) (string, string, string, error) { + return "", "", "", nil +} +func (p *stuckProvider) UpdateEvent(context.Context, string, string, string, time.Time, time.Time) error { + p.mu.Lock() + defer p.mu.Unlock() + p.updates++ + return p.err +} +func (p *stuckProvider) CancelEvent(context.Context, string, string, string) error { + p.mu.Lock() + defer p.mu.Unlock() + p.cancels++ + return p.err +} + +// unreachable stands in for the CalDAV provider's refusal: its own message, matching the +// sentinel the way caldav's errors do, rather than the bare sentinel. +type unreachable struct{} + +func (unreachable) Error() string { return "caldav: more than one account could hold this event" } +func (unreachable) Is(target error) bool { return target == calendar.ErrEventUnreachable } + +// newReconcileFixture seeds one host, an event type and a booking in the given status whose +// host row carries an event id, with needs_sync set. It returns the handler (logging into logs), +// the database and the booking id. +func newReconcileFixture(t *testing.T, status string, logs *bytes.Buffer) (*Handler, *sql.DB, string) { + t.Helper() + database, err := db.Open("sqlite://:memory:") + if err != nil { + t.Fatalf("db.Open: %v", err) + } + if err := db.Migrate(database); err != nil { + t.Fatalf("db.Migrate: %v", err) + } + t.Cleanup(func() { database.Close() }) + + start := time.Now().UTC().Add(24 * time.Hour) + for _, q := range []struct { + sql string + args []any + }{ + {`INSERT INTO users (id, email, name, iana_timezone) VALUES ('host-1', 'host@example.com', 'Host', 'UTC')`, nil}, + {`INSERT INTO event_types (id, user_id, slug, name, duration_minutes) VALUES ('et-1', 'host-1', 'intro', 'Intro', 30)`, nil}, + {`INSERT INTO bookings (id, event_type_id, host_id, start_at, end_at, status) VALUES ('bk-1', 'et-1', 'host-1', ?, ?, ?)`, + []any{start.Format(time.RFC3339), start.Add(30 * time.Minute).Format(time.RFC3339), status}}, + {`INSERT INTO booking_hosts (id, booking_id, user_id, is_primary, external_event_id, external_calendar_id, needs_sync) + VALUES ('bh-1', 'bk-1', 'host-1', 1, 'https://dav.example.com/calendars/host/home/ev1.ics', 'https://dav.example.com/calendars/host/home/', 1)`, nil}, + } { + if _, err := database.Exec(q.sql, q.args...); err != nil { + t.Fatalf("seed: %v\n%s", err, q.sql) + } + } + return New(database, slog.New(slog.NewTextHandler(logs, nil))), database, "bk-1" +} + +func hostRow(t *testing.T, database *sql.DB) (eventID sql.NullString, needsSync int) { + t.Helper() + if err := database.QueryRow( + `SELECT external_event_id, needs_sync FROM booking_hosts WHERE booking_id = 'bk-1' AND user_id = 'host-1'`, + ).Scan(&eventID, &needsSync); err != nil { + t.Fatalf("read booking_hosts: %v", err) + } + return eventID, needsSync +} + +// A reschedule the provider refuses as unreachable is not retried: the first sweep logs one +// warning and clears needs_sync, and the second sweep does not call the provider at all. Any +// other error leaves the flag set, so the next sweep tries again. +func TestReconcileReschedules_unreachableEventIsNotRetried(t *testing.T) { + cases := []struct { + name string + err error + wantUpdates int + wantSync int + }{ + {"unreachable: flag cleared, second sweep sends nothing", fmt.Errorf("update: %w", unreachable{}), 1, 0}, + {"transient: flag kept, retried every sweep", errors.New("caldav: fetch event for update returned status 503"), 2, 1}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var logs bytes.Buffer + h, database, _ := newReconcileFixture(t, "confirmed", &logs) + p := &stuckProvider{err: tc.err} + svc := calendar.NewService(database) + svc.Register(p) + + h.reconcileReschedules(context.Background(), svc) + h.reconcileReschedules(context.Background(), svc) + + if updates, _ := p.calls(); updates != tc.wantUpdates { + t.Errorf("UpdateEvent called %d times over two sweeps, want %d", updates, tc.wantUpdates) + } + eventID, needsSync := hostRow(t, database) + if needsSync != tc.wantSync { + t.Errorf("needs_sync = %d, want %d", needsSync, tc.wantSync) + } + if !eventID.Valid { + t.Error("external_event_id was cleared; a reschedule must keep the event id") + } + if tc.wantSync == 0 { + if n := strings.Count(logs.String(), "not retrying an event no single account holds"); n != 1 { + t.Errorf("logged the give-up warning %d times, want exactly once:\n%s", n, logs.String()) + } + } + }) + } +} + +// A cancellation the provider refuses as unreachable is not retried either. This sweep has no +// date bound, so before this change the refusal repeated every two minutes forever. The event id +// is logged (it is the only record of which event was left behind), then cleared. +func TestReconcileCancellations_unreachableEventIsNotRetried(t *testing.T) { + cases := []struct { + name string + err error + wantCancels int + wantID bool + }{ + {"unreachable: id cleared, second sweep sends nothing", fmt.Errorf("cancel: %w", unreachable{}), 1, false}, + {"transient: id kept, retried every sweep", errors.New("caldav: delete event returned status 503"), 2, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var logs bytes.Buffer + h, database, _ := newReconcileFixture(t, "cancelled", &logs) + p := &stuckProvider{err: tc.err} + svc := calendar.NewService(database) + svc.Register(p) + + h.reconcileCancellations(context.Background(), svc) + h.reconcileCancellations(context.Background(), svc) + + if _, cancels := p.calls(); cancels != tc.wantCancels { + t.Errorf("CancelEvent called %d times over two sweeps, want %d", cancels, tc.wantCancels) + } + eventID, _ := hostRow(t, database) + if eventID.Valid != tc.wantID { + t.Errorf("external_event_id present = %v, want %v", eventID.Valid, tc.wantID) + } + if !tc.wantID { + out := logs.String() + if n := strings.Count(out, "not retrying the delete of an event no single account holds"); n != 1 { + t.Errorf("logged the give-up warning %d times, want exactly once:\n%s", n, out) + } + if !strings.Contains(out, "calendars/host/home/ev1.ics") { + t.Errorf("the warning does not name the event it leaves behind:\n%s", out) + } + } + }) + } +} + +// The event id is a URL a host typed the server part of, so it can carry userinfo. The give-up +// warning must not write a password into the log. +func TestReconcileCancellations_warningRedactsEventURL(t *testing.T) { + var logs bytes.Buffer + h, database, _ := newReconcileFixture(t, "cancelled", &logs) + if _, err := database.Exec(`UPDATE booking_hosts SET external_event_id = 'https://host:s3cret@dav.example.com/calendars/host/home/ev1.ics'`); err != nil { + t.Fatal(err) + } + svc := calendar.NewService(database) + svc.Register(&stuckProvider{err: unreachable{}}) + + h.reconcileCancellations(context.Background(), svc) + + if strings.Contains(logs.String(), "s3cret") { + t.Errorf("the warning logged the password embedded in the event URL:\n%s", logs.String()) + } + if !strings.Contains(logs.String(), "calendars/host/home/ev1.ics") { + t.Errorf("the warning does not name the event:\n%s", logs.String()) + } +}