Merge pull request #1849 from ywk253100/170329_send_email

Fix sending email bug

src/common/utils/email/mail.go

@@ -16,87 +16,34 @@
 package email
 
 import (
-	"bytes"
 	tlspkg "crypto/tls"
+	"fmt"
 	"net"
-	"strconv"
+	"net/smtp"
+	"strings"
 	"time"
 
-	"net/smtp"
-	"text/template"
-
-	"github.com/vmware/harbor/src/common/models"
 	"github.com/vmware/harbor/src/common/utils/log"
-	"github.com/vmware/harbor/src/ui/config"
 )
 
-// Mail holds information about content of Email
-type Mail struct {
-	From    string
-	To      []string
-	Subject string
-	Message string
-}
+// Send ...
+func Send(addr, identity, username, password string,
+	timeout int, tls, insecure bool, from string,
+	to []string, subject, message string) error {
 
-var mc *models.Email
-
-// SendMail sends Email according to the configurations
-func (m Mail) SendMail() error {
-	var err error
-	mc, err = config.Email()
-	if err != nil {
-		return err
-	}
-
-	mailTemplate, err := template.ParseFiles("views/mail.tpl")
-	if err != nil {
-		return err
-	}
-	mailContent := new(bytes.Buffer)
-	err = mailTemplate.Execute(mailContent, m)
-	if err != nil {
-		return err
-	}
-	content := mailContent.Bytes()
-
-	auth := smtp.PlainAuth(mc.Identity, mc.Username, mc.Password, mc.Host)
-	if mc.SSL {
-		err = sendMailWithTLS(m, auth, content)
-	} else {
-		err = sendMail(m, auth, content)
-	}
-
-	return err
-}
-
-func sendMail(m Mail, auth smtp.Auth, content []byte) error {
-	return smtp.SendMail(mc.Host+":"+strconv.Itoa(mc.Port), auth, m.From, m.To, content)
-}
-
-func sendMailWithTLS(m Mail, auth smtp.Auth, content []byte) error {
-	conn, err := tlspkg.Dial("tcp", mc.Host+":"+strconv.Itoa(mc.Port), nil)
-	if err != nil {
-		return err
-	}
-
-	client, err := smtp.NewClient(conn, mc.Host)
+	client, err := newClient(addr, identity, username,
+		password, timeout, tls, insecure)
 	if err != nil {
 		return err
 	}
 	defer client.Close()
 
-	if ok, _ := client.Extension("AUTH"); ok {
-		if err = client.Auth(auth); err != nil {
-			return err
-		}
-	}
-
-	if err = client.Mail(m.From); err != nil {
+	if err = client.Mail(from); err != nil {
 		return err
 	}
 
-	for _, to := range m.To {
-		if err = client.Rcpt(to); err != nil {
+	for _, t := range to {
+		if err = client.Rcpt(t); err != nil {
 			return err
 		}
 	}
@@ -106,7 +53,11 @@ func sendMailWithTLS(m Mail, auth smtp.Auth, content []byte) error {
 		return err
 	}
 
-	_, err = w.Write(content)
+	template := "From: %s\r\nTo: %s\r\nSubject: %s\r\nMIME-version: 1.0;\r\nContent-Type: text/html; charset=\"UTF-8\"\r\n\n%s\r\n"
+	data := fmt.Sprintf(template, from,
+		strings.Join(to, ","), subject, message)
+
+	_, err = w.Write([]byte(data))
 	if err != nil {
 		return err
 	}
@@ -126,18 +77,29 @@ func sendMailWithTLS(m Mail, auth smtp.Auth, content []byte) error {
 // Ping doesn't verify the server's certificate and hostname when
 // needed if the parameter insecure is ture
 func Ping(addr, identity, username, password string,
-	timeout int, tls, insecure bool) (err error) {
+	timeout int, tls, insecure bool) error {
+	client, err := newClient(addr, identity, username, password,
+		timeout, tls, insecure)
+	if err != nil {
+		return err
+	}
+	defer client.Close()
+	return nil
+}
+
+// caller needs to close the client
+func newClient(addr, identity, username, password string,
+	timeout int, tls, insecure bool) (*smtp.Client, error) {
 	log.Debugf("establishing TCP connection with %s ...", addr)
 	conn, err := net.DialTimeout("tcp", addr,
 		time.Duration(timeout)*time.Second)
 	if err != nil {
-		return
+		return nil, err
 	}
-	defer conn.Close()
 
 	host, _, err := net.SplitHostPort(addr)
 	if err != nil {
-		return
+		return nil, err
 	}
 
 	if tls {
@@ -147,9 +109,8 @@ func Ping(addr, identity, username, password string,
 			InsecureSkipVerify: insecure,
 		})
 		if err = tlsConn.Handshake(); err != nil {
-			return
+			return nil, err
 		}
-		defer tlsConn.Close()
 
 		conn = tlsConn
 	}
@@ -157,9 +118,8 @@ func Ping(addr, identity, username, password string,
 	log.Debugf("creating SMTP client for %s ...", host)
 	client, err := smtp.NewClient(conn, host)
 	if err != nil {
-		return
+		return nil, err
 	}
-	defer client.Close()
 
 	//try to swith to SSL/TLS
 	if !tls {
@@ -169,7 +129,7 @@ func Ping(addr, identity, username, password string,
 				ServerName:         host,
 				InsecureSkipVerify: insecure,
 			}); err != nil {
-				return
+				return nil, err
 			}
 		} else {
 			log.Debugf("the email server %s does not support STARTTLS", addr)
@@ -181,14 +141,14 @@ func Ping(addr, identity, username, password string,
 		// only support plain auth
 		if err = client.Auth(smtp.PlainAuth(identity,
 			username, password, host)); err != nil {
-			return
+			return nil, err
 		}
 	} else {
 		log.Debugf("the email server %s does not support AUTH, skip",
 			addr)
 	}
 
-	log.Debug("ping email server successfully")
+	log.Debug("create smtp client successfully")
 
-	return
+	return client, nil
 }

src/common/utils/email/mail_test.go

@@ -18,20 +18,81 @@ package email
 import (
 	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
+func TestSend(t *testing.T) {
+	addr := "smtp.gmail.com:465"
+	identity := ""
+	username := "harbortestonly@gmail.com"
+	password := "harborharbor"
+	timeout := 60
+	tls := true
+	insecure := false
+	from := "from"
+	to := []string{username}
+	subject := "subject"
+	message := "message"
+
+	// tls connection
+	tls = true
+	err := Send(addr, identity, username, password,
+		timeout, tls, insecure, from, to,
+		subject, message)
+	assert.Nil(t, err)
+
+	/*not work on travis
+	// non-tls connection
+	addr = "smtp.gmail.com:25"
+	tls = false
+	err = Send(addr, identity, username, password,
+		timeout, tls, insecure, from, to,
+		subject, message)
+	assert.Nil(t, err)
+	*/
+
+	//invalid username/password
+	username = "invalid_username"
+	err = Send(addr, identity, username, password,
+		timeout, tls, insecure, from, to,
+		subject, message)
+	if err == nil {
+		t.Errorf("there should be an auth error")
+	} else {
+		if !strings.Contains(err.Error(), "535") {
+			t.Errorf("unexpected error: %v", err)
+		}
+	}
+}
+
 func TestPing(t *testing.T) {
 	addr := "smtp.gmail.com:465"
 	identity := ""
-	username := "wrong_username"
-	password := "wrong_password"
-	timeout := 60
+	username := "harbortestonly@gmail.com"
+	password := "harborharbor"
+	timeout := 0
 	tls := true
 	insecure := false
 
-	// test secure connection
+	// tls connection
 	err := Ping(addr, identity, username, password,
 		timeout, tls, insecure)
+	assert.Nil(t, err)
+
+	/*not work on travis
+	// non-tls connection
+	addr = "smtp.gmail.com:25"
+	tls = false
+	err = Ping(addr, identity, username, password,
+		timeout, tls, insecure)
+	assert.Nil(t, err)
+	*/
+
+	//invalid username/password
+	username = "invalid_username"
+	err = Ping(addr, identity, username, password,
+		timeout, tls, insecure)
 	if err == nil {
 		t.Errorf("there should be an auth error")
 	} else {

src/ui/controllers/base.go

@@ -3,19 +3,21 @@ package controllers
 import (
 	"bytes"
 	"html/template"
+	"net"
 	"net/http"
 	"os"
 	"regexp"
+	"strconv"
 
 	"github.com/astaxie/beego"
 	"github.com/beego/i18n"
-	"github.com/vmware/harbor/src/common"
 	"github.com/vmware/harbor/src/common/dao"
 	"github.com/vmware/harbor/src/common/models"
 	"github.com/vmware/harbor/src/common/utils"
-	"github.com/vmware/harbor/src/common/utils/email"
+	email_util "github.com/vmware/harbor/src/common/utils/email"
 	"github.com/vmware/harbor/src/common/utils/log"
 	"github.com/vmware/harbor/src/ui/auth"
+	"github.com/vmware/harbor/src/ui/config"
 )
 
 // CommonController handles request from UI that doesn't expect a page, such as /SwitchLanguage /logout ...
@@ -87,24 +89,36 @@ func (cc *CommonController) UserExists() {
 // SendEmail verifies the Email address and contact SMTP server to send reset password Email.
 func (cc *CommonController) SendEmail() {
 
-	emailStr := cc.GetString("email")
+	email := cc.GetString("email")
 
-	pass, _ := regexp.MatchString(`^(([^<>()[\]\\.,;:\s@\"]+(\.[^<>()[\]\\.,;:\s@\"]+)*)|(\".+\"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$`, emailStr)
+	valid, err := regexp.MatchString(`^(([^<>()[\]\\.,;:\s@\"]+(\.[^<>()[\]\\.,;:\s@\"]+)*)|(\".+\"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$`, email)
+	if err != nil {
+		log.Errorf("failed to match regexp: %v", err)
+		cc.CustomAbort(http.StatusInternalServerError, "Internal error.")
+	}
 
-	if !pass {
-		cc.CustomAbort(http.StatusBadRequest, "email_content_illegal")
-	} else {
+	if !valid {
+		cc.CustomAbort(http.StatusBadRequest, "invalid email")
+	}
 
-		queryUser := models.User{Email: emailStr}
+	queryUser := models.User{Email: email}
 	exist, err := dao.UserExists(queryUser, "email")
 	if err != nil {
 		log.Errorf("Error occurred in UserExists: %v", err)
 		cc.CustomAbort(http.StatusInternalServerError, "Internal error.")
 	}
 	if !exist {
+		log.Debugf("email %s not found", email)
 		cc.CustomAbort(http.StatusNotFound, "email_does_not_exist")
 	}
 
+	uuid := utils.GenerateRandomString()
+	user := models.User{ResetUUID: uuid, Email: email}
+	if err = dao.UpdateUserResetUUID(user); err != nil {
+		log.Errorf("failed to update user reset UUID: %v", err)
+		cc.CustomAbort(http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError))
+	}
+
 	messageTemplate, err := template.ParseFiles("views/reset-password-mail.tpl")
 	if err != nil {
 		log.Errorf("Parse email template file failed: %v", err)
@@ -113,11 +127,12 @@ func (cc *CommonController) SendEmail() {
 
 	message := new(bytes.Buffer)
 
-		harborURL := common.ExtEndpoint
-		if harborURL == "" {
-			harborURL = "localhost"
+	harborURL, err := config.ExtEndpoint()
+	if err != nil {
+		log.Errorf("failed to get domain name: %v", err)
+		cc.CustomAbort(http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError))
 	}
-		uuid := utils.GenerateRandomString()
+
 	err = messageTemplate.Execute(message, messageDetail{
 		Hint: cc.Tr("reset_email_hint"),
 		URL:  harborURL,
@@ -129,30 +144,26 @@ func (cc *CommonController) SendEmail() {
 		cc.CustomAbort(http.StatusInternalServerError, "internal_error")
 	}
 
-		config, err := beego.AppConfig.GetSection("mail")
+	settings, err := config.Email()
 	if err != nil {
-			log.Errorf("Can not load app.conf: %v", err)
+		log.Errorf("failed to get email configurations: %v", err)
 		cc.CustomAbort(http.StatusInternalServerError, "internal_error")
 	}
 
-		mail := email.Mail{
-			From:    config["from"],
-			To:      []string{emailStr},
-			Subject: cc.Tr("reset_email_subject"),
-			Message: message.String()}
-
-		err = mail.SendMail()
-
+	addr := net.JoinHostPort(settings.Host, strconv.Itoa(settings.Port))
+	err = email_util.Send(addr,
+		settings.Identity,
+		settings.Username,
+		settings.Password,
+		60, settings.SSL,
+		false, settings.From,
+		[]string{email},
+		cc.Tr("reset_email_subject"),
+		message.String())
 	if err != nil {
 		log.Errorf("Send email failed: %v", err)
 		cc.CustomAbort(http.StatusInternalServerError, "send_email_failed")
 	}
-
-		user := models.User{ResetUUID: uuid, Email: emailStr}
-		dao.UpdateUserResetUUID(user)
-
-	}
-
 }
 
 // ResetPassword handles request from the reset page and reset password