From 829e425b4df3d378d22f43381829f5ac4e1e66a1 Mon Sep 17 00:00:00 2001 From: "grafana-delivery-bot[bot]" <132647405+grafana-delivery-bot[bot]@users.noreply.github.com> Date: Wed, 23 Oct 2024 15:47:42 +0200 Subject: [PATCH] [v11.0.x] Fix: Deduplicate OrgID in SA logins (#94400) * Fix: Deduplicate OrgID in SA logins (#94378) * Fix: Deduplicate OrgID in SA logins (cherry picked from commit b90e09e96619ce21684454539edbc029ebac4a85) * Fix: Actually call the DedupOrgInLogin migration (#94520) * Fix: Account for conflicting logins in dedupOrgInlogin migration (#94669) --------- Co-authored-by: Gabriel MABILLE --- pkg/services/sqlstore/migrations/user_mig.go | 3 + ...ice_account_multiple_org_login_migrator.go | 58 +++++++ .../usermig/test/service_account_test.go | 159 ++++++++++++++++++ 3 files changed, 220 insertions(+) diff --git a/pkg/services/sqlstore/migrations/user_mig.go b/pkg/services/sqlstore/migrations/user_mig.go index d4d8384a4d3..68fe8ba9af4 100644 --- a/pkg/services/sqlstore/migrations/user_mig.go +++ b/pkg/services/sqlstore/migrations/user_mig.go @@ -158,6 +158,9 @@ 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(usermig.AllowSameLoginCrossOrgs, &usermig.ServiceAccountsSameLoginCrossOrgs{}) + // Before it was fixed, the previous migration introduced the org_id again in logins that already had it. + // This migration removes the duplicate org_id from the login. + mg.AddMigration(usermig.DedupOrgInLogin, &usermig.ServiceAccountsDeduplicateOrgInLogin{}) // Users login and email should be in lower case mg.AddMigration(usermig.LowerCaseUserLoginAndEmail, &usermig.UsersLowerCaseLoginAndEmail{}) diff --git a/pkg/services/sqlstore/migrations/usermig/service_account_multiple_org_login_migrator.go b/pkg/services/sqlstore/migrations/usermig/service_account_multiple_org_login_migrator.go index d51cfacd5a6..05436292267 100644 --- a/pkg/services/sqlstore/migrations/usermig/service_account_multiple_org_login_migrator.go +++ b/pkg/services/sqlstore/migrations/usermig/service_account_multiple_org_login_migrator.go @@ -9,6 +9,7 @@ import ( const ( AllowSameLoginCrossOrgs = "update login field with orgid to allow for multiple service accounts with same name across orgs" + DedupOrgInLogin = "update service accounts login field orgid to appear only once" ) // Service accounts login were not unique per org. this migration is part of making it unique per org @@ -76,3 +77,60 @@ func (p *ServiceAccountsSameLoginCrossOrgs) Exec(sess *xorm.Session, mg *migrato } return err } + +type ServiceAccountsDeduplicateOrgInLogin struct { + migrator.MigrationBase +} + +func (p *ServiceAccountsDeduplicateOrgInLogin) SQL(dialect migrator.Dialect) string { + return "code migration" +} + +func (p *ServiceAccountsDeduplicateOrgInLogin) Exec(sess *xorm.Session, mg *migrator.Migrator) error { + dialect := mg.Dialect + var err error + + // var logins []Login + switch dialect.DriverName() { + case migrator.Postgres: + _, err = sess.Exec(` + UPDATE "user" AS u + SET login = 'sa-' || org_id::text || SUBSTRING(login FROM LENGTH('sa-' || org_id::text || '-' || org_id::text)+1) + WHERE login IS NOT NULL + AND is_service_account = true + AND login LIKE 'sa-' || org_id::text || '-' || org_id::text || '-%' + AND NOT EXISTS ( + SELECT 1 + FROM "user" AS u2 + WHERE u2.login = 'sa-' || u.org_id::text || SUBSTRING(u.login FROM LENGTH('sa-' || u.org_id::text || '-' || u.org_id::text)+1) + );; + `) + case migrator.MySQL: + _, err = sess.Exec(` + UPDATE user AS u + LEFT JOIN user AS u2 ON u2.login = CONCAT('sa-', u.org_id, SUBSTRING(u.login, LENGTH(CONCAT('sa-', u.org_id, '-', u.org_id))+1)) + SET u.login = CONCAT('sa-', u.org_id, SUBSTRING(u.login, LENGTH(CONCAT('sa-', u.org_id, '-', u.org_id))+1)) + WHERE u.login IS NOT NULL + AND u.is_service_account = 1 + AND u.login LIKE CONCAT('sa-', u.org_id, '-', u.org_id, '-%') + AND u2.login IS NULL; + `) + case migrator.SQLite: + _, err = sess.Exec(` + UPDATE ` + dialect.Quote("user") + ` AS u + SET login = 'sa-' || CAST(u.org_id AS TEXT) || SUBSTRING(u.login, LENGTH('sa-'||CAST(u.org_id AS TEXT)||'-'||CAST(u.org_id AS TEXT))+1) + WHERE u.login IS NOT NULL + AND u.is_service_account = 1 + AND u.login LIKE 'sa-'||CAST(u.org_id AS TEXT)||'-'||CAST(u.org_id AS TEXT)||'-%' + AND NOT EXISTS ( + SELECT 1 + FROM ` + dialect.Quote("user") + `AS u2 + WHERE u2.login = 'sa-' || CAST(u.org_id AS TEXT) || SUBSTRING(u.login, LENGTH('sa-'||CAST(u.org_id AS TEXT)||'-'||CAST(u.org_id AS TEXT))+1) + );; + `) + default: + return fmt.Errorf("dialect not supported: %s", dialect) + } + + return err +} diff --git a/pkg/services/sqlstore/migrations/usermig/test/service_account_test.go b/pkg/services/sqlstore/migrations/usermig/test/service_account_test.go index 17330453cce..7b289e451bc 100644 --- a/pkg/services/sqlstore/migrations/usermig/test/service_account_test.go +++ b/pkg/services/sqlstore/migrations/usermig/test/service_account_test.go @@ -285,3 +285,162 @@ func TestIntegrationServiceAccountMigration(t *testing.T) { }) } } + +func TestIntegrationServiceAccountDedupOrgMigration(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test in short mode") + } + // Run initial migration to have a working DB + x := setupTestDB(t) + + type migrationTestCase struct { + desc string + serviceAccounts []*user.User + wantServiceAccounts []*user.User + } + testCases := []migrationTestCase{ + { + desc: "no change", + serviceAccounts: []*user.User{ + { + ID: 1, + UID: "u1", + Name: "sa-1-nochange", + Login: "sa-1-nochange", + Email: "sa-1-nochange@example.org", + OrgID: 1, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + { + ID: 2, + UID: "u2", + Name: "sa-2-nochange", + Login: "sa-2-nochange", + Email: "sa-2-nochange@example.org", + OrgID: 2, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + }, + wantServiceAccounts: []*user.User{ + { + ID: 1, + Login: "sa-1-nochange", + }, + { + ID: 2, + Login: "sa-2-nochange", + }, + }, + }, + { + desc: "dedup org in login", + serviceAccounts: []*user.User{ + { + ID: 3, + UID: "u3", + Name: "sa-1-dedup", + Login: "sa-1-1-dedup", + Email: "sa-1-dedup@example.org", + OrgID: 1, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + { + ID: 4, + UID: "u4", + Name: "sa-6480-dedup", + Login: "sa-6480-6480-dedup", + Email: "sa-6480-dedup@example.org", + OrgID: 6480, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + }, + wantServiceAccounts: []*user.User{ + { + ID: 3, + Login: "sa-1-dedup", + }, + { + ID: 4, + Login: "sa-6480-dedup", + }, + }, + }, + { + desc: "handle conflicts", + serviceAccounts: []*user.User{ + { + ID: 5, + UID: "u5", + Name: "sa-2-conflict", + Login: "sa-2-conflict", + Email: "sa-2-conflict@example.org", + OrgID: 2, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + { + ID: 6, + UID: "u6", + Name: "sa-2b-conflict", + Login: "sa-2-2-conflict", + Email: "sa-2b-conflict@example.org", + OrgID: 2, + Created: now, + Updated: now, + IsServiceAccount: true, + }, + }, + wantServiceAccounts: []*user.User{ + { + ID: 5, + Login: "sa-2-conflict", + }, + { + ID: 6, + Login: "sa-2-2-conflict", + }, + }, + }, + } + + 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.DedupOrgInLogin) + 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")}) + usermigrator.AddMigration(usermig.DedupOrgInLogin, &usermig.ServiceAccountsDeduplicateOrgInLogin{}) + 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) + } + } + } + }) + } +}