Merge pull request #4563 from cyrusimap/imap_xfer_fix

imapd.c: make sure we delete XFERed mailboxes from source (issue #4370)
cyrusimap · Aug 17, 2023 · 5625e15 · 5625e15
2 parents c53219d + 1caa0a3
commit 5625e15
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 26 deletions.
diff --git a/cassandane/Cassandane/Cyrus/MurderIMAP.pm b/cassandane/Cassandane/Cyrus/MurderIMAP.pm
@@ -53,6 +53,11 @@ $Data::Dumper::Sortkeys = 1;
 sub new
 {
     my $class = shift;
+
+    my $config = Cassandane::Config->default()->clone();
+
+    $config->set(conversations => 'yes');
+
     return $class->SUPER::new({
         imapmurder => 1, adminstore => 1, deliver => 1,
     }, @_);
@@ -639,6 +644,65 @@ sub test_xfer_user_noaltns_nounixhs
     });
 }
 
+sub test_xfer_user_verify_cleanup
+    :AllowMoves :NoAltNamespace :Conversations
+    :needs_component_murder :min_version_3_9
+{
+    my ($self) = @_;
+
+    # set up some data for cassandane on backend1
+    my $expected = $self->populate_user($self->{instance},
+                                        $self->{backend1_store},
+                                        [qw(INBOX INBOX.Drafts)]);
+
+    my $imaptalk = $self->{backend1_store}->get_client();
+    my $admintalk = $self->{backend1_adminstore}->get_client();
+    my $backend2_servername = $self->{backend2}->get_servername();
+
+    xlog $self, "Subscribe to INBOX";
+    $imaptalk->subscribe("INBOX");
+
+    xlog $self, "Install a sieve script";
+    $self->{instance}->install_sieve_script(<<EOF
+keep;
+EOF
+    );
+
+    xlog $self, "Run squatter";
+    $self->{instance}->run_command({cyrus => 1}, 'squatter');
+
+    xlog $self, "Verify user mailbox directories exist";
+    my $inbox_dir = $self->{instance}->folder_to_directory('INBOX');
+    my $drafts_dir = $self->{instance}->folder_to_directory('INBOX.Drafts');
+    $self->assert_file_test($inbox_dir, '-d');
+    $self->assert_file_test($drafts_dir, '-d');
+
+    xlog $self, "Verify user data files/directories exist";
+    my $data = $self->{instance}->run_mbpath('-u', 'cassandane');
+    $self->assert_file_test($data->{user}{'sub'}, '-f');
+    $self->assert_file_test($data->{user}{counters}, '-f');
+    $self->assert_file_test($data->{user}{conversations}, '-f');
+    $self->assert_file_test($data->{user}{xapianactive}, '-f');
+    $self->assert_file_test("$data->{user}{sieve}/defaultbc", '-f');
+    $self->assert_file_test($data->{xapian}{t1}, '-d');
+
+    # now xfer the cassandane user to backend2
+    my $ret = $admintalk->_imap_cmd('xfer', 0, {},
+                                    'user.cassandane', $backend2_servername);
+
+    xlog $self, "Verify user mailbox directories have been deleted";
+    $self->assert_not_file_test($inbox_dir, '-e');
+    $self->assert_not_file_test($drafts_dir, '-e');
+
+    xlog $self, "Verify user data files/directories have been deleted";
+    $self->assert_not_file_test($data->{user}{'sub'}, '-e');
+    $self->assert_not_file_test($data->{user}{counters}, '-e');
+    $self->assert_not_file_test($data->{user}{conversations}, '-e');
+    $self->assert_not_file_test($data->{user}{xapianactive}, '-e');
+    $self->assert_not_file_test($data->{user}{sieve}, '-e');
+    $self->assert_not_file_test($data->{xapian}{t1}, '-e');
+}
+
 sub test_xfer_user_altns_unixhs_virtdom
     :AllowMoves :AltNamespace :UnixHierarchySep :VirtDomains
     :needs_component_murder :min_version_3_2

diff --git a/imap/imapd.c b/imap/imapd.c
@@ -12506,7 +12506,6 @@ static int xfer_reactivate(struct xfer_header *xfer)
 
 static int xfer_delete(struct xfer_header *xfer)
 {
-    mbentry_t *newentry = NULL;
     struct xfer_item *item;
     int r;
 
@@ -12515,38 +12514,18 @@ static int xfer_delete(struct xfer_header *xfer)
     /* 7) local delete of mailbox
      * & remove local "remote" mailboxlist entry */
     for (item = xfer->items; item; item = item->next) {
-        /* Set mailbox as DELETED on local server
-           (need to also reset to local partition,
-           otherwise mailbox can not be opened for deletion) */
-        /* XXX - this code is awful... need a sane way to manage mbentries */
-        newentry = mboxlist_entry_create();
-        newentry->name = xstrdupnull(item->mbentry->name);
-        newentry->uniqueid = xstrdupnull(item->mbentry->uniqueid);
-        newentry->acl = xstrdupnull(item->mbentry->acl);
-        newentry->server = xstrdupnull(item->mbentry->server);
-        newentry->partition = xstrdupnull(item->mbentry->partition);
-        newentry->mbtype = item->mbentry->mbtype | MBTYPE_DELETED;
-        r = mboxlist_updatelock(newentry, 1);
-        mboxlist_entry_free(&newentry);
-
-        if (r) {
-            syslog(LOG_ERR,
-                   "Could not move mailbox: %s, mboxlist_update failed (%s)",
-                   item->mbentry->name, error_message(r));
-        }
-
         /* Note that we do not check the ACL, and we don't update MUPDATE */
         /* note also that we need to remember to let proxyadmins do this */
         /* On a unified system, the subsequent MUPDATE PUSH on the remote
            should repopulate the local mboxlist entry */
         r = mboxlist_deletemailboxlock(item->mbentry->name,
                                    imapd_userisadmin || imapd_userisproxyadmin,
                                    imapd_userid, imapd_authstate, NULL,
-                                   MBOXLIST_DELETE_LOCALONLY);
+                                   MBOXLIST_DELETE_LOCALONLY|MBOXLIST_DELETE_FORCE);
         if (r) {
             syslog(LOG_ERR,
-                   "Could not delete local mailbox during move of %s",
-                   item->mbentry->name);
+                   "Could not delete local mailbox during move of %s: %s",
+                   item->mbentry->name, error_message(r));
             /* can't abort now! */
         }
     }

diff --git a/imap/mboxlist.c b/imap/mboxlist.c
@@ -2380,7 +2380,11 @@ EXPORTED int mboxlist_deletemailbox(const char *name, int isadmin,
 
     /* Lock the mailbox if it isn't a remote mailbox */
     if (!isremote) {
-        r = mailbox_open_iwl(name, &mailbox);
+        if (force) {
+            /* Allow deleting moved (XFERed) mailboxes */
+            mbentry->mbtype &= ~MBTYPE_MOVING;
+        }
+        r = mailbox_open_from_mbe(mbentry, &mailbox);
         if (!r) mailbox->silentchanges = silent;
     }
     if (r && !force) goto done;
@@ -2409,7 +2413,8 @@ EXPORTED int mboxlist_deletemailbox(const char *name, int isadmin,
         int haschildren = mboxlist_haschildren(name);
         mbentry_t *newmbentry = mboxlist_entry_create();
         newmbentry->name = xstrdupnull(name);
-        newmbentry->mbtype |= haschildren ? MBTYPE_INTERMEDIATE : MBTYPE_DELETED;
+        newmbentry->mbtype = mbentry->mbtype |
+            (haschildren ? MBTYPE_INTERMEDIATE : MBTYPE_DELETED);
         if (mailbox) {
             newmbentry->uniqueid = xstrdupnull(mailbox_uniqueid(mailbox));
             newmbentry->uidvalidity = mailbox->i.uidvalidity;