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

UserService: use the UserService instead of calling sqlstore directly (#55745)

Browse Source 
* UserService: update callers to use the UserService instead of calling sqlstore directly

There is one major change hiding in this PR. UserService.Delete originally called a number of services to delete user-related records. I moved everything except the actual call to the user table, and moved those into the API. This was done to avoid dependencies cycles; many of our services depend on the user service, so the user service itself should have as few dependencies as possible.
This commit is contained in:
Kristin Laemmert
2022-09-27 07:58:49 -04:00
committed by GitHub
parent 4f6c2d35c2
commit 701f6d5436
33 changed files with 277 additions and 360 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/api/admin_users.go
+63 -1
View File
@@ -6,10 +6,13 @@ import (
"net/http"
"strconv"
"golang.org/x/sync/errgroup"
"github.com/grafana/grafana/pkg/api/dtos"
"github.com/grafana/grafana/pkg/api/response"
"github.com/grafana/grafana/pkg/infra/metrics"
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/util"
"github.com/grafana/grafana/pkg/web"
@@ -156,7 +159,7 @@ func (hs *HTTPServer) AdminUpdateUserPermissions(c *models.ReqContext) response.
return response.Error(http.StatusBadRequest, "id is invalid", err)
}
err = hs.SQLStore.UpdateUserPermissions(userID, form.IsGrafanaAdmin)
err = hs.userService.UpdatePermissions(userID, form.IsGrafanaAdmin)
if err != nil {
if errors.Is(err, user.ErrLastGrafanaAdmin) {
return response.Error(400, user.ErrLastGrafanaAdmin.Error(), nil)
@@ -198,6 +201,65 @@ func (hs *HTTPServer) AdminDeleteUser(c *models.ReqContext) response.Response {
return response.Error(500, "Failed to delete user", err)
}
g, ctx := errgroup.WithContext(c.Req.Context())
g.Go(func() error {
if err := hs.starService.DeleteByUser(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := hs.orgService.DeleteUserFromAll(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := hs.DashboardService.DeleteACLByUser(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := hs.preferenceService.DeleteByUser(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := hs.teamGuardian.DeleteByUser(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := hs.userAuthService.Delete(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := hs.userAuthService.DeleteToken(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := hs.QuotaService.DeleteByUser(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := hs.accesscontrolService.DeleteUserPermissions(ctx, accesscontrol.GlobalOrgID, cmd.UserID); err != nil {
return err
}
return nil
})
if err := g.Wait(); err != nil {
return response.Error(500, "Failed to delete user", err)
}
return response.Success("User deleted")
}
pkg/api/admin_users_test.go
+4 -5
View File
@@ -37,14 +37,12 @@ func TestAdminAPIEndpoint(t *testing.T) {
updateCmd := dtos.AdminUpdateUserPermissionsForm{
IsGrafanaAdmin: false,
}
mock := &mockstore.SQLStoreMock{
ExpectedError: user.ErrLastGrafanaAdmin,
}
userService := usertest.FakeUserService{ExpectedError: user.ErrLastGrafanaAdmin}
putAdminScenario(t, "When calling PUT on", "/api/admin/users/1/permissions",
"/api/admin/users/:id/permissions", role, updateCmd, func(sc *scenarioContext) {
sc.fakeReqWithParams("PUT", sc.url, map[string]string{}).exec()
assert.Equal(t, 400, sc.resp.Code)
}, mock)
}, nil, &userService)
})
t.Run("When a server admin attempts to logout himself from all devices", func(t *testing.T) {
@@ -235,12 +233,13 @@ func TestAdminAPIEndpoint(t *testing.T) {
}
func putAdminScenario(t *testing.T, desc string, url string, routePattern string, role org.RoleType,
cmd dtos.AdminUpdateUserPermissionsForm, fn scenarioFunc, sqlStore sqlstore.Store) {
cmd dtos.AdminUpdateUserPermissionsForm, fn scenarioFunc, sqlStore sqlstore.Store, userSvc user.Service) {
t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) {
hs := &HTTPServer{
Cfg: setting.NewCfg(),
SQLStore: sqlStore,
authInfoService: &logintest.AuthInfoServiceFake{},
userService: userSvc,
}
sc := setupScenarioContext(t, url)
pkg/api/common_test.go
+10 -9
View File
@@ -56,6 +56,7 @@ import (
"github.com/grafana/grafana/pkg/services/team/teamimpl"
"github.com/grafana/grafana/pkg/services/team/teamtest"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/services/user/userimpl"
"github.com/grafana/grafana/pkg/services/user/usertest"
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/web"
@@ -209,7 +210,7 @@ func getContextHandler(t *testing.T, cfg *setting.Cfg) *contexthandler.ContextHa
renderSvc := &fakeRenderService{}
authJWTSvc := models.NewFakeJWTService()
tracer := tracing.InitializeTracerForTest()
authProxy := authproxy.ProvideAuthProxy(cfg, remoteCacheSvc, loginservice.LoginServiceMock{}, sqlStore)
authProxy := authproxy.ProvideAuthProxy(cfg, remoteCacheSvc, loginservice.LoginServiceMock{}, &usertest.FakeUserService{}, sqlStore)
loginService := &logintest.LoginServiceFake{}
authenticator := &logintest.AuthenticatorFake{}
ctxHdlr := contexthandler.ProvideService(cfg, userAuthTokenSvc, authJWTSvc, remoteCacheSvc, renderSvc, sqlStore, tracer, authProxy, loginService, nil, authenticator, usertest.NewUserServiceFake())
@@ -292,8 +293,6 @@ type accessControlScenarioContext struct {
// acmock is an accesscontrol mock used to fake users rights.
acmock *accesscontrolmock.Mock
usermock *usertest.FakeUserService
// db is a test database initialized with InitTestDB
db *sqlstore.SQLStore
@@ -302,6 +301,7 @@ type accessControlScenarioContext struct {
dashboardsStore dashboards.Store
teamService team.Service
userService user.Service
}
func setAccessControlPermissions(acmock *accesscontrolmock.Mock, perms []accesscontrol.Permission, org int64) {
@@ -378,6 +378,8 @@ func setupHTTPServerWithCfgDb(
var ac accesscontrol.AccessControl
var acService accesscontrol.Service
var userSvc user.Service
// Defining the accesscontrol service has to be done before registering routes
if useFakeAccessControl {
acmock = accesscontrolmock.New()
@@ -386,19 +388,18 @@ func setupHTTPServerWithCfgDb(
}
ac = acmock
acService = acmock
userSvc = &usertest.FakeUserService{}
} else {
var err error
acService, err = acimpl.ProvideService(cfg, db, routeRegister, localcache.ProvideService())
require.NoError(t, err)
ac = acimpl.ProvideAccessControl(cfg)
userSvc = userimpl.ProvideService(db, nil, cfg, db)
}
teamPermissionService, err := ossaccesscontrol.ProvideTeamPermissions(cfg, routeRegister, db, ac, license, acService, teamService)
teamPermissionService, err := ossaccesscontrol.ProvideTeamPermissions(cfg, routeRegister, db, ac, license, acService, teamService, userSvc)
require.NoError(t, err)
// Create minimal HTTP Server
userMock := usertest.NewUserServiceFake()
userMock.ExpectedUser = &user.User{ID: 1}
hs := &HTTPServer{
Cfg: cfg,
Features: features,
@@ -416,7 +417,7 @@ func setupHTTPServerWithCfgDb(
accesscontrolmock.NewMockedPermissionsService(), accesscontrolmock.NewMockedPermissionsService(), ac,
),
preferenceService: preftest.NewPreferenceServiceFake(),
userService: userMock,
userService: userSvc,
orgService: orgtest.NewOrgServiceFake(),
teamService: teamService,
annotationsRepo: annotationstest.NewFakeAnnotationsRepo(),
@@ -455,7 +456,7 @@ func setupHTTPServerWithCfgDb(
cfg: cfg,
dashboardsStore: dashboardsStore,
teamService: teamService,
usermock: userMock,
userService: userSvc,
}
}
pkg/api/http_server.go
+5 -1
View File
@@ -16,6 +16,7 @@ import (
"github.com/grafana/grafana/pkg/bus"
"github.com/grafana/grafana/pkg/middleware/csrf"
"github.com/grafana/grafana/pkg/services/searchV2"
"github.com/grafana/grafana/pkg/services/userauth"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promhttp"
@@ -199,6 +200,7 @@ type HTTPServer struct {
accesscontrolService accesscontrol.Service
annotationsRepo annotations.Repository
tagService tag.Service
userAuthService userauth.Service
}
type ServerOptions struct {
@@ -240,6 +242,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi
loginAttemptService loginAttempt.Service, orgService org.Service, teamService team.Service,
accesscontrolService accesscontrol.Service, dashboardThumbsService dashboardThumbs.Service, navTreeService navtree.Service,
annotationRepo annotations.Repository, tagService tag.Service, searchv2HTTPService searchV2.SearchHTTPService,
userAuthService userauth.Service,
) (*HTTPServer, error) {
web.Env = cfg.Env
m := web.New()
@@ -340,6 +343,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi
accesscontrolService: accesscontrolService,
annotationsRepo: annotationRepo,
tagService: tagService,
userAuthService: userAuthService,
}
if hs.Listener != nil {
hs.log.Debug("Using provided listener")
@@ -592,7 +596,7 @@ func (hs *HTTPServer) addMiddlewaresAndStaticRoutes() {
m.Use(hs.frontendLogEndpoints())
m.UseMiddleware(hs.ContextHandler.Middleware)
m.Use(middleware.OrgRedirect(hs.Cfg, hs.SQLStore))
m.Use(middleware.OrgRedirect(hs.Cfg, hs.userService))
m.Use(accesscontrol.LoadPermissionsMiddleware(hs.accesscontrolService))
// needs to be after context handler
pkg/api/org_invite.go
+1 -1
View File
@@ -302,7 +302,7 @@ func (hs *HTTPServer) applyUserInvite(ctx context.Context, usr *user.User, invit
if setActive {
// set org to active
if err := hs.SQLStore.SetUsingOrg(ctx, &models.SetUsingOrgCommand{OrgId: invite.OrgId, UserId: usr.ID}); err != nil {
if err := hs.userService.SetUsingOrg(ctx, &user.SetUsingOrgCommand{OrgID: invite.OrgId, UserID: usr.ID}); err != nil {
return false, response.Error(500, "Failed to set org as active", err)
}
}
pkg/api/org_test.go
+2 -1
View File
@@ -12,6 +12,7 @@ import (
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/org/orgimpl"
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/user/usertest"
"github.com/grafana/grafana/pkg/setting"
)
@@ -198,7 +199,7 @@ func setupOrgsDBForAccessControlTests(t *testing.T, db sqlstore.Store, c accessC
setInitCtxSignedInViewer(c.initCtx)
u := *c.initCtx.SignedInUser
u.OrgID = orgID
c.usermock.ExpectedSignedInUser = &u
c.userService.(*usertest.FakeUserService).ExpectedSignedInUser = &u
// Create `orgsCount` orgs
for i := 1; i <= int(orgID); i++ {
pkg/api/org_users_test.go
+12 -17
View File
@@ -388,8 +388,7 @@ func TestGetOrgUsersAPIEndpoint_AccessControlMetadata(t *testing.T) {
cfg.RBACEnabled = tc.enableAccessControl
sc := setupHTTPServerWithCfg(t, false, cfg, func(hs *HTTPServer) {
hs.userService = userimpl.ProvideService(
hs.SQLStore, nil, nil, nil, nil,
nil, nil, nil, nil, nil, hs.SQLStore.(*sqlstore.SQLStore),
hs.SQLStore, nil, cfg, hs.SQLStore.(*sqlstore.SQLStore),
)
hs.orgService = orgimpl.ProvideService(hs.SQLStore, cfg)
})
@@ -493,8 +492,7 @@ func TestGetOrgUsersAPIEndpoint_AccessControl(t *testing.T) {
cfg.RBACEnabled = tc.enableAccessControl
sc := setupHTTPServerWithCfg(t, false, cfg, func(hs *HTTPServer) {
hs.userService = userimpl.ProvideService(
hs.SQLStore, nil, nil, nil, nil,
nil, nil, nil, nil, nil, hs.SQLStore.(*sqlstore.SQLStore),
hs.SQLStore, nil, cfg, hs.SQLStore.(*sqlstore.SQLStore),
)
hs.orgService = orgimpl.ProvideService(hs.SQLStore, cfg)
})
@@ -599,10 +597,10 @@ func TestPostOrgUsersAPIEndpoint_AccessControl(t *testing.T) {
cfg.RBACEnabled = tc.enableAccessControl
sc := setupHTTPServerWithCfg(t, false, cfg, func(hs *HTTPServer) {
hs.userService = userimpl.ProvideService(
hs.SQLStore, nil, nil, nil, nil,
nil, nil, nil, nil, nil, hs.SQLStore.(*sqlstore.SQLStore),
hs.SQLStore, nil, cfg, hs.SQLStore.(*sqlstore.SQLStore),
)
})
setupOrgUsersDBForAccessControlTests(t, sc.db)
setInitCtxSignedInUser(sc.initCtx, tc.user)
@@ -718,8 +716,7 @@ func TestOrgUsersAPIEndpointWithSetPerms_AccessControl(t *testing.T) {
sc := setupHTTPServer(t, true, func(hs *HTTPServer) {
hs.tempUserService = tempuserimpl.ProvideService(hs.SQLStore)
hs.userService = userimpl.ProvideService(
hs.SQLStore, nil, nil, nil, nil,
nil, nil, nil, nil, nil, hs.SQLStore.(*sqlstore.SQLStore),
hs.SQLStore, nil, nil, hs.SQLStore.(*sqlstore.SQLStore),
)
})
setInitCtxSignedInViewer(sc.initCtx)
@@ -837,8 +834,7 @@ func TestPatchOrgUsersAPIEndpoint_AccessControl(t *testing.T) {
cfg.RBACEnabled = tc.enableAccessControl
sc := setupHTTPServerWithCfg(t, false, cfg, func(hs *HTTPServer) {
hs.userService = userimpl.ProvideService(
hs.SQLStore, nil, nil, nil, nil,
nil, nil, nil, nil, nil, hs.SQLStore.(*sqlstore.SQLStore),
hs.SQLStore, nil, cfg, hs.SQLStore.(*sqlstore.SQLStore),
)
hs.orgService = orgimpl.ProvideService(hs.SQLStore, cfg)
})
@@ -858,13 +854,13 @@ func TestPatchOrgUsersAPIEndpoint_AccessControl(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, tc.expectedMessage, message)
getUserQuery := models.GetSignedInUserQuery{
UserId: tc.targetUserId,
OrgId: tc.targetOrg,
getUserQuery := user.GetSignedInUserQuery{
UserID: tc.targetUserId,
OrgID: tc.targetOrg,
}
err = sc.db.GetSignedInUser(context.Background(), &getUserQuery)
usr, err := sc.userService.GetSignedInUser(context.Background(), &getUserQuery)
require.NoError(t, err)
assert.Equal(t, tc.expectedUserRole, getUserQuery.Result.OrgRole)
assert.Equal(t, tc.expectedUserRole, usr.OrgRole)
}
})
}
@@ -965,8 +961,7 @@ func TestDeleteOrgUsersAPIEndpoint_AccessControl(t *testing.T) {
cfg.RBACEnabled = tc.enableAccessControl
sc := setupHTTPServerWithCfg(t, false, cfg, func(hs *HTTPServer) {
hs.userService = userimpl.ProvideService(
hs.SQLStore, nil, nil, nil, nil,
nil, nil, nil, nil, nil, hs.SQLStore.(*sqlstore.SQLStore),
hs.SQLStore, nil, cfg, hs.SQLStore.(*sqlstore.SQLStore),
)
hs.orgService = orgimpl.ProvideService(hs.SQLStore, cfg)
})
pkg/api/user.go
+21 -20
View File
@@ -49,9 +49,10 @@ func (hs *HTTPServer) GetUserByID(c *models.ReqContext) response.Response {
}
func (hs *HTTPServer) getUserUserProfile(c *models.ReqContext, userID int64) response.Response {
query := models.GetUserProfileQuery{UserId: userID}
query := user.GetUserProfileQuery{UserID: userID}
if err := hs.SQLStore.GetUserProfile(c.Req.Context(), &query); err != nil {
userProfile, err := hs.userService.GetUserProfile(c.Req.Context(), &query)
if err != nil {
if errors.Is(err, user.ErrUserNotFound) {
return response.Error(404, user.ErrUserNotFound.Error(), nil)
}
@@ -59,17 +60,17 @@ func (hs *HTTPServer) getUserUserProfile(c *models.ReqContext, userID int64) res
}
getAuthQuery := models.GetAuthInfoQuery{UserId: userID}
query.Result.AuthLabels = []string{}
userProfile.AuthLabels = []string{}
if err := hs.authInfoService.GetAuthInfo(c.Req.Context(), &getAuthQuery); err == nil {
authLabel := login.GetAuthProviderLabel(getAuthQuery.Result.AuthModule)
query.Result.AuthLabels = append(query.Result.AuthLabels, authLabel)
query.Result.IsExternal = true
userProfile.AuthLabels = append(userProfile.AuthLabels, authLabel)
userProfile.IsExternal = true
}
query.Result.AccessControl = hs.getAccessControlMetadata(c, c.OrgID, "global.users:id:", strconv.FormatInt(userID, 10))
query.Result.AvatarUrl = dtos.GetGravatarUrl(query.Result.Email)
userProfile.AccessControl = hs.getAccessControlMetadata(c, c.OrgID, "global.users:id:", strconv.FormatInt(userID, 10))
userProfile.AvatarUrl = dtos.GetGravatarUrl(userProfile.Email)
return response.JSON(http.StatusOK, query.Result)
return response.JSON(http.StatusOK, userProfile)
}
// swagger:route GET /users/lookup users getUserByLoginOrEmail
@@ -171,9 +172,9 @@ func (hs *HTTPServer) UpdateUserActiveOrg(c *models.ReqContext) response.Respons
return response.Error(401, "Not a valid organization", nil)
}
cmd := models.SetUsingOrgCommand{UserId: userID, OrgId: orgID}
cmd := user.SetUsingOrgCommand{UserID: userID, OrgID: orgID}
if err := hs.SQLStore.SetUsingOrg(c.Req.Context(), &cmd); err != nil {
if err := hs.userService.SetUsingOrg(c.Req.Context(), &cmd); err != nil {
return response.Error(500, "Failed to change active organization", err)
}
@@ -334,9 +335,9 @@ func (hs *HTTPServer) UserSetUsingOrg(c *models.ReqContext) response.Response {
return response.Error(401, "Not a valid organization", nil)
}
cmd := models.SetUsingOrgCommand{UserId: c.UserID, OrgId: orgID}
cmd := user.SetUsingOrgCommand{UserID: c.UserID, OrgID: orgID}
if err := hs.SQLStore.SetUsingOrg(c.Req.Context(), &cmd); err != nil {
if err := hs.userService.SetUsingOrg(c.Req.Context(), &cmd); err != nil {
return response.Error(500, "Failed to change active organization", err)
}
@@ -355,9 +356,9 @@ func (hs *HTTPServer) ChangeActiveOrgAndRedirectToHome(c *models.ReqContext) {
hs.NotFoundHandler(c)
}
cmd := models.SetUsingOrgCommand{UserId: c.UserID, OrgId: orgID}
cmd := user.SetUsingOrgCommand{UserID: c.UserID, OrgID: orgID}
if err := hs.SQLStore.SetUsingOrg(c.Req.Context(), &cmd); err != nil {
if err := hs.userService.SetUsingOrg(c.Req.Context(), &cmd); err != nil {
hs.NotFoundHandler(c)
}
@@ -449,12 +450,12 @@ func (hs *HTTPServer) SetHelpFlag(c *models.ReqContext) response.Response {
bitmask := &c.HelpFlags1
bitmask.AddFlag(user.HelpFlags1(flag))
cmd := models.SetUserHelpFlagCommand{
UserId: c.UserID,
cmd := user.SetUserHelpFlagCommand{
UserID: c.UserID,
HelpFlags1: *bitmask,
}
if err := hs.SQLStore.SetUserHelpFlag(c.Req.Context(), &cmd); err != nil {
if err := hs.userService.SetUserHelpFlag(c.Req.Context(), &cmd); err != nil {
return response.Error(500, "Failed to update help flag", err)
}
@@ -471,12 +472,12 @@ func (hs *HTTPServer) SetHelpFlag(c *models.ReqContext) response.Response {
// 403: forbiddenError
// 500: internalServerError
func (hs *HTTPServer) ClearHelpFlags(c *models.ReqContext) response.Response {
cmd := models.SetUserHelpFlagCommand{
UserId: c.UserID,
cmd := user.SetUserHelpFlagCommand{
UserID: c.UserID,
HelpFlags1: user.HelpFlags1(0),
}
if err := hs.SQLStore.SetUserHelpFlag(c.Req.Context(), &cmd); err != nil {
if err := hs.userService.SetUserHelpFlag(c.Req.Context(), &cmd); err != nil {
return response.Error(500, "Failed to update help flag", err)
}
pkg/api/user_test.go
+2
View File
@@ -26,6 +26,7 @@ import (
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/services/user/userimpl"
"github.com/grafana/grafana/pkg/services/user/usertest"
"github.com/grafana/grafana/pkg/setting"
)
@@ -67,6 +68,7 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) {
}
user, err := sqlStore.CreateUser(context.Background(), createUserCmd)
require.Nil(t, err)
hs.userService = userimpl.ProvideService(sqlStore, nil, sc.cfg, sqlStore)
sc.handlerFunc = hs.GetUserByID