From 2e7080e68db4213cb898d8c2e5269c489bfc937a Mon Sep 17 00:00:00 2001
From: Lev <1187448+levb@users.noreply.github.com>
Date: Wed, 10 Apr 2019 16:41:29 -0700
Subject: [PATCH] Mitigated MM-14084 - Handling of PostAction Update props
 (#10546)

* Mitigated MM-14084 - Handling of PostAction Update props. Also fixed MM-14795.

- Added logic to preserve the original post's Props in their entirety
  if integration's Update.Props == nil
- Removed mandatory setting of "from_webhook"
- TODO: ?? Extend the list of protected props?

* Preserve IsPinned, HasReactions

- and so much for the tests, fixed a previous bug where UpdatePost was
not in the right place.

* Improved comments and names, as per feedback

* Added test for IsPinned, HasReactions as perfeedback
---
 app/integration_action.go      | 43 ++++++++++++++++++++++++++--------
 app/integration_action_test.go |  6 +++++
 2 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/app/integration_action.go b/app/integration_action.go
index 15a8190cb1..7091c53224 100644
--- a/app/integration_action.go
+++ b/app/integration_action.go
@@ -35,11 +35,23 @@ func (a *App) DoPostAction(postId, actionId, userId, selectedOption string) (str
 }
 
 func (a *App) DoPostActionWithCookie(postId, actionId, userId, selectedOption string, cookie *model.PostActionCookie) (string, *model.AppError) {
-	// the prop values that we need to retain/clear in replacement message to match the original
+
+	// PostAction may result in the original post being updated. For the
+	// updated post, we need to unconditionally preserve the original
+	// IsPinned and HasReaction attributes, and preserve its entire
+	// original Props set unless the plugin returns a replacement value.
+	// originalXxx variables are used to preserve these values.
+	var originalProps map[string]interface{}
+	originalIsPinned := false
+	originalHasReactions := false
+
+	// If the updated post does contain a replacement Props set, we still
+	// need to preserve some original values, as listed in
+	// model.PostActionRetainPropKeys. remove and retain track these.
 	remove := []string{}
 	retain := map[string]interface{}{}
-	datasource := ""
 
+	datasource := ""
 	upstreamURL := ""
 	rootPostId := ""
 	upstreamRequest := &model.PostActionIntegrationRequest{
@@ -94,7 +106,8 @@ func (a *App) DoPostActionWithCookie(postId, actionId, userId, selectedOption st
 		upstreamRequest.Context = action.Integration.Context
 		datasource = action.DataSource
 
-		// Set override_username, override_icon_ur to what they were before.
+		// Save the original values that may need to be preserved (including selected
+		// Props, i.e. override_username, override_icon_url)
 		for _, key := range model.PostActionRetainPropKeys {
 			value, ok := post.Props[key]
 			if ok {
@@ -103,6 +116,9 @@ func (a *App) DoPostActionWithCookie(postId, actionId, userId, selectedOption st
 				remove = append(remove, key)
 			}
 		}
+		originalProps = post.Props
+		originalIsPinned = post.IsPinned
+		originalHasReactions = post.HasReactions
 
 		if post.RootId == "" {
 			rootPostId = post.Id
@@ -141,13 +157,21 @@ func (a *App) DoPostActionWithCookie(postId, actionId, userId, selectedOption st
 
 	if response.Update != nil {
 		response.Update.Id = postId
-		response.Update.AddProp("from_webhook", "true")
-		for key, value := range retain {
-			response.Update.AddProp(key, value)
-		}
-		for _, key := range remove {
-			delete(response.Update.Props, key)
+
+		// Restore the post attributes and Props that need to be preserved
+		if response.Update.Props == nil {
+			response.Update.Props = originalProps
+		} else {
+			for key, value := range retain {
+				response.Update.AddProp(key, value)
+			}
+			for _, key := range remove {
+				delete(response.Update.Props, key)
+			}
 		}
+		response.Update.IsPinned = originalIsPinned
+		response.Update.HasReactions = originalHasReactions
+
 		if _, appErr = a.UpdatePost(response.Update, false); appErr != nil {
 			return "", appErr
 		}
@@ -160,7 +184,6 @@ func (a *App) DoPostActionWithCookie(postId, actionId, userId, selectedOption st
 			RootId:    rootPostId,
 			UserId:    userId,
 		}
-		ephemeralPost.AddProp("from_webhook", "true")
 		for key, value := range retain {
 			ephemeralPost.AddProp(key, value)
 		}
diff --git a/app/integration_action_test.go b/app/integration_action_test.go
index b8f5e586b0..edf95eed93 100644
--- a/app/integration_action_test.go
+++ b/app/integration_action_test.go
@@ -325,6 +325,8 @@ func TestPostActionProps(t *testing.T) {
 		fmt.Fprintf(w, `{
 			"update": {
 				"message": "updated",
+				"has_reactions": true,
+				"is_pinned": false,
 				"props": {
 					"override_username":"new_override_user",
 					"override_icon_url":"new_override_icon",
@@ -341,6 +343,8 @@ func TestPostActionProps(t *testing.T) {
 		ChannelId:     th.BasicChannel.Id,
 		PendingPostId: model.NewId() + ":" + fmt.Sprint(model.GetMillis()),
 		UserId:        th.BasicUser.Id,
+		HasReactions:  false,
+		IsPinned:      true,
 		Props: model.StringInterface{
 			"attachments": []*model.SlackAttachment{
 				{
@@ -380,6 +384,8 @@ func TestPostActionProps(t *testing.T) {
 	require.Nil(t, result.Err)
 	newPost := result.Data.(*model.Post)
 
+	assert.True(t, newPost.IsPinned)
+	assert.False(t, newPost.HasReactions)
 	assert.Nil(t, newPost.Props["B"])
 	assert.Nil(t, newPost.Props["override_username"])
 	assert.Equal(t, "AA", newPost.Props["A"])