From 371d9bc3b87d1af5677f858b5cc452b2aa1832f1 Mon Sep 17 00:00:00 2001 From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com> Date: Mon, 6 Jun 2022 08:02:58 -0400 Subject: [PATCH] RBAC: change migration logic (#50187) (#50245) * change migration logic * linting * linting * fix an issue with the migration logic * make tests runnable against other DBs Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> (cherry picked from commit 7c800421d3e94fa02c1c2d398d48fde52f112039) Co-authored-by: Ieva --- .../accesscontrol/action_migrator.go | 68 ++++- .../migrations/accesscontrol/test/ac_test.go | 35 ++- .../test/action_migrator_test.go | 242 ++++++++++++++++++ 3 files changed, 334 insertions(+), 11 deletions(-) create mode 100644 pkg/services/sqlstore/migrations/accesscontrol/test/action_migrator_test.go diff --git a/pkg/services/sqlstore/migrations/accesscontrol/action_migrator.go b/pkg/services/sqlstore/migrations/accesscontrol/action_migrator.go index 704f268812f..7e2266163a4 100644 --- a/pkg/services/sqlstore/migrations/accesscontrol/action_migrator.go +++ b/pkg/services/sqlstore/migrations/accesscontrol/action_migrator.go @@ -2,6 +2,7 @@ package accesscontrol import ( "fmt" + "strings" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/sqlstore/migrator" @@ -9,8 +10,10 @@ import ( "xorm.io/xorm" ) +const ActionMigrationID = "RBAC action name migrator" + func AddActionNameMigrator(mg *migrator.Migrator) { - mg.AddMigration("RBAC action name migrator", &actionNameMigrator{}) + mg.AddMigration(ActionMigrationID, &actionNameMigrator{}) } type actionNameMigrator struct { @@ -41,6 +44,7 @@ func (m *actionNameMigrator) migrateActionNames() error { "users.password:update": accesscontrol.ActionUsersPasswordUpdate, "users.permissions:update": accesscontrol.ActionUsersPermissionsUpdate, "users.quotas:update": accesscontrol.ActionUsersQuotasUpdate, + "roles:list": "roles:read", "teams.roles:list": "teams.roles:read", "users.roles:list": "users.roles:read", "users.authtoken:list": accesscontrol.ActionUsersAuthTokenList, @@ -49,18 +53,62 @@ func (m *actionNameMigrator) migrateActionNames() error { "alert.instances:update": accesscontrol.ActionAlertingInstanceUpdate, "alert.rules:update": accesscontrol.ActionAlertingRuleUpdate, } + + var oldActionNames, newActionNames []interface{} for oldName, newName := range actionNameMapping { - _, err := m.sess.Table(&accesscontrol.Permission{}).Where("action = ?", oldName).Update(&accesscontrol.Permission{Action: newName}) - if err != nil { - return fmt.Errorf("failed to update permission table for action %s: %w", oldName, err) - } + oldActionNames = append(oldActionNames, oldName) + newActionNames = append(newActionNames, newName) } - actionsToDelete := []string{"users.teams:read", "roles:list"} - for _, action := range actionsToDelete { - _, err := m.sess.Table(&accesscontrol.Permission{}).Where("action = ?", action).Delete(accesscontrol.Permission{}) - if err != nil { - return fmt.Errorf("failed to update permission table for action %s: %w", action, err) + actionNameQuery := strings.Builder{} + actionNameQuery.WriteRune(' ') + actionNameQuery.WriteString("action IN ") + actionNameQuery.WriteString("(?") + actionNameQuery.WriteString(strings.Repeat(",?", len(actionNameMapping)-1)) + actionNameQuery.WriteRune(')') + + oldActionNamePermissions := make([]*accesscontrol.Permission, 0) + err := m.sess.Where(actionNameQuery.String(), oldActionNames...).Find(&oldActionNamePermissions) + if err != nil { + return fmt.Errorf("failed to list permissions with legacy action names: %w", err) + } + + newActionNamePermissions := make([]*accesscontrol.Permission, 0) + err = m.sess.Where(actionNameQuery.String(), newActionNames...).Find(&newActionNamePermissions) + if err != nil { + return fmt.Errorf("failed to list permissions with new action names: %w", err) + } + + permissionsToCreate := make([]*accesscontrol.Permission, 0) + for _, oldNamePermission := range oldActionNamePermissions { + newPermission := oldNamePermission + newPermission.Action = actionNameMapping[oldNamePermission.Action] + + // if there already is a permission in the database with the new action name and the same role ID and scope as the old permissions, + // we can just drop the old permission (otherwise the permission table uniqueness constraint won't be satisfied) + newNamePermissionExists := false + // note - there should not be many permissions with the new action names, so this should not be an expensive iteration + for _, existingPermission := range newActionNamePermissions { + if existingPermission.Action == newPermission.Action && + existingPermission.RoleID == newPermission.RoleID && + existingPermission.Scope == newPermission.Scope { + newNamePermissionExists = true + continue + } + } + if newNamePermissionExists { + continue + } + + permissionsToCreate = append(permissionsToCreate, oldNamePermission) + } + + if _, err := m.sess.Where(actionNameQuery.String(), oldActionNames...).Delete(accesscontrol.Permission{}); err != nil { + return fmt.Errorf("failed to delete permissions with legacy action names: %w", err) + } + if len(permissionsToCreate) != 0 { + if _, err := m.sess.InsertMulti(permissionsToCreate); err != nil { + return fmt.Errorf("failed to create permissions with the new action names: %w", err) } } diff --git a/pkg/services/sqlstore/migrations/accesscontrol/test/ac_test.go b/pkg/services/sqlstore/migrations/accesscontrol/test/ac_test.go index 30430b559ee..389a63216a1 100644 --- a/pkg/services/sqlstore/migrations/accesscontrol/test/ac_test.go +++ b/pkg/services/sqlstore/migrations/accesscontrol/test/ac_test.go @@ -2,6 +2,7 @@ package test import ( "fmt" + "os" "testing" "time" @@ -94,6 +95,37 @@ func convertToRawPermissions(permissions []accesscontrol.Permission) []rawPermis return raw } +func getDBType() string { + dbType := migrator.SQLite + + // environment variable present for test db? + if db, present := os.LookupEnv("GRAFANA_TEST_DB"); present { + dbType = db + } + return dbType +} + +func getTestDB(t *testing.T, dbType string) sqlutil.TestDB { + switch dbType { + case "mysql": + return sqlutil.MySQLTestDB() + case "postgres": + return sqlutil.PostgresTestDB() + default: + f, err := os.CreateTemp(".", "grafana-test-db-") + require.NoError(t, err) + t.Cleanup(func() { + err := os.Remove(f.Name()) + require.NoError(t, err) + }) + + return sqlutil.TestDB{ + DriverName: "sqlite3", + ConnStr: f.Name(), + } + } +} + func TestMigrations(t *testing.T) { // Run initial migration to have a working DB x := setupTestDB(t) @@ -212,7 +244,8 @@ func TestMigrations(t *testing.T) { func setupTestDB(t *testing.T) *xorm.Engine { t.Helper() - testDB := sqlutil.SQLite3TestDB() + dbType := getDBType() + testDB := getTestDB(t, dbType) x, err := xorm.NewEngine(testDB.DriverName, testDB.ConnStr) require.NoError(t, err) diff --git a/pkg/services/sqlstore/migrations/accesscontrol/test/action_migrator_test.go b/pkg/services/sqlstore/migrations/accesscontrol/test/action_migrator_test.go new file mode 100644 index 00000000000..c3d78009a8b --- /dev/null +++ b/pkg/services/sqlstore/migrations/accesscontrol/test/action_migrator_test.go @@ -0,0 +1,242 @@ +package test + +import ( + "fmt" + "testing" + + "github.com/grafana/grafana/pkg/services/dashboards" + "github.com/stretchr/testify/assert" + + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/accesscontrol" + acmig "github.com/grafana/grafana/pkg/services/sqlstore/migrations/accesscontrol" + "github.com/grafana/grafana/pkg/services/sqlstore/migrator" + "github.com/grafana/grafana/pkg/setting" + "github.com/stretchr/testify/require" +) + +func TestActionMigration(t *testing.T) { + // Run initial migration to have a working DB + x := setupTestDB(t) + + type migrationTestCase struct { + desc string + permissionSeed []*accesscontrol.Permission + wantPermissions []*accesscontrol.Permission + } + testCases := []migrationTestCase{ + { + desc: "empty perms", + permissionSeed: []*accesscontrol.Permission{}, + wantPermissions: []*accesscontrol.Permission{}, + }, + { + desc: "no permissions with legacy names", + permissionSeed: []*accesscontrol.Permission{ + { + RoleID: 1, + Action: dashboards.ActionDashboardsRead, + Scope: dashboards.ScopeDashboardsAll, + Created: now, + Updated: now, + }, + { + RoleID: 1, + Action: accesscontrol.ActionTeamsCreate, + Scope: accesscontrol.ScopeTeamsAll, + Created: now, + Updated: now, + }, + }, + wantPermissions: []*accesscontrol.Permission{ + { + RoleID: 1, + Action: dashboards.ActionDashboardsRead, + Scope: dashboards.ScopeDashboardsAll, + }, + { + RoleID: 1, + Action: accesscontrol.ActionTeamsCreate, + Scope: accesscontrol.ScopeTeamsAll, + }, + }, + }, + { + desc: "some permissions with legacy names", + permissionSeed: []*accesscontrol.Permission{ + { + RoleID: 1, + Action: "org.users.role:update", + Scope: accesscontrol.ScopeUsersAll, + Created: now, + Updated: now, + }, + { + RoleID: 2, + Action: accesscontrol.ActionTeamsCreate, + Scope: accesscontrol.ScopeTeamsAll, + Created: now, + Updated: now, + }, + { + RoleID: 1, + Action: "teams.roles:list", + Scope: accesscontrol.ScopeTeamsAll, + Created: now, + Updated: now, + }, + }, + wantPermissions: []*accesscontrol.Permission{ + { + RoleID: 1, + Action: accesscontrol.ActionOrgUsersWrite, + Scope: accesscontrol.ScopeUsersAll, + }, + { + RoleID: 2, + Action: accesscontrol.ActionTeamsCreate, + Scope: accesscontrol.ScopeTeamsAll, + }, + { + RoleID: 1, + Action: "teams.roles:read", + Scope: accesscontrol.ScopeTeamsAll, + }, + }, + }, + { + desc: "permission with legacy name and permission with new name with the same role ID and scope", + permissionSeed: []*accesscontrol.Permission{ + { + RoleID: 1, + Action: "org.users.role:update", + Scope: accesscontrol.ScopeUsersAll, + Created: now, + Updated: now, + }, + { + RoleID: 1, + Action: accesscontrol.ActionOrgUsersWrite, + Scope: accesscontrol.ScopeUsersAll, + Created: now, + Updated: now, + }, + }, + wantPermissions: []*accesscontrol.Permission{ + { + RoleID: 1, + Action: accesscontrol.ActionOrgUsersWrite, + Scope: accesscontrol.ScopeUsersAll, + }, + }, + }, + { + desc: "permission with legacy name and permission with new name with different role ID and scope", + permissionSeed: []*accesscontrol.Permission{ + { + RoleID: 1, + Action: "org.users.role:update", + Scope: accesscontrol.ScopeUsersAll, + Created: now, + Updated: now, + }, + { + RoleID: 2, + Action: accesscontrol.ActionOrgUsersWrite, + Scope: accesscontrol.ScopeUsersAll, + Created: now, + Updated: now, + }, + }, + wantPermissions: []*accesscontrol.Permission{ + { + RoleID: 1, + Action: accesscontrol.ActionOrgUsersWrite, + Scope: accesscontrol.ScopeUsersAll, + }, + { + RoleID: 2, + Action: accesscontrol.ActionOrgUsersWrite, + Scope: accesscontrol.ScopeUsersAll, + }, + }, + }, + { + desc: "permission with legacy name and a different permission with new name with the same role ID and scope", + permissionSeed: []*accesscontrol.Permission{ + { + RoleID: 1, + Action: "org.users.role:update", + Scope: accesscontrol.ScopeUsersAll, + Created: now, + Updated: now, + }, + { + RoleID: 1, + Action: accesscontrol.ActionUsersPasswordUpdate, + Scope: accesscontrol.ScopeUsersAll, + Created: now, + Updated: now, + }, + }, + wantPermissions: []*accesscontrol.Permission{ + { + RoleID: 1, + Action: accesscontrol.ActionOrgUsersWrite, + Scope: accesscontrol.ScopeUsersAll, + }, + { + RoleID: 1, + Action: accesscontrol.ActionUsersPasswordUpdate, + Scope: accesscontrol.ScopeUsersAll, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + // Remove migration and permissions + _, errDeleteMig := x.Exec(`DELETE FROM migration_log WHERE migration_id = ?`, acmig.ActionMigrationID) + require.NoError(t, errDeleteMig) + _, errDeletePerms := x.Exec(`DELETE FROM permission`) + require.NoError(t, errDeletePerms) + + // seed DB with permissions + if len(tc.permissionSeed) != 0 { + permissionsCount, err := x.Insert(tc.permissionSeed) + require.NoError(t, err) + require.Equal(t, int64(len(tc.permissionSeed)), permissionsCount) + } + + // Run RBAC action name migration + acmigrator := migrator.NewMigrator(x, &setting.Cfg{Logger: log.New("acmigration.test")}) + acmig.AddActionNameMigrator(acmigrator) + + errRunningMig := acmigrator.Start(false, 0) + require.NoError(t, errRunningMig) + + // Check permissions + resultingPermissions := []accesscontrol.Permission{} + err := x.Table("permission").Find(&resultingPermissions) + require.NoError(t, err) + + // verify got == want + assert.Equal(t, len(tc.wantPermissions), len(resultingPermissions)) + for _, wantPermission := range tc.wantPermissions { + foundMatch := false + for _, resultingPermission := range resultingPermissions { + if wantPermission.Action == resultingPermission.Action && + wantPermission.Scope == resultingPermission.Scope && + wantPermission.RoleID == resultingPermission.RoleID { + assert.NotEmpty(t, resultingPermission.Updated) + assert.NotEmpty(t, resultingPermission.Created) + foundMatch = true + continue + } + } + assert.True(t, foundMatch, fmt.Sprintf("there should be a permission with action %s in the DB after the migration", wantPermission.Action)) + } + }) + } +}