From cc3e8c5117ab9f538970a9881b24f88d0fab3feb Mon Sep 17 00:00:00 2001 From: link2xt Date: Sat, 29 May 2021 00:00:00 +0300 Subject: [PATCH] imap: refactor to always create Imap configured `Imap` structure is always created in a configured state now. There is no default value for `ImapConfig` anymore. Also resultify Scheduler::start() to fail on database errors, for example if IMAP configuration cannot be read from the database during `start_io()`. Previosuly errors during reading keys such as `mvbox_watch` were simply ignored and folders were not watched until the application is completely restarted, now start_io() will fail and scheduler will only be started at the next start_io() call which usually happens when app is brought to the foreground. --- src/configure.rs | 66 ++++----- src/context.rs | 4 +- src/imap.rs | 297 ++++++++++++++++----------------------- src/imap/idle.rs | 4 +- src/imap/scan_folders.rs | 2 +- src/job.rs | 10 +- src/scheduler.rs | 34 ++--- 7 files changed, 183 insertions(+), 234 deletions(-) diff --git a/src/configure.rs b/src/configure.rs index 0bc5185b7..77566c05d 100644 --- a/src/configure.rs +++ b/src/configure.rs @@ -345,10 +345,8 @@ async fn configure(ctx: &Context, param: &mut LoginParam) -> Result<()> { progress!(ctx, 600); // Configure IMAP - let (_s, r) = async_std::channel::bounded(1); - let mut imap = Imap::new(r); - let mut imap_configured = false; + let mut imap: Option = None; let imap_servers: Vec<&ServerParams> = servers .iter() .filter(|params| params.protocol == Protocol::Imap) @@ -361,18 +359,9 @@ async fn configure(ctx: &Context, param: &mut LoginParam) -> Result<()> { param.imap.port = imap_server.port; param.imap.security = imap_server.socket; - match try_imap_one_param( - ctx, - ¶m.imap, - ¶m.addr, - oauth2, - provider_strict_tls, - &mut imap, - ) - .await - { - Ok(_) => { - imap_configured = true; + match try_imap_one_param(ctx, ¶m.imap, ¶m.addr, oauth2, provider_strict_tls).await { + Ok(configured_imap) => { + imap = Some(configured_imap); break; } Err(e) => errors.push(e), @@ -382,9 +371,10 @@ async fn configure(ctx: &Context, param: &mut LoginParam) -> Result<()> { 600 + (800 - 600) * (1 + imap_server_index) / imap_servers_count ); } - if !imap_configured { - bail!(nicer_configuration_error(ctx, errors).await); - } + let mut imap = match imap { + Some(imap) => imap, + None => bail!(nicer_configuration_error(ctx, errors).await), + }; progress!(ctx, 850); @@ -520,26 +510,38 @@ async fn try_imap_one_param( addr: &str, oauth2: bool, provider_strict_tls: bool, - imap: &mut Imap, -) -> Result<(), ConfigurationError> { +) -> Result { let inf = format!( "imap: {}@{}:{} security={} certificate_checks={} oauth2={}", param.user, param.server, param.port, param.security, param.certificate_checks, oauth2 ); info!(context, "Trying: {}", inf); - if let Err(err) = imap - .connect(context, param, addr, oauth2, provider_strict_tls) - .await - { - info!(context, "failure: {}", err); - Err(ConfigurationError { - config: inf, - msg: err.to_string(), - }) - } else { - info!(context, "success: {}", inf); - Ok(()) + let (_s, r) = async_std::channel::bounded(1); + + let mut imap = match Imap::new(param, addr, oauth2, provider_strict_tls, r).await { + Err(err) => { + info!(context, "failure: {}", err); + return Err(ConfigurationError { + config: inf, + msg: err.to_string(), + }); + } + Ok(imap) => imap, + }; + + match imap.connect(context).await { + Err(err) => { + info!(context, "failure: {}", err); + return Err(ConfigurationError { + config: inf, + msg: err.to_string(), + }); + } + Ok(()) => { + info!(context, "success: {}", inf); + return Ok(imap); + } } } diff --git a/src/context.rs b/src/context.rs index 0307e7444..ee12b6d04 100644 --- a/src/context.rs +++ b/src/context.rs @@ -161,7 +161,9 @@ impl Context { { let l = &mut *self.inner.scheduler.write().await; - l.start(self.clone()).await; + if let Err(err) = l.start(self.clone()).await { + error!(self, "Failed to start IO: {}", err) + } } } diff --git a/src/imap.rs b/src/imap.rs index 52e0eb21e..7159c901a 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -8,7 +8,7 @@ use std::{cmp, cmp::max, collections::BTreeMap}; use anyhow::{bail, format_err, Context as _, Result}; use async_imap::{ error::Result as ImapResult, - types::{Capability, Fetch, Flag, Mailbox, Name, NameAttribute}, + types::{Fetch, Flag, Mailbox, Name, NameAttribute}, }; use async_std::channel::Receiver; use async_std::prelude::*; @@ -92,6 +92,10 @@ pub struct Imap { interrupt: Option, should_reconnect: bool, login_failed_once: bool, + + /// True if CAPABILITY command was run successfully once and config.can_* contain correct + /// values. + capabilities_determined: bool, } #[derive(Debug)] @@ -141,6 +145,7 @@ struct ImapConfig { pub selected_folder: Option, pub selected_mailbox: Option, pub selected_folder_needs_expunge: bool, + pub can_idle: bool, /// True if the server has MOVE capability as defined in @@ -148,58 +153,93 @@ struct ImapConfig { pub can_move: bool, } -impl Default for ImapConfig { - fn default() -> Self { - ImapConfig { - addr: "".into(), - lp: Default::default(), - strict_tls: false, - oauth2: false, +impl Imap { + /// Creates new disconnected IMAP client using the specific login parameters. + /// + /// `addr` is used to renew token if OAuth2 authentication is used. + pub async fn new( + lp: &ServerLoginParam, + addr: &str, + oauth2: bool, + provider_strict_tls: bool, + idle_interrupt: Receiver, + ) -> Result { + if lp.server.is_empty() || lp.user.is_empty() || lp.password.is_empty() { + bail!("Incomplete IMAP connection parameters"); + } + + let strict_tls = match lp.certificate_checks { + CertificateChecks::Automatic => provider_strict_tls, + CertificateChecks::Strict => true, + CertificateChecks::AcceptInvalidCertificates + | CertificateChecks::AcceptInvalidCertificates2 => false, + }; + let config = ImapConfig { + addr: addr.to_string(), + lp: lp.clone(), + strict_tls, + oauth2, selected_folder: None, selected_mailbox: None, selected_folder_needs_expunge: false, can_idle: false, can_move: false, - } - } -} + }; -impl Imap { - pub fn new(idle_interrupt: Receiver) -> Self { - Imap { + let imap = Imap { idle_interrupt, - config: Default::default(), - session: Default::default(), - connected: Default::default(), - interrupt: Default::default(), - should_reconnect: Default::default(), - login_failed_once: Default::default(), + config, + session: None, + connected: false, + interrupt: None, + should_reconnect: false, + login_failed_once: false, + capabilities_determined: false, + }; + + Ok(imap) + } + + /// Creates new disconnected IMAP client using configured parameters. + pub async fn new_configured( + context: &Context, + idle_interrupt: Receiver, + ) -> Result { + if !context.is_configured().await? { + bail!("IMAP Connect without configured params"); } - } - pub fn is_connected(&self) -> bool { - self.connected - } + let param = LoginParam::from_database(context, "configured_").await?; + // the trailing underscore is correct - pub fn should_reconnect(&self) -> bool { - self.should_reconnect - } - - pub fn trigger_reconnect(&mut self) { - self.should_reconnect = true; + let imap = Self::new( + ¶m.imap, + ¶m.addr, + param.server_flags & DC_LP_AUTH_OAUTH2 != 0, + param.provider.map_or(false, |provider| provider.strict_tls), + idle_interrupt, + ) + .await?; + Ok(imap) } /// Connects or reconnects if needed. /// - /// It is safe to call this function if already connected, actions - /// are performed only as needed. - async fn try_setup_handle(&mut self, context: &Context) -> Result<()> { + /// It is safe to call this function if already connected, actions are performed only as needed. + /// + /// Does not emit network errors, can be used to try various parameters during + /// autoconfiguration. + /// + /// Calling this function is not enough to perform IMAP operations. Use [`Imap::prepare`] + /// instead if you are going to actually use connection rather than trying connection + /// parameters. + pub async fn connect(&mut self, context: &Context) -> Result<()> { if self.config.lp.server.is_empty() { bail!("IMAP operation attempted while it is torn down"); } if self.should_reconnect() { - self.unsetup_handle(context).await; + self.disconnect(context).await; self.should_reconnect = false; } else if self.is_connected() { return Ok(()); @@ -269,6 +309,10 @@ impl Imap { self.connected = true; self.session = Some(session); self.login_failed_once = false; + emit_event!( + context, + EventType::ImapConnected(format!("IMAP-LOGIN as {}", self.config.lp.user)) + ); Ok(()) } @@ -305,20 +349,49 @@ impl Imap { } } - /// Connects or reconnects if not already connected. + /// Determine server capabilities if not done yet. + async fn determine_capabilities(&mut self) -> Result<()> { + if self.capabilities_determined { + return Ok(()); + } + + match &mut self.session { + Some(ref mut session) => match session.capabilities().await { + Ok(caps) => { + self.config.can_idle = caps.has_str("IDLE"); + self.config.can_move = caps.has_str("MOVE"); + self.capabilities_determined = true; + Ok(()) + } + Err(err) => { + bail!("CAPABILITY command error: {}", err); + } + }, + None => { + bail!("Can't determine server capabilities because connection was not established") + } + } + } + + /// Prepare for IMAP operation. /// - /// This function emits network error if it fails. It should not - /// be used during configuration to avoid showing failed attempt - /// errors to the user. - async fn setup_handle(&mut self, context: &Context) -> Result<()> { - let res = self.try_setup_handle(context).await; + /// Ensure that IMAP client is connected, folders are created and IMAP capabilities are + /// determined. + /// + /// This function emits network error if it fails. It should not be used during configuration + /// to avoid showing failed attempt errors to the user. + pub async fn prepare(&mut self, context: &Context) -> Result<()> { + let res = self.connect(context).await; if let Err(ref err) = res { emit_event!(context, EventType::ErrorNetwork(err.to_string())); } + + self.ensure_configured_folders(context, true).await?; + self.determine_capabilities().await?; res } - async fn unsetup_handle(&mut self, context: &Context) { + async fn disconnect(&mut self, context: &Context) { // Close folder if messages should be expunged if let Err(err) = self.close_folder(context).await { warn!(context, "failed to close folder: {:?}", err); @@ -331,139 +404,21 @@ impl Imap { } } self.connected = false; + self.capabilities_determined = false; self.config.selected_folder = None; self.config.selected_mailbox = None; } - async fn free_connect_params(&mut self) { - let mut cfg = &mut self.config; - - cfg.addr = "".into(); - cfg.lp = Default::default(); - - cfg.can_idle = false; - cfg.can_move = false; + pub fn is_connected(&self) -> bool { + self.connected } - /// Connects to IMAP account using already-configured parameters. - /// - /// Emits network error if connection fails. - pub async fn connect_configured(&mut self, context: &Context) -> Result<()> { - if self.is_connected() && !self.should_reconnect() { - return Ok(()); - } - if !context.is_configured().await? { - bail!("IMAP Connect without configured params"); - } - - let param = LoginParam::from_database(context, "configured_").await?; - // the trailing underscore is correct - - if let Err(err) = self - .connect( - context, - ¶m.imap, - ¶m.addr, - param.server_flags & DC_LP_AUTH_OAUTH2 != 0, - param.provider.map_or(false, |provider| provider.strict_tls), - ) - .await - { - bail!("IMAP Connection Failed with params {}: {}", param, err); - } else { - self.ensure_configured_folders(context, true).await - } + pub fn should_reconnect(&self) -> bool { + self.should_reconnect } - /// Tries connecting to imap account using the specific login parameters. - /// - /// `addr` is used to renew token if OAuth2 authentication is used. - /// - /// Does not emit network errors, can be used to try various - /// parameters during autoconfiguration. - pub async fn connect( - &mut self, - context: &Context, - lp: &ServerLoginParam, - addr: &str, - oauth2: bool, - provider_strict_tls: bool, - ) -> Result<()> { - if lp.server.is_empty() || lp.user.is_empty() || lp.password.is_empty() { - bail!("Incomplete IMAP connection parameters"); - } - - { - let mut config = &mut self.config; - config.addr = addr.to_string(); - config.lp = lp.clone(); - config.strict_tls = match lp.certificate_checks { - CertificateChecks::Automatic => provider_strict_tls, - CertificateChecks::Strict => true, - CertificateChecks::AcceptInvalidCertificates - | CertificateChecks::AcceptInvalidCertificates2 => false, - }; - config.oauth2 = oauth2; - } - - if let Err(err) = self.try_setup_handle(context).await { - warn!(context, "try_setup_handle: {}", err); - self.free_connect_params().await; - return Err(err); - } - - let teardown = match &mut self.session { - Some(ref mut session) => match session.capabilities().await { - Ok(caps) => { - if !context.sql.is_open().await { - warn!(context, "IMAP-LOGIN as {} ok but ABORTING", lp.user,); - true - } else { - let can_idle = caps.has_str("IDLE"); - let can_move = caps.has_str("MOVE"); - let caps_list = caps.iter().fold(String::new(), |s, c| { - if let Capability::Atom(x) = c { - s + &format!(" {}", x) - } else { - s + &format!(" {:?}", c) - } - }); - - self.config.can_idle = can_idle; - self.config.can_move = can_move; - self.connected = true; - emit_event!( - context, - EventType::ImapConnected(format!( - "IMAP-LOGIN as {}, capabilities: {}", - lp.user, caps_list, - )) - ); - false - } - } - Err(err) => { - info!(context, "CAPABILITY command error: {}", err); - true - } - }, - None => true, - }; - - if teardown { - self.disconnect(context).await; - - warn!( - context, - "IMAP disconnected immediately after connecting due to error" - ); - } - Ok(()) - } - - pub async fn disconnect(&mut self, context: &Context) { - self.unsetup_handle(context).await; - self.free_connect_params().await; + pub fn trigger_reconnect(&mut self) { + self.should_reconnect = true; } pub async fn fetch(&mut self, context: &Context, watch_folder: &str) -> Result<()> { @@ -471,7 +426,7 @@ impl Imap { // probably shutdown bail!("IMAP operation attempted while it is torn down"); } - self.setup_handle(context).await?; + self.prepare(context).await?; while self .fetch_new_messages(context, &watch_folder, false) @@ -988,10 +943,6 @@ impl Imap { (last_uid, read_errors) } - pub async fn can_move(&self) -> bool { - self.config.can_move - } - pub async fn mv( &mut self, context: &Context, @@ -1016,7 +967,7 @@ impl Imap { let set = format!("{}", uid); let display_folder_id = format!("{}/{}", folder, uid); - if self.can_move().await { + if self.config.can_move { if let Some(ref mut session) = &mut self.session { match session.uid_mv(&set, &dest_folder).await { Ok(_) => { @@ -1142,7 +1093,7 @@ impl Imap { // TODO: make INBOX/SENT/MVBOX perform the jobs on their // respective folders to avoid select_folder network traffic // and the involved error states - if let Err(err) = self.connect_configured(context).await { + if let Err(err) = self.prepare(context).await { warn!(context, "prepare_imap_op failed: {}", err); return Some(ImapActionResult::RetryLater); } diff --git a/src/imap/idle.rs b/src/imap/idle.rs index 08fa1507c..ce59e2be4 100644 --- a/src/imap/idle.rs +++ b/src/imap/idle.rs @@ -25,7 +25,7 @@ impl Imap { if !self.can_idle() { bail!("IMAP server does not have IDLE capability"); } - self.setup_handle(context).await?; + self.prepare(context).await?; self.select_folder(context, watch_folder.as_deref()).await?; @@ -158,7 +158,7 @@ impl Imap { // try to connect with proper login params // (setup_handle_if_needed might not know about them if we // never successfully connected) - if let Err(err) = self.connect_configured(context).await { + if let Err(err) = self.prepare(context).await { warn!(context, "fake_idle: could not connect: {}", err); continue; } diff --git a/src/imap/scan_folders.rs b/src/imap/scan_folders.rs index 325868388..5c7d982f9 100644 --- a/src/imap/scan_folders.rs +++ b/src/imap/scan_folders.rs @@ -25,7 +25,7 @@ impl Imap { } info!(context, "Starting full folder scan"); - self.connect_configured(context).await?; + self.prepare(context).await?; let session = self.session.as_mut(); let session = session.context("scan_folders(): IMAP No Connection established")?; let folders: Vec<_> = session.list(Some(""), Some("*")).await?.collect().await; diff --git a/src/job.rs b/src/job.rs index e7d4871e9..c7191a5d7 100644 --- a/src/job.rs +++ b/src/job.rs @@ -532,7 +532,7 @@ impl Job { } async fn move_msg(&mut self, context: &Context, imap: &mut Imap) -> Status { - if let Err(err) = imap.connect_configured(context).await { + if let Err(err) = imap.prepare(context).await { warn!(context, "could not connect: {:?}", err); return Status::RetryLater; } @@ -594,7 +594,7 @@ impl Job { /// records pointing to the same message on the server, the job /// also removes the message on the server. async fn delete_msg_on_imap(&mut self, context: &Context, imap: &mut Imap) -> Status { - if let Err(err) = imap.connect_configured(context).await { + if let Err(err) = imap.prepare(context).await { warn!(context, "could not connect: {:?}", err); return Status::RetryLater; } @@ -682,7 +682,7 @@ impl Job { if job_try!(context.get_config_bool(Config::Bot).await) { return Status::Finished(Ok(())); // Bots don't want those messages } - if let Err(err) = imap.connect_configured(context).await { + if let Err(err) = imap.prepare(context).await { warn!(context, "could not connect: {:?}", err); return Status::RetryLater; } @@ -755,7 +755,7 @@ impl Job { /// Chat in contrast to the Sent folder, which is normally managed /// by the user via webmail or another email client. async fn resync_folders(&mut self, context: &Context, imap: &mut Imap) -> Status { - if let Err(err) = imap.connect_configured(context).await { + if let Err(err) = imap.prepare(context).await { warn!(context, "could not connect: {:?}", err); return Status::RetryLater; } @@ -779,7 +779,7 @@ impl Job { } async fn markseen_msg_on_imap(&mut self, context: &Context, imap: &mut Imap) -> Status { - if let Err(err) = imap.connect_configured(context).await { + if let Err(err) = imap.prepare(context).await { warn!(context, "could not connect: {:?}", err); return Status::RetryLater; } diff --git a/src/scheduler.rs b/src/scheduler.rs index cd87669f4..c96510d70 100644 --- a/src/scheduler.rs +++ b/src/scheduler.rs @@ -1,3 +1,4 @@ +use anyhow::{bail, Result}; use async_std::prelude::*; use async_std::{ channel::{self, Receiver, Sender}, @@ -130,7 +131,7 @@ async fn inbox_loop(ctx: Context, started: Sender<()>, inbox_handlers: ImapConne async fn fetch(ctx: &Context, connection: &mut Imap) { match ctx.get_config(Config::ConfiguredInboxFolder).await { Ok(Some(watch_folder)) => { - if let Err(err) = connection.connect_configured(ctx).await { + if let Err(err) = connection.prepare(ctx).await { error_network!(ctx, "{}", err); return; } @@ -159,7 +160,7 @@ async fn fetch_idle(ctx: &Context, connection: &mut Imap, folder: Config) -> Int match ctx.get_config(folder).await { Ok(Some(watch_folder)) => { // connect and fake idle if unable to connect - if let Err(err) = connection.connect_configured(ctx).await { + if let Err(err) = connection.prepare(ctx).await { warn!(ctx, "imap connection failed: {}", err); return connection.fake_idle(ctx, Some(watch_folder)).await; } @@ -301,11 +302,11 @@ async fn smtp_loop(ctx: Context, started: Sender<()>, smtp_handlers: SmtpConnect impl Scheduler { /// Start the scheduler, panics if it is already running. - pub async fn start(&mut self, ctx: Context) { - let (mvbox, mvbox_handlers) = ImapConnectionState::new(); - let (sentbox, sentbox_handlers) = ImapConnectionState::new(); + pub async fn start(&mut self, ctx: Context) -> Result<()> { + let (mvbox, mvbox_handlers) = ImapConnectionState::new(&ctx).await?; + let (sentbox, sentbox_handlers) = ImapConnectionState::new(&ctx).await?; let (smtp, smtp_handlers) = SmtpConnectionState::new(); - let (inbox, inbox_handlers) = ImapConnectionState::new(); + let (inbox, inbox_handlers) = ImapConnectionState::new(&ctx).await?; let (inbox_start_send, inbox_start_recv) = channel::bounded(1); let (mvbox_start_send, mvbox_start_recv) = channel::bounded(1); @@ -321,11 +322,7 @@ impl Scheduler { })) }; - if ctx - .get_config_bool(Config::MvboxWatch) - .await - .unwrap_or_default() - { + if ctx.get_config_bool(Config::MvboxWatch).await? { let ctx = ctx.clone(); mvbox_handle = Some(task::spawn(async move { simple_imap_loop( @@ -343,11 +340,7 @@ impl Scheduler { .expect("mvbox start send, missing receiver"); } - if ctx - .get_config_bool(Config::SentboxWatch) - .await - .unwrap_or_default() - { + if ctx.get_config_bool(Config::SentboxWatch).await? { let ctx = ctx.clone(); sentbox_handle = Some(task::spawn(async move { simple_imap_loop( @@ -391,10 +384,11 @@ impl Scheduler { .try_join(smtp_start_recv.recv()) .await { - error!(ctx, "failed to start scheduler: {}", err); + bail!("failed to start scheduler: {}", err); } info!(ctx, "scheduler is running"); + Ok(()) } async fn maybe_network(&self) { @@ -588,13 +582,13 @@ pub(crate) struct ImapConnectionState { impl ImapConnectionState { /// Construct a new connection. - fn new() -> (Self, ImapConnectionHandlers) { + async fn new(context: &Context) -> Result<(Self, ImapConnectionHandlers)> { let (stop_sender, stop_receiver) = channel::bounded(1); let (shutdown_sender, shutdown_receiver) = channel::bounded(1); let (idle_interrupt_sender, idle_interrupt_receiver) = channel::bounded(1); let handlers = ImapConnectionHandlers { - connection: Imap::new(idle_interrupt_receiver), + connection: Imap::new_configured(context, idle_interrupt_receiver).await?, stop_receiver, shutdown_sender, }; @@ -607,7 +601,7 @@ impl ImapConnectionState { let conn = ImapConnectionState { state }; - (conn, handlers) + Ok((conn, handlers)) } /// Interrupt any form of idle.