Resource Permissions: Move validator higher up (#111557)

* move resource permission create and update validator higher up the chain

* undo unwanted change
This commit is contained in:
Ieva
2025-09-26 11:30:28 +00:00
committed by GitHub
parent 41de5fa380
commit 2b86de8b7f
8 changed files with 260 additions and 278 deletions
+6
View File
@@ -328,9 +328,15 @@ func (b *IdentityAccessManagementAPIBuilder) Validate(ctx context.Context, a adm
return serviceaccount.ValidateOnCreate(ctx, typedObj)
case *iamv0.Team:
return team.ValidateOnCreate(ctx, typedObj)
case *iamv0.ResourcePermission:
return resourcepermission.ValidateCreateAndUpdateInput(ctx, typedObj)
}
return nil
case admission.Update:
switch typedObj := a.GetObject().(type) {
case *iamv0.ResourcePermission:
return resourcepermission.ValidateCreateAndUpdateInput(ctx, typedObj)
}
return nil
case admission.Delete:
return nil
@@ -249,22 +249,28 @@ func (s *ResourcePermSqlBackend) parseScope(scope string) (*groupResourceName, e
}
// splitResourceName splits a resource name in the format <group>-<resource>-<name> (e.g. dashboard.grafana.app-dashboards-ad5rwqs) into its components
func (s *ResourcePermSqlBackend) splitResourceName(resourceName string) (Mapper, *groupResourceName, error) {
func splitResourceName(resourceName string) (*groupResourceName, error) {
// e.g. dashboard.grafana.app-dashboards-ad5rwqs
parts := strings.SplitN(resourceName, "-", 3)
if len(parts) != 3 {
return nil, nil, fmt.Errorf("%w: %s", errInvalidName, resourceName)
return nil, fmt.Errorf("%w: %s", errInvalidName, resourceName)
}
group, resourceType, uid := parts[0], parts[1], parts[2]
mapper, ok := s.mappers[schema.GroupResource{Group: group, Resource: resourceType}]
if !ok {
return nil, nil, fmt.Errorf("%w: %s/%s", errUnknownGroupResource, group, resourceType)
}
return mapper, &groupResourceName{
return &groupResourceName{
Group: group,
Resource: resourceType,
Name: uid,
}, nil
}
// getResourceMapper returns the Mapper of the given group and resource to access levels and scope prefix for that resource.
func (s *ResourcePermSqlBackend) getResourceMapper(group, resource string) (Mapper, error) {
mapper, ok := s.mappers[schema.GroupResource{Group: group, Resource: resource}]
if !ok {
return nil, fmt.Errorf("%w: %s/%s", errUnknownGroupResource, group, resource)
}
return mapper, nil
}
+12 -39
View File
@@ -158,7 +158,12 @@ func (s *ResourcePermSqlBackend) getRbacAssignmentsWithTx(ctx context.Context, s
// getResourcePermission retrieves a single ResourcePermission by its name in the format <group>-<resource>-<name> (e.g. dashboard.grafana.app-dashboards-ad5rwqs)
func (s *ResourcePermSqlBackend) getResourcePermission(ctx context.Context, sql *legacysql.LegacyDatabaseHelper, tx *session.SessionTx, ns types.NamespaceInfo, name string) (*v0alpha1.ResourcePermission, error) {
mapper, grn, err := s.splitResourceName(name)
grn, err := splitResourceName(name)
if err != nil {
return nil, apierrors.NewInternalError(err)
}
mapper, err := s.getResourceMapper(grn.Group, grn.Resource)
if err != nil {
return nil, apierrors.NewInternalError(err)
}
@@ -371,10 +376,6 @@ func (s *ResourcePermSqlBackend) existsResourcePermission(ctx context.Context, t
func (s *ResourcePermSqlBackend) createResourcePermission(
ctx context.Context, dbHelper *legacysql.LegacyDatabaseHelper, ns types.NamespaceInfo, mapper Mapper, grn *groupResourceName, v0ResourcePerm *v0alpha1.ResourcePermission,
) (int64, error) {
if err := validateCreateAndUpdateInput(v0ResourcePerm, grn); err != nil {
return 0, err
}
assignments, err := s.buildRbacAssignments(ctx, ns, mapper, v0ResourcePerm.Spec.Permissions, mapper.Scope(grn.Name))
if err != nil {
return 0, err
@@ -404,10 +405,6 @@ func (s *ResourcePermSqlBackend) createResourcePermission(
}
func (s *ResourcePermSqlBackend) updateResourcePermission(ctx context.Context, dbHelper *legacysql.LegacyDatabaseHelper, ns types.NamespaceInfo, mapper Mapper, grn *groupResourceName, v0ResourcePerm *v0alpha1.ResourcePermission) (int64, error) {
if err := validateCreateAndUpdateInput(v0ResourcePerm, grn); err != nil {
return 0, err
}
err := dbHelper.DB.GetSqlxSession().WithTransaction(ctx, func(tx *session.SessionTx) error {
currentPerms, err := s.getResourcePermission(ctx, dbHelper, tx, ns, grn.string())
if err != nil {
@@ -500,38 +497,14 @@ func diffPermissions(currentPermissions, desiredPermissions []v0alpha1.ResourceP
return permissionsToAdd, permissionsToRemove
}
func validateCreateAndUpdateInput(v0ResourcePerm *v0alpha1.ResourcePermission, grn *groupResourceName) error {
if v0ResourcePerm == nil {
return fmt.Errorf("resource permission cannot be nil")
}
if len(v0ResourcePerm.Spec.Permissions) == 0 {
return fmt.Errorf("resource permission must have at least one permission: %w", errInvalidSpec)
}
// Validate that the group/resource/name in the name matches the spec
if grn.Group != v0ResourcePerm.Spec.Resource.ApiGroup ||
grn.Resource != v0ResourcePerm.Spec.Resource.Resource ||
grn.Name != v0ResourcePerm.Spec.Resource.Name {
return fmt.Errorf("resource permission name does not match spec: %w", errInvalidSpec)
}
// Check for duplicate entities (same kind and name should appear only once)
seen := make(map[string]bool)
for _, perm := range v0ResourcePerm.Spec.Permissions {
key := fmt.Sprintf("%s:%s", perm.Kind, perm.Name)
if seen[key] {
return fmt.Errorf("duplicate entity found: kind=%s, name=%s (each entity can only appear once per resource): %w", perm.Kind, perm.Name, errInvalidSpec)
}
seen[key] = true
}
return nil
}
// deleteResourcePermission deletes resource permissions for a single ResourcePermission resource referenced by its name in the format <group>-<resource>-<name> (e.g. dashboard.grafana.app-dashboards-ad5rwqs)
func (s *ResourcePermSqlBackend) deleteResourcePermission(ctx context.Context, sql *legacysql.LegacyDatabaseHelper, ns types.NamespaceInfo, name string) error {
mapper, grn, err := s.splitResourceName(name)
grn, err := splitResourceName(name)
if err != nil {
return err
}
mapper, err := s.getResourceMapper(grn.Group, grn.Resource)
if err != nil {
return err
}
@@ -450,7 +450,10 @@ func TestIntegration_ResourcePermSqlBackend_CreateResourcePermission(t *testing.
sqlHelper, _ := backend.dbProvider(ctx)
backend.identityStore = NewFakeIdentityStore(t)
mapper, grn, err := backend.splitResourceName(resourcePerm.Name)
grn, err := splitResourceName(resourcePerm.Name)
require.NoError(t, err)
mapper, err := backend.getResourceMapper(grn.Group, grn.Resource)
require.NoError(t, err)
rv, err := backend.createResourcePermission(ctx, sqlHelper, types.NamespaceInfo{Value: "default", OrgID: 1}, mapper, grn, resourcePerm)
@@ -544,7 +547,10 @@ func TestIntegration_ResourcePermSqlBackend_UpdateResourcePermission(t *testing.
},
}
mapper, grn, err := backend.splitResourceName(resourcePerm.Name)
grn, err := splitResourceName(resourcePerm.Name)
require.NoError(t, err)
mapper, err := backend.getResourceMapper(grn.Group, grn.Resource)
require.NoError(t, err)
_, err = backend.updateResourcePermission(ctx, sql, types.NamespaceInfo{Value: "default", OrgID: 1}, mapper, grn, resourcePerm)
@@ -583,7 +589,10 @@ func TestIntegration_ResourcePermSqlBackend_UpdateResourcePermission(t *testing.
},
}
mapper, grn, err := backend.splitResourceName(resourcePerm.Name)
grn, err := splitResourceName(resourcePerm.Name)
require.NoError(t, err)
mapper, err := backend.getResourceMapper(grn.Group, grn.Resource)
require.NoError(t, err)
rv, err := backend.updateResourcePermission(ctx, sql, types.NamespaceInfo{Value: "default", OrgID: 1}, mapper, grn, resourcePerm)
@@ -670,104 +679,6 @@ func (f *fakeIdentityStore) GetUserInternalID(ctx context.Context, ns types.Name
return &legacy.GetUserInternalIDResult{ID: id}, nil
}
func TestValidateCreateAndUpdateInput(t *testing.T) {
grn := &groupResourceName{
Group: "dashboard.grafana.app",
Resource: "dashboards",
Name: "test-dashboard",
}
t.Run("Should pass validation with valid permissions", func(t *testing.T) {
resourcePerm := &v0alpha1.ResourcePermission{
Spec: v0alpha1.ResourcePermissionSpec{
Resource: v0alpha1.ResourcePermissionspecResource{
ApiGroup: "dashboard.grafana.app",
Resource: "dashboards",
Name: "test-dashboard",
},
Permissions: []v0alpha1.ResourcePermissionspecPermission{
{
Kind: v0alpha1.ResourcePermissionSpecPermissionKindBasicRole,
Name: "Editor",
Verb: "edit",
},
{
Kind: v0alpha1.ResourcePermissionSpecPermissionKindBasicRole,
Name: "Viewer", // Different entity name - should be allowed
Verb: "view",
},
{
Kind: v0alpha1.ResourcePermissionSpecPermissionKindUser,
Name: "user-1",
Verb: "edit", // Different kind - should be allowed
},
},
},
}
err := validateCreateAndUpdateInput(resourcePerm, grn)
require.NoError(t, err)
})
t.Run("Should fail validation with duplicate entities", func(t *testing.T) {
resourcePerm := &v0alpha1.ResourcePermission{
Spec: v0alpha1.ResourcePermissionSpec{
Resource: v0alpha1.ResourcePermissionspecResource{
ApiGroup: "dashboard.grafana.app",
Resource: "dashboards",
Name: "test-dashboard",
},
Permissions: []v0alpha1.ResourcePermissionspecPermission{
{
Kind: v0alpha1.ResourcePermissionSpecPermissionKindBasicRole,
Name: "Editor",
Verb: "edit",
},
{
Kind: v0alpha1.ResourcePermissionSpecPermissionKindBasicRole,
Name: "Editor",
Verb: "view", // Same entity, different verb - should fail
},
},
},
}
err := validateCreateAndUpdateInput(resourcePerm, grn)
require.Error(t, err)
require.Contains(t, err.Error(), "duplicate entity found")
require.Contains(t, err.Error(), "kind=BasicRole")
require.Contains(t, err.Error(), "name=Editor")
require.Contains(t, err.Error(), "each entity can only appear once per resource")
})
t.Run("Should pass validation with same name but different kinds", func(t *testing.T) {
resourcePerm := &v0alpha1.ResourcePermission{
Spec: v0alpha1.ResourcePermissionSpec{
Resource: v0alpha1.ResourcePermissionspecResource{
ApiGroup: "dashboard.grafana.app",
Resource: "dashboards",
Name: "test-dashboard",
},
Permissions: []v0alpha1.ResourcePermissionspecPermission{
{
Kind: v0alpha1.ResourcePermissionSpecPermissionKindUser,
Name: "editor", // Same name but different kind
Verb: "edit",
},
{
Kind: v0alpha1.ResourcePermissionSpecPermissionKindBasicRole,
Name: "editor", // Same name but different kind
Verb: "view",
},
},
},
}
err := validateCreateAndUpdateInput(resourcePerm, grn)
require.NoError(t, err)
})
}
func TestIntegration_UpdateResourcePermission_VerbChange(t *testing.T) {
testutil.SkipIntegrationTestInShortMode(t)
@@ -234,11 +234,16 @@ func (s *ResourcePermSqlBackend) WriteEvent(ctx context.Context, event resource.
return 0, errDatabaseHelper
}
mapper, grn, err := s.splitResourceName(event.Key.Name)
grn, err := splitResourceName(event.Key.Name)
if err != nil {
return 0, apierrors.NewBadRequest(fmt.Sprintf("invalid resource name %q: %v", event.Key.Name, err.Error()))
}
mapper, err := s.getResourceMapper(grn.Group, grn.Resource)
if err != nil {
return 0, apierrors.NewBadRequest(fmt.Sprintf("invalid group/resource in resource name %q: %v", event.Key.Name, err.Error()))
}
if grn.Name == "" {
return 0, fmt.Errorf("resource name cannot be empty: %w", errInvalidName)
}
@@ -143,71 +143,6 @@ func TestWriteEvent_Add(t *testing.T) {
require.Contains(t, err.Error(), "requires a valid namespace")
})
t.Run("should error if there is no permission", func(t *testing.T) {
backend := ProvideStorageBackend(dbProvider)
resourcePerm, err := utils.MetaAccessor(&v0alpha1.ResourcePermission{
ObjectMeta: metav1.ObjectMeta{
Name: "folder.grafana.app-folders-fold1",
Namespace: "default",
},
Spec: v0alpha1.ResourcePermissionSpec{
Resource: v0alpha1.ResourcePermissionspecResource{
ApiGroup: "folder.grafana.app",
Resource: "folders",
Name: "fold1",
},
},
})
require.NoError(t, err)
gr := v0alpha1.ResourcePermissionInfo.GroupResource()
rv, err := backend.WriteEvent(context.Background(), resource.WriteEvent{
Type: resourcepb.WatchEvent_ADDED,
Key: &resourcepb.ResourceKey{Group: gr.Group, Resource: gr.Resource, Name: "folder.grafana.app-folders-fold1", Namespace: "default"},
Object: resourcePerm,
})
require.Zero(t, rv)
require.NotNil(t, err)
require.Contains(t, err.Error(), errInvalidSpec.Error())
})
t.Run("should error if name and spec do not match", func(t *testing.T) {
backend := ProvideStorageBackend(dbProvider)
resourcePerm, err := utils.MetaAccessor(&v0alpha1.ResourcePermission{
ObjectMeta: metav1.ObjectMeta{
Name: "folder.grafana.app-folders-fold1",
Namespace: "default",
},
Spec: v0alpha1.ResourcePermissionSpec{
Resource: v0alpha1.ResourcePermissionspecResource{
ApiGroup: "folder.grafana.app",
Resource: "folders",
Name: "fold2",
},
Permissions: []v0alpha1.ResourcePermissionspecPermission{
{
Kind: v0alpha1.ResourcePermissionSpecPermissionKindBasicRole,
Name: "Viewer",
Verb: "Admin",
},
},
},
})
require.NoError(t, err)
gr := v0alpha1.ResourcePermissionInfo.GroupResource()
rv, err := backend.WriteEvent(context.Background(), resource.WriteEvent{
Type: resourcepb.WatchEvent_ADDED,
Key: &resourcepb.ResourceKey{Group: gr.Group, Resource: gr.Resource, Name: "folder.grafana.app-folders-fold1", Namespace: "default"},
Object: resourcePerm,
})
require.Zero(t, rv)
require.NotNil(t, err)
require.Contains(t, err.Error(), errInvalidSpec.Error())
})
t.Run("should error if resource name is empty", func(t *testing.T) {
backend := ProvideStorageBackend(dbProvider)
@@ -747,71 +682,6 @@ func TestWriteEvent_Modify(t *testing.T) {
require.Contains(t, err.Error(), "requires a valid namespace")
})
t.Run("should error if there are no permission specified in the body", func(t *testing.T) {
backend := ProvideStorageBackend(dbProvider)
resourcePerm, err := utils.MetaAccessor(&v0alpha1.ResourcePermission{
ObjectMeta: metav1.ObjectMeta{
Name: "folder.grafana.app-folders-fold1",
Namespace: "default",
},
Spec: v0alpha1.ResourcePermissionSpec{
Resource: v0alpha1.ResourcePermissionspecResource{
ApiGroup: "folder.grafana.app",
Resource: "folders",
Name: "fold1",
},
},
})
require.NoError(t, err)
gr := v0alpha1.ResourcePermissionInfo.GroupResource()
rv, err := backend.WriteEvent(context.Background(), resource.WriteEvent{
Type: resourcepb.WatchEvent_MODIFIED,
Key: &resourcepb.ResourceKey{Group: gr.Group, Resource: gr.Resource, Name: "folder.grafana.app-folders-fold1", Namespace: "default"},
Object: resourcePerm,
})
require.Zero(t, rv)
require.NotNil(t, err)
require.Contains(t, err.Error(), errInvalidSpec.Error())
})
t.Run("should error if name and spec do not match", func(t *testing.T) {
backend := ProvideStorageBackend(dbProvider)
resourcePerm, err := utils.MetaAccessor(&v0alpha1.ResourcePermission{
ObjectMeta: metav1.ObjectMeta{
Name: "folder.grafana.app-folders-fold1",
Namespace: "default",
},
Spec: v0alpha1.ResourcePermissionSpec{
Resource: v0alpha1.ResourcePermissionspecResource{
ApiGroup: "folder.grafana.app",
Resource: "folders",
Name: "fold2",
},
Permissions: []v0alpha1.ResourcePermissionspecPermission{
{
Kind: v0alpha1.ResourcePermissionSpecPermissionKindBasicRole,
Name: "Viewer",
Verb: "Admin",
},
},
},
})
require.NoError(t, err)
gr := v0alpha1.ResourcePermissionInfo.GroupResource()
rv, err := backend.WriteEvent(context.Background(), resource.WriteEvent{
Type: resourcepb.WatchEvent_MODIFIED,
Key: &resourcepb.ResourceKey{Group: gr.Group, Resource: gr.Resource, Name: "folder.grafana.app-folders-fold1", Namespace: "default"},
Object: resourcePerm,
})
require.Zero(t, rv)
require.NotNil(t, err)
require.Contains(t, err.Error(), errInvalidSpec.Error())
})
t.Run("should error if resource name is empty", func(t *testing.T) {
backend := ProvideStorageBackend(dbProvider)
@@ -0,0 +1,42 @@
package resourcepermission
import (
"context"
"fmt"
"github.com/grafana/grafana/apps/iam/pkg/apis/iam/v0alpha1"
)
func ValidateCreateAndUpdateInput(ctx context.Context, v0ResourcePerm *v0alpha1.ResourcePermission) error {
if v0ResourcePerm == nil {
return fmt.Errorf("resource permission cannot be nil")
}
if len(v0ResourcePerm.Spec.Permissions) == 0 {
return fmt.Errorf("resource permission must have at least one permission: %w", errInvalidSpec)
}
grn, err := splitResourceName(v0ResourcePerm.Name)
if err != nil {
return fmt.Errorf("invalid resource permission name: %w", err)
}
// Validate that the group/resource/name in the name matches the spec
if grn.Group != v0ResourcePerm.Spec.Resource.ApiGroup ||
grn.Resource != v0ResourcePerm.Spec.Resource.Resource ||
grn.Name != v0ResourcePerm.Spec.Resource.Name {
return fmt.Errorf("resource permission name does not match spec: %w", errInvalidSpec)
}
// Check for duplicate entities (same kind and name should appear only once)
seen := make(map[string]bool)
for _, perm := range v0ResourcePerm.Spec.Permissions {
key := fmt.Sprintf("%s:%s", perm.Kind, perm.Name)
if seen[key] {
return fmt.Errorf("duplicate entity found: kind=%s, name=%s (each entity can only appear once per resource): %w", perm.Kind, perm.Name, errInvalidSpec)
}
seen[key] = true
}
return nil
}
@@ -0,0 +1,169 @@
package resourcepermission
import (
"context"
"testing"
"github.com/stretchr/testify/assert"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
iamv0alpha1 "github.com/grafana/grafana/apps/iam/pkg/apis/iam/v0alpha1"
)
func TestValidateOnCreate(t *testing.T) {
tests := []struct {
name string
obj *iamv0alpha1.ResourcePermission
want error
}{
{
name: "missing permissions - should fail",
obj: &iamv0alpha1.ResourcePermission{
ObjectMeta: v1.ObjectMeta{
Name: "folder.grafana.app-folders-test_folder",
},
Spec: iamv0alpha1.ResourcePermissionSpec{
Resource: iamv0alpha1.ResourcePermissionspecResource{
ApiGroup: "folder.grafana.app",
Resource: "folders",
Name: "test_folder",
},
Permissions: []iamv0alpha1.ResourcePermissionspecPermission{},
},
},
want: errInvalidSpec,
},
{
name: "invalid name - should fail",
obj: &iamv0alpha1.ResourcePermission{
ObjectMeta: v1.ObjectMeta{
Name: "some-invalid-name",
},
},
want: errInvalidName,
},
{
name: "mismatched name and spec - should fail",
obj: &iamv0alpha1.ResourcePermission{
ObjectMeta: v1.ObjectMeta{
Name: "folder.grafana.app-folders-test_folder",
},
Spec: iamv0alpha1.ResourcePermissionSpec{
Resource: iamv0alpha1.ResourcePermissionspecResource{
ApiGroup: "folder.grafana.app",
Resource: "folders",
Name: "some_other_folder",
},
Permissions: []iamv0alpha1.ResourcePermissionspecPermission{
{
Kind: iamv0alpha1.ResourcePermissionSpecPermissionKindUser,
Name: "test-user",
Verb: "view",
},
},
},
},
want: errInvalidSpec,
},
{
name: "valid spec - should pass",
obj: &iamv0alpha1.ResourcePermission{
ObjectMeta: v1.ObjectMeta{
Name: "folder.grafana.app-folders-test_folder",
},
Spec: iamv0alpha1.ResourcePermissionSpec{
Resource: iamv0alpha1.ResourcePermissionspecResource{
ApiGroup: "folder.grafana.app",
Resource: "folders",
Name: "test_folder",
},
Permissions: []iamv0alpha1.ResourcePermissionspecPermission{
{
Kind: iamv0alpha1.ResourcePermissionSpecPermissionKindBasicRole,
Name: "Editor",
Verb: "edit",
},
{
Kind: iamv0alpha1.ResourcePermissionSpecPermissionKindBasicRole,
Name: "Viewer", // Different entity name - should be allowed
Verb: "view",
},
{
Kind: iamv0alpha1.ResourcePermissionSpecPermissionKindUser,
Name: "user-1",
Verb: "edit", // Different kind - should be allowed
},
},
},
},
want: nil,
},
{
name: "duplicate entities - should fail",
obj: &iamv0alpha1.ResourcePermission{
ObjectMeta: v1.ObjectMeta{
Name: "dashboard.grafana.app-dashboards-test_dashboard",
},
Spec: iamv0alpha1.ResourcePermissionSpec{
Resource: iamv0alpha1.ResourcePermissionspecResource{
ApiGroup: "dashboard.grafana.app",
Resource: "dashboards",
Name: "test_dashboard",
},
Permissions: []iamv0alpha1.ResourcePermissionspecPermission{
{
Kind: iamv0alpha1.ResourcePermissionSpecPermissionKindBasicRole,
Name: "Editor",
Verb: "edit",
},
{
Kind: iamv0alpha1.ResourcePermissionSpecPermissionKindBasicRole,
Name: "Editor",
Verb: "view", // Same entity, different verb - should fail
},
},
},
},
want: errInvalidSpec,
},
{
name: "duplicate names but different kinds - should pass",
obj: &iamv0alpha1.ResourcePermission{
ObjectMeta: v1.ObjectMeta{
Name: "dashboard.grafana.app-dashboards-test_dashboard",
},
Spec: iamv0alpha1.ResourcePermissionSpec{
Resource: iamv0alpha1.ResourcePermissionspecResource{
ApiGroup: "dashboard.grafana.app",
Resource: "dashboards",
Name: "test_dashboard",
},
Permissions: []iamv0alpha1.ResourcePermissionspecPermission{
{
Kind: iamv0alpha1.ResourcePermissionSpecPermissionKindUser,
Name: "Editor",
Verb: "edit",
},
{
Kind: iamv0alpha1.ResourcePermissionSpecPermissionKindBasicRole,
Name: "Editor",
Verb: "view",
},
},
},
},
want: nil,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
err := ValidateCreateAndUpdateInput(context.Background(), test.obj)
if test.want == nil {
assert.NoError(t, err)
} else {
assert.ErrorAs(t, test.want, &err)
}
})
}
}