Chore: Split get user by ID (#52442)

* Remove user from preferences, stars, orguser, team member * Fix lint * Add Delete user from org and dashboard acl * Delete user from user auth * Add DeleteUser to quota * Add test files and adjust user auth store * Rename package in wire for user auth * Import Quota Service interface in other services * do the same in tests * fix lint tests * Fix tests * Add some tests * Rename InsertUser and DeleteUser to InsertOrgUser and DeleteOrgUser * Rename DeleteUser to DeleteByUser in quota * changing a method name in few additional places * Fix in other places * Fix lint * Fix tests * Chore: Split Delete User method * Add fakes for userauth * Add mock for access control Delete User permossion, use interface * Use interface for ream guardian * Add simple fake for dashboard acl * Add go routines, clean up, use interfaces * fix lint * Update pkg/services/user/userimpl/user_test.go Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> * Update pkg/services/user/userimpl/user_test.go Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> * Update pkg/services/user/userimpl/user_test.go Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> * Split get user by ID * Use new method in api * Add tests * Aplly emthod in auth info service * Fix lint and some tests * Fix get user by ID * Fix lint Remove unused fakes * Use split get user id in admin users * Use GetbyID in cli commands * Clean up after merge * Remove commented out code * Clena up imports * add back ) * Fix wire generation for runner after merge with main Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com>
2022-08-02 16:58:05 +02:00
parent 64488f6b90
commit fab6c38c95
28 changed files with 1182 additions and 1522 deletions
@@ -14,8 +14,10 @@ import (
 type store interface {
 	Insert(context.Context, *user.User) (int64, error)
 	Get(context.Context, *user.User) (*user.User, error)
+	GetByID(context.Context, int64) (*user.User, error)
 	GetNotServiceAccount(context.Context, int64) (*user.User, error)
 	Delete(context.Context, int64) error
+	CaseInsensitiveLoginConflict(context.Context, string, string) error
 }

 type sqlStore struct {
@@ -91,8 +93,42 @@ func (ss *sqlStore) GetNotServiceAccount(ctx context.Context, userID int64) (*us
 	return &usr, err
 }

+func (ss *sqlStore) GetByID(ctx context.Context, userID int64) (*user.User, error) {
+	var usr user.User
+
+	err := ss.db.WithDbSession(ctx, func(sess *sqlstore.DBSession) error {
+		has, err := sess.ID(&userID).
+			Where(ss.notServiceAccountFilter()).
+			Get(&usr)
+
+		if err != nil {
+			return err
+		} else if !has {
+			return user.ErrUserNotFound
+		}
+		return nil
+	})
+	return &usr, err
+}
+
 func (ss *sqlStore) notServiceAccountFilter() string {
 	return fmt.Sprintf("%s.is_service_account = %s",
 		ss.dialect.Quote("user"),
 		ss.dialect.BooleanStr(false))
 }
+
+func (ss *sqlStore) CaseInsensitiveLoginConflict(ctx context.Context, login, email string) error {
+	users := make([]user.User, 0)
+	err := ss.db.WithDbSession(ctx, func(sess *sqlstore.DBSession) error {
+		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
+	})
+	return err
+}
@@ -3,7 +3,6 @@ package userimpl
 import (
 	"context"
 	"errors"
-	"fmt"
 	"time"

 	"github.com/grafana/grafana/pkg/services/accesscontrol"
@@ -32,6 +31,8 @@ type Service struct {
 	userAuthService    userauth.Service
 	quotaService       quota.Service
 	accessControlStore accesscontrol.AccessControl
+
+	cfg *setting.Cfg
 }

 func ProvideService(
@@ -44,6 +45,7 @@ func ProvideService(
 	userAuthService userauth.Service,
 	quotaService quota.Service,
 	accessControlStore accesscontrol.AccessControl,
+	cfg *setting.Cfg,
 ) user.Service {
 	return &Service{
 		store: &sqlStore{
@@ -58,6 +60,7 @@ func ProvideService(
 		userAuthService:    userAuthService,
 		quotaService:       quotaService,
 		accessControlStore: accessControlStore,
+		cfg:                cfg,
 	}
 }

@@ -157,7 +160,7 @@ func (s *Service) Create(ctx context.Context, cmd *user.CreateUserCommand) (*use
 func (s *Service) Delete(ctx context.Context, cmd *user.DeleteUserCommand) error {
 	_, err := s.store.GetNotServiceAccount(ctx, cmd.UserID)
 	if err != nil {
-		return fmt.Errorf("failed to get user with not service account: %w", err)
+		return err
 	}
 	// delete from all the stores
 	if err := s.store.Delete(ctx, cmd.UserID); err != nil {
@@ -225,3 +228,16 @@ func (s *Service) Delete(ctx context.Context, cmd *user.DeleteUserCommand) error

 	return nil
 }
+
+func (s *Service) GetByID(ctx context.Context, query *user.GetUserByIDQuery) (*user.User, error) {
+	user, err := s.store.GetByID(ctx, query.ID)
+	if err != nil {
+		return nil, err
+	}
+	if s.cfg.CaseInsensitiveLogin {
+		if err := s.store.CaseInsensitiveLoginConflict(ctx, user.Login, user.Email); err != nil {
+			return nil, err
+		}
+	}
+	return user, nil
+}
@@ -14,6 +14,8 @@ import (
 	"github.com/grafana/grafana/pkg/services/teamguardian/manager"
 	"github.com/grafana/grafana/pkg/services/user"
 	"github.com/grafana/grafana/pkg/services/userauth/userauthtest"
+	"github.com/grafana/grafana/pkg/setting"
+
 	"github.com/stretchr/testify/require"
 )

@@ -40,7 +42,67 @@ func TestUserService(t *testing.T) {
 	}

 	t.Run("create user", func(t *testing.T) {
-		_, err := userService.Create(context.Background(), &user.CreateUserCommand{})
+		_, err := userService.Create(context.Background(), &user.CreateUserCommand{
+			Email: "email",
+			Login: "login",
+			Name:  "name",
+		})
+		require.NoError(t, err)
+	})
+
+	t.Run("get user by ID", func(t *testing.T) {
+		userService.cfg = setting.NewCfg()
+		userService.cfg.CaseInsensitiveLogin = false
+		userStore.ExpectedUser = &user.User{ID: 1, Email: "email", Login: "login", Name: "name"}
+		u, err := userService.GetByID(context.Background(), &user.GetUserByIDQuery{ID: 1})
+		require.NoError(t, err)
+		require.Equal(t, "login", u.Login)
+		require.Equal(t, "name", u.Name)
+		require.Equal(t, "email", u.Email)
+	})
+
+	t.Run("get user by ID with case insensitive login", func(t *testing.T) {
+		userService.cfg = setting.NewCfg()
+		userService.cfg.CaseInsensitiveLogin = true
+		userStore.ExpectedUser = &user.User{ID: 1, Email: "email", Login: "login", Name: "name"}
+		u, err := userService.GetByID(context.Background(), &user.GetUserByIDQuery{ID: 1})
+		require.NoError(t, err)
+		require.Equal(t, "login", u.Login)
+		require.Equal(t, "name", u.Name)
+		require.Equal(t, "email", u.Email)
+	})
+
+	t.Run("delete user store returns error", func(t *testing.T) {
+		userStore.ExpectedDeleteUserError = user.ErrUserNotFound
+		t.Cleanup(func() {
+			userStore.ExpectedDeleteUserError = nil
+		})
+		err := userService.Delete(context.Background(), &user.DeleteUserCommand{UserID: 1})
+		require.Error(t, err, user.ErrUserNotFound)
+	})
+
+	t.Run("delete user returns from team", func(t *testing.T) {
+		teamMemberService.ExpectedError = errors.New("some error")
+		t.Cleanup(func() {
+			teamMemberService.ExpectedError = nil
+		})
+		err := userService.Delete(context.Background(), &user.DeleteUserCommand{UserID: 1})
+		require.Error(t, err)
+	})
+
+	t.Run("delete user returns from team and pref", func(t *testing.T) {
+		teamMemberService.ExpectedError = errors.New("some error")
+		preferenceService.ExpectedError = errors.New("some error 2")
+		t.Cleanup(func() {
+			teamMemberService.ExpectedError = nil
+			preferenceService.ExpectedError = nil
+		})
+		err := userService.Delete(context.Background(), &user.DeleteUserCommand{UserID: 1})
+		require.Error(t, err)
+	})
+
+	t.Run("delete user successfully", func(t *testing.T) {
+		err := userService.Delete(context.Background(), &user.DeleteUserCommand{UserID: 1})
 		require.NoError(t, err)
 	})

@@ -104,3 +166,11 @@ func (f *FakeUserStore) Delete(ctx context.Context, userID int64) error {
 func (f *FakeUserStore) GetNotServiceAccount(ctx context.Context, userID int64) (*user.User, error) {
 	return f.ExpectedUser, f.ExpectedError
 }
+
+func (f *FakeUserStore) GetByID(context.Context, int64) (*user.User, error) {
+	return f.ExpectedUser, f.ExpectedError
+}
+
+func (f *FakeUserStore) CaseInsensitiveLoginConflict(context.Context, string, string) error {
+	return f.ExpectedError
+}