MM-32133 shared channel username collisions (#17347)

Support for handling username collisions between remote clusters. Users belonging to remote clusters have their username changed to include the remote name e.g. wiggin becomes wiggin:mattermost.

@mentions are also modified so the munged username is replaced with the original username when the post is sync'd with the remote the user belongs to.

When adding remote users:
- append the remote name to the username with colon separator
- append the remote name to the email address with colon separator
- store the original username and email address in user props
- when resolving @mentions replace with the stored original username

api4/shared_channel_test.go

@@ -95,7 +95,7 @@ func TestGetRemoteClusterById(t *testing.T) {
 	// create a remote cluster
 	rc := &model.RemoteCluster{
 		RemoteId:     model.NewId(),
-		DisplayName:  "Test1",
+		Name:         "Test1",
 		RemoteTeamId: model.NewId(),
 		SiteURL:      model.NewId(),
 		CreatorId:    model.NewId(),
@@ -130,7 +130,7 @@ func TestGetRemoteClusterById(t *testing.T) {
 	t.Run("valid remote, user is member", func(t *testing.T) {
 		rcInfo, resp := th.Client.GetRemoteClusterInfo(rc.RemoteId)
 		CheckNoError(t, resp)
-		assert.Equal(t, rc.DisplayName, rcInfo.DisplayName)
+		assert.Equal(t, rc.Name, rcInfo.Name)
 	})
 
 	t.Run("invalid remote", func(t *testing.T) {
@@ -173,9 +173,9 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) {
 		localUser := th.BasicUser
 		remoteUser := th.CreateUser()
 		rc := &model.RemoteCluster{
-			DisplayName: "test",
-			Token:       model.NewId(),
-			CreatorId:   localUser.Id,
+			Name:      "test",
+			Token:     model.NewId(),
+			CreatorId: localUser.Id,
 		}
 		rc, err := th.App.AddRemoteCluster(rc)
 		require.Nil(t, err)
@@ -206,9 +206,9 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) {
 		localUser := th.BasicUser
 		remoteUser := th.CreateUser()
 		rc := &model.RemoteCluster{
-			DisplayName: "test",
-			Token:       model.NewId(),
-			CreatorId:   localUser.Id,
+			Name:      "test",
+			Token:     model.NewId(),
+			CreatorId: localUser.Id,
 		}
 		rc, err := th.App.AddRemoteCluster(rc)
 		require.Nil(t, err)

app/remote_cluster_service_mock.go

@@ -66,7 +66,7 @@ func (mrcs *mockRemoteClusterService) SendFile(ctx context.Context, us *model.Up
 	return nil
 }
 
-func (mrcs *mockRemoteClusterService) AcceptInvitation(invite *model.RemoteClusterInvite, name string, creatorId string, teamId string, siteURL string) (*model.RemoteCluster, error) {
+func (mrcs *mockRemoteClusterService) AcceptInvitation(invite *model.RemoteClusterInvite, name string, displayName string, creatorId string, teamId string, siteURL string) (*model.RemoteCluster, error) {
 	return nil, nil
 }
 

app/remote_cluster_test.go

@@ -19,7 +19,7 @@ func TestAddRemoteCluster(t *testing.T) {
 
 		remoteCluster := &model.RemoteCluster{
 			RemoteTeamId: model.NewId(),
-			DisplayName:  "test",
+			Name:         "test",
 			SiteURL:      "http://localhost:8065",
 			Token:        "test",
 			RemoteToken:  "test",
@@ -42,7 +42,7 @@ func TestAddRemoteCluster(t *testing.T) {
 
 		remoteCluster := &model.RemoteCluster{
 			RemoteTeamId: model.NewId(),
-			DisplayName:  "test",
+			Name:         "test",
 			SiteURL:      "http://localhost:8065",
 			Token:        "test",
 			RemoteToken:  "test",
@@ -76,7 +76,7 @@ func TestUpdateRemoteCluster(t *testing.T) {
 
 		remoteCluster := &model.RemoteCluster{
 			RemoteTeamId: model.NewId(),
-			DisplayName:  "test",
+			Name:         "test",
 			SiteURL:      "http://localhost:8065",
 			Token:        "test",
 			RemoteToken:  "test",
@@ -86,7 +86,7 @@ func TestUpdateRemoteCluster(t *testing.T) {
 
 		otherRemoteCluster := &model.RemoteCluster{
 			RemoteTeamId: model.NewId(),
-			DisplayName:  "test",
+			Name:         "test",
 			SiteURL:      "http://localhost:8066",
 			Token:        "test",
 			RemoteToken:  "test",
@@ -113,7 +113,7 @@ func TestUpdateRemoteCluster(t *testing.T) {
 
 		remoteCluster := &model.RemoteCluster{
 			RemoteTeamId: model.NewId(),
-			DisplayName:  "test",
+			Name:         "test",
 			SiteURL:      "http://localhost:8065",
 			Token:        "test",
 			RemoteToken:  "test",
@@ -123,7 +123,7 @@ func TestUpdateRemoteCluster(t *testing.T) {
 
 		otherRemoteCluster := &model.RemoteCluster{
 			RemoteTeamId: model.NewId(),
-			DisplayName:  "test",
+			Name:         "test",
 			SiteURL:      "http://localhost:8066",
 			Token:        "test",
 			RemoteToken:  "test",

app/session_test.go

@@ -448,7 +448,7 @@ func TestGetRemoteClusterSession(t *testing.T) {
 	rc := model.RemoteCluster{
 		RemoteId:     remoteId,
 		RemoteTeamId: model.NewId(),
-		DisplayName:  "test",
+		Name:         "test",
 		Token:        token,
 		CreatorId:    model.NewId(),
 	}

app/slashcommands/command_remote.go

@@ -40,10 +40,12 @@ func (rp *RemoteProvider) GetCommand(a *app.App, T i18n.TranslateFunc) *model.Co
 	invite := model.NewAutocompleteData("invite", "", T("api.command_remote.invite.help"))
 	invite.AddNamedTextArgument("password", T("api.command_remote.invite_password.help"), T("api.command_remote.invite_password.hint"), "", true)
 	invite.AddNamedTextArgument("name", T("api.command_remote.name.help"), T("api.command_remote.name.hint"), "", true)
+	invite.AddNamedTextArgument("displayname", T("api.command_remote.displayname.help"), T("api.command_remote.displayname.hint"), "", false)
 
 	accept := model.NewAutocompleteData("accept", "", T("api.command_remote.accept.help"))
 	accept.AddNamedTextArgument("password", T("api.command_remote.invite_password.help"), T("api.command_remote.invite_password.hint"), "", true)
 	accept.AddNamedTextArgument("name", T("api.command_remote.name.help"), T("api.command_remote.name.hint"), "", true)
+	accept.AddNamedTextArgument("displayname", T("api.command_remote.displayname.help"), T("api.command_remote.displayname.hint"), "", false)
 	accept.AddNamedTextArgument("invite", T("api.command_remote.invitation.help"), T("api.command_remote.invitation.hint"), "", true)
 
 	remove := model.NewAutocompleteData("remove", "", T("api.command_remote.remove.help"))
@@ -114,6 +116,7 @@ func (rp *RemoteProvider) doInvite(a *app.App, args *model.CommandArgs, margs ma
 	if name == "" {
 		return responsef(args.T("api.command_remote.missing_empty", map[string]interface{}{"Arg": "name"}))
 	}
+	displayname := margs["displayname"]
 
 	url := a.GetSiteURL()
 	if url == "" {
@@ -121,7 +124,8 @@ func (rp *RemoteProvider) doInvite(a *app.App, args *model.CommandArgs, margs ma
 	}
 
 	rc := &model.RemoteCluster{
-		DisplayName: name,
+		Name:        name,
+		DisplayName: displayname,
 		Token:       model.NewId(),
 		CreatorId:   args.UserId,
 	}
@@ -159,6 +163,7 @@ func (rp *RemoteProvider) doAccept(a *app.App, args *model.CommandArgs, margs ma
 	if name == "" {
 		return responsef(args.T("api.command_remote.missing_empty", map[string]interface{}{"Arg": "name"}))
 	}
+	displayname := margs["displayname"]
 
 	blob := margs["invite"]
 	if blob == "" {
@@ -186,7 +191,7 @@ func (rp *RemoteProvider) doAccept(a *app.App, args *model.CommandArgs, margs ma
 		return responsef(args.T("api.command_remote.site_url_not_set"))
 	}
 
-	rc, err := rcs.AcceptInvitation(invite, name, args.UserId, args.TeamId, url)
+	rc, err := rcs.AcceptInvitation(invite, name, displayname, args.UserId, args.TeamId, url)
 	if err != nil {
 		return responsef(args.T("api.command_remote.accept_invitation.error", map[string]interface{}{"Error": err.Error()}))
 	}
@@ -241,7 +246,7 @@ func (rp *RemoteProvider) doStatus(a *app.App, args *model.CommandArgs, _ map[st
 
 		lastPing := formatTimestamp(model.GetTimeForMillis(rc.LastPingAt))
 
-		fmt.Fprintf(&sb, "| %s | %s | %s | %s | %s | %s |\n", rc.DisplayName, rc.SiteURL, rc.RemoteId, accepted, online, lastPing)
+		fmt.Fprintf(&sb, "| %s | %s | %s | %s | %s | %s | %s |\n", rc.Name, rc.DisplayName, rc.SiteURL, rc.RemoteId, accepted, online, lastPing)
 	}
 	return responsef(sb.String())
 }

i18n/en.json

@@ -1110,6 +1110,14 @@
     "id": "api.command_remote.desc",
     "translation": "Invite remote Mattermost clusters for inter-cluster communication."
   },
+  {
+    "id": "api.command_remote.displayname.help",
+    "translation": "Remote cluster display name"
+  },
+  {
+    "id": "api.command_remote.displayname.hint",
+    "translation": "A display name for the remote cluster"
+  },
   {
     "id": "api.command_remote.encrypt_invitation.error",
     "translation": "Could not encrypt invitation: {{.Error}}"
@@ -1172,7 +1180,7 @@
   },
   {
     "id": "api.command_remote.name.hint",
-    "translation": "A display name for the remote cluster"
+    "translation": "A unique name for the remote cluster"
   },
   {
     "id": "api.command_remote.permission_required",

model/at_mentions.go

@@ -8,7 +8,7 @@ import (
 	"strings"
 )
 
-var atMentionRegexp = regexp.MustCompile(`\B@[[:alnum:]][[:alnum:]\.\-_]*`)
+var atMentionRegexp = regexp.MustCompile(`\B@[[:alnum:]][[:alnum:]\.\-_:]*`)
 
 const usernameSpecialChars = ".-_"
 
@@ -24,7 +24,7 @@ func PossibleAtMentions(message string) []string {
 	alreadyMentioned := make(map[string]bool)
 	for _, match := range atMentionRegexp.FindAllString(message, -1) {
 		name := NormalizeUsername(match[1:])
-		if !alreadyMentioned[name] && IsValidUsername(name) {
+		if !alreadyMentioned[name] && IsValidUsernameAllowRemote(name) {
 			names = append(names, name)
 			alreadyMentioned[name] = true
 		}

model/auditconv.go

@@ -673,6 +673,7 @@ func (h auditOutgoingWebhook) IsNil() bool {
 type auditRemoteCluster struct {
 	RemoteId     string
 	RemoteTeamId string
+	Name         string
 	DisplayName  string
 	SiteURL      string
 	CreateAt     int64
@@ -686,6 +687,7 @@ func newRemoteCluster(r *RemoteCluster) auditRemoteCluster {
 	if r != nil {
 		rc.RemoteId = r.RemoteId
 		rc.RemoteTeamId = r.RemoteTeamId
+		rc.Name = r.Name
 		rc.DisplayName = r.DisplayName
 		rc.SiteURL = r.SiteURL
 		rc.CreateAt = r.CreateAt
@@ -698,6 +700,7 @@ func newRemoteCluster(r *RemoteCluster) auditRemoteCluster {
 func (r auditRemoteCluster) MarshalJSONObject(enc *gojay.Encoder) {
 	enc.StringKey("remote_id", r.RemoteId)
 	enc.StringKey("remote_team_id", r.RemoteTeamId)
+	enc.StringKey("name", r.Name)
 	enc.StringKey("display_name", r.DisplayName)
 	enc.StringKey("site_url", r.SiteURL)
 	enc.Int64Key("create_at", r.CreateAt)

model/config.go

@@ -19,6 +19,7 @@ import (
 	"time"
 
 	"github.com/mattermost/ldap"
+
 	"github.com/mattermost/mattermost-server/v5/shared/filestore"
 	"github.com/mattermost/mattermost-server/v5/shared/mlog"
 )

model/remote_cluster.go

@@ -11,16 +11,24 @@ import (
 	"encoding/json"
 	"io"
 	"net/http"
+	"regexp"
 	"strings"
 )
 
 const (
 	RemoteOfflineAfterMillis = 1000 * 60 * 5 // 5 minutes
+	RemoteNameMinLength      = 1
+	RemoteNameMaxLength      = 64
+)
+
+var (
+	validRemoteNameChars = regexp.MustCompile(`^[a-zA-Z0-9\.\-\_]+$`)
 )
 
 type RemoteCluster struct {
 	RemoteId     string `json:"remote_id"`
 	RemoteTeamId string `json:"remote_team_id"`
+	Name         string `json:"name"`
 	DisplayName  string `json:"display_name"`
 	SiteURL      string `json:"site_url"`
 	CreateAt     int64  `json:"create_at"`
@@ -36,6 +44,14 @@ func (rc *RemoteCluster) PreSave() {
 		rc.RemoteId = NewId()
 	}
 
+	if rc.DisplayName == "" {
+		rc.DisplayName = rc.Name
+	}
+
+	rc.Name = SanitizeUnicode(rc.Name)
+	rc.DisplayName = SanitizeUnicode(rc.DisplayName)
+	rc.Name = NormalizeRemoteName(rc.Name)
+
 	if rc.Token == "" {
 		rc.Token = NewId()
 	}
@@ -51,8 +67,8 @@ func (rc *RemoteCluster) IsValid() *AppError {
 		return NewAppError("RemoteCluster.IsValid", "model.cluster.is_valid.id.app_error", nil, "id="+rc.RemoteId, http.StatusBadRequest)
 	}
 
-	if rc.DisplayName == "" {
-		return NewAppError("RemoteCluster.IsValid", "model.cluster.is_valid.name.app_error", nil, "display_name empty", http.StatusBadRequest)
+	if !IsValidRemoteName(rc.Name) {
+		return NewAppError("RemoteCluster.IsValid", "model.cluster.is_valid.name.app_error", nil, "name="+rc.Name, http.StatusBadRequest)
 	}
 
 	if rc.CreateAt == 0 {
@@ -65,7 +81,21 @@ func (rc *RemoteCluster) IsValid() *AppError {
 	return nil
 }
 
+func IsValidRemoteName(s string) bool {
+	if len(s) < RemoteNameMinLength || len(s) > RemoteNameMaxLength {
+		return false
+	}
+	return validRemoteNameChars.MatchString(s)
+}
+
 func (rc *RemoteCluster) PreUpdate() {
+	if rc.DisplayName == "" {
+		rc.DisplayName = rc.Name
+	}
+
+	rc.Name = SanitizeUnicode(rc.Name)
+	rc.DisplayName = SanitizeUnicode(rc.DisplayName)
+	rc.Name = NormalizeRemoteName(rc.Name)
 	rc.fixTopics()
 }
 
@@ -105,12 +135,17 @@ func (rc *RemoteCluster) ToJSON() (string, error) {
 
 func (rc *RemoteCluster) ToRemoteClusterInfo() RemoteClusterInfo {
 	return RemoteClusterInfo{
+		Name:        rc.Name,
 		DisplayName: rc.DisplayName,
 		CreateAt:    rc.CreateAt,
 		LastPingAt:  rc.LastPingAt,
 	}
 }
 
+func NormalizeRemoteName(name string) string {
+	return strings.ToLower(name)
+}
+
 func RemoteClusterFromJSON(data io.Reader) (*RemoteCluster, *AppError) {
 	var rc RemoteCluster
 	err := json.NewDecoder(data).Decode(&rc)
@@ -122,6 +157,7 @@ func RemoteClusterFromJSON(data io.Reader) (*RemoteCluster, *AppError) {
 
 // RemoteClusterInfo provides a subset of RemoteCluster fields suitable for sending to clients.
 type RemoteClusterInfo struct {
+	Name        string `json:"name"`
 	DisplayName string `json:"display_name"`
 	CreateAt    int64  `json:"create_at"`
 	LastPingAt  int64  `json:"last_ping_at"`

model/remote_cluster_test.go

@@ -15,7 +15,7 @@ import (
 )
 
 func TestRemoteClusterJson(t *testing.T) {
-	o := RemoteCluster{RemoteId: NewId(), DisplayName: "test"}
+	o := RemoteCluster{RemoteId: NewId(), Name: "test"}
 
 	json, err := o.ToJSON()
 	require.NoError(t, err)
@@ -24,7 +24,7 @@ func TestRemoteClusterJson(t *testing.T) {
 	require.Nil(t, appErr)
 
 	require.Equal(t, o.RemoteId, ro.RemoteId)
-	require.Equal(t, o.DisplayName, ro.DisplayName)
+	require.Equal(t, o.Name, ro.Name)
 }
 
 func TestRemoteClusterIsValid(t *testing.T) {
@@ -38,13 +38,13 @@ func TestRemoteClusterIsValid(t *testing.T) {
 	}{
 		{name: "Zero value", rc: &RemoteCluster{}, valid: false},
 		{name: "Missing cluster_name", rc: &RemoteCluster{RemoteId: id}, valid: false},
-		{name: "Missing host_name", rc: &RemoteCluster{RemoteId: id, DisplayName: "test cluster"}, valid: false},
-		{name: "Missing create_at", rc: &RemoteCluster{RemoteId: id, DisplayName: "test cluster", SiteURL: "example.com"}, valid: false},
-		{name: "Missing last_ping_at", rc: &RemoteCluster{RemoteId: id, DisplayName: "test cluster", SiteURL: "example.com", CreatorId: creator, CreateAt: now}, valid: true},
-		{name: "Missing creator", rc: &RemoteCluster{RemoteId: id, DisplayName: "test cluster", SiteURL: "example.com", CreateAt: now, LastPingAt: now}, valid: false},
-		{name: "RemoteCluster valid", rc: &RemoteCluster{RemoteId: id, DisplayName: "test cluster", SiteURL: "example.com", CreateAt: now, LastPingAt: now, CreatorId: creator}, valid: true},
-		{name: "Include protocol", rc: &RemoteCluster{RemoteId: id, DisplayName: "test cluster", SiteURL: "http://example.com", CreateAt: now, LastPingAt: now, CreatorId: creator}, valid: true},
-		{name: "Include protocol & port", rc: &RemoteCluster{RemoteId: id, DisplayName: "test cluster", SiteURL: "http://example.com:8065", CreateAt: now, LastPingAt: now, CreatorId: creator}, valid: true},
+		{name: "Missing host_name", rc: &RemoteCluster{RemoteId: id, Name: NewId()}, valid: false},
+		{name: "Missing create_at", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "example.com"}, valid: false},
+		{name: "Missing last_ping_at", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "example.com", CreatorId: creator, CreateAt: now}, valid: true},
+		{name: "Missing creator", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "example.com", CreateAt: now, LastPingAt: now}, valid: false},
+		{name: "RemoteCluster valid", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "example.com", CreateAt: now, LastPingAt: now, CreatorId: creator}, valid: true},
+		{name: "Include protocol", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "http://example.com", CreateAt: now, LastPingAt: now, CreatorId: creator}, valid: true},
+		{name: "Include protocol & port", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "http://example.com:8065", CreateAt: now, LastPingAt: now, CreatorId: creator}, valid: true},
 	}
 
 	for _, item := range data {
@@ -60,7 +60,7 @@ func TestRemoteClusterIsValid(t *testing.T) {
 func TestRemoteClusterPreSave(t *testing.T) {
 	now := GetMillis()
 
-	o := RemoteCluster{RemoteId: NewId(), DisplayName: "test"}
+	o := RemoteCluster{RemoteId: NewId(), Name: NewId()}
 	o.PreSave()
 
 	require.GreaterOrEqual(t, o.CreateAt, now)

model/user.go

@@ -272,8 +272,14 @@ func (u *User) IsValid() *AppError {
 		return InvalidUserError("update_at", u.Id)
 	}
 
-	if !IsValidUsername(u.Username) {
-		return InvalidUserError("username", u.Id)
+	if u.IsRemote() {
+		if !IsValidUsernameAllowRemote(u.Username) {
+			return InvalidUserError("username", u.Id)
+		}
+	} else {
+		if !IsValidUsername(u.Username) {
+			return InvalidUserError("username", u.Id)
+		}
 	}
 
 	if len(u.Email) > USER_EMAIL_MAX_LENGTH || u.Email == "" || !IsValidEmail(u.Email) {
@@ -746,6 +752,21 @@ func (u *User) IsRemote() bool {
 	return u.RemoteId != nil && *u.RemoteId != ""
 }
 
+// GetProp fetches a prop value by name.
+func (u *User) GetProp(name string) (string, bool) {
+	val, ok := u.Props[name]
+	return val, ok
+}
+
+// SetProp sets a prop value by name, creating the map if nil.
+// Not thread safe.
+func (u *User) SetProp(name string, value string) {
+	if u.Props == nil {
+		u.Props = make(map[string]string)
+	}
+	u.Props[name] = value
+}
+
 func (u *User) ToPatch() *UserPatch {
 	return &UserPatch{
 		Username: &u.Username, Password: &u.Password,
@@ -836,12 +857,13 @@ func ComparePassword(hash string, password string) bool {
 }
 
 var validUsernameChars = regexp.MustCompile(`^[a-z0-9\.\-_]+$`)
+var validUsernameCharsForRemote = regexp.MustCompile(`^[a-z0-9\.\-_:]+$`)
 
-var restrictedUsernames = []string{
-	"all",
-	"channel",
-	"matterbot",
-	"system",
+var restrictedUsernames = map[string]struct{}{
+	"all":       {},
+	"channel":   {},
+	"matterbot": {},
+	"system":    {},
 }
 
 func IsValidUsername(s string) bool {
@@ -853,13 +875,21 @@ func IsValidUsername(s string) bool {
 		return false
 	}
 
-	for _, restrictedUsername := range restrictedUsernames {
-		if s == restrictedUsername {
-			return false
-		}
+	_, found := restrictedUsernames[s]
+	return !found
+}
+
+func IsValidUsernameAllowRemote(s string) bool {
+	if len(s) < USER_NAME_MIN_LENGTH || len(s) > USER_NAME_MAX_LENGTH {
+		return false
 	}
 
-	return true
+	if !validUsernameCharsForRemote.MatchString(s) {
+		return false
+	}
+
+	_, found := restrictedUsernames[s]
+	return !found
 }
 
 func CleanUsername(username string) string {

model/user_test.go

@@ -211,26 +211,30 @@ func TestUserGetDisplayNameWithPrefix(t *testing.T) {
 	assert.Equal(t, user.GetDisplayNameWithPrefix(SHOW_NICKNAME_FULLNAME, "@"), "nickname", "Display name should be nickname")
 }
 
-var usernames = []struct {
-	value    string
-	expected bool
-}{
-	{"spin-punch", true},
-	{"sp", true},
-	{"s", true},
-	{"1spin-punch", true},
-	{"-spin-punch", true},
-	{".spin-punch", true},
-	{"Spin-punch", false},
-	{"spin punch-", false},
-	{"spin_punch", true},
-	{"spin", true},
-	{"PUNCH", false},
-	{"spin.punch", true},
-	{"spin'punch", false},
-	{"spin*punch", false},
-	{"all", false},
-	{"system", false},
+type usernamesTest struct {
+	value              string
+	expected           bool
+	expectedWhenRemote bool
+}
+
+var usernames = []usernamesTest{
+	{"spin-punch", true, true},
+	{"sp", true, true},
+	{"s", true, true},
+	{"1spin-punch", true, true},
+	{"-spin-punch", true, true},
+	{".spin-punch", true, true},
+	{"Spin-punch", false, false},
+	{"spin punch-", false, false},
+	{"spin_punch", true, true},
+	{"spin", true, true},
+	{"PUNCH", false, false},
+	{"spin.punch", true, true},
+	{"spin'punch", false, false},
+	{"spin*punch", false, false},
+	{"all", false, false},
+	{"system", false, false},
+	{"spin:punch", false, true},
 }
 
 func TestValidUsername(t *testing.T) {
@@ -239,6 +243,11 @@ func TestValidUsername(t *testing.T) {
 			t.Errorf("expect %v as %v", v.value, v.expected)
 		}
 	}
+	for _, v := range usernames {
+		if IsValidUsernameAllowRemote(v.value) != v.expectedWhenRemote {
+			t.Errorf("expect %v as %v", v.value, v.expectedWhenRemote)
+		}
+	}
 }
 
 func TestNormalizeUsername(t *testing.T) {

services/remotecluster/invitation.go

@@ -12,11 +12,12 @@ import (
 )
 
 // AcceptInvitation is called when accepting an invitation to connect with a remote cluster.
-func (rcs *Service) AcceptInvitation(invite *model.RemoteClusterInvite, name string, creatorId string, teamId string, siteURL string) (*model.RemoteCluster, error) {
+func (rcs *Service) AcceptInvitation(invite *model.RemoteClusterInvite, name string, displayName, creatorId string, teamId string, siteURL string) (*model.RemoteCluster, error) {
 	rc := &model.RemoteCluster{
 		RemoteId:     invite.RemoteId,
 		RemoteTeamId: invite.RemoteTeamId,
-		DisplayName:  name,
+		Name:         name,
+		DisplayName:  displayName,
 		Token:        model.NewId(),
 		RemoteToken:  invite.Token,
 		SiteURL:      invite.SiteURL,

services/remotecluster/send_test.go

@@ -177,14 +177,14 @@ func makeRemoteClusters(num int, siteURL string) []*model.RemoteCluster {
 
 func makeRemoteCluster(name string, siteURL string, topics string) *model.RemoteCluster {
 	return &model.RemoteCluster{
-		RemoteId:    model.NewId(),
-		DisplayName: name,
-		SiteURL:     siteURL,
-		Token:       model.NewId(),
-		Topics:      topics,
-		CreateAt:    model.GetMillis(),
-		LastPingAt:  model.GetMillis(),
-		CreatorId:   model.NewId(),
+		RemoteId:   model.NewId(),
+		Name:       name,
+		SiteURL:    siteURL,
+		Token:      model.NewId(),
+		Topics:     topics,
+		CreateAt:   model.GetMillis(),
+		LastPingAt: model.GetMillis(),
+		CreatorId:  model.NewId(),
 	}
 }
 

services/remotecluster/sendmsg.go

@@ -84,7 +84,9 @@ func (rcs *Service) sendMsg(task sendMsgTask) {
 	defer func() {
 		if r := recover(); r != nil {
 			rcs.server.GetLogger().Log(mlog.LvlRemoteClusterServiceError, "Remote Cluster sendMsg panic",
-				mlog.String("remote", task.rc.DisplayName), mlog.String("msgId", task.msg.Id), mlog.Any("panic", r))
+				mlog.String("remote", task.rc.DisplayName),
+				mlog.String("msgId", task.msg.Id), mlog.Any("panic", r),
+			)
 		}
 
 		if errResp != nil {

services/remotecluster/service.go

@@ -63,7 +63,7 @@ type RemoteClusterServiceIFace interface {
 	RemoveConnectionStateListener(listenerId string)
 	SendMsg(ctx context.Context, msg model.RemoteClusterMsg, rc *model.RemoteCluster, f SendMsgResultFunc) error
 	SendFile(ctx context.Context, us *model.UploadSession, fi *model.FileInfo, rc *model.RemoteCluster, rp ReaderProvider, f SendFileResultFunc) error
-	AcceptInvitation(invite *model.RemoteClusterInvite, name string, creatorId string, teamId string, siteURL string) (*model.RemoteCluster, error)
+	AcceptInvitation(invite *model.RemoteClusterInvite, name string, displayName string, creatorId string, teamId string, siteURL string) (*model.RemoteCluster, error)
 	ReceiveIncomingMsg(rc *model.RemoteCluster, msg model.RemoteClusterMsg) Response
 }
 

services/sharedchannel/channelinvite_test.go

@@ -59,7 +59,7 @@ func TestOnReceiveChannelInvite(t *testing.T) {
 		}
 
 		mockStore := &mocks.Store{}
-		remoteCluster := &model.RemoteCluster{DisplayName: "test"}
+		remoteCluster := &model.RemoteCluster{Name: "test"}
 		invitation := channelInviteMsg{
 			ChannelId: model.NewId(),
 			TeamId:    model.NewId(),
@@ -119,7 +119,7 @@ func TestOnReceiveChannelInvite(t *testing.T) {
 		}
 
 		mockStore := &mocks.Store{}
-		remoteCluster := &model.RemoteCluster{DisplayName: "test"}
+		remoteCluster := &model.RemoteCluster{Name: "test2"}
 		invitation := channelInviteMsg{
 			ChannelId: model.NewId(),
 			TeamId:    model.NewId(),
@@ -161,7 +161,7 @@ func TestOnReceiveChannelInvite(t *testing.T) {
 		}
 
 		mockStore := &mocks.Store{}
-		remoteCluster := &model.RemoteCluster{DisplayName: "test", CreatorId: model.NewId()}
+		remoteCluster := &model.RemoteCluster{Name: "test3", CreatorId: model.NewId()}
 		invitation := channelInviteMsg{
 			ChannelId:            model.NewId(),
 			TeamId:               model.NewId(),

services/sharedchannel/mock_AppIface_test.go

@@ -230,6 +230,22 @@ func (_m *MockAppIface) GetOrCreateDirectChannel(userId string, otherUserId stri
 	return r0, r1
 }
 
+// MentionsToTeamMembers provides a mock function with given fields: message, teamID
+func (_m *MockAppIface) MentionsToTeamMembers(message string, teamID string) model.UserMentionMap {
+	ret := _m.Called(message, teamID)
+
+	var r0 model.UserMentionMap
+	if rf, ok := ret.Get(0).(func(string, string) model.UserMentionMap); ok {
+		r0 = rf(message, teamID)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(model.UserMentionMap)
+		}
+	}
+
+	return r0
+}
+
 // PatchChannelModerationsForChannel provides a mock function with given fields: channel, channelModerationsPatch
 func (_m *MockAppIface) PatchChannelModerationsForChannel(channel *model.Channel, channelModerationsPatch []*model.ChannelModerationPatch) ([]*model.ChannelModeration, *model.AppError) {
 	ret := _m.Called(channel, channelModerationsPatch)

services/sharedchannel/msg.go

@@ -6,6 +6,7 @@ package sharedchannel
 import (
 	"context"
 	"encoding/json"
+	"strings"
 
 	"github.com/mattermost/mattermost-server/v5/model"
 	"github.com/mattermost/mattermost-server/v5/shared/mlog"
@@ -54,6 +55,7 @@ func (u userCache) Add(id string) {
 func (scs *Service) postsToSyncMessages(posts []*model.Post, rc *model.RemoteCluster, nextSyncAt int64) ([]syncMsg, error) {
 	syncMessages := make([]syncMsg, 0, len(posts))
 
+	var teamId string
 	uCache := make(userCache)
 
 	for _, p := range posts {
@@ -61,6 +63,19 @@ func (scs *Service) postsToSyncMessages(posts []*model.Post, rc *model.RemoteClu
 			continue
 		}
 
+		// lookup team id once
+		if teamId == "" {
+			sc, err := scs.server.GetStore().SharedChannel().Get(p.ChannelId)
+			if err != nil {
+				scs.server.GetLogger().Log(mlog.LvlSharedChannelServiceError, "Could not get shared channel for post",
+					mlog.String("post_id", p.Id),
+					mlog.Err(err),
+				)
+				continue
+			}
+			teamId = sc.TeamId
+		}
+
 		// any reactions originating from the remote cluster are filtered out
 		reactions, err := scs.server.GetStore().Reaction().GetForPostSince(p.Id, nextSyncAt, rc.RemoteId, true)
 		if err != nil {
@@ -104,7 +119,7 @@ func (scs *Service) postsToSyncMessages(posts []*model.Post, rc *model.RemoteClu
 		}
 
 		// any users originating from the remote cluster are filtered out
-		users := scs.usersForPost(postSync, reactions, rc, uCache)
+		users := scs.usersForPost(postSync, reactions, teamId, rc, uCache)
 
 		// if everything was filtered out then don't send an empty message.
 		if postSync == nil && len(reactions) == 0 && len(users) == 0 {
@@ -127,8 +142,9 @@ func (scs *Service) postsToSyncMessages(posts []*model.Post, rc *model.RemoteClu
 // usersForPost provides a list of Users associated with the post that need to be synchronized.
 // The user cache ensures the same user is not synchronized redundantly if they appear in multiple
 // posts for this sync batch.
-func (scs *Service) usersForPost(post *model.Post, reactions []*model.Reaction, rc *model.RemoteCluster, uCache userCache) []*model.User {
+func (scs *Service) usersForPost(post *model.Post, reactions []*model.Reaction, teamID string, rc *model.RemoteCluster, uCache userCache) []*model.User {
 	userIds := make([]string, 0)
+	var mentionMap model.UserMentionMap
 
 	if post != nil && !uCache.Has(post.UserId) {
 		userIds = append(userIds, post.UserId)
@@ -142,7 +158,20 @@ func (scs *Service) usersForPost(post *model.Post, reactions []*model.Reaction,
 		}
 	}
 
-	// TODO: extract @mentions to local users and sync those as well?
+	// get mentions and userids for each mention
+	if post != nil {
+		mentionMap = scs.app.MentionsToTeamMembers(post.Message, teamID)
+		for mention, id := range mentionMap {
+			if !uCache.Has(id) {
+				userIds = append(userIds, id)
+				uCache.Add(id)
+				scs.server.GetLogger().Log(mlog.LvlSharedChannelServiceDebug, "Found mention",
+					mlog.String("mention", mention),
+					mlog.String("user_id", id),
+				)
+			}
+		}
+	}
 
 	users := make([]*model.User, 0)
 
@@ -152,27 +181,52 @@ func (scs *Service) usersForPost(post *model.Post, reactions []*model.Reaction,
 			if sync, err2 := scs.shouldUserSync(user, rc); err2 != nil {
 				scs.server.GetLogger().Log(mlog.LvlSharedChannelServiceError, "Could not find user for post",
 					mlog.String("user_id", id),
-					mlog.Err(err2))
+					mlog.Err(err2),
+				)
 				continue
 			} else if sync {
 				users = append(users, sanitizeUserForSync(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.
+			if user.RemoteId != nil && *user.RemoteId == rc.RemoteId {
+				fixMention(post, mentionMap, user)
+			}
 		} else {
 			scs.server.GetLogger().Log(mlog.LvlSharedChannelServiceError, "Error checking if user should sync",
 				mlog.String("user_id", id),
-				mlog.Err(err))
+				mlog.Err(err),
+			)
 		}
 	}
 	return users
 }
 
+// fixMention replaces any mentions in a post for the user with the user's real username.
+func fixMention(post *model.Post, mentionMap model.UserMentionMap, user *model.User) {
+	if post == nil || len(mentionMap) == 0 {
+		return
+	}
+
+	realUsername, ok := user.GetProp(KeyRemoteUsername)
+	if !ok {
+		return
+	}
+
+	// there may be more than one mention for each user so we have to walk the whole map.
+	for mention, id := range mentionMap {
+		if id == user.Id && strings.Contains(mention, ":") {
+			post.Message = strings.ReplaceAll(post.Message, "@"+mention, "@"+realUsername)
+		}
+	}
+}
+
 func sanitizeUserForSync(user *model.User) *model.User {
 	user.Password = model.NewId()
 	user.AuthData = nil
 	user.AuthService = ""
 	user.Roles = "system_user"
 	user.AllowMarketing = false
-	user.Props = model.StringMap{}
 	user.NotifyProps = model.StringMap{}
 	user.LastPasswordUpdate = 0
 	user.LastPictureUpdate = 0

services/sharedchannel/service.go

@@ -25,6 +25,9 @@ const (
 	MaxPostsPerSync              = 12 // a bit more than one typical screenfull of posts
 	NotifyRemoteOfflineThreshold = time.Second * 10
 	NotifyMinimumDelay           = time.Second * 2
+	MaxUpsertRetries             = 25
+	KeyRemoteUsername            = "RemoteUsername"
+	KeyRemoteEmail               = "RemoteEmail"
 )
 
 // Mocks can be re-generated with `make sharedchannel-mocks`.
@@ -53,6 +56,7 @@ type AppIface interface {
 	PatchChannelModerationsForChannel(channel *model.Channel, channelModerationsPatch []*model.ChannelModerationPatch) ([]*model.ChannelModeration, *model.AppError)
 	CreateUploadSession(us *model.UploadSession) (*model.UploadSession, *model.AppError)
 	FileReader(path string) (filestore.ReadCloseSeeker, *model.AppError)
+	MentionsToTeamMembers(message, teamID string) model.UserMentionMap
 }
 
 // errNotFound allows checking against Store.ErrNotFound errors without making Store a dependency.

services/sharedchannel/sync_recv.go

@@ -8,6 +8,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"strconv"
 
 	"github.com/mattermost/mattermost-server/v5/model"
 	"github.com/mattermost/mattermost-server/v5/services/remotecluster"
@@ -117,7 +118,7 @@ func (scs *Service) processSyncMessages(syncMessages []syncMsg, rc *model.Remote
 				sm.Post.Message = scs.processPermalinkFromRemote(sm.Post, team)
 			}
 
-			// add/update post (may be nil if only reactions changed)
+			// add/update post
 			rpost, err := scs.upsertSyncPost(sm.Post, channel, rc)
 			if err != nil {
 				postErrors = append(postErrors, sm.Post.Id)
@@ -169,11 +170,11 @@ func (scs *Service) processSyncMessages(syncMessages []syncMsg, rc *model.Remote
 
 func (scs *Service) upsertSyncUser(user *model.User, channel *model.Channel, rc *model.RemoteCluster) (*model.User, error) {
 	var err error
-	var userSaved *model.User
+	if user.RemoteId == nil || *user.RemoteId == "" {
+		user.RemoteId = model.NewString(rc.RemoteId)
+	}
 
-	user.RemoteId = model.NewString(rc.RemoteId)
-
-	// does the user already exist?
+	// Check if user already exists
 	euser, err := scs.server.GetStore().User().Get(context.Background(), user.Id)
 	if err != nil {
 		if _, ok := err.(errNotFound); !ok {
@@ -181,41 +182,33 @@ func (scs *Service) upsertSyncUser(user *model.User, channel *model.Channel, rc
 		}
 	}
 
+	var userSaved *model.User
 	if euser == nil {
-		if userSaved, err = scs.server.GetStore().User().Save(user); err != nil {
-			if e, ok := err.(errInvalidInput); ok {
-				_, field, value := e.InvalidInputInfo()
-				if field == "email" || field == "username" {
-					// username or email collision
-					// TODO: handle collision by modifying username/email (MM-32133)
-					return nil, fmt.Errorf("collision inserting sync user (%s=%s): %w", field, value, err)
-				}
-			}
-			return nil, fmt.Errorf("error inserting sync user: %w", err)
+		if userSaved, err = scs.insertSyncUser(user, channel, rc); err != nil {
+			return nil, err
 		}
 	} else {
 		patch := &model.UserPatch{
+			Username:  &user.Username,
 			Nickname:  &user.Nickname,
 			FirstName: &user.FirstName,
 			LastName:  &user.LastName,
+			Email:     &user.Email,
 			Position:  &user.Position,
 			Locale:    &user.Locale,
 			Timezone:  user.Timezone,
 			RemoteId:  user.RemoteId,
 		}
-		euser.Patch(patch)
-		userUpdated, err := scs.server.GetStore().User().Update(euser, false)
-		if err != nil {
-			return nil, fmt.Errorf("error updating sync user: %w", err)
+		if userSaved, err = scs.updateSyncUser(patch, euser, channel, rc); err != nil {
+			return nil, err
 		}
-		userSaved = userUpdated.New
 	}
 
-	// add user to team. We do this here regardless of whether the user was
+	// Add user to team. We do this here regardless of whether the user was
 	// just created or patched since there are three steps to adding a user
 	// (insert rec, add to team, add to channel) and any one could fail.
 	// Instead of undoing what succeeded on any failure we simply do all steps each
-	// time. AddUserToChannel & AddUserToTeamByTeamId do not error if user already
+	// time. AddUserToChannel & AddUserToTeamByTeamId do not error if user was already
 	// added and exit quickly.
 	if err := scs.app.AddUserToTeamByTeamId(channel.TeamId, userSaved); err != nil {
 		return nil, fmt.Errorf("error adding sync user to Team: %w", err)
@@ -228,6 +221,97 @@ func (scs *Service) upsertSyncUser(user *model.User, channel *model.Channel, rc
 	return userSaved, nil
 }
 
+func (scs *Service) insertSyncUser(user *model.User, channel *model.Channel, rc *model.RemoteCluster) (*model.User, error) {
+	var err error
+	var userSaved *model.User
+	var suffix string
+
+	// save the originals in props (if not already done by another remote)
+	if _, ok := user.GetProp(KeyRemoteUsername); !ok {
+		user.SetProp(KeyRemoteUsername, user.Username)
+	}
+	if _, ok := user.GetProp(KeyRemoteEmail); !ok {
+		user.SetProp(KeyRemoteEmail, user.Email)
+	}
+
+	// Apply a suffix to the username until it is unique. Collisions will be quite
+	// rare since we are joining a username that is unique at a remote site with a unique
+	// name for that site. However we need to truncate the combined name to 64 chars and
+	// that might introduce a collision.
+	for i := 1; i <= MaxUpsertRetries; i++ {
+		if i > 1 {
+			suffix = strconv.FormatInt(int64(i), 10)
+		}
+
+		user.Username = mungUsername(user.Username, rc.Name, suffix, model.USER_NAME_MAX_LENGTH)
+		user.Email = mungEmail(rc.Name, model.USER_EMAIL_MAX_LENGTH)
+
+		if userSaved, err = scs.server.GetStore().User().Save(user); err != nil {
+			e, ok := err.(errInvalidInput)
+			if !ok {
+				break
+			}
+			_, field, value := e.InvalidInputInfo()
+			if field == "email" || field == "username" {
+				// username or email collision; try again with different suffix
+				scs.server.GetLogger().Log(mlog.LvlSharedChannelServiceWarn, "Collision inserting sync user",
+					mlog.String("field", field),
+					mlog.Any("value", value),
+					mlog.Int("attempt", i),
+					mlog.Err(err),
+				)
+			}
+		} else {
+			return userSaved, nil
+		}
+	}
+	return nil, fmt.Errorf("error inserting sync user %s: %w", user.Id, err)
+}
+
+func (scs *Service) updateSyncUser(patch *model.UserPatch, user *model.User, channel *model.Channel, rc *model.RemoteCluster) (*model.User, error) {
+	var err error
+	var update *model.UserUpdate
+	var suffix string
+
+	if patch.Username != nil {
+		user.SetProp(KeyRemoteUsername, *patch.Username)
+	}
+	if patch.Email != nil {
+		user.SetProp(KeyRemoteEmail, *patch.Email)
+	}
+
+	user.Patch(patch)
+
+	// Apply a suffix to the username until it is unique.
+	for i := 1; i <= MaxUpsertRetries; i++ {
+		if i > 1 {
+			suffix = strconv.FormatInt(int64(i), 10)
+		}
+		user.Username = mungUsername(user.Username, rc.Name, suffix, model.USER_NAME_MAX_LENGTH)
+		user.Email = mungEmail(rc.Name, model.USER_EMAIL_MAX_LENGTH)
+
+		if update, err = scs.server.GetStore().User().Update(user, false); err != nil {
+			e, ok := err.(errInvalidInput)
+			if !ok {
+				break
+			}
+			_, field, value := e.InvalidInputInfo()
+			if field == "email" || field == "username" {
+				// username or email collision; try again with different suffix
+				scs.server.GetLogger().Log(mlog.LvlSharedChannelServiceWarn, "Collision updating sync user",
+					mlog.String("field", field),
+					mlog.Any("value", value),
+					mlog.Int("attempt", i),
+					mlog.Err(err),
+				)
+			}
+		} else {
+			return update.New, nil
+		}
+	}
+	return nil, fmt.Errorf("error updating sync user %s: %w", user.Id, err)
+}
+
 func (scs *Service) upsertSyncPost(post *model.Post, channel *model.Channel, rc *model.RemoteCluster) (*model.Post, error) {
 	var appErr *model.AppError
 

services/sharedchannel/sync_send.go

@@ -315,7 +315,7 @@ func (scs *Service) updateForRemote(task syncTask, rc *model.RemoteCluster) erro
 		// All posts were filtered out, meaning no need to send them. Fast forward SharedChannelRemote's NextSyncAt.
 		scs.updateNextSyncForRemote(scr.Id, rc, nextSince)
 
-		// everything was filtered out, nothing to send.
+		// if there are more posts eligible to sync then schedule another sync
 		if repeat {
 			scs.addTask(newSyncTask(task.channelId, task.remoteId, nil))
 		}

services/sharedchannel/util.go

@@ -0,0 +1,66 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package sharedchannel
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/mattermost/mattermost-server/v5/model"
+)
+
+// mungUsername creates a new username by combining username and remote cluster name, plus
+// a suffix to create uniqueness. If the resulting username exceeds the max length then
+// it is truncated and ellipses added.
+func mungUsername(username string, remotename string, suffix string, maxLen int) string {
+	if suffix != "" {
+		suffix = "~" + suffix
+	}
+
+	// If the username already contains a colon then another server already munged it.
+	// In that case we can split on the colon and use the existing remote name.
+	// We still need to re-mung with suffix in case of collision.
+	comps := strings.Split(username, ":")
+	if len(comps) >= 2 {
+		username = comps[0]
+		remotename = strings.Join(comps[1:], "")
+	}
+
+	var userEllipses string
+	var remoteEllipses string
+
+	// The remotename is allowed to use up to half the maxLen, and the username gets the remaining space.
+	// Username might have a suffix to account for, and remotename always has a preceding colon.
+	half := maxLen / 2
+
+	// If the remotename is less than half the maxLen, then the left over space can be given to
+	// the username.
+	extra := half - (len(remotename) + 1)
+	if extra < 0 {
+		extra = 0
+	}
+
+	truncUser := (len(username) + len(suffix)) - (half + extra)
+	if truncUser > 0 {
+		username = username[:len(username)-truncUser-3]
+		userEllipses = "..."
+	}
+
+	truncRemote := (len(remotename) + 1) - (maxLen - (len(username) + len(userEllipses) + len(suffix)))
+	if truncRemote > 0 {
+		remotename = remotename[:len(remotename)-truncRemote-3]
+		remoteEllipses = "..."
+	}
+
+	return fmt.Sprintf("%s%s%s:%s%s", username, suffix, userEllipses, remotename, remoteEllipses)
+}
+
+// mungEmail creates a unique email address using a UID and remote name.
+func mungEmail(remotename string, maxLen int) string {
+	s := fmt.Sprintf("%s@%s", model.NewId(), remotename)
+	if len(s) > maxLen {
+		s = s[:maxLen]
+	}
+	return s
+}

services/sharedchannel/util_test.go

@@ -0,0 +1,76 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package sharedchannel
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func Test_mungUsername(t *testing.T) {
+	type args struct {
+		username   string
+		remotename string
+		suffix     string
+		maxLen     int
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{
+		{"everything empty", args{username: "", remotename: "", suffix: "", maxLen: 64}, ":"},
+
+		{"no trunc, no suffix", args{username: "bart", remotename: "example.com", suffix: "", maxLen: 64}, "bart:example.com"},
+		{"no trunc, suffix", args{username: "bart", remotename: "example.com", suffix: "2", maxLen: 64}, "bart~2:example.com"},
+
+		{"trunc remote, no suffix", args{username: "bart", remotename: "example1234567890.com", suffix: "", maxLen: 24}, "bart:example123456789..."},
+		{"trunc remote, suffix", args{username: "bart", remotename: "example1234567890.com", suffix: "2", maxLen: 24}, "bart~2:example1234567..."},
+
+		{"trunc both, no suffix", args{username: R(24, "A"), remotename: R(24, "B"), suffix: "", maxLen: 24}, "AAAAAAAAA...:BBBBBBBB..."},
+		{"trunc both, suffix", args{username: R(24, "A"), remotename: R(24, "B"), suffix: "10", maxLen: 24}, "AAAAAA~10...:BBBBBBBB..."},
+
+		{"trunc user, no suffix", args{username: R(40, "A"), remotename: "abc", suffix: "", maxLen: 24}, "AAAAAAAAAAAAAAAAA...:abc"},
+		{"trunc user, suffix", args{username: R(40, "A"), remotename: "abc", suffix: "11", maxLen: 24}, "AAAAAAAAAAAAAA~11...:abc"},
+
+		{"trunc user, remote, no suffix", args{username: R(40, "A"), remotename: "abcdefghijk", suffix: "", maxLen: 24}, "AAAAAAAAA...:abcdefghijk"},
+		{"trunc user, remote, suffix", args{username: R(40, "A"), remotename: "abcdefghijk", suffix: "19", maxLen: 24}, "AAAAAA~19...:abcdefghijk"},
+
+		{"short user, long remote, no suffix", args{username: "bart", remotename: R(40, "B"), suffix: "", maxLen: 24}, "bart:BBBBBBBBBBBBBBBB..."},
+		{"long user, short remote, no suffix", args{username: R(40, "A"), remotename: "abc.com", suffix: "", maxLen: 24}, "AAAAAAAAAAAAA...:abc.com"},
+
+		{"short user, long remote, suffix", args{username: "bart", remotename: R(40, "B"), suffix: "12", maxLen: 24}, "bart~12:BBBBBBBBBBBBB..."},
+		{"long user, short remote, suffix", args{username: R(40, "A"), remotename: "abc.com", suffix: "12", maxLen: 24}, "AAAAAAAAAA~12...:abc.com"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := mungUsername(tt.args.username, tt.args.remotename, tt.args.suffix, tt.args.maxLen); got != tt.want {
+				t.Errorf("mungUsername() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func Test_mungUsernameFuzz(t *testing.T) {
+	// ensure no index out of bounds panic for any combination
+	for i := 0; i < 70; i++ {
+		for j := 0; j < 70; j++ {
+			for k := 0; k < 3; k++ {
+				username := R(i, "A")
+				remotename := R(j, "B")
+				suffix := R(k, "1")
+
+				result := mungUsername(username, remotename, suffix, 64)
+				require.LessOrEqual(t, len(result), 64)
+			}
+		}
+	}
+}
+
+// R returns a string with the specified string repeated `count` times.
+func R(count int, s string) string {
+	return strings.Repeat(s, count)
+}

store/sqlstore/remote_cluster_store.go

@@ -22,9 +22,10 @@ func newSqlRemoteClusterStore(sqlStore *SqlStore) store.RemoteClusterStore {
 	s := &sqlRemoteClusterStore{sqlStore}
 
 	for _, db := range sqlStore.GetAllConns() {
-		table := db.AddTableWithName(model.RemoteCluster{}, "RemoteClusters").SetKeys(false, "RemoteId")
+		table := db.AddTableWithName(model.RemoteCluster{}, "RemoteClusters").SetKeys(false, "RemoteId", "Name")
 		table.ColMap("RemoteId").SetMaxSize(26)
 		table.ColMap("RemoteTeamId").SetMaxSize(26)
+		table.ColMap("Name").SetMaxSize(64)
 		table.ColMap("DisplayName").SetMaxSize(64)
 		table.ColMap("SiteURL").SetMaxSize(512)
 		table.ColMap("Token").SetMaxSize(26)

store/storetest/remote_cluster_store.go

@@ -29,9 +29,9 @@ func testRemoteClusterSave(t *testing.T, ss store.Store) {
 
 	t.Run("Save", func(t *testing.T) {
 		rc := &model.RemoteCluster{
-			DisplayName: "some remote",
-			SiteURL:     "somewhere.com",
-			CreatorId:   model.NewId(),
+			Name:      "some_remote",
+			SiteURL:   "somewhere.com",
+			CreatorId: model.NewId(),
 		}
 
 		rcSaved, err := ss.RemoteCluster().Save(rc)
@@ -53,8 +53,8 @@ func testRemoteClusterSave(t *testing.T, ss store.Store) {
 
 	t.Run("Save missing creator id", func(t *testing.T) {
 		rc := &model.RemoteCluster{
-			DisplayName: "some remote",
-			SiteURL:     "somewhere.com",
+			Name:    "some_remote 2",
+			SiteURL: "somewhere.com",
 		}
 		_, err := ss.RemoteCluster().Save(rc)
 		require.Error(t, err)
@@ -64,9 +64,9 @@ func testRemoteClusterSave(t *testing.T, ss store.Store) {
 func testRemoteClusterDelete(t *testing.T, ss store.Store) {
 	t.Run("Delete", func(t *testing.T) {
 		rc := &model.RemoteCluster{
-			DisplayName: "shortlived remote",
-			SiteURL:     "nowhere.com",
-			CreatorId:   model.NewId(),
+			Name:      "shortlived_remote",
+			SiteURL:   "nowhere.com",
+			CreatorId: model.NewId(),
 		}
 		rcSaved, err := ss.RemoteCluster().Save(rc)
 		require.NoError(t, err)
@@ -86,9 +86,9 @@ func testRemoteClusterDelete(t *testing.T, ss store.Store) {
 func testRemoteClusterGet(t *testing.T, ss store.Store) {
 	t.Run("Get", func(t *testing.T) {
 		rc := &model.RemoteCluster{
-			DisplayName: "shortlived remote",
-			SiteURL:     "nowhere.com",
-			CreatorId:   model.NewId(),
+			Name:      "shortlived_remote_2",
+			SiteURL:   "nowhere.com",
+			CreatorId: model.NewId(),
 		}
 		rcSaved, err := ss.RemoteCluster().Save(rc)
 		require.NoError(t, err)
@@ -112,11 +112,11 @@ func testRemoteClusterGetAll(t *testing.T, ss store.Store) {
 	pingLongAgo := model.GetMillis() - (model.RemoteOfflineAfterMillis * 3)
 
 	data := []*model.RemoteCluster{
-		{DisplayName: "offline remote", CreatorId: userId, SiteURL: "somewhere.com", LastPingAt: pingLongAgo, Topics: " shared incident "},
-		{DisplayName: "some online remote", CreatorId: userId, SiteURL: "nowhere.com", LastPingAt: now, Topics: " shared incident "},
-		{DisplayName: "another online remote", CreatorId: model.NewId(), SiteURL: "underwhere.com", LastPingAt: now, Topics: ""},
-		{DisplayName: "another offline remote", CreatorId: model.NewId(), SiteURL: "knowhere.com", LastPingAt: pingLongAgo, Topics: " shared "},
-		{DisplayName: "brand new offline remote", CreatorId: userId, SiteURL: "", LastPingAt: 0, Topics: " bogus shared stuff "},
+		{Name: "offline_remote", CreatorId: userId, SiteURL: "somewhere.com", LastPingAt: pingLongAgo, Topics: " shared incident "},
+		{Name: "some_online_remote", CreatorId: userId, SiteURL: "nowhere.com", LastPingAt: now, Topics: " shared incident "},
+		{Name: "another_online_remote", CreatorId: model.NewId(), SiteURL: "underwhere.com", LastPingAt: now, Topics: ""},
+		{Name: "another_offline_remote", CreatorId: model.NewId(), SiteURL: "knowhere.com", LastPingAt: pingLongAgo, Topics: " shared "},
+		{Name: "brand_new_offline_remote", CreatorId: userId, SiteURL: "", LastPingAt: 0, Topics: " bogus shared stuff "},
 	}
 
 	idsAll := make([]string, 0)
@@ -240,11 +240,11 @@ func testRemoteClusterGetAllInChannel(t *testing.T, ss store.Store) {
 
 	// Create some remote clusters
 	rcData := []*model.RemoteCluster{
-		{DisplayName: "AAAA Inc", CreatorId: userId, SiteURL: "aaaa.com", RemoteId: model.NewId(), LastPingAt: now},
-		{DisplayName: "BBBB Inc", CreatorId: userId, SiteURL: "bbbb.com", RemoteId: model.NewId(), LastPingAt: 0},
-		{DisplayName: "CCCC Inc", CreatorId: userId, SiteURL: "cccc.com", RemoteId: model.NewId(), LastPingAt: now},
-		{DisplayName: "DDDD Inc", CreatorId: userId, SiteURL: "dddd.com", RemoteId: model.NewId(), LastPingAt: now},
-		{DisplayName: "EEEE Inc", CreatorId: userId, SiteURL: "eeee.com", RemoteId: model.NewId(), LastPingAt: 0},
+		{Name: "AAAA_Inc", CreatorId: userId, SiteURL: "aaaa.com", RemoteId: model.NewId(), LastPingAt: now},
+		{Name: "BBBB_Inc", CreatorId: userId, SiteURL: "bbbb.com", RemoteId: model.NewId(), LastPingAt: 0},
+		{Name: "CCCC_Inc", CreatorId: userId, SiteURL: "cccc.com", RemoteId: model.NewId(), LastPingAt: now},
+		{Name: "DDDD_Inc", CreatorId: userId, SiteURL: "dddd.com", RemoteId: model.NewId(), LastPingAt: now},
+		{Name: "EEEE_Inc", CreatorId: userId, SiteURL: "eeee.com", RemoteId: model.NewId(), LastPingAt: 0},
 	}
 	for _, item := range rcData {
 		_, err := ss.RemoteCluster().Save(item)
@@ -347,11 +347,11 @@ func testRemoteClusterGetAllNotInChannel(t *testing.T, ss store.Store) {
 
 	// Create some remote clusters
 	rcData := []*model.RemoteCluster{
-		{DisplayName: "AAAA Inc", CreatorId: userId, SiteURL: "aaaa.com", RemoteId: model.NewId()},
-		{DisplayName: "BBBB Inc", CreatorId: userId, SiteURL: "bbbb.com", RemoteId: model.NewId()},
-		{DisplayName: "CCCC Inc", CreatorId: userId, SiteURL: "cccc.com", RemoteId: model.NewId()},
-		{DisplayName: "DDDD Inc", CreatorId: userId, SiteURL: "dddd.com", RemoteId: model.NewId()},
-		{DisplayName: "EEEE Inc", CreatorId: userId, SiteURL: "eeee.com", RemoteId: model.NewId()},
+		{Name: "AAAA_Inc", CreatorId: userId, SiteURL: "aaaa.com", RemoteId: model.NewId()},
+		{Name: "BBBB_Inc", CreatorId: userId, SiteURL: "bbbb.com", RemoteId: model.NewId()},
+		{Name: "CCCC_Inc", CreatorId: userId, SiteURL: "cccc.com", RemoteId: model.NewId()},
+		{Name: "DDDD_Inc", CreatorId: userId, SiteURL: "dddd.com", RemoteId: model.NewId()},
+		{Name: "EEEE_Inc", CreatorId: userId, SiteURL: "eeee.com", RemoteId: model.NewId()},
 	}
 	for _, item := range rcData {
 		_, err := ss.RemoteCluster().Save(item)
@@ -429,13 +429,13 @@ func testRemoteClusterGetByTopic(t *testing.T, ss store.Store) {
 	require.NoError(t, clearRemoteClusters(ss))
 
 	rcData := []*model.RemoteCluster{
-		{DisplayName: "AAAA Inc", CreatorId: model.NewId(), SiteURL: "aaaa.com", RemoteId: model.NewId(), Topics: ""},
-		{DisplayName: "BBBB Inc", CreatorId: model.NewId(), SiteURL: "bbbb.com", RemoteId: model.NewId(), Topics: " share "},
-		{DisplayName: "CCCC Inc", CreatorId: model.NewId(), SiteURL: "cccc.com", RemoteId: model.NewId(), Topics: " incident share "},
-		{DisplayName: "DDDD Inc", CreatorId: model.NewId(), SiteURL: "dddd.com", RemoteId: model.NewId(), Topics: " bogus "},
-		{DisplayName: "EEEE Inc", CreatorId: model.NewId(), SiteURL: "eeee.com", RemoteId: model.NewId(), Topics: " logs share incident "},
-		{DisplayName: "FFFF Inc", CreatorId: model.NewId(), SiteURL: "ffff.com", RemoteId: model.NewId(), Topics: " bogus incident "},
-		{DisplayName: "GGGG Inc", CreatorId: model.NewId(), SiteURL: "gggg.com", RemoteId: model.NewId(), Topics: "*"},
+		{Name: "AAAA_Inc", CreatorId: model.NewId(), SiteURL: "aaaa.com", RemoteId: model.NewId(), Topics: ""},
+		{Name: "BBBB_Inc", CreatorId: model.NewId(), SiteURL: "bbbb.com", RemoteId: model.NewId(), Topics: " share "},
+		{Name: "CCCC_Inc", CreatorId: model.NewId(), SiteURL: "cccc.com", RemoteId: model.NewId(), Topics: " incident share "},
+		{Name: "DDDD_Inc", CreatorId: model.NewId(), SiteURL: "dddd.com", RemoteId: model.NewId(), Topics: " bogus "},
+		{Name: "EEEE_Inc", CreatorId: model.NewId(), SiteURL: "eeee.com", RemoteId: model.NewId(), Topics: " logs share incident "},
+		{Name: "FFFF_Inc", CreatorId: model.NewId(), SiteURL: "ffff.com", RemoteId: model.NewId(), Topics: " bogus incident "},
+		{Name: "GGGG_Inc", CreatorId: model.NewId(), SiteURL: "gggg.com", RemoteId: model.NewId(), Topics: "*"},
 	}
 	for _, item := range rcData {
 		_, err := ss.RemoteCluster().Save(item)
@@ -474,6 +474,7 @@ func testRemoteClusterUpdateTopics(t *testing.T, ss store.Store) {
 	remoteId := model.NewId()
 	rc := &model.RemoteCluster{
 		DisplayName: "Blap Inc",
+		Name:        "blap",
 		SiteURL:     "blap.com",
 		RemoteId:    remoteId,
 		Topics:      "",

store/storetest/shared_channel_store.go

@@ -674,9 +674,9 @@ func testGetRemoteForUser(t *testing.T, ss store.Store) {
 	channel, err := createSharedTestChannel(ss, "share_test_channel", true)
 	require.NoError(t, err)
 	remotes := []*model.RemoteCluster{
-		{RemoteId: model.NewId(), SiteURL: model.NewId(), CreatorId: model.NewId(), RemoteTeamId: teamId, DisplayName: "Test Remote 1"},
-		{RemoteId: model.NewId(), SiteURL: model.NewId(), CreatorId: model.NewId(), RemoteTeamId: teamId, DisplayName: "Test Remote 2"},
-		{RemoteId: model.NewId(), SiteURL: model.NewId(), CreatorId: model.NewId(), RemoteTeamId: teamId, DisplayName: "Test Remote 3"},
+		{RemoteId: model.NewId(), SiteURL: model.NewId(), CreatorId: model.NewId(), RemoteTeamId: teamId, Name: "Test_Remote_1"},
+		{RemoteId: model.NewId(), SiteURL: model.NewId(), CreatorId: model.NewId(), RemoteTeamId: teamId, Name: "Test_Remote_2"},
+		{RemoteId: model.NewId(), SiteURL: model.NewId(), CreatorId: model.NewId(), RemoteTeamId: teamId, Name: "Test_Remote_3"},
 	}
 	var channelRemotes []*model.SharedChannelRemote
 	for _, rc := range remotes {