[mattermost-communityGH-84] Do not rewrite messages from bots (matter…

…most-community#98)
mo2menelzeiny · Apr 4, 2020 · ba9e039 · ba9e039
1 parent 4aa0fa5
commit ba9e039
Show file tree

Hide file tree

Showing 3 changed files with 177 additions and 1 deletion.
diff --git a/server/autolink/autolink_test.go b/server/autolink/autolink_test.go
@@ -25,6 +25,12 @@ func setupTestPlugin(t *testing.T, l autolink.Autolink) *autolinkplugin.Plugin {
 		TeamId: "theteam_id0123456789012345",
 		Name:   "thechannel_name",
 	}, (*model.AppError)(nil))
+
+	testUser := model.User{
+		IsBot: false,
+	}
+	api.On("GetUser", mock.AnythingOfType("string")).Return(&testUser, nil)
+
 	p.SetAPI(api)
 
 	err := l.Compile()

diff --git a/server/autolinkplugin/plugin.go b/server/autolinkplugin/plugin.go
@@ -78,6 +78,16 @@ func contains(team string, channel string, list []string) bool {
 	return false
 }
 
+func (p *Plugin) isBotUser(userId string) (bool, *model.AppError) {
+	user, appErr := p.API.GetUser(userId)
+	if appErr != nil {
+		p.API.LogError("failed to check if message for rewriting was send by a bot", "error", appErr)
+		return false, appErr
+	}
+
+	return user.IsBot, nil
+}
+
 func (p *Plugin) ProcessPost(c *plugin.Context, post *model.Post) (*model.Post, string) {
 	conf := p.getConfig()
 	postText := post.Message
@@ -150,7 +160,23 @@ func (p *Plugin) ProcessPost(c *plugin.Context, post *model.Post) (*model.Post,
 
 		return true
 	})
-	post.Message = postText
+	if post.Message != postText {
+		isBot, appErr := p.isBotUser(post.UserId)
+		if appErr != nil {
+			// NOTE: Not sure how we want to handle errors here, we can either:
+			// * assume that occasional rewrites of Bot messges are ok
+			// * assume that occasional not rewriting of all messages is ok
+			// Let's assume for now that former is a lesser evil and carry on.
+		} else if isBot {
+			// We intentionally use a single if/else block so that the code is
+			// more readable and does not relly on hidden side effect of
+			// isBot==false when appErr!=nil.
+			p.API.LogDebug("not rewriting message from bot", "userId", post.UserId)
+			return nil, ""
+		}
+
+		post.Message = postText
+	}
 
 	post.Hashtags, _ = model.ParseHashtags(post.Message)
 

diff --git a/server/autolinkplugin/plugin_test.go b/server/autolinkplugin/plugin_test.go
@@ -47,6 +47,11 @@ func TestPlugin(t *testing.T) {
 	api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil)
 	api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil)
 
+	testUser := model.User{
+		IsBot: false,
+	}
+	api.On("GetUser", mock.AnythingOfType("string")).Return(&testUser, nil)
+
 	p := New()
 	p.SetAPI(api)
 	p.OnConfigurationChange()
@@ -325,6 +330,11 @@ func TestSpecialCases(t *testing.T) {
 	api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil)
 	api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil)
 
+	testUser := model.User{
+		IsBot: false,
+	}
+	api.On("GetUser", mock.AnythingOfType("string")).Return(&testUser, nil)
+
 	p := New()
 	p.SetAPI(api)
 	p.OnConfigurationChange()
@@ -498,6 +508,135 @@ func TestSpecialCases(t *testing.T) {
 	}
 }
 
+func TestBotMessagesAreRewritenWhenGetUserFails(t *testing.T) {
+	conf := Config{
+		Links: []autolink.Autolink{
+			autolink.Autolink{
+				Pattern:  "(Mattermost)",
+				Template: "[Mattermost](https://mattermost.com)",
+			},
+		},
+	}
+
+	testChannel := model.Channel{
+		Name: "TestChanel",
+	}
+
+	testTeam := model.Team{
+		Name: "TestTeam",
+	}
+
+	api := &plugintest.API{}
+
+	api.On("LoadPluginConfiguration", mock.AnythingOfType("*autolinkplugin.Config")).Return(func(dest interface{}) error {
+		*dest.(*Config) = conf
+		return nil
+	}).Once()
+	api.On("UnregisterCommand", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return((*model.AppError)(nil)).Once()
+
+	api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil).Once()
+	api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil).Once()
+	api.On("LogError", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("*model.AppError"))
+
+	err := &model.AppError{
+		Message: "foo error!",
+	}
+	api.On("GetUser", mock.AnythingOfType("string")).Return(nil, err).Once()
+
+	p := New()
+	p.SetAPI(api)
+	p.OnConfigurationChange()
+
+	post := &model.Post{Message: "Welcome to Mattermost!"}
+	rpost, _ := p.MessageWillBePosted(&plugin.Context{}, post)
+
+	assert.Equal(t, "Welcome to [Mattermost](https://mattermost.com)!", rpost.Message)
+}
+
+func TestGetUserApiCallIsNotExecutedWhenThereAreNoChanges(t *testing.T) {
+	conf := Config{
+		Links: []autolink.Autolink{
+			autolink.Autolink{
+				Pattern:  "(Mattermost)",
+				Template: "[Mattermost](https://mattermost.com)",
+			},
+		},
+	}
+
+	testChannel := model.Channel{
+		Name: "TestChanel",
+	}
+
+	testTeam := model.Team{
+		Name: "TestTeam",
+	}
+
+	api := &plugintest.API{}
+
+	api.On("LoadPluginConfiguration", mock.AnythingOfType("*autolinkplugin.Config")).Return(func(dest interface{}) error {
+		*dest.(*Config) = conf
+		return nil
+	}).Once()
+	api.On("UnregisterCommand", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return((*model.AppError)(nil)).Once()
+
+	api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil).Once()
+	api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil).Once()
+	api.On("LogDebug", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("string"))
+
+	p := New()
+	p.SetAPI(api)
+	p.OnConfigurationChange()
+
+	post := &model.Post{Message: "Welcome to FooBarism!"}
+	rpost, _ := p.MessageWillBePosted(&plugin.Context{}, post)
+
+	assert.Equal(t, "Welcome to FooBarism!", rpost.Message)
+}
+
+func TestBotMessagesAreNotRewriten(t *testing.T) {
+	conf := Config{
+		Links: []autolink.Autolink{
+			autolink.Autolink{
+				Pattern:  "(Mattermost)",
+				Template: "[Mattermost](https://mattermost.com)",
+			},
+		},
+	}
+
+	testChannel := model.Channel{
+		Name: "TestChanel",
+	}
+
+	testTeam := model.Team{
+		Name: "TestTeam",
+	}
+
+	api := &plugintest.API{}
+
+	api.On("LoadPluginConfiguration", mock.AnythingOfType("*autolinkplugin.Config")).Return(func(dest interface{}) error {
+		*dest.(*Config) = conf
+		return nil
+	}).Once()
+	api.On("UnregisterCommand", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return((*model.AppError)(nil)).Once()
+
+	api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil).Once()
+	api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil).Once()
+	testUser := model.User{
+		IsBot: true,
+	}
+	api.On("GetUser", mock.AnythingOfType("string")).Return(&testUser, nil).Once()
+	api.On("LogDebug", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("string"))
+
+	p := New()
+	p.SetAPI(api)
+	p.OnConfigurationChange()
+
+	post := &model.Post{Message: "Welcome to Mattermost!"}
+	rpost, _ := p.MessageWillBePosted(&plugin.Context{}, post)
+
+	assert.Nil(t, rpost)
+}
+
 func TestHashtags(t *testing.T) {
 	conf := Config{
 		Links: []autolink.Autolink{
@@ -531,6 +670,11 @@ func TestHashtags(t *testing.T) {
 	api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil)
 	api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil)
 
+	testUser := model.User{
+		IsBot: false,
+	}
+	api.On("GetUser", mock.AnythingOfType("string")).Return(&testUser, nil)
+
 	p := New()
 	p.SetAPI(api)
 	p.OnConfigurationChange()