public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

unistore: add check when update the folder of a resource (#102699)

Browse Source 
* Add check for move folder

* make the server test generic

* address comment
This commit is contained in:
Georges Chaudy
2025-04-14 16:57:40 +02:00
committed by GitHub
parent 27795ff7bf
commit e69052a417
3 changed files with 401 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/storage/unified/resource/server.go
+81 -20
View File
@@ -384,23 +384,16 @@ func (s *server) newEvent(ctx context.Context, user claims.AuthInfo, key *Resour
return nil, NewBadRequestError("can not save annotation: " + utils.AnnoKeyGrantPermissions)
}
check := claims.CheckRequest{
Verb: utils.VerbCreate,
Group: key.Group,
Resource: key.Resource,
Namespace: key.Namespace,
}
event := &WriteEvent{
Value: value,
Key: key,
Object: obj,
}
if oldValue == nil {
event.Type = WatchEvent_ADDED
} else {
event.Type = WatchEvent_MODIFIED
check.Verb = utils.VerbUpdate
temp := &unstructured.Unstructured{}
err = temp.UnmarshalJSON(oldValue)
@@ -444,19 +437,34 @@ func (s *server) newEvent(ctx context.Context, user claims.AuthInfo, key *Resour
return nil, err
}
// We only set name for update checks
if check.Verb == utils.VerbUpdate {
check.Name = key.Name
}
// For folder moves, we need to check permissions on both folders
if s.isFolderMove(event) {
if err := s.checkFolderMovePermissions(ctx, user, key, event.ObjectOld.GetFolder(), obj.GetFolder()); err != nil {
return nil, err
}
} else {
// Regular permission check for create/update
check := claims.CheckRequest{
Verb: utils.VerbCreate,
Group: key.Group,
Resource: key.Resource,
Namespace: key.Namespace,
}
check.Folder = obj.GetFolder()
a, err := s.access.Check(ctx, user, check)
if err != nil {
return nil, AsErrorResult(err)
}
if !a.Allowed {
return nil, &ErrorResult{
Code: http.StatusForbidden,
if event.Type == WatchEvent_MODIFIED {
check.Verb = utils.VerbUpdate
check.Name = key.Name
}
check.Folder = obj.GetFolder()
a, err := s.access.Check(ctx, user, check)
if err != nil {
return nil, AsErrorResult(err)
}
if !a.Allowed {
return nil, &ErrorResult{
Code: http.StatusForbidden,
}
}
}
@@ -470,6 +478,59 @@ func (s *server) newEvent(ctx context.Context, user claims.AuthInfo, key *Resour
return event, nil
}
// isFolderMove determines if an event represents a resource being moved between folders
func (s *server) isFolderMove(event *WriteEvent) bool {
return event.Type == WatchEvent_MODIFIED &&
event.ObjectOld != nil &&
event.ObjectOld.GetFolder() != event.Object.GetFolder()
}
// checkFolderMovePermissions handles permission checks when a resource is being moved between folders
func (s *server) checkFolderMovePermissions(ctx context.Context, user claims.AuthInfo, key *ResourceKey, oldFolder, newFolder string) *ErrorResult {
// First check if user can update the resource in the original folder
updateCheck := claims.CheckRequest{
Verb: utils.VerbUpdate,
Group: key.Group,
Resource: key.Resource,
Namespace: key.Namespace,
Name: key.Name,
Folder: oldFolder,
}
a, err := s.access.Check(ctx, user, updateCheck)
if err != nil {
return AsErrorResult(err)
}
if !a.Allowed {
return &ErrorResult{
Code: http.StatusForbidden,
Message: "not allowed to update resource in the source folder",
}
}
// Then check if user can create the resource in the destination folder
createCheck := claims.CheckRequest{
Verb: utils.VerbCreate,
Group: key.Group,
Resource: key.Resource,
Namespace: key.Namespace,
Folder: newFolder,
}
a, err = s.access.Check(ctx, user, createCheck)
if err != nil {
return AsErrorResult(err)
}
if !a.Allowed {
return &ErrorResult{
Code: http.StatusForbidden,
Message: "not allowed to create resource in the destination folder",
}
}
return nil
}
func (s *server) Create(ctx context.Context, req *CreateRequest) (*CreateResponse, error) {
ctx, span := s.tracer.Start(ctx, "storage_server.Create")
defer span.End()
pkg/storage/unified/sql/test/integration_test.go
+22
View File
@@ -29,6 +29,28 @@ func TestMain(m *testing.M) {
testsuite.Run(m)
}
func TestIntegrationStorageServer(t *testing.T) {
if infraDB.IsTestDBSpanner() {
t.Skip("skipping integration test")
}
unitest.RunStorageServerTest(t, func(ctx context.Context) resource.StorageBackend {
dbstore := infraDB.InitTestDB(t)
eDB, err := dbimpl.ProvideResourceDB(dbstore, setting.NewCfg(), nil)
require.NoError(t, err)
require.NotNil(t, eDB)
backend, err := sql.NewBackend(sql.BackendOptions{
DBProvider: eDB,
IsHA: true,
})
require.NoError(t, err)
require.NotNil(t, backend)
err = backend.Init(testutil.NewDefaultTestContext(t))
require.NoError(t, err)
return backend
})
}
// TestStorageBackend is a test for the StorageBackend interface.
func TestIntegrationSQLStorageBackend(t *testing.T) {
if infraDB.IsTestDBSpanner() {
pkg/storage/unified/testing/server.go
+298
View File
@@ -0,0 +1,298 @@
package test
import (
"context"
"fmt"
"net/http"
"testing"
"github.com/grafana/authlib/types"
"github.com/grafana/grafana/pkg/apimachinery/identity"
"github.com/grafana/grafana/pkg/apimachinery/utils"
"github.com/grafana/grafana/pkg/storage/unified/resource"
"github.com/stretchr/testify/require"
)
// RunStorageServerTest runs the storage server test suite
func RunStorageServerTest(t *testing.T, newBackend NewBackendFunc) {
runTestResourcePermissionScenarios(t, newBackend(context.Background()), GenerateRandomNSPrefix())
}
// func runTestIntegrationBackendHappyPath(t *testing.T, backend resource.StorageBackend, nsPrefix string) {
func runTestResourcePermissionScenarios(t *testing.T, backend resource.StorageBackend, nsPrefix string) {
// Test user
testUser := &identity.StaticRequester{
Type: types.TypeUser,
Login: "testuser",
UserID: 123,
UserUID: "u123",
OrgRole: identity.RoleAdmin,
IsGrafanaAdmin: true,
}
testCases := []struct {
name string
initialFolder string
targetFolder string
permissionMap map[string]bool
expectSuccess bool
expectedChecks int
expectedError int32 // HTTP status code for error, 0 if no error expected
}{
{
name: "Create resource in folder",
initialFolder: "folder1",
targetFolder: "", // No move, just create
permissionMap: map[string]bool{"folder1:create": true},
expectSuccess: true,
expectedChecks: 1,
expectedError: 0,
},
{
name: "Create resource denied",
initialFolder: "folder1",
targetFolder: "", // No move, just create
permissionMap: map[string]bool{"folder1:create": false},
expectSuccess: false,
expectedChecks: 1,
expectedError: http.StatusForbidden,
},
{
name: "Update resource in same folder",
initialFolder: "folder1",
targetFolder: "folder1", // Same folder, just update
permissionMap: map[string]bool{"folder1:update": true},
expectSuccess: true,
expectedChecks: 1,
expectedError: 0,
},
{
name: "Update resource denied",
initialFolder: "folder1",
targetFolder: "folder1", // Same folder
permissionMap: map[string]bool{"folder1:update": false},
expectSuccess: false,
expectedChecks: 1,
expectedError: http.StatusForbidden,
},
{
name: "Move resource to another folder - allowed",
initialFolder: "folder1",
targetFolder: "folder2", // Moving to a different folder
permissionMap: map[string]bool{"folder1:update": true, "folder2:create": true},
expectSuccess: true,
expectedChecks: 2, // Should check both folders
expectedError: 0,
},
{
name: "Move resource - source folder access denied",
initialFolder: "folder1",
targetFolder: "folder2",
permissionMap: map[string]bool{"folder1:update": false, "folder2:create": true},
expectSuccess: false,
expectedChecks: 1, // Should stop at first check
expectedError: http.StatusForbidden,
},
{
name: "Move resource - destination folder access denied",
initialFolder: "folder1",
targetFolder: "folder2",
permissionMap: map[string]bool{"folder1:update": true, "folder2:create": false},
expectSuccess: false,
expectedChecks: 2, // Should do both checks but fail on second
expectedError: http.StatusForbidden,
},
{
name: "Move resource - from empty folder to named folder",
initialFolder: "",
targetFolder: "folder1",
permissionMap: map[string]bool{":update": true, "folder1:create": true},
expectSuccess: true,
expectedChecks: 2,
expectedError: 0,
},
}
for i, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
// Create a unique resource name for each test
resourceName := fmt.Sprintf("test-resource-%d", i)
resourceUID := fmt.Sprintf("test123-%d", i)
// Create a mock access client with the test case's permission map
checksPerformed := []types.CheckRequest{}
mockAccess := &mockAccessClient{
allowed: false, // Default to false
allowedMap: tc.permissionMap,
checkFn: func(req types.CheckRequest) {
checksPerformed = append(checksPerformed, req)
},
}
server, err := resource.NewResourceServer(resource.ResourceServerOptions{
Backend: backend,
AccessClient: mockAccess,
})
require.NoError(t, err)
ctx := types.WithAuthInfo(context.Background(), testUser)
key := &resource.ResourceKey{
Group: "test.grafana.app",
Resource: "testresources",
Namespace: nsPrefix + "-ns1",
Name: resourceName,
}
if tc.targetFolder == "" {
// Create resource with unique name
resourceJSON := fmt.Sprintf(`{
"apiVersion": "test.grafana.app/v1",
"kind": "TestResource",
"metadata": {
"name": "%s",
"uid": "%s",
"namespace": "%s",
"annotations": {
"grafana.app/folder": "%s"
}
},
"spec": {
"title": "Test Resource %d"
}
}`, resourceName, resourceUID, nsPrefix+"-ns1", tc.initialFolder, i)
checksPerformed = []types.CheckRequest{}
created, err := server.Create(ctx, &resource.CreateRequest{
Value: []byte(resourceJSON),
Key: key,
})
require.NoError(t, err)
if tc.expectSuccess {
require.Nil(t, created.Error)
require.True(t, created.ResourceVersion > 0)
} else {
require.NotNil(t, created.Error)
require.Equal(t, tc.expectedError, created.Error.Code)
}
require.Len(t, checksPerformed, tc.expectedChecks)
if len(checksPerformed) > 0 {
require.Equal(t, utils.VerbCreate, checksPerformed[0].Verb)
require.Equal(t, tc.initialFolder, checksPerformed[0].Folder)
}
} else {
// Create a resource first, then update/move it
initialResourceJSON := fmt.Sprintf(`{
"apiVersion": "test.grafana.app/v1",
"kind": "TestResource",
"metadata": {
"name": "%s",
"uid": "%s",
"namespace": "%s",
"annotations": {
"grafana.app/folder": "%s"
}
},
"spec": {
"title": "Test Resource %d"
}
}`, resourceName, resourceUID, nsPrefix+"-ns1", tc.initialFolder, i)
// Override permissions for initial creation to always succeed
mockAccess.allowed = true
created, err := server.Create(ctx, &resource.CreateRequest{
Value: []byte(initialResourceJSON),
Key: key,
})
require.NoError(t, err)
require.Nil(t, created.Error)
// Now try the update/move with the configured permissions
targetResourceJSON := fmt.Sprintf(`{
"apiVersion": "test.grafana.app/v1",
"kind": "TestResource",
"metadata": {
"name": "%s",
"uid": "%s",
"namespace": "%s",
"annotations": {
"grafana.app/folder": "%s"
}
},
"spec": {
"title": "Test Resource %d Updated"
}
}`, resourceName, resourceUID, nsPrefix+"-ns1", tc.targetFolder, i)
mockAccess.allowed = false // Reset to use the map
checksPerformed = []types.CheckRequest{}
updated, err := server.Update(ctx, &resource.UpdateRequest{
Key: key,
Value: []byte(targetResourceJSON),
ResourceVersion: created.ResourceVersion,
})
require.NoError(t, err)
if tc.expectSuccess {
require.Nil(t, updated.Error)
require.True(t, updated.ResourceVersion > created.ResourceVersion)
} else {
require.NotNil(t, updated.Error)
require.Equal(t, tc.expectedError, updated.Error.Code)
}
require.Len(t, checksPerformed, tc.expectedChecks)
// Verify the correct permission checks were made
if tc.initialFolder != tc.targetFolder && len(checksPerformed) >= 2 {
// This is a folder move operation
require.Equal(t, utils.VerbUpdate, checksPerformed[0].Verb)
require.Equal(t, tc.initialFolder, checksPerformed[0].Folder)
require.Equal(t, utils.VerbCreate, checksPerformed[1].Verb)
require.Equal(t, tc.targetFolder, checksPerformed[1].Folder)
} else if len(checksPerformed) > 0 {
// Regular update, no folder change
require.Equal(t, utils.VerbUpdate, checksPerformed[0].Verb)
require.Equal(t, tc.initialFolder, checksPerformed[0].Folder)
}
}
})
}
}
// Mock access client for testing
type mockAccessClient struct {
allowed bool
allowedMap map[string]bool
checkFn func(types.CheckRequest)
}
func (m *mockAccessClient) Check(ctx context.Context, user types.AuthInfo, req types.CheckRequest) (types.CheckResponse, error) {
if m.checkFn != nil {
m.checkFn(req)
}
// Check specific folder:verb mappings if provided
if m.allowedMap != nil {
key := fmt.Sprintf("%s:%s", req.Folder, req.Verb)
if allowed, exists := m.allowedMap[key]; exists {
return types.CheckResponse{Allowed: allowed}, nil
}
}
return types.CheckResponse{Allowed: m.allowed}, nil
}
func (m *mockAccessClient) Compile(ctx context.Context, user types.AuthInfo, req types.ListRequest) (types.ItemChecker, error) {
return func(name, folder string) bool {
key := fmt.Sprintf("%s:%s", folder, req.Verb)
if allowed, exists := m.allowedMap[key]; exists {
return allowed
}
return m.allowed
}, nil
}