fix: fixes for transport JsonRPC (#6680)

Follow-up to #6582

---------

Co-authored-by: adbenitez <asieldbenitez@gmail.com>

deltachat-jsonrpc/src/api.rs

@@ -457,7 +457,7 @@ impl CommandApi {
     ///
     /// This function stops and starts IO as needed.
     ///
-    /// Usually it will be enough to only set `addr` and `imap.password`,
+    /// Usually it will be enough to only set `addr` and `password`,
     /// and all the other settings will be autoconfigured.
     ///
     /// During configuration, ConfigureProgress events are emitted;

deltachat-jsonrpc/src/api/types/login_param.rs

@@ -4,53 +4,6 @@ use serde::Deserialize;
 use serde::Serialize;
 use yerpc::TypeDef;
 
-#[derive(Serialize, Deserialize, TypeDef, schemars::JsonSchema)]
-#[serde(rename_all = "camelCase")]
-pub struct EnteredServerLoginParam {
-    /// Server hostname or IP address.
-    pub server: String,
-
-    /// Server port.
-    ///
-    /// 0 if not specified.
-    pub port: u16,
-
-    /// Socket security.
-    pub security: Socket,
-
-    /// Username.
-    ///
-    /// Empty string if not specified.
-    pub user: String,
-
-    /// Password.
-    pub password: String,
-}
-
-impl From<dc::EnteredServerLoginParam> for EnteredServerLoginParam {
-    fn from(param: dc::EnteredServerLoginParam) -> Self {
-        Self {
-            server: param.server,
-            port: param.port,
-            security: param.security.into(),
-            user: param.user,
-            password: param.password,
-        }
-    }
-}
-
-impl From<EnteredServerLoginParam> for dc::EnteredServerLoginParam {
-    fn from(param: EnteredServerLoginParam) -> Self {
-        Self {
-            server: param.server,
-            port: param.port,
-            security: param.security.into(),
-            user: param.user,
-            password: param.password,
-        }
-    }
-}
-
 /// Login parameters entered by the user.
 
 #[derive(Serialize, Deserialize, TypeDef, schemars::JsonSchema)]
@@ -59,28 +12,67 @@ pub struct EnteredLoginParam {
     /// Email address.
     pub addr: String,
 
-    /// IMAP settings.
+    /// Password.
-    pub imap: EnteredServerLoginParam,
+    pub password: String,
 
-    /// SMTP settings.
+    /// Imap server hostname or IP address.
-    pub smtp: EnteredServerLoginParam,
+    pub imap_server: Option<String>,
+
+    /// Imap server port.
+    pub imap_port: Option<u16>,
+
+    /// Imap socket security.
+    pub imap_security: Option<Socket>,
+
+    /// Imap username.
+    pub imap_user: Option<String>,
+
+    /// SMTP server hostname or IP address.
+    pub smtp_server: Option<String>,
+
+    /// SMTP server port.
+    pub smtp_port: Option<u16>,
+
+    /// SMTP socket security.
+    pub smtp_security: Option<Socket>,
+
+    /// SMTP username.
+    pub smtp_user: Option<String>,
+
+    /// SMTP Password.
+    ///
+    /// Only needs to be specified if different than IMAP password.
+    pub smtp_password: Option<String>,
 
     /// TLS options: whether to allow invalid certificates and/or
-    /// invalid hostnames
+    /// invalid hostnames.
-    pub certificate_checks: EnteredCertificateChecks,
+    /// Default: Automatic
+    pub certificate_checks: Option<EnteredCertificateChecks>,
 
-    /// If true, login via OAUTH2 (not recommended anymore)
+    /// If true, login via OAUTH2 (not recommended anymore).
-    pub oauth2: bool,
+    /// Default: false
+    pub oauth2: Option<bool>,
 }
 
 impl From<dc::EnteredLoginParam> for EnteredLoginParam {
     fn from(param: dc::EnteredLoginParam) -> Self {
+        let imap_security: Socket = param.imap.security.into();
+        let smtp_security: Socket = param.smtp.security.into();
+        let certificate_checks: EnteredCertificateChecks = param.certificate_checks.into();
         Self {
             addr: param.addr,
-            imap: param.imap.into(),
+            password: param.imap.password,
-            smtp: param.smtp.into(),
+            imap_server: param.imap.server.into_option(),
-            certificate_checks: param.certificate_checks.into(),
+            imap_port: param.imap.port.into_option(),
-            oauth2: param.oauth2,
+            imap_security: imap_security.into_option(),
+            imap_user: param.imap.user.into_option(),
+            smtp_server: param.smtp.server.into_option(),
+            smtp_port: param.smtp.port.into_option(),
+            smtp_security: smtp_security.into_option(),
+            smtp_user: param.smtp.user.into_option(),
+            smtp_password: param.smtp.password.into_option(),
+            certificate_checks: certificate_checks.into_option(),
+            oauth2: param.oauth2.into_option(),
         }
     }
 }
@@ -91,18 +83,31 @@ impl TryFrom<EnteredLoginParam> for dc::EnteredLoginParam {
     fn try_from(param: EnteredLoginParam) -> Result<Self> {
         Ok(Self {
             addr: param.addr,
-            imap: param.imap.into(),
+            imap: dc::EnteredServerLoginParam {
-            smtp: param.smtp.into(),
+                server: param.imap_server.unwrap_or_default(),
-            certificate_checks: param.certificate_checks.into(),
+                port: param.imap_port.unwrap_or_default(),
-            oauth2: param.oauth2,
+                security: param.imap_security.unwrap_or_default().into(),
+                user: param.imap_user.unwrap_or_default(),
+                password: param.password,
+            },
+            smtp: dc::EnteredServerLoginParam {
+                server: param.smtp_server.unwrap_or_default(),
+                port: param.smtp_port.unwrap_or_default(),
+                security: param.smtp_security.unwrap_or_default().into(),
+                user: param.smtp_user.unwrap_or_default(),
+                password: param.smtp_password.unwrap_or_default(),
+            },
+            certificate_checks: param.certificate_checks.unwrap_or_default().into(),
+            oauth2: param.oauth2.unwrap_or_default(),
         })
     }
 }
 
-#[derive(Serialize, Deserialize, TypeDef, schemars::JsonSchema)]
+#[derive(Serialize, Deserialize, TypeDef, schemars::JsonSchema, Default, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub enum Socket {
     /// Unspecified socket security, select automatically.
+    #[default]
     Automatic,
 
     /// TLS connection.
@@ -137,12 +142,13 @@ impl From<Socket> for dc::Socket {
     }
 }
 
-#[derive(Serialize, Deserialize, TypeDef, schemars::JsonSchema)]
+#[derive(Serialize, Deserialize, TypeDef, schemars::JsonSchema, Default, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub enum EnteredCertificateChecks {
     /// `Automatic` means that provider database setting should be taken.
     /// If there is no provider database setting for certificate checks,
     /// check certificates strictly.
+    #[default]
     Automatic,
 
     /// Ensure that TLS certificate is valid for the server hostname.
@@ -177,3 +183,19 @@ impl From<EnteredCertificateChecks> for dc::EnteredCertificateChecks {
         }
     }
 }
+
+trait IntoOption<T> {
+    fn into_option(self) -> Option<T>;
+}
+impl<T> IntoOption<T> for T
+where
+    T: Default + std::cmp::PartialEq,
+{
+    fn into_option(self) -> Option<T> {
+        if self == T::default() {
+            None
+        } else {
+            Some(self)
+        }
+    }
+}

deltachat-rpc-client/src/deltachat_rpc_client/pytestplugin.py

@@ -12,14 +12,6 @@ from ._utils import futuremethod
 from .rpc import Rpc
 
 
-def get_temp_credentials() -> dict:
-    domain = os.getenv("CHATMAIL_DOMAIN")
-    username = "ci-" + "".join(random.choice("2345789acdefghjkmnpqrstuvwxyz") for i in range(6))
-    password = f"{username}${username}"
-    addr = f"{username}@{domain}"
-    return {"email": addr, "password": password}
-
-
 class ACFactory:
     def __init__(self, deltachat: DeltaChat) -> None:
         self.deltachat = deltachat
@@ -32,26 +24,25 @@ class ACFactory:
     def get_unconfigured_bot(self) -> Bot:
         return Bot(self.get_unconfigured_account())
 
-    def new_preconfigured_account(self) -> Account:
+    def get_credentials(self) -> (str, str):
-        """Make a new account with configuration options set, but configuration not started."""
+        domain = os.getenv("CHATMAIL_DOMAIN")
-        credentials = get_temp_credentials()
+        username = "ci-" + "".join(random.choice("2345789acdefghjkmnpqrstuvwxyz") for i in range(6))
-        account = self.get_unconfigured_account()
+        return f"{username}@{domain}", f"{username}${username}"
-        account.set_config("addr", credentials["email"])
-        account.set_config("mail_pw", credentials["password"])
-        assert not account.is_configured()
-        return account
 
     @futuremethod
     def new_configured_account(self):
-        account = self.new_preconfigured_account()
+        addr, password = self.get_credentials()
-        yield account.configure.future()
+        account = self.get_unconfigured_account()
+        params = {"addr": addr, "password": password}
+        yield account._rpc.add_transport.future(account.id, params)
+
         assert account.is_configured()
         return account
 
     def new_configured_bot(self) -> Bot:
-        credentials = get_temp_credentials()
+        addr, password = self.get_credentials()
         bot = self.get_unconfigured_bot()
-        bot.configure(credentials["email"], credentials["password"])
+        bot.configure(addr, password)
         return bot
 
     @futuremethod

deltachat-rpc-client/tests/test_account_events.py

@@ -13,10 +13,11 @@ def test_event_on_configuration(acfactory: ACFactory) -> None:
     Test if ACCOUNTS_ITEM_CHANGED event is emitted on configure
     """
 
-    account = acfactory.new_preconfigured_account()
+    addr, password = acfactory.get_credentials()
+    account = acfactory.get_unconfigured_account()
     account.clear_all_events()
     assert not account.is_configured()
-    future = account.configure.future()
+    future = account._rpc.add_transport.future(account.id, {"addr": addr, "password": password})
     while True:
         event = account.wait_for_event()
         if event.kind == EventType.ACCOUNTS_ITEM_CHANGED:

deltachat-rpc-client/tests/test_securejoin.py

@@ -458,8 +458,7 @@ def test_aeap_flow_verified(acfactory):
     """Test that a new address is added to a contact when it changes its address."""
     ac1, ac2 = acfactory.get_online_accounts(2)
 
-    # ac1new is only used to get a new address.
+    addr, password = acfactory.get_credentials()
-    ac1new = acfactory.new_preconfigured_account()
 
     logging.info("ac1: create verified-group QR, ac2 scans and joins")
     chat = ac1.create_group("hello", protect=True)
@@ -479,8 +478,8 @@ def test_aeap_flow_verified(acfactory):
     assert msg_in_1.text == msg_out.text
 
     logging.info("changing email account")
-    ac1.set_config("addr", ac1new.get_config("addr"))
+    ac1.set_config("addr", addr)
-    ac1.set_config("mail_pw", ac1new.get_config("mail_pw"))
+    ac1.set_config("mail_pw", password)
     ac1.stop_io()
     ac1.configure()
     ac1.start_io()
@@ -493,11 +492,9 @@ def test_aeap_flow_verified(acfactory):
     msg_in_2_snapshot = msg_in_2.get_snapshot()
     assert msg_in_2_snapshot.text == msg_out.text
     assert msg_in_2_snapshot.chat.id == msg_in_1.chat.id
-    assert msg_in_2.get_sender_contact().get_snapshot().address == ac1new.get_config("addr")
+    assert msg_in_2.get_sender_contact().get_snapshot().address == addr
     assert len(msg_in_2_snapshot.chat.get_contacts()) == 2
-    assert ac1new.get_config("addr") in [
+    assert addr in [contact.get_snapshot().address for contact in msg_in_2_snapshot.chat.get_contacts()]
-        contact.get_snapshot().address for contact in msg_in_2_snapshot.chat.get_contacts()
-    ]
 
 
 def test_gossip_verification(acfactory) -> None:

deltachat-rpc-client/tests/test_something.py

@@ -61,45 +61,70 @@ def test_acfactory(acfactory) -> None:
 
 
 def test_configure_starttls(acfactory) -> None:
-    account = acfactory.new_preconfigured_account()
+    addr, password = acfactory.get_credentials()
-
+    account = acfactory.get_unconfigured_account()
-    # Use STARTTLS
+    account._rpc.add_transport(
-    account.set_config("mail_security", "2")
+        account.id,
-    account.set_config("send_security", "2")
+        {
-    account.configure()
+            "addr": addr,
+            "password": password,
+            "imapSecurity": "starttls",
+            "smtpSecurity": "starttls",
+        },
+    )
     assert account.is_configured()
 
 
 def test_configure_ip(acfactory) -> None:
-    account = acfactory.new_preconfigured_account()
+    addr, password = acfactory.get_credentials()
+    account = acfactory.get_unconfigured_account()
+    ip_address = socket.gethostbyname(addr.rsplit("@")[-1])
 
-    domain = account.get_config("addr").rsplit("@")[-1]
-    ip_address = socket.gethostbyname(domain)
-
-    # This should fail TLS check.
-    account.set_config("mail_server", ip_address)
     with pytest.raises(JsonRpcError):
-        account.configure()
+        account._rpc.add_transport(
+            account.id,
+            {
+                "addr": addr,
+                "password": password,
+                # This should fail TLS check.
+                "imapServer": ip_address,
+            },
+        )
 
 
 def test_configure_alternative_port(acfactory) -> None:
     """Test that configuration with alternative port 443 works."""
-    account = acfactory.new_preconfigured_account()
+    addr, password = acfactory.get_credentials()
-
+    account = acfactory.get_unconfigured_account()
-    account.set_config("mail_port", "443")
+    account._rpc.add_transport(
-    account.set_config("send_port", "443")
+        account.id,
-
+        {
-    account.configure()
+            "addr": addr,
+            "password": password,
+            "imapPort": 443,
+            "smtpPort": 443,
+        },
+    )
+    assert account.is_configured()
 
 
-def test_configure_username(acfactory) -> None:
+def test_list_transports(acfactory) -> None:
-    account = acfactory.new_preconfigured_account()
+    addr, password = acfactory.get_credentials()
-
+    account = acfactory.get_unconfigured_account()
-    addr = account.get_config("addr")
+    account._rpc.add_transport(
-    account.set_config("mail_user", addr)
+        account.id,
-    account.configure()
+        {
-
+            "addr": addr,
-    assert account.get_config("configured_mail_user") == addr
+            "password": password,
+            "imapUser": addr,
+        },
+    )
+    transports = account._rpc.list_transports(account.id)
+    assert len(transports) == 1
+    params = transports[0]
+    assert params["addr"] == addr
+    assert params["password"] == password
+    assert params["imapUser"] == addr
 
 
 def test_account(acfactory) -> None:
@@ -396,9 +421,11 @@ def test_wait_next_messages(acfactory) -> None:
     alice = acfactory.new_configured_account()
 
     # Create a bot account so it does not receive device messages in the beginning.
-    bot = acfactory.new_preconfigured_account()
+    addr, password = acfactory.get_credentials()
+    bot = acfactory.get_unconfigured_account()
     bot.set_config("bot", "1")
-    bot.configure()
+    bot._rpc.add_transport(bot.id, {"addr": addr, "password": password})
+    assert bot.is_configured()
 
     # There are no old messages and the call returns immediately.
     assert not bot.wait_next_messages()
@@ -577,9 +604,13 @@ def test_reactions_for_a_reordering_move(acfactory, direct_imap):
     messages they refer to and thus dropped.
     """
     (ac1,) = acfactory.get_online_accounts(1)
-    ac2 = acfactory.new_preconfigured_account()
+
-    ac2.configure()
+    addr, password = acfactory.get_credentials()
+    ac2 = acfactory.get_unconfigured_account()
+    ac2._rpc.add_transport(ac2.id, {"addr": addr, "password": password})
     ac2.set_config("mvbox_move", "1")
+    assert ac2.is_configured()
+
     ac2.bring_online()
     chat1 = acfactory.get_accepted_chat(ac1, ac2)
     ac2.stop_io()

src/configure.rs

@@ -144,7 +144,8 @@ impl Context {
             // We are using Anyhow's .context() and to show the
             // inner error, too, we need the {:#}:
             let error_msg = stock_str::configuration_failed(self, &format!("{err:#}")).await;
-            progress!(self, 0, Some(error_msg));
+            progress!(self, 0, Some(error_msg.clone()));
+            bail!(error_msg);
         } else {
             param.save(self).await?;
             progress!(self, 1000);
@@ -157,9 +158,22 @@ impl Context {
     /// using the server encoded in the QR code.
     /// See [Self::add_transport].
     pub async fn add_transport_from_qr(&self, qr: &str) -> Result<()> {
-        set_account_from_qr(self, qr).await?;
+        self.stop_io().await;
-        self.configure().await?;
 
+        let result = async move {
+            set_account_from_qr(self, qr).await?;
+            self.configure().await?;
+            Ok(())
+        }
+        .await;
+
+        if result.is_err() {
+            if let Ok(true) = self.is_configured().await {
+                self.start_io().await;
+            }
+            return result;
+        }
+        self.start_io().await;
         Ok(())
     }