Chore: Remove disable user, disable batch users and searchusers methods from store interface (#53717)

* Chore: Remove disable user and searchusers methods from store interface

* Remove disable batch user from sqlstore interface

* Remove sqlstore from search store

* Fix lint

pkg/api/admin_users.go

@@ -228,8 +228,8 @@ func (hs *HTTPServer) AdminDisableUser(c *models.ReqContext) response.Response {
 		return response.Error(500, "Could not disable external user", nil)
 	}
 
-	disableCmd := models.DisableUserCommand{UserId: userID, IsDisabled: true}
+	disableCmd := user.DisableUserCommand{UserID: userID, IsDisabled: true}
-	if err := hs.SQLStore.DisableUser(c.Req.Context(), &disableCmd); err != nil {
+	if err := hs.userService.Disable(c.Req.Context(), &disableCmd); err != nil {
 		if errors.Is(err, user.ErrUserNotFound) {
 			return response.Error(404, user.ErrUserNotFound.Error(), nil)
 		}
@@ -271,8 +271,8 @@ func (hs *HTTPServer) AdminEnableUser(c *models.ReqContext) response.Response {
 		return response.Error(500, "Could not enable external user", nil)
 	}
 
-	disableCmd := models.DisableUserCommand{UserId: userID, IsDisabled: false}
+	disableCmd := user.DisableUserCommand{UserID: userID, IsDisabled: false}
-	if err := hs.SQLStore.DisableUser(c.Req.Context(), &disableCmd); err != nil {
+	if err := hs.userService.Disable(c.Req.Context(), &disableCmd); err != nil {
 		if errors.Is(err, user.ErrUserNotFound) {
 			return response.Error(404, user.ErrUserNotFound.Error(), nil)
 		}

pkg/api/admin_users_test.go

@@ -90,9 +90,10 @@ func TestAdminAPIEndpoint(t *testing.T) {
 	t.Run("When a server admin attempts to enable/disable a nonexistent user", func(t *testing.T) {
 		adminDisableUserScenario(t, "Should return user not found on a POST request", "enable",
 			"/api/admin/users/42/enable", "/api/admin/users/:id/enable", func(sc *scenarioContext) {
-				store := sc.sqlStore.(*mockstore.SQLStoreMock)
+				userService := sc.userService.(*usertest.FakeUserService)
 				sc.authInfoService.ExpectedError = user.ErrUserNotFound
-				store.ExpectedError = user.ErrUserNotFound
+
+				userService.ExpectedError = user.ErrUserNotFound
 
 				sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec()
 
@@ -101,15 +102,13 @@ func TestAdminAPIEndpoint(t *testing.T) {
 				require.NoError(t, err)
 
 				assert.Equal(t, "user not found", respJSON.Get("message").MustString())
-
-				assert.Equal(t, int64(42), store.LatestUserId)
 			})
 
 		adminDisableUserScenario(t, "Should return user not found on a POST request", "disable",
 			"/api/admin/users/42/disable", "/api/admin/users/:id/disable", func(sc *scenarioContext) {
-				store := sc.sqlStore.(*mockstore.SQLStoreMock)
+				userService := sc.userService.(*usertest.FakeUserService)
 				sc.authInfoService.ExpectedError = user.ErrUserNotFound
-				store.ExpectedError = user.ErrUserNotFound
+				userService.ExpectedError = user.ErrUserNotFound
 
 				sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec()
 
@@ -118,8 +117,6 @@ func TestAdminAPIEndpoint(t *testing.T) {
 				require.NoError(t, err)
 
 				assert.Equal(t, "user not found", respJSON.Get("message").MustString())
-
-				assert.Equal(t, int64(42), store.LatestUserId)
 			})
 	})
 
@@ -353,11 +350,13 @@ func adminDisableUserScenario(t *testing.T, desc string, action string, url stri
 			SQLStore:         mockstore.NewSQLStoreMock(),
 			AuthTokenService: fakeAuthTokenService,
 			authInfoService:  authInfoService,
+			userService:      usertest.NewUserServiceFake(),
 		}
 
 		sc := setupScenarioContext(t, url)
 		sc.sqlStore = hs.SQLStore
 		sc.authInfoService = authInfoService
+		sc.userService = hs.userService
 		sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
 			sc.context = c
 			sc.context.UserID = testUserID

pkg/api/common_test.go

@@ -250,7 +250,7 @@ func setupAccessControlScenarioContext(t *testing.T, cfg *setting.Cfg, url strin
 		QuotaService:       &quotaimpl.Service{Cfg: cfg},
 		RouteRegister:      routing.NewRouteRegister(),
 		AccessControl:      accesscontrolmock.New().WithPermissions(permissions),
-		searchUsersService: searchusers.ProvideUsersService(store, filters.ProvideOSSSearchUserFilter()),
+		searchUsersService: searchusers.ProvideUsersService(filters.ProvideOSSSearchUserFilter(), usertest.NewUserServiceFake()),
 		ldapGroups:         ldap.ProvideGroupsService(),
 	}
 
@@ -391,7 +391,7 @@ func setupHTTPServerWithCfgDb(t *testing.T, useFakeAccessControl bool, cfg *sett
 		License:                &licensing.OSSLicensingService{},
 		AccessControl:          ac,
 		teamPermissionsService: teamPermissionService,
-		searchUsersService:     searchusers.ProvideUsersService(db, filters.ProvideOSSSearchUserFilter()),
+		searchUsersService:     searchusers.ProvideUsersService(filters.ProvideOSSSearchUserFilter(), usertest.NewUserServiceFake()),
 		DashboardService: dashboardservice.ProvideDashboardService(
 			cfg, dashboardsStore, nil, features,
 			accesscontrolmock.NewMockedPermissionsService(), accesscontrolmock.NewMockedPermissionsService(), ac,

pkg/api/user_test.go

@@ -39,18 +39,19 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) {
 		AccessControl: acmock.New(),
 	}
 
-	mockResult := models.SearchUserQueryResult{
+	mockResult := user.SearchUserQueryResult{
-		Users: []*models.UserSearchHitDTO{
+		Users: []*user.UserSearchHitDTO{
 			{Name: "user1"},
 			{Name: "user2"},
 		},
 		TotalCount: 2,
 	}
 	mock := mockstore.NewSQLStoreMock()
+	userMock := usertest.NewUserServiceFake()
 	loggedInUserScenario(t, "When calling GET on", "api/users/1", "api/users/:id", func(sc *scenarioContext) {
 		fakeNow := time.Date(2019, 2, 11, 17, 30, 40, 0, time.UTC)
 		secretsService := secretsManager.SetupTestService(t, database.ProvideSecretsStore(sqlStore))
-		authInfoStore := authinfostore.ProvideAuthInfoStore(sqlStore, secretsService, usertest.NewUserServiceFake())
+		authInfoStore := authinfostore.ProvideAuthInfoStore(sqlStore, secretsService, userMock)
 		srv := authinfoservice.ProvideAuthInfoService(
 			&authinfoservice.OSSUserProtectionImpl{},
 			authInfoStore,
@@ -138,9 +139,9 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) {
 	}, mock)
 
 	loggedInUserScenario(t, "When calling GET on", "/api/users", "/api/users", func(sc *scenarioContext) {
-		mock.ExpectedSearchUsers = mockResult
+		userMock.ExpectedSearchUsers = mockResult
 
-		searchUsersService := searchusers.ProvideUsersService(mock, filters.ProvideOSSSearchUserFilter())
+		searchUsersService := searchusers.ProvideUsersService(filters.ProvideOSSSearchUserFilter(), userMock)
 		sc.handlerFunc = searchUsersService.SearchUsers
 		sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec()
 
@@ -151,9 +152,9 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) {
 	}, mock)
 
 	loggedInUserScenario(t, "When calling GET with page and limit querystring parameters on", "/api/users", "/api/users", func(sc *scenarioContext) {
-		mock.ExpectedSearchUsers = mockResult
+		userMock.ExpectedSearchUsers = mockResult
 
-		searchUsersService := searchusers.ProvideUsersService(mock, filters.ProvideOSSSearchUserFilter())
+		searchUsersService := searchusers.ProvideUsersService(filters.ProvideOSSSearchUserFilter(), userMock)
 		sc.handlerFunc = searchUsersService.SearchUsers
 		sc.fakeReqWithParams("GET", sc.url, map[string]string{"perpage": "10", "page": "2"}).exec()
 
@@ -164,9 +165,9 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) {
 	}, mock)
 
 	loggedInUserScenario(t, "When calling GET on", "/api/users/search", "/api/users/search", func(sc *scenarioContext) {
-		mock.ExpectedSearchUsers = mockResult
+		userMock.ExpectedSearchUsers = mockResult
 
-		searchUsersService := searchusers.ProvideUsersService(mock, filters.ProvideOSSSearchUserFilter())
+		searchUsersService := searchusers.ProvideUsersService(filters.ProvideOSSSearchUserFilter(), userMock)
 		sc.handlerFunc = searchUsersService.SearchUsersWithPaging
 		sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec()
 
@@ -180,9 +181,9 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) {
 	}, mock)
 
 	loggedInUserScenario(t, "When calling GET with page and perpage querystring parameters on", "/api/users/search", "/api/users/search", func(sc *scenarioContext) {
-		mock.ExpectedSearchUsers = mockResult
+		userMock.ExpectedSearchUsers = mockResult
 
-		searchUsersService := searchusers.ProvideUsersService(mock, filters.ProvideOSSSearchUserFilter())
+		searchUsersService := searchusers.ProvideUsersService(filters.ProvideOSSSearchUserFilter(), userMock)
 		sc.handlerFunc = searchUsersService.SearchUsersWithPaging
 		sc.fakeReqWithParams("GET", sc.url, map[string]string{"perpage": "10", "page": "2"}).exec()
 

pkg/services/comments/handlers.go

@@ -8,7 +8,6 @@ import (
 	"sort"
 
 	"github.com/grafana/grafana/pkg/api/dtos"
-	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/comments/commentmodel"
 	"github.com/grafana/grafana/pkg/services/user"
 )
@@ -36,12 +35,12 @@ func commentToDto(comment *commentmodel.Comment, userMap map[int64]*commentmodel
 	return comment.ToDTO(u)
 }
 
-func searchUserToCommentUser(searchUser *models.UserSearchHitDTO) *commentmodel.CommentUser {
+func searchUserToCommentUser(searchUser *user.UserSearchHitDTO) *commentmodel.CommentUser {
 	if searchUser == nil {
 		return nil
 	}
 	return &commentmodel.CommentUser{
-		Id:        searchUser.Id,
+		Id:        searchUser.ID,
 		Name:      searchUser.Name,
 		Login:     searchUser.Login,
 		Email:     searchUser.Email,
@@ -122,6 +121,7 @@ func (s *Service) Create(ctx context.Context, orgID int64, signedInUser *user.Si
 }
 
 func (s *Service) Get(ctx context.Context, orgID int64, signedInUser *user.SignedInUser, cmd GetCmd) ([]*commentmodel.CommentDto, error) {
+	var res *user.SearchUserQueryResult
 	ok, err := s.permissions.CheckReadPermissions(ctx, orgID, signedInUser, cmd.ObjectType, cmd.ObjectID)
 	if err != nil {
 		return nil, err
@@ -147,20 +147,20 @@ func (s *Service) Get(ctx context.Context, orgID int64, signedInUser *user.Signe
 	}
 
 	// NOTE: probably replace with comment and user table join.
-	query := &models.SearchUsersQuery{
+	query := &user.SearchUsersQuery{
 		Query:        "",
 		Page:         0,
 		Limit:        len(userIds),
 		SignedInUser: signedInUser,
 		Filters:      []user.Filter{NewIDFilter(userIds)},
 	}
-	if err := s.sqlStore.SearchUsers(ctx, query); err != nil {
+	if res, err = s.userService.Search(ctx, query); err != nil {
 		return nil, err
 	}
 
-	userMap := make(map[int64]*commentmodel.CommentUser, len(query.Result.Users))
+	userMap := make(map[int64]*commentmodel.CommentUser, len(res.Users))
-	for _, v := range query.Result.Users {
+	for _, v := range res.Users {
-		userMap[v.Id] = searchUserToCommentUser(v)
+		userMap[v.ID] = searchUserToCommentUser(v)
 	}
 
 	result := commentsToDto(messages, userMap)

pkg/services/comments/service.go

@@ -9,6 +9,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
 	"github.com/grafana/grafana/pkg/services/live"
 	"github.com/grafana/grafana/pkg/services/sqlstore"
+	"github.com/grafana/grafana/pkg/services/user"
 	"github.com/grafana/grafana/pkg/setting"
 )
 
@@ -18,11 +19,12 @@ type Service struct {
 	sqlStore    *sqlstore.SQLStore
 	storage     Storage
 	permissions *commentmodel.PermissionChecker
+	userService user.Service
 }
 
 func ProvideService(cfg *setting.Cfg, store *sqlstore.SQLStore, live *live.GrafanaLive,
 	features featuremgmt.FeatureToggles, accessControl accesscontrol.AccessControl,
-	dashboardService dashboards.DashboardService) *Service {
+	dashboardService dashboards.DashboardService, userService user.Service) *Service {
 	s := &Service{
 		cfg:      cfg,
 		live:     live,
@@ -31,6 +33,7 @@ func ProvideService(cfg *setting.Cfg, store *sqlstore.SQLStore, live *live.Grafa
 			sql: store,
 		},
 		permissions: commentmodel.NewPermissionChecker(store, features, accessControl, dashboardService),
+		userService: userService,
 	}
 	return s
 }

pkg/services/login/loginservice/loginservice.go

@@ -126,9 +126,9 @@ func (ls *Implementation) UpsertUser(ctx context.Context, cmd *models.UpsertUser
 
 		if extUser.AuthModule == login.LDAPAuthModule && usr.IsDisabled {
 			// Re-enable user when it found in LDAP
-			if errDisableUser := ls.SQLStore.DisableUser(ctx,
+			if errDisableUser := ls.userService.Disable(ctx,
-				&models.DisableUserCommand{
+				&user.DisableUserCommand{
-					UserId: cmd.Result.ID, IsDisabled: false}); errDisableUser != nil {
+					UserID: cmd.Result.ID, IsDisabled: false}); errDisableUser != nil {
 				return errDisableUser
 			}
 		}
@@ -176,12 +176,12 @@ func (ls *Implementation) DisableExternalUser(ctx context.Context, username stri
 	)
 
 	// Mark user as disabled in grafana db
-	disableUserCmd := &models.DisableUserCommand{
+	disableUserCmd := &user.DisableUserCommand{
-		UserId:     userQuery.Result.UserId,
+		UserID:     userQuery.Result.UserId,
 		IsDisabled: true,
 	}
 
-	if err := ls.SQLStore.DisableUser(ctx, disableUserCmd); err != nil {
+	if err := ls.userService.Disable(ctx, disableUserCmd); err != nil {
 		logger.Debug(
 			"Error disabling external user",
 			"user",

pkg/services/searchusers/searchusers.go

@@ -7,7 +7,6 @@ import (
 	"github.com/grafana/grafana/pkg/api/response"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/login"
-	"github.com/grafana/grafana/pkg/services/sqlstore"
 	"github.com/grafana/grafana/pkg/services/user"
 )
 
@@ -17,12 +16,16 @@ type Service interface {
 }
 
 type OSSService struct {
-	sqlStore         sqlstore.Store
 	searchUserFilter user.SearchUserFilter
+	userService      user.Service
 }
 
-func ProvideUsersService(sqlStore sqlstore.Store, searchUserFilter user.SearchUserFilter) *OSSService {
+func ProvideUsersService(searchUserFilter user.SearchUserFilter, userService user.Service,
-	return &OSSService{sqlStore: sqlStore, searchUserFilter: searchUserFilter}
+) *OSSService {
+	return &OSSService{
+		searchUserFilter: searchUserFilter,
+		userService:      userService,
+	}
 }
 
 // swagger:route GET /users users searchUsers
@@ -37,12 +40,12 @@ func ProvideUsersService(sqlStore sqlstore.Store, searchUserFilter user.SearchUs
 // 403: forbiddenError
 // 500: internalServerError
 func (s *OSSService) SearchUsers(c *models.ReqContext) response.Response {
-	query, err := s.SearchUser(c)
+	result, err := s.SearchUser(c)
 	if err != nil {
 		return response.Error(500, "Failed to fetch users", err)
 	}
 
-	return response.JSON(http.StatusOK, query.Result.Users)
+	return response.JSON(http.StatusOK, result.Users)
 }
 
 // swagger:route GET /users/search users searchUsersWithPaging
@@ -56,15 +59,15 @@ func (s *OSSService) SearchUsers(c *models.ReqContext) response.Response {
 // 404: notFoundError
 // 500: internalServerError
 func (s *OSSService) SearchUsersWithPaging(c *models.ReqContext) response.Response {
-	query, err := s.SearchUser(c)
+	result, err := s.SearchUser(c)
 	if err != nil {
 		return response.Error(500, "Failed to fetch users", err)
 	}
 
-	return response.JSON(http.StatusOK, query.Result)
+	return response.JSON(http.StatusOK, result)
 }
 
-func (s *OSSService) SearchUser(c *models.ReqContext) (*models.SearchUsersQuery, error) {
+func (s *OSSService) SearchUser(c *models.ReqContext) (*user.SearchUserQueryResult, error) {
 	perPage := c.QueryInt("perpage")
 	if perPage <= 0 {
 		perPage = 1000
@@ -84,7 +87,7 @@ func (s *OSSService) SearchUser(c *models.ReqContext) (*models.SearchUsersQuery,
 		}
 	}
 
-	query := &models.SearchUsersQuery{
+	query := &user.SearchUsersQuery{
 		// added SignedInUser to the query, as to only list the users that the user has permission to read
 		SignedInUser: c.SignedInUser,
 		Query:        searchQuery,
@@ -92,11 +95,12 @@ func (s *OSSService) SearchUser(c *models.ReqContext) (*models.SearchUsersQuery,
 		Page:         page,
 		Limit:        perPage,
 	}
-	if err := s.sqlStore.SearchUsers(c.Req.Context(), query); err != nil {
+	res, err := s.userService.Search(c.Req.Context(), query)
+	if err != nil {
 		return nil, err
 	}
 
-	for _, user := range query.Result.Users {
+	for _, user := range res.Users {
 		user.AvatarUrl = dtos.GetGravatarUrl(user.Email)
 		user.AuthLabels = make([]string, 0)
 		if user.AuthModule != nil && len(user.AuthModule) > 0 {
@@ -106,8 +110,8 @@ func (s *OSSService) SearchUser(c *models.ReqContext) (*models.SearchUsersQuery,
 		}
 	}
 
-	query.Result.Page = page
+	res.Page = page
-	query.Result.PerPage = perPage
+	res.PerPage = perPage
 
-	return query, nil
+	return res, nil
 }

pkg/services/sqlstore/mockstore/mockstore.go

@@ -156,20 +156,6 @@ func (m *SQLStoreMock) GetSignedInUser(ctx context.Context, query *models.GetSig
 	return m.ExpectedError
 }
 
-func (m *SQLStoreMock) SearchUsers(ctx context.Context, query *models.SearchUsersQuery) error {
-	query.Result = m.ExpectedSearchUsers
-	return m.ExpectedError
-}
-
-func (m *SQLStoreMock) DisableUser(ctx context.Context, cmd *models.DisableUserCommand) error {
-	m.LatestUserId = cmd.UserId
-	return m.ExpectedError
-}
-
-func (m *SQLStoreMock) BatchDisableUsers(ctx context.Context, cmd *models.BatchDisableUsersCommand) error {
-	return m.ExpectedError
-}
-
 func (m *SQLStoreMock) UpdateUserPermissions(userID int64, isAdmin bool) error {
 	return m.ExpectedError
 }

pkg/services/sqlstore/store.go

@@ -35,9 +35,6 @@ type Store interface {
 	GetUserOrgList(ctx context.Context, query *models.GetUserOrgListQuery) error
 	GetSignedInUser(ctx context.Context, query *models.GetSignedInUserQuery) error
 	GetSignedInUserWithCacheCtx(ctx context.Context, query *models.GetSignedInUserQuery) error
-	SearchUsers(ctx context.Context, query *models.SearchUsersQuery) error
-	DisableUser(ctx context.Context, cmd *models.DisableUserCommand) error
-	BatchDisableUsers(ctx context.Context, cmd *models.BatchDisableUsersCommand) error
 	UpdateUserPermissions(userID int64, isAdmin bool) error
 	SetUserHelpFlag(ctx context.Context, cmd *models.SetUserHelpFlagCommand) error
 	CreateTeam(name, email string, orgID int64) (models.Team, error)

pkg/services/user/usertest/fake.go

@@ -11,6 +11,7 @@ type FakeUserService struct {
 	ExpectedSignedInUser     *user.SignedInUser
 	ExpectedError            error
 	ExpectedSetUsingOrgError error
+	ExpectedSearchUsers      user.SearchUserQueryResult
 }
 
 func NewUserServiceFake() *FakeUserService {
@@ -62,7 +63,7 @@ func (f *FakeUserService) GetSignedInUser(ctx context.Context, query *user.GetSi
 }
 
 func (f *FakeUserService) Search(ctx context.Context, query *user.SearchUsersQuery) (*user.SearchUserQueryResult, error) {
-	return &user.SearchUserQueryResult{}, f.ExpectedError
+	return &f.ExpectedSearchUsers, f.ExpectedError
 }
 
 func (f *FakeUserService) Disable(ctx context.Context, cmd *user.DisableUserCommand) error {