Skip to content

Commit

Permalink
Chore: Switch over to team.Service instead of sqlstore (grafana#55497)
Browse files Browse the repository at this point in the history
* switch to using team service

* trying to fix tests

* more tests to fix

* add missing teamtest package
  • Loading branch information
zserge authored Sep 20, 2022
1 parent 40bc140 commit 305d494
Show file tree
Hide file tree
Showing 25 changed files with 192 additions and 94 deletions.
5 changes: 3 additions & 2 deletions pkg/api/annotations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
"github.com/grafana/grafana/pkg/services/team/teamtest"
)

func TestAnnotationsAPIEndpoint(t *testing.T) {
Expand Down Expand Up @@ -922,7 +923,7 @@ func setUpACL() {
viewerRole := org.RoleViewer
editorRole := org.RoleEditor
store := mockstore.NewSQLStoreMock()
store.ExpectedTeamsByUser = []*models.TeamDTO{}
teamSvc := &teamtest.FakeService{}
dashSvc := &dashboards.FakeDashboardService{}
dashSvc.On("GetDashboardACLInfoList", mock.Anything, mock.AnythingOfType("*models.GetDashboardACLInfoListQuery")).Run(func(args mock.Arguments) {
q := args.Get(1).(*models.GetDashboardACLInfoListQuery)
Expand All @@ -932,7 +933,7 @@ func setUpACL() {
}
}).Return(nil)

guardian.InitLegacyGuardian(store, dashSvc)
guardian.InitLegacyGuardian(store, dashSvc, teamSvc)
}

func setUpRBACGuardian(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (hs *HTTPServer) registerRoutes() {
reqGrafanaAdmin := middleware.ReqGrafanaAdmin
reqEditorRole := middleware.ReqEditorRole
reqOrgAdmin := middleware.ReqOrgAdmin
reqOrgAdminDashOrFolderAdminOrTeamAdmin := middleware.OrgAdminDashOrFolderAdminOrTeamAdmin(hs.SQLStore, hs.DashboardService)
reqOrgAdminDashOrFolderAdminOrTeamAdmin := middleware.OrgAdminDashOrFolderAdminOrTeamAdmin(hs.SQLStore, hs.DashboardService, hs.teamService)
reqCanAccessTeams := middleware.AdminOrEditorAndFeatureEnabled(hs.Cfg.EditorsCanAdmin)
reqSnapshotPublicModeOrSignedIn := middleware.SnapshotPublicModeOrSignedIn(hs.Cfg)
redirectFromLegacyPanelEditURL := middleware.RedirectFromLegacyPanelEditURL(hs.Cfg)
Expand Down
10 changes: 9 additions & 1 deletion pkg/api/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ import (
"github.com/grafana/grafana/pkg/services/searchusers/filters"
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
"github.com/grafana/grafana/pkg/services/team"
"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/usertest"
"github.com/grafana/grafana/pkg/setting"
Expand Down Expand Up @@ -297,6 +300,7 @@ type accessControlScenarioContext struct {
cfg *setting.Cfg

dashboardsStore dashboards.Store
teamService team.Service
}

func setAccessControlPermissions(acmock *accesscontrolmock.Mock, perms []accesscontrol.Permission, org int64) {
Expand Down Expand Up @@ -366,6 +370,7 @@ func setupHTTPServerWithCfgDb(

license := &licensing.OSSLicensingService{}
routeRegister := routing.NewRouteRegister()
teamService := teamimpl.ProvideService(db)
dashboardsStore := dashboardsstore.ProvideDashboardStore(db, featuremgmt.WithFeatures())

var acmock *accesscontrolmock.Mock
Expand Down Expand Up @@ -412,6 +417,7 @@ func setupHTTPServerWithCfgDb(
preferenceService: preftest.NewPreferenceServiceFake(),
userService: userMock,
orgService: orgtest.NewOrgServiceFake(),
teamService: teamService,
annotationsRepo: annotationstest.NewFakeAnnotationsRepo(),
}

Expand Down Expand Up @@ -447,6 +453,7 @@ func setupHTTPServerWithCfgDb(
db: db,
cfg: cfg,
dashboardsStore: dashboardsStore,
teamService: teamService,
usermock: userMock,
}
}
Expand Down Expand Up @@ -524,11 +531,12 @@ func setUp(confs ...setUpConf) *HTTPServer {
}
}
store.ExpectedTeamsByUser = []*models.TeamDTO{}
teamSvc := &teamtest.FakeService{}
dashSvc := &dashboards.FakeDashboardService{}
dashSvc.On("GetDashboardACLInfoList", mock.Anything, mock.AnythingOfType("*models.GetDashboardACLInfoListQuery")).Run(func(args mock.Arguments) {
q := args.Get(1).(*models.GetDashboardACLInfoListQuery)
q.Result = aclMockResp
}).Return(nil)
guardian.InitLegacyGuardian(store, dashSvc)
guardian.InitLegacyGuardian(store, dashSvc, teamSvc)
return hs
}
7 changes: 5 additions & 2 deletions pkg/api/dashboard_snapshot_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/grafana/grafana/pkg/services/guardian"
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
"github.com/grafana/grafana/pkg/services/team/teamtest"
)

func TestDashboardSnapshotAPIEndpoint_singleSnapshot(t *testing.T) {
Expand Down Expand Up @@ -70,10 +71,11 @@ func TestDashboardSnapshotAPIEndpoint_singleSnapshot(t *testing.T) {
hs := &HTTPServer{dashboardsnapshotsService: setUpSnapshotTest(t, 0, "")}
sc.handlerFunc = hs.DeleteDashboardSnapshot

teamSvc := &teamtest.FakeService{}
dashSvc := dashboards.NewFakeDashboardService(t)
dashSvc.On("GetDashboardACLInfoList", mock.Anything, mock.AnythingOfType("*models.GetDashboardACLInfoListQuery")).Return(nil).Maybe()

guardian.InitLegacyGuardian(sc.sqlStore, dashSvc)
guardian.InitLegacyGuardian(sc.sqlStore, dashSvc, teamSvc)
sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec()

assert.Equal(t, 403, sc.resp.Code)
Expand Down Expand Up @@ -107,6 +109,7 @@ func TestDashboardSnapshotAPIEndpoint_singleSnapshot(t *testing.T) {
})

t.Run("When user is editor and dashboard has default ACL", func(t *testing.T) {
teamSvc := &teamtest.FakeService{}
dashSvc := &dashboards.FakeDashboardService{}
dashSvc.On("GetDashboardACLInfoList", mock.Anything, mock.AnythingOfType("*models.GetDashboardACLInfoListQuery")).Run(func(args mock.Arguments) {
q := args.Get(1).(*models.GetDashboardACLInfoListQuery)
Expand All @@ -118,7 +121,7 @@ func TestDashboardSnapshotAPIEndpoint_singleSnapshot(t *testing.T) {

loggedInUserScenarioWithRole(t, "Should be able to delete a snapshot when calling DELETE on", "DELETE",
"/api/snapshots/12345", "/api/snapshots/:key", org.RoleEditor, func(sc *scenarioContext) {
guardian.InitLegacyGuardian(sc.sqlStore, dashSvc)
guardian.InitLegacyGuardian(sc.sqlStore, dashSvc, teamSvc)
var externalRequest *http.Request
ts := setupRemoteServer(func(rw http.ResponseWriter, req *http.Request) {
rw.WriteHeader(200)
Expand Down
21 changes: 13 additions & 8 deletions pkg/api/dashboard_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
"github.com/grafana/grafana/pkg/services/quota/quotaimpl"
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
"github.com/grafana/grafana/pkg/services/team/teamtest"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/web"
Expand Down Expand Up @@ -129,6 +130,7 @@ func TestDashboardAPIEndpoint(t *testing.T) {
fakeDash.HasACL = false
fakeDashboardVersionService := dashvertest.NewDashboardVersionServiceFake()
fakeDashboardVersionService.ExpectedDashboardVersion = &dashver.DashboardVersion{}
teamService := &teamtest.FakeService{}
dashboardService := dashboards.NewFakeDashboardService(t)
dashboardService.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) {
q := args.Get(1).(*models.GetDashboardQuery)
Expand Down Expand Up @@ -157,7 +159,7 @@ func TestDashboardAPIEndpoint(t *testing.T) {
{Role: &editorRole, Permission: models.PERMISSION_EDIT},
}
}).Return(nil)
guardian.InitLegacyGuardian(mockSQLStore, dashboardService)
guardian.InitLegacyGuardian(mockSQLStore, dashboardService, teamService)
}

// This tests two scenarios:
Expand Down Expand Up @@ -235,6 +237,7 @@ func TestDashboardAPIEndpoint(t *testing.T) {
fakeDash.HasACL = true
fakeDashboardVersionService := dashvertest.NewDashboardVersionServiceFake()
fakeDashboardVersionService.ExpectedDashboardVersion = &dashver.DashboardVersion{}
teamService := &teamtest.FakeService{}
dashboardService := dashboards.NewFakeDashboardService(t)
dashboardService.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) {
q := args.Get(1).(*models.GetDashboardQuery)
Expand Down Expand Up @@ -274,7 +277,7 @@ func TestDashboardAPIEndpoint(t *testing.T) {
setting.ViewersCanEdit = origCanEdit
})
setting.ViewersCanEdit = false
guardian.InitLegacyGuardian(mockSQLStore, dashboardService)
guardian.InitLegacyGuardian(mockSQLStore, dashboardService, teamService)
}

// This tests six scenarios:
Expand Down Expand Up @@ -378,7 +381,7 @@ func TestDashboardAPIEndpoint(t *testing.T) {
{OrgId: 1, DashboardId: 2, UserId: 1, Permission: models.PERMISSION_EDIT},
}
}).Return(nil)
guardian.InitLegacyGuardian(mockSQLStore, dashboardService)
guardian.InitLegacyGuardian(mockSQLStore, dashboardService, teamService)
}

loggedInUserScenarioWithRole(t, "When calling GET on", "GET", "/api/dashboards/uid/abcdefghi",
Expand Down Expand Up @@ -440,7 +443,7 @@ func TestDashboardAPIEndpoint(t *testing.T) {
{OrgId: 1, DashboardId: 2, UserId: 1, Permission: models.PERMISSION_VIEW},
}
}).Return(nil)
guardian.InitLegacyGuardian(mockSQLStore, dashboardService)
guardian.InitLegacyGuardian(mockSQLStore, dashboardService, teamService)
}

loggedInUserScenarioWithRole(t, "When calling GET on", "GET", "/api/dashboards/uid/abcdefghi", "/api/dashboards/uid/:uid", role, func(sc *scenarioContext) {
Expand Down Expand Up @@ -480,7 +483,7 @@ func TestDashboardAPIEndpoint(t *testing.T) {
{OrgId: 1, DashboardId: 2, UserId: 1, Permission: models.PERMISSION_ADMIN},
}
}).Return(nil)
guardian.InitLegacyGuardian(mockSQLStore, dashboardService)
guardian.InitLegacyGuardian(mockSQLStore, dashboardService, teamService)
}

loggedInUserScenarioWithRole(t, "When calling GET on", "GET", "/api/dashboards/uid/abcdefghi", "/api/dashboards/uid/:uid", role, func(sc *scenarioContext) {
Expand Down Expand Up @@ -533,7 +536,7 @@ func TestDashboardAPIEndpoint(t *testing.T) {
{OrgId: 1, DashboardId: 2, UserId: 1, Permission: models.PERMISSION_VIEW},
}
}).Return(nil)
guardian.InitLegacyGuardian(mockSQLStore, dashboardService)
guardian.InitLegacyGuardian(mockSQLStore, dashboardService, teamService)
}

loggedInUserScenarioWithRole(t, "When calling GET on", "GET", "/api/dashboards/uid/abcdefghi", "/api/dashboards/uid/:uid", role, func(sc *scenarioContext) {
Expand Down Expand Up @@ -743,9 +746,10 @@ func TestDashboardAPIEndpoint(t *testing.T) {
}
sqlmock := mockstore.SQLStoreMock{}
setUp := func() {
teamSvc := &teamtest.FakeService{}
dashSvc := dashboards.NewFakeDashboardService(t)
dashSvc.On("GetDashboardACLInfoList", mock.Anything, mock.AnythingOfType("*models.GetDashboardACLInfoListQuery")).Return(nil)
guardian.InitLegacyGuardian(&sqlmock, dashSvc)
guardian.InitLegacyGuardian(&sqlmock, dashSvc, teamSvc)
}

cmd := dtos.CalculateDiffOptions{
Expand Down Expand Up @@ -861,6 +865,7 @@ func TestDashboardAPIEndpoint(t *testing.T) {
dashboardStore := dashboards.NewFakeDashboardStore(t)
dashboardStore.On("GetProvisionedDataByDashboardID", mock.Anything).Return(&models.DashboardProvisioning{ExternalId: "/dashboard1.json"}, nil).Once()

teamService := &teamtest.FakeService{}
dashboardService := dashboards.NewFakeDashboardService(t)

dataValue, err := simplejson.NewJson([]byte(`{"id": 1, "editable": true, "style": "dark"}`))
Expand All @@ -873,7 +878,7 @@ func TestDashboardAPIEndpoint(t *testing.T) {
q := args.Get(1).(*models.GetDashboardACLInfoListQuery)
q.Result = []*models.DashboardACLInfoDTO{{OrgId: testOrgID, DashboardId: 1, UserId: testUserID, Permission: models.PERMISSION_EDIT}}
}).Return(nil)
guardian.InitLegacyGuardian(mockSQLStore, dashboardService)
guardian.InitLegacyGuardian(mockSQLStore, dashboardService, teamService)

loggedInUserScenarioWithRole(t, "When calling GET on", "GET", "/api/dashboards/uid/dash", "/api/dashboards/uid/:uid", org.RoleEditor, func(sc *scenarioContext) {
fakeProvisioningService := provisioning.NewProvisioningServiceMock(context.Background())
Expand Down
4 changes: 3 additions & 1 deletion pkg/api/folder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/grafana/grafana/pkg/services/guardian"
"github.com/grafana/grafana/pkg/services/quota/quotatest"
"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
"github.com/grafana/grafana/pkg/services/team/teamtest"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/web/webtest"
Expand Down Expand Up @@ -237,13 +238,14 @@ func createFolderScenario(t *testing.T, desc string, url string, routePattern st
setUpRBACGuardian(t)
t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) {
aclMockResp := []*models.DashboardACLInfoDTO{}
teamSvc := &teamtest.FakeService{}
dashSvc := &dashboards.FakeDashboardService{}
dashSvc.On("GetDashboardACLInfoList", mock.Anything, mock.AnythingOfType("*models.GetDashboardACLInfoListQuery")).Run(func(args mock.Arguments) {
q := args.Get(1).(*models.GetDashboardACLInfoListQuery)
q.Result = aclMockResp
}).Return(nil)
store := mockstore.NewSQLStoreMock()
guardian.InitLegacyGuardian(store, dashSvc)
guardian.InitLegacyGuardian(store, dashSvc, teamSvc)
hs := HTTPServer{
AccessControl: acmock.New(),
folderService: folderService,
Expand Down
6 changes: 5 additions & 1 deletion pkg/api/http_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ import (
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/star"
"github.com/grafana/grafana/pkg/services/store"
"github.com/grafana/grafana/pkg/services/team"
"github.com/grafana/grafana/pkg/services/teamguardian"
tempUser "github.com/grafana/grafana/pkg/services/temp_user"
"github.com/grafana/grafana/pkg/services/thumbs"
Expand Down Expand Up @@ -189,6 +190,7 @@ type HTTPServer struct {
dashboardThumbsService dashboardThumbs.Service
loginAttemptService loginAttempt.Service
orgService org.Service
teamService team.Service
accesscontrolService accesscontrol.Service
annotationsRepo annotations.Repository
}
Expand Down Expand Up @@ -228,7 +230,8 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi
playlistService playlist.Service, apiKeyService apikey.Service, kvStore kvstore.KVStore,
secretsMigrator secrets.Migrator, secretsPluginManager plugins.SecretsPluginManager, secretsService secrets.Service,
secretsPluginMigrator spm.SecretMigrationProvider, secretsStore secretsKV.SecretsKVStore,
publicDashboardsApi *publicdashboardsApi.Api, userService user.Service, tempUserService tempUser.Service, loginAttemptService loginAttempt.Service, orgService org.Service,
publicDashboardsApi *publicdashboardsApi.Api, userService user.Service, tempUserService tempUser.Service,
loginAttemptService loginAttempt.Service, orgService org.Service, teamService team.Service,
accesscontrolService accesscontrol.Service, dashboardThumbsService dashboardThumbs.Service, annotationRepo annotations.Repository,
) (*HTTPServer, error) {
web.Env = cfg.Env
Expand Down Expand Up @@ -324,6 +327,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi
dashboardThumbsService: dashboardThumbsService,
loginAttemptService: loginAttemptService,
orgService: orgService,
teamService: teamService,
accesscontrolService: accesscontrolService,
annotationsRepo: annotationRepo,
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/api/org_users_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,9 @@ func TestOrgUsersAPIEndpoint_LegacyAccessControl_TeamAdmin(t *testing.T) {
setInitCtxSignedInViewer(sc.initCtx)

// Setup store teams
team1, err := sc.db.CreateTeam("testteam1", "[email protected]", testOrgID)
team1, err := sc.teamService.CreateTeam("testteam1", "[email protected]", testOrgID)
require.NoError(t, err)
err = sc.db.AddTeamMember(testUserID, testOrgID, team1.Id, false, models.PERMISSION_ADMIN)
err = sc.teamService.AddTeamMember(testUserID, testOrgID, team1.Id, false, models.PERMISSION_ADMIN)
require.NoError(t, err)

response := callAPI(sc.server, http.MethodGet, "/api/org/users/lookup", nil, t)
Expand Down
10 changes: 5 additions & 5 deletions pkg/api/team.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func (hs *HTTPServer) CreateTeam(c *models.ReqContext) response.Response {
return response.Error(403, "Not allowed to create team.", nil)
}

team, err := hs.SQLStore.CreateTeam(cmd.Name, cmd.Email, c.OrgID)
team, err := hs.teamService.CreateTeam(cmd.Name, cmd.Email, c.OrgID)
if err != nil {
if errors.Is(err, models.ErrTeamNameTaken) {
return response.Error(409, "Team name taken", err)
Expand Down Expand Up @@ -88,7 +88,7 @@ func (hs *HTTPServer) UpdateTeam(c *models.ReqContext) response.Response {
}
}

if err := hs.SQLStore.UpdateTeam(c.Req.Context(), &cmd); err != nil {
if err := hs.teamService.UpdateTeam(c.Req.Context(), &cmd); err != nil {
if errors.Is(err, models.ErrTeamNameTaken) {
return response.Error(400, "Team name taken", err)
}
Expand Down Expand Up @@ -122,7 +122,7 @@ func (hs *HTTPServer) DeleteTeamByID(c *models.ReqContext) response.Response {
}
}

if err := hs.SQLStore.DeleteTeam(c.Req.Context(), &models.DeleteTeamCommand{OrgId: orgId, Id: teamId}); err != nil {
if err := hs.teamService.DeleteTeam(c.Req.Context(), &models.DeleteTeamCommand{OrgId: orgId, Id: teamId}); err != nil {
if errors.Is(err, models.ErrTeamNotFound) {
return response.Error(404, "Failed to delete Team. ID not found", nil)
}
Expand Down Expand Up @@ -167,7 +167,7 @@ func (hs *HTTPServer) SearchTeams(c *models.ReqContext) response.Response {
HiddenUsers: hs.Cfg.HiddenUsers,
}

if err := hs.SQLStore.SearchTeams(c.Req.Context(), &query); err != nil {
if err := hs.teamService.SearchTeams(c.Req.Context(), &query); err != nil {
return response.Error(500, "Failed to search Teams", err)
}

Expand Down Expand Up @@ -231,7 +231,7 @@ func (hs *HTTPServer) GetTeamByID(c *models.ReqContext) response.Response {
UserIdFilter: userIdFilter,
}

if err := hs.SQLStore.GetTeamById(c.Req.Context(), &query); err != nil {
if err := hs.teamService.GetTeamById(c.Req.Context(), &query); err != nil {
if errors.Is(err, models.ErrTeamNotFound) {
return response.Error(404, "Team not found", err)
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/api/team_members.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func (hs *HTTPServer) GetTeamMembers(c *models.ReqContext) response.Response {
}
}

if err := hs.SQLStore.GetTeamMembers(c.Req.Context(), &query); err != nil {
if err := hs.teamService.GetTeamMembers(c.Req.Context(), &query); err != nil {
return response.Error(500, "Failed to get Team Members", err)
}

Expand Down Expand Up @@ -94,7 +94,7 @@ func (hs *HTTPServer) AddTeamMember(c *models.ReqContext) response.Response {
}
}

isTeamMember, err := hs.SQLStore.IsTeamMember(c.OrgID, cmd.TeamId, cmd.UserId)
isTeamMember, err := hs.teamService.IsTeamMember(c.OrgID, cmd.TeamId, cmd.UserId)
if err != nil {
return response.Error(500, "Failed to add team member.", err)
}
Expand Down Expand Up @@ -143,7 +143,7 @@ func (hs *HTTPServer) UpdateTeamMember(c *models.ReqContext) response.Response {
}
}

isTeamMember, err := hs.SQLStore.IsTeamMember(orgId, teamId, userId)
isTeamMember, err := hs.teamService.IsTeamMember(orgId, teamId, userId)
if err != nil {
return response.Error(500, "Failed to update team member.", err)
}
Expand Down
Loading

0 comments on commit 305d494

Please sign in to comment.