From f83d32e42c45d80d4ec2a6747652ddd5a3c01951 Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Fri, 10 Apr 2026 09:40:08 -0700 Subject: [PATCH] Strip remote_id field from user patch API requests (#36008) * Reapply "Strip remote_id field from user patch API requests (#35910)" (#35996) This reverts commit d1ca297721b75700b212370e339ed37ae1b1d6b1. * Fix SetUserRemoteID to use test's own database in parallel mode Replace testlib.SetUserRemoteID (which used mainHelper's shared database) with a squirrel query against GetInternalMasterDB(), which resolves to the correct per-test pooled database under parallel execution. --- server/channels/api4/apitestlib.go | 20 ++++ 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 | 20 ++++ ..._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 + 11 files changed, 155 insertions(+), 32 deletions(-) diff --git a/server/channels/api4/apitestlib.go b/server/channels/api4/apitestlib.go index facb392d651..31a565d5717 100644 --- a/server/channels/api4/apitestlib.go +++ b/server/channels/api4/apitestlib.go @@ -21,6 +21,7 @@ import ( "time" "github.com/gorilla/websocket" + sq "github.com/mattermost/squirrel" s3 "github.com/minio/minio-go/v7" "github.com/minio/minio-go/v7/pkg/credentials" "github.com/stretchr/testify/require" @@ -1409,6 +1410,25 @@ 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() + + query, args, err := sq.StatementBuilder.PlaceholderFormat(sq.Dollar). + Update("Users"). + Set("RemoteId", remoteID). + Where(sq.Eq{"Id": userID}). + ToSql() + require.NoError(tb, err) + + _, err = th.App.Srv().Store().GetInternalMasterDB().Exec(query, args...) + 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 cfb07e76efd..1b06dbe19d8 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..84edf5c4b42 100644 --- a/server/channels/app/helper_test.go +++ b/server/channels/app/helper_test.go @@ -15,6 +15,7 @@ import ( "testing" "time" + sq "github.com/mattermost/squirrel" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -814,6 +815,25 @@ 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() + + query, args, err := sq.StatementBuilder.PlaceholderFormat(sq.Dollar). + Update("Users"). + Set("RemoteId", remoteID). + Where(sq.Eq{"Id": userID}). + ToSql() + require.NoError(tb, err) + + _, err = th.App.Srv().Store().GetInternalMasterDB().Exec(query, args...) + 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 cea598ba723..82f535db130 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