diff --git a/pkg/api/folder.go b/pkg/api/folder.go index 78544cacedf..6c2c1174366 100644 --- a/pkg/api/folder.go +++ b/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) } diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index a274e2bcc61..491e6940cc6 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/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 - 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{ + cmd := &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, - ParentUID: parentUID, - }) - if err != nil { + Description: cmd.Description, + ParentUID: cmd.ParentUID, + } + 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) - err = s.DeleteFolder(ctx, &folder.DeleteFolderCommand{UID: createdFolder.UID, OrgID: cmd.OrgID, ForceDeleteRules: true}) - if err != nil { - s.log.Error("error deleting folder after failed save to nested folder store", err) + logger.Error("error saving folder to nested folder store", "error", err) + // do not shallow create error if the legacy folder delete fails + deleteErr := s.DeleteFolder(ctx, &folder.DeleteFolderCommand{UID: createdFolder.UID, OrgID: cmd.OrgID, ForceDeleteRules: true, SignedInUser: user}) + 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 diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index 6a30e6e5485..fc2785e8847 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/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(ctx, &folder.CreateFolderCommand{ + _, err := service.Create(context.Background(), &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(ctx, &folder.CreateFolderCommand{ + actualFolder, err := service.Create(context.Background(), &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(ctx, &folder.CreateFolderCommand{ + _, err := service.Create(context.Background(), &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() - ctx = appcontext.WithUser(ctx, usr) - err := service.DeleteFolder(ctx, &folder.DeleteFolderCommand{ + err := service.DeleteFolder(context.Background(), &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(ctx, &folder.CreateFolderCommand{SignedInUser: usr}) + res, err := folderService.Create(context.Background(), &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(ctx, &folder.CreateFolderCommand{ + _, err := foldersvc.Create(context.Background(), &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(ctx, &folder.CreateFolderCommand{ + _, err := foldersvc.Create(context.Background(), &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(ctx, &folder.CreateFolderCommand{ + _, err := foldersvc.Create(context.Background(), &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) + }) }) } diff --git a/pkg/services/folder/folderimpl/sqlstore.go b/pkg/services/folder/folderimpl/sqlstore.go index 92390db472f..b8dbfbe9351 100644 --- a/pkg/services/folder/folderimpl/sqlstore.go +++ b/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 - createdBy := user.UserID + updatedBy := cmd.SignedInUser.UserID + createdBy := cmd.SignedInUser.UserID */ err := ss.db.WithDbSession(ctx, func(sess *db.Session) error { var sqlOrArgs []interface{} diff --git a/pkg/services/folder/folderimpl/sqlstore_test.go b/pkg/services/folder/folderimpl/sqlstore_test.go index 6a6dcb85182..4824d934d38 100644 --- a/pkg/services/folder/folderimpl/sqlstore_test.go +++ b/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) {