[MM-37585] (#28634)

* add tests

* pass userID to the function instead of the user object.

* remove concurrent login simulation

---------

Co-authored-by: Mattermost Build <build@mattermost.com>

server/channels/api4/user_test.go

@@ -2219,14 +2219,14 @@ func TestPatchUser(t *testing.T) {
 	require.Equal(t, "America/New_York", ruser.Timezone["automaticTimezone"], "automaticTimezone should update properly")
 	require.Empty(t, ruser.Timezone["manualTimezone"], "manualTimezone should update properly")
 
-	appErr := th.App.CheckPasswordAndAllCriteria(th.Context, user, *patch.Password, "")
+	appErr := th.App.CheckPasswordAndAllCriteria(th.Context, user.Id, *patch.Password, "")
 	require.NotNil(t, appErr, "Password should not match")
 
 	currentPassword := user.Password
 	user, appErr = th.App.GetUser(ruser.Id)
 	require.Nil(t, appErr)
 
-	appErr = th.App.CheckPasswordAndAllCriteria(th.Context, user, currentPassword, "")
+	appErr = th.App.CheckPasswordAndAllCriteria(th.Context, user.Id, currentPassword, "")
 	require.Nil(t, appErr, "Password should still match")
 
 	patch = &model.UserPatch{}

server/channels/app/app_iface.go

@@ -483,7 +483,7 @@ type AppIface interface {
 	CheckForClientSideCert(r *http.Request) (string, string, string)
 	CheckIntegrity() <-chan model.IntegrityCheckResult
 	CheckMandatoryS3Fields(settings *model.FileSettings) *model.AppError
-	CheckPasswordAndAllCriteria(rctx request.CTX, user *model.User, password string, mfaToken string) *model.AppError
+	CheckPasswordAndAllCriteria(rctx request.CTX, userID string, password string, mfaToken string) *model.AppError
 	CheckPostReminders(rctx request.CTX)
 	CheckRolesExist(roleNames []string) *model.AppError
 	CheckUserAllAuthenticationCriteria(rctx request.CTX, user *model.User, mfaToken string) *model.AppError

server/channels/app/authentication.go

@@ -59,12 +59,25 @@ func (a *App) IsPasswordValid(rctx request.CTX, password string) *model.AppError
 	return nil
 }
 
-func (a *App) CheckPasswordAndAllCriteria(rctx request.CTX, user *model.User, password string, mfaToken string) *model.AppError {
-	if err := a.CheckUserPreflightAuthenticationCriteria(rctx, user, mfaToken); err != nil {
+func (a *App) CheckPasswordAndAllCriteria(rctx request.CTX, userID string, password string, mfaToken string) *model.AppError {
+	// MM-37585
+	// Use locks to avoid concurrently checking AND updating the failed login attempts.
+	a.ch.loginAttemptsMut.Lock()
+	defer a.ch.loginAttemptsMut.Unlock()
+
+	user, err := a.GetUser(userID)
+	if err != nil {
+		if err.Id != MissingAccountError {
+			err.StatusCode = http.StatusInternalServerError
+			return err
+		}
+		err.StatusCode = http.StatusBadRequest
 		return err
 	}
 
-	defer a.Srv().Store().User().InvalidateProfileCacheForUser(user.Id)
+	if err := a.CheckUserPreflightAuthenticationCriteria(rctx, user, mfaToken); err != nil {
+		return err
+	}
 
 	if err := users.CheckUserPassword(user, password); err != nil {
 		if passErr := a.Srv().Store().User().UpdateFailedPasswordAttempts(user.Id, user.FailedAttempts+1); passErr != nil {
@@ -271,7 +284,7 @@ func (a *App) authenticateUser(rctx request.CTX, user *model.User, password, mfa
 		return user, err
 	}
 
-	if err := a.CheckPasswordAndAllCriteria(rctx, user, password, mfaToken); err != nil {
+	if err := a.CheckPasswordAndAllCriteria(rctx, user.Id, password, mfaToken); err != nil {
 		if err.Id == "api.user.check_user_password.invalid.app_error" {
 			rctx.Logger().LogM(mlog.MlvlLDAPInfo, "A user tried to sign in, which matched a Mattermost account, but the password was incorrect.", mlog.String("username", user.Username))
 		}

server/channels/app/authentication_test.go

@@ -4,11 +4,15 @@
 package app
 
 import (
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"strconv"
+	"sync"
 	"testing"
+	"time"
 
+	"github.com/dgryski/dgoogauth"
 	"github.com/stretchr/testify/require"
 
 	"github.com/mattermost/mattermost/server/public/model"
@@ -55,3 +59,97 @@ func TestParseAuthTokenFromRequest(t *testing.T) {
 		require.Equal(t, tc.expectedLocation, location, "Wrong location on test "+strconv.Itoa(testnum))
 	}
 }
+
+func TestCheckPasswordAndAllCriteria(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	const maxFailedLoginAttempts = 3
+	const concurrentAttempts = maxFailedLoginAttempts + 1
+	th.App.UpdateConfig(func(cfg *model.Config) {
+		*cfg.ServiceSettings.MaximumLoginAttempts = maxFailedLoginAttempts
+		*cfg.ServiceSettings.EnableMultifactorAuthentication = true
+	})
+
+	password := "newpassword1"
+	appErr := th.App.UpdatePassword(th.Context, th.BasicUser, password)
+	require.Nil(t, appErr)
+
+	// setup MFA
+	secret, appErr := th.App.GenerateMfaSecret(th.BasicUser.Id)
+	require.Nil(t, appErr)
+	err := th.Server.Store().User().UpdateMfaActive(th.BasicUser.Id, true)
+	require.NoError(t, err)
+	err = th.Server.Store().User().UpdateMfaSecret(th.BasicUser.Id, secret.Secret)
+	require.NoError(t, err)
+
+	t.Run("should run successfully when attempts are available", func(t *testing.T) {
+		err = th.App.Srv().Store().User().UpdateFailedPasswordAttempts(th.BasicUser.Id, maxFailedLoginAttempts-1)
+		require.NoError(t, err)
+		code := dgoogauth.ComputeCode(secret.Secret, time.Now().UTC().Unix()/30)
+		token := fmt.Sprintf("%06d", code)
+
+		appErr = th.App.CheckPasswordAndAllCriteria(th.Context, th.BasicUser.Id, password, token)
+		require.Nil(t, appErr)
+	})
+
+	t.Run("validate concurrent failed attempts to bypass checks", func(t *testing.T) {
+		testCases := []struct {
+			name          string
+			password      string
+			mfaToken      string
+			expectedErrID string
+		}{
+			{
+				name:          "should not breach max. login attempts when password is wrong",
+				password:      "wrong password",
+				expectedErrID: "api.user.check_user_password.invalid.app_error",
+			},
+			{
+				name:          "should not breach max. login attempts when MFA is wrong",
+				password:      password,
+				mfaToken:      "123456",
+				expectedErrID: "api.user.check_user_mfa.bad_code.app_error",
+			},
+		}
+
+		for _, tc := range testCases {
+			t.Run(tc.name, func(t *testing.T) {
+				// Reset login attempts
+				err := th.App.Srv().Store().User().UpdateFailedPasswordAttempts(th.BasicUser.Id, 0)
+				require.NoError(t, err)
+
+				// Capture all concurrent errors
+				appErrs := make([]*model.AppError, concurrentAttempts)
+
+				// Wait to complete the test
+				var completeWG sync.WaitGroup
+				completeWG.Add(concurrentAttempts)
+
+				for i := 0; i < concurrentAttempts; i++ {
+					go func(i int) {
+						defer completeWG.Done()
+						// Simulate concurrent failed login checks by same user
+						appErrs[i] = th.App.CheckPasswordAndAllCriteria(th.Context, th.BasicUser.Id, tc.password, tc.mfaToken)
+					}(i)
+				}
+
+				completeWG.Wait()
+
+				expectedErrsCount := 0
+				for i := 0; i < concurrentAttempts; i++ {
+					if appErrs[i].Id == tc.expectedErrID {
+						expectedErrsCount++
+						continue
+					}
+
+					require.Equal(t, "api.user.check_user_login_attempts.too_many.app_error", appErrs[i].Id, "All other errors should be of too many login attempts only.")
+				}
+
+				// Password/MFA failure attempts should not breach the maxFailedAttempts
+				// even during concurrent access by the same user.
+				require.Equal(t, maxFailedLoginAttempts, expectedErrsCount)
+			})
+		}
+	})
+}

server/channels/app/channels.go

@@ -76,6 +76,8 @@ type Channels struct {
 
 	postReminderMut  sync.Mutex
 	postReminderTask *model.ScheduledTask
+
+	loginAttemptsMut sync.Mutex
 }
 
 func NewChannels(s *Server) (*Channels, error) {

server/channels/app/ldap.go

@@ -99,7 +99,7 @@ func (a *App) SwitchEmailToLdap(c request.CTX, email, password, code, ldapLoginI
 		return "", err
 	}
 
-	if err := a.CheckPasswordAndAllCriteria(c, user, password, code); err != nil {
+	if err := a.CheckPasswordAndAllCriteria(c, user.Id, password, code); err != nil {
 		return "", err
 	}
 

server/channels/app/oauth.go

@@ -956,7 +956,7 @@ func (a *App) SwitchEmailToOAuth(c request.CTX, w http.ResponseWriter, r *http.R
 		return "", err
 	}
 
-	if err = a.CheckPasswordAndAllCriteria(c, user, password, code); err != nil {
+	if err = a.CheckPasswordAndAllCriteria(c, user.Id, password, code); err != nil {
 		return "", err
 	}
 

server/channels/app/opentracing/opentracing_layer.go

@@ -1274,7 +1274,7 @@ func (a *OpenTracingAppLayer) CheckMandatoryS3Fields(settings *model.FileSetting
 	return resultVar0
 }
 
-func (a *OpenTracingAppLayer) CheckPasswordAndAllCriteria(rctx request.CTX, user *model.User, password string, mfaToken string) *model.AppError {
+func (a *OpenTracingAppLayer) CheckPasswordAndAllCriteria(rctx request.CTX, userID string, password string, mfaToken string) *model.AppError {
 	origCtx := a.ctx
 	span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.CheckPasswordAndAllCriteria")
 
@@ -1286,7 +1286,7 @@ func (a *OpenTracingAppLayer) CheckPasswordAndAllCriteria(rctx request.CTX, user
 	}()
 
 	defer span.Finish()
-	resultVar0 := a.app.CheckPasswordAndAllCriteria(rctx, user, password, mfaToken)
+	resultVar0 := a.app.CheckPasswordAndAllCriteria(rctx, userID, password, mfaToken)
 
 	if resultVar0 != nil {
 		span.LogFields(spanlog.Error(resultVar0))

server/channels/store/localcachelayer/user_layer.go

@@ -216,6 +216,11 @@ func (s *LocalCacheUserStore) GetProfileByIds(ctx context.Context, userIds []str
 	return users, nil
 }
 
+func (s *LocalCacheUserStore) UpdateFailedPasswordAttempts(userID string, attempts int) error {
+	s.InvalidateProfileCacheForUser(userID)
+	return s.UserStore.UpdateFailedPasswordAttempts(userID, attempts)
+}
+
 // Get is a cache wrapper around the SqlStore method to get a user profile by id.
 // It checks if the user entry is present in the cache, returning the entry from cache
 // if it is present. Otherwise, it fetches the entry from the store and stores it in the