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 (cherry picked from commit e394e16)
grafana · Apr 26, 2024 · c00b43a · c00b43a
1 parent aba28be
commit c00b43a
Show file tree

Hide file tree

Showing 8 changed files with 371 additions and 67 deletions.
diff --git 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/...ce_account_multiple_org_login_migrator.go → ...ce_account_multiple_org_login_migrator.go b/...ce_account_multiple_org_login_migrator.go → ...ce_account_multiple_org_login_migrator.go
@@ -1,4 +1,4 @@
-package user
+package usermig
 
 import (
 	"fmt"

diff --git a/...rations/user/test/service_account_test.go → ...ions/usermig/test/service_account_test.go b/...rations/user/test/service_account_test.go → ...ions/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
@@ -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/...qlstore/migrations/user/test/user_test.go → ...tore/migrations/usermig/test/user_test.go b/...qlstore/migrations/user/test/user_test.go → ...tore/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
@@ -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
+}