Nested Folders: Fix /api/folders pagination (#79447)

* Nested Folders: Fix /api/folders pagination

We used to check access to the root folders after fetching them from the DB with pagination.
This fix splits logic for fetching folders in:
- fetching subfolders
- fetching root folders
and refactors the query for the latter so that is filters by folders with permissions

* Add tests

* Update benchmarks
This commit is contained in:
Sofia Papagiannaki
2023-12-15 19:34:08 +02:00
committed by GitHub
parent cf8e8852c3
commit d89a8a3a82
9 changed files with 434 additions and 55 deletions
+2 -2
View File
@@ -100,13 +100,13 @@ func BenchmarkFolderListAndSearch(b *testing.B) {
{
desc: "impl=default nested_folders=on get root folders",
url: "/api/folders",
expectedLen: LEVEL0_FOLDER_NUM,
expectedLen: LEVEL0_FOLDER_NUM + 1, // for shared with me folder
features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders),
},
{
desc: "impl=permissionsFilterRemoveSubquery nested_folders=on get root folders",
url: "/api/folders",
expectedLen: LEVEL0_FOLDER_NUM,
expectedLen: LEVEL0_FOLDER_NUM + 1, // for shared with me folder
features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders, featuremgmt.FlagPermissionsFilterRemoveSubquery),
},
{
+80 -41
View File
@@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"runtime"
"strings"
"sync"
"time"
@@ -11,6 +12,7 @@ import (
"github.com/prometheus/client_golang/prometheus"
"golang.org/x/exp/slices"
"github.com/grafana/dskit/concurrency"
"github.com/grafana/grafana/pkg/bus"
"github.com/grafana/grafana/pkg/events"
"github.com/grafana/grafana/pkg/infra/db"
@@ -149,32 +151,36 @@ func (s *Service) Get(ctx context.Context, cmd *folder.GetFolderQuery) (*folder.
return f, err
}
func (s *Service) GetChildren(ctx context.Context, cmd *folder.GetChildrenQuery) ([]*folder.Folder, error) {
if cmd.SignedInUser == nil {
func (s *Service) GetChildren(ctx context.Context, q *folder.GetChildrenQuery) ([]*folder.Folder, error) {
if q.SignedInUser == nil {
return nil, folder.ErrBadRequest.Errorf("missing signed in user")
}
if s.features.IsEnabled(ctx, featuremgmt.FlagNestedFolders) && cmd.UID == folder.SharedWithMeFolderUID {
return s.GetSharedWithMe(ctx, cmd)
if s.features.IsEnabled(ctx, featuremgmt.FlagNestedFolders) && q.UID == folder.SharedWithMeFolderUID {
return s.GetSharedWithMe(ctx, q)
}
if cmd.UID != "" {
g, err := guardian.NewByUID(ctx, cmd.UID, cmd.OrgID, cmd.SignedInUser)
if err != nil {
return nil, err
}
canView, err := g.CanView()
if err != nil {
return nil, err
}
if !canView {
return nil, dashboards.ErrFolderAccessDenied
}
if q.UID == "" {
return s.getRootFolders(ctx, q)
}
children, err := s.store.GetChildren(ctx, *cmd)
// we only need to check access to the folder
// if the parent is accessible then the subfolders are accessible as well (due to inheritance)
g, err := guardian.NewByUID(ctx, q.UID, q.OrgID, q.SignedInUser)
if err != nil {
return nil, err
}
canView, err := g.CanView()
if err != nil {
return nil, err
}
if !canView {
return nil, dashboards.ErrFolderAccessDenied
}
children, err := s.store.GetChildren(ctx, *q)
if err != nil {
return nil, err
}
@@ -184,12 +190,11 @@ func (s *Service) GetChildren(ctx context.Context, cmd *folder.GetChildrenQuery)
childrenUIDs = append(childrenUIDs, f.UID)
}
dashFolders, err := s.dashboardFolderStore.GetFolders(ctx, cmd.OrgID, childrenUIDs)
dashFolders, err := s.dashboardFolderStore.GetFolders(ctx, q.OrgID, childrenUIDs)
if err != nil {
return nil, folder.ErrInternal.Errorf("failed to fetch subfolders from dashboard store: %w", err)
}
filtered := make([]*folder.Folder, 0, len(children))
for _, f := range children {
// fetch folder from dashboard store
dashFolder, ok := dashFolders[f.UID]
@@ -201,33 +206,67 @@ func (s *Service) GetChildren(ctx context.Context, cmd *folder.GetChildrenQuery)
// always expose the dashboard store sequential ID
// nolint:staticcheck
f.ID = dashFolder.ID
}
if cmd.UID != "" {
// parent access has been checked already
// the subfolder must be accessible as well (due to inheritance)
filtered = append(filtered, f)
continue
}
return children, nil
}
g, err := guardian.NewByFolder(ctx, dashFolder, dashFolder.OrgID, cmd.SignedInUser)
if err != nil {
return nil, err
func (s *Service) getRootFolders(ctx context.Context, q *folder.GetChildrenQuery) ([]*folder.Folder, error) {
permissions := q.SignedInUser.GetPermissions()
folderPermissions := permissions[dashboards.ActionFoldersRead]
folderPermissions = append(folderPermissions, permissions[dashboards.ActionDashboardsRead]...)
q.FolderUIDs = make([]string, 0, len(folderPermissions))
for _, p := range folderPermissions {
if p == dashboards.ScopeFoldersAll {
// no need to query for folders with permissions
// the user has permission to access all folders
q.FolderUIDs = nil
break
}
canView, err := g.CanView()
if err != nil {
return nil, err
}
if canView {
filtered = append(filtered, f)
if folderUid, found := strings.CutPrefix(p, dashboards.ScopeFoldersPrefix); found {
if !slices.Contains(q.FolderUIDs, folderUid) {
q.FolderUIDs = append(q.FolderUIDs, folderUid)
}
}
}
if len(filtered) < len(children) {
// add "shared with me" folder
filtered = append(filtered, &folder.SharedWithMeFolder)
children, err := s.store.GetChildren(ctx, *q)
if err != nil {
return nil, err
}
return filtered, nil
childrenUIDs := make([]string, 0, len(children))
for _, f := range children {
childrenUIDs = append(childrenUIDs, f.UID)
}
dashFolders, err := s.dashboardFolderStore.GetFolders(ctx, q.OrgID, childrenUIDs)
if err != nil {
return nil, folder.ErrInternal.Errorf("failed to fetch subfolders from dashboard store: %w", err)
}
if err := concurrency.ForEachJob(ctx, len(children), runtime.NumCPU(), func(ctx context.Context, i int) error {
f := children[i]
// fetch folder from dashboard store
dashFolder, ok := dashFolders[f.UID]
if !ok {
s.log.Error("failed to fetch folder by UID from dashboard store", "orgID", f.OrgID, "uid", f.UID)
}
// always expose the dashboard store sequential ID
// nolint:staticcheck
f.ID = dashFolder.ID
return nil
}); err != nil {
return nil, folder.ErrInternal.Errorf("failed to assign folder sequential ID: %w", err)
}
// add "shared with me" folder on the 1st page
if (q.Page == 0 || q.Page == 1) && len(q.FolderUIDs) != 0 {
children = append(children, &folder.SharedWithMeFolder)
}
return children, nil
}
// GetSharedWithMe returns folders available to user, which cannot be accessed from the root folders
@@ -253,7 +292,7 @@ func (s *Service) getAvailableNonRootFolders(ctx context.Context, orgID int64, u
folderPermissions := permissions[dashboards.ActionFoldersRead]
folderPermissions = append(folderPermissions, permissions[dashboards.ActionDashboardsRead]...)
nonRootFolders := make([]*folder.Folder, 0)
folderUids := make([]string, 0)
folderUids := make([]string, 0, len(folderPermissions))
for _, p := range folderPermissions {
if folderUid, found := strings.CutPrefix(p, dashboards.ScopeFoldersPrefix); found {
if !slices.Contains(folderUids, folderUid) {
+17 -4
View File
@@ -2,6 +2,7 @@ package folderimpl
import (
"context"
"runtime"
"strings"
"time"
@@ -213,7 +214,7 @@ func (ss *sqlStore) GetParents(ctx context.Context, q folder.GetParentsQuery) ([
return nil, err
}
if err := concurrency.ForEachJob(ctx, len(folders), len(folders), func(ctx context.Context, idx int) error {
if err := concurrency.ForEachJob(ctx, len(folders), runtime.NumCPU(), func(ctx context.Context, idx int) error {
folders[idx].WithURL()
return nil
}); err != nil {
@@ -240,13 +241,25 @@ func (ss *sqlStore) GetChildren(ctx context.Context, q folder.GetChildrenQuery)
sql := strings.Builder{}
args := make([]any, 0, 2)
if q.UID == "" {
sql.WriteString("SELECT * FROM folder WHERE parent_uid IS NULL AND org_id=? ORDER BY title ASC")
sql.WriteString("SELECT * FROM folder WHERE parent_uid IS NULL AND org_id=?")
args = append(args, q.OrgID)
} else {
sql.WriteString("SELECT * FROM folder WHERE parent_uid=? AND org_id=? ORDER BY title ASC")
sql.WriteString("SELECT * FROM folder WHERE parent_uid=? AND org_id=?")
args = append(args, q.UID, q.OrgID)
}
if q.FolderUIDs != nil {
sql.WriteString(" AND uid IN (?")
for range q.FolderUIDs[1:] {
sql.WriteString(", ?")
}
sql.WriteString(")")
for _, uid := range q.FolderUIDs {
args = append(args, uid)
}
}
sql.WriteString(" ORDER BY title ASC")
if q.Limit != 0 {
var offset int64 = 0
if q.Page > 0 {
@@ -259,7 +272,7 @@ func (ss *sqlStore) GetChildren(ctx context.Context, q folder.GetChildrenQuery)
return folder.ErrDatabaseError.Errorf("failed to get folder children: %w", err)
}
if err := concurrency.ForEachJob(ctx, len(folders), len(folders), func(ctx context.Context, idx int) error {
if err := concurrency.ForEachJob(ctx, len(folders), runtime.NumCPU(), func(ctx context.Context, idx int) error {
folders[idx].WithURL()
return nil
}); err != nil {
@@ -642,6 +642,26 @@ func TestIntegrationGetChildren(t *testing.T) {
t.Errorf("Result mismatch (-want +got):\n%s", diff)
}
// fetch folder with specific UIDs and pagination
children, err = folderStore.GetChildren(context.Background(), folder.GetChildrenQuery{
UID: parent.UID,
OrgID: orgID,
Limit: 2,
Page: 1,
FolderUIDs: treeLeaves[3:4],
})
require.NoError(t, err)
childrenUIDs = make([]string, 0, len(children))
for _, c := range children {
assert.NotEmpty(t, c.URL)
childrenUIDs = append(childrenUIDs, c.UID)
}
if diff := cmp.Diff(treeLeaves[3:4], childrenUIDs); diff != "" {
t.Errorf("Result mismatch (-want +got):\n%s", diff)
}
// no page is set
children, err = folderStore.GetChildren(context.Background(), folder.GetChildrenQuery{
UID: parent.UID,
+5 -2
View File
@@ -160,8 +160,8 @@ type GetParentsQuery struct {
// return a list of child folders of the given folder.
type GetChildrenQuery struct {
UID string `xorm:"uid"`
OrgID int64 `xorm:"org_id"`
UID string
OrgID int64
Depth int64
// Pagination options
@@ -169,6 +169,9 @@ type GetChildrenQuery struct {
Page int64
SignedInUser identity.Requester `json:"-"`
// array of folder uids to filter by
FolderUIDs []string `json:"-"`
}
type HasEditPermissionInFoldersQuery struct {
+199
View File
@@ -0,0 +1,199 @@
package folders
import (
"context"
"fmt"
"net/http"
"runtime"
"testing"
"github.com/grafana/dskit/concurrency"
"github.com/grafana/grafana-openapi-client-go/client/folders"
"github.com/grafana/grafana-openapi-client-go/models"
"github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/folder"
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/tests"
"github.com/grafana/grafana/pkg/tests/testinfra"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestGetFolders(t *testing.T) {
// Setup Grafana and its Database
dir, p := testinfra.CreateGrafDir(t, testinfra.GrafanaOpts{
DisableLegacyAlerting: true,
EnableUnifiedAlerting: true,
DisableAnonymous: true,
AppModeProduction: true,
EnableFeatureToggles: []string{featuremgmt.FlagNestedFolders},
})
grafanaListedAddr, store := testinfra.StartGrafana(t, dir, p)
orgID := int64(1)
// Create a users to make authenticated requests
tests.CreateUser(t, store, user.CreateUserCommand{
DefaultOrgRole: string(org.RoleViewer),
OrgID: orgID,
Password: "viewer",
Login: "viewer",
})
tests.CreateUser(t, store, user.CreateUserCommand{
OrgID: orgID,
DefaultOrgRole: string(org.RoleEditor),
Password: "editor",
Login: "editor",
})
tests.CreateUser(t, store, user.CreateUserCommand{
OrgID: orgID,
DefaultOrgRole: string(org.RoleAdmin),
Password: "admin",
Login: "admin",
})
adminClient := tests.GetClient(grafanaListedAddr, "admin", "admin")
editorClient := tests.GetClient(grafanaListedAddr, "editor", "editor")
viewerClient := tests.GetClient(grafanaListedAddr, "viewer", "viewer")
// access control permissions store
permissionsStore := resourcepermissions.NewStore(store, featuremgmt.WithFeatures())
numberOfFolders := 5
indexWithoutPermission := 3
err := concurrency.ForEachJob(context.Background(), numberOfFolders, runtime.NumCPU(), func(_ context.Context, job int) error {
resp, err := adminClient.Folders.CreateFolder(&models.CreateFolderCommand{
Title: fmt.Sprintf("Folder %d", job),
UID: fmt.Sprintf("folder-%d", job),
})
if err != nil {
return err
}
require.Equal(t, http.StatusOK, resp.Code())
if job == indexWithoutPermission {
tests.RemoveFolderPermission(t, permissionsStore, orgID, org.RoleViewer, resp.Payload.UID)
t.Log("Removed viewer permission from folder", resp.Payload.UID)
}
return nil
})
require.NoError(t, err)
t.Run("Admin can get all folders", func(t *testing.T) {
res, err := adminClient.Folders.GetFolders(folders.NewGetFoldersParams())
require.NoError(t, err)
actualFolders := make([]string, 0, len(res.Payload))
for i := range res.Payload {
actualFolders = append(actualFolders, res.Payload[i].UID)
}
assert.Equal(t, []string{"folder-0", "folder-1", "folder-2", "folder-3", "folder-4"}, actualFolders)
})
t.Run("Pagination works as expect for admin", func(t *testing.T) {
limit := int64(2)
page := int64(1)
res, err := adminClient.Folders.GetFolders(folders.NewGetFoldersParams().WithLimit(&limit).WithPage(&page))
require.NoError(t, err)
actualFolders := make([]string, 0, len(res.Payload))
for i := range res.Payload {
actualFolders = append(actualFolders, res.Payload[i].UID)
}
assert.Equal(t, []string{"folder-0", "folder-1"}, actualFolders)
page = int64(2)
res, err = adminClient.Folders.GetFolders(folders.NewGetFoldersParams().WithLimit(&limit).WithPage(&page))
require.NoError(t, err)
actualFolders = make([]string, 0, len(res.Payload))
for i := range res.Payload {
actualFolders = append(actualFolders, res.Payload[i].UID)
}
assert.Equal(t, []string{"folder-2", "folder-3"}, actualFolders)
page = int64(3)
res, err = adminClient.Folders.GetFolders(folders.NewGetFoldersParams().WithLimit(&limit).WithPage(&page))
require.NoError(t, err)
actualFolders = make([]string, 0, len(res.Payload))
for i := range res.Payload {
actualFolders = append(actualFolders, res.Payload[i].UID)
}
assert.Equal(t, []string{"folder-4"}, actualFolders)
})
t.Run("Editor can get all folders", func(t *testing.T) {
res, err := editorClient.Folders.GetFolders(folders.NewGetFoldersParams())
require.NoError(t, err)
actualFolders := make([]string, 0, len(res.Payload))
for i := range res.Payload {
actualFolders = append(actualFolders, res.Payload[i].UID)
}
assert.Equal(t, []string{"folder-0", "folder-1", "folder-2", "folder-3", "folder-4", folder.SharedWithMeFolderUID}, actualFolders)
})
t.Run("Pagination works as expect for editor", func(t *testing.T) {
limit := int64(2)
page := int64(1)
res, err := editorClient.Folders.GetFolders(folders.NewGetFoldersParams().WithLimit(&limit).WithPage(&page))
require.NoError(t, err)
actualFolders := make([]string, 0, len(res.Payload))
for i := range res.Payload {
actualFolders = append(actualFolders, res.Payload[i].UID)
}
assert.Equal(t, []string{"folder-0", "folder-1", folder.SharedWithMeFolderUID}, actualFolders)
page = int64(2)
res, err = editorClient.Folders.GetFolders(folders.NewGetFoldersParams().WithLimit(&limit).WithPage(&page))
require.NoError(t, err)
actualFolders = make([]string, 0, len(res.Payload))
for i := range res.Payload {
actualFolders = append(actualFolders, res.Payload[i].UID)
}
assert.Equal(t, []string{"folder-2", "folder-3"}, actualFolders)
page = int64(3)
res, err = editorClient.Folders.GetFolders(folders.NewGetFoldersParams().WithLimit(&limit).WithPage(&page))
require.NoError(t, err)
actualFolders = make([]string, 0, len(res.Payload))
for i := range res.Payload {
actualFolders = append(actualFolders, res.Payload[i].UID)
}
assert.Equal(t, []string{"folder-4"}, actualFolders)
})
t.Run("Viewer can get only the folders has access too", func(t *testing.T) {
res, err := viewerClient.Folders.GetFolders(folders.NewGetFoldersParams())
require.NoError(t, err)
actualFolders := make([]string, 0, len(res.Payload))
for i := range res.Payload {
actualFolders = append(actualFolders, res.Payload[i].UID)
}
assert.Equal(t, []string{"folder-0", "folder-1", "folder-2", "folder-4", folder.SharedWithMeFolderUID}, actualFolders)
})
t.Run("Pagination works as expect for viewer", func(t *testing.T) {
limit := int64(2)
page := int64(1)
res, err := viewerClient.Folders.GetFolders(folders.NewGetFoldersParams().WithLimit(&limit).WithPage(&page))
require.NoError(t, err)
actualFolders := make([]string, 0, len(res.Payload))
for i := range res.Payload {
actualFolders = append(actualFolders, res.Payload[i].UID)
}
assert.Equal(t, []string{"folder-0", "folder-1", folder.SharedWithMeFolderUID}, actualFolders)
page = int64(2)
res, err = viewerClient.Folders.GetFolders(folders.NewGetFoldersParams().WithLimit(&limit).WithPage(&page))
require.NoError(t, err)
actualFolders = make([]string, 0, len(res.Payload))
for i := range res.Payload {
actualFolders = append(actualFolders, res.Payload[i].UID)
}
assert.Equal(t, []string{"folder-2", "folder-4"}, actualFolders)
page = int64(3)
res, err = viewerClient.Folders.GetFolders(folders.NewGetFoldersParams().WithLimit(&limit).WithPage(&page))
require.NoError(t, err)
assert.Len(t, res.Payload, 0)
})
}
+88
View File
@@ -0,0 +1,88 @@
package tests
import (
"context"
"crypto/tls"
"net/url"
"os"
"testing"
"github.com/go-openapi/strfmt"
goapi "github.com/grafana/grafana-openapi-client-go/client"
"github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions"
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/services/org/orgimpl"
"github.com/grafana/grafana/pkg/services/quota/quotaimpl"
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/supportbundles/supportbundlestest"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/services/user/userimpl"
"github.com/stretchr/testify/require"
)
func CreateUser(t *testing.T, store *sqlstore.SQLStore, cmd user.CreateUserCommand) int64 {
t.Helper()
store.Cfg.AutoAssignOrg = true
store.Cfg.AutoAssignOrgId = 1
quotaService := quotaimpl.ProvideService(store, store.Cfg)
orgService, err := orgimpl.ProvideService(store, store.Cfg, quotaService)
require.NoError(t, err)
usrSvc, err := userimpl.ProvideService(store, orgService, store.Cfg, nil, nil, quotaService, supportbundlestest.NewFakeBundleService())
require.NoError(t, err)
u, err := usrSvc.Create(context.Background(), &cmd)
require.NoError(t, err)
return u.ID
}
func GetClient(host string, username string, password string) *goapi.GrafanaHTTPAPI {
cfg := &goapi.TransportConfig{
// Host is the doman name or IP address of the host that serves the API.
Host: host,
// BasePath is the URL prefix for all API paths, relative to the host root.
BasePath: "/api",
// Schemes are the transfer protocols used by the API (http or https).
Schemes: []string{"http"},
// APIKey is an optional API key or service account token.
APIKey: os.Getenv("API_ACCESS_TOKEN"),
// BasicAuth is optional basic auth credentials.
BasicAuth: url.UserPassword(username, password),
// OrgID provides an optional organization ID.
// OrgID is only supported with BasicAuth since API keys are already org-scoped.
OrgID: 1,
// TLSConfig provides an optional configuration for a TLS client
TLSConfig: &tls.Config{},
// NumRetries contains the optional number of attempted retries
NumRetries: 3,
// RetryTimeout sets an optional time to wait before retrying a request
RetryTimeout: 0,
// RetryStatusCodes contains the optional list of status codes to retry
// Use "x" as a wildcard for a single digit (default: [429, 5xx])
RetryStatusCodes: []string{"420", "5xx"},
// HTTPHeaders contains an optional map of HTTP headers to add to each request
HTTPHeaders: map[string]string{},
}
return goapi.NewHTTPClientWithConfig(strfmt.Default, cfg)
}
func RemoveFolderPermission(t *testing.T, store resourcepermissions.Store, orgID int64, role org.RoleType, uid string) {
t.Helper()
// remove org role permissions from folder
_, _ = store.SetBuiltInResourcePermission(context.Background(), orgID, string(role), resourcepermissions.SetResourcePermissionCommand{
Resource: "folders",
ResourceID: uid,
ResourceAttribute: "uid",
}, nil)
// remove org role children permissions from folder
for _, c := range role.Children() {
_, _ = store.SetBuiltInResourcePermission(context.Background(), orgID, string(c), resourcepermissions.SetResourcePermissionCommand{
Resource: "folders",
ResourceID: uid,
ResourceAttribute: "uid",
}, nil)
}
}