From acd7be1cb419cff29e31b4d6e822b8d64a436f58 Mon Sep 17 00:00:00 2001 From: Yuriy Tseretyan Date: Wed, 23 Mar 2022 13:11:30 -0400 Subject: [PATCH] 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 | 15 ++-- .../ngalert/api/authorization_test.go | 69 ++++++++++++------- 2 files changed, 53 insertions(+), 31 deletions(-) diff --git a/pkg/services/ngalert/api/authorization.go b/pkg/services/ngalert/api/authorization.go index eda3a104b06..5c77c457ec4 100644 --- a/pkg/services/ngalert/api/authorization.go +++ b/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 -func getEvaluatorForAlertRule(rule *ngmodels.AlertRule) ac.Evaluator { - scopes := make([]ac.Evaluator, 0, len(rule.Data)) +// authorizeDatasourceAccessForRule checks that user has access to all data sources declared by the rule +func authorizeDatasourceAccessForRule(rule *ngmodels.AlertRule, evaluator func(evaluator ac.Evaluator) bool) bool { 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) } diff --git a/pkg/services/ngalert/api/authorization_test.go b/pkg/services/ngalert/api/authorization_test.go index cc0b7bc342b..bc8b6ab1ac0 100644 --- a/pkg/services/ngalert/api/authorization_test.go +++ b/pkg/services/ngalert/api/authorization_test.go @@ -228,37 +228,58 @@ func TestAuthorizeRuleChanges(t *testing.T) { } } -func TestGetEvaluatorForAlertRule(t *testing.T) { - t.Run("should not consider expressions", func(t *testing.T) { - rule := models.AlertRuleGen()() - - expressionByType := models.GenerateAlertQuery() - expressionByType.QueryType = expr.DatasourceType - expressionByUID := models.GenerateAlertQuery() - expressionByUID.DatasourceUID = expr.OldDatasourceUID - - var data []models.AlertQuery - var scopes []string - for i := 0; i < rand.Intn(3)+2; i++ { - q := models.GenerateAlertQuery() - scopes = append(scopes, dashboards.ScopeFoldersProvider.GetResourceScopeUID(q.DatasourceUID)) - data = append(data, q) +func TestCheckDatasourcePermissionsForRule(t *testing.T) { + rule := models.AlertRuleGen()() + + expressionByType := models.GenerateAlertQuery() + expressionByType.QueryType = expr.DatasourceType + expressionByUID := models.GenerateAlertQuery() + expressionByUID.DatasourceUID = expr.OldDatasourceUID + + var data []models.AlertQuery + var scopes []string + expectedExecutions := rand.Intn(3) + 2 + for i := 0; i < expectedExecutions; i++ { + q := models.GenerateAlertQuery() + 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) - rand.Shuffle(len(data), func(i, j int) { - data[j], data[i] = data[i], data[j] + executed := 0 + + 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{ - datasources.ActionQuery: scopes, + eval := authorizeDatasourceAccessForRule(rule, func(evaluator ac.Evaluator) bool { + executed++ + return false }) - require.NoError(t, err) - require.True(t, allowed) + require.False(t, eval) + require.Equal(t, 1, executed) }) }