diff --git a/pkg/services/sqlstore/migrations/user_mig.go b/pkg/services/sqlstore/migrations/user_mig.go index 77ed12e5876..8603314a3d1 100644 --- a/pkg/services/sqlstore/migrations/user_mig.go +++ b/pkg/services/sqlstore/migrations/user_mig.go @@ -5,7 +5,7 @@ import ( "xorm.io/xorm" - "github.com/grafana/grafana/pkg/services/sqlstore/migrations/user" + "github.com/grafana/grafana/pkg/services/sqlstore/migrations/usermig" . "github.com/grafana/grafana/pkg/services/sqlstore/migrator" "github.com/grafana/grafana/pkg/util" ) @@ -157,7 +157,10 @@ 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(user.AllowSameLoginCrossOrgs, &user.ServiceAccountsSameLoginCrossOrgs{}) + mg.AddMigration(usermig.AllowSameLoginCrossOrgs, &usermig.ServiceAccountsSameLoginCrossOrgs{}) + + // Users login and email should be in lower case + mg.AddMigration(usermig.LowerCaseUserLoginAndEmail, &usermig.UsersLowerCaseLoginAndEmail{}) } const migSQLITEisServiceAccountNullable = `ALTER TABLE user ADD COLUMN tmp_service_account BOOLEAN DEFAULT 0; diff --git a/pkg/services/sqlstore/migrations/user/service_account_multiple_org_login_migrator.go b/pkg/services/sqlstore/migrations/usermig/service_account_multiple_org_login_migrator.go similarity index 99% rename from pkg/services/sqlstore/migrations/user/service_account_multiple_org_login_migrator.go rename to pkg/services/sqlstore/migrations/usermig/service_account_multiple_org_login_migrator.go index 064985c0f73..1d60a4f7f28 100644 --- a/pkg/services/sqlstore/migrations/user/service_account_multiple_org_login_migrator.go +++ b/pkg/services/sqlstore/migrations/usermig/service_account_multiple_org_login_migrator.go @@ -1,4 +1,4 @@ -package user +package usermig import ( "fmt" diff --git a/pkg/services/sqlstore/migrations/user/test/service_account_test.go b/pkg/services/sqlstore/migrations/usermig/test/service_account_test.go similarity index 98% rename from pkg/services/sqlstore/migrations/user/test/service_account_test.go rename to pkg/services/sqlstore/migrations/usermig/test/service_account_test.go index eab2f5ac444..043c9852245 100644 --- a/pkg/services/sqlstore/migrations/user/test/service_account_test.go +++ b/pkg/services/sqlstore/migrations/usermig/test/service_account_test.go @@ -8,7 +8,7 @@ import ( "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/migrations/usermig" "github.com/grafana/grafana/pkg/services/sqlstore/migrator" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" diff --git a/pkg/services/sqlstore/migrations/usermig/test/user_lowercase_login_and_email_test.go b/pkg/services/sqlstore/migrations/usermig/test/user_lowercase_login_and_email_test.go new file mode 100644 index 00000000000..33e206ca38c --- /dev/null +++ b/pkg/services/sqlstore/migrations/usermig/test/user_lowercase_login_and_email_test.go @@ -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) + } + } + } + }) + } +} diff --git a/pkg/services/sqlstore/migrations/user/test/user_test.go b/pkg/services/sqlstore/migrations/usermig/test/user_test.go similarity index 100% rename from pkg/services/sqlstore/migrations/user/test/user_test.go rename to pkg/services/sqlstore/migrations/usermig/test/user_test.go diff --git a/pkg/services/sqlstore/migrations/usermig/user_lowercase_login_and_email.go b/pkg/services/sqlstore/migrations/usermig/user_lowercase_login_and_email.go new file mode 100644 index 00000000000..570b79ba52e --- /dev/null +++ b/pkg/services/sqlstore/migrations/usermig/user_lowercase_login_and_email.go @@ -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 +} diff --git a/pkg/services/user/userimpl/store.go b/pkg/services/user/userimpl/store.go index c0bc043e499..ec8b6c8cf08 100644 --- a/pkg/services/user/userimpl/store.go +++ b/pkg/services/user/userimpl/store.go @@ -90,9 +90,10 @@ func (ss *sqlStore) Insert(ctx context.Context, cmd *user.User) (int64, error) { func (ss *sqlStore) Get(ctx context.Context, usr *user.User) (*user.User, error) { ret := &user.User{} err := ss.db.WithDbSession(ctx, func(sess *db.Session) error { - login := usr.Login - email := usr.Email - where := "LOWER(email)=LOWER(?) OR LOWER(login)=LOWER(?)" + // enforcement of lowercase due to forcement of caseinsensitive login + login := strings.ToLower(usr.Login) + email := strings.ToLower(usr.Email) + where := "email=? OR login=?" exists, err := sess.Where(where, email, login).Get(ret) if !exists { @@ -178,6 +179,9 @@ func (ss *sqlStore) CaseInsensitiveLoginConflict(ctx context.Context, login, ema } func (ss *sqlStore) GetByLogin(ctx context.Context, query *user.GetUserByLoginQuery) (*user.User, error) { + // enforcement of lowercase due to forcement of caseinsensitive login + query.LoginOrEmail = strings.ToLower(query.LoginOrEmail) + usr := &user.User{} err := ss.db.WithDbSession(ctx, func(sess *db.Session) error { if query.LoginOrEmail == "" { @@ -191,7 +195,7 @@ func (ss *sqlStore) GetByLogin(ctx context.Context, query *user.GetUserByLoginQu // Since username can be an email address, attempt login with email address // first if the login field has the "@" symbol. if strings.Contains(query.LoginOrEmail, "@") { - where = "LOWER(email)=LOWER(?)" + where = "email=?" has, err = sess.Where(ss.notServiceAccountFilter()).Where(where, query.LoginOrEmail).Get(usr) if err != nil { return err @@ -200,7 +204,7 @@ func (ss *sqlStore) GetByLogin(ctx context.Context, query *user.GetUserByLoginQu // Look for the login field instead of email if !has { - where = "LOWER(login)=LOWER(?)" + where = "login=?" has, err = sess.Where(ss.notServiceAccountFilter()).Where(where, query.LoginOrEmail).Get(usr) } @@ -209,9 +213,6 @@ func (ss *sqlStore) GetByLogin(ctx context.Context, query *user.GetUserByLoginQu } else if !has { return user.ErrUserNotFound } - if err := ss.userCaseInsensitiveLoginConflict(ctx, sess, usr.Login, usr.Email); err != nil { - return err - } return nil }) @@ -223,13 +224,16 @@ func (ss *sqlStore) GetByLogin(ctx context.Context, query *user.GetUserByLoginQu } func (ss *sqlStore) GetByEmail(ctx context.Context, query *user.GetUserByEmailQuery) (*user.User, error) { + // enforcement of lowercase due to forcement of caseinsensitive login + query.Email = strings.ToLower(query.Email) + usr := &user.User{} err := ss.db.WithDbSession(ctx, func(sess *db.Session) error { if query.Email == "" { return user.ErrUserNotFound } - where := "LOWER(email)=LOWER(?)" + where := "email=?" has, err := sess.Where(ss.notServiceAccountFilter()).Where(where, query.Email).Get(usr) if err != nil { @@ -237,10 +241,6 @@ func (ss *sqlStore) GetByEmail(ctx context.Context, query *user.GetUserByEmailQu } else if !has { return user.ErrUserNotFound } - - if err := ss.userCaseInsensitiveLoginConflict(ctx, sess, usr.Login, usr.Email); err != nil { - return err - } return nil }) if err != nil { @@ -249,21 +249,6 @@ func (ss *sqlStore) GetByEmail(ctx context.Context, query *user.GetUserByEmailQu return usr, nil } -func (ss *sqlStore) userCaseInsensitiveLoginConflict(ctx context.Context, sess *db.Session, login, email string) error { - users := make([]user.User, 0) - - if err := sess.Where("LOWER(email)=LOWER(?) OR LOWER(login)=LOWER(?)", - email, login).Find(&users); err != nil { - return err - } - - if len(users) > 1 { - return &user.ErrCaseInsensitiveLoginConflict{Users: users} - } - - return nil -} - // LoginConflict returns an error if the provided email or login are already // associated with a user. If caseInsensitive is true the search is not case // sensitive. @@ -299,6 +284,10 @@ func (ss *sqlStore) loginConflict(ctx context.Context, sess *db.Session, login, } func (ss *sqlStore) Update(ctx context.Context, cmd *user.UpdateUserCommand) error { + // enforcement of lowercase due to forcement of caseinsensitive login + cmd.Login = strings.ToLower(cmd.Login) + cmd.Email = strings.ToLower(cmd.Email) + return ss.db.WithTransactionalDbSession(ctx, func(sess *db.Session) error { user := user.User{ Name: cmd.Name, diff --git a/pkg/services/user/userimpl/store_test.go b/pkg/services/user/userimpl/store_test.go index 6803ff4ce01..5cc3731c2a7 100644 --- a/pkg/services/user/userimpl/store_test.go +++ b/pkg/services/user/userimpl/store_test.go @@ -344,37 +344,6 @@ func TestIntegrationUserDataAccess(t *testing.T) { return nil }) require.NoError(t, err) - - t.Run("GetByEmail - email conflict", func(t *testing.T) { - query := user.GetUserByEmailQuery{Email: "confusertest@test.com"} - _, err = userStore.GetByEmail(context.Background(), &query) - require.Error(t, err) - }) - - t.Run("GetByEmail - login conflict", func(t *testing.T) { - query := user.GetUserByEmailQuery{Email: "user_test_login_conflict@test.com"} - _, err = userStore.GetByEmail(context.Background(), &query) - require.Error(t, err) - }) - - t.Run("GetByLogin - email conflict", func(t *testing.T) { - query := user.GetUserByLoginQuery{LoginOrEmail: "user_email_conflict_two"} - _, err = userStore.GetByLogin(context.Background(), &query) - require.Error(t, err) - }) - - t.Run("GetByLogin - login conflict", func(t *testing.T) { - query := user.GetUserByLoginQuery{LoginOrEmail: "user_test_login_conflict"} - _, err = userStore.GetByLogin(context.Background(), &query) - require.Error(t, err) - }) - - t.Run("GetByLogin - login conflict by email", func(t *testing.T) { - query := user.GetUserByLoginQuery{LoginOrEmail: "user_test_login_conflict@test.com"} - _, err = userStore.GetByLogin(context.Background(), &query) - require.Error(t, err) - }) - t.Run("GetByLogin - user2 uses user1.email as login", func(t *testing.T) { // create user_1 user1 := &user.User{