MM-62745: [Shared Channels] Fix duplicate mentioning - local user with the same username as someone on the remote server - Part2 (#32101) (#33414)

Automatic Merge

server/channels/app/platform/shared_channel_service_iface.go

@@ -5,6 +5,7 @@ package platform
 
 import (
 	"github.com/mattermost/mattermost/server/public/model"
+	"github.com/mattermost/mattermost/server/public/shared/request"
 	"github.com/mattermost/mattermost/server/v8/platform/services/sharedchannel"
 )
 
@@ -24,6 +25,7 @@ type SharedChannelServiceIFace interface {
 	CheckChannelIsShared(channelID string) error
 	CheckCanInviteToSharedChannel(channelId string) error
 	HandleMembershipChange(channelID, userID string, isAdd bool, remoteID string)
+	TransformMentionsOnReceiveForTesting(ctx request.CTX, post *model.Post, targetChannel *model.Channel, rc *model.RemoteCluster, mentionTransforms map[string]string)
 }
 
 type MockOptionSharedChannelService func(service *mockSharedChannelService)
@@ -82,3 +84,7 @@ func (mrcs *mockSharedChannelService) NumInvitations() int {
 func (mrcs *mockSharedChannelService) HandleMembershipChange(channelID, userID string, isAdd bool, remoteID string) {
 	// This is a mock implementation - it doesn't need to do anything
 }
+
+func (mrcs *mockSharedChannelService) TransformMentionsOnReceiveForTesting(ctx request.CTX, post *model.Post, targetChannel *model.Channel, rc *model.RemoteCluster, mentionTransforms map[string]string) {
+	// This is a mock implementation - it doesn't need to do anything
+}

server/channels/app/shared_channel_service_iface.go

@@ -6,6 +6,7 @@ package app
 // TODO: platform: remove this and use from platform package
 import (
 	"github.com/mattermost/mattermost/server/public/model"
+	"github.com/mattermost/mattermost/server/public/shared/request"
 	"github.com/mattermost/mattermost/server/v8/platform/services/sharedchannel"
 )
 
@@ -27,6 +28,7 @@ type SharedChannelServiceIFace interface {
 	CheckChannelIsShared(channelID string) error
 	CheckCanInviteToSharedChannel(channelId string) error
 	HandleMembershipChange(channelID, userID string, isAdd bool, remoteID string)
+	TransformMentionsOnReceiveForTesting(ctx request.CTX, post *model.Post, targetChannel *model.Channel, rc *model.RemoteCluster, mentionTransforms map[string]string)
 }
 
 func NewMockSharedChannelService(service SharedChannelServiceIFace) *mockSharedChannelService {

server/channels/app/shared_channel_test.go

@@ -22,6 +22,7 @@ func setupSharedChannels(tb testing.TB) *TestHelper {
 		*cfg.ConnectedWorkspacesSettings.EnableRemoteClusterService = true
 		*cfg.ConnectedWorkspacesSettings.EnableSharedChannels = true
 		cfg.FeatureFlags.EnableSharedChannelsMemberSync = true
+		cfg.ClusterSettings.ClusterName = model.NewPointer("test-remote")
 	})
 }
 
@@ -556,3 +557,299 @@ func TestSyncMessageErrChannelNotSharedResponse(t *testing.T) {
 	}
 	require.NotNil(t, systemPost, "System message should be posted when channel becomes unshared")
 }
+
+// TestTransformMentionsOnReceive provides comprehensive unit testing for the mention transformation logic
+// using explicit mentionTransforms. This tests ONLY the receiver-side transformation logic
+// without requiring complex end-to-end cross-cluster setup.
+func TestTransformMentionsOnReceive(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := setupSharedChannels(t).InitBasic()
+
+	// Setup shared channel
+	sharedChannel := th.CreateChannel(th.Context, th.BasicTeam)
+	sc := &model.SharedChannel{
+		ChannelId: sharedChannel.Id,
+		TeamId:    th.BasicTeam.Id,
+		Home:      true,
+		ShareName: "testchannel",
+		CreatorId: th.BasicUser.Id,
+	}
+	_, err := th.App.ShareChannel(th.Context, sc)
+	require.NoError(t, err)
+
+	// Setup remote cluster representing the sender
+	remoteCluster := &model.RemoteCluster{
+		RemoteId:    model.NewId(),
+		Name:        "remote1",
+		DisplayName: "Remote 1",
+		SiteURL:     "http://remote1.example.com",
+		Token:       model.NewId(),
+		CreatorId:   th.BasicUser.Id,
+		CreateAt:    model.GetMillis(),
+		LastPingAt:  model.GetMillis(),
+	}
+	savedRemoteCluster, appErr := th.App.AddRemoteCluster(remoteCluster)
+	require.Nil(t, appErr)
+
+	// Get shared channel service
+	scs := th.App.Srv().Platform().GetSharedChannelService()
+	require.NotNil(t, scs)
+	concreteScs, ok := scs.(*sharedchannel.Service)
+	require.True(t, ok)
+
+	// Helper to create test users
+	createUser := func(username string, remoteId *string) *model.User {
+		user := th.CreateUser()
+		user.Username = username
+		if remoteId != nil {
+			user.RemoteId = remoteId
+		}
+		user, updateErr := th.App.UpdateUser(th.Context, user, false)
+		require.Nil(t, updateErr)
+		th.LinkUserToTeam(user, th.BasicTeam)
+		th.AddUserToChannel(user, sharedChannel)
+		return user
+	}
+
+	// Helper to test transformation
+	testTransformation := func(originalMessage string, mentionTransforms map[string]string, expectedMessage string, description string) {
+		post := &model.Post{
+			Id:        model.NewId(),
+			ChannelId: sharedChannel.Id,
+			UserId:    th.BasicUser.Id,
+			Message:   originalMessage,
+		}
+
+		t.Logf("Testing: %s", description)
+		t.Logf("  Original: %s", originalMessage)
+		t.Logf("  Transforms: %v", mentionTransforms)
+
+		// Call the transformation function directly
+		concreteScs.TransformMentionsOnReceiveForTesting(th.Context, post, sharedChannel, savedRemoteCluster, mentionTransforms)
+
+		t.Logf("  Result: %s", post.Message)
+		t.Logf("  Expected: %s", expectedMessage)
+
+		require.Equal(t, expectedMessage, post.Message, description)
+	}
+
+	t.Run("Scenario 1.1: Remote mentions local user (simple mention)", func(t *testing.T) {
+		// Create remote user that was synced to receiver
+		remoteUser := createUser("admin:remote1", &savedRemoteCluster.RemoteId)
+
+		// Scenario: remote1 mentions "@admin" (their local user) → sent to receiver
+		// mentionTransforms["admin"] = remote1AdminUserId
+		mentionTransforms := map[string]string{
+			"admin": remoteUser.Id,
+		}
+
+		testTransformation(
+			"Hello @admin, can you help?",
+			mentionTransforms,
+			"Hello @admin:remote1, can you help?", // Use synced username
+			"Simple mention of synced remote user should use synced username",
+		)
+	})
+
+	t.Run("Scenario 1.2: Remote mentions local user (different username)", func(t *testing.T) {
+		// Create remote user that was synced to receiver
+		remoteUser := createUser("user:remote1", &savedRemoteCluster.RemoteId)
+
+		mentionTransforms := map[string]string{
+			"user": remoteUser.Id,
+		}
+
+		testTransformation(
+			"Hello @user, can you help?",
+			mentionTransforms,
+			"Hello @user:remote1, can you help?",
+			"Simple mention of different synced remote user should use synced username",
+		)
+	})
+
+	t.Run("Scenario 2.1: Remote mentions with colon (local user)", func(t *testing.T) {
+		// Create local user on receiver
+		localUser := createUser("admin", nil)
+
+		// Scenario: remote2 mentions "@admin:remote1" → sent to remote1
+		// mentionTransforms["admin:remote1"] = remote1AdminUserId
+		mentionTransforms := map[string]string{
+			"admin:remote1": localUser.Id,
+		}
+
+		testTransformation(
+			"Hello @admin:remote1, can you help?",
+			mentionTransforms,
+			"Hello @admin, can you help?", // Strip suffix for local user
+			"Colon mention of local user should strip cluster suffix",
+		)
+	})
+
+	t.Run("Scenario 2.2: Remote mentions with colon (different local user)", func(t *testing.T) {
+		// Create local user on receiver
+		localUser := createUser("user", nil)
+
+		mentionTransforms := map[string]string{
+			"user:remote1": localUser.Id,
+		}
+
+		testTransformation(
+			"Hello @user:remote1, can you help?",
+			mentionTransforms,
+			"Hello @user, can you help?",
+			"Colon mention of different local user should strip cluster suffix",
+		)
+	})
+
+	t.Run("Scenario A1: Name clash - remote user mention, local user exists", func(t *testing.T) {
+		// Create local user with same name
+		_ = createUser("alice", nil) // Create name clash scenario
+		// Create remote user that was synced
+		remoteUser := createUser("alice:remote1", &savedRemoteCluster.RemoteId)
+
+		// When remote1 mentions "@alice" (their local user), receiver gets explicit transform
+		mentionTransforms := map[string]string{
+			"alice": remoteUser.Id, // Points to synced remote user, not local user
+		}
+
+		testTransformation(
+			"Hello @alice, can you help?",
+			mentionTransforms,
+			"Hello @alice:remote1, can you help?",
+			"Matrix A1: Remote user mention with local name clash should add cluster suffix",
+		)
+	})
+
+	t.Run("Scenario A2: Same user - previously synced", func(t *testing.T) {
+		// Create user that was synced from sender
+		syncedUser := createUser("bob:remote1", &savedRemoteCluster.RemoteId)
+
+		mentionTransforms := map[string]string{
+			"bob": syncedUser.Id,
+		}
+
+		testTransformation(
+			"Hello @bob, can you help?",
+			mentionTransforms,
+			"Hello @bob:remote1, can you help?",
+			"Matrix A2: Previously synced user should display synced username",
+		)
+	})
+
+	t.Run("Scenario A3: No user exists on receiver", func(t *testing.T) {
+		// Use non-existent user ID
+		nonExistentUserId := model.NewId()
+
+		mentionTransforms := map[string]string{
+			"charlie": nonExistentUserId,
+		}
+
+		testTransformation(
+			"Hello @charlie, can you help?",
+			mentionTransforms,
+			"Hello @charlie:remote1, can you help?",
+			"Matrix A3: Unknown user should get cluster suffix",
+		)
+	})
+
+	t.Run("Scenario B1: User exists on origin with same ID", func(t *testing.T) {
+		// Create local user (representing user on their home cluster)
+		localUser := createUser("dave", nil)
+
+		// Remote mentions "@dave:remote1" pointing to local user ID
+		mentionTransforms := map[string]string{
+			"dave:remote1": localUser.Id,
+		}
+
+		testTransformation(
+			"Hello @dave:remote1, can you help?",
+			mentionTransforms,
+			"Hello @dave, can you help?",
+			"Matrix B1: Remote mention of local user should strip cluster suffix",
+		)
+	})
+
+	t.Run("Scenario B2: User does not exist on origin", func(t *testing.T) {
+		// Use non-existent user ID
+		nonExistentUserId := model.NewId()
+
+		mentionTransforms := map[string]string{
+			"eve:remote1": nonExistentUserId,
+		}
+
+		testTransformation(
+			"Hello @eve:remote1, can you help?",
+			mentionTransforms,
+			"Hello @eve:remote1, can you help?",
+			"Matrix B2: Unknown colon mention should remain unchanged",
+		)
+	})
+
+	t.Run("Empty mentionTransforms", func(t *testing.T) {
+		// No transforms provided
+		mentionTransforms := map[string]string{}
+
+		testTransformation(
+			"Hello @anyone, can you help?",
+			mentionTransforms,
+			"Hello @anyone, can you help?",
+			"Message without transforms should remain unchanged",
+		)
+	})
+
+	t.Run("Mixed scenarios in single message", func(t *testing.T) {
+		// Setup users
+		localUser := createUser("frank", nil)
+		remoteUser := createUser("george:remote1", &savedRemoteCluster.RemoteId)
+
+		// Multiple transforms in one message
+		mentionTransforms := map[string]string{
+			"frank:remote1": localUser.Id,  // Colon mention → strip suffix
+			"george":        remoteUser.Id, // Simple mention → use synced username
+		}
+
+		testTransformation(
+			"Hello @frank:remote1 and @george, let's collaborate!",
+			mentionTransforms,
+			"Hello @frank and @george:remote1, let's collaborate!",
+			"Mixed mention types should transform correctly",
+		)
+	})
+
+	t.Run("Colon mention of remote user", func(t *testing.T) {
+		// Create remote user that was synced
+		remoteUser := createUser("guest:remote1", &savedRemoteCluster.RemoteId)
+
+		// Colon mention pointing to remote user (edge case)
+		mentionTransforms := map[string]string{
+			"guest:remote1": remoteUser.Id,
+		}
+
+		testTransformation(
+			"Hello @guest:remote1, welcome!",
+			mentionTransforms,
+			"Hello @guest:remote1, welcome!",
+			"Colon mention of remote user should use synced username",
+		)
+	})
+
+	t.Run("Performance: Large message with many mentions", func(t *testing.T) {
+		// Create users for performance test
+		user1 := createUser("user1:remote1", &savedRemoteCluster.RemoteId)
+		user2 := createUser("user2:remote1", &savedRemoteCluster.RemoteId)
+		user3 := createUser("user3:remote1", &savedRemoteCluster.RemoteId)
+
+		mentionTransforms := map[string]string{
+			"user1": user1.Id,
+			"user2": user2.Id,
+			"user3": user3.Id,
+		}
+
+		testTransformation(
+			"Meeting with @user1, @user2, and @user3 about @user1's proposal. @user2 will present, @user3 will take notes.",
+			mentionTransforms,
+			"Meeting with @user1:remote1, @user2:remote1, and @user3:remote1 about @user1:remote1's proposal. @user2:remote1 will present, @user3:remote1 will take notes.",
+			"Multiple mentions should transform efficiently",
+		)
+	})
+}

server/platform/services/sharedchannel/service.go

@@ -380,3 +380,8 @@ func (scs *Service) OnReceiveSyncMessageForTesting(msg model.RemoteClusterMsg, r
 func (scs *Service) HandleChannelNotSharedErrorForTesting(msg *model.SyncMsg, rc *model.RemoteCluster) {
 	scs.handleChannelNotSharedError(msg, rc)
 }
+
+// TransformMentionsOnReceiveForTesting allows testing the full mention transformation flow
+func (scs *Service) TransformMentionsOnReceiveForTesting(ctx request.CTX, post *model.Post, targetChannel *model.Channel, rc *model.RemoteCluster, mentionTransforms map[string]string) {
+	scs.transformMentionsOnReceive(ctx, post, targetChannel, rc, mentionTransforms)
+}

server/platform/services/sharedchannel/sync_recv.go

@@ -197,7 +197,7 @@ func (scs *Service) processSyncMessage(c request.CTX, syncMsg *model.SyncMsg, rc
 		}
 
 		// add/update post
-		rpost, err := scs.upsertSyncPost(post, targetChannel, rc)
+		rpost, err := scs.upsertSyncPost(post, targetChannel, rc, syncMsg.MentionTransforms)
 		if err != nil {
 			syncResp.PostErrors = append(syncResp.PostErrors, post.Id)
 			scs.server.Log().Log(mlog.LvlSharedChannelServiceError, "Error upserting sync post",
@@ -454,7 +454,7 @@ func (scs *Service) updateSyncUser(rctx request.CTX, patch *model.UserPatch, use
 	return nil, fmt.Errorf("error updating sync user %s: %w", user.Id, err)
 }
 
-func (scs *Service) upsertSyncPost(post *model.Post, targetChannel *model.Channel, rc *model.RemoteCluster) (*model.Post, error) {
+func (scs *Service) upsertSyncPost(post *model.Post, targetChannel *model.Channel, rc *model.RemoteCluster, mentionTransforms map[string]string) (*model.Post, error) {
 	var appErr *model.AppError
 
 	post.RemoteId = model.NewPointer(rc.RemoteId)
@@ -483,11 +483,14 @@ func (scs *Service) upsertSyncPost(post *model.Post, targetChannel *model.Channe
 			return nil, fmt.Errorf("post sync failed: %w", ErrRemoteIDMismatch)
 		}
 
+		scs.transformMentionsOnReceive(rctx, post, targetChannel, rc, mentionTransforms)
+
 		rpost, appErr = scs.app.CreatePost(rctx, post, targetChannel, model.CreatePostFlags{TriggerWebhooks: true, SetOnline: true})
 		if appErr == nil {
 			scs.server.Log().Log(mlog.LvlSharedChannelServiceDebug, "Created sync post",
 				mlog.String("post_id", post.Id),
-				mlog.String("channel_id", post.ChannelId))
+				mlog.String("channel_id", post.ChannelId),
+			)
 		}
 	} else if post.DeleteAt > 0 {
 		// delete post
@@ -499,6 +502,7 @@ func (scs *Service) upsertSyncPost(post *model.Post, targetChannel *model.Channe
 			)
 		}
 	} else if post.EditAt > rpost.EditAt || post.Message != rpost.Message || post.UpdateAt > rpost.UpdateAt || post.Metadata != nil {
+		scs.transformMentionsOnReceive(rctx, post, targetChannel, rc, mentionTransforms)
 		var priority *model.PostPriority
 		var acknowledgements []*model.PostAcknowledgement
 
@@ -741,3 +745,44 @@ func (scs *Service) upsertSyncAcknowledgement(acknowledgement *model.PostAcknowl
 	}
 	return savedAcknowledgement, retErr
 }
+
+// transformMentionsOnReceive transforms mentions in received posts using explicit mentionTransforms.
+func (scs *Service) transformMentionsOnReceive(rctx request.CTX, post *model.Post, targetChannel *model.Channel, rc *model.RemoteCluster, mentionTransforms map[string]string) {
+	if post.Message == "" || len(mentionTransforms) == 0 {
+		return
+	}
+
+	// Process mentions directly using mentionTransforms - no need to re-parse with regex
+	for mention, userID := range mentionTransforms {
+		oldMention := "@" + mention
+		var newMention string
+
+		// Get the user to determine transformation type
+		if user, err := scs.server.GetStore().User().Get(context.Background(), userID); err == nil && user != nil {
+			// User exists in receiver's database
+			if strings.Contains(mention, ":") {
+				// Colon mention (e.g., "@admin:remote1") - always use the user's actual username
+				newMention = "@" + user.Username
+			} else {
+				// Simple mention (e.g., "@admin")
+				if user.GetRemoteID() == "" {
+					// This is a local user, keep as-is
+					newMention = "@" + mention
+				} else {
+					// This is a remote user that was synced, use their synced username
+					newMention = "@" + user.Username
+				}
+			}
+		} else {
+			// User doesn't exist in receiver's database
+			if strings.Contains(mention, ":") {
+				// Colon mention for unknown user - keep as-is
+				newMention = oldMention
+			} else {
+				// Simple mention for unknown user - add cluster suffix to indicate it's from remote
+				newMention = "@" + mention + ":" + rc.Name
+			}
+		}
+		post.Message = strings.ReplaceAll(post.Message, oldMention, newMention)
+	}
+}

server/platform/services/sharedchannel/sync_send_remote.go

@@ -32,13 +32,14 @@ type syncData struct {
 	rc   *model.RemoteCluster
 	scr  *model.SharedChannelRemote
 
-	users            map[string]*model.User
+	users             map[string]*model.User
-	profileImages    map[string]*model.User
+	profileImages     map[string]*model.User
-	posts            []*model.Post
+	posts             []*model.Post
-	reactions        []*model.Reaction
+	reactions         []*model.Reaction
-	acknowledgements []*model.PostAcknowledgement
+	acknowledgements  []*model.PostAcknowledgement
-	statuses         []*model.Status
+	statuses          []*model.Status
-	attachments      []attachment
+	attachments       []attachment
+	mentionTransforms map[string]string
 
 	resultRepeat                bool
 	resultNextCursor            model.GetPostsSinceForSyncCursor
@@ -47,11 +48,12 @@ type syncData struct {
 
 func newSyncData(task syncTask, rc *model.RemoteCluster, scr *model.SharedChannelRemote) *syncData {
 	return &syncData{
-		task:          task,
+		task:              task,
-		rc:            rc,
+		rc:                rc,
-		scr:           scr,
+		scr:               scr,
-		users:         make(map[string]*model.User),
+		users:             make(map[string]*model.User),
-		profileImages: make(map[string]*model.User),
+		profileImages:     make(map[string]*model.User),
+		mentionTransforms: make(map[string]string),
 		resultNextCursor: model.GetPostsSinceForSyncCursor{
 			LastPostUpdateAt: scr.LastPostUpdateAt, LastPostUpdateID: scr.LastPostUpdateID,
 			LastPostCreateAt: scr.LastPostCreateAt, LastPostCreateID: scr.LastPostCreateID,
@@ -444,9 +446,6 @@ func (scs *Service) fetchPostUsersForSync(sd *syncData) error {
 	}
 
 	for _, post := range sd.posts {
-		// add author
-		userIDs[post.UserId] = p2mm{}
-
 		// get mentions and users for each mention
 		mentionMap := scs.app.MentionsToTeamMembers(request.EmptyContext(scs.server.Log()), post.Message, sc.TeamId)
 
@@ -458,10 +457,19 @@ func (scs *Service) fetchPostUsersForSync(sd *syncData) error {
 			}
 
 			// Skip remote users unless mention contains a colon (@username:remote)
-			if user.RemoteId != nil && !strings.Contains(mention, ":") {
+			if user.IsRemote() && !strings.Contains(mention, ":") {
 				continue
 			}
+		}
 
+		// add author with post and mentionMap so transformations can be applied
+		userIDs[post.UserId] = p2mm{
+			post:       post,
+			mentionMap: mentionMap,
+		}
+
+		// Add all mentioned users
+		for _, userID := range mentionMap {
 			userIDs[userID] = p2mm{
 				post:       post,
 				mentionMap: mentionMap,
@@ -470,7 +478,6 @@ func (scs *Service) fetchPostUsersForSync(sd *syncData) error {
 	}
 
 	merr := merror.New()
-
 	for userID, v := range userIDs {
 		user, err := scs.server.GetStore().User().Get(context.Background(), userID)
 		if err != nil {
@@ -480,7 +487,7 @@ func (scs *Service) fetchPostUsersForSync(sd *syncData) error {
 
 		sync, syncImage, err2 := scs.shouldUserSync(user, sd.task.channelID, sd.rc)
 		if err2 != nil {
-			merr.Append(fmt.Errorf("could not check should sync user %s: %w", userID, err))
+			merr.Append(fmt.Errorf("could not check should sync user %s: %w", userID, err2))
 			continue
 		}
 
@@ -492,9 +499,15 @@ func (scs *Service) fetchPostUsersForSync(sd *syncData) error {
 			sd.profileImages[user.Id] = user
 		}
 
-		// Transform @username:remote to @username when sending to a user's home cluster
+		// Collect mention transforms for all mentioned users
-		if v.post != nil && user.RemoteId != nil && *user.RemoteId == sd.rc.RemoteId {
+		if v.mentionMap != nil {
-			fixMention(v.post, v.mentionMap, user)
+			for mention, mentionUserID := range v.mentionMap {
+				if mentionUserID == userID {
+					// Always add the mention transform - let receiver decide how to display
+					// The sender should NOT modify the message, only provide the mapping
+					sd.mentionTransforms[mention] = userID
+				}
+			}
 		}
 	}
 	return merr.ErrorOrNil()
@@ -690,6 +703,7 @@ func (scs *Service) sendPostSyncData(sd *syncData) error {
 
 	msg := model.NewSyncMsg(sd.task.channelID)
 	msg.Posts = sd.posts
+	msg.MentionTransforms = sd.mentionTransforms
 
 	return scs.sendSyncMsgToRemote(msg, sd.rc, func(syncResp model.SyncResponse, errResp error) {
 		if len(syncResp.PostErrors) != 0 {

server/public/model/shared_channel.go

@@ -292,6 +292,7 @@ type SyncMsg struct {
 	Statuses          []*Status              `json:"statuses,omitempty"`
 	MembershipChanges []*MembershipChangeMsg `json:"membership_changes,omitempty"`
 	Acknowledgements  []*PostAcknowledgement `json:"acknowledgements,omitempty"`
+	MentionTransforms map[string]string      `json:"mention_transforms,omitempty"`
 }
 
 func NewSyncMsg(channelID string) *SyncMsg {