From 6c8895e349d3726826c4e0a3849bce7a030b89cb Mon Sep 17 00:00:00 2001 From: Eric Leijonmarck Date: Fri, 15 Mar 2024 15:00:25 +0100 Subject: [PATCH] Service accounts: Same Org fix migration to account for duplicate entries (#84349) * bug: fix migration to account for duplicate entries * refactoring to create test folder for user migrations * fix migration log * added the migration * additional tests * added extSrv tests --- ...ice_account_multiple_org_login_migrator.go | 68 +++++ .../user/test/service_account_test.go | 247 ++++++++++++++++++ .../migrations/user/test/user_test.go | 55 ++++ pkg/services/sqlstore/migrations/user_mig.go | 6 +- 4 files changed, 372 insertions(+), 4 deletions(-) create mode 100644 pkg/services/sqlstore/migrations/user/service_account_multiple_org_login_migrator.go create mode 100644 pkg/services/sqlstore/migrations/user/test/service_account_test.go create mode 100644 pkg/services/sqlstore/migrations/user/test/user_test.go diff --git a/pkg/services/sqlstore/migrations/user/service_account_multiple_org_login_migrator.go b/pkg/services/sqlstore/migrations/user/service_account_multiple_org_login_migrator.go new file mode 100644 index 00000000000..064985c0f73 --- /dev/null +++ b/pkg/services/sqlstore/migrations/user/service_account_multiple_org_login_migrator.go @@ -0,0 +1,68 @@ +package user + +import ( + "fmt" + + "github.com/grafana/grafana/pkg/services/sqlstore/migrator" + "xorm.io/xorm" +) + +const ( + AllowSameLoginCrossOrgs = "update login field with orgid to allow for multiple service accounts with same name across orgs" +) + +// Service accounts login were not unique per org. this migration is part of making it unique per org +// to be able to create service accounts that are unique per org +func AddServiceAccountsAllowSameLoginCrossOrgs(mg *migrator.Migrator) { + mg.AddMigration(AllowSameLoginCrossOrgs, &ServiceAccountsSameLoginCrossOrgs{}) +} + +var _ migrator.CodeMigration = new(ServiceAccountsSameLoginCrossOrgs) + +type ServiceAccountsSameLoginCrossOrgs struct { + sess *xorm.Session + dialect migrator.Dialect + migrator.MigrationBase +} + +func (p *ServiceAccountsSameLoginCrossOrgs) SQL(dialect migrator.Dialect) string { + return "code migration" +} + +func (p *ServiceAccountsSameLoginCrossOrgs) Exec(sess *xorm.Session, mg *migrator.Migrator) error { + p.sess = sess + p.dialect = mg.Dialect + var err error + switch p.dialect.DriverName() { + case migrator.Postgres: + _, err = p.sess.Exec(`UPDATE "user" + SET login = 'sa-' || org_id::text || '-' || + CASE + WHEN login LIKE 'sa-%' THEN SUBSTRING(login FROM 4) + ELSE login + END + WHERE login IS NOT NULL AND is_service_account = true;`, + ) + case migrator.MySQL: + _, err = p.sess.Exec(`UPDATE user + SET login = CONCAT('sa-', CAST(org_id AS CHAR), '-', + CASE + WHEN login LIKE 'sa-%' THEN SUBSTRING(login, 4) + ELSE login + END) + WHERE login IS NOT NULL AND is_service_account = 1;`, + ) + case migrator.SQLite: + _, err = p.sess.Exec(`Update ` + p.dialect.Quote("user") + ` + SET login = 'sa-' || CAST(org_id AS TEXT) || '-' || + CASE + WHEN SUBSTR(login, 1, 3) = 'sa-' THEN SUBSTR(login, 4) + ELSE login + END + WHERE login IS NOT NULL AND is_service_account = 1;`, + ) + default: + return fmt.Errorf("dialect not supported: %s", p.dialect) + } + return err +} diff --git a/pkg/services/sqlstore/migrations/user/test/service_account_test.go b/pkg/services/sqlstore/migrations/user/test/service_account_test.go new file mode 100644 index 00000000000..eab2f5ac444 --- /dev/null +++ b/pkg/services/sqlstore/migrations/user/test/service_account_test.go @@ -0,0 +1,247 @@ +package test + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/infra/log" + usermig "github.com/grafana/grafana/pkg/services/sqlstore/migrations/user" + "github.com/grafana/grafana/pkg/services/sqlstore/migrator" + "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/setting" +) + +func TestIntegrationServiceAccountMigration(t *testing.T) { + // Run initial migration to have a working DB + x := setupTestDB(t) + + orgId := 1 + + type migrationTestCase struct { + desc string + serviceAccounts []*user.User + wantServiceAccounts []*user.User + } + testCases := []migrationTestCase{ + { + desc: "basic case", + serviceAccounts: []*user.User{ + { + ID: 1, + UID: "u1", + Name: "sa-basic", + Login: "sa-basic", + Email: "sa-basic", + OrgID: 1, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + { + ID: 2, + UID: "u2", + Name: "sa-basic-admin", + Login: "sa-basic-admin", + Email: "sa-basic-admin", + OrgID: 1, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + }, + wantServiceAccounts: []*user.User{ + { + ID: 1, + Login: fmt.Sprintf("sa-%d-basic", orgId), + }, + { + ID: 2, + Login: fmt.Sprintf("sa-%d-basic-admin", orgId), + }, + }, + }, + { + desc: "should be able to handle multiple sa", + serviceAccounts: []*user.User{ + { + ID: 3, + UID: "u3", + Name: "sa-doan", + Login: "sa-doan", + Email: "sa-doan", + OrgID: 1, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + { + ID: 4, + UID: "u4", + Name: "sa-admin-doan", + Login: "sa-admin-doan", + Email: "sa-admin-doan", + OrgID: 1, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + }, + wantServiceAccounts: []*user.User{ + { + ID: 3, + Login: fmt.Sprintf("sa-%d-doan", orgId), + }, + { + ID: 4, + Login: fmt.Sprintf("sa-%d-admin-doan", orgId), + }, + }, + }, + { + desc: "duplicate logins across different orgs", + serviceAccounts: []*user.User{ + { + ID: 5, + UID: "u5", + Name: "sa-common", + Login: "sa-common@org1.com", + Email: "sa-common@org1.com", + OrgID: 1, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + { + ID: 6, + UID: "u6", + Name: "sa-common", + Login: "sa-common@org2.com", + Email: "sa-common@org2.com", + OrgID: 2, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + }, + wantServiceAccounts: []*user.User{ + { + ID: 5, + Login: "sa-1-common@org1.com", + }, + { + ID: 6, + Login: "sa-2-common@org2.com", + }, + }, + }, + { + desc: "pre-existing sa- prefix", + serviceAccounts: []*user.User{ + { + ID: 7, + UID: "u7", + Name: "sa-preexisting", + Login: "sa-preexisting", + Email: "sa-preexisting@org.com", + OrgID: 1, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + { + ID: 8, + UID: "u8", + Name: "sa-sa-preexisting", + Login: "sa-sa-preexisting", + Email: "sa-sa-preexisting@org.com", + OrgID: 1, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + }, + wantServiceAccounts: []*user.User{ + { + ID: 7, + Login: "sa-1-preexisting", + }, + { + ID: 8, + Login: "sa-1-sa-preexisting", // Ensuring only the first 'sa-' is handled + }, + }, + }, + { + desc: "extSrv accounts also renamed", + serviceAccounts: []*user.User{ + { + ID: 9, + UID: "u9", + Name: "sa-extsvc-slug", + Login: "sa-extsvc-slug", + Email: "sa-extsvc-slug@org.com", + OrgID: 1, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + { + ID: 10, + UID: "u10", + Name: "sa-extsvc-slug2", + Login: "sa-extsvc-slug2", + Email: "sa-extsvc-slug2@org.com", + OrgID: 2, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + }, + wantServiceAccounts: []*user.User{ + { + ID: 9, + Login: "sa-1-extsvc-slug", + }, + { + ID: 10, + Login: "sa-2-extsvc-slug2", + }, + }, + }, + } + + 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 = ?`, usermig.AllowSameLoginCrossOrgs) + require.NoError(t, errDeleteMig) + + // insert service accounts + serviceAccoutsCount, err := x.Insert(tc.serviceAccounts) + require.NoError(t, err) + require.Equal(t, int64(len(tc.serviceAccounts)), serviceAccoutsCount) + + // run the migration + usermigrator := migrator.NewMigrator(x, &setting.Cfg{Logger: log.New("usermigration.test")}) + usermig.AddServiceAccountsAllowSameLoginCrossOrgs(usermigrator) + errRunningMig := usermigrator.Start(false, 0) + require.NoError(t, errRunningMig) + + // Check service accounts + resultingServiceAccounts := []user.User{} + err = x.Table("user").Find(&resultingServiceAccounts) + require.NoError(t, err) + + for i := range tc.wantServiceAccounts { + for _, sa := range resultingServiceAccounts { + if sa.ID == tc.wantServiceAccounts[i].ID { + assert.Equal(t, tc.wantServiceAccounts[i].Login, sa.Login) + } + } + } + }) + } +} diff --git a/pkg/services/sqlstore/migrations/user/test/user_test.go b/pkg/services/sqlstore/migrations/user/test/user_test.go new file mode 100644 index 00000000000..ab7b34e1287 --- /dev/null +++ b/pkg/services/sqlstore/migrations/user/test/user_test.go @@ -0,0 +1,55 @@ +package test + +import ( + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/require" + "gopkg.in/ini.v1" + "xorm.io/xorm" + + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/sqlstore/migrations" + "github.com/grafana/grafana/pkg/services/sqlstore/migrator" + "github.com/grafana/grafana/pkg/services/sqlstore/sqlutil" + "github.com/grafana/grafana/pkg/setting" +) + +// Setup users +var ( + now = time.Now() +) + +func setupTestDB(t *testing.T) *xorm.Engine { + t.Helper() + dbType := sqlutil.GetTestDBType() + testDB, err := sqlutil.GetTestDB(dbType) + require.NoError(t, err) + + t.Cleanup(testDB.Cleanup) + + x, err := xorm.NewEngine(testDB.DriverName, testDB.ConnStr) + require.NoError(t, err) + + t.Cleanup(func() { + if err := x.Close(); err != nil { + fmt.Printf("failed to close xorm engine: %v", err) + } + }) + + err = migrator.NewDialect(x.DriverName()).CleanDB(x) + require.NoError(t, err) + + mg := migrator.NewMigrator(x, &setting.Cfg{ + Logger: log.New("users.test"), + Raw: ini.Empty(), + }) + migrations := &migrations.OSSMigrations{} + migrations.AddMigration(mg) + + err = mg.Start(false, 0) + require.NoError(t, err) + + return x +} diff --git a/pkg/services/sqlstore/migrations/user_mig.go b/pkg/services/sqlstore/migrations/user_mig.go index bac9f99bf11..77ed12e5876 100644 --- a/pkg/services/sqlstore/migrations/user_mig.go +++ b/pkg/services/sqlstore/migrations/user_mig.go @@ -5,6 +5,7 @@ import ( "xorm.io/xorm" + "github.com/grafana/grafana/pkg/services/sqlstore/migrations/user" . "github.com/grafana/grafana/pkg/services/sqlstore/migrator" "github.com/grafana/grafana/pkg/util" ) @@ -156,10 +157,7 @@ func addUserMigrations(mg *Migrator) { // Service accounts login were not unique per org. this migration is part of making it unique per org // to be able to create service accounts that are unique per org - mg.AddMigration("Update login field for service accounts", NewRawSQLMigration(""). - SQLite("UPDATE user SET login = 'sa-' || CAST(org_id AS TEXT) || '-' || REPLACE(login, 'sa-', '') WHERE login IS NOT NULL AND is_service_account = 1;"). - Postgres("UPDATE \"user\" SET login = 'sa-' || org_id::text || '-' || REPLACE(login, 'sa-', '') WHERE login IS NOT NULL AND is_service_account = true;"). - Mysql("UPDATE user SET login = CONCAT('sa-', CAST(org_id AS CHAR), '-', REPLACE(login, 'sa-', '')) WHERE login IS NOT NULL AND is_service_account = 1;")) + mg.AddMigration(user.AllowSameLoginCrossOrgs, &user.ServiceAccountsSameLoginCrossOrgs{}) } const migSQLITEisServiceAccountNullable = `ALTER TABLE user ADD COLUMN tmp_service_account BOOLEAN DEFAULT 0;