From d076cabb6093041dee266db6b31257ae5b67bdc3 Mon Sep 17 00:00:00 2001 From: Yuriy Tseretyan Date: Thu, 10 Mar 2022 12:19:50 -0500 Subject: [PATCH] Folder name scope resolver (#46380) * move dashboard store mock to parent package to avoid cycle of dependencies * add scope resolver for folders that resolves names to id --- pkg/api/dashboard_permission_test.go | 11 +-- pkg/api/dashboard_test.go | 11 +-- pkg/api/folder_permission_test.go | 6 +- pkg/services/dashboards/accesscontrol.go | 32 ++++++++- pkg/services/dashboards/accesscontrol_test.go | 71 +++++++++++++++++++ pkg/services/dashboards/dashboard.go | 2 +- .../{database => }/database_mock.go | 2 +- .../manager/dashboard_service_test.go | 8 +-- .../dashboards/manager/folder_service.go | 3 + .../dashboards/manager/folder_service_test.go | 15 ++-- .../libraryelements/libraryelements_test.go | 9 ++- .../librarypanels/librarypanels_test.go | 10 ++- pkg/services/ngalert/tests/util.go | 2 +- 13 files changed, 147 insertions(+), 35 deletions(-) create mode 100644 pkg/services/dashboards/accesscontrol_test.go rename pkg/services/dashboards/{database => }/database_mock.go (99%) diff --git a/pkg/api/dashboard_permission_test.go b/pkg/api/dashboard_permission_test.go index cd2b68e373a..940c6cc4fca 100644 --- a/pkg/api/dashboard_permission_test.go +++ b/pkg/api/dashboard_permission_test.go @@ -5,26 +5,27 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/models" accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" - "github.com/grafana/grafana/pkg/services/dashboards/database" + "github.com/grafana/grafana/pkg/services/dashboards" dashboardservice "github.com/grafana/grafana/pkg/services/dashboards/manager" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/grafana/grafana/pkg/setting" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" ) func TestDashboardPermissionAPIEndpoint(t *testing.T) { t.Run("Dashboard permissions test", func(t *testing.T) { settings := setting.NewCfg() - dashboardStore := &database.FakeDashboardStore{} + dashboardStore := &dashboards.FakeDashboardStore{} defer dashboardStore.AssertExpectations(t) features := featuremgmt.WithFeatures() diff --git a/pkg/api/dashboard_test.go b/pkg/api/dashboard_test.go index f38d8048ba2..eb8a87e27e0 100644 --- a/pkg/api/dashboard_test.go +++ b/pkg/api/dashboard_test.go @@ -9,6 +9,10 @@ import ( "net/http" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" @@ -30,9 +34,6 @@ import ( "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" ) func TestGetHomeDashboard(t *testing.T) { @@ -544,7 +545,7 @@ func TestDashboardAPIEndpoint(t *testing.T) { }) t.Run("Post dashboard response tests", func(t *testing.T) { - dashboardStore := &database.FakeDashboardStore{} + dashboardStore := &dashboards.FakeDashboardStore{} defer dashboardStore.AssertExpectations(t) // This tests that a valid request returns correct response t.Run("Given a correct request for creating a dashboard", func(t *testing.T) { @@ -883,7 +884,7 @@ func TestDashboardAPIEndpoint(t *testing.T) { return "/tmp/grafana/dashboards" } - dashboardStore := &database.FakeDashboardStore{} + dashboardStore := &dashboards.FakeDashboardStore{} defer dashboardStore.AssertExpectations(t) dashboardStore.On("GetProvisionedDataByDashboardID", mock.Anything).Return(&models.DashboardProvisioning{ExternalId: "/dashboard1.json"}, nil).Once() diff --git a/pkg/api/folder_permission_test.go b/pkg/api/folder_permission_test.go index 921c751b8ba..18b45c069cb 100644 --- a/pkg/api/folder_permission_test.go +++ b/pkg/api/folder_permission_test.go @@ -10,19 +10,19 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/stretchr/testify/mock" + "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/dashboards" - "github.com/grafana/grafana/pkg/services/dashboards/database" service "github.com/grafana/grafana/pkg/services/dashboards/manager" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/grafana/grafana/pkg/setting" - "github.com/stretchr/testify/mock" ) func TestFolderPermissionAPIEndpoint(t *testing.T) { @@ -31,7 +31,7 @@ func TestFolderPermissionAPIEndpoint(t *testing.T) { folderService := &dashboards.FakeFolderService{} defer folderService.AssertExpectations(t) - dashboardStore := &database.FakeDashboardStore{} + dashboardStore := &dashboards.FakeDashboardStore{} defer dashboardStore.AssertExpectations(t) features := featuremgmt.WithFeatures() diff --git a/pkg/services/dashboards/accesscontrol.go b/pkg/services/dashboards/accesscontrol.go index 005ce21d668..340a5948f6d 100644 --- a/pkg/services/dashboards/accesscontrol.go +++ b/pkg/services/dashboards/accesscontrol.go @@ -1,6 +1,12 @@ package dashboards -import "github.com/grafana/grafana/pkg/services/accesscontrol" +import ( + "context" + "strconv" + "strings" + + ac "github.com/grafana/grafana/pkg/services/accesscontrol" +) const ( ActionFoldersCreate = "folders:create" @@ -14,6 +20,26 @@ const ( ) var ( - ScopeFoldersAll = accesscontrol.GetResourceAllScope(ScopeFoldersRoot) - ScopeFoldersProvider = accesscontrol.NewScopeProvider(ScopeFoldersRoot) + ScopeFoldersAll = ac.GetResourceAllScope(ScopeFoldersRoot) + ScopeFoldersProvider = ac.NewScopeProvider(ScopeFoldersRoot) ) + +// NewNameScopeResolver provides an AttributeScopeResolver that is able to convert a scope prefixed with "folders:name:" into an id based scope. +func NewNameScopeResolver(db Store) (string, ac.AttributeScopeResolveFunc) { + prefix := ScopeFoldersProvider.GetResourceScopeName("") + resolver := func(ctx context.Context, orgID int64, scope string) (string, error) { + if !strings.HasPrefix(scope, prefix) { + return "", ac.ErrInvalidScope + } + nsName := scope[len(prefix):] + if len(nsName) == 0 { + return "", ac.ErrInvalidScope + } + folder, err := db.GetFolderByTitle(orgID, nsName) + if err != nil { + return "", err + } + return ScopeFoldersProvider.GetResourceScope(strconv.FormatInt(folder.Id, 10)), nil + } + return prefix, resolver +} diff --git a/pkg/services/dashboards/accesscontrol_test.go b/pkg/services/dashboards/accesscontrol_test.go new file mode 100644 index 00000000000..05eb83624cd --- /dev/null +++ b/pkg/services/dashboards/accesscontrol_test.go @@ -0,0 +1,71 @@ +package dashboards + +import ( + "context" + "fmt" + "math/rand" + "testing" + + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/models" + ac "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/util" +) + +func TestNewNameScopeResolver(t *testing.T) { + t.Run("prefix should be expected", func(t *testing.T) { + prefix, _ := NewNameScopeResolver(&FakeDashboardStore{}) + require.Equal(t, "folders:name:", prefix) + }) + + t.Run("resolver should convert to id scope", func(t *testing.T) { + dashboardStore := &FakeDashboardStore{} + + _, resolver := NewNameScopeResolver(dashboardStore) + + orgId := rand.Int63() + title := "Very complex :title with: and /" + util.GenerateShortUID() + + db := &models.Dashboard{Id: rand.Int63()} + dashboardStore.On("GetFolderByTitle", mock.Anything, mock.Anything).Return(db, nil).Once() + + scope := "folders:name:" + title + + resolvedScope, err := resolver(context.Background(), orgId, scope) + require.NoError(t, err) + + require.Equal(t, fmt.Sprintf("folders:id:%v", db.Id), resolvedScope) + + dashboardStore.AssertCalled(t, "GetFolderByTitle", orgId, title) + }) + t.Run("resolver should fail if input scope is not expected", func(t *testing.T) { + dashboardStore := &FakeDashboardStore{} + _, resolver := NewNameScopeResolver(dashboardStore) + + _, err := resolver(context.Background(), rand.Int63(), "folders:id:123") + require.ErrorIs(t, err, ac.ErrInvalidScope) + }) + t.Run("resolver should fail if resource of input scope is empty", func(t *testing.T) { + dashboardStore := &FakeDashboardStore{} + _, resolver := NewNameScopeResolver(dashboardStore) + + _, err := resolver(context.Background(), rand.Int63(), "folders:name:") + require.ErrorIs(t, err, ac.ErrInvalidScope) + }) + t.Run("returns 'not found' if folder does not exist", func(t *testing.T) { + dashboardStore := &FakeDashboardStore{} + + _, resolver := NewNameScopeResolver(dashboardStore) + + orgId := rand.Int63() + dashboardStore.On("GetFolderByTitle", mock.Anything, mock.Anything).Return(nil, models.ErrDashboardNotFound).Once() + + scope := "folders:name:" + util.GenerateShortUID() + + resolvedScope, err := resolver(context.Background(), orgId, scope) + require.ErrorIs(t, err, models.ErrDashboardNotFound) + require.Empty(t, resolvedScope) + }) +} diff --git a/pkg/services/dashboards/dashboard.go b/pkg/services/dashboards/dashboard.go index da1273ffa9f..9bdbe935f99 100644 --- a/pkg/services/dashboards/dashboard.go +++ b/pkg/services/dashboards/dashboard.go @@ -29,7 +29,7 @@ type DashboardProvisioningService interface { DeleteOrphanedProvisionedDashboards(ctx context.Context, cmd *models.DeleteOrphanedProvisionedDashboardsCommand) error } -//go:generate mockery --name Store --structname FakeDashboardStore --output database --outpkg database --filename database_mock.go +//go:generate mockery --name Store --structname FakeDashboardStore --inpackage --filename database_mock.go // Store is a dashboard store. type Store interface { // ValidateDashboardBeforeSave validates a dashboard before save. diff --git a/pkg/services/dashboards/database/database_mock.go b/pkg/services/dashboards/database_mock.go similarity index 99% rename from pkg/services/dashboards/database/database_mock.go rename to pkg/services/dashboards/database_mock.go index 8091583a48c..cef866f635e 100644 --- a/pkg/services/dashboards/database/database_mock.go +++ b/pkg/services/dashboards/database_mock.go @@ -1,6 +1,6 @@ // Code generated by mockery v2.10.0. DO NOT EDIT. -package database +package dashboards import ( context "context" diff --git a/pkg/services/dashboards/manager/dashboard_service_test.go b/pkg/services/dashboards/manager/dashboard_service_test.go index dba479c8160..4f8570a3e1d 100644 --- a/pkg/services/dashboards/manager/dashboard_service_test.go +++ b/pkg/services/dashboards/manager/dashboard_service_test.go @@ -8,23 +8,23 @@ import ( "errors" "testing" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" m "github.com/grafana/grafana/pkg/services/dashboards" - "github.com/grafana/grafana/pkg/services/dashboards/database" "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/setting" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" ) func TestDashboardService(t *testing.T) { t.Run("Dashboard service tests", func(t *testing.T) { bus.ClearBusHandlers() - fakeStore := database.FakeDashboardStore{} + fakeStore := m.FakeDashboardStore{} defer fakeStore.AssertExpectations(t) service := &DashboardServiceImpl{ log: log.New("test.logger"), diff --git a/pkg/services/dashboards/manager/folder_service.go b/pkg/services/dashboards/manager/folder_service.go index 87683f11345..f7e7a4e2fb3 100644 --- a/pkg/services/dashboards/manager/folder_service.go +++ b/pkg/services/dashboards/manager/folder_service.go @@ -31,7 +31,10 @@ type FolderServiceImpl struct { func ProvideFolderService( cfg *setting.Cfg, dashboardService dashboards.DashboardService, dashboardStore dashboards.Store, searchService *search.SearchService, features featuremgmt.FeatureToggles, permissionsServices accesscontrol.PermissionsServices, + ac accesscontrol.AccessControl, ) *FolderServiceImpl { + ac.RegisterAttributeScopeResolver(dashboards.NewNameScopeResolver(dashboardStore)) + return &FolderServiceImpl{ cfg: cfg, log: log.New("folder-service"), diff --git a/pkg/services/dashboards/manager/folder_service_test.go b/pkg/services/dashboards/manager/folder_service_test.go index d462fad6889..45b4ac898d1 100644 --- a/pkg/services/dashboards/manager/folder_service_test.go +++ b/pkg/services/dashboards/manager/folder_service_test.go @@ -7,17 +7,17 @@ import ( "context" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/dashboards" - "github.com/grafana/grafana/pkg/services/dashboards/database" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/setting" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" ) var orgID = int64(1) @@ -25,17 +25,20 @@ var user = &models.SignedInUser{UserId: 1} func TestFolderService(t *testing.T) { t.Run("Folder service tests", func(t *testing.T) { - store := &database.FakeDashboardStore{} + store := &dashboards.FakeDashboardStore{} defer store.AssertExpectations(t) cfg := setting.NewCfg() features := featuremgmt.WithFeatures() permissionsServices := acmock.NewPermissionsServicesMock() dashboardService := ProvideDashboardService(cfg, store, nil, features, permissionsServices) + ac := acmock.New() service := ProvideFolderService( cfg, &dashboards.FakeDashboardService{DashboardService: dashboardService}, - store, nil, features, permissionsServices, + store, nil, features, permissionsServices, ac, ) + require.Len(t, ac.Calls.RegisterAttributeScopeResolver, 1) + t.Run("Given user has no permissions", func(t *testing.T) { origNewGuardian := guardian.New guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{}) diff --git a/pkg/services/libraryelements/libraryelements_test.go b/pkg/services/libraryelements/libraryelements_test.go index d13c0df3ece..d27aace6675 100644 --- a/pkg/services/libraryelements/libraryelements_test.go +++ b/pkg/services/libraryelements/libraryelements_test.go @@ -10,6 +10,8 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/models" @@ -22,7 +24,6 @@ import ( "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web" - "github.com/stretchr/testify/require" ) const userInDbName = "user_in_db" @@ -222,9 +223,10 @@ func createFolderWithACL(t *testing.T, sqlStore *sqlstore.SQLStore, title string cfg, dashboardStore, nil, features, permissionsServices, ) + ac := acmock.New() s := dashboardservice.ProvideFolderService( cfg, d, dashboardStore, nil, - features, permissionsServices, + features, permissionsServices, ac, ) t.Logf("Creating folder with title and UID %q", title) folder, err := s.CreateFolder(context.Background(), &user, user.OrgId, title, title) @@ -314,12 +316,13 @@ func testScenario(t *testing.T, desc string, fn func(t *testing.T, sc scenarioCo setting.NewCfg(), dashboardStore, nil, featuremgmt.WithFeatures(), acmock.NewPermissionsServicesMock(), ) + ac := acmock.New() service := LibraryElementService{ Cfg: setting.NewCfg(), SQLStore: sqlStore, folderService: dashboardservice.ProvideFolderService( setting.NewCfg(), dashboardService, dashboardStore, nil, - featuremgmt.WithFeatures(), acmock.NewPermissionsServicesMock(), + featuremgmt.WithFeatures(), acmock.NewPermissionsServicesMock(), ac, ), } diff --git a/pkg/services/librarypanels/librarypanels_test.go b/pkg/services/librarypanels/librarypanels_test.go index a41de4c9b38..e76c6aa5382 100644 --- a/pkg/services/librarypanels/librarypanels_test.go +++ b/pkg/services/librarypanels/librarypanels_test.go @@ -8,6 +8,8 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/models" @@ -20,7 +22,6 @@ import ( "github.com/grafana/grafana/pkg/services/libraryelements" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/setting" - "github.com/stretchr/testify/require" ) const userInDbName = "user_in_db" @@ -1439,7 +1440,8 @@ func createFolderWithACL(t *testing.T, sqlStore *sqlstore.SQLStore, title string permissionsServices := acmock.NewPermissionsServicesMock() dashboardStore := database.ProvideDashboardStore(sqlStore) d := dashboardservice.ProvideDashboardService(cfg, dashboardStore, nil, features, permissionsServices) - s := dashboardservice.ProvideFolderService(cfg, d, dashboardStore, nil, features, permissionsServices) + ac := acmock.New() + s := dashboardservice.ProvideFolderService(cfg, d, dashboardStore, nil, features, permissionsServices, ac) t.Logf("Creating folder with title and UID %q", title) folder, err := s.CreateFolder(context.Background(), user, user.OrgId, title, title) @@ -1536,9 +1538,11 @@ func testScenario(t *testing.T, desc string, fn func(t *testing.T, sc scenarioCo cfg, dashboardStore, &alerting.DashAlertExtractorService{}, features, permissionsServices, ) + ac := acmock.New() + folderService := dashboardservice.ProvideFolderService( cfg, dashboardService, dashboardStore, nil, - features, permissionsServices, + features, permissionsServices, ac, ) elementService := libraryelements.ProvideService(cfg, sqlStore, routing.NewRouteRegister(), folderService) diff --git a/pkg/services/ngalert/tests/util.go b/pkg/services/ngalert/tests/util.go index 4ab5e027433..98d2f4eef78 100644 --- a/pkg/services/ngalert/tests/util.go +++ b/pkg/services/ngalert/tests/util.go @@ -54,7 +54,7 @@ func SetupTestEnv(t *testing.T, baseInterval time.Duration) (*ngalert.AlertNG, * ) folderService := dashboardservice.ProvideFolderService( cfg, dashboardService, dashboardStore, nil, - features, permissionsServices, + features, permissionsServices, ac, ) ng, err := ngalert.ProvideService(