From cc5b3a2f69ebf332655e0db367103c2326f10c35 Mon Sep 17 00:00:00 2001 From: Vishal Date: Tue, 22 Oct 2024 12:21:36 +0530 Subject: [PATCH] [MM-37585] (#28634) * add tests * pass userID to the function instead of the user object. * remove concurrent login simulation --------- Co-authored-by: Mattermost Build --- server/channels/api4/user_test.go | 4 +- server/channels/app/app_iface.go | 2 +- server/channels/app/authentication.go | 21 +++- server/channels/app/authentication_test.go | 98 +++++++++++++++++++ server/channels/app/channels.go | 2 + server/channels/app/ldap.go | 2 +- server/channels/app/oauth.go | 2 +- .../app/opentracing/opentracing_layer.go | 4 +- .../store/localcachelayer/user_layer.go | 5 + 9 files changed, 129 insertions(+), 11 deletions(-) diff --git a/server/channels/api4/user_test.go b/server/channels/api4/user_test.go index 1744a72b8e..374c962ffe 100644 --- a/server/channels/api4/user_test.go +++ b/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{} diff --git a/server/channels/app/app_iface.go b/server/channels/app/app_iface.go index 921b711024..21e62ce488 100644 --- a/server/channels/app/app_iface.go +++ b/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 diff --git a/server/channels/app/authentication.go b/server/channels/app/authentication.go index 0a53ad41ef..08115b0346 100644 --- a/server/channels/app/authentication.go +++ b/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)) } diff --git a/server/channels/app/authentication_test.go b/server/channels/app/authentication_test.go index c3ca41a95d..586550669e 100644 --- a/server/channels/app/authentication_test.go +++ b/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) + }) + } + }) +} diff --git a/server/channels/app/channels.go b/server/channels/app/channels.go index 9a9e9a5d72..a63bb5c1e7 100644 --- a/server/channels/app/channels.go +++ b/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) { diff --git a/server/channels/app/ldap.go b/server/channels/app/ldap.go index 54342204ac..d524a6518e 100644 --- a/server/channels/app/ldap.go +++ b/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 } diff --git a/server/channels/app/oauth.go b/server/channels/app/oauth.go index 636cb3f818..7362698dfc 100644 --- a/server/channels/app/oauth.go +++ b/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 } diff --git a/server/channels/app/opentracing/opentracing_layer.go b/server/channels/app/opentracing/opentracing_layer.go index 867cce9439..7ada0dd57a 100644 --- a/server/channels/app/opentracing/opentracing_layer.go +++ b/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)) diff --git a/server/channels/store/localcachelayer/user_layer.go b/server/channels/store/localcachelayer/user_layer.go index bb130b2bf6..986a810068 100644 --- a/server/channels/store/localcachelayer/user_layer.go +++ b/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