From fc9d3be3689d7218f9bd5dd8699e74b33c440911 Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Wed, 8 Apr 2026 21:47:57 -0700 Subject: [PATCH 01/10] Strip remote_id field from user patch API requests (#35910) * Strip remote_id from user patch API requests * Ignore remote_id in user update API endpoints Add SetUserRemoteID test helper in testlib to set remote_id via direct SQL, bypassing the now-protected store Update method. Update existing tests in app and api4 packages to use the new helper. --------- Co-authored-by: Mattermost Build --- server/channels/api4/apitestlib.go | 10 ++ server/channels/api4/shared_channel_test.go | 18 +--- server/channels/api4/user.go | 2 + server/channels/api4/user_test.go | 98 +++++++++++++++++++ server/channels/app/channel_test.go | 8 +- server/channels/app/helper_test.go | 10 ++ ..._global_user_sync_self_referential_test.go | 2 +- ...l_membership_sync_self_referential_test.go | 4 +- server/channels/app/shared_channel_test.go | 9 +- server/channels/app/user_test.go | 5 +- server/channels/store/sqlstore/user_store.go | 1 + server/channels/testlib/helper.go | 7 ++ 12 files changed, 142 insertions(+), 32 deletions(-) diff --git a/server/channels/api4/apitestlib.go b/server/channels/api4/apitestlib.go index facb392d651..3a811a27187 100644 --- a/server/channels/api4/apitestlib.go +++ b/server/channels/api4/apitestlib.go @@ -1409,6 +1409,16 @@ func (th *TestHelper) SetupScheme(tb testing.TB, scope string) *model.Scheme { return scheme } +func (th *TestHelper) SetUserRemoteID(tb testing.TB, userID, remoteID string) *model.User { + tb.Helper() + err := testlib.SetUserRemoteID(mainHelper.GetSQLStore(), userID, remoteID) + require.NoError(tb, err) + th.App.InvalidateCacheForUser(userID) + user, appErr := th.App.GetUser(userID) + require.Nil(tb, appErr) + return user +} + func (th *TestHelper) Parallel(t *testing.T) { mainHelper.Parallel(t) } diff --git a/server/channels/api4/shared_channel_test.go b/server/channels/api4/shared_channel_test.go index 1926ab09d46..26b900809db 100644 --- a/server/channels/api4/shared_channel_test.go +++ b/server/channels/api4/shared_channel_test.go @@ -221,10 +221,7 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) { }() localUser := th.BasicUser - remoteUser := th.CreateUser(t) - remoteUser.RemoteId = model.NewPointer(model.NewId()) - remoteUser, appErr := th.App.UpdateUser(th.Context, remoteUser, false) - require.Nil(t, appErr) + remoteUser := th.SetUserRemoteID(t, th.CreateUser(t).Id, model.NewId()) dm, _, err := client.CreateDirectChannel(context.Background(), localUser.Id, remoteUser.Id) require.Error(t, err) @@ -243,10 +240,7 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) { }() localUser := th.BasicUser - remoteUser := th.CreateUser(t) - remoteUser.RemoteId = model.NewPointer(model.NewId()) - remoteUser, appErr := th.App.UpdateUser(th.Context, remoteUser, false) - require.Nil(t, appErr) + remoteUser := th.SetUserRemoteID(t, th.CreateUser(t).Id, model.NewId()) dm, _, err := client.CreateDirectChannel(context.Background(), localUser.Id, remoteUser.Id) require.NoError(t, err) @@ -278,9 +272,7 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) { rc, appErr := th.App.AddRemoteCluster(rc) require.Nil(t, appErr) - remoteUser.RemoteId = model.NewPointer(rc.RemoteId) - remoteUser, appErr = th.App.UpdateUser(th.Context, remoteUser, false) - require.Nil(t, appErr) + th.SetUserRemoteID(t, remoteUser.Id, rc.RemoteId) dm, _, err := client.CreateDirectChannel(context.Background(), localUser.Id, remoteUser.Id) require.NoError(t, err) @@ -312,9 +304,7 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) { rc, appErr := th.App.AddRemoteCluster(rc) require.Nil(t, appErr) - remoteUser.RemoteId = model.NewPointer(rc.RemoteId) - remoteUser, appErr = th.App.UpdateUser(th.Context, remoteUser, false) - require.Nil(t, appErr) + th.SetUserRemoteID(t, remoteUser.Id, rc.RemoteId) dm, _, err := client.CreateDirectChannel(context.Background(), remoteUser.Id, localUser.Id) require.NoError(t, err) diff --git a/server/channels/api4/user.go b/server/channels/api4/user.go index 584cefd8bbf..91d996dfcc4 100644 --- a/server/channels/api4/user.go +++ b/server/channels/api4/user.go @@ -1450,6 +1450,8 @@ func patchUser(c *Context, w http.ResponseWriter, r *http.Request) { return } + patch.RemoteId = nil + auditRec := c.MakeAuditRecord(model.AuditEventPatchUser, model.AuditStatusFail) model.AddEventParameterAuditableToAuditRec(auditRec, "user_patch", &patch) defer c.LogAuditRec(auditRec) diff --git a/server/channels/api4/user_test.go b/server/channels/api4/user_test.go index cc25a3f72a0..fbf844e6330 100644 --- a/server/channels/api4/user_test.go +++ b/server/channels/api4/user_test.go @@ -2316,6 +2316,50 @@ func TestUpdateUser(t *testing.T) { }) } +func TestUpdateUserRemoteIdIgnored(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t) + + t.Run("remote_id in update body is ignored for regular user", func(t *testing.T) { + user := th.CreateUser(t) + _, _, err := th.Client.Login(context.Background(), user.Email, user.Password) + require.NoError(t, err) + + user.RemoteId = model.NewPointer("attacker-remote-id") + user.Nickname = "updated-nickname" + ruser, _, err := th.Client.UpdateUser(context.Background(), user) + require.NoError(t, err) + require.Equal(t, "updated-nickname", ruser.Nickname) + require.Empty(t, model.SafeDereference(ruser.RemoteId), "remote_id should remain empty") + + dbUser, appErr := th.App.GetUser(user.Id) + require.Nil(t, appErr) + require.Empty(t, model.SafeDereference(dbUser.RemoteId), "remote_id should not be persisted") + }) + + t.Run("remote_id in update body is ignored for system admin", func(t *testing.T) { + user := th.CreateUser(t) + + user.RemoteId = model.NewPointer("admin-remote-id") + user.Nickname = "admin-updated" + ruser, _, err := th.SystemAdminClient.UpdateUser(context.Background(), user) + require.NoError(t, err) + require.Equal(t, "admin-updated", ruser.Nickname) + require.Empty(t, model.SafeDereference(ruser.RemoteId), "remote_id should remain empty even for admin") + }) + + t.Run("existing remote_id is preserved when updating other fields", func(t *testing.T) { + remoteId := model.NewId() + user := th.SetUserRemoteID(t, th.CreateUser(t).Id, remoteId) + + user.Nickname = "updated-nickname" + ruser, _, err := th.SystemAdminClient.UpdateUser(context.Background(), user) + require.NoError(t, err) + require.Equal(t, "updated-nickname", ruser.Nickname) + require.Equal(t, remoteId, model.SafeDereference(ruser.RemoteId), "existing remote_id should be preserved") + }) +} + func TestUpdateAdminUser(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t) @@ -2474,6 +2518,60 @@ func TestPatchUser(t *testing.T) { require.NoError(t, err) } +func TestPatchUserRemoteIdIgnored(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + t.Run("remote_id in patch is ignored for regular user", func(t *testing.T) { + user := th.CreateUser(t) + _, _, err := th.Client.Login(context.Background(), user.Email, user.Password) + require.NoError(t, err) + + patch := &model.UserPatch{ + RemoteId: model.NewPointer("attacker-remote-id"), + Nickname: model.NewPointer("new-nickname"), + } + ruser, _, err := th.Client.PatchUser(context.Background(), user.Id, patch) + require.NoError(t, err) + require.Equal(t, "new-nickname", ruser.Nickname) + require.Empty(t, model.SafeDereference(ruser.RemoteId), "remote_id should remain empty") + + dbUser, appErr := th.App.GetUser(user.Id) + require.Nil(t, appErr) + require.Empty(t, model.SafeDereference(dbUser.RemoteId), "remote_id should not be persisted") + }) + + t.Run("remote_id in patch is ignored for system admin", func(t *testing.T) { + user := th.CreateUser(t) + + patch := &model.UserPatch{ + RemoteId: model.NewPointer("admin-remote-id"), + Nickname: model.NewPointer("admin-patched"), + } + ruser, _, err := th.SystemAdminClient.PatchUser(context.Background(), user.Id, patch) + require.NoError(t, err) + require.Equal(t, "admin-patched", ruser.Nickname) + require.Empty(t, model.SafeDereference(ruser.RemoteId), "remote_id should remain empty even for admin") + + dbUser, appErr := th.App.GetUser(user.Id) + require.Nil(t, appErr) + require.Empty(t, model.SafeDereference(dbUser.RemoteId), "remote_id should not be persisted even for admin") + }) + + t.Run("existing remote_id is preserved when patching other fields", func(t *testing.T) { + remoteId := model.NewId() + user := th.SetUserRemoteID(t, th.CreateUser(t).Id, remoteId) + + patch := &model.UserPatch{ + Nickname: model.NewPointer("updated-nickname"), + } + ruser, _, err := th.SystemAdminClient.PatchUser(context.Background(), user.Id, patch) + require.NoError(t, err) + require.Equal(t, "updated-nickname", ruser.Nickname) + require.Equal(t, remoteId, model.SafeDereference(ruser.RemoteId), "existing remote_id should be preserved") + }) +} + func TestPatchBotUser(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t) diff --git a/server/channels/app/channel_test.go b/server/channels/app/channel_test.go index 21592cdde2f..776c2d2ab07 100644 --- a/server/channels/app/channel_test.go +++ b/server/channels/app/channel_test.go @@ -599,9 +599,7 @@ func TestGetOrCreateDirectChannel(t *testing.T) { }) t.Run("Cannot create with a remote user", func(t *testing.T) { - user2.RemoteId = model.NewPointer(model.NewId()) - _, appErr := th.App.UpdateUser(th.Context, user2, false) - require.Nil(t, appErr) + th.SetUserRemoteID(t, user2.Id, model.NewId()) dm, appErr := th.App.GetOrCreateDirectChannel(th.Context, user1.Id, user2.Id) require.Nil(t, dm) @@ -622,9 +620,7 @@ func TestCreateGroupChannel(t *testing.T) { groupUserIds = append(groupUserIds, th.BasicUser.Id) t.Run("Should not allow to create a group with a remote user", func(t *testing.T) { - user2.RemoteId = model.NewPointer(model.NewId()) - _, appErr := th.App.UpdateUser(th.Context, user2, false) - require.Nil(t, appErr) + th.SetUserRemoteID(t, user2.Id, model.NewId()) dm, appErr := th.App.CreateGroupChannel(th.Context, groupUserIds, th.BasicUser.Id) require.NotNil(t, appErr) diff --git a/server/channels/app/helper_test.go b/server/channels/app/helper_test.go index 223be342b94..3a4f990d716 100644 --- a/server/channels/app/helper_test.go +++ b/server/channels/app/helper_test.go @@ -814,6 +814,16 @@ func decodeJSON[T any](tb testing.TB, o any, result *T) *T { return result } +func (th *TestHelper) SetUserRemoteID(tb testing.TB, userID, remoteID string) *model.User { + tb.Helper() + err := testlib.SetUserRemoteID(th.GetSqlStore(), userID, remoteID) + require.NoError(tb, err) + th.App.InvalidateCacheForUser(userID) + user, appErr := th.App.GetUser(userID) + require.Nil(tb, appErr) + return user +} + func (th *TestHelper) Parallel(t *testing.T) { mainHelper.Parallel(t) } diff --git a/server/channels/app/shared_channel_global_user_sync_self_referential_test.go b/server/channels/app/shared_channel_global_user_sync_self_referential_test.go index 64a9e2c790b..3f5ffbd5174 100644 --- a/server/channels/app/shared_channel_global_user_sync_self_referential_test.go +++ b/server/channels/app/shared_channel_global_user_sync_self_referential_test.go @@ -227,7 +227,7 @@ func TestSharedChannelGlobalUserSyncSelfReferential(t *testing.T) { // Create remote user (should NOT be synced) remoteUser := th.CreateUser(t) - remoteUser.RemoteId = &selfCluster.RemoteId + remoteUser = th.SetUserRemoteID(t, remoteUser.Id, selfCluster.RemoteId) remoteUser.UpdateAt = baseTime + 600 _, err = ss.User().Update(th.Context, remoteUser, true) require.NoError(t, err) diff --git a/server/channels/app/shared_channel_membership_sync_self_referential_test.go b/server/channels/app/shared_channel_membership_sync_self_referential_test.go index ade5f29c9e4..991acc86725 100644 --- a/server/channels/app/shared_channel_membership_sync_self_referential_test.go +++ b/server/channels/app/shared_channel_membership_sync_self_referential_test.go @@ -984,9 +984,7 @@ func TestSharedChannelMembershipSyncSelfReferential(t *testing.T) { // Create a remote user belonging to cluster-2 userFromCluster2 := th.CreateUser(t) - userFromCluster2.RemoteId = &clusters[1].RemoteId - userFromCluster2, appErr = th.App.UpdateUser(th.Context, userFromCluster2, false) - require.Nil(t, appErr) + userFromCluster2 = th.SetUserRemoteID(t, userFromCluster2.Id, clusters[1].RemoteId) _, _, appErr = th.App.AddUserToTeam(th.Context, team.Id, userFromCluster2.Id, th.BasicUser.Id) require.Nil(t, appErr) diff --git a/server/channels/app/shared_channel_test.go b/server/channels/app/shared_channel_test.go index 40a482b83ee..fc851fce009 100644 --- a/server/channels/app/shared_channel_test.go +++ b/server/channels/app/shared_channel_test.go @@ -598,12 +598,13 @@ func TestTransformMentionsOnReceive(t *testing.T) { // Helper to create test users createUser := func(username string, remoteId *string) *model.User { user := th.CreateUser(t) - user.Username = username if remoteId != nil { - user.RemoteId = remoteId + user = th.SetUserRemoteID(t, user.Id, *remoteId) } - user, updateErr := th.App.UpdateUser(th.Context, user, false) - require.Nil(t, updateErr) + user.Username = username + var appErr *model.AppError + user, appErr = th.App.UpdateUser(th.Context, user, false) + require.Nil(t, appErr) th.LinkUserToTeam(t, user, th.BasicTeam) th.AddUserToChannel(t, user, sharedChannel) return user diff --git a/server/channels/app/user_test.go b/server/channels/app/user_test.go index 813e9eeb727..12624bc20a0 100644 --- a/server/channels/app/user_test.go +++ b/server/channels/app/user_test.go @@ -2602,10 +2602,7 @@ func createTestRemoteCluster(t *testing.T, th *TestHelper, ss store.Store, name, func createRemoteUser(t *testing.T, th *TestHelper, remoteCluster *model.RemoteCluster) *model.User { user := th.CreateUser(t) - user.RemoteId = &remoteCluster.RemoteId - updatedUser, appErr := th.App.UpdateUser(th.Context, user, false) - require.Nil(t, appErr) - return updatedUser + return th.SetUserRemoteID(t, user.Id, remoteCluster.RemoteId) } func ensureRemoteClusterConnected(t *testing.T, ss store.Store, cluster *model.RemoteCluster, connected bool) { diff --git a/server/channels/store/sqlstore/user_store.go b/server/channels/store/sqlstore/user_store.go index 469b4488d0e..08d98d8abec 100644 --- a/server/channels/store/sqlstore/user_store.go +++ b/server/channels/store/sqlstore/user_store.go @@ -287,6 +287,7 @@ func (us SqlUserStore) Update(rctx request.CTX, user *model.User, trustedUpdateD user.MfaActive = oldUser.MfaActive user.MfaUsedTimestamps = oldUser.MfaUsedTimestamps user.LastLogin = oldUser.LastLogin + user.RemoteId = oldUser.RemoteId if !trustedUpdateData { user.Roles = oldUser.Roles diff --git a/server/channels/testlib/helper.go b/server/channels/testlib/helper.go index 94e0e6ae1ec..3aad9f33671 100644 --- a/server/channels/testlib/helper.go +++ b/server/channels/testlib/helper.go @@ -342,6 +342,13 @@ func (h *MainHelper) GetSQLStore() *sqlstore.SqlStore { return h.SQLStore } +// SetUserRemoteID sets the RemoteId on a user via direct SQL, bypassing the +// store's Update method which preserves RemoteId from the existing row. +func SetUserRemoteID(sqlStore *sqlstore.SqlStore, userID, remoteID string) error { + _, err := sqlStore.GetMaster().Exec("UPDATE Users SET RemoteId = ? WHERE Id = ?", remoteID, userID) + return err +} + func (h *MainHelper) GetClusterInterface() *FakeClusterInterface { if h.ClusterInterface == nil { panic("MainHelper not initialized with cluster interface.") From 010aad6308f87e659971f80f985dfe96855772f2 Mon Sep 17 00:00:00 2001 From: Harshil Sharma <18575143+harshilsharma63@users.noreply.github.com> Date: Thu, 9 Apr 2026 10:21:08 +0530 Subject: [PATCH 02/10] Fixed a bug where signup link showed up when signup was disabled (#35769) * Fixed a bug where signup link showed up when signup was disabled * Removed unused component * fixed test name * CI * fixed a test * fixed a commit --------- Co-authored-by: Mattermost Build --- .../auth_sso/authentication_4_spec.ts | 8 +-- .../login_close_server_spec.js | 5 +- .../access_problem/access_problem.scss | 40 --------------- .../src/components/access_problem/index.tsx | 51 ------------------- .../src/components/login/login.test.tsx | 19 +++++++ .../channels/src/components/login/login.tsx | 15 ++---- webapp/channels/src/components/root/root.tsx | 5 -- webapp/channels/src/i18n/en.json | 2 - 8 files changed, 28 insertions(+), 117 deletions(-) delete mode 100644 webapp/channels/src/components/access_problem/access_problem.scss delete mode 100644 webapp/channels/src/components/access_problem/index.tsx diff --git a/e2e-tests/cypress/tests/integration/channels/auth_sso/authentication_4_spec.ts b/e2e-tests/cypress/tests/integration/channels/auth_sso/authentication_4_spec.ts index 64234aecb66..24d8a049b89 100644 --- a/e2e-tests/cypress/tests/integration/channels/auth_sso/authentication_4_spec.ts +++ b/e2e-tests/cypress/tests/integration/channels/auth_sso/authentication_4_spec.ts @@ -209,12 +209,8 @@ describe('Authentication', () => { // # Go to front page cy.visit('/login'); - // * Assert that create account button is visible - cy.findByText('Don\'t have an account?', {timeout: TIMEOUTS.ONE_MIN}).should('be.visible').click(); - - // * Verify redirection to access problem page since account creation is disabled - cy.url().should('include', '/access_problem'); - cy.findByText('Contact your workspace admin'); + // * Assert that create account button is not visible + cy.findByText('Don\'t have an account?', {timeout: TIMEOUTS.ONE_MIN}).should('not.exist'); // # Go to sign up with email page cy.visit('/signup_user_complete'); diff --git a/e2e-tests/cypress/tests/integration/channels/signin_authentication/login_close_server_spec.js b/e2e-tests/cypress/tests/integration/channels/signin_authentication/login_close_server_spec.js index db4a64eebb9..83011c91130 100644 --- a/e2e-tests/cypress/tests/integration/channels/signin_authentication/login_close_server_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/signin_authentication/login_close_server_spec.js @@ -36,8 +36,7 @@ describe('Login page with close server', () => { // Restore backed up settings cy.apiAdminLogin().apiUpdateConfig(oldSettings); }); - it('MM-47222 Should verify access problem page can be reached', () => { - cy.findByText('Don\'t have an account?').should('be.visible').click(); - cy.findByText('Contact your workspace admin').should('be.visible'); + it('MM-47222 Should verify signup link not visible', () => { + cy.findByText('Don\'t have an account?').should('not.exist'); }); }); diff --git a/webapp/channels/src/components/access_problem/access_problem.scss b/webapp/channels/src/components/access_problem/access_problem.scss deleted file mode 100644 index 7386478f691..00000000000 --- a/webapp/channels/src/components/access_problem/access_problem.scss +++ /dev/null @@ -1,40 +0,0 @@ -.header-footer-route .header-footer-route-container { - display: flex; - justify-content: space-between; -} - -.AccessProblem { - &__body { - display: flex; - flex-direction: column; - align-items: center; - justify-content: center; - } - - &__title { - display: flex; - align-items: center; - margin: 32px 0 16px 0; - color: var(--portal-denim); - font-family: 'Metropolis'; - font-size: 22px; - font-style: normal; - font-weight: 600; - line-height: 28px; - text-align: center; - } - - &__description { - display: flex; - max-width: 640px; - align-items: center; - padding: 0 40px; - color: var(--portal-denim); - font-family: 'Open Sans'; - font-size: 14px; - font-style: normal; - font-weight: 400; - line-height: 20px; - text-align: center; - } -} diff --git a/webapp/channels/src/components/access_problem/index.tsx b/webapp/channels/src/components/access_problem/index.tsx deleted file mode 100644 index 05429c7fbe0..00000000000 --- a/webapp/channels/src/components/access_problem/index.tsx +++ /dev/null @@ -1,51 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import React, {useCallback, useEffect} from 'react'; -import {useIntl} from 'react-intl'; -import {useHistory} from 'react-router-dom'; - -import AccessProblemSVG from 'components/common/svg_images_components/access_problem_svg'; -import type {CustomizeHeaderType} from 'components/header_footer_route/header_footer_route'; - -import './access_problem.scss'; - -type AccessProblemProps = { - onCustomizeHeader?: CustomizeHeaderType; -} - -const AccessProblem = ({ - onCustomizeHeader, -}: AccessProblemProps) => { - const {formatMessage} = useIntl(); - const history = useHistory(); - - const handleHeaderBackButtonOnClick = useCallback(() => { - history.goBack(); - }, [history]); - - useEffect(() => { - if (onCustomizeHeader) { - onCustomizeHeader({ - onBackButtonClick: handleHeaderBackButtonOnClick, - }); - } - }, [onCustomizeHeader, handleHeaderBackButtonOnClick]); - - return ( -
- -
- {formatMessage({id: 'login.contact_admin.title', defaultMessage: 'Contact your workspace admin'})} -
-
- {formatMessage({id: 'login.contact_admin.detail', defaultMessage: "To access your team's workspace, contact your workspace admin. If you've been invited already, check your email inbox for a Mattermost workspace invite."})} -
-
- ); -}; - -export default AccessProblem; diff --git a/webapp/channels/src/components/login/login.test.tsx b/webapp/channels/src/components/login/login.test.tsx index 2f901fb0890..fc87d76a110 100644 --- a/webapp/channels/src/components/login/login.test.tsx +++ b/webapp/channels/src/components/login/login.test.tsx @@ -408,6 +408,25 @@ describe('components/login/Login', () => { expect(history.push).toHaveBeenCalledWith(redirectPath); }); + it('should not show dont have an account when open server is disabled', () => { + const state = mergeObjects(baseState, { + entities: { + general: { + config: { + EnableOpenServer: 'false', + }, + }, + }, + }); + + renderWithContext( + , + state, + ); + + expect(screen.queryByText('Don\'t have an account')).not.toBeInTheDocument(); + }); + describe('EnableGuestMagicLink', () => { it('should show password field when EnableGuestMagicLink is false', async () => { const state = mergeObjects(baseState, { diff --git a/webapp/channels/src/components/login/login.tsx b/webapp/channels/src/components/login/login.tsx index f44c9185e01..4466cac1ca9 100644 --- a/webapp/channels/src/components/login/login.tsx +++ b/webapp/channels/src/components/login/login.tsx @@ -437,23 +437,18 @@ const Login = ({onCustomizeHeader}: LoginProps) => { }, [sessionExpired, formatMessage, onDismissSessionExpired, extraParam, siteName, searchParam]); const getAlternateLink = useCallback(() => { + if (!showSignup) { + return undefined; + } + const linkLabel = formatMessage({ id: 'login.noAccount', defaultMessage: 'Don\'t have an account?', }); - if (showSignup) { - return ( - - ); - } return ( ); diff --git a/webapp/channels/src/components/root/root.tsx b/webapp/channels/src/components/root/root.tsx index 38c71e840f0..f7db6c6cb3f 100644 --- a/webapp/channels/src/components/root/root.tsx +++ b/webapp/channels/src/components/root/root.tsx @@ -50,7 +50,6 @@ const MobileViewWatcher = makeAsyncComponent('MobileViewWatcher', lazy(() => imp const WindowSizeObserver = makeAsyncComponent('WindowSizeObserver', lazy(() => import('components/window_size_observer/WindowSizeObserver'))); const ErrorPage = makeAsyncComponent('ErrorPage', lazy(() => import('components/error_page'))); const Login = makeAsyncComponent('LoginController', lazy(() => import('components/login/login'))); -const AccessProblem = makeAsyncComponent('AccessProblem', lazy(() => import('components/access_problem'))); const PasswordResetSendLink = makeAsyncComponent('PasswordResedSendLink', lazy(() => import('components/password_reset_send_link'))); const PasswordResetForm = makeAsyncComponent('PasswordResetForm', lazy(() => import('components/password_reset_form'))); const Signup = makeAsyncComponent('SignupController', lazy(() => import('components/signup/signup'))); @@ -320,10 +319,6 @@ export default class Root extends React.PureComponent { path={'/login'} component={Login} /> - Date: Thu, 9 Apr 2026 13:20:50 +0530 Subject: [PATCH 03/10] Upgraded board prepackaged version to v9.2.4 (#35969) --- server/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/Makefile b/server/Makefile index f7bf86a6544..63505b41f8e 100644 --- a/server/Makefile +++ b/server/Makefile @@ -165,7 +165,7 @@ PLUGIN_PACKAGES += mattermost-plugin-playbooks-v2.8.0 PLUGIN_PACKAGES += mattermost-plugin-servicenow-v2.4.0 PLUGIN_PACKAGES += mattermost-plugin-zoom-v1.12.0 PLUGIN_PACKAGES += mattermost-plugin-agents-v1.7.2 -PLUGIN_PACKAGES += mattermost-plugin-boards-v9.2.2 +PLUGIN_PACKAGES += mattermost-plugin-boards-v9.2.4 PLUGIN_PACKAGES += mattermost-plugin-user-survey-v1.1.1 PLUGIN_PACKAGES += mattermost-plugin-mscalendar-v1.6.0 PLUGIN_PACKAGES += mattermost-plugin-msteams-meetings-v2.4.1 From d1ca297721b75700b212370e339ed37ae1b1d6b1 Mon Sep 17 00:00:00 2001 From: Pavel Zeman Date: Thu, 9 Apr 2026 05:06:42 -0400 Subject: [PATCH 04/10] Revert "Strip remote_id field from user patch API requests (#35910)" (#35996) This reverts commit fc9d3be3689d7218f9bd5dd8699e74b33c440911 which introduced a CI regression. --- server/channels/api4/apitestlib.go | 10 -- server/channels/api4/shared_channel_test.go | 18 +++- server/channels/api4/user.go | 2 - server/channels/api4/user_test.go | 98 ------------------- server/channels/app/channel_test.go | 8 +- server/channels/app/helper_test.go | 10 -- ..._global_user_sync_self_referential_test.go | 2 +- ...l_membership_sync_self_referential_test.go | 4 +- server/channels/app/shared_channel_test.go | 11 +-- server/channels/app/user_test.go | 5 +- server/channels/store/sqlstore/user_store.go | 1 - server/channels/testlib/helper.go | 7 -- 12 files changed, 33 insertions(+), 143 deletions(-) diff --git a/server/channels/api4/apitestlib.go b/server/channels/api4/apitestlib.go index 3a811a27187..facb392d651 100644 --- a/server/channels/api4/apitestlib.go +++ b/server/channels/api4/apitestlib.go @@ -1409,16 +1409,6 @@ func (th *TestHelper) SetupScheme(tb testing.TB, scope string) *model.Scheme { return scheme } -func (th *TestHelper) SetUserRemoteID(tb testing.TB, userID, remoteID string) *model.User { - tb.Helper() - err := testlib.SetUserRemoteID(mainHelper.GetSQLStore(), userID, remoteID) - require.NoError(tb, err) - th.App.InvalidateCacheForUser(userID) - user, appErr := th.App.GetUser(userID) - require.Nil(tb, appErr) - return user -} - func (th *TestHelper) Parallel(t *testing.T) { mainHelper.Parallel(t) } diff --git a/server/channels/api4/shared_channel_test.go b/server/channels/api4/shared_channel_test.go index 26b900809db..1926ab09d46 100644 --- a/server/channels/api4/shared_channel_test.go +++ b/server/channels/api4/shared_channel_test.go @@ -221,7 +221,10 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) { }() localUser := th.BasicUser - remoteUser := th.SetUserRemoteID(t, th.CreateUser(t).Id, model.NewId()) + remoteUser := th.CreateUser(t) + remoteUser.RemoteId = model.NewPointer(model.NewId()) + remoteUser, appErr := th.App.UpdateUser(th.Context, remoteUser, false) + require.Nil(t, appErr) dm, _, err := client.CreateDirectChannel(context.Background(), localUser.Id, remoteUser.Id) require.Error(t, err) @@ -240,7 +243,10 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) { }() localUser := th.BasicUser - remoteUser := th.SetUserRemoteID(t, th.CreateUser(t).Id, model.NewId()) + remoteUser := th.CreateUser(t) + remoteUser.RemoteId = model.NewPointer(model.NewId()) + remoteUser, appErr := th.App.UpdateUser(th.Context, remoteUser, false) + require.Nil(t, appErr) dm, _, err := client.CreateDirectChannel(context.Background(), localUser.Id, remoteUser.Id) require.NoError(t, err) @@ -272,7 +278,9 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) { rc, appErr := th.App.AddRemoteCluster(rc) require.Nil(t, appErr) - th.SetUserRemoteID(t, remoteUser.Id, rc.RemoteId) + remoteUser.RemoteId = model.NewPointer(rc.RemoteId) + remoteUser, appErr = th.App.UpdateUser(th.Context, remoteUser, false) + require.Nil(t, appErr) dm, _, err := client.CreateDirectChannel(context.Background(), localUser.Id, remoteUser.Id) require.NoError(t, err) @@ -304,7 +312,9 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) { rc, appErr := th.App.AddRemoteCluster(rc) require.Nil(t, appErr) - th.SetUserRemoteID(t, remoteUser.Id, rc.RemoteId) + remoteUser.RemoteId = model.NewPointer(rc.RemoteId) + remoteUser, appErr = th.App.UpdateUser(th.Context, remoteUser, false) + require.Nil(t, appErr) dm, _, err := client.CreateDirectChannel(context.Background(), remoteUser.Id, localUser.Id) require.NoError(t, err) diff --git a/server/channels/api4/user.go b/server/channels/api4/user.go index 91d996dfcc4..584cefd8bbf 100644 --- a/server/channels/api4/user.go +++ b/server/channels/api4/user.go @@ -1450,8 +1450,6 @@ func patchUser(c *Context, w http.ResponseWriter, r *http.Request) { return } - patch.RemoteId = nil - auditRec := c.MakeAuditRecord(model.AuditEventPatchUser, model.AuditStatusFail) model.AddEventParameterAuditableToAuditRec(auditRec, "user_patch", &patch) defer c.LogAuditRec(auditRec) diff --git a/server/channels/api4/user_test.go b/server/channels/api4/user_test.go index fbf844e6330..cc25a3f72a0 100644 --- a/server/channels/api4/user_test.go +++ b/server/channels/api4/user_test.go @@ -2316,50 +2316,6 @@ func TestUpdateUser(t *testing.T) { }) } -func TestUpdateUserRemoteIdIgnored(t *testing.T) { - mainHelper.Parallel(t) - th := Setup(t) - - t.Run("remote_id in update body is ignored for regular user", func(t *testing.T) { - user := th.CreateUser(t) - _, _, err := th.Client.Login(context.Background(), user.Email, user.Password) - require.NoError(t, err) - - user.RemoteId = model.NewPointer("attacker-remote-id") - user.Nickname = "updated-nickname" - ruser, _, err := th.Client.UpdateUser(context.Background(), user) - require.NoError(t, err) - require.Equal(t, "updated-nickname", ruser.Nickname) - require.Empty(t, model.SafeDereference(ruser.RemoteId), "remote_id should remain empty") - - dbUser, appErr := th.App.GetUser(user.Id) - require.Nil(t, appErr) - require.Empty(t, model.SafeDereference(dbUser.RemoteId), "remote_id should not be persisted") - }) - - t.Run("remote_id in update body is ignored for system admin", func(t *testing.T) { - user := th.CreateUser(t) - - user.RemoteId = model.NewPointer("admin-remote-id") - user.Nickname = "admin-updated" - ruser, _, err := th.SystemAdminClient.UpdateUser(context.Background(), user) - require.NoError(t, err) - require.Equal(t, "admin-updated", ruser.Nickname) - require.Empty(t, model.SafeDereference(ruser.RemoteId), "remote_id should remain empty even for admin") - }) - - t.Run("existing remote_id is preserved when updating other fields", func(t *testing.T) { - remoteId := model.NewId() - user := th.SetUserRemoteID(t, th.CreateUser(t).Id, remoteId) - - user.Nickname = "updated-nickname" - ruser, _, err := th.SystemAdminClient.UpdateUser(context.Background(), user) - require.NoError(t, err) - require.Equal(t, "updated-nickname", ruser.Nickname) - require.Equal(t, remoteId, model.SafeDereference(ruser.RemoteId), "existing remote_id should be preserved") - }) -} - func TestUpdateAdminUser(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t) @@ -2518,60 +2474,6 @@ func TestPatchUser(t *testing.T) { require.NoError(t, err) } -func TestPatchUserRemoteIdIgnored(t *testing.T) { - mainHelper.Parallel(t) - th := Setup(t).InitBasic(t) - - t.Run("remote_id in patch is ignored for regular user", func(t *testing.T) { - user := th.CreateUser(t) - _, _, err := th.Client.Login(context.Background(), user.Email, user.Password) - require.NoError(t, err) - - patch := &model.UserPatch{ - RemoteId: model.NewPointer("attacker-remote-id"), - Nickname: model.NewPointer("new-nickname"), - } - ruser, _, err := th.Client.PatchUser(context.Background(), user.Id, patch) - require.NoError(t, err) - require.Equal(t, "new-nickname", ruser.Nickname) - require.Empty(t, model.SafeDereference(ruser.RemoteId), "remote_id should remain empty") - - dbUser, appErr := th.App.GetUser(user.Id) - require.Nil(t, appErr) - require.Empty(t, model.SafeDereference(dbUser.RemoteId), "remote_id should not be persisted") - }) - - t.Run("remote_id in patch is ignored for system admin", func(t *testing.T) { - user := th.CreateUser(t) - - patch := &model.UserPatch{ - RemoteId: model.NewPointer("admin-remote-id"), - Nickname: model.NewPointer("admin-patched"), - } - ruser, _, err := th.SystemAdminClient.PatchUser(context.Background(), user.Id, patch) - require.NoError(t, err) - require.Equal(t, "admin-patched", ruser.Nickname) - require.Empty(t, model.SafeDereference(ruser.RemoteId), "remote_id should remain empty even for admin") - - dbUser, appErr := th.App.GetUser(user.Id) - require.Nil(t, appErr) - require.Empty(t, model.SafeDereference(dbUser.RemoteId), "remote_id should not be persisted even for admin") - }) - - t.Run("existing remote_id is preserved when patching other fields", func(t *testing.T) { - remoteId := model.NewId() - user := th.SetUserRemoteID(t, th.CreateUser(t).Id, remoteId) - - patch := &model.UserPatch{ - Nickname: model.NewPointer("updated-nickname"), - } - ruser, _, err := th.SystemAdminClient.PatchUser(context.Background(), user.Id, patch) - require.NoError(t, err) - require.Equal(t, "updated-nickname", ruser.Nickname) - require.Equal(t, remoteId, model.SafeDereference(ruser.RemoteId), "existing remote_id should be preserved") - }) -} - func TestPatchBotUser(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t) diff --git a/server/channels/app/channel_test.go b/server/channels/app/channel_test.go index 776c2d2ab07..21592cdde2f 100644 --- a/server/channels/app/channel_test.go +++ b/server/channels/app/channel_test.go @@ -599,7 +599,9 @@ func TestGetOrCreateDirectChannel(t *testing.T) { }) t.Run("Cannot create with a remote user", func(t *testing.T) { - th.SetUserRemoteID(t, user2.Id, model.NewId()) + user2.RemoteId = model.NewPointer(model.NewId()) + _, appErr := th.App.UpdateUser(th.Context, user2, false) + require.Nil(t, appErr) dm, appErr := th.App.GetOrCreateDirectChannel(th.Context, user1.Id, user2.Id) require.Nil(t, dm) @@ -620,7 +622,9 @@ func TestCreateGroupChannel(t *testing.T) { groupUserIds = append(groupUserIds, th.BasicUser.Id) t.Run("Should not allow to create a group with a remote user", func(t *testing.T) { - th.SetUserRemoteID(t, user2.Id, model.NewId()) + user2.RemoteId = model.NewPointer(model.NewId()) + _, appErr := th.App.UpdateUser(th.Context, user2, false) + require.Nil(t, appErr) dm, appErr := th.App.CreateGroupChannel(th.Context, groupUserIds, th.BasicUser.Id) require.NotNil(t, appErr) diff --git a/server/channels/app/helper_test.go b/server/channels/app/helper_test.go index 3a4f990d716..223be342b94 100644 --- a/server/channels/app/helper_test.go +++ b/server/channels/app/helper_test.go @@ -814,16 +814,6 @@ func decodeJSON[T any](tb testing.TB, o any, result *T) *T { return result } -func (th *TestHelper) SetUserRemoteID(tb testing.TB, userID, remoteID string) *model.User { - tb.Helper() - err := testlib.SetUserRemoteID(th.GetSqlStore(), userID, remoteID) - require.NoError(tb, err) - th.App.InvalidateCacheForUser(userID) - user, appErr := th.App.GetUser(userID) - require.Nil(tb, appErr) - return user -} - func (th *TestHelper) Parallel(t *testing.T) { mainHelper.Parallel(t) } diff --git a/server/channels/app/shared_channel_global_user_sync_self_referential_test.go b/server/channels/app/shared_channel_global_user_sync_self_referential_test.go index 3f5ffbd5174..64a9e2c790b 100644 --- a/server/channels/app/shared_channel_global_user_sync_self_referential_test.go +++ b/server/channels/app/shared_channel_global_user_sync_self_referential_test.go @@ -227,7 +227,7 @@ func TestSharedChannelGlobalUserSyncSelfReferential(t *testing.T) { // Create remote user (should NOT be synced) remoteUser := th.CreateUser(t) - remoteUser = th.SetUserRemoteID(t, remoteUser.Id, selfCluster.RemoteId) + remoteUser.RemoteId = &selfCluster.RemoteId remoteUser.UpdateAt = baseTime + 600 _, err = ss.User().Update(th.Context, remoteUser, true) require.NoError(t, err) diff --git a/server/channels/app/shared_channel_membership_sync_self_referential_test.go b/server/channels/app/shared_channel_membership_sync_self_referential_test.go index 991acc86725..ade5f29c9e4 100644 --- a/server/channels/app/shared_channel_membership_sync_self_referential_test.go +++ b/server/channels/app/shared_channel_membership_sync_self_referential_test.go @@ -984,7 +984,9 @@ func TestSharedChannelMembershipSyncSelfReferential(t *testing.T) { // Create a remote user belonging to cluster-2 userFromCluster2 := th.CreateUser(t) - userFromCluster2 = th.SetUserRemoteID(t, userFromCluster2.Id, clusters[1].RemoteId) + userFromCluster2.RemoteId = &clusters[1].RemoteId + userFromCluster2, appErr = th.App.UpdateUser(th.Context, userFromCluster2, false) + require.Nil(t, appErr) _, _, appErr = th.App.AddUserToTeam(th.Context, team.Id, userFromCluster2.Id, th.BasicUser.Id) require.Nil(t, appErr) diff --git a/server/channels/app/shared_channel_test.go b/server/channels/app/shared_channel_test.go index fc851fce009..40a482b83ee 100644 --- a/server/channels/app/shared_channel_test.go +++ b/server/channels/app/shared_channel_test.go @@ -598,13 +598,12 @@ func TestTransformMentionsOnReceive(t *testing.T) { // Helper to create test users createUser := func(username string, remoteId *string) *model.User { user := th.CreateUser(t) - if remoteId != nil { - user = th.SetUserRemoteID(t, user.Id, *remoteId) - } user.Username = username - var appErr *model.AppError - user, appErr = th.App.UpdateUser(th.Context, user, false) - require.Nil(t, appErr) + if remoteId != nil { + user.RemoteId = remoteId + } + user, updateErr := th.App.UpdateUser(th.Context, user, false) + require.Nil(t, updateErr) th.LinkUserToTeam(t, user, th.BasicTeam) th.AddUserToChannel(t, user, sharedChannel) return user diff --git a/server/channels/app/user_test.go b/server/channels/app/user_test.go index 12624bc20a0..813e9eeb727 100644 --- a/server/channels/app/user_test.go +++ b/server/channels/app/user_test.go @@ -2602,7 +2602,10 @@ func createTestRemoteCluster(t *testing.T, th *TestHelper, ss store.Store, name, func createRemoteUser(t *testing.T, th *TestHelper, remoteCluster *model.RemoteCluster) *model.User { user := th.CreateUser(t) - return th.SetUserRemoteID(t, user.Id, remoteCluster.RemoteId) + user.RemoteId = &remoteCluster.RemoteId + updatedUser, appErr := th.App.UpdateUser(th.Context, user, false) + require.Nil(t, appErr) + return updatedUser } func ensureRemoteClusterConnected(t *testing.T, ss store.Store, cluster *model.RemoteCluster, connected bool) { diff --git a/server/channels/store/sqlstore/user_store.go b/server/channels/store/sqlstore/user_store.go index 08d98d8abec..469b4488d0e 100644 --- a/server/channels/store/sqlstore/user_store.go +++ b/server/channels/store/sqlstore/user_store.go @@ -287,7 +287,6 @@ func (us SqlUserStore) Update(rctx request.CTX, user *model.User, trustedUpdateD user.MfaActive = oldUser.MfaActive user.MfaUsedTimestamps = oldUser.MfaUsedTimestamps user.LastLogin = oldUser.LastLogin - user.RemoteId = oldUser.RemoteId if !trustedUpdateData { user.Roles = oldUser.Roles diff --git a/server/channels/testlib/helper.go b/server/channels/testlib/helper.go index 3aad9f33671..94e0e6ae1ec 100644 --- a/server/channels/testlib/helper.go +++ b/server/channels/testlib/helper.go @@ -342,13 +342,6 @@ func (h *MainHelper) GetSQLStore() *sqlstore.SqlStore { return h.SQLStore } -// SetUserRemoteID sets the RemoteId on a user via direct SQL, bypassing the -// store's Update method which preserves RemoteId from the existing row. -func SetUserRemoteID(sqlStore *sqlstore.SqlStore, userID, remoteID string) error { - _, err := sqlStore.GetMaster().Exec("UPDATE Users SET RemoteId = ? WHERE Id = ?", remoteID, userID) - return err -} - func (h *MainHelper) GetClusterInterface() *FakeClusterInterface { if h.ClusterInterface == nil { panic("MainHelper not initialized with cluster interface.") From cf102afc17f0efbd512994a491e742b107730fcf Mon Sep 17 00:00:00 2001 From: Pavel Zeman Date: Thu, 9 Apr 2026 05:44:50 -0400 Subject: [PATCH 05/10] ci: disable fullyparallel for binary parameters job (#35995) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Binary parameters tests run unsharded on a single runner. With fullyparallel enabled, all ~755 api4 tests run concurrently, causing resource exhaustion (too many server instances, WebSocket hubs, and DB connections). The test binary gets killed after 11 minutes with no individual test failures — just overwhelmed resources. Disabling fullyparallel for this specific job lets binary parameters tests pass while we evaluate moving them to a nightly/weekly schedule. Co-authored-by: Claude --- .github/workflows/server-ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/server-ci.yml b/.github/workflows/server-ci.yml index 5ca710d3120..2c80b1761fb 100644 --- a/.github/workflows/server-ci.yml +++ b/.github/workflows/server-ci.yml @@ -208,6 +208,7 @@ jobs: logsartifact: postgres-binary-server-test-logs go-version: ${{ needs.go.outputs.version }} fips-enabled: false + fullyparallel: false # -- Sharded into 4 parallel runners for ~88% wall-time improvement -- test-postgres-normal: name: Postgres (shard ${{ matrix.shard }}) From 6878d095476d3fdbbafd93ee4df99b79262af151 Mon Sep 17 00:00:00 2001 From: Vicktor <79470910+Victor-Nyagudi@users.noreply.github.com> Date: Thu, 9 Apr 2026 16:16:23 +0300 Subject: [PATCH 06/10] refactor(brand_image_setting): migrate BrandImageSetting to a function component (#34536) * refactor(brand_image_setting): migrate to function component * test(brand_image_setting): update tests Migrated tests to React Testing Library. * refactor(brand_image_setting): wrap functions with useCallback * test(brand_image_setting): use nock to mock fetch api * test(brand_image_setting): use findby query instead of getby * test(brand_image_setting): remove unnecessary scope assertion * chore(brand_image_setting): split useEffect into two Also extracted the handleSave function and wrapped it in useCallback. * test(brand_image_setting): add e2e test for deleting brand image * test(brand_image_setting): use destructured functions * chore: delete unnecessary comment * Revert "test(brand_image_setting): use destructured functions" This reverts commit 71dc6628eda6e9af44e9dd1de490a753d0277d4d. * Fix bad merge * Fully revert changes to test from merge --------- Co-authored-by: Mattermost Build Co-authored-by: Harrison Healey --- .../ui_and_api/customization_spec.js | 20 +- .../brand_image_setting.test.tsx | 114 +++--- .../brand_image_setting.tsx | 377 +++++++++--------- 3 files changed, 244 insertions(+), 267 deletions(-) diff --git a/e2e-tests/cypress/tests/integration/channels/system_console/ui_and_api/customization_spec.js b/e2e-tests/cypress/tests/integration/channels/system_console/ui_and_api/customization_spec.js index d956613d9b8..8873927b318 100644 --- a/e2e-tests/cypress/tests/integration/channels/system_console/ui_and_api/customization_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/system_console/ui_and_api/customization_spec.js @@ -49,10 +49,28 @@ describe('Customization', () => { // # Save setting saveSetting(); - // # Verify that after page reload image exist cy.reload(); cy.findByTestId('CustomBrandImage').should('be.visible').within(() => { + // * Verify that after page reload image exist cy.get('img').should('have.attr', 'src').and('include', '/api/v4/brand/image?t='); + + // * Verify that there's an option to delete the image. + cy.findByTestId('remove-image__btn').should('be.visible'); + + // # delete the image + cy.findByTestId('remove-image__btn').click(); + }); + + // # Save setting + saveSetting(); + + cy.reload(); + cy.findByTestId('CustomBrandImage').should('be.visible').within(() => { + // * Verify that after page reload, the image doesn't exist. + cy.findByAltText('brand image').should('not.exist'); + + // * Verify there's no option to delete the image. + cy.findByTestId('remove-image__btn').should('not.exist'); }); }); diff --git a/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.test.tsx b/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.test.tsx index 6f65ef09781..93ea64e1841 100644 --- a/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.test.tsx +++ b/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.test.tsx @@ -1,30 +1,18 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. +import nock from 'nock'; import React from 'react'; -import {uploadBrandImage, deleteBrandImage} from 'actions/admin_actions.jsx'; +import {Client4} from 'mattermost-redux/client'; import {renderWithContext, screen, userEvent, waitFor} from 'tests/react_testing_utils'; import BrandImageSetting from './brand_image_setting'; -// Real implementations are async (await dispatch(...)); mocks must return Promises so handleSave can await them. -jest.mock('actions/admin_actions.jsx', () => ({ - ...jest.requireActual('actions/admin_actions.jsx'), - uploadBrandImage: jest.fn(async () => {}), - deleteBrandImage: jest.fn(async () => {}), -})); +Client4.setUrl('http://localhost:8065'); describe('components/admin_console/brand_image_setting', () => { - beforeEach(() => { - jest.spyOn(global, 'fetch').mockResolvedValue({status: 404} as Response); - }); - - afterEach(() => { - jest.restoreAllMocks(); - }); - const baseProps = { disabled: false, setSaveNeeded: jest.fn(), @@ -32,71 +20,59 @@ describe('components/admin_console/brand_image_setting', () => { unRegisterSaveAction: jest.fn(), }; - test('should have called deleteBrandImage or uploadBrandImage on save depending on component state', async () => { - let saveAction: (() => Promise) | undefined; - const registerSaveAction = jest.fn((fn: () => Promise) => { - saveAction = fn; - }); + const deleteButtonTestId = 'remove-image__btn'; - const {container, unmount} = renderWithContext( - , - ); + let scope: nock.Scope; - // Wait for componentDidMount fetch to resolve - await waitFor(() => { - expect(registerSaveAction).toHaveBeenCalled(); - }); - expect(saveAction).toBeDefined(); + beforeAll(() => { + scope = nock(Client4.getBaseRoute()).persist().get('/brand/image').query(true).reply(200); + }); - // Simulate selecting a file via the file input to set brandImage - const file = new File(['brand_image_file'], 'brand.png', {type: 'image/png'}); - const fileInput = container.querySelector('input[type="file"]'); - expect(fileInput).toBeInTheDocument(); - await userEvent.upload(fileInput as HTMLInputElement, file); + afterAll(() => { + nock.cleanAll(); + }); - // Now call save - should call uploadBrandImage - await saveAction!(); - expect(deleteBrandImage).toHaveBeenCalledTimes(0); - expect(uploadBrandImage).toHaveBeenCalledTimes(1); + test('should register and unregister save handler when mounted and unmounted respectively', () => { + const {unmount} = renderWithContext(); + + expect(baseProps.registerSaveAction).toHaveBeenCalledTimes(1); - // To test deleteBrandImage path, unmount then re-mount with fetch returning 200 unmount(); - jest.clearAllMocks(); - (global.fetch as jest.Mock).mockResolvedValueOnce({status: 200} as Response); - let saveAction2: (() => Promise) | undefined; - const registerSaveAction2 = jest.fn((fn: () => Promise) => { - saveAction2 = fn; - }); + expect(baseProps.unRegisterSaveAction).toHaveBeenCalledTimes(1); + }); - renderWithContext( - , - ); + test('should show delete button if brand image exists', async () => { + renderWithContext(); - await waitFor(() => { - expect(registerSaveAction2).toHaveBeenCalled(); - }); - expect(saveAction2).toBeDefined(); + await waitFor(() => expect(scope.isDone()).toBe(true)); + + expect(screen.getByTestId(deleteButtonTestId)).toBeVisible(); + }); + + test('should hide delete button if the setting is disabled', async () => { + const props = {...baseProps, disabled: true}; + + renderWithContext(); + + await waitFor(() => expect(screen.queryByTestId(deleteButtonTestId)).toBe(null)); + }); + + test('should call setSaveNeeded when a brand image is uploaded', async () => { + renderWithContext(); + + await userEvent.upload(screen.getByTestId('file__upload-input'), new File(['brand_image_file'], 'brand_image_file.png', {type: 'image/png'})); + + expect(baseProps.setSaveNeeded).toHaveBeenCalledTimes(1); + }); + + test('should call setSaveNeeded when the delete button is pressed', async () => { + renderWithContext(); + + const deleteButton = await screen.findByTestId(deleteButtonTestId); - // Wait for the brand image to be detected and delete button to appear - await waitFor(() => { - expect(screen.getByText('×')).toBeInTheDocument(); - }); - const deleteButton = screen.getByText('×').closest('button')!; await userEvent.click(deleteButton); - await waitFor(() => { - expect(screen.getByText('No brand image uploaded')).toBeInTheDocument(); - }); - - await saveAction2!(); - expect(deleteBrandImage).toHaveBeenCalledTimes(1); - expect(uploadBrandImage).toHaveBeenCalledTimes(0); + expect(baseProps.setSaveNeeded).toHaveBeenCalledTimes(1); }); }); diff --git a/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.tsx b/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.tsx index f058e9bbc0c..7a155f07304 100644 --- a/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.tsx +++ b/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.tsx @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React from 'react'; +import React, {memo, useCallback, useEffect, useRef, useState} from 'react'; import {FormattedMessage} from 'react-intl'; import {Client4} from 'mattermost-redux/client'; @@ -9,6 +9,7 @@ import {Client4} from 'mattermost-redux/client'; import {uploadBrandImage, deleteBrandImage} from 'actions/admin_actions.jsx'; import SettingSet from 'components/admin_console/setting_set'; +import useDidUpdate from 'components/common/hooks/useDidUpdate'; import FormError from 'components/form_error'; import WithTooltip from 'components/with_tooltip'; @@ -44,242 +45,224 @@ type Props = { unRegisterSaveAction: (saveAction: () => Promise) => void; }; -type State = { - deleteBrandImage: boolean; - brandImage?: Blob; - brandImageExists: boolean; - brandImageTimestamp: number; - error: string; -}; +const BrandImageSetting = ({ + id, + disabled, + setSaveNeeded, + registerSaveAction, + unRegisterSaveAction, +}: Props) => { + const imageRef = useRef(null); + const fileInputRef = useRef(null); -export default class BrandImageSetting extends React.PureComponent { - private imageRef: React.RefObject; - private fileInputRef: React.RefObject; + const [brandImage, setBrandImage] = useState(); + const [shouldDeleteBrandImage, setShouldDeleteBrandImage] = useState(false); + const [brandImageExists, setBrandImageExists] = useState(false); + const [brandImageTimestamp, setBrandImageTimestamp] = useState(Date.now()); - constructor(props: Props) { - super(props); + const [errorFromState, setErrorFromState] = useState(''); - this.state = { - deleteBrandImage: false, - brandImageExists: false, - brandImageTimestamp: Date.now(), - error: '', - }; + const handleSave = useCallback(async () => { + setErrorFromState(''); - this.imageRef = React.createRef(); - this.fileInputRef = React.createRef(); - } + let error; + if (shouldDeleteBrandImage) { + await deleteBrandImage( + () => { + setShouldDeleteBrandImage(false); + setBrandImageExists(false); + setBrandImage(undefined); + }, + (err: Error) => { + error = err; + setErrorFromState(err.message); + }, + ); + } else if (brandImage) { + await uploadBrandImage( + brandImage, + () => { + setBrandImageExists(true); + setBrandImage(undefined); + setBrandImageTimestamp(Date.now()); + }, + (err: Error) => { + error = err; + setErrorFromState(err.message); + }, + ); + } + return {error}; + }, [brandImage, shouldDeleteBrandImage]); - componentDidMount() { + useEffect(() => { fetch( - Client4.getBrandImageUrl(String(this.state.brandImageTimestamp)), + Client4.getBrandImageUrl(String(Date.now())), ).then((resp) => { if (resp.status === HTTP_STATUS_OK) { - this.setState({brandImageExists: true}); + setBrandImageExists(true); } else { - this.setState({brandImageExists: false}); + setBrandImageExists(false); } }).catch((err) => { console.error(`unable to retrieve brand image: ${err}`); //eslint-disable-line no-console - this.setState({brandImageExists: false}); + setBrandImageExists(false); }); + }, []); - this.props.registerSaveAction(this.handleSave); - } + useEffect(() => { + registerSaveAction(handleSave); - componentWillUnmount() { - this.props.unRegisterSaveAction(this.handleSave); - } + return () => { + unRegisterSaveAction(handleSave); + }; + }, [handleSave, registerSaveAction, unRegisterSaveAction]); - componentDidUpdate() { - if (this.imageRef.current) { + useDidUpdate(() => { + if (imageRef.current) { const reader = new FileReader(); - const img = this.imageRef.current; + const img = imageRef.current; reader.onload = (e) => { const src = - e.target?.result instanceof ArrayBuffer ? e.target?.result.toString() : e.target?.result; + e.target?.result instanceof ArrayBuffer ? e.target?.result.toString() : e.target?.result; if (src) { img.setAttribute('src', src); } }; - if (this.state.brandImage) { - reader.readAsDataURL(this.state.brandImage); + if (brandImage) { + reader.readAsDataURL(brandImage); } } - } + }, [brandImage]); - handleSelectClick = () => { - this.fileInputRef.current?.click(); - }; + const handleSelectClick = useCallback(() => { + fileInputRef.current?.click(); + }, []); - handleImageChange = () => { - if (!this.fileInputRef.current) { + const handleImageChange = useCallback(() => { + if (!fileInputRef.current) { return; } - const element = this.fileInputRef.current; + const element = fileInputRef.current; if (element.files && element.files.length > 0) { - this.props.setSaveNeeded(); - this.setState({ - brandImage: element.files[0], - deleteBrandImage: false, - }); + setSaveNeeded(); + setBrandImage(element.files[0]); + setShouldDeleteBrandImage(false); } - }; + }, [setSaveNeeded]); - handleDeleteButtonPressed = () => { - this.setState({ - deleteBrandImage: true, - brandImage: undefined, - brandImageExists: false, - }); - this.props.setSaveNeeded(); - }; + const handleDeleteButtonPressed = useCallback(() => { + setShouldDeleteBrandImage(true); + setBrandImage(undefined); + setBrandImageExists(false); - handleSave = async () => { - this.setState({ - error: '', - }); + setSaveNeeded(); + }, [setSaveNeeded]); - let error; - if (this.state.deleteBrandImage) { - await deleteBrandImage( - () => { - this.setState({ - deleteBrandImage: false, - brandImageExists: false, - brandImage: undefined, - }); - }, - (err: Error) => { - error = err; - this.setState({ - error: err.message, - }); - }, - ); - } else if (this.state.brandImage) { - await uploadBrandImage( - this.state.brandImage, - () => { - this.setState({ - brandImageExists: true, - brandImage: undefined, - brandImageTimestamp: Date.now(), - }); - }, - (err: Error) => { - error = err; - this.setState({ - error: err.message, - }); - }, - ); - } - return {error}; - }; - - render() { - let img = null; - if (this.state.brandImage) { - img = ( -
- brand image -
- ); - } else if (this.state.brandImageExists) { - let overlay; - if (!this.props.disabled) { - overlay = ( - - )} - isVertical={false} - > - - - ); - } - img = ( -
- brand image - {overlay} -
- ); - } else { - img = ( -

- -

- ); - } - - return ( - - } - label={ - - } - setByEnv={false} - > -
-
{img}
-
-
+ let img = null; + if (brandImage) { + img = ( +
+ brand image +
+ ); + } else if (brandImageExists) { + let overlay; + if (!disabled) { + overlay = ( + + )} + isVertical={false} + > - -
- - + + ); + } + img = ( +
+ brand image + {overlay} +
+ ); + } else { + img = ( +

+ +

); } -} + + return ( + + } + label={ + + } + setByEnv={false} + > +
+
{img}
+
+
+ + +
+ + + ); +}; + +export default memo(BrandImageSetting); From f441b34deeda1c3d0e474518352dc0d3cfaca30e Mon Sep 17 00:00:00 2001 From: Scott Bishel Date: Thu, 9 Apr 2026 07:50:36 -0600 Subject: [PATCH 07/10] Fix interactive dialog bugs: dynamic select lookups, radio values, field refresh (#35640) * Fix interactive dialog bugs: dynamic select lookups, radio values, and field refresh - Cache sanitized fields in AppsForm to preserve object identity across renders, preventing AsyncSelect from remounting and re-triggering dynamic select lookups on every keystroke in any field - Normalize radio field default values to plain strings in getDefaultValue() so the value shape is consistent with what RadioSetting.onChange returns (e.target.value). Accept both string and {label, value} object shapes downstream for backwards compatibility. - Fix radio field [object Object] in submission by extracting .value from AppSelectOption objects in convertAppFormValuesToDialogSubmission - Include selected_field in refresh submission so plugins know which field triggered the refresh. Use a shallow copy of accumulatedValues to avoid permanently contaminating the accumulated state. - Send empty string for cleared select fields in refresh submissions. Previously, extractPrimitiveValues skipped null values and the spread merge never overwrote stale accumulated keys. --- .../apps_form/apps_form_component.tsx | 23 +- .../apps_form_field/apps_form_field.tsx | 6 +- .../interactive_dialog_adapter.test.tsx | 148 ++++++++-- .../interactive_dialog_adapter.tsx | 22 +- .../src/utils/dialog_conversion.test.ts | 257 +++++++++++++++++- .../channels/src/utils/dialog_conversion.ts | 86 +++--- webapp/platform/types/src/apps.ts | 2 +- 7 files changed, 477 insertions(+), 67 deletions(-) diff --git a/webapp/channels/src/components/apps_form/apps_form_component.tsx b/webapp/channels/src/components/apps_form/apps_form_component.tsx index 39efda5be43..27228735ec7 100644 --- a/webapp/channels/src/components/apps_form/apps_form_component.tsx +++ b/webapp/channels/src/components/apps_form/apps_form_component.tsx @@ -237,6 +237,12 @@ const initFormValues = (form: AppForm, timezone?: string): AppFormValues => { }; export class AppsForm extends React.PureComponent { + // Cache sanitized fields to preserve object identity across renders. + // Without this, createSanitizedField() returns a new object every render, + // causing AppsFormSelectField to remount its AsyncSelect (via refreshNonce), + // which re-triggers dynamic select lookups on every keystroke in any field. + private sanitizedFieldCache = new Map(); + constructor(props: Props) { super(props); @@ -271,6 +277,14 @@ export class AppsForm extends React.PureComponent { return null; } + componentDidUpdate(prevProps: Props) { + // Clear sanitized field cache when the form changes (e.g., refresh or multistep) + // so stale entries don't accumulate. + if (prevProps.form !== this.props.form) { + this.sanitizedFieldCache.clear(); + } + } + updateErrors = (elements: DialogElement[], fieldErrors?: {[x: string]: string}, formError?: string): boolean => { let hasErrors = false; const state = {} as State; @@ -657,8 +671,13 @@ export class AppsForm extends React.PureComponent { } return fields.filter((f) => f.name !== form.submit_buttons).map((originalField, index) => { - // Use sanitized field for safe usage in components - const field = createSanitizedField(originalField); + // Use cached sanitized field to preserve object identity across renders. + // This prevents AsyncSelect remounts that trigger spurious lookup calls. + let field = this.sanitizedFieldCache.get(originalField); + if (!field) { + field = createSanitizedField(originalField); + this.sanitizedFieldCache.set(originalField, field); + } return ( { ); } case AppFieldTypes.RADIO: { - const radioValue = value as string; + // Radio values may be stored as AppSelectOption objects (from initial default) + // or plain strings (after user interaction via RadioSetting.onChange) + const radioValue = isAppSelectOption(value) ? value.value : (value as string) ?? ''; return ( { const mockCall = MockAppsFormContainer.mock.calls[0][0]; const submitAdapter = mockCall.actions.doAppSubmit; - // Test with null value for required field - should not crash + // Test with null value for required field - should not crash. + // processFormValues normalizes null to '' in accumulatedValues, + // so the validation sees an empty string (not null) and does not + // produce a required-field error. const result = await submitAdapter({ values: { 'text-field': 'valid', - 'required-field': null, // Missing required field + 'required-field': null, // Cleared field — normalized to '' }, }); - // Should complete successfully (null values are simply skipped) + // Should complete successfully (null values are normalized to empty strings) expect(result.data?.type).toBe('ok'); - expect(mockConsole.warn).toHaveBeenCalledWith( - '[InteractiveDialogAdapter]', - 'Form submission validation errors', - expect.objectContaining({ - errorCount: expect.any(Number), - errors: expect.arrayContaining([ - expect.objectContaining({ - field: expect.stringContaining('required-field'), - message: expect.any(String), - }), - ]), - }), - ); }); }); @@ -2305,5 +2295,131 @@ describe('components/interactive_dialog/InteractiveDialogAdapter', () => { data: {items: []}, }); }); + + test('should include selected_field in refresh submission', async () => { + const mockSubmitDialog = jest.fn().mockResolvedValue({data: {}}); + + const refreshSelectElement: DialogElement = { + name: 'category', + type: 'select', + display_name: 'Category', + help_text: '', + placeholder: '', + default: '', + optional: false, + max_length: 0, + min_length: 0, + subtype: '', + data_source: '', + options: [ + {text: 'Option A', value: 'a'}, + {text: 'Option B', value: 'b'}, + ], + }; + + const props = { + ...baseProps, + sourceUrl: '/plugins/myplugin/refresh', + elements: [refreshSelectElement], + actions: { + submitInteractiveDialog: mockSubmitDialog, + lookupInteractiveDialog: jest.fn().mockResolvedValue({data: {items: []}}), + }, + }; + + const {getByTestId} = renderWithContext( + , + ); + + await waitFor(() => { + expect(getByTestId('apps-form-container')).toBeInTheDocument(); + }); + + // Get the refresh handler from the MockAppsFormContainer + const mockCall = MockAppsFormContainer.mock.calls[0][0]; + const refreshHandler = mockCall.actions.doAppFetchForm; + + // Trigger refresh with selected_field set + await refreshHandler({ + selected_field: 'category', + values: {category: 'a'}, + }); + + expect(mockSubmitDialog).toHaveBeenCalledWith( + expect.objectContaining({ + url: '/plugins/myplugin/refresh', + submission: expect.objectContaining({ + selected_field: 'category', + category: 'a', + }), + }), + ); + }); + + test('should send empty string for cleared select field values in refresh', async () => { + const mockSubmitDialog = jest.fn().mockResolvedValue({data: {}}); + + const refreshSelectElement: DialogElement = { + name: 'priority', + type: 'select', + display_name: 'Priority', + help_text: '', + placeholder: '', + default: '', + optional: true, + max_length: 0, + min_length: 0, + subtype: '', + data_source: '', + options: [ + {text: 'High', value: 'high'}, + {text: 'Low', value: 'low'}, + ], + }; + + const props = { + ...baseProps, + sourceUrl: '/plugins/myplugin/refresh', + elements: [refreshSelectElement], + actions: { + submitInteractiveDialog: mockSubmitDialog, + lookupInteractiveDialog: jest.fn().mockResolvedValue({data: {items: []}}), + }, + }; + + const {getByTestId} = renderWithContext( + , + ); + + await waitFor(() => { + expect(getByTestId('apps-form-container')).toBeInTheDocument(); + }); + + // Get the refresh handler from the MockAppsFormContainer + const mockCall = MockAppsFormContainer.mock.calls[0][0]; + const refreshHandler = mockCall.actions.doAppFetchForm; + + // First refresh sets a value so it gets accumulated + await refreshHandler({ + selected_field: 'priority', + values: {priority: 'high'}, + }); + + mockSubmitDialog.mockClear(); + + // Second refresh clears the field (null simulates a cleared select) + await refreshHandler({ + selected_field: 'priority', + values: {priority: null}, + }); + + expect(mockSubmitDialog).toHaveBeenCalledWith( + expect.objectContaining({ + submission: expect.objectContaining({ + priority: '', + }), + }), + ); + }); }); }); diff --git a/webapp/channels/src/components/dialog_router/interactive_dialog_adapter.tsx b/webapp/channels/src/components/dialog_router/interactive_dialog_adapter.tsx index 78839f01f6e..813f2f81013 100644 --- a/webapp/channels/src/components/dialog_router/interactive_dialog_adapter.tsx +++ b/webapp/channels/src/components/dialog_router/interactive_dialog_adapter.tsx @@ -169,13 +169,12 @@ class InteractiveDialogAdapter extends React.PureComponent { * Common logic for processing form values - used by both submit and refresh */ private processFormValues = (currentValues: Record): void => { - // Normalize current values to extract primitive values from select objects - const normalizedCurrentValues = extractPrimitiveValues(currentValues); - - // Accumulate values: merge current with existing accumulated values + // Normalize current values to extract primitive values from select objects. + // clearEmptyFields=true ensures cleared fields emit '' or [] so they + // overwrite any previously accumulated value for that key. this.accumulatedValues = { - ...this.accumulatedValues, // Previous steps' values (including other pages) - ...normalizedCurrentValues, // Current form normalized values + ...this.accumulatedValues, + ...extractPrimitiveValues(currentValues, true), }; }; @@ -533,8 +532,15 @@ class InteractiveDialogAdapter extends React.PureComponent { const currentValues = call.values || {}; this.processFormValues(currentValues); - // For refresh, send all accumulated normalized values - const refreshPayload = this.accumulatedValues; + // For refresh, use a shallow copy so that adding selected_field + // does not permanently contaminate this.accumulatedValues + const refreshPayload = {...this.accumulatedValues}; + + // Include the changed field name in the submission so the plugin + // knows which field triggered the refresh + if (call.selected_field) { + refreshPayload.selected_field = call.selected_field; + } const refreshSubmission: DialogSubmission = { url: this.props.sourceUrl, diff --git a/webapp/channels/src/utils/dialog_conversion.test.ts b/webapp/channels/src/utils/dialog_conversion.test.ts index 8be2641fdbc..bd93e0e4bc0 100644 --- a/webapp/channels/src/utils/dialog_conversion.test.ts +++ b/webapp/channels/src/utils/dialog_conversion.test.ts @@ -8,6 +8,7 @@ import { convertDialogToAppForm, convertAppFormValuesToDialogSubmission, DialogElementTypes, + extractPrimitiveValues, getDefaultValue, getFieldType, getOptions, @@ -335,10 +336,24 @@ describe('dialog_conversion', () => { } as DialogElement; const result = getDefaultValue(element); - expect(result).toEqual({ - label: 'Option 1', - value: 'option1', - }); + + // Radio defaults are plain strings (not {label, value} objects) + // because RadioSetting.onChange returns e.target.value (a string) + expect(result).toBe('option1'); + }); + + it('should return null for radio default that does not match any option', () => { + const element = { + type: 'radio', + default: 'stale_value', + options: [ + {text: 'Option 1', value: 'option1'}, + {text: 'Option 2', value: 'option2'}, + ], + } as DialogElement; + + const result = getDefaultValue(element); + expect(result).toBeNull(); }); it('should handle dynamic select defaults', () => { @@ -973,6 +988,48 @@ describe('dialog_conversion', () => { expect(form.fields?.[0].refresh).toBe(true); }); + it('should handle refresh property for date and datetime fields', () => { + const elements: DialogElement[] = [ + { + name: 'refreshable_date', + type: 'date', + display_name: 'Refreshable Date', + optional: false, + refresh: true, + } as DialogElement, + { + name: 'refreshable_datetime', + type: 'datetime', + display_name: 'Refreshable Datetime', + optional: false, + refresh: true, + } as DialogElement, + { + name: 'non_refreshable_date', + type: 'date', + display_name: 'Non-Refreshable Date', + optional: false, + } as DialogElement, + ]; + + const {form, errors} = convertDialogToAppForm( + elements, + 'Test Dialog', + undefined, + undefined, + undefined, + 'http://example.com/source', + '', + legacyOptions, + ); + + expect(errors).toHaveLength(0); + expect(form.fields).toHaveLength(3); + expect(form.fields?.[0].refresh).toBe(true); + expect(form.fields?.[1].refresh).toBe(true); + expect(form.fields?.[2].refresh).toBeUndefined(); + }); + it('should include state in submit and source AppCall objects', () => { const elements: DialogElement[] = [ { @@ -1234,6 +1291,48 @@ describe('dialog_conversion', () => { }); }); + it('should extract value from radio field stored as AppSelectOption object', () => { + const values = { + radio_object: {label: 'Option A', value: 'optA'}, + radio_string: 'optB', + } as unknown as AppFormValues; + + const elements: DialogElement[] = [ + { + name: 'radio_object', + type: 'radio', + display_name: 'Radio Object Field', + optional: false, + options: [ + {text: 'Option A', value: 'optA'}, + {text: 'Option B', value: 'optB'}, + ], + } as DialogElement, + { + name: 'radio_string', + type: 'radio', + display_name: 'Radio String Field', + optional: false, + options: [ + {text: 'Option A', value: 'optA'}, + {text: 'Option B', value: 'optB'}, + ], + } as DialogElement, + ]; + + const {submission, errors} = convertAppFormValuesToDialogSubmission( + values, + elements, + legacyOptions, + ); + + expect(errors).toHaveLength(0); + expect(submission).toEqual({ + radio_object: 'optA', + radio_string: 'optB', + }); + }); + it('should handle textarea field values', () => { const values = { textarea_field: 'Long text content', @@ -1682,4 +1781,154 @@ describe('dialog_conversion', () => { }); }); }); + + describe('extractPrimitiveValues', () => { + it('should extract value from a single select option', () => { + const result = extractPrimitiveValues({ + color: {label: 'Red', value: 'red'}, + }); + expect(result).toEqual({color: 'red'}); + }); + + it('should extract values from a multiselect array', () => { + const result = extractPrimitiveValues({ + colors: [ + {label: 'Red', value: 'red'}, + {label: 'Blue', value: 'blue'}, + ], + }); + expect(result).toEqual({colors: ['red', 'blue']}); + }); + + it('should pass through primitive strings', () => { + const result = extractPrimitiveValues({ + name: 'hello', + }); + expect(result).toEqual({name: 'hello'}); + }); + + it('should pass through booleans', () => { + const result = extractPrimitiveValues({ + enabled: true, + disabled: false, + }); + expect(result).toEqual({enabled: true, disabled: false}); + }); + + it('should skip null, undefined, empty string, and values', () => { + const result = extractPrimitiveValues({ + a: null, + b: undefined, + c: '', + d: '', + e: 'keep', + }); + expect(result).toEqual({e: 'keep'}); + }); + + it('should skip select option with empty value', () => { + const result = extractPrimitiveValues({ + color: {label: '', value: ''}, + }); + expect(result).toEqual({}); + }); + + it('should skip select option with value', () => { + const result = extractPrimitiveValues({ + color: {label: 'None', value: ''}, + }); + expect(result).toEqual({}); + }); + + it('should skip empty multiselect arrays', () => { + const result = extractPrimitiveValues({ + colors: [], + }); + expect(result).toEqual({}); + }); + + it('should pass through primitive string arrays', () => { + const result = extractPrimitiveValues({ + dates: ['2026-01-01', '2026-01-15'], + }); + expect(result).toEqual({dates: ['2026-01-01', '2026-01-15']}); + }); + + it('should handle mixed arrays of select options and primitives', () => { + const result = extractPrimitiveValues({ + items: [ + {label: 'Red', value: 'red'}, + 'already-extracted', + ], + }); + expect(result).toEqual({items: ['red', 'already-extracted']}); + }); + + it('should filter out meaningless values from multiselect arrays', () => { + const result = extractPrimitiveValues({ + colors: [ + {label: 'Red', value: 'red'}, + {label: 'Empty', value: ''}, + {label: 'Blue', value: 'blue'}, + ], + }); + expect(result).toEqual({colors: ['red', 'blue']}); + }); + + describe('with clearEmptyFields=true', () => { + it('should emit empty string for null values', () => { + const result = extractPrimitiveValues({a: null}, true); + expect(result).toEqual({a: ''}); + }); + + it('should emit empty string for undefined values', () => { + const result = extractPrimitiveValues({a: undefined}, true); + expect(result).toEqual({a: ''}); + }); + + it('should emit empty string for empty string values', () => { + const result = extractPrimitiveValues({a: ''}, true); + expect(result).toEqual({a: ''}); + }); + + it('should emit empty string for values', () => { + const result = extractPrimitiveValues({a: ''}, true); + expect(result).toEqual({a: ''}); + }); + + it('should emit empty array for empty multiselect arrays', () => { + const result = extractPrimitiveValues({colors: []}, true); + expect(result).toEqual({colors: []}); + }); + + it('should emit empty string for select option with empty value', () => { + const result = extractPrimitiveValues({ + color: {label: '', value: ''}, + }, true); + expect(result).toEqual({color: ''}); + }); + + it('should emit empty string for select option with value', () => { + const result = extractPrimitiveValues({ + color: {label: 'None', value: ''}, + }, true); + expect(result).toEqual({color: ''}); + }); + + it('should still extract meaningful values normally', () => { + const result = extractPrimitiveValues({ + name: 'hello', + color: {label: 'Red', value: 'red'}, + cleared: null, + emptied: [], + }, true); + expect(result).toEqual({ + name: 'hello', + color: 'red', + cleared: '', + emptied: [], + }); + }); + }); + }); }); diff --git a/webapp/channels/src/utils/dialog_conversion.ts b/webapp/channels/src/utils/dialog_conversion.ts index 345aca40757..63e708970fb 100644 --- a/webapp/channels/src/utils/dialog_conversion.ts +++ b/webapp/channels/src/utils/dialog_conversion.ts @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import type {AppForm, AppField, AppFormValue, AppSelectOption, AppFormValues} from '@mattermost/types/apps'; +import {isAppSelectOption, type AppForm, type AppField, type AppFormValue, type AppSelectOption, type AppFormValues} from '@mattermost/types/apps'; import type {DialogElement} from '@mattermost/types/integrations'; import {AppFieldTypes} from 'mattermost-redux/constants/apps'; @@ -253,10 +253,19 @@ export function getDefaultValue(element: DialogElement): AppFormValue { return boolString === 'true' || boolString === '1' || boolString === 'yes'; } - case DialogElementTypes.SELECT: case DialogElementTypes.RADIO: { + // Radio values are always plain strings (RadioSetting.onChange returns e.target.value). + // Normalize to string from the start so the value shape never changes after user interaction. + if (element.options && element.default) { + const match = element.options.find((opt) => opt.value === element.default); + return match ? match.value : null; + } + return element.default ? String(element.default) : null; + } + + case DialogElementTypes.SELECT: { // Handle dynamic selects that use data_source instead of static options - if (element.type === 'select' && element.data_source === 'dynamic' && element.default) { + if (element.data_source === 'dynamic' && element.default) { if (element.multiselect) { const values = Array.isArray(element.default) ? element.default : @@ -274,7 +283,7 @@ export function getDefaultValue(element: DialogElement): AppFormValue { if (element.options && element.default) { // Handle multiselect defaults (comma-separated values) - if (element.type === 'select' && element.multiselect) { + if (element.multiselect) { const defaultValues = Array.isArray(element.default) ? element.default : String(element.default).split(',').map((val) => val.trim()); const defaultOptions = defaultValues.map((value) => { @@ -439,6 +448,13 @@ export function convertElement(element: DialogElement, options: ConversionOption } } + // Add refresh support for bool fields + if (element.type === DialogElementTypes.BOOL) { + if (element.refresh !== undefined) { + appField.refresh = element.refresh; + } + } + // Add date/datetime specific properties if (element.type === DialogElementTypes.DATE || element.type === DialogElementTypes.DATETIME) { // Use datetime_config if provided @@ -456,6 +472,10 @@ export function convertElement(element: DialogElement, options: ConversionOption if (element.time_interval !== undefined && element.type === DialogElementTypes.DATETIME) { appField.time_interval = Number(element.time_interval); } + + if (element.refresh !== undefined) { + appField.refresh = element.refresh; + } } return {field: appField, errors}; @@ -567,42 +587,34 @@ export function convertDialogToAppForm( /** * Extract primitive values from form field objects for storage/submission * Converts select option objects {label: "Text", value: "val"} to primitive "val" - * Filters out null, undefined, empty, and "" values + * Filters out null, undefined, empty, and "" values unless clearEmptyFields is true, + * in which case empty/null fields are emitted as '' or [] so they can overwrite prior values. */ -export function extractPrimitiveValues(values: Record): Record { - const normalized: Record = {}; - - Object.entries(values).forEach(([key, value]) => { - // Skip null, undefined, empty string, and "" values - if (value === null || value === undefined || value === '' || value === '') { - return; - } +export function extractPrimitiveValues(values: Record, clearEmptyFields = false): Record { + const isMeaningful = (v: any): v is string | boolean => v != null && v !== '' && v !== ''; + return Object.entries(values).reduce>((acc, [key, value]) => { if (Array.isArray(value)) { - // Handle multiselect arrays - extract values from each option object - const extractedValues = value. - filter((item) => item && typeof item === 'object' && 'value' in item). - map((item) => item.value). - filter((val) => val !== null && val !== undefined && val !== '' && val !== ''); + const extracted = value. + map((item) => (isAppSelectOption(item) ? item.value : item)). + filter(isMeaningful); - if (extractedValues.length > 0) { - normalized[key] = extractedValues; + if (extracted.length > 0 || clearEmptyFields) { + acc[key] = extracted; } - } else if (value && typeof value === 'object' && 'value' in value) { - // Extract value from single select option object {label: "...", value: "..."} - const extractedValue = value.value; - - // Only store if the extracted value is meaningful - if (extractedValue !== null && extractedValue !== undefined && extractedValue !== '' && extractedValue !== '') { - normalized[key] = extractedValue; + } else if (isAppSelectOption(value)) { + if (isMeaningful(value.value)) { + acc[key] = value.value; + } else if (clearEmptyFields) { + acc[key] = ''; } - } else { - // Keep primitive values as-is (but skip empty/nil values) - normalized[key] = value; + } else if (isMeaningful(value)) { + acc[key] = value; + } else if (clearEmptyFields) { + acc[key] = ''; } - }); - - return normalized; + return acc; + }, {}); } /** @@ -693,7 +705,13 @@ export function convertAppFormValuesToDialogSubmission( break; case DialogElementTypes.RADIO: - submission[element.name] = String(value); + // Radio values are normally plain strings, but accept {label, value} + // objects for backwards compatibility with older code paths. + if (isAppSelectOption(value)) { + submission[element.name] = value.value; + } else { + submission[element.name] = String(value); + } break; case DialogElementTypes.SELECT: diff --git a/webapp/platform/types/src/apps.ts b/webapp/platform/types/src/apps.ts index 2b61571f0fe..d7b5f069f18 100644 --- a/webapp/platform/types/src/apps.ts +++ b/webapp/platform/types/src/apps.ts @@ -417,7 +417,7 @@ export type AppSelectOption = { icon_data?: string; }; -function isAppSelectOption(v: unknown): v is AppSelectOption { +export function isAppSelectOption(v: unknown): v is AppSelectOption { if (typeof v !== 'object' || v === null) { return false; } From 2be57a7ec0c67004b77c76386f20a630920196e3 Mon Sep 17 00:00:00 2001 From: Carlos Garcia Date: Thu, 9 Apr 2026 16:12:15 +0200 Subject: [PATCH 08/10] adds team member data sanitizing (#35562) * adds team member data sanitizing * assert using require * adds data sanitizing to team members for user endpoint * team admin data visibility now tests with different user --- server/channels/api4/team.go | 38 +++++ server/channels/api4/team_test.go | 244 +++++++++++++++++++++++++++++ server/public/model/team_member.go | 11 ++ 3 files changed, 293 insertions(+) diff --git a/server/channels/api4/team.go b/server/channels/api4/team.go index 4d731c7e8b8..8676c26cce3 100644 --- a/server/channels/api4/team.go +++ b/server/channels/api4/team.go @@ -657,6 +657,10 @@ func getTeamMember(c *Context, w http.ResponseWriter, r *http.Request) { return } + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + team.SanitizeRoleData(c.AppContext.Session().UserId) + } + if err := json.NewEncoder(w).Encode(team); err != nil { c.Logger.Warn("Error while writing response", mlog.Err(err)) } @@ -695,6 +699,13 @@ func getTeamMembers(c *Context, w http.ResponseWriter, r *http.Request) { return } + currentUserId := c.AppContext.Session().UserId + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + for _, m := range members { + m.SanitizeRoleData(currentUserId) + } + } + js, err := json.Marshal(members) if err != nil { c.Err = model.NewAppError("getTeamMembers", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) @@ -734,6 +745,13 @@ func getTeamMembersForUser(c *Context, w http.ResponseWriter, r *http.Request) { return } + currentUserId := c.AppContext.Session().UserId + for _, m := range members { + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), m.TeamId, model.PermissionManageTeamRoles) { + m.SanitizeRoleData(currentUserId) + } + } + js, err := json.Marshal(members) if err != nil { c.Err = model.NewAppError("getTeamMembersForUser", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) @@ -777,6 +795,13 @@ func getTeamMembersByIds(c *Context, w http.ResponseWriter, r *http.Request) { return } + currentUserId := c.AppContext.Session().UserId + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + for _, m := range members { + m.SanitizeRoleData(currentUserId) + } + } + js, err := json.Marshal(members) if err != nil { c.Err = model.NewAppError("getTeamMembersByIds", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) @@ -883,6 +908,10 @@ func addTeamMember(c *Context, w http.ResponseWriter, r *http.Request) { auditRec.AddEventObjectType("team_member") // TODO verify this is the final state. should it be the team instead? auditRec.Success() + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + tm.SanitizeRoleData(c.AppContext.Session().UserId) + } + w.WriteHeader(http.StatusCreated) if err := json.NewEncoder(w).Encode(tm); err != nil { c.Logger.Warn("Error while writing response", mlog.Err(err)) @@ -1037,6 +1066,15 @@ func addTeamMembers(c *Context, w http.ResponseWriter, r *http.Request) { return } + currentUserId := c.AppContext.Session().UserId + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + for _, m := range membersWithErrors { + if m.Member != nil { + m.Member.SanitizeRoleData(currentUserId) + } + } + } + var ( js []byte err error diff --git a/server/channels/api4/team_test.go b/server/channels/api4/team_test.go index 4070366030c..06305a34363 100644 --- a/server/channels/api4/team_test.go +++ b/server/channels/api4/team_test.go @@ -4609,3 +4609,247 @@ func TestInvalidateAllEmailInvites(t *testing.T) { CheckOKStatus(t, res) }) } + +func setupTeamWithAdminAndMember(t *testing.T, th *TestHelper) *model.Client4 { + t.Helper() + th.UpdateUserToTeamAdmin(t, th.BasicUser2, th.BasicTeam) + require.Nil(t, th.App.Srv().InvalidateAllCaches()) + teamAdminClient := th.CreateClient() + _, _, err := teamAdminClient.Login(context.Background(), th.BasicUser2.Email, th.BasicUser2.Password) + require.NoError(t, err) + return teamAdminClient +} + +func assertRoleDataSanitized(t *testing.T, m *model.TeamMember) { + t.Helper() + assert.Empty(t, m.Roles) + assert.Empty(t, m.ExplicitRoles) + assert.False(t, m.SchemeAdmin) + assert.False(t, m.SchemeGuest) + assert.False(t, m.SchemeUser) + assert.Equal(t, int64(-1), m.DeleteAt) +} + +func TestGetTeamMembersRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("non-admin cannot see role data of others", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembers(context.Background(), th.BasicTeam.Id, 0, 100, "") + require.NoError(t, err) + + for _, m := range members { + if m.UserId != th.BasicUser.Id { + assertRoleDataSanitized(t, m) + } + } + }) + + t.Run("non-admin sees own role data", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembers(context.Background(), th.BasicTeam.Id, 0, 100, "") + require.NoError(t, err) + + for _, m := range members { + if m.UserId == th.BasicUser.Id { + assert.True(t, m.SchemeUser) + return + } + } + require.Fail(t, "current user not found in members") + }) + + t.Run("team admin sees full role data for other user", func(t *testing.T) { + members, _, err := teamAdminClient.GetTeamMembers(context.Background(), th.BasicTeam.Id, 0, 100, "") + require.NoError(t, err) + + for _, m := range members { + if m.UserId == th.BasicUser.Id { + assert.True(t, m.SchemeUser) + return + } + } + require.Fail(t, "target user not found in members") + }) + + t.Run("system admin sees full role data", func(t *testing.T) { + members, _, err := th.SystemAdminClient.GetTeamMembers(context.Background(), th.BasicTeam.Id, 0, 100, "") + require.NoError(t, err) + + for _, m := range members { + if m.UserId == th.BasicUser2.Id { + assert.True(t, m.SchemeAdmin) + return + } + } + require.Fail(t, "team admin not found in members") + }) +} + +func TestGetTeamMemberRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("non-admin cannot see role data of others", func(t *testing.T) { + member, _, err := th.Client.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + assertRoleDataSanitized(t, member) + }) + + t.Run("non-admin sees own role data", func(t *testing.T) { + member, _, err := th.Client.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser.Id, "") + require.NoError(t, err) + assert.True(t, member.SchemeUser) + }) + + t.Run("team admin sees full role data for other user", func(t *testing.T) { + member, _, err := teamAdminClient.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser.Id, "") + require.NoError(t, err) + assert.True(t, member.SchemeUser) + }) + + t.Run("system admin sees full role data", func(t *testing.T) { + member, _, err := th.SystemAdminClient.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + assert.True(t, member.SchemeAdmin) + }) +} + +func TestGetTeamMembersByIdsRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("non-admin cannot see role data of others", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembersByIds(context.Background(), th.BasicTeam.Id, []string{th.BasicUser2.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assertRoleDataSanitized(t, members[0]) + }) + + t.Run("non-admin sees own role data", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembersByIds(context.Background(), th.BasicTeam.Id, []string{th.BasicUser.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assert.True(t, members[0].SchemeUser) + }) + + t.Run("team admin sees full role data for other user", func(t *testing.T) { + members, _, err := teamAdminClient.GetTeamMembersByIds(context.Background(), th.BasicTeam.Id, []string{th.BasicUser.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assert.True(t, members[0].SchemeUser) + }) + + t.Run("system admin sees full role data", func(t *testing.T) { + members, _, err := th.SystemAdminClient.GetTeamMembersByIds(context.Background(), th.BasicTeam.Id, []string{th.BasicUser2.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assert.True(t, members[0].SchemeAdmin) + }) +} + +func TestAddTeamMemberRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("team admin adding user sees full role data in response", func(t *testing.T) { + newUser := th.CreateUser(t) + tm, _, err := teamAdminClient.AddTeamMember(context.Background(), th.BasicTeam.Id, newUser.Id) + require.NoError(t, err) + assert.True(t, tm.SchemeUser) + }) + + t.Run("non-admin adding user sees sanitized role data in response", func(t *testing.T) { + defaultRolePermissions := th.SaveDefaultRolePermissions(t) + defer th.RestoreDefaultRolePermissions(t, defaultRolePermissions) + th.AddPermissionToRole(t, model.PermissionAddUserToTeam.Id, model.TeamUserRoleId) + + newUser := th.CreateUser(t) + tm, _, err := th.Client.AddTeamMember(context.Background(), th.BasicTeam.Id, newUser.Id) + require.NoError(t, err) + assertRoleDataSanitized(t, tm) + }) +} + +func TestAddTeamMembersRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("team admin adding users sees full role data in response", func(t *testing.T) { + newUser := th.CreateUser(t) + members, _, err := teamAdminClient.AddTeamMembers(context.Background(), th.BasicTeam.Id, []string{newUser.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assert.True(t, members[0].SchemeUser) + }) + + t.Run("non-admin adding users sees sanitized role data in response", func(t *testing.T) { + defaultRolePermissions := th.SaveDefaultRolePermissions(t) + defer th.RestoreDefaultRolePermissions(t, defaultRolePermissions) + th.AddPermissionToRole(t, model.PermissionAddUserToTeam.Id, model.TeamUserRoleId) + + newUser := th.CreateUser(t) + members, _, err := th.Client.AddTeamMembers(context.Background(), th.BasicTeam.Id, []string{newUser.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assertRoleDataSanitized(t, members[0]) + }) +} + +func TestGetTeamMembersForUserRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("user sees own role data", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembersForUser(context.Background(), th.BasicUser.Id, "") + require.NoError(t, err) + require.NotEmpty(t, members) + for _, m := range members { + assert.True(t, m.SchemeUser) + } + }) + + t.Run("non-admin cannot see role data of another user", func(t *testing.T) { + defaultRolePermissions := th.SaveDefaultRolePermissions(t) + defer th.RestoreDefaultRolePermissions(t, defaultRolePermissions) + th.AddPermissionToRole(t, model.PermissionReadOtherUsersTeams.Id, model.SystemUserRoleId) + + members, _, err := th.Client.GetTeamMembersForUser(context.Background(), th.BasicUser2.Id, "") + require.NoError(t, err) + require.NotEmpty(t, members) + for _, m := range members { + assertRoleDataSanitized(t, m) + } + }) + + t.Run("team admin sees full role data for other user in managed team", func(t *testing.T) { + members, _, err := teamAdminClient.GetTeamMembersForUser(context.Background(), th.BasicUser.Id, "") + require.NoError(t, err) + require.NotEmpty(t, members) + for _, m := range members { + if m.TeamId == th.BasicTeam.Id { + assert.True(t, m.SchemeUser) + return + } + } + require.Fail(t, "basic team membership not found") + }) + + t.Run("system admin sees full role data", func(t *testing.T) { + members, _, err := th.SystemAdminClient.GetTeamMembersForUser(context.Background(), th.BasicUser2.Id, "") + require.NoError(t, err) + require.NotEmpty(t, members) + for _, m := range members { + if m.TeamId == th.BasicTeam.Id { + assert.True(t, m.SchemeAdmin) + return + } + } + require.Fail(t, "basic team membership not found") + }) +} diff --git a/server/public/model/team_member.go b/server/public/model/team_member.go index 53cf25f072b..59c73936794 100644 --- a/server/public/model/team_member.go +++ b/server/public/model/team_member.go @@ -142,3 +142,14 @@ func (o *TeamMember) PreUpdate() { func (o *TeamMember) GetRoles() []string { return strings.Fields(o.Roles) } + +func (o *TeamMember) SanitizeRoleData(currentUserId string) { + if o.UserId != currentUserId { + o.Roles = "" + o.ExplicitRoles = "" + o.SchemeAdmin = false + o.SchemeGuest = false + o.SchemeUser = false + o.DeleteAt = -1 + } +} From 860df69621bf8940ccf2aa4b3350a38034435c27 Mon Sep 17 00:00:00 2001 From: Pavel Zeman Date: Thu, 9 Apr 2026 15:27:50 -0400 Subject: [PATCH 09/10] ci: re-enable server test coverage with 4-shard parallelism (#35743) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ci: re-enable server test coverage with 4-shard parallelism The test-coverage job was disabled due to OOM failures when running all tests with coverage instrumentation in a single process. Re-enable it by distributing the workload across 4 parallel runners using the shard infrastructure from the sharding PRs. Changes: - Replace disabled single-runner test-coverage with 4-shard matrix - Add merge-coverage job to combine per-shard cover.out files - Upload merged coverage to Codecov with server flag - Skip per-shard Codecov upload when sharding is active - Add coverage profile merging to run-shard-tests.sh for multi-run shards - Restore original condition: skip coverage on release branch PRs - Keep fullyparallel=true (fast within each shard) - Keep continue-on-error=true (coverage never blocks PRs) Co-authored-by: Claude * fix: disable fullyparallel for coverage shards t.Parallel() + t.Setenv() panics kill entire test binaries under fullyparallel mode. With 4-shard splitting, serial execution within each shard should still be fast enough (~15 min). We can re-enable fullyparallel once the incompatible tests are fixed. Co-authored-by: Claude * fix: add checkout to coverage merge job for Codecov file mapping Codecov needs the source tree to map coverage data to files. Without checkout, the upload succeeds but reports 0% coverage because it can't associate cover.out lines with source files. Co-authored-by: Claude * ci: add codecov.yml and retain merged coverage artifact Add codecov.yml with: - Project coverage: track against parent commit, 1% threshold, advisory - Patch coverage: 50% target for new code, advisory (warns, doesn't block) - Ignore generated code (retrylayer, timerlayer, serial_gen, mocks, storetest, plugintest, searchtest) — these inflate the denominator from 146K to 100K statements, rebasing coverage from 36% to 53% - PR comments on coverage changes with condensed layout Save merged cover.out as artifact with 30-day retention (~3.5MB/run). 90-day retention was considered (~6.3GB total vs ~2.1GB at 30 days) but deferred to keep storage costs low. #### Release Note ```release-note NONE ``` Co-authored-by: Claude * ci: add codecov.yml to exclude generated code and enable PR comments (#35748) * ci: add codecov.yml to exclude generated code and enable PR comments Add Codecov configuration to improve coverage signal quality: - Exclude generated code from coverage denominator: - store/retrylayer (~10k stmts, auto-generated retry wrappers) - store/timerlayer (~14k lines, auto-generated timing wrappers) - *_serial_gen.go (serialization codegen) - **/mocks (mockery-generated mocks) - Exclude test infrastructure: - store/storetest (~63k lines, test helpers not production code) - plugin/plugintest (plugin test helpers) - Exclude thin wrappers: - model/client4.go (~4k stmts, HTTP client methods tested via integration) - Enable PR comments with condensed layout - Set project threshold at 0.5% drop tolerance - Set patch target at 60% for new/changed lines This rebases the effective coverage metric from ~33.8% to ~43% by removing ~50k non-production statements from the denominator, giving a more accurate picture of actual test coverage. Co-authored-by: Claude * Update codecov.yml --------- Co-authored-by: Claude Co-authored-by: Jesse Hallam * fix: bump upload-artifact to v7 and add client4.go to codecov ignore - Align upload-artifact pin with the rest of the workflow (v4 → v7) - Add model/client4.go to codecov.yml ignore list as documented in PR description Co-authored-by: Claude * fix(ci): address Jesse review feedback on coverage sharding - Remove client4.go from codecov ignore list (coverage is meaningful) - Remove historical comment block above test-coverage job - Set fullyparallel back to true (safe per-shard since each runs different packages; parallel test fixes tracked in #35751) - Replace merge-coverage job with per-shard Codecov uploads using flags parameter; configure after_n_builds: 4 so Codecov waits for all shards before reporting status - Add clarifying comment in run-shard-tests.sh explaining intra-shard coverage merge (multiple gotestsum runs) vs cross-shard merge (handled natively by Codecov) - Simplify codecov.yml: remove verbose comments, use informational status checks, streamlined ignore list Co-authored-by: Claude * fix(ci): set fullyparallel back to false for coverage shards Coverage shards 1-3 failed with hundreds of test failures because fullyparallel: true causes panics and races in tests that use t.Setenv, os.Setenv, and os.Chdir without parallel-safe alternatives. The parallel-safety fixes are tracked in a separate PR chain: - #35746: t.Setenv → test hooks - #35749: os.Setenv → parallel-safe alternatives - #35750: os.Chdir → t.Chdir - #35751: flip fullyparallel: true (final step) Once that chain merges, fullyparallel can be enabled for coverage too. Co-authored-by: Claude * fix(ci): split fullyparallel and allow-failure into separate inputs Previously fullyparallel controlled both parallel test execution AND continue-on-error, meaning disabling parallelism also made coverage failures blocking. Split into two independent inputs: - fullyparallel: controls ENABLE_FULLY_PARALLEL_TESTS (test execution) - allow-failure: controls continue-on-error (advisory vs blocking) Coverage shards now run with fullyparallel: true (Claudio's original approach) and allow-failure: true (failures don't block PRs until parallel-safety fixes land in #35746 → #35751). Co-authored-by: Claude * ci: use per-flag after_n_builds for server and webapp coverage Replace the global after_n_builds: 2 with per-flag values: - server: after_n_builds: 4 (one per shard) - webapp: after_n_builds: 1 (single merged upload) Tag the webapp Codecov upload with flags: webapp so each flag independently waits for its expected upload count. This prevents Codecov from firing notifications with incomplete data when the webapp upload arrives before all server shards complete. Addresses review feedback from @esarafianou. Co-authored-by: Claude * fix: consolidate codecov config into .github/codecov.yml Move all codecov configuration into the existing .github/codecov.yml instead of introducing a duplicate file at the repo root. Merges improvements from the root file (broader ignore list, informational statuses, require_ci_to_pass: false) while preserving the webapp flag from the original config. Updates after_n_builds to 5 (4 server + 1 webapp). Co-authored-by: Claude --------- Co-authored-by: Claude Co-authored-by: Jesse Hallam --- .github/codecov.yml | 51 ++++++++++++++++------ .github/workflows/server-ci.yml | 17 +++++--- .github/workflows/server-test-template.yml | 5 +++ .github/workflows/webapp-ci.yml | 1 + server/scripts/run-shard-tests.sh | 14 ++++++ 5 files changed, 69 insertions(+), 19 deletions(-) diff --git a/.github/codecov.yml b/.github/codecov.yml index cbc4185fa81..36d71df8cf2 100644 --- a/.github/codecov.yml +++ b/.github/codecov.yml @@ -1,17 +1,42 @@ -comment: - layout: "condensed_header, condensed_files, condensed_footer" - behavior: default - require_changes: "uncovered_patch" # only post comment if the patch has uncovered lines - hide_project_coverage: true # only show coverage on the git diff +codecov: + require_ci_to_pass: false + # Wait for all coverage uploads (4 server shards + 1 webapp) before + # computing status. Without this, Codecov may report partial coverage + # from the first shard to finish, showing a misleading drop on the PR. + notify: + after_n_builds: 5 + coverage: status: - changes: false - patch: false project: default: - threshold: 1.0 -codecov: - notify: - after_n_builds: 2 # Server and webapp at this point -ignore: - - ^store/storetest.* + target: auto + threshold: 1% + informational: true + patch: + default: + target: 50% + informational: true + + # Exclude generated code, mocks, and test infrastructure from reporting. + # Go compiles these into the test binary, so they appear in cover.out, + # but they aren't production code and inflate the denominator. + ignore: + - "server/**/retrylayer/**" + - "server/**/timerlayer/**" + - "server/**/*_serial_gen.go" + - "server/**/mocks/**" + - "server/**/storetest/**" + - "server/**/plugintest/**" + - "server/**/searchtest/**" + +flags: + server: + after_n_builds: 4 # 4 server test shards + webapp: + after_n_builds: 1 # 1 merged webapp upload + +comment: + layout: "condensed_header,diff,flags" + behavior: default + require_changes: true diff --git a/.github/workflows/server-ci.yml b/.github/workflows/server-ci.yml index 2c80b1761fb..601e5505e8b 100644 --- a/.github/workflows/server-ci.yml +++ b/.github/workflows/server-ci.yml @@ -271,22 +271,27 @@ jobs: artifact-name: postgres-server-fips-test-logs test-coverage: - name: Generate Test Coverage - # Disabled: Running out of memory and causing spurious failures. - # Old condition: ${{ github.event_name != 'pull_request' || !startsWith(github.event.pull_request.base.ref, 'release-') }} - if: false + name: "Coverage (shard ${{ matrix.shard }})" + if: ${{ github.event_name != 'pull_request' || !startsWith(github.event.pull_request.base.ref, 'release-') }} needs: go + strategy: + fail-fast: false + matrix: + shard: [0, 1, 2, 3] uses: ./.github/workflows/server-test-template.yml secrets: inherit with: - name: Generate Test Coverage + name: "Coverage (shard ${{ matrix.shard }})" datasource: postgres://mmuser:mostest@postgres:5432/mattermost_test?sslmode=disable&connect_timeout=10 drivername: postgres - logsartifact: coverage-server-test-logs + logsartifact: "coverage-server-test-logs-shard-${{ matrix.shard }}" fullyparallel: true allow-failure: true enablecoverage: true go-version: ${{ needs.go.outputs.version }} + fips-enabled: false + shard-index: ${{ matrix.shard }} + shard-total: 4 test-mmctl: name: Run mmctl tests needs: go diff --git a/.github/workflows/server-test-template.yml b/.github/workflows/server-test-template.yml index 2e67e79aa2a..a5ee54e7c63 100644 --- a/.github/workflows/server-test-template.yml +++ b/.github/workflows/server-test-template.yml @@ -22,6 +22,10 @@ on: required: false type: boolean default: false + allow-failure: + required: false + type: boolean + default: false enablecoverage: required: false type: boolean @@ -222,6 +226,7 @@ jobs: token: ${{ secrets.CODECOV_TOKEN }} disable_search: true files: server/cover.out + flags: server - name: Stop docker compose run: | diff --git a/.github/workflows/webapp-ci.yml b/.github/workflows/webapp-ci.yml index 65fa9ab9f8d..869413d7e58 100644 --- a/.github/workflows/webapp-ci.yml +++ b/.github/workflows/webapp-ci.yml @@ -223,6 +223,7 @@ jobs: token: ${{ secrets.CODECOV_TOKEN }} disable_search: true files: ./webapp/channels/coverage/merged/lcov.info + flags: webapp build: needs: check-lint diff --git a/server/scripts/run-shard-tests.sh b/server/scripts/run-shard-tests.sh index 57c55ab059d..688e3f2019e 100755 --- a/server/scripts/run-shard-tests.sh +++ b/server/scripts/run-shard-tests.sh @@ -108,6 +108,20 @@ fi cat gotestsum-*.json > gotestsum.json 2>/dev/null || true +# ── Merge coverage profiles within this shard (if coverage is enabled) ── +# A single shard may run multiple gotestsum invocations (light packages + +# heavy package splits), each producing its own cover-N.out. This merges +# them into one cover.out per shard. The cross-shard merge (combining all +# shards into a single report) is handled by Codecov's after_n_builds. +if [[ "${ENABLE_COVERAGE:-false}" == "true" ]] && ls cover-*.out 1>/dev/null 2>&1; then + echo "Merging coverage profiles..." + { + head -1 cover-0.out # "mode: atomic" header + tail -q -n +2 cover-*.out # data lines from all files + } > cover.out + echo "Merged $(ls cover-*.out | wc -l) coverage files into cover.out" +fi + if [[ $FAILURES -gt 0 ]]; then echo "Shard complete: $RUN_IDX gotestsum runs, $FAILURES failed" exit 1 From 78b2980ed5f1bd5c0ef05e0f81cc1b6c5179d44e Mon Sep 17 00:00:00 2001 From: Pavel Zeman Date: Thu, 9 Apr 2026 15:47:20 -0400 Subject: [PATCH 10/10] fix: remove duplicate allow-failure input in server test template (#36004) The allow-failure input was defined twice in the workflow_call inputs, causing GitHub Actions to reject the workflow with 0 jobs on master push. Duplicate was introduced in #35743 merge. Release Note NONE Co-authored-by: Claude --- .github/workflows/server-test-template.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.github/workflows/server-test-template.yml b/.github/workflows/server-test-template.yml index a5ee54e7c63..3d0f9abd7f4 100644 --- a/.github/workflows/server-test-template.yml +++ b/.github/workflows/server-test-template.yml @@ -22,10 +22,6 @@ on: required: false type: boolean default: false - allow-failure: - required: false - type: boolean - default: false enablecoverage: required: false type: boolean