MM-11575: change plugin nil semantics (#9212)

* change MessageWillBePosted nil return semantics

* change FileWillBeUploaded nil return semantics

* use LogDebug to verify plugin inputs vs. the confusing Delete(User|Team)

5 files changed, 466 insertions, 165 deletions

diff --git a/app/file.go b/app/file.go
index 7a642a956..d2a145c81 100644
--- a/app/file.go
+++ b/app/file.go
@@ -435,20 +435,27 @@ func (a *App) DoUploadFileExpectModification(now time.Time, rawTeamId string, ra
 	}
 
 	if a.PluginsReady() {
+		var rejectionError *model.AppError
 		pluginContext := &plugin.Context{}
-		var rejectionReason string
 		a.Plugins.RunMultiPluginHook(func(hooks plugin.Hooks) bool {
 			var newBytes bytes.Buffer
-			info, rejectionReason = hooks.FileWillBeUploaded(pluginContext, info, bytes.NewReader(data), &newBytes)
-			rejected := info == nil
-			if !rejected && newBytes.Len() != 0 {
+			replacementInfo, rejectionReason := hooks.FileWillBeUploaded(pluginContext, info, bytes.NewReader(data), &newBytes)
+			if rejectionReason != "" {
+				rejectionError = model.NewAppError("DoUploadFile", "File rejected by plugin. "+rejectionReason, nil, "", http.StatusBadRequest)
+				return false
+			}
+			if replacementInfo != nil {
+				info = replacementInfo
+			}
+			if newBytes.Len() != 0 {
 				data = newBytes.Bytes()
 				info.Size = int64(len(data))
 			}
-			return !rejected
+
+			return true
 		}, plugin.FileWillBeUploadedId)
-		if info == nil {
-			return nil, data, model.NewAppError("DoUploadFile", "File rejected by plugin. "+rejectionReason, nil, "", http.StatusBadRequest)
+		if rejectionError != nil {
+			return nil, data, rejectionError
 		}
 	}
 
diff --git a/app/plugin_deadlock_test.go b/app/plugin_deadlock_test.go
index 381206943..1dcaaa228 100644
--- a/app/plugin_deadlock_test.go
+++ b/app/plugin_deadlock_test.go
@@ -45,7 +45,7 @@ func TestPluginDeadlock(t *testing.T) {
 
 			func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
 				if _, from_plugin := post.Props["from_plugin"]; from_plugin {
-					return post, ""
+					return nil, ""
 				}
 
 				p.API.CreatePost(&model.Post{
@@ -57,7 +57,7 @@ func TestPluginDeadlock(t *testing.T) {
 					},
 				})
 
-				return post, ""
+				return nil, ""
 			}
 
 			func main() {
@@ -120,7 +120,7 @@ func TestPluginDeadlock(t *testing.T) {
 
 			func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
 				if _, from_plugin := post.Props["from_plugin"]; from_plugin {
-					return post, ""
+					return nil, ""
 				}
 
 				p.API.CreatePost(&model.Post{
@@ -132,7 +132,7 @@ func TestPluginDeadlock(t *testing.T) {
 					},
 				})
 
-				return post, ""
+				return nil, ""
 			}
 
 			func main() {
diff --git a/app/plugin_hooks_test.go b/app/plugin_hooks_test.go
index b85e3b9a0..f098374ad 100644
--- a/app/plugin_hooks_test.go
+++ b/app/plugin_hooks_test.go
@@ -56,113 +56,238 @@ func SetAppEnvironmentWithPlugins(t *testing.T, pluginCode []string, app *App, a
 }
 
 func TestHookMessageWillBePosted(t *testing.T) {
-	th := Setup().InitBasic()
-	defer th.TearDown()
+	t.Run("rejected", func(t *testing.T) {
+		th := Setup().InitBasic()
+		defer th.TearDown()
 
-	SetAppEnvironmentWithPlugins(t,
-		[]string{
+		SetAppEnvironmentWithPlugins(t, []string{
 			`
-		package main
+			package main
 
-		import (
-			"github.com/mattermost/mattermost-server/plugin"
-			"github.com/mattermost/mattermost-server/model"
-		)
+			import (
+				"github.com/mattermost/mattermost-server/plugin"
+				"github.com/mattermost/mattermost-server/model"
+			)
 
-		type MyPlugin struct {
-			plugin.MattermostPlugin
-		}
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+			}
 
-		func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
-			post.Message = post.Message + "fromplugin"
-			return post, ""
-		}
+			func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
+				return nil, "rejected"
+			}
 
-		func main() {
-			plugin.ClientMain(&MyPlugin{})
-		}
-	`}, th.App, th.App.NewPluginAPI)
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+			`,
+		}, th.App, th.App.NewPluginAPI)
 
-	post := &model.Post{
-		UserId:    th.BasicUser.Id,
-		ChannelId: th.BasicChannel.Id,
-		Message:   "message_",
-		CreateAt:  model.GetMillis() - 10000,
-	}
-	post, err := th.App.CreatePost(post, th.BasicChannel, false)
-	if err != nil {
-		t.Fatal(err)
-	}
-	assert.Equal(t, "message_fromplugin", post.Message)
-	if result := <-th.App.Srv.Store.Post().GetSingle(post.Id); result.Err != nil {
-		t.Fatal(err)
-	} else {
-		assert.Equal(t, "message_fromplugin", result.Data.(*model.Post).Message)
-	}
-}
+		post := &model.Post{
+			UserId:    th.BasicUser.Id,
+			ChannelId: th.BasicChannel.Id,
+			Message:   "message_",
+			CreateAt:  model.GetMillis() - 10000,
+		}
+		post, err := th.App.CreatePost(post, th.BasicChannel, false)
+		if assert.NotNil(t, err) {
+			assert.Equal(t, "Post rejected by plugin. rejected", err.Message)
+		}
+	})
 
-func TestHookMessageWillBePostedMultiple(t *testing.T) {
-	th := Setup().InitBasic()
-	defer th.TearDown()
+	t.Run("rejected, returned post ignored", func(t *testing.T) {
+		th := Setup().InitBasic()
+		defer th.TearDown()
 
-	SetAppEnvironmentWithPlugins(t,
-		[]string{
+		SetAppEnvironmentWithPlugins(t, []string{
 			`
-		package main
-
-		import (
-			"github.com/mattermost/mattermost-server/plugin"
-			"github.com/mattermost/mattermost-server/model"
-		)
+			package main
+
+			import (
+				"github.com/mattermost/mattermost-server/plugin"
+				"github.com/mattermost/mattermost-server/model"
+			)
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+			}
+
+			func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
+				post.Message = "ignored"
+				return post, "rejected"
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+			`,
+		}, th.App, th.App.NewPluginAPI)
 
-		type MyPlugin struct {
-			plugin.MattermostPlugin
+		post := &model.Post{
+			UserId:    th.BasicUser.Id,
+			ChannelId: th.BasicChannel.Id,
+			Message:   "message_",
+			CreateAt:  model.GetMillis() - 10000,
 		}
-
-		func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
-			
-			post.Message = "prefix_" + post.Message
-			return post, ""
+		post, err := th.App.CreatePost(post, th.BasicChannel, false)
+		if assert.NotNil(t, err) {
+			assert.Equal(t, "Post rejected by plugin. rejected", err.Message)
 		}
+	})
 
-		func main() {
-			plugin.ClientMain(&MyPlugin{})
-		}
-	`,
+	t.Run("allowed", func(t *testing.T) {
+		th := Setup().InitBasic()
+		defer th.TearDown()
+
+		SetAppEnvironmentWithPlugins(t, []string{
 			`
-		package main
+			package main
 
-		import (
-			"github.com/mattermost/mattermost-server/plugin"
-			"github.com/mattermost/mattermost-server/model"
-		)
+			import (
+				"github.com/mattermost/mattermost-server/plugin"
+				"github.com/mattermost/mattermost-server/model"
+			)
 
-		type MyPlugin struct {
-			plugin.MattermostPlugin
-		}
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+			}
 
-		func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
-			post.Message = post.Message + "_suffix"
-			return post, ""
+			func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
+				return nil, ""
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+			`,
+		}, th.App, th.App.NewPluginAPI)
+
+		post := &model.Post{
+			UserId:    th.BasicUser.Id,
+			ChannelId: th.BasicChannel.Id,
+			Message:   "message",
+			CreateAt:  model.GetMillis() - 10000,
+		}
+		post, err := th.App.CreatePost(post, th.BasicChannel, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+		assert.Equal(t, "message", post.Message)
+		if result := <-th.App.Srv.Store.Post().GetSingle(post.Id); result.Err != nil {
+			t.Fatal(err)
+		} else {
+			assert.Equal(t, "message", result.Data.(*model.Post).Message)
 		}
+	})
 
-		func main() {
-			plugin.ClientMain(&MyPlugin{})
+	t.Run("updated", func(t *testing.T) {
+		th := Setup().InitBasic()
+		defer th.TearDown()
+
+		SetAppEnvironmentWithPlugins(t, []string{
+			`
+			package main
+
+			import (
+				"github.com/mattermost/mattermost-server/plugin"
+				"github.com/mattermost/mattermost-server/model"
+			)
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+			}
+
+			func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
+				post.Message = post.Message + "_fromplugin"
+				return post, ""
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+			`,
+		}, th.App, th.App.NewPluginAPI)
+
+		post := &model.Post{
+			UserId:    th.BasicUser.Id,
+			ChannelId: th.BasicChannel.Id,
+			Message:   "message",
+			CreateAt:  model.GetMillis() - 10000,
+		}
+		post, err := th.App.CreatePost(post, th.BasicChannel, false)
+		if err != nil {
+			t.Fatal(err)
 		}
-	`,
+		assert.Equal(t, "message_fromplugin", post.Message)
+		if result := <-th.App.Srv.Store.Post().GetSingle(post.Id); result.Err != nil {
+			t.Fatal(err)
+		} else {
+			assert.Equal(t, "message_fromplugin", result.Data.(*model.Post).Message)
+		}
+	})
+
+	t.Run("multiple updated", func(t *testing.T) {
+		th := Setup().InitBasic()
+		defer th.TearDown()
+
+		SetAppEnvironmentWithPlugins(t, []string{
+			`
+			package main
+
+			import (
+				"github.com/mattermost/mattermost-server/plugin"
+				"github.com/mattermost/mattermost-server/model"
+			)
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+			}
+
+			func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
+				
+				post.Message = "prefix_" + post.Message
+				return post, ""
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+			`,
+			`
+			package main
+
+			import (
+				"github.com/mattermost/mattermost-server/plugin"
+				"github.com/mattermost/mattermost-server/model"
+			)
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+			}
+
+			func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
+				post.Message = post.Message + "_suffix"
+				return post, ""
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+			`,
 		}, th.App, th.App.NewPluginAPI)
 
-	post := &model.Post{
-		UserId:    th.BasicUser.Id,
-		ChannelId: th.BasicChannel.Id,
-		Message:   "message",
-		CreateAt:  model.GetMillis() - 10000,
-	}
-	post, err := th.App.CreatePost(post, th.BasicChannel, false)
-	if err != nil {
-		t.Fatal(err)
-	}
-	assert.Equal(t, "prefix_message_suffix", post.Message)
+		post := &model.Post{
+			UserId:    th.BasicUser.Id,
+			ChannelId: th.BasicChannel.Id,
+			Message:   "message",
+			CreateAt:  model.GetMillis() - 10000,
+		}
+		post, err := th.App.CreatePost(post, th.BasicChannel, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+		assert.Equal(t, "prefix_message_suffix", post.Message)
+	})
 }
 
 func TestHookMessageHasBeenPosted(t *testing.T) {
@@ -171,7 +296,7 @@ func TestHookMessageHasBeenPosted(t *testing.T) {
 
 	var mockAPI plugintest.API
 	mockAPI.On("LoadPluginConfiguration", mock.Anything).Return(nil)
-	mockAPI.On("DeleteUser", "message").Return(nil)
+	mockAPI.On("LogDebug", "message").Return(nil)
 
 	SetAppEnvironmentWithPlugins(t,
 		[]string{
@@ -188,7 +313,7 @@ func TestHookMessageHasBeenPosted(t *testing.T) {
 		}
 
 		func (p *MyPlugin) MessageHasBeenPosted(c *plugin.Context, post *model.Post) {
-			p.API.DeleteUser(post.Message)
+			p.API.LogDebug(post.Message)
 		}
 
 		func main() {
@@ -261,8 +386,8 @@ func TestHookMessageHasBeenUpdated(t *testing.T) {
 
 	var mockAPI plugintest.API
 	mockAPI.On("LoadPluginConfiguration", mock.Anything).Return(nil)
-	mockAPI.On("DeleteUser", "message_edited").Return(nil)
-	mockAPI.On("DeleteTeam", "message_").Return(nil)
+	mockAPI.On("LogDebug", "message_edited").Return(nil)
+	mockAPI.On("LogDebug", "message_").Return(nil)
 	SetAppEnvironmentWithPlugins(t,
 		[]string{
 			`
@@ -278,8 +403,8 @@ func TestHookMessageHasBeenUpdated(t *testing.T) {
 		}
 
 		func (p *MyPlugin) MessageHasBeenUpdated(c *plugin.Context, newPost, oldPost *model.Post) {
-			p.API.DeleteUser(newPost.Message)
-			p.API.DeleteTeam(oldPost.Message)
+			p.API.LogDebug(newPost.Message)
+			p.API.LogDebug(oldPost.Message)
 		}
 
 		func main() {
@@ -306,70 +431,224 @@ func TestHookMessageHasBeenUpdated(t *testing.T) {
 }
 
 func TestHookFileWillBeUploaded(t *testing.T) {
-	th := Setup().InitBasic()
-	defer th.TearDown()
-
-	var mockAPI plugintest.API
-	mockAPI.On("LoadPluginConfiguration", mock.Anything).Return(nil)
-	mockAPI.On("DeleteUser", "testhook.txt").Return(nil)
-	mockAPI.On("DeleteTeam", "inputfile").Return(nil)
-	SetAppEnvironmentWithPlugins(t,
-		[]string{
+	t.Run("rejected", func(t *testing.T) {
+		th := Setup().InitBasic()
+		defer th.TearDown()
+
+		var mockAPI plugintest.API
+		mockAPI.On("LoadPluginConfiguration", mock.Anything).Return(nil)
+		mockAPI.On("LogDebug", "testhook.txt").Return(nil)
+		mockAPI.On("LogDebug", "inputfile").Return(nil)
+		SetAppEnvironmentWithPlugins(t, []string{
 			`
-		package main
-
-		import (
-			"io"
-			"bytes"
-			"github.com/mattermost/mattermost-server/plugin"
-			"github.com/mattermost/mattermost-server/model"
+			package main
+
+			import (
+				"io"
+				"github.com/mattermost/mattermost-server/plugin"
+				"github.com/mattermost/mattermost-server/model"
+			)
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+			}
+
+			func (p *MyPlugin) FileWillBeUploaded(c *plugin.Context, info *model.FileInfo, file io.Reader, output io.Writer) (*model.FileInfo, string) {
+				return nil, "rejected"
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+			`,
+		}, th.App, func(*model.Manifest) plugin.API { return &mockAPI })
+
+		_, err := th.App.UploadFiles(
+			"noteam",
+			th.BasicChannel.Id,
+			th.BasicUser.Id,
+			[]io.ReadCloser{ioutil.NopCloser(bytes.NewBufferString("inputfile"))},
+			[]string{"testhook.txt"},
+			[]string{},
+			time.Now(),
 		)
-
-		type MyPlugin struct {
-			plugin.MattermostPlugin
+		if assert.NotNil(t, err) {
+			assert.Equal(t, "File rejected by plugin. rejected", err.Message)
 		}
+	})
 
-		func (p *MyPlugin) FileWillBeUploaded(c *plugin.Context, info *model.FileInfo, file io.Reader, output io.Writer) (*model.FileInfo, string) {
-			p.API.DeleteUser(info.Name)
-			var buf bytes.Buffer
-			buf.ReadFrom(file)
-			p.API.DeleteTeam(buf.String())
+	t.Run("rejected, returned file ignored", func(t *testing.T) {
+		th := Setup().InitBasic()
+		defer th.TearDown()
 
-			outbuf := bytes.NewBufferString("changedtext")
-			io.Copy(output, outbuf)
-			info.Name = "modifiedinfo"
-			return info, ""
+		var mockAPI plugintest.API
+		mockAPI.On("LoadPluginConfiguration", mock.Anything).Return(nil)
+		mockAPI.On("LogDebug", "testhook.txt").Return(nil)
+		mockAPI.On("LogDebug", "inputfile").Return(nil)
+		SetAppEnvironmentWithPlugins(t, []string{
+			`
+			package main
+
+			import (
+				"io"
+				"github.com/mattermost/mattermost-server/plugin"
+				"github.com/mattermost/mattermost-server/model"
+			)
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+			}
+
+			func (p *MyPlugin) FileWillBeUploaded(c *plugin.Context, info *model.FileInfo, file io.Reader, output io.Writer) (*model.FileInfo, string) {
+				output.Write([]byte("ignored"))
+				info.Name = "ignored"
+				return info, "rejected"
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+			`,
+		}, th.App, func(*model.Manifest) plugin.API { return &mockAPI })
+
+		_, err := th.App.UploadFiles(
+			"noteam",
+			th.BasicChannel.Id,
+			th.BasicUser.Id,
+			[]io.ReadCloser{ioutil.NopCloser(bytes.NewBufferString("inputfile"))},
+			[]string{"testhook.txt"},
+			[]string{},
+			time.Now(),
+		)
+		if assert.NotNil(t, err) {
+			assert.Equal(t, "File rejected by plugin. rejected", err.Message)
 		}
+	})
 
-		func main() {
-			plugin.ClientMain(&MyPlugin{})
-		}
-	`}, th.App, func(*model.Manifest) plugin.API { return &mockAPI })
+	t.Run("allowed", func(t *testing.T) {
+		th := Setup().InitBasic()
+		defer th.TearDown()
 
-	response, err := th.App.UploadFiles(
-		"noteam",
-		th.BasicChannel.Id,
-		th.BasicUser.Id,
-		[]io.ReadCloser{ioutil.NopCloser(bytes.NewBufferString("inputfile"))},
-		[]string{"testhook.txt"},
-		[]string{},
-		time.Now(),
-	)
-	assert.Nil(t, err)
-	assert.NotNil(t, response)
-	assert.Equal(t, 1, len(response.FileInfos))
-	fileId := response.FileInfos[0].Id
-
-	fileInfo, err := th.App.GetFileInfo(fileId)
-	assert.Nil(t, err)
-	assert.NotNil(t, fileInfo)
-	assert.Equal(t, "modifiedinfo", fileInfo.Name)
-
-	fileReader, err := th.App.FileReader(fileInfo.Path)
-	assert.Nil(t, err)
-	var resultBuf bytes.Buffer
-	io.Copy(&resultBuf, fileReader)
-	assert.Equal(t, "changedtext", resultBuf.String())
+		var mockAPI plugintest.API
+		mockAPI.On("LoadPluginConfiguration", mock.Anything).Return(nil)
+		mockAPI.On("LogDebug", "testhook.txt").Return(nil)
+		mockAPI.On("LogDebug", "inputfile").Return(nil)
+		SetAppEnvironmentWithPlugins(t, []string{
+			`
+			package main
+
+			import (
+				"io"
+				"github.com/mattermost/mattermost-server/plugin"
+				"github.com/mattermost/mattermost-server/model"
+			)
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+			}
+
+			func (p *MyPlugin) FileWillBeUploaded(c *plugin.Context, info *model.FileInfo, file io.Reader, output io.Writer) (*model.FileInfo, string) {
+				return nil, ""
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+			`,
+		}, th.App, func(*model.Manifest) plugin.API { return &mockAPI })
+
+		response, err := th.App.UploadFiles(
+			"noteam",
+			th.BasicChannel.Id,
+			th.BasicUser.Id,
+			[]io.ReadCloser{ioutil.NopCloser(bytes.NewBufferString("inputfile"))},
+			[]string{"testhook.txt"},
+			[]string{},
+			time.Now(),
+		)
+		assert.Nil(t, err)
+		assert.NotNil(t, response)
+		assert.Equal(t, 1, len(response.FileInfos))
+		fileId := response.FileInfos[0].Id
+
+		fileInfo, err := th.App.GetFileInfo(fileId)
+		assert.Nil(t, err)
+		assert.NotNil(t, fileInfo)
+		assert.Equal(t, "testhook.txt", fileInfo.Name)
+
+		fileReader, err := th.App.FileReader(fileInfo.Path)
+		assert.Nil(t, err)
+		var resultBuf bytes.Buffer
+		io.Copy(&resultBuf, fileReader)
+		assert.Equal(t, "inputfile", resultBuf.String())
+	})
+
+	t.Run("updated", func(t *testing.T) {
+		th := Setup().InitBasic()
+		defer th.TearDown()
+
+		var mockAPI plugintest.API
+		mockAPI.On("LoadPluginConfiguration", mock.Anything).Return(nil)
+		mockAPI.On("LogDebug", "testhook.txt").Return(nil)
+		mockAPI.On("LogDebug", "inputfile").Return(nil)
+		SetAppEnvironmentWithPlugins(t, []string{
+			`
+			package main
+
+			import (
+				"io"
+				"bytes"
+				"github.com/mattermost/mattermost-server/plugin"
+				"github.com/mattermost/mattermost-server/model"
+			)
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+			}
+
+			func (p *MyPlugin) FileWillBeUploaded(c *plugin.Context, info *model.FileInfo, file io.Reader, output io.Writer) (*model.FileInfo, string) {
+				p.API.LogDebug(info.Name)
+				var buf bytes.Buffer
+				buf.ReadFrom(file)
+				p.API.LogDebug(buf.String())
+
+				outbuf := bytes.NewBufferString("changedtext")
+				io.Copy(output, outbuf)
+				info.Name = "modifiedinfo"
+				return info, ""
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+			`,
+		}, th.App, func(*model.Manifest) plugin.API { return &mockAPI })
+
+		response, err := th.App.UploadFiles(
+			"noteam",
+			th.BasicChannel.Id,
+			th.BasicUser.Id,
+			[]io.ReadCloser{ioutil.NopCloser(bytes.NewBufferString("inputfile"))},
+			[]string{"testhook.txt"},
+			[]string{},
+			time.Now(),
+		)
+		assert.Nil(t, err)
+		assert.NotNil(t, response)
+		assert.Equal(t, 1, len(response.FileInfos))
+		fileId := response.FileInfos[0].Id
+
+		fileInfo, err := th.App.GetFileInfo(fileId)
+		assert.Nil(t, err)
+		assert.NotNil(t, fileInfo)
+		assert.Equal(t, "modifiedinfo", fileInfo.Name)
+
+		fileReader, err := th.App.FileReader(fileInfo.Path)
+		assert.Nil(t, err)
+		var resultBuf bytes.Buffer
+		io.Copy(&resultBuf, fileReader)
+		assert.Equal(t, "changedtext", resultBuf.String())
+	})
 }
 
 func TestUserWillLogIn_Blocked(t *testing.T) {
diff --git a/app/post.go b/app/post.go
index 8c44436aa..299c9ce3a 100644
--- a/app/post.go
+++ b/app/post.go
@@ -162,14 +162,22 @@ func (a *App) CreatePost(post *model.Post, channel *model.Channel, triggerWebhoo
 	}
 
 	if a.PluginsReady() {
-		var rejectionReason string
+		var rejectionError *model.AppError
 		pluginContext := &plugin.Context{}
 		a.Plugins.RunMultiPluginHook(func(hooks plugin.Hooks) bool {
-			post, rejectionReason = hooks.MessageWillBePosted(pluginContext, post)
-			return post != nil
+			replacementPost, rejectionReason := hooks.MessageWillBePosted(pluginContext, post)
+			if rejectionReason != "" {
+				rejectionError = model.NewAppError("createPost", "Post rejected by plugin. "+rejectionReason, nil, "", http.StatusBadRequest)
+				return false
+			}
+			if replacementPost != nil {
+				post = replacementPost
+			}
+
+			return true
 		}, plugin.MessageWillBePostedId)
-		if post == nil {
-			return nil, model.NewAppError("createPost", "Post rejected by plugin. "+rejectionReason, nil, "", http.StatusBadRequest)
+		if rejectionError != nil {
+			return nil, rejectionError
 		}
 	}
 
diff --git a/plugin/hooks.go b/plugin/hooks.go
index 363a69fc0..4af177c0d 100644
--- a/plugin/hooks.go
+++ b/plugin/hooks.go
@@ -71,7 +71,10 @@ type Hooks interface {
 
 	// MessageWillBePosted is invoked when a message is posted by a user before it is committed
 	// to the database. If you also want to act on edited posts, see MessageWillBeUpdated.
-	// Return values should be the modified post or nil if rejected and an explanation for the user.
+	//
+	// To reject a post, return an non-empty string describing why the post was rejected.
+	// To modify the post, return the replacement, non-nil *model.Post and an empty string.
+	// To allow the post without modification, return a nil *model.Post and an empty string.
 	//
 	// If you don't need to modify or reject posts, use MessageHasBeenPosted instead.
 	//
@@ -129,8 +132,12 @@ type Hooks interface {
 	UserHasLoggedIn(c *Context, user *model.User)
 
 	// FileWillBeUploaded is invoked when a file is uploaded, but before it is committed to backing store.
-	// Read from file to retrieve the body of the uploaded file. You may modify the body of the file by writing to output.
-	// Returned FileInfo will be used instead of input FileInfo. Return nil to reject the file upload and include a text reason as the second argument.
+	// Read from file to retrieve the body of the uploaded file.
+	//
+	// To reject a file upload, return an non-empty string describing why the file was rejected.
+	// To modify the file, write to the output and/or return a non-nil *model.FileInfo, as well as an empty string.
+	// To allow the file without modification, do not write to the output and return a nil *model.FileInfo and an empty string.
+	//
 	// Note that this method will be called for files uploaded by plugins, including the plugin that uploaded the post.
 	// FileInfo.Size will be automatically set properly if you modify the file.
 	FileWillBeUploaded(c *Context, info *model.FileInfo, file io.Reader, output io.Writer) (*model.FileInfo, string)