MM-14039 Improving password lockout functionality. (#10254)

* Improving password lockout functionality.

* Switching order of mfa password checks to passowrd -> mfa

api4/user_test.go

@@ -1124,7 +1124,7 @@ func TestPatchUser(t *testing.T) {
 	if ruser.Username != user.Username {
 		t.Fatal("Username should not have updated")
 	}
-	if ruser.Password != ""{
+	if ruser.Password != "" {
 		t.Fatal("Password should not be returned")
 	}
 	if ruser.NotifyProps["comment"] != "somethingrandom" {
@@ -3100,3 +3100,40 @@ func TestGetUserTermsOfService(t *testing.T) {
 	assert.Equal(t, termsOfService.Id, userTermsOfService.TermsOfServiceId)
 	assert.NotEmpty(t, userTermsOfService.CreateAt)
 }
+
+func TestLoginLockout(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+
+	_, resp := th.Client.Logout()
+	CheckNoError(t, resp)
+
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.MaximumLoginAttempts = 3 })
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableMultifactorAuthentication = true })
+
+	_, resp = th.Client.Login(th.BasicUser.Email, "wrong")
+	CheckErrorMessage(t, resp, "api.user.check_user_password.invalid.app_error")
+	_, resp = th.Client.Login(th.BasicUser.Email, "wrong")
+	CheckErrorMessage(t, resp, "api.user.check_user_password.invalid.app_error")
+	_, resp = th.Client.Login(th.BasicUser.Email, "wrong")
+	CheckErrorMessage(t, resp, "api.user.check_user_password.invalid.app_error")
+	_, resp = th.Client.Login(th.BasicUser.Email, "wrong")
+	CheckErrorMessage(t, resp, "api.user.check_user_login_attempts.too_many.app_error")
+	_, resp = th.Client.Login(th.BasicUser.Email, "wrong")
+	CheckErrorMessage(t, resp, "api.user.check_user_login_attempts.too_many.app_error")
+
+	// Fake user has MFA enabled
+	if result := <-th.Server.Store.User().UpdateMfaActive(th.BasicUser2.Id, true); result.Err != nil {
+		t.Fatal(result.Err)
+	}
+	_, resp = th.Client.LoginWithMFA(th.BasicUser2.Email, th.BasicUser2.Password, "000000")
+	CheckErrorMessage(t, resp, "api.user.check_user_mfa.bad_code.app_error")
+	_, resp = th.Client.LoginWithMFA(th.BasicUser2.Email, th.BasicUser2.Password, "000000")
+	CheckErrorMessage(t, resp, "api.user.check_user_mfa.bad_code.app_error")
+	_, resp = th.Client.LoginWithMFA(th.BasicUser2.Email, th.BasicUser2.Password, "000000")
+	CheckErrorMessage(t, resp, "api.user.check_user_mfa.bad_code.app_error")
+	_, resp = th.Client.LoginWithMFA(th.BasicUser2.Email, th.BasicUser2.Password, "000000")
+	CheckErrorMessage(t, resp, "api.user.check_user_login_attempts.too_many.app_error")
+	_, resp = th.Client.LoginWithMFA(th.BasicUser2.Email, th.BasicUser2.Password, "000000")
+	CheckErrorMessage(t, resp, "api.user.check_user_login_attempts.too_many.app_error")
+}

app/authentication.go

@@ -46,9 +46,23 @@ func (a *App) CheckPasswordAndAllCriteria(user *model.User, password string, mfa
 	}
 
 	if err := a.checkUserPassword(user, password); err != nil {
+		if result := <-a.Srv.Store.User().UpdateFailedPasswordAttempts(user.Id, user.FailedAttempts+1); result.Err != nil {
+			return result.Err
+		}
 		return err
 	}
 
+	if err := a.CheckUserMfa(user, mfaToken); err != nil {
+		if result := <-a.Srv.Store.User().UpdateFailedPasswordAttempts(user.Id, user.FailedAttempts+1); result.Err != nil {
+			return result.Err
+		}
+		return err
+	}
+
+	if result := <-a.Srv.Store.User().UpdateFailedPasswordAttempts(user.Id, 0); result.Err != nil {
+		return result.Err
+	}
+
 	if err := a.CheckUserPostflightAuthenticationCriteria(user); err != nil {
 		return err
 	}
@@ -63,25 +77,24 @@ func (a *App) DoubleCheckPassword(user *model.User, password string) *model.AppE
 	}
 
 	if err := a.checkUserPassword(user, password); err != nil {
+		if result := <-a.Srv.Store.User().UpdateFailedPasswordAttempts(user.Id, user.FailedAttempts+1); result.Err != nil {
+			return result.Err
+		}
 		return err
 	}
 
+	if result := <-a.Srv.Store.User().UpdateFailedPasswordAttempts(user.Id, 0); result.Err != nil {
+		return result.Err
+	}
+
 	return nil
 }
 
 func (a *App) checkUserPassword(user *model.User, password string) *model.AppError {
 	if !model.ComparePassword(user.Password, password) {
-		if result := <-a.Srv.Store.User().UpdateFailedPasswordAttempts(user.Id, user.FailedAttempts+1); result.Err != nil {
-			return result.Err
-		}
-
 		return model.NewAppError("checkUserPassword", "api.user.check_user_password.invalid.app_error", nil, "user_id="+user.Id, http.StatusUnauthorized)
 	}
 
-	if result := <-a.Srv.Store.User().UpdateFailedPasswordAttempts(user.Id, 0); result.Err != nil {
-		return result.Err
-	}
-
 	return nil
 }
 
@@ -122,10 +135,6 @@ func (a *App) CheckUserAllAuthenticationCriteria(user *model.User, mfaToken stri
 }
 
 func (a *App) CheckUserPreflightAuthenticationCriteria(user *model.User, mfaToken string) *model.AppError {
-	if err := a.CheckUserMfa(user, mfaToken); err != nil {
-		return err
-	}
-
 	if err := checkUserNotDisabled(user); err != nil {
 		return err
 	}

model/client4.go

@@ -622,6 +622,15 @@ func (c *Client4) LoginWithDevice(loginId string, password string, deviceId stri
 	return c.login(m)
 }
 
+// LoginWithMFA logs a user in with a MFA token
+func (c *Client4) LoginWithMFA(loginId, password, mfaToken string) (*User, *Response) {
+	m := make(map[string]string)
+	m["login_id"] = loginId
+	m["password"] = password
+	m["token"] = mfaToken
+	return c.login(m)
+}
+
 func (c *Client4) login(m map[string]string) (*User, *Response) {
 	r, err := c.DoApiPost("/users/login", MapToJson(m))
 	if err != nil {