Skip to content

Add email validity check #13475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Nov 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions integrations/api_admin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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": "[email protected]\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)
}
38 changes: 38 additions & 0 deletions integrations/signup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
}{
{"[email protected]\r\n", http.StatusOK, i18n.Tr("en", "form.email_invalid", nil)},
{"[email protected]\r", http.StatusOK, i18n.Tr("en", "form.email_invalid", nil)},
{"[email protected]\n", http.StatusOK, i18n.Tr("en", "form.email_invalid", nil)},
{"[email protected]", 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()),
)
}
}
}
15 changes: 15 additions & 0 deletions models/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 12 additions & 1 deletion models/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"errors"
"fmt"
_ "image/jpeg" // Needed for jpeg support
"net/mail"
"os"
"path/filepath"
"regexp"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
10 changes: 10 additions & 0 deletions models/user_mail.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package models
import (
"errors"
"fmt"
"net/mail"
"strings"

"code.gitea.io/gitea/modules/log"
Expand Down Expand Up @@ -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
}
Expand All @@ -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 {
Expand Down
15 changes: 15 additions & 0 deletions models/user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,21 @@ func TestCreateUser(t *testing.T) {
assert.NoError(t, DeleteUser(user))
}

func TestCreateUserInvalidEmail(t *testing.T) {
user := &User{
Name: "GiteaBot",
Email: "[email protected]\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
Expand Down
1 change: 1 addition & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
6 changes: 6 additions & 0 deletions routers/admin/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down
30 changes: 30 additions & 0 deletions routers/admin/users_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 := "[email protected]\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)
}
3 changes: 2 additions & 1 deletion routers/api/v1/admin/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions routers/api/v1/user/email.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package user

import (
"fmt"
"net/http"

"code.gitea.io/gitea/models"
Expand Down Expand Up @@ -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)
}
Expand Down
6 changes: 6 additions & 0 deletions routers/user/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions routers/user/setting/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down