diff --git a/pkg/services/sqlstore/migrations/accesscontrol/managed_permission_migrator.go b/pkg/services/sqlstore/migrations/accesscontrol/managed_permission_migrator.go index 425247d0314..756502be02d 100644 --- a/pkg/services/sqlstore/migrations/accesscontrol/managed_permission_migrator.go +++ b/pkg/services/sqlstore/migrations/accesscontrol/managed_permission_migrator.go @@ -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 @@ -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 } @@ -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} diff --git a/pkg/services/sqlstore/migrations/accesscontrol/test/managed_permission_migrator_test.go b/pkg/services/sqlstore/migrations/accesscontrol/test/managed_permission_migrator_test.go index 9d8946e2ed6..e8dd6d728c6 100644 --- a/pkg/services/sqlstore/migrations/accesscontrol/test/managed_permission_migrator_test.go +++ b/pkg/services/sqlstore/migrations/accesscontrol/test/managed_permission_migrator_test.go @@ -1,6 +1,7 @@ package test import ( + "fmt" "strconv" "strings" "testing" @@ -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{}, @@ -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: { @@ -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 + }, }, }, } +} - for _, tc := range testCases { +func TestManagedPermissionsMigration(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 = ?; -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) @@ -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 { @@ -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) } } }