Fix: Deduplicate OrgID in SA logins (#94378)

* Fix: Deduplicate OrgID in SA logins

pkg/services/sqlstore/migrations/usermig/service_account_multiple_org_login_migrator.go

@@ -9,12 +9,16 @@ 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
 // to be able to create service accounts that are unique per org
 func AddServiceAccountsAllowSameLoginCrossOrgs(mg *migrator.Migrator) {
 	mg.AddMigration(AllowSameLoginCrossOrgs, &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(DedupOrgInLogin, &ServiceAccountsDeduplicateOrgInLogin{})
 }
 
 var _ migrator.CodeMigration = new(ServiceAccountsSameLoginCrossOrgs)
@@ -76,3 +80,48 @@ 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"
+            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 || '-%';
+        `)
+	case migrator.MySQL:
+		_, err = sess.Exec(`
+            UPDATE user
+            SET login = CONCAT('sa-', org_id, SUBSTRING(login, LENGTH(CONCAT('sa-', org_id, '-', org_id))+1))
+            WHERE login IS NOT NULL
+                AND is_service_account = 1
+                AND login LIKE CONCAT('sa-', org_id, '-', org_id, '-%');
+        `)
+	case migrator.SQLite:
+		_, err = sess.Exec(`
+            UPDATE ` + dialect.Quote("user") + `
+            SET login = 'sa-' || CAST(org_id AS TEXT) || SUBSTRING(login, LENGTH('sa-'||CAST(org_id AS TEXT)||'-'||CAST(org_id AS TEXT))+1)
+            WHERE login IS NOT NULL
+                AND is_service_account = 1
+                AND login LIKE 'sa-'||CAST(org_id AS TEXT)||'-'||CAST(org_id AS TEXT)||'-%';
+        `)
+	default:
+		return fmt.Errorf("dialect not supported: %s", dialect)
+	}
+
+	return err
+}

pkg/services/sqlstore/migrations/usermig/test/service_account_test.go

@@ -285,3 +285,125 @@ 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",
+				},
+			},
+		},
+	}
+
+	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)
+					}
+				}
+			}
+		})
+	}
+}