RBAC: change migration logic (#50187)

* 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>
2024-11-24 09:50:29 -06:00 · 2022-06-06 12:51:07 +01:00 · 2022-06-06 12:51:07 +01:00 · 7c800421d3
commit 7c800421d3
parent 333e261074
3 changed files with 334 additions and 11 deletions
--- 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)
 		}
 	}

--- 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)
--- a/pkg/services/sqlstore/migrations/accesscontrol/test/action_migrator_test.go
+++ 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))
+			}
+		})
+	}
+}