Alerting: Change getEvaluatorForAlertRule to checkDatasourcePermissionsForRule (#46887)

update method getEvaluatorForAlertRule to accept permissions evaluator and exit on the first negative result, which is more effective than returning an evaluator that in fact is a bunch of slices.

pkg/services/ngalert/api/authorization.go

@@ -172,16 +172,17 @@ func (api *API) authorize(method, path string) web.Handler {
 	panic(fmt.Sprintf("no authorization handler for method [%s] of endpoint [%s]", method, path))
 }
 
-// GetDatasourceScopesFromAlertRule extracts data source scopes from an alert rule
+// authorizeDatasourceAccessForRule checks that user has access to all data sources declared by the rule
-func getEvaluatorForAlertRule(rule *ngmodels.AlertRule) ac.Evaluator {
+func authorizeDatasourceAccessForRule(rule *ngmodels.AlertRule, evaluator func(evaluator ac.Evaluator) bool) bool {
-	scopes := make([]ac.Evaluator, 0, len(rule.Data))
 	for _, query := range rule.Data {
 		if query.QueryType == expr.DatasourceType || query.DatasourceUID == expr.OldDatasourceUID {
 			continue
 		}
-		scopes = append(scopes, ac.EvalPermission(datasources.ActionQuery, dashboards.ScopeFoldersProvider.GetResourceScopeUID(query.DatasourceUID)))
+		if !evaluator(ac.EvalPermission(datasources.ActionQuery, dashboards.ScopeFoldersProvider.GetResourceScopeUID(query.DatasourceUID))) {
+			return false
+		}
 	}
-	return ac.EvalAll(scopes...)
+	return true
 }
 
 // authorizeRuleChanges analyzes changes in the rule group, determines what actions the user is trying to perform and check whether those actions are authorized.
@@ -203,7 +204,7 @@ func authorizeRuleChanges(namespace *models.Folder, changes *changes, evaluator
 			return fmt.Errorf("%w user cannot create alert rules in the folder %s", ErrAuthorization, namespace.Title)
 		}
 		for _, rule := range changes.New {
-			dsAllowed := evaluator(getEvaluatorForAlertRule(rule))
+			dsAllowed := authorizeDatasourceAccessForRule(rule, evaluator)
 			if !dsAllowed {
 				return fmt.Errorf("%w to create a new alert rule '%s' because the user does not have read permissions for one or many datasources the rule uses", ErrAuthorization, rule.Title)
 			}
@@ -211,7 +212,7 @@ func authorizeRuleChanges(namespace *models.Folder, changes *changes, evaluator
 	}
 
 	for _, rule := range changes.Update {
-		dsAllowed := evaluator(getEvaluatorForAlertRule(rule.New))
+		dsAllowed := authorizeDatasourceAccessForRule(rule.New, evaluator)
 		if !dsAllowed {
 			return fmt.Errorf("%w to update alert rule '%s' (UID: %s) because the user does not have read permissions for one or many datasources the rule uses", ErrAuthorization, rule.Existing.Title, rule.Existing.UID)
 		}

pkg/services/ngalert/api/authorization_test.go

@@ -228,37 +228,58 @@ func TestAuthorizeRuleChanges(t *testing.T) {
 	}
 }
 
-func TestGetEvaluatorForAlertRule(t *testing.T) {
+func TestCheckDatasourcePermissionsForRule(t *testing.T) {
-	t.Run("should not consider expressions", func(t *testing.T) {
+	rule := models.AlertRuleGen()()
-		rule := models.AlertRuleGen()()
 
-		expressionByType := models.GenerateAlertQuery()
+	expressionByType := models.GenerateAlertQuery()
-		expressionByType.QueryType = expr.DatasourceType
+	expressionByType.QueryType = expr.DatasourceType
-		expressionByUID := models.GenerateAlertQuery()
+	expressionByUID := models.GenerateAlertQuery()
-		expressionByUID.DatasourceUID = expr.OldDatasourceUID
+	expressionByUID.DatasourceUID = expr.OldDatasourceUID
 
-		var data []models.AlertQuery
+	var data []models.AlertQuery
-		var scopes []string
+	var scopes []string
-		for i := 0; i < rand.Intn(3)+2; i++ {
+	expectedExecutions := rand.Intn(3) + 2
-			q := models.GenerateAlertQuery()
+	for i := 0; i < expectedExecutions; i++ {
-			scopes = append(scopes, dashboards.ScopeFoldersProvider.GetResourceScopeUID(q.DatasourceUID))
+		q := models.GenerateAlertQuery()
-			data = append(data, q)
+		scopes = append(scopes, dashboards.ScopeFoldersProvider.GetResourceScopeUID(q.DatasourceUID))
+		data = append(data, q)
+	}
+
+	data = append(data, expressionByType, expressionByUID)
+	rand.Shuffle(len(data), func(i, j int) {
+		data[j], data[i] = data[i], data[j]
+	})
+
+	rule.Data = data
+
+	t.Run("should check only expressions", func(t *testing.T) {
+		permissions := map[string][]string{
+			datasources.ActionQuery: scopes,
 		}
 
-		data = append(data, expressionByType, expressionByUID)
+		executed := 0
-		rand.Shuffle(len(data), func(i, j int) {
+
-			data[j], data[i] = data[i], data[j]
+		eval := authorizeDatasourceAccessForRule(rule, func(evaluator ac.Evaluator) bool {
+			response, err := evaluator.Evaluate(permissions)
+			require.Truef(t, response, "provided permissions [%v] is not enough for requested permissions [%s]", permissions, evaluator.GoString())
+			require.NoError(t, err)
+			executed++
+			return true
 		})
 
-		rule.Data = data
+		require.True(t, eval)
+		require.Equal(t, expectedExecutions, executed)
+	})
 
-		eval := getEvaluatorForAlertRule(rule)
+	t.Run("should return on first negative evaluation", func(t *testing.T) {
+		executed := 0
 
-		allowed, err := eval.Evaluate(map[string][]string{
+		eval := authorizeDatasourceAccessForRule(rule, func(evaluator ac.Evaluator) bool {
-			datasources.ActionQuery: scopes,
+			executed++
+			return false
 		})
 
-		require.NoError(t, err)
+		require.False(t, eval)
-		require.True(t, allowed)
+		require.Equal(t, 1, executed)
 	})
 }