From 992186c88f34df11a8aa582dedd1b8d8393ad41f Mon Sep 17 00:00:00 2001 From: "Arati R." <33031346+suntala@users.noreply.github.com> Date: Fri, 11 Oct 2024 15:13:56 +0200 Subject: [PATCH] K8s/Folders: Require create permissions when creating folder (#94514) * Require create permissions when creating folder * Test folder create permissions * Add test for nested folder permissions on creation * Replace hardcoded verbs --- pkg/registry/apis/folders/legacy_storage.go | 1 + pkg/registry/apis/folders/register.go | 29 ++-- pkg/tests/apis/folder/folders_test.go | 175 +++++++++++++++----- 3 files changed, 155 insertions(+), 50 deletions(-) diff --git a/pkg/registry/apis/folders/legacy_storage.go b/pkg/registry/apis/folders/legacy_storage.go index 283c5d47887..858d2fdaef4 100644 --- a/pkg/registry/apis/folders/legacy_storage.go +++ b/pkg/registry/apis/folders/legacy_storage.go @@ -174,6 +174,7 @@ func (s *legacyStorage) Create(ctx context.Context, if err != nil { return nil, err } + parent := accessor.GetFolder() out, err := s.service.Create(ctx, &folder.CreateFolderCommand{ diff --git a/pkg/registry/apis/folders/register.go b/pkg/registry/apis/folders/register.go index 470d4baf287..a891d5f249d 100644 --- a/pkg/registry/apis/folders/register.go +++ b/pkg/registry/apis/folders/register.go @@ -15,6 +15,7 @@ import ( "k8s.io/kube-openapi/pkg/spec3" "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/apis/folder/v0alpha1" grafanaregistry "github.com/grafana/grafana/pkg/apiserver/registry/generic" grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" @@ -154,7 +155,9 @@ func (b *FolderAPIBuilder) PostProcessOpenAPI(oas *spec3.OpenAPI) (*spec3.OpenAP func (b *FolderAPIBuilder) GetAuthorizer() authorizer.Authorizer { return authorizer.AuthorizerFunc( func(ctx context.Context, attr authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { - if !attr.IsResourceRequest() || attr.GetName() == "" { + verb := attr.GetVerb() + name := attr.GetName() + if (!attr.IsResourceRequest()) || (name == "" && verb != utils.VerbCreate) { return authorizer.DecisionNoOpinion, "", nil } @@ -164,24 +167,24 @@ func (b *FolderAPIBuilder) GetAuthorizer() authorizer.Authorizer { return authorizer.DecisionDeny, "valid user is required", err } - action := dashboards.ActionFoldersRead - scope := dashboards.ScopeFoldersProvider.GetResourceScopeUID(attr.GetName()) + scope := dashboards.ScopeFoldersProvider.GetResourceScopeUID(name) + eval := accesscontrol.EvalPermission(dashboards.ActionFoldersRead, scope) // "get" is used for sub-resources with GET http (parents, access, count) - switch attr.GetVerb() { - case "patch": + switch verb { + case utils.VerbCreate: + eval = accesscontrol.EvalPermission(dashboards.ActionFoldersCreate) + case utils.VerbPatch: fallthrough - case "create": + case utils.VerbUpdate: + eval = accesscontrol.EvalPermission(dashboards.ActionFoldersWrite, scope) + case utils.VerbDeleteCollection: fallthrough - case "update": - action = dashboards.ActionFoldersWrite - case "deletecollection": - fallthrough - case "delete": - action = dashboards.ActionFoldersDelete + case utils.VerbDelete: + eval = accesscontrol.EvalPermission(dashboards.ActionFoldersDelete, scope) } - ok, err := b.accessControl.Evaluate(ctx, user, accesscontrol.EvalPermission(action, scope)) + ok, err := b.accessControl.Evaluate(ctx, user, eval) if ok { return authorizer.DecisionAllow, "", nil } diff --git a/pkg/tests/apis/folder/folders_test.go b/pkg/tests/apis/folder/folders_test.go index 9683eb03cbb..6c7afa8e2cc 100644 --- a/pkg/tests/apis/folder/folders_test.go +++ b/pkg/tests/apis/folder/folders_test.go @@ -17,8 +17,10 @@ import ( "github.com/grafana/grafana/pkg/api/dtos" folderv0alpha1 "github.com/grafana/grafana/pkg/apis/folder/v0alpha1" grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" + "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder" + "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/tests/apis" "github.com/grafana/grafana/pkg/tests/testinfra" @@ -289,8 +291,8 @@ func TestIntegrationFoldersApp(t *testing.T) { doFolderTests(t, helper) }) - t.Run("with dual write (unified storage, mode 1, nested folders)", func(t *testing.T) { - checkNestedCreate(t, apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + t.Run("with dual write (unified storage, mode 1, create nested folders)", func(t *testing.T) { + doNestedCreateTest(t, apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ AppModeProduction: true, DisableAnonymous: true, APIServerStorageType: "unified", @@ -388,7 +390,48 @@ func doFolderTests(t *testing.T, helper *apis.K8sTestHelper) *apis.K8sTestHelper return helper } -func checkNestedCreate(t *testing.T, helper *apis.K8sTestHelper) { +// This does a get with both k8s and legacy API, and verifies the results are the same +func getFromBothAPIs(t *testing.T, + helper *apis.K8sTestHelper, + client *apis.K8sResourceClient, + uid string, + // Optionally match some expect some values + expect *folder.Folder, +) *unstructured.Unstructured { + t.Helper() + + found, err := client.Resource.Get(context.Background(), uid, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, uid, found.GetName()) + + dto := apis.DoRequest(helper, apis.RequestParams{ + User: client.Args.User, + Method: http.MethodGet, + Path: "/api/folders/" + uid, + }, &folder.Folder{}).Result + require.NotNil(t, dto) + require.Equal(t, uid, dto.UID) + + spec, ok := found.Object["spec"].(map[string]any) + require.True(t, ok) + require.Equal(t, dto.UID, found.GetName()) + require.Equal(t, dto.Title, spec["title"]) + // #TODO add checks for other fields + + if expect != nil { + if expect.Title != "" { + require.Equal(t, expect.Title, dto.Title) + require.Equal(t, expect.Title, spec["title"]) + } + if expect.UID != "" { + require.Equal(t, expect.UID, dto.UID) + require.Equal(t, expect.UID, found.GetName()) + } + } + return found +} + +func doNestedCreateTest(t *testing.T, helper *apis.K8sTestHelper) { client := helper.GetResourceClient(apis.ResourceClientArgs{ User: helper.Org1.Admin, GVR: gvr, @@ -431,43 +474,101 @@ func checkNestedCreate(t *testing.T, helper *apis.K8sTestHelper) { require.Equal(t, parentCreate.Result.URL, parent.URL) } -// This does a get with both k8s and legacy API, and verifies the results are the same -func getFromBothAPIs(t *testing.T, - helper *apis.K8sTestHelper, - client *apis.K8sResourceClient, - uid string, - // Optionally match some expect some values - expect *folder.Folder, -) *unstructured.Unstructured { - t.Helper() +func TestIntegrationFolderCreatePermissions(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } - found, err := client.Resource.Get(context.Background(), uid, metav1.GetOptions{}) - require.NoError(t, err) - require.Equal(t, uid, found.GetName()) + folderWithoutParentInput := "{ \"uid\": \"uid\", \"title\": \"Folder\"}" + folderWithParentInput := "{ \"uid\": \"uid\", \"title\": \"Folder\", \"parentUid\": \"parentuid\"}" - dto := apis.DoRequest(helper, apis.RequestParams{ - User: client.Args.User, - Method: http.MethodGet, - Path: "/api/folders/" + uid, - }, &folder.Folder{}).Result - require.NotNil(t, dto) - require.Equal(t, uid, dto.UID) + type testCase struct { + description string + input string + permissions []resourcepermissions.SetResourcePermissionCommand + expectedCode int + } + tcs := []testCase{ + { + description: "creation of folder without parent succeeds given the correct request for creating a folder", + input: folderWithoutParentInput, + expectedCode: http.StatusOK, + permissions: []resourcepermissions.SetResourcePermissionCommand{ + { + Actions: []string{"folders:create"}, + Resource: "folders", + ResourceAttribute: "uid", + ResourceID: "*", + }, + }, + }, + { + description: "creation of folder without parent fails without permissions to create a folder", + input: folderWithoutParentInput, + expectedCode: http.StatusForbidden, + permissions: []resourcepermissions.SetResourcePermissionCommand{}, + }, + { + description: "creation of folder with parent succeeds given the correct request for creating a folder", + input: folderWithParentInput, + expectedCode: http.StatusOK, + permissions: []resourcepermissions.SetResourcePermissionCommand{ + { + Actions: []string{"folders:create"}, + Resource: "folders", + ResourceAttribute: "uid", + ResourceID: "parentuid", + }, + }, + }, + } - spec, ok := found.Object["spec"].(map[string]any) - require.True(t, ok) - require.Equal(t, dto.UID, found.GetName()) - require.Equal(t, dto.Title, spec["title"]) - // #TODO add checks for other fields + 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{ + folderv0alpha1.RESOURCEGROUP: { + DualWriterMode: grafanarest.Mode1, + }, + }, + EnableFeatureToggles: []string{ + featuremgmt.FlagGrafanaAPIServerTestingWithExperimentalAPIs, + featuremgmt.FlagNestedFolders, + featuremgmt.FlagKubernetesFolders, + }, + }) - if expect != nil { - if expect.Title != "" { - require.Equal(t, expect.Title, dto.Title) - require.Equal(t, expect.Title, spec["title"]) - } - if expect.UID != "" { - require.Equal(t, expect.UID, dto.UID) - require.Equal(t, expect.UID, found.GetName()) - } + user := helper.CreateUser("user", apis.Org1, org.RoleViewer, tc.permissions) + + 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) + } + }) } - return found }