From db61acf513fd4eee73172b5c33eb253877b600d7 Mon Sep 17 00:00:00 2001 From: mohammad-hamid Date: Fri, 19 Sep 2025 10:55:08 -0400 Subject: [PATCH] `grafana-iam`: prevent duplicate permissions upon ResourcePermission create (#111361) prevent duplicate permissions --- .../apis/iam/resourcepermission/sql.go | 10 + .../apis/iam/resourcepermission/sql_test.go | 179 ++++++++++++++++++ 2 files changed, 189 insertions(+) diff --git a/pkg/registry/apis/iam/resourcepermission/sql.go b/pkg/registry/apis/iam/resourcepermission/sql.go index 034c438d2ae..d635ef2db09 100644 --- a/pkg/registry/apis/iam/resourcepermission/sql.go +++ b/pkg/registry/apis/iam/resourcepermission/sql.go @@ -516,6 +516,16 @@ func validateCreateAndUpdateInput(v0ResourcePerm *v0alpha1.ResourcePermission, g 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 } diff --git a/pkg/registry/apis/iam/resourcepermission/sql_test.go b/pkg/registry/apis/iam/resourcepermission/sql_test.go index c5a1d8feef9..8ff91859d9e 100644 --- a/pkg/registry/apis/iam/resourcepermission/sql_test.go +++ b/pkg/registry/apis/iam/resourcepermission/sql_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "github.com/grafana/authlib/types" @@ -668,3 +669,181 @@ 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) + + backend := setupBackend(t) + backend.identityStore = NewFakeIdentityStore(t) + ctx := context.Background() + sql, err := backend.dbProvider(ctx) + require.NoError(t, err) + setupTestRoles(t, sql.DB) + + mapper := backend.mappers[schema.GroupResource{Group: "dashboard.grafana.app", Resource: "dashboards"}] + grn := &groupResourceName{Group: "dashboard.grafana.app", Resource: "dashboards", Name: "test-dash"} + + t.Run("should allow changing verb for same entity", func(t *testing.T) { + //Create initial permission with BasicRole Editor having "edit" verb + initialResourcePerm := &v0alpha1.ResourcePermission{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dashboard.grafana.app-dashboards-test-dash", + Namespace: "default", + }, + Spec: v0alpha1.ResourcePermissionSpec{ + Resource: v0alpha1.ResourcePermissionspecResource{ + ApiGroup: "dashboard.grafana.app", + Resource: "dashboards", + Name: "test-dash", + }, + Permissions: []v0alpha1.ResourcePermissionspecPermission{ + { + Kind: v0alpha1.ResourcePermissionSpecPermissionKindBasicRole, + Name: "Editor", + Verb: "edit", + }, + }, + }, + } + + rv, err := backend.createResourcePermission(ctx, sql, types.NamespaceInfo{Value: "default", OrgID: 1}, mapper, grn, initialResourcePerm) + require.NoError(t, err) + require.Greater(t, rv, int64(0)) + + //Update the same entity (BasicRole Editor) to have "view" verb instead + updatedResourcePerm := &v0alpha1.ResourcePermission{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dashboard.grafana.app-dashboards-test-dash", + Namespace: "default", + }, + Spec: v0alpha1.ResourcePermissionSpec{ + Resource: v0alpha1.ResourcePermissionspecResource{ + ApiGroup: "dashboard.grafana.app", + Resource: "dashboards", + Name: "test-dash", + }, + Permissions: []v0alpha1.ResourcePermissionspecPermission{ + { + Kind: v0alpha1.ResourcePermissionSpecPermissionKindBasicRole, + Name: "Editor", + Verb: "view", + }, + }, + }, + } + + rv, err = backend.updateResourcePermission(ctx, sql, types.NamespaceInfo{Value: "default", OrgID: 1}, mapper, grn, updatedResourcePerm) + require.NoError(t, err) + require.Greater(t, rv, int64(0)) + + //Verify the update worked by reading back the permission + err = sql.DB.GetSqlxSession().WithTransaction(ctx, func(tx *session.SessionTx) error { + finalResourcePerm, err := backend.getResourcePermission(ctx, sql, tx, types.NamespaceInfo{Value: "default", OrgID: 1}, grn.string()) + require.NoError(t, err) + require.Len(t, finalResourcePerm.Spec.Permissions, 1) + require.Equal(t, v0alpha1.ResourcePermissionSpecPermissionKindBasicRole, finalResourcePerm.Spec.Permissions[0].Kind) + require.Equal(t, "Editor", finalResourcePerm.Spec.Permissions[0].Name) + require.Equal(t, "view", finalResourcePerm.Spec.Permissions[0].Verb) + return nil + }) + require.NoError(t, err) + }) +}