From d2a1dc792f9a2bb147afc6e8fde2c9f723f29cf1 Mon Sep 17 00:00:00 2001 From: Duco van Amstel Date: Wed, 7 Nov 2018 20:35:59 +0000 Subject: [PATCH] Refactor and clean-up handlers. (slack) (#533) --- bridge/helper/helper.go | 11 +- bridge/slack/handlers.go | 341 ++++++++++++++++----------------------- bridge/slack/helpers.go | 86 ++++++++-- bridge/slack/slack.go | 39 ++++- 4 files changed, 255 insertions(+), 222 deletions(-) diff --git a/bridge/helper/helper.go b/bridge/helper/helper.go index a13e02af..bfa7d827 100644 --- a/bridge/helper/helper.go +++ b/bridge/helper/helper.go @@ -56,13 +56,10 @@ func SplitStringLength(input string, length int) string { func HandleExtra(msg *config.Message, general *config.Protocol) []config.Message { extra := msg.Extra rmsg := []config.Message{} - if len(extra[config.EVENT_FILE_FAILURE_SIZE]) > 0 { - for _, f := range extra[config.EVENT_FILE_FAILURE_SIZE] { - fi := f.(config.FileInfo) - text := fmt.Sprintf("file %s too big to download (%#v > allowed size: %#v)", fi.Name, fi.Size, general.MediaDownloadSize) - rmsg = append(rmsg, config.Message{Text: text, Username: " ", Channel: msg.Channel, Account: msg.Account}) - } - return rmsg + for _, f := range extra[config.EVENT_FILE_FAILURE_SIZE] { + fi := f.(config.FileInfo) + text := fmt.Sprintf("file %s too big to download (%#v > allowed size: %#v)", fi.Name, fi.Size, general.MediaDownloadSize) + rmsg = append(rmsg, config.Message{Text: text, Username: " ", Channel: msg.Channel, Account: msg.Account}) } return rmsg } diff --git a/bridge/slack/handlers.go b/bridge/slack/handlers.go index d6894bf6..9bcafde6 100644 --- a/bridge/slack/handlers.go +++ b/bridge/slack/handlers.go @@ -1,7 +1,6 @@ package bslack import ( - "bytes" "fmt" "html" "regexp" @@ -61,7 +60,15 @@ func (b *Bslack) handleSlackClient(messages chan *config.Message) { case *slack.OutgoingErrorEvent: b.Log.Debugf("%#v", ev.Error()) case *slack.ChannelJoinedEvent: + // When we join a channel we update the full list of users as + // well as the information for the channel that we joined as this + // should now tell that we are a member of it. b.populateUsers() + + b.channelsMutex.Lock() + b.channelsByID[ev.Channel.ID] = &ev.Channel + b.channelsByName[ev.Channel.Name] = &ev.Channel + b.channelsMutex.Unlock() case *slack.ConnectedEvent: b.si = ev.Info b.populateChannels() @@ -90,108 +97,115 @@ func (b *Bslack) handleMatterHook(messages chan *config.Message) { } } -var commentRE = regexp.MustCompile(`.*?commented: (.*)`) +// skipMessageEvent skips event that need to be skipped :-) +func (b *Bslack) skipMessageEvent(ev *slack.MessageEvent) bool { + switch ev.SubType { + case sChannelLeave, sChannelJoin: + return b.GetBool(noSendJoinConfig) + case sPinnedItem, sUnpinnedItem: + return true + } -// handleDownloadFile handles file download -func (b *Bslack) handleDownloadFile(rmsg *config.Message, file *slack.File) error { - // if we have a file attached, download it (in memory) and put a pointer to it in msg.Extra - // limit to 1MB for now - comment := "" - results := commentRE.FindAllStringSubmatch(rmsg.Text, -1) - if len(results) > 0 { - comment = results[0][1] + // Skip any messages that we made ourselves or from 'slackbot' (see #527). + if ev.Username == sSlackBotUser || + (b.rtm != nil && ev.Username == b.si.User.Name) || + (len(ev.Attachments) > 0 && ev.Attachments[0].CallbackID == "matterbridge_"+b.uuid) { + return true } - err := helper.HandleDownloadSize(b.Log, rmsg, file.Name, int64(file.Size), b.General) - if err != nil { - return err + + // It seems ev.SubMessage.Edited == nil when slack unfurls. + // Do not forward these messages. See Github issue #266. + if ev.SubMessage != nil && + ev.SubMessage.ThreadTimestamp != ev.SubMessage.Timestamp && + ev.SubMessage.Edited == nil { + return true } - // actually download the file - data, err := helper.DownloadFileAuth(file.URLPrivateDownload, "Bearer "+b.GetString(tokenConfig)) - if err != nil { - return fmt.Errorf("download %s failed %#v", file.URLPrivateDownload, err) - } - // add the downloaded data to the message - helper.HandleDownloadData(b.Log, rmsg, file.Name, comment, file.URLPrivateDownload, data, b.General) - return nil + return false } -// handleUploadFile handles native upload of files -func (b *Bslack) handleUploadFile(msg *config.Message, channelID string) { - for _, f := range msg.Extra["file"] { - fi := f.(config.FileInfo) - if msg.Text == fi.Comment { - msg.Text = "" - } - /* because the result of the UploadFile is slower than the MessageEvent from slack - we can't match on the file ID yet, so we have to match on the filename too - */ - b.Log.Debugf("Adding file %s to cache %s", fi.Name, time.Now().String()) - b.cache.Add("filename"+fi.Name, time.Now()) - res, err := b.sc.UploadFile(slack.FileUploadParameters{ - Reader: bytes.NewReader(*fi.Data), - Filename: fi.Name, - Channels: []string{channelID}, - InitialComment: fi.Comment, - }) - if res.ID != "" { - b.Log.Debugf("Adding fileid %s to cache %s", res.ID, time.Now().String()) - b.cache.Add("file"+res.ID, time.Now()) - } - if err != nil { - b.Log.Errorf("uploadfile %#v", err) - } - } -} - -// handleMessageEvent handles the message events +// handleMessageEvent handles the message events. Together with any called sub-methods, +// this method implements the following event processing pipeline: +// +// 1. Check if the message should be ignored. +// NOTE: This is not actually part of the method below but is done just before it +// is called via the 'skipMessageEvent()' method. +// 2. Populate the Matterbridge message that will be sent to the router based on the +// received event and logic that is common to all events that are not skipped. +// 3. Detect and handle any message that is "status" related (think join channel, etc.). +// This might result in an early exit from the pipeline and passing of the +// pre-populated message to the Matterbridge router. +// 4. Handle the specific case of messages that edit existing messages depending on +// configuration. +// 5. Handle any attachments of the received event. +// 6. Check that the Matterbridge message that we end up with after at the end of the +// pipeline is valid before sending it to the Matterbridge router. func (b *Bslack) handleMessageEvent(ev *slack.MessageEvent) (*config.Message, error) { - var err error - - // update the userlist on a channel_join - if ev.SubType == sChannelJoin { - b.populateUsers() - } - - // Edit message - if !b.GetBool(editDisableConfig) && ev.SubMessage != nil && ev.SubMessage.ThreadTimestamp != ev.SubMessage.Timestamp { - b.Log.Debugf("SubMessage %#v", ev.SubMessage) - ev.User = ev.SubMessage.User - ev.Text = ev.SubMessage.Text + b.GetString(editSuffixConfig) - } - - // use our own func because rtm.GetChannelInfo doesn't work for private channels - channelInfo, err := b.getChannelByID(ev.Channel) + rmsg, err := b.populateReceivedMessage(ev) if err != nil { return nil, err } - rmsg := config.Message{ - Text: ev.Text, - Channel: channelInfo.Name, - Account: b.Account, - ID: "slack " + ev.Timestamp, - Extra: map[string][]interface{}{}, - ParentID: ev.ThreadTimestamp, + // Handle some message types early. + if b.handleStatusEvent(ev, rmsg) { + return rmsg, nil } - if b.useChannelID { - rmsg.Channel = "ID:" + channelInfo.ID - } - - // find the user id and name - if ev.User != "" && ev.SubType != sMessageDeleted && ev.SubType != sFileComment { - user, err := b.rtm.GetUserInfo(ev.User) - if err != nil { - return nil, err - } - rmsg.UserID = user.ID - rmsg.Username = user.Name - if user.Profile.DisplayName != "" { - rmsg.Username = user.Profile.DisplayName + // Handle 'edit' messages. + if ev.SubMessage != nil && !b.GetBool(editDisableConfig) { + rmsg.ID = "slack " + ev.SubMessage.Timestamp + if ev.SubMessage.ThreadTimestamp != ev.SubMessage.Timestamp { + b.Log.Debugf("SubMessage %#v", ev.SubMessage) + rmsg.Username = ev.SubMessage.User + rmsg.Text = ev.SubMessage.Text + b.GetString(editSuffixConfig) } } - // See if we have some text in the attachments + b.handleAttachments(ev, rmsg) + + // Verify that we have the right information and the message + // is well-formed before sending it out to the router. + if len(ev.Files) == 0 && (rmsg.Text == "" || rmsg.Username == "") { + if ev.BotID != "" { + // This is probably a webhook we couldn't resolve. + return nil, fmt.Errorf("message handling resulted in an empty bot message (probably an incoming webhook we couldn't resolve): %#v", ev) + } + return nil, fmt.Errorf("message handling resulted in an empty message: %#v", ev) + } + return rmsg, nil +} + +func (b *Bslack) handleStatusEvent(ev *slack.MessageEvent, rmsg *config.Message) bool { + switch ev.SubType { + case sChannelJoined, sMemberJoined: + b.populateUsers() + // There's no further processing needed on channel events + // so we return 'true'. + return true + case sChannelJoin, sChannelLeave: + rmsg.Username = sSystemUser + rmsg.Event = config.EVENT_JOIN_LEAVE + case sChannelTopic, sChannelPurpose: + rmsg.Event = config.EVENT_TOPIC_CHANGE + case sMessageDeleted: + rmsg.Text = config.EVENT_MSG_DELETE + rmsg.Event = config.EVENT_MSG_DELETE + rmsg.ID = "slack " + ev.DeletedTimestamp + // If a message is being deleted we do not need to process + // the event any further so we return 'true'. + return true + case sMeMessage: + rmsg.Event = config.EVENT_USER_ACTION + } + return false +} + +func (b *Bslack) handleAttachments(ev *slack.MessageEvent, rmsg *config.Message) { + // File comments are set by the system (because there is no username given). + if ev.SubType == sFileComment { + rmsg.Username = sSystemUser + } + + // See if we have some text in the attachments. if rmsg.Text == "" { for _, attach := range ev.Attachments { if attach.Text != "" { @@ -205,134 +219,55 @@ func (b *Bslack) handleMessageEvent(ev *slack.MessageEvent) (*config.Message, er } } - // when using webhookURL we can't check if it's our webhook or not for now - if rmsg.Username == "" && ev.BotID != "" && b.GetString(outgoingWebhookConfig) == "" { - bot, err := b.rtm.GetBotInfo(ev.BotID) - if err != nil { - return nil, err - } - if bot.Name != "" { - rmsg.Username = bot.Name - if ev.Username != "" { - rmsg.Username = ev.Username - } - rmsg.UserID = bot.ID - } - - // fixes issues with matterircd users - if bot.Name == "Slack API Tester" { - user, err := b.rtm.GetUserInfo(ev.User) - if err != nil { - return nil, err - } - rmsg.UserID = user.ID - rmsg.Username = user.Name - if user.Profile.DisplayName != "" { - rmsg.Username = user.Profile.DisplayName - } - } - } - - // file comments are set by the system (because there is no username given) - if ev.SubType == sFileComment { - rmsg.Username = sSystemUser - } - - // do we have a /me action - if ev.SubType == sMeMessage { - rmsg.Event = config.EVENT_USER_ACTION - } - - // Handle join/leave - if ev.SubType == sChannelLeave || ev.SubType == sChannelJoin { - rmsg.Username = sSystemUser - rmsg.Event = config.EVENT_JOIN_LEAVE - } - - // edited messages have a submessage, use this timestamp - if ev.SubMessage != nil { - rmsg.ID = "slack " + ev.SubMessage.Timestamp - } - - // deleted message event - if ev.SubType == sMessageDeleted { - rmsg.Text = config.EVENT_MSG_DELETE - rmsg.Event = config.EVENT_MSG_DELETE - rmsg.ID = "slack " + ev.DeletedTimestamp - } - - // topic change event - if ev.SubType == sChannelTopic || ev.SubType == sChannelPurpose { - rmsg.Event = config.EVENT_TOPIC_CHANGE - } - - // Only deleted messages can have a empty username and text - if (rmsg.Text == "" || rmsg.Username == "") && ev.SubType != sMessageDeleted && len(ev.Files) == 0 { - // this is probably a webhook we couldn't resolve - if ev.BotID != "" { - return nil, fmt.Errorf("probably an incoming webhook we couldn't resolve (maybe ourselves)") - } - return nil, fmt.Errorf("empty message and not a deleted message") - } - - // save the attachments, so that we can send them to other slack (compatible) bridges + // Save the attachments, so that we can send them to other slack (compatible) bridges. if len(ev.Attachments) > 0 { rmsg.Extra[sSlackAttachment] = append(rmsg.Extra[sSlackAttachment], ev.Attachments) } - // if we have a file attached, download it (in memory) and put a pointer to it in msg.Extra + // If we have files attached, download them (in memory) and put a pointer to it in msg.Extra. for _, f := range ev.Files { - err := b.handleDownloadFile(&rmsg, &f) + err := b.handleDownloadFile(rmsg, &f) if err != nil { - b.Log.Errorf("download failed: %s", err) + b.Log.Errorf("Could not download incoming file: %#v", err) } } - - return &rmsg, nil } -// skipMessageEvent skips event that need to be skipped :-) -func (b *Bslack) skipMessageEvent(ev *slack.MessageEvent) bool { - if ev.SubType == sChannelLeave || ev.SubType == sChannelJoin { - return b.GetBool(noSendJoinConfig) +var commentRE = regexp.MustCompile(`.*?commented: (.*)`) + +// handleDownloadFile handles file download +func (b *Bslack) handleDownloadFile(rmsg *config.Message, file *slack.File) error { + if b.fileIsAvailable(file) { + return nil } - // ignore pinned items - if ev.SubType == sPinnedItem || ev.SubType == sUnpinnedItem { + // Check that the file is neither too large nor blacklisted. + if err := helper.HandleDownloadSize(b.Log, rmsg, file.Name, int64(file.Size), b.General); err != nil { + b.Log.WithError(err).Infof("Skipping download of incoming file.") + return nil + } + + // Actually download the file. + data, err := helper.DownloadFileAuth(file.URLPrivateDownload, "Bearer "+b.GetString(tokenConfig)) + if err != nil { + return fmt.Errorf("download %s failed %#v", file.URLPrivateDownload, err) + } + + // Add the downloaded data to the message. + var comment string + if results := commentRE.FindAllStringSubmatch(rmsg.Text, -1); len(results) > 0 { + comment = results[0][1] + } + helper.HandleDownloadData(b.Log, rmsg, file.Name, comment, file.URLPrivateDownload, data, b.General) + return nil +} + +func (b *Bslack) fileIsAvailable(file *slack.File) bool { + // Only download a file if it is not in the cache or if it has been entered more than a minute ago. + if ts, ok := b.cache.Get("file" + file.ID); ok && time.Since(ts.(time.Time)) > time.Minute { + return true + } else if ts, ok = b.cache.Get("filename" + file.Name); ok && time.Since(ts.(time.Time)) > 10*time.Second { return true } - - // do not send messages from ourself - if b.GetString(outgoingWebhookConfig) == "" && b.GetString(incomingWebhookConfig) == "" && ev.Username == b.si.User.Name { - return true - } - - // skip messages we made ourselves - if len(ev.Attachments) > 0 { - if ev.Attachments[0].CallbackID == "matterbridge_"+b.uuid { - return true - } - } - - if !b.GetBool(editDisableConfig) && ev.SubMessage != nil && ev.SubMessage.ThreadTimestamp != ev.SubMessage.Timestamp { - // it seems ev.SubMessage.Edited == nil when slack unfurls - // do not forward these messages #266 - if ev.SubMessage.Edited == nil { - return true - } - } - - for _, f := range ev.Files { - // if the file is in the cache and isn't older then a minute, skip it - if ts, ok := b.cache.Get("file" + f.ID); ok && time.Since(ts.(time.Time)) < time.Minute { - b.Log.Debugf("Not downloading file id %s which we uploaded", f.ID) - return true - } else if ts, ok := b.cache.Get("filename" + f.Name); ok && time.Since(ts.(time.Time)) < 10*time.Second { - b.Log.Debugf("Not downloading file name %s which we uploaded", f.Name) - return true - } - b.Log.Debugf("Not skipping %s %s", f.Name, time.Now().String()) - } - return false } diff --git a/bridge/slack/helpers.go b/bridge/slack/helpers.go index 23d5816a..e46a71ab 100644 --- a/bridge/slack/helpers.go +++ b/bridge/slack/helpers.go @@ -6,27 +6,31 @@ import ( "strings" "time" + "github.com/42wim/matterbridge/bridge/config" "github.com/nlopes/slack" ) +func (b *Bslack) getUser(id string) *slack.User { + b.usersMutex.RLock() + defer b.usersMutex.RUnlock() + + return b.users[id] +} + func (b *Bslack) getUsername(id string) string { - for _, u := range b.users { - if u.ID == id { - if u.Profile.DisplayName != "" { - return u.Profile.DisplayName - } - return u.Name + if user := b.getUser(id); user != nil { + if user.Profile.DisplayName != "" { + return user.Profile.DisplayName } + return user.Name } b.Log.Warnf("Could not find user with ID '%s'", id) return "" } -func (b *Bslack) getAvatar(userid string) string { - for _, u := range b.users { - if userid == u.ID { - return u.Profile.Image48 - } +func (b *Bslack) getAvatar(id string) string { + if user := b.getUser(id); user != nil { + return user.Profile.Image48 } return "" } @@ -142,6 +146,66 @@ func (b *Bslack) populateChannels() { b.refreshInProgress = false } +// populateReceivedMessage shapes the initial Matterbridge message that we will forward to the +// router before we apply message-dependent modifications. +func (b *Bslack) populateReceivedMessage(ev *slack.MessageEvent) (*config.Message, error) { + // Use our own func because rtm.GetChannelInfo doesn't work for private channels. + channel, err := b.getChannelByID(ev.Channel) + if err != nil { + return nil, err + } + + rmsg := &config.Message{ + Text: ev.Text, + Channel: channel.Name, + Account: b.Account, + ID: "slack " + ev.Timestamp, + Extra: make(map[string][]interface{}), + ParentID: ev.ThreadTimestamp, + } + if b.useChannelID { + rmsg.Channel = "ID:" + channel.ID + } + + if err = b.populateMessageWithUserInfo(ev, rmsg); err != nil { + return nil, err + } + return rmsg, err +} + +func (b *Bslack) populateMessageWithUserInfo(ev *slack.MessageEvent, rmsg *config.Message) error { + if ev.SubType == sMessageDeleted || ev.SubType == sFileComment { + return nil + } + + if ev.BotID != "" && b.GetString(outgoingWebhookConfig) == "" { + bot, err := b.rtm.GetBotInfo(ev.BotID) + if err != nil { + return err + } + if bot.Name != "" && bot.Name != "Slack API Tester" { + rmsg.Username = bot.Name + if ev.Username != "" { + rmsg.Username = ev.Username + } + rmsg.UserID = bot.ID + } + } + + if ev.User != "" { + user := b.getUser(ev.User) + if user == nil { + return fmt.Errorf("could not find information for user with id %s", ev.User) + } + rmsg.UserID = user.ID + rmsg.Username = user.Name + if user.Profile.DisplayName != "" { + rmsg.Username = user.Profile.DisplayName + } + } + return nil +} + var ( mentionRE = regexp.MustCompile(`<@([a-zA-Z0-9]+)>`) channelRE = regexp.MustCompile(`<#[a-zA-Z0-9]+\|(.+?)>`) diff --git a/bridge/slack/slack.go b/bridge/slack/slack.go index c1386a46..8c96837e 100644 --- a/bridge/slack/slack.go +++ b/bridge/slack/slack.go @@ -1,6 +1,7 @@ package bslack import ( + "bytes" "errors" "fmt" "strings" @@ -45,6 +46,8 @@ type Bslack struct { const ( sChannelJoin = "channel_join" sChannelLeave = "channel_leave" + sChannelJoined = "channel_joined" + sMemberJoined = "member_joined_channel" sMessageDeleted = "message_deleted" sSlackAttachment = "slack_attachment" sPinnedItem = "pinned_item" @@ -56,6 +59,7 @@ const ( sUserTyping = "user_typing" sLatencyReport = "latency_report" sSystemUser = "system" + sSlackBotUser = "slackbot" tokenConfig = "Token" incomingWebhookConfig = "WebhookBindAddress" @@ -295,7 +299,7 @@ func (b *Bslack) sendRTM(msg config.Message) (string, error) { messageParameters := b.prepareMessageParameters(&msg) - // Upload a file if it exists + // Upload a file if it exists. if msg.Extra != nil { for _, rmsg := range helper.HandleExtra(&msg, b.General) { _, _, err = b.rtm.PostMessage(channelInfo.ID, rmsg.Username+rmsg.Text, *messageParameters) @@ -315,6 +319,39 @@ func (b *Bslack) sendRTM(msg config.Message) (string, error) { return "slack " + id, nil } +// handleUploadFile handles native upload of files +func (b *Bslack) handleUploadFile(msg *config.Message, channelID string) { + for _, f := range msg.Extra["file"] { + fi := f.(config.FileInfo) + if msg.Text == fi.Comment { + msg.Text = "" + } + // Because the result of the UploadFile is slower than the MessageEvent from slack + // we can't match on the file ID yet, so we have to match on the filename too. + ts := time.Now() + b.Log.Debugf("Adding file %s to cache at %s with timestamp", fi.Name, ts.String()) + if !b.cache.Add("filename"+fi.Name, ts) { + b.Log.Warnf("Failed to add file %s to cache at %s with timestamp", fi.Name, ts.String()) + } + res, err := b.sc.UploadFile(slack.FileUploadParameters{ + Reader: bytes.NewReader(*fi.Data), + Filename: fi.Name, + Channels: []string{channelID}, + InitialComment: fi.Comment, + }) + if err != nil { + b.Log.Errorf("uploadfile %#v", err) + return + } + if res.ID != "" { + b.Log.Debugf("Adding file ID %s to cache with timestamp %s", res.ID, ts.String()) + if !b.cache.Add("file"+res.ID, ts) { + b.Log.Warnf("Failed to add file ID %s to cache with timestamp %s", res.ID, ts.String()) + } + } + } +} + func (b *Bslack) prepareMessageParameters(msg *config.Message) *slack.PostMessageParameters { params := slack.NewPostMessageParameters() if b.GetBool(useNickPrefixConfig) {