skip email validation on empty string (#13627)

- move validation into its own function
- use a session for UpdateUserSetting

2 files changed, 35 insertions, 17 deletions

diff --git a/models/user.go b/models/user.go
index 9489ff4e8b..2a76995578 100644
--- a/models/user.go
+++ b/models/user.go
@@ -14,7 +14,6 @@ import (
 	"errors"
 	"fmt"
 	_ "image/jpeg" // Needed for jpeg support
-	"net/mail"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -809,9 +808,8 @@ func CreateUser(u *User) (err error) {
 		return ErrEmailAlreadyUsed{u.Email}
 	}
 
-	_, err = mail.ParseAddress(u.Email)
-	if err != nil {
-		return ErrEmailInvalid{u.Email}
+	if err = ValidateEmail(u.Email); err != nil {
+		return err
 	}
 
 	isExist, err = isEmailUsed(sess, u.Email)
@@ -956,11 +954,10 @@ func checkDupEmail(e Engine, u *User) error {
 	return nil
 }
 
-func updateUser(e Engine, u *User) error {
+func updateUser(e Engine, u *User) (err error) {
 	u.Email = strings.ToLower(u.Email)
-	_, err := mail.ParseAddress(u.Email)
-	if err != nil {
-		return ErrEmailInvalid{u.Email}
+	if err = ValidateEmail(u.Email); err != nil {
+		return err
 	}
 	_, err = e.ID(u.ID).AllCols().Update(u)
 	return err
@@ -982,13 +979,21 @@ func updateUserCols(e Engine, u *User, cols ...string) error {
 }
 
 // UpdateUserSetting updates user's settings.
-func UpdateUserSetting(u *User) error {
+func UpdateUserSetting(u *User) (err error) {
+	sess := x.NewSession()
+	defer sess.Close()
+	if err = sess.Begin(); err != nil {
+		return err
+	}
 	if !u.IsOrganization() {
-		if err := checkDupEmail(x, u); err != nil {
+		if err = checkDupEmail(sess, u); err != nil {
 			return err
 		}
 	}
-	return updateUser(x, u)
+	if err = updateUser(sess, u); err != nil {
+		return err
+	}
+	return sess.Commit()
 }
 
 // deleteBeans deletes all given beans, beans should contain delete conditions.
diff --git a/models/user_mail.go b/models/user_mail.go
index e15b5a3adf..1e56a5cd83 100644
--- a/models/user_mail.go
+++ b/models/user_mail.go
@@ -33,6 +33,21 @@ type EmailAddress struct {
 	IsPrimary   bool `xorm:"-"`
 }
 
+// ValidateEmail check if email is a allowed address
+func ValidateEmail(email string) error {
+	if len(email) == 0 {
+		return nil
+	}
+
+	if _, err := mail.ParseAddress(email); err != nil {
+		return ErrEmailInvalid{email}
+	}
+
+	// TODO: add an email allow/block list
+
+	return nil
+}
+
 // GetEmailAddresses returns all email addresses belongs to given user.
 func GetEmailAddresses(uid int64) ([]*EmailAddress, error) {
 	emails := make([]*EmailAddress, 0, 5)
@@ -144,9 +159,8 @@ func addEmailAddress(e Engine, email *EmailAddress) error {
 		return ErrEmailAlreadyUsed{email.Email}
 	}
 
-	_, err = mail.ParseAddress(email.Email)
-	if err != nil {
-		return ErrEmailInvalid{email.Email}
+	if err = ValidateEmail(email.Email); err != nil {
+		return err
 	}
 
 	_, err = e.Insert(email)
@@ -173,9 +187,8 @@ func AddEmailAddresses(emails []*EmailAddress) error {
 		} else if used {
 			return ErrEmailAlreadyUsed{emails[i].Email}
 		}
-		_, err = mail.ParseAddress(emails[i].Email)
-		if err != nil {
-			return ErrEmailInvalid{emails[i].Email}
+		if err = ValidateEmail(emails[i].Email); err != nil {
+			return err
 		}
 	}