From 0689c5839a7cb366f44c3bd19a4bb3cfe56df54c Mon Sep 17 00:00:00 2001 From: Jguer Date: Fri, 24 Jun 2022 14:59:45 +0000 Subject: [PATCH] Auth: Add option for case insensitive login (#49262) * add case insensitive option * treat id as case insensitive * Users: Add integration tests for case insensitive querying * Prefer config struct to global variable * change key to case_insensitive_login * impede conflicting users from logging in * add tests for impeding user retrieval if conflicting * nits and picks Co-authored-by: gamab * Add check in transaction for conflicting user Co-authored-by: Gabriel MABILLE * add update tests * skip on mysql * add custom messages for user admin view Co-authored-by: Gabriel MABILLE * nit: extra else * linting mistake Co-authored-by: gamab Co-authored-by: Gabriel MABILLE --- pkg/api/user.go | 7 +- pkg/api/user_token.go | 8 +- pkg/models/user.go | 1 + pkg/services/sqlstore/user.go | 111 ++++++++++++++-- pkg/services/sqlstore/user_test.go | 198 +++++++++++++++++++++++++++++ pkg/setting/setting.go | 3 + 6 files changed, 315 insertions(+), 13 deletions(-) diff --git a/pkg/api/user.go b/pkg/api/user.go index 4ea249b29ca..d3a6d19ca97 100644 --- a/pkg/api/user.go +++ b/pkg/api/user.go @@ -136,12 +136,15 @@ func (hs *HTTPServer) handleUpdateUser(ctx context.Context, cmd models.UpdateUse if len(cmd.Login) == 0 { cmd.Login = cmd.Email if len(cmd.Login) == 0 { - return response.Error(400, "Validation error, need to specify either username or email", nil) + return response.Error(http.StatusBadRequest, "Validation error, need to specify either username or email", nil) } } if err := hs.SQLStore.UpdateUser(ctx, &cmd); err != nil { - return response.Error(500, "Failed to update user", err) + if errors.Is(err, models.ErrCaseInsensitive) { + return response.Error(http.StatusConflict, "Update would result in user login conflict", err) + } + return response.Error(http.StatusInternalServerError, "Failed to update user", err) } return response.Success("User updated") diff --git a/pkg/api/user_token.go b/pkg/api/user_token.go index edb5c34da41..ee73259ca9f 100644 --- a/pkg/api/user_token.go +++ b/pkg/api/user_token.go @@ -53,9 +53,13 @@ func (hs *HTTPServer) getUserAuthTokensInternal(c *models.ReqContext, userID int if err := hs.SQLStore.GetUserById(c.Req.Context(), &userQuery); err != nil { if errors.Is(err, models.ErrUserNotFound) { - return response.Error(404, "User not found", err) + return response.Error(http.StatusNotFound, "User not found", err) + } else if errors.Is(err, models.ErrCaseInsensitive) { + return response.Error(http.StatusConflict, + "User has conflicting login or email with another user. Please contact server admin", err) } - return response.Error(500, "Failed to get user", err) + + return response.Error(http.StatusInternalServerError, "Failed to get user", err) } tokens, err := hs.AuthTokenService.GetUserTokens(c.Req.Context(), userID) diff --git a/pkg/models/user.go b/pkg/models/user.go index 9ed0996020b..b29f11a7324 100644 --- a/pkg/models/user.go +++ b/pkg/models/user.go @@ -7,6 +7,7 @@ import ( // Typed errors var ( + ErrCaseInsensitive = errors.New("case insensitive conflict") ErrUserNotFound = errors.New("user not found") ErrUserAlreadyExists = errors.New("user already exists") ErrLastGrafanaAdmin = errors.New("cannot remove last grafana admin") diff --git a/pkg/services/sqlstore/user.go b/pkg/services/sqlstore/user.go index 9cd80da3967..e0071f12d23 100644 --- a/pkg/services/sqlstore/user.go +++ b/pkg/services/sqlstore/user.go @@ -14,6 +14,27 @@ import ( "github.com/grafana/grafana/pkg/util" ) +type ErrCaseInsensitiveLoginConflict struct { + users []models.User +} + +func (e *ErrCaseInsensitiveLoginConflict) Unwrap() error { + return models.ErrCaseInsensitive +} + +func (e *ErrCaseInsensitiveLoginConflict) Error() string { + n := len(e.users) + + userStrings := make([]string, 0, n) + for _, v := range e.users { + userStrings = append(userStrings, fmt.Sprintf("%s (email:%s, id:%d)", v.Login, v.Email, v.Id)) + } + + return fmt.Sprintf( + "Found a conflict in user login information. %d users already exist with either the same login or email: [%s].", + n, strings.Join(userStrings, ", ")) +} + func (ss *SQLStore) getOrgIDForNewUser(sess *DBSession, args models.CreateUserCommand) (int64, error) { if ss.Cfg.AutoAssignOrg && args.OrgId != 0 { if err := verifyExistingOrg(sess, args.OrgId); err != nil { @@ -30,6 +51,21 @@ func (ss *SQLStore) getOrgIDForNewUser(sess *DBSession, args models.CreateUserCo return ss.getOrCreateOrg(sess, orgName) } +func (ss *SQLStore) userCaseInsensitiveLoginConflict(ctx context.Context, sess *DBSession, login, email string) error { + users := make([]models.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 &ErrCaseInsensitiveLoginConflict{users} + } + + return nil +} + // createUser creates a user in the database func (ss *SQLStore) createUser(ctx context.Context, sess *DBSession, args models.CreateUserCommand) (models.User, error) { var user models.User @@ -46,7 +82,14 @@ func (ss *SQLStore) createUser(ctx context.Context, sess *DBSession, args models args.Email = args.Login } - exists, err := sess.Where("email=? OR login=?", args.Email, args.Login).Get(&models.User{}) + where := "email=? OR login=?" + if ss.Cfg.CaseInsensitiveLogin { + where = "LOWER(email)=LOWER(?) OR LOWER(login)=LOWER(?)" + args.Login = strings.ToLower(args.Login) + args.Email = strings.ToLower(args.Email) + } + + exists, err := sess.Where(where, args.Email, args.Login).Get(&models.User{}) if err != nil { return user, err } @@ -158,6 +201,12 @@ func (ss *SQLStore) GetUserById(ctx context.Context, query *models.GetUserByIdQu return models.ErrUserNotFound } + if ss.Cfg.CaseInsensitiveLogin { + if err := ss.userCaseInsensitiveLoginConflict(ctx, sess, user.Login, user.Email); err != nil { + return err + } + } + query.Result = user return nil @@ -172,9 +221,13 @@ func (ss *SQLStore) GetUserByLogin(ctx context.Context, query *models.GetUserByL // Try and find the user by login first. // It's not sufficient to assume that a LoginOrEmail with an "@" is an email. - user := &models.User{Login: query.LoginOrEmail} - has, err := sess.Where(notServiceAccountFilter(ss)).Get(user) + user := &models.User{} + where := "login=?" + if ss.Cfg.CaseInsensitiveLogin { + where = "LOWER(login)=LOWER(?)" + } + has, err := sess.Where(notServiceAccountFilter(ss)).Where(where, query.LoginOrEmail).Get(user) if err != nil { return err } @@ -182,8 +235,12 @@ func (ss *SQLStore) GetUserByLogin(ctx context.Context, query *models.GetUserByL if !has && strings.Contains(query.LoginOrEmail, "@") { // If the user wasn't found, and it contains an "@" fallback to finding the // user by email. - user = &models.User{Email: query.LoginOrEmail} - has, err = sess.Get(user) + where = "email=?" + if ss.Cfg.CaseInsensitiveLogin { + where = "LOWER(email)=LOWER(?)" + } + user = &models.User{} + has, err = sess.Where(notServiceAccountFilter(ss)).Where(where, query.LoginOrEmail).Get(user) } if err != nil { @@ -192,6 +249,12 @@ func (ss *SQLStore) GetUserByLogin(ctx context.Context, query *models.GetUserByL return models.ErrUserNotFound } + if ss.Cfg.CaseInsensitiveLogin { + if err := ss.userCaseInsensitiveLoginConflict(ctx, sess, user.Login, user.Email); err != nil { + return err + } + } + query.Result = user return nil @@ -204,8 +267,13 @@ func (ss *SQLStore) GetUserByEmail(ctx context.Context, query *models.GetUserByE return models.ErrUserNotFound } - user := &models.User{Email: query.Email} - has, err := sess.Where(notServiceAccountFilter(ss)).Get(user) + user := &models.User{} + where := "email=?" + if ss.Cfg.CaseInsensitiveLogin { + where = "LOWER(email)=LOWER(?)" + } + + has, err := sess.Where(notServiceAccountFilter(ss)).Where(where, query.Email).Get(user) if err != nil { return err @@ -213,6 +281,12 @@ func (ss *SQLStore) GetUserByEmail(ctx context.Context, query *models.GetUserByE return models.ErrUserNotFound } + if ss.Cfg.CaseInsensitiveLogin { + if err := ss.userCaseInsensitiveLoginConflict(ctx, sess, user.Login, user.Email); err != nil { + return err + } + } + query.Result = user return nil @@ -220,6 +294,11 @@ func (ss *SQLStore) GetUserByEmail(ctx context.Context, query *models.GetUserByE } func (ss *SQLStore) UpdateUser(ctx context.Context, cmd *models.UpdateUserCommand) error { + if ss.Cfg.CaseInsensitiveLogin { + cmd.Login = strings.ToLower(cmd.Login) + cmd.Email = strings.ToLower(cmd.Email) + } + return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { user := models.User{ Name: cmd.Name, @@ -233,6 +312,12 @@ func (ss *SQLStore) UpdateUser(ctx context.Context, cmd *models.UpdateUserComman return err } + if ss.Cfg.CaseInsensitiveLogin { + if err := ss.userCaseInsensitiveLoginConflict(ctx, sess, user.Login, user.Email); err != nil { + return err + } + } + sess.publishAfterCommit(&events.UserUpdated{ Timestamp: user.Created, Id: user.Id, @@ -430,9 +515,17 @@ func (ss *SQLStore) GetSignedInUser(ctx context.Context, query *models.GetSigned case query.UserId > 0: sess.SQL(rawSQL+"WHERE u.id=?", query.UserId) case query.Login != "": - sess.SQL(rawSQL+"WHERE u.login=?", query.Login) + if ss.Cfg.CaseInsensitiveLogin { + sess.SQL(rawSQL+"WHERE LOWER(u.login)=LOWER(?)", query.Login) + } else { + sess.SQL(rawSQL+"WHERE u.login=?", query.Login) + } case query.Email != "": - sess.SQL(rawSQL+"WHERE u.email=?", query.Email) + if ss.Cfg.CaseInsensitiveLogin { + sess.SQL(rawSQL+"WHERE LOWER(u.email)=LOWER(?)", query.Email) + } else { + sess.SQL(rawSQL+"WHERE u.email=?", query.Email) + } } var user models.SignedInUser diff --git a/pkg/services/sqlstore/user_test.go b/pkg/services/sqlstore/user_test.go index a1a941797c7..e6277d5b310 100644 --- a/pkg/services/sqlstore/user_test.go +++ b/pkg/services/sqlstore/user_test.go @@ -6,10 +6,78 @@ import ( "testing" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/sqlstore/migrator" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestIntegrationUserUpdate(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + + ss := InitTestDB(t) + + users := createFiveTestUsers(t, ss, func(i int) *models.CreateUserCommand { + return &models.CreateUserCommand{ + Email: fmt.Sprint("USER", i, "@test.com"), + Name: fmt.Sprint("USER", i), + Login: fmt.Sprint("loginUSER", i), + IsDisabled: false, + } + }) + + ss.Cfg.CaseInsensitiveLogin = true + + t.Run("Testing DB - update generates duplicate user", func(t *testing.T) { + err := ss.UpdateUser(context.Background(), &models.UpdateUserCommand{ + Login: "loginuser2", + UserId: users[0].Id, + }) + + require.Error(t, err) + }) + + t.Run("Testing DB - update lowercases existing user", func(t *testing.T) { + err := ss.UpdateUser(context.Background(), &models.UpdateUserCommand{ + Login: "loginUSER0", + Email: "USER0@test.com", + UserId: users[0].Id, + }) + require.NoError(t, err) + + query := models.GetUserByIdQuery{Id: users[0].Id} + err = ss.GetUserById(context.Background(), &query) + require.NoError(t, err) + + require.Equal(t, "loginuser0", query.Result.Login) + require.Equal(t, "user0@test.com", query.Result.Email) + }) + + t.Run("Testing DB - no user info provided", func(t *testing.T) { + err := ss.UpdateUser(context.Background(), &models.UpdateUserCommand{ + Login: "", + Email: "", + Name: "Change Name", + UserId: users[3].Id, + }) + require.NoError(t, err) + + query := models.GetUserByIdQuery{Id: users[3].Id} + err = ss.GetUserById(context.Background(), &query) + require.NoError(t, err) + + // Changed + require.Equal(t, "Change Name", query.Result.Name) + + // Unchanged + require.Equal(t, "loginUSER3", query.Result.Login) + require.Equal(t, "USER3@test.com", query.Result.Email) + }) + + ss.Cfg.CaseInsensitiveLogin = false +} + func TestIntegrationUserDataAccess(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") @@ -48,6 +116,52 @@ func TestIntegrationUserDataAccess(t *testing.T) { require.Len(t, query.Result.Rands, 10) require.Len(t, query.Result.Salt, 10) require.False(t, query.Result.IsDisabled) + + t.Run("Get User by email case insensitive", func(t *testing.T) { + ss.Cfg.CaseInsensitiveLogin = true + query := models.GetUserByEmailQuery{Email: "USERtest@TEST.COM"} + err = ss.GetUserByEmail(context.Background(), &query) + require.Nil(t, err) + + require.Equal(t, query.Result.Email, "usertest@test.com") + require.Equal(t, query.Result.Password, "") + require.Len(t, query.Result.Rands, 10) + require.Len(t, query.Result.Salt, 10) + require.False(t, query.Result.IsDisabled) + + ss.Cfg.CaseInsensitiveLogin = false + }) + + t.Run("Get User by login - case insensitive", func(t *testing.T) { + ss.Cfg.CaseInsensitiveLogin = true + + query := models.GetUserByLoginQuery{LoginOrEmail: "USER_test_login"} + err = ss.GetUserByLogin(context.Background(), &query) + require.Nil(t, err) + + require.Equal(t, query.Result.Email, "usertest@test.com") + require.Equal(t, query.Result.Password, "") + require.Len(t, query.Result.Rands, 10) + require.Len(t, query.Result.Salt, 10) + require.False(t, query.Result.IsDisabled) + + ss.Cfg.CaseInsensitiveLogin = false + }) + + t.Run("Get User by login - email fallback case insensitive", func(t *testing.T) { + ss.Cfg.CaseInsensitiveLogin = true + query := models.GetUserByLoginQuery{LoginOrEmail: "USERtest@TEST.COM"} + err = ss.GetUserByLogin(context.Background(), &query) + require.Nil(t, err) + + require.Equal(t, query.Result.Email, "usertest@test.com") + require.Equal(t, query.Result.Password, "") + require.Len(t, query.Result.Rands, 10) + require.Len(t, query.Result.Salt, 10) + require.False(t, query.Result.IsDisabled) + + ss.Cfg.CaseInsensitiveLogin = false + }) }) t.Run("Testing DB - creates and loads disabled user", func(t *testing.T) { @@ -359,6 +473,90 @@ func TestIntegrationUserDataAccess(t *testing.T) { assert.Len(t, query.Result.Users, 2) }) + t.Run("Testing DB - error on case insensitive conflict", func(t *testing.T) { + if ss.engine.Dialect().DBType() == migrator.MySQL { + t.Skip("Skipping on MySQL due to case insensitive indexes") + } + + cmd := models.CreateUserCommand{ + Email: "confusertest@test.com", + Name: "user name", + Login: "user_email_conflict", + } + userEmailConflict, err := ss.CreateUser(context.Background(), cmd) + require.NoError(t, err) + + cmd = models.CreateUserCommand{ + Email: "confusertest@TEST.COM", + Name: "user name", + Login: "user_email_conflict_two", + } + _, err = ss.CreateUser(context.Background(), cmd) + require.NoError(t, err) + + cmd = models.CreateUserCommand{ + Email: "user_test_login_conflict@test.com", + Name: "user name", + Login: "user_test_login_conflict", + } + userLoginConflict, err := ss.CreateUser(context.Background(), cmd) + require.NoError(t, err) + + cmd = models.CreateUserCommand{ + Email: "user_test_login_conflict_two@test.com", + Name: "user name", + Login: "user_test_login_CONFLICT", + } + _, err = ss.CreateUser(context.Background(), cmd) + require.NoError(t, err) + + ss.Cfg.CaseInsensitiveLogin = true + + t.Run("GetUserByEmail - email conflict", func(t *testing.T) { + query := models.GetUserByEmailQuery{Email: "confusertest@test.com"} + err = ss.GetUserByEmail(context.Background(), &query) + require.Error(t, err) + }) + + t.Run("GetUserByEmail - login conflict", func(t *testing.T) { + query := models.GetUserByEmailQuery{Email: "user_test_login_conflict@test.com"} + err = ss.GetUserByEmail(context.Background(), &query) + require.Error(t, err) + }) + + t.Run("GetUserByID - email conflict", func(t *testing.T) { + query := models.GetUserByIdQuery{Id: userEmailConflict.Id} + err = ss.GetUserById(context.Background(), &query) + require.Error(t, err) + }) + + t.Run("GetUserByID - login conflict", func(t *testing.T) { + query := models.GetUserByIdQuery{Id: userLoginConflict.Id} + err = ss.GetUserById(context.Background(), &query) + require.Error(t, err) + }) + + t.Run("GetUserByLogin - email conflict", func(t *testing.T) { + query := models.GetUserByLoginQuery{LoginOrEmail: "user_email_conflict_two"} + err = ss.GetUserByLogin(context.Background(), &query) + require.Error(t, err) + }) + + t.Run("GetUserByLogin - login conflict", func(t *testing.T) { + query := models.GetUserByLoginQuery{LoginOrEmail: "user_test_login_conflict"} + err = ss.GetUserByLogin(context.Background(), &query) + require.Error(t, err) + }) + + t.Run("GetUserByLogin - login conflict by email", func(t *testing.T) { + query := models.GetUserByLoginQuery{LoginOrEmail: "user_test_login_conflict@test.com"} + err = ss.GetUserByLogin(context.Background(), &query) + require.Error(t, err) + }) + + ss.Cfg.CaseInsensitiveLogin = false + }) + ss = InitTestDB(t) t.Run("Testing DB - enable all users", func(t *testing.T) { diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index 222a807b9af..5fc5fe9f1dc 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -360,6 +360,7 @@ type Cfg struct { // User UserInviteMaxLifetime time.Duration HiddenUsers map[string]struct{} + CaseInsensitiveLogin bool // Login and Email will be considered case insensitive // Annotations AnnotationCleanupJobBatchSize int64 @@ -1354,6 +1355,8 @@ func readUserSettings(iniFile *ini.File, cfg *Cfg) error { AutoAssignOrgRole = cfg.AutoAssignOrgRole VerifyEmailEnabled = users.Key("verify_email_enabled").MustBool(false) + cfg.CaseInsensitiveLogin = users.Key("case_insensitive_login").MustBool(false) + LoginHint = valueAsString(users, "login_hint", "") PasswordHint = valueAsString(users, "password_hint", "") cfg.DefaultTheme = valueAsString(users, "default_theme", "")