diff --git a/pkg/api/folder.go b/pkg/api/folder.go index 2c85ef59b77..aa15065ccbc 100644 --- a/pkg/api/folder.go +++ b/pkg/api/folder.go @@ -49,7 +49,6 @@ func (hs *HTTPServer) registerFolderAPI(apiRoute routing.RouteRegister, authoriz folderRoute.Get("/id/:id", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersRead, idScope)), routing.Wrap(hs.GetFolderByID)) folderRoute.Group("/:uid", func(folderUidRoute routing.RouteRegister) { - folderUidRoute.Post("/move", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersWrite, uidScope)), routing.Wrap(hs.MoveFolder)) folderUidRoute.Group("/permissions", func(folderPermissionRoute routing.RouteRegister) { folderPermissionRoute.Get("/", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersPermissionsRead, uidScope)), routing.Wrap(hs.GetFolderPermissionList)) folderPermissionRoute.Post("/", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersPermissionsWrite, uidScope)), routing.Wrap(hs.UpdateFolderPermissions)) @@ -65,6 +64,7 @@ func (hs *HTTPServer) registerFolderAPI(apiRoute routing.RouteRegister, authoriz folderUidRoute.Delete("/", handler.deleteFolder) folderUidRoute.Get("/", handler.getFolder) folderUidRoute.Get("/counts", handler.countFolderContent) + folderUidRoute.Post("/move", handler.moveFolder) }) } else { folderRoute.Post("/", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersCreate)), routing.Wrap(hs.CreateFolder)) @@ -74,6 +74,7 @@ func (hs *HTTPServer) registerFolderAPI(apiRoute routing.RouteRegister, authoriz folderUidRoute.Delete("/", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersDelete, uidScope)), routing.Wrap(hs.DeleteFolder)) folderUidRoute.Get("/", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersRead, uidScope)), routing.Wrap(hs.GetFolderByUID)) folderUidRoute.Get("/counts", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersRead, uidScope)), routing.Wrap(hs.GetFolderDescendantCounts)) + folderUidRoute.Post("/move", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersWrite, uidScope)), routing.Wrap(hs.MoveFolder)) }) } }) @@ -844,7 +845,51 @@ func (fk8s *folderK8sHandler) updateFolder(c *contextmodel.ReqContext) { return } - out, err := client.Update(c.Req.Context(), &obj, v1.UpdateOptions{}) + out, err := client.Update(c.Req.Context(), obj, v1.UpdateOptions{}) + if err != nil { + fk8s.writeError(c, err) + return + } + + folderDTO, err := fk8s.newToFolderDto(c, *out, c.SignedInUser.GetOrgID()) + if err != nil { + fk8s.writeError(c, err) + return + } + + c.JSON(http.StatusOK, folderDTO) +} + +func (fk8s *folderK8sHandler) moveFolder(c *contextmodel.ReqContext) { + client, ok := fk8s.getClient(c) + if !ok { + return + } + + ctx := c.Req.Context() + + cmd := folder.MoveFolderCommand{} + if err := web.Bind(c.Req, &cmd); err != nil { + c.JsonApiErr(http.StatusBadRequest, "bad request data", err) + return + } + cmd.OrgID = c.SignedInUser.GetOrgID() + cmd.UID = web.Params(c.Req)[":uid"] + cmd.SignedInUser = c.SignedInUser + + obj, err := client.Get(ctx, cmd.UID, v1.GetOptions{}) + if err != nil { + fk8s.writeError(c, err) + return + } + + obj, err = internalfolders.LegacyMoveCommandToUnstructured(obj, cmd) + if err != nil { + fk8s.writeError(c, err) + return + } + + out, err := client.Update(c.Req.Context(), obj, v1.UpdateOptions{}) if err != nil { fk8s.writeError(c, err) return diff --git a/pkg/registry/apis/folders/conversions.go b/pkg/registry/apis/folders/conversions.go index 456771c8964..83662596deb 100644 --- a/pkg/registry/apis/folders/conversions.go +++ b/pkg/registry/apis/folders/conversions.go @@ -41,9 +41,9 @@ func LegacyCreateCommandToUnstructured(cmd folder.CreateFolderCommand) (unstruct return obj, nil } -func LegacyUpdateCommandToUnstructured(cmd folder.UpdateFolderCommand) (unstructured.Unstructured, error) { +func LegacyUpdateCommandToUnstructured(cmd folder.UpdateFolderCommand) (*unstructured.Unstructured, error) { // #TODO add other fields ; do we support updating the UID/orgID? - obj := unstructured.Unstructured{ + obj := &unstructured.Unstructured{ Object: map[string]interface{}{ "spec": map[string]interface{}{ "title": cmd.NewTitle, @@ -56,8 +56,16 @@ func LegacyUpdateCommandToUnstructured(cmd folder.UpdateFolderCommand) (unstruct if cmd.NewParentUID == nil { return obj, nil } - if err := setParentUID(&obj, *cmd.NewParentUID); err != nil { - return unstructured.Unstructured{}, err + if err := setParentUID(obj, *cmd.NewParentUID); err != nil { + return &unstructured.Unstructured{}, err + } + + return obj, nil +} + +func LegacyMoveCommandToUnstructured(obj *unstructured.Unstructured, cmd folder.MoveFolderCommand) (*unstructured.Unstructured, error) { + if err := setParentUID(obj, cmd.NewParentUID); err != nil { + return &unstructured.Unstructured{}, err } return obj, nil diff --git a/pkg/registry/apis/folders/register.go b/pkg/registry/apis/folders/register.go index dd4a4d3c826..3f9f868c31e 100644 --- a/pkg/registry/apis/folders/register.go +++ b/pkg/registry/apis/folders/register.go @@ -257,7 +257,11 @@ func (b *FolderAPIBuilder) Validate(ctx context.Context, a admission.Attributes, case admission.Delete: return b.validateOnDelete(ctx, f) case admission.Update: - return nil + old := a.GetOldObject() + if old == nil { + return fmt.Errorf("old object is nil") + } + return b.validateOnUpdate(ctx, obj, old) case admission.Connect: return nil } @@ -302,22 +306,12 @@ func (b *FolderAPIBuilder) validateOnCreate(ctx context.Context, id string, obj return dashboards.ErrFolderTitleEmpty } - for i := 1; i <= folderValidationRules.maxDepth; i++ { - parent := getParent(obj) - if parent == "" { - break - } - if i == folderValidationRules.maxDepth { - return folder.ErrMaximumDepthReached - } - - parentObj, err := b.storage.Get(ctx, parent, &metav1.GetOptions{}) - if err != nil { - return err - } - obj = parentObj + _, err := b.checkFolderMaxDepth(ctx, obj) + if err != nil { + return err } - return nil + + return err } func getParent(o runtime.Object) string { @@ -327,3 +321,66 @@ func getParent(o runtime.Object) string { } return meta.GetFolder() } + +func (b *FolderAPIBuilder) checkFolderMaxDepth(ctx context.Context, obj runtime.Object) ([]string, error) { + var parents = []string{} + for i := 0; i < folderValidationRules.maxDepth; i++ { + parent := getParent(obj) + if parent == "" { + break + } + parents = append(parents, parent) + if i+1 == folderValidationRules.maxDepth { + return parents, folder.ErrMaximumDepthReached + } + + parentObj, err := b.storage.Get(ctx, parent, &metav1.GetOptions{}) + if err != nil { + return parents, err + } + obj = parentObj + } + return parents, nil +} + +func (b *FolderAPIBuilder) validateOnUpdate(ctx context.Context, obj, old runtime.Object) error { + f, ok := obj.(*v0alpha1.Folder) + if !ok { + return fmt.Errorf("obj is not v0alpha1.Folder") + } + + fOld, ok := old.(*v0alpha1.Folder) + if !ok { + return fmt.Errorf("obj is not v0alpha1.Folder") + } + var newParent = getParent(obj) + if newParent != getParent(fOld) { + // it's a move operation + return b.validateMove(ctx, obj, newParent) + } + // it's a spec update + if f.Spec.Title == "" { + return dashboards.ErrFolderTitleEmpty + } + return nil +} + +func (b *FolderAPIBuilder) validateMove(ctx context.Context, obj runtime.Object, newParent string) error { + // folder cannot be moved to a k6 folder + if newParent == accesscontrol.K6FolderUID { + return fmt.Errorf("k6 project may not be moved") + } + + //FIXME: until we have a way to represent the tree, we can only + // look at folder parents to check how deep the new folder tree will be + parents, err := b.checkFolderMaxDepth(ctx, obj) + if err != nil { + return err + } + + // if by moving a folder we exceed the max depth, return an error + if len(parents)+1 >= folderValidationRules.maxDepth { + return folder.ErrMaximumDepthReached + } + return nil +} diff --git a/pkg/registry/apis/folders/register_test.go b/pkg/registry/apis/folders/register_test.go index 1409f3e37ab..99335fc05a6 100644 --- a/pkg/registry/apis/folders/register_test.go +++ b/pkg/registry/apis/folders/register_test.go @@ -8,6 +8,7 @@ import ( "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/apis/folder/v0alpha1" grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" + "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" "github.com/grafana/grafana/pkg/services/authz/zanzana" "github.com/grafana/grafana/pkg/services/dashboards" @@ -257,7 +258,7 @@ func TestFolderAPIBuilder_Validate_Create(t *testing.T) { }, }, { - name: "should return error when creating a nested folder higher than max depth", + name: "should not allow creating a folder in a tree that is too deep", input: input{ obj: &v0alpha1.Folder{ Spec: v0alpha1.Spec{ @@ -410,3 +411,169 @@ func TestFolderAPIBuilder_Validate_Delete(t *testing.T) { }) } } + +func TestFolderAPIBuilder_Validate_Update(t *testing.T) { + var circularObj = &v0alpha1.Folder{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "stacks-123", + Name: "new-parent", + Annotations: map[string]string{"grafana.app/folder": "new-parent"}, + }, + } + + tests := []struct { + name string + updatedObj *v0alpha1.Folder + expected *v0alpha1.Folder + setupFn func(*mock.Mock) + wantErr bool + }{ + { + name: "should allow updating a folder spec", + updatedObj: &v0alpha1.Folder{ + Spec: v0alpha1.Spec{ + Title: "different title", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "stacks-123", + Name: "valid-name", + Annotations: map[string]string{"grafana.app/folder": "valid-parent"}, + }, + }, + expected: &v0alpha1.Folder{ + Spec: v0alpha1.Spec{ + Title: "different title", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "stacks-123", + Name: "valid-name", + Annotations: map[string]string{"grafana.app/folder": "valid-parent"}, + }, + }, + }, + { + name: "updated title should not be empty", + updatedObj: &v0alpha1.Folder{ + Spec: v0alpha1.Spec{ + Title: "", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "stacks-123", + Name: "valid-name", + Annotations: map[string]string{"grafana.app/folder": "valid-parent"}, + }, + }, + wantErr: true, + }, + { + name: "should allow moving to a valid parent", + updatedObj: &v0alpha1.Folder{ + Spec: v0alpha1.Spec{ + Title: "foo", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "stacks-123", + Name: "valid-name", + Annotations: map[string]string{"grafana.app/folder": "new-parent"}, + }, + }, + setupFn: func(m *mock.Mock) { + m.On("Get", mock.Anything, "new-parent", mock.Anything).Return( + &v0alpha1.Folder{}, + nil).Once() + }, + }, + { + name: "should not allow moving to a k6 folder", + updatedObj: &v0alpha1.Folder{ + Spec: v0alpha1.Spec{ + Title: "foo", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "stacks-123", + Name: "valid-name", + Annotations: map[string]string{"grafana.app/folder": accesscontrol.K6FolderUID}, + }, + }, + setupFn: func(m *mock.Mock) { + m.On("Get", mock.Anything, accesscontrol.K6FolderUID, mock.Anything).Return( + &v0alpha1.Folder{}, + nil).Once() + }, + wantErr: true, + }, + { + name: "should not allow moving to a folder that is too deep", + updatedObj: &v0alpha1.Folder{ + Spec: v0alpha1.Spec{ + Title: "foo", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "stacks-123", + Name: "valid-name", + Annotations: map[string]string{"grafana.app/folder": "new-parent"}, + }, + }, + setupFn: func(m *mock.Mock) { + m.On("Get", mock.Anything, "new-parent", mock.Anything).Return( + circularObj, + nil) + }, + wantErr: true, + }, + } + + s := (grafanarest.Storage)(nil) + m := &mock.Mock{} + us := storageMock{m, s} + sm := searcherMock{Mock: m} + + obj := &v0alpha1.Folder{ + Spec: v0alpha1.Spec{ + Title: "foo", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "stacks-123", + Name: "valid-name", + Annotations: map[string]string{"grafana.app/folder": "valid-parent"}, + }, + } + + for _, tt := range tests { + if tt.setupFn != nil { + tt.setupFn(m) + } + t.Run(tt.name, func(t *testing.T) { + b := &FolderAPIBuilder{ + gv: resourceInfo.GroupVersion(), + features: nil, + namespacer: func(_ int64) string { return "123" }, + folderSvc: foldertest.NewFakeService(), + storage: us, + accessControl: acimpl.ProvideAccessControl(featuremgmt.WithFeatures("nestedFolders"), zanzana.NewNoopClient()), + searcher: sm, + } + + err := b.Validate(context.Background(), admission.NewAttributesRecord( + tt.updatedObj, + obj, + v0alpha1.SchemeGroupVersion.WithKind("folder"), + tt.updatedObj.Namespace, + tt.updatedObj.Name, + v0alpha1.SchemeGroupVersion.WithResource("folders"), + "", + "UPDATE", + nil, + true, + &user.SignedInUser{}, + ), + nil) + + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } +}