From f8069aebcfe5f34657c09d367db5e131d38d7541 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Jim=C3=A9nez=20S=C3=A1nchez?= Date: Tue, 16 Dec 2025 18:26:32 +0100 Subject: [PATCH] Provisioning: delegate authorization to access checker in dualwriter (#115407) * refactor: delegate authorization to access checker in dualwriter - Remove role-based authorization checks (editor/admin role checks) - Delegate all authorization to access checker which checks resource-level permissions - Update authorizeCreateFolder to use access checker instead of role-based checks - Add comprehensive authorization tests for viewer, editor, and admin roles - Tests cover GET, POST, PUT, DELETE operations and folder creation This change ensures that authorization is consistently handled through the access checker, which checks resource-level permissions rather than just organization roles. * fix: format files_test.go * fix: check error return value of resp.Body.Close() * fix: grant permissions to all dashboards for editor role in authorization test Use SetPermissions with wildcard to grant permissions to Editor user for all dashboards, not just the initial one. This ensures that dashboards created during tests (like in DELETE operations) have the necessary permissions for the editor role. --- .../apis/provisioning/resources/dualwriter.go | 44 +- pkg/tests/apis/provisioning/files_test.go | 390 ++++++++++++++++++ 2 files changed, 416 insertions(+), 18 deletions(-) diff --git a/pkg/registry/apis/provisioning/resources/dualwriter.go b/pkg/registry/apis/provisioning/resources/dualwriter.go index 8ffcee696e8..7c9005a8dd5 100644 --- a/pkg/registry/apis/provisioning/resources/dualwriter.go +++ b/pkg/registry/apis/provisioning/resources/dualwriter.go @@ -503,35 +503,43 @@ func (r *DualReadWriter) authorize(ctx context.Context, parsed *ParsedResource, }, parsed.Meta.GetFolder()) if err != nil || !rsp.Allowed { return apierrors.NewForbidden(parsed.GVR.GroupResource(), parsed.Obj.GetName(), - fmt.Errorf("no access to read the embedded file")) + fmt.Errorf("no access to perform %s on the resource", verb)) } - idType, _, err := authlib.ParseTypeID(id.GetID()) - if err != nil { - return apierrors.NewForbidden(parsed.GVR.GroupResource(), parsed.Obj.GetName(), fmt.Errorf("could not determine identity type to check access")) - } - // only apply role based access if identity is not of type access policy - if idType == authlib.TypeAccessPolicy || id.GetOrgRole().Includes(identity.RoleEditor) { - return nil - } - - return apierrors.NewForbidden(parsed.GVR.GroupResource(), parsed.Obj.GetName(), - fmt.Errorf("must be admin or editor to access files from provisioning")) + return nil } -func (r *DualReadWriter) authorizeCreateFolder(ctx context.Context, _ string) error { +func (r *DualReadWriter) authorizeCreateFolder(ctx context.Context, path string) error { id, err := identity.GetRequester(ctx) if err != nil { return apierrors.NewUnauthorized(err.Error()) } - // Simple role based access for now - if id.GetOrgRole().Includes(identity.RoleEditor) { - return nil + // Determine parent folder from path + parentFolder := "" + if path != "" { + parentPath := safepath.Dir(path) + if parentPath != "" { + parentFolder = ParseFolder(parentPath, r.repo.Config().Name).ID + } else { + parentFolder = RootFolder(r.repo.Config()) + } } - return apierrors.NewForbidden(FolderResource.GroupResource(), "", - fmt.Errorf("must be admin or editor to access folders with provisioning")) + // For folder create operations, use empty name to check parent folder permissions + rsp, err := r.access.Check(ctx, id, authlib.CheckRequest{ + Group: FolderResource.Group, + Resource: FolderResource.Resource, + Namespace: id.GetNamespace(), + Name: "", // Empty name for create operations + Verb: utils.VerbCreate, + }, parentFolder) + if err != nil || !rsp.Allowed { + return apierrors.NewForbidden(FolderResource.GroupResource(), path, + fmt.Errorf("no access to create folder in parent folder '%s'", parentFolder)) + } + + return nil } func (r *DualReadWriter) deleteFolder(ctx context.Context, opts DualWriteOptions) (*ParsedResource, error) { diff --git a/pkg/tests/apis/provisioning/files_test.go b/pkg/tests/apis/provisioning/files_test.go index 823241aa6b0..e31674be505 100644 --- a/pkg/tests/apis/provisioning/files_test.go +++ b/pkg/tests/apis/provisioning/files_test.go @@ -1,7 +1,9 @@ package provisioning import ( + "bytes" "context" + "encoding/json" "errors" "fmt" "io" @@ -11,6 +13,7 @@ import ( "testing" "github.com/grafana/grafana/pkg/apimachinery/utils" + "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" "github.com/grafana/grafana/pkg/util/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -590,3 +593,390 @@ func TestIntegrationProvisioning_FilesOwnershipProtection(t *testing.T) { require.Equal(t, repo2, dashboard2.GetAnnotations()[utils.AnnoKeyManagerIdentity], "repo2's dashboard should still be owned by repo2") }) } + +func TestIntegrationProvisioning_FilesAuthorization(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + helper := runGrafana(t) + ctx := context.Background() + + const repo = "auth-test-repo" + helper.CreateRepo(t, TestRepo{ + Name: repo, + Path: helper.ProvisioningPath, + Target: "instance", + Copies: map[string]string{ + "testdata/all-panels.json": "dashboard1.json", + }, + ExpectedDashboards: 1, + ExpectedFolders: 0, + }) + + // Wait for initial sync to complete + var dashboardUID string + require.EventuallyWithT(t, func(collect *assert.CollectT) { + dashboards, err := helper.DashboardsV1.Resource.List(t.Context(), metav1.ListOptions{}) + if err != nil { + collect.Errorf("could not list dashboards error: %s", err.Error()) + return + } + if len(dashboards.Items) != 1 { + collect.Errorf("should have the expected dashboards after sync. got: %d. expected: %d", len(dashboards.Items), 1) + return + } + assert.Len(collect, dashboards.Items, 1) + dashboardUID = dashboards.Items[0].GetName() + }, waitTimeoutDefault, waitIntervalDefault, "should have the expected dashboards after sync") + + // Grant permissions to Editor user for all dashboards using wildcard + // The access checker checks resource-level permissions, so we need to grant them + // Using wildcard "*" to grant permissions to all dashboards (including ones created during tests) + // Note: Viewer role gets permissions via HTTP API below, Editor gets them here via SetPermissions + helper.SetPermissions(helper.Org1.Editor, []resourcepermissions.SetResourcePermissionCommand{ + { + Actions: []string{"dashboards:read", "dashboards:write", "dashboards:delete"}, + Resource: "dashboards", + ResourceAttribute: "uid", + ResourceID: "*", + }, + }) + + // Grant view permission to Viewer role via HTTP API (for the initial dashboard) + // Note: This only grants permissions to the initial dashboard, but viewers should be able to read all + addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String() + setDashboardPermissions := func(permissions []map[string]interface{}) { + payload := map[string]interface{}{ + "items": permissions, + } + payloadBytes, err := json.Marshal(payload) + require.NoError(t, err) + url := fmt.Sprintf("http://admin:admin@%s/api/dashboards/uid/%s/permissions", addr, dashboardUID) + req, err := http.NewRequest(http.MethodPost, url, bytes.NewBuffer(payloadBytes)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.NoError(t, resp.Body.Close()) + } + + // Grant view permission to Viewer role for the initial dashboard + setDashboardPermissions([]map[string]interface{}{ + {"role": "Viewer", "permission": 1}, // View permission + }) + + t.Run("GET operations", func(t *testing.T) { + t.Run("viewer can GET files", func(t *testing.T) { + var statusCode int + result := helper.ViewerREST.Get(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "dashboard1.json"). + Do(ctx).StatusCode(&statusCode) + + require.NoError(t, result.Error(), "viewer should be able to GET files") + require.Equal(t, http.StatusOK, statusCode, "should return 200 OK") + }) + + t.Run("editor can GET files", func(t *testing.T) { + var statusCode int + result := helper.EditorREST.Get(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "dashboard1.json"). + Do(ctx).StatusCode(&statusCode) + + require.NoError(t, result.Error(), "editor should be able to GET files") + require.Equal(t, http.StatusOK, statusCode, "should return 200 OK") + }) + + t.Run("admin can GET files", func(t *testing.T) { + var statusCode int + result := helper.AdminREST.Get(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "dashboard1.json"). + Do(ctx).StatusCode(&statusCode) + + require.NoError(t, result.Error(), "admin should be able to GET files") + require.Equal(t, http.StatusOK, statusCode, "should return 200 OK") + }) + }) + + t.Run("POST operations", func(t *testing.T) { + t.Run("viewer cannot POST files", func(t *testing.T) { + var statusCode int + result := helper.ViewerREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "viewer-test.json"). + Body(helper.LoadFile("testdata/text-options.json")). + SetHeader("Content-Type", "application/json"). + Do(ctx).StatusCode(&statusCode) + + require.Error(t, result.Error(), "viewer should not be able to POST files") + require.Equal(t, http.StatusForbidden, statusCode, "should return 403 Forbidden") + require.True(t, apierrors.IsForbidden(result.Error()), "error should be forbidden") + }) + + t.Run("editor can POST files", func(t *testing.T) { + var statusCode int + result := helper.EditorREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "editor-test.json"). + Body(helper.LoadFile("testdata/text-options.json")). + SetHeader("Content-Type", "application/json"). + Do(ctx).StatusCode(&statusCode) + + require.NoError(t, result.Error(), "editor should be able to POST files") + require.Equal(t, http.StatusOK, statusCode, "should return 200 OK") + + // Clean up + helper.AdminREST.Delete(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "editor-test.json"). + Do(ctx) + }) + + t.Run("admin can POST files", func(t *testing.T) { + var statusCode int + result := helper.AdminREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "admin-test.json"). + Body(helper.LoadFile("testdata/text-options.json")). + SetHeader("Content-Type", "application/json"). + Do(ctx).StatusCode(&statusCode) + + require.NoError(t, result.Error(), "admin should be able to POST files") + require.Equal(t, http.StatusOK, statusCode, "should return 200 OK") + + // Clean up + helper.AdminREST.Delete(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "admin-test.json"). + Do(ctx) + }) + }) + + t.Run("PUT operations", func(t *testing.T) { + // Create a test file first using admin + helper.AdminREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "update-test.json"). + Body(helper.LoadFile("testdata/text-options.json")). + SetHeader("Content-Type", "application/json"). + Do(ctx) + + t.Run("viewer cannot PUT files", func(t *testing.T) { + var statusCode int + result := helper.ViewerREST.Put(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "update-test.json"). + Body(helper.LoadFile("testdata/timeline-demo.json")). + SetHeader("Content-Type", "application/json"). + Do(ctx).StatusCode(&statusCode) + + require.Error(t, result.Error(), "viewer should not be able to PUT files") + require.Equal(t, http.StatusForbidden, statusCode, "should return 403 Forbidden") + require.True(t, apierrors.IsForbidden(result.Error()), "error should be forbidden") + }) + + t.Run("editor can PUT files", func(t *testing.T) { + var statusCode int + result := helper.EditorREST.Put(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "update-test.json"). + Body(helper.LoadFile("testdata/timeline-demo.json")). + SetHeader("Content-Type", "application/json"). + Do(ctx).StatusCode(&statusCode) + + require.NoError(t, result.Error(), "editor should be able to PUT files") + require.Equal(t, http.StatusOK, statusCode, "should return 200 OK") + }) + + t.Run("admin can PUT files", func(t *testing.T) { + var statusCode int + result := helper.AdminREST.Put(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "update-test.json"). + Body(helper.LoadFile("testdata/text-options.json")). + SetHeader("Content-Type", "application/json"). + Do(ctx).StatusCode(&statusCode) + + require.NoError(t, result.Error(), "admin should be able to PUT files") + require.Equal(t, http.StatusOK, statusCode, "should return 200 OK") + }) + + // Clean up + helper.AdminREST.Delete(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "update-test.json"). + Do(ctx) + }) + + t.Run("DELETE operations", func(t *testing.T) { + // Create test files for deletion tests + helper.AdminREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "delete-viewer-test.json"). + Body(helper.LoadFile("testdata/text-options.json")). + SetHeader("Content-Type", "application/json"). + Do(ctx) + + helper.AdminREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "delete-editor-test.json"). + Body(helper.LoadFile("testdata/text-options.json")). + SetHeader("Content-Type", "application/json"). + Do(ctx) + + helper.AdminREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "delete-admin-test.json"). + Body(helper.LoadFile("testdata/text-options.json")). + SetHeader("Content-Type", "application/json"). + Do(ctx) + + t.Run("viewer cannot DELETE files", func(t *testing.T) { + var statusCode int + result := helper.ViewerREST.Delete(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "delete-viewer-test.json"). + Do(ctx).StatusCode(&statusCode) + + require.Error(t, result.Error(), "viewer should not be able to DELETE files") + require.Equal(t, http.StatusForbidden, statusCode, "should return 403 Forbidden") + require.True(t, apierrors.IsForbidden(result.Error()), "error should be forbidden") + + // Verify file still exists + _, err := helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", "delete-viewer-test.json") + require.NoError(t, err, "file should still exist after failed delete") + }) + + t.Run("editor can DELETE files", func(t *testing.T) { + var statusCode int + result := helper.EditorREST.Delete(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "delete-editor-test.json"). + Do(ctx).StatusCode(&statusCode) + + require.NoError(t, result.Error(), "editor should be able to DELETE files") + require.Equal(t, http.StatusOK, statusCode, "should return 200 OK") + + // Verify file was deleted + _, err := helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", "delete-editor-test.json") + require.Error(t, err, "file should be deleted") + require.True(t, apierrors.IsNotFound(err), "should return NotFound for deleted file") + }) + + t.Run("admin can DELETE files", func(t *testing.T) { + var statusCode int + result := helper.AdminREST.Delete(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "delete-admin-test.json"). + Do(ctx).StatusCode(&statusCode) + + require.NoError(t, result.Error(), "admin should be able to DELETE files") + require.Equal(t, http.StatusOK, statusCode, "should return 200 OK") + + // Verify file was deleted + _, err := helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", "delete-admin-test.json") + require.Error(t, err, "file should be deleted") + require.True(t, apierrors.IsNotFound(err), "should return NotFound for deleted file") + }) + }) + + t.Run("folder operations", func(t *testing.T) { + t.Run("viewer cannot create folders", func(t *testing.T) { + // Create a folder by POSTing to a directory path + addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String() + url := fmt.Sprintf("http://viewer:viewer@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/test-folder/", addr, repo) + req, err := http.NewRequest(http.MethodPost, url, nil) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + // nolint:errcheck + defer resp.Body.Close() + + require.Equal(t, http.StatusForbidden, resp.StatusCode, "viewer should not be able to create folders") + }) + + t.Run("editor can create folders", func(t *testing.T) { + addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String() + url := fmt.Sprintf("http://editor:editor@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/editor-folder/", addr, repo) + req, err := http.NewRequest(http.MethodPost, url, nil) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + // nolint:errcheck + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode, "editor should be able to create folders") + + // Clean up - delete folder + deleteURL := fmt.Sprintf("http://admin:admin@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/editor-folder/", addr, repo) + deleteReq, err := http.NewRequest(http.MethodDelete, deleteURL, nil) + require.NoError(t, err) + deleteResp, err := http.DefaultClient.Do(deleteReq) + require.NoError(t, err) + // nolint:errcheck + defer deleteResp.Body.Close() + }) + + t.Run("admin can create folders", func(t *testing.T) { + addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String() + url := fmt.Sprintf("http://admin:admin@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/admin-folder/", addr, repo) + req, err := http.NewRequest(http.MethodPost, url, nil) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + // nolint:errcheck + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode, "admin should be able to create folders") + + // Clean up - delete folder + deleteURL := fmt.Sprintf("http://admin:admin@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/admin-folder/", addr, repo) + deleteReq, err := http.NewRequest(http.MethodDelete, deleteURL, nil) + require.NoError(t, err) + deleteResp, err := http.DefaultClient.Do(deleteReq) + require.NoError(t, err) + // nolint:errcheck + defer deleteResp.Body.Close() + }) + }) +}