Some improvments to password handling (#3549)

api/authentication.go

@@ -13,11 +13,24 @@ import (
 )
 
 func checkPasswordAndAllCriteria(user *model.User, password string, mfaToken string) *model.AppError {
+	if err := checkUserAdditionalAuthenticationCriteria(user, mfaToken); err != nil {
+		return err
+	}
+
 	if err := checkUserPassword(user, password); err != nil {
 		return err
 	}
 
-	if err := checkUserAdditionalAuthenticationCriteria(user, mfaToken); err != nil {
+	return nil
+}
+
+// This to be used for places we check the users password when they are already logged in
+func doubleCheckPassword(user *model.User, password string) *model.AppError {
+	if err := checkUserLoginAttempts(user); err != nil {
+		return err
+	}
+
+	if err := checkUserPassword(user, password); err != nil {
 		return err
 	}
 

api/user.go

@@ -1388,8 +1388,12 @@ func updatePassword(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if !model.ComparePassword(user.Password, currentPassword) {
-		c.Err = model.NewLocAppError("updatePassword", "api.user.update_password.incorrect.app_error", nil, "")
+	if err := doubleCheckPassword(user, currentPassword); err != nil {
+		if err.Id == "api.user.check_user_password.invalid.app_error" {
+			c.Err = model.NewLocAppError("updatePassword", "api.user.update_password.incorrect.app_error", nil, "")
+		} else {
+			c.Err = err
+		}
 		c.Err.StatusCode = http.StatusForbidden
 		return
 	}

api/user_test.go

@@ -249,6 +249,42 @@ func TestLoginWithDeviceId(t *testing.T) {
 	}
 }
 
+func TestPasswordGuessLockout(t *testing.T) {
+	th := Setup().InitBasic()
+	Client := th.BasicClient
+	user := th.BasicUser
+	Client.Must(Client.Logout())
+
+	enableSignInWithEmail := *utils.Cfg.EmailSettings.EnableSignInWithEmail
+	passwordAttempts := utils.Cfg.ServiceSettings.MaximumLoginAttempts
+	defer func() {
+		*utils.Cfg.EmailSettings.EnableSignInWithEmail = enableSignInWithEmail
+		utils.Cfg.ServiceSettings.MaximumLoginAttempts = passwordAttempts
+	}()
+	*utils.Cfg.EmailSettings.EnableSignInWithEmail = true
+	utils.Cfg.ServiceSettings.MaximumLoginAttempts = 2
+
+	// OK to log in
+	if _, err := Client.Login(user.Username, user.Password); err != nil {
+		t.Fatal(err)
+	}
+
+	Client.Must(Client.Logout())
+
+	// Fail twice
+	if _, err := Client.Login(user.Email, "notthepassword"); err == nil {
+		t.Fatal("Shouldn't be able to login with bad password.")
+	}
+	if _, err := Client.Login(user.Email, "notthepassword"); err == nil {
+		t.Fatal("Shouldn't be able to login with bad password.")
+	}
+
+	// Locked out
+	if _, err := Client.Login(user.Email, user.Password); err == nil {
+		t.Fatal("Shouldn't be able to login with password when account is locked out.")
+	}
+}
+
 func TestSessions(t *testing.T) {
 	th := Setup().InitBasic()
 	Client := th.BasicClient
@@ -746,6 +782,26 @@ func TestUserUpdatePassword(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// Test lockout
+	passwordAttempts := utils.Cfg.ServiceSettings.MaximumLoginAttempts
+	defer func() {
+		utils.Cfg.ServiceSettings.MaximumLoginAttempts = passwordAttempts
+	}()
+	utils.Cfg.ServiceSettings.MaximumLoginAttempts = 2
+
+	// Fail twice
+	if _, err := Client.UpdateUserPassword(user.Id, "badpwd", "newpwd"); err == nil {
+		t.Fatal("Should have errored")
+	}
+	if _, err := Client.UpdateUserPassword(user.Id, "badpwd", "newpwd"); err == nil {
+		t.Fatal("Should have errored")
+	}
+
+	// Should fail because account is locked out
+	if _, err := Client.UpdateUserPassword(user.Id, "newpwd1", "newpwd2"); err == nil {
+		t.Fatal("Should have errored")
+	}
+
 	user2 := &model.User{Email: strings.ToLower(model.NewId()) + "success+test@simulator.amazonses.com", Nickname: "Corey Hulen", Password: "passwd1"}
 	user2 = Client.Must(Client.CreateUser(user2, "")).Data.(*model.User)
 	LinkUserToTeam(user2, team)