diff --git a/pkg/api/folder_bench_test.go b/pkg/api/folder_bench_test.go index 9a52eec53e4..407addf94b5 100644 --- a/pkg/api/folder_bench_test.go +++ b/pkg/api/folder_bench_test.go @@ -96,49 +96,97 @@ func BenchmarkFolderListAndSearch(b *testing.B) { features *featuremgmt.FeatureManager }{ { - desc: "get root folders with nested folders feature enabled", + desc: "impl=default nested_folders=on get root folders", + url: "/api/folders", + expectedLen: LEVEL0_FOLDER_NUM, + features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), + }, + { + desc: "impl=permissionsFilterRemoveSubquery nested_folders=on get root folders", url: "/api/folders", expectedLen: LEVEL0_FOLDER_NUM, features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders, featuremgmt.FlagPermissionsFilterRemoveSubquery), }, { - desc: "get subfolders with nested folders feature enabled", + desc: "impl=default nested_folders=on get subfolders", + url: "/api/folders?parentUid=folder0", + expectedLen: LEVEL1_FOLDER_NUM, + features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), + }, + { + desc: "impl=permissionsFilterRemoveSubquery nested_folders=on get subfolders", url: "/api/folders?parentUid=folder0", expectedLen: LEVEL1_FOLDER_NUM, features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders, featuremgmt.FlagPermissionsFilterRemoveSubquery), }, { - desc: "list all inherited dashboards with nested folders feature enabled", + desc: "impl=default nested_folders=on list all inherited dashboards", + url: "/api/search?type=dash-db&limit=5000", + expectedLen: withLimit(all), + features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), + }, + { + desc: "impl=permissionsFilterRemoveSubquery nested_folders=on list all inherited dashboards", url: "/api/search?type=dash-db&limit=5000", expectedLen: withLimit(all), features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders, featuremgmt.FlagPermissionsFilterRemoveSubquery), }, { - desc: "search for pattern with nested folders feature enabled", + desc: "impl=default nested_folders=on search for pattern", + url: "/api/search?type=dash-db&query=dashboard_0_0&limit=5000", + expectedLen: withLimit(1 + LEVEL1_DASHBOARD_NUM + LEVEL2_FOLDER_NUM*LEVEL2_DASHBOARD_NUM), + features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), + }, + { + desc: "impl=permissionsFilterRemoveSubquery nested_folders=on search for pattern", url: "/api/search?type=dash-db&query=dashboard_0_0&limit=5000", expectedLen: withLimit(1 + LEVEL1_DASHBOARD_NUM + LEVEL2_FOLDER_NUM*LEVEL2_DASHBOARD_NUM), features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders, featuremgmt.FlagPermissionsFilterRemoveSubquery), }, { - desc: "search for specific dashboard nested folders feature enabled", + desc: "impl=default nested_folders=on search for specific dashboard", + url: "/api/search?type=dash-db&query=dashboard_0_0_0_0", + expectedLen: 1, + features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), + }, + { + desc: "impl=permissionsFilterRemoveSubquery nested_folders=on search for specific dashboard", url: "/api/search?type=dash-db&query=dashboard_0_0_0_0", expectedLen: 1, features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders, featuremgmt.FlagPermissionsFilterRemoveSubquery), }, { - desc: "get root folders with nested folders feature disabled", + desc: "impl=default nested_folders=off get root folders", + url: "/api/folders?limit=5000", + expectedLen: withLimit(LEVEL0_FOLDER_NUM), + features: featuremgmt.WithFeatures(), + }, + { + desc: "impl=permissionsFilterRemoveSubquery nested_folders=off get root folders", url: "/api/folders?limit=5000", expectedLen: withLimit(LEVEL0_FOLDER_NUM), features: featuremgmt.WithFeatures(featuremgmt.FlagPermissionsFilterRemoveSubquery), }, { - desc: "list all dashboards with nested folders feature disabled", + desc: "impl=default nested_folders=off list all dashboards", + url: "/api/search?type=dash-db&limit=5000", + expectedLen: withLimit(LEVEL0_FOLDER_NUM * LEVEL0_DASHBOARD_NUM), + features: featuremgmt.WithFeatures(), + }, + { + desc: "impl=permissionsFilterRemoveSubquery nested_folders=off list all dashboards", url: "/api/search?type=dash-db&limit=5000", expectedLen: withLimit(LEVEL0_FOLDER_NUM * LEVEL0_DASHBOARD_NUM), features: featuremgmt.WithFeatures(featuremgmt.FlagPermissionsFilterRemoveSubquery), }, { - desc: "search specific dashboard with nested folders feature disabled", + desc: "impl=default nested_folders=off search specific dashboard", + url: "/api/search?type=dash-db&query=dashboard_0_0", + expectedLen: 1, + features: featuremgmt.WithFeatures(), + }, + { + desc: "impl=permissionsFilterRemoveSubquery nested_folders=off search specific dashboard", url: "/api/search?type=dash-db&query=dashboard_0_0", expectedLen: 1, features: featuremgmt.WithFeatures(featuremgmt.FlagPermissionsFilterRemoveSubquery), diff --git a/pkg/services/sqlstore/permissions/dashboard.go b/pkg/services/sqlstore/permissions/dashboard.go index 27cc1235e8b..af8852d930f 100644 --- a/pkg/services/sqlstore/permissions/dashboard.go +++ b/pkg/services/sqlstore/permissions/dashboard.go @@ -41,7 +41,7 @@ type PermissionsFilter interface { Where() (string, []any) buildClauses() - nestedFoldersSelectors(permSelector string, permSelectorArgs []any, leftTableCol string, rightTableCol string) (string, []any) + nestedFoldersSelectors(permSelector string, permSelectorArgs []any, leftTableCol string, rightTableCol string, orgID int64) (string, []any) } // NewAccessControlDashboardPermissionFilter creates a new AccessControlDashboardPermissionFilter that is configured with specific actions calculated based on the dashboards.PermissionType and query type @@ -126,7 +126,8 @@ func (f *accessControlDashboardPermissionFilter) buildClauses() { userID, _ = identity.IntIdentifier(namespaceID, identifier) } - filter, params := accesscontrol.UserRolesFilter(f.user.GetOrgID(), userID, f.user.GetTeams(), accesscontrol.GetOrgRoles(f.user)) + orgID := f.user.GetOrgID() + filter, params := accesscontrol.UserRolesFilter(orgID, userID, f.user.GetTeams(), accesscontrol.GetOrgRoles(f.user)) rolesFilter := " AND role_id IN(SELECT id FROM role " + filter + ") " var args []any builder := strings.Builder{} @@ -208,9 +209,10 @@ func (f *accessControlDashboardPermissionFilter) buildClauses() { builder.WriteString("(dashboard.folder_id IN (SELECT d.id FROM dashboard as d ") recQueryName := fmt.Sprintf("RecQry%d", len(f.recQueries)) f.addRecQry(recQueryName, permSelector.String(), permSelectorArgs) - builder.WriteString(fmt.Sprintf("WHERE d.uid IN (SELECT uid FROM %s)", recQueryName)) + builder.WriteString(fmt.Sprintf("WHERE d.org_id = ? AND d.uid IN (SELECT uid FROM %s)", recQueryName)) + args = append(args, orgID) default: - nestedFoldersSelectors, nestedFoldersArgs := f.nestedFoldersSelectors(permSelector.String(), permSelectorArgs, "dashboard.folder_id", "d.id") + nestedFoldersSelectors, nestedFoldersArgs := f.nestedFoldersSelectors(permSelector.String(), permSelectorArgs, "dashboard.folder_id", "d.id", orgID) builder.WriteRune('(') builder.WriteString(nestedFoldersSelectors) args = append(args, nestedFoldersArgs...) @@ -222,7 +224,8 @@ func (f *accessControlDashboardPermissionFilter) buildClauses() { default: builder.WriteString("(dashboard.folder_id IN (SELECT d.id FROM dashboard as d ") if len(permSelectorArgs) > 0 { - builder.WriteString("WHERE d.uid IN ") + builder.WriteString("WHERE d.org_id = ? AND d.uid IN ") + args = append(args, orgID) builder.WriteString(permSelector.String()) args = append(args, permSelectorArgs...) } else { @@ -287,7 +290,7 @@ func (f *accessControlDashboardPermissionFilter) buildClauses() { builder.WriteString("(dashboard.uid IN ") builder.WriteString(fmt.Sprintf("(SELECT uid FROM %s)", recQueryName)) default: - nestedFoldersSelectors, nestedFoldersArgs := f.nestedFoldersSelectors(permSelector.String(), permSelectorArgs, "dashboard.uid", "d.uid") + nestedFoldersSelectors, nestedFoldersArgs := f.nestedFoldersSelectors(permSelector.String(), permSelectorArgs, "dashboard.uid", "d.uid", orgID) builder.WriteRune('(') builder.WriteString(nestedFoldersSelectors) builder.WriteRune(')') @@ -372,7 +375,7 @@ func actionsToCheck(actions []string, permissions map[string][]string, wildcards return toCheck } -func (f *accessControlDashboardPermissionFilter) nestedFoldersSelectors(permSelector string, permSelectorArgs []any, leftTableCol string, rightTableCol string) (string, []any) { +func (f *accessControlDashboardPermissionFilter) nestedFoldersSelectors(permSelector string, permSelectorArgs []any, leftTableCol string, rightTableCol string, orgID int64) (string, []any) { wheres := make([]string, 0, folder.MaxNestedFolderDepth+1) args := make([]any, 0, len(permSelectorArgs)*(folder.MaxNestedFolderDepth+1)) @@ -387,7 +390,8 @@ func (f *accessControlDashboardPermissionFilter) nestedFoldersSelectors(permSele s := fmt.Sprintf(tmpl, t, prev, onCol, t, prev, t) joins = append(joins, s) - wheres = append(wheres, fmt.Sprintf("(%s IN (SELECT %s FROM dashboard d %s WHERE %s.uid IN %s)", leftTableCol, rightTableCol, strings.Join(joins, " "), t, permSelector)) + wheres = append(wheres, fmt.Sprintf("(%s IN (SELECT %s FROM dashboard d %s WHERE %s.org_id = ? AND %s.uid IN %s)", leftTableCol, rightTableCol, strings.Join(joins, " "), t, t, permSelector)) + args = append(args, orgID) args = append(args, permSelectorArgs...) prev = t diff --git a/pkg/services/sqlstore/permissions/dashboard_filter_no_subquery.go b/pkg/services/sqlstore/permissions/dashboard_filter_no_subquery.go index 2507c21fba9..e4b13db9c44 100644 --- a/pkg/services/sqlstore/permissions/dashboard_filter_no_subquery.go +++ b/pkg/services/sqlstore/permissions/dashboard_filter_no_subquery.go @@ -40,7 +40,8 @@ func (f *accessControlDashboardPermissionFilterNoFolderSubquery) buildClauses() userID, _ = identity.IntIdentifier(namespaceID, identifier) } - filter, params := accesscontrol.UserRolesFilter(f.user.GetOrgID(), userID, f.user.GetTeams(), accesscontrol.GetOrgRoles(f.user)) + orgID := f.user.GetOrgID() + filter, params := accesscontrol.UserRolesFilter(orgID, userID, f.user.GetTeams(), accesscontrol.GetOrgRoles(f.user)) rolesFilter := " AND role_id IN(SELECT id FROM role " + filter + ") " var args []any builder := strings.Builder{} @@ -124,7 +125,7 @@ func (f *accessControlDashboardPermissionFilterNoFolderSubquery) buildClauses() f.addRecQry(recQueryName, permSelector.String(), permSelectorArgs) builder.WriteString("(folder.uid IN (SELECT uid FROM " + recQueryName) default: - nestedFoldersSelectors, nestedFoldersArgs := f.nestedFoldersSelectors(permSelector.String(), permSelectorArgs, "folder.uid", "") + nestedFoldersSelectors, nestedFoldersArgs := f.nestedFoldersSelectors(permSelector.String(), permSelectorArgs, "folder.uid", "", orgID) builder.WriteRune('(') builder.WriteString(nestedFoldersSelectors) args = append(args, nestedFoldersArgs...) @@ -202,7 +203,7 @@ func (f *accessControlDashboardPermissionFilterNoFolderSubquery) buildClauses() builder.WriteString("(dashboard.uid IN ") builder.WriteString(fmt.Sprintf("(SELECT uid FROM %s)", recQueryName)) default: - nestedFoldersSelectors, nestedFoldersArgs := f.nestedFoldersSelectors(permSelector.String(), permSelectorArgs, "dashboard.uid", "") + nestedFoldersSelectors, nestedFoldersArgs := f.nestedFoldersSelectors(permSelector.String(), permSelectorArgs, "dashboard.uid", "", orgID) builder.WriteRune('(') builder.WriteString(nestedFoldersSelectors) builder.WriteRune(')') @@ -230,7 +231,7 @@ func (f *accessControlDashboardPermissionFilterNoFolderSubquery) buildClauses() f.where = clause{string: builder.String(), params: args} } -func (f *accessControlDashboardPermissionFilterNoFolderSubquery) nestedFoldersSelectors(permSelector string, permSelectorArgs []any, leftTableCol string, _ string) (string, []any) { +func (f *accessControlDashboardPermissionFilterNoFolderSubquery) nestedFoldersSelectors(permSelector string, permSelectorArgs []any, leftTableCol string, _ string, orgID int64) (string, []any) { wheres := make([]string, 0, folder.MaxNestedFolderDepth+1) args := make([]any, 0, len(permSelectorArgs)*(folder.MaxNestedFolderDepth+1)) @@ -247,7 +248,8 @@ func (f *accessControlDashboardPermissionFilterNoFolderSubquery) nestedFoldersSe s := fmt.Sprintf(tmpl, t, prev, t, prev, t) joins = append(joins, s) - wheres = append(wheres, fmt.Sprintf("(%s IN (SELECT f1.uid FROM folder f1 %s WHERE %s.uid IN %s)", leftTableCol, strings.Join(joins, " "), t, permSelector)) + wheres = append(wheres, fmt.Sprintf("(%s IN (SELECT f1.uid FROM folder f1 %s WHERE %s.org_id = ? AND %s.uid IN %s)", leftTableCol, strings.Join(joins, " "), t, t, permSelector)) + args = append(args, orgID) args = append(args, permSelectorArgs...) prev = t diff --git a/pkg/services/sqlstore/searchstore/search_test.go b/pkg/services/sqlstore/searchstore/search_test.go index b36b0323b0f..1dfa9c328e3 100644 --- a/pkg/services/sqlstore/searchstore/search_test.go +++ b/pkg/services/sqlstore/searchstore/search_test.go @@ -3,7 +3,6 @@ package searchstore_test import ( "context" - "strings" "testing" "github.com/stretchr/testify/assert" @@ -120,12 +119,12 @@ func TestBuilder_RBAC(t *testing.T) { testsCases := []struct { desc string userPermissions []accesscontrol.Permission - features []any + features featuremgmt.FeatureToggles expectedParams []any }{ { desc: "no user permissions", - features: []any{}, + features: featuremgmt.WithFeatures(), expectedParams: []any{ int64(1), }, @@ -135,7 +134,83 @@ func TestBuilder_RBAC(t *testing.T) { userPermissions: []accesscontrol.Permission{ {Action: dashboards.ActionDashboardsRead, Scope: "dashboards:uid:1"}, }, - features: []any{}, + features: featuremgmt.WithFeatures(), + expectedParams: []any{ + int64(1), + int64(1), + int64(1), + 0, + "Viewer", + int64(1), + 0, + "dashboards:read", + "dashboards:write", + 2, + int64(1), + int64(1), + int64(1), + 0, + "Viewer", + int64(1), + 0, + "dashboards:read", + "dashboards:write", + 2, + int64(1), + int64(1), + 0, + "Viewer", + int64(1), + 0, + "folders:read", + "dashboards:create", + 2, + }, + }, + { + desc: "user with view permission with nesting", + userPermissions: []accesscontrol.Permission{ + {Action: dashboards.ActionDashboardsRead, Scope: "dashboards:uid:1"}, + }, + features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), + expectedParams: []any{ + int64(1), + int64(1), + 0, + "Viewer", + int64(1), + 0, + "dashboards:read", + "dashboards:write", + 2, + int64(1), + int64(1), + 0, + "Viewer", + int64(1), + 0, + "folders:read", + "dashboards:create", + 2, + int64(1), + int64(1), + int64(1), + 0, + "Viewer", + int64(1), + 0, + "dashboards:read", + "dashboards:write", + 2, + int64(1), + }, + }, + { + desc: "user with view permission with remove subquery", + userPermissions: []accesscontrol.Permission{ + {Action: dashboards.ActionDashboardsRead, Scope: "dashboards:uid:1"}, + }, + features: featuremgmt.WithFeatures(featuremgmt.FlagPermissionsFilterRemoveSubquery), expectedParams: []any{ int64(1), int64(1), @@ -168,11 +243,11 @@ func TestBuilder_RBAC(t *testing.T) { }, }, { - desc: "user with view permission with nesting", + desc: "user with view permission with nesting and remove subquery", userPermissions: []accesscontrol.Permission{ {Action: dashboards.ActionDashboardsRead, Scope: "dashboards:uid:1"}, }, - features: []any{featuremgmt.FlagNestedFolders}, + features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders, featuremgmt.FlagPermissionsFilterRemoveSubquery), expectedParams: []any{ int64(1), int64(1), @@ -219,48 +294,39 @@ func TestBuilder_RBAC(t *testing.T) { require.NoError(t, err) for _, tc := range testsCases { - for _, features := range []*featuremgmt.FeatureManager{featuremgmt.WithFeatures(tc.features...), featuremgmt.WithFeatures(append(tc.features, featuremgmt.FlagPermissionsFilterRemoveSubquery)...)} { - m := features.GetEnabled(context.Background()) - keys := make([]string, 0, len(m)) - for k := range m { - keys = append(keys, k) + t.Run(tc.desc, func(t *testing.T) { + if len(tc.userPermissions) > 0 { + user.Permissions = map[int64]map[string][]string{1: accesscontrol.GroupScopesByAction(tc.userPermissions)} } - t.Run(tc.desc+" with features "+strings.Join(keys, ","), func(t *testing.T) { - if len(tc.userPermissions) > 0 { - user.Permissions = map[int64]map[string][]string{1: accesscontrol.GroupScopesByAction(tc.userPermissions)} - } + level := dashboards.PERMISSION_EDIT - level := dashboards.PERMISSION_EDIT + builder := &searchstore.Builder{ + Filters: []any{ + searchstore.OrgFilter{OrgId: user.OrgID}, + searchstore.TitleSorter{}, + permissions.NewAccessControlDashboardPermissionFilter( + user, + level, + "", + tc.features, + recursiveQueriesAreSupported, + ), + }, + Dialect: store.GetDialect(), + Features: tc.features, + } - builder := &searchstore.Builder{ - Filters: []any{ - searchstore.OrgFilter{OrgId: user.OrgID}, - searchstore.TitleSorter{}, - permissions.NewAccessControlDashboardPermissionFilter( - user, - level, - "", - features, - recursiveQueriesAreSupported, - ), - }, - Dialect: store.GetDialect(), - Features: features, - } - - res := []dashboards.DashboardSearchProjection{} - err := store.WithDbSession(context.Background(), func(sess *db.Session) error { - sql, params := builder.ToSQL(limit, page) - // TODO: replace with a proper test - assert.Equal(t, tc.expectedParams, params) - return sess.SQL(sql, params...).Find(&res) - }) - require.NoError(t, err) - - assert.Len(t, res, 0) + res := []dashboards.DashboardSearchProjection{} + err := store.WithDbSession(context.Background(), func(sess *db.Session) error { + sql, params := builder.ToSQL(limit, page) + assert.Equal(t, tc.expectedParams, params) + return sess.SQL(sql, params...).Find(&res) }) - } + require.NoError(t, err) + + assert.Len(t, res, 0) + }) } }