From 238867e24762ab1557f676c589820615d7293d5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Garc=C3=ADa=20Montoro?= Date: Mon, 18 May 2026 12:16:32 +0200 Subject: [PATCH 1/6] MM-68732: Remove global mutex for login attempts in favour of database serialization (#36515) * Add atomic login-attempt counter primitives to UserStore Two new store methods back the upcoming switch from a global per-node mutex to per-user atomic slot claiming: TryIncrementFailedPasswordAttempts(userID, maxAttempts) (bool, error) UPDATE Users SET FailedAttempts = FailedAttempts + 1 WHERE Id = ? AND FailedAttempts < maxAttempts Returns true when a slot was claimed (rows affected == 1) and false when the cap was already reached. The conditional UPDATE serialises concurrent attempts on the same user via the row lock, so the cap is enforced without any application-level locking and without serialising attempts across users. DecrementFailedPasswordAttempts(userID) error UPDATE Users SET FailedAttempts = FailedAttempts - 1 WHERE Id = ? AND FailedAttempts > 0 Releases a slot previously claimed by TryIncrement when the in-flight authentication turns out not to be a credential failure. The conditional UPDATE means concurrent decrements cannot underflow. Storetest covers both primitives: claim-below-cap, reject-at-cap, reject-above-cap, no-op for unknown user, and a 50-goroutine concurrent test with a start barrier asserting exactly maxAttempts slots are ever claimed and that decrement clamps at zero under contention. The testify mock is regenerated here so the storetest package that returns *mocks.UserStore as a store.UserStore still satisfies the interface; the wrapper layers are regenerated in the next commit. ------ AI assisted commit * Regenerate store layers for the new primitives Pick up TryIncrementFailedPasswordAttempts and DecrementFailedPasswordAttempts in every generated wrapper: - retrylayer: retry on repeatable errors using the standard three-attempt loop. - timerlayer: record store-method duration metrics under UserStore.TryIncrementFailedPasswordAttempts and UserStore.DecrementFailedPasswordAttempts. - localcachelayer: invalidate the profile cache only after the underlying conditional UPDATE actually changes a row; an at-cap no-op return on TryIncrement no longer produces unnecessary cluster invalidation traffic. ------ AI assisted commit * Drop login-attempt mutex; use per-user slot claiming Replace the global per-node mutex that serialised every login attempt with the database-side atomic slot machine added on the Users row. Each of the three authentication entry points now pre-claims a slot via TryIncrementFailedPasswordAttempts before running the expensive password / LDAP / MFA check, and releases the slot when the failure path is not a real credential mismatch: - CheckPasswordAndAllCriteria (email/password): refunds the slot on backend errors during the password check (malformed stored hash, hasher misc failure, password-migration write failure) so a transient infra issue cannot ratchet FailedAttempts to a lockout for a user with valid credentials; refunds on the MFA pre-flight probe (empty mfaToken on an MFA-enabled user) so the probe is not counted as a real attempt. - DoubleCheckPassword: same backend-error refund predicate. - checkLdapUserPasswordAndAllCriteria: pre-claims only for existing users (first-time LDAP users have no local row to claim against); refunds non-credential DoLogin errors (server unreachable, transient) so an LDAP outage cannot lock out everyone; refunds the MFA pre-flight probe; for first-time users, explicitly bumps the counter via UpdateFailedPasswordAttempts on a real bad-password or bad-MFA attempt, matching the pre-refactor counting behaviour. If the refund itself fails the underlying authentication error is preserved and returned to the caller (the failure is logged); a leaked slot is annoying, but masking the real failure with a generic store 500 would be a clear observability regression. Cluster-wide behaviour also changes: the previous design honoured MaximumLoginAttempts per node, so an n-node cluster effectively permitted n * MaximumLoginAttempts attempts. The cap is now enforced globally. ------ AI assisted commit * Cover app-layer behaviors of the new login slot machine The store-layer tests already exercise TryIncrement and Decrement under concurrency and at the cap boundary. The new behavioural contracts at the app layer were not covered, so a regression that flipped a refund predicate, a probe condition, or a first-time LDAP path would have slipped through type checking and existing unit tests. Add tests around the three callers of the new path: - CheckPasswordAndAllCriteria: an MFA pre-flight probe (empty token) does not consume a slot; a real attempt with a wrong non-empty token does; a backend error during the password check (malformed stored hash) refunds the slot; the happy path also asserts FailedAttempts resets to zero. - DoubleCheckPassword: gets its first test coverage, covering the happy path, rate-limit rejection once max attempts is reached, and the backend-error refund path. - checkLdapUserPasswordAndAllCriteria: covers paths the table loop did not exercise, first-time LDAP user with a bad password (uses GetUserByAuth to reach the freshly created row), first-time LDAP user with a wrong MFA token, existing LDAP user with a non-credential DoLogin error (slot refunded), and the existing LDAP user MFA pre-flight probe (slot refunded). ------ AI assisted commit * Address coderabbit review ------ AI assisted commit * Fix race in first-time LDAP failed-attempt counter For first-time LDAP users we have no local row to pre-claim, so the bad-password and bad-MFA branches fell back to an absolute UpdateFailedPasswordAttempts(id, ldapUser.FailedAttempts+1) based on a snapshot from GetUserByAuth. Concurrent first-attempt requests for the same user could all read FailedAttempts == 0 and all write 1, losing increments. As a secondary issue the absolute set did not enforce MaximumLoginAttempts, so the counter could also drift past the cap. Switch both branches to TryIncrementFailedPasswordAttempts, the atomic conditional UPDATE already used on every other path. The row lock serialises concurrent increments and the predicate caps at MaximumLoginAttempts. A new concurrent storetest-style subtest runs 3 * maxFailedLoginAttempts goroutines through the first-time bad-password path against the same fresh LDAP row and asserts FailedAttempts lands at exactly maxFailedLoginAttempts. Against the previous absolute-set implementation the test fails (observed FailedAttempts = 4 with maxFailedLoginAttempts = 3, either a lost increment or a cap overshoot). The first-time bad-password branch also switches from a wrapped 500 return on store error to log-and-continue, matching the rest of the file's refund/probe error handling: the underlying LDAP authentication failure is the more useful error for the caller. ------ AI assisted commit * Address review comments ------ AI assisted commit --------- Co-authored-by: Mattermost Build --- server/channels/app/authentication.go | 139 +++++--- server/channels/app/authentication_test.go | 318 ++++++++++++++++-- server/channels/app/channels.go | 8 +- .../store/localcachelayer/user_layer.go | 19 ++ .../channels/store/retrylayer/retrylayer.go | 42 +++ server/channels/store/sqlstore/user_store.go | 39 +++ server/channels/store/store.go | 2 + .../store/storetest/mocks/UserStore.go | 46 +++ server/channels/store/storetest/user_store.go | 143 ++++++++ .../channels/store/timerlayer/timerlayer.go | 32 ++ 10 files changed, 715 insertions(+), 73 deletions(-) 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/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/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() From 669eb104c60c82e3e3eed2b18d1e7c64877aa71e Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Mon, 18 May 2026 12:25:05 +0200 Subject: [PATCH 2/6] Fix webhook list ordering instability when paginating (MM-65732) (#36470) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix webhook list ordering instability when paginating (MM-65732) The webhook list view reorders entries when navigating between pages. The first page initially shows webhooks in insertion order (from the server), but after loading additional pages the display settles into alphabetical order. Going back to page 1 then shows different items than were originally visible. Root causes: 1. Server: GetIncomingByTeamByUser, GetIncomingListByUser, GetOutgoingByTeamByUser, and GetOutgoingListByUser had no ORDER BY clause, so the database could return rows in any order. 2. Client (incoming webhooks): incomingWebhookCompare only resolved the channel-name fallback for the 'a' argument, not 'b', making the comparator asymmetric and producing an unstable sort. 3. Client: both installed_incoming_webhooks and installed_outgoing_webhooks called Array.prototype.sort() directly on the props array, mutating it. Fix: - Add ORDER BY DisplayName, Id to the four listing SQL queries so API pages always come back in alphabetical order. With a stable server order, the client sort over merged pages produces the same slice for each page number regardless of how many pages have been loaded. - Symmetrise incomingWebhookCompare by applying the same channel-name and 'Private Webhook' fallback to the 'b' argument. - Sort a copy ([...hooks].sort()) in both webhook list components so the original prop arrays are never mutated. Co-authored-by: Miguel de la Cruz * Fix lint: remove space before JSX closing tag in webhook test Co-authored-by: Miguel de la Cruz * Fold ordering tests into existing webhook store test functions Instead of four separate top-level test registrations (GetIncomingListByUserOrdering, etc.), each ordering assertion is now a t.Run sub-test inside its corresponding existing function: testWebhookStoreGetIncomingListByUser └─ "GetIncomingListByUser, ordered alphabetically by display name" TestWebhookStoreGetIncomingByTeamByUser └─ "GetIncomingByTeamByUser, ordered alphabetically by display name" testWebhookStoreGetOutgoingListByUser └─ "GetOutgoingListByUser, ordered alphabetically by display name" testWebhookStoreGetOutgoingByTeamByUser └─ "GetOutgoingByTeamByUser, ordered alphabetically by display name" Each sub-test creates fresh hooks (Charlie, Alpha, Bravo in insertion order) scoped to its own IDs so they do not interfere with the outer test's fixtures. Co-authored-by: Miguel de la Cruz * Fix govet shadow and gofmt issues in webhook store tests - Rename the outer err variable to errSave in three functions (testWebhookStoreGetIncomingListByUser, TestWebhookStoreGetIncomingByTeamByUser, testWebhookStoreGetOutgoingByTeamByUser) so that hooks, err := declarations in sub-test closures no longer shadow it. - Change hookC, err = to hookC, err := in each ordering sub-test to declare a local err instead of capturing the outer one. - Remove a trailing blank line at the end of the file (gofmt). Co-authored-by: Miguel de la Cruz * Remove jest.mock from installed_incoming_webhooks test The mock for delete_integration_link was copied from the outgoing webhooks list test but is not needed here: the real component renders fine within renderWithContext (as shown by installed_incoming_webhook.test.tsx which tests the individual item without any mocks). Since the ordering tests do not interact with delete functionality, drop the mock and align the action stub style to mockReturnValue(Promise.resolve()). Co-authored-by: Miguel de la Cruz * Add ORDER BY to GetOutgoingByChannelByUser; add ordering sub-test GetOutgoingByChannelByUser was the last paginated webhook listing function without an ORDER BY clause. Add OrderBy("DisplayName", "Id") consistent with all other listing functions. Add the corresponding ordering sub-test inside testWebhookStoreGetOutgoingByChannelByUser, following the same errSave pattern established for the other functions to avoid govet shadow warnings. Co-authored-by: Miguel de la Cruz --------- Co-authored-by: Cursor Agent Co-authored-by: Miguel de la Cruz --- .../channels/store/sqlstore/webhook_store.go | 9 +- .../channels/store/storetest/webhook_store.go | 139 ++++++++++++++-- .../installed_incoming_webhooks.test.tsx | 154 ++++++++++++++++++ .../installed_incoming_webhooks.tsx | 13 +- .../installed_outgoing_webhooks.tsx | 2 +- 5 files changed, 294 insertions(+), 23 deletions(-) create mode 100644 webapp/channels/src/components/integrations/installed_incoming_webhooks/installed_incoming_webhooks.test.tsx 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/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/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) => { From 9d0615554077ca123057e9bab158f94f2e0c52a9 Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Mon, 18 May 2026 12:27:38 +0200 Subject: [PATCH 3/6] Update bot checks (#36503) * Fix bot permission checks in revokeSession, revokeAllSessionsForUser, and updatePassword MM-68701: Align permission checks with the bot-aware pattern used by updateUser, patchUser, deleteUser, and (via MM-68686) updateUserActive. Three handlers were missing the IsBot branch: - revokeSession / revokeAllSessionsForUser: both gated access through SessionHasPermissionToUser, which only requires EditOtherUsers (an ancillary permission granted to User Managers). Switching to SessionHasPermissionToUserOrBot routes bot targets through SessionHasPermissionToManageBot first and falls back to the user path only when the target is not a bot. - updatePassword: the permission flag canUpdatePassword was set by checking PermissionSysconsoleWriteUserManagementUsers (or PermissionManageSystem for system admins) with no IsBot branch. Adding an else-if user.IsBot guard routes bot targets through SessionHasPermissionToManageBot, consistent with every other handler in the file that touches bot accounts. Co-authored-by: Miguel de la Cruz * Improve TestRevokeSessionBotPermissions: revoke a real bot session Seed a session directly via th.App.CreateSession instead of passing a fake ID and expecting a 400. The test now validates the full happy path: the session row is created, the privileged user revokes it, and the call returns 200 OK. Co-authored-by: Miguel de la Cruz * Strengthen forbidden sub-test: revoke a real bot session with no perms Seed a real session for the bot before the unprivileged revoke call. The test now proves the permission gate blocks access even when the target session ID genuinely exists in the database. Co-authored-by: Miguel de la Cruz * Address review feedback: add post-conditions to bot session revoke tests - TestRevokeSessionBotPermissions: after RevokeSession succeeds, assert GetSessionById returns an error to confirm the row is gone. - TestRevokeAllSessionsForUserBotPermissions: seed a real session before RevokeAllSessions so the call is not a no-op, then assert GetSessions returns an empty list afterwards. Co-authored-by: Miguel de la Cruz --------- Co-authored-by: Cursor Agent Co-authored-by: Miguel de la Cruz Co-authored-by: Mattermost Build --- server/channels/api4/user.go | 6 +- server/channels/api4/user_test.go | 149 ++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 2 deletions(-) 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() From f067fcde92eb16ef1bec24a8ddc1856fc3ca7f1d Mon Sep 17 00:00:00 2001 From: Maria A Nunez Date: Mon, 18 May 2026 07:22:16 -0400 Subject: [PATCH 4/6] MM-66339 Hide empty content-flagging "With comment" section in reviewer DM (#36552) * Add Cursor Cloud Agent Docker environment Co-authored-by: Cursor * Fix Cloud Agent enterprise and Docker access Co-authored-by: Cursor * Fix Cloud Agent Go path setup Co-authored-by: Cursor * MM-66339 Stop double-JSON-stringifying content flagging comments The flagPost, removeFlaggedPost, and keepFlaggedPost Client4 helpers were calling JSON.stringify on the comment value before placing it in the JSON request body. When the reporter or reviewer left the optional comment blank, JSON.stringify('') returned the literal two-character string '""', which the server then stored as the comment and embedded in the reviewer DM as 'With comment:\n\n> ""'. Send comment as the plain string instead so an empty comment stays empty and the 'With comment' section is omitted entirely. Co-authored-by: Maria A Nunez --------- Co-authored-by: Nick Misasi Co-authored-by: Cursor --- webapp/platform/client/src/client4.test.ts | 93 ++++++++++++++++++++++ webapp/platform/client/src/client4.ts | 6 +- 2 files changed, 96 insertions(+), 3 deletions(-) diff --git a/webapp/platform/client/src/client4.test.ts b/webapp/platform/client/src/client4.test.ts index 01797a1f57a..8aa37520ee5 100644 --- a/webapp/platform/client/src/client4.test.ts +++ b/webapp/platform/client/src/client4.test.ts @@ -111,6 +111,99 @@ describe('Client4', () => { }); }); + describe('content flagging routes', () => { + let client: Client4; + + beforeEach(() => { + client = new Client4(); + client.setUrl('http://mattermost.example.com'); + }); + + test('flagPost should send comment as a plain string', async () => { + let receivedBody: any; + nock(client.getBaseRoute()). + post('/content_flagging/post/post123/flag', (body) => { + receivedBody = body; + return true; + }). + reply(200, {status: 'OK'}); + + await client.flagPost('post123', 'Spam', 'looks suspicious'); + + expect(receivedBody).toEqual({reason: 'Spam', comment: 'looks suspicious'}); + }); + + test('flagPost should preserve an empty comment as an empty string', async () => { + let receivedBody: any; + nock(client.getBaseRoute()). + post('/content_flagging/post/post123/flag', (body) => { + receivedBody = body; + return true; + }). + reply(200, {status: 'OK'}); + + await client.flagPost('post123', 'Spam', ''); + + expect(receivedBody).toEqual({reason: 'Spam', comment: ''}); + }); + + test('removeFlaggedPost should send comment as a plain string', async () => { + let receivedBody: any; + nock(client.getBaseRoute()). + put('/content_flagging/post/post123/remove', (body) => { + receivedBody = body; + return true; + }). + reply(200, {status: 'OK'}); + + await client.removeFlaggedPost('post123', 'violates policy'); + + expect(receivedBody).toEqual({comment: 'violates policy'}); + }); + + test('removeFlaggedPost should preserve an empty comment as an empty string', async () => { + let receivedBody: any; + nock(client.getBaseRoute()). + put('/content_flagging/post/post123/remove', (body) => { + receivedBody = body; + return true; + }). + reply(200, {status: 'OK'}); + + await client.removeFlaggedPost('post123', ''); + + expect(receivedBody).toEqual({comment: ''}); + }); + + test('keepFlaggedPost should send comment as a plain string', async () => { + let receivedBody: any; + nock(client.getBaseRoute()). + put('/content_flagging/post/post123/keep', (body) => { + receivedBody = body; + return true; + }). + reply(200, {status: 'OK'}); + + await client.keepFlaggedPost('post123', 'looks fine'); + + expect(receivedBody).toEqual({comment: 'looks fine'}); + }); + + test('keepFlaggedPost should preserve an empty comment as an empty string', async () => { + let receivedBody: any; + nock(client.getBaseRoute()). + put('/content_flagging/post/post123/keep', (body) => { + receivedBody = body; + return true; + }). + reply(200, {status: 'OK'}); + + await client.keepFlaggedPost('post123', ''); + + expect(receivedBody).toEqual({comment: ''}); + }); + }); + describe('doFetchWithResponse', () => { test('serverVersion should be set from response header', async () => { const client = new Client4(); diff --git a/webapp/platform/client/src/client4.ts b/webapp/platform/client/src/client4.ts index 9d8124e1e29..3ca3afe26e6 100644 --- a/webapp/platform/client/src/client4.ts +++ b/webapp/platform/client/src/client4.ts @@ -5010,7 +5010,7 @@ export default class Client4 { `${this.getContentFlaggingRoute()}/post/${postId}/flag`, { method: 'post', - body: JSON.stringify({reason, comment: JSON.stringify(comment)}), + body: JSON.stringify({reason, comment}), }, ); }; @@ -5020,7 +5020,7 @@ export default class Client4 { `${this.getContentFlaggingRoute()}/post/${postId}/remove`, { method: 'put', - body: JSON.stringify({comment: JSON.stringify(comment)}), + body: JSON.stringify({comment}), }, ); }; @@ -5030,7 +5030,7 @@ export default class Client4 { `${this.getContentFlaggingRoute()}/post/${postId}/keep`, { method: 'put', - body: JSON.stringify({comment: JSON.stringify(comment)}), + body: JSON.stringify({comment}), }, ); }; From bab90098251922b8c7df136b0f3a30c7de7727bc Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Mon, 18 May 2026 13:47:45 +0200 Subject: [PATCH 5/6] MM-68592: Add leave confirmation modal for policy-added public channels (#36439) * MM-68592: Add leave confirmation modal for policy-added public channels When a user attempts to leave a public channel they were auto-added to via a membership policy (channel.policy_enforced), show a confirmation modal informing them that the leave is permanent and offering a 'Mute instead' option as a lighter alternative. The flow follows the existing pattern used for private channel leave confirmation. The modal is opened from: - Channel header menu Leave action - Sidebar channel menu Leave action - /leave slash command The Mute instead button is hidden when the channel is already muted. Co-authored-by: Ibrahim Serdar Acikgoz * MM-68592: address CodeRabbit review - Make handleMuteInstead async and only close the modal when the mute action resolves successfully, leaving it open on error so the user can retry or choose to leave instead. - Move autoFocus from the destructive 'Leave channel' button to the non-destructive secondary action ('Mute instead' or 'Cancel') so pressing Enter does not default-confirm a permanent leave. - Cover the failure path with a new unit test that asserts the modal remains open when muteChannel returns an error. Co-authored-by: Ibrahim Serdar Acikgoz --------- Co-authored-by: Cursor Agent Co-authored-by: Ibrahim Serdar Acikgoz --- webapp/channels/src/actions/command.ts | 2 +- .../menu_items/leave_channel.test.tsx | 24 ++++ .../menu_items/leave_channel.tsx | 2 +- .../leave_channel_modal.test.tsx.snap | 111 ++++++++++++++++ .../components/leave_channel_modal/index.ts | 24 +++- .../leave_channel_modal.test.tsx | 125 ++++++++++++++++++ .../leave_channel_modal.tsx | 123 ++++++++++++++++- .../sidebar_base_channel.test.tsx | 31 +++++ .../sidebar_base_channel.tsx | 6 +- webapp/channels/src/i18n/en.json | 5 + 10 files changed, 446 insertions(+), 7 deletions(-) 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/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`] = ` + +