diff --git a/model/bot.go b/model/bot.go index 18d64fec53..362c439ec7 100644 --- a/model/bot.go +++ b/model/bot.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "net/http" + "strings" "unicode/utf8" ) @@ -217,3 +218,15 @@ func (l *BotList) Etag() string { func MakeBotNotFoundError(userId string) *AppError { return NewAppError("SqlBotStore.Get", "store.sql_bot.get.missing.app_error", map[string]interface{}{"user_id": userId}, "", http.StatusNotFound) } + +func IsBotDMChannel(channel *Channel, botUserID string) bool { + if channel.Type != CHANNEL_DIRECT { + return false + } + + if !strings.HasPrefix(channel.Name, botUserID+"__") && !strings.HasSuffix(channel.Name, "__"+botUserID) { + return false + } + + return true +} diff --git a/model/bot_test.go b/model/bot_test.go index 9b1e8a3cb2..3a7a0db9fc 100644 --- a/model/bot_test.go +++ b/model/bot_test.go @@ -683,3 +683,45 @@ func TestBotListEtag(t *testing.T) { }) } } + +func TestIsBotChannel(t *testing.T) { + for _, test := range []struct { + Name string + Channel *Channel + Expected bool + }{ + { + Name: "not a direct channel", + Channel: &Channel{Type: CHANNEL_OPEN}, + Expected: false, + }, + { + Name: "a direct channel with another user", + Channel: &Channel{ + Name: "user1__user2", + Type: CHANNEL_DIRECT, + }, + Expected: false, + }, + { + Name: "a direct channel with the name containing the bot's ID first", + Channel: &Channel{ + Name: "botUserID__user2", + Type: CHANNEL_DIRECT, + }, + Expected: true, + }, + { + Name: "a direct channel with the name containing the bot's ID second", + Channel: &Channel{ + Name: "user1__botUserID", + Type: CHANNEL_DIRECT, + }, + Expected: true, + }, + } { + t.Run(test.Name, func(t *testing.T) { + assert.Equal(t, test.Expected, IsBotDMChannel(test.Channel, "botUserID")) + }) + } +} diff --git a/plugin/helpers.go b/plugin/helpers.go index 5fa58492ef..31cc728d1c 100644 --- a/plugin/helpers.go +++ b/plugin/helpers.go @@ -57,6 +57,18 @@ type Helpers interface { // // Minimum server version: 5.6 KVSetWithExpiryJSON(key string, value interface{}, expireInSeconds int64) error + + // ShouldProcessMessage returns if the message should be processed by a message hook. + // + // Use this method to avoid processing unnecessary messages in a MessageHasBeenPosted + // or MessageWillBePosted hook, and indeed in some cases avoid an infinite loop between + // two automated bots or plugins. + // + // The behaviour is customizable using the given options, since plugin needs may vary. + // By default, system messages and messages from bots will be skipped. + // + // Minimum server version: 5.2 + ShouldProcessMessage(post *model.Post, options ...ShouldProcessMessageOption) (bool, error) } // HelpersImpl implements the helpers interface with an API that retrieves data on behalf of the plugin. diff --git a/plugin/helpers_bots.go b/plugin/helpers_bots.go index 3d3492a399..142ccc49b3 100644 --- a/plugin/helpers_bots.go +++ b/plugin/helpers_bots.go @@ -32,19 +32,6 @@ func IconImagePath(path string) EnsureBotOption { } } -func (p *HelpersImpl) readFile(path string) ([]byte, error) { - bundlePath, err := p.API.GetBundlePath() - if err != nil { - return nil, errors.Wrap(err, "failed to get bundle path") - } - - imageBytes, err := ioutil.ReadFile(filepath.Join(bundlePath, path)) - if err != nil { - return nil, errors.Wrap(err, "failed to read image") - } - return imageBytes, nil -} - // EnsureBot implements Helpers.EnsureBot func (p *HelpersImpl) EnsureBot(bot *model.Bot, options ...EnsureBotOption) (retBotID string, retErr error) { err := p.ensureServerVersion("5.10.0") @@ -74,6 +61,129 @@ func (p *HelpersImpl) EnsureBot(bot *model.Bot, options ...EnsureBotOption) (ret return botID, nil } +type ShouldProcessMessageOption func(*shouldProcessMessageOptions) + +type shouldProcessMessageOptions struct { + AllowSystemMessages bool + AllowBots bool + FilterChannelIDs []string + FilterUserIDs []string + OnlyBotDMs bool +} + +// AllowSystemMessages configures a call to ShouldProcessMessage to return true for system messages. +// +// As it is typically desirable only to consume messages from users of the system, ShouldProcessMessage ignores system messages by default. +func AllowSystemMessages() ShouldProcessMessageOption { + return func(options *shouldProcessMessageOptions) { + options.AllowSystemMessages = true + } +} + +// AllowBots configures a call to ShouldProcessMessage to return true for bot posts. +// +// As it is typically desirable only to consume messages from human users of the system, ShouldProcessMessage ignores bot messages by default. When allowing bots, take care to avoid a loop where two plugins respond to each others posts repeatedly. +func AllowBots() ShouldProcessMessageOption { + return func(options *shouldProcessMessageOptions) { + options.AllowBots = true + } +} + +// FilterChannelIDs configures a call to ShouldProcessMessage to return true only for the given channels. +// +// By default, posts from all channels are allowed to be processed. +func FilterChannelIDs(filterChannelIDs []string) ShouldProcessMessageOption { + return func(options *shouldProcessMessageOptions) { + options.FilterChannelIDs = filterChannelIDs + } +} + +// FilterUserIDs configures a call to ShouldProcessMessage to return true only for the given users. +// +// By default, posts from all non-bot users are allowed. +func FilterUserIDs(filterUserIDs []string) ShouldProcessMessageOption { + return func(options *shouldProcessMessageOptions) { + options.FilterUserIDs = filterUserIDs + } +} + +// OnlyBotDMs configures a call to ShouldProcessMessage to return true only for direct messages sent to the bot created by EnsureBot. +// +// By default, posts from all channels are allowed. +func OnlyBotDMs() ShouldProcessMessageOption { + return func(options *shouldProcessMessageOptions) { + options.OnlyBotDMs = true + } +} + +// ShouldProcessMessage implements Helpers.ShouldProcessMessage +func (p *HelpersImpl) ShouldProcessMessage(post *model.Post, options ...ShouldProcessMessageOption) (bool, error) { + messageProcessOptions := &shouldProcessMessageOptions{} + for _, option := range options { + option(messageProcessOptions) + } + + botIdBytes, kvGetErr := p.API.KVGet(BOT_USER_KEY) + if kvGetErr != nil { + return false, errors.Wrap(kvGetErr, "failed to get bot") + } + + if botIdBytes != nil { + if post.UserId == string(botIdBytes) { + return false, nil + } + } + + if post.IsSystemMessage() && !messageProcessOptions.AllowSystemMessages { + return false, nil + } + + if !messageProcessOptions.AllowBots { + user, appErr := p.API.GetUser(post.UserId) + if appErr != nil { + return false, errors.Wrap(appErr, "unable to get user") + } + + if user.IsBot { + return false, nil + } + } + + if len(messageProcessOptions.FilterChannelIDs) != 0 && !utils.StringInSlice(post.ChannelId, messageProcessOptions.FilterChannelIDs) { + return false, nil + } + + if len(messageProcessOptions.FilterUserIDs) != 0 && !utils.StringInSlice(post.UserId, messageProcessOptions.FilterUserIDs) { + return false, nil + } + + if botIdBytes != nil && messageProcessOptions.OnlyBotDMs { + channel, appErr := p.API.GetChannel(post.ChannelId) + if appErr != nil { + return false, errors.Wrap(appErr, "unable to get channel") + } + + if !model.IsBotDMChannel(channel, string(botIdBytes)) { + return false, nil + } + } + + return true, nil +} + +func (p *HelpersImpl) readFile(path string) ([]byte, error) { + bundlePath, err := p.API.GetBundlePath() + if err != nil { + return nil, errors.Wrap(err, "failed to get bundle path") + } + + imageBytes, err := ioutil.ReadFile(filepath.Join(bundlePath, path)) + if err != nil { + return nil, errors.Wrap(err, "failed to read image") + } + return imageBytes, nil +} + func (p *HelpersImpl) ensureBot(bot *model.Bot) (retBotID string, retErr error) { // Must provide a bot with a username if bot == nil || len(bot.Username) < 1 { diff --git a/plugin/helpers_bots_test.go b/plugin/helpers_bots_test.go index b7fb1ac2ee..8960e61efe 100644 --- a/plugin/helpers_bots_test.go +++ b/plugin/helpers_bots_test.go @@ -340,3 +340,111 @@ func TestEnsureBot(t *testing.T) { }) }) } + +func TestShouldProcessMessage(t *testing.T) { + p := &plugin.HelpersImpl{} + expectedBotId := model.NewId() + + setupAPI := func() *plugintest.API { + return &plugintest.API{} + } + + t.Run("should not respond to itself", func(t *testing.T) { + api := setupAPI() + api.On("KVGet", plugin.BOT_USER_KEY).Return([]byte(expectedBotId), nil) + p.API = api + shouldProcessMessage, _ := p.ShouldProcessMessage(&model.Post{Type: model.POST_HEADER_CHANGE, UserId: expectedBotId}, plugin.AllowSystemMessages(), plugin.AllowBots()) + + assert.False(t, shouldProcessMessage) + }) + + t.Run("should not process as the post is generated by system", func(t *testing.T) { + shouldProcessMessage, _ := p.ShouldProcessMessage(&model.Post{Type: model.POST_HEADER_CHANGE}) + + assert.False(t, shouldProcessMessage) + }) + + t.Run("should not process as the post is sent to another channel", func(t *testing.T) { + channelID := "channel-id" + api := setupAPI() + api.On("GetChannel", channelID).Return(&model.Channel{Id: channelID, Type: model.CHANNEL_GROUP}, nil) + p.API = api + api.On("KVGet", plugin.BOT_USER_KEY).Return([]byte(expectedBotId), nil) + + shouldProcessMessage, _ := p.ShouldProcessMessage(&model.Post{ChannelId: channelID}, plugin.AllowSystemMessages(), plugin.AllowBots(), plugin.FilterChannelIDs([]string{"another-channel-id"})) + + assert.False(t, shouldProcessMessage) + }) + + t.Run("should not process as the post is created by bot", func(t *testing.T) { + userID := "user-id" + channelID := "1" + api := setupAPI() + p.API = api + api.On("GetUser", userID).Return(&model.User{IsBot: true}, nil) + api.On("KVGet", plugin.BOT_USER_KEY).Return([]byte(expectedBotId), nil) + + shouldProcessMessage, _ := p.ShouldProcessMessage(&model.Post{UserId: userID, ChannelId: channelID}, + plugin.AllowSystemMessages(), plugin.FilterUserIDs([]string{"another-user-id"})) + + assert.False(t, shouldProcessMessage) + }) + + t.Run("should not process the message as the post is not in bot dm channel", func(t *testing.T) { + userID := "user-id" + channelID := "1" + channel := model.Channel{ + Name: "user1__" + expectedBotId, + Type: model.CHANNEL_OPEN, + } + api := setupAPI() + api.On("GetChannel", channelID).Return(&channel, nil) + api.On("KVGet", plugin.BOT_USER_KEY).Return([]byte(expectedBotId), nil) + p.API = api + + shouldProcessMessage, _ := p.ShouldProcessMessage(&model.Post{UserId: userID, ChannelId: channelID}, plugin.AllowSystemMessages(), plugin.AllowBots(), plugin.OnlyBotDMs()) + + assert.False(t, shouldProcessMessage) + }) + + t.Run("should process the message", func(t *testing.T) { + channelID := "1" + api := setupAPI() + api.On("KVGet", plugin.BOT_USER_KEY).Return([]byte(expectedBotId), nil) + p.API = api + + shouldProcessMessage, _ := p.ShouldProcessMessage(&model.Post{UserId: "1", Type: model.POST_HEADER_CHANGE, ChannelId: channelID}, + plugin.AllowSystemMessages(), plugin.FilterChannelIDs([]string{channelID}), plugin.AllowBots(), plugin.FilterUserIDs([]string{"1"})) + + assert.True(t, shouldProcessMessage) + }) + + t.Run("should process the message for plugin without a bot", func(t *testing.T) { + channelID := "1" + api := setupAPI() + api.On("KVGet", plugin.BOT_USER_KEY).Return(nil, nil) + p.API = api + + shouldProcessMessage, _ := p.ShouldProcessMessage(&model.Post{UserId: "1", Type: model.POST_HEADER_CHANGE, ChannelId: channelID}, + plugin.AllowSystemMessages(), plugin.FilterChannelIDs([]string{channelID}), plugin.AllowBots(), plugin.FilterUserIDs([]string{"1"})) + + assert.True(t, shouldProcessMessage) + }) + + t.Run("should process the message when filter channel and filter users list is empty", func(t *testing.T) { + channelID := "1" + api := setupAPI() + channel := model.Channel{ + Name: "user1__" + expectedBotId, + Type: model.CHANNEL_DIRECT, + } + api.On("GetChannel", channelID).Return(&channel, nil) + api.On("KVGet", plugin.BOT_USER_KEY).Return([]byte(expectedBotId), nil) + p.API = api + + shouldProcessMessage, _ := p.ShouldProcessMessage(&model.Post{UserId: "1", Type: model.POST_HEADER_CHANGE, ChannelId: channelID}, + plugin.AllowSystemMessages(), plugin.AllowBots()) + + assert.True(t, shouldProcessMessage) + }) +} diff --git a/plugin/plugintest/helpers.go b/plugin/plugintest/helpers.go index 8bc81da9a3..e8839b8185 100644 --- a/plugin/plugintest/helpers.go +++ b/plugin/plugintest/helpers.go @@ -133,3 +133,31 @@ func (_m *Helpers) KVSetWithExpiryJSON(key string, value interface{}, expireInSe return r0 } + +// ShouldProcessMessage provides a mock function with given fields: post, options +func (_m *Helpers) ShouldProcessMessage(post *model.Post, options ...plugin.ShouldProcessMessageOption) (bool, error) { + _va := make([]interface{}, len(options)) + for _i := range options { + _va[_i] = options[_i] + } + var _ca []interface{} + _ca = append(_ca, post) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + var r0 bool + if rf, ok := ret.Get(0).(func(*model.Post, ...plugin.ShouldProcessMessageOption) bool); ok { + r0 = rf(post, options...) + } else { + r0 = ret.Get(0).(bool) + } + + var r1 error + if rf, ok := ret.Get(1).(func(*model.Post, ...plugin.ShouldProcessMessageOption) error); ok { + r1 = rf(post, options...) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +}