From b7bb62747a3c74b26ad9769ae7739a34a61e8537 Mon Sep 17 00:00:00 2001 From: Ieva Date: Fri, 19 Sep 2025 14:04:05 +0100 Subject: [PATCH] Resource permissions: Fix permission cleanup during resource permission update (#111317) * fix permission cleanup during resource permission update * fix template tests --- .../apis/iam/resourcepermission/models.go | 5 ++- .../queries/permission_remove.sql | 9 ----- .../resource_permission_deletion_query.sql | 8 +++- .../apis/iam/resourcepermission/sql.go | 8 +++- .../apis/iam/resourcepermission/sql_test.go | 15 ++++++++ .../apis/iam/resourcepermission/templates.go | 37 +++---------------- .../iam/resourcepermission/templates_test.go | 29 +++++---------- ...l--permission_remove-remove_permission.sql | 9 ----- ...sion_deletion_query-basic_delete_query.sql | 4 +- ...tion_query-specific_role_cleanup_query.sql | 8 ++++ ...s--permission_remove-remove_permission.sql | 9 ----- ...sion_deletion_query-basic_delete_query.sql | 4 +- ...tion_query-specific_role_cleanup_query.sql | 8 ++++ ...e--permission_remove-remove_permission.sql | 9 ----- ...sion_deletion_query-basic_delete_query.sql | 4 +- ...tion_query-specific_role_cleanup_query.sql | 8 ++++ 16 files changed, 75 insertions(+), 99 deletions(-) delete mode 100644 pkg/registry/apis/iam/resourcepermission/queries/permission_remove.sql delete mode 100755 pkg/registry/apis/iam/resourcepermission/testdata/mysql--permission_remove-remove_permission.sql create mode 100755 pkg/registry/apis/iam/resourcepermission/testdata/mysql--resource_permission_deletion_query-specific_role_cleanup_query.sql delete mode 100755 pkg/registry/apis/iam/resourcepermission/testdata/postgres--permission_remove-remove_permission.sql create mode 100755 pkg/registry/apis/iam/resourcepermission/testdata/postgres--resource_permission_deletion_query-specific_role_cleanup_query.sql delete mode 100755 pkg/registry/apis/iam/resourcepermission/testdata/sqlite--permission_remove-remove_permission.sql create mode 100755 pkg/registry/apis/iam/resourcepermission/testdata/sqlite--resource_permission_deletion_query-specific_role_cleanup_query.sql diff --git a/pkg/registry/apis/iam/resourcepermission/models.go b/pkg/registry/apis/iam/resourcepermission/models.go index 44b03a16058..60d18dcd011 100644 --- a/pkg/registry/apis/iam/resourcepermission/models.go +++ b/pkg/registry/apis/iam/resourcepermission/models.go @@ -57,8 +57,9 @@ type ListResourcePermissionsQuery struct { } type DeleteResourcePermissionsQuery struct { - Scope string - OrgID int64 + Scope string + OrgID int64 + RoleName string } type rbacAssignmentCreate struct { diff --git a/pkg/registry/apis/iam/resourcepermission/queries/permission_remove.sql b/pkg/registry/apis/iam/resourcepermission/queries/permission_remove.sql deleted file mode 100644 index 56ffa56dab0..00000000000 --- a/pkg/registry/apis/iam/resourcepermission/queries/permission_remove.sql +++ /dev/null @@ -1,9 +0,0 @@ -DELETE FROM {{ .Ident .PermissionTable }} AS p -WHERE p.scope = {{ .Arg .Scope }} AND p.action = {{ .Arg .Action }} -AND p.role_id = ( - SELECT r.id - FROM {{ .Ident .RoleTable }} AS r - WHERE r.org_id = {{ .Arg .OrgID }} - AND r.name = {{ .Arg .RoleName }} - LIMIT 1 -) diff --git a/pkg/registry/apis/iam/resourcepermission/queries/resource_permission_deletion_query.sql b/pkg/registry/apis/iam/resourcepermission/queries/resource_permission_deletion_query.sql index 54ffc873918..a1621ca1564 100644 --- a/pkg/registry/apis/iam/resourcepermission/queries/resource_permission_deletion_query.sql +++ b/pkg/registry/apis/iam/resourcepermission/queries/resource_permission_deletion_query.sql @@ -3,6 +3,10 @@ WHERE p.scope = {{ .Arg .Query.Scope }} AND p.role_id IN ( SELECT r.id FROM {{ .Ident .RoleTable }} as r - WHERE r.name LIKE {{ .Arg .ManagedRolePattern }} - AND r.org_id = {{ .Arg .Query.OrgID }} + WHERE r.org_id = {{ .Arg .Query.OrgID }} + {{ if .RoleName }} + AND r.name = {{ .Arg .RoleName }} + {{ else }} + AND r.name LIKE {{ .Arg .ManagedRolePattern }} + {{ end }} ) diff --git a/pkg/registry/apis/iam/resourcepermission/sql.go b/pkg/registry/apis/iam/resourcepermission/sql.go index 5c5f7abe511..034c438d2ae 100644 --- a/pkg/registry/apis/iam/resourcepermission/sql.go +++ b/pkg/registry/apis/iam/resourcepermission/sql.go @@ -427,7 +427,13 @@ func (s *ResourcePermSqlBackend) updateResourcePermission(ctx context.Context, d } for _, perm := range permsToRemove { - removePermQuery, args, err := buildRemovePermissionQuery(dbHelper, perm.Scope, perm.Action, perm.RoleName, ns.OrgID) + resourceQuery := &DeleteResourcePermissionsQuery{ + Scope: perm.Scope, + OrgID: ns.OrgID, + RoleName: perm.RoleName, + } + + removePermQuery, args, err := buildDeleteResourcePermissionsQueryFromTemplate(dbHelper, resourceQuery) if err != nil { return err } diff --git a/pkg/registry/apis/iam/resourcepermission/sql_test.go b/pkg/registry/apis/iam/resourcepermission/sql_test.go index 0fd19133fa1..c5a1d8feef9 100644 --- a/pkg/registry/apis/iam/resourcepermission/sql_test.go +++ b/pkg/registry/apis/iam/resourcepermission/sql_test.go @@ -192,6 +192,20 @@ func setupTestRoles(t *testing.T, store db.DB) { require.NoError(t, err) } +func setupFineGrainedPermissions(t *testing.T, store db.DB) { + sess := store.GetSqlxSession() + + // Permissions + _, err := sess.Exec(context.Background(), + `INSERT INTO permission (role_id, action, scope, created, updated) + VALUES (?, ?, ?, ?, ?), (?, ?, ?, ?, ?)`, + // Permissions for managed:users:2:permissions + 2, "folders:read", "folders:uid:fold1", "2025-09-02", "2025-09-02", + 2, "folders:create", "folders:uid:fold1", "2025-09-02", "2025-09-02", + ) + require.NoError(t, err) +} + func TestIntegration_ResourcePermSqlBackend_newRoleIterator(t *testing.T) { testutil.SkipIntegrationTestInShortMode(t) @@ -505,6 +519,7 @@ func TestIntegration_ResourcePermSqlBackend_UpdateResourcePermission(t *testing. sql, err := backend.dbProvider(ctx) require.NoError(t, err) setupTestRoles(t, sql.DB) + setupFineGrainedPermissions(t, sql.DB) t.Run("should fail to update resource permission for a resource that doesn't have any permissions yet", func(t *testing.T) { resourcePerm := &v0alpha1.ResourcePermission{ diff --git a/pkg/registry/apis/iam/resourcepermission/templates.go b/pkg/registry/apis/iam/resourcepermission/templates.go index 039e653939c..a1608d25dce 100644 --- a/pkg/registry/apis/iam/resourcepermission/templates.go +++ b/pkg/registry/apis/iam/resourcepermission/templates.go @@ -23,7 +23,6 @@ var ( roleInsertTplt = mustTemplate("role_insert.sql") assignmentInsertTplt = mustTemplate("assignment_insert.sql") permissionInsertTplt = mustTemplate("permission_insert.sql") - permissionRemoveTplt = mustTemplate("permission_remove.sql") pageQueryTplt = mustTemplate("page_query.sql") latestUpdateTplt = mustTemplate("latest_update_query.sql") ) @@ -231,43 +230,13 @@ func buildInsertPermissionQuery(dbHelper *legacysql.LegacyDatabaseHelper, roleID return rawQuery, req.GetArgs(), nil } -type removePermissionTemplate struct { - sqltemplate.SQLTemplate - PermissionTable string - RoleTable string - Scope string - Action string - OrgID int64 - RoleName string -} - -func (t removePermissionTemplate) Validate() error { - return nil -} - -func buildRemovePermissionQuery(dbHelper *legacysql.LegacyDatabaseHelper, scope, action, roleName string, orgID int64) (string, []any, error) { - req := removePermissionTemplate{ - SQLTemplate: sqltemplate.New(dbHelper.DialectForDriver()), - PermissionTable: dbHelper.Table("permission"), - RoleTable: dbHelper.Table("role"), - Scope: scope, - Action: action, - OrgID: orgID, - RoleName: roleName, - } - rawQuery, err := sqltemplate.Execute(permissionRemoveTplt, req) - if err != nil { - return "", nil, fmt.Errorf("rendering sql template: %w", err) - } - return rawQuery, req.GetArgs(), nil -} - type deleteResourcePermissionsQueryTemplate struct { sqltemplate.SQLTemplate Query *DeleteResourcePermissionsQuery PermissionTable string RoleTable string ManagedRolePattern string + RoleName string } func (r deleteResourcePermissionsQueryTemplate) Validate() error { @@ -283,6 +252,10 @@ func buildDeleteResourcePermissionsQueryFromTemplate(sql *legacysql.LegacyDataba ManagedRolePattern: "managed:%", } + if query.RoleName != "" { + req.RoleName = query.RoleName + } + rawQuery, err := sqltemplate.Execute(resourcePermissionDeletionQueryTplt, req) if err != nil { return "", nil, fmt.Errorf("execute template %q: %w", resourcePermissionDeletionQueryTplt.Name(), err) diff --git a/pkg/registry/apis/iam/resourcepermission/templates_test.go b/pkg/registry/apis/iam/resourcepermission/templates_test.go index 6229add4140..4d78cda7e6f 100644 --- a/pkg/registry/apis/iam/resourcepermission/templates_test.go +++ b/pkg/registry/apis/iam/resourcepermission/templates_test.go @@ -43,20 +43,6 @@ func TestTemplates(t *testing.T) { return &v } - getRemovePermission := func(scope, action, roleName string) sqltemplate.SQLTemplate { - v := removePermissionTemplate{ - SQLTemplate: sqltemplate.New(nodb.DialectForDriver()), - PermissionTable: nodb.Table("permission"), - RoleTable: nodb.Table("role"), - Scope: scope, - Action: action, - OrgID: 55, - RoleName: roleName, - } - v.SQLTemplate = mocks.NewTestingSQLTemplate() - return &v - } - getInsertAssignment := func(orgID int64, roleID int64, assignment rbacAssignmentCreate) sqltemplate.SQLTemplate { v := insertAssignmentTemplate{ SQLTemplate: sqltemplate.New(nodb.DialectForDriver()), @@ -120,6 +106,7 @@ func TestTemplates(t *testing.T) { PermissionTable: nodb.Table("permission"), RoleTable: nodb.Table("role"), ManagedRolePattern: "managed:%", + RoleName: q.RoleName, } v.SQLTemplate = mocks.NewTestingSQLTemplate() return &v @@ -151,12 +138,6 @@ func TestTemplates(t *testing.T) { }), }, }, - permissionRemoveTplt: { - { - Name: "remove_permission", - Data: getRemovePermission("folders:uid:folder1", "folders:edit", "managed:users:1:permissions"), - }, - }, assignmentInsertTplt: { { Name: "insert user assignment", @@ -220,6 +201,14 @@ func TestTemplates(t *testing.T) { OrgID: 3, }), }, + { + Name: "specific_role_cleanup_query", + Data: getDeleteResourcePermissionsQuery(&DeleteResourcePermissionsQuery{ + Scope: "dash_123", + OrgID: 3, + RoleName: "managed:users:1:permissions", + }), + }, }, }, }) diff --git a/pkg/registry/apis/iam/resourcepermission/testdata/mysql--permission_remove-remove_permission.sql b/pkg/registry/apis/iam/resourcepermission/testdata/mysql--permission_remove-remove_permission.sql deleted file mode 100755 index 2ad8648da08..00000000000 --- a/pkg/registry/apis/iam/resourcepermission/testdata/mysql--permission_remove-remove_permission.sql +++ /dev/null @@ -1,9 +0,0 @@ -DELETE FROM `grafana`.`permission` AS p -WHERE p.scope = 'folders:uid:folder1' AND p.action = 'folders:edit' -AND p.role_id = ( - SELECT r.id - FROM `grafana`.`role` AS r - WHERE r.org_id = 55 - AND r.name = 'managed:users:1:permissions' - LIMIT 1 -) diff --git a/pkg/registry/apis/iam/resourcepermission/testdata/mysql--resource_permission_deletion_query-basic_delete_query.sql b/pkg/registry/apis/iam/resourcepermission/testdata/mysql--resource_permission_deletion_query-basic_delete_query.sql index 52cac6e9423..e47edc9743f 100755 --- a/pkg/registry/apis/iam/resourcepermission/testdata/mysql--resource_permission_deletion_query-basic_delete_query.sql +++ b/pkg/registry/apis/iam/resourcepermission/testdata/mysql--resource_permission_deletion_query-basic_delete_query.sql @@ -3,6 +3,6 @@ WHERE p.scope = 'dash_123' AND p.role_id IN ( SELECT r.id FROM `grafana`.`role` as r - WHERE r.name LIKE 'managed:%' - AND r.org_id = 3 + WHERE r.org_id = 3 + AND r.name LIKE 'managed:%' ) diff --git a/pkg/registry/apis/iam/resourcepermission/testdata/mysql--resource_permission_deletion_query-specific_role_cleanup_query.sql b/pkg/registry/apis/iam/resourcepermission/testdata/mysql--resource_permission_deletion_query-specific_role_cleanup_query.sql new file mode 100755 index 00000000000..5ccaf6aab61 --- /dev/null +++ b/pkg/registry/apis/iam/resourcepermission/testdata/mysql--resource_permission_deletion_query-specific_role_cleanup_query.sql @@ -0,0 +1,8 @@ +DELETE FROM `grafana`.`permission` as p +WHERE p.scope = 'dash_123' + AND p.role_id IN ( + SELECT r.id + FROM `grafana`.`role` as r + WHERE r.org_id = 3 + AND r.name = 'managed:users:1:permissions' + ) diff --git a/pkg/registry/apis/iam/resourcepermission/testdata/postgres--permission_remove-remove_permission.sql b/pkg/registry/apis/iam/resourcepermission/testdata/postgres--permission_remove-remove_permission.sql deleted file mode 100755 index 1a5e5326586..00000000000 --- a/pkg/registry/apis/iam/resourcepermission/testdata/postgres--permission_remove-remove_permission.sql +++ /dev/null @@ -1,9 +0,0 @@ -DELETE FROM "grafana"."permission" AS p -WHERE p.scope = 'folders:uid:folder1' AND p.action = 'folders:edit' -AND p.role_id = ( - SELECT r.id - FROM "grafana"."role" AS r - WHERE r.org_id = 55 - AND r.name = 'managed:users:1:permissions' - LIMIT 1 -) diff --git a/pkg/registry/apis/iam/resourcepermission/testdata/postgres--resource_permission_deletion_query-basic_delete_query.sql b/pkg/registry/apis/iam/resourcepermission/testdata/postgres--resource_permission_deletion_query-basic_delete_query.sql index 5511d8c961b..91023e3b9cf 100755 --- a/pkg/registry/apis/iam/resourcepermission/testdata/postgres--resource_permission_deletion_query-basic_delete_query.sql +++ b/pkg/registry/apis/iam/resourcepermission/testdata/postgres--resource_permission_deletion_query-basic_delete_query.sql @@ -3,6 +3,6 @@ WHERE p.scope = 'dash_123' AND p.role_id IN ( SELECT r.id FROM "grafana"."role" as r - WHERE r.name LIKE 'managed:%' - AND r.org_id = 3 + WHERE r.org_id = 3 + AND r.name LIKE 'managed:%' ) diff --git a/pkg/registry/apis/iam/resourcepermission/testdata/postgres--resource_permission_deletion_query-specific_role_cleanup_query.sql b/pkg/registry/apis/iam/resourcepermission/testdata/postgres--resource_permission_deletion_query-specific_role_cleanup_query.sql new file mode 100755 index 00000000000..a4a9f996572 --- /dev/null +++ b/pkg/registry/apis/iam/resourcepermission/testdata/postgres--resource_permission_deletion_query-specific_role_cleanup_query.sql @@ -0,0 +1,8 @@ +DELETE FROM "grafana"."permission" as p +WHERE p.scope = 'dash_123' + AND p.role_id IN ( + SELECT r.id + FROM "grafana"."role" as r + WHERE r.org_id = 3 + AND r.name = 'managed:users:1:permissions' + ) diff --git a/pkg/registry/apis/iam/resourcepermission/testdata/sqlite--permission_remove-remove_permission.sql b/pkg/registry/apis/iam/resourcepermission/testdata/sqlite--permission_remove-remove_permission.sql deleted file mode 100755 index 1a5e5326586..00000000000 --- a/pkg/registry/apis/iam/resourcepermission/testdata/sqlite--permission_remove-remove_permission.sql +++ /dev/null @@ -1,9 +0,0 @@ -DELETE FROM "grafana"."permission" AS p -WHERE p.scope = 'folders:uid:folder1' AND p.action = 'folders:edit' -AND p.role_id = ( - SELECT r.id - FROM "grafana"."role" AS r - WHERE r.org_id = 55 - AND r.name = 'managed:users:1:permissions' - LIMIT 1 -) diff --git a/pkg/registry/apis/iam/resourcepermission/testdata/sqlite--resource_permission_deletion_query-basic_delete_query.sql b/pkg/registry/apis/iam/resourcepermission/testdata/sqlite--resource_permission_deletion_query-basic_delete_query.sql index 5511d8c961b..91023e3b9cf 100755 --- a/pkg/registry/apis/iam/resourcepermission/testdata/sqlite--resource_permission_deletion_query-basic_delete_query.sql +++ b/pkg/registry/apis/iam/resourcepermission/testdata/sqlite--resource_permission_deletion_query-basic_delete_query.sql @@ -3,6 +3,6 @@ WHERE p.scope = 'dash_123' AND p.role_id IN ( SELECT r.id FROM "grafana"."role" as r - WHERE r.name LIKE 'managed:%' - AND r.org_id = 3 + WHERE r.org_id = 3 + AND r.name LIKE 'managed:%' ) diff --git a/pkg/registry/apis/iam/resourcepermission/testdata/sqlite--resource_permission_deletion_query-specific_role_cleanup_query.sql b/pkg/registry/apis/iam/resourcepermission/testdata/sqlite--resource_permission_deletion_query-specific_role_cleanup_query.sql new file mode 100755 index 00000000000..a4a9f996572 --- /dev/null +++ b/pkg/registry/apis/iam/resourcepermission/testdata/sqlite--resource_permission_deletion_query-specific_role_cleanup_query.sql @@ -0,0 +1,8 @@ +DELETE FROM "grafana"."permission" as p +WHERE p.scope = 'dash_123' + AND p.role_id IN ( + SELECT r.id + FROM "grafana"."role" as r + WHERE r.org_id = 3 + AND r.name = 'managed:users:1:permissions' + )