Nested Folder: Enforce maximum nested folder depth (#59213)

* Nested Folder: Enforce maximum nested folder depth

* Cleanup

* Fix logging

pkg/api/folder.go

@@ -212,7 +212,7 @@ func (hs *HTTPServer) DeleteFolder(c *models.ReqContext) response.Response { //
 	}
 
 	uid := web.Params(c.Req)[":uid"]
-	err = hs.folderService.DeleteFolder(c.Req.Context(), &folder.DeleteFolderCommand{UID: uid, OrgID: c.OrgID, ForceDeleteRules: c.QueryBool("forceDeleteRules")})
+	err = hs.folderService.DeleteFolder(c.Req.Context(), &folder.DeleteFolderCommand{UID: uid, OrgID: c.OrgID, ForceDeleteRules: c.QueryBool("forceDeleteRules"), SignedInUser: c.SignedInUser})
 	if err != nil {
 		return apierrors.ToFolderErrorResponse(err)
 	}

pkg/services/folder/folderimpl/folder.go

@@ -7,7 +7,6 @@ import (
 
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/events"
-	"github.com/grafana/grafana/pkg/infra/appcontext"
 	"github.com/grafana/grafana/pkg/infra/db"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/models"
@@ -206,6 +205,8 @@ func (s *Service) getFolderByTitle(ctx context.Context, user *user.SignedInUser,
 }
 
 func (s *Service) Create(ctx context.Context, cmd *folder.CreateFolderCommand) (*folder.Folder, error) {
+	logger := s.log.FromContext(ctx)
+
 	dashFolder := models.NewDashboardFolder(cmd.Title)
 	dashFolder.OrgId = cmd.OrgID
 
@@ -270,36 +271,28 @@ func (s *Service) Create(ctx context.Context, cmd *folder.CreateFolderCommand) (
 	}
 
 	if permissionErr != nil {
-		s.log.Error("Could not make user admin", "folder", createdFolder.Title, "user", userID, "error", permissionErr)
+		logger.Error("Could not make user admin", "folder", createdFolder.Title, "user", userID, "error", permissionErr)
 	}
 
 	if s.features.IsEnabled(featuremgmt.FlagNestedFolders) {
-		var description string
+		cmd := &folder.CreateFolderCommand{
-		if dash.Data != nil {
-			description = dash.Data.Get("description").MustString()
-		}
-
-		parentUID := folder.RootFolderUID
-		if cmd.ParentUID != "" {
-			parentUID = cmd.ParentUID
-		}
-		_, err := s.store.Create(ctx, folder.CreateFolderCommand{
 			// TODO: Today, if a UID isn't specified, the dashboard store
 			// generates a new UID. The new folder store will need to do this as
 			// well, but for now we take the UID from the newly created folder.
 			UID:         dash.Uid,
 			OrgID:       cmd.OrgID,
 			Title:       cmd.Title,
-			Description: description,
+			Description: cmd.Description,
-			ParentUID:   parentUID,
+			ParentUID:   cmd.ParentUID,
-		})
+		}
-		if err != nil {
+		if err := s.nestedFolderCreate(ctx, cmd); err != nil {
 			// We'll log the error and also roll back the previously-created
 			// (legacy) folder.
-			s.log.Error("error saving folder to nested folder store", err)
+			logger.Error("error saving folder to nested folder store", "error", err)
-			err = s.DeleteFolder(ctx, &folder.DeleteFolderCommand{UID: createdFolder.UID, OrgID: cmd.OrgID, ForceDeleteRules: true})
+			// do not shallow create error if the legacy folder delete fails
-			if err != nil {
+			deleteErr := s.DeleteFolder(ctx, &folder.DeleteFolderCommand{UID: createdFolder.UID, OrgID: cmd.OrgID, ForceDeleteRules: true, SignedInUser: user})
-				s.log.Error("error deleting folder after failed save to nested folder store", err)
+			if deleteErr != nil {
+				logger.Error("error deleting folder after failed save to nested folder store", "error", err)
 			}
 			return folder.FromDashboard(dash), err
 		}
@@ -346,6 +339,8 @@ func (s *Service) Update(ctx context.Context, user *user.SignedInUser, orgID int
 }
 
 func (s *Service) legacyUpdate(ctx context.Context, user *user.SignedInUser, orgID int64, existingUid string, cmd *models.UpdateFolderCommand) (*folder.Folder, error) {
+	logger := s.log.FromContext(ctx)
+
 	query := models.GetDashboardQuery{OrgId: orgID, Uid: existingUid}
 	_, err := s.dashboardStore.GetDashboard(ctx, &query)
 	if err != nil {
@@ -392,30 +387,31 @@ func (s *Service) legacyUpdate(ctx context.Context, user *user.SignedInUser, org
 			UID:       dash.Uid,
 			OrgID:     orgID,
 		}); err != nil {
-			s.log.Error("failed to publish FolderTitleUpdated event", "folder", foldr.Title, "user", user.UserID, "error", err)
+			logger.Error("failed to publish FolderTitleUpdated event", "folder", foldr.Title, "user", user.UserID, "error", err)
 		}
 	}
 	return foldr, nil
 }
 
 func (s *Service) DeleteFolder(ctx context.Context, cmd *folder.DeleteFolderCommand) error {
+	logger := s.log.FromContext(ctx)
+	if cmd.SignedInUser == nil {
+		return folder.ErrBadRequest.Errorf("missing signed in user")
+	}
+
 	if s.features.IsEnabled(featuremgmt.FlagNestedFolders) {
 		err := s.Delete(ctx, cmd)
 		if err != nil {
-			s.log.Error("the delete folder on folder table failed with err: ", err.Error())
+			logger.Error("the delete folder on folder table failed with err: ", "error", err)
 		}
 	}
-	user, err := appcontext.User(ctx)
-	if err != nil {
-		return err
-	}
 
 	dashFolder, err := s.dashboardStore.GetFolderByUID(ctx, cmd.OrgID, cmd.UID)
 	if err != nil {
 		return err
 	}
 
-	guard := guardian.New(ctx, dashFolder.ID, cmd.OrgID, user)
+	guard := guardian.New(ctx, dashFolder.ID, cmd.OrgID, cmd.SignedInUser)
 	if canSave, err := guard.CanDelete(); err != nil || !canSave {
 		if err != nil {
 			return toFolderError(err)
@@ -498,6 +494,29 @@ func (s *Service) MakeUserAdmin(ctx context.Context, orgID int64, userID, folder
 	return s.dashboardService.MakeUserAdmin(ctx, orgID, userID, folderID, setViewAndEditPermissions)
 }
 
+func (s *Service) nestedFolderCreate(ctx context.Context, cmd *folder.CreateFolderCommand) error {
+	if cmd.ParentUID != "" {
+		if err := s.validateParent(ctx, cmd.OrgID, cmd.ParentUID); err != nil {
+			return err
+		}
+	}
+	_, err := s.store.Create(ctx, *cmd)
+	return err
+}
+
+func (s *Service) validateParent(ctx context.Context, orgID int64, parentUID string) error {
+	ancestors, err := s.store.GetParents(ctx, folder.GetParentsQuery{UID: parentUID, OrgID: orgID})
+	if err != nil {
+		return err
+	}
+
+	if len(ancestors) == folder.MaxNestedFolderDepth {
+		return folder.ErrMaximumDepthReached
+	}
+
+	return nil
+}
+
 func toFolderError(err error) error {
 	if errors.Is(err, dashboards.ErrDashboardTitleEmpty) {
 		return dashboards.ErrFolderTitleEmpty

pkg/services/folder/folderimpl/folder_test.go

@@ -3,6 +3,7 @@ package folderimpl
 import (
 	"context"
 	"errors"
+	"fmt"
 	"math/rand"
 	"testing"
 
@@ -12,7 +13,6 @@ import (
 	"github.com/xorcare/pointer"
 
 	"github.com/grafana/grafana/pkg/bus"
-	"github.com/grafana/grafana/pkg/infra/appcontext"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/infra/tracing"
 	"github.com/grafana/grafana/pkg/models"
@@ -117,8 +117,7 @@ func TestIntegrationFolderService(t *testing.T) {
 
 			t.Run("When creating folder should return access denied error", func(t *testing.T) {
 				dashStore.On("ValidateDashboardBeforeSave", mock.Anything, mock.AnythingOfType("*models.Dashboard"), mock.AnythingOfType("bool")).Return(true, nil).Times(2)
-				ctx := appcontext.WithUser(context.Background(), usr)
+				_, err := service.Create(context.Background(), &folder.CreateFolderCommand{
-				_, err := service.Create(ctx, &folder.CreateFolderCommand{
 					OrgID:        orgID,
 					Title:        f.Title,
 					UID:          folderUID,
@@ -141,16 +140,13 @@ func TestIntegrationFolderService(t *testing.T) {
 			})
 
 			t.Run("When deleting folder by uid should return access denied error", func(t *testing.T) {
-				ctx := context.Background()
-				ctx = appcontext.WithUser(ctx, usr)
-
 				newFolder := models.NewFolder("Folder")
 				newFolder.Uid = folderUID
 
 				dashStore.On("GetFolderByID", mock.Anything, orgID, folderId).Return(newFolder, nil)
 				dashStore.On("GetFolderByUID", mock.Anything, orgID, folderUID).Return(newFolder, nil)
 
-				err := service.DeleteFolder(ctx, &folder.DeleteFolderCommand{
+				err := service.DeleteFolder(context.Background(), &folder.DeleteFolderCommand{
 					UID:              folderUID,
 					OrgID:            orgID,
 					ForceDeleteRules: false,
@@ -178,8 +174,7 @@ func TestIntegrationFolderService(t *testing.T) {
 				dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("models.SaveDashboardCommand")).Return(dash, nil).Once()
 				dashStore.On("GetFolderByID", mock.Anything, orgID, dash.Id).Return(f, nil)
 
-				ctx := appcontext.WithUser(context.Background(), usr)
+				actualFolder, err := service.Create(context.Background(), &folder.CreateFolderCommand{
-				actualFolder, err := service.Create(ctx, &folder.CreateFolderCommand{
 					OrgID:        orgID,
 					Title:        dash.Title,
 					UID:          "someuid",
@@ -193,8 +188,7 @@ func TestIntegrationFolderService(t *testing.T) {
 				dash := models.NewDashboardFolder("Test-Folder")
 				dash.Id = rand.Int63()
 
-				ctx := appcontext.WithUser(context.Background(), usr)
+				_, err := service.Create(context.Background(), &folder.CreateFolderCommand{
-				_, err := service.Create(ctx, &folder.CreateFolderCommand{
 					OrgID:        orgID,
 					Title:        dash.Title,
 					UID:          "general",
@@ -235,9 +229,7 @@ func TestIntegrationFolderService(t *testing.T) {
 				}).Return(nil).Once()
 
 				expectedForceDeleteRules := rand.Int63()%2 == 0
-				ctx := context.Background()
+				err := service.DeleteFolder(context.Background(), &folder.DeleteFolderCommand{
-				ctx = appcontext.WithUser(ctx, usr)
-				err := service.DeleteFolder(ctx, &folder.DeleteFolderCommand{
 					UID:              f.UID,
 					OrgID:            orgID,
 					ForceDeleteRules: expectedForceDeleteRules,
@@ -338,11 +330,11 @@ func TestNestedFolderServiceFeatureToggle(t *testing.T) {
 		dashboardStore:   &dashStore,
 		dashboardService: &dashboardsvc,
 		features:         featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders),
+		log:              log.New("test-folder-service"),
 	}
 	t.Run("create folder", func(t *testing.T) {
 		folderStore.ExpectedFolder = &folder.Folder{}
-		ctx := appcontext.WithUser(context.Background(), usr)
+		res, err := folderService.Create(context.Background(), &folder.CreateFolderCommand{SignedInUser: usr})
-		res, err := folderService.Create(ctx, &folder.CreateFolderCommand{SignedInUser: usr})
 		require.NoError(t, err)
 		require.NotNil(t, res.UID)
 	})
@@ -379,7 +371,6 @@ func TestNestedFolderServiceFeatureToggle(t *testing.T) {
 
 func TestNestedFolderService(t *testing.T) {
 	t.Run("with feature flag unset", func(t *testing.T) {
-		ctx := appcontext.WithUser(context.Background(), usr)
 		store := &FakeStore{}
 		dashStore := dashboards.FakeDashboardStore{}
 		dashboardsvc := dashboards.FakeDashboardService{}
@@ -403,8 +394,7 @@ func TestNestedFolderService(t *testing.T) {
 			dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("models.SaveDashboardCommand")).Return(&models.Dashboard{}, nil)
 			dashStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&folder.Folder{}, nil)
 
-			ctx = appcontext.WithUser(ctx, usr)
+			_, err := foldersvc.Create(context.Background(), &folder.CreateFolderCommand{
-			_, err := foldersvc.Create(ctx, &folder.CreateFolderCommand{
 				OrgID:        orgID,
 				Title:        "myFolder",
 				UID:          "myFolder",
@@ -425,7 +415,7 @@ func TestNestedFolderService(t *testing.T) {
 			g := guardian.New
 			guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true})
 
-			err := foldersvc.DeleteFolder(ctx, &folder.DeleteFolderCommand{UID: "myFolder", OrgID: orgID, SignedInUser: usr})
+			err := foldersvc.DeleteFolder(context.Background(), &folder.DeleteFolderCommand{UID: "myFolder", OrgID: orgID, SignedInUser: usr})
 			require.NoError(t, err)
 			require.NotNil(t, actualCmd)
 
@@ -437,7 +427,6 @@ func TestNestedFolderService(t *testing.T) {
 	})
 
 	t.Run("with nested folder feature flag on", func(t *testing.T) {
-		ctx := appcontext.WithUser(context.Background(), usr)
 		store := &FakeStore{}
 		dashStore := &dashboards.FakeDashboardStore{}
 		dashboardsvc := &dashboards.FakeDashboardService{}
@@ -463,8 +452,7 @@ func TestNestedFolderService(t *testing.T) {
 				mock.AnythingOfType("bool"), mock.AnythingOfType("bool")).Return(&models.SaveDashboardCommand{}, nil)
 			dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("models.SaveDashboardCommand")).Return(&models.Dashboard{}, nil)
 			dashStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&folder.Folder{}, nil)
-			ctx = appcontext.WithUser(ctx, usr)
+			_, err := foldersvc.Create(context.Background(), &folder.CreateFolderCommand{
-			_, err := foldersvc.Create(ctx, &folder.CreateFolderCommand{
 				OrgID:        orgID,
 				Title:        "myFolder",
 				UID:          "myFolder",
@@ -492,8 +480,7 @@ func TestNestedFolderService(t *testing.T) {
 			store.ExpectedError = errors.New("FAILED")
 
 			// the service return success as long as the legacy create succeeds
-			ctx = appcontext.WithUser(ctx, usr)
+			_, err := foldersvc.Create(context.Background(), &folder.CreateFolderCommand{
-			_, err := foldersvc.Create(ctx, &folder.CreateFolderCommand{
 				OrgID:        orgID,
 				Title:        "myFolder",
 				UID:          "myFolder",
@@ -512,7 +499,7 @@ func TestNestedFolderService(t *testing.T) {
 		t.Run("move, no error", func(t *testing.T) {
 			store.ExpectedError = nil
 			store.ExpectedFolder = &folder.Folder{UID: "myFolder", ParentUID: "newFolder"}
-			f, err := foldersvc.Move(ctx, &folder.MoveFolderCommand{UID: "myFolder", NewParentUID: "newFolder", OrgID: orgID, SignedInUser: usr})
+			f, err := foldersvc.Move(context.Background(), &folder.MoveFolderCommand{UID: "myFolder", NewParentUID: "newFolder", OrgID: orgID, SignedInUser: usr})
 			require.NoError(t, err)
 			require.NotNil(t, f)
 		})
@@ -527,7 +514,7 @@ func TestNestedFolderService(t *testing.T) {
 			g := guardian.New
 			guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true})
 
-			err := foldersvc.DeleteFolder(ctx, &folder.DeleteFolderCommand{UID: "myFolder", OrgID: orgID, SignedInUser: usr})
+			err := foldersvc.DeleteFolder(context.Background(), &folder.DeleteFolderCommand{UID: "myFolder", OrgID: orgID, SignedInUser: usr})
 			require.NoError(t, err)
 			require.NotNil(t, actualCmd)
 
@@ -536,5 +523,43 @@ func TestNestedFolderService(t *testing.T) {
 			})
 			require.True(t, store.DeleteCalled)
 		})
+
+		t.Run("create returns error if maximum depth reached", func(t *testing.T) {
+			// This test creates and deletes the dashboard, so needs some extra setup.
+			g := guardian.New
+			guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true})
+			t.Cleanup(func() {
+				guardian.New = g
+			})
+
+			// dashboard store & service commands that should be called.
+			dashboardsvc.On("BuildSaveDashboardCommand",
+				mock.Anything, mock.AnythingOfType("*dashboards.SaveDashboardDTO"),
+				mock.AnythingOfType("bool"), mock.AnythingOfType("bool")).Return(&models.SaveDashboardCommand{}, nil)
+			dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("models.SaveDashboardCommand")).Return(&models.Dashboard{}, nil)
+			dashStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&folder.Folder{}, nil)
+			dashStore.On("GetFolderByUID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("string")).Return(&folder.Folder{}, nil)
+			var actualCmd *models.DeleteDashboardCommand
+			dashStore.On("DeleteDashboard", mock.Anything, mock.Anything).Run(func(args mock.Arguments) {
+				actualCmd = args.Get(1).(*models.DeleteDashboardCommand)
+			}).Return(nil).Once()
+
+			parents := make([]*folder.Folder, 0, folder.MaxNestedFolderDepth)
+			for i := 0; i < folder.MaxNestedFolderDepth; i++ {
+				parents = append(parents, &folder.Folder{UID: fmt.Sprintf("folder%d", i)})
+			}
+			store.ExpectedFolders = parents
+			store.ExpectedError = nil
+
+			_, err := foldersvc.Create(context.Background(), &folder.CreateFolderCommand{
+				Title:        "folder",
+				OrgID:        orgID,
+				ParentUID:    parents[len(parents)-1].UID,
+				UID:          util.GenerateShortUID(),
+				SignedInUser: usr,
+			})
+			assert.ErrorIs(t, err, folder.ErrMaximumDepthReached)
+			require.NotNil(t, actualCmd)
+		})
 	})
 }

pkg/services/folder/folderimpl/sqlstore.go

@@ -38,13 +38,9 @@ func (ss *sqlStore) Create(ctx context.Context, cmd folder.CreateFolderCommand)
 
 	var foldr *folder.Folder
 	/*
-		user, err := appcontext.User(ctx)
-		if err != nil {
-			return nil, err
-		}
 		version := 1
-		updatedBy := user.UserID
+		updatedBy := cmd.SignedInUser.UserID
-		createdBy := user.UserID
+		createdBy := cmd.SignedInUser.UserID
 	*/
 	err := ss.db.WithDbSession(ctx, func(sess *db.Session) error {
 		var sqlOrArgs []interface{}

pkg/services/folder/folderimpl/sqlstore_test.go

@@ -133,28 +133,6 @@ func TestIntegrationCreate(t *testing.T) {
 		assert.Equal(t, "folder desc", ff.Description)
 		assert.Equal(t, parentUID, ff.ParentUID)
 	})
-
-	/*
-		t.Run("creating a nested folder with the maximum nested folder depth should fail", func(t *testing.T) {
-			ancestorUIDs := createSubTree(t, folderStore, orgID, accesscontrol.GeneralFolderUID, folder.MaxNestedFolderDepth, "")
-
-			t.Cleanup(func() {
-				for _, uid := range ancestorUIDs[1:] {
-					err := folderStore.Delete(context.Background(), uid, orgID)
-					require.NoError(t, err)
-				}
-			})
-
-			title := fmt.Sprintf("folder-%d", len(ancestorUIDs))
-			_, err := folderStore.Create(context.Background(), folder.CreateFolderCommand{
-				Title:     "folder1",
-				OrgID:     orgID,
-				ParentUID: ancestorUIDs[len(ancestorUIDs)-1],
-				UID:       util.GenerateShortUID(),
-			})
-			assert.Error(t, err)
-		})
-	*/
 }
 
 func TestIntegrationDelete(t *testing.T) {