Storage: refactor filtering, improve performance (#47403)

* Storage: refactor filtering, improve performance

* added a comment to `newpathfilter`

* after merge fixes

go.mod

@@ -246,6 +246,7 @@ require (
 	github.com/Azure/azure-sdk-for-go/sdk/azidentity v0.13.2
 	github.com/Azure/azure-sdk-for-go/sdk/keyvault/azkeys v0.4.0
 	github.com/Azure/go-autorest/autorest/adal v0.9.17
+	github.com/armon/go-radix v1.0.0
 	github.com/golang-migrate/migrate/v4 v4.7.0
 	github.com/grafana/dskit v0.0.0-20211011144203-3a88ec0b675f
 	github.com/grafana/thema v0.0.0-20220413232647-fc54c169b508

go.sum

@@ -347,6 +347,7 @@ github.com/armon/go-metrics v0.3.8/go.mod h1:4O98XIr/9W0sxpJ8UaYkvjk10Iff7SnFrb4
 github.com/armon/go-metrics v0.3.10 h1:FR+drcQStOe+32sYyJYyZ7FIdgoGGBnwLl+flodp8Uo=
 github.com/armon/go-metrics v0.3.10/go.mod h1:4O98XIr/9W0sxpJ8UaYkvjk10Iff7SnFrb4QAOwNTFc=
 github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8=
+github.com/armon/go-radix v1.0.0 h1:F4z6KzEeeQIMeLFa97iZU6vupzoecKdU5TX24SNppXI=
 github.com/armon/go-radix v1.0.0/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8=
 github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5/go.mod h1:wHh0iHkYZB8zMSxRWpUBQtwG5a7fFgvEO+odwuTv2gs=
 github.com/aryann/difflib v0.0.0-20170710044230-e206f873d14a/go.mod h1:DAHtR1m6lCRdSC2Tm3DSWRPvIPr6xNKyeHdqDQSQT+A=

pkg/infra/filestorage/api.go

@@ -60,13 +60,6 @@ type UpsertFileCommand struct {
 	Properties map[string]string
 }
 
-type PathFilters struct {
-	allowedPrefixes    []string
-	disallowedPrefixes []string
-	allowedPaths       []string
-	disallowedPaths    []string
-}
-
 func toLower(list []string) []string {
 	if list == nil {
 		return nil
@@ -78,63 +71,6 @@ func toLower(list []string) []string {
 	return lower
 }
 
-func allowAllPathFilters() *PathFilters {
-	return NewPathFilters(nil, nil, nil, nil)
-}
-
-//nolint:deadcode,unused
-func denyAllPathFilters() *PathFilters {
-	return NewPathFilters([]string{}, []string{}, nil, nil)
-}
-
-func NewPathFilters(allowedPrefixes []string, allowedPaths []string, disallowedPrefixes []string, disallowedPaths []string) *PathFilters {
-	return &PathFilters{
-		allowedPrefixes:    toLower(allowedPrefixes),
-		allowedPaths:       toLower(allowedPaths),
-		disallowedPaths:    toLower(disallowedPaths),
-		disallowedPrefixes: toLower(disallowedPrefixes),
-	}
-}
-
-func (f *PathFilters) isDenyAll() bool {
-	return f.allowedPaths != nil && f.allowedPrefixes != nil && (len(f.allowedPaths)+len(f.allowedPrefixes) == 0)
-}
-
-func (f *PathFilters) IsAllowed(path string) bool {
-	if f == nil {
-		return true
-	}
-
-	path = strings.ToLower(path)
-	for i := range f.disallowedPaths {
-		if f.disallowedPaths[i] == path {
-			return false
-		}
-	}
-
-	for i := range f.disallowedPrefixes {
-		if strings.HasPrefix(path, f.disallowedPrefixes[i]) {
-			return false
-		}
-	}
-
-	if f.allowedPrefixes == nil && f.allowedPaths == nil {
-		return true
-	}
-
-	for i := range f.allowedPaths {
-		if f.allowedPaths[i] == path {
-			return true
-		}
-	}
-	for i := range f.allowedPrefixes {
-		if strings.HasPrefix(path, f.allowedPrefixes[i]) {
-			return true
-		}
-	}
-	return false
-}
-
 type ListResponse struct {
 	Files    []*File
 	HasMore  bool
@@ -146,7 +82,7 @@ type ListOptions struct {
 	WithFiles    bool
 	WithFolders  bool
 	WithContents bool
-	*PathFilters
+	Filter       PathFilter
 }
 
 type FileStorage interface {

pkg/infra/filestorage/cdk_blob_filestorage.go

@@ -24,11 +24,11 @@ type cdkBlobStorage struct {
 	bucket *blob.Bucket
 }
 
-func NewCdkBlobStorage(log log.Logger, bucket *blob.Bucket, rootFolder string, pathFilters *PathFilters) FileStorage {
+func NewCdkBlobStorage(log log.Logger, bucket *blob.Bucket, rootFolder string, filter PathFilter) FileStorage {
 	return newWrapper(log, &cdkBlobStorage{
 		log:    log,
 		bucket: bucket,
-	}, pathFilters, rootFolder)
+	}, filter, rootFolder)
 }
 
 func (c cdkBlobStorage) Get(ctx context.Context, filePath string) (*File, error) {
@@ -266,7 +266,7 @@ func (c cdkBlobStorage) list(ctx context.Context, folderPath string, paging *Pag
 
 		path := obj.Key
 		lowerPath := strings.ToLower(path)
-		allowed := options.IsAllowed(lowerPath)
+		allowed := options.Filter.IsAllowed(lowerPath)
 
 		if obj.IsDir && recursive && !visitedFolders[lowerPath] {
 			iterators = append([]*blob.ListIterator{c.bucket.List(&blob.ListOptions{

pkg/infra/filestorage/db_filestorage.go

@@ -31,11 +31,11 @@ type dbFileStorage struct {
 	log log.Logger
 }
 
-func NewDbStorage(log log.Logger, db *sqlstore.SQLStore, pathFilters *PathFilters, rootFolder string) FileStorage {
+func NewDbStorage(log log.Logger, db *sqlstore.SQLStore, filter PathFilter, rootFolder string) FileStorage {
 	return newWrapper(log, &dbFileStorage{
 		log: log,
 		db:  db,
-	}, pathFilters, rootFolder)
+	}, filter, rootFolder)
 }
 
 func (s dbFileStorage) getProperties(sess *sqlstore.DBSession, lowerCasePaths []string) (map[string]map[string]string, error) {
@@ -268,40 +268,8 @@ func (s dbFileStorage) List(ctx context.Context, folderPath string, paging *Pagi
 			sess.Where("path LIKE ?", "%/")
 		}
 
-		if len(options.PathFilters.allowedPrefixes)+len(options.PathFilters.allowedPaths) > 0 {
-			queries := make([]string, 0)
-			args := make([]interface{}, 0)
-			for _, prefix := range options.PathFilters.allowedPrefixes {
-				queries = append(queries, "LOWER(path) LIKE ?")
-				args = append(args, prefix+"%")
-			}
-
-			for _, path := range options.PathFilters.allowedPaths {
-				queries = append(queries, "LOWER(path) = ?")
-				args = append(args, path)
-			}
-			sess.Where(strings.Join(queries, " OR "), args...)
-		}
-
-		if options.PathFilters.disallowedPrefixes != nil && len(options.PathFilters.disallowedPrefixes) > 0 {
-			queries := make([]string, 0)
-			args := make([]interface{}, 0)
-			for _, prefix := range options.PathFilters.disallowedPrefixes {
-				queries = append(queries, "LOWER(path) NOT LIKE ?")
-				args = append(args, prefix+"%")
-			}
-			sess.Where(strings.Join(queries, " AND "), args...)
-		}
-
-		if options.PathFilters.disallowedPaths != nil && len(options.PathFilters.disallowedPaths) > 0 {
-			queries := make([]string, 0)
-			args := make([]interface{}, 0)
-			for _, path := range options.PathFilters.disallowedPaths {
-				queries = append(queries, "LOWER(path) != ?")
-				args = append(args, path)
-			}
-			sess.Where(strings.Join(queries, " AND "), args...)
-		}
+		sqlFilter := options.Filter.asSQLFilter()
+		sess.Where(sqlFilter.Where, sqlFilter.Args...)
 
 		sess.OrderBy("path")
 

pkg/infra/filestorage/filter.go

@@ -0,0 +1,249 @@
+package filestorage
+
+import (
+	"strings"
+
+	"github.com/armon/go-radix"
+	"github.com/grafana/grafana/pkg/services/accesscontrol"
+)
+
+type treeValue string
+
+const (
+	treeValueDisallowedPath   treeValue = "!"
+	treeValueDisallowedPrefix treeValue = "!*"
+	treeValueAllowedPath      treeValue = ""
+	treeValueAllowedPrefix    treeValue = "*"
+)
+
+type PathFilter interface {
+	IsAllowed(path string) bool
+	ToString() string
+
+	asSQLFilter() accesscontrol.SQLFilter
+}
+
+// NewPathFilter factory function for a tree-based PathFilter
+// `nil` and empty arrays are treated in a different way. The PathFilter will not perform checks associated with a `nil` array, examples:
+//   - `allowedPrefixes` & `allowedPaths` are nil -> all paths are allowed (unless included in `disallowedX` arrays)
+//   - `allowedPrefixes` & `allowedPaths` are both empty -> no paths are allowed, regardless of what is inside `disallowedX` arrays
+//   - `allowedPrefixes` is nil, `allowedPaths` is not nil. -> only paths specified in `allowedPaths` are allowed (unless included in `disallowedX` arrays)
+func NewPathFilter(allowedPrefixes []string, allowedPaths []string, disallowedPrefixes []string, disallowedPaths []string) PathFilter {
+	if allowedPrefixes != nil && allowedPaths != nil && (len(allowedPrefixes)+len(allowedPaths) == 0) {
+		return NewDenyAllPathFilter()
+	}
+
+	if allowedPaths == nil && allowedPrefixes == nil && (len(disallowedPaths)+len(disallowedPrefixes) == 0) {
+		return NewAllowAllPathFilter()
+	}
+
+	return &radixTreePathFilter{
+		tree: createRadixTree(toLower(allowedPrefixes), toLower(allowedPaths), toLower(disallowedPrefixes), toLower(disallowedPaths)),
+	}
+}
+
+func NewAllowAllPathFilter() PathFilter {
+	return &radixTreePathFilter{
+		tree: createRadixTree([]string{Delimiter}, nil, nil, nil),
+	}
+}
+
+func NewDenyAllPathFilter() PathFilter {
+	return &radixTreePathFilter{
+		tree: createRadixTree(nil, nil, []string{Delimiter}, nil),
+	}
+}
+
+type radixTreePathFilter struct {
+	tree *radix.Tree
+}
+
+func (r *radixTreePathFilter) asSQLFilter() accesscontrol.SQLFilter {
+	denyConditions := make([]string, 0)
+	denyArgs := make([]interface{}, 0)
+
+	allowConditions := make([]string, 0)
+	allowArgs := make([]interface{}, 0)
+
+	r.tree.Walk(func(path string, v interface{}) bool {
+		switch v.(treeValue) {
+		case treeValueAllowedPrefix:
+			allowConditions = append(allowConditions, "LOWER(PATH) LIKE ?")
+			allowArgs = append(allowArgs, path+"%")
+		case treeValueAllowedPath:
+			allowConditions = append(allowConditions, "LOWER(PATH) = ?")
+			allowArgs = append(allowArgs, path)
+		case treeValueDisallowedPrefix:
+			denyConditions = append(denyConditions, "LOWER(PATH) NOT LIKE ? ")
+			denyArgs = append(denyArgs, path+"%")
+		case treeValueDisallowedPath:
+			denyConditions = append(denyConditions, "LOWER(PATH) != ?")
+			denyArgs = append(denyArgs, path)
+		}
+		return false
+	})
+
+	if len(denyConditions)+len(allowConditions) == 0 {
+		return accesscontrol.SQLFilter{Where: "1 = 1", Args: nil}
+	}
+
+	allowQuery := strings.Join(allowConditions, " OR ")
+	denyQuery := strings.Join(denyConditions, " AND ")
+
+	if len(allowConditions) == 0 {
+		return accesscontrol.SQLFilter{
+			Where: denyQuery,
+			Args:  denyArgs,
+		}
+	}
+
+	if len(denyConditions) == 0 {
+		return accesscontrol.SQLFilter{
+			Where: allowQuery,
+			Args:  allowArgs,
+		}
+	}
+
+	return accesscontrol.SQLFilter{
+		Where: "(" + allowQuery + ") AND ( " + denyQuery + " )",
+		Args:  append(allowArgs, denyArgs...),
+	}
+}
+
+func (r *radixTreePathFilter) ToString() string {
+	builder := strings.Builder{}
+	r.tree.Walk(func(s string, v interface{}) bool {
+		if builder.Len() != 0 {
+			builder.WriteString("\n")
+		}
+		switch v.(treeValue) {
+		case treeValueAllowedPrefix:
+			builder.WriteString(s)
+			builder.WriteString("*")
+		case treeValueAllowedPath:
+			builder.WriteString(s)
+		case treeValueDisallowedPrefix:
+			builder.WriteString("!")
+			builder.WriteString(s)
+			builder.WriteString("*")
+		case treeValueDisallowedPath:
+			builder.WriteString("!")
+			builder.WriteString(s)
+		}
+		return false
+	})
+	return builder.String()
+}
+
+func (r *radixTreePathFilter) IsAllowed(path string) bool {
+	path = strings.ToLower(path)
+
+	allowed := false
+	denied := false
+	r.tree.WalkPath(path, func(s string, v interface{}) bool {
+		if v == treeValueDisallowedPrefix || (s == path && v == treeValueDisallowedPath) {
+			denied = true
+			return true
+		}
+
+		if v == treeValueAllowedPrefix || (s == path && v == treeValueAllowedPath) {
+			allowed = true
+			// have to keep traversing to look for explicit denies
+		}
+		return false
+	})
+
+	if denied {
+		return false
+	}
+
+	return allowed
+}
+
+func createRadixTree(allowedPrefixes []string, allowedPaths []string, disallowedPrefixes []string, disallowedPaths []string) *radix.Tree {
+	tree := radix.New()
+	for _, disallowedPrefix := range disallowedPrefixes {
+		tree.Insert(disallowedPrefix, treeValueDisallowedPrefix)
+	}
+
+	for _, disallowedPath := range disallowedPaths {
+		tree.Insert(disallowedPath, treeValueDisallowedPath)
+	}
+
+	for _, allowedPath := range allowedPaths {
+		isDenied := false
+		tree.WalkPath(allowedPath, func(s string, v interface{}) bool {
+			if v == treeValueDisallowedPrefix || s == allowedPath {
+				isDenied = true
+				return true
+			}
+			return false
+		})
+
+		if !isDenied {
+			tree.Insert(allowedPath, treeValueAllowedPath)
+		}
+	}
+
+	for _, allowedPrefix := range allowedPrefixes {
+		isDenied := false
+		tree.WalkPath(allowedPrefix, func(s string, v interface{}) bool {
+			if v == treeValueDisallowedPrefix {
+				isDenied = true
+				return true
+			}
+			return false
+		})
+
+		if !isDenied {
+			tree.Insert(allowedPrefix, treeValueAllowedPrefix)
+		}
+	}
+
+	return tree
+}
+
+type allOfPathFilter struct {
+	filters []PathFilter
+}
+
+func (m allOfPathFilter) IsAllowed(path string) bool {
+	for _, filter := range m.filters {
+		isAllowed := filter.IsAllowed(path)
+		if !isAllowed {
+			return false
+		}
+	}
+	return true
+}
+
+func (m allOfPathFilter) ToString() string {
+	s := strings.Builder{}
+	for _, filter := range m.filters {
+		if s.Len() != 0 {
+			s.WriteString("\n\nAND\n")
+		}
+		s.WriteString(filter.ToString())
+	}
+	return s.String()
+}
+
+func (m allOfPathFilter) asSQLFilter() accesscontrol.SQLFilter {
+	queries := make([]string, 0)
+
+	args := make([]interface{}, 0)
+	for _, filter := range m.filters {
+		sqlFilter := filter.asSQLFilter()
+		queries = append(queries, "("+sqlFilter.Where+")")
+		args = append(args, sqlFilter.Args...)
+	}
+
+	return accesscontrol.SQLFilter{
+		Where: "(" + strings.Join(queries, " AND ") + ")",
+		Args:  args,
+	}
+}
+
+func newAndPathFilter(filters ...PathFilter) PathFilter {
+	return &allOfPathFilter{filters: filters}
+}

pkg/infra/filestorage/filter_test.go

@@ -0,0 +1,135 @@
+package filestorage
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestAuth(t *testing.T) {
+	var tests = []struct {
+		name          string
+		filter        PathFilter
+		expectedDeny  []string
+		expectedAllow []string
+	}{
+		{
+			name:   "can not access anything without an allow rule (deny by default)",
+			filter: NewPathFilter([]string{"/b"}, nil, nil, nil),
+			expectedDeny: []string{
+				"/a/b.jpg",
+				"/a/",
+				"/c",
+				"/",
+			},
+			expectedAllow: []string{
+				"/b/",
+				"/b/a.jpg",
+			},
+		},
+		{
+			name:   "can access any path with / prefix and no denies",
+			filter: NewPathFilter([]string{"/"}, nil, nil, nil),
+			expectedAllow: []string{
+				"/a/b/c/d/e.jpg",
+			},
+		},
+		{
+			name:   "can not access paths which are explicitly denied",
+			filter: NewPathFilter([]string{"/"}, nil, []string{"/x/"}, []string{"/a/b/c/d/e.jpg"}),
+			expectedDeny: []string{
+				"/a/b/c/d/e.jpg",
+				"/x/",
+				"/x/a.jpg",
+				"/x/a/b.jpg",
+			},
+			expectedAllow: []string{
+				"/a/b/c/d/x.jpg",
+				"/a/b/c/d/",
+			},
+		},
+		{
+			name:   "can not access paths with denied prefixes - parent folder",
+			filter: NewPathFilter([]string{"/"}, nil, []string{"/a/b/c/"}, []string{"/a/b/c/d/e.jpg"}),
+			expectedDeny: []string{
+				"/a/b/c/d/e.jpg",
+			},
+			expectedAllow: []string{
+				"/a/b/x/d/e.jpg",
+			},
+		},
+		{
+			name:   "can not access paths with denied prefixes - root folder",
+			filter: NewPathFilter([]string{"/"}, nil, []string{"/"}, nil),
+			expectedDeny: []string{
+				"/a/b/c/d/e.jpg",
+				"/",
+				"/a.jpg",
+			},
+		},
+		{
+			name:   "can not access paths with denied prefixes - same folder",
+			filter: NewPathFilter([]string{"/"}, nil, []string{"/a/b/c/d/e"}, nil),
+			expectedDeny: []string{
+				"/a/b/c/d/e.jpg",
+			},
+		},
+		{
+			name:   "can not access paths with denied prefixes - parent folder with a dot",
+			filter: NewPathFilter([]string{"/"}, nil, []string{"/a/b/c/d."}, nil),
+			expectedDeny: []string{
+				"/a/b/c/d.e/f.jpg",
+			},
+			expectedAllow: []string{
+				"/a/b/c/e.jpg",
+			},
+		},
+		{
+			name:   "can not access paths with denied prefixes even if path is explicitly allowed - deny takes priority",
+			filter: NewPathFilter([]string{"/"}, []string{"/a/b/c/d/f.jpg"}, []string{"/a/"}, nil),
+			expectedDeny: []string{
+				"/a/b/c/d/f.jpg",
+			},
+		},
+		{
+			name: "multiple rules",
+			filter: NewPathFilter(
+				[]string{"/gitB/", "/s3/folder/", "/gitC/"},
+				[]string{"/gitA/dashboard2.json"},
+				[]string{"/s3/folder/nested/"},
+				[]string{"/gitC/nestedC/"},
+			),
+			expectedAllow: []string{
+				"/gitA/dashboard2.json",
+				"/gitB/",
+				"/gitB/nested/",
+				"/gitB/nested/dashboard.json",
+				"/gitB/nested2/dashboard2.json",
+				"/gitC/",
+				"/gitC/nestedC/dashboardC.json",
+				"/s3/folder/",
+				"/s3/folder/file.jpg",
+				"/s3/folder/nested2/file.jpg",
+			},
+			expectedDeny: []string{
+				"/gitA/dashboard.json",             // not explicitly allowed
+				"/s3/folder/nested/dashboard.json", // denied with '/s3/folder/nested/' prefix
+				"/s3/nestedC/",                     // not explicitly allowed
+				"/s3/anyFile.jpg",                  // not explicitly allowed
+				"/s3/",                             // not explicitly allowed
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			for _, expectedAllow := range tt.expectedAllow {
+				require.Truef(t, tt.filter.IsAllowed(expectedAllow), "expected access to %s", expectedAllow)
+			}
+
+			for _, expectedDeny := range tt.expectedDeny {
+				require.Falsef(t, tt.filter.IsAllowed(expectedDeny), "expected no access to %s", expectedDeny)
+			}
+		})
+	}
+}

pkg/infra/filestorage/fs_integration_test.go

@@ -95,7 +95,6 @@ func runTests(createCases func() []fsTestCase, t *testing.T) {
 	setupLocalFsNestedPath := func() {
 		commonSetup()
 		tmpDir, err := ioutil.TempDir("", "")
-		tempDir = tmpDir
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -318,6 +317,13 @@ func TestFsStorage(t *testing.T) {
 							Properties: map[string]string{"prop1": "val1"},
 						},
 					},
+					cmdUpsert{
+						cmd: UpsertFileCommand{
+							Path:       "/dashboards/dashboards/file-inner.jpg",
+							Contents:   emptyContents,
+							Properties: map[string]string{"prop1": "val1"},
+						},
+					},
 					queryListFiles{
 						input: queryListFilesInput{path: "/folder1/file-inner.jp", options: &ListOptions{Recursive: true}},
 						list:  checks(listSize(0), listHasMore(false), listLastPath("")),
@@ -355,6 +361,13 @@ func TestFsStorage(t *testing.T) {
 							checks(fPath("/folder1/file-inner.jpg"), fName("file-inner.jpg"), fProperties(map[string]string{"prop1": "val1"})),
 						},
 					},
+					queryListFiles{
+						input: queryListFilesInput{path: "/dashboards/dashboards/file-inner.jpg", options: &ListOptions{Recursive: true, WithFiles: true}},
+						list:  checks(listSize(1), listHasMore(false), listLastPath("/dashboards/dashboards/file-inner.jpg")),
+						files: [][]interface{}{
+							checks(fPath("/dashboards/dashboards/file-inner.jpg"), fName("file-inner.jpg")),
+						},
+					},
 					queryListFiles{
 						input: queryListFilesInput{path: "/folder1/file-inner.jpg", options: &ListOptions{Recursive: true, WithFolders: true, WithFiles: true}},
 						list:  checks(listSize(1), listHasMore(false), listLastPath("/folder1/file-inner.jpg")),
@@ -380,23 +393,23 @@ func TestFsStorage(t *testing.T) {
 						},
 					},
 					queryListFiles{
-						input: queryListFilesInput{path: "/folder1", options: &ListOptions{Recursive: true, PathFilters: &PathFilters{allowedPrefixes: []string{"/folder2"}}}},
+						input: queryListFilesInput{path: "/folder1", options: &ListOptions{Recursive: true, Filter: NewPathFilter([]string{"/folder2"}, nil, nil, nil)}},
 						list:  checks(listSize(0), listHasMore(false), listLastPath("")),
 					},
 					queryListFiles{
-						input: queryListFilesInput{path: "/folder1", options: &ListOptions{Recursive: true, WithFiles: true, WithFolders: true, PathFilters: &PathFilters{allowedPrefixes: []string{"/folder2"}}}},
+						input: queryListFilesInput{path: "/folder1", options: &ListOptions{Recursive: true, WithFiles: true, WithFolders: true, Filter: NewPathFilter([]string{"/folder2"}, nil, nil, nil)}},
 						list:  checks(listSize(0), listHasMore(false), listLastPath("")),
 						files: [][]interface{}{},
 					},
 					queryListFiles{
-						input: queryListFilesInput{path: "/folder1", options: &ListOptions{Recursive: true, PathFilters: &PathFilters{allowedPrefixes: []string{"/folder1/folder"}}}},
+						input: queryListFilesInput{path: "/folder1", options: &ListOptions{Recursive: true, Filter: NewPathFilter([]string{"/folder1/folder"}, nil, nil, nil)}},
 						list:  checks(listSize(1), listHasMore(false)),
 						files: [][]interface{}{
 							checks(fPath("/folder1/folder2/file.jpg")),
 						},
 					},
 					queryListFiles{
-						input: queryListFilesInput{path: "/folder1", options: &ListOptions{Recursive: true, WithFiles: true, WithFolders: true, PathFilters: &PathFilters{allowedPrefixes: []string{"/folder1/folder"}}}},
+						input: queryListFilesInput{path: "/folder1", options: &ListOptions{Recursive: true, WithFiles: true, WithFolders: true, Filter: NewPathFilter([]string{"/folder1/folder"}, nil, nil, nil)}},
 						list:  checks(listSize(2), listHasMore(false)),
 						files: [][]interface{}{
 							checks(fPath("/folder1/folder2"), fMimeType("directory")),
@@ -1151,7 +1164,7 @@ func TestFsStorage(t *testing.T) {
 	}
 
 	createPathFiltersCases := func() []fsTestCase {
-		pathFilters := NewPathFilters(
+		pathFilters := NewPathFilter(
 			[]string{"/gitB/", "/s3/folder/", "/gitC/"},
 			[]string{"/gitA/dashboard2.json"},
 			[]string{"/s3/folder/nested/"},
@@ -1205,22 +1218,22 @@ func TestFsStorage(t *testing.T) {
 					},
 					queryListFiles{
 						input: queryListFilesInput{path: "/", options: &ListOptions{
-							Recursive:   true,
-							PathFilters: allowAllPathFilters(),
+							Recursive: true,
+							Filter:    NewAllowAllPathFilter(),
 						}},
 						list: checks(listSize(7)),
 					},
 					queryListFiles{
 						input: queryListFilesInput{path: "/", options: &ListOptions{
-							Recursive:   true,
-							PathFilters: denyAllPathFilters(),
+							Recursive: true,
+							Filter:    NewDenyAllPathFilter(),
 						}},
 						list: checks(listSize(0)),
 					},
 					queryListFiles{
 						input: queryListFilesInput{path: "/", options: &ListOptions{
-							Recursive:   true,
-							PathFilters: pathFilters,
+							Recursive: true,
+							Filter:    pathFilters,
 						}},
 						list: checks(listSize(5), listHasMore(false), listLastPath("/s3/folder/dashboard.json")),
 						files: [][]interface{}{
@@ -1236,7 +1249,7 @@ func TestFsStorage(t *testing.T) {
 					queryListFiles{
 						input: queryListFilesInput{path: "/", options: &ListOptions{
 							Recursive:   true,
-							PathFilters: pathFilters,
+							Filter:      pathFilters,
 							WithFiles:   true,
 							WithFolders: true,
 						}},
@@ -1260,8 +1273,8 @@ func TestFsStorage(t *testing.T) {
 					},
 					queryListFolders{
 						input: queryListFoldersInput{path: "/", options: &ListOptions{
-							Recursive:   true,
-							PathFilters: pathFilters,
+							Recursive: true,
+							Filter:    pathFilters,
 						}},
 						checks: [][]interface{}{
 							// /gitA is missing due to the lack of explicit allow
@@ -1277,8 +1290,8 @@ func TestFsStorage(t *testing.T) {
 					},
 					queryListFiles{
 						input: queryListFilesInput{path: "/gitA", options: &ListOptions{
-							Recursive:   false,
-							PathFilters: pathFilters,
+							Recursive: false,
+							Filter:    pathFilters,
 						}},
 						list: checks(listSize(1), listHasMore(false), listLastPath("/gitA/dashboard2.json")),
 						files: [][]interface{}{
@@ -1287,23 +1300,23 @@ func TestFsStorage(t *testing.T) {
 					},
 					queryListFolders{
 						input: queryListFoldersInput{path: "/gitA", options: &ListOptions{
-							Recursive:   false,
-							PathFilters: pathFilters,
+							Recursive: false,
+							Filter:    pathFilters,
 						}},
 						checks: [][]interface{}{},
 					},
 					queryListFiles{
 						input: queryListFilesInput{path: "/gitC", options: &ListOptions{
-							Recursive:   false,
-							PathFilters: pathFilters,
+							Recursive: false,
+							Filter:    pathFilters,
 						}},
 						list:  checks(listSize(0), listHasMore(false), listLastPath("")),
 						files: [][]interface{}{},
 					},
 					queryListFiles{
 						input: queryListFilesInput{path: "/gitC/nestedC", options: &ListOptions{
-							Recursive:   false,
-							PathFilters: pathFilters,
+							Recursive: false,
+							Filter:    pathFilters,
 						}},
 						list: checks(listSize(1), listHasMore(false), listLastPath("/gitC/nestedC/dashboardC.json")),
 						files: [][]interface{}{
@@ -1312,8 +1325,8 @@ func TestFsStorage(t *testing.T) {
 					},
 					queryListFolders{
 						input: queryListFoldersInput{path: "/gitC", options: &ListOptions{
-							Recursive:   false,
-							PathFilters: pathFilters,
+							Recursive: false,
+							Filter:    pathFilters,
 						}},
 						checks: [][]interface{}{},
 					},

pkg/infra/filestorage/test_utils.go

@@ -313,7 +313,7 @@ func handleQuery(t *testing.T, ctx context.Context, query interface{}, queryName
 				WithFiles:    false,
 				WithFolders:  true,
 				WithContents: false,
-				PathFilters:  nil,
+				Filter:       nil,
 			}
 		} else {
 			opts.WithFolders = true

pkg/infra/filestorage/wrapper.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 
 	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/services/accesscontrol"
 )
 
 var (
@@ -17,102 +18,54 @@ var (
 )
 
 type wrapper struct {
-	log         log.Logger
-	wrapped     FileStorage
-	pathFilters *PathFilters
-	rootFolder  string
+	log        log.Logger
+	wrapped    FileStorage
+	filter     PathFilter
+	rootFolder string
 }
 
-func addRootFolderToFilters(pathFilters *PathFilters, rootFolder string) *PathFilters {
-	if pathFilters == nil {
-		return pathFilters
-	}
-
-	for i := range pathFilters.disallowedPaths {
-		pathFilters.disallowedPaths[i] = rootFolder + strings.TrimPrefix(pathFilters.disallowedPaths[i], Delimiter)
-	}
-	for i := range pathFilters.disallowedPrefixes {
-		pathFilters.disallowedPrefixes[i] = rootFolder + strings.TrimPrefix(pathFilters.disallowedPrefixes[i], Delimiter)
-	}
-	for i := range pathFilters.allowedPaths {
-		pathFilters.allowedPaths[i] = rootFolder + strings.TrimPrefix(pathFilters.allowedPaths[i], Delimiter)
-	}
-	for i := range pathFilters.allowedPrefixes {
-		pathFilters.allowedPrefixes[i] = rootFolder + strings.TrimPrefix(pathFilters.allowedPrefixes[i], Delimiter)
-	}
-
-	return pathFilters
+func wrapPathFilter(filter PathFilter, rootFolder string) PathFilter {
+	return &wrappedPathFilter{filter: filter, rootFolder: rootFolder}
 }
 
-func copyPathFilters(p *PathFilters) *PathFilters {
-	if p == nil {
-		return nil
-	}
-
-	return NewPathFilters(p.allowedPrefixes, p.allowedPaths, p.disallowedPrefixes, p.disallowedPaths)
+type wrappedPathFilter struct {
+	rootFolder string
+	filter     PathFilter
 }
 
-func addPathFilters(base *PathFilters, new *PathFilters) *PathFilters {
-	if new == nil {
-		return base
-	}
+func (w wrappedPathFilter) IsAllowed(path string) bool {
+	pathWithReplacedRoot := Delimiter + strings.TrimPrefix(path, w.rootFolder)
+	return w.filter.IsAllowed(pathWithReplacedRoot)
+}
 
-	if new.allowedPrefixes != nil {
-		if base.allowedPrefixes != nil {
-			base.allowedPrefixes = append(base.allowedPrefixes, new.allowedPrefixes...)
-		} else {
-			copiedPrefixes := make([]string, len(new.allowedPrefixes))
-			copy(copiedPrefixes, new.allowedPrefixes)
-			base.allowedPrefixes = copiedPrefixes
+func (w wrappedPathFilter) ToString() string {
+	return w.filter.ToString()
+}
+
+func (w wrappedPathFilter) asSQLFilter() accesscontrol.SQLFilter {
+	sqlFilter := w.filter.asSQLFilter()
+	for i := range sqlFilter.Args {
+		if path, ok := sqlFilter.Args[i].(string); ok {
+			sqlFilter.Args[i] = w.rootFolder + strings.TrimPrefix(path, Delimiter)
 		}
 	}
 
-	if new.allowedPaths != nil {
-		if base.allowedPaths != nil {
-			base.allowedPaths = append(base.allowedPaths, new.allowedPaths...)
-		} else {
-			copiedPaths := make([]string, len(new.allowedPaths))
-			copy(copiedPaths, new.allowedPaths)
-			base.allowedPaths = copiedPaths
-		}
-	}
-
-	if new.disallowedPrefixes != nil {
-		if base.disallowedPrefixes != nil {
-			base.disallowedPrefixes = append(base.disallowedPrefixes, new.disallowedPrefixes...)
-		} else {
-			copiedPrefixes := make([]string, len(new.disallowedPrefixes))
-			copy(copiedPrefixes, new.disallowedPrefixes)
-			base.disallowedPrefixes = copiedPrefixes
-		}
-	}
-
-	if new.disallowedPaths != nil {
-		if base.disallowedPaths != nil {
-			base.disallowedPaths = append(base.disallowedPaths, new.disallowedPaths...)
-		} else {
-			copiedPaths := make([]string, len(new.disallowedPaths))
-			copy(copiedPaths, new.disallowedPaths)
-			base.disallowedPaths = copiedPaths
-		}
-	}
-
-	return base
+	return sqlFilter
 }
 
-func newWrapper(log log.Logger, wrapped FileStorage, pathFilters *PathFilters, rootFolder string) FileStorage {
-	var rootedPathFilters *PathFilters
-	if pathFilters != nil {
-		rootedPathFilters = addRootFolderToFilters(copyPathFilters(pathFilters), rootFolder)
+func newWrapper(log log.Logger, wrapped FileStorage, pathFilter PathFilter, rootFolder string) FileStorage {
+	var wrappedPathFilter PathFilter
+	if pathFilter != nil {
+		wrappedPathFilter = wrapPathFilter(pathFilter, rootFolder)
 	} else {
-		rootedPathFilters = allowAllPathFilters()
+		wrappedPathFilter = wrapPathFilter(NewAllowAllPathFilter(), rootFolder)
 	}
 
 	return &wrapper{
-		log:         log,
-		wrapped:     wrapped,
-		pathFilters: rootedPathFilters,
-		rootFolder:  rootFolder,
+		log:        log,
+		wrapped:    wrapped,
+		filter:     wrappedPathFilter,
+		rootFolder: rootFolder,
 	}
 }
 
@@ -198,7 +151,7 @@ func (b wrapper) Get(ctx context.Context, path string) (*File, error) {
 	}
 
 	rootedPath := b.addRoot(path)
-	if !b.pathFilters.IsAllowed(rootedPath) {
+	if !b.filter.IsAllowed(rootedPath) {
 		return nil, nil
 	}
 
@@ -212,13 +165,14 @@ func (b wrapper) Get(ctx context.Context, path string) (*File, error) {
 	}
 	return file, err
 }
+
 func (b wrapper) Delete(ctx context.Context, path string) error {
 	if err := b.validatePath(path); err != nil {
 		return err
 	}
 
 	rootedPath := b.addRoot(path)
-	if !b.pathFilters.IsAllowed(rootedPath) {
+	if !b.filter.IsAllowed(rootedPath) {
 		return nil
 	}
 
@@ -242,7 +196,7 @@ func (b wrapper) Upsert(ctx context.Context, file *UpsertFileCommand) error {
 	}
 
 	rootedPath := b.addRoot(file.Path)
-	if !b.pathFilters.IsAllowed(rootedPath) {
+	if !b.filter.IsAllowed(rootedPath) {
 		return nil
 	}
 
@@ -284,7 +238,7 @@ func (b wrapper) listOptionsWithDefaults(options *ListOptions) *ListOptions {
 	if options == nil {
 		return &ListOptions{
 			Recursive:    false,
-			PathFilters:  b.pathFilters,
+			Filter:       b.filter,
 			WithFiles:    true,
 			WithFolders:  false,
 			WithContents: false,
@@ -295,20 +249,25 @@ func (b wrapper) listOptionsWithDefaults(options *ListOptions) *ListOptions {
 	if !options.WithFiles && !options.WithFolders {
 		withFiles = true
 	}
-	if options.PathFilters == nil {
+	if b.filter == nil {
 		return &ListOptions{
 			Recursive:    options.Recursive,
-			PathFilters:  b.pathFilters,
+			Filter:       b.filter,
 			WithFiles:    withFiles,
 			WithFolders:  options.WithFolders,
 			WithContents: options.WithContents,
 		}
 	}
 
-	rootedFilters := addRootFolderToFilters(copyPathFilters(options.PathFilters), b.rootFolder)
+	var filter PathFilter
+	if options.Filter != nil {
+		filter = newAndPathFilter(b.filter, wrapPathFilter(options.Filter, b.rootFolder))
+	} else {
+		filter = b.filter
+	}
 	return &ListOptions{
 		Recursive:    options.Recursive,
-		PathFilters:  addPathFilters(rootedFilters, b.pathFilters),
+		Filter:       filter,
 		WithFiles:    withFiles,
 		WithFolders:  options.WithFolders,
 		WithContents: options.WithContents,
@@ -321,7 +280,7 @@ func (b wrapper) CreateFolder(ctx context.Context, path string) error {
 	}
 
 	rootedPath := b.addRoot(path)
-	if !b.pathFilters.IsAllowed(rootedPath) {
+	if !b.filter.IsAllowed(rootedPath) {
 		return nil
 	}
 
@@ -334,7 +293,7 @@ func (b wrapper) DeleteFolder(ctx context.Context, path string) error {
 	}
 
 	rootedPath := b.addRoot(path)
-	if !b.pathFilters.IsAllowed(rootedPath) {
+	if !b.filter.IsAllowed(rootedPath) {
 		return nil
 	}
 
@@ -356,13 +315,6 @@ func (b wrapper) List(ctx context.Context, folderPath string, paging *Paging, op
 	}
 
 	options = b.listOptionsWithDefaults(options)
-	if (!options.WithFiles && !options.WithFolders) || options.isDenyAll() {
-		return &ListResponse{
-			Files:    []*File{},
-			HasMore:  false,
-			LastPath: "",
-		}, nil
-	}
 
 	var fileChan = make(chan *File)
 	fileRetrievalCtx, cancelFileGet := context.WithCancel(ctx)
@@ -397,7 +349,6 @@ func (b wrapper) List(ctx context.Context, folderPath string, paging *Paging, op
 
 	file := <-fileChan
 	if file != nil {
-		file.FileMetadata.FullPath = b.removeRoot(file.FileMetadata.FullPath)
 		var contents []byte
 		if options.WithContents {
 			contents = file.Contents

pkg/infra/filestorage/wrapper_test.go

@@ -7,157 +7,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestWrapper_addRootFolderToFilters(t *testing.T) {
-	t.Run("should return null if passed filters are null", func(t *testing.T) {
-		require.Nil(t, addRootFolderToFilters(nil, "root"))
-	})
-
-	t.Run("should not allocate empty arrays in place of nil arrays", func(t *testing.T) {
-		filters := NewPathFilters(nil, nil, nil, nil)
-		rootedFilters := addRootFolderToFilters(filters, "root")
-		require.NotNil(t, rootedFilters)
-		require.Nil(t, rootedFilters.disallowedPrefixes)
-		require.Nil(t, rootedFilters.disallowedPaths)
-		require.Nil(t, rootedFilters.allowedPrefixes)
-		require.Nil(t, rootedFilters.allowedPaths)
-	})
-
-	t.Run("should preserve empty arrays", func(t *testing.T) {
-		filters := NewPathFilters([]string{}, []string{}, nil, nil)
-		rootedFilters := addRootFolderToFilters(filters, "root")
-		require.NotNil(t, rootedFilters)
-		require.Nil(t, rootedFilters.disallowedPrefixes)
-		require.Nil(t, rootedFilters.disallowedPaths)
-		require.NotNil(t, rootedFilters.allowedPrefixes)
-		require.Equal(t, []string{}, rootedFilters.allowedPrefixes)
-
-		require.NotNil(t, rootedFilters.allowedPaths)
-		require.Equal(t, []string{}, rootedFilters.allowedPaths)
-	})
-
-	t.Run("should mutate arrays rather than reallocate", func(t *testing.T) {
-		filters := NewPathFilters([]string{"/abc", "/abc2"}, nil, nil, []string{"/abc/", "/abc2/"})
-		originalAllowedPrefixes := filters.allowedPrefixes
-		originalDisallowedPaths := filters.disallowedPaths
-
-		rootedFilters := addRootFolderToFilters(filters, "root/")
-		require.NotNil(t, rootedFilters)
-		require.Nil(t, rootedFilters.allowedPaths)
-		require.Nil(t, rootedFilters.disallowedPrefixes)
-
-		expectedAllowedPrefixes := []string{"root/abc", "root/abc2"}
-		expectedDisallowedPaths := []string{"root/abc/", "root/abc2/"}
-		require.Equal(t, expectedAllowedPrefixes, rootedFilters.allowedPrefixes)
-		require.Equal(t, expectedDisallowedPaths, rootedFilters.disallowedPaths)
-
-		require.Equal(t, expectedAllowedPrefixes, originalAllowedPrefixes)
-		require.Equal(t, expectedDisallowedPaths, originalDisallowedPaths)
-	})
-}
-
-func TestWrapper_copyPathFilters(t *testing.T) {
-	t.Run("should return null if passed pathFilters are null", func(t *testing.T) {
-		require.Nil(t, copyPathFilters(nil))
-	})
-
-	t.Run("should not allocate empty arrays in place of nil arrays", func(t *testing.T) {
-		copiedFilters := copyPathFilters(NewPathFilters(nil, nil, nil, nil))
-		require.NotNil(t, copiedFilters)
-		require.Nil(t, copiedFilters.disallowedPrefixes)
-		require.Nil(t, copiedFilters.disallowedPaths)
-		require.Nil(t, copiedFilters.allowedPrefixes)
-		require.Nil(t, copiedFilters.allowedPaths)
-	})
-
-	t.Run("should preserve empty arrays", func(t *testing.T) {
-		copiedFilters := copyPathFilters(NewPathFilters([]string{}, []string{}, nil, nil))
-		require.NotNil(t, copiedFilters)
-		require.Nil(t, copiedFilters.disallowedPrefixes)
-		require.Nil(t, copiedFilters.disallowedPaths)
-		require.NotNil(t, copiedFilters.allowedPrefixes)
-		require.Equal(t, []string{}, copiedFilters.allowedPrefixes)
-
-		require.NotNil(t, copiedFilters.allowedPaths)
-		require.Equal(t, []string{}, copiedFilters.allowedPaths)
-	})
-
-	t.Run("should new pointer with new slices", func(t *testing.T) {
-		filters := NewPathFilters([]string{"/abc", "/abc2"}, nil, nil, []string{"/abc/", "/abc2/"})
-
-		copiedFilters := copyPathFilters(filters)
-
-		require.NotSame(t, filters, copiedFilters)
-
-		require.Equal(t, filters.allowedPrefixes, copiedFilters.allowedPrefixes)
-		require.Equal(t, filters.allowedPaths, copiedFilters.allowedPaths)
-		require.Equal(t, filters.disallowedPrefixes, copiedFilters.disallowedPrefixes)
-		require.Equal(t, filters.disallowedPaths, copiedFilters.disallowedPaths)
-
-		copiedFilters.disallowedPaths[0] = "changed"
-		require.Equal(t, []string{"/abc/", "/abc2/"}, filters.disallowedPaths)
-		require.Equal(t, []string{"changed", "/abc2/"}, copiedFilters.disallowedPaths)
-		require.NotEqual(t, filters.disallowedPaths, copiedFilters.disallowedPaths)
-	})
-}
-
-func TestWrapper_addPathFilters(t *testing.T) {
-	t.Run("should return pointer to the first argument", func(t *testing.T) {
-		base := NewPathFilters(nil, nil, nil, nil)
-		toAdd := NewPathFilters([]string{"abc"}, []string{"abc2"}, []string{"abc3"}, []string{"abc4"})
-		require.Same(t, base, addPathFilters(base, toAdd))
-	})
-
-	testcases := []struct {
-		base     *PathFilters
-		toAdd    *PathFilters
-		expected *PathFilters
-	}{
-		{
-			base:     NewPathFilters(nil, nil, nil, nil),
-			toAdd:    NewPathFilters([]string{"abc"}, []string{"abc2"}, []string{"abc3"}, []string{"abc4"}),
-			expected: NewPathFilters([]string{"abc"}, []string{"abc2"}, []string{"abc3"}, []string{"abc4"}),
-		},
-		{
-			base:     NewPathFilters([]string{"abc"}, []string{"abc2"}, []string{"abc3"}, []string{"abc4"}),
-			toAdd:    NewPathFilters(nil, nil, nil, nil),
-			expected: NewPathFilters([]string{"abc"}, []string{"abc2"}, []string{"abc3"}, []string{"abc4"}),
-		},
-		{
-			base:     NewPathFilters([]string{"abc"}, []string{"abc2"}, []string{"abc3"}, []string{"abc4"}),
-			toAdd:    NewPathFilters([]string{"abc"}, []string{"abc2"}, []string{"abc3"}, []string{"abc4"}),
-			expected: NewPathFilters([]string{"abc", "abc"}, []string{"abc2", "abc2"}, []string{"abc3", "abc3"}, []string{"abc4", "abc4"}),
-		},
-		{
-			base:     NewPathFilters([]string{"abc"}, []string{}, nil, []string{"abc4"}),
-			toAdd:    NewPathFilters([]string{"abc"}, []string{"abc2", "abc22", "abc222"}, []string{"abc3"}, []string{"abc4"}),
-			expected: NewPathFilters([]string{"abc", "abc"}, []string{"abc2", "abc22", "abc222"}, []string{"abc3"}, []string{"abc4", "abc4"}),
-		},
-	}
-
-	for _, tt := range testcases {
-		require.Equal(t, tt.expected, addPathFilters(tt.base, tt.toAdd))
-	}
-
-	t.Run("should not reuse arrays allocations from the second arg", func(t *testing.T) {
-		base := NewPathFilters(nil, []string{}, nil, nil)
-		toAdd := NewPathFilters([]string{"abc"}, []string{"abc2"}, []string{"abc3"}, []string{"abc4"})
-
-		_ = addPathFilters(base, toAdd)
-
-		require.Equal(t, toAdd.allowedPaths, base.allowedPaths)
-		base.allowedPaths[0] = "mutated"
-		require.Equal(t, []string{"mutated"}, base.allowedPaths)
-		require.Equal(t, []string{"abc2"}, toAdd.allowedPaths)
-		require.NotEqual(t, toAdd.allowedPaths, base.allowedPaths)
-
-		require.Equal(t, toAdd.allowedPrefixes, base.allowedPrefixes)
-		base.allowedPrefixes[0] = "mutated2"
-		require.Equal(t, []string{"mutated2"}, base.allowedPrefixes)
-		require.Equal(t, []string{"abc"}, toAdd.allowedPrefixes)
-		require.NotEqual(t, toAdd.allowedPrefixes, base.allowedPrefixes)
-	})
-}
-
 func TestFilestorage_getParentFolderPath(t *testing.T) {
 	var tests = []struct {
 		name     string

pkg/services/store/storage_disk.go

@@ -57,7 +57,7 @@ func newDiskStorage(prefix string, name string, cfg *StorageLocalDiskConfig) *ro
 		} else {
 			s.store = filestorage.NewCdkBlobStorage(grafanaStorageLogger,
 				bucket, "",
-				filestorage.NewPathFilters(cfg.Roots, nil, nil, nil))
+				filestorage.NewPathFilter(cfg.Roots, nil, nil, nil))
 
 			meta.Ready = true // exists!
 		}