Refactor and clean-up handlers. (slack) (#533)

1 files changed, 128 insertions, 193 deletions

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,249 +97,177 @@ func (b *Bslack) handleMatterHook(messages chan *config.Message) {
 	}
 }
 
-var commentRE = regexp.MustCompile(`.*?commented: (.*)`)
-
-// 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]
-	}
-	err := helper.HandleDownloadSize(b.Log, rmsg, file.Name, int64(file.Size), b.General)
-	if err != nil {
-		return err
+// 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
 	}
-	// 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)
+
+	// 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
 	}
-	// add the downloaded data to the message
-	helper.HandleDownloadData(b.Log, rmsg, file.Name, comment, file.URLPrivateDownload, data, b.General)
-	return 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
-		*/
-		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)
-		}
+	// 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
 	}
+	return false
 }
 
-// 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,
-	}
-
-	if b.useChannelID {
-		rmsg.Channel = "ID:" + channelInfo.ID
+	// Handle some message types early.
+	if b.handleStatusEvent(ev, rmsg) {
+		return rmsg, nil
 	}
 
-	// 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
-		}
-	}
-
-	// See if we have some text in the attachments
-	if rmsg.Text == "" {
-		for _, attach := range ev.Attachments {
-			if attach.Text != "" {
-				if attach.Title != "" {
-					rmsg.Text = attach.Title + "\n"
-				}
-				rmsg.Text += attach.Text
-			} else {
-				rmsg.Text = attach.Fallback
-			}
+	// 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)
 		}
 	}
 
-	// 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
-		}
+	b.handleAttachments(ev, rmsg)
 
-		// 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
-			}
+	// 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
+}
 
-	// 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 {
+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
-	}
-
-	// 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 {
+	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
+}
 
-	// topic change event
-	if ev.SubType == sChannelTopic || ev.SubType == sChannelPurpose {
-		rmsg.Event = config.EVENT_TOPIC_CHANGE
+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
 	}
 
-	// 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)")
+	// See if we have some text in the attachments.
+	if rmsg.Text == "" {
+		for _, attach := range ev.Attachments {
+			if attach.Text != "" {
+				if attach.Title != "" {
+					rmsg.Text = attach.Title + "\n"
+				}
+				rmsg.Text += attach.Text
+			} else {
+				rmsg.Text = attach.Fallback
+			}
 		}
-		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: (.*)`)
 
-	// ignore pinned items
-	if ev.SubType == sPinnedItem || ev.SubType == sUnpinnedItem {
-		return true
+// handleDownloadFile handles file download
+func (b *Bslack) handleDownloadFile(rmsg *config.Message, file *slack.File) error {
+	if b.fileIsAvailable(file) {
+		return nil
 	}
 
-	// do not send messages from ourself
-	if b.GetString(outgoingWebhookConfig) == "" && b.GetString(incomingWebhookConfig) == "" && ev.Username == b.si.User.Name {
-		return true
+	// 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
 	}
 
-	// skip messages we made ourselves
-	if len(ev.Attachments) > 0 {
-		if ev.Attachments[0].CallbackID == "matterbridge_"+b.uuid {
-			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)
 	}
 
-	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
-		}
+	// 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
+}
 
-	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())
+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
 	}
-
 	return false
 }