From 02702eb82d977ce96ce222bad0f33fdbda1d55af Mon Sep 17 00:00:00 2001 From: Selene Date: Wed, 29 Sep 2021 12:51:49 +0200 Subject: [PATCH] Extract search users functions into a service (#39002) * Extract search users to a new service * Fix wire provider * Fix common_test and remove RouteRegister * Remove old endpoints * Fix test * Add indexes to dashboards and orgs tables * Fix lint --- pkg/api/api.go | 4 +- pkg/api/common_test.go | 16 ++-- pkg/api/http_server.go | 6 +- pkg/api/org_users_test.go | 14 ++- pkg/api/user.go | 55 ----------- pkg/api/user_test.go | 14 ++- pkg/server/wire.go | 1 - pkg/server/wireexts_oss.go | 3 + pkg/services/searchusers/searchusers.go | 95 +++++++++++++++++++ .../sqlstore/migrations/dashboard_acl.go | 8 ++ .../sqlstore/migrations/dashboard_mig.go | 5 + pkg/services/sqlstore/migrations/org_mig.go | 1 + 12 files changed, 148 insertions(+), 74 deletions(-) create mode 100644 pkg/services/searchusers/searchusers.go diff --git a/pkg/api/api.go b/pkg/api/api.go index 4407325af89..291d413c765 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -168,8 +168,8 @@ func (hs *HTTPServer) registerRoutes() { // users (admin permission required) apiRoute.Group("/users", func(usersRoute routing.RouteRegister) { userIDScope := ac.Scope("global", "users", ac.Parameter(":id")) - usersRoute.Get("/", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersRead, ac.ScopeGlobalUsersAll)), routing.Wrap(SearchUsers)) - usersRoute.Get("/search", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersRead, ac.ScopeGlobalUsersAll)), routing.Wrap(SearchUsersWithPaging)) + usersRoute.Get("/", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersRead, ac.ScopeGlobalUsersAll)), routing.Wrap(hs.searchUsersService.SearchUsers)) + usersRoute.Get("/search", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersRead, ac.ScopeGlobalUsersAll)), routing.Wrap(hs.searchUsersService.SearchUsersWithPaging)) usersRoute.Get("/:id", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersRead, userIDScope)), routing.Wrap(GetUserByID)) usersRoute.Get("/:id/teams", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersTeamRead, userIDScope)), routing.Wrap(GetUserTeams)) usersRoute.Get("/:id/orgs", authorize(reqGrafanaAdmin, ac.EvalPermission(ac.ActionUsersRead, userIDScope)), routing.Wrap(GetUserOrgList)) diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index 5fe412601bf..66aa758b9fc 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -8,6 +8,8 @@ import ( "path/filepath" "testing" + "github.com/grafana/grafana/pkg/services/searchusers" + "github.com/stretchr/testify/require" "gopkg.in/macaron.v1" @@ -216,13 +218,15 @@ func setupAccessControlScenarioContext(t *testing.T, cfg *setting.Cfg, url strin cfg.FeatureToggles["accesscontrol"] = true cfg.Quota.Enabled = false + bus := bus.GetBus() hs := &HTTPServer{ - Cfg: cfg, - Bus: bus.GetBus(), - Live: newTestLive(t), - QuotaService: "a.QuotaService{Cfg: cfg}, - RouteRegister: routing.NewRouteRegister(), - AccessControl: accesscontrolmock.New().WithPermissions(permissions), + Cfg: cfg, + Bus: bus, + Live: newTestLive(t), + QuotaService: "a.QuotaService{Cfg: cfg}, + RouteRegister: routing.NewRouteRegister(), + AccessControl: accesscontrolmock.New().WithPermissions(permissions), + searchUsersService: searchusers.ProvideUsersService(bus), } sc := setupScenarioContext(t, url) diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index c4ef2218760..99f8247e228 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -13,6 +13,8 @@ import ( "strings" "sync" + "github.com/grafana/grafana/pkg/services/searchusers" + "github.com/grafana/grafana/pkg/api/routing" httpstatic "github.com/grafana/grafana/pkg/api/static" "github.com/grafana/grafana/pkg/bus" @@ -107,6 +109,7 @@ type HTTPServer struct { cleanUpService *cleanup.CleanUpService tracingService *tracing.TracingService internalMetricsSvc *metrics.InternalMetricsService + searchUsersService searchusers.Service } type ServerOptions struct { @@ -130,7 +133,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi notificationService *notifications.NotificationService, tracingService *tracing.TracingService, internalMetricsSvc *metrics.InternalMetricsService, quotaService *quota.QuotaService, socialService social.Service, oauthTokenService oauthtoken.OAuthTokenService, - encryptionService encryption.Service) (*HTTPServer, error) { + encryptionService encryption.Service, searchUsersService searchusers.Service) (*HTTPServer, error) { macaron.Env = cfg.Env m := macaron.New() @@ -177,6 +180,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi SocialService: socialService, OAuthTokenService: oauthTokenService, EncryptionService: encryptionService, + searchUsersService: searchUsersService, } if hs.Listener != nil { hs.log.Debug("Using provided listener") diff --git a/pkg/api/org_users_test.go b/pkg/api/org_users_test.go index 70172a19242..1228c614f29 100644 --- a/pkg/api/org_users_test.go +++ b/pkg/api/org_users_test.go @@ -8,6 +8,9 @@ import ( "testing" "time" + "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/services/searchusers" + "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/components/simplejson" @@ -137,11 +140,12 @@ func setupOrgUsersAPIcontext(t *testing.T, role models.RoleType) (*scenarioConte db := sqlstore.InitTestDB(t) hs := &HTTPServer{ - Cfg: cfg, - QuotaService: "a.QuotaService{Cfg: cfg}, - RouteRegister: routing.NewRouteRegister(), - AccessControl: accesscontrolmock.New().WithDisabled(), - SQLStore: db, + Cfg: cfg, + QuotaService: "a.QuotaService{Cfg: cfg}, + RouteRegister: routing.NewRouteRegister(), + AccessControl: accesscontrolmock.New().WithDisabled(), + SQLStore: db, + searchUsersService: searchusers.ProvideUsersService(bus.New()), } sc := setupScenarioContext(t, "/api/org/users/lookup") diff --git a/pkg/api/user.go b/pkg/api/user.go index 18c15249df3..9bbe536e6af 100644 --- a/pkg/api/user.go +++ b/pkg/api/user.go @@ -258,61 +258,6 @@ func redirectToChangePassword(c *models.ReqContext) { c.Redirect("/profile/password", 302) } -// GET /api/users -func SearchUsers(c *models.ReqContext) response.Response { - query, err := searchUser(c) - if err != nil { - return response.Error(500, "Failed to fetch users", err) - } - - return response.JSON(200, query.Result.Users) -} - -// GET /api/users/search -func SearchUsersWithPaging(c *models.ReqContext) response.Response { - query, err := searchUser(c) - if err != nil { - return response.Error(500, "Failed to fetch users", err) - } - - return response.JSON(200, query.Result) -} - -func searchUser(c *models.ReqContext) (*models.SearchUsersQuery, error) { - perPage := c.QueryInt("perpage") - if perPage <= 0 { - perPage = 1000 - } - page := c.QueryInt("page") - - if page < 1 { - page = 1 - } - - searchQuery := c.Query("query") - filter := c.Query("filter") - - query := &models.SearchUsersQuery{Query: searchQuery, Filter: models.SearchUsersFilter(filter), Page: page, Limit: perPage} - if err := bus.Dispatch(query); err != nil { - return nil, err - } - - for _, user := range query.Result.Users { - user.AvatarUrl = dtos.GetGravatarUrl(user.Email) - user.AuthLabels = make([]string, 0) - if user.AuthModule != nil && len(user.AuthModule) > 0 { - for _, authModule := range user.AuthModule { - user.AuthLabels = append(user.AuthLabels, GetAuthProviderLabel(authModule)) - } - } - } - - query.Result.Page = page - query.Result.PerPage = perPage - - return query, nil -} - func SetHelpFlag(c *models.ReqContext) response.Response { flag := c.ParamsInt64(":id") diff --git a/pkg/api/user_test.go b/pkg/api/user_test.go index e3e1b9b7d7d..3589defdd98 100644 --- a/pkg/api/user_test.go +++ b/pkg/api/user_test.go @@ -6,6 +6,8 @@ import ( "testing" "time" + "github.com/grafana/grafana/pkg/services/searchusers" + "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/components/simplejson" @@ -134,7 +136,8 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { return nil }) - sc.handlerFunc = SearchUsers + searchUsersService := searchusers.ProvideUsersService(bus.GetBus()) + sc.handlerFunc = searchUsersService.SearchUsers sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() assert.Equal(t, 1000, sentLimit) @@ -157,7 +160,8 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { return nil }) - sc.handlerFunc = SearchUsers + searchUsersService := searchusers.ProvideUsersService(bus.GetBus()) + sc.handlerFunc = searchUsersService.SearchUsers sc.fakeReqWithParams("GET", sc.url, map[string]string{"perpage": "10", "page": "2"}).exec() assert.Equal(t, 10, sentLimit) @@ -176,7 +180,8 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { return nil }) - sc.handlerFunc = SearchUsersWithPaging + searchUsersService := searchusers.ProvideUsersService(bus.GetBus()) + sc.handlerFunc = searchUsersService.SearchUsersWithPaging sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() assert.Equal(t, 1000, sentLimit) @@ -201,7 +206,8 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { return nil }) - sc.handlerFunc = SearchUsersWithPaging + searchUsersService := searchusers.ProvideUsersService(bus.GetBus()) + sc.handlerFunc = searchUsersService.SearchUsersWithPaging sc.fakeReqWithParams("GET", sc.url, map[string]string{"perpage": "10", "page": "2"}).exec() assert.Equal(t, 10, sentLimit) diff --git a/pkg/server/wire.go b/pkg/server/wire.go index 6ee1bfd6ca5..ce13a4d4c3b 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -5,7 +5,6 @@ package server import ( "github.com/google/wire" - sdkhttpclient "github.com/grafana/grafana-plugin-sdk-go/backend/httpclient" "github.com/grafana/grafana/pkg/api" "github.com/grafana/grafana/pkg/api/routing" diff --git a/pkg/server/wireexts_oss.go b/pkg/server/wireexts_oss.go index 97b64317edd..98a0ec229e6 100644 --- a/pkg/server/wireexts_oss.go +++ b/pkg/server/wireexts_oss.go @@ -18,6 +18,7 @@ import ( "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/login/authinfoservice" "github.com/grafana/grafana/pkg/services/provisioning" + "github.com/grafana/grafana/pkg/services/searchusers" "github.com/grafana/grafana/pkg/services/sqlstore/migrations" "github.com/grafana/grafana/pkg/services/validations" "github.com/grafana/grafana/pkg/setting" @@ -48,6 +49,8 @@ var wireExtsBasicSet = wire.NewSet( wire.Bind(new(login.UserProtectionService), new(*authinfoservice.OSSUserProtectionImpl)), ossencryption.ProvideService, wire.Bind(new(encryption.Service), new(*ossencryption.Service)), + searchusers.ProvideUsersService, + wire.Bind(new(searchusers.Service), new(*searchusers.OSSService)), ) var wireExtsSet = wire.NewSet( diff --git a/pkg/services/searchusers/searchusers.go b/pkg/services/searchusers/searchusers.go new file mode 100644 index 00000000000..173dd96d189 --- /dev/null +++ b/pkg/services/searchusers/searchusers.go @@ -0,0 +1,95 @@ +package searchusers + +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/models" +) + +type Service interface { + SearchUsers(c *models.ReqContext) response.Response + SearchUsersWithPaging(c *models.ReqContext) response.Response +} + +type OSSService struct { + bus bus.Bus +} + +func ProvideUsersService(bus bus.Bus) *OSSService { + return &OSSService{bus: bus} +} + +func (s *OSSService) SearchUsers(c *models.ReqContext) response.Response { + query, err := s.SearchUser(c) + if err != nil { + return response.Error(500, "Failed to fetch users", err) + } + + return response.JSON(200, query.Result.Users) +} + +func (s *OSSService) SearchUsersWithPaging(c *models.ReqContext) response.Response { + query, err := s.SearchUser(c) + if err != nil { + return response.Error(500, "Failed to fetch users", err) + } + + return response.JSON(200, query.Result) +} + +func (s *OSSService) SearchUser(c *models.ReqContext) (*models.SearchUsersQuery, error) { + perPage := c.QueryInt("perpage") + if perPage <= 0 { + perPage = 1000 + } + page := c.QueryInt("page") + + if page < 1 { + page = 1 + } + + searchQuery := c.Query("query") + filter := c.Query("filter") + + query := &models.SearchUsersQuery{Query: searchQuery, Filter: models.SearchUsersFilter(filter), Page: page, Limit: perPage} + if err := s.bus.Dispatch(query); err != nil { + return nil, err + } + + for _, user := range query.Result.Users { + user.AvatarUrl = dtos.GetGravatarUrl(user.Email) + user.AuthLabels = make([]string, 0) + if user.AuthModule != nil && len(user.AuthModule) > 0 { + for _, authModule := range user.AuthModule { + user.AuthLabels = append(user.AuthLabels, GetAuthProviderLabel(authModule)) + } + } + } + + query.Result.Page = page + query.Result.PerPage = perPage + + return query, nil +} + +func GetAuthProviderLabel(authModule string) string { + switch authModule { + case "oauth_github": + return "GitHub" + case "oauth_google": + return "Google" + case "oauth_azuread": + return "AzureAD" + case "oauth_gitlab": + return "GitLab" + case "oauth_grafana_com", "oauth_grafananet": + return "grafana.com" + case "auth.saml": + return "SAML" + case "ldap", "": + return "LDAP" + default: + return "OAuth" + } +} diff --git a/pkg/services/sqlstore/migrations/dashboard_acl.go b/pkg/services/sqlstore/migrations/dashboard_acl.go index 566a7cc1a3b..a7fff287f54 100644 --- a/pkg/services/sqlstore/migrations/dashboard_acl.go +++ b/pkg/services/sqlstore/migrations/dashboard_acl.go @@ -20,6 +20,10 @@ func addDashboardAclMigrations(mg *Migrator) { {Cols: []string{"dashboard_id"}}, {Cols: []string{"dashboard_id", "user_id"}, Type: UniqueIndex}, {Cols: []string{"dashboard_id", "team_id"}, Type: UniqueIndex}, + {Cols: []string{"user_id"}}, + {Cols: []string{"team_id"}}, + {Cols: []string{"org_id", "role"}}, + {Cols: []string{"permission"}}, }, } @@ -29,6 +33,10 @@ func addDashboardAclMigrations(mg *Migrator) { mg.AddMigration("add index dashboard_acl_dashboard_id", NewAddIndexMigration(dashboardAclV1, dashboardAclV1.Indices[0])) mg.AddMigration("add unique index dashboard_acl_dashboard_id_user_id", NewAddIndexMigration(dashboardAclV1, dashboardAclV1.Indices[1])) mg.AddMigration("add unique index dashboard_acl_dashboard_id_team_id", NewAddIndexMigration(dashboardAclV1, dashboardAclV1.Indices[2])) + mg.AddMigration("add index dashboard_acl_user_id", NewAddIndexMigration(dashboardAclV1, dashboardAclV1.Indices[3])) + mg.AddMigration("add index dashboard_acl_team_id", NewAddIndexMigration(dashboardAclV1, dashboardAclV1.Indices[4])) + mg.AddMigration("add index dashboard_acl_org_id_role", NewAddIndexMigration(dashboardAclV1, dashboardAclV1.Indices[5])) + mg.AddMigration("add index dashboard_permission", NewAddIndexMigration(dashboardAclV1, dashboardAclV1.Indices[6])) const rawSQL = ` INSERT INTO dashboard_acl diff --git a/pkg/services/sqlstore/migrations/dashboard_mig.go b/pkg/services/sqlstore/migrations/dashboard_mig.go index eb93d969160..94a4dc358e5 100644 --- a/pkg/services/sqlstore/migrations/dashboard_mig.go +++ b/pkg/services/sqlstore/migrations/dashboard_mig.go @@ -225,4 +225,9 @@ func addDashboardMigration(mg *Migrator) { mg.AddMigration("delete stars for deleted dashboards", NewRawSQLMigration( "DELETE FROM star WHERE dashboard_id NOT IN (SELECT id FROM dashboard)")) + + mg.AddMigration("Add index for dashboard_is_folder", NewAddIndexMigration(dashboardV2, &Index{ + Cols: []string{"is_folder"}, + Type: IndexType, + })) } diff --git a/pkg/services/sqlstore/migrations/org_mig.go b/pkg/services/sqlstore/migrations/org_mig.go index ee9b7becaca..bb357bfd422 100644 --- a/pkg/services/sqlstore/migrations/org_mig.go +++ b/pkg/services/sqlstore/migrations/org_mig.go @@ -41,6 +41,7 @@ func addOrgMigrations(mg *Migrator) { Indices: []*Index{ {Cols: []string{"org_id"}}, {Cols: []string{"org_id", "user_id"}, Type: UniqueIndex}, + {Cols: []string{"user_id"}}, }, }