PLT-7193: Regression - Custom slash commands don't work in direct or group message channels (#7635)

* No longer overriding specified team id for DMs/GMs, as these types of channels don't belong to a team, and doing so breaks slash commands for them

* Ensured user is on specified team in case of GM/DM, extended test suite

api4/apitestlib.go

@@ -296,13 +296,17 @@ func (me *TestHelper) CreatePrivateChannel() *model.Channel {
 }
 
 func (me *TestHelper) CreateChannelWithClient(client *model.Client4, channelType string) *model.Channel {
+	return me.CreateChannelWithClientAndTeam(client, channelType, me.BasicTeam.Id)
+}
+
+func (me *TestHelper) CreateChannelWithClientAndTeam(client *model.Client4, channelType string, teamId string) *model.Channel {
 	id := model.NewId()
 
 	channel := &model.Channel{
 		DisplayName: "dn_" + id,
 		Name:        GenerateTestChannelName(),
 		Type:        channelType,
-		TeamId:      me.BasicTeam.Id,
+		TeamId:      teamId,
 	}
 
 	utils.DisableDebugLogForTest()

api4/command.go

@@ -201,6 +201,7 @@ func executeCommand(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// checks that user is a member of the specified channel, and that they have permission to use slash commands in it
 	if !c.App.SessionHasPermissionToChannel(c.Session, commandArgs.ChannelId, model.PERMISSION_USE_SLASH_COMMANDS) {
 		c.SetPermissionError(model.PERMISSION_USE_SLASH_COMMANDS)
 		return
@@ -210,12 +211,21 @@ func executeCommand(c *Context, w http.ResponseWriter, r *http.Request) {
 	if err != nil {
 		c.Err = err
 		return
+	} else if channel.Type != model.CHANNEL_DIRECT && channel.Type != model.CHANNEL_GROUP {
+		// if this isn't a DM or GM, the team id is implicitly taken from the channel so that slash commands created on
+		// some other team can't be run against this one
+		commandArgs.TeamId = channel.TeamId
+	} else {
+		// if the slash command was used in a DM or GM, ensure that the user is a member of the specified team, so that
+		// they can't just execute slash commands against arbitrary teams
+		if c.Session.GetTeamByTeamId(commandArgs.TeamId) == nil {
+			if !app.SessionHasPermissionTo(c.Session, model.PERMISSION_USE_SLASH_COMMANDS) {
+				c.SetPermissionError(model.PERMISSION_USE_SLASH_COMMANDS)
+				return
+			}
+		}
 	}
 
-	// team id is implicitly taken from channel so that slash commands
-	// created on some other team can't be run against this one
-	commandArgs.TeamId = channel.TeamId
-
 	commandArgs.UserId = c.Session.UserId
 	commandArgs.T = c.T
 	commandArgs.Session = c.Session

api4/command_test.go

@@ -493,7 +493,7 @@ func TestExecuteCommand(t *testing.T) {
 }
 
 func TestExecuteCommandAgainstChannelOnAnotherTeam(t *testing.T) {
-	th := Setup().InitBasic().InitSystemAdmin()
+	th := Setup().InitBasic()
 	defer th.TearDown()
 	Client := th.Client
 	channel := th.BasicChannel
@@ -516,7 +516,6 @@ func TestExecuteCommandAgainstChannelOnAnotherTeam(t *testing.T) {
 		Method:    model.COMMAND_METHOD_POST,
 		Trigger:   "postcommand",
 	}
-
 	if _, err := th.App.CreateCommand(postCmd); err != nil {
 		t.Fatal("failed to create post command")
 	}
@@ -526,3 +525,131 @@ func TestExecuteCommandAgainstChannelOnAnotherTeam(t *testing.T) {
 	_, resp := Client.ExecuteCommand(channel.Id, "/postcommand")
 	CheckNotFoundStatus(t, resp)
 }
+
+func TestExecuteCommandAgainstChannelUserIsNotIn(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+	client := th.Client
+
+	enableCommands := *utils.Cfg.ServiceSettings.EnableCommands
+	allowedInternalConnections := *utils.Cfg.ServiceSettings.AllowedUntrustedInternalConnections
+	defer func() {
+		utils.Cfg.ServiceSettings.EnableCommands = &enableCommands
+		utils.Cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections
+	}()
+	*utils.Cfg.ServiceSettings.EnableCommands = true
+	*utils.Cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost"
+
+	// create a slash command on some other team where we have permission to do so
+	team2 := th.CreateTeam()
+	postCmd := &model.Command{
+		CreatorId: th.BasicUser.Id,
+		TeamId:    team2.Id,
+		URL:       fmt.Sprintf("http://localhost:%v", th.App.Srv.ListenAddr.Port) + model.API_URL_SUFFIX_V4 + "/teams/command_test",
+		Method:    model.COMMAND_METHOD_POST,
+		Trigger:   "postcommand",
+	}
+	if _, err := th.App.CreateCommand(postCmd); err != nil {
+		t.Fatal("failed to create post command")
+	}
+
+	// make a channel on that team, ensuring that our test user isn't in it
+	channel2 := th.CreateChannelWithClientAndTeam(client, model.CHANNEL_OPEN, team2.Id)
+	if success, _ := client.RemoveUserFromChannel(channel2.Id, th.BasicUser.Id); !success {
+		t.Fatal("Failed to remove user from channel")
+	}
+
+	// we should not be able to run the slash command in channel2, because we aren't in it
+	_, resp := client.ExecuteCommandWithTeam(channel2.Id, team2.Id, "/postcommand")
+	CheckForbiddenStatus(t, resp)
+}
+
+func TestExecuteCommandInDirectMessageChannel(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+	client := th.Client
+
+	enableCommands := *utils.Cfg.ServiceSettings.EnableCommands
+	allowedInternalConnections := *utils.Cfg.ServiceSettings.AllowedUntrustedInternalConnections
+	defer func() {
+		utils.Cfg.ServiceSettings.EnableCommands = &enableCommands
+		utils.Cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections
+	}()
+	*utils.Cfg.ServiceSettings.EnableCommands = true
+	*utils.Cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost"
+
+	// create a slash command on some other team where we have permission to do so
+	team2 := th.CreateTeam()
+	postCmd := &model.Command{
+		CreatorId: th.BasicUser.Id,
+		TeamId:    team2.Id,
+		URL:       fmt.Sprintf("http://localhost:%v", th.App.Srv.ListenAddr.Port) + model.API_URL_SUFFIX_V4 + "/teams/command_test",
+		Method:    model.COMMAND_METHOD_POST,
+		Trigger:   "postcommand",
+	}
+	if _, err := th.App.CreateCommand(postCmd); err != nil {
+		t.Fatal("failed to create post command")
+	}
+
+	// make a direct message channel
+	dmChannel, response := client.CreateDirectChannel(th.BasicUser.Id, th.BasicUser2.Id)
+	CheckCreatedStatus(t, response)
+
+	// we should be able to run the slash command in the DM channel
+	_, resp := client.ExecuteCommandWithTeam(dmChannel.Id, team2.Id, "/postcommand")
+	CheckOKStatus(t, resp)
+
+	// but we can't run the slash command in the DM channel if we sub in some other team's id
+	_, resp = client.ExecuteCommandWithTeam(dmChannel.Id, th.BasicTeam.Id, "/postcommand")
+	CheckNotFoundStatus(t, resp)
+}
+
+func TestExecuteCommandInTeamUserIsNotOn(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+	client := th.Client
+
+	enableCommands := *utils.Cfg.ServiceSettings.EnableCommands
+	allowedInternalConnections := *utils.Cfg.ServiceSettings.AllowedUntrustedInternalConnections
+	defer func() {
+		utils.Cfg.ServiceSettings.EnableCommands = &enableCommands
+		utils.Cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections
+	}()
+	*utils.Cfg.ServiceSettings.EnableCommands = true
+	*utils.Cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost"
+
+	// create a team that the user isn't a part of
+	team2 := th.CreateTeam()
+
+	// create a slash command on that team
+	postCmd := &model.Command{
+		CreatorId: th.BasicUser.Id,
+		TeamId:    team2.Id,
+		URL:       fmt.Sprintf("http://localhost:%v", th.App.Srv.ListenAddr.Port) + model.API_URL_SUFFIX_V4 + "/teams/command_test",
+		Method:    model.COMMAND_METHOD_POST,
+		Trigger:   "postcommand",
+	}
+	if _, err := th.App.CreateCommand(postCmd); err != nil {
+		t.Fatal("failed to create post command")
+	}
+
+	// make a direct message channel
+	dmChannel, response := client.CreateDirectChannel(th.BasicUser.Id, th.BasicUser2.Id)
+	CheckCreatedStatus(t, response)
+
+	// we should be able to run the slash command in the DM channel
+	_, resp := client.ExecuteCommandWithTeam(dmChannel.Id, team2.Id, "/postcommand")
+	CheckOKStatus(t, resp)
+
+	// if the user is removed from the team, they should NOT be able to run the slash command in the DM channel
+	if success, _ := client.RemoveTeamMember(team2.Id, th.BasicUser.Id); !success {
+		t.Fatal("Failed to remove user from team")
+	}
+	_, resp = client.ExecuteCommandWithTeam(dmChannel.Id, team2.Id, "/postcommand")
+	CheckForbiddenStatus(t, resp)
+
+	// if we omit the team id from the request, the slash command will fail because this is a DM channel, and the
+	// team id can't be inherited from the channel
+	_, resp = client.ExecuteCommand(dmChannel.Id, "/postcommand")
+	CheckForbiddenStatus(t, resp)
+}

model/client4.go

@@ -2816,7 +2816,7 @@ func (c *Client4) ListCommands(teamId string, customOnly bool) ([]*Command, *Res
 	}
 }
 
-// ExecuteCommand executes a given command.
+// ExecuteCommand executes a given slash command.
 func (c *Client4) ExecuteCommand(channelId, command string) (*CommandResponse, *Response) {
 	commandArgs := &CommandArgs{
 		ChannelId: channelId,
@@ -2830,6 +2830,22 @@ func (c *Client4) ExecuteCommand(channelId, command string) (*CommandResponse, *
 	}
 }
 
+// ExecuteCommand executes a given slash command against the specified team
+// Use this when executing slash commands in a DM/GM, since the team id cannot be inferred in that case
+func (c *Client4) ExecuteCommandWithTeam(channelId, teamId, command string) (*CommandResponse, *Response) {
+	commandArgs := &CommandArgs{
+		ChannelId: channelId,
+		TeamId:    teamId,
+		Command:   command,
+	}
+	if r, err := c.DoApiPost(c.GetCommandsRoute()+"/execute", commandArgs.ToJson()); err != nil {
+		return nil, BuildErrorResponse(r, err)
+	} else {
+		defer closeBody(r)
+		return CommandResponseFromJson(r.Body), BuildResponse(r)
+	}
+}
+
 // ListCommands will retrieve a list of commands available in the team.
 func (c *Client4) ListAutocompleteCommands(teamId string) ([]*Command, *Response) {
 	if r, err := c.DoApiGet(c.GetTeamAutoCompleteCommandsRoute(teamId), ""); err != nil {