diff --git a/server/bridge/manager.go b/server/bridge/manager.go index 1137368..5dc1f9a 100644 --- a/server/bridge/manager.go +++ b/server/bridge/manager.go @@ -4,6 +4,7 @@ import ( "fmt" "sync" + "github.com/mattermost/mattermost-plugin-bridge-xmpp/server/config" "github.com/mattermost/mattermost-plugin-bridge-xmpp/server/logger" "github.com/mattermost/mattermost-plugin-bridge-xmpp/server/model" mmModel "github.com/mattermost/mattermost/server/public/model" @@ -196,7 +197,7 @@ func (m *BridgeManager) Shutdown() error { } // OnPluginConfigurationChange propagates configuration changes to all registered bridges -func (m *BridgeManager) OnPluginConfigurationChange(config any) error { +func (m *BridgeManager) OnPluginConfigurationChange(config *config.Configuration) error { m.mu.RLock() defer m.mu.RUnlock() diff --git a/server/bridge/mattermost/bridge.go b/server/bridge/mattermost/bridge.go index 79baf6e..c2cdbec 100644 --- a/server/bridge/mattermost/bridge.go +++ b/server/bridge/mattermost/bridge.go @@ -52,11 +52,18 @@ func NewBridge(log logger.Logger, api plugin.API, kvstore kvstore.KVStore, cfg * return bridge } +// getConfiguration safely retrieves the current configuration +func (b *mattermostBridge) getConfiguration() *config.Configuration { + b.configMu.RLock() + defer b.configMu.RUnlock() + return b.config +} + // UpdateConfiguration updates the bridge configuration -func (b *mattermostBridge) UpdateConfiguration(newConfig any) error { - cfg, ok := newConfig.(*config.Configuration) - if !ok { - return fmt.Errorf("invalid configuration type") +func (b *mattermostBridge) UpdateConfiguration(cfg *config.Configuration) error { + // Validate configuration using built-in validation + if err := cfg.IsValid(); err != nil { + return fmt.Errorf("invalid configuration: %w", err) } b.configMu.Lock() diff --git a/server/bridge/xmpp/bridge.go b/server/bridge/xmpp/bridge.go index 4a1ffe6..b375ce4 100644 --- a/server/bridge/xmpp/bridge.go +++ b/server/bridge/xmpp/bridge.go @@ -80,56 +80,50 @@ func (b *xmppBridge) createXMPPClient(cfg *config.Configuration) *xmppClient.Cli ) } +// getConfiguration safely retrieves the current configuration +func (b *xmppBridge) getConfiguration() *config.Configuration { + b.configMu.RLock() + defer b.configMu.RUnlock() + return b.config +} + // UpdateConfiguration updates the bridge configuration -func (b *xmppBridge) UpdateConfiguration(newConfig any) error { - cfg, ok := newConfig.(*config.Configuration) - if !ok { - return fmt.Errorf("invalid configuration type") +// It handles validation and reconnection logic when the configuration changes +func (b *xmppBridge) UpdateConfiguration(cfg *config.Configuration) error { + // Validate configuration using built-in validation + if err := cfg.IsValid(); err != nil { + return fmt.Errorf("invalid configuration: %w", err) } - b.configMu.Lock() - oldConfig := b.config - b.config = cfg - defer b.configMu.Unlock() + // Get current config to check if restart is needed + oldConfig := b.getConfiguration() - b.logger.LogInfo("XMPP bridge configuration updated") + // Update configuration under lock, then release immediately + b.configMu.Lock() + b.config = cfg // Initialize or update XMPP client with new configuration - if cfg.EnableSync { - if cfg.XMPPServerURL == "" || cfg.XMPPUsername == "" || cfg.XMPPPassword == "" { - return fmt.Errorf("XMPP server URL, username, and password are required when sync is enabled") + if !cfg.Equals(oldConfig) { + if b.bridgeClient != nil && b.bridgeClient.Disconnect() != nil { + b.logger.LogError("Failed to disconnect old XMPP bridge client") } - b.bridgeClient = b.createXMPPClient(cfg) - } else { - b.bridgeClient = nil + } + b.configMu.Unlock() + + // Stop the bridge + if err := b.Stop(); err != nil { + b.logger.LogWarn("Error stopping bridge during restart", "error", err) } - // Check if we need to restart the bridge due to configuration changes - wasConnected := b.connected.Load() - needsRestart := oldConfig != nil && !oldConfig.Equals(cfg) && wasConnected - - // Log the configuration change - if needsRestart { - b.logger.LogInfo("Configuration changed, restarting bridge") - } else { - b.logger.LogInfo("Configuration updated", "config", cfg) + // Start the bridge with new configuration + // Start() method already uses getConfiguration() safely + if err := b.Start(); err != nil { + b.logger.LogError("Failed to restart bridge with new configuration", "error", err) + return fmt.Errorf("failed to restart bridge: %w", err) } - if needsRestart { - b.logger.LogInfo("Configuration changed, restarting bridge") - - // Stop the bridge - if err := b.Stop(); err != nil { - b.logger.LogWarn("Error stopping bridge during restart", "error", err) - } - - // Start the bridge with new configuration - if err := b.Start(); err != nil { - b.logger.LogError("Failed to restart bridge with new configuration", "error", err) - return fmt.Errorf("failed to restart bridge: %w", err) - } - } + b.logger.LogDebug("XMPP bridge configuration updated successfully") return nil } diff --git a/server/configuration.go b/server/configuration.go index 79fa130..b6bad1f 100644 --- a/server/configuration.go +++ b/server/configuration.go @@ -57,7 +57,7 @@ func (p *Plugin) OnConfigurationChange() error { return errors.Wrap(err, "failed to load plugin configuration") } - p.API.LogDebug("Loaded configuration in OnConfigurationChange", "configuration", configuration) + p.API.LogDebug("Plugin configuration changed") // Validate the configuration if err := configuration.IsValid(); err != nil { diff --git a/server/model/bridge.go b/server/model/bridge.go index bd00672..a5df73e 100644 --- a/server/model/bridge.go +++ b/server/model/bridge.go @@ -110,7 +110,7 @@ type BridgeManager interface { // OnPluginConfigurationChange propagates configuration changes to all registered bridges. // Returns an error if any bridge fails to update its configuration, but continues to // attempt updating all bridges. - OnPluginConfigurationChange(config any) error + OnPluginConfigurationChange(config *config.Configuration) error // CreateChannelMapping is called when a channel mapping is created. CreateChannelMapping(req CreateChannelMappingRequest) error @@ -121,7 +121,7 @@ type BridgeManager interface { type Bridge interface { // UpdateConfiguration updates the bridge configuration - UpdateConfiguration(config any) error + UpdateConfiguration(config *config.Configuration) error // Start starts the bridge Start() error diff --git a/server/xmpp/client.go b/server/xmpp/client.go index aad5e24..83f3709 100644 --- a/server/xmpp/client.go +++ b/server/xmpp/client.go @@ -18,6 +18,11 @@ import ( "mellium.im/xmpp/stanza" ) +const ( + // defaultOperationTimeout is the default timeout for XMPP operations + defaultOperationTimeout = 5 * time.Second +) + // Client represents an XMPP client for communicating with XMPP servers. type Client struct { serverURL string @@ -150,9 +155,13 @@ func (c *Client) Connect() error { } } - // Use DialClientSession for proper SASL authentication + // Create a timeout context for the connection attempt (30 seconds) + connectCtx, connectCancel := context.WithTimeout(c.ctx, 30*time.Second) + defer connectCancel() + + // Use DialClientSession for proper SASL authentication with timeout c.session, err = xmpp.DialClientSession( - c.ctx, + connectCtx, c.jidAddr, xmpp.StartTLS(tlsConfig), xmpp.SASL("", c.password, sasl.Plain), @@ -171,9 +180,12 @@ func (c *Client) Connect() error { if !c.sessionServing { return fmt.Errorf("failed to start session serving") } + c.logger.LogInfo("XMPP client connected successfully", "jid", c.jidAddr.String()) return nil - case <-time.After(5 * time.Second): + case <-time.After(10 * time.Second): return fmt.Errorf("timeout waiting for session to be ready") + case <-c.ctx.Done(): + return fmt.Errorf("connection cancelled: %w", c.ctx.Err()) } } @@ -206,18 +218,46 @@ func (c *Client) serveSession() { // Disconnect closes the XMPP connection func (c *Client) Disconnect() error { - if c.session != nil { - err := c.session.Close() - c.session = nil - if err != nil { - return fmt.Errorf("failed to close XMPP session: %w", err) - } + if c.session == nil { + return nil // Already disconnected } + c.logger.LogInfo("Disconnecting XMPP client", "jid", c.jidAddr.String()) + + // Send offline presence before disconnecting to properly leave rooms + if err := c.SetOfflinePresence(); err != nil { + c.logger.LogWarn("Failed to set offline presence before disconnect", "error", err) + // Don't fail the disconnect for presence issues + } + + // Close the session with a timeout to prevent hanging + sessionCloseCtx, cancel := context.WithTimeout(context.Background(), defaultOperationTimeout) + defer cancel() + + sessionCloseDone := make(chan error, 1) + go func() { + sessionCloseDone <- c.session.Close() + }() + + select { + case err := <-sessionCloseDone: + c.session = nil + if err != nil { + c.logger.LogWarn("Error closing XMPP session", "error", err) + return fmt.Errorf("failed to close XMPP session: %w", err) + } + case <-sessionCloseCtx.Done(): + c.logger.LogWarn("Timeout closing XMPP session, forcing disconnect") + c.session = nil + // Continue with cleanup even on timeout + } + + // Cancel the client context if c.cancel != nil { c.cancel() } + c.logger.LogInfo("XMPP client disconnected successfully") return nil } @@ -418,6 +458,30 @@ func (c *Client) SetOnlinePresence() error { return nil } +// SetOfflinePresence sends an offline presence stanza to indicate the client is going offline +func (c *Client) SetOfflinePresence() error { + if c.session == nil { + return fmt.Errorf("XMPP session not established") + } + + // Create presence stanza indicating we're unavailable + presence := stanza.Presence{ + Type: stanza.UnavailablePresence, + From: c.jidAddr, + } + + // Create a context with timeout for the presence update + ctx, cancel := context.WithTimeout(context.Background(), defaultOperationTimeout) + defer cancel() + + // Send the presence stanza + if err := c.session.Encode(ctx, presence); err != nil { + return fmt.Errorf("failed to send offline presence: %w", err) + } + + return nil +} + // CheckRoomExists verifies if an XMPP room exists and is accessible using disco#info func (c *Client) CheckRoomExists(roomJID string) (bool, error) { if c.session == nil {