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

Chore: Remove GetUserByEmail and GetUserByLogin from sqlstore (#55903)

Browse Source 
* Chore: Remove GetUserByEmail and GetUserByLogin from sqlstore
 Rename GetUserProfile to GetProfile

* Fix lint

* Skip test for mysql

* Add missing method to sqlstore mock
This commit is contained in:
idafurjes
2022-09-28 13:18:19 +02:00
committed by GitHub
parent a8f43b97a2
commit a45ef61d25
14 changed files with 279 additions and 261 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/user/user.go
+1 -1
View File
@@ -21,5 +21,5 @@ type Service interface {
BatchDisableUsers(context.Context, *BatchDisableUsersCommand) error
UpdatePermissions(int64, bool) error
SetUserHelpFlag(context.Context, *SetUserHelpFlagCommand) error
GetUserProfile(context.Context, *GetUserProfileQuery) (UserProfileDTO, error)
GetProfile(context.Context, *GetUserProfileQuery) (UserProfileDTO, error)
}
pkg/services/user/userimpl/store.go
+101
View File
@@ -3,6 +3,7 @@ package userimpl
import (
"context"
"fmt"
"strings"
"github.com/grafana/grafana/pkg/events"
"github.com/grafana/grafana/pkg/infra/log"
@@ -20,6 +21,8 @@ type store interface {
GetNotServiceAccount(context.Context, int64) (*user.User, error)
Delete(context.Context, int64) error
CaseInsensitiveLoginConflict(context.Context, string, string) error
GetByLogin(context.Context, *user.GetUserByLoginQuery) (*user.User, error)
GetByEmail(context.Context, *user.GetUserByEmailQuery) (*user.User, error)
}
type sqlStore struct {
@@ -145,3 +148,101 @@ func (ss *sqlStore) CaseInsensitiveLoginConflict(ctx context.Context, login, ema
})
return err
}
func (ss *sqlStore) GetByLogin(ctx context.Context, query *user.GetUserByLoginQuery) (*user.User, error) {
usr := &user.User{}
err := ss.db.WithDbSession(ctx, func(sess *sqlstore.DBSession) error {
if query.LoginOrEmail == "" {
return user.ErrUserNotFound
}
// Try and find the user by login first.
// It's not sufficient to assume that a LoginOrEmail with an "@" is an email.
where := "login=?"
if ss.cfg.CaseInsensitiveLogin {
where = "LOWER(login)=LOWER(?)"
}
has, err := sess.Where(ss.notServiceAccountFilter()).Where(where, query.LoginOrEmail).Get(usr)
if err != nil {
return err
}
if !has && strings.Contains(query.LoginOrEmail, "@") {
// If the user wasn't found, and it contains an "@" fallback to finding the
// user by email.
where = "email=?"
if ss.cfg.CaseInsensitiveLogin {
where = "LOWER(email)=LOWER(?)"
}
usr = &user.User{}
has, err = sess.Where(ss.notServiceAccountFilter()).Where(where, query.LoginOrEmail).Get(usr)
}
if err != nil {
return err
} else if !has {
return user.ErrUserNotFound
}
if ss.cfg.CaseInsensitiveLogin {
if err := ss.userCaseInsensitiveLoginConflict(ctx, sess, usr.Login, usr.Email); err != nil {
return err
}
}
return nil
})
if err != nil {
return nil, err
}
return usr, nil
}
func (ss *sqlStore) GetByEmail(ctx context.Context, query *user.GetUserByEmailQuery) (*user.User, error) {
usr := &user.User{}
err := ss.db.WithDbSession(ctx, func(sess *sqlstore.DBSession) error {
if query.Email == "" {
return user.ErrUserNotFound
}
where := "email=?"
if ss.cfg.CaseInsensitiveLogin {
where = "LOWER(email)=LOWER(?)"
}
has, err := sess.Where(ss.notServiceAccountFilter()).Where(where, query.Email).Get(usr)
if err != nil {
return err
} else if !has {
return user.ErrUserNotFound
}
if ss.cfg.CaseInsensitiveLogin {
if err := ss.userCaseInsensitiveLoginConflict(ctx, sess, usr.Login, usr.Email); err != nil {
return err
}
}
return nil
})
if err != nil {
return nil, err
}
return usr, nil
}
func (ss *sqlStore) userCaseInsensitiveLoginConflict(ctx context.Context, sess *sqlstore.DBSession, 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
}
pkg/services/user/userimpl/store_test.go
+140
View File
@@ -8,6 +8,7 @@ import (
"github.com/stretchr/testify/require"
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/sqlstore/migrator"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/setting"
)
@@ -54,4 +55,143 @@ func TestIntegrationUserDataAccess(t *testing.T) {
)
require.NoError(t, err)
})
t.Run("Testing DB - creates and loads user", func(t *testing.T) {
ss := sqlstore.InitTestDB(t)
cmd := user.CreateUserCommand{
Email: "usertest@test.com",
Name: "user name",
Login: "user_test_login",
}
usr, err := ss.CreateUser(context.Background(), cmd)
require.NoError(t, err)
result, err := userStore.GetByID(context.Background(), usr.ID)
require.Nil(t, err)
require.Equal(t, result.Email, "usertest@test.com")
require.Equal(t, result.Password, "")
require.Len(t, result.Rands, 10)
require.Len(t, result.Salt, 10)
require.False(t, result.IsDisabled)
result, err = userStore.GetByID(context.Background(), usr.ID)
require.Nil(t, err)
require.Equal(t, result.Email, "usertest@test.com")
require.Equal(t, result.Password, "")
require.Len(t, result.Rands, 10)
require.Len(t, result.Salt, 10)
require.False(t, result.IsDisabled)
t.Run("Get User by email case insensitive", func(t *testing.T) {
userStore.cfg.CaseInsensitiveLogin = true
query := user.GetUserByEmailQuery{Email: "USERtest@TEST.COM"}
result, err := userStore.GetByEmail(context.Background(), &query)
require.Nil(t, err)
require.Equal(t, result.Email, "usertest@test.com")
require.Equal(t, result.Password, "")
require.Len(t, result.Rands, 10)
require.Len(t, result.Salt, 10)
require.False(t, result.IsDisabled)
userStore.cfg.CaseInsensitiveLogin = false
})
t.Run("Testing DB - creates and loads user", func(t *testing.T) {
result, err = userStore.GetByID(context.Background(), usr.ID)
require.Nil(t, err)
require.Equal(t, result.Email, "usertest@test.com")
require.Equal(t, result.Password, "")
require.Len(t, result.Rands, 10)
require.Len(t, result.Salt, 10)
require.False(t, result.IsDisabled)
result, err = userStore.GetByID(context.Background(), usr.ID)
require.Nil(t, err)
require.Equal(t, result.Email, "usertest@test.com")
require.Equal(t, result.Password, "")
require.Len(t, result.Rands, 10)
require.Len(t, result.Salt, 10)
require.False(t, result.IsDisabled)
ss.Cfg.CaseInsensitiveLogin = false
})
})
t.Run("Testing DB - error on case insensitive conflict", func(t *testing.T) {
if ss.GetDBType() == migrator.MySQL {
t.Skip("Skipping on MySQL due to case insensitive indexes")
}
userStore.cfg.CaseInsensitiveLogin = true
cmd := user.CreateUserCommand{
Email: "confusertest@test.com",
Name: "user name",
Login: "user_email_conflict",
}
// userEmailConflict
_, err := ss.CreateUser(context.Background(), cmd)
require.NoError(t, err)
cmd = user.CreateUserCommand{
Email: "confusertest@TEST.COM",
Name: "user name",
Login: "user_email_conflict_two",
}
_, err = ss.CreateUser(context.Background(), cmd)
require.NoError(t, err)
cmd = user.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 = user.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("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)
})
ss.Cfg.CaseInsensitiveLogin = false
})
}
pkg/services/user/userimpl/user.go
+3 -15
View File
@@ -152,24 +152,12 @@ func (s *Service) GetByID(ctx context.Context, query *user.GetUserByIDQuery) (*u
return user, nil
}
// TODO: remove wrapper around sqlstore
func (s *Service) GetByLogin(ctx context.Context, query *user.GetUserByLoginQuery) (*user.User, error) {
q := models.GetUserByLoginQuery{LoginOrEmail: query.LoginOrEmail}
err := s.sqlStore.GetUserByLogin(ctx, &q)
if err != nil {
return nil, err
}
return q.Result, nil
return s.store.GetByLogin(ctx, query)
}
// TODO: remove wrapper around sqlstore
func (s *Service) GetByEmail(ctx context.Context, query *user.GetUserByEmailQuery) (*user.User, error) {
q := models.GetUserByEmailQuery{Email: query.Email}
err := s.sqlStore.GetUserByEmail(ctx, &q)
if err != nil {
return nil, err
}
return q.Result, nil
return s.store.GetByEmail(ctx, query)
}
// TODO: remove wrapper around sqlstore
@@ -313,7 +301,7 @@ func (s *Service) SetUserHelpFlag(ctx context.Context, cmd *user.SetUserHelpFlag
}
// TODO: remove wrapper around sqlstore
func (s *Service) GetUserProfile(ctx context.Context, query *user.GetUserProfileQuery) (user.UserProfileDTO, error) {
func (s *Service) GetProfile(ctx context.Context, query *user.GetUserProfileQuery) (user.UserProfileDTO, error) {
q := &models.GetUserProfileQuery{
UserId: query.UserID,
}
pkg/services/user/userimpl/user_test.go
+17
View File
@@ -2,6 +2,7 @@ package userimpl
import (
"context"
"errors"
"testing"
"github.com/grafana/grafana/pkg/services/org/orgtest"
@@ -77,6 +78,14 @@ func TestUserService(t *testing.T) {
err := userService.Delete(context.Background(), &user.DeleteUserCommand{UserID: 1})
require.NoError(t, err)
})
t.Run("GetByID - email conflict", func(t *testing.T) {
userService.cfg.CaseInsensitiveLogin = true
userStore.ExpectedError = errors.New("email conflict")
query := user.GetUserByIDQuery{}
_, err := userService.GetByID(context.Background(), &query)
require.Error(t, err)
})
}
type FakeUserStore struct {
@@ -112,3 +121,11 @@ func (f *FakeUserStore) GetByID(context.Context, int64) (*user.User, error) {
func (f *FakeUserStore) CaseInsensitiveLoginConflict(context.Context, string, string) error {
return f.ExpectedError
}
func (f *FakeUserStore) GetByLogin(ctx context.Context, query *user.GetUserByLoginQuery) (*user.User, error) {
return f.ExpectedUser, f.ExpectedError
}
func (f *FakeUserStore) GetByEmail(ctx context.Context, query *user.GetUserByEmailQuery) (*user.User, error) {
return f.ExpectedUser, f.ExpectedError
}
pkg/services/user/usertest/fake.go
+1 -1
View File
@@ -91,6 +91,6 @@ func (f *FakeUserService) SetUserHelpFlag(ctx context.Context, cmd *user.SetUser
return f.ExpectedError
}
func (f *FakeUserService) GetUserProfile(ctx context.Context, query *user.GetUserProfileQuery) (user.UserProfileDTO, error) {
func (f *FakeUserService) GetProfile(ctx context.Context, query *user.GetUserProfileQuery) (user.UserProfileDTO, error) {
return f.ExpectedUSerProfileDTO, f.ExpectedError
}