MM-15741: Omit Reply-To email header if EmailSettings.ReplyToAddress … (#10910)

* MM-15741: Omit Reply-To email header if EmailSettings.ReplyToAddress is blank. * MM-15741: Adds tests. * MM-15741: Changes interface name because the '-er' convention doesn't apply. * MM-15741: Combines two structs and un-exports. * MM-15741: Switches to table test. * MM-15741: Renames test. * MM-15741: Unexport interface. * MM-15741: Change to allow test cacheing.
2019-05-23 12:46:35 -04:00
--- a/services/mailservice/mail.go
+++ b/services/mailservice/mail.go
@@ -25,6 +25,14 @@ import (
 	"github.com/mattermost/mattermost-server/utils"
 )

+// smtpClient is implemented by an smtp.Client. See https://golang.org/pkg/net/smtp/#Client.
+//
+type smtpClient interface {
+	Mail(string) error
+	Rcpt(string) error
+	Data() (io.WriteCloser, error)
+}
+
 func encodeRFC2047Word(s string) string {
 	return mime.BEncoding.Encode("utf-8", s)
 }
@@ -231,7 +239,7 @@ func SendMailUsingConfigAdvanced(mimeTo, smtpTo string, from, replyTo mail.Addre
 	return SendMail(c, mimeTo, smtpTo, from, replyTo, subject, htmlBody, attachments, mimeHeaders, fileBackend, time.Now())
 }

-func SendMail(c *smtp.Client, mimeTo, smtpTo string, from, replyTo mail.Address, subject, htmlBody string, attachments []*model.FileInfo, mimeHeaders map[string]string, fileBackend filesstore.FileBackend, date time.Time) *model.AppError {
+func SendMail(c smtpClient, mimeTo, smtpTo string, from, replyTo mail.Address, subject, htmlBody string, attachments []*model.FileInfo, mimeHeaders map[string]string, fileBackend filesstore.FileBackend, date time.Time) *model.AppError {
 	mlog.Debug(fmt.Sprintf("sending mail to %v with subject of '%v'", smtpTo, subject))

 	htmlMessage := "\r\n<html><body>" + htmlBody + "</body></html>"
@@ -244,13 +252,17 @@ func SendMail(c *smtp.Client, mimeTo, smtpTo string, from, replyTo mail.Address,

 	headers := map[string][]string{
 		"From":                      {from.String()},
-		"Reply-To":                  {replyTo.String()},
 		"To":                        {mimeTo},
 		"Subject":                   {encodeRFC2047Word(subject)},
 		"Content-Transfer-Encoding": {"8bit"},
 		"Auto-Submitted":            {"auto-generated"},
 		"Precedence":                {"bulk"},
 	}
+
+	if len(replyTo.Address) > 0 {
+		headers["Reply-To"] = []string{replyTo.String()}
+	}
+
 	for k, v := range mimeHeaders {
 		headers[k] = []string{encodeRFC2047Word(v)}
 	}
--- a/services/mailservice/mail_test.go
+++ b/services/mailservice/mail_test.go
@@ -6,8 +6,12 @@ package mailservice
 import (
 	"bytes"
 	"fmt"
+	"io"
+	"io/ioutil"
+	"os"
 	"strings"
 	"testing"
+	"time"

 	"net/mail"
 	"net/smtp"
@@ -300,3 +304,60 @@ func TestAuthMethods(t *testing.T) {
 		})
 	}
 }
+
+type mockMailer struct {
+	data []byte
+}
+
+func (m *mockMailer) Mail(string) error             { return nil }
+func (m *mockMailer) Rcpt(string) error             { return nil }
+func (m *mockMailer) Data() (io.WriteCloser, error) { return m, nil }
+func (m *mockMailer) Write(p []byte) (int, error) {
+	m.data = append(m.data, p...)
+	return len(p), nil
+}
+func (m *mockMailer) Close() error { return nil }
+
+func TestSendMail(t *testing.T) {
+	dir, err := ioutil.TempDir(".", "mail-test-")
+	require.Nil(t, err)
+	defer os.RemoveAll(dir)
+	settings := model.FileSettings{
+		DriverName: model.NewString(model.IMAGE_DRIVER_LOCAL),
+		Directory:  &dir,
+	}
+	mockBackend, appErr := filesstore.NewFileBackend(&settings, true)
+	require.Nil(t, appErr)
+	mocm := &mockMailer{}
+
+	testCases := map[string]struct {
+		replyTo     mail.Address
+		contains    string
+		notContains string
+	}{
+		"adds reply-to header": {
+			mail.Address{Address: "foo@test.com"},
+			"\r\nReply-To: <foo@test.com>\r\n",
+			"",
+		},
+		"doesn't add reply-to header": {
+			mail.Address{},
+			"",
+			"\r\nReply-To:",
+		},
+	}
+
+	for testName, tc := range testCases {
+		t.Run(testName, func(t *testing.T) {
+			appErr = SendMail(mocm, "", "", mail.Address{}, tc.replyTo, "", "", nil, nil, mockBackend, time.Now())
+			require.Nil(t, appErr)
+			if len(tc.contains) > 0 {
+				require.Contains(t, string(mocm.data), tc.contains)
+			}
+			if len(tc.notContains) > 0 {
+				require.NotContains(t, string(mocm.data), tc.notContains)
+			}
+			mocm.data = []byte{}
+		})
+	}
+}