Isolating configuration, sending emails and connection to SMTP. (#8452)

* Isolating configuration, sending emails and connection to SMTP.

* Building smtpAddress once

* Remove unnecesary errX variables

* Moving mail connection information to new data structure

2 files changed, 142 insertions, 57 deletions

diff --git a/utils/mail.go b/utils/mail.go
index 3b9f4bd9d..c59406a18 100644
--- a/utils/mail.go
+++ b/utils/mail.go
@@ -26,16 +26,27 @@ func encodeRFC2047Word(s string) string {
 	return mime.BEncoding.Encode("utf-8", s)
 }
 
+type SmtpConnectionInfo struct {
+	SmtpUsername         string
+	SmtpPassword         string
+	SmtpServer           string
+	SmtpPort             string
+	SkipCertVerification bool
+	ConnectionSecurity   string
+	Auth                 bool
+}
+
 type authChooser struct {
 	smtp.Auth
-	Config *model.Config
+	connectionInfo *SmtpConnectionInfo
 }
 
 func (a *authChooser) Start(server *smtp.ServerInfo) (string, []byte, error) {
-	a.Auth = LoginAuth(a.Config.EmailSettings.SMTPUsername, a.Config.EmailSettings.SMTPPassword, a.Config.EmailSettings.SMTPServer+":"+a.Config.EmailSettings.SMTPPort)
+	smtpAddress := a.connectionInfo.SmtpServer + ":" + a.connectionInfo.SmtpPort
+	a.Auth = LoginAuth(a.connectionInfo.SmtpUsername, a.connectionInfo.SmtpPassword, smtpAddress)
 	for _, method := range server.Auth {
 		if method == "PLAIN" {
-			a.Auth = smtp.PlainAuth("", a.Config.EmailSettings.SMTPUsername, a.Config.EmailSettings.SMTPPassword, a.Config.EmailSettings.SMTPServer+":"+a.Config.EmailSettings.SMTPPort)
+			a.Auth = smtp.PlainAuth("", a.connectionInfo.SmtpUsername, a.connectionInfo.SmtpPassword, a.connectionInfo.SmtpServer+":"+a.connectionInfo.SmtpPort)
 			break
 		}
 	}
@@ -76,22 +87,23 @@ func (a *loginAuth) Next(fromServer []byte, more bool) ([]byte, error) {
 	return nil, nil
 }
 
-func connectToSMTPServer(config *model.Config) (net.Conn, *model.AppError) {
+func ConnectToSMTPServerAdvanced(connectionInfo *SmtpConnectionInfo) (net.Conn, *model.AppError) {
 	var conn net.Conn
 	var err error
 
-	if config.EmailSettings.ConnectionSecurity == model.CONN_SECURITY_TLS {
+	smtpAddress := connectionInfo.SmtpServer + ":" + connectionInfo.SmtpPort
+	if connectionInfo.ConnectionSecurity == model.CONN_SECURITY_TLS {
 		tlsconfig := &tls.Config{
-			InsecureSkipVerify: *config.EmailSettings.SkipServerCertificateVerification,
-			ServerName:         config.EmailSettings.SMTPServer,
+			InsecureSkipVerify: connectionInfo.SkipCertVerification,
+			ServerName:         connectionInfo.SmtpServer,
 		}
 
-		conn, err = tls.Dial("tcp", config.EmailSettings.SMTPServer+":"+config.EmailSettings.SMTPPort, tlsconfig)
+		conn, err = tls.Dial("tcp", smtpAddress, tlsconfig)
 		if err != nil {
 			return nil, model.NewAppError("SendMail", "utils.mail.connect_smtp.open_tls.app_error", nil, err.Error(), http.StatusInternalServerError)
 		}
 	} else {
-		conn, err = net.Dial("tcp", config.EmailSettings.SMTPServer+":"+config.EmailSettings.SMTPPort)
+		conn, err = net.Dial("tcp", smtpAddress)
 		if err != nil {
 			return nil, model.NewAppError("SendMail", "utils.mail.connect_smtp.open.app_error", nil, err.Error(), http.StatusInternalServerError)
 		}
@@ -100,14 +112,24 @@ func connectToSMTPServer(config *model.Config) (net.Conn, *model.AppError) {
 	return conn, nil
 }
 
-func newSMTPClient(conn net.Conn, config *model.Config) (*smtp.Client, *model.AppError) {
-	c, err := smtp.NewClient(conn, config.EmailSettings.SMTPServer+":"+config.EmailSettings.SMTPPort)
+func ConnectToSMTPServer(config *model.Config) (net.Conn, *model.AppError) {
+	return ConnectToSMTPServerAdvanced(
+		&SmtpConnectionInfo{
+			ConnectionSecurity:   config.EmailSettings.ConnectionSecurity,
+			SkipCertVerification: *config.EmailSettings.SkipServerCertificateVerification,
+			SmtpServer:           config.EmailSettings.SMTPServer,
+			SmtpPort:             config.EmailSettings.SMTPPort,
+		},
+	)
+}
+
+func NewSMTPClientAdvanced(conn net.Conn, hostname string, connectionInfo *SmtpConnectionInfo) (*smtp.Client, *model.AppError) {
+	c, err := smtp.NewClient(conn, connectionInfo.SmtpServer+":"+connectionInfo.SmtpPort)
 	if err != nil {
 		l4g.Error(T("utils.mail.new_client.open.error"), err)
 		return nil, model.NewAppError("SendMail", "utils.mail.connect_smtp.open_tls.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 
-	hostname := GetHostnameFromSiteURL(*config.ServiceSettings.SiteURL)
 	if hostname != "" {
 		err := c.Hello(hostname)
 		if err != nil {
@@ -116,35 +138,51 @@ func newSMTPClient(conn net.Conn, config *model.Config) (*smtp.Client, *model.Ap
 		}
 	}
 
-	if config.EmailSettings.ConnectionSecurity == model.CONN_SECURITY_STARTTLS {
+	if connectionInfo.ConnectionSecurity == model.CONN_SECURITY_STARTTLS {
 		tlsconfig := &tls.Config{
-			InsecureSkipVerify: *config.EmailSettings.SkipServerCertificateVerification,
-			ServerName:         config.EmailSettings.SMTPServer,
+			InsecureSkipVerify: connectionInfo.SkipCertVerification,
+			ServerName:         connectionInfo.SmtpServer,
 		}
 		c.StartTLS(tlsconfig)
 	}
 
-	if *config.EmailSettings.EnableSMTPAuth {
-		if err = c.Auth(&authChooser{Config: config}); err != nil {
+	if connectionInfo.Auth {
+		if err = c.Auth(&authChooser{connectionInfo: connectionInfo}); err != nil {
 			return nil, model.NewAppError("SendMail", "utils.mail.new_client.auth.app_error", nil, err.Error(), http.StatusInternalServerError)
 		}
 	}
 	return c, nil
 }
 
+func NewSMTPClient(conn net.Conn, config *model.Config) (*smtp.Client, *model.AppError) {
+	return NewSMTPClientAdvanced(
+		conn,
+		GetHostnameFromSiteURL(*config.ServiceSettings.SiteURL),
+		&SmtpConnectionInfo{
+			ConnectionSecurity:   config.EmailSettings.ConnectionSecurity,
+			SkipCertVerification: *config.EmailSettings.SkipServerCertificateVerification,
+			SmtpServer:           config.EmailSettings.SMTPServer,
+			SmtpPort:             config.EmailSettings.SMTPPort,
+			Auth:                 *config.EmailSettings.EnableSMTPAuth,
+			SmtpUsername:         config.EmailSettings.SMTPUsername,
+			SmtpPassword:         config.EmailSettings.SMTPPassword,
+		},
+	)
+}
+
 func TestConnection(config *model.Config) {
 	if !config.EmailSettings.SendEmailNotifications {
 		return
 	}
 
-	conn, err1 := connectToSMTPServer(config)
+	conn, err1 := ConnectToSMTPServer(config)
 	if err1 != nil {
 		l4g.Error(T("utils.mail.test.configured.error"), T(err1.Message), err1.DetailedError)
 		return
 	}
 	defer conn.Close()
 
-	c, err2 := newSMTPClient(conn, config)
+	c, err2 := NewSMTPClient(conn, config)
 	if err2 != nil {
 		l4g.Error(T("utils.mail.test.configured.error"), T(err2.Message), err2.DetailedError)
 		return
@@ -155,19 +193,38 @@ func TestConnection(config *model.Config) {
 
 func SendMailUsingConfig(to, subject, htmlBody string, config *model.Config, enableComplianceFeatures bool) *model.AppError {
 	fromMail := mail.Address{Name: config.EmailSettings.FeedbackName, Address: config.EmailSettings.FeedbackEmail}
-	return sendMail(to, to, fromMail, subject, htmlBody, nil, nil, config, enableComplianceFeatures)
+
+	return SendMailUsingConfigAdvanced(to, to, fromMail, subject, htmlBody, nil, nil, config, enableComplianceFeatures)
 }
 
 // allows for sending an email with attachments and differing MIME/SMTP recipients
 func SendMailUsingConfigAdvanced(mimeTo, smtpTo string, from mail.Address, subject, htmlBody string, attachments []*model.FileInfo, mimeHeaders map[string]string, config *model.Config, enableComplianceFeatures bool) *model.AppError {
-	return sendMail(mimeTo, smtpTo, from, subject, htmlBody, attachments, mimeHeaders, config, enableComplianceFeatures)
-}
-
-func sendMail(mimeTo, smtpTo string, from mail.Address, subject, htmlBody string, attachments []*model.FileInfo, mimeHeaders map[string]string, config *model.Config, enableComplianceFeatures bool) *model.AppError {
 	if !config.EmailSettings.SendEmailNotifications || len(config.EmailSettings.SMTPServer) == 0 {
 		return nil
 	}
 
+	conn, err := ConnectToSMTPServer(config)
+	if err != nil {
+		return err
+	}
+	defer conn.Close()
+
+	c, err := NewSMTPClient(conn, config)
+	if err != nil {
+		return err
+	}
+	defer c.Quit()
+	defer c.Close()
+
+	fileBackend, err := NewFileBackend(&config.FileSettings, enableComplianceFeatures)
+	if err != nil {
+		return err
+	}
+
+	return SendMail(c, mimeTo, smtpTo, from, subject, htmlBody, attachments, mimeHeaders, fileBackend)
+}
+
+func SendMail(c *smtp.Client, mimeTo, smtpTo string, from mail.Address, subject, htmlBody string, attachments []*model.FileInfo, mimeHeaders map[string]string, fileBackend FileBackend) *model.AppError {
 	l4g.Debug(T("utils.mail.send_mail.sending.debug"), mimeTo, subject)
 
 	htmlMessage := "\r\n<html><body>" + htmlBody + "</body></html>"
@@ -197,11 +254,6 @@ func sendMail(mimeTo, smtpTo string, from mail.Address, subject, htmlBody string
 	m.AddAlternative("text/html", htmlMessage)
 
 	if attachments != nil {
-		fileBackend, err := NewFileBackend(&config.FileSettings, enableComplianceFeatures)
-		if err != nil {
-			return err
-		}
-
 		for _, fileInfo := range attachments {
 			bytes, err := fileBackend.ReadFile(fileInfo.Path)
 			if err != nil {
@@ -217,19 +269,6 @@ func sendMail(mimeTo, smtpTo string, from mail.Address, subject, htmlBody string
 		}
 	}
 
-	conn, err1 := connectToSMTPServer(config)
-	if err1 != nil {
-		return err1
-	}
-	defer conn.Close()
-
-	c, err2 := newSMTPClient(conn, config)
-	if err2 != nil {
-		return err2
-	}
-	defer c.Quit()
-	defer c.Close()
-
 	if err := c.Mail(from.Address); err != nil {
 		return model.NewAppError("SendMail", "utils.mail.send_mail.from_address.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
diff --git a/utils/mail_test.go b/utils/mail_test.go
index 31a4f8996..50cf09dac 100644
--- a/utils/mail_test.go
+++ b/utils/mail_test.go
@@ -4,24 +4,27 @@
 package utils
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
+	"net/mail"
 	"net/smtp"
 
 	"github.com/mattermost/mattermost-server/model"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-func TestMailConnection(t *testing.T) {
+func TestMailConnectionFromConfig(t *testing.T) {
 	cfg, _, err := LoadConfig("config.json")
 	require.Nil(t, err)
 
-	if conn, err := connectToSMTPServer(cfg); err != nil {
+	if conn, err := ConnectToSMTPServer(cfg); err != nil {
 		t.Log(err)
 		t.Fatal("Should connect to the STMP Server")
 	} else {
-		if _, err1 := newSMTPClient(conn, cfg); err1 != nil {
+		if _, err1 := NewSMTPClient(conn, cfg); err1 != nil {
 			t.Log(err)
 			t.Fatal("Should get new smtp client")
 		}
@@ -30,7 +33,53 @@ func TestMailConnection(t *testing.T) {
 	cfg.EmailSettings.SMTPServer = "wrongServer"
 	cfg.EmailSettings.SMTPPort = "553"
 
-	if _, err := connectToSMTPServer(cfg); err == nil {
+	if _, err := ConnectToSMTPServer(cfg); err == nil {
+		t.Log(err)
+		t.Fatal("Should not to the STMP Server")
+	}
+}
+
+func TestMailConnectionAdvanced(t *testing.T) {
+	cfg, _, err := LoadConfig("config.json")
+	require.Nil(t, err)
+
+	if conn, err := ConnectToSMTPServerAdvanced(
+		&SmtpConnectionInfo{
+			ConnectionSecurity:   cfg.EmailSettings.ConnectionSecurity,
+			SkipCertVerification: *cfg.EmailSettings.SkipServerCertificateVerification,
+			SmtpServer:           cfg.EmailSettings.SMTPServer,
+			SmtpPort:             cfg.EmailSettings.SMTPPort,
+		},
+	); err != nil {
+		t.Log(err)
+		t.Fatal("Should connect to the STMP Server")
+	} else {
+		if _, err1 := NewSMTPClientAdvanced(
+			conn,
+			GetHostnameFromSiteURL(*cfg.ServiceSettings.SiteURL),
+			&SmtpConnectionInfo{
+				ConnectionSecurity:   cfg.EmailSettings.ConnectionSecurity,
+				SkipCertVerification: *cfg.EmailSettings.SkipServerCertificateVerification,
+				SmtpServer:           cfg.EmailSettings.SMTPServer,
+				SmtpPort:             cfg.EmailSettings.SMTPPort,
+				Auth:                 *cfg.EmailSettings.EnableSMTPAuth,
+				SmtpUsername:         cfg.EmailSettings.SMTPUsername,
+				SmtpPassword:         cfg.EmailSettings.SMTPPassword,
+			},
+		); err1 != nil {
+			t.Log(err)
+			t.Fatal("Should get new smtp client")
+		}
+	}
+
+	if _, err := ConnectToSMTPServerAdvanced(
+		&SmtpConnectionInfo{
+			ConnectionSecurity:   cfg.EmailSettings.ConnectionSecurity,
+			SkipCertVerification: *cfg.EmailSettings.SkipServerCertificateVerification,
+			SmtpServer:           "wrongServer",
+			SmtpPort:             "553",
+		},
+	); err == nil {
 		t.Log(err)
 		t.Fatal("Should not to the STMP Server")
 	}
@@ -79,7 +128,7 @@ func TestSendMailUsingConfig(t *testing.T) {
 	}
 }
 
-/*func TestSendMailUsingConfigAdvanced(t *testing.T) {
+func TestSendMailUsingConfigAdvanced(t *testing.T) {
 	cfg, _, err := LoadConfig("config.json")
 	require.Nil(t, err)
 	T = GetUserTranslations("en")
@@ -171,20 +220,17 @@ func TestSendMailUsingConfig(t *testing.T) {
 			}
 		}
 	}
-}*/
+}
 
 func TestAuthMethods(t *testing.T) {
-	config := model.Config{
-		EmailSettings: model.EmailSettings{
-			EnableSMTPAuth: model.NewBool(false),
-			SMTPUsername:   "test",
-			SMTPPassword:   "fakepass",
-			SMTPServer:     "fakeserver",
-			SMTPPort:       "25",
+	auth := &authChooser{
+		connectionInfo: &SmtpConnectionInfo{
+			SmtpUsername: "test",
+			SmtpPassword: "fakepass",
+			SmtpServer:   "fakeserver",
+			SmtpPort:     "25",
 		},
 	}
-
-	auth := &authChooser{Config: &config}
 	tests := []struct {
 		desc   string
 		server *smtp.ServerInfo