From c990348e841ecb605c91fd7346aaefc893e098a0 Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 31 Jul 2026 10:02:29 -0500 Subject: [PATCH] refactor(mail): remove fake/dead protocol code paths Delete crates/nuncio-cal/src/carddav.rs, an orphaned duplicate CardDavClient/Contact that made no network call and was unused outside its own tests -- the real CardDAV client lives in nuncio-contacts. Make credential-less ImapEngine/JmapEngine sync_folders/sync_messages return an honest MailError::AuthError instead of returning hardcoded folders/messages. This also fixes a latent bug in ImapEngine::sync_folders: a genuinely empty real mailbox list previously fell through to fake canned folders instead of being returned as-is. Remove send_email from the MailBackend trait: production sends route through MessageSender/SmtpTransportEngine exclusively, and the IMAP/JMAP implementations were both no-op fabricated successes with no real transport behind them. Migrate the tests that exercised the removed fixture paths to real fixtures: a scripted loopback IMAP server drives ImapEngine::sync_folders through a real LOGIN/LIST/LOGOUT exchange, and credential-less calls are now asserted to fail honestly rather than fabricate data. Refs #253 --- crates/nuncio-cal/src/carddav.rs | 148 --------------------------- crates/nuncio-cal/src/lib.rs | 2 - crates/nuncio-mail/src/backend.rs | 3 - crates/nuncio-mail/src/imap.rs | 165 +++++++++++++++++------------- crates/nuncio-mail/src/jmap.rs | 126 ++++++++--------------- crates/nuncio-mail/src/mock.rs | 31 ------ 6 files changed, 137 insertions(+), 338 deletions(-) delete mode 100644 crates/nuncio-cal/src/carddav.rs diff --git a/crates/nuncio-cal/src/carddav.rs b/crates/nuncio-cal/src/carddav.rs deleted file mode 100644 index c83898b..0000000 --- a/crates/nuncio-cal/src/carddav.rs +++ /dev/null @@ -1,148 +0,0 @@ -//! CardDAV (vCard RFC 6350) contact sync and XML query parser. - -use crate::parser::CalendarError; -use serde::{Deserialize, Serialize}; - -/// Contact entity representation synced over CardDAV. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub struct Contact { - pub id: String, - pub name: String, - pub email: String, - pub phone: Option, - pub organization: Option, -} - -/// CardDAV client protocol engine managing vCard address book queries. -pub struct CardDavClient { - account_id: String, -} - -impl CardDavClient { - /// Create a new `CardDavClient` bound to an account ID. - pub fn new(account_id: &str) -> Self { - Self { - account_id: account_id.to_string(), - } - } - - /// Construct a standard CardDAV `` XML payload. - pub fn build_addressbook_query() -> String { - r#" - - - - - -"# - .to_string() - } - - /// Parse raw vCard string into domain [`Contact`] entity. - pub fn parse_vcard(id: &str, raw_vcard: &str) -> Result { - let mut name = String::new(); - let mut email = String::new(); - let mut phone = None; - let mut organization = None; - - for line in raw_vcard.lines() { - let line = line.trim(); - if let Some(val) = line.strip_prefix("FN:") { - name = val.trim().to_string(); - } else if let Some(val) = line.strip_prefix("EMAIL:") { - email = val.trim().to_string(); - } else if line.starts_with("EMAIL;") { - if let Some((_, val)) = line.split_once(':') { - email = val.trim().to_string(); - } - } else if let Some(val) = line.strip_prefix("TEL:") { - phone = Some(val.trim().to_string()); - } else if line.starts_with("TEL;") { - if let Some((_, val)) = line.split_once(':') { - phone = Some(val.trim().to_string()); - } - } else if let Some(val) = line.strip_prefix("ORG:") { - organization = Some(val.trim().to_string()); - } - } - - if name.is_empty() && email.is_empty() { - return Err(CalendarError::ParseFailed( - "invalid or empty vCard data".to_string(), - )); - } - - if name.is_empty() { - name = email.clone(); - } - - Ok(Contact { - id: id.to_string(), - name, - email, - phone, - organization, - }) - } - - /// Parse CardDAV WebDAV XML `` response containing embedded vCard data. - pub fn parse_multistatus_response(&self, raw_xml: &str) -> Result, CalendarError> { - let mut contacts = Vec::new(); - - for block in raw_xml.split("") { - if let Some((vcard_data, _)) = block.split_once("") { - let clean_vcard = vcard_data.trim(); - if !clean_vcard.is_empty() { - let contact_id = format!("{}-contact-{}", self.account_id, contacts.len() + 1); - if let Ok(contact) = Self::parse_vcard(&contact_id, clean_vcard) { - contacts.push(contact); - } - } - } - } - - Ok(contacts) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn parse_vcard_valid_structure() { - let vcard = r#"BEGIN:VCARD -VERSION:4.0 -FN:James Maes -EMAIL:james.maes@kof22.com -TEL:+1-555-0199 -ORG:Kof22 Labs -END:VCARD"#; - - let contact = CardDavClient::parse_vcard("c-1", vcard).expect("parse vcard"); - assert_eq!(contact.name, "James Maes"); - assert_eq!(contact.email, "james.maes@kof22.com"); - assert_eq!(contact.phone.as_deref(), Some("+1-555-0199")); - assert_eq!(contact.organization.as_deref(), Some("Kof22 Labs")); - } - - #[test] - fn parse_multistatus_carddav_xml() { - let client = CardDavClient::new("acct-1"); - let xml = r#" - - - BEGIN:VCARD -FN:Alice Dev -EMAIL:alice@nuncio.mx -END:VCARD - - "#; - - let contacts = client - .parse_multistatus_response(xml) - .expect("parse multistatus"); - assert_eq!(contacts.len(), 1); - assert_eq!(contacts[0].name, "Alice Dev"); - } -} diff --git a/crates/nuncio-cal/src/lib.rs b/crates/nuncio-cal/src/lib.rs index 58d3846..d9a72bf 100644 --- a/crates/nuncio-cal/src/lib.rs +++ b/crates/nuncio-cal/src/lib.rs @@ -5,7 +5,6 @@ pub mod backend; pub mod caldav; -pub mod carddav; pub mod mock; pub mod nlp; pub mod parser; @@ -14,7 +13,6 @@ pub mod scheduling; pub use backend::CalendarBackend; pub use caldav::{CalDavAccountConfig, CalDavClient}; -pub use carddav::{CardDavClient, Contact}; pub use mock::MockCalendarBackend; pub use nlp::{NaturalLanguageScheduler, NlpError, ParsedSchedulingIntent}; pub use parser::{CalendarError, IcalParserAdapter}; diff --git a/crates/nuncio-mail/src/backend.rs b/crates/nuncio-mail/src/backend.rs index 138c5ee..a766d4d 100644 --- a/crates/nuncio-mail/src/backend.rs +++ b/crates/nuncio-mail/src/backend.rs @@ -18,9 +18,6 @@ pub trait MailBackend: Send + Sync { folder_id: &str, since_state: Option<&str>, ) -> Result<(Vec, String), MailError>; - - /// Send an email message over the configured transport. - async fn send_email(&self, email: &Email) -> Result<(), MailError>; } /// A composed outbound email message ready to send over SMTP. Deliberately diff --git a/crates/nuncio-mail/src/imap.rs b/crates/nuncio-mail/src/imap.rs index 36e8a3c..2b2f50c 100644 --- a/crates/nuncio-mail/src/imap.rs +++ b/crates/nuncio-mail/src/imap.rs @@ -742,24 +742,9 @@ impl ImapEngine { let (username, password) = match (&self.username, &self.password) { (Some(u), Some(p)) => (u.as_str(), p.as_str()), _ => { - // Credential-less fixture path used by unit tests that never - // reach a real server. The checkpoint (one past the fixture's - // UID) is internally consistent with the fixture rather than - // derived from `emails.len()`. - let mock_emails = vec![Email { - id: "imap-uid-100".to_string(), - account_id: self.account_id.clone(), - folder_id: folder_id.to_string(), - subject: "IMAP Sync Message".to_string(), - sender: "sender@nuncio.mx".to_string(), - recipient: "me@nuncio.mx".to_string(), - received_at: 1700000000, - read: true, - body_plain: Some("IMAP message body content".to_string()), - body_html: None, - attachments: Vec::new(), - }]; - return Ok((mock_emails, "101".to_string())); + return Err(MailError::AuthError( + "IMAP message sync requires credentials".to_string(), + )) } }; @@ -875,47 +860,37 @@ impl ImapEngine { #[async_trait] impl MailBackend for ImapEngine { async fn sync_folders(&self) -> Result, MailError> { - if let (Some(u), Some(p)) = (&self.username, &self.password) { - let mut session = self.socket_manager.connect_session(u, p).await?; - let mut mailboxes = session - .list(None, Some("*")) - .await - .map_err(|e| MailError::ImapError(format!("failed to list mailboxes: {}", e)))?; - - let mut folders = Vec::new(); - while let Some(mb_res) = mailboxes.next().await { - let mb = mb_res.map_err(|e| { - MailError::ImapError(format!("failed reading mailbox item: {}", e)) - })?; - let folder_name = mb.name().to_string(); - folders.push(Folder { - id: folder_name.clone(), - name: folder_name, - total_messages: 0, - unread_messages: 0, - }); + let (u, p) = match (&self.username, &self.password) { + (Some(u), Some(p)) => (u.as_str(), p.as_str()), + _ => { + return Err(MailError::AuthError( + "IMAP folder sync requires credentials".to_string(), + )) } - drop(mailboxes); - let _ = session.logout().await; - if !folders.is_empty() { - return Ok(folders); - } - } + }; + let mut session = self.socket_manager.connect_session(u, p).await?; + let mut mailboxes = session + .list(None, Some("*")) + .await + .map_err(|e| MailError::ImapError(format!("failed to list mailboxes: {}", e)))?; - Ok(vec![ - Folder { - id: "INBOX".to_string(), - name: "Inbox".to_string(), - total_messages: 10, - unread_messages: 2, - }, - Folder { - id: "Sent".to_string(), - name: "Sent Messages".to_string(), - total_messages: 5, + let mut folders = Vec::new(); + while let Some(mb_res) = mailboxes.next().await { + let mb = mb_res + .map_err(|e| MailError::ImapError(format!("failed reading mailbox item: {}", e)))?; + let folder_name = mb.name().to_string(); + folders.push(Folder { + id: folder_name.clone(), + name: folder_name, + total_messages: 0, unread_messages: 0, - }, - ]) + }); + } + drop(mailboxes); + let _ = session.logout().await; + // An empty mailbox is a genuine result, not a signal to fall back to + // placeholder folders -- the caller gets exactly what the server reported. + Ok(folders) } async fn sync_messages( @@ -925,10 +900,6 @@ impl MailBackend for ImapEngine { ) -> Result<(Vec, String), MailError> { self.sync_folder_messages(folder_id, since_state).await } - - async fn send_email(&self, _email: &Email) -> Result<(), MailError> { - Ok(()) - } } #[cfg(test)] @@ -952,20 +923,72 @@ mod tests { } #[tokio::test] - async fn imap_engine_sync_folders_and_messages() -> Result<(), MailError> { + async fn sync_folders_and_sync_messages_require_credentials() { + // Without credentials there is no server to sync against; both + // methods must surface an honest auth error rather than fabricating + // folders or messages. let engine = ImapEngine::new("acct-1", "mail.kof22.com", 993); - let folders = engine.sync_folders().await?; + + let err = engine + .sync_folders() + .await + .expect_err("credential-less sync_folders must fail"); + assert!(matches!(err, MailError::AuthError(_))); + + let err = engine + .sync_messages("INBOX", None) + .await + .expect_err("credential-less sync_messages must fail"); + assert!(matches!(err, MailError::AuthError(_))); + } + + #[tokio::test] + async fn sync_folders_lists_real_mailboxes_over_a_scripted_server() { + // Drives `MailBackend::sync_folders` against a real, plain-TCP + // loopback IMAP server scripted to answer LOGIN/LIST/LOGOUT -- proving + // the returned folder list is genuinely read off the wire. + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind loopback listener"); + let addr = listener.local_addr().expect("listener addr"); + + let server = tokio::spawn(async move { + let (mut socket, _) = listener.accept().await.expect("accept connection"); + while let Some(line) = read_scripted_line(&mut socket).await { + let tag = line.split_whitespace().next().unwrap_or("").to_string(); + let upper = line.to_ascii_uppercase(); + if upper.contains("LOGIN") { + let _ = socket + .write_all(format!("{tag} OK Logged in\r\n").as_bytes()) + .await; + } else if upper.contains("LIST") { + let resp = format!( + "* LIST () \"/\" INBOX\r\n* LIST () \"/\" Archive\r\n{tag} OK LIST completed\r\n" + ); + let _ = socket.write_all(resp.as_bytes()).await; + } else if upper.contains("LOGOUT") { + let _ = socket + .write_all(format!("* BYE\r\n{tag} OK LOGOUT\r\n").as_bytes()) + .await; + break; + } + } + }); + + let engine = ImapEngine::with_credentials( + "acct-1", + "127.0.0.1", + addr.port(), + TlsMode::Plain, + "user", + "pass", + ); + let folders = engine.sync_folders().await.expect("sync_folders succeeds"); + server.await.expect("scripted server task completes"); + assert_eq!(folders.len(), 2); assert_eq!(folders[0].id, "INBOX"); - - let (emails, checkpoint) = engine.sync_messages("INBOX", None).await?; - // Credential-less fixture: checkpoint is one past the fixture UID. - assert_eq!(checkpoint, "101"); - assert_eq!(emails.len(), 1); - assert_eq!(emails[0].id, "imap-uid-100"); - - engine.send_email(&emails[0]).await?; - Ok(()) + assert_eq!(folders[1].id, "Archive"); } #[test] diff --git a/crates/nuncio-mail/src/jmap.rs b/crates/nuncio-mail/src/jmap.rs index 5ff136f..76fad28 100644 --- a/crates/nuncio-mail/src/jmap.rs +++ b/crates/nuncio-mail/src/jmap.rs @@ -408,44 +408,26 @@ impl JmapEngine { #[async_trait] impl MailBackend for JmapEngine { async fn sync_folders(&self) -> Result, MailError> { - if self.has_credentials() { - let session = self.discover_session().await?; - let account_id = self.resolve_account_id(&session); - let request = Self::build_mailbox_get_request(&account_id); - let raw = self.post_jmap(&session.api_url, &request).await?; - let mailboxes = Self::parse_mailbox_get_response(&raw)?; - - return Ok(mailboxes - .into_iter() - .map(|mb| Folder { - id: mb.id, - name: mb.name, - total_messages: mb.total_emails as usize, - unread_messages: mb.unread_emails as usize, - }) - .collect()); + if !self.has_credentials() { + return Err(MailError::AuthError( + "JMAP folder sync requires credentials".to_string(), + )); } + let session = self.discover_session().await?; + let account_id = self.resolve_account_id(&session); + let request = Self::build_mailbox_get_request(&account_id); + let raw = self.post_jmap(&session.api_url, &request).await?; + let mailboxes = Self::parse_mailbox_get_response(&raw)?; - Ok(vec![ - Folder { - id: "inbox".to_string(), - name: "Inbox".to_string(), - total_messages: 1, - unread_messages: 1, - }, - Folder { - id: "sent".to_string(), - name: "Sent".to_string(), - total_messages: 0, - unread_messages: 0, - }, - Folder { - id: "archive".to_string(), - name: "Archive".to_string(), - total_messages: 0, - unread_messages: 0, - }, - ]) + Ok(mailboxes + .into_iter() + .map(|mb| Folder { + id: mb.id, + name: mb.name, + total_messages: mb.total_emails as usize, + unread_messages: mb.unread_emails as usize, + }) + .collect()) } async fn sync_messages( @@ -453,46 +435,21 @@ impl MailBackend for JmapEngine { folder_id: &str, _since_state: Option<&str>, ) -> Result<(Vec, String), MailError> { - if self.has_credentials() { - let session = self.discover_session().await?; - let account_id = self.resolve_account_id(&session); - - let query_request = Self::build_email_query_request(&account_id, folder_id); - let query_raw = self.post_jmap(&session.api_url, &query_request).await?; - let ids = Self::parse_email_query_response(&query_raw)?; - - let get_request = Self::build_email_get_request(&account_id, Some(ids)); - let get_raw = self.post_jmap(&session.api_url, &get_request).await?; - return self.parse_email_get_response(&get_raw); - } - - let sample_jmap = json!({ - "state": "s-100", - "list": [ - { - "id": "jmap-msg-1", - "subject": "Welcome to JMAP Sync", - "from": [{ "email": "support@nuncio.mx" }], - "to": [{ "email": "user@nuncio.mx" }], - "receivedAt": 1700000000i64, - "isUnread": true, - "bodySnippet": "RFC 8620 / 8621 JMAP Sync Engine active." - } - ] - }) - .to_string(); - - let (emails, state) = self.parse_email_get_response(&sample_jmap)?; - Ok((emails, state)) - } - - async fn send_email(&self, email: &Email) -> Result<(), MailError> { - if email.recipient.is_empty() { - return Err(MailError::TransportFailed( - "missing recipient address".to_string(), + if !self.has_credentials() { + return Err(MailError::AuthError( + "JMAP message sync requires credentials".to_string(), )); } - Ok(()) + let session = self.discover_session().await?; + let account_id = self.resolve_account_id(&session); + + let query_request = Self::build_email_query_request(&account_id, folder_id); + let query_raw = self.post_jmap(&session.api_url, &query_request).await?; + let ids = Self::parse_email_query_response(&query_raw)?; + + let get_request = Self::build_email_get_request(&account_id, Some(ids)); + let get_raw = self.post_jmap(&session.api_url, &get_request).await?; + self.parse_email_get_response(&get_raw) } } @@ -622,20 +579,23 @@ mod tests { } #[tokio::test] - async fn uncredentialed_engine_keeps_static_fallback_behaviour() { + async fn uncredentialed_engine_requires_credentials_to_sync() { + // Without credentials there is no server to discover a session from; + // both methods must surface an honest auth error rather than + // fabricating folders or messages. let engine = JmapEngine::new("acct-1"); assert!(!engine.has_credentials()); - let folders = engine.sync_folders().await.expect("sync folders"); - assert_eq!(folders.len(), 3); - assert_eq!(folders[0].id, "inbox"); + let err = engine + .sync_folders() + .await + .expect_err("credential-less sync_folders must fail"); + assert!(matches!(err, MailError::AuthError(_))); - let (emails, state) = engine + let err = engine .sync_messages("inbox", None) .await - .expect("sync messages"); - assert_eq!(state, "s-100"); - assert_eq!(emails.len(), 1); - assert_eq!(emails[0].id, "jmap-msg-1"); + .expect_err("credential-less sync_messages must fail"); + assert!(matches!(err, MailError::AuthError(_))); } } diff --git a/crates/nuncio-mail/src/mock.rs b/crates/nuncio-mail/src/mock.rs index d2352fe..ec2d4f3 100644 --- a/crates/nuncio-mail/src/mock.rs +++ b/crates/nuncio-mail/src/mock.rs @@ -12,7 +12,6 @@ use crate::parser::MailError; pub struct MockMailBackend { folders: Arc>>, messages: Arc>>, - sent_messages: Arc>>, should_fail: Arc>, /// Every `since_state` argument this mock's `sync_messages` was called /// with, in call order, so tests can prove a caller threads the returned @@ -71,14 +70,6 @@ impl MockMailBackend { guard.push(email); } } - - /// Retrieve sent messages recorded by the mock. - pub fn sent_messages(&self) -> Vec { - self.sent_messages - .lock() - .map(|guard| guard.clone()) - .unwrap_or_default() - } } #[async_trait] @@ -144,24 +135,6 @@ impl MailBackend for MockMailBackend { .collect(); Ok((matches, returned_state)) } - - async fn send_email(&self, email: &Email) -> Result<(), MailError> { - let should_fail = self - .should_fail - .lock() - .map_err(|e| MailError::ParseFailed(e.to_string()))?; - if *should_fail { - return Err(MailError::ParseFailed( - "simulated network failure".to_string(), - )); - } - let mut sent = self - .sent_messages - .lock() - .map_err(|e| MailError::ParseFailed(e.to_string()))?; - sent.push(email.clone()); - Ok(()) - } } /// Deterministic mock [`MessageSender`] for offline testing of the outbound @@ -257,14 +230,10 @@ mod tests { assert_eq!(msgs.len(), 1); assert_eq!(state, "mock-state-token-100"); - mock.send_email(&email).await.expect("send succeeds"); - assert_eq!(mock.sent_messages().len(), 1); - // Test error simulation mock.set_should_fail(true); assert!(mock.sync_folders().await.is_err()); assert!(mock.sync_messages("inbox", None).await.is_err()); - assert!(mock.send_email(&email).await.is_err()); } fn sample_outbound_message() -> OutboundMessage { -- 2.54.0