From 2a7477877627e26d3844d9c2c101bae893f906b9 Mon Sep 17 00:00:00 2001 From: Leonor Oliveira <9090754+leonorfmartins@users.noreply.github.com> Date: Fri, 22 Nov 2024 16:51:53 +0100 Subject: [PATCH] Support delete endpoint for folders (#96573) * Support delete endpoint for folders * Include authorizer * Add test for delete verb * Add delete command to delete options * Pass query string to context to admission * Dont support nested folder deletion for now * Skip test if feature flag is present * Add test case * Remove comment * Only rely on the storage type config to run alerting tests * Dont change legacy subpath * Remove unised function * Add test case when an editor can delete alert rules * Lint --- pkg/api/folder.go | 8 +- pkg/registry/apis/folders/register.go | 4 +- pkg/registry/apis/folders/register_test.go | 26 +++- .../api/alerting/api_alertmanager_test.go | 140 +++++++++--------- pkg/tests/apis/folder/folders_test.go | 8 +- 5 files changed, 106 insertions(+), 80 deletions(-) diff --git a/pkg/api/folder.go b/pkg/api/folder.go index a066fa21b50..5c9447a8692 100644 --- a/pkg/api/folder.go +++ b/pkg/api/folder.go @@ -52,7 +52,6 @@ func (hs *HTTPServer) registerFolderAPI(apiRoute routing.RouteRegister, authoriz folderUidRoute.Get("/", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersRead, uidScope)), routing.Wrap(hs.GetFolderByUID)) folderUidRoute.Put("/", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersWrite, uidScope)), routing.Wrap(hs.UpdateFolder)) folderUidRoute.Post("/move", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersWrite, uidScope)), routing.Wrap(hs.MoveFolder)) - folderUidRoute.Delete("/", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersDelete, uidScope)), routing.Wrap(hs.DeleteFolder)) folderUidRoute.Get("/counts", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersRead, uidScope)), routing.Wrap(hs.GetFolderDescendantCounts)) folderUidRoute.Group("/permissions", func(folderPermissionRoute routing.RouteRegister) { @@ -65,16 +64,21 @@ func (hs *HTTPServer) registerFolderAPI(apiRoute routing.RouteRegister, authoriz handler := newFolderK8sHandler(hs) folderRoute.Post("/", handler.createFolder) folderRoute.Get("/", handler.getFolders) + folderRoute.Group("/:uid", func(folderUidRoute routing.RouteRegister) { + folderUidRoute.Delete("/", handler.deleteFolder) + }) } else { folderRoute.Post("/", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersCreate)), routing.Wrap(hs.CreateFolder)) folderRoute.Get("/", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersRead)), routing.Wrap(hs.GetFolders)) + folderRoute.Group("/:uid", func(folderUidRoute routing.RouteRegister) { + folderUidRoute.Delete("/", authorize(accesscontrol.EvalPermission(dashboards.ActionFoldersDelete, uidScope)), routing.Wrap(hs.DeleteFolder)) + }) } // Only adding support for some routes with the k8s handler for now. Include the rest here. if false { handler := newFolderK8sHandler(hs) folderRoute.Group("/:uid", func(folderUidRoute routing.RouteRegister) { folderUidRoute.Get("/", handler.getFolder) - folderUidRoute.Delete("/", handler.deleteFolder) folderUidRoute.Put("/:uid", handler.updateFolder) }) } diff --git a/pkg/registry/apis/folders/register.go b/pkg/registry/apis/folders/register.go index c46395ee271..0da47483355 100644 --- a/pkg/registry/apis/folders/register.go +++ b/pkg/registry/apis/folders/register.go @@ -3,6 +3,7 @@ package folders import ( "context" "errors" + "slices" grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" "github.com/prometheus/client_golang/prometheus" @@ -187,9 +188,10 @@ func (b *FolderAPIBuilder) GetAuthorizer() authorizer.Authorizer { } func authorizerFunc(ctx context.Context, attr authorizer.Attributes) (*authorizerParams, error) { + allowedVerbs := []string{utils.VerbCreate, utils.VerbDelete, utils.VerbList} verb := attr.GetVerb() name := attr.GetName() - if (!attr.IsResourceRequest()) || (name == "" && verb != utils.VerbCreate && verb != utils.VerbList) { + if (!attr.IsResourceRequest()) || (name == "" && verb != utils.VerbCreate && slices.Contains(allowedVerbs, verb)) { return nil, errNoResource } diff --git a/pkg/registry/apis/folders/register_test.go b/pkg/registry/apis/folders/register_test.go index 33e9344e52b..6113a60516d 100644 --- a/pkg/registry/apis/folders/register_test.go +++ b/pkg/registry/apis/folders/register_test.go @@ -103,18 +103,36 @@ func TestFolderAPIBuilder_getAuthorizerFunc(t *testing.T) { OrgID: orgID, Name: "123", Permissions: map[int64]map[string][]string{ - orgID: {dashboards.ActionFoldersRead: {dashboards.ScopeFoldersAll}}, + orgID: {}, }, }, verb: string(utils.VerbList), }, expect: expect{ eval: "folders:read", + allow: false, + }, + }, + { + name: "user with delete permissions should be able to delete a folder", + input: input{ + user: &user.SignedInUser{ + UserID: 1, + OrgID: orgID, + Name: "123", + Permissions: map[int64]map[string][]string{ + orgID: {dashboards.ActionFoldersDelete: {dashboards.ScopeFoldersAll}, dashboards.ActionFoldersWrite: {dashboards.ScopeFoldersAll}}, + }, + }, + verb: string(utils.VerbDelete), + }, + expect: expect{ + eval: "folders:delete", allow: true, }, }, { - name: "user without read permissions should not be able to list folders", + name: "user without delete permissions should NOT be able to delete a folder", input: input{ user: &user.SignedInUser{ UserID: 1, @@ -124,10 +142,10 @@ func TestFolderAPIBuilder_getAuthorizerFunc(t *testing.T) { orgID: {}, }, }, - verb: string(utils.VerbList), + verb: string(utils.VerbDelete), }, expect: expect{ - eval: "folders:read", + eval: "folders:delete", allow: false, }, }, diff --git a/pkg/tests/api/alerting/api_alertmanager_test.go b/pkg/tests/api/alerting/api_alertmanager_test.go index 67f6bb696eb..a4dcb3862b5 100644 --- a/pkg/tests/api/alerting/api_alertmanager_test.go +++ b/pkg/tests/api/alerting/api_alertmanager_test.go @@ -22,6 +22,7 @@ import ( "github.com/grafana/grafana/pkg/expr" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/tracing" + "github.com/grafana/grafana/pkg/services/apiserver/options" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" ngstore "github.com/grafana/grafana/pkg/services/ngalert/store" @@ -694,15 +695,18 @@ func TestIntegrationRulerAccess(t *testing.T) { func TestIntegrationDeleteFolderWithRules(t *testing.T) { testinfra.SQLiteIntegrationTest(t) - // Setup Grafana and its Database - dir, path := testinfra.CreateGrafDir(t, testinfra.GrafanaOpts{ + opts := testinfra.GrafanaOpts{ DisableLegacyAlerting: true, EnableUnifiedAlerting: true, EnableQuota: true, DisableAnonymous: true, ViewersCanEdit: true, AppModeProduction: true, - }) + APIServerStorageType: options.StorageTypeLegacy, + } + + // Setup Grafana and its Database + dir, path := testinfra.CreateGrafDir(t, opts) grafanaListedAddr, env := testinfra.StartGrafanaEnv(t, dir, path) @@ -725,8 +729,7 @@ func TestIntegrationDeleteFolderWithRules(t *testing.T) { createRule(t, apiClient, "default") - // First, let's have an editor create a rule within the folder/namespace. - { + t.Run("editor create a rule within the folder/namespace", func(t *testing.T) { u := fmt.Sprintf("http://editor:editor@%s/api/ruler/grafana/api/v1/rules", grafanaListedAddr) // nolint:gosec resp, err := http.Get(u) @@ -746,68 +749,66 @@ func TestIntegrationDeleteFolderWithRules(t *testing.T) { b = re.ReplaceAll(b, []byte(`"updated":"2021-05-19T19:47:55Z"`)) expectedGetRulesResponseBody := fmt.Sprintf(`{ - "default": [ - { - "name": "arulegroup", - "interval": "1m", - "rules": [ - { - "expr": "", - "for": "2m", - "labels": { - "label1": "val1" - }, - "annotations": { - "annotation1": "val1" - }, - "grafana_alert": { - "id": 1, - "orgId": 1, - "title": "rule under folder default", - "condition": "A", - "data": [ - { - "refId": "A", - "queryType": "", - "relativeTimeRange": { - "from": 18000, - "to": 10800 - }, - "datasourceUid": "__expr__", - "model": { - "expression": "2 + 3 > 1", - "intervalMs": 1000, - "maxDataPoints": 43200, - "type": "math" + "default": [ + { + "name": "arulegroup", + "interval": "1m", + "rules": [ + { + "expr": "", + "for": "2m", + "labels": { + "label1": "val1" + }, + "annotations": { + "annotation1": "val1" + }, + "grafana_alert": { + "id": 1, + "orgId": 1, + "title": "rule under folder default", + "condition": "A", + "data": [ + { + "refId": "A", + "queryType": "", + "relativeTimeRange": { + "from": 18000, + "to": 10800 + }, + "datasourceUid": "__expr__", + "model": { + "expression": "2 + 3 > 1", + "intervalMs": 1000, + "maxDataPoints": 43200, + "type": "math" + } + } + ], + "updated": "2021-05-19T19:47:55Z", + "intervalSeconds": 60, + "is_paused": false, + "version": 1, + "uid": "", + "namespace_uid": %q, + "rule_group": "arulegroup", + "no_data_state": "NoData", + "exec_err_state": "Alerting", + "metadata": { + "editor_settings": { + "simplified_query_and_expressions_section": false, + "simplified_notifications_section": false } - } - ], - "updated": "2021-05-19T19:47:55Z", - "intervalSeconds": 60, - "is_paused": false, - "version": 1, - "uid": "", - "namespace_uid": %q, - "rule_group": "arulegroup", - "no_data_state": "NoData", - "exec_err_state": "Alerting", - "metadata": { - "editor_settings": { - "simplified_query_and_expressions_section": false, - "simplified_notifications_section": false } } } - } - ] - } - ] - }`, namespaceUID) + ] + } + ] + }`, namespaceUID) assert.JSONEq(t, expectedGetRulesResponseBody, string(b)) - } - - // Next, the editor can not delete the folder because it contains Grafana 8 alerts. - { + }) + t.Run("editor can not delete the folder because it contains Grafana 8 alerts", func(t *testing.T) { u := fmt.Sprintf("http://editor:editor@%s/api/folders/%s", grafanaListedAddr, namespaceUID) req, err := http.NewRequest(http.MethodDelete, u, nil) require.NoError(t, err) @@ -825,10 +826,8 @@ func TestIntegrationDeleteFolderWithRules(t *testing.T) { err = json.Unmarshal(b, &errutilErr) require.NoError(t, err) assert.Equal(t, "Folder cannot be deleted: folder is not empty", errutilErr.Message) - } - - // Next, the editor can delete the folder if forceDeleteRules is true. - { + }) + t.Run("editor can delete the folder if forceDeleteRules is true", func(t *testing.T) { u := fmt.Sprintf("http://editor:editor@%s/api/folders/%s?forceDeleteRules=true", grafanaListedAddr, namespaceUID) req, err := http.NewRequest(http.MethodDelete, u, nil) require.NoError(t, err) @@ -842,10 +841,8 @@ func TestIntegrationDeleteFolderWithRules(t *testing.T) { _, err = io.ReadAll(resp.Body) require.NoError(t, err) require.Equal(t, 200, resp.StatusCode) - } - - // Finally, we ensure the rules were deleted. - { + }) + t.Run("editor can delete rules", func(t *testing.T) { u := fmt.Sprintf("http://editor:editor@%s/api/ruler/grafana/api/v1/rules", grafanaListedAddr) // nolint:gosec resp, err := http.Get(u) @@ -859,7 +856,8 @@ func TestIntegrationDeleteFolderWithRules(t *testing.T) { assert.Equal(t, 200, resp.StatusCode) assert.JSONEq(t, "{}", string(b)) - } + }) + // TODO(@leonorfmartins): write tests for uni store when we are able to support it } func TestIntegrationAlertRuleCRUD(t *testing.T) { diff --git a/pkg/tests/apis/folder/folders_test.go b/pkg/tests/apis/folder/folders_test.go index 511d4b44e97..dfad1e0f505 100644 --- a/pkg/tests/apis/folder/folders_test.go +++ b/pkg/tests/apis/folder/folders_test.go @@ -440,7 +440,7 @@ func doFolderTests(t *testing.T, helper *apis.K8sTestHelper) *apis.K8sTestHelper } slices.Sort(uids) // make list compare stable - // Check all playlists + // Check all folders for _, uid := range uids { getFromBothAPIs(t, helper, client, uid, nil) } @@ -461,17 +461,21 @@ func doFolderTests(t *testing.T, helper *apis.K8sTestHelper) *apis.K8sTestHelper require.Equal(t, first.GetUID(), updated.GetUID()) require.Equal(t, "Test folder (replaced from k8s; 1 item; PUT)", title) require.Equal(t, "New description", description) + // #TODO figure out why this breaks just for MySQL integration tests // require.Less(t, first.GetResourceVersion(), updated.GetResourceVersion()) // ensure that we get 4 items when listing via k8s l, err := client.Resource.List(context.Background(), metav1.ListOptions{}) require.NoError(t, err) - require.NotNil(t, l) folders, err := meta.ExtractList(l) require.NoError(t, err) require.NotNil(t, folders) require.Equal(t, len(folders), 4) + + // delete test + errDelete := client.Resource.Delete(context.Background(), first.GetName(), metav1.DeleteOptions{}) + require.NoError(t, errDelete) }) return helper }