MM-27722 Make sidebar category validation silently reject bad channel IDs (#15324)

* MM-27722 Make sidebar category validation silently reject bad channel IDs

* Call validateSidebarCategories when possible

* Remove blank line

* Stop restarting server between subtests

api4/channel_category.go

@@ -7,6 +7,7 @@ import (
 	"net/http"
 
 	"github.com/mattermost/mattermost-server/v5/audit"
+	"github.com/mattermost/mattermost-server/v5/mlog"
 	"github.com/mattermost/mattermost-server/v5/model"
 )
 
@@ -49,10 +50,12 @@ func createCategoryForTeamForUser(c *Context, w http.ResponseWriter, r *http.Req
 		c.SetInvalidParam("category")
 		return
 	}
-	if appErr := validateUserChannels("createCategoryForTeamForUser", c, c.Params.TeamId, c.Params.UserId, categoryCreateRequest.Channels); appErr != nil {
+
+	if appErr := validateSidebarCategory(c, c.Params.TeamId, c.Params.UserId, categoryCreateRequest); appErr != nil {
 		c.Err = appErr
 		return
 	}
+
 	category, appErr := c.App.CreateSidebarCategory(c.Params.UserId, c.Params.TeamId, categoryCreateRequest)
 	if appErr != nil {
 		c.Err = appErr
@@ -155,15 +158,15 @@ func updateCategoriesForTeamForUser(c *Context, w http.ResponseWriter, r *http.R
 		c.SetInvalidParam("category")
 		return
 	}
-	var channelsToCheck []string
+
 	for _, category := range categoriesUpdateRequest {
 		if !c.App.SessionHasPermissionToCategory(*c.App.Session(), c.Params.UserId, c.Params.TeamId, category.Id) {
 			c.SetInvalidParam("category")
 			return
 		}
-		channelsToCheck = append(channelsToCheck, category.Channels...)
 	}
-	if appErr := validateUserChannels("updateCategoriesForTeamForUser", c, c.Params.TeamId, c.Params.UserId, channelsToCheck); appErr != nil {
+
+	if appErr := validateSidebarCategories(c, c.Params.TeamId, c.Params.UserId, categoriesUpdateRequest); appErr != nil {
 		c.Err = appErr
 		return
 	}
@@ -178,15 +181,34 @@ func updateCategoriesForTeamForUser(c *Context, w http.ResponseWriter, r *http.R
 	w.Write(model.SidebarCategoriesWithChannelsToJson(categories))
 }
 
-// validateUserChannels confirms that the given user is a member of the given channel IDs. Returns an error if the user
-// is not a member of any channel or nil if the user is a member of each channel.
-func validateUserChannels(operationName string, c *Context, teamId, userId string, channelIDs []string) *model.AppError {
+func validateSidebarCategory(c *Context, teamId, userId string, category *model.SidebarCategoryWithChannels) *model.AppError {
 	channels, err := c.App.GetChannelsForUser(teamId, userId, true, 0)
 	if err != nil {
-		return model.NewAppError("Api4."+operationName, "api.invalid_channel", nil, err.Error(), http.StatusBadRequest)
+		return model.NewAppError("validateSidebarCategory", "api.invalid_channel", nil, err.Error(), http.StatusBadRequest)
 	}
 
-	for _, channelId := range channelIDs {
+	category.Channels = validateSidebarCategoryChannels(userId, category.Channels, channels)
+
+	return nil
+}
+
+func validateSidebarCategories(c *Context, teamId, userId string, categories []*model.SidebarCategoryWithChannels) *model.AppError {
+	channels, err := c.App.GetChannelsForUser(teamId, userId, true, 0)
+	if err != nil {
+		return model.NewAppError("validateSidebarCategory", "api.invalid_channel", nil, err.Error(), http.StatusBadRequest)
+	}
+
+	for _, category := range categories {
+		category.Channels = validateSidebarCategoryChannels(userId, category.Channels, channels)
+	}
+
+	return nil
+}
+
+func validateSidebarCategoryChannels(userId string, channelIds []string, channels *model.ChannelList) []string {
+	var filtered []string
+
+	for _, channelId := range channelIds {
 		found := false
 		for _, channel := range *channels {
 			if channel.Id == channelId {
@@ -195,12 +217,14 @@ func validateUserChannels(operationName string, c *Context, teamId, userId strin
 			}
 		}
 
-		if !found {
-			return model.NewAppError("Api4."+operationName, "api.invalid_channel", nil, "", http.StatusBadRequest)
+		if found {
+			filtered = append(filtered, channelId)
+		} else {
+			mlog.Info("Stopping user from adding channel to their sidebar when they are not a member", mlog.String("user_id", userId), mlog.String("channel_id", channelId))
 		}
 	}
 
-	return nil
+	return filtered
 }
 
 func updateCategoryForTeamForUser(c *Context, w http.ResponseWriter, r *http.Request) {
@@ -223,10 +247,11 @@ func updateCategoryForTeamForUser(c *Context, w http.ResponseWriter, r *http.Req
 		return
 	}
 
-	if appErr := validateUserChannels("updateCategoryForTeamForUser", c, c.Params.TeamId, c.Params.UserId, categoryUpdateRequest.Channels); appErr != nil {
+	if appErr := validateSidebarCategory(c, c.Params.TeamId, c.Params.UserId, categoryUpdateRequest); appErr != nil {
 		c.Err = appErr
 		return
 	}
+
 	categoryUpdateRequest.Id = c.Params.CategoryId
 
 	categories, appErr := c.App.UpdateSidebarCategories(c.Params.UserId, c.Params.TeamId, []*model.SidebarCategoryWithChannels{categoryUpdateRequest})

api4/channel_category_test.go

@@ -11,12 +11,75 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestUpdateCategoryForTeamForUser(t *testing.T) {
-	t.Run("should update the channel order of the Channels category", func(t *testing.T) {
-		th := Setup(t).InitBasic()
-		defer th.TearDown()
+func TestCreateCategoryForTeamForUser(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
 
-		categories, resp := th.Client.GetSidebarCategoriesForTeamForUser(th.BasicUser.Id, th.BasicTeam.Id, "")
+	t.Run("should silently prevent the user from creating a category with an invalid channel ID", func(t *testing.T) {
+		user, client := setupUserForSubtest(t, th)
+
+		categories, resp := client.GetSidebarCategoriesForTeamForUser(user.Id, th.BasicTeam.Id, "")
+		require.Nil(t, resp.Error)
+		require.Len(t, categories.Categories, 3)
+		require.Len(t, categories.Order, 3)
+
+		// Attempt to create the category
+		category := &model.SidebarCategoryWithChannels{
+			SidebarCategory: model.SidebarCategory{
+				UserId:      user.Id,
+				TeamId:      th.BasicTeam.Id,
+				DisplayName: "test",
+			},
+			Channels: []string{th.BasicChannel.Id, "notachannel", th.BasicChannel2.Id},
+		}
+
+		received, resp := client.CreateSidebarCategoryForTeamForUser(user.Id, th.BasicTeam.Id, category)
+		require.Nil(t, resp.Error)
+		assert.NotContains(t, received.Channels, "notachannel")
+		assert.Equal(t, []string{th.BasicChannel.Id, th.BasicChannel2.Id}, received.Channels)
+	})
+
+	t.Run("should silently prevent the user from creating a category with a channel that they're not a member of", func(t *testing.T) {
+		user, client := setupUserForSubtest(t, th)
+
+		categories, resp := client.GetSidebarCategoriesForTeamForUser(user.Id, th.BasicTeam.Id, "")
+		require.Nil(t, resp.Error)
+		require.Len(t, categories.Categories, 3)
+		require.Len(t, categories.Order, 3)
+
+		// Have another user create a channel that user isn't a part of
+		channel, resp := th.SystemAdminClient.CreateChannel(&model.Channel{
+			TeamId: th.BasicTeam.Id,
+			Type:   model.CHANNEL_OPEN,
+			Name:   "testchannel",
+		})
+		require.Nil(t, resp.Error)
+
+		// Attempt to create the category
+		category := &model.SidebarCategoryWithChannels{
+			SidebarCategory: model.SidebarCategory{
+				UserId:      user.Id,
+				TeamId:      th.BasicTeam.Id,
+				DisplayName: "test",
+			},
+			Channels: []string{th.BasicChannel.Id, channel.Id},
+		}
+
+		received, resp := client.CreateSidebarCategoryForTeamForUser(user.Id, th.BasicTeam.Id, category)
+		require.Nil(t, resp.Error)
+		assert.NotContains(t, received.Channels, channel.Id)
+		assert.Equal(t, []string{th.BasicChannel.Id}, received.Channels)
+	})
+}
+
+func TestUpdateCategoryForTeamForUser(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	t.Run("should update the channel order of the Channels category", func(t *testing.T) {
+		user, client := setupUserForSubtest(t, th)
+
+		categories, resp := client.GetSidebarCategoriesForTeamForUser(user.Id, th.BasicTeam.Id, "")
 		require.Nil(t, resp.Error)
 		require.Len(t, categories.Categories, 3)
 		require.Len(t, categories.Order, 3)
@@ -31,23 +94,22 @@ func TestUpdateCategoryForTeamForUser(t *testing.T) {
 			Channels:        []string{channelsCategory.Channels[1], channelsCategory.Channels[0], channelsCategory.Channels[4], channelsCategory.Channels[3], channelsCategory.Channels[2]},
 		}
 
-		received, resp := th.Client.UpdateSidebarCategoryForTeamForUser(th.BasicUser.Id, th.BasicTeam.Id, channelsCategory.Id, updatedCategory)
+		received, resp := client.UpdateSidebarCategoryForTeamForUser(user.Id, th.BasicTeam.Id, channelsCategory.Id, updatedCategory)
 		assert.Nil(t, resp.Error)
 		assert.Equal(t, channelsCategory.Id, received.Id)
 		assert.Equal(t, updatedCategory.Channels, received.Channels)
 
 		// And when requesting the category later
-		received, resp = th.Client.GetSidebarCategoryForTeamForUser(th.BasicUser.Id, th.BasicTeam.Id, channelsCategory.Id, "")
+		received, resp = client.GetSidebarCategoryForTeamForUser(user.Id, th.BasicTeam.Id, channelsCategory.Id, "")
 		assert.Nil(t, resp.Error)
 		assert.Equal(t, channelsCategory.Id, received.Id)
 		assert.Equal(t, updatedCategory.Channels, received.Channels)
 	})
 
 	t.Run("should update the sort order of the DM category", func(t *testing.T) {
-		th := Setup(t).InitBasic()
-		defer th.TearDown()
+		user, client := setupUserForSubtest(t, th)
 
-		categories, resp := th.Client.GetSidebarCategoriesForTeamForUser(th.BasicUser.Id, th.BasicTeam.Id, "")
+		categories, resp := client.GetSidebarCategoriesForTeamForUser(user.Id, th.BasicTeam.Id, "")
 		require.Nil(t, resp.Error)
 		require.Len(t, categories.Categories, 3)
 		require.Len(t, categories.Order, 3)
@@ -63,25 +125,24 @@ func TestUpdateCategoryForTeamForUser(t *testing.T) {
 		}
 		updatedCategory.Sorting = model.SidebarCategorySortAlphabetical
 
-		received, resp := th.Client.UpdateSidebarCategoryForTeamForUser(th.BasicUser.Id, th.BasicTeam.Id, dmsCategory.Id, updatedCategory)
+		received, resp := client.UpdateSidebarCategoryForTeamForUser(user.Id, th.BasicTeam.Id, dmsCategory.Id, updatedCategory)
 		assert.Nil(t, resp.Error)
 		assert.Equal(t, dmsCategory.Id, received.Id)
 		assert.Equal(t, model.SidebarCategorySortAlphabetical, received.Sorting)
 
 		// And when requesting the category later
-		received, resp = th.Client.GetSidebarCategoryForTeamForUser(th.BasicUser.Id, th.BasicTeam.Id, dmsCategory.Id, "")
+		received, resp = client.GetSidebarCategoryForTeamForUser(user.Id, th.BasicTeam.Id, dmsCategory.Id, "")
 		assert.Nil(t, resp.Error)
 		assert.Equal(t, dmsCategory.Id, received.Id)
 		assert.Equal(t, model.SidebarCategorySortAlphabetical, received.Sorting)
 	})
 
 	t.Run("should update the display name of a custom category", func(t *testing.T) {
-		th := Setup(t).InitBasic()
-		defer th.TearDown()
+		user, client := setupUserForSubtest(t, th)
 
-		customCategory, resp := th.Client.CreateSidebarCategoryForTeamForUser(th.BasicUser.Id, th.BasicTeam.Id, &model.SidebarCategoryWithChannels{
+		customCategory, resp := client.CreateSidebarCategoryForTeamForUser(user.Id, th.BasicTeam.Id, &model.SidebarCategoryWithChannels{
 			SidebarCategory: model.SidebarCategory{
-				UserId:      th.BasicUser.Id,
+				UserId:      user.Id,
 				TeamId:      th.BasicTeam.Id,
 				DisplayName: "custom123",
 			},
@@ -96,23 +157,22 @@ func TestUpdateCategoryForTeamForUser(t *testing.T) {
 		}
 		updatedCategory.DisplayName = "abcCustom"
 
-		received, resp := th.Client.UpdateSidebarCategoryForTeamForUser(th.BasicUser.Id, th.BasicTeam.Id, customCategory.Id, updatedCategory)
+		received, resp := client.UpdateSidebarCategoryForTeamForUser(user.Id, th.BasicTeam.Id, customCategory.Id, updatedCategory)
 		assert.Nil(t, resp.Error)
 		assert.Equal(t, customCategory.Id, received.Id)
 		assert.Equal(t, updatedCategory.DisplayName, received.DisplayName)
 
 		// And when requesting the category later
-		received, resp = th.Client.GetSidebarCategoryForTeamForUser(th.BasicUser.Id, th.BasicTeam.Id, customCategory.Id, "")
+		received, resp = client.GetSidebarCategoryForTeamForUser(user.Id, th.BasicTeam.Id, customCategory.Id, "")
 		assert.Nil(t, resp.Error)
 		assert.Equal(t, customCategory.Id, received.Id)
 		assert.Equal(t, updatedCategory.DisplayName, received.DisplayName)
 	})
 
 	t.Run("should update the channel order of the category even if it contains archived channels", func(t *testing.T) {
-		th := Setup(t).InitBasic()
-		defer th.TearDown()
+		user, client := setupUserForSubtest(t, th)
 
-		categories, resp := th.Client.GetSidebarCategoriesForTeamForUser(th.BasicUser.Id, th.BasicTeam.Id, "")
+		categories, resp := client.GetSidebarCategoriesForTeamForUser(user.Id, th.BasicTeam.Id, "")
 		require.Nil(t, resp.Error)
 		require.Len(t, categories.Categories, 3)
 		require.Len(t, categories.Order, 3)
@@ -122,7 +182,7 @@ func TestUpdateCategoryForTeamForUser(t *testing.T) {
 		require.Len(t, channelsCategory.Channels, 5) // Town Square, Off Topic, and the 3 channels created by InitBasic
 
 		// Delete one of the channels
-		_, resp = th.Client.DeleteChannel(th.BasicChannel.Id)
+		_, resp = client.DeleteChannel(th.BasicChannel.Id)
 		require.Nil(t, resp.Error)
 
 		// Should still be able to reorder the channels
@@ -131,9 +191,145 @@ func TestUpdateCategoryForTeamForUser(t *testing.T) {
 			Channels:        []string{channelsCategory.Channels[1], channelsCategory.Channels[0], channelsCategory.Channels[4], channelsCategory.Channels[3], channelsCategory.Channels[2]},
 		}
 
-		received, resp := th.Client.UpdateSidebarCategoryForTeamForUser(th.BasicUser.Id, th.BasicTeam.Id, channelsCategory.Id, updatedCategory)
+		received, resp := client.UpdateSidebarCategoryForTeamForUser(user.Id, th.BasicTeam.Id, channelsCategory.Id, updatedCategory)
 		require.Nil(t, resp.Error)
 		assert.Equal(t, channelsCategory.Id, received.Id)
 		assert.Equal(t, updatedCategory.Channels, received.Channels)
 	})
+
+	t.Run("should silently prevent the user from adding an invalid channel ID", func(t *testing.T) {
+		user, client := setupUserForSubtest(t, th)
+
+		categories, resp := client.GetSidebarCategoriesForTeamForUser(user.Id, th.BasicTeam.Id, "")
+		require.Nil(t, resp.Error)
+		require.Len(t, categories.Categories, 3)
+		require.Len(t, categories.Order, 3)
+
+		channelsCategory := categories.Categories[1]
+		require.Equal(t, model.SidebarCategoryChannels, channelsCategory.Type)
+
+		updatedCategory := &model.SidebarCategoryWithChannels{
+			SidebarCategory: channelsCategory.SidebarCategory,
+			Channels:        append(channelsCategory.Channels, "notachannel"),
+		}
+
+		received, resp := client.UpdateSidebarCategoryForTeamForUser(user.Id, th.BasicTeam.Id, channelsCategory.Id, updatedCategory)
+		require.Nil(t, resp.Error)
+		assert.Equal(t, channelsCategory.Id, received.Id)
+		assert.NotContains(t, received.Channels, "notachannel")
+		assert.Equal(t, channelsCategory.Channels, received.Channels)
+	})
+
+	t.Run("should silently prevent the user from adding a channel that they're not a member of", func(t *testing.T) {
+		user, client := setupUserForSubtest(t, th)
+
+		categories, resp := client.GetSidebarCategoriesForTeamForUser(user.Id, th.BasicTeam.Id, "")
+		require.Nil(t, resp.Error)
+		require.Len(t, categories.Categories, 3)
+		require.Len(t, categories.Order, 3)
+
+		channelsCategory := categories.Categories[1]
+		require.Equal(t, model.SidebarCategoryChannels, channelsCategory.Type)
+
+		// Have another user create a channel that user isn't a part of
+		channel, resp := th.SystemAdminClient.CreateChannel(&model.Channel{
+			TeamId: th.BasicTeam.Id,
+			Type:   model.CHANNEL_OPEN,
+			Name:   "testchannel",
+		})
+		require.Nil(t, resp.Error)
+
+		// Attempt to update the category
+		updatedCategory := &model.SidebarCategoryWithChannels{
+			SidebarCategory: channelsCategory.SidebarCategory,
+			Channels:        append(channelsCategory.Channels, channel.Id),
+		}
+
+		received, resp := client.UpdateSidebarCategoryForTeamForUser(user.Id, th.BasicTeam.Id, channelsCategory.Id, updatedCategory)
+		require.Nil(t, resp.Error)
+		assert.Equal(t, channelsCategory.Id, received.Id)
+		assert.NotContains(t, received.Channels, channel.Id)
+		assert.Equal(t, channelsCategory.Channels, received.Channels)
+	})
+}
+
+func TestUpdateCategoriesForTeamForUser(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	t.Run("should silently prevent the user from adding an invalid channel ID", func(t *testing.T) {
+		user, client := setupUserForSubtest(t, th)
+
+		categories, resp := client.GetSidebarCategoriesForTeamForUser(user.Id, th.BasicTeam.Id, "")
+		require.Nil(t, resp.Error)
+		require.Len(t, categories.Categories, 3)
+		require.Len(t, categories.Order, 3)
+
+		channelsCategory := categories.Categories[1]
+		require.Equal(t, model.SidebarCategoryChannels, channelsCategory.Type)
+
+		updatedCategory := &model.SidebarCategoryWithChannels{
+			SidebarCategory: channelsCategory.SidebarCategory,
+			Channels:        append(channelsCategory.Channels, "notachannel"),
+		}
+
+		received, resp := client.UpdateSidebarCategoriesForTeamForUser(user.Id, th.BasicTeam.Id, []*model.SidebarCategoryWithChannels{updatedCategory})
+		require.Nil(t, resp.Error)
+		assert.Equal(t, channelsCategory.Id, received[0].Id)
+		assert.NotContains(t, received[0].Channels, "notachannel")
+		assert.Equal(t, channelsCategory.Channels, received[0].Channels)
+	})
+
+	t.Run("should silently prevent the user from adding a channel that they're not a member of", func(t *testing.T) {
+		user, client := setupUserForSubtest(t, th)
+
+		categories, resp := client.GetSidebarCategoriesForTeamForUser(user.Id, th.BasicTeam.Id, "")
+		require.Nil(t, resp.Error)
+		require.Len(t, categories.Categories, 3)
+		require.Len(t, categories.Order, 3)
+
+		channelsCategory := categories.Categories[1]
+		require.Equal(t, model.SidebarCategoryChannels, channelsCategory.Type)
+
+		// Have another user create a channel that user isn't a part of
+		channel, resp := th.SystemAdminClient.CreateChannel(&model.Channel{
+			TeamId: th.BasicTeam.Id,
+			Type:   model.CHANNEL_OPEN,
+			Name:   "testchannel",
+		})
+		require.Nil(t, resp.Error)
+
+		// Attempt to update the category
+		updatedCategory := &model.SidebarCategoryWithChannels{
+			SidebarCategory: channelsCategory.SidebarCategory,
+			Channels:        append(channelsCategory.Channels, channel.Id),
+		}
+
+		received, resp := client.UpdateSidebarCategoriesForTeamForUser(user.Id, th.BasicTeam.Id, []*model.SidebarCategoryWithChannels{updatedCategory})
+		require.Nil(t, resp.Error)
+		assert.Equal(t, channelsCategory.Id, received[0].Id)
+		assert.NotContains(t, received[0].Channels, channel.Id)
+		assert.Equal(t, channelsCategory.Channels, received[0].Channels)
+	})
+}
+
+func setupUserForSubtest(t *testing.T, th *TestHelper) (*model.User, *model.Client4) {
+	password := "password"
+	user, err := th.App.CreateUser(&model.User{
+		Email:    th.GenerateTestEmail(),
+		Username: "user_" + model.NewId(),
+		Password: password,
+	})
+	require.Nil(t, err)
+
+	th.LinkUserToTeam(user, th.BasicTeam)
+	th.AddUserToChannel(user, th.BasicChannel)
+	th.AddUserToChannel(user, th.BasicChannel2)
+	th.AddUserToChannel(user, th.BasicPrivateChannel)
+
+	client := th.CreateClient()
+	user, resp := client.Login(user.Email, password)
+	require.Nil(t, resp.Error)
+
+	return user, client
 }