public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys
@ 2024-06-24 12:31 Lukas Wagner
  2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 2/6] sys: mark email fn's as deprecated Lukas Wagner
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-06-24 12:31 UTC (permalink / raw)
  To: pve-devel

proxmox_notify is the only user of those functions, so it makes
sense to move them here. A future commit will mark the
original functions from proxmox_sys as deprecated.

The functions were slightly modified, mostly to not
rely on anyhow for error reporting. Also they
are now private functions.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-notify/Cargo.toml                |   1 +
 proxmox-notify/src/endpoints/sendmail.rs | 189 ++++++++++++++++++++++-
 2 files changed, 188 insertions(+), 2 deletions(-)

diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
index d3eae584..e55be0cc 100644
--- a/proxmox-notify/Cargo.toml
+++ b/proxmox-notify/Cargo.toml
@@ -9,6 +9,7 @@ exclude.workspace = true
 
 [dependencies]
 anyhow.workspace = true
+base64.workspace = true
 const_format.workspace = true
 handlebars = { workspace = true }
 lettre = { workspace = true, optional = true }
diff --git a/proxmox-notify/src/endpoints/sendmail.rs b/proxmox-notify/src/endpoints/sendmail.rs
index da0c0cc7..e75902fc 100644
--- a/proxmox-notify/src/endpoints/sendmail.rs
+++ b/proxmox-notify/src/endpoints/sendmail.rs
@@ -1,3 +1,6 @@
+use std::io::Write;
+use std::process::{Command, Stdio};
+
 use serde::{Deserialize, Serialize};
 
 use proxmox_schema::api_types::COMMENT_SCHEMA;
@@ -133,7 +136,7 @@ impl Endpoint for SendmailEndpoint {
                     .clone()
                     .unwrap_or_else(|| context().default_sendmail_author());
 
-                proxmox_sys::email::sendmail(
+                sendmail(
                     &recipients_str,
                     &subject,
                     Some(&text_part),
@@ -145,7 +148,7 @@ impl Endpoint for SendmailEndpoint {
             }
             #[cfg(feature = "mail-forwarder")]
             Content::ForwardedMail { raw, uid, .. } => {
-                proxmox_sys::email::forward(&recipients_str, &mailfrom, raw, *uid)
+                forward(&recipients_str, &mailfrom, raw, *uid)
                     .map_err(|err| Error::NotifyFailed(self.config.name.clone(), err.into()))
             }
         }
@@ -160,3 +163,185 @@ impl Endpoint for SendmailEndpoint {
         self.config.disable.unwrap_or_default()
     }
 }
+
+/// Sends multi-part mail with text and/or html to a list of recipients
+///
+/// Includes the header `Auto-Submitted: auto-generated`, so that auto-replies
+/// (i.e. OOO replies) won't trigger.
+/// ``sendmail`` is used for sending the mail.
+fn sendmail(
+    mailto: &[&str],
+    subject: &str,
+    text: Option<&str>,
+    html: Option<&str>,
+    mailfrom: Option<&str>,
+    author: Option<&str>,
+) -> Result<(), Error> {
+    use std::fmt::Write as _;
+
+    if mailto.is_empty() {
+        return Err(Error::Generic(
+            "At least one recipient has to be specified!".into(),
+        ));
+    }
+    let mailfrom = mailfrom.unwrap_or("root");
+    let recipients = mailto.join(",");
+    let author = author.unwrap_or("Proxmox Backup Server");
+
+    let now = proxmox_time::epoch_i64();
+
+    let mut sendmail_process = match Command::new("/usr/sbin/sendmail")
+        .arg("-B")
+        .arg("8BITMIME")
+        .arg("-f")
+        .arg(mailfrom)
+        .arg("--")
+        .args(mailto)
+        .stdin(Stdio::piped())
+        .spawn()
+    {
+        Err(err) => {
+            return Err(Error::Generic(format!(
+                "could not spawn sendmail process: {err}"
+            )))
+        }
+        Ok(process) => process,
+    };
+    let mut is_multipart = false;
+    if let (Some(_), Some(_)) = (text, html) {
+        is_multipart = true;
+    }
+
+    let mut body = String::new();
+    let boundary = format!("----_=_NextPart_001_{}", now);
+    if is_multipart {
+        body.push_str("Content-Type: multipart/alternative;\n");
+        let _ = writeln!(body, "\tboundary=\"{}\"", boundary);
+        body.push_str("MIME-Version: 1.0\n");
+    } else if !subject.is_ascii() {
+        body.push_str("MIME-Version: 1.0\n");
+    }
+    if !subject.is_ascii() {
+        let _ = writeln!(body, "Subject: =?utf-8?B?{}?=", base64::encode(subject));
+    } else {
+        let _ = writeln!(body, "Subject: {}", subject);
+    }
+    let _ = writeln!(body, "From: {} <{}>", author, mailfrom);
+    let _ = writeln!(body, "To: {}", &recipients);
+    let rfc2822_date = proxmox_time::epoch_to_rfc2822(now)
+        .map_err(|err| Error::Generic(format!("failed to format time: {err}")))?;
+    let _ = writeln!(body, "Date: {}", rfc2822_date);
+    body.push_str("Auto-Submitted: auto-generated;\n");
+
+    if is_multipart {
+        body.push('\n');
+        body.push_str("This is a multi-part message in MIME format.\n");
+        let _ = write!(body, "\n--{}\n", boundary);
+    }
+    if let Some(text) = text {
+        body.push_str("Content-Type: text/plain;\n");
+        body.push_str("\tcharset=\"UTF-8\"\n");
+        body.push_str("Content-Transfer-Encoding: 8bit\n");
+        body.push('\n');
+        body.push_str(text);
+        if is_multipart {
+            let _ = write!(body, "\n--{}\n", boundary);
+        }
+    }
+    if let Some(html) = html {
+        body.push_str("Content-Type: text/html;\n");
+        body.push_str("\tcharset=\"UTF-8\"\n");
+        body.push_str("Content-Transfer-Encoding: 8bit\n");
+        body.push('\n');
+        body.push_str(html);
+        if is_multipart {
+            let _ = write!(body, "\n--{}--", boundary);
+        }
+    }
+
+    if let Err(err) = sendmail_process
+        .stdin
+        .take()
+        .unwrap()
+        .write_all(body.as_bytes())
+    {
+        return Err(Error::Generic(format!(
+            "couldn't write to sendmail stdin: {err}"
+        )));
+    };
+
+    // wait() closes stdin of the child
+    if let Err(err) = sendmail_process.wait() {
+        return Err(Error::Generic(format!(
+            "sendmail did not exit successfully: {err}"
+        )));
+    }
+
+    Ok(())
+}
+
+/// Forwards an email message to a given list of recipients.
+///
+/// ``sendmail`` is used for sending the mail, thus `message` must be
+/// compatible with that (the message is piped into stdin unmodified).
+#[cfg(feature = "mail-forwarder")]
+fn forward(mailto: &[&str], mailfrom: &str, message: &[u8], uid: Option<u32>) -> Result<(), Error> {
+    use std::os::unix::process::CommandExt;
+
+    if mailto.is_empty() {
+        return Err(Error::Generic(
+            "At least one recipient has to be specified!".into(),
+        ));
+    }
+
+    let mut builder = Command::new("/usr/sbin/sendmail");
+
+    builder
+        .args([
+            "-N", "never", // never send DSN (avoid mail loops)
+            "-f", mailfrom, "--",
+        ])
+        .args(mailto)
+        .stdin(Stdio::piped())
+        .stdout(Stdio::null())
+        .stderr(Stdio::null());
+
+    if let Some(uid) = uid {
+        builder.uid(uid);
+    }
+
+    let mut process = builder
+        .spawn()
+        .map_err(|err| Error::Generic(format!("could not spawn sendmail process: {err}")))?;
+
+    process
+        .stdin
+        .take()
+        .unwrap()
+        .write_all(message)
+        .map_err(|err| Error::Generic(format!("couldn't write to sendmail stdin: {err}")))?;
+
+    process
+        .wait()
+        .map_err(|err| Error::Generic(format!("sendmail did not exit successfully: {err}")))?;
+
+    Ok(())
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn email_without_recipients() {
+        let result = sendmail(
+            &[],
+            "Subject2",
+            None,
+            Some("<b>HTML</b>"),
+            None,
+            Some("test1"),
+        );
+        assert!(result.is_err());
+    }
+}
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [pve-devel] [PATCH proxmox 2/6] sys: mark email fn's as deprecated
  2024-06-24 12:31 [pve-devel] [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys Lukas Wagner
@ 2024-06-24 12:31 ` Lukas Wagner
  2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 3/6] notify: sendmail: make mailfrom and author non-optional Lukas Wagner
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-06-24 12:31 UTC (permalink / raw)
  To: pve-devel

The only user was proxmox-notify which now uses its own
copies of these functions.

Also added #[allow(deprecated)] to the test case cause
we don't want any deprecation warnings when running the
test.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-sys/src/email.rs | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/proxmox-sys/src/email.rs b/proxmox-sys/src/email.rs
index 85d171d7..eb8fd10a 100644
--- a/proxmox-sys/src/email.rs
+++ b/proxmox-sys/src/email.rs
@@ -10,6 +10,7 @@ use anyhow::{bail, format_err, Error};
 /// Includes the header `Auto-Submitted: auto-generated`, so that auto-replies
 /// (i.e. OOO replies) won't trigger.
 /// ``sendmail`` is used for sending the mail.
+#[deprecated(note="Use proxmox-notify's abstractions instead")]
 pub fn sendmail(
     mailto: &[&str],
     subject: &str,
@@ -114,6 +115,7 @@ pub fn sendmail(
 ///
 /// ``sendmail`` is used for sending the mail, thus `message` must be
 /// compatible with that (the message is piped into stdin unmodified).
+#[deprecated(note="Use proxmox-notify's abstractions instead")]
 pub fn forward(
     mailto: &[&str],
     mailfrom: &str,
@@ -162,6 +164,7 @@ pub fn forward(
 
 #[cfg(test)]
 mod test {
+    #![allow(deprecated)]
     use crate::email::sendmail;
 
     #[test]
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [pve-devel] [PATCH proxmox 3/6] notify: sendmail: make mailfrom and author non-optional
  2024-06-24 12:31 [pve-devel] [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys Lukas Wagner
  2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 2/6] sys: mark email fn's as deprecated Lukas Wagner
@ 2024-06-24 12:31 ` Lukas Wagner
  2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 4/6] notify: move mail formatting to separate function Lukas Wagner
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-06-24 12:31 UTC (permalink / raw)
  To: pve-devel

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-notify/src/endpoints/sendmail.rs | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/proxmox-notify/src/endpoints/sendmail.rs b/proxmox-notify/src/endpoints/sendmail.rs
index e75902fc..0f7a61b0 100644
--- a/proxmox-notify/src/endpoints/sendmail.rs
+++ b/proxmox-notify/src/endpoints/sendmail.rs
@@ -141,8 +141,8 @@ impl Endpoint for SendmailEndpoint {
                     &subject,
                     Some(&text_part),
                     Some(&html_part),
-                    Some(&mailfrom),
-                    Some(&author),
+                    &mailfrom,
+                    &author,
                 )
                 .map_err(|err| Error::NotifyFailed(self.config.name.clone(), err.into()))
             }
@@ -174,8 +174,8 @@ fn sendmail(
     subject: &str,
     text: Option<&str>,
     html: Option<&str>,
-    mailfrom: Option<&str>,
-    author: Option<&str>,
+    mailfrom: &str,
+    author: &str,
 ) -> Result<(), Error> {
     use std::fmt::Write as _;
 
@@ -184,9 +184,7 @@ fn sendmail(
             "At least one recipient has to be specified!".into(),
         ));
     }
-    let mailfrom = mailfrom.unwrap_or("root");
     let recipients = mailto.join(",");
-    let author = author.unwrap_or("Proxmox Backup Server");
 
     let now = proxmox_time::epoch_i64();
 
@@ -339,8 +337,8 @@ mod test {
             "Subject2",
             None,
             Some("<b>HTML</b>"),
-            None,
-            Some("test1"),
+            "root",
+            "Proxmox",
         );
         assert!(result.is_err());
     }
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [pve-devel] [PATCH proxmox 4/6] notify: move mail formatting to separate function
  2024-06-24 12:31 [pve-devel] [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys Lukas Wagner
  2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 2/6] sys: mark email fn's as deprecated Lukas Wagner
  2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 3/6] notify: sendmail: make mailfrom and author non-optional Lukas Wagner
@ 2024-06-24 12:31 ` Lukas Wagner
  2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 5/6] notify: sendmail: always send multi-part message Lukas Wagner
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-06-24 12:31 UTC (permalink / raw)
  To: pve-devel

This way we can test this in a sane manner and refactor
safely.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-notify/src/endpoints/sendmail.rs | 109 +++++++++++++++++------
 1 file changed, 81 insertions(+), 28 deletions(-)

diff --git a/proxmox-notify/src/endpoints/sendmail.rs b/proxmox-notify/src/endpoints/sendmail.rs
index 0f7a61b0..241a2578 100644
--- a/proxmox-notify/src/endpoints/sendmail.rs
+++ b/proxmox-notify/src/endpoints/sendmail.rs
@@ -177,16 +177,13 @@ fn sendmail(
     mailfrom: &str,
     author: &str,
 ) -> Result<(), Error> {
-    use std::fmt::Write as _;
-
     if mailto.is_empty() {
         return Err(Error::Generic(
             "At least one recipient has to be specified!".into(),
         ));
     }
-    let recipients = mailto.join(",");
-
     let now = proxmox_time::epoch_i64();
+    let body = format_mail(mailto, mailfrom, author, subject, text, html, now)?;
 
     let mut sendmail_process = match Command::new("/usr/sbin/sendmail")
         .arg("-B")
@@ -205,13 +202,46 @@ fn sendmail(
         }
         Ok(process) => process,
     };
+
+    if let Err(err) = sendmail_process
+        .stdin
+        .take()
+        .unwrap()
+        .write_all(body.as_bytes())
+    {
+        return Err(Error::Generic(format!(
+            "couldn't write to sendmail stdin: {err}"
+        )));
+    };
+
+    // wait() closes stdin of the child
+    if let Err(err) = sendmail_process.wait() {
+        return Err(Error::Generic(format!(
+            "sendmail did not exit successfully: {err}"
+        )));
+    }
+
+    Ok(())
+}
+
+fn format_mail(
+    mailto: &[&str],
+    mailfrom: &str,
+    author: &str,
+    subject: &str,
+    text: Option<&str>,
+    html: Option<&str>,
+    timestamp: i64,
+) -> Result<String, Error> {
+    use std::fmt::Write as _;
+
+    let recipients = mailto.join(",");
     let mut is_multipart = false;
     if let (Some(_), Some(_)) = (text, html) {
         is_multipart = true;
     }
-
     let mut body = String::new();
-    let boundary = format!("----_=_NextPart_001_{}", now);
+    let boundary = format!("----_=_NextPart_001_{}", timestamp);
     if is_multipart {
         body.push_str("Content-Type: multipart/alternative;\n");
         let _ = writeln!(body, "\tboundary=\"{}\"", boundary);
@@ -226,11 +256,10 @@ fn sendmail(
     }
     let _ = writeln!(body, "From: {} <{}>", author, mailfrom);
     let _ = writeln!(body, "To: {}", &recipients);
-    let rfc2822_date = proxmox_time::epoch_to_rfc2822(now)
+    let rfc2822_date = proxmox_time::epoch_to_rfc2822(timestamp)
         .map_err(|err| Error::Generic(format!("failed to format time: {err}")))?;
     let _ = writeln!(body, "Date: {}", rfc2822_date);
     body.push_str("Auto-Submitted: auto-generated;\n");
-
     if is_multipart {
         body.push('\n');
         body.push_str("This is a multi-part message in MIME format.\n");
@@ -256,26 +285,7 @@ fn sendmail(
             let _ = write!(body, "\n--{}--", boundary);
         }
     }
-
-    if let Err(err) = sendmail_process
-        .stdin
-        .take()
-        .unwrap()
-        .write_all(body.as_bytes())
-    {
-        return Err(Error::Generic(format!(
-            "couldn't write to sendmail stdin: {err}"
-        )));
-    };
-
-    // wait() closes stdin of the child
-    if let Err(err) = sendmail_process.wait() {
-        return Err(Error::Generic(format!(
-            "sendmail did not exit successfully: {err}"
-        )));
-    }
-
-    Ok(())
+    Ok(body)
 }
 
 /// Forwards an email message to a given list of recipients.
@@ -342,4 +352,47 @@ mod test {
         );
         assert!(result.is_err());
     }
+
+    #[test]
+    fn test_format_mail_multipart() {
+        let message = format_mail(
+            &["Tony Est <test@example.com>"],
+            "foobar@example.com",
+            "Fred Oobar",
+            "This is the subject",
+            Some("This is the plain body"),
+            Some("<body>This is the HTML body</body>"),
+            1718977850,
+        )
+        .expect("format_message failed");
+
+        assert_eq!(
+            message,
+            r#"Content-Type: multipart/alternative;
+	boundary="----_=_NextPart_001_1718977850"
+MIME-Version: 1.0
+Subject: This is the subject
+From: Fred Oobar <foobar@example.com>
+To: Tony Est <test@example.com>
+Date: Fri, 21 Jun 2024 15:50:50 +0200
+Auto-Submitted: auto-generated;
+
+This is a multi-part message in MIME format.
+
+------_=_NextPart_001_1718977850
+Content-Type: text/plain;
+	charset="UTF-8"
+Content-Transfer-Encoding: 8bit
+
+This is the plain body
+------_=_NextPart_001_1718977850
+Content-Type: text/html;
+	charset="UTF-8"
+Content-Transfer-Encoding: 8bit
+
+<body>This is the HTML body</body>
+------_=_NextPart_001_1718977850--"#
+                .to_owned()
+        );
+    }
 }
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [pve-devel] [PATCH proxmox 5/6] notify: sendmail: always send multi-part message
  2024-06-24 12:31 [pve-devel] [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys Lukas Wagner
                   ` (2 preceding siblings ...)
  2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 4/6] notify: move mail formatting to separate function Lukas Wagner
@ 2024-06-24 12:31 ` Lukas Wagner
  2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 6/6] notify: sendmail: code style improvements Lukas Wagner
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-06-24 12:31 UTC (permalink / raw)
  To: pve-devel

Even if we don't have an HTML template available, we always
send an HTML part (the plain text part wrapped in <pre>) to
improve rendering in certain mail clients. This means
we can simply message formatting, since we do not have to
distinguish between single-part and multi-part messages.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-notify/src/endpoints/sendmail.rs | 81 +++++++++---------------
 1 file changed, 29 insertions(+), 52 deletions(-)

diff --git a/proxmox-notify/src/endpoints/sendmail.rs b/proxmox-notify/src/endpoints/sendmail.rs
index 241a2578..c28d9211 100644
--- a/proxmox-notify/src/endpoints/sendmail.rs
+++ b/proxmox-notify/src/endpoints/sendmail.rs
@@ -139,8 +139,8 @@ impl Endpoint for SendmailEndpoint {
                 sendmail(
                     &recipients_str,
                     &subject,
-                    Some(&text_part),
-                    Some(&html_part),
+                    &text_part,
+                    &html_part,
                     &mailfrom,
                     &author,
                 )
@@ -172,8 +172,8 @@ impl Endpoint for SendmailEndpoint {
 fn sendmail(
     mailto: &[&str],
     subject: &str,
-    text: Option<&str>,
-    html: Option<&str>,
+    text: &str,
+    html: &str,
     mailfrom: &str,
     author: &str,
 ) -> Result<(), Error> {
@@ -229,26 +229,20 @@ fn format_mail(
     mailfrom: &str,
     author: &str,
     subject: &str,
-    text: Option<&str>,
-    html: Option<&str>,
+    text: &str,
+    html: &str,
     timestamp: i64,
 ) -> Result<String, Error> {
     use std::fmt::Write as _;
 
     let recipients = mailto.join(",");
-    let mut is_multipart = false;
-    if let (Some(_), Some(_)) = (text, html) {
-        is_multipart = true;
-    }
     let mut body = String::new();
+
     let boundary = format!("----_=_NextPart_001_{}", timestamp);
-    if is_multipart {
-        body.push_str("Content-Type: multipart/alternative;\n");
-        let _ = writeln!(body, "\tboundary=\"{}\"", boundary);
-        body.push_str("MIME-Version: 1.0\n");
-    } else if !subject.is_ascii() {
-        body.push_str("MIME-Version: 1.0\n");
-    }
+    body.push_str("Content-Type: multipart/alternative;\n");
+    let _ = writeln!(body, "\tboundary=\"{}\"", boundary);
+    body.push_str("MIME-Version: 1.0\n");
+
     if !subject.is_ascii() {
         let _ = writeln!(body, "Subject: =?utf-8?B?{}?=", base64::encode(subject));
     } else {
@@ -260,31 +254,21 @@ fn format_mail(
         .map_err(|err| Error::Generic(format!("failed to format time: {err}")))?;
     let _ = writeln!(body, "Date: {}", rfc2822_date);
     body.push_str("Auto-Submitted: auto-generated;\n");
-    if is_multipart {
-        body.push('\n');
-        body.push_str("This is a multi-part message in MIME format.\n");
-        let _ = write!(body, "\n--{}\n", boundary);
-    }
-    if let Some(text) = text {
-        body.push_str("Content-Type: text/plain;\n");
-        body.push_str("\tcharset=\"UTF-8\"\n");
-        body.push_str("Content-Transfer-Encoding: 8bit\n");
-        body.push('\n');
-        body.push_str(text);
-        if is_multipart {
-            let _ = write!(body, "\n--{}\n", boundary);
-        }
-    }
-    if let Some(html) = html {
-        body.push_str("Content-Type: text/html;\n");
-        body.push_str("\tcharset=\"UTF-8\"\n");
-        body.push_str("Content-Transfer-Encoding: 8bit\n");
-        body.push('\n');
-        body.push_str(html);
-        if is_multipart {
-            let _ = write!(body, "\n--{}--", boundary);
-        }
-    }
+    body.push('\n');
+    body.push_str("This is a multi-part message in MIME format.\n");
+    let _ = write!(body, "\n--{}\n", boundary);
+    body.push_str("Content-Type: text/plain;\n");
+    body.push_str("\tcharset=\"UTF-8\"\n");
+    body.push_str("Content-Transfer-Encoding: 8bit\n");
+    body.push('\n');
+    body.push_str(text);
+    let _ = write!(body, "\n--{}\n", boundary);
+    body.push_str("Content-Type: text/html;\n");
+    body.push_str("\tcharset=\"UTF-8\"\n");
+    body.push_str("Content-Transfer-Encoding: 8bit\n");
+    body.push('\n');
+    body.push_str(html);
+    let _ = write!(body, "\n--{}--", boundary);
     Ok(body)
 }
 
@@ -342,14 +326,7 @@ mod test {
 
     #[test]
     fn email_without_recipients() {
-        let result = sendmail(
-            &[],
-            "Subject2",
-            None,
-            Some("<b>HTML</b>"),
-            "root",
-            "Proxmox",
-        );
+        let result = sendmail(&[], "Subject2", "", "<b>HTML</b>", "root", "Proxmox");
         assert!(result.is_err());
     }
 
@@ -360,8 +337,8 @@ mod test {
             "foobar@example.com",
             "Fred Oobar",
             "This is the subject",
-            Some("This is the plain body"),
-            Some("<body>This is the HTML body</body>"),
+            "This is the plain body",
+            "<body>This is the HTML body</body>",
             1718977850,
         )
         .expect("format_message failed");
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [pve-devel] [PATCH proxmox 6/6] notify: sendmail: code style improvements
  2024-06-24 12:31 [pve-devel] [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys Lukas Wagner
                   ` (3 preceding siblings ...)
  2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 5/6] notify: sendmail: always send multi-part message Lukas Wagner
@ 2024-06-24 12:31 ` Lukas Wagner
  2024-07-12  8:56 ` [pve-devel] partially-applied: [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys Wolfgang Bumiller
  2024-11-25 10:22 ` [pve-devel] " Lukas Wagner
  6 siblings, 0 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-06-24 12:31 UTC (permalink / raw)
  To: pve-devel

No functional changes intended.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-notify/src/endpoints/sendmail.rs | 57 +++++++++++-------------
 1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/proxmox-notify/src/endpoints/sendmail.rs b/proxmox-notify/src/endpoints/sendmail.rs
index c28d9211..42b2d3a8 100644
--- a/proxmox-notify/src/endpoints/sendmail.rs
+++ b/proxmox-notify/src/endpoints/sendmail.rs
@@ -185,7 +185,7 @@ fn sendmail(
     let now = proxmox_time::epoch_i64();
     let body = format_mail(mailto, mailfrom, author, subject, text, html, now)?;
 
-    let mut sendmail_process = match Command::new("/usr/sbin/sendmail")
+    let mut sendmail_process = Command::new("/usr/sbin/sendmail")
         .arg("-B")
         .arg("8BITMIME")
         .arg("-f")
@@ -194,32 +194,18 @@ fn sendmail(
         .args(mailto)
         .stdin(Stdio::piped())
         .spawn()
-    {
-        Err(err) => {
-            return Err(Error::Generic(format!(
-                "could not spawn sendmail process: {err}"
-            )))
-        }
-        Ok(process) => process,
-    };
+        .map_err(|err| Error::Generic(format!("could not spawn sendmail process: {err}")))?;
 
-    if let Err(err) = sendmail_process
+    sendmail_process
         .stdin
         .take()
-        .unwrap()
+        .expect("stdin already taken")
         .write_all(body.as_bytes())
-    {
-        return Err(Error::Generic(format!(
-            "couldn't write to sendmail stdin: {err}"
-        )));
-    };
-
-    // wait() closes stdin of the child
-    if let Err(err) = sendmail_process.wait() {
-        return Err(Error::Generic(format!(
-            "sendmail did not exit successfully: {err}"
-        )));
-    }
+        .map_err(|err| Error::Generic(format!("couldn't write to sendmail stdin: {err}")))?;
+
+    sendmail_process
+        .wait()
+        .map_err(|err| Error::Generic(format!("sendmail did not exit successfully: {err}")))?;
 
     Ok(())
 }
@@ -236,39 +222,46 @@ fn format_mail(
     use std::fmt::Write as _;
 
     let recipients = mailto.join(",");
+    let boundary = format!("----_=_NextPart_001_{timestamp}");
+
     let mut body = String::new();
 
-    let boundary = format!("----_=_NextPart_001_{}", timestamp);
+    // Format email header
     body.push_str("Content-Type: multipart/alternative;\n");
-    let _ = writeln!(body, "\tboundary=\"{}\"", boundary);
+    let _ = writeln!(body, "\tboundary=\"{boundary}\"");
     body.push_str("MIME-Version: 1.0\n");
 
     if !subject.is_ascii() {
         let _ = writeln!(body, "Subject: =?utf-8?B?{}?=", base64::encode(subject));
     } else {
-        let _ = writeln!(body, "Subject: {}", subject);
+        let _ = writeln!(body, "Subject: {subject}");
     }
-    let _ = writeln!(body, "From: {} <{}>", author, mailfrom);
-    let _ = writeln!(body, "To: {}", &recipients);
+    let _ = writeln!(body, "From: {author} <{mailfrom}>");
+    let _ = writeln!(body, "To: {recipients}");
     let rfc2822_date = proxmox_time::epoch_to_rfc2822(timestamp)
         .map_err(|err| Error::Generic(format!("failed to format time: {err}")))?;
-    let _ = writeln!(body, "Date: {}", rfc2822_date);
+    let _ = writeln!(body, "Date: {rfc2822_date}");
     body.push_str("Auto-Submitted: auto-generated;\n");
     body.push('\n');
+
+    // Format email body
     body.push_str("This is a multi-part message in MIME format.\n");
-    let _ = write!(body, "\n--{}\n", boundary);
+    let _ = write!(body, "\n--{boundary}\n");
+
     body.push_str("Content-Type: text/plain;\n");
     body.push_str("\tcharset=\"UTF-8\"\n");
     body.push_str("Content-Transfer-Encoding: 8bit\n");
     body.push('\n');
     body.push_str(text);
-    let _ = write!(body, "\n--{}\n", boundary);
+    let _ = write!(body, "\n--{boundary}\n");
+
     body.push_str("Content-Type: text/html;\n");
     body.push_str("\tcharset=\"UTF-8\"\n");
     body.push_str("Content-Transfer-Encoding: 8bit\n");
     body.push('\n');
     body.push_str(html);
-    let _ = write!(body, "\n--{}--", boundary);
+    let _ = write!(body, "\n--{boundary}--");
+
     Ok(body)
 }
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [pve-devel] partially-applied: [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys
  2024-06-24 12:31 [pve-devel] [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys Lukas Wagner
                   ` (4 preceding siblings ...)
  2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 6/6] notify: sendmail: code style improvements Lukas Wagner
@ 2024-07-12  8:56 ` Wolfgang Bumiller
  2024-11-25 10:22 ` [pve-devel] " Lukas Wagner
  6 siblings, 0 replies; 12+ messages in thread
From: Wolfgang Bumiller @ 2024-07-12  8:56 UTC (permalink / raw)
  To: Lukas Wagner; +Cc: pve-devel

Applied the first patch.
The 2nd one I skipped and is now obsolete, since I instead immediately
removed the functions since I was doing a major bump to the sys crate
anyway.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [pve-devel] [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys
  2024-06-24 12:31 [pve-devel] [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys Lukas Wagner
                   ` (5 preceding siblings ...)
  2024-07-12  8:56 ` [pve-devel] partially-applied: [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys Wolfgang Bumiller
@ 2024-11-25 10:22 ` Lukas Wagner
  2024-11-25 22:19   ` [pve-devel] applied: " Thomas Lamprecht
  6 siblings, 1 reply; 12+ messages in thread
From: Lukas Wagner @ 2024-11-25 10:22 UTC (permalink / raw)
  To: pve-devel

Ping for patches 3-6, these were never applied.
They do not add any functional changes, but only aim to improve
code style and add some tests.

-- 
- Lukas


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [pve-devel] applied: [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys
  2024-11-25 10:22 ` [pve-devel] " Lukas Wagner
@ 2024-11-25 22:19   ` Thomas Lamprecht
  2024-11-26  9:27     ` Shannon Sterz
  0 siblings, 1 reply; 12+ messages in thread
From: Thomas Lamprecht @ 2024-11-25 22:19 UTC (permalink / raw)
  To: Proxmox VE development discussion, Lukas Wagner

Am 25.11.24 um 11:22 schrieb Lukas Wagner:
> Ping for patches 3-6, these were never applied.
> They do not add any functional changes, but only aim to improve
> code style and add some tests.
> 


I applied them now, thanks!

btw. I totally forgot to mention today that Shannon might need these sendmail
stuff too for the internal tooling she works on, so maybe making this public,
or moving it to a micro-crate could indeed be worth it.
Copying such small stuff more than once is most of the time a good signal that
it might be nonetheless good to share it; but no hard feelings either way from
my side, I just wanted to note that use case before I forget it.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [pve-devel] applied: [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys
  2024-11-25 22:19   ` [pve-devel] applied: " Thomas Lamprecht
@ 2024-11-26  9:27     ` Shannon Sterz
  2024-11-26 10:21       ` Thomas Lamprecht
  0 siblings, 1 reply; 12+ messages in thread
From: Shannon Sterz @ 2024-11-26  9:27 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion, Lukas Wagner

On Mon Nov 25, 2024 at 11:19 PM CET, Thomas Lamprecht wrote:
> Am 25.11.24 um 11:22 schrieb Lukas Wagner:
>> Ping for patches 3-6, these were never applied.
>> They do not add any functional changes, but only aim to improve
>> code style and add some tests.
>>
>
>
> I applied them now, thanks!
>
> btw. I totally forgot to mention today that Shannon might need these sendmail
> stuff too for the internal tooling she works on, so maybe making this public,
> or moving it to a micro-crate could indeed be worth it.
> Copying such small stuff more than once is most of the time a good signal that
> it might be nonetheless good to share it; but no hard feelings either way from
> my side, I just wanted to note that use case before I forget it.

actually i needed to adapt this to add support for attachments, which
require reworking the multipart logic a bit too. i can factor that out
into it's own `proxmox-sendmail` crate or similar and make
`proxmox-notify`'s sendmail endpoint use that too. if that's desired, i
can send patches for that too.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [pve-devel] applied: [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys
  2024-11-26  9:27     ` Shannon Sterz
@ 2024-11-26 10:21       ` Thomas Lamprecht
  2024-11-29 10:55         ` Shannon Sterz
  0 siblings, 1 reply; 12+ messages in thread
From: Thomas Lamprecht @ 2024-11-26 10:21 UTC (permalink / raw)
  To: Shannon Sterz, Proxmox VE development discussion, Lukas Wagner

Am 26.11.24 um 10:27 schrieb Shannon Sterz:
> actually i needed to adapt this to add support for attachments, which
> require reworking the multipart logic a bit too. i can factor that out
> into it's own `proxmox-sendmail` crate or similar and make
> `proxmox-notify`'s sendmail endpoint use that too. if that's desired, i
> can send patches for that too.

Yeah, that might be nice. While this probably won't have a very high reusability,
it can be carved out nicely in a standalone crate, and it should also not be a
(fast) moving target, so fine to have as separate crate IMO.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [pve-devel] applied: [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys
  2024-11-26 10:21       ` Thomas Lamprecht
@ 2024-11-29 10:55         ` Shannon Sterz
  0 siblings, 0 replies; 12+ messages in thread
From: Shannon Sterz @ 2024-11-29 10:55 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion, Lukas Wagner

On Tue Nov 26, 2024 at 11:21 AM CET, Thomas Lamprecht wrote:
> Am 26.11.24 um 10:27 schrieb Shannon Sterz:
> > actually i needed to adapt this to add support for attachments, which
> > require reworking the multipart logic a bit too. i can factor that out
> > into it's own `proxmox-sendmail` crate or similar and make
> > `proxmox-notify`'s sendmail endpoint use that too. if that's desired, i
> > can send patches for that too.
>
> Yeah, that might be nice. While this probably won't have a very high reusability,
> it can be carved out nicely in a standalone crate, and it should also not be a
> (fast) moving target, so fine to have as separate crate IMO.

finally managed to send patches for this, see here [1].

[1]: https://lore.proxmox.com/pbs-devel/20241129105321.143877-1-s.sterz@proxmox.com/



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2024-11-29 10:55 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-06-24 12:31 [pve-devel] [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys Lukas Wagner
2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 2/6] sys: mark email fn's as deprecated Lukas Wagner
2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 3/6] notify: sendmail: make mailfrom and author non-optional Lukas Wagner
2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 4/6] notify: move mail formatting to separate function Lukas Wagner
2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 5/6] notify: sendmail: always send multi-part message Lukas Wagner
2024-06-24 12:31 ` [pve-devel] [PATCH proxmox 6/6] notify: sendmail: code style improvements Lukas Wagner
2024-07-12  8:56 ` [pve-devel] partially-applied: [PATCH proxmox 1/6] notify: copy sendmail/forward fn's from proxmox_sys Wolfgang Bumiller
2024-11-25 10:22 ` [pve-devel] " Lukas Wagner
2024-11-25 22:19   ` [pve-devel] applied: " Thomas Lamprecht
2024-11-26  9:27     ` Shannon Sterz
2024-11-26 10:21       ` Thomas Lamprecht
2024-11-29 10:55         ` Shannon Sterz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal