Repository: lettre/lettre (language: rust)

Issue:
RFC compliance flag
Some design decisions have to be made.

There is the case where the user adds multiple "from" to an email, but doesn't add a sender. In the case of multiple "from" headers, the RFC dictates that there must be a "sender" header. We can
- Choose one from (e.g. the first) as the sender. This might be the wrong one, and thus could introduce a semantic error in the output email.
- Return an error. This error would be so specific, that a badly written software using this lib might just panic instead of handling it correctly.
- leave the "sender" header to create a non-RFC-conform email.

There is the case where the user does not add a "from" header, but manually takes care for the envelope from. The RFC dictates, that there must be exactly one from header and it can't be empty. We can
- Return an error
- Leave the "from" header. This works because all mail programs I tested handle this gracefully by not showing a "from" and this might be exactly what the user wants (i.e. writing a "mysterious" email without an obvious sender)

Currently, in the former case, we do the former and in the latter case the latter. But I think none of the solutions can always fit. Maybe we should add a flag to disable enforcement of the RFC compliance.

In compliant mode, the choices could then be: second, first
In non-compliant mode: third, second

What do you think?



Discussion / hints:
I think there are two main usecases, like for the transport part, with the simple sending method vs. SMTP commands.
- The general case, i.e. a developer who wants to send an email in his program. He does not really know about SMTP/Email formats, but wants to send this text from an address to another. The main goal of lettre is to help him getting this done easily and safely, with understandable errors. That means we should do our best to enforce the RFC and more generally to ensure things will work as expected.
- The lower level case, which is the reason to expose the low level methods as a library. They can be used for a lot of different reasons, some requiring RFC compliance, some requiring not to follow the RFC.

Concerning the header/envelope issue:
- For the first point (several from and no sender), I prefer the error, as I don't like silently taking a decision for the user.
- For the second point (manually set envelope), as it is a lower level method, I think we could keep it as is.

However, I also like the idea of the flag because it allows both usecases while being consistent. Compliant mode should return an error every time we know there will likely be an issue, and non-compliant skip all RFC checks.

Just thinking, for the RFC compliant variant, we could have a separate builder struct, to enforce some constraints via the type system (because compile-time errors > run-time errors).

``` rust
struct RFCMailBuilder;
impl RFCMailBuilder {
    fn set_main_author(self, address) -> EmailBuilder {…}
}
```

The main author would be used as `Sender` if multiple `From` will be added later. And it also works as the minimum one `From` address we need for compliance.

We could also then rename the `EmailBuilder` to `RawEmailBuilder`, so people will be more reluctant to use it directly.

This issue might affect the API of this crate. Therefore I suggest adding its solution to the 1.0 milestone.

After some more thinking: As you said, this crate walks the line between two things:
a) an E-Mail builder with much control, where you set the Headers yourself and are to some degree responsible for RFC compliance. This allows for fine-grained control and is suitable to build e.g. a GUI email client.
b) an easy-to-use E-Mail library with few options, but more guidance, to just "send this text to these people". A user of it needs to know nothing about the RFC, but has less control about the exact headers. This is suitable to daemons that need to notify somebody on some event, or web stores that send emails to customers etc.

I don't see a way that one struct (i.e. `EmailBuilder`) can be excellent at both.

I propose removing all low-level features from `lettre` (e.g. `set_message_type`, `add_child`, `add_header`, envelope customization), adding a whole lot of types to enforce correct usage using the type system and guaranteeing RFC compliance. People who need the low-level features should be pointed to the `email` crate. We can't cover _all_ of their use cases anyway.

After more thinking: I think there is a way to yell at people and still give them the power to change every detail about an EMail. It involves a build.rs script tough...

Wether or not we should yell at a user for an Email depends on these booleans:
- does it have a from header?
- does it have multiple from headers?
- does it have a sender header?
- does it have a to header?
- does it have a subject?
- does it have a body?

The idea is to yell at the user if
- anything important (to, from, subject, body) is missing
- or it has multiple froms, but no sender

We can encode the required booleans into struct names by appending ones and zeros to a name, e.g. "SafeEmailBuilder". Example: `struct SafeEmailBuilder100111` would be the name for a simple, Email that has one from, at least one to, a subject and a body. SafeEmailBuilder would be a typedef for SafeEmailBuilder000000.

Each such struct should be defined like this:

``` rust
struct SafeEmailBuilderXXXXXX {
    raw: EmailBuilder
}
```

All the safe methods of EmailBuilder are also in each of the impls. They are implemented simply like this:

``` rust
impl SafeEmailBuilderXXXXXX {
    pub fn foo(mut self, arg) -> SafeEmailBuilderYYYYYY {
        self.raw.set_foo(arg);
        SafeEmailBuilderYYYYYY { raw: self.raw }
    }
}
```

If you just add a Cc, Bcc or reply-to, the output type will stay the same, i.e. XXXXXX == YYYYYY. But if you add a To, Subject, Sender, body, the type will change, e.g.

``` rust
SafeEmailBuilder00XXXX.to() -> SafeEmailBuilder10XXXX
```

The build method will only be available in the conforming structs:
- 100111
- 111111
- 101111

So if the user tries to build an Email, which is to be yelled at (e.g. something missing), the build method will just not exist for that Email and the compiler will yell at the user. Also the method `body` will be missing from any state XXXXX1, and the method subject missing from any state XXXX1X.

Furthermore, the structs that could be built, will also have an identity (i.e. nothing changing) function namend `assert_buildable`. The structs will also implement `Into<EmailBuilder>` as a "downgrade" possibility. So if you want to make a valid EMail (and make sure it's valid) and then add a custom header, you can do it like this

``` rust
let safe_email = SafeEmailBuilder::new().to(…).bcc(…).from(…).body(…).subject(…).assert_buildable();
let mut footgun_email : EmailBuilder = safe_email.into();
footgun_email.header(…);
```

This would require 3*2^4=48 structs (because 01XXXX codes cannot exist). Of course, we don't want to write these by hand. So we need a build.rs script to generate them.

For stabilization I think, we shouldn't stabilize the intermediate struct names at all. Just say there are structs with those properties and a user may not assume a type name for an intermediate struct.

If we want, we can rename `EmailBuilder` to `RawEmailBuilder` or something like that. This would encourage people to have a look at the safe API when they upgrade. But this is optional.

We could also only add a few of those states by dictating the order in which the parameters must be given (i.e. `mail.from(…).to(…)` would be valid, while `mail.to(…).from(…)` would not). But where is the fun in that? 😄

What do you think?


Fix patch:
diff --git a/src/lib.rs b/src/lib.rs
index e193e2b42..e9ab0adff 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -25,12 +25,11 @@ pub mod error;
 pub mod message;
 pub mod transport;
 
-pub use crate::address::Address;
 use crate::error::Error;
 #[cfg(feature = "builder")]
 pub use crate::message::{
     header::{self, Headers},
-    EmailFormat, Mailboxes, Message,
+    EmailFormat, Mailbox, Mailboxes, Message,
 };
 #[cfg(feature = "file-transport")]
 pub use crate::transport::file::FileTransport;
@@ -42,11 +41,10 @@ pub use crate::transport::smtp::client::net::TlsParameters;
 pub use crate::transport::smtp::r2d2::SmtpConnectionManager;
 #[cfg(feature = "smtp-transport")]
 pub use crate::transport::smtp::{SmtpTransport, Tls};
-pub use crate::transport::stub::StubTransport;
+pub use crate::{address::Address, transport::stub::StubTransport};
 #[cfg(feature = "builder")]
 use std::convert::TryFrom;
-use std::error::Error as StdError;
-use std::fmt;
+use std::{error::Error as StdError, fmt};
 
 /// Simple email envelope representation
 ///
@@ -92,9 +90,15 @@ impl TryFrom<&Headers> for Envelope {
         let from = match headers.get::<header::Sender>() {
             // If there is a Sender, use it
             Some(header::Sender(a)) => Some(a.email.clone()),
-            // ... else use the first From address
+            // ... else try From
             None => match headers.get::<header::From>() {
-                Some(header::From(ref a)) => Some(a.iter().next().unwrap().email.clone()),
+                Some(header::From(a)) => {
+                    let from: Vec<Mailbox> = a.clone().into();
+                    if from.len() > 1 {
+                        return Err(Error::TooManyFrom);
+                    }
+                    Some(from[0].email.clone())
+                }
                 None => None,
             },
         };
diff --git a/src/message/mod.rs b/src/message/mod.rs
index 062dc5bad..cfc8d50ff 100644
--- a/src/message/mod.rs
+++ b/src/message/mod.rs
@@ -364,25 +364,32 @@ impl MessageBuilder {
         self
     }
 
-    fn insert_missing_headers(self) -> Self {
-        let mut new = self;
+    // TODO: High-level methods for attachments and embedded files
+
+    /// Create message from body
+    fn build(self, body: Body) -> Result<Message, EmailError> {
+        // Check for missing required headers
+        // https://tools.ietf.org/html/rfc5322#section-3.6
 
         // Insert Date if missing
-        new = if new.headers.get::<header::Date>().is_none() {
-            new.date_now()
+        let res = if self.headers.get::<header::Date>().is_none() {
+            self.date_now()
         } else {
-            new
+            self
         };
 
-        // TODO insert sender if needed?
-        new
-    }
-
-    // TODO: High-level methods for attachments and embedded files
-
-    /// Create message from body
-    fn build(self, body: Body) -> Result<Message, EmailError> {
-        let res = self.insert_missing_headers();
+        // Fail is missing correct originator (Sender or From)
+        match res.headers.get::<header::From>() {
+            Some(header::From(f)) => {
+                let from: Vec<Mailbox> = f.clone().into();
+                if from.len() > 1 && res.headers.get::<header::Sender>().is_none() {
+                    return Err(EmailError::TooManyFrom);
+                }
+            }
+            None => {
+                return Err(EmailError::MissingFrom);
+            }
+        }
 
         let envelope = match res.envelope {
             Some(e) => e,
@@ -485,6 +492,29 @@ impl Default for MessageBuilder {
 mod test {
     use crate::message::{header, mailbox::Mailbox, Message};
 
+    #[test]
+    fn email_missing_originator() {
+        assert!(Message::builder().body("Happy new year!").is_err());
+    }
+
+    #[test]
+    fn email_miminal_message() {
+        assert!(Message::builder()
+            .from("NoBody <nobody@domain.tld>".parse().unwrap())
+            .to("NoBody <nobody@domain.tld>".parse().unwrap())
+            .body("Happy new year!")
+            .is_ok());
+    }
+
+    #[test]
+    fn email_missing_sender() {
+        assert!(Message::builder()
+            .from("NoBody <nobody@domain.tld>".parse().unwrap())
+            .from("AnyBody <anybody@domain.tld>".parse().unwrap())
+            .body("Happy new year!")
+            .is_err());
+    }
+
     #[test]
     fn email_message() {
         let date = "Tue, 15 Nov 1994 08:12:31 GMT".parse().unwrap();
diff --git a/src/transport/stub/mod.rs b/src/transport/stub/mod.rs
index 0fbd7e8e2..3419b5d84 100644
--- a/src/transport/stub/mod.rs
+++ b/src/transport/stub/mod.rs
@@ -23,8 +23,7 @@
 //! ```
 
 use crate::{Envelope, Transport};
-use std::error::Error as StdError;
-use std::fmt;
+use std::{error::Error as StdError, fmt};
 
 #[derive(Debug, Copy, Clone)]
 pub struct Error;


Test patch: