fix: If importing a backup fails, delete the partially-imported profile (#7885)

fix https://github.com/chatmail/core/issues/7863 `test_import_encrypted_bak_into_encrypted_acct` CFFI test fails because it tests that trying to import an encrypted account with a wrong passphrase into an already-encrypted database will fail, but leave the already-encrypted database (esp, leave it with the same password). But in order to reset the database after a failed login attempt, I'm using this code: ```rust context.sql.close().await; fs::remove_file(context.sql.dbfile.as_path()) .await .log_err(context) .ok(); context .sql .open(context, "".to_string()) // <-- NOTE THIS LINE .await .log_err(context) .ok(); ``` We're not remembering the password, so, we can't just pass the correct password there. Since password-protected databases are not really supported anyways, we decided to just skip the test. I also tried two tricks for deleting everything [found on Stackoverflow](https://stackoverflow.com/questions/525512/drop-all-tables-command), but neither of them managed to actually reset the database (i.e. they led to a failed Rust test, because asserting `!context2.is_configured().await?` failed): ```rust context .sql .call_write(|conn| { let mut stmt = conn.prepare( "select 'drop table ' || name || ';' from sqlite_master where type = 'table';", )?; let mut iter = stmt.query(())?; while iter.next()?.is_some() {} Ok(()) }) .await .log_err(context) .ok(); context .sql .run_migrations(context) .await .log_err(context) .ok(); ``` ```rust context .sql .transaction(|t| { t.execute_batch( " PRAGMA writable_schema = 1; delete from sqlite_master where type in ('table', 'index', 'trigger'); PRAGMA writable_schema = 0", )?; Ok(()) }) .await .log_err(context) .ok(); context .sql .run_migrations(context) .await .log_err(context) .ok(); ``` --------- Co-authored-by: l <link2xt@testrun.org>
2026-05-04 22:06:29 +03:00 · 2026-02-25 16:25:33 +01:00
parent d7bf10d7a4
commit 7d8989a068
3 changed files with 138 additions and 16 deletions
--- a/python/tests/test_3_offline.py
+++ b/python/tests/test_3_offline.py
@@ -558,6 +558,12 @@ class TestOfflineChat:
        assert messages[0 + E2EE_INFO_MSGS].text == "msg1"
        assert os.path.exists(messages[1 + E2EE_INFO_MSGS].filename)

+    @pytest.mark.skip(
+        reason="We didn't find a way to correctly reset an account after a failed import attempt "
+        "while simultaneously making sure "
+        "that the password of an encrypted account survives a failed import attempt. "
+        "Since passphrases are not really supported anymore, we decided to just disable the test.",
+    )
    def test_import_encrypted_bak_into_encrypted_acct(self, acfactory, tmp_path):
        """
        Test that account passphrase isn't lost if backup failed to be imported.
--- a/src/imex.rs
+++ b/src/imex.rs
@@ -103,7 +103,8 @@ pub async fn imex(

    if let Err(err) = res.as_ref() {
        // We are using Anyhow's .context() and to show the inner error, too, we need the {:#}:
-        error!(context, "IMEX failed to complete: {:#}", err);
+        error!(context, "{:#}", err);
+        warn!(context, "IMEX failed to complete: {:#}", err);
        context.emit_event(EventType::ImexProgress(0));
    } else {
        info!(context, "IMEX successfully completed");
@@ -209,15 +210,6 @@ async fn import_backup(
    backup_to_import: &Path,
    passphrase: String,
 ) -> Result<()> {
-    ensure!(
-        !context.is_configured().await?,
-        "Cannot import backups to accounts in use."
-    );
-    ensure!(
-        !context.scheduler.is_running().await,
-        "cannot import backup, IO is running"
-    );
-
    let backup_file = File::open(backup_to_import).await?;
    let file_size = backup_file.metadata().await?.len();
    info!(
@@ -251,6 +243,15 @@ pub(crate) async fn import_backup_stream<R: tokio::io::AsyncRead + Unpin>(
    file_size: u64,
    passphrase: String,
 ) -> Result<()> {
+    ensure!(
+        !context.is_configured().await?,
+        "Cannot import backups to accounts in use"
+    );
+    ensure!(
+        !context.scheduler.is_running().await,
+        "Cannot import backup, IO is running"
+    );
+
    import_backup_stream_inner(context, backup_file, file_size, passphrase)
        .await
        .0
@@ -317,6 +318,9 @@ where
    }
 }

+// This function returns a tuple (Result<()>,) rather than Result<()>
+// so that we don't accidentally early-return with `?`
+// and forget to cleanup.
 async fn import_backup_stream_inner<R: tokio::io::AsyncRead + Unpin>(
    context: &Context,
    backup_file: R,
@@ -363,11 +367,6 @@ async fn import_backup_stream_inner<R: tokio::io::AsyncRead + Unpin>(
            }
        }
    };
-    if res.is_err() {
-        for blob in blobs {
-            fs::remove_file(&blob).await.log_err(context).ok();
-        }
-    }

    let unpacked_database = context.get_blobdir().join(DBFILE_BACKUP_NAME);
    if res.is_ok() {
@@ -390,6 +389,22 @@ async fn import_backup_stream_inner<R: tokio::io::AsyncRead + Unpin>(
        res = context.sql.run_migrations(context).await;
        context.emit_event(EventType::AccountsItemChanged);
    }
+    if res.is_err() {
+        context.sql.close().await;
+        fs::remove_file(context.sql.dbfile.as_path())
+            .await
+            .log_err(context)
+            .ok();
+        for blob in blobs {
+            fs::remove_file(&blob).await.log_err(context).ok();
+        }
+        context
+            .sql
+            .open(context, "".to_string())
+            .await
+            .log_err(context)
+            .ok();
+    }
    if res.is_ok() {
        delete_and_reset_all_device_msgs(context)
            .await
@@ -807,7 +822,7 @@ mod tests {

    use super::*;
    use crate::config::Config;
-    use crate::test_utils::{TestContext, alice_keypair};
+    use crate::test_utils::{TestContext, TestContextManager, alice_keypair};

    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
    async fn test_export_public_key_to_asc_file() {
@@ -1024,6 +1039,81 @@ mod tests {
        Ok(())
    }

+    /// Tests that [`crate::qr::DCBACKUP_VERSION`] is checked correctly.
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn test_import_backup_fails_because_of_dcbackup_version() -> Result<()> {
+        let mut tcm = TestContextManager::new();
+        let context1 = tcm.alice().await;
+        let context2 = tcm.unconfigured().await;
+
+        assert!(context1.is_configured().await?);
+        assert!(!context2.is_configured().await?);
+
+        let backup_dir = tempfile::tempdir().unwrap();
+
+        tcm.section("export from context1");
+        assert!(
+            imex(&context1, ImexMode::ExportBackup, backup_dir.path(), None)
+                .await
+                .is_ok()
+        );
+        let _event = context1
+            .evtracker
+            .get_matching(|evt| matches!(evt, EventType::ImexProgress(1000)))
+            .await;
+        let backup = has_backup(&context2, backup_dir.path()).await?;
+        let modified_backup = backup_dir.path().join("modified_backup.tar");
+
+        tcm.section("Change backup_version to be higher than DCBACKUP_VERSION");
+        {
+            let unpack_dir = tempfile::tempdir().unwrap();
+            let mut ar = Archive::new(File::open(&backup).await?);
+            ar.unpack(&unpack_dir).await?;
+
+            let sql = sql::Sql::new(unpack_dir.path().join(DBFILE_BACKUP_NAME));
+            sql.open(&context2, "".to_string()).await?;
+            assert_eq!(
+                sql.get_raw_config_int("backup_version").await?.unwrap(),
+                DCBACKUP_VERSION
+            );
+            sql.set_raw_config_int("backup_version", DCBACKUP_VERSION + 1)
+                .await?;
+            sql.close().await;
+
+            let modified_backup_file = File::create(&modified_backup).await?;
+            let mut builder = tokio_tar::Builder::new(modified_backup_file);
+            builder.append_dir_all("", unpack_dir.path()).await?;
+            builder.finish().await?;
+        }
+
+        tcm.section("import to context2");
+        let err = imex(&context2, ImexMode::ImportBackup, &modified_backup, None)
+            .await
+            .unwrap_err();
+        assert!(err.to_string().starts_with("This profile is from a newer version of Delta Chat. Please update Delta Chat and try again"));
+
+        // Some UIs show the error from the event to the user.
+        // Therefore, it must also be a user-facing string, rather than some technical info:
+        let err_event = context2
+            .evtracker
+            .get_matching(|evt| matches!(evt, EventType::Error(_)))
+            .await;
+        let EventType::Error(err_msg) = err_event else {
+            unreachable!()
+        };
+        assert!(err_msg.starts_with("This profile is from a newer version of Delta Chat. Please update Delta Chat and try again"));
+
+        context2
+            .evtracker
+            .get_matching(|evt| matches!(evt, EventType::ImexProgress(0)))
+            .await;
+
+        assert!(!context2.is_configured().await?);
+        assert_eq!(context2.get_config(Config::ConfiguredAddr).await?, None);
+
+        Ok(())
+    }
+
    /// This is a regression test for
    /// https://github.com/deltachat/deltachat-android/issues/2263
    /// where the config cache wasn't reset properly after a backup.
--- a/src/imex/transfer.rs
+++ b/src/imex/transfer.rs
@@ -467,6 +467,32 @@ mod tests {
        }
    }

+    /// Tests that trying to accidentally overwrite a profile
+    /// that is in use will fail.
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn test_cant_overwrite_profile_in_use() -> Result<()> {
+        let mut tcm = TestContextManager::new();
+        let ctx0 = &tcm.alice().await;
+        let ctx1 = &tcm.bob().await;
+
+        // Prepare to transfer backup.
+        let provider = BackupProvider::prepare(ctx0).await?;
+
+        // Try to overwrite an existing profile.
+        let err = get_backup(ctx1, provider.qr()).await.unwrap_err();
+        assert!(format!("{err:#}").contains("Cannot import backups to accounts in use"));
+
+        // ctx0 is supposed to also finish, and emit an error:
+        provider.await.unwrap();
+        ctx0.evtracker
+            .get_matching(|e| matches!(e, EventType::Error(_)))
+            .await;
+
+        assert_eq!(ctx1.get_primary_self_addr().await?, "bob@example.net");
+
+        Ok(())
+    }
+
    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
    async fn test_drop_provider() {
        let mut tcm = TestContextManager::new();