diff --git a/api4/apitestlib.go b/api4/apitestlib.go index 3d8e425c7f..67a6eddf1e 100644 --- a/api4/apitestlib.go +++ b/api4/apitestlib.go @@ -294,7 +294,7 @@ func (me *TestHelper) CreateUserWithClient(client *model.Client4) *model.User { } ruser.Password = "Password1" - store.Must(me.App.Srv.Store.User().VerifyEmail(ruser.Id)) + store.Must(me.App.Srv.Store.User().VerifyEmail(ruser.Id, ruser.Email)) utils.EnableDebugLogForTest() return ruser } diff --git a/api4/user.go b/api4/user.go index 04a630ac5c..10c7dec470 100644 --- a/api4/user.go +++ b/api4/user.go @@ -1355,7 +1355,7 @@ func sendVerificationEmail(c *Context, w http.ResponseWriter, r *http.Request) { return } - if err = c.App.SendEmailVerification(user); err != nil { + if err = c.App.SendEmailVerification(user, user.Email); err != nil { // Don't want to leak whether the email is valid or not mlog.Error(err.Error()) ReturnStatusOK(w) diff --git a/api4/user_test.go b/api4/user_test.go index dfc2417cea..cb5b307321 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -1220,7 +1220,7 @@ func TestUpdateUserAuth(t *testing.T) { user := th.CreateUser() th.LinkUserToTeam(user, team) - store.Must(th.App.Srv.Store.User().VerifyEmail(user.Id)) + store.Must(th.App.Srv.Store.User().VerifyEmail(user.Id, user.Email)) userAuth := &model.UserAuth{} userAuth.AuthData = user.AuthData @@ -1260,7 +1260,7 @@ func TestUpdateUserAuth(t *testing.T) { // Regular user can not use endpoint user2 := th.CreateUser() th.LinkUserToTeam(user2, team) - store.Must(th.App.Srv.Store.User().VerifyEmail(user2.Id)) + store.Must(th.App.Srv.Store.User().VerifyEmail(user2.Id, user2.Email)) th.SystemAdminClient.Login(user2.Email, "passwd1") @@ -2220,11 +2220,12 @@ func TestVerifyUserEmail(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() - user := model.User{Email: th.GenerateTestEmail(), Nickname: "Darth Vader", Password: "hello1", Username: GenerateTestUsername(), Roles: model.SYSTEM_ADMIN_ROLE_ID + " " + model.SYSTEM_USER_ROLE_ID} + email := th.GenerateTestEmail() + user := model.User{Email: email, Nickname: "Darth Vader", Password: "hello1", Username: GenerateTestUsername(), Roles: model.SYSTEM_ADMIN_ROLE_ID + " " + model.SYSTEM_USER_ROLE_ID} ruser, _ := th.Client.CreateUser(&user) - token, err := th.App.CreateVerifyEmailToken(ruser.Id) + token, err := th.App.CreateVerifyEmailToken(ruser.Id, email) if err != nil { t.Fatal("Unable to create email verify token") } diff --git a/api4/websocket_test.go b/api4/websocket_test.go index 8ca1941336..3874e7153b 100644 --- a/api4/websocket_test.go +++ b/api4/websocket_test.go @@ -304,12 +304,12 @@ func TestWebSocketStatuses(t *testing.T) { user := model.User{Email: strings.ToLower(model.NewId()) + "success+test@simulator.amazonses.com", Nickname: "Corey Hulen", Password: "passwd1"} ruser := Client.Must(Client.CreateUser(&user)).(*model.User) th.LinkUserToTeam(ruser, rteam) - store.Must(th.App.Srv.Store.User().VerifyEmail(ruser.Id)) + store.Must(th.App.Srv.Store.User().VerifyEmail(ruser.Id, ruser.Email)) user2 := model.User{Email: strings.ToLower(model.NewId()) + "success+test@simulator.amazonses.com", Nickname: "Corey Hulen", Password: "passwd1"} ruser2 := Client.Must(Client.CreateUser(&user2)).(*model.User) th.LinkUserToTeam(ruser2, rteam) - store.Must(th.App.Srv.Store.User().VerifyEmail(ruser2.Id)) + store.Must(th.App.Srv.Store.User().VerifyEmail(ruser2.Id, ruser2.Email)) Client.Login(user.Email, user.Password) diff --git a/app/auto_users.go b/app/auto_users.go index b61ab053f6..e1776e3476 100644 --- a/app/auto_users.go +++ b/app/auto_users.go @@ -48,7 +48,7 @@ func (a *App) CreateBasicUser(client *model.Client4) *model.AppError { if resp.Error != nil { return resp.Error } - store.Must(a.Srv.Store.User().VerifyEmail(ruser.Id)) + store.Must(a.Srv.Store.User().VerifyEmail(ruser.Id, ruser.Email)) store.Must(a.Srv.Store.Team().SaveMember(&model.TeamMember{TeamId: basicteam.Id, UserId: ruser.Id}, *a.Config().TeamSettings.MaxUsersPerTeam)) } return nil @@ -83,7 +83,7 @@ func (cfg *AutoUserCreator) createRandomUser() (*model.User, bool) { } // We need to cheat to verify the user's email - store.Must(cfg.app.Srv.Store.User().VerifyEmail(ruser.Id)) + store.Must(cfg.app.Srv.Store.User().VerifyEmail(ruser.Id, ruser.Email)) return ruser, true } diff --git a/app/email.go b/app/email.go index c1333aec24..9d886641e9 100644 --- a/app/email.go +++ b/app/email.go @@ -180,7 +180,7 @@ func (a *App) SendWelcomeEmail(userId string, email string, verified bool, local } if !verified { - token, err := a.CreateVerifyEmailToken(userId) + token, err := a.CreateVerifyEmailToken(userId, email) if err != nil { return err } diff --git a/app/import_functions.go b/app/import_functions.go index 6ebefccc85..9fbfa42aeb 100644 --- a/app/import_functions.go +++ b/app/import_functions.go @@ -477,7 +477,7 @@ func (a *App) ImportUser(data *UserImportData, dryRun bool) *model.AppError { } if emailVerified { if hasUserEmailVerifiedChanged { - if err := a.VerifyUserEmail(user.Id); err != nil { + if err := a.VerifyUserEmail(user.Id, user.Email); err != nil { return err } } diff --git a/app/slackimport.go b/app/slackimport.go index 546f59ce38..c810fdebb3 100644 --- a/app/slackimport.go +++ b/app/slackimport.go @@ -746,7 +746,7 @@ func (a *App) OldImportUser(team *model.Team, user *model.User) *model.User { } ruser := result.Data.(*model.User) - if cresult := <-a.Srv.Store.User().VerifyEmail(ruser.Id); cresult.Err != nil { + if cresult := <-a.Srv.Store.User().VerifyEmail(ruser.Id, ruser.Email); cresult.Err != nil { mlog.Error(fmt.Sprintf("Failed to set email verified err=%v", cresult.Err)) } diff --git a/app/user.go b/app/user.go index 7c0a8565cd..96493cb4d0 100644 --- a/app/user.go +++ b/app/user.go @@ -6,6 +6,7 @@ package app import ( "bytes" b64 "encoding/base64" + "encoding/json" "fmt" "hash/fnv" "image" @@ -243,7 +244,7 @@ func (a *App) createUser(user *model.User) (*model.User, *model.AppError) { ruser := result.Data.(*model.User) if user.EmailVerified { - if err := a.VerifyUserEmail(ruser.Id); err != nil { + if err := a.VerifyUserEmail(ruser.Id, user.Email); err != nil { mlog.Error(fmt.Sprintf("Failed to set email verified err=%v", err)) } } @@ -996,34 +997,49 @@ func (a *App) sendUpdatedUserEvent(user model.User) { } func (a *App) UpdateUser(user *model.User, sendNotifications bool) (*model.User, *model.AppError) { + result := <-a.Srv.Store.User().Get(user.Id) + if result.Err != nil { + return nil, result.Err + } + prev := result.Data.(*model.User) + if !CheckUserDomain(user, *a.Config().TeamSettings.RestrictCreationToDomains) { - result := <-a.Srv.Store.User().Get(user.Id) - if result.Err != nil { - return nil, result.Err - } - prev := result.Data.(*model.User) if !prev.IsLDAPUser() && !prev.IsSAMLUser() && user.Email != prev.Email { return nil, model.NewAppError("UpdateUser", "api.user.create_user.accepted_domain.app_error", nil, "", http.StatusBadRequest) } } - result := <-a.Srv.Store.User().Update(user, false) + // Don't set new eMail on user account if email verification is required, this will be done as a post-verification action + // to avoid users being able to set non-controlled eMails as their account email + newEmail := "" + if *a.Config().EmailSettings.RequireEmailVerification && prev.Email != user.Email { + newEmail = user.Email + + _, err := a.GetUserByEmail(newEmail) + if err == nil { + return nil, model.NewAppError("UpdateUser", "store.sql_user.update.email_taken.app_error", nil, "user_id="+user.Id, http.StatusBadRequest) + } + + user.Email = prev.Email + } + + result = <-a.Srv.Store.User().Update(user, false) if result.Err != nil { return nil, result.Err } rusers := result.Data.([2]*model.User) if sendNotifications { - if rusers[0].Email != rusers[1].Email { - a.Srv.Go(func() { - if err := a.SendEmailChangeEmail(rusers[1].Email, rusers[0].Email, rusers[0].Locale, a.GetSiteURL()); err != nil { - mlog.Error(err.Error()) - } - }) - + if rusers[0].Email != rusers[1].Email || newEmail != "" { if *a.Config().EmailSettings.RequireEmailVerification { a.Srv.Go(func() { - if err := a.SendEmailVerification(rusers[0]); err != nil { + if err := a.SendEmailVerification(rusers[0], newEmail); err != nil { + mlog.Error(err.Error()) + } + }) + } else { + a.Srv.Go(func() { + if err := a.SendEmailChangeEmail(rusers[1].Email, rusers[0].Email, rusers[0].Locale, a.GetSiteURL()); err != nil { mlog.Error(err.Error()) } }) @@ -1366,16 +1382,16 @@ func (a *App) PermanentDeleteAllUsers() *model.AppError { return nil } -func (a *App) SendEmailVerification(user *model.User) *model.AppError { - token, err := a.CreateVerifyEmailToken(user.Id) +func (a *App) SendEmailVerification(user *model.User, newEmail string) *model.AppError { + token, err := a.CreateVerifyEmailToken(user.Id, newEmail) if err != nil { return err } if _, err := a.GetStatus(user.Id); err != nil { - return a.SendVerifyEmail(user.Email, user.Locale, a.GetSiteURL(), token.Token) + return a.SendVerifyEmail(newEmail, user.Locale, a.GetSiteURL(), token.Token) } - return a.SendEmailChangeVerifyEmail(user.Email, user.Locale, a.GetSiteURL(), token.Token) + return a.SendEmailChangeVerifyEmail(newEmail, user.Locale, a.GetSiteURL(), token.Token) } func (a *App) VerifyEmailFromToken(userSuppliedTokenString string) *model.AppError { @@ -1384,11 +1400,36 @@ func (a *App) VerifyEmailFromToken(userSuppliedTokenString string) *model.AppErr return err } if model.GetMillis()-token.CreateAt >= PASSWORD_RECOVER_EXPIRY_TIME { - return model.NewAppError("resetPassword", "api.user.reset_password.link_expired.app_error", nil, "", http.StatusBadRequest) + return model.NewAppError("VerifyEmailFromToken", "api.user.verify_email.link_expired.app_error", nil, "", http.StatusBadRequest) } - if err := a.VerifyUserEmail(token.Extra); err != nil { + + tokenData := struct { + UserId string + Email string + }{} + + err2 := json.Unmarshal([]byte(token.Extra), &tokenData) + if err2 != nil { + return model.NewAppError("VerifyEmailFromToken", "api.user.verify_email.token_parse.error", nil, "", http.StatusInternalServerError) + } + + user, err := a.GetUser(tokenData.UserId) + if err != nil { return err } + + if err := a.VerifyUserEmail(tokenData.UserId, tokenData.Email); err != nil { + return err + } + + if user.Email != tokenData.Email { + a.Srv.Go(func() { + if err := a.SendEmailChangeEmail(user.Email, tokenData.Email, user.Locale, a.GetSiteURL()); err != nil { + mlog.Error(err.Error()) + } + }) + } + if err := a.DeleteToken(token); err != nil { mlog.Error(err.Error()) } @@ -1396,8 +1437,21 @@ func (a *App) VerifyEmailFromToken(userSuppliedTokenString string) *model.AppErr return nil } -func (a *App) CreateVerifyEmailToken(userId string) (*model.Token, *model.AppError) { - token := model.NewToken(TOKEN_TYPE_VERIFY_EMAIL, userId) +func (a *App) CreateVerifyEmailToken(userId string, newEmail string) (*model.Token, *model.AppError) { + tokenExtra := struct { + UserId string + Email string + }{ + userId, + newEmail, + } + jsonData, err := json.Marshal(tokenExtra) + + if err != nil { + return nil, model.NewAppError("CreateVerifyEmailToken", "api.user.create_email_token.error", nil, "", http.StatusInternalServerError) + } + + token := model.NewToken(TOKEN_TYPE_VERIFY_EMAIL, string(jsonData)) if result := <-a.Srv.Store.Token().Save(token); result.Err != nil { return nil, result.Err @@ -1429,8 +1483,22 @@ func (a *App) GetTotalUsersStats() (*model.UsersStats, *model.AppError) { return stats, nil } -func (a *App) VerifyUserEmail(userId string) *model.AppError { - return (<-a.Srv.Store.User().VerifyEmail(userId)).Err +func (a *App) VerifyUserEmail(userId, email string) *model.AppError { + err := (<-a.Srv.Store.User().VerifyEmail(userId, email)).Err + + if err != nil { + return err + } + + user, err := a.GetUser(userId) + + if err != nil { + return err + } + + a.sendUpdatedUserEvent(*user) + + return nil } func (a *App) SearchUsers(props *model.UserSearch, options *model.UserSearchOptions) ([]*model.User, *model.AppError) { diff --git a/app/user_test.go b/app/user_test.go index 33a7409132..5527bec12c 100644 --- a/app/user_test.go +++ b/app/user_test.go @@ -286,6 +286,66 @@ func TestUpdateOAuthUserAttrs(t *testing.T) { }) } +func TestUpdateUserEmail(t *testing.T) { + th := Setup(t) + defer th.TearDown() + + user := th.CreateUser() + + t.Run("RequireVerification", func(t *testing.T){ + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.EmailSettings.RequireEmailVerification = true + }) + + currentEmail := user.Email + newEmail := th.MakeEmail() + + user.Email = newEmail + user2, err := th.App.UpdateUser(user, false) + assert.Nil(t, err) + assert.Equal(t, currentEmail, user2.Email) + assert.True(t, user2.EmailVerified) + + token, err := th.App.CreateVerifyEmailToken(user2.Id, newEmail) + assert.Nil(t, err) + + err = th.App.VerifyEmailFromToken(token.Token) + assert.Nil(t, err) + + user2, err = th.App.GetUser(user2.Id) + assert.Nil(t, err) + assert.Equal(t, newEmail, user2.Email) + assert.True(t, user2.EmailVerified) + }) + + t.Run("RequireVerificationAlreadyUsedEmail", func(t *testing.T){ + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.EmailSettings.RequireEmailVerification = true + }) + + user2 := th.CreateUser() + newEmail := user2.Email + + user.Email = newEmail + user3, err := th.App.UpdateUser(user, false) + assert.NotNil(t, err) + assert.Nil(t, user3) + }) + + t.Run("NoVerification", func(t *testing.T){ + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.EmailSettings.RequireEmailVerification = false + }) + + newEmail := th.MakeEmail() + + user.Email = newEmail + user2, err := th.App.UpdateUser(user, false) + assert.Nil(t, err) + assert.Equal(t, newEmail, user2.Email) + }) +} + func getUserFromDB(a *App, id string, t *testing.T) *model.User { user, err := a.GetUser(id) if err != nil { diff --git a/cmd/mattermost/commands/user.go b/cmd/mattermost/commands/user.go index 71692c73b2..5e3106e1fc 100644 --- a/cmd/mattermost/commands/user.go +++ b/cmd/mattermost/commands/user.go @@ -693,7 +693,7 @@ func verifyUserCmdF(command *cobra.Command, args []string) error { CommandPrintErrorln("Unable to find user '" + args[i] + "'") continue } - if cresult := <-a.Srv.Store.User().VerifyEmail(user.Id); cresult.Err != nil { + if cresult := <-a.Srv.Store.User().VerifyEmail(user.Id, user.Email); cresult.Err != nil { CommandPrintErrorln("Unable to verify '" + args[i] + "' email. Error: " + cresult.Err.Error()) } } diff --git a/i18n/en.json b/i18n/en.json index 60048b8c5f..415de58ab0 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2362,6 +2362,18 @@ "id": "api.user.send_email_change_verify_email_and_forget.error", "translation": "Failed to send email change verification email successfully" }, + { + "id": "api.user.verify_email.link_expired.app_error", + "translation": "The email verification link has expired." + }, + { + "id": "api.user.verify_email.token_parse.error", + "translation": "Failed to parse token data from email verification" + }, + { + "id": "api.user.create_email_token.error", + "translation": "Failed to create token data for email verification" + }, { "id": "api.user.send_mfa_change_email.error", "translation": "Unable to send email notification for MFA change." diff --git a/manualtesting/manual_testing.go b/manualtesting/manual_testing.go index df72fbb57a..9878bb52e7 100644 --- a/manualtesting/manual_testing.go +++ b/manualtesting/manual_testing.go @@ -102,7 +102,7 @@ func manualTest(c *web.Context, w http.ResponseWriter, r *http.Request) { return } - <-c.App.Srv.Store.User().VerifyEmail(user.Id) + <-c.App.Srv.Store.User().VerifyEmail(user.Id, user.Email) <-c.App.Srv.Store.Team().SaveMember(&model.TeamMember{TeamId: teamID, UserId: user.Id}, *c.App.Config().TeamSettings.MaxUsersPerTeam) userID = user.Id diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index e602281728..6c622f02d6 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -1025,9 +1025,10 @@ func (us SqlUserStore) GetForLogin(loginId string, allowSignInWithUsername, allo }) } -func (us SqlUserStore) VerifyEmail(userId string) store.StoreChannel { +func (us SqlUserStore) VerifyEmail(userId, email string) store.StoreChannel { return store.Do(func(result *store.StoreResult) { - if _, err := us.GetMaster().Exec("UPDATE Users SET EmailVerified = true WHERE Id = :UserId", map[string]interface{}{"UserId": userId}); err != nil { + curTime := model.GetMillis() + if _, err := us.GetMaster().Exec("UPDATE Users SET Email = :email, EmailVerified = true, UpdateAt = :Time WHERE Id = :UserId", map[string]interface{}{"email": email, "Time": curTime, "UserId": userId}); err != nil { result.Err = model.NewAppError("SqlUserStore.VerifyEmail", "store.sql_user.verify_email.app_error", nil, "userId="+userId+", "+err.Error(), http.StatusInternalServerError) } diff --git a/store/store.go b/store/store.go index 88098b98be..611e40fd7a 100644 --- a/store/store.go +++ b/store/store.go @@ -259,7 +259,7 @@ type UserStore interface { GetAllUsingAuthService(authService string) StoreChannel GetByUsername(username string) StoreChannel GetForLogin(loginId string, allowSignInWithUsername, allowSignInWithEmail bool) StoreChannel - VerifyEmail(userId string) StoreChannel + VerifyEmail(userId, email string) StoreChannel GetEtagForAllProfiles() StoreChannel GetEtagForProfiles(teamId string) StoreChannel UpdateFailedPasswordAttempts(userId string, attempts int) StoreChannel diff --git a/store/storetest/mocks/UserStore.go b/store/storetest/mocks/UserStore.go index d3081c8910..e3c40f72eb 100644 --- a/store/storetest/mocks/UserStore.go +++ b/store/storetest/mocks/UserStore.go @@ -833,13 +833,13 @@ func (_m *UserStore) UpdateUpdateAt(userId string) store.StoreChannel { return r0 } -// VerifyEmail provides a mock function with given fields: userId -func (_m *UserStore) VerifyEmail(userId string) store.StoreChannel { - ret := _m.Called(userId) +// VerifyEmail provides a mock function with given fields: userId, email +func (_m *UserStore) VerifyEmail(userId string, email string) store.StoreChannel { + ret := _m.Called(userId, email) var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string) store.StoreChannel); ok { - r0 = rf(userId) + if rf, ok := ret.Get(0).(func(string, string) store.StoreChannel); ok { + r0 = rf(userId, email) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(store.StoreChannel)