public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[v11.0.x] Auth: Force lowercase login/email for users (#86985)

Browse Source 
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 e394e16073)
This commit is contained in:
Eric Leijonmarck
2024-05-02 13:05:25 +01:00
committed by GitHub
parent f0302dd2d7
commit b219aa7688
8 changed files with 371 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/sqlstore/migrations/user_mig.go
+5 -2
View File
@@ -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;
pkg/services/sqlstore/migrations/user/service_account_multiple_org_login_migrator.go → pkg/services/sqlstore/migrations/usermig/service_account_multiple_org_login_migrator.go
+1 -1
View File
@@ -1,4 +1,4 @@
package user
package usermig
import (
"fmt"
pkg/services/sqlstore/migrations/user/test/service_account_test.go → pkg/services/sqlstore/migrations/usermig/test/service_account_test.go
+1 -1
View File
@@ -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"
pkg/services/sqlstore/migrations/usermig/test/user_lowercase_login_and_email_test.go
+253
View File
@@ -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)
}
}
}
})
}
}
pkg/services/sqlstore/migrations/user/test/user_test.go → pkg/services/sqlstore/migrations/usermig/test/user_test.go
View File
pkg/services/sqlstore/migrations/usermig/user_lowercase_login_and_email.go
+97
View File
@@ -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
}
pkg/services/user/userimpl/store.go
+14 -32
View File
@@ -89,9 +89,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 {
@@ -177,6 +178,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 == "" {
@@ -190,7 +194,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
@@ -199,7 +203,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)
}
@@ -208,9 +212,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
})
@@ -222,13 +223,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 {
@@ -236,10 +240,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 {
@@ -248,21 +248,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.
@@ -298,6 +283,7 @@ 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)
@@ -321,10 +307,6 @@ func (ss *sqlStore) Update(ctx context.Context, cmd *user.UpdateUserCommand) err
return err
}
if err := ss.userCaseInsensitiveLoginConflict(ctx, sess, user.Login, user.Email); err != nil {
return err
}
sess.PublishAfterCommit(&events.UserUpdated{
Timestamp: user.Created,
Id: user.ID,
pkg/services/user/userimpl/store_test.go
-40
View File
@@ -345,37 +345,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{
@@ -923,15 +892,6 @@ func TestIntegrationUserUpdate(t *testing.T) {
}
})
t.Run("Testing DB - update generates duplicate user", func(t *testing.T) {
err := userStore.Update(context.Background(), &user.UpdateUserCommand{
Login: "loginuser2",
UserID: users[0].ID,
})
require.Error(t, err)
})
t.Run("Testing DB - update lowercases existing user", func(t *testing.T) {
err := userStore.Update(context.Background(), &user.UpdateUserCommand{
Login: "loginUSER0",