From 35d059736747ed86c02782338401085e31de313e Mon Sep 17 00:00:00 2001 From: Ieva Date: Mon, 10 Jun 2024 14:44:34 +0300 Subject: [PATCH] RBAC: Only check for the write action when listing editable dashboards/folders (#88518) * only check for the write action when listing editable resources * test fix --- .../sqlstore/permissions/dashboard.go | 116 ++++++------------ .../dashboard_filter_no_subquery.go | 43 ++----- .../sqlstore/searchstore/search_test.go | 68 +++++----- 3 files changed, 90 insertions(+), 137 deletions(-) diff --git a/pkg/services/sqlstore/permissions/dashboard.go b/pkg/services/sqlstore/permissions/dashboard.go index 7efaf7a17b8..0b05dfeadfd 100644 --- a/pkg/services/sqlstore/permissions/dashboard.go +++ b/pkg/services/sqlstore/permissions/dashboard.go @@ -25,10 +25,10 @@ type clause struct { } type accessControlDashboardPermissionFilter struct { - user identity.Requester - dashboardActions []string - folderActions []string - features featuremgmt.FeatureToggles + user identity.Requester + dashboardAction string + folderAction string + features featuremgmt.FeatureToggles where clause // any recursive CTE queries (if supported) @@ -51,38 +51,32 @@ type PermissionsFilter interface { func NewAccessControlDashboardPermissionFilter(user identity.Requester, permissionLevel dashboardaccess.PermissionType, queryType string, features featuremgmt.FeatureToggles, recursiveQueriesAreSupported bool) PermissionsFilter { needEdit := permissionLevel > dashboardaccess.PERMISSION_VIEW - var folderActions []string - var dashboardActions []string + var folderAction string + var dashboardAction string if queryType == searchstore.TypeFolder { - folderActions = append(folderActions, dashboards.ActionFoldersRead) + folderAction = dashboards.ActionFoldersRead + //folderAction = append(folderAction, dashboards.ActionFoldersRead) if needEdit { - folderActions = append(folderActions, dashboards.ActionDashboardsCreate) + folderAction = dashboards.ActionDashboardsCreate } } else if queryType == searchstore.TypeDashboard { - dashboardActions = append(dashboardActions, dashboards.ActionDashboardsRead) + dashboardAction = dashboards.ActionDashboardsRead if needEdit { - dashboardActions = append(dashboardActions, dashboards.ActionDashboardsWrite) + dashboardAction = dashboards.ActionDashboardsWrite } } else if queryType == searchstore.TypeAlertFolder { - folderActions = append( - folderActions, - dashboards.ActionFoldersRead, - accesscontrol.ActionAlertingRuleRead, - ) + folderAction = accesscontrol.ActionAlertingRuleRead if needEdit { - folderActions = append( - folderActions, - accesscontrol.ActionAlertingRuleCreate, - ) + folderAction = accesscontrol.ActionAlertingRuleCreate } } else if queryType == searchstore.TypeAnnotation { - dashboardActions = append(dashboardActions, accesscontrol.ActionAnnotationsRead) + dashboardAction = accesscontrol.ActionAnnotationsRead } else { - folderActions = append(folderActions, dashboards.ActionFoldersRead) - dashboardActions = append(dashboardActions, dashboards.ActionDashboardsRead) + folderAction = dashboards.ActionFoldersRead + dashboardAction = dashboards.ActionDashboardsRead if needEdit { - folderActions = append(folderActions, dashboards.ActionDashboardsCreate) - dashboardActions = append(dashboardActions, dashboards.ActionDashboardsWrite) + folderAction = dashboards.ActionDashboardsCreate + dashboardAction = dashboards.ActionDashboardsWrite } } @@ -90,12 +84,12 @@ func NewAccessControlDashboardPermissionFilter(user identity.Requester, permissi if features.IsEnabledGlobally(featuremgmt.FlagPermissionsFilterRemoveSubquery) { f = &accessControlDashboardPermissionFilterNoFolderSubquery{ accessControlDashboardPermissionFilter: accessControlDashboardPermissionFilter{ - user: user, folderActions: folderActions, dashboardActions: dashboardActions, features: features, + user: user, folderAction: folderAction, dashboardAction: dashboardAction, features: features, recursiveQueriesAreSupported: recursiveQueriesAreSupported, }, } } else { - f = &accessControlDashboardPermissionFilter{user: user, folderActions: folderActions, dashboardActions: dashboardActions, features: features, + f = &accessControlDashboardPermissionFilter{user: user, folderAction: folderAction, dashboardAction: dashboardAction, features: features, recursiveQueriesAreSupported: recursiveQueriesAreSupported, } } @@ -117,7 +111,7 @@ func (f *accessControlDashboardPermissionFilter) Where() (string, []any) { // Check if user has no permissions required for search to skip expensive query func (f *accessControlDashboardPermissionFilter) hasRequiredActions() bool { permissions := f.user.GetPermissions() - requiredActions := append(f.folderActions, f.dashboardActions...) + requiredActions := []string{f.folderAction, f.dashboardAction} for _, action := range requiredActions { if _, ok := permissions[action]; ok { return true @@ -155,24 +149,16 @@ func (f *accessControlDashboardPermissionFilter) buildClauses() { // currently it's used for the extended JWT module (when the user is authenticated via a JWT token generated by Grafana) useSelfContainedPermissions := f.user.IsAuthenticatedBy(login.ExtendedJWTModule) - if len(f.dashboardActions) > 0 { - toCheck := actionsToCheck(f.dashboardActions, f.user.GetPermissions(), dashWildcards, folderWildcards) + if len(f.dashboardAction) > 0 { + toCheck := actionsToCheck(f.dashboardAction, f.user.GetPermissions(), dashWildcards, folderWildcards) if len(toCheck) > 0 { if !useSelfContainedPermissions { builder.WriteString("(dashboard.uid IN (SELECT identifier FROM permission WHERE kind = 'dashboards' AND attribute = 'uid'") builder.WriteString(rolesFilter) args = append(args, params...) - - if len(toCheck) == 1 { - builder.WriteString(" AND action = ?") - args = append(args, toCheck[0]) - } else { - builder.WriteString(" AND action IN (?" + strings.Repeat(", ?", len(toCheck)-1) + ") GROUP BY role_id, scope, identifier HAVING COUNT(action) = ?") - args = append(args, toCheck...) - args = append(args, len(toCheck)) - } - builder.WriteString(") AND NOT dashboard.is_folder)") + builder.WriteString(" AND action = ?) AND NOT dashboard.is_folder)") + args = append(args, toCheck[0]) } else { actions := parseStringSliceFromInterfaceSlice(toCheck) @@ -193,15 +179,8 @@ func (f *accessControlDashboardPermissionFilter) buildClauses() { permSelector.WriteString("(SELECT identifier FROM permission WHERE kind = 'folders' AND attribute = 'uid'") permSelector.WriteString(rolesFilter) permSelectorArgs = append(permSelectorArgs, params...) - - if len(toCheck) == 1 { - permSelector.WriteString(" AND action = ?") - permSelectorArgs = append(permSelectorArgs, toCheck[0]) - } else { - permSelector.WriteString(" AND action IN (?" + strings.Repeat(", ?", len(toCheck)-1) + ") GROUP BY role_id, scope, identifier HAVING COUNT(action) = ?") - permSelectorArgs = append(permSelectorArgs, toCheck...) - permSelectorArgs = append(permSelectorArgs, len(toCheck)) - } + permSelector.WriteString(" AND action = ?") + permSelectorArgs = append(permSelectorArgs, toCheck[0]) } else { actions := parseStringSliceFromInterfaceSlice(toCheck) @@ -262,25 +241,19 @@ func (f *accessControlDashboardPermissionFilter) buildClauses() { permSelector.Reset() permSelectorArgs = permSelectorArgs[:0] - if len(f.folderActions) > 0 { - if len(f.dashboardActions) > 0 { + if len(f.folderAction) > 0 { + if len(f.dashboardAction) > 0 { builder.WriteString(" OR ") } - toCheck := actionsToCheck(f.folderActions, f.user.GetPermissions(), folderWildcards) + toCheck := actionsToCheck(f.folderAction, f.user.GetPermissions(), folderWildcards) if len(toCheck) > 0 { if !useSelfContainedPermissions { permSelector.WriteString("(SELECT identifier FROM permission WHERE kind = 'folders' AND attribute = 'uid'") permSelector.WriteString(rolesFilter) permSelectorArgs = append(permSelectorArgs, params...) - if len(toCheck) == 1 { - permSelector.WriteString(" AND action = ?") - permSelectorArgs = append(permSelectorArgs, toCheck[0]) - } else { - permSelector.WriteString(" AND action IN (?" + strings.Repeat(", ?", len(toCheck)-1) + ") GROUP BY role_id, scope, identifier HAVING COUNT(action) = ?") - permSelectorArgs = append(permSelectorArgs, toCheck...) - permSelectorArgs = append(permSelectorArgs, len(toCheck)) - } + permSelector.WriteString(" AND action = ?") + permSelectorArgs = append(permSelectorArgs, toCheck[0]) } else { actions := parseStringSliceFromInterfaceSlice(toCheck) @@ -369,27 +342,16 @@ func (f *accessControlDashboardPermissionFilter) addRecQry(queryName string, whe }) } -func actionsToCheck(actions []string, permissions map[string][]string, wildcards ...accesscontrol.Wildcards) []any { - toCheck := make([]any, 0, len(actions)) - - for _, a := range actions { - var hasWildcard bool - - outer: - for _, scope := range permissions[a] { - for _, w := range wildcards { - if w.Contains(scope) { - hasWildcard = true - break outer - } +func actionsToCheck(action string, permissions map[string][]string, wildcards ...accesscontrol.Wildcards) []any { + for _, scope := range permissions[action] { + for _, w := range wildcards { + if w.Contains(scope) { + return []any{} } } - - if !hasWildcard { - toCheck = append(toCheck, a) - } } - return toCheck + + return []any{action} } func (f *accessControlDashboardPermissionFilter) nestedFoldersSelectors(permSelector string, permSelectorArgs []any, leftTable string, leftCol string, rightTableCol string, orgID int64) (string, []any) { diff --git a/pkg/services/sqlstore/permissions/dashboard_filter_no_subquery.go b/pkg/services/sqlstore/permissions/dashboard_filter_no_subquery.go index ac993bd4c9d..911311a1f1d 100644 --- a/pkg/services/sqlstore/permissions/dashboard_filter_no_subquery.go +++ b/pkg/services/sqlstore/permissions/dashboard_filter_no_subquery.go @@ -53,24 +53,16 @@ func (f *accessControlDashboardPermissionFilterNoFolderSubquery) buildClauses() // currently it's used for the extended JWT module (when the user is authenticated via a JWT token generated by Grafana) useSelfContainedPermissions := f.user.GetAuthenticatedBy() == login.ExtendedJWTModule - if len(f.dashboardActions) > 0 { - toCheck := actionsToCheck(f.dashboardActions, f.user.GetPermissions(), dashWildcards, folderWildcards) + if len(f.dashboardAction) > 0 { + toCheck := actionsToCheck(f.dashboardAction, f.user.GetPermissions(), dashWildcards, folderWildcards) if len(toCheck) > 0 { if !useSelfContainedPermissions { builder.WriteString("(dashboard.uid IN (SELECT identifier FROM permission WHERE kind = 'dashboards' AND attribute = 'uid'") builder.WriteString(rolesFilter) args = append(args, params...) - - if len(toCheck) == 1 { - builder.WriteString(" AND action = ?") - args = append(args, toCheck[0]) - } else { - builder.WriteString(" AND action IN (?" + strings.Repeat(", ?", len(toCheck)-1) + ") GROUP BY role_id, scope, identifier HAVING COUNT(action) = ?") - args = append(args, toCheck...) - args = append(args, len(toCheck)) - } - builder.WriteString(") AND NOT dashboard.is_folder)") + builder.WriteString(" AND action = ?) AND NOT dashboard.is_folder)") + args = append(args, toCheck[0]) } else { actions := parseStringSliceFromInterfaceSlice(toCheck) @@ -91,15 +83,8 @@ func (f *accessControlDashboardPermissionFilterNoFolderSubquery) buildClauses() permSelector.WriteString("(SELECT identifier FROM permission WHERE kind = 'folders' AND attribute = 'uid'") permSelector.WriteString(rolesFilter) permSelectorArgs = append(permSelectorArgs, params...) - - if len(toCheck) == 1 { - permSelector.WriteString(" AND action = ?") - permSelectorArgs = append(permSelectorArgs, toCheck[0]) - } else { - permSelector.WriteString(" AND action IN (?" + strings.Repeat(", ?", len(toCheck)-1) + ") GROUP BY role_id, scope, identifier HAVING COUNT(action) = ?") - permSelectorArgs = append(permSelectorArgs, toCheck...) - permSelectorArgs = append(permSelectorArgs, len(toCheck)) - } + permSelector.WriteString(" AND action = ?") + permSelectorArgs = append(permSelectorArgs, toCheck[0]) } else { actions := parseStringSliceFromInterfaceSlice(toCheck) @@ -160,25 +145,19 @@ func (f *accessControlDashboardPermissionFilterNoFolderSubquery) buildClauses() permSelector.Reset() permSelectorArgs = permSelectorArgs[:0] - if len(f.folderActions) > 0 { - if len(f.dashboardActions) > 0 { + if len(f.folderAction) > 0 { + if len(f.dashboardAction) > 0 { builder.WriteString(" OR ") } - toCheck := actionsToCheck(f.folderActions, f.user.GetPermissions(), folderWildcards) + toCheck := actionsToCheck(f.folderAction, f.user.GetPermissions(), folderWildcards) if len(toCheck) > 0 { if !useSelfContainedPermissions { permSelector.WriteString("(SELECT identifier FROM permission WHERE kind = 'folders' AND attribute = 'uid'") permSelector.WriteString(rolesFilter) permSelectorArgs = append(permSelectorArgs, params...) - if len(toCheck) == 1 { - permSelector.WriteString(" AND action = ?") - permSelectorArgs = append(permSelectorArgs, toCheck[0]) - } else { - permSelector.WriteString(" AND action IN (?" + strings.Repeat(", ?", len(toCheck)-1) + ") GROUP BY role_id, scope, identifier HAVING COUNT(action) = ?") - permSelectorArgs = append(permSelectorArgs, toCheck...) - permSelectorArgs = append(permSelectorArgs, len(toCheck)) - } + permSelector.WriteString(" AND action = ?") + permSelectorArgs = append(permSelectorArgs, toCheck[0]) } else { actions := parseStringSliceFromInterfaceSlice(toCheck) diff --git a/pkg/services/sqlstore/searchstore/search_test.go b/pkg/services/sqlstore/searchstore/search_test.go index 7b60949d3a7..39732fdadbf 100644 --- a/pkg/services/sqlstore/searchstore/search_test.go +++ b/pkg/services/sqlstore/searchstore/search_test.go @@ -125,6 +125,7 @@ func TestBuilder_RBAC(t *testing.T) { testsCases := []struct { desc string userPermissions []accesscontrol.Permission + level dashboardaccess.PermissionType features featuremgmt.FeatureToggles expectedParams []any }{ @@ -140,6 +141,7 @@ func TestBuilder_RBAC(t *testing.T) { userPermissions: []accesscontrol.Permission{ {Action: dashboards.ActionDashboardsRead, Scope: "dashboards:uid:1"}, }, + level: dashboardaccess.PERMISSION_VIEW, features: featuremgmt.WithFeatures(), expectedParams: []any{ int64(1), @@ -150,8 +152,6 @@ func TestBuilder_RBAC(t *testing.T) { int64(1), 0, "dashboards:read", - "dashboards:write", - 2, int64(1), int64(1), int64(1), @@ -160,8 +160,6 @@ func TestBuilder_RBAC(t *testing.T) { int64(1), 0, "dashboards:read", - "dashboards:write", - 2, int64(1), int64(1), 0, @@ -169,8 +167,39 @@ func TestBuilder_RBAC(t *testing.T) { int64(1), 0, "folders:read", + }, + }, + { + desc: "user with write permission", + userPermissions: []accesscontrol.Permission{ + {Action: dashboards.ActionDashboardsWrite, Scope: "dashboards:uid:1"}, + }, + level: dashboardaccess.PERMISSION_EDIT, + features: featuremgmt.WithFeatures(), + expectedParams: []any{ + int64(1), + int64(1), + int64(1), + 0, + "Viewer", + int64(1), + 0, + "dashboards:write", + int64(1), + int64(1), + int64(1), + 0, + "Viewer", + int64(1), + 0, + "dashboards:write", + int64(1), + int64(1), + 0, + "Viewer", + int64(1), + 0, "dashboards:create", - 2, }, }, { @@ -178,6 +207,7 @@ func TestBuilder_RBAC(t *testing.T) { userPermissions: []accesscontrol.Permission{ {Action: dashboards.ActionDashboardsRead, Scope: "dashboards:uid:1"}, }, + level: dashboardaccess.PERMISSION_VIEW, features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), expectedParams: []any{ int64(1), @@ -188,8 +218,6 @@ func TestBuilder_RBAC(t *testing.T) { int64(1), 0, "dashboards:read", - "dashboards:write", - 2, int64(1), int64(1), int64(1), @@ -198,8 +226,6 @@ func TestBuilder_RBAC(t *testing.T) { int64(1), 0, "folders:read", - "dashboards:create", - 2, int64(1), int64(1), int64(1), @@ -208,8 +234,6 @@ func TestBuilder_RBAC(t *testing.T) { int64(1), 0, "dashboards:read", - "dashboards:write", - 2, int64(1), }, }, @@ -218,6 +242,7 @@ func TestBuilder_RBAC(t *testing.T) { userPermissions: []accesscontrol.Permission{ {Action: dashboards.ActionDashboardsRead, Scope: "dashboards:uid:1"}, }, + level: dashboardaccess.PERMISSION_VIEW, features: featuremgmt.WithFeatures(featuremgmt.FlagPermissionsFilterRemoveSubquery), expectedParams: []any{ int64(1), @@ -228,8 +253,6 @@ func TestBuilder_RBAC(t *testing.T) { int64(1), 0, "dashboards:read", - "dashboards:write", - 2, int64(1), int64(1), 0, @@ -237,8 +260,6 @@ func TestBuilder_RBAC(t *testing.T) { int64(1), 0, "dashboards:read", - "dashboards:write", - 2, int64(1), int64(1), 0, @@ -246,15 +267,14 @@ func TestBuilder_RBAC(t *testing.T) { int64(1), 0, "folders:read", - "dashboards:create", - 2, }, }, { - desc: "user with view permission with nesting and remove subquery", + desc: "user with edit permission with nesting and remove subquery", userPermissions: []accesscontrol.Permission{ - {Action: dashboards.ActionDashboardsRead, Scope: "dashboards:uid:1"}, + {Action: dashboards.ActionDashboardsWrite, Scope: "dashboards:uid:1"}, }, + level: dashboardaccess.PERMISSION_EDIT, features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders, featuremgmt.FlagPermissionsFilterRemoveSubquery), expectedParams: []any{ int64(1), @@ -264,9 +284,7 @@ func TestBuilder_RBAC(t *testing.T) { "Viewer", int64(1), 0, - "dashboards:read", "dashboards:write", - 2, int64(1), int64(1), int64(1), @@ -274,9 +292,7 @@ func TestBuilder_RBAC(t *testing.T) { "Viewer", int64(1), 0, - "folders:read", "dashboards:create", - 2, int64(1), int64(1), int64(1), @@ -284,9 +300,7 @@ func TestBuilder_RBAC(t *testing.T) { "Viewer", int64(1), 0, - "dashboards:read", "dashboards:write", - 2, }, }, } @@ -309,15 +323,13 @@ func TestBuilder_RBAC(t *testing.T) { user.Permissions = map[int64]map[string][]string{1: accesscontrol.GroupScopesByAction(tc.userPermissions)} } - level := dashboardaccess.PERMISSION_EDIT - builder := &searchstore.Builder{ Filters: []any{ searchstore.OrgFilter{OrgId: user.OrgID}, searchstore.TitleSorter{}, permissions.NewAccessControlDashboardPermissionFilter( user, - level, + tc.level, "", tc.features, recursiveQueriesAreSupported,