all lists on 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal