From fee3ef7963f2c496fd6d0071690634fbe0a4cfea Mon Sep 17 00:00:00 2001 From: Scott Bishel Date: Thu, 9 Mar 2023 07:49:13 -0700 Subject: [PATCH] Merge pull request #4620 from mattermost/MM-51062-fix-api check permissions to channel before patching via api (cherry picked from commit 3a9f0fed7ee055fef4aea486680ae8d7cd4d8c0f) --- server/app/boards.go | 13 +++++ server/app/boards_test.go | 99 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+) diff --git a/server/app/boards.go b/server/app/boards.go index 2ddee918f13..d31bd573ca5 100644 --- a/server/app/boards.go +++ b/server/app/boards.go @@ -355,12 +355,15 @@ func (a *App) PatchBoard(patch *model.BoardPatch, boardID, userID string) (*mode var oldMembers []*model.BoardMember if patch.Type != nil || patch.ChannelID != nil { + testChannel := "" if patch.ChannelID != nil && *patch.ChannelID == "" { var err error oldMembers, err = a.GetMembersForBoard(boardID) if err != nil { a.logger.Error("Unable to get the board members", mlog.Err(err)) } + } else if patch.ChannelID != nil && *patch.ChannelID != "" { + testChannel = *patch.ChannelID } board, err := a.store.GetBoard(boardID) @@ -372,7 +375,17 @@ func (a *App) PatchBoard(patch *model.BoardPatch, boardID, userID string) (*mode } oldChannelID = board.ChannelID isTemplate = board.IsTemplate + if testChannel == "" { + testChannel = oldChannelID + } + + if testChannel != "" { + if !a.permissions.HasPermissionToChannel(userID, testChannel, model.PermissionCreatePost) { + return nil, model.NewErrPermission("access denied to channel") + } + } } + updatedBoard, err := a.store.PatchBoard(boardID, patch, userID) if err != nil { return nil, err diff --git a/server/app/boards_test.go b/server/app/boards_test.go index fc9771e54d5..8cd3679f2df 100644 --- a/server/app/boards_test.go +++ b/server/app/boards_test.go @@ -185,6 +185,7 @@ func TestPatchBoard(t *testing.T) { // Type not null will retrieve team members th.Store.EXPECT().GetUsersByTeam(teamID, "", false, false).Return([]*model.User{}, nil) + th.Store.EXPECT().GetUserByID(userID).Return(&model.User{ID: userID, Username: "UserName"}, nil) th.Store.EXPECT().PatchBoard(boardID, patch, userID).Return( &model.Board{ @@ -399,6 +400,104 @@ func TestPatchBoard(t *testing.T) { require.NoError(t, err) require.Equal(t, boardID, patchedBoard.ID) }) + + t.Run("patch type channel, user without post permissions", func(t *testing.T) { + const boardID = "board_id_1" + const userID = "user_id_2" + const teamID = "team_id_1" + + channelID := "myChannel" + patchType := model.BoardTypeOpen + patch := &model.BoardPatch{ + Type: &patchType, + ChannelID: &channelID, + } + + // Type not nil, will cause board to be reteived + // to check isTemplate + th.Store.EXPECT().GetBoard(boardID).Return(&model.Board{ + ID: boardID, + TeamID: teamID, + IsTemplate: true, + }, nil).Times(1) + + th.API.EXPECT().HasPermissionToChannel(userID, channelID, model.PermissionCreatePost).Return(false).Times(1) + _, err := th.App.PatchBoard(patch, boardID, userID) + require.Error(t, err) + }) + + t.Run("patch type channel, user with post permissions", func(t *testing.T) { + const boardID = "board_id_1" + const userID = "user_id_2" + const teamID = "team_id_1" + + channelID := "myChannel" + patch := &model.BoardPatch{ + ChannelID: &channelID, + } + + // Type not nil, will cause board to be reteived + // to check isTemplate + th.Store.EXPECT().GetBoard(boardID).Return(&model.Board{ + ID: boardID, + TeamID: teamID, + }, nil).Times(2) + + th.API.EXPECT().HasPermissionToChannel(userID, channelID, model.PermissionCreatePost).Return(true).Times(1) + + th.Store.EXPECT().PatchBoard(boardID, patch, userID).Return( + &model.Board{ + ID: boardID, + TeamID: teamID, + }, + nil) + + // Should call GetMembersForBoard 2 times + // - for WS BroadcastBoardChange + // - for AddTeamMembers check + th.Store.EXPECT().GetMembersForBoard(boardID).Return([]*model.BoardMember{}, nil).Times(2) + + th.Store.EXPECT().PostMessage(utils.Anything, "", "").Times(1) + + patchedBoard, err := th.App.PatchBoard(patch, boardID, userID) + require.NoError(t, err) + require.Equal(t, boardID, patchedBoard.ID) + }) + + t.Run("patch type remove channel, user without post permissions", func(t *testing.T) { + const boardID = "board_id_1" + const userID = "user_id_2" + const teamID = "team_id_1" + + const channelID = "myChannel" + clearChannel := "" + patchType := model.BoardTypeOpen + patch := &model.BoardPatch{ + Type: &patchType, + ChannelID: &clearChannel, + } + + // Type not nil, will cause board to be reteived + // to check isTemplate + th.Store.EXPECT().GetBoard(boardID).Return(&model.Board{ + ID: boardID, + TeamID: teamID, + IsTemplate: true, + ChannelID: channelID, + }, nil).Times(2) + + th.API.EXPECT().HasPermissionToChannel(userID, channelID, model.PermissionCreatePost).Return(false).Times(1) + + th.API.EXPECT().HasPermissionToTeam(userID, teamID, model.PermissionManageTeam).Return(false).Times(1) + // Should call GetMembersForBoard 2 times + // for WS BroadcastBoardChange + // for AddTeamMembers check + // We are returning the user as a direct Board Member, so BroadcastMemberDelete won't be called + th.Store.EXPECT().GetMembersForBoard(boardID).Return([]*model.BoardMember{{BoardID: boardID, UserID: userID, SchemeEditor: true}}, nil).Times(1) + + _, err := th.App.PatchBoard(patch, boardID, userID) + require.Error(t, err) + }) } func TestGetBoardCount(t *testing.T) {