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

* initial checkin * i18n/en.json * fix e2e test * fix Makefile * initial checkin * fix logic * fix spacing * simplify * further simplify * address spec update * remove removeMention * simplify code * simplify code(2) * simplify code(3) * simplify code(4) * update comment * simplify comments * remove useless test --------- Co-authored-by: Catalin Tomai <catalintomai@catalins-macbook-pro-2.home>
2025-05-27 15:18:41 +02:00
--- a/server/platform/services/sharedchannel/sync_send_remote.go
+++ b/server/platform/services/sharedchannel/sync_send_remote.go
@@ -7,6 +7,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"strings"
 	"sync"
 	"time"

@@ -398,7 +399,19 @@ func (scs *Service) fetchPostUsersForSync(sd *syncData) error {

 		// get mentions and users for each mention
 		mentionMap := scs.app.MentionsToTeamMembers(request.EmptyContext(scs.server.Log()), post.Message, sc.TeamId)
-		for _, userID := range mentionMap {
+
+		// Skip notifications for remote users unless mentioned with @username:remote format
+		for mention, userID := range mentionMap {
+			user, err := scs.server.GetStore().User().Get(context.Background(), userID)
+			if err != nil {
+				continue
+			}
+
+			// Skip remote users unless mention contains a colon (@username:remote)
+			if user.RemoteId != nil && !strings.Contains(mention, ":") {
+				continue
+			}
+
 			userIDs[userID] = p2mm{
 				post:       post,
 				mentionMap: mentionMap,
@@ -429,8 +442,7 @@ func (scs *Service) fetchPostUsersForSync(sd *syncData) error {
 			sd.profileImages[user.Id] = user
 		}

-		// if this was a mention then put the real username in place of the username+remotename, but only
-		// when sending to the remote that the user belongs to.
+		// Transform @username:remote to @username when sending to a user's home cluster
 		if v.post != nil && user.RemoteId != nil && *user.RemoteId == sd.rc.RemoteId {
 			fixMention(v.post, v.mentionMap, user)
 		}
--- a/server/platform/services/sharedchannel/util.go
+++ b/server/platform/services/sharedchannel/util.go
@@ -12,7 +12,8 @@ import (
 	"github.com/mattermost/mattermost/server/v8/channels/store"
 )

-// fixMention replaces any mentions in a post for the user with the user's real username.
+// fixMention transforms @username:remotename mentions to @username format
+// Used when syncing posts to a user's home cluster
 func fixMention(post *model.Post, mentionMap model.UserMentionMap, user *model.User) {
 	if post == nil || len(mentionMap) == 0 {
 		return
@@ -23,8 +24,8 @@ func fixMention(post *model.Post, mentionMap model.UserMentionMap, user *model.U
 		return
 	}

-	// there may be more than one mention for each user so we have to walk the whole map.
 	for mention, id := range mentionMap {
+		// Only process mentions with colons that match this user's ID
 		if id == user.Id && strings.Contains(mention, ":") {
 			post.Message = strings.ReplaceAll(post.Message, "@"+mention, "@"+realUsername)
 		}
--- a/server/platform/services/sharedchannel/util_test.go
+++ b/server/platform/services/sharedchannel/util_test.go
@@ -7,6 +7,7 @@ import (
 	"strings"
 	"testing"

+	"github.com/mattermost/mattermost/server/public/model"
 	"github.com/stretchr/testify/require"
 )

@@ -70,7 +71,180 @@ func Test_mungUsernameFuzz(t *testing.T) {
 	}
 }

+func Test_fixMention(t *testing.T) {
+	tests := []struct {
+		name       string
+		post       *model.Post
+		mentionMap model.UserMentionMap
+		user       *model.User
+		expected   string
+	}{
+		{
+			name:       "nil post",
+			post:       nil,
+			mentionMap: model.UserMentionMap{"user:remote": "userid"},
+			user:       &model.User{Id: "userid"},
+			expected:   "",
+		},
+		{
+			name:       "empty mentionMap",
+			post:       &model.Post{Message: "hello @user:remote"},
+			mentionMap: model.UserMentionMap{},
+			user:       &model.User{Id: "userid"},
+			expected:   "hello @user:remote",
+		},
+		{
+			name:       "no RemoteUsername prop",
+			post:       &model.Post{Message: "hello @user:remote"},
+			mentionMap: model.UserMentionMap{"user:remote": "userid"},
+			user:       &model.User{Id: "userid"},
+			expected:   "hello @user:remote",
+		},
+		// Username clash test cases (same username on both local and remote)
+		// With the new behavior, when syncing to a user's home cluster, we always remove the remote suffix
+		{
+			name:       "username clash - simple mention",
+			post:       &model.Post{Message: "hello @user:remote"},
+			mentionMap: model.UserMentionMap{"user:remote": "userid"},
+			user:       &model.User{Id: "userid", RemoteId: model.NewPointer("remoteid"), Props: model.StringMap{model.UserPropsKeyRemoteUsername: "user"}},
+			expected:   "hello @user",
+		},
+		{
+			name:       "username clash - mention at start",
+			post:       &model.Post{Message: "@user:remote hello"},
+			mentionMap: model.UserMentionMap{"user:remote": "userid"},
+			user:       &model.User{Id: "userid", RemoteId: model.NewPointer("remoteid"), Props: model.StringMap{model.UserPropsKeyRemoteUsername: "user"}},
+			expected:   "@user hello",
+		},
+		{
+			name:       "username clash - multiple mentions",
+			post:       &model.Post{Message: "hello @user:remote and @user:remote again"},
+			mentionMap: model.UserMentionMap{"user:remote": "userid"},
+			user:       &model.User{Id: "userid", RemoteId: model.NewPointer("remoteid"), Props: model.StringMap{model.UserPropsKeyRemoteUsername: "user"}},
+			expected:   "hello @user and @user again",
+		},
+		{
+			name:       "simple mention different username",
+			post:       &model.Post{Message: "hello @user:remote"},
+			mentionMap: model.UserMentionMap{"user:remote": "userid"},
+			user:       &model.User{Id: "userid", RemoteId: model.NewPointer("remoteid"), Props: model.StringMap{model.UserPropsKeyRemoteUsername: "realuser"}},
+			expected:   "hello @realuser",
+		},
+		{
+			name:       "simple mention same username",
+			post:       &model.Post{Message: "hello @user:remote"},
+			mentionMap: model.UserMentionMap{"user:remote": "userid"},
+			user:       &model.User{Id: "userid", RemoteId: model.NewPointer("remoteid"), Props: model.StringMap{model.UserPropsKeyRemoteUsername: "user"}},
+			expected:   "hello @user",
+		},
+		{
+			name:       "mention at start",
+			post:       &model.Post{Message: "@user:remote hello"},
+			mentionMap: model.UserMentionMap{"user:remote": "userid"},
+			user:       &model.User{Id: "userid", RemoteId: model.NewPointer("remoteid"), Props: model.StringMap{model.UserPropsKeyRemoteUsername: "realuser"}},
+			expected:   "@realuser hello",
+		},
+		{
+			name:       "mention at end",
+			post:       &model.Post{Message: "hello @user:remote"},
+			mentionMap: model.UserMentionMap{"user:remote": "userid"},
+			user:       &model.User{Id: "userid", RemoteId: model.NewPointer("remoteid"), Props: model.StringMap{model.UserPropsKeyRemoteUsername: "realuser"}},
+			expected:   "hello @realuser",
+		},
+		{
+			name:       "multiple mentions of same user",
+			post:       &model.Post{Message: "hello @user:remote and @user:remote again"},
+			mentionMap: model.UserMentionMap{"user:remote": "userid"},
+			user:       &model.User{Id: "userid", RemoteId: model.NewPointer("remoteid"), Props: model.StringMap{model.UserPropsKeyRemoteUsername: "realuser"}},
+			expected:   "hello @realuser and @realuser again",
+		},
+		{
+			name:       "multiple different mentions",
+			post:       &model.Post{Message: "hello @user:remote and @other:remote"},
+			mentionMap: model.UserMentionMap{"user:remote": "userid", "other:remote": "otherid"},
+			user:       &model.User{Id: "userid", RemoteId: model.NewPointer("remoteid"), Props: model.StringMap{model.UserPropsKeyRemoteUsername: "realuser"}},
+			expected:   "hello @realuser and @other:remote",
+		},
+		{
+			name:       "mention without colon",
+			post:       &model.Post{Message: "hello @user"},
+			mentionMap: model.UserMentionMap{"user": "userid"},
+			user:       &model.User{Id: "userid", RemoteId: model.NewPointer("remoteid"), Props: model.StringMap{model.UserPropsKeyRemoteUsername: "realuser"}},
+			expected:   "hello @user",
+		},
+		{
+			name:       "mention different user id",
+			post:       &model.Post{Message: "hello @user:remote"},
+			mentionMap: model.UserMentionMap{"user:remote": "different"},
+			user:       &model.User{Id: "userid", RemoteId: model.NewPointer("remoteid"), Props: model.StringMap{model.UserPropsKeyRemoteUsername: "realuser"}},
+			expected:   "hello @user:remote",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			fixMention(tt.post, tt.mentionMap, tt.user)
+			if tt.post != nil {
+				require.Equal(t, tt.expected, tt.post.Message)
+			}
+		})
+	}
+}
+
 // R returns a string with the specified string repeated `count` times.
 func R(count int, s string) string {
 	return strings.Repeat(s, count)
 }
+
+func TestHandleUserMentions(t *testing.T) {
+	t.Run("should format mentions across clusters", func(t *testing.T) {
+		// This test verifies the behavior when same username exists on multiple clusters
+
+		// Define remote cluster names (not server names)
+		clusterBName := "remote_cluster_B"
+
+		// Create user with admin username on remote cluster B
+		adminUserB := &model.User{
+			Id:       "adminB",
+			Username: "admin",
+			RemoteId: model.NewPointer(clusterBName),
+			Props:    model.StringMap{model.UserPropsKeyRemoteUsername: "admin"},
+		}
+
+		// Scenario: User on Cluster B mentions local admin
+		// The mention should show as @admin on Cluster B
+		postOnClusterB := &model.Post{
+			Message: "Hey @admin, please review this.",
+		}
+
+		// Mentions are preserved when synced between clusters
+		// so that the local admin on Cluster A would receive a notification
+
+		// We're verifying the post will keep its mention format after being synced
+		require.Contains(t, postOnClusterB.Message, "@admin",
+			"Message should preserve @admin mention when synced between clusters")
+
+		// Scenario: User on Cluster A explicitly mentions admin on Cluster B
+		// It will show as @admin:remote_cluster_B on Cluster A
+		postWithRemoteMention := &model.Post{
+			Message: "Let's ask @admin:remote_cluster_B about this",
+		}
+
+		mentionMapWithRemote := model.UserMentionMap{
+			"admin:remote_cluster_B": "adminB",
+		}
+
+		// When this post is synced to Cluster B, fixMention will transform
+		// @admin:remote_cluster_B to @admin (removing the remote suffix)
+		postCopy := &model.Post{Message: postWithRemoteMention.Message}
+		fixMention(postCopy, mentionMapWithRemote, adminUserB)
+
+		// fixMention should replace @username:remote with @realusername (without the remote suffix)
+		// when syncing to the user's home cluster
+		expected := "Let's ask @admin about this"
+
+		// The part we're testing is that mentions are simplified when sent to the user's home cluster
+		require.Equal(t, expected, postCopy.Message,
+			"Should remove remote cluster suffix when message is viewed on user's home server")
+	})
+}