From 85c60f1402ecb14d647bc869d2af884342edbb48 Mon Sep 17 00:00:00 2001 From: Daniel Schalla Date: Sat, 2 Feb 2019 00:06:49 +0100 Subject: [PATCH] [MM-13839] Check for password when updating the eMail (#10207) * Check for password when updating the eMail * Require password for email change * Enhance unit testing * Restructure error handling for update email path * govet --- api4/user.go | 34 ++++++++++++++++++++++++++++------ api4/user_test.go | 42 +++++++++++++++++++++++++++++++++++++++++- app/authentication.go | 2 +- app/user.go | 2 +- model/user.go | 1 + 5 files changed, 72 insertions(+), 9 deletions(-) diff --git a/api4/user.go b/api4/user.go index 97c3daaff0..8601e65e43 100644 --- a/api4/user.go +++ b/api4/user.go @@ -685,13 +685,13 @@ func updateUser(c *Context, w http.ResponseWriter, r *http.Request) { return } - if c.App.Session.IsOAuth { - ouser, err := c.App.GetUser(user.Id) - if err != nil { - c.Err = err - return - } + ouser, err := c.App.GetUser(user.Id) + if err != nil { + c.Err = err + return + } + if c.App.Session.IsOAuth { if ouser.Email != user.Email { c.SetPermissionError(model.PERMISSION_EDIT_OTHER_USERS) c.Err.DetailedError += ", attempted email update by oauth app" @@ -699,6 +699,15 @@ func updateUser(c *Context, w http.ResponseWriter, r *http.Request) { } } + // If eMail update is attempted by the currently logged in user, check if correct password was provided + if user.Email != "" && ouser.Email != user.Email && c.App.Session.UserId == c.Params.UserId { + err = c.App.DoubleCheckPassword(ouser, user.Password) + if err != nil { + c.SetInvalidParam("password") + return + } + } + ruser, err := c.App.UpdateUserAsUser(user, c.IsSystemAdmin()) if err != nil { c.Err = err @@ -745,6 +754,19 @@ func patchUser(c *Context, w http.ResponseWriter, r *http.Request) { } } + // If eMail update is attempted by the currently logged in user, check if correct password was provided + if patch.Email != nil && ouser.Email != *patch.Email && c.App.Session.UserId == c.Params.UserId { + if patch.Password == nil { + c.SetInvalidParam("password") + return + } + + if err = c.App.DoubleCheckPassword(ouser, *patch.Password); err != nil { + c.SetInvalidParam("password") + return + } + } + ruser, err := c.App.PatchUser(c.Params.UserId, patch, c.IsSystemAdmin()) if err != nil { c.Err = err diff --git a/api4/user_test.go b/api4/user_test.go index a881ce624a..5f64b373d3 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -1037,6 +1037,15 @@ func TestUpdateUser(t *testing.T) { t.Fatal("LastPasswordUpdate should not have updated") } + ruser.Email = th.GenerateTestEmail() + _, resp = th.Client.UpdateUser(ruser) + CheckBadRequestStatus(t, resp) + + ruser.Password = user.Password + ruser, resp = th.Client.UpdateUser(ruser) + CheckNoError(t, resp) + CheckUserSanitization(t, ruser) + ruser.Id = "junk" _, resp = th.Client.UpdateUser(ruser) CheckBadRequestStatus(t, resp) @@ -1084,7 +1093,7 @@ func TestPatchUser(t *testing.T) { th.Client.Login(user.Email, user.Password) patch := &model.UserPatch{} - + patch.Password = model.NewString("testpassword") patch.Nickname = model.NewString("Joram Wilander") patch.FirstName = model.NewString("Joram") patch.LastName = model.NewString("Wilander") @@ -1115,6 +1124,9 @@ func TestPatchUser(t *testing.T) { if ruser.Username != user.Username { t.Fatal("Username should not have updated") } + if ruser.Password != ""{ + t.Fatal("Password should not be returned") + } if ruser.NotifyProps["comment"] != "somethingrandom" { t.Fatal("NotifyProps did not update properly") } @@ -1128,6 +1140,34 @@ func TestPatchUser(t *testing.T) { t.Fatal("manualTimezone did not update properly") } + err := th.App.CheckPasswordAndAllCriteria(ruser, *patch.Password, "") + assert.Error(t, err, "Password should not match") + + currentPassword := user.Password + user, err = th.App.GetUser(ruser.Id) + if err != nil { + t.Fatal("User Get shouldn't error") + } + + err = th.App.CheckPasswordAndAllCriteria(user, currentPassword, "") + if err != nil { + t.Fatal("Password should still match") + } + + patch = &model.UserPatch{} + patch.Email = model.NewString(th.GenerateTestEmail()) + + _, resp = th.Client.PatchUser(user.Id, patch) + CheckBadRequestStatus(t, resp) + + patch.Password = model.NewString(currentPassword) + ruser, resp = th.Client.PatchUser(user.Id, patch) + CheckNoError(t, resp) + + if ruser.Email != *patch.Email { + t.Fatal("Email did not update properly") + } + patch.Username = model.NewString(th.BasicUser2.Username) _, resp = th.Client.PatchUser(user.Id, patch) CheckBadRequestStatus(t, resp) diff --git a/app/authentication.go b/app/authentication.go index fbf0c9b352..c418590f10 100644 --- a/app/authentication.go +++ b/app/authentication.go @@ -57,7 +57,7 @@ func (a *App) CheckPasswordAndAllCriteria(user *model.User, password string, mfa } // This to be used for places we check the users password when they are already logged in -func (a *App) doubleCheckPassword(user *model.User, password string) *model.AppError { +func (a *App) DoubleCheckPassword(user *model.User, password string) *model.AppError { if err := checkUserLoginAttempts(user, *a.Config().ServiceSettings.MaximumLoginAttempts); err != nil { return err } diff --git a/app/user.go b/app/user.go index de38025923..7c0a8565cd 100644 --- a/app/user.go +++ b/app/user.go @@ -854,7 +854,7 @@ func (a *App) UpdatePasswordAsUser(userId, currentPassword, newPassword string) return err } - if err := a.doubleCheckPassword(user, currentPassword); err != nil { + if err := a.DoubleCheckPassword(user, currentPassword); err != nil { if err.Id == "api.user.check_user_password.invalid.app_error" { err = model.NewAppError("updatePassword", "api.user.update_password.incorrect.app_error", nil, "", http.StatusBadRequest) } diff --git a/model/user.go b/model/user.go index 2db4be5d74..a400be40f5 100644 --- a/model/user.go +++ b/model/user.go @@ -84,6 +84,7 @@ type User struct { type UserPatch struct { Username *string `json:"username"` + Password *string `json:"password,omitempty"` Nickname *string `json:"nickname"` FirstName *string `json:"first_name"` LastName *string `json:"last_name"`