Auth: Force lowercase login/email for users (#86359)

* [WIP]: Force lowercase login/email for user CRUD

* warn and remove use of userCaseInsensitiveLogin check

* remove log warning

* reimplementation of the caseinsensitive

* need to decide if we want the conflict check or not

* remvoved the tests for conflict user by getEmail, getLogin

* added tests for user lowercase migration

* wip: emails next

* tests for email lowercasing

* review comments

* optimized login and email lookup before migrating
This commit is contained in:
Eric Leijonmarck
2024-04-25 17:31:17 +01:00
committed by GitHub
parent 748b3c855c
commit e394e16073
8 changed files with 374 additions and 63 deletions
@@ -0,0 +1,68 @@
package usermig
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
}
@@ -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"
"github.com/grafana/grafana/pkg/services/sqlstore/migrations/usermig"
"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)
}
}
}
})
}
}
@@ -0,0 +1,253 @@
package test
import (
"testing"
"time"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/sqlstore/migrations/usermig"
"github.com/grafana/grafana/pkg/services/sqlstore/migrator"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/setting"
)
func TestLowerCaseMigration(t *testing.T) {
type migrationTestCase struct {
desc string
users []*user.User
wantUsers []*user.User
}
testCases := []migrationTestCase{
{
desc: "basic case updates login and email to lowercase",
users: []*user.User{
{
ID: 1,
UID: "u1",
Login: "User1",
Email: "USER1@domain.com",
Name: "user1",
OrgID: 1,
Created: now,
Updated: now,
},
{
ID: 2,
UID: "u2",
Login: "User2",
Email: "USER2@domain.com",
Name: "user2",
OrgID: 1,
Created: now,
Updated: now,
},
},
wantUsers: []*user.User{
{
ID: 1,
Login: "user1",
Email: "user1@domain.com",
},
{
ID: 2,
Login: "user2",
Email: "user2@domain.com",
},
},
},
// "2 users - same login, one already lowercase"
{
desc: "2 users with same login one already has lowercase so we keep both",
users: []*user.User{
{
ID: 1,
UID: "u1",
Login: "user1",
Email: "user1@email.com",
Name: "user1",
OrgID: 1,
Created: now,
Updated: now,
},
{
ID: 2,
UID: "u2",
Login: "User1",
Email: "user1-new@email.com",
Name: "user2",
OrgID: 1,
Created: now,
Updated: now,
},
},
wantUsers: []*user.User{
{
ID: 1,
Login: "user1",
Email: "user1@email.com",
},
{
ID: 2,
Login: "User1",
Email: "user1-new@email.com",
},
},
},
// "2 users - same login, one already lowercase"
{
desc: "2 users with same login one already has lowercase so we keep both case for uppercasing comes first in our loop",
users: []*user.User{
{
ID: 1,
UID: "u1",
Login: "User1",
Email: "user1@email.com",
Name: "user1",
OrgID: 1,
Created: now,
Updated: now,
},
{
ID: 2,
UID: "u2",
Login: "user1",
Email: "user1-new@email.com",
Name: "user2",
OrgID: 1,
Created: now,
Updated: now,
},
},
wantUsers: []*user.User{
{
ID: 1,
Login: "User1",
Email: "user1@email.com",
},
{
ID: 2,
Login: "user1",
Email: "user1-new@email.com",
},
},
},
// "2 users - same email, one already lowercase"
{
desc: "2 users with same email one already has lowercase so we keep both",
users: []*user.User{
{
ID: 1,
UID: "u1",
Login: "user1",
Email: "USER1@email.com",
Name: "user1",
OrgID: 1,
Created: now,
Updated: now,
},
{
ID: 2,
UID: "u2",
Login: "user1-new-login",
Email: "user1@email.com",
Name: "user2",
OrgID: 1,
Created: now,
Updated: now,
},
},
wantUsers: []*user.User{
{
ID: 1,
Login: "user1",
Email: "USER1@email.com",
},
{
ID: 2,
Login: "user1-new-login",
Email: "user1@email.com",
},
},
},
// "2 users - same login, none lowercase"
{
desc: "2 users with same login noone is lowercased we pick the most recent user to lowercase",
users: []*user.User{
{
ID: 1,
UID: "u1",
Login: "USER1",
Email: "user1@mail.com",
Name: "user1",
OrgID: 1,
Created: now.Add(-1 * time.Hour),
Updated: now.Add(-1 * time.Hour),
},
{
ID: 2,
UID: "u2",
Login: "User1",
Email: "user1-new@mail.com",
Name: "user2",
OrgID: 1,
Created: now,
Updated: now,
},
},
wantUsers: []*user.User{
{
ID: 1,
Login: "user1",
Email: "user1@mail.com",
},
{
ID: 2,
Login: "User1",
Email: "user1-new@mail.com",
},
},
},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
// Run initial migration to have a working DB
x := setupTestDB(t)
// Remove migration
_, errDeleteMig := x.Exec(`DELETE FROM migration_log WHERE migration_id = ?`, usermig.LowerCaseUserLoginAndEmail)
require.NoError(t, errDeleteMig)
// insert users
usersCount, err := x.Insert(tc.users)
require.NoError(t, err)
require.Equal(t, int64(len(tc.users)), usersCount)
// run the migration
usermigrator := migrator.NewMigrator(x, &setting.Cfg{Logger: log.New("usermigration.test")})
usermig.AddLowerCaseUserLoginAndEmail(usermigrator)
errRunningMig := usermigrator.Start(false, 0)
require.NoError(t, errRunningMig)
// Check users
resultingUsers := []user.User{}
err = x.Table("user").Find(&resultingUsers)
require.NoError(t, err)
// Check that the users have been updated
require.Equal(t, len(tc.wantUsers), len(resultingUsers))
for i := range tc.wantUsers {
for _, u := range resultingUsers {
if u.ID == tc.wantUsers[i].ID {
assert.Equal(t, tc.wantUsers[i].Login, u.Login)
assert.Equal(t, tc.wantUsers[i].Email, u.Email)
}
}
}
})
}
}
@@ -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
}
@@ -0,0 +1,97 @@
package usermig
import (
"strings"
"github.com/grafana/grafana/pkg/services/sqlstore/migrator"
"github.com/grafana/grafana/pkg/services/user"
"xorm.io/xorm"
)
const (
LowerCaseUserLoginAndEmail = "update login and email fields to lowercase"
)
// AddLowerCaseUserLoginAndEmail adds a migration that updates the login and email fields of all users to be in lower case.
func AddLowerCaseUserLoginAndEmail(mg *migrator.Migrator) {
mg.AddMigration(LowerCaseUserLoginAndEmail, &UsersLowerCaseLoginAndEmail{})
}
var _ migrator.CodeMigration = new(UsersLowerCaseLoginAndEmail)
type UsersLowerCaseLoginAndEmail struct {
migrator.MigrationBase
}
func (p *UsersLowerCaseLoginAndEmail) SQL(dialect migrator.Dialect) string {
return "code migration"
}
func (p *UsersLowerCaseLoginAndEmail) Exec(sess *xorm.Session, mg *migrator.Migrator) error {
// Get all users
users := make([]*user.User, 0)
err := sess.Table("user").Find(&users)
if err != nil {
return err
}
processedLogins := make(map[string]bool)
processedEmails := make(map[string]bool)
for _, usr := range users {
/*
LOGIN
*/
lowerLogin := strings.ToLower(usr.Login)
// only work through if login is not already in lower case
if usr.Login != lowerLogin && !processedLogins[lowerLogin] {
// Check if lower login exists
existingLowerCasedUserLogin := &user.User{}
// lowercaseexists in database
hasLowerCasedLogin, err := sess.Table("user").Where("login = ?", lowerLogin).Get(existingLowerCasedUserLogin)
if err != nil {
return err
}
// If exact login does not exist and lower case login does not exist, update the user's login to be in lower case
if !hasLowerCasedLogin {
uLogin := user.User{
Name: usr.Name,
Login: lowerLogin,
}
_, err := sess.ID(usr.ID).Update(&uLogin)
if err != nil {
return err
}
}
}
processedLogins[lowerLogin] = true
/*
EMAIL
*/
lowerEmail := strings.ToLower(usr.Email)
// only work through if email is not already in lower case
if usr.Email != lowerEmail && !processedEmails[lowerEmail] {
// Check if lower case email exists
existingUserEmail := &user.User{}
hasLowerCasedEmail, err := sess.Table("user").Where("email = ?", lowerEmail).Get(existingUserEmail)
if err != nil {
return err
}
// If lower case email does not exist, update the user's email to be in lower case
if !hasLowerCasedEmail {
uEmail := user.User{
Name: usr.Name,
Email: lowerEmail,
}
_, err := sess.ID(usr.ID).Update(&uEmail)
if err != nil {
return err
}
}
}
processedEmails[lowerEmail] = true
}
return nil
}