MM-21727 add an endpoint to move a command to another team (#13624)

* MM-21727 add an endpoint to move a command to another team

* endpoint
* mock / client
* unit tests

* MM-21727 PR feedback, addressed nits

* MM-21727 remove CommandMove base route

* MM-21272 replace TeamId struct with CommandMoveRequest struct

* MM-21727 fixed typo in CommandMoveRequest struct name

* MM-21727 return not-found for all getCommandById calls

* MM-21727 ensure no command ids leak

* when calling GetCommandById with invalid id return not_found
* when checking perms to manage commands for team return same not_found
* update unit tests to check for not_found

* MM-21727 Rename TeamIdFromCommandMoveRequestJson -> CommandMoveRequestFromJson

api4/command.go

@@ -18,6 +18,7 @@ func (api *API) InitCommand() {
 
 	api.BaseRoutes.Command.Handle("", api.ApiSessionRequired(getCommand)).Methods("GET")
 	api.BaseRoutes.Command.Handle("", api.ApiSessionRequired(updateCommand)).Methods("PUT")
+	api.BaseRoutes.Command.Handle("/move", api.ApiSessionRequired(moveCommand)).Methods("PUT")
 	api.BaseRoutes.Command.Handle("", api.ApiSessionRequired(deleteCommand)).Methods("DELETE")
 
 	api.BaseRoutes.Team.Handle("/commands/autocomplete", api.ApiSessionRequired(listAutocompleteCommands)).Methods("GET")
@@ -67,7 +68,7 @@ func updateCommand(c *Context, w http.ResponseWriter, r *http.Request) {
 
 	oldCmd, err := c.App.GetCommand(c.Params.CommandId)
 	if err != nil {
-		c.Err = err
+		c.SetCommandNotFoundError()
 		return
 	}
 
@@ -78,7 +79,9 @@ func updateCommand(c *Context, w http.ResponseWriter, r *http.Request) {
 
 	if !c.App.SessionHasPermissionToTeam(c.App.Session, oldCmd.TeamId, model.PERMISSION_MANAGE_SLASH_COMMANDS) {
 		c.LogAudit("fail - inappropriate permissions")
-		c.SetPermissionError(model.PERMISSION_MANAGE_SLASH_COMMANDS)
+		// here we return Not_found instead of a permissions error so we don't leak the existence of
+		// a command to someone without permissions for the team it belongs to.
+		c.SetCommandNotFoundError()
 		return
 	}
 
@@ -99,6 +102,55 @@ func updateCommand(c *Context, w http.ResponseWriter, r *http.Request) {
 	w.Write([]byte(rcmd.ToJson()))
 }
 
+func moveCommand(c *Context, w http.ResponseWriter, r *http.Request) {
+	c.RequireCommandId()
+	if c.Err != nil {
+		return
+	}
+
+	c.LogAudit("attempt")
+
+	cmr, err := model.CommandMoveRequestFromJson(r.Body)
+	if err != nil {
+		c.SetInvalidParam("team_id")
+		return
+	}
+
+	newTeam, appErr := c.App.GetTeam(cmr.TeamId)
+	if appErr != nil {
+		c.Err = appErr
+		return
+	}
+
+	if !c.App.SessionHasPermissionToTeam(c.App.Session, newTeam.Id, model.PERMISSION_MANAGE_SLASH_COMMANDS) {
+		c.LogAudit("fail - inappropriate permissions")
+		c.SetPermissionError(model.PERMISSION_MANAGE_SLASH_COMMANDS)
+		return
+	}
+
+	cmd, appErr := c.App.GetCommand(c.Params.CommandId)
+	if appErr != nil {
+		c.SetCommandNotFoundError()
+		return
+	}
+
+	if !c.App.SessionHasPermissionToTeam(c.App.Session, cmd.TeamId, model.PERMISSION_MANAGE_SLASH_COMMANDS) {
+		c.LogAudit("fail - inappropriate permissions")
+		// here we return Not_found instead of a permissions error so we don't leak the existence of
+		// a command to someone without permissions for the team it belongs to.
+		c.SetCommandNotFoundError()
+		return
+	}
+
+	if appErr = c.App.MoveCommand(newTeam, cmd); appErr != nil {
+		c.Err = appErr
+		return
+	}
+
+	c.LogAudit("success")
+	ReturnStatusOK(w)
+}
+
 func deleteCommand(c *Context, w http.ResponseWriter, r *http.Request) {
 	c.RequireCommandId()
 	if c.Err != nil {
@@ -109,13 +161,15 @@ func deleteCommand(c *Context, w http.ResponseWriter, r *http.Request) {
 
 	cmd, err := c.App.GetCommand(c.Params.CommandId)
 	if err != nil {
-		c.Err = err
+		c.SetCommandNotFoundError()
 		return
 	}
 
 	if !c.App.SessionHasPermissionToTeam(c.App.Session, cmd.TeamId, model.PERMISSION_MANAGE_SLASH_COMMANDS) {
 		c.LogAudit("fail - inappropriate permissions")
-		c.SetPermissionError(model.PERMISSION_MANAGE_SLASH_COMMANDS)
+		// here we return Not_found instead of a permissions error so we don't leak the existence of
+		// a command to someone without permissions for the team it belongs to.
+		c.SetCommandNotFoundError()
 		return
 	}
 
@@ -296,13 +350,15 @@ func regenCommandToken(c *Context, w http.ResponseWriter, r *http.Request) {
 	c.LogAudit("attempt")
 	cmd, err := c.App.GetCommand(c.Params.CommandId)
 	if err != nil {
-		c.Err = err
+		c.SetCommandNotFoundError()
 		return
 	}
 
 	if !c.App.SessionHasPermissionToTeam(c.App.Session, cmd.TeamId, model.PERMISSION_MANAGE_SLASH_COMMANDS) {
 		c.LogAudit("fail - inappropriate permissions")
-		c.SetPermissionError(model.PERMISSION_MANAGE_SLASH_COMMANDS)
+		// here we return Not_found instead of a permissions error so we don't leak the existence of
+		// a command to someone without permissions for the team it belongs to.
+		c.SetCommandNotFoundError()
 		return
 	}
 

api4/command_test.go

@@ -128,13 +128,71 @@ func TestUpdateCommand(t *testing.T) {
 	cmd2.TeamId = team.Id
 
 	_, resp = th.Client.UpdateCommand(cmd2)
-	CheckForbiddenStatus(t, resp)
+	CheckNotFoundStatus(t, resp)
 
 	Client.Logout()
 	_, resp = Client.UpdateCommand(cmd2)
 	CheckUnauthorizedStatus(t, resp)
 }
 
+func TestMoveCommand(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+	Client := th.SystemAdminClient
+	user := th.SystemAdminUser
+	team := th.BasicTeam
+	newTeam := th.CreateTeam()
+
+	enableCommands := *th.App.Config().ServiceSettings.EnableCommands
+	defer func() {
+		th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableCommands = &enableCommands })
+	}()
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableCommands = true })
+
+	cmd1 := &model.Command{
+		CreatorId: user.Id,
+		TeamId:    team.Id,
+		URL:       "http://nowhere.com",
+		Method:    model.COMMAND_METHOD_POST,
+		Trigger:   "trigger1",
+	}
+
+	rcmd1, _ := th.App.CreateCommand(cmd1)
+
+	ok, resp := Client.MoveCommand(newTeam.Id, rcmd1.Id)
+	CheckNoError(t, resp)
+	require.True(t, ok)
+
+	rcmd1, _ = th.App.GetCommand(rcmd1.Id)
+	require.NotNil(t, rcmd1)
+	require.Equal(t, newTeam.Id, rcmd1.TeamId)
+
+	ok, resp = Client.MoveCommand(newTeam.Id, "bogus")
+	CheckBadRequestStatus(t, resp)
+	require.False(t, ok)
+
+	ok, resp = Client.MoveCommand(GenerateTestId(), rcmd1.Id)
+	CheckNotFoundStatus(t, resp)
+	require.False(t, ok)
+
+	cmd2 := &model.Command{
+		CreatorId: user.Id,
+		TeamId:    team.Id,
+		URL:       "http://nowhere.com",
+		Method:    model.COMMAND_METHOD_POST,
+		Trigger:   "trigger2",
+	}
+
+	rcmd2, _ := th.App.CreateCommand(cmd2)
+
+	_, resp = th.Client.MoveCommand(newTeam.Id, rcmd2.Id)
+	CheckNotFoundStatus(t, resp)
+
+	Client.Logout()
+	_, resp = Client.MoveCommand(newTeam.Id, rcmd2.Id)
+	CheckUnauthorizedStatus(t, resp)
+}
+
 func TestDeleteCommand(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
@@ -185,7 +243,7 @@ func TestDeleteCommand(t *testing.T) {
 	rcmd2, _ := th.App.CreateCommand(cmd2)
 
 	_, resp = th.Client.DeleteCommand(rcmd2.Id)
-	CheckForbiddenStatus(t, resp)
+	CheckNotFoundStatus(t, resp)
 
 	Client.Logout()
 	_, resp = Client.DeleteCommand(rcmd2.Id)
@@ -435,7 +493,7 @@ func TestRegenToken(t *testing.T) {
 	require.NotEqual(t, createdCmd.Token, token, "should update the token")
 
 	token, resp = Client.RegenCommandToken(createdCmd.Id)
-	CheckForbiddenStatus(t, resp)
+	CheckNotFoundStatus(t, resp)
 	require.Empty(t, token, "should not return the token")
 }
 

model/client4.go

@@ -352,6 +352,10 @@ func (c *Client4) GetCommandRoute(commandId string) string {
 	return fmt.Sprintf(c.GetCommandsRoute()+"/%v", commandId)
 }
 
+func (c *Client4) GetCommandMoveRoute(commandId string) string {
+	return fmt.Sprintf(c.GetCommandsRoute()+"/%v/move", commandId)
+}
+
 func (c *Client4) GetEmojisRoute() string {
 	return fmt.Sprintf("/emoji")
 }
@@ -4019,6 +4023,17 @@ func (c *Client4) UpdateCommand(cmd *Command) (*Command, *Response) {
 	return CommandFromJson(r.Body), BuildResponse(r)
 }
 
+// MoveCommand moves a command to a different team.
+func (c *Client4) MoveCommand(teamId string, commandId string) (bool, *Response) {
+	cmr := CommandMoveRequest{TeamId: teamId}
+	r, err := c.DoApiPut(c.GetCommandMoveRoute(commandId), cmr.ToJson())
+	if err != nil {
+		return false, BuildErrorResponse(r, err)
+	}
+	defer closeBody(r)
+	return CheckStatusOK(r), BuildResponse(r)
+}
+
 // DeleteCommand deletes a command based on the provided command id string.
 func (c *Client4) DeleteCommand(commandId string) (bool, *Response) {
 	r, err := c.DoApiDelete(c.GetCommandRoute(commandId))

model/command_request.go

@@ -0,0 +1,31 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package model
+
+import (
+	"encoding/json"
+	"io"
+)
+
+type CommandMoveRequest struct {
+	TeamId string `json:"team_id"`
+}
+
+func CommandMoveRequestFromJson(data io.Reader) (*CommandMoveRequest, error) {
+	decoder := json.NewDecoder(data)
+	var cmr CommandMoveRequest
+	err := decoder.Decode(&cmr)
+	if err != nil {
+		return nil, err
+	}
+	return &cmr, nil
+}
+
+func (cmr *CommandMoveRequest) ToJson() string {
+	b, err := json.Marshal(cmr)
+	if err != nil {
+		return ""
+	}
+	return string(b)
+}