[pull] master from mattermost:master#710
Merged
Merged
Conversation
…e 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 <build@mattermost.com>
) * 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 <mgdelacroix@users.noreply.github.com> * Fix lint: remove space before JSX closing tag in webhook test Co-authored-by: Miguel de la Cruz <mgdelacroix@users.noreply.github.com> * 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 <mgdelacroix@users.noreply.github.com> * 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 <mgdelacroix@users.noreply.github.com> * 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 <mgdelacroix@users.noreply.github.com> * 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 <mgdelacroix@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Miguel de la Cruz <mgdelacroix@users.noreply.github.com>
* 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 <mgdelacroix@users.noreply.github.com> * 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 <mgdelacroix@users.noreply.github.com> * 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 <mgdelacroix@users.noreply.github.com> * 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 <mgdelacroix@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Miguel de la Cruz <mgdelacroix@users.noreply.github.com> Co-authored-by: Mattermost Build <build@mattermost.com>
…er DM (#36552) * Add Cursor Cloud Agent Docker environment Co-authored-by: Cursor <cursoragent@cursor.com> * Fix Cloud Agent enterprise and Docker access Co-authored-by: Cursor <cursoragent@cursor.com> * Fix Cloud Agent Go path setup Co-authored-by: Cursor <cursoragent@cursor.com> * 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 <maria.nunez@mattermost.com> --------- Co-authored-by: Nick Misasi <nick.misasi@mattermost.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…ls (#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 <isacikgoz@users.noreply.github.com> * 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 <isacikgoz@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Ibrahim Serdar Acikgoz <isacikgoz@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )