Add email validity check (#13475)

* Improve error feedback for duplicate deploy keys

Instead of a generic HTTP 500 error page, a flash message is rendered
with the deploy key page template so inform the user that a key with the
intended title already exists.

* API returns 422 error when key with name exists

* Add email validity checking

Add email validity checking for the following routes:
[Web interface]
1. User registration
2. User creation by admin
3. Adding an email through user settings
[API]
1. POST /admin/users
2. PATCH /admin/users/:username
3. POST /user/emails

* Add further tests

* Add signup email tests

* Add email validity check for linking existing account

* Address PR comments

* Remove unneeded DB session

* Move email check to updateUser

Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>

diff --git a/integrations/api_admin_test.go b/integrations/api_admin_test.go
index 9ff9d71493fab6244ca2a03a98485007f6a46e5c..80d6b5228954261e9af110eb217f9029f6a9e865 100644 (file)
--- a/integrations/api_admin_test.go
+++ b/integrations/api_admin_test.go
@@ -144,3 +144,22 @@ func TestAPIListUsersNonAdmin(t *testing.T) {
        req := NewRequestf(t, "GET", "/api/v1/admin/users?token=%s", token)
        session.MakeRequest(t, req, http.StatusForbidden)
 }
+
+func TestAPICreateUserInvalidEmail(t *testing.T) {
+       defer prepareTestEnv(t)()
+       adminUsername := "user1"
+       session := loginUser(t, adminUsername)
+       token := getTokenForLoggedInUser(t, session)
+       urlStr := fmt.Sprintf("/api/v1/admin/users?token=%s", token)
+       req := NewRequestWithValues(t, "POST", urlStr, map[string]string{
+               "email":                "invalid_email@domain.com\r\n",
+               "full_name":            "invalid user",
+               "login_name":           "invalidUser",
+               "must_change_password": "true",
+               "password":             "password",
+               "send_notify":          "true",
+               "source_id":            "0",
+               "username":             "invalidUser",
+       })
+       session.MakeRequest(t, req, http.StatusUnprocessableEntity)
+}

diff --git a/integrations/signup_test.go b/integrations/signup_test.go
index 02262ec85372ee7d18701c104db4c711f1e66cc3..5208a42ce59189779cf639b5f0602b2b843d1496 100644 (file)
--- a/integrations/signup_test.go
+++ b/integrations/signup_test.go
@@ -5,10 +5,14 @@
 package integrations
 
 import (
+       "fmt"
        "net/http"
+       "strings"
        "testing"
 
        "code.gitea.io/gitea/modules/setting"
+       "github.com/stretchr/testify/assert"
+       "github.com/unknwon/i18n"
 )
 
 func TestSignup(t *testing.T) {
@@ -28,3 +32,37 @@ func TestSignup(t *testing.T) {
        req = NewRequest(t, "GET", "/exampleUser")
        MakeRequest(t, req, http.StatusOK)
 }
+
+func TestSignupEmail(t *testing.T) {
+       defer prepareTestEnv(t)()
+
+       setting.Service.EnableCaptcha = false
+
+       tests := []struct {
+               email      string
+               wantStatus int
+               wantMsg    string
+       }{
+               {"exampleUser@example.com\r\n", http.StatusOK, i18n.Tr("en", "form.email_invalid", nil)},
+               {"exampleUser@example.com\r", http.StatusOK, i18n.Tr("en", "form.email_invalid", nil)},
+               {"exampleUser@example.com\n", http.StatusOK, i18n.Tr("en", "form.email_invalid", nil)},
+               {"exampleUser@example.com", http.StatusFound, ""},
+       }
+
+       for i, test := range tests {
+               req := NewRequestWithValues(t, "POST", "/user/sign_up", map[string]string{
+                       "user_name": fmt.Sprintf("exampleUser%d", i),
+                       "email":     test.email,
+                       "password":  "examplePassword!1",
+                       "retype":    "examplePassword!1",
+               })
+               resp := MakeRequest(t, req, test.wantStatus)
+               if test.wantMsg != "" {
+                       htmlDoc := NewHTMLParser(t, resp.Body)
+                       assert.Equal(t,
+                               test.wantMsg,
+                               strings.TrimSpace(htmlDoc.doc.Find(".ui.message").Text()),
+                       )
+               }
+       }
+}

diff --git a/models/error.go b/models/error.go
index b2273f74c91e198c506b1ef49af1f43819c584f7..83354ff173d55e594fceaa88bd8089b6695eb7aa 100644 (file)
--- a/models/error.go
+++ b/models/error.go
@@ -193,6 +193,21 @@ func (err ErrEmailAlreadyUsed) Error() string {
        return fmt.Sprintf("e-mail already in use [email: %s]", err.Email)
 }
 
+// ErrEmailInvalid represents an error where the email address does not comply with RFC 5322
+type ErrEmailInvalid struct {
+       Email string
+}
+
+// IsErrEmailInvalid checks if an error is an ErrEmailInvalid
+func IsErrEmailInvalid(err error) bool {
+       _, ok := err.(ErrEmailInvalid)
+       return ok
+}
+
+func (err ErrEmailInvalid) Error() string {
+       return fmt.Sprintf("e-mail invalid [email: %s]", err.Email)
+}
+
 // ErrOpenIDAlreadyUsed represents a "OpenIDAlreadyUsed" kind of error.
 type ErrOpenIDAlreadyUsed struct {
        OpenID string

diff --git a/models/user.go b/models/user.go
index 42f70b4666ffe9778b3f4cc5f33fd259cd7129e6..9489ff4e8bb58bef67d19626385dc9f08f5e1f7c 100644 (file)
--- a/models/user.go
+++ b/models/user.go
@@ -14,6 +14,7 @@ import (
        "errors"
        "fmt"
        _ "image/jpeg" // Needed for jpeg support
+       "net/mail"
        "os"
        "path/filepath"
        "regexp"
@@ -808,6 +809,11 @@ func CreateUser(u *User) (err error) {
                return ErrEmailAlreadyUsed{u.Email}
        }
 
+       _, err = mail.ParseAddress(u.Email)
+       if err != nil {
+               return ErrEmailInvalid{u.Email}
+       }
+
        isExist, err = isEmailUsed(sess, u.Email)
        if err != nil {
                return err
@@ -951,7 +957,12 @@ func checkDupEmail(e Engine, u *User) error {
 }
 
 func updateUser(e Engine, u *User) error {
-       _, err := e.ID(u.ID).AllCols().Update(u)
+       u.Email = strings.ToLower(u.Email)
+       _, err := mail.ParseAddress(u.Email)
+       if err != nil {
+               return ErrEmailInvalid{u.Email}
+       }
+       _, err = e.ID(u.ID).AllCols().Update(u)
        return err
 }
 

diff --git a/models/user_mail.go b/models/user_mail.go
index 60354e23ffb22fa6eb16a76a1c58ab0c8bdb72ec..e15b5a3adfa964b128217f8a8c3c0b842f92805b 100644 (file)
--- a/models/user_mail.go
+++ b/models/user_mail.go
@@ -8,6 +8,7 @@ package models
 import (
        "errors"
        "fmt"
+       "net/mail"
        "strings"
 
        "code.gitea.io/gitea/modules/log"
@@ -143,6 +144,11 @@ func addEmailAddress(e Engine, email *EmailAddress) error {
                return ErrEmailAlreadyUsed{email.Email}
        }
 
+       _, err = mail.ParseAddress(email.Email)
+       if err != nil {
+               return ErrEmailInvalid{email.Email}
+       }
+
        _, err = e.Insert(email)
        return err
 }
@@ -167,6 +173,10 @@ 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 := x.Insert(emails); err != nil {

diff --git a/models/user_test.go b/models/user_test.go
index 7a6f5aa5122b7d5fac866f9eb0efa88959904414..216cd44c591d13e21002d9b2dec6828218a9dbc8 100644 (file)
--- a/models/user_test.go
+++ b/models/user_test.go
@@ -329,6 +329,21 @@ func TestCreateUser(t *testing.T) {
        assert.NoError(t, DeleteUser(user))
 }
 
+func TestCreateUserInvalidEmail(t *testing.T) {
+       user := &User{
+               Name:               "GiteaBot",
+               Email:              "GiteaBot@gitea.io\r\n",
+               Passwd:             ";p['////..-++']",
+               IsAdmin:            false,
+               Theme:              setting.UI.DefaultTheme,
+               MustChangePassword: false,
+       }
+
+       err := CreateUser(user)
+       assert.Error(t, err)
+       assert.True(t, IsErrEmailInvalid(err))
+}
+
 func TestCreateUser_Issue5882(t *testing.T) {
 
        // Init settings

diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
index b1447f310867f8e3d067c90a4a63bcda2438199a..f4cdcac427938691a0493837f72b2cbccdb9ac7e 100644 (file)
--- a/options/locale/locale_en-US.ini
+++ b/options/locale/locale_en-US.ini
@@ -366,6 +366,7 @@ org_name_been_taken = The organization name is already taken.
 team_name_been_taken = The team name is already taken.
 team_no_units_error = Allow access to at least one repository section.
 email_been_used = The email address is already used.
+email_invalid = The email address is invalid.
 openid_been_used = The OpenID address '%s' is already used.
 username_password_incorrect = Username or password is incorrect.
 password_complexity = Password does not pass complexity requirements:

diff --git a/routers/admin/users.go b/routers/admin/users.go
index 9fb758621b0a0976ce7ea109a93e30ce6ced9ced..4382ee3877f4e734dbad314cbb334264c0188f6a 100644 (file)
--- a/routers/admin/users.go
+++ b/routers/admin/users.go
@@ -129,6 +129,9 @@ func NewUserPost(ctx *context.Context, form auth.AdminCreateUserForm) {
                case models.IsErrEmailAlreadyUsed(err):
                        ctx.Data["Err_Email"] = true
                        ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplUserNew, &form)
+               case models.IsErrEmailInvalid(err):
+                       ctx.Data["Err_Email"] = true
+                       ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplUserNew, &form)
                case models.IsErrNameReserved(err):
                        ctx.Data["Err_UserName"] = true
                        ctx.RenderWithErr(ctx.Tr("user.form.name_reserved", err.(models.ErrNameReserved).Name), tplUserNew, &form)
@@ -277,6 +280,9 @@ func EditUserPost(ctx *context.Context, form auth.AdminEditUserForm) {
                if models.IsErrEmailAlreadyUsed(err) {
                        ctx.Data["Err_Email"] = true
                        ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplUserEdit, &form)
+               } else if models.IsErrEmailInvalid(err) {
+                       ctx.Data["Err_Email"] = true
+                       ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplUserEdit, &form)
                } else {
                        ctx.ServerError("UpdateUser", err)
                }

diff --git a/routers/admin/users_test.go b/routers/admin/users_test.go
index 2b36b45d49cdd6faa6f7fa60eb512514f64a336c..a282507f56b6ccf5e9cf61a11f68e311bbd910bc 100644 (file)
--- a/routers/admin/users_test.go
+++ b/routers/admin/users_test.go
@@ -87,3 +87,33 @@ func TestNewUserPost_MustChangePasswordFalse(t *testing.T) {
        assert.Equal(t, email, u.Email)
        assert.False(t, u.MustChangePassword)
 }
+
+func TestNewUserPost_InvalidEmail(t *testing.T) {
+
+       models.PrepareTestEnv(t)
+       ctx := test.MockContext(t, "admin/users/new")
+
+       u := models.AssertExistsAndLoadBean(t, &models.User{
+               IsAdmin: true,
+               ID:      2,
+       }).(*models.User)
+
+       ctx.User = u
+
+       username := "gitea"
+       email := "gitea@gitea.io\r\n"
+
+       form := auth.AdminCreateUserForm{
+               LoginType:          "local",
+               LoginName:          "local",
+               UserName:           username,
+               Email:              email,
+               Password:           "abc123ABC!=$",
+               SendNotify:         false,
+               MustChangePassword: false,
+       }
+
+       NewUserPost(ctx, form)
+
+       assert.NotEmpty(t, ctx.Flash.ErrorMsg)
+}

diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go
index dc095f3a1351a4683fef6f5719265fbfa54bc535..c4b52e4bd637884a02548f84932a8730e9d06909 100644 (file)
--- a/routers/api/v1/admin/user.go
+++ b/routers/api/v1/admin/user.go
@@ -101,6 +101,7 @@ func CreateUser(ctx *context.APIContext, form api.CreateUserOption) {
                        models.IsErrEmailAlreadyUsed(err) ||
                        models.IsErrNameReserved(err) ||
                        models.IsErrNameCharsNotAllowed(err) ||
+                       models.IsErrEmailInvalid(err) ||
                        models.IsErrNamePatternNotAllowed(err) {
                        ctx.Error(http.StatusUnprocessableEntity, "", err)
                } else {
@@ -208,7 +209,7 @@ func EditUser(ctx *context.APIContext, form api.EditUserOption) {
        }
 
        if err := models.UpdateUser(u); err != nil {
-               if models.IsErrEmailAlreadyUsed(err) {
+               if models.IsErrEmailAlreadyUsed(err) || models.IsErrEmailInvalid(err) {
                        ctx.Error(http.StatusUnprocessableEntity, "", err)
                } else {
                        ctx.Error(http.StatusInternalServerError, "UpdateUser", err)

diff --git a/routers/api/v1/user/email.go b/routers/api/v1/user/email.go
index 07fcde625e7354fef691888339fbeddb910acca3..d848f5e58d8d286012dc3b9628c095600b00fab6 100644 (file)
--- a/routers/api/v1/user/email.go
+++ b/routers/api/v1/user/email.go
@@ -5,6 +5,7 @@
 package user
 
 import (
+       "fmt"
        "net/http"
 
        "code.gitea.io/gitea/models"
@@ -78,6 +79,9 @@ func AddEmail(ctx *context.APIContext, form api.CreateEmailOption) {
        if err := models.AddEmailAddresses(emails); err != nil {
                if models.IsErrEmailAlreadyUsed(err) {
                        ctx.Error(http.StatusUnprocessableEntity, "", "Email address has been used: "+err.(models.ErrEmailAlreadyUsed).Email)
+               } else if models.IsErrEmailInvalid(err) {
+                       errMsg := fmt.Sprintf("Email address %s invalid", err.(models.ErrEmailInvalid).Email)
+                       ctx.Error(http.StatusUnprocessableEntity, "", errMsg)
                } else {
                        ctx.Error(http.StatusInternalServerError, "AddEmailAddresses", err)
                }

diff --git a/routers/user/auth.go b/routers/user/auth.go
index 32b031fc7417ab4ef7a5cb38e3af556a4ffeebae..ba6420967f64689a0a57838eb437cea13c0a4bad 100644 (file)
--- a/routers/user/auth.go
+++ b/routers/user/auth.go
@@ -964,6 +964,9 @@ func LinkAccountPostRegister(ctx *context.Context, cpt *captcha.Captcha, form au
                case models.IsErrEmailAlreadyUsed(err):
                        ctx.Data["Err_Email"] = true
                        ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplLinkAccount, &form)
+               case models.IsErrEmailInvalid(err):
+                       ctx.Data["Err_Email"] = true
+                       ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplSignUp, &form)
                case models.IsErrNameReserved(err):
                        ctx.Data["Err_UserName"] = true
                        ctx.RenderWithErr(ctx.Tr("user.form.name_reserved", err.(models.ErrNameReserved).Name), tplLinkAccount, &form)
@@ -1151,6 +1154,9 @@ func SignUpPost(ctx *context.Context, cpt *captcha.Captcha, form auth.RegisterFo
                case models.IsErrEmailAlreadyUsed(err):
                        ctx.Data["Err_Email"] = true
                        ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplSignUp, &form)
+               case models.IsErrEmailInvalid(err):
+                       ctx.Data["Err_Email"] = true
+                       ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplSignUp, &form)
                case models.IsErrNameReserved(err):
                        ctx.Data["Err_UserName"] = true
                        ctx.RenderWithErr(ctx.Tr("user.form.name_reserved", err.(models.ErrNameReserved).Name), tplSignUp, &form)

diff --git a/routers/user/setting/account.go b/routers/user/setting/account.go
index 99e20177bc9863665cce0677f123a498f8740ceb..9b72e2a31a23fb752556482434a1bff6b4298d4f 100644 (file)
--- a/routers/user/setting/account.go
+++ b/routers/user/setting/account.go
@@ -179,6 +179,11 @@ func EmailPost(ctx *context.Context, form auth.AddEmailForm) {
 
                        ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplSettingsAccount, &form)
                        return
+               } else if models.IsErrEmailInvalid(err) {
+                       loadAccountData(ctx)
+
+                       ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplSettingsAccount, &form)
+                       return
                }
                ctx.ServerError("AddEmailAddress", err)
                return