From d89a8a3a82cfe78bf0e473126c167ee3cdd4b9aa Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> Date: Fri, 15 Dec 2023 19:34:08 +0200 Subject: [PATCH] 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 --- go.mod | 13 +- go.sum | 16 ++ pkg/api/folder_bench_test.go | 4 +- pkg/services/folder/folderimpl/folder.go | 121 +++++++---- pkg/services/folder/folderimpl/sqlstore.go | 21 +- .../folder/folderimpl/sqlstore_test.go | 20 ++ pkg/services/folder/model.go | 7 +- pkg/tests/api/folders/api_folders_test.go | 199 ++++++++++++++++++ pkg/tests/utils.go | 88 ++++++++ 9 files changed, 434 insertions(+), 55 deletions(-) create mode 100644 pkg/tests/api/folders/api_folders_test.go create mode 100644 pkg/tests/utils.go diff --git a/go.mod b/go.mod index c823e153b3e..397bbb81197 100644 --- a/go.mod +++ b/go.mod @@ -45,7 +45,7 @@ require ( github.com/fatih/color v1.15.0 // @grafana/backend-platform github.com/gchaincl/sqlhooks v1.3.0 // @grafana/backend-platform github.com/go-ldap/ldap/v3 v3.4.4 // @grafana/grafana-authnz-team - github.com/go-openapi/strfmt v0.21.7 // @grafana/alerting-squad-backend + github.com/go-openapi/strfmt v0.21.9 // @grafana/alerting-squad-backend github.com/go-redis/redis/v8 v8.11.5 // @grafana/backend-platform github.com/go-sourcemap/sourcemap v2.1.3+incompatible // @grafana/backend-platform github.com/go-sql-driver/mysql v1.7.1 // @grafana/backend-platform @@ -153,13 +153,13 @@ require ( github.com/go-logfmt/logfmt v0.6.0 // indirect github.com/go-openapi/analysis v0.21.4 // indirect github.com/go-openapi/errors v0.20.4 // indirect - github.com/go-openapi/jsonpointer v0.19.6 // indirect + github.com/go-openapi/jsonpointer v0.20.0 // indirect github.com/go-openapi/jsonreference v0.20.2 // indirect github.com/go-openapi/loads v0.21.2 // @grafana/alerting-squad-backend - github.com/go-openapi/runtime v0.26.0 // @grafana/alerting-squad-backend - github.com/go-openapi/spec v0.20.9 // indirect + github.com/go-openapi/runtime v0.26.2 // @grafana/alerting-squad-backend + github.com/go-openapi/spec v0.20.11 // indirect github.com/go-openapi/swag v0.22.4 // indirect - github.com/go-openapi/validate v0.22.1 // indirect + github.com/go-openapi/validate v0.22.3 // indirect github.com/golang-jwt/jwt/v4 v4.5.0 // @grafana/backend-platform github.com/golang-sql/civil v0.0.0-20220223132316-b832511892a9 // indirect github.com/golang/glog v1.1.2 // indirect @@ -210,7 +210,7 @@ require ( github.com/uber/jaeger-lib v2.4.1+incompatible // indirect github.com/valyala/bytebufferpool v1.0.0 // indirect github.com/yudai/golcs v0.0.0-20170316035057-ecda9a501e82 // indirect - go.mongodb.org/mongo-driver v1.11.3 // indirect + go.mongodb.org/mongo-driver v1.13.1 // indirect go.opencensus.io v0.24.0 // indirect go.uber.org/atomic v1.11.0 // @grafana/alerting-squad-backend go.uber.org/goleak v1.3.0 // indirect @@ -475,6 +475,7 @@ require ( github.com/go-errors/errors v1.4.2 // indirect github.com/golang-jwt/jwt/v5 v5.0.0 // indirect github.com/google/gnostic-models v0.6.8 // indirect + github.com/grafana/grafana-openapi-client-go v0.0.0-20231213163343-bd475d63fb79 // @grafana/backend-platform github.com/matttproud/golang_protobuf_extensions/v2 v2.0.0 // indirect ) diff --git a/go.sum b/go.sum index 57d0489ea6e..993c5abe970 100644 --- a/go.sum +++ b/go.sum @@ -1216,6 +1216,8 @@ github.com/go-openapi/jsonpointer v0.19.3/go.mod h1:Pl9vOtqEWErmShwVjC8pYs9cog34 github.com/go-openapi/jsonpointer v0.19.5/go.mod h1:Pl9vOtqEWErmShwVjC8pYs9cog34VGT37dQOVbmoatg= github.com/go-openapi/jsonpointer v0.19.6 h1:eCs3fxoIi3Wh6vtgmLTOjdhSpiqphQ+DaPn38N2ZdrE= github.com/go-openapi/jsonpointer v0.19.6/go.mod h1:osyAmYz/mB/C3I+WsTTSgw1ONzaLJoLCyoi6/zppojs= +github.com/go-openapi/jsonpointer v0.20.0 h1:ESKJdU9ASRfaPNOPRx12IUyA1vn3R9GiE3KYD14BXdQ= +github.com/go-openapi/jsonpointer v0.20.0/go.mod h1:6PGzBjjIIumbLYysB73Klnms1mwnU4G3YHOECG3CedA= github.com/go-openapi/jsonreference v0.17.0/go.mod h1:g4xxGn04lDIRh0GJb5QlpE3HfopLOL6uZrK/VgnsK9I= github.com/go-openapi/jsonreference v0.18.0/go.mod h1:g4xxGn04lDIRh0GJb5QlpE3HfopLOL6uZrK/VgnsK9I= github.com/go-openapi/jsonreference v0.19.2/go.mod h1:jMjeRr2HHw6nAVajTXJ4eiUwohSTlpa0o73RUL1owJc= @@ -1241,6 +1243,8 @@ github.com/go-openapi/runtime v0.19.15/go.mod h1:dhGWCTKRXlAfGnQG0ONViOZpjfg0m2g github.com/go-openapi/runtime v0.19.26/go.mod h1:BvrQtn6iVb2QmiVXRsFAm6ZCAZBpbVKFfN6QWCp582M= github.com/go-openapi/runtime v0.26.0 h1:HYOFtG00FM1UvqrcxbEJg/SwvDRvYLQKGhw2zaQjTcc= github.com/go-openapi/runtime v0.26.0/go.mod h1:QgRGeZwrUcSHdeh4Ka9Glvo0ug1LC5WyE+EV88plZrQ= +github.com/go-openapi/runtime v0.26.2 h1:elWyB9MacRzvIVgAZCBJmqTi7hBzU0hlKD4IvfX0Zl0= +github.com/go-openapi/runtime v0.26.2/go.mod h1:O034jyRZ557uJKzngbMDJXkcKJVzXJiymdSfgejrcRw= github.com/go-openapi/spec v0.17.0/go.mod h1:XkF/MOi14NmjsfZ8VtAKf8pIlbZzyoTvZsdfssdxcBI= github.com/go-openapi/spec v0.18.0/go.mod h1:XkF/MOi14NmjsfZ8VtAKf8pIlbZzyoTvZsdfssdxcBI= github.com/go-openapi/spec v0.19.2/go.mod h1:sCxk3jxKgioEJikev4fgkNmwS+3kuYdJtcsZsD5zxMY= @@ -1252,6 +1256,8 @@ github.com/go-openapi/spec v0.20.6/go.mod h1:2OpW+JddWPrpXSCIX8eOx7lZ5iyuWj3RYR6 github.com/go-openapi/spec v0.20.8/go.mod h1:2OpW+JddWPrpXSCIX8eOx7lZ5iyuWj3RYR6VaaBKcWA= github.com/go-openapi/spec v0.20.9 h1:xnlYNQAwKd2VQRRfwTEI0DcK+2cbuvI/0c7jx3gA8/8= github.com/go-openapi/spec v0.20.9/go.mod h1:2OpW+JddWPrpXSCIX8eOx7lZ5iyuWj3RYR6VaaBKcWA= +github.com/go-openapi/spec v0.20.11 h1:J/TzFDLTt4Rcl/l1PmyErvkqlJDncGvPTMnCI39I4gY= +github.com/go-openapi/spec v0.20.11/go.mod h1:2OpW+JddWPrpXSCIX8eOx7lZ5iyuWj3RYR6VaaBKcWA= github.com/go-openapi/strfmt v0.17.0/go.mod h1:P82hnJI0CXkErkXi8IKjPbNBM6lV6+5pLP5l494TcyU= github.com/go-openapi/strfmt v0.18.0/go.mod h1:P82hnJI0CXkErkXi8IKjPbNBM6lV6+5pLP5l494TcyU= github.com/go-openapi/strfmt v0.19.0/go.mod h1:+uW+93UVvGGq2qGaZxdDeJqSAqBqBdl+ZPMF/cC8nDY= @@ -1264,6 +1270,8 @@ github.com/go-openapi/strfmt v0.21.1/go.mod h1:I/XVKeLc5+MM5oPNN7P6urMOpuLXEcNrC github.com/go-openapi/strfmt v0.21.3/go.mod h1:k+RzNO0Da+k3FrrynSNN8F7n/peCmQQqbbXjtDfvmGg= github.com/go-openapi/strfmt v0.21.7 h1:rspiXgNWgeUzhjo1YU01do6qsahtJNByjLVbPLNHb8k= github.com/go-openapi/strfmt v0.21.7/go.mod h1:adeGTkxE44sPyLk0JV235VQAO/ZXUr8KAzYjclFs3ew= +github.com/go-openapi/strfmt v0.21.9 h1:LnEGOO9qyEC1v22Bzr323M98G13paIUGPU7yeJtG9Xs= +github.com/go-openapi/strfmt v0.21.9/go.mod h1:0k3v301mglEaZRJdDDGSlN6Npq4VMVU69DE0LUyf7uA= github.com/go-openapi/swag v0.17.0/go.mod h1:AByQ+nYG6gQg71GINrmuDXCPWdL640yX49/kXLo40Tg= github.com/go-openapi/swag v0.18.0/go.mod h1:AByQ+nYG6gQg71GINrmuDXCPWdL640yX49/kXLo40Tg= github.com/go-openapi/swag v0.19.2/go.mod h1:POnQmlKehdgb5mhVOsnJFsivZCEZ/vjK9gh66Z9tfKk= @@ -1282,6 +1290,8 @@ github.com/go-openapi/validate v0.19.3/go.mod h1:90Vh6jjkTn+OT1Eefm0ZixWNFjhtOH7 github.com/go-openapi/validate v0.19.10/go.mod h1:RKEZTUWDkxKQxN2jDT7ZnZi2bhZlbNMAuKvKB+IaGx8= github.com/go-openapi/validate v0.22.1 h1:G+c2ub6q47kfX1sOBLwIQwzBVt8qmOAARyo/9Fqs9NU= github.com/go-openapi/validate v0.22.1/go.mod h1:rjnrwK57VJ7A8xqfpAOEKRH8yQSGUriMu5/zuPSQ1hg= +github.com/go-openapi/validate v0.22.3 h1:KxG9mu5HBRYbecRb37KRCihvGGtND2aXziBAv0NNfyI= +github.com/go-openapi/validate v0.22.3/go.mod h1:kVxh31KbfsxU8ZyoHaDbLBWU5CnMdqBUEtadQ2G4d5M= github.com/go-pdf/fpdf v0.5.0/go.mod h1:HzcnA+A23uwogo0tp9yU+l3V+KXhiESpt1PMayhOh5M= github.com/go-pdf/fpdf v0.6.0/go.mod h1:HzcnA+A23uwogo0tp9yU+l3V+KXhiESpt1PMayhOh5M= github.com/go-playground/assert/v2 v2.0.1/go.mod h1:VDjEfimB/XKnb+ZQfWdccd7VUvScMdVu0Titje2rxJ4= @@ -1822,6 +1832,8 @@ github.com/grafana/grafana-azure-sdk-go v1.11.0 h1:nc6MgOZ5fIaxvBfZjYU5rSqB4zaD7 github.com/grafana/grafana-azure-sdk-go v1.11.0/go.mod h1:5a3FuG2lEsYNop9HDNgTO1bx4ExCgsjvrFhpuqolYAU= github.com/grafana/grafana-google-sdk-go v0.1.0 h1:LKGY8z2DSxKjYfr2flZsWgTRTZ6HGQbTqewE3JvRaNA= github.com/grafana/grafana-google-sdk-go v0.1.0/go.mod h1:Vo2TKWfDVmNTELBUM+3lkrZvFtBws0qSZdXhQxRdJrE= +github.com/grafana/grafana-openapi-client-go v0.0.0-20231213163343-bd475d63fb79 h1:r+mU5bGMzcXCRVAuOrTn54S80qbfVkvTdUJZfSfTNbs= +github.com/grafana/grafana-openapi-client-go v0.0.0-20231213163343-bd475d63fb79/go.mod h1:wc6Hbh3K2TgCUSfBC/BOzabItujtHMESZeFk5ZhdxhQ= github.com/grafana/grafana-plugin-sdk-go v0.94.0/go.mod h1:3VXz4nCv6wH5SfgB3mlW39s+c+LetqSCjFj7xxPC5+M= github.com/grafana/grafana-plugin-sdk-go v0.114.0/go.mod h1:D7x3ah+1d4phNXpbnOaxa/osSaZlwh9/ZUnGGzegRbk= github.com/grafana/grafana-plugin-sdk-go v0.197.0 h1:5oUAQfa3gv5AX8Qhkoyuaj5E4hXbdR5mfa9P4zNQ0IE= @@ -2888,8 +2900,10 @@ github.com/xanzy/go-gitlab v0.15.0/go.mod h1:8zdQa/ri1dfn8eS3Ir1SyfvOKlw7WBJ8DVT github.com/xdg-go/pbkdf2 v1.0.0/go.mod h1:jrpuAogTd400dnrH08LKmI/xc1MbPOebTwRqcT5RDeI= github.com/xdg-go/scram v1.0.2/go.mod h1:1WAq6h33pAW+iRreB34OORO2Nf7qel3VV3fjBj+hCSs= github.com/xdg-go/scram v1.1.1/go.mod h1:RaEWvsqvNKKvBPvcKeFjrG2cJqOkHTiyTpzz23ni57g= +github.com/xdg-go/scram v1.1.2/go.mod h1:RT/sEzTbU5y00aCK8UOx6R7YryM0iF1N2MOmC3kKLN4= github.com/xdg-go/stringprep v1.0.2/go.mod h1:8F9zXuvzgwmyT5DUm4GUfZGDdT3W+LCvS6+da4O5kxM= github.com/xdg-go/stringprep v1.0.3/go.mod h1:W3f5j4i+9rC0kuIEJL0ky1VpHXQU3ocBgklLGvcBnW8= +github.com/xdg-go/stringprep v1.0.4/go.mod h1:mPGuuIYwz7CmR2bT9j4GbQqutWS1zV24gijq1dTyGkM= github.com/xdg/scram v0.0.0-20180814205039-7eeb5667e42c/go.mod h1:lB8K/P019DLNhemzwFU4jHLhdvlE6uDZjXFejJXr49I= github.com/xdg/stringprep v0.0.0-20180714160509-73f8eece6fdc/go.mod h1:Jhud4/sHMO4oL310DaZAKk9ZaJ08SJfe+sJh0HrGL1Y= github.com/xdg/stringprep v1.0.0/go.mod h1:Jhud4/sHMO4oL310DaZAKk9ZaJ08SJfe+sJh0HrGL1Y= @@ -2980,6 +2994,8 @@ go.mongodb.org/mongo-driver v1.10.0/go.mod h1:wsihk0Kdgv8Kqu1Anit4sfK+22vSFbUrAV go.mongodb.org/mongo-driver v1.11.2/go.mod h1:s7p5vEtfbeR1gYi6pnj3c3/urpbLv2T5Sfd6Rp2HBB8= go.mongodb.org/mongo-driver v1.11.3 h1:Ql6K6qYHEzB6xvu4+AU0BoRoqf9vFPcc4o7MUIdPW8Y= go.mongodb.org/mongo-driver v1.11.3/go.mod h1:PTSz5yu21bkT/wXpkS7WR5f0ddqw5quethTUn9WM+2g= +go.mongodb.org/mongo-driver v1.13.1 h1:YIc7HTYsKndGK4RFzJ3covLz1byri52x0IoMB0Pt/vk= +go.mongodb.org/mongo-driver v1.13.1/go.mod h1:wcDf1JBCXy2mOW0bWHwO/IOYqdca1MPCwDtFu/Z9+eo= go.opencensus.io v0.15.0/go.mod h1:UffZAU+4sDEINUGP/B7UfBBkq4fqLu9zXAX7ke6CHW0= go.opencensus.io v0.20.1/go.mod h1:6WKK9ahsWS3RSO+PY9ZHZUfv2irvY6gN279GOPZjmmk= go.opencensus.io v0.20.2/go.mod h1:6WKK9ahsWS3RSO+PY9ZHZUfv2irvY6gN279GOPZjmmk= diff --git a/pkg/api/folder_bench_test.go b/pkg/api/folder_bench_test.go index 894754c5e3d..50510916d24 100644 --- a/pkg/api/folder_bench_test.go +++ b/pkg/api/folder_bench_test.go @@ -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), }, { diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index ebf82e61ff8..7d2a776b8bb 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -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) { diff --git a/pkg/services/folder/folderimpl/sqlstore.go b/pkg/services/folder/folderimpl/sqlstore.go index c37ea8b0015..75aa5e8f794 100644 --- a/pkg/services/folder/folderimpl/sqlstore.go +++ b/pkg/services/folder/folderimpl/sqlstore.go @@ -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 { diff --git a/pkg/services/folder/folderimpl/sqlstore_test.go b/pkg/services/folder/folderimpl/sqlstore_test.go index b416713c839..bd711630f78 100644 --- a/pkg/services/folder/folderimpl/sqlstore_test.go +++ b/pkg/services/folder/folderimpl/sqlstore_test.go @@ -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, diff --git a/pkg/services/folder/model.go b/pkg/services/folder/model.go index e43c5ac662c..a28f24fe8f4 100644 --- a/pkg/services/folder/model.go +++ b/pkg/services/folder/model.go @@ -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 { diff --git a/pkg/tests/api/folders/api_folders_test.go b/pkg/tests/api/folders/api_folders_test.go new file mode 100644 index 00000000000..f58c08338cd --- /dev/null +++ b/pkg/tests/api/folders/api_folders_test.go @@ -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) + }) +} diff --git a/pkg/tests/utils.go b/pkg/tests/utils.go new file mode 100644 index 00000000000..47f85298738 --- /dev/null +++ b/pkg/tests/utils.go @@ -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) + } +}