Authorization/Access control integrity

PoC to demonstrate the issue and the fix (repro valid in a controlled Grafana provisioning environment): Prerequisites: - Grafana instance with provisioning of folder metadata enabled (folder-level metadata via _folder.json). - Admin access to Grafana API for creating folders, setting permissions, and querying permissions. - A repository-based provisioning setup that supports git-backed incremental/full sync (as used by Grafana provisioning tests). Steps to reproduce (pre-fix behavior): 1) Create a folder A with a _folder.json containing stable metadata (Name, GVK, UID) and assign a permission entry, e.g., Admin for role Admin with permission level 4. 2) Move folder A to a new path B via a git mv (mv A B), commit, and push. This triggers a full or incremental sync that performs a delete of A and a create of B (new path, possibly new UID). 3) Run provisioning sync (full or incremental as appropriate). 4) Query the permissions for B. Expected outcome without the fix: ACL entries associated with A’s UID may be lost or reset because the resource identity changed. Steps to verify the fix (post-fix behavior): 1) After the same move (git mv A B) and provisioning sync, query B’s ACLs. 2) Expect to see the same permission entry (e.g., Admin:4) present on B, and that the UID/identity has been preserved or correctly updated in-place to maintain the same resource identity, due to the rename-detection logic collapsing delete+create into a single in-place update. Minimal API interaction example (adjust host, tokens, and IDs accordingly): - Set permission on A: POST /api/folders/{A_uid}/permissions with body {"items":[{"role":"Admin","permission":4}]} - Move via git: git mv A B; git commit -m "move A to B"; git push - Sync: Trigger full/incremental provisioning sync (or wait for CI to run it) - Verify on B: GET /api/folders/{B_uid}/permissions and ensure the entry {"role":"Admin","permission":4} remains present Note: In a scenario where A and B represent the same logical resource identity, the fix ensures the update path preserves UID and permissions, whereas without the fix the move could result in a new resource being created with a fresh UID and lost permissions.

diff --git a/pkg/tests/apis/provisioning/foldermetadata/permissions_move_test.go b/pkg/tests/apis/provisioning/foldermetadata/full_permissions_move_test.go similarity index 100% rename from pkg/tests/apis/provisioning/foldermetadata/permissions_move_test.go rename to pkg/tests/apis/provisioning/foldermetadata/full_permissions_move_test.go diff --git a/pkg/tests/apis/provisioning/git/common/testing.go b/pkg/tests/apis/provisioning/git/common/testing.go index 02fa4073a85b0..c04dd4785e491 100644 --- a/pkg/tests/apis/provisioning/git/common/testing.go +++ b/pkg/tests/apis/provisioning/git/common/testing.go @@ -334,6 +334,7 @@ func (h *GitTestHelper) SyncAndWait(t *testing.T, repoName string) { // repository and waits for all active jobs to complete. // //nolint:unused // Called from incremental_folder_metadata_test.go + func (h *GitTestHelper) SyncAndWaitIncremental(t *testing.T, repoName string) { t.Helper() h.triggerJobAndWaitForComplete(t, repoName, provisioning.JobSpec{ diff --git a/pkg/tests/apis/provisioning/git/foldermetadata_incremental/incremental_permissions_move_test.go b/pkg/tests/apis/provisioning/git/foldermetadata_incremental/incremental_permissions_move_test.go new file mode 100644 index 0000000000000..361d13361ad67 --- /dev/null +++ b/pkg/tests/apis/provisioning/git/foldermetadata_incremental/incremental_permissions_move_test.go @@ -0,0 +1,573 @@ +package foldermetadataincremental + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/grafana/grafana/pkg/tests/apis/provisioning/common" + gitcommon "github.com/grafana/grafana/pkg/tests/apis/provisioning/git/common" + "github.com/grafana/grafana/pkg/util/testutil" +) + +// TestIntegrationProvisioning_IncrementalSync_FolderMovePreservesPermissions verifies +// that custom permissions set on a provisioned folder are preserved after the folder is +// moved to a different location via git mv and an incremental sync is performed. +func TestIntegrationProvisioning_IncrementalSync_FolderMovePreservesPermissions(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + helper := gitcommon.RunGrafanaWithGitServer(t, common.WithProvisioningFolderMetadata) + ctx := context.Background() + + const repoName = "incr-move-perms" + + _, local := helper.CreateGitRepo(t, repoName, map[string][]byte{ + "teamA/_folder.json": folderMetadataJSON("team-a-uid", "Team A"), + "teamB/_folder.json": folderMetadataJSON("team-b-uid", "Team B"), + }) + + common.SyncAndWaitWithSuccess(t, helper, repoName) + + requireGitFolderState(t, helper, ctx, "team-a-uid", "Team A", "teamA", "") + requireGitFolderState(t, helper, ctx, "team-b-uid", "Team B", "teamB", "") + + addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String() + + // Set a known ACL on teamA (check 4) so we can assert it is not touched when + // a child folder is moved into it. + _, code, err := common.PostHelper(t, *helper.K8sTestHelper, + "/api/folders/team-a-uid/permissions", + map[string]interface{}{ + "items": []map[string]interface{}{ + {"role": "Admin", "permission": 4}, + }, + }, + helper.Org1.Admin) + require.NoError(t, err, "setting permissions on teamA folder should succeed") + require.Equal(t, http.StatusOK, code) + + // Set multiple role-based ACL entries on teamB (check 3): + // Viewer → View (1) and Editor → Edit (2). + _, code, err = common.PostHelper(t, *helper.K8sTestHelper, + "/api/folders/team-b-uid/permissions", + map[string]interface{}{ + "items": []map[string]interface{}{ + {"role": "Viewer", "permission": 1}, + {"role": "Editor", "permission": 2}, + }, + }, + helper.Org1.Admin) + require.NoError(t, err, "setting permissions on teamB folder should succeed") + require.Equal(t, http.StatusOK, code) + + // Snapshot permissions before the move and verify the expected entries are present. + teamAPermsBefore := snapshotFolderPermissions(t, addr, "team-a-uid") + requirePermissionsContainRole(t, teamAPermsBefore, "Admin", 4) + + teamBPermsBefore := snapshotFolderPermissions(t, addr, "team-b-uid") + requirePermissionsContainRole(t, teamBPermsBefore, "Viewer", 1) + requirePermissionsContainRole(t, teamBPermsBefore, "Editor", 2) + + // Move teamB inside teamA via git mv, commit, and push. + _, err = local.Git("mv", "teamB", "teamA/teamB") + require.NoError(t, err) + _, err = local.Git("commit", "-m", "move teamB into teamA") + require.NoError(t, err) + _, err = local.Git("push") + require.NoError(t, err) + + common.SyncAndWaitSuccessfulIncremental(t, helper, repoName) + + // teamB should now live under teamA with the same stable UID. + requireGitFolderState(t, helper, ctx, "team-b-uid", "Team B", "teamA/teamB", "team-a-uid") + + teamBPermsAfter := snapshotFolderPermissions(t, addr, "team-b-uid") + + // Check 1: The ACL entry count must not change after the move. + require.Equal(t, len(teamBPermsBefore), len(teamBPermsAfter), + "ACL entry count must not change after folder move") + + // Check 2 & 3: The full (role → permission) map must be identical before and after. + requireRolePermissionSetEqual(t, teamBPermsBefore, teamBPermsAfter) + requirePermissionsContainRole(t, teamBPermsAfter, "Viewer", 1) + requirePermissionsContainRole(t, teamBPermsAfter, "Editor", 2) + + // Check 4: teamA's own ACL must be unaffected by moving a child into it. + teamAPermsAfter := snapshotFolderPermissions(t, addr, "team-a-uid") + requireRolePermissionSetEqual(t, teamAPermsBefore, teamAPermsAfter) + + // Check 5: The moved folder must still be effectively accessible to a user + // carrying the granted role. + requireFolderAccessible(t, addr, "team-b-uid", "viewer", "viewer") +} + +// TestIntegrationProvisioning_IncrementalSync_FolderMoveDoesNotPreservePermissionsForLegacyFolder +// contrasts with the metadata test: a folder without _folder.json receives a brand-new UID when +// its path changes (delete + recreate), so permissions associated with the old UID are gone. +func TestIntegrationProvisioning_IncrementalSync_FolderMoveDoesNotPreservePermissionsForLegacyFolder(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + helper := gitcommon.RunGrafanaWithGitServer(t, common.WithProvisioningFolderMetadata) + ctx := context.Background() + + const repoName = "incr-move-legacy-perms" + + // Parent has stable metadata; the folder being moved does not. + // plain has no _folder.json, so syncs will warn about missing metadata. + _, local := helper.CreateGitRepo(t, repoName, map[string][]byte{ + "parent/_folder.json": folderMetadataJSON("parent-uid", "Parent"), + "plain/.keep": {}, + }) + + common.SyncAndWaitWithWarning(t, helper, repoName) + + requireGitFolderState(t, helper, ctx, "parent-uid", "Parent", "parent", "") + plainUID := findGitFolderUIDBySourcePath(t, helper, ctx, repoName, "plain") + require.NotEmpty(t, plainUID) + + addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String() + + // Set a Viewer permission on the legacy (no-metadata) folder. + _, code, err := common.PostHelper(t, *helper.K8sTestHelper, + fmt.Sprintf("/api/folders/%s/permissions", plainUID), + map[string]interface{}{ + "items": []map[string]interface{}{ + {"role": "Viewer", "permission": 1}, + }, + }, + helper.Org1.Admin) + require.NoError(t, err, "setting permissions on plain folder should succeed") + require.Equal(t, http.StatusOK, code) + + plainPermsBefore := snapshotFolderPermissions(t, addr, plainUID) + requirePermissionsContainRole(t, plainPermsBefore, "Viewer", 1) + + // Move the legacy folder under the parent via git mv, commit, and push. + _, err = local.Git("mv", "plain", "parent/plain") + require.NoError(t, err) + _, err = local.Git("commit", "-m", "move plain folder under parent") + require.NoError(t, err) + _, err = local.Git("push") + require.NoError(t, err) + + // parent/plain still has no _folder.json after the move, so the incremental + // sync warns about missing metadata. + common.SyncAndWaitIncrementalWithWarning(t, helper, repoName) + + // The old folder object must be gone — without metadata the path change + // causes a delete-and-recreate rather than an in-place update. + assertGitFolderAbsent(t, helper, ctx, plainUID) + + // The new folder at the moved path must exist with a different (hash-based) UID. + newPlainUID := findGitFolderUIDBySourcePath(t, helper, ctx, repoName, "parent/plain") + require.NotEmpty(t, newPlainUID) + require.NotEqual(t, plainUID, newPlainUID, + "legacy folder must get a new UID when its path changes") + + // The new folder must NOT carry the Viewer permission from the old object. + newPlainPerms := snapshotFolderPermissions(t, addr, newPlainUID) + for _, p := range newPlainPerms { + entry, ok := p.(map[string]interface{}) + if !ok { + continue + } + role, _ := entry["role"].(string) + level, _ := entry["permission"].(float64) + require.False(t, role == "Viewer" && int(level) == 1, + "new legacy folder must not inherit Viewer permission from the deleted predecessor; got: %v", newPlainPerms) + } +} + +// TestIntegrationProvisioning_IncrementalSync_NestedFolderMovePreservesPermissions verifies that +// permissions on a deeply nested folder survive when its parent subtree is relocated. +// All folders in the hierarchy carry _folder.json metadata, so UIDs are stable. +func TestIntegrationProvisioning_IncrementalSync_NestedFolderMovePreservesPermissions(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + helper := gitcommon.RunGrafanaWithGitServer(t, common.WithProvisioningFolderMetadata) + ctx := context.Background() + + const repoName = "incr-move-nested-perms" + + // Build root → child → grandchild; all have metadata so UIDs are stable. + // destination also carries _folder.json so it doesn't trigger a missing-metadata warning. + _, local := helper.CreateGitRepo(t, repoName, map[string][]byte{ + "root/_folder.json": folderMetadataJSON("root-uid", "Root"), + "root/child/_folder.json": folderMetadataJSON("child-uid", "Child"), + "root/child/grandchild/_folder.json": folderMetadataJSON("grandchild-uid", "Grandchild"), + "destination/_folder.json": folderMetadataJSON("destination-uid", "Destination"), + }) + + common.SyncAndWaitWithSuccess(t, helper, repoName) + + requireGitFolderState(t, helper, ctx, "root-uid", "Root", "root", "") + requireGitFolderState(t, helper, ctx, "child-uid", "Child", "root/child", "root-uid") + requireGitFolderState(t, helper, ctx, "grandchild-uid", "Grandchild", "root/child/grandchild", "child-uid") + requireGitFolderState(t, helper, ctx, "destination-uid", "Destination", "destination", "") + + addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String() + + // Grant Editor permission on the deepest (grandchild) folder. + _, code, err := common.PostHelper(t, *helper.K8sTestHelper, + "/api/folders/grandchild-uid/permissions", + map[string]interface{}{ + "items": []map[string]interface{}{ + {"role": "Editor", "permission": 2}, + }, + }, + helper.Org1.Admin) + require.NoError(t, err, "setting permissions on grandchild folder should succeed") + require.Equal(t, http.StatusOK, code) + + grandchildPermsBefore := snapshotFolderPermissions(t, addr, "grandchild-uid") + requirePermissionsContainRole(t, grandchildPermsBefore, "Editor", 2) + + // Move the child subtree (carrying grandchild with it) under destination via git mv. + _, err = local.Git("mv", "root/child", "destination/child") + require.NoError(t, err) + _, err = local.Git("commit", "-m", "move child subtree into destination") + require.NoError(t, err) + _, err = local.Git("push") + require.NoError(t, err) + + common.SyncAndWaitSuccessfulIncremental(t, helper, repoName) + + requireGitFolderState(t, helper, ctx, "destination-uid", "Destination", "destination", "") + requireGitFolderState(t, helper, ctx, "child-uid", "Child", "destination/child", "destination-uid") + requireGitFolderState(t, helper, ctx, "grandchild-uid", "Grandchild", "destination/child/grandchild", "child-uid") + + grandchildPermsAfter := snapshotFolderPermissions(t, addr, "grandchild-uid") + require.Equal(t, len(grandchildPermsBefore), len(grandchildPermsAfter), + "ACL entry count must not change after nested folder move") + requireRolePermissionSetEqual(t, grandchildPermsBefore, grandchildPermsAfter) + requirePermissionsContainRole(t, grandchildPermsAfter, "Editor", 2) +} + +// TestIntegrationProvisioning_IncrementalSync_RootToLeafMovePreservesPermissions verifies that +// a top-level (root) folder that is moved to a deeply nested position retains its permissions. +func TestIntegrationProvisioning_IncrementalSync_RootToLeafMovePreservesPermissions(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + helper := gitcommon.RunGrafanaWithGitServer(t, common.WithProvisioningFolderMetadata) + ctx := context.Background() + + const repoName = "incr-move-root-to-leaf" + + _, local := helper.CreateGitRepo(t, repoName, map[string][]byte{ + "top/_folder.json": folderMetadataJSON("top-uid", "Top"), + "container/_folder.json": folderMetadataJSON("container-uid", "Container"), + "container/inner/_folder.json": folderMetadataJSON("inner-uid", "Inner"), + }) + + common.SyncAndWaitWithSuccess(t, helper, repoName) + + requireGitFolderState(t, helper, ctx, "top-uid", "Top", "top", "") + requireGitFolderState(t, helper, ctx, "container-uid", "Container", "container", "") + requireGitFolderState(t, helper, ctx, "inner-uid", "Inner", "container/inner", "container-uid") + + addr := helper.GetEnv().Server.HTTPServer.Listener.Addr().String() + + // Grant Viewer permission on the root-level "top" folder before the move. + _, code, err := common.PostHelper(t, *helper.K8sTestHelper, + "/api/folders/top-uid/permissions", + map[string]interface{}{ + "items": []map[string]interface{}{ + {"role": "Viewer", "permission": 1}, + }, + }, + helper.Org1.Admin) + require.NoError(t, err, "setting permissions on top folder should succeed") + require.Equal(t, http.StatusOK, code) + + topPermsBefore := snapshotFolderPermissions(t, addr, "top-uid") + requirePermissionsContainRole(t, topPermsBefore, "Viewer", 1) + + // Move "top" under container/inner, making it a leaf three levels deep. + _, err = local.Git("mv", "top", "container/inner/top") + require.NoError(t, err) + _, err = local.Git("commit", "-m", "move top folder to leaf position under container/inner") + require.NoError(t, err) + _, err = local.Git("push") + require.NoError(t, err) + + common.SyncAndWaitSuccessfulIncremental(t, helper, repoName) + + requireGitFolderState(t, helper, ctx, "top-uid", "Top", "container/inner/top", "inner-uid") + + topPermsAfter := snapshotFolderPermissions(t, addr, "top-uid") + require.Equal(t, len(topPermsBefore), len(topPermsAfter), + "ACL entry co ... [truncated]