From 6ca2c2774757662663d229f1cb61b57e26f21318 Mon Sep 17 00:00:00 2001
From: James Hillyerd <james@hillyerd.com>
Date: Mon, 16 Jan 2017 10:50:28 -0800
Subject: [PATCH] Pull message delivery into its own method

---
 smtpd/handler.go         | 85 ++++++++++++++++++++++------------------
 swaks-tests/run-tests.sh |  2 +-
 2 files changed, 48 insertions(+), 39 deletions(-)

diff --git a/smtpd/handler.go b/smtpd/handler.go
index a9b49a1..0f0e3d9 100644
--- a/smtpd/handler.go
+++ b/smtpd/handler.go
@@ -67,6 +67,12 @@ var commands = map[string]bool{
 	"TURN": true,
 }
 
+// recipientDetails for message delivery
+type recipientDetails struct {
+	address, localPart, domainPart string
+	mailbox                        Mailbox
+}
+
 // Session holds the state of an SMTP session
 type Session struct {
 	server       *Server
@@ -341,13 +347,7 @@ func (ss *Session) mailHandler(cmd string, arg string) {
 
 // DATA
 func (ss *Session) dataHandler() {
-	type RecipientDetails struct {
-		address, localPart, domainPart string
-		mailbox                        Mailbox
-	}
-	recipients := make([]RecipientDetails, 0, ss.recipients.Len())
-	// Timestamp for Received header
-	stamp := time.Now().Format(timeStampFormat)
+	recipients := make([]recipientDetails, 0, ss.recipients.Len())
 	// Get a Mailbox and a new Message for each recipient
 	msgSize := 0
 	if ss.server.storeMessages {
@@ -369,7 +369,7 @@ func (ss *Session) dataHandler() {
 					ss.reset()
 					return
 				}
-				recipients = append(recipients, RecipientDetails{recip, local, domain, mb})
+				recipients = append(recipients, recipientDetails{recip, local, domain, mb})
 			} else {
 				log.Tracef("Not storing message for %q", recip)
 			}
@@ -399,39 +399,15 @@ func (ss *Session) dataHandler() {
 			if ss.server.storeMessages {
 				// Create a message for each valid recipient
 				for _, r := range recipients {
-					msg, err := r.mailbox.NewMessage()
-					if err != nil {
-						ss.logError("Failed to create message for %q: %s", r.localPart, err)
-						ss.send(fmt.Sprintf("451 Failed to create message for %v", r.localPart))
+					if ok := ss.deliverMessage(r, msgBuf); ok {
+						expReceivedTotal.Add(1)
+					} else {
+						// Delivery failure
+						ss.send(fmt.Sprintf("451 Failed to store message for %v", r.localPart))
 						ss.reset()
 						return
 					}
-
-					// Generate Received header
-					recd := fmt.Sprintf("Received: from %s ([%s]) by %s\r\n  for <%s>; %s\r\n",
-						ss.remoteDomain, ss.remoteHost, ss.server.domain, r.address, stamp)
-					if err := msg.Append([]byte(recd)); err != nil {
-						ss.logError("Failed to write received header for %q: %s", r.localPart, err)
-						ss.send(fmt.Sprintf("451 Failed to create message for %v", r.localPart))
-						ss.reset()
-						return
-					}
-					// Append lines from msgBuf
-					for _, line = range msgBuf {
-						if err := msg.Append(line); err != nil {
-							ss.logError("Failed to append to mailbox %v: %v",
-								r.mailbox, err)
-							ss.send("554 Something went wrong")
-							ss.reset()
-							// Should really cleanup the crap on filesystem
-							return
-						}
-					}
-					if err := msg.Close(); err != nil {
-						ss.logError("Error: %v while writing message", err)
-					}
-					expReceivedTotal.Add(1)
-				} // end for
+				}
 			} else {
 				expReceivedTotal.Add(1)
 			}
@@ -458,6 +434,39 @@ func (ss *Session) dataHandler() {
 	} // end for
 }
 
+// deliverMessage creates and populates a new Message for the specified recipient
+func (ss *Session) deliverMessage(r recipientDetails, msgBuf [][]byte) (ok bool) {
+	msg, err := r.mailbox.NewMessage()
+	if err != nil {
+		ss.logError("Failed to create message for %q: %s", r.localPart, err)
+		return false
+	}
+
+	// Generate Received header
+	stamp := time.Now().Format(timeStampFormat)
+	recd := fmt.Sprintf("Received: from %s ([%s]) by %s\r\n  for <%s>; %s\r\n",
+		ss.remoteDomain, ss.remoteHost, ss.server.domain, r.address, stamp)
+	if err := msg.Append([]byte(recd)); err != nil {
+		ss.logError("Failed to write received header for %q: %s", r.localPart, err)
+		return false
+	}
+
+	// Append lines from msgBuf
+	for _, line := range msgBuf {
+		if err := msg.Append(line); err != nil {
+			ss.logError("Failed to append to mailbox %v: %v", r.mailbox, err)
+			// Should really cleanup the crap on filesystem
+			return false
+		}
+	}
+	if err := msg.Close(); err != nil {
+		ss.logError("Error while closing message for %v: %v", r.mailbox, err)
+		return false
+	}
+
+	return true
+}
+
 func (ss *Session) enterState(state State) {
 	ss.state = state
 	ss.logTrace("Entering state %v", state)
diff --git a/swaks-tests/run-tests.sh b/swaks-tests/run-tests.sh
index f7c3564..e747dde 100755
--- a/swaks-tests/run-tests.sh
+++ b/swaks-tests/run-tests.sh
@@ -31,7 +31,7 @@ export SWAKS_OPT_to="$to@inbucket.local"
 swaks $* --h-Subject: "Swaks Plain Text" --body text.txt
 
 # Multi-recipient test
-swaks $* --to="$to@inbucket.local,Alt User <alternate@inbucket.local>" --h-Subject: "Swaks Multi-Recipient" \
+swaks $* --to="$to@inbucket.local,alternate@inbucket.local" --h-Subject: "Swaks Multi-Recipient" \
   --body text.txt
 
 # HTML test