diff --git a/pkg/api/apierrors/folder.go b/pkg/api/apierrors/folder.go index 18ab865426d..7ed21c04fb5 100644 --- a/pkg/api/apierrors/folder.go +++ b/pkg/api/apierrors/folder.go @@ -41,7 +41,8 @@ func ToFolderErrorResponse(err error) response.Response { return response.Error(http.StatusConflict, err.Error(), nil) } - if errors.Is(err, dashboards.ErrFolderVersionMismatch) { + if errors.Is(err, dashboards.ErrFolderVersionMismatch) || + k8sErrors.IsAlreadyExists(err) { return response.JSON(http.StatusPreconditionFailed, util.DynMap{"status": "version-mismatch", "message": dashboards.ErrFolderVersionMismatch.Error()}) } diff --git a/pkg/registry/apis/folders/folder_storage.go b/pkg/registry/apis/folders/folder_storage.go index 588681e0311..2f3ab79873e 100644 --- a/pkg/registry/apis/folders/folder_storage.go +++ b/pkg/registry/apis/folders/folder_storage.go @@ -11,6 +11,7 @@ import ( claims "github.com/grafana/authlib/types" folders "github.com/grafana/grafana/apps/folder/pkg/apis/folder/v1beta1" + "github.com/grafana/grafana/pkg/api/apierrors" "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/apimachinery/utils" grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" @@ -79,7 +80,8 @@ func (s *folderStorage) Create(ctx context.Context, ) (runtime.Object, error) { obj, err := s.store.Create(ctx, obj, createValidation, options) if err != nil { - return nil, err + statusErr := apierrors.ToFolderStatusError(err) + return nil, &statusErr } info, err := request.NamespaceInfoFrom(ctx, true) diff --git a/pkg/registry/apis/folders/register.go b/pkg/registry/apis/folders/register.go index 8407cf348cb..397cbcb94a5 100644 --- a/pkg/registry/apis/folders/register.go +++ b/pkg/registry/apis/folders/register.go @@ -34,6 +34,7 @@ import ( "github.com/grafana/grafana/pkg/storage/unified/apistore" "github.com/grafana/grafana/pkg/storage/unified/resource" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" + "github.com/grafana/grafana/pkg/util" ) var _ builder.APIGroupBuilder = (*FolderAPIBuilder)(nil) @@ -302,6 +303,14 @@ func (b *FolderAPIBuilder) validateOnCreate(ctx context.Context, id string, obj } } + if !util.IsValidShortUID(id) { + return dashboards.ErrDashboardInvalidUid + } + + if util.IsShortUIDTooLong(id) { + return dashboards.ErrDashboardUidTooLong + } + f, ok := obj.(*folders.Folder) if !ok { return fmt.Errorf("obj is not folders.Folder") diff --git a/pkg/tests/apis/folder/folders_test.go b/pkg/tests/apis/folder/folders_test.go index 29a5a6f1814..5001e90f42c 100644 --- a/pkg/tests/apis/folder/folders_test.go +++ b/pkg/tests/apis/folder/folders_test.go @@ -664,52 +664,56 @@ func TestIntegrationFolderCreatePermissions(t *testing.T) { }, } - for _, tc := range tcs { - t.Run(tc.description, func(t *testing.T) { - helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ - AppModeProduction: true, - DisableAnonymous: true, - APIServerStorageType: "unified", - UnifiedStorageConfig: map[string]setting.UnifiedStorageConfig{ - folders.RESOURCEGROUP: { - DualWriterMode: grafanarest.Mode1, + // test on all dualwriter modes + for mode := 0; mode <= 4; mode++ { + for _, tc := range tcs { + t.Run(fmt.Sprintf("[Mode: %v] "+tc.description, mode), func(t *testing.T) { + modeDw := grafanarest.DualWriterMode(mode) + helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + AppModeProduction: true, + DisableAnonymous: true, + APIServerStorageType: "unified", + UnifiedStorageConfig: map[string]setting.UnifiedStorageConfig{ + folders.RESOURCEGROUP: { + DualWriterMode: modeDw, + }, }, - }, - EnableFeatureToggles: []string{ - featuremgmt.FlagNestedFolders, - featuremgmt.FlagKubernetesClientDashboardsFolders, - }, - }) + EnableFeatureToggles: []string{ + featuremgmt.FlagNestedFolders, + featuremgmt.FlagKubernetesClientDashboardsFolders, + }, + }) - user := helper.CreateUser("user", apis.Org1, org.RoleViewer, tc.permissions) + user := helper.CreateUser("user", apis.Org1, org.RoleViewer, tc.permissions) - parentPayload := `{ + parentPayload := `{ "title": "Test/parent", "uid": "parentuid" }` - parentCreate := apis.DoRequest(helper, apis.RequestParams{ - User: helper.Org1.Admin, - Method: http.MethodPost, - Path: "/api/folders", - Body: []byte(parentPayload), - }, &folder.Folder{}) - require.NotNil(t, parentCreate.Result) - parentUID := parentCreate.Result.UID - require.NotEmpty(t, parentUID) - - resp := apis.DoRequest(helper, apis.RequestParams{ - User: user, - Method: http.MethodPost, - Path: "/api/folders", - Body: []byte(tc.input), - }, &dtos.Folder{}) - require.Equal(t, tc.expectedCode, resp.Response.StatusCode) - - if tc.expectedCode == http.StatusOK { - require.Equal(t, "uid", resp.Result.UID) - require.Equal(t, "Folder", resp.Result.Title) - } - }) + parentCreate := apis.DoRequest(helper, apis.RequestParams{ + User: helper.Org1.Admin, + Method: http.MethodPost, + Path: "/api/folders", + Body: []byte(parentPayload), + }, &folder.Folder{}) + require.NotNil(t, parentCreate.Result) + parentUID := parentCreate.Result.UID + require.NotEmpty(t, parentUID) + + resp := apis.DoRequest(helper, apis.RequestParams{ + User: user, + Method: http.MethodPost, + Path: "/api/folders", + Body: []byte(tc.input), + }, &dtos.Folder{}) + require.Equal(t, tc.expectedCode, resp.Response.StatusCode) + + if tc.expectedCode == http.StatusOK { + require.Equal(t, "uid", resp.Result.UID) + require.Equal(t, "Folder", resp.Result.Title) + } + }) + } } } @@ -768,100 +772,104 @@ func TestIntegrationFolderGetPermissions(t *testing.T) { }, } - for _, tc := range tcs { - t.Run(tc.description, func(t *testing.T) { - helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ - AppModeProduction: true, - DisableAnonymous: true, - APIServerStorageType: "unified", - UnifiedStorageConfig: map[string]setting.UnifiedStorageConfig{ - folders.RESOURCEGROUP: { - DualWriterMode: grafanarest.Mode1, + // test on all dualwriter modes + for mode := 0; mode <= 4; mode++ { + for _, tc := range tcs { + t.Run(fmt.Sprintf("[Mode: %v] "+tc.description, mode), func(t *testing.T) { + modeDw := grafanarest.DualWriterMode(mode) + helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + AppModeProduction: true, + DisableAnonymous: true, + APIServerStorageType: "unified", + UnifiedStorageConfig: map[string]setting.UnifiedStorageConfig{ + folders.RESOURCEGROUP: { + DualWriterMode: modeDw, + }, }, - }, - EnableFeatureToggles: []string{ - featuremgmt.FlagNestedFolders, - featuremgmt.FlagKubernetesClientDashboardsFolders, - }, - }) + EnableFeatureToggles: []string{ + featuremgmt.FlagNestedFolders, + featuremgmt.FlagKubernetesClientDashboardsFolders, + }, + }) - // Create parent folder - parentPayload := `{ + // Create parent folder + parentPayload := `{ "title": "testparent", "uid": "parentuid" }` - parentCreate := apis.DoRequest(helper, apis.RequestParams{ - User: helper.Org1.Admin, - Method: http.MethodPost, - Path: "/api/folders", - Body: []byte(parentPayload), - }, &folder.Folder{}) - require.NotNil(t, parentCreate.Result) - parentUID := parentCreate.Result.UID - require.NotEmpty(t, parentUID) - - // Create descendant folder - payload := "{ \"uid\": \"descuid\", \"title\": \"Folder\", \"parentUid\": \"parentuid\"}" - resp := apis.DoRequest(helper, apis.RequestParams{ - User: helper.Org1.Admin, - Method: http.MethodPost, - Path: "/api/folders", - Body: []byte(payload), - }, &dtos.Folder{}) - require.Equal(t, http.StatusOK, resp.Response.StatusCode) - - user := helper.CreateUser("user", apis.Org1, org.RoleNone, tc.permissions) - - // Get with accesscontrol disabled - getResp := apis.DoRequest(helper, apis.RequestParams{ - User: user, - Method: http.MethodGet, - Path: "/api/folders/descuid", - }, &dtos.Folder{}) - require.Equal(t, tc.expectedCode, getResp.Response.StatusCode) - require.NotNil(t, getResp.Result) - - require.False(t, getResp.Result.AccessControl[dashboards.ActionFoldersRead]) - require.False(t, getResp.Result.AccessControl[dashboards.ActionFoldersWrite]) - - parents := getResp.Result.Parents - require.Equal(t, len(tc.expectedParentUIDs), len(parents)) - require.Equal(t, len(tc.expectedParentTitles), len(parents)) - for i := 0; i < len(tc.expectedParentUIDs); i++ { - require.Equal(t, tc.expectedParentUIDs[i], parents[i].UID) - require.Equal(t, tc.expectedParentTitles[i], parents[i].Title) - } - - // Get with accesscontrol enabled - if tc.checkAccessControl { - acPerms := []resourcepermissions.SetResourcePermissionCommand{ - { - Actions: []string{dashboards.ActionFoldersRead}, - Resource: "folders", - ResourceAttribute: "uid", - ResourceID: "*", - }, - { - Actions: []string{dashboards.ActionFoldersWrite}, - Resource: "folders", - ResourceAttribute: "uid", - ResourceID: "parentuid", - }, - } - acUser := helper.CreateUser("acuser", apis.Org1, org.RoleNone, acPerms) + parentCreate := apis.DoRequest(helper, apis.RequestParams{ + User: helper.Org1.Admin, + Method: http.MethodPost, + Path: "/api/folders", + Body: []byte(parentPayload), + }, &folder.Folder{}) + require.NotNil(t, parentCreate.Result) + parentUID := parentCreate.Result.UID + require.NotEmpty(t, parentUID) + + // Create descendant folder + payload := "{ \"uid\": \"descuid\", \"title\": \"Folder\", \"parentUid\": \"parentuid\"}" + resp := apis.DoRequest(helper, apis.RequestParams{ + User: helper.Org1.Admin, + Method: http.MethodPost, + Path: "/api/folders", + Body: []byte(payload), + }, &dtos.Folder{}) + require.Equal(t, http.StatusOK, resp.Response.StatusCode) + + user := helper.CreateUser("user", apis.Org1, org.RoleNone, tc.permissions) - getWithAC := apis.DoRequest(helper, apis.RequestParams{ - User: acUser, + // Get with accesscontrol disabled + getResp := apis.DoRequest(helper, apis.RequestParams{ + User: user, Method: http.MethodGet, - Path: "/api/folders/descuid?accesscontrol=true", + Path: "/api/folders/descuid", }, &dtos.Folder{}) - require.Equal(t, tc.expectedCode, getWithAC.Response.StatusCode) - require.NotNil(t, getWithAC.Result) + require.Equal(t, tc.expectedCode, getResp.Response.StatusCode) + require.NotNil(t, getResp.Result) + + require.False(t, getResp.Result.AccessControl[dashboards.ActionFoldersRead]) + require.False(t, getResp.Result.AccessControl[dashboards.ActionFoldersWrite]) + + parents := getResp.Result.Parents + require.Equal(t, len(tc.expectedParentUIDs), len(parents)) + require.Equal(t, len(tc.expectedParentTitles), len(parents)) + for i := 0; i < len(tc.expectedParentUIDs); i++ { + require.Equal(t, tc.expectedParentUIDs[i], parents[i].UID) + require.Equal(t, tc.expectedParentTitles[i], parents[i].Title) + } - require.True(t, getWithAC.Result.AccessControl[dashboards.ActionFoldersRead]) - require.True(t, getWithAC.Result.AccessControl[dashboards.ActionFoldersWrite]) - } - }) + // Get with accesscontrol enabled + if tc.checkAccessControl { + acPerms := []resourcepermissions.SetResourcePermissionCommand{ + { + Actions: []string{dashboards.ActionFoldersRead}, + Resource: "folders", + ResourceAttribute: "uid", + ResourceID: "*", + }, + { + Actions: []string{dashboards.ActionFoldersWrite}, + Resource: "folders", + ResourceAttribute: "uid", + ResourceID: "parentuid", + }, + } + acUser := helper.CreateUser("acuser", apis.Org1, org.RoleNone, acPerms) + + getWithAC := apis.DoRequest(helper, apis.RequestParams{ + User: acUser, + Method: http.MethodGet, + Path: "/api/folders/descuid?accesscontrol=true", + }, &dtos.Folder{}) + require.Equal(t, tc.expectedCode, getWithAC.Response.StatusCode) + require.NotNil(t, getWithAC.Result) + + require.True(t, getWithAC.Result.AccessControl[dashboards.ActionFoldersRead]) + require.True(t, getWithAC.Result.AccessControl[dashboards.ActionFoldersWrite]) + } + }) + } } } @@ -948,76 +956,80 @@ func TestIntegrationFoldersCreateAPIEndpointK8S(t *testing.T) { }, } - for _, tc := range tcs { - t.Run(testDescription(tc.description, tc.expectedFolderSvcError), func(t *testing.T) { - helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ - AppModeProduction: true, - DisableAnonymous: true, - APIServerStorageType: "unified", - UnifiedStorageConfig: map[string]setting.UnifiedStorageConfig{ - folders.RESOURCEGROUP: { - DualWriterMode: grafanarest.Mode1, + // test on all dualwriter modes + for mode := 0; mode <= 4; mode++ { + for _, tc := range tcs { + t.Run(fmt.Sprintf("[Mode: %v] "+testDescription(tc.description, tc.expectedFolderSvcError), mode), func(t *testing.T) { + modeDw := grafanarest.DualWriterMode(mode) + helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + AppModeProduction: true, + DisableAnonymous: true, + APIServerStorageType: "unified", + UnifiedStorageConfig: map[string]setting.UnifiedStorageConfig{ + folders.RESOURCEGROUP: { + DualWriterMode: modeDw, + }, }, - }, - EnableFeatureToggles: []string{ - featuremgmt.FlagNestedFolders, - featuremgmt.FlagKubernetesClientDashboardsFolders, - }, - }) + EnableFeatureToggles: []string{ + featuremgmt.FlagNestedFolders, + featuremgmt.FlagKubernetesClientDashboardsFolders, + }, + }) - userTest := helper.CreateUser("user", apis.Org1, org.RoleViewer, tc.permissions) + userTest := helper.CreateUser("user", apis.Org1, org.RoleViewer, tc.permissions) - if tc.createSecondRecord { - client := helper.GetResourceClient(apis.ResourceClientArgs{ - User: helper.Org1.Admin, - GVR: gvr, - }) - create2 := apis.DoRequest(helper, apis.RequestParams{ - User: client.Args.User, - Method: http.MethodPost, - Path: "/api/folders", - Body: []byte(tc.input), - }, &folder.Folder{}) - require.NotEmpty(t, create2.Response) - require.Equal(t, http.StatusOK, create2.Response.StatusCode) - } - - addr := helper.GetEnv().Server.HTTPServer.Listener.Addr() - login := userTest.Identity.GetLogin() - baseUrl := fmt.Sprintf("http://%s:%s@%s", login, user.Password("user"), addr) - - req, err := http.NewRequest(http.MethodPost, fmt.Sprintf( - "%s%s", - baseUrl, - "/api/folders", - ), bytes.NewBuffer([]byte(tc.input))) - require.NoError(t, err) - req.Header.Set("Content-Type", "application/json") + if tc.createSecondRecord { + client := helper.GetResourceClient(apis.ResourceClientArgs{ + User: helper.Org1.Admin, + GVR: gvr, + }) + create2 := apis.DoRequest(helper, apis.RequestParams{ + User: client.Args.User, + Method: http.MethodPost, + Path: "/api/folders", + Body: []byte(tc.input), + }, &folder.Folder{}) + require.NotEmpty(t, create2.Response) + require.Equal(t, http.StatusOK, create2.Response.StatusCode) + } - resp, err := http.DefaultClient.Do(req) - require.NoError(t, err) - require.NotNil(t, resp) - require.Equal(t, tc.expectedCode, resp.StatusCode) + addr := helper.GetEnv().Server.HTTPServer.Listener.Addr() + login := userTest.Identity.GetLogin() + baseUrl := fmt.Sprintf("http://%s:%s@%s", login, user.Password("user"), addr) - type folderWithMessage struct { - dtos.Folder - Message string `json:"message"` - } + req, err := http.NewRequest(http.MethodPost, fmt.Sprintf( + "%s%s", + baseUrl, + "/api/folders", + ), bytes.NewBuffer([]byte(tc.input))) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") - folder := folderWithMessage{} - err = json.NewDecoder(resp.Body).Decode(&folder) - require.NoError(t, err) - require.NoError(t, resp.Body.Close()) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NotNil(t, resp) + require.Equal(t, tc.expectedCode, resp.StatusCode) - if tc.expectedCode == http.StatusOK { - require.Equal(t, "uid", folder.UID) - require.Equal(t, "Folder", folder.Title) - } + type folderWithMessage struct { + dtos.Folder + Message string `json:"message"` + } - if tc.expectedMessage != "" { - require.Equal(t, tc.expectedMessage, folder.Message) - } - }) + folder := folderWithMessage{} + err = json.NewDecoder(resp.Body).Decode(&folder) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + + if tc.expectedCode == http.StatusOK { + require.Equal(t, "uid", folder.UID) + require.Equal(t, "Folder", folder.Title) + } + + if tc.expectedMessage != "" { + require.Equal(t, tc.expectedMessage, folder.Message) + } + }) + } } }