diff --git a/server/channels/api4/channel.go b/server/channels/api4/channel.go index 0684d62c6ec6..445b46ae1faa 100644 --- a/server/channels/api4/channel.go +++ b/server/channels/api4/channel.go @@ -2910,7 +2910,11 @@ func getChannelAccessControlAttributes(c *Context, w http.ResponseWriter, r *htt return } - attributes, err := c.App.GetAccessControlPolicyAttributes(c.AppContext, c.Params.ChannelId, "*") + // Channel banners care about the membership rule — the attributes that + // determine who can be in the channel. Since the v0.3 migration stores the + // action as "membership" rather than "*", ask for it explicitly; the + // wildcard fallback in GetRule still covers older policies that kept "*". + attributes, err := c.App.GetAccessControlPolicyAttributes(c.AppContext, c.Params.ChannelId, model.AccessControlPolicyActionMembership) if err != nil { c.Err = err return diff --git a/server/channels/api4/remote_cluster.go b/server/channels/api4/remote_cluster.go index 7f615257f37d..01aba674274c 100644 --- a/server/channels/api4/remote_cluster.go +++ b/server/channels/api4/remote_cluster.go @@ -660,6 +660,8 @@ func patchRemoteCluster(c *Context, w http.ResponseWriter, r *http.Request) { return } + updatedRC.Sanitize() + auditRec.Success() auditRec.AddEventResultState(updatedRC) diff --git a/server/channels/api4/remote_cluster_test.go b/server/channels/api4/remote_cluster_test.go index 4924970fe576..c39e72175b30 100644 --- a/server/channels/api4/remote_cluster_test.go +++ b/server/channels/api4/remote_cluster_test.go @@ -545,9 +545,10 @@ func TestGenerateRemoteClusterInvite(t *testing.T) { func TestGetRemoteCluster(t *testing.T) { mainHelper.Parallel(t) newRC := &model.RemoteCluster{ - Name: "remotecluster", - SiteURL: "http://example.com", - Token: model.NewId(), + Name: "remotecluster", + SiteURL: "http://example.com", + Token: model.NewId(), + RemoteToken: model.NewId(), } t.Run("Should not work if the remote cluster service is not enabled", func(t *testing.T) { @@ -596,6 +597,7 @@ func TestGetRemoteCluster(t *testing.T) { require.Equal(t, rc.RemoteId, fetchedRC.RemoteId) require.Equal(t, th.BasicTeam.Id, fetchedRC.DefaultTeamId) require.Empty(t, fetchedRC.Token) + require.Empty(t, fetchedRC.RemoteToken) }) } @@ -646,6 +648,7 @@ func TestPatchRemoteCluster(t *testing.T) { DisplayName: "initialvalue", SiteURL: "http://example.com", Token: model.NewId(), + RemoteToken: model.NewId(), } rcp := &model.RemoteClusterPatch{DisplayName: model.NewPointer("different value")} @@ -699,6 +702,8 @@ func TestPatchRemoteCluster(t *testing.T) { require.NoError(t, err) require.Equal(t, "patched!", patchedRC.DisplayName) require.Equal(t, newTeamId, patchedRC.DefaultTeamId) + require.Empty(t, patchedRC.Token) + require.Empty(t, patchedRC.RemoteToken) }) } diff --git a/server/channels/app/bot.go b/server/channels/app/bot.go index a149801859d9..6cc77c790b09 100644 --- a/server/channels/app/bot.go +++ b/server/channels/app/bot.go @@ -65,18 +65,19 @@ func (a *App) EnsureBot(rctx request.CTX, pluginID string, bot *model.Bot) (stri if appErr := a.SetPluginKey(pluginID, botUserKey, []byte(user.Id)); appErr != nil { return "", fmt.Errorf("failed to set plugin key: %w", appErr) } - } else { - rctx.Logger().Error("Plugin attempted to use an account that already exists. Convert user to a bot "+ - "account in the CLI by running 'mattermost user convert --bot'. If the user is an "+ - "existing user account you want to preserve, change its username and restart the Mattermost server, "+ - "after which the plugin will create a bot account with that name. For more information about bot "+ - "accounts, see https://mattermost.com/pl/default-bot-accounts", mlog.String("username", - bot.Username), - mlog.String("user_id", - user.Id), - ) - } - return user.Id, nil + return user.Id, nil + } + + rctx.Logger().Error("Plugin attempted to use an account that already exists. Convert user to a bot "+ + "account in the CLI by running 'mattermost user convert --bot'. If the user is an "+ + "existing user account you want to preserve, change its username and restart the Mattermost server, "+ + "after which the plugin will create a bot account with that name. For more information about bot "+ + "accounts, see https://mattermost.com/pl/default-bot-accounts", mlog.String("username", + bot.Username), + mlog.String("user_id", + user.Id), + ) + return "", fmt.Errorf("username %q is already taken by a non-bot user", bot.Username) } createdBot, err := a.CreateBot(rctx, bot) diff --git a/server/channels/app/bot_test.go b/server/channels/app/bot_test.go index ad0f328ceadc..a6885a6231ae 100644 --- a/server/channels/app/bot_test.go +++ b/server/channels/app/bot_test.go @@ -147,6 +147,22 @@ func TestEnsureBot(t *testing.T) { assert.Equal(t, "another bot", bot.Description) }) + t.Run("ensure bot should fail if username belongs to a non-bot user", func(t *testing.T) { + th := Setup(t).InitBasic(t) + + pluginId := "pluginId" + + // th.BasicUser is a regular (non-bot) user created by InitBasic. + // EnsureBot must return an error — not the human user's ID. + botID, err := th.App.EnsureBot(th.Context, pluginId, &model.Bot{ + Username: th.BasicUser.Username, + Description: "a bot", + OwnerId: th.BasicUser.Id, + }) + require.Error(t, err) + assert.Empty(t, botID) + }) + t.Run("ensure bot should pass even after delete bot user", func(t *testing.T) { th := Setup(t).InitBasic(t) diff --git a/server/channels/app/platform/support_packet.go b/server/channels/app/platform/support_packet.go index 30f5b6659b68..43c8720f33c5 100644 --- a/server/channels/app/platform/support_packet.go +++ b/server/channels/app/platform/support_packet.go @@ -5,7 +5,11 @@ package platform import ( "bytes" + "context" "encoding/json" + "fmt" + "net/http" + "net/url" "os" "runtime" rpprof "runtime/pprof" @@ -19,6 +23,8 @@ import ( "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/shared/mlog" "github.com/mattermost/mattermost/server/public/shared/request" + "github.com/mattermost/mattermost/server/v8/channels/utils" + "github.com/mattermost/mattermost/server/v8/platform/shared/mail" ) const ( @@ -221,6 +227,8 @@ func (ps *PlatformService) getSupportPacketDiagnostics(rctx request.CTX) (*model } d.LDAP.ServerName = severName d.LDAP.ServerVersion = serverVersion + } else { + d.LDAP.Status = model.StatusDisabled } /* SAML */ @@ -231,14 +239,64 @@ func (ps *PlatformService) getSupportPacketDiagnostics(rctx request.CTX) (*model /* Elastic Search */ if se := ps.SearchEngine.ElasticsearchEngine; se != nil { d.ElasticSearch.Backend = *ps.Config().ElasticsearchSettings.Backend + d.ElasticSearch.ServerVersion = se.GetFullVersion() + d.ElasticSearch.ServerPlugins = se.GetPlugins() if *ps.Config().ElasticsearchSettings.EnableIndexing { appErr := se.TestConfig(rctx, ps.Config()) if appErr != nil { + d.ElasticSearch.Status = model.StatusFail d.ElasticSearch.Error = appErr.Error() + } else { + d.ElasticSearch.Status = model.StatusOk } + } else { + d.ElasticSearch.Status = model.StatusDisabled } - d.ElasticSearch.ServerVersion = se.GetFullVersion() - d.ElasticSearch.ServerPlugins = se.GetPlugins() + } else { + d.ElasticSearch.Status = model.StatusDisabled + } + + /* Email Notifications */ + if model.SafeDereference(ps.Config().EmailSettings.SendEmailNotifications) { + emailSettings := ps.Config().EmailSettings + hostname := utils.GetHostnameFromSiteURL(model.SafeDereference(ps.Config().ServiceSettings.SiteURL)) + mailCfg := &mail.SMTPConfig{ + Hostname: hostname, + ConnectionSecurity: model.SafeDereference(emailSettings.ConnectionSecurity), + SkipServerCertificateVerification: model.SafeDereference(emailSettings.SkipServerCertificateVerification), + ServerName: model.SafeDereference(emailSettings.SMTPServer), + Server: model.SafeDereference(emailSettings.SMTPServer), + Port: model.SafeDereference(emailSettings.SMTPPort), + ServerTimeout: model.SafeDereference(emailSettings.SMTPServerTimeout), + Username: model.SafeDereference(emailSettings.SMTPUsername), + Password: model.SafeDereference(emailSettings.SMTPPassword), + EnableSMTPAuth: model.SafeDereference(emailSettings.EnableSMTPAuth), + SendEmailNotifications: true, + FeedbackName: model.SafeDereference(emailSettings.FeedbackName), + FeedbackEmail: model.SafeDereference(emailSettings.FeedbackEmail), + ReplyToAddress: model.SafeDereference(emailSettings.ReplyToAddress), + } + if smtpErr := mail.TestConnection(mailCfg); smtpErr != nil { + d.Notifications.Email.Status = model.StatusFail + d.Notifications.Email.Error = smtpErr.Error() + } else { + d.Notifications.Email.Status = model.StatusOk + } + } else { + d.Notifications.Email.Status = model.StatusDisabled + } + + /* Push Notifications */ + if model.SafeDereference(ps.Config().EmailSettings.SendPushNotifications) { + pushServerURL := model.SafeDereference(ps.Config().EmailSettings.PushNotificationServer) + if pushErr := testPushProxyConnection(rctx.Context(), pushServerURL); pushErr != nil { + d.Notifications.Push.Status = model.StatusFail + d.Notifications.Push.Error = pushErr.Error() + } else { + d.Notifications.Push.Status = model.StatusOk + } + } else { + d.Notifications.Push.Status = model.StatusDisabled } b, err := yaml.Marshal(&d) @@ -253,6 +311,29 @@ func (ps *PlatformService) getSupportPacketDiagnostics(rctx request.CTX) (*model return fileData, rErr.ErrorOrNil() } +// TODO: move this into its own push proxy package once one exists (see also pushNotificationClient in server.go) +func testPushProxyConnection(ctx context.Context, serverURL string) error { + ctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + versionURL, err := url.JoinPath(serverURL, "version") + if err != nil { + return err + } + req, err := http.NewRequestWithContext(ctx, http.MethodGet, versionURL, nil) + if err != nil { + return err + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + return err + } + resp.Body.Close() + if resp.StatusCode >= http.StatusBadRequest { + return fmt.Errorf("push proxy returned unexpected status %d", resp.StatusCode) + } + return nil +} + func (ps *PlatformService) getSanitizedConfigFile(rctx request.CTX) (*model.FileData, error) { config := ps.getSanitizedConfig(rctx, &model.SanitizeOptions{PartiallyRedactDataSources: true}) spConfig := model.SupportPacketConfig{ diff --git a/server/channels/app/platform/support_packet_test.go b/server/channels/app/platform/support_packet_test.go index b8181656ece2..570a5cc7a582 100644 --- a/server/channels/app/platform/support_packet_test.go +++ b/server/channels/app/platform/support_packet_test.go @@ -4,12 +4,18 @@ package platform import ( + "bufio" "bytes" "encoding/json" "errors" + "net" + "net/http" + "net/http/httptest" "os" "path" "runtime" + "strconv" + "strings" "testing" "github.com/goccy/go-yaml" @@ -255,7 +261,7 @@ func TestGetSupportPacketDiagnostics(t *testing.T) { assert.Zero(t, d.Cluster.NumberOfNodes) /* LDAP */ - assert.Empty(t, d.LDAP.Status) + assert.Equal(t, model.StatusDisabled, d.LDAP.Status) assert.Empty(t, d.LDAP.Error) assert.Empty(t, d.LDAP.ServerName) assert.Empty(t, d.LDAP.ServerVersion) @@ -264,6 +270,7 @@ func TestGetSupportPacketDiagnostics(t *testing.T) { assert.Empty(t, d.SAML.ProviderType) /* Elastic Search */ + assert.Equal(t, model.StatusDisabled, d.ElasticSearch.Status) assert.Empty(t, d.ElasticSearch.ServerVersion) assert.Empty(t, d.ElasticSearch.ServerPlugins) }) @@ -314,6 +321,7 @@ func TestGetSupportPacketDiagnostics(t *testing.T) { packet := getDiagnostics(t) + assert.Equal(t, model.StatusDisabled, packet.LDAP.Status) assert.Equal(t, "", packet.LDAP.ServerName) assert.Equal(t, "", packet.LDAP.ServerVersion) }) @@ -475,6 +483,7 @@ func TestGetSupportPacketDiagnostics(t *testing.T) { packet := getDiagnostics(t) + assert.Equal(t, model.StatusDisabled, packet.ElasticSearch.Status) assert.Equal(t, model.ElasticsearchSettingsESBackend, packet.ElasticSearch.Backend) assert.Equal(t, "7.10.0", packet.ElasticSearch.ServerVersion) assert.Equal(t, []string{"plugin1", "plugin2"}, packet.ElasticSearch.ServerPlugins) @@ -499,6 +508,7 @@ func TestGetSupportPacketDiagnostics(t *testing.T) { packet := getDiagnostics(t) + assert.Equal(t, model.StatusOk, packet.ElasticSearch.Status) assert.Equal(t, model.ElasticsearchSettingsOSBackend, packet.ElasticSearch.Backend) assert.Equal(t, "2.5.0", packet.ElasticSearch.ServerVersion) assert.Equal(t, []string{"opensearch-plugin"}, packet.ElasticSearch.ServerPlugins) @@ -524,11 +534,170 @@ func TestGetSupportPacketDiagnostics(t *testing.T) { packet := getDiagnostics(t) + assert.Equal(t, model.StatusFail, packet.ElasticSearch.Status) assert.Equal(t, model.ElasticsearchSettingsESBackend, packet.ElasticSearch.Backend) assert.Equal(t, "7.10.0", packet.ElasticSearch.ServerVersion) assert.Equal(t, []string{"plugin1", "plugin2"}, packet.ElasticSearch.ServerPlugins) assert.Equal(t, "TestConfig: ent.elasticsearch.test_config.connection_failed, connection refused", packet.ElasticSearch.Error) }) + + t.Run("push notifications disabled", func(t *testing.T) { + th.Service.UpdateConfig(func(cfg *model.Config) { + cfg.EmailSettings.SendPushNotifications = model.NewPointer(false) + }) + t.Cleanup(func() { + th.Service.UpdateConfig(func(cfg *model.Config) { + cfg.EmailSettings.SendPushNotifications = model.NewPointer(true) + }) + }) + + packet := getDiagnostics(t) + + assert.Equal(t, model.StatusDisabled, packet.Notifications.Push.Status) + assert.Empty(t, packet.Notifications.Push.Error) + }) + + t.Run("push notifications reachable", func(t *testing.T) { + pushServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/version", r.URL.Path) + w.WriteHeader(http.StatusOK) + })) + defer pushServer.Close() + + th.Service.UpdateConfig(func(cfg *model.Config) { + cfg.EmailSettings.SendPushNotifications = model.NewPointer(true) + cfg.EmailSettings.PushNotificationServer = model.NewPointer(pushServer.URL) + }) + t.Cleanup(func() { + th.Service.UpdateConfig(func(cfg *model.Config) { + cfg.EmailSettings.SendPushNotifications = model.NewPointer(true) + cfg.EmailSettings.PushNotificationServer = model.NewPointer(model.GenericNotificationServer) + }) + }) + + packet := getDiagnostics(t) + + assert.Equal(t, model.StatusOk, packet.Notifications.Push.Status) + assert.Empty(t, packet.Notifications.Push.Error) + }) + + t.Run("push notifications unreachable", func(t *testing.T) { + th.Service.UpdateConfig(func(cfg *model.Config) { + cfg.EmailSettings.SendPushNotifications = model.NewPointer(true) + cfg.EmailSettings.PushNotificationServer = model.NewPointer("http://localhost:1") + }) + t.Cleanup(func() { + th.Service.UpdateConfig(func(cfg *model.Config) { + cfg.EmailSettings.SendPushNotifications = model.NewPointer(true) + cfg.EmailSettings.PushNotificationServer = model.NewPointer(model.GenericNotificationServer) + }) + }) + + packet := getDiagnostics(t) + + assert.Equal(t, model.StatusFail, packet.Notifications.Push.Status) + assert.NotEmpty(t, packet.Notifications.Push.Error) + }) + + t.Run("email notifications disabled", func(t *testing.T) { + th.Service.UpdateConfig(func(cfg *model.Config) { + cfg.EmailSettings.SendEmailNotifications = model.NewPointer(false) + }) + t.Cleanup(func() { + th.Service.UpdateConfig(func(cfg *model.Config) { + cfg.EmailSettings.SendEmailNotifications = model.NewPointer(true) + }) + }) + + packet := getDiagnostics(t) + + assert.Equal(t, model.StatusDisabled, packet.Notifications.Email.Status) + assert.Empty(t, packet.Notifications.Email.Error) + }) + + t.Run("email notifications reachable", func(t *testing.T) { + l, listenErr := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, listenErr) + defer l.Close() + + go func() { + for { + conn, err := l.Accept() + if err != nil { + return + } + go func(c net.Conn) { + defer c.Close() + rw := bufio.NewReadWriter(bufio.NewReader(c), bufio.NewWriter(c)) + _, _ = rw.WriteString("220 localhost ESMTP Test\r\n") + rw.Flush() + for { + line, err := rw.ReadString('\n') + if err != nil { + return + } + if strings.HasPrefix(strings.ToUpper(strings.TrimSpace(line)), "QUIT") { + _, _ = rw.WriteString("221 Bye\r\n") + rw.Flush() + return + } + _, _ = rw.WriteString("250 OK\r\n") + rw.Flush() + } + }(conn) + } + }() + + tcpAddr := l.Addr().(*net.TCPAddr) + smtpPort := strconv.Itoa(tcpAddr.Port) + + // MM_EMAILSETTINGS_SMTPSERVER may be set in CI and would override UpdateConfig. + // Use t.Setenv so the env var is updated before UpdateConfig calls Store.Set(), + // which re-reads GetEnvironment() (os.Environ()) and applies overrides. + t.Setenv("MM_EMAILSETTINGS_SMTPSERVER", "127.0.0.1") + + th.Service.UpdateConfig(func(cfg *model.Config) { + cfg.EmailSettings.SendEmailNotifications = model.NewPointer(true) + cfg.EmailSettings.SMTPServer = model.NewPointer("127.0.0.1") + cfg.EmailSettings.SMTPPort = model.NewPointer(smtpPort) + cfg.EmailSettings.EnableSMTPAuth = model.NewPointer(false) + cfg.EmailSettings.ConnectionSecurity = model.NewPointer("") + }) + t.Cleanup(func() { + th.Service.UpdateConfig(func(cfg *model.Config) { + cfg.EmailSettings.SendEmailNotifications = model.NewPointer(true) + cfg.EmailSettings.SMTPServer = model.NewPointer(model.EmailSMTPDefaultServer) + cfg.EmailSettings.SMTPPort = model.NewPointer(model.EmailSMTPDefaultPort) + }) + }) + + packet := getDiagnostics(t) + + assert.Equal(t, model.StatusOk, packet.Notifications.Email.Status) + assert.Empty(t, packet.Notifications.Email.Error) + }) + + t.Run("email notifications unreachable", func(t *testing.T) { + th.Service.UpdateConfig(func(cfg *model.Config) { + cfg.EmailSettings.SendEmailNotifications = model.NewPointer(true) + cfg.EmailSettings.SMTPServer = model.NewPointer("localhost") + cfg.EmailSettings.SMTPPort = model.NewPointer("1") + cfg.EmailSettings.SMTPServerTimeout = model.NewPointer(1) + }) + t.Cleanup(func() { + th.Service.UpdateConfig(func(cfg *model.Config) { + cfg.EmailSettings.SendEmailNotifications = model.NewPointer(true) + cfg.EmailSettings.SMTPServer = model.NewPointer(model.EmailSMTPDefaultServer) + cfg.EmailSettings.SMTPPort = model.NewPointer(model.EmailSMTPDefaultPort) + cfg.EmailSettings.SMTPServerTimeout = model.NewPointer(10) + }) + }) + + packet := getDiagnostics(t) + + assert.Equal(t, model.StatusFail, packet.Notifications.Email.Status) + assert.NotEmpty(t, packet.Notifications.Email.Error) + }) } func TestGetSanitizedConfigFile(t *testing.T) { diff --git a/server/channels/app/remote_cluster.go b/server/channels/app/remote_cluster.go index 43d441d0c8cb..4f7b144b2fe2 100644 --- a/server/channels/app/remote_cluster.go +++ b/server/channels/app/remote_cluster.go @@ -62,9 +62,11 @@ func (a *App) RegisterPluginForSharedChannels(rctx request.CTX, opts model.Regis return rc.RemoteId, nil } - // New connection. + // New connection. Name must satisfy the slug-style regex enforced by + // RemoteCluster.IsValid, so derive it from Displayname; the original + // human-readable label is preserved on DisplayName. rc = &model.RemoteCluster{ - Name: opts.Displayname, + Name: model.CleanRemoteName(opts.Displayname), DisplayName: opts.Displayname, SiteURL: opts.SiteURL, Token: model.NewId(), diff --git a/server/public/model/client4.go b/server/public/model/client4.go index 8a57eebeb517..37bee97aa285 100644 --- a/server/public/model/client4.go +++ b/server/public/model/client4.go @@ -45,6 +45,7 @@ const ( STATUS = "status" StatusOk = "OK" StatusFail = "FAIL" + StatusDisabled = "disabled" StatusUnhealthy = "UNHEALTHY" StatusRemove = "REMOVE" ConnectionId = "Connection-Id" diff --git a/server/public/model/remote_cluster.go b/server/public/model/remote_cluster.go index be55c861d8e9..ccd2947de74c 100644 --- a/server/public/model/remote_cluster.go +++ b/server/public/model/remote_cluster.go @@ -11,6 +11,7 @@ import ( "crypto/sha256" "encoding/json" "errors" + "fmt" "io" "net/http" "net/url" @@ -201,6 +202,35 @@ func IsValidRemoteName(s string) bool { return validRemoteNameChars.MatchString(s) } +// CleanRemoteName converts an arbitrary string into a slug compatible with +// IsValidRemoteName: lowercased, with spaces and other disallowed characters +// replaced by hyphens. The result is truncated to RemoteNameMaxLength. If +// the cleaned value is still invalid (e.g. empty input), a new ID is +// substituted so the caller always receives a valid name. +func CleanRemoteName(s string) string { + s = strings.ToLower(strings.ReplaceAll(s, " ", "-")) + s = strings.TrimSpace(s) + + for _, c := range s { + char := fmt.Sprintf("%c", c) + if !validRemoteNameChars.MatchString(char) { + s = strings.ReplaceAll(s, char, "-") + } + } + + s = strings.Trim(s, "-") + + if len(s) > RemoteNameMaxLength { + s = strings.Trim(s[:RemoteNameMaxLength], "-") + } + + if !IsValidRemoteName(s) { + s = NewId() + } + + return s +} + func (rc *RemoteCluster) PreUpdate() { if rc.DisplayName == "" { rc.DisplayName = rc.Name diff --git a/server/public/model/remote_cluster_test.go b/server/public/model/remote_cluster_test.go index 9c6418ead168..dbc99e3566df 100644 --- a/server/public/model/remote_cluster_test.go +++ b/server/public/model/remote_cluster_test.go @@ -6,6 +6,7 @@ package model import ( "crypto/rand" "io" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -257,3 +258,44 @@ func TestRemoteClusterIsPlugin(t *testing.T) { assert.False(t, rc.IsPlugin(), "IsPlugin should only check PluginID, not SiteURL prefix") }) } + +func TestCleanRemoteName(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {name: "single space", in: "legacy plugin", want: "legacy-plugin"}, + {name: "multiple spaces", in: "remote 1 plugin", want: "remote-1-plugin"}, + {name: "uppercase", in: "Plugin A", want: "plugin-a"}, + {name: "preserves dot, hyphen, underscore", in: "com.example_plugin-1", want: "com.example_plugin-1"}, + {name: "trims separators", in: " ---my remote--- ", want: "my-remote"}, + {name: "punctuation collapses", in: "plugin@home!", want: "plugin-home"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := CleanRemoteName(tc.in) + assert.Equal(t, tc.want, got) + assert.True(t, IsValidRemoteName(got), "CleanRemoteName must produce a valid name") + }) + } + + t.Run("empty input falls back to NewId", func(t *testing.T) { + got := CleanRemoteName("") + assert.True(t, IsValidRemoteName(got)) + assert.Len(t, got, 26) + }) + + t.Run("only invalid characters falls back to NewId", func(t *testing.T) { + got := CleanRemoteName("@@@ !!! ???") + assert.True(t, IsValidRemoteName(got)) + assert.Len(t, got, 26) + }) + + t.Run("over-length input is truncated", func(t *testing.T) { + in := strings.Repeat("a", RemoteNameMaxLength+10) + got := CleanRemoteName(in) + assert.True(t, IsValidRemoteName(got)) + assert.LessOrEqual(t, len(got), RemoteNameMaxLength) + }) +} diff --git a/server/public/model/support_packet.go b/server/public/model/support_packet.go index 2616202305fd..7a2d85a893c7 100644 --- a/server/public/model/support_packet.go +++ b/server/public/model/support_packet.go @@ -75,6 +75,17 @@ type SupportPacketDiagnostics struct { NumberOfNodes int `yaml:"number_of_nodes"` } `yaml:"cluster"` + Notifications struct { + Email struct { + Status string `yaml:"status"` + Error string `yaml:"error,omitempty"` + } `yaml:"email,omitempty"` + Push struct { + Status string `yaml:"status"` + Error string `yaml:"error,omitempty"` + } `yaml:"push,omitempty"` + } `yaml:"notifications,omitempty"` + LDAP struct { Status string `yaml:"status,omitempty"` Error string `yaml:"error,omitempty"` @@ -87,6 +98,7 @@ type SupportPacketDiagnostics struct { } `yaml:"saml"` ElasticSearch struct { + Status string `yaml:"status,omitempty"` Backend string `yaml:"backend,omitempty"` ServerVersion string `yaml:"server_version,omitempty"` ServerPlugins []string `yaml:"server_plugins,omitempty"` diff --git a/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.scss b/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.scss index df3acf111955..d793b81c6e86 100644 --- a/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.scss +++ b/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.scss @@ -40,6 +40,11 @@ &__policy-banner { .TagGroup { margin-top: 12px; + + // Tighten vertical spacing when tags wrap onto multiple rows + // (TagGroup defaults to gap: 8px which is too airy in this + // banner). Column spacing is preserved. + row-gap: 0; } } } diff --git a/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.test.tsx b/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.test.tsx index 67aac2b79a88..62ed4b7bfec3 100644 --- a/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.test.tsx +++ b/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.test.tsx @@ -502,9 +502,9 @@ describe('components/channel_invite_modal', () => { // Modal renders in a portal, so query from document instead of container expect(document.querySelector('.AlertBanner')).not.toBeNull(); - // Check that the attribute tags are shown - expect(screen.getByText('tag1')).toBeInTheDocument(); - expect(screen.getByText('tag2')).toBeInTheDocument(); + // Check that the attribute tags are shown in "Attribute: value" form. + expect(screen.getByText('Attribute1: tag1')).toBeInTheDocument(); + expect(screen.getByText('Attribute1: tag2')).toBeInTheDocument(); }); test('should not show AlertBanner when policy_enforced is false', () => { diff --git a/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.tsx b/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.tsx index a20029404a70..0bdd450eddc1 100644 --- a/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.tsx +++ b/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.tsx @@ -33,6 +33,7 @@ import GuestTag from 'components/widgets/tag/guest_tag'; import TagGroup from 'components/widgets/tag/tag_group'; import Constants, {ModalIdentifiers} from 'utils/constants'; +import {formatAttributeName} from 'utils/format_attribute_name'; import {sortUsersAndGroups} from 'utils/utils'; import GroupOption from './group_option'; @@ -112,14 +113,29 @@ const ChannelInviteModalComponent = (props: Props) => { props.channel.policy_enforced, ); - // Helper function to format attribute names for tooltips - const formatAttributeName = (name: string): string => { - // Convert snake_case or camelCase to Title Case with spaces - return name. - replace(/_/g, ' '). - replace(/([A-Z])/g, ' $1'). - replace(/\w\S*/g, (txt) => txt.charAt(0).toUpperCase() + txt.substring(1).toLowerCase()); - }; + // Memoise the rendered access-control tags so they don't re-render on + // every keystroke in the invite text box. + const accessControlTags = useMemo(() => { + if (structuredAttributes.length === 0) { + return null; + } + return ( + + {structuredAttributes.flatMap((attribute) => + attribute.values.map((value) => { + const attributeLabel = formatAttributeName(attribute.name); + return ( + + ); + }), + )} + + ); + }, [structuredAttributes]); // Helper function to add a user or group to the selected list const addValue = useCallback((value: UserProfileValue | GroupValue) => { @@ -667,19 +683,7 @@ const ChannelInviteModalComponent = (props: Props) => { /> )} > - {structuredAttributes.length > 0 && ( - - {structuredAttributes.flatMap((attribute) => - attribute.values.map((value) => ( - - )), - )} - - )} + {accessControlTags} )} diff --git a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.scss b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.scss index bcd8f25472d8..838a589ac40d 100644 --- a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.scss +++ b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.scss @@ -34,6 +34,11 @@ .TagGroup { margin-top: 12px; + + // Tighten vertical spacing when tags wrap onto multiple rows + // (TagGroup defaults to gap: 8px which is too airy in this + // banner). Column spacing is preserved. + row-gap: 0; } } } diff --git a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.test.tsx b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.test.tsx index d7b6f5f56401..8b580440f498 100644 --- a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.test.tsx +++ b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.test.tsx @@ -225,7 +225,9 @@ describe('channel_members_rhs/channel_members_rhs', () => { ); expect(screen.getByText('Channel access is restricted by user attributes')).toBeInTheDocument(); - expect(screen.getByText('tag1')).toBeInTheDocument(); - expect(screen.getByText('tag2')).toBeInTheDocument(); + + // Each tag is rendered as "Attribute: value" for readability. + expect(screen.getByText('Attribute1: tag1')).toBeInTheDocument(); + expect(screen.getByText('Attribute1: tag2')).toBeInTheDocument(); }); }); diff --git a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.tsx b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.tsx index 22fc87e4108e..15aa6dcaa0c8 100644 --- a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.tsx +++ b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.tsx @@ -2,7 +2,7 @@ // See LICENSE.txt for license information. import debounce from 'lodash/debounce'; -import React, {useCallback, useEffect, useState} from 'react'; +import React, {useCallback, useEffect, useMemo, useState} from 'react'; import {FormattedMessage, useIntl} from 'react-intl'; import {useHistory} from 'react-router-dom'; @@ -20,6 +20,7 @@ import AlertTag from 'components/widgets/tag/alert_tag'; import TagGroup from 'components/widgets/tag/tag_group'; import Constants, {ModalIdentifiers} from 'utils/constants'; +import {formatAttributeName} from 'utils/format_attribute_name'; import type {ModalData} from 'types/actions'; @@ -84,14 +85,29 @@ export default function ChannelMembersRHS({ channel.policy_enforced, ); - // Helper function to format attribute names for tooltips - const formatAttributeName = (name: string): string => { - // Convert snake_case or camelCase to Title Case with spaces - return name. - replace(/_/g, ' '). - replace(/([A-Z])/g, ' $1'). - replace(/\w\S*/g, (txt) => txt.charAt(0).toUpperCase() + txt.substring(1).toLowerCase()); - }; + // Memoise the rendered access-control tags so they don't re-render on + // every unrelated state change in the centre channel. + const accessControlTags = useMemo(() => { + if (structuredAttributes.length === 0) { + return null; + } + return ( + + {structuredAttributes.flatMap((attribute) => + attribute.values.map((value) => { + const attributeLabel = formatAttributeName(attribute.name); + return ( + + ); + }), + )} + + ); + }, [structuredAttributes]); const searching = searchTerms !== ''; @@ -245,19 +261,7 @@ export default function ChannelMembersRHS({ defaultMessage: 'Channel access is restricted by user attributes', })} > - {structuredAttributes.length > 0 && ( - - {structuredAttributes.flatMap((attribute) => - attribute.values.map((value) => ( - - )), - )} - - )} + {accessControlTags} {loading && {'Loading...'}} diff --git a/webapp/channels/src/components/common/hooks/useAccessControlAttributes.test.tsx b/webapp/channels/src/components/common/hooks/useAccessControlAttributes.test.tsx index 35c4113e5782..c32d0807151a 100644 --- a/webapp/channels/src/components/common/hooks/useAccessControlAttributes.test.tsx +++ b/webapp/channels/src/components/common/hooks/useAccessControlAttributes.test.tsx @@ -7,7 +7,7 @@ import {Provider} from 'react-redux'; import configureStore from 'redux-mock-store'; import {thunk} from 'redux-thunk'; -import {useAccessControlAttributes, EntityType} from './useAccessControlAttributes'; +import {invalidateAccessControlAttributesCache, useAccessControlAttributes, EntityType} from './useAccessControlAttributes'; // Mock the getChannelAccessControlAttributes action jest.mock('mattermost-redux/actions/channels', () => { @@ -202,4 +202,31 @@ describe('useAccessControlAttributes', () => { // The action should have been called again expect(getChannelAccessControlAttributes).toHaveBeenCalledWith('channel-1'); }); + + test('invalidateAccessControlAttributesCache forces a refresh on next read', async () => { + // Prime the cache with a first fetch. + const {result: result1} = renderHook(() => useAccessControlAttributes(EntityType.Channel, 'channel-1', true), {wrapper}); + + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 100)); + }); + expect(result1.current.structuredAttributes).toEqual([ + {name: 'department', values: ['engineering', 'marketing']}, + {name: 'location', values: ['remote']}, + ]); + + const getChannelAccessControlAttributes = require('mattermost-redux/actions/channels').getChannelAccessControlAttributes; + getChannelAccessControlAttributes.mockClear(); + + // After invalidating the cache the next mount should hit the action again. + invalidateAccessControlAttributesCache(EntityType.Channel, 'channel-1'); + + const {result: result2} = renderHook(() => useAccessControlAttributes(EntityType.Channel, 'channel-1', true), {wrapper}); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 100)); + }); + + expect(getChannelAccessControlAttributes).toHaveBeenCalledWith('channel-1'); + expect(result2.current.structuredAttributes).toEqual(result1.current.structuredAttributes); + }); }); diff --git a/webapp/channels/src/components/common/hooks/useAccessControlAttributes.ts b/webapp/channels/src/components/common/hooks/useAccessControlAttributes.ts index 1b21e86694fa..d2c83392d416 100644 --- a/webapp/channels/src/components/common/hooks/useAccessControlAttributes.ts +++ b/webapp/channels/src/components/common/hooks/useAccessControlAttributes.ts @@ -15,22 +15,74 @@ export enum EntityType { // more entity types will be added here in the future } -// Module-level cache for access control attributes -// The cache stores processed data with a timestamp to implement a TTL (time-to-live) +// ProcessedAttributes is the shape that components consume: a structured +// dictionary of attribute -> values (used for the policy banners) plus a flat +// tag array (kept for backwards compatibility with consumers that only need +// the values without their owning attribute). +type ProcessedAttributes = { + attributeTags: string[]; + structuredAttributes: AccessControlAttribute[]; +}; + +// Module-level cache for access control attributes. The cache stores already +// processed data with a timestamp to implement a short TTL. const attributesCache: Record = {}; -// Cache TTL in milliseconds (5 minutes) +// Cache TTL in milliseconds (5 minutes). const CACHE_TTL = 5 * 60 * 1000; // Array of supported entity types for validation const SUPPORTED_ENTITY_TYPES = Object.values(EntityType); +const EMPTY_PROCESSED: ProcessedAttributes = { + attributeTags: [], + structuredAttributes: [], +}; + +// processAttributeData converts the server response (a dictionary keyed by +// attribute name with arrays of literal values) into the structured form +// consumed by the UI. The dictionary preserves both the attribute name and +// the values associated with it so each tag can be displayed alongside its +// originating attribute. +function processAttributeData(data: Record | undefined | null): ProcessedAttributes { + if (!data) { + return EMPTY_PROCESSED; + } + + const attributeTags: string[] = []; + const structuredAttributes: AccessControlAttribute[] = []; + + // Format: { "attributeName": ["value1", "value2"], "anotherAttribute": ["value3"] } + for (const [name, values] of Object.entries(data)) { + if (!Array.isArray(values)) { + continue; + } + + structuredAttributes.push({name, values: [...values]}); + + for (const value of values) { + if (value !== undefined && value !== null) { + attributeTags.push(value); + } + } + } + + return {attributeTags, structuredAttributes}; +} + +/** + * Invalidates the cached attributes for a single entity. Components that + * mutate the underlying access policy should call this to ensure the next + * read fetches fresh data instead of serving up to CACHE_TTL milliseconds of + * stale state. + */ +export function invalidateAccessControlAttributesCache(entityType: EntityType, entityId: string): void { + delete attributesCache[`${entityType}:${entityId}`]; +} + /** * A hook for fetching access control attributes for an entity * @@ -50,35 +102,9 @@ export const useAccessControlAttributes = ( const [error, setError] = useState(null); const dispatch = useDispatch(); - // Helper function to process attribute data and extract tags - const processAttributeData = useCallback((data: Record | undefined) => { - if (!data) { - setAttributeTags([]); - setStructuredAttributes([]); - return; - } - - const tags: string[] = []; - const attributes: AccessControlAttribute[] = []; - - // Extract values from all properties in the response - // Format: { "attributeName": ["value1", "value2"], "anotherAttribute": ["value3"] } - Object.entries(data).forEach(([name, values]) => { - // Add to structured format - if (Array.isArray(values)) { - attributes.push({name, values: [...values]}); - - // Add to flat tags (existing behavior) - values.forEach((value) => { - if (value !== undefined && value !== null) { - tags.push(value); - } - }); - } - }); - - setAttributeTags(tags); - setStructuredAttributes(attributes); + const applyProcessed = useCallback((processed: ProcessedAttributes) => { + setAttributeTags(processed.attributeTags); + setStructuredAttributes(processed.structuredAttributes); }, []); const fetchAttributes = useCallback(async (forceRefresh = false) => { @@ -86,32 +112,25 @@ export const useAccessControlAttributes = ( return; } - // Set loading state at the beginning setLoading(true); setError(null); try { - // Validate entity type first if (!SUPPORTED_ENTITY_TYPES.includes(entityType)) { throw new Error(`Unsupported entity type: ${entityType}`); } - // Check cache first (unless forceRefresh is true) const cacheKey = `${entityType}:${entityId}`; const cachedEntry = attributesCache[cacheKey]; const now = Date.now(); - // Use cache if it exists and is not too old and forceRefresh is false - // But still set loading to false to trigger a state update for tests + // Serve from cache when fresh and the caller didn't force a refresh. if (!forceRefresh && cachedEntry && (now - cachedEntry.timestamp < CACHE_TTL)) { - // Use the cached processed data directly instead of reprocessing - setAttributeTags(cachedEntry.processedData.attributeTags); - setStructuredAttributes(cachedEntry.processedData.structuredAttributes); + applyProcessed(cachedEntry.processedData); setLoading(false); return; } - // Handle different entity types let result; switch (entityType) { case EntityType.Channel: @@ -122,56 +141,21 @@ export const useAccessControlAttributes = ( throw new Error(`Unsupported entity type: ${entityType}`); } - // Check for error in the result if (result.error) { throw result.error; } - const data = result.data; - - // Process the data and store it in cache - if (data) { - const processedTags: string[] = []; - const processedAttributes: AccessControlAttribute[] = []; - - // Process the data once - Object.entries(data).forEach(([name, values]) => { - if (Array.isArray(values)) { - processedAttributes.push({name, values: [...values]}); - values.forEach((value) => { - if (value !== undefined && value !== null) { - processedTags.push(value); - } - }); - } - }); - - // Store only the processed data - attributesCache[cacheKey] = { - processedData: { - attributeTags: processedTags, - structuredAttributes: processedAttributes, - }, - timestamp: now, - }; - - // Set state - setAttributeTags(processedTags); - setStructuredAttributes(processedAttributes); - } else { - // Handle the case where data is undefined or null - setAttributeTags([]); - setStructuredAttributes([]); - } + + const processed = processAttributeData(result.data); + attributesCache[cacheKey] = {processedData: processed, timestamp: now}; + applyProcessed(processed); } catch (err) { setError(err as Error); - setAttributeTags([]); - setStructuredAttributes([]); + applyProcessed(EMPTY_PROCESSED); } finally { setLoading(false); } - }, [entityType, entityId, hasAccessControl, processAttributeData]); + }, [entityType, entityId, hasAccessControl, dispatch, applyProcessed]); - // Fetch attributes when the component mounts or when dependencies change useEffect(() => { fetchAttributes(); }, [fetchAttributes]); diff --git a/webapp/channels/src/components/sidebar/sidebar_channel/sidebar_channel_menu/__snapshots__/sidebar_channel_menu.test.tsx.snap b/webapp/channels/src/components/sidebar/sidebar_channel/sidebar_channel_menu/__snapshots__/sidebar_channel_menu.test.tsx.snap index 8b17f0a3de0b..f0b8fb0305de 100644 --- a/webapp/channels/src/components/sidebar/sidebar_channel/sidebar_channel_menu/__snapshots__/sidebar_channel_menu.test.tsx.snap +++ b/webapp/channels/src/components/sidebar/sidebar_channel/sidebar_channel_menu/__snapshots__/sidebar_channel_menu.test.tsx.snap @@ -32,55 +32,7 @@ exports[`components/sidebar/sidebar_channel/sidebar_channel_menu should match sn + /> + />