Folders move (#98100)

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

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

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{})
+	_, err := b.checkFolderMaxDepth(ctx, obj)
 	if err != nil {
 		return err
 	}
-		obj = parentObj
-	}
-	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
+}

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)
+		})
+	}
+}