Merge pull request #4620 from mattermost/MM-51062-fix-api

check permissions to channel before patching via api
mattermost-community · Mar 9, 2023 · 3a9f0fe · 3a9f0fe
2 parents 1842500 + b230064
commit 3a9f0fe
Show file tree

Hide file tree

Showing 2 changed files with 112 additions and 0 deletions.
diff --git 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
@@ -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) {