apitech
/
grafana
mirror of https://github.com/grafana/grafana
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Chore: Remove bus from admin users (#44869)

Browse Source 
* 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 <serge.zaitsev@grafana.com>
pull/44928/head
Kat Yang 4 years ago committed by GitHub
parent ddfe2dce74
commit 7105bb3be7
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 65 additions and 63 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 21
      pkg/api/admin_users.go
  2. 92
      pkg/api/admin_users_test.go
  3. 6
      pkg/api/api.go
  4. 1
      pkg/api/common_test.go
  5. 7
      pkg/services/sqlstore/mockstore/mockstore.go
  6. 1
      pkg/services/sqlstore/store.go

21
pkg/api/admin_users.go
Unescape View File

@ -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)
}

92
pkg/api/admin_users_test.go
Unescape View File

@ -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)

6
pkg/api/api.go
Unescape View File

@ -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))

1
pkg/api/common_test.go
Unescape View File

@ -158,6 +158,7 @@ type scenarioContext struct {
url string
userAuthTokenService *auth.FakeUserAuthTokenService
sqlStore sqlstore.Store
authInfoService *mockAuthInfoService
}
func (sc *scenarioContext) exec() {

7
pkg/services/sqlstore/mockstore/mockstore.go
Unescape View File

@ -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
}

1
pkg/services/sqlstore/store.go
Unescape View File

@ -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

Write Preview
Loading…
Cancel
Save