From fc9d3be3689d7218f9bd5dd8699e74b33c440911 Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Wed, 8 Apr 2026 21:47:57 -0700 Subject: [PATCH] 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.")