diff --git a/crates/nuncio-cli/src/args.rs b/crates/nuncio-cli/src/args.rs index abd2f38..96bba71 100644 --- a/crates/nuncio-cli/src/args.rs +++ b/crates/nuncio-cli/src/args.rs @@ -258,6 +258,12 @@ pub enum AccountSubcommand { /// Updated SMTP server port. #[arg(long, help = "Updated SMTP server port")] smtp_port: Option, + /// Updated IMAP transport mode (implicit_tls, start_tls, plain). + #[arg(long, help = "Updated IMAP transport mode")] + imap_mode: Option, + /// Updated SMTP transport mode (implicit_tls, start_tls, plain). + #[arg(long, help = "Updated SMTP transport mode")] + smtp_mode: Option, }, /// Remove a configured account profile. Delete { diff --git a/crates/nuncio-cli/src/runner.rs b/crates/nuncio-cli/src/runner.rs index e5a3a53..68c34f3 100644 --- a/crates/nuncio-cli/src/runner.rs +++ b/crates/nuncio-cli/src/runner.rs @@ -291,45 +291,32 @@ impl HeadlessRunner { ) .await } - AccountSubcommand::Show { id } => { - let accounts = self.db.list_accounts().await.unwrap_or_default(); - let acct = accounts.into_iter().find(|a| a.id == *id); - if json_mode { - format_json(&json!({ "account": acct })) - } else if let Some(a) = acct { - format!("Account {}: Email: {}", a.id, a.email_address) - } else { - format!("Account '{}' not found", id) - } - } + AccountSubcommand::Show { id } => self.handle_account_show(id, json_mode).await, AccountSubcommand::Edit { id, email, - imap_host: _, - imap_port: _, - smtp_host: _, - smtp_port: _, + imap_host, + imap_port, + smtp_host, + smtp_port, + imap_mode, + smtp_mode, } => { - if json_mode { - format_json(&json!({ "status": "updated", "id": id, "email": email })) - } else { - format!("Account '{}' updated successfully.", id) - } - } - AccountSubcommand::Delete { id } => { - if json_mode { - format_json(&json!({ "status": "deleted", "id": id })) - } else { - format!("Account '{}' removed.", id) - } - } - AccountSubcommand::Test { id } => { - if json_mode { - format_json(&json!({ "status": "ok", "id": id, "latency_ms": 24 })) - } else { - format!("✓ Account '{}' connection test OK (24ms latency).", id) - } + self.handle_account_edit( + id, + email.as_deref(), + imap_host.as_deref(), + *imap_port, + smtp_host.as_deref(), + *smtp_port, + imap_mode.as_deref(), + smtp_mode.as_deref(), + json_mode, + ) + .await } + AccountSubcommand::Delete { id } => self.handle_account_delete(id, json_mode).await, + AccountSubcommand::Test { id } => self.handle_account_test(id, json_mode).await, }, Commands::Mail { action } => match action { MailSubcommand::Sync => self.handle_sync(json_mode).await, @@ -1753,6 +1740,229 @@ impl HeadlessRunner { } } + /// Fetches every account from the daemon and returns the one matching + /// `id`, or an error string. There is deliberately no single-account + /// `GetAccount` RPC in the contract, so `account show`/`edit` filter the + /// `ListAccounts` result client-side -- always over the real daemon API, + /// never this runner's own ephemeral local `db`. + async fn fetch_account_by_id( + &self, + id: &str, + ) -> Result { + let mut client = self.connect_accounts_client().await?; + let accounts = client + .list_accounts(nuncio_proto::v1::ListAccountsRequest {}) + .await + .map_err(|status| format!("nunciod daemon rejected list_accounts: {status}"))? + .into_inner() + .accounts; + accounts + .into_iter() + .find(|a| a.id == id) + .ok_or_else(|| format!("account '{id}' not found")) + } + + /// `account show`: a real thin gRPC client of the daemon's + /// `nuncio.v1.Accounts/ListAccounts` API, filtered client-side to a single + /// account. Reads ONLY the daemon's persistent store -- never this + /// runner's own ephemeral local `db`. + async fn handle_account_show(&self, id: &str, json_mode: bool) -> String { + let account = match self.fetch_account_by_id(id).await { + Ok(account) => account, + Err(e) => return Self::render_error(&e, json_mode), + }; + + if json_mode { + format_json(&json!({ + "id": account.id, + "name": account.name, + "email_address": account.email_address, + "protocol": account.protocol().as_str_name(), + "server_host": account.server_host, + "server_port": account.server_port, + "smtp_host": account.smtp_host, + "smtp_port": account.smtp_port, + "use_tls": account.use_tls, + "imap_tls_mode": account.imap_tls_mode().as_str_name(), + "smtp_tls_mode": account.smtp_tls_mode().as_str_name(), + "keyring_secret_key": account.keyring_secret_key, + "sync_interval_secs": account.sync_interval_secs, + })) + } else { + format!( + "Account [{}] {} <{}> IMAP {}:{} ({}) SMTP {}:{} ({})", + account.id, + account.name, + account.email_address, + account.server_host, + account.server_port, + account.imap_tls_mode().as_str_name(), + account.smtp_host, + account.smtp_port, + account.smtp_tls_mode().as_str_name(), + ) + } + } + + /// `account edit`: a real thin gRPC client of the daemon's + /// `nuncio.v1.Accounts/UpdateAccount` API. Fetches the existing account, + /// applies ONLY the flags actually supplied onto it, and persists the + /// merged config through the daemon. No password is sent, so the existing + /// keyring credential is preserved (see `UpdateAccountRequest`). + #[allow(clippy::too_many_arguments)] + async fn handle_account_edit( + &self, + id: &str, + email: Option<&str>, + imap_host: Option<&str>, + imap_port: Option, + smtp_host: Option<&str>, + smtp_port: Option, + imap_mode: Option<&str>, + smtp_mode: Option<&str>, + json_mode: bool, + ) -> String { + let mut config = match self.fetch_account_by_id(id).await { + Ok(config) => config, + Err(e) => return Self::render_error(&e, json_mode), + }; + + if let Some(email) = email { + config.email_address = email.to_string(); + } + if let Some(host) = imap_host { + config.server_host = host.to_string(); + } + if let Some(port) = imap_port { + config.server_port = u32::from(port); + } + if let Some(host) = smtp_host { + config.smtp_host = host.to_string(); + } + if let Some(port) = smtp_port { + config.smtp_port = u32::from(port); + } + if let Some(mode) = imap_mode { + match parse_tls_mode(mode) { + Ok(mode) => config.imap_tls_mode = map_tls_mode_to_proto(mode).into(), + Err(e) => return Self::render_error(&e, json_mode), + } + } + if let Some(mode) = smtp_mode { + match parse_tls_mode(mode) { + Ok(mode) => config.smtp_tls_mode = map_tls_mode_to_proto(mode).into(), + Err(e) => return Self::render_error(&e, json_mode), + } + } + + let mut client = match self.connect_accounts_client().await { + Ok(client) => client, + Err(e) => return Self::render_error(&e, json_mode), + }; + + match client + .update_account(nuncio_proto::v1::UpdateAccountRequest { + config: Some(config), + password: None, + }) + .await + { + Ok(_) => { + if json_mode { + format_json(&json!({ "status": "updated", "id": id })) + } else { + format!("Account '{id}' updated via nunciod daemon.") + } + } + Err(status) => Self::render_error( + &format!("nunciod daemon rejected update_account: {status}"), + json_mode, + ), + } + } + + /// `account delete`: a real thin gRPC client of the daemon's + /// `nuncio.v1.Accounts/RemoveAccount` API -- genuinely removes the + /// persisted account and its keyring credential, never a fabricated + /// "removed." string. + async fn handle_account_delete(&self, id: &str, json_mode: bool) -> String { + let mut client = match self.connect_accounts_client().await { + Ok(client) => client, + Err(e) => return Self::render_error(&e, json_mode), + }; + + match client + .remove_account(nuncio_proto::v1::RemoveAccountRequest { id: id.to_string() }) + .await + { + Ok(_) => { + if json_mode { + format_json(&json!({ "status": "deleted", "id": id })) + } else { + format!("Account '{id}' removed via nunciod daemon.") + } + } + Err(status) => Self::render_error( + &format!("nunciod daemon rejected remove_account: {status}"), + json_mode, + ), + } + } + + /// `account test`: a real thin gRPC client of the daemon's + /// `nuncio.v1.Accounts/TestAccountConnection` API. Reports the GENUINE + /// per-protocol outcome the daemon measured -- never a fabricated + /// "connection test OK (24ms latency)". The two legs verify different + /// depths (see the RPC's proto doc): IMAP is a full login, SMTP is + /// transport reachability only (no AUTH), so the output labels them + /// distinctly and never implies SMTP authentication was verified. + async fn handle_account_test(&self, id: &str, json_mode: bool) -> String { + let mut client = match self.connect_accounts_client().await { + Ok(client) => client, + Err(e) => return Self::render_error(&e, json_mode), + }; + + match client + .test_account_connection(nuncio_proto::v1::TestAccountConnectionRequest { + id: id.to_string(), + }) + .await + { + Ok(response) => { + let report = response.into_inner(); + if json_mode { + format_json(&json!({ + "id": id, + "imap_ok": report.imap_ok, + "smtp_ok": report.smtp_ok, + "imap_error": report.imap_error, + "smtp_error": report.smtp_error, + })) + } else { + // Label each leg by what it actually verifies: IMAP is a + // full login, SMTP is transport reachability only (no + // AUTH). The wording must never imply SMTP authentication + // was checked when it was not. + let imap = match &report.imap_error { + Some(e) => format!("IMAP (login): FAIL ({e})"), + None if report.imap_ok => "IMAP (login): OK".to_string(), + None => "IMAP (login): FAIL".to_string(), + }; + let smtp = match &report.smtp_error { + Some(e) => format!("SMTP (reachability, no auth): FAIL ({e})"), + None if report.smtp_ok => "SMTP (reachability, no auth): OK".to_string(), + None => "SMTP (reachability, no auth): FAIL".to_string(), + }; + format!("Connection test for '{id}'\n {imap}\n {smtp}") + } + } + Err(status) => Self::render_error( + &format!("nunciod daemon rejected test_account_connection: {status}"), + json_mode, + ), + } + } + /// Resolves the gRPC bearer token from the injected vault and dials the /// `nuncio.v1.Accounts` service at `self.grpc_addr`, shared by /// [`Self::handle_add_account`] and [`Self::handle_accounts_list`]. @@ -2246,60 +2456,6 @@ mod tests { assert!(bad_smtp_mode.starts_with("Error: invalid tls mode")); } - #[tokio::test] - async fn headless_runner_executes_all_pure_noun_verb_commands() { - let runner = HeadlessRunner::ephemeral() - .await - .expect("ephemeral runner initializes"); - - // Account Noun Commands: `Add`/`List` are exercised separately - // below via `ephemeral_with` + `SecretManager::mock()` against a - // live test gRPC server, for the exact same reason `system status` - // is: they are real gRPC clients of the `nunciod` daemon's - // `Accounts` API, so they must never run against this - // `ephemeral()`-constructed runner's production `SecretManager` or - // its (unreachable in CI) default gRPC address. `Show` still reads - // this runner's own ephemeral local `db`, so it is safe to - // exercise here. - let acct_show = runner - .execute_command( - &Commands::Account { - action: AccountSubcommand::Show { - id: "missing".to_string(), - }, - }, - false, - ) - .await; - assert!(acct_show.contains("Account 'missing' not found")); - - // Mail Noun Commands: `Sync`/`List`/`Read`/`Search`/`Mark`/`Send` - // (and `Folder::List`) are all real gRPC clients of the `nunciod` - // daemon's `Mail` API -- exactly like `Account::Add`/`List` and - // `System::Status` above, they must never - // run against this `ephemeral()`-constructed runner's production - // `SecretManager` or its (unreachable in CI) default gRPC address. - // They are exercised separately below via `ephemeral_with` + - // `SecretManager::mock()` against a live stub `Mail` gRPC server - // (`mail_rpcs_round_trip_over_grpc_to_a_stub_daemon`). - - // Cal Noun Commands: `List`/`Sync` are real gRPC clients of the - // `nunciod` daemon's `Calendar` API -- exactly like the Mail Noun - // Commands above, they must never run against this - // `ephemeral()`-constructed runner's production `SecretManager` or - // its (unreachable in CI) default gRPC address. They are exercised - // separately below via `ephemeral_with` + `SecretManager::mock()` - // against a live stub `Calendar` gRPC server - // (`calendar_rpcs_round_trip_over_grpc_to_a_stub_daemon`). - - // System Noun Commands are exercised separately below via - // `ephemeral_with` + `SecretManager::mock()`: `system status` is a - // real gRPC client of the `nunciod` daemon, so it must never run - // against the production - // `SecretManager` this `ephemeral()`-constructed runner holds (that - // would touch the real OS keyring during a test run). - } - /// Binds an ephemeral loopback TCP listener, reads back its OS-assigned /// address, then immediately drops the listener so the port is free /// again. Nothing is listening on the returned address, so connecting @@ -2447,7 +2603,9 @@ mod tests { use nuncio_proto::v1::{ AccountConfig as AccountConfigProto, AccountProtocol as AccountProtocolProto, AddAccountRequest, AddAccountResponse, ListAccountsRequest, ListAccountsResponse, - TlsMode as TlsModeProto, + RemoveAccountRequest, RemoveAccountResponse, TestAccountConnectionRequest, + TestAccountConnectionResponse, TlsMode as TlsModeProto, UpdateAccountRequest, + UpdateAccountResponse, }; use std::sync::Mutex; @@ -2479,6 +2637,32 @@ mod tests { Ok(tonic::Response::new(AddAccountResponse { id })) } + async fn update_account( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Ok(tonic::Response::new(UpdateAccountResponse {})) + } + + async fn remove_account( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Ok(tonic::Response::new(RemoveAccountResponse {})) + } + + async fn test_account_connection( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Ok(tonic::Response::new(TestAccountConnectionResponse { + imap_ok: true, + smtp_ok: true, + imap_error: None, + smtp_error: None, + })) + } + async fn list_accounts( &self, _request: tonic::Request, @@ -2584,6 +2768,90 @@ mod tests { // Plain output must show each account's details, not just the count. assert!(list_out_text.contains("stub@nuncio.mx")); assert!(list_out_text.contains("imap.nuncio.mx")); + + // `account show` reads the daemon's ListAccounts result (filtered + // client-side), not the runner's own ephemeral local db. + let show_out = runner + .execute_command( + &Commands::Account { + action: AccountSubcommand::Show { + id: "acct-stub-1".to_string(), + }, + }, + true, + ) + .await; + assert!(show_out.contains("stub@nuncio.mx")); + assert!(show_out.contains("acct-stub-1")); + + // `account edit` drives the real UpdateAccount RPC (stub accepts it). + let edit_out = runner + .execute_command( + &Commands::Account { + action: AccountSubcommand::Edit { + id: "acct-stub-1".to_string(), + email: None, + imap_host: None, + imap_port: None, + smtp_host: None, + smtp_port: None, + imap_mode: Some("start_tls".to_string()), + smtp_mode: None, + }, + }, + true, + ) + .await; + assert!(edit_out.contains(r#""status":"updated""#)); + + // A garbage transport mode is rejected client-side before any RPC. + let bad_edit = runner + .execute_command( + &Commands::Account { + action: AccountSubcommand::Edit { + id: "acct-stub-1".to_string(), + email: None, + imap_host: None, + imap_port: None, + smtp_host: None, + smtp_port: None, + imap_mode: Some("nonsense".to_string()), + smtp_mode: None, + }, + }, + true, + ) + .await; + assert!(bad_edit.contains("invalid tls mode")); + + // `account test` reports the daemon's genuine per-protocol result -- + // never a fabricated "OK (24ms latency)". + let test_out = runner + .execute_command( + &Commands::Account { + action: AccountSubcommand::Test { + id: "acct-stub-1".to_string(), + }, + }, + true, + ) + .await; + assert!(test_out.contains(r#""imap_ok":true"#)); + assert!(test_out.contains(r#""smtp_ok":true"#)); + assert!(!test_out.contains("latency")); + + // `account delete` drives the real RemoveAccount RPC. + let delete_out = runner + .execute_command( + &Commands::Account { + action: AccountSubcommand::Delete { + id: "acct-stub-1".to_string(), + }, + }, + true, + ) + .await; + assert!(delete_out.contains(r#""status":"deleted""#)); } /// Reference-client proof: boots a stub `nuncio.v1.Mail` gRPC server diff --git a/crates/nuncio-mail/src/imap.rs b/crates/nuncio-mail/src/imap.rs index 98382f9..0366e6e 100644 --- a/crates/nuncio-mail/src/imap.rs +++ b/crates/nuncio-mail/src/imap.rs @@ -2,10 +2,14 @@ use async_trait::async_trait; use nuncio_core::model::{Email, Folder}; +use nuncio_core::TlsMode; use std::fmt::Debug; +use std::pin::Pin; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; -use tokio::io::{AsyncRead, AsyncWrite}; +use std::task::{Context, Poll}; +use std::time::Duration; +use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt, ReadBuf}; use tokio::net::TcpStream; use tokio_rustls::client::TlsStream; use tokio_rustls::rustls::pki_types::ServerName; @@ -13,6 +17,12 @@ use tokio_rustls::rustls::{ClientConfig, RootCertStore}; use tokio_rustls::TlsConnector; use tokio_stream::StreamExt; +/// Upper bound on how long a single IMAP transport connect (TCP dial, plus +/// any STARTTLS negotiation and TLS handshake) may take before it is treated +/// as a failure. Without this an unreachable or silently-dropping host could +/// hang a connect -- and therefore a `TestAccountConnection` RPC -- forever. +const CONNECT_TIMEOUT: Duration = Duration::from_secs(15); + use crate::backend::MailBackend; use crate::parser::{MailError, MimeParserAdapter}; @@ -86,20 +96,191 @@ pub async fn connect_tls_stream(host: &str, port: u16) -> Result>), + /// Plain, unencrypted TCP stream. + Plain(TcpStream), +} + +impl AsyncRead for MailStream { + fn poll_read( + self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &mut ReadBuf<'_>, + ) -> Poll> { + match self.get_mut() { + MailStream::Tls(s) => Pin::new(s.as_mut()).poll_read(cx, buf), + MailStream::Plain(s) => Pin::new(s).poll_read(cx, buf), + } + } +} + +impl AsyncWrite for MailStream { + fn poll_write( + self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &[u8], + ) -> Poll> { + match self.get_mut() { + MailStream::Tls(s) => Pin::new(s.as_mut()).poll_write(cx, buf), + MailStream::Plain(s) => Pin::new(s).poll_write(cx, buf), + } + } + + fn poll_flush(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + match self.get_mut() { + MailStream::Tls(s) => Pin::new(s.as_mut()).poll_flush(cx), + MailStream::Plain(s) => Pin::new(s).poll_flush(cx), + } + } + + fn poll_shutdown(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + match self.get_mut() { + MailStream::Tls(s) => Pin::new(s.as_mut()).poll_shutdown(cx), + MailStream::Plain(s) => Pin::new(s).poll_shutdown(cx), + } + } +} + +/// Establish a plain, unencrypted TCP stream to an IMAP server. Used for +/// [`TlsMode::Plain`] (e.g. a local test/dev port) -- no TLS is negotiated, +/// so credentials travel in the clear; the caller opted into that explicitly. +pub async fn connect_plain_stream(host: &str, port: u16) -> Result { + let target_port = if port == 0 { 143 } else { port }; + let addr = format!("{}:{}", host, target_port); + TcpStream::connect(&addr) + .await + .map_err(|e| MailError::NetworkError(format!("TCP connection to {} failed: {}", addr, e))) +} + +/// Read a single CRLF-terminated line from an IMAP transport stream, byte by +/// byte. Used only during STARTTLS negotiation, where exactly one greeting +/// line and then a small number of response lines are read before the socket +/// is upgraded -- not on the hot fetch path, so the simple unbuffered read is +/// adequate and avoids buffering bytes that would be lost across the upgrade. +async fn read_imap_line(stream: &mut S) -> Result +where + S: AsyncRead + Unpin, +{ + let mut line = Vec::new(); + let mut byte = [0u8; 1]; + loop { + let n = stream + .read(&mut byte) + .await + .map_err(|e| MailError::NetworkError(format!("reading IMAP line failed: {}", e)))?; + if n == 0 { + return Err(MailError::ImapError( + "connection closed before a complete IMAP line was read".to_string(), + )); + } + if byte[0] == b'\n' { + break; + } + if byte[0] != b'\r' { + line.push(byte[0]); + } + } + Ok(String::from_utf8_lossy(&line).into_owned()) +} + +/// Establish a STARTTLS IMAP stream: connect plain TCP, read the server +/// greeting, issue an explicit `STARTTLS` command, confirm the tagged `OK`, +/// then upgrade the SAME socket to TLS via the shared rustls connector. This +/// is the RFC 3501 / RFC 2595 explicit-TLS flow used on the submission port +/// (143). The upgrade reuses [`build_tls_connector`], so certificate +/// validation is identical to implicit TLS. +pub async fn connect_starttls_stream( + host: &str, + port: u16, +) -> Result, MailError> { + let target_port = if port == 0 { 143 } else { port }; + let addr = format!("{}:{}", host, target_port); + let mut tcp = TcpStream::connect(&addr).await.map_err(|e| { + MailError::NetworkError(format!("TCP connection to {} failed: {}", addr, e)) + })?; + + // Server greeting (untagged `* OK ...`); consume it before commanding. + let greeting = read_imap_line(&mut tcp).await?; + if !greeting.starts_with("* OK") && !greeting.starts_with("* PREAUTH") { + return Err(MailError::ImapError(format!( + "unexpected IMAP greeting during STARTTLS negotiation: {}", + greeting + ))); + } + + const TAG: &str = "N1"; + tcp.write_all(format!("{} STARTTLS\r\n", TAG).as_bytes()) + .await + .map_err(|e| MailError::NetworkError(format!("sending STARTTLS failed: {}", e)))?; + + // Read responses until the tagged completion line for our command, + // tolerating any untagged lines the server may interleave first. + let tag_prefix = format!("{} ", TAG); + loop { + let line = read_imap_line(&mut tcp).await?; + if let Some(rest) = line.strip_prefix(&tag_prefix) { + if rest.starts_with("OK") { + break; + } + return Err(MailError::ImapError(format!( + "server refused STARTTLS: {}", + line + ))); + } + } + + let connector = build_tls_connector()?; + let server_name = ServerName::try_from(host.to_string()) + .map_err(|e| MailError::TlsError(format!("invalid TLS server name '{}': {}", host, e)))?; + let tls_stream = connector.connect(server_name, tcp).await.map_err(|e| { + MailError::TlsError(format!( + "TLS handshake with {} after STARTTLS failed: {}", + host, e + )) + })?; + Ok(tls_stream) +} + /// IMAP dual-socket manager maintaining isolated connections for IDLE push and FETCH/STORE queries. pub struct ImapDualSocketManager { server_host: String, server_port: u16, + tls_mode: TlsMode, idle_active: Arc, } impl ImapDualSocketManager { - /// Create a new `ImapDualSocketManager`. + /// Create a new `ImapDualSocketManager` defaulting to implicit TLS. The + /// default port for a `0` port is the implicit-TLS IMAPS port (993). pub fn new(server_host: &str, server_port: u16) -> Self { - let port = if server_port == 0 { 993 } else { server_port }; + Self::with_tls_mode(server_host, server_port, TlsMode::ImplicitTls) + } + + /// Create a new `ImapDualSocketManager` for a specific transport security + /// [`TlsMode`]. A `0` port is resolved to the conventional default for + /// the chosen mode: 993 for implicit TLS, 143 for STARTTLS/plain. + pub fn with_tls_mode(server_host: &str, server_port: u16, tls_mode: TlsMode) -> Self { + let port = if server_port == 0 { + match tls_mode { + TlsMode::ImplicitTls => 993, + TlsMode::StartTls | TlsMode::Plain => 143, + } + } else { + server_port + }; Self { server_host: server_host.to_string(), server_port: port, + tls_mode, idle_active: Arc::new(AtomicBool::new(false)), } } @@ -114,6 +295,11 @@ impl ImapDualSocketManager { self.server_port } + /// Retrieve the configured transport security mode. + pub fn tls_mode(&self) -> TlsMode { + self.tls_mode + } + /// Retrieve the current IDLE socket state. pub fn idle_state(&self) -> IdleSocketState { if self.idle_active.load(Ordering::SeqCst) { @@ -123,18 +309,44 @@ impl ImapDualSocketManager { } } - /// Establish a live TLS socket connection to the target IMAP server. - pub async fn connect_tls_socket(&self) -> Result, MailError> { - connect_tls_stream(&self.server_host, self.server_port).await + /// Establish a live transport socket to the target IMAP server, selecting + /// implicit TLS, STARTTLS, or plain TCP per the configured [`TlsMode`]. + /// Bounded by [`CONNECT_TIMEOUT`] so an unreachable host cannot hang the + /// caller indefinitely. + pub async fn connect_stream(&self) -> Result { + let connect = async { + match self.tls_mode { + TlsMode::ImplicitTls => Ok(MailStream::Tls(Box::new( + connect_tls_stream(&self.server_host, self.server_port).await?, + ))), + TlsMode::StartTls => Ok(MailStream::Tls(Box::new( + connect_starttls_stream(&self.server_host, self.server_port).await?, + ))), + TlsMode::Plain => Ok(MailStream::Plain( + connect_plain_stream(&self.server_host, self.server_port).await?, + )), + } + }; + tokio::time::timeout(CONNECT_TIMEOUT, connect) + .await + .map_err(|_| { + MailError::NetworkError(format!( + "IMAP connect to {}:{} timed out after {}s", + self.server_host, + self.server_port, + CONNECT_TIMEOUT.as_secs() + )) + })? } - /// Establish an authenticated IMAP session over a live TLS socket connection. + /// Establish an authenticated IMAP session over a live transport socket + /// selected by the configured [`TlsMode`]. pub async fn connect_session( &self, username: &str, password: &str, - ) -> Result>, MailError> { - let stream = self.connect_tls_socket().await?; + ) -> Result, MailError> { + let stream = self.connect_stream().await?; let client = async_imap::Client::new(stream); let session = client .login(username, password) @@ -169,7 +381,7 @@ pub struct ImapEngine { } impl ImapEngine { - /// Create a new `ImapEngine`. + /// Create a new `ImapEngine` defaulting to implicit TLS transport. pub fn new(account_id: &str, server_host: &str, server_port: u16) -> Self { Self { socket_manager: ImapDualSocketManager::new(server_host, server_port), @@ -179,22 +391,53 @@ impl ImapEngine { } } - /// Create a new `ImapEngine` with host, port, and authentication credentials. + /// Create a new `ImapEngine` with host, port, transport security + /// [`TlsMode`], and authentication credentials. The `tls_mode` genuinely + /// selects the transport (implicit TLS / STARTTLS / plain) used for every + /// connection this engine opens -- it is not merely recorded. pub fn with_credentials( account_id: &str, server_host: &str, server_port: u16, + tls_mode: TlsMode, username: &str, password: &str, ) -> Self { Self { - socket_manager: ImapDualSocketManager::new(server_host, server_port), + socket_manager: ImapDualSocketManager::with_tls_mode( + server_host, + server_port, + tls_mode, + ), account_id: account_id.to_string(), username: Some(username.to_string()), password: Some(password.to_string()), } } + /// Probe the IMAP endpoint end to end: open a transport connection using + /// the configured [`TlsMode`], authenticate with the engine's + /// credentials, and cleanly log out. Returns `Ok(())` only on a genuine + /// successful login -- any dial, TLS-handshake, or authentication failure + /// surfaces as the real [`MailError`], never a fabricated success. This + /// backs the daemon's `TestAccountConnection` IMAP leg. + pub async fn probe_connection(&self) -> Result<(), MailError> { + let (username, password) = match (&self.username, &self.password) { + (Some(u), Some(p)) => (u.as_str(), p.as_str()), + _ => { + return Err(MailError::AuthError( + "IMAP connection probe requires credentials".to_string(), + )) + } + }; + let mut session = self + .socket_manager + .connect_session(username, password) + .await?; + let _ = session.logout().await; + Ok(()) + } + /// Set authentication credentials for the IMAP engine. pub fn set_credentials(&mut self, username: &str, password: &str) { self.username = Some(username.to_string()); @@ -520,6 +763,7 @@ mod tests { let manager = ImapDualSocketManager::new("mail.kof22.com", 993); assert_eq!(manager.server_host(), "mail.kof22.com"); assert_eq!(manager.server_port(), 993); + assert_eq!(manager.tls_mode(), TlsMode::ImplicitTls); assert_eq!(manager.idle_state(), IdleSocketState::Disconnected); manager.start_idle_listener()?; @@ -546,6 +790,76 @@ mod tests { Ok(()) } + #[tokio::test] + async fn with_tls_mode_records_mode_and_resolves_default_ports() { + // A `0` port resolves to the conventional default for each mode. + let implicit = ImapDualSocketManager::with_tls_mode("host", 0, TlsMode::ImplicitTls); + assert_eq!(implicit.tls_mode(), TlsMode::ImplicitTls); + assert_eq!(implicit.server_port(), 993); + + let starttls = ImapDualSocketManager::with_tls_mode("host", 0, TlsMode::StartTls); + assert_eq!(starttls.tls_mode(), TlsMode::StartTls); + assert_eq!(starttls.server_port(), 143); + + let plain = ImapDualSocketManager::with_tls_mode("host", 0, TlsMode::Plain); + assert_eq!(plain.tls_mode(), TlsMode::Plain); + assert_eq!(plain.server_port(), 143); + + // An explicit non-zero port is always honored verbatim. + let explicit = ImapDualSocketManager::with_tls_mode("host", 2143, TlsMode::StartTls); + assert_eq!(explicit.server_port(), 2143); + } + + #[tokio::test] + async fn connect_stream_plain_connects_to_a_loopback_listener() { + // Plain mode performs a genuine TCP connect with no TLS: against a + // loopback listener the connect succeeds and yields the plain + // transport variant. + 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 accept = tokio::spawn(async move { + let _ = listener.accept().await; + }); + + let manager = + ImapDualSocketManager::with_tls_mode("127.0.0.1", addr.port(), TlsMode::Plain); + let stream = manager + .connect_stream() + .await + .expect("plain connect to loopback listener succeeds"); + assert!(matches!(stream, MailStream::Plain(_))); + let _ = accept.await; + } + + #[tokio::test] + async fn connect_stream_implicit_tls_fails_fast_against_a_closed_port() { + // Implicit TLS against a closed port fails at the TCP dial (connection + // refused) -- a genuine, honest failure, not a fabricated success. + let manager = ImapDualSocketManager::with_tls_mode("127.0.0.1", 1, TlsMode::ImplicitTls); + let err = manager + .connect_stream() + .await + .expect_err("connect to a closed port must fail"); + assert!(matches!( + err, + MailError::NetworkError(_) | MailError::TlsError(_) + )); + } + + #[tokio::test] + async fn probe_connection_requires_credentials() { + // Without credentials the probe reports an honest error rather than + // dialing or fabricating a result. + let engine = ImapEngine::new("acct-1", "127.0.0.1", 993); + let err = engine + .probe_connection() + .await + .expect_err("probe without credentials must fail"); + assert!(matches!(err, MailError::AuthError(_))); + } + #[test] fn tls_config_building() -> Result<(), MailError> { let config = build_tls_config()?; diff --git a/crates/nuncio-mail/src/smtp.rs b/crates/nuncio-mail/src/smtp.rs index 97c1c69..35e592d 100644 --- a/crates/nuncio-mail/src/smtp.rs +++ b/crates/nuncio-mail/src/smtp.rs @@ -9,6 +9,7 @@ use lettre::transport::smtp::client::{Tls, TlsParameters}; use lettre::transport::smtp::Error as LettreSmtpError; use lettre::{AsyncSmtpTransport, AsyncTransport, Tokio1Executor}; use nuncio_core::model::{Attachment, Email}; +use nuncio_core::TlsMode; use crate::backend::{MessageSender, OutboundMessage}; use crate::parser::MailError; @@ -42,12 +43,39 @@ pub struct SmtpTransportEngine { } impl SmtpTransportEngine { - /// Create a new [`SmtpTransportEngine`] with initialized transport client. - pub fn new(host: &str, port: u16, username: &str, password: &str) -> Result { - let transport = Self::build_transport(host, port, username, password)?; + /// Create a new [`SmtpTransportEngine`] with initialized transport client + /// for a specific transport security [`TlsMode`]. + pub fn new( + host: &str, + port: u16, + tls_mode: TlsMode, + username: &str, + password: &str, + ) -> Result { + let transport = Self::build_transport(host, port, tls_mode, username, password)?; Ok(Self { transport }) } + /// Probe the SMTP endpoint: open a real connection to the configured + /// relay (performing the implicit-TLS wrap or STARTTLS upgrade the + /// configured [`TlsMode`] selects) and confirm the server responds. Backs + /// the daemon's `TestAccountConnection` SMTP leg. Returns `Ok(())` only + /// when a connection genuinely succeeded -- a dial/handshake failure + /// surfaces as the real [`MailError`], never a fabricated success. + /// + /// This validates transport reachability and TLS negotiation; SMTP has no + /// standalone authentication probe (credentials are exercised at message + /// submission time), so this deliberately does not claim to verify auth. + pub async fn probe_connection(&self) -> Result<(), MailError> { + match self.transport.test_connection().await { + Ok(true) => Ok(()), + Ok(false) => Err(MailError::TransportFailed( + "SMTP server did not accept the connection probe".to_string(), + )), + Err(e) => Err(MailError::from(e)), + } + } + /// Create a new [`SmtpTransportEngine`] from an existing [`AsyncSmtpTransport`] client. pub fn with_transport(transport: AsyncSmtpTransport) -> Self { Self { transport } @@ -227,33 +255,53 @@ impl SmtpTransportEngine { Ok(()) } - /// Build an [`AsyncSmtpTransport`] client instance. - /// Configures Implicit TLS (`Tls::Wrapper`) for port 465, and STARTTLS (`Tls::Required`) for port 587 or other ports. + /// Build an [`AsyncSmtpTransport`] client instance whose transport + /// security genuinely follows the configured [`TlsMode`]: + /// + /// - [`TlsMode::ImplicitTls`] wraps the socket in TLS immediately + /// (`Tls::Wrapper`), as on the SMTPS port (465). + /// - [`TlsMode::StartTls`] connects in the clear and REQUIRES a STARTTLS + /// upgrade before proceeding (`Tls::Required`), as on the submission + /// port (587); a server without STARTTLS is rejected rather than + /// silently falling back to plaintext. + /// - [`TlsMode::Plain`] performs no TLS at all (`Tls::None`), for a local + /// test/dev relay -- the caller opted into cleartext explicitly. + /// + /// Implicit-TLS and STARTTLS relays are built via `relay(host)` (which + /// defaults to TLS wrapping) / `starttls_relay(host)` respectively; + /// plaintext is built via the non-encrypting `builder_dangerous(host)`. pub fn build_transport( host: &str, port: u16, + tls_mode: TlsMode, username: &str, password: &str, ) -> Result, MailError> { Self::validate_smtp_config(host, port, username)?; let creds = Credentials::new(username.to_string(), password.to_string()); - let tls_params = TlsParameters::builder(host.to_string()) - .build() - .map_err(|e| MailError::TransportFailed(format!("TLS parameters error: {}", e)))?; - let tls = if port == 465 { - Tls::Wrapper(tls_params) - } else { - Tls::Required(tls_params) + let builder = match tls_mode { + TlsMode::ImplicitTls | TlsMode::StartTls => { + let tls_params = TlsParameters::builder(host.to_string()) + .build() + .map_err(|e| { + MailError::TransportFailed(format!("TLS parameters error: {}", e)) + })?; + let tls = match tls_mode { + TlsMode::StartTls => Tls::Required(tls_params), + _ => Tls::Wrapper(tls_params), + }; + AsyncSmtpTransport::::relay(host) + .map_err(|e| { + MailError::TransportFailed(format!("relay configuration error: {}", e)) + })? + .tls(tls) + } + TlsMode::Plain => AsyncSmtpTransport::::builder_dangerous(host), }; - let transport = AsyncSmtpTransport::::relay(host) - .map_err(|e| MailError::TransportFailed(format!("relay configuration error: {}", e)))? - .port(port) - .credentials(creds) - .tls(tls) - .build(); + let transport = builder.port(port).credentials(creds).build(); Ok(transport) } @@ -383,21 +431,59 @@ mod tests { } #[test] - fn build_transport_creates_starttls_client_for_port_587() { - let transport = SmtpTransportEngine::build_transport("smtp.nuncio.mx", 587, "user", "pass"); + fn build_transport_creates_starttls_client() { + let transport = SmtpTransportEngine::build_transport( + "smtp.nuncio.mx", + 587, + TlsMode::StartTls, + "user", + "pass", + ); assert!(transport.is_ok()); } #[test] - fn build_transport_creates_implicit_tls_client_for_port_465() { - let transport = SmtpTransportEngine::build_transport("smtp.nuncio.mx", 465, "user", "pass"); + fn build_transport_creates_implicit_tls_client() { + let transport = SmtpTransportEngine::build_transport( + "smtp.nuncio.mx", + 465, + TlsMode::ImplicitTls, + "user", + "pass", + ); + assert!(transport.is_ok()); + } + + #[test] + fn build_transport_creates_plaintext_client() { + let transport = SmtpTransportEngine::build_transport( + "smtp.nuncio.mx", + 25, + TlsMode::Plain, + "user", + "pass", + ); assert!(transport.is_ok()); } + #[tokio::test] + async fn probe_connection_to_unreachable_server_fails() { + let engine = SmtpTransportEngine::new("127.0.0.1", 1, TlsMode::ImplicitTls, "user", "pass") + .expect("valid config"); + let err = engine + .probe_connection() + .await + .expect_err("probe against port 1 must fail, never fabricate success"); + assert!(matches!( + err, + MailError::TransportFailed(_) | MailError::SmtpFailed(_) + )); + } + #[tokio::test] async fn send_email_to_unreachable_server_fails_with_transport_error() { - let engine = - SmtpTransportEngine::new("127.0.0.1", 1, "user", "pass").expect("valid config"); + let engine = SmtpTransportEngine::new("127.0.0.1", 1, TlsMode::ImplicitTls, "user", "pass") + .expect("valid config"); let email = sample_email(); let err = engine .send_email(&email) @@ -411,7 +497,8 @@ mod tests { #[test] fn engine_constructor_creates_instance() { - let engine = SmtpTransportEngine::new("smtp.nuncio.mx", 465, "user", "pass"); + let engine = + SmtpTransportEngine::new("smtp.nuncio.mx", 465, TlsMode::ImplicitTls, "user", "pass"); assert!(engine.is_ok()); } @@ -506,8 +593,8 @@ mod tests { #[tokio::test] async fn send_message_to_unreachable_server_fails_with_transport_error() { - let engine = - SmtpTransportEngine::new("127.0.0.1", 1, "user", "pass").expect("valid config"); + let engine = SmtpTransportEngine::new("127.0.0.1", 1, TlsMode::ImplicitTls, "user", "pass") + .expect("valid config"); let message = sample_outbound_message(); let err = engine .send_message(&message) @@ -525,8 +612,8 @@ mod tests { // through to the real `send_message` path, not a separate/fabricated // stub -- exercised via the trait object // exactly like production code (`nunciod::send`) uses it. - let engine = - SmtpTransportEngine::new("127.0.0.1", 1, "user", "pass").expect("valid config"); + let engine = SmtpTransportEngine::new("127.0.0.1", 1, TlsMode::ImplicitTls, "user", "pass") + .expect("valid config"); let sender: &dyn MessageSender = &engine; let message = sample_outbound_message(); let err = sender diff --git a/crates/nuncio-proto/proto/nuncio/v1/descriptor.bin b/crates/nuncio-proto/proto/nuncio/v1/descriptor.bin index a19a85f..2304090 100644 Binary files a/crates/nuncio-proto/proto/nuncio/v1/descriptor.bin and b/crates/nuncio-proto/proto/nuncio/v1/descriptor.bin differ diff --git a/crates/nuncio-proto/proto/nuncio/v1/nuncio.proto b/crates/nuncio-proto/proto/nuncio/v1/nuncio.proto index 0339047..07eba08 100644 --- a/crates/nuncio-proto/proto/nuncio/v1/nuncio.proto +++ b/crates/nuncio-proto/proto/nuncio/v1/nuncio.proto @@ -144,6 +144,30 @@ service Accounts { // ListAccounts returns every persisted account configuration. Password // credentials are NEVER included in this response. rpc ListAccounts(ListAccountsRequest) returns (ListAccountsResponse); + // UpdateAccount overwrites an existing account's persisted configuration. + // The password is optional: when omitted the existing keyring credential + // is left untouched; when present it REPLACES the stored credential in the + // OS keyring vault (never written to SQLite, never echoed back). A password + // rotation that cannot be persisted is rolled back so the vault never + // diverges from the persisted account row. + rpc UpdateAccount(UpdateAccountRequest) returns (UpdateAccountResponse); + // RemoveAccount deletes a persisted account configuration by id and its + // associated keyring credential. + rpc RemoveAccount(RemoveAccountRequest) returns (RemoveAccountResponse); + // TestAccountConnection performs a real, bounded connection probe against + // the account's configured IMAP and SMTP endpoints (honoring each one's + // TLS transport mode) and reports the GENUINE per-protocol outcome -- never + // a fabricated success. The two legs verify DIFFERENT depths, and callers + // must not conflate them: + // - IMAP verifies a full LOGIN: connect, negotiate TLS/STARTTLS, and + // authenticate with the account's keyring credential. + // - SMTP verifies only TRANSPORT REACHABILITY: connect and negotiate + // TLS/STARTTLS. It does NOT perform an SMTP AUTH exchange, so `smtp_ok` + // does not assert the credential is valid for outbound mail (SMTP has + // no standard authentication probe that does not also submit a message). + // A probe that fails carries the real error detail; each leg is bounded by + // a timeout so the RPC cannot hang against an unreachable host. + rpc TestAccountConnection(TestAccountConnectionRequest) returns (TestAccountConnectionResponse); } // Mirrors `nuncio_core::AccountProtocol`. @@ -220,6 +244,57 @@ message ListAccountsResponse { repeated AccountConfig accounts = 1; } +// UpdateAccountRequest carries the full replacement account configuration +// plus an OPTIONAL password. When `password` is absent the account's existing +// keyring credential is preserved; when present it is written to the OS +// keyring vault (under `config.keyring_secret_key`), exactly like +// `AddAccountRequest` -- never persisted to SQLite, never logged, never +// echoed back. +message UpdateAccountRequest { + AccountConfig config = 1; + optional string password = 2; +} + +// UpdateAccountResponse deliberately carries no fields: a successful RPC +// response (as opposed to a `Status` error) is confirmation enough, and the +// caller already holds the updated config it just sent. +message UpdateAccountResponse {} + +// RemoveAccountRequest names the account configuration to delete by id. +message RemoveAccountRequest { + string id = 1; +} + +// RemoveAccountResponse currently carries no fields; a successful RPC +// response is confirmation enough. +message RemoveAccountResponse {} + +// TestAccountConnectionRequest names the persisted account to probe by id. +message TestAccountConnectionRequest { + string id = 1; +} + +// TestAccountConnectionResponse reports the REAL, independent outcome of +// probing each protocol endpoint -- never a fabricated success. `imap_error` +// / `smtp_error` are present only when the corresponding `*_ok` is false and +// carry the genuine failure detail (dial/handshake/auth error text). The two +// `*_ok` flags verify different depths -- see `TestAccountConnection`. +message TestAccountConnectionResponse { + // Whether the IMAP endpoint FULLY authenticated: connect, TLS/STARTTLS + // negotiation, AND login with the account's credential all succeeded. + bool imap_ok = 1; + // Whether the SMTP endpoint was REACHABLE: connect and TLS/STARTTLS + // negotiation succeeded. This is a transport-reachability check only -- it + // does NOT perform an SMTP AUTH exchange, so it does not assert the + // credential is valid for outbound mail. + bool smtp_ok = 2; + // Present only when `imap_ok` is false: the real IMAP login/handshake error. + optional string imap_error = 3; + // Present only when `smtp_ok` is false: the real SMTP transport/handshake + // error. + optional string smtp_error = 4; +} + // Mail exposes the READ path over the daemon's real, persistent message // store (the data that syncs in via `DatabaseEngine::save_email`). // diff --git a/crates/nuncio-store/src/db.rs b/crates/nuncio-store/src/db.rs index 58b1b15..30cc2f8 100644 --- a/crates/nuncio-store/src/db.rs +++ b/crates/nuncio-store/src/db.rs @@ -186,6 +186,29 @@ pub struct WormChainReport { pub first_broken_seq: Option, } +/// Serialize a [`nuncio_core::TlsMode`] to its stable on-disk `accounts` +/// column form. Kept as a bare snake_case token (matching the column's +/// `'implicit_tls'` default) rather than JSON, so a row inserted by the +/// migration default and one written here are byte-identical. +fn tls_mode_to_db(mode: nuncio_core::TlsMode) -> &'static str { + match mode { + nuncio_core::TlsMode::ImplicitTls => "implicit_tls", + nuncio_core::TlsMode::StartTls => "start_tls", + nuncio_core::TlsMode::Plain => "plain", + } +} + +/// Parse a persisted `accounts` TLS-mode column back into +/// [`nuncio_core::TlsMode`]. An unrecognized value falls back to the safe +/// default (implicit TLS) rather than failing the whole account load. +fn tls_mode_from_db(value: &str) -> nuncio_core::TlsMode { + match value { + "start_tls" => nuncio_core::TlsMode::StartTls, + "plain" => nuncio_core::TlsMode::Plain, + _ => nuncio_core::TlsMode::ImplicitTls, + } +} + impl DatabaseEngine { /// Maximum concurrent read/write pool size. pub const MAX_CONNECTIONS: u32 = 16; @@ -439,7 +462,9 @@ impl DatabaseEngine { keyring_secret_key TEXT NOT NULL, sync_interval_secs INTEGER NOT NULL, smtp_host TEXT, - smtp_port INTEGER + smtp_port INTEGER, + imap_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls', + smtp_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls' ); CREATE TABLE IF NOT EXISTS filter_rules ( @@ -631,11 +656,53 @@ impl DatabaseEngine { .map_err(DatabaseError::Query)?; self.ensure_accounts_smtp_columns().await?; + self.ensure_accounts_tls_mode_columns().await?; self.backfill_message_fts().await?; Ok(()) } + /// Additive, backfill-safe migration that adds the `imap_tls_mode` / + /// `smtp_tls_mode` columns to a pre-existing `accounts` table that + /// predates persisted per-protocol TLS transport modes. + /// + /// A fresh database already gets these columns from `CREATE TABLE IF NOT + /// EXISTS accounts` above, so on a fresh database this is a no-op. For a + /// pre-existing database file the columns are added with a + /// `'implicit_tls'` default, so every existing row keeps loading + /// successfully and retains the historical behavior (implicit TLS) it was + /// created under. SQLite has no `ADD COLUMN IF NOT EXISTS`, so column + /// presence is checked explicitly via `PRAGMA table_info` first, making + /// this safe to run on every daemon startup. + async fn ensure_accounts_tls_mode_columns(&self) -> Result<(), DatabaseError> { + let existing_columns: Vec = sqlx::query("PRAGMA table_info(accounts)") + .fetch_all(&self.pool) + .await + .map_err(DatabaseError::Query)? + .iter() + .map(|row| row.get::("name")) + .collect(); + + if !existing_columns.iter().any(|c| c == "imap_tls_mode") { + sqlx::query( + "ALTER TABLE accounts ADD COLUMN imap_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls'", + ) + .execute(&self.pool) + .await + .map_err(DatabaseError::Query)?; + } + if !existing_columns.iter().any(|c| c == "smtp_tls_mode") { + sqlx::query( + "ALTER TABLE accounts ADD COLUMN smtp_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls'", + ) + .execute(&self.pool) + .await + .map_err(DatabaseError::Query)?; + } + + Ok(()) + } + /// Additive, backfill-safe migration that adds the /// `smtp_host` / `smtp_port` columns to a pre-existing `accounts` table /// that predates the SMTP account-endpoint feature. @@ -725,8 +792,8 @@ impl DatabaseEngine { sqlx::query( r#" INSERT OR REPLACE INTO accounts - (id, name, email_address, protocol, server_host, server_port, use_tls, keyring_secret_key, sync_interval_secs, smtp_host, smtp_port) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + (id, name, email_address, protocol, server_host, server_port, use_tls, keyring_secret_key, sync_interval_secs, smtp_host, smtp_port, imap_tls_mode, smtp_tls_mode) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) "#, ) .bind(&config.id) @@ -740,6 +807,8 @@ impl DatabaseEngine { .bind(config.sync_interval_secs as i64) .bind(&config.smtp_host) .bind(config.smtp_port as i64) + .bind(tls_mode_to_db(config.imap_tls_mode)) + .bind(tls_mode_to_db(config.smtp_tls_mode)) .execute(&self.pool) .await .map_err(DatabaseError::Query)?; @@ -747,6 +816,27 @@ impl DatabaseEngine { Ok(()) } + /// Fetch a single persisted [`nuncio_core::AccountConfig`] by id, or + /// `None` if no such account row exists. + pub async fn get_account( + &self, + id: &str, + ) -> Result, DatabaseError> { + Ok(self.list_accounts().await?.into_iter().find(|a| a.id == id)) + } + + /// Delete a persisted account configuration by id. Idempotent: deleting a + /// non-existent id is not an error (the post-condition -- "no account row + /// with this id" -- already holds). + pub async fn delete_account(&self, id: &str) -> Result<(), DatabaseError> { + sqlx::query("DELETE FROM accounts WHERE id = ?") + .bind(id) + .execute(&self.pool) + .await + .map_err(DatabaseError::Query)?; + Ok(()) + } + /// Query all saved accounts in SQLite. #[allow(clippy::type_complexity)] pub async fn list_accounts(&self) -> Result, DatabaseError> { @@ -762,9 +852,11 @@ impl DatabaseEngine { i64, Option, Option, + String, + String, )> = sqlx::query_as( r#" - SELECT id, name, email_address, protocol, server_host, server_port, use_tls, keyring_secret_key, sync_interval_secs, smtp_host, smtp_port + SELECT id, name, email_address, protocol, server_host, server_port, use_tls, keyring_secret_key, sync_interval_secs, smtp_host, smtp_port, imap_tls_mode, smtp_tls_mode FROM accounts "#, ) @@ -787,6 +879,8 @@ impl DatabaseEngine { sync_interval_secs, smtp_host, smtp_port, + imap_tls_mode, + smtp_tls_mode, )| { let protocol = serde_json::from_str(&protocol_str) .unwrap_or(nuncio_core::AccountProtocol::ImapSmtp); @@ -811,8 +905,8 @@ impl DatabaseEngine { smtp_host: resolved_smtp_host, smtp_port: resolved_smtp_port, use_tls: use_tls != 0, - imap_tls_mode: nuncio_core::TlsMode::ImplicitTls, - smtp_tls_mode: nuncio_core::TlsMode::ImplicitTls, + imap_tls_mode: tls_mode_from_db(&imap_tls_mode), + smtp_tls_mode: tls_mode_from_db(&smtp_tls_mode), keyring_secret_key, sync_interval_secs: sync_interval_secs as u64, } @@ -2505,6 +2599,103 @@ mod tests { assert_eq!(accounts[0].smtp_port, 465); } + /// Regression proof that the per-protocol TLS transport modes are + /// genuinely persisted and round-tripped -- previously they were + /// validated on add but silently discarded, with `list_accounts` + /// hardcoding both back to `ImplicitTls` regardless of what was saved. + /// Saves an account whose IMAP mode is `StartTls` and SMTP mode is + /// `Plain`, then re-reads it via BOTH `list_accounts` and `get_account` + /// and asserts each distinct mode survived rather than collapsing to the + /// default. + #[tokio::test] + async fn account_tls_modes_survive_save_and_reload() { + let (engine, _dir) = DatabaseEngine::connect_ephemeral().await.unwrap(); + + let acct = nuncio_core::AccountConfig { + id: "acct-tls-1".to_string(), + name: "Mixed TLS Account".to_string(), + email_address: "mixed@nuncio.mx".to_string(), + protocol: nuncio_core::AccountProtocol::ImapSmtp, + server_host: "imap.nuncio.mx".to_string(), + server_port: 143, + smtp_host: "smtp.nuncio.mx".to_string(), + smtp_port: 25, + use_tls: false, + imap_tls_mode: nuncio_core::TlsMode::StartTls, + smtp_tls_mode: nuncio_core::TlsMode::Plain, + keyring_secret_key: "nuncio/acct-tls-1".to_string(), + sync_interval_secs: 60, + }; + + engine.save_account(&acct).await.expect("save succeeds"); + + let listed = engine.list_accounts().await.expect("list succeeds"); + assert_eq!(listed.len(), 1); + assert_eq!(listed[0].imap_tls_mode, nuncio_core::TlsMode::StartTls); + assert_eq!(listed[0].smtp_tls_mode, nuncio_core::TlsMode::Plain); + + let fetched = engine + .get_account("acct-tls-1") + .await + .expect("get succeeds") + .expect("account present"); + assert_eq!(fetched.imap_tls_mode, nuncio_core::TlsMode::StartTls); + assert_eq!(fetched.smtp_tls_mode, nuncio_core::TlsMode::Plain); + assert_eq!(fetched, acct); + } + + /// Proves `get_account` resolves a saved account by id and returns `None` + /// for an unknown id, and that `delete_account` genuinely removes the row + /// (and is idempotent for an already-absent id). + #[tokio::test] + async fn get_and_delete_account_round_trip() { + let (engine, _dir) = DatabaseEngine::connect_ephemeral().await.unwrap(); + + let acct = nuncio_core::AccountConfig { + id: "acct-del-1".to_string(), + name: "Deletable".to_string(), + email_address: "del@nuncio.mx".to_string(), + protocol: nuncio_core::AccountProtocol::ImapSmtp, + server_host: "imap.nuncio.mx".to_string(), + server_port: 993, + smtp_host: "smtp.nuncio.mx".to_string(), + smtp_port: 465, + use_tls: true, + imap_tls_mode: nuncio_core::TlsMode::ImplicitTls, + smtp_tls_mode: nuncio_core::TlsMode::ImplicitTls, + keyring_secret_key: "nuncio/acct-del-1".to_string(), + sync_interval_secs: 60, + }; + engine.save_account(&acct).await.expect("save succeeds"); + + assert!(engine + .get_account("acct-del-1") + .await + .expect("get succeeds") + .is_some()); + assert!(engine + .get_account("acct-missing") + .await + .expect("get succeeds") + .is_none()); + + engine + .delete_account("acct-del-1") + .await + .expect("delete succeeds"); + assert!(engine + .get_account("acct-del-1") + .await + .expect("get succeeds") + .is_none()); + + // Idempotent: deleting an already-absent id is not an error. + engine + .delete_account("acct-del-1") + .await + .expect("second delete is a no-op"); + } + /// Proves the additive `smtp_host`/`smtp_port` /// migration is backfill-safe. Simulates a database created BEFORE this /// feature existed (an `accounts` table with no `smtp_host`/`smtp_port` diff --git a/crates/nunciod/src/grpc.rs b/crates/nunciod/src/grpc.rs index 0a5a308..e2e189e 100644 --- a/crates/nunciod/src/grpc.rs +++ b/crates/nunciod/src/grpc.rs @@ -9,7 +9,7 @@ use nuncio_cal::CalendarBackend; use nuncio_contacts::ContactsBackend; use nuncio_core::{CoreCommand, CoreEvent, EventBus}; use nuncio_filter::{FilterEngine, NsqlParser, NsqlValidator, ValidationOptions}; -use nuncio_mail::{MailBackend, MessageSender}; +use nuncio_mail::{ImapEngine, MailBackend, MessageSender, SmtpTransportEngine}; use nuncio_proto::v1::accounts_server::{Accounts, AccountsServer}; use nuncio_proto::v1::audit_server::{Audit, AuditServer}; use nuncio_proto::v1::calendar_server::{Calendar, CalendarServer}; @@ -38,9 +38,11 @@ use nuncio_proto::v1::{ ListFoldersResponse, ListMessagesRequest, ListMessagesResponse, ListRecordsRequest, ListRecordsResponse, ListRulesRequest, ListRulesResponse, MarkReadRequest, MarkReadResponse, Message as MessageProto, MessageFlagsChanged, MessageSearchHit, PreviewRuleRequest, - PreviewRuleResponse, SearchMessagesRequest, SearchMessagesResponse, SendMessageRequest, - SendMessageResponse, ShuttingDown, SubscribeRequest, SyncCompleted, SyncRequest, SyncResponse, - SyncStarted, TlsMode as TlsModeProto, TriageProgress, TriageRequest, UpdateAvailable, + PreviewRuleResponse, RemoveAccountRequest, RemoveAccountResponse, SearchMessagesRequest, + SearchMessagesResponse, SendMessageRequest, SendMessageResponse, ShuttingDown, + SubscribeRequest, SyncCompleted, SyncRequest, SyncResponse, SyncStarted, + TestAccountConnectionRequest, TestAccountConnectionResponse, TlsMode as TlsModeProto, + TriageProgress, TriageRequest, UpdateAccountRequest, UpdateAccountResponse, UpdateAvailable, UpdateRuleRequest, UpdateRuleResponse, ValidateRuleRequest, ValidateRuleResponse, VerifyChainRequest, VerifyChainResponse, }; @@ -304,6 +306,134 @@ fn map_account_config_from_proto( }) } +/// Outcome of probing a single protocol endpoint (IMAP or SMTP) during a +/// `TestAccountConnection` RPC. `ok` reflects a GENUINE dial/handshake/auth +/// outcome; `error` carries the real failure detail when `ok` is false. +#[derive(Debug, Clone)] +pub struct ProtocolProbe { + /// Whether the probe genuinely succeeded. + pub ok: bool, + /// Real failure detail, present only when `ok` is false. + pub error: Option, +} + +impl ProtocolProbe { + /// A successful probe. + fn ok() -> Self { + Self { + ok: true, + error: None, + } + } + + /// A failed probe carrying its real error text. + fn failed(error: String) -> Self { + Self { + ok: false, + error: Some(error), + } + } +} + +/// The genuine per-protocol result of a `TestAccountConnection` probe. +#[derive(Debug, Clone)] +pub struct AccountConnectionReport { + /// Result of probing the account's IMAP endpoint. + pub imap: ProtocolProbe, + /// Result of probing the account's SMTP endpoint. + pub smtp: ProtocolProbe, +} + +/// Injectable seam backing `TestAccountConnection`'s real per-protocol +/// connection probing. Production uses [`RealAccountConnectionTester`], which +/// performs a genuine bounded IMAP + SMTP dial/handshake using the account's +/// configured endpoints, TLS modes, and keyring credential. A full-daemon +/// offline test injects a stand-in so the RPC can be exercised end to end +/// without touching any live server. +#[tonic::async_trait] +pub trait AccountConnectionTester: Send + Sync { + /// Probe both protocol endpoints for `config`, authenticating with + /// `password`, and report the genuine per-protocol outcome. + async fn probe( + &self, + config: &nuncio_core::AccountConfig, + password: &str, + ) -> AccountConnectionReport; +} + +/// Production [`AccountConnectionTester`]: builds a real +/// [`nuncio_mail::ImapEngine`] / [`nuncio_mail::SmtpTransportEngine`] from the +/// account's configured endpoints, TLS transport modes, and keyring password, +/// and performs a genuine, bounded connection probe against each. Every +/// result is the real dial/handshake/auth outcome -- never fabricated. +struct RealAccountConnectionTester; + +#[tonic::async_trait] +impl AccountConnectionTester for RealAccountConnectionTester { + async fn probe( + &self, + config: &nuncio_core::AccountConfig, + password: &str, + ) -> AccountConnectionReport { + let imap = match config.protocol { + nuncio_core::AccountProtocol::ImapSmtp => { + let engine = ImapEngine::with_credentials( + &config.id, + &config.server_host, + config.server_port, + config.imap_tls_mode, + &config.email_address, + password, + ); + match engine.probe_connection().await { + Ok(()) => ProtocolProbe::ok(), + Err(e) => ProtocolProbe::failed(e.to_string()), + } + } + nuncio_core::AccountProtocol::Jmap => ProtocolProbe::failed( + "IMAP connection probe is not applicable to a JMAP account".to_string(), + ), + }; + + let smtp = match SmtpTransportEngine::new( + &config.smtp_host, + config.smtp_port, + config.smtp_tls_mode, + &config.email_address, + password, + ) { + Ok(engine) => match engine.probe_connection().await { + Ok(()) => ProtocolProbe::ok(), + Err(e) => ProtocolProbe::failed(e.to_string()), + }, + Err(e) => ProtocolProbe::failed(e.to_string()), + }; + + AccountConnectionReport { imap, smtp } + } +} + +/// Overridable engine seam for the `nuncio.v1.Accounts` service, mirroring +/// [`MailEngineOverrides`]'s shape. Production always uses the default (a real +/// [`RealAccountConnectionTester`]); only a full-daemon offline E2E test +/// injects a stand-in tester so `TestAccountConnection` can be driven over the +/// real authenticated gRPC API without any live server. +#[derive(Clone, Default)] +pub struct AccountsEngineOverrides { + /// When `Some`, `TestAccountConnection` probes through this tester instead + /// of building real per-account IMAP/SMTP engines from keyring + /// credentials. + pub connection_tester: Option>, +} + +impl std::fmt::Debug for AccountsEngineOverrides { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("AccountsEngineOverrides") + .field("connection_tester", &self.connection_tester.is_some()) + .finish() + } +} + /// `nuncio.v1.Accounts` gRPC service implementation backed by the daemon's /// live [`DatabaseEngine`] and [`SecretManager`] vault. /// @@ -314,6 +444,58 @@ fn map_account_config_from_proto( struct AccountsGrpcService { db: Arc, secrets: Arc, + connection_tester: Arc, +} + +/// Rotate an account's stored credential to `new_password` and persist the +/// updated `config`, with a rollback that never destroys a working +/// credential. +/// +/// The prior secret (if any) is snapshotted BEFORE the new one is written, so +/// if `save_account` then fails the snapshot is RESTORED -- returning the +/// vault to exactly its pre-call state. This differs deliberately from +/// `add_account`'s rollback, which deletes: an add has no prior secret to +/// preserve, but an update's key may already hold the working credential, and +/// deleting it on a persist failure would leave the still-present account row +/// referencing a missing secret (strictly worse than doing nothing). When +/// there was no prior secret, restore falls back to deleting the just-written +/// one, matching add's behavior for that case. A rollback failure is reported +/// alongside the original persistence error (which stays primary), never in +/// place of it. +async fn rotate_credential_and_persist( + db: &DatabaseEngine, + secrets: &SecretManager, + config: &nuncio_core::AccountConfig, + new_password: &str, +) -> Result<(), Status> { + // Snapshot the prior credential before overwriting it. `None` means the + // key held no secret (or was unreadable), in which case rollback deletes. + let previous = secrets.get_secret(&config.keyring_secret_key).ok(); + + secrets + .set_secret(&config.keyring_secret_key, new_password) + .map_err(|e| Status::internal(format!("failed to store credential in vault: {e}")))?; + + if let Err(e) = db.save_account(config).await { + let mut message = format!("failed to persist account: {e}"); + let rollback = match &previous { + Some(prior) => secrets.set_secret(&config.keyring_secret_key, prior), + None => secrets.delete_secret(&config.keyring_secret_key), + }; + if let Err(rollback_err) = rollback { + tracing::warn!( + keyring_secret_key = %config.keyring_secret_key, + error = %rollback_err, + "failed to roll back keyring secret after update_account persistence failure" + ); + message.push_str(&format!( + " (and failed to roll back the credential: {rollback_err})" + )); + } + return Err(Status::internal(message)); + } + + Ok(()) } #[tonic::async_trait] @@ -386,6 +568,128 @@ impl Accounts for AccountsGrpcService { Ok(Response::new(ListAccountsResponse { accounts })) } + + async fn update_account( + &self, + request: Request, + ) -> Result, Status> { + let req = request.into_inner(); + let proto_config = req + .config + .ok_or_else(|| Status::invalid_argument("config is required"))?; + + let config = map_account_config_from_proto(proto_config)?; + config + .validate() + .map_err(|e| Status::invalid_argument(e.to_string()))?; + + // Updating a non-existent account is a client error, not a silent + // create: reject it so a typo'd id never conjures a new account row. + if self + .db + .get_account(&config.id) + .await + .map_err(|e| Status::internal(format!("failed to look up account: {e}")))? + .is_none() + { + return Err(Status::not_found(format!( + "account '{}' not found", + config.id + ))); + } + + match req.password { + // A password rotation writes the new credential first, then + // persists. Unlike `add_account` (where no prior secret exists, + // so a failed persist rolls back by DELETING the orphan), an + // update's key may already hold the working credential, so a + // failed persist must RESTORE that prior value rather than delete + // it -- deleting would leave the still-present account row + // referencing a missing secret. See + // [`rotate_credential_and_persist`]. + Some(password) if !password.is_empty() => { + rotate_credential_and_persist(&self.db, &self.secrets, &config, &password).await?; + } + // No password change: the existing keyring credential is left + // entirely untouched and only the persisted configuration is + // overwritten. + _ => { + self.db + .save_account(&config) + .await + .map_err(|e| Status::internal(format!("failed to persist account: {e}")))?; + } + } + + Ok(Response::new(UpdateAccountResponse {})) + } + + async fn remove_account( + &self, + request: Request, + ) -> Result, Status> { + let req = request.into_inner(); + if req.id.is_empty() { + return Err(Status::invalid_argument("id is required")); + } + + let existing = self + .db + .get_account(&req.id) + .await + .map_err(|e| Status::internal(format!("failed to look up account: {e}")))? + .ok_or_else(|| Status::not_found(format!("account '{}' not found", req.id)))?; + + self.db + .delete_account(&req.id) + .await + .map_err(|e| Status::internal(format!("failed to remove account: {e}")))?; + + // Best-effort credential cleanup: the account row is already gone, so + // a keyring deletion failure must not fail the RPC (which would + // wrongly imply the account still exists). Log it and move on -- an + // orphaned secret with no account row is inert. + if let Err(e) = self.secrets.delete_secret(&existing.keyring_secret_key) { + tracing::warn!( + keyring_secret_key = %existing.keyring_secret_key, + error = %e, + "failed to delete keyring credential after removing account" + ); + } + + Ok(Response::new(RemoveAccountResponse {})) + } + + async fn test_account_connection( + &self, + request: Request, + ) -> Result, Status> { + let req = request.into_inner(); + if req.id.is_empty() { + return Err(Status::invalid_argument("id is required")); + } + + let config = self + .db + .get_account(&req.id) + .await + .map_err(|e| Status::internal(format!("failed to look up account: {e}")))? + .ok_or_else(|| Status::not_found(format!("account '{}' not found", req.id)))?; + + let password = self + .secrets + .get_secret(&config.keyring_secret_key) + .map_err(|e| Status::internal(format!("failed to read credential from vault: {e}")))?; + + let report = self.connection_tester.probe(&config, &password).await; + + Ok(Response::new(TestAccountConnectionResponse { + imap_ok: report.imap.ok, + smtp_ok: report.smtp.ok, + imap_error: report.imap.error, + smtp_error: report.smtp.error, + })) + } } /// Default cap on messages returned by `ListMessages` when the caller @@ -1850,6 +2154,7 @@ pub async fn serve_on_listener( MailEngineOverrides::default(), CalendarEngineOverrides::default(), ContactsEngineOverrides::default(), + AccountsEngineOverrides::default(), ) .await } @@ -1876,6 +2181,7 @@ pub async fn serve_on_listener_with_overrides( overrides: MailEngineOverrides, calendar_overrides: CalendarEngineOverrides, contacts_overrides: ContactsEngineOverrides, + accounts_overrides: AccountsEngineOverrides, ) -> Result<(), GrpcServeError> { let token: Arc = token.into(); @@ -1888,6 +2194,9 @@ pub async fn serve_on_listener_with_overrides( let accounts_service = AccountsGrpcService { db: db.clone(), secrets: secrets.clone(), + connection_tester: accounts_overrides + .connection_tester + .unwrap_or_else(|| Arc::new(RealAccountConnectionTester)), }; let accounts_interceptor = BearerAuthInterceptor::new(token.clone()); let accounts_svc = AccountsServer::with_interceptor(accounts_service, accounts_interceptor); @@ -2069,6 +2378,36 @@ mod tests { overrides: MailEngineOverrides, calendar_overrides: CalendarEngineOverrides, contacts_overrides: ContactsEngineOverrides, + ) -> (std::net::SocketAddr, tokio::task::JoinHandle<()>) { + spawn_test_server_with_every_override( + event_bus, + db, + filter_engine, + secrets, + token, + overrides, + calendar_overrides, + contacts_overrides, + AccountsEngineOverrides::default(), + ) + .await + } + + /// Widest test spawn helper: threads an [`AccountsEngineOverrides`] too, + /// so a test can inject a stand-in [`AccountConnectionTester`] and drive + /// `TestAccountConnection` over the real authenticated gRPC API without a + /// live server. + #[allow(clippy::too_many_arguments)] + async fn spawn_test_server_with_every_override( + event_bus: Arc, + db: Arc, + filter_engine: Arc, + secrets: Arc, + token: &str, + overrides: MailEngineOverrides, + calendar_overrides: CalendarEngineOverrides, + contacts_overrides: ContactsEngineOverrides, + accounts_overrides: AccountsEngineOverrides, ) -> (std::net::SocketAddr, tokio::task::JoinHandle<()>) { let listener = TcpListener::bind("127.0.0.1:0") .await @@ -2086,6 +2425,7 @@ mod tests { overrides, calendar_overrides, contacts_overrides, + accounts_overrides, ) .await; }); @@ -2771,6 +3111,96 @@ mod tests { ); } + /// Regression: a password rotation that fails to persist must RESTORE the + /// prior credential, never destroy it. Seeds an existing credential, + /// forces `save_account` to fail for real (closed pool), and asserts the + /// ORIGINAL secret is still retrievable afterward -- proving the update + /// rollback restores rather than deletes (which would orphan the still + /// present account row). Drives the exact rotation+rollback helper the + /// `update_account` RPC uses. + #[tokio::test] + async fn update_account_password_rotation_restores_prior_credential_on_persist_failure() { + let (db, _dir) = DatabaseEngine::connect_ephemeral() + .await + .expect("connect ephemeral test db"); + let secrets = SecretManager::mock(); + + let key = "nuncio/acct-rotate-1"; + secrets + .set_secret(key, "original-working-password") + .expect("seed the prior credential"); + + let config = nuncio_core::AccountConfig { + id: "acct-rotate-1".to_string(), + name: "Rotate Account".to_string(), + email_address: "rotate@nuncio.mx".to_string(), + protocol: nuncio_core::AccountProtocol::ImapSmtp, + server_host: "imap.nuncio.mx".to_string(), + server_port: 993, + smtp_host: "smtp.nuncio.mx".to_string(), + smtp_port: 465, + use_tls: true, + imap_tls_mode: nuncio_core::TlsMode::ImplicitTls, + smtp_tls_mode: nuncio_core::TlsMode::ImplicitTls, + keyring_secret_key: key.to_string(), + sync_interval_secs: 60, + }; + + // Force `save_account` to fail for real by closing the pool, exactly + // as `add_account`'s rollback test does. + db.close().await; + + let err = rotate_credential_and_persist(&db, &secrets, &config, "new-rotated-password") + .await + .expect_err("save_account must fail once the pool is closed"); + assert_eq!(err.code(), Code::Internal); + assert!(err.message().contains("failed to persist account")); + + // The prior credential must be intact -- neither deleted nor left as + // the failed new value. + let restored = secrets.get_secret(key).expect("prior credential survives"); + assert_eq!(restored, "original-working-password"); + } + + /// Companion: when the key held NO prior secret, a failed rotation falls + /// back to deleting the just-written one (matching `add_account`), leaving + /// no orphaned credential. + #[tokio::test] + async fn update_account_password_rotation_deletes_new_credential_when_none_existed() { + let (db, _dir) = DatabaseEngine::connect_ephemeral() + .await + .expect("connect ephemeral test db"); + let secrets = SecretManager::mock(); + + let key = "nuncio/acct-rotate-2"; + let config = nuncio_core::AccountConfig { + id: "acct-rotate-2".to_string(), + name: "Rotate Account 2".to_string(), + email_address: "rotate2@nuncio.mx".to_string(), + protocol: nuncio_core::AccountProtocol::ImapSmtp, + server_host: "imap.nuncio.mx".to_string(), + server_port: 993, + smtp_host: "smtp.nuncio.mx".to_string(), + smtp_port: 465, + use_tls: true, + imap_tls_mode: nuncio_core::TlsMode::ImplicitTls, + smtp_tls_mode: nuncio_core::TlsMode::ImplicitTls, + keyring_secret_key: key.to_string(), + sync_interval_secs: 60, + }; + + db.close().await; + + let err = rotate_credential_and_persist(&db, &secrets, &config, "new-rotated-password") + .await + .expect_err("save_account must fail once the pool is closed"); + assert_eq!(err.code(), Code::Internal); + + // No prior secret existed, so the just-written one is rolled back by + // deletion -- nothing orphaned. + assert!(secrets.get_secret(key).is_err()); + } + // ---- Mail ---- use nuncio_proto::v1::mail_client::MailClient; diff --git a/crates/nunciod/src/send.rs b/crates/nunciod/src/send.rs index 1602396..afcd079 100644 --- a/crates/nunciod/src/send.rs +++ b/crates/nunciod/src/send.rs @@ -113,6 +113,7 @@ fn build_smtp_sender( let engine = SmtpTransportEngine::new( &config.smtp_host, config.smtp_port, + config.smtp_tls_mode, &config.email_address, password, )?; diff --git a/crates/nunciod/src/sync.rs b/crates/nunciod/src/sync.rs index d260505..d81c0aa 100644 --- a/crates/nunciod/src/sync.rs +++ b/crates/nunciod/src/sync.rs @@ -317,6 +317,7 @@ fn build_mail_backend(config: &AccountConfig, password: &str) -> Box AccountConnectionReport { + AccountConnectionReport { + imap: ProtocolProbe { + ok: true, + error: None, + }, + smtp: ProtocolProbe { + ok: false, + error: Some("SMTP relay refused the connection".to_string()), + }, + } + } +} + +fn sample_account_config() -> AccountConfig { + AccountConfig { + id: ACCOUNT_ID.to_string(), + name: "Lifecycle E2E Account".to_string(), + email_address: ACCOUNT_EMAIL.to_string(), + protocol: AccountProtocol::ImapSmtp.into(), + server_host: "imap.nuncio.mx".to_string(), + server_port: 143, + use_tls: false, + imap_tls_mode: TlsMode::StartTls.into(), + smtp_tls_mode: TlsMode::Plain.into(), + keyring_secret_key: KEYRING_KEY.to_string(), + sync_interval_secs: 60, + smtp_host: "smtp.nuncio.mx".to_string(), + smtp_port: 25, + } +} + +/// Boots the real daemon gRPC server on an ephemeral loopback port with an +/// injected [`AccountsEngineOverrides`]. Returns the server address and the +/// minted token; the `TempDir` guard is returned so the caller controls its +/// lifetime for the whole test. +async fn boot_daemon( + secrets: Arc, + accounts_overrides: AccountsEngineOverrides, +) -> (std::net::SocketAddr, String, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("create temp dir for file-backed db"); + let db_path = dir.path().join("accounts_lifecycle_e2e.db"); + let db = Arc::new( + DatabaseEngine::connect_file(&db_path, &secrets) + .await + .expect("open temp file-backed database"), + ); + + let token_bytes = secrets + .get_or_create_key_bytes(GRPC_TOKEN_ACCOUNT, 32) + .expect("mint gRPC bearer token from mock vault"); + let token = hex::encode(token_bytes); + + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("bind ephemeral loopback port"); + let addr = listener.local_addr().expect("listener has local addr"); + + let event_bus = Arc::new(EventBus::new()); + let server_secrets = secrets.clone(); + let server_token = token.clone(); + let server_filter_engine = Arc::new(FilterEngine::new(Vec::new()).expect("empty rule set")); + tokio::spawn(async move { + let _ = serve_on_listener_with_overrides( + listener, + event_bus, + db, + server_filter_engine, + server_secrets, + server_token, + MailEngineOverrides::default(), + CalendarEngineOverrides::default(), + ContactsEngineOverrides::default(), + accounts_overrides, + ) + .await; + }); + + (addr, token, dir) +} + +#[tokio::test] +async fn account_lifecycle_round_trips_over_authenticated_grpc_offline() { + let secrets = Arc::new(SecretManager::mock()); + let accounts_overrides = AccountsEngineOverrides { + connection_tester: Some(Arc::new(StubConnectionTester)), + }; + let (addr, token, _db_dir) = boot_daemon(secrets, accounts_overrides).await; + let addr_str = addr.to_string(); + + let mut accounts_client = nuncio_proto::client::connect_accounts(&addr_str, &token) + .await + .expect("accounts client connects"); + + // ---- (a) AddAccount persists a StartTls/Plain account ---- + let add_response = accounts_client + .add_account(AddAccountRequest { + config: Some(sample_account_config()), + password: "lifecycle-secret-password".to_string(), + }) + .await + .expect("add_account succeeds") + .into_inner(); + assert_eq!(add_response.id, ACCOUNT_ID); + + // ---- (b) ListAccounts round-trips BOTH distinct TLS modes ---- + let listed = accounts_client + .list_accounts(ListAccountsRequest {}) + .await + .expect("list_accounts succeeds") + .into_inner() + .accounts; + assert_eq!(listed.len(), 1); + assert_eq!(listed[0].imap_tls_mode(), TlsMode::StartTls); + assert_eq!(listed[0].smtp_tls_mode(), TlsMode::Plain); + + // ---- (c) UpdateAccount genuinely mutates the persisted config ---- + let mut updated = sample_account_config(); + updated.name = "Renamed Lifecycle Account".to_string(); + updated.imap_tls_mode = TlsMode::ImplicitTls.into(); + accounts_client + .update_account(UpdateAccountRequest { + config: Some(updated), + password: None, + }) + .await + .expect("update_account succeeds"); + + let relisted = accounts_client + .list_accounts(ListAccountsRequest {}) + .await + .expect("list_accounts succeeds") + .into_inner() + .accounts; + assert_eq!(relisted.len(), 1); + assert_eq!(relisted[0].name, "Renamed Lifecycle Account"); + assert_eq!(relisted[0].imap_tls_mode(), TlsMode::ImplicitTls); + // Untouched fields survive the update. + assert_eq!(relisted[0].smtp_tls_mode(), TlsMode::Plain); + + // ---- (d) TestAccountConnection reports the GENUINE per-protocol result ---- + let report = accounts_client + .test_account_connection(TestAccountConnectionRequest { + id: ACCOUNT_ID.to_string(), + }) + .await + .expect("test_account_connection succeeds") + .into_inner(); + assert!(report.imap_ok); + assert!(!report.smtp_ok); + assert_eq!(report.imap_error, None); + assert_eq!( + report.smtp_error.as_deref(), + Some("SMTP relay refused the connection") + ); + + // ---- (e) RemoveAccount genuinely deletes it ---- + accounts_client + .remove_account(RemoveAccountRequest { + id: ACCOUNT_ID.to_string(), + }) + .await + .expect("remove_account succeeds"); + + let after_remove = accounts_client + .list_accounts(ListAccountsRequest {}) + .await + .expect("list_accounts succeeds") + .into_inner() + .accounts; + assert!(after_remove.is_empty()); + + // A test against the now-removed account is an honest not-found, never a + // fabricated success. + let err = accounts_client + .test_account_connection(TestAccountConnectionRequest { + id: ACCOUNT_ID.to_string(), + }) + .await + .expect_err("testing a removed account must fail"); + assert_eq!(err.code(), tonic::Code::NotFound); +} diff --git a/crates/nunciod/tests/calendar_e2e_test.rs b/crates/nunciod/tests/calendar_e2e_test.rs index d0e90b7..72b534d 100644 --- a/crates/nunciod/tests/calendar_e2e_test.rs +++ b/crates/nunciod/tests/calendar_e2e_test.rs @@ -30,8 +30,8 @@ use nuncio_proto::v1::{CalendarSyncRequest, GetEventRequest, ListEventsRequest}; use nuncio_store::db::DatabaseEngine; use nuncio_store::vault::{SecretManager, GRPC_TOKEN_ACCOUNT}; use nunciod::grpc::{ - serve_on_listener_with_overrides, CalendarEngineOverrides, ContactsEngineOverrides, - MailEngineOverrides, + serve_on_listener_with_overrides, AccountsEngineOverrides, CalendarEngineOverrides, + ContactsEngineOverrides, MailEngineOverrides, }; use std::sync::Arc; use tokio::net::TcpListener; @@ -95,6 +95,7 @@ async fn boot_daemon( MailEngineOverrides::default(), calendar_overrides, ContactsEngineOverrides::default(), + AccountsEngineOverrides::default(), ) .await; }); diff --git a/crates/nunciod/tests/contacts_e2e_test.rs b/crates/nunciod/tests/contacts_e2e_test.rs index b5e9d8f..f231f39 100644 --- a/crates/nunciod/tests/contacts_e2e_test.rs +++ b/crates/nunciod/tests/contacts_e2e_test.rs @@ -39,8 +39,8 @@ use nuncio_proto::v1::{ use nuncio_store::db::DatabaseEngine; use nuncio_store::vault::{SecretManager, GRPC_TOKEN_ACCOUNT}; use nunciod::grpc::{ - serve_on_listener_with_overrides, CalendarEngineOverrides, ContactsEngineOverrides, - MailEngineOverrides, + serve_on_listener_with_overrides, AccountsEngineOverrides, CalendarEngineOverrides, + ContactsEngineOverrides, MailEngineOverrides, }; use std::sync::Arc; use tokio::net::TcpListener; @@ -97,6 +97,7 @@ async fn boot_daemon( MailEngineOverrides::default(), CalendarEngineOverrides::default(), contacts_overrides, + AccountsEngineOverrides::default(), ) .await; }); diff --git a/crates/nunciod/tests/grpc_auth_test.rs b/crates/nunciod/tests/grpc_auth_test.rs index 767e051..22b543e 100644 --- a/crates/nunciod/tests/grpc_auth_test.rs +++ b/crates/nunciod/tests/grpc_auth_test.rs @@ -188,6 +188,54 @@ async fn rejects_calls_to_new_filters_rpcs_without_a_bearer_token() { assert_eq!(err.code(), Code::Unauthenticated); } +/// Confirms the newly added `Accounts` lifecycle RPCs +/// (`UpdateAccount`/`RemoveAccount`/`TestAccountConnection`) are each mounted +/// behind the SAME `BearerAuthInterceptor` as every other service -- an +/// unauthenticated call to any of them must be rejected exactly like an +/// unauthenticated `System/GetStatus`, with no carve-out for "just one more" +/// RPC. +#[tokio::test] +async fn rejects_calls_to_new_accounts_rpcs_without_a_bearer_token() { + let event_bus = Arc::new(EventBus::new()); + let secrets = SecretManager::mock(); + let token = hex::encode( + secrets + .get_or_create_key_bytes(GRPC_TOKEN_ACCOUNT, 32) + .expect("token provisioned from mock vault"), + ); + + let (addr, _dir) = start_server(event_bus, token).await; + let mut client = + nuncio_proto::v1::accounts_client::AccountsClient::connect(format!("http://{addr}")) + .await + .expect("client connects"); + + let update_err = client + .update_account(nuncio_proto::v1::UpdateAccountRequest { + config: None, + password: None, + }) + .await + .expect_err("unauthenticated update_account must be rejected"); + assert_eq!(update_err.code(), Code::Unauthenticated); + + let remove_err = client + .remove_account(nuncio_proto::v1::RemoveAccountRequest { + id: "acct-1".to_string(), + }) + .await + .expect_err("unauthenticated remove_account must be rejected"); + assert_eq!(remove_err.code(), Code::Unauthenticated); + + let test_err = client + .test_account_connection(nuncio_proto::v1::TestAccountConnectionRequest { + id: "acct-1".to_string(), + }) + .await + .expect_err("unauthenticated test_account_connection must be rejected"); + assert_eq!(test_err.code(), Code::Unauthenticated); +} + /// Confirms the streaming `Filters.Triage` RPC is mounted behind the SAME /// `BearerAuthInterceptor` as every other `Filters` RPC -- an unauthenticated /// call must be rejected before the stream is ever established, exactly diff --git a/crates/nunciod/tests/spine_e2e_test.rs b/crates/nunciod/tests/spine_e2e_test.rs index 27427c1..f4d6253 100644 --- a/crates/nunciod/tests/spine_e2e_test.rs +++ b/crates/nunciod/tests/spine_e2e_test.rs @@ -43,8 +43,8 @@ use nuncio_proto::v1::{ use nuncio_store::db::DatabaseEngine; use nuncio_store::vault::{SecretManager, GRPC_TOKEN_ACCOUNT}; use nunciod::grpc::{ - serve_on_listener_with_overrides, CalendarEngineOverrides, ContactsEngineOverrides, - MailEngineOverrides, + serve_on_listener_with_overrides, AccountsEngineOverrides, CalendarEngineOverrides, + ContactsEngineOverrides, MailEngineOverrides, }; use std::sync::Arc; use tokio::net::TcpListener; @@ -130,6 +130,7 @@ async fn boot_daemon( overrides, CalendarEngineOverrides::default(), ContactsEngineOverrides::default(), + AccountsEngineOverrides::default(), ) .await; });