diff --git a/api4/shared_channel_test.go b/api4/shared_channel_test.go index f6d60b54f4..36d48f8368 100644 --- a/api4/shared_channel_test.go +++ b/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) diff --git a/app/remote_cluster_service_mock.go b/app/remote_cluster_service_mock.go index 0e85e2d876..4a42e64a15 100644 --- a/app/remote_cluster_service_mock.go +++ b/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 } diff --git a/app/remote_cluster_test.go b/app/remote_cluster_test.go index 4cf49745e0..69d99b00a1 100644 --- a/app/remote_cluster_test.go +++ b/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", diff --git a/app/session_test.go b/app/session_test.go index 0e6f24d8af..a7673911a2 100644 --- a/app/session_test.go +++ b/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(), } diff --git a/app/slashcommands/command_remote.go b/app/slashcommands/command_remote.go index d0b0976fd7..a8d375708d 100644 --- a/app/slashcommands/command_remote.go +++ b/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()) } diff --git a/i18n/en.json b/i18n/en.json index 2b3deed2fb..541dc895b5 100644 --- a/i18n/en.json +++ b/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", diff --git a/model/at_mentions.go b/model/at_mentions.go index f41d182ad3..b5bfc0c70c 100644 --- a/model/at_mentions.go +++ b/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 } diff --git a/model/auditconv.go b/model/auditconv.go index 195ad11e49..b3cf60629a 100644 --- a/model/auditconv.go +++ b/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) diff --git a/model/config.go b/model/config.go index ea1d5a59dc..464749ef1a 100644 --- a/model/config.go +++ b/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" ) diff --git a/model/remote_cluster.go b/model/remote_cluster.go index 32b2a46278..6ae0aa3349 100644 --- a/model/remote_cluster.go +++ b/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"` diff --git a/model/remote_cluster_test.go b/model/remote_cluster_test.go index 999557b533..7a752920ac 100644 --- a/model/remote_cluster_test.go +++ b/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) diff --git a/model/user.go b/model/user.go index 27b88108d2..73d506a24f 100644 --- a/model/user.go +++ b/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 { diff --git a/model/user_test.go b/model/user_test.go index e8870a7580..fc5f35dcb7 100644 --- a/model/user_test.go +++ b/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) { diff --git a/services/remotecluster/invitation.go b/services/remotecluster/invitation.go index 744928ae8e..4f4aecd241 100644 --- a/services/remotecluster/invitation.go +++ b/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, diff --git a/services/remotecluster/send_test.go b/services/remotecluster/send_test.go index e001c7615f..27b9b20e1e 100644 --- a/services/remotecluster/send_test.go +++ b/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(), } } diff --git a/services/remotecluster/sendmsg.go b/services/remotecluster/sendmsg.go index 7e6f5067d7..ea7dda9137 100644 --- a/services/remotecluster/sendmsg.go +++ b/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 { diff --git a/services/remotecluster/service.go b/services/remotecluster/service.go index 9824344d48..981886bd69 100644 --- a/services/remotecluster/service.go +++ b/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 } diff --git a/services/sharedchannel/channelinvite_test.go b/services/sharedchannel/channelinvite_test.go index a2929744e9..38d2bddcdc 100644 --- a/services/sharedchannel/channelinvite_test.go +++ b/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(), diff --git a/services/sharedchannel/mock_AppIface_test.go b/services/sharedchannel/mock_AppIface_test.go index 96e96c81e6..00d98242a5 100644 --- a/services/sharedchannel/mock_AppIface_test.go +++ b/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) diff --git a/services/sharedchannel/msg.go b/services/sharedchannel/msg.go index 09917d4e3b..7e59e5bbf8 100644 --- a/services/sharedchannel/msg.go +++ b/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 diff --git a/services/sharedchannel/service.go b/services/sharedchannel/service.go index ebf7dfcae2..cc5ea8b5e7 100644 --- a/services/sharedchannel/service.go +++ b/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. diff --git a/services/sharedchannel/sync_recv.go b/services/sharedchannel/sync_recv.go index b1007cb8f9..ff25a15a99 100644 --- a/services/sharedchannel/sync_recv.go +++ b/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 diff --git a/services/sharedchannel/sync_send.go b/services/sharedchannel/sync_send.go index 251d9f8d89..075f45b4b6 100644 --- a/services/sharedchannel/sync_send.go +++ b/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)) } diff --git a/services/sharedchannel/util.go b/services/sharedchannel/util.go new file mode 100644 index 0000000000..93d019d900 --- /dev/null +++ b/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 +} diff --git a/services/sharedchannel/util_test.go b/services/sharedchannel/util_test.go new file mode 100644 index 0000000000..0c5ec5a540 --- /dev/null +++ b/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) +} diff --git a/store/sqlstore/remote_cluster_store.go b/store/sqlstore/remote_cluster_store.go index 62bba253af..ea7acf98cd 100644 --- a/store/sqlstore/remote_cluster_store.go +++ b/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) diff --git a/store/storetest/remote_cluster_store.go b/store/storetest/remote_cluster_store.go index 9792bf61fd..8d1cd3b946 100644 --- a/store/storetest/remote_cluster_store.go +++ b/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: "", diff --git a/store/storetest/shared_channel_store.go b/store/storetest/shared_channel_store.go index 8b9445905a..898cc2475b 100644 --- a/store/storetest/shared_channel_store.go +++ b/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 {