Skip to content

Commit

Permalink
RBAC: Inheritance permission migration should handle empty managed ro…
Browse files Browse the repository at this point in the history 
…les (grafana#50611)

* Make inheritance permission migration more robust

* Better fix

* Add more tests to the migration

* Add removed test case

* Add test case for empty role in empty org

* Handling the role.ID 0 case with a log

Co-authored-by: Jguer <[email protected]>

Co-authored-by: Jguer <[email protected]>
  • Loading branch information
@gamab @Jguer
gamab and Jguer authored Jun 10, 2022
1 parent 913ac82 commit 307a0d4
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 28 deletions.
18 changes: 8 additions & 10 deletions pkg/services/sqlstore/migrations/accesscontrol/managed_permission_migrator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,10 @@ func (sp *managedPermissionMigrator) Exec(sess *xorm.Session, mg *migrator.Migra
return errFindPermissions
}

roleMap := make(map[int64]map[string]int64) // map[org_id][role_name] = role_id
permissionMap := make(map[int64]map[string]map[Permission]bool) // map[org_id][role_name][Permission] = toInsert

// for each managed permission make a map of which permissions need to be added to inheritors
for _, p := range managedPermissions {
if _, ok := roleMap[p.OrgID]; !ok {
roleMap[p.OrgID] = map[string]int64{p.RoleName: p.RoleID}
} else {
roleMap[p.OrgID][p.RoleName] = p.RoleID
}

// this ensures we can use p as a key in the map between different permissions
// ensuring we're only comparing on the action and scope
roleName := p.RoleName
Expand Down Expand Up @@ -106,7 +99,8 @@ func (sp *managedPermissionMigrator) Exec(sess *xorm.Session, mg *migrator.Migra
for orgID, orgMap := range permissionMap {
for managedRole, permissions := range orgMap {
// ensure managed role exists, create and add to map if it doesn't
ok, err := sess.Get(&accesscontrol.Role{Name: managedRole, OrgID: orgID})
foundRole := &accesscontrol.Role{Name: managedRole, OrgID: orgID}
ok, err := sess.Get(foundRole)
if err != nil {
return err
}
Expand Down Expand Up @@ -135,11 +129,15 @@ func (sp *managedPermissionMigrator) Exec(sess *xorm.Session, mg *migrator.Migra
return err
}

roleMap[orgID][managedRole] = createdRole.ID
foundRole = &createdRole
}

// assign permissions if they don't exist to the role
roleID := roleMap[orgID][managedRole]
roleID := foundRole.ID
if roleID == 0 {
logger.Warn("Unable to create managed permission, got role ID 0", "orgID", orgID, "managedRole", managedRole)
continue
}
for p, toInsert := range permissions {
if toInsert {
perm := accesscontrol.Permission{RoleID: roleID, Action: p.Action, Scope: p.Scope, Created: now, Updated: now}
Expand Down
127 changes: 109 additions & 18 deletions pkg/services/sqlstore/migrations/accesscontrol/test/managed_permission_migrator_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package test

import (
"fmt"
"strconv"
"strings"
"testing"
Expand All @@ -14,19 +15,17 @@ import (
"xorm.io/xorm"
)

func TestManagedPermissionsMigration(t *testing.T) {
// Run initial migration to have a working DB
x := setupTestDB(t)
type inheritanceTestCase struct {
desc string
putRolePerms map[int64]map[string][]rawPermission
wantRolePerms map[int64]map[string][]rawPermission
}

func inheritanceTestCases() []inheritanceTestCase {
team1Scope := accesscontrol.Scope("teams", "id", "1")
team2Scope := accesscontrol.Scope("teams", "id", "2")

type teamMigrationTestCase struct {
desc string
putRolePerms map[int64]map[string][]rawPermission
wantRolePerms map[int64]map[string][]rawPermission
}
testCases := []teamMigrationTestCase{
return []inheritanceTestCase{
{
desc: "empty perms",
putRolePerms: map[int64]map[string][]rawPermission{},
Expand Down Expand Up @@ -74,6 +73,16 @@ func TestManagedPermissionsMigration(t *testing.T) {
{Action: "teams:write", Scope: team1Scope},
},
},
3: {
"managed:builtins:editor:permissions": {
{Action: "teams.permissions:read", Scope: team1Scope},
{Action: "teams.permissions:write", Scope: team2Scope},
},
"managed:builtins:admin:permissions": {}, //Role existed with no permission
},
4: {
"managed:builtins:admin:permissions": {}, //Role existed with no permission
},
},
wantRolePerms: map[int64]map[string][]rawPermission{
1: {
Expand Down Expand Up @@ -111,16 +120,37 @@ func TestManagedPermissionsMigration(t *testing.T) {
{Action: "teams:write", Scope: team1Scope},
},
},
3: {
"managed:builtins:editor:permissions": {
{Action: "teams.permissions:read", Scope: team1Scope},
{Action: "teams.permissions:write", Scope: team2Scope},
},
"managed:builtins:admin:permissions": {
{Action: "teams.permissions:read", Scope: team1Scope},
{Action: "teams.permissions:write", Scope: team2Scope},
},
},
4: {
"managed:builtins:admin:permissions": {}, //Role existed with no permission
},
},
},
}
}

func TestManagedPermissionsMigration(t *testing.T) {
// Run initial migration to have a working DB
x := setupTestDB(t)

for _, tc := range testCases {
for _, tc := range inheritanceTestCases() {
t.Run(tc.desc, func(t *testing.T) {
// Remove migration
_, errDeleteMig := x.Exec(`DELETE FROM migration_log WHERE migration_id = ?;
DELETE FROM permission; DELETE FROM role`, acmig.ManagedPermissionsMigrationID)
_, errDeleteMig := x.Exec(`DELETE FROM migration_log WHERE migration_id = ?`, acmig.ManagedPermissionsMigrationID)
require.NoError(t, errDeleteMig)
_, errDeletePerm := x.Exec(`DELETE FROM permission`)
require.NoError(t, errDeletePerm)
_, errDeleteRole := x.Exec(`DELETE FROM role`)
require.NoError(t, errDeleteRole)

// put permissions
putTestPermissions(t, x, tc.putRolePerms)
Expand Down Expand Up @@ -163,6 +193,65 @@ DELETE FROM permission; DELETE FROM role`, acmig.ManagedPermissionsMigrationID)
}
}

func TestManagedPermissionsMigrationRunTwice(t *testing.T) {
// Run initial migration to have a working DB
x := setupTestDB(t)

for _, tc := range inheritanceTestCases() {
t.Run(tc.desc, func(t *testing.T) {
// Remove migration
_, errDeleteMig := x.Exec(`DELETE FROM migration_log WHERE migration_id LIKE ?`, acmig.ManagedPermissionsMigrationID+"%")
require.NoError(t, errDeleteMig)
_, errDeletePerm := x.Exec(`DELETE FROM permission`)
require.NoError(t, errDeletePerm)
_, errDeleteRole := x.Exec(`DELETE FROM role`)
require.NoError(t, errDeleteRole)

for i := 0; i < 2; i++ {
if i == 0 {
// put permissions
putTestPermissions(t, x, tc.putRolePerms)
}

// Run accesscontrol migration (permissions insertion should not have conflicted)
acmigrator := migrator.NewMigrator(x, &setting.Cfg{Logger: log.New("acmigration.test")})
acmig.AddManagedPermissionsMigration(acmigrator, acmig.ManagedPermissionsMigrationID+fmt.Sprint(i))

errRunningMig := acmigrator.Start(false, 0)
require.NoError(t, errRunningMig)

// verify got == want
for orgID, roles := range tc.wantRolePerms {
for roleName := range roles {
// Check managed roles exist
role := accesscontrol.Role{}
hasRole, errManagedRoleSearch := x.Table("role").Where("org_id = ? AND name = ?", orgID, roleName).Get(&role)

require.NoError(t, errManagedRoleSearch)
require.True(t, hasRole, "expected role to exist", "orgID", orgID, "role", roleName)

// Check permissions associated with each role
perms := []accesscontrol.Permission{}
count, errManagedPermsSearch := x.Table("permission").Where("role_id = ?", role.ID).FindAndCount(&perms)

require.NoError(t, errManagedPermsSearch)
require.Equal(t, int64(len(tc.wantRolePerms[orgID][roleName])), count, "expected role to be tied to permissions", "orgID", orgID, "role", roleName)

gotRawPerms := convertToRawPermissions(perms)
require.ElementsMatch(t, gotRawPerms, tc.wantRolePerms[orgID][roleName], "expected role to have permissions", "orgID", orgID, "role", roleName)

// Check assignment of the roles
br := accesscontrol.BuiltinRole{}
has, errAssignmentSearch := x.Table("builtin_role").Where("role_id = ? AND role = ? AND org_id = ?", role.ID, acmig.ParseRoleFromName(roleName), orgID).Get(&br)
require.NoError(t, errAssignmentSearch)
require.True(t, has, "expected assignment of role to builtin role", "orgID", orgID, "role", roleName)
}
}
}
})
}
}

func putTestPermissions(t *testing.T, x *xorm.Engine, rolePerms map[int64]map[string][]rawPermission) {
for orgID, roles := range rolePerms {
for roleName, perms := range roles {
Expand Down Expand Up @@ -190,13 +279,15 @@ func putTestPermissions(t *testing.T, x *xorm.Engine, rolePerms map[int64]map[st
require.NoError(t, err)
require.Equal(t, int64(1), brCount)

permissions := []accesscontrol.Permission{}
for _, p := range perms {
permissions = append(permissions, p.toPermission(role.ID, now))
if len(perms) > 0 {
permissions := []accesscontrol.Permission{}
for _, p := range perms {
permissions = append(permissions, p.toPermission(role.ID, now))
}
permissionsCount, err := x.Insert(permissions)
require.NoError(t, err)
require.Equal(t, int64(len(perms)), permissionsCount)
}
permissionsCount, err := x.Insert(permissions)
require.NoError(t, err)
require.Equal(t, int64(len(perms)), permissionsCount)
}
}
}

0 comments on commit 307a0d4

Please sign in to comment.