From 7105bb3be7856961efbea92a901ab2f92bfe2d1b Mon Sep 17 00:00:00 2001 From: Kat Yang <69819079+yangkb09@users.noreply.github.com> Date: Fri, 4 Feb 2022 13:45:42 -0500 Subject: [PATCH] Chore: Remove bus from admin users (#44869) * Chore: Remove bus from admin users * Mock authinfoservice * Update user id * attempt to fix the tests in admin users api * fix type cast * revert skipped tests Co-authored-by: Serge Zaitsev --- pkg/api/admin_users.go | 21 +++-- pkg/api/admin_users_test.go | 92 +++++++++----------- pkg/api/api.go | 6 +- pkg/api/common_test.go | 1 + pkg/services/sqlstore/mockstore/mockstore.go | 7 ++ pkg/services/sqlstore/store.go | 1 + 6 files changed, 65 insertions(+), 63 deletions(-) diff --git a/pkg/api/admin_users.go b/pkg/api/admin_users.go index fef1af41415..c9fa2407533 100644 --- a/pkg/api/admin_users.go +++ b/pkg/api/admin_users.go @@ -8,7 +8,6 @@ import ( "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" - "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/infra/metrics" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/util" @@ -62,7 +61,7 @@ func (hs *HTTPServer) AdminCreateUser(c *models.ReqContext) response.Response { return response.JSON(200, result) } -func AdminUpdateUserPassword(c *models.ReqContext) response.Response { +func (hs *HTTPServer) AdminUpdateUserPassword(c *models.ReqContext) response.Response { form := dtos.AdminUpdateUserPasswordForm{} if err := web.Bind(c.Req, &form); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) @@ -79,7 +78,7 @@ func AdminUpdateUserPassword(c *models.ReqContext) response.Response { userQuery := models.GetUserByIdQuery{Id: userID} - if err := bus.Dispatch(c.Req.Context(), &userQuery); err != nil { + if err := hs.SQLStore.GetUserById(c.Req.Context(), &userQuery); err != nil { return response.Error(500, "Could not read user from database", err) } @@ -93,7 +92,7 @@ func AdminUpdateUserPassword(c *models.ReqContext) response.Response { NewPassword: passwordHashed, } - if err := bus.Dispatch(c.Req.Context(), &cmd); err != nil { + if err := hs.SQLStore.ChangeUserPassword(c.Req.Context(), &cmd); err != nil { return response.Error(500, "Failed to update user password", err) } @@ -123,7 +122,7 @@ func (hs *HTTPServer) AdminUpdateUserPermissions(c *models.ReqContext) response. return response.Success("User permissions updated") } -func AdminDeleteUser(c *models.ReqContext) response.Response { +func (hs *HTTPServer) AdminDeleteUser(c *models.ReqContext) response.Response { userID, err := strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64) if err != nil { return response.Error(http.StatusBadRequest, "id is invalid", err) @@ -131,7 +130,7 @@ func AdminDeleteUser(c *models.ReqContext) response.Response { cmd := models.DeleteUserCommand{UserId: userID} - if err := bus.Dispatch(c.Req.Context(), &cmd); err != nil { + if err := hs.SQLStore.DeleteUser(c.Req.Context(), &cmd); err != nil { if errors.Is(err, models.ErrUserNotFound) { return response.Error(404, models.ErrUserNotFound.Error(), nil) } @@ -150,12 +149,12 @@ func (hs *HTTPServer) AdminDisableUser(c *models.ReqContext) response.Response { // External users shouldn't be disabled from API authInfoQuery := &models.GetAuthInfoQuery{UserId: userID} - if err := bus.Dispatch(c.Req.Context(), authInfoQuery); !errors.Is(err, models.ErrUserNotFound) { + if err := hs.authInfoService.GetAuthInfo(c.Req.Context(), authInfoQuery); !errors.Is(err, models.ErrUserNotFound) { return response.Error(500, "Could not disable external user", nil) } disableCmd := models.DisableUserCommand{UserId: userID, IsDisabled: true} - if err := bus.Dispatch(c.Req.Context(), &disableCmd); err != nil { + if err := hs.SQLStore.DisableUser(c.Req.Context(), &disableCmd); err != nil { if errors.Is(err, models.ErrUserNotFound) { return response.Error(404, models.ErrUserNotFound.Error(), nil) } @@ -171,7 +170,7 @@ func (hs *HTTPServer) AdminDisableUser(c *models.ReqContext) response.Response { } // POST /api/admin/users/:id/enable -func AdminEnableUser(c *models.ReqContext) response.Response { +func (hs *HTTPServer) AdminEnableUser(c *models.ReqContext) response.Response { userID, err := strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64) if err != nil { return response.Error(http.StatusBadRequest, "id is invalid", err) @@ -179,12 +178,12 @@ func AdminEnableUser(c *models.ReqContext) response.Response { // External users shouldn't be disabled from API authInfoQuery := &models.GetAuthInfoQuery{UserId: userID} - if err := bus.Dispatch(c.Req.Context(), authInfoQuery); !errors.Is(err, models.ErrUserNotFound) { + if err := hs.authInfoService.GetAuthInfo(c.Req.Context(), authInfoQuery); !errors.Is(err, models.ErrUserNotFound) { return response.Error(500, "Could not enable external user", nil) } disableCmd := models.DisableUserCommand{UserId: userID, IsDisabled: false} - if err := bus.Dispatch(c.Req.Context(), &disableCmd); err != nil { + if err := hs.SQLStore.DisableUser(c.Req.Context(), &disableCmd); err != nil { if errors.Is(err, models.ErrUserNotFound) { return response.Error(404, models.ErrUserNotFound.Error(), nil) } diff --git a/pkg/api/admin_users_test.go b/pkg/api/admin_users_test.go index 7e3fca4b436..2d95eee079c 100644 --- a/pkg/api/admin_users_test.go +++ b/pkg/api/admin_users_test.go @@ -28,6 +28,20 @@ const ( existingTestLogin = "existing@example.com" ) +type mockAuthInfoService struct { + LatestUserID int64 + ExpectedError error +} + +func (m *mockAuthInfoService) LookupAndUpdate(ctx context.Context, query *models.GetUserByAuthInfoQuery) (*models.User, error) { + m.LatestUserID = query.UserId + return nil, m.ExpectedError +} +func (m *mockAuthInfoService) GetAuthInfo(ctx context.Context, query *models.GetAuthInfoQuery) error { + m.LatestUserID = query.UserId + return m.ExpectedError +} + func TestAdminAPIEndpoint(t *testing.T) { const role = models.ROLE_ADMIN @@ -96,17 +110,9 @@ 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) { - var userID int64 - isDisabled := false - bus.AddHandler("test", func(ctx context.Context, cmd *models.GetAuthInfoQuery) error { - return models.ErrUserNotFound - }) - - bus.AddHandler("test", func(ctx context.Context, cmd *models.DisableUserCommand) error { - userID = cmd.UserId - isDisabled = cmd.IsDisabled - return models.ErrUserNotFound - }) + store := sc.sqlStore.(*mockstore.SQLStoreMock) + sc.authInfoService.ExpectedError = models.ErrUserNotFound + store.ExpectedError = models.ErrUserNotFound sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() @@ -116,23 +122,14 @@ func TestAdminAPIEndpoint(t *testing.T) { assert.Equal(t, "user not found", respJSON.Get("message").MustString()) - assert.Equal(t, int64(42), userID) - assert.Equal(t, false, isDisabled) + 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) { - var userID int64 - isDisabled := false - bus.AddHandler("test", func(ctx context.Context, cmd *models.GetAuthInfoQuery) error { - return models.ErrUserNotFound - }) - - bus.AddHandler("test", func(ctx context.Context, cmd *models.DisableUserCommand) error { - userID = cmd.UserId - isDisabled = cmd.IsDisabled - return models.ErrUserNotFound - }) + store := sc.sqlStore.(*mockstore.SQLStoreMock) + sc.authInfoService.ExpectedError = models.ErrUserNotFound + store.ExpectedError = models.ErrUserNotFound sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() @@ -142,20 +139,13 @@ func TestAdminAPIEndpoint(t *testing.T) { assert.Equal(t, "user not found", respJSON.Get("message").MustString()) - assert.Equal(t, int64(42), userID) - assert.Equal(t, true, isDisabled) + assert.Equal(t, int64(42), store.LatestUserId) }) }) t.Run("When a server admin attempts to disable/enable external user", func(t *testing.T) { adminDisableUserScenario(t, "Should return Could not disable external user error", "disable", "/api/admin/users/42/disable", "/api/admin/users/:id/disable", func(sc *scenarioContext) { - var userID int64 - bus.AddHandler("test", func(ctx context.Context, cmd *models.GetAuthInfoQuery) error { - userID = cmd.UserId - return nil - }) - sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() assert.Equal(t, 500, sc.resp.Code) @@ -163,17 +153,11 @@ func TestAdminAPIEndpoint(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Could not disable external user", respJSON.Get("message").MustString()) - assert.Equal(t, int64(42), userID) + assert.Equal(t, int64(42), sc.authInfoService.LatestUserID) }) adminDisableUserScenario(t, "Should return Could not enable external user error", "enable", "/api/admin/users/42/enable", "/api/admin/users/:id/enable", func(sc *scenarioContext) { - var userID int64 - bus.AddHandler("test", func(ctx context.Context, cmd *models.GetAuthInfoQuery) error { - userID = cmd.UserId - return nil - }) - sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() assert.Equal(t, 500, sc.resp.Code) @@ -181,6 +165,7 @@ func TestAdminAPIEndpoint(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Could not enable external user", respJSON.Get("message").MustString()) + userID := sc.authInfoService.LatestUserID assert.Equal(t, int64(42), userID) }) }) @@ -188,13 +173,10 @@ func TestAdminAPIEndpoint(t *testing.T) { t.Run("When a server admin attempts to delete a nonexistent user", func(t *testing.T) { adminDeleteUserScenario(t, "Should return user not found error", "/api/admin/users/42", "/api/admin/users/:id", func(sc *scenarioContext) { - var userID int64 - bus.AddHandler("test", func(ctx context.Context, cmd *models.DeleteUserCommand) error { - userID = cmd.UserId - return models.ErrUserNotFound - }) - + sc.sqlStore.(*mockstore.SQLStoreMock).ExpectedError = models.ErrUserNotFound + sc.authInfoService.ExpectedError = models.ErrUserNotFound sc.fakeReqWithParams("DELETE", sc.url, map[string]string{}).exec() + userID := sc.sqlStore.(*mockstore.SQLStoreMock).LatestUserId assert.Equal(t, 404, sc.resp.Code) @@ -291,8 +273,9 @@ func putAdminScenario(t *testing.T, desc string, url string, routePattern string t.Cleanup(bus.ClearBusHandlers) hs := &HTTPServer{ - Cfg: setting.NewCfg(), - SQLStore: sqlStore, + Cfg: setting.NewCfg(), + SQLStore: sqlStore, + authInfoService: &mockAuthInfoService{}, } sc := setupScenarioContext(t, url) @@ -405,18 +388,24 @@ func adminDisableUserScenario(t *testing.T, desc string, action string, url stri fakeAuthTokenService := auth.NewFakeUserAuthTokenService() + authInfoService := &mockAuthInfoService{} + hs := HTTPServer{ Bus: bus.GetBus(), + SQLStore: mockstore.NewSQLStoreMock(), AuthTokenService: fakeAuthTokenService, + authInfoService: authInfoService, } sc := setupScenarioContext(t, url) + sc.sqlStore = hs.SQLStore + sc.authInfoService = authInfoService sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response { sc.context = c sc.context.UserId = testUserID if action == "enable" { - return AdminEnableUser(c) + return hs.AdminEnableUser(c) } return hs.AdminDisableUser(c) @@ -429,15 +418,20 @@ func adminDisableUserScenario(t *testing.T, desc string, action string, url stri } func adminDeleteUserScenario(t *testing.T, desc string, url string, routePattern string, fn scenarioFunc) { + hs := HTTPServer{ + SQLStore: mockstore.NewSQLStoreMock(), + } t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) { t.Cleanup(bus.ClearBusHandlers) sc := setupScenarioContext(t, url) + sc.sqlStore = hs.SQLStore + sc.authInfoService = &mockAuthInfoService{} sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response { sc.context = c sc.context.UserId = testUserID - return AdminDeleteUser(c) + return hs.AdminDeleteUser(c) }) sc.m.Delete(routePattern, sc.defaultHandler) diff --git a/pkg/api/api.go b/pkg/api/api.go index 034fb8dd8a2..fc55b46e326 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -488,11 +488,11 @@ func (hs *HTTPServer) registerRoutes() { userIDScope := ac.Scope("global", "users", "id", ac.Parameter(":id")) adminUserRoute.Post("/", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersCreate)), routing.Wrap(hs.AdminCreateUser)) - adminUserRoute.Put("/:id/password", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersPasswordUpdate, userIDScope)), routing.Wrap(AdminUpdateUserPassword)) + adminUserRoute.Put("/:id/password", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersPasswordUpdate, userIDScope)), routing.Wrap(hs.AdminUpdateUserPassword)) adminUserRoute.Put("/:id/permissions", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersPermissionsUpdate, userIDScope)), routing.Wrap(hs.AdminUpdateUserPermissions)) - adminUserRoute.Delete("/:id", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersDelete, userIDScope)), routing.Wrap(AdminDeleteUser)) + adminUserRoute.Delete("/:id", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersDelete, userIDScope)), routing.Wrap(hs.AdminDeleteUser)) adminUserRoute.Post("/:id/disable", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersDisable, userIDScope)), routing.Wrap(hs.AdminDisableUser)) - adminUserRoute.Post("/:id/enable", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersEnable, userIDScope)), routing.Wrap(AdminEnableUser)) + adminUserRoute.Post("/:id/enable", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersEnable, userIDScope)), routing.Wrap(hs.AdminEnableUser)) adminUserRoute.Get("/:id/quotas", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersQuotasList, userIDScope)), routing.Wrap(hs.GetUserQuotas)) adminUserRoute.Put("/:id/quotas/:target", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersQuotasUpdate, userIDScope)), routing.Wrap(hs.UpdateUserQuota)) diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index 0bba9e04d3e..0212a34e748 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -158,6 +158,7 @@ type scenarioContext struct { url string userAuthTokenService *auth.FakeUserAuthTokenService sqlStore sqlstore.Store + authInfoService *mockAuthInfoService } func (sc *scenarioContext) exec() { diff --git a/pkg/services/sqlstore/mockstore/mockstore.go b/pkg/services/sqlstore/mockstore/mockstore.go index 909fb442dbe..cbf396db1ff 100644 --- a/pkg/services/sqlstore/mockstore/mockstore.go +++ b/pkg/services/sqlstore/mockstore/mockstore.go @@ -10,6 +10,7 @@ import ( type SQLStoreMock struct { LastGetAlertsQuery *models.GetAlertsQuery + LatestUserId int64 ExpectedUser *models.User ExpectedDatasource *models.DataSource @@ -156,11 +157,17 @@ func (m *SQLStoreMock) GetSignedInUser(ctx context.Context, query *models.GetSig 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) DeleteUser(ctx context.Context, cmd *models.DeleteUserCommand) error { + m.LatestUserId = cmd.UserId return m.ExpectedError } diff --git a/pkg/services/sqlstore/store.go b/pkg/services/sqlstore/store.go index c1a41110a85..6fd02f8ae98 100644 --- a/pkg/services/sqlstore/store.go +++ b/pkg/services/sqlstore/store.go @@ -41,6 +41,7 @@ type Store interface { GetUserOrgList(ctx context.Context, query *models.GetUserOrgListQuery) error GetSignedInUserWithCacheCtx(ctx context.Context, query *models.GetSignedInUserQuery) error GetSignedInUser(ctx context.Context, query *models.GetSignedInUserQuery) error + DisableUser(ctx context.Context, cmd *models.DisableUserCommand) error BatchDisableUsers(ctx context.Context, cmd *models.BatchDisableUsersCommand) error DeleteUser(ctx context.Context, cmd *models.DeleteUserCommand) error UpdateUserPermissions(userID int64, isAdmin bool) error