cyrusimap / cyrus-imapd

Cyrus IMAP is an email, contacts and calendar server
http://cyrusimap.org
Other
544 stars 149 forks source link

[2.5.11] XFER destroys calendar and addressbook flags on folders #2220

Open MASHtm opened 6 years ago

MASHtm commented 6 years ago

After moving a mailbox with XFER between backends on 2.5.11 in a murder setup all MBTYPE_CALENDAR and MBTYPE_ADDRESSBOOK flags get lost and calendars and addressbooks are inaccessible. dav_reconstruct does nothing since there are no mailboxes of those types.

Fixing the flags using ctl_mboxlist (-d/-u) worked, but now dav_reconstruct fails with SIGSEGV. I will file another report for dav_reconstruct.

MASHtm commented 6 years ago

Am I right that up to master xfer_localcreate() should use RFC 4466 style parameters and provide TYPE ADDRESSBOOK|CALENDAR for LOCALCREATE?

MASHtm commented 6 years ago

compiles but completely untested:

--- imap/imapd.c.orig   2017-10-27 23:05:36.978713287 +0200
+++ imap/imapd.c    2017-12-21 17:24:18.120554089 +0100
@@ -10245,14 +10245,29 @@
     int r;

     for (item = xfer->items; item; item = item->next) {
-   if (xfer->topart) {
-       /* need to send partition as an atom */
-       prot_printf(xfer->be->out, "LC1 LOCALCREATE {" SIZE_T_FMT "+}\r\n%s %s\r\n",
-           strlen(item->extname), item->extname, xfer->topart);
-   } else {
-       prot_printf(xfer->be->out, "LC1 LOCALCREATE {" SIZE_T_FMT "+}\r\n%s\r\n",
-           strlen(item->extname), item->extname);
+   prot_printf(xfer->be->out, "LC1 LOCALCREATE {" SIZE_T_FMT "+}\r\n%s",
+           strlen(item->extname), item->extname);
+   // MBTYPES_DAV means RFC4466 readyness needed on destination
+   if (item->mbentry->mbtype & MBTYPES_DAV) {
+       prot_printf(xfer->be->out, " (TYPE ");
+       if (item->mbentry->mbtype & MBTYPE_CALENDAR)
+       prot_printf(xfer->be->out, "CALENDAR");
+       else
+       prot_printf(xfer->be->out, "ADDRESSBOOK");
+
+       if (xfer->topart) {
+       prot_printf(xfer->be->out, " PARTITION ");
+       prot_printastring(xfer->be->out, xfer->topart);
+            }
+       prot_putc(')', xfer->be->out);
+   }
+   // send partition as an atom for older servers
+   else if (xfer->topart) {
+       prot_putc(' ', xfer->be->out);
+       prot_printastring(xfer->be->out, xfer->topart);
    }
+   prot_printf(xfer->be->out, "\r\n");
+
    r = getresult(xfer->be->in, "LC1");
    if (r) {
        syslog(LOG_ERR, "Could not move mailbox: %s, LOCALCREATE failed",
elliefm commented 6 years ago

note to self: probably should write some cassandane tests for this. not sure what correct implementation is, but if it can successfully xfer the mailboxes without losing the addressbook/calendar flags, then that sounds like a win

elliefm commented 6 years ago

Have had a deeper look and this patch looks sane to my eyes/understanding. Will still make a Cassandane test for it though, cause it'd be nice to not break it in the future. :)

elliefm commented 3 years ago

Might be able to add some calendar/addressbook/etc data to MurderIMAP::populate/check_user(), and then all the xfer tests will verify that these data types are handled correctly.

elliefm commented 3 years ago

I believe this is probably already fixed for XFERs between 3.0+ servers, since their XFER uses replication rather than the old localcreate and undump thing. But it would be nice to prove that, and also prove whether this patch fixes xfers to 2.5 backends (which don't support xfer-over-replication, and so the sender falls back to localcreate/undump)

MASHtm commented 3 years ago

at least I'm still using this patch on my local 2.5.17 build and it works as expected. And I'm moving mailboxes regularly between a lot of backends.