MM-54130 MM-54149 MM-54151 Fix validation (#1868)

* Improving permissions for playbook channel actions.

* Validate posts provided to timeline

* Handle failing type conversion

* Test fix

server/api/actions.go

@@ -7,6 +7,7 @@ import (
 	"net/url"
 
 	"github.com/mattermost/mattermost-plugin-playbooks/server/app"
+	"github.com/mitchellh/mapstructure"
 	"github.com/pkg/errors"
 
 	"github.com/gorilla/mux"
@@ -65,6 +66,18 @@ func (a *ActionsHandler) createChannelAction(c *Context, w http.ResponseWriter,
 		return
 	}
 
+	if channelAction.ActionType == app.ActionTypePromptRunPlaybook {
+		var payload app.PromptRunPlaybookFromKeywordsPayload
+		if err := mapstructure.Decode(channelAction.Payload, &payload); err != nil {
+			a.HandleErrorWithCode(w, c.logger, http.StatusBadRequest, "couldn't verify permissions for run playbook action", err)
+			return
+		}
+
+		if !a.PermissionsCheck(w, c.logger, a.permissions.PlaybookView(userID, payload.PlaybookID)) {
+			return
+		}
+	}
+
 	// Validate the action type and payload
 	if err := a.channelActionsService.Validate(channelAction); err != nil {
 		a.HandleErrorWithCode(w, c.logger, http.StatusBadRequest, "invalid action", err)
@@ -195,6 +208,18 @@ func (a *ActionsHandler) updateChannelAction(c *Context, w http.ResponseWriter,
 		return
 	}
 
+	if newChannelAction.ActionType == app.ActionTypePromptRunPlaybook {
+		var payload app.PromptRunPlaybookFromKeywordsPayload
+		if err := mapstructure.Decode(newChannelAction.Payload, &payload); err != nil {
+			a.HandleErrorWithCode(w, c.logger, http.StatusBadRequest, "couldn't verify permissions for run playbook action", err)
+			return
+		}
+
+		if !a.PermissionsCheck(w, c.logger, a.permissions.PlaybookView(userID, payload.PlaybookID)) {
+			return
+		}
+	}
+
 	// Validate the new action type and payload
 	if err := a.channelActionsService.Validate(newChannelAction); err != nil {
 		a.HandleErrorWithCode(w, c.logger, http.StatusBadRequest, "invalid action", err)

server/api/playbook_runs.go

@@ -381,7 +381,17 @@ func (h *PlaybookRunHandler) addToTimelineDialog(c *Context, w http.ResponseWrit
 		return
 	}
 
-	if err = h.playbookRunService.AddPostToTimeline(playbookRunID, userID, state.PostID, summary); err != nil {
+	post, err := h.pluginAPI.Post.GetPost(state.PostID)
+	if err != nil {
+		h.HandleErrorWithCode(w, c.logger, http.StatusBadRequest, "couldn't get post ID", err)
+		return
+	}
+
+	if !h.pluginAPI.User.HasPermissionToChannel(userID, post.ChannelId, model.PermissionReadChannel) {
+		h.HandleErrorWithCode(w, c.logger, http.StatusForbidden, "no permission to post specified", nil)
+	}
+
+	if err = h.playbookRunService.AddPostToTimeline(playbookRunID, userID, post, summary); err != nil {
 		h.HandleError(w, c.logger, errors.Wrap(err, "failed to add post to timeline"))
 		return
 	}
@@ -922,12 +932,22 @@ func (h *PlaybookRunHandler) updateStatusDialog(c *Context, w http.ResponseWrite
 
 	var options app.StatusUpdateOptions
 	if message, ok := request.Submission[app.DialogFieldMessageKey]; ok {
-		options.Message = message.(string)
+		messageStr, valid := message.(string)
+		if !valid {
+			h.HandleErrorWithCode(w, c.logger, http.StatusBadRequest, "message must be a string", nil)
+			return
+		}
+		options.Message = messageStr
 	}
 
 	if reminderI, ok := request.Submission[app.DialogFieldReminderInSecondsKey]; ok {
+		reminderStr, valid := reminderI.(string)
+		if !valid {
+			h.HandleErrorWithCode(w, c.logger, http.StatusBadRequest, "reminder must be a string", nil)
+			return
+		}
 		var reminder int
-		reminder, err = strconv.Atoi(reminderI.(string))
+		reminder, err = strconv.Atoi(reminderStr)
 		if err != nil {
 			h.HandleError(w, c.logger, err)
 			return

server/api_actions_test.go

@@ -63,7 +63,8 @@ func TestActionCreation(t *testing.T) {
 			ActionType:  client.ActionTypePromptRunPlaybook,
 			TriggerType: client.TriggerTypeKeywordsPosted,
 			Payload: client.PromptRunPlaybookFromKeywordsPayload{
-				Keywords: []string{"one"},
+				Keywords:   []string{"one"},
+				PlaybookID: e.BasicPlaybook.ID,
 			},
 		})
 
@@ -72,6 +73,24 @@ func TestActionCreation(t *testing.T) {
 		assert.NotEmpty(t, actionID)
 	})
 
+	t.Run("create playbook action no permissions to playbook", func(t *testing.T) {
+		// Create a brand new channel
+		channel := createNewChannel(t, "create-playbook-action-no-permissions")
+
+		_, err := e.PlaybooksClient.Actions.Create(context.Background(), channel.Id, client.ChannelActionCreateOptions{
+			ChannelID:   channel.Id,
+			Enabled:     true,
+			ActionType:  client.ActionTypePromptRunPlaybook,
+			TriggerType: client.TriggerTypeKeywordsPosted,
+			Payload: client.PromptRunPlaybookFromKeywordsPayload{
+				Keywords:   []string{"one"},
+				PlaybookID: e.PrivatePlaybookNoMembers.ID,
+			},
+		})
+
+		requireErrorWithStatusCode(t, err, http.StatusForbidden)
+	})
+
 	t.Run("create invalid action - duplicate action and trigger types", func(t *testing.T) {
 		// Create a brand new channel
 		channel := createNewChannel(t, "create-invalid-action-duplicate")
@@ -186,7 +205,7 @@ func TestActionCreation(t *testing.T) {
 			TriggerType: client.TriggerTypeKeywordsPosted,
 			Payload: client.PromptRunPlaybookFromKeywordsPayload{
 				Keywords:   []string{"one", "two"},
-				PlaybookID: model.NewId(),
+				PlaybookID: e.BasicPlaybook.ID,
 			},
 		})
 
@@ -224,7 +243,7 @@ func TestActionList(t *testing.T) {
 	})
 	assert.NoError(t, err)
 
-	playbookID := model.NewId()
+	playbookID := e.BasicPlaybook.ID
 	promptActionID, err := e.PlaybooksClient.Actions.Create(context.Background(), e.BasicPublicChannel.Id, client.ChannelActionCreateOptions{
 		ChannelID:   e.BasicPublicChannel.Id,
 		Enabled:     true,
@@ -399,6 +418,27 @@ func TestActionUpdate(t *testing.T) {
 		assert.Len(t, updatedPayload.Keywords, 0)
 	})
 
+	t.Run("invalid update - permissions", func(t *testing.T) {
+		actionOld := action
+		defer func() {
+			// Restore the original action
+			action = actionOld
+		}()
+		// Make an invalid modification
+		action.Payload = client.PromptRunPlaybookFromKeywordsPayload{
+			Keywords:   []string{"one"},
+			PlaybookID: e.PrivatePlaybookNoMembers.ID,
+		}
+		action.TriggerType = client.TriggerTypeKeywordsPosted
+		action.ActionType = client.ActionTypePromptRunPlaybook
+
+		// Make the Update request
+		err := e.PlaybooksClient.Actions.Update(context.Background(), action)
+
+		// Verify that the API fails with a permissions error
+		requireErrorWithStatusCode(t, err, http.StatusForbidden)
+	})
+
 	t.Run("invalid update - wrong action type", func(t *testing.T) {
 		// Make an invalid modification
 		action.ActionType = "wrong"

server/app/playbook_run.go

@@ -604,7 +604,7 @@ type PlaybookRunService interface {
 	OpenAddChecklistItemDialog(triggerID, userID, playbookRunID string, checklist int) error
 
 	// AddPostToTimeline adds an event based on a post to a playbook run's timeline.
-	AddPostToTimeline(playbookRunID, userID, postID, summary string) error
+	AddPostToTimeline(playbookRunID, userID string, post *model.Post, summary string) error
 
 	// RemoveTimelineEvent removes the timeline event (sets the DeleteAt to the current time).
 	RemoveTimelineEvent(playbookRunID, userID, eventID string) error

server/app/playbook_run_service.go

@@ -653,12 +653,7 @@ func (s *PlaybookRunServiceImpl) OpenAddChecklistItemDialog(triggerID, userID, p
 	return nil
 }
 
-func (s *PlaybookRunServiceImpl) AddPostToTimeline(playbookRunID, userID, postID, summary string) error {
-	post, err := s.pluginAPI.Post.GetPost(postID)
-	if err != nil {
-		return errors.Wrap(err, "failed to find post")
-	}
-
+func (s *PlaybookRunServiceImpl) AddPostToTimeline(playbookRunID, userID string, post *model.Post, summary string) error {
 	event := &TimelineEvent{
 		PlaybookRunID: playbookRunID,
 		CreateAt:      model.GetMillis(),
@@ -667,12 +662,12 @@ func (s *PlaybookRunServiceImpl) AddPostToTimeline(playbookRunID, userID, postID
 		EventType:     EventFromPost,
 		Summary:       summary,
 		Details:       "",
-		PostID:        postID,
+		PostID:        post.Id,
 		SubjectUserID: post.UserId,
 		CreatorUserID: userID,
 	}
 
-	if _, err = s.store.CreateTimelineEvent(event); err != nil {
+	if _, err := s.store.CreateTimelineEvent(event); err != nil {
 		return errors.Wrap(err, "failed to create timeline event")
 	}