RBAC: Improve performance of dashboard filter query

pkg/services/accesscontrol/database/database.go

@@ -9,10 +9,6 @@ import (
 	"github.com/grafana/grafana/pkg/services/accesscontrol"
 )
 
-const (
-	globalOrgID = 0
-)
-
 func ProvideService(sql db.DB) *AccessControlStore {
 	return &AccessControlStore{sql}
 }
@@ -29,7 +25,7 @@ func (s *AccessControlStore) GetUserPermissions(ctx context.Context, query acces
 			return nil
 		}
 
-		filter, params := userRolesFilter(query.OrgID, query.UserID, query.TeamIDs, query.Roles)
+		filter, params := accesscontrol.UserRolesFilter(query.OrgID, query.UserID, query.TeamIDs, query.Roles)
 
 		q := `
 		SELECT
@@ -59,57 +55,6 @@ func (s *AccessControlStore) GetUserPermissions(ctx context.Context, query acces
 	return result, err
 }
 
-func userRolesFilter(orgID, userID int64, teamIDs []int64, roles []string) (string, []interface{}) {
-	var params []interface{}
-	builder := strings.Builder{}
-
-	// This is an additional security. We should never have permissions granted to userID 0.
-	// Only allow real users to get user/team permissions (anonymous/apikeys)
-	if userID > 0 {
-		builder.WriteString(`
-			SELECT ur.role_id
-			FROM user_role AS ur
-			WHERE ur.user_id = ?
-			AND (ur.org_id = ? OR ur.org_id = ?)
-		`)
-		params = []interface{}{userID, orgID, globalOrgID}
-	}
-
-	if len(teamIDs) > 0 {
-		if builder.Len() > 0 {
-			builder.WriteString("UNION")
-		}
-		builder.WriteString(`
-			SELECT tr.role_id FROM team_role as tr
-			WHERE tr.team_id IN(?` + strings.Repeat(", ?", len(teamIDs)-1) + `)
-			AND tr.org_id = ?
-		`)
-		for _, id := range teamIDs {
-			params = append(params, id)
-		}
-		params = append(params, orgID)
-	}
-
-	if len(roles) != 0 {
-		if builder.Len() > 0 {
-			builder.WriteString("UNION")
-		}
-
-		builder.WriteString(`
-			SELECT br.role_id FROM builtin_role AS br
-			WHERE br.role IN (?` + strings.Repeat(", ?", len(roles)-1) + `)
-			AND (br.org_id = ? OR br.org_id = ?)
-		`)
-		for _, role := range roles {
-			params = append(params, role)
-		}
-
-		params = append(params, orgID, globalOrgID)
-	}
-
-	return "INNER JOIN (" + builder.String() + ") as all_role ON role.id = all_role.role_id", params
-}
-
 func (s *AccessControlStore) DeleteUserPermissions(ctx context.Context, orgID, userID int64) error {
 	err := s.sql.WithDbSession(ctx, func(sess *db.Session) error {
 		roleDeleteQuery := "DELETE FROM user_role WHERE user_id = ?"

pkg/services/accesscontrol/filter.go

@@ -125,3 +125,54 @@ func SetAcceptListForTest(list map[string]struct{}) func() {
 		sqlIDAcceptList = original
 	}
 }
+
+func UserRolesFilter(orgID, userID int64, teamIDs []int64, roles []string) (string, []interface{}) {
+	var params []interface{}
+	builder := strings.Builder{}
+
+	// This is an additional security. We should never have permissions granted to userID 0.
+	// Only allow real users to get user/team permissions (anonymous/apikeys)
+	if userID > 0 {
+		builder.WriteString(`
+			SELECT ur.role_id
+			FROM user_role AS ur
+			WHERE ur.user_id = ?
+			AND (ur.org_id = ? OR ur.org_id = ?)
+		`)
+		params = []interface{}{userID, orgID, GlobalOrgID}
+	}
+
+	if len(teamIDs) > 0 {
+		if builder.Len() > 0 {
+			builder.WriteString("UNION")
+		}
+		builder.WriteString(`
+			SELECT tr.role_id FROM team_role as tr
+			WHERE tr.team_id IN(?` + strings.Repeat(", ?", len(teamIDs)-1) + `)
+			AND tr.org_id = ?
+		`)
+		for _, id := range teamIDs {
+			params = append(params, id)
+		}
+		params = append(params, orgID)
+	}
+
+	if len(roles) != 0 {
+		if builder.Len() > 0 {
+			builder.WriteString("UNION")
+		}
+
+		builder.WriteString(`
+			SELECT br.role_id FROM builtin_role AS br
+			WHERE br.role IN (?` + strings.Repeat(", ?", len(roles)-1) + `)
+			AND (br.org_id = ? OR br.org_id = ?)
+		`)
+		for _, role := range roles {
+			params = append(params, role)
+		}
+
+		params = append(params, orgID, GlobalOrgID)
+	}
+
+	return "INNER JOIN (" + builder.String() + ") as all_role ON role.id = all_role.role_id", params
+}

pkg/services/annotations/annotationsimpl/xorm_store_test.go

@@ -5,7 +5,9 @@ import (
 	"fmt"
 	"strings"
 	"testing"
+	"time"
 
+	"github.com/grafana/grafana/pkg/services/sqlstore"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
@@ -407,13 +409,15 @@ func TestIntegrationAnnotationListingWithRBAC(t *testing.T) {
 		t.Skip("skipping integration test")
 	}
 	sql := db.InitTestDB(t)
+
 	var maximumTagsLength int64 = 60
 	repo := xormRepositoryImpl{db: sql, cfg: setting.NewCfg(), log: log.New("annotation.test"), tagService: tagimpl.ProvideService(sql, sql.Cfg), maximumTagsLength: maximumTagsLength}
 	dashboardStore := dashboardstore.ProvideDashboardStore(sql, sql.Cfg, featuremgmt.WithFeatures(), tagimpl.ProvideService(sql, sql.Cfg))
 
 	testDashboard1 := models.SaveDashboardCommand{
-		UserId: 1,
-		OrgId:  1,
+		UserId:   1,
+		OrgId:    1,
+		IsFolder: false,
 		Dashboard: simplejson.NewFromAny(map[string]interface{}{
 			"title": "Dashboard 1",
 		}),
@@ -459,6 +463,7 @@ func TestIntegrationAnnotationListingWithRBAC(t *testing.T) {
 		UserID: 1,
 		OrgID:  1,
 	}
+	role := setupRBACRole(t, repo, user)
 
 	type testStruct struct {
 		description           string
@@ -519,6 +524,8 @@ func TestIntegrationAnnotationListingWithRBAC(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
 			user.Permissions = map[int64]map[string][]string{1: tc.permissions}
+			setupRBACPermission(t, repo, role, user)
+
 			results, err := repo.Get(context.Background(), &annotations.ItemQuery{
 				OrgId:        1,
 				SignedInUser: user,
@@ -535,3 +542,61 @@ func TestIntegrationAnnotationListingWithRBAC(t *testing.T) {
 		})
 	}
 }
+
+func setupRBACRole(t *testing.T, repo xormRepositoryImpl, user *user.SignedInUser) *accesscontrol.Role {
+	t.Helper()
+	var role *accesscontrol.Role
+	err := repo.db.WithDbSession(context.Background(), func(sess *sqlstore.DBSession) error {
+		role = &accesscontrol.Role{
+			OrgID:   user.OrgID,
+			UID:     "test_role",
+			Name:    "test:role",
+			Updated: time.Now(),
+			Created: time.Now(),
+		}
+		_, err := sess.Insert(role)
+		if err != nil {
+			return err
+		}
+
+		_, err = sess.Insert(accesscontrol.UserRole{
+			OrgID:   role.OrgID,
+			RoleID:  role.ID,
+			UserID:  user.UserID,
+			Created: time.Now(),
+		})
+		if err != nil {
+			return err
+		}
+		return nil
+	})
+
+	require.NoError(t, err)
+	return role
+}
+
+func setupRBACPermission(t *testing.T, repo xormRepositoryImpl, role *accesscontrol.Role, user *user.SignedInUser) {
+	t.Helper()
+	err := repo.db.WithDbSession(context.Background(), func(sess *sqlstore.DBSession) error {
+		if _, err := sess.Exec("DELETE FROM permission WHERE role_id = ?", role.ID); err != nil {
+			return err
+		}
+
+		var acPermission []accesscontrol.Permission
+		for action, scopes := range user.Permissions[user.OrgID] {
+			for _, scope := range scopes {
+				acPermission = append(acPermission, accesscontrol.Permission{
+					RoleID: role.ID, Action: action, Scope: scope, Created: time.Now(), Updated: time.Now(),
+				})
+			}
+		}
+
+		if _, err := sess.InsertMulti(&acPermission); err != nil {
+			return err
+		}
+
+		return nil
+	})
+
+	require.NoError(t, err)
+}

pkg/services/sqlstore/permissions/dashboard.go

@@ -80,9 +80,9 @@ func (d DashboardPermissionFilter) Where() (string, []interface{}) {
 }
 
 type AccessControlDashboardPermissionFilter struct {
-	User             *user.SignedInUser
-	dashboardActions []string
+	user             *user.SignedInUser
 	folderActions    []string
+	dashboardActions []string
 }
 
 // NewAccessControlDashboardPermissionFilter creates a new AccessControlDashboardPermissionFilter that is configured with specific actions calculated based on the models.PermissionType and query type
@@ -102,39 +102,80 @@ func NewAccessControlDashboardPermissionFilter(user *user.SignedInUser, permissi
 			dashboardActions = append(dashboardActions, dashboards.ActionDashboardsWrite)
 		}
 	}
-	return AccessControlDashboardPermissionFilter{User: user, folderActions: folderActions, dashboardActions: dashboardActions}
+	return AccessControlDashboardPermissionFilter{user: user, folderActions: folderActions, dashboardActions: dashboardActions}
 }
 
 func (f AccessControlDashboardPermissionFilter) Where() (string, []interface{}) {
+	if f.user == nil || f.user.Permissions == nil || f.user.Permissions[f.user.OrgID] == nil {
+		return "(1 = 0)", nil
+	}
+	dashWildcards := accesscontrol.WildcardsFromPrefix(dashboards.ScopeDashboardsPrefix)
+	folderWildcards := accesscontrol.WildcardsFromPrefix(dashboards.ScopeFoldersPrefix)
+
+	filter, params := accesscontrol.UserRolesFilter(f.user.OrgID, f.user.UserID, f.user.Teams, accesscontrol.GetOrgRoles(f.user))
+	rolesFilter := "AND role_id IN(SELECT distinct id FROM role " + filter + ")"
 	var args []interface{}
 	builder := strings.Builder{}
-	builder.WriteString("(")
-
+	builder.WriteRune('(')
 	if len(f.dashboardActions) > 0 {
-		builder.WriteString("((")
-
-		dashFilter, _ := accesscontrol.Filter(f.User, "dashboard.uid", dashboards.ScopeDashboardsPrefix, f.dashboardActions...)
-		builder.WriteString(dashFilter.Where)
-		args = append(args, dashFilter.Args...)
+		actionsToCheck := make([]interface{}, 0, len(f.dashboardActions))
+		for _, action := range f.dashboardActions {
+			var hasWildcard bool
+			for _, scope := range f.user.Permissions[f.user.OrgID][action] {
+				if dashWildcards.Contains(scope) || folderWildcards.Contains(scope) {
+					hasWildcard = true
+					break
+				}
+			}
+			if !hasWildcard {
+				actionsToCheck = append(actionsToCheck, action)
+			}
+		}
 
-		builder.WriteString(" OR dashboard.folder_id IN(SELECT id FROM dashboard WHERE ")
-		dashFolderFilter, _ := accesscontrol.Filter(f.User, "dashboard.uid", dashboards.ScopeFoldersPrefix, f.dashboardActions...)
+		if len(actionsToCheck) > 0 {
+			builder.WriteString("(dashboard.uid IN (SELECT substr(scope, 16) FROM permission WHERE action IN (?" + strings.Repeat(", ?", len(actionsToCheck)-1) + ") AND scope LIKE 'dashboards:uid:%' " + rolesFilter + " GROUP BY role_id, scope HAVING COUNT(action) = ?) AND NOT dashboard.is_folder)")
+			args = append(args, actionsToCheck...)
+			args = append(args, params...)
+			args = append(args, len(actionsToCheck))
 
-		builder.WriteString(dashFolderFilter.Where)
-		builder.WriteString(")) AND NOT dashboard.is_folder)")
-		args = append(args, dashFolderFilter.Args...)
+			builder.WriteString(" OR ")
+			builder.WriteString("(dashboard.folder_id IN (SELECT id FROM dashboard as d WHERE d.uid IN (SELECT substr(scope, 13) FROM permission WHERE action IN (?" + strings.Repeat(", ?", len(actionsToCheck)-1) + ") AND scope LIKE 'folders:uid:%' " + rolesFilter + " GROUP BY role_id, scope HAVING COUNT(action) = ?)) AND NOT dashboard.is_folder)")
+			args = append(args, actionsToCheck...)
+			args = append(args, params...)
+			args = append(args, len(actionsToCheck))
+		} else {
+			builder.WriteString("NOT dashboard.is_folder")
+		}
 	}
 
 	if len(f.folderActions) > 0 {
 		if len(f.dashboardActions) > 0 {
 			builder.WriteString(" OR ")
 		}
-		builder.WriteString("(")
-		folderFilter, _ := accesscontrol.Filter(f.User, "dashboard.uid", dashboards.ScopeFoldersPrefix, f.folderActions...)
-		builder.WriteString(folderFilter.Where)
-		builder.WriteString(" AND dashboard.is_folder)")
-		args = append(args, folderFilter.Args...)
+
+		actionsToCheck := make([]interface{}, 0, len(f.folderActions))
+		for _, action := range f.folderActions {
+			var hasWildcard bool
+			for _, scope := range f.user.Permissions[f.user.OrgID][action] {
+				if folderWildcards.Contains(scope) {
+					hasWildcard = true
+					break
+				}
+			}
+			if !hasWildcard {
+				actionsToCheck = append(actionsToCheck, action)
+			}
+		}
+
+		if len(actionsToCheck) > 0 {
+			builder.WriteString("(dashboard.uid IN (SELECT substr(scope, 13) FROM permission WHERE action IN (?" + strings.Repeat(", ?", len(actionsToCheck)-1) + ") AND scope LIKE 'folders:uid:%' " + rolesFilter + " GROUP BY role_id, scope HAVING COUNT(action) = ?) AND dashboard.is_folder)")
+			args = append(args, actionsToCheck...)
+			args = append(args, params...)
+			args = append(args, len(actionsToCheck))
+		} else {
+			builder.WriteString("dashboard.is_folder")
+		}
 	}
-	builder.WriteString(")")
+	builder.WriteRune(')')
 	return builder.String(), args
 }