diff --git a/pkg/api/admin_users.go b/pkg/api/admin_users.go index 11cf6564b40..fd77a184804 100644 --- a/pkg/api/admin_users.go +++ b/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} - if err := hs.SQLStore.DisableUser(c.Req.Context(), &disableCmd); err != nil { + disableCmd := user.DisableUserCommand{UserID: userID, IsDisabled: true} + 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} - if err := hs.SQLStore.DisableUser(c.Req.Context(), &disableCmd); err != nil { + disableCmd := user.DisableUserCommand{UserID: userID, IsDisabled: false} + 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) } diff --git a/pkg/api/admin_users_test.go b/pkg/api/admin_users_test.go index 8285ac64f16..c1e394d72e8 100644 --- a/pkg/api/admin_users_test.go +++ b/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 diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index 7f7cf594164..8f03d614ffd 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -250,7 +250,7 @@ func setupAccessControlScenarioContext(t *testing.T, cfg *setting.Cfg, url strin QuotaService: "aimpl.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, diff --git a/pkg/api/user_test.go b/pkg/api/user_test.go index 042ca44b266..d19cd4841be 100644 --- a/pkg/api/user_test.go +++ b/pkg/api/user_test.go @@ -39,18 +39,19 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { AccessControl: acmock.New(), } - mockResult := models.SearchUserQueryResult{ - Users: []*models.UserSearchHitDTO{ + mockResult := user.SearchUserQueryResult{ + 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() diff --git a/pkg/services/comments/handlers.go b/pkg/services/comments/handlers.go index f8bd54fa1b1..da07d3db206 100644 --- a/pkg/services/comments/handlers.go +++ b/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)) - for _, v := range query.Result.Users { - userMap[v.Id] = searchUserToCommentUser(v) + userMap := make(map[int64]*commentmodel.CommentUser, len(res.Users)) + for _, v := range res.Users { + userMap[v.ID] = searchUserToCommentUser(v) } result := commentsToDto(messages, userMap) diff --git a/pkg/services/comments/service.go b/pkg/services/comments/service.go index 085da02c120..682681be948 100644 --- a/pkg/services/comments/service.go +++ b/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 } diff --git a/pkg/services/login/loginservice/loginservice.go b/pkg/services/login/loginservice/loginservice.go index 19f2fab039a..8c91c2dda8b 100644 --- a/pkg/services/login/loginservice/loginservice.go +++ b/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, - &models.DisableUserCommand{ - UserId: cmd.Result.ID, IsDisabled: false}); errDisableUser != nil { + if errDisableUser := ls.userService.Disable(ctx, + &user.DisableUserCommand{ + 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{ - UserId: userQuery.Result.UserId, + disableUserCmd := &user.DisableUserCommand{ + 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", diff --git a/pkg/services/searchusers/searchusers.go b/pkg/services/searchusers/searchusers.go index 38c4223da28..fbf994536af 100644 --- a/pkg/services/searchusers/searchusers.go +++ b/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 { - return &OSSService{sqlStore: sqlStore, searchUserFilter: searchUserFilter} +func ProvideUsersService(searchUserFilter user.SearchUserFilter, userService user.Service, +) *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 - query.Result.PerPage = perPage + res.Page = page + res.PerPage = perPage - return query, nil + return res, nil } diff --git a/pkg/services/sqlstore/mockstore/mockstore.go b/pkg/services/sqlstore/mockstore/mockstore.go index 6430919130c..dd32cca0e31 100644 --- a/pkg/services/sqlstore/mockstore/mockstore.go +++ b/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 } diff --git a/pkg/services/sqlstore/store.go b/pkg/services/sqlstore/store.go index 2c52b634310..e28f6877c3f 100644 --- a/pkg/services/sqlstore/store.go +++ b/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) diff --git a/pkg/services/user/usertest/fake.go b/pkg/services/user/usertest/fake.go index 4c9964d6858..d14313fca63 100644 --- a/pkg/services/user/usertest/fake.go +++ b/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 {