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 <build@mattermost.com>

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)
 }

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)

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)

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)

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)

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)
 }

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)

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)
 

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

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) {

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

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.")