diff --git a/NOTICE.txt b/NOTICE.txt index 1fb5981b936..28ed72d119c 100644 --- a/NOTICE.txt +++ b/NOTICE.txt @@ -2570,6 +2570,78 @@ The above copyright notice and this permission notice shall be included in all c THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +--- + +## Azure/azure-sdk-for-go + +This product contains 'Azure/azure-sdk-for-go' by Microsoft Azure. + +This repository is for active development of the Azure SDK for Go. For consumers of the SDK we recommend visiting our public developer docs at: + +* HOMEPAGE: + * https://docs.microsoft.com/azure/developer/go/ + +* LICENSE: MIT License + +The MIT License (MIT) + +Copyright (c) Microsoft Corporation. + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. + + +--- + +## Azure/azure-sdk-for-go + +This product contains 'Azure/azure-sdk-for-go' by Microsoft Azure. + +This repository is for active development of the Azure SDK for Go. For consumers of the SDK we recommend visiting our public developer docs at: + +* HOMEPAGE: + * https://docs.microsoft.com/azure/developer/go/ + +* LICENSE: MIT License + +The MIT License (MIT) + +Copyright (c) Microsoft Corporation. + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. + + --- ## Go @@ -2846,42 +2918,6 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. ---- - -## anthonynsimon/bild - -This product contains 'anthonynsimon/bild' by anthonynsimon. - -Image processing algorithms in pure Go - -* HOMEPAGE: - * https://github.com/anthonynsimon/bild - -* LICENSE: MIT License - -MIT License - -Copyright (c) 2016-2024 Anthony Simon - -Permission is hereby granted, free of charge, to any person obtaining a copy -of this software and associated documentation files (the "Software"), to deal -in the Software without restriction, including without limitation the rights -to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -copies of the Software, and to permit persons to whom the Software is -furnished to do so, subject to the following conditions: - -The above copyright notice and this permission notice shall be included in all -copies or substantial portions of the Software. - -THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -SOFTWARE. - - --- ## aws/aws-sdk-go-v2 @@ -4074,6 +4110,42 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +--- + +## boxes-ltd/imaging + +This product contains 'boxes-ltd/imaging' by Boxes Ltd. + +Imaging is a simple image processing package for Go + +* HOMEPAGE: + * https://github.com/boxes-ltd/imaging + +* LICENSE: MIT License + +The MIT License (MIT) + +Copyright (c) 2012 Grigory Dryapak + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. + + --- ## buffer @@ -5840,6 +5912,48 @@ GoMock is a mocking framework for the Go programming language. limitations under the License. +--- + +## google/uuid + +This product contains 'google/uuid' by Google. + +Go package for UUIDs based on RFC 4122 and DCE 1.1: Authentication and Security Services. + +* HOMEPAGE: + * https://github.com/google/uuid + +* LICENSE: BSD 3-Clause "New" or "Revised" License + +Copyright (c) 2009,2014 Google Inc. All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are +met: + + * Redistributions of source code must retain the above copyright +notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the following disclaimer +in the documentation and/or other materials provided with the +distribution. + * Neither the name of Google Inc. nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + + --- ## gorilla/handlers diff --git a/server/channels/api4/user.go b/server/channels/api4/user.go index 7a5aec2426d..2f4d96571eb 100644 --- a/server/channels/api4/user.go +++ b/server/channels/api4/user.go @@ -1955,6 +1955,8 @@ func updatePassword(c *Context, w http.ResponseWriter, r *http.Request) { if user.IsSystemAdmin() { canUpdatePassword = c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) + } else if user.IsBot { + canUpdatePassword = c.App.SessionHasPermissionToManageBot(c.AppContext, *c.AppContext.Session(), c.Params.UserId) == nil } else { canUpdatePassword = c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleWriteUserManagementUsers) } @@ -2527,7 +2529,7 @@ func revokeSession(c *Context, w http.ResponseWriter, r *http.Request) { auditRec := c.MakeAuditRecord(model.AuditEventRevokeSession, model.AuditStatusFail) defer c.LogAuditRec(auditRec) - if !c.App.SessionHasPermissionToUser(*c.AppContext.Session(), c.Params.UserId) { + if !c.App.SessionHasPermissionToUserOrBot(c.AppContext, *c.AppContext.Session(), c.Params.UserId) { c.SetPermissionError(model.PermissionEditOtherUsers) return } @@ -2575,7 +2577,7 @@ func revokeAllSessionsForUser(c *Context, w http.ResponseWriter, r *http.Request defer c.LogAuditRec(auditRec) model.AddEventParameterToAuditRec(auditRec, "user_id", c.Params.UserId) - if !c.App.SessionHasPermissionToUser(*c.AppContext.Session(), c.Params.UserId) { + if !c.App.SessionHasPermissionToUserOrBot(c.AppContext, *c.AppContext.Session(), c.Params.UserId) { c.SetPermissionError(model.PermissionEditOtherUsers) return } diff --git a/server/channels/api4/user_test.go b/server/channels/api4/user_test.go index 127aa75a9ed..fddc86cb308 100644 --- a/server/channels/api4/user_test.go +++ b/server/channels/api4/user_test.go @@ -4512,6 +4512,61 @@ func TestRevokeSessions(t *testing.T) { require.NoError(t, err) } +func TestRevokeSessionBotPermissions(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableBotAccountCreation = true + }) + + bot, botResp, err := th.SystemAdminClient.CreateBot(context.Background(), &model.Bot{ + Username: GenerateTestUsername(), + DisplayName: "Test Bot", + Description: "bot for revoke-session permission test", + }) + require.NoError(t, err) + CheckCreatedStatus(t, botResp) + defer func() { + appErr := th.App.PermanentDeleteBot(th.Context, bot.UserId) + assert.Nil(t, appErr) + }() + + t.Run("user manager without bot permissions cannot revoke bot session", func(t *testing.T) { + th.AddPermissionToRole(t, model.PermissionSysconsoleWriteUserManagementUsers.Id, model.SystemUserRoleId) + defer th.RemovePermissionFromRole(t, model.PermissionSysconsoleWriteUserManagementUsers.Id, model.SystemUserRoleId) + + // Seed a real session so the test confirms the permission gate blocks + // access even when the target session genuinely exists. + botSession, appErr := th.App.CreateSession(th.Context, &model.Session{UserId: bot.UserId}) + require.Nil(t, appErr) + + th.LoginBasic(t) + + resp, err := th.Client.RevokeSession(context.Background(), bot.UserId, botSession.Id) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + }) + + t.Run("user with bot management permissions can revoke bot session", func(t *testing.T) { + th.AddPermissionToRole(t, model.PermissionManageOthersBots.Id, model.SystemUserRoleId) + defer th.RemovePermissionFromRole(t, model.PermissionManageOthersBots.Id, model.SystemUserRoleId) + + // Seed a real session for the bot directly via the app layer. + botSession, appErr := th.App.CreateSession(th.Context, &model.Session{UserId: bot.UserId}) + require.Nil(t, appErr) + + th.LoginBasic(t) + + _, err := th.Client.RevokeSession(context.Background(), bot.UserId, botSession.Id) + require.NoError(t, err) + + // Confirm the session row is gone. + _, appErr = th.App.GetSessionById(th.Context, botSession.Id) + require.NotNil(t, appErr, "session should no longer exist after revocation") + }) +} + func TestRevokeAllSessions(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t) @@ -7405,6 +7460,49 @@ func TestUpdatePassword(t *testing.T) { }) } +func TestUpdatePasswordBotPermissions(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableBotAccountCreation = true + }) + + bot, botResp, err := th.SystemAdminClient.CreateBot(context.Background(), &model.Bot{ + Username: GenerateTestUsername(), + DisplayName: "Test Bot", + Description: "bot for update-password permission test", + }) + require.NoError(t, err) + CheckCreatedStatus(t, botResp) + defer func() { + appErr := th.App.PermanentDeleteBot(th.Context, bot.UserId) + assert.Nil(t, appErr) + }() + + t.Run("user manager without bot permissions cannot update bot password", func(t *testing.T) { + th.AddPermissionToRole(t, model.PermissionSysconsoleWriteUserManagementUsers.Id, model.SystemUserRoleId) + defer th.RemovePermissionFromRole(t, model.PermissionSysconsoleWriteUserManagementUsers.Id, model.SystemUserRoleId) + + th.LoginBasic(t) + + resp, err := th.Client.UpdatePassword(context.Background(), bot.UserId, "", model.NewTestPassword()) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + }) + + t.Run("user with bot management permissions can update bot password", func(t *testing.T) { + th.AddPermissionToRole(t, model.PermissionManageOthersBots.Id, model.SystemUserRoleId) + defer th.RemovePermissionFromRole(t, model.PermissionManageOthersBots.Id, model.SystemUserRoleId) + + th.LoginBasic(t) + + resp, err := th.Client.UpdatePassword(context.Background(), bot.UserId, "", model.NewTestPassword()) + require.NoError(t, err) + CheckOKStatus(t, resp) + }) +} + func TestUpdatePasswordAudit(t *testing.T) { logFile, err := os.CreateTemp("", "adv.log") require.NoError(t, err) @@ -9823,6 +9921,57 @@ func TestRevokeAllSessionsForUser(t *testing.T) { }) } +func TestRevokeAllSessionsForUserBotPermissions(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableBotAccountCreation = true + }) + + bot, botResp, err := th.SystemAdminClient.CreateBot(context.Background(), &model.Bot{ + Username: GenerateTestUsername(), + DisplayName: "Test Bot", + Description: "bot for revoke-all-sessions permission test", + }) + require.NoError(t, err) + CheckCreatedStatus(t, botResp) + defer func() { + appErr := th.App.PermanentDeleteBot(th.Context, bot.UserId) + assert.Nil(t, appErr) + }() + + t.Run("user manager without bot permissions cannot revoke all sessions for a bot", func(t *testing.T) { + th.AddPermissionToRole(t, model.PermissionSysconsoleWriteUserManagementUsers.Id, model.SystemUserRoleId) + defer th.RemovePermissionFromRole(t, model.PermissionSysconsoleWriteUserManagementUsers.Id, model.SystemUserRoleId) + + th.LoginBasic(t) + + resp, err := th.Client.RevokeAllSessions(context.Background(), bot.UserId) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + }) + + t.Run("user with bot management permissions can revoke all sessions for a bot", func(t *testing.T) { + th.AddPermissionToRole(t, model.PermissionManageOthersBots.Id, model.SystemUserRoleId) + defer th.RemovePermissionFromRole(t, model.PermissionManageOthersBots.Id, model.SystemUserRoleId) + + // Seed a real session so RevokeAllSessions is not a no-op. + _, appErr := th.App.CreateSession(th.Context, &model.Session{UserId: bot.UserId}) + require.Nil(t, appErr) + + th.LoginBasic(t) + + _, err := th.Client.RevokeAllSessions(context.Background(), bot.UserId) + require.NoError(t, err) + + // Confirm all sessions for the bot are gone. + sessions, appErr := th.App.GetSessions(th.Context, bot.UserId) + require.Nil(t, appErr) + require.Empty(t, sessions, "all bot sessions should be revoked") + }) +} + func TestResetPasswordFailedAttempts(t *testing.T) { th := SetupEnterprise(t).InitBasic(t) th.SetupLdapConfig() diff --git a/server/channels/app/authentication.go b/server/channels/app/authentication.go index edda80f5aac..73e7668f856 100644 --- a/server/channels/app/authentication.go +++ b/server/channels/app/authentication.go @@ -62,7 +62,7 @@ func (a *App) IsPasswordValid(rctx request.CTX, password string) *model.AppError return nil } -func (a *App) checkUserPassword(user *model.User, password string, invalidateCache bool) *model.AppError { +func (a *App) checkUserPassword(user *model.User, password string) *model.AppError { if user.Password == "" || password == "" { return model.NewAppError("checkUserPassword", "api.user.check_user_password.invalid.app_error", nil, "user_id="+user.Id, http.StatusUnauthorized) } @@ -76,16 +76,6 @@ func (a *App) checkUserPassword(user *model.User, password string, invalidateCac // Compare the password using the hasher that generated it err = hasher.CompareHashAndPassword(phc, password) if err != nil && errors.Is(err, hashers.ErrMismatchedHashAndPassword) { - // Increment the number of failed password attempts in case of - // mismatched hash and password - if passErr := a.Srv().Store().User().UpdateFailedPasswordAttempts(user.Id, user.FailedAttempts+1); passErr != nil { - return model.NewAppError("CheckPasswordAndAllCriteria", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(passErr) - } - - if invalidateCache { - a.InvalidateCacheForUser(user.Id) - } - return model.NewAppError("checkUserPassword", "api.user.check_user_password.invalid.app_error", nil, "user_id="+user.Id, http.StatusUnauthorized).Wrap(err) } else if err != nil { return model.NewAppError("checkUserPassword", "app.valid_password_generic.app_error", nil, "", http.StatusInternalServerError).Wrap(err) @@ -118,11 +108,6 @@ func (a *App) migratePassword(user *model.User, password string) *model.AppError } 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.emailLoginAttemptsMut.Lock() - defer a.ch.emailLoginAttemptsMut.Unlock() - user, err := a.GetUser(userID) if err != nil { if err.Id != MissingAccountError { @@ -137,16 +122,36 @@ func (a *App) CheckPasswordAndAllCriteria(rctx request.CTX, userID string, passw return err } - if err := a.checkUserPassword(user, password, false); err != nil { + maxAttempts := *a.Config().ServiceSettings.MaximumLoginAttempts + claimed, claimErr := a.Srv().Store().User().TryIncrementFailedPasswordAttempts(user.Id, maxAttempts) + if claimErr != nil { + return model.NewAppError("CheckPasswordAndAllCriteria", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(claimErr) + } + if !claimed { + return model.NewAppError("checkUserLoginAttempts", "api.user.check_user_login_attempts.too_many.app_error", nil, "user_id="+user.Id, http.StatusUnauthorized) + } + + if err := a.checkUserPassword(user, password); err != nil { + // Only keep the claimed slot when the failure is an actual + // credential mismatch; backend errors (hasher failures, migration + // failures, malformed stored hash) must not consume a slot or a + // transient infra issue could lock out a user with valid creds. + if err.Id != "api.user.check_user_password.invalid.app_error" { + if passErr := a.Srv().Store().User().DecrementFailedPasswordAttempts(user.Id); passErr != nil { + rctx.Logger().Warn("failed to refund login attempt slot", mlog.String("user_id", user.Id), mlog.Err(passErr)) + } + } return err } if err := a.CheckUserMfa(rctx, user, mfaToken); err != nil { - // If the mfaToken is not set, we assume the client used this as a pre-flight request to query the server - // about the MFA state of the user in question - if mfaToken != "" { - if passErr := a.Srv().Store().User().UpdateFailedPasswordAttempts(user.Id, user.FailedAttempts+1); passErr != nil { - return model.NewAppError("CheckPasswordAndAllCriteria", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(passErr) + // The slot we claimed already counts this as a failed attempt; + // the only special case is when no mfaToken was provided, which + // is treated as a pre-flight MFA-state probe rather than a real + // attempt — refund the slot so the probe is not counted. + if mfaToken == "" { + if passErr := a.Srv().Store().User().DecrementFailedPasswordAttempts(user.Id); passErr != nil { + rctx.Logger().Warn("failed to refund MFA probe slot", mlog.String("user_id", user.Id), mlog.Err(passErr)) } } @@ -166,11 +171,21 @@ func (a *App) CheckPasswordAndAllCriteria(rctx request.CTX, userID string, passw // This to be used for places we check the users password when they are already logged in func (a *App) DoubleCheckPassword(rctx request.CTX, user *model.User, password string) *model.AppError { - if err := checkUserLoginAttempts(user, *a.Config().ServiceSettings.MaximumLoginAttempts); err != nil { - return err + maxAttempts := *a.Config().ServiceSettings.MaximumLoginAttempts + claimed, claimErr := a.Srv().Store().User().TryIncrementFailedPasswordAttempts(user.Id, maxAttempts) + if claimErr != nil { + return model.NewAppError("DoubleCheckPassword", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(claimErr) + } + if !claimed { + return model.NewAppError("checkUserLoginAttempts", "api.user.check_user_login_attempts.too_many.app_error", nil, "user_id="+user.Id, http.StatusUnauthorized) } - if err := a.checkUserPassword(user, password, true); err != nil { + if err := a.checkUserPassword(user, password); err != nil { + if err.Id != "api.user.check_user_password.invalid.app_error" { + if passErr := a.Srv().Store().User().DecrementFailedPasswordAttempts(user.Id); passErr != nil { + rctx.Logger().Warn("failed to refund login attempt slot", mlog.String("user_id", user.Id), mlog.Err(passErr)) + } + } return err } @@ -184,11 +199,7 @@ func (a *App) DoubleCheckPassword(rctx request.CTX, user *model.User, password s } func (a *App) checkLdapUserPasswordAndAllCriteria(rctx request.CTX, user *model.User, password, mfaToken string) (*model.User, *model.AppError) { - // MM-37585: Use locks to avoid concurrently checking AND updating the failed login attempts. - a.ch.ldapLoginAttemptsMut.Lock() - defer a.ch.ldapLoginAttemptsMut.Unlock() - - // We need to get the latest value of the user from the database after we acquire the lock. user is nil for first-time LDAP users. + // We need to get the latest value of the user from the database. user.Id is empty for first-time LDAP users. if user.Id != "" { var err *model.AppError user, err = a.GetUser(user.Id) @@ -209,10 +220,16 @@ func (a *App) checkLdapUserPasswordAndAllCriteria(rctx request.CTX, user *model. return nil, err } - // First time LDAP users will not have a userID + maxAttempts := *a.Config().LdapSettings.MaximumLoginAttempts + + // First-time LDAP users have no local row yet to pre-claim against. if user.Id != "" { - if err := checkUserLoginAttempts(user, *a.Config().LdapSettings.MaximumLoginAttempts); err != nil { - return nil, err + claimed, claimErr := a.Srv().Store().User().TryIncrementFailedPasswordAttempts(user.Id, maxAttempts) + if claimErr != nil { + return nil, model.NewAppError("checkLdapUserPasswordAndAllCriteria", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(claimErr) + } + if !claimed { + return nil, model.NewAppError("checkUserLoginAttempts", "api.user.check_user_login_attempts.too_many_ldap.app_error", nil, "user_id="+user.Id, http.StatusUnauthorized) } } @@ -233,8 +250,23 @@ func (a *App) checkLdapUserPasswordAndAllCriteria(rctx request.CTX, user *model. if err.Id == "ent.ldap.do_login.invalid_password.app_error" { rctx.Logger().LogM(mlog.MlvlLDAPInfo, "A user tried to sign in, which matched an LDAP account, but the password was incorrect.", mlog.String("ldap_id", *ldapID)) - if passErr := a.Srv().Store().User().UpdateFailedPasswordAttempts(ldapUser.Id, ldapUser.FailedAttempts+1); passErr != nil { - return nil, model.NewAppError("CheckPasswordAndAllCriteria", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(passErr) + // For existing users we already claimed the slot above, so the + // counter has already been bumped. For first-time users (the + // row was just created by DoLogin) we still need to count the + // failed attempt explicitly, using the atomic primitive so + // concurrent first-attempt requests cannot overwrite each + // other's increments. + if user.Id == "" { + if _, passErr := a.Srv().Store().User().TryIncrementFailedPasswordAttempts(ldapUser.Id, maxAttempts); passErr != nil { + rctx.Logger().Warn("failed to record failed attempt for first-time LDAP user", mlog.String("user_id", ldapUser.Id), mlog.Err(passErr)) + } + } + } else if user.Id != "" { + // Non-credential failure (LDAP unreachable, transient error, + // etc.) on an existing user must not consume the slot we + // pre-claimed, or an LDAP outage could lock out everyone. + if passErr := a.Srv().Store().User().DecrementFailedPasswordAttempts(user.Id); passErr != nil { + rctx.Logger().Warn("failed to refund LDAP login attempt slot", mlog.String("user_id", user.Id), mlog.Err(passErr)) } } @@ -243,24 +275,43 @@ func (a *App) checkLdapUserPasswordAndAllCriteria(rctx request.CTX, user *model. } if err = a.CheckUserMfa(rctx, ldapUser, mfaToken); err != nil { - // If the mfaToken is not set, we assume the client used this as a pre-flight request to query the server - // about the MFA state of the user in question - if mfaToken != "" && ldapUser.Id != "" { - if passErr := a.Srv().Store().User().UpdateFailedPasswordAttempts(ldapUser.Id, ldapUser.FailedAttempts+1); passErr != nil { - return nil, model.NewAppError("CheckPasswordAndAllCriteria", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(passErr) + // For existing LDAP users we pre-claimed a slot, so it already + // counts as a failed attempt. The only special case is when no + // mfaToken was provided, which is treated as a pre-flight + // MFA-state probe rather than a real attempt — refund the slot + // so the probe is not counted. + // + // For first-time LDAP users we did not pre-claim (no row to + // claim against), so a real MFA attempt with a non-empty token + // still needs to be counted explicitly against the freshly + // created row. + switch { + case user.Id == "" && mfaToken != "": + if _, passErr := a.Srv().Store().User().TryIncrementFailedPasswordAttempts(ldapUser.Id, maxAttempts); passErr != nil { + rctx.Logger().Warn("failed to record failed MFA attempt for first-time LDAP user", mlog.String("user_id", ldapUser.Id), mlog.Err(passErr)) + } + case user.Id != "" && mfaToken == "": + if passErr := a.Srv().Store().User().DecrementFailedPasswordAttempts(ldapUser.Id); passErr != nil { + rctx.Logger().Warn("failed to refund LDAP MFA probe slot", mlog.String("user_id", ldapUser.Id), mlog.Err(passErr)) } } return nil, err } if err = checkUserNotDisabled(ldapUser); err != nil { + // Existing LDAP users had a slot pre-claimed; a disabled-account + // rejection is not a credential failure, so refund the slot so a + // reactivated user is not immediately rate-limited. + if user.Id != "" { + if passErr := a.Srv().Store().User().DecrementFailedPasswordAttempts(ldapUser.Id); passErr != nil { + rctx.Logger().Warn("failed to refund disabled LDAP login attempt slot", mlog.String("user_id", ldapUser.Id), mlog.Err(passErr)) + } + } return nil, err } - if ldapUser.FailedAttempts > 0 { - if passErr := a.Srv().Store().User().UpdateFailedPasswordAttempts(ldapUser.Id, 0); passErr != nil { - return nil, model.NewAppError("CheckPasswordAndAllCriteria", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(passErr) - } + if passErr := a.Srv().Store().User().UpdateFailedPasswordAttempts(ldapUser.Id, 0); passErr != nil { + return nil, model.NewAppError("checkLdapUserPasswordAndAllCriteria", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(passErr) } // user successfully authenticated diff --git a/server/channels/app/authentication_test.go b/server/channels/app/authentication_test.go index 4e718b58d80..a581af0e4e9 100644 --- a/server/channels/app/authentication_test.go +++ b/server/channels/app/authentication_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "golang.org/x/crypto/bcrypt" + "golang.org/x/sync/errgroup" "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/v8/channels/app/password/hashers" @@ -96,6 +97,63 @@ func TestCheckPasswordAndAllCriteria(t *testing.T) { appErr = th.App.CheckPasswordAndAllCriteria(th.Context, th.BasicUser.Id, password, token) require.Nil(t, appErr) + + updatedUser, appErr := th.App.GetUser(th.BasicUser.Id) + require.Nil(t, appErr) + require.Equal(t, 0, updatedUser.FailedAttempts, "successful login must reset FailedAttempts") + }) + + t.Run("MFA pre-flight probe does not consume a slot", func(t *testing.T) { + // An empty mfaToken on an MFA-enabled user is a pre-flight probe + // (the client is checking whether MFA is required) and must not + // count as a failed attempt. + err := th.App.Srv().Store().User().UpdateFailedPasswordAttempts(th.BasicUser.Id, 0) + require.NoError(t, err) + + appErr := th.App.CheckPasswordAndAllCriteria(th.Context, th.BasicUser.Id, password, "") + require.NotNil(t, appErr) + require.Equal(t, "mfa.validate_token.authenticate.app_error", appErr.Id) + + updatedUser, appErr := th.App.GetUser(th.BasicUser.Id) + require.Nil(t, appErr) + require.Equal(t, 0, updatedUser.FailedAttempts, "MFA probe must not consume a slot") + }) + + t.Run("MFA real attempt with a wrong token consumes a slot", func(t *testing.T) { + // A non-empty bad mfaToken is a real attempt, not a probe; the + // slot the pre-claim consumed stays consumed. + err := th.App.Srv().Store().User().UpdateFailedPasswordAttempts(th.BasicUser.Id, 0) + require.NoError(t, err) + + appErr := th.App.CheckPasswordAndAllCriteria(th.Context, th.BasicUser.Id, password, "123456") + require.NotNil(t, appErr) + require.Equal(t, "api.user.check_user_mfa.bad_code.app_error", appErr.Id) + + updatedUser, appErr := th.App.GetUser(th.BasicUser.Id) + require.Nil(t, appErr) + require.Equal(t, 1, updatedUser.FailedAttempts, "real MFA failure must consume a slot") + }) + + t.Run("backend error refunds the slot", func(t *testing.T) { + // Backend errors during the password check (malformed stored hash, + // hasher misc failure, migration failure) must not consume a slot + // or a transient infra issue could lock out a user with valid + // credentials. We trigger this via an unparseable PHC string, + // which surfaces as invalid_hash.app_error. + badHashUser := th.CreateUser(t) + err := th.Server.Store().User().UpdatePassword(badHashUser.Id, "$pbkdf2$bogus") + require.NoError(t, err) + th.App.InvalidateCacheForUser(badHashUser.Id) + err = th.App.Srv().Store().User().UpdateFailedPasswordAttempts(badHashUser.Id, 0) + require.NoError(t, err) + + appErr := th.App.CheckPasswordAndAllCriteria(th.Context, badHashUser.Id, "any-password", "") + require.NotNil(t, appErr) + require.Equal(t, "api.user.check_user_password.invalid_hash.app_error", appErr.Id) + + updatedUser, appErr := th.App.GetUser(badHashUser.Id) + require.Nil(t, appErr) + require.Equal(t, 0, updatedUser.FailedAttempts, "backend error must not consume a slot") }) t.Run("validate concurrent failed attempts to bypass checks", func(t *testing.T) { @@ -159,6 +217,66 @@ func TestCheckPasswordAndAllCriteria(t *testing.T) { }) } +func TestDoubleCheckPassword(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + const maxFailedLoginAttempts = 3 + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.MaximumLoginAttempts = maxFailedLoginAttempts + }) + + password := model.NewTestPassword() + appErr := th.App.UpdatePassword(th.Context, th.BasicUser, password) + require.Nil(t, appErr) + + // DoubleCheckPassword does not re-fetch the user; it inspects user.Password + // directly. Pull a fresh struct that reflects the hash we just wrote. + user, appErr := th.App.GetUser(th.BasicUser.Id) + require.Nil(t, appErr) + + t.Run("correct password succeeds and resets the counter", func(t *testing.T) { + err := th.App.Srv().Store().User().UpdateFailedPasswordAttempts(user.Id, maxFailedLoginAttempts-1) + require.NoError(t, err) + + appErr := th.App.DoubleCheckPassword(th.Context, user, password) + require.Nil(t, appErr) + + updatedUser, appErr := th.App.GetUser(user.Id) + require.Nil(t, appErr) + require.Equal(t, 0, updatedUser.FailedAttempts) + }) + + t.Run("rate limit is enforced once max attempts is reached", func(t *testing.T) { + err := th.App.Srv().Store().User().UpdateFailedPasswordAttempts(user.Id, maxFailedLoginAttempts) + require.NoError(t, err) + + appErr := th.App.DoubleCheckPassword(th.Context, user, password) + require.NotNil(t, appErr) + require.Equal(t, "api.user.check_user_login_attempts.too_many.app_error", appErr.Id) + }) + + t.Run("backend error refunds the slot", func(t *testing.T) { + badHashUser := th.CreateUser(t) + err := th.Server.Store().User().UpdatePassword(badHashUser.Id, "$pbkdf2$bogus") + require.NoError(t, err) + th.App.InvalidateCacheForUser(badHashUser.Id) + err = th.App.Srv().Store().User().UpdateFailedPasswordAttempts(badHashUser.Id, 0) + require.NoError(t, err) + + user, appErr := th.App.GetUser(badHashUser.Id) + require.Nil(t, appErr) + + appErr = th.App.DoubleCheckPassword(th.Context, user, "any-password") + require.NotNil(t, appErr) + require.Equal(t, "api.user.check_user_password.invalid_hash.app_error", appErr.Id) + + updatedUser, appErr := th.App.GetUser(badHashUser.Id) + require.Nil(t, appErr) + require.Equal(t, 0, updatedUser.FailedAttempts, "backend error must not consume a slot") + }) +} + func TestCheckLdapUserPasswordAndAllCriteria(t *testing.T) { th := SetupEnterprise(t).InitBasic(t) @@ -256,6 +374,172 @@ func TestCheckLdapUserPasswordAndAllCriteria(t *testing.T) { } }) } + + // The cases below cover paths the table loop above does not exercise: + // first-time LDAP users (user.Id == ""), LDAP backend errors that are + // not credential failures, and the MFA pre-flight probe refund. Each + // subtest builds its own mockLdap so expectations from previous + // subtests cannot match the wrong call. + + createLdapUserWithMFA := func(t *testing.T, emailLocal string) (*model.User, *string) { + t.Helper() + userAuthData := model.NewRandomString(32) + created, appErr := th.App.CreateUser(th.Context, &model.User{ + Email: emailLocal + "@mattermost-customer.com", + Username: emailLocal, + AuthService: model.UserAuthServiceLdap, + AuthData: &userAuthData, + EmailVerified: true, + }) + require.Nil(t, appErr) + secret, appErr := th.App.GenerateMfaSecret(created.Id) + require.Nil(t, appErr) + require.NoError(t, th.Server.Store().User().UpdateMfaActive(created.Id, true)) + require.NoError(t, th.Server.Store().User().UpdateMfaSecret(created.Id, secret.Secret)) + require.NoError(t, th.App.Srv().Store().User().UpdateFailedPasswordAttempts(created.Id, 0)) + created, appErr = th.App.GetUser(created.Id) + require.Nil(t, appErr) + created.AuthData = &userAuthData + return created, &userAuthData + } + + t.Run("first-time LDAP user with wrong password increments counter", func(t *testing.T) { + // DoLogin in production creates the row before reporting a bad + // password; we pre-create it here so GetUserByAuth can resolve it. + firstAuthData := model.NewRandomString(32) + preCreated, appErr := th.App.CreateUser(th.Context, &model.User{ + Email: "ldapuser-first-bad-pwd@mattermost-customer.com", + Username: "ldapuser-first-bad-pwd", + AuthService: model.UserAuthServiceLdap, + AuthData: &firstAuthData, + EmailVerified: true, + }) + require.Nil(t, appErr) + require.NoError(t, th.App.Srv().Store().User().UpdateFailedPasswordAttempts(preCreated.Id, 0)) + + freshMock := &mocks.LdapInterface{} + th.App.Channels().Ldap = freshMock + t.Cleanup(func() { th.App.Channels().Ldap = mockLdap }) + freshMock.Mock.On("DoLogin", th.Context, firstAuthData, wrongPassword).Return(nil, &model.AppError{Id: "ent.ldap.do_login.invalid_password.app_error"}) + + _, appErr = th.App.checkLdapUserPasswordAndAllCriteria(th.Context, &model.User{ + AuthService: model.UserAuthServiceLdap, + AuthData: &firstAuthData, + }, wrongPassword, "") + require.NotNil(t, appErr) + require.Equal(t, "ent.ldap.do_login.invalid_password.app_error", appErr.Id) + + updatedUser, appErr := th.App.GetUser(preCreated.Id) + require.Nil(t, appErr) + require.Equal(t, 1, updatedUser.FailedAttempts, "first-time LDAP wrong password must be counted") + }) + + t.Run("first-time LDAP user with wrong MFA token increments counter", func(t *testing.T) { + // DoLogin returns the freshly created user struct; the function + // then calls CheckUserMfa, which fails on a wrong non-empty token. + preCreated, authDataPtr := createLdapUserWithMFA(t, "ldapuser-first-bad-mfa") + + freshMock := &mocks.LdapInterface{} + th.App.Channels().Ldap = freshMock + t.Cleanup(func() { th.App.Channels().Ldap = mockLdap }) + freshMock.Mock.On("DoLogin", th.Context, *authDataPtr, validPassword).Return(preCreated, nil) + + _, appErr := th.App.checkLdapUserPasswordAndAllCriteria(th.Context, &model.User{ + AuthService: model.UserAuthServiceLdap, + AuthData: authDataPtr, + }, validPassword, "123456") + require.NotNil(t, appErr) + require.Equal(t, "api.user.check_user_mfa.bad_code.app_error", appErr.Id) + + updatedUser, appErr := th.App.GetUser(preCreated.Id) + require.Nil(t, appErr) + require.Equal(t, 1, updatedUser.FailedAttempts, "first-time LDAP wrong MFA must be counted") + }) + + t.Run("existing LDAP user with LDAP backend error refunds the slot", func(t *testing.T) { + // A non-credential LDAP error (server unreachable, transient + // failure) on an existing user must not consume the pre-claimed + // slot, or an LDAP outage could lock out everyone. + require.NoError(t, th.App.Srv().Store().User().UpdateFailedPasswordAttempts(user.Id, 0)) + + freshMock := &mocks.LdapInterface{} + th.App.Channels().Ldap = freshMock + t.Cleanup(func() { th.App.Channels().Ldap = mockLdap }) + freshMock.Mock.On("DoLogin", th.Context, authData, wrongPassword).Return(nil, &model.AppError{Id: "ent.ldap.do_login.unable_to_connect.app_error"}) + + _, appErr := th.App.checkLdapUserPasswordAndAllCriteria(th.Context, user, wrongPassword, "") + require.NotNil(t, appErr) + require.Equal(t, "ent.ldap.do_login.unable_to_connect.app_error", appErr.Id) + + updatedUser, appErr := th.App.GetUser(user.Id) + require.Nil(t, appErr) + require.Equal(t, 0, updatedUser.FailedAttempts, "LDAP backend error must refund the slot") + }) + + t.Run("existing LDAP user MFA pre-flight probe refunds the slot", func(t *testing.T) { + // Empty mfaToken on an MFA-enabled LDAP user is a probe; the slot + // the pre-claim consumed is refunded. + preCreated, authDataPtr := createLdapUserWithMFA(t, "ldapuser-existing-mfa-probe") + + freshMock := &mocks.LdapInterface{} + th.App.Channels().Ldap = freshMock + t.Cleanup(func() { th.App.Channels().Ldap = mockLdap }) + freshMock.Mock.On("DoLogin", th.Context, *authDataPtr, validPassword).Return(preCreated, nil) + + _, appErr := th.App.checkLdapUserPasswordAndAllCriteria(th.Context, preCreated, validPassword, "") + require.NotNil(t, appErr) + require.Equal(t, "mfa.validate_token.authenticate.app_error", appErr.Id) + + updatedUser, appErr := th.App.GetUser(preCreated.Id) + require.Nil(t, appErr) + require.Equal(t, 0, updatedUser.FailedAttempts, "MFA probe on existing LDAP user must not consume a slot") + }) + + t.Run("concurrent first-time LDAP wrong password caps at maxAttempts", func(t *testing.T) { + // A first-time LDAP user has no local row yet, so the slot is + // not pre-claimed. The fallback counter bump must use the atomic + // TryIncrement primitive: a previous implementation used an + // absolute UPDATE Users SET FailedAttempts = ldapUser.FailedAttempts + 1 + // based on an in-memory snapshot, which lost increments when + // concurrent first-attempt requests all read FailedAttempts = 0 + // and all wrote 1. Under the atomic primitive the counter caps + // at maxFailedLoginAttempts regardless of contention. + concurrentAuthData := model.NewRandomString(32) + preCreated, appErr := th.App.CreateUser(th.Context, &model.User{ + Email: "ldapuser-first-bad-pwd-conc@mattermost-customer.com", + Username: "ldapuser-first-bad-pwd-conc", + AuthService: model.UserAuthServiceLdap, + AuthData: &concurrentAuthData, + EmailVerified: true, + }) + require.Nil(t, appErr) + require.NoError(t, th.App.Srv().Store().User().UpdateFailedPasswordAttempts(preCreated.Id, 0)) + + freshMock := &mocks.LdapInterface{} + th.App.Channels().Ldap = freshMock + t.Cleanup(func() { th.App.Channels().Ldap = mockLdap }) + freshMock.Mock.On("DoLogin", th.Context, concurrentAuthData, wrongPassword).Return(nil, &model.AppError{Id: "ent.ldap.do_login.invalid_password.app_error"}) + + const goroutines = maxFailedLoginAttempts * 3 + var g errgroup.Group + start := make(chan struct{}) + for range goroutines { + g.Go(func() error { + <-start + _, _ = th.App.checkLdapUserPasswordAndAllCriteria(th.Context, &model.User{ + AuthService: model.UserAuthServiceLdap, + AuthData: &concurrentAuthData, + }, wrongPassword, "") + return nil + }) + } + close(start) + require.NoError(t, g.Wait()) + + updatedUser, appErr := th.App.GetUser(preCreated.Id) + require.Nil(t, appErr) + require.Equal(t, maxFailedLoginAttempts, updatedUser.FailedAttempts, "concurrent first-time attempts must not lose increments and must cap at maxAttempts") + }) } func TestCheckLdapUserPasswordConcurrency(t *testing.T) { @@ -400,13 +684,7 @@ func TestCheckUserPassword(t *testing.T) { t.Run("valid password with current hashing", func(t *testing.T) { user := createUserWithHash(pwdPBKDF2) - err := th.App.checkUserPassword(user, pwd, false) - require.Nil(t, err) - }) - - t.Run("valid password with current hashing and cache invalidation", func(t *testing.T) { - user := createUserWithHash(pwdPBKDF2) - err := th.App.checkUserPassword(user, pwd, true) + err := th.App.checkUserPassword(user, pwd) require.Nil(t, err) }) @@ -415,13 +693,9 @@ func TestCheckUserPassword(t *testing.T) { t.Run("invalid password", func(t *testing.T) { user := createUserWithHash(pwdPBKDF2) - err := th.App.checkUserPassword(user, wrongPassword, false) + err := th.App.checkUserPassword(user, wrongPassword) require.NotNil(t, err) require.Equal(t, "api.user.check_user_password.invalid.app_error", err.Id) - - updatedUser, err := th.App.GetUser(user.Id) - require.Nil(t, err) - require.Equal(t, user.FailedAttempts+1, updatedUser.FailedAttempts) }) t.Run("password migration from outdated hash", func(t *testing.T) { @@ -429,7 +703,7 @@ func TestCheckUserPassword(t *testing.T) { require.Contains(t, user.Password, "$2a$10") require.NotContains(t, user.Password, "pbkdf2") - err := th.App.checkUserPassword(user, pwd, false) + err := th.App.checkUserPassword(user, pwd) require.Nil(t, err) updatedUser, err := th.App.GetUser(user.Id) @@ -438,20 +712,16 @@ func TestCheckUserPassword(t *testing.T) { require.Contains(t, updatedUser.Password, "$pbkdf2") // Re-check with updated password - err = th.App.checkUserPassword(user, pwd, false) + err = th.App.checkUserPassword(updatedUser, pwd) require.Nil(t, err) }) t.Run("password migration fails with invalid password", func(t *testing.T) { user := createUserWithHash(pwdBcrypt) - err := th.App.checkUserPassword(user, wrongPassword, false) + err := th.App.checkUserPassword(user, wrongPassword) require.NotNil(t, err) require.Equal(t, "api.user.check_user_password.invalid.app_error", err.Id) - - updatedUser, err := th.App.GetUser(user.Id) - require.Nil(t, err) - require.Equal(t, user.FailedAttempts+1, updatedUser.FailedAttempts) }) t.Run("empty password", func(t *testing.T) { @@ -460,7 +730,7 @@ func TestCheckUserPassword(t *testing.T) { user, err := th.App.GetUser(user.Id) require.Nil(t, err) - err = th.App.checkUserPassword(user, "", false) + err = th.App.checkUserPassword(user, "") require.NotNil(t, err) require.Equal(t, "api.user.check_user_password.invalid.app_error", err.Id) }) @@ -471,7 +741,7 @@ func TestCheckUserPassword(t *testing.T) { user, err := th.App.GetUser(user.Id) require.Nil(t, err) - err = th.App.checkUserPassword(user, pwd, false) + err = th.App.checkUserPassword(user, pwd) require.NotNil(t, err) require.Equal(t, "api.user.check_user_password.invalid.app_error", err.Id) }) @@ -489,7 +759,7 @@ func TestCheckUserPassword(t *testing.T) { // The user hash contains the old parameter require.Contains(t, user.Password, "w=10000") - appErr := th.App.checkUserPassword(user, pwd, false) + appErr := th.App.checkUserPassword(user, pwd) require.Nil(t, appErr) updatedUser, appErr := th.App.GetUser(user.Id) @@ -500,7 +770,7 @@ func TestCheckUserPassword(t *testing.T) { require.NotContains(t, updatedUser.Password, "w=10000") // Re-check with updated password - appErr = th.App.checkUserPassword(user, pwd, false) + appErr = th.App.checkUserPassword(updatedUser, pwd) require.Nil(t, appErr) }) } @@ -542,7 +812,7 @@ func TestMigratePassword(t *testing.T) { require.Contains(t, updatedUser.Password, "$pbkdf2") // Re-check with updated password - err = th.App.checkUserPassword(user, pwd, false) + err = th.App.checkUserPassword(updatedUser, pwd) require.Nil(t, err) }) } diff --git a/server/channels/app/channels.go b/server/channels/app/channels.go index df3e90c2e85..eaa21d4ccd3 100644 --- a/server/channels/app/channels.go +++ b/server/channels/app/channels.go @@ -92,11 +92,9 @@ type Channels struct { postReminderMut sync.Mutex postReminderTask *model.ScheduledTask - interruptQuitChan chan struct{} - scheduledPostMut sync.Mutex - scheduledPostTask *model.ScheduledTask - emailLoginAttemptsMut sync.Mutex - ldapLoginAttemptsMut sync.Mutex + interruptQuitChan chan struct{} + scheduledPostMut sync.Mutex + scheduledPostTask *model.ScheduledTask } func NewChannels(s *Server) (*Channels, error) { diff --git a/server/channels/store/localcachelayer/user_layer.go b/server/channels/store/localcachelayer/user_layer.go index f9c6c182a61..4132007705e 100644 --- a/server/channels/store/localcachelayer/user_layer.go +++ b/server/channels/store/localcachelayer/user_layer.go @@ -222,6 +222,25 @@ func (s *LocalCacheUserStore) UpdateFailedPasswordAttempts(userID string, attemp return s.UserStore.UpdateFailedPasswordAttempts(userID, attempts) } +func (s *LocalCacheUserStore) TryIncrementFailedPasswordAttempts(userID string, maxAttempts int) (bool, error) { + claimed, err := s.UserStore.TryIncrementFailedPasswordAttempts(userID, maxAttempts) + if err != nil { + return false, err + } + if claimed { + s.InvalidateProfileCacheForUser(userID) + } + return claimed, nil +} + +func (s *LocalCacheUserStore) DecrementFailedPasswordAttempts(userID string) error { + if err := s.UserStore.DecrementFailedPasswordAttempts(userID); err != nil { + return err + } + s.InvalidateProfileCacheForUser(userID) + return nil +} + // 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 diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index 938cd91d1c7..18e3ca33dc4 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -17413,6 +17413,48 @@ func (s *RetryLayerUserStore) UpdateFailedPasswordAttempts(userID string, attemp } +func (s *RetryLayerUserStore) TryIncrementFailedPasswordAttempts(userID string, maxAttempts int) (bool, error) { + + tries := 0 + for { + result, err := s.UserStore.TryIncrementFailedPasswordAttempts(userID, maxAttempts) + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + +func (s *RetryLayerUserStore) DecrementFailedPasswordAttempts(userID string) error { + + tries := 0 + for { + err := s.UserStore.DecrementFailedPasswordAttempts(userID) + if err == nil { + return nil + } + if !isRepeatableError(err) { + return err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerUserStore) UpdateLastLogin(userID string, lastLogin int64) error { tries := 0 diff --git a/server/channels/store/sqlstore/user_store.go b/server/channels/store/sqlstore/user_store.go index 524bc917373..cfdfb2c4d3a 100644 --- a/server/channels/store/sqlstore/user_store.go +++ b/server/channels/store/sqlstore/user_store.go @@ -426,6 +426,45 @@ func (us SqlUserStore) UpdateFailedPasswordAttempts(userId string, attempts int) return nil } +// TryIncrementFailedPasswordAttempts atomically increments FailedAttempts by one +// for the given user, only if FailedAttempts is strictly less than maxAttempts. +// Returns true if the row was updated (a slot was claimed), false if the cap had +// already been reached (or the user does not exist). The row lock taken by the +// UPDATE serializes concurrent attempts on the same user, so the cap predicate +// is enforced without any application-level locking. +func (us SqlUserStore) TryIncrementFailedPasswordAttempts(userId string, maxAttempts int) (bool, error) { + res, err := us.GetMaster().Exec( + "UPDATE Users SET FailedAttempts = FailedAttempts + 1 WHERE Id = ? AND FailedAttempts < ?", + userId, maxAttempts, + ) + if err != nil { + return false, errors.Wrapf(err, "failed to update User with userId=%s", userId) + } + + rows, err := res.RowsAffected() + if err != nil { + return false, errors.Wrapf(err, "failed to read rows affected for userId=%s", userId) + } + + return rows == 1, nil +} + +// DecrementFailedPasswordAttempts atomically decrements FailedAttempts by one +// for the given user, only if FailedAttempts is strictly greater than zero. It +// is used to refund a slot previously claimed by TryIncrementFailedPasswordAttempts +// when the in-flight authentication turns out not to be a credential-failure +// event (e.g. a backend error or an MFA pre-flight probe). +func (us SqlUserStore) DecrementFailedPasswordAttempts(userId string) error { + _, err := us.GetMaster().Exec( + "UPDATE Users SET FailedAttempts = FailedAttempts - 1 WHERE Id = ? AND FailedAttempts > 0", + userId, + ) + if err != nil { + return errors.Wrapf(err, "failed to update User with userId=%s", userId) + } + return nil +} + func (us SqlUserStore) UpdateAuthData(userId string, service string, authData *string, email string, resetMfa bool) (string, error) { updateAt := model.GetMillis() diff --git a/server/channels/store/sqlstore/webhook_store.go b/server/channels/store/sqlstore/webhook_store.go index d0fc10b122c..4a7857f878f 100644 --- a/server/channels/store/sqlstore/webhook_store.go +++ b/server/channels/store/sqlstore/webhook_store.go @@ -166,6 +166,7 @@ func (s SqlWebhookStore) GetIncomingListByUser(userId string, offset, limit int) query := s.incomingWebhookSelectQuery. Where(sq.Eq{"DeleteAt": 0}). + OrderBy("DisplayName", "Id"). Limit(uint64(limit)). Offset(uint64(offset)) @@ -188,6 +189,7 @@ func (s SqlWebhookStore) GetIncomingByTeamByUser(teamId string, userId string, o sq.Eq{"TeamId": teamId}, sq.Eq{"DeleteAt": 0}, }). + OrderBy("DisplayName", "Id"). Limit(uint64(limit)). Offset(uint64(offset)) @@ -271,6 +273,7 @@ func (s SqlWebhookStore) GetOutgoingListByUser(userId string, offset, limit int) Where(sq.And{ sq.Eq{"DeleteAt": 0}, }). + OrderBy("DisplayName", "Id"). Limit(uint64(limit)). Offset(uint64(offset)) @@ -296,7 +299,8 @@ func (s SqlWebhookStore) GetOutgoingByChannelByUser(channelId string, userId str Where(sq.And{ sq.Eq{"ChannelId": channelId}, sq.Eq{"DeleteAt": 0}, - }) + }). + OrderBy("DisplayName", "Id") if userId != "" { query = query.Where(sq.Eq{"CreatorId": userId}) @@ -323,7 +327,8 @@ func (s SqlWebhookStore) GetOutgoingByTeamByUser(teamId string, userId string, o Where(sq.And{ sq.Eq{"TeamId": teamId}, sq.Eq{"DeleteAt": 0}, - }) + }). + OrderBy("DisplayName", "Id") if userId != "" { query = query.Where(sq.Eq{"CreatorId": userId}) diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 596af20bbeb..c93eb8a2725 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -482,6 +482,8 @@ type UserStore interface { GetEtagForAllProfiles() string GetEtagForProfiles(teamID string) string UpdateFailedPasswordAttempts(userID string, attempts int) error + TryIncrementFailedPasswordAttempts(userID string, maxAttempts int) (bool, error) + DecrementFailedPasswordAttempts(userID string) error GetSystemAdminProfiles() (map[string]*model.User, error) PermanentDelete(rctx request.CTX, userID string) error AnalyticsActiveCount(timestamp int64, options model.UserCountOptions) (int64, error) diff --git a/server/channels/store/storetest/mocks/UserStore.go b/server/channels/store/storetest/mocks/UserStore.go index 12026e7f4b6..dda8cf0571b 100644 --- a/server/channels/store/storetest/mocks/UserStore.go +++ b/server/channels/store/storetest/mocks/UserStore.go @@ -2156,6 +2156,52 @@ func (_m *UserStore) UpdateFailedPasswordAttempts(userID string, attempts int) e return r0 } +// TryIncrementFailedPasswordAttempts provides a mock function with given fields: userID, maxAttempts +func (_m *UserStore) TryIncrementFailedPasswordAttempts(userID string, maxAttempts int) (bool, error) { + ret := _m.Called(userID, maxAttempts) + + if len(ret) == 0 { + panic("no return value specified for TryIncrementFailedPasswordAttempts") + } + + var r0 bool + var r1 error + if rf, ok := ret.Get(0).(func(string, int) (bool, error)); ok { + return rf(userID, maxAttempts) + } + if rf, ok := ret.Get(0).(func(string, int) bool); ok { + r0 = rf(userID, maxAttempts) + } else { + r0 = ret.Get(0).(bool) + } + + if rf, ok := ret.Get(1).(func(string, int) error); ok { + r1 = rf(userID, maxAttempts) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// DecrementFailedPasswordAttempts provides a mock function with given fields: userID +func (_m *UserStore) DecrementFailedPasswordAttempts(userID string) error { + ret := _m.Called(userID) + + if len(ret) == 0 { + panic("no return value specified for DecrementFailedPasswordAttempts") + } + + var r0 error + if rf, ok := ret.Get(0).(func(string) error); ok { + r0 = rf(userID) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // UpdateLastLogin provides a mock function with given fields: userID, lastLogin func (_m *UserStore) UpdateLastLogin(userID string, lastLogin int64) error { ret := _m.Called(userID, lastLogin) diff --git a/server/channels/store/storetest/user_store.go b/server/channels/store/storetest/user_store.go index da4ef47f1a2..7520267b39f 100644 --- a/server/channels/store/storetest/user_store.go +++ b/server/channels/store/storetest/user_store.go @@ -9,11 +9,13 @@ import ( "fmt" "sort" "strings" + "sync/atomic" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/shared/request" @@ -54,6 +56,8 @@ func TestUserStore(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore) { t.Run("Update", func(t *testing.T) { testUserStoreUpdate(t, rctx, ss) }) t.Run("UpdateUpdateAt", func(t *testing.T) { testUserStoreUpdateUpdateAt(t, rctx, ss) }) t.Run("UpdateFailedPasswordAttempts", func(t *testing.T) { testUserStoreUpdateFailedPasswordAttempts(t, rctx, ss) }) + t.Run("TryIncrementFailedPasswordAttempts", func(t *testing.T) { testUserStoreTryIncrementFailedPasswordAttempts(t, rctx, ss) }) + t.Run("DecrementFailedPasswordAttempts", func(t *testing.T) { testUserStoreDecrementFailedPasswordAttempts(t, rctx, ss) }) t.Run("Get", func(t *testing.T) { testUserStoreGet(t, rctx, ss) }) t.Run("GetAllUsingAuthService", func(t *testing.T) { testGetAllUsingAuthService(t, rctx, ss) }) t.Run("GetAllProfiles", func(t *testing.T) { testUserStoreGetAllProfiles(t, rctx, ss) }) @@ -350,6 +354,145 @@ func testUserStoreUpdateFailedPasswordAttempts(t *testing.T, rctx request.CTX, s require.Equal(t, 3, user.FailedAttempts, "FailedAttempts not updated correctly") } +func testUserStoreTryIncrementFailedPasswordAttempts(t *testing.T, rctx request.CTX, ss store.Store) { + u1 := &model.User{} + u1.Email = MakeEmail() + _, err := ss.User().Save(rctx, u1) + require.NoError(t, err) + defer func() { require.NoError(t, ss.User().PermanentDelete(rctx, u1.Id)) }() + _, nErr := ss.Team().SaveMember(rctx, &model.TeamMember{TeamId: model.NewId(), UserId: u1.Id}, -1) + require.NoError(t, nErr) + + const maxAttempts = 3 + + t.Run("claims a slot when below cap", func(t *testing.T) { + require.NoError(t, ss.User().UpdateFailedPasswordAttempts(u1.Id, 0)) + + claimed, err := ss.User().TryIncrementFailedPasswordAttempts(u1.Id, maxAttempts) + require.NoError(t, err) + require.True(t, claimed) + + user, err := ss.User().Get(context.Background(), u1.Id) + require.NoError(t, err) + require.Equal(t, 1, user.FailedAttempts) + }) + + t.Run("does not claim a slot when at cap", func(t *testing.T) { + require.NoError(t, ss.User().UpdateFailedPasswordAttempts(u1.Id, maxAttempts)) + + claimed, err := ss.User().TryIncrementFailedPasswordAttempts(u1.Id, maxAttempts) + require.NoError(t, err) + require.False(t, claimed) + + user, err := ss.User().Get(context.Background(), u1.Id) + require.NoError(t, err) + require.Equal(t, maxAttempts, user.FailedAttempts, "counter must not advance past the cap") + }) + + t.Run("does not claim a slot when above cap", func(t *testing.T) { + require.NoError(t, ss.User().UpdateFailedPasswordAttempts(u1.Id, maxAttempts+5)) + + claimed, err := ss.User().TryIncrementFailedPasswordAttempts(u1.Id, maxAttempts) + require.NoError(t, err) + require.False(t, claimed) + + user, err := ss.User().Get(context.Background(), u1.Id) + require.NoError(t, err) + require.Equal(t, maxAttempts+5, user.FailedAttempts) + }) + + t.Run("does not claim a slot for unknown user", func(t *testing.T) { + claimed, err := ss.User().TryIncrementFailedPasswordAttempts(model.NewId(), maxAttempts) + require.NoError(t, err) + require.False(t, claimed) + }) + + t.Run("concurrent attempts cap at maxAttempts", func(t *testing.T) { + require.NoError(t, ss.User().UpdateFailedPasswordAttempts(u1.Id, 0)) + + const goroutines = 50 + var g errgroup.Group + var claimed atomic.Int64 + start := make(chan struct{}) + for range goroutines { + g.Go(func() error { + <-start + ok, err := ss.User().TryIncrementFailedPasswordAttempts(u1.Id, maxAttempts) + if err != nil { + return err + } + if ok { + claimed.Add(1) + } + return nil + }) + } + close(start) + require.NoError(t, g.Wait()) + + require.Equal(t, int64(maxAttempts), claimed.Load(), "exactly maxAttempts goroutines must have claimed a slot") + + user, err := ss.User().Get(context.Background(), u1.Id) + require.NoError(t, err) + require.Equal(t, maxAttempts, user.FailedAttempts) + }) +} + +func testUserStoreDecrementFailedPasswordAttempts(t *testing.T, rctx request.CTX, ss store.Store) { + u1 := &model.User{} + u1.Email = MakeEmail() + _, err := ss.User().Save(rctx, u1) + require.NoError(t, err) + defer func() { require.NoError(t, ss.User().PermanentDelete(rctx, u1.Id)) }() + _, nErr := ss.Team().SaveMember(rctx, &model.TeamMember{TeamId: model.NewId(), UserId: u1.Id}, -1) + require.NoError(t, nErr) + + t.Run("decrements when above zero", func(t *testing.T) { + require.NoError(t, ss.User().UpdateFailedPasswordAttempts(u1.Id, 3)) + + require.NoError(t, ss.User().DecrementFailedPasswordAttempts(u1.Id)) + + user, err := ss.User().Get(context.Background(), u1.Id) + require.NoError(t, err) + require.Equal(t, 2, user.FailedAttempts) + }) + + t.Run("does not go below zero", func(t *testing.T) { + require.NoError(t, ss.User().UpdateFailedPasswordAttempts(u1.Id, 0)) + + require.NoError(t, ss.User().DecrementFailedPasswordAttempts(u1.Id)) + + user, err := ss.User().Get(context.Background(), u1.Id) + require.NoError(t, err) + require.Equal(t, 0, user.FailedAttempts) + }) + + t.Run("no-op for unknown user", func(t *testing.T) { + require.NoError(t, ss.User().DecrementFailedPasswordAttempts(model.NewId())) + }) + + t.Run("concurrent decrements never go below zero", func(t *testing.T) { + const initial = 10 + const goroutines = 50 + require.NoError(t, ss.User().UpdateFailedPasswordAttempts(u1.Id, initial)) + + var g errgroup.Group + start := make(chan struct{}) + for range goroutines { + g.Go(func() error { + <-start + return ss.User().DecrementFailedPasswordAttempts(u1.Id) + }) + } + close(start) + require.NoError(t, g.Wait()) + + user, err := ss.User().Get(context.Background(), u1.Id) + require.NoError(t, err) + require.Equal(t, 0, user.FailedAttempts, "decrement must clamp at zero under contention") + }) +} + func testUserStoreGet(t *testing.T, rctx request.CTX, ss store.Store) { u1 := &model.User{ Email: MakeEmail(), diff --git a/server/channels/store/storetest/webhook_store.go b/server/channels/store/storetest/webhook_store.go index 6de9ddbb1a8..a0884698bce 100644 --- a/server/channels/store/storetest/webhook_store.go +++ b/server/channels/store/storetest/webhook_store.go @@ -132,8 +132,8 @@ func testWebhookStoreGetIncomingListByUser(t *testing.T, rctx request.CTX, ss st o1.UserId = model.NewId() o1.TeamId = model.NewId() - o1, err := ss.Webhook().SaveIncoming(o1) - require.NoError(t, err) + o1, errSave := ss.Webhook().SaveIncoming(o1) + require.NoError(t, errSave) t.Run("GetIncomingListByUser, known user filtered", func(t *testing.T) { hooks, err := ss.Webhook().GetIncomingListByUser(o1.UserId, 0, 100) @@ -147,6 +147,27 @@ func testWebhookStoreGetIncomingListByUser(t *testing.T, rctx request.CTX, ss st require.NoError(t, err) require.Equal(t, 0, len(hooks)) }) + + t.Run("GetIncomingListByUser, ordered alphabetically by display name", func(t *testing.T) { + userId := model.NewId() + hookC := &model.IncomingWebhook{ChannelId: model.NewId(), UserId: userId, TeamId: model.NewId(), DisplayName: "Charlie"} + hookA := &model.IncomingWebhook{ChannelId: model.NewId(), UserId: userId, TeamId: model.NewId(), DisplayName: "Alpha"} + hookB := &model.IncomingWebhook{ChannelId: model.NewId(), UserId: userId, TeamId: model.NewId(), DisplayName: "Bravo"} + + hookC, err := ss.Webhook().SaveIncoming(hookC) + require.NoError(t, err) + hookA, err = ss.Webhook().SaveIncoming(hookA) + require.NoError(t, err) + hookB, err = ss.Webhook().SaveIncoming(hookB) + require.NoError(t, err) + + hooks, err := ss.Webhook().GetIncomingListByUser(userId, 0, 100) + require.NoError(t, err) + require.Len(t, hooks, 3) + require.Equal(t, hookA.Id, hooks[0].Id, "first result should be Alpha (alphabetical order)") + require.Equal(t, hookB.Id, hooks[1].Id, "second result should be Bravo (alphabetical order)") + require.Equal(t, hookC.Id, hooks[2].Id, "third result should be Charlie (alphabetical order)") + }) } func testWebhookStoreGetIncomingByTeam(t *testing.T, rctx request.CTX, ss store.Store) { @@ -166,16 +187,14 @@ func testWebhookStoreGetIncomingByTeam(t *testing.T, rctx request.CTX, ss store. } func TestWebhookStoreGetIncomingByTeamByUser(t *testing.T, rctx request.CTX, ss store.Store) { - var err error - o1 := buildIncomingWebhook() - o1, err = ss.Webhook().SaveIncoming(o1) - require.NoError(t, err) + o1, errSave := ss.Webhook().SaveIncoming(o1) + require.NoError(t, errSave) o2 := buildIncomingWebhook() o2.TeamId = o1.TeamId //Set both to the same team - o2, err = ss.Webhook().SaveIncoming(o2) - require.NoError(t, err) + o2, errSave = ss.Webhook().SaveIncoming(o2) + require.NoError(t, errSave) t.Run("GetIncomingByTeamByUser, no user filter", func(t *testing.T) { hooks, err := ss.Webhook().GetIncomingByTeam(o1.TeamId, 0, 100) @@ -195,6 +214,28 @@ func TestWebhookStoreGetIncomingByTeamByUser(t *testing.T, rctx request.CTX, ss require.NoError(t, err) require.Equal(t, len(hooks), 0) }) + + t.Run("GetIncomingByTeamByUser, ordered alphabetically by display name", func(t *testing.T) { + teamId := model.NewId() + userId := model.NewId() + hookC := &model.IncomingWebhook{ChannelId: model.NewId(), UserId: userId, TeamId: teamId, DisplayName: "Charlie"} + hookA := &model.IncomingWebhook{ChannelId: model.NewId(), UserId: userId, TeamId: teamId, DisplayName: "Alpha"} + hookB := &model.IncomingWebhook{ChannelId: model.NewId(), UserId: userId, TeamId: teamId, DisplayName: "Bravo"} + + hookC, err := ss.Webhook().SaveIncoming(hookC) + require.NoError(t, err) + hookA, err = ss.Webhook().SaveIncoming(hookA) + require.NoError(t, err) + hookB, err = ss.Webhook().SaveIncoming(hookB) + require.NoError(t, err) + + hooks, err := ss.Webhook().GetIncomingByTeamByUser(teamId, userId, 0, 100) + require.NoError(t, err) + require.Len(t, hooks, 3) + require.Equal(t, hookA.Id, hooks[0].Id, "first result should be Alpha (alphabetical order)") + require.Equal(t, hookB.Id, hooks[1].Id, "second result should be Bravo (alphabetical order)") + require.Equal(t, hookC.Id, hooks[2].Id, "third result should be Charlie (alphabetical order)") + }) } func testWebhookStoreGetIncomingByChannel(t *testing.T, rctx request.CTX, ss store.Store) { @@ -332,6 +373,27 @@ func testWebhookStoreGetOutgoingListByUser(t *testing.T, rctx request.CTX, ss st require.NoError(t, err) require.Equal(t, 0, len(hooks)) }) + + t.Run("GetOutgoingListByUser, ordered alphabetically by display name", func(t *testing.T) { + creatorId := model.NewId() + hookC := &model.OutgoingWebhook{ChannelId: model.NewId(), CreatorId: creatorId, TeamId: model.NewId(), CallbackURLs: []string{"http://nowhere.com/"}, DisplayName: "Charlie"} + hookA := &model.OutgoingWebhook{ChannelId: model.NewId(), CreatorId: creatorId, TeamId: model.NewId(), CallbackURLs: []string{"http://nowhere.com/"}, DisplayName: "Alpha"} + hookB := &model.OutgoingWebhook{ChannelId: model.NewId(), CreatorId: creatorId, TeamId: model.NewId(), CallbackURLs: []string{"http://nowhere.com/"}, DisplayName: "Bravo"} + + hookC, err := ss.Webhook().SaveOutgoing(hookC) + require.NoError(t, err) + hookA, err = ss.Webhook().SaveOutgoing(hookA) + require.NoError(t, err) + hookB, err = ss.Webhook().SaveOutgoing(hookB) + require.NoError(t, err) + + hooks, err := ss.Webhook().GetOutgoingListByUser(creatorId, 0, 100) + require.NoError(t, err) + require.Len(t, hooks, 3) + require.Equal(t, hookA.Id, hooks[0].Id, "first result should be Alpha (alphabetical order)") + require.Equal(t, hookB.Id, hooks[1].Id, "second result should be Bravo (alphabetical order)") + require.Equal(t, hookC.Id, hooks[2].Id, "third result should be Charlie (alphabetical order)") + }) } func testWebhookStoreGetOutgoingList(t *testing.T, rctx request.CTX, ss store.Store) { @@ -400,8 +462,8 @@ func testWebhookStoreGetOutgoingByChannelByUser(t *testing.T, rctx request.CTX, o1.TeamId = model.NewId() o1.CallbackURLs = []string{"http://nowhere.com/"} - o1, err := ss.Webhook().SaveOutgoing(o1) - require.NoError(t, err) + o1, errSave := ss.Webhook().SaveOutgoing(o1) + require.NoError(t, errSave) o2 := &model.OutgoingWebhook{} o2.ChannelId = o1.ChannelId @@ -409,8 +471,8 @@ func testWebhookStoreGetOutgoingByChannelByUser(t *testing.T, rctx request.CTX, o2.TeamId = model.NewId() o2.CallbackURLs = []string{"http://nowhere.com/"} - _, err = ss.Webhook().SaveOutgoing(o2) - require.NoError(t, err) + _, errSave = ss.Webhook().SaveOutgoing(o2) + require.NoError(t, errSave) t.Run("GetOutgoingByChannelByUser, no user filter", func(t *testing.T) { hooks, err := ss.Webhook().GetOutgoingByChannel(o1.ChannelId, 0, 100) @@ -430,6 +492,27 @@ func testWebhookStoreGetOutgoingByChannelByUser(t *testing.T, rctx request.CTX, require.NoError(t, err) require.Equal(t, 0, len(hooks)) }) + + t.Run("GetOutgoingByChannelByUser, ordered alphabetically by display name", func(t *testing.T) { + channelId := model.NewId() + hookC := &model.OutgoingWebhook{ChannelId: channelId, CreatorId: model.NewId(), TeamId: model.NewId(), CallbackURLs: []string{"http://nowhere.com/"}, DisplayName: "Charlie"} + hookA := &model.OutgoingWebhook{ChannelId: channelId, CreatorId: model.NewId(), TeamId: model.NewId(), CallbackURLs: []string{"http://nowhere.com/"}, DisplayName: "Alpha"} + hookB := &model.OutgoingWebhook{ChannelId: channelId, CreatorId: model.NewId(), TeamId: model.NewId(), CallbackURLs: []string{"http://nowhere.com/"}, DisplayName: "Bravo"} + + hookC, err := ss.Webhook().SaveOutgoing(hookC) + require.NoError(t, err) + hookA, err = ss.Webhook().SaveOutgoing(hookA) + require.NoError(t, err) + hookB, err = ss.Webhook().SaveOutgoing(hookB) + require.NoError(t, err) + + hooks, err := ss.Webhook().GetOutgoingByChannel(channelId, 0, 100) + require.NoError(t, err) + require.Len(t, hooks, 3) + require.Equal(t, hookA.Id, hooks[0].Id, "first result should be Alpha (alphabetical order)") + require.Equal(t, hookB.Id, hooks[1].Id, "second result should be Bravo (alphabetical order)") + require.Equal(t, hookC.Id, hooks[2].Id, "third result should be Charlie (alphabetical order)") + }) } func testWebhookStoreGetOutgoingByTeam(t *testing.T, rctx request.CTX, ss store.Store) { @@ -451,16 +534,14 @@ func testWebhookStoreGetOutgoingByTeam(t *testing.T, rctx request.CTX, ss store. } func testWebhookStoreGetOutgoingByTeamByUser(t *testing.T, rctx request.CTX, ss store.Store) { - var err error - o1 := &model.OutgoingWebhook{} o1.ChannelId = model.NewId() o1.CreatorId = model.NewId() o1.TeamId = model.NewId() o1.CallbackURLs = []string{"http://nowhere.com/"} - o1, err = ss.Webhook().SaveOutgoing(o1) - require.NoError(t, err) + o1, errSave := ss.Webhook().SaveOutgoing(o1) + require.NoError(t, errSave) o2 := &model.OutgoingWebhook{} o2.ChannelId = model.NewId() @@ -468,8 +549,8 @@ func testWebhookStoreGetOutgoingByTeamByUser(t *testing.T, rctx request.CTX, ss o2.TeamId = o1.TeamId o2.CallbackURLs = []string{"http://nowhere.com/"} - o2, err = ss.Webhook().SaveOutgoing(o2) - require.NoError(t, err) + o2, errSave = ss.Webhook().SaveOutgoing(o2) + require.NoError(t, errSave) t.Run("GetOutgoingByTeamByUser, no user filter", func(t *testing.T) { hooks, err := ss.Webhook().GetOutgoingByTeam(o1.TeamId, 0, 100) @@ -489,6 +570,28 @@ func testWebhookStoreGetOutgoingByTeamByUser(t *testing.T, rctx request.CTX, ss require.NoError(t, err) require.Equal(t, len(hooks), 0) }) + + t.Run("GetOutgoingByTeamByUser, ordered alphabetically by display name", func(t *testing.T) { + teamId := model.NewId() + creatorId := model.NewId() + hookC := &model.OutgoingWebhook{ChannelId: model.NewId(), CreatorId: creatorId, TeamId: teamId, CallbackURLs: []string{"http://nowhere.com/"}, DisplayName: "Charlie"} + hookA := &model.OutgoingWebhook{ChannelId: model.NewId(), CreatorId: creatorId, TeamId: teamId, CallbackURLs: []string{"http://nowhere.com/"}, DisplayName: "Alpha"} + hookB := &model.OutgoingWebhook{ChannelId: model.NewId(), CreatorId: creatorId, TeamId: teamId, CallbackURLs: []string{"http://nowhere.com/"}, DisplayName: "Bravo"} + + hookC, err := ss.Webhook().SaveOutgoing(hookC) + require.NoError(t, err) + hookA, err = ss.Webhook().SaveOutgoing(hookA) + require.NoError(t, err) + hookB, err = ss.Webhook().SaveOutgoing(hookB) + require.NoError(t, err) + + hooks, err := ss.Webhook().GetOutgoingByTeamByUser(teamId, creatorId, 0, 100) + require.NoError(t, err) + require.Len(t, hooks, 3) + require.Equal(t, hookA.Id, hooks[0].Id, "first result should be Alpha (alphabetical order)") + require.Equal(t, hookB.Id, hooks[1].Id, "second result should be Bravo (alphabetical order)") + require.Equal(t, hookC.Id, hooks[2].Id, "third result should be Charlie (alphabetical order)") + }) } func testWebhookStoreDeleteOutgoing(t *testing.T, rctx request.CTX, ss store.Store) { diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index b1b4e1ed635..6fa6f706031 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -13773,6 +13773,38 @@ func (s *TimerLayerUserStore) UpdateFailedPasswordAttempts(userID string, attemp return err } +func (s *TimerLayerUserStore) TryIncrementFailedPasswordAttempts(userID string, maxAttempts int) (bool, error) { + start := time.Now() + + result, err := s.UserStore.TryIncrementFailedPasswordAttempts(userID, maxAttempts) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("UserStore.TryIncrementFailedPasswordAttempts", success, elapsed) + } + return result, err +} + +func (s *TimerLayerUserStore) DecrementFailedPasswordAttempts(userID string) error { + start := time.Now() + + err := s.UserStore.DecrementFailedPasswordAttempts(userID) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("UserStore.DecrementFailedPasswordAttempts", success, elapsed) + } + return err +} + func (s *TimerLayerUserStore) UpdateLastLogin(userID string, lastLogin int64) error { start := time.Now() diff --git a/webapp/channels/src/actions/command.ts b/webapp/channels/src/actions/command.ts index 41a1b5f746e..3fb49056071 100644 --- a/webapp/channels/src/actions/command.ts +++ b/webapp/channels/src/actions/command.ts @@ -81,7 +81,7 @@ export function executeCommand(message: string, args: CommandArgs): ActionFuncAs if (!channel) { return {data: {silentFailureReason: new Error('cannot find current channel')}}; } - if (channel.type === Constants.PRIVATE_CHANNEL) { + if (channel.type === Constants.PRIVATE_CHANNEL || channel.policy_enforced) { dispatch(openModal({modalId: ModalIdentifiers.LEAVE_PRIVATE_CHANNEL_MODAL, dialogType: LeaveChannelModal, dialogProps: {channel}})); return {data: {frontendHandled: true}}; } diff --git a/webapp/channels/src/components/channel_header_menu/menu_items/leave_channel.test.tsx b/webapp/channels/src/components/channel_header_menu/menu_items/leave_channel.test.tsx index 7f14137c08e..17b3d9a2a01 100644 --- a/webapp/channels/src/components/channel_header_menu/menu_items/leave_channel.test.tsx +++ b/webapp/channels/src/components/channel_header_menu/menu_items/leave_channel.test.tsx @@ -66,4 +66,28 @@ describe('components/ChannelHeaderMenu/MenuItems/LeaveChannelTest', () => { }, }); }); + + test('opens leave confirmation modal for a policy enforced public channel', async () => { + const channel = TestHelper.getChannelMock({type: 'O', policy_enforced: true}); + + renderWithContext( + + + , {}, + ); + + const menuItem = screen.getByText('Leave Channel'); + expect(menuItem).toBeInTheDocument(); + + await userEvent.click(menuItem); + expect(channelActions.leaveChannel).not.toHaveBeenCalled(); + expect(modalActions.openModal).toHaveBeenCalledTimes(1); + expect(modalActions.openModal).toHaveBeenCalledWith({ + modalId: ModalIdentifiers.LEAVE_PRIVATE_CHANNEL_MODAL, + dialogType: LeaveChannelModal, + dialogProps: { + channel, + }, + }); + }); }); diff --git a/webapp/channels/src/components/channel_header_menu/menu_items/leave_channel.tsx b/webapp/channels/src/components/channel_header_menu/menu_items/leave_channel.tsx index 7dc78b001b7..3b1c3010d57 100644 --- a/webapp/channels/src/components/channel_header_menu/menu_items/leave_channel.tsx +++ b/webapp/channels/src/components/channel_header_menu/menu_items/leave_channel.tsx @@ -29,7 +29,7 @@ const LeaveChannel = ({ }: Props) => { const dispatch = useDispatch(); const handleLeave = () => { - if (channel.type === Constants.PRIVATE_CHANNEL) { + if (channel.type === Constants.PRIVATE_CHANNEL || channel.policy_enforced) { dispatch( openModal({ modalId: ModalIdentifiers.LEAVE_PRIVATE_CHANNEL_MODAL, diff --git a/webapp/channels/src/components/integrations/installed_incoming_webhooks/installed_incoming_webhooks.test.tsx b/webapp/channels/src/components/integrations/installed_incoming_webhooks/installed_incoming_webhooks.test.tsx new file mode 100644 index 00000000000..b0ac74c7d8a --- /dev/null +++ b/webapp/channels/src/components/integrations/installed_incoming_webhooks/installed_incoming_webhooks.test.tsx @@ -0,0 +1,154 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {screen, waitFor} from '@testing-library/react'; +import React from 'react'; + +import type {IncomingWebhook} from '@mattermost/types/integrations'; + +import InstalledIncomingWebhooks from 'components/integrations/installed_incoming_webhooks/installed_incoming_webhooks'; + +import {renderWithContext} from 'tests/react_testing_utils'; +import {TestHelper} from 'utils/test_helper'; + +describe('components/integrations/InstalledIncomingWebhooks', () => { + const team = TestHelper.getTeamMock({id: 'teamId', name: 'test'}); + const user = TestHelper.getUserMock({id: 'userId'}); + const channel = TestHelper.getChannelMock({ + id: 'channelId', + display_name: 'Town Square', + }); + + const hookAlpha: IncomingWebhook = TestHelper.getIncomingWebhookMock({ + id: 'hook-alpha', + display_name: 'Alpha Webhook', + channel_id: 'channelId', + team_id: 'teamId', + user_id: 'userId', + }); + const hookCharlie: IncomingWebhook = TestHelper.getIncomingWebhookMock({ + id: 'hook-charlie', + display_name: 'Charlie Webhook', + channel_id: 'channelId', + team_id: 'teamId', + user_id: 'userId', + }); + const hookBravo: IncomingWebhook = TestHelper.getIncomingWebhookMock({ + id: 'hook-bravo', + display_name: 'Bravo Webhook', + channel_id: 'channelId', + team_id: 'teamId', + user_id: 'userId', + }); + + const initialState = { + entities: { + general: {config: {}}, + users: {currentUserId: 'userId'}, + }, + }; + + const defaultProps = { + team, + user, + incomingHooks: [hookAlpha, hookCharlie, hookBravo], + incomingHooksTotalCount: 3, + channels: {channelId: channel}, + users: {userId: user}, + canManageOthersWebhooks: true, + enableIncomingWebhooks: true, + actions: { + removeIncomingHook: jest.fn(), + loadIncomingHooksAndProfilesForTeam: jest.fn().mockReturnValue(Promise.resolve()), + }, + }; + + test('renders webhooks sorted alphabetically by display name', async () => { + renderWithContext( + , + initialState, + ); + + await waitFor(() => { + expect(screen.getByText('Alpha Webhook')).toBeInTheDocument(); + }); + + const items = screen.getAllByText(/Webhook/); + const names = items.map((el) => el.textContent); + + const alphaIdx = names.findIndex((n) => n?.includes('Alpha')); + const bravoIdx = names.findIndex((n) => n?.includes('Bravo')); + const charlieIdx = names.findIndex((n) => n?.includes('Charlie')); + + expect(alphaIdx).toBeLessThan(bravoIdx); + expect(bravoIdx).toBeLessThan(charlieIdx); + }); + + test('does not mutate the incomingHooks prop array when sorting', async () => { + const hooks: IncomingWebhook[] = [hookAlpha, hookCharlie, hookBravo]; + const originalOrder = hooks.map((h) => h.id); + + const props = {...defaultProps, incomingHooks: hooks}; + + renderWithContext( + , + initialState, + ); + + await waitFor(() => { + expect(screen.getByText('Alpha Webhook')).toBeInTheDocument(); + }); + + // The original array passed as prop must not be mutated by the sort + expect(hooks.map((h) => h.id)).toEqual(originalOrder); + }); + + test('compares hooks with missing display_name symmetrically using channel name fallback', async () => { + const noNameHook: IncomingWebhook = TestHelper.getIncomingWebhookMock({ + id: 'hook-no-name', + display_name: '', + channel_id: 'channelId', + team_id: 'teamId', + user_id: 'userId', + }); + const namedHook: IncomingWebhook = TestHelper.getIncomingWebhookMock({ + id: 'hook-named', + display_name: 'Zeta Webhook', + channel_id: 'channelId', + team_id: 'teamId', + user_id: 'userId', + }); + + // channel display_name is "Town Square" which sorts before "Zeta Webhook" + const props = { + ...defaultProps, + incomingHooks: [namedHook, noNameHook], + incomingHooksTotalCount: 2, + }; + + renderWithContext( + , + initialState, + ); + + await waitFor(() => { + expect(screen.getByText('Zeta Webhook')).toBeInTheDocument(); + }); + + const townSquareEl = screen.getByText('Town Square'); + const zetaEl = screen.getByText('Zeta Webhook'); + + expect(townSquareEl).toBeInTheDocument(); + expect(zetaEl).toBeInTheDocument(); + + // Verify DOM order: Town Square (channel fallback) should appear before Zeta Webhook + const position = townSquareEl.compareDocumentPosition(zetaEl); + expect(position & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy(); + }); +}); diff --git a/webapp/channels/src/components/integrations/installed_incoming_webhooks/installed_incoming_webhooks.tsx b/webapp/channels/src/components/integrations/installed_incoming_webhooks/installed_incoming_webhooks.tsx index 137c3cb57e5..cd8ea6a61b0 100644 --- a/webapp/channels/src/components/integrations/installed_incoming_webhooks/installed_incoming_webhooks.tsx +++ b/webapp/channels/src/components/integrations/installed_incoming_webhooks/installed_incoming_webhooks.tsx @@ -95,11 +95,20 @@ export default class InstalledIncomingWebhooks extends React.PureComponent this.props.incomingHooks. + incomingWebhooks = (filter: string) => [...this.props.incomingHooks]. sort(this.incomingWebhookCompare). filter((incomingWebhook: IncomingWebhook) => matchesFilter(incomingWebhook, this.props.channels[incomingWebhook.channel_id], filter)). map((incomingWebhook: IncomingWebhook) => { diff --git a/webapp/channels/src/components/integrations/installed_outgoing_webhooks/installed_outgoing_webhooks.tsx b/webapp/channels/src/components/integrations/installed_outgoing_webhooks/installed_outgoing_webhooks.tsx index 0da6ca38461..5d5eafd9756 100644 --- a/webapp/channels/src/components/integrations/installed_outgoing_webhooks/installed_outgoing_webhooks.tsx +++ b/webapp/channels/src/components/integrations/installed_outgoing_webhooks/installed_outgoing_webhooks.tsx @@ -136,7 +136,7 @@ export default class InstalledOutgoingWebhooks extends React.PureComponent this.props.outgoingWebhooks. + outgoingWebhooks = (filter: string) => [...this.props.outgoingWebhooks]. sort(this.outgoingWebhookCompare). filter((outgoingWebhook) => matchesFilter(outgoingWebhook, this.props.channels[outgoingWebhook.channel_id], filter)). map((outgoingWebhook) => { diff --git a/webapp/channels/src/components/leave_channel_modal/__snapshots__/leave_channel_modal.test.tsx.snap b/webapp/channels/src/components/leave_channel_modal/__snapshots__/leave_channel_modal.test.tsx.snap index 7880e780fb4..cd3008578c7 100644 --- a/webapp/channels/src/components/leave_channel_modal/__snapshots__/leave_channel_modal.test.tsx.snap +++ b/webapp/channels/src/components/leave_channel_modal/__snapshots__/leave_channel_modal.test.tsx.snap @@ -1,5 +1,116 @@ // Jest Snapshot v1, https://jestjs.io/docs/snapshot-testing +exports[`components/LeaveChannelModal should match snapshot for the policy enforced public channel variant 1`] = ` + +