From 9b688ae971346f9d60455bd34848fe34143efc82 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Mon, 31 Aug 2020 09:45:50 -0400 Subject: [PATCH] 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 | 51 +++++-- api4/channel_category_test.go | 242 ++++++++++++++++++++++++++++++---- model/client4.go | 22 +++- 3 files changed, 277 insertions(+), 38 deletions(-) diff --git a/api4/channel_category.go b/api4/channel_category.go index 72bcff2b8f..1a716ece23 100644 --- a/api4/channel_category.go +++ b/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}) diff --git a/api4/channel_category_test.go b/api4/channel_category_test.go index 426068c7fa..68211c2b79 100644 --- a/api4/channel_category_test.go +++ b/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 } diff --git a/model/client4.go b/model/client4.go index 5034b6e79a..6ab34f1ac3 100644 --- a/model/client4.go +++ b/model/client4.go @@ -5405,6 +5405,24 @@ func (c *Client4) CreateSidebarCategoryForTeamForUser(userID, teamID string, cat return cat, BuildResponse(r) } +func (c *Client4) UpdateSidebarCategoriesForTeamForUser(userID, teamID string, categories []*SidebarCategoryWithChannels) ([]*SidebarCategoryWithChannels, *Response) { + payload, _ := json.Marshal(categories) + route := c.GetUserCategoryRoute(userID, teamID) + + r, appErr := c.doApiPutBytes(route, payload) + if appErr != nil { + return nil, BuildErrorResponse(r, appErr) + } + defer closeBody(r) + + categories, err := SidebarCategoriesFromJson(r.Body) + if err != nil { + return nil, BuildErrorResponse(r, NewAppError("Client4.UpdateSidebarCategoriesForTeamForUser", "model.utils.decode_json.app_error", nil, err.Error(), r.StatusCode)) + } + + return categories, BuildResponse(r) +} + func (c *Client4) GetSidebarCategoryOrderForTeamForUser(userID, teamID, etag string) ([]string, *Response) { route := c.GetUserCategoryRoute(userID, teamID) + "/order" r, err := c.DoApiGet(route, etag) @@ -5435,7 +5453,7 @@ func (c *Client4) GetSidebarCategoryForTeamForUser(userID, teamID, categoryID, e defer closeBody(r) cat, err := SidebarCategoryFromJson(r.Body) if err != nil { - return nil, &Response{StatusCode: http.StatusBadRequest, Error: NewAppError(c.GetUserRoute(userID), "model.client.connecting.app_error", nil, err.Error(), http.StatusForbidden)} + return nil, BuildErrorResponse(r, NewAppError("Client4.UpdateSidebarCategoriesForTeamForUser", "model.utils.decode_json.app_error", nil, err.Error(), r.StatusCode)) } return cat, BuildResponse(r) @@ -5451,7 +5469,7 @@ func (c *Client4) UpdateSidebarCategoryForTeamForUser(userID, teamID, categoryID defer closeBody(r) cat, err := SidebarCategoryFromJson(r.Body) if err != nil { - return nil, &Response{StatusCode: http.StatusBadRequest, Error: NewAppError(c.GetUserRoute(userID), "model.client.connecting.app_error", nil, err.Error(), http.StatusForbidden)} + return nil, BuildErrorResponse(r, NewAppError("Client4.UpdateSidebarCategoriesForTeamForUser", "model.utils.decode_json.app_error", nil, err.Error(), r.StatusCode)) } return cat, BuildResponse(r)