Forbid updating limited cards (#3127)

* Forbid updating limited cards

* Improve card limit access to avoid races and add get limits client endpoint

server/api/api.go

@@ -841,6 +841,10 @@ func (a *API) handlePatchBlock(w http.ResponseWriter, r *http.Request) {
 	auditRec.AddMeta("blockID", blockID)
 
 	err = a.app.PatchBlock(*container, blockID, patch, userID)
+	if errors.Is(err, app.ErrPatchUpdatesLimitedCards) {
+		a.errorResponse(w, r.URL.Path, http.StatusForbidden, "", err)
+		return
+	}
 	if err != nil {
 		a.errorResponse(w, r.URL.Path, http.StatusInternalServerError, "", err)
 		return
@@ -882,8 +886,6 @@ func (a *API) handlePatchBlocks(w http.ResponseWriter, r *http.Request) {
 	//     schema:
 	//       "$ref": "#/definitions/ErrorResponse"
 
-	// ToDo: prevent updates if cloud limited
-
 	ctx := r.Context()
 	session := ctx.Value(sessionContextKey).(*model.Session)
 	userID := session.UserID
@@ -914,6 +916,10 @@ func (a *API) handlePatchBlocks(w http.ResponseWriter, r *http.Request) {
 	}
 
 	err = a.app.PatchBlocks(*container, patches, userID)
+	if errors.Is(err, app.ErrPatchUpdatesLimitedCards) {
+		a.errorResponse(w, r.URL.Path, http.StatusForbidden, "", err)
+		return
+	}
 	if err != nil {
 		a.errorResponse(w, r.URL.Path, http.StatusInternalServerError, "", err)
 		return

server/app/app.go

@@ -1,6 +1,8 @@
 package app
 
 import (
+	"sync"
+
 	"github.com/mattermost/focalboard/server/auth"
 	"github.com/mattermost/focalboard/server/services/config"
 	"github.com/mattermost/focalboard/server/services/metrics"
@@ -38,8 +40,8 @@ type App struct {
 	logger        *mlog.Logger
 	pluginAPI     plugin.API
 
-	// ToDo: do we require a mutex?
-	CardLimit int
+	cardLimitMux sync.RWMutex
+	cardLimit    int
 }
 
 func (a *App) SetConfig(config *config.Configuration) {
@@ -61,3 +63,15 @@ func New(config *config.Configuration, wsAdapter ws.Adapter, services Services)
 	app.initialize(services.SkipTemplateInit)
 	return app
 }
+
+func (a *App) CardLimit() int {
+	a.cardLimitMux.RLock()
+	defer a.cardLimitMux.RUnlock()
+	return a.cardLimit
+}
+
+func (a *App) SetCardLimit(cardLimit int) {
+	a.cardLimitMux.Lock()
+	defer a.cardLimitMux.Unlock()
+	a.cardLimit = cardLimit
+}

server/app/blocks.go

@@ -14,6 +14,7 @@ import (
 )
 
 var ErrViewsLimitReached = errors.New("views limit reached for board")
+var ErrPatchUpdatesLimitedCards = errors.New("patch updates cards that are limited")
 
 func (a *App) GetBlocks(c store.Container, parentID string, blockType string) ([]model.Block, error) {
 	if blockType != "" && parentID != "" {
@@ -42,7 +43,17 @@ func (a *App) GetParentID(c store.Container, blockID string) (string, error) {
 func (a *App) PatchBlock(c store.Container, blockID string, blockPatch *model.BlockPatch, modifiedByID string) error {
 	oldBlock, err := a.store.GetBlock(c, blockID)
 	if err != nil {
-		return nil
+		return err
+	}
+
+	if a.IsCloudLimited() {
+		containsLimitedBlocks, lErr := a.ContainsLimitedBlocks(c, []model.Block{*oldBlock})
+		if lErr != nil {
+			return lErr
+		}
+		if containsLimitedBlocks {
+			return ErrPatchUpdatesLimitedCards
+		}
 	}
 
 	err = a.store.PatchBlock(c, blockID, blockPatch, modifiedByID)
@@ -53,7 +64,7 @@ func (a *App) PatchBlock(c store.Container, blockID string, blockPatch *model.Bl
 	a.metrics.IncrementBlocksPatched(1)
 	block, err := a.store.GetBlock(c, blockID)
 	if err != nil {
-		return nil
+		return err
 	}
 	a.wsAdapter.BroadcastBlockChange(c.WorkspaceID, *block)
 	go func() {
@@ -64,25 +75,30 @@ func (a *App) PatchBlock(c store.Container, blockID string, blockPatch *model.Bl
 }
 
 func (a *App) PatchBlocks(c store.Container, blockPatches *model.BlockPatchBatch, modifiedByID string) error {
-	oldBlocks := make([]model.Block, 0, len(blockPatches.BlockIDs))
-	for _, blockID := range blockPatches.BlockIDs {
-		oldBlock, err := a.store.GetBlock(c, blockID)
+	oldBlocks, err := a.store.GetBlocksByIDs(c, blockPatches.BlockIDs)
+	if err != nil {
+		return err
+	}
+
+	if a.IsCloudLimited() {
+		containsLimitedBlocks, err := a.ContainsLimitedBlocks(c, oldBlocks)
 		if err != nil {
-			return nil
+			return err
+		}
+		if containsLimitedBlocks {
+			return ErrPatchUpdatesLimitedCards
 		}
-		oldBlocks = append(oldBlocks, *oldBlock)
 	}
 
-	err := a.store.PatchBlocks(c, blockPatches, modifiedByID)
-	if err != nil {
+	if err := a.store.PatchBlocks(c, blockPatches, modifiedByID); err != nil {
 		return err
 	}
 
 	a.metrics.IncrementBlocksPatched(len(oldBlocks))
 	for i, blockID := range blockPatches.BlockIDs {
 		newBlock, err := a.store.GetBlock(c, blockID)
 		if err != nil {
-			return nil
+			return err
 		}
 		a.wsAdapter.BroadcastBlockChange(c.WorkspaceID, *newBlock)
 		go func(currentIndex int) {

server/app/blocks_test.go

@@ -1,13 +1,16 @@
 package app
 
 import (
+	"database/sql"
 	"testing"
 
-	"github.com/mattermost/focalboard/server/model"
-
 	"github.com/golang/mock/gomock"
-	st "github.com/mattermost/focalboard/server/services/store"
 	"github.com/stretchr/testify/require"
+
+	mmModel "github.com/mattermost/mattermost-server/v6/model"
+
+	"github.com/mattermost/focalboard/server/model"
+	st "github.com/mattermost/focalboard/server/services/store"
 )
 
 type blockError struct {
@@ -71,17 +74,63 @@ func TestPatchBlocks(t *testing.T) {
 		WorkspaceID: "0",
 	}
 	t.Run("patchBlocks success scenerio", func(t *testing.T) {
-		blockPatches := model.BlockPatchBatch{}
+		blockPatches := model.BlockPatchBatch{
+			BlockIDs: []string{"block1"},
+			BlockPatches: []model.BlockPatch{
+				{Title: mmModel.NewString("new title")},
+			},
+		}
+
+		block1 := model.Block{ID: "block1"}
+		th.Store.EXPECT().GetBlocksByIDs(container, []string{"block1"}).Return([]model.Block{block1}, nil)
 		th.Store.EXPECT().PatchBlocks(gomock.Eq(container), gomock.Eq(&blockPatches), gomock.Eq("user-id-1")).Return(nil)
+		th.Store.EXPECT().GetBlock(container, "block1").Return(&block1, nil)
 		err := th.App.PatchBlocks(container, &blockPatches, "user-id-1")
 		require.NoError(t, err)
 	})
 
 	t.Run("patchBlocks error scenerio", func(t *testing.T) {
-		blockPatches := model.BlockPatchBatch{}
-		th.Store.EXPECT().PatchBlocks(gomock.Eq(container), gomock.Eq(&blockPatches), gomock.Eq("user-id-1")).Return(blockError{"error"})
+		blockPatches := model.BlockPatchBatch{BlockIDs: []string{}}
+		th.Store.EXPECT().GetBlocksByIDs(container, []string{}).Return(nil, sql.ErrNoRows)
 		err := th.App.PatchBlocks(container, &blockPatches, "user-id-1")
-		require.Error(t, err, "error")
+		require.ErrorIs(t, err, sql.ErrNoRows)
+	})
+
+	t.Run("cloud limit error scenario", func(t *testing.T) {
+		th.App.SetCardLimit(5)
+
+		fakeLicense := &mmModel.License{
+			Features: &mmModel.Features{Cloud: mmModel.NewBool(true)},
+		}
+
+		blockPatches := model.BlockPatchBatch{
+			BlockIDs: []string{"block1"},
+			BlockPatches: []model.BlockPatch{
+				{Title: mmModel.NewString("new title")},
+			},
+		}
+
+		block1 := model.Block{
+			ID:       "block1",
+			Type:     model.TypeCard,
+			ParentID: "board1",
+			RootID:   "board1",
+			UpdateAt: 100,
+		}
+
+		board1 := model.Block{
+			ID:       "board1",
+			Type:     model.TypeBoard,
+			ParentID: "board1",
+			RootID:   "board1",
+		}
+
+		th.Store.EXPECT().GetBlocksByIDs(container, []string{"block1"}).Return([]model.Block{block1}, nil)
+		th.Store.EXPECT().GetLicense().Return(fakeLicense)
+		th.Store.EXPECT().GetCardLimitTimestamp().Return(int64(150), nil)
+		th.Store.EXPECT().GetBlocksByIDs(container, []string{"board1"}).Return([]model.Block{board1}, nil)
+		err := th.App.PatchBlocks(container, &blockPatches, "user-id-1")
+		require.ErrorIs(t, err, ErrPatchUpdatesLimitedCards)
 	})
 }
 

server/app/cloud.go

@@ -60,22 +60,22 @@ func (a *App) IsCloud() bool {
 // IsCloudLimited returns true if the server is running in cloud mode
 // and the card limit has been set.
 func (a *App) IsCloudLimited() bool {
-	return a.CardLimit != 0 && a.IsCloud()
+	return a.CardLimit() != 0 && a.IsCloud()
 }
 
 // SetCloudLimits sets the limits of the server.
 func (a *App) SetCloudLimits(limits *mmModel.ProductLimits) error {
-	oldCardLimit := a.CardLimit
+	oldCardLimit := a.CardLimit()
 
 	// if the limit object doesn't come complete, we assume limits are
 	// being disabled
 	cardLimit := 0
 	if limits != nil && limits.Boards != nil {
 		cardLimit = *limits.Boards.Cards
 	}
-	a.CardLimit = cardLimit
 
 	if oldCardLimit != cardLimit {
+		a.SetCardLimit(cardLimit)
 		return a.doUpdateCardLimitTimestamp()
 	}
 
@@ -85,7 +85,7 @@ func (a *App) SetCloudLimits(limits *mmModel.ProductLimits) error {
 // doUpdateCardLimitTimestamp performs the update without running any
 // checks.
 func (a *App) doUpdateCardLimitTimestamp() error {
-	cardLimitTimestamp, err := a.store.UpdateCardLimitTimestamp(a.CardLimit)
+	cardLimitTimestamp, err := a.store.UpdateCardLimitTimestamp(a.CardLimit())
 	if err != nil {
 		return err
 	}
@@ -202,8 +202,7 @@ func (a *App) ApplyCloudLimits(c store.Container, blocks []model.Block) ([]model
 			continue
 		}
 
-		if block.Type == model.TypeCard &&
-			block.UpdateAt < cardLimitTimestamp {
+		if block.ShouldBeLimited(cardLimitTimestamp) {
 			limitedBlocks[i] = block.GetLimited()
 		} else {
 			limitedBlocks[i] = block
@@ -213,6 +212,80 @@ func (a *App) ApplyCloudLimits(c store.Container, blocks []model.Block) ([]model
 	return limitedBlocks, nil
 }
 
+// ContainsLimitedBlocks checks if a list of block IDs contain any
+// block that references a limited card.
+func (a *App) ContainsLimitedBlocks(c store.Container, blocks []model.Block) (bool, error) {
+	cardLimitTimestamp, err := a.store.GetCardLimitTimestamp()
+	if err != nil {
+		return false, err
+	}
+
+	if cardLimitTimestamp == 0 {
+		return false, nil
+	}
+
+	cards := []model.Block{}
+	cardIDMap := map[string]bool{}
+	for _, block := range blocks {
+		switch block.Type {
+		case model.TypeBoard:
+		case model.TypeCard:
+			cards = append(cards, block)
+		default:
+			cardIDMap[block.ParentID] = true
+		}
+	}
+
+	cardIDs := []string{}
+	// if the card is already present on the set, we don't need to
+	// fetch it from the database
+	for cardID := range cardIDMap {
+		alreadyPresent := false
+		for _, card := range cards {
+			if card.ID == cardID {
+				alreadyPresent = true
+				break
+			}
+		}
+
+		if !alreadyPresent {
+			cardIDs = append(cardIDs, cardID)
+		}
+	}
+
+	if len(cardIDs) > 0 {
+		fetchedCards, fErr := a.store.GetBlocksByIDs(c, cardIDs)
+		if fErr != nil {
+			return false, fErr
+		}
+		cards = append(cards, fetchedCards...)
+	}
+
+	templateMap, err := a.getTemplateMapForBlocks(c, blocks)
+	if err != nil {
+		return false, err
+	}
+
+	for _, card := range cards {
+		isTemplate, ok := templateMap[card.RootID]
+		if !ok {
+			return false, newErrBoardNotFoundInTemplateMap(card.RootID)
+		}
+
+		// if the block belongs to a template, it will never be
+		// limited
+		if isTemplate {
+			continue
+		}
+
+		if card.ShouldBeLimited(cardLimitTimestamp) {
+			return true, nil
+		}
+	}
+
+	return false, nil
+}
+
 func (a *App) NotifyPortalAdminsUpgradeRequest(workspaceID string) error {
 	if a.pluginAPI == nil {
 		return ErrNilPluginAPI